sequentialthinking: a branchId that collides with an Object.prototype key throws instead of creating the branch, and the failed call still extends thoughtHistory

[BlueX888]

sequentialthinking: a branchId that collides with an Object.prototype key throws instead of creating the branch, and the failed call still extends thoughtHistory

Environment

• repo: modelcontextprotocol/servers @ d73f99efbfd40c3aa1b61e88728b3d49fb52608f
• server: src/sequentialthinking (@modelcontextprotocol/server-sequential-thinking 0.6.2)
• Node.js v22.23.2 (ESM), npm 10.9.8, @modelcontextprotocol/sdk ^1.30.0, server built with npm ci && npm run build
• OS: macOS 26.6.2 (Darwin 25.6.0), arm64
• No network and no API keys needed: the repro connects a real SDK Client to the built server over stdio and calls the sequentialthinking tool.

Minimal reproduction

/tmp/st-probe/repro-prototype-branchid.mjs — drive the built server over stdio and call sequentialthinking three times: once with an ordinary branch id, once with the same schema-valid call using branchId: "constructor", then once more with no branch, to show what the failed call did to thoughtHistoryLength.

const SDK = '<repo>/node_modules/@modelcontextprotocol/sdk/dist/esm';
const { Client } = await import(SDK + '/client/index.js');
const { StdioClientTransport } = await import(SDK + '/client/stdio.js');

const dist = '<repo>/src/sequentialthinking/dist/index.js';
const cwd  = '<repo>/src/sequentialthinking';
const transport = new StdioClientTransport({ command: process.execPath, args: [dist], cwd, stderr: 'ignore' });
const client = new Client({ name: 'repro', version: '0.0.0' });
await client.connect(transport);

const call = async (label, args) => {
  const r = await client.callTool({ name: 'sequentialthinking', arguments: args });
  console.log(`${label}: isError=${r.isError} ${JSON.stringify(r.content[0].text)}`);
};

// 1. ordinary branch id
await call('baseline branchId="alt"      ',
  { thought: 't1', nextThoughtNeeded: true,  thoughtNumber: 1, totalThoughts: 3, branchFromThought: 1, branchId: 'alt' });

// 2. same schema-valid call, branchId that collides with Object.prototype
await call('branchId="constructor"       ',
  { thought: 't2', nextThoughtNeeded: true,  thoughtNumber: 2, totalThoughts: 3, branchFromThought: 1, branchId: 'constructor' });

// 3. the failed call still mutated thoughtHistory
await call('after the failed call (1 more)',
  { thought: 't3', nextThoughtNeeded: false, thoughtNumber: 3, totalThoughts: 3 });

await client.close();
process.exit(0);

Actual output

Verbatim from node /tmp/st-probe/repro-prototype-branchid.mjs (verifier 1, rerun here — identical):

baseline branchId="alt"      : isError=undefined "{\n  \"thoughtNumber\": 1,\n  \"totalThoughts\": 3,\n  \"nextThoughtNeeded\": true,\n  \"branches\": [\n    \"alt\"\n  ],\n  \"thoughtHistoryLength\": 1\n}"
branchId="constructor"       : isError=true "{\n  \"error\": \"this.branches[input.branchId].push is not a function\",\n  \"status\": \"failed\"\n}"
after the failed call (1 more): isError=undefined "{\n  \"thoughtNumber\": 3,\n  \"totalThoughts\": 3,\n  \"nextThoughtNeeded\": false,\n  \"branches\": [\n    \"alt\"\n  ],\n  \"thoughtHistoryLength\": 3\n}"

The advertised schema, and an id sweep over the same code path (verifier 2's run; whitespace inside the payloads collapsed for readability, the raw run above is unchanged):

SCHEMA.branchId = {"description":"Branch identifier","type":"string"}  required=["thought","nextThoughtNeeded","thoughtNumber","totalThoughts"]

branchId="alt"             -> isError=undefined { "thoughtNumber": 1, "totalThoughts": 8, "nextThoughtNeeded": true,  "branches": [ "alt" ], "thoughtHistoryLength": 1 }
branchId="constructor"     -> isError=true { "error": "this.branches[input.branchId].push is not a function", "status": "failed" }
branchId="__proto__"       -> isError=true { "error": "this.branches[input.branchId].push is not a function", "status": "failed" }
branchId="hasOwnProperty"  -> isError=true { "error": "this.branches[input.branchId].push is not a function", "status": "failed" }
branchId="toString"        -> isError=true { "error": "this.branches[input.branchId].push is not a function", "status": "failed" }
branchId="valueOf"         -> isError=true { "error": "this.branches[input.branchId].push is not a function", "status": "failed" }
after failures             -> isError=undefined { "thoughtNumber": 7, "totalThoughts": 8, "nextThoughtNeeded": false, "branches": [ "alt" ], "thoughtHistoryLength": 7 }

Two things are visible in that output:

1. Only the spelling of the id changes the outcome. "alt" creates a branch; "constructor" / "__proto__" / "hasOwnProperty" / "toString" / "valueOf" return isError: true with the raw internal message.
2. The failed calls are still counted. "branches" never gains the colliding id, yet thoughtHistoryLength is 7 after 1 successful + 5 failed + 1 successful call. In the three-call run above, one success + one failure + one success reports thoughtHistoryLength: 3.

Expected behaviour and basis

Expected: a schema-valid branchId is an opaque identifier. branchId: "constructor" should create and track a branch exactly like branchId: "alt", and a call that ends in isError should not have already mutated the server's reported thoughtHistoryLength.

Basis in this repo:

• src/sequentialthinking/index.ts:92 — branchId: z.string().optional().describe("Branch identifier"). Dumped live over tools/list, the advertised schema is exactly {"description":"Branch identifier","type":"string"} with no pattern, format, minLength, or refine, so "constructor" is a documented, schema-valid value.
• src/sequentialthinking/index.ts:69 — the tool description says branchId: Identifier for the current branch (if any); nothing reserves or restricts prototype key names.
• src/sequentialthinking/lib.ts:17 — private branches: Record<string, ThoughtData[]> = {}; is typed as a map from any string to an array of thoughts, and the guard at :63 whose sibling branch at :64 exists precisely to lazily materialize the array for an unseen id. Any string key is meant to work; the neighbouring Object.keys(this.branches) at :81 is the only consumer and keeps working under a prototype-less map.
• The tool is registered with annotations: { readOnlyHint: true, idempotentHint: true } (src/sequentialthinking/index.ts:95-98), so a call that is well-formed under the advertised schema should succeed, not fail with an internal TypeError.
• Repo precedent that this bug class is real and gets fixed here: modelcontextprotocol/servers#4157 (closed) — "filesystem: edit_file newText interpreted as String.prototype.replace replacement-pattern" — the same shape, where JS builtin semantics hijack a documented string input. The other official servers avoid this class by structuring state per key rather than by inherited-property lookup (cf. the Maps used by src/memory).

Reachability: any tools/call to sequentialthinking with branchFromThought >= 1 and branchId set to an Object.prototype key name. A fuzzing client reaches it directly, and an agent whose branchId is influenced by prompt-injected content reaches it without any unusual setup.

Root cause

src/sequentialthinking/lib.ts:63 — if (!this.branches[input.branchId]). this.branches is a plain object literal, so the lookup walks the prototype chain: for every Object.prototype property name the guard reads a truthy inherited value, never assigns [], and line 66 then calls .push on something that is not an array. The catch at :86 converts that into isError: true carrying the internal message. Because this.thoughtHistory.push(input) at line 60 runs before the branch block, the failed call has already been appended, so thoughtHistoryLength is inflated for every later call.

Proposed fix

Use a prototype-less map or an own-property test — e.g. private branches: Record<string, ThoughtData[]> = Object.create(null);, or if (!Object.prototype.hasOwnProperty.call(this.branches, input.branchId)) — and consider not recording the thought in thoughtHistory when the call ends in isError. A Map<string, ThoughtData[]> fixes it as well, and Object.keys(this.branches) in the response keeps working either way. A regression test that branchId: "constructor" is tracked like any other id would cover it. Happy to open a PR with this approach if it's welcome.

Related issues / PRs

• Collision checks (open issues and PRs referencing branchId, branches, sequentialthinking, and Object.prototype / prototype keys) found no issue or PR covering this: the only sequentialthinking PRs open are four annotation fixes for #4721 (#4784, #4749, #4747, #4722), which touch different lines.
• modelcontextprotocol/servers#4157 (closed) — "filesystem: edit_file newText interpreted as String.prototype.replace replacement-pattern (literal $ corrupted)": same bug class (JS builtin semantics hijacking a documented string input) in a sibling server.
• modelcontextprotocol/servers#3537 (open) — "Security Audit: Unconstrained string parameters across all official servers": relevant to the unconstrained branchId schema that makes the colliding value reachable.

[BlueX888]

Reproduced this. Root cause: src/sequentialthinking/lib.ts:63 — if (!this.branches[input.branchId]) reads a truthy inherited Object.prototype member instead of an own array, so line 66 calls .push on a non-array, while line 60 has already appended the thought to thoughtHistory.

Minimal reproduction:

node /tmp/st-probe/repro-prototype-branchid.mjs

Observed output:

baseline branchId="alt"       : isError=undefined "{\"thoughtNumber\": 1, \"totalThoughts\": 3, \"nextThoughtNeeded\": true, \"branches\": [\"alt\"], \"thoughtHistoryLength\": 1}"
branchId="constructor"        : isError=true "{\"error\": \"this.branches[input.branchId].push is not a function\", \"status\": \"failed\"}"
after the failed call (1 more): isError=undefined "{\"thoughtNumber\": 3, \"totalThoughts\": 3, \"nextThoughtNeeded\": false, \"branches\": [\"alt\"], \"thoughtHistoryLength\": 3}"

Happy to open a PR with this fix, or to be assigned if you prefer to take it.