From c1ccb5739b4a2cb3d06301d8b2a1009e23a2984d Mon Sep 17 00:00:00 2001 From: Twest2 Date: Fri, 3 Jul 2026 22:57:50 -0500 Subject: [PATCH] Add optimistic revisions, keep dirty state on failed saves, quarantine corrupt data MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 1 of the improvement plan (PR 4 of the sequence): stop concurrent whole-object saves from losing edits, stop failed saves from reporting clean, and stop corrupt user data from silently vanishing. Revisions (core/schema.js, core/store.js): - Every guide and step carries a monotonic `revision`, bumped on each store write. Legacy v1 data without the field reads as revision 0 and upgrades on its next save — no migration pass, no data rewrite. - saveGuide/saveStep accept { expectedRevision } for compare-and-swap saves; a mismatch throws RevisionConflictError instead of clobbering. Direct user edits pass no expectation (the user is the authority); background writers must pass one. Stale AI responses (app/text-intel.js): - generateStepPatch snapshots the step revision before the (slow) model call, re-reads the step after it, and saves with the original expectedRevision. A user edit made during generation now surfaces as "the step changed while AI was generating; nothing was overwritten" — previously the AI response silently overwrote the newer edit. Autosave truthfulness (app/renderer/editor.js): - flushStep/flushGuide cleared the dirty flag BEFORE awaiting the IPC save, so a rejected save (invoked via a debounce that never handled rejections) lost the visible dirty state. The flag is now cleared only after a durable save; failures keep it dirty, surface a persistent saveError in editor meta, toast the user, and retry on the next edit or explicit save. - Navigating away from the editor flushes pending debounced saves so the last edit can never be dropped by a view switch. Corruption quarantine (core/store.js): - listGuides/listSteps used to silently skip unreadable entries — a corrupt guide just vanished from the library. Corrupt guide/step directories are now moved to library/quarantine (original bytes preserved) and recorded in a recovery report (store.getRecoveryReport()) for the UI. Empty in-progress directories are still skipped quietly — absence of guide.json is not corruption. Tests: revision increments, stale-CAS rejection with user edit surviving, CAS success path, guide CAS, v1 no-revision upgrade, guide/step quarantine with preserved bytes + recovery report, empty-dir non-quarantine, AI stale-write rejection end-to-end (user edit mid-generation survives) and the clean-apply path. 240 unit tests pass; startup smoke and sample-artifact E2E pass. Co-Authored-By: Claude Fable 5 --- app/renderer/editor.js | 38 +++++++- app/text-intel.js | 26 +++++- core/schema.js | 5 + core/store.js | 98 ++++++++++++++++++-- tests/unit/ai-network.test.js | 70 ++++++++++++++ tests/unit/store-revisions.test.js | 143 +++++++++++++++++++++++++++++ 6 files changed, 364 insertions(+), 16 deletions(-) create mode 100644 tests/unit/store-revisions.test.js diff --git a/app/renderer/editor.js b/app/renderer/editor.js index b0c1a4b..3da28dc 100644 --- a/app/renderer/editor.js +++ b/app/renderer/editor.js @@ -124,6 +124,7 @@ class GuideEditor { this.currentZoom = 'fit'; this.pendingSave = false; this.pendingGuideSave = false; + this.saveError = null; this.canvasHistory = []; this.canvasFuture = []; this.beforeCanvasSnapshot = null; @@ -151,9 +152,14 @@ class GuideEditor { setActive(active) { this.active = Boolean(active); - // Leaving the editor cancels any in-flight AI request for this guide so a - // slow response can't resolve against a guide the user has closed. if (!this.active && this.guideId) { + // Leaving the editor: flush pending debounced saves so navigation can + // never drop the last edit (failures keep the dirty state and retry), + // and cancel any in-flight AI request for this guide so a slow response + // can't resolve against a guide the user has closed. + if (this.pendingSave || this.pendingGuideSave) { + this.saveAll().catch(() => {}); + } api.ai.cancel({ guideId: this.guideId }).catch(() => {}); } } @@ -319,6 +325,7 @@ class GuideEditor { selectedAnnotationId: this.selectedAnnotationId, linked: Boolean(this.guide && this.guide.linkedSource), dirty: this.pendingSave || this.pendingGuideSave || this.descriptionDirty || this.titleDirty, + saveError: this.saveError || null, view: 'editor', }; } @@ -1424,8 +1431,22 @@ class GuideEditor { async flushStep(step = this.currentStep) { if (!step) return; + // Clear the dirty flag only AFTER a durable save. Clearing it first meant + // a rejected IPC save silently lost the unsaved state (and this runs from + // a debounce that does not handle rejections). Keep it dirty on failure, + // surface it, and retry on the next edit or explicit save. + let saved; + try { + saved = await api.step.save({ guideId: this.guideId, step }); + } catch (err) { + this.pendingSave = true; + this.saveError = (err && err.message) || 'Save failed'; + this.emitMeta(); + this.onToast('Could not save this step — your changes are kept. Retrying…', { error: true }); + return null; + } this.pendingSave = false; - const saved = await api.step.save({ guideId: this.guideId, step }); + this.saveError = null; const committed = this.commitSavedStep(saved); if (this.selectedStepId === committed.stepId) { this.renderStepList(); @@ -1470,8 +1491,17 @@ class GuideEditor { async flushGuide() { if (!this.guide) return; + try { + await api.guide.save({ guide: this.guide }); + } catch (err) { + this.pendingGuideSave = true; + this.saveError = (err && err.message) || 'Save failed'; + this.emitMeta(); + this.onToast('Could not save guide details — your changes are kept. Retrying…', { error: true }); + return; + } this.pendingGuideSave = false; - await api.guide.save({ guide: this.guide }); + this.saveError = null; this.emitMeta(); } diff --git a/app/text-intel.js b/app/text-intel.js index 7183bf2..6aed4f0 100644 --- a/app/text-intel.js +++ b/app/text-intel.js @@ -624,6 +624,10 @@ public static class Win32 { if (!guide || !step) { return { ok: false, reason: 'Guide or step not found.' }; } + // Snapshot the revision now: AI generation is slow, and the user may + // edit the step meanwhile. We save with this expectedRevision so a + // response built from stale data cannot overwrite a newer user edit. + const baseRevision = Number.isInteger(step.revision) ? step.revision : 0; const currentBlock = blockId ? [...(step.textBlocks || []), ...(step.codeBlocks || []), ...(step.tableBlocks || [])].find((b) => b.id === blockId) || null @@ -716,8 +720,26 @@ public static class Win32 { guideId, }); const patch = normalizeAiPatch(raw); - const updated = applyAiPatchToStep(step, patch, { target, blockId }); - const saved = this.store.saveStep(guideId, updated); + // Re-read the step: while generation ran, a capture auto-doc or another + // background write may have advanced it. Apply the patch to the current + // step and save with the original expected revision so a user edit made + // during generation causes a conflict instead of a silent overwrite. + let currentStep = step; + try { + currentStep = this.store.getStep(guideId, stepId) || step; + } catch { + currentStep = step; + } + const updated = applyAiPatchToStep(currentStep, patch, { target, blockId }); + let saved; + try { + saved = this.store.saveStep(guideId, updated, { expectedRevision: baseRevision }); + } catch (err) { + if (err && err.code === 'STEPFORGE_REVISION_CONFLICT') { + return { ok: false, reason: 'The step changed while AI was generating; nothing was overwritten.' }; + } + throw err; + } return { ok: true, step: saved, patch }; } catch (err) { return { ok: false, reason: err && err.message ? err.message : 'AI generation failed.' }; diff --git a/core/schema.js b/core/schema.js index 7bbd122..c141715 100644 --- a/core/schema.js +++ b/core/schema.js @@ -52,6 +52,9 @@ function createGuide(fields = {}) { favorite: Boolean(fields.favorite), linkedSource: fields.linkedSource || null, exportProfiles: { ...(fields.exportProfiles || {}) }, + // Monotonic revision for optimistic concurrency. Absent in v1 data (reads + // as 0), bumped on every store write. + revision: Number.isInteger(fields.revision) && fields.revision >= 0 ? fields.revision : 0, }; } @@ -89,6 +92,8 @@ function createStep(fields = {}) { captureMetadata: (fields.captureMetadata && typeof fields.captureMetadata === 'object' && !Array.isArray(fields.captureMetadata)) ? { ...fields.captureMetadata } : null, + // Monotonic revision for optimistic concurrency (see createGuide). + revision: Number.isInteger(fields.revision) && fields.revision >= 0 ? fields.revision : 0, }; } diff --git a/core/store.js b/core/store.js index 8cf1eb3..bdc4bf5 100644 --- a/core/store.js +++ b/core/store.js @@ -12,6 +12,22 @@ const { } = require('./schema'); const { sanitizeHtml } = require('./sanitize'); +/** + * Thrown by revision-aware saves when the on-disk revision no longer matches + * the caller's expectation — i.e. someone else wrote in between. Callers that + * pass expectedRevision (background/AI/capture writes) use this to avoid + * clobbering a newer user edit. + */ +class RevisionConflictError extends Error { + constructor(kind, id, expected, actual) { + super(`${kind} ${id} changed since it was read (expected revision ${expected}, found ${actual})`); + this.name = 'RevisionConflictError'; + this.code = 'STEPFORGE_REVISION_CONFLICT'; + this.expected = expected; + this.actual = actual; + } +} + /** * Folder-based guide store. One directory per guide, one directory per step, * all JSON written atomically. This is the only module that knows the @@ -27,21 +43,49 @@ class GuideStore { this.guidesDir = path.join(this.libraryDir, 'guides'); this.indexDir = path.join(this.libraryDir, 'index'); this.trashDir = path.join(this.libraryDir, 'trash'); + this.quarantineDir = path.join(this.libraryDir, 'quarantine'); this.tempDir = path.join(rootDir, 'temp'); this.sharedLinksDir = path.join(rootDir, 'shared-links'); this.foldersFile = path.join(this.libraryDir, 'folders.json'); + // In-memory log of files quarantined this session (corrupt/unreadable), + // surfaced to the UI instead of silently vanishing. + this.recoveryReport = []; this.ensureLayout(); } ensureLayout() { for (const dir of [ this.settingsDir, this.templatesDir, this.guidesDir, this.indexDir, - this.trashDir, this.tempDir, this.sharedLinksDir, + this.trashDir, this.quarantineDir, this.tempDir, this.sharedLinksDir, ]) { fs.mkdirSync(dir, { recursive: true }); } } + /** + * Move a corrupt/unreadable file or directory into quarantine (preserving + * the original bytes) and record it, rather than silently dropping it. A + * guide/step never just disappears without an explanation. + */ + quarantine(sourcePath, kind, reason) { + const stamp = `${Date.now()}-${Math.random().toString(36).slice(2, 8)}`; + const dest = path.join(this.quarantineDir, `${kind}-${path.basename(sourcePath)}-${stamp}`); + try { + fs.mkdirSync(this.quarantineDir, { recursive: true }); + fs.renameSync(sourcePath, dest); + } catch { + // If we cannot move it (e.g. cross-device or vanished), still record it. + } + const entry = { kind, source: sourcePath, quarantined: dest, reason: String(reason || 'unreadable'), at: nowIso() }; + this.recoveryReport.push(entry); + return entry; + } + + /** Corrupt files quarantined this session (for a recovery UI). */ + getRecoveryReport() { + return [...this.recoveryReport]; + } + guideDir(guideId) { if (!/^[a-zA-Z0-9_-]+$/.test(guideId)) throw new Error(`bad guide id: ${guideId}`); return path.join(this.guidesDir, guideId); @@ -70,10 +114,19 @@ class GuideStore { return normalizeGuide(raw); } - saveGuide(guide, { touch = true } = {}) { + saveGuide(guide, { touch = true, expectedRevision = null } = {}) { validateGuide(guide); + // Optimistic concurrency: a caller that read the guide can pass the + // revision it saw; if disk moved on since, refuse rather than clobber. + if (expectedRevision !== null) { + const current = this.guideExists(guide.guideId) ? this.getGuide(guide.guideId).revision : 0; + if (current !== expectedRevision) { + throw new RevisionConflictError('guide', guide.guideId, expectedRevision, current); + } + } const stored = deepClone(guide); stored.descriptionHtml = sanitizeHtml(stored.descriptionHtml); + stored.revision = (Number.isInteger(stored.revision) ? stored.revision : 0) + 1; if (touch) stored.updatedAt = nowIso(); writeJsonSync(path.join(this.guideDir(guide.guideId), 'guide.json'), stored); return stored; @@ -83,11 +136,16 @@ class GuideStore { const out = []; for (const entry of fs.readdirSync(this.guidesDir, { withFileTypes: true })) { if (!entry.isDirectory()) continue; - const file = path.join(this.guidesDir, entry.name, 'guide.json'); + const dir = path.join(this.guidesDir, entry.name); + const file = path.join(dir, 'guide.json'); + if (!fs.existsSync(file)) continue; // in-progress/empty dir, not corruption try { out.push(normalizeGuide(readJsonSync(file))); - } catch { - // skip unreadable entries rather than failing the whole library + } catch (err) { + // A corrupt guide.json used to make the guide silently vanish from the + // library. Quarantine the directory (preserving it) and record it so + // the user can be told, instead of losing it without explanation. + this.quarantine(dir, 'guide', err && err.message); } } out.sort((a, b) => (a.updatedAt < b.updatedAt ? 1 : -1)); @@ -211,18 +269,38 @@ class GuideStore { if (!fs.existsSync(stepsRoot)) return map; for (const entry of fs.readdirSync(stepsRoot, { withFileTypes: true })) { if (!entry.isDirectory()) continue; + const dir = path.join(stepsRoot, entry.name); + const file = path.join(dir, 'step.json'); + if (!fs.existsSync(file)) continue; try { - map.set(entry.name, normalizeStep(readJsonSync(path.join(stepsRoot, entry.name, 'step.json')))); - } catch { - // skip unreadable step + map.set(entry.name, normalizeStep(readJsonSync(file))); + } catch (err) { + // Quarantine a corrupt step (preserving it) and record it rather than + // silently dropping it from the guide. + this.quarantine(dir, 'step', err && err.message); } } return map; } - saveStep(guideId, step) { + saveStep(guideId, step, { expectedRevision = null } = {}) { + // Optimistic concurrency for background/AI/capture writes: refuse to + // overwrite a step that changed since it was read. Direct user edits pass + // no expectedRevision (last-write-wins — the user is the authority). + if (expectedRevision !== null) { + let current = 0; + try { + current = this.getStep(guideId, step.stepId).revision; + } catch { + current = 0; // step vanished; treat as revision 0 + } + if (current !== expectedRevision) { + throw new RevisionConflictError('step', step.stepId, expectedRevision, current); + } + } const stored = normalizeStep(deepClone(step)); stored.descriptionHtml = sanitizeHtml(stored.descriptionHtml); + stored.revision = (Number.isInteger(stored.revision) ? stored.revision : 0) + 1; validateStep(stored); writeJsonSync(path.join(this.stepDir(guideId, step.stepId), 'step.json'), stored); const guide = this.getGuide(guideId); @@ -352,4 +430,4 @@ class GuideStore { } } -module.exports = { GuideStore }; +module.exports = { GuideStore, RevisionConflictError }; diff --git a/tests/unit/ai-network.test.js b/tests/unit/ai-network.test.js index 66012d8..719dc81 100644 --- a/tests/unit/ai-network.test.js +++ b/tests/unit/ai-network.test.js @@ -181,6 +181,76 @@ test('shortcut detection still works with typed text disabled', () => { assert.equal(off.snapshotKeyContext().recentShortcut, 'Ctrl+T'); }); +// ---- stale AI responses cannot overwrite user edits -------------------------- + +test('an AI patch built from stale data does not clobber a mid-generation user edit', async (t) => { + const { GuideStore } = require('../../core/store'); + const root = makeTmpDir('ai-stale-write'); + t.after(() => rmrf(root)); + const store = new GuideStore(root); + const guide = store.createGuide({ title: 'G' }); + const step = store.addStep(guide.guideId, { title: 'original title' }); + + const service = new TextIntelService({ + store, + settings: makeSettings(), + dataDir: root, + fetchImpl: async (url) => { + const pathname = new URL(url).pathname; + if (pathname === '/api/show') { + return { ok: true, json: async () => ({ capabilities: ['completion'] }) }; + } + if (pathname === '/api/chat') { + // While the model "thinks", the user edits and saves the step. + const current = store.getStep(guide.guideId, step.stepId); + store.saveStep(guide.guideId, { ...current, title: 'user edit during generation' }); + return { + ok: true, + json: async () => ({ message: { content: JSON.stringify({ title: 'AI title from stale context' }) } }), + }; + } + throw new Error(`unexpected fetch: ${pathname}`); + }, + }); + + const result = await service.generateStepPatch({ guideId: guide.guideId, stepId: step.stepId, target: 'title' }); + assert.equal(result.ok, false); + assert.match(result.reason, /changed while AI was generating/i); + assert.equal(store.getStep(guide.guideId, step.stepId).title, 'user edit during generation'); +}); + +test('an AI patch applies cleanly when nothing changed during generation', async (t) => { + const { GuideStore } = require('../../core/store'); + const root = makeTmpDir('ai-clean-write'); + t.after(() => rmrf(root)); + const store = new GuideStore(root); + const guide = store.createGuide({ title: 'G' }); + const step = store.addStep(guide.guideId, { title: '' }); + + const service = new TextIntelService({ + store, + settings: makeSettings(), + dataDir: root, + fetchImpl: async (url) => { + const pathname = new URL(url).pathname; + if (pathname === '/api/show') { + return { ok: true, json: async () => ({ capabilities: ['completion'] }) }; + } + if (pathname === '/api/chat') { + return { + ok: true, + json: async () => ({ message: { content: JSON.stringify({ title: 'Generated title' }) } }), + }; + } + throw new Error(`unexpected fetch: ${pathname}`); + }, + }); + + const result = await service.generateStepPatch({ guideId: guide.guideId, stepId: step.stepId, target: 'title' }); + assert.equal(result.ok, true); + assert.equal(store.getStep(guide.guideId, step.stepId).title, 'Generated title'); +}); + // ---- source-level guards ---------------------------------------------------- const fs = require('node:fs'); diff --git a/tests/unit/store-revisions.test.js b/tests/unit/store-revisions.test.js new file mode 100644 index 0000000..9c70fdd --- /dev/null +++ b/tests/unit/store-revisions.test.js @@ -0,0 +1,143 @@ +'use strict'; + +const test = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); + +const { GuideStore, RevisionConflictError } = require('../../core/store'); +const { makeTmpDir, rmrf, TINY_PNG } = require('./helpers'); + +// ---- revisions -------------------------------------------------------------- + +test('revisions start at 0 and increment on every save', (t) => { + const root = makeTmpDir('store-rev'); + t.after(() => rmrf(root)); + const store = new GuideStore(root); + + const guide = store.createGuide({ title: 'G' }); + assert.equal(guide.revision, 0); + + const step = store.addStep(guide.guideId, { title: 'S' }, TINY_PNG, { width: 1, height: 1 }); + const r0 = store.getStep(guide.guideId, step.stepId).revision; + + const saved1 = store.saveStep(guide.guideId, { ...step, title: 'S1' }); + assert.equal(saved1.revision, r0 + 1); + const saved2 = store.saveStep(guide.guideId, { ...saved1, title: 'S2' }); + assert.equal(saved2.revision, r0 + 2); +}); + +test('compare-and-swap: a stale expectedRevision is rejected, not clobbered', (t) => { + const root = makeTmpDir('store-cas'); + t.after(() => rmrf(root)); + const store = new GuideStore(root); + const guide = store.createGuide({ title: 'G' }); + const step = store.addStep(guide.guideId, { title: 'S' }, TINY_PNG, { width: 1, height: 1 }); + + const base = store.getStep(guide.guideId, step.stepId); + // A user edit lands first (no expectedRevision -> last-write-wins). + store.saveStep(guide.guideId, { ...base, title: 'user edit' }); + + // A background writer that read `base` tries to save with the stale revision. + assert.throws( + () => store.saveStep(guide.guideId, { ...base, title: 'stale background' }, { expectedRevision: base.revision }), + (err) => err instanceof RevisionConflictError && err.code === 'STEPFORGE_REVISION_CONFLICT' + ); + // The user edit survived. + assert.equal(store.getStep(guide.guideId, step.stepId).title, 'user edit'); +}); + +test('compare-and-swap succeeds when the revision still matches', (t) => { + const root = makeTmpDir('store-cas-ok'); + t.after(() => rmrf(root)); + const store = new GuideStore(root); + const guide = store.createGuide({ title: 'G' }); + const step = store.addStep(guide.guideId, { title: 'S' }, TINY_PNG, { width: 1, height: 1 }); + const base = store.getStep(guide.guideId, step.stepId); + + const saved = store.saveStep(guide.guideId, { ...base, title: 'ok' }, { expectedRevision: base.revision }); + assert.equal(saved.title, 'ok'); + assert.equal(saved.revision, base.revision + 1); +}); + +test('guide saves are revision-aware too', (t) => { + const root = makeTmpDir('store-guide-cas'); + t.after(() => rmrf(root)); + const store = new GuideStore(root); + const guide = store.createGuide({ title: 'G' }); + const base = store.getGuide(guide.guideId); + store.saveGuide({ ...base, title: 'first' }); + assert.throws( + () => store.saveGuide({ ...base, title: 'stale' }, { expectedRevision: base.revision }), + RevisionConflictError + ); +}); + +test('v1 data without a revision field reads as revision 0 and upgrades on save', (t) => { + const root = makeTmpDir('store-v1'); + t.after(() => rmrf(root)); + const store = new GuideStore(root); + const guide = store.createGuide({ title: 'G' }); + + // Simulate legacy on-disk data: strip the revision field. + const file = path.join(store.guidesDir, guide.guideId, 'guide.json'); + const raw = JSON.parse(fs.readFileSync(file, 'utf8')); + delete raw.revision; + fs.writeFileSync(file, JSON.stringify(raw)); + + const loaded = store.getGuide(guide.guideId); + assert.equal(loaded.revision, 0); + const saved = store.saveGuide(loaded); + assert.equal(saved.revision, 1); +}); + +// ---- corruption quarantine -------------------------------------------------- + +test('a corrupt guide is quarantined and reported, not silently dropped', (t) => { + const root = makeTmpDir('store-quarantine-guide'); + t.after(() => rmrf(root)); + const store = new GuideStore(root); + const good = store.createGuide({ title: 'Good' }); + const bad = store.createGuide({ title: 'Bad' }); + + // Corrupt the bad guide's JSON. + fs.writeFileSync(path.join(store.guidesDir, bad.guideId, 'guide.json'), '{ not valid json'); + + const listed = store.listGuides(); + assert.deepEqual(listed.map((g) => g.guideId), [good.guideId]); + + const report = store.getRecoveryReport(); + assert.equal(report.length, 1); + assert.equal(report[0].kind, 'guide'); + // The original bytes are preserved in quarantine, not deleted. + assert.ok(fs.existsSync(report[0].quarantined)); + // The bad guide dir is gone from the live library. + assert.equal(fs.existsSync(path.join(store.guidesDir, bad.guideId)), false); +}); + +test('a corrupt step is quarantined and reported', (t) => { + const root = makeTmpDir('store-quarantine-step'); + t.after(() => rmrf(root)); + const store = new GuideStore(root); + const guide = store.createGuide({ title: 'G' }); + const good = store.addStep(guide.guideId, { title: 'good' }, TINY_PNG, { width: 1, height: 1 }); + const bad = store.addStep(guide.guideId, { title: 'bad' }, TINY_PNG, { width: 1, height: 1 }); + + fs.writeFileSync(path.join(store.stepDir(guide.guideId, bad.stepId), 'step.json'), 'nonsense'); + + const steps = store.listSteps(guide.guideId); + assert.ok(steps.has(good.stepId)); + assert.equal(steps.has(bad.stepId), false); + const report = store.getRecoveryReport(); + assert.equal(report.some((r) => r.kind === 'step'), true); +}); + +test('an empty/in-progress guide directory is not treated as corruption', (t) => { + const root = makeTmpDir('store-empty-dir'); + t.after(() => rmrf(root)); + const store = new GuideStore(root); + // A directory with no guide.json (e.g. mid-create) must be skipped quietly. + fs.mkdirSync(path.join(store.guidesDir, 'orphan-dir'), { recursive: true }); + assert.doesNotThrow(() => store.listGuides()); + assert.equal(store.getRecoveryReport().length, 0); +});