Repository navigation
fix(openai-shim): recover GLM/Qwen XML tool calls emitted as text - #1791
Conversation
GLM/Qwen-family models routed through OpenAI-compatible gateways emit tool calls as XML text (`<tool_call><function=…>`) instead of structured `tool_calls`. The shim had no parser, so these leaked into visible prose and never executed — the turn ended with no tool_use block and the agent appeared to "forget" and stop mid-task. Add `parseXmlToolCalls` covering the three dialects seen in the wild (function/parameter, GLM-native arg_key/arg_value, Hermes JSON), wired into both the streaming and non-streaming paths. The streaming path holds back text from `<tool_call>` onward (incl. an opener split across SSE deltas), converts it to tool_use blocks at finalize, and flips the finish reason to tool_calls — mirroring the existing Ollama fallback. Structured tool_calls and false-positive prose are handled losslessly. Co-Authored-By: OpenClaude <openclaude@gitlawb.com>
📝 WalkthroughWalkthroughThe OpenAI shim now recovers XML-wrapped tool calls from compatible providers in streaming and non-streaming responses. It buffers chunked XML, parses three XML formats, strips recovered text, and adds coverage for the new paths. ChangesXML tool-call recovery
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/services/api/openaiShim.ts (1)
2141-2191: 🎯 Functional Correctness | 🟠 MajorStreaming finalize inherits the dedup-range leak above.
stripRanges(buffered, toolCallRanges)at Line 2150 strips only recorded ranges, so a duplicated<tool_call>block survives intostrippedVisibleand is emitted as text. No separate fix needed here — resolving the root cause inparseXmlToolCallscovers this path.🤖 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 2141 - 2191, The streaming finalize path in openaiShim.ts inherits the duplicate-range issue from parseXmlToolCalls, so a repeated <tool_call> block can still survive stripRanges(buffered, toolCallRanges) and be emitted as text. Fix the root cause in parseXmlToolCalls so it returns correct, non-overlapping toolCallRanges for duplicated XML tool calls; this emit path in the finalize block should then work unchanged.
🤖 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 1338-1343: The dedup logic in openaiShim’s tool-call parsing is
skipping range tracking for repeated named blocks, which leaves duplicate
<tool_call> XML visible in stripped output. In the parsing path around the
results/ranges handling, record every valid named block’s range before any dedup
check, and apply dedup only when pushing into results so stripRanges can remove
all XML occurrences. Also extend the openaiShim.xmlToolCalls test to assert the
duplicate block is removed from the final stripped text, not just that calls are
deduped.
---
Duplicate comments:
In `@src/services/api/openaiShim.ts`:
- Around line 2141-2191: The streaming finalize path in openaiShim.ts inherits
the duplicate-range issue from parseXmlToolCalls, so a repeated <tool_call>
block can still survive stripRanges(buffered, toolCallRanges) and be emitted as
text. Fix the root cause in parseXmlToolCalls so it returns correct,
non-overlapping toolCallRanges for duplicated XML tool calls; this emit path in
the finalize block should then work 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: 333af17f-0833-49ae-a135-06b76bf51d4f
📒 Files selected for processing (2)
src/services/api/openaiShim.tssrc/services/api/openaiShim.xmlToolCalls.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (13)
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript with strict mode and ESM imports
Files:
src/services/api/openaiShim.xmlToolCalls.test.tssrc/services/api/openaiShim.ts
{src/commands/**/*.ts,src/services/**/*.ts,src/entrypoints/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Use
chalkfor terminal color in CLI code
Files:
src/services/api/openaiShim.xmlToolCalls.test.tssrc/services/api/openaiShim.ts
{src/services/**/*.ts,src/utils/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Use
execafor child processes
Files:
src/services/api/openaiShim.xmlToolCalls.test.tssrc/services/api/openaiShim.ts
{src/integrations/**/*.ts,src/services/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Test the exact provider/model path you changed when possible for provider modifications
Files:
src/services/api/openaiShim.xmlToolCalls.test.tssrc/services/api/openaiShim.ts
**/*.{ts,tsx,js,jsx,py,json,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Follow the existing code style in the touched files
Files:
src/services/api/openaiShim.xmlToolCalls.test.tssrc/services/api/openaiShim.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Add or update tests when the change affects behavior
Files:
src/services/api/openaiShim.xmlToolCalls.test.ts
**/*.test.{ts,tsx,js}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Test the exact provider/model path you changed when possible
Files:
src/services/api/openaiShim.xmlToolCalls.test.ts
**/*.{ts,tsx,js,jsx,py}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Keep comments useful and concise
Files:
src/services/api/openaiShim.xmlToolCalls.test.tssrc/services/api/openaiShim.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Follow TypeScript strict mode and type safety practices by running typecheck before submitting
Files:
src/services/api/openaiShim.xmlToolCalls.test.tssrc/services/api/openaiShim.ts
**
⚙️ CodeRabbit configuration file
**: # AGENTS.md - AI Agent Coding GuideThis guide is for AI coding agents working in the OpenClaude repository. Read it before changing code, and also follow CONTRIBUTING.md for contributor policy, PR expectations, review follow-up, and project scope.
Project Snapshot
OpenClaude is a coding-agent CLI for cloud and local model providers. It supports OpenAI-compatible APIs, Anthropic, Gemini, DeepSeek, Ollama, MCP, local backends, slash commands, tools, agents, and a React/Ink terminal UI.
The installed CLI runs on Node.js
>=22.0.0. Bun is used for source builds, scripts, dependency management, and tests.Work Style
- Keep changes focused on one problem.
- Prefer existing patterns in the file or nearby module.
- Avoid unrelated formatting, renames, dependency changes, or broad rewrites.
- Add or update tests when behavior changes.
- Update docs when setup, commands, provider behavior, or user-facing behavior changes.
- For new features, larger refactors, dependencies, or runtime changes, follow the issue-first guidance in CONTRIBUTING.md.
Stack And Conventions
- TypeScript with strict mode and ESM imports.
- React + Ink for terminal UI.
- Bun lockfile and Bun scripts for development workflows.
- Node runtime for the built CLI.
- Python exists for legacy/local-provider helper code. Do not add new Python code or expand Python-based features unless a maintainer explicitly approves that direction.
Common libraries and patterns:
chalkfor terminal color.commanderfor CLI argument parsing.execafor child processes.- Existing service, provider, settings, permission, and UI patterns over new abstractions.
Repository Map
src/commands/- slash and CLI command implementations.src/components/- React/Ink UI components.src/services/- API, MCP, OAuth, wiki, voice, and other service integrations.src/tools/- tool implementations.src/utils/- shared utilities.- `src/integration...
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
{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)
1630-1635: LGTM!Also applies to: 1877-1879, 1895-1928, 2204-2206, 3746-3782, 3795-3795, 3810-3810
src/services/api/openaiShim.xmlToolCalls.test.ts (1)
1-316: LGTM!
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
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)
3766-3777: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftPreserve recovered XML content ordering.
stripRanges(...).trim()merges all non-XML prose and pushes it before every recoveredtool_use. For mixed content likebefore <tool_call>A</tool_call> after <tool_call>B</tool_call> tail, the output becomes one text block (before after tail) followed by both tools, moving text that originally followed a tool ahead of execution. Emit text/tool blocks by walking the parsed XML ranges in source order, or have the parser return ordered parsed blocks so duplicates can still be stripped without reordering content.🤖 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 3766 - 3777, The XML recovery logic in openaiShim’s tool-call parsing is reordering mixed content by collecting all non-XML text with stripRanges(...).trim() and appending it before every tool_use, which moves text that originally appeared after a tool call ahead of it. Update the parsing flow around parseXmlToolCalls and the content assembly so text and recovered tool_use blocks are emitted in source order, either by iterating over parsed ranges sequentially or by having the parser return ordered blocks while still deduplicating stripped XML segments.
🤖 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 3766-3777: The XML recovery logic in openaiShim’s tool-call
parsing is reordering mixed content by collecting all non-XML text with
stripRanges(...).trim() and appending it before every tool_use, which moves text
that originally appeared after a tool call ahead of it. Update the parsing flow
around parseXmlToolCalls and the content assembly so text and recovered tool_use
blocks are emitted in source order, either by iterating over parsed ranges
sequentially or by having the parser return ordered blocks while still
deduplicating stripped XML segments.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5742d81f-3351-49a2-b6b8-5122fc60c08f
📒 Files selected for processing (1)
src/services/api/openaiShim.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (10)
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript with strict mode and ESM imports
Files:
src/services/api/openaiShim.ts
{src/commands/**/*.ts,src/services/**/*.ts,src/entrypoints/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Use
chalkfor terminal color in CLI code
Files:
src/services/api/openaiShim.ts
{src/services/**/*.ts,src/utils/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Use
execafor child processes
Files:
src/services/api/openaiShim.ts
{src/integrations/**/*.ts,src/services/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Test the exact provider/model path you changed when possible for provider modifications
Files:
src/services/api/openaiShim.ts
**/*.{ts,tsx,js,jsx,py,json,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Follow the existing code style in the touched files
Files:
src/services/api/openaiShim.ts
**/*.{ts,tsx,js,jsx,py}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Keep comments useful and concise
Files:
src/services/api/openaiShim.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Follow TypeScript strict mode and type safety practices by running typecheck before submitting
Files:
src/services/api/openaiShim.ts
**
⚙️ CodeRabbit configuration file
**: # AGENTS.md - AI Agent Coding GuideThis guide is for AI coding agents working in the OpenClaude repository. Read it before changing code, and also follow CONTRIBUTING.md for contributor policy, PR expectations, review follow-up, and project scope.
Project Snapshot
OpenClaude is a coding-agent CLI for cloud and local model providers. It supports OpenAI-compatible APIs, Anthropic, Gemini, DeepSeek, Ollama, MCP, local backends, slash commands, tools, agents, and a React/Ink terminal UI.
The installed CLI runs on Node.js
>=22.0.0. Bun is used for source builds, scripts, dependency management, and tests.Work Style
- Keep changes focused on one problem.
- Prefer existing patterns in the file or nearby module.
- Avoid unrelated formatting, renames, dependency changes, or broad rewrites.
- Add or update tests when behavior changes.
- Update docs when setup, commands, provider behavior, or user-facing behavior changes.
- For new features, larger refactors, dependencies, or runtime changes, follow the issue-first guidance in CONTRIBUTING.md.
Stack And Conventions
- TypeScript with strict mode and ESM imports.
- React + Ink for terminal UI.
- Bun lockfile and Bun scripts for development workflows.
- Node runtime for the built CLI.
- Python exists for legacy/local-provider helper code. Do not add new Python code or expand Python-based features unless a maintainer explicitly approves that direction.
Common libraries and patterns:
chalkfor terminal color.commanderfor CLI argument parsing.execafor child processes.- Existing service, provider, settings, permission, and UI patterns over new abstractions.
Repository Map
src/commands/- slash and CLI command implementations.src/components/- React/Ink UI components.src/services/- API, MCP, OAuth, wiki, voice, and other service integrations.src/tools/- tool implementations.src/utils/- shared utilities.- `src/integration...
Files:
src/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.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.ts
🔇 Additional comments (1)
src/services/api/openaiShim.ts (1)
1339-1339: LGTM!
Summary
GLM/Qwen-family models routed through OpenAI-compatible gateways emit tool calls
as XML text (
<tool_call><function=…>) instead of structuredtool_calls. Theshim had no parser for this, so the XML leaked into visible prose and never
executed — the turn ended with no
tool_useblock, so the agent appeared to"forget" and stop mid-task (the reported GLM-5.2 bug).
This adds
parseXmlToolCalls()covering the three dialects seen in the wild andwires it into both the streaming and non-streaming paths, mirroring the existing
Ollama text-tool-call fallback:
<tool_call><function=NAME><parameter=KEY>VALUE</parameter>…</function></tool_call><tool_call>NAME<arg_key>KEY</arg_key><arg_value>VALUE</arg_value>…</tool_call>(GLM-native)<tool_call>{"name":"NAME","arguments":{…}}</tool_call>(Hermes JSON)The streaming path holds back text from the
<tool_call>opener onward (includingan opener split across SSE deltas), converts the buffer to
tool_useblocks atfinalize, strips the raw XML, and flips the finish reason to
tool_calls.Structured
tool_callsand false-positive prose (a model literally writing<tool_call>) are both handled losslessly.Impact
text, fixing the "agent forgets / stops mid-task" symptom.
provider that emits structured
tool_calls.<tool_call>marker is distinctive enough to be safe (JSON text recovery remains Ollama-gated).
Testing
bun test src/services/api/openaiShim.xmlToolCalls.test.ts— 19/19 pass(3 dialects, split-opener, false-positive, structured-call coexistence,
interleaved-prose flatten case)
bun test src/services/api/— 873/873 pass, no regressionstsc --noEmit— 0 errorsbun run deadcode(knip) — cleanNotes
XML tool calls, surviving prose is flattened to prose-then-all-calls. This
matches the existing Ollama path and doesn't affect agent loops (which act on
the tool calls regardless of prose order). A test locks in this behavior.
from GLM/Qwen, but this client-side fallback is the right defensive layer
regardless.
Summary by CodeRabbit