mirror of
https://github.com/nexu-io/open-design.git
synced 2026-10-27 09:51:12 +01:00
* 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.
616 lines
26 KiB
TypeScript
616 lines
26 KiB
TypeScript
import { describe, expect, it } from 'vitest';
|
|
|
|
import {
|
|
ACP_CLI_SESSION_REFUSED_CODE,
|
|
acpRpcErrorId,
|
|
isAcpCliSessionRefusalText,
|
|
isAcpHandshakeRpcErrorText,
|
|
withAcpHandshakeFailureGuidance,
|
|
} from '../src/runtimes/acp-handshake-failure.js';
|
|
import { classifyRunFailure } from '../src/run-failure-classification.js';
|
|
import { decideSafeRunRetry } from '../src/run-retry-policy.js';
|
|
|
|
function classify(errorCode: string | null, error: string) {
|
|
return classifyRunFailure({
|
|
result: 'failed',
|
|
status: { status: 'failed', error, errorCode },
|
|
...(errorCode ? { errorCode } : {}),
|
|
agentId: 'kimi',
|
|
});
|
|
}
|
|
|
|
/**
|
|
* The same classification with the `runtime_close` diagnostic an ACP fatal
|
|
* really carries.
|
|
*
|
|
* `deriveRpcCloseReason` in server.ts records `fatal_rpc_error` whenever the
|
|
* host tears the child down after a protocol failure, so every ACP-fatal run
|
|
* reaching the classifier has this event in its stream. Omitting it — as the
|
|
* bare `classify` helper does — hides the branch that decides whether the
|
|
* failure keeps its retry, which is exactly what the readiness-exit regression
|
|
* turned on.
|
|
*/
|
|
function classifyWithAcpFatalClose(errorCode: string, error: string) {
|
|
return classifyRunFailure({
|
|
result: 'failed',
|
|
status: { status: 'failed', error, errorCode },
|
|
errorCode,
|
|
agentId: 'kimi',
|
|
events: [
|
|
{ event: 'diagnostic', data: { type: 'runtime_close', rpc_close_reason: 'fatal_rpc_error' } },
|
|
],
|
|
});
|
|
}
|
|
|
|
function retryDecisionFor(error: string) {
|
|
const failure = classify('AGENT_EXECUTION_FAILED', error);
|
|
return decideSafeRunRetry({
|
|
result: 'failed',
|
|
attemptCount: 0,
|
|
failure: {
|
|
...(failure?.failure_category ? { failure_category: failure.failure_category } : {}),
|
|
...(failure?.failure_detail ? { failure_detail: failure.failure_detail } : {}),
|
|
...(failure?.failure_stage ? { failure_stage: failure.failure_stage } : {}),
|
|
...(failure ? { retryable: failure.retryable } : {}),
|
|
},
|
|
sideEffects: {},
|
|
random: () => 0,
|
|
});
|
|
}
|
|
|
|
describe('acpRpcErrorId', () => {
|
|
it('reads the request id out of an ACP JSON-RPC error line', () => {
|
|
expect(acpRpcErrorId('json-rpc id 2: Internal error')).toBe(2);
|
|
expect(acpRpcErrorId('json-rpc id 11: Internal error')).toBe(11);
|
|
expect(acpRpcErrorId('ACP session exited before completion (code=1, signal=none)')).toBeNull();
|
|
expect(acpRpcErrorId('')).toBeNull();
|
|
});
|
|
|
|
it('treats only the two handshake requests as handshake failures', () => {
|
|
// session.ts numbers `initialize` 1 and `session/new` / `session/load` 2;
|
|
// model selection and `session/prompt` take 3 and up.
|
|
expect(isAcpHandshakeRpcErrorText('json-rpc id 1: Internal error')).toBe(true);
|
|
expect(isAcpHandshakeRpcErrorText('json-rpc id 2: Internal error')).toBe(true);
|
|
expect(isAcpHandshakeRpcErrorText('json-rpc id 3: Internal error')).toBe(false);
|
|
expect(isAcpHandshakeRpcErrorText('json-rpc id 12: Internal error')).toBe(false);
|
|
expect(isAcpHandshakeRpcErrorText('thread/start failed')).toBe(false);
|
|
});
|
|
});
|
|
|
|
describe('withAcpHandshakeFailureGuidance', () => {
|
|
const RAW = 'json-rpc id 2: Internal error';
|
|
|
|
it('names the failure with a code and structured identity instead of a sentence', () => {
|
|
const payload = withAcpHandshakeFailureGuidance(
|
|
{ message: RAW },
|
|
{ agentName: 'Kimi CLI' },
|
|
) as {
|
|
message: string;
|
|
error: { code: string; message: string; retryable: boolean; details: Record<string, unknown> };
|
|
};
|
|
|
|
// A code crosses the daemon/web boundary; an English paragraph does not.
|
|
// The web maps this code to localized copy (see amr-guidance.ts), so the
|
|
// identity the copy interpolates has to travel as data, not as prose.
|
|
expect(payload.error.code).toBe(ACP_CLI_SESSION_REFUSED_CODE);
|
|
expect(payload.error.details).toMatchObject({
|
|
kind: 'agent_cli',
|
|
action: 'update_cli',
|
|
agent: 'Kimi CLI',
|
|
});
|
|
// No CLI build is reported. Naming the version this run started with needs
|
|
// a pre-spawn `--version` read, which this failure deliberately does not
|
|
// buy — the copy says "the installed version" and stays true without it.
|
|
expect(payload.error.details).not.toHaveProperty('agentCliVersion');
|
|
// A CLI build that refuses `session/new` refuses it again; the payload says so.
|
|
expect(payload.error.retryable).toBe(false);
|
|
});
|
|
|
|
it('leaves the agent line verbatim on both message surfaces', () => {
|
|
const payload = withAcpHandshakeFailureGuidance(
|
|
{ message: RAW, error: { code: 'AGENT_EXECUTION_FAILED', message: RAW } },
|
|
{ agentName: 'Kimi CLI' },
|
|
) as { message: string; error: { message: string } };
|
|
|
|
// `run.error` is read from `error.message ?? message` and is BOTH the
|
|
// classifier's input and the text the card shows under 「查看错误详情」.
|
|
// Appending a `Details:` restatement made the card print it twice.
|
|
expect(payload.message).toBe(RAW);
|
|
expect(payload.error.message).toBe(RAW);
|
|
expect(JSON.stringify(payload)).not.toMatch(/Details:/i);
|
|
expect(JSON.stringify(payload)).not.toMatch(/refused to start a session/i);
|
|
});
|
|
|
|
it('degrades cleanly when the agent name was never resolved', () => {
|
|
const payload = withAcpHandshakeFailureGuidance({ message: RAW }) as {
|
|
error: { details: Record<string, unknown> };
|
|
};
|
|
expect(payload.error.details).toMatchObject({ kind: 'agent_cli', action: 'update_cli' });
|
|
expect(payload.error.details).not.toHaveProperty('agent');
|
|
expect(payload.error.details).not.toHaveProperty('agentCliVersion');
|
|
expect(JSON.stringify(payload)).not.toContain('undefined');
|
|
expect(JSON.stringify(payload)).not.toContain('null');
|
|
});
|
|
|
|
it('keeps the agent\'s own error data alongside the identity it adds', () => {
|
|
const payload = withAcpHandshakeFailureGuidance(
|
|
{
|
|
message: RAW,
|
|
error: { code: 'AGENT_EXECUTION_FAILED', message: RAW, details: { retryable: true } },
|
|
},
|
|
{ agentName: 'Kimi CLI' },
|
|
) as { error: { details: Record<string, unknown> } };
|
|
expect(payload.error.details).toMatchObject({
|
|
retryable: true,
|
|
kind: 'agent_cli',
|
|
agent: 'Kimi CLI',
|
|
});
|
|
});
|
|
|
|
it('keeps the raw line readable by the classifier', () => {
|
|
const payload = withAcpHandshakeFailureGuidance({ message: RAW }) as {
|
|
error: { message: string };
|
|
};
|
|
// Same text, same classification: naming the failure with a code must not
|
|
// move it off `agent_protocol_error` / `session_init`.
|
|
expect(
|
|
classify(ACP_CLI_SESSION_REFUSED_CODE, payload.error.message),
|
|
).toMatchObject({
|
|
failure_category: 'process_exit',
|
|
failure_detail: 'agent_protocol_error',
|
|
failure_stage: 'session_init',
|
|
user_action: 'install_cli',
|
|
});
|
|
});
|
|
|
|
it('leaves every non-refusal payload untouched by identity', () => {
|
|
for (const raw of [
|
|
'json-rpc id 4: Internal error',
|
|
'ACP session exited before completion (code=1, signal=none)',
|
|
]) {
|
|
const payload = withAcpHandshakeFailureGuidance(
|
|
{ message: raw },
|
|
{ agentName: 'Kimi CLI' },
|
|
);
|
|
expect(payload).toEqual({ message: raw });
|
|
}
|
|
});
|
|
});
|
|
|
|
describe('ACP handshake rejection classification', () => {
|
|
it('attributes a handshake JSON-RPC error to session_init and asks for a CLI fix', () => {
|
|
expect(classify('AGENT_EXECUTION_FAILED', 'json-rpc id 2: Internal error')).toMatchObject({
|
|
failure_category: 'process_exit',
|
|
failure_detail: 'agent_protocol_error',
|
|
failure_stage: 'session_init',
|
|
user_action: 'install_cli',
|
|
});
|
|
});
|
|
|
|
it('leaves a prompt-time protocol error on child_close', () => {
|
|
expect(classify('AGENT_EXECUTION_FAILED', 'json-rpc id 4: Internal error')).toMatchObject({
|
|
failure_category: 'process_exit',
|
|
failure_detail: 'agent_protocol_error',
|
|
failure_stage: 'child_close',
|
|
user_action: 'retry',
|
|
});
|
|
});
|
|
});
|
|
|
|
describe('ACP handshake rejection retry policy', () => {
|
|
it('does not retry a handshake rejection — the same CLI refuses again', () => {
|
|
const decision = retryDecisionFor('json-rpc id 2: Internal error');
|
|
expect(decision.shouldRetry).toBe(false);
|
|
// Classification already marks the handshake rejection non-retryable, so
|
|
// that is the reason recorded end to end.
|
|
expect(decision).toMatchObject({ retrySuppressedReason: 'not_retryable' });
|
|
});
|
|
|
|
it('suppresses on the stage even when the agent claims the error is retryable', () => {
|
|
// Second layer: `rpcErrorRetryable(details)` lets an agent mark its own
|
|
// JSON-RPC error retryable. The policy must still refuse a handshake-stage
|
|
// protocol error, otherwise a CLI that lies about retryability re-creates
|
|
// the endless-retry loop this fix removes.
|
|
const decision = decideSafeRunRetry({
|
|
result: 'failed',
|
|
attemptCount: 0,
|
|
failure: {
|
|
failure_category: 'process_exit',
|
|
failure_detail: 'agent_protocol_error',
|
|
failure_stage: 'session_init',
|
|
retryable: true,
|
|
},
|
|
random: () => 0,
|
|
});
|
|
expect(decision.shouldRetry).toBe(false);
|
|
expect(decision).toMatchObject({ retrySuppressedReason: 'unsafe_failure_stage' });
|
|
});
|
|
|
|
it('still retries protocol errors that are not the handshake', () => {
|
|
expect(retryDecisionFor('json-rpc id 4: Internal error').shouldRetry).toBe(true);
|
|
expect(
|
|
retryDecisionFor('ACP session exited before completion (code=1, signal=none)').shouldRetry,
|
|
).toBe(true);
|
|
});
|
|
|
|
it('keeps every other transient process-exit detail retryable', () => {
|
|
for (const failure_detail of [
|
|
'qoder_stop_sequence',
|
|
'session_resume_expired',
|
|
'stream_error',
|
|
'fatal_rpc_error',
|
|
] as const) {
|
|
expect(
|
|
decideSafeRunRetry({
|
|
result: 'failed',
|
|
attemptCount: 0,
|
|
failure: {
|
|
failure_category: 'process_exit',
|
|
failure_detail,
|
|
failure_stage: 'session_init',
|
|
retryable: true,
|
|
},
|
|
random: () => 0,
|
|
}).shouldRetry,
|
|
).toBe(true);
|
|
}
|
|
});
|
|
});
|
|
|
|
// Found by running a real Kimi CLI in an unauthenticated state, not by reading
|
|
// the code: the CLI answers `initialize`, then rejects `session/new` with
|
|
// `json-rpc id 2: Authentication required`. Because the handshake reading keyed
|
|
// on the JSON-RPC id alone, that user was told to update or reinstall a CLI
|
|
// that was working fine — the one thing they needed to do was sign in.
|
|
//
|
|
// Everything in this block is a handshake-numbered failure. What separates
|
|
// these from the CLI-refusal shape above is not *when* they happened but
|
|
// *why*: each names a cause with its own remedy.
|
|
describe('handshake failures that name their own remedy', () => {
|
|
const AUTH_REQUIRED = 'json-rpc id 2: Authentication required';
|
|
const UNAUTHORIZED = 'json-rpc id 1: HTTP 401 Unauthorized';
|
|
const RATE_LIMITED = 'json-rpc id 2: rate limit exceeded';
|
|
const NO_BALANCE = 'json-rpc id 2: insufficient balance';
|
|
const UPSTREAM_DOWN = 'json-rpc id 2: HTTP 503 Service Unavailable';
|
|
const REMEDIED = [AUTH_REQUIRED, UNAUTHORIZED, RATE_LIMITED, NO_BALANCE, UPSTREAM_DOWN];
|
|
|
|
it.each(REMEDIED)('is still recognised as handshake-stage: %s', (raw) => {
|
|
// The id question and the cause question are separate. These all happened
|
|
// inside the handshake; none of them is the CLI refusing on its own.
|
|
expect(isAcpHandshakeRpcErrorText(raw)).toBe(true);
|
|
expect(isAcpCliSessionRefusalText(raw)).toBe(false);
|
|
});
|
|
|
|
it.each(REMEDIED)('leaves the agent error intact: %s', (raw) => {
|
|
const payload = withAcpHandshakeFailureGuidance(
|
|
{ message: raw, error: { code: 'AGENT_EXECUTION_FAILED', message: raw } },
|
|
{ agentName: 'Kimi CLI' },
|
|
) as { message: string; error: { message: string } };
|
|
expect(payload.message).toBe(raw);
|
|
expect(payload.error.message).toBe(raw);
|
|
});
|
|
|
|
it.each(REMEDIED)('never files it as a CLI-version refusal: %s', (raw) => {
|
|
// The whole card hangs off the code now, so a wrong code here would tell a
|
|
// signed-out / throttled user to change their perfectly good CLI build.
|
|
expect(withAcpHandshakeFailureGuidance({ message: raw })).toEqual({ message: raw });
|
|
});
|
|
|
|
it('files an unauthenticated handshake under auth rather than install_cli', () => {
|
|
expect(classify('AGENT_EXECUTION_FAILED', AUTH_REQUIRED)).toMatchObject({
|
|
failure_category: 'auth',
|
|
user_action: 'login',
|
|
});
|
|
// …including when the CLI wraps the reason in a JSON-RPC `Internal error`
|
|
// envelope, which is the shape `isAgentProtocolErrorText` matches. Before,
|
|
// the envelope won and the run was filed as a CLI-install problem.
|
|
expect(
|
|
classify('AGENT_EXECUTION_FAILED', 'json-rpc id 2: Internal error: 401 Unauthorized'),
|
|
).toMatchObject({
|
|
failure_category: 'auth',
|
|
user_action: 'login',
|
|
});
|
|
});
|
|
|
|
it('files a throttled handshake under rate_limit rather than install_cli', () => {
|
|
expect(
|
|
classify('AGENT_EXECUTION_FAILED', 'json-rpc id 2: Internal error: 429 rate limit exceeded'),
|
|
).toMatchObject({ failure_category: 'rate_limit' });
|
|
});
|
|
|
|
it('files the remaining named causes under the category that owns them', () => {
|
|
expect(classify('AGENT_EXECUTION_FAILED', UNAUTHORIZED)).toMatchObject({
|
|
failure_category: 'auth',
|
|
user_action: 'login',
|
|
});
|
|
expect(classify('AGENT_EXECUTION_FAILED', RATE_LIMITED)).toMatchObject({
|
|
failure_category: 'rate_limit',
|
|
});
|
|
expect(classify('AGENT_EXECUTION_FAILED', NO_BALANCE)).toMatchObject({
|
|
failure_category: 'insufficient_balance',
|
|
});
|
|
expect(classify('AGENT_EXECUTION_FAILED', UPSTREAM_DOWN)).toMatchObject({
|
|
failure_category: 'upstream_unavailable',
|
|
});
|
|
});
|
|
|
|
it('does not retry a signed-out handshake either', () => {
|
|
// Suppressed as an auth failure rather than as a handshake rejection —
|
|
// a different, more accurate reason for the same outcome. Re-running while
|
|
// still signed out would reproduce it just as reliably.
|
|
const decision = retryDecisionFor(AUTH_REQUIRED);
|
|
expect(decision.shouldRetry).toBe(false);
|
|
expect(decision).toMatchObject({ retrySuppressedReason: 'non_retryable_category' });
|
|
});
|
|
|
|
it('still answers a bare CLI refusal with the upgrade guidance', () => {
|
|
// A CLI that answered `initialize` and then rejected the session with a
|
|
// bare protocol error, or with a method it does not implement, has told us
|
|
// nothing except that this build cannot do it. That is the case the
|
|
// upgrade copy was written for, and it is unchanged.
|
|
for (const raw of [
|
|
'json-rpc id 1: Internal error',
|
|
'json-rpc id 2: Internal error',
|
|
'json-rpc id 2: Method not found',
|
|
'json-rpc id 2: Invalid params',
|
|
]) {
|
|
expect(isAcpCliSessionRefusalText(raw)).toBe(true);
|
|
}
|
|
// …and a post-session error is still none of this module's business.
|
|
expect(isAcpCliSessionRefusalText('json-rpc id 4: Internal error')).toBe(false);
|
|
expect(isAcpHandshakeRpcErrorText('json-rpc id 4: Internal error')).toBe(false);
|
|
|
|
const payload = withAcpHandshakeFailureGuidance(
|
|
{ message: 'json-rpc id 2: Internal error' },
|
|
{ agentName: 'Kimi CLI' },
|
|
) as { message: string; error: { code: string } };
|
|
expect(payload.error.code).toBe(ACP_CLI_SESSION_REFUSED_CODE);
|
|
expect(payload.message).toBe('json-rpc id 2: Internal error');
|
|
expect(
|
|
classify('AGENT_EXECUTION_FAILED', 'json-rpc id 2: Internal error'),
|
|
).toMatchObject({
|
|
failure_category: 'process_exit',
|
|
failure_stage: 'session_init',
|
|
user_action: 'install_cli',
|
|
});
|
|
});
|
|
});
|
|
|
|
// The wording of a JSON-RPC rejection is the agent's choice, not a signal. A
|
|
// CLI build that cannot open a session says so as `Internal error`, as `Method
|
|
// not found`, or as `Invalid params` depending on which layer refused — and
|
|
// every one of them means the same thing: this build answered `initialize` and
|
|
// then would not start a session.
|
|
//
|
|
// These run through the event shape a real ACP failure produces, because that
|
|
// is where the wording used to decide the outcome: `server.ts` emits a
|
|
// `runtime_close` diagnostic carrying `rpc_close_reason: 'fatal_rpc_error'`
|
|
// before classifying, and that close reason is enough to promote an unclaimed
|
|
// failure to a retryable `fatal_rpc_error` on `child_close`.
|
|
describe('handshake rejections the agent did not word as "Internal error"', () => {
|
|
const REFUSALS = [
|
|
'json-rpc id 1: Internal error',
|
|
'json-rpc id 2: Internal error',
|
|
'json-rpc id 1: Method not found',
|
|
'json-rpc id 2: Method not found',
|
|
'json-rpc id 2: Invalid params',
|
|
'json-rpc id 2: Server error',
|
|
];
|
|
|
|
/** The events a real ACP handshake failure leaves behind before classification. */
|
|
function acpCloseEvents(error: string, retryable?: boolean) {
|
|
return [
|
|
{
|
|
event: 'error',
|
|
data: {
|
|
error: {
|
|
code: 'AGENT_EXECUTION_FAILED',
|
|
message: error,
|
|
...(retryable === undefined ? {} : { retryable }),
|
|
},
|
|
},
|
|
},
|
|
{
|
|
event: 'diagnostic',
|
|
data: {
|
|
type: 'runtime_close',
|
|
rpc_close_reason: 'fatal_rpc_error',
|
|
status: 'failed',
|
|
},
|
|
},
|
|
];
|
|
}
|
|
|
|
function classifyAcpClose(errorCode: string, error: string, retryable?: boolean) {
|
|
return classifyRunFailure({
|
|
result: 'failed',
|
|
status: { status: 'failed', error, errorCode },
|
|
errorCode,
|
|
agentId: 'kimi',
|
|
events: acpCloseEvents(error, retryable),
|
|
});
|
|
}
|
|
|
|
function retryFor(errorCode: string, error: string, retryable?: boolean) {
|
|
const failure = classifyAcpClose(errorCode, error, retryable);
|
|
return decideSafeRunRetry({
|
|
result: 'failed',
|
|
attemptCount: 0,
|
|
failure: failure
|
|
? {
|
|
failure_category: failure.failure_category,
|
|
failure_detail: failure.failure_detail,
|
|
failure_stage: failure.failure_stage,
|
|
retryable: failure.retryable,
|
|
}
|
|
: {},
|
|
sideEffects: {},
|
|
random: () => 0,
|
|
});
|
|
}
|
|
|
|
it.each(REFUSALS)('files it as a handshake-stage CLI refusal: %s', (raw) => {
|
|
expect(classifyAcpClose('AGENT_EXECUTION_FAILED', raw)).toMatchObject({
|
|
failure_category: 'process_exit',
|
|
failure_detail: 'agent_protocol_error',
|
|
failure_stage: 'session_init',
|
|
retryable: false,
|
|
user_action: 'install_cli',
|
|
});
|
|
});
|
|
|
|
it.each(REFUSALS)('reaches the same verdict once the code is stamped: %s', (raw) => {
|
|
// By the time the run is classified, the ACP payload rewrite has already
|
|
// replaced the error code with `AGENT_CLI_SESSION_REFUSED`. The verdict
|
|
// must not depend on which of the two codes classification happens to see,
|
|
// or the run lands in the `unknown` bucket and stops being triageable.
|
|
expect(classifyAcpClose(ACP_CLI_SESSION_REFUSED_CODE, raw)).toMatchObject({
|
|
failure_category: 'process_exit',
|
|
failure_detail: 'agent_protocol_error',
|
|
failure_stage: 'session_init',
|
|
retryable: false,
|
|
user_action: 'install_cli',
|
|
});
|
|
});
|
|
|
|
it.each(REFUSALS)('refuses to re-run it, even when the CLI calls it transient: %s', (raw) => {
|
|
expect(retryFor('AGENT_EXECUTION_FAILED', raw).shouldRetry).toBe(false);
|
|
expect(retryFor('AGENT_EXECUTION_FAILED', raw, true).shouldRetry).toBe(false);
|
|
expect(retryFor(ACP_CLI_SESSION_REFUSED_CODE, raw, true).shouldRetry).toBe(false);
|
|
});
|
|
|
|
// The guard the fix must not trade away: ids 3 and up belong to model
|
|
// selection and `session/prompt`, so a session already existed and the
|
|
// failure stays the transient, retryable protocol error it has always been.
|
|
it.each([
|
|
'json-rpc id 3: Internal error',
|
|
'json-rpc id 3: Method not found',
|
|
'json-rpc id 4: Invalid params',
|
|
'json-rpc id 12: Internal error',
|
|
])('leaves a post-session protocol error transient: %s', (raw) => {
|
|
const failure = classifyAcpClose('AGENT_EXECUTION_FAILED', raw);
|
|
expect(failure?.failure_stage).not.toBe('session_init');
|
|
expect(failure?.user_action).not.toBe('install_cli');
|
|
expect(failure?.retryable).toBe(true);
|
|
expect(retryFor('AGENT_EXECUTION_FAILED', raw).shouldRetry).toBe(true);
|
|
});
|
|
});
|
|
|
|
// A handshake failure that the run classifier already recognises has a remedy
|
|
// of its own, and that remedy is the one the user must follow. Reading only the
|
|
// three `classifyAgentServiceFailure` classes left every other recognised cause
|
|
// — prompt size above all — looking like an unexplained CLI refusal, so a user
|
|
// whose content was too long was told their CLI version was incompatible.
|
|
describe('handshake failures the run classifier already has a remedy for', () => {
|
|
const ALREADY_REMEDIED: ReadonlyArray<readonly [string, string, string]> = [
|
|
[
|
|
'json-rpc id 2: [code=request_too_large] request body exceeds configured limit',
|
|
'prompt_too_large',
|
|
'reduce_context',
|
|
],
|
|
[
|
|
'json-rpc id 2: prompt is too long: 210000 tokens > 200000 maximum',
|
|
'prompt_too_large',
|
|
'reduce_context',
|
|
],
|
|
['json-rpc id 2: Authentication required', 'auth', 'login'],
|
|
['json-rpc id 2: rate limit exceeded', 'rate_limit', 'retry'],
|
|
];
|
|
|
|
it.each(ALREADY_REMEDIED)(
|
|
'does not read %s as an unexplained CLI refusal',
|
|
(raw, category) => {
|
|
// Precedence, not a second signature list: whatever `classifyRunFailure`
|
|
// can already name, the ACP rewrite must leave alone.
|
|
expect(classify('AGENT_EXECUTION_FAILED', raw)).toMatchObject({
|
|
failure_category: category,
|
|
});
|
|
expect(isAcpCliSessionRefusalText(raw)).toBe(false);
|
|
},
|
|
);
|
|
|
|
it.each(ALREADY_REMEDIED)(
|
|
'ships %s to the client under its own remedy, not the CLI upgrade copy',
|
|
(raw) => {
|
|
// The predicate and the classifier must not prescribe two different
|
|
// fixes for one failure: the card would say "update your CLI" while the
|
|
// telemetry says "shorten the prompt".
|
|
expect(withAcpHandshakeFailureGuidance({ message: raw })).toEqual({ message: raw });
|
|
const payload = withAcpHandshakeFailureGuidance(
|
|
{ message: raw, error: { code: 'AGENT_EXECUTION_FAILED', message: raw } },
|
|
{ agentName: 'Kimi CLI' },
|
|
) as { error: { code: string; details?: Record<string, unknown> } };
|
|
expect(payload.error.code).not.toBe(ACP_CLI_SESSION_REFUSED_CODE);
|
|
expect(payload.error.details?.action).not.toBe('update_cli');
|
|
},
|
|
);
|
|
|
|
it('keeps the user action the classifier assigns', () => {
|
|
for (const [raw, , action] of ALREADY_REMEDIED) {
|
|
expect(classify('AGENT_EXECUTION_FAILED', raw)?.user_action).toBe(action);
|
|
}
|
|
});
|
|
|
|
// A handshake-numbered frame carrying an OS-level crash banner reports a
|
|
// child that DIED; the JSON-RPC envelope is only how the corpse arrived. The
|
|
// remedy is the crash's (ship a compatible runtime), never "change your CLI
|
|
// version". Sampled from a real AMR failure on Windows.
|
|
it('leaves a crashed child to the crash reading, not the refusal reading', () => {
|
|
const CRASHED =
|
|
'json-rpc id 2: start opencode server: opencode exited before readiness: exit status 0xc0000409';
|
|
expect(isAcpHandshakeRpcErrorText(CRASHED)).toBe(true);
|
|
expect(isAcpCliSessionRefusalText(CRASHED)).toBe(false);
|
|
expect(classify('AGENT_SIGNAL_SIGTERM', CRASHED)).toMatchObject({
|
|
failure_category: 'process_exit',
|
|
failure_detail: 'process_crashed',
|
|
user_action: 'none',
|
|
});
|
|
expect(withAcpHandshakeFailureGuidance({ message: CRASHED })).toEqual({ message: CRASHED });
|
|
});
|
|
|
|
// The same AMR wrapper text WITHOUT a crash banner, which is the common case:
|
|
// vela's bundled OpenCode simply exited before it answered a health check (a
|
|
// port collision, an OOM kill, a half-written config). vela reports that from
|
|
// inside `session/new`, so the frame is handshake-numbered — but the agent CLI
|
|
// refused nothing, and its build is not the variable. This shape was a
|
|
// retryable `fatal_rpc_error` before the refusal guidance existed, and both
|
|
// halves of the misfire matter: the user is told to replace a healthy CLI,
|
|
// and the automatic retry that actually recovers a startup race is withdrawn.
|
|
it('leaves a bundled runtime that never started to the startup reading', () => {
|
|
const NEVER_READY =
|
|
'json-rpc id 2: start opencode server: opencode exited before readiness: exit status 3';
|
|
expect(isAcpHandshakeRpcErrorText(NEVER_READY)).toBe(true);
|
|
expect(isAcpCliSessionRefusalText(NEVER_READY)).toBe(false);
|
|
expect(classifyWithAcpFatalClose('AGENT_EXECUTION_FAILED', NEVER_READY)).toMatchObject({
|
|
failure_category: 'process_exit',
|
|
failure_detail: 'fatal_rpc_error',
|
|
retryable: true,
|
|
user_action: 'retry',
|
|
});
|
|
expect(withAcpHandshakeFailureGuidance({ message: NEVER_READY })).toEqual({
|
|
message: NEVER_READY,
|
|
});
|
|
});
|
|
|
|
// The wrapper's other startup shapes, from the same two vela call sites
|
|
// (`acp_runtime.go` newSession/loadSession). None of them is a statement
|
|
// about the agent CLI's own build, and none is deterministic — so the
|
|
// invariant asserted here is the one that matters to the user, not the
|
|
// bucket: keep the retry, never prescribe a CLI change. Which bucket each
|
|
// lands in is a different question, and an earlier branch that recognises a
|
|
// more specific cause (a readiness wait that timed out is a `timeout`) is
|
|
// free to claim it.
|
|
it.each([
|
|
'json-rpc id 2: start opencode server: opencode readiness timed out for http://127.0.0.1:51423',
|
|
'json-rpc id 2: start opencode server: allocate localhost port: bind: address already in use',
|
|
'json-rpc id 2: opencode exited before readiness',
|
|
])('does not read %s as an unexplained CLI refusal', (raw) => {
|
|
expect(isAcpHandshakeRpcErrorText(raw)).toBe(true);
|
|
expect(isAcpCliSessionRefusalText(raw)).toBe(false);
|
|
const failure = classifyWithAcpFatalClose('AGENT_EXECUTION_FAILED', raw);
|
|
expect(failure).toMatchObject({ retryable: true });
|
|
expect(failure?.user_action).not.toBe('install_cli');
|
|
expect(failure?.failure_stage).not.toBe('session_init');
|
|
});
|
|
});
|