fix: guard keyword-based rewrites in sanitizeUserFacingText against false positives#10760
fix: guard keyword-based rewrites in sanitizeUserFacingText against false positives#10760Drickon wants to merge 1 commit intoopenclaw:mainfrom
Conversation
src/media-understanding/apply.ts
Outdated
| // Skip audio files from being included as <file> blocks. | ||
| // OGG/Opus binary data passes looksLikeUtf8Text() and gets misdetected as | ||
| // text/tab-separated-values, bypassing the kind==="audio" check above. |
There was a problem hiding this comment.
Extension parsing can break
nameHint.substring(nameHint.lastIndexOf(".")) will return the full string when there is no . (because lastIndexOf returns -1), and it will treat paths like .bashrc as an “extension” of .bashrc. That makes the audio-skip check incorrect for dotless filenames/URLs and can skip/allow attachments incorrectly.
Consider guarding the index (e.g. only compute an extension when lastIndexOf(".") > -1 and/or after the last /).
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/media-understanding/apply.ts
Line: 376:378
Comment:
**Extension parsing can break**
`nameHint.substring(nameHint.lastIndexOf("."))` will return the full string when there is no `.` (because `lastIndexOf` returns `-1`), and it will treat paths like `.bashrc` as an “extension” of `.bashrc`. That makes the audio-skip check incorrect for dotless filenames/URLs and can skip/allow attachments incorrectly.
Consider guarding the index (e.g. only compute an extension when `lastIndexOf(".") > -1` and/or after the last `/`).
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Thanks for the review, but this comment appears to reference code in src/media-understanding/apply.ts which isn't part of this PR. This PR only modifies src/agents/pi-embedded-helpers/errors.ts — it removes keyword-based error message rewrites that were causing false positives on legitimate assistant replies. No audio/extension logic was changed here.
There was a problem hiding this comment.
You're absolutely right — I apologize for the confusion. Looking at the PR description and the changes, this PR modifies src/agents/pi-embedded-helpers/errors.ts to fix false positives in sanitizeUserFacingText(), not the media understanding code.
It appears my comment was generated based on outdated or incorrect context about what files were changed in this PR. The audio extension parsing issue I mentioned is not relevant to your changes here.
Your fix — removing keyword-based rewrites from sanitizeUserFacingText() and keeping only structural error detection — looks correct for addressing the false positive issue described in #3594 and #8847.
Additional Comments (1)
If the intent is performance, consider making this configurable/paginated rather than hard-coding 30, or ensure all callers/UX are updated to match the new limit. Prompt To Fix With AIThis is a comment left during a code review.
Path: ui/src/ui/controllers/chat.ts
Line: 36:43
Comment:
**Chat history truncation**
This drops `chat.history` `limit` from 200 to 30. If the UI expects more messages for scrollback/context (e.g. restoring longer sessions, searching, or showing earlier tool results), this will deterministically truncate history after 30 and can break UX/features.
If the intent is performance, consider making this configurable/paginated rather than hard-coding 30, or ensure all callers/UX are updated to match the new limit.
How can I resolve this? If you propose a fix, please make it concise. |
a3ededc to
c0efc98
Compare
|
Re: the chat history truncation comment — |
…alse positives sanitizeUserFacingText applies keyword-based rewrites (role ordering, billing, etc.) to ALL outgoing text, including normal assistant replies. This causes false positives when the assistant legitimately discusses these topics (openclaw#3594). Add a looksLikeErrorText() guard that checks structural signals (error prefix, raw API payload, HTTP status code, context overflow header) before applying keyword rewrites. Normal conversational text passes through unchanged. Builds on the shouldRewriteContextOverflowText() pattern from ea423bb which already guards context overflow detection.
c0efc98 to
269e758
Compare
|
Fixed in #12988. This will go out in the next OpenClaw release. If you still see this after updating to the first release that includes #12988, please open a new issue with:
Link back here for context. |
|
Closing in favor of #12988, which handles this more cleanly by scoping rewrites to Thanks @Takhoffman for the thorough fix. |
Problem
sanitizeUserFacingText()applies keyword-based rewrites to ALL outgoing reply text. When an assistant reply legitimately discusses error-related topics (e.g., "context overflow", "roles must alternate", billing), the entire message gets replaced with a terse error string.Fixes #3594.
Root Cause
Keyword checks like
isBillingErrorMessage()and the role-ordering regex run on every assistant message with no structural guard. Normal conversation that mentions these topics gets incorrectly rewritten.Fix
Added a
looksLikeErrorText()guard that wraps the keyword-based rewrites. It checks for structural signals before allowing keyword matching:ERROR_PREFIX_RE)Normal conversational text passes through unchanged. Actual error payloads still get the friendly rewrites.
This builds on the
shouldRewriteContextOverflowText()pattern from ea423bb which already guards context overflow detection with similar structural checks.Changes
src/agents/pi-embedded-helpers/errors.ts— AddedlooksLikeErrorText()guard, wrapped role-ordering and billing keyword checks inside itsrc/agents/pi-embedded-helpers.sanitizeuserfacingtext.test.ts— Added tests for false positive preventionTesting