From 018ccada2c96041d7e07e1f6399896e51642179d Mon Sep 17 00:00:00 2001 From: Matthew Valancy Date: Sat, 13 Jun 2026 10:49:20 -0700 Subject: [PATCH] Fix clone_graph: throws on most graphs + corrupts every edge type MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two real, AI-reachable bugs in the clone_graph MCP tool: 1. ParameterMissing(teamId): Neo4j does not persist null-valued properties, so a graph created without a teamId has NO teamId property. cloneGraph read it back as `undefined` and passed it straight to the driver, which rejects undefined param values — so clone threw for essentially every graph created via the API. Coalesce teamId (and type/isShared/settings defensively) to non-undefined values. 2. Edge types silently collapsed to DEPENDS_ON: the edge-clone query matched every relationship type but hard-coded `CREATE (a)-[:DEPENDS_ON ...]->(b)`, rewriting BLOCKS / RELATES_TO / CONTAINS / PART_OF into DEPENDS_ON on clone. Use apoc.create.relationship(newW, type(r), ...) to preserve the real type (APOC 5.x ships with the project's Neo4j). Adds a real-Neo4j contract case: clone a graph with BLOCKS + RELATES_TO edges and assert all nodes copy and both edge types survive. The existing mock unit tests passed through both bugs — they can't reproduce real-DB param/relationship-type behavior, which is exactly why the contract test exists. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../mcp-server/src/services/graph-service.ts | 23 +++++---- .../mcp-server/tests/neo4j-contract.test.ts | 50 +++++++++++++++++++ 2 files changed, 63 insertions(+), 10 deletions(-) diff --git a/packages/mcp-server/src/services/graph-service.ts b/packages/mcp-server/src/services/graph-service.ts index c9e74071..cf1f287f 100644 --- a/packages/mcp-server/src/services/graph-service.ts +++ b/packages/mcp-server/src/services/graph-service.ts @@ -3586,10 +3586,14 @@ export class GraphService { const newGraph = await tx.run(createGraphQuery, { newName: args.newName, description: `Cloned from: ${sourceGraph.name}`, - type: sourceGraph.type, - teamId: args.teamId || sourceGraph.teamId, - isShared: sourceGraph.isShared, - settings: sourceGraph.settings, + type: sourceGraph.type ?? 'PROJECT', + // Neo4j does not store null-valued properties, so a graph created + // without a teamId has NO teamId property — reading it back yields + // `undefined`, and the driver rejects an undefined param value with + // ParameterMissing. Coalesce to null so clone works for every graph. + teamId: args.teamId ?? sourceGraph.teamId ?? null, + isShared: sourceGraph.isShared ?? false, + settings: sourceGraph.settings ?? '{}', sourceGraphId: args.sourceGraphId }); @@ -3636,12 +3640,11 @@ export class GraphService { MATCH (sourceW)-[r:DEPENDS_ON|BLOCKS|RELATES_TO|CONTAINS|PART_OF]->(targetW:WorkItem)-[:BELONGS_TO]->(sourceG) MATCH (newG)<-[:BELONGS_TO]-(newTargetW:WorkItem) WHERE newTargetW.originalId = targetW.id - CREATE (newW)-[newR:DEPENDS_ON { - type: r.type, - weight: r.weight, - metadata: r.metadata - }]->(newTargetW) - RETURN count(newR) as edgeCount + // Preserve the real relationship type — the previous code + // hard-coded :DEPENDS_ON, silently rewriting every BLOCKS / + // RELATES_TO / CONTAINS / PART_OF edge into a DEPENDS_ON on clone. + CALL apoc.create.relationship(newW, type(r), { weight: r.weight, metadata: r.metadata }, newTargetW) YIELD rel + RETURN count(rel) as edgeCount `; const edgesResult = await tx.run(cloneEdgesQuery, { diff --git a/packages/mcp-server/tests/neo4j-contract.test.ts b/packages/mcp-server/tests/neo4j-contract.test.ts index f4db34a5..eadc6ff4 100644 --- a/packages/mcp-server/tests/neo4j-contract.test.ts +++ b/packages/mcp-server/tests/neo4j-contract.test.ts @@ -44,6 +44,9 @@ describe.skipIf(!RUN)('MCP GraphService — real Neo4j contract', () => { await session?.run('MATCH (n:WorkItem) WHERE n.id IN $ids DETACH DELETE n', { ids: createdNodes }); } if (createdGraphs.length) { + // Also remove any WorkItems that belong to these graphs (clone creates + // brand-new node ids we don't otherwise track). + await session?.run('MATCH (g:Graph) WHERE g.id IN $ids OPTIONAL MATCH (g)<-[:BELONGS_TO]-(w:WorkItem) DETACH DELETE w', { ids: createdGraphs }); await session?.run('MATCH (g:Graph) WHERE g.id IN $ids DETACH DELETE g', { ids: createdGraphs }); } } catch { /* ignore */ } @@ -130,6 +133,53 @@ describe.skipIf(!RUN)('MCP GraphService — real Neo4j contract', () => { expect(ctx.counts.byStatus.IN_PROGRESS, 'both items tallied under IN_PROGRESS').toBe(2); }); + it('cloneGraph copies nodes AND preserves each edge type (not all DEPENDS_ON)', async () => { + // Regressions this guards: + // 1. clone threw ParameterMissing(teamId) because Neo4j never stores a + // null teamId, so reading it back yields undefined. + // 2. clone hard-coded :DEPENDS_ON, silently rewriting BLOCKS/RELATES_TO/… + const src = parse(await svc.createGraph({ name: `Contract Clone Src ${Date.now()}`, type: 'PROJECT' } as any)); + const srcId = src.graph.id; + createdGraphs.push(srcId); + + const session = driver.session(); + const mk = async (title: string) => { + const id = parse(await svc.createNode({ title, type: 'TASK', status: 'PROPOSED' } as any)).node.id; + createdNodes.push(id); + await session.run('MATCH (w:WorkItem {id: $id}), (g:Graph {id: $gid}) MERGE (w)-[:BELONGS_TO]->(g)', { id, gid: srcId }); + return id; + }; + let a: string, b: string, c: string; + try { + a = await mk(`Clone A ${Date.now()}`); + b = await mk(`Clone B ${Date.now()}`); + c = await mk(`Clone C ${Date.now()}`); + } finally { + await session.close(); + } + await svc.createEdge({ source_id: a, target_id: b, type: 'BLOCKS' } as any); + await svc.createEdge({ source_id: b, target_id: c, type: 'RELATES_TO' } as any); + + const cloned = parse(await svc.cloneGraph({ sourceGraphId: srcId, newName: `Contract Clone Dst ${Date.now()}` } as any)); + const dstId = cloned.newGraph.id; + createdGraphs.push(dstId); + expect(cloned.newGraph.clonedNodes, 'all three nodes cloned').toBe(3); + expect(cloned.newGraph.clonedEdges, 'both edges cloned').toBe(2); + + // The cloned edges must keep their real types, not all become DEPENDS_ON + const s2 = driver.session(); + try { + const r = await s2.run( + 'MATCH (g:Graph {id: $gid})<-[:BELONGS_TO]-(:WorkItem)-[rel]->(:WorkItem) RETURN type(rel) AS t ORDER BY t', + { gid: dstId } + ); + const types = r.records.map((rec) => rec.get('t')).sort(); + expect(types, 'cloned relationship types are preserved').toEqual(['BLOCKS', 'RELATES_TO']); + } finally { + await s2.close(); + } + }); + it('browseGraph returns well-formed data over a real DB', async () => { const browsed = parse(await svc.browseGraph({ query_type: 'all_nodes', limit: 25 } as any)); const arr = browsed.nodes ?? browsed.results ?? browsed.workItems;