diff --git a/docs/opencode-phase1-review.md b/docs/opencode-phase1-review.md index 437e05d3..5df75d1a 100644 --- a/docs/opencode-phase1-review.md +++ b/docs/opencode-phase1-review.md @@ -1,5 +1,10 @@ # Phase 1 review, and one bug the Live panel work made load-bearing +> **Status: everything below is now FIXED — do not re-fix it.** Andre asked me to take it while you were +> on another matter. The supersede defect, both comment corrections, and two related leaks found while +> fixing it landed after this review was written; see "What was actually done" at the end for the diff +> you are inheriting. The review text is left exactly as first written so the reasoning is still readable. + Review of `cfbf58c`, `d7b2231`, `8b409e8`, `e8bd946`, `1402880` against `docs/opencode-parity.md` and `docs/opencode-phase1-report.md`. @@ -132,3 +137,52 @@ bug that is getting better hidden is the one to take while it is still findable. The fork itself: agreed, do not start it, and agreed the third option is the one to take if it is taken. Andre makes that call, not either of us. + +--- + +## What was actually done + +Written after the fact. Andre asked me to implement this rather than hand it back, so you are inheriting +a fixed tree, not a task list. + +**Reproduced first, as asked of you.** `runner.test.ts` grew a lifecycle block that needs no real +`opencode`: `RunnerConfig.bin` points at a shell script that sleeps, which stands in perfectly for a turn +that is still generating. The supersede test failed with exactly the predicted +`OpenCode exited with code 143` before any fix went in. + +**The fix, in `runner.ts`:** + +- `RunHandle` gained `superseded`, set in the supersede branch *before* the kill. +- `finish` became `settle(event | null)`. `null` retires a turn silently. The delete is now + identity-checked — `if (running.get(sessionKey) === handle)` — because a superseded turn no longer owns + that key. +- `proc.exited` returns through `settle(null)` for a superseded handle: no result, no error, no emit. + +**Two further leaks in the same family, found while fixing it and not in the original review:** + +1. **An early return would not have been enough.** Both watchdogs call `finish`, so a superseded turn + that simply returned early would leave an armed 10-minute `hardTimer` to fire an error at whichever + turn held the key by then — the identical cross-talk, delayed past the point anyone would connect it + to a supersede. `settle(null)` clears the timers, which is why the fix retires rather than ignores. +2. **Buffered stdout outlived the turn.** `handleLine` had no `done` guard, so lines still draining from + a killed process were emitted under a sessionKey that now belonged to its replacement — interleaving + one turn's output into another's. Guarded. + +**The control test matters as much as the failing one.** `still reports a turn that dies on its own` +pins that the guard did not overreach: an ordinary non-zero exit still produces an error event. It uses +its own stub rather than `/bin/false`, which is `/usr/bin/false` on macOS — and note that `Bun.spawn` +*throws* on a missing binary rather than emitting, so a bad `OPENCODE_BIN` currently escapes +`runOpenCodeTurn` synchronously into the command handler. Left alone: it is a real edge, but it is not +this bug and it deserves its own change. + +**Both comments corrected.** `LiveOpenCodeSession` moved below `LiveClaudeSession` so the docblock +documents the type it describes again, and it now states plainly that it carries no `ses_…` id — which +is *why* the `/chat/live` rows are permanently unnamed. `chat.ts` says "always null, not +null-until-known" and names what widening the type would buy. + +**Verified:** `tsgo` clean; 575 tests, 573 passing. The two failures are the same host-local pair as +before (`cliamp not found on host`, pty test timing out) and are unrelated. + +**Still yours, unchanged:** exercising `opencode:list` against a real turn — now worth doing, since the +defect that would have made it lie is gone — plus B7, `sweepStaleServes` on macOS, and untracked +`opencode run` children surviving sidecar shutdown.