fix(git-sync): hold refs on suppressed deletes + stamp delete/restore provenance (PR #119 review)

Two stability warnings from the #119 review:

1. delete-cap no longer drops deletions forever. When planned deletes exceed
   GIT_SYNC_MAX_DELETES_PER_CYCLE the apply client's deletePage now THROWS
   instead of resolving to a no-op. A throw is recorded by the engine as a
   per-page failure, so `refs/docmost/last-pushed` is NOT advanced past the
   commit that dropped the files — the next cycle re-diffs from the un-advanced
   ref and re-plans the same deletes (a transient over-cap is retried, not
   silently dropped and then recreated by the next pull). Previously a resolving
   no-op let the engine count `deleted++` with no failure, advance the ref, and
   never replay the deletions.

2. git-sync soft-delete and restore now stamp provenance. deletePage routes
   GIT_SYNC_PROVENANCE through pageService.removePage, and restorePage stamps
   lastUpdatedSource='git-sync' on the restore update — so the page-change
   listener's loop-guard (skip when lastUpdatedSource==='git-sync') recognizes
   both as its own writes instead of scheduling a wasted echo cycle. Done via a
   backward-compatible optional `lastUpdatedSource` param on
   pageRepo.removePage/restorePage (omitted for ordinary user deletes/restores).

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

apps/server/src/core/page/services/page.service.ts

@@ -1289,8 +1289,18 @@ export class PageService {
     pageId: string,
     userId: string,
     workspaceId: string,
+    // Optional provenance. A git-sync-driven soft-delete stamps
+    // `lastUpdatedSource = 'git-sync'` so the change-listener loop-guard skips
+    // its own write (mirrors the create/update/move provenance branches above).
+    provenance?: AuthProvenanceData,
   ): Promise<void> {
-    await this.pageRepo.removePage(pageId, userId, workspaceId);
+    const isGitSync = provenance?.actor === 'git-sync';
+    await this.pageRepo.removePage(
+      pageId,
+      userId,
+      workspaceId,
+      isGitSync ? 'git-sync' : undefined,
+    );
   }
 
   private async parseProsemirrorContent(

apps/server/src/database/repos/page/page.repo.ts

@@ -294,6 +294,11 @@ export class PageRepo {
     pageId: string,
     deletedById: string,
     workspaceId: string,
+    // Optional provenance marker. When the soft-delete is driven by an automated
+    // data plane (e.g. git-sync), stamp `lastUpdatedSource` so the change-listener
+    // loop-guard recognizes it as its own write and does not schedule an echo
+    // cycle. Omitted for ordinary user deletes (column keeps its prior value).
+    lastUpdatedSource?: string,
   ): Promise<void> {
     const currentDate = new Date();
 
@@ -344,6 +349,7 @@ export class PageRepo {
           .set({
             deletedById: deletedById,
             deletedAt: currentDate,
+            ...(lastUpdatedSource ? { lastUpdatedSource } : {}),
           })
           .where('id', 'in', pageIds)
           .where('deletedAt', 'is', null)
@@ -374,7 +380,14 @@ export class PageRepo {
     }
   }
 
-  async restorePage(pageId: string, workspaceId: string): Promise<void> {
+  async restorePage(
+    pageId: string,
+    workspaceId: string,
+    // See removePage: stamp `lastUpdatedSource` for automated (git-sync) restores
+    // so the change-listener loop-guard skips the echo cycle. Omitted for
+    // ordinary user restores.
+    lastUpdatedSource?: string,
+  ): Promise<void> {
     // First, check if the page being restored has a deleted parent
     const pageToRestore = await this.db
       .selectFrom('pages')
@@ -425,7 +438,12 @@ export class PageRepo {
       // On restore, disarm the death timer: pulling a note out of trash means
       // "keep it". Otherwise a deadline now in the past would re-trash it on the
       // next cleanup sweep.
-      .set({ deletedById: null, deletedAt: null, temporaryExpiresAt: null })
+      .set({
+        deletedById: null,
+        deletedAt: null,
+        temporaryExpiresAt: null,
+        ...(lastUpdatedSource ? { lastUpdatedSource } : {}),
+      })
       .where('id', 'in', pageIds)
       .execute();
 

apps/server/src/integrations/git-sync/services/git-sync.orchestrator.spec.ts

@@ -303,7 +303,7 @@ describe('GitSyncOrchestrator', () => {
   });
 
   describe('delete cap (anti-data-loss)', () => {
-    it('neutralizes deletePage on the apply client when planned deletes exceed the cap', async () => {
+    it('suppresses deletePage on the apply client (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.
@@ -315,20 +315,22 @@ describe('GitSyncOrchestrator', () => {
       expect(res.ran).toBe(true);
       expect(runPushMock).toHaveBeenCalledTimes(2);
 
-      // The second runPush (real apply, dryRun:false) got a neutralized client.
+      // 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();
-      // deletePage is still a function (the engine may call it)...
+      // deletePage is still a function (the engine calls it)...
       expect(typeof applyClient.deletePage).toBe('function');
-      await applyClient.deletePage('p1');
-      // ...but it is a NO-OP: the underlying real deletePage was NOT invoked.
+      // ...but it THROWS, so the engine records a per-page failure and holds
+      // `last-pushed` (a resolving no-op would advance past the dropped deletes
+      // and never replay them — PR #119 review). The real deletePage is NOT hit.
+      await expect(applyClient.deletePage('p1')).rejects.toThrow(/suppress/i);
       expect(built.client.deletePage).not.toHaveBeenCalled();
       // Creates/updates pass through to the real client.
       expect(applyClient.createPage).toBe(built.client.createPage);
     });
 
-    it('fails safe: a throwing dry-run still suppresses deletes and does not throw', async () => {
+    it('fails safe: a throwing dry-run still suppresses deletes and does not throw the cycle', async () => {
       jest.spyOn(Logger.prototype, 'warn').mockImplementation(() => undefined);
       const built = build({ maxDeletes: 5 });
       runPushMock
@@ -340,7 +342,8 @@ describe('GitSyncOrchestrator', () => {
       expect(res.ran).toBe(true);
       const [applyDeps] = runPushMock.mock.calls[1];
       const applyClient = applyDeps.makeClient();
-      await applyClient.deletePage('p1');
+      // Suppressed via throw (same fail-safe path as the over-cap case).
+      await expect(applyClient.deletePage('p1')).rejects.toThrow(/suppress/i);
       expect(built.client.deletePage).not.toHaveBeenCalled();
     });
 

apps/server/src/integrations/git-sync/services/git-sync.orchestrator.ts

@@ -473,11 +473,26 @@ export class GitSyncOrchestrator implements OnModuleInit, OnModuleDestroy {
       );
     }
 
-    // When over the cap, neutralize deletes by wrapping the client's deletePage
-    // into a no-op (every other op is forwarded). The dry-run already committed
-    // the working tree to `main`, so the apply re-diffs and converges normally.
+    // 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, deletePage: async () => undefined }
+      ? {
+          ...client,
+          deletePage: async () => {
+            throw new Error(
+              'git-sync: delete suppressed this cycle ' +
+                '(over GIT_SYNC_MAX_DELETES_PER_CYCLE) — refs intentionally held ' +
+                'so the deletion is retried, not dropped',
+            );
+          },
+        }
       : client;
 
     const pushResult = await runPush(

apps/server/src/integrations/git-sync/services/gitmost-datasource.service.spec.ts

@@ -292,10 +292,13 @@ describe('GitmostDataSourceService', () => {
     it('uses the soft-delete path (removePage), not a force delete', async () => {
       const { service, mocks } = build();
       await service.bind(CTX).deletePage('p1');
+      // Passes git-sync provenance so the soft-delete stamps
+      // lastUpdatedSource='git-sync' (loop-guard, PR #119 review).
       expect(mocks.pageService.removePage).toHaveBeenCalledWith(
         'p1',
         'svc-user',
         'ws-1',
+        { actor: 'git-sync', aiChatId: null },
       );
       // No forceDelete on the service surface used here.
       expect((mocks.pageService as any).forceDelete).toBeUndefined();
@@ -358,7 +361,12 @@ describe('GitmostDataSourceService', () => {
     it('restores via the repo restore path scoped to the workspace', async () => {
       const { service, mocks } = build();
       await service.bind(CTX).restorePage('p1');
-      expect(mocks.pageRepo.restorePage).toHaveBeenCalledWith('p1', 'ws-1');
+      // Stamps lastUpdatedSource='git-sync' on restore (loop-guard, PR #119).
+      expect(mocks.pageRepo.restorePage).toHaveBeenCalledWith(
+        'p1',
+        'ws-1',
+        'git-sync',
+      );
     });
   });
 });

apps/server/src/integrations/git-sync/services/gitmost-datasource.service.ts

@@ -232,7 +232,12 @@ export class GitmostDataSourceService {
     ctx: GitSyncBindContext,
     pageId: string,
   ): Promise<unknown> {
-    await this.pageService.removePage(pageId, ctx.userId, ctx.workspaceId);
+    await this.pageService.removePage(
+      pageId,
+      ctx.userId,
+      ctx.workspaceId,
+      GIT_SYNC_PROVENANCE,
+    );
     return { id: pageId };
   }
 
@@ -368,7 +373,13 @@ export class GitmostDataSourceService {
     ctx: GitSyncBindContext,
     pageId: string,
   ): Promise<unknown> {
-    await this.pageRepo.restorePage(pageId, ctx.workspaceId);
+    // Stamp git-sync provenance so the change-listener loop-guard skips the
+    // PAGE_RESTORED echo (mirrors deletePage / create / update / move).
+    await this.pageRepo.restorePage(
+      pageId,
+      ctx.workspaceId,
+      GIT_SYNC_PROVENANCE.actor,
+    );
     return { id: pageId };
   }