Skip to content

fix(sequentialthinking): harden thought-box rendering and validation#4100

Open
1060996408 wants to merge 2 commits intomodelcontextprotocol:mainfrom
1060996408:fix/sequentialthinking-rendering-and-validation
Open

fix(sequentialthinking): harden thought-box rendering and validation#4100
1060996408 wants to merge 2 commits intomodelcontextprotocol:mainfrom
1060996408:fix/sequentialthinking-rendering-and-validation

Conversation

@1060996408
Copy link
Copy Markdown

Description

Hardens the sequentialthinking reference server against several real-world inputs that currently render incorrectly or are silently ignored, and reports the right package version to MCP clients.

Server Details

  • Server: sequentialthinking
  • Changes to: tool handler, formatThought rendering, processThought API contract, server version reporting, cross-field input validation

Motivation and Context

While reading through the smallest reference server, the existing test output itself revealed alignment problems in the rendered thought box. Pulling the thread surfaced 11 distinct issues, all in lib.ts / index.ts:

# Where Issue
1 formatThought Header row was missing right-side padding, so when the thought was longer than the header the right of the header line hung in mid-air.
2 formatThought header.length included chalk's ANSI escape bytes, inflating the computed border width.
3 formatThought Thoughts containing \n broke the box layout (subsequent lines escaped the right border).
4 displayWidth (new) CJK ideographs were counted as 1 column instead of 2, narrowing the box.
5 displayWidth SMP-plane emoji (💭 🔄 🌿 — the prefixes the server itself emits) were counted as 1 column.
6 displayWidth ZWJ-joined emoji like 👨‍💻 were counted as 5 columns instead of 2.
7 formatThought \t characters were emitted verbatim; terminals advance to the next tab stop and broke the right border.
8 formatThought A long single-line thought produced a box wider than the terminal.
9 processThought Auto-adjusting totalThoughts mutated the caller-supplied input object.
10 index.ts McpServer was constructed with a hardcoded "0.2.0" while package.json had drifted to 0.6.2, so clients saw a stale version in serverInfo.
11 handler No cross-field validation: isRevision: true without revisesThought silently rendered (revising thought undefined); branchFromThought without branchId silently dropped the branch entry.

How Has This Been Tested?

  • npm test in src/sequentialthinking46 / 46 passing (was 14 / 14 before)
  • npm run build — clean under TypeScript strict mode
  • Wire-level smoke test via stdio JSON-RPC: serverInfo.version correctly reports 0.6.2, and tools/call with isRevision: true and no revisesThought returns a well-formed isError response ("revisesThought is required when isRevision is true") instead of rendering (revising thought undefined).
  • I have not wired this through a real LLM client end-to-end — happy to do that if a maintainer wants a specific scenario validated.

Coverage gain:

  • lib.ts: 84.93 % → 100 % line, 91.35 % branch
  • index.ts: 0 % → 80 % line (handler is now extracted and tested)
  • all files: ~50 % → 88.88 %

Breaking Changes

None. All changes are backward-compatible:

  • Bugs 1–8 fix rendering for inputs that previously produced visibly broken output.
  • Bug 9 only changes whether the caller's object is mutated; the response shape and thoughtHistory content are unchanged.
  • Bug 10 changes the version string the server reports — from a stale value to the correct one.
  • Bug 11 turns inputs that previously silently misrendered into proper isError responses. A client that was relying on the broken behaviour would have been seeing nonsense; the new error message points at the actual problem.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality) — validateThoughtData
  • Breaking change
  • Documentation update

Checklist

  • I have read the MCP Protocol Documentation
  • My changes follow MCP security best practices (no new IO, no new env vars, no new dependencies)
  • I have updated the server's README accordingly — README only documents inputs and config, neither of which change in this PR
  • I have tested this with an LLM client — wire-level smoke test only, see "How Has This Been Tested?"
  • My code follows the repository's style guidelines (kebab-case files, 2-space indent, Zod for input shape, .js import suffix)
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have documented all environment variables and configuration options — none added

Additional context

  • All Unicode-width handling is implemented via Node's built-in Intl.Segmenterno new dependencies.
  • The new displayWidth and wrapToWidth are exported from lib.ts so they are independently unit-tested and could be reused by other servers if useful.
  • The handler in index.ts is now an exported handleSequentialThinkingTool function. The auto-start of runServer() is guarded by an ESM import.meta.url === pathToFileURL(process.argv[1]).href check so importing the module in tests does not boot the stdio loop.
  • The commits are split as:
    1. fix(sequentialthinking): harden thought-box rendering and add cross-field validation — all lib.ts changes
    2. refactor(sequentialthinking): sync server version, extract handler, wire validator — all index.ts changes

🤖 Generated with Claude Code

Jia Xuan and others added 2 commits May 5, 2026 01:57
…ield validation

formatThought:
- pad the header row so its right border lines up with the rest of
  the box (previously hung in mid-air when the thought was longer
  than the header)
- account for chalk's ANSI escape bytes when measuring header width
- render multi-line thoughts as one row per line, expand tabs to a
  fixed width so terminal tab stops can't break the right border
- wrap thought lines whose display width exceeds 80 columns at
  grapheme boundaries, so the box never overflows a typical terminal
- new displayWidth helper uses Intl.Segmenter to count terminal
  columns correctly for CJK ideographs, SMP-plane emoji (💭🔄🌿),
  ZWJ-joined emoji like 👨‍💻, and VS-16 variation selectors

processThought:
- stop mutating the caller-supplied input object when auto-adjusting
  totalThoughts; work on a local copy and report the adjusted value
  in the response as before

new validateThoughtData:
- cross-field semantic checks Zod's field-level shape can't express:
  revisesThought is required (and must precede thoughtNumber) when
  isRevision is true; same for branchFromThought / branchId

lib.ts line coverage: 84.93% → 100%.
Tests in lib.test.ts: 14 → 41.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ire validator

- read the server version from package.json at startup so MCP clients
  see the same version that npm publishes; previously the McpServer
  was constructed with a hardcoded "0.2.0" while package.json had
  drifted to 0.6.2.
- extract the tool handler into a named, exported
  handleSequentialThinkingTool so tests can exercise the SDK-facing
  contract (structuredContent shape, isError pass-through) without
  booting the stdio server.
- guard runServer() with an ESM "is main module" check so importing
  index.ts from a test file does not auto-start the stdio loop.
- call the new validateThoughtData up-front so cross-field violations
  return a well-formed isError response instead of silently rendering
  "(revising thought undefined)" or dropping branch data on the floor.

Adds a new __tests__/index.test.ts covering the success path,
isError pass-through, and the two validator failure modes.

index.ts line coverage: 0% → 80%.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant