fix(sequential-thinking): render box correctly with ANSI codes and multi-line thoughts#4005
Open
abhicris wants to merge 1 commit intomodelcontextprotocol:mainfrom
Open
Conversation
…codes and for multi-line thoughts The formatThought box renderer used `string.length` on the chalk-colored header to size the border. When the terminal supports colour (the default on any TTY), chalk wraps the prefix in CSI escape sequences, inflating the raw character count by ~10 code points without adding any visible columns. The border is then drawn wider than the visible header and the right-hand `│` floats free of the frame. The same function also did not handle thoughts that contain newlines: `thought.padEnd(border.length - 2)` is applied to the whole string, so a multi-line thought produces one very long padded row with the trailing `│` pushed onto a new line — the box is shattered into fragments. This change: - strips CSI escape sequences when measuring the header width so the border matches the visible content - splits multi-line thoughts and renders each line as its own framed row, sizing the box to the widest line Adds `__tests__/format.test.ts` which asserts that every line of the rendered frame has the same visible width (ANSI-stripped length) for regular, revision, branch, multi-line, and width-dominated cases. All existing tests continue to pass.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
SequentialThinkingServer.formatThoughthas two rendering bugs that break the ASCII box written to stderr whenDISABLE_THOUGHT_LOGGINGis not set:ANSI escape sequences inflate the border width. The border is sized as
Math.max(header.length, thought.length) + 4whereheaderis the chalk-colored string. On any terminal that supports colour (the default on a TTY), chalk wraps the prefix in CSI escape sequences that add ~10 characters to.lengthwithout adding any visible columns. The border is then drawn wider than the visible header, and the right-hand│floats free of the frame.Multi-line thoughts shatter the frame.
thought.padEnd(border.length - 2)is applied to the entire string, so a thought that contains\nproduces one long padded row with the trailing│pushed onto a new line.Reproduction (on current
main)Running the compiled server locally with
FORCE_COLOR=3:After the patch, both cases render as clean rectangles (visible widths verified equal across every line of the frame).
Server Details
Motivation and Context
The formatted box is the server's primary diagnostic output. Broken rendering makes it hard to skim the log of a thinking run, and since it only manifests when colour is on, it doesn't show up in the existing test suite (which mocks
chalkto return plain strings).How Has This Been Tested?
npx vitest run— all existing 14 tests plus 7 new tests in__tests__/format.test.tspass (21/21).dist/lib.jswithFORCE_COLOR=3across (a) a basic thought, (b) a multi-line thought, and (c) a branch with a long context string. Every case produces a frame whose ANSI-stripped line widths are all equal.npm run buildpasses with no new TS errors.The new tests assert rectangularity directly: for each rendered block, every line's ANSI-stripped length must be identical. This catches both the CSI-inflation bug and the multi-line bug, and will catch any future regression that desynchronises header width and border width.
Breaking Changes
None — this is an internal renderer change. No tool schema, no API, no environment variable, and no on-the-wire behavior changes.
Types of changes
Checklist
Additional context
The fix adds a small
visibleWidthhelper that strips CSI sequences before measuring, and splits the thought on\nso each line becomes its own framed row sized tomax(headerWidth, widestThoughtLine). The ANSI regex is scoped to CSI (\x1b\[...[A-Za-z]), which is what chalk emits; OSC/DCS sequences are not relevant here.No new runtime dependencies. The existing
chalkmock in__tests__/lib.test.tsis intentionally left untouched so the legacy tests remain deterministic; the newformat.test.tsuses real chalk behavior and asserts rectangularity, which is what actually matters on end-user terminals.Part of open-source MCP work from kcolbchain / Abhishek Krishna.