fix: gate merge phase on "branch ahead of main", not "this iteration's commits"#649
Open
a9a4k wants to merge 1 commit into
Open
fix: gate merge phase on "branch ahead of main", not "this iteration's commits"#649a9a4k wants to merge 1 commit into
a9a4k wants to merge 1 commit into
Conversation
…s commits" The `parallel-planner` and `parallel-planner-with-review` templates (and the repo's own self-driving `.sandcastle/run.ts`) filtered which branches to hand to the merger by checking the implementer's `commits.length` from the current iteration. This is wrong when a previous iteration's implementer wrote the fix but the merger never landed it: every subsequent iteration's implementer re-runs, finds the fix already in place, produces zero new commits, and the orchestrator filters the branch out before the merger ever sees it. The branch then sits ready-to-merge forever. Now asks git directly via `git rev-list --count main..<branch>` — any branch ahead of `main` reaches the merger, regardless of which iteration produced the commits. Per ADR 0009 the `branchIsAheadOfMain` helper is copied into each template that needs it rather than extracted into a shared module. The misleading comment "An agent that ran successfully but made no commits has nothing to merge" is replaced — that's the precise assumption this PR refutes.
|
@TESTPERSONAL is attempting to deploy a commit to the Matt Pocock's projects Team on Vercel. A member of the Team first needs to authorize it. |
Owner
|
Super smart find, thank you |
Author
|
Thanks for building it. Was super nice to work with |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
The
parallel-plannerandparallel-planner-with-reviewtemplates (and the repo's own.sandcastle/run.ts) filter which branches to hand to the merger by checkingentry.outcome.value.commits.length > 0— i.e. only branches whose this iteration's implementer added new commits.This silently strands any branch that was already advanced by a previous iteration whose merger never landed it. The next iteration's implementer reruns, finds the fix already in place, produces zero new commits, the orchestrator filters the branch out, and the merger never sees it. The branch then sits ready-to-merge forever.
I hit this deterministically on a downstream project — the same branch was skipped across three consecutive merger cycles even though
git log main..<branch>showed it was one commit ahead the whole time.The fix
Replace the in-memory
commits.lengthcheck withgit rev-list --count main..<branch>. Now any branch ahead ofmainreaches the merger, regardless of which iteration produced the commits.Per ADR 0009 (templates do not share code), the
branchIsAheadOfMainhelper is copied into each template that needs it rather than extracted.Also replaces the misleading inline comment "An agent that ran successfully but made no commits has nothing to merge" — that's the precise assumption this PR refutes.
Changes
.sandcastle/run.ts(self-driving config)src/templates/parallel-planner/main.mtssrc/templates/parallel-planner-with-review/main.mts.changeset/merge-gate-on-branch-ahead-of-main.md(patch)Verification
npm run typecheck: cleannpm run build: cleannpm test: 34 pre-existing failures unchanged (confirmed identical on clean main; macOS path-resolution noise unrelated to this change)