Kill subprocess before mutating state in change_workspace()

[dcellison]

Summary

• Move await self._kill() above attribute mutations in change_workspace() to fix a race condition
• An in-flight _send_locked() reads self.workspace, self.timeout_seconds, and self.workspace_config at multiple await points during streaming
• With the old ordering (mutate then kill), the stream could see new config values while still running the old workspace process - e.g., applying workspace B timeout to workspace A subprocess
• Killing first ensures the stream hits EOF and exits before any state changes take effect

The attribute mutation block itself is unchanged. This is purely a reordering of _kill() relative to the mutations.

The race

                         Old ordering                    New ordering
                         ───────────                     ───────────
_send_locked() running   self.workspace = B              await self._kill()
  on workspace A...      self.model = B_model              stream sees EOF, exits
                         self.timeout = B_timeout
                         await self._kill()              self.workspace = B
                           stream sees B timeout           self.model = B_model
                           while still on A                self.timeout = B_timeout

Test plan

• New ordering test: patches _kill with a side-effect that records self.workspace at call time, asserts it sees the original workspace (not the new one)
• All 1034 existing tests pass (existing TestChangeWorkspace and TestWorkspaceConfig tests verify final attribute values, which are unchanged)
• make lint clean
• Visual verification: await self._kill() appears before any self.workspace = or self.model = assignments

Fixes #121

[dcellison]

Review by Kai

PR Review: Kill subprocess before mutating state in change_workspace()

Overall: Clean. The fix is correct and the test directly validates the ordering invariant.

---

Logic / Correctness

The race condition is real and the fix is sound. An async streaming loop in _send_locked() can yield at multiple await points, and with the old ordering it could observe self.timeout_seconds or self.workspace_config belonging to workspace B while still draining stdout from workspace A's process. Killing first closes that window.

Error-handling is now strictly better. With the old ordering, if _kill() raised, state would already be partially mutated (self.workspace and self.workspace_config set to new values, but self.model/self.timeout_seconds not yet). With the new ordering, a _kill() exception leaves the object fully unchanged — a cleaner failure mode.

No new concurrency hazard introduced. A double change_workspace() call was already racy before this patch. The reorder doesn't worsen that scenario (double-kill on most implementations is a safe no-op).

---

Test

test_kill_before_state_mutation (tests/test_claude.py:1558) directly captures claude.workspace inside the patched _kill body and asserts it matches the original workspace. This is exactly the right assertion for an ordering guarantee. AsyncMock auto-detection via patch.object on an async def method works correctly in Python 3.8+, so no mock-mechanics issue.

---

Suggestions (non-blocking)

• suggestion tests/test_claude.py:1558 — /tmp/new-workspace is Unix-specific. Low risk for this project, but tmp_path (a pytest fixture) would be portable and avoids any CI environment where /tmp is restricted.

---

No bugs, no security issues, no missing error handling, no style violations. The attribute mutation block is unchanged and the comment rewrite accurately describes the invariant. Ready to merge.