mirror of
https://github.com/nexu-io/open-design.git
synced 2026-09-28 13:33:03 +08:00
lefarcen
204ee7a06d
* fix(daemon): explain an ACP handshake rejection instead of retrying it
An agent CLI that answers `initialize` and then refuses `session/new` /
`session/load` surfaced as a bare `json-rpc id 2: Internal error`, and the
retry policy treated it as transient — so the run failed, retried, and failed
identically while the user read a JSON-RPC frame.
Classify a handshake-numbered protocol error as `session_init` /
`install_cli`, keep every later protocol error on `child_close`, and lead the
user-facing message with the CLI and its detected version. The raw line stays
appended because `run.error` is also the classifier's input.
* fix(daemon): route the ACP handshake guidance onto the path users hit
The guidance copy was only reachable through `rewriteKnownAgentStreamError`,
which the ACP failure path never calls. An ACP agent's error goes
`attachAcpSession` -> `fail()` -> the bridge's `send('error', payload)`, and
that callback forwarded the payload verbatim; the close handler then returned
on `acpFatalErrorObservedBeforeCancellation && hasFatalError()` before the
stderr-tail rewrite further down. So a Kimi user still read
`json-rpc id 2: Internal error`.
Rewrite the payload at the bridge instead. That one object is the whole
user-visible surface of an ACP failure: `send` streams it to SSE clients and
`design.runs.emit` reads `run.error` out of it, so correcting it there fixes
both surfaces at once. `withAcpHandshakeFailureGuidance` rewrites only the
message fields, and only for a handshake-numbered JSON-RPC error, so
`AMR_MODEL_UNAVAILABLE`, promoted opencode errors, and post-session protocol
errors keep the shape their own handling depends on. The raw line stays
embedded because `run.error` is also the classifier's input.
Cover it through the full server run cycle with a fake ACP CLI that answers
`initialize` and rejects `session/new`, asserting on what a client and the
telemetry pipeline actually observe. Against the pre-fix tree the run error is
the bare `json-rpc id 2: Internal error`; against the pre-PR tree the daemon
also opened `session/new` twice.
* fix(daemon): read the handshake failure's cause, not just its id
Running a real Kimi CLI while signed out produces `json-rpc id 2:
Authentication required`. The handshake reading keyed on the JSON-RPC id
alone, so that user was told their CLI version was incompatible and they
should update or reinstall it — a prescription for a problem they did not
have, in place of the one sentence that pointed at the fix.
The id answers *when* a failure happened; it can never answer *why*. Split
the two questions: `isAcpHandshakeRpcErrorText` stays structural, and
`isAcpCliSessionRefusalText` is what the copy and the `install_cli` verdict
now hang off — handshake numbering AND no cause with a remedy of its own.
The "has its own remedy" half reuses `classifyAgentServiceFailure` rather
than growing a second signature list: it is agent-agnostic, already covers
auth / quota / upstream, and carries its own suite. It is deliberately
unrelated to, and narrower than, `isCliNotInstalledText` — that predicate
answers "was the binary even there", which a CLI that already answered
`initialize` has plainly settled.
A handshake failure that names its cause now keeps its own text and falls
through to the branch that owns that cause, so telemetry and copy agree: an
unauthenticated handshake is `auth` / `login`, a throttled one is
`rate_limit`. A bare refusal (`Internal error`, `Method not found`) is
unchanged and still gets the CLI guidance and the retry suppression.
No pure-function test could have caught this: the helpers were consistent
with themselves. The wiring test drives a fake ACP CLI that rejects
`session/new` with `Authentication required` and asserts on the text a
signed-out user actually reads.
* test(daemon): pin the category each named handshake cause is filed under
Verifies the routing claimed in the PR body rather than asserting it. Caught
one: `insufficient balance` has its own `insufficient_balance` category, not
`rate_limit`.
* fix(chat): name the ACP session refusal instead of writing its sentence
The guidance this PR added reached the user as a paragraph the daemon
composed: "The Kimi CLI (0.38.0) accepted the connection but refused to start
a session. … Details: json-rpc id 2: Internal error". That is the wrong layer,
and it showed:
- The daemon has no locale. The string went straight into `run.error` and
the chat rendered it verbatim, so a Chinese UI put a Chinese title over an
English body. No amount of rewording fixes that; only moving the sentence
to the client does.
- The paragraph restated the raw agent line, which the card's details block
already prints — the same text twice in one card.
So the daemon now NAMES the failure and stops there: `AGENT_CLI_SESSION_REFUSED`
plus `details: { kind: 'agent_cli', action: 'update_cli', agent, agentCliVersion }`,
with `retryable: false` because a build that refuses `session/new` refuses the
identical request identically. `withAcpHandshakeFailureGuidance` stays the single
invariant point (the ACP bridge payload feeds SSE and `run.error` at once) and now
stamps only the structured half; the stderr-tail fallbacks wrap their payloads in
it too, so no path is left composing prose.
`run.error` keeps the agent's `json-rpc id N: …` line byte for byte. It is the
classifier's input — rewriting it degrades this failure class to `unknown` in
telemetry — and it is what the details block shows, so leaving it alone is also
what removes the duplicate.
The web maps the code to localized copy in `resolveRunFailureUi`, resolved before
every other branch because the copy is not a fixed sentence: it names the CLI
build the daemon detected, and falls back to a version-less wording when there
was none (the same rule the rolling model-window copy follows). The version rides
to the client as data, on the status event next to `code` / `failureDetail`.
Three keys in all 19 locales.
Red first, on the path users hit: the wiring suite (real `startServer` + a fake
ACP CLI) went red on `run.error` being the English paragraph, and a new ChatPane
render spec went red on the raw line appearing twice in one card. Both green here.
Verified in a real browser against a local runtime: the same failed run renders in
zh-CN and in en from one persisted event.
* test(daemon): stop the wiring suite pinning a racy CLI-version probe
CI run 32683047377 caught this on the previous head: the wiring test demanded
`0.38.0` inside `run.error` and got a version-less sentence, because
`getDetectedRuntimeVersions` is a process-wide cache that `probe()` clears
before each probe and refills after — a concurrent `/api/agents` refresh can
leave it empty exactly while a run is failing. Locally it always won the race;
on CI it did not.
The new assertions inherited that: `details.agentCliVersion` was pinned to an
exact string on both wiring cases. Assert the property that actually holds
instead — the version is either the one the daemon detected or absent, never
some other value — and say in the helper why presence is not asserted here.
Both copy paths (version present, version absent) stay covered
deterministically at the layers that own them: the payload unit suite and the
two web suites.
Making detection itself deterministic is a separate finding, still open on
this PR, and deliberately not fixed here.
* fix(daemon): let the classifier, not the wording, name a handshake refusal
Three review findings, one shape: the ACP handshake verdict was being decided
in more than one place, so the places disagreed.
1. The handshake reading sat inside `isAgentProtocolErrorText`, whose JSON-RPC
arm matches only `json-rpc id N: Internal error`. A CLI that refuses
`session/new` with `Method not found` or `Invalid params` skipped it
entirely and was promoted to a retryable `fatal_rpc_error` on `child_close`
— the no-auto-retry behaviour this PR claims never applied to it — or, once
the payload rewrite had stamped its code, fell all the way through to
`unknown` / `finalize`, which is exactly the untriageable bucket the module
docblock says the rewrite must never produce.
The wording an agent picks for its own rejection is not a signal. The
handshake branch now sits AFTER every cause branch instead of inside the
protocol one, so precedence is a fact of the control flow rather than a
promise: whatever auth, rate limit, balance, upstream, prompt size, or CPU
already claimed keeps it, and only an unexplained refusal reaches the last
line. Ids 3 and up never enter it and keep the transient treatment they
have always had.
2. `handshakeFailureNamesItsOwnRemedy` knew only the three
`classifyAgentServiceFailure` classes, so every other cause the run
classifier can already advise on was rewritten into the CLI-upgrade copy.
A user whose content was too long read "your CLI version is incompatible"
while the same run was filed as `prompt_too_large` / `reduce_context`.
`isAcpCliSessionRefusalText` now IS the classifier — it runs
`classifyRunFailure` over the text and reports whether that reading is the
handshake refusal — so the card the user gets and the bucket the run lands
in cannot prescribe two different fixes. The id-only helpers moved to
`runtimes/acp-handshake-id.ts` so the classifier can read the id without the
id-reader having to reach back into the classifier.
3. The guidance read the process-wide detected-version cache at FAILURE time,
while `probe()` cleared each entry before re-reading it. Any `/api/agents`
refresh overlapping a failing run decided whether the user was told which
build refused them; CI caught it (run 32683047377) and the assertion was
loosened instead of the race closed.
Both halves are closed. A probe now publishes its reading once, at the end,
and only when no later probe has already published, so the cache holds the
newest COMPLETED reading rather than the state of one in flight. And the run
freezes the version it is spawning with, so failure-time guidance reads the
run and never the live cache.
Coverage is at the wiring layer, not just the predicates: the fake ACP CLI
gained named gates so a run's handshake and a concurrent `/api/agents` refresh
can be held open together and the failure built at the exact moment the cache
is mid-probe — deterministic, no sleeping on wall-clock timing. The wiring
suite's CLI-version assertion goes back to strict.
One deferral found by the existing suite and now pinned: a handshake-numbered
frame carrying an OS-level crash banner (Windows STATUS_ILLEGAL_INSTRUCTION
from the bundled opencode) reports a child that died, not one that refused, and
stays with the crash reading.
* test(daemon): describe the overlap the gate test actually constructs
The comment said the failure is built "at the precise moment the cache is
blank". That was the pre-fix window; the cache no longer goes blank mid-probe,
which is half of what the test proves. Say what it now holds open — a probe in
flight — and name both halves it exercises.
* fix(daemon): drop the duplicate detection import that blocks startup
`getDetectedRuntimeVersions` was imported twice — once standalone and
once inside the `./runtimes/detection.js` group — so the module has two
declarations of the same lexical binding. Node's ESM loader rejects that
outright with `Identifier 'getDetectedRuntimeVersions' has already been
declared`, i.e. the daemon could not boot.
The suite never caught it: vitest transforms through esbuild, which
tolerates the duplicate, so 179 tests passed against a module real Node
refuses to load. Verified by importing server.ts under Node directly —
SyntaxError before, gone after.
* fix(daemon): report the CLI build this run actually started, on both surfaces
A version-specific refusal (`AGENT_CLI_SESSION_REFUSED`) names the CLI build
that refused, so the user knows which one to change. Two things could make
that name wrong.
**The persisted event dropped it.** `send()` stores every run event on the
assistant message before emitting it, and `runSseEventToPersistedAgentEvent`
serialized only `detail` and `code`. The live card named the build; a reloaded
one fell back to the version-less sentence — exactly when the user comes back
to act on it. The mapper now lifts `error.details.agentCliVersion` out of the
frame, validated as a non-blank string, because `details` is a pass-through bag
that also carries the agent's own JSON-RPC `error.data`.
**The spawn-time capture could name a different executable.**
`getDetectedRuntimeVersions` answers per agent id and says nothing about which
binary the reading came from. A user who repointed the CLI after Settings last
listed agents (Settings writes `KIMI_BIN`; PATH changes) ran one build and was
told to change another. The capture now resolves through
`ensureDetectedRuntimeVersions` with the same `configuredAgentEnv` that
produced this run's `agentLaunch` — cached when the launch identity is
unchanged, re-probed through the same bounded `--version` read when it is not,
and taken before the spawn marks so preflight work is accounted to preflight.
Both are pinned end to end in `acp-handshake-failure-wiring.test.ts`: one test
reads the conversation back through the messages route a reloading client
calls, the other repoints `KIMI_BIN` between detection and the run and reads
the two fake CLIs' invocation logs to establish which child actually ran before
asserting what the failure reports.
* fix(daemon): keep a runtime that never started out of the CLI-refusal verdict
An ACP handshake rejection was decided on the JSON-RPC request id alone, so any
failure numbered 1 or 2 became "this CLI build cannot open a session; change
it". AMR is the population that reaches this path, and what it runs underneath
is OpenCode: when vela's bundled OpenCode child fails to come up it reports
`start opencode server: opencode exited before readiness: exit status 3` from
inside `session/new`, which arrives handshake-numbered while saying nothing
about the agent CLI's own build.
Two things went wrong at once. The user was told to replace a perfectly healthy
CLI, and the automatic retry that actually recovers a startup race was
withdrawn — `process_exit / agent_protocol_error / session_init`,
`retryable: false`, `install_cli`, with the ACP wrapper stamping
`AGENT_CLI_SESSION_REFUSED` on top. A startup race clears on the second
attempt; a CLI-build verdict never does, so the two must not collapse.
`isManagedRuntimeStartupFailureText` names the distinction: a CLI reporting
that a runtime IT manages never came up has named the moving part itself, so
its build is not the only variable left. The markers are vela's own wrapper
text, already trusted by `isCpuUnsupportedCrashText` for the crash-banner
sibling of this shape and now shared with it. Both deferrals now read the same
way: the handshake frame is the envelope, not the evidence — a child that DIED
(crash banner) or one that never STARTED (readiness) is not a refusal.
Also stops this suite's runtime from being a property of the host. The wiring
cases drive `GET /api/agents`, which probes every shipped runtime and resolves
binaries from PATH plus the machine's toolchain directories, so a host with
real agent CLIs installed turns each refresh into a dozen real spawns: ~2.8s
here, ~28s on a reviewer's machine — past the 20s test timeout for the
mid-refresh case, which waits on two. `isolateAgentDetection` points
`OD_AGENT_HOME` at an empty per-test directory, detection's own mechanism for
scoping the search, so the only CLI anything can find is the fixture's. Same
route, same probe path, same deterministic gates and overlap assertions; the
file drops from 109s to 14s and the mid-refresh case from 30s (timing out) to
1.2s.
* revert(daemon): drop the CLI-version half of the handshake-refusal card
The copy wanted to name the build that refused, and the chain that
followed ended in the launch path: the version had to be frozen before
the spawn, freezing meant awaiting a `--version` probe there, and an
await before `spawn()` sits after the run's only pre-spawn cancellation
guard. Stop clicked during that window finishes the run as canceled,
the probe then resolves, and the child is spawned onto an already
terminal run that nothing will signal.
The whole sub-branch goes rather than a second guard. What it bought
was `(0.38.0)` in a sentence that already read correctly without it;
what it cost was a run the user asked to stop starting anyway.
Removed: the pre-spawn `ensureDetectedRuntimeVersions` await,
`run.spawnedAgentCliVersion`, and the version half of
`agentFailureIdentity`; detection's probe generation guard;
`agentCliVersion` persistence, its `PersistedAgentEvent` field, its web
pass-through, and the `{version}` copy variant in all 19 locales.
`server.ts` no longer touches the run lifecycle at all — its diff is
four `send('error', ...)` call sites and one helper.
Detection's generation guard fixes a real, independent daemon bug (for
the length of every `/api/agents` refresh the daemon cannot report the
version it detected). It ships as its own PR, and naming the build in
the copy follows it.
Kept: the localized `AGENT_CLI_SESSION_REFUSED` card in 19 languages,
no auto-retry for a handshake-stage refusal, `run.error` exactly equal
to the agent's own line, auth/quota/upstream/managed-runtime-startup
each keeping their own verdict, and ids 3+ staying out of the branch.
The cases that asserted a version number now assert the same behaviour
without one, plus `not.toHaveProperty('agentCliVersion')` and a locale
guard against a stray `{version}` placeholder, so a partial re-land
cannot slip half of this back in.