fix(generation): make master child slices baseline-aware in a shared worktree#121
Merged
Merged
Conversation
…worktree
The master executor runs every child slice in the SAME checkout
(`.run({ cwd: process.cwd() })`, and each `run-<child>` step is just
`ricky run <child> --foreground` in that cwd). So by the time a later
child is reviewed, earlier siblings' changes are already dirty in the
tree. Child slices declare a per-slice changed-files scope; reviewers
and fix-loops enforce it by reading `git status`/`git diff`. With a
shared worktree they saw dozens of out-of-scope sibling files, assigned
BLOCKED, and wrote BLOCKED_NO_COMMIT.md — a false block that stalled the
entire master plan for hours even though the slice's own work was
correct and all proofs passed (observed on the workspace-primitives
PR-39 run, child `update-last-week`: 50 dirty entries / 21 files /
4433 insertions, all sibling-owned).
The spec's own execution model prescribes one worktree per slice, but
the renderer emits shared-checkout runs, so the scope check must be
baseline-relative:
- `prepare-context` now snapshots the pre-child dirty set to
`<artifactsDir>/scope-baseline.txt` (`git status --porcelain`) before
the child touches anything.
- `lead-plan` and `implement-slice` prose tell agents the worktree is
shared, baseline files are sibling/parent state, and must not be
edited, reverted, or cleaned.
- A single `sharedWorktreeScopeRule` is injected into every
review/fix/final task: judge scope, findings, and BLOCKED only on the
delta this child introduces on top of the baseline (current
`git status --porcelain` minus scope-baseline.txt), and never BLOCK
solely because unrelated sibling files are dirty.
This preserves the scope gate's real purpose (catch a child editing
outside its declared targets) while tolerating the shared worktree the
renderer actually produces.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds shared-worktree baseline scoping to child workflow generation. Before execution, each child snapshots the initial worktree dirty state. All reviewer and fix-loop stages inject a ChangesShared-worktree baseline scoping for child workflows
sequenceDiagram 🎯 2 (Simple) | ⏱️ ~12 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/product/generation/master-workflow-renderer.ts`:
- Around line 387-395: The baseline snapshot command writing to
scope-baseline.txt (uses artifactsDir and the git status command) is taken while
sibling children may still mutate the shared checkout (master executor runs
concurrent children), so later sibling changes can be misattributed to the wrong
child; fix by either (A) serializing child execution in the master workflow
renderer so the snapshot is taken with no concurrent siblings (turn off/guard
the concurrency path around the child-run dispatch referenced near the
concurrent-run logic) or (B) switch to per-child isolated worktrees before
taking the baseline (create a per-child git worktree/checkout or temporary clone
for the child, then run the `git status --porcelain >
${artifactsDir}/scope-baseline.txt` inside that isolated worktree), and ensure
all subsequent scope-delta calculations use that isolated workspace; update the
code paths that reference artifactsDir/scope-baseline.txt and the
child-dispatch/concurrency logic accordingly.
In `@src/product/generation/pipeline.test.ts`:
- Around line 85-95: Replace the raw substring assertions on the generated
workflow text (the local variable content and its toContain checks for
'Shared-worktree scope rule' and related strings) with parser-based structural
assertions: parse the generated workflow source (use a TypeScript/JSON/YAML
parser appropriate to the workflow format) to extract the steps/tasks for the
review and fix stages, then assert that each expected review/fix step contains
the shared-worktree scope rule object (i.e., the rule is present as a structured
node/field attached to those tasks) rather than matching on literal text; update
assertions around the "prepare-context" snapshot check to verify the scope
baseline is used by checking the task's input/argument AST/node that references
scope-baseline.txt and the subtraction expression (current git status minus
scope-baseline.txt). Ensure you operate on the same test-local variables
(content) and test names but replace toContain usages with AST/JSON/YAML field
checks against the task/step nodes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2398453d-63f1-42e3-871b-1e32f3d02234
📒 Files selected for processing (2)
src/product/generation/master-workflow-renderer.tssrc/product/generation/pipeline.test.ts
Comment on lines
+387
to
+395
| // Snapshot the worktree's dirty set BEFORE this child touches anything. | ||
| // The master executor runs every child in the SAME checkout, so by the | ||
| // time a later child starts, earlier siblings' changes are already | ||
| // dirty here. Files listed in scope-baseline.txt are pre-existing | ||
| // sibling/parent state this child does not own — the scope gate must | ||
| // judge this child only on the delta it introduces, never on this | ||
| // baseline. Without it, every child after the first false-blocks on | ||
| // sibling contamination. | ||
| `git status --porcelain > ${shellQuote(`${artifactsDir}/scope-baseline.txt`)} 2>/dev/null || : > ${shellQuote(`${artifactsDir}/scope-baseline.txt`)}`, |
Contributor
There was a problem hiding this comment.
Baseline snapshot is not stable when sibling children run concurrently.
Line 395 captures scope-baseline.txt only once at child start, but Line 206 allows concurrent child runs in the same checkout. Sibling dirt introduced after this snapshot is still outside the baseline and can be misclassified as this child’s scope delta, which reintroduces false BLOCKED outcomes.
Consider either serializing child runs in shared worktree mode or moving to per-child isolated worktrees before relying on baseline subtraction for gating.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/product/generation/master-workflow-renderer.ts` around lines 387 - 395,
The baseline snapshot command writing to scope-baseline.txt (uses artifactsDir
and the git status command) is taken while sibling children may still mutate the
shared checkout (master executor runs concurrent children), so later sibling
changes can be misattributed to the wrong child; fix by either (A) serializing
child execution in the master workflow renderer so the snapshot is taken with no
concurrent siblings (turn off/guard the concurrency path around the child-run
dispatch referenced near the concurrent-run logic) or (B) switch to per-child
isolated worktrees before taking the baseline (create a per-child git
worktree/checkout or temporary clone for the child, then run the `git status
--porcelain > ${artifactsDir}/scope-baseline.txt` inside that isolated
worktree), and ensure all subsequent scope-delta calculations use that isolated
workspace; update the code paths that reference artifactsDir/scope-baseline.txt
and the child-dispatch/concurrency logic accordingly.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
… concurrency residual Addresses PR #121 review: - CodeRabbit (pipeline.test.ts): replace raw substring assertions on the generated workflow blob with TypeScript-AST extraction. Export childWorkflowSource; the test now parses each `.step("<id>", {...})` config and asserts the baseline snapshot is on `prepare-context` and the shared-worktree scope rule is attached to each review/fix stage's `task` — verifying the contract is wired to the right steps, not that text appears anywhere (AGENTS.md parser-not-regex rule). - CodeRabbit (master-workflow-renderer.ts): document the honest scope of the baseline heuristic. It fully fixes sequential accumulation (the observed stall). Concurrent siblings within a .maxConcurrency(4) wave remain a residual a snapshot cannot close; per-child git-worktree isolation is the durable fix, deferred as a separate larger change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Member
Author
|
Addressed both review comments in 924ec3b: 1. pipeline.test.ts — parser-based assertions (CodeRabbit, quick win): Done. Exported 2. Concurrent baseline race (CodeRabbit, valid): Acknowledged and documented honestly rather than papered over. The observed failure ( All checks were green; full suite still 1192/1192, typecheck clean. |
Contributor
Ricky Eval ReviewRun: Passed: 11 | Needs human: 44 | Reviewable: 44 | Missing output: 0 | Failed: 0 | Skipped: 0 Human Review CasesThese cases passed deterministic checks and include captured Ricky output for a human verdict against their REVIEW
|
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
1 participant
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.
Summary
Root cause of the workspace-primitives PR-39 child
update-last-weekstall (the third finding from the #117 / #119 thread).The master executor runs every child slice in the same checkout —
.run({ cwd: process.cwd() }), and eachrun-<child>step is justricky run <child> --foregroundin that cwd. Child slices declare a per-slice changed-files scope; reviewers and fix-loops enforce it by readinggit status/git diff. With a shared worktree, by the time a later child is reviewed the tree already contains every earlier sibling's dirty files, so the reviewer/validator saw dozens of out-of-scope files, assignedBLOCKED, and wroteBLOCKED_NO_COMMIT.md.Concretely,
update-last-week's codex validator blocked on:— all sibling-owned (digest runtime, mountfuse, mountsync, Python SDK, CLI…). The slice's own deliverable was correct and every proof passed (
go test,pytest, contract check, typecheck, dry-run). It was a false block, and combined with #117/#119 it looped for hours.The spec's own execution model prescribes one worktree per slice, but the renderer emits shared-checkout runs — so the scope check must be baseline-relative.
Fix
prepare-contextsnapshots the pre-child dirty set to<artifactsDir>/scope-baseline.txt(git status --porcelain) before the child touches anything.lead-plan/implement-sliceprose: the worktree is shared; baseline files are sibling/parent state and must not be edited, reverted, or cleaned.sharedWorktreeScopeRuleinjected into every review/fix/final task: judge scope, findings, andBLOCKEDonly on the delta this child introduces on top of the baseline (currentgit status --porcelainminusscope-baseline.txt); neverBLOCKEDsolely because unrelated sibling files are dirty.Preserves the scope gate's real purpose (catch a child editing outside its declared targets) while tolerating the shared worktree the renderer actually produces.
Alternative considered
True per-child
git worktreeisolation (what the spec prescribes). Rejected for this PR: it needs a merge-back strategy for each slice's declared files and changes the master execution model significantly. Baseline-relative scoping is the minimal correct fix; worktree isolation can follow as a separate, larger change.Test plan
scope-baseline.txtsnapshot, theShared-worktree scope rule, the "do not BLOCK on sibling dirt" clause, and the baseline-subtraction definitionnpx vitest run— 1192/1192 passnpx tsc --noEmitcleanRelated
Completes the trio from one master run: #117 (resume-anchor skip-loop), #119 (opaque gate failure), this (shared-worktree false BLOCK — the actual root cause).
🤖 Generated with Claude Code