Skip to content

fix: use manifest model.fallback at runtime for provider resilience#70

Open
stealthwhizz wants to merge 2 commits into
open-gitagent:mainfrom
stealthwhizz:fix/knowledge-file-context
Open

fix: use manifest model.fallback at runtime for provider resilience#70
stealthwhizz wants to merge 2 commits into
open-gitagent:mainfrom
stealthwhizz:fix/knowledge-file-context

Conversation

@stealthwhizz

Copy link
Copy Markdown
Collaborator

Problem

The agent manifest supports a model.fallback list, and the scaffolding templates (index.ts, session.ts) tell users to fill it in — but nothing ever read it at runtime. loadAgent resolved 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 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.fallback was dead config.

Fix

Resolve fallbacks (loader.ts)

  • Extract resolveModel() (custom endpoints, base-url override, key normalization) so the preferred model and every fallback entry resolve identically.
  • Resolve manifest.model.fallback into an ordered fallbackModels list on LoadedAgent. Duplicate and unparseable entries are skipped rather than failing startup.

Retry at runtime (sdk.ts)

  • Candidate list = [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.
  • Each switch emits a fallback system message; only when all candidates are exhausted is the error surfaced. The errored assistant message is still emitted on terminal failure (preserves the existing messages() contract).
  • Captures both failure paths: in-stream error (message_end with stopReason: "error") and thrown errors routed through pi-agent-core's handleRunFailure (agent_end only).

Classify errors (model-fallback.ts)

  • isRetryableProviderError() treats billing / credit / quota / rate-limit / auth / availability errors as retryable; request-shaped errors (bad max_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

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
Copilot AI review requested due to automatic review settings July 6, 2026 13:22

@shreyas-lyzr shreyas-lyzr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 loadAgent to resolve manifest.model.fallback into LoadedAgent.fallbackModels.
  • Update the SDK query runner to attempt [preferred, ...fallbacks] and emit a system:fallback message 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.

Comment thread src/sdk.ts Outdated
Comment on lines +388 to +391
// Reset accumulators and skip cost tracking (usage is empty on error).
accText = "";
accThinking = "";
break;
Comment thread src/sdk.ts
Comment on lines +652 to 653
await runPrompt(userMsg.content);
}
Comment thread src/loader.ts
Comment on lines +441 to +445
const fallbackModels: Model<any>[] = [];
for (const fb of manifest.model.fallback ?? []) {
if (!fb || fb === modelStr) continue;
try {
fallbackModels.push(resolveModel(fb));
Comment on lines +78 to +79
});

Comment thread test/model-fallback.test.ts Outdated
@@ -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()
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Agent ignores configured files and returns credit balance error on queries

3 participants