Repository navigation
Conversation
📝 WalkthroughWalkthroughAdds per-provider self-hosted tool configuration, propagates it through profile environments and presets, updates provider setup flows, and extends the OpenAI shim with gated semantic-boundary and JSON/XML tool-call recovery. ChangesSelf-hosted OpenAI tool compatibility
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/services/api/openaiShim.test.ts`:
- Line 7450: Remove the duplicate content property signatures from the type
assertion in the test, retaining a single content definition with the required
fields so bun run typecheck succeeds.
🪄 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: cd30b78c-c8c1-4296-8cb1-d6d1fcde123e
📒 Files selected for processing (11)
.env.examplesrc/components/ProviderManager.test.tsxsrc/components/ProviderManager.tsxsrc/services/api/openaiShim.test.tssrc/services/api/openaiShim.tssrc/services/api/providerConfig.local.test.tssrc/services/api/providerConfig.tssrc/utils/config.tssrc/utils/providerProfile.tssrc/utils/providerProfiles.test.tssrc/utils/providerProfiles.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: smoke-and-tests (22)
🧰 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}: Run type validation for TypeScript changes usingbun run typecheckand, where relevant,bun run typecheck:type-tests.
Run focused tests for affected TypeScript test files and use the relevant provider test commands when provider behavior changes.
Files:
src/utils/config.tssrc/services/api/providerConfig.local.test.tssrc/services/api/providerConfig.tssrc/components/ProviderManager.test.tsxsrc/services/api/openaiShim.test.tssrc/utils/providerProfiles.test.tssrc/components/ProviderManager.tsxsrc/utils/providerProfile.tssrc/utils/providerProfiles.tssrc/services/api/openaiShim.ts
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*: Keep changes focused on one problem or clearly scoped feature, and avoid unrelated cleanup or broad rewrites.
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; keep comments useful and concise; do not reformat unrelated files.
Provider changes must avoid breaking third-party providers while fixing first-party behavior, explicitly identify affected providers, test the exact changed provider/model path when possible, and document limitations or follow-up work.
Do not assign or use provider tags in provider-change pull requests; maintainers control those tags.
Dependency changes require a concrete project benefit, such as fixing a bug, addressing a security issue, or supporting an approved feature.
Run the narrowest meaningful validation command for the touched area before opening a pull request; pull requests must pass required CI checks.
Runbun run security:pr-scanbefore submitting changes when the pull request requires the project’s intent/security scan.
Files:
src/utils/config.tssrc/services/api/providerConfig.local.test.tssrc/services/api/providerConfig.tssrc/components/ProviderManager.test.tsxsrc/services/api/openaiShim.test.tssrc/utils/providerProfiles.test.tssrc/components/ProviderManager.tsxsrc/utils/providerProfile.tssrc/utils/providerProfiles.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/utils/config.tssrc/services/api/providerConfig.local.test.tssrc/services/api/providerConfig.tssrc/components/ProviderManager.test.tsxsrc/services/api/openaiShim.test.tssrc/utils/providerProfiles.test.tssrc/components/ProviderManager.tsxsrc/utils/providerProfile.tssrc/utils/providerProfiles.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/utils/config.tssrc/services/api/providerConfig.local.test.tssrc/services/api/providerConfig.tssrc/components/ProviderManager.test.tsxsrc/services/api/openaiShim.test.tssrc/utils/providerProfiles.test.tssrc/components/ProviderManager.tsxsrc/utils/providerProfile.tssrc/utils/providerProfiles.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/providerConfig.local.test.tssrc/services/api/providerConfig.tssrc/services/api/openaiShim.test.tssrc/utils/providerProfiles.test.tssrc/utils/providerProfile.tssrc/utils/providerProfiles.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/providerConfig.local.test.tssrc/components/ProviderManager.test.tsxsrc/services/api/openaiShim.test.tssrc/utils/providerProfiles.test.ts
src/components/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
Use React + Ink for terminal UI components under
src/components/.
Files:
src/components/ProviderManager.test.tsxsrc/components/ProviderManager.tsx
🪛 dotenv-linter (4.0.0)
.env.example
[warning] 301-301: [ExtraBlankLine] Extra blank line detected
(ExtraBlankLine)
🔇 Additional comments (33)
src/services/api/openaiShim.ts (1)
91-93: LGTM!Also applies to: 1114-1121, 1378-1390, 2431-2449, 2554-2559, 2583-2587, 3134-3137, 3167-3167, 3229-3232, 3619-3632, 3880-3883
src/services/api/providerConfig.local.test.ts (1)
10-12: LGTM!Also applies to: 92-135, 136-169
src/services/api/openaiShim.test.ts (1)
7313-7415: LGTM!src/utils/config.ts (1)
233-239: LGTM!src/utils/providerProfile.ts (2)
79-80: LGTM!
165-168: LGTM!src/utils/providerProfiles.ts (7)
79-103: LGTM!
362-367: LGTM!
408-408: LGTM!
484-485: LGTM!
916-923: LGTM!
1336-1343: LGTM!
1358-1358: LGTM!src/services/api/providerConfig.ts (3)
558-564: LGTM!
566-608: LGTM!
610-636: LGTM!.env.example (1)
285-301: LGTM!src/utils/providerProfiles.test.ts (2)
38-39: LGTM!
310-338: LGTM!src/components/ProviderManager.tsx (10)
69-69: LGTM!
148-148: LGTM!
200-207: LGTM!
264-264: LGTM!
300-300: LGTM!
331-340: LGTM!
905-907: LGTM!
1584-1585: LGTM!
1687-1691: LGTM!
2193-2215: LGTM!src/components/ProviderManager.test.tsx (4)
682-685: LGTM!
1305-1311: LGTM!
1381-1405: LGTM!
2375-2384: LGTM!
|
kevincodex1
left a comment
kevincodex1
left a comment
There was a problem hiding this comment.
LGTM. please check too @jatmn
0xghost42
left a comment
0xghost42
left a comment
There was a problem hiding this comment.
The per-profile gating design here is good — selfHostedTools on the profile, providerProfileSupportsSelfHostedTools filtering the form step, and shouldInjectToolResultSemanticBoundary narrowing the placeholder to Mistral/Devstral all look right, and the Mistral fix itself is correct. One finding on the new non-streaming path.
Findings
-
[P1] The new non-streaming JSON-in-text recovery is ungated, so it fires on cloud providers too
src/services/api/openaiShim.ts:2432The streaming call site gates this behaviour carefully:
isLikelyOllamaEndpoint(request.baseUrl) || (Boolean(params.tools?.length) && shouldUseSelfHostedToolCompat(request.baseUrl))
The new non-streaming block has no equivalent gate —
convertNonStreamingResponseToAnthropicMessage(data, model)never receivesbaseUrlorparams.tools, so it cannot gate. It runs for every OpenAI-compatible provider whenevertool_callsis absent. Onupstream/mainparseTextToolCallshad exactly one caller (theisOllamaStream-gated streaming path); this PR adds a second, unconditional one.That matters because
parseAndAdd(openaiShim.ts:1728) accepts any JSON object with a stringname, defaultingargumentsto{}— there is no check against the advertised tool list. The context guard only skips JSON followed by prose, so JSON that ends a message is always accepted.Repro against
https://api.openai.com/v1,gpt-4o, no tools advertised, assistant content:Here is an example person object: ```json {"name": "Alice", "age": 30}On this branch: ```json [{"type":"text","text":"Here is an example person object:"}, {"type":"tool_use","id":"ollama_tc_1","name":"Alice","input":{}}] stop_reason = "tool_use"On
upstream/main, same input:[{"type":"text","text":"Here is an example person object:\n\n```json\n{\"name\": \"Alice\", \"age\": 30}\n```"}] stop_reason = "end_turn"So any non-streaming reply that ends in a
{"name": ...}object — a person record, apackage.jsonfragment, an API example, a config snippet — is converted into a phantomtool_usefor a tool that does not exist, the JSON is stripped out of the visible text, and the turn endsstop_reason: tool_use. The user loses the answer they asked for. This is the case the rest of the PR is explicitly trying to prevent for cloud backends.Suggested fix: thread the boolean the streaming call site already computes into
convertNonStreamingResponseToAnthropicMessageand gate the block on it. Both call sites (:2639inside the ignored-stream:truefallback, and:5022in_convertNonStreamingResponse) haverequest/paramsin scope. Worth mirroring theparams.tools?.lengthhalf of the streaming condition too — recovering a tool call when no tools were advertised is never correct, on any backend.A regression test on a cloud base URL with a benign
{"name": ...}payload asserting a singletextblock andstop_reason: end_turnwould lock this down; the existing new tests only cover local/LAN base URLs, where the intended behaviour and the bug are indistinguishable.
ecc815b
|
Done. @0xghost42 |
|
please rebase to latest main |
ecc815b to
7d01161
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/utils/config.ts (1)
200-213: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRemove the unrelated compaction-default change from this provider PR.
These lines silently make unset/invalid values resolve to
'200', enabling message-count compaction globally. Split or revert this behavior so the PR remains limited to self-hosted tool compatibility.As per coding guidelines, “Keep changes focused on one problem or feature and avoid mixing unrelated cleanup into the same change.”
Also applies to: 697-697, 756-759, 1168-1182
🤖 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/config.ts` around lines 200 - 213, The compaction-default behavior is unrelated to this provider compatibility change. Revert or isolate the changes involving isValidMaxMessagesCompactionThreshold, normalizeMaxMessagesCompactionThreshold, and the corresponding references so unset or invalid values are not silently normalized to '200'; keep the PR limited to self-hosted tool compatibility.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.
Inline comments:
In `@src/components/ProviderManager.test.tsx`:
- Around line 699-702: Extend the test around the Self-hosted tools selection to
choose “Enabled” and assert the persisted payload contains selfHostedTools:
true, rather than only checking that the option is displayed. Update the preset
mock to match the production Ollama default, and keep the test focused on
verifying saved behavior.
In `@src/components/ProviderManager.tsx`:
- Line 70: Update mockProviderProfilesModule in ProviderManager.test.tsx to
include providerProfileSupportsSelfHostedTools from the actual providerProfiles
module before applying the mock overrides, ensuring ProviderManager receives the
newly imported helper.
In `@src/services/api/openaiShim.test.ts`:
- Around line 7344-7446: Add a shim-level regression test alongside the existing
non-Mistral tests that configures the Mistral/Devstral provider and captures the
outbound request from createOpenAIShimClient. Submit assistant tool_use, user
tool_result, and a following user message, then assert the serialized roles are
assistant, tool, assistant, user and that the inserted assistant message is the
expected semantic boundary placeholder. Exercise the exact provider/model path
used by the Mistral behavior.
In `@src/services/api/providerConfig.ts`:
- Around line 591-599: Update the Mistral detection logic around baseUrl to
parse the URL and match only hostname === 'mistral.ai' or hostnames ending with
'.mistral.ai'. Remove the arbitrary lowercase substring check so unrelated
domains and URL paths cannot trigger Mistral behavior, while preserving the
existing base URL fallback order.
In `@src/utils/providerProfile.ts`:
- Around line 80-81: Update buildLaunchEnv to copy OPENAI_SELF_HOSTED_TOOLS and
OPENAI_PARSE_TEXT_TOOL_CALLS into the rebuilt OpenAI environment, resolving each
value with shell processEnv taking precedence over persistedEnv. Ensure
buildCompatibilityProcessEnv receives these keys so saved selfHostedTools
settings survive profile relaunch.
---
Outside diff comments:
In `@src/utils/config.ts`:
- Around line 200-213: The compaction-default behavior is unrelated to this
provider compatibility change. Revert or isolate the changes involving
isValidMaxMessagesCompactionThreshold, normalizeMaxMessagesCompactionThreshold,
and the corresponding references so unset or invalid values are not silently
normalized to '200'; keep the PR limited to self-hosted tool compatibility.
🪄 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: 95700125-130f-4807-b564-59c9a2ba751f
📒 Files selected for processing (11)
.env.examplesrc/components/ProviderManager.test.tsxsrc/components/ProviderManager.tsxsrc/services/api/openaiShim.test.tssrc/services/api/openaiShim.tssrc/services/api/providerConfig.local.test.tssrc/services/api/providerConfig.tssrc/utils/config.tssrc/utils/providerProfile.tssrc/utils/providerProfiles.test.tssrc/utils/providerProfiles.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/utils/config.tssrc/utils/providerProfiles.test.tssrc/services/api/providerConfig.local.test.tssrc/services/api/providerConfig.tssrc/components/ProviderManager.test.tsxsrc/utils/providerProfile.tssrc/components/ProviderManager.tsxsrc/services/api/openaiShim.test.tssrc/utils/providerProfiles.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/utils/config.tssrc/utils/providerProfiles.test.tssrc/services/api/providerConfig.local.test.tssrc/services/api/providerConfig.tssrc/components/ProviderManager.test.tsxsrc/utils/providerProfile.tssrc/components/ProviderManager.tsxsrc/services/api/openaiShim.test.tssrc/utils/providerProfiles.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/utils/config.tssrc/utils/providerProfiles.test.tssrc/services/api/providerConfig.local.test.tssrc/services/api/providerConfig.tssrc/components/ProviderManager.test.tsxsrc/utils/providerProfile.tssrc/components/ProviderManager.tsxsrc/services/api/openaiShim.test.tssrc/utils/providerProfiles.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/utils/config.tssrc/utils/providerProfiles.test.tssrc/services/api/providerConfig.local.test.tssrc/services/api/providerConfig.tssrc/components/ProviderManager.test.tsxsrc/utils/providerProfile.tssrc/components/ProviderManager.tsxsrc/services/api/openaiShim.test.tssrc/utils/providerProfiles.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/utils/providerProfiles.test.tssrc/services/api/providerConfig.local.test.tssrc/services/api/providerConfig.tssrc/utils/providerProfile.tssrc/services/api/openaiShim.test.tssrc/utils/providerProfiles.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/utils/providerProfiles.test.tssrc/services/api/providerConfig.local.test.tssrc/components/ProviderManager.test.tsxsrc/services/api/openaiShim.test.ts
src/components/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
Use React + Ink for terminal UI components under
src/components/.
Files:
src/components/ProviderManager.test.tsxsrc/components/ProviderManager.tsx
🪛 dotenv-linter (4.0.0)
.env.example
[warning] 307-307: [ExtraBlankLine] Extra blank line detected
(ExtraBlankLine)
🔇 Additional comments (12)
src/utils/providerProfiles.test.ts (1)
22-22: LGTM!Also applies to: 39-40, 51-51, 116-116, 313-341, 2291-2304, 3287-3324, 3532-3626
src/components/ProviderManager.tsx (1)
17-17: LGTM!Also applies to: 149-149, 201-208, 258-269, 293-314, 322-345, 903-942, 1596-1639, 1658-1722, 1955-1980, 2203-2306
src/components/ProviderManager.test.tsx (1)
16-23: LGTM!Also applies to: 118-118, 161-162, 234-243, 581-581, 683-683, 713-762, 1373-1379, 1449-1473, 2443-2452
src/services/api/openaiShim.test.ts (2)
7488-7491: Remove the repeatedcontentproperty signatures.This matches the previously reported type-assertion concern.
Also applies to: 7536-7539
4114-4143: LGTM!Also applies to: 7448-7487, 7493-7500, 7502-7535, 7541-7546, 8981-9183
src/utils/config.ts (1)
239-245: LGTM!src/utils/providerProfile.ts (1)
1-1: LGTM!Also applies to: 67-67, 154-154, 1218-1218, 1330-1343, 1519-1553
src/utils/providerProfiles.ts (1)
67-103: LGTM!Also applies to: 231-261, 312-370, 394-446, 480-505, 546-557, 624-808, 869-1059, 1190-1254, 1312-1437, 1440-1627, 1689-1769
src/services/api/providerConfig.ts (1)
558-590: LGTM!Also applies to: 601-635
.env.example (1)
36-40: LGTM!Also applies to: 291-305
src/services/api/openaiShim.ts (1)
91-93: LGTM!Also applies to: 1103-1432, 2388-2548, 2563-2568, 2592-2652, 3147-3245, 3600-3712, 3900-3903, 3951-3953, 4562-4563, 4668-4670, 4754-4755, 5022-5050, 5071-5078
src/services/api/providerConfig.local.test.ts (1)
10-12: LGTM!Also applies to: 92-169
Done. |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Preserve XML tool-call recovery on self-hosted streams
src/services/api/openaiShim.ts:3612
The new self-hosted gate reusesisOllamaStream, so local or opted-in llama-server/vLLM requests with tools buffer all content and run onlyparseTextToolCallsat completion. The existing XML detector/finalizer is skipped by!isOllamaStream; consequently a Qwen/GLM response such as<tool_call><function=Bash>…</tool_call>is emitted as text withend_turnrather than astool_use. This was supported for every non-Ollama OpenAI-compatible endpoint before the change, and it leaves affected self-hosted agents unable to execute their tools. Keep XML recovery active alongside the new JSON buffering and add a self-hosted streaming regression test. -
[P1] Do not convert arbitrary JSON prose into a tool invocation
src/services/api/openaiShim.ts:1728
parseAndAddaccepts any JSON object containing a stringname, defaults a missingargumentsfield to{}, and never checks the advertised tool names. This parser is now enabled for non-Ollama local/self-hosted endpoints whenever a request advertises tools. A normal completion ending in{"name":"Alice","age":30}is therefore stripped and returned astool_use(name="Alice", input={}), causing the runtime to attempt an unavailable tool instead of returning the answer. Validate the complete tool-call shape and require the name to be one of the request's tools before extracting it. -
[P2] Match the Mistral hostname instead of a URL substring
src/services/api/providerConfig.ts:597
baseUrl.includes('mistral.ai')classifies unrelated endpoints such ashttps://mistral.ai-proxy.example/v1(or a gateway path containing that text) as Mistral. The shim then inserts[Tool results received]betweentoolanduser, reintroducing the exact Qwen/llama echo-and-stall failure this PR is meant to prevent. Parse the URL and limit this check tomistral.aiand its subdomains, while retaining the explicit Mistral-mode/model cases. -
[P2] Preserve documented shell overrides during profile activation
src/utils/providerProfile.ts:80
The new override variables are added toPROFILE_ENV_KEYS, soapplyProviderProfileToProcessEnvclears them before applying an active profile. Profiles only re-addOPENAI_SELF_HOSTED_TOOLSwhen their saved boolean is true, which means an exportedOPENAI_SELF_HOSTED_TOOLS=1or legacyOPENAI_PARSE_TEXT_TOOL_CALLS=1is silently discarded when the user activates a reverse-proxied profile whose UI switch is unset. That contradicts both the documented shell override and the new launch-env “shell wins” behavior; preserve/reapply explicit shell overrides through the managed profile swap.
6a9b74a to
640ce98
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
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)
1730-1773: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject malformed or non-object
argumentsinstead of converting them to{}.A malformed JSON string currently becomes an empty argument object, while strings decoding to arrays, primitives, or
nullcan pass through as tool input. Reject these candidates and use the same object validator for both supported envelopes.Proposed fix
if (typeof rawArgs === 'string') { try { - args = JSON.parse(rawArgs) as Record<string, unknown> + const parsed = JSON.parse(rawArgs) + if (!parsed || typeof parsed !== 'object' || Array.isArray(parsed)) { + return false + } + args = parsed as Record<string, unknown> } catch { - args = {} + return false }🤖 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 1730 - 1773, Update the tool-call parsing logic in the visible direct-object and type=function envelopes to reject malformed JSON and any arguments value that does not decode to a non-array object, rather than defaulting to {}. Apply the same existing object-arguments validator to both paths, including parsed string arguments, while preserving support for valid object arguments and the established missing/null behavior only if the validator explicitly permits it.
🤖 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 3235-3243: Update the XML recovery branch around parseXmlToolCalls
so recovered calls are filtered through the advertised allowedToolNames
allowlist before assignment. Keep each recoveredCalls entry paired with its
corresponding recoveredRanges entry, and assign only the filtered calls and
ranges; preserve the existing behavior when no allowed calls remain.
In `@src/services/api/openaiShim.xmlToolCalls.test.ts`:
- Around line 514-524: Update the beforeEach/afterEach test lifecycle around
originalFetch to capture the initial OPENAI_API_KEY and OPENAI_BASE_URL values
before overriding them. In afterEach, restore each captured environment value,
deleting the key only when its original value was undefined, while preserving
the existing globalThis.fetch restoration.
In `@src/utils/providerProfiles.ts`:
- Around line 873-877: Update the profile activation flow around
shellSelfHostedTools and shellParseTextToolCalls so only values originating from
the shell are preserved; do not treat values previously written by an active
profile as shell overrides. Track override provenance separately from
PROFILE_ENV_KEYS and ensure a direct selfHostedTools true-to-false activation
applies false. Add a regression test covering that activation sequence.
---
Outside diff comments:
In `@src/services/api/openaiShim.ts`:
- Around line 1730-1773: Update the tool-call parsing logic in the visible
direct-object and type=function envelopes to reject malformed JSON and any
arguments value that does not decode to a non-array object, rather than
defaulting to {}. Apply the same existing object-arguments validator to both
paths, including parsed string arguments, while preserving support for valid
object arguments and the established missing/null behavior only if the validator
explicitly permits it.
🪄 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: e5762c22-a48e-4c71-acfd-69e9c3112664
📒 Files selected for processing (7)
src/services/api/openaiShim.ollamaTextToolCalls.test.tssrc/services/api/openaiShim.tssrc/services/api/openaiShim.xmlToolCalls.test.tssrc/services/api/providerConfig.local.test.tssrc/services/api/providerConfig.tssrc/utils/providerProfiles.test.tssrc/utils/providerProfiles.ts
📜 Review details
🧰 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.ollamaTextToolCalls.test.tssrc/utils/providerProfiles.test.tssrc/services/api/openaiShim.xmlToolCalls.test.tssrc/services/api/providerConfig.local.test.tssrc/utils/providerProfiles.tssrc/services/api/providerConfig.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.ollamaTextToolCalls.test.tssrc/utils/providerProfiles.test.tssrc/services/api/openaiShim.xmlToolCalls.test.tssrc/services/api/providerConfig.local.test.tssrc/utils/providerProfiles.tssrc/services/api/providerConfig.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.ollamaTextToolCalls.test.tssrc/utils/providerProfiles.test.tssrc/services/api/openaiShim.xmlToolCalls.test.tssrc/services/api/providerConfig.local.test.tssrc/utils/providerProfiles.tssrc/services/api/providerConfig.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.ollamaTextToolCalls.test.tssrc/utils/providerProfiles.test.tssrc/services/api/openaiShim.xmlToolCalls.test.tssrc/services/api/providerConfig.local.test.tssrc/utils/providerProfiles.tssrc/services/api/providerConfig.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.ollamaTextToolCalls.test.tssrc/utils/providerProfiles.test.tssrc/services/api/openaiShim.xmlToolCalls.test.tssrc/services/api/providerConfig.local.test.tssrc/utils/providerProfiles.tssrc/services/api/providerConfig.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.ollamaTextToolCalls.test.tssrc/utils/providerProfiles.test.tssrc/services/api/openaiShim.xmlToolCalls.test.tssrc/services/api/providerConfig.local.test.ts
🔇 Additional comments (8)
src/utils/providerProfiles.ts (1)
88-103: LGTM!Also applies to: 365-370, 411-411, 503-504, 950-957, 1377-1384, 1399-1399
src/services/api/providerConfig.ts (1)
558-645: LGTM!src/utils/providerProfiles.test.ts (1)
39-40: LGTM!Also applies to: 313-360
src/services/api/openaiShim.ts (2)
3693-3699: 📐 Maintainability & Code QualityConfirm the required provider validation suite.
The PR report only confirms
bun run smoke. Please also confirm:bun run typecheck bun run typecheck:type-tests bun run test:provider bun run test:provider-recommendation bun run security:pr-scanAs per coding guidelines, “Run provider tests and provider recommendation tests when changing provider behavior” and “Run
bun run security:pr-scanbefore submitting a pull request.”Sources: Coding guidelines, Path instructions
91-93: LGTM!Also applies to: 1114-1121, 1378-1390, 1717-1717, 1775-1870, 2440-2514, 2619-2710, 3205-3234, 3244-3328, 3722-3799, 3987-3990, 5074-5078
src/services/api/openaiShim.ollamaTextToolCalls.test.ts (1)
120-143: LGTM!src/services/api/openaiShim.xmlToolCalls.test.ts (1)
509-513: LGTM!Also applies to: 526-587
src/services/api/providerConfig.local.test.ts (1)
10-12: LGTM!Also applies to: 92-177
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/services/api/openaiShim.ts (2)
1778-1818: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDo not treat an empty allowlist as unrestricted.
An empty
Setcurrently accepts every name. Additionally, a non-empty malformedtoolsarray becomesundefined, enabling recovery while removing all filtering.Proposed fix
- if (allowedToolNames && allowedToolNames.size > 0 && !allowedToolNames.has(name)) { + if (allowedToolNames && !allowedToolNames.has(name)) { return false } ... - return names.size > 0 ? names : undefined + return namesAdd an empty-allowlist regression test. As per coding guidelines, “Add or update tests when a code change affects behavior.”
🤖 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 1778 - 1818, Update toolNamesFromShimParams and the allowedToolNames check so an explicitly supplied empty or malformed non-empty tools array produces an empty allowlist that rejects every tool name, while omitted tools remain unrestricted. Remove the size-based bypass in the allowlist validation and distinguish undefined from an empty Set. Add a regression test covering an explicitly empty allowlist.Source: Coding guidelines
4011-4033: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftSplit the unrelated GPT/Azure cloud-routing feature from this PR.
The shared change expands cloud-provider behavior despite this PR being scoped to self-hosted tool compatibility and explicitly promising unchanged cloud behavior.
src/services/api/openaiShim.ts#L4011-L4033: move GPT-5 chat/reasoning behavior to the cloud-routing change.src/services/api/openaiShim.ts#L4539-L4668: move Azure detection and URL normalization.src/services/api/openaiShim.ts#L3695-L3708: move provider-override Azure environment handling.src/services/api/openaiShim.ts#L3845-L3957: move the associated request-environment plumbing.src/services/api/openaiShim.test.ts#L689-L1253: move GPT/Azure request-routing tests.src/services/api/openaiShim.test.ts#L10137-L10216: move chat-completions reasoning assertions.src/services/api/providerConfig.local.test.ts#L465-L650: move model-routing and Azure helper tests.As per coding guidelines, “Keep changes focused on one problem or feature.” As per path instructions, keep changes focused on “the single provider-behavior problem.”
🤖 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 4011 - 4033, Remove the unrelated GPT/Azure cloud-routing changes so this PR remains limited to self-hosted tool compatibility: revert GPT-5 chat/reasoning handling around resolveOpenAIShimReasoningRequestPlan, Azure detection and URL normalization, provider-override environment handling, and associated request-environment plumbing in src/services/api/openaiShim.ts (3695-3708, 3845-3957, 4011-4033, 4539-4668). Also remove the corresponding GPT/Azure routing and chat-reasoning tests from src/services/api/openaiShim.test.ts (689-1253, 10137-10216) and src/services/api/providerConfig.local.test.ts (465-650); the listed sites all require removal or relocation to the separate cloud-routing change.Sources: Coding guidelines, Path instructions
♻️ Duplicate comments (1)
src/components/ProviderManager.test.tsx (1)
699-702: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftAssert the selected value is persisted, not merely displayed.
This test still only accepts the default "Disabled" option by simply writing
\rand continues. It does not select "Enabled" nor does it assert that the saved payload containsselfHostedTools: trueas requested.Select the "Enabled" option explicitly and assert the saved behavior. As per coding guidelines, "Add or update tests when a code change affects behavior." As per path instructions, "Review tests for meaningful coverage of the changed behavior... 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/components/ProviderManager.test.tsx` around lines 699 - 702, Update the test around the Self-hosted tools prompt to explicitly navigate from the default Disabled option to Enabled before submitting, rather than pressing Enter on the default. After submission, assert the persisted/saved payload includes selfHostedTools: true, using the test’s existing persistence or output assertion mechanism.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.
Inline comments:
In @.env.example:
- Around line 307-323: Remove the trailing blank line after the
OPENAI_SELF_HOSTED_TOOLS entry in the llama-server configuration section,
leaving the file with no extra blank line at the end.
In `@src/services/api/openaiShim.ts`:
- Around line 1740-1751: Update the rawArgs handling in the tool-use argument
preparation flow to reject JSON parse failures instead of replacing them with an
empty object, and validate parsed results as non-null, non-array objects before
assigning args. Preserve acceptance of valid object arguments and absent nullish
arguments as appropriate, return false for malformed strings and decoded
primitives or arrays, and add regression tests covering malformed JSON plus
"null" and array-shaped JSON.
In `@src/utils/providerProfiles.ts`:
- Around line 883-887: The profile activation logic around shellSelfHostedTools
and shellParseTextToolCalls must distinguish genuine shell overrides from values
previously injected by the active profile; track that provenance separately so
switching directly from selfHostedTools true to false clears the environment. In
src/utils/providerProfiles.ts lines 883-887, preserve only actual shell-provided
overrides. In src/utils/providerProfiles.test.ts lines 330-357, remove the
manual clearProviderProfileEnvFromProcessEnv() call and assert the direct
true-to-false activation clears the managed environment value.
---
Outside diff comments:
In `@src/services/api/openaiShim.ts`:
- Around line 1778-1818: Update toolNamesFromShimParams and the allowedToolNames
check so an explicitly supplied empty or malformed non-empty tools array
produces an empty allowlist that rejects every tool name, while omitted tools
remain unrestricted. Remove the size-based bypass in the allowlist validation
and distinguish undefined from an empty Set. Add a regression test covering an
explicitly empty allowlist.
- Around line 4011-4033: Remove the unrelated GPT/Azure cloud-routing changes so
this PR remains limited to self-hosted tool compatibility: revert GPT-5
chat/reasoning handling around resolveOpenAIShimReasoningRequestPlan, Azure
detection and URL normalization, provider-override environment handling, and
associated request-environment plumbing in src/services/api/openaiShim.ts
(3695-3708, 3845-3957, 4011-4033, 4539-4668). Also remove the corresponding
GPT/Azure routing and chat-reasoning tests from
src/services/api/openaiShim.test.ts (689-1253, 10137-10216) and
src/services/api/providerConfig.local.test.ts (465-650); the listed sites all
require removal or relocation to the separate cloud-routing change.
---
Duplicate comments:
In `@src/components/ProviderManager.test.tsx`:
- Around line 699-702: Update the test around the Self-hosted tools prompt to
explicitly navigate from the default Disabled option to Enabled before
submitting, rather than pressing Enter on the default. After submission, assert
the persisted/saved payload includes selfHostedTools: true, using the test’s
existing persistence or output assertion mechanism.
🪄 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: ead91b74-0db2-4800-8270-0cd3a8d18091
📒 Files selected for processing (14)
.env.examplesrc/components/ProviderManager.test.tsxsrc/components/ProviderManager.tsxsrc/services/api/openaiShim.ollamaTextToolCalls.test.tssrc/services/api/openaiShim.test.tssrc/services/api/openaiShim.tssrc/services/api/openaiShim.xmlToolCalls.test.tssrc/services/api/providerConfig.local.test.tssrc/services/api/providerConfig.tssrc/utils/config.tssrc/utils/providerProfile.test.tssrc/utils/providerProfile.tssrc/utils/providerProfiles.test.tssrc/utils/providerProfiles.ts
📜 Review details
🧰 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/utils/providerProfile.test.tssrc/utils/config.tssrc/services/api/openaiShim.ollamaTextToolCalls.test.tssrc/services/api/openaiShim.test.tssrc/utils/providerProfile.tssrc/utils/providerProfiles.test.tssrc/services/api/openaiShim.xmlToolCalls.test.tssrc/components/ProviderManager.test.tsxsrc/services/api/providerConfig.tssrc/services/api/providerConfig.local.test.tssrc/components/ProviderManager.tsxsrc/utils/providerProfiles.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/utils/providerProfile.test.tssrc/utils/config.tssrc/services/api/openaiShim.ollamaTextToolCalls.test.tssrc/services/api/openaiShim.test.tssrc/utils/providerProfile.tssrc/utils/providerProfiles.test.tssrc/services/api/openaiShim.xmlToolCalls.test.tssrc/components/ProviderManager.test.tsxsrc/services/api/providerConfig.tssrc/services/api/providerConfig.local.test.tssrc/components/ProviderManager.tsxsrc/utils/providerProfiles.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/utils/providerProfile.test.tssrc/utils/config.tssrc/services/api/openaiShim.ollamaTextToolCalls.test.tssrc/services/api/openaiShim.test.tssrc/utils/providerProfile.tssrc/utils/providerProfiles.test.tssrc/services/api/openaiShim.xmlToolCalls.test.tssrc/components/ProviderManager.test.tsxsrc/services/api/providerConfig.tssrc/services/api/providerConfig.local.test.tssrc/components/ProviderManager.tsxsrc/utils/providerProfiles.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/utils/providerProfile.test.tssrc/utils/config.tssrc/services/api/openaiShim.ollamaTextToolCalls.test.tssrc/services/api/openaiShim.test.tssrc/utils/providerProfile.tssrc/utils/providerProfiles.test.tssrc/services/api/openaiShim.xmlToolCalls.test.tssrc/components/ProviderManager.test.tsxsrc/services/api/providerConfig.tssrc/services/api/providerConfig.local.test.tssrc/components/ProviderManager.tsxsrc/utils/providerProfiles.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/utils/providerProfile.test.tssrc/services/api/openaiShim.ollamaTextToolCalls.test.tssrc/services/api/openaiShim.test.tssrc/utils/providerProfile.tssrc/utils/providerProfiles.test.tssrc/services/api/openaiShim.xmlToolCalls.test.tssrc/services/api/providerConfig.tssrc/services/api/providerConfig.local.test.tssrc/utils/providerProfiles.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/utils/providerProfile.test.tssrc/services/api/openaiShim.ollamaTextToolCalls.test.tssrc/services/api/openaiShim.test.tssrc/utils/providerProfiles.test.tssrc/services/api/openaiShim.xmlToolCalls.test.tssrc/components/ProviderManager.test.tsxsrc/services/api/providerConfig.local.test.ts
src/components/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
Use React + Ink for terminal UI components under
src/components/.
Files:
src/components/ProviderManager.test.tsxsrc/components/ProviderManager.tsx
🪛 dotenv-linter (4.0.0)
.env.example
[warning] 323-323: [ExtraBlankLine] Extra blank line detected
(ExtraBlankLine)
🔇 Additional comments (16)
src/utils/config.ts (1)
240-246: LGTM!src/utils/providerProfile.ts (1)
81-82: LGTM!Also applies to: 169-172, 2015-2035
src/utils/providerProfiles.ts (1)
80-104: LGTM!Also applies to: 370-375, 417-417, 509-510, 960-967, 1391-1398, 1414-1414
src/services/api/providerConfig.ts (1)
619-625: LGTM!Also applies to: 627-635, 642-678, 680-705
src/utils/providerProfiles.test.ts (1)
40-41: LGTM!Also applies to: 359-377
src/components/ProviderManager.tsx (1)
70-70: LGTM!Also applies to: 149-149, 201-208, 262-265, 297-301, 336-345, 910-912, 1599-1600, 1721-1725, 2227-2249
src/components/ProviderManager.test.tsx (1)
1373-1379: LGTM!Also applies to: 1449-1473, 2443-2452
src/utils/providerProfile.test.ts (1)
2236-2253: LGTM!Also applies to: 2255-2272
src/services/api/openaiShim.ts (2)
3239-3246: XML recovery still bypasses the advertised-tool allowlist.The fallback assigns every parsed XML call and range without filtering. This is the same unresolved finding from the previous review.
94-96: LGTM!Also applies to: 1117-1124, 1381-1393, 1720-1739, 1821-1880, 2443-2517, 2622-2713, 3208-3238, 3248-3331, 3709-3715, 3738-3745, 3780-3815, 4005-4008, 4067-4069, 5202-5208
src/services/api/openaiShim.test.ts (2)
7934-8036: Positive Mistral shim regression coverage is still missing.The new cases only prove suppression on non-Mistral paths. This is the same previously reported coverage gap.
3042-3061: LGTM!Also applies to: 8038-8136
src/services/api/openaiShim.xmlToolCalls.test.ts (2)
514-524: Environment teardown still deletes runner-provided values.Capture and restore the original API key and base URL instead of unconditionally deleting them. This is the same previously reported isolation issue.
509-513: LGTM!Also applies to: 526-588
src/services/api/openaiShim.ollamaTextToolCalls.test.ts (1)
120-143: LGTM!src/services/api/providerConfig.local.test.ts (1)
96-181: LGTM!
There was a problem hiding this comment.
Actionable comments posted: 3
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)
1750-1760: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winPrevent runtime crash on
nullpayload.If the LLM generates a fenced JSON block containing exactly
"null",JSON.parsereturnsnull. The subsequent property accessobj['name']will throw aTypeError: Cannot read properties of null (reading 'name'), crashing the parser and aborting the stream.As per path instructions for shims, reject
nulland require an object immediately after parsing to prevent runtime failure on unexpected model output.🛡️ Proposed fix
let obj: Record<string, unknown> try { obj = JSON.parse(raw) } catch { return false } + + if (!obj || typeof obj !== 'object' || Array.isArray(obj)) { + return false + } let name: string | undefined🤖 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 1750 - 1760, After JSON.parse in the parser, validate that the parsed value is a non-null object before accessing obj['name']; return false for null or any non-object payload. Keep the existing parsing behavior for valid object payloads and the JSON parse failure path unchanged.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.ollamaTextToolCalls.test.ts`:
- Around line 127-140: Add a regression test alongside the existing malformed
and array argument tests in parseTextToolCalls coverage for
{"name":"Bash","arguments":"null"}, asserting that no calls are returned.
In `@src/services/api/openaiShim.ts`:
- Around line 3266-3268: In the filtering logic around xmlParsed.toolCallRanges,
replace the conditional `if (range)` guard with a non-null assertion when adding
the range to filteredRanges. Preserve index-for-index alignment with
filteredCalls while relying on the guaranteed parallel population of
toolCallRanges and calls.
In `@src/utils/providerProfiles.ts`:
- Around line 1108-1114: Update the shell override checks for
shellSelfHostedToolsOverride and shellParseTextToolCallsOverride to use strict
!== undefined comparisons instead of truthiness checks, ensuring explicitly
empty environment values are reapplied to process.env and clear profile-managed
values.
---
Outside diff comments:
In `@src/services/api/openaiShim.ts`:
- Around line 1750-1760: After JSON.parse in the parser, validate that the
parsed value is a non-null object before accessing obj['name']; return false for
null or any non-object payload. Keep the existing parsing behavior for valid
object payloads and the JSON parse failure path 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: baa64c70-0189-46bc-9a6d-56535a9b7e7a
📒 Files selected for processing (5)
src/services/api/openaiShim.ollamaTextToolCalls.test.tssrc/services/api/openaiShim.tssrc/services/api/openaiShim.xmlToolCalls.test.tssrc/utils/providerProfiles.test.tssrc/utils/providerProfiles.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}: 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.xmlToolCalls.test.tssrc/utils/providerProfiles.test.tssrc/services/api/openaiShim.ollamaTextToolCalls.test.tssrc/utils/providerProfiles.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.xmlToolCalls.test.tssrc/utils/providerProfiles.test.tssrc/services/api/openaiShim.ollamaTextToolCalls.test.tssrc/utils/providerProfiles.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.xmlToolCalls.test.tssrc/utils/providerProfiles.test.tssrc/services/api/openaiShim.ollamaTextToolCalls.test.tssrc/utils/providerProfiles.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.xmlToolCalls.test.tssrc/utils/providerProfiles.test.tssrc/services/api/openaiShim.ollamaTextToolCalls.test.tssrc/utils/providerProfiles.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.xmlToolCalls.test.tssrc/utils/providerProfiles.test.tssrc/services/api/openaiShim.ollamaTextToolCalls.test.tssrc/utils/providerProfiles.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.xmlToolCalls.test.tssrc/utils/providerProfiles.test.tssrc/services/api/openaiShim.ollamaTextToolCalls.test.ts
🔇 Additional comments (2)
src/utils/providerProfiles.test.ts (1)
382-414: LGTM!src/services/api/openaiShim.xmlToolCalls.test.ts (1)
514-536: LGTM!
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P2] Gate Ollama text-tool recovery on advertised tools
src/services/api/openaiShim.ts:3746
The Ollama branch enables JSON-in-text recovery even whenparams.toolsis absent, soallowedToolNamesis undefined. A normal Ollama response such as{"name":"status","arguments":{"ok":true}}is consequently stripped from visible text and returned as an unadvertisedtool_usewithstop_reason: tool_use. The agent then attempts a tool it never exposed instead of displaying the answer. Require a nonempty advertised tool set for this fallback as well as for other self-hosted endpoints. -
[P2] Do not discard buffered self-hosted stream content at EOF
src/services/api/openaiShim.ts:3024
The new self-hosted path buffers every text delta whenever tools are present, but flushes that buffer only from thefinish_reasonbranch. If an SSE response ends after delivering text but without a terminal choice—for example, a gateway closes a truncated stream—the generator emits onlymessage_stopand loses all received assistant text. Finalize the buffer at EOF (or surface an explicit stream error) and add a regression case for this response shape. -
[P2] Keep startup-profile flags distinct from shell overrides
src/utils/providerProfiles.ts:80
A profile withselfHostedTools: truepersistsOPENAI_SELF_HOSTED_TOOLS=1for startup, but startup applies that value without the profile-managed marker. The first later profile activation captures the persisted value as a shell override and re-applies it at line 1111. Switching to a disabled or cloud profile therefore leaves text-tool recovery enabled for the session, defeating the new per-profile isolation. Track startup-applied provenance (or avoid recapturing persisted profile fields) before preserving shell overrides. -
[P2] Preserve XML ranges when deduplicating recovered calls
src/services/api/openaiShim.ts:3264
This new self-hosted XML recovery loop assumesparseXmlToolCalls()returns one range per call, but that parser deduplicates calls while retaining a range for every XML block. WithBash(pwd), a duplicateBash(pwd), thenRead(...), theReadcall is paired with the duplicate Bash range; stripping leaves the actual Read XML visible in assistant text while still emittingReadas a tool call. Keep call/range pairs through parsing/filtering, or independently strip every accepted XML range. -
[P2] Recognize Codestral as a Mistral-class boundary model
src/services/api/providerConfig.ts:677
The new model fallback coversdevstral,mistral, andministralbut omits the supportedcodestralmodel. A custom or reverse-proxied Mistral Codestral endpoint has neither a*.mistral.aihostname norCLAUDE_CODE_USE_MISTRAL, so it now receivestool → userinstead of the required Mistral semantic boundary and can reject or stall tool continuations. Include Codestral in the predicate and cover a proxy-base URL. -
[P2] Treat self-hosted-flag drift as active-profile drift
src/utils/providerProfiles.ts:785
isProcessEnvAlignedWithProfile()was not extended to compareOPENAI_SELF_HOSTED_TOOLS(or its legacy alias). After a true profile’s flag is cleared or changed by another settings/environment operation,applyActiveProviderProfileFromConfig()considers the marked profile aligned and declines to restore it. The toggle then silently stops affecting the active session. Compare the expected profile flag(s), as the existing context-window drift check does.
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 2030-2043: Update the addCall function so range is a required
[number, number] parameter rather than optional, and always append it to ranges
whenever a result is added. Ensure all addCall callsites provide a valid range,
preserving the 1:1 alignment between results and ranges.
In `@src/utils/providerProfile.ts`:
- Around line 2028-2051: Update the environment flag selection in the
self-hosted tools and parse-text tool-calls blocks to use strict undefined
checks instead of truthiness. In the assignments to selfHostedToolsFlag and
parseTextToolCallsFlag, prefer the shell value whenever it is not undefined,
including an empty string; only fall back to the persisted value when the shell
value is undefined, and preserve the existing startup-marker conditions
accordingly.
🪄 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: 996a54e2-937f-401c-8ec6-de3927052b70
📒 Files selected for processing (7)
src/services/api/openaiShim.ollamaTextToolCalls.test.tssrc/services/api/openaiShim.tssrc/services/api/providerConfig.local.test.tssrc/services/api/providerConfig.tssrc/utils/providerProfile.tssrc/utils/providerProfiles.test.tssrc/utils/providerProfiles.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}: 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/providerConfig.local.test.tssrc/services/api/openaiShim.ollamaTextToolCalls.test.tssrc/utils/providerProfile.tssrc/services/api/providerConfig.tssrc/utils/providerProfiles.test.tssrc/utils/providerProfiles.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/providerConfig.local.test.tssrc/services/api/openaiShim.ollamaTextToolCalls.test.tssrc/utils/providerProfile.tssrc/services/api/providerConfig.tssrc/utils/providerProfiles.test.tssrc/utils/providerProfiles.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/providerConfig.local.test.tssrc/services/api/openaiShim.ollamaTextToolCalls.test.tssrc/utils/providerProfile.tssrc/services/api/providerConfig.tssrc/utils/providerProfiles.test.tssrc/utils/providerProfiles.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/providerConfig.local.test.tssrc/services/api/openaiShim.ollamaTextToolCalls.test.tssrc/utils/providerProfile.tssrc/services/api/providerConfig.tssrc/utils/providerProfiles.test.tssrc/utils/providerProfiles.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/providerConfig.local.test.tssrc/services/api/openaiShim.ollamaTextToolCalls.test.tssrc/utils/providerProfile.tssrc/services/api/providerConfig.tssrc/utils/providerProfiles.test.tssrc/utils/providerProfiles.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/providerConfig.local.test.tssrc/services/api/openaiShim.ollamaTextToolCalls.test.tssrc/utils/providerProfiles.test.ts
🔇 Additional comments (6)
src/utils/providerProfile.ts (1)
81-85: LGTM!Also applies to: 172-179
src/utils/providerProfiles.ts (1)
67-108: LGTM!Also applies to: 122-147, 409-414, 456-456, 548-549, 814-817, 926-929, 1002-1009, 1119-1127, 1435-1442, 1458-1458
src/services/api/providerConfig.ts (1)
619-626: LGTM!Also applies to: 627-635, 642-678, 691-705
src/services/api/providerConfig.local.test.ts (1)
12-14: LGTM!Also applies to: 96-156, 158-190
src/utils/providerProfiles.test.ts (1)
40-41: LGTM!Also applies to: 330-357, 359-380, 382-418, 420-453
src/services/api/openaiShim.ollamaTextToolCalls.test.ts (1)
163-207: LGTM!
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/services/api/openaiShim.ts`:
- Around line 2035-2040: Update the XML parsing flow around the dedupKey/seen
logic so every matching range is retained for stripping, while ranges remains
1:1 with emitted results and deduplicated calls still return false. Use a
separate all-ranges collection for stripRanges, and add a regression test
covering repeated identical XML blocks to verify no duplicate XML remains in
visible assistant text.
🪄 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: bf18a8bc-017b-44e7-a56b-e3fe7b417d3b
📒 Files selected for processing (2)
src/services/api/openaiShim.tssrc/utils/providerProfile.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 (4)
**/*.{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/utils/providerProfile.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/utils/providerProfile.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/utils/providerProfile.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/utils/providerProfile.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/utils/providerProfile.tssrc/services/api/openaiShim.ts
🔇 Additional comments (3)
src/utils/providerProfile.ts (1)
2028-2060: LGTM!src/services/api/openaiShim.ts (2)
3786-3823: 📐 Maintainability & Code QualityRun the required provider-specific validation before merge.
The supplied summary reports smoke/build checks, but not the required TypeScript and provider checks for this behavior change. Please run and report:
bun run typecheck bun run typecheck:type-tests bun run test:provider bun run test:provider-recommendationAs per coding guidelines, provider behavior changes require exact-path tests and these validation commands. As per path instructions, provider changes must follow the provider-change workflow and list exact checks.
Sources: Coding guidelines, Path instructions
94-96: LGTM!Also applies to: 1117-1124, 1381-1393, 1716-1793, 1815-1885, 2071-2132, 2460-2475, 2514-2534, 2639-2673, 2725-2730, 3225-3375, 3611-3643, 3858-3893, 4083-4086, 5280-5286
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/services/api/openaiShim.ts (1)
2517-2524: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winApply the
allowedToolNamesfilter to non-streaming XML tool calls.While the streaming path correctly filters recovered XML tool calls against the allowlist, the non-streaming path emits all parsed XML tool calls indiscriminately. If a model hallucinates an XML tool call for a tool that wasn't advertised, it will be emitted as a
tool_useblock, bypassing theallowedToolNamesconstraint provided viaoptions.Filter the
xmlToolCallsloop to ensure only allowed tools are emitted, maintaining parity with the JSON-in-text and streaming recovery paths.♻️ Proposed fix
- for (const toolCall of xmlToolCalls) { - content.push({ - type: 'tool_use', - id: toolCall.id, - name: toolCall.name, - input: toolCall.arguments, - }) - } + for (const toolCall of xmlToolCalls) { + const isAllowed = !allowedToolNames || + (allowedToolNames instanceof Set + ? allowedToolNames.has(toolCall.name) + : allowedToolNames.includes(toolCall.name)) + + if (isAllowed) { + content.push({ + type: 'tool_use', + id: toolCall.id, + name: toolCall.name, + input: toolCall.arguments, + }) + } + }🤖 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 2517 - 2524, Update the non-streaming xmlToolCalls loop in the response conversion flow to consult options.allowedToolNames before pushing each tool_use block. Skip recovered XML calls whose names are not allowlisted, while preserving emission of allowed calls and matching the existing JSON-in-text and streaming filtering behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/services/api/openaiShim.ts`:
- Around line 2517-2524: Update the non-streaming xmlToolCalls loop in the
response conversion flow to consult options.allowedToolNames before pushing each
tool_use block. Skip recovered XML calls whose names are not allowlisted, while
preserving emission of allowed calls and matching the existing JSON-in-text and
streaming filtering behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b222ba01-3b07-4770-9cd8-ba8f04070b82
📒 Files selected for processing (2)
src/services/api/openaiShim.tssrc/services/api/openaiShim.xmlToolCalls.test.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}: 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.xmlToolCalls.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.xmlToolCalls.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.xmlToolCalls.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.xmlToolCalls.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.xmlToolCalls.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.xmlToolCalls.test.ts
🔇 Additional comments (2)
src/services/api/openaiShim.ts (1)
1716-1793: LGTM!Also applies to: 2022-2051
src/services/api/openaiShim.xmlToolCalls.test.ts (1)
51-65: LGTM!Also applies to: 252-265, 538-628
Clear self-hosted tool flags on providerOverride routes (in-process and agent env) so parent profile recovery does not apply to remote overrides. Recover text-form tools when finish_reason is tool_calls without structured deltas. Preserve shell OPENAI_SELF_HOSTED_TOOLS on profile clear, and persist Disabled as OPENAI_SELF_HOSTED_TOOLS=0 so local auto-detect cannot re-enable recovery.
Add Automatic (undefined) for self-hosted tools UI so existing profiles keep local auto-detect; Disabled still forces off. Alignment treats a captured shell OPENAI_SELF_HOSTED_TOOLS override as expected so re-apply does not loop. Pass requestProcessEnv into Mistral boundary detection for providerOverride routes.
Extract selfHostedToolsEnvValue for apply and alignment, and expose providerProfileSupportsSelfHostedTools on the ProviderManager test mock so formSteps/persistDraft do not call undefined.
dfaf632 to
e93bee4
Compare
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 (2)
src/utils/providerProfiles.test.ts (1)
17-41: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRESTORED_KEYS omits the two
OPENCLAUDE_STARTUP_*provenance markers.
PROFILE_ENV_KEYS(providerProfile.ts) managesOPENCLAUDE_STARTUP_SELF_HOSTED_TOOLS/OPENCLAUDE_STARTUP_PARSE_TEXT_TOOL_CALLSalongside the two flags added here, but this test's env-restoration list only covers the flags, not the markers. If any test sets those markers onprocess.env, they will leak into later tests in this file.As per path instructions, "Review tests for ... isolation of global/env/config state, ... provider profile leaks."
🧹 Proposed fix
'OPENAI_SELF_HOSTED_TOOLS', 'OPENAI_PARSE_TEXT_TOOL_CALLS', + 'OPENCLAUDE_STARTUP_SELF_HOSTED_TOOLS', + 'OPENCLAUDE_STARTUP_PARSE_TEXT_TOOL_CALLS',🤖 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/providerProfiles.test.ts` around lines 17 - 41, Update the RESTORED_KEYS array in providerProfiles.test.ts to include OPENCLAUDE_STARTUP_SELF_HOSTED_TOOLS and OPENCLAUDE_STARTUP_PARSE_TEXT_TOOL_CALLS, ensuring these PROFILE_ENV_KEYS markers are restored between tests like the other provider environment variables.Source: Path instructions
src/utils/config.ts (1)
302-303: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
compactTailTurnsis unrelated to this PR's self-hosted-tools scope.This adds an auto-compact message-retention setting that has nothing to do with self-hosted tool compatibility (the PR's stated purpose). Consider splitting it into its own change.
As per coding guidelines, "Keep pull requests focused on one issue or one clearly scoped improvement; avoid unrelated cleanup, fixes, features, or refactors in the same change."
Also applies to: 780-780
🤖 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/config.ts` around lines 302 - 303, Remove the unrelated compactTailTurns configuration field from the config definition. Keep toolHistoryCompressionEnabled and the rest of the self-hosted-tools compatibility changes unchanged; do not introduce or retain auto-compact retention settings in this PR.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.ollamaTextToolCalls.test.ts`:
- Around line 163-207: Update the test around the environment setup in “without
advertised tools, Ollama path does not recover JSON as tool_use” to capture the
original OPENAI_BASE_URL and OPENAI_API_KEY values before overriding them. In
the finally block, restore each original value when defined and delete it only
when originally undefined, matching the established pattern in
openaiShim.xmlToolCalls.test.ts.
---
Outside diff comments:
In `@src/utils/config.ts`:
- Around line 302-303: Remove the unrelated compactTailTurns configuration field
from the config definition. Keep toolHistoryCompressionEnabled and the rest of
the self-hosted-tools compatibility changes unchanged; do not introduce or
retain auto-compact retention settings in this PR.
In `@src/utils/providerProfiles.test.ts`:
- Around line 17-41: Update the RESTORED_KEYS array in providerProfiles.test.ts
to include OPENCLAUDE_STARTUP_SELF_HOSTED_TOOLS and
OPENCLAUDE_STARTUP_PARSE_TEXT_TOOL_CALLS, ensuring these PROFILE_ENV_KEYS
markers are restored between tests like the other provider environment
variables.
🪄 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: f247f117-f88f-47b8-8b13-fed21e7a159f
📒 Files selected for processing (16)
.env.examplesrc/components/ProviderManager.test.tsxsrc/components/ProviderManager.tsxsrc/services/api/agentRouting.test.tssrc/services/api/agentRouting.tssrc/services/api/openaiShim.ollamaTextToolCalls.test.tssrc/services/api/openaiShim.test.tssrc/services/api/openaiShim.tssrc/services/api/openaiShim.xmlToolCalls.test.tssrc/services/api/providerConfig.local.test.tssrc/services/api/providerConfig.tssrc/utils/config.tssrc/utils/providerProfile.test.tssrc/utils/providerProfile.tssrc/utils/providerProfiles.test.tssrc/utils/providerProfiles.ts
📜 Review details
🧰 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}: Provider changes must follow the documented integration patterns and avoid inconsistent behavior across provider paths.
When changing provider behavior, avoid breaking third-party providers and test the exact provider/model path changed when possible.
Review AI-generated code for correctness, style consistency, unnecessary changes, and adherence to project architecture before submission.
Run multiple rounds of self-review on AI-generated code; compilation alone is insufficient to establish correctness.
Files:
src/services/api/agentRouting.tssrc/utils/providerProfile.test.tssrc/services/api/providerConfig.local.test.tssrc/services/api/agentRouting.test.tssrc/utils/config.tssrc/services/api/openaiShim.xmlToolCalls.test.tssrc/services/api/providerConfig.tssrc/components/ProviderManager.test.tsxsrc/utils/providerProfile.tssrc/services/api/openaiShim.test.tssrc/utils/providerProfiles.test.tssrc/components/ProviderManager.tsxsrc/services/api/openaiShim.ollamaTextToolCalls.test.tssrc/utils/providerProfiles.tssrc/services/api/openaiShim.ts
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*: Keep pull requests focused on one issue or one clearly scoped improvement; avoid unrelated cleanup, fixes, features, or refactors in the same change.
Preserve existing repository patterns unless intentionally refactoring them, and stay within the project's existing language, runtime, dependency, and architectural direction.
Add or update tests when a change affects behavior.
Update documentation when setup, commands, or user-facing behavior changes.
Follow the existing code style in touched files.
Prefer small, readable changes over broad rewrites.
Do not reformat unrelated files merely because they are nearby.
Keep comments useful and concise.
Run the narrowest meaningful validation command for the touched area before opening a pull request, and ensure relevant CI checks pass before merge.
Provider-change pull requests must identify affected providers, state the tested provider/model path, and document limitations or follow-up work.
Do not assign or use provider tags; provider tags are controlled and applied by maintainers.
Security reports must follow the instructions inSECURITY.md.
PR descriptions must explain what changed and why, user or developer impact, exact checks run, and include relevant issue links; UI, terminal presentation, or VS Code extension changes require screenshots.
PR authors must address CodeRabbit findings before maintainer review proceeds.
Files:
src/services/api/agentRouting.tssrc/utils/providerProfile.test.tssrc/services/api/providerConfig.local.test.tssrc/services/api/agentRouting.test.tssrc/utils/config.tssrc/services/api/openaiShim.xmlToolCalls.test.tssrc/services/api/providerConfig.tssrc/components/ProviderManager.test.tsxsrc/utils/providerProfile.tssrc/services/api/openaiShim.test.tssrc/utils/providerProfiles.test.tssrc/components/ProviderManager.tsxsrc/services/api/openaiShim.ollamaTextToolCalls.test.tssrc/utils/providerProfiles.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/agentRouting.tssrc/utils/providerProfile.test.tssrc/services/api/providerConfig.local.test.tssrc/services/api/agentRouting.test.tssrc/utils/config.tssrc/services/api/openaiShim.xmlToolCalls.test.tssrc/services/api/providerConfig.tssrc/components/ProviderManager.test.tsxsrc/utils/providerProfile.tssrc/services/api/openaiShim.test.tssrc/utils/providerProfiles.test.tssrc/components/ProviderManager.tsxsrc/services/api/openaiShim.ollamaTextToolCalls.test.tssrc/utils/providerProfiles.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/agentRouting.tssrc/utils/providerProfile.test.tssrc/services/api/providerConfig.local.test.tssrc/services/api/agentRouting.test.tssrc/services/api/openaiShim.xmlToolCalls.test.tssrc/services/api/providerConfig.tssrc/utils/providerProfile.tssrc/services/api/openaiShim.test.tssrc/utils/providerProfiles.test.tssrc/services/api/openaiShim.ollamaTextToolCalls.test.tssrc/utils/providerProfiles.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/utils/providerProfile.test.tssrc/services/api/providerConfig.local.test.tssrc/services/api/agentRouting.test.tssrc/services/api/openaiShim.xmlToolCalls.test.tssrc/components/ProviderManager.test.tsxsrc/services/api/openaiShim.test.tssrc/utils/providerProfiles.test.tssrc/services/api/openaiShim.ollamaTextToolCalls.test.ts
src/components/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
Use React + Ink for terminal UI components under
src/components/.
Files:
src/components/ProviderManager.test.tsxsrc/components/ProviderManager.tsx
🪛 dotenv-linter (4.0.0)
.env.example
[warning] 323-323: [ExtraBlankLine] Extra blank line detected
(ExtraBlankLine)
[warning] 414-414: [ExtraBlankLine] Extra blank line detected
(ExtraBlankLine)
🔇 Additional comments (21)
src/services/api/agentRouting.ts (1)
60-64: LGTM!src/services/api/agentRouting.test.ts (1)
735-762: LGTM!src/services/api/openaiShim.test.ts (3)
8779-8881: Still missing a positive shim-level Mistral boundary regression test.Both new tests here prove non-Mistral paths do not inject
[Tool results received], but nothing exercises the Mistral/Devstral path to confirm the boundary is still actually inserted end-to-end. This is the same gap flagged in a prior review round that was never marked addressed.
8923-8927: Repeatedcontent:line at 8924/8972 — matches a previously-confirmed rendering artifact, not real duplicated source.This is the same pattern flagged in a prior review round (
TS2300concern) where the author confirmed "False alarm ...bun run typecheckpasses." No action needed unless the actual file genuinely contains 5 duplicate property signatures (worth a quickrg -n 'content: Array<{ type: string'sanity check if in doubt).Also applies to: 8971-8975
8883-8982: LGTM! Good gated-recovery coverage, including the false-positive guard for cloud prose containing example JSON.src/utils/config.ts (1)
240-246: LGTM!src/utils/providerProfile.ts (2)
83-87: LGTM!Also applies to: 175-182
2137-2179: LGTM! Correctly resolves the two prior review comments (missing carry-over and the truthy-vs-!== undefinedcheck).src/utils/providerProfiles.ts (1)
71-112: LGTM! The shell-override provenance tracking correctly resolves the prior critical bugs (profile-value leaking into the next activation, non-strict env truthiness checks).Also applies to: 126-179, 464-471, 513-513, 605-606, 871-877, 981-1002, 1075-1082, 1206-1210, 1521-1544
src/services/api/providerConfig.ts (2)
701-737: LGTM! Hostname-only matching correctly resolves the prior critical review comment about substring false positives (e.g.mistral.ai-proxy.example).Also applies to: 757-779
85-103: LGTM!Also applies to: 369-425, 1238-1271
src/utils/providerProfiles.test.ts (1)
330-520: LGTM! Good regression coverage of the exact true→false and shell-survival scenarios flagged in prior review rounds.src/utils/providerProfile.test.ts (1)
2657-2693: LGTM!src/components/ProviderManager.tsx (1)
70-70: LGTM! Both prior review findings (missing test mock export, "Automatic" incorrectly collapsing todisabled) are resolved, and the auto/enabled/disabled tri-state is threaded consistently through draft, persistence, and summary display.Also applies to: 149-149, 201-208, 265-270, 305-311, 346-355, 920-922, 1609-1610, 1731-1737, 2239-2275
.env.example (1)
321-323: Trailing extra blank line after theOPENAI_SELF_HOSTED_TOOLSentry. Already flagged previously; dotenv-linter still reportsExtraBlankLineat line 323.Source: Linters/SAST tools
src/components/ProviderManager.test.tsx (2)
714-717: Still only accepts the default and advances — no persistence assertion. Prior review asked to select "Enabled" and assert the saved payload carriesselfHostedTools: true(and mirror the Ollama preset default). The step is exercised but the saved behavior is not.As per path instructions, tests must provide meaningful coverage of changed behavior rather than only navigation.
Source: Path instructions
1388-1394: LGTM!Also applies to: 1464-1488, 2458-2467
src/services/api/providerConfig.local.test.ts (1)
96-209: LGTM!src/services/api/openaiShim.ts (1)
2188-2273: LGTM!Also applies to: 2494-2621, 4265-4289, 4633-4637
src/services/api/openaiShim.xmlToolCalls.test.ts (1)
51-265: LGTM!Also applies to: 538-791
src/services/api/openaiShim.ollamaTextToolCalls.test.ts (1)
766-819: 🩺 Stability & AvailabilityNo fetch leak here The enclosing
describerestoresglobalThis.fetchinafterEach, so this override is cleaned up automatically.> Likely an incorrect or invalid review comment.
Capture and restore OPENAI_BASE_URL/API_KEY in the Ollama text-tool gate test, and include startup self-hosted PROFILE_ENV_KEYS in RESTORED_KEYS.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Do not inherit Mistral mode into an in-process provider override
src/services/api/openaiShim.ts:4267
The override-specific environment clears the Azure and self-hosted flags but leavesCLAUDE_CODE_USE_MISTRALfrom the parent session.shouldInjectToolResultSemanticBoundary()returns true from that flag before it inspects the override URL/model, so a Mistral parent running a Qwen/OpenAI override again receives the synthetic[Tool results received]assistant message and can hit the exact post-tool stall this PR is meant to remove. Clear the parent provider selector for a complete override (or make this decision exclusively from the resolved override route). -
[P1] Honor the profile's Disabled selection for Ollama endpoints
src/services/api/openaiShim.ts:4285
shouldUseSelfHostedToolCompat()correctly treatsOPENAI_SELF_HOSTED_TOOLS=0as an explicit opt-out, but this expression separately ORs inisLikelyOllamaEndpoint(). As a result, a profile set to Disabled against an Ollama URL still buffers text and recovers JSON/XML as tool calls. Use the compatibility helper as the sole decision here, since it already handles Ollama auto-detection, and cover the explicit-disabled Ollama case. -
[P2] Persist self-hosted-tools for the specialized OpenAI startup profiles
src/utils/providerProfiles.ts:1696
The new toggle is offered to every OpenAI-compatible profile and is applied to the live process, but the NVIDIA NIM, Venice, Xiaomi Mimo, and Atlas Cloud branches serialize their startup profile through specialized builders withoutapplySelfHostedToolsProfileEnv(). Selecting Enabled therefore works until restart and is then silently lost for those profiles. Apply the field to each startup-env branch (or narrow the toggle's eligibility) and add a relaunch regression test. -
[P2] Do not strip rejected XML tool calls from the assistant response
src/services/api/openaiShim.ts:3775
In self-hosted streaming recovery, after accepting any advertised XML tool, the code stripsstripToolCallRangesfor every parsed XML block. A response containing one allowed call and one unadvertised call executes the allowed call but silently removes the rejected call from visible text, losing model output with no indication that it was not executed. Keep stripping ranges aligned with the accepted calls (while still removing duplicate accepted blocks), or leave rejected XML visible.
- P1: Clear CLAUDE_CODE_USE_MISTRAL in provider override env to prevent inheriting Mistral mode from parent session (openaiShim.ts:3419) - P2: Use shouldUseSelfHostedToolCompat as sole decision for text tool call fallback, removing separate isLikelyOllamaEndpoint check that bypassed Disabled profile setting (openaiShim.ts:3434) - P3: Apply selfHostedTools env to NVIDIA NIM, Venice, Xiaomi Mimo, and Atlas Cloud startup profiles (providerProfiles.ts:1705,1731,1744,1757) - P4: Filter stripToolCallRanges to only include ranges for accepted XML tool calls, preserving rejected calls in visible output (openaiShim.ts:2880) - Import missing functions from messageConversion.ts - Use shared textToolCallSequence counter for XML tool calls
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.ts (1)
2875-2924: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winStrip all duplicate XML tool-call blocks for filtered calls.
stripToolCallRangeskeeps every duplicate XML block, including repeats of the same accepted call, but the allowlist branch maps only one matching range per accepted unique call before assigningfilteredStripRanges. Any duplicate XML block for that accepted call is not included in the ranges passed tostripRanges(), so it remains visible in the assistant text. Track a parallel owner/unique-call index for duplicate ranges and push every range whose owner is kept. Add a regression test with the same acceptedname/argsXML block emitted twice.🤖 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 2875 - 2924, Update the allowlist filtering around recoveredCalls and filteredStripRanges so every duplicate stripToolCallRanges entry belonging to an accepted unique call is retained, not just the first matching range. Track each strip range’s owning unique-call index in parallel, then include all ranges whose owner remains in filteredCalls while preserving allowlist exclusions. Add a regression test covering the same accepted name/args XML tool-call block emitted twice and verify both blocks are stripped.
♻️ Duplicate comments (1)
src/services/api/openaiShim.test.ts (1)
8619-8721: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winStill missing a positive Mistral-boundary regression test.
These two tests only prove the negative case (non-Mistral hosts skip the boundary). No shim-level test exercises a Mistral/Devstral base URL to confirm
assistant → tool → assistant placeholder → useris still produced — so the preserved Mistral behavior this PR claims to leave "unchanged" has no regression coverage guarding it here.As per coding guidelines, "test the exact provider/model path changed when possible" and "explicitly identify affected providers."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/api/openaiShim.test.ts` around lines 8619 - 8721, Add a positive shim-level regression test alongside the existing boundary tests using a Mistral or Devstral base URL and model. Submit assistant tool_use, user tool_result, and follow-up user messages, capture the outgoing request, and assert the roles are assistant, tool, assistant, user with the assistant placeholder content present. Keep the test focused on the provider/model path that should retain semantic-boundary injection.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.ts`:
- Around line 2876-2881: Extract the allowedToolNames normalization currently
assigned to allow into a shared helper, preserving Set reuse, iterable-to-Set
conversion, and undefined for absent values. Replace this inline ternary in the
relevant consumers, including JSON-in-text and XML recovery paths, so all
allowedToolNames handling uses the same empty/unrestricted semantics.
---
Outside diff comments:
In `@src/services/api/openaiShim.ts`:
- Around line 2875-2924: Update the allowlist filtering around recoveredCalls
and filteredStripRanges so every duplicate stripToolCallRanges entry belonging
to an accepted unique call is retained, not just the first matching range. Track
each strip range’s owning unique-call index in parallel, then include all ranges
whose owner remains in filteredCalls while preserving allowlist exclusions. Add
a regression test covering the same accepted name/args XML tool-call block
emitted twice and verify both blocks are stripped.
---
Duplicate comments:
In `@src/services/api/openaiShim.test.ts`:
- Around line 8619-8721: Add a positive shim-level regression test alongside the
existing boundary tests using a Mistral or Devstral base URL and model. Submit
assistant tool_use, user tool_result, and follow-up user messages, capture the
outgoing request, and assert the roles are assistant, tool, assistant, user with
the assistant placeholder content present. Keep the test focused on the
provider/model path that should retain semantic-boundary injection.
🪄 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 Plus
Run ID: 45ef032f-eda4-42be-87ab-9b04c5fffa8a
📒 Files selected for processing (3)
src/services/api/openaiShim.test.tssrc/services/api/openaiShim.tssrc/utils/providerProfiles.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: smoke-and-tests (24.11.x)
⚠️ CI failures not shown inline (4)
GitHub Actions: PR Checks / smoke-and-tests (22): fix(provider): stop Mistral tool boundary on self-hosted OpenAI backends
Conclusion: failure
##[group]src/utils/reportTask.test.ts:
(pass) task report generation > uses an empty validation list and explicit warning when no validation was observed [3.00ms]
(pass) task report generation > captures passing validation commands from observed Bash results [2.00ms]
(pass) task report generation > captures passing validation commands from observed PowerShell results [1.00ms]
(pass) task report generation > captures failing validation commands with exit code when it is persisted [1.00ms]
(pass) task report generation > treats nonzero observed exit code as an error status [1.00ms]
(pass) task report generation > captures numeric structured exit codes from observed Bash results [1.00ms]
(pass) task report generation > reports backgrounded validation commands with unknown status [1.00ms]
(pass) task report generation > reconciles completed backgrounded validation notifications [1.00ms]
(pass) task report generation > reconciles failed backgrounded validation notifications [2.00ms]
(pass) task report generation > does not let task notifications override resolved foreground shell results [4.00ms]
(pass) task report generation > classifies validation commands from the raw Bash command before truncation [2.00ms]
(pass) task report generation > classifies validation commands inside quoted shell wrappers [2.00ms]
(pass) task report generation > classifies documented package checks as validations [5.00ms]
(pass) task report generation > captures file changes and branch metadata when available [1.00ms]
(pass) task report generation > normalizes in-repo paths whose relative path starts with dots [1.00ms]
(pass) task report generation > normalizes Windows-style tool paths before merging with git paths [1.00ms]
(pass) task report generation > prefers transcript cwd over caller cwd for git metadata [3.00ms]
(pass) task report generation > does not serialize file read result content in tool summaries [1.00ms]
(pass) task report generation > does not collect li...
GitHub Actions: PR Checks / 0_smoke-and-tests (22).txt: fix(provider): stop Mistral tool boundary on self-hosted OpenAI backends
Conclusion: failure
##[group]src/utils/reportTask.test.ts:
(pass) task report generation > uses an empty validation list and explicit warning when no validation was observed [3.00ms]
(pass) task report generation > captures passing validation commands from observed Bash results [2.00ms]
(pass) task report generation > captures passing validation commands from observed PowerShell results [1.00ms]
(pass) task report generation > captures failing validation commands with exit code when it is persisted [1.00ms]
(pass) task report generation > treats nonzero observed exit code as an error status [1.00ms]
(pass) task report generation > captures numeric structured exit codes from observed Bash results [1.00ms]
(pass) task report generation > reports backgrounded validation commands with unknown status [1.00ms]
(pass) task report generation > reconciles completed backgrounded validation notifications [1.00ms]
(pass) task report generation > reconciles failed backgrounded validation notifications [2.00ms]
(pass) task report generation > does not let task notifications override resolved foreground shell results [4.00ms]
(pass) task report generation > classifies validation commands from the raw Bash command before truncation [2.00ms]
(pass) task report generation > classifies validation commands inside quoted shell wrappers [2.00ms]
(pass) task report generation > classifies documented package checks as validations [5.00ms]
(pass) task report generation > captures file changes and branch metadata when available [1.00ms]
(pass) task report generation > normalizes in-repo paths whose relative path starts with dots [1.00ms]
(pass) task report generation > normalizes Windows-style tool paths before merging with git paths [1.00ms]
(pass) task report generation > prefers transcript cwd over caller cwd for git metadata [3.00ms]
(pass) task report generation > does not serialize file read result content in tool summaries [1.00ms]
(pass) task report generation > does not collect li...
GitHub Actions: PR Checks / smoke-and-tests (24.11.x): fix(provider): stop Mistral tool boundary on self-hosted OpenAI backends
Conclusion: failure
##[group]src/utils/reportTask.test.ts:
(pass) task report generation > uses an empty validation list and explicit warning when no validation was observed [3.00ms]
(pass) task report generation > captures passing validation commands from observed Bash results [2.00ms]
(pass) task report generation > captures passing validation commands from observed PowerShell results [1.00ms]
(pass) task report generation > captures failing validation commands with exit code when it is persisted
(pass) task report generation > treats nonzero observed exit code as an error status [1.00ms]
(pass) task report generation > captures numeric structured exit codes from observed Bash results
(pass) task report generation > reports backgrounded validation commands with unknown status [1.00ms]
(pass) task report generation > reconciles completed backgrounded validation notifications [1.00ms]
(pass) task report generation > reconciles failed backgrounded validation notifications
(pass) task report generation > does not let task notifications override resolved foreground shell results
(pass) task report generation > classifies validation commands from the raw Bash command before truncation [1.00ms]
(pass) task report generation > classifies validation commands inside quoted shell wrappers [1.00ms]
(pass) task report generation > classifies documented package checks as validations [9.00ms]
(pass) task report generation > captures file changes and branch metadata when available [1.00ms]
(pass) task report generation > normalizes in-repo paths whose relative path starts with dots
(pass) task report generation > normalizes Windows-style tool paths before merging with git paths [1.00ms]
(pass) task report generation > prefers transcript cwd over caller cwd for git metadata [1.00ms]
(pass) task report generation > does not serialize file read result content in tool summaries
(pass) task report generation > does not collect linked references from tool result content [1.00ms]
(pa...
GitHub Actions: PR Checks / 1_smoke-and-tests (24.11.x).txt: fix(provider): stop Mistral tool boundary on self-hosted OpenAI backends
Conclusion: failure
##[group]src/utils/reportTask.test.ts:
(pass) task report generation > uses an empty validation list and explicit warning when no validation was observed [3.00ms]
(pass) task report generation > captures passing validation commands from observed Bash results [2.00ms]
(pass) task report generation > captures passing validation commands from observed PowerShell results [1.00ms]
(pass) task report generation > captures failing validation commands with exit code when it is persisted
(pass) task report generation > treats nonzero observed exit code as an error status [1.00ms]
(pass) task report generation > captures numeric structured exit codes from observed Bash results
(pass) task report generation > reports backgrounded validation commands with unknown status [1.00ms]
(pass) task report generation > reconciles completed backgrounded validation notifications [1.00ms]
(pass) task report generation > reconciles failed backgrounded validation notifications
(pass) task report generation > does not let task notifications override resolved foreground shell results
(pass) task report generation > classifies validation commands from the raw Bash command before truncation [1.00ms]
(pass) task report generation > classifies validation commands inside quoted shell wrappers [1.00ms]
(pass) task report generation > classifies documented package checks as validations [9.00ms]
(pass) task report generation > captures file changes and branch metadata when available [1.00ms]
(pass) task report generation > normalizes in-repo paths whose relative path starts with dots
(pass) task report generation > normalizes Windows-style tool paths before merging with git paths [1.00ms]
(pass) task report generation > prefers transcript cwd over caller cwd for git metadata [1.00ms]
(pass) task report generation > does not serialize file read result content in tool summaries
(pass) task report generation > does not collect linked references from tool result content [1.00ms]
(pa...
🧰 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.test.tssrc/utils/providerProfiles.tssrc/services/api/openaiShim.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.test.tssrc/utils/providerProfiles.tssrc/services/api/openaiShim.ts
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/services/api/openaiShim.test.tssrc/utils/providerProfiles.tssrc/services/api/openaiShim.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.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.test.tssrc/utils/providerProfiles.tssrc/services/api/openaiShim.ts
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/services/api/openaiShim.test.ts
🔇 Additional comments (13)
src/utils/providerProfiles.ts (1)
1705-1705: LGTM!Also applies to: 1731-1731, 1744-1744, 1757-1757
src/services/api/openaiShim.ts (10)
5108-5160: Unchanged in this PR; no concerns.
45-46: LGTM!Also applies to: 144-171
418-432: LGTM!
745-753: LGTM!Also applies to: 1010-1024
1388-1445: LGTM!
1538-1543: LGTM!
1673-1702: LGTM!Also applies to: 1738-1800
3987-3991: LGTM!Also applies to: 4035-4046, 4080-4082, 4213-4226, 5029-5034
3412-3438: LGTM!
3461-3468: LGTM!Also applies to: 3503-3508, 3533-3538, 5096-5106
src/services/api/openaiShim.test.ts (2)
8763-8767: 🎯 Functional CorrectnessLikely annotation-tool artifact, not real duplicate code.
Same repeated-line pattern flagged before and confirmed a false alarm (
bun run typecheckpassed). Not re-raising as a blocker — flagging only for awareness in case the rendering artifact obscures a real diff here.Also applies to: 8811-8815
8723-8821: LGTM!
…te stripping + Mistral semantic boundary test - Add normalizeAllowedToolNames() helper to avoid inline ternary duplication - Fix XML tool call allowlist filtering to strip ALL duplicate blocks for accepted calls using content-based matching - Add regression test for Mistral semantic boundary injection - Add regression test for duplicate XML block stripping
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 2890-2937: Update parseXmlToolCalls to return a
stripRangeOwnerIndex array parallel to stripToolCallRanges, recording each
range’s owning toolCallRanges index. Replace the content comparisons in the
acceptedCallIndices filtering block with direct owner-index lookups, and update
the companion XML tool-call tests to exercise this returned mapping instead of
duplicating the ownership algorithm.
In `@src/services/api/openaiShim.xmlToolCalls.test.ts`:
- Around line 266-309: Replace the handcrafted filtering logic in the
duplicate-block test with an integration-style test using createOpenAIShimClient
and a matching tools allowlist. Stream two duplicate XML tool-call blocks
through the real OpenAI shim path, then assert the emitted tool_use and
text_delta events show both accepted calls stripped from text while preserving
“next”. Remove the direct parseXmlToolCalls and inline range-matching assertions
so the test covers the production allowlist filtering behavior.
🪄 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 Plus
Run ID: c6375b98-cc00-4fae-97db-a49e0b8f3af1
📒 Files selected for processing (3)
src/services/api/openaiShim.test.tssrc/services/api/openaiShim.tssrc/services/api/openaiShim.xmlToolCalls.test.ts
📜 Review details
⚠️ CI failures not shown inline (4)
GitHub Actions: PR Checks / 1_smoke-and-tests (24.11.x).txt: fix(provider): stop Mistral tool boundary on self-hosted OpenAI backends
Conclusion: failure
##[group]src/utils/reportTask.test.ts:
(pass) task report generation > uses an empty validation list and explicit warning when no validation was observed [3.00ms]
(pass) task report generation > captures passing validation commands from observed Bash results [2.00ms]
(pass) task report generation > captures passing validation commands from observed PowerShell results
(pass) task report generation > captures failing validation commands with exit code when it is persisted [1.00ms]
(pass) task report generation > treats nonzero observed exit code as an error status [1.00ms]
(pass) task report generation > captures numeric structured exit codes from observed Bash results [1.00ms]
(pass) task report generation > reports backgrounded validation commands with unknown status [1.00ms]
(pass) task report generation > reconciles completed backgrounded validation notifications [1.00ms]
(pass) task report generation > reconciles failed backgrounded validation notifications [1.00ms]
(pass) task report generation > does not let task notifications override resolved foreground shell results [1.00ms]
(pass) task report generation > classifies validation commands from the raw Bash command before truncation [2.00ms]
(pass) task report generation > classifies validation commands inside quoted shell wrappers [4.00ms]
(pass) task report generation > classifies documented package checks as validations [4.00ms]
(pass) task report generation > captures file changes and branch metadata when available [2.00ms]
(pass) task report generation > normalizes in-repo paths whose relative path starts with dots [1.00ms]
(pass) task report generation > normalizes Windows-style tool paths before merging with git paths [1.00ms]
(pass) task report generation > prefers transcript cwd over caller cwd for git metadata
(pass) task report generation > does not serialize file read result content in tool summaries [1.00ms]
(pass) task report generation > does not collect linked references fr...
GitHub Actions: PR Checks / smoke-and-tests (22): fix(provider): stop Mistral tool boundary on self-hosted OpenAI backends
Conclusion: failure
##[group]src/utils/reportTask.test.ts:
(pass) task report generation > uses an empty validation list and explicit warning when no validation was observed [2.00ms]
(pass) task report generation > captures passing validation commands from observed Bash results [1.00ms]
(pass) task report generation > captures passing validation commands from observed PowerShell results [1.00ms]
(pass) task report generation > captures failing validation commands with exit code when it is persisted [1.00ms]
(pass) task report generation > treats nonzero observed exit code as an error status
(pass) task report generation > captures numeric structured exit codes from observed Bash results [1.00ms]
(pass) task report generation > reports backgrounded validation commands with unknown status [1.00ms]
(pass) task report generation > reconciles completed backgrounded validation notifications [1.00ms]
(pass) task report generation > reconciles failed backgrounded validation notifications
(pass) task report generation > does not let task notifications override resolved foreground shell results
(pass) task report generation > classifies validation commands from the raw Bash command before truncation [2.00ms]
(pass) task report generation > classifies validation commands inside quoted shell wrappers [1.00ms]
(pass) task report generation > classifies documented package checks as validations [5.00ms]
(pass) task report generation > captures file changes and branch metadata when available [2.00ms]
(pass) task report generation > normalizes in-repo paths whose relative path starts with dots
(pass) task report generation > normalizes Windows-style tool paths before merging with git paths [1.00ms]
(pass) task report generation > prefers transcript cwd over caller cwd for git metadata
(pass) task report generation > does not serialize file read result content in tool summaries [1.00ms]
(pass) task report generation > does not collect linked references from tool result content [1.0...
GitHub Actions: PR Checks / smoke-and-tests (24.11.x): fix(provider): stop Mistral tool boundary on self-hosted OpenAI backends
Conclusion: failure
##[group]src/utils/reportTask.test.ts:
(pass) task report generation > uses an empty validation list and explicit warning when no validation was observed [3.00ms]
(pass) task report generation > captures passing validation commands from observed Bash results [2.00ms]
(pass) task report generation > captures passing validation commands from observed PowerShell results
(pass) task report generation > captures failing validation commands with exit code when it is persisted [1.00ms]
(pass) task report generation > treats nonzero observed exit code as an error status [1.00ms]
(pass) task report generation > captures numeric structured exit codes from observed Bash results [1.00ms]
(pass) task report generation > reports backgrounded validation commands with unknown status [1.00ms]
(pass) task report generation > reconciles completed backgrounded validation notifications [1.00ms]
(pass) task report generation > reconciles failed backgrounded validation notifications [1.00ms]
(pass) task report generation > does not let task notifications override resolved foreground shell results [1.00ms]
(pass) task report generation > classifies validation commands from the raw Bash command before truncation [2.00ms]
(pass) task report generation > classifies validation commands inside quoted shell wrappers [4.00ms]
(pass) task report generation > classifies documented package checks as validations [4.00ms]
(pass) task report generation > captures file changes and branch metadata when available [2.00ms]
(pass) task report generation > normalizes in-repo paths whose relative path starts with dots [1.00ms]
(pass) task report generation > normalizes Windows-style tool paths before merging with git paths [1.00ms]
(pass) task report generation > prefers transcript cwd over caller cwd for git metadata
(pass) task report generation > does not serialize file read result content in tool summaries [1.00ms]
(pass) task report generation > does not collect linked references fr...
GitHub Actions: PR Checks / 3_smoke-and-tests (22).txt: fix(provider): stop Mistral tool boundary on self-hosted OpenAI backends
Conclusion: failure
##[group]src/utils/reportTask.test.ts:
(pass) task report generation > uses an empty validation list and explicit warning when no validation was observed [2.00ms]
(pass) task report generation > captures passing validation commands from observed Bash results [1.00ms]
(pass) task report generation > captures passing validation commands from observed PowerShell results [1.00ms]
(pass) task report generation > captures failing validation commands with exit code when it is persisted [1.00ms]
(pass) task report generation > treats nonzero observed exit code as an error status
(pass) task report generation > captures numeric structured exit codes from observed Bash results [1.00ms]
(pass) task report generation > reports backgrounded validation commands with unknown status [1.00ms]
(pass) task report generation > reconciles completed backgrounded validation notifications [1.00ms]
(pass) task report generation > reconciles failed backgrounded validation notifications
(pass) task report generation > does not let task notifications override resolved foreground shell results
(pass) task report generation > classifies validation commands from the raw Bash command before truncation [2.00ms]
(pass) task report generation > classifies validation commands inside quoted shell wrappers [1.00ms]
(pass) task report generation > classifies documented package checks as validations [5.00ms]
(pass) task report generation > captures file changes and branch metadata when available [2.00ms]
(pass) task report generation > normalizes in-repo paths whose relative path starts with dots
(pass) task report generation > normalizes Windows-style tool paths before merging with git paths [1.00ms]
(pass) task report generation > prefers transcript cwd over caller cwd for git metadata
(pass) task report generation > does not serialize file read result content in tool summaries [1.00ms]
(pass) task report generation > does not collect linked references from tool result content [1.0...
🧰 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.xmlToolCalls.test.tssrc/services/api/openaiShim.test.tssrc/services/api/openaiShim.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.xmlToolCalls.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.xmlToolCalls.test.tssrc/services/api/openaiShim.test.tssrc/services/api/openaiShim.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.xmlToolCalls.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.xmlToolCalls.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.xmlToolCalls.test.tssrc/services/api/openaiShim.test.ts
🔇 Additional comments (3)
src/services/api/openaiShim.ts (2)
1545-1558: Good extraction of the allowlist-normalization helper.This addresses the earlier nitpick about the
instanceof Set ? ... : ...ternary drifting across call sites.
2919-2924: 🎯 Functional CorrectnessNo change needed:
noUncheckedIndexedAccessis not enabled.
tsconfig.jsonenablesstrict, but the repo does not setnoUncheckedIndexedAccess, soxmlParsed.toolCallRanges[j]![0]/[1]would not be required by the current project config.> Likely an incorrect or invalid review comment.src/services/api/openaiShim.test.ts (1)
8619-8671: 🩺 Stability & AvailabilityNo change needed for env cleanup.
afterEachrestoresOPENAI_BASE_URLandOPENAI_API_KEYfrom the savedoriginalEnvstate, so this Mistral test does not leave stale env vars for later tests.
…rship mapping - Add stripRangeOwnerIndex array parallel to stripToolCallRanges - Each entry records the owning toolCallRanges/calls index - Replaces O(n*m) content-based matching with O(1) index lookup - Update unit test to verify stripRangeOwnerIndex values - Replace integration test with production allowlist filtering behavior test covering duplicate XML blocks stripping and deduping
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.xmlToolCalls.test.ts`:
- Around line 312-324: Update the tool definition in the test around the
`client.beta.messages.create` call to use the Anthropic shape expected by
`toolNamesFromShimParams()`: provide the tool’s top-level `name` and
`input_schema` fields instead of nested OpenAI `function` fields. Keep the
duplicate-tool scenario and assertions unchanged so the test exercises
`allowedToolNames` filtering rather than fallback behavior.
- Around line 278-283: Restore the mutated global state in this test: capture
the original OPENAI_BASE_URL, OPENAI_API_KEY, and globalThis.fetch before
overriding them, then restore each value in teardown after the test. Follow the
existing beforeEach/afterEach isolation pattern used by the self-hosted testing
describe blocks, anchoring the changes to the test containing capturedBody and
its fetch override.
🪄 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 Plus
Run ID: baef00f7-506f-4a01-9cd6-a2b69710c6c3
📒 Files selected for processing (2)
src/services/api/openaiShim.tssrc/services/api/openaiShim.xmlToolCalls.test.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}: 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.xmlToolCalls.test.tssrc/services/api/openaiShim.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.xmlToolCalls.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.xmlToolCalls.test.tssrc/services/api/openaiShim.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.xmlToolCalls.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.xmlToolCalls.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.xmlToolCalls.test.ts
🔇 Additional comments (6)
src/services/api/openaiShim.ts (5)
745-745: LGTM!
1687-1828: LGTM!
2147-2167: LGTM!
2925-2929: LGTM!
2127-2146: 🎯 Functional CorrectnessNo change needed: XML recovery is allowlisted in the non-streaming path.
parseXmlToolCallsalready owns the XML parsing; this branch filters accepted call indices and corresponding strips before emittingtool_useblocks, while keeping XML recovery enabled for HY3-specific provider support.> Likely an incorrect or invalid review comment.src/services/api/openaiShim.xmlToolCalls.test.ts (1)
257-267: LGTM!
- Capture and restore OPENAI_BASE_URL, OPENAI_API_KEY, and fetch in teardown
- Use Anthropic-shaped tool params { name, input_schema } instead of OpenAI { type, function }
- Align with toolNamesFromShimParams() expected shape
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.xmlToolCalls.test.ts`:
- Around line 278-280: Update the test teardown around the environment snapshot
and restoration logic in the affected test cases to run through afterEach or a
try/finally, ensuring OPENAI_BASE_URL, OPENAI_API_KEY, and globalThis.fetch are
restored even when requests or assertions fail. Apply the same cleanup guarantee
to the logic covering the additional referenced section.
🪄 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 Plus
Run ID: c08e2d8b-2175-4eb3-b8c2-eb9c9a7e3349
📒 Files selected for processing (1)
src/services/api/openaiShim.xmlToolCalls.test.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.xmlToolCalls.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.xmlToolCalls.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.xmlToolCalls.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.xmlToolCalls.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.xmlToolCalls.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.xmlToolCalls.test.ts
🔇 Additional comments (1)
src/services/api/openaiShim.xmlToolCalls.test.ts (1)
319-343: 📐 Maintainability & Code QualityRun the focused XML recovery suite locally.
bun test src/services/api/openaiShim.xmlToolCalls.test.tsneeds to be reported in the PR to show the changed XML tool call behavior is covered;bun run smokeis not sufficient.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Please rebase on main to resolve smoke issues.
Additionally openaiShim.ts will likely need to be rebased multiple times as its currently being refactored to demonolith it.
Findings
-
[P1] Restore immutable module snapshots in the user test cleanup
src/utils/user.test.ts:19-25, 97-104
mock.module()replaces the live namespace bindings for the rest of the Bun process. This change removes the pre-mock plain-object snapshots, then restoresauth,config,env,envUtils, andexecafrom those live bindings (and dynamically importsexecaafter an earlier mock may already exist). The cleanup can therefore permanently reinstall the test stub. The current required CI run demonstrates this: later task-report tests receive the stub withoutstderrand crash atinside.stderr.trim(), while the ads tests retain the mocked config. Preserve and restore the pre-mock snapshots instead. -
[P1] Honor Disabled for XML text-tool recovery
src/services/api/openaiShim.ts:2129-2146, 2670-2701, 3021-3045
A profile saved with Self-hosted tools: Disabled emitsOPENAI_SELF_HOSTED_TOOLS=0, andshouldUseSelfHostedToolCompat()correctly returns false. That value only disables the JSON buffering path, though: the non-streaming converter always parses XML, and the streaming converter enters its XML holdback/recovery path wheneverisOllamaStreamis false. A local Qwen/GLM-style response containing<tool_call>will consequently still be converted intotool_use, despite the UI promising “structured API tool_calls only.” Gate XML recovery on the same compatibility decision, with disabled-profile streaming and non-streaming regressions. -
[P2] Persist the self-hosted-tools setting for xAI OAuth profiles
src/utils/providerProfiles.ts:1774-1781
The xAI OAuth/no-API-key startup branch builds its saved environment withoutapplySelfHostedToolsProfileEnv. The provider is classified as OpenAI-compatible, so the UI exposes this control and same-session activation applies it, but selecting Enabled or Disabled on such a profile is dropped whensetActiveProviderProfile()writes the startup profile. On the next launch it reverts to automatic detection. Apply the profile setting in this branch as in the other OpenAI-compatible startup branches. -
[P2] Make the XML recovery test restore globals on every exit path
src/services/api/openaiShim.xmlToolCalls.test.ts:278-360
This test mutatesOPENAI_BASE_URL,OPENAI_API_KEY, andglobalThis.fetch, but restores them only after its successful assertions. A rejected request or failed assertion skips that restoration and leaks the localhost endpoint/fetch stub into subsequent tests, creating order-dependent failures and masking. Put the restoration inafterEachor atry/finally.
4 participants
Summary
Self-hosted OpenAI-compatible backends (llama-server, vLLM, Ollama, custom hosts) were stalling after tool calls because the OpenAI shim always injected a synthetic assistant message
[Tool results received]betweentoolanduserroles. That boundary is only valid for Mistral/Devstral Jinja templates; Qwen and similar models often echo the placeholder and end the turn with no further tool calls.This PR:
CLAUDE_CODE_USE_MISTRAL,mistral.ai, or model names matching mistral/devstral/ministral).selfHostedTools(UI: “Self-hosted tools” in/provider) so JSON-in-text tool recovery is scoped to the active provider without requiring shell env..local/ Ollama-like endpoints; public hosts use the profile flag (or optionalOPENAI_SELF_HOSTED_TOOLS=1override).Cloud providers and Mistral behaviour are intentionally unchanged.
Motivation
Users on llama-server (any host/port/domain) with Qwen-class models saw turns stop after tools with visible
[Tool results received]in the UI. Previous approaches tried to treat that echo as a continuation signal; the correct fix is not to inject the boundary for non-Mistral backends, and to opt self-hosted tool recovery in per provider profile.Changes
providerConfig.tsshouldInjectToolResultSemanticBoundary,shouldUseSelfHostedToolCompat, constantsopenaiShim.tsconfig.ts/providerProfiles*selfHostedTools; apply/clear via profile env lifecycleProviderManager.tsx.env.exampleTest plan
bun test ./src/services/api/providerConfig.local.test.tsbun test ./src/services/api/openaiShim.test.tsbun test ./src/services/api/openaiShim.ollamaTextToolCalls.test.ts(related stream path)bun test ./src/utils/providerProfiles.test.ts(includes selfHostedTools apply/clear)bun test ./src/components/ProviderManager.test.tsx[Tool results received]stallhttp://myhost.com:PORT/v1) and confirm tools continueNotes for reviewers
fix_continuemarker-stall/nudge logic inquery.ts; this is the shim/provider-scoped fix.OPENAI_SELF_HOSTED_TOOLS/OPENAI_PARSE_TEXT_TOOL_CALLSremain as optional shell overrides; preferred path is the profile toggle.Summary by CodeRabbit
.env.examplewith a llama-server (OpenAI-compatible) provider option and related flags.