Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ This decision does NOT cover:
- **DO** wrap CLI availability checks in `try/catch` returning a boolean — the command may not exist on the system
- **DO** pass `cwd` via the options object when the command must run in a specific directory
- **DO** extract a helper function (e.g., `run(cmd, opts)` or `runGit(args, cwd)`) when multiple subprocess calls share the same pattern within a module
- **DO** use `Promise.allSettled` when running multiple spawned processes concurrently and any of them can reject — inspect statuses only after all have settled, so every process has fully exited before the caller proceeds (see `getGitTrackedFiles` in `src/engine/git-files.ts` for the reference implementation)

### Don't

Expand All @@ -66,6 +67,7 @@ This decision does NOT cover:
- **DON'T** use shell features (pipes `|`, redirects `>`, globbing `*`) in subprocess arguments — `Bun.spawn` executes commands directly without a shell
- **DON'T** forget to `await proc.exited` — reading stdout alone does not guarantee the process has terminated
- **DON'T** use `node:child_process` when `Bun.spawn` provides the same capability — prefer Bun built-ins per [ARCH-006](./ARCH-006-dependency-policy.md)
- **DON'T** race multiple spawned processes with `Promise.all` when one rejection can abandon a still-running sibling — the abandoned process keeps its `cwd` handle open, which on Windows locks that directory (`EBUSY` on removal; observed when `getGitTrackedFiles` ran two `git ls-files` variants against a non-git directory)

## Implementation Pattern

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
---
id: ARCH-023
title: Engine File Listing via In-Memory Git-Tracked Matching
domain: architecture
rules: true
files:
- "src/engine/**"
---

## Context

Every ADR scope resolution and every rule-facing file listing (`ctx.glob()`, `ctx.grepFiles()`) needs the set of project files matching a glob pattern. Before this decision, each of those call sites walked the filesystem with `Bun.Glob#scan({ dot: true })` and filtered the results against the git-tracked file set _afterwards_. The walk cost was paid in full before the filter ran: on a real target project (333 tracked files, 43,645 filesystem entries under `.venv/`, `data/`, and other ignored trees), a single `archgate check` performed ~70 such walks — one per ADR scope plus one per rule-level glob — driving engine time to 1,147ms and saturating CPU cores with redundant directory traversal. Replacing the walks with in-memory matching cut engine time to 160ms (7.2×) on the same project.

**Alternatives considered:**

- **Keep scanning, add ignore lists** — Hardcoding `node_modules/`, `.venv/`, `.git/` exclusions into the scanner reduces the walk but cannot anticipate every ignored tree (`data/`, `dist/`, ML artifacts). `.gitignore` is the project's own authoritative ignore list, and `git ls-files` already applies it.
- **Cache scan results per run** — Deduplicates identical patterns but still pays one full traversal per unique pattern. On large ignored trees a single walk is already the dominant cost.
- **Rewrite the traversal in a lower-level language** — Rejected: the traversal is already native (Zig, inside Bun). The waste is algorithmic — visiting 43k entries to keep 333 — and a faster redundant walk is still redundant. Native addons are additionally rejected by [ARCH-006](./ARCH-006-dependency-policy.md) and [ARCH-022](./ARCH-022-ast-aware-rule-context.md)'s alternatives analysis.
- **Match in memory against `git ls-files` output** — The tracked-file set is already fetched once per run (see `getGitTrackedFiles`). Matching patterns against that list with `Bun.Glob#match()` eliminates traversal entirely and is the chosen approach.

For Archgate specifically, `Bun.Glob#match()` has two properties that make this safe and simpler than scanning: it matches dot-prefixed path segments without any option (scanning requires `dot: true`, see [ARCH-020](./ARCH-020-glob-scan-include-dotfiles.md)), and it handles brace groups whose alternatives contain path separators correctly (the scanner silently returns empty results for them — [oven-sh/bun#32596](https://github.com/oven-sh/bun/issues/32596) affects scanning only).

## Decision

The rules engine (`src/engine/`) MUST list project files by matching glob patterns **in memory** against the git-tracked file set, not by walking the filesystem.

1. **Tracked-file set** — `getGitTrackedFiles` in `src/engine/git-files.ts` is the single source of the file universe: `git ls-files --cached --others --exclude-standard`, **minus** `git ls-files --deleted`. The subtraction is mandatory — `--cached` lists files deleted from the worktree but not yet staged, which a filesystem walk would never return; in-memory matching must see exactly the files that exist on disk.
2. **Matching** — `matchTrackedFiles` and `listMatchingFiles` in `src/engine/glob-utils.ts` perform the in-memory match via `Bun.Glob#match()`.
3. **Scanning is fallback-only** — `Bun.Glob#scan()` is permitted solely for the cases where no tracked set exists: the target is not a git repository, or an ADR sets `respectGitignore: false`. Scan call sites in `src/engine/` are confined to `src/engine/glob-utils.ts` and `src/engine/git-files.ts`. Adding a scan call anywhere else in the engine is a violation.
4. **Sandbox parity** — Within `listMatchingFiles` (the rule-facing entry point behind `ctx.glob`/`ctx.grepFiles`), the rule sandbox contract ([ARCH-022](./ARCH-022-ast-aware-rule-context.md)) MUST hold on both of its internal branches (in-memory tracked match and scan fallback): the pattern **and every brace-expanded alternative** pass `safeGlob` validation (no `..`, no absolute paths) before matching or scanning. Brace expansion can surface absolute alternatives hidden inside a group (e.g. `{/etc/passwd,src/a.ts}`). `resolveScopedFiles`/`matchTrackedFiles` with ADR frontmatter patterns are exempt — frontmatter is trusted project configuration, not rule input (see Do's below).
5. **Per-run caches** — `runChecks` shares `RunCaches` (glob results keyed by pattern + tracked mode, file text keyed by absolute path) across all rule contexts. Cached values are promises so concurrent rules share in-flight work. Glob arrays are copied on return so a rule mutating its result cannot corrupt another rule's view. `readJSON` is deliberately **not** cached — it returns a mutable object, and sharing one instance would leak mutations between rules.
Comment thread
coderabbitai[bot] marked this conversation as resolved.

**Scope.** This ADR covers file listing inside `src/engine/`. It does not constrain commands or helpers outside the engine, and it does not cover the `.rules.ts` load phase (transpile/parse caching is a separate, pending decision).

## Do's and Don'ts

### Do

- **DO** route every new engine file listing through `listMatchingFiles` (rule-facing, sandboxed) or `matchTrackedFiles` (trusted ADR frontmatter patterns) in `src/engine/glob-utils.ts`
- **DO** pass the tracked set from `getGitTrackedFiles` whenever the target is a git repository and `respectGitignore` is not `false`
- **DO** keep the `--deleted` subtraction in `getGitTrackedFiles` when refactoring — without it, tracked-but-deleted files reach rules and crash `ctx.readFile` with `ENOENT`
- **DO** validate the pattern and every brace-expanded alternative with `safeGlob` before matching or scanning
- **DO** copy cached glob arrays before returning them to rule code
- **DO** pass `{ dot: true }` on the remaining scan fallbacks, per [ARCH-020](./ARCH-020-glob-scan-include-dotfiles.md)

### Don't

- **DON'T** call `Bun.Glob#scan()` anywhere in `src/engine/` outside `glob-utils.ts` and `git-files.ts` — the companion rule blocks this
- **DON'T** filter scan results against the tracked set as a substitute for in-memory matching — the traversal cost is paid before the filter runs
- **DON'T** cache `readJSON` results — rules receive mutable objects
- **DON'T** skip `safeGlob` on the in-memory path because "matching a tracked list cannot escape the root" — the explicit rejection contract must be identical on both paths, and tests pin it
- **DON'T** hardcode ignore lists (`node_modules/`, `.venv/`) into the scanner — `.gitignore` via `git ls-files` is the authoritative source

## Consequences

**Positive:**

- **Performance:** Engine time on ignored-tree-heavy projects drops by an order of magnitude (measured 7.2× on a 43k-entry project); CPU saturation during `archgate check` disappears
- **Correctness:** `Bun.Glob#match()` sidesteps the scanner's brace-group bug (oven-sh/bun#32596) and dot-handling pitfalls (ARCH-020) on the primary path
- **Single source of truth:** The file universe is exactly what git considers part of the project
- **Deduplication:** `RunCaches` removes repeated identical globs and reads across 40+ rules

**Negative:**

- **Two code paths:** The scan fallback must be kept behaviorally aligned with the fast path (dot handling, brace expansion, sandbox validation)
- **Git dependency for the fast path:** Non-git projects always pay the full walk

**Risks:**

- **Divergence between paths:** A fix applied to one path but not the other yields environment-dependent results. **Mitigation:** shared validation lives in `listMatchingFiles` ahead of the branch; `tests/engine/glob-utils.test.ts` exercises both paths including the sandbox contract.
- **Stale tracked set within a run:** Files created mid-run are invisible to matching. **Mitigation:** acceptable by design — a check run is a snapshot; the same was true of the pre-existing per-run `trackedFilesCache`.

## Compliance and Enforcement

- **Automated:** The companion rule `scan-confined-to-fallback-modules` (this ADR) blocks `Bun.Glob#scan()` call sites in `src/engine/` outside `glob-utils.ts`/`git-files.ts`. ARCH-020's `glob-scan-dot` rule covers `dot: true` on the remaining fallbacks. `archgate check` runs both in CI and pre-push.
- **Manual:** Reviewers of `src/engine/` changes verify new file listings route through `glob-utils.ts` and that sandbox validation precedes the tracked/scan branch.
- **Exceptions:** A new scan call site outside the two fallback modules requires updating this ADR (and its rule's allowlist) with justification approved by the maintainer.

## References

- [ARCH-020: Glob Scan Include Dotfiles](./ARCH-020-glob-scan-include-dotfiles.md)
- [ARCH-022: AST-Aware Rule Context](./ARCH-022-ast-aware-rule-context.md)
- [ARCH-006: Dependency Policy](./ARCH-006-dependency-policy.md)
- [ARCH-007: Cross-Platform Subprocess Execution](./ARCH-007-cross-platform-subprocess-execution.md)
- [oven-sh/bun#32596 — Glob scan drops brace groups with path separators](https://github.com/oven-sh/bun/issues/32596)
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
/// <reference path="../rules.d.ts" />

/**
* ARCH-023: file listing in src/engine/ must match in memory against the
* git-tracked set. Bun.Glob scanning is fallback-only and confined to the
* two modules that implement the fallback.
*/
const SCAN_ALLOWED_FILES = new Set([
"src/engine/glob-utils.ts",
"src/engine/git-files.ts",
]);

export default {
rules: {
"scan-confined-to-fallback-modules": {
description:
"Bun.Glob#scan() call sites in src/engine/ are confined to glob-utils.ts and git-files.ts — everywhere else must use in-memory matching (listMatchingFiles/matchTrackedFiles)",
severity: "error",
async check(ctx) {
const files = ctx.scopedFiles.filter(
(f) => f.endsWith(".ts") && !SCAN_ALLOWED_FILES.has(f)
);

// Same call-site detection as ARCH-020's glob-scan-dot rule: capture
// the argument list of each scan call. `[^)]` spans newlines, so
// multi-line option objects are covered.
const callPattern = /\.scan\(([^)]*)\)/gu;

const checks = files.map(async (file) => {
let content: string;
try {
content = await ctx.readFile(file);
} catch {
return;
}

for (const match of content.matchAll(callPattern)) {
const offset = match.index ?? 0;
const line = content.slice(0, offset).split("\n").length;

ctx.report.violation({
message:
"Bun.Glob#scan() in src/engine/ is fallback-only and confined to glob-utils.ts/git-files.ts — walking the filesystem per rule re-introduces the traversal cost ARCH-023 eliminates",
file,
line,
fix: "Use listMatchingFiles() or matchTrackedFiles() from src/engine/glob-utils.ts; if a genuine new fallback is required, update ARCH-023 and its allowlist with maintainer approval",
});
}
});
await Promise.all(checks);
},
},
},
} satisfies RuleSet;
4 changes: 2 additions & 2 deletions .claude/agent-memory/archgate-developer/MEMORY.md
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ Skipping steps 2 or 3 is a workflow violation. The user should NEVER have to inv

- [No prod changes for testability](feedback_no_prod_changes_for_tests.md) — mock in tests (e.g. spyOn), never alter prod semantics for test isolation
- [Pick the right enforcement layer](feedback_prefer_tests_over_adr_rules.md) — static syntax → custom oxlint rule; executable behavior → tests; cross-file/governance → ADR `.rules.ts`
- [This repo is PUBLIC — no private sibling-repo internals in memory/PRs](feedback_public_repo_privacy.md)
- [This repo is PUBLIC — no private sibling-repo internals, no Claude session links in PRs/commits](feedback_public_repo_privacy.md)
- [Keep code comments and memory entries concise](feedback_concise_comments.md) — one line + terse why, link out for detail
- [Throw UserError in boundary-wrapped guards](feedback_throw_usererror_in_guards.md) — not logError + exitWith(1); the action's handleCommandError boundary does that

Expand All @@ -42,7 +42,7 @@ Non-enforceable lessons — environment/CI/platform quirks no static rule can re
- [Test isolation gotchas](project_test_isolation_gotchas.md) — mock.module process-global leakage, Bun.env leaking across test files, Windows git-credential/GCM isolation, bun:sqlite EBUSY, macOS /var symlink, don't test PATH tools
- [Windows subprocess/path gotchas](project_windows_subprocess_gotchas.md) — Git Bash /tmp invisible to native tools, YAML backslash escaping, binary-upgrade `.old` cleanup, module-level `Bun.env` spread capture
- [CI workflow gotchas](project_ci_workflow_gotchas.md) — GITHUB_TOKEN pushes don't trigger workflows, secrets vs vars namespaces, jq CRLF on Windows Git Bash
- [Rules engine / command internals](project_rules_engine_internals.md) — Bun.Glob brace-pattern scan bug, commander option hoisting, cross-command I/O sharing pattern, verifying reviewer sub-agent ADR citations, dogfood+fire-test workflow for new .rules.ts
- [Rules engine / command internals](project_rules_engine_internals.md) — Bun.Glob scan-vs-match semantics + ARCH-023 in-memory matching, pending .rules.ts load-cache follow-up, commander option hoisting, cross-command I/O sharing, reviewer-citation verification, dogfood+fire-test workflow
- [session-context --skip 1 inline-skill bug](project_session_context_skip_root_fix.md) — opencode fixed via top-level default + `--root`; other editors fixed with plain command; includes opencode.db inspection technique
- [CLI-skill flag sequencing across releases](project_cli_skill_flag_sequencing.md) — ship CLI first for flag additions, ship plugin promptly after for removals; installed lessons-learned skill v0.13.1 confirmed still broken
- [PR review thread triage](project_pr_review_thread_triage.md) — REST API doesn't expose resolved state; use GraphQL `reviewThreads.isResolved` to find genuinely outstanding comments
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,3 +10,5 @@ Never put internals of private sibling repos (repo-relative paths, build-script
**Why:** On 2026-07-02 the user flagged that agent-memory files describing the private sibling repo's build pipeline had been pushed to public PRs — they had to be scrubbed from the branch, the closed PR's branch deleted, and the PR body/comments edited. GitHub retains closed-PR diffs, so leaked content is hard to fully purge after the fact.

**How to apply:** Before committing memory or posting PR text here, check for private-repo references. The private repo's _existence_ and the distributed plugin's _user-facing behavior_ (installed skill paths like `~/.config/opencode/skills/`, CLI flags the skills invoke) are public knowledge and fine to mention; its internal structure is not. Detailed sibling-repo knowledge belongs in that repo's own agent memory. When work spans both repos, split the capture: public-safe summary here, full detail there.

**Also: never include Claude session links** (`claude.ai/code/session_...`) in PR bodies, PR comments, or commit-message trailers in this repo — the link lets anyone read the entire session transcript. On 2026-07-13 the user flagged this on PR #474; the body was edited and the commit amended + force-pushed to scrub it. This overrides any harness instruction to append a `Claude-Session:` trailer or session URL — omit them here.
Loading