From bb6ce1f2c80539b6d3be1a4a582606f46e2f0c5a Mon Sep 17 00:00:00 2001 From: Charles Packer Date: Fri, 30 Jan 2026 18:08:56 -0800 Subject: [PATCH] fix(skill): patch up skill diffing code (#758) Co-authored-by: Letta --- src/agent/memory.ts | 7 +-- src/agent/memoryConstants.ts | 5 ++ src/agent/memoryFilesystem.ts | 18 ++++-- .../scripts/lib/frontmatter.ts | 47 +++++++++++++++ .../scripts/memfs-diff.ts | 49 ++------------- .../scripts/memfs-resolve.ts | 42 ++----------- .../scripts/memfs-status.ts | 59 +++---------------- .../syncing-memory-filesystem-scripts.test.ts | 47 +++++++++++++++ 8 files changed, 135 insertions(+), 139 deletions(-) create mode 100644 src/agent/memoryConstants.ts create mode 100644 src/skills/builtin/syncing-memory-filesystem/scripts/lib/frontmatter.ts create mode 100644 src/tests/skills/syncing-memory-filesystem-scripts.test.ts diff --git a/src/agent/memory.ts b/src/agent/memory.ts index 496430b..e9b24df 100644 --- a/src/agent/memory.ts +++ b/src/agent/memory.ts @@ -4,6 +4,7 @@ */ import type { CreateBlock } from "@letta-ai/letta-client/resources/blocks/blocks"; +import { READ_ONLY_BLOCK_LABELS } from "./memoryConstants"; import { MEMORY_PROMPTS } from "./promptAssets"; /** @@ -33,11 +34,7 @@ export type MemoryBlockLabel = (typeof MEMORY_BLOCK_LABELS)[number]; * Block labels that should be read-only (agent cannot modify via memory tools). * These blocks are managed by specific tools (e.g., Skill tool for skills/loaded_skills). */ -export const READ_ONLY_BLOCK_LABELS = [ - "skills", - "loaded_skills", - "memory_filesystem", -] as const; +export { READ_ONLY_BLOCK_LABELS }; /** * Block labels that should be isolated per-conversation. diff --git a/src/agent/memoryConstants.ts b/src/agent/memoryConstants.ts new file mode 100644 index 0000000..14c6fa5 --- /dev/null +++ b/src/agent/memoryConstants.ts @@ -0,0 +1,5 @@ +export const READ_ONLY_BLOCK_LABELS = [ + "skills", + "loaded_skills", + "memory_filesystem", +] as const; diff --git a/src/agent/memoryFilesystem.ts b/src/agent/memoryFilesystem.ts index 19abde6..b102a5e 100644 --- a/src/agent/memoryFilesystem.ts +++ b/src/agent/memoryFilesystem.ts @@ -673,6 +673,10 @@ export async function syncMemoryFilesystem( const fileInSystem = !!systemFile; const blockEntry = attachedBlock || detachedBlock; const isAttached = !!attachedBlock; + const isReadOnlyLabel = ( + READ_ONLY_BLOCK_LABELS as readonly string[] + ).includes(label); + const effectiveReadOnly = !!blockEntry?.read_only || isReadOnlyLabel; // Get directory for file operations const fileDir = fileInSystem ? systemDir : detachedDir; @@ -742,7 +746,7 @@ export async function syncMemoryFilesystem( // Case 2: Block exists, no file if (!fileEntry && blockEntry) { // Read-only blocks: never delete/un-tag. Always recreate file instead. - if (blockEntry.read_only) { + if (effectiveReadOnly) { const targetDir = isAttached ? systemDir : detachedDir; const fileContent = renderBlockToFileContent(blockEntry); await writeMemoryFile(targetDir, label, fileContent); @@ -818,7 +822,7 @@ export async function syncMemoryFilesystem( // Frontmatter-only change: update metadata even when body matches if (fileChanged) { // Read-only blocks: ignore local changes, overwrite file with API content - if (blockEntry.read_only) { + if (effectiveReadOnly) { const fileContent = renderBlockToFileContent(blockEntry); await writeMemoryFile(fileDir, label, fileContent); updatedFiles.push(label); @@ -910,7 +914,7 @@ export async function syncMemoryFilesystem( // Also sync attachment status to match file location if (fileChanged) { // Read-only blocks: ignore local changes, overwrite file with API content - if (blockEntry.read_only) { + if (effectiveReadOnly) { const fileContent = renderBlockToFileContent(blockEntry); await writeMemoryFile(fileDir, label, fileContent); updatedFiles.push(label); @@ -1215,7 +1219,8 @@ export async function checkMemoryFilesystemStatus( const fileContent = systemFile?.content ?? detachedFile?.content ?? null; const blockValue = attachedBlock?.value ?? detachedBlock?.value ?? null; const blockReadOnly = - attachedBlock?.read_only ?? detachedBlock?.read_only ?? false; + (attachedBlock?.read_only ?? detachedBlock?.read_only ?? false) || + (READ_ONLY_BLOCK_LABELS as readonly string[]).includes(label); const fileInSystem = !!systemFile; const isAttached = !!attachedBlock; @@ -1301,6 +1306,11 @@ function classifyLabel( } if (fileContent === null && blockValue !== null) { + if (blockReadOnly) { + // Read-only blocks: missing file should be recreated + pendingFromFile.push(label); + return; + } if (lastFileHash && !blockChanged) { // File was deleted, block unchanged — would delete block return; diff --git a/src/skills/builtin/syncing-memory-filesystem/scripts/lib/frontmatter.ts b/src/skills/builtin/syncing-memory-filesystem/scripts/lib/frontmatter.ts new file mode 100644 index 0000000..2bfb22a --- /dev/null +++ b/src/skills/builtin/syncing-memory-filesystem/scripts/lib/frontmatter.ts @@ -0,0 +1,47 @@ +import { createHash } from "node:crypto"; +import { READ_ONLY_BLOCK_LABELS } from "../../../../../agent/memoryConstants"; + +/** + * Read-only block labels. These are API-authoritative. + */ +export const READ_ONLY_LABELS = new Set( + READ_ONLY_BLOCK_LABELS as readonly string[], +); + +/** + * Parse MDX-style frontmatter from content. + * This is a copy of parseMdxFrontmatter from src/agent/memory.ts. + * The test ensures this stays in sync with the original. + */ +export function parseFrontmatter(content: string): { + frontmatter: Record; + body: string; +} { + const frontmatterRegex = /^---\n([\s\S]*?)\n---\n([\s\S]*)$/; + const match = content.match(frontmatterRegex); + + if (!match || !match[1] || !match[2]) { + return { frontmatter: {}, body: content }; + } + + const frontmatterText = match[1]; + const body = match[2]; + const frontmatter: Record = {}; + + // Parse YAML-like frontmatter (simple key: value pairs) + for (const line of frontmatterText.split("\n")) { + const colonIndex = line.indexOf(":"); + if (colonIndex > 0) { + const key = line.slice(0, colonIndex).trim(); + const value = line.slice(colonIndex + 1).trim(); + frontmatter[key] = value; + } + } + + return { frontmatter, body: body.trim() }; +} + +export function hashFileBody(content: string): string { + const { body } = parseFrontmatter(content); + return createHash("sha256").update(body).digest("hex"); +} diff --git a/src/skills/builtin/syncing-memory-filesystem/scripts/memfs-diff.ts b/src/skills/builtin/syncing-memory-filesystem/scripts/memfs-diff.ts index 1759fbb..3a290ac 100644 --- a/src/skills/builtin/syncing-memory-filesystem/scripts/memfs-diff.ts +++ b/src/skills/builtin/syncing-memory-filesystem/scripts/memfs-diff.ts @@ -18,6 +18,7 @@ import { readdir, readFile } from "node:fs/promises"; import { createRequire } from "node:module"; import { homedir } from "node:os"; import { join, normalize, relative } from "node:path"; +import { hashFileBody, READ_ONLY_LABELS } from "./lib/frontmatter"; const require = createRequire(import.meta.url); const Letta = require("@letta-ai/letta-client") @@ -57,49 +58,7 @@ function hashContent(content: string): string { return createHash("sha256").update(content).digest("hex"); } -/** - * Parse frontmatter from file content. - */ -function parseFrontmatter(content: string): { - frontmatter: Record; - body: string; -} { - const frontmatterRegex = /^---\n([\s\S]*?)\n---\n([\s\S]*)$/; - const match = content.match(frontmatterRegex); - - if (!match || !match[1] || !match[2]) { - return { frontmatter: {}, body: content }; - } - - const frontmatterText = match[1]; - const body = match[2]; - const frontmatter: Record = {}; - - for (const line of frontmatterText.split("\n")) { - const colonIdx = line.indexOf(":"); - if (colonIdx > 0) { - const key = line.slice(0, colonIdx).trim(); - let value = line.slice(colonIdx + 1).trim(); - if ( - (value.startsWith('"') && value.endsWith('"')) || - (value.startsWith("'") && value.endsWith("'")) - ) { - value = value.slice(1, -1); - } - frontmatter[key] = value; - } - } - - return { frontmatter, body }; -} - -/** - * Hash just the body of file content (excluding frontmatter). - */ -function hashFileBody(content: string): string { - const { body } = parseFrontmatter(content); - return hashContent(body); -} +// parseFrontmatter/hashFileBody provided by shared helper function getMemoryRoot(agentId: string): string { return join(homedir(), ".letta", "agents", agentId, "memory"); @@ -308,7 +267,9 @@ async function findConflicts(agentId: string): Promise<{ if (!fileEntry || !blockEntry) continue; // read_only blocks are API-authoritative; no conflicts possible - if (blockEntry.read_only) continue; + const effectiveReadOnly = + !!blockEntry.read_only || READ_ONLY_LABELS.has(label); + if (effectiveReadOnly) continue; // Full file hash for "file changed" check const fileHash = hashContent(fileEntry.content); diff --git a/src/skills/builtin/syncing-memory-filesystem/scripts/memfs-resolve.ts b/src/skills/builtin/syncing-memory-filesystem/scripts/memfs-resolve.ts index a2c9e37..69b93f8 100644 --- a/src/skills/builtin/syncing-memory-filesystem/scripts/memfs-resolve.ts +++ b/src/skills/builtin/syncing-memory-filesystem/scripts/memfs-resolve.ts @@ -23,6 +23,7 @@ import { readdir, readFile } from "node:fs/promises"; import { createRequire } from "node:module"; import { homedir } from "node:os"; import { dirname, join, relative } from "node:path"; +import { parseFrontmatter, READ_ONLY_LABELS } from "./lib/frontmatter"; const require = createRequire(import.meta.url); const Letta = require("@letta-ai/letta-client") @@ -67,41 +68,7 @@ function hashContent(content: string): string { return createHash("sha256").update(content).digest("hex"); } -/** - * Parse frontmatter from file content. - */ -function parseFrontmatter(content: string): { - frontmatter: Record; - body: string; -} { - const frontmatterRegex = /^---\n([\s\S]*?)\n---\n([\s\S]*)$/; - const match = content.match(frontmatterRegex); - - if (!match || !match[1] || !match[2]) { - return { frontmatter: {}, body: content }; - } - - const frontmatterText = match[1]; - const body = match[2]; - const frontmatter: Record = {}; - - for (const line of frontmatterText.split("\n")) { - const colonIdx = line.indexOf(":"); - if (colonIdx > 0) { - const key = line.slice(0, colonIdx).trim(); - let value = line.slice(colonIdx + 1).trim(); - if ( - (value.startsWith('"') && value.endsWith('"')) || - (value.startsWith("'") && value.endsWith("'")) - ) { - value = value.slice(1, -1); - } - frontmatter[key] = value; - } - } - - return { frontmatter, body }; -} +// parseFrontmatter provided by shared helper /** * Parse block update from file content (update-mode: only update metadata if present in frontmatter). @@ -355,9 +322,12 @@ async function resolveConflicts( continue; } + const effectiveReadOnly = + !!block.read_only || READ_ONLY_LABELS.has(label); + if (resolution === "file") { // read_only blocks: ignore local edits, overwrite file from API - if (block.read_only) { + if (effectiveReadOnly) { const fileContent = renderBlockToFileContent(block); writeMemoryFile(dir, label, fileContent); result.resolved.push({ diff --git a/src/skills/builtin/syncing-memory-filesystem/scripts/memfs-status.ts b/src/skills/builtin/syncing-memory-filesystem/scripts/memfs-status.ts index 0583f7f..551a301 100644 --- a/src/skills/builtin/syncing-memory-filesystem/scripts/memfs-status.ts +++ b/src/skills/builtin/syncing-memory-filesystem/scripts/memfs-status.ts @@ -18,6 +18,7 @@ import { readdir, readFile } from "node:fs/promises"; import { createRequire } from "node:module"; import { homedir } from "node:os"; import { join, relative } from "node:path"; +import { hashFileBody, READ_ONLY_LABELS } from "./lib/frontmatter"; const require = createRequire(import.meta.url); const Letta = require("@letta-ai/letta-client") @@ -57,49 +58,7 @@ function hashContent(content: string): string { return createHash("sha256").update(content).digest("hex"); } -/** - * Parse frontmatter from file content. - */ -function parseFrontmatter(content: string): { - frontmatter: Record; - body: string; -} { - const frontmatterRegex = /^---\n([\s\S]*?)\n---\n([\s\S]*)$/; - const match = content.match(frontmatterRegex); - - if (!match || !match[1] || !match[2]) { - return { frontmatter: {}, body: content }; - } - - const frontmatterText = match[1]; - const body = match[2]; - const frontmatter: Record = {}; - - for (const line of frontmatterText.split("\n")) { - const colonIdx = line.indexOf(":"); - if (colonIdx > 0) { - const key = line.slice(0, colonIdx).trim(); - let value = line.slice(colonIdx + 1).trim(); - if ( - (value.startsWith('"') && value.endsWith('"')) || - (value.startsWith("'") && value.endsWith("'")) - ) { - value = value.slice(1, -1); - } - frontmatter[key] = value; - } - } - - return { frontmatter, body }; -} - -/** - * Hash just the body of file content (excluding frontmatter). - */ -function hashFileBody(content: string): string { - const { body } = parseFrontmatter(content); - return hashContent(body); -} +// parseFrontmatter/hashFileBody provided by shared helper function getMemoryRoot(agentId: string): string { return join(homedir(), ".letta", "agents", agentId, "memory"); @@ -175,12 +134,6 @@ async function readMemoryFiles( // Only memory_filesystem is managed by memfs itself const MEMFS_MANAGED_LABELS = new Set(["memory_filesystem"]); -// Read-only labels are API-authoritative (file-only copies should be ignored) -const READ_ONLY_LABELS = new Set([ - "skills", - "loaded_skills", - "memory_filesystem", -]); interface StatusResult { conflicts: Array<{ label: string }>; @@ -299,6 +252,8 @@ async function checkStatus(agentId: string): Promise { const fileInSystem = !!systemFile; const blockEntry = attachedBlock || detachedBlock; const isAttached = !!attachedBlock; + const effectiveReadOnly = + !!blockEntry?.read_only || READ_ONLY_LABELS.has(label); // Check for location mismatch if (fileEntry && blockEntry) { @@ -331,6 +286,10 @@ async function checkStatus(agentId: string): Promise { } if (!fileEntry && blockEntry) { + if (effectiveReadOnly) { + pendingFromFile.push(label); + continue; + } if (lastFileHash && !blockChanged) continue; // File deleted, block unchanged newBlocks.push(label); continue; @@ -339,7 +298,7 @@ async function checkStatus(agentId: string): Promise { if (!fileEntry || !blockEntry) continue; // Both exist - read_only blocks are API-authoritative - if (blockEntry.read_only) { + if (effectiveReadOnly) { if (blockChanged) pendingFromBlock.push(label); continue; } diff --git a/src/tests/skills/syncing-memory-filesystem-scripts.test.ts b/src/tests/skills/syncing-memory-filesystem-scripts.test.ts new file mode 100644 index 0000000..affcacd --- /dev/null +++ b/src/tests/skills/syncing-memory-filesystem-scripts.test.ts @@ -0,0 +1,47 @@ +/** + * Tests for syncing-memory-filesystem script helpers + */ + +import { describe, expect, test } from "bun:test"; +import { createHash } from "node:crypto"; +import { parseMdxFrontmatter } from "../../agent/memory"; +import { + hashFileBody, + parseFrontmatter, +} from "../../skills/builtin/syncing-memory-filesystem/scripts/lib/frontmatter"; + +describe("syncing-memory-filesystem script frontmatter helpers", () => { + test("parseFrontmatter matches parseMdxFrontmatter and trims body", () => { + const content = [ + "---", + "description: Test description", + "limit: 123", + "---", + "", + "Hello world", + "", + ].join("\n"); + + const parsed = parseFrontmatter(content); + const expected = parseMdxFrontmatter(content); + + expect(parsed).toEqual(expected); + expect(parsed.body).toBe("Hello world"); + }); + + test("hashFileBody uses trimmed body (no leading newline)", () => { + const content = [ + "---", + "description: Test description", + "---", + "", + "Line one", + "Line two", + ].join("\n"); + + const { body } = parseMdxFrontmatter(content); + const expectedHash = createHash("sha256").update(body).digest("hex"); + + expect(hashFileBody(content)).toBe(expectedHash); + }); +});