Skip to content

fix(agent): pin staged auto_with_approval merge to the reviewed head#1227

Merged
JSONbored merged 1 commit into
mainfrom
fix/staged-approval-merge-revalidation
Jun 24, 2026
Merged

fix(agent): pin staged auto_with_approval merge to the reviewed head#1227
JSONbored merged 1 commit into
mainfrom
fix/staged-approval-merge-revalidation

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Summary

Closes the highest-severity finding from the reviewer-engine adversarial audit: a staged auto_with_approval merge could merge un-reviewed code.

A staged merge pending action stored only its params (mergeMethod) — no head SHA and no gate/CI/mergeable snapshot. On accept, decidePendingAgentAction re-read pr.headSha fresh and the executor merged whatever the head currently was, with no re-validation. Exploit: a contributor gets a clean PR staged ("gate passed, CI green", one notification), force-pushes a backdoor (the sticky pending row is never refreshed and sends no new notification), and a maintainer clicking Accept on the stale entry merges the malicious commit.

The live (non-approval) path was already safe — a force-push there yields a 409 SHA-mismatch that classifyMergeFailure treats as terminal. The approval path defeated that by re-reading the SHA fresh instead of pinning to the reviewed commit.

Fix (defense-in-depth, no migration)

  • Planner (agent-actions.ts): the merge action now carries expectedHeadSha = pr.headSha (the reviewed head). actionParams already persists expectedHeadSha, so it travels into the pending row.
  • Approval queue (agent-approval-queue.ts): on accept, if the staged expectedHeadSha no longer matches the live head, the action is refused and superseded (row marked rejected, agent.pending_action.superseded audited, executionOutcome: "head_moved") — no doomed API call.
  • Executor (agent-action-executor.ts): the GitHub merge sha is pinned to action.expectedHeadSha ?? ctx.headSha, so even if the app-level check is bypassed, a moved head 409s → terminal hold instead of merging un-reviewed code.

A live sweep plans expectedHeadSha == ctx.headSha, so its behavior is byte-identical.

Scope

  • Backend only (src/services/agent-action-executor.ts, src/services/agent-approval-queue.ts, src/settings/agent-actions.ts)
  • Narrow, single coherent change; no public API/schema change; no migration

Validation

  • npm run test:ci — full gate green
  • npm audit --audit-level=moderate — 0 vulnerabilities
  • New tests: planner pins expectedHeadSha; executor pins the merge to the reviewed head over the context head; accept supersedes on a moved head; accept executes on a matching head; accept skips the guard when no PR record exists
  • Every changed line + branch covered (both sides of the new ternaries/guards)

Safety

  • No secrets / wallets / hotkeys / trust scores / reward values
  • No changes to site/ / CNAME / lovable
  • Closes a real auth/merge-integrity gap; adds a regression test pinning the fix

A staged `auto_with_approval` merge stored no head SHA and no gate/CI
snapshot. On accept the executor re-read the current head fresh and merged
it, so a contributor could get a clean PR staged, force-push a backdoor,
and a maintainer accepting the stale "gate passed, CI green" queue entry
would merge the malicious commit with no re-validation.

The planner now pins the merge to the exact reviewed head SHA
(expectedHeadSha, persisted via actionParams into the pending row). On
accept, the approval queue refuses and supersedes the staged action when
the live head no longer matches the reviewed head, and the executor pins
the GitHub merge `sha` to the reviewed commit as a backstop — a moved head
fails safe with a 409 (terminal hold) instead of merging un-reviewed code.
A live sweep plans expectedHeadSha == the current head, so its behavior is
unchanged.
@dosubot dosubot Bot added the size:M label Jun 24, 2026
@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@codecov

codecov Bot commented Jun 24, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.82%. Comparing base (2ea7ed1) to head (222f0e4).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1227   +/-   ##
=======================================
  Coverage   94.82%   94.82%           
=======================================
  Files         158      158           
  Lines       19157    19164    +7     
  Branches     6939     6942    +3     
=======================================
+ Hits        18165    18172    +7     
  Misses        396      396           
  Partials      596      596           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@JSONbored
JSONbored merged commit e6eae6d into main Jun 24, 2026
18 checks passed
@JSONbored
JSONbored deleted the fix/staged-approval-merge-revalidation branch June 24, 2026 11:16
JSONbored added a commit that referenced this pull request Jun 24, 2026
…y) (#1243)

Re-ports reviewbot's instant global freeze that the convergence dropped
(audit §5.2). The only global brake was the env var AGENT_ACTIONS_PAUSED,
which needs a Worker redeploy/secret push to flip, and the /status isFrozen
field was a hardcoded `false` stub.

Adds a singleton global_agent_controls row an operator can flip with one SQL
statement (no redeploy). The action-mode resolver now folds the DB freeze
into globalPaused at all three call sites (executor, the queue re-gate sweep,
and the MCP activation preview), so a freeze halts every agent write action
within one evaluation cycle. /status now reports the real freeze state. The
read fails OPEN (the env var remains the hard backstop) so a transient D1
hiccup can't by itself halt the fleet.

The per-PR SubmissionLock / consumer-concurrency piece of §5.3 is a larger
Durable-Object port and the audit rated its race refuted-to-low (the planner
is deterministic and all side-effect writes are idempotent upserts keyed by
repo#pr, with the merge SHA-pin from #1227 as the hard backstop); deferred.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant