diff --git a/docs/dev/cookbook.md b/docs/dev/cookbook.md index f04c3dc0..d80ca8bd 100644 --- a/docs/dev/cookbook.md +++ b/docs/dev/cookbook.md @@ -46,11 +46,18 @@ unaffected. `CustomEvent` (`nak:recipes:changed`) so tools stay UI-unaware - its handler calls `loadRecipes`, i.e. a tool mutation resets the list to the top rather than trying to patch a row in the middle of a page. -- `src/lib/tools/recipe_save.ts`, `recipe_list.ts`, `recipe_get.ts`, - `recipe_update.ts`, `recipe_delete.ts`, `recipe_photos_attach.ts`, - `recipe_photos_remove.ts`, `recipe_photos_reorder.ts`, - `recipe_photo_label_set.ts` — the nine LLM tools. Mutating tools - fire `notifyCookbookChanged` on success. +- `src/lib/tools/recipe_*.schema.ts` — the nine LLM tools' wire + schemas (name, description, JSON Schema parameters). Schema only; + the browser never executes a recipe tool. +- `supabase/functions/venice/tools/recipe_save.ts`, `recipe_list.ts`, + `recipe_get.ts`, `recipe_update.ts`, `recipe_delete.ts`, and + `recipe_photos.ts` (which carries all four photo verbs) — the + implementations, dispatched function-side against the admin client. + `_recipe_helpers.ts` holds `readRecipePhotoMeta`, the "newest + version's link set" read that `recipe_get` and `recipe_update` + both answer photo questions with. Mutating tools reach the UI + through the realtime relay, not `notifyCookbookChanged` - see the + relay gotcha below. - `src/lib/supabase.ts` — `Recipe`, `RecipeVersion`, `RecipePhoto`, `RecipePhotoMeta`, and `RecipePhotoInput` types + `createRecipe / updateRecipe / deleteRecipe / getRecipe / listRecipes / @@ -284,9 +291,12 @@ unaffected. - `MAX_RECIPE_COOKLANG_CHARS = 20_000`, `MAX_RECIPE_TITLE_CHARS = 160` — shared between the tools and the modal so schema validation agrees everywhere. -- Tool contract follows the standard `ToolDef` (see `./tools.md`); - mutating tools (`recipe_save / update / delete`) call - `notifyCookbookChanged()` before returning. +- Tool contract follows the standard `ToolDef` (see `./tools.md`). + The tools run function-side, so a mutation reaches the UI through + the `recipes` realtime relay rather than an in-process notify call. +- A mutating tool's response must describe state it actually read + back, not the shape the caller asked for. See the echoed-row gotcha + below for the two fields this went wrong on. ## Versioning @@ -500,6 +510,23 @@ keystrokes; the LLM tool path keeps using `listRecipes`. doesn't grow a new concern. Edit-pane photo controls live in their own form-row between the change-message field and the cooklang+preview panes. +- **A tool's echoed row is a claim about live state - read it back.** + `recipe_update` answered with a hardcoded `photos: []` and echoed the + `topics` column, and both read as data loss to the model, which + relayed "your photos and tags are gone" to the user after an edit + that had preserved every one of them. Photos: the RPC inherits the + previous version's links when `p_set_image_ids` is false, so the tool + reads the post-write link set back through + `readRecipePhotoMeta` (`tools/_recipe_helpers.ts`, shared with + `recipe_get`) instead of asserting a shape. Topics: the + `clear_recipe_topics_on_change` trigger empties the column so the + curation unit re-tags the row, and the RPC's `return query` reads the + row back AFTER that trigger fires - so the field is always `[]` on + this path and the tool strips it rather than echoing a number that + only ever means "re-queued." `recipe_save`'s `photos: []` is a + different case and stays: a create passes `p_image_ids: null`, so + the recipe genuinely has no photos yet. + `tests/recipe_update.test.ts` guards both. - **Photo IDs are stable across versions.** A photo upserted into `recipe_images` keeps the same id forever for that user; reordering or appending changes the link rows, not the image diff --git a/docs/user/cookbook.md b/docs/user/cookbook.md index d11c1df4..16c2b111 100644 --- a/docs/user/cookbook.md +++ b/docs/user/cookbook.md @@ -309,6 +309,11 @@ yet. "mexican". - **Tags are managed for you.** No manual tagging UI; any edit to a recipe re-queues it, and the worker re-tags it on its next pass. +- **An edited recipe goes untagged until the worker catches up.** + Re-queuing clears the old tags immediately, so a recipe you just + edited drops out of its topic filters and shows under **untagged** + for a while - the worker's sweep runs hourly. The tags come back + on their own; nothing was lost. The pill row below the dropdown carries the active selection; each pill's × clears just that one tag, and a "clear" link appears when diff --git a/src/lib/tools/recipe_update.schema.ts b/src/lib/tools/recipe_update.schema.ts index 8205b0c7..a70dd960 100644 --- a/src/lib/tools/recipe_update.schema.ts +++ b/src/lib/tools/recipe_update.schema.ts @@ -18,7 +18,9 @@ export const recipeUpdateSchema = { 'multi-word braced name (`@pre-minced garlic{1%tbsp}`), not two ' + '`@` tokens; mark optional ingredients with `@?` ' + '(`@?cilantro{2%tbsp}`). change_message is REQUIRED and lands in the recipe ' + - 'history. Returns the updated row.', + "history. Returns the updated row plus the recipe's current photo " + + 'list, which this tool never changes - use the recipe_photos_* ' + + 'tools to edit photos.', shortDescription: 'edit a saved recipe', parameters: { type: 'object', diff --git a/supabase/functions/tests/recipe_update.test.ts b/supabase/functions/tests/recipe_update.test.ts new file mode 100644 index 00000000..1426b6f2 --- /dev/null +++ b/supabase/functions/tests/recipe_update.test.ts @@ -0,0 +1,138 @@ +// Return-shape guards for venice/tools/recipe_update.ts. +// +// The regression these exist for: a scalar edit (title / cooklang / +// source / rating) inherits the recipe's photo links onto the new +// version, but the tool answered with a hardcoded `photos: []` and +// echoed the `topics` column that the re-tag trigger had just emptied. +// The model read both as data loss and told the user its edit had +// dropped three photos and every topic tag. Nothing had been dropped. + +import { assertEquals, assertRejects } from '@std/assert'; +import type { SupabaseClient } from '@supabase/supabase-js'; +import type { ToolContext } from '../venice/performToolCall.ts'; +import { recipeUpdate } from '../venice/tools/recipe_update.ts'; + +interface PhotoLink { + position: number; + image_id: string; + label: string | null; +} + +interface Scenario { + /** Row the RPC echoes back, read AFTER the re-tag trigger fires. */ + rpcRow?: Record; + /** Links hanging off the recipe's newest version row. */ + links?: PhotoLink[] | null; + /** No version row at all - the defensive branch. */ + noVersionRow?: boolean; +} + +/** + * Thenable PostgREST-builder stub over the one read recipe_update makes + * (newest recipe_versions row + its recipe_version_images links), plus + * the recipe_update_with_version RPC. + */ +function fakeCtx(scenario: Scenario): { + ctx: ToolContext; + rpcCalls: Array>; +} { + const rpcCalls: Array> = []; + const versionRow = scenario.noVersionRow + ? null + : { id: 'v-2', recipe_version_images: scenario.links ?? [] }; + + const adminClient = { + rpc: (_name: string, args: Record) => { + rpcCalls.push(args); + return Promise.resolve({ + data: [scenario.rpcRow ?? { id: 'r-1', title: 'Meatballs', topics: [] }], + error: null, + }); + }, + from: () => { + const c: Record = {}; + for (const m of ['select', 'eq', 'order', 'limit']) c[m] = () => c; + c.maybeSingle = () => Promise.resolve({ data: versionRow, error: null }); + return c; + }, + } as unknown as SupabaseClient; + + return { + ctx: { + adminClient, + userId: 'u-1', + threadId: 't-1', + signal: new AbortController().signal, + depth: 0, + } as ToolContext, + rpcCalls, + }; +} + +const ARGS = { id: 'r-1', title: 'Meatballs', change_message: 'Retitled' }; + +Deno.test('recipe_update reports the photos it carried forward', async () => { + const { ctx } = fakeCtx({ + links: [ + { position: 1, image_id: 'img-b', label: 'crumb shot' }, + { position: 0, image_id: 'img-a', label: null }, + { position: 2, image_id: 'img-c', label: null }, + ], + }); + const out = (await recipeUpdate.execute(ARGS, ctx)) as { + photos: Array<{ id: string; position: number; label: string | null }>; + }; + // Sorted by position, not by the order the join happened to return. + assertEquals(out.photos, [ + { id: 'img-a', position: 0, label: null }, + { id: 'img-b', position: 1, label: 'crumb shot' }, + { id: 'img-c', position: 2, label: null }, + ]); +}); + +Deno.test('recipe_update omits the re-queued topics column', async () => { + // The trigger empties `topics` on every content edit so the curation + // unit re-tags the row, and the RPC reads the row back after it fires. + // An empty array here is bookkeeping, not the recipe's tags, so the + // field must not reach the model at all. + const { ctx } = fakeCtx({ + rpcRow: { id: 'r-1', title: 'Meatballs', rating: 4, topics: [] }, + }); + const out = (await recipeUpdate.execute(ARGS, ctx)) as Record; + assertEquals('topics' in out, false); + assertEquals(out.rating, 4); +}); + +Deno.test('recipe_update reports no photos when the recipe has none', async () => { + const { ctx } = fakeCtx({ links: [] }); + const out = (await recipeUpdate.execute(ARGS, ctx)) as { photos: unknown[] }; + assertEquals(out.photos, []); +}); + +Deno.test('recipe_update survives a recipe with no version row', async () => { + const { ctx } = fakeCtx({ noVersionRow: true }); + const out = (await recipeUpdate.execute(ARGS, ctx)) as { photos: unknown[] }; + assertEquals(out.photos, []); +}); + +Deno.test('recipe_update leaves the photo set alone', async () => { + // The photo-editing verbs are the recipe_photos_* tools. A scalar + // edit must reach the RPC with p_set_image_ids false so the previous + // version's links are inherited rather than cleared. + const { ctx, rpcCalls } = fakeCtx({ + links: [{ position: 0, image_id: 'img-a', label: null }], + }); + await recipeUpdate.execute(ARGS, ctx); + assertEquals(rpcCalls.length, 1); + assertEquals(rpcCalls[0].p_set_image_ids, false); + assertEquals(rpcCalls[0].p_image_ids, null); +}); + +Deno.test('recipe_update still rejects a patch with nothing to change', async () => { + const { ctx } = fakeCtx({}); + await assertRejects( + () => recipeUpdate.execute({ id: 'r-1', change_message: 'noop' }, ctx), + Error, + 'provide at least one of', + ); +}); diff --git a/supabase/functions/venice/tools/_recipe_helpers.ts b/supabase/functions/venice/tools/_recipe_helpers.ts new file mode 100644 index 00000000..6d1de6fe --- /dev/null +++ b/supabase/functions/venice/tools/_recipe_helpers.ts @@ -0,0 +1,50 @@ +// Shared read-back of a recipe's current photo set. +// +// The newest `recipe_versions` row carries the recipe's live photo +// links, so "which photos does this recipe have right now" is a join +// against that one row. Both recipe_get and recipe_update need the +// answer: get because the model asked for it, update because every +// scalar edit inherits the previous version's links and has to report +// what it carried forward. +// +// RLS OFF: callers validate recipe ownership before calling - recipe_get +// with an explicit user_id filter, recipe_update through the RPC's own +// user scoping - and recipe_versions inherits ownership through +// recipe_id, so an unscoped lookup by recipe_id is safe here. + +import type { SupabaseClient } from '@supabase/supabase-js'; + +export interface RecipePhotoMeta { + id: string; + position: number; + label: string | null; +} + +interface PhotoLinkRow { + position: number; + image_id: string; + label: string | null; +} + +export async function readRecipePhotoMeta( + adminClient: SupabaseClient, + recipeId: string, +): Promise { + const { data: versionRow, error } = await adminClient + .from('recipe_versions') + .select('id, recipe_version_images(position, image_id, label)') + .eq('recipe_id', recipeId) + .order('created_at', { ascending: false }) + .limit(1) + .maybeSingle(); + if (error) throw new Error(`listRecipePhotoMeta failed: ${error.message}`); + if (!versionRow) return []; + + const links = (versionRow as { recipe_version_images?: PhotoLinkRow[] | null }) + .recipe_version_images; + if (!Array.isArray(links)) return []; + + return links + .map((l) => ({ id: l.image_id, position: l.position, label: l.label ?? null })) + .sort((a, b) => a.position - b.position); +} diff --git a/supabase/functions/venice/tools/recipe_get.ts b/supabase/functions/venice/tools/recipe_get.ts index 6385291c..fede2c1a 100644 --- a/supabase/functions/venice/tools/recipe_get.ts +++ b/supabase/functions/venice/tools/recipe_get.ts @@ -8,12 +8,7 @@ // Auth: b-strict. recipes.user_id direct ownership filter. import { registerTool, type ToolContext, type ToolDef } from '../performToolCall.ts'; - -interface RecipePhotoMeta { - id: string; - position: number; - label: string | null; -} +import { readRecipePhotoMeta } from './_recipe_helpers.ts'; export const recipeGet: ToolDef = { name: 'recipe_get', @@ -34,35 +29,10 @@ export const recipeGet: ToolDef = { if (recipeErr) throw new Error(`getRecipe failed: ${recipeErr.message}`); if (!recipe) return { found: false }; - // Newest recipe_version row carries the current photo set. The - // browser-side wrapper joins recipe_version_images in one round; - // mirror the join here. RLS-OFF rationale: recipe ownership was - // already validated above, and recipe_versions inherits ownership - // through recipe_id, so an unscoped lookup by recipe_id is safe. - const { data: versionRow, error: versionErr } = await ctx.adminClient - .from('recipe_versions') - .select('id, recipe_version_images(position, image_id, label)') - .eq('recipe_id', id) - .order('created_at', { ascending: false }) - .limit(1) - .maybeSingle(); - if (versionErr) throw new Error(`listRecipePhotoMeta failed: ${versionErr.message}`); - - let photos: RecipePhotoMeta[] = []; - if (versionRow) { - type LinkRow = { position: number; image_id: string; label: string | null }; - const links = (versionRow as { recipe_version_images?: LinkRow[] | null }) - .recipe_version_images; - if (Array.isArray(links)) { - photos = links - .map((l) => ({ - id: l.image_id, - position: l.position, - label: l.label ?? null, - })) - .sort((a, b) => a.position - b.position); - } - } + // Newest recipe_version row carries the current photo set; the + // shared helper owns that join (recipe_update reads it back the + // same way). Ownership was validated by the select above. + const photos = await readRecipePhotoMeta(ctx.adminClient, id); return { found: true, diff --git a/supabase/functions/venice/tools/recipe_update.ts b/supabase/functions/venice/tools/recipe_update.ts index 8fde0175..6ab02741 100644 --- a/supabase/functions/venice/tools/recipe_update.ts +++ b/supabase/functions/venice/tools/recipe_update.ts @@ -4,14 +4,16 @@ // (p_user_id-aware). Cooklang validation mirrors recipe_save's // inline check. // -// What we skip: the photo set is left alone (the RPC inherits the -// previous version's link set when p_set_image_ids=false). The -// browser-side echo of "current photo set" via listRecipePhotoMeta -// is also skipped - the recipe-photo manipulation tools aren't -// ported for v1, and recipe_get is the alternative when the model -// needs to see the current photos. +// This tool never sets the photo list (p_set_image_ids=false), so the +// RPC carries the previous version's links onto the new version. The +// response still reads those links back and reports them: an update +// that reported `photos: []` looked like it had wiped the recipe's +// photos, and the model relayed that to the user as data loss on an +// edit that had in fact preserved every photo. Changing the photo set +// is the recipe_photos_* tools' job. import { registerTool, type ToolContext, type ToolDef } from '../performToolCall.ts'; +import { readRecipePhotoMeta } from './_recipe_helpers.ts'; import { ArgErrors } from './_validate.ts'; // Mirror of src/lib/recipe-limits.ts - the caps the wire schema @@ -133,7 +135,18 @@ export const recipeUpdate: ToolDef = { const rows = (data ?? []) as Array>; if (rows.length === 0) throw new Error('updateRecipe returned no row'); - return { ...rows[0], photos: [] }; + // Drop `topics` from the echoed row. The + // clear_recipe_topics_on_change trigger empties the column on any + // content edit so the recipe-topics curation unit re-tags it, and + // the RPC reads the row back after that trigger has fired - so this + // field is ALWAYS an empty array here, whatever the recipe was + // tagged with a moment earlier and will be tagged with again once + // the unit catches up. Echoing it invited the model to report the + // tags as lost. Callers that want the live tags read them back with + // recipe_get after the curation unit has run. + const { topics: _requeuedTopics, ...row } = rows[0]; + + return { ...row, photos: await readRecipePhotoMeta(ctx.adminClient, id) }; }, };