From d7b223127ae042cc3f34b66a65423357d9803385 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Padez?= Date: Mon, 10 Aug 2026 02:55:18 +0000 Subject: [PATCH] delete the dead serve-turn client, and answer phase 2's blocking question first MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 1 item 7, done in the order the parity doc asks: read the dead design into a note, then delete it. `docs/opencode-serve-path.md` records what `event-mapper.ts` and the SSE half of `client.ts` did, and what a rebuild would want back from them — the delta model and tool-state transitions, which are exactly parity Phase 3's token streaming rather than new work. Deleted: `event-mapper.ts` entirely, and `subscribe`/the shared `GET /event` SSE loop, `createSession`, `postMessage`, `abort` from the client, plus `isServerHealthy` from server-manager. All had no callers. `client.ts` goes 200-odd lines to 99. What stays is the REST reads the chat list and transcript use: listSessions, getSession, getMessages, deleteSession, renameSession. While in there, Phase 2's blocking question turned out to be cheap to settle, so it is answered rather than left open. The review asked whether the serve can take a per-request directory, since without one a serve-based turn path would reintroduce the single-directory coupling that shelved this work: POST /session?directory=/tmp/oc-phase2-probe -> directory: "/tmp/oc-phase2-probe" honoured POST /session with directory in the BODY -> directory: "" ignored It is a query parameter on every /session* route. So the coupling is gone on both architectures and the blocker is cleared. The note does NOT start the migration: which of the three options to take is a product call, and it lays them out rather than presuming one. The first probe put `directory` in the body and appeared to prove the opposite. Recorded in the note, because it is the obvious way to test this and it gives a confident wrong answer. Co-Authored-By: Claude Opus 5 --- docs/opencode-serve-path.md | 120 ++++++++++++++ src/servers/api/chat/opencode/client.ts | 144 +---------------- src/servers/api/chat/opencode/event-mapper.ts | 146 ------------------ .../api/chat/opencode/server-manager.ts | 14 +- 4 files changed, 132 insertions(+), 292 deletions(-) create mode 100644 docs/opencode-serve-path.md delete mode 100644 src/servers/api/chat/opencode/event-mapper.ts diff --git a/docs/opencode-serve-path.md b/docs/opencode-serve-path.md new file mode 100644 index 00000000..330d317f --- /dev/null +++ b/docs/opencode-serve-path.md @@ -0,0 +1,120 @@ +# The serve-based turn path: what it was, and whether to rebuild it + +Written 2026-08-10, before deleting `event-mapper.ts` and the unused half of +`api/chat/opencode/client.ts`. `docs/opencode-parity.md` item 7 asks for exactly this: read the dead +design into a note first, because Phase 2 may rebuild it. + +It also answers Phase 2's blocking question, which turned out to be cheap to settle. + +--- + +## The question Phase 2 was waiting on + +`docs/opencode-phase0-review.md` narrowed the fork to one thing: + +> Moving turns onto the serve would reintroduce exactly the single-directory coupling that caused the +> original pain, **unless the serve's API can take a per-request directory**. That question — not "why +> was it replaced" — is now the one to answer first. + +**It can. Tested against the running serve (opencode 1.17.9 on this machine):** + +``` +POST /session?directory=/tmp/oc-phase2-probe → { directory: "/tmp/oc-phase2-probe" } ✅ matches +POST /session body { directory: … } → { directory: "" } ❌ ignored +``` + +`directory` is a **query parameter on every `/session*` route** — create, message, abort, fork, +summarize, prompt_async, shell, all of them — alongside a `workspace` param. It is not a body field, +which is why a first probe that sent it in the body appeared to disprove the whole idea. + +**So the coupling is gone on both paths.** `opencode run --dir` anchors a subprocess turn (verified in +the phase-0 review), and `?directory=` anchors a serve turn. The single-server constraint that shelved +this work does not exist in either architecture any more. + +That removes the reason not to move. It does not by itself decide the move — see the trade below. + +--- + +## What the dead code actually was + +Two files, ~200 of ~390 platform-side lines, all reachable from nothing: + +**`event-mapper.ts` (146 lines)** — maps OpenCode's SSE event stream to officer's `ChatEvent`s. Handles +`message.part.updated` (text deltas, tool state transitions), `message.updated`, `session.idle` and +`session.error`. Its shape assumes a _streaming_ source: partial text arriving as deltas, tool calls +transitioning pending → running → completed as separate events. + +**The SSE half of `client.ts`** — `subscribe(sessionId, listener)`, one shared `GET /event` stream per +base URL demultiplexed to per-session listeners, with reconnect. Plus `createSession`, `postMessage`, +`abort`, `isServerHealthy`. + +The live half of `client.ts` stays: `listSessions`, `getSession`, `getMessages`, `deleteSession`, +`renameSession`, all used by `opencode-sessions.ts` for the chat list and transcript reads. + +### Why this matters for a rebuild + +The dead mapper is **not** a sketch to be dusted off — it is a finished, working shape for a design that +was measured against a real event stream. Two things in it are worth keeping if the serve path returns: + +1. **The delta model.** `runner.ts` emits whole `text` blocks because `opencode run --format json` emits + whole blocks; the mapper emits deltas because SSE emits deltas. Token streaming (parity Phase 3, item 11) is not new work on the serve path — it is this file. +2. **Tool-state transitions.** The mapper tracks a tool call across pending/running/completed. The + subprocess path only ever sees the finished call. + +Both are recoverable from git after deletion (`5d077a4` is the last commit where the serve path was +live), which is the argument for deleting rather than keeping it compiled-but-unreachable: an unused +file rots silently against a moving API, and this one is already pinned to a version two minor releases +behind what some machines run. + +--- + +## The trade, now that the blocker is gone + +**Keep the subprocess (`opencode run --dir`)** + +- No rewrite; it works today. +- Permanently forfeits: token streaming, mid-turn injection, background tasks, live-session enumeration, + reattach-by-id, interrupt-without-teardown. Every one of those is a `stdin: 'ignore'` consequence. +- One process per turn, no warm state to leak or garbage-collect. + +**Move turns onto the serve (`POST /session/{id}/message?directory=…`)** + +- Unblocks the whole of parity Phase 3 at once — those six capabilities are all downstream of a + persistent, addressable session. +- Re-adopts an SSE stream officer must keep alive, demultiplex and reconnect. That machinery already + exists in the deleted code, so the cost is smaller than it looks. +- Introduces warm sessions and therefore a lifetime question OpenCode currently does not have: idle GC, + orphan adoption after a restart, the same problems the Claude path spent months getting right. +- The serve becomes load-bearing rather than a convenience. Today a serve crash costs session listing; + then it would cost every turn in flight. + +**A third option, not in the parity doc:** move only what needs the serve. Keep `opencode run` for turns +and add `?directory=`-scoped serve calls for enumeration and reattach. That buys the Live panel and +reattach-by-id without warm sessions or an SSE loop. It does not buy streaming or mid-turn injection, +which are the two most visible gaps. + +## Recommendation + +**Do not start the migration on this pass.** The blocker is cleared and that is the deliverable here; +choosing between the three is a product call about how much OpenCode should behave like Claude, and it +should be made deliberately rather than as a side effect of a cleanup branch. + +If it is taken: the third option first. It is incremental, it is the only one with no lifetime +questions, and it makes the Live panel — which today silently omits every running OpenCode turn — tell +the truth. + +--- + +## Verified facts this note rests on + +| Claim | How | +| ---------------------------------------------------------------------------------- | --------------------------------------------------------- | +| `?directory=` on `POST /session` is honoured | Created a session, read `directory` back: matched | +| A body `directory` is ignored | Same call with the field in the body: got the serve's cwd | +| `directory` is on every `/session*` route | Read the serve's own `/doc` (OpenAPI) | +| `event-mapper.ts` has no importers outside `client.ts` | grep | +| `createSession`/`postMessage`/`abort`/`isServerHealthy` have no callers | grep for call sites | +| `listSessions`/`getSession`/`getMessages`/`deleteSession`/`renameSession` are live | all from `opencode-sessions.ts` | + +Machine note: this server runs opencode **1.17.9**; the phase-0 review's tests ran against **1.18.11** +elsewhere. The `?directory=` result above is from 1.17.9, so it holds on the older of the two. diff --git a/src/servers/api/chat/opencode/client.ts b/src/servers/api/chat/opencode/client.ts index 1157743e..7ac41dd9 100644 --- a/src/servers/api/chat/opencode/client.ts +++ b/src/servers/api/chat/opencode/client.ts @@ -1,143 +1,15 @@ -import type { OpenCodeEvent } from './event-mapper'; -import { logger } from '../logger'; - -// One connection per `opencode serve` base URL: a single shared SSE subscription on `GET /event` -// demultiplexed to per-session listeners, plus the REST calls a turn needs. The SDK-style -// `/session/*` route family is used (feature-complete, incl. DELETE). - -type Listener = (event: OpenCodeEvent) => void; +// One connection per `opencode serve` base URL, for the REST reads the chat list and transcript need. +// The SDK-style `/session/*` route family is used (feature-complete, incl. DELETE). +// +// This used to also hold a shared SSE subscription (`GET /event`) demultiplexed per session, plus +// `createSession`, `postMessage`, `abort` and `isServerHealthy` — the client half of a serve-based turn +// path that was replaced by `opencode run --dir` subprocesses and had no callers left. Removed +// 2026-08-10; `docs/opencode-serve-path.md` records what it did and what a rebuild would want back, +// because Phase 2 may want exactly this shape again. class ServerConnection { - private listeners = new Map>(); - private sseRunning = false; - constructor(private readonly baseUrl: string) {} - subscribe(sessionId: string, listener: Listener): () => void { - let set = this.listeners.get(sessionId); - if (!set) { - set = new Set(); - this.listeners.set(sessionId, set); - } - set.add(listener); - void this.ensureSse(); - - return () => { - const current = this.listeners.get(sessionId); - if (!current) return; - current.delete(listener); - if (current.size === 0) this.listeners.delete(sessionId); - }; - } - - private dispatch(event: OpenCodeEvent): void { - const sessionId = event.properties?.sessionID; - if (typeof sessionId !== 'string') return; - const set = this.listeners.get(sessionId); - if (!set) return; - for (const listener of set) { - try { - listener(event); - } catch (err) { - logger.error('opencode SSE listener threw', { error: String(err) }); - } - } - } - - /** Keep a single `/event` SSE stream open while any session has listeners; reconnect on drop. */ - private async ensureSse(): Promise { - if (this.sseRunning) return; - this.sseRunning = true; - - void (async () => { - while (this.listeners.size > 0) { - try { - const res = await fetch(`${this.baseUrl}/event`, { headers: { accept: 'text/event-stream' } }); - if (!res.body) throw new Error('no SSE body'); - const reader = res.body.getReader(); - const decoder = new TextDecoder(); - let buffer = ''; - - while (true) { - const { value, done } = await reader.read(); - if (done) break; - buffer += decoder.decode(value, { stream: true }); - - // SSE frames are separated by a blank line. - let sep = buffer.indexOf('\n\n'); - while (sep !== -1) { - const frame = buffer.slice(0, sep); - buffer = buffer.slice(sep + 2); - this.handleFrame(frame); - sep = buffer.indexOf('\n\n'); - } - } - } catch (err) { - logger.warn('opencode SSE stream error; will reconnect', { baseUrl: this.baseUrl, error: String(err) }); - } - if (this.listeners.size > 0) await new Promise((r) => setTimeout(r, 500)); - } - this.sseRunning = false; - })(); - } - - private handleFrame(frame: string): void { - const data = frame - .split('\n') - .filter((line) => line.startsWith('data:')) - .map((line) => line.slice(5).trim()) - .join(''); - if (!data) return; - try { - this.dispatch(JSON.parse(data) as OpenCodeEvent); - } catch { - /* non-JSON keep-alive or partial frame */ - } - } - - private async postJson(path: string, body: unknown): Promise { - const res = await fetch(`${this.baseUrl}${path}`, { - method: 'POST', - headers: { 'content-type': 'application/json' }, - body: JSON.stringify(body), - }); - if (!res.ok) throw new Error(`opencode POST ${path} → ${res.status} ${await res.text().catch(() => '')}`); - return (await res.json()) as T; - } - - async createSession(metadata?: Record, title?: string): Promise { - // OpenCode has no per-session `directory` (a session inherits the server's cwd). We instead tag - // the session with our own free-form `metadata` (e.g. { officer: { cwd } }) — round-trips on the - // list + detail endpoints and is never touched by opencode core — to know where it belongs. - const body: Record = {}; - if (metadata) body.metadata = metadata; - if (title) body.title = title; - const session = await this.postJson<{ id?: string }>('/session', body); - if (!session.id) throw new Error('opencode POST /session returned no id'); - return session.id; - } - - async postMessage( - sessionId: string, - providerID: string, - modelID: string, - text: string, - system?: string, - ): Promise { - // `system` is appended to OpenCode's built-in system prompt (additive, not an override) and is - // sent to the LLM as a system message — it never appears as a visible chat part. - const body: Record = { - model: { providerID, modelID }, - parts: [{ type: 'text', text }], - }; - if (system) body.system = system; - await this.postJson(`/session/${sessionId}/message`, body); - } - - async abort(sessionId: string): Promise { - await this.postJson(`/session/${sessionId}/abort`, {}).catch(() => {}); - } - // ── Session history (REST; OpenCode's SQLite store is the source of truth) ── async listSessions(): Promise { diff --git a/src/servers/api/chat/opencode/event-mapper.ts b/src/servers/api/chat/opencode/event-mapper.ts deleted file mode 100644 index cc3bda08..00000000 --- a/src/servers/api/chat/opencode/event-mapper.ts +++ /dev/null @@ -1,146 +0,0 @@ -import type { ChatEvent, MessageCost } from '../types'; - -// Translates OpenCode's SSE events into our internal ChatEvent union — the OpenCode analog of -// sidecar/claude/stream-parser.ts. Shapes verified live against opencode 1.17.9's `GET /event`: -// -// message.part.updated { part:{ id, type } } → learn partID→type -// message.part.delta { partID, delta } → delta (only if partID is a text part) -// message.part.updated { part:{ type:'tool', callID, tool, state } } → tool:start / tool:result -// message.updated { info:{ role:'assistant', cost, tokens } } → stash final cost -// session.idle → result (turn complete) -// session.error → error -// -// IMPORTANT: deltas always carry `field:'text'` even for the model's *reasoning* — reasoning and answer -// are only distinguishable by the delta's part TYPE (a message.part.updated declaring the part as -// `reasoning` vs `text` always precedes that part's deltas). So we gate deltas on partID being a `text` -// part; reasoning-part deltas are dropped (parity with the Claude harness, which hides thinking). -// The turn translator (`sidecar/claude/turn-stream.ts`, shared by both harnesses) flushes the assistant -// text buffer on tool:start and result, so no explicit `text` event is needed — the deltas are enough. - -export type OpenCodeEvent = { - id?: string; - type: string; - properties?: Record; -}; - -type ToolState = { - status?: string; - input?: Record; - output?: string; - error?: string; -}; - -type OpenCodePart = { - id?: string; - type?: string; - callID?: string; - tool?: string; - field?: string; - state?: ToolState; -}; - -type AssistantInfo = { - role?: string; - cost?: number; - tokens?: { input?: number; output?: number }; -}; - -/** - * Returns a stateful handler that maps raw OpenCode events to ChatEvents, invoking `onEvent` for each. - * State tracks per-callID tool progress (to emit start/result exactly once) and the latest assistant - * cost/tokens (emitted with the terminal `result`). - */ -export function createEventMapper(onEvent: (event: ChatEvent) => void) { - const toolStarted = new Set(); - const toolFinished = new Set(); - const partTypes = new Map(); // partID → type (declared before that part's deltas) - let cost: MessageCost = { inputTokens: 0, outputTokens: 0, totalUSD: 0 }; - let done = false; - - const finish = (event: ChatEvent) => { - if (done) return; - done = true; - onEvent(event); - }; - - return function handle(evt: OpenCodeEvent): void { - const p = evt.properties ?? {}; - - switch (evt.type) { - case 'message.part.delta': { - // Only stream deltas belonging to a `text` part — reasoning parts also emit field:'text' deltas. - const partID = p.partID as string | undefined; - if (partID && partTypes.get(partID) === 'text' && typeof p.delta === 'string' && p.delta.length > 0) { - onEvent({ type: 'delta', text: p.delta }); - } - return; - } - - case 'message.part.updated': { - const part = p.part as OpenCodePart | undefined; - if (!part) return; - if (part.id && part.type) partTypes.set(part.id, part.type); - if (part.type !== 'tool' || !part.callID) return; - const callID = part.callID; - const status = part.state?.status; - - if ((status === 'running' || status === 'completed' || status === 'error') && !toolStarted.has(callID)) { - toolStarted.add(callID); - onEvent({ - type: 'tool:start', - toolCallId: callID, - toolName: part.tool ?? 'tool', - toolInput: part.state?.input ?? {}, - }); - } - - if (status === 'completed' && !toolFinished.has(callID)) { - toolFinished.add(callID); - onEvent({ - type: 'tool:result', - toolCallId: callID, - output: String(part.state?.output ?? ''), - isError: false, - }); - } else if (status === 'error' && !toolFinished.has(callID)) { - toolFinished.add(callID); - onEvent({ - type: 'tool:result', - toolCallId: callID, - output: String(part.state?.error ?? 'Tool failed'), - isError: true, - }); - } - return; - } - - case 'message.updated': { - const info = (p.info as AssistantInfo | undefined) ?? (p as AssistantInfo) ?? {}; - if (info.role === 'assistant' && info.tokens) { - cost = { - inputTokens: info.tokens.input ?? 0, - outputTokens: info.tokens.output ?? 0, - totalUSD: info.cost ?? 0, - }; - } - return; - } - - case 'session.idle': { - finish({ type: 'result', cost }); - return; - } - - case 'session.error': { - const error = p.error; - const message = - typeof error === 'string' ? error : ((error as { message?: string })?.message ?? 'OpenCode error'); - finish({ type: 'error', message }); - return; - } - - default: - return; - } - }; -} diff --git a/src/servers/api/chat/opencode/server-manager.ts b/src/servers/api/chat/opencode/server-manager.ts index 549ce2a8..dd1377de 100644 --- a/src/servers/api/chat/opencode/server-manager.ts +++ b/src/servers/api/chat/opencode/server-manager.ts @@ -4,16 +4,10 @@ import { getOpenCodeServerUrl } from './sidecar-server'; // random port (cwd = DATA_PATH/opencode-sidecar) and reports it to the API (getOpenCodeServerUrl). // All OpenCode HTTP traffic routes to whatever port the sidecar last reported. -export async function isServerHealthy(): Promise { - const baseUrl = getOpenCodeServerUrl(); - if (!baseUrl) return false; - try { - const res = await fetch(`${baseUrl}/api/health`, { signal: AbortSignal.timeout(3000) }); - return res.ok; - } catch { - return false; - } -} +// `isServerHealthy` lived here and had no callers — removed 2026-08-10 with the rest of the dead +// serve-turn client. It polled `/api/health`, which is a real route and answers `{"healthy":true}`, so +// it worked; it simply had nothing asking. `/global/health` is the richer one (it also reports the +// version) if a health check is ever wanted back. /** The base URL of the OpenCode server the sidecar is running. Throws if it hasn't reported in yet. */ export async function ensureServer(): Promise<{ baseUrl: string }> {