fix(engine): allow symlinks that resolve inside the project root

.claude/agent-memory/archgate-developer/project_rules_engine_internals.md

@@ -16,3 +16,7 @@ metadata:
 - **Scanner walk drops any node `AstNodeSchema.safeParse` rejects — WITH its whole subtree — so an over-strict schema is a silent security false-negative, not a validation nicety. Now ADR-governed (ARCH-024 clause 7).** `parseNode()` returns null on schema failure and the walk skips it (`if (child) walk(child)`), so a banned global/eval/import nested inside a rejected node goes unscanned and `check` reports pass. `type` is always present and every typed child field recurses, so a `Literal`'s `value` is the ONLY leaf that can independently fail — keep it `z.unknown().optional()` (read only via `typeof … === "string"` guards). Fired twice: null `source` on `export`-without-`from` (#493), and exotic `value` — meriyah emits `{}` for a RegExpLiteral and a `bigint` for `123n` (#494, e.g. `/x/.constructor.constructor` = Function ctor off a regex receiver). Always fix by WIDENING the schema (the block already exists, it's just unreached), never by adding a new check. Zod gotcha: `z.unknown()` is NON-optional in this Zod (v4) — bare `z.unknown()` made every node lacking `value` fail; needs `.optional()`.
 - **`Bun.Transpiler` strips provably-dead code before the scanner sees it — this is correct, not a gap.** `const y = /x/ || import("node:child_process")` transpiles to `const y = /x/;` (a RegExp literal is always truthy, so the `||` right side is unreachable and eliminated). Unreachable payloads genuinely never run, so not scanning them is right. When writing an escape regression test, put the payload in a REACHABLE position (`[/x/, import(...)]`, comma operator, a separate statement) and confirm `transformSync` keeps it — else the test passes for the wrong reason.
 - **Spawning a language interpreter to parse an untrusted project file is an RCE surface — the specifics are now ADR-governed (ARCH-022), noted here for any FUTURE interpreter-subprocess feature.** (1) `python -c` puts the target project cwd on `sys.path`, so a planted `ast.py`/`json.py` executes when the serializer imports stdlib — always pass `-I` (isolated). Verified: without `-I`, `import ast` resolves to a cwd shadow. (2) Python's plain `utf-8` codec keeps the BOM as U+FEFF and `ast.parse` then errors — read with `utf-8-sig` (`r:bom|utf-8` for Ruby). (3) `meriyah` `loc` after `Bun.Transpiler` is transpiled-relative, NOT original-source lines (type-only/comment/blank lines dropped) — re-locate via `readFile`+`indexOf` for the TS path; JS is parsed untranspiled so its `loc` is accurate. (4) Windows: probe the `py` launcher too (python.org registers it even when PATH opt-in is unchecked). The single sanctioned meriyah call site is `src/engine/js-parser.ts`; interpreter probe/spawn is `src/engine/ast-support.ts`.
+- **Fire-test a guard in BOTH directions: that it blocks the bad case AND still permits the legitimate one.** Proving a guard is load-bearing (stub it out → test fails) only shows the gate closes; it says nothing about over-rejection. 2026-07-25: the ARCH-024 ancestor-symlink guard (#499) passed a green suite while rejecting _every_ symlinked component below the project root, including links pointing back INSIDE the project — which breaks workspace monorepos (pnpm/npm/yarn/bun symlink `node_modules/<pkg>` → `packages/<pkg>`) and symlinked shared source dirs. It reached main before the false positive was caught; fixed in #500 by gating on the link's resolved TARGET rather than its existence. For any boundary check, write the allow-case test alongside the deny-case one.
+- **`realpath` comparisons must be realpath-against-realpath, never realpath-against-lexical.** `realpathSync` case-canonicalizes on Windows and macOS, so comparing its output against a lexically-built path rejects case-mismatched-but-legitimate paths. Resolve both sides — see `realRootOf` in `src/engine/safe-path.ts`. This is why the first version of the symlink walk deliberately used boolean `lstat` instead.
+- **When testing an "empty X" case, test the degenerate form, not a near-empty variant.** A test named "returns {} for an empty frontmatter block" used `---\n\n---` — a blank line the body group can capture — and passed, hiding that the truly empty `---\n---` did not match at all. The near-miss variant exercises a different path than the one the test name claims.
+- **Never chain cleanup of a temp script with `&&`, and never `git add -A` after using one.** `python fix.py && rm -f fix.py` leaves the script behind whenever it exits non-zero — exactly when a failed edit makes cleanup most necessary — and a later `git add -A` commits it. A throwaway `fix-sec-test.py` reached PR #500 this way (2026-07-25) and the maintainer caught it in review; nothing in `validate` flags a stray root-level `.py` (not linted, not in `src/`, invisible to knip). Use `; rm -f` or a `trap`, prefer `git add <explicit paths>`, and check `git diff --name-only origin/main...HEAD | grep -v /` before pushing.

src/engine/safe-path.ts

@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: Apache-2.0
 // Copyright 2026 Archgate
-import { lstatSync } from "node:fs";
+import { lstatSync, realpathSync } from "node:fs";
 import { resolve, isAbsolute, join, relative } from "node:path";
 
 import { UserError } from "../helpers/user-error";
@@ -31,23 +31,45 @@ export function isWithinRoot(resolvedRoot: string, absPath: string): boolean {
 }
 
 /**
- * Directories verified as real (non-symlink), memoized per process: safePath
- * runs once per glob result and siblings share every ancestor, so without this
- * the walk below re-lstats the same directories thousands of times per run.
+ * Real (non-symlink) components already verified, memoized per process:
+ * safePath runs once per glob result and siblings share every ancestor.
+ * Symlinks are never stored — they are re-resolved on every access. Nested per
+ * root, since a link leaving root A may land inside an enclosing root B.
  */
-const verifiedRealDirs = new Set<string>();
+const verifiedComponents = new Map<string, Set<string>>();
+
+/** Project roots resolved through `realpath`, memoized per process. */
+const realRoots = new Map<string, string>();
+
+/**
+ * The project root with symlinks in its own path resolved, so a link target
+ * compares against it like-for-like. Falls back to the lexical root.
+ */
+function realRootOf(resolvedRoot: string): string {
+  let hit = realRoots.get(resolvedRoot);
+  if (hit === undefined) {
+    try {
+      hit = realpathSync(resolvedRoot);
+    } catch {
+      hit = resolvedRoot;
+    }
+    realRoots.set(resolvedRoot, hit);
+  }
+  return hit;
+}
 
 /**
- * Reject a symlink anywhere in the path below the project root — the leaf OR
- * any ancestor, since a linked ancestor makes the leaf look like an ordinary
- * file to both `isWithinRoot` and a leaf `lstat`. Components at or above the
- * root are deliberately not inspected, and each test is a boolean `lstat`
- * rather than a `realpath` comparison.
+ * Reject a path whose real target lies outside the project root, testing every
+ * component below it — the leaf AND every ancestor, since a linked ancestor
+ * makes the leaf look ordinary to both `isWithinRoot` and a leaf `lstat`. The
+ * gate is where a link POINTS, not that one exists: links resolving back
+ * inside the project are a normal layout (workspace monorepos, shared dirs).
  *
- * @throws {UserError} When any component below the root is a symbolic link.
- * @see ARCH-022 — why the walk stops at the root and avoids `realpath`
+ * @throws {UserError} When a component resolves outside the project root.
+ * @see ARCH-022 — why the walk stops at the root
+ * @see .claude/agent-memory/archgate-developer/project_rules_engine_internals.md — why both sides are realpath'd
  */
-function assertNoSymlinkInPath(
+function assertNoEscapingSymlink(
   resolvedRoot: string,
   absPath: string,
   userPath: string
@@ -57,10 +79,16 @@ function assertNoSymlinkInPath(
   if (!rel || rel.startsWith("..")) return;
 
   const segments = rel.split(/[/\\]/u);
+  let verified = verifiedComponents.get(resolvedRoot);
+  if (!verified) {
+    verified = new Set();
+    verifiedComponents.set(resolvedRoot, verified);
+  }
+
   let current = resolvedRoot;
-  for (const [index, segment] of segments.entries()) {
+  for (const segment of segments) {
     current = join(current, segment);
-    if (verifiedRealDirs.has(current)) continue;
+    if (verified.has(current)) continue;
     let stat;
     try {
       stat = lstatSync(current);
@@ -70,20 +98,31 @@ function assertNoSymlinkInPath(
       return;
     }
     if (stat.isSymbolicLink()) {
-      throw new UserError(
-        index === segments.length - 1
-          ? `Path "${userPath}" is a symbolic link — access denied`
-          : `Path "${userPath}" traverses symbolic link "${segment}" — access denied`
-      );
+      let target: string;
+      try {
+        target = realpathSync(current);
+      } catch {
+        return; // Broken link — resolves to nothing, so it reaches nothing.
+      }
+      if (!isWithinRoot(realRootOf(resolvedRoot), target)) {
+        throw new UserError(
+          `Path "${userPath}" resolves outside the project through symbolic link "${segment}" — access denied`
+        );
+      }
+      // Deliberately NOT memoized: a link is re-resolved on every access, so a
+      // target repointed later in the process cannot ride an earlier pass.
+      continue;
     }
-    if (stat.isDirectory()) verifiedRealDirs.add(current);
+    verified.add(current);
   }
 }
 
 /**
- * Resolve a user-supplied path and ensure it stays within projectRoot.
- * Throws if the resolved path escapes the project boundary, is a symlink, or
- * reaches its target through a symlinked ancestor directory.
+ * Resolve a user-supplied path and ensure it stays within projectRoot, either
+ * lexically or after resolving symlinks at any component. Links resolving back
+ * inside the project are allowed.
+ *
+ * @throws {UserError} When the path escapes the project root.
  */
 export function safePath(resolvedRoot: string, userPath: string): string {
   const absPath = resolveUserPath(resolvedRoot, userPath);
@@ -92,6 +131,6 @@ export function safePath(resolvedRoot: string, userPath: string): string {
       `Path "${userPath}" escapes project root — access denied`
     );
   }
-  assertNoSymlinkInPath(resolvedRoot, absPath, userPath);
+  assertNoEscapingSymlink(resolvedRoot, absPath, userPath);
   return absPath;
 }

tests/commands/check-security.test.ts

@@ -14,21 +14,60 @@ import { join } from "node:path";
 import { loadRuleAdrs } from "../../src/engine/loader";
 import { runChecks } from "../../src/engine/runner";
 
+/**
+ * Whether this platform/account can create a link of the given kind. Windows
+ * allows directory junctions unprivileged but needs elevation for file
+ * symlinks, so the two are probed separately.
+ */
+function canLink(kind: "dir" | "file"): boolean {
+  const probe = mkdtempSync(join(tmpdir(), "archgate-linkprobe-"));
+  try {
+    if (kind === "dir") {
+      symlinkSync(probe, join(probe, "link"), "junction");
+    } else {
+      writeFileSync(join(probe, "f.txt"), "x");
+      symlinkSync(join(probe, "f.txt"), join(probe, "link.txt"));
+    }
+    return true;
+  } catch {
+    return false;
+  } finally {
+    rmSync(probe, { recursive: true, force: true });
+  }
+}
+
+const DIR_LINKS = canLink("dir");
+const FILE_LINKS = canLink("file");
+
 describe("check command security", () => {
   let tempDir: string;
   let adrsDir: string;
+  let outsideDirs: string[];
 
   beforeEach(() => {
     tempDir = mkdtempSync(join(tmpdir(), "archgate-check-sec-"));
     adrsDir = join(tempDir, ".archgate", "adrs");
     mkdirSync(adrsDir, { recursive: true });
     mkdirSync(join(tempDir, "src"), { recursive: true });
+    outsideDirs = [];
   });
 
+  // Cleanup runs here, not after each assertion: a failing expect() throws
+  // immediately, so a trailing rmSync would be skipped and leak the dir.
   afterEach(() => {
     rmSync(tempDir, { recursive: true, force: true });
+    for (const dir of outsideDirs) {
+      rmSync(dir, { recursive: true, force: true });
+    }
   });
 
+  /** A temp directory outside the project root, removed in afterEach. */
+  function makeOutsideDir(): string {
+    const dir = mkdtempSync(join(tmpdir(), "archgate-outside-"));
+    outsideDirs.push(dir);
+    return dir;
+  }
+
   const adrTemplate = (id: string) =>
     `---\nid: ${id}\ntitle: Security Test\ndomain: general\nrules: true\n---\n`;
 
@@ -146,25 +185,19 @@ describe("check command security", () => {
     expect(result.results[0].error).toContain("access denied");
   });
 
-  test("blocks symlink to file outside project", async () => {
-    const outsideDir = mkdtempSync(join(tmpdir(), "archgate-outside-"));
-    writeFileSync(join(outsideDir, "secret.txt"), "sensitive data");
-
-    // Create a symlink inside the project pointing outside
-    try {
+  test.skipIf(!FILE_LINKS)(
+    "blocks symlink to file outside project",
+    async () => {
+      const outsideDir = makeOutsideDir();
+      writeFileSync(join(outsideDir, "secret.txt"), "sensitive data");
       symlinkSync(
         join(outsideDir, "secret.txt"),
         join(tempDir, "src", "linked.txt")
       );
-    } catch {
-      // Symlink creation may fail on Windows without admin privileges — skip
-      rmSync(outsideDir, { recursive: true, force: true });
-      return;
-    }
 
-    writeAdrAndRule(
-      "SEC-006",
-      `export default {
+      writeAdrAndRule(
+        "SEC-006",
+        `export default {
   rules: {
     "read-symlink": {
       description: "Attempt to read symlinked file",
@@ -175,36 +208,29 @@ describe("check command security", () => {
   },
 };
 `
-    );
-
-    const loaded = await loadRuleAdrs(tempDir);
-    const result = await runChecks(tempDir, loaded);
-    expect(result.results[0].error).toContain("symbolic link");
-
-    rmSync(outsideDir, { recursive: true, force: true });
-  });
+      );
 
-  test("blocks reads that tunnel out through a symlinked ancestor directory", async () => {
-    // The leaf here is an ordinary file — only an ANCESTOR is the symlink, so
-    // a leaf-only lstat reports "not a link" while the OS still resolves the
-    // real path outside the project and reads it.
-    const outsideDir = mkdtempSync(join(tmpdir(), "archgate-outside-"));
-    writeFileSync(join(outsideDir, "secret.txt"), "sensitive data");
-
-    try {
-      // "junction" is ignored on POSIX (plain symlink) but on Windows creates
-      // a directory junction, which needs no admin privileges and which
-      // lstat() reports as a symbolic link — so unlike the file-symlink test
-      // above, this case runs for real on every platform instead of skipping.
-      symlinkSync(outsideDir, join(tempDir, "src", "linkdir"), "junction");
-    } catch {
-      rmSync(outsideDir, { recursive: true, force: true });
-      return;
+      const loaded = await loadRuleAdrs(tempDir);
+      const result = await runChecks(tempDir, loaded);
+      expect(result.results[0].error).toContain("symbolic link");
     }
+  );
+
+  test.skipIf(!DIR_LINKS)(
+    "blocks reads that tunnel out through a symlinked ancestor directory",
+    async () => {
+      // The leaf here is an ordinary file — only an ANCESTOR is the symlink, so
+      // a leaf-only lstat reports "not a link" while the OS still resolves the
+      // real path outside the project and reads it.
+      const outsideDir = makeOutsideDir();
+      writeFileSync(join(outsideDir, "secret.txt"), "sensitive data");
+      // "junction" is ignored on POSIX; on Windows it needs no elevation and
+      // lstat reports it as a symlink, so this runs on every platform.
+      symlinkSync(outsideDir, join(tempDir, "src", "linkdir"), "junction");
 
-    writeAdrAndRule(
-      "SEC-007",
-      `export default {
+      writeAdrAndRule(
+        "SEC-007",
+        `export default {
   rules: {
     "read-through-symlinked-dir": {
       description: "Attempt to read a real file via a symlinked parent",
@@ -215,15 +241,14 @@ describe("check command security", () => {
   },
 };
 `
-    );
-
-    const loaded = await loadRuleAdrs(tempDir);
-    const result = await runChecks(tempDir, loaded);
-    expect(result.results[0].error).toContain("access denied");
-    expect(result.results[0].error).toContain("symbolic link");
+      );
 
-    rmSync(outsideDir, { recursive: true, force: true });
-  });
+      const loaded = await loadRuleAdrs(tempDir);
+      const result = await runChecks(tempDir, loaded);
+      expect(result.results[0].error).toContain("access denied");
+      expect(result.results[0].error).toContain("symbolic link");
+    }
+  );
 
   test("allows reads under real nested directories (no false positives)", async () => {
     // Guards the ancestor walk against over-rejecting: a deep, entirely real