From 72f4dcbdb32363d430aa1d7eca7a00d265499636 Mon Sep 17 00:00:00 2001 From: Andre Padez Date: Mon, 10 Aug 2026 12:50:43 +0100 Subject: [PATCH] mark the phase 1 review resolved, so it is not fixed twice The review was written as a handover; it became a fixed tree instead. Records what landed, including the two leaks that only showed up while fixing it, and leaves the original reasoning untouched so it still reads as the argument it was. Co-Authored-By: Claude Opus 5 --- docs/opencode-phase1-review.md | 54 ++++++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) 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.