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
4 changes: 3 additions & 1 deletion .archgate/adrs/ARCH-022-ast-aware-rule-context.md
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ This method dispatches internally based on `language`, and the dispatch mechanis
- **DO** resolve the base as the merge base of `--base` and HEAD, matching `changedFiles`' three-dot (`ref...HEAD`) diff, so a rule compares against the same commit as its change set
- **DO** use `ast(path, language, { comments: true })` for comment-governance rules — the returned tree's `comments` array is structured (`type`/`value`/`loc`), replacing fragile line-by-line regex
- **DO** rely on comment `loc` being ORIGINAL-source-accurate even for `"typescript"` (comments are scanned from the pre-transpile source), unlike the tree's own transpiled-relative `loc`
- **DO** mirror every `RuleContext` surface change — methods, properties, and the ambient types they reference — into the generated shim in `src/helpers/rules-shim.ts` in the same change; the `rulecontext-shim-parity` rule fails on member-name drift, and reviewers MUST verify the full signatures and JSDoc match as well

### Don't

Expand Down Expand Up @@ -148,12 +149,13 @@ This method dispatches internally based on `language`, and the dispatch mechanis

### Automated Enforcement

`ctx.ast()` has shipped, and this ADR now carries `rules: true` with four companion checks in `ARCH-022-ast-aware-rule-context.rules.ts`:
`ctx.ast()` has shipped, and this ADR now carries `rules: true` with five companion checks in `ARCH-022-ast-aware-rule-context.rules.ts`:

- **`ast-guardrail-ordering`** — parses `src/engine/runner.ts` via `ctx.ast()` itself (dogfooding the capability this ADR introduces) and verifies the `ast()` method inside `createRuleContext()` invokes the four guardrail markers — `safePath`, `AST_LANGUAGE_EXTENSIONS`, `probeInterpreter`, `runAstSubprocess` — each present and in exactly that order.
- **`no-unsanctioned-engine-subprocess`** — flags any `Bun.spawn`/`Bun.spawnSync` call in `src/engine/` outside the sanctioned helpers (`ast-support.ts` for `ctx.ast()`, `git-files.ts` for git), and bans `child_process` imports in the engine entirely, mirroring how `ARCH-007/no-bun-shell` scans for banned subprocess patterns.
- **`single-ast-method`** — verifies `RuleContext` (in `src/formats/rules.ts` and the generated shim in `src/helpers/rules-shim.ts`) declares exactly one `ast(path, language)` signature and no per-language variants (`pythonAst()`, `rubyAst()`, etc.).
- **`python-subprocess-isolated`** — asserts the Python branch of the guarded invocation in `src/engine/runner.ts` includes the `-I` isolation flag, so a future refactor cannot silently reintroduce the cwd stdlib-shadowing code-execution vector.
- **`rulecontext-shim-parity`** — extracts the member names of the `interface RuleContext { … }` block from `src/formats/rules.ts` and from the generated shim template in `src/helpers/rules-shim.ts` (regex over raw text — `ctx.ast()` cannot see type-only declarations, which `Bun.Transpiler` erases before parsing) and fails on any member present in one surface but missing from the other, in both directions. Added when the `readYAML`/`checkCase` helpers showed the shim mirror is maintained entirely by hand and only the `ast()` signature was previously enforced (`single-ast-method`); a drifted shim silently hands rule authors wrong types. The check is member-name parity — full signature and JSDoc parity remains a manual review item.

The base-revision surface (`ast(path, language, { rev: "base" })` and `fileAtBase()`) is covered by the four rules above unchanged — it adds no new subprocess site and no new guardrail — plus behavioural coverage in `tests/engine/runner-ast-base.test.ts` (base parsing per language, comment-only structural equivalence, and the throw-vs-null semantics of `ast({ rev: "base" })` versus `fileAtBase()`).

Expand Down
63 changes: 63 additions & 0 deletions .archgate/adrs/ARCH-022-ast-aware-rule-context.rules.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,25 @@ const SANCTIONED_SPAWN_FILES = new Set([
"src/engine/git-files.ts", // git subprocess helper, predates ARCH-022
]);

/**
* Member names declared inside an `interface RuleContext { … }` block. Regex
* over raw text, NOT ctx.ast(): Bun.Transpiler erases type-only declarations
* before parsing. Overloads repeat a name, hence the Set. Works on both the
* real interface and the shim's template literal, whose member lines match.
*/
function ruleContextMembers(content: string): Set<string> {
const members = new Set<string>();
const start = content.indexOf("interface RuleContext {");
if (start === -1) return members;
const block = content.slice(start);
const end = block.indexOf("\n}");
const body = end === -1 ? block : block.slice(0, end);
for (const match of body.matchAll(/^ {2}([A-Za-z_$][\w$]*)\??\s*[(:]/gmu)) {
members.add(match[1]);
}
return members;
}

/** Depth-first walk over an ESTree-shaped tree. */
function walk(node: unknown, visit: (n: EsTreeNode) => void): void {
if (Array.isArray(node)) {
Expand Down Expand Up @@ -199,6 +218,50 @@ export default {
}
},
},
"rulecontext-shim-parity": {
description:
"RuleContext members in src/formats/rules.ts and the generated shim in src/helpers/rules-shim.ts must stay in sync — a drifted shim silently hands rule authors wrong types",
severity: "error",
async check(ctx) {
const sourceFile = "src/formats/rules.ts";
const shimFile = "src/helpers/rules-shim.ts";
const [sourceMembers, shimMembers] = await Promise.all([
ctx.readFile(sourceFile).then((c) => ruleContextMembers(c)),
ctx.readFile(shimFile).then((c) => ruleContextMembers(c)),
]);

const surfaces: [string, Set<string>][] = [
[sourceFile, sourceMembers],
[shimFile, shimMembers],
];
for (const [file, members] of surfaces) {
if (members.size > 0) continue;
ctx.report.violation({
message: `Could not locate any members in the \`interface RuleContext\` block of ${file} — parity cannot be verified`,
file,
fix: "Restore the interface RuleContext { … } declaration (2-space-indented members)",
});
}
if (sourceMembers.size === 0 || shimMembers.size === 0) return;

for (const member of sourceMembers) {
if (shimMembers.has(member)) continue;
ctx.report.violation({
message: `RuleContext member "${member}" is declared in ${sourceFile} but missing from the generated shim ${shimFile}`,
file: shimFile,
fix: `Mirror the "${member}" declaration (and any ambient types it references) into the RuleContext block of generateRulesDts() in ${shimFile}`,
});
}
for (const member of shimMembers) {
if (sourceMembers.has(member)) continue;
ctx.report.violation({
message: `RuleContext member "${member}" is declared in the shim ${shimFile} but missing from ${sourceFile}`,
file: sourceFile,
fix: `Add "${member}" to the RuleContext interface in ${sourceFile}, or remove it from the shim`,
});
}
},
},
"single-ast-method": {
description:
"RuleContext exposes exactly one ast(path, language) method — no per-language variants like pythonAst()/rubyAst()",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ metadata:
- **`no-negated-condition`** — write ternaries/`if-else` with the positive condition first: `x === null ? A : B`, not `x !== null ? B : A`.
- **`no-unused-vars` on catch params** — use bare `catch { }` when the error is unused, not `catch (err) { }`.
- **`no-await-in-loop`** — sequential `await` in a `for` loop is flagged; suppress with a reason comment when the sequential order is intentional.
- **`eslint(max-lines)` caps files at 500 code lines; `src/engine/runner.ts` sits at the cap** — implement new `ctx.*` helper logic in its own `src/engine/` module (thin wiring in `createRuleContext`), and remember ARCH-005's `test-mirrors-src` then requires a matching test file. Hit on the readYAML/checkCase PR (safePath trio extracted to `src/engine/safe-path.ts`).
- **ARCH-020's `glob-scan-dot` rule matches `.scan()` inside comments too** (regex `/\.scan\(([^)]*)\)/gu`) — rephrase comments to avoid the literal `.scan()` text.
- **jsPlugins get the full ESLint-compatible `context.sourceCode`** — incl. `getAllComments()`, `getCommentsBefore/After/Inside`, `getJSDocComment`, `lines`, `getLocFromIndex` (verified 2026-07-24). This is what powers the token-based GEN-004 comment rules in `.archgate/lint/concise-comments.ts`; `context.report({loc, message})` works without a node.
- **`*/` inside a JSDoc block terminates it early** — a glob path like `archgate-*/SKILL.md` written in a `/** */` comment breaks the parse; reword the path or use `//` comments.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ metadata:
type: project
---

- **Commit BEFORE fire-testing a rule or guard.** The fire-test loop (mutate a file → confirm the check fails → restore) restores with `git checkout <file>`, which discards ALL uncommitted work in that file — including the fix being tested. Cost a full re-apply of the safePath fix on 2026-07-25. Either commit first, or restore from an explicit `cp` backup rather than git.

- **`Bun.Glob.scan()` silently fails for brace patterns with path separators.** `new Bun.Glob("svc/{src/env.ts,env.ts}").scan(...)` returns zero results (no error) while `.match()` works — the scanner wasn't updated when the match engine's brace expansion was rewritten in Bun 1.2.3. Filed upstream: [oven-sh/bun#32596](https://github.com/oven-sh/bun/issues/32596). Workaround: `expandBracePattern()` (now in `src/engine/glob-utils.ts`) pre-expands `/`-containing brace groups before scanning; only the scan fallback needs it.
- **`Bun.Glob.match()` semantics (verified 2026-07-13, Bun 1.3.14):** matches dot-prefixed path segments WITHOUT any option (`**/*.yml` matches `.github/workflows/ci.yml` — scan needs `dot: true` for the same), handles `/`-containing brace groups correctly, `*` does not cross `/`. This is why in-memory matching (ARCH-023) is both faster and simpler than scanning: engine file listing matches patterns against the `git ls-files` set (minus `--deleted`) in memory; scan is fallback-only (non-git / `respectGitignore: false`), confined to `glob-utils.ts`/`git-files.ts`. Measured 7.2× engine speedup (1147ms→160ms) on a 333-tracked/43k-entry project.
- **Pending follow-up (fix 3): `.rules.ts` load phase re-transpiles + meriyah-parses every rules file per invocation** (~700ms of remaining wall clock incl. Bun startup on the akerbp project). Plan: content-hash cache under `~/.archgate/`. User approved deferring to a separate PR (2026-07-13).
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,5 +12,6 @@ metadata:
- **GCM prompt suppression needs 5 env vars**, not just `GIT_TERMINAL_PROMPT=0`: also `GCM_INTERACTIVE=never`, `GCM_GUI_PROMPT=false`, `GIT_ASKPASS=""`, `SSH_ASKPASS=""` (see `gitCredentialEnv()` in `src/helpers/credential-store.ts`).
- **`bun:sqlite` file handles persist after `db.close()` on Windows.** `rmSync` on the temp dir in `afterEach` can throw `EBUSY`. Set `PRAGMA journal_mode = DELETE` to avoid WAL/SHM files, and wrap `rmSync` in try/catch.
- **macOS `/var` → `/private/var` symlink breaks temp dir path comparisons.** `mkdtempSync` returns `/var/folders/...` but `process.cwd()` after `chdir()` resolves to `/private/var/...`. Always wrap with `realpathSync`. Invisible on ubuntu-only PR CI — only surfaces in release builds.
- **Test DIRECTORY symlinks with `symlinkSync(target, path, "junction")` so the test runs on Windows instead of skipping.** The type arg is ignored on POSIX (plain symlink), but on Windows it creates a junction, which needs no admin rights and which `lstatSync().isSymbolicLink()` reports as `true`. FILE symlinks still need elevation/Developer Mode, so those keep the try/catch-and-return skip. Verified 2026-07-25 on the ARCH-024 ancestor-symlink fix (PR #499) — the existing leaf-symlink test in `check-security.test.ts` had been silently skipping on Windows for its whole life.
- **Don't test that well-known tools exist on PATH** (e.g. `expect(resolveCommand("bun")).toBe("bun")`) — asserts CI environment state, not logic, and fails when tools are installed via shims. Delete such tests; the null-return and `.exe`-fallback tests already cover the real logic.
- **Attach `.catch()` at promise creation, not at `await`.** When firing an async call in parallel (`const p = asyncFn()`) and awaiting later, Bun reports an unhandled rejection in the window between creation and the later `await p.catch(...)`. Fix: `const p = asyncFn().catch(() => null)` then `await p`.
Loading
Loading