fix(sync): separator-agnostic path containment — unblock sync on Windows (#3057)

isPathSafe's inline `startsWith(rootReal + '/')` was always false on
Windows (realpathSync returns backslash separators), so every in-repo
file was recorded SYMLINK_NOT_ALLOWED and the sync bookmark froze.
Same separator class as #2828/#2836.

Root cause is the shared idiom: isPathContained in path-confine.ts
carried the same hardcoded '/'. Fixed there via a pure, path-module-
injectable isResolvedContained (relative-path idiom, matching
isWriteTargetContained); isPathSafe and the NAV-1/NAV-2 scope-entry
guard in sync.ts now delegate to it. win32 semantics are pinned on
POSIX CI by injecting path.win32 in the regression test.

Fixes #3057

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

src/commands/sync.ts

@@ -1,6 +1,7 @@
 import { existsSync, readFileSync, writeFileSync, statSync, realpathSync } from 'fs';
 import { execFileSync } from 'child_process';
 import { join, relative } from 'path';
+import { isPathContained, isResolvedContained } from '../core/path-confine.ts';
 import type { BrainEngine } from '../core/engine.ts';
 import { DELETE_BATCH_SIZE } from '../core/engine-constants.ts';
 import { importFile } from '../core/import-file.ts';
@@ -1112,15 +1113,14 @@ function createSyncBaselineCommit(repoPath: string): void {
  * #774 NAV-1 TOCTOU: true only if filePath realpath-resolves inside gitRoot.
  * Guards symlink escape at the per-file level (a committed symlink whose
  * target lives outside the repo), not just at scope entry.
+ *
+ * #3057: delegates to the shared separator-agnostic helper — the previous
+ * inline `startsWith(rootReal + '/')` was always false on Windows
+ * (realpathSync returns backslashes), recording every in-repo file as
+ * SYMLINK_NOT_ALLOWED and freezing the sync bookmark.
  */
 function isPathSafe(filePath: string, gitRoot: string): boolean {
-  try {
-    const real = realpathSync(filePath);
-    const rootReal = realpathSync(gitRoot);
-    return real === rootReal || real.startsWith(rootReal + '/');
-  } catch {
-    return false;
-  }
+  return isPathContained(filePath, gitRoot);
 }
 
 function hasOriginRemote(repoPath: string): boolean {
@@ -1887,7 +1887,8 @@ async function performSyncInner(engine: BrainEngine, opts: SyncOpts): Promise<Sy
   // NAV-1/NAV-2 scope-entry guard: the realpath-resolved scope must live
   // inside the realpath-resolved git root. Catches `--src-subpath ../escape`
   // AND a symlinked subdir pointing outside the repo, before any git op runs.
-  if (syncScopeRoot !== gitContextRoot && !syncScopeRoot.startsWith(gitContextRoot + '/')) {
+  // #3057: separator-agnostic (both sides already realpath'd above).
+  if (!isResolvedContained(syncScopeRoot, gitContextRoot)) {
     throw new Error(
       `Sync scope ${syncScopeRoot} resolves outside git repo ${gitContextRoot}. ` +
       `Refusing to sync: possible path traversal via --src-subpath.`,

src/core/path-confine.ts

@@ -20,11 +20,34 @@
  */
 
 import { realpathSync, existsSync, type Stats } from 'fs';
+import * as nodePath from 'path';
 import { resolve as resolvePath, relative, isAbsolute, dirname, basename, join } from 'path';
 
+/**
+ * Pure containment predicate over ALREADY-resolved paths: true iff `child`
+ * IS `parent` or lives under it. Separator-agnostic via `path.relative`.
+ *
+ * #3057: the previous `startsWith(parent + '/')` form is false for EVERY
+ * path on Windows — `realpathSync` returns backslash separators there — so
+ * every containment check built on it failed closed and blocked sync
+ * entirely (same separator class as #2828/#2836). `pathMod` is injectable
+ * so POSIX CI can pin the win32 semantics with `path.win32`.
+ */
+export function isResolvedContained(
+  child: string,
+  parent: string,
+  pathMod: Pick<typeof nodePath, 'relative' | 'isAbsolute' | 'sep'> = nodePath,
+): boolean {
+  const rel = pathMod.relative(parent, child);
+  // '' = same path; '..' or '../…' = escapes upward; absolute = different
+  // root entirely (e.g. another drive on Windows, where relative() returns
+  // the child verbatim).
+  return rel === '' || (rel !== '..' && !rel.startsWith('..' + pathMod.sep) && !pathMod.isAbsolute(rel));
+}
+
 /**
  * Symlink-safe path confinement: realpath BOTH sides, then a separator-aware
- * prefix check. A plain `startsWith()` on un-resolved paths would let a
+ * containment check. A plain `startsWith()` on un-resolved paths would let a
  * `parent/skills` symlink → `/etc` (or `$GBRAIN_HOME/clones/<id>` → `/etc`)
  * bypass the boundary; resolving first defeats that.
  *
@@ -41,9 +64,7 @@ export function isPathContained(child: string, parent: string): boolean {
   } catch {
     return false; // missing / unresolvable path → not contained
   }
-  // Append a separator so /foo doesn't match /foobar.
-  const parentWithSep = resolvedParent.endsWith('/') ? resolvedParent : resolvedParent + '/';
-  return resolvedChild === resolvedParent || resolvedChild.startsWith(parentWithSep);
+  return isResolvedContained(resolvedChild, resolvedParent);
 }
 
 /**

test/path-confine.test.ts

@@ -15,10 +15,10 @@ import {
   mkdtempSync, mkdirSync, writeFileSync, rmSync, symlinkSync, chmodSync,
   lstatSync, type Stats,
 } from 'fs';
-import { join } from 'path';
+import { join, win32, posix } from 'path';
 import { tmpdir } from 'os';
 import {
-  isTrustedDotfile, isPathContained, realpathOrResolve, isWriteTargetContained,
+  isTrustedDotfile, isPathContained, isResolvedContained, realpathOrResolve, isWriteTargetContained,
 } from '../src/core/path-confine.ts';
 import { validateSlug } from '../src/core/utils.ts';
 import { resolveSourceId } from '../src/core/source-resolver.ts';
@@ -149,6 +149,37 @@ describe('isPathContained', () => {
   });
 });
 
+// #3057: the containment predicate must be separator-agnostic. On Windows,
+// realpathSync returns backslash separators, and the old
+// `startsWith(parent + '/')` form was false for EVERY in-repo file — sync's
+// isPathSafe recorded every file SYMLINK_NOT_ALLOWED and froze the bookmark.
+// win32 semantics are pinned here via the injectable path module so this
+// regression is caught on POSIX CI.
+describe('isResolvedContained — win32 separators (#3057)', () => {
+  test('in-repo file with backslash separators IS contained', () => {
+    expect(isResolvedContained('C:\\repo\\notes\\a.md', 'C:\\repo', win32)).toBe(true);
+  });
+  test('root itself is contained', () => {
+    expect(isResolvedContained('C:\\repo', 'C:\\repo', win32)).toBe(true);
+  });
+  test('upward escape is NOT contained', () => {
+    expect(isResolvedContained('C:\\other\\x.md', 'C:\\repo', win32)).toBe(false);
+    expect(isResolvedContained('C:\\', 'C:\\repo', win32)).toBe(false);
+  });
+  test('sibling prefix does not match (C:\\repo vs C:\\repofoo)', () => {
+    expect(isResolvedContained('C:\\repofoo\\x.md', 'C:\\repo', win32)).toBe(false);
+  });
+  test('different drive is NOT contained', () => {
+    expect(isResolvedContained('D:\\repo\\x.md', 'C:\\repo', win32)).toBe(false);
+  });
+  test('posix semantics unchanged', () => {
+    expect(isResolvedContained('/repo/notes/a.md', '/repo', posix)).toBe(true);
+    expect(isResolvedContained('/repofoo/x.md', '/repo', posix)).toBe(false);
+    expect(isResolvedContained('/repo', '/repo', posix)).toBe(true);
+    expect(isResolvedContained('/', '/repo', posix)).toBe(false);
+  });
+});
+
 describe('realpathOrResolve', () => {
   test('resolves a symlink to its real target', () => {
     const dir = scratch();