Keep beads_update off the merge lane's critical path - #5
Open
rsetia wants to merge 3 commits into
Open
Conversation
The merge lane exists to stop two merge_resolve nodes touching targetBranch at once. It was chained through each bead's terminal node, which is the beads_update node whenever includeBeadsUpdate is on (the default) — so every later merge waited on the previous bead's issue-tracker bookkeeping, an executor that never touches the target branch. Chain merge -> merge instead. beads_update still hangs off its own merge, it just no longer gates the next one. This does not change the lane's ordering (still alphabetical by bead id) or its failure semantics (a failed implement still blocks every downstream merge, because the lane is a dependency edge). Both need the lane to become an engine-level lease rather than an edge; that is a separate change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The shape assertion says merge-b depends on merge-a. This adds the behavioral claim underneath it: hold update-a open indefinitely and merge-b still runs. Reverting the generator change fails both tests — the shape one on the edge list, this one on `expected [ 'merge-a' ] to include 'merge-b'` — so it is a real regression test, not a restatement. It flushes a fixed number of macrotask ticks rather than awaiting merge-b, so the old behavior fails in milliseconds instead of hanging to a vitest timeout. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The flag governing the lane had no test at any level — not the SDK option, not the --no-serialize-merges switch. Its entire contract is "emit no lane edges", so a regression that leaked edges into the opt-out path would have passed the whole suite. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
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.
What
The merge lane exists to stop two
merge_resolvenodes racing each other intotargetBranch. It was chained through each bead's terminal node, which is thebeads_updatenode wheneverincludeBeadsUpdateis on (the default). So every later merge waited on the previous bead's issue-tracker bookkeeping.Chain merge → merge instead.
beads_updatestill hangs off its own merge; it just no longer gates the next one.beads_updateis a singlebd update <id>execFile inbeadsRepo ?? cwd(cli-builtins.ts:355-372) — no git, nogh, no target branch.merge_resolveis a Codex agent that merges through GitHub and never direct-pushes the target (codex-contracts.ts:304-344), running in its own provisioned worktree. The two contend for nothing, so the edge between them was pure latency.What this does not change
orderedPlansbreaks topological ties withcompareStrings, so among independent beads the merge order is fixed before the run starts and has nothing to do with which PR is ready first.propagateBlockedNodesiterates to a fixpoint, one failedimplementstill blocks every downstream merge — including PRs that already passed review.Both need the lane to become an engine-level lease (a named resource granted in readiness order) rather than an edge. That is a larger change to
dispatchReadyNodesand the graph schema, and is out of scope here.Regenerating graphs
This is a generator change, so graphs already emitted keep the old shape until regenerated with
prism beads-dag. Old-shape graphs remain valid and runnable —update-a → merge-a → implement-ais a real, satisfiable path, just a longer one. No schema change, graph version stays 1, nothing to migrate.Runs in flight are unaffected either way:
engine.resumereplaysstored.graphfrom the run store, so a run is pinned to the snapshot taken at creation.Test plan
Three commits: the fix, a behavioral test, and coverage for the flag that governs it.
merge-b → [implement-b, merge-a]andupdate-a → [merge-a].maxConcurrency: 4with abeads_updateexecutor that never resolves forupdate-a, and assertsmerge-bran anyway. This is the claim the shape assertion only implies: an earlier bead's bookkeeping can hang forever without delaying a later merge.serializeMerges: falsehad no coverage at any level, SDK or CLI, despite being the flag governing these exact lines. Now asserts both merges stay independent.Reverting the one-line fix fails both new tests, with
expected [ 'merge-a' ] to include 'merge-b'andexpected [ 'implement-b', 'update-a' ] to deeply equal [ 'implement-b', 'merge-a' ]— so they are regression tests, not restatements of the diff. The behavioral one flushes a fixed number of macrotask ticks rather than awaitingmerge-b, so the old behavior fails in ~20ms instead of hanging to a vitest timeout.Full suite: 501 tests pass.
npm run lintandnpm run typecheckclean.Not covered:
merge_resolve's actual GitHub behavior. Its implementation is instruction text executed by an LLM, and its only seam is theExecutionBackendthat launches the codex worker — faking that is stubbing the agent. There is no fake-ghrung to drop to without running a real model. That gap doesn't widen here, since this PR changes whenmerge_resolvemay start, not what it does.🤖 Generated with Claude Code