feat(cua-driver-rs): strip trailing agent-protocol closing tags from type_text payloads - #1695
Merged
Merged
Conversation
…sing tags from type_text payloads
When an LLM agent calls `type_text` with a long multi-line text body it
occasionally hallucinates its own tool-invocation closing tags into the
trailing whitespace - Anthropic's tool format closing tags for text,
parameter, invoke, function_calls (shown here in prose form to keep the
sanitizer from recursively eating its own commit message). The MCP
transport correctly strips them at the protocol layer but the VALUE of
the `text` field still contains them; they get faithfully delivered to
the target window and the agent often can't recover (Backspace is
blocked on VCL targets under background dispatch, etc.).
This change adds a small defensive sanitizer in `mcp_server::text_sanitize`
that strips a trailing run of recognized agent-protocol closing tags
before the text leaves the tool layer. Three guardrails against false
positives:
1. Only tags in the known protocol set are eligible (text, parameter,
invoke, function_calls, function_call, tool_use, tool_call, plus
the antml-namespaced variants). Generic HTML closings (div/span/
etc.) are left alone.
2. Only tags at the very end (after trimming trailing whitespace) are
eligible. Inline tags in the middle of legitimate document content
are untouched.
3. Balance check: an unbalanced closing is stripped; a closing with a
matching opener earlier in the body is kept. So document content
like "[text]foo[/text]" (balanced) survives, while a trailing
"[/text][/invoke]" (no openers) is stripped.
Returns `Cow::Borrowed` on the no-match fast path so the common case is
allocation-free. Emits a `tracing::warn` when sanitization fires so
operators can see the rate at which agents stumble into this.
Wired into all 6 type_text call sites:
- platform-windows/src/tools/impl_.rs (TypeTextTool, TypeTextCharsTool)
- platform-macos/src/tools/type_text.rs
- platform-macos/src/tools/type_text_chars.rs
- platform-linux/src/tools/impl_.rs (TypeTextTool, TypeTextCharsTool)
8 unit tests, all green: strip-trailing-text-invoke-tail,
strip-with-antml-namespace, leave-balanced-text-alone,
leave-balanced-parameter-alone, leave-unrelated-html-alone,
leave-plain-text-alone, strip-only-trailing-keeps-inner-html,
empty-input-is-borrowed. Tests use placeholder substitution (`{C}` ->
"[/text]" etc.) so the test source can be written without the literal
sequences that would otherwise trip on the agent's own tool boundary
when editing the file via an LLM.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Contributor
📝 WalkthroughWalkthroughThis PR adds defensive text sanitization to strip trailing agent-protocol closing tags from tool text inputs. A new ChangesText Sanitization for Agent Protocol Hallucinations
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
libs/cua-driver/rust/crates/platform-linux/src/tools/impl_.rs (1)
666-670: ⚡ Quick winAvoid unconditional clone on sanitizer fast path.
Both call sites clone the entire string on
Cow::Borrowedvia.into_owned(). Sincetext_rawis already owned, you can keep it without copying when no sanitization occurs.Proposed change
- let text = mcp_server::text_sanitize::strip_trailing_agent_protocol_tags(&text_raw) - .into_owned(); + let text = match mcp_server::text_sanitize::strip_trailing_agent_protocol_tags(&text_raw) { + std::borrow::Cow::Borrowed(_) => text_raw, + std::borrow::Cow::Owned(s) => s, + };Apply the same pattern at both Line 669 and Line 1993 call sites.
Also applies to: 1990-1994
🤖 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 `@libs/cua-driver/rust/crates/platform-linux/src/tools/impl_.rs` around lines 666 - 670, The code unconditionally calls .into_owned() on the Cow returned by mcp_server::text_sanitize::strip_trailing_agent_protocol_tags(&text_raw), causing an unnecessary clone when the sanitizer returns Cow::Borrowed; change both call sites (the one using args.require_str("text") -> text_raw and the other at the 1990–1994 site) to match on the returned Cow and return the existing owned String when the Cow is Borrowed (e.g., if Cow::Borrowed(_) -> use text_raw) and use the owned String when Cow::Owned(s) -> s, so you avoid copying the whole string on the sanitizer fast path while still returning a String.libs/cua-driver/rust/crates/platform-windows/src/tools/impl_.rs (1)
2110-2117: ⚡ Quick winPreserve the sanitizer’s zero-allocation fast path
Both call sites immediately do
.into_owned(), which forces an allocation even when the sanitizer returnsCow::Borrowed. That negates the performance contract documented in comments and inmcp_server::text_sanitize.💡 Proposed fix
- let text_raw = match args.require_str("text") { Ok(v) => v, Err(e) => return e }; + let text_raw = match args.require_str("text") { Ok(v) => v, Err(e) => return e }; // Strip trailing agent-protocol closing tags before delivery — // catches the case where an LLM hallucinated its own tool- // invocation tags into the text param (see text_sanitize docs). - // Returns Cow::Borrowed on the no-match fast path so the common - // case is allocation-free. - let text = mcp_server::text_sanitize::strip_trailing_agent_protocol_tags(&text_raw) - .into_owned(); + // Preserve no-match fast path: reuse `text_raw` when Borrowed. + let text = match mcp_server::text_sanitize::strip_trailing_agent_protocol_tags(&text_raw) { + std::borrow::Cow::Borrowed(_) => text_raw, + std::borrow::Cow::Owned(s) => s, + };- let text_raw = match args.require_str("text") { Ok(v) => v, Err(e) => return e }; + let text_raw = match args.require_str("text") { Ok(v) => v, Err(e) => return e }; // Same trailing-protocol-tag scrub as the main TypeTextTool — see // mcp_server::text_sanitize for rationale. - let text = mcp_server::text_sanitize::strip_trailing_agent_protocol_tags(&text_raw) - .into_owned(); + let text = match mcp_server::text_sanitize::strip_trailing_agent_protocol_tags(&text_raw) { + std::borrow::Cow::Borrowed(_) => text_raw, + std::borrow::Cow::Owned(s) => s, + };Also applies to: 4586-4590
🤖 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 `@libs/cua-driver/rust/crates/platform-windows/src/tools/impl_.rs` around lines 2110 - 2117, The sanitizer strip_trailing_agent_protocol_tags returns a Cow to preserve a zero-allocation fast path, but the code immediately calls .into_owned() (see variables text_raw and text) which forces allocation; change the call site to accept the Cow and only clone/into_owned when a mutable/owned String is actually needed (e.g., work with the returned Cow<&str> from mcp_server::text_sanitize::strip_trailing_agent_protocol_tags and call to_string() or into_owned() only at the final point where ownership is required). Apply the same change to the other identical call site that currently calls .into_owned().
🤖 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 `@libs/cua-driver/rust/crates/mcp-server/src/text_sanitize.rs`:
- Around line 99-105: The current balance check using has_matching_opener can
stop too early when there is a legitimate opener/closer pair followed by an
orphaned trailing closer; modify the logic around trimmed_head, open and inner
to call a new has_unclosed_opener(preceding, inner) that counts occurrences of
the same tag name (opening vs closing) in preceding and returns true only if
open_count > close_count (i.e., there is an unmatched opener), replace the
has_matching_opener call with this count-aware check, and add a regression test
asserting that input "<text>ok</text></text>" becomes "<text>ok</text>" to
ensure the orphaned trailing closer is stripped.
---
Nitpick comments:
In `@libs/cua-driver/rust/crates/platform-linux/src/tools/impl_.rs`:
- Around line 666-670: The code unconditionally calls .into_owned() on the Cow
returned by
mcp_server::text_sanitize::strip_trailing_agent_protocol_tags(&text_raw),
causing an unnecessary clone when the sanitizer returns Cow::Borrowed; change
both call sites (the one using args.require_str("text") -> text_raw and the
other at the 1990–1994 site) to match on the returned Cow and return the
existing owned String when the Cow is Borrowed (e.g., if Cow::Borrowed(_) -> use
text_raw) and use the owned String when Cow::Owned(s) -> s, so you avoid copying
the whole string on the sanitizer fast path while still returning a String.
In `@libs/cua-driver/rust/crates/platform-windows/src/tools/impl_.rs`:
- Around line 2110-2117: The sanitizer strip_trailing_agent_protocol_tags
returns a Cow to preserve a zero-allocation fast path, but the code immediately
calls .into_owned() (see variables text_raw and text) which forces allocation;
change the call site to accept the Cow and only clone/into_owned when a
mutable/owned String is actually needed (e.g., work with the returned Cow<&str>
from mcp_server::text_sanitize::strip_trailing_agent_protocol_tags and call
to_string() or into_owned() only at the final point where ownership is
required). Apply the same change to the other identical call site that currently
calls .into_owned().
🪄 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: 8e84851f-2acc-4c56-ad2f-ba1312c20371
📒 Files selected for processing (6)
libs/cua-driver/rust/crates/mcp-server/src/lib.rslibs/cua-driver/rust/crates/mcp-server/src/text_sanitize.rslibs/cua-driver/rust/crates/platform-linux/src/tools/impl_.rslibs/cua-driver/rust/crates/platform-macos/src/tools/type_text.rslibs/cua-driver/rust/crates/platform-macos/src/tools/type_text_chars.rslibs/cua-driver/rust/crates/platform-windows/src/tools/impl_.rs
Comment on lines
+99
to
+105
| // Balance check: is there an opening `<NAME` (case-insensitive, | ||
| // optionally followed by `>` or whitespace) earlier in the head? | ||
| // If so, treat the closing as legitimate document content and stop. | ||
| let preceding = &trimmed_head[..open]; | ||
| if has_matching_opener(preceding, inner) { | ||
| break; | ||
| } |
Contributor
There was a problem hiding this comment.
Balance guard can miss orphaned duplicate trailing closers.
If the body contains one legitimate pair and then an extra trailing closer (e.g., <text>x</text></text>), Line 103 sees an opener and stops, so the orphaned tail is not stripped.
Suggested fix direction
- if has_matching_opener(preceding, inner) {
+ if has_unclosed_opener(preceding, inner) {
break;
}Implement has_unclosed_opener as a count-aware check for the same tag name in preceding (open_count > close_count), and add a regression test for <text>ok</text></text> expecting "<text>ok</text>".
🤖 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 `@libs/cua-driver/rust/crates/mcp-server/src/text_sanitize.rs` around lines 99
- 105, The current balance check using has_matching_opener can stop too early
when there is a legitimate opener/closer pair followed by an orphaned trailing
closer; modify the logic around trimmed_head, open and inner to call a new
has_unclosed_opener(preceding, inner) that counts occurrences of the same tag
name (opening vs closing) in preceding and returns true only if open_count >
close_count (i.e., there is an unmatched opener), replace the
has_matching_opener call with this count-aware check, and add a regression test
asserting that input "<text>ok</text></text>" becomes "<text>ok</text>" to
ensure the orphaned trailing closer is stripped.
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
When an LLM agent calls `type_text` with a long multi-line body it occasionally hallucinates its own tool-invocation closing tags into the trailing whitespace — text / parameter / invoke / function_calls closing tags from Anthropic's tool format leak past the parameter boundary. The MCP transport strips them at the protocol layer but the VALUE of the `text` field still contains them. They get faithfully delivered to the target window and the agent often can't recover (Backspace is blocked on VCL/SAL targets under background dispatch).
This PR adds a small defensive sanitizer that strips a trailing run of recognized agent-protocol closing tags before the text leaves the tool layer.
Demonstrated regression
While writing a design doc on LibreOffice (via vision-only mode), my own `type_text` call accidentally trailed the protocol closing tags into the document. They showed up as the last two lines of the saved doc and couldn't be removed — backspace on VCL Writer is blocked by the SAL keystroke guard under `dispatch:"background"`.
Three guardrails against false positives
Returns `Cow::Borrowed` on the no-match fast path so the common case is allocation-free. Emits a `tracing::warn` when sanitization fires.
Call sites
Wired into all 6 `type_text` invocations:
Test plan
Tests use a placeholder-substitution helper (`{C}` → "[/text]" etc.) so the test source can be written and edited via an LLM-driven tool chain without the literal sequences tripping on the parent tool boundary.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Improvements