diff --git a/src/routes/myResources.js b/src/routes/myResources.js index d043b631..5579b2b3 100644 --- a/src/routes/myResources.js +++ b/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. diff --git a/src/utils/metrics.js b/src/utils/metrics.js index 3cc242eb..a525e483 100644 --- a/src/utils/metrics.js +++ b/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 }; diff --git a/test/my-resources.test.js b/test/my-resources.test.js index ef837457..da58bac8 100644 --- a/test/my-resources.test.js +++ b/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. diff --git a/test/slide-spec.test.js b/test/slide-spec.test.js index 735c450e..cf16cff0 100644 --- a/test/slide-spec.test.js +++ b/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\)/);