fix: strip adjacent function_response sanitizer leak#82155
Conversation
b56868e to
83e9aa9
Compare
|
Codex review: needs maintainer review before merge. Summary Reproducibility: yes. at source level. Current main strips standalone Real behavior proof Next step before merge Security Review detailsBest possible solution: Land the shared-sanitizer fix after maintainer review and normal merge gates, keeping the cleanup channel-agnostic and preserving literal-prose safeguards. Do we have a high-confidence way to reproduce the issue? Yes, at source level. Current main strips standalone Is this the best way to solve the issue? Yes. Extending the shared sanitizer to track stripped-block adjacency is narrower and more maintainable than adding channel-specific cleanup around final reply delivery. What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 9c389487002c. Re-review progress:
|
83e9aa9 to
5739e6e
Compare
45ca0b8 to
a52e7f2
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a52e7f2c24
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const functionResponseCloseStart = | ||
| tag.tagName === "function_response" | ||
| ? findMatchingToolCallCloseIndex(text, tag.end, tag.tagName) | ||
| : -1; |
There was a problem hiding this comment.
Avoid quadratic closing-tag scans per function_response tag
stripToolCallXmlTags() now calls findMatchingToolCallCloseIndex() for every <function_response> opening tag before checking whether adjacent-strip conditions even apply. That helper scans forward through the rest of the string each time, so messages containing many literal <function_response> examples (for example docs/tutorial-style output) become O(n²) to sanitize, which can add noticeable latency on user-visible reply paths. Gate the close-tag search behind the adjacency/standalone predicates (or reuse the existing single-pass state) to keep this pass linear.
Useful? React with 👍 / 👎.
|
Verification for current head a52e7f2:
Known proof gap: broad GitHub CI/security gates are currently queued on GitHub runners for this head; auto-merge is armed and will merge only after required checks pass. |
Summary
Fixes the remaining user-visible sanitizer leak where a stripped XML tool-call block can expose an immediately adjacent
<function_response>block.Root cause:
stripToolCallXmlTags()already removed high-confidence<function_calls>/<tool_call>scaffolding, but it did not carry forward the fact that the next tag is adjacent to just-stripped internal workflow XML. That meant<function_response>could be evaluated as ordinary text after the preceding block disappeared, especially for compact same-line forms, chained response blocks, and dangling response starts.This patch tracks the end of the last stripped XML tool-call block and strips adjacent
<function_response>blocks across whitespace. It covers:<function_calls>It keeps the literal-prose side tight: inline examples still preserve ordinary
<function_response>mentions, and a narrow tag-explanation exception preserves prose such as<function_response> is the response wrapperafter a stripped example block. The exception no longer treats generic prose-looking payloads such asis enabledas prose, because adjacent workflow output must strip even when output text starts like a sentence.Fixes #47444.
Real behavior proof
Behavior addressed: adjacent
<function_response>workflow output no longer survives after stripped XML tool-call scaffolding.Real environment tested: local OpenClaw checkout on the patched branch, running the production
sanitizeUserFacingText()path throughnode --import tsx.Exact steps or command run after this patch:
node --import tsx - <<'EOF' ... sanitizeUserFacingText(...) ... EOFEvidence after fix: terminal output from the patched sanitizer probe:
Observed result after fix: adjacent multiline and leading-space prose-like
<function_response>payloads are removed from user-facing output, while literal wrapper explanation prose remains visible.What was not tested: no live provider workflow was run; this is a deterministic text sanitizer change exercised through the production sanitizer function.
Verification
pnpm test src/shared/text/assistant-visible-text.test.ts src/agents/pi-embedded-helpers.sanitizeuserfacingtext.test.ts -- --reporter=verbose(203 tests passed)git diff --check/Users/steipete/Projects/agent-scripts/skills/codex-review/scripts/codex-review --mode local --full-access --output /tmp/codex-review-47444-r4.txt --parallel-tests "pnpm test src/shared/text/assistant-visible-text.test.ts src/agents/pi-embedded-helpers.sanitizeuserfacingtext.test.ts -- --reporter=dot"(clean, no accepted/actionable findings)pnpm check:changed(Blacksmith Testboxtbx_01krnwp12a74pe8pvayaprmy7z, GitHub Actions run https://github.com/openclaw/openclaw/actions/runs/25920010654, exit 0)