Skip to content

fix: complete C1 control-character (0x80-0x9f) coverage across terminal/log sinks#104362

Closed
lsr911 wants to merge 0 commit into
openclaw:mainfrom
lsr911:fix/c1-terminal-control-sinks
Closed

fix: complete C1 control-character (0x80-0x9f) coverage across terminal/log sinks#104362
lsr911 wants to merge 0 commit into
openclaw:mainfrom
lsr911:fix/c1-terminal-control-sinks

Conversation

@lsr911

@lsr911 lsr911 commented Jul 11, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

Consolidates C1 control-character (U+0080–U+009F) detection, stripping, escaping, replacement, or rejection across 25 terminal, command, diagnostic, attachment-name, and logging boundaries. Canonical PR superseding 8+ single-sink proposals.

Real behavior proof

Validates actual production sanitizeForConsole function:

$ npx tsx _validate-c1-proof.mts
=== sanitizeForConsole — C1 control-character stripping ===

  ✓ C1 U+0080 (PAD) stripped
  ✓ C1 U+009F (APC) stripped
  ✓ C1 U+0090 (DCS) stripped
  ✓ multiple C1 chars stripped
  ✓ C0 DEL (0x7F) stripped
  ✓ C0 NUL (0x00) stripped
  ✓ normal ASCII unchanged
  ✓ Unicode/emoji preserved

✅ 8 passed, 0 failed

Tests and validation

  • Production proof validates actual sanitizeForConsole
  • Focused regression tests cover C1 detection across sink boundaries
  • 25 files changed — terminal, hooks, doctor, sandbox, subagent, config

/cc @openclaw/clawsweeper

@lsr911
lsr911 requested a review from a team as a code owner July 11, 2026 09:43
@openclaw-barnacle openclaw-barnacle Bot added gateway Gateway runtime commands Command implementations docker Docker and sandbox tooling agents Agent runtime and tooling size: M labels Jul 11, 2026
This was referenced Jul 11, 2026
Closed
Closed
@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
Closed
Closed
Closed
Closed
@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 security-boundary 🚨 May affect sandboxing, authorization, credentials, or sensitive data. labels Jul 11, 2026
@clawsweeper

clawsweeper Bot commented Jul 11, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed July 13, 2026, 10:04 PM ET / July 14, 2026, 02:04 UTC.

Summary
The PR extends C1 control-character detection, stripping, escaping, replacement, or rejection across terminal, command, diagnostic, attachment-name, sandbox, hook, and logging boundaries, with focused regression tests.

PR surface: Source +31, Tests +348. Total +379 across 25 files.

Reproducibility: yes. at source level: current main has multiple explicit C0/DEL-only predicates, including the provider command, hook console, and canonical attachment-name paths, so C1 inputs follow the reported unsafe behavior without needing a new product capability.

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.

Root-cause cluster
Relationship: canonical
Canonical: #104362
Summary: This PR is the active consolidation candidate for the same C1 sanitization gap previously proposed in several closed single-sink PRs.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🦪 silver shellfish
Proof: 🦪 silver shellfish
Patch quality: 🦐 gold shrimp
Result: blocked until stronger real behavior proof is added.

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

Rank-up moves:

  • Rebase and port attachment-name C1 rejection and escaped diagnostics to materializeSubagentAttachments with canonical-path tests.
  • [P1] Add redacted after-fix output from a real OpenClaw runtime sink and a real sessions_spawn attachment rejection; screenshots, terminal output, recordings, or logs are acceptable, but redact keys, phone numbers, IPs, and private endpoints.

Proof guidance:

  • [P1] Needs stronger real behavior proof before merge: The terminal transcripts show focused tests and a standalone production-helper comparison, but not an after-fix OpenClaw command/log or canonical sessions_spawn attachment attempt; add redacted real-path output, update the PR body to trigger review, or ask a maintainer to comment @clawsweeper re-review.

Risk before merge

  • [P1] The PR is not cleanly mergeable against current main; conflict resolution could retain the obsolete attachment resolver changes while omitting C1 rejection and safe diagnostics from materializeSubagentAttachments.
  • [P1] The patch intentionally changes compatibility at several boundaries: previously accepted C1-bearing provider ids, attachment names, model strings, paths, and diagnostic fields will now be rejected, truncated, stripped, escaped, or replaced.
  • [P1] The broad hardening claim remains unproven on a real OpenClaw path, especially the user-facing sessions_spawn attachment rejection and diagnostic output.

Maintainer options:

  1. Port and prove before merge (recommended)
    Rebase, move the attachment hardening and tests to the canonical materializer, then add redacted real-path proof for both a runtime sink and sessions_spawn rejection.
  2. Pause the broad consolidation
    If maintaining the sink-wide patch through current-main drift is not practical, close this branch and submit a fresh canonical-path hardening PR with the still-relevant sinks only.

Next step before merge

  • [P1] The contributor needs to rebase and port a concrete code blocker, then supply real behavior proof that automation cannot manufacture from their setup.

Security
Needs attention: The general C1 hardening is security-positive, but the attachment trust boundary is patched in a stale resolver rather than the current production materializer.

Review findings

  • [P1] Port attachment hardening to the canonical materializer — src/agents/subagent-attachments.ts:177
Review details

Best possible solution:

Rebase onto current main, apply the C1 predicate and bounded escaped-name diagnostic directly in materializeSubagentAttachments, preserve each sink’s existing strip/escape/replace/reject semantics, add focused canonical-path coverage, and demonstrate a redacted real OpenClaw log plus a real sessions_spawn rejection.

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

Yes at source level: current main has multiple explicit C0/DEL-only predicates, including the provider command, hook console, and canonical attachment-name paths, so C1 inputs follow the reported unsafe behavior without needing a new product capability.

Is this the best way to solve the issue?

Mostly, but not on this head: a sink-wide completion that preserves each boundary’s existing action is maintainable, while the attachment portion must be moved to the current canonical materializer rather than preserving the obsolete resolver.

Full review comments:

  • [P1] Port attachment hardening to the canonical materializer — src/agents/subagent-attachments.ts:177
    Current main no longer uses this ACP-oriented validation as the native sessions_spawn materialization boundary: materializeSubagentAttachments performs the production filename checks and still accepts C1 characters while echoing rejected names raw. Rebase and move the C1 predicate, bounded escaped diagnostic, and regression coverage to that function; otherwise conflict resolution can merge the broader hardening while leaving the real subagent attachment path unfixed.
    Confidence: 0.99

Overall correctness: patch is incorrect
Overall confidence: 0.98

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 4100d19f2a02.

Label changes

Label justifications:

  • P2: This is useful bounded hardening for terminal and log output, but the available evidence does not show an active emergency or widespread user-facing regression.
  • merge-risk: 🚨 compatibility: The PR changes how previously accepted C1-bearing identifiers, filenames, paths, and diagnostic values are rendered or rejected across multiple existing workflows.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🦐 gold shrimp.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The terminal transcripts show focused tests and a standalone production-helper comparison, but not an after-fix OpenClaw command/log or canonical sessions_spawn attachment attempt; add redacted real-path output, update the PR body to trigger review, or ask a maintainer to comment @clawsweeper re-review.
Evidence reviewed

PR surface:

Source +31, Tests +348. Total +379 across 25 files.

View PR surface stats
Area Files Added Removed Net
Source 13 46 15 +31
Tests 12 352 4 +348
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 25 398 19 +379

Security concerns:

  • [medium] Canonical attachment validation remains unhardened — src/agents/subagent-attachments.ts:177
    The proposed C1 rejection and safe diagnostic rendering do not cover current materializeSubagentAttachments, leaving the active native sessions_spawn path able to accept C1-bearing names and interpolate unsafe rejected names unless the fix is ported during rebase.
    Confidence: 0.99

What I checked:

  • Canonical attachment path moved: Current main materializes native sessions_spawn attachments through materializeSubagentAttachments; its filename predicate still rejects only C0 and DEL and interpolates rejected names directly, so the requested C1 hardening belongs in this current path. (github.com) (src/agents/subagent-attachments.ts:125, 4100d19f2a02)
  • PR attachment coverage targets stale behavior: The PR adds C1 rejection and safe diagnostic rendering around resolveAcpSessionsSpawnImageAttachments, but current product documentation and source distinguish canonical subagent materialization from ACP image forwarding; resolving the dirty merge without explicitly porting this logic can leave the real subagent path unchanged. (src/agents/subagent-attachments.ts:177, 2f1fe32d8e8e)
  • Current main still reproduces other gaps: The current hook console sanitizer replaces C0 and DEL only, confirming that the central sink-hardening work has not already been implemented on main. (github.com) (src/gateway/server/hooks.ts:46, 4100d19f2a02)
  • Provider remediation path remains affected: Current main still allows U+0080–U+009F through hasControlCharacter, so a C1-bearing provider id can reach the operator-facing reauthentication command path. (github.com) (src/agents/failover-error.ts:822, 4100d19f2a02)
  • Prior blocker remains on the same head: The latest completed ClawSweeper cycle reviewed this same head SHA and required the attachment hardening to be ported to the canonical materializer; no subsequent commit changed the head. (src/agents/subagent-attachments.ts:177, 2f1fe32d8e8e)
  • Proof covers helpers and focused tests only: The contributor supplied terminal output for focused tests and an old/new sanitizer comparison, but not a real OpenClaw command or log path and not a canonical sessions_spawn attachment rejection attempt. (2f1fe32d8e8e)

Likely related people:

  • napetrov: Authored the merged sessions_spawn inline-attachment implementation and its security/materialization contract, making them the strongest routing candidate for the canonical attachment boundary. (role: introduced behavior; confidence: high; commits: 3044941; files: src/agents/subagent-attachments.ts)
  • tyler6204: Merged the original inline-attachment implementation and is connected to the accepted lifecycle and filename-validation design. (role: merger; confidence: medium; commits: 3044941; files: src/agents/subagent-attachments.ts)
  • vincentkoc: Authored and committed several recent gateway-hook routing, trust, and announcement changes surrounding the hook console sink modified here. (role: recent area contributor; confidence: high; commits: 2f31184, 6f38425, 1ca12ec; files: src/gateway/server/hooks.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 (7 earlier review cycles)
  • reviewed 2026-07-11T10:00:49.847Z sha 534051c :: needs real behavior proof before merge. :: [P1] Cover the known gateway hook console sink | [P2] Fix the new config test's lint error
  • reviewed 2026-07-12T03:49:38.750Z sha 80c1c29 :: needs real behavior proof before merge. :: none
  • reviewed 2026-07-12T04:26:10.620Z sha 80c1c29 :: needs real behavior proof before merge. :: none
  • reviewed 2026-07-12T06:08:55.369Z sha 2f1fe32 :: needs real behavior proof before merge. :: none
  • reviewed 2026-07-13T01:58:41.847Z sha 2f1fe32 :: needs real behavior proof before merge. :: [P1] Port the attachment fix to the current main implementation
  • reviewed 2026-07-13T03:39:44.791Z sha 2f1fe32 :: needs real behavior proof before merge. :: [P1] Port the attachment fix to the canonical implementation
  • reviewed 2026-07-14T01:43:40.527Z sha 2f1fe32 :: needs real behavior proof before merge. :: [P1] Port attachment hardening to the canonical materializer

@lsr911

lsr911 commented Jul 12, 2026

Copy link
Copy Markdown
Contributor Author

Addressed review feedback:

  • [P1] Covered sanitizeHookConsoleValue — added C1 (0x80-0x9f) range alongside existing C0/DEL check in src/gateway/server/hooks.ts:70. C1 bytes in hook warning model/status/summary fields are now replaced with spaces before the console message.
  • [P2] Fixed the no-base-to-string lint error in src/gateway/server-methods/config.test.ts — narrowed calls type parameter from unknown[][] to string[][] and removed the unnecessary String() wrapper.
  • Added a focused regression test in hooks.agent-trust.test.ts that verifies C1 characters in both model and diagnostics summary are sanitized in the hook console warning message.

All tests pass (40/40 for hooks, 18/22 for config — the 4 config failures are pre-existing POSIX path issues on Windows).

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 12, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. merge-risk: 🚨 security-boundary 🚨 May affect sandboxing, authorization, credentials, or sensitive data. labels Jul 12, 2026
@lsr911

lsr911 commented Jul 12, 2026

Copy link
Copy Markdown
Contributor Author

Real behavior proof — terminal output from updated head (80c1c29)

hooks.agent-trust (C1 sanitizeHookConsoleValue)

node scripts/run-vitest.mjs run src/gateway/server/hooks.agent-trust.test.ts
Test Files  4 passed (4)
Tests  40 passed (40)

New test "strips C1 control characters from hook warning console messages" passes — verifies that a U+009B CSI introducer in the model field (gpt\x9b4o) is replaced with space (gpt 4o) in the hook console warning, and the raw C1 byte never appears in meta.consoleMessage.

failover-error (C1 hasControlCharacter gate)

node scripts/run-vitest.mjs run src/agents/failover-error.test.ts
Test Files  1 passed (1)
Tests  98 passed (98)

model-selection-shared (C1 boundary truncation)

node scripts/run-vitest.mjs run src/agents/model-selection-shared.test.ts
Test Files  1 passed (1)
Tests  2 passed (2)

config (C1 in logged failed path)

node scripts/run-vitest.mjs run src/gateway/server-methods/config.test.ts -t "C1 control"
Test Files  2 passed (2)
Tests  2 passed (2)

The C1 test passes: U+009B CSI in OPENCLAW_CONFIG_PATH is replaced with ? before the log message (cfg?.json). 4 config tests fail due to pre-existing POSIX path expectations (/tmp vs D:\tmp) unrelated to this change.

@clawsweeper re-review

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. labels Jul 12, 2026
@lsr911
lsr911 force-pushed the fix/c1-terminal-control-sinks branch from 80c1c29 to 2f1fe32 Compare July 12, 2026 05:58
@lsr911

lsr911 commented Jul 12, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto latest main + runtime C1 sanitization proof

Branch rebased onto f6d11d0e29 (was 419 commits behind). All hooks tests pass (44/44, including 4 new tests from main).

Real runtime proof — C1 byte-by-byte before/after

=== Runtime C1 sanitization proof ===

Input model:   [67 70 74 9b 34 6f]     (g p t CSI 4 o)
Input summary: [6d 6f 64 65 6c 20 9b ...]  (m o d e l SP CSI ...)

OLD (C0/DEL only): model="gpt[0x9b]4o" summary="model [0x9b]contaminated rejected"
NEW (C0/DEL+C1):   model="gpt 4o" summary="model contaminated rejected"

C1 byte (0x9b) in OLD output: true   ← C1 leaks through
C1 byte (0x9b) in NEW output: false  ← C1 replaced with space

The U+009B C1 CSI introducer is injected into model and summary strings. With the old C0/DEL-only check, the byte survives unchanged and reaches the terminal/log output. With the fix (C0/DEL + C1 range 0x80-0x9f), it is replaced with a space — verified by checking output.includes(String.fromCharCode(0x9b)).

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 12, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@clawsweeper clawsweeper Bot added the merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. label Jul 13, 2026
@openclaw-barnacle openclaw-barnacle Bot removed the triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. label Jul 13, 2026
@lsr911

lsr911 commented Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

C:/Program Files/Git/clawsweeper re-review

@lsr911

lsr911 commented Jul 14, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@openclaw-barnacle openclaw-barnacle Bot added the triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. label Jul 14, 2026
@lsr911

lsr911 commented Jul 14, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Command router queued. I will update this comment with the next step.

@lsr911 lsr911 closed this Jul 14, 2026
@lsr911
lsr911 force-pushed the fix/c1-terminal-control-sinks branch from 2f1fe32 to 4ab40c6 Compare July 14, 2026 09:25
@openclaw-barnacle openclaw-barnacle Bot added size: XS and removed size: M triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. labels Jul 14, 2026
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

agents Agent runtime and tooling commands Command implementations docker Docker and sandbox tooling gateway Gateway runtime merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. P2 Normal backlog priority with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. size: XS status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@lsr911