refactor(openai-shim): extract request executor helpers - #2011
Conversation
📝 WalkthroughWalkthroughChangesThe OpenAI shim delegates authentication, routing, retries, credential pooling, recovery, and response handling to OpenAI shim executor
Estimated code review effort: 5 (Critical) | ~90 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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/requestExecutor.test.ts`:
- Around line 10-14: Update the “waits for the requested retry delay” test to
use fake timers or an injectable scheduler instead of measuring Date.now()
around a real sleepMs(5) call. Advance the controlled timer by the requested
delay and assert that the sleep resolves only after that advancement, preserving
verification of the exact 5 ms delay without wall-clock timing.
🪄 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: 0ca65f35-312f-4eed-a214-47dbb7fc72c3
📒 Files selected for processing (3)
src/services/api/openaiShim.tssrc/services/api/openaiShim/requestExecutor.test.tssrc/services/api/openaiShim/requestExecutor.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: typecheck
- GitHub Check: smoke-and-tests (22)
- 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.
**/*.{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/openaiShim/requestExecutor.test.tssrc/services/api/openaiShim/requestExecutor.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/openaiShim/requestExecutor.test.tssrc/services/api/openaiShim/requestExecutor.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/requestExecutor.test.tssrc/services/api/openaiShim/requestExecutor.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/openaiShim/requestExecutor.test.tssrc/services/api/openaiShim/requestExecutor.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/requestExecutor.test.tssrc/services/api/openaiShim/requestExecutor.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/requestExecutor.test.ts
🔇 Additional comments (3)
src/services/api/openaiShim/requestExecutor.ts (1)
1-8: LGTM!src/services/api/openaiShim/requestExecutor.test.ts (1)
1-8: LGTM!src/services/api/openaiShim.ts (1)
123-123: LGTM!Also applies to: 4802-4809
5dd9b40 to
29318b1
Compare
0175b59 to
f937279
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
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)
4123-4128: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSuppress
tool_choiceduring Anthropic toolless recovery.The executor sets
omitTools.anthropic, but this builder still copiesparams.tool_choice. The retry therefore sendstool_choicewithouttoolsand can be rejected again.Proposed fix
if (!omitTools.anthropic && params.tools && params.tools.length > 0) { anthropicBody.tools = params.tools } - if (params.tool_choice) { + if (!omitTools.anthropic && params.tool_choice) { anthropicBody.tool_choice = params.tool_choice }Add a focused
/messagestoolless-recovery test. As per coding guidelines, “avoid breaking third-party providers and test the exact provider/model path changed when possible.”🤖 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 4123 - 4128, Update the Anthropic request-building logic around the tools and tool_choice assignments so params.tool_choice is copied only when omitTools.anthropic is false, preventing toolless recovery requests from sending tool_choice without tools. Add a focused /messages test covering the affected Anthropic provider/model recovery path and asserting tool_choice is omitted.Source: Coding guidelines
🤖 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/architecture.test.ts`:
- Around line 8-33: The extractionDeltas baseline is inconsistent with the
intended fully extracted 1,057-line ceiling: the listed reductions total 4,054,
yielding 1,086 from 5,140. Update the architecture guard’s baseline or
extraction reductions so the activeReduction calculation enforces a 1,057-line
ceiling when all modules exist, while preserving the existing module-detection
behavior.
In `@src/services/api/openaiShim/requestExecutor.integration.test.ts`:
- Around line 431-521: Complete environment isolation in both executor test
suites: in src/services/api/openaiShim/requestExecutor.integration.test.ts at
lines 431-521, capture the original HICAP_API_KEY, clear it in beforeEach, and
restore it in afterEach; in src/services/api/openaiShim/requestExecutor.test.ts
at lines 438-527, capture, clear, and restore OPENAI_AZURE_STYLE. Use the
existing environment snapshot and restore patterns in each suite.
In `@src/services/api/openaiShim/requestExecutor.ts`:
- Around line 837-841: Update the HTTP failure classification calls around
responsesFailure and the corresponding request failure path to pass the actual
request URL: use responsesUrl for classifyOpenAIHttpFailure in the responses
flow and requestUrl in the other flow. Preserve the existing status, body, and
hasImages arguments so remote 404 diagnostics use the correct endpoint guidance.
- Around line 929-950: The toolless recovery branch in the request execution
loop must retain the current credential lease instead of selecting another
credential on the next iteration. Update the retry control flow around
shouldAttemptLocalToollessRetry and credentialPool.next() to reuse the existing
credential, matching the tool-stream recovery behavior, and add coverage with
credentials having unequal model access against the affected provider/model
path.
- Around line 794-801: The GitHub 429 retry branch in the request executor
bypasses credential cooldown and failure reporting. Before sleeping and
continuing, update the isGithub 429 handling to call
credentialPool.reportFailure for the active credential, then add a focused test
covering the GitHub provider/model path that verifies the rate-limited
credential is rotated or cooled down before retry.
---
Outside diff comments:
In `@src/services/api/openaiShim.ts`:
- Around line 4123-4128: Update the Anthropic request-building logic around the
tools and tool_choice assignments so params.tool_choice is copied only when
omitTools.anthropic is false, preventing toolless recovery requests from sending
tool_choice without tools. Add a focused /messages test covering the affected
Anthropic provider/model recovery path and asserting tool_choice is omitted.
🪄 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: 14986c9c-726e-479c-9f03-99ddc80c26af
📒 Files selected for processing (7)
package.jsonsrc/services/api/openaiShim.test.tssrc/services/api/openaiShim.tssrc/services/api/openaiShim/architecture.test.tssrc/services/api/openaiShim/requestExecutor.integration.test.tssrc/services/api/openaiShim/requestExecutor.test.tssrc/services/api/openaiShim/requestExecutor.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: smoke-and-tests (22)
- GitHub Check: smoke-and-tests (24.11.x)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{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/openaiShim/architecture.test.tssrc/services/api/openaiShim/requestExecutor.tssrc/services/api/openaiShim/requestExecutor.test.tssrc/services/api/openaiShim/requestExecutor.integration.test.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/openaiShim/architecture.test.tspackage.jsonsrc/services/api/openaiShim/requestExecutor.tssrc/services/api/openaiShim/requestExecutor.test.tssrc/services/api/openaiShim/requestExecutor.integration.test.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/openaiShim/architecture.test.tspackage.jsonsrc/services/api/openaiShim/requestExecutor.tssrc/services/api/openaiShim/requestExecutor.test.tssrc/services/api/openaiShim/requestExecutor.integration.test.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/openaiShim/architecture.test.tssrc/services/api/openaiShim/requestExecutor.tssrc/services/api/openaiShim/requestExecutor.test.tssrc/services/api/openaiShim/requestExecutor.integration.test.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/openaiShim/architecture.test.tssrc/services/api/openaiShim/requestExecutor.tssrc/services/api/openaiShim/requestExecutor.test.tssrc/services/api/openaiShim/requestExecutor.integration.test.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/architecture.test.tssrc/services/api/openaiShim/requestExecutor.test.tssrc/services/api/openaiShim/requestExecutor.integration.test.tssrc/services/api/openaiShim.test.ts
{bin/**,scripts/**,package.json,src/setup.ts,src/main.tsx,src/entrypoints/**}
⚙️ CodeRabbit configuration file
{bin/**,scripts/**,package.json,src/setup.ts,src/main.tsx,src/entrypoints/**}: Review install, launcher, build, packaging, startup, and entrypoint changes for cross-platform compatibility, tracked-source rewrites, env/config precedence, and release safety. Block on changes that can break Windows/macOS/Linux startup or publish unexpected artifacts.
Files:
package.json
🔇 Additional comments (7)
src/services/api/openaiShim/requestExecutor.ts (1)
1-793: LGTM!Also applies to: 802-836, 842-859, 865-928, 951-1007
src/services/api/openaiShim.ts (1)
81-85: LGTM!Also applies to: 1744-1744, 1813-1818, 1960-1960, 2349-2350, 2526-2527, 3445-3446, 3571-3585, 3697-3697, 3802-3809, 3826-3826, 3863-3884, 3915-3915, 4023-4027, 4078-4078, 4256-4256, 4274-4276, 4318-4334
src/services/api/openaiShim/requestExecutor.test.ts (1)
1-18: LGTM!Also applies to: 60-437, 528-758, 791-1913
src/services/api/openaiShim/requestExecutor.integration.test.ts (1)
1-15: LGTM!Also applies to: 58-430, 522-4082, 4133-4733
src/services/api/openaiShim.test.ts (1)
527-6609: LGTM!src/services/api/openaiShim/architecture.test.ts (1)
1-7: LGTM!package.json (1)
69-69: LGTM!
f937279 to
99d6966
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
src/services/api/openaiShim/requestExecutor.ts (1)
971-993: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFormatting-only retries must pin the credential lease — the toolless branch and its test coverage both miss it. The
tool_streamrecovery pins the lease (Line 1016) but thetool_call_incompatibletoolless recovery does not, so the next iteration draws a fresh pool credential for a retry that only changed the payload shape.
src/services/api/openaiShim/requestExecutor.ts#L971-L993: setretryCredentialLease = credentialLeaseafterrefreshSerializedBody(), matching thetool_streambranch.src/services/api/openaiShim/requestExecutor.test.ts#L1291-L1338: add a sibling test withOPENAI_API_KEYS='key-a,key-b'that drives atool_call_incompatible400 and asserts both attempts sendBearer key-a.🤖 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/requestExecutor.ts` around lines 971 - 993, The toolless tool_call_incompatible retry must retain the current credential lease, with regression coverage. In src/services/api/openaiShim/requestExecutor.ts lines 971-993, assign retryCredentialLease = credentialLease immediately after refreshSerializedBody(), matching the tool_stream recovery. In src/services/api/openaiShim/requestExecutor.test.ts lines 1291-1338, add sibling coverage using OPENAI_API_KEYS='key-a,key-b' that triggers a tool_call_incompatible 400 and verifies both attempts use Bearer key-a.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/architecture.test.ts`:
- Around line 8-33: Update the extraction ceiling configuration in the
`extractionDeltas` test so that when all ten representative modules exist, the
calculated façade limit is exactly 1,057 lines. Reconcile the baseline and/or
individual reduction values used by the `activeReduction` calculation,
preserving the module-detection behavior and the existing `facadeLines`
assertion.
In `@src/services/api/openaiShim/requestExecutor.integration.test.ts`:
- Around line 3030-3049: Update the test around createOpenAIShimClient and the
globalThis.fetch mock so it records the parsed request body, then assert stream
and absent stream_options after client.beta.messages.create resolves; ensure the
test also verifies the mock was invoked, preserving focused coverage if request
routing changes.
In `@src/services/api/openaiShim/requestExecutor.test.ts`:
- Around line 1291-1338: Add a focused mirror test alongside “Shim retries a
tool_stream rejection with the same pooled credential (`#1950`)” covering the
toolless tool_call_incompatible retry path. Configure pooled credentials, make
the first request return the tool_call_incompatible rejection and the retry
succeed, record Authorization headers, and assert both attempts reuse the same
credential; follow the existing test setup and symbols without changing
production behavior.
- Around line 1913-1924: Remove or retitle the “JSON fallback regression tests
(`#1749`)” section header above the retry-hint test so it accurately describes the
nearby formatRetryAfterHint and sleepMs helper tests. Keep the JSON fallback
coverage referenced only where those integration tests now reside.
In `@src/services/api/openaiShim/requestExecutor.ts`:
- Around line 914-931: Update the successful Copilot 401 refresh branch in the
request execution flow to assign the current credentialLease to
retryCredentialLease before continuing, matching the existing tool_stream
recovery behavior. Preserve the existing token refresh and retry conditions so
the next iteration reuses the lease instead of acquiring a new one.
- Around line 85-86: Confirm whether fetchWithProxyRetry is used anywhere beyond
the executor contract and its destructuring. If no call sites exist and
proxy-retry behavior is not required, remove fetchWithProxyRetry from the
executor type and related destructuring/wiring, leaving fetchRequest as the
transport path; otherwise restore its usage so proxy retries remain effective.
- Around line 237-241: Update requestExecutor credential and authentication
resolution to use the injected requestProcessEnv consistently instead of
process.env, including resolveRouteCredentialValue and the reads near the
referenced credential/authentication paths. Preserve the existing
isAzureStyleBaseUrl behavior and ensure providerOverride requests use the same
environment source throughout; if any direct process.env reads must remain for
compatibility with pre-extraction behavior, document that explicitly with a
short comment.
---
Duplicate comments:
In `@src/services/api/openaiShim/requestExecutor.ts`:
- Around line 971-993: The toolless tool_call_incompatible retry must retain the
current credential lease, with regression coverage. In
src/services/api/openaiShim/requestExecutor.ts lines 971-993, assign
retryCredentialLease = credentialLease immediately after
refreshSerializedBody(), matching the tool_stream recovery. In
src/services/api/openaiShim/requestExecutor.test.ts lines 1291-1338, add sibling
coverage using OPENAI_API_KEYS='key-a,key-b' that triggers a
tool_call_incompatible 400 and verifies both attempts use Bearer key-a.
🪄 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: 8a9fe75a-90b0-48e3-9976-08d77b7e5a5f
📒 Files selected for processing (6)
src/services/api/openaiShim.test.tssrc/services/api/openaiShim.tssrc/services/api/openaiShim/architecture.test.tssrc/services/api/openaiShim/requestExecutor.integration.test.tssrc/services/api/openaiShim/requestExecutor.test.tssrc/services/api/openaiShim/requestExecutor.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: smoke-and-tests (24.11.x)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
TypeScript code in this repository must use strict mode and ESM imports.
**/*.{ts,tsx}: Add or update tests when a TypeScript or TSX change affects behavior.
Run the relevant TypeScript validation checks for changed code, includingbun run typecheckand, when applicable,bun run typecheck:type-tests.
Files:
src/services/api/openaiShim/architecture.test.tssrc/services/api/openaiShim.tssrc/services/api/openaiShim/requestExecutor.test.tssrc/services/api/openaiShim/requestExecutor.tssrc/services/api/openaiShim.test.tssrc/services/api/openaiShim/requestExecutor.integration.test.ts
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*: Preserve existing repository patterns unless intentionally refactoring them.
Keep changes small, readable, and focused; avoid broad rewrites or unrelated cleanup.
Do not reformat unrelated files, and keep comments useful and concise.
Update documentation when setup, commands, or user-facing behavior changes.
Review AI-generated changes for correctness, style consistency, unnecessary noise, and adherence to project architecture before submitting them.
Provider changes must follow the documented integration patterns indocs/integrations/overview.mdand the focused guides underdocs/integrations/how-to/.
When changing provider behavior, avoid breaking third-party providers and test the exact provider/model path changed when possible.
Provider pull requests must explicitly identify affected providers, limitations, and follow-up work.
Run the narrowest meaningful validation command for the touched area, and ensure relevant CI checks pass before merging.
Usebun installto install dependencies and the repository's Bun scripts for building, testing, smoke testing, and development.
Dependency changes require a concrete project benefit such as a bug fix, security issue, or approved feature; preference alone is insufficient.
Do not change the project's language, core runtime, or dependency stack, or introduce a new runtime, without prior maintainer agreement.
Keep each pull request focused on one issue or clearly scoped improvement and avoid bundling unrelated fixes, features, or refactors.
Files:
src/services/api/openaiShim/architecture.test.tssrc/services/api/openaiShim.tssrc/services/api/openaiShim/requestExecutor.test.tssrc/services/api/openaiShim/requestExecutor.tssrc/services/api/openaiShim.test.tssrc/services/api/openaiShim/requestExecutor.integration.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:
src/services/api/openaiShim/architecture.test.tssrc/services/api/openaiShim.tssrc/services/api/openaiShim/requestExecutor.test.tssrc/services/api/openaiShim/requestExecutor.tssrc/services/api/openaiShim.test.tssrc/services/api/openaiShim/requestExecutor.integration.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/openaiShim/architecture.test.tssrc/services/api/openaiShim.tssrc/services/api/openaiShim/requestExecutor.test.tssrc/services/api/openaiShim/requestExecutor.tssrc/services/api/openaiShim.test.tssrc/services/api/openaiShim/requestExecutor.integration.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/architecture.test.tssrc/services/api/openaiShim/requestExecutor.test.tssrc/services/api/openaiShim.test.tssrc/services/api/openaiShim/requestExecutor.integration.test.ts
🔇 Additional comments (6)
src/services/api/openaiShim.test.ts (1)
8362-8362: LGTM!Also applies to: 8474-8494
src/services/api/openaiShim.ts (2)
101-101: LGTM!
1467-1476: LGTM!src/services/api/openaiShim/requestExecutor.ts (2)
437-560: LGTM!
687-834: LGTM!src/services/api/openaiShim/requestExecutor.integration.test.ts (1)
432-525: LGTM!
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)
src/services/api/openaiShim/requestExecutor.ts (1)
719-731: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
isAbortErrordetection — extract a helper.The abort-error detection (signal-aborted check +
DOMException/namefallback) is now implemented twice with slightly different structuring: once at Line 719-731 and again, newly added, at Line 873-883 for the Copilot/responsesretry path. Any future tweak (e.g., additional abort signal shapes) now needs two edits kept in sync.♻️ Proposed extraction
+function isAbortLikeError(error: unknown): boolean { + return ( + (typeof DOMException !== 'undefined' && + error instanceof DOMException && + error.name === 'AbortError') || + (typeof error === 'object' && + error !== null && + 'name' in error && + error.name === 'AbortError') + ) +}Then at Line 719-728:
- const isAbortError = - options?.signal?.aborted === true || - (typeof DOMException !== 'undefined' && - error instanceof DOMException && - error.name === 'AbortError') || - (typeof error === 'object' && - error !== null && - 'name' in error && - error.name === 'AbortError') + const isAbortError = + options?.signal?.aborted === true || isAbortLikeError(error)And at Line 873-883:
- if ( - (typeof DOMException !== 'undefined' && - error instanceof DOMException && - error.name === 'AbortError') || - (typeof error === 'object' && - error !== null && - 'name' in error && - error.name === 'AbortError') - ) { + if (isAbortLikeError(error)) { throw error }Also applies to: 873-883
🤖 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/requestExecutor.ts` around lines 719 - 731, Extract the shared abort-error detection used by the request execution and Copilot /responses retry paths into a single helper, preserving the existing signal-aborted and DOMException/name-based checks. Replace both inline isAbortError implementations with calls to that helper so future detection changes require only one update.
🤖 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/requestExecutor.ts`:
- Around line 322-326: Add a test in the existing auth-header test suite
covering the guard in the request executor: set OPENAI_AUTH_HEADER_VALUE to a
value containing \r or \n, invoke the relevant request/auth setup path, and
assert it throws the expected CR/LF validation error. Keep the existing
happy-path tests unchanged.
---
Outside diff comments:
In `@src/services/api/openaiShim/requestExecutor.ts`:
- Around line 719-731: Extract the shared abort-error detection used by the
request execution and Copilot /responses retry paths into a single helper,
preserving the existing signal-aborted and DOMException/name-based checks.
Replace both inline isAbortError implementations with calls to that helper so
future detection changes require only one update.
🪄 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: 12b16a3b-13c8-43da-b41e-feb6d9351def
📒 Files selected for processing (2)
src/services/api/openaiShim.tssrc/services/api/openaiShim/requestExecutor.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: smoke-and-tests (22)
- GitHub Check: smoke-and-tests (24.11.x)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
TypeScript code in this repository must use strict mode and ESM imports.
**/*.{ts,tsx}: Add or update tests when a TypeScript or TSX change affects behavior.
Run the relevant TypeScript validation checks for changed code, includingbun run typecheckand, when applicable,bun run typecheck:type-tests.
Files:
src/services/api/openaiShim.tssrc/services/api/openaiShim/requestExecutor.ts
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*: Preserve existing repository patterns unless intentionally refactoring them.
Keep changes small, readable, and focused; avoid broad rewrites or unrelated cleanup.
Do not reformat unrelated files, and keep comments useful and concise.
Update documentation when setup, commands, or user-facing behavior changes.
Review AI-generated changes for correctness, style consistency, unnecessary noise, and adherence to project architecture before submitting them.
Provider changes must follow the documented integration patterns indocs/integrations/overview.mdand the focused guides underdocs/integrations/how-to/.
When changing provider behavior, avoid breaking third-party providers and test the exact provider/model path changed when possible.
Provider pull requests must explicitly identify affected providers, limitations, and follow-up work.
Run the narrowest meaningful validation command for the touched area, and ensure relevant CI checks pass before merging.
Usebun installto install dependencies and the repository's Bun scripts for building, testing, smoke testing, and development.
Dependency changes require a concrete project benefit such as a bug fix, security issue, or approved feature; preference alone is insufficient.
Do not change the project's language, core runtime, or dependency stack, or introduce a new runtime, without prior maintainer agreement.
Keep each pull request focused on one issue or clearly scoped improvement and avoid bundling unrelated fixes, features, or refactors.
Files:
src/services/api/openaiShim.tssrc/services/api/openaiShim/requestExecutor.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.tssrc/services/api/openaiShim/requestExecutor.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.tssrc/services/api/openaiShim/requestExecutor.ts
🔇 Additional comments (3)
src/services/api/openaiShim/requestExecutor.ts (2)
829-844: Previously-flagged retry/credential issues confirmed resolved.GitHub 429 now cools down the credential (
credentialPool.reportFailureat Line 835-841) before rotating,classifyOpenAIHttpFailurecalls now passurl(Line 900, 924), the toolless retry reuses the lease (retryCredentialLease = credentialLeaseat Line 1009), and the Copilot 401 refresh path now setsretryCredentialLease = credentialLease(Line 947) instead of silently dropping the lease. All match the previously requested fixes.Also applies to: 897-902, 921-926, 938-953, 1009-1009
607-619: 🔒 Security & PrivacyRedaction asymmetry between transport and HTTP error paths.
throwClassifiedTransportErrornow runs the failure message through three redaction layers (redactEncodedSecretSubstringsForDisplay→redactSecretSubstringsForDisplay→redactSecretValueForDisplay, Line 607-619).throwClassifiedHttpError, right below it, still interpolates the raw providererrorBodydirectly into the thrown message (Line 672) with no redaction at all. If a gateway ever echoes request headers/credentials in an error body, this path leaks them unredacted while the sibling path is hardened. Worth confirming this is intentional (e.g., error bodies are assumed never to contain secrets) or applying the same redaction chain here.Also applies to: 640-677
src/services/api/openaiShim.ts (1)
101-101: LGTM!Also applies to: 1467-1476
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/requestExecutor.test.ts`:
- Around line 687-729: Update the test around createOpenAIShimClient to use
bun:test fake timers instead of the real 20ms delay, advancing timers far enough
to let the first request enter its sleepMs backoff before starting the second
request. Integrate fake-timer setup and restoration with the file’s existing
beforeEach/afterEach lifecycle, preserve the current authorization and rejection
assertions, and ensure pending asynchronous work is settled before cleanup.
🪄 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: 2655a1fe-256d-4415-a162-7f7533ca4c5d
📒 Files selected for processing (2)
src/services/api/openaiShim/requestExecutor.test.tssrc/services/api/openaiShim/requestExecutor.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
TypeScript code in this repository must use strict mode and ESM imports.
**/*.{ts,tsx}: Add or update tests when a TypeScript or TSX change affects behavior.
Run the relevant TypeScript validation checks for changed code, includingbun run typecheckand, when applicable,bun run typecheck:type-tests.
Files:
src/services/api/openaiShim/requestExecutor.tssrc/services/api/openaiShim/requestExecutor.test.ts
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*: Preserve existing repository patterns unless intentionally refactoring them.
Keep changes small, readable, and focused; avoid broad rewrites or unrelated cleanup.
Do not reformat unrelated files, and keep comments useful and concise.
Update documentation when setup, commands, or user-facing behavior changes.
Review AI-generated changes for correctness, style consistency, unnecessary noise, and adherence to project architecture before submitting them.
Provider changes must follow the documented integration patterns indocs/integrations/overview.mdand the focused guides underdocs/integrations/how-to/.
When changing provider behavior, avoid breaking third-party providers and test the exact provider/model path changed when possible.
Provider pull requests must explicitly identify affected providers, limitations, and follow-up work.
Run the narrowest meaningful validation command for the touched area, and ensure relevant CI checks pass before merging.
Usebun installto install dependencies and the repository's Bun scripts for building, testing, smoke testing, and development.
Dependency changes require a concrete project benefit such as a bug fix, security issue, or approved feature; preference alone is insufficient.
Do not change the project's language, core runtime, or dependency stack, or introduce a new runtime, without prior maintainer agreement.
Keep each pull request focused on one issue or clearly scoped improvement and avoid bundling unrelated fixes, features, or refactors.
Files:
src/services/api/openaiShim/requestExecutor.tssrc/services/api/openaiShim/requestExecutor.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:
src/services/api/openaiShim/requestExecutor.tssrc/services/api/openaiShim/requestExecutor.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/openaiShim/requestExecutor.tssrc/services/api/openaiShim/requestExecutor.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/requestExecutor.test.ts
🔇 Additional comments (1)
src/services/api/openaiShim/requestExecutor.ts (1)
854-904: 🎯 Functional CorrectnessNo issue here.
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/credentialPool.ts`:
- Around line 106-114: Add a concise inline comment in reportFailure immediately
before the kind-based generation check, documenting that auth failures
intentionally bypass generation validation while other failure kinds require it;
leave reportSuccess and the existing control flow unchanged.
🪄 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: d4a6aa44-4a8e-4c3e-aa8e-6754e6806b80
📒 Files selected for processing (3)
src/services/api/credentialPool.test.tssrc/services/api/credentialPool.tssrc/services/api/openaiShim/requestExecutor.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
TypeScript code in this repository must use strict mode and ESM imports.
**/*.{ts,tsx}: Add or update tests when a TypeScript or TSX change affects behavior.
Run the relevant TypeScript validation checks for changed code, includingbun run typecheckand, when applicable,bun run typecheck:type-tests.
Files:
src/services/api/credentialPool.test.tssrc/services/api/credentialPool.tssrc/services/api/openaiShim/requestExecutor.test.ts
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*: Preserve existing repository patterns unless intentionally refactoring them.
Keep changes small, readable, and focused; avoid broad rewrites or unrelated cleanup.
Do not reformat unrelated files, and keep comments useful and concise.
Update documentation when setup, commands, or user-facing behavior changes.
Review AI-generated changes for correctness, style consistency, unnecessary noise, and adherence to project architecture before submitting them.
Provider changes must follow the documented integration patterns indocs/integrations/overview.mdand the focused guides underdocs/integrations/how-to/.
When changing provider behavior, avoid breaking third-party providers and test the exact provider/model path changed when possible.
Provider pull requests must explicitly identify affected providers, limitations, and follow-up work.
Run the narrowest meaningful validation command for the touched area, and ensure relevant CI checks pass before merging.
Usebun installto install dependencies and the repository's Bun scripts for building, testing, smoke testing, and development.
Dependency changes require a concrete project benefit such as a bug fix, security issue, or approved feature; preference alone is insufficient.
Do not change the project's language, core runtime, or dependency stack, or introduce a new runtime, without prior maintainer agreement.
Keep each pull request focused on one issue or clearly scoped improvement and avoid bundling unrelated fixes, features, or refactors.
Files:
src/services/api/credentialPool.test.tssrc/services/api/credentialPool.tssrc/services/api/openaiShim/requestExecutor.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:
src/services/api/credentialPool.test.tssrc/services/api/credentialPool.tssrc/services/api/openaiShim/requestExecutor.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/credentialPool.test.tssrc/services/api/credentialPool.tssrc/services/api/openaiShim/requestExecutor.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/credentialPool.test.tssrc/services/api/openaiShim/requestExecutor.test.ts
🔇 Additional comments (5)
src/services/api/credentialPool.ts (2)
6-14: LGTM!Also applies to: 28-28, 50-50
72-95: LGTM!src/services/api/openaiShim/requestExecutor.test.ts (2)
2-2: LGTM!Also applies to: 485-485
694-697: Fake-timer conversion correctly resolves the prior real-clock race.This replaces the previous real 20ms-wait race window (flagged in an earlier review) with deterministic fake timers gated on
jest.getTimerCount()and a body-read signal, and it correctly exercises the "final 429 when all pooled keys are cooling" behavior. Traced againstrequestExecutor.ts'shasAvailableCredential()gating — assertions line up.Also applies to: 710-720, 731-751
src/services/api/credentialPool.test.ts (1)
48-75: LGTM!Also applies to: 87-87
|
@kevincodex1 LGTM |
2 participants
Summary
Extracts OpenAI-compatible request execution into
openaiShim/requestExecutor.ts. The executor owns credential pools and rotation, authentication retries, token refresh, network and HTTP diagnostics, local fallback, tool retries, and per-attempt request state.The architecture guard now requires all ten extracted modules and enforces the verified 1,582-line façade budget, preventing a removed module from being folded back into
openaiShim.tsunnoticed.GitHub pooled-key retries now return the final 429 once every key is cooling down instead of immediately retrying a cooled credential.
Validation
bun test src/services/api/openaiShim/requestExecutor.test.ts src/services/api/openaiShim/requestExecutor.integration.test.ts src/services/api/openaiShim/architecture.test.ts— 151 passedbun run typecheckbun run buildbun run security:pr-scanSummary by CodeRabbit