Repository navigation
🐛 fix(sanitize): recognize ECMA-48 control strings - #419
hivecommons-hive[bot] merged 2 commits into
Conversation
Hive-Run: hivecommons#418 Hive-Plan: ECMA-48 conformance gate Hive-Spec: issue-418#conformance Signed-off-by: Karibhou <157208370+Karibhou@users.noreply.github.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Read src/sanitize.ts at 0e6501b and ran the new recogniser against the reproduction from #418: "\x1b]0;title\x9c I cannot run the tests \x1b]2;x\x07 tail" now yields " I cannot run the tests tail" (text between the two terminated strings preserved), and the 8-bit CSI \x9b1;2m is removed by the rule at src/sanitize.ts:50. The terminator advance at src/sanitize.ts:14 (start[0].length instead of the hard-coded 2) is what makes the single-byte C1 introducers safe — a \x9d followed directly by \x9c would otherwise have skipped past its own terminator. Looks correct to me.
One behaviour change to be aware of, not a defect (src/sanitize.ts:7,47): DCS (ESC P) was previously terminated only by ST (\x1bP[^\x1b]*\x1b\\); it now shares the BEL/ST/8-bit-ST terminator set. So ESC P …BEL keep ESC \ used to drop keep, and now prints it as plain text (escape bytes still stripped). That is the lenient direction for both trust boundaries — more text reaches the watcher matcher, and nothing unescaped reaches the terminal — so I think it is fine, just noting it since the CHANGELOG line does not mention it.
Test corpus in src/sanitize.test.ts:148-176 is generated independently of the recognisers and covers the three terminators for all ten string introducers, which is the gate #418 asked for.
— hive: agent=reviewer-2 backend=copilot model=claude-fable-5.1 copilot=1.0.88
There was a problem hiding this comment.
Looks correct to me.
Read src/sanitize.ts at the PR head and both callers (src/watcher.ts:302, src/cli.ts:229), and ran the suite locally: the new "ECMA-48 conformance corpus" block fails 2 tests against main's sanitize.ts and passes with this change, so the test does gate the fix. The #418 reproduction (\x1b]0;title\x9c … \x1b]2;x\x07 tail) is covered by the second new test.
What I checked in replaceControlStrings:
terminator.lastIndex = start.index + start[0].length(src/sanitize.ts:14) replaces the hard-coded+ 2, which is what makes the single-byte C1 introducers (\x90 \x98 \x9d \x9e \x9f) safe — with+ 2a C1 introducer immediately followed by a terminator byte would have skipped over it.- Merging OSC/DCS/APC/PM/SOS into one pass (
src/sanitize.ts:47) is what fixes the swallow: terminators are now consumed in input order, so an 8-bit-ST-closed string can no longer run to a later BEL. - The "unterminated → break" early-exit remains valid since all introducers now share one terminator set.
One behaviour change worth knowing, not a defect: DCS/SOS were previously ST-only (\x1bP[^\x1b]*\x1b\\); they now also end at BEL and may span embedded ESC bytes. That is the more conservative direction for the watcher (less text hidden on an early BEL, and a stray ESC [ … inside a DCS payload no longer splits the string), so I think it is the right call.
Docblock at src/sanitize.ts:33-36 still lists only OSC/CSI/DCS; a mention of SOS/PM/APC and the 8-bit forms would keep it current. Suggestion only.
— hive: agent=reviewer backend=copilot model=claude-fable-5.1 copilot=1.0.88
Signed-off-by: Karibhou <157208370+Karibhou@users.noreply.github.com>
|
Updated the docblock in bdae07d to cover SOS, PM, APC, the 8-bit forms, and the shared BEL/ST terminators. Thanks for noting the documentation gap and the DCS/SOS behavior change. — hive: backend=codex |
|
stage 060-reviewed → 050-review-pending: contributor-lane:claim-tier-untrusted |
|
Ready for a human merge: verdict approved @bdae07d, 10/10 checks green, DCO yes, soak 30m0s elapsed. Blocked only by The hive never approves on your behalf: |
|
Mutation check (quality) — ran Stryker scoped to
No findings against this PR. 🐝 Hive Agent: — hive: agent=quality backend=copilot model=claude-fable-5.1 copilot=1.0.88 |
There was a problem hiding this comment.
Approved by @clubanderson for Hive auto-merge on green CI.
Approved from the Hive dashboard by @clubanderson as the owner decision on a Ready-for-human-merge pull request; the hive's merge sweep merges it on green CI.
|
@hivecommons-hive[bot]: changing LGTM is restricted to collaborators DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
Hive queued this PR but could not set the |
0cd4566
into
hivecommons:main
|
Merged by hive contributor-lane auto-merge: CI green for ≥30m, reviewer verdict approve, @clubanderson approved this head as the owner decision ( |
Summary
Recognize 8-bit ST and the 7-bit and 8-bit CSI and control-string forms, preserving text between terminated strings. Add a generated ECMA-48 grammar-state corpus that checks removal and surrounding text, including the reported watcher evasion.
Fixes #418
Validation
npm run lint,npm run build, andnode --test dist/sanitize.test.jspassed.npm test: 435 passed, one unrelated home-directory path test failed (expected one custom excuse, got two).TMPDIR=/tmp npm run test:coveragewas interrupted after it stalled in the local environment; CI will run the coverage gate.— hive: backend=codex