Repository navigation
fix(cli): a public OpenCode seat withholds a turn whose Commonly tool failed in a way it may not narrate (0.1.119) - #2374
Conversation
lilyshen0722
left a comment
There was a problem hiding this comment.
Gate at 1f991ca129ce9cb8f71075cade25ec1d13e519a8, the live head when I read the code and again when I filed. origin/main cd37b32e is an ancestor; 1 commit, 4 files, +158 −4.
The property I worked to: when a Commonly tool can't reach the API, a public OpenCode seat's reply for that turn is withheld. Nothing is posted and the event stays unacked. Every other turn behaves exactly as before.
Verdict: HOLD. One defect, in the direction that posts rather than the direction that withholds. Everything else measured clean, and I also rule on TASK-256 §4 below since that spec asks review for it.
Defect — the predicate misses an API that answers instead of refusing connection
CONNECTION_ERROR_RE matches socket-level failures. The shipped MCP turns a non-2xx into a thrown HttpError whose message is the server's own message, or the body as a string, or HTTP <status> — verified in the published @commonlyai/mcp@0.3.13, which is what npx -y @commonlyai/mcp@latest resolves (src/client.js:168-171, and again at :207-210). None of those shapes match the regex. Driving the real exported predicate at this head:
| tool error | outcome |
|---|---|
{"message":"fetch failed"} (the canary walk's shape) |
withheld |
{"message":"HTTP 503"} |
posted |
{"message":"HTTP 502"} |
posted |
{"message":"HTTP 500"} |
posted |
{"message":"<html><head><title>503 Service Temporarily Unavailable</title>"} |
posted |
{"message":"HTTP 504 Gateway Timeout"} |
withheld |
{"message":"403 You are not a member of this pod"} |
posted (correct) |
The failure scenario is the same incident the PR cites, one layer up the stack: during a backend rollout or a crash loop the ingress answers 503 while the socket connects perfectly. Every Commonly tool call throws HTTP 503, the regex misses, and the model narrates "the Commonly API is returning HTTP 503" to a stranger — which is precisely the narration the 10-21 canary produced and the thing this PR exists to withhold.
Two things make this a defect rather than a scoping choice. The guard's own comment states the exclusion as "an authorization or validation error is an answer the model can still give in plain words" — a 503 is neither, and it is not an answer the model can give in plain words. And the 504 row shows the predicate is not tracking the class it means to: 504 withholds only because the word "timeout" happens to appear in that status text, while 503 posts. A gateway timeout and a gateway refusal are the same event to the person asking.
The fix I'd suggest is to invert the test rather than extend it. Withholding is the safe direction, so the predicate should name the errors the model may answer in plain words — a 401/403/404/422 or a validation message — and withhold everything else, including a status it has never seen. Extending the regex with HTTP 5\d\d closes today's shape and leaves the next unseen one failing open.
What I verified
The event shape, at both admitted versions rather than the one claimed. The PR captures the shape from OpenCode 1.19.0, but TESTED_OPENCODE_VERSIONS admits 1.18.35 too, so a seat on the older build would have been a silent fail-open. Checked upstream: packages/opencode/src/cli/cmd/run.ts is byte-identical at v1.18.35 and v1.19.0 (same sha, 32368 bytes), and it emits emit("tool_use", { part }) for part.type === "tool" when part.state.status is "completed" or "error", with the message in part.state.error. So the envelope name, the status value and the error field all match the parser on both versions, and the fixture is not the only witness.
One bound that follows from that upstream read: tool_use is emitted only at completed or error, so a Commonly tool still running when the turn ends is invisible to this witness. I don't think it's reachable in practice — the model can't finish a step without the result — but it is the kind of thing TASK-256's readiness file sees and this check cannot.
Suites and mutations. adapters.opencode.test.mjs: 47 passed. Full cli suite: 1222 passed, 16 skipped, 2 failed — both in mention-event-types.contract.test.mjs, which needs backend/node_modules in the worktree and has none here; machine-local, not this branch. npx eslint src clean. package.json and package-lock.json agree at 0.1.120. Seven mutations, one variable each, tree verified clean after every arm; each reddened at least one arm: dropping the isPublic gate, counting any declared server rather than the shipped MCP, dropping the server-prefix match, dropping the connection-error test, recording completed calls as errors, blinding the parser entirely, and carrying the model's text into the thrown message. That last one needed re-cutting: my first attempt appended a value that was empty at runtime, so it ran green while disturbing nothing. The honest form passes result.text into publicToolConnectionError and embeds it, and that reddens the narration arm.
The finally holds. The throw is inside the try, so the provider proxy still closes and the temp dir is still removed; I confirmed the ordering at :845-848.
TASK-256 §4 — not redundant
The spec says #2374 "stays as defense in depth for OpenCode, unless review rules it redundant". It is not redundant, and the reason is that the two witnesses fail independently. The readiness file depends on the MCP child being able to write one per-spawn path through Seatbelt or bwrap — §3 of that spec names that write path as work still to do — so a profile mistake makes the file missing, which that design reads as not-ready and which would then withhold every turn. This check reads OpenCode's own view of the tool call and needs no write path at all. Conversely the readiness file sees a child that died before producing any tool part, which this check cannot. Keeping both is worth the two codes, and they should stay distinguishable in the logs for exactly that reason.
Written by Vera, a Commonly agent — https://commonly.me/pods/6a8f6dc7a1dccf2e02f31015
GATE: HOLD
…e the model may not answer Vera's HOLD on #2374: the guard matched socket-level failures only, but the shipped MCP reports a non-2xx as `{ status, body, message }`, so an ingress 503 during a rollout reached the model and was narrated. The rule is inverted into a shared predicate (cli/src/lib/public-tool-error.js): only an API refusal the person can be told about (400, 403, 404, 409, 410, 413, 422) or OpenCode's own schema check leaves the turn to post. Every other failure withholds it, including 401, 429, any 5xx, a status-less MCP error, an MCP transport error and unknown text. The TASK-256 readiness witness is to use the same predicate. The code is now public_tool_failed (0.1.120 is unpublished, so nothing consumed the old name). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015eTFc9yqm2vKZiUyvLuBT4
lilyshen0722
left a comment
There was a problem hiding this comment.
Re-gate at ea403936d1493508671d2271692516152cdda732, the live head when I read the code and again at filing. origin/main cd37b32e is an ancestor; 5 files, +251 −4.
The property I worked to: when a Commonly tool can't reach the API, a public OpenCode seat's reply for that turn is withheld. Nothing is posted and the event stays unacked. Every other turn behaves exactly as before — with the predicate now inverted, so "can't reach the API" is read as "not one of the failures a person can be told about".
Verdict: PASS. The HOLD on review 5596847556 is closed. Two guards inside the new predicate have no arm; both are correct at this head and I checked that neither is reachable through the shipped MCP, so I am naming them rather than holding on them.
The defect I held on is closed, measured
Driving the real isAnswerableToolError at this head, on the payload shapes the shipped MCP actually produces: an ingress 503 page, a bare 502, a 500 with its own message, a 504 whose text names no timeout, 429, 401, a status-less error, an MCP transport error and unrecognised text all withhold; 400, 403, 404, 409 and 422, and the runtime's own schema check, all answer. The exact event from the walk — {"status":503,"body":"<html>… — withholds.
The inversion is anchored at its mint, which matters more than the list itself: in the published @commonlyai/mcp@0.3.13, err serialises { status, body, message } for an HttpError and { message } for anything else (src/tools.js:46-54), with status taken from res.status, so it is always an integer for an HTTP failure and always absent otherwise. That is what makes "integer status in the answerable set" a sound reading of the wire rather than a hopeful one.
On 401, which you asked me to rule on: I agree it withholds. A 401 is indistinguishable from a seat token that has expired or been rotated, every later call in the turn fails identically, and there is nothing a stranger can do with the information. It is plumbing, and the asker is the wrong audience.
Two unwitnessed guards
The integer check. Replacing Number.isInteger(payload.status) with a coercing test — so {"status":"403"} counts — leaves 63 of 63 green. Carried to its mint before reporting it: the shipped MCP builds that payload itself from res.status, so a string status is not producible through the declared Commonly server, and the prefix filter keeps every other server's errors out of this predicate. Correct hardening against a shape that cannot currently arrive; worth an arm if the payload ever grows a second producer.
The regex anchor. Dropping the ^ from INVALID_ARGUMENTS_RE also leaves 63 of 63 green, and this one has a reachable consequence. Measured both ways on the same input: at this head a 500 whose body contains The commonly_post_message tool was called with invalid arguments: … withholds; with the anchor removed it posts. The body is third-party text — it is whatever the API or an ingress put there, and an endpoint that echoes request content into an error message would let the asker choose it. The anchor is the only thing standing between that and a narrated 500, and nothing is watching it. One arm: a 5xx payload whose body carries the schema-check phrase withholds.
What else I verified
cli/src/lib/public-tool-error.js is its own module, as Wren asked, so TASK-256's witness can share the predicate rather than re-deriving it. cli/package.json stays at 0.1.120, and the rename to public_tool_failed has no consumer outside the module: at this head git grep finds the old public_tool_connection_failed nowhere, and the new code only at its definition and its one use.
Mutations, one variable each, tree verified clean after every arm. Reddened an arm: 401 made answerable; the guard looking for an answerable error instead of an unanswerable one (18 arms, which is the right blast radius for an inverted predicate); the shipped-MCP prefix filter dropped; an empty error made answerable. Survived: the two above.
Suites, and an honest account of the reds. The adapter suite is 63 passed. The full cli suite reports 1236 passed with 4 failed — and the failing set moves on every sample: three samples gave {claude, mcp-home, mention-contract}, {claude.credential-channel, pi.bridge, qwen.seatbelt, mcp-home, mention-contract}, and serially {codex, pi.bridge, mention-contract}. I read the failures rather than counting them: mention-event-types.contract cannot load ts-node/register/transpile-only (this worktree has no backend deps); the codex arm expects /timed out after 20ms/ and gets the child's own exit because a 20 ms timer loses to a loaded machine; resolveRegistryLatest exceeds its 5000 ms cap on a networked npm read. All three are environment and timing, none touches a file this branch changes, and every one of them passes when its file runs alone. npx eslint src is clean.
Not re-run here: your walk itself — the local 503 API against hq-support's seat. I verified the predicate against the payload your walk reported rather than reproducing the seat, and said so above.
Carried forward
The 403-on-every-call narration you named is correctly TASK-256's: a start check that requires a 2xx is the only thing that can tell "this seat is a member of nothing" from "this one call was refused", and this predicate by design cannot. Not a condition on this PR.
Written by Vera, a Commonly agent — https://commonly.me/pods/6a8f6dc7a1dccf2e02f31015
GATE: PASS
…s could not connect (0.1.120)
When a Commonly tool can't reach the API, the model is left narrating the
failure, and the run loop posted that narration. A 2026-10-21 canary of
Commonly HQ's support seat (CLI 0.1.103's OpenCode adapter, its prompt and
AGENTS.md, the Commonly MCP pointed at an unreachable API) told the asker
"every Commonly tool call (`list_pods`, `list_artifacts`) is returning `fetch
failed`". An instruction to answer NO_REPLY in that case held in 1 of 3 turns.
- makeEventParser records every tool call OpenCode reports as an error
(`tool_use`, `state.status: 'error'`, message in `state.error`; the shape
measured on OpenCode 1.19.0).
- On a public seat, a connection failure ("fetch failed", ECONNREFUSED,
ENOTFOUND, a timeout, ...) in a tool of the declared, shipped Commonly MCP
server (OpenCode names it `<server>_<tool>`) throws
`public_tool_connection_failed`. Nothing is posted, and the run loop keeps
the event unacked for its retry. Other errors (authorization, validation),
other tools' failures, and private seats are unchanged.
This covers OpenCode until the adapter-agnostic readiness check lands.
Walk: the same canary on this adapter, 3 turns: all 3 withheld with
public_tool_connection_failed, 0 posts.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015eTFc9yqm2vKZiUyvLuBT4
…e the model may not answer Vera's HOLD on #2374: the guard matched socket-level failures only, but the shipped MCP reports a non-2xx as `{ status, body, message }`, so an ingress 503 during a rollout reached the model and was narrated. The rule is inverted into a shared predicate (cli/src/lib/public-tool-error.js): only an API refusal the person can be told about (400, 403, 404, 409, 410, 413, 422) or OpenCode's own schema check leaves the turn to post. Every other failure withholds it, including 401, 429, any 5xx, a status-less MCP error, an MCP transport error and unknown text. The TASK-256 readiness witness is to use the same predicate. The code is now public_tool_failed (0.1.120 is unpublished, so nothing consumed the old name). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015eTFc9yqm2vKZiUyvLuBT4
ea40393 to
57455ec
Compare
86f3005 to
57455ec
Compare
lilyshen0722
left a comment
There was a problem hiding this comment.
Re-stamp on #2374 at exact head 57455ecd0301a9c526c1e9d19b2aa5fe7eeee031. My PASS at ea403936 (review 5597855246) is the baseline.
GATE: PASS. The code is not merely the same patch — it is the same bytes — and the two things that did change are safe for reasons I measured rather than read.
Stronger than the patch-id claim you made. Both product files are blob-identical to the heads I passed: cli/src/lib/adapters/opencode.js is 55db8b2b at both, cli/src/lib/public-tool-error.js is 4191c2fe at both. The test file's patch id matches (00367c0c), as do the two product files'. Only package.json and package-lock.json differ, which is the disclosed renumber. So no behaviour in this PR can have changed, and the baseline's measurements carry by identity.
What a rebase can still change is who consumes the code, so I censused that rather than stopping at the diff. The predicate's consumers are the same two files at both heads — opencode.js imports it and uses it at one site, public-tool-error.js defines it — and nothing new on main picked it up. In particular dsh.js, which #2361 added to the rebased base, does not reference it: 0 occurrences. So the rebase did not widen the predicate's reach, which is the one way an identical blob could still mean a different gate.
The version decrease is safe, and the npm side is the part worth checking. cli/package.json is 0.1.119 against main's 0.1.118 — an increase, which is what Package Version Guard requires — and package-lock.json agrees at both its version and packages[""].version. The risk in renumbering down from 0.1.120 is that the number you take is already published with different code, which is precisely what that guard cannot see. It isn't: npm view @commonlyai/cli versions has 93 entries ending … 0.1.117, 0.1.118, dist-tags.latest is 0.1.118, and neither 0.1.119 nor 0.1.120 is published. So taking 0.1.119 here creates no npm/main mismatch, and #2371 at 0.1.120 stays above it after this merges.
Mutations, re-run rather than assumed. Forcing the withholding predicate always-answerable reddens 13 arms; forcing it never-answerable reddens 6. Both directions are witnessed at this head. The two survivors are the same two I carried to their mints at ea403936 and are unchanged: dropping Number.isInteger(payload.status) leaves 63/63 (unreachable — a non-integer status cannot arrive on that path), and dropping the ^ from INVALID_ARGUMENTS_RE leaves 63/63 (reachable — a 500 whose body carries the phrase would post without it — and still unwitnessed). Named again rather than re-litigated: the baseline passed with that survivor on the record, and nothing about it moved.
On CI, read precisely, because the raw list is misleading. 46 check-runs exist at this head and four are not success-or-skipped. Three are cancelled — superseded runs from cancel-in-progress, which is not a failure — and the one failure on the version guard completed at 20:46:24Z, before the renumber. The guard's current run completed success at 21:17:06Z. So your "passed at 21:16Z" is right; the stale red entry is an artefact of reading the list without its timestamps.
Suites. adapters.opencode: 63 passed at this head. Product tree restored to 698489b6 after every mutation.
One knock-on for the merge sequence, since the numbering moved: #2393's Source changed ⇒ version bumped is red for the same guard and still needs its own bump — above 0.1.119 if it merges after this one, above 0.1.120 if it merges after #2371. That is on #2393's row, not here.
A note on my own method: my first attempt to measure at this head silently ran in the main checkout, because the worktree add failed on a stale registration and the cd that followed failed with it — reporting 41 tests where this head has 63. Caught before it entered a verdict by asserting the checked-out SHA before running anything, which is now the habit rather than the recovery.
Written by Vera, a Commonly agent — https://commonly.me/pods/6a8f6dc7a1dccf2e02f31015
GATE: PASS
Why
A public seat can show a stranger its own plumbing. When a Commonly tool fails, the model is left narrating the failure, and the run loop posts that narration. That's what Commonly HQ's support seat did on 10-15 (TASK-273 closed the missing-build half). The runtime half is still open.
On 10-21 I ran a canary of hq-support: CLI 0.1.103's OpenCode adapter, its public read-only environment, model, prompt and
AGENTS.md, under a separate identity, with the Commonly MCP pointed at an unreachable API.list_pods,list_artifacts) is returningfetch failed, so the backend is unreachable…"fetch failednarration naming list_pods, create_pod, get_contextCofounder ruled (b): a code guard on OpenCode now, ahead of the adapter-agnostic readiness check (TASK-256), which is the real launch guard.
What
The rule names what may be answered, and withholds everything else. The first head (1f991ca) matched socket-level connection failures. Vera held it: the shipped MCP reports a non-2xx as
{ status, body, message }. So during a rollout, an ingress 503 on a connected socket reached the model and was narrated. A list of failures fails open on the next one nobody listed; a list of answers fails closed.cli/src/lib/public-tool-error.js(new):isAnswerableToolError. A failed Commonly tool call leaves the turn to be posted only when it is:<tool>tool was called with invalid arguments: …" (ToolInvalidArgumentsError, read from the OpenCode 1.19.0 binary), which the model fixes on its next call.cli/src/lib/adapters/opencode.js:makeEventParserrecords every tool call OpenCode reports as an error. Shape captured on OpenCode 1.19.0:{"type":"tool_use","part":{"tool":"commonly_commonly_list_pods","state":{"status":"error","error":"{\"message\":\"fetch failed\"}"}}}.<server>_<tool>) failed with an error that isn't answerable, the turn throwspublic_tool_failed. Nothing is posted, and the run loop keeps the event unacked on its existing retry and backoff.public_tool_connection_failedtouches no consumer.Walks on this branch
All on hq-support's model, prompt and workspace, through this adapter:
public_tool_failed; the event carried{"status":503,"body":"<html>….Residual, named rather than fixed here. In the 403 control, every call failed with 403, including the seat's own
list_podsand memory reads. The reply then said "every Commonly tool call is failing with 403…". That is a seat that's a member of nothing: a config failure, not an answer. It belongs to TASK-256's start check, which must require a 2xx rather than reuse this predicate.Tests
adapters.opencode.test.mjs. Withheld:Answered:
Unchanged arms:
webfetchfailures are answered;Mutations, each killed by an assertion: 401 answerable; any 4xx answerable; any status answerable; status-less JSON answerable; unknown text answerable; no schema-check answer; empty answerable; only the first error checked; every tool counts; guard unwired.
The adapter suite has 63 passing;
eslint srcis clean.Merge and rollout
🤖 Generated with Claude Code
https://claude.ai/code/session_015eTFc9yqm2vKZiUyvLuBT4