fix(agent-actions): pin a staged approve to its reviewed head SHA - #2377
Conversation
decidePendingAgentAction's supersede check only fires when pending.params.expectedHeadSha is truthy, but planAgentMaintenanceActions set expectedHeadSha on a planned merge action and never on a planned approve action — so for every staged approve, the check was a silent no-op. Unlike merge, approve also had no head-SHA pin anywhere in the executor chain, and createPullRequestReview's APPROVE call had no commit_id pinning it to a specific commit. A force-push between staging and accept meant the maintainer's accept would silently approve the new, unreviewed commit rather than being denied or superseded. Set expectedHeadSha on the planned approve action the same way merge already does, so it persists into the pending row's params and the existing (already-correct, action-class-agnostic) supersede check actually engages. Add an optional commitId parameter to createPullRequestReview and pass the pinned head through from the executor's approve case, mirroring the merge case's identical pattern, so the review itself is pinned to the reviewed commit via GitHub's commit_id rather than always targeting the PR's current head.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2377 +/- ##
=======================================
Coverage 95.84% 95.84%
=======================================
Files 224 224
Lines 25048 25055 +7
Branches 9106 9109 +3
=======================================
+ Hits 24007 24014 +7
Misses 427 427
Partials 614 614
🚀 New features to boost your workflow:
|
|
Warning 🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨 ⏸️ Gittensory review result - manual review recommendedReview updated: 2026-07-01 22:09:47 UTC
⏸️ Suggested Action - Manual Review
Review summary Nits — 6 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.
|
# Conflicts: # src/services/agent-action-executor.ts # test/unit/agent-action-executor.test.ts # test/unit/agent-approval-queue.test.ts
…ge broke "LIVE request_changes/approve without a reviewBody falls back to an empty string" predates this PR's SHA-pinning change to the approve path (performAction now always passes a 5th commit_id arg, falling back to ctx.headSha) but was never updated to expect it -- a gap in the original change, not something introduced by resolving the merge conflicts.
A staged 'approve' with no expectedHeadSha (e.g. staged by code predating this PR's head-pin change, or a planning pass that ran while the stored PR head SHA was transiently null) carries no record of what was actually reviewed. Unlike merge's `sha` param -- which GitHub 409s on if the head moved -- the reviews API's `commit_id` is purely advisory: GitHub will happily post an APPROVE at any valid commit, current or not. The existing supersede check in decidePendingAgentAction only fires when a pin EXISTS and disagrees with the live head, so an unpinned row fell through to the executor's `ctx.headSha` fallback and would silently approve whatever commit is live at accept time -- ratifying code that was never actually gate-evaluated, under the authority of a review that happened against a different (or no) commit. Extend the accept-flow's staleness gate: an approve staged without a pin is refused outright (status: rejected, executionOutcome: "unpinned_legacy_action") rather than replayed through the fallback. 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. This is deliberately scoped to the accept-flow gate, not performAction's shared executor: the executor's ctx.headSha fallback is still correct and necessary for a genuinely fresh, same-pass live plan (where ctx.headSha and any omitted expectedHeadSha both come from the same evaluation, so there is no staging-to-accept gap to bridge) -- changing that shared path would have broken the existing tests that correctly model that case, per an adversarial-review pass that also found the earlier "not exploitable today, legacy-rows-only" framing was wrong (a live planning pass CAN omit expectedHeadSha today whenever the stored PR head SHA is transiently null, and createPendingAgentActionIfAbsent's onConflictDoNothing then freezes that row's missing pin until decided).
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
gittensory-ui | 6c8b30f | Commit Preview URL Branch Preview URL |
Jul 01 2026, 10:11 PM |
…anch (#2430) performAction's update_branch case passed action.expectedHeadSha directly into updatePullRequestBranch with no ?? ctx.headSha fallback, unlike the approve/merge cases immediately above it. Add the same fallback for parity, and document why update_branch does NOT need the accept-flow-level "unpinned -> deny" gate #2377/#2422 added for approve/merge: it only merges the base into the head, never contributor-controlled content, so it cannot itself ratify unreviewed code, and it's already covered by the generic freshness guard + the approval-queue's actionClass-agnostic supersede check. Closes #2424
#2377 closed this gap for approve; merge had the identical fallback in performAction (mergeSha = action.expectedHeadSha ?? ctx.headSha). The usual "GitHub 409s on a stale sha" backstop doesn't cover this case: the fallback substitutes whatever head is live right now, so it trivially matches and no 409 is possible. Extend the same accept-flow denial gate to merge. Closes #2422
What
decidePendingAgentAction's supersede check only fires whenpending.params.expectedHeadShais truthy, butplanAgentMaintenanceActionssetexpectedHeadShaon a plannedmergeaction and never on a plannedapproveaction — so for every staged approve,stagedHeadwasundefinedand the pre-check was a silent no-op.The underlying consequence is more significant than a mislabeled audit row: unlike
merge,approvehad no head-SHA pin anywhere in the executor chain, andcreatePullRequestReview's APPROVE call had nocommit_idparameter pinning it to a specific commit. A force-push between staging and accept meant the maintainer's accept would silently approve the NEW, unreviewed commit rather than being denied or superseded.Failure scenario: a repo runs
approveatauto_with_approval; gittensory stages an approve, the contributor force-pushes, and the maintainer taps Accept believing they're approving what was originally staged. The approve executes against GitHub's current (unreviewed) head, since there's no SHA pin anywhere in the approve path to detect or prevent this.Fix
expectedHeadShaon the plannedapproveaction the same waymergealready does (src/settings/agent-actions.ts), so it's persisted into the pending row's params via the existing (already action-class-agnostic)actionParams()/supersede-check machinery — no changes needed there, since the check itself was always correct, just never fed a value for approve.commitIdparameter tocreatePullRequestReview(src/github/pr-actions.ts), sent as GitHub'scommit_idwhen provided.src/services/agent-action-executor.ts), mirroring the merge case's identicalaction.expectedHeadSha ?? ctx.headShafallback pattern, so a live sweep (no staging) still pins to the current head and a staged replay pins to the reviewed one.Tests
createPullRequestReviewsendscommit_idwhen acommitIdis provided, and omits it when not.Full unsharded
test:coveragegreen (5604 passed);typecheckgreen;npm auditclean.Advances #1936. Closes #2262.