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

[rhuanbarreto]

Follow-up to #499, addressing the caveat you raised on it:

There's one caveat here to take care of: what if the symlink is on a project main worktree? We need to check what to do.

You were right, and it's a regression I introduced in #499 — it is on main now.

The problem

#499 rejected any symlinked path component below the project root, regardless of where it pointed. But a symlink resolving back inside the project escapes nothing. Rejecting those means archgate refuses to read files it is meant to govern, failing the rule with access denied.

Symlinked directories inside a repository are an ordinary layout, not an edge case:

• Workspace monorepos — pnpm, npm, yarn and bun all symlink node_modules/<pkg> → packages/<pkg>
• Shared source directories — e.g. apps/web/shared → packages/shared

Reproduced before fixing, with a junction so it runs on Windows too:

INSIDE-pointing symlink: REJECTED <<< FALSE POSITIVE
  Path "apps/web/shared/index.ts" traverses symbolic link "shared" — access denied
OUTSIDE-pointing symlink: REJECTED (correct)

The mistake was conflating "is a symlink" with "escapes the project". The boundary is the target, not the link.

The fix

Each component below the root is resolved, and rejected only when its target lands outside the root. The escape #499 closed stays closed:

Case	Before	After
Ancestor links outside	rejected	rejected
Leaf links outside	rejected	rejected
Ancestor links inside (monorepo)	rejected ❌	allowed ✅
Leaf links inside	rejected ❌	allowed ✅
Hops inside, then outside	rejected	rejected

Three details worth review attention:

• Comparison is realpath-against-realpath, never realpath-against-lexical. realpath case-canonicalizes on Windows and macOS, so mixing the two rejects case-mismatched-but-legitimate paths — the reason fix(engine): reject rule-file reads through a symlinked ancestor directory #499 used boolean lstat rather than a realpath comparison in the first place. Resolving both sides applies the same canonicalization to each, which is what makes the comparison safe here. The root's own realpath is memoized alongside the per-component memo.
• The component memo is keyed by (root, component), not component alone. The same component can sit under two roots with different verdicts — a link leaving root A may land inside an enclosing root B — so a component-only key could carry one root's verdict into another's.
• A hop that legitimately stays inside must not stop the walk. Allowing the first hop and returning early would reopen the escape one level deeper; there is a regression test for exactly that chain.

Deliberate widening

This also relaxes the pre-existing leaf-symlink check (which predates #499), since it rejected in-project links too. Leaving the leaf strict while allowing ancestors would be arbitrary: a file reachable as packages/shared/index.ts should not become unreadable because it is also linked as index.ts. Flagging it explicitly because it is a behavior change beyond repairing my own regression — say the word and I will keep the leaf strict, though I think that is the wrong shape.

Tests

tests/engine/safe-path.test.ts covers every row of the table above; tests/commands/check-security.test.ts still asserts the end-to-end escape is blocked through runChecks. Directory links use symlink type "junction" so the cases run for real on Windows rather than skipping.

bun run validate passes (lint, typecheck, format:check, full suite, archgate check 44/44, knip, build:check).

[coderabbitai]

Warning

Review limit reached

@rhuanbarreto, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 25 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 97e9b570-aea0-4320-b7bd-2a81eb927cc3

📥 Commits

Reviewing files that changed from the base of the PR and between 282e8bc and e7e1198.

📒 Files selected for processing (4)

• .claude/agent-memory/archgate-developer/project_rules_engine_internals.md
• src/engine/safe-path.ts
• tests/commands/check-security.test.ts
• tests/engine/safe-path.test.ts

📝 Walkthrough

Walkthrough

Safe path validation now resolves symbolic-link targets and rejects paths only when a target escapes the project root. It caches verified path components and canonical project roots, while broken links return without further validation. Tests now cover escaping and internal ancestor and leaf links, and update error-message assertions for chained symlink paths. Project guidance adds requirements for both allow and deny guard cases, realpath-to-realpath boundary comparisons, and exact empty-input test forms.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main change: allowing symlinks that resolve inside the project root.
Description check	✅ Passed	The description is detailed and directly describes the symlink fix, its rationale, and the test coverage.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[cloudflare-workers-and-pages]

Deploying archgate-cli with    Cloudflare Pages

Latest commit:	e7e1198
Status:	✅  Deploy successful!
Preview URL:	https://eaba690e.archgate-cli.pages.dev
Branch Preview URL:	https://fix-allow-in-project-symlink.archgate-cli.pages.dev

View logs

[github-actions]

Code Coverage

Metric	Value
Lines	91.3% (8186 / 8964)
Threshold	90% minimum — met
Platforms	Linux + Windows

Full HTML report available in workflow artifacts.

Per-directory breakdown

Directory	Coverage	Lines
src/commands/	89.0%	1959 / 2200
src/engine/	93.9%	2069 / 2203
src/formats/	98.7%	148 / 150
src/helpers/	90.9%	4010 / 4411

[rhuanbarreto]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

tests/engine/safe-path.test.ts (1)

82-99: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

outsideDir cleanup isn't guaranteed on assertion failure.

In each of these tests, outsideDir is created via mkdtempSync and only removed by an explicit trailing rmSync call after the expect(...).toThrow(...) assertion. If that assertion fails, the rmSync never runs and the temp directory leaks. As per path instructions, "clean them up in afterEach or afterAll" is intended to avoid exactly this.

🧹 Proposed fix (chain-hop test shown; same pattern applies to the other two)

     test("throws when a chain hops inside the project and then out", () => {
       const outsideDir = mkdtempSync(join(tmpdir(), "archgate-outside-"));
-      writeFileSync(join(outsideDir, "secret.txt"), "sensitive");
-      mkdirSync(join(tempDir, "real"), { recursive: true });
-      try {
-        symlinkSync(join(tempDir, "real"), join(tempDir, "hop"), "junction");
-        symlinkSync(outsideDir, join(tempDir, "real", "escape"), "junction");
-      } catch {
-        rmSync(outsideDir, { recursive: true, force: true });
-        return;
-      }
-      expect(() => safePath(tempDir, "hop/escape/secret.txt")).toThrow(
-        /resolves outside the project.*access denied/u
-      );
-      rmSync(outsideDir, { recursive: true, force: true });
+      try {
+        writeFileSync(join(outsideDir, "secret.txt"), "sensitive");
+        mkdirSync(join(tempDir, "real"), { recursive: true });
+        try {
+          symlinkSync(join(tempDir, "real"), join(tempDir, "hop"), "junction");
+          symlinkSync(outsideDir, join(tempDir, "real", "escape"), "junction");
+        } catch {
+          return;
+        }
+        expect(() => safePath(tempDir, "hop/escape/secret.txt")).toThrow(
+          /resolves outside the project.*access denied/u
+        );
+      } finally {
+        rmSync(outsideDir, { recursive: true, force: true });
+      }
     });

Also applies to: 123-137, 153-170

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/engine/safe-path.test.ts` around lines 82 - 99, Ensure every temporary
outsideDir created in the affected safePath tests is cleaned up even when
assertions fail by registering cleanup through the test suite’s afterEach or
afterAll mechanism. Update the tests around the ancestor-link case and the other
two referenced chain-hop cases, removing reliance on trailing rmSync calls while
preserving their existing assertions and setup behavior.

Source: Path instructions

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/engine/safe-path.ts`:
- Around line 100-114: Update assertNoEscapingSymlink so verifiedComponents
caches only non-symlink path components, while every symlink is rechecked with
lstat/realpath on each call. Preserve the existing root cache and escaping-link
rejection, but avoid adding passing symlink components to the process-scoped
verified set.

In `@tests/engine/safe-path.test.ts`:
- Around line 101-121: Replace the bare early-return handling around symlink
creation in the affected tests with explicit conditional skips using the test
framework’s skipIf/skip mechanism. Reuse a shared symlink-capability probe such
as symlinksSupported, and apply it to the tests near the ancestor-directory case
and the additional ranges noted, so unsupported environments report skipped
tests while supported environments retain their assertions.

---

Outside diff comments:
In `@tests/engine/safe-path.test.ts`:
- Around line 82-99: Ensure every temporary outsideDir created in the affected
safePath tests is cleaned up even when assertions fail by registering cleanup
through the test suite’s afterEach or afterAll mechanism. Update the tests
around the ancestor-link case and the other two referenced chain-hop cases,
removing reliance on trailing rmSync calls while preserving their existing
assertions and setup behavior.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: b3ee0f1b-dc7a-418f-a0e2-f6dd83f648d0

📥 Commits

Reviewing files that changed from the base of the PR and between 9a114b3 and 282e8bc.

📒 Files selected for processing (3)

• .claude/agent-memory/archgate-developer/project_rules_engine_internals.md
• src/engine/safe-path.ts
• tests/engine/safe-path.test.ts

📜 Review details

⏰ Context from checks skipped due to timeout. (1)

• GitHub Check: Cloudflare Pages

🧰 Additional context used

📓 Path-based instructions (10)

**/*.{ts,tsx,js,jsx,mjs,cjs}

📄 CodeRabbit inference engine (.archgate/adrs/ARCH-006-dependency-policy.md)

**/*.{ts,tsx,js,jsx,mjs,cjs}: Use Bun built-ins for file I/O (Bun.file, Bun.write), HTTP, subprocess execution (Bun.spawn), globbing (Bun.Glob), and testing (bun:test).
Do not use Node.js-specific APIs when Bun alternatives exist; for example, use Bun.file() instead of fs.readFile() for simple reads.
Prefer node: built-in modules such as node:util, node:path, and node:fs over npm alternatives.
Do not use utility libraries for single functions, such as importing lodash for one helper like pick.
Do not use path aliases (tsconfig paths); use relative imports with Bun's native module resolution.
Do not install packages globally during development; use bunx for one-off tools.

Files:

• tests/engine/safe-path.test.ts
• src/engine/safe-path.ts

tests/**/*.ts

📄 CodeRabbit inference engine (.archgate/adrs/ARCH-009-platform-detection-helper.md)

In test files, use _resetPlatformCache() to simulate different platforms instead of mocking or mutating process.platform directly.

tests/**/*.ts: Use Bun's built-in bun:test runner for all tests; do not import test utilities from node:test.
Shared test helpers must also use restoreEnv when restoring environment variables.

Files:

• tests/engine/safe-path.test.ts

{src,tests}/**/*.ts

📄 CodeRabbit inference engine (.archgate/adrs/LEGAL-001-spdx-license-headers.md)

{src,tests}/**/*.ts: Every TypeScript source file in src/ and tests/ must begin with // SPDX-License-Identifier: Apache-2.0 followed by // Copyright 2026 Archgate.
If a TypeScript file has a shebang line (for example #!/usr/bin/env bun in src/cli.ts), the SPDX license header must appear immediately after the shebang.
Use single-line // comments for the SPDX header; do not use block comments (/* */) or alternate license identifiers.

Files:

• tests/engine/safe-path.test.ts
• src/engine/safe-path.ts

tests/**/*.test.ts

📄 CodeRabbit inference engine (.archgate/adrs/ARCH-005-testing-standards.md)

tests/**/*.test.ts: Place tests in tests/ mirroring the src/ directory structure, and name test files with the .test.ts suffix.
Test public module interfaces rather than private implementation details, use descriptive test names, and do not depend on network access.
Use isolated temporary directories created with mkdtemp for filesystem tests, and clean them up in afterEach or afterAll. Do not use hardcoded user or system paths.
Close external SDK instances such as servers, clients, and transports in afterEach or afterAll; manage their lifecycle in test hooks rather than individual test bodies.
When a temporary git repository performs commits, configure local user.email and user.name immediately after git init; do not rely on global git identity.
Every runnable test() or it() must contain at least one expect() assertion. Make implicit no-throw contracts explicit with not.toThrow() or resolves.toBeUndefined(). Use test.skip or test.todo for intentional placeholders.
Use test.skipIf(condition), test.skip, or test.todo for conditional or intentionally disabled tests; do not use bare early returns or empty callbacks to skip tests, and do not skip without a tracking issue.
When adding assertions to an older test file, import expect from bun:test.
Mock HTTP requests by assigning directly to globalThis.fetch, and restore the original fetch implementation or mock in afterEach; do not mock node:fetch.
Mock first-party modules with import * as mod and spyOn(mod, "fn"), restoring spies after each test; do not use process-global mock.module() for first-party modules.
Wrap inline spyOn or mockImplementation usage in try/finally so mockRestore() always runs, or manage spies in beforeEach and afterEach.
When redirecting user-scope paths, mock node:os's homedir() rather than relying on runtime HOME overrides; use environment overrides only for code that reads environment variables directly at call time.
Re...

Files:

• tests/engine/safe-path.test.ts

**

⚙️ CodeRabbit configuration file

**: This project uses Archgate — an AI governance framework based on
Architecture Decision Records (ADRs). The ADRs in .archgate/adrs/
are the authoritative rules for this codebase. Each ADR has a companion
.rules.ts file with automated checks that run via archgate check.

When reviewing, you must:

1. Treat ADR violations as blocking issues, not suggestions.
2. Cite the specific ADR ID when flagging a violation (e.g., "Violates ARCH-006").
3. Focus on semantic and contextual violations that automated rules cannot catch —
   the .rules.ts files already cover syntactic/structural patterns.
4. If you are unsure whether something violates an ADR, flag it as a question
   rather than approving it.

Files:

• tests/engine/safe-path.test.ts
• src/engine/safe-path.ts

src/**/*.ts

📄 CodeRabbit inference engine (.archgate/adrs/ARCH-004-no-barrel-files.md)

src/**/*.ts: Do not re-export symbols from another module in any source file; statements like export { X } from "./other" and export type { X } from "./other" are forbidden.
Import symbols directly from the module that defines them; do not import from a directory path such as ../formats expecting implicit index.ts resolution.

src/**/*.ts: Use await Bun.file(path).json() when reading JSON files in Bun TypeScript source code; do not use JSON.parse(await Bun.file(path).text()) or JSON.parse(fs.readFileSync(path, "utf-8")) for file reads.
Use Bun.JSONC.parse() when reading files that may contain comments, such as tsconfig.json, instead of plain JSON.parse() on file contents.
Reserve JSON.parse() for parsing JSON strings from non-file sources such as API responses or string variables; do not use it as the default for reading JSON files.

src/**/*.ts: In TypeScript source files under src/, use Bun.env instead of process.env for all environment variable reads and writes; process.env must not be used.
In TypeScript source files under src/, use nullish coalescing for environment-variable defaults, e.g. Bun.env.NODE_ENV ?? "production".
In TypeScript source files under src/, use Boolean(Bun.env.CI) for truthy checks on environment flags.
In TypeScript source files under src/, do not destructure Bun.env (for example, const { HOME } = Bun.env); access properties individually instead.
In TypeScript source files under src/, do not reference process.env even in comments that suggest using it.

src/**/*.ts: Heavy dependencies such as inquirer, posthog-node, @sentry/*, and any similarly expensive runtime-only dependency MUST be loaded with dynamic import() at the point of use, never as a static value import at module top level.
Type-only references to heavy modules must use import type (for example, import type { PostHog } from "posthog-node" or import type * as SentryNs from "@sentry/node-core/light"); type-only...

Files:

• src/engine/safe-path.ts

src/**/!(*platform).ts

📄 CodeRabbit inference engine (.archgate/adrs/ARCH-009-platform-detection-helper.md)

src/**/!(*platform).ts: In src/ TypeScript source files, do not read process.platform directly; use src/helpers/platform.ts (isWindows(), isMacOS(), isLinux(), isWSL(), getPlatformInfo()) for all platform detection.
Use the centralized platform helper instead of duplicating OS/WSL detection logic inline anywhere in src/ TypeScript source.
When behavior differs between Linux and Windows, account for WSL by using isWSL() rather than assuming `

Files:

• src/engine/safe-path.ts

src/{helpers,engine}/**/*.ts

📄 CodeRabbit inference engine (.archgate/adrs/ARCH-002-error-handling.md)

Do not use console.log(), console.warn(), or console.info() directly in helper or engine files; use logInfo() or logWarn() instead.

Files:

• src/engine/safe-path.ts

src/engine/**/*.ts

📄 CodeRabbit inference engine (.archgate/adrs/ARCH-023-engine-file-listing-via-in-memory-git-tracked-matching.md)

src/engine/**/*.ts: In the rules engine, list project files by matching glob patterns in memory against the git-tracked file set rather than walking the filesystem.
Route new engine file listings through listMatchingFiles for rule-facing inputs or matchTrackedFiles for trusted ADR frontmatter patterns.
When the target is a Git repository and respectGitignore is not false, pass the tracked set from getGitTrackedFiles to matching operations.
Per-run RunCaches must share glob results keyed by pattern and tracked mode and file text keyed by absolute path; cached promises should share in-flight work, and returned glob arrays must be copied before exposing them to rules.
Do not cache readJSON results because rules receive mutable objects whose shared mutation could leak between rules.
Do not filter filesystem scan results against the tracked set as a substitute for in-memory matching, and do not hardcode ignore directories; use the Git-derived tracked set instead.

No unsanctioned subprocess calls may be added to the engine. Bun.spawn/Bun.spawnSync for AST support belongs only in the sanctioned AST helper, git subprocesses belong only in git-files.ts, and child_process imports are prohibited.

Files:

• src/engine/safe-path.ts

src/{engine/**/*.ts,formats/rules.ts,helpers/rules-shim.ts}

📄 CodeRabbit inference engine (.archgate/adrs/ARCH-022-ast-aware-rule-context.md)

src/{engine/**/*.ts,formats/rules.ts,helpers/rules-shim.ts}: RuleContext MUST expose exactly one language-dispatching method: ast(path: string, language: "typescript" | "javascript" | "python" | "ruby", opts?: AstOptions): Promise<AstNode>, with no per-language AST methods. The returned AST shape is language-native and may differ by language.
TypeScript/JavaScript AST parsing MUST reuse the existing in-process meriyah parser. The duplicated parseModule() logic in rule-scanner.ts MUST be factored into one shared exported helper used by both rule scanning and ctx.ast().

Files:

• src/engine/safe-path.ts

🧠 Learnings (4)

📚 Learning: 2026-06-11T12:50:28.661Z

Learnt from: rhuanbarreto
Repo: archgate/cli PR: 406
File: .claude/agent-memory/archgate-developer/feedback_prefer_tests_over_adr_rules.md:8-18
Timestamp: 2026-06-11T12:50:28.661Z
Learning: In `archgate/cli`, for markdown files under `.claude/agent-memory/`, follow the established convention: use YAML frontmatter (with a `name:` field used as the document title) and do not require a top-level `#` (H1) heading. During code review, do not flag missing first-line/first-top-level H1 headings (e.g., MD041) for these agent-memory files since markdownlint is not part of the repo’s `bun run validate` lint pipeline (oxlint/oxfmt only).

Applied to files:

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

📚 Learning: 2026-07-25T00:05:20.592Z

Learnt from: rhuanbarreto
Repo: archgate/cli PR: 496
File: .claude/agent-memory/archgate-developer/project_test_isolation_gotchas.md:10-10
Timestamp: 2026-07-25T00:05:20.592Z
Learning: When reviewing documentation/agent-memory entries under `.claude/agent-memory/**`, do not enforce GEN-004’s “forward-only” comment/narrative requirement. These entries are allowed to keep historical/past-tense incident narratives and dated markers (e.g., `Found YYYY-MM-DD`) because the context is intended to help future agents evaluate edge cases. Outside this scope, GEN-004’s forward-only rule should still apply.

Applied to files:

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

📚 Learning: 2026-07-25T00:05:58.884Z

Learnt from: rhuanbarreto
Repo: archgate/cli PR: 496
File: tests/helpers/auth.test.ts:38-46
Timestamp: 2026-07-25T00:05:58.884Z
Learning: When reviewing the Archgate CLI repository’s GEN-004 “concise forward-only narration” comments, don’t rely only on the automated phrase-based narration checks. Those checks can pass even when the comment wording describes historical/transfer semantics rather than current behavior (e.g., saying a prior restore “leaked” a value or a later subprocess “inherited it”). Manually verify that the comment describes the code’s current, forward behavior; flag or adjust comments that imply past/historical state transfer even if GEN-004 enforcement passes.

Applied to files:

• tests/engine/safe-path.test.ts
• src/engine/safe-path.ts

📚 Learning: 2026-07-25T00:05:59.109Z

Learnt from: rhuanbarreto
Repo: archgate/cli PR: 496
File: src/cli.ts:0-0
Timestamp: 2026-07-25T00:05:59.109Z
Learning: Code comments may include a concise issue/PR reference (per GEN-004) when it’s used to point readers to fuller rationale instead of inlining that rationale. During review, flag surrounding comment prose that reads like historical context or narrates refactors/relocations; a bare GEN-004-style reference is allowed and should not be flagged by itself.

Applied to files:

• tests/engine/safe-path.test.ts
• src/engine/safe-path.ts

🔇 Additional comments (4)

src/engine/safe-path.ts (3)

3-3: LGTM!

---

117-133: LGTM!

---

61-71: 🔒 Security & Privacy

No change needed. ARCH-022 already defines the path-safety goal as “no symlink escapes… every component below the project root,” and the docstring’s escape-only wording is consistent with that.

.claude/agent-memory/archgate-developer/project_rules_engine_internals.md (1)

19-21: LGTM!

[rhuanbarreto]

@coderabbitai approve

[coderabbitai]

✅ Action performed

Comments resolved and changes approved.