From 1b677b8642f015f8d8a5917a81c6916b1c1df914 Mon Sep 17 00:00:00 2001 From: Valera Brizhatiuk <19537764+plombeer31@users.noreply.github.com> Date: Thu, 3 Sep 2026 02:05:08 +0300 Subject: [PATCH 1/4] fix(llm): provider-availability failures no longer block fallover MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two kinds of "this provider is unusable" failure were filed under categories that shouldAdvance refuses to advance on, so a permanently broken link pinned the whole fallback chain and the user got a diagnosis for a problem they did not have. - llama-server 4xx: every non-null status under 500 mapped to grammar. A 404 (the configured localModels.url does not serve completions) or a 405 read as "Turn failed [grammar]" and stopped the chain. The endpoint/auth/availability statuses (401 402 403 404 405 408 409 429) now classify as transport; the request-shape statuses (400 413 422 and any other unlisted 4xx) stay grammar. - SubscriptionCliNotInstalledError / SubscriptionCliAuthError were plain Errors, so they fell through to the catch-all tool arm. A missing or signed-out claude/codex CLI now classifies as transport. Routing the llama 404 to transport also unblocks the llama-unreachable hint in format-agent-error-for-chat, which is gated on the transport category — the one failure where "check your llama URL" is the right advice was the one that never got it. Doc comments in failure-category.ts, classify-failure.ts and should-advance.ts updated to the new rule. --- src/llm/fallback/should-advance.test.ts | 59 ++++++++++++++++- src/llm/fallback/should-advance.ts | 14 +++-- src/llm/reliability/classify-failure.test.ts | 66 +++++++++++++++++++- src/llm/reliability/classify-failure.ts | 40 ++++++++++++ src/llm/reliability/failure-category.ts | 15 +++-- src/tui/format-agent-error-for-chat.test.ts | 47 ++++++++++++++ 6 files changed, 230 insertions(+), 11 deletions(-) diff --git a/src/llm/fallback/should-advance.test.ts b/src/llm/fallback/should-advance.test.ts index 98f1291e..259f3d2e 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,65 @@ 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 via threshold on a local llama 404 — the URL serves no completions", () => { + // The endpoint is permanently wrong (bad `localModels.url`, or a + // server that is not a llama-server). Not an immediate signal: 404 is + // not in the 429/408/5xx provider-down set, so it advances once the + // consecutive-failure threshold trips. + expect(shouldAdvance(new LlamaServerError("x", 404, "http://local"))).toEqual({ + advance: true, + immediate: false, + }); + }); + + it("advances via threshold on a local llama 405", () => { + expect(shouldAdvance(new LlamaServerError("x", 405, "http://local"))).toEqual({ + advance: true, + immediate: false, + }); + }); + + it("advances immediately on a local llama 429", () => { + // Transport category plus an unambiguous provider-down status — the + // `isImmediateSignal` status read already handled 429; it was simply + // unreachable while 4xx classified as grammar. + expect(shouldAdvance(new LlamaServerError("x", 429, "http://local"))).toEqual({ + advance: true, + immediate: true, + }); + }); + + it("advances immediately on a local llama 408", () => { + expect(shouldAdvance(new LlamaServerError("x", 408, "http://local"))).toEqual({ + advance: true, + immediate: true, + }); + }); + + it("advances on a subscription-CLI binary that is not installed", () => { + // No HTTP status to read, so it advances via the threshold — but it + // must advance: a missing `claude` binary otherwise pins the chain to + // a provider that can never serve a turn. + 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..75070d76 100644 --- a/src/llm/fallback/should-advance.ts +++ b/src/llm/fallback/should-advance.ts @@ -29,15 +29,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..cd9a77a6 100644 --- a/src/tui/format-agent-error-for-chat.test.ts +++ b/src/tui/format-agent-error-for-chat.test.ts @@ -1,5 +1,7 @@ import { describe, expect, it } from "vitest"; +import { LlamaServerError } from "../llm/llama-server-client.js"; +import { classifyFailure } from "../llm/reliability/classify-failure.js"; import { formatAgentErrorForChat } from "./format-agent-error-for-chat.js"; describe("formatAgentErrorForChat", () => { @@ -47,3 +49,48 @@ describe("formatAgentErrorForChat", () => { ).toBe("Turn failed [model]: empty completion"); }); }); + +describe("formatAgentErrorForChat — classified llama failures", () => { + // Mirrors the real pipeline: `agent-loop` classifies the thrown error + // and the reducer hands that category straight to the formatter. The + // hint is gated on `transport`, so the one failure where "check your + // llama URL" is exactly right — a 404 from a wrong `localModels.url` — + // used to be the one failure that never got it. + const local = { + activeProviderIsLocal: true, + llamaUrl: "http://127.0.0.1:19091", + }; + + it("carries the unreachable hint for a llama 404 on a local provider", () => { + const err = new LlamaServerError( + "llama-server returned http 404", + 404, + local.llamaUrl, + ); + const text = formatAgentErrorForChat( + classifyFailure(err), + err.message, + local, + ); + 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", () => { + const err = new LlamaServerError( + "llama-server returned http 400", + 400, + local.llamaUrl, + ); + const text = formatAgentErrorForChat( + classifyFailure(err), + err.message, + local, + ); + expect(text).toBe( + "Turn failed [grammar]: llama-server returned http 400", + ); + }); +}); From 33af1f60d50c3f14012eed43a942dca132568c3b Mon Sep 17 00:00:00 2001 From: Valera Brizhatiuk <19537764+plombeer31@users.noreply.github.com> Date: Thu, 3 Sep 2026 02:37:39 +0300 Subject: [PATCH 2/4] fix(agent): route the llama status split through classifyFailure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit toLlmFailure carried a second, hardcoded copy of the llama-server taxonomy (status null or >= 500 => transport, everything else => grammar). That copy, not classifyFailure, decided the category the user actually reads: executeStep wraps every escaping error through toLlmFailure and rethrows the wrapper, and classifyFailure short-circuits on `err instanceof LlmFailure`, so the new endpoint/availability status set was never consulted on the path that produces the chat message. Concretely, a raw LlamaServerError(404) still surfaced as `Turn failed [grammar]: llama-server returned http 404` with no unreachable hint, and the Sentry clusters CLI-B7 / CLI-BE (category=grammar, cause_type=LlamaServerError) are exactly the signature of the GrammarError constructed here — they would have kept firing at the same rate. Delete the duplicate and ask classifyFailure instead: transport keeps TransportError(message, status, url), anything else keeps the historical GrammarError(message, ""). The cause chain is unchanged in both arms, so the scrubber's causeType still resolves. classifyFailure cannot answer cancelled/model/tool for a LlamaServerError, and an aborted step is already claimed by the ctx.signal.aborted check above this arm, so no other category is laundered into a TransportError. The fallover half was already correct: runWithFallback catches the raw LlamaServerError before executeStep's wrapper, so only the user-facing category was stuck on the old taxonomy. --- src/agent/step-executor.ts | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) 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 }); From 778f2ee4e253d2d3d602f533e345e051ddac5022 Mon Sep 17 00:00:00 2001 From: Valera Brizhatiuk <19537764+plombeer31@users.noreply.github.com> Date: Thu, 3 Sep 2026 02:37:46 +0300 Subject: [PATCH 3/4] test(tui): drive the llama chat-error assertion through the real pipeline The block added in this PR claimed to mirror production but composed formatAgentErrorForChat(classifyFailure(err), err.message, local) by hand, omitting the toLlmFailure link that was exactly what was broken. It was green while a 404 still reached the user as a grammar failure. Rewritten to run the whole path: AgentLoop executes a step whose llmComplete throws a raw LlamaServerError, executeStep normalises it through toLlmFailure, the loop's catch classifies that wrapper and emits loop_failed { category, error }, and the assertion formats exactly those fields the way agent-event-reducer's loop_failed case does. With the toLlmFailure change reverted, the 404 and 405 cases fail with `Turn failed [grammar]: llama-server returned http 404`, reproducing the production defect. The 400 case is the regression guard for the half that is intentionally unchanged. --- src/tui/format-agent-error-for-chat.test.ts | 160 +++++++++++++++----- 1 file changed, 125 insertions(+), 35 deletions(-) diff --git a/src/tui/format-agent-error-for-chat.test.ts b/src/tui/format-agent-error-for-chat.test.ts index cd9a77a6..1a9908d0 100644 --- a/src/tui/format-agent-error-for-chat.test.ts +++ b/src/tui/format-agent-error-for-chat.test.ts @@ -1,7 +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 { classifyFailure } from "../llm/reliability/classify-failure.js"; +import type { + CapabilitiesSummary, + SkillCatalogEntry, + ToolDescriptor, +} from "../prompt/stable-prefix.js"; import { formatAgentErrorForChat } from "./format-agent-error-for-chat.js"; describe("formatAgentErrorForChat", () => { @@ -50,47 +61,126 @@ describe("formatAgentErrorForChat", () => { }); }); -describe("formatAgentErrorForChat — classified llama failures", () => { - // Mirrors the real pipeline: `agent-loop` classifies the thrown error - // and the reducer hands that category straight to the formatter. The - // hint is gated on `transport`, so the one failure where "check your - // llama URL" is exactly right — a 404 from a wrong `localModels.url` — - // used to be the one failure that never got it. - const local = { - activeProviderIsLocal: true, - llamaUrl: "http://127.0.0.1:19091", - }; - - it("carries the unreachable hint for a llama 404 on a local provider", () => { - const err = new LlamaServerError( - "llama-server returned http 404", - 404, - local.llamaUrl, +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, + }, ); - const text = formatAgentErrorForChat( - classifyFailure(err), - err.message, - local, + 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("keeps a llama 400 as a grammar failure with no URL advice", () => { - const err = new LlamaServerError( - "llama-server returned http 400", - 400, - local.llamaUrl, - ); - const text = formatAgentErrorForChat( - classifyFailure(err), - err.message, - local, - ); - expect(text).toBe( - "Turn failed [grammar]: llama-server returned http 400", + 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"); + }); }); From 1aa7141e0cb7f740da5913ac8a7b744801ec4233 Mon Sep 17 00:00:00 2001 From: Valera Brizhatiuk <19537764+plombeer31@users.noreply.github.com> Date: Thu, 3 Sep 2026 02:37:53 +0300 Subject: [PATCH 4/4] docs(fallback): say what AdvanceDecision.immediate actually governs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The AdvanceDecision doc has long said an immediate signal "should switch on the FIRST occurrence, bypassing the consecutive-failure threshold", and the test names this PR added inherited that wording ("advances via threshold on a local llama 404", "advances immediately on a local llama 429"). ProviderFallbackChain.advanceFrom returns the next link on the FIRST fallover-worthy failure whenever decision.advance is true, immediate or not. What immediate changes is registerFailure: it arms the breaker cooldown right away instead of waiting for failureThreshold consecutive failures. The threshold governs how long a failed link stays quarantined across later turns, not the in-turn switch. Assertions are unchanged — only the doc comment and the six new test names/comments that misstated the mechanism. --- src/llm/fallback/should-advance.test.ts | 27 ++++++++++++++++--------- src/llm/fallback/should-advance.ts | 10 +++++++-- 2 files changed, 25 insertions(+), 12 deletions(-) diff --git a/src/llm/fallback/should-advance.test.ts b/src/llm/fallback/should-advance.test.ts index 259f3d2e..78941d38 100644 --- a/src/llm/fallback/should-advance.test.ts +++ b/src/llm/fallback/should-advance.test.ts @@ -88,10 +88,14 @@ describe("shouldAdvance", () => { }); }); - it("advances via threshold on a local llama 404 — the URL serves no completions", () => { + 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). Not an immediate signal: 404 is - // not in the 429/408/5xx provider-down set, so it advances once the + // 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, @@ -99,24 +103,25 @@ describe("shouldAdvance", () => { }); }); - it("advances via threshold on a local llama 405", () => { + 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 immediately on a local llama 429", () => { + 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. + // 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 immediately on a local llama 408", () => { + 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, @@ -124,9 +129,11 @@ describe("shouldAdvance", () => { }); it("advances on a subscription-CLI binary that is not installed", () => { - // No HTTP status to read, so it advances via the threshold — but it - // must advance: a missing `claude` binary otherwise pins the chain to - // a provider that can never serve a turn. + // 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 }); diff --git a/src/llm/fallback/should-advance.ts b/src/llm/fallback/should-advance.ts index 75070d76..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;