Skip to content

fix(normalize): strip slash-command envelope remnants and ANSI escapes (#1333) - #1701

Open
terrizoaguimor wants to merge 2 commits into
MemPalace:developfrom
terrizoaguimor:fix/strip-noise-envelope-ansi
Open

fix(normalize): strip slash-command envelope remnants and ANSI escapes (#1333)#1701
terrizoaguimor wants to merge 2 commits into
MemPalace:developfrom
terrizoaguimor:fix/strip-noise-envelope-ansi

Conversation

@terrizoaguimor

Copy link
Copy Markdown
Contributor

Problem

strip_noise() covered only 3 of Claude Code's 5-tag slash-command envelope
(system-reminder/command-message/command-name) and no ANSI escapes. So
<command-args>, <local-command-caveat>, <local-command-stdout> and the
ANSI color bytes from Bash tool output survived into drawers — polluting search
and bloating embeddings (each escape is several BPE tokens). Closes #1333.

Fix

  • Add command-args, local-command-caveat, local-command-stdout to
    _NOISE_TAGS — they inherit the existing line-anchored, blank-line-bounded
    safety (a dangling tag can't eat neighbouring messages).
  • Add ECMA-48 CSI + OSC strippers, applied after tag removal. Anchored
    on the literal ESC byte (0x1B), a control char that never appears in prose,
    so text that merely names [1m/ESC[0m is preserved. ReDoS-safe by
    construction (disjoint/negated classes — the failure mode behind
    CVE-2021-3807). CSI covers SGR colors + cursor/erase; OSC covers hyperlinks
    (BEL or ST terminated).

Why it's safe

"Verbatim is sacred" is preserved: every removal is anchored (line-start for
tags, the ESC byte for ANSI). New tags reuse the audited _tag_pattern. No new
dependencies.

Tests

8 new tests in test_normalize.py: full envelope stripping, CSI SGR colors (the
issue's real sample), OSC BEL + ST termination, ANSI nested inside a stripped
tag, and verbatim preservation of prose that names ANSI sequences. Verified
locally: ruff check clean, ruff format clean, and
test_normalize.py + test_miner.py + test_convo_miner.py (252 tests) pass.

MemPalace#1333)

strip_noise() covered only 3 of Claude Code's 5-tag slash-command envelope
(system-reminder/command-message/command-name) and no ANSI escapes, so
<command-args>, <local-command-caveat>, <local-command-stdout> and the ANSI
color bytes from Bash tool output survived into drawers — polluting search and
bloating embeddings (each escape is several BPE tokens).

- Add command-args, local-command-caveat, local-command-stdout to _NOISE_TAGS;
  they inherit the existing line-anchored, blank-line-bounded safety.
- Add ECMA-48 CSI + OSC strippers applied after tag removal, anchored on the
  literal ESC byte (0x1B) so prose that merely names "[1m"/"ESC[0m" is kept.
  ReDoS-safe by construction (disjoint/negated classes). CSI covers SGR colors
  + cursor/erase; OSC covers hyperlinks (BEL or ST terminated).

Closes MemPalace#1333.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request adds support for stripping additional Claude Code slash-command envelope tags (command-args, local-command-caveat, and local-command-stdout) and ANSI escape sequences (CSI and OSC) from text transcripts, along with corresponding unit tests. The review feedback highlights a critical issue where the existing _tag_pattern implementation prevents matching across blank lines, which will cause local-command-stdout blocks containing blank lines to fail to be stripped. A code suggestion is provided to redefine _tag_pattern to allow blank lines while safely preventing unclosed tags from matching across subsequent blocks.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread mempalace/normalize.py
# caveat, args, and stdout envelopes leaked into drawers verbatim.
"command-args",
"local-command-caveat",
"local-command-stdout",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The newly added local-command-stdout tag is highly likely to contain blank lines in its content (for example, multi-paragraph command outputs, formatted tables, or stack traces). However, the existing _tag_pattern implementation (line 63) uses (?:(?!\\n\\s*\\n)[\\s\\S])*? which explicitly prevents matching across blank lines. Consequently, any local-command-stdout block containing a blank line will fail to be stripped entirely, leaking the raw tags and all the command output into the final transcript.\n\nTo allow blank lines inside these tags while still preventing the 'span-eating' bug (where an unclosed tag matches across to a subsequent closed tag), _tag_pattern can be redefined to forbid matching another opening tag of the same name instead of forbidding blank lines.\n\nFor example, redefining _tag_pattern as follows resolves this issue:\n\npython\ndef _tag_pattern(name: str) -> \"re.Pattern[str]\":\n return re.compile(\n rf\"(?m)^(?:> )?<{name}(?:\\s[^>]*)?>\" rf\"(?:(?!<{name}\\b)[\\s\\S])*?\" rf\"</{name}>[ \\t]*\\n?\"\n )\n\n\nThis allows blank lines to be fully supported inside all tags (including local-command-stdout), while preventing an unclosed tag from 'eating' subsequent messages or other closed tags of the same type because the negative lookahead (?!<{name}\\b) halts the match if another opening tag is encountered.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — applied in 09e9484. <local-command-stdout> (command output) routinely contains blank lines, so the (?!\n\s*\n) guard would have leaked the whole envelope. Switched the body guard to (?!<{name}\b) as you suggested: blank lines inside a tag are now allowed, while a dangling open still can't span-eat past the next same-name block — the line-start anchor remains the first line of defense. Added regression tests for both the blank-line case and the same-tag re-open anti-span-eating case.

…emPalace#1701)

<local-command-stdout> command output routinely contains blank lines, which
the old (?!\n\s*\n) body guard refused to cross — leaking the whole envelope.
Replace it with (?!<{name}\b): the lazy body halts at a re-opened same-name tag
instead of at a blank line, so blank lines inside a tag are allowed while a
dangling open still can't span-eat past the next same-tag block (the line-start
anchor remains the first defense). Adds regression tests for both behaviours.
@terrizoaguimor

Copy link
Copy Markdown
Contributor Author

Heads-up on the red CI here: all three failing checks (lint, test-linux (3.11), test-macos) are pre-existing on develop (@ 334fc6d), not introduced by this PR. This change only touches mempalace/normalize.py and its tests, which pass. I verified each failure reproduces on a clean develop checkout with nothing applied — flagging so the red isn't mistaken for a regression from this change.

1. lintruff check . fails on an untouched file

C901 `search_memories` is too complex (30 > 25)
 --> mempalace/searcher.py:873:5
Found 1 error.

search_memories is over ruff's mccabe complexity ceiling (max 25). Reproduce on clean develop with the repo's pinned ruff:

pip install "ruff==0.15.14" && ruff check .

2. test-linux (3.11) / test-macos / test-windowsTestParamShapeDiagnostics (2 tests)

tests/test_mcp_server.py::TestParamShapeDiagnostics::test_missing_required_returns_32602_with_param_name and ::test_two_missing_required_lists_both_names both fail on clean develop:

  • They expect a missing-argument tools/call to return a JSON-RPC error objectresp["error"]["code"] == -32602 with a message naming the missing parameter(s).
  • Current behavior has no error key (KeyError: 'error'), and for two missing args the message names only the first (Missing required parameter 'agent_name' for tool mempalace_diary_write) instead of listing both with the word "parameters".

So handle_request's missing-param path and these tests have drifted apart. Reproduce:

git checkout develop && pytest tests/test_mcp_server.py::TestParamShapeDiagnostics

The transcript-noise change in this PR is green on its own: ruff check mempalace/normalize.py is clean and the full test_normalize.py + test_miner.py + test_convo_miner.py suites pass locally.

Happy to send a separate PR for either of these (the C901 refactor and/or realigning the param-diagnostics tests with handle_request) if it'd help get develop green again — just say the word.

@igorls

igorls commented Aug 15, 2026

Copy link
Copy Markdown
Member

Thanks for this contribution, and apologies for the slow turnaround.

develop has moved a fair way since this was opened and the branch no longer merges cleanly. If you're still interested in landing it, could you rebase onto current develop? Once it merges cleanly and CI is green I'll get it reviewed for the 3.8.0 cycle.

If you'd rather not pick it back up, no problem at all — just say so and I'll close it out, and thanks either way for taking the time to send it.

@igorls igorls added bug Something isn't working area/mining File and conversation mining needs-rebase PR has merge conflicts with develop and needs rebase labels Aug 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/mining File and conversation mining bug Something isn't working needs-rebase PR has merge conflicts with develop and needs rebase

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Claude Code transcript noise: <local-command-*> tags and ANSI escapes survive strip_noise()

2 participants