fix(opencode): classify provider API rejections as a terminal cause and stop overwriting their message - #1468
Merged
Conversation
Contributor
|
Warning Review limit reached
More reviews will be available in 47 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses rolling per-developer review limits. Reviews become available again as older review attempts age out of the rolling limit window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (13)
📝 WalkthroughWalkthroughIntroduces a ChangesProvider API Error Classification and Retry Routing
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
harness
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/opencode/src/session/processor.ts (1)
1732-1760: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winSingle-parse design with structured evidence forwarding.
The rework correctly parses the error once and extracts
providerFailurewith all necessary fields (kind,code,statusCode,hasResponseBody) for downstream routing. ThehasResponseBodycheck is appropriately defensive (empty string or non-string types yieldfalse).♻️ Optional: Extract inline type for clarity
The inline type annotation at lines 1734-1736 could be extracted to a named type if this return shape is referenced elsewhere or grows more complex:
type RetrySignalProviderFailure = { kind: ProviderFailureKind code?: string statusCode?: number hasResponseBody?: boolean }However, since this is a local return type and not referenced elsewhere, the current inline form is acceptable.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/opencode/src/session/processor.ts` around lines 1732 - 1760, The current implementation with the inline type annotation for the return shape (containing kind, code, statusCode, and hasResponseBody fields) is acceptable and correctly handles the providerFailure extraction with defensive checks. The refactoring to extract this inline type definition into a named type like RetrySignalProviderFailure is purely optional for improved code clarity and maintainability, and should only be pursued if this return type shape is referenced elsewhere in the codebase or if the team prefers explicit named types for consistency. No mandatory changes are required.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/opencode/src/session/processor.ts`:
- Around line 1732-1760: The current implementation with the inline type
annotation for the return shape (containing kind, code, statusCode, and
hasResponseBody fields) is acceptable and correctly handles the providerFailure
extraction with defensive checks. The refactoring to extract this inline type
definition into a named type like RetrySignalProviderFailure is purely optional
for improved code clarity and maintainability, and should only be pursued if
this return type shape is referenced elsewhere in the codebase or if the team
prefers explicit named types for consistency. No mandatory changes are required.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 012387f9-11d0-4371-ae8f-21eb52dc6c8c
📒 Files selected for processing (12)
packages/opencode/src/provider/error.tspackages/opencode/src/session/processor.tspackages/opencode/src/session/run-incident/derive.tspackages/opencode/src/session/run-incident/index.tspackages/opencode/src/session/run-incident/policy.tspackages/opencode/src/session/run-incident/presentation.tspackages/opencode/src/session/run-incident/types.tspackages/opencode/src/session/run-observability/recorder.tspackages/opencode/src/session/run-observability/types.tspackages/opencode/test/session/export.test.tspackages/opencode/test/session/processor-effect.test.tspackages/opencode/test/session/run-observability.test.ts
13 tasks
Astro-Han
added a commit
that referenced
this pull request
Jun 23, 2026
…sking them (#1466) Fixes the classification layer behind the "Connection lost" misreport for provider billing/quota/auth failures (e.g. DeepSeek 402 "Insufficient Balance"), which were classified as `unknown` with the nested provider reason dropped. Change boundary (`packages/opencode/src/provider/error.ts` + tests): - 402 -> `quota_exhausted`; strong/weak billing patterns (weak ones status-gated to 400/402/403 with no rate-limit signal; 429 and FreeUsageLimitError excluded). - `message()` now parses the structured body before the reason-phrase early-return, so a custom SDK message ("API call failed") no longer suppresses the nested `{error:{message}}` reason. - `parseStreamError` classifies auth/rate-limit/billing codes; a typed `{type:"error"}` envelope with an unhandled code upgrades to `APIError(kind="unknown")` so the frontend can read `code`/`responseBody` (a bare `{code}` body stays `UnknownError`). - `looksTransientCode` is a deny-by-default exact allowlist (`resource_exhausted`, `unavailable`, `overloaded`, ...) instead of a substring scan, so terminal codes like `model_unavailable_for_account` are no longer retried in a loop. Review follow-ups (GPT-Pro + CodeRabbit): - P2 `message()` early-return fixed; 402 + non-reason-phrase regression test added. - P2 billing over-match: guards pinned (rate-limit signal suppresses weak billing on a billing-shaped status; weak billing never overrides a 5xx). - P3 transient substring -> exact allowlist; guards for `*_unavailable_for_*` (non-retryable) and gRPC `UNAVAILABLE` (retryable). - P3 scope (split to a single narrow fix): kept as one coherent change — both entry points are the same defect and the typed-unknown fallback feeds the PR3 frontend decoder; the rationale is in the PR's Scope section. Verification: provider+session suite 1337 pass / 0 fail; `tsgo --noEmit` clean; `lint` clean; full CI green (43 checks). Refs #1123, #1105. Part of the classify->passthrough->render series (#1467 transport errno, #1468 run-incident terminal cause, #1469 frontend decode). Residual: the per-kind error-card UX (PR4) is design-gated and deferred — see docs/handoff/2026-06-23-error-render-pr4-handoff.md.
Astro-Han
added a commit
that referenced
this pull request
Jun 23, 2026
…manent DNS failures (#1467) Transport-disconnect errors were collapsed into one retryable bucket, so a permanent DNS failure (ENOTFOUND) was retried like a transient blip, and the real errno never informed retry safety. Change boundary (opencode session classification): - Broaden transport errno coverage: EAI_AGAIN, EPIPE, ECONNABORTED, EHOSTUNREACH, ENETUNREACH, ENOTFOUND, UND_ERR_CONNECT / HEADERS / BODY_TIMEOUT. - Per-errno retryability: `TransportDisconnect.retryable` becomes a boolean — ENOTFOUND (permanent name resolution failure) is non-retryable; EAI_AGAIN and other transient transport errnos still retry. - `fromError` passes `retryable` through; `classifyRetry` gives `transport_disconnect` its own branch that reads `isRetryable` (removed from RETRY_TRANSIENT_KINDS). `retrySignal.retryable` also feeds run-observability retry_safety, so ENOTFOUND routes to #1118's provider_terminal_failure. - `statusCode` short-circuit on top: an error carrying an HTTP status is judged as an API error by status and is no longer mis-grabbed by a transport-coded cause or a message match (e.g. "socket hang up"). Review follow-ups (codex, 2 rounds, final clean): message-fallback mis-grabbing HTTP "socket hang up" and a transport-coded cause mis-grabbing HTTP — both fixed by the statusCode short-circuit. Verification: opencode suite 1328 pass / 0 fail; `tsgo --noEmit` clean; full CI green (43 checks). Labels bug + harness + P2. Refs #1123, #1105, #1118. Part of the classify->passthrough->render series (PR1 #1466 merged; #1468 run-incident terminal cause, #1469 frontend decode still open). Residual: none specific to this PR.
…nd stop overwriting their message A DeepSeek direct account in arrears returns 402 "Insufficient Balance", but it surfaced as "Connection lost. Please check whether the last operation completed before resending." Two of the three root-cause layers live here (the third, 402 classification, is PR #1466): 1. run-incident had only watchdog/transport terminal causes, so every provider API rejection was recorded as provider_transport_disconnect. 2. halt() overwrote the real provider message with a generic connection-lost recovery string. Changes: - New provider_api_error TerminalCause (run-incident) and Classification (run-observability), subcategory from providerFailure.kind. Bumps both RUN_INCIDENT_SCHEMA_VERSION and RunObservability SCHEMA_VERSION to 2; export.test.ts schema/version assertions updated. - The processor parses the failure once and passes providerFailure (with HTTP evidence) to the recorder, which routes a real provider API rejection to provider_api_error instead of defaulting it to a transport disconnect. classificationForIncident and retrySafetyFor gain provider_api_error branches (retryable=false -> provider_terminal_failure, aligned with #1118). - recoveryFor: a terminal provider API error stops with reason provider_api_error (out of the connection-lost set); a retryable one (rate_limit / server_overload) flows through the existing auto-retry tree. - The terminal halt no longer overwrites a provider API rejection's real message with the connection-lost recovery string. Lifecycle-close and user-cancel halts keep their authoritative interruption messages. - isProviderApiError gates the catch-all "unknown" kind on HTTP evidence (status code or response body) so a wrapped connection failure is not mislabeled a provider API error. Combined with PR #1466 (402 -> quota_exhausted), a billing failure now surfaces with its real provider message instead of "Connection lost". Refs #1105, #1123. Claude-Session: https://claude.ai/code/session_015bW9JQSkuB156gkNQdxCzi
Astro-Han
force-pushed
the
claude/i1105-pr2-run-incident-provider-api
branch
from
June 23, 2026 10:00
a2daaa2 to
2f7867d
Compare
…rrors after a side effect
P1 (review): the halt suppressed its recovery interruption message for ANY
provider-API kind, but only a *terminal* rejection should pass its own message
through. A retryable rate_limit / server_overload that exhausted its retries
after a tool ran or an unsafe side effect started would lose its safety hint
("check whether the last operation completed before resending"), risking a
repeated side-effecting operation. Gate suppression on reason
"provider_api_error" (the terminal reason — retryable provider errors never get
it; see run-incident/policy.ts) and extract the choice into a documented,
unit-tested haltInterruptionMessage helper.
P3 (review): collapse ProviderApiErrorKind's parallel type/runtime lists into a
single literal tuple source of truth — the type is derived from it and the
runtime check reads it directly — so a new ProviderFailureKind can't drift
between the two.
Refs #1105, #1118.
Claude-Session: https://claude.ai/code/session_012743rGkjEzqaUMKy2nvYMM
… errors too P1 (review round 2): the previous fix only covered retryable provider errors. A *terminal* provider rejection (e.g. a 402 "Insufficient Balance", retryable=false) that lands after a tool ran still short-circuited to reason "provider_api_error" before the side-effect gate, so haltInterruptionMessage passed the provider text through verbatim and dropped the "check external state" warning — a user who fixed their balance and resent could silently re-run a side-effecting operation. policy.recoveryFor: a terminal provider rejection now only keeps the pure "provider_api_error" passthrough reason when there is no side-effect risk; once a tool ran, an unsafe side effect started, or side-effect facts are incomplete it surfaces that side-effect reason instead (still recommendation do_not_retry, so no retry/observability behavior changes — only the reason, which solely drives message selection). haltInterruptionMessage then combines the provider's real message with the bare safety hint (no "Connection lost." framing, which would mislabel the rejection). This also makes the retryable case surface both. Tests: recoveryFor terminal-provider matrix (no-risk→provider_api_error; tool/unsafe/incomplete→safety reason, do_not_retry) and haltInterruptionMessage combine assertions (provider text + hint, no "Connection lost"). Both verified to fail under the pre-fix logic. Refs #1105, #1118. Claude-Session: https://claude.ai/code/session_012743rGkjEzqaUMKy2nvYMM
…side effect
Adds the precise check the review asked for: record a tool execution, then drive
recordAttemptFailureAndDeriveRecovery with a terminal quota_exhausted
(retryable=false), and assert the derived recovery carries the side-effect reason
(tool_execution_started, do_not_retry) and the final halt message keeps BOTH the
provider reason ("Insufficient Balance") and the "check whether the last
operation completed" safety hint (and not the "Connection lost" framing).
Exercises the recorder→derive→message plumbing the prior unit tests covered only
in isolation; verified to fail under the pre-fix policy (reason stayed
"provider_api_error"). The fix itself landed in 4ec4b1c.
Refs #1105, #1118.
Claude-Session: https://claude.ai/code/session_012743rGkjEzqaUMKy2nvYMM
… drop second parse The halt path re-parsed result.error only to read data.message for the provider-rejection passthrough, duplicating the parse boundary that retrySignalFor() already crosses and risking drift from its classification. Surface providerMessage alongside providerFailure from the single parse in retrySignalFor() and pass retrySignal.providerMessage to haltInterruptionMessage(), removing parsedForMessage and its isRecord sniff. Behavior-preserving: terminal-provider-after-tool, pure-402 passthrough, and retryable safety-hint tests all unchanged. Claude-Session: https://claude.ai/code/session_012743rGkjEzqaUMKy2nvYMM
13 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a
provider_api_errorterminal cause so a provider's API-level rejection is classified as what it is, and stopshalt()from overwriting that rejection's real message with a generic connection-lost string. This is the run-incident + render half of the error-classification work; the classification half (402 →quota_exhausted) is PR #1466.provider_api_erroris added torun-incident'sTerminalCauseand torun-observability'sClassification, withsubcategorytaken fromproviderFailure.kind(auth/rate_limit/quota_exhausted/server_overload/invalid_request/unknown). BothRUN_INCIDENT_SCHEMA_VERSIONandRunObservabilitySCHEMA_VERSIONbump to2; the export pipeline preserves each existing package's own version (export.ts), so old bundles still read back as1.providerFailure(with HTTP evidence) intorecordAttemptFailureAndDeriveRecovery. The recorder routes a real provider API rejection toprovider_api_errorinstead of defaulting it toprovider_transport_disconnect. The recorder does not parse a third time.classificationForIncidentandretrySafetyForgainprovider_api_errorbranches:retryable === false→do_not_auto_retry/provider_terminal_failure, aligned with the fix(opencode): stop recommending auto-retry for terminal provider failures #1118retry_safetyaxis; a retryable one stayscandidate_safe_auto_retry.recoveryForstops a terminal provider API error with reasonprovider_api_error(kept out of the connection-lost reason set); a retryable provider API error (rate_limit/server_overload) flows through the existing auto-retry tree viaretryableProviderFailure. The terminalhalt()no longer overwrites a provider API rejection's real message with the connection-lost recovery string — while lifecycle-close and user-cancel halts keep their authoritative interruption messages.isProviderApiErrorgates the catch-allunknownkind on HTTP evidence (a status code or a response body). A wrapped connection failure — e.g. anAPICallErrorwith no HTTP response, or a DNS failure the stream classifier didn't recognize — stays on the transport path rather than being mislabeled a provider API error. Explicit kinds are self-evidencing.Why
A Windows user's DeepSeek-direct account in arrears returned
402 {"error":{"message":"Insufficient Balance",...}}, but it surfaced as "Connection lost. Please check whether the last operation completed before resending." Three layers caused this: (1) 402 classified asunknown; (2) run-incident had no provider-API category, so it defaulted toprovider_transport_disconnect; (3)halt()overwrote the real message with a recovery string. Layers 2 and 3 are fixed here; layer 1 is PR #1466. Combined, a billing failure now surfaces with its real provider message instead of "Connection lost".Related Issue
Refs #1105 (classification spine), #1123 (classify-then-render umbrella).
Human Review Status
Pending
Review Focus
isProviderApiError): explicit kinds always route;unknownonly routes with HTTP evidence so a wrapped connection failure isn't mislabeled.transport_disconnectanddecompressionstay on the transport path.recoveryForsplit: terminal provider errors stop; retryable ones (rate_limit/server_overload) must still auto-retry throughretryableProviderFailure— confirm the existing retry tests still pass.2; the decode path preserves each bundle's own version, and the only freshly-stamped assertions updated are inexport.test.ts.Risk Notes
Data-contract change (intentional, tested): two schema versions bump to
2and two enums gain aprovider_api_errorvalue. Additive and backward-compatible — old bundles carry and keep their ownschema_version: 1; the enums are not pinned byz.literal, so widening them does not reject old data. No SDK regeneration: these diagnostics enums are not exposed insdk/js. No UI in this PR (per-kind cards and i18n are a follow-up). Behavior change: a terminal provider API error no longer auto-retries and shows its real message; a retryable provider API error keeps retrying exactly as before.How To Verify
Pinned: 402 (kind
unknown, status 402) →provider_api_error, real message preserved;quota_exhaustedretryable=false →do_not_auto_retry/provider_terminal_failure;rate_limitretryable=true still auto-retries;transport_disconnect/decompression/ unknown-without-evidence → transport path; lifecycle-close halt keeps its message.Screenshots or Recordings
N/A — backend classification + message-passthrough change, no UI.
Checklist
bug,enhancement,task,documentation. Type labels are author-added; the labeler bot does NOT assign them. Add the label in the GitHub UI, then tick this.app,ui,platform,harness,ci. The labeler bot assigns these on PR open based on changed paths. Confirm the bot's choice (or override if wrong), then tick this.P0,P1,P2,P3. The priority-triage bot suggests one on PR open. Confirm or override, then tick this.Pending,Approved by @<reviewer>, orNot required: <reason>(default isPending; "not required" is restricted to bot-authored low-risk PRs).dev, and my PR title and commit messages use Conventional Commits in English.https://claude.ai/code/session_015bW9JQSkuB156gkNQdxCzi
Summary by CodeRabbit
New Features
Improvements