From 96fd9e93a28baa643e81578ee122939ae3646116 Mon Sep 17 00:00:00 2001 From: Andre Padez Date: Mon, 10 Aug 2026 12:43:09 +0100 Subject: [PATCH] review phase 1, and report the bug the live panel made load-bearing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 1 accepted and phase 2 answered well. One real defect: the supersede path in runOpenCodeTurn kills a stale turn without marking it, so the dead process late-fires finish() against the turn that replaced it — committing a false "OpenCode exited" to chat_session_events, deleting the live handle from `running`, killing the stop button and orphaning the process. Predates this pass; reported now because e8bd946 is what made the map load-bearing. Reproduced with a stub binary rather than argued — the transcript is in the doc. Co-Authored-By: Claude Opus 5 --- docs/opencode-phase1-review.md | 134 +++++++++++++++++++++++++++++++++ 1 file changed, 134 insertions(+) create mode 100644 docs/opencode-phase1-review.md diff --git a/docs/opencode-phase1-review.md b/docs/opencode-phase1-review.md new file mode 100644 index 00000000..437e05d3 --- /dev/null +++ b/docs/opencode-phase1-review.md @@ -0,0 +1,134 @@ +# Phase 1 review, and one bug the Live panel work made load-bearing + +Review of `cfbf58c`, `d7b2231`, `8b409e8`, `e8bd946`, `1402880` against `docs/opencode-parity.md` and +`docs/opencode-phase1-report.md`. + +**Verdict: Phase 1 accepted. Phase 2's answer accepted, and the recommendation is the right one.** One +real defect, reproduced below, plus two comments that describe behaviour the code does not have. + +--- + +## Accepted, and why + +**The `AGENTS.md` deletion is complete.** Not just the seeding code — the file itself is gone from +`DATA_PATH/opencode_server/` on alpha, which the code could not have done and which a lesser pass would +have left behind for the next reader to find and wonder about. + +**The dead serve-turn deletion is the model for how to delete things.** `5d077a4` recorded as the last +commit where the path was live, `isServerHealthy`'s removal annotated with what it did, that it *worked*, +and which route to prefer if a health check is ever wanted back. Nothing here needs archaeology later. + +**`mapRunLine` is a faithful extraction.** Checked branch by branch against the original `handleLine`, +including the two easy things to get wrong: cost accrues only on `step_finish` (every other line returns +a zero delta, so unconditional accumulation is equivalent), and the early `tool_use` bail on a missing +`callID` still contributes nothing. The 11 tests are real ones — NaN-vs-missing-tokens, empty text parts, +non-JSON interleaving, an unknown future event type. + +**Phase 2's blocker is genuinely answered**, and the trap is the valuable half: the body form of +`directory` being silently ignored yields a session in the serve's own cwd, which reads exactly like "the +serve cannot do per-request directories". That is a wrong answer anyone re-checking this would have +reached too. Recording it cost a line and saves the next person a day. + +**Verified independently:** `tsgo` clean. 573 tests, 571 passing — the two failures are host-local +(`cliamp not found on host`, and the pty test timing out waiting for a real shell), unrelated to this +work. + +--- + +## The defect: a superseded turn takes the live turn down with it + +`runOpenCodeTurn` supersedes a lingering turn for the same `sessionKey` (`runner.ts:59-67`) by killing the +process and dropping it from `running`. It does not mark the handle. So when that process actually dies, +its own `proc.exited` closure still runs — with `done === false` and `killedByUser === false` — and calls +`finish(...)`, which now acts on **the turn that replaced it**. + +Reproduced with a stub binary in place of `opencode` (a script that sleeps), two turns on one +`sessionKey`: + +``` +--- turn 1 starts --- +running: [{"sessionKey":"sess-A"}] +--- turn 2 supersedes it --- +running immediately after: [{"sessionKey":"sess-A"}] + EMIT: {"type":"opencode:event","sessionKey":"sess-A", + "event":{"type":"error","message":"OpenCode exited with code 143"}} +--- after turn 1 is reaped, turn 2 still generating --- +running: [] +stop button reached a process? false +orphan still alive: /bin/sh fake-opencode run --format json … --dir /tmp/oc-probe two +``` + +Four consequences, worst first: + +1. **A false error is committed to the transcript.** The emit goes through + `sessionLog.push(sessionKey, event, durable)` in `index.ts:139-143`, so `OpenCode exited with code 143` + lands in `chat_session_events` against a session that is generating normally, and replays on every + reload. This is not a transient UI artefact. +2. **`listRunningOpenCodeTurns` goes blind.** `finish` calls `running.delete(sessionKey)` unconditionally, + removing the *new* handle. The Live panel added in `e8bd946` therefore omits exactly the turn it + exists to show. +3. **The stop button dies.** `killOpenCodeTurn` finds nothing in `running` and returns silently. +4. **The process orphans.** No handle means the next turn's supersede cannot kill it either. + +The bug predates this pass — it is a property of the supersede path, not of anything in these commits. +It is reported here because `e8bd946` is what made `running`'s accuracy load-bearing: before the Live +panel, entries 2 and 3 were invisible. + +**Suggested fix**, both halves needed: + +- Mark the handle in the supersede branch (`stale.superseded = true`), and have `proc.exited` return + early on it — no `finish`, no emit, no delete. A turn the system replaced on purpose is not an error. +- Make the delete identity-checked regardless: `if (running.get(sessionKey) === handle) running.delete(...)`. + Cheap, and it closes the whole family rather than this one path. + +**Worth reproducing before fixing**, in your own words back to me: the stub-binary trick above is enough, +no real `opencode` needed. `RunnerConfig.bin` is the only injection point required. + +### The related gap + +The extraction moved the pure mapping under test and left the caller holding the two line-spanning +concerns — emit-the-session-id-once, and cost accumulation. That split is correct. But those two are now +the *only* untested logic on the path, and this defect lives in that same untested caller. Worth one test +over `runOpenCodeTurn`'s lifecycle (stub binary, two turns, assert `running` and the emitted events) +rather than more tests over `mapRunLine`, which is well covered. + +--- + +## Two comments that outrun the code + +**1. `/chat/live` describes a lookup that does not happen** (`chat.ts`, the OpenCode block). The comment +says title and cwd "come from the session store, which is keyed on the `ses_…` id the runner reports … +so a turn whose id has not been reported yet shows unnamed rather than guessing." Nothing is looked up — +both fields are literal `null`, and `LiveOpenCodeSession` carries only `sessionKey`, so there is no id to +look one up with. They are null permanently, not until-reported. + +The report repeats this as "title/cwd are null until the `ses_…` id is reported". No mechanism exists to +change them. Either say so plainly, or plumb the id into `LiveOpenCodeSession` and do the lookup — but +the comment should not describe the second while the code does the first. + +**2. `protocol.ts` — the new type landed inside another type's docblock.** `LiveOpenCodeSession` was +inserted between `LiveClaudeSession`'s comment and `LiveClaudeSession` itself. That comment (about +`isGenerating`, `pendingTasks`, and what the idle GC consults) now reads as documentation for the +OpenCode type, where it is not merely wrong but directly contradicted by the correct comment immediately +below it. Cosmetic anywhere else; this file is the wire contract between the server and every sidecar. + +--- + +## On `opencode:list` being unexercised + +Your own flag — it has never returned a non-empty list — is the right thing to have flagged, and it +compounds with the defect above: the first time it *is* exercised with two messages on one session, it +will return `[]` for a turn that is plainly running, and that will look like the enumeration being broken +rather than the supersede path deleting the handle. Fix the supersede first, then exercise it; otherwise +the five minutes of testing produces a misleading result. + +--- + +## Agreed next, unchanged from your list + +Your four suggestions are the right four. Ordering note only: **item 4 (B7's latent spurious `cut-off`) +before the fork decision**, for the reason you gave yourself — your B2 fix deepens the mask, and a latent +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.