From 3a03a61060c4b1f71f8a89d95527f8df9a865bfe Mon Sep 17 00:00:00 2001 From: claude code agent 227 Date: Sun, 21 Jun 2026 15:37:09 +0300 Subject: [PATCH] fix(git-sync): branch choreography + strict scoping + delete cap (Phase B hardening) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes found by the live pull/push e2e: - CRITICAL: driveCycle never checked out the 'docmost' branch before applyPullActions, so Docmost content was written straight onto 'main', clobbering local file edits before push could diff them. Now checkout 'docmost' before pull (applyPullActions commits there then checks out main + merges) — mirrors the engine's pull main(). Round-trip now works both ways. - add an unresolved-merge guard (SPEC §9): skip the cycle if the vault is mid-merge instead of failing on checkout. - SAFETY: enabledSpaces() is now STRICT opt-in — only spaces with settings.gitSync.enabled===true; removed the all-spaces fallback that synced every space (incl. a 92-page one) the moment GIT_SYNC_ENABLED flipped. - SAFETY: per-cycle delete cap (GIT_SYNC_MAX_DELETES_PER_CYCLE, default 5): dry-run the push, and if planned deletes exceed the cap, run the apply with deletePage neutralized — phantom absence-deletions from a non-convergent vault can't soft-delete real pages. Fails safe if the dry-run throws. - fix manual trigger: TriggerGitSyncDto.spaceId needs @IsUUID or the global whitelist ValidationPipe strips it (arrived undefined -> vault 'undefined'). Live-verified on an isolated flagged space: push (vault file edit -> Docmost content, stamped lastUpdatedSource='git-sync') and pull (Docmost rename -> vault file + meta) both work; an unrelated 92-page space stayed untouched throughout. Co-Authored-By: Claude Opus 4.8 --- .../environment/environment.service.spec.ts | 21 ++++ .../environment/environment.service.ts | 14 +++ .../environment/environment.validation.ts | 7 ++ .../git-sync/git-sync.controller.ts | 5 + .../services/git-sync.orchestrator.ts | 117 +++++++++++++----- 5 files changed, 133 insertions(+), 31 deletions(-) diff --git a/apps/server/src/integrations/environment/environment.service.spec.ts b/apps/server/src/integrations/environment/environment.service.spec.ts index efef25b0..6168bdbe 100644 --- a/apps/server/src/integrations/environment/environment.service.spec.ts +++ b/apps/server/src/integrations/environment/environment.service.spec.ts @@ -14,4 +14,25 @@ describe('EnvironmentService', () => { it('should be defined', () => { 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); + }); + }); }); diff --git a/apps/server/src/integrations/environment/environment.service.ts b/apps/server/src/integrations/environment/environment.service.ts index 5cff4764..31297620 100644 --- a/apps/server/src/integrations/environment/environment.service.ts +++ b/apps/server/src/integrations/environment/environment.service.ts @@ -362,6 +362,20 @@ export class EnvironmentService { ); } + /** + * 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'), + ); + return Number.isFinite(parsed) && parsed > 0 ? parsed : 5; + } + /** * The service user id git-sync writes are attributed to. Required when sync is * enabled (validated in environment.validation.ts); optional otherwise. diff --git a/apps/server/src/integrations/environment/environment.validation.ts b/apps/server/src/integrations/environment/environment.validation.ts index 923efa66..66627124 100644 --- a/apps/server/src/integrations/environment/environment.validation.ts +++ b/apps/server/src/integrations/environment/environment.validation.ts @@ -195,6 +195,13 @@ export class EnvironmentVariables { @IsString() GIT_SYNC_DEBOUNCE_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 (plan §7.2). Optional otherwise. @ValidateIf((obj) => obj.GIT_SYNC_ENABLED === 'true') diff --git a/apps/server/src/integrations/git-sync/git-sync.controller.ts b/apps/server/src/integrations/git-sync/git-sync.controller.ts index 4c8a5267..f2c70b75 100644 --- a/apps/server/src/integrations/git-sync/git-sync.controller.ts +++ b/apps/server/src/integrations/git-sync/git-sync.controller.ts @@ -18,6 +18,7 @@ import { WorkspaceCaslSubject, } from '../../core/casl/interfaces/workspace-ability.type'; import { EnvironmentService } from '../environment/environment.service'; +import { IsUUID } from 'class-validator'; import { GitSyncOrchestrator, GitSyncRunStatus, @@ -25,6 +26,10 @@ import { /** Body for the manual one-shot trigger. */ class TriggerGitSyncDto { + // The global ValidationPipe runs with whitelist:true, which STRIPS any field + // lacking a validation decorator — without this @IsUUID the spaceId would be + // dropped and arrive as undefined. + @IsUUID() spaceId: string; } 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 2f34395f..95a82683 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 @@ -34,7 +34,12 @@ export interface GitSyncRunStatus { spaceId: string; ran: boolean; /** Why the cycle did not run (lock held elsewhere, busy, disabled, error). */ - skipped?: 'lock-held' | 'in-progress' | 'disabled' | 'no-service-user'; + skipped?: + | 'lock-held' + | 'in-progress' + | 'disabled' + | 'no-service-user' + | 'merge-in-progress'; pull?: { written: number; deleted: number; conflict: boolean }; push?: { mode: string; failures: number }; error?: string; @@ -46,12 +51,13 @@ export interface GitSyncRunStatus { * in-process per-space mutex (no overlapping cycles on one instance), it runs a * PULL (Docmost -> vault) then a PUSH (vault -> Docmost) for a space. * - * Enumeration of enabled spaces (plan §10 / Phase-C dependency): the per-space - * UI flag `space.settings.gitSync.enabled` is Phase C and not built yet, so this - * queries spaces whose jsonb flag is already set AND, when none are, treats - * GIT_SYNC_ENABLED as a master switch that enables ALL spaces (so the feature is - * exercisable before the UI lands). Once Phase C writes the flag, only flagged - * spaces sync. The whole loop is gated on GIT_SYNC_ENABLED first. + * Enumeration of enabled spaces (plan §10): STRICT opt-in. Only spaces whose + * per-space flag `space.settings.gitSync.enabled === true` (written by the Phase-C + * UI) are reconciled. There is intentionally NO all-spaces fallback: when no space + * carries the flag, git-sync does NOTHING (an empty list) — flagging every space + * the moment GIT_SYNC_ENABLED flips on is a safety hazard (it could mass-sync large + * spaces). The whole loop is still gated on the GIT_SYNC_ENABLED master switch + * first; per-space opt-in is now REQUIRED on top of it. */ @Injectable() export class GitSyncOrchestrator { @@ -112,27 +118,19 @@ export class GitSyncOrchestrator { // --- enabled-space enumeration (plan §10) -------------------------------- /** - * Enumerate the spaces the poll loop should reconcile. Prefers the Phase-C - * per-space flag (`settings->'gitSync'->>'enabled' = 'true'`); when NO space - * carries it yet (UI not built), falls back to enumerating ALL live spaces (the - * GIT_SYNC_ENABLED master switch already gates whether we get here at all). + * Enumerate the spaces the poll loop should reconcile. STRICT opt-in: ONLY + * spaces carrying the Phase-C per-space flag (`settings->'gitSync'->>'enabled' + * = 'true'`, written by the Phase-C UI) are returned. There is intentionally NO + * fallback to "all live spaces" — when no space is flagged this returns an empty + * list and git-sync does nothing (correct opt-in behavior). The GIT_SYNC_ENABLED + * master switch gates whether the loop runs at all; this flag gates which spaces. */ private async enabledSpaces(): Promise { - const flagged = await this.db - .selectFrom('spaces') - .select(['id as spaceId', 'workspaceId']) - .where('deletedAt', 'is', null) - .where(sql`settings->'gitSync'->>'enabled' = 'true'`) - .execute(); - - if (flagged.length > 0) return flagged; - - // No per-space flag set yet (Phase C UI pending): the master switch enables - // all spaces so the feature can be verified end-to-end before the UI lands. return this.db .selectFrom('spaces') .select(['id as spaceId', 'workspaceId']) .where('deletedAt', 'is', null) + .where(sql`settings->'gitSync'->>'enabled' = 'true'`) .execute(); } @@ -226,7 +224,24 @@ export class GitSyncOrchestrator { // 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 / plan §11.2): 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 (plan §11.1/§11.2) -------------------------------------------- // readExisting deps (ReadExistingDeps): list tracked *.md + read by relPath. @@ -261,16 +276,56 @@ export class GitSyncOrchestrator { // 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 (plan §11.3 step 6). 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; + 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.`, + ); + } + } 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, 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. + const applyClient = suppressDeletes + ? { ...client, deletePage: async () => undefined } + : client; + const pushResult = await runPush( - { - settings, - git: vault, - makeClient: () => client, - readFile: (relPath) => readFile(`${vaultRoot}/${relPath}`, 'utf8'), - writeFile: (relPath, text) => - writeFile(`${vaultRoot}/${relPath}`, text, 'utf8'), - log: (line) => this.logger.log(`git-sync[${spaceId}] ${line}`), - }, + { ...pushDeps, makeClient: () => applyClient }, { dryRun: false }, );