fix(opencode): stop recommending auto-retry for terminal provider failures - #1118
Merged
Conversation
…lures
Terminal provider failures (auth, invalid_request, quota_exhausted) are
recorded through the transport-failure path and classified as
external_stream_disconnect, whose retry_safety branch unconditionally
returned candidate_safe_auto_retry. That contradicted the recovery
decision, which already marks these technical_retryable=false, so the two
diagnostics signals disagreed for the same failure.
Thread the recorded failure's retryable flag into retrySafetyFor and, when
a transport failure is non-retryable, return do_not_auto_retry with the new
provider_terminal_failure reason. The run-phase classification label
(external_stream_disconnect) is left unchanged; only the retry-safety
recommendation is corrected. retryable=true and the undefined fallback keep
their prior behavior, so genuine stream disconnects still surface as
candidate_safe_auto_retry.
The production wiring already passes retryable through
recordAttemptFailureAndDeriveRecovery (retrySignalFor returns
{retryable:false} for terminal classifications), so this closes the
inconsistency end to end.
Contributor
|
Warning Review limit reached
More reviews will be available in 35 minutes and 11 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. 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 (3)
✨ 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}
Astro-Han
added
tech-debt
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for handling non-retryable transport failures during external stream disconnects. It updates the observability recorder to propagate the retryable status of a failure, recommending do_not_auto_retry when a terminal provider failure occurs. Corresponding types and unit tests have been added to verify this behavior. No review comments were provided, and the implementation is clean and well-tested.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
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.
Astro-Han
added a commit
that referenced
this pull request
Jun 23, 2026
…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
added a commit
that referenced
this pull request
Jun 23, 2026
…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
Astro-Han
added a commit
that referenced
this pull request
Jun 23, 2026
… 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
Astro-Han
added a commit
that referenced
this pull request
Jun 23, 2026
…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
Astro-Han
added a commit
that referenced
this pull request
Jun 23, 2026
…nd stop overwriting their message (#1468) * fix(opencode): classify provider API rejections as a terminal cause and 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 * fix(opencode): keep the recovery safety hint for retryable provider errors 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 * fix(opencode): keep the side-effect safety hint for terminal provider 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 * test(opencode): end-to-end guard for terminal provider error after a 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 * refactor(opencode): source provider halt message from retrySignalFor, 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
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.
Why
Part of the #1105 provider-failure classification spine. Earlier slices made
classifyRetryreadproviderFailure.kind, so terminal provider failures (auth / invalid_request / quota_exhausted) are no longer retried. But the run-observability diagnostics layer never got that memo: those failures are recorded through the transport-failure path and classified asexternal_stream_disconnect, whoseretry_safetybranch unconditionally returnedcandidate_safe_auto_retry.That left two diagnostics signals contradicting each other for the same failure: the recovery decision already records
technical_retryable=false, whileretry_safetyrecommended a safe auto-retry.What
retryableflag intoretrySafetyFor.external_stream_disconnectbranch, when the failure is non-retryable (retryable === false), returndo_not_auto_retrywith a newprovider_terminal_failurereason — checked first, before the existingcandidate_safe_auto_retryreturns.external_stream_disconnect) is intentionally left unchanged. Only the retry-safety recommendation is corrected; this is a diagnostics-layer fix, not a re-taxonomy.retryable === trueand theundefinedfallback keep their prior behavior, so genuine stream disconnects still surface ascandidate_safe_auto_retry.End to end
The production wiring already passes
retryablethroughrecordAttemptFailureAndDeriveRecovery:retrySignalForreturns{retryable: false}for terminal classifications (whenclassifyRetryreturnsundefinedorretryAction === "stop"). So the guard fires for real terminal provider failures, closing the inconsistency end to end.halt()'s fallbackrecordTransportFailuredoes not setretryable, but it only writes viafailure ??=after the attempt loop has already recorded the failure, so the flag is preserved.Tests
retryable: false) onexternal_stream_disconnect→do_not_auto_retry/provider_terminal_failure, classification unchanged.retryable: true) still →candidate_safe_auto_retry/no_visible_output_or_tool_execution.run-observability.test.tssuite green (70 pass),tsgo --noEmitclean.Stays within
packages/opencode. No schema field is removed;RetrySafety.reasongains one optional union member. Sessions persist as JSON and this only widens an existing string union, so back-compat holds.