refactor(git-sync): move the PULL->PUSH cycle into the engine as runCycle (PR #119 review, arch #1)

The reconcile choreography (ensureRepo -> merge-check -> ensureBranch ->
checkout('docmost') -> pull -> push) was hand-rolled in the app orchestrator's
driveCycle, duplicating an order the vendored engine owns and could drift from on
upgrade — the failure mode is data clobber. Lift it into @docmost/git-sync as a
single entry point, `runCycle(deps)`. The orchestrator now calls runCycle and
keeps only the lock (its caller) and the gitmost-specific delete-cap POLICY,
injected as the `resolveApplyClient` hook (the engine does the dry-run, hands the
hook the planned delete count — Infinity if planning failed — and uses whatever
client it returns for the apply). driveCycle drops from ~150 lines to ~30.

Tests:
- engine test/cycle.test.ts: composition (merge-in-progress short-circuit;
  ensureRepo->ensureBranch->checkout staging order before the pull; the cap hook
  is consulted with the planned count; no dry-run when no hook).
- engine test/cycle-roundtrip.test.ts: runCycle against a REAL VaultGit in a temp
  repo with a faked Docmost client — a git-originated CREATE flows pull->push and
  the assigned pageId is written back; an unresolved merge short-circuits before
  any client call.
- orchestrator spec rewired to mock runCycle and assert the wiring + the
  resolveApplyClient cap policy (the engine-internal cycle-order/merge tests moved
  to the engine).

Validated end to end on a live stand (real Postgres/Redis + server): a git clone
-> edit -> push over the /git remote round-trips the change into the Docmost page
through the refactored cycle.

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

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

@@ -0,0 +1,169 @@
+import { execFile } from "node:child_process";
+import { mkdtemp, rm, writeFile, readFile } from "node:fs/promises";
+import { tmpdir } from "node:os";
+import { join } from "node:path";
+import { promisify } from "node:util";
+import { afterEach, beforeAll, describe, expect, it, vi } from "vitest";
+import { runCycle } from "../src/engine/cycle";
+import type { CycleFs } from "../src/engine/cycle";
+import { VaultGit } from "../src/engine/git";
+import type { Settings } from "../src/engine/settings";
+import { serializeDocmostMarkdownBody } from "../src/lib/index";
+
+const execFileAsync = promisify(execFile);
+
+// runCycle (full PULL -> PUSH choreography) against a REAL VaultGit in a temp
+// repo, with a faked Docmost client. This is the integration guard for the
+// extraction of the cycle out of the app orchestrator: it proves runCycle wires
+// the real engine pull + push together against real git and delivers a
+// git-originated CREATE to the client. (The full two-way data-loss invariant —
+// a local main edit surviving a concurrent Docmost edit — is exercised end to
+// end against a live server in the git-sync e2e stand.)
+
+async function gitAvailable(): Promise<boolean> {
+  try {
+    await execFileAsync("git", ["--version"]);
+    return true;
+  } catch {
+    return false;
+  }
+}
+
+function makeSettings(vaultPath: string): Settings {
+  return {
+    docmostApiUrl: "https://docmost.example.com",
+    docmostEmail: "you@example.com",
+    docmostPassword: "secret",
+    docmostSpaceId: "space-1",
+    vaultPath,
+    pollIntervalMs: 15000,
+    debounceMs: 2000,
+    logLevel: "info",
+  } as Settings;
+}
+
+/** Node-fs CycleFs rooted nowhere (absolute paths are passed through). */
+const nodeFs: CycleFs = {
+  readFile: (absPath) => readFile(absPath, "utf8"),
+  writeFile: (absPath, text) => writeFile(absPath, text, "utf8"),
+  mkdir: async (absDir) => {
+    const fs = await import("node:fs/promises");
+    await fs.mkdir(absDir, { recursive: true });
+  },
+  rm: async (absPath) => {
+    const fs = await import("node:fs/promises");
+    await fs.rm(absPath, { force: true });
+  },
+};
+
+/** A minimal recording client; empty Docmost so the pull is a no-op. */
+function makeEmptyClientFake() {
+  return {
+    listSpaceTree: vi.fn(async () => ({ pages: [], complete: true })),
+    getPageJson: vi.fn(),
+    importPageMarkdown: vi.fn(async () => ({ updatedAt: "2026-06-20T00:00:00.000Z" })),
+    createPage: vi.fn(async (title: string) => ({
+      data: { id: "new-id", title },
+      updatedAt: "2026-06-20T00:00:00.000Z",
+    })),
+    deletePage: vi.fn(async () => ({})),
+    movePage: vi.fn(async () => ({})),
+    renamePage: vi.fn(async () => ({})),
+    listRecentSince: vi.fn(async () => []),
+    listTrash: vi.fn(async () => []),
+    restorePage: vi.fn(async () => ({})),
+  };
+}
+
+describe("runCycle against a REAL VaultGit (integration)", () => {
+  let available = false;
+  let dir: string;
+
+  beforeAll(async () => {
+    available = await gitAvailable();
+  });
+
+  afterEach(async () => {
+    if (dir) await rm(dir, { recursive: true, force: true });
+  });
+
+  it("runs the full PULL->PUSH and delivers a git-originated CREATE to the client", async () => {
+    if (!available) return; // skip gracefully when git is unavailable
+
+    dir = await mkdtemp(join(tmpdir(), "docmost-cycle-realgit-"));
+    const git = new VaultGit(dir);
+    await git.ensureRepo();
+    await git.ensureBranch("docmost", "main");
+
+    // A human committed a brand-new file on `main` (meta has title + spaceId but
+    // NO pageId) -> the push side must classify it as a CREATE.
+    const newFile = serializeDocmostMarkdownBody(
+      { version: 1, title: "From Git", spaceId: "space-1" },
+      "a body authored in git",
+    );
+    await writeFile(join(dir, "From Git.md"), newFile, "utf8");
+    await git.stageAll();
+    await git.commit("add From Git.md", {
+      authorName: "Human",
+      authorEmail: "human@local",
+    });
+
+    const client = makeEmptyClientFake();
+    const res = await runCycle({
+      spaceId: "space-1",
+      client: client as any,
+      vault: git,
+      settings: makeSettings(dir),
+      fs: nodeFs,
+      log: () => undefined,
+    });
+
+    expect(res.ran).toBe(true);
+    expect(res.push?.failures).toBe(0);
+    // The CREATE reached Docmost (the push side ran end to end through runCycle).
+    expect(client.createPage).toHaveBeenCalledTimes(1);
+    expect(client.createPage.mock.calls[0][0]).toBe("From Git");
+
+    // The engine wrote the assigned pageId back into the file on disk.
+    const onDisk = await readFile(join(dir, "From Git.md"), "utf8");
+    expect(onDisk).toContain("new-id");
+  });
+
+  it("an unresolved merge short-circuits before any client call", async () => {
+    if (!available) return;
+
+    dir = await mkdtemp(join(tmpdir(), "docmost-cycle-merge-"));
+    const git = new VaultGit(dir);
+    await git.ensureRepo();
+    // Force a conflicting state: create divergent commits on main and docmost
+    // touching the same file, then attempt a merge so the tree is left mid-merge.
+    await writeFile(join(dir, "C.md"), "base\n", "utf8");
+    await git.stageAll();
+    await git.commit("base", { authorName: "h", authorEmail: "h@l" });
+    await git.ensureBranch("docmost", "main");
+    await git.checkout("docmost");
+    await writeFile(join(dir, "C.md"), "docmost-side\n", "utf8");
+    await git.stageAll();
+    await git.commit("docmost edit", { authorName: "h", authorEmail: "h@l" });
+    await git.checkout("main");
+    await writeFile(join(dir, "C.md"), "main-side\n", "utf8");
+    await git.stageAll();
+    await git.commit("main edit", { authorName: "h", authorEmail: "h@l" });
+    // Start a conflicting merge and leave it unresolved.
+    await execFileAsync("git", ["-C", dir, "merge", "docmost"]).catch(() => {});
+
+    const client = makeEmptyClientFake();
+    const res = await runCycle({
+      spaceId: "space-1",
+      client: client as any,
+      vault: git,
+      settings: makeSettings(dir),
+      fs: nodeFs,
+      log: () => undefined,
+    });
+
+    expect(res).toEqual({ ran: false, skipped: "merge-in-progress" });
+    expect(client.listSpaceTree).not.toHaveBeenCalled();
+    expect(client.createPage).not.toHaveBeenCalled();
+  });
+});

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

@@ -0,0 +1,118 @@
+import { describe, it, expect, vi } from "vitest";
+import { runCycle, type RunCycleDeps } from "../src/engine/cycle";
+
+// A fake VaultGit recording the staging calls. An EMPTY vault/tree lets the real
+// readExisting/computePullActions/applyPullActions/runPush run trivially (no
+// files, no pages) so we can assert runCycle's choreography without real git.
+function fakeVault(overrides: Record<string, any> = {}) {
+  const order: string[] = [];
+  const rec =
+    (name: string, ret?: any) =>
+    async (...args: any[]) => {
+      order.push(args.length ? `${name}:${args.join(",")}` : name);
+      return ret;
+    };
+  const vault: any = {
+    order,
+    assertGitAvailable: rec("assertGitAvailable"),
+    ensureRepo: rec("ensureRepo"),
+    isMergeInProgress: vi.fn(async () => false),
+    ensureBranch: rec("ensureBranch"),
+    checkout: rec("checkout"),
+    listTrackedFiles: vi.fn(async () => [] as string[]),
+    // push-side git surface (empty diff -> a clean no-op push)
+    stageAll: rec("stageAll"),
+    commit: rec("commit", { committed: false }),
+    merge: rec("merge", { ok: true, conflict: false, output: "" }),
+    readRef: vi.fn(async () => null),
+    revParse: vi.fn(async () => "0000000000000000000000000000000000000000"),
+    diffNameStatus: vi.fn(async () => [] as any[]),
+    showFileAtRef: vi.fn(async () => ""),
+    updateRef: rec("updateRef"),
+    fastForwardBranch: rec("fastForwardBranch", { ok: true }),
+    ...overrides,
+  };
+  return vault;
+}
+
+function baseDeps(vault: any, over: Partial<RunCycleDeps> = {}): RunCycleDeps {
+  return {
+    spaceId: "space-1",
+    client: {
+      listSpaceTree: vi.fn(async () => ({ pages: [], complete: true })),
+      getPageJson: vi.fn(),
+      importPageMarkdown: vi.fn(),
+      createPage: vi.fn(),
+      deletePage: vi.fn(),
+      movePage: vi.fn(),
+      renamePage: vi.fn(),
+      listRecentSince: vi.fn(),
+      listTrash: vi.fn(),
+      restorePage: vi.fn(),
+    } as any,
+    vault,
+    settings: { vaultPath: "/vault" } as any,
+    fs: {
+      readFile: vi.fn(async () => ""),
+      writeFile: vi.fn(async () => undefined),
+      mkdir: vi.fn(async () => undefined),
+      rm: vi.fn(async () => undefined),
+    },
+    log: vi.fn(),
+    ...over,
+  };
+}
+
+describe("runCycle (composition)", () => {
+  it("short-circuits with skipped:'merge-in-progress' and runs no pull/push", async () => {
+    const vault = fakeVault({ isMergeInProgress: vi.fn(async () => true) });
+    const deps = baseDeps(vault);
+
+    const res = await runCycle(deps);
+
+    expect(res).toEqual({ ran: false, skipped: "merge-in-progress" });
+    // Never advanced to the pull (listSpaceTree) or push.
+    expect(deps.client.listSpaceTree).not.toHaveBeenCalled();
+    expect(vault.order).not.toContain("checkout:docmost");
+  });
+
+  it("stages ensureRepo -> ensureBranch(docmost,main) -> checkout(docmost) BEFORE pulling", async () => {
+    const vault = fakeVault();
+    const deps = baseDeps(vault);
+
+    const res = await runCycle(deps);
+
+    expect(res.ran).toBe(true);
+    const ensureRepoIdx = vault.order.indexOf("ensureRepo");
+    const ensureBranchIdx = vault.order.indexOf("ensureBranch:docmost,main");
+    const checkoutIdx = vault.order.indexOf("checkout:docmost");
+    expect(ensureRepoIdx).toBeGreaterThanOrEqual(0);
+    expect(ensureBranchIdx).toBeGreaterThan(ensureRepoIdx);
+    expect(checkoutIdx).toBeGreaterThan(ensureBranchIdx);
+    expect(deps.client.listSpaceTree).toHaveBeenCalledTimes(1);
+  });
+
+  it("consults the resolveApplyClient hook with the planned delete count", 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 res = await runCycle(deps);
+    expect(res.ran).toBe(true);
+    // With no cap hook there is a single runPush (the apply) — no dry-run pass.
+    // diffNameStatus is read once per runPush; assert a single planning pass.
+    expect(vault.diffNameStatus).toHaveBeenCalledTimes(1);
+  });
+});