Skip to content

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

Description

@JSONbored

Parent: #1936

Problem

Today's hardening sprint pinned staged approve/merge actions to a reviewed head SHA and re-verified live state at accept time (#2354, #2377, #2431, #2388). Staged close actions never got the same treatment.

src/settings/agent-actions.ts has three close-construction sites (blacklist close, and two heuristic-close sites) — none of them set expectedHeadSha, unlike the approve/merge construction sites which both pin it. In src/services/agent-approval-queue.ts, decidePendingAgentAction's stale-head supersede check (~line 51) is skipped whenever stagedHead is falsy, and isUnpinnedRatifyingAction (~line 75) explicitly excludes actionClass === "close". The executor's own freshness guard (agent-action-executor.ts, ~line 105) computes expectedHeadSha = action.expectedHeadSha ?? ctx.headSha, where ctx.headSha is the live head just fetched in the same call — so for an unpinned close this trivially compares the live head against itself and always reports "current," regardless of how much the PR changed since staging.

Two concrete failure modes:

  1. Blacklist close: a contributor is staged for a blacklist close. The maintainer later removes them from the blacklist (or edits .gittensory.yml) while the row sits pending. blacklistMatch is resolved once, upstream in src/queue/processors.ts, at original plan time only — there is no re-check at accept. The executor's CI re-check (agent-action-executor.ts ~line 128) explicitly exempts closeKind: "blacklist". Net effect: the now-un-blacklisted contributor's PR is closed anyway, citing stale reasoning, with zero live re-verification of the one fact that changed.
  2. Heuristic close: a staged heuristic close cites a non-CI reason (duplicate-of-open-PR, over the slop-score threshold). The contributor pushes a new commit that fixes CI but doesn't resolve the actual cited reason. The executor's CI-specific re-check can only deny the close if CI itself flipped state — it never re-evaluates the non-CI reasoning (duplicate/slop) against the new, unreviewed diff.

Fix

  • Set expectedHeadSha on every close-construction site in src/settings/agent-actions.ts (mirroring approve/merge).
  • Extend isUnpinnedRatifyingAction (or add an equivalent check) to cover close, so a moved head without a pin is rejected as unpinned-legacy.
  • For closeKind: "blacklist", re-resolve blacklist membership live at accept time before executing (fail-closed if the check can't be determined, matching the existing linked-issue hard-rule pattern in agent-approval-queue.ts ~lines 178-188).
  • For closeKind: "heuristic", either re-run the full heuristic-close evaluation against current PR state, or supersede/reject rather than silently replaying stale non-CI reasoning against unreviewed content.

Regression tests

  • Accept of a staged blacklist close after the contributor is removed from the blacklist must NOT close the PR.
  • Accept of a staged heuristic (duplicate/slop) close after new commits must re-evaluate, not blindly replay stale reasoning.
  • Force-pushed staged close (unpinned) must be rejected as stale, matching the approve/merge unpinned-rejection behavior.

Metadata

Metadata

Assignees

Labels

maintainer-onlyOwner-only work — yields no Gittensor points.

Projects

Status
Done

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions