fix: use manifest model.fallback at runtime for provider resilience#70
fix: use manifest model.fallback at runtime for provider resilience#70stealthwhizz wants to merge 2 commits into
Conversation
The agent manifest lets you declare model.fallback, and the scaffolding tells users to fill it in, but nothing ever read it at runtime. loadAgent picked only model.preferred and built a single-model agent. So when the primary provider failed — e.g. Anthropic returning "credit balance too low" — the run died even when a working provider (and its key) was also configured. From the outside the agent looked like it "ignored the configured files" and errored out (issue open-gitagent#24). - loader: extract resolveModel() and resolve manifest.model.fallback into an ordered fallbackModels list on LoadedAgent (bad/duplicate entries skipped). - sdk: run the prompt against the preferred model, and on a retryable provider error that occurred before any output was streamed, swap the model on the same agent (preserving conversation history), roll back the failed turn, and retry the next candidate. A 'fallback' system message reports each switch; only when all candidates are exhausted is the error surfaced. - model-fallback: isRetryableProviderError() classifies billing/quota/rate-limit/ auth/availability errors as retryable; request-shaped errors are not. - Captures both error paths (in-stream error message and thrown/handleRunFailure). Tests cover the retry classifier and manifest fallback resolution; full suite 33 passing. Closes open-gitagent#24
shreyas-lyzr
left a comment
There was a problem hiding this comment.
Clean, well-scoped fix for a genuine gap — the fallback list was always dead config, and this wires it up correctly.
A few things I looked at closely:
resolveModel() extraction (loader.ts): The refactor is faithful — the new function is identical logic to what was inline before, just reusable. The side-effecting mutation of process.env.OPENAI_API_KEY and model.provider is a pre-existing pattern carried forward, not introduced here, so no regression there.
Retry classifier (model-fallback.ts): The RETRYABLE_PATTERNS list is conservative and correct. The choice to retry on empty/null errors (give the next model a chance on opaque failures) is reasonable and documented. One minor note: /\b500\b/ and /\b502\b/ etc. correctly use word boundaries so they won't spuriously match numbers embedded in longer strings (e.g. "1500 tokens") — verified.
Transcript rollback (sdk.ts, line 560–573): The shallow array copy ([...agent.state.messages]) is appropriate here because message arrays are append-only in pi-agent-core — new turns are pushed, not mutated in-place. A rollback to the snapshot correctly drops the failed user+assistant turn before the retry.
session_start deduplication: The sessionStartEmitted guard correctly prevents duplicate session-start events across fallback retries. Good.
finalizeRun() guard: The runFinalized flag prevents double-emission of session_end when the outer channel.finish() call at the bottom of the async block also runs. Correct.
Tests: The classifier tests cover billing, availability, auth, empty, and request-shaped errors. The manifest resolution tests cover dedup and empty-string skipping. Good coverage of the new logic paths.
Overall the implementation does exactly what it says, is well-tested, and doesn't break existing callers. Approved.
There was a problem hiding this comment.
Pull request overview
This PR implements runtime support for manifest.model.fallback so an agent can transparently retry a prompt on alternate models/providers when the preferred provider fails with a retryable error (e.g., credit/quota/rate-limit), improving resilience and addressing issue #24.
Changes:
- Add retry classification logic (
isRetryableProviderError) to decide when fallback is appropriate. - Extend
loadAgentto resolvemanifest.model.fallbackintoLoadedAgent.fallbackModels. - Update the SDK query runner to attempt
[preferred, ...fallbacks]and emit asystem:fallbackmessage when switching models.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| test/model-fallback.test.ts | Adds tests for retry classification and manifest fallback resolution. |
| src/sdk.ts | Implements fallback-aware prompt execution and streams a fallback system event when switching models. |
| src/sdk-types.ts | Extends GCSystemMessage.subtype to include fallback. |
| src/model-fallback.ts | Introduces retryable-provider-error classifier used by the SDK fallback runner. |
| src/loader.ts | Extracts model resolution logic and resolves manifest fallback models into LoadedAgent. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Reset accumulators and skip cost tracking (usage is empty on error). | ||
| accText = ""; | ||
| accThinking = ""; | ||
| break; |
| await runPrompt(userMsg.content); | ||
| } |
| const fallbackModels: Model<any>[] = []; | ||
| for (const fb of manifest.model.fallback ?? []) { | ||
| if (!fb || fb === modelStr) continue; | ||
| try { | ||
| fallbackModels.push(resolveModel(fb)); |
| }); | ||
|
|
| @@ -0,0 +1,108 @@ | |||
| import { describe, it, before } from "node:test"; | |||
…tries - sdk: reset _llmCallStart in the errored message_end path so a partial stream before the error can't skew the next successful turn's duration metric - loader: trim fallback entries and de-duplicate them (including whitespace variants and matches against the preferred model) before resolving - test: cover the whitespace-dup case and clean up the temp agent dir in after()
Problem
The agent manifest supports a
model.fallbacklist, and the scaffolding templates (index.ts,session.ts) tell users to fill it in — but nothing ever read it at runtime.loadAgentresolved onlymodel.preferredand built a single-model agent.So when the primary provider failed — e.g. Anthropic returning
credit balance too low— the run died even though the user had configured a second provider and key expecting resilience. From the outside the agent looked like it "ignored the configured files" and returned a credit error (issue #24: generic chat works, file/knowledge queries die on the out-of-credits provider).Root cause
grep -rn "model.fallback\|\.fallback" src/→ only the type declaration and scaffolding strings. No runtime consumer.manifest.model.fallbackwas dead config.Fix
Resolve fallbacks (
loader.ts)resolveModel()(custom endpoints, base-url override, key normalization) so the preferred model and every fallback entry resolve identically.manifest.model.fallbackinto an orderedfallbackModelslist onLoadedAgent. Duplicate and unparseable entries are skipped rather than failing startup.Retry at runtime (
sdk.ts)[preferred, ...fallbacks]. Run the prompt on the current model; if it fails with a retryable provider error before any output was streamed, swap the model on the same agent (preserving conversation history), roll back the failed turn, and retry the next candidate.fallbacksystem message; only when all candidates are exhausted is the error surfaced. The errored assistant message is still emitted on terminal failure (preserves the existingmessages()contract).message_endwithstopReason: "error") and thrown errors routed through pi-agent-core'shandleRunFailure(agent_endonly).Classify errors (
model-fallback.ts)isRetryableProviderError()treats billing / credit / quota / rate-limit / auth / availability errors as retryable; request-shaped errors (badmax_tokens, content filter) are not — switching models won't fix those.Result for the #24 reporter
With OpenAI + Anthropic both configured and listed in
fallback, a "leave policy" query where Anthropic is out of credits now transparently retries on OpenAI and answers from the files — no error. If all providers fail, they get one clear message naming what failed, instead of an agent that silently "ignores files."Tests
test/model-fallback.test.ts— retry classifier (billing/availability/auth → retry; empty → retry; request-shaped → no retry) and manifest fallback resolution (dedupe + skip blanks; empty list when unconfigured). Full suite: 33 passing.Closes #24