platform/docs/opencode-phase1-review.md

# 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`.

**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.

---

## 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.