From 8ee7e92aa67082a3760613a3f2e03778161a342f Mon Sep 17 00:00:00 2001 From: Viscerous Date: Tue, 23 Jun 2026 01:17:22 +0200 Subject: [PATCH] fix(librarian): let the analyze pass read fragments before editing to prevent blind overwrites --- src/server/librarian/agent.ts | 8 +- src/server/librarian/analysis-tools.ts | 24 +++++ src/server/librarian/blocks.ts | 20 +++- tests/librarian/analysis-tools.test.ts | 136 +++++++++++++++++++++++++ 4 files changed, 185 insertions(+), 3 deletions(-) diff --git a/src/server/librarian/agent.ts b/src/server/librarian/agent.ts index 42251a9c..97868d66 100644 --- a/src/server/librarian/agent.ts +++ b/src/server/librarian/agent.ts @@ -26,6 +26,7 @@ import { reportUsage } from '../llm/token-tracker' import { createLogger } from '../logging' import { compileAgentContext } from '../agents/compile-agent-context' import { createEmptyCollector, createAnalysisTools } from './analysis-tools' +import { createFragmentTools } from '../llm/tools' import { buildAnalyzeSystemPrompt } from './blocks' import { createAnalysisBuffer, @@ -129,7 +130,10 @@ async function runLibrarianInner( const disableDirections = story.settings?.disableLibrarianDirections === true const disableSuggestions = story.settings?.disableLibrarianSuggestions === true const collector = createEmptyCollector() - const analysisTools = createAnalysisTools(collector, { dataDir, storyId, disableDirections, disableSuggestions }) + const analysisTools = { + ...createAnalysisTools(collector, { dataDir, storyId, disableDirections, disableSuggestions }), + ...createFragmentTools(dataDir, storyId, { readOnly: true }), + } // Compile context via block system const compiled = await compileAgentContext(dataDir, storyId, 'librarian.analyze', blockContext, analysisTools) @@ -160,7 +164,7 @@ async function runLibrarianInner( instructions: systemMessage?.content || 'You are a helpful assistant.', tools: compiled.tools, toolChoice: 'auto', - stopWhen: stepCountIs(3), + stopWhen: stepCountIs(6), temperature, providerOptions, }) diff --git a/src/server/librarian/analysis-tools.ts b/src/server/librarian/analysis-tools.ts index c62c0078..00a06e05 100644 --- a/src/server/librarian/analysis-tools.ts +++ b/src/server/librarian/analysis-tools.ts @@ -170,6 +170,30 @@ export function createAnalysisTools(collector: AnalysisCollector, opts?: { dataD }, }), + editFragment: tool({ + description: 'Edit an existing character, knowledge, or guideline fragment by replacing a specific text span (oldText) with newText. Use this for precise corrections and edits to avoid rewriting the whole fragment.', + inputSchema: z.object({ + fragmentId: z.string().describe('The ID of the fragment to edit (e.g. ch-abc, kn-xyz)'), + oldText: z.string().describe('The exact text span inside the fragment to find and replace'), + newText: z.string().describe('The replacement text'), + }), + execute: async ({ fragmentId, oldText, newText }) => { + if (!opts) return { error: 'editFragment not available in this context' } + const existing = await getFragment(opts.dataDir, opts.storyId, fragmentId) + if (!existing) return { error: `Fragment ${fragmentId} not found` } + if (existing.type === 'prose') return { error: 'Cannot edit prose fragments via this tool' } + if (!existing.content.includes(oldText)) { + return { error: `Text not found in fragment ${fragmentId}: "${oldText}"` } + } + const editedContent = existing.content.replace(oldText, newText) + const protection = checkFragmentWrite(existing, { content: editedContent }) + if (!protection.allowed) return { error: protection.reason } + const updated = await updateFragmentVersioned(opts.dataDir, opts.storyId, fragmentId, { content: editedContent }, { reason: 'librarian-analysis' }) + if (!updated) return { error: `Failed to edit fragment ${fragmentId}` } + return { ok: true, fragmentId: updated.id } + }, + }), + updateFragment: tool({ description: 'Directly update an existing fragment by ID. Use this to correct or enrich character, knowledge, or guideline fragments based on new information from the prose.', inputSchema: z.object({ diff --git a/src/server/librarian/blocks.ts b/src/server/librarian/blocks.ts index 9f692aff..89f38df6 100644 --- a/src/server/librarian/blocks.ts +++ b/src/server/librarian/blocks.ts @@ -41,7 +41,13 @@ export function buildAnalyzeSystemPrompt(opts?: { disableDirections?: boolean; d } toolLines.push( - `${toolNumber}. updateFragment — Directly update an existing fragment by ID. Use this when an existing character, knowledge, or guideline fragment needs correction or enrichment based on new information.`, + `${toolNumber}. editFragment — Edit an existing character, knowledge, or guideline fragment by replacing a specific text span. Use this for precise corrections to avoid rewriting the whole fragment.`, + ' - Provide fragmentId, oldText, and newText.', + ) + toolNumber++ + + toolLines.push( + `${toolNumber}. updateFragment — Directly update an existing fragment by ID. Use this when an existing character, knowledge, or guideline fragment needs full correction or enrichment.`, ' - Provide the fragmentId and one or more of: name, description, content.', ' - Retain important established facts when updating content.', ) @@ -70,6 +76,18 @@ You have ${toolNumber - (opts?.disableDirections ? 1 : 0)} reporting tools. Use ${toolLines.join('\n')} +In addition to the reporting tools, you have read-only lookup tools: +- getCharacter(id), listCharacters() — Read character sheets. +- getKnowledge(id), listKnowledge() — Read world knowledge. +- getGuideline(id), listGuidelines() — Read story guidelines. +- getFragment(id), listFragments(type?) — Read any fragment, or list fragments by type. +- searchFragments(query, type?) — Search for text across all fragments. + +Instructions: +1. Your context includes a story summary and fragment summaries (IDs, names, descriptions) — not full content. Use the appropriate get tool to read the full content of any fragment you need. +2. Before editing or updating an existing character, knowledge, or guideline fragment, read it first using the appropriate get tool (e.g. getCharacter or getFragment). Its full content is not in your context, and updateFragment overwrites whatever you do not carry over. +3. Prefer editFragment over updateFragment for small, precise corrections — it changes only the named span and leaves the rest of the sheet intact. + ${alwaysCall} Only call the other tools if there are relevant findings. If there are no contradictions, suggestions, mentions, or timeline events, don't call those tools. Only return 'Analysis complete' in your final output. diff --git a/tests/librarian/analysis-tools.test.ts b/tests/librarian/analysis-tools.test.ts index 96472cc2..03c69cf1 100644 --- a/tests/librarian/analysis-tools.test.ts +++ b/tests/librarian/analysis-tools.test.ts @@ -32,6 +32,7 @@ describe('analysis-tools', () => { 'reportMentions', 'reportContradictions', 'reportTimeline', + 'editFragment', 'updateFragment', 'suggestFragment', 'suggestDirections', @@ -293,5 +294,140 @@ describe('analysis-tools', () => { expect(collector.fragmentSuggestions).toHaveLength(1) expect(result).toEqual({ ok: true }) }) + + it('editFragment replaces only the named span and preserves the rest of the body', async () => { + const { getFragment, updateFragmentVersioned } = await import('@/server/fragments/storage') + vi.mocked(getFragment).mockResolvedValueOnce({ + id: 'ch-001', + type: 'character', + name: 'Alice', + description: 'A warrior', + content: 'Alice is a brave warrior with blue eyes. Currently twenty years old, captain of the city guard.', + tags: [], + refs: [], + sticky: false, + placement: 'user', + createdAt: '', + updatedAt: '', + order: 0, + meta: {}, + version: 1, + versions: [], + }) + vi.mocked(updateFragmentVersioned).mockResolvedValueOnce({ id: 'ch-001' } as any) + + const collector = createEmptyCollector() + const tools = createAnalysisTools(collector, { dataDir: '/tmp', storyId: 'test-story' }) + const result = await tools.editFragment.execute!({ + fragmentId: 'ch-001', + oldText: 'twenty years old', + newText: 'twenty-one years old', + }, { toolCallId: 'a', messages: [], abortSignal: undefined as unknown as AbortSignal }) + + expect(result).toEqual({ ok: true, fragmentId: 'ch-001' }) + // The whole body is rewritten with only the span changed; the rest is preserved, + // and the analysis provenance stamp is retained. + expect(updateFragmentVersioned).toHaveBeenCalledWith( + '/tmp', + 'test-story', + 'ch-001', + { content: 'Alice is a brave warrior with blue eyes. Currently twenty-one years old, captain of the city guard.' }, + { reason: 'librarian-analysis' }, + ) + }) + + it('editFragment returns an error when oldText is not present', async () => { + const { getFragment } = await import('@/server/fragments/storage') + vi.mocked(getFragment).mockResolvedValueOnce({ + id: 'ch-001', + type: 'character', + name: 'Alice', + description: 'A warrior', + content: 'Alice is a brave warrior with blue eyes.', + tags: [], + refs: [], + sticky: false, + placement: 'user', + createdAt: '', + updatedAt: '', + order: 0, + meta: {}, + version: 1, + versions: [], + }) + + const collector = createEmptyCollector() + const tools = createAnalysisTools(collector, { dataDir: '/tmp', storyId: 'test-story' }) + const result = await tools.editFragment.execute!({ + fragmentId: 'ch-001', + oldText: 'green eyes', + newText: 'hazel eyes', + }, { toolCallId: 'a', messages: [], abortSignal: undefined as unknown as AbortSignal }) + + expect(result).toHaveProperty('error') + expect((result as any).error).toContain('Text not found') + }) + + it('editFragment refuses to edit prose fragments', async () => { + const { getFragment } = await import('@/server/fragments/storage') + vi.mocked(getFragment).mockResolvedValueOnce({ + id: 'pr-001', + type: 'prose', + name: 'Chapter 1', + description: '', + content: 'Once upon a time.', + tags: [], + refs: [], + sticky: false, + placement: 'user', + createdAt: '', + updatedAt: '', + order: 0, + meta: {}, + version: 1, + versions: [], + }) + + const collector = createEmptyCollector() + const tools = createAnalysisTools(collector, { dataDir: '/tmp', storyId: 'test-story' }) + const result = await tools.editFragment.execute!({ + fragmentId: 'pr-001', + oldText: 'Once', + newText: 'Twice', + }, { toolCallId: 'a', messages: [], abortSignal: undefined as unknown as AbortSignal }) + + expect((result as any).error).toContain('Cannot edit prose') + }) + + it('editFragment refuses to edit a locked fragment', async () => { + const { getFragment } = await import('@/server/fragments/storage') + vi.mocked(getFragment).mockResolvedValueOnce({ + id: 'ch-locked', + type: 'character', + name: 'Locked Char', + description: '', + content: 'Original content with a name inside.', + tags: [], + refs: [], + sticky: false, + placement: 'user', + createdAt: '', + updatedAt: '', + order: 0, + meta: { locked: true }, + version: 1, + versions: [], + }) + + const collector = createEmptyCollector() + const tools = createAnalysisTools(collector, { dataDir: '/tmp', storyId: 'test-story' }) + const result = await tools.editFragment.execute!({ + fragmentId: 'ch-locked', + oldText: 'a name', + newText: 'another name', + }, { toolCallId: 'a', messages: [], abortSignal: undefined as unknown as AbortSignal }) + + expect(result).toHaveProperty('error') + }) }) })