Skip to content

Fix ANSI escape sequences being split at wrapLine boundaries#223

Merged
shellicar merged 2 commits intomainfrom
fix/ansi-wrap
Apr 8, 2026
Merged

Fix ANSI escape sequences being split at wrapLine boundaries#223
shellicar merged 2 commits intomainfrom
fix/ansi-wrap

Conversation

@shellicar
Copy link
Copy Markdown
Owner

Problem

wrapLine iterated over graphemes with Intl.Segmenter and called stringWidth on each individual grapheme. ANSI escape sequences like \x1b[7m were split into their component characters (\x1b, [, 7, m) by the segmenter.

This produced two visible symptoms:

Symptom A — the ESC byte has zero measured width so it stuck to the old line, while the remaining chars ([7m) each measured as width 1 and rendered as literal text on the new line. This is the [7m visible in the terminal screenshot.

Symptom B — the non-ESC bytes of any ANSI sequence inflate the apparent line width by the number of those bytes, causing the split to fire too early. A \x1b[33m prefix made the line look 3 cols wider than it was, so only 7 visible chars would fit on a 10-col line instead of 10.

Fix

Tokenize the string into ANSI sequences and plain-text runs before iterating graphemes. ANSI sequences are buffered in pendingAnsi and prepended to whichever wrapped line receives the next visible character, so they always travel with their target rather than being stranded.

The CSI regex (ESC [ params final-letter) is written as new RegExp rather than a literal to avoid Biome's noControlCharactersInRegex rule.

Tests

Added 5 tests to a new test/reflow.spec.ts (also wires up vitest for claude-core, which previously had no test runner):

  • Plain text wrapping — regression baseline
  • No-wrap early return — regression baseline
  • Symptom A: ESC stranded at boundary (cursor at EOL)
  • Symptom B: color codes before text shift the split column
  • Symptom B: mid-line cursor sequence shifts the split column

All 903 workspace tests pass.

…boundaries

wrapLine iterated over graphemes with Intl.Segmenter and called stringWidth on
each individual grapheme. ANSI escape sequences like \x1b[7m were split into
their component characters (\x1b, [, 7, m) by the segmenter. The ESC byte has
zero measured width so it stuck to the old line; the remaining chars each
measured as width 1, so they rendered as literal "[7m" on the new line.

The same per-grapheme measurement also inflated the apparent line width by the
number of non-ESC bytes in any sequence, causing splits to fire too early —
a \x1b[33m prefix made the line look 3 chars wider than it was.

Fix: tokenize the string into ANSI sequences and plain-text runs using a CSI
regex before iterating graphemes. ANSI sequences are buffered in pendingAnsi
and prepended to whichever wrapped line receives the next visible character,
so they always travel with their target rather than being stranded.

Also wires up vitest for claude-core (previously had no test script).
@shellicar shellicar added this to the 1.0 milestone Apr 8, 2026
@shellicar shellicar added the bug Something isn't working label Apr 8, 2026
@shellicar shellicar self-assigned this Apr 8, 2026
@shellicar shellicar requested a review from bananabot9000 April 8, 2026 05:53
@shellicar shellicar enabled auto-merge (squash) April 8, 2026 05:53
Copy link
Copy Markdown
Collaborator

@bananabot9000 bananabot9000 left a comment

Choose a reason for hiding this comment

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

Clean ANSI-aware reflow fix. Tests first, focused scope 🍌

The tokenize-then-buffer approach is the right call. Splitting ANSI sequences into graphemes was always going to break — Intl.Segmenter has no concept of escape sequences. The pendingAnsi buffer pattern ensures ANSI codes travel atomically with their target visible character. No post-processing pass, no fixup logic.

ANSI_RE scope: \x1b\[[^a-zA-Z]*[a-zA-Z] covers CSI sequences only. Correct for this codebase (cursor, color, reset). OSC (\x1b]), raw ESC, and other forms aren't produced by the ANSI helpers. Good scoping decision.

Edge case check — trailing ANSI with no following visible char: pendingAnsi accumulates, never flushed by placeChar, then current += pendingAnsi at the end catches it. Falls through to result.push(current). Handled correctly.

Edge case check — consecutive ANSI sequences: Multiple codes without intervening text (\x1b[1m\x1b[33m) concatenate in pendingAnsi and flush together on the next visible char. Correct.

Tests: Clear symptom-based naming. Importing actual ANSI constants from ../src/ansi rather than hardcoding escape strings means the tests break if the constants change. Good coupling.

computeLineSegments/rewrapFromSegments left for separate PR: Noted in the session log. Same root cause, different call site. Correct scoping.

vitest.config.ts: Missing trailing semicolon after }); — Biome may or may not care depending on your config, but worth checking for consistency.

No sensitive data, no reverted changes, no suspect files. Session log addition is documentation, not artifacts.

Reviewed by BananaBot9000 🍌

@shellicar shellicar merged commit b52aefb into main Apr 8, 2026
4 checks passed
@shellicar shellicar deleted the fix/ansi-wrap branch April 8, 2026 08:00
shellicar added a commit that referenced this pull request Apr 8, 2026
CLAUDE.md current state was pointing at PR #196 (step 5b) as in-progress;
the architecture refactor completed through PR #199 (step 5e) weeks ago.
Config loading (#222), git delta injection (#225), and the ANSI wrap fix
(#223) had also shipped without being recorded.

Updated current state, added two missing recent-decisions entries, removed
a duplicate closing tag.

Created issue #226 for CLAUDE.md loading and added the design to
cli-features.md: load order, hot reload pattern, config opt-out.
shellicar added a commit that referenced this pull request Apr 8, 2026
CLAUDE.md current state was pointing at PR #196 (step 5b) as in-progress;
the architecture refactor completed through PR #199 (step 5e) weeks ago.
Config loading (#222), git delta injection (#225), and the ANSI wrap fix
(#223) had also shipped without being recorded.

Updated current state, added two missing recent-decisions entries, removed
a duplicate closing tag.

Created issue #226 for CLAUDE.md loading and added the design to
cli-features.md: load order, hot reload pattern, config opt-out.
@shellicar shellicar added the pkg: claude-core Core utilities — ansi, reflow, rendering label Apr 8, 2026
shellicar added a commit that referenced this pull request Apr 9, 2026
* Add CLAUDE.md loading issue #226 and update stale harness

CLAUDE.md current state was pointing at PR #196 (step 5b) as in-progress;
the architecture refactor completed through PR #199 (step 5e) weeks ago.
Config loading (#222), git delta injection (#225), and the ANSI wrap fix
(#223) had also shipped without being recorded.

Updated current state, added two missing recent-decisions entries, removed
a duplicate closing tag.

Created issue #226 for CLAUDE.md loading and added the design to
cli-features.md: load order, hot reload pattern, config opt-out.

* WIP: Add cachedReminders and CLAUDE.md loading (feature incomplete)

Adds cachedReminders?: string[] to RunAgentQuery — each entry becomes a
<system-reminder> block prepended to the first user message of a new
conversation. Stored in history so the prefix is cached by the API on
every subsequent turn.

ClaudeMdLoader reads ~/.claude/CLAUDE.md, CLAUDE.md, .claude/CLAUDE.md,
and CLAUDE.md.local at startup (missing files silently skipped). Content
is combined under a single instruction prefix and passed as cachedReminders.

Stopping here to fix a separate bug in systemReminder (injected into
tool-result messages instead of only human turns) before completing
this feature. Will rebase onto the fix once it lands.

Closes #226 (not yet ready — resuming after systemReminder fix).

* Complete CLAUDE.md loading: config flag, cachedReminders tests

The WIP commit had ClaudeMdLoader, the cachedReminders SDK field, and the
wiring in main.ts/runAgent.ts, but was missing two things:

1. The claudeMd.enabled config flag. Loading is on by default; setting
   it to false in sdk-config.json disables it entirely. Follows the same
   .optional().default().catch() pattern as historyReplay so invalid
   values silently fall back to the default rather than crashing.

2. Tests for the cachedReminders injection path in AgentRun:
   - injects reminder as first block when history is empty
   - skips injection when the conversation already has messages
   The second test asserts absence of a <system-reminder> block rather
   than string content type, because RequestBuilder converts all string
   content to arrays before the streamer sees it.

Closes #226

* Re-inject cachedReminders after compaction

The injection condition was `history.messages.length === 0`, which only
covered a fresh conversation. After compaction, the history contains one
message — the compaction block (assistant role) — so the condition was
false and reminders were not re-injected.

This is wrong. Compaction drops all content before the compaction block,
including the first user message that held the cached reminders. The
next human turn needs the reminders re-injected so they are present in
the effective context.

Fix: change the condition to check for absence of user messages rather
than an empty history. After compaction only the assistant compaction
block remains — no user messages — so injection correctly fires. Once
the new user message (with reminders) is pushed, subsequent turns have a
user message in history and injection is correctly skipped.

Test added first to prove the bug: post-compaction history with only the
compaction block, verifying the first user message sent to the API
contains a <system-reminder> block. The test failed before the fix and
passes after.

* Fix Biome violations in ClaudeMdLoader files

The WIP commit had two issues caught by the pre-push hook:
- INSTRUCTION_PREFIX split across lines (format violation)
- Braces missing on single-line if return (useBlockStatements)
- Four non-null assertions in the spec (noNonNullAssertion)

Replaced ! assertions with ?? '' — the tests that use the result for
string operations still work correctly, and the null case would produce
an empty string that fails the subsequent toContain assertions anyway.

* Session log 2026-04-09 — systemReminder fix, CLAUDE.md loading

* Add claudeMd to generated JSON schema

* Use IFileSystem in ClaudeMdLoader: cwd/homedir from fs, read on every turn

IFileSystem changed to an abstract class with cwd() added alongside
homedir(), so the filesystem owns all path context. NodeFileSystem
implements cwd() via process.cwd(); MemoryFileSystem takes a cwd
constructor param so tests can set it alongside home.

ClaudeMdLoader drops the separate cwd/home constructor params and calls
fs.cwd()/fs.homedir() inside getContent(), keeping the constructor to
a single IFileSystem argument. A nodeFs singleton is exported from the
fs entry so callers import it directly instead of newing a NodeFileSystem.

Content was read once before the loop and stored in cachedReminders,
making the on-demand design pointless. It now reads inside the loop on
every turn so CLAUDE.md changes are picked up without a watcher.

* Fix fs imports.

* Linting.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working pkg: claude-core Core utilities — ansi, reflow, rendering

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants