Repository navigation
refactor(openai-shim): extract provider compatibility - #2003
Conversation
📝 WalkthroughWalkthroughProvider compatibility logic was extracted into ChangesProvider compatibility extraction
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ 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}
d097af8 to
193302a
Compare
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.test.ts (1)
8008-8008: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winBlocking: restore the NVIDIA streaming
tool_streamregression test.This behavior was not moved into
providerCompatibility.test.ts, so deleting the test is unrelated cleanup and leaves NVIDIA streaming requests unprotected. Restore it here or relocate it into the focused suite.As per coding guidelines, “Keep changes focused on one problem or feature and avoid mixing unrelated cleanup into the same change.”
🤖 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` at line 8008, Restore the deleted regression test for NVIDIA NIM Z.AI GLM streaming requests with tools, or move it unchanged into providerCompatibility.test.ts. Ensure it continues asserting that streaming requests do not send the tool_stream parameter, using the existing test setup and symbols around the test.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/openaiShim.test.ts`:
- Line 8008: Restore the deleted regression test for NVIDIA NIM Z.AI GLM
streaming requests with tools, or move it unchanged into
providerCompatibility.test.ts. Ensure it continues asserting that streaming
requests do not send the tool_stream parameter, using the existing test setup
and symbols around the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9541a0b8-95b6-468b-8e52-a538262661dc
📒 Files selected for processing (4)
src/services/api/openaiShim.test.tssrc/services/api/openaiShim.tssrc/services/api/openaiShim/providerCompatibility.test.tssrc/services/api/openaiShim/providerCompatibility.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/openaiShim/providerCompatibility.tssrc/services/api/openaiShim/providerCompatibility.test.tssrc/services/api/openaiShim.tssrc/services/api/openaiShim.test.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/providerCompatibility.tssrc/services/api/openaiShim/providerCompatibility.test.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:
src/services/api/openaiShim/providerCompatibility.tssrc/services/api/openaiShim/providerCompatibility.test.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/openaiShim/providerCompatibility.tssrc/services/api/openaiShim/providerCompatibility.test.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/openaiShim/providerCompatibility.tssrc/services/api/openaiShim/providerCompatibility.test.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/providerCompatibility.test.tssrc/services/api/openaiShim.test.ts
🔇 Additional comments (4)
src/services/api/openaiShim/providerCompatibility.ts (1)
1-148: LGTM!src/services/api/openaiShim.ts (1)
262-290: LGTM!src/services/api/openaiShim/providerCompatibility.test.ts (1)
1-565: LGTM!src/services/api/openaiShim.test.ts (1)
12-12: LGTM!Also applies to: 524-524, 1014-1014, 2451-2451, 2814-2814, 4000-4022, 4063-4070, 6939-6939, 7436-7436
193302a to
9d49d04
Compare
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.test.ts (1)
7940-7940: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winRestore the NVIDIA NIM
tool_streamregression test.This deletion contradicts the PR objective that the regression remains in the monolith suite, and the new focused suite never asserts that streaming tool calls omit
tool_stream.As per coding guidelines, “test the exact provider/model path changed when possible.” As per path instructions, “Block when risky runtime changes lack focused regression coverage.”
🤖 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` at line 7940, Restore the deleted regression test in the monolith suite for the NVIDIA NIM Z.AI GLM streaming request with tools, centered on the existing test title and provider/model path. Ensure it verifies that streaming tool calls omit the tool_stream parameter, preserving coverage for regression `#1950`.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/openaiShim.test.ts`:
- Line 7940: Restore the deleted regression test in the monolith suite for the
NVIDIA NIM Z.AI GLM streaming request with tools, centered on the existing test
title and provider/model path. Ensure it verifies that streaming tool calls omit
the tool_stream parameter, preserving coverage for regression `#1950`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ce4a1b48-df2d-40dd-a2b1-8e2d51464713
📒 Files selected for processing (4)
src/services/api/openaiShim.test.tssrc/services/api/openaiShim.tssrc/services/api/openaiShim/providerCompatibility.test.tssrc/services/api/openaiShim/providerCompatibility.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/openaiShim/providerCompatibility.test.tssrc/services/api/openaiShim.tssrc/services/api/openaiShim/providerCompatibility.tssrc/services/api/openaiShim.test.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/providerCompatibility.test.tssrc/services/api/openaiShim.tssrc/services/api/openaiShim/providerCompatibility.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:
src/services/api/openaiShim/providerCompatibility.test.tssrc/services/api/openaiShim.tssrc/services/api/openaiShim/providerCompatibility.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/openaiShim/providerCompatibility.test.tssrc/services/api/openaiShim.tssrc/services/api/openaiShim/providerCompatibility.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/openaiShim/providerCompatibility.test.tssrc/services/api/openaiShim.tssrc/services/api/openaiShim/providerCompatibility.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/providerCompatibility.test.tssrc/services/api/openaiShim.test.ts
🔇 Additional comments (4)
src/services/api/openaiShim/providerCompatibility.ts (1)
1-148: LGTM!src/services/api/openaiShim.ts (1)
262-290: LGTM!src/services/api/openaiShim/providerCompatibility.test.ts (1)
1-607: LGTM!src/services/api/openaiShim.test.ts (1)
12-12: LGTM!Also applies to: 524-524, 1014-1014, 2450-2451, 2747-2836, 3932-4002, 6870-6872, 7368-7368
6b53f6f to
d062d8f
Compare
f867e09 to
72bf8a9
Compare
72bf8a9 to
d6e9cbf
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 388-401: Move the providerCompatibility import block and the
hasMistralApiHost re-export to the top-level import/export section of
openaiShim.ts, alongside the file’s other module imports. Preserve all imported
symbols and existing behavior; only adjust their placement for navigability.
In `@src/services/api/openaiShim/providerCompatibility.ts`:
- Around line 124-148: Update maybeSetNvidiaNimChatTemplateThinking so
reasoningRequestPlan.thinkingType === 'disabled' always returns before modifying
chat_template_kwargs, regardless of reasoningEffort. Preserve the existing
enablement behavior for enabled thinking and the current skip behavior when no
thinking type or reasoning effort is provided.
🪄 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: c86e06f1-d66a-4c7b-a6d1-d6d7f99f5678
📒 Files selected for processing (4)
src/services/api/openaiShim.test.tssrc/services/api/openaiShim.tssrc/services/api/openaiShim/providerCompatibility.test.tssrc/services/api/openaiShim/providerCompatibility.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 (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
TypeScript code in this repository must use strict mode and ESM imports.
**/*.{ts,tsx}: Follow the existing code style and architectural patterns in touched TypeScript and TSX files.
Add or update tests when TypeScript or TSX changes affect behavior.
Review AI-generated TypeScript and TSX changes for correctness beyond compilation, consistency with repository architecture and style, unnecessary generated noise, and subtle bugs before submission.
Files:
src/services/api/openaiShim.tssrc/services/api/openaiShim/providerCompatibility.tssrc/services/api/openaiShim/providerCompatibility.test.tssrc/services/api/openaiShim.test.ts
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*: Keep pull requests focused on one problem or feature; do not mix unrelated cleanup, fixes, features, or refactors into the same change.
Preserve existing repository patterns unless intentionally refactoring them, and prefer small, readable changes over broad rewrites.
Do not reformat unrelated files, and keep comments useful and concise.
Update documentation when setup, commands, or user-facing behavior changes.
When changing provider behavior, avoid breaking third-party providers, test the exact provider/model path changed when possible, explicitly identify affected providers, and document limitations or follow-up work.
Do not assign or use provider tags; provider tags are controlled and applied by maintainers.
Run the relevant validation checks locally before submitting; CI-required checks includebun run check,bun run test:full, provider tests when applicable, typechecks, andbun run security:pr-scan. Web changes additionally requirebun run web:typecheckandbun run web:build.
Dependency changes must have a concrete project benefit, such as fixing a bug, addressing a security issue, or supporting an approved feature; preference alone is insufficient.
Do not change the project's language, core runtime, dependency stack, or significantly restructure dependencies without prior maintainer agreement.
Before implementing a new feature or other non-trivial change, open an issue to establish scope and alignment with the project roadmap.
Files:
src/services/api/openaiShim.tssrc/services/api/openaiShim/providerCompatibility.tssrc/services/api/openaiShim/providerCompatibility.test.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:
src/services/api/openaiShim.tssrc/services/api/openaiShim/providerCompatibility.tssrc/services/api/openaiShim/providerCompatibility.test.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/openaiShim.tssrc/services/api/openaiShim/providerCompatibility.tssrc/services/api/openaiShim/providerCompatibility.test.tssrc/services/api/openaiShim.test.ts
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Run focused tests for changed behavior and ensure provider-specific changes include the relevant provider tests.
Files:
src/services/api/openaiShim/providerCompatibility.test.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/providerCompatibility.test.tssrc/services/api/openaiShim.test.ts
🔇 Additional comments (4)
src/services/api/openaiShim/providerCompatibility.ts (1)
1-123: LGTM!src/services/api/openaiShim.ts (1)
403-416: LGTM!src/services/api/openaiShim/providerCompatibility.test.ts (1)
1-610: LGTM!src/services/api/openaiShim.test.ts (1)
585-12050: LGTM!
2 participants
Summary
Extracts provider compatibility policy from
src/services/api/openaiShim.tsintosrc/services/api/openaiShim/providerCompatibility.ts.The module owns provider-host recognition, Anthropic/header filtering, NVIDIA NIM reasoning kwargs, GitHub-mode detection, and Gemini thought-signature handling. The façade delegates those decisions while retaining request orchestration.
All branches share stable source and test extraction seams so adjacent edits remain disjoint after any number of earlier PRs merge.
Independent merge
Gitlawb/openclaude:mainat0effa0f42b6dbc6a4800e19f4b2d8d588269906b.jatmn/openclaude:de-mono1-provider-compat.f867e09f36b09f678e3e8ca9b6846856e04e8719.Source-file budget
src/services/api/openaiShim.ts: +59 / -150 = 209 changed lines, below the 1,500-line source-file cap. Test changes are excluded.Test migration
src/services/api/openaiShim.test.ts: +686 / -794; 15 provider-compatibility test bodies are removed from the monolith.src/services/api/openaiShim/providerCompatibility.test.ts: 606 lines / 13 focused tests.Validation
git diff --check: passed.bun run typecheck: passed.bun run typecheck:type-tests: passed.bun run build: passed.bun run check: build, smoke, bundle, and dead-code checks passed; 7,184 passed / 2 skipped, with only the same 10 pre-existing PowerShell governance failures on Linux.Prepared according to
CONTRIBUTING.mdandAGENTS.md.Summary by CodeRabbit