Skip to content

fix(doctor): strip C1 control characters from doctor error messages#103383

Closed
lsr911 wants to merge 2 commits into
openclaw:mainfrom
lsr911:fix/doctor-error-c1-control-chars
Closed

fix(doctor): strip C1 control characters from doctor error messages#103383
lsr911 wants to merge 2 commits into
openclaw:mainfrom
lsr911:fix/doctor-error-c1-control-chars

Conversation

@lsr911

@lsr911 lsr911 commented Jul 10, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

What Problem This Solves

scrubDoctorErrorMessage() in src/flows/doctor-error-message.ts strips control characters before doctor/lint/repair errors are printed to the terminal, but it only removed the C0 range (0x00-0x1f) and DEL (0x7f). It did not remove the C1 control range (0x80-0x9f), which includes the CSI introducer U+009B — an alternative ANSI escape prefix equivalent to ESC [. A C1-bearing error message could therefore reach the terminal and drive cursor/color escapes.

Change

Extend the existing predicate to also drop 0x80-0x9f:

if (code > 0x1f && code !== 0x7f && (code < 0x80 || code > 0x9f)) {
  stripped += raw.charAt(index);
}

Same C1 completion Peter merged for renderTerminalBufferText in #103274 ("Extended the existing terminal-text residual filter through DEL/C1 while preserving tabs").

Testing / Proof

Added a focused regression test src/flows/doctor-error-message.test.ts that calls the real scrubDoctorErrorMessage and asserts:

  • U+009B (CSI) is removed while surrounding text is preserved
  • the full C1 range 0x80-0x9f is removed
  • printable Unicode above C1 (accented Latin, CJK, emoji) is preserved
  • C0 controls and DEL are still stripped
$ node scripts/run-vitest.mjs run src/flows/doctor-error-message.test.ts

 RUN  v4.1.8 D:/IdeaWork/openclaw-repo

 Test Files  1 passed (1)
      Tests  4 passed (4)
   Duration  2.36s

The test exercises the modified function directly, so the passing run demonstrates the exact before/after behavior: on current main the U+009B and full-C1 cases fail (bytes retained), and with this change they are removed while ordinary Unicode survives.

@lsr911 @claude
fix(doctor): strip C1 control characters from doctor error messages
5e7a652 
scrubDoctorErrorMessage() used a whitelist that kept characters
above 0x1f except DEL (0x7f). C1 control characters (0x80-0x9f)
satisfy both conditions and pass through unfiltered into terminal
output shown by openclaw doctor.

Add C1 exclusion to the whitelist. Same pattern as the C1 fixes
in openclaw#103226, openclaw#103274, openclaw#103379-openclaw#103382.

Co-Authored-By: Claude <[email protected]>
@openclaw-barnacle openclaw-barnacle Bot added size: XS triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. and removed triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. labels Jul 10, 2026
@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal backlog priority with limited blast radius. labels Jul 10, 2026
@clawsweeper

clawsweeper Bot commented Jul 10, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed July 11, 2026, 4:13 AM ET / 08:13 UTC.

Summary
The PR extends the capped doctor error-message sanitizer to remove U+0080–U+009F and adds four focused regression tests.

PR surface: Source 0, Tests +35. Total +35 across 2 files.

Reproducibility: yes. at source level with high confidence: live main retains U+0080–U+009F and interpolates the result into doctor lint and repair output, although no real invocation was run or attached.

Review metrics: none identified.

Stored data model
Persistent data-model change detected: migration/backfill/repair: src/flows/doctor-error-message.test.ts. Confirm migration or upgrade compatibility proof before merge.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🐚 platinum hermit
Result: blocked until real behavior proof from a real setup is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • [P1] Post redacted terminal output, logs, or a recording from a real doctor/lint/repair path containing a C1-bearing error; remove credentials, IP addresses, phone numbers, private endpoints, and other sensitive details.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR provides direct unit-test output only; add redacted terminal output, logs, or a recording from a real doctor/lint/repair run, then update the PR body to trigger a fresh review or ask a maintainer to comment @clawsweeper re-review.

Risk before merge

  • [P1] Reviewers have not yet observed a C1-bearing failure passing through a real doctor, lint, or repair invocation and emerging sanitized in terminal-visible output.

Maintainer options:

  1. Decide the mitigation before merge
    Preserve the doctor-specific 256-code-unit cap, apply the proposed C1 exclusion, retain the focused regression tests, and add redacted real-path terminal evidence before merge.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

  • [P1] No autonomous code repair is needed; contributor-provided real behavior proof is the remaining merge gate.

Security
Cleared: The diff tightens untrusted terminal text filtering and adds tests without introducing dependency, workflow, permission, secret, or artifact-execution risk.

Review details

Best possible solution:

Preserve the doctor-specific 256-code-unit cap, apply the proposed C1 exclusion, retain the focused regression tests, and add redacted real-path terminal evidence before merge.

Do we have a high-confidence way to reproduce the issue?

Yes at source level with high confidence: live main retains U+0080–U+009F and interpolates the result into doctor lint and repair output, although no real invocation was run or attached.

Is this the best way to solve the issue?

Yes: extending the existing doctor-owned capped sanitizer is the narrowest maintainable solution and aligns it with the established terminal C0/DEL/C1 filtering invariant.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 81ce206ee84f.

Label changes

Label justifications:

  • P2: This is a bounded doctor terminal-output correctness and hardening fix with limited blast radius.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🐚 platinum hermit.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR provides direct unit-test output only; add redacted terminal output, logs, or a recording from a real doctor/lint/repair run, then update the PR body to trigger a fresh review or ask a maintainer to comment @clawsweeper re-review.
Evidence reviewed

PR surface:

Source 0, Tests +35. Total +35 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 1 2 2 0
Tests 1 35 0 +35
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 37 2 +35

What I checked:

Likely related people:

  • giodl73-repo: Introduced the doctor lint health-check contract and original scrubbed, capped error-message behavior in merged PR Doctor: add health-check contract and --lint validation #80055. (role: introduced behavior; confidence: high; commits: 9a5f2f61e76f; files: src/flows/doctor-error-message.ts, src/flows/doctor-lint-flow.ts)
  • mushuiyu886: Authored the latest focused hardening of the exact sanitizer and its UTF-16 truncation regression coverage. (role: recent area contributor; confidence: high; commits: 7daa0909357e; files: src/flows/doctor-error-message.ts, src/flows/doctor-lint-flow.test.ts, src/flows/doctor-repair-flow.test.ts)
  • steipete: Co-authored the latest merged sanitizer hardening and is connected to recent review and maintenance of this path. (role: recent collaborator; confidence: medium; commits: 7daa0909357e; files: src/flows/doctor-error-message.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.
Review history (1 earlier review cycle)
  • reviewed 2026-07-10T10:34:49.639Z sha 5e7a652 :: needs real behavior proof before merge. :: none

@lsr911
test(doctor): cover C1 control-character removal in scrubDoctorErrorM…
acbf2bc 
…essage

Add focused regression coverage for the C1 range (0x80-0x9f) removal:
- U+009B (CSI introducer) stripped, surrounding text preserved
- full C1 range 0x80-0x9f removed
- printable Unicode above C1 (accented latin, CJK, emoji) preserved
- C0 controls and DEL still stripped

Signed-off-by: lsr911 <[email protected]>
@openclaw-barnacle openclaw-barnacle Bot added the triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. label Jul 11, 2026
This was referenced Jul 11, 2026
Closed
Closed
@lsr911

lsr911 commented Jul 11, 2026

Copy link
Copy Markdown
Contributor Author

Superseded by #104362 — a single sink-wide C1 (0x80–0x9f) pass that folds this sink's fix in with a valid U+009B regression, alongside an audit of every other control-character sink (already-covered / deferred / out-of-scope). This consolidates the isolated C1 PRs per the direction on #103226. Closing in favor of the consolidated review.

@lsr911 lsr911 closed this Jul 11, 2026
lsr911 added a commit to lsr911/openclaw that referenced this pull request Jul 12, 2026
@lsr911
fix: complete C1 control-char (0x80-0x9f) coverage across terminal/lo…
c817223 
…g sinks

Terminal-control sanitizers guard untrusted text before it reaches a terminal
or log (CWE-117). The canonical range is C0 (0x00-0x1f), DEL (0x7f), and C1
(0x80-0x9f) - the range already used by the shared sanitizeForLog and by
renderTerminalBufferText (openclaw#103274). C1 includes the 8-bit CSI introducer U+009B.

Deliberate sink-wide pass: closes the C1 gap in twelve terminal/log output
sinks, each keeping its own strip/escape/replace/truncate/reject action and
gaining a focused valid-U+009B regression test. The PR body audits every other
control-char sink (already-covered, deferred, or out-of-scope).

Consolidates the isolated PRs openclaw#103379 openclaw#103380 openclaw#103381 openclaw#103382 openclaw#103383 openclaw#103402

Signed-off-by: lsr911 <[email protected]>
openclaw#103405 and re-lands the sanitizeForConsole fix from openclaw#103226 cleanly.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

P2 Normal backlog priority with limited blast radius. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: XS status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@lsr911