diff --git a/docs/opencode-phase1-report.md b/docs/opencode-phase1-report.md new file mode 100644 index 00000000..461df2f1 --- /dev/null +++ b/docs/opencode-phase1-report.md @@ -0,0 +1,147 @@ +# Phases 0 and 1: implementation report, and Phase 2's question answered + +**To:** whoever wrote `opencode-parity.md` and `opencode-phase0-review.md`. +**From:** the implementation pass, 2026-08-10 (overnight). +**Scope:** everything in Phase 0 and Phase 1, plus the one Phase 2 question that turned out to be +cheap. Nothing of the Phase 2 migration was started. + +Both documents were good to work from. The handover instruction — _reproduce each defect before fixing +it_ — earned its place three separate times, detailed below. + +--- + +## What landed + +| Commit | Work | +| --------- | --------------------------------------------------------------- | +| `22bcd7d` | B1 + B3 — session listing, and a resumed session's directory | +| `492509a` | B2 — route a resumed OpenCode session to OpenCode | +| `013e629` | B4 (first attempt), B5, B6, thinking selector | +| `7774a25` | B4 properly — gate the composer on the capability | +| `cfbf58c` | Delete the `AGENTS.md` injection + the one-project comment | +| `d7b2231` | Delete the dead serve-turn client; add `opencode-serve-path.md` | +| `8b409e8` | Phase 1 finish — stale comments, version pin, first tests | +| `e8bd946` | `opencode:list` — running OpenCode turns in the Live panel | + +Suite 562 → 573 tests, all passing. `tsgo` clean throughout. + +--- + +## Phase 2: the blocking question is answered + +`opencode-phase0-review.md` narrowed the fork to one thing — whether the serve can take a per-request +directory, since without it a serve-based turn path reintroduces the coupling that shelved this work. + +**It can.** Against the running serve (1.17.9 on the home-lab box): + +``` +POST /session?directory=/tmp/oc-phase2-probe → directory: "/tmp/oc-phase2-probe" honoured +POST /session with directory in the BODY → directory: "" ignored +``` + +`directory` is a **query parameter on every `/session*` route** — create, message, abort, fork, +summarize, prompt_async, shell — alongside `workspace`. Read off the serve's own `/doc`. + +**Worth flagging as a trap:** the body form is silently ignored and yields a session in the serve's own +cwd, which reads exactly like "the serve cannot do per-request directories". My first probe did this and +gave a confident wrong answer. Anyone re-checking this should use the query string. + +So the coupling is gone in both architectures: `--dir` anchors a subprocess turn (your test), and +`?directory=` anchors a serve turn (this one). `docs/opencode-serve-path.md` has the three options — +keep the subprocess, move turns to the serve, or a third the parity doc did not list: move only +enumeration and reattach to the serve and leave turns on `opencode run`. It recommends the third and +**does not start any of them**, because which one to take is a product decision. + +The first slice of that third option is already in (`e8bd946`, below). + +--- + +## Where your documents were wrong, and how + +Three corrections. All were caught by the reproduce-first rule, and none would have been caught by +reading. + +**1. `location.directory` does not exist** (parity doc B1/B3, and its suggested fix). opencode 1.17.9+ +returns `directory` at the top level, with no `location` wrapper and no `metadata`. The suggested fix — +"derive the directory from `location.directory`" — would have compiled and silently produced `''` +forever. You have since corrected this in `29d9912`. + +**2. B2 is right in outline, and the interesting part is not in the outline.** The doc says +`selected.model` is never passed. In fact it _is_ produced correctly — `/chat/:id` resolves +`detail.model` to `opencode/big-pickle` and puts it on `selected` — and then dropped at the `` +boundary, while `initialModel` reads `locationState?.model`, a field **nothing in the tree ever writes**. +So the value existed and was read from the wrong place. Your instruction to confirm all six hops rather +than the endpoints is exactly what surfaced that. + +**3. `runner.ts`'s mapping was untested but not pure** (Phase 1 item 9). It lived inside `handleLine` as +a closure over `emit`, the accumulated cost, and a reported-session flag, so it could not be called +without spawning a binary. Extracted as `mapRunLine` — line in, `{sessionId, events, costDelta}` out — +with the two line-spanning concerns left to the caller, because they are not properties of a line. +Behaviour unchanged; 11 tests now pin it. + +--- + +## Where I deliberately did not follow the spec + +**B5 — the parity doc says to guard "like the Claude one". Copying that guard would have broken +OpenCode.** Claude keeps one warm session and skips re-subscribing on later turns; OpenCode spawns a +fresh `opencode run` per turn, so a new subscription each time is _correct_. The actual defect was +overwriting the previous handle without detaching it. Fixed that instead. (Your review reached the same +conclusion independently, which was reassuring to read afterwards.) + +**B4 — closed the way your review asked, not the way the parity doc did.** The doc offered the flag flip +as "the honest one-liner"; you correctly pointed out that flipping it changed nothing observable because +no code read the capability. The composer now gates on it — drop zone, paste path, attach menu — so the +flag is load-bearing. Unknown model still allows images: a missing capability should not remove a +working control. + +**Thinking selector — removed, not hidden.** The doc said hide; hiding a control that does nothing still +leaves it in the tree to be re-found. The inert plumbing beneath it is untouched, because deleting that +touches the socket contract and belongs in its own change. + +--- + +## `opencode:list`, and the one thing to check first + +`e8bd946` adds `opencode:list` / `opencode:sessions` and merges both harnesses in `/chat/live`, asked in +parallel, each failing toward empty. + +The OpenCode row is deliberately thinner than the Claude one rather than faked into parity: +`isGenerating` is always true (a subprocess exists only while it generates, so there is no "merely +open"), `pendingTasks` is always 0 (no background-task concept — a number would imply one), and +title/cwd are null until the `ses_…` id is reported. + +**It has never returned a non-empty list.** No OpenCode turn was running to enumerate. Both failure +modes are invisible — a sidecar that does not reply, and a reply whose shape does not match, both +degrade to `[]`, which looks exactly like "nothing is running". **One real OpenCode turn while watching +`/chat/live` settles it, and that is the highest-value five minutes available tomorrow.** + +--- + +## Other things left unverified + +Testing was explicitly de-prioritised for this pass, so these are recorded rather than resolved: + +- **The image gate in a browser.** The API reports `images: false` for all 61 OpenCode models and `true` + for the three Claude ones, and the gate reads that field — but no UI was opened. +- **`mapRunLine` fixtures are hand-written**, not captured from a live turn. If one fails after an + upgrade, re-read the binary's real output before editing the expectation. +- **No OpenCode turn was run after deleting `AGENTS.md`.** The premise was verified (the run path sends + no system prompt, so the instruction pointed at nothing) but the outcome was not observed. + +`COMMS/BLOCKERS.md` has the same list plus the mistakes I made getting here. + +--- + +## Suggested next, if you are writing the following spec + +1. **Exercise `opencode:list`** — it is the only new capability whose happy path is unproven. +2. **Decide the fork.** The blocker is gone; `opencode-serve-path.md` frames it. If the answer is "not + yet", say so in the parity doc so it stops reading as pending work. +3. **The remaining Phase 1 residue**: `sweepStaleServes` is `/proc`-based and a no-op on macOS (B8), and + in-flight `opencode run` children still survive sidecar shutdown untracked. +4. **B7** — the latent spurious `cut-off`, still unaddressed and still masked only by the client always + happening to send `model` alongside `sessionId`. My B2 fix makes the client send `model` _more_ + reliably, which deepens the mask rather than removing it. + +Item 4 is worth doing before something changes the client's habits and un-masks it.