fix(nvidia-nim): enable reasoning template kwargs - #1893
Conversation
📝 WalkthroughWalkthroughAdds NVIDIA NIM request shaping in the OpenAI shim and updates provider profile logic to detect concrete OpenAI-compatible routes, preserve NVIDIA-specific env, and prefer env-based startup selection over stale persisted profiles. ChangesNVIDIA NIM shim request shaping
Provider profile route selection
Estimated code review effort: 4 (Complex) | ~45 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)
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: 4
🤖 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 8028-8034: The current assertions in openaiShim.test.ts only
verify the enabled-thinking path and miss the NVIDIA NIM disabled-state
regression in openaiShim.ts. Add a new test case around the same
request-building logic that passes thinking as disabled, or omits both thinking
and reasoningEffort, with an NVIDIA NIM base URL, and assert that
chat_template_kwargs is not force-populated with thinking/enabled flags. Reuse
the existing requestBody assertions in the openai shim test around the request
construction path so the new case guards the gating logic in openaiShim.ts from
re-enabling thinking unintentionally.
In `@src/services/api/openaiShim.ts`:
- Around line 3781-3787: The NVIDIA NIM gating logic is duplicated in both the
deepseek_compatible and zai_compatible branches, so extract the shared
conditional into a small helper such as maybeApplyNvidiaNimThinkingKwargs using
setNvidiaNimChatTemplateThinking, hasNvidiaNimApiHost, request.baseUrl, and
reasoningRequestPlan; then call that helper from both branches so the
disabled-state behavior is fixed in one place and the duplicate block is
removed.
In `@src/utils/providerProfile.test.ts`:
- Around line 510-536: Add a regression test for the explicit opt-out path in
buildStartupEnvFromProfile. In providerProfile.test.ts, extend the coverage
around buildStartupEnvFromProfile and resolveActiveRouteIdFromEnv to verify that
when processEnv includes CLAUDE_CODE_USE_OPENAI set to "0" alongside concrete
OPENAI_* values, the env preserves the opt-out and does not switch to the OpenAI
route. Use the existing test patterns around buildStartupEnvFromProfile,
profile(), and resolveActiveRouteIdFromEnv to pin the precedence behavior and
prevent regressions in providerProfile.ts.
In `@src/utils/providerProfile.ts`:
- Around line 1976-1990: The OpenAI-compatible fallback in providerProfile’s
launch selection currently treats CLAUDE_CODE_USE_OPENAI=0 as if it were unset,
so the early return can override an explicit opt-out. Update the conditional
around getConcreteOpenAICompatibleEnvRouteId/processEnv so it checks for an
explicit disable value before returning buildLaunchEnv with profile: 'openai',
and only take this path when the env var is not explicitly set to 0.
🪄 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: e7554027-0811-46c4-8bdd-ac4a0edfe8fa
📒 Files selected for processing (4)
src/services/api/openaiShim.test.tssrc/services/api/openaiShim.tssrc/utils/providerProfile.test.tssrc/utils/providerProfile.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)
Use TypeScript with strict mode and ESM imports in source and test files.
Files:
src/utils/providerProfile.test.tssrc/services/api/openaiShim.test.tssrc/services/api/openaiShim.tssrc/utils/providerProfile.ts
src/{commands,components,services,tools,utils,integrations,entrypoints,tasks}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Prefer the existing service, provider, settings, permission, and UI patterns over introducing new abstractions.
Files:
src/utils/providerProfile.test.tssrc/services/api/openaiShim.test.tssrc/services/api/openaiShim.tssrc/utils/providerProfile.ts
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
In the files you touch, preserve the existing code style.
Files:
src/utils/providerProfile.test.tssrc/services/api/openaiShim.test.tssrc/services/api/openaiShim.tssrc/utils/providerProfile.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/utils/providerProfile.test.tssrc/services/api/openaiShim.test.tssrc/services/api/openaiShim.tssrc/utils/providerProfile.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/utils/providerProfile.test.tssrc/services/api/openaiShim.test.tssrc/services/api/openaiShim.tssrc/utils/providerProfile.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/utils/providerProfile.test.tssrc/services/api/openaiShim.test.tssrc/services/api/openaiShim.tssrc/utils/providerProfile.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/utils/providerProfile.test.tssrc/services/api/openaiShim.test.ts
🔇 Additional comments (6)
src/services/api/openaiShim.ts (2)
367-388: NIM detection/mutation helpers look correct.Hostname check via
URL().hostnamecorrectly avoids substring-based host spoofing (e.g.evil.com/integrate.api.nvidia.com), and the kwargs merge preserves any pre-existingchat_template_kwargs.
3774-3808: 🎯 Functional CorrectnessNo issue: NIM thinking only turns on when the resolved plan enables reasoning.
resolveOpenAIShimReasoningRequestPlan()never pairsthinkingType: 'disabled'with a truthyreasoningEffort, sosetNvidiaNimChatTemplateThinking()only runs when reasoning is actually enabled.> Likely an incorrect or invalid review comment.src/services/api/openaiShim.test.ts (1)
7996-8034: 🎯 Functional CorrectnessNo env cleanup issue here. The shared
beforeEach/afterEachalready resetsOPENAI_BASE_URLandNVIDIA_API_KEY, so these tests don’t leak provider config into later cases.> Likely an incorrect or invalid review comment.src/utils/providerProfile.ts (3)
1248-1251: LGTM!
1308-1339: LGTM!
1897-1897: LGTM!Also applies to: 1907-1909, 1920-1925
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/utils/providerProfile.ts (1)
2002-2018: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftGate this fallback on env-only provider intent
This early return only checkshasExplicitNonOpenAIProviderSelection, so env-only provider setups can still be overridden by staleOPENAI_BASE_URL/OPENAI_MODEL/NIM env beforehasConcreteProviderSelectionruns. Use the same precedence guard here, or route throughresolveActiveRouteIdFromEnv, so the OpenAI/NIM fallback doesn't win over an already-selected provider.🤖 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/utils/providerProfile.ts` around lines 2002 - 2018, The fallback in providerProfile’s OpenAI route selection is being triggered without fully honoring env-only provider intent. Update the early return guarded by getConcreteOpenAICompatibleEnvRouteId so it uses the same precedence check as the rest of the provider resolution flow, such as hasConcreteProviderSelection or resolveActiveRouteIdFromEnv, and keep the existing explicit opt-out checks. This should ensure stale OPENAI_BASE_URL/OPENAI_MODEL/NIM env values do not override an already-selected provider when building the launch env.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/utils/providerProfile.test.ts`:
- Around line 558-580: The current test only covers explicit Claude Gemini flag
selection, so add a companion case in buildStartupEnvFromProfile that uses
env-only Gemini settings without CLAUDE_CODE_USE_GEMINI alongside the concrete
NIM variables. Use the existing helpers resolveActiveRouteIdFromEnv and
isDefaultStartupProviderEnv to assert the intended precedence and document that
the env-only provider choice is preserved over the NIM setup.
---
Outside diff comments:
In `@src/utils/providerProfile.ts`:
- Around line 2002-2018: The fallback in providerProfile’s OpenAI route
selection is being triggered without fully honoring env-only provider intent.
Update the early return guarded by getConcreteOpenAICompatibleEnvRouteId so it
uses the same precedence check as the rest of the provider resolution flow, such
as hasConcreteProviderSelection or resolveActiveRouteIdFromEnv, and keep the
existing explicit opt-out checks. This should ensure stale
OPENAI_BASE_URL/OPENAI_MODEL/NIM env values do not override an already-selected
provider when building the launch env.
🪄 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: 0d0b47b4-6903-4e74-8830-018b3d34a554
📒 Files selected for processing (4)
src/services/api/openaiShim.test.tssrc/services/api/openaiShim.tssrc/utils/providerProfile.test.tssrc/utils/providerProfile.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: typecheck
- GitHub Check: smoke-and-tests (24.11.x)
- GitHub Check: smoke-and-tests (22)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript with strict mode and ESM imports in source and test files.
Files:
src/utils/providerProfile.test.tssrc/services/api/openaiShim.tssrc/services/api/openaiShim.test.tssrc/utils/providerProfile.ts
src/{commands,components,services,tools,utils,integrations,entrypoints,tasks}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Prefer the existing service, provider, settings, permission, and UI patterns over introducing new abstractions.
Files:
src/utils/providerProfile.test.tssrc/services/api/openaiShim.tssrc/services/api/openaiShim.test.tssrc/utils/providerProfile.ts
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
In the files you touch, preserve the existing code style.
Files:
src/utils/providerProfile.test.tssrc/services/api/openaiShim.tssrc/services/api/openaiShim.test.tssrc/utils/providerProfile.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/utils/providerProfile.test.tssrc/services/api/openaiShim.tssrc/services/api/openaiShim.test.tssrc/utils/providerProfile.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/utils/providerProfile.test.tssrc/services/api/openaiShim.tssrc/services/api/openaiShim.test.tssrc/utils/providerProfile.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/utils/providerProfile.test.tssrc/services/api/openaiShim.tssrc/services/api/openaiShim.test.tssrc/utils/providerProfile.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/utils/providerProfile.test.tssrc/services/api/openaiShim.test.ts
🔇 Additional comments (5)
src/services/api/openaiShim.test.ts (1)
8036-8069: LGTM!Also applies to: 8680-8710, 8712-8744
src/utils/providerProfile.ts (2)
1241-1262: LGTM!Also applies to: 1319-1326
1923-1951: 🔒 Security & PrivacyNo action needed for NVIDIA carry-over
NVIDIA_API_KEYandNVIDIA_NIMare already scoped tonvidia-nim, so they won't be mirrored into other OpenAI-compatible routes or pulled from a persisted profile outside that route.> Likely an incorrect or invalid review comment.src/utils/providerProfile.test.ts (1)
510-556: LGTM!src/services/api/openaiShim.ts (1)
389-406: 🎯 Functional CorrectnessNo issue here The disabled-thinking path clears
reasoningEffortbeforemaybeSetNvidiaNimChatTemplateThinking()runs, sochat_template_kwargsstays unset.> Likely an incorrect or invalid review comment.
2 participants
Summary
Fixes #1892.
Risk surface
Pending PR overlap checked
Tests
Summary by CodeRabbit