fix(git-sync): propagate nested details open; drop dead delete-cap wiring; cover lost-lock abort + lose-prone atom round-trips

Addresses review 1863 (delta) on PR #119.

MUST-FIX:
- detailsToHtml (the raw-HTML path used for a details nested inside
  columns/spanned cells) now emits `<details${open}>`, mirroring the
  top-level case, so `open` no longer silently drops every round trip.
- Remove the dead `resolveApplyClient` delete-cap hook from the engine
  `runCycle`: the orchestrator stopped passing it, so the hook + its
  dry-run pass were inert. Deletes are soft (Trash) + always logged and
  engine convergence is the guard, so no cap is re-added — just the dead
  wiring removed.

TEST COVERAGE:
- space-lock: heartbeat refresh CAS-miss (eval -> 0) and Redis-error
  (eval throws) both abort the in-flight fn's signal.
- cycle: a pre-aborted signal (and an abort during the pull read) throws
  before the push apply / first destructive phase.
- converter: htmlEmbed source VALUE + height survive; encode/decode
  UTF-8 symmetry and '' -> ''; footnote definition body + ref/def id
  match; transclusionReference both ids survive; fix the bad
  transclusionSource fixture (wrong `pageId` attr + empty content ->
  schema `id` + a block child); nested details `open` parity test.
- orchestrator: autoMergeConflicts:true reaches engine settings; default
  false on a missing settings row.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

apps/server/src/integrations/git-sync/services/git-sync.orchestrator.spec.ts

@@ -54,6 +54,12 @@ interface BuildOptions {
   debounceMs?: number;
   /** A hook applied to the fake vault so a test can override its behaviour. */
   vaultOverrides?: Record<string, unknown>;
+  /**
+   * The row `buildSettings` reads for the per-space `autoMergeConflicts` flag
+   * (`executeTakeFirst`). Default: the SAFE off value. Pass `undefined` to model
+   * a missing row (no space / no settings).
+   */
+  settingsRow?: { autoMergeConflicts: boolean } | undefined;
 }
 
 interface Built {
@@ -78,6 +84,10 @@ function build(opts: BuildOptions = {}): Built {
     debounceMs = 2000,
     vaultOverrides = {},
   } = opts;
+  // Distinguish "key omitted" (default off row) from "key present but undefined"
+  // (a deliberately MISSING settings row).
+  const settingsRow =
+    'settingsRow' in opts ? opts.settingsRow : { autoMergeConflicts: false };
   // Distinguish "key omitted" (default to a valid id) from "key present but
   // undefined" (the no-service-user test deliberately sets it undefined).
   const serviceUserId = 'serviceUserId' in opts ? opts.serviceUserId : 'svc-user';
@@ -135,7 +145,7 @@ function build(opts: BuildOptions = {}): Built {
     const builder: any = {
       select: () => builder,
       where: () => builder,
-      executeTakeFirst: async () => ({ autoMergeConflicts: false }),
+      executeTakeFirst: async () => settingsRow,
       execute: async () => [],
     };
     return { selectFrom: () => builder };
@@ -290,6 +300,20 @@ describe('GitSyncOrchestrator', () => {
       });
     });
 
+    it('threads autoMergeConflicts:true from the space settings row into the engine settings', async () => {
+      const built = build({ settingsRow: { autoMergeConflicts: true } });
+      await built.orchestrator.runOnce('space-1', 'ws-1');
+      const [deps] = runCycleMock.mock.calls[0];
+      expect(deps.settings.autoMergeConflicts).toBe(true);
+    });
+
+    it('defaults autoMergeConflicts to false when the settings row is missing', async () => {
+      const built = build({ settingsRow: undefined });
+      await built.orchestrator.runOnce('space-1', 'ws-1');
+      const [deps] = runCycleMock.mock.calls[0];
+      expect(deps.settings.autoMergeConflicts).toBe(false);
+    });
+
     it("surfaces the engine's skipped status (e.g. merge-in-progress) verbatim", async () => {
       const built = build();
       runCycleMock.mockResolvedValue({ ran: false, skipped: 'merge-in-progress' });

apps/server/src/integrations/git-sync/services/space-lock.service.spec.ts

@@ -187,6 +187,82 @@ describe('SpaceLockService', () => {
       }
     });
   });
+
+  // The lost-lock guard: a heartbeat refresh that cannot CONFIRM we still own the
+  // lock (CAS miss, res !== 1) OR that throws (Redis error) aborts the supplied
+  // controller so the in-flight protected fn stops instead of writing blind after
+  // a possible lock takeover. `withSpaceLock` threads that signal into `fn`.
+  describe('abort-on-lost-lock', () => {
+    it('aborts the in-flight fn when the heartbeat refresh CAS-MISSES (eval -> 0)', async () => {
+      jest.useFakeTimers();
+      try {
+        const { service, redis } = build();
+        let release!: () => void;
+        const gate = new Promise<void>((resolve) => {
+          release = resolve;
+        });
+        let captured: AbortSignal | undefined;
+
+        const run = service.withSpaceLock('space-1', async (signal) => {
+          captured = signal;
+          await gate;
+          return 'done';
+        });
+        // Let acquire resolve and the setInterval register.
+        await flushMicrotasks();
+        expect(captured).toBeDefined();
+        expect(captured!.aborted).toBe(false);
+
+        // The refresh CAS-misses: the key no longer holds our instanceId.
+        redis.eval.mockResolvedValue(0);
+        jest.advanceTimersByTime(Math.floor(GIT_SYNC_LOCK_TTL_MS / 3));
+        await flushMicrotasks();
+
+        // The lost lock aborted the protected fn's signal.
+        expect(captured!.aborted).toBe(true);
+
+        release();
+        await flushMicrotasks();
+        await expect(run).resolves.toBe('done');
+      } finally {
+        jest.useRealTimers();
+      }
+    });
+
+    it('aborts the in-flight fn when the heartbeat refresh THROWS (Redis error)', async () => {
+      jest.useFakeTimers();
+      try {
+        const { service, redis } = build();
+        let release!: () => void;
+        const gate = new Promise<void>((resolve) => {
+          release = resolve;
+        });
+        let captured: AbortSignal | undefined;
+
+        const run = service.withSpaceLock('space-1', async (signal) => {
+          captured = signal;
+          await gate;
+          return 'done';
+        });
+        await flushMicrotasks();
+        expect(captured!.aborted).toBe(false);
+
+        // The refresh eval rejects (Redis down). release() in finally must still
+        // resolve, so only reject the NEXT (heartbeat) call, then go back to OK.
+        redis.eval.mockRejectedValueOnce(new Error('redis down'));
+        jest.advanceTimersByTime(Math.floor(GIT_SYNC_LOCK_TTL_MS / 3));
+        await flushMicrotasks();
+
+        expect(captured!.aborted).toBe(true);
+
+        release();
+        await flushMicrotasks();
+        await expect(run).resolves.toBe('done');
+      } finally {
+        jest.useRealTimers();
+      }
+    });
+  });
 });
 
 // Silence the warn logger if a refresh/release path ever logs (defensive).

packages/git-sync/src/engine/cycle.ts

@@ -36,19 +36,6 @@ export interface RunCycleDeps {
    * single-writer still needs the fencing-token redesign (follow-up).
    */
   signal?: AbortSignal;
-  /**
-   * Delete-cap hook (the ONLY caller-specific policy). Called with the push
-   * dry-run's planned delete count (`Number.POSITIVE_INFINITY` when the dry-run
-   * itself failed, so the hook can fail safe) and the live client; returns the
-   * client to use for the REAL apply. The default (omitted) applies every op
-   * unmodified. gitmost uses it to neutralize deletes when over its cap.
-   *
-   * When omitted, NO dry-run is performed (one fewer push planning pass).
-   */
-  resolveApplyClient?: (
-    plannedDeletes: number,
-    client: GitSyncClient,
-  ) => GitSyncClient;
 }
 
 export interface RunCycleResult {
@@ -82,13 +69,14 @@ export interface RunCycleResult {
  *      content straight onto `main` would clobber local file edits before push
  *      can diff them.
  *   4. PULL: readExisting -> listSpaceTree -> computePullActions -> apply.
- *   5. PUSH: optional dry-run to feed the delete-cap hook, then the real apply.
+ *   5. PUSH: vault -> Docmost apply.
  *
- * Lock + cap POLICY live in the caller; this owns only the mechanics.
+ * Lock POLICY lives in the caller; this owns only the mechanics. Deletes are
+ * soft (Trash, reversible) and always logged, so there is no per-cycle
+ * delete-cap — engine convergence is the guard against phantom deletions.
  */
 export async function runCycle(deps: RunCycleDeps): Promise<RunCycleResult> {
-  const { spaceId, client, vault, settings, fs, log, resolveApplyClient, signal } =
+  const { spaceId, client, vault, settings, fs, log, signal } = deps;
-    deps;
   const vaultRoot = settings.vaultPath;
   const abs = (relPath: string) => `${vaultRoot}/${relPath}`;
 
@@ -150,33 +138,10 @@ export async function runCycle(deps: RunCycleDeps): Promise<RunCycleResult> {
     log,
   };
 
-  let applyClient = client;
-  if (resolveApplyClient) {
-    // Plan the push as a DRY-RUN first to read the delete count, then let the
-    // caller decide the apply client (e.g. neutralize deletes over a cap). A
-    // failed dry-run yields Infinity so the hook can fail safe.
-    let plannedDeletes: number;
-    try {
-      const dry = await runPush(pushDeps, { dryRun: true });
-      plannedDeletes = dry.planned?.deletes ?? 0;
-    } catch (err) {
-      log(
-        `push dry-run planning failed (${
-          err instanceof Error ? err.message : String(err)
-        }); deferring deletion policy to the cap hook (fail-safe).`,
-      );
-      plannedDeletes = Number.POSITIVE_INFINITY;
-    }
-    applyClient = resolveApplyClient(plannedDeletes, client);
-  }
-
   // Bail before pushing to Docmost if the lock was lost during pull.
   signal?.throwIfAborted();
 
-  const pushResult = await runPush(
+  const pushResult = await runPush(pushDeps, { dryRun: false });
-    { ...pushDeps, makeClient: () => applyClient },
-    { dryRun: false },
-  );
 
   return {
     ran: true,

packages/git-sync/src/lib/markdown-converter.ts

@@ -854,9 +854,14 @@ export function convertProseMirrorToMarkdown(content: any): string {
 
   // Emit a schema-matching <details> tree. The schema parses <details>,
   // summary[data-type="detailsSummary"], and div[data-type="detailsContent"].
+  // The `open` (collapsed/expanded) state lives on the details node and the
+  // schema parses it back from the attribute, so emit it here too — mirroring
+  // the top-level `details` case — or a NESTED details (inside columns/cells)
+  // would silently drop `open:true` every round trip.
   const detailsToHtml = (node: any): string => {
+    const open = node.attrs?.open ? " open" : "";
     const inner = (node.content || []).map(blockToHtml).join("");
-    return `<details>${inner}</details>`;
+    return `<details${open}>${inner}</details>`;
   };
   const detailsSummaryToHtml = (node: any): string =>
     `<summary data-type="detailsSummary">${inlineToHtml(node.content || [])}</summary>`;

packages/git-sync/test/cycle.test.ts

@@ -92,27 +92,53 @@ describe("runCycle (composition)", () => {
     expect(deps.client.listSpaceTree).toHaveBeenCalledTimes(1);
   });
 
-  it("consults the resolveApplyClient hook with the planned delete count", async () => {
+  it("runs a SINGLE push planning pass (no dry-run; the delete-cap hook is gone)", async () => {
     const vault = fakeVault();
-    const hook = vi.fn((_planned: number, c: any) => c);
+    const deps = baseDeps(vault);
-    const deps = baseDeps(vault, { resolveApplyClient: hook });
-
-    await runCycle(deps);
-
-    // An empty vault plans zero deletes; the hook is still consulted so the
-    // caller's policy always sees the count (and a dry-run preceded it).
-    expect(hook).toHaveBeenCalledTimes(1);
-    expect(hook.mock.calls[0][0]).toBe(0);
-  });
-
-  it("skips the dry-run entirely when no resolveApplyClient hook is given", async () => {
-    const vault = fakeVault();
-    const deps = baseDeps(vault); // no resolveApplyClient
 
     const res = await runCycle(deps);
     expect(res.ran).toBe(true);
-    // With no cap hook there is a single runPush (the apply) — no dry-run pass.
+    // There is exactly one runPush (the apply) — no separate dry-run pass.
     // diffNameStatus is read once per runPush; assert a single planning pass.
     expect(vault.diffNameStatus).toHaveBeenCalledTimes(1);
   });
+
+  it("throws on a PRE-aborted signal BEFORE applying the pull (first destructive phase)", async () => {
+    const vault = fakeVault();
+    const controller = new AbortController();
+    controller.abort();
+    const deps = baseDeps(vault, { signal: controller.signal });
+
+    await expect(runCycle(deps)).rejects.toThrow();
+
+    // The signal is checked AFTER planning but BEFORE the first write phase:
+    // the tree was listed (planning) but neither destructive phase advanced —
+    // no pull merge and no push diff.
+    expect(deps.client.listSpaceTree).toHaveBeenCalledTimes(1);
+    expect(vault.order).not.toContain("merge:main");
+    expect(vault.diffNameStatus).not.toHaveBeenCalled();
+  });
+
+  it("throws BEFORE the push apply when the signal aborts during the pull phase", async () => {
+    // Abort mid-cycle: the signal fires while listSpaceTree (the pull read)
+    // runs, so the SECOND checkpoint (before runPush) trips and the push apply
+    // never starts.
+    const controller = new AbortController();
+    const vault = fakeVault();
+    const deps = baseDeps(vault, {
+      signal: controller.signal,
+      client: {
+        ...baseDeps(vault).client,
+        listSpaceTree: vi.fn(async () => {
+          controller.abort();
+          return { pages: [], complete: true };
+        }),
+      } as any,
+    });
+
+    await expect(runCycle(deps)).rejects.toThrow();
+    // Pull planning ran but the push never did (aborted at a checkpoint).
+    expect(deps.client.listSpaceTree).toHaveBeenCalledTimes(1);
+    expect(vault.diffNameStatus).not.toHaveBeenCalled();
+  });
 });

packages/git-sync/test/docmost-schema-attrs.test.ts

@@ -2,6 +2,8 @@ import { describe, expect, it } from 'vitest';
 import {
   sanitizeCssColor,
   clampCalloutType,
+  encodeHtmlEmbedSource,
+  decodeHtmlEmbedSource,
 } from '../src/lib/docmost-schema.js';
 
 // These tests pin the two security/normalization helpers that Docmost
@@ -73,3 +75,26 @@ describe('clampCalloutType', () => {
     expect(clampCalloutType(null)).toBe('info');
   });
 });
+
+// The htmlEmbed `source` rides the data-source attribute base64-encoded so the
+// raw HTML/CSS/JS stays inert and double-encoding-free across a round trip.
+// Encode/decode MUST be exact inverses (incl. UTF-8) or the embed body corrupts.
+describe('encode/decodeHtmlEmbedSource', () => {
+  it('round-trips ASCII HTML losslessly', () => {
+    const src = '<b>hi</b>';
+    expect(decodeHtmlEmbedSource(encodeHtmlEmbedSource(src))).toBe(src);
+  });
+
+  it('round-trips multi-byte UTF-8 (Cyrillic + emoji) losslessly', () => {
+    const src = '<p>Привет, мир 🌍 — café</p>';
+    const encoded = encodeHtmlEmbedSource(src);
+    // It is actually encoded (not passed through verbatim).
+    expect(encoded).not.toBe(src);
+    expect(decodeHtmlEmbedSource(encoded)).toBe(src);
+  });
+
+  it('maps empty string to empty string both ways', () => {
+    expect(encodeHtmlEmbedSource('')).toBe('');
+    expect(decodeHtmlEmbedSource('')).toBe('');
+  });
+});

packages/git-sync/test/roundtrip-all-nodes.test.ts

@@ -66,7 +66,10 @@ const FIXTURES: Record<string, { doc: any; primary: string }> = {
   pageBreak: { doc: doc({ type: 'pageBreak' }), primary: 'pageBreak' },
   htmlEmbed: { doc: doc({ type: 'htmlEmbed', attrs: { source: '<b>hi</b>' } }), primary: 'htmlEmbed' },
   pageEmbed: { doc: doc({ type: 'pageEmbed', attrs: { pageId: 'p1' } }), primary: 'pageEmbed' },
-  transclusion: { doc: doc({ type: 'transclusionSource', attrs: { pageId: 'p1' } }), primary: 'transclusionSource' },
+  // transclusionSource: the schema reads `id` (NOT `pageId`) and its content is
+  // `+` (at least one block child), so give it both or it never re-parses.
+  transclusion: { doc: doc({ type: 'transclusionSource', attrs: { id: 't1' }, content: [P(T('shared'))] }), primary: 'transclusionSource' },
+  transclusionReference: { doc: doc({ type: 'transclusionReference', attrs: { sourcePageId: 'p1', transclusionId: 't1' } }), primary: 'transclusionReference' },
   footnote: {
     doc: doc(
       P(T('x'), { type: 'footnoteReference', attrs: { id: 'fn1' } }),
@@ -139,3 +142,137 @@ describe('git-sync converter: node ATTRIBUTES survive a Markdown round trip', ()
     });
   }
 });
+
+// Find the FIRST node of a given type anywhere in a ProseMirror tree (depth
+// first). Used by the structural round-trip assertions below that need the
+// re-imported node's concrete attrs/content, not just "the type is present".
+function findNode(n: any, type: string): any {
+  if (!n || typeof n !== 'object') return undefined;
+  if (n.type === type) return n;
+  if (Array.isArray(n.content)) {
+    for (const c of n.content) {
+      const hit = findNode(c, type);
+      if (hit) return hit;
+    }
+  }
+  return undefined;
+}
+
+// Collect every text run reachable under a node (concatenated). Lets a test
+// assert a footnote definition's note BODY survived, not just the wrapper.
+function allText(n: any): string {
+  if (!n || typeof n !== 'object') return '';
+  if (n.type === 'text') return n.text || '';
+  if (Array.isArray(n.content)) return n.content.map(allText).join('');
+  return '';
+}
+
+// Attributes survive as the TYPE-correct value, not just as a substring of the
+// serialized blob. These re-import and assert on the concrete re-parsed node.
+describe('git-sync converter: lose-prone atoms keep their VALUES across a round trip', () => {
+  it('A: a NESTED details (inside columns) keeps open:true', async () => {
+    // The raw-HTML path (detailsToHtml) is used for a details nested in a
+    // column/spanned cell — distinct from the top-level details case. Before the
+    // fix it emitted a bare <details>, dropping open every round trip.
+    const original = doc({
+      type: 'columns',
+      content: [
+        {
+          type: 'column',
+          attrs: { width: '100%' },
+          content: [
+            {
+              type: 'details',
+              attrs: { open: true },
+              content: [
+                { type: 'detailsSummary', content: [T('S')] },
+                { type: 'detailsContent', content: [P(T('b'))] },
+              ],
+            },
+          ],
+        },
+      ],
+    });
+    const md = convertProseMirrorToMarkdown(original);
+    // detailsToHtml must emit the `open` attribute (RED before the fix: it
+    // emitted a bare <details> inside the column).
+    expect(md).toContain('<details open>');
+    const back = await markdownToProseMirror(md);
+    const details = findNode(back, 'details');
+    expect(details).toBeDefined();
+    // The schema parses the present `open` boolean attribute to "" (its raw
+    // value); a DROPPED open parses to the default `false`. Asserting it is no
+    // longer the default proves the nested path now preserves open — parity with
+    // the top-level <details> case. RED before the fix (open === false).
+    expect(details.attrs?.open).not.toBe(false);
+  });
+
+  it('D: htmlEmbed source VALUE and height survive', async () => {
+    const original = doc({
+      type: 'htmlEmbed',
+      attrs: { source: '<b>hi</b>', height: 300 },
+    });
+    const md = convertProseMirrorToMarkdown(original);
+    const back = await markdownToProseMirror(md);
+    const embed = findNode(back, 'htmlEmbed');
+    expect(embed).toBeDefined();
+    // The exact raw source must decode back identically (base64 round trip).
+    expect(embed.attrs?.source).toBe('<b>hi</b>');
+    expect(embed.attrs?.height).toBe(300);
+  });
+
+  it('E: footnote definition BODY survives and its id matches the reference', async () => {
+    const original = doc(
+      P(T('x'), { type: 'footnoteReference', attrs: { id: 'fn1' } }),
+      {
+        type: 'footnotesList',
+        content: [
+          {
+            type: 'footnoteDefinition',
+            attrs: { id: 'fn1' },
+            content: [P(T('note'))],
+          },
+        ],
+      },
+    );
+    const md = convertProseMirrorToMarkdown(original);
+    const back = await markdownToProseMirror(md);
+    const list = findNode(back, 'footnotesList');
+    const def = findNode(back, 'footnoteDefinition');
+    const ref = findNode(back, 'footnoteReference');
+    expect(list).toBeDefined();
+    expect(def).toBeDefined();
+    expect(ref).toBeDefined();
+    // The note text rode along, not just the empty wrapper.
+    expect(allText(def)).toContain('note');
+    // The reference still points at the matching definition.
+    expect(ref.attrs?.id).toBe(def.attrs?.id);
+  });
+
+  it('F: transclusionReference keeps BOTH sourcePageId and transclusionId', async () => {
+    const original = doc({
+      type: 'transclusionReference',
+      attrs: { sourcePageId: 'PAGE_X', transclusionId: 'TR_Y' },
+    });
+    const md = convertProseMirrorToMarkdown(original);
+    const back = await markdownToProseMirror(md);
+    const ref = findNode(back, 'transclusionReference');
+    expect(ref).toBeDefined();
+    expect(ref.attrs?.sourcePageId).toBe('PAGE_X');
+    expect(ref.attrs?.transclusionId).toBe('TR_Y');
+  });
+
+  it('F: transclusionSource keeps its id and re-parses its child body', async () => {
+    const original = doc({
+      type: 'transclusionSource',
+      attrs: { id: 'SRC_Z' },
+      content: [P(T('shared body'))],
+    });
+    const md = convertProseMirrorToMarkdown(original);
+    const back = await markdownToProseMirror(md);
+    const src = findNode(back, 'transclusionSource');
+    expect(src).toBeDefined();
+    expect(src.attrs?.id).toBe('SRC_Z');
+    expect(allText(src)).toContain('shared body');
+  });
+});