diff --git a/docs/dev/memory.md b/docs/dev/memory.md index 2ff5fd96..14b951bb 100644 --- a/docs/dev/memory.md +++ b/docs/dev/memory.md @@ -857,6 +857,23 @@ renders the delta as a chip (`memorySizeDelta` in ## Gotchas +- **A write tool must not echo `topics`.** `memory_update` and + `memory_reshape` select the row back with `RETURNING`, which runs + AFTER `clear_memory_topics_on_change` has emptied the column to + re-queue the row for the memory-topics curation unit. Echoing the + field therefore reports an empty tag list at exactly the moment the + model edited the text, and reads as "your tags were dropped" on a + write that lost nothing - the same false alarm the recipe tools hit + (see `./cookbook.md`). Both tools leave `topics` out of the select + entirely; `tests/memory_write_shape.test.ts` asserts on the column + list, not just the response, so re-adding it to the select fails the + gate. Selecting it only on `memory_update`'s confidence-only path + (where the trigger does not fire and the tags do survive) would be + worse than omitting it: the model would have no way to tell an + accurate empty list from a re-queued one. `memory_search` and + `memory_get` are the read-back paths. `memory_create` keeps its + `topics` because an insert never fires the trigger - a new memory + genuinely has no tags yet. - **DELETE events need the (id, user_id) replica identity.** The `subscribeToMemoryChanges` relay filters on `user_id`, but a DELETE's WAL record carries only the table's replica identity - diff --git a/src/lib/tools/record_delete.schema.ts b/src/lib/tools/record_delete.schema.ts index 058ef86d..ff87146a 100644 --- a/src/lib/tools/record_delete.schema.ts +++ b/src/lib/tools/record_delete.schema.ts @@ -8,7 +8,9 @@ export const recordDeleteSchema = { 'Delete a record the user no longer wants kept. Provide id (from ' + 'record_list or record_search). Hard delete - records are historical ' + 'documentation, so only remove one when the user explicitly asks or ' + - 'it is clearly a duplicate / mistake. Returns {deleted: true}.', + 'it is clearly a duplicate / mistake. Returns {deleted: true}, or ' + + '{deleted: false} when no record with that id exists - check the ' + + 'flag rather than assuming the delete landed.', shortDescription: 'delete a record', parameters: { type: 'object', diff --git a/supabase/functions/tests/memory_write_shape.test.ts b/supabase/functions/tests/memory_write_shape.test.ts new file mode 100644 index 00000000..a8e72e0e --- /dev/null +++ b/supabase/functions/tests/memory_write_shape.test.ts @@ -0,0 +1,92 @@ +// Return-shape guards for the memory write tools. +// +// Same regression as tests/recipe_update.test.ts, found by auditing the +// other tools for it: a write echoed the row's `topics` column, but the +// label/data edit that triggered the write also fires +// clear_memory_topics_on_change, which empties that column so the +// curation unit re-tags the row. RETURNING reads the row back after the +// trigger, so the field reported an empty list exactly when the model +// had just edited the text - and reads as "the edit dropped your tags" +// on a write that lost nothing. +// +// These assert on the SELECT column list, which is where the field +// enters the response. A stub that returned a fixed row would pass even +// if someone re-added `topics` to the select. + +import { assert, assertEquals } from '@std/assert'; +import type { SupabaseClient } from '@supabase/supabase-js'; +import type { ToolContext } from '../venice/performToolCall.ts'; +import { memoryUpdate } from '../venice/tools/memory_update.ts'; +import { memoryReshape } from '../venice/tools/memory_reshape.ts'; + +function fakeCtx(): { ctx: ToolContext; selects: string[] } { + const selects: string[] = []; + const row = { + id: 'm-1', + label: 'likes rye', + data: 'Prefers rye bread.', + confidence: 7, + created_at: 't0', + updated_at: 't1', + }; + + const adminClient = { + from: () => { + const c: Record = {}; + for (const m of ['update', 'eq', 'insert', 'order', 'limit', 'in']) c[m] = () => c; + c.select = (cols?: string) => { + if (typeof cols === 'string') selects.push(cols); + return c; + }; + c.single = () => Promise.resolve({ data: row, error: null }); + c.maybeSingle = () => Promise.resolve({ data: row, error: null }); + c.then = (res: (v: unknown) => unknown, rej?: (e: unknown) => unknown) => + Promise.resolve({ data: [row], error: null }).then(res, rej); + return c; + }, + } as unknown as SupabaseClient; + + return { + ctx: { + adminClient, + userId: 'u-1', + threadId: 't-1', + signal: new AbortController().signal, + depth: 0, + } as ToolContext, + selects, + }; +} + +/** The column list the tool reads its response row back with. */ +function writeSelect(selects: string[]): string { + const cols = selects.find((s) => s.includes('confidence')); + assert(cols, `no response-row select found in ${JSON.stringify(selects)}`); + return cols; +} + +Deno.test('memory_update does not echo the re-queued topics column', async () => { + const { ctx, selects } = fakeCtx(); + const out = (await memoryUpdate.execute( + { id: 'm-1', data: 'Prefers rye bread.', message: 'tightened' }, + ctx, + )) as Record; + assertEquals(writeSelect(selects).includes('topics'), false); + assertEquals('topics' in out, false); + // The fields the model actually needs still come back. + assertEquals(out.id, 'm-1'); + assertEquals(out.confidence, 7); +}); + +Deno.test('memory_reshape does not echo the re-queued topics column', async () => { + // A reshape always rewrites label or data, so it always fires the + // re-tag trigger - this field could never be anything but empty. + const { ctx, selects } = fakeCtx(); + const out = (await memoryReshape.execute( + { id: 'm-1', data: 'Prefers rye.', message: 'de-narrated' }, + ctx, + )) as Record; + assertEquals(writeSelect(selects).includes('topics'), false); + assertEquals('topics' in out, false); + assertEquals(out.label, 'likes rye'); +}); diff --git a/supabase/functions/venice/tools/memory_reshape.ts b/supabase/functions/venice/tools/memory_reshape.ts index 51cc1b97..12ae03df 100644 --- a/supabase/functions/venice/tools/memory_reshape.ts +++ b/supabase/functions/venice/tools/memory_reshape.ts @@ -83,7 +83,13 @@ export const memoryReshape: ToolDef = { .update(patch) .eq('id', id) .eq('user_id', ctx.userId) - .select('id, label, data, confidence, topics, created_at, updated_at') + // No `topics`: a reshape always changes label or data, so it always + // fires clear_memory_topics_on_change, and the RETURNING clause + // reads the row back after that trigger has emptied the column. + // The field could only ever echo an empty list here, reading as + // "the reshape dropped the tags" when the curation unit has merely + // been asked to re-tag. Same reasoning as memory_update. + .select('id, label, data, confidence, created_at, updated_at') .single(); if (error) throw new Error(`reshapeMemory failed: ${error.message}`); diff --git a/supabase/functions/venice/tools/memory_update.ts b/supabase/functions/venice/tools/memory_update.ts index 5ba1a6b8..c5d439a3 100644 --- a/supabase/functions/venice/tools/memory_update.ts +++ b/supabase/functions/venice/tools/memory_update.ts @@ -5,6 +5,17 @@ // trigger that nulls the embedding, queuing the row for re-embedding // by the worker; a confidence-only patch does not. Wire schema in // src/lib/tools/memory_update.schema.ts. +// +// The echoed row deliberately omits `topics`. The same label/data change +// also fires clear_memory_topics_on_change, which empties the column so +// the memory-topics curation unit re-tags the row, and the RETURNING +// clause reads the row back AFTER that trigger - so the field would read +// as an empty list precisely when the model had just edited the text. +// Reporting it invites "your tags are gone" on a write that lost +// nothing. Selecting it only on the confidence-only path (where it does +// survive) would be worse: the model has no way to tell an accurate +// empty list from a re-queued one. memory_search and memory_get are the +// read-back paths once the unit has caught up. import { registerTool, type ToolContext, type ToolDef } from '../performToolCall.ts'; import { appendMemoryChangelog } from './_memory_changelog.ts'; @@ -90,7 +101,7 @@ export const memoryUpdate: ToolDef = { .update(patch) .eq('id', id) .eq('user_id', ctx.userId) - .select('id, label, data, confidence, topics, created_at, updated_at') + .select('id, label, data, confidence, created_at, updated_at') .single(); if (error) throw new Error(`updateMemory failed: ${error.message}`);