fix: generation stopped working whenever the slide reviewer was switched off

savedFigureIds was declared inside the review branch, so with no reviewer
configured — the default, and what everyone is running — it was undefined by the
time the INSERT stringified it. JSON.stringify(undefined) is not a string, the
column is NOT NULL, and every generation failed with "Generation failed". `var`
is function-scoped, so nothing complained until the database did.

This is the second bug of exactly this shape in this file, so the test asserts
position rather than presence: the value must be declared before both the review
and the insert read it.

Found by the logging added in the same change, which is the other half of this
commit. Every modification now says what it did:

  [my-resources] refine id=29 path=deck outcome=applied 13→14 slides changed=yes
  [my-resources] refine id=37 path=markdown outcome=applied 2635→3018 chars changed=yes

CHANGED=no is warn-level and deliberately shouty, because that is the failure
worth catching: the response says success either way, the row updates, and the
download is identical — which is exactly how the deck bug went unnoticed. A
refusal logs its reason. ped_ai_resource_refine_total{path,outcome} counts the
same thing over time, so "did that modification do anything" is answerable
without watching logs live.

Verified across every path rather than the one that was broken: a deck
presentation modified and exported to both pptx and docx carries the change; a
legacy presentation with no stored deck still takes the markdown path and
carries it; an article generates, modifies and exports; and a presentation
generates with the reviewer off.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Dv6sqaY6Vq3ChZHMem3cnU

src/routes/myResources.js

@@ -349,11 +349,17 @@ router.post('/my-resources/generate', async function (req, res) {
     // columns are invisible to it. One pass, on generation only, and only when
     // an administrator has named a reviewer — a second pass costs as much as
     // the first and fixes far less.
+    // Computed here, before anything reads it. It used to be declared inside the
+    // review branch below, so with no reviewer configured — the default — it was
+    // undefined by the time the INSERT stringified it, and every generation
+    // failed on a NOT NULL constraint. `var` is function-scoped, so nothing
+    // complained until the database did.
+    var savedFigureIds = (ai.imageJobs || []).map(function (job) { return job.jobId; }).filter(Boolean);
+
     var reviewed = { reviewed: false, reason: 'not attempted' };
     if (deck) {
       var reviewModel = String(await db.getSetting('my_resources.review_model', '') || '');
       if (reviewModel) {
-        var figureIds = (ai.imageJobs || []).map(function (job) { return job.jobId; }).filter(Boolean);
         reviewed = await deckReview.review(deck, {
           model: reviewModel,
           callAI: callAI,
@@ -362,7 +368,7 @@ router.post('/my-resources/generate', async function (req, res) {
           mime: documentExport.FORMATS.pptx.mime,
           // Rendered without the figures: they are still being drawn at this
           // point, and a reviewer judges layout, not artwork.
-          pptx: await documentExport.renderDeck(deck, [], figureIds)
+          pptx: await documentExport.renderDeck(deck, [], savedFigureIds)
         });
         deck = reviewed.deck;
       }
@@ -374,14 +380,11 @@ router.post('/my-resources/generate', async function (req, res) {
     var markdown = deck ? deckSchema.toMarkdown(deck) : String((ai && ai.content) || '').trim();
     if (!markdown) return res.status(502).json({ error: 'The model returned nothing. Try again.' });
 
-    // The figures belong to the resource, or an exported deck has no way to
-    // include the pictures the author asked for.
-    var savedFigureIds = (ai.imageJobs || []).map(function (job) { return job.jobId; }).filter(Boolean);
     var row = await db.get(
       'INSERT INTO user_resources (user_id, title, kind, markdown, topic, grounded_count, image_ids, deck) ' +
       'VALUES (?, ?, ?, ?, ?, ?, ?, ?) RETURNING id, title, kind, topic, grounded_count, created_at',
       [req.user.id, deck && deck.title ? deck.title : firstHeading(markdown, topic), kind, markdown,
-       topic.slice(0, 500), corpus.sources.length, JSON.stringify(figureIds),
+       topic.slice(0, 500), corpus.sources.length, JSON.stringify(savedFigureIds),
        deck ? JSON.stringify(deck) : null]
     );
 
@@ -533,7 +536,8 @@ router.post('/my-resources/:id/refine', async function (req, res) {
     if (existingDeck && !revisedDeck) {
       // A reply that is not a deck would otherwise be saved as the markdown and
       // silently drop every layout the deck held.
-      console.warn('[my-resources] refine did not return a usable deck; nothing changed');
+      logRefine({ id: existing.id, path: 'deck', outcome: 'refused',
+                  detail: 'the model did not return a usable deck', instructions: instructions });
       return res.status(502).json({ error: 'That change could not be applied. Try wording it differently.' });
     }
     if (revisedDeck) {
@@ -557,6 +561,23 @@ router.post('/my-resources/:id/refine', async function (req, res) {
       [revised, firstHeading(revised), JSON.stringify(added),
        revisedDeck ? JSON.stringify(revisedDeck) : null, existing.id, req.user.id]
     );
+    // Said out loud, every time. A modification that changes nothing is the
+    // failure worth catching, and it is invisible from the response: the row
+    // updates, the title updates, and the file is identical.
+    logRefine({
+      id: existing.id,
+      path: revisedDeck ? 'deck' : 'markdown',
+      outcome: 'applied',
+      before: existingDeck ? (existingDeck.slides || []).length : existing.markdown.length,
+      after: revisedDeck ? (revisedDeck.slides || []).length : revised.length,
+      unchanged: revisedDeck
+        ? JSON.stringify(existingDeck.slides) === JSON.stringify(revisedDeck.slides)
+        : revised.trim() === String(existing.markdown || '').trim(),
+      figures: added.length,
+      model: ai && ai.model,
+      instructions: instructions
+    });
+
     res.json({
       success: true, resource: row, markdown: revised,
       grounding: { used: Boolean(sources.corpus.context), count: sources.corpus.sources.length,
@@ -572,6 +593,31 @@ router.post('/my-resources/:id/refine', async function (req, res) {
   }
 });
 
+// What a modification actually did, in one line, so "it spat out the same thing"
+// can be checked rather than guessed at. The unchanged case is the one worth
+// watching: the response says success either way, and the download is identical.
+function logRefine(event) {
+  var unit = event.path === 'deck' ? ' slides' : ' chars';
+  var parts = ['[my-resources] refine id=' + event.id, 'path=' + event.path, 'outcome=' + event.outcome];
+  if (event.outcome === 'applied') {
+    parts.push(event.before + '\u2192' + event.after + unit);
+    parts.push(event.unchanged ? 'CHANGED=no' : 'changed=yes');
+    if (event.figures) parts.push('figures=+' + event.figures);
+    if (event.model) parts.push('model=' + event.model);
+  } else if (event.detail) {
+    parts.push('(' + event.detail + ')');
+  }
+  parts.push('instruction="' + String(event.instructions || '').slice(0, 70).replace(/\s+/g, ' ') + '"');
+  var line = parts.join(' ');
+  if (event.outcome !== 'applied' || event.unchanged) console.warn(line); else console.log(line);
+  try {
+    require('../utils/metrics').resourceRefines.inc({
+      path: event.path,
+      outcome: event.outcome === 'applied' ? (event.unchanged ? 'unchanged' : 'changed') : event.outcome
+    });
+  } catch (e) { /* never worth an error here */ }
+}
+
 // One line per distinct thing a deck wanted and could not have, plus a counter
 // so it can be watched over time. Never fails anything: this is a note to
 // whoever decides what to build next, not part of the generation.

src/utils/metrics.js

@@ -82,9 +82,19 @@ const deckVocabularyGaps = new client.Counter({
   registers: [register]
 });
 
+// What modifying a resource did. "unchanged" is the one to watch: it means a
+// person was told their change was applied and got back exactly what they had.
+const resourceRefines = new client.Counter({
+  name: 'ped_ai_resource_refine_total',
+  help: 'Modifications to a generated resource, by path taken and what happened',
+  labelNames: ['path', 'outcome'],
+  registers: [register]
+});
+
 module.exports = {
   metricsHandler,
   metricsMiddleware,
   deckVocabularyGaps,
+  resourceRefines,
   register
 };

test/my-resources.test.js

@@ -279,6 +279,23 @@ test('the library is bounded, searchable, and drives the modify picker', () => {
   assert.match(js, /library\.length\s*\n?\s*\? 'Nothing matches/);
 });
 
+test('the figure ids are computed before anything reads them', () => {
+  const route = read('src/routes/myResources.js');
+  // They were declared inside the slide-review branch, so with no reviewer
+  // configured — the default — the value was undefined by the time the INSERT
+  // stringified it, and every generation failed on a NOT NULL constraint. `var`
+  // is function-scoped, so nothing complained until the database did. This is
+  // the second bug of exactly this shape in this file.
+  const body = route.slice(route.indexOf("router.post('/my-resources/generate'"));
+  const declared = body.indexOf('var savedFigureIds =');
+  const reviewUses = body.indexOf('renderDeck(deck, [], savedFigureIds)');
+  const insertUses = body.indexOf('JSON.stringify(savedFigureIds)');
+  assert.ok(declared > -1 && declared < reviewUses, 'declared before the review reads it');
+  assert.ok(declared < insertUses, 'and before the insert reads it');
+  // Declared at the top level of the handler, not inside a branch.
+  assert.doesNotMatch(body.slice(0, insertUses), /if \(reviewModel\) \{\s*\n\s*var savedFigureIds/);
+});
+
 test('modifying a presentation edits the deck, not only its markdown', () => {
   const route = read('src/routes/myResources.js');
   // Export renders from the stored deck. Refine edited the markdown beside it
@@ -291,7 +308,7 @@ test('modifying a presentation edits the deck, not only its markdown', () => {
 
   // A reply that is not a deck must not be saved as markdown: that would drop
   // every layout the deck held while looking like it worked.
-  assert.match(route, /refine did not return a usable deck; nothing changed/);
+  assert.match(route, /outcome: 'refused',\s*\n\s*detail: 'the model did not return a usable deck'/);
   assert.match(route, /That change could not be applied\. Try wording it differently\./);
 
   // An article has no deck and keeps the markdown path.

test/slide-spec.test.js

@@ -98,7 +98,7 @@ test('a resource remembers its figures, so an export can include them', () => {
   // They were queued and shown on screen, but nothing tied them to the
   // resource, so an exported deck could never contain them.
   assert.match(read('migrations/1780500000000_resource-images.js'), /image_ids JSONB/);
-  assert.match(route, /var figureIds = \(ai\.imageJobs \|\| \[\]\)\.map/);
+  assert.match(route, /var savedFigureIds = \(ai\.imageJobs \|\| \[\]\)\.map/);
   // "Add two more diagrams" means more, not instead.
   assert.match(route, /image_ids = image_ids \|\| \?::jsonb/);
   assert.match(route, /async function collectFigures\(ids, user, dir\)/);