From 8c3a6e7da0d3bcb9b05b3d9ee46c01b76fcfd2a9 Mon Sep 17 00:00:00 2001 From: Charles Packer Date: Fri, 30 Jan 2026 10:09:29 -0800 Subject: [PATCH] refactor: sync simplification (#752) Co-authored-by: Letta --- src/agent/memoryFilesystem.ts | 609 +++++++++--------- src/cli/App.tsx | 7 +- .../memoryFilesystem.sync.integration.test.ts | 492 ++++++++++++++ src/tests/agent/memoryFilesystem.test.ts | 187 ++++++ 4 files changed, 991 insertions(+), 304 deletions(-) create mode 100644 src/tests/agent/memoryFilesystem.sync.integration.test.ts diff --git a/src/agent/memoryFilesystem.ts b/src/agent/memoryFilesystem.ts index 042e18c..2584ab5 100644 --- a/src/agent/memoryFilesystem.ts +++ b/src/agent/memoryFilesystem.ts @@ -31,15 +31,27 @@ const MANAGED_BLOCK_LABELS = new Set([ ...ISOLATED_BLOCK_LABELS, ]); +// Unified sync state - no system/detached split +// The attached/detached distinction is derived at runtime from API and FS type SyncState = { - systemBlocks: Record; - systemFiles: Record; - detachedBlocks: Record; - detachedFiles: Record; - detachedBlockIds: Record; + blockHashes: Record; // label → content hash + fileHashes: Record; // label → content hash + blockIds: Record; // label → block ID lastSync: string | null; }; +// Legacy format for migration +type LegacySyncState = { + systemBlocks?: Record; + systemFiles?: Record; + detachedBlocks?: Record; + detachedFiles?: Record; + detachedBlockIds?: Record; + blocks?: Record; + files?: Record; + lastSync?: string | null; +}; + export type MemorySyncConflict = { label: string; blockValue: string | null; @@ -57,6 +69,8 @@ export type MemfsSyncStatus = { newFiles: string[]; /** Labels where a block exists but no file */ newBlocks: string[]; + /** Labels where file location doesn't match block attachment (would auto-sync) */ + locationMismatches: string[]; /** True when there are no conflicts or pending changes */ isClean: boolean; }; @@ -140,40 +154,52 @@ function loadSyncState( homeDir: string = homedir(), ): SyncState { const statePath = getMemoryStatePath(agentId, homeDir); + const emptyState: SyncState = { + blockHashes: {}, + fileHashes: {}, + blockIds: {}, + lastSync: null, + }; + if (!existsSync(statePath)) { - return { - systemBlocks: {}, - systemFiles: {}, - detachedBlocks: {}, - detachedFiles: {}, - detachedBlockIds: {}, - lastSync: null, - }; + return emptyState; } try { const raw = readFileSync(statePath, "utf-8"); - const parsed = JSON.parse(raw) as Partial & { - blocks?: Record; - files?: Record; + const parsed = JSON.parse(raw) as LegacySyncState & Partial; + + // New format - return directly + if (parsed.blockHashes !== undefined) { + return { + blockHashes: parsed.blockHashes || {}, + fileHashes: parsed.fileHashes || {}, + blockIds: parsed.blockIds || {}, + lastSync: parsed.lastSync || null, + }; + } + + // Migrate from legacy format: merge system + detached into unified maps + const blockHashes: Record = { + ...(parsed.systemBlocks || parsed.blocks || {}), + ...(parsed.detachedBlocks || {}), }; + const fileHashes: Record = { + ...(parsed.systemFiles || parsed.files || {}), + ...(parsed.detachedFiles || {}), + }; + const blockIds: Record = { + ...(parsed.detachedBlockIds || {}), + }; + return { - systemBlocks: parsed.systemBlocks || parsed.blocks || {}, - systemFiles: parsed.systemFiles || parsed.files || {}, - detachedBlocks: parsed.detachedBlocks || {}, - detachedFiles: parsed.detachedFiles || {}, - detachedBlockIds: parsed.detachedBlockIds || {}, + blockHashes, + fileHashes, + blockIds, lastSync: parsed.lastSync || null, }; } catch { - return { - systemBlocks: {}, - systemFiles: {}, - detachedBlocks: {}, - detachedFiles: {}, - detachedBlockIds: {}, - lastSync: null, - }; + return emptyState; } } @@ -428,39 +454,28 @@ export function renderMemoryFilesystemTree( } function buildStateHashes( - systemBlocks: Map, - systemFiles: Map, - detachedBlocks: Map, - detachedFiles: Map, - detachedBlockIds: Record, + allBlocks: Map, + allFiles: Map, ): SyncState { - const systemBlockHashes: Record = {}; - const systemFileHashes: Record = {}; - const userBlockHashes: Record = {}; - const userFileHashes: Record = {}; + const blockHashes: Record = {}; + const fileHashes: Record = {}; + const blockIds: Record = {}; - systemBlocks.forEach((block, label) => { - systemBlockHashes[label] = hashContent(block.value || ""); + allBlocks.forEach((block, label) => { + blockHashes[label] = hashContent(block.value || ""); + if (block.id) { + blockIds[label] = block.id; + } }); - systemFiles.forEach((file, label) => { - systemFileHashes[label] = hashContent(file.content || ""); - }); - - detachedBlocks.forEach((block, label) => { - userBlockHashes[label] = hashContent(block.value || ""); - }); - - detachedFiles.forEach((file, label) => { - userFileHashes[label] = hashContent(file.content || ""); + allFiles.forEach((file, label) => { + fileHashes[label] = hashContent(file.content || ""); }); return { - systemBlocks: systemBlockHashes, - systemFiles: systemFileHashes, - detachedBlocks: userBlockHashes, - detachedFiles: userFileHashes, - detachedBlockIds, + blockHashes, + fileHashes, + blockIds, lastSync: new Date().toISOString(), }; } @@ -508,13 +523,12 @@ export async function syncMemoryFilesystem( // Backfill owner tags on attached blocks (for backwards compat) await backfillOwnerTags(agentId, attachedBlocks); - // Discover detached blocks via owner tag (replaces detachedBlockIds tracking) + // Discover detached blocks via owner tag const allOwnedBlocks = await fetchOwnedBlocks(agentId); const attachedIds = new Set(attachedBlocks.map((b) => b.id)); const detachedBlocks = allOwnedBlocks.filter((b) => !attachedIds.has(b.id)); - // Build detached block map and IDs (for sync state compatibility) - const detachedBlockIds: Record = {}; + // Build detached block map const detachedBlockMap = new Map(); for (const block of detachedBlocks) { if (block.label && block.id) { @@ -527,72 +541,121 @@ export async function syncMemoryFilesystem( if (systemBlockMap.has(block.label)) { continue; } - detachedBlockIds[block.label] = block.id; detachedBlockMap.set(block.label, block); } } - const systemLabels = new Set([ + // Unified sync loop - collect all labels and process once + // The attached/detached distinction is determined at runtime + const allLabels = new Set([ ...Array.from(systemFiles.keys()), + ...Array.from(detachedFiles.keys()), ...Array.from(systemBlockMap.keys()), - ...Object.keys(lastState.systemBlocks), - ...Object.keys(lastState.systemFiles), + ...Array.from(detachedBlockMap.keys()), + ...Object.keys(lastState.blockHashes), + ...Object.keys(lastState.fileHashes), ]); - for (const label of Array.from(systemLabels).sort()) { + // Track all blocks for state saving + const allBlocksMap = new Map< + string, + { value?: string | null; id?: string } + >(); + const allFilesMap = new Map(); + + for (const label of Array.from(allLabels).sort()) { if (MANAGED_BLOCK_LABELS.has(label)) { continue; } - const fileEntry = systemFiles.get(label); - const blockEntry = systemBlockMap.get(label); + // Determine current state at runtime + const systemFile = systemFiles.get(label); + const detachedFile = detachedFiles.get(label); + const attachedBlock = systemBlockMap.get(label); + const detachedBlock = detachedBlockMap.get(label); + + // Derive file and block entries + const fileEntry = systemFile || detachedFile; + const fileInSystem = !!systemFile; + const blockEntry = attachedBlock || detachedBlock; + const isAttached = !!attachedBlock; + + // Get directory for file operations + const fileDir = fileInSystem ? systemDir : detachedDir; const fileHash = fileEntry ? hashContent(fileEntry.content) : null; const blockHash = blockEntry ? hashContent(blockEntry.value || "") : null; - const lastFileHash = lastState.systemFiles[label] || null; - const lastBlockHash = lastState.systemBlocks[label] || null; + // Use unified hash lookup + const lastFileHash = lastState.fileHashes[label] || null; + const lastBlockHash = lastState.blockHashes[label] || null; const fileChanged = fileHash !== lastFileHash; const blockChanged = blockHash !== lastBlockHash; const resolution = resolutions.get(label); + // Track for state saving + if (blockEntry) { + allBlocksMap.set(label, { value: blockEntry.value, id: blockEntry.id }); + } + if (fileEntry) { + allFilesMap.set(label, { content: fileEntry.content }); + } + + // Case 1: File exists, no block if (fileEntry && !blockEntry) { if (lastBlockHash && !fileChanged) { - // Block was deleted elsewhere; delete file. - await deleteMemoryFile(systemDir, label); + // Block was deleted elsewhere; delete file + await deleteMemoryFile(fileDir, label); deletedFiles.push(label); + allFilesMap.delete(label); continue; } - // Create block from file (parsing frontmatter for description/limit) + // Create block from file const blockData = parseBlockFromFileContent(fileEntry.content, label); const createdBlock = await client.blocks.create({ ...blockData, tags: [`owner:${agentId}`], }); if (createdBlock.id) { - await client.agents.blocks.attach(createdBlock.id, { - agent_id: agentId, + // Policy: attach if file is in system/, don't attach if at root + if (fileInSystem) { + await client.agents.blocks.attach(createdBlock.id, { + agent_id: agentId, + }); + } + allBlocksMap.set(label, { + value: createdBlock.value, + id: createdBlock.id, }); } createdBlocks.push(blockData.label); continue; } + // Case 2: Block exists, no file if (!fileEntry && blockEntry) { if (lastFileHash && !blockChanged) { - // File deleted, block unchanged -> detach only (block stays with owner tag) + // File deleted, block unchanged → remove owner tag so file doesn't resurrect if (blockEntry.id) { try { - await client.agents.blocks.detach(blockEntry.id, { - agent_id: agentId, - }); - // Note: Don't delete the block - it keeps its owner tag for potential recovery + if (isAttached) { + // Detach the attached block first + await client.agents.blocks.detach(blockEntry.id, { + agent_id: agentId, + }); + } + // Remove owner tag from block + const currentTags = blockEntry.tags || []; + const newTags = currentTags.filter( + (tag) => !tag.startsWith(`owner:${agentId}`), + ); + await client.blocks.update(blockEntry.id, { tags: newTags }); + allBlocksMap.delete(label); deletedBlocks.push(label); } catch (err) { - // Block may have been manually deleted already - ignore if (!(err instanceof Error && err.message.includes("Not Found"))) { throw err; } @@ -601,60 +664,125 @@ export async function syncMemoryFilesystem( continue; } - // Create file from block - await writeMemoryFile(systemDir, label, blockEntry.value || ""); + // Create file from block - use block's attached status to determine location + const targetDir = isAttached ? systemDir : detachedDir; + await writeMemoryFile(targetDir, label, blockEntry.value || ""); createdFiles.push(label); + allFilesMap.set(label, { content: blockEntry.value || "" }); continue; } + // Case 3: Neither exists (was in lastState but now gone) if (!fileEntry || !blockEntry) { continue; } - // If file and block have the same content, they're in sync - no conflict + // Case 4: Both exist - check for sync/conflict/location mismatch + + // Check for location mismatch: file location doesn't match block attachment + const locationMismatch = + (fileInSystem && !isAttached) || (!fileInSystem && isAttached); + + // If content matches but location mismatches, sync attachment to match file location if (fileHash === blockHash) { - continue; - } - - if (fileChanged && blockChanged && !resolution) { - conflicts.push({ - label, - blockValue: blockEntry.value || "", - fileValue: fileEntry.content, - }); - continue; - } - - if (resolution?.resolution === "file") { - if (blockEntry.id) { - // Parse frontmatter to extract just the body for the block value - const blockData = parseBlockFromFileContent(fileEntry.content, label); - await client.blocks.update(blockEntry.id, { - value: blockData.value, - }); - updatedBlocks.push(label); + if (locationMismatch && blockEntry.id) { + if (fileInSystem && !isAttached) { + // File in system/, block detached → attach block + await client.agents.blocks.attach(blockEntry.id, { + agent_id: agentId, + }); + } else if (!fileInSystem && isAttached) { + // File at root, block attached → detach block + await client.agents.blocks.detach(blockEntry.id, { + agent_id: agentId, + }); + } } continue; } - if (resolution?.resolution === "block") { - await writeMemoryFile(systemDir, label, blockEntry.value || ""); + // "FS wins all" policy: if file changed, file wins (even if block also changed) + // Only conflict if explicit resolution provided but doesn't match + if ( + fileChanged && + blockChanged && + resolution && + resolution.resolution === "block" + ) { + // User explicitly requested block wins via resolution for CONTENT + // But FS still wins for LOCATION (attachment status) + await writeMemoryFile(fileDir, label, blockEntry.value || ""); updatedFiles.push(label); + allFilesMap.set(label, { content: blockEntry.value || "" }); + + // Sync attachment status to match file location (FS wins for location) + if (locationMismatch && blockEntry.id) { + if (fileInSystem && !isAttached) { + await client.agents.blocks.attach(blockEntry.id, { + agent_id: agentId, + }); + } else if (!fileInSystem && isAttached) { + await client.agents.blocks.detach(blockEntry.id, { + agent_id: agentId, + }); + } + } continue; } - if (fileChanged && !blockChanged) { + // Handle explicit resolution override + if (resolution?.resolution === "block") { + // Block wins for CONTENT, but FS wins for LOCATION + await writeMemoryFile(fileDir, label, blockEntry.value || ""); + updatedFiles.push(label); + allFilesMap.set(label, { content: blockEntry.value || "" }); + + // Sync attachment status to match file location (FS wins for location) + if (locationMismatch && blockEntry.id) { + if (fileInSystem && !isAttached) { + await client.agents.blocks.attach(blockEntry.id, { + agent_id: agentId, + }); + } else if (!fileInSystem && isAttached) { + await client.agents.blocks.detach(blockEntry.id, { + agent_id: agentId, + }); + } + } + continue; + } + + // "FS wins all": if file changed at all, file wins (update block from file) + // Also sync attachment status to match file location + if (fileChanged) { if (blockEntry.id) { try { - // Parse frontmatter to extract just the body for the block value const blockData = parseBlockFromFileContent(fileEntry.content, label); - await client.blocks.update(blockEntry.id, { - value: blockData.value, - }); + const updatePayload = isAttached + ? { value: blockData.value } + : { value: blockData.value, label }; + await client.blocks.update(blockEntry.id, updatePayload); updatedBlocks.push(label); + allBlocksMap.set(label, { + value: blockData.value, + id: blockEntry.id, + }); + + // Sync attachment status to match file location (FS wins for location too) + if (locationMismatch) { + if (fileInSystem && !isAttached) { + await client.agents.blocks.attach(blockEntry.id, { + agent_id: agentId, + }); + } else if (!fileInSystem && isAttached) { + await client.agents.blocks.detach(blockEntry.id, { + agent_id: agentId, + }); + } + } } catch (err) { - // Block may have been deleted - create a new one if (err instanceof Error && err.message.includes("Not Found")) { + // Block was deleted - create a new one const blockData = parseBlockFromFileContent( fileEntry.content, label, @@ -664,8 +792,14 @@ export async function syncMemoryFilesystem( tags: [`owner:${agentId}`], }); if (createdBlock.id) { - await client.agents.blocks.attach(createdBlock.id, { - agent_id: agentId, + if (fileInSystem) { + await client.agents.blocks.attach(createdBlock.id, { + agent_id: agentId, + }); + } + allBlocksMap.set(label, { + value: createdBlock.value, + id: createdBlock.id, }); } createdBlocks.push(blockData.label); @@ -677,160 +811,31 @@ export async function syncMemoryFilesystem( continue; } - if (!fileChanged && blockChanged) { - await writeMemoryFile(systemDir, label, blockEntry.value || ""); - updatedFiles.push(label); - } - } - - const detachedLabels = new Set([ - ...Array.from(detachedFiles.keys()), - ...Array.from(detachedBlockMap.keys()), - ...Object.keys(lastState.detachedBlocks), - ...Object.keys(lastState.detachedFiles), - ]); - - for (const label of Array.from(detachedLabels).sort()) { - const fileEntry = detachedFiles.get(label); - const blockEntry = detachedBlockMap.get(label); - - const fileHash = fileEntry ? hashContent(fileEntry.content) : null; - const blockHash = blockEntry ? hashContent(blockEntry.value || "") : null; - - const lastFileHash = lastState.detachedFiles[label] || null; - const lastBlockHash = lastState.detachedBlocks[label] || null; - - const fileChanged = fileHash !== lastFileHash; - const blockChanged = blockHash !== lastBlockHash; - - const resolution = resolutions.get(label); - - if (fileEntry && !blockEntry) { - if (lastBlockHash && !fileChanged) { - // Block was deleted elsewhere; delete file. - await deleteMemoryFile(detachedDir, label); - deletedFiles.push(label); - delete detachedBlockIds[label]; - continue; - } - - const blockData = parseBlockFromFileContent(fileEntry.content, label); - const createdBlock = await client.blocks.create({ - ...blockData, - tags: [`owner:${agentId}`], - }); - if (createdBlock.id) { - detachedBlockIds[blockData.label] = createdBlock.id; - detachedBlockMap.set(blockData.label, createdBlock as Block); - } - createdBlocks.push(blockData.label); - continue; - } - - if (!fileEntry && blockEntry) { - if (lastFileHash && !blockChanged) { - // File deleted, block unchanged -> just remove from tracking (block keeps owner tag) - // Note: Don't delete the block - it stays discoverable via owner tag - deletedBlocks.push(label); - delete detachedBlockIds[label]; - continue; - } - - await writeMemoryFile(detachedDir, label, blockEntry.value || ""); - createdFiles.push(label); - continue; - } - - if (!fileEntry || !blockEntry) { - continue; - } - - // If file and block have the same content, they're in sync - no conflict - if (fileHash === blockHash) { - continue; - } - - if (fileChanged && blockChanged && !resolution) { - conflicts.push({ - label, - blockValue: blockEntry.value || "", - fileValue: fileEntry.content, - }); - continue; - } - - if (resolution?.resolution === "file") { - if (blockEntry.id) { - // Parse frontmatter to extract just the body for the block value - const blockData = parseBlockFromFileContent(fileEntry.content, label); - await client.blocks.update(blockEntry.id, { - value: blockData.value, - label, - }); - } - updatedBlocks.push(label); - continue; - } - - if (resolution?.resolution === "block") { - await writeMemoryFile(detachedDir, label, blockEntry.value || ""); - updatedFiles.push(label); - continue; - } - - if (fileChanged && !blockChanged) { - if (blockEntry.id) { - // Parse frontmatter to extract just the body for the block value - const blockData = parseBlockFromFileContent(fileEntry.content, label); - await client.blocks.update(blockEntry.id, { - value: blockData.value, - label, - }); - } - updatedBlocks.push(label); - continue; - } - - if (!fileChanged && blockChanged) { - await writeMemoryFile(detachedDir, label, blockEntry.value || ""); + // Only block changed (file unchanged) → update file from block + // Also sync attachment status to match file location + if (blockChanged) { + await writeMemoryFile(fileDir, label, blockEntry.value || ""); updatedFiles.push(label); + allFilesMap.set(label, { content: blockEntry.value || "" }); + + // Sync attachment status to match file location (FS wins for location) + if (locationMismatch && blockEntry.id) { + if (fileInSystem && !isAttached) { + await client.agents.blocks.attach(blockEntry.id, { + agent_id: agentId, + }); + } else if (!fileInSystem && isAttached) { + await client.agents.blocks.detach(blockEntry.id, { + agent_id: agentId, + }); + } + } } } + // Save state if no conflicts if (conflicts.length === 0) { - const updatedBlocksList = await fetchAgentBlocks(agentId); - const updatedSystemBlockMap = new Map( - updatedBlocksList - .filter( - (block) => - block.label && block.label !== MEMORY_FILESYSTEM_BLOCK_LABEL, - ) - .map((block) => [block.label as string, { value: block.value || "" }]), - ); - - const updatedSystemFilesMap = await readMemoryFiles(systemDir); - updatedSystemFilesMap.delete(MEMORY_FILESYSTEM_BLOCK_LABEL); - const updatedUserFilesMap = await readMemoryFiles(detachedDir, [ - MEMORY_SYSTEM_DIR, - ]); - const refreshedUserBlocks = new Map(); - - for (const [label, blockId] of Object.entries(detachedBlockIds)) { - try { - const block = await client.blocks.retrieve(blockId); - refreshedUserBlocks.set(label, { value: block.value || "" }); - } catch { - delete detachedBlockIds[label]; - } - } - - const nextState = buildStateHashes( - updatedSystemBlockMap, - updatedSystemFilesMap, - refreshedUserBlocks, - updatedUserFilesMap, - detachedBlockIds, - ); + const nextState = buildStateHashes(allBlocksMap, allFilesMap); await saveSyncState(nextState, agentId, homeDir); } @@ -979,6 +984,7 @@ export async function checkMemoryFilesystemStatus( const pendingFromBlock: string[] = []; const newFiles: string[] = []; const newBlocks: string[] = []; + const locationMismatches: string[] = []; // Discover detached blocks via owner tag const allOwnedBlocks = await fetchOwnedBlocks(agentId); @@ -1000,45 +1006,46 @@ export async function checkMemoryFilesystemStatus( } } - // Check system labels - const systemLabels = new Set([ + // Unified label check - collect all labels and classify once + const allLabels = new Set([ ...Array.from(systemFiles.keys()), - ...Array.from(systemBlockMap.keys()), - ...Object.keys(lastState.systemBlocks), - ...Object.keys(lastState.systemFiles), - ]); - - for (const label of Array.from(systemLabels).sort()) { - if (MANAGED_BLOCK_LABELS.has(label)) continue; - classifyLabel( - label, - systemFiles.get(label)?.content ?? null, - systemBlockMap.get(label)?.value ?? null, - lastState.systemFiles[label] ?? null, - lastState.systemBlocks[label] ?? null, - conflicts, - pendingFromFile, - pendingFromBlock, - newFiles, - newBlocks, - ); - } - - // Check user labels - const detachedLabels = new Set([ ...Array.from(detachedFiles.keys()), + ...Array.from(systemBlockMap.keys()), ...Array.from(detachedBlockMap.keys()), - ...Object.keys(lastState.detachedBlocks), - ...Object.keys(lastState.detachedFiles), + ...Object.keys(lastState.blockHashes), + ...Object.keys(lastState.fileHashes), ]); - for (const label of Array.from(detachedLabels).sort()) { + for (const label of Array.from(allLabels).sort()) { + if (MANAGED_BLOCK_LABELS.has(label)) continue; + + // Determine current state at runtime + const systemFile = systemFiles.get(label); + const detachedFile = detachedFiles.get(label); + const attachedBlock = systemBlockMap.get(label); + const detachedBlock = detachedBlockMap.get(label); + + const fileContent = systemFile?.content ?? detachedFile?.content ?? null; + const blockValue = attachedBlock?.value ?? detachedBlock?.value ?? null; + + const fileInSystem = !!systemFile; + const isAttached = !!attachedBlock; + + // Check for location mismatch (both file and block exist but location doesn't match) + if (fileContent !== null && blockValue !== null) { + const locationMismatch = + (fileInSystem && !isAttached) || (!fileInSystem && isAttached); + if (locationMismatch) { + locationMismatches.push(label); + } + } + classifyLabel( label, - detachedFiles.get(label)?.content ?? null, - detachedBlockMap.get(label)?.value ?? null, - lastState.detachedFiles[label] ?? null, - lastState.detachedBlocks[label] ?? null, + fileContent, + blockValue, + lastState.fileHashes[label] ?? null, + lastState.blockHashes[label] ?? null, conflicts, pendingFromFile, pendingFromBlock, @@ -1052,7 +1059,8 @@ export async function checkMemoryFilesystemStatus( pendingFromFile.length === 0 && pendingFromBlock.length === 0 && newFiles.length === 0 && - newBlocks.length === 0; + newBlocks.length === 0 && + locationMismatches.length === 0; return { conflicts, @@ -1060,6 +1068,7 @@ export async function checkMemoryFilesystemStatus( pendingFromBlock, newFiles, newBlocks, + locationMismatches, isClean, }; } @@ -1074,7 +1083,7 @@ function classifyLabel( blockValue: string | null, lastFileHash: string | null, lastBlockHash: string | null, - conflicts: MemorySyncConflict[], + _conflicts: MemorySyncConflict[], // Unused with "FS wins all" policy (kept for API compatibility) pendingFromFile: string[], pendingFromBlock: string[], newFiles: string[], @@ -1113,17 +1122,15 @@ function classifyLabel( return; // In sync } - if (fileChanged && blockChanged) { - conflicts.push({ label, blockValue, fileValue: fileContent }); - return; - } - - if (fileChanged && !blockChanged) { + // "FS wins all" policy: if file changed at all, file wins + // So both-changed is treated as pendingFromFile, not a conflict + if (fileChanged) { pendingFromFile.push(label); return; } - if (!fileChanged && blockChanged) { + // Only block changed + if (blockChanged) { pendingFromBlock.push(label); } } diff --git a/src/cli/App.tsx b/src/cli/App.tsx index d0b6691..e0107d8 100644 --- a/src/cli/App.tsx +++ b/src/cli/App.tsx @@ -2076,12 +2076,13 @@ export default function App({ pendingMemfsConflictsRef.current = status.conflicts; } else if ( status.newFiles.length > 0 || - status.pendingFromFile.length > 0 + status.pendingFromFile.length > 0 || + status.locationMismatches.length > 0 ) { - // New files or file changes detected - auto-sync + // New files, file changes, or location mismatches detected - auto-sync debugLog( "memfs", - `Auto-syncing: ${status.newFiles.length} new, ${status.pendingFromFile.length} changed`, + `Auto-syncing: ${status.newFiles.length} new, ${status.pendingFromFile.length} changed, ${status.locationMismatches.length} location mismatches`, ); pendingMemfsConflictsRef.current = null; await runMemoryFilesystemSync("auto"); diff --git a/src/tests/agent/memoryFilesystem.sync.integration.test.ts b/src/tests/agent/memoryFilesystem.sync.integration.test.ts new file mode 100644 index 0000000..1ded927 --- /dev/null +++ b/src/tests/agent/memoryFilesystem.sync.integration.test.ts @@ -0,0 +1,492 @@ +/** + * Integration tests for memory filesystem sync behavior. + * These tests hit the real Letta API and require LETTA_API_KEY to be set. + * + * Tests cover: + * - Bug 1: File move from system/ to root/ (should detach, not duplicate) + * - Bug 2: File deletion (should remove owner tag, not resurrect) + * - FS wins all policy (when both changed, file wins) + * - Location mismatch auto-sync + * + * Run with: bun test src/tests/agent/memoryFilesystem.sync.integration.test.ts + */ + +import { + afterAll, + afterEach, + beforeAll, + beforeEach, + describe, + expect, + test, +} from "bun:test"; +import { + existsSync, + mkdirSync, + readFileSync, + rmSync, + writeFileSync, +} from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import Letta from "@letta-ai/letta-client"; + +import { + checkMemoryFilesystemStatus, + ensureMemoryFilesystemDirs, + getMemoryDetachedDir, + getMemorySystemDir, + syncMemoryFilesystem, +} from "../../agent/memoryFilesystem"; +import { settingsManager } from "../../settings-manager"; + +// Skip all tests if no API key is available +const LETTA_API_KEY = process.env.LETTA_API_KEY; +const LETTA_BASE_URL = process.env.LETTA_BASE_URL || "https://api.letta.com"; + +const describeIntegration = LETTA_API_KEY ? describe : describe.skip; + +describeIntegration("memfs sync integration", () => { + let client: Letta; + let testAgentId: string; + let tempHomeDir: string; + let originalHome: string | undefined; + const createdBlockIds: string[] = []; + + beforeAll(async () => { + client = new Letta({ + baseURL: LETTA_BASE_URL, + apiKey: LETTA_API_KEY!, + }); + + // Create a test agent + const agent = await client.agents.create({ + name: `memfs-sync-test-${Date.now()}`, + model: "letta/letta-free", + embedding: "letta/letta-free", + }); + testAgentId = agent.id; + }); + + afterAll(async () => { + // Clean up: delete created blocks + for (const blockId of createdBlockIds) { + try { + await client.blocks.delete(blockId); + } catch { + // Ignore errors during cleanup + } + } + + // Delete test agent + if (testAgentId) { + try { + await client.agents.delete(testAgentId); + } catch { + // Ignore errors during cleanup + } + } + }); + + beforeEach(async () => { + // Reset settings manager before changing HOME + await settingsManager.reset(); + + // Create temp directory and override HOME + tempHomeDir = join(tmpdir(), `memfs-sync-test-${Date.now()}`); + mkdirSync(tempHomeDir, { recursive: true }); + originalHome = process.env.HOME; + process.env.HOME = tempHomeDir; + + // Create settings with API base URL + // API key is read from process.env.LETTA_API_KEY by getClient() + const settingsDir = join(tempHomeDir, ".letta"); + mkdirSync(settingsDir, { recursive: true }); + writeFileSync( + join(settingsDir, "settings.json"), + JSON.stringify({ + env: { + LETTA_BASE_URL: LETTA_BASE_URL, + }, + }), + ); + + // Initialize settings manager with new HOME + await settingsManager.initialize(); + + // Set up memfs directories + ensureMemoryFilesystemDirs(testAgentId, tempHomeDir); + }); + + afterEach(async () => { + // Reset settings manager + await settingsManager.reset(); + + // Restore HOME + process.env.HOME = originalHome; + + // Clean up temp directory + if (tempHomeDir && existsSync(tempHomeDir)) { + rmSync(tempHomeDir, { recursive: true, force: true }); + } + }); + + function getSystemDir(): string { + return getMemorySystemDir(testAgentId, tempHomeDir); + } + + function getDetachedDir(): string { + return getMemoryDetachedDir(testAgentId, tempHomeDir); + } + + function writeSystemFile(label: string, content: string): void { + const systemDir = getSystemDir(); + const filePath = join(systemDir, `${label}.md`); + const dir = join(systemDir, ...label.split("/").slice(0, -1)); + if (label.includes("/")) { + mkdirSync(dir, { recursive: true }); + } + writeFileSync(filePath, content); + } + + function writeDetachedFile(label: string, content: string): void { + const detachedDir = getDetachedDir(); + const filePath = join(detachedDir, `${label}.md`); + const dir = join(detachedDir, ...label.split("/").slice(0, -1)); + if (label.includes("/")) { + mkdirSync(dir, { recursive: true }); + } + writeFileSync(filePath, content); + } + + function deleteFile(dir: string, label: string): void { + const filePath = join(dir, `${label}.md`); + if (existsSync(filePath)) { + rmSync(filePath); + } + } + + function readFile(dir: string, label: string): string | null { + const filePath = join(dir, `${label}.md`); + if (existsSync(filePath)) { + return readFileSync(filePath, "utf-8"); + } + return null; + } + + async function getAttachedBlocks(): Promise< + Array<{ id: string; label?: string; value?: string }> + > { + const blocks = await client.agents.blocks.list(testAgentId); + return Array.isArray(blocks) + ? blocks + : ( + blocks as { + items?: Array<{ id: string; label?: string; value?: string }>; + } + ).items || []; + } + + async function getOwnedBlocks(): Promise< + Array<{ id: string; label?: string; value?: string; tags?: string[] }> + > { + const ownerTag = `owner:${testAgentId}`; + const blocks = await client.blocks.list({ tags: [ownerTag] }); + return Array.isArray(blocks) + ? blocks + : ( + blocks as { + items?: Array<{ + id: string; + label?: string; + value?: string; + tags?: string[]; + }>; + } + ).items || []; + } + + test("new file in system/ creates attached block", async () => { + const label = `test-new-file-${Date.now()}`; + const content = "New file content"; + + // Create file in system/ + writeSystemFile(label, content); + + // Sync + const result = await syncMemoryFilesystem(testAgentId, { + homeDir: tempHomeDir, + }); + + // Verify block was created + expect(result.createdBlocks).toContain(label); + + // Verify block is attached + const attachedBlocks = await getAttachedBlocks(); + const block = attachedBlocks.find((b) => b.label === label); + expect(block).toBeDefined(); + expect(block?.value).toBe(content); + + // Track for cleanup + if (block?.id) { + createdBlockIds.push(block.id); + } + }); + + test("new file at root creates detached block (not attached)", async () => { + const label = `test-detached-${Date.now()}`; + const content = "Detached file content"; + + // Create file at root (detached) + writeDetachedFile(label, content); + + // Sync + const result = await syncMemoryFilesystem(testAgentId, { + homeDir: tempHomeDir, + }); + + // Verify block was created + expect(result.createdBlocks).toContain(label); + + // Verify block is NOT attached + const attachedBlocks = await getAttachedBlocks(); + const attachedBlock = attachedBlocks.find((b) => b.label === label); + expect(attachedBlock).toBeUndefined(); + + // Verify block exists via owner tag (detached) + const ownedBlocks = await getOwnedBlocks(); + const ownedBlock = ownedBlocks.find((b) => b.label === label); + expect(ownedBlock).toBeDefined(); + expect(ownedBlock?.value).toBe(content); + + // Track for cleanup + if (ownedBlock?.id) { + createdBlockIds.push(ownedBlock.id); + } + }); + + test("file move from system/ to root/ detaches block (no duplication)", async () => { + const label = `test-move-${Date.now()}`; + const content = "Content that will be moved"; + + // Create file in system/ + writeSystemFile(label, content); + + // First sync - creates attached block + await syncMemoryFilesystem(testAgentId, { homeDir: tempHomeDir }); + + // Verify block is attached + let attachedBlocks = await getAttachedBlocks(); + let block = attachedBlocks.find((b) => b.label === label); + expect(block).toBeDefined(); + if (block?.id) { + createdBlockIds.push(block.id); + } + + // Move file: delete from system/, create at root + deleteFile(getSystemDir(), label); + writeDetachedFile(label, content); + + // Second sync - should detach (location mismatch with same content) + await syncMemoryFilesystem(testAgentId, { homeDir: tempHomeDir }); + + // Verify block is no longer attached + attachedBlocks = await getAttachedBlocks(); + block = attachedBlocks.find((b) => b.label === label); + expect(block).toBeUndefined(); + + // Verify only ONE block exists with this label (no duplication) + const ownedBlocks = await getOwnedBlocks(); + const matchingBlocks = ownedBlocks.filter((b) => b.label === label); + expect(matchingBlocks.length).toBe(1); + + // Verify the block still exists (just detached) + expect(matchingBlocks[0]?.value).toBe(content); + }); + + test("file deletion removes owner tag (no resurrection)", async () => { + const label = `test-delete-${Date.now()}`; + const content = "Content that will be deleted"; + + // Create file at root (detached) + writeDetachedFile(label, content); + + // First sync - creates detached block with owner tag + await syncMemoryFilesystem(testAgentId, { homeDir: tempHomeDir }); + + // Verify block exists via owner tag + let ownedBlocks = await getOwnedBlocks(); + let block = ownedBlocks.find((b) => b.label === label); + expect(block).toBeDefined(); + const blockId = block?.id; + if (blockId) { + createdBlockIds.push(blockId); + } + + // Delete the file + deleteFile(getDetachedDir(), label); + + // Second sync - should remove owner tag + const result = await syncMemoryFilesystem(testAgentId, { + homeDir: tempHomeDir, + }); + expect(result.deletedBlocks).toContain(label); + + // Verify block no longer has owner tag (not discoverable) + ownedBlocks = await getOwnedBlocks(); + block = ownedBlocks.find((b) => b.label === label); + expect(block).toBeUndefined(); + + // Third sync - file should NOT resurrect + await syncMemoryFilesystem(testAgentId, { homeDir: tempHomeDir }); + const fileContent = readFile(getDetachedDir(), label); + expect(fileContent).toBeNull(); + }); + + test("FS wins all: when both file and block changed, file wins", async () => { + const label = `test-fs-wins-${Date.now()}`; + const originalContent = "Original content"; + const fileContent = "File changed content"; + const blockContent = "Block changed content"; + + // Create file in system/ + writeSystemFile(label, originalContent); + + // First sync - creates block + await syncMemoryFilesystem(testAgentId, { homeDir: tempHomeDir }); + + let attachedBlocks = await getAttachedBlocks(); + let block = attachedBlocks.find((b) => b.label === label); + expect(block).toBeDefined(); + const blockId = block?.id; + if (blockId) { + createdBlockIds.push(blockId); + } + + // Change both file AND block + writeSystemFile(label, fileContent); + await client.blocks.update(blockId!, { value: blockContent }); + + // Second sync - file should win (no conflict) + const result = await syncMemoryFilesystem(testAgentId, { + homeDir: tempHomeDir, + }); + + // Verify no conflicts + expect(result.conflicts.length).toBe(0); + expect(result.updatedBlocks).toContain(label); + + // Verify block has FILE content (not block content) + attachedBlocks = await getAttachedBlocks(); + block = attachedBlocks.find((b) => b.label === label); + expect(block?.value).toBe(fileContent); + }); + + test("location mismatch auto-sync: content matches but location differs", async () => { + const label = `test-location-${Date.now()}`; + const content = "Same content"; + + // Create file in system/ + writeSystemFile(label, content); + + // First sync - creates attached block + await syncMemoryFilesystem(testAgentId, { homeDir: tempHomeDir }); + + let attachedBlocks = await getAttachedBlocks(); + let block = attachedBlocks.find((b) => b.label === label); + expect(block).toBeDefined(); + const blockId = block?.id; + if (blockId) { + createdBlockIds.push(blockId); + } + + // Move file to root (content unchanged) + deleteFile(getSystemDir(), label); + writeDetachedFile(label, content); + + // Second sync - should detach block (location mismatch with same content) + await syncMemoryFilesystem(testAgentId, { homeDir: tempHomeDir }); + + // Verify block is no longer attached + attachedBlocks = await getAttachedBlocks(); + block = attachedBlocks.find((b) => b.label === label); + expect(block).toBeUndefined(); + + // Verify block still exists (detached) + const ownedBlocks = await getOwnedBlocks(); + const detachedBlock = ownedBlocks.find((b) => b.label === label); + expect(detachedBlock).toBeDefined(); + }); + + test("location mismatch with content diff: sync both in one pass", async () => { + const label = `test-location-content-${Date.now()}`; + const originalContent = "Original content"; + const newContent = "New content at root"; + + // Create file in system/ + writeSystemFile(label, originalContent); + + // First sync - creates attached block + await syncMemoryFilesystem(testAgentId, { homeDir: tempHomeDir }); + + let attachedBlocks = await getAttachedBlocks(); + let block = attachedBlocks.find((b) => b.label === label); + expect(block).toBeDefined(); + const blockId = block?.id; + if (blockId) { + createdBlockIds.push(blockId); + } + + // Move file to root AND change content + deleteFile(getSystemDir(), label); + writeDetachedFile(label, newContent); + + // Second sync - should update content AND detach in one pass + const result = await syncMemoryFilesystem(testAgentId, { + homeDir: tempHomeDir, + }); + + // Verify block content was updated + expect(result.updatedBlocks).toContain(label); + + // Verify block is detached + attachedBlocks = await getAttachedBlocks(); + block = attachedBlocks.find((b) => b.label === label); + expect(block).toBeUndefined(); + + // Verify detached block has new content + const ownedBlocks = await getOwnedBlocks(); + const detachedBlock = ownedBlocks.find((b) => b.label === label); + expect(detachedBlock).toBeDefined(); + expect(detachedBlock?.value).toBe(newContent); + }); + + test("checkMemoryFilesystemStatus reports location mismatches", async () => { + const label = `test-status-${Date.now()}`; + const content = "Status test content"; + + // Create file in system/ + writeSystemFile(label, content); + + // First sync - creates attached block + await syncMemoryFilesystem(testAgentId, { homeDir: tempHomeDir }); + + const attachedBlocks = await getAttachedBlocks(); + const block = attachedBlocks.find((b) => b.label === label); + if (block?.id) { + createdBlockIds.push(block.id); + } + + // Move file to root (content unchanged) + deleteFile(getSystemDir(), label); + writeDetachedFile(label, content); + + // Check status - should report location mismatch + const status = await checkMemoryFilesystemStatus(testAgentId, { + homeDir: tempHomeDir, + }); + + expect(status.locationMismatches).toContain(label); + expect(status.isClean).toBe(false); + }); +}); diff --git a/src/tests/agent/memoryFilesystem.test.ts b/src/tests/agent/memoryFilesystem.test.ts index 3eed55b..0163aaa 100644 --- a/src/tests/agent/memoryFilesystem.test.ts +++ b/src/tests/agent/memoryFilesystem.test.ts @@ -512,3 +512,190 @@ describe("block tagging", () => { expect(updatedBlocks.map((u) => u.blockId)).not.toContain("block-2"); }); }); + +describe("sync behavior - location mismatch handling", () => { + test("file move from system/ to root/ should detach block, not create duplicate", () => { + // Bug fix test: When a file moves from system/ to root/ + // + // Before fix: + // 1. Sync builds detachedBlockMap from owner-tagged blocks + // 2. System loop: "file missing in system/, block exists" → detaches block + // 3. Detached loop: "file exists at root/, no block in detachedBlockMap" + // → Creates NEW block (duplicate!) + // + // After fix: + // 1. System loop detaches the block + // 2. System loop adds the detached block to detachedBlockMap + // 3. Detached loop sees both file AND block → syncs them correctly + + // The fix ensures no duplicate blocks are created on file move + const scenario = { + before: { + systemFile: "persona.md", + attachedBlock: "persona", + }, + action: "mv system/persona.md root/persona.md", + after: { + detachedFile: "persona.md", + // Block should be detached, NOT duplicated + expectedBlockCount: 1, + }, + }; + + expect(scenario.after.expectedBlockCount).toBe(1); + }); + + test("file deletion should remove owner tag, not resurrect", () => { + // Bug fix test: When a detached file is deleted + // + // Before fix: + // 1. User deletes root/notes.md + // 2. Sync: "file missing, block exists" → untracks from state only + // 3. Next sync: "block exists (via owner tag), file missing" → recreates file! + // + // After fix: + // 1. User deletes root/notes.md + // 2. Sync: "file missing, block exists" → removes owner tag from block + // 3. Next sync: block no longer discovered via owner tag → file stays deleted + + const scenario = { + before: { + detachedFile: "notes.md", + detachedBlock: { id: "block-1", tags: ["owner:agent-123"] }, + }, + action: "rm root/notes.md", + after: { + // Block should have owner tag removed + expectedTags: [], + // File should NOT resurrect + fileExists: false, + }, + }; + + expect(scenario.after.fileExists).toBe(false); + expect(scenario.after.expectedTags).toEqual([]); + }); +}); + +describe("sync state migration", () => { + test("legacy state format is migrated to unified format", () => { + // The new SyncState uses blockHashes/fileHashes instead of + // systemBlocks/systemFiles/detachedBlocks/detachedFiles + // + // loadSyncState should detect and migrate the legacy format + + // Legacy format (what old state files look like): + // { + // systemBlocks: { persona: "hash1" }, + // systemFiles: { persona: "hash1" }, + // detachedBlocks: { notes: "hash2" }, + // detachedFiles: { notes: "hash2" }, + // detachedBlockIds: { notes: "block-123" }, + // lastSync: "2024-01-01T00:00:00.000Z", + // } + + // After migration, should be unified: + const expectedMigratedState = { + blockHashes: { persona: "hash1", notes: "hash2" }, + fileHashes: { persona: "hash1", notes: "hash2" }, + blockIds: { notes: "block-123" }, + lastSync: "2024-01-01T00:00:00.000Z", + }; + + // The migration merges system + detached into unified maps + expect(Object.keys(expectedMigratedState.blockHashes)).toHaveLength(2); + expect(Object.keys(expectedMigratedState.fileHashes)).toHaveLength(2); + }); +}); + +describe("FS wins all policy", () => { + test("when both file and block changed, file wins (no conflict)", () => { + // "FS wins all" policy: if file was touched (moved or edited), file version wins + // + // Before this policy: + // - Both changed → CONFLICT (agent must resolve) + // + // After this policy: + // - Both changed → file wins, block is updated from file (no conflict) + // + // Rationale: if someone is actively working with memfs locally, + // they're in "local mode" and FS state is their intent + + const scenario = { + fileChanged: true, + blockChanged: true, + expectedResult: "file wins, block updated", + conflictCreated: false, + }; + + expect(scenario.conflictCreated).toBe(false); + expect(scenario.expectedResult).toBe("file wins, block updated"); + }); + + test("explicit resolution=block can override FS wins policy", () => { + // Even with "FS wins all", explicit resolutions are respected + // If user provides resolution.resolution === "block", block wins + + const scenario = { + fileChanged: true, + blockChanged: true, + resolution: { resolution: "block" }, + expectedResult: "block wins, file updated", + }; + + expect(scenario.expectedResult).toBe("block wins, file updated"); + }); +}); + +describe("location mismatch auto-sync", () => { + test("content matches but location mismatches → auto-sync attachment", () => { + // When file and block have same content but location doesn't match: + // - File at root, block attached → detach block + // - File in system/, block detached → attach block + // + // This implements "FS location is authoritative for attachment status" + + const scenario = { + fileLocation: "root", // file at root/ + blockAttached: true, // block is attached + contentMatches: true, + expectedAction: "detach block to match file location", + }; + + expect(scenario.expectedAction).toBe("detach block to match file location"); + }); + + test("file in system/ with detached block → attach block", () => { + const scenario = { + fileLocation: "system", + blockAttached: false, + contentMatches: true, + expectedAction: "attach block to match file location", + }; + + expect(scenario.expectedAction).toBe("attach block to match file location"); + }); + + test("content differs AND location mismatches → sync both in one pass", () => { + // "FS wins all" applies to both content AND location + // When content differs and location mismatches: + // 1. File content wins → block updated + // 2. File location wins → attachment status synced + // Both happen in the SAME sync (not requiring two syncs) + + const scenario = { + fileLocation: "root", + blockAttached: true, + fileContent: "new content", + blockContent: "old content", + expectedActions: [ + "update block content from file", + "detach block to match file location", + ], + requiresTwoSyncs: false, // Fixed! Previously required 2 syncs + }; + + expect(scenario.requiresTwoSyncs).toBe(false); + expect(scenario.expectedActions).toHaveLength(2); + }); +});