Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (9)
📜 Recent review details🧰 Additional context used📓 Path-based instructions (5)**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
⚙️ CodeRabbit configuration file
Files:
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}⚙️ CodeRabbit configuration file
Files:
{README.md,CONTRIBUTING.md,docs/**,.github/pull_request_template.md}⚙️ CodeRabbit configuration file
Files:
{src/services/api/**,src/integrations/**,src/utils/model/**,src/utils/provider*.ts,src/commands/provider/**}⚙️ CodeRabbit configuration file
Files:
🔇 Additional comments (10)
📝 WalkthroughWalkthroughOpenClaude now applies provider-aware tool-result boundary injection, suppresses placeholder echoes in converted history, detects echoed placeholders as continuation signals, expands local endpoint classification, and documents local OpenAI-compatible server configuration. ChangesSemantic tool-result boundary handling
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 6 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (6 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 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/providerConfig.ts`:
- Around line 662-682: Remove the mappedDotted declaration and its associated
IPv4 conversion and return logic from the hostname classification code. Keep the
mappedHex handling unchanged, as it processes the normalized IPv4-mapped IPv6
form used by current callers.
🪄 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: 7b4ff0a1-3ac2-40d6-accd-0ca26b2e3643
📒 Files selected for processing (9)
docs/advanced-setup.mdsrc/__tests__/bugfixes.test.tssrc/services/api/openaiShim.test.tssrc/services/api/openaiShim.tssrc/services/api/openaiShim/messageConversion.test.tssrc/services/api/openaiShim/messageConversion.tssrc/services/api/providerConfig.local.test.tssrc/services/api/providerConfig.tssrc/utils/continuation.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}: Add or update tests when a TypeScript or TSX change affects behavior.
Run the relevant TypeScript validation checks for changed code, includingbun run typecheckand, when applicable,bun run typecheck:type-tests.
Files:
src/__tests__/bugfixes.test.tssrc/utils/continuation.tssrc/services/api/providerConfig.local.test.tssrc/services/api/openaiShim/messageConversion.test.tssrc/services/api/openaiShim.test.tssrc/services/api/openaiShim/messageConversion.tssrc/services/api/openaiShim.tssrc/services/api/providerConfig.ts
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*: Preserve existing repository patterns unless intentionally refactoring them.
Keep changes small, readable, and focused; avoid broad rewrites or unrelated cleanup.
Do not reformat unrelated files, and keep comments useful and concise.
Update documentation when setup, commands, or user-facing behavior changes.
Review AI-generated changes for correctness, style consistency, unnecessary noise, and adherence to project architecture before submitting them.
Provider changes must follow the documented integration patterns indocs/integrations/overview.mdand the focused guides underdocs/integrations/how-to/.
When changing provider behavior, avoid breaking third-party providers and test the exact provider/model path changed when possible.
Provider pull requests must explicitly identify affected providers, limitations, and follow-up work.
Run the narrowest meaningful validation command for the touched area, and ensure relevant CI checks pass before merging.
Usebun installto install dependencies and the repository's Bun scripts for building, testing, smoke testing, and development.
Dependency changes require a concrete project benefit such as a bug fix, security issue, or approved feature; preference alone is insufficient.
Do not change the project's language, core runtime, or dependency stack, or introduce a new runtime, without prior maintainer agreement.
Keep each pull request focused on one issue or clearly scoped improvement and avoid bundling unrelated fixes, features, or refactors.
Files:
src/__tests__/bugfixes.test.tsdocs/advanced-setup.mdsrc/utils/continuation.tssrc/services/api/providerConfig.local.test.tssrc/services/api/openaiShim/messageConversion.test.tssrc/services/api/openaiShim.test.tssrc/services/api/openaiShim/messageConversion.tssrc/services/api/openaiShim.tssrc/services/api/providerConfig.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/__tests__/bugfixes.test.tsdocs/advanced-setup.mdsrc/utils/continuation.tssrc/services/api/providerConfig.local.test.tssrc/services/api/openaiShim/messageConversion.test.tssrc/services/api/openaiShim.test.tssrc/services/api/openaiShim/messageConversion.tssrc/services/api/openaiShim.tssrc/services/api/providerConfig.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/__tests__/bugfixes.test.tssrc/services/api/providerConfig.local.test.tssrc/services/api/openaiShim/messageConversion.test.tssrc/services/api/openaiShim.test.ts
{README.md,CONTRIBUTING.md,docs/**,.github/pull_request_template.md}
⚙️ CodeRabbit configuration file
{README.md,CONTRIBUTING.md,docs/**,.github/pull_request_template.md}: Review docs for accuracy against current code behavior. Flag security or provider claims that overpromise, stale install commands, missing setup caveats, and instructions that could push users toward unsafe credential handling. Keep purely wording-level suggestions non-blocking.
Files:
docs/advanced-setup.md
{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/messageConversion.test.tssrc/services/api/openaiShim.test.tssrc/services/api/openaiShim/messageConversion.tssrc/services/api/openaiShim.tssrc/services/api/providerConfig.ts
🔇 Additional comments (9)
src/services/api/providerConfig.ts (1)
304-306: 🚀 Performance & ScalabilityCGNAT range now treated as "local" everywhere
isLocalProviderUrlis consulted.Classifying
100.64.0.0/10as local is reasonable for Tailscale/self-hosted setups, butisLocalProviderUrlgates more than the semantic-boundary decision — it also drivesstream_optionsomission, fast-path skipping of tool-history compression/stable-stringify, and local retry-base-URL promotion inopenaiShim.ts. CGNAT space is also used by some ISPs for public-facing NAT, so a legitimately remote (non-self-hosted) endpoint reachable via a CGNAT-assigned address would silently get the "local" fast path treatment across all of these unrelated behaviors. Likely an acceptable tradeoff given the target use case, but worth confirming the wider blast radius is intended rather than scoped only to this feature.src/services/api/providerConfig.local.test.ts (1)
12-13: LGTM!Also applies to: 81-93, 102-212
src/services/api/openaiShim/messageConversion.ts (1)
1-5: LGTM!Also applies to: 24-30, 161-169, 228-232, 283-309
src/services/api/openaiShim.ts (1)
110-111: LGTM!Also applies to: 770-771, 1137-1146, 1479-1484
src/services/api/openaiShim/messageConversion.test.ts (1)
139-140: LGTM!Also applies to: 164-176, 177-201, 203-222, 224-235, 237-252
src/services/api/openaiShim.test.ts (1)
46-46: LGTM!Also applies to: 557-557, 8364-8367, 8502-8593
src/utils/continuation.ts (1)
2-2: LGTM!Also applies to: 108-118, 141-146
src/__tests__/bugfixes.test.ts (1)
114-126: LGTM!docs/advanced-setup.md (1)
236-242: 📐 Maintainability & Code Quality | ⚡ Quick winDoc list of local TLDs is incomplete vs. code.
isLocalProviderUrlinproviderConfig.tsalso treats.internaland.intranetsuffixes as local, but this doc only mentions.local/.localhost/.home.arpa/.lan. A self-hoster reasoning about whether their hostname (e.g.foo.internal) avoids the Mistral placeholder should see the full list. Also,.lanisn't actually an RFC/ICANN-reserved TLD (only.local/.localhost/.home.arpaare RFC-reserved and.internalis ICANN-reserved as of 2024) — worth softening that wording too.As per path instructions, "Review docs for accuracy against current code behavior."
📝 Suggested fix
-OpenClaude never inserts the Mistral/Devstral tool-boundary placeholder -(`[Tool results received]`) for local OpenAI-compatible hosts (loopback, -RFC1918, CGNAT/`100.64/10`, `host.docker.internal`, reserved TLDs such as -`.local` / `.localhost` / `.home.arpa` / `.lan`) or Ollama endpoints. That +OpenClaude never inserts the Mistral/Devstral tool-boundary placeholder +(`[Tool results received]`) for local OpenAI-compatible hosts (loopback, +RFC1918, CGNAT/`100.64/10`, `host.docker.internal`, local-only suffixes such +as `.local` / `.localhost` / `.home.arpa` / `.lan` / `.internal` / +`.intranet`) or Ollama endpoints. ThatSource: Path instructions
|
Addressed CI + CodeRabbit on
Focused tests + |
…ackends Gate the synthetic "[Tool results received]" assistant boundary to Mistral/Devstral routes so local OpenAI-compatible models stop echoing it as end_turn, and nudge continuation if a placeholder-only reply still appears.
Prevent a Mistral parent session from re-injecting the tool-result semantic placeholder into a non-Mistral providerOverride route.
Block the Mistral semantic placeholder on local OpenAI-compatible hosts, strip prior placeholder-only echoes from history, share the placeholder constant for continuation recovery, and cover snip-reminder tool boundaries.
Reuse the shared placeholder echo normalizer when stripping history, and skip injection on Ollama endpoints whose hostname is not loopback.
Include mixtral/magistral/mathstral in the tool-boundary injection heuristic so non-local Mixtral routes keep Jinja-compatible sequencing.
Treat host.docker.internal and Tailscale CGNAT addresses as local so Mistral-class model ids on those backends do not re-enable placeholder injection, and strengthen the providerOverride regression test.
Unwrap ::ffff:a.b.c.d hostnames in isLocalProviderUrl so dual-stack loopback/RFC1918/CGNAT URLs keep the same tool-boundary exclusion as IPv4.
URL parsers expand ::ffff:127.0.0.1 to ::ffff:7f00:1; classify both forms as local for tool-boundary exclusion.
Align isLocalProviderUrl with cacheMetrics reserved-TLD classification so .localhost/.home.arpa/.lan hosts do not re-enable placeholder injection.
Stop treating corporate .internal/.intranet hosts as local so StartupScreen proxy fallbacks stay generic, and drop the unreachable dotted IPv4-mapped branch.
4e82b45 to
7e1cea7
Compare
|
Confirmed on a cloud OpenAI-compatible gateway too — not just local backends. Running against the opencode.ai gateway (
Every stall: the model echoes the injected placeholder verbatim as its entire reply (6 output tokens) and returns Since PR is currently a draft and has a merge conflict with main — happy to help rebase if useful. Any ETA on moving this to ready-for-review? |
2 participants
Summary
[Tool results received]assistant boundary so it is no longer inserted on everytool→userconversion. Local OpenAI-compatible hosts (loopback, RFC1918, CGNAT, Docker internal, reserved TLDs) and Ollama never get it; Mistral cloud /CLAUDE_CODE_USE_MISTRAL/ Mistral-class model ids on non-local routes still can for Jinja sequencing.CLAUDE_CODE_USE_MISTRALonproviderOverride, and treat placeholder-only model replies as a continuation nudge.docs/advanced-setup.md.Fixes #2059
Fixes #2039
Related incomplete work: #1977 (still conflicting / changes-requested; this PR is a focused root-cause fix on current
main).Impact
[Tool results received].shouldInjectToolResultSemanticBoundaryis the single decision point;isLocalProviderUrlnow matches cacheMetrics reserved-TLD / CGNAT classification more closely.Testing
bun run buildbun run smokebun run checkbun test ./src/services/api/openaiShim/messageConversion.test.ts ./src/services/api/providerConfig.local.test.ts ./src/__tests__/bugfixes.test.ts ./src/services/api/openaiShim.test.ts(371 pass)Notes
CLAUDE_CODE_USE_MISTRAL=1on a remote non-Mistral primary route still injects (product flag semantics); overrides clear the flag.--printmay briefly stream an echoed placeholder before the continuation nudge recovers.Summary by CodeRabbit
New Features
Bug Fixes
Documentation