From ba167cef5bcc1244f95b4fe9a6fbe6d488b10e52 Mon Sep 17 00:00:00 2001 From: Yufeng He <40085740+he-yufeng@users.noreply.github.com> Date: Thu, 3 Sep 2026 09:15:26 +0800 Subject: [PATCH 1/8] fix: declare zod runtime dependencies (#4289) Co-authored-by: olaservo --- package-lock.json | 9 ++++++--- src/filesystem/package.json | 3 ++- src/memory/package.json | 3 ++- src/sequentialthinking/package.json | 5 +++-- 4 files changed, 13 insertions(+), 7 deletions(-) diff --git a/package-lock.json b/package-lock.json index 1845571736..632d3404ad 100644 --- a/package-lock.json +++ b/package-lock.json @@ -3866,7 +3866,8 @@ "@modelcontextprotocol/sdk": "^1.30.0", "diff": "^8.0.3", "glob": "^13.0.6", - "minimatch": "^10.0.1" + "minimatch": "^10.0.1", + "zod": "^4.0.0" }, "bin": { "mcp-server-filesystem": "dist/index.js" @@ -3886,7 +3887,8 @@ "version": "0.6.3", "license": "SEE LICENSE IN LICENSE", "dependencies": { - "@modelcontextprotocol/sdk": "^1.30.0" + "@modelcontextprotocol/sdk": "^1.30.0", + "zod": "^4.0.0" }, "bin": { "mcp-server-memory": "dist/index.js" @@ -3906,7 +3908,8 @@ "dependencies": { "@modelcontextprotocol/sdk": "^1.30.0", "chalk": "^5.3.0", - "yargs": "^17.7.2" + "yargs": "^17.7.2", + "zod": "^4.0.0" }, "bin": { "mcp-server-sequential-thinking": "dist/index.js" diff --git a/src/filesystem/package.json b/src/filesystem/package.json index 139c4f00b4..3c288a2673 100644 --- a/src/filesystem/package.json +++ b/src/filesystem/package.json @@ -28,7 +28,8 @@ "@modelcontextprotocol/sdk": "^1.30.0", "diff": "^8.0.3", "glob": "^13.0.6", - "minimatch": "^10.0.1" + "minimatch": "^10.0.1", + "zod": "^4.0.0" }, "devDependencies": { "@types/diff": "^5.0.9", diff --git a/src/memory/package.json b/src/memory/package.json index 4fdcd3f9a4..ea10979f1b 100644 --- a/src/memory/package.json +++ b/src/memory/package.json @@ -25,7 +25,8 @@ "test": "vitest run --coverage" }, "dependencies": { - "@modelcontextprotocol/sdk": "^1.30.0" + "@modelcontextprotocol/sdk": "^1.30.0", + "zod": "^4.0.0" }, "devDependencies": { "@types/node": "^22", diff --git a/src/sequentialthinking/package.json b/src/sequentialthinking/package.json index 03fbb2b361..a668a47fd9 100644 --- a/src/sequentialthinking/package.json +++ b/src/sequentialthinking/package.json @@ -27,7 +27,8 @@ "dependencies": { "@modelcontextprotocol/sdk": "^1.30.0", "chalk": "^5.3.0", - "yargs": "^17.7.2" + "yargs": "^17.7.2", + "zod": "^4.0.0" }, "devDependencies": { "@types/node": "^22", @@ -37,4 +38,4 @@ "typescript": "^5.3.3", "vitest": "^4.1.8" } -} \ No newline at end of file +} From 242751e8ee46c8b87e1bd82cbc2fb0cbcf8ff9ea Mon Sep 17 00:00:00 2001 From: Nick Veenhof Date: Thu, 3 Sep 2026 03:15:35 +0200 Subject: [PATCH 2/8] fix(memory): add trailing newline to JSONL output (#3653) Co-authored-by: Nick Veenhof Co-authored-by: olaservo --- src/memory/__tests__/knowledge-graph.test.ts | 70 ++++++++++++++++++++ src/memory/index.ts | 2 +- 2 files changed, 71 insertions(+), 1 deletion(-) diff --git a/src/memory/__tests__/knowledge-graph.test.ts b/src/memory/__tests__/knowledge-graph.test.ts index 61e823a2a6..0a6fc8f9b0 100644 --- a/src/memory/__tests__/knowledge-graph.test.ts +++ b/src/memory/__tests__/knowledge-graph.test.ts @@ -458,6 +458,76 @@ describe('KnowledgeGraphManager', () => { expect(JSON.parse(lines[1])).toHaveProperty('type', 'relation'); }); + it('should write a trailing newline to produce valid JSONL', async () => { + await manager.createEntities([ + { name: 'Alice', entityType: 'person', observations: ['test'] }, + ]); + + const fileContent = await fs.readFile(testFilePath, 'utf-8'); + expect(fileContent.endsWith('\n')).toBe(true); + }); + + it('should produce a file where every line is individually valid JSON', async () => { + // This test catches the bug where saveGraph wrote lines.join("\n") + // without a trailing newline. When the file was later appended to + // (e.g. by a concurrent process or external tool), the last JSON + // object and the new first JSON object ended up on the same line, + // producing invalid JSONL like: + // {"type":"entity","name":"Alice"}{"type":"relation","from":"Alice",...} + // which fails with: "Unexpected non-whitespace character after JSON + // at position N" + await manager.createEntities([ + { name: 'Alice', entityType: 'person', observations: ['test'] }, + { name: 'Bob', entityType: 'person', observations: [] }, + ]); + await manager.createRelations([ + { from: 'Alice', to: 'Bob', relationType: 'knows' }, + ]); + + const fileContent = await fs.readFile(testFilePath, 'utf-8'); + const allLines = fileContent.split('\n'); + + // Every non-empty line must be valid JSON on its own + for (const line of allLines) { + if (line.trim() === '') continue; + expect(() => JSON.parse(line)).not.toThrow(); + } + }); + + it('should not corrupt JSONL when content is appended to the file externally', async () => { + // Simulate the real-world corruption scenario: + // 1. saveGraph writes entities to the file + // 2. An external process appends a new JSON line to the file + // 3. loadGraph must still parse the file without errors + // + // Without a trailing newline on step 1, the appended content in + // step 2 lands on the same line as the last entity, producing + // invalid JSONL that breaks loadGraph. + await manager.createEntities([ + { name: 'Alice', entityType: 'person', observations: ['original'] }, + ]); + + // Simulate an external append (e.g. another process, a script, or + // a crash-recovery replay). This is what triggers the bug: without + // a trailing newline, this JSON object concatenates onto line 1. + const externalLine = JSON.stringify({ + type: 'entity', + name: 'External', + entityType: 'person', + observations: ['appended externally'], + }); + await fs.appendFile(testFilePath, externalLine + '\n'); + + // A new manager instance forces a fresh loadGraph from disk + const manager2 = new KnowledgeGraphManager(testFilePath); + const graph = await manager2.readGraph(); + + // Both entities must load without a JSON parse error + expect(graph.entities).toHaveLength(2); + expect(graph.entities.map(e => e.name)).toContain('Alice'); + expect(graph.entities.map(e => e.name)).toContain('External'); + }); + it('should strip type field from entities when loading from file', async () => { // Create entities and relations (these get saved with type field) await manager.createEntities([ diff --git a/src/memory/index.ts b/src/memory/index.ts index 3f1179dd66..eb6560d21a 100644 --- a/src/memory/index.ts +++ b/src/memory/index.ts @@ -132,7 +132,7 @@ export class KnowledgeGraphManager { ); try { - await fs.writeFile(tempFilePath, lines.join("\n")); + await fs.writeFile(tempFilePath, lines.join("\n") + "\n"); await fs.rename(tempFilePath, this.memoryFilePath); } catch (error) { // Never leave a stray temp file behind on failure. From 649af5856e8fc4b8598ceb1e75cc31215a9e0e90 Mon Sep 17 00:00:00 2001 From: Amey Pawar <138877912+ameyypawar@users.noreply.github.com> Date: Thu, 3 Sep 2026 06:45:44 +0530 Subject: [PATCH 3/8] fix(memory): expand leading ~ in MEMORY_FILE_PATH to the home directory (#4447) * fix(memory): expand leading ~ in MEMORY_FILE_PATH to the home directory MCP clients pass MEMORY_FILE_PATH from JSON config, where no shell expands a leading "~". Because path.isAbsolute("~/memory.jsonl") is false, the value was joined onto the package directory, so the server persisted to a literal "~" folder inside the install instead of the user's intended location. Add an expandHome() helper that mirrors the one already used by the filesystem server (src/filesystem/path-utils.ts) and apply it in ensureMemoryFilePath() before the existing absolute/relative resolution. Absolute paths, relative paths, and a "~" not at the start are unaffected. Adds unit tests for expandHome plus an end-to-end test through ensureMemoryFilePath. Addresses #1600 * test(memory): use a literal ~/ path in the tilde expansion test path.join('~', ...) produces a backslash on Windows, which expandHome does not match, so the test failed there. --------- Co-authored-by: olaservo --- src/memory/__tests__/file-path.test.ts | 38 +++++++++++++++++++++++++- src/memory/index.ts | 23 +++++++++++++--- 2 files changed, 56 insertions(+), 5 deletions(-) diff --git a/src/memory/__tests__/file-path.test.ts b/src/memory/__tests__/file-path.test.ts index d1a16e4600..fe5fdeb0db 100644 --- a/src/memory/__tests__/file-path.test.ts +++ b/src/memory/__tests__/file-path.test.ts @@ -1,8 +1,9 @@ import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; import { promises as fs } from 'fs'; import path from 'path'; +import os from 'os'; import { fileURLToPath } from 'url'; -import { ensureMemoryFilePath, defaultMemoryPath } from '../index.js'; +import { ensureMemoryFilePath, defaultMemoryPath, expandHome } from '../index.js'; describe('ensureMemoryFilePath', () => { const testDir = path.dirname(fileURLToPath(import.meta.url)); @@ -72,6 +73,15 @@ describe('ensureMemoryFilePath', () => { expect(path.isAbsolute(result)).toBe(true); } }); + + it('should expand a leading "~/" to the home directory', async () => { + process.env.MEMORY_FILE_PATH = '~/custom-memory.jsonl'; + + const result = await ensureMemoryFilePath(); + + expect(result).toBe(path.join(os.homedir(), 'custom-memory.jsonl')); + expect(path.isAbsolute(result)).toBe(true); + }); }); describe('without MEMORY_FILE_PATH environment variable', () => { @@ -154,3 +164,29 @@ describe('ensureMemoryFilePath', () => { }); }); }); + +describe('expandHome', () => { + it('expands a bare "~" to the home directory', () => { + expect(expandHome('~')).toBe(os.homedir()); + }); + + it('expands a leading "~/" to the home directory', () => { + expect(expandHome('~/notes/memory.jsonl')).toBe( + path.join(os.homedir(), 'notes/memory.jsonl') + ); + }); + + it('leaves a "~" not followed by a separator unchanged', () => { + expect(expandHome('~backup.jsonl')).toBe('~backup.jsonl'); + }); + + it('leaves absolute paths unchanged', () => { + expect(expandHome('/var/data/memory.jsonl')).toBe('/var/data/memory.jsonl'); + }); + + it('leaves relative paths unchanged', () => { + expect(expandHome(path.join('data', 'memory.jsonl'))).toBe( + path.join('data', 'memory.jsonl') + ); + }); +}); diff --git a/src/memory/index.ts b/src/memory/index.ts index eb6560d21a..82fe3845ec 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 os from 'os'; import { randomBytes } from 'crypto'; import { fileURLToPath } from 'url'; import { SERVER_VERSION } from './version.js'; @@ -13,13 +14,27 @@ import { SERVER_VERSION } from './version.js'; // Define memory file path using environment variable with fallback export const defaultMemoryPath = path.join(path.dirname(fileURLToPath(import.meta.url)), 'memory.jsonl'); +// Expand a leading "~" to the user's home directory. MCP clients pass +// MEMORY_FILE_PATH from JSON config, where no shell performs tilde expansion, +// so an unexpanded "~" would otherwise be treated as a relative path and +// joined onto the package directory. Mirrors the helper of the same name in +// the filesystem server (src/filesystem/path-utils.ts). +export function expandHome(filepath: string): string { + if (filepath.startsWith('~/') || filepath === '~') { + return path.join(os.homedir(), filepath.slice(1)); + } + return filepath; +} + // Handle backward compatibility: migrate memory.json to memory.jsonl if needed export async function ensureMemoryFilePath(): Promise { if (process.env.MEMORY_FILE_PATH) { - // Custom path provided, use it as-is (with absolute path resolution) - return path.isAbsolute(process.env.MEMORY_FILE_PATH) - ? process.env.MEMORY_FILE_PATH - : path.join(path.dirname(fileURLToPath(import.meta.url)), process.env.MEMORY_FILE_PATH); + // Custom path provided. Expand a leading "~" first, then resolve relative + // paths against the package directory (absolute paths are used as-is). + const customPath = expandHome(process.env.MEMORY_FILE_PATH); + return path.isAbsolute(customPath) + ? customPath + : path.join(path.dirname(fileURLToPath(import.meta.url)), customPath); } // No custom path set, check for backward compatibility migration From abbbddb2e3cbf1d3c24814bdff874c3c55f652e8 Mon Sep 17 00:00:00 2001 From: Connor Moss <140426627+ConnorMoss02@users.noreply.github.com> Date: Wed, 2 Sep 2026 21:34:53 -0400 Subject: [PATCH 4/8] fix(memory): stop reporting deletions that did not happen (#4738) delete_entities, delete_observations and delete_relations returned success: true with a hardcoded "deleted successfully" message regardless of what matched. An agent that mistypes an entity name is told its memory is clean while the data is still on disk, and nothing in the response contradicts that. addObservations throws for the same condition ten lines above, so the file already disagreed with itself. Staying quiet is deliberate and documented, so nothing throws and the output schema is unchanged. The three manager methods now return what they matched, and the handlers say so. A delete where everything is found returns the same message it always did. README updated: the three "Silent operation" bullets described the absence of an error, which is still true, but read as if the response said nothing either. --- src/memory/README.md | 6 +- src/memory/__tests__/delete-reporting.test.ts | 99 +++++++++++++++++++ src/memory/index.ts | 48 ++++++--- 3 files changed, 138 insertions(+), 15 deletions(-) create mode 100644 src/memory/__tests__/delete-reporting.test.ts diff --git a/src/memory/README.md b/src/memory/README.md index 18851aedaa..de7be2c060 100644 --- a/src/memory/README.md +++ b/src/memory/README.md @@ -87,7 +87,7 @@ Example: - Remove entities and their relations - Input: `entityNames` (string[]) - Cascading deletion of associated relations - - Silent operation if entity doesn't exist + - No error if an entity doesn't exist; the response reports which names were not found - **delete_observations** - Remove specific observations from entities @@ -95,7 +95,7 @@ Example: - Each object contains: - `entityName` (string): Target entity - `observations` (string[]): Observations to remove - - Silent operation if observation doesn't exist + - No error if an observation doesn't exist; the response reports how many were deleted - **delete_relations** - Remove specific relations from the graph @@ -104,7 +104,7 @@ Example: - `from` (string): Source entity name - `to` (string): Target entity name - `relationType` (string): Relationship type - - Silent operation if relation doesn't exist + - No error if a relation doesn't exist; the response reports how many were deleted - **read_graph** - Read the entire knowledge graph diff --git a/src/memory/__tests__/delete-reporting.test.ts b/src/memory/__tests__/delete-reporting.test.ts new file mode 100644 index 0000000000..944c30204f --- /dev/null +++ b/src/memory/__tests__/delete-reporting.test.ts @@ -0,0 +1,99 @@ +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { promises as fs } from 'fs'; +import path from 'path'; +import { fileURLToPath } from 'url'; +import { KnowledgeGraphManager, Entity, Relation } from '../index.js'; + +/** + * The delete tools stay silent when a target is absent, which the README + * documents. What they must not do is report a deletion that did not happen: + * an agent that mistypes a name is told its memory is clean while the data is + * still on disk, and nothing in the response contradicts that. + */ +describe('delete reporting', () => { + let manager: KnowledgeGraphManager; + let testFilePath: string; + + const entities: Entity[] = [ + { name: 'Alice', entityType: 'person', observations: ['works at Acme Corp', 'likes tea'] }, + { name: 'Bob', entityType: 'person', observations: ['likes programming'] }, + ]; + const relations: Relation[] = [{ from: 'Alice', to: 'Bob', relationType: 'works_with' }]; + + beforeEach(async () => { + testFilePath = path.join( + path.dirname(fileURLToPath(import.meta.url)), + `test-delete-reporting-${Date.now()}-${Math.random().toString(16).slice(2)}.jsonl` + ); + manager = new KnowledgeGraphManager(testFilePath); + await manager.createEntities(entities); + await manager.createRelations(relations); + }); + + afterEach(async () => { + try { + await fs.unlink(testFilePath); + } catch { + // the file is gone already + } + }); + + describe('deleteEntities', () => { + it('reports which names matched and which did not', async () => { + const result = await manager.deleteEntities(['Alice', 'Alise']); + expect(result).toEqual({ deleted: ['Alice'], notFound: ['Alise'] }); + }); + + it('reports nothing deleted when no name matches', async () => { + const result = await manager.deleteEntities(['Nobody']); + expect(result).toEqual({ deleted: [], notFound: ['Nobody'] }); + + const graph = await manager.readGraph(); + expect(graph.entities).toHaveLength(2); + }); + + it('still deletes the entity and its relations', async () => { + await manager.deleteEntities(['Alice']); + + const graph = await manager.readGraph(); + expect(graph.entities.map(e => e.name)).toEqual(['Bob']); + expect(graph.relations).toHaveLength(0); + }); + }); + + describe('deleteObservations', () => { + it('counts only the observations that were present', async () => { + const result = await manager.deleteObservations([ + { entityName: 'Alice', observations: ['likes tea', 'never said this'] }, + ]); + expect(result).toEqual({ deletedCount: 1, missingEntities: [] }); + }); + + it('names an entity that does not exist', async () => { + const result = await manager.deleteObservations([ + { entityName: 'Carol', observations: ['anything'] }, + ]); + expect(result).toEqual({ deletedCount: 0, missingEntities: ['Carol'] }); + }); + }); + + describe('deleteRelations', () => { + it('counts only the relations that matched', async () => { + const result = await manager.deleteRelations([ + { from: 'Alice', to: 'Bob', relationType: 'works_with' }, + { from: 'Alice', to: 'Bob', relationType: 'never_existed' }, + ]); + expect(result).toEqual({ deletedCount: 1 }); + }); + + it('reports nothing deleted when the relation type is wrong', async () => { + const result = await manager.deleteRelations([ + { from: 'Alice', to: 'Bob', relationType: 'manages' }, + ]); + expect(result).toEqual({ deletedCount: 0 }); + + const graph = await manager.readGraph(); + expect(graph.relations).toHaveLength(1); + }); + }); +}); diff --git a/src/memory/index.ts b/src/memory/index.ts index 82fe3845ec..95e0bc796f 100644 --- a/src/memory/index.ts +++ b/src/memory/index.ts @@ -202,32 +202,45 @@ export class KnowledgeGraphManager { return results; } - async deleteEntities(entityNames: string[]): Promise { + async deleteEntities(entityNames: string[]): Promise<{ deleted: string[]; notFound: string[] }> { const graph = await this.loadGraph(); + const present = new Set(graph.entities.map(e => e.name)); + const deleted = entityNames.filter(name => present.has(name)); + const notFound = entityNames.filter(name => !present.has(name)); graph.entities = graph.entities.filter(e => !entityNames.includes(e.name)); graph.relations = graph.relations.filter(r => !entityNames.includes(r.from) && !entityNames.includes(r.to)); await this.saveGraph(graph); + return { deleted, notFound }; } - async deleteObservations(deletions: { entityName: string; observations: string[] }[]): Promise { + async deleteObservations(deletions: { entityName: string; observations: string[] }[]): Promise<{ deletedCount: number; missingEntities: string[] }> { const graph = await this.loadGraph(); + let deletedCount = 0; + const missingEntities: string[] = []; deletions.forEach(d => { const entity = graph.entities.find(e => e.name === d.entityName); if (entity) { + const before = entity.observations.length; entity.observations = entity.observations.filter(o => !d.observations.includes(o)); + deletedCount += before - entity.observations.length; + } else { + missingEntities.push(d.entityName); } }); await this.saveGraph(graph); + return { deletedCount, missingEntities }; } - async deleteRelations(relations: Relation[]): Promise { + async deleteRelations(relations: Relation[]): Promise<{ deletedCount: number }> { const graph = await this.loadGraph(); + const before = graph.relations.length; graph.relations = graph.relations.filter(r => !relations.some(delRelation => r.from === delRelation.from && r.to === delRelation.to && r.relationType === delRelation.relationType )); await this.saveGraph(graph); + return { deletedCount: before - graph.relations.length }; } async readGraph(): Promise { @@ -436,11 +449,14 @@ server.registerTool( } }, async ({ entityNames }) => { - await knowledgeGraphManager.deleteEntities(entityNames); + const { deleted, notFound } = await knowledgeGraphManager.deleteEntities(entityNames); notifyGraphUpdated(); + const message = notFound.length === 0 + ? "Entities deleted successfully" + : `Deleted ${deleted.length} of ${entityNames.length} entities. Not found: ${notFound.join(", ")}`; return { - content: [{ type: "text" as const, text: "Entities deleted successfully" }], - structuredContent: { success: true, message: "Entities deleted successfully" } + content: [{ type: "text" as const, text: message }], + structuredContent: { success: true, message } }; } ); @@ -469,11 +485,16 @@ server.registerTool( } }, async ({ deletions }) => { - await knowledgeGraphManager.deleteObservations(deletions); + const { deletedCount, missingEntities } = await knowledgeGraphManager.deleteObservations(deletions); notifyGraphUpdated(); + const requested = deletions.reduce((total, d) => total + d.observations.length, 0); + const message = deletedCount === requested + ? "Observations deleted successfully" + : `Deleted ${deletedCount} of ${requested} observations.` + + (missingEntities.length ? ` Entities not found: ${missingEntities.join(", ")}` : ""); return { - content: [{ type: "text" as const, text: "Observations deleted successfully" }], - structuredContent: { success: true, message: "Observations deleted successfully" } + content: [{ type: "text" as const, text: message }], + structuredContent: { success: true, message } }; } ); @@ -499,11 +520,14 @@ server.registerTool( } }, async ({ relations }) => { - await knowledgeGraphManager.deleteRelations(relations); + const { deletedCount } = await knowledgeGraphManager.deleteRelations(relations); notifyGraphUpdated(); + const message = deletedCount === relations.length + ? "Relations deleted successfully" + : `Deleted ${deletedCount} of ${relations.length} relations. The rest matched nothing.`; return { - content: [{ type: "text" as const, text: "Relations deleted successfully" }], - structuredContent: { success: true, message: "Relations deleted successfully" } + content: [{ type: "text" as const, text: message }], + structuredContent: { success: true, message } }; } ); From c3d8e43d1f31a4ac99b7136eb7d8c7958ed9c376 Mon Sep 17 00:00:00 2001 From: Yiheng Zhao Date: Thu, 3 Sep 2026 09:34:58 +0800 Subject: [PATCH 5/8] fix(memory): validate knowledge graph entries when loading from disk (#4717) loadGraph() trusted the persisted memory file and pushed entities and relations without validating their fields. A corrupted or legacy entry (e.g. an entity missing entityType, or an observation that is not a string) would reach searchNodes and crash with "Cannot read properties of undefined (reading 'toLowerCase')". Validate each line against the existing EntitySchema/RelationSchema and skip malformed entries with a warning, so the in-memory graph only ever contains well-formed data. Malformed JSON lines are skipped as well. Fixes #2044 --- src/memory/__tests__/knowledge-graph.test.ts | 47 +++++++++++++++++ src/memory/index.ts | 55 ++++++++++++++------ 2 files changed, 86 insertions(+), 16 deletions(-) diff --git a/src/memory/__tests__/knowledge-graph.test.ts b/src/memory/__tests__/knowledge-graph.test.ts index 0a6fc8f9b0..d1739f4f2e 100644 --- a/src/memory/__tests__/knowledge-graph.test.ts +++ b/src/memory/__tests__/knowledge-graph.test.ts @@ -617,4 +617,51 @@ describe('KnowledgeGraphManager', () => { expect(result.relations[0]).not.toHaveProperty('type'); }); }); + + describe('loadGraph validation', () => { + it('skips corrupt entities instead of crashing search', async () => { + const lines = [ + JSON.stringify({ type: 'entity', name: 'Alice', entityType: 'person', observations: ['works at Acme Corp'] }), + JSON.stringify({ type: 'entity', name: 'Broken', observations: ['missing entityType'] }), + JSON.stringify({ type: 'entity', name: 'BadObs', entityType: 'person', observations: ['ok', null] }), + ]; + await fs.writeFile(testFilePath, lines.join('\n') + '\n'); + + const graph = await manager.readGraph(); + expect(graph.entities).toHaveLength(1); + expect(graph.entities[0].name).toBe('Alice'); + + // searchNodes must not throw even though the file contains corrupt entries + const result = await manager.searchNodes('Acme'); + expect(result.entities).toHaveLength(1); + expect(result.entities[0].name).toBe('Alice'); + }); + + it('skips corrupt relations', async () => { + const lines = [ + JSON.stringify({ type: 'entity', name: 'Alice', entityType: 'person', observations: [] }), + JSON.stringify({ type: 'relation', from: 'Alice', to: 'Bob' }), // missing relationType + JSON.stringify({ type: 'relation', from: 'Alice', to: 'Bob', relationType: 'knows' }), + ]; + await fs.writeFile(testFilePath, lines.join('\n') + '\n'); + + const graph = await manager.readGraph(); + expect(graph.entities).toHaveLength(1); + expect(graph.relations).toHaveLength(1); + expect(graph.relations[0].relationType).toBe('knows'); + }); + + it('skips malformed JSON lines', async () => { + const lines = [ + JSON.stringify({ type: 'entity', name: 'Alice', entityType: 'person', observations: [] }), + '{this is not valid json', + JSON.stringify({ type: 'entity', name: 'Bob', entityType: 'person', observations: [] }), + ]; + await fs.writeFile(testFilePath, lines.join('\n') + '\n'); + + const graph = await manager.readGraph(); + expect(graph.entities).toHaveLength(2); + expect(graph.entities.map(e => e.name)).toEqual(['Alice', 'Bob']); + }); + }); }); diff --git a/src/memory/index.ts b/src/memory/index.ts index 95e0bc796f..c8d4987990 100644 --- a/src/memory/index.ts +++ b/src/memory/index.ts @@ -90,24 +90,47 @@ export class KnowledgeGraphManager { try { const data = await fs.readFile(this.memoryFilePath, "utf-8"); const lines = data.split("\n").filter(line => line.trim() !== ""); - return lines.reduce((graph: KnowledgeGraph, line) => { - const item = JSON.parse(line); - if (item.type === "entity") { - graph.entities.push({ - name: item.name, - entityType: item.entityType, - observations: item.observations - }); + const graph: KnowledgeGraph = { entities: [], relations: [] }; + + for (const line of lines) { + let item: unknown; + try { + item = JSON.parse(line); + } catch { + console.error("Skipping malformed line in memory file"); + continue; } - if (item.type === "relation") { - graph.relations.push({ - from: item.from, - to: item.to, - relationType: item.relationType - }); + + if (typeof item !== "object" || item === null) { + console.error("Skipping non-object line in memory file"); + continue; + } + + const record = item as Record; + if (record.type === "entity") { + const parsed = EntitySchema.safeParse(item); + if (parsed.success) { + graph.entities.push(parsed.data); + } else { + console.error( + "Skipping invalid entity in memory file:", + parsed.error.issues.map(issue => `${issue.path.join(".")}: ${issue.message}`).join(", ") + ); + } + } else if (record.type === "relation") { + const parsed = RelationSchema.safeParse(item); + if (parsed.success) { + graph.relations.push(parsed.data); + } else { + console.error( + "Skipping invalid relation in memory file:", + parsed.error.issues.map(issue => `${issue.path.join(".")}: ${issue.message}`).join(", ") + ); + } } - return graph; - }, { entities: [], relations: [] }); + } + + return graph; } catch (error) { if (error instanceof Error && 'code' in error && (error as any).code === "ENOENT") { return { entities: [], relations: [] }; From f41b666edf470795d3724b06899dbca2d102b4e2 Mon Sep 17 00:00:00 2001 From: fei Date: Thu, 3 Sep 2026 09:35:03 +0800 Subject: [PATCH 6/8] fix(memory): constrain search_nodes query length (#4662) The search_nodes tool accepted an unbounded query string (per modelcontextprotocol/servers#3537, official servers should constrain string parameters). An oversized query costs an O(graph) scan per call with no value. Cap it at 2048 chars via an exported SearchNodesQuerySchema, and add vitest coverage for the boundary (at-limit accepted, over-limit rejected, non-string still rejected). Signed-off-by: fei <204683769+feiiiiii5@users.noreply.github.com> --- .../__tests__/search-nodes-schema.test.ts | 25 +++++++++++++++++++ src/memory/index.ts | 9 ++++++- 2 files changed, 33 insertions(+), 1 deletion(-) create mode 100644 src/memory/__tests__/search-nodes-schema.test.ts diff --git a/src/memory/__tests__/search-nodes-schema.test.ts b/src/memory/__tests__/search-nodes-schema.test.ts new file mode 100644 index 0000000000..c03f384ebb --- /dev/null +++ b/src/memory/__tests__/search-nodes-schema.test.ts @@ -0,0 +1,25 @@ +import { describe, it, expect } from 'vitest'; +import { SearchNodesQuerySchema, SEARCH_QUERY_MAX_LENGTH } from '../index.js'; + +describe('search_nodes input schema', () => { + it('should accept a normal query', () => { + expect(SearchNodesQuerySchema.safeParse('Alice').success).toBe(true); + expect(SearchNodesQuerySchema.safeParse('works at Acme Corp').success).toBe(true); + }); + + it('should accept a query at exactly the max length', () => { + const atLimit = 'a'.repeat(SEARCH_QUERY_MAX_LENGTH); + expect(SearchNodesQuerySchema.safeParse(atLimit).success).toBe(true); + }); + + it('should reject a query longer than the max length', () => { + const oversized = 'a'.repeat(SEARCH_QUERY_MAX_LENGTH + 1); + const result = SearchNodesQuerySchema.safeParse(oversized); + expect(result.success).toBe(false); + }); + + it('should still reject non-string input', () => { + expect(SearchNodesQuerySchema.safeParse(42).success).toBe(false); + expect(SearchNodesQuerySchema.safeParse(null).success).toBe(false); + }); +}); diff --git a/src/memory/index.ts b/src/memory/index.ts index c8d4987990..7555e4ad4c 100644 --- a/src/memory/index.ts +++ b/src/memory/index.ts @@ -582,6 +582,13 @@ server.registerTool( } ); +export const SEARCH_QUERY_MAX_LENGTH = 2048; + +export const SearchNodesQuerySchema = z + .string() + .max(SEARCH_QUERY_MAX_LENGTH) + .describe("The search query to match against entity names, types, and observation content"); + // Register search_nodes tool server.registerTool( "search_nodes", @@ -589,7 +596,7 @@ server.registerTool( title: "Search Nodes", description: "Search for nodes in the knowledge graph based on a query", inputSchema: { - query: z.string().describe("The search query to match against entity names, types, and observation content") + query: SearchNodesQuerySchema }, outputSchema: { entities: z.array(EntitySchema), From 1ec570c27d977a122a619da58ae3104f7fdf8be7 Mon Sep 17 00:00:00 2001 From: JSap0914 <116227558+JSap0914@users.noreply.github.com> Date: Thu, 3 Sep 2026 10:38:22 +0900 Subject: [PATCH 7/8] fix(memory): skip duplicate entities and relations within a single batch (#4383) create_entities and create_relations only de-duplicated against the existing graph, so passing the same entity name (or identical relation) twice in one call persisted duplicate records. This contradicts the documented behavior ("Ignores entities with existing names" / "Skips duplicate relations") and breaks the implicit name-uniqueness invariant the rest of the manager relies on (e.g. addObservations/deleteEntities key on name). De-duplicate within the input batch as well, keeping the first occurrence. Co-authored-by: JSap0914 Co-authored-by: olaservo --- src/memory/__tests__/knowledge-graph.test.ts | 32 ++++++++++++++++++++ src/memory/index.ts | 20 ++++++++---- 2 files changed, 46 insertions(+), 6 deletions(-) diff --git a/src/memory/__tests__/knowledge-graph.test.ts b/src/memory/__tests__/knowledge-graph.test.ts index d1739f4f2e..17a85aa97f 100644 --- a/src/memory/__tests__/knowledge-graph.test.ts +++ b/src/memory/__tests__/knowledge-graph.test.ts @@ -59,6 +59,20 @@ describe('KnowledgeGraphManager', () => { const newEntities = await manager.createEntities([]); expect(newEntities).toHaveLength(0); }); + + it('should ignore duplicate entity names within a single batch', async () => { + const entities: Entity[] = [ + { name: 'Alice', entityType: 'person', observations: ['first'] }, + { name: 'Alice', entityType: 'person', observations: ['second'] }, + ]; + + const newEntities = await manager.createEntities(entities); + expect(newEntities).toHaveLength(1); + + const graph = await manager.readGraph(); + expect(graph.entities).toHaveLength(1); + expect(graph.entities[0].name).toBe('Alice'); + }); }); describe('createRelations', () => { @@ -135,6 +149,24 @@ describe('KnowledgeGraphManager', () => { const newRelations = await manager.createRelations([]); expect(newRelations).toHaveLength(0); }); + + it('should skip duplicate relations within a single batch', async () => { + await manager.createEntities([ + { name: 'Alice', entityType: 'person', observations: [] }, + { name: 'Bob', entityType: 'person', observations: [] }, + ]); + + const relations: Relation[] = [ + { from: 'Alice', to: 'Bob', relationType: 'knows' }, + { from: 'Alice', to: 'Bob', relationType: 'knows' }, + ]; + + const newRelations = await manager.createRelations(relations); + expect(newRelations).toHaveLength(1); + + const graph = await manager.readGraph(); + expect(graph.relations).toHaveLength(1); + }); }); describe('addObservations', () => { diff --git a/src/memory/index.ts b/src/memory/index.ts index 7555e4ad4c..e0a8ce92cf 100644 --- a/src/memory/index.ts +++ b/src/memory/index.ts @@ -181,7 +181,11 @@ export class KnowledgeGraphManager { async createEntities(entities: Entity[]): Promise { const graph = await this.loadGraph(); - const newEntities = entities.filter(e => !graph.entities.some(existingEntity => existingEntity.name === e.name)); + const newEntities = entities.filter((e, index) => + !graph.entities.some(existingEntity => existingEntity.name === e.name) && + // Also skip duplicates appearing earlier in this same batch + !entities.slice(0, index).some(earlier => earlier.name === e.name) + ); graph.entities.push(...newEntities); await this.saveGraph(graph); return newEntities; @@ -200,11 +204,15 @@ export class KnowledgeGraphManager { } }); - const newRelations = relations.filter(r => !graph.relations.some(existingRelation => - existingRelation.from === r.from && - existingRelation.to === r.to && - existingRelation.relationType === r.relationType - )); + const isSameRelation = (a: Relation, b: Relation) => + a.from === b.from && + a.to === b.to && + a.relationType === b.relationType; + const newRelations = relations.filter((r, index) => + !graph.relations.some(existingRelation => isSameRelation(existingRelation, r)) && + // Also skip duplicates appearing earlier in this same batch + !relations.slice(0, index).some(earlier => isSameRelation(earlier, r)) + ); graph.relations.push(...newRelations); await this.saveGraph(graph); return newRelations; From d73f99efbfd40c3aa1b61e88728b3d49fb52608f Mon Sep 17 00:00:00 2001 From: Nandini-Inuguru Date: Thu, 3 Sep 2026 07:12:26 +0530 Subject: [PATCH 8/8] fix(memory): serialize graph mutations to prevent concurrent write race (#4555) createEntities, createRelations, addObservations, deleteEntities, deleteObservations, and deleteRelations each independently did load -> mutate -> save with no synchronization. Concurrent tool calls (e.g. multiple mutations dispatched from one LLM turn) could race: both read the same starting state, both write back their own copy, and whichever write landed last silently discarded the other's changes. Interleaved writes could also corrupt the file outright. Adds an in-process async mutex (KnowledgeGraphManager.withLock) that serializes all six mutation methods through a single queue. Read-only methods (readGraph, searchNodes, openNodes) are unaffected. Verified: reverting the fix and re-running the new concurrency tests reproduces the bug exactly (lost entities, lost relations, malformed JSONL lines). With the fix, all 39 tests pass. Fixes #1819 Co-authored-by: olaservo --- src/memory/__tests__/knowledge-graph.test.ts | 75 ++++++++ src/memory/index.ts | 178 +++++++++++-------- 2 files changed, 180 insertions(+), 73 deletions(-) diff --git a/src/memory/__tests__/knowledge-graph.test.ts b/src/memory/__tests__/knowledge-graph.test.ts index 17a85aa97f..7d05a0c053 100644 --- a/src/memory/__tests__/knowledge-graph.test.ts +++ b/src/memory/__tests__/knowledge-graph.test.ts @@ -696,4 +696,79 @@ describe('KnowledgeGraphManager', () => { expect(graph.entities.map(e => e.name)).toEqual(['Alice', 'Bob']); }); }); + + describe('concurrent mutations', () => { + // Regression test for #1819: concurrent tool calls each independently + // load the graph, mutate their own copy, and write it back. Without + // serialization, whichever write lands last silently discards the + // other's changes. All mutations below are fired without awaiting each + // other first, simulating multiple tool calls landing close together. + + it('should not lose entities created concurrently', async () => { + const batch1: Entity[] = Array.from({ length: 10 }, (_, i) => ({ + name: `batch1-entity-${i}`, + entityType: 'test', + observations: [], + })); + const batch2: Entity[] = Array.from({ length: 10 }, (_, i) => ({ + name: `batch2-entity-${i}`, + entityType: 'test', + observations: [], + })); + + // Fire both concurrently instead of awaiting sequentially. + await Promise.all([ + manager.createEntities(batch1), + manager.createEntities(batch2), + ]); + + const graph = await manager.readGraph(); + expect(graph.entities).toHaveLength(20); + expect(graph.entities.map(e => e.name).sort()).toEqual( + [...batch1, ...batch2].map(e => e.name).sort() + ); + }); + + it('should not lose relations created concurrently with entity creation', async () => { + await manager.createEntities([ + { name: 'Alice', entityType: 'person', observations: [] }, + { name: 'Bob', entityType: 'person', observations: [] }, + { name: 'Carol', entityType: 'person', observations: [] }, + ]); + + await Promise.all([ + manager.createRelations([{ from: 'Alice', to: 'Bob', relationType: 'knows' }]), + manager.createRelations([{ from: 'Bob', to: 'Carol', relationType: 'knows' }]), + manager.addObservations([ + { entityName: 'Alice', contents: ['likes coffee'] }, + ]), + ]); + + const graph = await manager.readGraph(); + expect(graph.relations).toHaveLength(2); + expect(graph.entities.find(e => e.name === 'Alice')?.observations).toContain('likes coffee'); + }); + + it('should keep the file valid JSONL after many concurrent mutations', async () => { + const operations = Array.from({ length: 25 }, (_, i) => + manager.createEntities([ + { name: `stress-entity-${i}`, entityType: 'test', observations: [] }, + ]) + ); + + await Promise.all(operations); + + const raw = await fs.readFile(testFilePath, 'utf-8'); + const lines = raw.split('\n').filter(line => line.trim() !== ''); + + // Every line must parse as valid JSON; a corrupted interleaved write + // would produce a truncated or malformed line here. + for (const line of lines) { + expect(() => JSON.parse(line)).not.toThrow(); + } + + const graph = await manager.readGraph(); + expect(graph.entities).toHaveLength(25); + }); + }); }); diff --git a/src/memory/index.ts b/src/memory/index.ts index e0a8ce92cf..d9f814877b 100644 --- a/src/memory/index.ts +++ b/src/memory/index.ts @@ -86,6 +86,26 @@ export interface KnowledgeGraph { export class KnowledgeGraphManager { constructor(private memoryFilePath: string) {} + // Serializes all read-modify-write graph mutations behind a single queue. + // Without this, concurrent tool calls (e.g. multiple mutations dispatched + // from one LLM turn) each independently load the graph, mutate their own + // copy, and write it back — so whichever write lands last silently + // overwrites the other's changes, and interleaved writes to the same file + // can corrupt it outright. See #1819. + private mutationQueue: Promise = Promise.resolve(); + + private async withLock(operation: () => Promise): Promise { + const result = this.mutationQueue.then(operation, operation); + // Always resolve the queue itself, even if this operation failed, so a + // single failed mutation doesn't permanently wedge every call after it. + // The failure still propagates normally to whoever awaited `result`. + this.mutationQueue = result.then( + () => undefined, + () => undefined, + ); + return result; + } + private async loadGraph(): Promise { try { const data = await fs.readFile(this.memoryFilePath, "utf-8"); @@ -180,98 +200,110 @@ export class KnowledgeGraphManager { } async createEntities(entities: Entity[]): Promise { - const graph = await this.loadGraph(); - const newEntities = entities.filter((e, index) => - !graph.entities.some(existingEntity => existingEntity.name === e.name) && - // Also skip duplicates appearing earlier in this same batch - !entities.slice(0, index).some(earlier => earlier.name === e.name) - ); - graph.entities.push(...newEntities); - await this.saveGraph(graph); - return newEntities; + return this.withLock(async () => { + const graph = await this.loadGraph(); + const newEntities = entities.filter((e, index) => + !graph.entities.some(existingEntity => existingEntity.name === e.name) && + // Also skip duplicates appearing earlier in this same batch + !entities.slice(0, index).some(earlier => earlier.name === e.name) + ); + graph.entities.push(...newEntities); + await this.saveGraph(graph); + return newEntities; + }); } async createRelations(relations: Relation[]): Promise { - const graph = await this.loadGraph(); - const entityNames = new Set(graph.entities.map(e => e.name)); + return this.withLock(async () => { + const graph = await this.loadGraph(); + const entityNames = new Set(graph.entities.map(e => e.name)); - relations.forEach(r => { - if (!entityNames.has(r.from)) { - throw new Error(`Entity with name ${r.from} not found`); - } - if (!entityNames.has(r.to)) { - throw new Error(`Entity with name ${r.to} not found`); - } + relations.forEach(r => { + if (!entityNames.has(r.from)) { + throw new Error(`Entity with name ${r.from} not found`); + } + if (!entityNames.has(r.to)) { + throw new Error(`Entity with name ${r.to} not found`); + } + }); + + const isSameRelation = (a: Relation, b: Relation) => + a.from === b.from && + a.to === b.to && + a.relationType === b.relationType; + const newRelations = relations.filter((r, index) => + !graph.relations.some(existingRelation => isSameRelation(existingRelation, r)) && + // Also skip duplicates appearing earlier in this same batch + !relations.slice(0, index).some(earlier => isSameRelation(earlier, r)) + ); + graph.relations.push(...newRelations); + await this.saveGraph(graph); + return newRelations; }); - - const isSameRelation = (a: Relation, b: Relation) => - a.from === b.from && - a.to === b.to && - a.relationType === b.relationType; - const newRelations = relations.filter((r, index) => - !graph.relations.some(existingRelation => isSameRelation(existingRelation, r)) && - // Also skip duplicates appearing earlier in this same batch - !relations.slice(0, index).some(earlier => isSameRelation(earlier, r)) - ); - graph.relations.push(...newRelations); - await this.saveGraph(graph); - return newRelations; } async addObservations(observations: { entityName: string; contents: string[] }[]): Promise<{ entityName: string; addedObservations: string[] }[]> { - const graph = await this.loadGraph(); - const results = observations.map(o => { - const entity = graph.entities.find(e => e.name === o.entityName); - if (!entity) { - throw new Error(`Entity with name ${o.entityName} not found`); - } - const newObservations = o.contents.filter(content => !entity.observations.includes(content)); - entity.observations.push(...newObservations); - return { entityName: o.entityName, addedObservations: newObservations }; + return this.withLock(async () => { + const graph = await this.loadGraph(); + const results = observations.map(o => { + const entity = graph.entities.find(e => e.name === o.entityName); + if (!entity) { + throw new Error(`Entity with name ${o.entityName} not found`); + } + const newObservations = o.contents.filter(content => !entity.observations.includes(content)); + entity.observations.push(...newObservations); + return { entityName: o.entityName, addedObservations: newObservations }; + }); + await this.saveGraph(graph); + return results; }); - await this.saveGraph(graph); - return results; } async deleteEntities(entityNames: string[]): Promise<{ deleted: string[]; notFound: string[] }> { - const graph = await this.loadGraph(); - const present = new Set(graph.entities.map(e => e.name)); - const deleted = entityNames.filter(name => present.has(name)); - const notFound = entityNames.filter(name => !present.has(name)); - graph.entities = graph.entities.filter(e => !entityNames.includes(e.name)); - graph.relations = graph.relations.filter(r => !entityNames.includes(r.from) && !entityNames.includes(r.to)); - await this.saveGraph(graph); - return { deleted, notFound }; + return this.withLock(async () => { + const graph = await this.loadGraph(); + const present = new Set(graph.entities.map(e => e.name)); + const deleted = entityNames.filter(name => present.has(name)); + const notFound = entityNames.filter(name => !present.has(name)); + graph.entities = graph.entities.filter(e => !entityNames.includes(e.name)); + graph.relations = graph.relations.filter(r => !entityNames.includes(r.from) && !entityNames.includes(r.to)); + await this.saveGraph(graph); + return { deleted, notFound }; + }); } async deleteObservations(deletions: { entityName: string; observations: string[] }[]): Promise<{ deletedCount: number; missingEntities: string[] }> { - const graph = await this.loadGraph(); - let deletedCount = 0; - const missingEntities: string[] = []; - deletions.forEach(d => { - const entity = graph.entities.find(e => e.name === d.entityName); - if (entity) { - const before = entity.observations.length; - entity.observations = entity.observations.filter(o => !d.observations.includes(o)); - deletedCount += before - entity.observations.length; - } else { - missingEntities.push(d.entityName); - } + return this.withLock(async () => { + const graph = await this.loadGraph(); + let deletedCount = 0; + const missingEntities: string[] = []; + deletions.forEach(d => { + const entity = graph.entities.find(e => e.name === d.entityName); + if (entity) { + const before = entity.observations.length; + entity.observations = entity.observations.filter(o => !d.observations.includes(o)); + deletedCount += before - entity.observations.length; + } else { + missingEntities.push(d.entityName); + } + }); + await this.saveGraph(graph); + return { deletedCount, missingEntities }; }); - await this.saveGraph(graph); - return { deletedCount, missingEntities }; } async deleteRelations(relations: Relation[]): Promise<{ deletedCount: number }> { - const graph = await this.loadGraph(); - const before = graph.relations.length; - graph.relations = graph.relations.filter(r => !relations.some(delRelation => - r.from === delRelation.from && - r.to === delRelation.to && - r.relationType === delRelation.relationType - )); - await this.saveGraph(graph); - return { deletedCount: before - graph.relations.length }; + return this.withLock(async () => { + const graph = await this.loadGraph(); + const before = graph.relations.length; + graph.relations = graph.relations.filter(r => !relations.some(delRelation => + r.from === delRelation.from && + r.to === delRelation.to && + r.relationType === delRelation.relationType + )); + await this.saveGraph(graph); + return { deletedCount: before - graph.relations.length }; + }); } async readGraph(): Promise {