From 13cec5157d2088f0c10610f1d7e497c9a00996c5 Mon Sep 17 00:00:00 2001 From: RealDiligent Date: Sun, 26 Jul 2026 07:50:16 +0800 Subject: [PATCH] fix(review): delegate merge-train low-signal classification to canonical path-matchers src/review/merge-train.ts hand-rolled its own low-signal file/dir detection that had drifted from the canonical set its sibling files already use: - LOW_SIGNAL_FILENAME_RE matched only package-lock.json/yarn.lock/pnpm-lock.yaml/Cargo.lock (4 names), so two PRs sharing only a poetry.lock, go.sum, npm-shrinkwrap.json, etc. were treated as a real overlap and forced into a spurious merge-train wait. - LOW_SIGNAL_DIR_RE matched only dist/build/coverage/node_modules, missing 'out' and the vendored-code family (vendor/vendored/third_party/bower_components/jspm_packages). Replace the filename regex with the canonical isLockfile() helper from path-matchers.ts (the same dependency-free leaf utility review-diff.ts and review-grounding.ts already delegate to, covering all 24+ lockfile formats), and extend the directory regex to the canonical generated/vendored set. Two new tests prove PRs sharing only poetry.lock/go.sum or vendor/third_party paths now decide {wait:false} where they previously decided {wait:true}. --- src/review/merge-train.ts | 22 +++++++++++++--------- test/unit/merge-train.test.ts | 12 ++++++++++++ 2 files changed, 25 insertions(+), 9 deletions(-) diff --git a/src/review/merge-train.ts b/src/review/merge-train.ts index d96cb13819..7d24551063 100644 --- a/src/review/merge-train.ts +++ b/src/review/merge-train.ts @@ -19,6 +19,8 @@ // acceptable, bounded tradeoff given the 24h staleness cap already prevents an abandoned PR from blocking // forever. +import { isLockfile } from "../signals/path-matchers"; + /** The subset of a sibling PR's fields this gate actually needs -- kept minimal and independent of * `PullRequestRecord`'s full shape so this module has zero import surface beyond plain data. */ export type MergeTrainSibling = { @@ -43,17 +45,19 @@ export const MERGE_TRAIN_MAX_WAIT_MS = 24 * 60 * 60 * 1000; export type MergeTrainDecision = { wait: true; blockingPr: number } | { wait: false }; -/** Low-priority path buckets (lockfiles, generated/build output, dist/ artifacts) that overlapping alone - * never counts as real conflict risk -- ported from `review-grounding.ts`'s `diffFilePriority` classification - * (bucket 4, "least useful to review") so this module stays dependency-free rather than importing a whole - * review-pipeline module for one number. Kept as a small, explicit suffix/name list rather than a generic - * "some overlap" check: a shared `package-lock.json` or `dist/bundle.js` touch is routine noise, not the - * same-area conflict risk this gate exists to catch. */ -const LOW_SIGNAL_FILENAME_RE = /(?:^|\/)(?:package-lock\.json|yarn\.lock|pnpm-lock\.yaml|Cargo\.lock)$/i; -const LOW_SIGNAL_DIR_RE = /(?:^|\/)(?:dist|build|coverage|node_modules)\//i; +/** Low-priority path buckets (lockfiles, generated/build output, vendored third-party trees) that overlapping + * alone never counts as real conflict risk. Lockfile-NAME matching delegates to the canonical `isLockfile` + * helper -- a dependency-free leaf utility in path-matchers.ts, the same delegation `review-diff.ts` and + * `review-grounding.ts` already use -- so this classification never drifts behind the 24+ canonical lockfile + * formats the way the hand-rolled 4-name regex did (a shared `poetry.lock`, `go.sum`, or `npm-shrinkwrap.json` + * was wrongly treated as real overlap; #8647). The directory bucket mirrors the same canonical generated/ + * vendored set: build output (`dist`/`build`/`out`/`coverage`) and installed/vendored dependency trees + * (`node_modules` and the `vendor`/`third_party` family). A shared `package-lock.json` or `third_party/x` + * touch is routine noise, not the same-area conflict risk this gate exists to catch. */ +const LOW_SIGNAL_DIR_RE = /(?:^|\/)(?:dist|build|out|coverage|node_modules|vendor|vendored|third_party|third-party|bower_components|jspm_packages)\//i; function isMeaningfulPath(path: string): boolean { - return !LOW_SIGNAL_FILENAME_RE.test(path) && !LOW_SIGNAL_DIR_RE.test(path); + return !isLockfile(path) && !LOW_SIGNAL_DIR_RE.test(path); } /** True when `thisPr` and `sibling` overlap enough to carry real conflict/duplicate-effort risk: a shared diff --git a/test/unit/merge-train.test.ts b/test/unit/merge-train.test.ts index d7db0719af..57ba0c6b02 100644 --- a/test/unit/merge-train.test.ts +++ b/test/unit/merge-train.test.ts @@ -132,6 +132,18 @@ describe("shouldWaitForOlderSiblings (#selfhost-merge-train)", () => { expect(decide(110, "2026-07-07T11:00:00.000Z", siblings, NOW, { thisPrLinkedIssues: [1], thisPrChangedFiles: ["package-lock.json", "dist/bundle.js"] })).toEqual({ wait: false }); }); + it("does NOT treat a shared non-npm canonical lockfile (poetry.lock/go.sum) as meaningful overlap (#8647)", () => { + // The hand-rolled 4-name regex only knew package-lock/yarn/pnpm/Cargo, so these forced a spurious wait; + // delegating to the canonical isLockfile covers all 24+ formats. + const siblings: MergeTrainSibling[] = [{ number: 105, createdAt: "2026-07-07T10:00:00.000Z", linkedIssues: [99], changedFiles: ["poetry.lock", "backend/go.sum"] }]; + expect(decide(110, "2026-07-07T11:00:00.000Z", siblings, NOW, { thisPrLinkedIssues: [1], thisPrChangedFiles: ["poetry.lock", "backend/go.sum"] })).toEqual({ wait: false }); + }); + + it("does NOT treat a shared vendored-directory path (vendor/third_party) as meaningful overlap (#8647)", () => { + const siblings: MergeTrainSibling[] = [{ number: 105, createdAt: "2026-07-07T10:00:00.000Z", linkedIssues: [99], changedFiles: ["vendor/lib/x.go", "third_party/pkg/y.js"] }]; + expect(decide(110, "2026-07-07T11:00:00.000Z", siblings, NOW, { thisPrLinkedIssues: [1], thisPrChangedFiles: ["vendor/lib/x.go", "third_party/pkg/y.js"] })).toEqual({ wait: false }); + }); + it("a sibling with no linkedIssues field at all (undefined) can still match via a shared changed file", () => { const siblings: MergeTrainSibling[] = [{ number: 105, createdAt: "2026-07-07T10:00:00.000Z", changedFiles: ["src/a.ts"] }]; expect(decide(110, "2026-07-07T11:00:00.000Z", siblings, NOW, { thisPrLinkedIssues: [1], thisPrChangedFiles: ["src/a.ts"] })).toEqual({ wait: true, blockingPr: 105 });