Skip to content

Commit df743d4

Browse files
cliffhallclaude
andcommitted
fix(memory): keep unmodelled fields on stored records across writes
Entity and relation records that parse could carry members the schema does not model, such as a createdAt written by an older version or another client. Loading stripped them and saveGraph wrote only the modelled fields, so any write dropped them, even an empty create_entities call, while create_entities, create_relations and add_observations declare destructiveHint: false. Each valid record's extra members are now kept in a manager-owned WeakMap keyed by its parsed object, and saveGraph writes them back after the modelled fields. A record a delete tool removes takes its extras with it, and the extras never reach a tool result. Closes #5058 Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: cliffhall <[email protected]>
1 parent 846cc6c commit df743d4

3 files changed

Lines changed: 278 additions & 0 deletions

File tree

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"@modelcontextprotocol/server-memory": patch
3+
---
4+
5+
Keep the fields the server does not model on stored entities and relations (a `createdAt` written by an older version or another client, say) when a write saves the graph. Before, every write dropped them from the memory file, even an empty `create_entities` call, so `create_entities`, `create_relations` and `add_observations` destroyed data despite declaring `destructiveHint: false`. A record a delete tool removes still goes with its fields, and the fields never appear in tool results.
Lines changed: 235 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,235 @@
1+
// Regression tests for #5058, driven through an SDK Client over an in-memory
2+
// transport: members of a valid stored entity or relation record that the
3+
// schema does not model (a createdAt written by an older version or another
4+
// client, say) survive every write that keeps the record. Without that,
5+
// create_entities, create_relations and add_observations would drop data they
6+
// did not touch, while declaring destructiveHint: false. The extras stay out
7+
// of every tool result, since the output schemas do not allow them.
8+
import { describe, it, expect, beforeEach, afterEach } from "vitest";
9+
import { promises as fs } from "fs";
10+
import { call, connect, makeTempGraph, readFileText } from "./helpers.js";
11+
import type { Connection } from "./helpers.js";
12+
13+
// Extras of every JSON kind, nested ones included, and falsy values that a
14+
// truthiness check would lose.
15+
const aliceExtras = {
16+
createdAt: "2025-01-01T00:00:00Z",
17+
meta: { source: "import", tags: ["a", "b"], verified: false, rank: 0 },
18+
note: null,
19+
};
20+
const bobExtras = { createdAt: "2025-02-02T00:00:00Z" };
21+
const relationExtras = { since: 2019, weight: [1, 2, { strong: true }] };
22+
23+
const alice = {
24+
name: "Alice",
25+
entityType: "person",
26+
observations: ["Works at Acme"],
27+
};
28+
const bob = { name: "Bob", entityType: "person", observations: [] };
29+
const carol = { name: "Carol", entityType: "person", observations: [] };
30+
const aliceManagesBob = { from: "Alice", to: "Bob", relationType: "manages" };
31+
const bobKnowsAlice = { from: "Bob", to: "Alice", relationType: "knows" };
32+
33+
// The stored records, with the unknown members written before the modelled
34+
// ones, as another client might order them.
35+
const seedRecords = [
36+
{ ...aliceExtras, type: "entity", ...alice },
37+
{ type: "entity", ...bob, ...bobExtras },
38+
{ type: "relation", ...aliceManagesBob, ...relationExtras },
39+
];
40+
41+
describe("unmodelled fields on stored records (#5058)", () => {
42+
let filePath: string;
43+
let cleanup: () => Promise<void>;
44+
let conn: Connection | undefined;
45+
46+
beforeEach(async () => {
47+
({ filePath, cleanup } = await makeTempGraph());
48+
conn = undefined;
49+
await fs.writeFile(
50+
filePath,
51+
seedRecords.map((r) => JSON.stringify(r)).join("\n") + "\n",
52+
);
53+
});
54+
55+
afterEach(async () => {
56+
await conn?.close();
57+
await cleanup();
58+
});
59+
60+
async function open(): Promise<Connection> {
61+
conn = await connect(filePath);
62+
return conn;
63+
}
64+
65+
// The graph file, one parsed record per line.
66+
async function records(): Promise<unknown[]> {
67+
return (await readFileText(filePath))
68+
.split("\n")
69+
.filter((line) => line !== "")
70+
.map((line) => JSON.parse(line) as unknown);
71+
}
72+
73+
const storedAlice = (observations = alice.observations) => ({
74+
type: "entity",
75+
...alice,
76+
observations,
77+
...aliceExtras,
78+
});
79+
const storedBob = { type: "entity", ...bob, ...bobExtras };
80+
const storedRelation = {
81+
type: "relation",
82+
...aliceManagesBob,
83+
...relationExtras,
84+
};
85+
86+
it("writes the modelled fields first and the extras after them, unchanged", async () => {
87+
const { client } = await open();
88+
await call(client, "create_entities", { entities: [] });
89+
90+
expect((await readFileText(filePath)).split("\n")).toEqual([
91+
'{"type":"entity","name":"Alice","entityType":"person","observations":["Works at Acme"],"createdAt":"2025-01-01T00:00:00Z","meta":{"source":"import","tags":["a","b"],"verified":false,"rank":0},"note":null}',
92+
'{"type":"entity","name":"Bob","entityType":"person","observations":[],"createdAt":"2025-02-02T00:00:00Z"}',
93+
'{"type":"relation","from":"Alice","to":"Bob","relationType":"manages","since":2019,"weight":[1,2,{"strong":true}]}',
94+
"",
95+
]);
96+
});
97+
98+
// Each additive call, empty, duplicate and effective, keeps every record's
99+
// extras, and a record it creates is written with none.
100+
it.each([
101+
["an empty create_entities", "create_entities", { entities: [] }, []],
102+
[
103+
"a create_entities of an existing name",
104+
"create_entities",
105+
{ entities: [alice] },
106+
[],
107+
],
108+
[
109+
"a create_entities of a new entity",
110+
"create_entities",
111+
{ entities: [carol] },
112+
[{ type: "entity", ...carol }],
113+
],
114+
["an empty create_relations", "create_relations", { relations: [] }, []],
115+
[
116+
"a create_relations of an existing relation",
117+
"create_relations",
118+
{ relations: [aliceManagesBob] },
119+
[],
120+
],
121+
[
122+
"a create_relations of a new relation",
123+
"create_relations",
124+
{ relations: [bobKnowsAlice] },
125+
[{ type: "relation", ...bobKnowsAlice }],
126+
],
127+
["an empty add_observations", "add_observations", { observations: [] }, []],
128+
[
129+
"an add_observations of an existing observation",
130+
"add_observations",
131+
{ observations: [{ entityName: "Alice", contents: ["Works at Acme"] }] },
132+
[],
133+
],
134+
])(
135+
"keeps them on %s",
136+
async (_label, tool, args, added: Record<string, unknown>[]) => {
137+
const { client } = await open();
138+
const result = await call(client, tool, args);
139+
expect(result.isError).toBeUndefined();
140+
141+
const newEntities = added.filter((r) => r.type === "entity");
142+
const newRelations = added.filter((r) => r.type === "relation");
143+
expect(await records()).toEqual([
144+
storedAlice(),
145+
storedBob,
146+
...newEntities,
147+
storedRelation,
148+
...newRelations,
149+
]);
150+
},
151+
);
152+
153+
it("keeps them on an entity whose observations change, with the new observations", async () => {
154+
const { client } = await open();
155+
await call(client, "add_observations", {
156+
observations: [{ entityName: "Alice", contents: ["Likes tea"] }],
157+
});
158+
expect(await records()).toEqual([
159+
storedAlice(["Works at Acme", "Likes tea"]),
160+
storedBob,
161+
storedRelation,
162+
]);
163+
164+
await call(client, "delete_observations", {
165+
deletions: [{ entityName: "Alice", observations: ["Works at Acme"] }],
166+
});
167+
expect(await records()).toEqual([
168+
storedAlice(["Likes tea"]),
169+
storedBob,
170+
storedRelation,
171+
]);
172+
});
173+
174+
// A delete tool is destructive by declaration: a record it removes goes with
175+
// its extras, and the records it keeps keep theirs.
176+
it("drops them with a deleted entity and its relations, and keeps the rest", async () => {
177+
const { client } = await open();
178+
await call(client, "delete_entities", { entityNames: ["Bob"] });
179+
expect(await records()).toEqual([storedAlice()]);
180+
});
181+
182+
it("drops them with a deleted relation, and keeps the entities'", async () => {
183+
const { client } = await open();
184+
await call(client, "delete_relations", { relations: [aliceManagesBob] });
185+
expect(await records()).toEqual([storedAlice(), storedBob]);
186+
});
187+
188+
it("keeps them across a restart of the server", async () => {
189+
const first = await open();
190+
await call(first.client, "create_entities", { entities: [carol] });
191+
await first.close();
192+
conn = undefined;
193+
194+
const { client } = await open();
195+
await call(client, "add_observations", { observations: [] });
196+
expect(await records()).toEqual([
197+
storedAlice(),
198+
storedBob,
199+
{ type: "entity", ...carol },
200+
storedRelation,
201+
]);
202+
});
203+
204+
it("never returns them in a tool result", async () => {
205+
const { client } = await open();
206+
const graph = { entities: [alice, bob], relations: [aliceManagesBob] };
207+
208+
expect((await call(client, "read_graph")).structuredContent).toEqual(graph);
209+
expect(
210+
(await call(client, "search_nodes", { query: "Alice" }))
211+
.structuredContent,
212+
).toEqual({ entities: [alice], relations: [aliceManagesBob] });
213+
expect(
214+
(await call(client, "open_nodes", { names: ["Alice", "Bob"] }))
215+
.structuredContent,
216+
).toEqual(graph);
217+
});
218+
219+
// JSON.parse makes "__proto__" an ordinary member; it must be written back
220+
// as one, not set the prototype of the extras or of the written record.
221+
it("keeps a __proto__ member as data", async () => {
222+
const line =
223+
'{"type":"entity","name":"Dave","entityType":"person","observations":[],"__proto__":{"polluted":true}}';
224+
await fs.writeFile(filePath, line + "\n");
225+
const { client } = await open();
226+
227+
await call(client, "create_entities", { entities: [] });
228+
229+
expect(await readFileText(filePath)).toBe(line + "\n");
230+
expect((await call(client, "read_graph")).structuredContent).toEqual({
231+
entities: [{ name: "Dave", entityType: "person", observations: [] }],
232+
relations: [],
233+
});
234+
});
235+
});

‎src/memory/index.ts‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -340,6 +340,20 @@ export class KnowledgeGraphManager {
340340
// it against other server processes sharing the same file. See #4797.
341341
private mutationQueue: Promise<unknown> = Promise.resolve();
342342

343+
// Members of a stored entity or relation record that the schema does not
344+
// model (a createdAt written by an older version or another client, say),
345+
// keyed by the parsed object loadGraphWithUnreadable made from that record.
346+
// saveGraph writes them back, so a write keeps every record it did not
347+
// delete exactly as it found it: that is what makes create_entities,
348+
// create_relations and add_observations additive, as their
349+
// destructiveHint: false declares (#5058). A WeakMap rather than a property
350+
// on the object, so the extras never reach a tool result or read_graph,
351+
// whose output schemas do not allow them, and are collected with the graph
352+
// they came from. A deleted record is dropped from the graph and takes its
353+
// extras with it; one changed in place (add_observations,
354+
// delete_observations) keeps its object, so keeps them too.
355+
private extraFields = new WeakMap<object, Record<string, unknown>>();
356+
343357
private async withLock<T>(operation: () => Promise<T>): Promise<T> {
344358
const locked = () =>
345359
withFileLock(`${this.memoryFilePath}.lock`, operation, this.lockOptions);
@@ -393,6 +407,7 @@ export class KnowledgeGraphManager {
393407
if (record.type === "entity") {
394408
const parsed = EntitySchema.safeParse(item);
395409
if (parsed.success) {
410+
this.keepExtraFields(parsed.data, record, EntitySchema.shape);
396411
graph.entities.push(parsed.data);
397412
} else {
398413
unreadable.push(line);
@@ -406,6 +421,7 @@ export class KnowledgeGraphManager {
406421
} else if (record.type === "relation") {
407422
const parsed = RelationSchema.safeParse(item);
408423
if (parsed.success) {
424+
this.keepExtraFields(parsed.data, record, RelationSchema.shape);
409425
graph.relations.push(parsed.data);
410426
} else {
411427
unreadable.push(line);
@@ -434,17 +450,38 @@ export class KnowledgeGraphManager {
434450
}
435451
}
436452

453+
// Record the members of a valid stored record that are neither its type nor
454+
// a field of its schema, for saveGraph to write back. Object.fromEntries
455+
// defines each key as an own property, so even a "__proto__" member stays
456+
// data rather than setting the object's prototype.
457+
private keepExtraFields(
458+
parsed: object,
459+
record: Record<string, unknown>,
460+
shape: Record<string, unknown>,
461+
): void {
462+
const extras = Object.entries(record).filter(
463+
([key]) => key !== "type" && !Object.hasOwn(shape, key),
464+
);
465+
if (extras.length > 0) {
466+
this.extraFields.set(parsed, Object.fromEntries(extras));
467+
}
468+
}
469+
437470
private async saveGraph(
438471
graph: KnowledgeGraph,
439472
unreadable: string[],
440473
): Promise<void> {
474+
// The modelled fields come first and always hold the current values; a
475+
// record's extras (see extraFields) follow unchanged. A record created by
476+
// this write has none and is written exactly as before.
441477
const lines = [
442478
...graph.entities.map((e) =>
443479
JSON.stringify({
444480
type: "entity",
445481
name: e.name,
446482
entityType: e.entityType,
447483
observations: e.observations,
484+
...this.extraFields.get(e),
448485
}),
449486
),
450487
...graph.relations.map((r) =>
@@ -453,6 +490,7 @@ export class KnowledgeGraphManager {
453490
from: r.from,
454491
to: r.to,
455492
relationType: r.relationType,
493+
...this.extraFields.get(r),
456494
}),
457495
),
458496
...unreadable,

0 commit comments

Comments
 (0)