diff --git a/src/agent/step-executor.ts b/src/agent/step-executor.ts index 480efcfd..fc03e2da 100644 --- a/src/agent/step-executor.ts +++ b/src/agent/step-executor.ts @@ -1967,7 +1967,24 @@ function toLlmFailure(err: unknown, ctx: StepContext): LlmFailure { ); } if (err instanceof LlamaServerError) { - if (err.status === null || err.status >= 500) { + // Delegate the status split to `classifyFailure` rather than + // restating it. This arm used to carry its own hardcoded copy + // (`status === null || >= 500` ⇒ transport, everything else ⇒ + // grammar), and because `executeStep` rethrows *this* wrapper — and + // `classifyFailure`'s first line short-circuits on `LlmFailure` — + // the copy, not the classifier, decided the category the user reads. + // The two diverged the moment the taxonomy moved: a 404 from a wrong + // `localModels.url` still surfaced as `Turn failed [grammar]` with no + // unreachable hint. One taxonomy, one place. + // + // `classifyFailure` cannot return `cancelled`/`model`/`tool` for a + // `LlamaServerError` (its own arm returns only `transport` or + // `grammar`, and it is reached before the abort/network branches), + // and an aborted step has already been claimed by the + // `ctx.signal.aborted` check above — so nothing is laundered here. + // Only `transport` becomes a `TransportError`; every other answer + // keeps the historical `GrammarError`. + if (classifyFailure(err) === "transport") { return new TransportError(err.message, err.status, err.url, { cause: err }); } return new GrammarError(err.message, "", { cause: err }); diff --git a/src/llm/fallback/should-advance.test.ts b/src/llm/fallback/should-advance.test.ts index 98f1291e..78941d38 100644 --- a/src/llm/fallback/should-advance.test.ts +++ b/src/llm/fallback/should-advance.test.ts @@ -9,6 +9,10 @@ import { ToolExecutionError, CancelledError, } from "../reliability/llm-failures.js"; +import { + SubscriptionCliAuthError, + SubscriptionCliNotInstalledError, +} from "../provider/subscription-cli/subscription-cli-errors.js"; describe("shouldAdvance", () => { it("advances immediately on a cloud 429", () => { @@ -75,14 +79,72 @@ describe("shouldAdvance", () => { }); }); - it("does NOT advance on a local llama 4xx (grammar category)", () => { - // LlamaServerError with a 4xx status classifies as grammar. + it("does NOT advance on a local llama 400 (request-shape, grammar category)", () => { + // A 400 is the server rejecting THIS request; the next link rejects + // it the same way, so falling over buys nothing. expect(shouldAdvance(new LlamaServerError("x", 400, "http://local"))).toEqual({ advance: false, immediate: false, }); }); + it("advances on a local llama 404 without arming the breaker — the URL serves no completions", () => { + // The endpoint is permanently wrong (bad `localModels.url`, or a + // server that is not a llama-server), so the chain must move off this + // link — and it moves on THIS failure: `advanceFrom` returns the next + // provider whenever `advance` is true, immediate or not. + // `immediate: false` is about the breaker, not the switch: 404 is not + // in the 429/408/5xx provider-down set, so the cooldown that keeps the + // link quarantined across later turns is armed only once the + // consecutive-failure threshold trips. + expect(shouldAdvance(new LlamaServerError("x", 404, "http://local"))).toEqual({ + advance: true, + immediate: false, + }); + }); + + it("advances on a local llama 405 without arming the breaker", () => { + expect(shouldAdvance(new LlamaServerError("x", 405, "http://local"))).toEqual({ + advance: true, + immediate: false, + }); + }); + + it("advances on a local llama 429 and arms the breaker on the first failure", () => { + // Transport category plus an unambiguous provider-down status — the + // `isImmediateSignal` status read already handled 429; it was simply + // unreachable while 4xx classified as grammar. The extra `immediate` + // buys the cooldown straight away, not an earlier switch. + expect(shouldAdvance(new LlamaServerError("x", 429, "http://local"))).toEqual({ + advance: true, + immediate: true, + }); + }); + + it("advances on a local llama 408 and arms the breaker on the first failure", () => { + expect(shouldAdvance(new LlamaServerError("x", 408, "http://local"))).toEqual({ + advance: true, + immediate: true, + }); + }); + + it("advances on a subscription-CLI binary that is not installed", () => { + // It must advance: a missing `claude` binary otherwise pins the chain + // to a provider that can never serve a turn — and the switch happens + // on this first failure. There is no HTTP status to read, so + // `immediate` is false and the breaker cooldown waits for the + // consecutive-failure threshold. + expect( + shouldAdvance(new SubscriptionCliNotInstalledError("claude", "Install it.")), + ).toEqual({ advance: true, immediate: false }); + }); + + it("advances on a subscription-CLI provider that is signed out", () => { + expect( + shouldAdvance(new SubscriptionCliAuthError("codex", "Run /login.")), + ).toEqual({ advance: true, immediate: false }); + }); + it("advances via threshold on a TransportError carrying null status", () => { expect(shouldAdvance(new TransportError("x", null, "u"))).toEqual({ advance: true, diff --git a/src/llm/fallback/should-advance.ts b/src/llm/fallback/should-advance.ts index e7cf240c..1e76ba66 100644 --- a/src/llm/fallback/should-advance.ts +++ b/src/llm/fallback/should-advance.ts @@ -11,8 +11,14 @@ import { TransportError } from "../reliability/llm-failures.js"; * `false` means the error is deterministic (same request fails the * same way everywhere) or is a cancellation — propagate it untouched. * - `immediate`: an unambiguous provider-down signal (429 / 408 / 5xx / - * network-null) that should switch on the FIRST occurrence, bypassing - * the consecutive-failure threshold. + * network-null). This does NOT control whether the chain switches — + * `ProviderFallbackChain.advanceFrom` returns the next link on the + * FIRST fallover-worthy failure whenever `advance` is true, immediate + * or not. What it controls is the breaker: `registerFailure` arms the + * cooldown right away on an immediate signal, instead of waiting for + * `failureThreshold` consecutive failures. So `immediate` decides how + * long the failed link stays quarantined across later turns, not the + * in-turn switch. */ export interface AdvanceDecision { advance: boolean; @@ -29,15 +35,21 @@ const NO: AdvanceDecision = { advance: false, immediate: false }; * - `transport` → advance (provider unreachable). Note every cloud * `OpenAiHttpError` classifies as `transport` regardless of status, so * a 404 model-not-found or a 401 dead key advances too — a different - * link may have the model or a working key. Untyped socket failures + * link may have the model or a working key. The local path now agrees: + * a llama-server 404/405 (the configured URL does not serve + * completions) or 401/403/429 advances for the same reason. A + * subscription-CLI provider whose binary is missing or is signed out + * is the same story with no HTTP in it. Untyped socket failures * (undici's `TypeError: fetch failed` and friends, from surfaces that * do not wrap their own errors) land here too — see `isNetworkError`. * - `model` → advance. This is a *defective completion* from a reachable * provider (truncated / empty / no_stop), not "model not found"; the * same prompt would reproduce it here, so another link is worth a try. - * - `grammar` / `tool` / `cancelled` → do not advance. A grammar/4xx - * failure is request-shape and repeats identically on every provider; - * a tool failure is our own bug; a cancellation is user intent. + * - `grammar` / `tool` / `cancelled` → do not advance. A grammar failure + * is request-shape — an unparseable completion, or the narrow band of + * llama-server 4xx that rejects the request itself (400/413/422) — + * and repeats identically on every provider; a tool failure is our own + * bug; a cancellation is user intent. * * Immediate signals are read off the typed status carried by * `OpenAiHttpError` / `LlamaServerError` / `TransportError`: an explicit diff --git a/src/llm/reliability/classify-failure.test.ts b/src/llm/reliability/classify-failure.test.ts index 963847e2..d5c8748b 100644 --- a/src/llm/reliability/classify-failure.test.ts +++ b/src/llm/reliability/classify-failure.test.ts @@ -8,6 +8,11 @@ import { ToolExecutionError, TransportError, } from "./llm-failures.js"; +import { + SubscriptionCliAuthError, + SubscriptionCliInvocationError, + SubscriptionCliNotInstalledError, +} from "../provider/subscription-cli/subscription-cli-errors.js"; import { classifyFailure } from "./classify-failure.js"; describe("classifyFailure", () => { @@ -29,7 +34,7 @@ describe("classifyFailure", () => { expect(classifyFailure(err)).toBe("transport"); }); - it("maps LlamaServerError 4xx to grammar", () => { + it("maps LlamaServerError request-shape 4xx to grammar", () => { const err = new LlamaServerError("bad grammar", 400, "http://x"); expect(classifyFailure(err)).toBe("grammar"); }); @@ -87,3 +92,62 @@ describe("classifyFailure — raw network failures", () => { expect(classifyFailure(err)).toBe("cancelled"); }); }); + +describe("classifyFailure — llama-server HTTP statuses", () => { + // A 4xx that describes the *endpoint* must not be filed as `grammar`: + // that category blocks fallover (`shouldAdvance`) and tells the user + // their grammar is broken when the real answer is "that URL is not a + // llama-server". A 4xx that rejects the request itself stays grammar. + const cases: Array<[number | null, string]> = [ + [null, "transport"], + [400, "grammar"], + [401, "transport"], + [402, "transport"], + [403, "transport"], + [404, "transport"], + [405, "transport"], + [408, "transport"], + [409, "transport"], + [413, "grammar"], + [422, "grammar"], + [429, "transport"], + [500, "transport"], + [503, "transport"], + ]; + + for (const [status, expected] of cases) { + it(`maps status ${status ?? "null"} to ${expected}`, () => { + const err = new LlamaServerError("boom", status, "http://x"); + expect(classifyFailure(err)).toBe(expected); + }); + } + + it("leaves an unlisted 4xx on the request-shape side", () => { + // Conservative default: only the statuses we can name as + // endpoint/auth/availability earn a fallover. + expect(classifyFailure(new LlamaServerError("x", 418, "http://x"))).toBe( + "grammar", + ); + }); +}); + +describe("classifyFailure — subscription-CLI providers", () => { + it("maps a missing CLI binary to transport, not tool", () => { + // "claude is not on PATH" is a dead provider link, not a bug in our + // tool layer — the chain must be free to try the next provider. + const err = new SubscriptionCliNotInstalledError("claude", "Install it."); + expect(classifyFailure(err)).toBe("transport"); + }); + + it("maps a signed-out CLI to transport, not tool", () => { + const err = new SubscriptionCliAuthError("codex", "Run /login."); + expect(classifyFailure(err)).toBe("transport"); + }); + + it("still treats a failed CLI invocation as a tool failure", () => { + // The binary ran and came back unhappy: that is not evidence the + // link is unusable, so it keeps the non-advancing category. + const err = new SubscriptionCliInvocationError("claude exited 1", 1); + expect(classifyFailure(err)).toBe("tool"); + }); +}); diff --git a/src/llm/reliability/classify-failure.ts b/src/llm/reliability/classify-failure.ts index 08764071..7fa5f4f6 100644 --- a/src/llm/reliability/classify-failure.ts +++ b/src/llm/reliability/classify-failure.ts @@ -1,10 +1,31 @@ import { LlamaServerError } from "../llama-server-client.js"; import { OpenAiHttpError } from "../provider/openai/openai-http.js"; +import { + SubscriptionCliAuthError, + SubscriptionCliNotInstalledError, +} from "../provider/subscription-cli/subscription-cli-errors.js"; import { ToolCallParseError } from "../grammar/tool-call-grammar.js"; import type { LlmFailureCategory } from "./failure-category.js"; import { LlmFailure } from "./llm-failures.js"; import { isNetworkError } from "./network-error.js"; +/** + * llama-server 4xx statuses that describe the *endpoint*, not the request + * we sent it: the URL does not serve completions (404 — a wrong + * `localModels.url`, or a server that is not a llama-server at all), the + * method is not allowed (405), a proxy in front of it wants credentials + * (401/402/403), or the server is busy / timing us out / conflicting + * (408/409/429). None of these repeat identically on a different provider, + * so they are `transport`: the link is unusable, try the next one. + * + * Everything else in the 4xx range stays `grammar` — 400/413/422 are the + * server telling us THIS request was malformed or too large, which the + * next link would reject the same way. + */ +const LLAMA_ENDPOINT_UNAVAILABLE_STATUSES = new Set([ + 401, 402, 403, 404, 405, 408, 409, 429, +]); + /** * Classify any thrown value into the canonical failure taxonomy. * @@ -15,6 +36,13 @@ import { isNetworkError } from "./network-error.js"; * grammar parser errors, abort signals, and anything else treated as * a tool-layer problem by default). * + * The governing rule across every branch: a failure that means "this + * provider is unusable" must not land in a category that blocks + * fallover. `shouldAdvance` only advances on `transport` / `model`, so + * filing an unusable link under `grammar` or `tool` pins the chain to a + * permanently broken provider and hands the user a diagnosis for a + * problem they do not have. + * * The `isNetworkError` branch sits between the abort check and that * default: an untyped socket failure (MCP streamable-http, embeddings, * a vendor SDK with its own `fetch`) is a `transport` problem even @@ -29,6 +57,7 @@ export function classifyFailure(err: unknown): LlmFailureCategory { if (err instanceof LlamaServerError) { if (err.status === null) return "transport"; if (err.status >= 500) return "transport"; + if (LLAMA_ENDPOINT_UNAVAILABLE_STATUSES.has(err.status)) return "transport"; return "grammar"; } // Cloud provider failures are provider-boundary problems whatever the @@ -36,6 +65,17 @@ export function classifyFailure(err: unknown): LlmFailureCategory { // retry budget was already spent inside the HTTP client, matching the // TransportError contract. if (err instanceof OpenAiHttpError) return "transport"; + // A CLI-backed provider whose binary is missing or signed out is the + // same shape of problem as an unreachable HTTP endpoint: this link + // cannot serve the turn, and no other link is implicated. The default + // `tool` arm below would both mislabel it ("Turn failed [tool]" for a + // binary the user never installed) and stop the chain dead. + if ( + err instanceof SubscriptionCliNotInstalledError || + err instanceof SubscriptionCliAuthError + ) { + return "transport"; + } if (isAbortError(err)) return "cancelled"; // Checked after the abort branch on purpose: an aborted request can // surface as ECONNRESET, and a user pressing Esc is not a fallover. diff --git a/src/llm/reliability/failure-category.ts b/src/llm/reliability/failure-category.ts index c2608bee..01e6bd16 100644 --- a/src/llm/reliability/failure-category.ts +++ b/src/llm/reliability/failure-category.ts @@ -1,11 +1,18 @@ /** * Canonical taxonomy of failures surfaced by the agent runtime. * - * - `transport`: llama-server unreachable, network error, HTTP 5xx. + * - `transport`: the provider link is unusable — llama-server + * unreachable, network error, HTTP 5xx, a llama-server + * 4xx that describes the endpoint rather than the request + * (401/402/403/404/405/408/409/429), any cloud HTTP + * failure, or a CLI-backed provider whose binary is + * missing or signed out. Everything here is worth + * retrying on the next link in the fallback chain. * - `grammar`: the completion payload could not be parsed into a valid - * tool call; also covers HTTP 4xx from llama-server which - * generally means the server rejected the grammar or - * request shape. + * tool call; also covers the llama-server 4xx statuses + * that reject THIS request as malformed or oversized + * (400/413/422 and any other unlisted 4xx), which the + * next provider would reject identically. * - `model`: the completion itself is defective (truncated, empty, * or generated without a stop token). Retrying the same * prompt is unlikely to help, so the runtime does not. diff --git a/src/tui/format-agent-error-for-chat.test.ts b/src/tui/format-agent-error-for-chat.test.ts index 49bc435e..1a9908d0 100644 --- a/src/tui/format-agent-error-for-chat.test.ts +++ b/src/tui/format-agent-error-for-chat.test.ts @@ -1,5 +1,18 @@ -import { describe, expect, it } from "vitest"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import { mkdtempSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { AgentLoop } from "../agent/agent-loop.js"; +import { buildDefaultToolRegistry } from "../tools/index.js"; +import { SlotManager } from "../llm/slot-manager.js"; +import { createEmptySessionState } from "../session/session-state.js"; +import { LlamaServerError } from "../llm/llama-server-client.js"; +import type { + CapabilitiesSummary, + SkillCatalogEntry, + ToolDescriptor, +} from "../prompt/stable-prefix.js"; import { formatAgentErrorForChat } from "./format-agent-error-for-chat.js"; describe("formatAgentErrorForChat", () => { @@ -47,3 +60,127 @@ describe("formatAgentErrorForChat", () => { ).toBe("Turn failed [model]: empty completion"); }); }); + +const LOCAL = { + activeProviderIsLocal: true, + llamaUrl: "http://127.0.0.1:19091", +}; + +const TOOLS: ToolDescriptor[] = [ + { + name: "finish", + summary: "Finish the session with a summary.", + argsSchema: '{"summary": string}', + }, +]; + +const CAPS: CapabilitiesSummary = { + platform: "darwin", + arch: "arm64", + browserChannel: "chrome", + workingDir: "/work", + hasClipboard: true, + hasWmctrl: false, + hasNotifications: true, +}; + +const SKILLS: SkillCatalogEntry[] = []; + +describe("formatAgentErrorForChat — llama failures through the real pipeline", () => { + // Drives the WHOLE production path, not a hand-composed imitation of + // it: `AgentLoop` runs a step whose `llmComplete` throws a raw + // `LlamaServerError`; `executeStep` normalises it through + // `toLlmFailure`; the loop's catch calls `classifyFailure` on THAT + // wrapper and emits `loop_failed { category, error }`; the TUI reducer + // (`agent-event-reducer.ts`, "loop_failed" case) hands exactly those two + // fields plus the local-provider context to the formatter. + // + // The `toLlmFailure` link is the point of the exercise. It used to carry + // its own hardcoded copy of the llama status split, so a 404 reached the + // user as `Turn failed [grammar]` however `classifyFailure` was written — + // and a test that called `formatAgentErrorForChat(classifyFailure(err), …)` + // directly stayed green while production stayed broken. Route the + // assertion through the loop and that gap cannot hide. + let workingDir: string; + + beforeEach(() => { + workingDir = mkdtempSync(join(tmpdir(), "atomic-agent-chat-error-")); + }); + + afterEach(() => { + rmSync(workingDir, { recursive: true, force: true }); + }); + + /** + * Run one turn whose only LLM call throws `LlamaServerError(status)`, + * and render the resulting `loop_failed` exactly as the reducer does. + */ + async function chatTextForLlamaStatus(status: number): Promise { + const failures: Array<{ category: string; message: string }> = []; + const loop = new AgentLoop({ + registry: buildDefaultToolRegistry(), + slotManager: new SlotManager(2), + grammar: 'root ::= "ok"', + llmComplete: async () => { + throw new LlamaServerError( + `llama-server returned http ${status}`, + status, + LOCAL.llamaUrl, + ); + }, + toolDescriptors: TOOLS, + capabilities: CAPS, + skillCatalog: SKILLS, + onEvent: (event) => { + if (event.type === "loop_failed") { + failures.push({ + category: event.category, + message: event.error.message, + }); + } + }, + }); + const result = await loop.runTurn( + createEmptySessionState({ id: `s-llama-${status}`, workingDir }), + { + userMessage: "go", + maxSteps: 3, + signal: new AbortController().signal, + }, + ); + expect(result.reason).toBe("failed"); + expect(failures).toHaveLength(1); + return formatAgentErrorForChat( + failures[0]!.category, + failures[0]!.message, + LOCAL, + ); + } + + it("carries the unreachable hint for a llama 404 on a local provider", async () => { + // The one failure where "check your llama URL" is exactly the right + // advice — a wrong `localModels.url`, or a server that is not a + // llama-server — was the one failure that never got it. + const text = await chatTextForLlamaStatus(404); + expect(text).toContain("Turn failed [transport]"); + expect(text).toContain( + "llama-server is not reachable at http://127.0.0.1:19091", + ); + }); + + it("carries the unreachable hint for a llama 405 on a local provider", async () => { + const text = await chatTextForLlamaStatus(405); + expect(text).toContain("Turn failed [transport]"); + expect(text).toContain( + "llama-server is not reachable at http://127.0.0.1:19091", + ); + }); + + it("keeps a llama 400 as a grammar failure with no URL advice", async () => { + // Regression guard for the half that is intentionally unchanged: a + // 400 is the server rejecting THIS request, and the next link would + // reject it identically. + const text = await chatTextForLlamaStatus(400); + expect(text).toBe("Turn failed [grammar]: llama-server returned http 400"); + }); +});