fix: prevent cross-PR push from subagent-induced branch switch (#796)
* fix: prevent cross-PR push from subagent-induced branch switch A workflow_dispatch run for zed-industries/cloud (workflow run 26036155393) force-pushed the orchestrator's work onto an unrelated engineer's PR branch (origin/reactivate-pro-plan, PR #2582). The orchestrator's reviewfrog subagent called checkout_pr({pull_number: 2582}), which (1) moved the shared working tree to pr-2582 and (2) persisted pushDest pointing at reactivate-pro-plan. The orchestrator's subsequent commit + push_branch then clobbered the victim PR. Recovery + disclosure in PR #2584. Three compounding bugs closed here: 1. checkout_pr dirty-tree guard had a first-call hole: the previous condition required ctx.toolState.issueNumber to already be set, so on workflow_dispatch runs the first checkout_pr (commonly from a subagent) bypassed the guard entirely. Now any PR switch with a dirty tree is refused, including the first switch of a run. Idempotent same-PR re-checkouts are still absorbed by alreadyOnBranch inside checkoutPrBranch. 2. push_branch trusted sticky pushDest blindly. Added a backstop: refuse pushes where the local branch matches /^pr-(\d+)$/ AND pushDest.remoteBranch differs from it AND the current run is not scoped to PR N (event.is_pr === true && event.issue_number === N). This catches subagent-induced silent branch switches even if a future bug reintroduces a first-call hole in fix #1. 3. Build-mode self-review prompt told the orchestrator to ship "the output of git diff" to the reviewer. The model in this run synthesized git diff main...HEAD, which excludes uncommitted work — and Build self-review runs BEFORE the commit, so the reviewer saw an empty diff and thrashed, eventually calling checkout_pr on a random PR to find something to look at. Prompt now specifies git diff origin/<base-branch> (two-dot, no HEAD), which compares the working tree against the remote base. Refs: zed-industries/cloud workflow run 26036155393 zed-industries/cloud#2582 (victim) zed-industries/cloud#2584 (disclosure) * review: key dirty-tree guard on current branch + drop 'two-dot' misnomer Address review feedback on PR #796. 1. checkout_pr dirty-tree guard now keys off the live current branch (git rev-parse --abbrev-ref HEAD), not ctx.toolState.issueNumber. issueNumber is ALSO set by get_issue / get_issue_comments / get_issue_events, so a subagent doing get_issue(N) followed by checkout_pr(N) on a dirty tree would have bypassed the original guard (issueNumber === pull_number). The current branch is the actual primitive for "would this call move HEAD" — querying it directly avoids correlating on toolState that other tools write to. 2. modes.ts: drop the wrong "two-dot" label on git diff origin/<base>. That's the single-rev form, not two-dot. Copilot was right that the label was confusing/contradictory with the actually-shown command.
This commit is contained in:
committed by
pullfrog[bot]
parent
a0576a702a
commit
6c166ac1cc
@@ -201,7 +201,7 @@ export function computeModes(agentId: AgentId): Mode[] {
|
||||
|
||||
Otherwise delegate the \`${REVIEWER_AGENT_NAME}\` subagent to review your diff with fresh eyes against YOUR TASK. The subagent's baked-in system prompt enforces a non-mutative + non-recursive contract: read-only file/search/web tools and read-only MCP queries only; no writes, shell side effects, state-changing MCP calls, or nested subagent dispatch. Enforcement is prose-only — restate the constraint in your dispatch instructions and do not relax it.
|
||||
|
||||
Provide the subagent with YOUR TASK, the output of \`git diff\`, and a tight summary (not raw output) of any lint/typecheck/test failures you fixed during build — what broke, root cause, the fix — so it can check that fixes addressed root causes rather than suppressed symptoms; say "no build-phase failures" if the build path was clean. Instruct it to flag bugs, logic errors, missing edge cases, gaps between request and diff, and unintended changes.
|
||||
Provide the subagent with YOUR TASK, the output of \`git diff origin/<base-branch>\` (single-rev form, no \`HEAD\` — this compares the working tree against the remote base and captures committed + staged + unstaged work; \`main...HEAD\` and \`--cached\` both miss the uncommitted edits Build self-review runs on, since self-review happens BEFORE the commit), and a tight summary (not raw output) of any lint/typecheck/test failures you fixed during build — what broke, root cause, the fix — so it can check that fixes addressed root causes rather than suppressed symptoms; say "no build-phase failures" if the build path was clean. Instruct it to flag bugs, logic errors, missing edge cases, gaps between request and diff, and unintended changes.
|
||||
|
||||
Delegation + research discipline (distilled from \`/anneal\` canonical — these are codified learnings from many review rounds, not theoretical best practices):
|
||||
- Do NOT summarize what you implemented — that biases the subagent toward validating the shape of your solution rather than questioning it.
|
||||
|
||||
Reference in New Issue
Block a user