Repository navigation
Description
What happens
In the parallel-planner-with-review template, the reviewer only runs when this cycle's implementer made commits:
// src/templates/parallel-planner-with-review/main.mts, Phase 2
// Only review if the implementer produced commits
if (implement.commits.length > 0) {
const review = await sandbox.run({ name: "reviewer", ... });Since #1531, the merge phase no longer looks at this cycle's commits. It passes the merger every fulfilled branch that is ahead of HEAD (isAheadOfHead, i.e. git rev-list --count HEAD..<branch> > 0), whichever cycle made the commits.
Those two gates now disagree, and a branch can reach the merger without ever being reviewed:
- Cycle N: the implementer commits on
issue-42. The reviewer then fails (network error, sandbox crash, the agent erroring out), so the pipeline promise rejects and the branch is left out of the merge. - Cycle N+1: the planner picks issue 42 again, since it's still open. The implementer finds the work already on the branch and makes no new commits, so
implement.commits.length === 0and the reviewer is skipped. The pipeline fulfils. - The merge phase sees that
issue-42is ahead ofHEADand hands it to the merger, with no review.
The same happens when the implementer itself commits and then throws in cycle N: in that case its commits are never reviewed at all.
Where
src/templates/parallel-planner-with-review/main.mts: theimplement.commits.length > 0gate in Phase 2, compared with theisAheadOfHeadfilter before Phase 3.src/templates/parallel-planner/main.mtshas the sameisAheadOfHeadmerge gate but no reviewer, so it isn't affected.sequential-reviewercreates a fresh branch every iteration and has no merge phase, so it isn't affected either.
Expected
A branch that is ahead of HEAD and hasn't passed review gets reviewed before it can merge. In practice, every branch the merger receives in a cycle has had a reviewer run complete on it in that same cycle.
Prior work
- fix: gate merge phase on "branch ahead of HEAD", not this cycle's commits #1531 moved the merge gate to "branch ahead of
HEAD", which exposed this. - fix: retry review for existing branch work #938 (closed) fixed the retry case before fix: gate merge phase on "branch ahead of HEAD", not this cycle's commits #1531. It re-ran the reviewer when the branch had unmerged commits (
hasUnmergedCommits), and added ahasWorkflag for the merge filter. fix: gate merge phase on "branch ahead of HEAD", not this cycle's commits #1531 has since made thehasWorkhalf unnecessary, but the reviewer half still applies.
This issue was written by an AI agent (Claude Code).
Activity
xg-gh-25 commentedon Nov 11, 2026
The two gates are asking different questions: the reviewer asks "did this cycle produce commits?", the merger asks "is the branch ahead of
HEAD?". A gate keyed on this-run activity will always be bypassed by retries, because a retry is exactly the case where the work already exists. Both gates need to key on the same object: the branch tip being merged.Something like:
Recording the reviewed SHA instead of a boolean also covers the cases in the issue where commits land after the last review (implementer commits then throws, or anything commits after the reviewer ran): the tip moves, the marker no longer matches, and the branch can't reach the merger. Since
reviewedTipis per-cycle, it also matches the stated expectation that every branch the merger gets was reviewed in that same cycle. If you later want to skip re-reviewing an unchanged branch across cycles, persist the marker as a ref (refs/reviewed/issue-42-> sha) rather than in-memory state.The general rule we've landed on: a gate is only mechanical if its predicate is computed from the artifact being let through, not from what happened during the run.
From running a review-before-merge gate in SwarmAI's autonomous pipeline. Discussion: Mechanical Gate vs Ceremonial Gate
fix(templates): review any branch ahead of HEAD in parallel-planner-w…
Review every branch ahead of HEAD in .sandcastle/run.ts (#1632)