fix(providers): honor resolved terminal fetch errors - #3701
Conversation
Apply the existing fallback error resolver to terminal failures as well as exhausted fallback chains. Preserve cancellation, original routing errors, and per-attempt diagnostics. Extracted from #3683. Co-authored-by: hhh2210 <hzy2210@gmail.com>
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
|
Codex review: needs maintainer review before merge. Reviewed September 16, 2026, 10:15 PM ET / September 17, 2026, 02:15 UTC. ClawSweeper reviewWhat this changesThe PR makes terminal provider failures use the existing error-selection policy, with regression tests and Antigravity documentation. Merge readiness✅ Ready for maintainer review This is a useful, bounded repair that remains absent from current main and v0.60.4. No actionable correctness or security defects were found; the broader related proposal does not supersede it. Priority: P2 Review scores
Verification
How this fits togetherCodexBar tries provider usage sources in sequence and sends the resulting usage or error to its menu-bar app and CLI. The shared fetch pipeline selects the surfaced error while retaining each source’s original diagnostic. flowchart TD
A[Usage refresh] --> B[Ordered provider sources]
B --> C[Fetch pipeline]
C --> D{Fetch result}
D -->|Success| E[Usage display]
D -->|Failure| F[Provider error policy]
F --> G[App and CLI error]
C --> H[Per-source diagnostics]
Before mergeNone. Agent review detailsSecurityNone. Review metrics
Technical reviewBest possible solution: Use one consistent provider error-selection policy for exhausted and terminal failures while preserving cancellation, successful fallbacks, and original diagnostics. Do we have a high-confidence way to reproduce the issue? Yes, source establishes the defect: an earlier substantive failure followed by terminal offline notRunning loses the resolved error on current main. No runtime reproduction was executed during this read-only review. Is this the best way to solve the issue? Yes, returning the already-resolved error is the narrowest repair; it preserves the existing precedence contract without adding another policy or changing routing. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 094b18a2c5d7. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
|
|
The terminal-error repair is now on main through #3701, merged as e56bb5b. An unavailable final source now preserves the error selected by the existing provider policy, while cancellation and later substantive failures keep their existing precedence. Each source still records its original diagnostic. Validation: Thanks @hhh2210. This extracts only the terminal resolver repair; the broader first-error/per-source diagnostic proposal in #3683 remains open. |
An unavailable final Antigravity source could replace a useful earlier API, transport, or sign-in error when it stopped fallback. The pipeline had already selected the right error through its provider resolver, but its terminal return discarded that result.
Use the resolved error for terminal failures, matching exhausted fallback chains. Cancellation remains authoritative; routing and per-attempt diagnostics still receive each original error, and later substantive failures retain their existing precedence. This extracts the bounded repair from #3683; that broader proposal remains open.
Validation: reproduced 13 failing assertions before the fix; 110 tests in ProviderFetchErrorTests, AntigravityCLIHTTPSFetchStrategyTests, and supporting provider coverage now pass.
make checkreports zero violations. Independent P2 review is clean. Tests use synthetic strategies and injected transports; no live accounts, cookie imports, or Keychain reads were used.Thanks @hhh2210 for the original proposal.