fix(agent-actions): pin staged close actions to a head SHA and re-verify live state at accept time - #2477
Conversation
|
Warning 🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨 ⏸️ Gittensory review result - manual review recommendedReview updated: 2026-07-02 06:51:49 UTC
⏸️ Suggested Action - Manual Review
Review summary Nits — 7 non-blocking
Review context
Contributor next steps
Signal definitions
🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed 💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →. Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2477 +/- ##
==========================================
+ Coverage 95.93% 95.96% +0.02%
==========================================
Files 225 226 +1
Lines 25336 25450 +114
Branches 9217 9258 +41
==========================================
+ Hits 24306 24422 +116
Misses 417 417
+ Partials 613 611 -2
🚀 New features to boost your workflow:
|
…ify 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
…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.
7b84072 to
107e28c
Compare
…oses 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.
Summary
closeactions (blacklist, linked-issue-hard-rule, heuristic) never hadexpectedHeadShaset, unlikeapprove/merge. The accept-time force-push guard inagent-approval-queue.tsonly fires when a pin exists and disagrees with the live head — an unpinned close's freshness check instead falls back to comparing the live head against itself (both sides derive from the same freshgetPullRequestread at the top of the accept flow), so it could never actually catch a force-push that happened during the queue wait.expectedHeadShaon all three close-construction sites insrc/settings/agent-actions.ts, mirroring the existing merge/approve pattern exactly.isUnpinnedRatifyingActionguard (src/services/agent-approval-queue.ts) to also coverclose, so a legacy unpinned row — staged before this fix deploys, or from a transiently-null stored head SHA — is refused and must be re-staged from a fresh sweep, rather than silently replayed.closeKind: "blacklist": the head-SHA pin alone cannot catch a maintainer removing the contributor from the blacklist between staging and accept, since the PR's head never moves in that scenario. Re-runs the exact same purefindBlacklistEntrycheck the planner uses (processors.ts), just against the CURRENT repo settings instead of the plan-time snapshot baked into the sticky pending row.Scoping note: full re-verification of the non-CI heuristic-close reasoning (duplicate-of-open-PR, slop-score threshold) is intentionally out of scope for this PR. The executor's existing step-6 live-CI re-check (
agent-action-executor.ts, #2128) already covers the CI half of a heuristic close, and this PR's head-SHA pin now gives it real force-push protection too — bringing heuristic close to the same fail-safe level merge/approve already had, without importing duplicate-detection/slop-scoring logic into the accept path.Scope
type(scope): short summaryConventional Commit format, for examplefix(api): restore profile access checks.CONTRIBUTING.mdand does not reintroduce GitHub Pages, VitePress,site/, orCNAME.Validation
git diff --checknpm run actionlintnpm run typechecknpm run test:coverage— new code is 100% line+branch covered in both changed files (verified againstcoverage/lcov.info:agent-approval-queue.ts62/62 lines, 81/81 branches; the handful of uncovered branches remaining inagent-actions.tsare pre-existing, outside the lines this PR touches). Global 96.54%/95.52%.npm run test:workersnpm run build:mcpnpm run test:mcp-packnpm run ui:openapi:checknpm run ui:lintnpm run ui:typechecknpm run ui:buildnpm audit --audit-level=moderate— 0 vulnerabilitiesFull suite: 5994/5994 passing (one pre-existing test needed its fixture updated — it staged a heuristic close with no head pin to test the close-precision breaker, which this fix now correctly rejects before reaching the breaker; added the pin so it still exercises the breaker logic it was written for). Also ran
npm run db:migrations:check(unaffected, green).Safety
Notes
#2377/#2422/#2262) already fixed for approve/merge but not extended to close.