From 9d62d40967ddc2aaf331b32552d373ff2e11b4fa Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 17 Aug 2026 15:12:25 +0000 Subject: [PATCH] Stop recipe_update reporting photos and topics as lost Editing a recipe through the tool answered with `photos: []` and a `topics: []`, and the model read both as data loss - it told the user its edit had probably dropped three photos and every topic tag. Neither had been touched. The photo list was a literal. recipe_update never sets the photo set, so the RPC inherits the previous version's links onto the new version, and the tool asserted an empty array instead of looking. It now reads the post-write link set back the same way recipe_get does; the shared read moved into _recipe_helpers.ts so the two answer photo questions identically. The header comment justifying the shortcut ("the photo tools aren't ported for v1") had outlived the port. The topics array was true but meaningless. Any content edit fires the re-queue trigger, which empties the column so the curation unit re-tags the recipe, and the RPC reads the row back after that trigger runs - so the field is always empty on this path, whatever the recipe was tagged with a second earlier and will be tagged with again within the hour. It is bookkeeping, not the recipe's tags, so the tool strips it rather than echoing a number that can only mislead. recipe_save's `photos: []` is a different case and stays honest: a create links no images. The user doc gained the missing consequence - an edited recipe really does sit under "untagged" until the worker's next sweep - and the dev doc's file list, which pointed at browser-side tool paths that have been schema-only since the function port, now points where the code is. --- docs/dev/cookbook.md | 43 +++++- docs/user/cookbook.md | 5 + src/lib/tools/recipe_update.schema.ts | 4 +- .../functions/tests/recipe_update.test.ts | 138 ++++++++++++++++++ .../functions/venice/tools/_recipe_helpers.ts | 50 +++++++ supabase/functions/venice/tools/recipe_get.ts | 40 +---- .../functions/venice/tools/recipe_update.ts | 27 +++- 7 files changed, 256 insertions(+), 51 deletions(-) create mode 100644 supabase/functions/tests/recipe_update.test.ts create mode 100644 supabase/functions/venice/tools/_recipe_helpers.ts 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) }; }, };