From 33ce3203e096f701c135ede8b8758107ce0820f1 Mon Sep 17 00:00:00 2001 From: claude code agent 227 Date: Wed, 24 Jun 2026 02:08:38 +0300 Subject: [PATCH] refactor(git-sync): move the PULL->PUSH cycle into the engine as runCycle (PR #119 review, arch #1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../services/git-sync.orchestrator.spec.ts | 198 ++++++++---------- .../services/git-sync.orchestrator.ts | 168 ++++----------- packages/git-sync/src/engine/cycle.ts | 169 +++++++++++++++ packages/git-sync/src/index.ts | 7 + .../git-sync/test/cycle-roundtrip.test.ts | 169 +++++++++++++++ packages/git-sync/test/cycle.test.ts | 118 +++++++++++ 6 files changed, 580 insertions(+), 249 deletions(-) create mode 100644 packages/git-sync/src/engine/cycle.ts create mode 100644 packages/git-sync/test/cycle-roundtrip.test.ts create mode 100644 packages/git-sync/test/cycle.test.ts diff --git a/apps/server/src/integrations/git-sync/services/git-sync.orchestrator.spec.ts b/apps/server/src/integrations/git-sync/services/git-sync.orchestrator.spec.ts index f9d5b7d8..15bf0d22 100644 --- a/apps/server/src/integrations/git-sync/services/git-sync.orchestrator.spec.ts +++ b/apps/server/src/integrations/git-sync/services/git-sync.orchestrator.spec.ts @@ -1,26 +1,19 @@ -// Unit tests for the git-sync control plane. The vendored -// engine (@docmost/git-sync) is fully mocked so we exercise ONLY the -// orchestrator's wiring: gating, the Redis leader lock + in-process mutex, -// the pull/push call order, the delete-cap anti-data-loss guard, the remote -// template substitution, and the idempotent interval lifecycle. +// Unit tests for the git-sync control plane. The vendored engine's `runCycle` +// (which owns the PULL->PUSH branch choreography) is mocked so we exercise ONLY +// the orchestrator's wiring: gating, the Redis leader lock + in-process mutex +// (via SpaceLockService), the delete-cap POLICY it injects as `resolveApplyClient`, +// the remote-template substitution in the settings it hands the engine, the +// external-push ingest, and the idempotent interval lifecycle. The cycle +// mechanics themselves are covered by the engine's own cycle round-trip spec. // // The engine mock must be declared before importing the orchestrator so the -// module-graph import binds to the mocked functions (same idiom as the -// datasource spec's top-of-file jest.mock stubs that avoid the React graph). +// module-graph import binds to the mocked function. jest.mock('@docmost/git-sync', () => ({ - readExisting: jest.fn(), - computePullActions: jest.fn(), - applyPullActions: jest.fn(), - runPush: jest.fn(), + runCycle: jest.fn(), })); import { Logger } from '@nestjs/common'; -import { - readExisting, - computePullActions, - applyPullActions, - runPush, -} from '@docmost/git-sync'; +import { runCycle } from '@docmost/git-sync'; import { GitSyncOrchestrator, GitSyncLockHeldError, @@ -29,10 +22,14 @@ import { SpaceLockService } from './space-lock.service'; type AnyMock = jest.Mock; -const readExistingMock = readExisting as unknown as AnyMock; -const computePullActionsMock = computePullActions as unknown as AnyMock; -const applyPullActionsMock = applyPullActions as unknown as AnyMock; -const runPushMock = runPush as unknown as AnyMock; +const runCycleMock = runCycle as unknown as AnyMock; + +/** The default happy-path cycle result the engine returns. */ +const OK_CYCLE = { + ran: true, + pull: { written: 0, deleted: 0, conflict: false }, + push: { mode: 'apply', failures: 0 }, +}; interface BuildOptions { /** Env tunables (only the load-bearing ones are surfaced as overrides). */ @@ -150,16 +147,18 @@ function build(opts: BuildOptions = {}): Built { }; } -/** Reasonable engine defaults so a happy-path driveCycle completes. */ +/** The engine runs a clean cycle by default. */ function primeEngineHappyPath(): void { - readExistingMock.mockResolvedValue({}); - computePullActionsMock.mockReturnValue({ creates: [], updates: [], deletes: [] }); - applyPullActionsMock.mockResolvedValue({ - written: 0, - deleted: 0, - merge: { conflict: false }, - }); - runPushMock.mockResolvedValue({ mode: 'apply', failures: [], planned: { deletes: 0 } }); + runCycleMock.mockResolvedValue(OK_CYCLE); +} + +/** Pull the `resolveApplyClient` hook out of the (single) runCycle call args. */ +function lastResolveApplyClient(): ( + plannedDeletes: number, + client: unknown, +) => unknown { + const calls = runCycleMock.mock.calls; + return calls[calls.length - 1][0].resolveApplyClient; } beforeEach(() => { @@ -242,13 +241,10 @@ describe('GitSyncOrchestrator', () => { }); describe('poisoned-space protection', () => { - it('releases the lock and clears the mutex when driveCycle throws, returning { error }', async () => { + it('releases the lock and clears the mutex when the cycle throws, returning { error }', async () => { const built = build(); jest.spyOn(Logger.prototype, 'error').mockImplementation(() => undefined); - // Make the real apply runPush reject; dry-run still resolves first. - runPushMock - .mockResolvedValueOnce({ mode: 'apply', failures: [], planned: { deletes: 0 } }) - .mockRejectedValueOnce(new Error('boom')); + runCycleMock.mockRejectedValueOnce(new Error('boom')); const res = await built.orchestrator.runOnce('space-1', 'ws-1'); expect(res.ran).toBe(false); @@ -257,16 +253,34 @@ describe('GitSyncOrchestrator', () => { expect(built.redis.eval).toHaveBeenCalledTimes(1); // A subsequent call can re-acquire (mutex cleared after the throw). - runPushMock.mockResolvedValue({ mode: 'apply', failures: [], planned: { deletes: 0 } }); + runCycleMock.mockResolvedValue(OK_CYCLE); const res2 = await built.orchestrator.runOnce('space-1', 'ws-1'); expect(res2.ran).toBe(true); }); }); - describe('merge-in-progress guard', () => { - it("returns skipped:'merge-in-progress' and runs no pull/push", async () => { - jest.spyOn(Logger.prototype, 'warn').mockImplementation(() => undefined); - const built = build({ vaultOverrides: { isMergeInProgress: jest.fn(async () => true) } }); + describe('cycle wiring', () => { + it('drives runCycle with the space vault, the bound client, and settings', async () => { + const built = build(); + await built.orchestrator.runOnce('space-1', 'ws-1'); + + expect(runCycleMock).toHaveBeenCalledTimes(1); + const [deps] = runCycleMock.mock.calls[0]; + expect(deps.spaceId).toBe('space-1'); + expect(deps.vault).toBe(built.vault); + expect(deps.client).toBe(built.client); + expect(deps.settings.vaultPath).toBe('/vaults/space-1'); + expect(typeof deps.resolveApplyClient).toBe('function'); + // The bound datasource identity is the (workspace, service-user) pair. + expect(built.dataSource.bind).toHaveBeenCalledWith({ + workspaceId: 'ws-1', + userId: 'svc-user', + }); + }); + + 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' }); const res = await built.orchestrator.runOnce('space-1', 'ws-1'); expect(res).toEqual({ @@ -274,40 +288,6 @@ describe('GitSyncOrchestrator', () => { ran: false, skipped: 'merge-in-progress', }); - expect(applyPullActionsMock).not.toHaveBeenCalled(); - expect(runPushMock).not.toHaveBeenCalled(); - }); - }); - - describe('cycle order', () => { - it('runs ensureRepo -> ensureBranch(docmost,main) -> checkout(docmost) -> applyPullActions in order', async () => { - const order: string[] = []; - const built = build({ - vaultOverrides: { - ensureRepo: jest.fn(async () => { - order.push('ensureRepo'); - }), - ensureBranch: jest.fn(async (branch: string, base: string) => { - order.push(`ensureBranch:${branch}:${base}`); - }), - checkout: jest.fn(async (branch: string) => { - order.push(`checkout:${branch}`); - }), - }, - }); - applyPullActionsMock.mockImplementation(async () => { - order.push('applyPullActions'); - return { written: 0, deleted: 0, merge: { conflict: false } }; - }); - - await built.orchestrator.runOnce('space-1', 'ws-1'); - - expect(order).toEqual([ - 'ensureRepo', - 'ensureBranch:docmost:main', - 'checkout:docmost', - 'applyPullActions', - ]); }); }); @@ -315,9 +295,9 @@ describe('GitSyncOrchestrator', () => { it('streams the receive-pack FIRST, then runs the Docmost cycle', async () => { const order: string[] = []; const built = build(); - applyPullActionsMock.mockImplementation(async () => { + runCycleMock.mockImplementation(async () => { order.push('cycle'); - return { written: 0, deleted: 0, merge: { conflict: false } }; + return OK_CYCLE; }); const runReceivePack = jest.fn(async () => { order.push('receive-pack'); @@ -341,17 +321,14 @@ describe('GitSyncOrchestrator', () => { // We must never write to the working tree concurrently with a cycle. expect(runReceivePack).not.toHaveBeenCalled(); - expect(applyPullActionsMock).not.toHaveBeenCalled(); + expect(runCycleMock).not.toHaveBeenCalled(); }); it('swallows a post-push cycle error (the push is durable; poll retries)', async () => { jest.spyOn(Logger.prototype, 'error').mockImplementation(() => undefined); const built = build(); - // Dry-run resolves, the real apply rejects → driveCycle throws AFTER the - // receive-pack already succeeded. - runPushMock - .mockResolvedValueOnce({ mode: 'apply', failures: [], planned: { deletes: 0 } }) - .mockRejectedValueOnce(new Error('cycle boom')); + // The cycle throws AFTER the receive-pack already succeeded. + runCycleMock.mockRejectedValueOnce(new Error('cycle boom')); const runReceivePack = jest.fn(async () => undefined); // Does NOT throw — the durable push must not be reported as failed. @@ -373,7 +350,7 @@ describe('GitSyncOrchestrator', () => { ).resolves.toBeUndefined(); // The push is durable on main; the immediate cycle is skipped, not failed. expect(runReceivePack).toHaveBeenCalledTimes(1); - expect(applyPullActionsMock).not.toHaveBeenCalled(); + expect(runCycleMock).not.toHaveBeenCalled(); }); it('refuses (LockHeldError) and runs nothing when git-sync is globally disabled', async () => { @@ -389,22 +366,16 @@ describe('GitSyncOrchestrator', () => { }); describe('delete cap (anti-data-loss)', () => { - it('suppresses deletePage on the apply client (by throwing) when planned deletes exceed the cap', async () => { + // The cap is now a POLICY the orchestrator injects as runCycle's + // `resolveApplyClient` hook; the engine calls it with the dry-run's planned + // delete count. We pull the hook out of the runCycle args and exercise it. + it('suppresses deletePage (by throwing) when planned deletes exceed the cap', async () => { jest.spyOn(Logger.prototype, 'warn').mockImplementation(() => undefined); const built = build({ maxDeletes: 5 }); - // Dry-run plans 9 deletes (over the cap of 5); apply still runs. - runPushMock - .mockResolvedValueOnce({ mode: 'plan', failures: [], planned: { deletes: 9 } }) - .mockResolvedValueOnce({ mode: 'apply', failures: [], planned: { deletes: 0 } }); + await built.orchestrator.runOnce('space-1', 'ws-1'); - const res = await built.orchestrator.runOnce('space-1', 'ws-1'); - expect(res.ran).toBe(true); - expect(runPushMock).toHaveBeenCalledTimes(2); - - // The second runPush (real apply, dryRun:false) got a suppressed client. - const [applyDeps, applyOpts] = runPushMock.mock.calls[1]; - expect(applyOpts).toEqual({ dryRun: false }); - const applyClient = applyDeps.makeClient(); + const resolveApplyClient = lastResolveApplyClient(); + const applyClient = resolveApplyClient(9, built.client) as any; // deletePage is still a function (the engine calls it)... expect(typeof applyClient.deletePage).toBe('function'); // ...but it THROWS, so the engine records a per-page failure and holds @@ -416,32 +387,28 @@ describe('GitSyncOrchestrator', () => { expect(applyClient.createPage).toBe(built.client.createPage); }); - it('fails safe: a throwing dry-run still suppresses deletes and does not throw the cycle', async () => { + it('fails safe: an Infinity planned-delete count (dry-run failed) is suppressed', async () => { jest.spyOn(Logger.prototype, 'warn').mockImplementation(() => undefined); const built = build({ maxDeletes: 5 }); - runPushMock - .mockRejectedValueOnce(new Error('plan failed')) - .mockResolvedValueOnce({ mode: 'apply', failures: [], planned: { deletes: 0 } }); + await built.orchestrator.runOnce('space-1', 'ws-1'); - const res = await built.orchestrator.runOnce('space-1', 'ws-1'); - // The cycle still completes (ran:true), it does NOT throw. - expect(res.ran).toBe(true); - const [applyDeps] = runPushMock.mock.calls[1]; - const applyClient = applyDeps.makeClient(); - // Suppressed via throw (same fail-safe path as the over-cap case). + // runCycle passes Number.POSITIVE_INFINITY when its dry-run planning threw; + // Infinity > cap → suppress (same throwing path). + const resolveApplyClient = lastResolveApplyClient(); + const applyClient = resolveApplyClient( + Number.POSITIVE_INFINITY, + built.client, + ) as any; await expect(applyClient.deletePage('p1')).rejects.toThrow(/suppress/i); expect(built.client.deletePage).not.toHaveBeenCalled(); }); it('passes through the original client when planned deletes are within the cap', async () => { const built = build({ maxDeletes: 5 }); - runPushMock - .mockResolvedValueOnce({ mode: 'plan', failures: [], planned: { deletes: 3 } }) - .mockResolvedValueOnce({ mode: 'apply', failures: [], planned: { deletes: 0 } }); - await built.orchestrator.runOnce('space-1', 'ws-1'); - const [applyDeps] = runPushMock.mock.calls[1]; - const applyClient = applyDeps.makeClient(); + + const resolveApplyClient = lastResolveApplyClient(); + const applyClient = resolveApplyClient(3, built.client) as any; // The ORIGINAL client is used (deletePage forwards to the real one). expect(applyClient).toBe(built.client); await applyClient.deletePage('p1'); @@ -450,12 +417,11 @@ describe('GitSyncOrchestrator', () => { }); describe('remote template substitution', () => { - it('substitutes {spaceId} into the gitRemote handed to runPush', async () => { + it('substitutes {spaceId} into the gitRemote settings handed to the engine', async () => { const built = build({ remoteTemplate: 'git@h:vault-{spaceId}.git' }); await built.orchestrator.runOnce('space-42', 'ws-1'); - // Inspect the settings on the dry-run call (first runPush). - const [dryDeps] = runPushMock.mock.calls[0]; - expect(dryDeps.settings.gitRemote).toBe('git@h:vault-space-42.git'); + const [deps] = runCycleMock.mock.calls[0]; + expect(deps.settings.gitRemote).toBe('git@h:vault-space-42.git'); }); }); diff --git a/apps/server/src/integrations/git-sync/services/git-sync.orchestrator.ts b/apps/server/src/integrations/git-sync/services/git-sync.orchestrator.ts index 507e3f5e..caa49da3 100644 --- a/apps/server/src/integrations/git-sync/services/git-sync.orchestrator.ts +++ b/apps/server/src/integrations/git-sync/services/git-sync.orchestrator.ts @@ -10,13 +10,7 @@ import { dirname } from 'node:path'; import { InjectKysely } from 'nestjs-kysely'; import { KyselyDB } from '@docmost/db/types/kysely.types'; import { sql } from 'kysely'; -import { - type Settings, - readExisting, - computePullActions, - applyPullActions, - runPush, -} from '@docmost/git-sync'; +import { type Settings, runCycle } from '@docmost/git-sync'; import { EnvironmentService } from '../../environment/environment.service'; import { GitmostDataSourceService } from './gitmost-datasource.service'; import { VaultRegistryService } from './vault-registry.service'; @@ -242,10 +236,11 @@ export class GitSyncOrchestrator implements OnModuleInit, OnModuleDestroy { } /** - * The actual engine wiring. Mirrors the engine's own `main`: - * PULL — readExisting -> computePullActions -> applyPullActions, - * PUSH — runPush (dry-run disabled: a real apply). - * The dependency-object shapes match pull.ts/push.ts exactly (see comments). + * Drive ONE reconcile cycle for a space. The PULL->PUSH branch choreography + * lives in the engine's `runCycle` (so it can never drift from the engine it + * ships with); the orchestrator owns only the lock (its caller) and the + * gitmost-specific delete-cap POLICY, injected here as the `resolveApplyClient` + * hook. */ private async driveCycle( spaceId: string, @@ -254,118 +249,41 @@ export class GitSyncOrchestrator implements OnModuleInit, OnModuleDestroy { ): Promise { const settings = this.buildSettings(spaceId); const vault = await this.vaultRegistry.getVault(spaceId); - const vaultRoot = settings.vaultPath; const client = this.dataSource.bind({ workspaceId, userId: serviceUserId }); + const maxDeletes = this.environmentService.getGitSyncMaxDeletesPerCycle(); - // Engine state store is git: make sure the repo + branches exist before any - // tracked-file listing or diff (the engine's pull/push assume an inited repo). - await vault.assertGitAvailable(); - await vault.ensureRepo(); - - // Refuse to run on top of an unresolved merge (SPEC §9): a prior - // conflicting pull leaves the vault mid-merge; the next checkout would fail. - if (await vault.isMergeInProgress()) { - this.logger.warn( - `git-sync[${spaceId}]: vault has an unresolved merge — resolve it (or ` + - `'git merge --abort') and re-run (SPEC §9); skipping cycle.`, - ); - return { spaceId, ran: false, skipped: 'merge-in-progress' }; - } - - await vault.ensureBranch('docmost', 'main'); - // CRITICAL: pull writes happen on the `docmost` branch — applyPullActions - // commits there, then checks out `main` and merges docmost -> main. We MUST be - // on `docmost` BEFORE applying (mirrors the engine's own pull main()), else the - // Docmost content is written straight onto `main`, clobbering local file edits - // before push can diff them. - await vault.checkout('docmost'); - - // --- PULL -------------------------------------------- - // readExisting deps (ReadExistingDeps): list tracked *.md + read by relPath. - const existing = await readExisting({ - listTracked: () => vault.listTrackedFiles('*.md'), - readFile: (relPath) => readFile(`${vaultRoot}/${relPath}`, 'utf8'), - }); - - const tree = await client.listSpaceTree(spaceId); - const pullActions = computePullActions({ - pages: tree.pages, - treeComplete: true, - existing, - }); - - // applyPullActions deps (ApplyPullActionsDeps): the read-side client subset, - // the vault git subset, and ABSOLUTE-path fs ops (mkdir/writeFile/rm). - const pullResult = await applyPullActions( - { - client, - git: vault, + const result = await runCycle({ + spaceId, + client, + vault, + settings, + // ABSOLUTE-path fs primitives the engine cycle injects (it stays IO-free). + fs: { + readFile: (absPath) => readFile(absPath, 'utf8'), writeFile: (absPath, text) => writeFile(absPath, text, 'utf8'), mkdir: (absDir) => mkdir(absDir, { recursive: true }).then(() => undefined), rm: (absPath) => rm(absPath, { force: true }), }, - pullActions, - vaultRoot, - ); - - // --- PUSH -------------------------------------------------- - // runPush deps (PushDeps): settings, the full vault git object (method `this` - // binding must be preserved — pass the object, not bound method refs), a - // makeClient factory returning the push client subset, vault-relative fs - // read/write, and a logger. dryRun:false performs the real Docmost writes. - const pushDeps = { - settings, - git: vault, - makeClient: () => client, - readFile: (relPath: string) => readFile(`${vaultRoot}/${relPath}`, 'utf8'), - writeFile: (relPath: string, text: string) => - writeFile(`${vaultRoot}/${relPath}`, text, 'utf8'), log: (line: string) => this.logger.log(`git-sync[${spaceId}] ${line}`), - }; - - // DEFENSE-IN-DEPTH delete cap. A non-convergent vault - // (e.g. empty/duplicate titles -> colliding paths) can compute PHANTOM - // absence-deletions that slip under the engine's mass-delete FRACTION guard - // and soft-delete real pages. So plan the push as a DRY-RUN FIRST to read the - // delete count, and if it exceeds GIT_SYNC_MAX_DELETES_PER_CYCLE, run the real - // apply with a client whose deletePage is NEUTRALIZED — creates/updates/ - // moves/renames still apply, deletions are skipped this cycle. Never throws. - const maxDeletes = this.environmentService.getGitSyncMaxDeletesPerCycle(); - let suppressDeletes = false; - try { - const dry = await runPush(pushDeps, { dryRun: true }); - const plannedDeletes = dry.planned?.deletes ?? 0; - if (plannedDeletes > maxDeletes) { - suppressDeletes = true; + // DEFENSE-IN-DEPTH delete cap (gitmost-specific policy). A non-convergent + // vault (e.g. empty/duplicate titles -> colliding paths) can compute + // PHANTOM absence-deletions. When the push's planned delete count exceeds + // GIT_SYNC_MAX_DELETES_PER_CYCLE (or planning failed -> Infinity), suppress + // deletes by making deletePage THROW: the engine records each as a per-page + // failure, which keeps `refs/docmost/last-pushed` from advancing past the + // dropped-file commit, so the deletion is RETRIED next cycle rather than + // silently dropped (a no-op that resolved would advance the ref and a pull + // would then recreate the user's deleted files). See PR #119 review. + resolveApplyClient: (plannedDeletes, c) => { + if (plannedDeletes <= maxDeletes) return c; this.logger.warn( `git-sync[${spaceId}]: push delete count ${plannedDeletes} exceeds ` + - `GIT_SYNC_MAX_DELETES_PER_CYCLE=${maxDeletes}; skipping deletions this ` + - `cycle (possible non-convergence / collision). Investigate vault layout.`, + `GIT_SYNC_MAX_DELETES_PER_CYCLE=${maxDeletes}; suppressing deletions ` + + `this cycle (possible non-convergence / collision). Investigate vault ` + + `layout.`, ); - } - } catch (err) { - // A failed dry-run plan must not block the apply, but we cannot trust a - // delete count we never got — fail SAFE by suppressing deletes this cycle. - suppressDeletes = true; - this.logger.warn( - `git-sync[${spaceId}]: push dry-run planning failed (${ - err instanceof Error ? err.message : String(err) - }); skipping deletions this cycle as a precaution.`, - ); - } - - // When over the cap, suppress deletes by making deletePage THROW (every - // other op is forwarded). A throw is recorded by the engine as a per-page - // `failure`, which (a) keeps `refs/docmost/last-pushed` from advancing past - // the commit that dropped the files, and (b) makes the next cycle re-diff - // from the un-advanced ref and re-plan the same deletes — so a transient - // over-cap is retried rather than silently dropped forever. (A no-op that - // resolved would let the engine count `deleted++` with no failure, advance - // the ref, and never replay the deletions — a pull would then recreate the - // user's deleted files. See PR #119 review.) - const applyClient = suppressDeletes - ? { - ...client, + return { + ...c, deletePage: async () => { throw new Error( 'git-sync: delete suppressed this cycle ' + @@ -373,27 +291,11 @@ export class GitSyncOrchestrator implements OnModuleInit, OnModuleDestroy { 'so the deletion is retried, not dropped', ); }, - } - : client; - - const pushResult = await runPush( - { ...pushDeps, makeClient: () => applyClient }, - { dryRun: false }, - ); - - return { - spaceId, - ran: true, - pull: { - written: pullResult.written, - deleted: pullResult.deleted, - conflict: pullResult.merge.conflict, + }; }, - push: { - mode: pushResult.mode, - failures: pushResult.failures?.length ?? 0, - }, - }; + }); + + return { spaceId, ...result }; } // --- poll-safety interval ------------------------------------- diff --git a/packages/git-sync/src/engine/cycle.ts b/packages/git-sync/src/engine/cycle.ts new file mode 100644 index 00000000..84ce53d4 --- /dev/null +++ b/packages/git-sync/src/engine/cycle.ts @@ -0,0 +1,169 @@ +import { VaultGit } from "./git"; +import { GitSyncClient } from "./client.types"; +import { Settings } from "./settings"; +import { readExisting, computePullActions, applyPullActions } from "./pull"; +import { runPush } from "./push"; + +/** + * Absolute-path filesystem primitives the cycle needs. Injected (not imported) + * so the engine stays IO-free and unit-testable. `mkdir` is recursive; `rm` is + * force (a missing file is a no-op). + */ +export interface CycleFs { + readFile: (absPath: string) => Promise; + writeFile: (absPath: string, text: string) => Promise; + mkdir: (absDir: string) => Promise; + rm: (absPath: string) => Promise; +} + +export interface RunCycleDeps { + spaceId: string; + /** The Docmost seam (reads for pull, writes for push). */ + client: GitSyncClient; + /** The per-space git vault (a real working repo). */ + vault: VaultGit; + /** Engine settings; `vaultPath` roots the relPath -> absolute-path mapping. */ + settings: Settings; + fs: CycleFs; + log: (line: string) => void; + /** + * 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 { + ran: boolean; + /** Set when the cycle short-circuited without running pull/push. */ + skipped?: "merge-in-progress"; + pull?: { written: number; deleted: number; conflict: boolean }; + push?: { mode: string; failures: number }; +} + +/** + * Run ONE full reconcile cycle for a space: PULL (Docmost -> vault) then PUSH + * (vault -> Docmost), under the engine's required branch choreography. This is + * the single entry point the app drives — it owns the staging order so it can + * never drift from the engine it ships with. + * + * Staging (the ⭐ data-loss-critical order, SPEC §6/§9): + * 1. assertGitAvailable + ensureRepo (the git state store must exist). + * 2. refuse on an unresolved merge (a prior conflicting pull); next checkout + * would fail otherwise. + * 3. ensureBranch('docmost','main') + checkout('docmost'). Pull writes MUST + * land on `docmost`, not `main`: applyPullActions commits on `docmost`, + * then checks out `main` and merges docmost -> main. Writing Docmost + * 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. + * + * Lock + cap POLICY live in the caller; this owns only the mechanics. + */ +export async function runCycle(deps: RunCycleDeps): Promise { + const { spaceId, client, vault, settings, fs, log, resolveApplyClient } = + deps; + const vaultRoot = settings.vaultPath; + const abs = (relPath: string) => `${vaultRoot}/${relPath}`; + + // 1. The engine state store is git: make sure the repo + branches exist + // before any tracked-file listing or diff. + await vault.assertGitAvailable(); + await vault.ensureRepo(); + + // 2. Refuse to run on top of an unresolved merge (SPEC §9): a prior + // conflicting pull leaves the vault mid-merge; the next checkout would fail. + if (await vault.isMergeInProgress()) { + log( + `vault has an unresolved merge — resolve it (or 'git merge --abort') ` + + `and re-run (SPEC §9); skipping cycle.`, + ); + return { ran: false, skipped: "merge-in-progress" }; + } + + // 3. Pull writes happen on `docmost`; be on it BEFORE applying (see docstring). + await vault.ensureBranch("docmost", "main"); + await vault.checkout("docmost"); + + // 4. PULL -------------------------------------------------------------------- + const existing = await readExisting({ + listTracked: () => vault.listTrackedFiles("*.md"), + readFile: (relPath) => fs.readFile(abs(relPath)), + }); + + const tree = await client.listSpaceTree(spaceId); + const pullActions = computePullActions({ + pages: tree.pages, + treeComplete: tree.complete, + existing, + }); + + const pullResult = await applyPullActions( + { + client, + git: vault, + writeFile: (absPath, text) => fs.writeFile(absPath, text), + mkdir: (absDir) => fs.mkdir(absDir), + rm: (absPath) => fs.rm(absPath), + }, + pullActions, + vaultRoot, + ); + + // 5. PUSH -------------------------------------------------------------------- + const pushDeps = { + settings, + git: vault, + makeClient: () => client, + readFile: (relPath: string) => fs.readFile(abs(relPath)), + writeFile: (relPath: string, text: string) => fs.writeFile(abs(relPath), text), + 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); + } + + const pushResult = await runPush( + { ...pushDeps, makeClient: () => applyClient }, + { dryRun: false }, + ); + + return { + ran: true, + pull: { + written: pullResult.written, + deleted: pullResult.deleted, + conflict: pullResult.merge.conflict, + }, + push: { + mode: pushResult.mode, + failures: pushResult.failures?.length ?? 0, + }, + }; +} diff --git a/packages/git-sync/src/index.ts b/packages/git-sync/src/index.ts index f8588f41..dc87a8cb 100644 --- a/packages/git-sync/src/index.ts +++ b/packages/git-sync/src/index.ts @@ -112,3 +112,10 @@ export { parseSettings, envSchema } from "./engine/settings"; export type { Settings } from "./engine/settings"; export { loadSettingsOrExit } from "./engine/config-errors"; + +export { runCycle } from "./engine/cycle"; +export type { + RunCycleDeps, + RunCycleResult, + CycleFs, +} from "./engine/cycle"; diff --git a/packages/git-sync/test/cycle-roundtrip.test.ts b/packages/git-sync/test/cycle-roundtrip.test.ts new file mode 100644 index 00000000..e6533f5a --- /dev/null +++ b/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 { + 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(); + }); +}); diff --git a/packages/git-sync/test/cycle.test.ts b/packages/git-sync/test/cycle.test.ts new file mode 100644 index 00000000..ea01ae53 --- /dev/null +++ b/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 = {}) { + 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 { + 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); + }); +});