feat(engine): opt-in relative imports for rule files, contained to .archgate/ - #491
feat(engine): opt-in relative imports for rule files, contained to .archgate/#491hancrafted wants to merge 3 commits into
Conversation
…rchgate/ Rule files (.rules.ts) could only import a small node: allowlist, forcing shared helpers to be copy-pasted across every ADR. This adds an opt-in way to import helpers by relative path while keeping the in-process sandbox intact. Closes archgate#490. Added - `ruleImports.allowedDirs` in `.archgate/config.json`: directories a rule file may import from by relative path. - `resolveRuleImportDirs`: resolves + realpath-canonicalizes each entry and rejects (UserError) any that escapes `<root>/.archgate/`. This is a hard boundary enforced regardless of value — `..` and symlink escapes included. - Transitive scan: every allowed relative import is recursively scanned with the same options (cycle-guarded), so a helper still cannot reach child_process/fetch/eval/etc. Changed - `scanRuleSource(source, opts?)` takes an options object ({ preTranspiled, filePath, allowedImportDirs }); the 1-arg form is unchanged. Absent config ⇒ all relative imports stay blocked (default). Fixed - AST node schema rejected `export` declarations with `source: null`, so `parseNode` dropped the node and left top-level `export function` / `export const` bodies unscanned. Now tolerated, closing that bypass. Signed-off-by: hancrafted <hancrafted@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR adds an opt-in mechanism for .rules.ts files to use contained relative imports for shared helper modules, while preserving the default “no relative imports” security posture by requiring explicit configuration and enforcing filesystem containment.
Changes:
- Introduces
ruleImports.allowedDirsin.archgate/config.jsonand resolves it into realpath-canonicalized,.archgate/-contained directories. - Extends the rule security scanner to allow only contained relative imports and to transitively scan imported helper files under the same sandbox rules.
- Adds dedicated unit + end-to-end test coverage for containment, resolver behavior (including symlink escape), loader integration, and a regression fix for exported-declaration AST traversal.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/helpers/project-config-rule-imports.test.ts | Tests config-driven directory resolution and containment errors. |
| tests/engine/rule-scanner.test.ts | Adds regression coverage ensuring exported declarations are still scanned. |
| tests/engine/rule-scanner-contained-imports.test.ts | Direct scanner tests for contained relative imports + transitive scanning behavior. |
| tests/engine/rule-import-resolver.test.ts | Unit tests for relative-specifier detection and contained import resolution (incl. symlink escape). |
| tests/engine/loader-contained-imports.test.ts | End-to-end coverage from config → loader → scanner (default closed, opt-in open). |
| src/helpers/project-config.ts | Implements resolveRuleImportDirs to canonicalize and validate allowed import directories. |
| src/helpers/paths.ts | Adds isPathInside helper for containment checks. |
| src/formats/project-config.ts | Adds ruleImports.allowedDirs schema (filesystem validation deferred to resolver). |
| src/engine/rule-scanner.ts | Adds scan options + contained-relative-import allowance and transitive scanning of imported helpers; fixes AST schema for source: null. |
| src/engine/rule-import-resolver.ts | Implements secure contained import resolution with extension/index probing and realpath containment. |
| src/engine/loader.ts | Threads allowed import dirs into scanning during rule ADR loading. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let archgateRoot: string; | ||
| try { | ||
| archgateRoot = realpathSync(projectPaths(projectRoot).root); | ||
| } catch { | ||
| // No `.archgate/` on disk — nothing can be imported; treat as unconfigured | ||
| // rather than erroring, so the default (block everything) applies. | ||
| return []; | ||
| } |
| return dirs.map((dir) => { | ||
| const abs = resolve(projectRoot, dir); | ||
| let real: string; | ||
| try { | ||
| real = realpathSync(abs); | ||
| } catch { | ||
| throw new UserError( | ||
| `Invalid ruleImports.allowedDirs entry "${dir}": directory does not exist.`, | ||
| "It must be an existing directory inside .archgate/." | ||
| ); | ||
| } |
|
Warning Review limit reached
Next review available in: 46 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds opt-in contained relative imports for rule files. Project configuration accepts 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Address review feedback on resolveRuleImportDirs: - Anchor the .archgate/ tree to the realpath'd project root, rejecting a symlinked .archgate/ whose target escapes the repo. Without this, a symlinked .archgate/ could relocate the containment boundary and authorize rule-file imports outside the project. - Reject allowedDirs entries that resolve to a file rather than a directory (statSync isDirectory), matching the field's directory semantics and downstream assumptions. Containment is checked before the directory check so the security-relevant message wins. Adds regression tests for both cases. Signed-off-by: hancrafted <hancrafted@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/helpers/project-config.ts`:
- Around line 279-288: Narrow the catch around realpathSync in the project
configuration flow to handle only ENOENT as the unconfigured case returning [];
rethrow or wrap all other filesystem errors, such as EACCES or ELOOP, in the
established UserError type so they are surfaced consistently with the per-entry
handling.
In `@tests/helpers/project-config-rule-imports.test.ts`:
- Line 80: Update the duplicate test-name prefixes in the relevant tests near
the existing “(g)” cases: retain “(g)” for the original test, and rename the two
newly added tests to sequential unique “(h)” and “(i)” labels.
🪄 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: ef854ff7-57d3-47b1-9508-e9209cc5c838
📒 Files selected for processing (2)
src/helpers/project-config.tstests/helpers/project-config-rule-imports.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (8)
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 likeexport { X } from "./other"andexport type { X } from "./other"are forbidden.
Import symbols directly from the module that defines them; do not import from a directory path such as../formatsexpecting implicitindex.tsresolution.
src/**/*.ts: Useawait Bun.file(path).json()when reading JSON files in Bun TypeScript source code; do not useJSON.parse(await Bun.file(path).text())orJSON.parse(fs.readFileSync(path, "utf-8"))for file reads.
UseBun.JSONC.parse()when reading files that may contain comments, such astsconfig.json, instead of plainJSON.parse()on file contents.
ReserveJSON.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 undersrc/, useBun.envinstead ofprocess.envfor all environment variable reads and writes;process.envmust not be used.
In TypeScript source files undersrc/, use nullish coalescing for environment-variable defaults, e.g.Bun.env.NODE_ENV ?? "production".
In TypeScript source files undersrc/, useBoolean(Bun.env.CI)for truthy checks on environment flags.
In TypeScript source files undersrc/, do not destructureBun.env(for example,const { HOME } = Bun.env); access properties individually instead.
In TypeScript source files undersrc/, do not referenceprocess.enveven in comments that suggest using it.
src/**/*.ts: Heavy dependencies such asinquirer,posthog-node,@sentry/*, and any similarly expensive runtime-only dependency MUST be loaded with dynamicimport()at the point of use, never as a static value import at module top level.
Type-only references to heavy modules must useimport type(for example,import type { PostHog } from "posthog-node"orimport type * as SentryNs from "@sentry/node-core/light"); type-only...
Files:
src/helpers/project-config.ts
**/*.{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, useBun.file()instead offs.readFile()for simple reads.
Prefernode:built-in modules such asnode:util,node:path, andnode:fsover npm alternatives.
Do not use utility libraries for single functions, such as importing lodash for one helper likepick.
Do not use path aliases (tsconfig paths); use relative imports with Bun's native module resolution.
Do not install packages globally during development; usebunxfor one-off tools.
Files:
src/helpers/project-config.tstests/helpers/project-config-rule-imports.test.ts
src/**/!(*platform).ts
📄 CodeRabbit inference engine (.archgate/adrs/ARCH-009-platform-detection-helper.md)
src/**/!(*platform).ts: Insrc/TypeScript source files, do not readprocess.platformdirectly; usesrc/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 insrc/TypeScript source.
When behavior differs between Linux and Windows, account for WSL by usingisWSL()rather than assuming `
Files:
src/helpers/project-config.ts
{src,tests}/**/*.ts
📄 CodeRabbit inference engine (.archgate/adrs/LEGAL-001-spdx-license-headers.md)
{src,tests}/**/*.ts: Every TypeScript source file insrc/andtests/must begin with// SPDX-License-Identifier: Apache-2.0followed by// Copyright 2026 Archgate.
If a TypeScript file has a shebang line (for example#!/usr/bin/env buninsrc/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:
src/helpers/project-config.tstests/helpers/project-config-rule-imports.test.ts
src/{helpers,engine}/**/*.ts
📄 CodeRabbit inference engine (.archgate/adrs/ARCH-002-error-handling.md)
Do not use
console.log(),console.warn(), orconsole.info()directly in helper or engine files; uselogInfo()orlogWarn()instead.
Files:
src/helpers/project-config.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.tsfile with automated checks that run viaarchgate check.When reviewing, you must:
- Treat ADR violations as blocking issues, not suggestions.
- Cite the specific ADR ID when flagging a violation (e.g., "Violates ARCH-006").
- Focus on semantic and contextual violations that automated rules cannot catch —
the.rules.tsfiles already cover syntactic/structural patterns.- If you are unsure whether something violates an ADR, flag it as a question
rather than approving it.
Files:
src/helpers/project-config.tstests/helpers/project-config-rule-imports.test.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 mutatingprocess.platformdirectly.
tests/**/*.ts: Use Bun's built-inbun:testrunner for all tests; do not import test utilities fromnode:test.
Shared test helpers must also userestoreEnvwhen restoring environment variables.
Files:
tests/helpers/project-config-rule-imports.test.ts
tests/**/*.test.ts
📄 CodeRabbit inference engine (.archgate/adrs/ARCH-005-testing-standards.md)
tests/**/*.test.ts: Place tests intests/mirroring thesrc/directory structure, and name test files with the.test.tssuffix.
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 withmkdtempfor filesystem tests, and clean them up inafterEachorafterAll. Do not use hardcoded user or system paths.
Close external SDK instances such as servers, clients, and transports inafterEachorafterAll; manage their lifecycle in test hooks rather than individual test bodies.
When a temporary git repository performs commits, configure localuser.emailanduser.nameimmediately aftergit init; do not rely on global git identity.
Every runnabletest()orit()must contain at least oneexpect()assertion. Make implicit no-throw contracts explicit withnot.toThrow()orresolves.toBeUndefined(). Usetest.skiportest.todofor intentional placeholders.
Usetest.skipIf(condition),test.skip, ortest.todofor 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, importexpectfrombun:test.
Mock HTTP requests by assigning directly toglobalThis.fetch, and restore the original fetch implementation or mock inafterEach; do not mocknode:fetch.
Mock first-party modules withimport * as modandspyOn(mod, "fn"), restoring spies after each test; do not use process-globalmock.module()for first-party modules.
Wrap inlinespyOnormockImplementationusage intry/finallysomockRestore()always runs, or manage spies inbeforeEachandafterEach.
When redirecting user-scope paths, mocknode:os'shomedir()rather than relying on runtimeHOMEoverrides; use environment overrides only for code that reads environment variables directly at call time.
Re...
Files:
tests/helpers/project-config-rule-imports.test.ts
🧠 Learnings (3)
📚 Learning: 2026-07-02T16:03:33.031Z
Learnt from: rhuanbarreto
Repo: archgate/cli PR: 446
File: src/helpers/session-context-opencode.ts:81-100
Timestamp: 2026-07-02T16:03:33.031Z
Learning: For synchronous helper functions that use Bun’s sqlite sync API (i.e., they must remain synchronous), it’s acceptable to use `existsSync` from `node:fs` to check whether the SQLite database file exists. Avoid using `Bun.file(path).exists()` for this purpose because it’s async and would force the helper to become async (no equivalent synchronous Bun alternative). If the DB file is missing, throw/return a clear, actionable "No database found" error (per ARCH-006) rather than letting the sqlite open fail with a generic DB-open error.
Applied to files:
src/helpers/project-config.ts
📚 Learning: 2026-07-15T22:55:51.978Z
Learnt from: rhuanbarreto
Repo: archgate/cli PR: 476
File: tests/helpers/telemetry-config.test.ts:24-28
Timestamp: 2026-07-15T22:55:51.978Z
Learning: In this Bun/TypeScript codebase, when a unit under test spawns subprocesses via Bun.spawn (e.g., running `git credential ...`), prefer overriding relevant env vars (such as `HOME`, `GIT_CONFIG_GLOBAL`, `GIT_CONFIG_NOSYSTEM`) using `process.env` in the test and restoring them with the test utility (e.g., `restoreEnv` from `tests/test-utils.ts`). Avoid relying on `spyOn(os, 'homedir')` for this purpose, because it only affects in-process calls and does not change the environment inherited by subprocesses; env-var overrides should be used for subprocess-level isolation and must be applied at call time.
Applied to files:
tests/helpers/project-config-rule-imports.test.ts
📚 Learning: 2026-07-15T22:56:35.415Z
Learnt from: rhuanbarreto
Repo: archgate/cli PR: 476
File: tests/commands/clean.test.ts:61-62
Timestamp: 2026-07-15T22:56:35.415Z
Learning: When reviewing tests that rely on src/helpers/paths.ts `internalPath()`, note that `internalPath()` intentionally reads `Bun.env.HOME ?? Bun.env.USERPROFILE` at call time and only uses `os.homedir()` if neither env var is set. Therefore, don’t suggest changing tests to `spyOn(os, "homedir")` for this behavior; instead, use per-test `Bun.env.HOME` / `Bun.env.USERPROFILE` overrides (as applicable) so the tests control `internalPath()`’s inputs.
Applied to files:
tests/helpers/project-config-rule-imports.test.ts
🔇 Additional comments (2)
src/helpers/project-config.ts (1)
12-13: LGTM!Also applies to: 26-31, 257-270, 302-327
tests/helpers/project-config-rule-imports.test.ts (1)
80-84: LGTM!Both regression tests correctly exercise the new
statSync-based file-vs-directory check and the project-root-anchored symlink-escape check.Also applies to: 86-107
| expect(() => resolveRuleImportDirs(root)).toThrow(/does not exist/u); | ||
| }); | ||
|
|
||
| test("(g) rejects an allowedDirs entry that is a file, not a directory", () => { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Duplicate (g) test-name prefix.
Three tests now share the "(g) ..." prefix (line 55, and the two new tests here). Consider bumping these to (h) and (i) for unique, sequential labels.
Also applies to: 86-86
🤖 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/helpers/project-config-rule-imports.test.ts` at line 80, Update the
duplicate test-name prefixes in the relevant tests near the existing “(g)”
cases: retain “(g)” for the original test, and rename the two newly added tests
to sequential unique “(h)” and “(i)” labels.
| * Falls back to the standard `.archgate/adrs/` and `.archgate/lint/` | ||
| * defaults when no `paths` config is present. | ||
| */ |
| expect(dirs).toEqual([realpathSync(join(archgate, "lib"))]); | ||
| }); | ||
|
|
||
| test("(g) rejects an allowedDirs entry that escapes via ..", () => { |
- Narrow the .archgate/ realpath catch to ENOENT (missing => feature off); surface other faults (EACCES/ELOOP) as a UserError instead of silently disabling rule imports on an otherwise-configured project (CodeRabbit). - Re-attach the resolvedProjectPaths JSDoc that was orphaned above resolveRuleImportDirs (Copilot). - Rename the 'escapes via ..' test: the entry is a sibling dir with no .. traversal (Copilot). - Give the two new config-rejection tests unique sequential labels (i)/(j); (h) is already used by rule-scanner-contained-imports.test.ts (CodeRabbit). - Add a spyOn-based test (k) covering the non-ENOENT surface path. Signed-off-by: hancrafted <hancrafted@users.noreply.github.com>
| // configured dirs against it would authorize imports outside the project — | ||
| // defeating the containment boundary. Anchor to the real project root so the | ||
| // guarantee holds regardless of what `.archgate` points at. | ||
| if (!isPathInside(archgateRoot, join(realProjectRoot, ".archgate"))) { |
| if (!statSync(real).isDirectory()) { | ||
| throw new UserError( | ||
| `Invalid ruleImports.allowedDirs entry "${dir}": not a directory (${real}).`, | ||
| "Each ruleImports.allowedDirs entry must be a directory inside .archgate/." | ||
| ); | ||
| } |
|
Thanks for this, and for building it out instead of just describing it. The diff makes the tradeoff concrete, which I appreciate. I also want to be straight that the cost you're solving is one we chose on purpose: ARCH-024's Consequences already say helper reuse is refused and rule files have to be self-contained. You're not pointing at a gap we missed. You're asking us to revisit the tradeoff, which is fair. I'm leaning against merging this, and I'd rather walk through the reasoning than just cite the ADR, because I want your read on it before we decide anything. You've built the careful version of this. Realpath containment into And it's also where my hesitation sits. What the PR does is move the boundary from "refuse what the scanner can't read" to "a recursive resolver plus a containment checker that we now have to keep correct forever." ARCH-024 declined exactly that, and named it as a tracked risk: a future contributor fixes the refusal of relative imports by following them instead. The single-file view isn't a shortcoming the PR removes, it's the property that makes the scanner easy to trust. A resolver-and-containment layer is more code standing between an untrusted rule and a CI runner with deploy credentials, and one bug anywhere in it is an RCE that The containment story is strong, but "inside Procedurally, ARCH-024 also asks for a change like this to be a separate ADR with maintainer sign-off before implementation, since "follow imports transitively rather than refuse them" is called out under its Exceptions. So the way I'd want to handle it is to settle the security question first, then decide on the code. Two things I don't want to lose, whatever we land on. The And the duplication is still worth solving. The path I'd back is moving the common helpers onto I want your take on the resolver-as-boundary concern before we go further. If you can talk me out of it, I'm listening. |
|
Hi Rhuan — thanks for reasoning it through rather than just citing the ADR! Quick context on what I'm building, since it shapes where I go next: a reusable harness for AI / knowledge-work projects — heavy on markdown, light on code (https://github.com/hancrafted/typescript-ai-harness). The end state is a set of ADRs each governing one markdown type (PRD, report, tasks, …), which is where the duplication I was chasing really shows up. On the resolver-as-boundary concern: I concede and take up your offer to contribute features for ctx. I was more thinking as an engineer wanting to solve one (mine) problem rather than as an architect. On export … source: null: I'll create a separate PR and close this one. One broader question, because it shapes what I'd want from Thanks again — glad to be contributing, and looking forward to the ctx sketch. |
|
I think the solution to your request is actually using I'm still very positive to the extra helpers for ctx like |
|
Thanks for the reference. As for the the ctx helpers, let's start with readYAML and checkCase. Should I just propose a design and create ticket+PR? Or do you have another approach in mind? |
) Splits the scanner fix out of #491 (now closed) into a standalone PR, as suggested there. ### The bug ESTree sets `source: null` on any `export` declaration with no `from` clause — `export function`, `export const`, `export { local }`. `AstNodeSchema` typed `source` as optional-only, so `safeParse` failed on the node, `parseNode` returned `null`, and the walk (`if (child) walk(child)`) skipped the node **and its entire subtree**. A banned global, `eval`, or a dynamic `import("node:child_process")` nested inside a top-level `export` was therefore never scanned — a silent false-negative that `archgate check` reports as a pass. ### The fix Make `source` `.nullable().optional()` in the schema (and `AstNode.source?: AstNode | null`) so the node stays in the walk. `checkModuleSpecifier` already no-ops on a null source, so re-exports (`export { x } from "…"`) are unaffected. No behavior change beyond no longer dropping these nodes. ### Tests Three regression tests in `rule-scanner.test.ts`: - banned global inside `export function` — unscanned before, caught now - banned global inside `export const` — unscanned before, caught now - `export { x } from "node:fs"` — positive control, confirms re-export scanning still fires `bun run validate` passes locally: oxlint, `tsc --build`, `oxfmt --check`, 1579 tests, `archgate check` 44/44, compile. The commit is DCO signed-off. --------- Signed-off-by: hancrafted <hancrafted@users.noreply.github.com> Co-authored-by: hancrafted <hancrafted@users.noreply.github.com>
I'm coming with a PR soon. Will be on next release! |
…ctory (#499) Split out of #497 at your request, following the same call you made on #491 (pre-existing security finding → standalone PR). Surfaced by CodeRabbit while reviewing #497. The finding is real and **predates that PR** — #497 only moved this code verbatim out of `runner.ts`, so the gap exists on `main` today. ## The escape `safePath()` `lstat`'d only the **final** path component. Given a symlinked ancestor: ``` <root>/linkdir -> /somewhere/outside ``` then `ctx.readFile("linkdir/secret.txt")` gets through every gate: 1. `resolveUserPath()` uses `resolve()`, which is **purely lexical** — it does not resolve symlinks, so the path stays `<root>/linkdir/secret.txt`. 2. `isWithinRoot()` is a string-prefix test on that lexical path — **passes**. 3. `lstatSync(absPath)` inspects only the leaf, which is an ordinary file, not a link — **passes**. 4. `Bun.file(absPath).text()` hands the path to the OS, which *does* resolve `linkdir` — and reads outside the project. So a `.rules.ts` file could read arbitrary files outside the project through a symlinked directory, which is exactly the boundary ARCH-024 exists to hold. ## The fix The leaf check and the ancestor check become one walk over every path component **below** the project root. Two deliberate constraints: - **Nothing at or above the root is inspected.** The project root's own location is the user's business, and on macOS the temp prefix is itself a symlink (`/var` → `/private/var`) — walking above the root would reject every path under a temp-dir root, breaking a large share of the test suite for no security gain. - **Each component is a boolean `lstat`, not a string comparison against `realpath`.** A `realpath`-and-compare implementation is one syscall instead of N and was my first instinct, but `realpath` case-canonicalizes on Windows and macOS, so it would reject case-mismatched-but-legitimate paths. The boolean question has no such failure mode. Verified directories are memoized per process. `safePath` runs once per glob result and sibling files share every ancestor, so without the memo the walk would re-`lstat` the same handful of directories thousands of times per check run (`grepFiles` × 40+ rules). ## Tests - `tests/engine/safe-path.test.ts` — ancestor symlink rejected, leaf symlink rejected, deep real-directory chain accepted, non-existent in-root path tolerated, root itself accepted, traversal rejected - `tests/commands/check-security.test.ts` — end-to-end through `runChecks`: a rule reading through a symlinked parent is blocked, and a rule reading a deeply nested real path still succeeds (guards against over-rejection) **The regression tests actually run on Windows.** They create the directory link with symlink type `"junction"` — ignored on POSIX (plain symlink), but on Windows a junction needs no admin privileges and `lstat` reports it as a symbolic link. The pre-existing file-symlink test still skips on Windows, since file symlinks require elevation or Developer Mode; that limitation is now isolated to that one case rather than covering the whole symlink surface. I confirmed the fix is load-bearing rather than trusting a green suite: with the guard call stubbed out, the ancestor test fails; with it in place, it passes. ## Note on overlap with #497 The helpers move to `src/engine/safe-path.ts` here because `runner.ts` sits **exactly** at the 500-line `max-lines` cap on `main` — the fix cannot land in place. #497 performs the same extraction independently for the same reason. The two versions of the file are identical apart from this fix, so whichever merges second needs a trivial rebase. `bun run validate` passes (lint, typecheck, format:check, full suite, `archgate check` 44/44, knip, build:check). Signed-off-by: Rhuan Barreto <rhuan@barreto.work>
Closes #490. ## Context #490 asked for shared helper modules importable from `.rules.ts` files. That was declined in #491 — following relative imports would turn the scanner's single-file view into a recursive resolver plus containment checker, which becomes the security boundary (exactly what ARCH-024 declined and named as a tracked risk). The agreed path for the underlying duplication pain was to move the common helpers onto `ctx` instead, and `readYAML` (with frontmatter support) plus `checkCase` were the two named there. This PR adds both. The sandbox boundary is untouched — no new subprocess site, no new resolver, no new dependency (`Bun.YAML` is a runtime built-in, per ARCH-006). ## `ctx.readYAML(path)` Returns one object covering both YAML documents and Markdown frontmatter: ```typescript interface ReadYamlResult { frontmatter: Record<string, YamlValue> | null; content: YamlValue; } ``` Dispatch is **extension-based**, not content-sniffing: - **`.yml` / `.yaml`** — the whole document parses as YAML. `frontmatter` is always `null`, `content` is the parsed value. Because dispatch is by extension, a multi-document stream's `---` separators (Kubernetes manifests, CI configs) are never misread as a frontmatter block. - **Every other file** (typically Markdown) — the leading `---`-delimited block parses as `frontmatter`: `null` when absent, `{}` when present but empty. `content` is the remaining body text, trimmed, and is **not** parsed as YAML. `null` for "no frontmatter" is deliberate and mirrors `fileAtBase()`: "does this file have frontmatter?" is ordinary rule control flow, checkable with one null test rather than a `try/catch`. Values are typed `YamlValue` (the JSON-like data YAML's core schema produces) rather than `unknown`, so after a `typeof` check a rule can index into mappings and sequences without casting. Fail-closed like `ast()`: a malformed `.yml` file, or a frontmatter block that is invalid YAML or parses to a non-mapping (scalar/sequence), **throws** — surfacing as a rule execution error (exit 2) instead of a silent false pass. Reads go through the same `safePath` sandbox as `readFile`, and share the per-run file-text cache; the parsed value is deliberately **not** cached, matching `readJSON` (rules receive a mutable object, and sharing one instance would leak mutations between rules). ## `ctx.checkCase(value, scheme)` Pure and synchronous — no `await`. Schemes: `kebab-case`, `camelCase`, `PascalCase`, `snake_case`, `SCREAMING_SNAKE_CASE`. Matching is all-or-nothing and ASCII-only; the empty string matches no scheme. `camelCase`/`PascalCase` follow the ecosystem convention (typescript-eslint's `naming-convention`): a leading lower/uppercase letter followed by any ASCII alphanumerics, so acronym runs (`parseURL`, `HTTPServer`) match. An unrecognized scheme name **throws** rather than returning `false`, so a typo surfaces as a rule error instead of a silent false pass/fail. ## Incidental: ARCH-022 gains a fifth rule Wiring these helpers made it concrete that the `RuleContext` mirror between `src/formats/rules.ts` and the generated ambient shim in `src/helpers/rules-shim.ts` is maintained entirely **by hand**, and that only the `ast()` signature was enforced (`single-ast-method`). A drifted shim silently hands rule authors wrong types with no signal. The new `rulecontext-shim-parity` rule extracts the `interface RuleContext` member names from both surfaces and fails on any member present in one but missing from the other, in **both** directions. Extraction is a regex over raw text rather than `ctx.ast()` — `Bun.Transpiler` erases type-only interface declarations before the tree is built, so the AST never contains them. Both failure directions were fire-tested against a deliberately drifted shim before committing; the rule is member-name parity only, so full signature/JSDoc parity stays a documented manual review item. ## Stacked on #499 > **Base branch is `fix/safepath-ancestor-symlink` (#499), not `main`.** Merge #499 first; GitHub retargets this PR to `main` automatically. `safePath`/`isWithinRoot`/`resolveUserPath` move out of `runner.ts` into `src/engine/safe-path.ts` because `runner.ts` sits exactly at the 500-line `max-lines` cap. Review flagged that the extracted `safePath` only `lstat`s the **final** path component, so a symlinked ancestor directory escapes the sandbox. That gap is pre-existing — `git diff` against `main` shows the extraction is byte-for-byte apart from line-wrapping — so the fix went to its own fast-trackable PR (#499), per the standing preference for pre-existing security findings. But this is the PR that *creates* `safe-path.ts`, so merging it first would land a freshly-added file containing a known escape. Rather than leave that to merge-order convention, this PR is stacked on #499: the fix is present in this branch, the review thread is addressed in this PR's own diff, and the ordering hazard is structural rather than advisory. ## Tests - `tests/engine/yaml-utils.test.ts` — whole-document vs frontmatter dispatch, multi-document stream not misread as frontmatter, BOM, CRLF, empty block, body-never-parsed-as-YAML, invalid YAML, non-mapping block - `tests/engine/check-case.test.ts` — accept/reject tables per scheme, empty string, non-ASCII, unknown-scheme throw - `tests/engine/runner-yaml-case.test.ts` — end-to-end through `runChecks`, including the `safePath` sandbox on `readYAML` and a realistic kebab-case filename rule - `tests/engine/safe-path.test.ts` — extracted sandbox helpers ## Docs `reference/rule-api.mdx` in all three locales (en, nb, pt-br) per GEN-002: the `RuleContext` interface listing plus new `readYAML` and `checkCase` sections. `bun run validate` passes (lint, typecheck, format:check, 1600+ tests, `archgate check` 45/45, knip, build:check). --------- Signed-off-by: Rhuan Barreto <rhuan@barreto.work> Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
# archgate ## [0.51.0](v0.50.0...v0.51.0) (2026-07-26) ### Features * **adrs:** enforce concise, forward-only code comments (GEN-004) ([#496](#496)) ([9a114b3](9a114b3)), references [#2123](https://github.com/archgate/cli/issues/2123) * **adrs:** flag stray files at the repository root (GEN-005) ([#535](#535)) ([6a6e765](6a6e765)), closes [#514](#514), references [#500](#500) * **engine:** add ctx.readYAML and ctx.checkCase rule helpers ([#497](#497)) ([c5d82c5](c5d82c5)), closes [#490](#490), references [#490](#490) [#491](#491) [#499](#499) [#499](#499) [#499](#499) [#499](#499) * report truncated ADR briefings, trim the ADR corpus 15.6%, add GEN-005 briefing budget ([#501](#501)) ([a9dab40](a9dab40)) ### Bug Fixes * **docs:** relocate ADR content to clear briefing-budget warnings ([#531](#531)) ([c7419b3](c7419b3)) * **docs:** restore pt-br diacritics and enforce locale content integrity ([#523](#523)) ([db39104](db39104)), closes [#516](#516), references [#231](#231) * **engine:** allow symlinks that resolve inside the project root ([#500](#500)) ([387bf15](387bf15)) * **engine:** reject rule-file reads through a symlinked ancestor directory ([#499](#499)) ([a555f9d](a555f9d)), references [#497](#497) [#491](#491) [#497](#497) [#497](#497) [#497](#497) [#497](#497) * **engine:** scan top-level export declarations with a null source ([#493](#493)) ([d07db03](d07db03)), closes [#491](#491) * **engine:** stop dropping AST nodes with exotic literal values ([#494](#494)) ([0015542](0015542)), closes [#493](#493) [#493](#493) * **lint:** resolve no-bare-env-restore by captured key and lexical scope ([#524](#524)) ([7094a3a](7094a3a)), closes [#498](#498) * **rules:** make ARCH-020 and ARCH-023 match ctx.ast() instead of raw text ([#533](#533)) ([ad5529b](ad5529b)), closes [#513](#513), references [#486](#486) * **tests:** replace bun:test anti-patterns with idiomatic patterns ([#512](#512)) ([bcb086f](bcb086f)) --- This PR was generated with [simple-release](https://github.com/TrigenSoftware/simple-release). <details> <summary>📄 Cheatsheet</summary> <br> You can configure the bot's behavior through a pull request comment using the `!simple-release/set-options` command. ### Command Format ````md !simple-release/set-options ```json { "bump": {}, "publish": {} } ``` ```` ### Useful Parameters #### Bump | Parameter | Type | Description | |-----------|------|-------------| | `version` | `string` | Force set specific version | | `as` | `'major' \| 'minor' \| 'patch' \| 'prerelease'` | Release type | | `prerelease` | `string` | Pre-release identifier (e.g., "alpha", "beta") | | `firstRelease` | `boolean` | Whether this is the first release | | `skip` | `boolean` | Skip version bump | | `byProject` | `Record<string, object>` | Per-project bump options for monorepos | #### Publish | Parameter | Type | Description | |-----------|------|-------------| | `skip` | `boolean` | Skip publishing | | `access` | `'public' \| 'restricted'` | Package access level | | `tag` | `string` | Tag for npm publication | ### Usage Examples #### Force specific version ````md !simple-release/set-options ```json { "bump": { "version": "2.0.0" } } ``` ```` #### Force major bump ````md !simple-release/set-options ```json { "bump": { "as": "major" } } ``` ```` #### Create alpha pre-release ````md !simple-release/set-options ```json { "bump": { "prerelease": "alpha" } } ``` ```` #### Publish with specific access and tag ````md !simple-release/set-options ```json { "bump": { "prerelease": "beta" }, "publish": { "access": "public", "tag": "beta" } } ``` ```` ### Custom Changelog Preamble You can add custom markdown to the top of the changelog (right after the version header) using the `!simple-release/set-preamble` command. The markdown after the command line becomes the preamble. ```md !simple-release/set-preamble ## What's new? - The website was completely redesigned - The new API gives you awesome possibilities ``` In a monorepo, pass the full package name after the command to target a single package's changelog. Wrap the name in backticks so GitHub keeps it as text instead of a mention: ```md !simple-release/set-preamble `@your-org/core` ## Core changes - New plugin system ``` Use one comment per package, plus one without a name for the whole release. ### Access Restrictions The commands can only be used by users with permissions: - repository owner - organization member - collaborator ### Notes - The last comment with `!simple-release/set-options` command takes priority - The last `!simple-release/set-preamble` comment per package takes priority - JSON must be valid, otherwise the `set-options` command will be ignored - Parameters apply only to the current release execution - The commands can be updated by editing the comment or adding a new one </details> <!-- Please do not edit this comment. simple-release-pull-request: true simple-release-branch-from: release simple-release-branch-to: main --> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Closes #490.
Problem
.rules.tsfiles may only import a smallnode:allowlist (path|url|util|crypto); all relative imports are statically blocked bysrc/engine/rule-scanner.ts. Shared helpers (frontmatter parsing, glob helpers, kebab-case checks, …) therefore have to be copy-pasted into every ADR's rule file — with silent drift between copies.What this does
An opt-in, security-preserving way to import shared helpers by relative path. Default behavior is completely unchanged: with no config, every relative import stays blocked.
Opt-in config
.archgate/config.jsongains an optional field:{ "ruleImports": { "allowedDirs": [".archgate/lib"] } }Hard containment boundary (
.archgate/)resolveRuleImportDirsresolves each configured dir against the project root,realpath-canonicalizes it, and requires it to live inside<root>/.archgate/. Anything that escapes — an absolute path, a..traversal, or a symlink whose target is outside the tree — is rejected with aUserErrornaming the entry. This is enforced regardless of value: the config can never authorize a path outside.archgate/. A relative import is then allowed only when its resolved (realpath'd) target lands inside one of those dirs; bare specifiers and non-allowlistednode:/data:specifiers stay blocked exactly as before.Transitive scan (sandbox preserved)
Every allowed relative import is recursively scanned with the same options (cycle-guarded via a visited set), so a shared helper still cannot reach
child_process/fetch/eval/Bun/etc. Violations from imported files are annotated with the imported path.Runtime already supports this —
loader.tsimports rule files viaimport(pathToFileURL(...))on Bun, which transpiles.tsand resolves.archgate-local relative imports today; only the static scanner blocked them. No runtime import mechanism was changed.Scanner signature
scanRuleSource(source, opts?)now takes an options object ({ preTranspiled?, filePath?, allowedImportDirs? }). The one-arg form is unchanged, soscanImportedRuleSource(theadr importpath) is untouched — imported packs get no relative-import allowance and must stay self-contained.Incidental hardening
While wiring the transitive scan I found a pre-existing bypass: the AST-node schema rejected
exportdeclarations carryingsource: null(i.e.export function/export const/export { local }), soparseNodedropped the node and its whole subtree — leaving dangerous code inside a top-levelexport-declaration unscanned. The schema now toleratessource: null, closing that gap (regression tests added).Design note / deviation
Relative-import support is wired through the ESM module-specifier constructs the scanner already inspects (
import,export … from,export *, dynamicimport()).require/import.meta.requireremain fully blocked as today — that is the idiomatic sharing mechanism for these ESM (satisfies RuleSet) rule files, and it avoids carving a hole into the blanket global-requireban. This is strictly more restrictive than opening those forms; happy to extend if maintainers prefer.Tests
New coverage under
tests/:tests/engine/rule-scanner-contained-imports.test.ts— allow inside configured dir; block outside.archgate/; block inside.archgate/but outside allowed dir;..-escape; symlink escape; bare specifier; transitivechild_process/fetch/deep-eval; clean chains; cycle termination; default-blocked with no/empty opts.tests/engine/rule-import-resolver.test.ts— resolver unit tests (extension/index resolution, containment, symlink).tests/helpers/project-config-rule-imports.test.ts— config validation (absent/empty ⇒[];../absolute/symlink escapes rejected; non-existent rejected).tests/engine/loader-contained-imports.test.ts— end-to-end throughconfig.json(clean load, transitive block, default-blocked without config, whole-load rejection on escaping dir).export-declaration schema fix.bun run validatepasses locally — lint (oxlint--deny-warnings), typecheck (tsc), format:check (oxfmt), test (bun test), check (archgate self-check), knip, build:check.