From f020739bfdb42a00d11a3c7088e3a15df7c5f6ab Mon Sep 17 00:00:00 2001 From: a Date: Sun, 28 Jun 2026 01:35:33 +0300 Subject: [PATCH] =?UTF-8?q?refactor(git-sync):=20address=20PR=20#119=20rev?= =?UTF-8?q?iew=20#3=20=E2=80=94=20honest=20gitRemote=20scaffolding=20comme?= =?UTF-8?q?nts,=20env=20example,=20shared=20ESM=20bridge?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. gitRemote is NOT yet consumed (the vendored engine has no remote-push path, SPEC §7). Corrected the buildSettings docstring (it wrongly called gitRemote "load-bearing") and marked the env -> validation -> getter -> buildSettings chain as inert SCAFFOLDING for the deferred remote-push feature at all three sites. Kept the wiring (harmless; removing only churns). 2. .env.example: document that GIT_SYNC_REMOTE_TEMPLATE substitutes the literal "{spaceId}" per-space (with the example), so an operator doesn't point every space at one remote. 3. Extracted the copy-pasted CJS->ESM dynamic-import bridge (`new Function('s','return import(s)')`) into one shared common/helpers/esm-import.ts; git-sync.loader, docmost-client.loader and mcp.service now import it and keep their own typed loadX() wrappers. Deferred (notes only, not implemented): - lcs.ts + three-way-merge.ts could move into packages/git-sync, but that engine is vendored (manual re-sync) — added a one-line note at three-way-merge.ts to revisit once the re-sync story is settled. - schema-core single source + BullMQ/fencing remain documented from prior rounds. Co-Authored-By: Claude Opus 4.8 (1M context) --- .env.example | 4 +++- apps/server/src/common/helpers/esm-import.ts | 18 ++++++++++++++++++ .../ai-chat/tools/docmost-client.loader.ts | 11 +++-------- .../environment/environment.service.ts | 8 +++++++- .../environment/environment.validation.ts | 3 +++ .../integrations/git-sync/git-sync.loader.ts | 14 ++++---------- .../git-sync/services/git-sync.orchestrator.ts | 14 ++++++++++++-- .../git-sync/services/three-way-merge.ts | 7 +++++++ .../server/src/integrations/mcp/mcp.service.ts | 12 ++++-------- 9 files changed, 61 insertions(+), 30 deletions(-) create mode 100644 apps/server/src/common/helpers/esm-import.ts diff --git a/.env.example b/.env.example index 1a51a937..fda10d71 100644 --- a/.env.example +++ b/.env.example @@ -224,7 +224,9 @@ MCP_DOCMOST_PASSWORD= # GIT_SYNC_DATA_DIR= # # Optional remote URL template to mirror each space's vault to (e.g. a git host). -# Leave unset to keep vaults local-only. +# The literal "{spaceId}" is substituted per-space, so each space mirrors to its +# OWN remote — e.g. git@host:vault-{spaceId}.git. Without the placeholder every +# space would point at one remote. Leave unset to keep vaults local-only. # GIT_SYNC_REMOTE_TEMPLATE= # # Poll-safety interval in ms — the cadence of the background reconcile cycle diff --git a/apps/server/src/common/helpers/esm-import.ts b/apps/server/src/common/helpers/esm-import.ts new file mode 100644 index 00000000..95f9ebee --- /dev/null +++ b/apps/server/src/common/helpers/esm-import.ts @@ -0,0 +1,18 @@ +/** + * Dynamic ESM import bridge for a CommonJS build. + * + * The server compiles with `module: commonjs`, and TypeScript downlevels a + * literal `import()` expression to `require()` — which cannot load an ESM-only + * package (`@docmost/mcp`, `@docmost/git-sync`). Indirecting through `new + * Function` hides the `import()` from the TS downleveler so the REAL dynamic + * `import()` survives to runtime and can load ESM from CommonJS. + * + * This is the single shared copy of that bridge. The per-package typed loaders + * (git-sync.loader.ts, docmost-client.loader.ts, mcp.service.ts) import this and + * keep their own typed `loadX()` wrappers (require.resolve + pathToFileURL + + * memoization) on top. + */ +export const esmImport = new Function( + 'specifier', + 'return import(specifier)', +) as (specifier: string) => Promise; diff --git a/apps/server/src/core/ai-chat/tools/docmost-client.loader.ts b/apps/server/src/core/ai-chat/tools/docmost-client.loader.ts index 5b740cfe..c3f65f56 100644 --- a/apps/server/src/core/ai-chat/tools/docmost-client.loader.ts +++ b/apps/server/src/core/ai-chat/tools/docmost-client.loader.ts @@ -1,4 +1,5 @@ import { pathToFileURL } from 'node:url'; +import { esmImport } from '../../../common/helpers/esm-import'; /** * Minimal structural type for the `DocmostClient` class we consume from the @@ -192,14 +193,8 @@ interface DocmostMcpModule { SHARED_TOOL_SPECS: Record; } -// TS with module:commonjs downlevels a literal `import()` to `require()`, which -// cannot load the ESM-only `@docmost/mcp` package. Indirect through Function so -// the real dynamic `import()` survives compilation and can load ESM from -// CommonJS at runtime (same trick as integrations/mcp/mcp.service.ts). -const esmImport = new Function( - 'specifier', - 'return import(specifier)', -) as (specifier: string) => Promise; +// The CJS->ESM dynamic-import bridge lives in one shared helper +// (common/helpers/esm-import.ts). The typed `loadDocmostMcp()` wrapper stays here. // Memoize the in-flight/loaded module so the dynamic import runs at most once. let modulePromise: Promise | null = null; diff --git a/apps/server/src/integrations/environment/environment.service.ts b/apps/server/src/integrations/environment/environment.service.ts index 9c0bf4d5..21980c96 100644 --- a/apps/server/src/integrations/environment/environment.service.ts +++ b/apps/server/src/integrations/environment/environment.service.ts @@ -367,7 +367,13 @@ export class EnvironmentService { return `${dataDir.replace(/\/+$/, '')}/git-sync`; } - /** Optional remote template, e.g. `git@host:vault-{spaceId}.git`. */ + /** + * Optional remote template, e.g. `git@host:vault-{spaceId}.git` (`{spaceId}` is + * substituted per-space in the orchestrator). SCAFFOLDING for the deferred + * remote-push feature: the vendored engine has no remote-push path yet (SPEC + * §7), so this value is currently inert — kept so the wiring is ready when the + * engine grows a push path. + */ getGitSyncRemoteTemplate(): string | undefined { return this.configService.get('GIT_SYNC_REMOTE_TEMPLATE'); } diff --git a/apps/server/src/integrations/environment/environment.validation.ts b/apps/server/src/integrations/environment/environment.validation.ts index d843b211..b9d04fde 100644 --- a/apps/server/src/integrations/environment/environment.validation.ts +++ b/apps/server/src/integrations/environment/environment.validation.ts @@ -190,6 +190,9 @@ export class EnvironmentVariables { @IsString() GIT_SYNC_DATA_DIR: string; + // SCAFFOLDING for the deferred remote-push feature: the vendored engine does + // not consume gitRemote yet (SPEC §7), so this is currently inert — validated + // here so the wiring is ready when remote push lands. @IsOptional() @IsString() GIT_SYNC_REMOTE_TEMPLATE: string; diff --git a/apps/server/src/integrations/git-sync/git-sync.loader.ts b/apps/server/src/integrations/git-sync/git-sync.loader.ts index ac608842..6cebbed0 100644 --- a/apps/server/src/integrations/git-sync/git-sync.loader.ts +++ b/apps/server/src/integrations/git-sync/git-sync.loader.ts @@ -1,4 +1,5 @@ import { pathToFileURL } from 'node:url'; +import { esmImport } from '../../common/helpers/esm-import'; import type { VaultGit as VaultGitClass, vaultGitEnv as vaultGitEnvFn, @@ -21,16 +22,9 @@ interface GitSyncModule { markdownToProseMirror: typeof markdownToProseMirrorFn; } -// TS with module:commonjs downlevels a literal `import()` to `require()`, which -// cannot load the ESM-only `@docmost/git-sync` package. Indirect through -// Function so the real dynamic `import()` survives compilation and can load ESM -// from CommonJS at runtime (same trick as -// apps/server/src/core/ai-chat/tools/docmost-client.loader.ts and -// integrations/mcp/mcp.service.ts). -const esmImport = new Function( - 'specifier', - 'return import(specifier)', -) as (specifier: string) => Promise; +// The CJS->ESM dynamic-import bridge lives in one shared helper +// (common/helpers/esm-import.ts); see it for why `import()` must be hidden from +// the TS commonjs downleveler. The typed `loadGitSync()` wrapper stays here. // Memoize the in-flight/loaded module so the dynamic import runs at most once. let modulePromise: Promise | null = null; 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 3b66024e..2974233b 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 @@ -111,10 +111,20 @@ export class GitSyncOrchestrator implements OnModuleInit, OnModuleDestroy { /** * Build the engine `Settings` for a space. The engine's REST-era fields * (docmostApiUrl/email/password) are unused on the native path — the - * datasource writes in-process — so they are placeholders; only `vaultPath`, - * `gitRemote`, and the tunables are load-bearing. + * datasource writes in-process — so they are placeholders; only `vaultPath` + * and the tunables are load-bearing today. + * + * `gitRemote` is NOT yet consumed: the vendored engine has no remote-push path + * (see engine/git.ts, engine/pull.ts, SPEC §7 — remote push is deferred), so + * the GIT_SYNC_REMOTE_TEMPLATE env -> validation -> getter -> this field chain + * is inert SCAFFOLDING kept in place for the future remote-push feature. It is + * harmless (the engine ignores it) and removing it would only churn; we still + * populate it so the wiring is ready when the engine grows a push path. */ private async buildSettings(spaceId: string): Promise { + // Scaffolding for the deferred remote-push feature — the engine does not read + // `gitRemote` yet (see the docstring above). Substitute {spaceId} per-space so + // the value is correct the moment the engine starts consuming it. const remoteTemplate = this.environmentService.getGitSyncRemoteTemplate(); const gitRemote = remoteTemplate ? remoteTemplate.replace(/\{spaceId\}/g, spaceId) diff --git a/apps/server/src/integrations/git-sync/services/three-way-merge.ts b/apps/server/src/integrations/git-sync/services/three-way-merge.ts index debf1bbe..ce83491f 100644 --- a/apps/server/src/integrations/git-sync/services/three-way-merge.ts +++ b/apps/server/src/integrations/git-sync/services/three-way-merge.ts @@ -17,6 +17,13 @@ * the human and/or git rewrote; resolve each region three-way. Stable anchor * blocks are emitted from LIVE so the applier keeps the existing Yjs block * instances (and the human's in-flight edits) in place. + * + * LOCATION (deferred): this and its `lcs.ts` sibling are pure, framework-free and + * could conceptually live in `packages/git-sync` (the engine). They are kept in + * the server integration on purpose: `packages/git-sync` is a VENDORED engine + * (pinned upstream, manually re-synced), so adding first-party files there + * complicates the re-sync story, and the only consumer today is the server. Move + * them into the engine only once the vendoring re-sync story is settled. */ import { buildLcsTable } from './lcs'; diff --git a/apps/server/src/integrations/mcp/mcp.service.ts b/apps/server/src/integrations/mcp/mcp.service.ts index 637f3e56..d7985fc6 100644 --- a/apps/server/src/integrations/mcp/mcp.service.ts +++ b/apps/server/src/integrations/mcp/mcp.service.ts @@ -6,6 +6,7 @@ import { } from '@nestjs/common'; import { ModuleRef } from '@nestjs/core'; import { pathToFileURL } from 'node:url'; +import { esmImport } from '../../common/helpers/esm-import'; import { IncomingMessage } from 'node:http'; import { FastifyReply, FastifyRequest } from 'fastify'; import { EnvironmentService } from '../environment/environment.service'; @@ -63,14 +64,9 @@ const MCP_RESOLVED = Symbol('mcpResolvedConfig'); // (never the token value) so operators can migrate without log spam. let warnedLegacyMcpAuth = false; -// TS with module:commonjs downlevels a literal import() to require(), which -// cannot load the ESM-only @docmost/mcp package. Indirect through Function so -// the real dynamic import() survives compilation and can load ESM from -// CommonJS at runtime. -const esmImport = new Function( - 'specifier', - 'return import(specifier)', -) as (specifier: string) => Promise; +// The CJS->ESM dynamic-import bridge lives in one shared helper +// (common/helpers/esm-import.ts); see it for why import() must be hidden from the +// TS commonjs downleveler. @Injectable() export class McpService implements OnModuleDestroy {