fix(python-sdk): port current JS stripAnsi regex to strip_ansi_escape_codes#1545
fix(python-sdk): port current JS stripAnsi regex to strip_ansi_escape_codes#1545mishushakov wants to merge 2 commits into
Conversation
…_codes The Python port still used the old ansi-regex pattern while the JS SDK was rewritten (#895) to match OSC sequences non-greedily up to the first string terminator and CSI sequences without requiring one. Aligns both implementations and mirrors the 14-case Python test suite into the JS SDK, which had no stripAnsi tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 9bce30f The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
PR SummaryLow Risk Overview Reviewed by Cursor Bugbot for commit 9bce30f. Bugbot is set up for automated code reviews on this repo. Configure here. |
Package ArtifactsBuilt from a7ff0fe. Download artifacts from this workflow run. JS SDK ( npm install ./e2b-2.33.1-strip-ansi-python-sdk.0.tgzCLI ( npm install ./e2b-cli-2.13.3-strip-ansi-python-sdk.0.tgzPython SDK ( pip install ./e2b-2.32.0+strip.ansi.python.sdk-py3-none-any.whl |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f282d25cd8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| ] | ||
| ansi_escape = re.compile("|".join(pattern), re.UNICODE) | ||
| # OSC sequences only: ESC ] ... ST (non-greedy until the first ST) | ||
| osc = rf"(?:\u001B\][\s\S]*?{st})" |
There was a problem hiding this comment.
When Python build output contains ST-terminated ANSI string controls other than OSC, such as DCS/Sixel or tmux passthrough (ESC P ... ESC\\), this new OSC-only branch no longer removes the whole sequence: strip_ansi_escape_codes("\x1bPabc\x1b\\done") now leaves abc\x1b\\done, while the previous regex returned done. Since this helper sanitizes template log messages, those control payloads can leak into Python logs; keep handling the prior ST-terminated string-control form or add an explicit DCS branch.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in 9bce30f. The string-control branch now covers all ECMA-48 string controls (OSC, DCS, SOS, PM, APC — ESC ]/P/X/^/_ ... ST) in both the JS and Python implementations, so strip_ansi_escape_codes("\x1bPabc\x1b\\done") returns "done" again. Added DCS/Sixel, SOS, PM, APC, and unterminated-DCS test cases to both suites (20 mirrored cases per side, byte-for-byte identical output verified).
Addresses PR review feedback: the OSC-only branch regressed vs the old Python pattern, which stripped ST-terminated string controls like DCS (Sixel, tmux passthrough) through their terminator. Widen the branch to all ECMA-48 string controls (ESC ]/P/X/^/_ ... ST) in both the JS and Python implementations, keeping them byte-for-byte identical. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| """ | ||
| # Valid string terminator sequences are BEL, ESC\, and 0x9c | ||
| st = r"(?:\u0007|\u001B\u005C|\u009C)" | ||
| pattern = [ | ||
| rf"[\u001B\u009B][\[\]()#;?]*(?:(?:(?:(?:;[-a-zA-Z\d/#&.:=?%@~_]+)*|[a-zA-Z\d]+(?:;[-a-zA-Z\d/#&.:=?%@~_]*)*)?{st})", | ||
| r"(?:(?:\d{1,4}(?:[;:]\d{0,4})*)?[\dA-PR-TZcf-nq-uy=><~]))", | ||
| ] | ||
| ansi_escape = re.compile("|".join(pattern), re.UNICODE) | ||
| # OSC sequences only: ESC ] ... ST (non-greedy until the first ST) | ||
| osc = rf"(?:\u001B\][\s\S]*?{st})" | ||
| # CSI and related: ESC/C1, optional intermediates, optional params | ||
| # (supports ; and :) then final byte | ||
| csi = ( | ||
| r"[\u001B\u009B][\[\]()#;?]*(?:\d{1,4}(?:[;:]\d{0,4})*)?[\dA-PR-TZcf-nq-uy=><~]" | ||
| ) | ||
| ansi_escape = re.compile(f"{osc}|{csi}") | ||
| return ansi_escape.sub("", text) | ||
|
|
||
|
|
There was a problem hiding this comment.
🟡 Python's \d in the ported CSI regex matches any Unicode Nd decimal digit (e.g. Arabic-Indic ٣١ or fullwidth digits), not just ASCII 0-9 like JS's \d always does — so a CSI escape containing a non-ASCII digit is fully stripped in Python but left intact (except the trailing ASCII reset) in JS, breaking the byte-for-byte parity this PR claims. Fix by compiling the pattern with re.ASCII (verified this doesn't affect the OSC [\s\S] matching) or replacing \d with [0-9] in the csi pattern.
Extended reasoning...
The bug: In packages/python-sdk/e2b/template/utils.py (lines 316-329), the new csi pattern uses \d{1,4} for CSI parameter digits and [\dA-PR-TZcf-nq-uy=><~] for the final byte class. In Python, re's \d matches any Unicode category-Nd decimal digit by default (str patterns are Unicode in Python 3) — this includes things like Arabic-Indic digits (٣١, U+0663/U+0661) and fullwidth digits (１１, U+FF11). JS's \d in the mirrored ansiRegex (packages/js-sdk/src/utils.ts lines 88-99) is always ASCII-only ([0-9]), regardless of flags, since JS regex is byte/UTF-16-unit oriented for \d without a special mode extending it to non-ASCII digits.\n\nWhere it triggers: Any string cleaned by strip_ansi_escape_codes that contains a CSI escape (ESC [ ... ) whose parameter bytes include a non-ASCII Unicode decimal digit. The regex still matches the whole CSI sequence as if it were digits 0-9, so Python strips it entirely, while the JS implementation the PR is explicitly trying to mirror leaves it un-matched (only unrelated valid ASCII sequences elsewhere in the string get stripped).\n\nWhy nothing currently prevents it: The PR's own test suites (14 cases, mirrored in both SDKs) only use ASCII digit parameters (e.g. 31, 1;31;42, 38;5;82), so this divergence isn't exercised by any test added here, despite the PR description explicitly claiming byte-for-byte identical output between the two implementations on all cases.\n\nStep-by-step proof (verified empirically):\n1. Construct s = ESC + "[" + "٣١" (Arabic-Indic 3,1) + "mred" + ESC + "[0m".\n2. Run it through the exact ported Python pattern (no flags): ansi_escape.sub("", s) → 'red' — the whole leading CSI escape (including the non-ASCII digits) is stripped, along with the trailing reset.\n3. Run the equivalent through the exact JS ansiRegex-derived pattern on the identical string: only the trailing ASCII ESC[0m reset is stripped; the leading escape survives as '\x1b[٣١mred'.\n4. Recompiling the Python pattern with re.compile(pattern, re.ASCII) reproduces the JS behavior exactly ('\x1b[٣١mred'), and re-running the OSC newline-spanning test case (ESC]0;line1\nline2\x07after) with the same re.ASCII-compiled pattern still correctly yields 'after', confirming the fix doesn't regress the OSC [\s\S] matching (which relies on \S/\s, not \d, so it's unaffected).\n\nImpact: This is a genuine, reproducible divergence from the PR's stated goal (byte-for-byte JS/Python parity for cleaning template build logs), and it lands directly on the exact lines this PR modifies. That said, valid ANSI/ECMA-48 CSI sequences restrict parameter bytes to ASCII 0x30-0x3F, so no legitimate terminal or build tool emits non-ASCII digits inside a real CSI sequence — the divergence only shows up on malformed or adversarial escape sequences, and the only consequence is a cosmetic difference in sanitized log display text (no crash, data loss, or security impact).\n\nFix: Either compile the combined pattern with re.ASCII, or replace \d with [0-9] explicitly in the csi pattern (and note the final-byte character class [\dA-PR-TZcf-nq-uy=><~] would also need \d → [0-9] for full parity, or rely on re.ASCII to cover both in one place).
1 participant
Summary
The Python SDK's
strip_ansi_escape_codes(used to clean template build log messages) still used the old ansi-regex pattern, while the JS SDK'sstripAnsiwas rewritten in #895. This ports the current JS regex to Python so both SDKs clean logs identically: OSC sequences (hyperlinks, window titles) are matched non-greedily up to the first string terminator — including content spanning newlines — and CSI sequences are stripped without requiring a terminator.Following review feedback, both implementations now also strip the remaining ECMA-48 string controls — DCS (Sixel, tmux passthrough), SOS, PM, and APC — through their string terminator, so control payloads don't leak into cleaned logs. This goes beyond upstream
chalk/ansi-regex, click, and Rich, none of which fully strip DCS payloads, and restores what the old Python pattern handled.Also mirrors the Python test suite into the JS SDK (which previously had no
stripAnsitests) — 20 identical cases per side — and verified byte-for-byte identical output between the two implementations on all of them. Includes a patch changeset fore2band@e2b/python-sdk.Example
Log messages that previously leaked OSC or DCS sequences into template build output are now cleaned:
🤖 Generated with Claude Code