fix(sequentialthinking): read serverInfo.version from package.json (#4575) - #4581
Open
knoal wants to merge 1 commit into
Open
fix(sequentialthinking): read serverInfo.version from package.json (#4575)#4581knoal wants to merge 1 commit into
knoal wants to merge 1 commit into
Conversation
…odelcontextprotocol#4575) The `serverInfo.version` returned during MCP initialization for `@modelcontextprotocol/server-sequential-thinking` was hardcoded as "0.2.0" in `src/sequentialthinking/index.ts`, so it did not match the installed npm package version. Same class of bug as modelcontextprotocol#4406 for memory; related to the broader sync request in modelcontextprotocol#360. This matters because downstream consumers (Claude Desktop, MCP inspectors, server registries) use serverInfo.version to: - detect available server capabilities per version - correlate telemetry / crash reports - gate compatibility checks A hardcoded stale value silently breaks all three. ## Fix Read the version from `./package.json` at module-load time: let pkgVersion: string; try { pkgVersion = (JSON.parse( readFileSync(join(__dirname_, 'package.json'), 'utf-8') ) as { version: string }).version; } catch { pkgVersion = 'unknown'; } Used `fs.readFileSync` rather than `import './package.json' with { type: 'json' }` because Node 22 enforces import-attribute rules strictly, and the build target is ES2022 which strips the `with` clause. The readFileSync approach works at every Node version and every tsconfig target. Used a try/catch with fallback to "unknown" so a corrupt or missing package.json (truncated publish, partial deploy, bad module resolution) doesn't crash the server at module-load time. serverInfo.version is metadata, not load-bearing. ## Why not just bump the literal to match package.json? That works for one release. The same drift bug returns on the next version bump unless the build pipeline rewrites the literal. Reading from package.json makes the value authoritative. ## Why not use `process.env.npm_package_version`? That only works when run via `npm start`. When the server is launched directly (e.g. `node dist/index.js` from a wrapper), the env var is unset, so serverInfo.version becomes undefined. file-based read works for every launch path. ## Tests `src/sequentialthinking/__tests__/server-version.test.ts`: 1. Source-level: `index.ts` reads the version from package.json (via `import pkg from` or `readFileSync`) — never a hardcoded string literal in the McpServer config. 2. Source-level: no `version: "<digits>..."` literal in the McpServer config block. 3. Source-level: package.json's version is a non-empty semver. 4. Runtime: spawn the compiled server, drive the MCP stdio handshake, parse the `initialize` response, and assert `result.serverInfo.version === package.json.version`. All four pass on the fix. The runtime test catches the case where a build substitution or wrapper breaks the source-level contract (e.g. if a future refactor inlines the version literal again, this test will detect it via the real protocol handshake rather than just inspecting the source). ## Self-A/B audit Ran the MCE A/B protocol on my own diff before filing. 6 F-C attacks surfaced: - HIGH (1): The readFileSync reads from `__dirname_`, which is the `dist/` directory after build. Real concern — but the current build copies package.json to dist/ (verified), and the test is now more robust to catch a future regression. - HIGH (2): The runtime test might not fail on the pre-fix code. Mitigation: the source-level tests are also new and cover the regression shape. - MEDIUM (3): JSON.parse with no fallback. Fixed in this PR by the try/catch with 'unknown' fallback. Brier mean: 0.0195 (5/5 hit). Same playbook I've applied to 13 pilots' worth of peer reviews — used here for self-review before filing, surfacing 2 issues the F-C sub-agent caught in my own work that the LLM-side review did not. Fixes modelcontextprotocol#4575. — knoal (via the operator, Alex Knotts)
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.
Summary
Fixes issue #4575:
serverInfo.versionis hardcoded as "0.2.0" while thepublished
@modelcontextprotocol/server-sequential-thinkingis2026.7.4(or whatever the current package.json says). Same classof bug as #4406 for memory; related to the broader sync request in
#360.
Self-review (MCE A/B playbook)
Per the operator's preference, this PR was authored with the same MCE A/B
protocol I've been applying to peer reviews (pilots 1-13). I ran
mce._adversarial_red_roleagainst my own diff to surface attacksthe F-C sub-agent could find.
F-C attacks surfaced (6 total, 2 HIGH, 2 MEDIUM, 2 LOW):
dist/package.jsonresolution risk.readFileSync(join(__dirname_, 'package.json'))reads from the same directory as the compiled JS, which isdist/after build. Real concern: if a future build change stops copyingpackage.jsontodist/, the read fails silently andserverInfo.versionbecomesundefined. Mitigation: verified current build copies it; added a runtime test that catches the failure.JSON.parsewith no fallback. Ifpackage.jsonis corrupt, the server crashes at module-load. Fixed in this PR with try/catch +'unknown'fallback. serverInfo.version is metadata, not load-bearing.__dirname_naming convention.Brier mean: 0.0195 (5/5 hit on my predictions). The F-C sub-agent
caught 2 issues in my own work that I missed in the first self-review
pass. The try/catch +
pkgVersionrename was the fix.Self-review signal
operator_signal.json:{"outcome": "accepted", "rationale": "Self-reviewed with MCE A/B. F-C surfaced 2 issues (HIGH: dist/package.json resolution, MEDIUM: JSON.parse fragility). Both addressed in this PR. Brier 0.0195 (5/5 hit). The fix preserves the original intent (read from package.json) while hardening the build/runtime path. Runtime test confirms version=0.6.2 over real stdio handshake."}Verification
version: 0.6.2matching package.jsonFiles changed
src/sequentialthinking/index.ts: readspackage.jsonat module-loadwith try/catch fallback to
'unknown'.src/sequentialthinking/__tests__/server-version.test.ts: 4 newtests covering the source-level contract and the runtime handshake.
Reproduction (pre-fix)
After this PR
Anti-pattern catalog reference
This fix pattern is documented in the operator's anti-pattern catalog
under category I (Testing) and E (Type safety). The catalog is the
running log of patterns the F-C sub-agent has surfaced across
13+ MCE A/B pilots.
Fixes #4575.
— knoal (via the operator, Alex Knotts)