refactor(openai): remove text-form tool-call rescue - #1145
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (20)
💤 Files with no reviewable changes (4)
🚧 Files skipped from review as they are similar to previous changes (15)
📝 WalkthroughWalkthroughThe guardrail stack now validates native tool calls and structured output without text-based rescue. Thinking-block stripping is exposed as a shared utility. Parser-stage and rescued telemetry are removed. Runtime integrations, tests, corpus reporting, and documentation now reflect native validation. ChangesNative guardrail validation
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
|
This pull request is currently a draft. Reviews will not take place until the PR is marked as ready for review. |
0a1d4ef to
7d3d1dc
Compare
7d3d1dc to
37ab2fb
Compare
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
crates/mesh-llm-guardrails/src/content.rs (1)
32-43: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tests for the unterminated and multi-block cases.
The current test covers only one paired block per marker style. Add cases for an unterminated
<think>, for text before and after a block, and for multiple blocks in one string. These cases guard the loop logic instrip_tag_pairs.🤖 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 `@crates/mesh-llm-guardrails/src/content.rs` around lines 32 - 43, Add cases to strips_supported_thinking_blocks covering an unterminated <think> block, text before and after a thinking block, and multiple thinking blocks in one input. Assert the expected visible text for each case to exercise the loop behavior in strip_tag_pairs while preserving the existing paired-marker coverage.crates/mesh-llm-guardrails/src/lib.rs (1)
2-12: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider dropping the new crate-root re-export.
contentis public, so consumers can usemesh_llm_guardrails::content::strip_thinking_blocksdirectly. The coding guidelines ask to minimize crate-root re-exports and to import from the owning module in new code. Keeping only the module declaration would match that rule.As per coding guidelines: "Minimize crate-root re-exports. Temporary compatibility re-exports are allowed during refactors, but new code should import from the owning module directly."
🤖 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 `@crates/mesh-llm-guardrails/src/lib.rs` around lines 2 - 12, Remove the crate-root re-export of strip_thinking_blocks from lib.rs, while keeping the public content module declaration unchanged. Update any new callers to import strip_thinking_blocks through mesh_llm_guardrails::content instead of the crate root.Source: Coding guidelines
crates/openai-frontend/src/router.rs (1)
1331-1333: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider keeping router-level coverage for the guarded backend.
The deleted integration test wrapped the router with
GuardedOpenAiBackendin enforce mode. It covered more than rescue: request-ID propagation, the sanitized response shape, and the injected_mesh_respondtool. The remaining guardrail tests incrates/openai-frontend/src/guardrails/tests/response_validation.rsexercise the engine directly, not the router wiring.Add a smaller replacement test that posts to
/v1/chat/completionsthrough a guarded router with a backend that returns a native_mesh_respondtool call, and assert the sanitized text output.🤖 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 `@crates/openai-frontend/src/router.rs` around lines 1331 - 1333, Add a focused router-level test near the existing tokio tests in the router module, configuring GuardedOpenAiBackend in enforce mode with a backend that returns a native _mesh_respond tool call. POST to /v1/chat/completions through the guarded router and assert the response contains the expected sanitized text output, preserving coverage of router wiring without restoring unrelated assertions.crates/openai-frontend/src/guardrails/validation.rs (1)
97-310: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftSplit
classify_tool_call_valueinto named decision helpers.The function spans about 214 lines and repeats the
ClassifiedGuardrailResponseliteral in twelve branches. The coding guidelines set a Clippy line-count limit and a cognitive-complexity limit for Rust functions. This PR touches every branch, so this is a good point to split it.Suggested decomposition, all in
validation.rs:
parse_all_tool_calls(raw_calls, allowed) -> Result<Vec<ParsedToolCall>, GuardrailResponseCategory>for the loop at Lines 122-161.classify_contract_violations(prepared, &parsed_calls) -> Option<GuardrailResponseCategory>for the checks at Lines 173-225.classify_synthetic_respond(...)andclassify_synthetic_structured(...)for Lines 227-300.Add a small constructor such as
rejected(category, finish_reason)andrejected_with_calls(category, calls, finish_reason)to remove the repeated struct literals.As per coding guidelines: "Do not add Rust functions or methods exceeding the configured Clippy line-count limit; split long logic into semantically named helpers." and "Do not add Rust code over the configured cognitive-complexity limit; prefer small named decision helpers and clear control-flow phases."
🤖 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 `@crates/openai-frontend/src/guardrails/validation.rs` around lines 97 - 310, Refactor classify_tool_call_value into smaller named phases to satisfy line-count and cognitive-complexity limits: extract parsing into parse_all_tool_calls, contract checks into classify_contract_violations, and synthetic respond/structured handling into classify_synthetic_respond and classify_synthetic_structured. Add rejected and rejected_with_calls constructors to centralize repeated ClassifiedGuardrailResponse creation, while preserving all existing categories, payloads, and finish-reason behavior.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 `@crates/mesh-llm-guardrails/src/content.rs`:
- Around line 11-26: Update the unterminated-tag branch in strip_tag_pairs so it
does not reappend the prefix already added to result; exit the loop with an
empty or otherwise consumed remainder, preserving the existing behavior for
terminated tag pairs and trailing content.
In `@crates/mesh-llm-host-runtime/src/inference/consult.rs`:
- Around line 215-221: Update tool_calls_as_consultation_text to preserve valid
named tool calls when arguments are missing or null: treat either case as an
empty JSON object before serializing, while continuing to normalize and
serialize provided arguments through the existing path.
In `@crates/openai-frontend/src/guardrails/tests/response_validation.rs`:
- Around line 15-28: Update the fenced JSON test case in the response
classification loop to use a multiline raw string with actual line breaks
between the opening fence, JSON payload, and closing fence; preserve the
existing payload and malformed-tool-text assertions.
---
Nitpick comments:
In `@crates/mesh-llm-guardrails/src/content.rs`:
- Around line 32-43: Add cases to strips_supported_thinking_blocks covering an
unterminated <think> block, text before and after a thinking block, and multiple
thinking blocks in one input. Assert the expected visible text for each case to
exercise the loop behavior in strip_tag_pairs while preserving the existing
paired-marker coverage.
In `@crates/mesh-llm-guardrails/src/lib.rs`:
- Around line 2-12: Remove the crate-root re-export of strip_thinking_blocks
from lib.rs, while keeping the public content module declaration unchanged.
Update any new callers to import strip_thinking_blocks through
mesh_llm_guardrails::content instead of the crate root.
In `@crates/openai-frontend/src/guardrails/validation.rs`:
- Around line 97-310: Refactor classify_tool_call_value into smaller named
phases to satisfy line-count and cognitive-complexity limits: extract parsing
into parse_all_tool_calls, contract checks into classify_contract_violations,
and synthetic respond/structured handling into classify_synthetic_respond and
classify_synthetic_structured. Add rejected and rejected_with_calls constructors
to centralize repeated ClassifiedGuardrailResponse creation, while preserving
all existing categories, payloads, and finish-reason behavior.
In `@crates/openai-frontend/src/router.rs`:
- Around line 1331-1333: Add a focused router-level test near the existing tokio
tests in the router module, configuring GuardedOpenAiBackend in enforce mode
with a backend that returns a native _mesh_respond tool call. POST to
/v1/chat/completions through the guarded router and assert the response contains
the expected sanitized text output, preserving coverage of router wiring without
restoring unrelated assertions.
🪄 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 Plus
Run ID: dd99995e-b71d-45b2-b567-800c559658df
📒 Files selected for processing (20)
crates/mesh-llm-guardrails/src/content.rscrates/mesh-llm-guardrails/src/lib.rscrates/mesh-llm-guardrails/src/rescue.rscrates/mesh-llm-host-runtime/src/inference/consult.rscrates/mesh-llm-host-runtime/src/runtime/survey.rscrates/mesh-mixture-of-agents/src/normalize.rscrates/mesh-mixture-of-agents/tests/sim_tool_call_text_not_passed_as_content.rscrates/openai-frontend/src/guardrails/engine.rscrates/openai-frontend/src/guardrails/mod.rscrates/openai-frontend/src/guardrails/retry.rscrates/openai-frontend/src/guardrails/telemetry.rscrates/openai-frontend/src/guardrails/tests.rscrates/openai-frontend/src/guardrails/tests/response_validation.rscrates/openai-frontend/src/guardrails/validation.rscrates/openai-frontend/src/router.rscrates/openai-frontend/tests/benchy_contract.rsdocs/design/OPENAI_GUARDRAILS.mddocs/design/TESTING.mddocs/plugins/telemetry.mdscripts/run-openai-guardrail-corpus.py
💤 Files with no reviewable changes (4)
- crates/mesh-llm-guardrails/src/rescue.rs
- crates/openai-frontend/src/guardrails/telemetry.rs
- crates/openai-frontend/tests/benchy_contract.rs
- crates/mesh-mixture-of-agents/tests/sim_tool_call_text_not_passed_as_content.rs
|
this is ok - but will basically stop small models working I think - so might want a bit more attention on that - can you try with with say qwen 8B families for evidence? |
michaelneale
left a comment
michaelneale
left a comment
There was a problem hiding this comment.
I think may want to reconsider this for v small models, and get some tests/harnesses in place first. ie needs to be some cut off?
cf8133c to
dc17d85
Compare
ahh now I understand what its for. OK, I will restore that now. |
|
Agreed. Before deciding whether to restore the text-form rescue, I am adding and running tool-calling coverage across representative Qwen 8B GGUF families. The acceptance criterion is that the OpenAI endpoint emits native I will trace the llama.cpp chat-template/parser path at the same time. If native calls are absent because we are not applying the model parser infrastructure, the right fix is to wire that path correctly; a narrow compatibility shim remains reasonable while that lands. If the parser is working and smaller models still only emit text-form calls, we can make an explicit compatibility decision with evidence. |
dc17d85 to
21a3cc2
Compare
|
Investigation result: the native llama.cpp chat-template/parser path is already active in serving; it is not being bypassed. I ran Based on this evidence, I do not recommend restoring the removed broad text-form rescue for these Qwen families. The separate serving-side emulation remains the compatibility path for templates that genuinely lack native tool support. |
|
To make the distinction explicit: this PR removes a last-resort, post-parse text rescue. It does not remove the normal Qwen tool-call path. The serving path first gives llama.cpp the conversation, tool definitions, and tool choice through For the two representative Qwen families I tested, the normal path is working end-to-end: Qwen3-8B Q4_K_M and Qwen2.5-7B Instruct Q2_K both returned This does not mean every small model has native tool support. Models whose chat template genuinely has no native tool-call support still use the separate serving-side emulation path. Nor does it remove the need for guardrails to validate native calls, reject malformed arguments, and enforce policy. It only stops treating arbitrary text that the model/parser did not recognize as a call as though it were one. My recommendation is therefore to retain the removal rather than add a size cutoff: model size is not the capability boundary; chat-template/parser support is. If there is a particular small-model family we need to support that emits only text-form calls despite a native template, we should add it to the harness and decide on a deliberately narrow compatibility shim for that family. |
|
@michaelneale The key conclusion is simpler than my previous wording: if the reason this guardrail was added was that Mesh was not correctly using llama.cpp chat-template parsing, then we no longer need the text-form tool-call rescue for models that ship a valid native tool-call template. That is what the Qwen runs show. With the current Mesh serving path, Qwen3-8B and Qwen2.5-7B receive the tools via their chat template and llama.cpp parses their generated tool call back into native OpenAI So, for those models, the previous rescue was compensating for the broken/missing parser integration—not a model limitation. Restoring it would reintroduce a fallback that is unnecessary for a correctly templated model and can turn unrecognised plain text into an action. The qualification is only this: a model with no valid native tool-call template/parser needs a compatibility path. That is the separate serving-side emulation path, not this broad post-hoc guardrail rescue. So I think the right rule is: valid native template → native parsing → no text rescue; no native template → explicit emulation. I therefore recommend retaining this removal; no model-size cutoff is warranted. |
|
@i386 is that something we can show a before/after proof of? As yeah if it was that then yes that could explain it |
|
@michaelneale I reran this as the A/B you asked for, with the guardrail wrapper disabled in both cases. The result corrects my earlier conclusion.
Each run used the same direct local OpenAI endpoint and So Qwen3-8B does not demonstrate that the removed guardrail rescue is needed—and it also does not demonstrate that the parser change is what fixed Qwen3. The pre-change build already works for this model without guardrails. My earlier comment attributing Qwen success to the parser change was too strong; this A/B falsifies that attribution. The valid conclusion from this model is narrower: removing the broad rescue does not regress Qwen3-8B native tool calling. To establish why Michael originally added the rescue, we need the specific model/output shape that previously required it (or another reproducer that emits text-form calls with guardrails disabled). That is the right target for a compatibility test; a model-size cutoff is not evidenced by Qwen3. |
|
🤖 Posted by Mic's AI agent (CrocDundee) Concern: this PR removes the only test coverage for the "text-form tool intent must not leak as content" invariant, and there's no fail-closed replacement. The direction of the PR is sound — a second text-scanning interpreter that can execute quoted/explained tool-call syntax is a real footgun, and the native chat parser (plus capability-gated But the MoA path loses a guarantee here. In That's precisely the failure mode pinned by The execution-rescue can go, but the invariant it protected should survive: text-form tool intent must become an explicit error / reducer-escalation, never a silent successful |
|
@michaelneale I traced this back through the original guardrail work and reran the historical model family rather than relying on Qwen3-8B. What the history says
Retest on this PR head I downloaded and ran Qwen3.5-0.8B UD-Q8_K_XL and Qwen3.5-4B UD-Q4_K_XL against the current PR head, with the same direct local OpenAI endpoint. For each model I ran guardrails disabled and global guardrails enforce (verified from
I also forced a nested tool schema at Conclusion For the original Qwen3.5 family, I cannot reproduce a case where the broad text rescue provides a benefit. The observed failures are below or outside that layer: native parser failure and streaming behavior. This supports retaining the rescue removal for Qwen, but it does not prove no model ever needs a compatibility shim. The separate GLM tagged-call history is the concrete remaining candidate. If there is a specific historical text-form output that rescue successfully converted into a valid call, that exact model/prompt/response is what we should add as a narrow regression fixture before retaining a shim. |
|
@michaelneale I agree this is a separate concern from the parser/guardrail investigation. The Qwen A/B evidence supports removing broad text-to-executable-tool-call rescue: we should not scan arbitrary assistant text and synthesize I think the safe replacement is a narrow, tool-context-aware MoA guard—not execution rescue:
That preserves the fail-closed invariant without reconstructing or executing a call from text. The deleted inline-JSON success case should also fail closed unless the call arrived through the native tool-call channel. How would you like to proceed—would you prefer to take that narrow MoA guard/test, or should I implement it here? |
b816be8 to
7aa1c51
Compare
|
I think a more narrow MoA guard makes sense here ... I think... not sure what we lose in process of that though. One thing to try is to try a small model with almost any agent harness - that is only way you see this (basically none of them work even a little bit), if it works reasonably now (which would be obvious), then that is good enough to go, and we can look at MoA as a follow on? |
|
Ran the requested OpenCode tool-loop smoke test on the current PR head
The harness's direct OpenAI tool probe also returned This was a real local Metal run through the OpenAI surface and OpenCode 1.16.2. I disabled only the long-context soak portion ( |
7aa1c51 to
4d99c94
Compare
|
Correction to my prior test report: the observed behavior is slow continuation, not a confirmed deadlock. The capture proves OpenCode sent the post-tool request (assistant I had interrupted the original OpenCode run at approximately that latency threshold, so the earlier "stalled" conclusion was premature. This shows the OpenCode → tool result → mesh → next tool-call path does work for the 4B replay; it is simply much slower than the bounded wait I used. I have not yet rerun the entire multi-step smoke to completion under a longer time budget. |
3 participants
Problem
The OpenAI frontend had a second set of text scanners that tried to recover tool calls from generated prose after the model's native chat parser had already classified the output.
That creates two competing interpretations of the same response. Text that merely discusses or quotes tool-call syntax can be mistaken for an invocation, while each additional model dialect needs another heuristic parser.
Change
The model's native chat parser is now the only component allowed to identify a tool call. Text remains text unless that parser emits structured tool-call data.
Schema validation, retries, safe argument normalization, and thinking-block cleanup remain in place. Removing the rescue path avoids false invocations and gives every response one deterministic interpretation instead of extending a second parser indefinitely.
Use case
An assistant can explain, quote, or debug JSON and XML tool-call examples without the frontend unexpectedly executing them.
Validation
OpenAI, guardrail, and mixture-of-agents tests pass.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation