fix(sequentialthinking): track branches whose IDs match Object.prototype keys

[BlueX888]

Description

branchId is declared as an opaque string in the tool schema (src/sequentialthinking/index.ts:92 — z.string().optional().describe("Branch identifier"), no pattern/format/refine), but the branch map is a plain object literal, so a schema-valid id that happens to name an Object.prototype member is never treated as an ordinary key.

The lazy-init guard at src/sequentialthinking/lib.ts:63 reads this.branches[input.branchId] through the prototype chain. For branchId values such as constructor, toString, valueOf, hasOwnProperty or __proto__, that lookup returns a truthy inherited value, so the = [] assignment on line 64 is skipped, and line 66 calls .push on something that is not an array. The catch at src/sequentialthinking/lib.ts:86 converts the resulting TypeError into isError: true carrying the raw internal message:

branchId="constructor" : isError=true {"error": "this.branches[input.branchId].push is not a function", "status": "failed"}

Only the spelling of the id changes the outcome; branchId: "alt" creates a branch normally. Because this.thoughtHistory.push(input) at src/sequentialthinking/lib.ts:60 runs before the branch block, the rejected call has also already been appended to the history.

Fix: initialize branches with a null prototype (Object.create(null)) so every string id is an ordinary own key, the same way the other official servers keep per-key state in a Map.

Fixes #4813

Server Details

• Server: sequentialthinking
• Changes to: tools — branch bookkeeping inside SequentialThinkingServer.processThought

Motivation and Context

branchId is documented as an opaque identifier ("Identifier for the current branch (if any)", src/sequentialthinking/index.ts:69) and the tool is registered with annotations: { readOnlyHint: true, idempotentHint: true } (src/sequentialthinking/index.ts:95-98). A well-formed call under the advertised schema should therefore succeed, but a branchId that collides with an Object.prototype key instead returns isError: true with an internal JavaScript error message, and still counts toward thoughtHistoryLength for every later call.

The failure is reachable from any tools/call with branchFromThought >= 1 and a colliding branchId — no unusual setup is needed, and a client whose branchId is influenced by tool/prompt content reaches it without intending to.

Same bug class as the previously fixed modelcontextprotocol/servers#4157 (filesystem edit_file newText hijacked by String.prototype.replace semantics), where JS builtin semantics took over a documented string input.

How Has This Been Tested?

Unit tests only; no LLM client was used.

Red — the added test run against the unpatched tree (branches = {}):

 ❯ __tests__/lib.test.ts (19 tests | 5 failed) 7ms
       × should track a branch whose ID is an Object.prototype key: constructor
       × should track a branch whose ID is an Object.prototype key: toString
       × should track a branch whose ID is an Object.prototype key: valueOf
       × should track a branch whose ID is an Object.prototype key: hasOwnProperty
       × should track a branch whose ID is an Object.prototype key: __proto__
AssertionError: expected true to be undefined
 ❯ __tests__/lib.test.ts:194:30
    194|       expect(result.isError).toBeUndefined();
      Tests  5 failed | 14 passed (19)

Green — after the fix, and the module's full suite:

$ npx vitest run __tests__/lib.test.ts --coverage.enabled=false
      Tests  19 passed (19)

$ npm test
 Test Files  3 passed (3)
      Tests  31 passed (31)

$ npm run build
> tsc && shx chmod +x dist/*.js
(exit 0)

The regression test is a table over constructor, toString, valueOf, hasOwnProperty and __proto__ in the existing processThought - branching block of src/sequentialthinking/__tests__/lib.test.ts, asserting isError is undefined and the id appears in branches.

Breaking Changes

None. The tool's request and response shape is unchanged. branches is still serialized as a JSON array of strings through Object.keys(this.branches) (src/sequentialthinking/lib.ts:81), which behaves identically on a prototype-less object.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation update

Checklist

• My code follows the repository's style guidelines
• New and existing tests pass locally
• I have read the MCP Protocol Documentation
• My changes follows MCP security best practices
• I have updated the server's README accordingly
• I have tested this with an LLM client
• I have added appropriate error handling
• I have documented all environment variables and configuration options

The remaining boxes are left unchecked rather than ticked by default: this is a one-line state-initialization fix with no README, environment-variable, or LLM-client surface, and no new error handling is introduced.

Additional context

src/sequentialthinking/lib.ts:17 — private branches: Record<string, ThoughtData[]> = {}; → Object.create(null). The Object.keys() consumer at lib.ts:81 is the only reader of the map outside the guard, and it is unaffected.

git diff:

-  private branches: Record<string, ThoughtData[]> = {};
+  private branches: Record<string, ThoughtData[]> = Object.create(null);

Note on scope: this change fixes the branch-tracking failure, so the colliding call now succeeds and is recorded exactly once. It does not change the pre-existing behavior that a call ending in isError has already been appended to thoughtHistory (lib.ts:60 runs before the branch block) — that ordering affects error paths generally and is left untouched here to keep the patch minimal.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.