Merge pull request #9 from Twest2/StepForge pr/04-autosave-storage

Add optimistic revisions, keep dirty state on failed saves, quarantine corrupt data (plan PR 4)

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();
   }
 

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.' };

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,
   };
 }
 

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 };

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');

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);
+});