platform/docs/opencode-phase1-review.md

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.