fix(agent-actions): pin staged close actions to a head SHA and re-verify live state at accept time

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,83 @@ 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-validate a staged CLOSE tagged "linked-issue-hard-rule" against the CURRENT hard-rule state (flagged by
+  // 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 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: 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: closeEligible ? "linked_issue_no_longer_violated" : "no_longer_close_eligible",
+      };
+    }
+  }
+
   // 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

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).

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");