fix: parallel execution#217
Merged
Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
Disabled knowledge base sources:
WalkthroughParallel-task configuration, task graph manifest handling, worktree scope capture and purge, parallel event and UI plumbing, and terminal/process utilities are updated. Related tests and fixtures were expanded across the affected flows. ChangesParallel task configuration, manifest planning, worktree scope, and parallel events/UI
Process lifecycle utilities
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90+ minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
| Filename | Overview |
|---|---|
| internal/core/run/parallel/orchestrator.go | Major additions: parallel recovery worker pool, new fail terminal state distinct from rollback, emitPlanStarted for pre-execution DAG broadcast, SyncTaskArtifacts post-finalize hook, and scope-aware CommitTask/CommitStaged split. |
| internal/core/run/parallel/resolver.go | Replaces hardcoded make verify gate with a configurable ValidationCommand, adds conflictResolverSetupError sentinel, readConflictTextFile guards for symlinks and binary files, and validation-snapshot diffing to detect worktree mutations during validation. |
| internal/core/run/parallel/fsm.go | Adds failed state/event, extracts parallelTerminalSourceStates() helper to keep transition table DRY, and refactors inline []fsm.EventDesc into parallelFSMEvents() for testability. |
| internal/daemon/task_multi.go | Switches task-graph building from flat file scanning to manifest-driven LoadValidatedTaskGraphManifest, wires parallelRecoveryEventSink, adds WorktreeLifecycle.SyncTaskArtifacts, and emits per-task task_started events with child run ID and worktree path. |
| internal/daemon/task_multi_worktree.go | Replaces Commit(path, message) with CommitTask(TaskCommitSpec) and CommitStaged(StagedCommitSpec) for scope-aware commits; adds DiscardIntegrationBranchIfExists for purge; StagedFiles tracking on conflict sets; --no-verify on all integration-branch commits. |
| internal/daemon/task_multi_artifacts.go | New file implementing workflow-artifact mirroring (launch phase) and task-artifact write-back (post-finalize) with strict symlink/binary guards and path-traversal checks. |
| internal/core/run/ui/model.go | Adds translateRecoveryEvent for parallel child recovery routing, parallelPlanStartedMsg translation, and translateParallelPayloadEvent split; MaxAttempts is set to payload.Attempt which always equals the current attempt rather than the configured maximum. |
| pkg/compozy/events/kinds/task.go | New TaskParallelPlanPayload, TaskParallelPlanTask, TaskParallelPlanWave types; adds child_run_id and error to TaskParallelPayload; wave_index and index fields use omitempty which drops the value for wave 0. |
| internal/core/run/parallel/waves.go | New BuildWavesFromEdges(nodes, edges) entry-point replaces the task-entry-based builder, enabling manifest-declared DAG edges to drive topological wave assignment. |
| internal/core/run/ui/multi_remote.go | Adds parallelChildren map and parallelTaskChildBinding for routing recovery events to correct child tabs; introduces aggregate child model for parallel runs; ensureTabChild now guards on a real child run ID before creating the child cockpit. |
Sequence Diagram
%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant O as ExecutionOrchestrator
participant L as TaskLauncher
participant W as WorktreeLifecycle
participant E as EventEmitter
O->>E: emitPlanStarted (full DAG, waves, tasks)
loop Each Wave
O->>L: PrepareTask (WaveIndex, WaveTotal, Base)
L->>E: EmitParallelTaskStarted (ChildRunID, WorktreePath)
L-->>O: TaskRunResult (ProducedPaths, ScopeSupported)
O->>W: CommitTask (TaskCommitSpec w/ scope)
alt scope error
O->>O: fail() → parallelStateFailed
O->>E: emitFailed
else clean
O->>W: SquashMerge (integration ← task commit)
alt conflicts
O->>O: resolveConflict (ValidationCommand)
alt setup error
O->>O: fail() → parallelStateFailed
else exhausted
O->>O: rollback() → parallelStateRolledBack
end
O->>W: CommitStaged (StagedCommitSpec)
end
O->>E: emitWaveMerged
end
end
O->>W: FastForward (baseBranch ← integrationBranch)
O->>W: SyncTaskArtifacts (best-effort)
O->>E: emitCompleted
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant O as ExecutionOrchestrator
participant L as TaskLauncher
participant W as WorktreeLifecycle
participant E as EventEmitter
O->>E: emitPlanStarted (full DAG, waves, tasks)
loop Each Wave
O->>L: PrepareTask (WaveIndex, WaveTotal, Base)
L->>E: EmitParallelTaskStarted (ChildRunID, WorktreePath)
L-->>O: TaskRunResult (ProducedPaths, ScopeSupported)
O->>W: CommitTask (TaskCommitSpec w/ scope)
alt scope error
O->>O: fail() → parallelStateFailed
O->>E: emitFailed
else clean
O->>W: SquashMerge (integration ← task commit)
alt conflicts
O->>O: resolveConflict (ValidationCommand)
alt setup error
O->>O: fail() → parallelStateFailed
else exhausted
O->>O: rollback() → parallelStateRolledBack
end
O->>W: CommitStaged (StagedCommitSpec)
end
O->>E: emitWaveMerged
end
end
O->>W: FastForward (baseBranch ← integrationBranch)
O->>W: SyncTaskArtifacts (best-effort)
O->>E: emitCompleted
Reviews (1): Last reviewed commit: "docs: change skills" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+978
to
+983
| return jobRetryMsg{ | ||
| Index: 0, | ||
| Attempt: payload.Attempt, | ||
| MaxAttempts: payload.Attempt, | ||
| Reason: recoveryReason("recovery started", payload.Strategy), | ||
| }, true |
There was a problem hiding this comment.
MaxAttempts is set to payload.Attempt (the current attempt count) rather than to a maximum bound. On the second recovery attempt this renders as "2 of 2", on the third as "3 of 3", and so on — always making the current attempt look like the final one. RunRecoveryStartedPayload has no MaxAttempts field, so the safest fix is to use 0 (or a dedicated sentinel) to signal "unknown maximum" and let the rendering layer handle that gracefully.
Suggested change
| return jobRetryMsg{ | |
| Index: 0, | |
| Attempt: payload.Attempt, | |
| MaxAttempts: payload.Attempt, | |
| Reason: recoveryReason("recovery started", payload.Strategy), | |
| }, true | |
| return jobRetryMsg{ | |
| Index: 0, | |
| Attempt: payload.Attempt, | |
| MaxAttempts: 0, // max attempts not available in this payload | |
| Reason: recoveryReason("recovery started", payload.Strategy), | |
| }, true |
Comment on lines
+81
to
+94
| type TaskParallelPlanTask struct { | ||
| ID string `json:"id,omitempty"` | ||
| Number int `json:"number,omitempty"` | ||
| Title string `json:"title,omitempty"` | ||
| File string `json:"file,omitempty"` | ||
| Status string `json:"status,omitempty"` | ||
| Dependencies []string `json:"dependencies,omitempty"` | ||
| WaveIndex int `json:"wave_index,omitempty"` | ||
| } | ||
|
|
||
| type TaskParallelPlanWave struct { | ||
| Index int `json:"index,omitempty"` | ||
| TaskIDs []string `json:"task_ids,omitempty"` | ||
| } |
There was a problem hiding this comment.
Index on TaskParallelPlanWave and WaveIndex on TaskParallelPlanTask carry omitempty. Because wave 0 is the zero value for int, those fields will be omitted in JSON for every first-wave entry. A consumer that reconstructs the DAG from the serialized payload must silently assume "missing = 0", which is a fragile contract. Since wave index is semantically meaningful even at zero, dropping omitempty is the safer choice.
Suggested change
| type TaskParallelPlanTask struct { | |
| ID string `json:"id,omitempty"` | |
| Number int `json:"number,omitempty"` | |
| Title string `json:"title,omitempty"` | |
| File string `json:"file,omitempty"` | |
| Status string `json:"status,omitempty"` | |
| Dependencies []string `json:"dependencies,omitempty"` | |
| WaveIndex int `json:"wave_index,omitempty"` | |
| } | |
| type TaskParallelPlanWave struct { | |
| Index int `json:"index,omitempty"` | |
| TaskIDs []string `json:"task_ids,omitempty"` | |
| } | |
| type TaskParallelPlanTask struct { | |
| ID string `json:"id,omitempty"` | |
| Number int `json:"number,omitempty"` | |
| Title string `json:"title,omitempty"` | |
| File string `json:"file,omitempty"` | |
| Status string `json:"status,omitempty"` | |
| Dependencies []string `json:"dependencies,omitempty"` | |
| WaveIndex int `json:"wave_index"` | |
| } | |
| type TaskParallelPlanWave struct { | |
| Index int `json:"index"` | |
| TaskIDs []string `json:"task_ids,omitempty"` | |
| } |
Comment on lines
+309
to
+311
| if err != nil { | ||
| return "", err | ||
| } |
There was a problem hiding this comment.
--no-verify bypasses all git hooks on integration-branch commits
Both commitExplicitPaths and SquashMerge now pass --no-verify, skipping every pre-commit and commit-msg hook installed in the repository. For automated worktree commits this is often intentional, but any repo-level hook that enforces commit-message conventions, signing requirements, or security checks (secret scanners, policy gates) will be silently bypassed on every parallel-task merge commit. If the intent is only to skip slow linting/build hooks, a more targeted approach (e.g., SKIP=slow-hook env var patterns, or a dedicated .compozy-scoped hook config) would be less broad.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
internal/daemon/reconcile.go (1)
68-88: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPopulate
WorktreesRootin the resolved lifecycle settings.
purgeWorktreeAllocatoronly consultssettings.WorktreesRootor the live allocator root. Leaving the new field empty here means a configured worktree root is ignored; after a restart,Purgecan still delete run metadata/artifacts while silently skipping task/integration worktree cleanup.🤖 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 `@internal/daemon/reconcile.go` around lines 68 - 88, Populate WorktreesRoot when building the RunLifecycleSettings in resolveRunLifecycleSettings so the resolved settings carry the configured worktree root instead of leaving it empty. Read the value from cfg.WorktreesRoot alongside KeepTerminalDays, KeepMax, and ShutdownDrainTimeout, and copy it into settings.WorktreesRoot so purgeWorktreeAllocator can use it after restarts.internal/core/run/ui/model.go (2)
898-906: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep
task.parallel.faileddistinct from rollback.
task.parallel.failedis now emitted for non-rollback terminal paths like resolver setup failure and task commit scope rejection. Translating it intoparallelRolledBackMsgwill tell the UI the integration branch was rolled back when those paths explicitly preserve it.🤖 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 `@internal/core/run/ui/model.go` around lines 898 - 906, `task.parallel.failed` is being mapped to `parallelRolledBackMsg`, which conflates failure with rollback in the UI. Update the event handling in `model.go` around the `EventKindTaskParallelFailed` / `EventKindTaskParallelRolledBack` switch so `EventKindTaskParallelRolledBack` still returns `parallelRolledBackMsg`, but `EventKindTaskParallelFailed` returns a separate failure-specific UI message or preserves the integration branch state without implying rollback; use the existing `decodeUIEventPayload[kinds.TaskParallelPayload]` and `parallelRolledBackMsg` symbols to keep the paths distinct.
689-716: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd the recovery event kinds to the immediate-dispatch switch.
translateRecoveryEventbelow now turnsrun.recovery_*into retry/finish/failure UI messages, but raw recovery events still fall through this switch. That leaves those updates batched later than the equivalent job events.🤖 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 `@internal/core/run/ui/model.go` around lines 689 - 716, Add the recovery event kinds to the immediate-dispatch path in the UI event switch so they are handled like the existing job and run events. Update the switch in model.go around the event-kind dispatch in the same block that already lists EventKindJobQueued through EventKindTaskParallelRolledBack, and include the raw recovery kinds that translateRecoveryEvent maps from run.recovery_* so they are dispatched immediately instead of being batched later.
🟡 Minor comments (10)
pkg/compozy/events/kinds/docs_test.go-64-79 (1)
64-79: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winScope the field lookup to the relevant docs section.
Right now each field only needs to appear somewhere in
docs/events.md, so generic names likeid,status, andindexcan satisfy the assertion even if theTaskParallelPlan*payload docs are missing. Restrict the search to thetask.parallel.plan_startedsection or the specific payload heading so this test fails on real documentation regressions. As per path instructions, "Ensure tests verify behavior outcomes, not just function calls".🤖 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 `@pkg/compozy/events/kinds/docs_test.go` around lines 64 - 79, The docs assertion in readEventsDocumentation is too broad because it searches the entire docs/events.md content, allowing generic field names to pass even if the TaskParallelPlan* docs are missing. Update docs_test.go so the field checks are scoped to the task.parallel.plan_started section or the specific payload heading before iterating over TaskParallelPlanPayload, TaskParallelPlanTask, and TaskParallelPlanWave, and keep using jsonFieldName/reflect.TypeOf to verify the relevant documented fields only.Source: Path instructions
internal/core/workspace/config_test.go-558-584 (1)
558-584: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAlign the new validation-command tests with the required
Should...subtest pattern.The new standalone scenario and table case bypass the required
t.Run("Should...")naming convention. Wrap the standalone test body in at.Run("Should allow workspace to disable global conflict validation command", ...)and rename the table case to start withShould.... As per path instructions,**/*_test.go: “MUST use t.Run("Should...") pattern for ALL test cases”.Also applies to: 651-657
🤖 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 `@internal/core/workspace/config_test.go` around lines 558 - 584, The new validation-command tests are not following the required Should... subtest convention. Wrap the standalone scenario in t.Run("Should allow workspace to disable global conflict validation command", ...) and rename the table-driven case in config_test.go to start with Should... so every test case uses t.Run("Should...") consistently.Source: Path instructions
internal/core/sync_test.go-499-517 (1)
499-517: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse the required
t.Run("Should...")test shape here.This scenario is added as a standalone
Test...function, so it bypasses the repo's required subtest pattern for test cases. Please wrap it in at.Run("Should ...")case instead of introducing another top-level one. As per path instructions,**/*_test.goMUST uset.Run("Should...")pattern for ALL test cases.🤖 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 `@internal/core/sync_test.go` around lines 499 - 517, The new cleanupLegacyWorkflowMetadata test is using a standalone top-level Test function instead of the repo-required subtest pattern. Move this scenario under the existing test structure by wrapping it in a t.Run("Should ...") case inside the relevant test suite, keeping the assertions and setup the same; use the unique symbols cleanupLegacyWorkflowMetadata and TestCleanupLegacyWorkflowMetadataPreservesTaskGraphManifest to locate and refactor it.Source: Path instructions
internal/core/tasks/manifest_test.go-116-119 (1)
116-119: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAssert the concrete validation error instead of matching
err.Error().This only checks for
"cycle"in the formatted message, so the test can pass for the wrong failure mode and will break on harmless wording changes. Pleaseerrors.Asthe*TaskGraphManifestValidationErrorand assert the relevantIssueentry instead. As per path instructions, tests MUST have specific error assertions (ErrorContains,ErrorAs).🤖 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 `@internal/core/tasks/manifest_test.go` around lines 116 - 119, The manifest test currently matches a substring in err.Error() instead of asserting the concrete validation type. Update the LoadValidatedTaskGraphManifest assertion in manifest_test.go to use errors.As against *TaskGraphManifestValidationError and verify the specific Issue entry for the cycle failure, rather than checking for "cycle" in the formatted message. Keep the test aligned with the existing validation symbols LoadValidatedTaskGraphManifest and TaskGraphManifestValidationError, and use ErrorAs-style assertion semantics for the failure mode.Source: Path instructions
internal/core/worktree/snapshot_test.go-12-65 (1)
12-65: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winGroup these BuildScope scenarios under
t.Run("Should...")subtests.These are independent cases with near-identical setup, but they're added as separate top-level tests instead of the repo's required subtest pattern. Folding them into one parent test with
t.Run("Should...")cases will also let each subtest callt.Parallel(). As per coding guidelines, use table-driven tests with subtests (t.Run) as the default pattern and uset.Parallel()for independent subtests. As per path instructions,**/*_test.goMUST uset.Run("Should...")pattern for ALL test cases.Also applies to: 67-118, 120-142
🤖 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 `@internal/core/worktree/snapshot_test.go` around lines 12 - 65, Group the BuildScope test cases into a single parent test using t.Run("Should...") subtests instead of separate top-level tests. Refactor the scenarios in snapshot_test.go so each case becomes its own subtest under a shared table-driven structure, and have each independent subtest call t.Parallel(). Use the existing BuildScope, Capture, and mustScopeGit setup/assertion flow, but move the duplicated setup into shared helpers where possible and keep each case’s unique expectations inside its subtest.Sources: Coding guidelines, Path instructions
internal/core/agent/client_test.go-1528-1568 (2)
1528-1568: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winWrap this new scenario in a
t.Run("Should...")subtest.This new case is introduced as a standalone top-level test, but the repo test rules require the
t.Run("Should...")pattern for_test.gocases. As per coding guidelines,**/*_test.go: Use table-driven tests with subtests (`t.Run`) as the default pattern. As per path instructions,MUST use t.Run("Should...") pattern for ALL test cases.🤖 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 `@internal/core/agent/client_test.go` around lines 1528 - 1568, Wrap the new terminal kill scenario in a t.Run subtest using the required "Should..." naming pattern, since this test in TestClientTerminalKillTerminatesChildProcessTree is currently a top-level case. Keep the existing assertions and helper calls intact, but move them inside a subtest block so the _test.go conventions and repo rules are satisfied.Sources: Coding guidelines, Path instructions
1528-1568: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRelease the killed terminal in this test too.
This path exits after
WaitForTerminalExit/waitForProcessExitwithout callingReleaseTerminal, unlike the adjacent kill test. That can leak terminal/session state into later parallel tests.🧹 Proposed fix
if _, err := client.WaitForTerminalExit(waitCtx, acp.WaitForTerminalExitRequest{ SessionId: acp.SessionId(sessionID), TerminalId: resp.TerminalId, }); err != nil { t.Fatalf("wait for killed terminal: %v", err) } waitForProcessExit(t, childPID) + if _, err := client.ReleaseTerminal(context.Background(), acp.ReleaseTerminalRequest{ + SessionId: acp.SessionId(sessionID), + TerminalId: resp.TerminalId, + }); err != nil { + t.Fatalf("release killed terminal: %v", err) + } }🤖 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 `@internal/core/agent/client_test.go` around lines 1528 - 1568, The terminal kill test leaves the terminal/session allocated because it exits after WaitForTerminalExit and waitForProcessExit without releasing it. Update TestClientTerminalKillTerminatesChildProcessTree to call ReleaseTerminal on the same TerminalId/SessionId after the kill and exit checks, matching the adjacent terminal cleanup test, so no state leaks into later parallel tests.internal/core/agent/client_test.go-2684-2690 (1)
2684-2690: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Signal(0)can make this exit check flaky on Unix.A killed child can linger briefly as a zombie, and
process.Signal(syscall.Signal(0)) == nilstill reports that PID as live. That makeswaitForProcessExittime out even when the process tree cleanup actually worked.🤖 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 `@internal/core/agent/client_test.go` around lines 2684 - 2690, `processExists` is using `process.Signal(syscall.Signal(0))` to detect liveness, which can treat zombie PIDs as still running and make `waitForProcessExit` flaky on Unix. Update the existence check in `processExists` to verify the PID is actually reaped/absent rather than just signalable, using a more reliable Unix-specific process state check or by handling zombie status explicitly, while keeping the current `os.FindProcess` flow.internal/core/run/parallel/waves.go-106-110 (1)
106-110: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReport a missing
Tonode without reversing the dependency.
From -> TomeansTodepends onFrom. WhenTois absent, returningMissingDependencyError{TaskID: from, Dependency: to}points at the wrong task and describes the opposite relationship. Please use a dedicated “unknown target node” error here, or preserve the original edge semantics in this branch.🤖 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 `@internal/core/run/parallel/waves.go` around lines 106 - 110, The missing-node handling in Waves construction is reporting the wrong task when the target node is absent. Update the branch in the Waves logic that checks graph.predecessors for the edge From -> To so it preserves the original edge semantics: when To is missing, report an unknown target-node error (or otherwise keep TaskID as To and Dependency as From), rather than using MissingDependencyError with reversed fields. Make the fix in the code path around the graph.predecessors existence checks and any related MissingDependencyError creation.internal/daemon/task_multi_artifacts.go-23-35 (1)
23-35: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winValidate the destination root and slug before deriving the artifact path.
Only
sourceTaskDiris checked. A blankworktreeRootorslugcan makeTaskDirectoryForWorkspaceresolve to an unintended relative task directory.Proposed fix
func mirrorTaskMultiWorkflowArtifacts(sourceTaskDir, worktreeRoot, slug string) error { source := strings.TrimSpace(sourceTaskDir) if source == "" { return errors.New("daemon: workflow artifact source task directory is required") } - destination := model.TaskDirectoryForWorkspace(strings.TrimSpace(worktreeRoot), strings.TrimSpace(slug)) + root := strings.TrimSpace(worktreeRoot) + if root == "" { + return errors.New("daemon: workflow artifact destination worktree root is required") + } + workflowSlug := strings.TrimSpace(slug) + if workflowSlug == "" { + return errors.New("daemon: workflow artifact destination workflow slug is required") + } + destination := model.TaskDirectoryForWorkspace(root, workflowSlug) if err := requireDirectory(source); err != nil { return fmt.Errorf("mirror workflow artifacts for %q: source task directory %s: %w", slug, source, err) }🤖 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 `@internal/daemon/task_multi_artifacts.go` around lines 23 - 35, The destination path is derived in mirrorTaskMultiWorkflowArtifacts without validating worktreeRoot or slug, so blank values can produce an unintended relative directory. Add explicit non-empty checks for worktreeRoot and slug before calling model.TaskDirectoryForWorkspace, and keep the existing sourceTaskDir and requireDirectory validation flow intact so the destination is only computed from validated inputs.
🧹 Nitpick comments (3)
internal/core/run/ui/multi_remote_test.go (1)
930-998: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAdd
t.Parallel()to the new independent subtests.These cases each build fresh model state and don't share fixtures, so keeping them serial works against the
_test.goguideline and makes the-racepath slower than necessary. As per coding guidelines,**/*_test.go: "Uset.Parallel()for independent subtests".Also applies to: 1000-1270, 1908-1925
🤖 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 `@internal/core/run/ui/multi_remote_test.go` around lines 930 - 998, The new independent subtests in multi_remote_test.go are still running serially; add t.Parallel() at the start of each standalone t.Run block that creates its own fresh model state. Update the relevant subtest closures in the test functions around the new aggregate parallel event scenarios so they opt into parallel execution without sharing fixtures, keeping the tests aligned with the *_test.go guidance.Source: Coding guidelines
internal/daemon/shutdown.go (1)
300-314: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winWrap per-run purge failures with the run ID.
The new early returns from
purgeRunWorktreesandDeleteRunsdrop which candidate failed, which will make mixed purge batches much harder to diagnose.♻️ Proposed fix
purgedWorktrees, err := m.purgeRunWorktrees(listCtx, run, settings) if err != nil { - return result, err + return result, fmt.Errorf("purge worktrees for run %q: %w", run.RunID, err) } @@ if err := m.globalDB.DeleteRuns(listCtx, []string{run.RunID}); err != nil { - return result, err + return result, fmt.Errorf("delete run %q: %w", run.RunID, err) }As per coding guidelines,
**/*.go: Prefer explicit error returns with wrapped context usingfmt.Errorf("context: %w", err).🤖 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 `@internal/daemon/shutdown.go` around lines 300 - 314, The per-run cleanup path in shutdown processing loses the candidate identity when `purgeRunWorktrees` or `globalDB.DeleteRuns` fails, making batch failures hard to trace. Update the error returns in the `shutdown.go` run-removal flow to wrap those failures with the current `run.RunID` using explicit contextual wrapping (for example, in the `purgeRunWorktrees` and `DeleteRuns` call sites), so callers can tell which run failed while preserving the original error.Source: Coding guidelines
internal/cli/tasks_run_wizard_test.go (1)
598-623: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSplit the new dimension loop into
t.Run("Should...")subtests.The viewport cases at Lines 602-622 are separate test cases, but a single failure currently hides which size regressed. Wrapping each dimension in its own
t.Run("Should...")keeps this aligned with the repo’s default test pattern and gives isolated failures. As per coding guidelines, "**/*_test.go: Use table-driven tests with subtests (t.Run) as the default pattern". As per path instructions, "MUST use t.Run("Should...") pattern for ALL test cases`".🤖 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 `@internal/cli/tasks_run_wizard_test.go` around lines 598 - 623, The dimension loop in `Should fit bounds with every execution section expanded` should be split into individual `t.Run("Should...")` subtests so each viewport size fails independently. Update the table-driven test in `tasks_run_wizard_test.go` to wrap each `dim` case with its own `t.Run`, keeping the existing setup around `newTaskRunWizardModel`, `Update`, `syncTextFocus`, and `assertTaskRunWizardViewFits` inside the subtest.Sources: Coding guidelines, Path instructions
🤖 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 `@internal/cli/tasks_run_wizard_view.go`:
- Around line 179-191: Clamp focusSpan to the available content rows inside
wizardScrollWindow before computing lastWanted so the active line never gets
scrolled out of view. Update the windowing logic in wizardScrollWindow to cap
the span against avail-2 (or the computed content height) before the start
adjustment, then derive start/end from that clamped span while preserving the
up/down affordances.
In `@internal/cli/tasks_run_wizard.go`:
- Around line 2431-2433: The early return in the workflow selection logic leaves
stale parallel state behind for single-workflow runs. Update the code around
selectedTaskRunWizardWorkflows and the existing state.parallel handling so that
when len(selectedTaskRunWizardWorkflows(*inputs)) <= 1, any previously set
parallel flag is explicitly cleared/reset before returning instead of preserving
the old true value. Ensure the run-command path no longer sees a hidden
parallel=true when only one workflow remains selected.
In `@internal/core/run/executor/review_hooks.go`:
- Around line 61-65: The scope-capture path in
captureTaskWorktreeScope/BuildScope is treating a failed capture like a valid
unchanged scope, which can skip task completion and propagate an empty scope.
Update the logic around the scope.Unchanged check so it only applies when scope
capture succeeded, and do not continue with a zero/partial scope after a capture
error. Make the same fix in both review_hook call sites where this pattern
appears, using the existing scope and error handling in
executor/review_hooks.go.
In `@internal/core/run/ui/integration.go`:
- Around line 218-220: The parallel-task start path is dropping the new
child-run metadata, so update handleParallelTaskStarted to forward ChildRunID
and WorktreePath from parallelTaskStartedMsg into markParallelTaskRunning as
well. Make sure the corresponding UI/task-state flow in uiModel stays wired
through the same symbols so parallel jobs keep their child-run and worktree
linkage instead of only wave/task state.
In `@internal/core/sync.go`:
- Around line 570-583: The legacy-task cleanup logic in
shouldRemoveLegacyTaskList/isTaskGraphManifest is too aggressive and can delete
_tasks.md on parse failures or unknown schema versions. Update
shouldRemoveLegacyTaskList to remove only when the content is positively
identified as the old authored task list, and make isTaskGraphManifest treat
malformed or forward-version manifests as non-legacy so
cleanupLegacyWorkflowMetadata keeps the file unless legacy format is certain.
In `@internal/core/tasks/validate.go`:
- Around line 76-90: The manifest check in validateTaskGraph should not be
skipped for empty task directories; right now the early return before
validateTaskGraphManifestFile prevents manifest-only validation when names is
empty. Move the manifest validation path so validateTaskGraphManifestFile runs
even when there are zero task files, while keeping validateTaskFiles gated by
the existing task list handling. Use validateTaskGraph, validateTaskFiles, and
validateTaskGraphManifestFile as the key locations to adjust control flow.
In `@internal/core/worktree/snapshot.go`:
- Around line 512-545: filesystemFingerprint currently treats submodule paths as
ordinary directories and WalkDir hashes the nested checkout, so update this
function to detect gitlinks/submodules before recursing. Use safeWorkspacePath
and the existing fingerprinting flow in filesystemFingerprint to branch on
git-reported state for a path, and when a gitlink is detected, hash the
submodule’s tracked state instead of walking its contents. Keep
writeFileFingerprint for regular files/directories, but avoid filepath.WalkDir
for submodule directories so snapshotting stays aligned with Git state and does
not scan the entire submodule worktree.
In `@internal/daemon/purge_test.go`:
- Around line 128-587: These purge tests are violating the repo’s test contract
by adding many standalone top-level cases and using raw strings.Contains checks
for failures. Refactor the new cases in purge_test.go into t.Run("Should...")
subtests, preferably table-driven where the setup/expectations are similar, and
call t.Parallel() in independent subtests. Replace direct error string matching
with the project’s specific assertions (ErrorContains or ErrorAs) while keeping
the same behavioral checks in the affected test functions.
In `@internal/daemon/task_multi_artifacts.go`:
- Around line 90-103: Reject symlinked artifact destinations before writing in
copyTaskMultiArtifactFile to prevent os.OpenFile from following an existing
symlink. Add and call a helper like rejectTaskMultiArtifactDestinationSymlink on
destination after MkdirAll and before OpenFile, using os.Lstat to allow missing
paths, reject ModeSymlink, and wrap any stat errors clearly. Keep the existing
copy and error handling in copyTaskMultiArtifactFile unchanged otherwise.
In `@internal/daemon/task_multi_worktree.go`:
- Around line 552-577: Validate the fallback purge path in
resolveIntegrationPurgePath, since returning plannedPath directly skips the
ownership check and can point outside worktreesRoot. Apply
cleanOwnedWorktreePath to the plannedPath fallback as well, and return an error
if it is not owned; keep the existing registeredPath branch behavior unchanged.
In `@internal/daemon/task_multi.go`:
- Around line 1742-1744: The child runtime config in remapTaskMultiChildRuntime
should not preserve an inherited WorkflowName, because it can leave the child
stamped with the parent workflow. Update the WorkflowName assignment logic so it
always sets remapped.WorkflowName to the child slug-derived value (trimmedSlug)
for the child runtime, instead of only filling it when empty. Keep the fix
localized to remapTaskMultiChildRuntime and its remapped config fields.
In `@internal/daemon/worktree_purge.go`:
- Around line 389-419: `cleanOwnedWorktreePath` currently accepts a path based
on the lexical `filepath.Rel` check before any symlink validation, which can
incorrectly mark a symlinked child as owned. Update the ownership check so
`isSymlinkEquivalentWorktreeChild` is evaluated before returning success from
`cleanOwnedWorktreePath`, and only accept the path if the resolved target is
still within `worktreesRoot`; keep the existing `filepath.Abs`,
`filepath.Clean`, and `isRelativeChildPath` flow but ensure symlink resolution
gates the final owned result.
---
Outside diff comments:
In `@internal/core/run/ui/model.go`:
- Around line 898-906: `task.parallel.failed` is being mapped to
`parallelRolledBackMsg`, which conflates failure with rollback in the UI. Update
the event handling in `model.go` around the `EventKindTaskParallelFailed` /
`EventKindTaskParallelRolledBack` switch so `EventKindTaskParallelRolledBack`
still returns `parallelRolledBackMsg`, but `EventKindTaskParallelFailed` returns
a separate failure-specific UI message or preserves the integration branch state
without implying rollback; use the existing
`decodeUIEventPayload[kinds.TaskParallelPayload]` and `parallelRolledBackMsg`
symbols to keep the paths distinct.
- Around line 689-716: Add the recovery event kinds to the immediate-dispatch
path in the UI event switch so they are handled like the existing job and run
events. Update the switch in model.go around the event-kind dispatch in the same
block that already lists EventKindJobQueued through
EventKindTaskParallelRolledBack, and include the raw recovery kinds that
translateRecoveryEvent maps from run.recovery_* so they are dispatched
immediately instead of being batched later.
In `@internal/daemon/reconcile.go`:
- Around line 68-88: Populate WorktreesRoot when building the
RunLifecycleSettings in resolveRunLifecycleSettings so the resolved settings
carry the configured worktree root instead of leaving it empty. Read the value
from cfg.WorktreesRoot alongside KeepTerminalDays, KeepMax, and
ShutdownDrainTimeout, and copy it into settings.WorktreesRoot so
purgeWorktreeAllocator can use it after restarts.
---
Minor comments:
In `@internal/core/agent/client_test.go`:
- Around line 1528-1568: Wrap the new terminal kill scenario in a t.Run subtest
using the required "Should..." naming pattern, since this test in
TestClientTerminalKillTerminatesChildProcessTree is currently a top-level case.
Keep the existing assertions and helper calls intact, but move them inside a
subtest block so the _test.go conventions and repo rules are satisfied.
- Around line 1528-1568: The terminal kill test leaves the terminal/session
allocated because it exits after WaitForTerminalExit and waitForProcessExit
without releasing it. Update TestClientTerminalKillTerminatesChildProcessTree to
call ReleaseTerminal on the same TerminalId/SessionId after the kill and exit
checks, matching the adjacent terminal cleanup test, so no state leaks into
later parallel tests.
- Around line 2684-2690: `processExists` is using
`process.Signal(syscall.Signal(0))` to detect liveness, which can treat zombie
PIDs as still running and make `waitForProcessExit` flaky on Unix. Update the
existence check in `processExists` to verify the PID is actually reaped/absent
rather than just signalable, using a more reliable Unix-specific process state
check or by handling zombie status explicitly, while keeping the current
`os.FindProcess` flow.
In `@internal/core/run/parallel/waves.go`:
- Around line 106-110: The missing-node handling in Waves construction is
reporting the wrong task when the target node is absent. Update the branch in
the Waves logic that checks graph.predecessors for the edge From -> To so it
preserves the original edge semantics: when To is missing, report an unknown
target-node error (or otherwise keep TaskID as To and Dependency as From),
rather than using MissingDependencyError with reversed fields. Make the fix in
the code path around the graph.predecessors existence checks and any related
MissingDependencyError creation.
In `@internal/core/sync_test.go`:
- Around line 499-517: The new cleanupLegacyWorkflowMetadata test is using a
standalone top-level Test function instead of the repo-required subtest pattern.
Move this scenario under the existing test structure by wrapping it in a
t.Run("Should ...") case inside the relevant test suite, keeping the assertions
and setup the same; use the unique symbols cleanupLegacyWorkflowMetadata and
TestCleanupLegacyWorkflowMetadataPreservesTaskGraphManifest to locate and
refactor it.
In `@internal/core/tasks/manifest_test.go`:
- Around line 116-119: The manifest test currently matches a substring in
err.Error() instead of asserting the concrete validation type. Update the
LoadValidatedTaskGraphManifest assertion in manifest_test.go to use errors.As
against *TaskGraphManifestValidationError and verify the specific Issue entry
for the cycle failure, rather than checking for "cycle" in the formatted
message. Keep the test aligned with the existing validation symbols
LoadValidatedTaskGraphManifest and TaskGraphManifestValidationError, and use
ErrorAs-style assertion semantics for the failure mode.
In `@internal/core/workspace/config_test.go`:
- Around line 558-584: The new validation-command tests are not following the
required Should... subtest convention. Wrap the standalone scenario in
t.Run("Should allow workspace to disable global conflict validation command",
...) and rename the table-driven case in config_test.go to start with Should...
so every test case uses t.Run("Should...") consistently.
In `@internal/core/worktree/snapshot_test.go`:
- Around line 12-65: Group the BuildScope test cases into a single parent test
using t.Run("Should...") subtests instead of separate top-level tests. Refactor
the scenarios in snapshot_test.go so each case becomes its own subtest under a
shared table-driven structure, and have each independent subtest call
t.Parallel(). Use the existing BuildScope, Capture, and mustScopeGit
setup/assertion flow, but move the duplicated setup into shared helpers where
possible and keep each case’s unique expectations inside its subtest.
In `@internal/daemon/task_multi_artifacts.go`:
- Around line 23-35: The destination path is derived in
mirrorTaskMultiWorkflowArtifacts without validating worktreeRoot or slug, so
blank values can produce an unintended relative directory. Add explicit
non-empty checks for worktreeRoot and slug before calling
model.TaskDirectoryForWorkspace, and keep the existing sourceTaskDir and
requireDirectory validation flow intact so the destination is only computed from
validated inputs.
In `@pkg/compozy/events/kinds/docs_test.go`:
- Around line 64-79: The docs assertion in readEventsDocumentation is too broad
because it searches the entire docs/events.md content, allowing generic field
names to pass even if the TaskParallelPlan* docs are missing. Update
docs_test.go so the field checks are scoped to the task.parallel.plan_started
section or the specific payload heading before iterating over
TaskParallelPlanPayload, TaskParallelPlanTask, and TaskParallelPlanWave, and
keep using jsonFieldName/reflect.TypeOf to verify the relevant documented fields
only.
---
Nitpick comments:
In `@internal/cli/tasks_run_wizard_test.go`:
- Around line 598-623: The dimension loop in `Should fit bounds with every
execution section expanded` should be split into individual `t.Run("Should...")`
subtests so each viewport size fails independently. Update the table-driven test
in `tasks_run_wizard_test.go` to wrap each `dim` case with its own `t.Run`,
keeping the existing setup around `newTaskRunWizardModel`, `Update`,
`syncTextFocus`, and `assertTaskRunWizardViewFits` inside the subtest.
In `@internal/core/run/ui/multi_remote_test.go`:
- Around line 930-998: The new independent subtests in multi_remote_test.go are
still running serially; add t.Parallel() at the start of each standalone t.Run
block that creates its own fresh model state. Update the relevant subtest
closures in the test functions around the new aggregate parallel event scenarios
so they opt into parallel execution without sharing fixtures, keeping the tests
aligned with the *_test.go guidance.
In `@internal/daemon/shutdown.go`:
- Around line 300-314: The per-run cleanup path in shutdown processing loses the
candidate identity when `purgeRunWorktrees` or `globalDB.DeleteRuns` fails,
making batch failures hard to trace. Update the error returns in the
`shutdown.go` run-removal flow to wrap those failures with the current
`run.RunID` using explicit contextual wrapping (for example, in the
`purgeRunWorktrees` and `DeleteRuns` call sites), so callers can tell which run
failed while preserving the original error.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e5e958db-d1f5-497d-bc4f-a60c1ebda15a
⛔ Files ignored due to path filters (30)
.agents/skills/cmux-orchestration/SKILL.mdis excluded by!**/*.md,!.agents/**.agents/skills/cmux-orchestration/agents/openai.yamlis excluded by!**/*.yaml,!.agents/**.agents/skills/cmux-orchestration/assets/fable-orchestrator-dark.pngis excluded by!**/*.png,!.agents/**.agents/skills/cmux-orchestration/assets/fable-orchestrator.excalidrawis excluded by!.agents/**.agents/skills/cmux-orchestration/assets/fable-orchestrator.pngis excluded by!**/*.png,!.agents/**.agents/skills/compozy/references/skills-reference.mdis excluded by!**/*.md,!.agents/**.agents/skills/compozy/references/workflow-guide.mdis excluded by!**/*.md,!.agents/**.agents/skills/cy-create-prd/SKILL.mdis excluded by!**/*.md,!.agents/**.agents/skills/cy-create-prd/references/question-protocol.mdis excluded by!**/*.md,!.agents/**.agents/skills/cy-create-tasks/SKILL.mdis excluded by!**/*.md,!.agents/**.agents/skills/cy-create-tasks/references/task-context-schema.mdis excluded by!**/*.md,!.agents/**.agents/skills/cy-create-tasks/references/task-template.mdis excluded by!**/*.md,!.agents/**.agents/skills/cy-create-techspec/SKILL.mdis excluded by!**/*.md,!.agents/**README.mdis excluded by!**/*.mddocs/events.mdis excluded by!**/*.mdskeeper.lockis excluded by!**/*.lockskills-lock.jsonis excluded by!**/*.jsonskills/compozy/SKILL.mdis excluded by!**/*.mdskills/compozy/references/cli-reference.mdis excluded by!**/*.mdskills/compozy/references/skills-reference.mdis excluded by!**/*.mdskills/compozy/references/workflow-guide.mdis excluded by!**/*.mdskills/cy-create-prd/SKILL.mdis excluded by!**/*.mdskills/cy-create-prd/references/question-protocol.mdis excluded by!**/*.mdskills/cy-create-tasks/SKILL.mdis excluded by!**/*.mdskills/cy-create-tasks/references/task-context-schema.mdis excluded by!**/*.mdskills/cy-create-tasks/references/task-template.mdis excluded by!**/*.mdskills/cy-create-techspec/SKILL.mdis excluded by!**/*.mdskills/cy-execute-task/SKILL.mdis excluded by!**/*.mdskills/cy-execute-task/references/tracking-checklist.mdis excluded by!**/*.mdskills/git-rebase/SKILL.mdis excluded by!**/*.md
📒 Files selected for processing (70)
internal/cli/daemon_commands.gointernal/cli/root_command_execution_test.gointernal/cli/runs.gointernal/cli/tasks_run_parallel_e2e_test.gointernal/cli/tasks_run_wizard.gointernal/cli/tasks_run_wizard_test.gointernal/cli/tasks_run_wizard_view.gointernal/core/agent/client_test.gointernal/core/agent/terminal.gointernal/core/model/artifacts.gointernal/core/model/model_test.gointernal/core/model/runtime_config.gointernal/core/plan/prepare.gointernal/core/plan/prepare_test.gointernal/core/run/executor/execution_test.gointernal/core/run/executor/review_hooks.gointernal/core/run/executor/runner.gointernal/core/run/internal/acpshared/session_exec.gointernal/core/run/internal/acpshared/session_exec_test.gointernal/core/run/internal/worktree/snapshot.gointernal/core/run/internal/worktree/snapshot_test.gointernal/core/run/parallel/events.gointernal/core/run/parallel/fsm.gointernal/core/run/parallel/orchestrator.gointernal/core/run/parallel/orchestrator_test.gointernal/core/run/parallel/resolver.gointernal/core/run/parallel/resolver_test.gointernal/core/run/parallel/waves.gointernal/core/run/recovery/audit.gointernal/core/run/recovery/orchestrator.gointernal/core/run/recovery/orchestrator_test.gointernal/core/run/ui/adapter_test.gointernal/core/run/ui/integration.gointernal/core/run/ui/integration_test.gointernal/core/run/ui/model.gointernal/core/run/ui/multi_remote.gointernal/core/run/ui/multi_remote_test.gointernal/core/run/ui/timeline.gointernal/core/run/ui/types.gointernal/core/run/ui/update.gointernal/core/subprocess/process.gointernal/core/sync.gointernal/core/sync_test.gointernal/core/tasks/manifest.gointernal/core/tasks/manifest_test.gointernal/core/tasks/validate.gointernal/core/workspace/config_merge.gointernal/core/workspace/config_test.gointernal/core/workspace/config_types.gointernal/core/workspace/config_validate.gointernal/core/worktree/snapshot.gointernal/core/worktree/snapshot_test.gointernal/daemon/purge_test.gointernal/daemon/query_service.gointernal/daemon/reconcile.gointernal/daemon/run_manager.gointernal/daemon/run_manager_test.gointernal/daemon/shutdown.gointernal/daemon/task_multi.gointernal/daemon/task_multi_artifacts.gointernal/daemon/task_multi_test.gointernal/daemon/task_multi_worktree.gointernal/daemon/task_multi_worktree_test.gointernal/daemon/worktree_purge.gopkg/compozy/events/docs_test.gopkg/compozy/events/event.gopkg/compozy/events/kinds/docs_test.gopkg/compozy/events/kinds/run.gopkg/compozy/events/kinds/task.goprompt.txt
💤 Files with no reviewable changes (2)
- internal/core/run/internal/worktree/snapshot.go
- internal/core/run/internal/worktree/snapshot_test.go
|
Too many files changed for review. ( |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/cli/tasks_run_wizard.go (1)
2431-2443: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winClear
parallelLimitwhen workflow parallelism is turned off.Line 2439 only updates
parallel-limitwhen the toggle is on, so disabling parallel workflows can leave a stale limit behind in state. The downstream launcher still resolvesparallel-limitand rejects it outside parallel mode, which can break a serial multi-workflow run after the user turns parallelism back off.Proposed fix
applyInput(cmd, "parallel", inputs.parallelWorkflows, passThroughInput[bool], func(value bool) { state.parallel = value }) - if inputs.parallelWorkflows { - applyInput(cmd, "parallel-limit", inputs.parallelWorkflowLimit, parseIntInput, func(value int) { - state.parallelLimit = value - }) - } + if !inputs.parallelWorkflows { + state.parallelLimit = 0 + return + } + applyInput(cmd, "parallel-limit", inputs.parallelWorkflowLimit, parseIntInput, func(value int) { + state.parallelLimit = value + }) }🤖 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 `@internal/cli/tasks_run_wizard.go` around lines 2431 - 2443, The workflow state in tasks_run_wizard.go leaves a stale parallelLimit when parallel workflows are turned off in the selectedTaskRunWizardWorkflows flow. Update the handling around applyInput for parallel and parallel-limit so that when state.parallel is set to false, state.parallelLimit is also cleared/reset, and ensure the parallel-limit input only persists when inputs.parallelWorkflows is enabled.
🧹 Nitpick comments (2)
internal/daemon/reconcile_test.go (1)
156-170: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWrap this case in a
t.Run("Should...")subtest.This one mutates
HOME, so keeping it non-parallel is fine, but the repo's test contract still expects the scenario itself to live under aShould...subtest. As per coding guidelines,**/*_test.go: Use table-driven tests with subtests (t.Run) as the default pattern. As per path instructions,MUST use t.Run("Should...") pattern for ALL test cases.🤖 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 `@internal/daemon/reconcile_test.go` around lines 156 - 170, Wrap the TestLoadRunLifecycleSettingsPopulatesWorktreesRoot scenario in a t.Run("Should...") subtest while keeping the HOME mutation and assertions inside that subtest; use the existing TestLoadRunLifecycleSettingsPopulatesWorktreesRoot function as the parent and move the setup plus LoadRunLifecycleSettings checks into the Should... case to match the test contract.Sources: Coding guidelines, Path instructions
internal/core/run/executor/execution_test.go (1)
784-821: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWrap this case in a
t.Run("Should...")subtest.The scenario is already isolated, so when you add the required subtest wrapper, move
t.Parallel()into that subtest to match the rest of the suite. As per coding guidelines,**/*_test.go: Use table-driven tests with subtests (t.Run) as the default pattern and uset.Parallel()for independent subtests. As per path instructions,MUST use t.Run("Should...") pattern for ALL test cases.🤖 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 `@internal/core/run/executor/execution_test.go` around lines 784 - 821, The test case in TestCaptureTaskWorktreeScopeSkipsArtifactWhenCaptureFails should be wrapped in a t.Run("Should...") subtest to match the suite’s required test pattern. Move the t.Parallel() call from the parent test into that subtest so the case remains independent and parallelized correctly. Keep the existing assertions and setup inside the new subtest, using the TestCaptureTaskWorktreeScopeSkipsArtifactWhenCaptureFails name and its current workspace/captureTaskWorktreeScope flow as the location to refactor.Sources: Coding guidelines, Path instructions
🤖 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.
Outside diff comments:
In `@internal/cli/tasks_run_wizard.go`:
- Around line 2431-2443: The workflow state in tasks_run_wizard.go leaves a
stale parallelLimit when parallel workflows are turned off in the
selectedTaskRunWizardWorkflows flow. Update the handling around applyInput for
parallel and parallel-limit so that when state.parallel is set to false,
state.parallelLimit is also cleared/reset, and ensure the parallel-limit input
only persists when inputs.parallelWorkflows is enabled.
---
Nitpick comments:
In `@internal/core/run/executor/execution_test.go`:
- Around line 784-821: The test case in
TestCaptureTaskWorktreeScopeSkipsArtifactWhenCaptureFails should be wrapped in a
t.Run("Should...") subtest to match the suite’s required test pattern. Move the
t.Parallel() call from the parent test into that subtest so the case remains
independent and parallelized correctly. Keep the existing assertions and setup
inside the new subtest, using the
TestCaptureTaskWorktreeScopeSkipsArtifactWhenCaptureFails name and its current
workspace/captureTaskWorktreeScope flow as the location to refactor.
In `@internal/daemon/reconcile_test.go`:
- Around line 156-170: Wrap the
TestLoadRunLifecycleSettingsPopulatesWorktreesRoot scenario in a
t.Run("Should...") subtest while keeping the HOME mutation and assertions inside
that subtest; use the existing
TestLoadRunLifecycleSettingsPopulatesWorktreesRoot function as the parent and
move the setup plus LoadRunLifecycleSettings checks into the Should... case to
match the test contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cc87b9b9-28a0-4972-8526-937caaff8b18
⛔ Files ignored due to path filters (1)
skeeper.lockis excluded by!**/*.lock
📒 Files selected for processing (31)
internal/cli/tasks_run_wizard.gointernal/cli/tasks_run_wizard_test.gointernal/cli/tasks_run_wizard_view.gointernal/core/agent/client_test.gointernal/core/run/executor/execution_test.gointernal/core/run/executor/review_hooks.gointernal/core/run/parallel/waves.gointernal/core/run/parallel/waves_test.gointernal/core/run/ui/integration.gointernal/core/run/ui/model.gointernal/core/run/ui/multi_remote_test.gointernal/core/run/ui/types.gointernal/core/run/ui/update.gointernal/core/sync.gointernal/core/sync_test.gointernal/core/tasks/manifest_test.gointernal/core/tasks/validate.gointernal/core/tasks/validate_test.gointernal/core/workspace/config_test.gointernal/core/worktree/snapshot.gointernal/core/worktree/snapshot_test.gointernal/daemon/purge_test.gointernal/daemon/reconcile.gointernal/daemon/reconcile_test.gointernal/daemon/shutdown.gointernal/daemon/task_multi.gointernal/daemon/task_multi_artifacts.gointernal/daemon/task_multi_test.gointernal/daemon/task_multi_worktree.gointernal/daemon/worktree_purge.gopkg/compozy/events/kinds/docs_test.go
🚧 Files skipped from review as they are similar to previous changes (19)
- internal/core/tasks/manifest_test.go
- internal/cli/tasks_run_wizard_test.go
- internal/core/run/executor/review_hooks.go
- internal/core/sync_test.go
- internal/cli/tasks_run_wizard_view.go
- internal/core/run/ui/update.go
- internal/core/run/parallel/waves.go
- internal/daemon/shutdown.go
- pkg/compozy/events/kinds/docs_test.go
- internal/core/agent/client_test.go
- internal/core/workspace/config_test.go
- internal/daemon/task_multi_artifacts.go
- internal/daemon/task_multi.go
- internal/daemon/task_multi_worktree.go
- internal/core/run/ui/multi_remote_test.go
- internal/core/worktree/snapshot.go
- internal/daemon/worktree_purge.go
- internal/core/run/ui/integration.go
- internal/daemon/task_multi_test.go
This was referenced Jun 27, 2026
compozybot
added a commit
that referenced
this pull request
Jul 3, 2026
* docs: update skill * fix: support parallel execution when compozy runs inside a git worktree Worktree-backed parallel execution (#200, #217) assumed the workspace root is a primary checkout and was never tested from inside a linked git worktree. Root-caused and fixed four confirmed failure modes: - Git env leakage: daemon/CLI git runners inherited GIT_DIR, GIT_WORK_TREE, GIT_INDEX_FILE, GIT_COMMON_DIR and could resolve the wrong repository. Added internal/core/gitenv with a shared sanitizer applied to the worktree allocator, review-watch runner, CLI preflight, and snapshot runner. - Repo-wide prune blast radius: scoped cleanup no longer runs git worktree prune across the whole repo family; owned paths are removed explicitly and Prune was dropped from the production WorktreeLifecycle surface. - Purge vs live nested runs: run purge now defers when a planned worktree hosts another active run's workspace root, and when the registered workspace root no longer exists; shutdown logs and skips deferred purges instead of failing the pass. - Late detached-HEAD failure: parallel task modes now preflight in the CLI before daemon bootstrap, rejecting compozy-managed worktree recursion and detached HEAD with actionable errors before any run rows, events, or worktrees are created. Refuted with new linked-worktree coverage: fast-forward semantics, purge branch-retention scope, artifact mirroring, workspace registry double identity, and snapshot/scope on .git-file worktrees. Added a linked-worktree fixture and S1-S6 scenario tests (unit + e2e). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: address pr 223 review batch * fix: wrap gitenv run errors * fix: address gitenv review feedback --------- Co-authored-by: Pedro Nauck <pedronauck@gmail.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
pedronauck
added a commit
that referenced
this pull request
Jul 3, 2026
Add human-written release notes for the next release covering the work merged since v0.2.10: - feat: dependency-aware parallel task execution (--parallel-tasks) (#212) - feat: agentic recovery for failed runs (--recovery) (#212) - feat: COMPOZY_HOME for isolated per-project daemons (#216) - feat: repo-level default overrides in compozy setup (#90) - fix: safer worktree management for parallel runs (#217, #223) - fix: ACP runs consistently apply the selected model (#215) - fix: run TUI elapsed timer restored across all terminal outcomes (#221) Co-Authored-By: Claude Fable 5 <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
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 by CodeRabbit
New Features
_tasks.mdtask-graph manifest support for workflow graphs and dependencies.Bug Fixes
Tests