From 2bd0f64c84f955836d1222d1a9e028703f27e31b Mon Sep 17 00:00:00 2001 From: claude code agent 227 Date: Fri, 26 Jun 2026 03:59:12 +0300 Subject: [PATCH] feat(git-sync): remove the per-cycle delete cap; deletes apply + are logged every cycle MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The delete cap (GIT_SYNC_MAX_DELETES_PER_CYCLE, default 5) was a defense-in-depth guard that SUPPRESSED a cycle's deletions when the planned count exceeded the limit. In practice it was a crutch over engine correctness that also blocked legitimate deletes: deleting a folder with many child pages is a normal action, and git-sync deletes are SOFT (Trash, reversible), so a blocking limit has little upside and real downside. There is also no user-facing surface to "confirm" a large delete from a background sync — the only channel is the operator log. So: drop the cap entirely. Deletes apply unconditionally; every cycle already logs its full push plan, per-action `delete: ` lines, and completion counts through the engine `log`, so what was deleted (and what was skipped) is always recorded. Engine correctness (the reconcile/layout/round-trip tests) is what prevents phantom deletions — not a blocking cap. Removed: orchestrator `resolveApplyClient` cap hook + `maxDeletes`, `getGitSyncMaxDeletesPerCycle`, the `GIT_SYNC_MAX_DELETES_PER_CYCLE` env/validation/.env.example, and the cap tests. (The engine's generic optional `resolveApplyClient` hook is left as an unused extension point.) server tsc clean, git-sync + environment jest 174. Co-Authored-By: Claude Opus 4.8 (1M context) --- .env.example | 4 -- .../environment/environment.service.spec.ts | 21 ------ .../environment/environment.service.ts | 14 ---- .../environment/environment.validation.ts | 6 -- .../services/git-sync.orchestrator.spec.ts | 66 +------------------ .../services/git-sync.orchestrator.ts | 39 +++-------- 6 files changed, 9 insertions(+), 141 deletions(-) diff --git a/.env.example b/.env.example index 93791c81..fe096983 100644 --- a/.env.example +++ b/.env.example @@ -243,7 +243,3 @@ MCP_DOCMOST_PASSWORD= # after this deadline so it cannot hold the per-space lock forever. # GIT_SYNC_BACKEND_TIMEOUT_MS=120000 # -# Defense-in-depth absolute cap on soft-deletes applied per push cycle -# (default: 5). A non-convergent / phantom-absence cycle can never trash more -# than this many pages without an explicit override. -# GIT_SYNC_MAX_DELETES_PER_CYCLE=5 diff --git a/apps/server/src/integrations/environment/environment.service.spec.ts b/apps/server/src/integrations/environment/environment.service.spec.ts index 7d70ffd2..96fede4c 100644 --- a/apps/server/src/integrations/environment/environment.service.spec.ts +++ b/apps/server/src/integrations/environment/environment.service.spec.ts @@ -15,27 +15,6 @@ describe('EnvironmentService', () => { expect(service).toBeDefined(); }); - describe('getGitSyncMaxDeletesPerCycle', () => { - const withEnv = (value?: string) => - new EnvironmentService({ - get: (_key: string, fallback?: string) => value ?? fallback, - } as any); - - it('defaults to 5 when unset', () => { - expect(withEnv().getGitSyncMaxDeletesPerCycle()).toBe(5); - }); - - it('parses a valid positive int', () => { - expect(withEnv('12').getGitSyncMaxDeletesPerCycle()).toBe(12); - }); - - it('falls back to 5 for non-positive or unparseable values', () => { - expect(withEnv('0').getGitSyncMaxDeletesPerCycle()).toBe(5); - expect(withEnv('-3').getGitSyncMaxDeletesPerCycle()).toBe(5); - expect(withEnv('not-a-number').getGitSyncMaxDeletesPerCycle()).toBe(5); - }); - }); - describe('getGitSyncPollIntervalMs', () => { const withEnv = (value?: string) => new EnvironmentService({ diff --git a/apps/server/src/integrations/environment/environment.service.ts b/apps/server/src/integrations/environment/environment.service.ts index e0511a17..99f78a6c 100644 --- a/apps/server/src/integrations/environment/environment.service.ts +++ b/apps/server/src/integrations/environment/environment.service.ts @@ -411,20 +411,6 @@ export class EnvironmentService { return Number.isFinite(parsed) && parsed > 0 ? parsed : 2000; } - /** - * Defense-in-depth absolute cap on how many pages a single push cycle may - * soft-delete (default 5). A non-convergent / phantom-absence cycle whose push - * plan would delete more than this is forced to skip deletions that cycle (the - * orchestrator logs a WARNING). A non-positive or unparseable value falls back - * to the default 5 so the cap can never be silently disabled. - */ - getGitSyncMaxDeletesPerCycle(): number { - const parsed = parseInt( - this.configService.get('GIT_SYNC_MAX_DELETES_PER_CYCLE', '5'), - 10, - ); - return Number.isFinite(parsed) && parsed > 0 ? parsed : 5; - } /** * The service user id git-sync writes are attributed to. Required when sync is diff --git a/apps/server/src/integrations/environment/environment.validation.ts b/apps/server/src/integrations/environment/environment.validation.ts index 62539fc8..db04cc60 100644 --- a/apps/server/src/integrations/environment/environment.validation.ts +++ b/apps/server/src/integrations/environment/environment.validation.ts @@ -209,12 +209,6 @@ export class EnvironmentVariables { @IsString() GIT_SYNC_BACKEND_TIMEOUT_MS: string; - // Defense-in-depth absolute cap on soft-deletes per push cycle (default 5): a - // non-convergent / phantom-absence cycle can never trash more than this many - // pages without an explicit override. Optional int (validated as a string env). - @IsOptional() - @IsString() - GIT_SYNC_MAX_DELETES_PER_CYCLE: string; // Required when git-sync is enabled: the service user create/move/rename/delete // are attributed to (issue #194 §7.2). Optional otherwise. 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 064686a8..9c2a446e 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,7 +1,7 @@ // Unit tests for the git-sync control plane. The 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`, +// (via SpaceLockService), // 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. @@ -48,7 +48,6 @@ interface BuildOptions { /** Env tunables (only the load-bearing ones are surfaced as overrides). */ enabled?: boolean; serviceUserId?: string | undefined; - maxDeletes?: number; remoteTemplate?: string | undefined; dataDir?: string; pollIntervalMs?: number; @@ -73,7 +72,6 @@ interface Built { function build(opts: BuildOptions = {}): Built { const { enabled = true, - maxDeletes = 100, remoteTemplate = undefined, dataDir = '/vaults', pollIntervalMs = 15000, @@ -87,7 +85,6 @@ function build(opts: BuildOptions = {}): Built { const env: Record = { isGitSyncEnabled: jest.fn(() => enabled), getGitSyncServiceUserId: jest.fn(() => serviceUserId), - getGitSyncMaxDeletesPerCycle: jest.fn(() => maxDeletes), getGitSyncRemoteTemplate: jest.fn(() => remoteTemplate), getGitSyncDataDir: jest.fn(() => dataDir), getGitSyncPollIntervalMs: jest.fn(() => pollIntervalMs), @@ -177,15 +174,6 @@ function primeEngineHappyPath(): void { 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(() => { jest.clearAllMocks(); primeEngineHappyPath(); @@ -295,7 +283,6 @@ describe('GitSyncOrchestrator', () => { 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', @@ -390,57 +377,6 @@ describe('GitSyncOrchestrator', () => { }); }); - describe('delete cap (anti-data-loss)', () => { - // 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 }); - await built.orchestrator.runOnce('space-1', 'ws-1'); - - 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 - // `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: an Infinity planned-delete count (dry-run failed) is suppressed', async () => { - jest.spyOn(Logger.prototype, 'warn').mockImplementation(() => undefined); - const built = build({ maxDeletes: 5 }); - await built.orchestrator.runOnce('space-1', 'ws-1'); - - // 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 }); - await built.orchestrator.runOnce('space-1', 'ws-1'); - - 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'); - expect(built.client.deletePage).toHaveBeenCalledWith('p1'); - }); - }); - describe('remote template substitution', () => { it('substitutes {spaceId} into the gitRemote settings handed to the engine', async () => { const built = build({ remoteTemplate: 'git@h:vault-{spaceId}.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 00f52a94..46ff688d 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 @@ -253,8 +253,8 @@ export class GitSyncOrchestrator implements OnModuleInit, OnModuleDestroy { * 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. + * service binding. There is no delete cap — deletes apply unconditionally (they + * are soft/reversible) and every cycle logs what it deleted via `log`. */ private async driveCycle( spaceId: string, @@ -266,7 +266,6 @@ export class GitSyncOrchestrator implements OnModuleInit, OnModuleDestroy { const settings = await this.buildSettings(spaceId); const vault = await this.vaultRegistry.getVault(spaceId); const client = this.dataSource.bind({ workspaceId, userId: serviceUserId }); - const maxDeletes = this.environmentService.getGitSyncMaxDeletesPerCycle(); const result = await runCycle({ // Cooperative-abort signal from the per-space lock: if a heartbeat refresh @@ -284,35 +283,13 @@ export class GitSyncOrchestrator implements OnModuleInit, OnModuleDestroy { mkdir: (absDir) => mkdir(absDir, { recursive: true }).then(() => undefined), rm: (absPath) => rm(absPath, { force: true }), }, + // Every cycle logs its full push plan + per-action lines + completion + // counts (created/updated/deleted/skipped/failures) through this `log`, so + // what was deleted (and what was not) is always recorded. There is no + // delete cap: deletes are soft (Trash, reversible), so a blocking limit + // only got in the way of legitimate deletes; engine correctness (covered by + // the reconcile/layout tests) is what prevents phantom deletions. log: (line: string) => this.logger.log(`git-sync[${spaceId}] ${line}`), - // 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}; suppressing deletions ` + - `this cycle (possible non-convergence / collision). Investigate vault ` + - `layout.`, - ); - return { - ...c, - 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', - ); - }, - }; - }, }); return { spaceId, ...result };