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>

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 } =
-    deps;
+  const { spaceId, client, vault, settings, fs, log, signal } = 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(
-    { ...pushDeps, makeClient: () => applyClient },
-    { dryRun: false },
-  );
+  const pushResult = await runPush(pushDeps, { 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, { 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 deps = baseDeps(vault);
 
     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');
+  });
+});