Skip to content

fix(agents): classify upstream_error provider transport failures as fallbackable#96509

Closed
googlerest wants to merge 2 commits into
openclaw:mainfrom
googlerest:fix/fallback-trigger-on-upstream-error
Closed

fix(agents): classify upstream_error provider transport failures as fallbackable#96509
googlerest wants to merge 2 commits into
openclaw:mainfrom
googlerest:fix/fallback-trigger-on-upstream-error

Conversation

@googlerest

@googlerest googlerest commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Fixes #95519

What Problem This Solves

When the primary provider returns a transport-level upstream_error ({"error":{"message":"Upstream request failed","type":"upstream_error",...}}), OpenClaw ends the turn immediately with the generic LLM request failed error instead of trying any configured fallback models under agents.defaults.model.fallbacks. The trajectory shows only the primary provider/model — no model.fallback_step events, no fallback attempt — even though this is exactly the class of transient failure the fallback chain exists for.

Why This Change Was Made

Failover classification in src/agents/embedded-agent-helpers/errors.ts / failover-matches.ts matches known transient error shapes (server_error, overloaded, rate limits, timeouts, etc.) against the raw error text/JSON to decide whether to engage the configured fallback chain. Neither the structured JSON shape "type":"upstream_error" nor the bare message Upstream request failed matched any existing pattern — "upstream error" (with a space) and "upstream connect error" were already covered, but not the underscored upstream_error type or its associated message text, so this exact failure fell through to "unclassified" and ended the turn.

Fix:

  • isStructuredServerErrorMessage in errors.ts now also matches "type":"upstream_error" (alongside the existing "type":"server_error" / "code":"server_error" checks), since it is structurally the same kind of relay/transport failure.
  • ERROR_PATTERNS.serverError in failover-matches.ts now also matches the bare strings upstream_error and upstream request failed, covering callers that only see the flattened message text rather than the full JSON body.

Both lead to an already-existing, already-fallbackable FailoverReason (server_error / timeout) — no new reason code, no change to non-transient error handling.

User Impact

Configured fallback models now get a chance to handle a primary-provider upstream_error transport failure instead of the turn ending immediately. Non-transient/intentional errors (auth, billing, format) are unaffected — the new checks only add a previously-unmatched transient shape.

Evidence

Behavior addressed: neither the JSON-shaped "type":"upstream_error" provider error nor its plain message Upstream request failed were classified as a fallbackable FailoverReason, so agents.defaults.model.fallbacks was never engaged.

Real environment tested: macOS arm64 (M4), Darwin 25.5.0, Node v25.9.0, branch fix/fallback-trigger-on-upstream-error, commit 2cf755c2b7.

Exact steps or command run after this patch:

node scripts/run-vitest.mjs src/agents/embedded-agent-helpers/failover-matches.test.ts src/agents/embedded-agent-helpers/errors.test.ts
node scripts/run-oxlint.mjs src/agents/embedded-agent-helpers/errors.ts src/agents/embedded-agent-helpers/failover-matches.ts src/agents/embedded-agent-helpers/failover-matches.test.ts
pnpm tsgo:core
pnpm tsgo:core:test

Evidence after fix:

failover-matches.test.ts: 32 passed (32)
errors.test.ts: 5 passed (5)
oxlint: exit 0, no findings
tsgo:core: exit 0
tsgo:core:test: exit 0

Observed result after fix: added 3 regression tests reproducing the exact issue shape — the full {"error":{"type":"upstream_error",...}} JSON body now classifies as "server_error" via classifyFailoverReason, the bare message "Upstream request failed" classifies as "timeout", and isServerErrorMessage("upstream_error") returns true. Confirmed all three fail against the pre-fix code (return null/false) by reverting just the two source files while keeping the tests, then pass after restoring the fix.

What was not tested: did not reproduce against a live provider that actually emits upstream_error (no credentials/access to such a provider in this environment) — this is a classifier unit fix verified against the exact error shape quoted in the issue, not a live end-to-end fallback round-trip.

@openclaw-barnacle openclaw-barnacle Bot added agents Agent runtime and tooling size: XS labels Jun 24, 2026
@clawsweeper

clawsweeper Bot commented Jun 24, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed June 24, 2026, 1:25 PM ET / 17:25 UTC.

Summary
The PR adds upstream_error and Upstream request failed matching to shared failover classifiers and adds matcher-level regression tests.

PR surface: Source +10, Tests +19. Total +29 across 3 files.

Reproducibility: yes. at source level: current main can be traced from the embedded error payload classifier to a null result for server_error/timeout provider payloads, which prevents runWithModelFallback from advancing. I did not run a live provider outage reproduction in this read-only review.

Review metrics: none identified.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #95519
Summary: This PR is one candidate fix for the canonical upstream_error fallback issue, but the current patch does not cover the full embedded result classifier path.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🧂 unranked krab
Result: blocked until real behavior proof from a real setup is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • Patch result-fallback-classifier.ts so the upstream_error payload path advances configured fallbacks.
  • [P2] Add a regression in result-fallback-classifier or the nearest production fallback contract for the exact upstream_error payload.
  • [P2] Add redacted real behavior proof showing a configured fallback attempt or model.fallback_step after the patch.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR body provides focused test and typecheck output, but no real embedded or gateway run showing a configured fallback attempt or model.fallback_step after the change. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Risk before merge

  • [P2] Merging as-is can auto-close the linked bug while embedded error payloads classified as server_error or timeout still do not advance configured fallback models.
  • [P1] The PR body shows matcher and unit-test terminal output only, not a real embedded or gateway run with a model.fallback_step after the patch.

Maintainer options:

  1. Fix the production fallback gate (recommended)
    Patch result-fallback-classifier so upstream_error payloads classified as server_error or timeout become fallback classifications, then add the focused regression and real fallback proof before merge.
  2. Use the proof-positive sibling instead
    Maintainers can choose the open sibling PR with result-classifier coverage as the landing path and close or supersede this narrower branch after preserving any useful matcher cases.

Next step before merge

  • [P1] Manual review is needed because the PR has a concrete production-path defect and the contributor proof gate is mock-only, so ClawSweeper should not auto-repair or merge it.

Security
Cleared: The diff only changes TypeScript classifier patterns and tests; no dependency, workflow, secrets, permissions, or supply-chain surface changed.

Review findings

  • [P1] Patch the embedded result fallback gate — src/agents/embedded-agent-helpers/errors.ts:1666
Review details

Best possible solution:

Update the production embedded result fallback classifier to accept the transient provider error reasons this matcher now detects, preserve the existing visible-output and policy guards, add result-classifier coverage, and require real fallback proof.

Do we have a high-confidence way to reproduce the issue?

Yes at source level: current main can be traced from the embedded error payload classifier to a null result for server_error/timeout provider payloads, which prevents runWithModelFallback from advancing. I did not run a live provider outage reproduction in this read-only review.

Is this the best way to solve the issue?

No: matcher coverage is useful but not the best complete fix because the production embedded result fallback gate still needs to accept the transient reason and have coverage.

Full review comments:

  • [P1] Patch the embedded result fallback gate — src/agents/embedded-agent-helpers/errors.ts:1666
    This change makes the shared matcher classify the upstream_error payload as server_error or timeout, but classifyEmbeddedAgentRunResultForModelFallback still only permits auth/auth_permanent/billing/rate_limit from error payloads. In the linked embedded result path those new reasons still become null, so runWithModelFallback treats the primary result as success and never records a fallback step; please update that production classifier path and add a regression there.
    Confidence: 0.91

Overall correctness: patch is incorrect
Overall confidence: 0.91

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 47d3d1b1f1b2.

Label changes

Label changes:

  • add merge-risk: 🚨 message-delivery: If the production embedded payload path still skips configured fallbacks, users can receive a failed turn instead of a reply from a healthy fallback model.
  • add merge-risk: 🚨 auth-provider: The diff touches provider/model fallback classification, and merging the incomplete fix can leave provider fallback routing broken while closing the linked bug.

Label justifications:

  • P2: This is a normal-priority provider/model fallback bugfix PR with a concrete linked issue and limited blast radius.
  • merge-risk: 🚨 auth-provider: The diff touches provider/model fallback classification, and merging the incomplete fix can leave provider fallback routing broken while closing the linked bug.
  • merge-risk: 🚨 message-delivery: If the production embedded payload path still skips configured fallbacks, users can receive a failed turn instead of a reply from a healthy fallback model.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🧂 unranked krab.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body provides focused test and typecheck output, but no real embedded or gateway run showing a configured fallback attempt or model.fallback_step after the change. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed

PR surface:

Source +10, Tests +19. Total +29 across 3 files.

View PR surface stats
Area Files Added Removed Net
Source 2 11 1 +10
Tests 1 19 0 +19
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 3 30 1 +29

What I checked:

Likely related people:

  • jason-allen-oneal: Recent GitHub history shows work on result-fallback-classifier.ts and adjacent raw provider error handling, including Codex usage-limit fallback classification and raw provider error suppression. (role: recent area contributor; confidence: high; commits: b8f1961aaeba, 01f6ad605615; files: src/agents/embedded-agent-runner/result-fallback-classifier.ts, src/agents/embedded-agent-helpers/errors.ts)
  • steipete: History shows repeated model fallback, embedded runner, and model-failover documentation work around this boundary. (role: feature owner / recent area contributor; confidence: high; commits: 0314819f918a, 4b0f16d496e5, bb46b79d3c14; files: src/agents/embedded-agent-runner/result-fallback-classifier.ts, src/agents/model-fallback.ts, docs/concepts/model-failover.md)
  • takhoffman: The earlier embedded provider business-denial fallback classifier commit records takhoffman as approved/co-authored, making them relevant to the current filter boundary. (role: adjacent reviewer; confidence: medium; commits: 18f94fc83a72; files: src/agents/embedded-agent-runner/result-fallback-classifier.ts)
  • vincentkoc: GitHub history shows recent touches and co-authored work in the shared agent error helpers that this PR modifies. (role: recent error-helper contributor; confidence: medium; commits: 2f9107f672df, 7c97c6da9bfa; files: src/agents/embedded-agent-helpers/errors.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal backlog priority with limited blast radius. labels Jun 24, 2026
@clawsweeper clawsweeper Bot added merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. merge-risk: 🚨 auth-provider 🚨 May break OAuth, tokens, provider routing, model choice, or credentials. labels Jun 24, 2026
@googlerest

googlerest commented Jun 24, 2026

Copy link
Copy Markdown
Contributor Author

Closing this in favor of #95542.

My patch only covers the matcher layer (isStructuredServerErrorMessage in errors.ts and ERROR_PATTERNS.serverError in failover-matches.ts). The actual fallback gate for embedded runs is classifyBusinessDenialErrorPayloadReason in result-fallback-classifier.ts (~line 150), which only lets auth | auth_permanent | billing | rate_limit through. Even after the matcher correctly classifies upstream_error as server_error, that reason gets dropped at this switch, and runFallbackAttempt treats the resulting null classification as a successful result — so fallback still never advances for this payload shape.

#95542 covers both layers (structured errorType classification + the embedded result classifier allowlist) and already has sufficient real behavior proof, including an independent live repro. No need to duplicate effort here.

@googlerest googlerest closed this Jun 24, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jul 5, 2026
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

agents Agent runtime and tooling merge-risk: 🚨 auth-provider 🚨 May break OAuth, tokens, provider routing, model choice, or credentials. merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. P2 Normal backlog priority with limited blast radius. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: XS status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug]: Fallback should trigger on provider upstream_error / LLM request failed

1 participant

@googlerest