|
| 1 | +# Phase 2 Design: Spawn-Context Isolation Fix |
| 2 | + |
| 3 | +**Skill**: disciplined-design |
| 4 | +**Issue**: 1806 |
| 5 | +**Date**: 2026-05-29 |
| 6 | + |
| 7 | +--- |
| 8 | + |
| 9 | +## 5/25 Rule: Explicitly Out of Scope |
| 10 | + |
| 11 | +| # | Thing not fixed | Reason | |
| 12 | +|---|----------------|--------| |
| 13 | +| 1 | `build_spawn_context_for_agent` signature | Adding a `working_dir` parameter would cascade changes across all 9 call sites and change a stable internal API; the two-line patch after the call is the minimal, contained fix | |
| 14 | +| 2 | `Provider.working_dir` redundancy | After the fix, `agent_working_dir` is stored in both `spawn_ctx.working_dir` and `Provider.working_dir`; removing the Provider field would touch the spawner config path and risk breaking fallback behaviour for non-project agents | |
| 15 | +| 3 | Haiku/review-tier worktree isolation | Review agents intentionally skip isolation (`needs_isolation = false`); this is a correct design choice, not a bug; changing it would increase resource consumption and is out of scope | |
| 16 | + |
| 17 | +--- |
| 18 | + |
| 19 | +## Problem Summary |
| 20 | + |
| 21 | +`build_spawn_context_for_agent` constructs a `SpawnContext` whose `working_dir` field is set to the project root (`project.working_dir`) because at that point the worktree path has not yet been computed. Immediately after that call, the orchestrator allocates a git worktree and stores the resulting path in `agent_working_dir`, then places it on `Provider.working_dir`. However, the spawner's priority rule gives `spawn_ctx.working_dir` the highest precedence over `config.working_dir`, so the stale project-root value silently wins and the agent process is launched in the project root instead of the isolated worktree. The env var `ADF_WORKING_DIR` received the same stale value, so shell scripts inside the agent also operated on the live repository. |
| 22 | + |
| 23 | +--- |
| 24 | + |
| 25 | +## Architecture |
| 26 | + |
| 27 | +### Before (bug) |
| 28 | + |
| 29 | +``` |
| 30 | +build_spawn_context_for_agent() |
| 31 | + +-- spawn_ctx.working_dir = project_root <-- stale, set too early |
| 32 | + +-- ADF_WORKING_DIR = project_root |
| 33 | +
|
| 34 | +create_agent_worktree() |
| 35 | + +-- agent_working_dir = /.../.worktrees/agent-xyz |
| 36 | +
|
| 37 | +Provider.working_dir = agent_working_dir (correct) |
| 38 | +spawn_ctx.working_dir = project_root (wins over Provider) |
| 39 | + | |
| 40 | + v |
| 41 | +spawner: ctx.working_dir wins --> agent runs in PROJECT ROOT [WRONG] |
| 42 | +``` |
| 43 | + |
| 44 | +### After (fix) |
| 45 | + |
| 46 | +``` |
| 47 | +build_spawn_context_for_agent() |
| 48 | + +-- spawn_ctx.working_dir = project_root (temporary, will be overwritten) |
| 49 | +
|
| 50 | +create_agent_worktree() |
| 51 | + +-- agent_working_dir = /.../.worktrees/agent-xyz |
| 52 | +
|
| 53 | +spawn_ctx.working_dir = agent_working_dir [OVERWRITE - 1 line] |
| 54 | +ADF_WORKING_DIR = agent_working_dir [OVERWRITE - 3 lines total] |
| 55 | + | |
| 56 | + v |
| 57 | +spawner: ctx.working_dir wins --> agent runs in WORKTREE [CORRECT] |
| 58 | +``` |
| 59 | + |
| 60 | +--- |
| 61 | + |
| 62 | +## File Changes |
| 63 | + |
| 64 | +| File | Change | Lines | |
| 65 | +|------|--------|-------| |
| 66 | +| `crates/terraphim_orchestrator/src/lib.rs` | Overwrite `spawn_ctx.working_dir` and `ADF_WORKING_DIR` with `agent_working_dir` after the worktree is allocated, before `spawn_with_fallback` | ~3 | |
| 67 | + |
| 68 | +No other file is modified. |
| 69 | + |
| 70 | +--- |
| 71 | + |
| 72 | +## Proposed Change |
| 73 | + |
| 74 | +```rust |
| 75 | +// Before (line 2404-2405, pre-fix state at HEAD~1): |
| 76 | +let mut spawn_ctx = |
| 77 | + build_spawn_context_for_agent(&self.config, def, self.output_poster.as_ref()); |
| 78 | +// ADF_WORKING_DIR and spawn_ctx.working_dir both point to project root here. |
| 79 | +// spawn_with_fallback follows immediately. |
| 80 | + |
| 81 | +// After (lines 2404-2410, current committed fix): |
| 82 | +let mut spawn_ctx = |
| 83 | + build_spawn_context_for_agent(&self.config, def, self.output_poster.as_ref()); |
| 84 | +spawn_ctx.working_dir = Some(agent_working_dir.clone()); // line 2406 |
| 85 | +spawn_ctx = spawn_ctx.with_env( // line 2407 |
| 86 | + "ADF_WORKING_DIR", // line 2408 |
| 87 | + agent_working_dir.to_string_lossy().into_owned(), // line 2409 |
| 88 | +); // line 2410 |
| 89 | +``` |
| 90 | + |
| 91 | +The insertion point is after line 2405 (after `build_spawn_context_for_agent` returns) and before the `if let Some(event) = synthetic_event` block at line 2411. `agent_working_dir` is in scope at this point, having been resolved at line 2329. |
| 92 | + |
| 93 | +--- |
| 94 | + |
| 95 | +## Test Plan |
| 96 | + |
| 97 | +| Test | What it verifies | |
| 98 | +|------|-----------------| |
| 99 | +| `test_spawn_ctx_working_dir_set_to_agent_working_dir` (lib.rs:11956) | Simulates the before-state (project root in spawn_ctx), applies the two-statement fix, then asserts (1) `spawn_ctx.working_dir` equals the worktree path and (2) `ADF_WORKING_DIR` env override equals the worktree path string | |
| 100 | + |
| 101 | +The test is a synchronous unit test with no I/O, no mocks, and no external dependencies. It directly exercises the mutation logic and can be run with: |
| 102 | + |
| 103 | +``` |
| 104 | +cargo test -p terraphim_orchestrator test_spawn_ctx_working_dir_set_to_agent_working_dir |
| 105 | +``` |
| 106 | + |
| 107 | +--- |
| 108 | + |
| 109 | +## Eliminated Options |
| 110 | + |
| 111 | +| Option | Why rejected | |
| 112 | +|--------|-------------| |
| 113 | +| Modify `build_spawn_context_for_agent` to accept `agent_working_dir` as a parameter | Would require changing the function signature and all call sites. The function is called in multiple paths; this refactoring is disproportionate to the single-path fix needed for worktree isolation. | |
| 114 | +| Move worktree allocation before `build_spawn_context_for_agent` | Worktree allocation is async and depends on model-tier resolution (`needs_isolation`), which itself requires the agent definition and config. Reordering these steps would interleave unrelated logic and widen the diff. | |
| 115 | +| Change spawner priority to prefer `config.working_dir` over `ctx.working_dir` | The spawner priority rule (`ctx > config > default`) is the correct design; callers that want to override the directory are expected to set `ctx.working_dir`. Inverting the priority would break every other caller that deliberately sets `ctx.working_dir`. | |
| 116 | + |
| 117 | +--- |
| 118 | + |
| 119 | +## Rollback |
| 120 | + |
| 121 | +Revert the addition of lines 2406-2410 in `lib.rs`. The test at line 11956 should be removed in the same revert. No other files are touched. |
0 commit comments