test(git-sync): exhaustive converter coverage + fix 3 round-trip data-loss bugs

Coder↔reviewer design loop (9 rounds, reviewer verdict: exhaustive) produced 92 specs; implemented +123 tests (465 -> 588 passing). The new round-trip coverage exposed three genuine data-loss bugs in the Markdown<->ProseMirror converter, all now FIXED (round-trip is lossless for these): 1. pageBreak was lost on export (no converter case -> rendered to "" and the node vanished). Now emits <div data-type="pageBreak"></div>, which the schema parses back -> round-trips. 2. A block image between blocks left an empty <p> artifact after import-hoisting, producing a phantom blank-gap diff on every sync. markdownToProseMirror now strips content-less paragraphs after generateJSON — with a schema-validity guard that keeps the obligatory single empty paragraph in `content: "block+"` containers (tableCell/tableHeader/blockquote/column/callout/doc), so empty cells/quotes never become an invalid `content: []`. 3. The `code` mark combined with another mark was not byte-stable (emitted nested HTML that the schema's `code` `excludes:"_"` collapsed on import). The converter now emits code-only when `code` co-occurs, matching the editor. New coverage spans media/diagram/details/columns/math/mention attribute round-trips, converter emission branches, git error paths, and engine decision branches. A dedicated test pins the empty-container schema validity (the review catch on the bug-2 fix). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2026-06-23 06:50:20 +03:00
parent c7440fe8a4
commit d215147d13
18 changed files with 2902 additions and 50 deletions
--- a/packages/git-sync/test/apply-pull-actions.test.ts
+++ b/packages/git-sync/test/apply-pull-actions.test.ts
@@ -415,3 +415,250 @@ describe('applyPullActions — merge result is surfaced, not swallowed', () => {
    expect(res.merge.conflict).toBe(false);
  });
 });
+
+// ===========================================================================
+// R-Pull-2 coverage gaps (review-driven): the suppression warning FORKS for
+// `empty-live` and `mass-delete` reasons (pull.ts 278-290), and the
+// fault-tolerant `removePath` catch branch (pull.ts 354-364) where `deps.rm`
+// REJECTS. The existing block above only exercises the `incomplete-fetch`
+// reason and an rm that always succeeds.
+//
+// Helper: build a deps object whose `rm` rejects for a chosen set of absolute
+// paths and resolves otherwise. We override the recording fs's `rm` (a vi.fn)
+// in place so `fs.rms` still records the SUCCESSFUL calls only (a rejecting rm
+// throws before pushing), matching the real `node:fs/promises` semantics where
+// a thrown rm performed no removal.
+function makeFsWithRejectingRm(rejectFor: Set<string>) {
+  const base = makeFs();
+  base.fs.rm = vi.fn(async (abs: string) => {
+    if (rejectFor.has(abs)) {
+      throw new Error(`rm failed for ${abs}`);
+    }
+    base.rms.push(abs);
+  });
+  return base;
+}
+
+describe('applyPullActions — suppression warning forks (empty-live / mass-delete)', () => {
+  it('emits the empty-live warning (with existingCount) and performs no removals', async () => {
+    // SPEC §8 empty-live fork: live fetch returned 0 pages but files are
+    // tracked. Mirrors the incomplete-fetch suppression test, but the message
+    // text + its `existingCount` interpolation are a DISTINCT branch.
+    const { client } = makeClient();
+    const g = makeGit();
+    const fs = makeFs();
+
+    const res = await applyPullActions(
+      deps(client, g.git, fs),
+      actions({
+        toWrite: [{ pageId: 'p1', relPath: 'A.md' }],
+        toDelete: [], // suppressed -> already empty
+        deletionDecision: { apply: false, reason: 'empty-live' },
+        plannedDeleteCount: 3,
+        existingCount: 4,
+      }),
+      VAULT,
+    );
+
+    expect(res.deleted).toBe(0);
+    expect(fs.rms).toEqual([]);
+    // The empty-live message names the tracked-file count and "deletions
+    // suppressed".
+    expect(console.warn).toHaveBeenCalledWith(
+      expect.stringMatching(/live fetch returned 0 pages but 4 file\(s\) are tracked/),
+    );
+    expect(console.warn).toHaveBeenCalledWith(
+      expect.stringMatching(/deletions suppressed/),
+    );
+  });
+
+  it('emits the mass-delete guard warning (with planned AND existing counts) and performs no removals', async () => {
+    // SPEC §8 mass-delete fork (the final else branch): the message
+    // interpolates BOTH plannedDeleteCount and existingCount ("would delete N
+    // of M"), distinct from the other two suppression messages.
+    const { client } = makeClient();
+    const g = makeGit();
+    const fs = makeFs();
+
+    const res = await applyPullActions(
+      deps(client, g.git, fs),
+      actions({
+        toWrite: [{ pageId: 'p1', relPath: 'A.md' }],
+        toDelete: [],
+        deletionDecision: { apply: false, reason: 'mass-delete' },
+        plannedDeleteCount: 5,
+        existingCount: 6,
+      }),
+      VAULT,
+    );
+
+    expect(res.deleted).toBe(0);
+    expect(fs.rms).toEqual([]);
+    expect(console.warn).toHaveBeenCalledWith(
+      expect.stringMatching(/plan would delete 5 of 6 tracked file\(s\) \(mass-delete guard\)/),
+    );
+    expect(console.warn).toHaveBeenCalledWith(
+      expect.stringMatching(/deletions suppressed/),
+    );
+  });
+});
+
+describe('applyPullActions — removePath fault tolerance (rm REJECTS)', () => {
+  it('does NOT reject, logs the failure, and does not count the failed removal', async () => {
+    // pull.ts 354-364: when `deps.rm` throws, removePath logs via console.error
+    // and returns false; the run continues. Existing delete tests use an rm
+    // that always succeeds, leaving this catch branch uncovered.
+    const { client } = makeClient();
+    const g = makeGit();
+    const fs = makeFsWithRejectingRm(new Set(['/vault/Dead.md']));
+
+    const res = await applyPullActions(
+      deps(client, g.git, fs),
+      actions({
+        toWrite: [],
+        toDelete: ['Dead.md'],
+        deletionDecision: APPLY,
+        plannedDeleteCount: 1,
+        existingCount: 1,
+      }),
+      VAULT,
+    );
+
+    // Resolved (not rejected) — the pull is fault-tolerant.
+    expect(res.deleted).toBe(0);
+    // removePath's catch logs "pull: failed to delete Dead.md: ...".
+    expect(console.error).toHaveBeenCalledWith(
+      expect.stringMatching(/failed to .* Dead\.md/),
+      expect.anything(),
+    );
+    // The (would-be) removal never succeeded, so nothing was recorded.
+    expect(fs.rms).toEqual([]);
+  });
+
+  it('counts ONLY successful removals on a partial-failure delete batch (1 reject of 3)', async () => {
+    // pull.ts 388-391 increments `deleted` only when removePath returns true.
+    // Here Dead1/Dead3 succeed and Dead2 rejects -> deleted === 2, and the
+    // deleted>0 subject branch (399-400) fires with written=0.
+    const { client } = makeClient();
+    const g = makeGit();
+    const fs = makeFsWithRejectingRm(new Set(['/vault/Dead2.md']));
+
+    const res = await applyPullActions(
+      deps(client, g.git, fs),
+      actions({
+        toWrite: [],
+        moved: [],
+        toDelete: ['Dead1.md', 'Dead2.md', 'Dead3.md'],
+        deletionDecision: APPLY,
+        plannedDeleteCount: 3,
+        existingCount: 5,
+      }),
+      VAULT,
+    );
+
+    expect(res.deleted).toBe(2);
+    expect(fs.rms).toEqual(['/vault/Dead1.md', '/vault/Dead3.md']);
+    expect(g.committedSubject).toBe('docmost: sync 0 page(s), 2 deleted');
+    // Exactly one rejection was logged (Dead2.md).
+    expect(console.error).toHaveBeenCalledTimes(1);
+    expect(console.error).toHaveBeenCalledWith(
+      expect.stringMatching(/failed to .* Dead2\.md/),
+      expect.anything(),
+    );
+    // The run still reached commit + checkout + merge.
+    expect(g.order).toEqual([
+      'stageAll',
+      'commit:docmost: sync 0 page(s), 2 deleted',
+      'checkout:main',
+      'merge',
+    ]);
+  });
+});
+
+describe('applyPullActions — move old-path removal rejects vs move-write fails', () => {
+  it('a move old-path rm REJECTION does not increment movedApplied but an independent delete still succeeds', async () => {
+    // pull.ts 383 increments movedApplied only when removePath of the old path
+    // succeeds. Here the new-path write SUCCEEDS (so the page is not in
+    // failedPageIds and the move loop proceeds to rm) but the old-path rm
+    // REJECTS — distinct from the move-write-failure guard at 376. An absence
+    // delete in the same run must still succeed independently.
+    const { client } = makeClient();
+    const g = makeGit();
+    const fs = makeFsWithRejectingRm(new Set(['/vault/Old/M.md']));
+
+    const res = await applyPullActions(
+      deps(client, g.git, fs),
+      actions({
+        toWrite: [{ pageId: 'm', relPath: 'New/M.md' }],
+        moved: [
+          {
+            pageId: 'm',
+            fromRelPath: 'Old/M.md',
+            toRelPath: 'New/M.md',
+            removeOldPath: true,
+          },
+        ],
+        toDelete: ['Dead.md'],
+        deletionDecision: APPLY,
+        plannedDeleteCount: 1,
+        existingCount: 3,
+      }),
+      VAULT,
+    );
+
+    expect(res.written).toBe(1);
+    expect(res.movedApplied).toBe(0); // old-path rm failed -> not counted
+    expect(res.deleted).toBe(1); // independent absence delete still succeeded
+    expect(fs.rms).toEqual(['/vault/Dead.md']); // Old/M.md rm threw, not recorded
+    expect(g.committedSubject).toBe('docmost: sync 1 page(s), 1 deleted');
+    // The failure log named the moved old path.
+    expect(console.error).toHaveBeenCalledWith(
+      expect.stringMatching(/failed to .* Old\/M\.md/),
+      expect.anything(),
+    );
+  });
+
+  it('a move-write FAILURE keeps the old path: rm is never attempted for it (data-loss guard, 374-383)', async () => {
+    // Distinct branch from the move-old-path rm rejection above: here the
+    // new-path WRITE itself fails, so `m` enters failedPageIds and the move
+    // loop short-circuits at line 376 BEFORE calling rm — emitting a
+    // console.warn and PRESERVING the old path (the only copy).
+    const { client } = makeClient();
+    const g = makeGit();
+    const fs = makeFs({ failWriteFor: new Set(['/vault/New/M.md']) });
+
+    const res = await applyPullActions(
+      deps(client, g.git, fs),
+      actions({
+        toWrite: [{ pageId: 'm', relPath: 'New/M.md' }],
+        moved: [
+          {
+            pageId: 'm',
+            fromRelPath: 'Old/M.md',
+            toRelPath: 'New/M.md',
+            removeOldPath: true,
+          },
+        ],
+        toDelete: [],
+        deletionDecision: APPLY,
+        plannedDeleteCount: 0,
+        existingCount: 1,
+      }),
+      VAULT,
+    );
+
+    expect(res.written).toBe(0);
+    expect(res.movedApplied).toBe(0);
+    // The old path was NEVER removed (rm not even attempted for it).
+    expect(fs.fs.rm).not.toHaveBeenCalledWith('/vault/Old/M.md');
+    expect(fs.rms).toEqual([]);
+    // The "keeping old path" warning fired exactly once for `m`.
+    const warnCalls = (console.warn as unknown as ReturnType<typeof vi.fn>).mock.calls
+      .map((c: unknown[]) => String(c[0]))
+      .filter((m: string) => m.includes('move write for m failed'));
+    expect(warnCalls.length).toBe(1);
+    expect(warnCalls[0]).toContain('keeping old path Old/M.md');
+    // deleted === 0 -> no ", N deleted" suffix.
+    expect(g.committedSubject).toBe('docmost: sync 0 page(s)');
+  });
+});