From b3b7ba507e45ef0777149f9b50fbd04da050f75b Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Thu, 2 Jul 2026 23:25:11 -0700 Subject: [PATCH] fix(signals): require a real shared file before path-driven PR collisions cluster A repo-wide shadow test of buildCollisionReport against the live open-PR queue found that folding changedFiles into the same bag-of-words scorer as titles/labels let generic directory-segment tokens (src, review, test, unit) drive false-positive collisions between unrelated PRs: 12 of 17 newly-flagged pairs shared zero actual files. Require an actual shared, non-lockfile file path when a match is driven by path terms alone, mirroring the existing same-author re-score guard. Re-validated: every remaining flagged pair is backed by a real shared file. Enables GITTENSORY_OPEN_PR_FILE_COLLISION now that the false-positive rate is resolved. --- src/signals/engine.ts | 37 +++++++++++----- test/unit/signals-v2.test.ts | 84 ++++++++++++++++++++++++++++++++++++ worker-configuration.d.ts | 4 +- wrangler.jsonc | 10 +++-- 4 files changed, 120 insertions(+), 15 deletions(-) diff --git a/src/signals/engine.ts b/src/signals/engine.ts index ba82fe815a..a41002e85d 100644 --- a/src/signals/engine.ts +++ b/src/signals/engine.ts @@ -33,6 +33,7 @@ import { PREFLIGHT_LIMITS } from "./preflight-limits"; import type { UnifiedCollapsible } from "../review/unified-comment"; import { splitAiReviewNits } from "../review/ai-notes"; import { GITTENSORY_GATE_CHECK_NAME } from "../review/check-names"; +import { diffFilePriority } from "../review/review-diff"; export type ParticipationLane = "direct_pr" | "issue_discovery" | "split" | "inactive" | "unknown"; export type SignalFinding = AdvisoryFinding; @@ -862,16 +863,23 @@ export function buildCollisionReport( } const overlap = termOverlap(itemTerms.get(itemKey(left)) ?? collisionTerms(left), itemTerms.get(itemKey(right)) ?? collisionTerms(right)); if (overlap.score < 0.58 || overlap.shared < 2) continue; - // A contributor iterating on their own work (e.g. a follow-up PR touching the same file as their still-open - // prior PR) is not duplicate effort. Title/label overlap between a contributor's own items is today's - // established behavior (unchanged, e.g. a self-filed issue and its own PR); what's new here is that - // `changedFiles` now also feeds this same heuristic, and two of a contributor's own PRs sharing a file is - // exactly the false-positive path-overlap creates. Re-score without paths: if the pair only clears the bar - // WITH file-path terms, paths alone drove the match — self-authored, so skip it. If title/label terms alone - // already clear the bar, this is pre-existing behavior and still clusters. - if (isPullRequestShapedItem(left) && isPullRequestShapedItem(right) && Boolean(left.authorLogin) && sameLogin(left.authorLogin, right.authorLogin ?? "")) { - const titleOnlyOverlap = termOverlap(collisionTerms(left, false), collisionTerms(right, false)); - if (titleOnlyOverlap.score < 0.58 || titleOnlyOverlap.shared < 2) continue; + // Re-score without path terms: tells us whether title/label overlap ALONE already clears the bar + // (pre-existing behavior, unaffected) or whether changedFiles tokens are what pushed this pair over — + // the two false-positive shapes that creates are guarded separately below. + const titleOnlyOverlap = termOverlap(collisionTerms(left, false), collisionTerms(right, false)); + const pathDrivenMatch = titleOnlyOverlap.score < 0.58 || titleOnlyOverlap.shared < 2; + if (pathDrivenMatch) { + // A contributor iterating on their own work (e.g. a follow-up PR touching the same file as their + // still-open prior PR) is not duplicate effort — self-authored path-only overlap is dropped outright. + if (isPullRequestShapedItem(left) && isPullRequestShapedItem(right) && Boolean(left.authorLogin) && sameLogin(left.authorLogin, right.authorLogin ?? "")) { + continue; + } + // Different authors: file paths tokenize into directory segments (src, review, test, unit, ...) that + // recur across nearly every PR in a consistently-organized repo, so shared TOKENS alone are not + // reliable collision evidence — a repo-wide shadow test found this drove the large majority of + // path-only matches with zero actual shared files. Require an ACTUAL shared file (ignoring + // lockfiles/generated artifacts nobody would call a collision over) before clustering. + if (!sharesMeaningfulFile(left.changedFiles, right.changedFiles)) continue; } const key = [itemKey(left), itemKey(right)].sort().join("--"); if (clusters.has(key)) continue; @@ -5406,6 +5414,15 @@ function isPullRequestShapedItem(item: CollisionItem): boolean { return item.type === "pull_request" || item.type === "recent_merged_pull_request"; } +/** True when two changed-file lists share at least one path that isn't a lockfile/generated/vendor artifact + * (diffFilePriority's least-useful-to-review bucket) — a shared package-lock.json or dist/ output is touched + * incidentally by unrelated PRs and is not evidence of a real collision. */ +function sharesMeaningfulFile(left: string[] | undefined, right: string[] | undefined): boolean { + if (!left || !right || left.length === 0 || right.length === 0) return false; + const rightSet = new Set(right); + return left.some((path) => rightSet.has(path) && diffFilePriority(path) < 4); +} + function sameRepo(left: string | null | undefined, right: string | null | undefined): boolean { return Boolean(left && right && left.toLowerCase() === right.toLowerCase()); } diff --git a/test/unit/signals-v2.test.ts b/test/unit/signals-v2.test.ts index 4dd795d95f..ed0926c200 100644 --- a/test/unit/signals-v2.test.ts +++ b/test/unit/signals-v2.test.ts @@ -257,6 +257,90 @@ describe("v2 signal builders", () => { expect(cluster).toBeDefined(); }); + it("does not flag two open PRs from different authors whose only overlap is generic directory-segment tokens, not a real shared file", () => { + // Both touch src/review/*.ts + test/unit/*.ts, so "src"/"review"/"test"/"unit" tokenize as shared terms + // even though the two PRs share zero actual file paths — a repo-wide shadow test found this drove most + // path-only false positives once changedFiles fed the same bag-of-words scorer as titles/labels. + const ivanPr: PullRequestRecord = { + repoFullName: repo.fullName, + number: 223, + title: "CLA check", + state: "open", + authorLogin: "ivan", + authorAssociation: "NONE", + labels: [], + linkedIssues: [], + changedFiles: ["src/review/cla-check.ts", "test/unit/cla-check.test.ts"], + }; + const judyPr: PullRequestRecord = { + repoFullName: repo.fullName, + number: 224, + title: "RAG builder", + state: "open", + authorLogin: "judy", + authorAssociation: "NONE", + labels: [], + linkedIssues: [], + changedFiles: ["src/review/rag.ts", "test/unit/rag.test.ts"], + }; + const report = buildCollisionReport(repo.fullName, [], [ivanPr, judyPr]); + expect(findCluster(report, 223, 224)).toBeUndefined(); + }); + + it("does not flag two open PRs whose only shared file is a lockfile (touched incidentally, not real overlap)", () => { + const kenPr: PullRequestRecord = { + repoFullName: repo.fullName, + number: 225, + title: "Rotate secrets", + state: "open", + authorLogin: "ken", + authorAssociation: "NONE", + labels: [], + linkedIssues: [], + changedFiles: ["package-lock.json"], + }; + const lisaPr: PullRequestRecord = { + repoFullName: repo.fullName, + number: 226, + title: "Trim caches", + state: "open", + authorLogin: "lisa", + authorAssociation: "NONE", + labels: [], + linkedIssues: [], + changedFiles: ["package-lock.json"], + }; + const report = buildCollisionReport(repo.fullName, [], [kenPr, lisaPr]); + expect(findCluster(report, 225, 226)).toBeUndefined(); + }); + + it("flags two open PRs from different authors once a real (non-lockfile) shared file joins otherwise-generic path tokens", () => { + const ivanPr: PullRequestRecord = { + repoFullName: repo.fullName, + number: 227, + title: "CLA check", + state: "open", + authorLogin: "ivan", + authorAssociation: "NONE", + labels: [], + linkedIssues: [], + changedFiles: ["src/review/cla-check.ts", "test/unit/cla-check.test.ts", "src/shared/helper.ts"], + }; + const judyPr: PullRequestRecord = { + repoFullName: repo.fullName, + number: 228, + title: "RAG builder", + state: "open", + authorLogin: "judy", + authorAssociation: "NONE", + labels: [], + linkedIssues: [], + changedFiles: ["src/review/rag.ts", "test/unit/rag.test.ts", "src/shared/helper.ts"], + }; + const report = buildCollisionReport(repo.fullName, [], [ivanPr, judyPr]); + expect(findCluster(report, 227, 228)).toBeDefined(); + }); + it("flags an open PR against a recently-merged PR from a different author sharing a file (extends to merged history)", () => { const openPr: PullRequestRecord = { repoFullName: repo.fullName, diff --git a/worker-configuration.d.ts b/worker-configuration.d.ts index 5a2686f78b..7ae8f77db1 100644 --- a/worker-configuration.d.ts +++ b/worker-configuration.d.ts @@ -1,5 +1,5 @@ /* eslint-disable */ -// Generated by Wrangler by running `wrangler types` (hash: ed445a7a260a431e4c5988e63c82a35a) +// Generated by Wrangler by running `wrangler types` (hash: 11efd919605a2b914db02ea113814228) // Runtime types generated with workerd@1.20260617.1 2026-05-28 nodejs_compat interface __BaseEnv_Env { DB: D1Database; @@ -35,7 +35,7 @@ interface __BaseEnv_Env { GITTENSORY_PUBLIC_STATS: "true"; GITTENSORY_PUBLIC_STATS_REPOS: "JSONbored/gittensory,JSONbored/awesome-claude,JSONbored/metagraphed"; GITTENSORY_DUPLICATE_WINNER: "true"; - GITTENSORY_OPEN_PR_FILE_COLLISION: "false"; + GITTENSORY_OPEN_PR_FILE_COLLISION: "true"; RATE_LIMITER: DurableObjectNamespace; } declare namespace Cloudflare { diff --git a/wrangler.jsonc b/wrangler.jsonc index de64d4fc22..df1a5468df 100644 --- a/wrangler.jsonc +++ b/wrangler.jsonc @@ -139,9 +139,13 @@ "GITTENSORY_DUPLICATE_WINNER": "true", // Open-PR file-path collision (#2653): enrich changedFiles on the reviewed PR and its open siblings from // the pull_request_files cache before building the collision report, so two independently-open PRs on the - // same file get flagged the way two title-similar PRs already are. Default OFF — unset/false leaves every - // PullRequestRecord's changedFiles unset (byte-identical to today, no extra D1 reads). - "GITTENSORY_OPEN_PR_FILE_COLLISION": "false", + // same file get flagged the way two title-similar PRs already are. ENABLED: a repo-wide shadow test against + // the live open-PR queue found path-only term overlap alone drove 12 of 17 newly-flagged pairs to false + // positives (shared generic directory-segment tokens like src/review/test/unit, zero actual shared files) — + // fixed by requiring an actual shared, non-lockfile file path before a path-driven match clusters (see the + // sharesMeaningfulFile guard in buildCollisionReport). Re-validated post-fix: every remaining flagged pair is + // backed by a real shared file. + "GITTENSORY_OPEN_PR_FILE_COLLISION": "true", }, "routes": [ {