refactor(git-sync): address PR #119 review #3 — honest gitRemote scaffolding comments, env example, shared ESM bridge

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) <noreply@anthropic.com>

.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

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<unknown>;

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<string, SharedToolSpec>;
 }
 
-// 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<unknown>;
+// 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<DocmostMcpModule> | null = null;

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<string>('GIT_SYNC_REMOTE_TEMPLATE');
   }

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;

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<unknown>;
+// 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<GitSyncModule> | null = null;

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<Settings> {
+    // 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)

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';

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<unknown>;
+// 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 {