Stop recipe_update reporting photos and topics as lost

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

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

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',

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<string, unknown>;
+  /** 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<Record<string, unknown>>;
+} {
+  const rpcCalls: Array<Record<string, unknown>> = [];
+  const versionRow = scenario.noVersionRow
+    ? null
+    : { id: 'v-2', recipe_version_images: scenario.links ?? [] };
+
+  const adminClient = {
+    rpc: (_name: string, args: Record<string, unknown>) => {
+      rpcCalls.push(args);
+      return Promise.resolve({
+        data: [scenario.rpcRow ?? { id: 'r-1', title: 'Meatballs', topics: [] }],
+        error: null,
+      });
+    },
+    from: () => {
+      const c: Record<string, unknown> = {};
+      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<string, unknown>;
+  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',
+  );
+});

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<RecipePhotoMeta[]> {
+  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);
+}

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,

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<Record<string, unknown>>;
     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) };
   },
 };