diff --git a/packages/api/package.json b/packages/api/package.json index c6493ae0..6f1ba566 100644 --- a/packages/api/package.json +++ b/packages/api/package.json @@ -41,6 +41,7 @@ "jsonwebtoken": "^9.0.2", "link-preview-js": "^3.2.0", "node-cron": "^4.2.1", + "node-diff3": "^3.2.0", "qrcode": "^1.5.4", "qrcode-terminal": "^0.12.0", "telegraf": "^4.15.0", diff --git a/packages/api/src/mcp/tools/artifact-handlers.test.ts b/packages/api/src/mcp/tools/artifact-handlers.test.ts new file mode 100644 index 00000000..2756b9c1 --- /dev/null +++ b/packages/api/src/mcp/tools/artifact-handlers.test.ts @@ -0,0 +1,406 @@ +/** + * Artifact Handler Tests + * + * Tests for three-way merge logic in update_artifact. + */ + +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { handleUpdateArtifact } from './artifact-handlers'; + +// ===================================================== +// MOCK SETUP +// ===================================================== + +vi.mock('../../services/user-resolver', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + resolveUserOrThrow: vi.fn().mockResolvedValue({ + user: { id: '00000000-0000-0000-0000-000000000001' }, + resolvedBy: 'userId', + }), + }; +}); + +vi.mock('../../utils/logger', () => ({ + logger: { + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + debug: vi.fn(), + }, +})); + +// ===================================================== +// HELPERS +// ===================================================== + +function createMockSupabase(overrides: { + artifact?: Record | null; + historyContent?: string | null; + historyError?: boolean; + casFailure?: boolean; +} = {}) { + const artifact = overrides.artifact ?? { + id: 'artifact-1', + uri: 'pcp://test/doc', + title: 'Test Doc', + content: 'Line 1\nLine 2\nLine 3\n', + version: 1, + metadata: {}, + collaborators: ['wren'], + created_by_agent_id: 'wren', + created_at: '2026-01-01', + updated_at: '2026-01-01', + }; + + const updatedArtifact = { ...artifact }; + const insertedHistory: Record[] = []; + + const mockFrom = vi.fn().mockImplementation((table: string) => { + if (table === 'artifacts') { + return { + select: vi.fn().mockReturnValue({ + eq: vi.fn().mockReturnValue({ + eq: vi.fn().mockReturnValue({ + single: vi.fn().mockResolvedValue({ + data: artifact, + error: artifact ? null : { message: 'Not found' }, + }), + }), + }), + }), + update: vi.fn().mockImplementation((updates: Record) => { + Object.assign(updatedArtifact, updates); + // CAS guard: .eq('id', ...).eq('version', ...).select().maybeSingle() + return { + eq: vi.fn().mockReturnValue({ + eq: vi.fn().mockReturnValue({ + select: vi.fn().mockReturnValue({ + maybeSingle: vi.fn().mockResolvedValue({ + data: overrides.casFailure ? null : updatedArtifact, + error: null, + }), + }), + }), + }), + }; + }), + }; + } + if (table === 'artifact_history') { + return { + select: vi.fn().mockReturnValue({ + eq: vi.fn().mockReturnValue({ + eq: vi.fn().mockReturnValue({ + single: vi.fn().mockResolvedValue({ + data: overrides.historyError ? null : { content: overrides.historyContent ?? artifact?.content }, + error: overrides.historyError ? { message: 'Not found' } : null, + }), + }), + }), + }), + insert: vi.fn().mockImplementation((entry: Record) => { + insertedHistory.push(entry); + return { error: null }; + }), + }; + } + return {}; + }); + + return { + supabase: { from: mockFrom }, + updatedArtifact, + insertedHistory, + mockFrom, + }; +} + +function createMockDataComposer(supabase: { from: ReturnType }) { + return { + getClient: () => supabase, + repositories: {}, + } as unknown as Parameters[1]; +} + +// ===================================================== +// TESTS +// ===================================================== + +describe('handleUpdateArtifact', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + describe('without baseVersion (backward compatible)', () => { + it('should update content with last-write-wins when baseVersion is omitted', async () => { + const { supabase, updatedArtifact } = createMockSupabase(); + const dataComposer = createMockDataComposer(supabase); + + const result = await handleUpdateArtifact( + { + userId: '00000000-0000-0000-0000-000000000001', + uri: 'pcp://test/doc', + content: 'Completely new content', + agentId: 'wren', + }, + dataComposer, + ); + + const parsed = JSON.parse(result.content[0].text); + expect(parsed.success).toBe(true); + expect(parsed.mergePerformed).toBeFalsy(); + expect(updatedArtifact.content).toBe('Completely new content'); + }); + }); + + describe('with baseVersion matching current', () => { + it('should update normally when baseVersion matches current version', async () => { + const { supabase, updatedArtifact } = createMockSupabase(); + const dataComposer = createMockDataComposer(supabase); + + const result = await handleUpdateArtifact( + { + userId: '00000000-0000-0000-0000-000000000001', + uri: 'pcp://test/doc', + content: 'Updated content', + baseVersion: 1, // matches current version + agentId: 'wren', + }, + dataComposer, + ); + + const parsed = JSON.parse(result.content[0].text); + expect(parsed.success).toBe(true); + expect(parsed.mergePerformed).toBeFalsy(); + expect(updatedArtifact.content).toBe('Updated content'); + }); + }); + + describe('three-way merge', () => { + it('should auto-merge when changes are in different sections', async () => { + // Base (version 1): three sections + const baseContent = '# Section A\nOriginal A content\n\n# Section B\nOriginal B content\n\n# Section C\nOriginal C content\n'; + + // Current (version 2): someone edited section B + const currentContent = '# Section A\nOriginal A content\n\n# Section B\nModified B content by Myra\n\n# Section C\nOriginal C content\n'; + + // Incoming: agent edited section C (based on version 1) + const incomingContent = '# Section A\nOriginal A content\n\n# Section B\nOriginal B content\n\n# Section C\nModified C content by Wren\n'; + + const { supabase, updatedArtifact, insertedHistory } = createMockSupabase({ + artifact: { + id: 'artifact-1', + uri: 'pcp://test/doc', + title: 'Test Doc', + content: currentContent, + version: 2, + metadata: {}, + collaborators: ['wren', 'myra'], + created_by_agent_id: 'wren', + created_at: '2026-01-01', + updated_at: '2026-01-01', + }, + historyContent: baseContent, + }); + const dataComposer = createMockDataComposer(supabase); + + const result = await handleUpdateArtifact( + { + userId: '00000000-0000-0000-0000-000000000001', + uri: 'pcp://test/doc', + content: incomingContent, + baseVersion: 1, // doesn't match current version 2 + agentId: 'wren', + }, + dataComposer, + ); + + const parsed = JSON.parse(result.content[0].text); + expect(parsed.success).toBe(true); + expect(parsed.mergePerformed).toBe(true); + expect(parsed.mergedFromBase).toBe(1); + + // The merged content should have Myra's B changes AND Wren's C changes + const merged = updatedArtifact.content as string; + expect(merged).toContain('Modified B content by Myra'); + expect(merged).toContain('Modified C content by Wren'); + + // History should record it as a merge + expect(insertedHistory[0].change_type).toBe('merge'); + }); + + it('should return conflict when both editors changed the same lines', async () => { + const baseContent = '# Title\nOriginal content here\n'; + const currentContent = '# Title\nMyra changed this line\n'; + const incomingContent = '# Title\nWren changed this line differently\n'; + + const { supabase } = createMockSupabase({ + artifact: { + id: 'artifact-1', + uri: 'pcp://test/doc', + title: 'Test Doc', + content: currentContent, + version: 2, + metadata: {}, + collaborators: ['wren', 'myra'], + created_by_agent_id: 'wren', + created_at: '2026-01-01', + updated_at: '2026-01-01', + }, + historyContent: baseContent, + }); + const dataComposer = createMockDataComposer(supabase); + + const result = await handleUpdateArtifact( + { + userId: '00000000-0000-0000-0000-000000000001', + uri: 'pcp://test/doc', + content: incomingContent, + baseVersion: 1, + agentId: 'wren', + }, + dataComposer, + ); + + const parsed = JSON.parse(result.content[0].text); + expect(parsed.success).toBe(false); + expect(parsed.conflict).toBe(true); + expect(parsed.currentVersion).toBe(2); + expect(parsed.baseVersion).toBe(1); + expect(parsed.conflicts).toBeDefined(); + expect(parsed.conflicts.length).toBeGreaterThan(0); + }); + + it('should handle false conflicts (both made same change) gracefully', async () => { + const baseContent = '# Title\nOriginal content\n\n# Footer\nFooter content\n'; + // Both editors made the exact same change + const currentContent = '# Title\nSame new content\n\n# Footer\nFooter content\n'; + const incomingContent = '# Title\nSame new content\n\n# Footer\nFooter content\n'; + + const { supabase } = createMockSupabase({ + artifact: { + id: 'artifact-1', + uri: 'pcp://test/doc', + title: 'Test Doc', + content: currentContent, + version: 2, + metadata: {}, + collaborators: ['wren'], + created_by_agent_id: 'wren', + created_at: '2026-01-01', + updated_at: '2026-01-01', + }, + historyContent: baseContent, + }); + const dataComposer = createMockDataComposer(supabase); + + const result = await handleUpdateArtifact( + { + userId: '00000000-0000-0000-0000-000000000001', + uri: 'pcp://test/doc', + content: incomingContent, + baseVersion: 1, + agentId: 'wren', + }, + dataComposer, + ); + + const parsed = JSON.parse(result.content[0].text); + // excludeFalseConflicts: true means identical changes merge cleanly + expect(parsed.success).toBe(true); + }); + + it('should throw when base version is not found in history', async () => { + const { supabase } = createMockSupabase({ + artifact: { + id: 'artifact-1', + uri: 'pcp://test/doc', + title: 'Test Doc', + content: 'Current content', + version: 5, + metadata: {}, + collaborators: ['wren'], + created_by_agent_id: 'wren', + created_at: '2026-01-01', + updated_at: '2026-01-01', + }, + historyError: true, + }); + const dataComposer = createMockDataComposer(supabase); + + await expect( + handleUpdateArtifact( + { + userId: '00000000-0000-0000-0000-000000000001', + uri: 'pcp://test/doc', + content: 'New content', + baseVersion: 1, + agentId: 'wren', + }, + dataComposer, + ), + ).rejects.toThrow('Cannot merge: base version 1 not found in history'); + }); + }); + + describe('CAS (compare-and-swap) guard', () => { + it('should return staleWrite conflict when another writer wins the race', async () => { + const { supabase } = createMockSupabase({ + casFailure: true, // simulate another writer incrementing version between read and write + }); + const dataComposer = createMockDataComposer(supabase); + + const result = await handleUpdateArtifact( + { + userId: '00000000-0000-0000-0000-000000000001', + uri: 'pcp://test/doc', + content: 'My update', + agentId: 'wren', + }, + dataComposer, + ); + + const parsed = JSON.parse(result.content[0].text); + expect(parsed.success).toBe(false); + expect(parsed.conflict).toBe(true); + expect(parsed.staleWrite).toBe(true); + }); + }); + + describe('non-content updates', () => { + it('should not trigger merge for metadata-only updates even with baseVersion', async () => { + const { supabase, updatedArtifact } = createMockSupabase({ + artifact: { + id: 'artifact-1', + uri: 'pcp://test/doc', + title: 'Test Doc', + content: 'Original content', + version: 3, + metadata: {}, + collaborators: ['wren'], + created_by_agent_id: 'wren', + created_at: '2026-01-01', + updated_at: '2026-01-01', + }, + }); + const dataComposer = createMockDataComposer(supabase); + + const result = await handleUpdateArtifact( + { + userId: '00000000-0000-0000-0000-000000000001', + uri: 'pcp://test/doc', + tags: ['new-tag'], + baseVersion: 1, // mismatches but no content change + agentId: 'wren', + }, + dataComposer, + ); + + const parsed = JSON.parse(result.content[0].text); + expect(parsed.success).toBe(true); + expect(parsed.mergePerformed).toBeFalsy(); + }); + }); +}); diff --git a/packages/api/src/mcp/tools/artifact-handlers.ts b/packages/api/src/mcp/tools/artifact-handlers.ts index ef4be4af..e3121fd5 100644 --- a/packages/api/src/mcp/tools/artifact-handlers.ts +++ b/packages/api/src/mcp/tools/artifact-handlers.ts @@ -6,6 +6,7 @@ */ import { z } from 'zod'; +import { merge as diff3Merge, diff3Merge as diff3MergeRegions } from 'node-diff3'; import type { DataComposer } from '../../data/composer'; import { resolveUserOrThrow, userIdentifierBaseSchema } from '../../services/user-resolver'; import { logger } from '../../utils/logger'; @@ -43,6 +44,7 @@ const updateArtifactSchema = userIdentifierBaseSchema.extend({ artifactId: z.string().uuid().optional().describe('ID of the artifact to update'), title: z.string().optional().describe('New title'), content: z.string().optional().describe('New content'), + baseVersion: z.number().int().optional().describe('Version this edit is based on. When provided, enables three-way merge: if the artifact has been modified since this version, the server will attempt to merge changes automatically. Omit for legacy last-write-wins behavior.'), agentId: z.string().optional().describe('Agent making the update'), collaborators: z.array(z.string()).optional().describe('Updated collaborator list'), tags: z.array(z.string()).optional().describe('Updated tags'), @@ -233,7 +235,7 @@ export async function handleUpdateArtifact( const parsed = updateArtifactSchema.parse(args); const resolved = await resolveUserOrThrow(parsed, dataComposer); - const { uri, artifactId, title, content, agentId, collaborators, tags, changeSummary } = parsed; + const { uri, artifactId, title, content, baseVersion, agentId, collaborators, tags, changeSummary } = parsed; if (!uri && !artifactId) { throw new Error('Must provide either uri or artifactId'); @@ -261,6 +263,96 @@ export async function handleUpdateArtifact( } } + // Three-way merge logic when content is being updated and baseVersion is provided + let finalContent = content; + let mergePerformed = false; + + if (content !== undefined && baseVersion !== undefined && baseVersion !== current.version) { + // Version mismatch — attempt three-way merge + logger.info('Version mismatch detected, attempting three-way merge', { + uri: current.uri, + baseVersion, + currentVersion: current.version, + agentId, + }); + + // Fetch the base version content from history + const { data: baseHistory, error: historyError } = await supabase + .from('artifact_history') + .select('content') + .eq('artifact_id', current.id) + .eq('version', baseVersion) + .single(); + + if (historyError || !baseHistory?.content) { + throw new Error( + `Cannot merge: base version ${baseVersion} not found in history. ` + + `Current version is ${current.version}. Re-read the artifact and try again.` + ); + } + + const baseContent = baseHistory.content; + const currentContent = current.content || ''; + const incomingContent = content; + + // Run three-way merge: merge(a, o, b) where a=incoming, o=base, b=current + // Use line-based splitting for markdown documents + const mergeOptions = { + excludeFalseConflicts: true, + stringSeparator: /\n/, + }; + const mergeResult = diff3Merge(incomingContent, baseContent, currentContent, mergeOptions); + + if (mergeResult.conflict) { + // Merge failed — return conflict details so the agent can resolve + const regions = diff3MergeRegions(incomingContent, baseContent, currentContent, mergeOptions); + + const conflicts = regions + .filter((r): r is { conflict: { a: string[]; b: string[]; o: string[] } } => 'conflict' in r && r.conflict !== undefined) + .map((r) => ({ + yours: r.conflict.a.join('\n'), + theirs: r.conflict.b.join('\n'), + original: r.conflict.o.join('\n'), + })); + + logger.warn('Three-way merge conflict', { + uri: current.uri, + baseVersion, + currentVersion: current.version, + conflictCount: conflicts.length, + agentId, + }); + + return { + content: [ + { + type: 'text' as const, + text: JSON.stringify({ + success: false, + conflict: true, + message: `Merge conflict: ${conflicts.length} conflicting region(s). Both you and another editor modified the same sections since version ${baseVersion}. Re-read the artifact (now at version ${current.version}) and retry your edit.`, + currentVersion: current.version, + baseVersion, + conflicts, + }), + }, + ], + }; + } + + // Clean merge — use the merged result + // node-diff3 splits on newlines and returns lines without separators, so rejoin with \n + finalContent = mergeResult.result.join('\n'); + mergePerformed = true; + + logger.info('Three-way merge succeeded', { + uri: current.uri, + baseVersion, + currentVersion: current.version, + agentId, + }); + } + const newVersion = (current.version ?? 0) + 1; // Build update object @@ -275,22 +367,56 @@ export async function handleUpdateArtifact( }; if (title !== undefined) updates.title = title; - if (content !== undefined) updates.content = content; + if (finalContent !== undefined) updates.content = finalContent; if (collaborators !== undefined) updates.collaborators = collaborators; if (tags !== undefined) updates.tags = tags; + // CAS (compare-and-swap) guard: only write if version hasn't changed since we read it. + // This prevents true race conditions where two concurrent writers both pass the + // merge check but then one silently overwrites the other. + const expectedVersion = current.version ?? 0; + const { data: updated, error: updateError } = await supabase .from('artifacts') .update(updates) .eq('id', current.id) + .eq('version', expectedVersion) .select() - .single(); + .maybeSingle(); if (updateError) { throw new Error(`Failed to update artifact: ${updateError.message}`); } + // No row updated — another writer won the race + if (!updated) { + logger.warn('CAS conflict: artifact version changed during update', { + uri: current.uri, + expectedVersion, + agentId, + }); + + return { + content: [ + { + type: 'text' as const, + text: JSON.stringify({ + success: false, + conflict: true, + staleWrite: true, + message: 'Artifact was modified by another writer during your update. Re-read the artifact and retry your edit with the new baseVersion.', + }), + }, + ], + }; + } + // Create history entry for this update + const changeType = mergePerformed ? 'merge' : 'update'; + const mergeSummary = mergePerformed + ? `Auto-merged with version ${current.version} (base: ${baseVersion}). ${changeSummary || ''}` + : changeSummary || null; + await supabase.from('artifact_history').insert({ artifact_id: current.id, version: newVersion, @@ -298,11 +424,11 @@ export async function handleUpdateArtifact( content: updated.content, changed_by_agent_id: agentId || null, changed_by_user_id: agentId ? null : resolved.user.id, - change_type: 'update', - change_summary: changeSummary || null, + change_type: changeType, + change_summary: mergeSummary, }); - logger.info('Artifact updated', { uri: current.uri, version: updated.version, agentId }); + logger.info('Artifact updated', { uri: current.uri, version: updated.version, agentId, mergePerformed }); return { content: [ @@ -310,7 +436,7 @@ export async function handleUpdateArtifact( type: 'text' as const, text: JSON.stringify({ success: true, - message: 'Artifact updated', + message: mergePerformed ? 'Artifact updated (auto-merged)' : 'Artifact updated', artifact: { id: updated.id, uri: updated.uri, @@ -319,6 +445,8 @@ export async function handleUpdateArtifact( updatedAt: updated.updated_at, }, previousVersion: current.version, + mergePerformed, + ...(mergePerformed ? { mergedFromBase: baseVersion } : {}), }), }, ], @@ -468,7 +596,7 @@ export const artifactToolDefinitions = [ { name: 'update_artifact', description: - 'Update an artifact. Automatically versions the content and tracks who made changes.', + 'Update an artifact. Supports three-way merge via baseVersion parameter to prevent data loss during concurrent edits. Pass baseVersion (from the version you read) to enable auto-merge.', schema: updateArtifactSchema, handler: handleUpdateArtifact, }, diff --git a/packages/api/src/mcp/tools/index.ts b/packages/api/src/mcp/tools/index.ts index 2a6cf1de..9e9ec754 100644 --- a/packages/api/src/mcp/tools/index.ts +++ b/packages/api/src/mcp/tools/index.ts @@ -2193,6 +2193,10 @@ User can be identified by ONE of: userId, email, phone, or platform + platformId { description: `Update an artifact. Automatically versions the content and tracks who made changes. +Supports three-way merge: pass baseVersion (the version you read before editing) to enable automatic merging when another agent has edited the artifact since you read it. If changes don't overlap, they merge cleanly. If they conflict, you'll get structured conflict details and should re-read and retry. + +Omit baseVersion for legacy last-write-wins behavior (not recommended for collaborative editing). + User can be identified by ONE of: userId, email, phone, or platform + platformId`, inputSchema: artifactToolDefinitions[2].schema, }, diff --git a/yarn.lock b/yarn.lock index e88ae65d..a06cf0b4 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1949,6 +1949,7 @@ __metadata: jsonwebtoken: "npm:^9.0.2" link-preview-js: "npm:^3.2.0" node-cron: "npm:^4.2.1" + node-diff3: "npm:^3.2.0" qrcode: "npm:^1.5.4" qrcode-terminal: "npm:^0.12.0" telegraf: "npm:^4.15.0" @@ -9940,6 +9941,13 @@ __metadata: languageName: node linkType: hard +"node-diff3@npm:^3.2.0": + version: 3.2.0 + resolution: "node-diff3@npm:3.2.0" + checksum: 10/594cdf658d38285bf529e73eadb4abaa73ed05b45ff65614a99fb9be68408918ae332a4e16ceb691855127329b4d6103c9e1ced218f1560f062fbc865fafb592 + languageName: node + linkType: hard + "node-domexception@npm:^1.0.0": version: 1.0.0 resolution: "node-domexception@npm:1.0.0"