diff --git a/src/filesystem/__tests__/lib.test.ts b/src/filesystem/__tests__/lib.test.ts index 1da741f0b0..eeee46b99d 100644 --- a/src/filesystem/__tests__/lib.test.ts +++ b/src/filesystem/__tests__/lib.test.ts @@ -14,6 +14,7 @@ import { getFileStats, readFileContent, writeFileContent, + moveFile, // Search & filtering functions searchFilesWithValidation, // File editing functions @@ -26,6 +27,23 @@ import { vi.mock('fs/promises'); const mockFs = fs as any; +function createMockFileHandle(content: Buffer) { + return { + read: vi.fn( + async (buffer: Buffer, offset: number, length: number, position: number) => { + const bytesRead = content.copy( + buffer, + offset, + position, + Math.min(position + length, content.length), + ); + return { bytesRead, buffer }; + }, + ), + close: vi.fn().mockResolvedValue(undefined), + }; +} + describe('Lib Functions', () => { beforeEach(() => { vi.clearAllMocks(); @@ -190,19 +208,34 @@ describe('Lib Functions', () => { expect(result).toBe(path.resolve(newFilePath)); }); - it('rejects when parent directory does not exist', async () => { + it('walks past multiple missing ancestors to an existing allowed directory', async () => { + // e.g. create_directory('/home/user/nonexistent/nested/newfile.txt') when + // neither 'nonexistent' nor 'nonexistent/nested' exist yet, but '/home/user' + // (an allowed directory) does. Regression test for #4629. + const newFilePath = process.platform === 'win32' ? 'C:\\Users\\test\\nonexistent\\nested\\newfile.txt' : '/home/user/nonexistent/nested/newfile.txt'; + + const enoentError = new Error('ENOENT') as NodeJS.ErrnoException; + enoentError.code = 'ENOENT'; + + // The path itself is missing. The resolver then realpaths the allowed + // directory ('/home/user'), which falls through to the beforeEach base + // implementation and echoes the path back as-is. readdir is mocked to + // undefined, so every component below it counts as missing. + mockFs.realpath.mockRejectedValueOnce(enoentError); + + const result = await validatePath(newFilePath); + expect(result).toBe(path.resolve(newFilePath)); + }); + + it('rejects when no ancestor directory exists at all', async () => { const newFilePath = process.platform === 'win32' ? 'C:\\Users\\test\\nonexistent\\newfile.txt' : '/home/user/nonexistent/newfile.txt'; - - // Create errors with the ENOENT code - const enoentError1 = new Error('ENOENT') as NodeJS.ErrnoException; - enoentError1.code = 'ENOENT'; - const enoentError2 = new Error('ENOENT') as NodeJS.ErrnoException; - enoentError2.code = 'ENOENT'; - - mockFs.realpath - .mockRejectedValueOnce(enoentError1) - .mockRejectedValueOnce(enoentError2); - + + const enoentError = new Error('ENOENT') as NodeJS.ErrnoException; + enoentError.code = 'ENOENT'; + + // Every ancestor, all the way up to the filesystem root, is missing. + mockFs.realpath.mockRejectedValue(enoentError); + await expect(validatePath(newFilePath)) .rejects.toThrow('Parent directory does not exist'); }); @@ -339,6 +372,29 @@ describe('Lib Functions', () => { }); }); + describe('moveFile', () => { + it('moves the file when the destination does not exist', async () => { + const enoent = Object.assign(new Error('not found'), { code: 'ENOENT' }); + mockFs.lstat.mockRejectedValueOnce(enoent); + mockFs.rename.mockResolvedValueOnce(undefined); + + await moveFile('/test/source.txt', '/test/dest.txt'); + + expect(mockFs.rename).toHaveBeenCalledWith('/test/source.txt', '/test/dest.txt'); + }); + + it('fails without overwriting when the destination already exists', async () => { + // lstat resolving means the destination is occupied. + mockFs.lstat.mockResolvedValueOnce({} as any); + + await expect(moveFile('/test/source.txt', '/test/dest.txt')).rejects.toThrow( + 'Destination already exists' + ); + + expect(mockFs.rename).not.toHaveBeenCalled(); + }); + }); + }); describe('Search & Filtering Functions', () => { @@ -698,6 +754,23 @@ describe('Lib Functions', () => { expect(mockFileHandle.close).toHaveBeenCalled(); }); + it('preserves UTF-8 characters split across chunk boundaries', async () => { + const content = Buffer.concat([ + Buffer.from('discard\n'), + Buffer.from('界'), + Buffer.alloc(1017, 'a'), + Buffer.from('\nlast'), + ]); + const mockFileHandle = createMockFileHandle(content); + + mockFs.stat.mockResolvedValue({ size: content.length } as any); + mockFs.open.mockResolvedValue(mockFileHandle); + + const result = await tailFile('/test/file.txt', 2); + + expect(result).toBe(`界${'a'.repeat(1017)}\nlast`); + }); + it('handles read errors gracefully', async () => { mockFs.stat.mockResolvedValue({ size: 100 } as any); @@ -754,6 +827,20 @@ describe('Lib Functions', () => { expect(mockFileHandle.close).toHaveBeenCalled(); }); + it('preserves UTF-8 characters split across chunk boundaries', async () => { + const content = Buffer.concat([ + Buffer.alloc(1023, 'a'), + Buffer.from('界\nsecond'), + ]); + const mockFileHandle = createMockFileHandle(content); + + mockFs.open.mockResolvedValue(mockFileHandle); + + const result = await headFile('/test/file.txt', 1); + + expect(result).toBe(`${'a'.repeat(1023)}界`); + }); + it('handles files with leftover content', async () => { const mockFileHandle = { read: vi.fn(), diff --git a/src/filesystem/__tests__/nested-parents.test.ts b/src/filesystem/__tests__/nested-parents.test.ts new file mode 100644 index 0000000000..854dc4405f --- /dev/null +++ b/src/filesystem/__tests__/nested-parents.test.ts @@ -0,0 +1,35 @@ +import fs from 'fs/promises'; +import os from 'os'; +import path from 'path'; +import { afterEach, beforeEach, describe, expect, it } from 'vitest'; +import { setAllowedDirectories, validatePath } from '../lib.js'; + +// Regression coverage for #4629: validatePath must accept a path whose +// ancestors are missing several levels deep, so create_directory can mkdir -p. +describe('validatePath with multiple missing ancestors', () => { + let allowedDir: string; + let outsideDir: string; + + beforeEach(async () => { + allowedDir = await fs.realpath(await fs.mkdtemp(path.join(os.tmpdir(), 'mcp-nested-allowed-'))); + outsideDir = await fs.realpath(await fs.mkdtemp(path.join(os.tmpdir(), 'mcp-nested-outside-'))); + setAllowedDirectories([allowedDir]); + }); + + afterEach(async () => { + setAllowedDirectories([]); + await fs.rm(allowedDir, { recursive: true, force: true }); + await fs.rm(outsideDir, { recursive: true, force: true }); + }); + + it('returns the full path when several ancestors do not exist', async () => { + const requested = path.join(allowedDir, 'a', 'b', 'c'); + await expect(validatePath(requested)).resolves.toBe(requested); + }); + + it('rejects when the nearest existing ancestor is a symlink out of the allowed tree', async () => { + await fs.symlink(outsideDir, path.join(allowedDir, 'link'), 'junction'); + await expect(validatePath(path.join(allowedDir, 'link', 'a', 'b'))) + .rejects.toThrow('Access denied'); + }); +}); diff --git a/src/filesystem/__tests__/unicode-paths.test.ts b/src/filesystem/__tests__/unicode-paths.test.ts new file mode 100644 index 0000000000..2d28ac21f9 --- /dev/null +++ b/src/filesystem/__tests__/unicode-paths.test.ts @@ -0,0 +1,49 @@ +import fs from 'fs/promises'; +import os from 'os'; +import path from 'path'; +import { afterEach, beforeEach, describe, expect, it } from 'vitest'; +import { setAllowedDirectories, validatePath } from '../lib.js'; + +describe('Unicode-equivalent filesystem paths', () => { + let testDirectory: string; + + beforeEach(async () => { + testDirectory = await fs.mkdtemp(path.join(os.tmpdir(), 'mcp-unicode-paths-')); + setAllowedDirectories([testDirectory]); + }); + + afterEach(async () => { + setAllowedDirectories([]); + await fs.rm(testDirectory, { recursive: true, force: true }); + }); + + it('resolves an existing decomposed path from a composed request', async () => { + const onDiskDirectory = 'de\u0301marche'; + const onDiskFile = 're\u0301sume\u0301.txt'; + await fs.mkdir(path.join(testDirectory, onDiskDirectory)); + await fs.writeFile(path.join(testDirectory, onDiskDirectory, onDiskFile), 'content'); + + const resolved = await validatePath(path.join(testDirectory, 'd\u00e9marche', 'r\u00e9sum\u00e9.txt')); + + expect(resolved).toBe(await fs.realpath(path.join(testDirectory, onDiskDirectory, onDiskFile))); + }); + + it('preserves a new basename after resolving a Unicode-equivalent parent', async () => { + const onDiskDirectory = 'de\u0301marche'; + await fs.mkdir(path.join(testDirectory, onDiskDirectory)); + + const resolved = await validatePath(path.join(testDirectory, 'd\u00e9marche', 'new.txt')); + + expect(resolved).toBe(path.join(await fs.realpath(path.join(testDirectory, onDiskDirectory)), 'new.txt')); + }); + + it('rejects ambiguous canonically equivalent entries', async () => { + const composed = 'caf\u00e9'; + const decomposed = 'cafe\u0301'; + await fs.mkdir(path.join(testDirectory, composed)); + await fs.mkdir(path.join(testDirectory, decomposed)); + + await expect(validatePath(path.join(testDirectory, 'cafe\u0341', 'file.txt'))) + .rejects.toThrow('Ambiguous Unicode path component'); + }); +}); diff --git a/src/filesystem/index.ts b/src/filesystem/index.ts index 234605bb13..51ac523a66 100644 --- a/src/filesystem/index.ts +++ b/src/filesystem/index.ts @@ -21,6 +21,7 @@ import { getFileStats, readFileContent, writeFileContent, + moveFile, searchFilesWithValidation, applyFileEdits, tailFile, @@ -631,7 +632,7 @@ server.registerTool( async (args: z.infer) => { const validSourcePath = await validatePath(args.source); const validDestPath = await validatePath(args.destination); - await fs.rename(validSourcePath, validDestPath); + await moveFile(validSourcePath, validDestPath); const text = `Successfully moved ${args.source} to ${args.destination}`; const contentBlock = { type: "text" as const, text }; return { diff --git a/src/filesystem/lib.ts b/src/filesystem/lib.ts index a1c6f04b67..f2371a9a01 100644 --- a/src/filesystem/lib.ts +++ b/src/filesystem/lib.ts @@ -2,6 +2,7 @@ import fs from "fs/promises"; import path from "path"; import os from 'os'; import { randomBytes } from 'crypto'; +import { StringDecoder } from 'string_decoder'; import { diffLines, createTwoFilesPatch } from 'diff'; import { minimatch } from 'minimatch'; import { normalizePath, expandHome } from './path-utils.js'; @@ -96,6 +97,46 @@ function resolveRelativePathAgainstAllowedDirectories(relativePath: string): str } // Security & Validation Functions +async function resolveUnicodeEquivalentPath(absolutePath: string): Promise { + const allowedDirectory = [...allowedDirectories] + .sort((left, right) => right.length - left.length) + .find(directory => isPathWithinAllowedDirectories(normalizePath(absolutePath), [directory])); + + if (!allowedDirectory) { + return absolutePath; + } + + let currentPath = await fs.realpath(allowedDirectory); + const relativeParts = path.relative(allowedDirectory, absolutePath).split(path.sep).filter(Boolean); + + for (let index = 0; index < relativeParts.length; index++) { + const requestedPart = relativeParts[index]; + const entries = (await fs.readdir(currentPath)) ?? []; + const exactMatch = entries.find(entry => entry === requestedPart); + const equivalentMatches = exactMatch + ? [exactMatch] + : entries.filter(entry => entry.normalize('NFC') === requestedPart.normalize('NFC')); + + if (equivalentMatches.length > 1) { + throw new Error(`Ambiguous Unicode path component: ${requestedPart}`); + } + + if (equivalentMatches.length === 0) { + // Nothing below this point exists yet, so there are no symlinks left to + // resolve. currentPath is already realpath'd and inside an allowed + // directory; append the missing tail so create_directory can mkdir -p it. + return path.join(currentPath, ...relativeParts.slice(index)); + } + + currentPath = await fs.realpath(path.join(currentPath, equivalentMatches[0])); + if (!isPathWithinAllowedDirectories(normalizePath(currentPath), allowedDirectories)) { + throw new Error(`Access denied - symlink target outside allowed directories: ${currentPath} not in ${allowedDirectories.join(', ')}`); + } + } + + return currentPath; +} + export async function validatePath(requestedPath: string): Promise { const expandedPath = expandHome(requestedPath); const absolute = path.isAbsolute(expandedPath) @@ -123,16 +164,13 @@ export async function validatePath(requestedPath: string): Promise { // Security: For new files that don't exist yet, verify parent directory // This ensures we can't create files in unauthorized locations if ((error as NodeJS.ErrnoException).code === 'ENOENT') { - const parentDir = path.dirname(absolute); try { - const realParentPath = await fs.realpath(parentDir); - const normalizedParent = normalizePath(realParentPath); - if (!isPathWithinAllowedDirectories(normalizedParent, allowedDirectories)) { - throw new Error(`Access denied - parent directory outside allowed directories: ${realParentPath} not in ${allowedDirectories.join(', ')}`); + return await resolveUnicodeEquivalentPath(absolute); + } catch (resolutionError) { + if ((resolutionError as NodeJS.ErrnoException).code === 'ENOENT') { + throw new Error(`Parent directory does not exist: ${path.dirname(absolute)}`); } - return absolute; - } catch { - throw new Error(`Parent directory does not exist: ${parentDir}`); + throw resolutionError; } } throw error; @@ -193,6 +231,25 @@ export async function writeFileContent(filePath: string, content: string): Promi } +export async function moveFile(sourcePath: string, destinationPath: string): Promise { + // The move_file tool contract (and README) state the operation fails if the + // destination already exists. fs.rename would silently overwrite it, which is + // a data-loss bug, so reject up front when anything - file, directory, or + // symlink - occupies the target. lstat is used so an existing symlink at the + // destination is detected rather than followed. + try { + await fs.lstat(destinationPath); + } catch (error) { + if ((error as NodeJS.ErrnoException).code === 'ENOENT') { + await fs.rename(sourcePath, destinationPath); + return; + } + throw error; + } + throw new Error(`Destination already exists: ${destinationPath}`); +} + + // File Editing Functions interface FileEdit { oldText: string; @@ -308,42 +365,28 @@ export async function tailFile(filePath: string, numLines: number): Promise 0 && linesFound < numLines) { + while (position > 0 && newlinesFound < numLines) { const size = Math.min(CHUNK_SIZE, position); position -= size; const { bytesRead } = await fileHandle.read(chunk, 0, size, position); if (!bytesRead) break; - - // Get the chunk as a string and prepend any remaining text from previous iteration - const readData = chunk.slice(0, bytesRead).toString('utf-8'); - const chunkText = readData + remainingText; - - // Split by newlines and count - const chunkLines = normalizeLineEndings(chunkText).split('\n'); - - // If this isn't the end of the file, the first line is likely incomplete - // Save it to prepend to the next chunk - if (position > 0) { - remainingText = chunkLines[0]; - chunkLines.shift(); // Remove the first (incomplete) line - } - - // Add lines to our result (up to the number we need) - for (let i = chunkLines.length - 1; i >= 0 && linesFound < numLines; i--) { - lines.unshift(chunkLines[i]); - linesFound++; + + const readData = Buffer.from(chunk.subarray(0, bytesRead)); + chunks.unshift(readData); + for (const byte of readData) { + if (byte === 0x0a) newlinesFound++; } } - - return lines.join('\n'); + + const text = normalizeLineEndings(Buffer.concat(chunks).toString('utf-8')); + return text.split('\n').slice(-numLines).join('\n'); } finally { await fileHandle.close(); } @@ -357,13 +400,14 @@ export async function headFile(filePath: string, numLines: number): Promise 0 && lines.length < numLines) { diff --git a/src/memory/__tests__/atomic-save.test.ts b/src/memory/__tests__/atomic-save.test.ts new file mode 100644 index 0000000000..8c702b5f7e --- /dev/null +++ b/src/memory/__tests__/atomic-save.test.ts @@ -0,0 +1,133 @@ +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { promises as fs } from 'fs'; +import path from 'path'; +import os from 'os'; +import { KnowledgeGraphManager, Entity } from '../index.js'; + +/** + * Regression tests for durable persistence of the knowledge graph. + * + * saveGraph() previously called fs.writeFile() directly on the memory file. + * fs.writeFile opens the target with 'w', which truncates it before any new + * bytes are written. If the process dies between truncation and completion + * (SIGKILL, container stop, OOM kill, power loss) the memory file — the sole + * persistence layer for the graph — is left empty or half-written, and the + * accumulated memory is unrecoverable. + * + * The fix writes to a temporary file in the same directory and then renames + * it over the target. rename(2) is atomic on POSIX filesystems: a reader + * either sees the complete old file or the complete new one, never a + * truncated intermediate state. + */ +describe('KnowledgeGraphManager persistence durability', () => { + let testDir: string; + let testFilePath: string; + + beforeEach(async () => { + testDir = await fs.mkdtemp(path.join(os.tmpdir(), 'mcp-memory-atomic-')); + testFilePath = path.join(testDir, 'memory.jsonl'); + }); + + afterEach(async () => { + vi.restoreAllMocks(); + await fs.rm(testDir, { recursive: true, force: true }); + }); + + it('never truncates the live memory file in place', async () => { + const manager = new KnowledgeGraphManager(testFilePath); + await manager.createEntities([ + { name: 'Alice', entityType: 'person', observations: ['works at Acme Corp'] }, + ]); + + // Any write that targets the live file directly is destructive: it + // truncates committed data before the replacement is durable. + const writeFileSpy = vi.spyOn(fs, 'writeFile'); + const openSpy = vi.spyOn(fs, 'open'); + + await manager.createEntities([ + { name: 'Bob', entityType: 'person', observations: ['likes programming'] }, + ]); + + const destructiveTargets = [ + ...writeFileSpy.mock.calls.map(call => call[0]), + ...openSpy.mock.calls.map(call => call[0]), + ].filter(target => target === testFilePath); + + expect(destructiveTargets).toEqual([]); + }); + + it('leaves the committed graph intact when the write fails midway', async () => { + const manager = new KnowledgeGraphManager(testFilePath); + await manager.createEntities([ + { name: 'Alice', entityType: 'person', observations: ['works at Acme Corp'] }, + ]); + + const before = await fs.readFile(testFilePath, 'utf-8'); + expect(before).toContain('Alice'); + + // Simulate the process being interrupted while persisting the next write. + vi.spyOn(fs, 'writeFile').mockImplementationOnce(async () => { + throw new Error('ENOSPC: simulated interruption'); + }); + + await expect( + manager.createEntities([ + { name: 'Bob', entityType: 'person', observations: ['likes programming'] }, + ]) + ).rejects.toThrow(); + + vi.restoreAllMocks(); + + // The previously committed graph must survive untouched. + expect(await fs.readFile(testFilePath, 'utf-8')).toBe(before); + + const graph = await new KnowledgeGraphManager(testFilePath).readGraph(); + expect(graph.entities.map(e => e.name)).toEqual(['Alice']); + }); + + it('does not leave temporary files behind after a successful write', async () => { + const manager = new KnowledgeGraphManager(testFilePath); + await manager.createEntities([ + { name: 'Alice', entityType: 'person', observations: ['works at Acme Corp'] }, + ]); + + expect(await fs.readdir(testDir)).toEqual(['memory.jsonl']); + }); + + it('cleans up the temporary file when the write fails', async () => { + const manager = new KnowledgeGraphManager(testFilePath); + await manager.createEntities([ + { name: 'Alice', entityType: 'person', observations: ['works at Acme Corp'] }, + ]); + + vi.spyOn(fs, 'rename').mockImplementationOnce(async () => { + throw new Error('EXDEV: simulated rename failure'); + }); + + await expect( + manager.createEntities([ + { name: 'Bob', entityType: 'person', observations: ['likes programming'] }, + ]) + ).rejects.toThrow(); + + vi.restoreAllMocks(); + expect(await fs.readdir(testDir)).toEqual(['memory.jsonl']); + }); + + it('still persists graph contents correctly across reloads', async () => { + const manager = new KnowledgeGraphManager(testFilePath); + await manager.createEntities([ + { name: 'Alice', entityType: 'person', observations: ['works at Acme Corp'] }, + { name: 'Bob', entityType: 'person', observations: ['likes programming'] }, + ]); + await manager.createRelations([ + { from: 'Alice', to: 'Bob', relationType: 'knows' }, + ]); + + const reloaded = await new KnowledgeGraphManager(testFilePath).readGraph(); + expect(reloaded.entities.map(e => e.name).sort()).toEqual(['Alice', 'Bob']); + expect(reloaded.relations).toEqual([ + { from: 'Alice', to: 'Bob', relationType: 'knows' }, + ]); + }); +}); diff --git a/src/memory/index.ts b/src/memory/index.ts index 9865c5318e..b8704a9515 100644 --- a/src/memory/index.ts +++ b/src/memory/index.ts @@ -6,6 +6,7 @@ import { SubscribeRequestSchema, UnsubscribeRequestSchema } from "@modelcontextp import { z } from "zod"; import { promises as fs } from 'fs'; import path from 'path'; +import { randomBytes } from 'crypto'; import { fileURLToPath } from 'url'; // Define memory file path using environment variable with fallback @@ -114,7 +115,29 @@ export class KnowledgeGraphManager { relationType: r.relationType })), ]; - await fs.writeFile(this.memoryFilePath, lines.join("\n")); + + // Write to a temporary file in the same directory, then rename it over + // the target. fs.writeFile would truncate the memory file before writing, + // so an interruption (SIGKILL, container stop, OOM, power loss) would + // leave the only copy of the graph truncated and unrecoverable. + // rename(2) is atomic on POSIX filesystems: readers see either the + // complete old file or the complete new one, never a partial state. + // The temp file is kept in the same directory so the rename stays on one + // filesystem — renaming across mount points fails with EXDEV. + const directory = path.dirname(this.memoryFilePath); + const tempFilePath = path.join( + directory, + `${path.basename(this.memoryFilePath)}.${randomBytes(16).toString('hex')}.tmp` + ); + + try { + await fs.writeFile(tempFilePath, lines.join("\n")); + await fs.rename(tempFilePath, this.memoryFilePath); + } catch (error) { + // Never leave a stray temp file behind on failure. + await fs.unlink(tempFilePath).catch(() => {}); + throw error; + } } async createEntities(entities: Entity[]): Promise { diff --git a/src/sequentialthinking/__tests__/input-schema.test.ts b/src/sequentialthinking/__tests__/input-schema.test.ts new file mode 100644 index 0000000000..4ff7be663c --- /dev/null +++ b/src/sequentialthinking/__tests__/input-schema.test.ts @@ -0,0 +1,66 @@ +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { existsSync } from 'node:fs'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { Client } from '@modelcontextprotocol/sdk/client/index.js'; +import { StdioClientTransport } from '@modelcontextprotocol/sdk/client/stdio.js'; + +const packageRoot = path.join(path.dirname(fileURLToPath(import.meta.url)), '..'); +const distIndexPath = path.join(packageRoot, 'dist', 'index.js'); + +// Regression coverage for #4651: nextThoughtNeeded must stay in the advertised +// `required` array, and string coercion must keep accepting "True"/"FALSE" +// while rejecting anything else. Runs against the built server so it checks +// the schema the SDK actually emits, not the zod object. +describe.skipIf(!existsSync(distIndexPath))('sequentialthinking input schema', () => { + let client: Client; + + beforeAll(async () => { + const transport = new StdioClientTransport({ + command: process.execPath, + args: [distIndexPath], + cwd: packageRoot, + stderr: 'pipe', + }); + client = new Client({ name: 'input-schema-test', version: '0.0.0' }); + await client.connect(transport); + }); + + afterAll(async () => { + await client?.close(); + }); + + it('advertises nextThoughtNeeded as required', async () => { + const { tools } = await client.listTools(); + const tool = tools.find(t => t.name === 'sequentialthinking'); + expect(tool).toBeDefined(); + expect(tool!.inputSchema.required).toEqual( + expect.arrayContaining(['thought', 'nextThoughtNeeded', 'thoughtNumber', 'totalThoughts']) + ); + }); + + it('rejects a call that omits nextThoughtNeeded', async () => { + const result = await client.callTool({ + name: 'sequentialthinking', + arguments: { thought: 't', thoughtNumber: 1, totalThoughts: 1 }, + }); + expect(result.isError).toBe(true); + expect(JSON.stringify(result.content)).toMatch(/nextThoughtNeeded/); + }); + + it.each(['True', 'FALSE', 'true', 'false'])('accepts the string %s', async (value) => { + const result = await client.callTool({ + name: 'sequentialthinking', + arguments: { thought: 't', nextThoughtNeeded: value, thoughtNumber: 1, totalThoughts: 1 }, + }); + expect(result.isError).toBeFalsy(); + }); + + it.each(['yes', '', '1'])('rejects the string %j', async (value) => { + const result = await client.callTool({ + name: 'sequentialthinking', + arguments: { thought: 't', nextThoughtNeeded: value, thoughtNumber: 1, totalThoughts: 1 }, + }); + expect(result.isError).toBe(true); + }); +}); diff --git a/src/sequentialthinking/__tests__/server-version.test.ts b/src/sequentialthinking/__tests__/server-version.test.ts new file mode 100644 index 0000000000..c7d7f5cb39 --- /dev/null +++ b/src/sequentialthinking/__tests__/server-version.test.ts @@ -0,0 +1,57 @@ +import { describe, it, expect } from 'vitest'; +import { createRequire } from 'node:module'; +import { existsSync } from 'node:fs'; +import path from 'node:path'; +import { fileURLToPath, pathToFileURL } from 'node:url'; +import { Client } from '@modelcontextprotocol/sdk/client/index.js'; +import { StdioClientTransport } from '@modelcontextprotocol/sdk/client/stdio.js'; +import { resolvePackageVersion, SERVER_VERSION } from '../version.js'; + +const packageJson = createRequire(import.meta.url)('../package.json') as { version: string }; +const packageRoot = path.join(path.dirname(fileURLToPath(import.meta.url)), '..'); +const distVersionPath = path.join(packageRoot, 'dist', 'version.js'); +const distIndexPath = path.join(packageRoot, 'dist', 'index.js'); + +describe('server version', () => { + it('uses package.json version instead of a hardcoded string', () => { + expect(SERVER_VERSION).toBe(packageJson.version); + expect(resolvePackageVersion()).toBe(packageJson.version); + expect(SERVER_VERSION).not.toBe('0.2.0'); + }); + + // CI runs `npm test` before the dedicated build job. `npm ci` usually + // materializes dist/ via prepare, but that is not guaranteed (e.g. local + // `rm -rf dist && npm test`, or install with --ignore-scripts). + it.skipIf(!existsSync(distVersionPath))( + 'resolves package.json from the dist layout after build', + async () => { + const distModule = (await import(pathToFileURL(distVersionPath).href)) as { + SERVER_VERSION: string; + }; + expect(distModule.SERVER_VERSION).toBe(packageJson.version); + }, + ); + + it.skipIf(!existsSync(distIndexPath))( + 'stdio initialize reports package.json version in serverInfo', + async () => { + const transport = new StdioClientTransport({ + command: process.execPath, + args: [distIndexPath], + cwd: packageRoot, + stderr: 'pipe', + }); + const client = new Client({ name: 'version-smoke', version: '0.0.0' }); + + try { + await client.connect(transport); + const serverInfo = client.getServerVersion(); + expect(serverInfo?.name).toBe('sequential-thinking-server'); + expect(serverInfo?.version).toBe(packageJson.version); + expect(serverInfo?.version).not.toBe('0.2.0'); + } finally { + await client.close(); + } + }, + ); +}); diff --git a/src/sequentialthinking/index.ts b/src/sequentialthinking/index.ts index 217845bb3d..1ae09d1db8 100644 --- a/src/sequentialthinking/index.ts +++ b/src/sequentialthinking/index.ts @@ -4,20 +4,21 @@ import { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; import { StdioServerTransport } from "@modelcontextprotocol/sdk/server/stdio.js"; import { z } from "zod"; import { SequentialThinkingServer } from './lib.js'; +import { SERVER_VERSION } from './version.js'; -/** Safe boolean coercion that correctly handles string "false" */ -const coercedBoolean = z.preprocess((val) => { +/** Safe boolean coercion that correctly handles string "false". A union+transform, + * not z.preprocess (whose input type is `unknown`), so toJSONSchema keeps this required. */ +const coercedBoolean = z.union([z.boolean(), z.string()]).transform((val, ctx) => { if (typeof val === "boolean") return val; - if (typeof val === "string") { - if (val.toLowerCase() === "true") return true; - if (val.toLowerCase() === "false") return false; - } - return val; -}, z.boolean()); + if (val.toLowerCase() === "true") return true; + if (val.toLowerCase() === "false") return false; + ctx.addIssue({ code: "custom", message: `Expected boolean or "true"/"false" string, received "${val}"` }); + return z.NEVER; +}); const server = new McpServer({ name: "sequential-thinking-server", - version: "0.2.0", + version: SERVER_VERSION, }); const thinkingServer = new SequentialThinkingServer(); diff --git a/src/sequentialthinking/version.ts b/src/sequentialthinking/version.ts new file mode 100644 index 0000000000..1ab7d8f2fd --- /dev/null +++ b/src/sequentialthinking/version.ts @@ -0,0 +1,33 @@ +import { createRequire } from 'node:module'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; + +/** + * Resolve this package's version from package.json. + * + * Works both from source (`src/sequentialthinking/`) and from the published + * layout (`dist/`), where package.json lives one directory up. + */ +export function resolvePackageVersion(): string { + const require = createRequire(import.meta.url); + const moduleDir = path.dirname(fileURLToPath(import.meta.url)); + const candidates = [ + path.join(moduleDir, 'package.json'), + path.join(moduleDir, '..', 'package.json'), + ]; + + for (const candidate of candidates) { + try { + const pkg = require(candidate) as { version?: string }; + if (pkg.version) { + return pkg.version; + } + } catch { + // Try the next candidate when running from dist/ or source. + } + } + + throw new Error('Could not locate package.json for server version'); +} + +export const SERVER_VERSION = resolvePackageVersion();