From e244ee5240d50181bb98eba3fd96c76d362cf847 Mon Sep 17 00:00:00 2001 From: Daniel Date: Sat, 12 Sep 2026 06:55:16 +0200 Subject: [PATCH] feat: modifying a deck can see it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The model editing a deck could not see it, which made most of what people actually ask for unanswerable: "that slide is too crowded", "the diagram is in the wrong place", "this one looks empty" are facts about the rendered page, not about the JSON. When a vision model is configured, modifying now renders the current deck — with its figures, unlike the review pass, which runs while they are still being drawn — and hands the model one image per slide alongside the JSON. Same pipeline as review, reused rather than reimplemented: pptx, Gotenberg, PDF, pdftoppm, capped at MAX_SLIDES. The vision model then does the editing, which is a second and separately measured benefit. On a real 20-slide deck, ds-deepseek-v4-flash returned the deck unchanged for "make it better" — the echo reported yesterday — while openrouter-gemini-3.8-flash applied it. So the stronger model fixes the echo even without sight. A model the author picks explicitly still wins over both. The result is rendered and reviewed again. Generation-only was the old rule, on the reasoning that refining is a text edit; it is not. The edit is made against how the deck looked before it, so a slide that gains two bullets only overflows once it is rendered again. The reviewer may reposition but is held to the same words, so a verification pass cannot quietly undo what was just asked for. Whether an instruction achieved anything is judged on the model's edit, before the reviewer runs, or a reviewer nudging a slide into two columns would mask an instruction that did nothing. Sight is an upgrade, never a dependency: no vision model, Gotenberg down, a render that fails — each falls through to editing blind, and a test covers each of those paths. Verified against two mutations: keeping the text model when images are attached, and dropping the verification pass, each fail exactly one test. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Dv6sqaY6Vq3ChZHMem3cnU --- docs/my-resources.md | 25 +++++++- public/js/myResources.js | 7 ++- src/routes/myResources.js | 103 +++++++++++++++++++++++++++++-- src/utils/deckReview.js | 5 +- test/deck-review.test.js | 16 +++-- test/my-resources-refine.test.js | 94 ++++++++++++++++++++++++++-- test/my-resources.test.js | 2 +- 7 files changed, 234 insertions(+), 18 deletions(-) diff --git a/docs/my-resources.md b/docs/my-resources.md index 20098be7..f0017e4a 100644 --- a/docs/my-resources.md +++ b/docs/my-resources.md @@ -262,8 +262,29 @@ slide and a nine-item list that wants two columns are invisible to it. With a reviewer configured, each generated deck is rendered to PDF through Gotenberg, rasterised to one PNG per slide with `pdftoppm`, and shown to a vision model. -One pass, on generation only. A second pass costs as much as the first and fixes -far less, and refining is a text edit. +One pass per change — on generation, and again on the result of a modification. +Modifying was excluded at first on the reasoning that refining is a text edit. +It is not: an edit is made against how the deck looked *before* it, so a slide +that gains two bullets only overflows once it is rendered again, which is +exactly what the reviewer exists to catch. + +### Modifying can see the deck too + +When a vision model is configured, modifying renders the current deck — with +its figures — and hands the model one image per slide alongside the JSON. Most +of what people ask for while modifying is about the rendered page: "that slide +is crowded", "the diagram is in the wrong place", "this one looks empty". None +of it is answerable from the JSON. + +The vision model then does the editing, which is a second benefit measured +before this was built: on a real 20-slide deck, `ds-deepseek-v4-flash` returned +the deck unchanged for "make it better" and `openrouter-gemini-3.8-flash` did +not. A model the author picks explicitly still wins over both. + +Sight is an upgrade, never a dependency. No vision model configured, Gotenberg +down, a render that fails — each falls through to editing the JSON blind, which +is what this did before it could see at all, and none of them may cost someone +their modification. The reviewer must be able to see. Saving `my_resources.review_model` asks the gateway what it reports for that model and refuses one whose `supports_vision` diff --git a/public/js/myResources.js b/public/js/myResources.js index 5706cb7c..8d551046 100644 --- a/public/js/myResources.js +++ b/public/js/myResources.js @@ -378,7 +378,12 @@ say('The model returned it unchanged — nothing was modified. ' + 'Try naming the slide or section to change, and what to change about it.', 'bad'); } else { - say('Applied' + (g.used ? ', using ' + g.count + ' library excerpt' + (g.count === 1 ? '' : 's') : '') + + // Whether it could see the slides is worth saying: it is the + // difference between "slide 4 looks crowded" being actionable and + // being guesswork, and it explains why this took longer. + say('Applied' + + (data.saw ? ', after looking at all ' + data.saw + ' slides' : '') + + (g.used ? ', using ' + g.count + ' library excerpt' + (g.count === 1 ? '' : 's') : '') + '. Download it to see the result.', 'good'); } reportSearches(data.searches); diff --git a/src/routes/myResources.js b/src/routes/myResources.js index f51aaa08..e117a081 100644 --- a/src/routes/myResources.js +++ b/src/routes/myResources.js @@ -535,7 +535,7 @@ router.post('/my-resources/:id/refine', async function (req, res) { if (!instructions) return res.status(400).json({ error: 'Say what to change' }); var existing = await db.get( - 'SELECT id, kind, topic, markdown, deck FROM user_resources WHERE id = ? AND user_id = ?', + 'SELECT id, kind, topic, markdown, deck, image_ids FROM user_resources WHERE id = ? AND user_id = ?', [parseInt(req.params.id, 10), req.user.id] ); if (!existing) return res.status(404).json({ error: 'Not found' }); @@ -595,12 +595,31 @@ router.post('/my-resources/:id/refine', async function (req, res) { 'never a real patient. Returning the markdown is still required; a tool call is not a ' + 'substitute for it, and no image tag or URL goes into the markdown.'; + // Let it look at the deck it is about to edit, when a vision model is + // configured. Measured before building this: the text model returned the + // deck unchanged for "make it better", and a stronger model fixed that + // without seeing anything — but sight is what makes the visual half of the + // instructions answerable at all, and that is most of what people ask for + // while modifying. + var visionModel = existingDeck + ? String(await db.getSetting('my_resources.review_model', '') || '') : ''; + var slideViews = visionModel + ? await renderDeckForSight(existingDeck, existing.image_ids, req.user) + : []; + var sight = !slideViews.length ? '' : + '\n\nYou can see the deck as it renders now: ' + slideViews.length + ' image' + + (slideViews.length === 1 ? '' : 's') + ', one per slide, in order. The first image is ' + + 'slide index 0 in the JSON below. Use them for anything the instruction says about how ' + + 'a slide looks — crowded, empty, a figure in the wrong place, text running off the ' + + 'bottom. The JSON is what you edit; the images only tell you what it currently ' + + 'produces.'; + var messages = [{ role: 'user', content: existingDeck ? 'Revise the following slide deck according to the instruction. Return ONLY the ' + 'complete revised JSON object, in exactly the same shape, no commentary and no code ' + 'fences. Keep every slide that the instruction does not ask you to change, including ' + 'its layout and its "image_job" values, and keep any References slide last.\n\n' + - 'INSTRUCTION: ' + instructions + illustration + material + + 'INSTRUCTION: ' + instructions + illustration + material + sight + '\n\nThe layouts available are:\n' + deckSchema.instructions( (existingDeck.slides || []).length, 0) + '\n\nDECK JSON:\n' + JSON.stringify({ slides: existingDeck.slides }) @@ -610,7 +629,18 @@ router.post('/my-resources/:id/refine', async function (req, res) { 'References section at the end.\n\nINSTRUCTION: ' + instructions + illustration + material + '\n\nMARKDOWN:\n"""\n' + existing.markdown + '\n"""' }]; // The reply restates the whole resource, so it needs room for one. - var options = { model: await resolveModel(req.body.model), temperature: 0.2, maxTokens: 16000 }; + // + // The vision model does the editing when there is one — both because it is + // the only one that can act on what it sees, and because it follows a vague + // instruction better: measured on a real 20-slide deck, the text model + // echoed "make it better" back unchanged and the vision model did not. + // An explicit choice by the author still wins over both. + var options = { + model: req.body.model ? await resolveModel(req.body.model) + : (visionModel && slideViews.length ? visionModel : await resolveModel('')), + temperature: 0.2, maxTokens: 16000 + }; + if (slideViews.length) options.images = slideViews; // Deck mode declares its figures; only the markdown path needs the tool. var tools = sources.wantsImages && !existingDeck ? resourceImages.tools : []; var callOptions = tools.length ? Object.assign({}, options, { tools: tools }) : options; @@ -647,6 +677,31 @@ router.post('/my-resources/:id/refine', async function (req, res) { } } + // Look at the result. The edit was made against how the deck looked before + // it; a slide that gained two bullets now overflows, and only rendering it + // again shows that. Same reviewer as generation, which may reposition but + // is held to the same words — so a verification pass cannot quietly undo + // the change that was just asked for. + // Judged before the reviewer touches it. The question "did my instruction do + // anything" is about the model's edit; a reviewer that nudged a slide into + // two columns would otherwise mask an instruction that achieved nothing. + var echoedBack = revisedDeck + ? JSON.stringify(existingDeck.slides) === JSON.stringify(revisedDeck.slides) + : false; + + var verified = { reviewed: false, reason: 'not attempted' }; + if (revisedDeck && visionModel) { + verified = await deckReview.review(revisedDeck, { + model: visionModel, + callAI: callAI, + extractJson: deckBuild.extractJson, + gotenberg: documentExport.GOTENBERG, + mime: documentExport.FORMATS.pptx.mime, + pptx: await documentExport.renderDeck(revisedDeck, [], figureIdList(existing.image_ids)) + }); + revisedDeck = verified.deck; + } + var revised = revisedDeck ? deckSchema.toMarkdown(revisedDeck) : String((ai && ai.content) || '').trim(); if (!revised) return res.status(502).json({ error: 'The model returned nothing. Try again.' }); @@ -668,8 +723,7 @@ router.post('/my-resources/:id/refine', async function (req, res) { // while answering "Applied" left the one person who could do something about // it — the person who asked for the change — reading a success message next // to an identical file. - var unchanged = revisedDeck - ? JSON.stringify(existingDeck.slides) === JSON.stringify(revisedDeck.slides) + var unchanged = revisedDeck ? echoedBack : revised.trim() === String(existing.markdown || '').trim(); logRefine({ id: existing.id, @@ -685,6 +739,8 @@ router.post('/my-resources/:id/refine', async function (req, res) { res.json({ success: true, resource: row, markdown: revised, unchanged: unchanged, + saw: slideViews.length, + review: { applied: verified.reviewed, reason: verified.reason }, grounding: { used: Boolean(sources.corpus.context), count: sources.corpus.sources.length, reason: sources.corpus.reason || null }, searches: sources.searches, @@ -698,6 +754,43 @@ router.post('/my-resources/:id/refine', async function (req, res) { } }); +// ── Showing the deck to the model that is about to change it ──────────────── +// The model that writes a deck never sees it, and that is just as true when it +// is editing one. Most of what people ask for while modifying is about the +// rendered page — "that slide is too crowded", "the diagram is in the wrong +// place", "this one looks empty" — and none of it is answerable from the JSON. +// +// So the deck is rendered the same way a download is, turned into one image per +// slide, and handed to the model along with the JSON. Same pipeline as the +// review pass, reused rather than reimplemented: pptx -> Gotenberg -> PDF -> +// pdftoppm -> PNG, capped at deckReview.MAX_SLIDES. +// +// Never fatal. A gateway that is down, a render that fails, a deck too big — +// all of them fall through to editing the JSON blind, which is what this did +// before it could see at all. +async function renderDeckForSight(deck, figureIds, user) { + var fsp = require('fs/promises'); + var os = require('os'); + var pathMod = require('path'); + var dir = null; + try { + dir = await fsp.mkdtemp(pathMod.join(os.tmpdir(), 'sight-')); + // With its figures, unlike the review pass: a review runs while they are + // still being drawn, but by the time someone is modifying they exist, and + // "move the picture" is unanswerable without seeing the picture. + var figures = await collectFigures(figureIds, user, dir); + var pptx = await documentExport.renderDeck(deck, figures, figureIdList(figureIds)); + return await deckReview.slideImages(pptx, documentExport.GOTENBERG, + documentExport.FORMATS.pptx.mime); + } catch (e) { + logger.warn('[my-resources] could not render the deck to look at it; editing blind', + { error: e.message }); + return []; + } finally { + if (dir) await require('fs/promises').rm(dir, { recursive: true, force: true }).catch(function () {}); + } +} + // 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. diff --git a/src/utils/deckReview.js b/src/utils/deckReview.js index b58697e8..467e3fe4 100644 --- a/src/utils/deckReview.js +++ b/src/utils/deckReview.js @@ -266,4 +266,7 @@ async function review(deck, options) { } } -module.exports = { review, fingerprint, movedOnly, applyChanges, instructions, MAX_SLIDES, MAX_CHANGES, RENDER_DPI }; +// slideImages is exported because modifying a deck wants the same picture of it +// that reviewing does — same render path, same DPI, same per-slide cap — and two +// copies of this would drift. +module.exports = { review, slideImages, fingerprint, movedOnly, applyChanges, instructions, MAX_SLIDES, MAX_CHANGES, RENDER_DPI }; diff --git a/test/deck-review.test.js b/test/deck-review.test.js index fa656cd0..7f3c8f4a 100644 --- a/test/deck-review.test.js +++ b/test/deck-review.test.js @@ -137,14 +137,22 @@ test('nothing about the review can fail a generation', async () => { assert.match(long.reason, /too long/); }); -test('the reviewer is admin-chosen, off by default, and one pass on generation only', () => { +test('the reviewer is admin-chosen, off by default, and runs once per change', () => { const route = read('src/routes/myResources.js'); assert.match(route, /db\.getSetting\('my_resources\.review_model', ''\)/); assert.match(route, /if \(reviewModel\) \{/, 'nothing happens without one'); - // Generation only: refining a deck is a text edit, and re-reviewing costs as - // much as the first pass while fixing far less. + + // Modifying reviews too, which generation-only used to forbid. The reasoning + // changed with the evidence: an edit is made against how the deck looked + // *before* it, so a slide that gains two bullets only overflows once it is + // rendered again — exactly the class of fault the reviewer exists for. The + // old rule assumed refining was a text edit; it is a layout edit as often as + // not. const refine = route.slice(route.indexOf("router.post('/my-resources/:id/refine'")); - assert.doesNotMatch(refine, /deckReview/); + assert.match(refine, /deckReview\.review\(revisedDeck/); + assert.match(refine, /if \(revisedDeck && visionModel\)/, 'and only when one is configured'); + // Still one pass. The verification runs on the result, never in a loop. + assert.equal((refine.match(/deckReview\.review\(/g) || []).length, 1); // The key has to be writable, or saving it silently does nothing. assert.match(read('src/routes/adminConfig.js'), /'clinical_assistant\.', 'my_resources\.'\]/); // And the image can actually rasterise a deck. diff --git a/test/my-resources-refine.test.js b/test/my-resources-refine.test.js index fc21a554..d3258797 100644 --- a/test/my-resources-refine.test.js +++ b/test/my-resources-refine.test.js @@ -27,6 +27,7 @@ function router(t, overrides = {}) { const module = { exports: {} }; const aiCalls = []; const replies = overrides.replies ? overrides.replies.slice() : null; + const reviewCalls = []; const updates = []; const row = Object.assign({ id: 5, user_id: 7, title: 'Croup', kind: 'presentation', topic: 'croup', @@ -49,7 +50,11 @@ function router(t, overrides = {}) { }, all: async () => [], run: async () => ({}), - getSetting: async key => (key === 'clinical_assistant.image_model' ? 'synthetic-image' : '') + getSetting: async key => { + if (key === 'clinical_assistant.image_model') return 'synthetic-image'; + if (key === 'my_resources.review_model') return overrides.visionModel || ''; + return ''; + } }, '../middleware/auth': { authMiddleware: (req, res, next) => next() }, '../utils/ai': { @@ -64,8 +69,12 @@ function router(t, overrides = {}) { }, '../utils/deckBuild': deckBuild, '../utils/deckSchema': require('../src/utils/deckSchema'), - '../utils/deckReview': { enabled: async () => false, review: async d => d }, - '../utils/documentExport': { FORMATS: {}, isSupported: () => false, filename: () => 'x', mimeFor: () => '', render: async () => ({}), renderDeck: async () => ({}) }, + '../utils/deckReview': { + slideImages: async () => (overrides.slideImages || []), + review: async (deck) => { reviewCalls.push(deck); return { deck: deck, reviewed: true, reason: 'ok' }; }, + MAX_SLIDES: 20 + }, + '../utils/documentExport': { FORMATS: { pptx: { mime: 'application/vnd.openxmlformats-officedocument.presentationml.presentation' } }, GOTENBERG: 'http://gotenberg:3000', isSupported: () => false, filename: () => 'x', mimeFor: () => '', render: async () => ({}), renderDeck: async () => Buffer.from('pptx') }, '../utils/generatedImages': { service: () => ({ get: async () => null, asset: async () => null }), workflows: ['my_resources'] }, '../utils/learningRetrieval': { retrieve: async () => ({ context: '', sources: [], reason: null }) }, '../utils/logger': quiet, @@ -95,7 +104,7 @@ function router(t, overrides = {}) { await handler({ body, params: { id: '5' }, query: {}, user: { id: 7 } }, res); return res; } - return { request, aiCalls, updates }; + return { request, aiCalls, updates, reviewCalls }; } test('modifying a deck asks for a deck, even when illustration is on', async () => { @@ -231,3 +240,80 @@ test('a deck that parses first time is never asked for twice', async () => { assert.equal(r.aiCalls.length, 1, 'the retry costs a call and must only happen on failure'); assert.equal(res.body.deckFallback, null); }); + +// ── Modifying with sight ─────────────────────────────────────────────────── + +const TWO_PNGS = [ + { mimeType: 'image/png', dataBase64: 'aW1hZ2Ux' }, + { mimeType: 'image/png', dataBase64: 'aW1hZ2Uy' } +]; + +const EDITED = JSON.stringify({ + slides: [ + { type: 'title', title: 'Croup' }, + { type: 'bullets', title: 'Features', bullets: ['Barking cough', 'Stridor', 'Hoarse voice'] } + ] +}); + +test('modifying a deck shows the model what the deck currently looks like', async () => { + // Most of what people ask for while modifying is about the rendered page — + // "that slide is crowded", "the diagram is in the wrong place" — and none of + // it is answerable from the JSON alone. + const r = router(null, { visionModel: 'seeing-model', slideImages: TWO_PNGS, replies: [EDITED] }); + const res = await r.request('post', '/my-resources/:id/refine', { + instructions: 'slide 2 looks crowded, split it' + }); + + assert.equal(res.statusCode, 200); + assert.equal(r.aiCalls[0].options.images, TWO_PNGS, 'the rendered slides are attached'); + assert.equal(r.aiCalls[0].options.model, 'seeing-model', 'and the model that can see does the editing'); + assert.match(r.aiCalls[0].messages[0].content, /You can see the deck as it renders now: 2 images/); + assert.match(r.aiCalls[0].messages[0].content, /first image is slide index 0/, + 'the images must be anchored to the JSON or a slide reference means nothing'); + assert.equal(res.body.saw, 2); +}); + +test('the edited deck is rendered again and checked', async () => { + // The edit was made against how the deck looked *before* it. A slide that + // gained two bullets only overflows once it is rendered again. + const r = router(null, { visionModel: 'seeing-model', slideImages: TWO_PNGS, replies: [EDITED] }); + const res = await r.request('post', '/my-resources/:id/refine', { instructions: 'add a feature' }); + + assert.equal(r.reviewCalls.length, 1, 'the result goes back past the reviewer'); + assert.equal(res.body.review.applied, true); +}); + +test('with no vision model configured, modify still works and never renders', async () => { + // Sight is an upgrade, not a dependency. Nothing here may become a new way + // for a modification to fail. + const r = router(null, { replies: [EDITED] }); + const res = await r.request('post', '/my-resources/:id/refine', { instructions: 'add a feature' }); + + assert.equal(res.statusCode, 200); + assert.equal(res.body.saw, 0); + assert.equal(r.aiCalls[0].options.images, undefined); + assert.equal(r.reviewCalls.length, 0, 'and nothing is rendered or reviewed'); +}); + +test('a render that fails falls through to editing blind rather than failing', async () => { + // Gotenberg down, LibreOffice wedged, a deck too big: none of them may cost + // the author their modification. + const r = router(null, { visionModel: 'seeing-model', slideImages: [], replies: [EDITED] }); + const res = await r.request('post', '/my-resources/:id/refine', { instructions: 'add a feature' }); + + assert.equal(res.statusCode, 200); + assert.equal(res.body.saw, 0); + assert.equal(r.aiCalls[0].options.images, undefined); + assert.notEqual(r.aiCalls[0].options.model, 'seeing-model', + 'no images means no reason to pay for the vision model'); +}); + +test('an echo is still reported as an echo, even when the reviewer moved something', async () => { + // The reviewer may reposition, which would otherwise make an instruction that + // achieved nothing look like it had worked. + const r = router(null, { visionModel: 'seeing-model', slideImages: TWO_PNGS, + replies: [JSON.stringify({ slides: DECK.slides })] }); + const res = await r.request('post', '/my-resources/:id/refine', { instructions: 'make it better' }); + + assert.equal(res.body.unchanged, true, 'judged on the model edit, before the reviewer ran'); +}); diff --git a/test/my-resources.test.js b/test/my-resources.test.js index 361b5ce0..c18610e6 100644 --- a/test/my-resources.test.js +++ b/test/my-resources.test.js @@ -317,7 +317,7 @@ test('modifying a presentation edits the deck, not only its markdown', () => { // and left the deck alone, so a modification reported success, updated the // library row, and produced a byte-identical download. Measured: markdown // gained the new slide, the deck did not, and the exported pptx did not. - assert.match(route, /SELECT id, kind, topic, markdown, deck FROM user_resources/); + assert.match(route, /SELECT id, kind, topic, markdown, deck, image_ids FROM user_resources/); assert.match(route, /var revisedDeck = existingDeck \? deckBuild\.parse\(ai && ai\.content\) : null;/); assert.match(route, /deck = COALESCE\(\?::jsonb, deck\)/, 'and the deck is written back');