review phase 0, and settle the opencode cwd question by testing it
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 <noreply@anthropic.com>
This commit is contained in:
@@ -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 <cwd>` 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 <the platform repo>` — 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.
|
||||||
Reference in New Issue
Block a user