Merge pull request #19 from AdaInTheLab/bugfix/sync-overwrite

AdaInTheLab · web-flow · commit ef906aa3ddc3 · 2026-01-10T09:25:37.000-05:00
FIX [CONTENT] Prevent FS sync from overwriting admin-edited content 🧿
diff --git a/src/routes/adminRoutes.ts b/src/routes/adminRoutes.ts
@@ -4,15 +4,14 @@ import type Database from "better-sqlite3";
 import { randomUUID } from "node:crypto";
 import { marked } from "marked";
 import passport, { requireAdmin, isGithubOAuthEnabled } from "../auth.js";
-import { syncLabNotesFromFs } from "../services/syncLabNotesFromFs.js";
+import { syncLabNotesFromFs, SyncCounts } from "../services/syncLabNotesFromFs.js";
 import { normalizeLocale, sha256Hex } from "../lib/helpers.js";
 
 marked.setOptions({
     gfm: true,
     breaks: false, // ✅ strict
 });
 
-
 export function registerAdminRoutes(app: any, db: Database.Database) {
     // Must match your UI origin exactly (no trailing slash)
     const UI_BASE_URL = process.env.UI_BASE_URL ?? "http://localhost:8001";
@@ -434,15 +433,21 @@ export function registerAdminRoutes(app: any, db: Database.Database) {
     // ---------------------------------------------------------------------------
     // Admin: Syncs MD Files to DB (protected)
     // ---------------------------------------------------------------------------
-    app.post("/admin/notes/sync", requireAdmin, (req: any, res: { json: (arg0: { rootDir: string; locales: string[]; scanned: number; upserted: number; skipped: number; errors: Array<{ file: string; error: string; }>; ok: boolean; }) => void; status: (arg0: number) => { (): any; new(): any; json: { (arg0: { ok: boolean; error: any; }): void; new(): any; }; }; }) => {
+    app.post("/admin/notes/sync", requireAdmin, (req: Request, res: Response) => {
         try {
-            const result = syncLabNotesFromFs(db);
+            const force =
+                String(req.query.force ?? "").trim() === "1" ||
+                req.body?.force === true ||
+                String(req.body?.force ?? "").trim() === "1";
+
+            const result: SyncCounts = syncLabNotesFromFs(db, { force });
             res.json({ ok: true, ...result });
         } catch (e: any) {
             res.status(500).json({ ok: false, error: e?.message ?? String(e) });
         }
     });
 
+
     // ---------------------------------------------------------------------------
     // Auth helpers (always available)
     // ---------------------------------------------------------------------------
diff --git a/src/services/syncLabNotesFromFs.ts b/src/services/syncLabNotesFromFs.ts
@@ -5,16 +5,19 @@ import crypto from "node:crypto";
 import matter from "gray-matter";
 import type Database from "better-sqlite3";
 
-type SyncCounts = {
+export type SyncCounts = {
     rootDir: string;
     locales: string[];
     scanned: number;
     upserted: number;
     skipped: number;
     revisionsInserted: number;
     pointersUpdated: number;
+    pointersSkippedProtected: number;
+    pointersForced: number;
     emptyBodySkipped: number;
     errors: Array<{ file: string; error: string }>;
+    protected: Array<{ slug: string; locale: string; reason: string }>;
 };
 
 /* ===========================================================
@@ -83,7 +86,11 @@ function jsonString(v: any, fallback: any): string {
     }
 }
 
-export function syncLabNotesFromFs(db: Database.Database): SyncCounts {
+export function syncLabNotesFromFs(
+    db: Database.Database,
+    opts?: { force?: boolean }
+): SyncCounts {
+    const force = Boolean(opts?.force);
     const rootDir = String(process.env.LABNOTES_DIR || "").trim();
     if (!rootDir) throw new Error("LABNOTES_DIR is not set");
     if (!fs.existsSync(rootDir)) throw new Error(`LABNOTES_DIR not found: ${rootDir}`);
@@ -103,8 +110,11 @@ export function syncLabNotesFromFs(db: Database.Database): SyncCounts {
         skipped: 0,
         revisionsInserted: 0,
         pointersUpdated: 0,
+        pointersSkippedProtected: 0,
+        pointersForced: 0,
         emptyBodySkipped: 0,
         errors: [],
+        protected: [],
     };
 
     // -----------------------------
@@ -167,10 +177,18 @@ export function syncLabNotesFromFs(db: Database.Database): SyncCounts {
   `);
 
     const selectLatestRevision = db.prepare(`
-    SELECT id, revision_num, content_hash, length(content_markdown) AS md_len
-    FROM lab_note_revisions
-    WHERE note_id = ?
-    ORDER BY revision_num DESC
+        SELECT id, revision_num, content_hash, length(content_markdown) AS md_len, source
+        FROM lab_note_revisions
+        WHERE note_id = ?
+        ORDER BY revision_num DESC
+        LIMIT 1
+    `);
+
+    const selectCurrentRevisionSource = db.prepare(`
+    SELECT r.source AS source
+    FROM lab_notes n
+    LEFT JOIN lab_note_revisions r ON r.id = n.current_revision_id
+    WHERE n.id = ?
     LIMIT 1
   `);
 
@@ -287,12 +305,20 @@ export function syncLabNotesFromFs(db: Database.Database): SyncCounts {
             const hash = sha256Hex(markdown);
 
             const latest = selectLatestRevision.get(noteRow.id) as
-                | { id: string; revision_num: number; content_hash: string; md_len: number }
+                | { id: string; revision_num: number; content_hash: string; md_len: number; source: string | null }
                 | undefined;
 
             // 3) Idempotency: if the latest revision already matches this body, do nothing
+            // 3) Idempotency: if the latest revision already matches this body, usually do nothing
             if (latest && latest.content_hash === hash && (latest.md_len ?? 0) > 0) {
-                counts.skipped += 1;
+                // But if we're forcing, we may still need to advance pointers to the latest import revision
+                if (force && latest.source === "import") {
+                    updatePointers.run(latest.id, latest.id, noteRow.id);
+                    counts.pointersUpdated += 1;
+                    counts.pointersForced += 1;
+                } else {
+                    counts.skipped += 1;
+                }
                 return;
             }
 
@@ -350,9 +376,43 @@ export function syncLabNotesFromFs(db: Database.Database): SyncCounts {
             });
             counts.revisionsInserted += 1;
 
-            // 5) Advance pointers ONLY to a non-empty revision (this one is guaranteed non-empty)
-            updatePointers.run(newRevId, newRevId, noteRow.id);
-            counts.pointersUpdated += 1;
+            // 5) Advance pointers ONLY if safe (and non-empty revision is already guaranteed)
+            const curSourceRow = selectCurrentRevisionSource.get(noteRow.id) as
+                | { source?: string | null }
+                | undefined;
+
+            const curSource = curSourceRow?.source ?? null;
+
+            // Safe-to-advance rules:
+            // - force=true: always advance (explicit override)
+            // - no current revision: new note, safe
+            // - current source is 'import': FS already owns the current draft
+            const canAdvance =
+                force ||
+                !noteRow.current_revision_id ||
+                curSource === "import";
+
+            if (canAdvance) {
+                updatePointers.run(newRevId, newRevId, noteRow.id);
+                counts.pointersUpdated += 1;
+
+                if (force && noteRow.current_revision_id && curSource !== "import") {
+                    counts.pointersForced += 1;
+                }
+            } else {
+                // Protected admin draft: we still inserted the import revision, but we don't switch pointers
+                counts.pointersSkippedProtected += 1;
+
+                // Optional: record protected items for UI/debug (cap it)
+                if (counts.protected.length < 50) {
+                    counts.protected.push({
+                        slug,
+                        locale,
+                        reason: `protected admin draft (current source=${curSource})`,
+                    });
+                }
+            }
+
         } catch (e: any) {
             counts.errors.push({ file: filePath, error: e?.message ?? String(e) });
         }
diff --git a/tests/admin.routes.test.ts b/tests/admin.routes.test.ts
@@ -1,5 +1,8 @@
 import request from "supertest";
 import { createTestApp, api } from "./helpers/createTestApp.js";
+import fs from "fs";
+import os from "os";
+import path from "path";
 
 describe("Admin routes", () => {
     const OLD_ENV = { ...process.env };
@@ -133,5 +136,83 @@ describe("Admin routes", () => {
             expect(matches[0].title).toBe("Version 2");
             expect(matches[0].summary).toBe("two");
         });
+
+        test("POST /admin/notes/sync protects admin (web) current revision unless forced", async () => {
+            const tmp = fs.mkdtempSync(path.join(os.tmpdir(), "labnotes-"));
+            process.env.LABNOTES_DIR = tmp;
+
+            const localeDir = path.join(tmp, "en");
+            fs.mkdirSync(localeDir, { recursive: true });
+
+            const { app, db } = createTestApp();
+
+            // 1) Create admin draft (current revision should be source='web')
+            const slug = "sync-protect-me";
+            const locale = "en";
+
+            const create = await request(app).post(api("/admin/notes")).send({
+                title: "Admin Draft",
+                slug,
+                locale,
+                status: "draft",
+                contentMarkdown: "ADMIN VERSION", // use whatever field your admin route accepts
+            });
+
+            expect(create.status).toBe(201);
+
+            // 2) Write conflicting FS markdown for same slug/locale
+            const mdPath = path.join(localeDir, `${slug}.md`);
+            fs.writeFileSync(
+                mdPath,
+                [
+                    "---",
+                    `slug: ${slug}`,
+                    "title: FS Version",
+                    "type: labnote",
+                    "---",
+                    "",
+                    "FS VERSION",
+                    "",
+                ].join("\n"),
+                "utf8"
+            );
+
+            // Helper: read current revision source for the note
+            const getCurrentSource = () => {
+                const row = db
+                    .prepare(
+                        `
+        SELECT r.source AS source
+        FROM lab_notes n
+        JOIN lab_note_revisions r ON r.id = n.current_revision_id
+        WHERE n.slug = ? AND n.locale = ?
+        LIMIT 1
+      `
+                    )
+                    .get(slug, locale) as { source: string } | undefined;
+
+                return row?.source ?? null;
+            };
+
+            expect(getCurrentSource()).toBe("web");
+
+            // 3) Sync WITHOUT force -> should NOT clobber web pointer
+            const s1 = await request(app).post(api("/admin/notes/sync"));
+            expect(s1.status).toBe(200);
+            expect(s1.body?.ok).toBe(true);
+
+            // the key behavior change
+            expect(s1.body?.pointersSkippedProtected).toBeGreaterThanOrEqual(1);
+            expect(getCurrentSource()).toBe("web");
+
+            // 4) Sync WITH force -> should advance pointer to import
+            const s2 = await request(app).post(api("/admin/notes/sync?force=1"));
+            expect(s2.status).toBe(200);
+            expect(s2.body?.ok).toBe(true);
+
+            expect(getCurrentSource()).toBe("import");
+            expect(s2.body?.pointersForced).toBeGreaterThanOrEqual(1);
+        });
+
     });
 });
