From 41663bc207146e9a368138fa8f213d12148d839b Mon Sep 17 00:00:00 2001 From: Andre Padez Date: Mon, 10 Aug 2026 14:15:34 +0100 Subject: [PATCH] decide the phase 2 fork: turns stay on opencode run MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The serve has a second, newer API surface nobody here had looked at, and it publishes exactly what the parity doc calls impossible under stdin ignore: delivery steer and queue on POST /prompt, an interrupt that does not tear down, and a per-session event stream with an after cursor — the durable-replay machinery officer hand-built for claude, as a primitive. That would have made migrating obvious. It does not execute. A prompt is accepted with an admittedSeq, stored, emits prompt.admitted and prompted, and then never steps. Ruled out separately: the model, the permissions (build is *:allow, no pending requests), the per-request location (the surface is location-scoped via header or a deepObject query, supplied everywhere, no change), and a config gate. The legacy POST /session/id/message?directory= generates fine in 17s, so the serve itself works — only the new pipeline is inert. session.next.* is the tell. And not a version problem, which is the part everything here had backwards: this Mac runs 1.18.11 and alpha runs 1.17.9, measured. The dead pipeline was tested on the NEWER binary. The original "this server runs 1.17.9" meant alpha and was copied to a machine where it was false; corrected in runner.ts and the test. So building against it now would produce code that looks finished and does nothing, which is the failure mode this project keeps rediscovering. One request reopens the question after any upgrade, and the doc names it. Also de-flakes the lifecycle tests: they spawn real processes, and a fixed sleep(750) went red once on a machine busy running these probes. Presence assertions poll now. Co-Authored-By: Claude Opus 5 --- docs/opencode-fork-decision.md | 108 ++++++++++++++++++++ docs/opencode-parity.md | 10 +- src/servers/sidecar/opencode/runner.test.ts | 30 +++++- src/servers/sidecar/opencode/runner.ts | 11 +- 4 files changed, 148 insertions(+), 11 deletions(-) create mode 100644 docs/opencode-fork-decision.md diff --git a/docs/opencode-fork-decision.md b/docs/opencode-fork-decision.md new file mode 100644 index 00000000..5a666aa5 --- /dev/null +++ b/docs/opencode-fork-decision.md @@ -0,0 +1,108 @@ +# The Phase 2 fork: decided, and the probe behind it + +**Decision: turns stay on `opencode run --dir`. Do not migrate to the serve.** + +Taken 2026-08-10 by the agent now owning this work, after probing the serve rather than reasoning about +it. `docs/opencode-serve-path.md` framed the three options and deferred the choice as a product call. +It is no longer a product call — the option that would have justified migrating does not currently run. + +--- + +## What changed the analysis + +The serve exposes **two** API surfaces, and every prior document was written against the older one: + +- `/session/*` — what the deleted client used. `?directory=` per request. +- `/api/session/*` — a newer surface nobody here had looked at. + +The new one publishes, in `/doc`, exactly the capabilities the parity doc lists as impossible for +OpenCode because of `stdin: 'ignore'`: + +| Capability | New API | Parity doc said | +| ----------------------------- | ----------------------------------------------------------- | ---------------------- | +| Mid-turn injection | `POST /prompt` with `delivery: "steer"` | **No** — needs fork | +| Queue behind a turn | same, `delivery: "queue"` | **No** — needs fork | +| Interrupt without teardown | `POST /interrupt` → 204 | **No** — needs fork | +| Token streaming | `GET /event` → `text/event-stream` | **No** — needs fork | +| Reconnect + replay | same, `?after=` — a resumable per-session cursor | not considered | +| Compaction seams | `POST /compact` | "no signal exists" | +| Images | `prompt.files` | Phase 4 | +| Interactive approvals | `/permission`, `/question` + reply/reject | not considered | + +A per-session stream with a cursor is the striking one: it is the durable-replay machinery officer had +to hand-build for Claude, offered as a primitive. That would have made the migration look obvious. + +## Why the answer is still no + +**It does not execute.** On the newest binary we run, a prompt to `/api/session/{id}/prompt` is accepted +(200, with an `admittedSeq`), stored as a user message, emits `session.next.prompt.admitted` and +`session.next.prompted` — and then nothing. No `step.started`, no assistant reply, indefinitely. + +Ruled out, each by a separate probe: + +- **Not the model.** Reproduced with no model, and with an explicit `{providerID: 'opencode', id: + 'claude-sonnet-4-6'}` accepted and echoed back by session create. +- **Not permissions.** `GET /permission` and `GET /question` both return `{"data":[]}`, and the `build` + agent's own config is `*: allow`. +- **Not the missing location.** The new surface is location-scoped per request — `x-opencode-directory` + header, or `?location[directory]=` as a deepObject query (`/doc` confirms `location` is `deepObject`). + Supplying it on every call, including the event stream, changes nothing. +- **Not a config gate.** `/config` has no `experimental` key; `experimental` is null. +- **Not the version.** See below. + +**The serve itself is fine.** The legacy path generates normally: `POST /session/{id}/message?directory=` +returned a complete assistant reply in 17s with cost and tokens, and honoured the directory +(`cwd: /private/tmp/oc-serve-probe`). + +So the split is precise: **the serve can run turns, but only through the old endpoint, which has neither +steer nor queue.** The `session.next.*` event naming is the tell — a next-generation pipeline that +accepts input and is not yet wired to a runner. + +### The version, which everything in this repo had backwards + +`runner.ts` and the phase-0/phase-1 documents state 1.17.9 "here" and 1.18.11 "elsewhere". Measured: + +- **This Mac: 1.18.11** +- **alpha: 1.17.9** + +The other agent's "this server" meant alpha, and the comment was copied without re-measuring. It matters +for exactly one conclusion: the dead pipeline was tested on the **newer** binary, so this is not "we are +behind, upgrade and it works". We are ahead, and it still does not run. + +## What this means for each option + +- **Keep the subprocess** — chosen. It works; verified end to end today (`session:init` → + `assistant:text` → `result` with cost). Forfeits streaming, injection, background tasks, reattach. +- **Migrate to the serve's legacy path** — would buy token streaming and reattach, at the cost of the + SSE demux/reconnect machinery and warm-session lifetime questions (idle GC, orphan adoption — the + problems the Claude path spent months getting right). It would **not** buy steer or queue, which are + the two most visible gaps. Poor trade. +- **Migrate to the serve's new path** — buys everything, and cannot be built against today. + +Building against a preview that accepts input and never runs it is the worst of the three: the code +would look finished and do nothing, which is the failure mode this project keeps rediscovering. + +## The trigger to reopen this + +One command decides it, and it needs no design work: + +``` +POST /api/session/{id}/prompt → does `session.next.step.started` ever arrive? +``` + +When that produces a real turn on the installed binary, the migration becomes clearly worth doing and +should target the **new** surface directly — not the legacy one — because steer, queue, interrupt and a +resumable per-session cursor map one-to-one onto what officer already does for Claude by hand. + +Re-run `POST /api/session` + `POST /prompt` after any opencode upgrade. Until then this is settled, and +`docs/opencode-parity.md`'s bucket 1 should be read as "deferred by evidence", not "pending a decision". + +## Findings worth keeping regardless + +- `delivery` defaults to `"steer"` when omitted — the admitted event says so. +- New-surface responses are wrapped in `{"data": …}`; the legacy surface returns bare objects. Reading + `body.id` instead of `body.data.id` silently yields `undefined`. +- `?after=` on the event stream really does replay history — verified by replaying a finished session's + events after the fact. +- The serve is unauthenticated locally (`OPENCODE_SERVER_PASSWORD` unset), which is how all of the above + was probed with plain `curl`. diff --git a/docs/opencode-parity.md b/docs/opencode-parity.md index db07cc71..2f2f6c82 100644 --- a/docs/opencode-parity.md +++ b/docs/opencode-parity.md @@ -89,8 +89,14 @@ a superseded OpenCode turn ran its whole completion path against the turn that r **What bucket 0 being closed does and does not mean.** Every defect that made OpenCode behave *wrongly* is gone. What remains is bucket 1 — capabilities Claude has and OpenCode does not — and most of the visible ones (token streaming, mid-turn injection, background tasks, interrupt-without-teardown) are -downstream of `stdin: 'ignore'` and therefore of the Phase 2 fork, which is still Andre's call and still -unstarted. `docs/opencode-serve-path.md` frames it. +downstream of `stdin: 'ignore'` and therefore of the Phase 2 fork. + +**The fork is decided: turns stay on `opencode run --dir`.** Not as a preference — the serve publishes a +newer `/api/session/*` surface offering exactly those capabilities natively (`delivery: "steer" | +"queue"`, `/interrupt`, a resumable per-session event stream), and on the newest binary we run it accepts +prompts and never executes them. `docs/opencode-fork-decision.md` has the probe, what was ruled out, and +the single request that reopens the question after an upgrade. Bucket 1 below is therefore **deferred by +evidence**, not pending a decision. --- diff --git a/src/servers/sidecar/opencode/runner.test.ts b/src/servers/sidecar/opencode/runner.test.ts index 316917f5..5932b238 100644 --- a/src/servers/sidecar/opencode/runner.test.ts +++ b/src/servers/sidecar/opencode/runner.test.ts @@ -15,7 +15,8 @@ import { // // This covers the NDJSON → ChatEvent mapping from `opencode run --format json`, which is the piece most // likely to break against a new release: the event shape is not a documented contract, and this project -// already runs two different opencode versions across two machines (1.17.9 here, 1.18.11 elsewhere). +// already runs two different opencode versions across two machines (1.18.11 on the Mac, 1.17.9 on alpha +// — measured 2026-08-10; this file previously had them the wrong way round). // Before this, a shape change would have surfaced as a silently empty or malformed turn. // // The fixtures below are the shapes the live 1.17.9 binary emits. If one of these tests fails after an @@ -148,6 +149,22 @@ afterAll(() => { rmSync(stubDir, { recursive: true, force: true }); }); +/** + * Wait for something to BECOME true, rather than sleeping a guessed interval and hoping. + * + * These tests spawn real processes, so every "has it happened yet" is at the mercy of machine load — + * and a fixed `sleep(750)` duly failed once on a box that was busy running opencode probes. Polling + * makes a slow machine slow instead of red. Absence assertions still need a fixed wait, since there is + * no event to wait for; those are marked where they appear. + */ +async function waitFor(what: () => boolean, timeoutMs = 8000): Promise { + const deadline = Date.now() + timeoutMs; + while (Date.now() < deadline) { + if (what()) return; + await Bun.sleep(25); + } +} + describe('runOpenCodeTurn — a second turn on a live session', () => { it('lets the replacement keep the session: no error, still listed, still killable', async () => { const sessionKey = 'sess-supersede'; @@ -157,8 +174,11 @@ describe('runOpenCodeTurn — a second turn on a live session', () => { runOpenCodeTurn({ sessionKey, prompt: 'first', cwd: stubDir }, CONFIG, emit); runOpenCodeTurn({ sessionKey, prompt: 'second', cwd: stubDir }, CONFIG, emit); - // Let the superseded child actually die. Its exit handler is what used to reach across. - await Bun.sleep(750); + // Wait for the superseded child to actually die — its exit handler is what used to reach across. + // Absence assertion, so there is no event to wait for: give it a generous fixed window instead, and + // wait on something observable (the kill landing) rather than purely on the clock. + await waitFor(() => listRunningOpenCodeTurns().length === 1); + await Bun.sleep(1500); // 1. Nothing is emitted for a turn the system replaced on purpose. This one mattered most: the emit // is committed to chat_session_events by the sidecar, so a false "OpenCode exited" became history. @@ -169,7 +189,7 @@ describe('runOpenCodeTurn — a second turn on a live session', () => { // 3. And still reachable by the stop button, rather than orphaned with no handle. killOpenCodeTurn(sessionKey); - await Bun.sleep(250); + await waitFor(() => messages.some((m) => m.type === 'opencode:event' && m.event.type === 'stopped')); expect(listRunningOpenCodeTurns()).not.toContainEqual({ sessionKey }); expect(messages.some((m) => m.type === 'opencode:event' && m.event.type === 'stopped')).toBe(true); }); @@ -207,7 +227,7 @@ describe('runOpenCodeTurn — a second turn on a live session', () => { runOpenCodeTurn({ sessionKey, prompt: 'only', cwd: stubDir }, { ...CONFIG, bin: FAILING_BIN }, (m) => messages.push(m), ); - await Bun.sleep(750); + await waitFor(() => messages.some((m) => m.type === 'opencode:event' && m.event.type === 'error')); expect(messages.some((m) => m.type === 'opencode:event' && m.event.type === 'error')).toBe(true); expect(listRunningOpenCodeTurns()).not.toContainEqual({ sessionKey }); diff --git a/src/servers/sidecar/opencode/runner.ts b/src/servers/sidecar/opencode/runner.ts index ac5b0007..6f836bca 100644 --- a/src/servers/sidecar/opencode/runner.ts +++ b/src/servers/sidecar/opencode/runner.ts @@ -60,10 +60,13 @@ const running = new Map(); // Shape of `opencode run --format json` events. // -// Verified live against opencode 1.17.9 (this server) and reported working on 1.18.11 elsewhere. Nothing -// enforces either — the binary is whatever is installed on the machine, and two machines in this project -// already differ. `runner.test.ts` pins the mapping itself so a shape change fails a test rather than a -// turn; if it starts failing, re-read the NDJSON from the installed binary before editing the test. +// Verified live against opencode 1.18.11 (this Mac) and 1.17.9 (alpha) — MEASURED on 2026-08-10, having +// previously been recorded the other way round here: the "this server" in the original note meant alpha, +// and the comment was copied to a machine where it was false. Nothing enforces a version anyway; the +// binary is whatever is installed, and the two machines in this project already differ. +// +// `runner.test.ts` pins the mapping so a shape change fails a test rather than a turn; if it starts +// failing, re-read the NDJSON from the installed binary before editing the test. type RunPart = { type?: string; text?: string;