From 8ac36ed8b88e4540ae7443bbf2096671e0362ebe Mon Sep 17 00:00:00 2001 From: Andre Padez Date: Mon, 10 Aug 2026 03:44:34 +0100 Subject: [PATCH] review phase 0, and settle the opencode cwd question by testing it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 0 accepted except B4, which is reopened: flipping images to false made the metadata honest but nothing reads that capability, so the composer still accepts a drop and the image is still discarded before the model sees it. The defect described a user-visible lie and the lie is unchanged. Gate the composer on the flag, or plumb images through — the first is the Phase 0 one-liner. The cwd question is answered empirically rather than argued. Against opencode 1.18.11: --dir anchors the agent's file operations, not just the process cwd, and the anchor survives a four-step turn with a write in the middle. So the single-server constraint that shelved this work is already gone — it went away when turns moved off the serve to `opencode run --dir`. Two consequences. Per-directory servers are unnecessary; don't build them. And the injected AGENTS.md telling the agent its working directory is safe to delete — it points at a system prompt the run path never sends, and --dir does the job it was standing in for. Phase 2's fork also narrows: moving turns onto the serve would reintroduce the original coupling unless the serve takes a per-request directory, so that is the question to answer, not why it was replaced. Co-Authored-By: Claude Opus 5 --- docs/opencode-phase0-review.md | 107 +++++++++++++++++++++++++++++++++ 1 file changed, 107 insertions(+) create mode 100644 docs/opencode-phase0-review.md diff --git a/docs/opencode-phase0-review.md b/docs/opencode-phase0-review.md new file mode 100644 index 00000000..9c39e1be --- /dev/null +++ b/docs/opencode-phase0-review.md @@ -0,0 +1,107 @@ +# Phase 0 review, and what the cwd tests found + +Review of `22bcd7d`, `492509a`, `013e629` against `docs/opencode-parity.md`, plus two empirical tests of +the OpenCode cwd behaviour that change what Phase 1 and 2 should be. + +--- + +## Phase 0 — accepted, with one item to reopen + +**B1, B2, B3 — good, and better than the spec.** Each was reproduced against a running server before +being changed, which is what the handover asked for and is what caught the document being wrong: +`location.directory` does not exist in opencode 1.17.9+, which returns `directory` at the top level with +no `location` and no `metadata`. Deleting `officerMeta` and the metadata tag rather than fixing them was +the right call — the only writer had no callers, and tagging would have been a second source of truth for +something `directory` already answers. + +The B2 write-up is a model of what a fix note should be: the six-hop chain confirmed rather than +inferred, and the finding that the value was _produced correctly and read from the wrong place_ rather +than never produced. + +**B5 — correct, including the part where it deliberately does not copy Claude.** A fresh subscription per +turn IS right for OpenCode, because a fresh `opencode run` is spawned per turn; the defect was +overwriting the old handle without detaching it. Mirroring Claude's `_claudeKill` guard would have been +wrong, and the commit says so. + +**B6 — correct, and the restraint is the good part.** Clearing the `ses_…` map in `deleteSession` only, +never in `releaseSession`, with the reason stated: releasing means "let go, leave it running", so a +returning browser must be able to find the same `ses_…` again. Clearing it there would have quietly +orphaned every released OpenCode session. + +**Thinking selector — good.** Removing the control rather than fixing it, and leaving the inert plumbing +for a follow-up that touches the socket contract, is the right split. + +### B4 — reopen: the flag is now honest but inert + +`013e629` flipped `images: true` → `false` for OpenCode models, and the commit says "61 OpenCode models +now decline, the three Claude ones still accept". + +**Nothing declines.** No code in `src/workspaces` or `src/apps` reads that capability — the composer's +image affordances are ungated. Grep for a consumer of the model's `images` field returns nothing: +`InputArea`'s drop zone, the paste handler, and `AttachButton` all accept images regardless of model, and +`useAttachments` collects them regardless. + +So the observable defect is unchanged: on an OpenCode chat you can still drop an image, watch it render +in your own bubble, and have it discarded before the model sees it. The metadata is more truthful now, +which is worth having, but B4 described a user-visible lie and that lie is still on screen. + +Two ways to close it, and they are not equivalent: + +1. **Gate the composer on the capability.** Read the selected model's `images` flag and hide the drop + zone, the paste path and the attach-image button when it is false. Cheap. Makes the flag load-bearing, + so the flip in `013e629` starts doing something. +2. **Plumb images through `OpenCodeRunParams`** (currently Phase 4). Removes the limitation rather than + surfacing it. + +(1) is the honest one-liner Phase 0 was for; (2) is the real fix. Doing (1) now costs nothing if (2) +happens later — the gate simply stops firing once the capability is true. + +--- + +## What the cwd tests found — this changes Phase 1 and 2 + +Andre's memory of why OpenCode was shelved: everything ran through one `opencode serve`, which anchored +every call to the directory that server was started in. That broke listing sessions by cwd and forced the +injected `AGENTS.md` telling the agent what its working directory was supposed to be. + +**That constraint is gone, and it went away when turns moved off the serve.** Turns are now +`opencode run --dir ` subprocesses (`runner.ts:65`). Tested against the installed binary +(**1.18.11**; our comments still pin 1.17.9): + +**Test 1 — does `--dir` anchor file operations, or only the process cwd?** Ran a turn in a scratch +directory containing two marker files. The agent's `ls` returned exactly those two files and nothing from +the repo. **`--dir` anchors the agent.** + +**Test 2 — does the anchor survive a multi-step turn?** Four steps: `pwd`, write a file, `pwd` again, +`ls`. Both `pwd`s returned `/private/tmp/oc-cwd-test`, the file landed there, and the platform repo was +untouched. **No mid-turn drift.** + +One cosmetic residue: each run ends with `Shell cwd was reset to ` — some global +notion of "the project" that is neither the invoking directory nor `--dir`. It prints after the process +finishes and had no effect on either test. Worth knowing it exists; not worth chasing. + +### Consequences + +- **Do not build a server per directory.** It was a sound response to the old architecture and is now + unnecessary — N processes, N ports and a lifecycle to manage, to solve what `--dir` already solves. +- **The injected `AGENTS.md` is safe to delete.** `index.ts:22-35` seeds a file telling the agent to read + "Working directory for this session" from the system prompt; the run path sends no system prompt, so + that instruction points at nothing. `--dir` is the real anchor and it works. This is the "very nasty + hack" — it can go, and nothing needs to replace it. +- **The stale comment goes with it.** `opencode-sessions.ts:6-9` still claims all sessions live in one + server's project. They do not, and the listing now proves it: one serve returned 7 sessions across + several directories. +- **Phase 2's fork is narrower than the doc implies.** 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. + +--- + +## Suggested next work, in order + +1. **B4 properly** — gate the composer on the model's `images` capability (above). +2. **Delete the `AGENTS.md` injection and the stale one-project comment**, now that `--dir` is verified. + This is Phase 1 work and it is the thing Andre most wanted gone. +3. **Then the rest of Phase 1** — the dead `event-mapper.ts` and SSE machinery, the wrong path names in + comments, and the first tests over `runner.ts`'s NDJSON mapping, which is pure, untested, and pinned to + a version two minor releases behind what is installed.