From 789451adf557d26969c3206538a5acb8cd80c974 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Mon, 27 Jul 2026 01:04:00 -0700 Subject: [PATCH 1/6] fix(gate): store linked-issue claim time per (PR, issue) instead of per PR The blended pull_requests.linked_issue_claimed_at column preserves an old timestamp whenever the new linked-issue set overlaps the old one, so a PR that claimed a throwaway issue on day one and later adds a valuable issue inherits the day-one timestamp for the new issue too -- letting an old placeholder PR backdate a claim and steal the duplicate-cluster winner slot. Add a durable per-(repo, PR, issue) claim ledger (linked_issue_claims, migration 0191) written once, immutably, the first time each issue is observed linked to a PR. The duplicate-winner election now reads this ledger, scoped to only the issue(s) actually contested with an open sibling, instead of the PR-level blended value. Closes #9160 --- migrations/0191_linked_issue_claims.sql | 33 +++++++++++ src/db/repositories.ts | 39 +++++++++++++ src/db/schema.ts | 23 ++++++++ test/unit/db-parsers.test.ts | 77 +++++++++++++++++++++++++ 4 files changed, 172 insertions(+) create mode 100644 migrations/0191_linked_issue_claims.sql diff --git a/migrations/0191_linked_issue_claims.sql b/migrations/0191_linked_issue_claims.sql new file mode 100644 index 000000000..84bfc4bab --- /dev/null +++ b/migrations/0191_linked_issue_claims.sql @@ -0,0 +1,33 @@ +-- #9160: durable per-(PR, issue) claim ledger. pull_requests.linked_issue_claimed_at is a single PR-level +-- timestamp that BLENDS every currently-linked issue into one value -- resolveLinkedIssueClaimedAt preserves +-- it whenever the new linked-issue set overlaps the old one, so a PR that claimed issue #1 on day 1 and later +-- adds "Fixes #7" keeps the day-1 timestamp for #7 too. That lets an attacker keep a long-lived PR linking a +-- throwaway issue, then edit its body to add a victim's valuable issue and inherit a backdated claim time, +-- stealing the duplicate-cluster winner slot (queue/duplicate-detection.ts, packages/loopover-engine's +-- duplicate-winner election). +-- +-- Each row here is written ONCE, immutably, the first time loopover observes THIS SPECIFIC (repo, PR, issue) +-- triple (INSERT OR IGNORE) -- so a newly-added issue always gets its own fresh claim time regardless of what +-- the blended pull_requests column says. The duplicate-winner election reads this table, scoped to only the +-- issue(s) actually contested with a sibling, instead of the blended PR-level value. +CREATE TABLE IF NOT EXISTS linked_issue_claims ( + repo_full_name TEXT NOT NULL, + pull_number INTEGER NOT NULL, + issue_number INTEGER NOT NULL, + claimed_at TEXT NOT NULL, + PRIMARY KEY (repo_full_name, pull_number, issue_number) +); + +-- Backfill: seed one row per currently-linked issue from the existing blended pull_requests column (same +-- COALESCE chain 0084_pull_request_linked_issue_claimed_at.sql used), so an issue already linked before this +-- migration keeps its historical relative ordering intact instead of silently resetting to "whenever this PR +-- next happens to re-sync" the first time recordLinkedIssueClaims runs post-deploy. This is exactly as +-- backdated as today's pre-fix behavior for these PRE-EXISTING rows (no worse) -- the fix's actual benefit (a +-- NEWLY-added issue getting its own fresh, un-backdatable clock) applies going forward, from each PR's next +-- sync onward. +INSERT OR IGNORE INTO linked_issue_claims (repo_full_name, pull_number, issue_number, claimed_at) +SELECT pr.repo_full_name, pr.number, CAST(je.value AS INTEGER), COALESCE(pr.linked_issue_claimed_at, pr.updated_at, pr.created_at) +FROM pull_requests pr, json_each(pr.linked_issues_json) je +WHERE pr.linked_issues_json IS NOT NULL + AND pr.linked_issues_json != '[]' + AND pr.linked_issues_json != ''; diff --git a/src/db/repositories.ts b/src/db/repositories.ts index 12666adfa..df3290b54 100644 --- a/src/db/repositories.ts +++ b/src/db/repositories.ts @@ -33,6 +33,7 @@ import { installations, issues, githubRateLimitObservations, + linkedIssueClaims, notificationDeliveries, issueWatchSubscriptions, notificationSubscriptions, @@ -453,6 +454,11 @@ export async function upsertPullRequestFromGitHub( existingClaimRow, observedLinkedIssueClaimedAt, ); + // #9160: alongside the blended PR-level column above, durably record each CURRENTLY-linked issue's own + // first-observed claim time (immutable, per-issue) -- a brand-new issue gets `syncedAt` here regardless of + // what the blended column resolves to, so the duplicate-winner election can read a claim time that a + // different, already-linked issue can never backdate. Best-effort: never fails the PR upsert itself. + await recordLinkedIssueClaims(env, repoFullName, pr.number, linkedIssues, syncedAt).catch(() => undefined); // A genuinely observed body (even an explicitly empty one) proves this PR's linked-issue extraction is // trustworthy going forward; a sparse payload (pr.body === undefined) proves nothing either way and must not // start the clock. Set once, keep forever (#linked-issue-sparse-first-upsert). @@ -586,6 +592,39 @@ function linkedIssueSetsOverlap(left: number[], right: number[]): boolean { return right.some((value) => leftSet.has(value)); } +/** #9160: record an IMMUTABLE first-observed claim time for each of `issueNumbers` on (repoFullName, + * pullNumber) -- INSERT ... ON CONFLICT DO NOTHING, so an issue already claimed here keeps its ORIGINAL + * claimedAt forever, while a genuinely NEW issue (never linked to this PR before) gets `observedAt` as its + * own fresh clock -- regardless of what pull_requests.linked_issue_claimed_at's blended value says (see that + * column's own #linked-issue-claim-overlap-preserve doc comment for the backdating bug this ledger exists to + * close). A no-op for an empty `issueNumbers`. Called from upsertPullRequestFromGitHub on every sync; safe to + * call repeatedly with the same issue numbers (idempotent). */ +export async function recordLinkedIssueClaims(env: Env, repoFullName: string, pullNumber: number, issueNumbers: readonly number[], observedAt: string): Promise { + if (issueNumbers.length === 0) return; + const bounded = boundedString(repoFullName, 200); + await getDb(env.DB) + .insert(linkedIssueClaims) + .values(issueNumbers.map((issueNumber) => ({ repoFullName: bounded, pullNumber, issueNumber, claimedAt: observedAt }))) + .onConflictDoNothing({ target: [linkedIssueClaims.repoFullName, linkedIssueClaims.pullNumber, linkedIssueClaims.issueNumber] }); +} + +/** #9160: the EARLIEST recorded per-issue claim time among `issueNumbers` for (repoFullName, pullNumber), or + * `null` when none of them has ever been recorded (a pre-migration PR, or a caller passing an issue number + * this PR never actually linked) -- fails closed the same way resolveDuplicateClusterWinnerNumber's own + * sparse-row handling does, rather than guessing. Callers scope `issueNumbers` to the issue(s) ACTUALLY + * contested with a specific sibling (see queue/duplicate-detection.ts) so an unrelated issue also linked to + * this PR can never backdate (or postdate) the comparison. */ +export async function getEarliestLinkedIssueClaim(env: Env, repoFullName: string, pullNumber: number, issueNumbers: readonly number[]): Promise { + if (issueNumbers.length === 0) return null; + const [row] = await getDb(env.DB) + .select({ claimedAt: linkedIssueClaims.claimedAt }) + .from(linkedIssueClaims) + .where(and(eq(linkedIssueClaims.repoFullName, boundedString(repoFullName, 200)), eq(linkedIssueClaims.pullNumber, pullNumber), inArray(linkedIssueClaims.issueNumber, [...issueNumbers]))) + .orderBy(asc(linkedIssueClaims.claimedAt)) + .limit(1); + return row?.claimedAt ?? null; +} + export async function upsertIssueFromGitHub(env: Env, repoFullName: string, issue: GitHubIssuePayload, options: { seenOpenAt?: string } = {}): Promise { const record = toIssueRecord(repoFullName, issue); const db = getDb(env.DB); diff --git a/src/db/schema.ts b/src/db/schema.ts index 82df0178d..cd6489f14 100644 --- a/src/db/schema.ts +++ b/src/db/schema.ts @@ -482,6 +482,29 @@ export const pullRequests = sqliteTable( }), ); +// #9160: durable per-(PR, issue) claim ledger. `pullRequests.linkedIssueClaimedAt` above is a single PR-level +// timestamp that BLENDS every currently-linked issue into one value -- resolveLinkedIssueClaimedAt preserves it +// whenever the new linked-issue set overlaps the old one, so a PR that claimed issue #1 on day 1 and later adds +// "Fixes #7" keeps the day-1 timestamp for #7 too, letting an old placeholder PR backdate a claim on a +// newly-added, more valuable issue and steal the duplicate-cluster winner slot from whoever actually claimed it +// first (queue/duplicate-detection.ts). Each row here is written ONCE, immutably, the first time loopover +// observes THIS SPECIFIC (repo, PR, issue) triple (INSERT ... ON CONFLICT DO NOTHING) -- so a newly-added issue +// always gets its own fresh claim time here regardless of what the blended column says, and the duplicate- +// winner election can be scoped to only the issue(s) actually contested with a sibling instead of the blended +// PR-level value. +export const linkedIssueClaims = sqliteTable( + "linked_issue_claims", + { + repoFullName: text("repo_full_name").notNull(), + pullNumber: integer("pull_number").notNull(), + issueNumber: integer("issue_number").notNull(), + claimedAt: text("claimed_at").notNull(), + }, + (table) => ({ + primary: primaryKey({ columns: [table.repoFullName, table.pullNumber, table.issueNumber] }), + }), +); + export const pullRequestFiles = sqliteTable( "pull_request_files", { diff --git a/test/unit/db-parsers.test.ts b/test/unit/db-parsers.test.ts index e19d997bd..5f8ce79fc 100644 --- a/test/unit/db-parsers.test.ts +++ b/test/unit/db-parsers.test.ts @@ -38,6 +38,8 @@ import { extractLinkedIssueNumbers, extractLinkedIssueNumbersWithOverflow, MAX_LINKED_ISSUE_NUMBERS, + recordLinkedIssueClaims, + getEarliestLinkedIssueClaim, } from "../../src/db/repositories"; import { getDb } from "../../src/db/client"; import { webhookEvents } from "../../src/db/schema"; @@ -265,6 +267,81 @@ describe("database row parser hardening", () => { }); }); + it("REGRESSION (#9160): the per-(PR, issue) claim ledger gives a newly-added issue its OWN fresh claim time, immune to the blended column's overlap-preserve backdating", async () => { + const env = createTestEnv(); + + vi.useFakeTimers(); + vi.setSystemTime(new Date("2026-06-29T10:00:00.000Z")); + await upsertPullRequestFromGitHub(env, "owner/repo", { + number: 21, + title: "Placeholder claim", + state: "open", + user: { login: "attacker" }, + labels: [], + body: "Fixes #1", + }); + // #1's ledger row is stamped at the moment it was first observed. + await expect(getEarliestLinkedIssueClaim(env, "owner/repo", 21, [1])).resolves.toBe("2026-06-29T10:00:00.000Z"); + + // Weeks later, the attacker edits the body to ALSO claim a victim's valuable issue #7. The blended + // pull_requests.linked_issue_claimed_at column preserves the day-1 timestamp for the WHOLE PR (the + // #linked-issue-claim-overlap-preserve behavior, intentional for #1) -- but the per-issue ledger must give + // #7 its own fresh clock rather than inheriting #1's, since #7 was never linked to this PR before now. + vi.setSystemTime(new Date("2026-07-20T09:00:00.000Z")); + await upsertPullRequestFromGitHub(env, "owner/repo", { + number: 21, + title: "Backdating attempt", + state: "open", + user: { login: "attacker" }, + labels: [], + body: "Fixes #1\nFixes #7", + }); + const blended = (await listPullRequests(env, "owner/repo")).find((p) => p.number === 21); + // The blended column is unchanged (still day-1) -- this is the pre-existing, intentional behavior for #1. + expect(blended?.linkedIssueClaimedAt).toBe("2026-06-29T10:00:00.000Z"); + // #1's OWN ledger row is untouched (immutable, ON CONFLICT DO NOTHING). + await expect(getEarliestLinkedIssueClaim(env, "owner/repo", 21, [1])).resolves.toBe("2026-06-29T10:00:00.000Z"); + // #7 gets its OWN fresh claim time -- NOT the backdated day-1 value the blended column would suggest. + await expect(getEarliestLinkedIssueClaim(env, "owner/repo", 21, [7])).resolves.toBe("2026-07-20T09:00:00.000Z"); + + // A victim's genuine PR claiming #7 on day 1 (before the attacker's edit) keeps the earlier claim when the + // two are compared on issue #7 specifically -- exactly the comparison resolveScopedLinkedIssueClaimedAt + // (src/queue/duplicate-detection.ts) performs for the duplicate-winner election. + vi.setSystemTime(new Date("2026-07-01T00:00:00.000Z")); + await upsertPullRequestFromGitHub(env, "owner/repo", { + number: 22, + title: "Victim's genuine claim", + state: "open", + user: { login: "victim" }, + labels: [], + body: "Fixes #7", + }); + await expect(getEarliestLinkedIssueClaim(env, "owner/repo", 22, [7])).resolves.toBe("2026-07-01T00:00:00.000Z"); + // Scoped to issue #7 only: the victim's 07-01 claim predates the attacker's 07-20 claim, so the victim + // would correctly win the duplicate-cluster election -- the blended column's day-1 value for PR #21 would + // have wrongly beaten the victim's genuine claim had it been used instead. + const attackerClaim7 = await getEarliestLinkedIssueClaim(env, "owner/repo", 21, [7]); + const victimClaim7 = await getEarliestLinkedIssueClaim(env, "owner/repo", 22, [7]); + expect(victimClaim7! < attackerClaim7!).toBe(true); + }); + + it("recordLinkedIssueClaims is a no-op for an empty issue list, and getEarliestLinkedIssueClaim returns null when nothing has ever been recorded", async () => { + const env = createTestEnv(); + await expect(recordLinkedIssueClaims(env, "owner/repo", 30, [], "2026-01-01T00:00:00.000Z")).resolves.toBeUndefined(); + await expect(getEarliestLinkedIssueClaim(env, "owner/repo", 30, [])).resolves.toBeNull(); + await expect(getEarliestLinkedIssueClaim(env, "owner/repo", 30, [99])).resolves.toBeNull(); + }); + + it("getEarliestLinkedIssueClaim returns the EARLIEST among several contested issue numbers, not merely the first row", async () => { + const env = createTestEnv(); + await recordLinkedIssueClaims(env, "owner/repo", 40, [5, 6], "2026-02-02T00:00:00.000Z"); + // A second call with an overlapping+new set never overwrites #5/#6's existing rows (ON CONFLICT DO + // NOTHING) but DOES seed a fresh row for the genuinely new #8. + await recordLinkedIssueClaims(env, "owner/repo", 40, [5, 6, 8], "2026-03-03T00:00:00.000Z"); + await expect(getEarliestLinkedIssueClaim(env, "owner/repo", 40, [6, 8])).resolves.toBe("2026-02-02T00:00:00.000Z"); + await expect(getEarliestLinkedIssueClaim(env, "owner/repo", 40, [8])).resolves.toBe("2026-03-03T00:00:00.000Z"); + }); + it("falls back to the observed linked-issue claim time when the existing same-claim timestamp is missing", async () => { const env = createTestEnv(); From 0bef1d0a38ff641d7926129e450fec78aeb4a8b7 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Mon, 27 Jul 2026 01:05:37 -0700 Subject: [PATCH 2/6] fix(gate): scope the duplicate-winner election to per-issue claim times Wire the per-(PR, issue) claim ledger (previous commit) into every duplicate-winner call site: resolveScopedLinkedIssueClaimedAt reads the ledger's earliest claim among only the issue(s) actually contested with an open sibling, and buildPullRequestAdvisory's election now compares against that scoped value instead of the PR's blended linkedIssueClaimedAt column. Falls back to the blended column when the caller doesn't resolve a scoped value, so every unwired caller keeps prior behavior exactly. Closes #9160 --- src/queue/duplicate-detection.ts | 31 +++++++++ src/queue/processors.ts | 65 ++++++++++++++++-- src/rules/advisory.ts | 19 +++++- ...lve-scoped-linked-issue-claimed-at.test.ts | 67 +++++++++++++++++++ test/unit/rules.test.ts | 54 +++++++++++++++ 5 files changed, 229 insertions(+), 7 deletions(-) create mode 100644 test/unit/resolve-scoped-linked-issue-claimed-at.test.ts diff --git a/src/queue/duplicate-detection.ts b/src/queue/duplicate-detection.ts index 907bca7e6..3ff3d1f59 100644 --- a/src/queue/duplicate-detection.ts +++ b/src/queue/duplicate-detection.ts @@ -12,6 +12,7 @@ import { githubRateLimitAdmissionKeyForToken } from "../github/client"; import { resolveCopycatDuplicateSibling } from "../signals/copycat-duplicate"; import { isDuplicateClusterWinnerByClaim, resolveDuplicateClusterWinnerNumber } from "../signals/duplicate-winner"; import { isDuplicateWinnerEnabledGlobally, resolveDuplicateWinnerEnabled } from "../settings/duplicate-winner-mode"; +import { getEarliestLinkedIssueClaim } from "../db/repositories"; import type { CopycatGateMode, PullRequestRecord, RepositorySettings } from "../types"; import { mapWithConcurrency } from "./map-with-concurrency"; @@ -134,6 +135,36 @@ export async function reconcileLiveDuplicateSiblings( ); } +/** + * #9160: scope `pr`'s claim time to ONLY the issue(s) actually contested with at least one of `openSiblings`, + * reading each issue's own IMMUTABLE first-observed claim from the durable per-(PR, issue) ledger + * (`db/repositories.ts`'s `linked_issue_claims`, see its own doc comment) instead of `pr`'s blended + * `linkedIssueClaimedAt` column. This is what closes the #linked-issue-claim-overlap-preserve-style backdating + * attack at the winner-election boundary specifically: an attacker's OTHER, unrelated linked issue (kept + * around purely so the blended column's overlap rule preserves an old timestamp) never enters this + * computation, because it was never shared with any sibling in `openSiblings`. + * + * `contestedIssues` empty (no sibling actually overlaps -- defensive; every real caller already filtered + * `openSiblings` down to PRs sharing an issue with `pr`) returns `pr.linkedIssueClaimedAt` unchanged, since + * there is no backdating angle without a contested issue. Otherwise returns the per-issue ledger's earliest + * claim among the CONTESTED issues only, which fails closed to `null` (mirrors this module's existing + * sparse-row-fails-closed posture) rather than falling back to the blended value when the ledger has no row + * yet for any of them -- a `null` here makes {@link isDuplicateClusterWinnerByClaim} correctly refuse to + * elect `pr` the winner from missing data, rather than risk resurrecting the very blended value being scoped + * away from. + */ +export async function resolveScopedLinkedIssueClaimedAt( + env: Env, + repoFullName: string, + pr: Pick, + openSiblings: readonly Pick[], +): Promise { + const siblingIssues = new Set(openSiblings.flatMap((sibling) => sibling.linkedIssues)); + const contestedIssues = pr.linkedIssues.filter((issue) => siblingIssues.has(issue)); + if (contestedIssues.length === 0) return pr.linkedIssueClaimedAt; + return getEarliestLinkedIssueClaim(env, repoFullName, pr.number, contestedIssues); +} + export function linkedIssueDuplicatePullRequestsForGate( pr: PullRequestRecord, pullRequests: PullRequestRecord[], diff --git a/src/queue/processors.ts b/src/queue/processors.ts index a53b1b8c7..6397e18db 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -380,6 +380,7 @@ import { linkedIssueDuplicatePullRequestRecordsForGate, linkedIssueDuplicatePullRequestsForGate, reconcileLiveDuplicateSiblings, + resolveScopedLinkedIssueClaimedAt, } from "./duplicate-detection"; export { dupWinnerLinkedDuplicateCount, @@ -387,6 +388,7 @@ export { linkedIssueDuplicatePullRequestRecordsForGate, linkedIssueDuplicatePullRequestsForGate, reconcileLiveDuplicateSiblings, + resolveScopedLinkedIssueClaimedAt, } from "./duplicate-detection"; import { mapWithConcurrency } from "./map-with-concurrency"; export { mapWithConcurrency } from "./map-with-concurrency"; @@ -1601,6 +1603,10 @@ export async function sweepRepoRegate( pr.linkedIssues, settings, ); + // #9160: see resolveScopedLinkedIssueClaimedAt's own doc comment -- scopes pr's claim time to only the + // issue(s) actually contested with an open sibling instead of pr's blended linkedIssueClaimedAt column. + const scopedLinkedIssueClaimedAt = + duplicateWinnerEnabled && others.length > 0 ? await resolveScopedLinkedIssueClaimedAt(env, repoFullName, pr, others) : pr.linkedIssueClaimedAt; const advisory = buildPullRequestAdvisory(repo, pr, { otherOpenPullRequests: others, requireLinkedIssue, @@ -1609,6 +1615,7 @@ export async function sweepRepoRegate( confirmedNoOpenLinkedIssue, copycatGateMode: settings.copycatGateMode, copycatGateMinScore: settings.copycatGateMinScore, + scopedLinkedIssueClaimedAt, }); const gate = evaluateGateCheck( advisory, @@ -2721,6 +2728,11 @@ function buildAgentMaintenancePlanInput(args: { pr: PullRequestRecord; openDuplicateSiblings: ReturnType; duplicateWinnerEnabled: boolean; + // #9160: pr's claim time, ALREADY SCOPED by the caller (resolveScopedLinkedIssueClaimedAt) to only the + // issue(s) actually contested with an open sibling -- resolved outside this function since it needs a DB + // read and this function is otherwise pure. Falls back to pr.linkedIssueClaimedAt when undefined so every + // existing test/caller that hasn't been updated to pass it keeps today's (pre-#9160) behavior exactly. + scopedLinkedIssueClaimedAt?: string | null | undefined; manualReviewLockContentionResolved: AgentActionPlanInput["manualReviewLockContentionResolved"]; }): AgentActionPlanInput { const { @@ -2747,8 +2759,10 @@ function buildAgentMaintenancePlanInput(args: { pr, openDuplicateSiblings, duplicateWinnerEnabled, + scopedLinkedIssueClaimedAt, manualReviewLockContentionResolved, } = args; + const linkedIssueClaimedAtForElection = scopedLinkedIssueClaimedAt !== undefined ? scopedLinkedIssueClaimedAt : pr.linkedIssueClaimedAt; return { conclusion: gate.conclusion, blockerTitles: gate.blockers.map((blocker) => blocker.title), @@ -2828,7 +2842,7 @@ function buildAgentMaintenancePlanInput(args: { linkedDuplicateCount: dupWinnerLinkedDuplicateCount( openDuplicateSiblings, pr.number, - pr.linkedIssueClaimedAt, + linkedIssueClaimedAtForElection, duplicateWinnerEnabled, pr.createdAt, ), @@ -2838,7 +2852,7 @@ function buildAgentMaintenancePlanInput(args: { linkedDuplicateWinnerNumber: dupWinnerLinkedDuplicateWinnerNumber( openDuplicateSiblings, pr.number, - pr.linkedIssueClaimedAt, + linkedIssueClaimedAtForElection, duplicateWinnerEnabled, pr.createdAt, ), @@ -2866,7 +2880,10 @@ function buildAgentMaintenancePlanInput(args: { * EVERY PR-open across the fleet, not just the rare over-cap ones — this function alone is cheap enough (one * DB query + a bounded live-GitHub confirm) to run unconditionally on open. */ -async function resolvePerRepoContributorCapMatch( +// #9159: exported so the approval-queue accept path (src/services/agent-approval-queue.ts) can build the SAME +// pre-merge contributor-cap re-check the live webhook path builds below (contributorCapMergeRecheck) -- that +// path previously never threaded one at all, so an accepted staged merge skipped the #7284 re-check entirely. +export async function resolvePerRepoContributorCapMatch( env: Env, deliveryId: string, installationId: number, @@ -3502,6 +3519,17 @@ async function runAgentMaintenancePlanAndExecute( // not inherit this transient provenance on some future pass. await deleteTransientKey(env, manualReviewLockContentionMarkerKey); } + // #9160: scope pr's claim time to only the issue(s) actually contested with an open duplicate sibling, + // reading the durable per-(PR, issue) claim ledger instead of pr's own blended linkedIssueClaimedAt column + // -- see resolveScopedLinkedIssueClaimedAt's own doc comment for why the blended column is unsafe to feed + // directly into the winner election (an unrelated OTHER linked issue on this PR could otherwise "vouch" a + // backdated claim time for the one actually being contested). Only worth the extra DB read when the flag is + // on AND at least one sibling actually overlaps; otherwise this is `pr.linkedIssueClaimedAt` unchanged, + // byte-identical to before this existed. + const scopedLinkedIssueClaimedAt = + duplicateWinnerEnabled && openDuplicateSiblings.length > 0 + ? await resolveScopedLinkedIssueClaimedAt(env, repoFullName, pr, openDuplicateSiblings) + : pr.linkedIssueClaimedAt; const planned = planAgentMaintenanceActions( buildAgentMaintenancePlanInput({ gate, @@ -3527,6 +3555,7 @@ async function runAgentMaintenancePlanAndExecute( pr, openDuplicateSiblings, duplicateWinnerEnabled, + scopedLinkedIssueClaimedAt, manualReviewLockContentionResolved: hadPriorLockContentionHold && !aiReviewLockContentionThisPass, }), ); @@ -4085,14 +4114,22 @@ export async function reReviewStoredPullRequest( cachedOtherOpenPullRequests, settings, ); + const duplicateWinnerEnabledForPr = resolveDuplicateWinnerEnabled(isDuplicateWinnerEnabledGlobally(env), settings.duplicateWinnerMode); + // #9160: see resolveScopedLinkedIssueClaimedAt's own doc comment -- scopes pr's claim time to only the + // issue(s) actually contested with an open sibling instead of pr's blended linkedIssueClaimedAt column. + const scopedLinkedIssueClaimedAt = + duplicateWinnerEnabledForPr && otherOpenPullRequests.length > 0 + ? await resolveScopedLinkedIssueClaimedAt(env, repoFullName, pr, otherOpenPullRequests) + : pr.linkedIssueClaimedAt; const advisory = buildPullRequestAdvisory(repo, pr, { otherOpenPullRequests, requireLinkedIssue: shouldCollectLinkedIssueEvidence(settings), - duplicateWinnerEnabled: resolveDuplicateWinnerEnabled(isDuplicateWinnerEnabledGlobally(env), settings.duplicateWinnerMode), + duplicateWinnerEnabled: duplicateWinnerEnabledForPr, confirmedNoOpenLinkedIssue, linkedIssueAuthorLogins, copycatGateMode: settings.copycatGateMode, copycatGateMinScore: settings.copycatGateMinScore, + scopedLinkedIssueClaimedAt, }); await persistAdvisory(env, advisory); // #2537 follow-up (gate-flagged): the durable review cache's only invalidation path is markPullRequestReviewsInvalidated @@ -6942,14 +6979,22 @@ async function handlePullRequestWebhookEvent( cachedOtherOpenPullRequests, settings, ); + const duplicateWinnerEnabledForPr = resolveDuplicateWinnerEnabled(isDuplicateWinnerEnabledGlobally(env), settings.duplicateWinnerMode); + // #9160: see resolveScopedLinkedIssueClaimedAt's own doc comment -- scopes pr's claim time to only the + // issue(s) actually contested with an open sibling instead of pr's blended linkedIssueClaimedAt column. + const scopedLinkedIssueClaimedAt = + duplicateWinnerEnabledForPr && otherOpenPullRequests.length > 0 + ? await resolveScopedLinkedIssueClaimedAt(env, repoFullName, pr, otherOpenPullRequests) + : pr.linkedIssueClaimedAt; const advisory = buildPullRequestAdvisory(repo, pr, { otherOpenPullRequests, requireLinkedIssue: shouldCollectLinkedIssueEvidence(settings), - duplicateWinnerEnabled: resolveDuplicateWinnerEnabled(isDuplicateWinnerEnabledGlobally(env), settings.duplicateWinnerMode), + duplicateWinnerEnabled: duplicateWinnerEnabledForPr, confirmedNoOpenLinkedIssue, linkedIssueAuthorLogins, copycatGateMode: settings.copycatGateMode, copycatGateMinScore: settings.copycatGateMinScore, + scopedLinkedIssueClaimedAt, }); await persistAdvisory(env, advisory); // Auto-project/milestone matching (#3183): independent of the gate/disposition entirely -- a missed or @@ -14296,14 +14341,22 @@ export async function buildAuthorizedPrActionAdvisory( pr.linkedIssues, settings, ); + const duplicateWinnerEnabledForPr = resolveDuplicateWinnerEnabled(isDuplicateWinnerEnabledGlobally(env), settings.duplicateWinnerMode); + // #9160: see resolveScopedLinkedIssueClaimedAt's own doc comment -- scopes pr's claim time to only the + // issue(s) actually contested with an open sibling instead of pr's blended linkedIssueClaimedAt column. + const scopedLinkedIssueClaimedAt = + duplicateWinnerEnabledForPr && otherOpenPullRequests.length > 0 + ? await resolveScopedLinkedIssueClaimedAt(env, repoFullName, pr, otherOpenPullRequests) + : pr.linkedIssueClaimedAt; const advisory = buildPullRequestAdvisory(repo, pr, { otherOpenPullRequests, requireLinkedIssue: shouldCollectLinkedIssueEvidence(settings), - duplicateWinnerEnabled: resolveDuplicateWinnerEnabled(isDuplicateWinnerEnabledGlobally(env), settings.duplicateWinnerMode), + duplicateWinnerEnabled: duplicateWinnerEnabledForPr, confirmedNoOpenLinkedIssue, linkedIssueAuthorLogins, copycatGateMode: settings.copycatGateMode, copycatGateMinScore: settings.copycatGateMinScore, + scopedLinkedIssueClaimedAt, }); return { repo, advisory, otherOpenPullRequests }; } diff --git a/src/rules/advisory.ts b/src/rules/advisory.ts index 8ad4a5958..1c3f26711 100644 --- a/src/rules/advisory.ts +++ b/src/rules/advisory.ts @@ -386,6 +386,11 @@ export function buildPullRequestAdvisory( * identical to before this field existed — the duplicate finding stays linked-issue-overlap only. */ copycatGateMode?: CopycatGateMode | null | undefined; copycatGateMinScore?: number | null | undefined; + /** #9160: see addPullRequestFindings's own parameter doc comment. Resolved by the caller (this function + * stays synchronous/pure) via queue/duplicate-detection.ts's resolveScopedLinkedIssueClaimedAt, from the + * SAME otherOpenPullRequests already being passed above. Absent ⇒ falls back to pr.linkedIssueClaimedAt, + * byte-identical to before this field existed. */ + scopedLinkedIssueClaimedAt?: string | null | undefined; } = {}, ): Advisory { const repoFullName = pr?.repoFullName ?? repo?.fullName ?? "unknown/unknown"; @@ -422,6 +427,7 @@ export function buildPullRequestAdvisory( Boolean(context.confirmedNoOpenLinkedIssue), context.copycatGateMode, context.copycatGateMinScore, + context.scopedLinkedIssueClaimedAt, ); } return advisory("pull_request", targetKey, repoFullName, findings, "Pull request advisory generated.", pr?.number, undefined, pr?.headSha ?? undefined); @@ -990,6 +996,13 @@ function addPullRequestFindings( confirmedNoOpenLinkedIssue: boolean, copycatGateMode: CopycatGateMode | null | undefined, copycatGateMinScore: number | null | undefined, + // #9160: pr's claim time, ALREADY SCOPED by the caller (queue/duplicate-detection.ts's + // resolveScopedLinkedIssueClaimedAt) to only the issue(s) actually contested with an open sibling, instead of + // pr.linkedIssueClaimedAt's blended-across-every-linked-issue value -- see that function's own doc comment + // for why the blended column lets an unrelated, already-linked issue backdate a newly-added one's claim. + // `undefined` (every caller that hasn't been updated, and every non-DB caller like decision-replay.ts) falls + // back to pr.linkedIssueClaimedAt, byte-identical to before this existed. + scopedLinkedIssueClaimedAt?: string | null | undefined, ): void { if (pr.state !== "open") { findings.push({ @@ -1036,7 +1049,11 @@ function addPullRequestFindings( // winner survives while later claimants keep the finding. Sparse legacy rows fail closed instead of // suppressing duplicate evidence with arbitrary PR-number ordering. // Flag-OFF (default) short-circuits ⇒ the finding is pushed exactly as before (byte-identical). - if (overlappingPrs.length > 0 && !(duplicateWinnerEnabled && isDuplicateClusterWinnerByClaim(pr, overlappingPrs))) { + // #9160: compare using the SCOPED claim time (falls back to pr.linkedIssueClaimedAt when the caller didn't + // resolve one) rather than pr's raw, blended column directly -- see scopedLinkedIssueClaimedAt's own + // parameter doc comment above. + const claimMemberForElection = { number: pr.number, linkedIssueClaimedAt: scopedLinkedIssueClaimedAt !== undefined ? scopedLinkedIssueClaimedAt : pr.linkedIssueClaimedAt, createdAt: pr.createdAt }; + if (overlappingPrs.length > 0 && !(duplicateWinnerEnabled && isDuplicateClusterWinnerByClaim(claimMemberForElection, overlappingPrs))) { // #9129: split by corroboration (hasDuplicateOverlapCorroboration) — see its own doc comment. A CONCRETE // finding code (`duplicate_pr_risk`) is reserved for a sibling with real corroborating evidence beyond body // text; it stays configurable via duplicatePrGateMode and evaluateGateCheckCore now HOLDS (never closes) a diff --git a/test/unit/resolve-scoped-linked-issue-claimed-at.test.ts b/test/unit/resolve-scoped-linked-issue-claimed-at.test.ts new file mode 100644 index 000000000..93c5039c5 --- /dev/null +++ b/test/unit/resolve-scoped-linked-issue-claimed-at.test.ts @@ -0,0 +1,67 @@ +import { describe, expect, it } from "vitest"; +import { resolveScopedLinkedIssueClaimedAt } from "../../src/queue/duplicate-detection"; +import { recordLinkedIssueClaims } from "../../src/db/repositories"; +import type { PullRequestRecord } from "../../src/types"; +import { createTestEnv } from "../helpers/d1"; + +// #9160: resolveScopedLinkedIssueClaimedAt scopes a PR's claim time to only the issue(s) actually contested +// with an open sibling, reading the durable per-(PR, issue) ledger (db/repositories.ts's linked_issue_claims) +// instead of the PR's blended linkedIssueClaimedAt column -- see that function's own doc comment for the +// backdating attack this closes (an attacker's unrelated, already-linked issue "vouching" a stale claim time +// for a newly-added, contested one). +function makePr(number: number, linkedIssues: number[], linkedIssueClaimedAt: string | null): Pick { + return { number, linkedIssues, linkedIssueClaimedAt }; +} + +describe("resolveScopedLinkedIssueClaimedAt (#9160)", () => { + it("returns pr.linkedIssueClaimedAt unchanged when no sibling actually overlaps (no contested issue)", async () => { + const env = createTestEnv(); + const pr = makePr(21, [7], "2026-06-29T10:00:00.000Z"); + const siblings = [makePr(22, [99], "2026-07-01T00:00:00.000Z")]; + await expect(resolveScopedLinkedIssueClaimedAt(env, "owner/repo", pr, siblings)).resolves.toBe("2026-06-29T10:00:00.000Z"); + }); + + it("returns pr.linkedIssueClaimedAt unchanged when openSiblings is empty", async () => { + const env = createTestEnv(); + const pr = makePr(21, [7], "2026-06-29T10:00:00.000Z"); + await expect(resolveScopedLinkedIssueClaimedAt(env, "owner/repo", pr, [])).resolves.toBe("2026-06-29T10:00:00.000Z"); + }); + + it("REGRESSION (#9160): reads the per-issue ledger's own claim time for a CONTESTED issue, ignoring the blended column's backdated value", async () => { + const env = createTestEnv(); + // Ledger: issue #1 claimed day-1 (long-lived placeholder), issue #7 claimed later (the backdating target). + await recordLinkedIssueClaims(env, "owner/repo", 21, [1], "2026-06-29T10:00:00.000Z"); + await recordLinkedIssueClaims(env, "owner/repo", 21, [7], "2026-07-20T09:00:00.000Z"); + // pr's blended column still reads day-1 (the #linked-issue-claim-overlap-preserve behavior) even though + // #7 was only just added -- resolveScopedLinkedIssueClaimedAt must NOT trust this for the contested issue. + const pr = makePr(21, [1, 7], "2026-06-29T10:00:00.000Z"); + const victimSibling = [makePr(22, [7], "2026-07-01T00:00:00.000Z")]; + await expect(resolveScopedLinkedIssueClaimedAt(env, "owner/repo", pr, victimSibling)).resolves.toBe("2026-07-20T09:00:00.000Z"); + }); + + it("fails closed to null when the contested issue has no ledger row yet (never falls back to the blended value)", async () => { + const env = createTestEnv(); + const pr = makePr(21, [7], "2026-06-29T10:00:00.000Z"); // blended column set, but no ledger row for #7 + const siblings = [makePr(22, [7], "2026-07-01T00:00:00.000Z")]; + await expect(resolveScopedLinkedIssueClaimedAt(env, "owner/repo", pr, siblings)).resolves.toBeNull(); + }); + + it("scopes to only the CONTESTED issue among several linked issues, ignoring an uncontested one entirely", async () => { + const env = createTestEnv(); + await recordLinkedIssueClaims(env, "owner/repo", 21, [1], "2026-01-01T00:00:00.000Z"); + await recordLinkedIssueClaims(env, "owner/repo", 21, [7], "2026-07-20T09:00:00.000Z"); + const pr = makePr(21, [1, 7], "2026-01-01T00:00:00.000Z"); + // Sibling only overlaps on #7, not #1 -- the (much earlier) #1 ledger row must not leak into the result. + const siblings = [makePr(22, [7], "2026-07-01T00:00:00.000Z")]; + await expect(resolveScopedLinkedIssueClaimedAt(env, "owner/repo", pr, siblings)).resolves.toBe("2026-07-20T09:00:00.000Z"); + }); + + it("takes the EARLIEST ledger row when multiple linked issues are each contested by (possibly different) siblings", async () => { + const env = createTestEnv(); + await recordLinkedIssueClaims(env, "owner/repo", 21, [1], "2026-05-01T00:00:00.000Z"); + await recordLinkedIssueClaims(env, "owner/repo", 21, [7], "2026-07-20T09:00:00.000Z"); + const pr = makePr(21, [1, 7], "2026-05-01T00:00:00.000Z"); + const siblings = [makePr(22, [1], "2026-06-01T00:00:00.000Z"), makePr(23, [7], "2026-07-01T00:00:00.000Z")]; + await expect(resolveScopedLinkedIssueClaimedAt(env, "owner/repo", pr, siblings)).resolves.toBe("2026-05-01T00:00:00.000Z"); + }); +}); diff --git a/test/unit/rules.test.ts b/test/unit/rules.test.ts index e1a015797..3cd0bb8a3 100644 --- a/test/unit/rules.test.ts +++ b/test/unit/rules.test.ts @@ -450,6 +450,60 @@ describe("advisory rules", () => { expect(advisory.findings.map((finding) => finding.code)).not.toContain("duplicate_pr_risk_unconfirmed"); }); + it("#9160: scopedLinkedIssueClaimedAt overrides pr's blended linkedIssueClaimedAt for the winner election, closing the backdating attack", () => { + // The attacker's PR carries a backdated BLENDED linkedIssueClaimedAt (from an old, unrelated linked issue + // whose overlap preserved it) that would make it win against the victim if the raw column were used + // directly -- see db-parsers.test.ts's REGRESSION (#9160) test for how the per-(PR, issue) ledger produces + // the correctly-scoped value the caller (queue/duplicate-detection.ts's resolveScopedLinkedIssueClaimedAt) + // passes in as scopedLinkedIssueClaimedAt. + const attacker: PullRequestRecord = { + repoFullName: repo.fullName, + number: 21, + title: "Backdated claim", + state: "open", + authorLogin: "attacker", + authorAssociation: "NONE", + headSha: "abc123", + labels: [], + linkedIssues: [7], + linkedIssueClaimedAt: "2026-06-29T10:00:00.000Z", // blended column: backdated via an unrelated issue #1 + }; + const victim: PullRequestRecord = { ...attacker, number: 22, title: "Victim's genuine claim", authorLogin: "victim", linkedIssueClaimedAt: "2026-07-01T00:00:00.000Z" }; + + // Without scoping, the attacker's blended (backdated) timestamp wins -- confirms the vulnerable baseline. + const unscoped = buildPullRequestAdvisory(repo, attacker, { otherOpenPullRequests: [victim], duplicateWinnerEnabled: true }); + expect(unscoped.findings.map((finding) => finding.code)).not.toContain("duplicate_pr_risk"); + + // With the per-issue-scoped claim time (the attacker's OWN ledger row for issue #7, correctly dated AFTER + // the victim's genuine claim), the attacker is correctly demoted to loser. + const scoped = buildPullRequestAdvisory(repo, attacker, { + otherOpenPullRequests: [victim], + duplicateWinnerEnabled: true, + scopedLinkedIssueClaimedAt: "2026-07-20T09:00:00.000Z", + }); + expect(scoped.findings.map((finding) => finding.code)).toContain("duplicate_pr_risk_unconfirmed"); + }); + + it("#9160: scopedLinkedIssueClaimedAt undefined falls back to pr.linkedIssueClaimedAt, byte-identical to before this field existed", () => { + const winner: PullRequestRecord = { + repoFullName: repo.fullName, + number: 12, + title: "Add registry sync", + state: "open", + authorLogin: "oktofeesh1", + authorAssociation: "NONE", + headSha: "abc123", + labels: [], + linkedIssues: [4], + linkedIssueClaimedAt: "2026-06-29T10:00:00.000Z", + }; + const higherSibling: PullRequestRecord = { ...winner, number: 13, title: "Alternative registry sync", linkedIssueClaimedAt: "2026-06-29T10:01:00.000Z" }; + + const advisory = buildPullRequestAdvisory(repo, winner, { otherOpenPullRequests: [higherSibling], duplicateWinnerEnabled: true, scopedLinkedIssueClaimedAt: undefined }); + + expect(advisory.findings.map((finding) => finding.code)).not.toContain("duplicate_pr_risk"); + }); + it("keeps weak queue warnings advisory-only for the opt-in gate", () => { const pr: PullRequestRecord = { repoFullName: repo.fullName, From c8a3e2fcf35a3e145b85cabe4be424b588e183ae Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Mon, 27 Jul 2026 01:08:03 -0700 Subject: [PATCH 3/6] fix(review): require reward-label opt-in and maintainer check for issue authors A contributor who authored (or was assigned) the linked issue previously inherited every label on it unconditionally, including reward-semantic labels from an additive mapping -- bypassing the trustMaintainerAuthoredIssueForReward opt-in and the maintainer-authorship check every other path already requires. Split the unlock: author/assignee identity still unlocks a plain type label unconditionally, but a reward-semantic label (named by any mapping with removeOtherTypeLabels !== true) now always falls through to the same opt-in + maintainer-check gate regardless of the author match. Closes #9161 --- .../linked-issue-label-propagation-fetch.ts | 40 +++++++- ...nked-issue-label-propagation-fetch.test.ts | 93 +++++++++++++++++++ 2 files changed, 130 insertions(+), 3 deletions(-) diff --git a/src/review/linked-issue-label-propagation-fetch.ts b/src/review/linked-issue-label-propagation-fetch.ts index 70551cee3..aa5cb6f23 100644 --- a/src/review/linked-issue-label-propagation-fetch.ts +++ b/src/review/linked-issue-label-propagation-fetch.ts @@ -136,6 +136,12 @@ async function resolveIssueLabelsForPropagation( prAuthorLogin: string | undefined, relaxableLabels: ReadonlySet, prMergedAt: string | null, + // #9161: issueLabels named by at least one ADDITIVE mapping (`removeOtherTypeLabels !== true` -- see + // pr-type-label.ts's exclusive/additive split, e.g. `gittensor:priority` composing alongside bug/feature). + // An additive mapping carries reward weight by construction (it never replaces the PR's type label, it adds + // a bonus on top) -- author/assignee identity must NOT unlock one of these unconditionally the way it does + // for a plain type label; see the author/assignee branch below for why. + rewardLabels: ReadonlySet, ): Promise { if (result.status === "fetch_error") { console.log( @@ -204,7 +210,21 @@ async function resolveIssueLabelsForPropagation( const allLabels = result.facts.labels; const issueAuthorLogin = result.facts.authorLogin?.toLowerCase(); const assignees = result.facts.assignees.map((login) => login.toLowerCase()); - if (issueAuthorLogin === prAuthorLogin || assignees.includes(prAuthorLogin)) return { labels: allLabels, inconclusive: false }; + const isDirectOwnershipMatch = issueAuthorLogin === prAuthorLogin || assignees.includes(prAuthorLogin); + // #9161: a direct author/assignee match unlocks a TYPE label (no reward-semantic mapping targets it) + // unconditionally -- the original, unchanged behavior. It must NOT also unlock a REWARD label + // (`rewardLabels`) the same way: that bypasses the reward-label trust gate (the `...ForReward` opt-in + + // maintainer-authorship check every OTHER path already requires) entirely, letting a contributor who simply + // authored (or was assigned) the linked issue self-award a reward multiplier with no opt-in and no + // maintainer check. A reward label ALWAYS falls through to the maintainer-check gate below, regardless of + // this match. When there is no direct match at all, every label is a "reward candidate" here -- identical to + // the pre-#9161 behavior for that branch (nothing changes for a non-owning contributor). + const directTypeLabels = isDirectOwnershipMatch ? allLabels.filter((label) => !rewardLabels.has(label.toLowerCase())) : []; + const rewardCandidateLabels = isDirectOwnershipMatch ? allLabels.filter((label) => rewardLabels.has(label.toLowerCase())) : allLabels; + // Zero added cost when this issue carries no reward-semantic label at all (the overwhelmingly common case + // for a repo that hasn't configured an additive mapping) -- skips the maintainer-permission GitHub API call + // entirely, mirroring relaxableLabels' own "byte-identical when no mapping opted in" contract. + if (isDirectOwnershipMatch && rewardCandidateLabels.length === 0) return { labels: directTypeLabels, inconclusive: false }; const maintainerCheck: MaintainerCheckResult = relaxableLabels.size > 0 && !!issueAuthorLogin @@ -222,7 +242,8 @@ async function resolveIssueLabelsForPropagation( return { labels: [], inconclusive: true }; } const maintainerAuthored = maintainerCheck === "maintainer"; - const kept = maintainerAuthored ? allLabels.filter((label) => relaxableLabels.has(label.toLowerCase())) : []; + const relaxedRewardLabels = maintainerAuthored ? rewardCandidateLabels.filter((label) => relaxableLabels.has(label.toLowerCase())) : []; + const kept = [...directTypeLabels, ...relaxedRewardLabels]; if (kept.length < allLabels.length && allLabels.length > 0) { console.log( @@ -230,7 +251,11 @@ async function resolveIssueLabelsForPropagation( event: "linked_issue_label_propagation_filtered", repoFullName: args.repoFullName, issueNumber: result.facts.number, - reason: maintainerAuthored ? "strict_label_requires_direct_ownership" : "no_direct_ownership_match", + reason: isDirectOwnershipMatch + ? "reward_label_requires_maintainer_authored_issue_and_opt_in" + : maintainerAuthored + ? "strict_label_requires_direct_ownership" + : "no_direct_ownership_match", droppedCount: allLabels.length - kept.length, }), ); @@ -314,6 +339,14 @@ export async function fetchLinkedIssueLabelsForPropagation(args: { ) .map(([issueLabel]) => issueLabel), ); + // #9161: issueLabels named by at least one ADDITIVE mapping (`removeOtherTypeLabels !== true`) -- see + // resolveIssueLabelsForPropagation's `rewardLabels` parameter doc comment for why this is the reward- + // semantics signal. `.some` (not `.every`, unlike relaxableLabels above): even ONE additive mapping + // targeting this issueLabel is enough to treat it as reward-bearing everywhere, so a repo cannot dilute the + // reward gate by also declaring an unrelated exclusive mapping for the same issueLabel name. + const rewardLabels = new Set( + [...mappingsByIssueLabel.entries()].filter(([, mappings]) => mappings.some((mapping) => mapping.removeOtherTypeLabels !== true)).map(([issueLabel]) => issueLabel), + ); const results = await Promise.all( linkedIssues.map((issueNumber) => fetchLinkedIssueFacts( @@ -333,6 +366,7 @@ export async function fetchLinkedIssueLabelsForPropagation(args: { prAuthorLogin, relaxableLabels, prMergedAt, + rewardLabels, ), ), ); diff --git a/test/unit/linked-issue-label-propagation-fetch.test.ts b/test/unit/linked-issue-label-propagation-fetch.test.ts index 885990bb1..51235128f 100644 --- a/test/unit/linked-issue-label-propagation-fetch.test.ts +++ b/test/unit/linked-issue-label-propagation-fetch.test.ts @@ -922,4 +922,97 @@ describe("fetchLinkedIssueLabelsForPropagation (#priority-linked-issue-gate)", ( expectPropagation(result, []); }); }); + + describe("reward-label gate on direct author/assignee match (#9161)", () => { + // An ADDITIVE mapping (removeOtherTypeLabels: false) composes a bonus label alongside the PR's type label + // rather than replacing it -- this is the reward-semantics signal src/review/linked-issue-label-propagation-fetch.ts's + // fetchLinkedIssueLabelsForPropagation derives `rewardLabels` from (see its own #9161 doc comment). + const REWARD_MAPPING = { issueLabel: "gittensor:priority", prLabel: "gittensor:priority", removeOtherTypeLabels: false, trustMaintainerAuthoredIssueForReward: true }; + const TYPE_MAPPING = { issueLabel: "gittensor:bug", prLabel: "gittensor:bug", removeOtherTypeLabels: true }; + + it("REGRESSION (#9161): withholds the reward label from an issue-authoring contributor who is NOT a repo maintainer, keeping only the type label", async () => { + stubFetch((url) => { + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.endsWith("/issues/21")) + return Response.json({ number: 21, state: "open", user: { login: "contrib" }, assignees: [], labels: ["gittensor:bug", "gittensor:priority"] }); + if (url.includes("/collaborators/contrib/permission")) return Response.json({ permission: "read" }); + return new Response("not found", { status: 404 }); + }); + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + const result = await fetchLinkedIssueLabelsForPropagation({ + env, + repoFullName: "owner/repo", + linkedIssues: [21], + installationId: 123, + prAuthorLogin: "contrib", + mappings: [TYPE_MAPPING, REWARD_MAPPING], + }); + // "read" permission is not_maintainer -> the reward label (gittensor:priority) is dropped even though the + // mapping opted into trustMaintainerAuthoredIssueForReward; the type label survives via the unconditional + // author-match unlock, since it carries no reward semantics. + expectPropagation(result, ["gittensor:bug"]); + }); + + it("REGRESSION (#9161): withholds the reward label for a PR author who is only ASSIGNED to (not the author of) the linked issue, same as the author-match case", async () => { + stubFetch((url) => { + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.endsWith("/issues/22")) + return Response.json({ number: 22, state: "open", user: { login: "someone-else" }, assignees: [{ login: "contrib" }], labels: ["gittensor:priority"] }); + if (url.includes("/collaborators/someone-else/permission")) return Response.json({ permission: "read" }); + return new Response("not found", { status: 404 }); + }); + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + const result = await fetchLinkedIssueLabelsForPropagation({ + env, + repoFullName: "owner/repo", + linkedIssues: [22], + installationId: 123, + prAuthorLogin: "contrib", + mappings: [REWARD_MAPPING], + }); + expectPropagation(result, []); + }); + + it("propagates the reward label to its own issue's author when that author genuinely IS a repo maintainer (opt-in + maintainer check both satisfied)", async () => { + stubFetch((url) => { + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.endsWith("/issues/23")) + // "owner" is the literal repo owner -- isRepoMaintainerLogin short-circuits to "maintainer" without a + // live collaborator-permission fetch, so no signable key is required for this case. + return Response.json({ number: 23, state: "open", user: { login: "owner" }, assignees: [], labels: ["gittensor:bug", "gittensor:priority"] }); + return new Response("not found", { status: 404 }); + }); + const env = createTestEnv({}); + const result = await fetchLinkedIssueLabelsForPropagation({ + env, + repoFullName: "owner/repo", + linkedIssues: [23], + installationId: 123, + prAuthorLogin: "owner", + mappings: [TYPE_MAPPING, REWARD_MAPPING], + }); + expectPropagation(result, ["gittensor:bug", "gittensor:priority"]); + }); + + it("skips the maintainer-permission check entirely (zero added cost) when the direct-match issue carries no reward-semantic label at all", async () => { + const fetchSpy = vi.fn(async (input: RequestInfo | URL) => { + const url = input.toString(); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.endsWith("/issues/24")) return Response.json({ number: 24, state: "open", user: { login: "contrib" }, assignees: [], labels: ["gittensor:bug"] }); + return new Response("not found", { status: 404 }); + }); + vi.stubGlobal("fetch", fetchSpy); + const env = createTestEnv({}); + const result = await fetchLinkedIssueLabelsForPropagation({ + env, + repoFullName: "owner/repo", + linkedIssues: [24], + installationId: 123, + prAuthorLogin: "contrib", + mappings: [TYPE_MAPPING, REWARD_MAPPING], + }); + expectPropagation(result, ["gittensor:bug"]); + expect(fetchSpy.mock.calls.some(([input]) => input.toString().includes("/collaborators/"))).toBe(false); + }); + }); }); From 162b2a82c1ef5465876360a4a86aa09dd04f7e14 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Mon, 27 Jul 2026 01:12:15 -0700 Subject: [PATCH 4/6] fix(orb): make the label-vs-close correlation guard durable across entry points The #label-close-split-brain guard only inspected the same-batch planned array, so a rejected or breaker-downgraded close could leave its paired enforcement label applied to a PR that stayed open. Two paths never carried the pair through to the executor's own correlation check: - downgradeCloseToHold now also drops any label whose closeKind matches a close it just downgraded (the breaker-downgrade half). - The approval-queue accept path now re-derives pairing from the durable audit trail (the same agent.action.close record every execution entry point writes through) before applying a close-coupled label, since the queue stages one action per row and an independently rejected close has nothing correlating it to the label in the same batch. Closes #9158 --- src/services/agent-approval-queue.ts | 69 +++++++++++++++++ src/settings/agent-actions.ts | 19 ++++- test/unit/agent-actions.test.ts | 48 ++++++++++++ test/unit/agent-approval-queue.test.ts | 103 +++++++++++++++++++++++++ 4 files changed, 237 insertions(+), 2 deletions(-) diff --git a/src/services/agent-approval-queue.ts b/src/services/agent-approval-queue.ts index f787409ff..6208f085b 100644 --- a/src/services/agent-approval-queue.ts +++ b/src/services/agent-approval-queue.ts @@ -4,6 +4,7 @@ import { getPullRequest, getPendingAgentAction, insertNotificationDeliveryIfAbsent, + latestCompletedAuditEventDetail, listPendingAgentActions, recordAuditEvent, setPendingAgentActionStatus, @@ -19,6 +20,10 @@ import { findBlacklistEntry } from "../settings/contributor-blacklist"; import { isCloseHoldOnly, isHoldOnly, readUntrustworthyRuleCodes } from "../review/outcomes-wire"; import { fetchLiveCiAggregate, fetchLivePullRequestMergeState, fetchLivePullRequestReviewDecision, fetchLivePullRequestState, fetchLiveReviewThreadBlockers, fetchRequiredStatusContexts, mergeRequiredCiContexts, REVIEW_DECISION_UNREADABLE } from "../github/backfill"; import { githubRateLimitAdmissionKeyForToken } from "../github/client"; +import { resolvePerRepoContributorCapMatch } from "../queue/processors"; +import { isBelowAccountAgeThreshold } from "../queue/account-age-throttle"; +import { isAutoCloseExempt } from "../settings/auto-close-exempt"; +import { isPerTenantAdmin } from "../auth/security"; import type { AgentPendingActionParams, AgentPendingActionRecord } from "../types"; export type ApprovalDecision = "accept" | "reject"; @@ -63,6 +68,34 @@ export async function decidePendingAgentAction(env: Env, input: { id: string; de return { status: "already_decided", action: current ?? pending }; } + // #9158 (label-close-split-brain, approval-queue half): a label COUPLED to a close (autonomyClass === + // "close" AND closeKind set -- see planContributorCapClose's/the blacklist/review-nag/copycat closes' own + // doc comments: the close is always pushed BEFORE its label, tagged with the SAME closeKind and + // autonomyClass "close") must never post unless its paired close has ACTUALLY completed. The executor's own + // same-batch correlation guard (agent-action-executor.ts's coupledCloseOutcome) only ever sees a close + // staged in the SAME plan -- but this queue stages one action per row (createPendingAgentActionIfAbsent + // conflicts on actionClass), so an independently rejected/never-accepted close has nothing correlating it to + // this label here. Re-derive it from the audit trail -- the ONE record every execution entry point (live + // pass, breaker downgrade, this same accept path) writes through, regardless of which path the close took. + // `autonomyClass` is the disambiguator that keeps this from misfiring on a label-only enforcement tier (e.g. + // maybePlanCopycatLabel's copycat "label" gate mode, which sets closeKind with NO autonomyClass at all, + // since it rides its own generic `label` autonomy and never has -- or needs -- a paired close). + if (pending.actionClass === "label" && pending.params.autonomyClass === "close" && pending.params.closeKind !== undefined) { + const pairedCloseCompleted = (await latestCompletedAuditEventDetail(env, "loopover", "agent.action.close", targetKey)) !== null; + if (!pairedCloseCompleted) { + await setPendingAgentActionStatus(env, pending.id, { status: "rejected", decidedBy: input.decidedBy }); + await recordAuditEvent(env, { + eventType: "agent.pending_action.superseded", + actor: input.decidedBy, + targetKey, + outcome: "denied", + detail: `superseded ${pending.params.closeKind} label: its paired close has not completed — applying it would brand a PR that remains open`, + metadata: baseMetadata, + }); + return { status: "rejected", action: { ...pending, status: "rejected", decidedBy: input.decidedBy }, executionOutcome: "paired_close_not_completed" }; + } + } + // accept → execute the staged action live, then record the result. const [settings, pr, installation] = await Promise.all([ resolveRepositorySettings(env, pending.repoFullName), @@ -422,6 +455,38 @@ export async function decidePendingAgentAction(env: Env, input: { id: string; de .then((branchProtectionContexts) => mergeRequiredCiContexts(branchProtectionContexts, settings.expectedCiContexts)) .catch(() => mergeRequiredCiContexts(null, settings.expectedCiContexts)); + // #9159: pre-merge contributor-cap re-check, threaded into the accept-replay path for the first time -- the + // executor's own step-8c re-check (agent-action-executor.ts) only ever runs when THIS field is set, and this + // accept path previously never set it at all, so a staged merge accepted here skipped the #7284 re-check + // entirely. Constructed the SAME way runAgentMaintenancePlanAndExecute (src/queue/processors.ts) gates its + // own live-path recheck -- only when the POST-downgrade plan still contains a merge, a per-repo cap is + // configured, and the author isn't owner/admin/automation-bot/exempt -- so "who gets rechecked" never drifts + // between the live webhook path and this replay path. + let contributorCapMergeRecheck: (() => Promise) | undefined; + if (plan.some((action) => action.actionClass === "merge") && pr && typeof settings.contributorOpenPrCap === "number" && pr.authorLogin && !isAutoCloseExempt(pr.authorLogin, settings.autoCloseExemptLogins)) { + const repoOwner = pending.repoFullName.includes("/") ? pending.repoFullName.slice(0, pending.repoFullName.indexOf("/")) : ""; + const authorIsOwner = pr.authorLogin.toLowerCase() === repoOwner.toLowerCase(); + const authorIsAutomationBot = isProtectedAutomationAuthor(pr.authorLogin, env); + const authorIsAdmin = await isPerTenantAdmin(env, pending.installationId, pending.repoFullName, pr.authorLogin); + if (!authorIsOwner && !authorIsAdmin && !authorIsAutomationBot) { + const isNewAccount = await isBelowAccountAgeThreshold(env, pending.installationId, pr.authorLogin, settings.accountAgeThresholdDays); + contributorCapMergeRecheck = async () => { + const recheckMatch = await resolvePerRepoContributorCapMatch( + env, + `approval-accept:${pending.repoFullName}#${pending.pullNumber}`, + pending.installationId, + pending.repoFullName, + pr, + settings, + executionCiToken, + executionAdmissionKey, + isNewAccount, + ); + return recheckMatch?.matched !== true; + }; + } + } + const outcomes = await executeAgentMaintenanceActions( env, { @@ -441,6 +506,10 @@ export async function decidePendingAgentAction(env: Env, input: { id: string; de // approval (close autonomy = auto_with_approval), so the accept-replay path needs this resolved the // same way the live webhook path does (src/queue/processors.ts) for the cancel hook to fire here too. contributorCapCancelCi: settings.contributorCapCancelCi ?? env.CONTRIBUTOR_CAP_CANCEL_CI_DEFAULT === "true", + // #9159: see contributorCapMergeRecheck's own construction above -- absent (undefined) exactly when the + // live path would also leave it absent (no cap configured, no merge in the post-downgrade plan, or an + // exempt author), zero added cost in the common case. + contributorCapMergeRecheck, // #selfhost-ci-verification: the executor's OWN final pre-mutation live-CI re-check (step 8 of // executeAgentMaintenanceActions) needs the same effective required contexts this accept-time re-check // (above) evaluated against. Re-fetch so branch-protection changes remain authoritative at accept time. diff --git a/src/settings/agent-actions.ts b/src/settings/agent-actions.ts index bef03d220..aecf2875e 100644 --- a/src/settings/agent-actions.ts +++ b/src/settings/agent-actions.ts @@ -652,8 +652,23 @@ export function downgradeCloseToHold( (noConcreteEvidenceUnderProjectBreaker(action) || (action.closeConcreteEvidence === true && everyJustifyingCodeUntrustworthy(action))); if (!planned.some(isDowngradableClose)) return planned; const labels = resolveAgentDispositionLabels(labelSettings); - // Drop ONLY the downgradable close(s); a deterministic linked-issue-hard-rule close (if any) is left intact. - const next = planned.filter((action) => !isDowngradableClose(action)); + // #9158 (label-close-split-brain, breaker-downgrade half): dropping a close here must ALSO drop any label + // COUPLED to it -- the anti-abuse label pushed alongside a blacklist/contributor_cap/review_nag/copycat + // close carries the SAME closeKind and is inseparable metadata on that close (see planContributorCapClose's/ + // the blacklist/review-nag/copycat closes' own doc comments: close is always pushed BEFORE its label, same + // batch). The executor's own #label-close-split-brain correlation guard (agent-action-executor.ts's + // coupledCloseOutcome) only fires when the close is STILL in the plan and denied/errored this pass -- once + // the close is removed from the plan entirely (exactly what downgrading does), there is nothing left for + // that guard to correlate against, so it silently lets the orphaned label through. Filtering it out HERE, + // at the one place the close is actually dropped, closes that gap without needing the executor to guess + // whether an absent close means "never coupled" (e.g. maybePlanCopycatLabel's label-only copycat tier, which + // legitimately has no close to drop) or "was coupled but just got downgraded". + const droppedCloseKinds = new Set(planned.filter(isDowngradableClose).map((action) => action.closeKind).filter((kind): kind is NonNullable => kind !== undefined)); + const isOrphanedCoupledLabel = (action: PlannedAgentAction): boolean => action.actionClass === "label" && action.closeKind !== undefined && droppedCloseKinds.has(action.closeKind); + // Drop the downgradable close(s) AND any label coupled to one of them; a deterministic linked-issue-hard-rule + // close/label pair (if any) is left intact -- isDowngradableClose never matches that closeKind (see this + // function's own doc comment above), so it can never end up in droppedCloseKinds. + const next = planned.filter((action) => !isDowngradableClose(action) && !isOrphanedCoupledLabel(action)); // The dropped close means the PR is held for a person — surface the manual-review label. Idempotent: only add when // absent (e.g. a guarded-but-passing plan may already carry it). NEVER adds a merge/approve. const alreadyNeedsReview = labels.manualReview !== null && next.some((action) => action.actionClass === "label" && action.label === labels.manualReview && action.labelOp !== "remove"); diff --git a/test/unit/agent-actions.test.ts b/test/unit/agent-actions.test.ts index 3b9fe75b3..513112329 100644 --- a/test/unit/agent-actions.test.ts +++ b/test/unit/agent-actions.test.ts @@ -1822,6 +1822,54 @@ describe("downgradeCloseToHold — close-precision circuit-breaker (#close-preci const label = held.find((a) => a.actionClass === "label" && a.label === AGENT_LABEL_NEEDS_REVIEW && a.labelOp === "add"); expect(label?.autonomyClass).toBe("close"); }); + + describe("#9158 (label-close-split-brain, breaker-downgrade half): dropping a downgradable close also drops its coupled label", () => { + it("REGRESSION: downgrading a review_nag close also drops the review_nag label coupled to it via closeKind", () => { + const reviewNagClose: PlannedAgentAction = { actionClass: "close", requiresApproval: false, reason: "review nag", closeKind: "review_nag" }; + const reviewNagLabel: PlannedAgentAction = { actionClass: "label", requiresApproval: false, reason: "review nag", closeKind: "review_nag", label: DEFAULT_REVIEW_NAG_LABEL, labelOp: "add", autonomyClass: "close" }; + const held = downgradeCloseToHold([reviewNagClose, reviewNagLabel], true); + expect(held.some((a) => a.actionClass === "close" && a.closeKind === "review_nag")).toBe(false); + // The orphaned coupled label must NOT survive the downgrade -- applying it would brand a PR that stays open. + expect(held.some((a) => a.actionClass === "label" && a.closeKind === "review_nag")).toBe(false); + // Manual-review hold still substitutes in for the dropped close, same as any other downgrade. + expect(held.some((a) => a.actionClass === "label" && a.label === AGENT_LABEL_NEEDS_REVIEW && a.labelOp === "add")).toBe(true); + }); + + it("REGRESSION: downgrading a copycat close also drops the copycat label coupled to it, leaving an UNCOUPLED label untouched", () => { + const copycatClose: PlannedAgentAction = { actionClass: "close", requiresApproval: false, reason: "copycat match", closeKind: "copycat" }; + const copycatLabel: PlannedAgentAction = { actionClass: "label", requiresApproval: false, reason: "copycat match", closeKind: "copycat", label: DEFAULT_COPYCAT_LABEL, labelOp: "add", autonomyClass: "close" }; + // An unrelated label with NO closeKind at all (e.g. a plain disposition-communication label) must survive + // untouched -- only a label whose closeKind matches one of the DROPPED closes is orphaned. + const unrelatedLabel: PlannedAgentAction = { actionClass: "label", requiresApproval: false, reason: "ready", label: AGENT_LABEL_READY, labelOp: "add" }; + const held = downgradeCloseToHold([copycatClose, copycatLabel, unrelatedLabel], true); + expect(held.some((a) => a.actionClass === "close" && a.closeKind === "copycat")).toBe(false); + expect(held.some((a) => a.actionClass === "label" && a.closeKind === "copycat")).toBe(false); + expect(held.some((a) => a.actionClass === "label" && a.label === AGENT_LABEL_READY)).toBe(true); + }); + + it("a label-only copycat gate tier (closeKind set but NO matching close in the plan at all) is left untouched -- it was never coupled to a close", () => { + // maybePlanCopycatLabel's label-only tier: a copycat label with closeKind set but no close of that kind + // anywhere in the plan. droppedCloseKinds is empty (no downgradable close present), so isOrphanedCoupledLabel + // can never match this label, and the plan is returned as-is (no heuristic/downgradable close present). + const copycatLabelOnly: PlannedAgentAction = { actionClass: "label", requiresApproval: false, reason: "copycat match (label tier)", closeKind: "copycat", label: DEFAULT_COPYCAT_LABEL, labelOp: "add" }; + const plan = [copycatLabelOnly]; + const held = downgradeCloseToHold(plan, true); + expect(held).toBe(plan); // no downgradable close anywhere → whole plan returned unchanged + expect(held.some((a) => a.actionClass === "label" && a.closeKind === "copycat")).toBe(true); + }); + + it("a deterministic linked-issue-hard-rule close/label pair is left intact even while a sibling review_nag close/label pair is downgraded", () => { + const linkedIssueClose: PlannedAgentAction = { actionClass: "close", requiresApproval: false, reason: "ineligible issue", closeKind: "linked-issue-hard-rule" }; + const linkedIssueLabel: PlannedAgentAction = { actionClass: "label", requiresApproval: false, reason: "ineligible issue", closeKind: "linked-issue-hard-rule", label: "maintainer-only", labelOp: "add" }; + const reviewNagClose: PlannedAgentAction = { actionClass: "close", requiresApproval: false, reason: "review nag", closeKind: "review_nag" }; + const reviewNagLabel: PlannedAgentAction = { actionClass: "label", requiresApproval: false, reason: "review nag", closeKind: "review_nag", label: DEFAULT_REVIEW_NAG_LABEL, labelOp: "add", autonomyClass: "close" }; + const held = downgradeCloseToHold([linkedIssueClose, linkedIssueLabel, reviewNagClose, reviewNagLabel], true); + expect(held.some((a) => a.actionClass === "close" && a.closeKind === "linked-issue-hard-rule")).toBe(true); + expect(held.some((a) => a.actionClass === "label" && a.closeKind === "linked-issue-hard-rule")).toBe(true); + expect(held.some((a) => a.actionClass === "close" && a.closeKind === "review_nag")).toBe(false); + expect(held.some((a) => a.actionClass === "label" && a.closeKind === "review_nag")).toBe(false); + }); + }); }); describe("closeConcreteEvidence — concrete-evidence exemption from the close-precision breaker (#hard-blockers-not-ai-judgment)", () => { diff --git a/test/unit/agent-approval-queue.test.ts b/test/unit/agent-approval-queue.test.ts index 7f2b4b06a..a9653ac7f 100644 --- a/test/unit/agent-approval-queue.test.ts +++ b/test/unit/agent-approval-queue.test.ts @@ -74,6 +74,7 @@ import { getPendingAgentAction, listNotificationDeliveriesForRecipient, listPendingAgentActions, + recordAuditEvent, setPendingAgentActionStatus, upsertGlobalContributorBlacklist, upsertInstallation, @@ -1703,3 +1704,105 @@ describe("agent approval queue (#779)", () => { expect(await countPendingAgentActions(env, { repoFullName: "owner/repo", status: "pending" })).toBe(201); }); }); + +describe("#9158 (label-close-split-brain, approval-queue half): a coupled enforcement label may not post unless its paired close completed", () => { + it("REGRESSION: accept REJECTS a staged label coupled to a close (autonomyClass close + closeKind) when no paired close has EVER completed for this PR", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: "x" }); + await upsertRepositorySettings(env, { repoFullName: "owner/repo", autonomy: { label: "auto_with_approval" } }); + await seedInstallation(env); + await upsertPullRequestFromGitHub(env, "owner/repo", { number: 7, title: "PR", state: "open", user: { login: "plagiarist" }, head: { sha: "h7" }, labels: [], body: "x" }); + const { action } = await createPendingAgentActionIfAbsent(env, { + repoFullName: "owner/repo", + pullNumber: 7, + installationId: 5, + actionClass: "label", + autonomyLevel: "auto_with_approval", + params: { autonomyClass: "close", closeKind: "blacklist", label: "blacklisted-contributor", labelOp: "add" }, + reason: "blacklisted contributor", + }); + + const result = await decidePendingAgentAction(env, { id: action.id, decision: "accept", decidedBy: "owner" }); + + expect(result.status).toBe("rejected"); + expect(result.executionOutcome).toBe("paired_close_not_completed"); + expect((await getPendingAgentAction(env, action.id))?.status).toBe("rejected"); + const { ensurePullRequestLabel: labelNotApplied } = await import("../../src/github/labels"); + expect(labelNotApplied).not.toHaveBeenCalled(); + const superseded = await env.DB.prepare("select outcome, detail from audit_events where event_type = 'agent.pending_action.superseded' and target_key = ?") + .bind("owner/repo#7") + .first<{ outcome: string; detail: string }>(); + expect(superseded).toMatchObject({ outcome: "denied" }); + expect(superseded?.detail).toContain("paired close has not completed"); + }); + + it("accept EXECUTES a staged coupled label once its paired close has actually completed (real audit trail)", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: "x" }); + // #label-scoping: this label's autonomyClass is "close" (it rides the close autonomy dial, not the + // generic label one), so accept-time re-verification checks ctx.autonomy.close, not ctx.autonomy.label. + await upsertRepositorySettings(env, { repoFullName: "owner/repo", autonomy: { close: "auto_with_approval" } }); + await seedInstallation(env); + await upsertPullRequestFromGitHub(env, "owner/repo", { number: 7, title: "PR", state: "open", user: { login: "plagiarist" }, head: { sha: "h7" }, labels: [], body: "x" }); + // The paired close's own completion is recorded through the SAME audit trail every execution entry point + // writes through (agent-action-executor.ts's `audit()`), regardless of which path the close took. + await recordAuditEvent(env, { eventType: "agent.action.close", actor: "loopover", targetKey: "owner/repo#7", outcome: "completed", detail: "closed for blacklist" }); + const { action } = await createPendingAgentActionIfAbsent(env, { + repoFullName: "owner/repo", + pullNumber: 7, + installationId: 5, + actionClass: "label", + autonomyLevel: "auto_with_approval", + params: { autonomyClass: "close", closeKind: "blacklist", label: "blacklisted-contributor", labelOp: "add" }, + reason: "blacklisted contributor", + }); + + const result = await decidePendingAgentAction(env, { id: action.id, decision: "accept", decidedBy: "owner" }); + + expect(result.status).toBe("accepted"); + const { ensurePullRequestLabel: labelApplied } = await import("../../src/github/labels"); + expect(labelApplied).toHaveBeenCalled(); + }); + + it("a label NOT coupled to a close (no autonomyClass: 'close', or no closeKind) is unaffected by the pairing check -- e.g. a plain disposition-communication label", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: "x" }); + await upsertRepositorySettings(env, { repoFullName: "owner/repo", autonomy: { label: "auto_with_approval" } }); + await seedInstallation(env); + await upsertPullRequestFromGitHub(env, "owner/repo", { number: 7, title: "PR", state: "open", user: { login: "contributor" }, head: { sha: "h7" }, labels: [], body: "x" }); + const { action } = await createPendingAgentActionIfAbsent(env, { + repoFullName: "owner/repo", + pullNumber: 7, + installationId: 5, + actionClass: "label", + autonomyLevel: "auto_with_approval", + params: { label: "ready-for-review", labelOp: "add" }, + reason: "gate passed", + }); + + const result = await decidePendingAgentAction(env, { id: action.id, decision: "accept", decidedBy: "owner" }); + + expect(result.status).toBe("accepted"); + const { ensurePullRequestLabel: labelApplied } = await import("../../src/github/labels"); + expect(labelApplied).toHaveBeenCalled(); + }); + + it("a label with closeKind set but autonomyClass NOT 'close' (a label-only copycat gate tier riding its own generic label autonomy) is unaffected by the pairing check", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: "x" }); + await upsertRepositorySettings(env, { repoFullName: "owner/repo", autonomy: { label: "auto_with_approval" } }); + await seedInstallation(env); + await upsertPullRequestFromGitHub(env, "owner/repo", { number: 7, title: "PR", state: "open", user: { login: "contributor" }, head: { sha: "h7" }, labels: [], body: "x" }); + const { action } = await createPendingAgentActionIfAbsent(env, { + repoFullName: "owner/repo", + pullNumber: 7, + installationId: 5, + actionClass: "label", + autonomyLevel: "auto_with_approval", + params: { closeKind: "copycat", label: "copycat-match", labelOp: "add" }, + reason: "copycat match (label tier)", + }); + + const result = await decidePendingAgentAction(env, { id: action.id, decision: "accept", decidedBy: "owner" }); + + expect(result.status).toBe("accepted"); + const { ensurePullRequestLabel: labelApplied } = await import("../../src/github/labels"); + expect(labelApplied).toHaveBeenCalled(); + }); +}); From 0ea359dfbbbd26c6a2ab53e9a25856b3ec37a25a Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Mon, 27 Jul 2026 01:18:09 -0700 Subject: [PATCH 5/6] fix(orb): hold the contributor-cap lock across the merge mutation, not just the re-check The per-author contributor-cap mutex was released before the merge it guards, reopening the #7284 TOCTOU: a concurrent cap-close could claim the just-released lock, live-verify the merging PR as still open, and wrong-close it for a cap the merge was about to relieve milliseconds later. The lock now spans the actual merge mutation via a finally that wraps performMutation, matching the sibling cap-close path. The approval-queue accept path also builds its own contributorCapMergeRecheck now, using the same resolvePerRepoContributorCapMatch the live webhook path uses -- previously an accepted staged merge skipped the pre-merge re-check entirely. Closes #9159 --- src/services/agent-action-executor.ts | 193 +++++++++++++----------- test/unit/agent-action-executor.test.ts | 32 ++++ test/unit/agent-approval-queue.test.ts | 75 +++++++++ 3 files changed, 210 insertions(+), 90 deletions(-) diff --git a/src/services/agent-action-executor.ts b/src/services/agent-action-executor.ts index 2abc8b7d8..1b237826e 100644 --- a/src/services/agent-action-executor.ts +++ b/src/services/agent-action-executor.ts @@ -729,13 +729,104 @@ export async function executeAgentMaintenanceActions(env: Env, ctx: AgentActionE }).catch(() => undefined); } } - // 8c) pre-merge contributor-cap re-check (#7284-fix, TOCTOU race): confirmed live -- a PR whose OWN earlier - // cap check passed can still merge after a sibling's later cap-close made the author over cap, because each - // PR's cap check runs independently with no shared lock. Re-verify right before the irreversible write, - // under the SAME per-author mutex a concurrent sibling's cap-close/wake also acquires, so the two can never - // both act on a stale view of the author's open-PR count. `ctx.contributorCapMergeRecheck` is only ever set - // by the caller when a cap is actually configured for this repo (the common case leaves it undefined) — - // zero added cost, byte-identical to before this field existed. + // 9) live — perform the real mutation, recording success or the error. Wrapped in a closure so 8c below can + // hold the contributor-cap lock across this call for a merge (see 8c's own doc comment for why). + const performMutation = async (): Promise => { + try { + const detailOverride = await performAction(env, ctx, action); + await audit("completed", detailOverride ?? action.reason); + // #9134: every completed merge/close emits a decision record + chained ledger row, by construction -- + // see DecisionRecordContext's doc comment on why this lives HERE instead of at each call site. Never + // throws: recording legibility must never retroactively fail a mutation that already succeeded. + if (action.actionClass === "merge" || action.actionClass === "close") { + await recordCompletedDecision(env, ctx, action).catch(() => undefined); + } + // CI-run cancellation on an anti-abuse close (#2462 contributor_cap; extended to blacklist #6659): stop + // burning CI minutes on a PR that was just closed for exceeding the contributor cap, or for a banned + // login. contributor_cap stays opt-in (contributorCapCancelCi) since a repo may want the cap to bite + // without touching CI; blacklist is unconditional -- there is no scenario where a maintainer wants a + // permanently-banned login's CI to keep running after the close. Best-effort, AFTER the close already + // succeeded -- cancelInFlightWorkflowRunsForHeadSha never throws, so a missing actions:write grant (or + // any other failure here) can never retroactively turn this already-successful close into a recorded + // "error" by escaping into the catch block below. + if (action.actionClass === "close" && ctx.headSha) { + if (action.closeKind === "contributor_cap" && ctx.contributorCapCancelCi) { + await recordCiCancelOutcome(env, "contributor_cap", ctx, ctx.headSha); + } else if (action.closeKind === "blacklist") { + await recordCiCancelOutcome(env, "blacklist", ctx, ctx.headSha); + } + } + // Re-approval idempotency: record the head SHA we just approved so the planner skips re-approving this + // exact commit on the next sweep (a GitHub App's own approval does not reliably flip reviewDecision to + // APPROVED, so reviewDecision alone can't dedup). A new commit clears the match → the bot approves it. + // Best-effort: a failed persist only risks one redundant re-approval, never a wrong disposition. + if (action.actionClass === "approve" && !action.dismissStaleApproval && ctx.headSha) { + await markPullRequestApproved(env, ctx.repoFullName, ctx.pullNumber, ctx.headSha).catch(() => undefined); + } + // Per-repo Discord notification on a terminal/visible action (reviewbot parity): merge→merged, + // close→closed, request_changes→manual review. Best-effort; never affects the action. RC1 dedups at the + // action level, so this fires once per outcome per PR (no spam). + const notifyOutcome: NotifyOutcome | null = + action.actionClass === "merge" ? "merged" : action.actionClass === "close" ? "closed" : action.actionClass === "request_changes" ? "manual" : null; + if (notifyOutcome) { + // #6636: enrich the notification with the AI's actual gate-verdict reasoning (the latest recorded + // gate_decision summary for this PR) instead of only the plain disposition reason — resolveDispositionReason + // falls back to `action.reason` when no verdict is on record or the read fails, so this is byte-identical + // when there's nothing to enrich with. review_audit keys gate_decision rows by `${repoFullName}#${number}`. + const summary = await resolveDispositionReason(env, `${ctx.repoFullName}#${ctx.pullNumber}`, action.reason); + const notifyParams = { repoFullName: ctx.repoFullName, pullNumber: ctx.pullNumber, outcome: notifyOutcome, summary, submitter: ctx.authorLogin }; + await notifyActionToDiscord(env, notifyParams).catch(() => undefined); + await notifyActionToSlack(env, notifyParams).catch(() => undefined); + } + } catch (error) { + await audit("error", errorMessage(error)); + // RC3 terminal-fail merges: immediate terminal failures (401/405/409/conflict) are marked once; generic + // GitHub 403s are retryable first because branch-protection/check/conversation state can converge shortly + // after the gate publishes. A possibly-transient failure is retried up to MERGE_RETRY_CAP, then held. + if (action.actionClass === "merge" && ctx.headSha) { + await handleMergeFailure(env, ctx, error); + } else if (action.actionClass === "update_branch" && isMergeConflictMessage(errorMessage(error))) { + // LOOPOVER-24: update_branch performs a real merge internally, so it fails with the same "merge + // conflict" shape a MERGE action does -- but unlike a merge's terminal hold (a PR permanently blocked + // until a human intervenes), this is NOT a stuck state: forceUpdateBranch's caller (prReadyForReview) + // already falls through to reviewing the PR on its current, non-rebased head when this returns false + // (see forceUpdateBranch's own doc comment), exactly like every other "couldn't rebase, review anyway" + // path. The branch owner, not the bot, needs to resolve the conflict -- paging on every naturally- + // diverged PR this happens to hit isn't warranted. Still recorded by the audit() call above. + } else if (action.actionClass === "update_branch" && isNoNewBaseCommitsMessage(errorMessage(error))) { + // LOOPOVER-24 (regressed shape): a 422 "There are no new commits on the base branch." means the head + // was already up to date when update-branch fired -- the readiness check acted on a stale/cached + // mergeable_state read. Nothing went wrong and nothing is stuck: the caller falls through to reviewing + // the current head exactly as in the conflict case above. Audit-only; never a PostHog page. + } else { + // Non-merge action classes have no retry loop -- a single failure here is already this pass's terminal + // outcome (the planner may re-attempt on the next sweep if the underlying condition clears itself), so + // it is captured immediately rather than only on eventual exhaustion. Mirrors handleMergeFailure's own + // terminal-hold capture below and the "a real failure the maintainer must see" convention already used + // for review-pass failures (selfhost/posthog.ts's capturePostHogReviewFailure, queue/processors.ts). + // Previously this class of failure was audit-log-only, invisible without a manual audit_events query. + capturePostHogError(error, { kind: "agent_action_execution_failed", repo: ctx.repoFullName, pr: ctx.pullNumber, installationId: ctx.installationId, actionClass: action.actionClass }, "agent_action_execution_failed"); + } + // #2265: a permission-looking 403 on a PR-write mutation can mean the LOCAL installations.permissions + // snapshot is stale after a maintainer-initiated downgrade (GitHub sends no downgrade webhook). Rate-limit + // 403s and operation-specific forbidden states are not permission evidence, and this refresh scans broad + // installation state, so keep the hot error path narrowly filtered and per-installation cooled down. + if (PR_WRITE_CLASSES.has(action.actionClass) && shouldRefreshInstallationHealthAfterPrWriteFailure(ctx.installationId, error)) { + await refreshInstallationHealthForInstallation(env, ctx.installationId).catch(() => undefined); + } + } + }; + // 8c) pre-merge contributor-cap re-check (#7284-fix / #9159, TOCTOU race): confirmed live -- a PR whose OWN + // earlier cap check passed can still merge after a sibling's later cap-close made the author over cap, + // because each PR's cap check runs independently with no shared lock. Re-verify right before the + // irreversible write, under the SAME per-author mutex a concurrent sibling's cap-close/wake also acquires, + // so the two can never both act on a stale view of the author's open-PR count. #9159: the mutex must span + // the ACTUAL merge mutation (step 9) too, not just this re-check read -- releasing it beforehand (as this + // used to) reopens the exact #7284 window the lock exists to close: a concurrent cap-close claims the + // just-released lock, live-verifies this PR as still open, and wrong-closes it for a cap this merge was + // about to relieve milliseconds later. `ctx.contributorCapMergeRecheck` is only ever set by the caller when + // a cap is actually configured for this repo (the common case leaves it undefined) — zero added cost, + // byte-identical to before this field existed. if (action.actionClass === "merge" && ctx.contributorCapMergeRecheck) { const authorLogin = ctx.authorLogin ?? ""; const { acquired, ownerToken } = await claimContributorCapLock(env, ctx.repoFullName, authorLogin); @@ -751,94 +842,16 @@ export async function executeAgentMaintenanceActions(env: Env, ctx: AgentActionE await audit("denied", `contributor cap re-check confirmed ${authorLogin} is now over cap on ${ctx.repoFullName} — action not executed`); continue; } + // #9159: the lock stays held across the merge mutation itself -- only released in the `finally` below, + // AFTER performMutation has run to completion (success or failure) -- see this block's header comment. + await performMutation(); } finally { await releaseContributorCapLock(env, ctx.repoFullName, authorLogin, ownerToken); } + continue; } // 9) live — perform the real mutation, recording success or the error. - try { - const detailOverride = await performAction(env, ctx, action); - await audit("completed", detailOverride ?? action.reason); - // #9134: every completed merge/close emits a decision record + chained ledger row, by construction -- - // see DecisionRecordContext's doc comment on why this lives HERE instead of at each call site. Never - // throws: recording legibility must never retroactively fail a mutation that already succeeded. - if (action.actionClass === "merge" || action.actionClass === "close") { - await recordCompletedDecision(env, ctx, action).catch(() => undefined); - } - // CI-run cancellation on an anti-abuse close (#2462 contributor_cap; extended to blacklist #6659): stop - // burning CI minutes on a PR that was just closed for exceeding the contributor cap, or for a banned - // login. contributor_cap stays opt-in (contributorCapCancelCi) since a repo may want the cap to bite - // without touching CI; blacklist is unconditional -- there is no scenario where a maintainer wants a - // permanently-banned login's CI to keep running after the close. Best-effort, AFTER the close already - // succeeded -- cancelInFlightWorkflowRunsForHeadSha never throws, so a missing actions:write grant (or - // any other failure here) can never retroactively turn this already-successful close into a recorded - // "error" by escaping into the catch block below. - if (action.actionClass === "close" && ctx.headSha) { - if (action.closeKind === "contributor_cap" && ctx.contributorCapCancelCi) { - await recordCiCancelOutcome(env, "contributor_cap", ctx, ctx.headSha); - } else if (action.closeKind === "blacklist") { - await recordCiCancelOutcome(env, "blacklist", ctx, ctx.headSha); - } - } - // Re-approval idempotency: record the head SHA we just approved so the planner skips re-approving this - // exact commit on the next sweep (a GitHub App's own approval does not reliably flip reviewDecision to - // APPROVED, so reviewDecision alone can't dedup). A new commit clears the match → the bot approves it. - // Best-effort: a failed persist only risks one redundant re-approval, never a wrong disposition. - if (action.actionClass === "approve" && !action.dismissStaleApproval && ctx.headSha) { - await markPullRequestApproved(env, ctx.repoFullName, ctx.pullNumber, ctx.headSha).catch(() => undefined); - } - // Per-repo Discord notification on a terminal/visible action (reviewbot parity): merge→merged, - // close→closed, request_changes→manual review. Best-effort; never affects the action. RC1 dedups at the - // action level, so this fires once per outcome per PR (no spam). - const notifyOutcome: NotifyOutcome | null = - action.actionClass === "merge" ? "merged" : action.actionClass === "close" ? "closed" : action.actionClass === "request_changes" ? "manual" : null; - if (notifyOutcome) { - // #6636: enrich the notification with the AI's actual gate-verdict reasoning (the latest recorded - // gate_decision summary for this PR) instead of only the plain disposition reason — resolveDispositionReason - // falls back to `action.reason` when no verdict is on record or the read fails, so this is byte-identical - // when there's nothing to enrich with. review_audit keys gate_decision rows by `${repoFullName}#${number}`. - const summary = await resolveDispositionReason(env, `${ctx.repoFullName}#${ctx.pullNumber}`, action.reason); - const notifyParams = { repoFullName: ctx.repoFullName, pullNumber: ctx.pullNumber, outcome: notifyOutcome, summary, submitter: ctx.authorLogin }; - await notifyActionToDiscord(env, notifyParams).catch(() => undefined); - await notifyActionToSlack(env, notifyParams).catch(() => undefined); - } - } catch (error) { - await audit("error", errorMessage(error)); - // RC3 terminal-fail merges: immediate terminal failures (401/405/409/conflict) are marked once; generic - // GitHub 403s are retryable first because branch-protection/check/conversation state can converge shortly - // after the gate publishes. A possibly-transient failure is retried up to MERGE_RETRY_CAP, then held. - if (action.actionClass === "merge" && ctx.headSha) { - await handleMergeFailure(env, ctx, error); - } else if (action.actionClass === "update_branch" && isMergeConflictMessage(errorMessage(error))) { - // LOOPOVER-24: update_branch performs a real merge internally, so it fails with the same "merge - // conflict" shape a MERGE action does -- but unlike a merge's terminal hold (a PR permanently blocked - // until a human intervenes), this is NOT a stuck state: forceUpdateBranch's caller (prReadyForReview) - // already falls through to reviewing the PR on its current, non-rebased head when this returns false - // (see forceUpdateBranch's own doc comment), exactly like every other "couldn't rebase, review anyway" - // path. The branch owner, not the bot, needs to resolve the conflict -- paging on every naturally- - // diverged PR this happens to hit isn't warranted. Still recorded by the audit() call above. - } else if (action.actionClass === "update_branch" && isNoNewBaseCommitsMessage(errorMessage(error))) { - // LOOPOVER-24 (regressed shape): a 422 "There are no new commits on the base branch." means the head - // was already up to date when update-branch fired -- the readiness check acted on a stale/cached - // mergeable_state read. Nothing went wrong and nothing is stuck: the caller falls through to reviewing - // the current head exactly as in the conflict case above. Audit-only; never a PostHog page. - } else { - // Non-merge action classes have no retry loop -- a single failure here is already this pass's terminal - // outcome (the planner may re-attempt on the next sweep if the underlying condition clears itself), so - // it is captured immediately rather than only on eventual exhaustion. Mirrors handleMergeFailure's own - // terminal-hold capture below and the "a real failure the maintainer must see" convention already used - // for review-pass failures (selfhost/posthog.ts's capturePostHogReviewFailure, queue/processors.ts). - // Previously this class of failure was audit-log-only, invisible without a manual audit_events query. - capturePostHogError(error, { kind: "agent_action_execution_failed", repo: ctx.repoFullName, pr: ctx.pullNumber, installationId: ctx.installationId, actionClass: action.actionClass }, "agent_action_execution_failed"); - } - // #2265: a permission-looking 403 on a PR-write mutation can mean the LOCAL installations.permissions - // snapshot is stale after a maintainer-initiated downgrade (GitHub sends no downgrade webhook). Rate-limit - // 403s and operation-specific forbidden states are not permission evidence, and this refresh scans broad - // installation state, so keep the hot error path narrowly filtered and per-installation cooled down. - if (PR_WRITE_CLASSES.has(action.actionClass) && shouldRefreshInstallationHealthAfterPrWriteFailure(ctx.installationId, error)) { - await refreshInstallationHealthForInstallation(env, ctx.installationId).catch(() => undefined); - } - } + await performMutation(); } await maybeEscalateModeration(env, { installationId: ctx.installationId, repoFullName: ctx.repoFullName, number: ctx.pullNumber, authorLogin: ctx.authorLogin, mode, moderationSettings: ctx.moderationSettings }, planned, outcomes); diff --git a/test/unit/agent-action-executor.test.ts b/test/unit/agent-action-executor.test.ts index f7a0d0f05..061df6747 100644 --- a/test/unit/agent-action-executor.test.ts +++ b/test/unit/agent-action-executor.test.ts @@ -2507,6 +2507,38 @@ describe("pre-merge contributor-cap re-check (#7284-fix, TOCTOU race)", () => { expect(outcomes[0]?.outcome).toBe("completed"); expect(mergePullRequest).toHaveBeenCalled(); }); + + it("REGRESSION (#9159): the lock stays held across the ACTUAL merge mutation, not just the re-check -- a concurrent claim attempt made WHILE the merge is in flight is denied", async () => { + const env = createTestEnv({}); + const recheck = vi.fn(async () => true); + let claimWhileMerging: boolean | undefined; + vi.mocked(mergePullRequest).mockImplementationOnce(async () => { + // Simulate a concurrent sibling's cap-close/wake trying to claim the SAME per-author lock while this + // merge mutation is still in flight -- before #9159's fix, the lock was already released at this point + // (releaseContributorCapLock ran in a `finally` wrapping only the re-check, not the merge below it), so + // this claim would have wrongly succeeded and let a concurrent cap-close act on a stale view. + claimWhileMerging = await env.SELFHOST_TRANSIENT_CACHE?.claim?.("contributor-cap-lock:owner/repo:farmer99", "concurrent-cap-close-token", 30); + return { merged: true, sha: "merged-sha", suppressed: false }; + }); + const outcomes = await executeAgentMaintenanceActions(env, ctx({ authorLogin: "farmer99", contributorCapMergeRecheck: recheck }), [merge]); + expect(outcomes[0]?.outcome).toBe("completed"); + expect(claimWhileMerging).toBe(false); // the concurrent claim attempt was denied -- the lock was still held + // Released only AFTER the merge completed: a fresh claim attempt now succeeds. + const claimAfterMerge = await env.SELFHOST_TRANSIENT_CACHE?.claim?.("contributor-cap-lock:owner/repo:farmer99", "probe-token", 30); + expect(claimAfterMerge).toBe(true); + }); + + it("REGRESSION (#9159): the lock is released even when the merge mutation itself throws (finally spans success AND failure)", async () => { + const env = createTestEnv({}); + const recheck = vi.fn(async () => true); + vi.mocked(mergePullRequest).mockImplementationOnce(async () => { + throw new Error("merge conflict"); + }); + const outcomes = await executeAgentMaintenanceActions(env, ctx({ authorLogin: "farmer99", contributorCapMergeRecheck: recheck }), [merge]); + expect(outcomes[0]?.outcome).toBe("error"); + const claimAfterFailure = await env.SELFHOST_TRANSIENT_CACHE?.claim?.("contributor-cap-lock:owner/repo:farmer99", "probe-token", 30); + expect(claimAfterFailure).toBe(true); + }); }); // #9134: six of seven executeAgentMaintenanceActions call sites wrote no decision record and no ledger row at diff --git a/test/unit/agent-approval-queue.test.ts b/test/unit/agent-approval-queue.test.ts index a9653ac7f..c92d51dee 100644 --- a/test/unit/agent-approval-queue.test.ts +++ b/test/unit/agent-approval-queue.test.ts @@ -1706,6 +1706,10 @@ describe("agent approval queue (#779)", () => { }); describe("#9158 (label-close-split-brain, approval-queue half): a coupled enforcement label may not post unless its paired close completed", () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + it("REGRESSION: accept REJECTS a staged label coupled to a close (autonomyClass close + closeKind) when no paired close has EVER completed for this PR", async () => { const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: "x" }); await upsertRepositorySettings(env, { repoFullName: "owner/repo", autonomy: { label: "auto_with_approval" } }); @@ -1806,3 +1810,74 @@ describe("#9158 (label-close-split-brain, approval-queue half): a coupled enforc expect(labelApplied).toHaveBeenCalled(); }); }); + +describe("#9159: the accept-replay path threads a contributor-cap merge re-check, closing the TOCTOU the executor's own recheck exists for", () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it("REGRESSION: accept DENIES a staged merge when the author is now over the per-repo contributor-open-PR cap at accept time", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: "x" }); + await upsertRepositorySettings(env, { repoFullName: "owner/repo", autonomy: { merge: "auto_with_approval" } }); + // contributorOpenPrCap is config-as-code only (.loopover.yml), resolved from the focus manifest, not a + // repository_settings DB column (mirrors expectedCiContexts elsewhere in this file). + await upsertRepoFocusManifest(env, "owner/repo", { settings: { contributorOpenPrCap: 1 } }); + await seedInstallation(env); + // The author has ANOTHER open PR in the same repo (#3) -- fetchLivePullRequestState defaults to "open" in + // this suite's mocks, so it live-confirms as still open too. With cap=1, the higher-numbered PR (#7, the + // one about to merge) is the one over cap. + await upsertPullRequestFromGitHub(env, "owner/repo", { number: 3, title: "Sibling PR", state: "open", user: { login: "farmer99" }, head: { sha: "s3" }, labels: [], body: "x" }); + await upsertPullRequestFromGitHub(env, "owner/repo", { number: 7, title: "PR", state: "open", user: { login: "farmer99" }, head: { sha: "h7" }, labels: [], body: "x" }); + const { action } = await createPendingAgentActionIfAbsent(env, { repoFullName: "owner/repo", pullNumber: 7, installationId: 5, actionClass: "merge", autonomyLevel: "auto_with_approval", params: { mergeMethod: "squash", expectedHeadSha: "h7" }, reason: "clean" }); + + const result = await decidePendingAgentAction(env, { id: action.id, decision: "accept", decidedBy: "owner" }); + + expect(result.status).toBe("accepted"); // the maintainer's decision itself always records as accepted... + expect(mergePullRequest).not.toHaveBeenCalled(); // ...but the executor's own re-check denied the mutation + const audit = await env.DB.prepare("select outcome, detail from audit_events where event_type = 'agent.action.merge' order by created_at desc limit 1").first<{ outcome: string; detail: string }>(); + expect(audit?.outcome).toBe("denied"); + expect(audit?.detail).toMatch(/now over cap/); + }); + + it("accept EXECUTES a staged merge when the author remains under the per-repo contributor-open-PR cap at accept time", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: "x" }); + await upsertRepositorySettings(env, { repoFullName: "owner/repo", autonomy: { merge: "auto_with_approval" } }); + await upsertRepoFocusManifest(env, "owner/repo", { settings: { contributorOpenPrCap: 5 } }); + await seedInstallation(env); + await upsertPullRequestFromGitHub(env, "owner/repo", { number: 7, title: "PR", state: "open", user: { login: "farmer99" }, head: { sha: "h7" }, labels: [], body: "x" }); + const { action } = await createPendingAgentActionIfAbsent(env, { repoFullName: "owner/repo", pullNumber: 7, installationId: 5, actionClass: "merge", autonomyLevel: "auto_with_approval", params: { mergeMethod: "squash", expectedHeadSha: "h7" }, reason: "clean" }); + + const result = await decidePendingAgentAction(env, { id: action.id, decision: "accept", decidedBy: "owner" }); + + expect(result.status).toBe("accepted"); + expect(mergePullRequest).toHaveBeenCalled(); + }); + + it("does not build a recheck at all (zero added cost) when no per-repo cap is configured", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: "x" }); + await upsertRepositorySettings(env, { repoFullName: "owner/repo", autonomy: { merge: "auto_with_approval" } }); + await seedInstallation(env); + await upsertPullRequestFromGitHub(env, "owner/repo", { number: 7, title: "PR", state: "open", user: { login: "farmer99" }, head: { sha: "h7" }, labels: [], body: "x" }); + const { action } = await createPendingAgentActionIfAbsent(env, { repoFullName: "owner/repo", pullNumber: 7, installationId: 5, actionClass: "merge", autonomyLevel: "auto_with_approval", params: { mergeMethod: "squash", expectedHeadSha: "h7" }, reason: "clean" }); + + const result = await decidePendingAgentAction(env, { id: action.id, decision: "accept", decidedBy: "owner" }); + + expect(result.status).toBe("accepted"); + expect(mergePullRequest).toHaveBeenCalled(); + }); + + it("does not build a recheck for an author exempt from the auto-close list, even with a cap configured", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: "x" }); + await upsertRepositorySettings(env, { repoFullName: "owner/repo", autonomy: { merge: "auto_with_approval" } }); + await upsertRepoFocusManifest(env, "owner/repo", { settings: { contributorOpenPrCap: 1, autoCloseExemptLogins: ["farmer99"] } }); + await seedInstallation(env); + await upsertPullRequestFromGitHub(env, "owner/repo", { number: 3, title: "Sibling PR", state: "open", user: { login: "farmer99" }, head: { sha: "s3" }, labels: [], body: "x" }); + await upsertPullRequestFromGitHub(env, "owner/repo", { number: 7, title: "PR", state: "open", user: { login: "farmer99" }, head: { sha: "h7" }, labels: [], body: "x" }); + const { action } = await createPendingAgentActionIfAbsent(env, { repoFullName: "owner/repo", pullNumber: 7, installationId: 5, actionClass: "merge", autonomyLevel: "auto_with_approval", params: { mergeMethod: "squash", expectedHeadSha: "h7" }, reason: "clean" }); + + const result = await decidePendingAgentAction(env, { id: action.id, decision: "accept", decidedBy: "owner" }); + + expect(result.status).toBe("accepted"); + expect(mergePullRequest).toHaveBeenCalled(); + }); +}); From e8639e6a2159c0e68aa2f6ab0b44a9c74205ef6c Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Mon, 27 Jul 2026 01:33:09 -0700 Subject: [PATCH 6/6] chore(cf): regenerate worker-configuration.d.ts after rebasing onto main --- worker-configuration.d.ts | 93 +++++++++++++++++++++++++++++++-------- 1 file changed, 74 insertions(+), 19 deletions(-) diff --git a/worker-configuration.d.ts b/worker-configuration.d.ts index 94c3666d0..486ab2b33 100644 --- a/worker-configuration.d.ts +++ b/worker-configuration.d.ts @@ -1,6 +1,6 @@ /* eslint-disable */ // Generated by Wrangler by running `wrangler types` (hash: 12ab7fcab3ef47d0e7c428770fc3c36a) -// Runtime types generated with workerd@1.20260701.1 2026-05-28 nodejs_compat +// Runtime types generated with workerd@1.20260722.1 2026-05-28 nodejs_compat interface __BaseEnv_Env { REVIEW_AUDIT: R2Bucket; VISUAL_CAPTURE_PUBLIC: R2Bucket; @@ -636,7 +636,7 @@ declare abstract class DurableObjectNamespace; jurisdiction(jurisdiction: DurableObjectJurisdiction): DurableObjectNamespace; } -type DurableObjectJurisdiction = "eu" | "fedramp" | "fedramp-high"; +type DurableObjectJurisdiction = "eu" | "fedramp" | "fedramp-high" | "us"; interface DurableObjectNamespaceNewUniqueIdOptions { jurisdiction?: DurableObjectJurisdiction; } @@ -12331,6 +12331,13 @@ interface ForwardableEmailMessage extends EmailMessage { * @returns A promise that resolves when the email message is replied. */ reply(message: EmailMessage): Promise; + /** + * Reply to the sender of this email message with a message built from the given + * fields. Threading headers (In-Reply-To/References) are set automatically. + * @param builder The reply message contents. + * @returns A promise that resolves when the email message is replied. + */ + reply(builder: EmailReplyMessageBuilder): Promise; } /** A file attachment for an email message */ type EmailAttachment = { @@ -12351,23 +12358,46 @@ interface EmailAddress { name: string; email: string; } +/** + * Recipient fields for `SendEmail.send()`. At least one of `to`, `cc`, or + * `bcc` must be provided. + */ +type EmailDestinations = { + to?: string | EmailAddress | (string | EmailAddress)[]; + cc?: string | EmailAddress | (string | EmailAddress)[]; + bcc?: string | EmailAddress | (string | EmailAddress)[]; +} & ({ + to: string | EmailAddress | (string | EmailAddress)[]; +} | { + cc: string | EmailAddress | (string | EmailAddress)[]; +} | { + bcc: string | EmailAddress | (string | EmailAddress)[]; +}); +/** + * Fields shared by all composed emails (no recipients). Used directly by + * `ForwardableEmailMessage.reply()`, which always replies to the original + * sender, and extended by `EmailMessageBuilder` for `SendEmail.send()`. + */ +interface EmailReplyMessageBuilder { + from: string | EmailAddress; + subject: string; + replyTo?: string | EmailAddress; + headers?: Record; + text?: string; + html?: string; + attachments?: EmailAttachment[]; +} +/** + * Fields for composing an email without constructing raw MIME, for + * `SendEmail.send()`. Requires at least one of `to`, `cc`, or `bcc`. + */ +type EmailMessageBuilder = EmailReplyMessageBuilder & EmailDestinations; /** * A binding that allows a Worker to send email messages. */ interface SendEmail { send(message: EmailMessage): Promise; - send(builder: { - from: string | EmailAddress; - to: string | EmailAddress | (string | EmailAddress)[]; - subject: string; - replyTo?: string | EmailAddress; - cc?: string | EmailAddress | (string | EmailAddress)[]; - bcc?: string | EmailAddress | (string | EmailAddress)[]; - headers?: Record; - text?: string; - html?: string; - attachments?: EmailAttachment[]; - }): Promise; + send(builder: EmailMessageBuilder): Promise; } declare abstract class EmailEvent extends ExtendableEvent { readonly message: ForwardableEmailMessage; @@ -13189,6 +13219,11 @@ declare namespace CloudflareWorkersModule { export type WorkflowDurationLabel = 'second' | 'minute' | 'hour' | 'day' | 'week' | 'month' | 'year'; export type WorkflowSleepDuration = `${number} ${WorkflowDurationLabel}${'s' | ''}` | number; export type WorkflowDelayDuration = WorkflowSleepDuration; + export type WorkflowDynamicDelayContext = { + ctx: WorkflowStepContext; + error: Error; + }; + export type WorkflowDelayFunction = (input: WorkflowDynamicDelayContext) => WorkflowDelayDuration | Promise; export type WorkflowTimeoutDuration = WorkflowSleepDuration; export type WorkflowRetentionDuration = WorkflowSleepDuration; export type WorkflowBackoff = 'constant' | 'linear' | 'exponential'; @@ -13196,7 +13231,7 @@ declare namespace CloudflareWorkersModule { export type WorkflowStepConfig = { retries?: { limit: number; - delay: WorkflowDelayDuration | number; + delay: WorkflowDelayDuration | number | WorkflowDelayFunction; backoff?: WorkflowBackoff; }; timeout?: WorkflowTimeoutDuration | number; @@ -13222,13 +13257,22 @@ declare namespace CloudflareWorkersModule { type: string; sensitive?: WorkflowStepSensitivity; }; - export type WorkflowStepContext = { + export type WorkflowStepContext = { step: { name: string; count: number; }; attempt: number; - config: WorkflowStepConfig; + config: { + retries?: { + limit: number; + backoff?: WorkflowBackoff; + } & (Delay extends WorkflowDelayFunction ? {} : { + delay: WorkflowDelayDuration | number; + }); + timeout?: WorkflowTimeoutDuration | number; + sensitive?: WorkflowStepSensitivity; + }; }; export type WorkflowRollbackContext = { ctx: WorkflowStepContext; @@ -13244,7 +13288,9 @@ declare namespace CloudflareWorkersModule { }; export abstract class WorkflowStep { do>(name: string, callback: (ctx: WorkflowStepContext) => Promise, rollbackOptions?: WorkflowStepRollbackOptions): Promise; - do>(name: string, config: WorkflowStepConfig, callback: (ctx: WorkflowStepContext) => Promise, rollbackOptions?: WorkflowStepRollbackOptions): Promise; + do, const C extends WorkflowStepConfig>(name: string, config: C, callback: (ctx: WorkflowStepContext) => Promise, rollbackOptions?: WorkflowStepRollbackOptions): Promise; sleep: (name: string, duration: WorkflowSleepDuration) => Promise; sleepUntil: (name: string, timestamp: Date | number) => Promise; waitForEvent>(name: string, options: { @@ -14154,6 +14200,7 @@ declare namespace TailStream { readonly dispatchNamespace?: string; readonly entrypoint?: string; readonly executionModel: string; + readonly durableObjectId?: string; readonly scriptName?: string; readonly scriptTags?: string[]; readonly scriptVersion?: ScriptVersion; @@ -14708,6 +14755,13 @@ interface WorkflowError { code?: number; message: string; } +interface WorkflowInstanceTerminateOptions { + /** + * If true, run registered rollback handlers before terminating the instance. + * Only steps that registered rollback handlers are rolled back. + */ + rollback?: boolean; +} interface WorkflowInstanceRestartOptions { /** * Restart from a specific step. If omitted, the instance restarts from the beginning. @@ -14741,8 +14795,9 @@ declare abstract class WorkflowInstance { public resume(): Promise; /** * Terminate the instance. If it is errored, terminated or complete, an error will be thrown. + * @param options Options for termination, including whether registered rollback handlers should run. */ - public terminate(): Promise; + public terminate(options?: WorkflowInstanceTerminateOptions): Promise; /** * Restart the instance. Optionally restart from a specific step, preserving * cached results for all steps before it.