Skip to content

fix(api): honor OpenAI-compatible retry classification#1547

Merged
kevincodex1 merged 3 commits into
Gitlawb:mainfrom
chioarub:codex/openai-nonretryable-errors
Jun 6, 2026
Merged

fix(api): honor OpenAI-compatible retry classification#1547
kevincodex1 merged 3 commits into
Gitlawb:mainfrom
chioarub:codex/openai-nonretryable-errors

Conversation

@chioarub

@chioarub chioarub commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

Summary

  • expose retryability for OpenAI-compatible diagnostic categories
  • make withRetry stop retrying marked permanent OpenAI-compatible failures such as auth_invalid
  • preserve existing retry behavior for unmarked first-party/OAuth 401s and retryable OpenAI-compatible categories

Validation

  • bun test src/services/api/withRetry.test.ts src/services/api/openaiErrorClassification.test.ts src/services/api/errors.openaiCompatibility.test.ts

Summary by CodeRabbit

  • Bug Fixes

    • Improved retry behavior for OpenAI-compatible responses: clearly classify which error categories are retryable so non-retryable categories fail fast; repeated affordability adjustments no longer cause endless retries.
    • Network, rate-limit and provider-availability issues still trigger automatic retries; auth/model-not-found errors do not.
  • Tests

    • Expanded test coverage for error classification and retry scenarios, including affordability and context-overflow handling.

@coderabbitai

coderabbitai Bot commented Jun 5, 2026

Copy link
Copy Markdown

Ready to act? Review this PR in Change Stack to turn feedback into patch suggestions you can inspect and refine.

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 5e8e91d5-2b3f-4fb7-8a15-c3f10104195a

📥 Commits

Reviewing files that changed from the base of the PR and between 2498896 and 74b1498.

📒 Files selected for processing (2)
  • src/services/api/withRetry.test.ts
  • src/services/api/withRetry.ts
📜 Recent review details
🧰 Additional context used
📓 Path-based instructions (7)
**/*.test.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (CONTRIBUTING.md)

Add or update tests when the change affects behavior

Files:

  • src/services/api/withRetry.test.ts
**/*.{ts,tsx,js,jsx,py}

📄 CodeRabbit inference engine (CONTRIBUTING.md)

**/*.{ts,tsx,js,jsx,py}: Follow the existing code style in the touched files
Keep comments useful and concise

Files:

  • src/services/api/withRetry.test.ts
  • src/services/api/withRetry.ts
**/*.{ts,tsx}

📄 CodeRabbit inference engine (CONTRIBUTING.md)

Typecheck TypeScript code before submitting

Files:

  • src/services/api/withRetry.test.ts
  • src/services/api/withRetry.ts
**/*

⚙️ CodeRabbit configuration file

**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.

Files:

  • src/services/api/withRetry.test.ts
  • src/services/api/withRetry.ts
{src/services/api/**,src/integrations/**,src/utils/model/**,src/utils/provider*.ts,src/commands/provider/**}

⚙️ CodeRabbit configuration file

{src/services/api/**,src/integrations/**,src/utils/model/**,src/utils/provider*.ts,src/commands/provider/**}: Review provider routing, model selection, env precedence, auth/token handling, OpenAI-compatible shims, retries, proxy behavior, and outbound HTTP behavior with high scrutiny. Block on silent default changes, hidden fallback expansion, credential reuse mistakes, hardcoded provider assumptions, or new network reach that is not intentional and documented.

Files:

  • src/services/api/withRetry.test.ts
  • src/services/api/withRetry.ts
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}

⚙️ CodeRabbit configuration file

{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.

Files:

  • src/services/api/withRetry.test.ts
**

⚙️ CodeRabbit configuration file

**: # Contributing to OpenClaude

Thanks for contributing.

OpenClaude is a fast-moving open-source coding-agent CLI with support for multiple providers, local backends, MCP, and a terminal-first workflow. The best contributions here are focused, well-tested, and easy to review.

Before You Start

  • Search existing issues and discussions before opening a new thread.
  • Check open pull requests for work that overlaps with your contribution. If a PR already exists that addresses the same change, open an issue or discussion first to align on direction — duplicate PRs may be closed without review.
  • Use issues for confirmed bugs and actionable feature work.
  • Use discussions for setup help, ideas, and general community conversation.
  • For larger changes, open an issue first so the scope is clear before implementation.
  • For security reports, follow SECURITY.md.

Pull Requests

Every PR needs a reason. Your PR description must include:

  • what changed and why
  • the user or developer impact
  • the exact checks you ran
  • a linked issue when one exists, using Fixes fix: skip assertMinVersion for third-party providers #123, `Closes `#123, or another clear link
  • screenshots when the PR touches UI, terminal presentation, or the VS Code extension
  • which provider path was tested when the PR changes provider behavior

The PR author is responsible for ensuring their PR is merge-ready. PRs with merge conflicts will not be reviewed or approved until the conflicts are resolved.

Issues are the recommended starting point for anything non-trivial — opening one first helps avoid wasted effort if the change is out of scope or already being worked on. Small fixes, doc corrections, and obvious improvements can stand on their own without a linked issue, as long as the PR description explains the intent.

What Gets Closed Without Review

PRs may be closed without review...

Files:

  • src/services/api/withRetry.test.ts
  • src/services/api/withRetry.ts
🔇 Additional comments (2)
src/services/api/withRetry.ts (1)

798-815: LGTM!

The ordering is correct: local max_tokens recovery paths (context overflow and 402 affordability) are checked before the OpenAI category marker, ensuring these recoverable conditions can still retry even when the shim marks them as context_overflow or unknown. The comment at lines 798-800 documents this intentional precedence clearly.

src/services/api/withRetry.test.ts (1)

296-330: LGTM!

Good coverage for the key interaction: verifies that a context_overflow OpenAI category marker (which is non-retryable per the category set) doesn't block the local max_tokens recovery path. The expected maxTokensOverride of 10941 matches the calculation from the error message values.


📝 Walkthrough

Walkthrough

Adds a retryable-category set and exported predicate for OpenAI-compatible failure categories; integrates classification into withRetry to avoid retries for non-retryable OpenAI-marked errors and makes the OpenRouter/OpenAI 402 max_tokens adjustment a one-time action that fails fast if repeated.

Changes

OpenAI Error Retry Classification

Layer / File(s) Summary
Retry classification contract and predicate
src/services/api/openaiErrorClassification.ts, src/services/api/openaiErrorClassification.test.ts
RETRYABLE_OPENAI_COMPATIBILITY_FAILURE_CATEGORIES enumerates retryable categories; exports isRetryableOpenAICompatibilityFailureCategory() and tests its boolean results for several categories.
Retry logic integration and 402 guard
src/services/api/withRetry.ts, src/services/api/withRetry.test.ts
shouldRetry extracts OpenAI error-category markers and returns false for marked non-retryable categories. Adds a one-time max_tokens adjustment path for parseable 402 affordability errors and throws CannotRetryError if a subsequent 402 occurs after the adjustment; tests exercise auth-invalid, 402-adjustment, repeated-402, and context-overflow behaviors and include test helpers.

Estimated Code Review Effort

🎯 4 (Complex) | ⏱️ ~45 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 3

❌ Failed checks (2 warnings, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Risk Surface Disclosed ⚠️ Warning PR touches auth and provider routing (OpenAI-compatible auth_invalid non-retryable) but commit message does not disclose risk surface—only mentions token adjustment. Update commit message to call out: "honor OpenAI-compatible retry classification; auth_invalid errors now fail fast instead of retrying" or add PR review documenting this risk.
Description check ❓ Inconclusive Description covers the summary and validation, but is missing the Impact and Testing sections required by the template. Add Impact section (user-facing and developer/maintainer effects) and Testing section with completed checkboxes for build, smoke, check, and focused test commands run.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed Title accurately and concisely describes the main change—adding retry classification logic for OpenAI-compatible errors to honor their retryability status.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
No Hidden Policy Change ✅ Passed No hidden policy changes detected. PR is a technical retry classification fix that enforces existing error categorizations; no product, trust-model, routing, telemetry, or permission-policy changes.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

coderabbitai[bot]
coderabbitai Bot previously approved these changes Jun 5, 2026
@chioarub chioarub marked this pull request as ready for review June 5, 2026 10:23

@jatmn jatmn left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I found an issue that needs to be addressed before this is ready.

Findings

  • [P2] Keep affordable 402s on the max_tokens retry path
    src/services/api/withRetry.ts:795
    The new marker gate returns false for any non-retryable OpenAI-compatible category before the existing OpenRouter 402 recovery runs. OpenAI-compatible 402s that do not match a more specific category are emitted by the shim with openai_category=unknown, but the error message can still contain the merged #1263 shape (requested up to ... tokens, but can only afford ...). Those requests used to set maxTokensOverride and retry once; with this PR they fail after the first attempt as CannotRetryError. Please let parseable 402 affordability errors reach the existing parseOpenRouterAffordableMaxTokensError branch, or classify that marker as retryable only for this adjustment path.

coderabbitai[bot]
coderabbitai Bot previously approved these changes Jun 6, 2026
@chioarub chioarub requested a review from jatmn June 6, 2026 00:27

@jatmn jatmn left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I found an issue that needs to be addressed before this is ready.

Findings

  • [P2] Let parseable context overflows reach the max_tokens adjustment
    src/services/api/withRetry.ts:431
    The new marker gate runs before the existing parseMaxTokensContextOverflowError recovery branch, so OpenAI-compatible context-limit responses that include the parseable "input length and max_tokens exceed context limit: ..." shape now fail after the first attempt. The OpenAI shim already classifies those 400/5xx context-overflow bodies with [openai_category=context_overflow,...], and shouldRetry() returns false for that marker before the later branch can set retryContext.maxTokensOverride. I verified the behavior with the real withRetry generator: the same parseable overflow message retries with an adjusted max token cap when unmarked, but fails as CannotRetryError once the context-overflow marker is present. Please let parseable context-overflow errors reach the existing adjustment path, similar to the 402 affordability special case, before treating unadjustable context overflows as non-retryable.

@chioarub chioarub requested a review from jatmn June 6, 2026 07:03

@jatmn jatmn left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for the update. I rechecked the previously discussed paths and do not see any remaining actionable issues from my side.

@kevincodex1

@kevincodex1 kevincodex1 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.

looks great! thanks for this

@kevincodex1 kevincodex1 merged commit f1013df into Gitlawb:main Jun 6, 2026
3 checks passed
@chioarub chioarub deleted the codex/openai-nonretryable-errors branch June 6, 2026 15:54
hotmanxp added a commit to hotmanxp/openclaude that referenced this pull request Jun 7, 2026
Co-Authored-By: Claude <noreply@anthropic.com>
hotmanxp added a commit to hotmanxp/openclaude that referenced this pull request Jun 7, 2026
Upstream tier 2 KEEPs, applied 2 of 7 candidates (5 already applied
under prior syncs but missed by subject-match dedup):

  f1013df fix(api): honor OpenAI-compatible retry classification (Gitlawb#1547)
  8065f8d fix(vscode): send schema-valid permission responses (Gitlawb#1401)

Already-applied (real DIFFERS = local fork divergence only):
  1fc5116 fix(api): tighten reasoning_content heuristic (Gitlawb#1201)  -- byte-equal
  2bed184 perf(attachments): skip skill listings for utility forks (Gitlawb#1545)
                -- diff is feature('TRANSCRIPT_CLASSIFIER')→true + CLAUDE.md→AGENTS.md
  8416faa fix(BashTool): include captured output in non-zero-exit error (Gitlawb#1249)
                -- diff is feature('MONITOR_TOOL')→drop + local @ts-ignore
  f7d42c2 fix(cron): enforce MAX_CRON_PROMPT_CHARS cap (Gitlawb#1224)
                -- diff is 'Claude'→'Open CC' brand string in description

Notes:
- f1013df: openaiErrorClassification.ts adds RETRYABLE_OPENAI_COMPATIBILITY_
  FAILURE_CATEGORIES set + isRetryableOpenAICompatibilityFailureCategory();
  withRetry.ts integrates the new classifier. withRetry.test.ts had a
  merge conflict in the 'retry configuration' describe block (upstream
  added OPENCLAUDE_MAX_RETRIES/OPENCLAUDE_RETRY_DELAY_MS env var tests
  that fork doesn't support -- AGENTS.md keeps CLAUDE_CODE_* env vars).
  Resolved by KEEPING THEIRS for the new 'OpenAI-compatible retry
  classification' block, then DELETED the 'retry configuration' block
  (9 unsupported tests) per fork policy.
- 8065f8d: 2 new files (permissionResponse.js + .test.js) pulled; 2
  existing files patched cleanly. Fork has 2 separate vscode extensions
  (opencc-vscode v0.1.1 simplified + openclaude-vscode v0.2.0 full); the
  upstream sync only touches the openclaude-vscode chat/ subdir.
  permissionResponse.test.js uses jest-style globals; bun test passes
  3/3 (bun:test is jest-compatible for describe/it/expect).

Verification:
  typecheck:  0 errors
  bun test:   2532 pass / 0 fail / 34 skip (full suite)
  build:      Built v0.16.1 → dist/cli.mjs
  vscode:     permissionResponse.test.js 3/3 pass
  naming:     no new openclaude/gitlawb leaks (pre-existing
              openclaude-vscode dir name is intentional)
deagwon97 pushed a commit to deagwon97/openclaude that referenced this pull request Jun 11, 2026
* fix(api): honor OpenAI-compatible retry classification

* fix(api): preserve affordable 402 retry adjustment

* fix(api): preserve context overflow token adjustment
hotmanxp added a commit to hotmanxp/openclaude that referenced this pull request Jun 11, 2026
Upstream tier 2 KEEPs, applied 2 of 7 candidates (5 already applied
under prior syncs but missed by subject-match dedup):

  f1013df fix(api): honor OpenAI-compatible retry classification (Gitlawb#1547)
  8065f8d fix(vscode): send schema-valid permission responses (Gitlawb#1401)

Already-applied (real DIFFERS = local fork divergence only):
  1fc5116 fix(api): tighten reasoning_content heuristic (Gitlawb#1201)  -- byte-equal
  2bed184 perf(attachments): skip skill listings for utility forks (Gitlawb#1545)
                -- diff is feature('TRANSCRIPT_CLASSIFIER')→true + CLAUDE.md→AGENTS.md
  8416faa fix(BashTool): include captured output in non-zero-exit error (Gitlawb#1249)
                -- diff is feature('MONITOR_TOOL')→drop + local @ts-ignore
  f7d42c2 fix(cron): enforce MAX_CRON_PROMPT_CHARS cap (Gitlawb#1224)
                -- diff is 'Claude'→'Open CC' brand string in description

Notes:
- f1013df: openaiErrorClassification.ts adds RETRYABLE_OPENAI_COMPATIBILITY_
  FAILURE_CATEGORIES set + isRetryableOpenAICompatibilityFailureCategory();
  withRetry.ts integrates the new classifier. withRetry.test.ts had a
  merge conflict in the 'retry configuration' describe block (upstream
  added OPENCLAUDE_MAX_RETRIES/OPENCLAUDE_RETRY_DELAY_MS env var tests
  that fork doesn't support -- AGENTS.md keeps CLAUDE_CODE_* env vars).
  Resolved by KEEPING THEIRS for the new 'OpenAI-compatible retry
  classification' block, then DELETED the 'retry configuration' block
  (9 unsupported tests) per fork policy.
- 8065f8d: 2 new files (permissionResponse.js + .test.js) pulled; 2
  existing files patched cleanly. Fork has 2 separate vscode extensions
  (opencc-vscode v0.1.1 simplified + openclaude-vscode v0.2.0 full); the
  upstream sync only touches the openclaude-vscode chat/ subdir.
  permissionResponse.test.js uses jest-style globals; bun test passes
  3/3 (bun:test is jest-compatible for describe/it/expect).

Verification:
  typecheck:  0 errors
  bun test:   2532 pass / 0 fail / 34 skip (full suite)
  build:      Built v0.16.1 → dist/cli.mjs
  vscode:     permissionResponse.test.js 3/3 pass
  naming:     no new openclaude/gitlawb leaks (pre-existing
              openclaude-vscode dir name is intentional)
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.

3 participants