fix(git-sync): address PR #119 review (#1571)

Resolve the code-review findings from comment #1571 on PR #119. Engine (packages/git-sync): - Idempotent CREATE on retry: before createPage, look the page up in the live Docmost tree by (parentPageId, title) and ADOPT it instead of duplicating when a prior cycle created it but failed to persist the pageId back to disk. Only trust a COMPLETE tree for the lookup; fall back to createPage otherwise. Covered by new tests incl. a complete=false regression-lock. - Route applyPullActions diagnostics through an injected logger instead of bare console (thread log from the cycle). - Add a timeout to the git execFile chokepoint (runRaw) so a hung git subprocess cannot wedge a sync cycle. - Translate remaining Russian code comments to English. - Remove dead standalone-CLI code (parseArgs/PushParsedArgs, parseSettings/envSchema, loadSettingsOrExit + config-errors.ts) and the matching index exports/specs; keep the Settings type. - Fix the dangling docs link in package.json. - Add a schema-surface snapshot guard so any drift in the vendored document schema is a loud, must-review CI failure (+ provenance header). Server (apps/server): - Add a configurable watchdog timeout to the spawned git http-backend so a stalled push cannot hold the per-space lock forever (GIT_SYNC_BACKEND_TIMEOUT_MS). - Close the in-process TOCTOU window in SpaceLockService.withSpaceLock by reserving the slot synchronously before acquire. - Add tests: removePage git-sync provenance (both branches), ensureServable force-push-protection git configs, and the phase-B+ datasource methods. Docs / build: - AGENTS.md: list git-sync as the fifth workspace package and note the three schema mirrors; fix the dangling git-sync-plan.md backlog link. - pnpm-lock.yaml: add the missing @docmost/git-sync workspace link so pnpm install --frozen-lockfile (CI default) succeeds. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2026-06-26 00:06:44 +03:00
parent 52959de2f3
commit 28d2560dfd
31 changed files with 767 additions and 462 deletions
@@ -375,13 +375,109 @@ describe('GitmostDataSourceService', () => {
  describe('restorePage', () => {
    it('restores via the repo restore path scoped to the workspace', async () => {
      const { service, mocks } = build();
-      await service.bind(CTX).restorePage('p1');
+      const res = await service.bind(CTX).restorePage('p1');
      // Stamps lastUpdatedSource='git-sync' on restore (loop-guard, PR #119).
      expect(mocks.pageRepo.restorePage).toHaveBeenCalledWith(
        'p1',
        'ws-1',
        'git-sync',
      );
+      expect(res).toEqual({ id: 'p1' });
+    });
+  });
+
+  // Phase-B+ continuous-sync methods: not yet called by the engine but wired into
+  // the GitSyncClient seam (PR #119 review #5). Exercised via the bound client.
+  describe('listRecentSince', () => {
+    it('queries non-deleted pages newest-first and ISO-stringifies updatedAt', async () => {
+      const rows = [
+        {
+          id: 'p1',
+          slugId: 's1',
+          title: 'A',
+          parentPageId: null,
+          spaceId: 'space-1',
+          updatedAt: new Date('2026-06-20T10:00:00.000Z'),
+        },
+      ];
+      const { service, mocks } = build(rows);
+      const qb = mocks.db.selectFrom.mock.results; // populated after the call
+
+      const out = (await service
+        .bind(CTX)
+        .listRecentSince('space-1', '2026-06-19T00:00:00.000Z', 100)) as any[];
+
+      // Query builder shaped against the `pages` table with the expected chain.
+      expect(mocks.db.selectFrom).toHaveBeenCalledWith('pages');
+      const builder = qb[0].value;
+      expect(builder.select).toHaveBeenCalled();
+      expect(builder.orderBy).toHaveBeenCalledWith('updatedAt', 'desc');
+      // deletedAt is null + the conditional spaceId / since / cap clauses.
+      const whereArgs = builder.where.mock.calls.map((c: any[]) => c[0]);
+      expect(whereArgs).toContain('deletedAt');
+      expect(whereArgs).toContain('spaceId');
+      expect(whereArgs).toContain('updatedAt');
+      expect(builder.limit).toHaveBeenCalledWith(100);
+
+      expect(out).toEqual([
+        {
+          id: 'p1',
+          slugId: 's1',
+          title: 'A',
+          parentPageId: null,
+          spaceId: 'space-1',
+          updatedAt: '2026-06-20T10:00:00.000Z',
+        },
+      ]);
+    });
+
+    it('omits the spaceId / since / cap clauses when not supplied', async () => {
+      const { service, mocks } = build([]);
+
+      await service.bind(CTX).listRecentSince(undefined, null);
+
+      const builder = mocks.db.selectFrom.mock.results[0].value;
+      const whereArgs = builder.where.mock.calls.map((c: any[]) => c[0]);
+      // Only the deletedAt-is-null guard; no spaceId / updatedAt> clauses.
+      expect(whereArgs).toEqual(['deletedAt']);
+      expect(builder.limit).not.toHaveBeenCalled();
+    });
+  });
+
+  describe('listTrash', () => {
+    it('queries soft-deleted pages and ISO-stringifies deletedAt (null stays null)', async () => {
+      const rows = [
+        {
+          id: 'p1',
+          slugId: 's1',
+          title: 'Trashed',
+          parentPageId: null,
+          spaceId: 'space-1',
+          deletedAt: new Date('2026-06-21T09:00:00.000Z'),
+        },
+        {
+          id: 'p2',
+          slugId: 's2',
+          title: 'NoDate',
+          parentPageId: null,
+          spaceId: 'space-1',
+          deletedAt: null,
+        },
+      ];
+      const { service, mocks } = build(rows);
+
+      const out = (await service.bind(CTX).listTrash('space-1')) as any[];
+
+      expect(mocks.db.selectFrom).toHaveBeenCalledWith('pages');
+      const builder = mocks.db.selectFrom.mock.results[0].value;
+      const whereCalls = builder.where.mock.calls;
+      // deletedAt is-not null (the trash predicate) + spaceId filter.
+      expect(whereCalls).toContainEqual(['deletedAt', 'is not', null]);
+      expect(whereCalls).toContainEqual(['spaceId', '=', 'space-1']);
+      expect(builder.orderBy).toHaveBeenCalledWith('deletedAt', 'desc');
+
+      expect(out[0].deletedAt).toBe('2026-06-21T09:00:00.000Z');
+      expect(out[1].deletedAt).toBeNull();
    });
  });
 });
@@ -111,25 +111,33 @@ export class SpaceLockService {
    if (this.running.has(spaceId)) {
      return { skipped: 'in-progress' };
    }
-    if (!(await this.acquire(spaceId))) {
-      return { skipped: 'lock-held' };
-    }
+    // Reserve the in-process slot synchronously (before any await) so two
+    // concurrent same-space calls on THIS instance cannot both pass the guard and
+    // race acquire(). Redis NX is already authoritative across replicas; this just
+    // closes the in-process TOCTOU window. Released in the outer finally on every
+    // path (acquire-failure, fn-throw, normal completion).
    this.running.add(spaceId);
-    // Heartbeat: periodically (≈ TTL/3) extend the lock's TTL while `fn` runs so
-    // a long push (client-controlled receive-pack + the Docmost cycle) cannot
-    // outlive the fixed TTL and let a concurrent cycle race the working tree. The
-    // refresh is CAS-guarded (only extends while WE own it). `.unref()` keeps the
-    // timer from holding the event loop open; it is ALWAYS cleared in `finally`.
-    const heartbeat = setInterval(() => {
-      void this.refreshLock(spaceId);
-    }, Math.max(1, Math.floor(GIT_SYNC_LOCK_TTL_MS / 3)));
-    heartbeat.unref?.();
    try {
-      return await fn();
+      if (!(await this.acquire(spaceId))) {
+        return { skipped: 'lock-held' };
+      }
+      // Heartbeat: periodically (≈ TTL/3) extend the lock's TTL while `fn` runs so
+      // a long push (client-controlled receive-pack + the Docmost cycle) cannot
+      // outlive the fixed TTL and let a concurrent cycle race the working tree. The
+      // refresh is CAS-guarded (only extends while WE own it). `.unref()` keeps the
+      // timer from holding the event loop open; it is ALWAYS cleared in `finally`.
+      const heartbeat = setInterval(() => {
+        void this.refreshLock(spaceId);
+      }, Math.max(1, Math.floor(GIT_SYNC_LOCK_TTL_MS / 3)));
+      heartbeat.unref?.();
+      try {
+        return await fn();
+      } finally {
+        clearInterval(heartbeat);
+        await this.release(spaceId);
+      }
    } finally {
-      clearInterval(heartbeat);
      this.running.delete(spaceId);
-      await this.release(spaceId);
    }
  }
 }
@@ -7,18 +7,33 @@
 // `loadGitSync()` bridge (the ESM `@docmost/git-sync` package cannot be
 // `require()`d under jest), so we mock that loader rather than the package.
 import { mkdir } from 'node:fs/promises';
+import { execFile } from 'node:child_process';
 import { loadGitSync } from '../git-sync.loader';

 jest.mock('node:fs/promises', () => ({
  mkdir: jest.fn(async () => undefined),
 }));

+// ensureServable shells out via `promisify(execFile)`; mock execFile with a
+// callback-style fn so promisify resolves. Each `git config <key> <value>` call
+// is recorded so the four config writes (incl. the security-critical
+// receive.denyNonFastForwards=true) can be asserted.
+jest.mock('node:child_process', () => ({
+  execFile: jest.fn((_cmd: string, _args: string[], _opts: any, cb: any) =>
+    cb(null, { stdout: '', stderr: '' }),
+  ),
+}));
+
 // Cheap VaultGit stub: records the path it was constructed with; no shell-out.
-// Declared with a `mock`-prefixed name so jest allows referencing it inside the
-// hoisted `jest.mock` factory below.
+// `ensureRepo` is a resolved jest.fn so ensureServable can call it. Declared with
+// a `mock`-prefixed name so jest allows referencing it inside the hoisted
+// `jest.mock` factory below.
 const mockVaultGit = jest
  .fn()
-  .mockImplementation((path: string) => ({ path }));
+  .mockImplementation((path: string) => ({
+    path,
+    ensureRepo: jest.fn().mockResolvedValue(undefined),
+  }));

 jest.mock('../git-sync.loader', () => ({
  loadGitSync: jest.fn(async () => ({
@@ -32,6 +47,7 @@ import { VaultRegistryService } from './vault-registry.service';
 type AnyMock = jest.Mock;

 const mkdirMock = mkdir as unknown as AnyMock;
+const execFileMock = execFile as unknown as AnyMock;
 const VaultGitMock = mockVaultGit;
 void loadGitSync;

@@ -78,4 +94,52 @@ describe('VaultRegistryService', () => {
      });
    });
  });
+
+  describe('ensureServable', () => {
+    it('ensures the repo then writes the four force-push-protection git configs', async () => {
+      const { service } = build('/vaults');
+
+      const path = await service.ensureServable('space-1');
+      expect(path).toBe('/vaults/space-1');
+
+      // ensureRepo ran first on the cached vault.
+      const vault = await service.getVault('space-1');
+      expect((vault as any).ensureRepo).toHaveBeenCalledTimes(1);
+
+      // Collect every `git config <key> <value>` write.
+      const configWrites = execFileMock.mock.calls
+        .filter(([cmd, args]) => cmd === 'git' && args[0] === 'config')
+        .map(([, args]) => [args[1], args[2]]);
+
+      expect(configWrites).toEqual([
+        ['receive.denyCurrentBranch', 'updateInstead'],
+        // Security-critical: blocks force-push / history rewrites on main.
+        ['receive.denyNonFastForwards', 'true'],
+        ['http.receivepack', 'true'],
+        ['http.uploadpack', 'true'],
+      ]);
+
+      // Every config write targets THIS vault's cwd.
+      for (const [cmd, args, opts] of execFileMock.mock.calls) {
+        if (cmd === 'git' && args[0] === 'config') {
+          expect(opts.cwd).toBe('/vaults/space-1');
+        }
+      }
+    });
+
+    it('rejects (and writes no git config) when ensureRepo rejects', async () => {
+      const { service } = build('/vaults');
+      const vault = await service.getVault('space-1');
+      (vault as any).ensureRepo.mockRejectedValueOnce(new Error('init failed'));
+
+      await expect(service.ensureServable('space-1')).rejects.toThrow(
+        'init failed',
+      );
+
+      const configWrites = execFileMock.mock.calls.filter(
+        ([cmd, args]) => cmd === 'git' && args[0] === 'config',
+      );
+      expect(configWrites).toHaveLength(0);
+    });
+  });
 });