fix: crash-resumed work-item runs must not rely on ambient cwd for agent dispatch - #619
Conversation
…ent dispatch WorkflowEngine::recover() resumes non-terminal sdd_loop runs after a daemon restart by calling execute_workflow() directly, bypassing execute_work's run_in_worktree/EXECUTE_WORK_CWD_LOCK entirely (PR #601 only guards fresh dispatches from the job queue). A resumed run's agent dispatch still went through the ambient-cwd run_headless, so it silently inherited whatever worktree another concurrently-running item's chdir happened to have set -- confirmed live on items #186/#187, both binding into task/186's worktree. Thread the item's own worktree path through StepInvocation.cwd (persisted on WorkItemData, read at step-execution time so a resumed run still has it) and dispatch via run_headless_in instead, mirroring the pattern app_send_hook already uses for App workflows. This removes the dependency on global process cwd for this call path rather than trying to widen the lock to cover the recovery path too. Also splits the execute_work_impl dispatch-fixture tests out of work.rs into work_dispatch_fixture_tests.rs to stay under the file's LOC gate. Agentflare-Agent: claude-code Agentflare-Branch: task/191-opencode-agentflare-work-dispatch-doesn Agentflare-Item: 191 Agentflare-Session: 0be92ce2-29ad-46d0-9303-597ede893b7b
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
Included review availability: 1 review is currently available. Your included PR review attempts over the past 7 days set your current allowance at 2 reviews per hour. 📝 WalkthroughWalkthrough
ChangesWorktree-aware workflow dispatch
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The change carries each work item's persisted worktree into resumed agent dispatch while preserving existing behavior for unaffected callers; the supplied checks pass and no actionable merge-blocking risk remains. Sequence Diagram(s)sequenceDiagram
participant execute_work_impl
participant run_or_resume_with_sender
participant real_agent_send_hook
participant AgentSubprocess
execute_work_impl->>run_or_resume_with_sender: claimed worktree path
run_or_resume_with_sender->>real_agent_send_hook: StepInvocation with cwd
real_agent_send_hook->>AgentSubprocess: run_headless_in(worktree path)
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Description checkExplanation The description explains the failure mode, implementation, compatibility behavior, regression coverage, and validation results. It omits the template's Notes for reviewers section and generic checklist entries, but the required technical information is otherwise complete. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Summary
Fixes #191:
WorkflowEngine::recover()resumes non-terminalsdd_loopruns after a daemon restart by callingexecute_workflow()directly, bypassingexecute_work'srun_in_worktree/EXECUTE_WORK_CWD_LOCKentirely (PR #601 only guards fresh dispatches from the job queue). A resumed run's agent dispatch still went through the ambient-cwdrun_headless, so it silently inherited whatever worktree another concurrently-running item's chdir happened to have set — confirmed live on items #186/#187, both binding intotask/186's worktree whiletask/187's own worktree sat completely untouched.StepInvocation::cwd: Option<PathBuf>(crates/flare-workflow/src/json.rs).WorkItemData::worktree_path: String(persisted,#[serde(default)]), threaded throughrun_or_resume/run_or_resume_with_senderfromexecute_work_impl's own claimedwpath.build_sdd_loop_stepsetsStepInvocation.cwdfromctx.data.worktree_pathon every implementer/judge invocation — read at execution time from persistedctx.data, so a crash-resumed run still carries the right path.real_agent_send_hooknow dispatches viaagent_launch::run_headless_in(cwd, ...)whencwdisSome, instead of the ambient-cwdrun_headless— mirrors the patternapp_send_hookalready uses for App workflows. This removes the dependency on global process cwd for this call path entirely (no lock needed —Command::current_diris per-spawn, not shared mutable state).StepInvocationconsumers (agent_send_hook, the generic JSON-pipelinePromptExecutor) are unaffected —cwd: Nonepreserves their existing ambient-cwd behavior.execute_work_impl's dispatch-fixture tests out ofwork.rsintowork_dispatch_fixture_tests.rsto stay under the file's LOC gate.Test plan
sdd_loop_step_invocations_carry_the_item_s_own_worktree_path_as_cwd— captures everyStepInvocation.cwdsent throughrun_or_resume_with_senderand asserts it equals the item's own claimed worktree; verified it fails without the fix (temporarily reverted, confirmedNone != Some(worktree), then restored).cargo test --bin agentflare cli::work::— 61 passed (incl. the fix: serialize execute_work's worktree chdir against concurrent dispatch #601 cwd-race regression test and the fix: worktree stale-registration bug + item-pipeline metadata panic #550 worktree-error test).cargo test --bin agentflare work_item_pipeline::— 81 passed.cargo test --bin agentflare workflow::— 21 passed.cargo test -p flare-workflow— 27 lib + 26 integration passed.cargo check --workspace --all-targets,cargo clippy(touched crates),cargo fmt --checkall clean.Summary by CodeRabbit
Improvements
Bug Fixes
Tests