fix(api): enforce API_TIMEOUT_MS for OpenAI-compatible headers - #1940
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📜 Recent review details⏰ Context from checks skipped due to timeout. (3)
🧰 Additional context used📓 Path-based instructions (5)**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
⚙️ CodeRabbit configuration file
Files:
**/*.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
{src/services/api/**,src/integrations/**,src/utils/model/**,src/utils/provider*.ts,src/commands/provider/**}⚙️ CodeRabbit configuration file
Files:
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}⚙️ CodeRabbit configuration file
Files:
🔇 Additional comments (2)
📝 WalkthroughWalkthroughAdds configurable response-header deadlines for OpenAI-compatible requests, preserves caller abort reasons, prevents replay of pre-header timeouts, extends Copilot and proxy handling, expands URL secret redaction, and adds documentation and tests. ChangesAPI timeout handling
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 6 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (6 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@src/services/api/openaiShim.ts`:
- Around line 4965-4984: Add focused tests for the Copilot fallback flow that
first receives a 400 from /chat/completions, then switches to /responses. Verify
ResponseHeadersTimeoutError handling, including retrying classified retryable
failures up to the configured attempts, preserving and propagating caller
cancellation, and correctly handling non-retryable failures. Use the existing
provider-specific test helpers and target the fallback implementation
surrounding fetchWithHeadersDeadline.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ba0ddada-9f9f-44bb-b4df-26e351b20a47
📒 Files selected for processing (2)
src/services/api/openaiShim.test.tssrc/services/api/openaiShim.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: CodeRabbit
- GitHub Check: smoke-and-tests (24.11.x)
- GitHub Check: typecheck
- GitHub Check: smoke-and-tests (22)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
TypeScript code in this repository must use strict mode and ESM imports.
Files:
src/services/api/openaiShim.test.tssrc/services/api/openaiShim.ts
**
⚙️ CodeRabbit configuration file
**: # AGENTS.md - AI Agent Coding GuideThis guide is for AI coding agents working in the OpenClaude repository. Read it before changing code, and also follow CONTRIBUTING.md for contributor policy, PR expectations, review follow-up, and project scope.
Project Snapshot
OpenClaude is a coding-agent CLI for cloud and local model providers. It supports OpenAI-compatible APIs, Anthropic, Gemini, DeepSeek, Ollama, MCP, local backends, slash commands, tools, agents, and a React/Ink terminal UI.
The installed CLI runs on Node.js
>=22.0.0. Bun is used for source builds, scripts, dependency management, and tests.Work Style
- Keep changes focused on one problem.
- Prefer existing patterns in the file or nearby module.
- Avoid unrelated formatting, renames, dependency changes, or broad rewrites.
- Add or update tests when behavior changes.
- Update docs when setup, commands, provider behavior, or user-facing behavior changes.
- For new features, larger refactors, dependencies, or runtime changes, follow the issue-first guidance in CONTRIBUTING.md.
Stack And Conventions
- TypeScript with strict mode and ESM imports.
- React + Ink for terminal UI.
- Bun lockfile and Bun scripts for development workflows.
- Node runtime for the built CLI.
Common libraries and patterns:
chalkfor terminal color.commanderfor CLI argument parsing.execafor child processes.- Existing service, provider, settings, permission, and UI patterns over new abstractions.
Repository Map
src/commands/- slash and CLI command implementations.src/components/- React/Ink UI components.src/services/- API, MCP, OAuth, wiki, voice, and other service integrations.src/tools/- tool implementations.src/utils/- shared utilities.src/integrations/- provider and model integration metadata.src/entrypoints/- CLI, MCP, SDK, and generated public types.src/tasks/- local, remote, workflow, and monitor tas...
Files:
src/services/api/openaiShim.test.tssrc/services/api/openaiShim.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/openaiShim.test.tssrc/services/api/openaiShim.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/openaiShim.test.tssrc/services/api/openaiShim.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/openaiShim.test.ts
🔇 Additional comments (3)
src/services/api/openaiShim.ts (1)
128-128: LGTM!Also applies to: 151-168, 218-278, 3867-3867, 4671-4760, 4825-4869, 5215-5215
src/services/api/openaiShim.test.ts (2)
58-58: LGTM!Also applies to: 367-380, 409-423, 491-491, 535-535, 1605-1625, 6919-7128
6908-6910: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winRemove the duplicate declaration that breaks compilation.
erroris declared twice in the same block, so TypeScript cannot compile this test.Proposed fix
expect(caught).toBeDefined() const error = caught as Error & { constructor: { name: string } } - const error = caught as Error & { constructor: { name: string } } expect(error.constructor.name).toBe('APIConnectionError')> Likely an incorrect or invalid review comment.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Findings
F1 — MEDIUM: cleanupAfterHeaders() leaks the abortFromCaller listener on the caller's AbortSignal
Evidence: combineRequestSignals manual branch (openaiShim.ts:251–277):
const abortFromCaller = () => {
deadlineSignal.removeEventListener('abort', abortFromDeadline)
combined.abort(callerSignal.reason)
}
const cleanupAfterHeaders = () => {
deadlineSignal.removeEventListener('abort', abortFromDeadline) // only the deadline listener removed
}
const cleanup = () => {
callerSignal.removeEventListener('abort', abortFromCaller) // only cleanup() removes caller listener
cleanupAfterHeaders()
}
callerSignal.addEventListener('abort', abortFromCaller, { once: true })fetchWithHeadersDeadline success path (openaiShim.ts:4706–4713):
finally {
clearTimeout(timer)
if (headersReceived) { cleanupAfterHeaders() } else { cleanup() }
}Impact: After headers are received, cleanupAfterHeaders() removes only the deadline listener; the abortFromCaller { once: true } listener stays attached to callerSignal. That closure captures combined (an AbortController) and deadlineController, keeping them alive until callerSignal next aborts. If a single callerSignal is reused across many create() calls (common for session/conversation-scoped signals), each successful request adds one dangling listener + captured controller. Over a long session this grows unbounded → genuine memory leak. The combined signal is consumed only inside fetchWithHeadersDeadline, so it is dead weight after headers.
Scope: Only triggers when AbortSignal.any is unavailable (runtimes < Node 20 / older). On modern Node/Bun (the repo's targets) the native branch is used and is leak-free. Still, the function should be self-cleaning.
Recommendation:
const cleanupAfterHeaders = () => {
callerSignal.removeEventListener('abort', abortFromCaller)
deadlineSignal.removeEventListener('abort', abortFromDeadline)
}(Converged across wave1-state, wave2-state, wave2-contract, wave2-skeptic.)
F2 — LOW/MEDIUM: Caller-cancel vs header-timeout microtask race
Evidence: Retry decision in the loop catch (openaiShim.ts:4844–4859):
if (options?.signal?.aborted) { throw preserveCallerAbortError(error, options.signal) }
...
if (isResponseHeadersTimeout && failure.retryable && attempt < maxAttempts - 1) continueThe retry gate keys off the synchronous options?.signal?.aborted flag. If the deadline fires and the caller's abort() is observed only after the ResponseHeadersTimeoutError is thrown into the catch, options?.signal?.aborted is still false, so the code continues into a fresh attempt before honoring the cancel.
Impact: Not a hang — the new attempt's combineRequestSignals immediately sees callerSignal.aborted and aborts, re-throwing the caller's reason. The blast radius is exactly one wasted retry of an already-cancelled request. Narrow timing window.
Recommendation: Re-check options?.signal?.aborted after the header-timeout branch (or validate caller abort before every continue) so a late cancel always wins. Add a test for the "deadline-fires-first, then caller-aborts" ordering (current caller abort winning the timeout catch race test only covers the reverse ordering).
F3 — LOW (by design, worth noting): maxAttempts = Math.max(2, …) raises the retry floor
Evidence: openaiShim.ts:4720–4724:
const maxAttempts = Math.max(
2,
Math.max(isGithub ? GITHUB_429_MAX_RETRIES : 1, credentialPoolAttempts) + maxSelfHealAttempts,
)Impact: For a non-local, non-GitHub, single-credential backend the old value was 1 (no retry); it is now 2. This floor is required for the new header-timeout retry to fire. Importantly, there is no generic network-retry continue in the loop — the only continue reachable for these backends is the header-timeout one — so ordinary network errors do not gain a spurious extra attempt. GitHub (→3) and local self-heal counts are unchanged. Conclusion: behavior change is real but limited to the intended one-shot timeout retry; not an unintended regression.
Recommendation: Document the change explicitly in the PR; confirm no existing test asserts "exactly 1 call on first failure."
F4 — LOW/MEDIUM: URL redaction misses path-embedded secrets
Evidence: redactUrlForDiagnostics (openaiShim.ts:493–499) → redactUrlForDisplay (src/utils/redaction.ts:477+) redacts userinfo, sensitive query params, and # fragments, but never touches pathname. redactSecretValueForDisplay masks only the whole URL value, not substrings. The new ResponseHeadersTimeoutError message embeds the (redacted) URL (openaiShim.ts:151–158) and is logged via logForDebugging. Tests only assert exclusion of password (userinfo) and token=secret (query) (openaiShim.test.ts timeout tests).
Impact: A base URL with a token in the path (e.g. https://api.example.test/v1/abc123SECRET/...) would leak that segment into diagnostics/logs. Latent — depends on provider URL shape.
Recommendation: Run the redacted URL through a substring-based secret masker (e.g. redactSecretSubstringsForDisplay) before building the diagnostic, and extend the test to assert a path-embedded token is masked.
F5 — LOW: createOpenAIShimClient's timeout option is dead; API_TIMEOUT_MS parsed three ways
Evidence: createOpenAIShimClient accepts timeout?: number (openaiShim.ts:5194) but the body never reads it; fetchWithHeadersDeadline calls getApiTimeoutMs() (openaiShim.ts:218–225) which reads process.env.API_TIMEOUT_MS directly. Meanwhile client.ts (~493/516/555) and claude.ts (getNonstreamingFallbackTimeoutMs) parse the same env var with different logic (no trim/validation in client.ts; claude.ts returns 0/NaN unchecked; shim clamps to 2^31-1).
Impact: A programmatic timeout override has zero effect; operators tuning API_TIMEOUT_MS get different effective ceilings per code path (e.g. 0 → 600000 in shim but 0 in claude.ts; 3000000000 clamped in shim but raw elsewhere). No crash, but a foot-gun.
Recommendation: Use a single shared API_TIMEOUT_MS parser; either honor the timeout option or remove it.
F6 — LOW: getApiTimeoutMs edge behaviors & naming
API_TIMEOUT_MS=0does not disable the deadline (falls back to 600000). Intentional but undocumented.- Clamp reuses
MAX_STREAM_IDLE_TIMEOUT_MS(2³¹−1) — correctly matchessetTimeout's 32-bit-signed ceiling (so the 2³¹−1 bound is right, not a bug), but the name is misleading for an API-headers timeout. Introduce a dedicatedMAX_API_TIMEOUT_MS. - Retryability of the header timeout relies on
classifyOpenAINetworkFailurematching the substring"timeout"(present in"(API_TIMEOUT_MS)") — and the defaultnetwork_errorbranch is alsoretryable:true, so the retry is robustly enabled, but the dependency on message wording is indirect/fragile. An explicitResponseHeadersTimeoutErrorcase would be clearer.
F7 — LOW: Test gaps
- No test exercises the native
AbortSignal.anypath + caller-abort propagation; all such tests force the manual fallback viaObject.defineProperty(AbortSignal, 'any', undefined). The native branch is covered by the timeout-retry tests but the caller-abort-through-AbortSignal.anycontract isn't directly asserted. preserveCallerAbortErrorreturnscallerSignal.reason ?? error; if the caller aborts without a reason and the deadline fired, the surfaced error is the timeout error rather than the caller's abort (cosmetic edge).
|
Addressed the review against commit
Current validation: OpenAI shim 226/226, provider suite 1131/1131, typecheck, build, security scan, and diff checks all pass. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/services/api/openaiShim.ts (2)
4692-4737: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDefer
ResponseHeadersTimeoutErrorconstruction into the timer callback.
timeoutReason(including theredactUrlForDiagnostics(url)call — URL parse,decodeURIComponent, two regex-based redaction passes) is built synchronously on everyfetchWithHeadersDeadlineinvocation, even though it's only needed if the deadline actually fires. For fast/successful requests (the common case) and every attempt of a multi-attempt retry, this does unnecessary parsing/redaction work on the hot path.⚡ Proposed fix: build the error lazily when the timer fires
const fetchWithHeadersDeadline = async ( url: string, init: RequestInit, ): Promise<Response> => { const deadlineController = new AbortController() - const timeoutReason = new ResponseHeadersTimeoutError( - apiTimeoutMs, - redactUrlForDiagnostics(url), - ) const { signal, cleanupAfterHeaders, cleanup } = combineRequestSignals( options?.signal, deadlineController.signal, ) const timer = setTimeout( - () => deadlineController.abort(timeoutReason), + () => + deadlineController.abort( + new ResponseHeadersTimeoutError(apiTimeoutMs, redactUrlForDiagnostics(url)), + ), apiTimeoutMs, ) timer.unref?.() let headersReceived = false try { const response = await fetchWithProxyRetry(url, { ...init, signal }) headersReceived = true return response } catch (error) { if (options?.signal?.aborted) { throw preserveCallerAbortError(error, options.signal) } if ( deadlineController.signal.aborted && - deadlineController.signal.reason === timeoutReason + deadlineController.signal.reason instanceof ResponseHeadersTimeoutError ) { - throw timeoutReason + throw deadlineController.signal.reason } throw error } finally { ... } }🤖 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 `@src/services/api/openaiShim.ts` around lines 4692 - 4737, Move ResponseHeadersTimeoutError construction out of the synchronous fetchWithHeadersDeadline setup and into the setTimeout callback, creating the error immediately before aborting deadlineController. Update the timeout check to compare against the lazily created timeout reason while preserving the existing timeout and caller-abort handling.
4988-5007: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMirror the primary abort guard in the
/responsesfallback.
This catch can still receiveAbortError-shaped failures that are not the caller signal or the timeout controller; without the sameisAbortErrorshort-circuit, they get classified as retryablerequest_timeoutand handled differently from the primary path.🤖 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 `@src/services/api/openaiShim.ts` around lines 4988 - 5007, The /responses fallback catch in the responses request flow must mirror the primary abort handling for non-caller abort failures. After the preserveCallerAbortError check, add the existing isAbortError short-circuit to rethrow abort-shaped errors before classifyOpenAINetworkFailure; leave timeout retry handling unchanged.
🤖 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.
Outside diff comments:
In `@src/services/api/openaiShim.ts`:
- Around line 4692-4737: Move ResponseHeadersTimeoutError construction out of
the synchronous fetchWithHeadersDeadline setup and into the setTimeout callback,
creating the error immediately before aborting deadlineController. Update the
timeout check to compare against the lazily created timeout reason while
preserving the existing timeout and caller-abort handling.
- Around line 4988-5007: The /responses fallback catch in the responses request
flow must mirror the primary abort handling for non-caller abort failures. After
the preserveCallerAbortError check, add the existing isAbortError short-circuit
to rethrow abort-shaped errors before classifyOpenAINetworkFailure; leave
timeout retry handling unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7f587daa-b04e-46e6-bb42-5038cb715768
📒 Files selected for processing (2)
src/services/api/openaiShim.test.tssrc/services/api/openaiShim.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: CodeRabbit
- GitHub Check: smoke-and-tests (24.11.x)
- GitHub Check: smoke-and-tests (22)
- GitHub Check: typecheck
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
TypeScript code in this repository must use strict mode and ESM imports.
Files:
src/services/api/openaiShim.test.tssrc/services/api/openaiShim.ts
**
⚙️ CodeRabbit configuration file
**: # AGENTS.md - AI Agent Coding GuideThis guide is for AI coding agents working in the OpenClaude repository. Read it before changing code, and also follow CONTRIBUTING.md for contributor policy, PR expectations, review follow-up, and project scope.
Project Snapshot
OpenClaude is a coding-agent CLI for cloud and local model providers. It supports OpenAI-compatible APIs, Anthropic, Gemini, DeepSeek, Ollama, MCP, local backends, slash commands, tools, agents, and a React/Ink terminal UI.
The installed CLI runs on Node.js
>=22.0.0. Bun is used for source builds, scripts, dependency management, and tests.Work Style
- Keep changes focused on one problem.
- Prefer existing patterns in the file or nearby module.
- Avoid unrelated formatting, renames, dependency changes, or broad rewrites.
- Add or update tests when behavior changes.
- Update docs when setup, commands, provider behavior, or user-facing behavior changes.
- For new features, larger refactors, dependencies, or runtime changes, follow the issue-first guidance in CONTRIBUTING.md.
Stack And Conventions
- TypeScript with strict mode and ESM imports.
- React + Ink for terminal UI.
- Bun lockfile and Bun scripts for development workflows.
- Node runtime for the built CLI.
Common libraries and patterns:
chalkfor terminal color.commanderfor CLI argument parsing.execafor child processes.- Existing service, provider, settings, permission, and UI patterns over new abstractions.
Repository Map
src/commands/- slash and CLI command implementations.src/components/- React/Ink UI components.src/services/- API, MCP, OAuth, wiki, voice, and other service integrations.src/tools/- tool implementations.src/utils/- shared utilities.src/integrations/- provider and model integration metadata.src/entrypoints/- CLI, MCP, SDK, and generated public types.src/tasks/- local, remote, workflow, and monitor tas...
Files:
src/services/api/openaiShim.test.tssrc/services/api/openaiShim.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/openaiShim.test.tssrc/services/api/openaiShim.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/openaiShim.test.tssrc/services/api/openaiShim.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/openaiShim.test.ts
🔇 Additional comments (7)
src/services/api/openaiShim.ts (3)
100-100: LGTM!Also applies to: 129-129, 152-168, 219-279, 3890-3890, 5238-5238
494-522: LGTM! Pathname-level redaction (decode → substring-redact → re-encode viaparsed.pathname) correctly handles the percent-encoded secret case, and the malformed-pathname catch falls back gracefully to URL-level redaction.
4739-4755: LGTM! TheisResponseHeadersTimeout/isAbortErrorshort-circuit correctly keeps generic abort errors out ofclassifyOpenAINetworkFailure, and the retrycontinueis properly scoped to classified, retryable header timeouts only, matching the documented two-attempt-for-timeouts-only design.Also applies to: 4848-4883
src/services/api/openaiShim.test.ts (4)
58-58: LGTM!API_TIMEOUT_MSis properly saved/cleared/restored around tests, keeping global env state isolated. As per path instructions, tests should ensure "isolation of global/env/config state."Also applies to: 518-518, 562-562
Source: Path instructions
367-380: LGTM!Also applies to: 409-450
1632-1652: LGTM! Good coverage of the parser contract: default fallback, valid values (incl. whitespace-padded), INT32 clamp, and invalid inputs (including'0') falling back to default.
6902-6906: LGTM! Correctly asserts neither the raw nor percent-encoded secret leaks into the timeout error message, exercising the pathname-decode-and-redact path inredactUrlForDiagnostics.Also applies to: 6946-6947
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Apply the header deadline to the Copilot Responses transport
src/services/api/openaiShim.ts:3784
GitHub Copilot GPT-5/Codex models resolve tocodex_responsesand return throughperformCodexRequest()before_doOpenAIRequest()installsfetchWithHeadersDeadline(). That request ultimately callsfetchWithProxyRetry()with only the caller signal, so a Copilot/responsesconnection that never produces headers can still hang indefinitely and is never given the new timeout retry. This is a supported OpenAI-compatible/Copilot route (the adjacent provider-config test explicitly routes GitHub GPT-5 Codex models there), so it contradicts the PR's advertised API_TIMEOUT_MS enforcement. Thread the same bounded pre-header deadline through this transport and cover a stalled Copilotcodex_responsesrequest. -
[P1] Redact encoded pathname secrets even when the path cannot be decoded once
src/services/api/openaiShim.ts:500
The new path redaction decodes the whole pathname once, then only searches for the raw configured secret. A doubly encoded credential such asroute%252Fkey%252BAbC123(or a correctly encoded credential in a path that also contains an invalid%ZZescape) therefore survives the decode/search and is emitted in the timeout error and debug diagnostic. It is trivially reversible and defeats the stated path-secret protection. Mask encoded secret representations, or safely decode/redact path components until stable, and add a regression covering this case. -
[P2] Preserve non-caller aborts in the Copilot
/responsesfallback
src/services/api/openaiShim.ts:4993
The primary request catch explicitly rethrows an abort-shaped error unless it is this PR'sResponseHeadersTimeoutError, but the/responsesfallback immediately classifies every non-caller failure. If/chat/completionsproduces the fallback-triggering 400 and the/responsesfetch then rejects withAbortErrorfrom the transport/proxy, this branch turns it into a retryablerequest_timeoutAPI connection error. That changes cancellation semantics and bypasses callers' normal abort handling only for this fallback. Mirror the primaryisAbortErrorshort-circuit beforeclassifyOpenAINetworkFailure()and add the fallback-specific regression test.
|
The latest review item is addressed in commit 03546d2, and the branch remains based on the current
Validation completed on the merged branch:
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/services/api/openaiShim.ts (1)
3925-3986: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPropagate the header deadline to the non-GitHub Codex path. The
request.transport === 'codex_responses' && !isGithubModebranch still callsperformCodexRequestwithout the deadline-awarefetcher, soAPI_TIMEOUT_MSnever applies there. If this path should share the same timeout behavior as the Copilot branch, pass the samefetchWithHeadersDeadlinewrapper here too.🤖 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 `@src/services/api/openaiShim.ts` around lines 3925 - 3986, Update the non-GitHub codex_responses path to pass a deadline-aware fetcher to performCodexRequest, matching the Copilot branch’s fetchWithHeadersDeadline usage with options?.signal and the configured API timeout. Preserve the existing request credentials, headers, and response handling while ensuring API_TIMEOUT_MS applies to this branch.Source: Path instructions
🤖 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.
Outside diff comments:
In `@src/services/api/openaiShim.ts`:
- Around line 3925-3986: Update the non-GitHub codex_responses path to pass a
deadline-aware fetcher to performCodexRequest, matching the Copilot branch’s
fetchWithHeadersDeadline usage with options?.signal and the configured API
timeout. Preserve the existing request credentials, headers, and response
handling while ensuring API_TIMEOUT_MS applies to this branch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c09c39cd-5803-4f59-9d59-c49e96a51cc2
📒 Files selected for processing (3)
src/services/api/codexShim.tssrc/services/api/openaiShim.test.tssrc/services/api/openaiShim.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: smoke-and-tests (22)
- GitHub Check: typecheck
- GitHub Check: smoke-and-tests (24.11.x)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
TypeScript code in this repository must use strict mode and ESM imports.
Files:
src/services/api/codexShim.tssrc/services/api/openaiShim.test.tssrc/services/api/openaiShim.ts
**
⚙️ CodeRabbit configuration file
**: # AGENTS.md - AI Agent Coding GuideThis guide is for AI coding agents working in the OpenClaude repository. Read it before changing code, and also follow CONTRIBUTING.md for contributor policy, PR expectations, review follow-up, and project scope.
Project Snapshot
OpenClaude is a coding-agent CLI for cloud and local model providers. It supports OpenAI-compatible APIs, Anthropic, Gemini, DeepSeek, Ollama, MCP, local backends, slash commands, tools, agents, and a React/Ink terminal UI.
The installed CLI runs on Node.js
>=22.0.0. Bun is used for source builds, scripts, dependency management, and tests.Work Style
- Keep changes focused on one problem.
- Prefer existing patterns in the file or nearby module.
- Avoid unrelated formatting, renames, dependency changes, or broad rewrites.
- Add or update tests when behavior changes.
- Update docs when setup, commands, provider behavior, or user-facing behavior changes.
- For new features, larger refactors, dependencies, or runtime changes, follow the issue-first guidance in CONTRIBUTING.md.
Stack And Conventions
- TypeScript with strict mode and ESM imports.
- React + Ink for terminal UI.
- Bun lockfile and Bun scripts for development workflows.
- Node runtime for the built CLI.
Common libraries and patterns:
chalkfor terminal color.commanderfor CLI argument parsing.execafor child processes.- Existing service, provider, settings, permission, and UI patterns over new abstractions.
Repository Map
src/commands/- slash and CLI command implementations.src/components/- React/Ink UI components.src/services/- API, MCP, OAuth, wiki, voice, and other service integrations.src/tools/- tool implementations.src/utils/- shared utilities.src/integrations/- provider and model integration metadata.src/entrypoints/- CLI, MCP, SDK, and generated public types.src/tasks/- local, remote, workflow, and monitor tas...
Files:
src/services/api/codexShim.tssrc/services/api/openaiShim.test.tssrc/services/api/openaiShim.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/codexShim.tssrc/services/api/openaiShim.test.tssrc/services/api/openaiShim.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/codexShim.tssrc/services/api/openaiShim.test.tssrc/services/api/openaiShim.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/openaiShim.test.ts
🔇 Additional comments (13)
src/services/api/openaiShim.test.ts (5)
58-58: LGTM!Also applies to: 367-380, 409-450, 518-518, 562-562, 1632-1652
6904-6914: LGTM!Also applies to: 6956-6961
6965-7047: LGTM!Also applies to: 7048-7099, 7100-7174
9780-9835: LGTM!Also applies to: 9836-9966
58-58: 📐 Maintainability & Code QualityConfirm required GitHub checks before merge.
The PR summary reports 43
bun run checkfailures, albeit described as unrelated. This is code-ready only pending green required checks or maintainer triage of those failures. As per path instructions, “If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.”Source: Path instructions
src/services/api/openaiShim.ts (7)
5099-5127: This is the/chat/completions400 →/responsesCopilot fallback that a prior review flagged as lacking focused timeout/abort/retry coverage. The test file isn't in this diff view, so I can't confirm the branch is now exercised — please verify a regression test drives aResponseHeadersTimeoutErrorhere (retryable retry + caller-cancellation-wins + non-retryable failure).#!/bin/bash fd -t f 'openaiShim.test.ts' src/services/api --exec rg -nP -C2 '/responses|ResponseHeadersTimeout|chat/completions' {}Source: Path instructions
170-180: LGTM!
292-339: LGTM!
555-628: LGTM!
634-666: LGTM!
4871-4878: LGTM!
4969-4994: LGTM!src/services/api/codexShim.ts (1)
597-603: LGTM!Also applies to: 682-692
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P2] Do not replay an ambiguous generation request after a header timeout
src/services/api/openaiShim.ts:4987
A pre-header timeout only proves that this client did not receive a response; it does not prove that the provider did not receive and start the POST. This newcontinuereplays the same prompt without an idempotency contract, so a slow provider can process and bill two generations while OpenClaude returns only the second one. Avoid retrying this ambiguous POST automatically, or introduce a retry mechanism that the supported providers can deduplicate (and cover the received-but-delayed-header case). -
[P3] Document the now-effective
API_TIMEOUT_MSsetting
src/services/api/openaiShim.ts:231
This makesAPI_TIMEOUT_MSa user-visible control for all OpenAI-compatible requests, with a 600000 ms default, positive-integer validation, and a 32-bit timer cap. The OpenAI-compatible environment-variable table indocs/advanced-setup.mdhas no entry for it, so users who hit the new timeout (whose error tells them to increase it) have no documented configuration contract or valid range. Add the setting and its scope/default/validation to the setup docs.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Findings
F1 — P2 — Encoded env secrets in query strings survive redactUrlForDiagnostics
Where: src/services/api/openaiShim.ts (redactUrlForDiagnostics ~606–629, redactDecodedPathSecrets ~592–604; used by ResponseHeadersTimeoutError ~302–305 and transport logging ~646–654)
Evidence (HEAD):
-
Path redaction multi-decodes and substring-masks configured secrets (
redactDecodedPathSecretsloop). -
Query handling still relies on:
redactUrlForDisplayname-token redaction (token,key,secret, …), then- final
redactSecretSubstringsForDisplayon the raw string (no decode of query values).
-
Production-layer proof (same helpers as HEAD,
OPENAI_API_KEY=route/key+AbC123):Input Result path .../v1/{doubleEncodedSecret}masked ?prompt=route%2Fkey%2BAbC123LEAK (encoded secret intact) ?prompt=route%252Fkey%252BAbC123LEAK ?prompt=route/key+AbC123(plain)masked by final substring pass ?token=secret/?api_token=...masked by name rules -
Timeout / transport errors embed that string in user-visible
APIConnectionErrormessages and debug lines.
Impact: The PR’s security claim for path-embedded secrets is real, but the same configured secrets in non-sensitive-named, percent-encoded query params still appear in timeout diagnostics. The new regression test only exercises ?token=secret (name-based), so this hole is unguarded.
Recommendation: Apply the same decode→substring-redact loop to parsed.search / the full serialized URL (not only pathname). Extend the pre-header timeout test with encoded query cases (?prompt=${encodeURIComponent(secret)}, double-encoded). Prefer redactSecretSubstringsForDisplay (and decode) on transport safeMessage as defense-in-depth (createClassifiedTransportError currently uses whole-value redactSecretValueForDisplay only).
F2 — P2 — Production AbortSignal.any path cannot unlink the deadline from the fetch signal after headers
Where: src/services/api/openaiShim.ts combineRequestSignals ~257–262, fetchWithHeadersDeadline finally ~332–338
Evidence (HEAD):
- Repo engines require Node
>=22, whereAbortSignal.anyis always present → production always takes the native branch. - Native branch returns no-op
cleanupAfterHeaders/cleanup. Disarm is onlyclearTimeout(timer). - Manual fallback does remove the deadline listener so a late
deadlineController.abort(...)does not abort the combined signal. - Runtime proof (Node 22.14 + undici): aborting the deadline half of
AbortSignal.anyafter headers rejects subsequent body reads withAbortError. Manual unlink + residual deadline abort leaves the body readable. No-caller path binds fetch directly todeadlineSignalwith the same residual linkage.
Impact / practicality:
- For the common case,
clearTimeoutinfinallyruns synchronously afterawait fetch…resolves, so a timer-only residual race is narrow. - The documented contract (“deadline is disarmed after headers… does not limit response streaming” in
docs/advanced-setup.md) is stronger than the native implementation: the deadline signal remains fused into the request signal for the body’s lifetime. Any residual abort of that controller (timer mis-ordering, future change, shared controller misuse) cancels in-flight streams /response.text(). - Manual path and native path are asymmetric; tests that disable
AbortSignal.anyexercise the safer unlink behavior, not production.
Recommendation: Prefer a combined controller that can drop the deadline after headers on all runtimes (or stop attaching the deadline signal to the long-lived fetch signal once headers arrive). Align docs if residual linkage is intentionally accepted. Add a native-path regression that forces a post-header deadline abort and asserts the body still completes (or documents that only clearTimeout is required).
F3 — P3 — Manual-fallback success path retains abortFromCaller on reused caller signals
Where: combineRequestSignals manual branch ~265–290; success finally calls only cleanupAfterHeaders() (~334–335)
Evidence: Replaying the success cleanup path left N listeners on a shared callerSignal after N requests (verified with Node getEventListeners).
Impact: Unbounded listener + closure retention only when AbortSignal.any is unavailable. Production Node ≥22 uses the native branch (no leak). Still relevant for the intentional fallback and for tests that stub AbortSignal.any off. Author previously argued the listener is needed for post-header fetch cancellation; stream readers already listen on the original options.signal, so full listener removal after headers may be viable—worth a deliberate design note/test.
Recommendation: On success, remove the caller listener once post-header cancel is guaranteed via the stream/body path; or document that the fallback is best-effort and not used on supported engines.
F4 — P3 — API_TIMEOUT_MS multi-parser / multi-consumer inconsistency
Where / evidence:
| Consumer | Behavior |
|---|---|
openaiShim.getApiTimeoutMs |
strict /^\d+$/, safe int, >0, cap 2^31−1, default 600_000 |
client.ts Anthropic / shim timeout: |
parseInt(..., 10) (accepts garbage prefixes; no cap; NaN risk) |
claude.ts getNonstreamingFallbackTimeoutMs |
parseInt; truthy only; defaults 300s / 120s remote, not 600s |
Docs/.env.example describe the shim headers-deadline contract only. Operators who set API_TIMEOUT_MS=0 or fractional/large values get different outcomes depending on path. Pre-existing multi-use of the env var, but this PR makes the shim semantics user-facing and documents them as the definition of API_TIMEOUT_MS.
Recommendation: Note in docs that Anthropic SDK / non-streaming fallback still use separate parsers/defaults; or consolidate later (out of this PR’s narrow scope is fine if documented).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@src/services/api/openaiShim.test.ts`:
- Around line 6806-6825: Update the test “redacts configured secret substrings
from fetch network error messages” to isolate the OPENAI_API_KEY mutation: wrap
the test setup and assertion in try/finally and invoke the existing restoreEnv
helper in the finally block, unless the suite already guarantees restoration
through a global afterEach snapshot.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c6113b22-7deb-4230-92d4-6687c4ca57d0
📒 Files selected for processing (4)
.env.exampledocs/advanced-setup.mdsrc/services/api/openaiShim.test.tssrc/services/api/openaiShim.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: smoke-and-tests (24.11.x)
- GitHub Check: smoke-and-tests (22)
- GitHub Check: typecheck
🧰 Additional context used
📓 Path-based instructions (5)
**/*
⚙️ 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:
docs/advanced-setup.mdsrc/services/api/openaiShim.test.tssrc/services/api/openaiShim.ts
{README.md,CONTRIBUTING.md,docs/**,.github/pull_request_template.md}
⚙️ CodeRabbit configuration file
{README.md,CONTRIBUTING.md,docs/**,.github/pull_request_template.md}: Review docs for accuracy against current code behavior. Flag security or provider claims that overpromise, stale install commands, missing setup caveats, and instructions that could push users toward unsafe credential handling. Keep purely wording-level suggestions non-blocking.
Files:
docs/advanced-setup.md
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
TypeScript code in this repository must use strict mode and ESM imports.
Files:
src/services/api/openaiShim.test.tssrc/services/api/openaiShim.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/openaiShim.test.tssrc/services/api/openaiShim.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/openaiShim.test.ts
🔇 Additional comments (14)
.env.example (1)
425-426: LGTM!docs/advanced-setup.md (1)
420-420: LGTM!src/services/api/openaiShim.test.ts (2)
3-3: LGTM!
7144-7178: LGTM!src/services/api/openaiShim.ts (10)
5176-5238: 📐 Maintainability & Code Quality | ⚡ Quick winStill missing focused coverage for the Copilot
/chat/completions→/responsesfallback timeout path.This block now applies the same header-deadline + abort-preservation + non-retryable classification logic as the primary request path, but there's no visible test driving the initial 400 into
/responsesand then exercisingResponseHeadersTimeoutErrorhandling, caller-abort preservation, and non-retryable classification specifically on this fallback branch. This was already flagged on a prior revision;openaiShim.test.tsisn't in this file set so I can't confirm it's been closed.As per path instructions, provider behavior changes in
src/services/api/**should be reviewed with high scrutiny for retries/outbound HTTP behavior, and{src/**/*.test.ts,...}requires tests that cover the exact changed path including timeout, abort, and retry classification.Source: Path instructions
154-159: LGTM!
241-356: Signal cleanup bifurcation is correctly designed.
combineRequestSignals/wrapResponseBodyWithCleanupcorrectly separate "disarm the deadline after headers" from "fully release listeners after body completion," preserving caller cancellation through streaming while letting the deadline timer'sclearTimeoutbe the sole disarming mechanism for theAbortSignal.anyfast path. No correctness issues found.
358-409: Deadline-vs-caller disambiguation is robust.Throwing
timeoutReasonbased ondeadlineController.signal.aborted && reason === timeoutReason(rather than trusting whateverfetchrejected with) correctly sidesteps any runtime inconsistency in howAbortSignal/fetchpropagate custom abort reasons. Good defensive design.
661-707: Decode-then-redact loop is sound; preserves original encoding when nothing is found.Returning the original (undecoded)
valuewhenfoundSecretstaysfalseavoids gratuitously re-encoding/altering the diagnostic URL when no secret is present. Loop termination is bounded by decode-escape convergence. No issues found.
713-753: LGTM!
4147-4152: LGTM!
4956-5088: Deadline wiring and abort-preservation ordering in the main attempt loop look correct.Caller-abort check first, then non-timeout
AbortErrorpassthrough, then forced non-retryable classification forResponseHeadersTimeoutError, then local self-heal, then generic classification — this ordering matches the stated goal that caller cancellation wins races and header-timeouts are never silently retried as ambiguous POSTs.
5445-5445: LGTM!
4039-4050: 🎯 Functional CorrectnessNo issue here.
performCodexRequestalways calls the fetcher as(url, init)with a concreteRequestInit, so this adapter never receives a populatedRequestobject.> Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.env.example (1)
426-433: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument the exact timeout transport scope.
The documentation should explicitly include direct GitHub Copilot Responses and Copilot fallback transport. The current broad statement that non-streaming fallback paths retain existing handling conflicts with the implemented scope.
.env.example#L426-L433: list generic OpenAI-compatible requests and both affected Copilot transports; identify first-party Codex OAuth and Anthropic as unchanged.docs/advanced-setup.md#L440-L440: replace the broad fallback exclusion with the same explicit transport list.As per path instructions, “Review docs for accuracy against current code behavior,” and AGENTS.md requires documenting “the exact provider/model path affected.”
🤖 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 @.env.example around lines 426 - 433, Update the API_TIMEOUT_MS documentation to state its exact scope: generic OpenAI-compatible requests, direct GitHub Copilot Responses, and Copilot fallback transport are affected, while first-party Codex OAuth and Anthropic remain unchanged. Apply this clarification in .env.example lines 426-433 and docs/advanced-setup.md line 440, replacing the broad non-streaming fallback exclusion.Source: Path instructions
🤖 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.
Inline comments:
In `@src/services/api/openaiShim.test.ts`:
- Around line 4187-4216: Move the cooldown regression test around
createOpenAIShimClient and the Z.AI/NVIDIA tool_stream regression tests around
the referenced additional section into the changes that implement those
behaviors, keeping the API_TIMEOUT_MS change limited to timeout-related
validation.
---
Outside diff comments:
In @.env.example:
- Around line 426-433: Update the API_TIMEOUT_MS documentation to state its
exact scope: generic OpenAI-compatible requests, direct GitHub Copilot
Responses, and Copilot fallback transport are affected, while first-party Codex
OAuth and Anthropic remain unchanged. Apply this clarification in .env.example
lines 426-433 and docs/advanced-setup.md line 440, replacing the broad
non-streaming fallback exclusion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 071aac7e-244a-4815-925a-e276deeccc70
📒 Files selected for processing (5)
.env.exampledocs/advanced-setup.mdsrc/services/api/openaiErrorClassification.tssrc/services/api/openaiShim.test.tssrc/services/api/openaiShim.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: smoke-and-tests (24.11.x)
- GitHub Check: smoke-and-tests (22)
🧰 Additional context used
📓 Path-based instructions (6)
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*: Keep changes focused on one problem or feature and avoid mixing unrelated cleanup into the same change.
Preserve existing repository patterns unless intentionally refactoring them.
Update documentation when setup, commands, or user-facing behavior changes.
Review AI-generated changes for correctness, style consistency, unnecessary changes, and adherence to project architecture before submitting them.
Follow the existing code style in touched files.
Prefer small, readable changes over broad rewrites.
Do not reformat unrelated files.
Keep comments useful and concise.
Provider changes must explicitly identify affected providers, limitations, and follow-up work in the pull request description.
Do not assign or use provider tags; provider tags are controlled by maintainers.
Run the relevant validation checks locally before submitting changes; pull requests must pass CI checks.
Runbun run security:pr-scanbefore submitting a pull request.
Dependency changes require a concrete project benefit, such as fixing a bug, addressing a security issue, or supporting an approved feature.
Do not change the project's language, core runtime, or dependency stack without prior maintainer agreement.
Files:
docs/advanced-setup.mdsrc/services/api/openaiErrorClassification.tssrc/services/api/openaiShim.tssrc/services/api/openaiShim.test.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:
docs/advanced-setup.mdsrc/services/api/openaiErrorClassification.tssrc/services/api/openaiShim.tssrc/services/api/openaiShim.test.ts
{README.md,CONTRIBUTING.md,docs/**,.github/pull_request_template.md}
⚙️ CodeRabbit configuration file
{README.md,CONTRIBUTING.md,docs/**,.github/pull_request_template.md}: Review docs for accuracy against current code behavior. Flag security or provider claims that overpromise, stale install commands, missing setup caveats, and instructions that could push users toward unsafe credential handling. Keep purely wording-level suggestions non-blocking.
Files:
docs/advanced-setup.md
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
TypeScript code in this repository must use strict mode and ESM imports.
**/*.{ts,tsx}: When changing provider behavior, avoid breaking third-party providers and test the exact provider/model path changed when possible.
Runbun run typecheckandbun run typecheck:type-testsfor TypeScript changes.
Run provider tests and provider recommendation tests when changing provider behavior:bun run test:providerandbun run test:provider-recommendation.
Files:
src/services/api/openaiErrorClassification.tssrc/services/api/openaiShim.tssrc/services/api/openaiShim.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Add or update tests when a code change affects behavior.
Files:
src/services/api/openaiErrorClassification.tssrc/services/api/openaiShim.tssrc/services/api/openaiShim.test.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/openaiErrorClassification.tssrc/services/api/openaiShim.tssrc/services/api/openaiShim.test.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/openaiShim.test.ts
🔇 Additional comments (5)
src/services/api/openaiShim.ts (1)
4263-4265: LGTM!Also applies to: 4874-4875, 4988-4992, 5056-5057, 5350-5379
src/services/api/openaiErrorClassification.ts (1)
14-14: LGTM!Also applies to: 70-70, 174-254, 566-586
.env.example (1)
36-41: LGTM!docs/advanced-setup.md (1)
412-430: LGTM!src/services/api/openaiShim.test.ts (1)
3-3: LGTM!Also applies to: 13-16, 63-63, 372-372, 385-385, 414-455, 523-523, 567-567, 1637-1657, 6840-6905, 6976-7482, 10294-10482
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.env.example (1)
426-433: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument the exact timeout transport scope.
The documentation should explicitly include direct GitHub Copilot Responses and Copilot fallback transport. The current broad statement that non-streaming fallback paths retain existing handling conflicts with the implemented scope.
.env.example#L426-L433: list generic OpenAI-compatible requests and both affected Copilot transports; identify first-party Codex OAuth and Anthropic as unchanged.docs/advanced-setup.md#L440-L440: replace the broad fallback exclusion with the same explicit transport list.As per path instructions, “Review docs for accuracy against current code behavior,” and AGENTS.md requires documenting “the exact provider/model path affected.”
🤖 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 @.env.example around lines 426 - 433, Update the API_TIMEOUT_MS documentation to state its exact scope: generic OpenAI-compatible requests, direct GitHub Copilot Responses, and Copilot fallback transport are affected, while first-party Codex OAuth and Anthropic remain unchanged. Apply this clarification in .env.example lines 426-433 and docs/advanced-setup.md line 440, replacing the broad non-streaming fallback exclusion.Source: Path instructions
🤖 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.
Inline comments:
In `@src/services/api/openaiShim.test.ts`:
- Around line 4187-4216: Move the cooldown regression test around
createOpenAIShimClient and the Z.AI/NVIDIA tool_stream regression tests around
the referenced additional section into the changes that implement those
behaviors, keeping the API_TIMEOUT_MS change limited to timeout-related
validation.
---
Outside diff comments:
In @.env.example:
- Around line 426-433: Update the API_TIMEOUT_MS documentation to state its
exact scope: generic OpenAI-compatible requests, direct GitHub Copilot
Responses, and Copilot fallback transport are affected, while first-party Codex
OAuth and Anthropic remain unchanged. Apply this clarification in .env.example
lines 426-433 and docs/advanced-setup.md line 440, replacing the broad
non-streaming fallback exclusion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 071aac7e-244a-4815-925a-e276deeccc70
📒 Files selected for processing (5)
.env.exampledocs/advanced-setup.mdsrc/services/api/openaiErrorClassification.tssrc/services/api/openaiShim.test.tssrc/services/api/openaiShim.ts
📜 Review details
🔇 Additional comments (5)
src/services/api/openaiShim.ts (1)
4263-4265: LGTM!Also applies to: 4874-4875, 4988-4992, 5056-5057, 5350-5379
src/services/api/openaiErrorClassification.ts (1)
14-14: LGTM!Also applies to: 70-70, 174-254, 566-586
.env.example (1)
36-41: LGTM!docs/advanced-setup.md (1)
412-430: LGTM!src/services/api/openaiShim.test.ts (1)
3-3: LGTM!Also applies to: 13-16, 63-63, 372-372, 385-385, 414-455, 523-523, 567-567, 1637-1657, 6840-6905, 6976-7482, 10294-10482
🛑 Comments failed to post (1)
src/services/api/openaiShim.test.ts (1)
4187-4216: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Move the unrelated regression tests to their respective changes.
Key cooldown behavior and Z.AI/NVIDIA
tool_streamhandling are independent of theAPI_TIMEOUT_MSfeature. Keeping them here broadens validation and rollback scope unnecessarily.As per coding guidelines, “Keep changes focused on one problem or feature and avoid mixing unrelated cleanup into the same change”; AGENTS.md likewise requires tightly scoped changes.
Also applies to: 9399-9601
🤖 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 `@src/services/api/openaiShim.test.ts` around lines 4187 - 4216, Move the cooldown regression test around createOpenAIShimClient and the Z.AI/NVIDIA tool_stream regression tests around the referenced additional section into the changes that implement those behaviors, keeping the API_TIMEOUT_MS change limited to timeout-related validation.Sources: Coding guidelines, Path instructions
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@docs/advanced-setup.md`:
- Line 440: Update the API_TIMEOUT_MS entry in the advanced setup
environment-variable table to explicitly state that it must be exported by the
shell or runtime launcher because provider env-file loading ignores
runtime/debug settings. Preserve the existing timeout behavior and caveat that
unexported configuration falls back to the default.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: cb41edc3-c039-4cfe-b6bc-e0dcfc580ee2
📒 Files selected for processing (2)
.env.exampledocs/advanced-setup.md
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: smoke-and-tests (22)
- GitHub Check: smoke-and-tests (24.11.x)
- GitHub Check: typecheck
🧰 Additional context used
📓 Path-based instructions (2)
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*: Keep changes focused on one problem or feature and avoid mixing unrelated cleanup into the same change.
Preserve existing repository patterns unless intentionally refactoring them.
Update documentation when setup, commands, or user-facing behavior changes.
Review AI-generated changes for correctness, style consistency, unnecessary changes, and adherence to project architecture before submitting them.
Follow the existing code style in touched files.
Prefer small, readable changes over broad rewrites.
Do not reformat unrelated files.
Keep comments useful and concise.
Provider changes must explicitly identify affected providers, limitations, and follow-up work in the pull request description.
Do not assign or use provider tags; provider tags are controlled by maintainers.
Run the relevant validation checks locally before submitting changes; pull requests must pass CI checks.
Runbun run security:pr-scanbefore submitting a pull request.
Dependency changes require a concrete project benefit, such as fixing a bug, addressing a security issue, or supporting an approved feature.
Do not change the project's language, core runtime, or dependency stack without prior maintainer agreement.
Files:
docs/advanced-setup.md
⚙️ 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:
docs/advanced-setup.md
{README.md,CONTRIBUTING.md,docs/**,.github/pull_request_template.md}
⚙️ CodeRabbit configuration file
{README.md,CONTRIBUTING.md,docs/**,.github/pull_request_template.md}: Review docs for accuracy against current code behavior. Flag security or provider claims that overpromise, stale install commands, missing setup caveats, and instructions that could push users toward unsafe credential handling. Keep purely wording-level suggestions non-blocking.
Files:
docs/advanced-setup.md
🔇 Additional comments (1)
.env.example (1)
431-433: LGTM!
There was a problem hiding this comment.
Smoke issues from main should now be resolved, please rebase on main to fix.
I also found issues that need to be addressed before this is ready.
Findings
-
[P1] Restore the real modules for every process-global test mock
src/services/awaySummary.test.ts:30
Moving these sparsemock.module()registrations to module scope while removing the old teardown leaves them installed for the rest of Bun's shared test process—mock.restore()does not clear the module-mock registry. For example, every later importer of./api/claude.jsgets onlyqueryModelWithoutStreaming, and every later analytics importer aftersrc/utils/diff.test.tsgets onlylogEvent. The same incomplete-cleanup pattern is present in the changed compact, tips, and attribution tests (including the partial cleanup incompact.test.ts). This makes the smoke/full suite order-dependent and can either crash later imports or run them against test doubles. Restore the complete real module namespaces in teardown (and retain the existing shared-mutation lifetime discipline) for each affected mock. -
[P1] Restore the ProviderManager startup-overrides mock before the next suite
src/components/ProviderManager.test.tsx:573
This suite still installs a process-global mock forproviderStartupOverrides, but the change removes its real-module re-registration; meanwhileproviderStartupOverrides.test.tsnow statically imports that module. Runningbun test src/components/ProviderManager.test.tsx src/utils/providerStartupOverrides.test.tscauses the latter to receive the stale mock and fail itsupdateUserSettingsassertion (the test passes in isolation). Re-register the real module during ProviderManager teardown, or keep the consumer's cache-busted import with correct mock cleanup.
|
@jatmn I merged the current
The refreshed PR Checks are also green on Node 22, Node 24, typecheck, and web. Please re-review when convenient. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/services/api/fetchWithProxyRetry.ts (1)
37-42: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winCancel the discarded response body on post-fetch abort.
If the fetcher resolves after caller cancellation, this branch throws while leaving the returned body open. Cancel it without delaying propagation of the original abort reason, and add a focused regression test.
Proposed fix
if (init?.signal?.aborted) { + void response.body?.cancel().catch(() => {}) throw ( init.signal.reason ?? new DOMException('The operation was aborted.', 'AbortError')As per coding guidelines, “Add or update tests when a code change affects behavior.” As per path instructions, review tests for “async cleanup.”
🤖 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 `@src/services/api/fetchWithProxyRetry.ts` around lines 37 - 42, Update the post-fetch abort branch in fetchWithProxyRetry to cancel the resolved response body before throwing, without awaiting or otherwise delaying propagation of init.signal.reason (falling back to the existing AbortError). Add a focused regression test covering caller cancellation after fetch resolution and verify the body’s async cleanup is triggered.Sources: Coding guidelines, Path instructions
🤖 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.
Outside diff comments:
In `@src/services/api/fetchWithProxyRetry.ts`:
- Around line 37-42: Update the post-fetch abort branch in fetchWithProxyRetry
to cancel the resolved response body before throwing, without awaiting or
otherwise delaying propagation of init.signal.reason (falling back to the
existing AbortError). Add a focused regression test covering caller cancellation
after fetch resolution and verify the body’s async cleanup is triggered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 462c49ea-6578-4ffc-96b9-e694f2816729
📒 Files selected for processing (3)
src/services/api/fetchWithProxyRetry.tssrc/services/api/openaiShim.test.tssrc/services/api/openaiShim.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: smoke-and-tests (24.11.x)
- GitHub Check: smoke-and-tests (22)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
TypeScript code in this repository must use strict mode and ESM imports.
**/*.{ts,tsx}: When changing provider behavior, avoid breaking third-party providers and test the exact provider/model path changed when possible.
Runbun run typecheckandbun run typecheck:type-testsfor TypeScript changes.
Run provider tests and provider recommendation tests when changing provider behavior:bun run test:providerandbun run test:provider-recommendation.
Files:
src/services/api/fetchWithProxyRetry.tssrc/services/api/openaiShim.test.tssrc/services/api/openaiShim.ts
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*: Keep changes focused on one problem or feature and avoid mixing unrelated cleanup into the same change.
Preserve existing repository patterns unless intentionally refactoring them.
Update documentation when setup, commands, or user-facing behavior changes.
Review AI-generated changes for correctness, style consistency, unnecessary changes, and adherence to project architecture before submitting them.
Follow the existing code style in touched files.
Prefer small, readable changes over broad rewrites.
Do not reformat unrelated files.
Keep comments useful and concise.
Provider changes must explicitly identify affected providers, limitations, and follow-up work in the pull request description.
Do not assign or use provider tags; provider tags are controlled by maintainers.
Run the relevant validation checks locally before submitting changes; pull requests must pass CI checks.
Runbun run security:pr-scanbefore submitting a pull request.
Dependency changes require a concrete project benefit, such as fixing a bug, addressing a security issue, or supporting an approved feature.
Do not change the project's language, core runtime, or dependency stack without prior maintainer agreement.
Files:
src/services/api/fetchWithProxyRetry.tssrc/services/api/openaiShim.test.tssrc/services/api/openaiShim.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/fetchWithProxyRetry.tssrc/services/api/openaiShim.test.tssrc/services/api/openaiShim.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Add or update tests when a code change affects behavior.
Files:
src/services/api/fetchWithProxyRetry.tssrc/services/api/openaiShim.test.tssrc/services/api/openaiShim.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/fetchWithProxyRetry.tssrc/services/api/openaiShim.test.tssrc/services/api/openaiShim.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/openaiShim.test.ts
🔇 Additional comments (3)
src/services/api/openaiShim.ts (1)
369-426: LGTM!src/services/api/fetchWithProxyRetry.ts (1)
19-36: LGTM!Also applies to: 51-76
src/services/api/openaiShim.test.ts (1)
7145-7168: LGTM!
3 participants
Summary
Implementation
API_TIMEOUT_MSonce per request with a 600000 ms default and safe integer validation.Tests
bun run test:provider— 1214 passed.bun run typecheckbun run typecheck:type-testsbun run buildbun run smokebun run deadcodebun run security:pr-scanbun run checkwas attempted on the current upstream merge; its shared-process full-suite phase reported 44 failures outside this PR's 11-file effective diff. As a representative isolation check,bun test src/utils/reportTask.test.tspassed 41 tests after its two full-suite failures.Risk
fetchWithProxyRetryinterface are unchanged.AbortSignal.any, caller forwarding remains active through response-body consumption to preserve post-header cancellation, then is removed on body completion, error, cancellation, or caller abort. Supported Node and Bun runtimes use native signal composition.Summary by CodeRabbit
API_TIMEOUT_MSas an OpenAI-compatible response-headers deadline for Copilot requests and chat-to-Responses fallback (default: 600000 ms), with safe-positive parsing and a cap at 2147483647..env.exampleand advanced setup docs with the new semantics and validation/capping rules.