From 054a70057ccfe2bc04826e33ca9c2a7f5e4fd523 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 1 Jul 2026 22:26:13 -0700 Subject: [PATCH 1/3] fix(agent-actions): pin staged close actions to a head SHA and re-verify live state at accept time Staged close actions (blacklist, linked-issue-hard-rule, heuristic) never had expectedHeadSha set, unlike approve/merge. The accept-time force-push guard only fires when a pin exists, so an unpinned close's freshness check trivially compared the live head against itself (both sides derive from the same fresh DB read) and could never catch a force-push during the queue wait. - Pin expectedHeadSha on all three close-construction sites, mirroring merge/approve. - Extend the accept-time isUnpinnedRatifyingAction guard to cover close, so a legacy unpinned row (staged before this fix, or from a transiently-null stored head SHA) is refused rather than replayed. - Add a live blacklist-membership re-check specifically for closeKind: "blacklist": the head-SHA pin alone doesn't catch a maintainer removing the contributor from the blacklist between staging and accept, since the head never moves. Re-runs the same pure findBlacklistEntry check the planner uses, against CURRENT settings instead of the plan-time snapshot baked into the sticky pending row. Full re-verification of non-CI heuristic-close reasoning (duplicate- of-open-PR, slop threshold) is intentionally out of scope here: the executor's existing step-6 live-CI re-check already covers the CI half for a heuristic close, and this PR's head-SHA pin now gives it real force-push protection too, matching the same fail-safe level merge/approve already had. Fixes #2452 --- src/services/agent-approval-queue.ts | 47 ++++++++++++---- src/settings/agent-actions.ts | 33 ++++++++++-- test/unit/agent-actions.test.ts | 20 +++++++ test/unit/agent-approval-queue.test.ts | 74 +++++++++++++++++++++++++- 4 files changed, 159 insertions(+), 15 deletions(-) diff --git a/src/services/agent-approval-queue.ts b/src/services/agent-approval-queue.ts index 731c43aaa2..1c9f40fbcd 100644 --- a/src/services/agent-approval-queue.ts +++ b/src/services/agent-approval-queue.ts @@ -3,6 +3,7 @@ import { createInstallationToken } from "../github/app"; import { loadLinkedIssueHardRules, resolveLinkedIssueHardRule } from "../review/linked-issue-hard-rules"; import { executeAgentMaintenanceActions, pendingActionToPlanned } from "./agent-action-executor"; import { downgradeCloseToHold, downgradeMergeToHold, isProtectedAutomationAuthor, type PlannedAgentAction } from "../settings/agent-actions"; +import { findBlacklistEntry } from "../settings/contributor-blacklist"; import { isCloseHoldOnly, isHoldOnly } from "../review/outcomes-wire"; import { fetchLiveCiAggregate, fetchLivePullRequestMergeState, fetchLivePullRequestReviewDecision } from "../github/backfill"; import { githubRateLimitAdmissionKeyForToken } from "../github/client"; @@ -60,20 +61,23 @@ export async function decidePendingAgentAction(env: Env, input: { id: string; de }); return { status: "rejected", action: { ...pending, status: "rejected", decidedBy: input.decidedBy }, executionOutcome: "head_moved" }; } - // An unpinned staged approve or merge (no expectedHeadSha) cannot be safety-verified against a force-push that - // happened during the queue wait. For a PINNED merge, GitHub's `sha` param 409s on mismatch -- a real backstop. - // But that backstop only exists because there's something to compare against; an UNPINNED merge falls back to - // performAction's `mergeSha = action.expectedHeadSha ?? ctx.headSha`, which by construction substitutes - // whatever head is live right now, so it trivially "matches" and no 409 is possible. The reviews API's - // `commit_id` has no server-side staleness rejection at all, pinned or not (#2377). Either way, the check above - // only fires when a pin EXISTS and disagrees with the live head; a row staged with no pin at all (e.g. by code - // predating this head-pinning fix, or a planning pass that ran against a transiently-null stored head SHA) - // would otherwise fall through to the executor's `ctx.headSha` fallback and silently ratify whatever commit is - // live NOW, under the authority of a review/merge that was never actually performed against it (#2422). + // An unpinned staged approve, merge, or close (no expectedHeadSha) cannot be safety-verified against a + // force-push that happened during the queue wait. For a PINNED merge, GitHub's `sha` param 409s on mismatch -- + // a real backstop. But that backstop only exists because there's something to compare against; an UNPINNED + // merge falls back to performAction's `mergeSha = action.expectedHeadSha ?? ctx.headSha`, which by construction + // substitutes whatever head is live right now, so it trivially "matches" and no 409 is possible. The reviews + // API's `commit_id` has no server-side staleness rejection at all, pinned or not (#2377). close has no + // server-side commit target at all -- its OWN freshness relies entirely on this application-level pin, since + // closePullRequest doesn't take a sha the way merge/reviews do. Either way, the check above only fires when a + // pin EXISTS and disagrees with the live head; a row staged with no pin at all (e.g. by code predating this + // head-pinning fix, or a planning pass that ran against a transiently-null stored head SHA) would otherwise + // fall through to the executor's `ctx.headSha` fallback and silently ratify whatever commit is live NOW, under + // the authority of a review/merge/close that was never actually performed against it (#2422, #2452). // dismissStaleApproval is exempt: it RETRACTS the bot's existing approval rather than granting a new one at a // specific commit, so it carries no "ratify unreviewed code" risk and is safe to replay unpinned. const isUnpinnedRatifyingAction = - !stagedHead && ((pending.actionClass === "approve" && !pending.params.dismissStaleApproval) || pending.actionClass === "merge"); + !stagedHead && + ((pending.actionClass === "approve" && !pending.params.dismissStaleApproval) || pending.actionClass === "merge" || pending.actionClass === "close"); if (isUnpinnedRatifyingAction) { await setPendingAgentActionStatus(env, pending.id, { status: "rejected", decidedBy: input.decidedBy }); await recordAuditEvent(env, { @@ -87,6 +91,27 @@ export async function decidePendingAgentAction(env: Env, input: { id: string; de return { status: "rejected", action: { ...pending, status: "rejected", decidedBy: input.decidedBy }, executionOutcome: "unpinned_legacy_action" }; } + // Re-resolve blacklist membership live at accept time (#2452). The head-SHA pin above only catches a + // FORCE-PUSH; it says nothing about whether the contributor is STILL blacklisted, and a blacklist close is a + // sticky auto_with_approval row with no expiry -- a maintainer can remove the entry (or edit .gittensory.yml) + // at any point while it sits waiting. `settings` was fetched fresh at the top of this function, so this + // mirrors the exact same pure check the planner uses (processors.ts), just re-run against CURRENT config. + if (pending.actionClass === "close" && pending.params.closeKind === "blacklist" && pr) { + const stillBlacklisted = findBlacklistEntry(pr.authorLogin, settings.contributorBlacklist) !== null; + if (!stillBlacklisted) { + 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 blacklist close: contributor is no longer on the blacklist", + metadata: baseMetadata, + }); + return { status: "rejected", action: { ...pending, status: "rejected", decidedBy: input.decidedBy }, executionOutcome: "no_longer_blacklisted" }; + } + } + // Re-derive live justification for a staged MERGE at accept time. auto_with_approval rows have no expiry, so // CI can flip red, the base can go dirty, or a reviewer can request changes while the row just sits waiting for // a maintainer — none of which move the head SHA, so the check above alone would not catch it. Best-effort: a diff --git a/src/settings/agent-actions.ts b/src/settings/agent-actions.ts index 47a2cb032c..1c8b800147 100644 --- a/src/settings/agent-actions.ts +++ b/src/settings/agent-actions.ts @@ -282,7 +282,17 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne const label = input.blacklistLabel ?? DEFAULT_BLACKLIST_LABEL; if (acting("label")) actions.push({ actionClass: "label", requiresApproval: approval("label"), reason: "blacklisted contributor", label, labelOp: "add" }); if (acting("close")) { - actions.push({ actionClass: "close", requiresApproval: approval("close"), reason: "blacklisted contributor", closeComment: sanitizePublicComment(blacklistCloseMessage()), closeKind: "blacklist" }); + actions.push({ + actionClass: "close", + requiresApproval: approval("close"), + reason: "blacklisted contributor", + closeComment: sanitizePublicComment(blacklistCloseMessage()), + closeKind: "blacklist", + // Pin like merge/approve (#2452): for an auto_with_approval stage this travels into the pending row so + // the accept-time supersede check (agent-approval-queue.ts) can detect a force-push after staging, and + // decidePendingAgentAction separately re-resolves live blacklist membership for this closeKind. + ...(input.pr.headSha ? { expectedHeadSha: input.pr.headSha } : {}), + }); } return actions; } @@ -507,7 +517,15 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne const reason = linkedIssueHardRule?.reason ?? "the linked issue is not eligible for a community PR"; // Tagged "linked-issue-hard-rule": the close-precision breaker EXEMPTS this deterministic close (it is not // verdict-driven, and the verify path may already have promised closure in a comment). - actions.push({ actionClass: "close", requiresApproval: approval("close"), reason, closeComment: closeMessage([reason]), closeKind: "linked-issue-hard-rule" }); + actions.push({ + actionClass: "close", + requiresApproval: approval("close"), + reason, + closeComment: closeMessage([reason]), + closeKind: "linked-issue-hard-rule", + // Pin like merge/approve (#2452): lets the accept-time supersede check detect a force-push after staging. + ...(input.pr.headSha ? { expectedHeadSha: input.pr.headSha } : {}), + }); } else if (canMerge) { actions.push({ actionClass: "merge", @@ -532,7 +550,16 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne if (closeReasons.length === 0) closeReasons.push("the review gate is not satisfied"); // Tagged "heuristic": a verdict-driven close (gate-verdict / duplicate / slop / CI). This is the ONLY close // the close-precision breaker downgrades to a hold when close precision has dropped. - actions.push({ actionClass: "close", requiresApproval: approval("close"), reason: closeReasons.join("; "), closeComment: closeMessage(closeReasons), closeKind: "heuristic" }); + actions.push({ + actionClass: "close", + requiresApproval: approval("close"), + reason: closeReasons.join("; "), + closeComment: closeMessage(closeReasons), + closeKind: "heuristic", + // Pin like merge/approve (#2452): lets the accept-time supersede check detect a force-push after staging; + // the executor's own step-6 live-CI re-check (#2128) separately covers the CI-driven reason above. + ...(input.pr.headSha ? { expectedHeadSha: input.pr.headSha } : {}), + }); } // else: guarded → manual (needs-human/changes label above); not-good OWNER/automation → held // (request-changes above); review-good-but-not-yet-mergeable → held briefly (rebase/approve resolves it next pass). diff --git a/test/unit/agent-actions.test.ts b/test/unit/agent-actions.test.ts index 090e9af501..d56d787e92 100644 --- a/test/unit/agent-actions.test.ts +++ b/test/unit/agent-actions.test.ts @@ -217,6 +217,11 @@ describe("planAgentMaintenanceActions (#778)", () => { expect(classes(planAgentMaintenanceActions(input({ conclusion: "success", autonomy: { close: "auto" }, pr: { labels: [], slopRisk: 90 } })))).not.toContain("close"); }); + it("pins the heuristic close to the reviewed head, mirroring merge/approve (#2452)", () => { + const close = planAgentMaintenanceActions(input({ conclusion: "failure", autonomy: { close: "auto" }, blockerTitles: ["x"], pr: { labels: [], headSha: "h-reviewed" } })).find((a) => a.actionClass === "close"); + expect(close).toMatchObject({ closeKind: "heuristic", expectedHeadSha: "h-reviewed" }); + }); + it("#dup-winner disposition seam: the close reason includes the duplicate cause only when linkedDuplicateCount > 0", () => { // Loser path (count > 0, the caller's real count): the duplicate cause IS cited in the close reason. const loser = planAgentMaintenanceActions(input({ conclusion: "failure", autonomy: { close: "auto" }, blockerTitles: ["x"], pr: { labels: [], linkedDuplicateCount: 2 } })); @@ -556,6 +561,11 @@ describe("planAgentMaintenanceActions (#778)", () => { expect(close?.closeComment).toContain(violation.reason); }); + it("pins the linked-issue hard-rule close to the reviewed head, mirroring merge/approve (#2452)", () => { + const close = planAgentMaintenanceActions(input({ conclusion: "success", autonomy: { close: "auto" }, ciState: "passed", linkedIssueHardRule: violation, pr: { labels: [], mergeableState: "clean", reviewDecision: "APPROVED", headSha: "h-reviewed" } })).find((a) => a.actionClass === "close"); + expect(close).toMatchObject({ closeKind: "linked-issue-hard-rule", expectedHeadSha: "h-reviewed" }); + }); + it("does NOT close the same violation on an OWNER PR (the isContributor guard)", () => { const plan = classes(planAgentMaintenanceActions(input({ conclusion: "success", autonomy: { close: "auto" }, ciState: "passed", authorIsOwner: true, linkedIssueHardRule: violation, pr: { labels: [], mergeableState: "clean", reviewDecision: "APPROVED" } }))); expect(plan).not.toContain("close"); @@ -797,6 +807,16 @@ describe("contributor blacklist short-circuit (#1425)", () => { expect(plan[1]?.closeComment).toContain("blocked from contributing"); }); + it("pins the blacklist close to the reviewed head, mirroring merge/approve (#2452)", () => { + const plan = planAgentMaintenanceActions(blacklisted({ pr: { labels: [], headSha: "h-reviewed" } })); + expect(plan.find((a) => a.actionClass === "close")).toMatchObject({ closeKind: "blacklist", expectedHeadSha: "h-reviewed" }); + }); + + it("omits expectedHeadSha on the blacklist close when the PR record has no headSha (defensive fallback, #2452)", () => { + const plan = planAgentMaintenanceActions(blacklisted()); + expect(plan.find((a) => a.actionClass === "close")?.expectedHeadSha).toBeUndefined(); + }); + it("uses the repo-configured blacklistLabel, defaulting to 'slop' when unset", () => { expect(planAgentMaintenanceActions(blacklisted({ blacklistLabel: "abuse" }))[0]).toMatchObject({ label: "abuse" }); expect(DEFAULT_BLACKLIST_LABEL).toBe("slop"); diff --git a/test/unit/agent-approval-queue.test.ts b/test/unit/agent-approval-queue.test.ts index dcc68e05eb..b1f9ac20d6 100644 --- a/test/unit/agent-approval-queue.test.ts +++ b/test/unit/agent-approval-queue.test.ts @@ -240,6 +240,78 @@ describe("agent approval queue (#779)", () => { expect(audit?.detail).toContain("no reviewed-head pin"); }); + it("REGRESSION (#2452): accept denies a close staged with NO reviewed-head pin, rather than silently comparing the live head to itself", async () => { + // Unlike merge's `sha` param or approve's `commit_id`, close has NO server-side commit target at all -- the + // executor's step-5 freshness guard falls back to `action.expectedHeadSha ?? ctx.headSha`, and ctx.headSha is + // fetched fresh from the SAME DB row this function just re-read, so an unpinned close's freshness check + // trivially compares the live head against itself and can never catch a force-push after staging. + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: "x" }); + 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: "contributor" }, head: { sha: "h-UNREVIEWED" }, labels: [], body: "x" }); + const { action } = await createPendingAgentActionIfAbsent(env, { repoFullName: "owner/repo", pullNumber: 7, installationId: 5, actionClass: "close", autonomyLevel: "auto_with_approval", params: { closeComment: "noise", closeKind: "heuristic" }, reason: "ci-failed" }); + + const result = await decidePendingAgentAction(env, { id: action.id, decision: "accept", decidedBy: "owner" }); + expect(result.status).toBe("rejected"); + expect(result.executionOutcome).toBe("unpinned_legacy_action"); + const { closePullRequest: closeUnpinned } = await import("../../src/github/pr-actions"); + expect(closeUnpinned).not.toHaveBeenCalled(); + expect((await getPendingAgentAction(env, action.id))?.status).toBe("rejected"); + }); + + it("accept supersedes a staged close when the live head moved after staging (force-push fail-safe, #2452)", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: "x" }); + 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: "contributor" }, head: { sha: "h-NEW" }, labels: [], body: "x" }); + const { action } = await createPendingAgentActionIfAbsent(env, { repoFullName: "owner/repo", pullNumber: 7, installationId: 5, actionClass: "close", autonomyLevel: "auto_with_approval", params: { closeComment: "noise", closeKind: "heuristic", expectedHeadSha: "h-OLD" }, reason: "ci-failed" }); + + const result = await decidePendingAgentAction(env, { id: action.id, decision: "accept", decidedBy: "owner" }); + expect(result.status).toBe("rejected"); + expect(result.executionOutcome).toBe("head_moved"); + const { closePullRequest: closeMoved } = await import("../../src/github/pr-actions"); + expect(closeMoved).not.toHaveBeenCalled(); + }); + + it("accept executes a staged blacklist close when the contributor is STILL blacklisted at accept time (#2452)", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: "x" }); + await upsertRepositorySettings(env, { + repoFullName: "owner/repo", + autonomy: { close: "auto_with_approval" }, + contributorBlacklist: [{ login: "plagiarist", reason: "plagiarism" }], + }); + 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: "close", autonomyLevel: "auto_with_approval", params: { closeComment: "blocked", closeKind: "blacklist", expectedHeadSha: "h7" }, reason: "blacklisted contributor" }); + + const result = await decidePendingAgentAction(env, { id: action.id, decision: "accept", decidedBy: "owner" }); + expect(result.status).toBe("accepted"); + expect(result.executionOutcome).toBe("completed"); + const { closePullRequest: closeStillBlacklisted } = await import("../../src/github/pr-actions"); + expect(closeStillBlacklisted).toHaveBeenCalledWith(env, 5, "owner/repo", 7); + }); + + it("REGRESSION (#2452): accept supersedes a staged blacklist close when the contributor is NO LONGER blacklisted at accept time", async () => { + // The head-SHA pin alone cannot catch this: the contributor never force-pushed, so the freshness check above + // passes cleanly -- only re-resolving blacklist membership against the CURRENT repo settings (not the + // plan-time snapshot baked into the sticky pending row) detects that the maintainer removed the entry. + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: "x" }); + await upsertRepositorySettings(env, { repoFullName: "owner/repo", autonomy: { close: "auto_with_approval" }, contributorBlacklist: [] }); + await seedInstallation(env); + await upsertPullRequestFromGitHub(env, "owner/repo", { number: 7, title: "PR", state: "open", user: { login: "reformed" }, head: { sha: "h7" }, labels: [], body: "x" }); + const { action } = await createPendingAgentActionIfAbsent(env, { repoFullName: "owner/repo", pullNumber: 7, installationId: 5, actionClass: "close", autonomyLevel: "auto_with_approval", params: { closeComment: "blocked", closeKind: "blacklist", expectedHeadSha: "h7" }, reason: "blacklisted contributor" }); + + const result = await decidePendingAgentAction(env, { id: action.id, decision: "accept", decidedBy: "owner" }); + expect(result.status).toBe("rejected"); + expect(result.executionOutcome).toBe("no_longer_blacklisted"); + const { closePullRequest: closeNoLonger } = await import("../../src/github/pr-actions"); + expect(closeNoLonger).not.toHaveBeenCalled(); + expect((await getPendingAgentAction(env, action.id))?.status).toBe("rejected"); + const audit = await env.DB.prepare("select outcome, detail from audit_events where event_type = ?").bind("agent.pending_action.superseded").first<{ outcome: string; detail: string }>(); + expect(audit?.outcome).toBe("denied"); + expect(audit?.detail).toContain("no longer on the blacklist"); + }); + it("accept does NOT deny an unpinned dismissStaleApproval retraction — retracting an approval carries no ratify-unreviewed-code risk (#2377)", async () => { const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: "x" }); await upsertRepositorySettings(env, { repoFullName: "owner/repo", autonomy: { approve: "auto_with_approval" } }); @@ -491,7 +563,7 @@ describe("agent approval queue (#779)", () => { await upsertRepositorySettings(env, { repoFullName: "owner/repo", autonomy: { close: "auto_with_approval", label: "auto" } }); await seedInstallation(env); await upsertPullRequestFromGitHub(env, "owner/repo", { number: 8, title: "PR", state: "open", user: { login: "contributor" }, head: { sha: "h8" }, labels: [], body: "x" }); - const { action } = await createPendingAgentActionIfAbsent(env, { repoFullName: "owner/repo", pullNumber: 8, installationId: 5, actionClass: "close", autonomyLevel: "auto_with_approval", params: { closeComment: "noise", closeKind: "heuristic" }, reason: "ci-failed" }); + const { action } = await createPendingAgentActionIfAbsent(env, { repoFullName: "owner/repo", pullNumber: 8, installationId: 5, actionClass: "close", autonomyLevel: "auto_with_approval", params: { closeComment: "noise", closeKind: "heuristic", expectedHeadSha: "h8" }, reason: "ci-failed" }); await env.DB.prepare("INSERT INTO system_flags (key, value) VALUES (?, ?)").bind("closehold:owner/repo", "true").run(); const result = await decidePendingAgentAction(env, { id: action.id, decision: "accept", decidedBy: "owner" }); From 107e28cea78b713a82a58719a3d31b9e03f8d237 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 1 Jul 2026 23:36:02 -0700 Subject: [PATCH 2/3] fix(queue): also re-validate linked-issue hard-rule closes at accept time The blacklist close re-check added for #2452 only covered closeKind "blacklist". A closeKind "linked-issue-hard-rule" close staged while the linked issue was ineligible would still fire after the issue became eligible or the hard-rule config changed, as long as the PR head SHA didn't move (flagged by the gate's own review of #2452). Mirrors the blacklist re-check: re-resolve the hard rule against current state at accept time and supersede the close if it's no longer violated. Fails open on a token-mint failure, matching the existing merge-side re-check's fail-open contract. --- src/services/agent-approval-queue.ts | 40 +++++++++++++++ test/unit/agent-approval-queue.test.ts | 71 ++++++++++++++++++++++++++ 2 files changed, 111 insertions(+) diff --git a/src/services/agent-approval-queue.ts b/src/services/agent-approval-queue.ts index 1c9f40fbcd..9730c68d30 100644 --- a/src/services/agent-approval-queue.ts +++ b/src/services/agent-approval-queue.ts @@ -112,6 +112,46 @@ export async function decidePendingAgentAction(env: Env, input: { id: string; de } } + // Re-validate a staged CLOSE tagged "linked-issue-hard-rule" against the CURRENT hard-rule state (flagged by + // the gate's own review of #2452). Mirrors the blacklist re-check above: the head-SHA pin only catches a + // force-push, not a maintainer relabeling/reassigning the linked issue (or editing hard-rule config) while the + // close sits waiting in the queue -- head SHA unchanged, so the pin doesn't catch it. Unlike the merge re-check + // below (which supersedes a merge when the rule BECOMES violated), this close was staged BECAUSE the rule WAS + // violated, so this supersedes it when the rule is NO LONGER violated -- the close's own justification + // evaporated. No owner/automation-bot re-check needed: a row only reaches closeKind "linked-issue-hard-rule" in + // the first place because the planner already confirmed close-eligibility before staging it (#pendingActionToPlanned). + if (pending.actionClass === "close" && pending.params.closeKind === "linked-issue-hard-rule" && pr) { + const repoOwner = pending.repoFullName.includes("/") ? pending.repoFullName.slice(0, pending.repoFullName.indexOf("/")) : ""; + const linkedIssueRulesConfig = await loadLinkedIssueHardRules(env, pending.repoFullName); + // Best-effort mint, same fail-open contract as the merge re-check below (#2126/#2132): a failed mint or a + // resolution that can't gather issue facts falls back to resolveLinkedIssueHardRule's own "not violated" + // default, which this check then treats as "the close is no longer justified" -- the SAFE direction for an + // irreversible close (superseding it re-stages from a fresh sweep instead of risking a wrongful auto-close). + const ciToken = await createInstallationToken(env, pending.installationId).catch(() => undefined); + const linkedIssueHardRule = await resolveLinkedIssueHardRule({ + env, + repoFullName: pending.repoFullName, + repoOwner, + config: linkedIssueRulesConfig, + body: pr.body, + linkedIssues: pr.linkedIssues, + ciToken, + installationId: pending.installationId, + }); + if (!linkedIssueHardRule?.violated) { + 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 linked-issue hard-rule close: the linked issue is no longer ineligible", + metadata: baseMetadata, + }); + return { status: "rejected", action: { ...pending, status: "rejected", decidedBy: input.decidedBy }, executionOutcome: "linked_issue_no_longer_violated" }; + } + } + // Re-derive live justification for a staged MERGE at accept time. auto_with_approval rows have no expiry, so // CI can flip red, the base can go dirty, or a reviewer can request changes while the row just sits waiting for // a maintainer — none of which move the head SHA, so the check above alone would not catch it. Best-effort: a diff --git a/test/unit/agent-approval-queue.test.ts b/test/unit/agent-approval-queue.test.ts index b1f9ac20d6..adea73c033 100644 --- a/test/unit/agent-approval-queue.test.ts +++ b/test/unit/agent-approval-queue.test.ts @@ -312,6 +312,77 @@ describe("agent approval queue (#779)", () => { expect(audit?.detail).toContain("no longer on the blacklist"); }); + it("accept executes a staged linked-issue hard-rule close when the linked issue is STILL ineligible at accept time", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: "x" }); + 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: "contributor" }, head: { sha: "h7" }, labels: [], body: "Closes #9" }); + vi.mocked(resolveLinkedIssueHardRule).mockResolvedValueOnce({ violated: true, reason: "Linked issue #9 is labeled `maintainer-only` — it is not open for community PRs." }); + const { action } = await createPendingAgentActionIfAbsent(env, { repoFullName: "owner/repo", pullNumber: 7, installationId: 5, actionClass: "close", autonomyLevel: "auto_with_approval", params: { closeComment: "ineligible", closeKind: "linked-issue-hard-rule", expectedHeadSha: "h7" }, reason: "linked issue ineligible" }); + + const result = await decidePendingAgentAction(env, { id: action.id, decision: "accept", decidedBy: "owner" }); + expect(result.status).toBe("accepted"); + expect(result.executionOutcome).toBe("completed"); + const { closePullRequest: closeStillViolated } = await import("../../src/github/pr-actions"); + expect(closeStillViolated).toHaveBeenCalledWith(env, 5, "owner/repo", 7); + }); + + it("REGRESSION: accept supersedes a staged linked-issue hard-rule close when the linked issue is NO LONGER ineligible at accept time (flagged by the gate's own review of #2452)", async () => { + // The head-SHA pin alone cannot catch this: the contributor never force-pushed, so the freshness check above + // passes cleanly -- only re-resolving the hard rule against CURRENT issue/config state (not the plan-time + // snapshot baked into the sticky pending row) detects that a maintainer relabeled the linked issue (or the + // rule config changed) since staging. + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: "x" }); + 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: "contributor" }, head: { sha: "h7" }, labels: [], body: "Closes #9" }); + vi.mocked(resolveLinkedIssueHardRule).mockResolvedValueOnce({ violated: false, reason: null }); + const { action } = await createPendingAgentActionIfAbsent(env, { repoFullName: "owner/repo", pullNumber: 7, installationId: 5, actionClass: "close", autonomyLevel: "auto_with_approval", params: { closeComment: "ineligible", closeKind: "linked-issue-hard-rule", expectedHeadSha: "h7" }, reason: "linked issue ineligible" }); + + const result = await decidePendingAgentAction(env, { id: action.id, decision: "accept", decidedBy: "owner" }); + expect(result.status).toBe("rejected"); + expect(result.executionOutcome).toBe("linked_issue_no_longer_violated"); + const { closePullRequest: closeNoLongerViolated } = await import("../../src/github/pr-actions"); + expect(closeNoLongerViolated).not.toHaveBeenCalled(); + expect((await getPendingAgentAction(env, action.id))?.status).toBe("rejected"); + const audit = await env.DB.prepare("select outcome, detail from audit_events where event_type = ?").bind("agent.pending_action.superseded").first<{ outcome: string; detail: string }>(); + expect(audit?.outcome).toBe("denied"); + expect(audit?.detail).toContain("no longer ineligible"); + }); + + it("accept tolerates a slash-less repoFullName for a staged linked-issue hard-rule close (defensive fallback)", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: "x" }); + await upsertRepositorySettings(env, { repoFullName: "solorepo", autonomy: { close: "auto_with_approval" } }); + await upsertInstallation(env, { + installation: { id: 5, account: { login: "owner", id: 1, type: "User" }, repository_selection: "selected", permissions: { metadata: "read", pull_requests: "write", issues: "write" }, events: ["pull_request"] }, + repositories: [{ name: "solorepo", full_name: "solorepo", private: false, owner: { login: "owner" } }], + }); + await upsertPullRequestFromGitHub(env, "solorepo", { number: 7, title: "PR", state: "open", head: { sha: "h7" }, labels: [], body: "Closes #9" }); + vi.mocked(resolveLinkedIssueHardRule).mockResolvedValueOnce({ violated: true, reason: "still ineligible" }); + const { action } = await createPendingAgentActionIfAbsent(env, { repoFullName: "solorepo", pullNumber: 7, installationId: 5, actionClass: "close", autonomyLevel: "auto_with_approval", params: { closeComment: "ineligible", closeKind: "linked-issue-hard-rule", expectedHeadSha: "h7" }, reason: "linked issue ineligible" }); + + const result = await decidePendingAgentAction(env, { id: action.id, decision: "accept", decidedBy: "owner" }); + expect(result.status).toBe("accepted"); + expect(result.executionOutcome).toBe("completed"); + const { closePullRequest: closeSlashless } = await import("../../src/github/pr-actions"); + expect(closeSlashless).toHaveBeenCalledWith(env, 5, "solorepo", 7); + }); + + it("accept still executes a staged linked-issue hard-rule close when its own token mint fails — fails OPEN, ciToken passed as undefined", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: "x" }); + 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: "contributor" }, head: { sha: "h7" }, labels: [], body: "Closes #9" }); + vi.mocked(resolveLinkedIssueHardRule).mockResolvedValueOnce({ violated: true, reason: "still ineligible" }); + vi.mocked(createInstallationToken).mockRejectedValueOnce(new Error("installation suspended")); + const { action } = await createPendingAgentActionIfAbsent(env, { repoFullName: "owner/repo", pullNumber: 7, installationId: 5, actionClass: "close", autonomyLevel: "auto_with_approval", params: { closeComment: "ineligible", closeKind: "linked-issue-hard-rule", expectedHeadSha: "h7" }, reason: "linked issue ineligible" }); + + const result = await decidePendingAgentAction(env, { id: action.id, decision: "accept", decidedBy: "owner" }); + expect(result.status).toBe("accepted"); + expect(result.executionOutcome).toBe("completed"); + expect(vi.mocked(resolveLinkedIssueHardRule)).toHaveBeenCalledWith(expect.objectContaining({ ciToken: undefined })); + }); + it("accept does NOT deny an unpinned dismissStaleApproval retraction — retracting an approval carries no ratify-unreviewed-code risk (#2377)", async () => { const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: "x" }); await upsertRepositorySettings(env, { repoFullName: "owner/repo", autonomy: { approve: "auto_with_approval" } }); From aeb9cae090a8f5f756ede8e577934755dea2fd91 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 1 Jul 2026 23:43:52 -0700 Subject: [PATCH 3/3] fix(queue): re-derive close-eligibility for linked-issue hard-rule closes The linked-issue hard-rule close re-check only re-validated rule violation, not close-eligibility itself. settings.closeOwnerAuthors is a live toggle that can flip to false between staging and accept without moving the PR head SHA -- an owner PR staged for close while the setting was true would still close after it was turned off (flagged by the gate's own review, second pass on #2452). Mirrors the existing merge-side closeEligible derivation: if the author is no longer close-eligible, supersede without even consulting the hard rule, same as the merge path's owner/automation exemption. --- src/services/agent-approval-queue.ts | 68 ++++++++++++++++---------- test/unit/agent-approval-queue.test.ts | 39 +++++++++++++++ 2 files changed, 81 insertions(+), 26 deletions(-) diff --git a/src/services/agent-approval-queue.ts b/src/services/agent-approval-queue.ts index 9730c68d30..6a05efdb98 100644 --- a/src/services/agent-approval-queue.ts +++ b/src/services/agent-approval-queue.ts @@ -113,42 +113,58 @@ export async function decidePendingAgentAction(env: Env, input: { id: string; de } // Re-validate a staged CLOSE tagged "linked-issue-hard-rule" against the CURRENT hard-rule state (flagged by - // the gate's own review of #2452). Mirrors the blacklist re-check above: the head-SHA pin only catches a - // force-push, not a maintainer relabeling/reassigning the linked issue (or editing hard-rule config) while the - // close sits waiting in the queue -- head SHA unchanged, so the pin doesn't catch it. Unlike the merge re-check - // below (which supersedes a merge when the rule BECOMES violated), this close was staged BECAUSE the rule WAS - // violated, so this supersedes it when the rule is NO LONGER violated -- the close's own justification - // evaporated. No owner/automation-bot re-check needed: a row only reaches closeKind "linked-issue-hard-rule" in - // the first place because the planner already confirmed close-eligibility before staging it (#pendingActionToPlanned). + // the gate's own review of #2452, twice). Mirrors the blacklist re-check above: the head-SHA pin only catches + // a force-push, not a maintainer relabeling/reassigning the linked issue (or editing hard-rule config) while + // the close sits waiting in the queue -- head SHA unchanged, so the pin doesn't catch it. Unlike the merge + // re-check below (which supersedes a merge when the rule BECOMES violated), this close was staged BECAUSE the + // rule WAS violated, so this supersedes it when the rule is NO LONGER violated -- the close's own justification + // evaporated. Also re-derives closeEligible (mirrors the merge re-check's own closeEligible below): the planner + // confirmed eligibility at STAGING time, but settings.closeOwnerAuthors is a live toggle that can flip to false + // between staging and accept without moving the head SHA, same staleness class as the rule check itself -- an + // owner PR staged for close while the setting was true must not still close after it is turned off. if (pending.actionClass === "close" && pending.params.closeKind === "linked-issue-hard-rule" && pr) { const repoOwner = pending.repoFullName.includes("/") ? pending.repoFullName.slice(0, pending.repoFullName.indexOf("/")) : ""; - const linkedIssueRulesConfig = await loadLinkedIssueHardRules(env, pending.repoFullName); - // Best-effort mint, same fail-open contract as the merge re-check below (#2126/#2132): a failed mint or a - // resolution that can't gather issue facts falls back to resolveLinkedIssueHardRule's own "not violated" - // default, which this check then treats as "the close is no longer justified" -- the SAFE direction for an - // irreversible close (superseding it re-stages from a fresh sweep instead of risking a wrongful auto-close). - const ciToken = await createInstallationToken(env, pending.installationId).catch(() => undefined); - const linkedIssueHardRule = await resolveLinkedIssueHardRule({ - env, - repoFullName: pending.repoFullName, - repoOwner, - config: linkedIssueRulesConfig, - body: pr.body, - linkedIssues: pr.linkedIssues, - ciToken, - installationId: pending.installationId, - }); - if (!linkedIssueHardRule?.violated) { + const authorLogin = pr.authorLogin ?? ""; + const authorIsOwner = authorLogin.length > 0 && authorLogin.toLowerCase() === repoOwner.toLowerCase(); + const authorIsAutomationBot = isProtectedAutomationAuthor(pr.authorLogin); + const closeEligible = (!authorIsOwner && !authorIsAutomationBot) || (authorIsOwner && settings.closeOwnerAuthors === true); + let stillJustified = closeEligible; + if (closeEligible) { + const linkedIssueRulesConfig = await loadLinkedIssueHardRules(env, pending.repoFullName); + // Best-effort mint, same fail-open contract as the merge re-check below (#2126/#2132): a failed mint or a + // resolution that can't gather issue facts falls back to resolveLinkedIssueHardRule's own "not violated" + // default, which this check then treats as "the close is no longer justified" -- the SAFE direction for an + // irreversible close (superseding it re-stages from a fresh sweep instead of risking a wrongful auto-close). + const ciToken = await createInstallationToken(env, pending.installationId).catch(() => undefined); + const linkedIssueHardRule = await resolveLinkedIssueHardRule({ + env, + repoFullName: pending.repoFullName, + repoOwner, + config: linkedIssueRulesConfig, + body: pr.body, + linkedIssues: pr.linkedIssues, + ciToken, + installationId: pending.installationId, + }); + stillJustified = linkedIssueHardRule?.violated === true; + } + if (!stillJustified) { 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 linked-issue hard-rule close: the linked issue is no longer ineligible", + detail: closeEligible + ? "superseded linked-issue hard-rule close: the linked issue is no longer ineligible" + : "superseded linked-issue hard-rule close: the author is no longer close-eligible (owner/automation exemption now applies)", metadata: baseMetadata, }); - return { status: "rejected", action: { ...pending, status: "rejected", decidedBy: input.decidedBy }, executionOutcome: "linked_issue_no_longer_violated" }; + return { + status: "rejected", + action: { ...pending, status: "rejected", decidedBy: input.decidedBy }, + executionOutcome: closeEligible ? "linked_issue_no_longer_violated" : "no_longer_close_eligible", + }; } } diff --git a/test/unit/agent-approval-queue.test.ts b/test/unit/agent-approval-queue.test.ts index adea73c033..4277bdf7e0 100644 --- a/test/unit/agent-approval-queue.test.ts +++ b/test/unit/agent-approval-queue.test.ts @@ -350,6 +350,45 @@ describe("agent approval queue (#779)", () => { expect(audit?.detail).toContain("no longer ineligible"); }); + it("REGRESSION: accept supersedes a staged linked-issue hard-rule close for an owner PR when closeOwnerAuthors is turned off before accept (flagged by the gate's own review of #2452, second pass)", async () => { + // Staged while closeOwnerAuthors was true (planner confirmed eligibility at staging time); by accept time a + // maintainer flipped the setting off. The head SHA never moved, so the freshness pin above doesn't catch + // this -- only re-deriving closeEligible against CURRENT settings does. The hard rule itself is still + // violated (it must not even be consulted once eligibility fails, mirroring the merge-side exemption). + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: "x" }); + await upsertRepositorySettings(env, { repoFullName: "owner/repo", autonomy: { close: "auto_with_approval" }, closeOwnerAuthors: false }); + await seedInstallation(env); + await upsertPullRequestFromGitHub(env, "owner/repo", { number: 7, title: "PR", state: "open", user: { login: "owner" }, head: { sha: "h7" }, labels: [], body: "Closes #9" }); + vi.mocked(resolveLinkedIssueHardRule).mockResolvedValueOnce({ violated: true, reason: "would still violate, but must not even be checked" }); + const { action } = await createPendingAgentActionIfAbsent(env, { repoFullName: "owner/repo", pullNumber: 7, installationId: 5, actionClass: "close", autonomyLevel: "auto_with_approval", params: { closeComment: "ineligible", closeKind: "linked-issue-hard-rule", expectedHeadSha: "h7" }, reason: "linked issue ineligible" }); + + const result = await decidePendingAgentAction(env, { id: action.id, decision: "accept", decidedBy: "owner" }); + expect(result.status).toBe("rejected"); + expect(result.executionOutcome).toBe("no_longer_close_eligible"); + expect(resolveLinkedIssueHardRule).not.toHaveBeenCalled(); + const { closePullRequest: closeNoLongerEligible } = await import("../../src/github/pr-actions"); + expect(closeNoLongerEligible).not.toHaveBeenCalled(); + expect((await getPendingAgentAction(env, action.id))?.status).toBe("rejected"); + const audit = await env.DB.prepare("select outcome, detail from audit_events where event_type = ?").bind("agent.pending_action.superseded").first<{ outcome: string; detail: string }>(); + expect(audit?.outcome).toBe("denied"); + expect(audit?.detail).toContain("no longer close-eligible"); + }); + + it("accept still executes a staged linked-issue hard-rule close for an owner PR when closeOwnerAuthors is (still) true at accept time", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: "x" }); + await upsertRepositorySettings(env, { repoFullName: "owner/repo", autonomy: { close: "auto_with_approval" }, closeOwnerAuthors: true }); + await seedInstallation(env); + await upsertPullRequestFromGitHub(env, "owner/repo", { number: 7, title: "PR", state: "open", user: { login: "owner" }, head: { sha: "h7" }, labels: [], body: "Closes #9" }); + vi.mocked(resolveLinkedIssueHardRule).mockResolvedValueOnce({ violated: true, reason: "still ineligible" }); + const { action } = await createPendingAgentActionIfAbsent(env, { repoFullName: "owner/repo", pullNumber: 7, installationId: 5, actionClass: "close", autonomyLevel: "auto_with_approval", params: { closeComment: "ineligible", closeKind: "linked-issue-hard-rule", expectedHeadSha: "h7" }, reason: "linked issue ineligible" }); + + const result = await decidePendingAgentAction(env, { id: action.id, decision: "accept", decidedBy: "owner" }); + expect(result.status).toBe("accepted"); + expect(result.executionOutcome).toBe("completed"); + const { closePullRequest: closeStillEligible } = await import("../../src/github/pr-actions"); + expect(closeStillEligible).toHaveBeenCalledWith(env, 5, "owner/repo", 7); + }); + it("accept tolerates a slash-less repoFullName for a staged linked-issue hard-rule close (defensive fallback)", async () => { const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: "x" }); await upsertRepositorySettings(env, { repoFullName: "solorepo", autonomy: { close: "auto_with_approval" } });