diff --git a/packages/gittensory-engine/src/duplicate-winner.ts b/packages/gittensory-engine/src/duplicate-winner.ts index 48bfaa689e..0edb0ffef5 100644 --- a/packages/gittensory-engine/src/duplicate-winner.ts +++ b/packages/gittensory-engine/src/duplicate-winner.ts @@ -11,14 +11,11 @@ * caller can compute the winner ONCE per review run and thread the result boolean consistently into every * surface (advisory finding, close reason, slop, panels), so they agree by construction. * - * ELECTION ORDER (#dup-winner true-creation-time): prefer each PR's true GitHub `pull_request.created_at` — - * the real order contributors opened their PRs in — over `linkedIssueClaimedAt` (gittensory's own sync-time, - * i.e. whenever a webhook/sweep/backfill pass happened to OBSERVE the linked issue). Sync order and creation - * order diverge whenever processing isn't strictly FIFO (a stalled sweep catching up on a backlog, backfill - * reordering, webhook delivery delay), under the old claim-time-only rule, that divergence could crown a - * LATER contributor the winner and close the PR of whoever actually opened first. `createdAt` is compared - * only when BOTH sides of a given comparison have a valid one; otherwise this falls back to the legacy - * claim-time comparison unchanged, so sparse/legacy rows keep their existing fail-closed behavior exactly. + * ELECTION ORDER: compare `linkedIssueClaimedAt`, the time gittensory first observed the PR claiming + * the issue. GitHub `pull_request.created_at` is intentionally not an ordering signal here: contributors can + * edit an old placeholder PR to add a linked issue later, so creation time would let backdated claims steal + * duplicate-winner credit from the PR that actually claimed the issue first. Sparse legacy rows that lack + * claim timing keep failing closed so unknown ordering cannot suppress duplicate evidence. * * INVARIANT (the caller MUST honor it): {@link openSiblingNumbers} carries OPEN-only sibling PR numbers. The * existing sources already exclude closed/merged PRs. Once the winner closes (e.g. red CI), it leaves the open @@ -34,7 +31,7 @@ export type DuplicateClaimMember = { number: number; linkedIssueClaimedAt?: string | null | undefined; - /** GitHub's true PR creation time. See the module doc's "ELECTION ORDER" note. */ + /** GitHub's true PR creation time. Retained for caller compatibility; not used for winner ordering. */ createdAt?: string | null | undefined; }; @@ -55,8 +52,7 @@ export function isDuplicateClusterWinner(prNumber: number, openSiblingNumbers: n /** * True iff `pr` is the earliest-elected claimant in the open duplicate cluster (see the module doc's - * "ELECTION ORDER" note for the createdAt-vs-claim-time precedence). Sparse legacy rows fail closed; ties - * between equally-ordered members use PR number. + * "ELECTION ORDER" note). Sparse legacy rows fail closed; ties between equally-ordered members use PR number. */ export function isDuplicateClusterWinnerByClaim(pr: DuplicateClaimMember, openSiblings: DuplicateClaimMember[]): boolean { if (openSiblings.length === 0) return true; @@ -67,18 +63,11 @@ export function isDuplicateClusterWinnerByClaim(pr: DuplicateClaimMember, openSi } /** - * True iff `pr` is ordered at or ahead of `sibling` for cluster-winner purposes. Prefers `createdAt` when BOTH - * sides have a valid one (the true creation-time order); otherwise falls back to the legacy `linkedIssueClaimedAt` - * comparison unchanged (including its fail-closed-on-missing/invalid-timestamp behavior), so a mixed - * legacy/modern cluster never silently guesses using two different clocks for the two sides of one comparison. + * True iff `pr` is ordered at or ahead of `sibling` for cluster-winner purposes. Only the observed linked-issue + * claim time participates in the election; `createdAt` is deliberately ignored because an older PR can claim a + * linked issue later by editing its body. */ function prPrecedesSibling(pr: DuplicateClaimMember, sibling: DuplicateClaimMember): boolean { - const prCreated = claimTimeMs(pr.createdAt); - const siblingCreated = claimTimeMs(sibling.createdAt); - if (prCreated !== null && siblingCreated !== null) { - if (prCreated !== siblingCreated) return prCreated < siblingCreated; - return pr.number <= sibling.number; - } const prClaim = claimTimeMs(pr.linkedIssueClaimedAt); if (prClaim === null) return false; const siblingClaim = claimTimeMs(sibling.linkedIssueClaimedAt); diff --git a/packages/gittensory-engine/src/signals/duplicate-winner.ts b/packages/gittensory-engine/src/signals/duplicate-winner.ts index 9de8f9f496..0edb0ffef5 100644 --- a/packages/gittensory-engine/src/signals/duplicate-winner.ts +++ b/packages/gittensory-engine/src/signals/duplicate-winner.ts @@ -11,24 +11,27 @@ * caller can compute the winner ONCE per review run and thread the result boolean consistently into every * surface (advisory finding, close reason, slop, panels), so they agree by construction. * - * ELECTION ORDER (#dup-winner true-creation-time): prefer each PR's true GitHub `pull_request.created_at` — - * the real order contributors opened their PRs in — over `linkedIssueClaimedAt` (gittensory's own sync-time, - * i.e. whenever a webhook/sweep/backfill pass happened to OBSERVE the linked issue). Sync order and creation - * order diverge whenever processing isn't strictly FIFO (a stalled sweep catching up on a backlog, backfill - * reordering, webhook delivery delay), under the old claim-time-only rule, that divergence could crown a - * LATER contributor the winner and close the PR of whoever actually opened first. `createdAt` is compared - * only when BOTH sides of a given comparison have a valid one; otherwise this falls back to the legacy - * claim-time comparison unchanged, so sparse/legacy rows keep their existing fail-closed behavior exactly. + * ELECTION ORDER: compare `linkedIssueClaimedAt`, the time gittensory first observed the PR claiming + * the issue. GitHub `pull_request.created_at` is intentionally not an ordering signal here: contributors can + * edit an old placeholder PR to add a linked issue later, so creation time would let backdated claims steal + * duplicate-winner credit from the PR that actually claimed the issue first. Sparse legacy rows that lack + * claim timing keep failing closed so unknown ordering cannot suppress duplicate evidence. * * INVARIANT (the caller MUST honor it): {@link openSiblingNumbers} carries OPEN-only sibling PR numbers. The * existing sources already exclude closed/merged PRs. Once the winner closes (e.g. red CI), it leaves the open * set and the next-earliest OPEN claimant becomes the winner on re-eval — no permanently-orphaned cluster. + * + * SECOND CONSUMER (#2278): this module is intentionally engine-hosted (not `src/`-only) because its election + * logic is reusable for the miner's own soft-claim adjudication — deciding which of several miners claiming + * the same issue proceeds. A future contributor wiring the miner's local claim ledger should import this + * module rather than reimplementing the election rule, so both the maintainer gate and the miner agree on + * exactly one winner by construction. */ export type DuplicateClaimMember = { number: number; linkedIssueClaimedAt?: string | null | undefined; - /** GitHub's true PR creation time. See the module doc's "ELECTION ORDER" note. */ + /** GitHub's true PR creation time. Retained for caller compatibility; not used for winner ordering. */ createdAt?: string | null | undefined; }; @@ -49,8 +52,7 @@ export function isDuplicateClusterWinner(prNumber: number, openSiblingNumbers: n /** * True iff `pr` is the earliest-elected claimant in the open duplicate cluster (see the module doc's - * "ELECTION ORDER" note for the createdAt-vs-claim-time precedence). Sparse legacy rows fail closed; ties - * between equally-ordered members use PR number. + * "ELECTION ORDER" note). Sparse legacy rows fail closed; ties between equally-ordered members use PR number. */ export function isDuplicateClusterWinnerByClaim(pr: DuplicateClaimMember, openSiblings: DuplicateClaimMember[]): boolean { if (openSiblings.length === 0) return true; @@ -61,18 +63,11 @@ export function isDuplicateClusterWinnerByClaim(pr: DuplicateClaimMember, openSi } /** - * True iff `pr` is ordered at or ahead of `sibling` for cluster-winner purposes. Prefers `createdAt` when BOTH - * sides have a valid one (the true creation-time order); otherwise falls back to the legacy `linkedIssueClaimedAt` - * comparison unchanged (including its fail-closed-on-missing/invalid-timestamp behavior), so a mixed - * legacy/modern cluster never silently guesses using two different clocks for the two sides of one comparison. + * True iff `pr` is ordered at or ahead of `sibling` for cluster-winner purposes. Only the observed linked-issue + * claim time participates in the election; `createdAt` is deliberately ignored because an older PR can claim a + * linked issue later by editing its body. */ function prPrecedesSibling(pr: DuplicateClaimMember, sibling: DuplicateClaimMember): boolean { - const prCreated = claimTimeMs(pr.createdAt); - const siblingCreated = claimTimeMs(sibling.createdAt); - if (prCreated !== null && siblingCreated !== null) { - if (prCreated !== siblingCreated) return prCreated < siblingCreated; - return pr.number <= sibling.number; - } const prClaim = claimTimeMs(pr.linkedIssueClaimedAt); if (prClaim === null) return false; const siblingClaim = claimTimeMs(sibling.linkedIssueClaimedAt); diff --git a/src/signals/duplicate-winner.ts b/src/signals/duplicate-winner.ts index c7980fe396..65b8aa7676 100644 --- a/src/signals/duplicate-winner.ts +++ b/src/signals/duplicate-winner.ts @@ -2,7 +2,7 @@ * Duplicate-winner adjudication (#dup-winner), extracted to `@jsonbored/gittensory-engine` (#2278) so the * maintainer gate and the miner's own soft-claim adjudication (a later Phase-0 issue) import the identical, * versioned election logic instead of drifting apart. See the engine module's doc comment for the full - * election-order rationale (createdAt-vs-claim-time precedence, fail-closed semantics). + * election-order rationale (claim-time election, anti-backdating semantics). * * packages/gittensory-engine/src/duplicate-winner.ts (imported via relative source path, not the published * module, matching the #2282 scoring-preview extraction) is the source of truth. diff --git a/test/unit/duplicate-winner.test.ts b/test/unit/duplicate-winner.test.ts index 452e786d05..5936988d7a 100644 --- a/test/unit/duplicate-winner.test.ts +++ b/test/unit/duplicate-winner.test.ts @@ -92,76 +92,50 @@ describe("isDuplicateClusterWinnerByClaim (#dup-winner claim election)", () => { }); }); -describe("isDuplicateClusterWinnerByClaim createdAt precedence (#dup-winner true-creation-time)", () => { +describe("isDuplicateClusterWinnerByClaim claim-time election with createdAt present (#dup-winner anti-backdating)", () => { const member = (number: number, createdAt: string | null, linkedIssueClaimedAt: string | null) => ({ number, createdAt, linkedIssueClaimedAt }); - it("REGRESSION: elects the PR that GitHub says opened first, even when gittensory OBSERVED (claimed) the later-opened sibling first", () => { - // PR 13 truly opened first (10:00) but gittensory's stalled sweep only got around to syncing/claiming it at - // 11:00. PR 14 opened later (10:05) but was claimed immediately (10:06) because the sweep happened to reach - // it first. Under the old claim-time-only rule, 14 would wrongly win and 13 (the real first mover) would be - // closed as the "duplicate." createdAt must override that. + it("REGRESSION: does not let an older placeholder PR steal winner credit by adding the issue later", () => { + // PR 12 was opened first but only edited in the linked issue after PR 13 had already claimed it. The + // anti-backdating signal is linkedIssueClaimedAt, so createdAt must not override the actual claim order. expect( isDuplicateClusterWinnerByClaim( - member(13, "2026-06-29T10:00:00.000Z", "2026-06-29T11:00:00.000Z"), - [member(14, "2026-06-29T10:05:00.000Z", "2026-06-29T10:06:00.000Z")], - ), - ).toBe(true); - // And symmetrically, the later-created PR no longer wins just because it was claimed first. - expect( - isDuplicateClusterWinnerByClaim( - member(14, "2026-06-29T10:05:00.000Z", "2026-06-29T10:06:00.000Z"), - [member(13, "2026-06-29T10:00:00.000Z", "2026-06-29T11:00:00.000Z")], + member(12, "2026-06-29T09:00:00.000Z", "2026-06-29T10:05:00.000Z"), + [member(13, "2026-06-29T09:30:00.000Z", "2026-06-29T10:00:00.000Z")], ), ).toBe(false); - }); - - it("falls back to claim-time comparison when only ONE side has a valid createdAt (mixed legacy/modern cluster)", () => { - // pr has createdAt; sibling (a legacy row) does not — never mix clocks across the two sides of one - // comparison. pr's claim (10:00) is earlier than sibling's claim (10:05) ⇒ pr still wins via the fallback. expect( isDuplicateClusterWinnerByClaim( - { number: 12, createdAt: "2026-06-29T09:00:00.000Z", linkedIssueClaimedAt: "2026-06-29T10:00:00.000Z" }, - [{ number: 13, createdAt: null, linkedIssueClaimedAt: "2026-06-29T10:05:00.000Z" }], + member(13, "2026-06-29T09:30:00.000Z", "2026-06-29T10:00:00.000Z"), + [member(12, "2026-06-29T09:00:00.000Z", "2026-06-29T10:05:00.000Z")], ), ).toBe(true); - // Same mixed case, but pr's own claim is later than the sibling's ⇒ pr loses via the fallback. - expect( - isDuplicateClusterWinnerByClaim( - { number: 12, createdAt: "2026-06-29T09:00:00.000Z", linkedIssueClaimedAt: "2026-06-29T10:05:00.000Z" }, - [{ number: 13, createdAt: null, linkedIssueClaimedAt: "2026-06-29T10:00:00.000Z" }], - ), - ).toBe(false); }); - it("falls back to claim-time comparison when a createdAt value is present but unparseable", () => { + it("ignores createdAt even when both sides have valid values", () => { expect( isDuplicateClusterWinnerByClaim( - member(12, "not-a-date", "2026-06-29T10:00:00.000Z"), - [member(13, "2026-06-29T09:00:00.000Z", "2026-06-29T10:05:00.000Z")], + member(14, "2026-06-29T10:05:00.000Z", "2026-06-29T10:00:00.000Z"), + [member(13, "2026-06-29T10:00:00.000Z", "2026-06-29T11:00:00.000Z")], ), ).toBe(true); }); - it("tie-breaks equal createdAt values by PR number, mirroring the claim-time tie-break", () => { - expect(isDuplicateClusterWinnerByClaim(member(12, "2026-06-29T10:00:00.000Z", null), [member(13, "2026-06-29T10:00:00.000Z", null)])).toBe(true); - expect(isDuplicateClusterWinnerByClaim(member(13, "2026-06-29T10:00:00.000Z", null), [member(12, "2026-06-29T10:00:00.000Z", null)])).toBe(false); - }); - - it("createdAt-based cases are unaffected by (and do not require) a claim timestamp at all", () => { - expect(isDuplicateClusterWinnerByClaim(member(12, "2026-06-29T10:00:00.000Z", null), [member(13, "2026-06-29T10:05:00.000Z", null)])).toBe(true); + it("still fails closed when createdAt is present but claim timing is missing", () => { + expect(isDuplicateClusterWinnerByClaim(member(12, "2026-06-29T10:00:00.000Z", null), [member(13, "2026-06-29T10:05:00.000Z", null)])).toBe(false); }); }); describe("resolveDuplicateClusterWinnerNumber (#dup-winner-credit)", () => { it("returns this PR's own number when it is the winner", () => { - expect(resolveDuplicateClusterWinnerNumber({ number: 12, createdAt: "2026-06-29T10:00:00.000Z" }, [{ number: 13, createdAt: "2026-06-29T10:05:00.000Z" }])).toBe(12); + expect(resolveDuplicateClusterWinnerNumber({ number: 12, linkedIssueClaimedAt: "2026-06-29T10:00:00.000Z" }, [{ number: 13, linkedIssueClaimedAt: "2026-06-29T10:05:00.000Z" }])).toBe(12); }); it("returns the actual winning sibling's number when this PR is a loser, even with multiple siblings", () => { expect( - resolveDuplicateClusterWinnerNumber({ number: 14, createdAt: "2026-06-29T10:10:00.000Z" }, [ - { number: 13, createdAt: "2026-06-29T10:00:00.000Z" }, - { number: 15, createdAt: "2026-06-29T10:05:00.000Z" }, + resolveDuplicateClusterWinnerNumber({ number: 14, linkedIssueClaimedAt: "2026-06-29T10:10:00.000Z" }, [ + { number: 13, linkedIssueClaimedAt: "2026-06-29T10:00:00.000Z" }, + { number: 15, linkedIssueClaimedAt: "2026-06-29T10:05:00.000Z" }, ]), ).toBe(13); }); @@ -223,9 +197,9 @@ describe("dupWinnerLinkedDuplicateCount (#dup-winner close-reason seam)", () => expect(dupWinnerLinkedDuplicateCount([], 12, "2026-06-29T10:00:00.000Z", false)).toBe(0); }); - it("REGRESSION (#dup-winner true-creation-time): createdAt overrides a claim-time-only verdict when passed through", () => { - // By claim time alone this PR (12) would lose to sibling 13 (claimed earlier, 10:00 vs 10:05). But 12's true - // createdAt (09:00) precedes 13's (09:30), so passing createdAt flips the verdict to a win (count 0). + it("REGRESSION (#dup-winner anti-backdating): createdAt does not override claim-time ordering when passed through", () => { + // PR 12 is older, but sibling 13 claimed the linked issue first; passing createdAt must not suppress the + // duplicate count for the later claimant. expect( dupWinnerLinkedDuplicateCount( [{ number: 13, linkedIssueClaimedAt: "2026-06-29T10:00:00.000Z", createdAt: "2026-06-29T09:30:00.000Z" }], @@ -234,21 +208,21 @@ describe("dupWinnerLinkedDuplicateCount (#dup-winner close-reason seam)", () => true, "2026-06-29T09:00:00.000Z", ), - ).toBe(0); + ).toBe(1); }); }); describe("dupWinnerLinkedDuplicateWinnerNumber (#dup-winner-credit close-reason naming seam)", () => { it("flag OFF ⇒ null regardless of who would win (generic wording, byte-identical to before this existed)", () => { - expect(dupWinnerLinkedDuplicateWinnerNumber([{ number: 13, createdAt: "2026-06-29T10:05:00.000Z" }], 12, undefined, false, "2026-06-29T10:00:00.000Z")).toBeNull(); + expect(dupWinnerLinkedDuplicateWinnerNumber([{ number: 13, linkedIssueClaimedAt: "2026-06-29T10:05:00.000Z" }], 12, "2026-06-29T10:00:00.000Z", false, "2026-06-29T10:00:00.000Z")).toBeNull(); }); it("winner + flag ON ⇒ null (nothing to name — its own close reason omits the duplicate cause entirely)", () => { - expect(dupWinnerLinkedDuplicateWinnerNumber([{ number: 13, createdAt: "2026-06-29T10:05:00.000Z" }], 12, undefined, true, "2026-06-29T10:00:00.000Z")).toBeNull(); + expect(dupWinnerLinkedDuplicateWinnerNumber([{ number: 13, linkedIssueClaimedAt: "2026-06-29T10:05:00.000Z" }], 12, "2026-06-29T10:00:00.000Z", true, "2026-06-29T10:00:00.000Z")).toBeNull(); }); it("loser + flag ON ⇒ the actual winning sibling's number", () => { - expect(dupWinnerLinkedDuplicateWinnerNumber([{ number: 12, createdAt: "2026-06-29T10:00:00.000Z" }], 14, undefined, true, "2026-06-29T10:10:00.000Z")).toBe(12); + expect(dupWinnerLinkedDuplicateWinnerNumber([{ number: 12, linkedIssueClaimedAt: "2026-06-29T10:00:00.000Z" }], 14, "2026-06-29T10:10:00.000Z", true, "2026-06-29T10:10:00.000Z")).toBe(12); }); it("loser + flag ON, but the election is too ambiguous ⇒ null (falls back to generic wording)", () => { @@ -305,7 +279,7 @@ describe("listOtherOpenPullRequests ordering (#audit-3.9)", () => { }); }); -describe("upsertPullRequestFromGitHub createdAt threading (#dup-winner true-creation-time)", () => { +describe("upsertPullRequestFromGitHub createdAt threading", () => { it("populates createdAt from GitHub's true pull_request.created_at on the IMMEDIATE upsert return, not just on a later DB round-trip", async () => { const env = createTestEnv(); const record = await upsertPullRequestFromGitHub(env, "owner/repo", {