fix(openai): recover text-mode tool calls from native models that emit tags#76
Conversation
The inline <tool_call> text parser matched only the exact <tool_call> literal, so the attribute form <tool_call id="call_0"> emitted by Hermes / DeepSeek chat templates (and the <tool_call|> / DeepSeek begin/end variants) fell through and leaked as raw text. Match the tag prefix tolerantly, verify the tag name is properly delimited (so <tool_calls> is not matched), and drop a no-name body without leaking its markup. Add should_recover(native, has_tools, structured_calls) so callers can run text recovery as a native-mode fallback.
Native tool-calling models only read the structured tool_calls field, so a model that emitted its call as text (DeepSeek via OpenRouter) leaked the raw <tool_call> markup into assistant content. Gate the three recovery sites (unary, non-stream, SSE) on should_recover: prompt-guided models always recover; native models recover only when tools were offered but the structured tool_calls array came back empty. Native responses that already carry structured calls are returned byte-for-byte unchanged.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 2 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughOpenAI response handling now uses shared recovery gating, while prompt tool-call parsing accepts additional provider-specific markup and avoids false positives. Tests cover supported delimiters, malformed input, structured recovery, and native fallback behavior. ChangesTool-call recovery
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant OpenAiModel
participant ResponseParser
participant should_recover
participant apply_prompt_tool_calls
OpenAiModel->>ResponseParser: Parse response and count structured tool calls
ResponseParser->>should_recover: Evaluate recovery conditions
should_recover-->>OpenAiModel: Return recovery decision
OpenAiModel->>apply_prompt_tool_calls: Recover terminal text when enabled
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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
🧹 Nitpick comments (1)
src/harness/providers/openai/transport.rs (1)
1406-1419: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRecovery-gating pattern is duplicated across three call sites.
invoke()(1411-1417), the non-SSE tolerant fallback (1481-1487), and the SSE terminal mapper (1535-1545) all repeat the same "should_recover(...)→apply_prompt_tool_calls(...), else pass through" shape. Extracting a small helper (e.g.fn recover_if_needed(native: bool, has_tools: bool, response: ModelResponse) -> ModelResponse) would keep all three call sites in lock-step ifshould_recover's contract changes again.♻️ Proposed helper
fn recover_if_needed(native: bool, has_tools: bool, response: ModelResponse) -> ModelResponse { if crate::harness::tool::should_recover(native, has_tools, response.message.tool_calls.len()) { crate::harness::tool::apply_prompt_tool_calls(response) } else { response } }Also applies to: 1481-1488, 1522-1549
🤖 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/harness/providers/openai/transport.rs` around lines 1406 - 1419, Extract the duplicated recovery gating into a shared helper near the transport response-mapping logic, using the native flag, whether request tools are present, and the ModelResponse; have it call should_recover and apply_prompt_tool_calls or return the response unchanged. Replace the recovery blocks in invoke(), the non-SSE tolerant fallback, and the SSE terminal mapper with this helper so all three paths remain consistent.
🤖 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/harness/providers/openai/transport.rs`:
- Around line 1522-1549: Ensure the SSE streaming path removes recovered
tool-call markup from streamed MessageDelta text, not only from the terminal
Completed response. Update the delta handling associated with sse_next and the
stream transformation around should_recover/apply_prompt_tool_calls so native
fallback responses do not expose raw markup to live consumers, while preserving
unchanged streaming when no tools are offered and structured native tool calls
are present.
---
Nitpick comments:
In `@src/harness/providers/openai/transport.rs`:
- Around line 1406-1419: Extract the duplicated recovery gating into a shared
helper near the transport response-mapping logic, using the native flag, whether
request tools are present, and the ModelResponse; have it call should_recover
and apply_prompt_tool_calls or return the response unchanged. Replace the
recovery blocks in invoke(), the non-SSE tolerant fallback, and the SSE terminal
mapper with this helper so all three paths remain consistent.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bb42cc47-0a02-4c5f-9ebd-3b6215e056ea
📒 Files selected for processing (3)
src/harness/providers/openai/transport.rssrc/harness/tool/prompt.rssrc/harness/tool/prompt_test.rs
|
| Filename | Overview |
|---|---|
| src/harness/tool/prompt.rs | Core logic change: adds tolerant tag scanner (next_open), hold_from for streaming hold-back, ToolCallStreamScrubber, and should_recover. hold_from has a minor positional issue when multiple openable-but-unclosed prefixes appear in the same buffer. |
| src/harness/providers/openai/transport.rs | Replaces three identical guard conditions with should_recover, adds clean_stream_item wiring the scrubber into both live deltas and the terminal SSE response. |
| src/harness/tool/prompt_test.rs | Adds 9 targeted regression tests covering attribute/pipe/DeepSeek open-tag forms, no-name body drop, plural-tag non-match, scrubber fragment-split scenarios, and the full should_recover gating matrix. |
| src/harness/providers/openai/test.rs | Adds stream_cleanup_scrubs_leaked_markup_from_live_deltas exercising the full SSE path end-to-end. Minor formatting-only changes to an existing test. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[SSE fragment arrives] --> B{tools offered?}
B -- no --> C[Forward stream untouched]
B -- yes --> D[ToolCallStreamScrubber.feed]
D --> E{complete block found?}
E -- yes --> F[Drop block, emit prefix, loop]
F --> E
E -- no --> G{unclosed open tag found?}
G -- yes --> H[Emit prefix, hold open tag in buf]
G -- no --> I[hold_from: emit safe prefix, hold trailing partial]
H --> J[Clean text emitted to delta consumer]
I --> J
K[Completed item arrives] --> L[scrubber.flush]
L --> M{buffered tail?}
M -- yes --> N[Emit as final MessageDelta before Completed]
M -- no --> O[No extra delta]
N --> P[should_recover: native + has_tools + structured_calls.len]
O --> P
P -- native AND structured_calls gt 0 --> Q[Response unchanged]
P -- prompt-guided OR native fallback --> R[apply_prompt_tool_calls]
Q --> S[Emit Completed]
R --> S
Reviews (3): Last reviewed commit: "fix(openai): scrub leaked tool-call mark..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
2 participants
Summary
Recover DeepSeek/Hermes text-mode tool calls that leak into assistant content when a native-flagged model emits its call as
<tool_call …>text instead of the structuredtool_callsfield.Problem
opencompany's company agents run on OpenHuman → tinyagents. On staging the model is DeepSeek (via OpenRouter). DeepSeek/Hermes chat templates emit tool calls as text —
<tool_call id="call_0">{...}</tool_call>— but the model is flagged as native structured tool-calling. Two failures compound:<tool_call>literal, so<tool_call id="…">(and the<tool_call|>/ DeepSeek<｜tool▁call▁begin｜>variants) never matched.!profile.tool_calling. A native model that returned an empty structuredtool_callsarray but text markup got no recovery — so the raw<tool_call>…</tool_call>markup leaked into the assistant reply as literal text instead of executing.Solution
Ported as a delta on top of what
mainalready covers:mainalready covered (kept, not duplicated): malformed argument JSON recovery for the structuredfunction.argumentsfield (relaxed_json.rs, fix(openai): recover relaxed/malformed tool-call argument JSON #68) — a different concern (argument blob repair, not text-tag markup). The exact-literal<tool_call>prompt-guided path and its no-name drop also already existed.src/harness/tool/prompt.rs— a prefix-tolerant opening-delimiter scanner (next_open) handling<tool_call id="…">,<tool_call|>, and the DeepSeek<｜tool▁call▁begin｜>/endpair. The tag name must be properly delimited (>,|, or whitespace) so<tool_calls>/<tool_callable>don't match; a no-namebody is dropped without leaking its raw markup; a prose<tool_callwith no closing>is left verbatim.should_recover(native, has_tools, structured_calls)gate, wired into all three OpenAI recovery sites (unary, non-stream, SSE terminal): prompt-guided models always recover; native models recover only as a fallback when tools were offered but the structured array came back empty.Invariants preserved
tool_callspresent,structured_calls > 0) is byte-for-byte unchanged —should_recoverreturnsfalse, soapply_prompt_tool_callsis never invoked.<tool_call>without a full tag+body is not misparsed;<tool_calls>(plural) never matches.Test coverage
Added to
src/harness/tool/prompt_test.rs(9 new tests, all green):<tool_call id="x">{"name":"foo",…}</tool_call>parses tofoo, markup does not survive into content<tool_call|>and DeepSeek<｜tool▁call▁begin｜>delimiters parsenamebody ({}) dropped without leaking<tool_call/{}<tool_calls>not matched; prose open-tag without close not misparsedapply_prompt_tool_callsrecovers attribute markup from an empty-structured-calls responseshould_recovergating matrix + native-with-structured-calls skipcargo testgreen forharness::tool::prompt(22 passed) andharness::providers::openai(125 passed).cargo fmt --checkclean on touched files;cargo clippy --libclean.Related
Fixes the DeepSeek text-mode tool-call leak observed in opencompany company agents on staging.
Summary by CodeRabbit
Bug Fixes
Tests