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;