test(git-sync): add reviewer-requested coverage across engine, server, client

Implements the test cases called out in the PR #119 review threads
(code-review, test-strategy report, red-team) — TESTS ONLY, no production
code changes.

packages/git-sync (vitest):
- lib converter/markdown gaps: pageBreak data-loss (it.fails repro),
  subpages lossy round-trip, nested/fenced callouts, ol->taskList bridge,
  column.width number<->string drift, empty details.
- engine units: parentFolderFile, planReconciliation swap/chained move,
  buildVaultLayout last-resort-by-id, firstDivergence, applyPushActions /
  applyPullActions failure isolation.
- real temp-git integration: diffNameStatus -z rename+add/modify
  alignment, copy-line behavior, per-invocation committer identity (no
  leak into repo/global config).
- ENFORCED type-level GitSyncClient contract via vitest typecheck over a
  *.test-d.ts file (tsconfig.vitest.json; build tsconfig untouched).

apps/server (jest):
- orchestrator: delete-cap neutralization + fail-safe, Redis lock / mutex
  skip ladder + release-on-throw, merge guard, pull/push order, remote
  template substitution, poll lifecycle.
- page-change listener: loop-guard, debounce coalescing, id resolution,
  error swallowing.
- vault registry, controller authz (trigger + status), env
  validation/getters, page.service git-sync provenance stamping,
  persistence precedence (agent > git-sync > user) + no boundary snapshot,
  space.service audit-delta, space.repo jsonb-merge, converter-gate corpus
  extension (mention/math/details/marks).

apps/client (vitest + testing-library):
- history-item git-sync badge: render gating + non-clickable.
- edit-space-form toggle: initial state, optimistic payload, rollback on
  error, disabled states.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

packages/git-sync/test/engine-gaps.test.ts

@@ -0,0 +1,435 @@
+import { describe, expect, it, vi, beforeEach, afterEach } from 'vitest';
+import { parentFolderFile, applyPushActions } from '../src/engine/push';
+import type { ApplyPushDeps, PushActions } from '../src/engine/push';
+import { planReconciliation } from '../src/engine/reconcile';
+import { buildVaultLayout, type PageNode } from '../src/engine/layout';
+import { sanitizeTitle } from '../src/engine/sanitize';
+import { firstDivergence } from '../src/engine/roundtrip-helpers';
+import { applyPullActions } from '../src/engine/pull';
+import type { PullActions, ApplyPullActionsDeps } from '../src/engine/pull';
+import type { DeletionDecision } from '../src/engine/reconcile';
+import { serializeDocmostMarkdownBody } from '../src/lib/index';
+
+// Engine-layer coverage gaps flagged by the PR #119 reviewers (test-strategy
+// report, Module 2 `src/engine`). Each block targets a specific under-covered
+// branch directly. PURE units (no IO) are driven by plain inputs; the push/pull
+// appliers are driven by FAKES that record calls — no real git/fs/network.
+
+// --- 1. push.ts:parentFolderFile — move<->rename classification lynchpin -----
+//
+// `parentFolderFile(path)` returns the parent FOLDER's `.md` file for a vault-
+// relative path (the enclosing folder one level up, SPEC §5 path-as-truth), or
+// `null` for a root-level path with no enclosing folder. It is the lynchpin of
+// the move-vs-rename classifier, so it is tested directly here (it was only
+// covered indirectly before): root-level, deep nesting, and — critically —
+// names CONTAINING DOTS (the lastIndexOf('/') split must not be confused by a
+// dot in a segment; only the LAST slash matters).
+describe('parentFolderFile (push.ts)', () => {
+  it('returns null for a root-level path (no enclosing folder)', () => {
+    expect(parentFolderFile('Child.md')).toBeNull();
+    // A bare name with no slash at all is also root-level.
+    expect(parentFolderFile('README.md')).toBeNull();
+  });
+
+  it('returns the immediate enclosing folder file for a one-level path', () => {
+    expect(parentFolderFile('Space/Child.md')).toBe('Space.md');
+  });
+
+  it('returns the DEEPEST enclosing folder file for a deeply nested path', () => {
+    // Only the last slash matters: the parent is the immediate folder, turned
+    // into its `<folder>.md` page file (NOT the space root).
+    expect(parentFolderFile('Space/Parent/Sub/Child.md')).toBe(
+      'Space/Parent/Sub.md',
+    );
+  });
+
+  it('handles names CONTAINING DOTS without splitting on the dot', () => {
+    // A dot in a folder/file segment must not be mistaken for the path split.
+    // The split is purely on the LAST '/', so the `.md` is appended to the whole
+    // parent dir verbatim (dots and all).
+    expect(parentFolderFile('Space/v1.2.3/Child.md')).toBe('Space/v1.2.3.md');
+    expect(parentFolderFile('a.b/c.d.md')).toBe('a.b.md');
+    // A dotted root-level name still has no enclosing folder.
+    expect(parentFolderFile('v1.2.3.md')).toBeNull();
+  });
+});
+
+// --- 2. reconcile.ts:planReconciliation — chained/swap move (no data loss) ----
+//
+// A collision where one move's TARGET equals another move's OLD path is the
+// classic data-loss trap: naively removing the second move's old path would
+// clobber the first move's freshly-written file. The planner must flag the
+// reused old path `removeOldPath:false` so the caller never removes it. Both the
+// chained-move and the full swap are asserted (no clobber, no loss).
+describe('planReconciliation (reconcile.ts) — chained / swap move', () => {
+  it('chained move: A target == B old path -> B keeps its old path (no clobber)', () => {
+    // B is at b.md and moves to c.md; A is at a.md and moves to b.md. A's TARGET
+    // path (b.md) is exactly B's OLD path. Removing b.md for B's move would
+    // destroy A's just-written file, so B's move must record removeOldPath:false.
+    const live = [
+      { pageId: 'A', relPath: 'b.md' },
+      { pageId: 'B', relPath: 'c.md' },
+    ];
+    const existing = [
+      { pageId: 'A', relPath: 'a.md' },
+      { pageId: 'B', relPath: 'b.md' },
+    ];
+    const plan = planReconciliation(live, existing);
+
+    // Both pages are (re)written at their new paths; nothing is absence-deleted.
+    expect(plan.toWrite).toEqual([
+      { pageId: 'A', relPath: 'b.md' },
+      { pageId: 'B', relPath: 'c.md' },
+    ]);
+    expect(plan.toDelete).toEqual([]);
+
+    const moveOf = (id: string) => plan.moved.find((m) => m.pageId === id)!;
+    // A's old path (a.md) is free -> safe to remove.
+    expect(moveOf('A')).toEqual({
+      pageId: 'A',
+      fromRelPath: 'a.md',
+      toRelPath: 'b.md',
+      removeOldPath: true,
+    });
+    // B's old path (b.md) is reused by A's write -> MUST NOT be removed.
+    expect(moveOf('B')).toEqual({
+      pageId: 'B',
+      fromRelPath: 'b.md',
+      toRelPath: 'c.md',
+      removeOldPath: false,
+    });
+  });
+
+  it('swap move: A<->B exchange paths -> BOTH old paths are kept (no loss)', () => {
+    // A and B swap: A a.md -> b.md, B b.md -> a.md. Each old path is the OTHER
+    // page's live target, so NEITHER may be removed (the writes own them).
+    const live = [
+      { pageId: 'A', relPath: 'b.md' },
+      { pageId: 'B', relPath: 'a.md' },
+    ];
+    const existing = [
+      { pageId: 'A', relPath: 'a.md' },
+      { pageId: 'B', relPath: 'b.md' },
+    ];
+    const plan = planReconciliation(live, existing);
+
+    expect(plan.toDelete).toEqual([]);
+    // Both pages written at their swapped destinations.
+    expect(plan.toWrite).toEqual([
+      { pageId: 'A', relPath: 'b.md' },
+      { pageId: 'B', relPath: 'a.md' },
+    ]);
+    // Both moves recorded, both with removeOldPath:false (the swap is loss-free).
+    expect(plan.moved).toEqual([
+      {
+        pageId: 'A',
+        fromRelPath: 'a.md',
+        toRelPath: 'b.md',
+        removeOldPath: false,
+      },
+      {
+        pageId: 'B',
+        fromRelPath: 'b.md',
+        toRelPath: 'a.md',
+        removeOldPath: false,
+      },
+    ]);
+  });
+});
+
+// --- 3. layout.ts:buildVaultLayout — last-resort-by-id branch (~L135-139) ------
+//
+// The final full-path uniqueness pass has two fallbacks for a colliding leaf:
+// first re-stem with the sanitized slugId, and — if STILL colliding — append the
+// globally-unique sanitized pageId as a last resort. That id branch is reached
+// when FOUR pages share the SAME title AND slugId in the SAME (orphan) bucket:
+// the name pass only calls `disambiguate` ONCE, so the 3rd and 4th pages collide
+// in the FINAL pass, where the 4th's slugId-disambiguated stem ALSO collides
+// (with the 3rd's), forcing the id suffix.
+describe('buildVaultLayout (layout.ts) — last-resort-by-id disambiguation', () => {
+  it('falls through to the globally-unique pageId when title+slugId both collide', () => {
+    // Four orphans (parent outside the input set -> they all bucket at the root)
+    // with identical title "A" and identical slugId "s".
+    const pages: PageNode[] = [
+      { id: 'id1', title: 'A', slugId: 's', parentPageId: 'missing' },
+      { id: 'id2', title: 'A', slugId: 's', parentPageId: 'missing' },
+      { id: 'id3', title: 'A', slugId: 's', parentPageId: 'missing' },
+      { id: 'id4', title: 'A', slugId: 's', parentPageId: 'missing' },
+    ];
+    const layout = buildVaultLayout(pages);
+
+    // The disambiguation ladder:
+    //   id1 -> "A"            (name pass, free)
+    //   id2 -> "A ~s"         (name pass, slugId suffix)
+    //   id3 -> "A ~s ~s"      (FINAL pass, first attempt: slugId suffix)
+    //   id4 -> "A ~s ~s ~id4" (FINAL pass, LAST RESORT: sanitized pageId suffix)
+    expect(layout.get('id1')!.stem).toBe('A');
+    expect(layout.get('id2')!.stem).toBe('A ~s');
+    expect(layout.get('id3')!.stem).toBe('A ~s ~s');
+    // The last-resort branch appends the sanitized id (globally unique).
+    expect(layout.get('id4')!.stem).toBe(`A ~s ~s ~${sanitizeTitle('id4')}`);
+
+    // All four full paths are unique (the invariant the branch protects).
+    const pathOf = (e: { segments: string[]; stem: string }) =>
+      [...e.segments, e.stem].join('/');
+    const paths = ['id1', 'id2', 'id3', 'id4'].map((id) =>
+      pathOf(layout.get(id)!),
+    );
+    expect(new Set(paths).size).toBe(4);
+    // All orphans bucket at the vault root (segments: []).
+    for (const id of ['id1', 'id2', 'id3', 'id4']) {
+      expect(layout.get(id)!.segments).toEqual([]);
+    }
+  });
+});
+
+// --- 4. roundtrip-helpers.ts:firstDivergence — exported but 0% covered --------
+//
+// `firstDivergence(a, b)` deep-compares two values and returns either `null`
+// (equal) or `{ path, a, b }` locating the FIRST point of difference. Contract
+// learned by reading the function: arrays compare length first (`$.length`),
+// nested paths build a JSON-pointer-ish `$.x.y[i].z`, and a type/null mismatch
+// is reported at the current path with the raw differing values.
+describe('firstDivergence (roundtrip-helpers.ts)', () => {
+  it('returns null for deeply equal values (no divergence)', () => {
+    expect(firstDivergence({ a: 1, b: [1, 2, { c: 'x' }] }, { a: 1, b: [1, 2, { c: 'x' }] })).toBeNull();
+    expect(firstDivergence(42, 42)).toBeNull();
+    expect(firstDivergence(null, null)).toBeNull();
+    expect(firstDivergence([], [])).toBeNull();
+  });
+
+  it('locates a divergence at a leaf by path', () => {
+    expect(firstDivergence({ a: 1 }, { a: 2 })).toEqual({ path: '$.a', a: 1, b: 2 });
+  });
+
+  it('locates a divergence deep inside a nested array/object by path', () => {
+    const d = firstDivergence(
+      { x: { y: [1, { z: 'a' }] } },
+      { x: { y: [1, { z: 'b' }] } },
+    );
+    expect(d).toEqual({ path: '$.x.y[1].z', a: 'a', b: 'b' });
+  });
+
+  it('reports an array length mismatch at `<path>.length`', () => {
+    expect(firstDivergence([1, 2], [1, 2, 3])).toEqual({
+      path: '$.length',
+      a: 2,
+      b: 3,
+    });
+  });
+
+  it('reports a type mismatch (and null vs object) at the current path', () => {
+    expect(firstDivergence(1, '1')).toEqual({ path: '$', a: 1, b: '1' });
+    expect(firstDivergence(null, {})).toEqual({ path: '$', a: null, b: {} });
+    // array vs object at the same path
+    expect(firstDivergence([], {})).toEqual({ path: '$', a: [], b: {} });
+  });
+});
+
+// --- 5. push.ts:applyPushActions — prefetch-move failure isolation ------------
+//
+// The reviewer asked to exercise the per-entry try/catch around the rename/move
+// PREFETCH (push.ts ~L644-672): one move's prefetch should fail in isolation
+// while OTHER actions still apply. IMPORTANT FINDING (documented, not a skip of
+// the invariant): the prefetch helpers (`resolveParentPageIdViaTree`,
+// `metaAtViaTree`) SWALLOW their own IO errors internally (each wraps readFile /
+// showFileAtRef / parseDocmostMarkdown in try/catch and returns null), so an
+// injected `readFile`/`showFileAtRef` throw NEVER propagates into the L644-672
+// catch — that catch is defensive dead code reachable only by a future change to
+// the helpers (the source comment says exactly this). It therefore cannot be hit
+// through the public deps WITHOUT modifying production code (forbidden here).
+//
+// What IS testable — and is the invariant the reviewer cares about — is the
+// OBSERVABLE isolation: a move whose tree files are unreadable is isolated (it
+// resolves to a no-op Docmost call, never aborting the batch) while updates,
+// creates and deletes in the SAME batch still apply, and the refs still advance.
+describe('applyPushActions (push.ts) — move prefetch isolation', () => {
+  beforeEach(() => {
+    vi.spyOn(console, 'log').mockImplementation(() => {});
+    vi.spyOn(console, 'warn').mockImplementation(() => {});
+  });
+  afterEach(() => vi.restoreAllMocks());
+
+  function makeClient() {
+    return {
+      importPageMarkdown: vi.fn(async () => ({ updatedAt: 'u' })),
+      createPage: vi.fn(async () => ({ data: { id: 'new-id' } })),
+      deletePage: vi.fn(async () => ({})),
+      movePage: vi.fn(async () => ({})),
+      renamePage: vi.fn(async () => ({})),
+    };
+  }
+
+  it('isolates a move whose tree reads are unreadable; other actions still apply', async () => {
+    const client = makeClient();
+    const git = {
+      updateRef: vi.fn(async () => {}),
+      fastForwardBranch: vi.fn(async () => ({ ok: true })),
+      // The OLD-side parent/meta reads resolve to null (absent at last-pushed).
+      showFileAtRef: vi.fn(async () => null),
+    };
+    // The update file exists and is readable; the move's NEW-path tree reads
+    // throw (simulating an unreadable/missing parent folder file at `current`).
+    const store: Record<string, string> = {
+      'Up.md': serializeDocmostMarkdownBody(
+        { version: 1, pageId: 'u1', title: 'U', spaceId: 'sp' } as any,
+        'body',
+      ),
+    };
+    const deps: ApplyPushDeps = {
+      client,
+      git,
+      readFile: vi.fn(async (p: string) => {
+        if (p in store) return store[p];
+        throw new Error(`unreadable ${p}`);
+      }),
+      writeFile: vi.fn(async () => {}),
+    };
+    const actions: PushActions = {
+      creates: [],
+      updates: [{ pageId: 'u1', path: 'Up.md' }],
+      deletes: [{ pageId: 'd1' }],
+      renamesMoves: [
+        { pageId: 'pg', oldPath: 'Old/C.md', newPath: 'New/C.md' },
+      ],
+      skipped: [],
+    };
+
+    const res = await applyPushActions(deps, actions, 'COMMIT-SHA');
+
+    // The update and the delete in the SAME batch still applied.
+    expect(res.updated).toBe(1);
+    expect(res.deleted).toBe(1);
+    expect(client.importPageMarkdown).toHaveBeenCalledWith('u1', store['Up.md']);
+    expect(client.deletePage).toHaveBeenCalledWith('d1');
+
+    // The broken move was ISOLATED: no movePage/renamePage call, recorded as a
+    // graceful no-op (both parents resolve to ROOT/null, no title -> nothing to
+    // do), NOT a fatal error.
+    expect(client.movePage).not.toHaveBeenCalled();
+    expect(client.renamePage).not.toHaveBeenCalled();
+    expect(res.moved).toBe(0);
+    expect(res.renamed).toBe(0);
+    expect(res.noops).toHaveLength(1);
+    expect(res.noops[0]).toMatchObject({ pageId: 'pg', reason: 'path-only-rename' });
+
+    // No failures -> the refs advance (a clean batch is not blocked by the
+    // isolated, gracefully-handled move).
+    expect(res.failures).toEqual([]);
+    expect(res.lastPushedAdvanced).toBe(true);
+    expect(git.updateRef).toHaveBeenCalledWith(expect.any(String), 'COMMIT-SHA');
+  });
+});
+
+// --- 6. pull.ts:applyPullActions — failedPageIds keyed per-pageId -------------
+//
+// `failedPageIds` is keyed by pageId: when MULTIPLE moves each want their old
+// path removed, but ONE page's new-path write fails, ONLY that page's old path
+// must be KEPT (the ⭐ data-loss guard) — every OTHER page's old path is still
+// removed. This proves the set is keyed by pageId (the failing one only), not a
+// coarse all-or-nothing gate.
+describe('applyPullActions (pull.ts) — failedPageIds keyed per-pageId', () => {
+  const VAULT = '/vault';
+  const APPLY: DeletionDecision = { apply: true };
+
+  beforeEach(() => {
+    vi.spyOn(console, 'log').mockImplementation(() => {});
+    vi.spyOn(console, 'warn').mockImplementation(() => {});
+    vi.spyOn(console, 'error').mockImplementation(() => {});
+  });
+  afterEach(() => vi.restoreAllMocks());
+
+  function makeClient() {
+    return {
+      getPageJson: vi.fn(async (pageId: string) => ({
+        id: pageId,
+        slugId: `slug-${pageId}`,
+        title: `Title ${pageId}`,
+        spaceId: 'space',
+        parentPageId: null,
+        updatedAt: '2026-01-01T00:00:00.000Z',
+        content: {
+          type: 'doc',
+          content: [
+            { type: 'paragraph', content: [{ type: 'text', text: pageId }] },
+          ],
+        },
+      })),
+    };
+  }
+  function makeGit() {
+    return {
+      stageAll: vi.fn(async () => {}),
+      commit: vi.fn(async () => true),
+      checkout: vi.fn(async () => {}),
+      merge: vi.fn(async () => ({ ok: true, conflict: false, output: '' })),
+    };
+  }
+  function makeFs(failWriteFor: Set<string>) {
+    const rms: string[] = [];
+    const fs = {
+      writeFile: vi.fn(async (abs: string) => {
+        if (failWriteFor.has(abs)) throw new Error(`write failed for ${abs}`);
+      }),
+      mkdir: vi.fn(async () => {}),
+      rm: vi.fn(async (abs: string) => {
+        rms.push(abs);
+      }),
+    };
+    return { fs, rms };
+  }
+
+  it('keeps ONLY the failing page old path; the other moves still remove theirs', async () => {
+    // Two moves, both removeOldPath:true. Page "ok" writes fine; page "bad"
+    // fails its new-path write. Only "bad"'s old path must be kept.
+    const client = makeClient();
+    const git = makeGit();
+    const fs = makeFs(new Set(['/vault/NewBad/Bad.md']));
+
+    const deps: ApplyPullActionsDeps = {
+      client,
+      git,
+      writeFile: fs.fs.writeFile,
+      mkdir: fs.fs.mkdir,
+      rm: fs.fs.rm,
+    };
+    const actions: PullActions = {
+      toWrite: [
+        { pageId: 'ok', relPath: 'NewOk/Ok.md' },
+        { pageId: 'bad', relPath: 'NewBad/Bad.md' },
+      ],
+      moved: [
+        {
+          pageId: 'ok',
+          fromRelPath: 'OldOk/Ok.md',
+          toRelPath: 'NewOk/Ok.md',
+          removeOldPath: true,
+        },
+        {
+          pageId: 'bad',
+          fromRelPath: 'OldBad/Bad.md',
+          toRelPath: 'NewBad/Bad.md',
+          removeOldPath: true,
+        },
+      ],
+      toDelete: [],
+      deletionDecision: APPLY,
+      existingCount: 2,
+      plannedDeleteCount: 0,
+    };
+
+    const res = await applyPullActions(deps, actions, VAULT);
+
+    // One write succeeded ("ok"), one failed ("bad").
+    expect(res.written).toBe(1);
+    expect(res.failed).toBe(1);
+
+    // The healthy page's old path WAS removed; the failing page's old path was
+    // KEPT (failedPageIds is keyed by pageId -> only "bad" is suppressed).
+    expect(fs.rms).toContain('/vault/OldOk/Ok.md');
+    expect(fs.rms).not.toContain('/vault/OldBad/Bad.md');
+    // Exactly one move old-path removal applied (the healthy one).
+    expect(res.movedApplied).toBe(1);
+    expect(fs.rms).toEqual(['/vault/OldOk/Ok.md']);
+  });
+});