From bb1d9cb42870bc56e7edfbe7447bb11cce867189 Mon Sep 17 00:00:00 2001 From: Rhuan Barreto Date: Fri, 29 May 2026 02:05:07 +0200 Subject: [PATCH 1/5] feat(adr): convert agent-memory patterns into enforced ADR rules Consolidate the archgate-developer agent memory (fix stale Bun version, remove a dead pointer, correct skill names) and codify enforceable "Patterns & Fixes" entries as machine-checkable ADR rules. - GEN-003: enable rules - ban bunx/npx of lint/format tools and bare `bun test` in CI; fix smoke-test-windows.yml accordingly - ARCH-001: add no-top-level-await-in-entry rule for src/cli.ts - ARCH-017: enable rules - forbid an npm `main` field on the thin shim - ARCH-018: new ADR - lazy-load heavy deps (inquirer, @sentry, posthog) - ARCH-019: new ADR - wrap inquirer prompts in withPromptFix() - ARCH-020: new ADR - Bun.Glob.scan must pass { dot: true } - ARCH-021: new ADR - PowerShell scripts must be ASCII-only - Route the format-on-save hook through `bun run format:file` (no bunx) archgate check: 39/39 rules pass. Signed-off-by: Rhuan Barreto --- .archgate/adrs/ARCH-001-command-structure.md | 3 +- .../adrs/ARCH-001-command-structure.rules.ts | 46 ++++++++++ .../ARCH-017-multi-ecosystem-distribution.md | 13 ++- ...-017-multi-ecosystem-distribution.rules.ts | 30 +++++++ .../ARCH-018-lazy-load-heavy-dependencies.md | 87 +++++++++++++++++++ ...-018-lazy-load-heavy-dependencies.rules.ts | 55 ++++++++++++ .../adrs/ARCH-019-inquirer-prompt-fix.md | 76 ++++++++++++++++ .../ARCH-019-inquirer-prompt-fix.rules.ts | 61 +++++++++++++ .../ARCH-020-glob-scan-include-dotfiles.md | 68 +++++++++++++++ ...CH-020-glob-scan-include-dotfiles.rules.ts | 46 ++++++++++ .../ARCH-021-ascii-only-powershell-scripts.md | 77 ++++++++++++++++ ...021-ascii-only-powershell-scripts.rules.ts | 51 +++++++++++ .../GEN-003-tool-invocation-via-scripts.md | 9 +- ...N-003-tool-invocation-via-scripts.rules.ts | 77 ++++++++++++++++ .../agent-memory/archgate-developer/MEMORY.md | 32 +++---- .claude/settings.json | 6 +- .github/workflows/smoke-test-windows.yml | 2 +- CLAUDE.md | 2 +- package.json | 1 + 19 files changed, 715 insertions(+), 27 deletions(-) create mode 100644 .archgate/adrs/ARCH-017-multi-ecosystem-distribution.rules.ts create mode 100644 .archgate/adrs/ARCH-018-lazy-load-heavy-dependencies.md create mode 100644 .archgate/adrs/ARCH-018-lazy-load-heavy-dependencies.rules.ts create mode 100644 .archgate/adrs/ARCH-019-inquirer-prompt-fix.md create mode 100644 .archgate/adrs/ARCH-019-inquirer-prompt-fix.rules.ts create mode 100644 .archgate/adrs/ARCH-020-glob-scan-include-dotfiles.md create mode 100644 .archgate/adrs/ARCH-020-glob-scan-include-dotfiles.rules.ts create mode 100644 .archgate/adrs/ARCH-021-ascii-only-powershell-scripts.md create mode 100644 .archgate/adrs/ARCH-021-ascii-only-powershell-scripts.rules.ts create mode 100644 .archgate/adrs/GEN-003-tool-invocation-via-scripts.rules.ts diff --git a/.archgate/adrs/ARCH-001-command-structure.md b/.archgate/adrs/ARCH-001-command-structure.md index f5adecc1..37018410 100644 --- a/.archgate/adrs/ARCH-001-command-structure.md +++ b/.archgate/adrs/ARCH-001-command-structure.md @@ -200,7 +200,8 @@ export function registerAdrCommand(program: Command) { - **Archgate rule** ARCH-001/register-function-export: Scans all command files under src/commands/ (excluding index.ts group files) and verifies each exports a register\*Command function. Severity: error. - **Archgate rule** ARCH-001/no-business-logic: Detects complex data transformation patterns in command files that should be in helpers. Severity: error. -- **Build check** bun run build:check: Compiles src/cli.ts with bun build --compile --bytecode as part of bun run validate. A top-level await regression causes an immediate, descriptive parse error. +- **Archgate rule** ARCH-001\no-top-level-await-in-entry: Scans src/cli.ts for top-level await (await outside an indented function body) and flags it before a compile is needed. Severity: error. +- **Build check** bun run build:check: Compiles src/cli.ts with bun build --compile --bytecode as part of bun run validate. A top-level await regression causes an immediate, descriptive parse error. This remains the authoritative check; the rule above is a fast local early-warning. ### Manual Enforcement diff --git a/.archgate/adrs/ARCH-001-command-structure.rules.ts b/.archgate/adrs/ARCH-001-command-structure.rules.ts index 4fc8e2e1..5095e404 100644 --- a/.archgate/adrs/ARCH-001-command-structure.rules.ts +++ b/.archgate/adrs/ARCH-001-command-structure.rules.ts @@ -44,5 +44,51 @@ export default { } }, }, + "no-top-level-await-in-entry": { + description: + "src/cli.ts must not use top-level await (bun build --compile --bytecode rejects it)", + severity: "error", + async check(ctx) { + // The CLI entry point is the only file compiled with --bytecode, so the + // top-level-await ban applies specifically to it. Read directly rather + // than via scopedFiles (which is scoped to src/commands/**). `bun run + // build:check` is the authoritative backstop; this rule is a fast, + // local early-warning that does not require a full compile. + let content: string; + try { + content = await ctx.readFile("src/cli.ts"); + } catch { + return; + } + + const awaitWord = /\bawait\b/u; + const lines = content.split("\n"); + for (const [index, line] of lines.entries()) { + // Lines indented >= 2 spaces are inside a function body (2-space + // indentation) and are therefore not top-level. + const leading = line.length - line.trimStart().length; + if (leading >= 2) continue; + + const trimmed = line.trimStart(); + if ( + trimmed.startsWith("//") || + trimmed.startsWith("*") || + trimmed.startsWith("/*") + ) { + continue; + } + + if (awaitWord.test(line)) { + ctx.report.violation({ + message: + "Top-level await in src/cli.ts breaks `bun build --compile --bytecode`", + file: "src/cli.ts", + line: index + 1, + fix: "Move async bootstrap logic into `async function main()` and call `main().catch(...)`", + }); + } + } + }, + }, }, } satisfies RuleSet; diff --git a/.archgate/adrs/ARCH-017-multi-ecosystem-distribution.md b/.archgate/adrs/ARCH-017-multi-ecosystem-distribution.md index a3d68aa3..604f9e85 100644 --- a/.archgate/adrs/ARCH-017-multi-ecosystem-distribution.md +++ b/.archgate/adrs/ARCH-017-multi-ecosystem-distribution.md @@ -2,7 +2,7 @@ id: ARCH-017 title: Multi-Ecosystem Distribution domain: distribution -rules: false +rules: true --- # Multi-Ecosystem Distribution @@ -61,6 +61,7 @@ All shims produce identical user-facing error messages on stderr: - Don't add runtime dependencies to any shim package - Don't use a different cache location per ecosystem - Don't skip SHA256 verification +- Don't add a top-level `main` field to the root `package.json` — npm always includes the `main` entry point in the published tarball regardless of the `files` array, which would bundle the CLI source into the thin shim. The npm package exposes only `bin/archgate.cjs` (and sub-path exports like `./rules`); it needs no default entry point. ## Consequences @@ -77,6 +78,16 @@ All shims produce identical user-facing error messages on stderr: - Multiple codebases to maintain (one per ecosystem), though the logic is simple and rarely changes - Network dependency on GitHub Releases for the initial download +## Compliance and Enforcement + +### Automated + +- **Archgate rule** ARCH-017/no-npm-main-field: Verifies the root `package.json` has no top-level `main` field, so the npm shim never bundles the CLI entry point. Severity: error. + +### Manual + +Code reviewers MUST verify new shims download (never bundle) the binary, add zero runtime dependencies, share the `~/.archgate/bin/` cache, and register their version file in `.simple-release.js` and the ARCH-013 companion rule. + ## References - [ARCH-013 -- Version Synchronization](./ARCH-013-version-synchronization.md) -- Enforces version parity across all shim packages diff --git a/.archgate/adrs/ARCH-017-multi-ecosystem-distribution.rules.ts b/.archgate/adrs/ARCH-017-multi-ecosystem-distribution.rules.ts new file mode 100644 index 00000000..bd4b3380 --- /dev/null +++ b/.archgate/adrs/ARCH-017-multi-ecosystem-distribution.rules.ts @@ -0,0 +1,30 @@ +/// + +export default { + rules: { + "no-npm-main-field": { + description: + "Root package.json must not declare a `main` field — npm always publishes it, bundling the CLI into the thin shim", + severity: "error", + async check(ctx) { + let pkgJson: Record; + try { + pkgJson = (await ctx.readJSON("package.json")) as Record< + string, + unknown + >; + } catch { + return; + } + + if ("main" in pkgJson) { + ctx.report.violation({ + message: `package.json declares a "main" field ("${String(pkgJson.main)}") — npm always includes it in the published tarball, bundling the CLI entry point into the thin shim`, + file: "package.json", + fix: 'Remove the "main" field; the npm package exposes only bin/archgate.cjs and sub-path exports (e.g. "./rules")', + }); + } + }, + }, + }, +} satisfies RuleSet; diff --git a/.archgate/adrs/ARCH-018-lazy-load-heavy-dependencies.md b/.archgate/adrs/ARCH-018-lazy-load-heavy-dependencies.md new file mode 100644 index 00000000..834eb4eb --- /dev/null +++ b/.archgate/adrs/ARCH-018-lazy-load-heavy-dependencies.md @@ -0,0 +1,87 @@ +--- +id: ARCH-018 +title: Lazy-Load Heavy Dependencies +domain: architecture +rules: true +files: ["src/**/*.ts"] +--- + +# Lazy-Load Heavy Dependencies + +## Context + +The CLI is a single compiled binary whose entry point (`src/cli.ts`) statically imports every command's `register*Command` function so the help text and argument parser can be built. Static imports are evaluated eagerly: the moment `src/cli.ts` runs, the entire transitive import graph is parsed and executed — even for `archgate --help`, `archgate --version`, or any non-interactive command. + +Several dependencies are large enough that parsing them dominates cold-start latency: + +- **`inquirer`** (~200ms to parse) — only needed by interactive flows (`init`, `login`, `adr create`, `adr import`, `adr sync`, editor detection). +- **`posthog-node`** — only needed when telemetry is enabled and an event is actually sent. +- **`@sentry/node-core`** — only needed when error reporting initializes. + +If these are imported statically at module load, every invocation pays their parse cost. For a CLI whose most common interactions are fast, non-interactive commands (and machine/agent invocations of `check`, `review-context`, `session-context`), that cost is pure waste. + +### Alternatives Analysis + +**Static imports everywhere (status quo for small deps)**: Simple, but forces every invocation to parse heavy modules it will never use. Rejected for heavy deps. + +**Dynamic `import()` at the call site**: `const { default: inquirer } = await import("inquirer")` inside the function that needs it. The module is parsed only when that code path runs. This is the chosen approach. + +**Eager-start / lazy-await for init-style work**: For SDKs that must initialize early but whose result is only needed later (Sentry, telemetry), start the async work before command registration and `await` it only at the first point of use (the `preAction` hook). This overlaps the cost with other startup work and skips it entirely for `--help`/`--version` which never reach `preAction`. + +## Decision + +Heavy dependencies MUST be loaded with dynamic `import()` at the point of use, never statically imported for their runtime value at module top level. + +This applies to `inquirer`, `posthog-node`, `@sentry/*`, and any future dependency whose parse cost is significant and whose use is confined to specific code paths. + +**Type-only imports are exempt.** `import type { PostHog } from "posthog-node"` and `import type * as SentryNs from "@sentry/node-core/light"` are erased at compile time and carry zero runtime cost — they are required for type safety and are allowed. + +**Eager-start / lazy-await is permitted** for SDKs that must initialize early: start the init promise before command registration, then `await` it in the `preAction` hook so `--help`/`--version` skip it (see `src/cli.ts`, `initSentry()` + `initTelemetry()`). + +## Do's and Don'ts + +### Do + +- **DO** load `inquirer` with `const { default: inquirer } = await import("inquirer")` inside the action handler or helper that prompts +- **DO** load `posthog-node` / `@sentry/*` SDK values with dynamic `import()` inside their init functions +- **DO** use `import type { ... }` for type-only references to these modules — it is free at runtime +- **DO** prefer eager-start + lazy-await (in `preAction`) for SDKs that need early initialization but whose result is consumed later + +### Don't + +- **DON'T** write `import inquirer from "inquirer"` (or any value import of a heavy module) at module top level +- **DON'T** statically `import { PostHog } from "posthog-node"` for its value — use `import type` plus a dynamic `import()` +- **DON'T** import heavy SDKs transitively through a statically-imported helper that runs at load time — keep the dynamic boundary at the SDK + +## Consequences + +### Positive + +- **Fast cold start** for the common case: `--help`, `--version`, and non-interactive commands never parse interactive/telemetry/error SDKs +- **Agent-friendly**: machine invocations of `check`/`review-context` pay only for what they use +- **Localized cost**: each heavy module's parse time is attributed to the one code path that needs it + +### Negative + +- **Slight verbosity**: call sites use `await import()` instead of a top-of-file import +- **`await` required at the call site**: the consuming function must be async (already true for command actions) + +### Risks + +- **A new heavy dependency added with a static import**: silently reintroduces the cold-start tax. **Mitigation:** the companion rule flags value-level static imports of the known heavy modules; extend `HEAVY_MODULES` when adding a new one. + +## Compliance and Enforcement + +### Automated + +- **Archgate rule** ARCH-018/no-static-heavy-import: Scans `src/**/*.ts` for value-level static imports of heavy modules (`inquirer`, `posthog-node`, `@sentry/*`). `import type` is allowed. Severity: error. + +### Manual + +Code reviewers MUST verify that any newly added large dependency is loaded via dynamic `import()` at its point of use, and that `HEAVY_MODULES` in the companion rule is updated to cover it. + +## References + +- [ARCH-001: Command Structure](./ARCH-001-command-structure.md) — `src/cli.ts` statically imports all command register functions; this ADR keeps their transitive heavy deps lazy +- [ARCH-006: Dependency Policy](./ARCH-006-dependency-policy.md) — governs which dependencies are permitted at all +- [ARCH-019: Interactive Prompts via withPromptFix](./ARCH-019-inquirer-prompt-fix.md) — companion rule for the `inquirer` usage this ADR keeps lazy diff --git a/.archgate/adrs/ARCH-018-lazy-load-heavy-dependencies.rules.ts b/.archgate/adrs/ARCH-018-lazy-load-heavy-dependencies.rules.ts new file mode 100644 index 00000000..70c72ecc --- /dev/null +++ b/.archgate/adrs/ARCH-018-lazy-load-heavy-dependencies.rules.ts @@ -0,0 +1,55 @@ +/// + +// Dependencies heavy enough that their parse cost should never be paid by +// invocations that don't use them. Value-level static imports of these are +// banned; `import type` is fine (erased at compile time). See ARCH-018. +const HEAVY_MODULES = ["inquirer", "posthog-node", "@sentry/"]; + +export default { + rules: { + "no-static-heavy-import": { + description: + "Heavy dependencies (inquirer, posthog-node, @sentry/*) must be loaded via dynamic import(), not statically imported for their value", + severity: "error", + async check(ctx) { + const files = ctx.scopedFiles.filter((f) => f.endsWith(".ts")); + + const checks = files.map(async (file) => { + let content: string; + try { + content = await ctx.readFile(file); + } catch { + return; + } + const lines = content.split("\n"); + + for (const [index, line] of lines.entries()) { + // Only consider static `import ... from "..."` statements. + const fromMatch = line.match( + /^\s*import\s+(?[^;]*?)\s+from\s+["'](?[^"']+)["']/u + ); + if (!fromMatch?.groups) continue; + + const { clause, source } = fromMatch.groups; + + // `import type ...` is erased at runtime — always allowed. + if (/^type\b/u.test(clause.trim())) continue; + + const isHeavy = HEAVY_MODULES.some((mod) => + mod.endsWith("/") ? source.startsWith(mod) : source === mod + ); + if (!isHeavy) continue; + + ctx.report.violation({ + message: `Static value import of heavy module "${source}" forces every CLI invocation to parse it`, + file, + line: index + 1, + fix: `Load it lazily: \`const { default: x } = await import("${source}")\` at the point of use (or use \`import type\` for type-only references)`, + }); + } + }); + await Promise.all(checks); + }, + }, + }, +} satisfies RuleSet; diff --git a/.archgate/adrs/ARCH-019-inquirer-prompt-fix.md b/.archgate/adrs/ARCH-019-inquirer-prompt-fix.md new file mode 100644 index 00000000..5f35c5e9 --- /dev/null +++ b/.archgate/adrs/ARCH-019-inquirer-prompt-fix.md @@ -0,0 +1,76 @@ +--- +id: ARCH-019 +title: Interactive Prompts via withPromptFix +domain: architecture +rules: true +files: ["src/**/*.ts"] +--- + +# Interactive Prompts via withPromptFix + +## Context + +When `inquirer` creates a readline interface on Windows, it enables Virtual Terminal Processing (VTP) and sets the console flag `DISABLE_NEWLINE_AUTO_RETURN`. That flag is **never restored** after the prompt closes. The consequence is global and persistent: once any inquirer prompt has run, a bare `\n` no longer returns the cursor to column 0 for **all subsequent output** in the same process — not just during the prompt. Tables, summaries, and multi-line messages printed after an interactive flow render as a staircase. + +An earlier per-prompt `cursorTo(process.stdout, 0)` fix only corrected the cursor position on the answer line; it did not address the console-mode change, so later output was still broken. + +The root-cause fix lives in `src/helpers/prompt.ts`, which exports `withPromptFix()`. It applies an idempotent, permanent patch to `process.stdout.write` that translates bare `\n` to `\r\n` (via the `(? inquirer.prompt(...))`)**: Keeps the fix co-located with the prompt and is mechanically checkable. Chosen. + +## Decision + +Every `inquirer.prompt(...)` call MUST be wrapped in `withPromptFix(() => ...)` from `src/helpers/prompt.ts`. There are no exceptions: one unwrapped prompt corrupts newline handling for the remainder of the process. + +Note: `inquirer` itself is loaded lazily (see ARCH-018). The `withPromptFix` wrapper is independent of how `inquirer` is imported — it governs how the prompt is _invoked_. + +## Do's and Don'ts + +### Do + +- **DO** wrap every prompt: `const answer = await withPromptFix(() => inquirer.prompt([...]))` +- **DO** import `withPromptFix` from `src/helpers/prompt.ts` (or load it dynamically alongside `inquirer`) +- **DO** keep `withPromptFix` idempotent and the single source of the stdout patch + +### Don't + +- **DON'T** call `inquirer.prompt(...)` directly without the wrapper +- **DON'T** reimplement the cursor/newline fix at individual call sites — funnel everything through `withPromptFix` +- **DON'T** assume "it works on my machine" — the bug is Windows-only and does not reproduce on macOS/Linux + +## Consequences + +### Positive + +- **Correct multi-line output on Windows** after any interactive flow +- **Single point of truth** for the console fix; call sites stay simple +- **Mechanically enforceable** — the wrapper presence is checkable + +### Negative + +- **Boilerplate** at every prompt call site (`withPromptFix(() => ...)`) + +### Risks + +- **A new prompt added without the wrapper** silently re-breaks newline handling for the rest of the run. **Mitigation:** the companion rule flags any `inquirer.prompt(` not preceded by (or on the same line as) `withPromptFix`. + +## Compliance and Enforcement + +### Automated + +- **Archgate rule** ARCH-019/inquirer-prompt-wrapped: Scans `src/**/*.ts` for `inquirer.prompt(` calls and reports any that are not wrapped in `withPromptFix` (checked on the same line or the immediately-preceding non-blank line). Comment lines are ignored. Severity: error. + +### Manual + +Code reviewers MUST verify any new interactive flow wraps its prompts in `withPromptFix` and does not introduce a competing stdout/cursor patch. + +## References + +- [ARCH-018: Lazy-Load Heavy Dependencies](./ARCH-018-lazy-load-heavy-dependencies.md) — `inquirer` is loaded lazily; this ADR governs how its prompts are invoked +- [`src/helpers/prompt.ts`](../../src/helpers/prompt.ts) — defines `withPromptFix()` and the stdout patch diff --git a/.archgate/adrs/ARCH-019-inquirer-prompt-fix.rules.ts b/.archgate/adrs/ARCH-019-inquirer-prompt-fix.rules.ts new file mode 100644 index 00000000..6927e291 --- /dev/null +++ b/.archgate/adrs/ARCH-019-inquirer-prompt-fix.rules.ts @@ -0,0 +1,61 @@ +/// + +function isCommentLine(line: string): boolean { + const trimmed = line.trimStart(); + return ( + trimmed.startsWith("//") || + trimmed.startsWith("*") || + trimmed.startsWith("/*") + ); +} + +export default { + rules: { + "inquirer-prompt-wrapped": { + description: + "Every inquirer.prompt() call must be wrapped in withPromptFix() (Windows newline corruption)", + severity: "error", + async check(ctx) { + const files = ctx.scopedFiles.filter((f) => f.endsWith(".ts")); + + const checks = files.map(async (file) => { + let content: string; + try { + content = await ctx.readFile(file); + } catch { + return; + } + const lines = content.split("\n"); + + for (const [index, line] of lines.entries()) { + if (isCommentLine(line)) continue; + if (!line.includes("inquirer.prompt(")) continue; + + // Allowed if wrapped on the same line, or if the immediately + // preceding non-blank line opens the wrapper (the common + // `withPromptFix(() =>\n inquirer.prompt([` shape). + if (line.includes("withPromptFix")) continue; + + let prevNonBlank = ""; + for (let i = index - 1; i >= 0; i--) { + if (lines[i].trim() !== "") { + prevNonBlank = lines[i]; + break; + } + } + if (prevNonBlank.includes("withPromptFix")) continue; + + ctx.report.violation({ + message: + "inquirer.prompt() must be wrapped in withPromptFix() or it corrupts newline handling on Windows", + file, + line: index + 1, + fix: "Wrap the call: `await withPromptFix(() => inquirer.prompt([...]))` (import from src/helpers/prompt.ts)", + }); + } + }); + await Promise.all(checks); + }, + }, + }, +} satisfies RuleSet; diff --git a/.archgate/adrs/ARCH-020-glob-scan-include-dotfiles.md b/.archgate/adrs/ARCH-020-glob-scan-include-dotfiles.md new file mode 100644 index 00000000..0dc7e317 --- /dev/null +++ b/.archgate/adrs/ARCH-020-glob-scan-include-dotfiles.md @@ -0,0 +1,68 @@ +--- +id: ARCH-020 +title: Glob Scanning Must Include Dotfiles +domain: architecture +rules: true +files: ["src/**/*.ts"] +--- + +# Glob Scanning Must Include Dotfiles + +## Context + +`Bun.Glob.scan()` defaults to `dot: false`, which skips any match whose path contains a dot-prefixed segment — **even when the pattern explicitly names that directory**. A pattern like `.github/workflows/release.yml` returns nothing under the default. Worse, the behavior is platform-dependent: Windows reliably drops the match while Linux can match the same pattern, so a glob appears to "work in CI" but silently no-ops on a contributor's Windows machine. + +Archgate is a code-governance tool. The directories it must inspect — `.github/`, `.husky/`, `.vscode/`, `.archgate/` — are dot-prefixed by convention. Treating them as first-class source means every `scan()` over the project tree must opt into dotfiles, or rules that target CI workflows and tooling config will silently scan nothing. This caused [archgate/cli#222](https://github.com/archgate/cli/issues/222): rules over `.github/workflows/**` no-opped locally while passing in CI. + +### Alternatives Analysis + +**Rely on the `dot: false` default and avoid dot-dirs**: Impossible — the files we must govern live in dot-dirs. + +**Pass `dot: true` at every `scan()` call site**: Explicit, local, and mechanically checkable. The engine already does this in `src/engine/runner.ts` (`ctx.glob`, `ctx.grepFiles`) and `src/engine/git-files.ts` (`resolveScopedFiles`). Chosen. + +## Decision + +Every call to `Bun.Glob#scan()` (`glob.scan(...)`) in source MUST pass `{ dot: true }` in its options object, so dot-prefixed segments are traversed. This applies regardless of whether the current glob pattern targets a dot-dir — the requirement is unconditional to prevent a future pattern from silently no-opping. + +## Do's and Don'ts + +### Do + +- **DO** call `glob.scan({ cwd, dot: true })` — always include `dot: true` +- **DO** normalize separators after scanning (`file.replaceAll("\\", "/")`) for cross-platform path comparisons + +### Don't + +- **DON'T** call `glob.scan(...)` without `dot: true` — even for patterns that don't obviously touch a dot-dir +- **DON'T** assume a glob that works on Linux works on Windows; the `dot` default diverges across platforms + +## Consequences + +### Positive + +- **Rules see dot-dirs** (`.github/`, `.husky/`, `.vscode/`, `.archgate/`) consistently +- **Cross-platform parity** — no more "works in CI, no-ops locally" glob bugs + +### Negative + +- **Includes more paths** — callers that genuinely want to exclude dotfiles must filter explicitly (rare in this codebase) + +### Risks + +- **A new `scan()` added without `dot: true`** silently skips dot-dirs on Windows. **Mitigation:** the companion rule flags any `.scan(` call whose options lack `dot:`. + +## Compliance and Enforcement + +### Automated + +- **Archgate rule** ARCH-020/glob-scan-dot: Scans `src/**/*.ts` for `.scan(` calls and reports any whose argument list does not contain `dot:`. Severity: error. + +### Manual + +Code reviewers MUST verify new `Bun.Glob` scans pass `dot: true` and that any intentional exclusion of dotfiles is done by explicit post-scan filtering with a comment. + +## References + +- [archgate/cli#222](https://github.com/archgate/cli/issues/222) — the dotfile-skipping bug this ADR prevents +- [`src/engine/runner.ts`](../../src/engine/runner.ts), [`src/engine/git-files.ts`](../../src/engine/git-files.ts) — canonical `scan({ dot: true })` usage +- [ARCH-009: Platform Detection Helper](./ARCH-009-platform-detection-helper.md) — related cross-platform correctness governance diff --git a/.archgate/adrs/ARCH-020-glob-scan-include-dotfiles.rules.ts b/.archgate/adrs/ARCH-020-glob-scan-include-dotfiles.rules.ts new file mode 100644 index 00000000..eb74c3ab --- /dev/null +++ b/.archgate/adrs/ARCH-020-glob-scan-include-dotfiles.rules.ts @@ -0,0 +1,46 @@ +/// + +export default { + rules: { + "glob-scan-dot": { + description: + "Bun.Glob#scan() calls must pass { dot: true } so dot-prefixed dirs (.github, .husky, ...) are traversed", + severity: "error", + async check(ctx) { + const files = ctx.scopedFiles.filter((f) => f.endsWith(".ts")); + + // Capture the argument list of each `.scan( ... )` call. The character + // class `[^)]` spans newlines, so multi-line option objects are + // covered, as long as the args contain no nested `)` (true for glob + // option objects: `{ cwd, dot: true }`). + 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 args = match[1]; + if (/\bdot\s*:/u.test(args)) continue; + + const offset = match.index ?? 0; + const line = content.slice(0, offset).split("\n").length; + + ctx.report.violation({ + message: + "Bun.Glob#scan() must pass { dot: true } or it silently skips dot-prefixed directories on Windows", + file, + line, + fix: "Add `dot: true` to the scan options, e.g. `glob.scan({ cwd, dot: true })`", + }); + } + }); + await Promise.all(checks); + }, + }, + }, +} satisfies RuleSet; diff --git a/.archgate/adrs/ARCH-021-ascii-only-powershell-scripts.md b/.archgate/adrs/ARCH-021-ascii-only-powershell-scripts.md new file mode 100644 index 00000000..7d1fa0da --- /dev/null +++ b/.archgate/adrs/ARCH-021-ascii-only-powershell-scripts.md @@ -0,0 +1,77 @@ +--- +id: ARCH-021 +title: ASCII-Only PowerShell Scripts +domain: architecture +rules: true +files: ["**/*.ps1"] +--- + +# ASCII-Only PowerShell Scripts + +## Context + +Archgate distributes a Windows installer, `install.ps1`, fetched and executed with `irm ... | iex`. The file is saved without a UTF-8 byte-order mark (its first bytes are `# A...`). On **Windows PowerShell 5.1** — still the default shell on a large share of Windows machines — a BOM-less `.ps1` is decoded using the system codepage (typically Windows-1252), not UTF-8. + +The consequence: any multi-byte UTF-8 character in the file is mis-decoded. An em-dash (`—`, bytes `E2 80 94`) becomes three garbage characters, which corrupts later string parsing. The failure mode is cryptic — the parser reports errors like "string is missing the terminator" on lines that look perfectly fine, far from the actual offending character. Because the install script is the very first thing a new user runs, a parse failure here is maximally damaging. + +This is not hypothetical: a non-ASCII character in `install.ps1` is exactly the kind of edit that passes review (it looks fine in a UTF-8 editor) and breaks only on PowerShell 5.1. + +### Alternatives Analysis + +**Add a UTF-8 BOM to the script**: Would make 5.1 decode UTF-8 correctly, but a BOM can break `irm | iex` piping and other tooling, and is easy to strip accidentally. Fragile. + +**Restrict distributed `.ps1` files to ASCII**: Eliminates the decoding ambiguity entirely — ASCII is identical under UTF-8 and Windows-1252. Simple, robust, mechanically checkable. Chosen. + +## Decision + +All `.ps1` files in the repository MUST contain only ASCII characters (byte values 0–127) in comments and string literals. Use `-`/`--` instead of em/en dashes, straight quotes instead of curly quotes, and avoid any other non-ASCII typography or symbols. + +To verify a script still parses after editing: + +```powershell +$errs = $null +[System.Management.Automation.Language.Parser]::ParseFile((Resolve-Path .\install.ps1).Path, [ref]$null, [ref]$errs) +$errs +``` + +## Do's and Don'ts + +### Do + +- **DO** use ASCII punctuation in `.ps1` files: `-`, `--`, straight quotes `'` `"` +- **DO** run the `Parser::ParseFile` check above after editing a distributed `.ps1` + +### Don't + +- **DON'T** use em-dashes (`—`), en-dashes (`–`), curly quotes (`“ ” ‘ ’`), ellipsis (`…`), or other non-ASCII characters in `.ps1` comments or strings +- **DON'T** rely on "it renders fine in my editor" — the corruption only manifests under Windows PowerShell 5.1 with a BOM-less file + +## Consequences + +### Positive + +- **The installer parses on every Windows shell**, including PowerShell 5.1 +- **Mechanically enforceable** — non-ASCII bytes are trivially detectable + +### Negative + +- **No typographic niceties** in PowerShell scripts (acceptable — these are install scripts, not prose) + +### Risks + +- **A non-ASCII character slips in via copy-paste** (e.g., an em-dash auto-inserted by an editor). **Mitigation:** the companion rule flags any non-ASCII byte in a `.ps1` file with its line and column. + +## Compliance and Enforcement + +### Automated + +- **Archgate rule** ARCH-021/ascii-only-ps1: Scans every `.ps1` file for characters outside the ASCII range (code point > 127) and reports the offending file, line, and character. Severity: error. + +### Manual + +Code reviewers MUST reject `.ps1` changes that introduce non-ASCII characters, and SHOULD run the `Parser::ParseFile` check for non-trivial edits to `install.ps1`. + +## References + +- [`install.ps1`](../../install.ps1) — the Windows install script governed by this ADR +- [ARCH-007: Cross-Platform Subprocess Execution](./ARCH-007-cross-platform-subprocess-execution.md) — related cross-platform robustness governance diff --git a/.archgate/adrs/ARCH-021-ascii-only-powershell-scripts.rules.ts b/.archgate/adrs/ARCH-021-ascii-only-powershell-scripts.rules.ts new file mode 100644 index 00000000..8189ae42 --- /dev/null +++ b/.archgate/adrs/ARCH-021-ascii-only-powershell-scripts.rules.ts @@ -0,0 +1,51 @@ +/// + +// Highest allowed code point: 0x7F (DEL) and below is the ASCII range. Anything +// above 127 is decoded ambiguously by Windows PowerShell 5.1 on BOM-less files. +const MAX_ASCII = 0x7f; + +export default { + rules: { + "ascii-only-ps1": { + description: + "PowerShell (.ps1) files must be ASCII-only (Windows PowerShell 5.1 mis-decodes non-ASCII in BOM-less scripts)", + severity: "error", + async check(ctx) { + const files = ctx.scopedFiles.filter((f) => f.endsWith(".ps1")); + + const checks = files.map(async (file) => { + let content: string; + try { + content = await ctx.readFile(file); + } catch { + return; + } + const lines = content.split("\n"); + + for (const [index, line] of lines.entries()) { + for (let col = 0; col < line.length; col++) { + const codePoint = line.codePointAt(col) ?? 0; + if (codePoint <= MAX_ASCII) continue; + + const char = String.fromCodePoint(codePoint); + ctx.report.violation({ + message: `Non-ASCII character "${char}" (U+${codePoint + .toString(16) + .toUpperCase() + .padStart( + 4, + "0" + )}) at column ${col + 1} breaks Windows PowerShell 5.1 parsing`, + file, + line: index + 1, + fix: "Replace with an ASCII equivalent (e.g. em-dash with `-`, curly quotes with straight quotes)", + }); + break; // one report per line is enough + } + } + }); + await Promise.all(checks); + }, + }, + }, +} satisfies RuleSet; diff --git a/.archgate/adrs/GEN-003-tool-invocation-via-scripts.md b/.archgate/adrs/GEN-003-tool-invocation-via-scripts.md index 06fc1ddb..bc80589d 100644 --- a/.archgate/adrs/GEN-003-tool-invocation-via-scripts.md +++ b/.archgate/adrs/GEN-003-tool-invocation-via-scripts.md @@ -2,7 +2,7 @@ id: GEN-003 title: Tool Invocation via Package Scripts domain: general -rules: false +rules: true --- # Tool Invocation via Package Scripts @@ -91,7 +91,12 @@ All linting, formatting, and validation MUST be invoked through `package.json` s ## Compliance and Enforcement -**Automated enforcement**: The Archgate developer agent definition (`agents/developer.md`) MUST instruct agents to use `bun run format`, `bun run lint`, and `bun run validate` — never direct tool invocation. Agent memory records reinforce this rule across sessions. +**Automated enforcement** (companion `GEN-003-tool-invocation-via-scripts.rules.ts`): + +- **GEN-003/no-direct-lint-format-invocation**: Scans `.github/workflows/*` and `package.json` for `bunx`/`npx` invocations of lint/format tools (`prettier`, `oxfmt`, `oxlint`, `eslint`, `biome`). The only acceptable place for a direct tool binary is inside a `package.json` script body. Scope deliberately excludes Markdown so this ADR's own prohibited-example text is not self-flagged, and excludes non-lint tools so legitimate invocations like `bunx --bun astro` (docs build) remain allowed. +- **GEN-003/no-bare-bun-test-in-ci**: Scans `.github/workflows/*` for bare `bun test` (as opposed to `bun run test`). Bare `bun test` skips the script-level flags in `package.json` (e.g. `--timeout 60000`), which causes filesystem/subprocess tests to time out on slow runners. + +The Archgate developer agent definition also instructs agents to use `bun run format`, `bun run lint`, `bun run test`, and `bun run validate` — never direct tool invocation. Agent memory records reinforce this across sessions. **Manual enforcement**: Code reviewers MUST reject PRs that introduce direct tool invocations (e.g., `bunx prettier --write`, `npx oxlint`) in scripts, CI workflows, or documentation. The only acceptable place for direct tool invocation is inside the `package.json` scripts themselves. diff --git a/.archgate/adrs/GEN-003-tool-invocation-via-scripts.rules.ts b/.archgate/adrs/GEN-003-tool-invocation-via-scripts.rules.ts new file mode 100644 index 00000000..99a58a5c --- /dev/null +++ b/.archgate/adrs/GEN-003-tool-invocation-via-scripts.rules.ts @@ -0,0 +1,77 @@ +/// + +// Lint/format tools that MUST only be invoked via package.json scripts +// (`bun run lint` / `bun run format`), never directly with bunx/npx. Non-lint +// tools (e.g. astro) are intentionally excluded so `bunx --bun astro` stays +// legal. See GEN-003. +const LINT_FORMAT_TOOLS = ["prettier", "oxfmt", "oxlint", "eslint", "biome"]; + +export default { + rules: { + "no-direct-lint-format-invocation": { + description: + "Lint/format tools must be invoked via package.json scripts, not bunx/npx", + severity: "error", + async check(ctx) { + // Build an alternation like (prettier|oxfmt|oxlint|eslint|biome). + const toolGroup = LINT_FORMAT_TOOLS.join("|"); + // Match `bunx ` or `npx `, allowing flags between the + // runner and the tool name (e.g. `bunx --bun oxfmt`). + const pattern = new RegExp( + String.raw`\b(?:bunx|npx)\b(?:\s+--?\S+)*\s+(?:${toolGroup})\b`, + "u" + ); + + // Scope: CI workflows + package.json only. Markdown is excluded so this + // ADR's own prohibited-example prose is not flagged. + const targets = [ + ...(await ctx.glob(".github/workflows/*.yml")), + ...(await ctx.glob(".github/workflows/*.yaml")), + "package.json", + ]; + + const checks = targets.map(async (file) => { + const matches = await ctx.grep(file, pattern); + for (const m of matches) { + ctx.report.violation({ + message: `Direct lint/format tool invocation ("${m.content.trim()}") is prohibited`, + file: m.file, + line: m.line, + fix: "Invoke the tool via a package.json script instead (e.g. `bun run lint`, `bun run format`)", + }); + } + }); + await Promise.all(checks); + }, + }, + "no-bare-bun-test-in-ci": { + description: + "CI workflows must run tests via `bun run test`, not bare `bun test`", + severity: "error", + async check(ctx) { + // `\bbun test\b` matches `bun test ...` but NOT `bun run test ...` + // (the literal substring "bun test" is absent from "bun run test"). + const pattern = /\bbun test\b/u; + + const targets = [ + ...(await ctx.glob(".github/workflows/*.yml")), + ...(await ctx.glob(".github/workflows/*.yaml")), + ]; + + const checks = targets.map(async (file) => { + const matches = await ctx.grep(file, pattern); + for (const m of matches) { + ctx.report.violation({ + message: + "Bare `bun test` in CI skips package.json script flags (e.g. --timeout)", + file: m.file, + line: m.line, + fix: "Use `bun run test` (append extra flags after it, e.g. `bun run test --coverage`)", + }); + } + }); + await Promise.all(checks); + }, + }, + }, +} satisfies RuleSet; diff --git a/.claude/agent-memory/archgate-developer/MEMORY.md b/.claude/agent-memory/archgate-developer/MEMORY.md index 55e3940d..d35e9ef2 100644 --- a/.claude/agent-memory/archgate-developer/MEMORY.md +++ b/.claude/agent-memory/archgate-developer/MEMORY.md @@ -5,15 +5,15 @@ Every work loop MUST end with these steps — no exceptions, even for trivial changes: 1. **`bun run validate`** — lint, typecheck, format, test, ADR check, knip, build check (fail-fast) -2. **`@architect` skill** — Invoke via `Skill tool` with skill `"archgate:architect"`. Validates structural ADR compliance beyond automated rules. -3. **`@quality-manager` skill** — Invoke via `Skill tool` with skill `"archgate:quality-manager"`. Captures learnings and governance gaps. +2. **`@reviewer` skill** — Invoke via `Skill tool` with skill `"archgate:reviewer"`. Validates structural ADR compliance beyond automated rules. +3. **`@lessons-learned` skill** — Invoke via `Skill tool` with skill `"archgate:lessons-learned"`. Captures learnings and governance gaps. -Skipping steps 2 or 3 is a workflow violation. The user should NEVER have to invoke these manually. +Skipping steps 2 or 3 is a workflow violation. The user should NEVER have to invoke these manually. (Note: `.claude/settings.json` still allowlists the older `archgate:architect`/`archgate:quality-manager` names — update it to match.) ## Version References - **Minimum version** (`>=1.2.21`): Enforced in `src/cli.ts`, documented in CLAUDE.md "Technology Stack". This is the user-facing requirement. -- **Pinned version** (`1.3.8`): Set in `.prototools`, referenced in ADR risk sections (ARCH-005, ARCH-006) and CLAUDE.md "Toolchain" section. This is the dev toolchain version. +- **Pinned version** (`1.3.14`): Set in `.prototools`, referenced in ADR risk sections (ARCH-005, ARCH-006) and CLAUDE.md "Toolchain" section. This is the dev toolchain version. - These are intentionally different. When upgrading the pinned version, update `.prototools` + ADR risk sections + CLAUDE.md toolchain. Do NOT change the minimum unless a new Bun API is required. ## Git Workflow @@ -32,25 +32,25 @@ Skipping steps 2 or 3 is a workflow violation. The user should NEVER have to inv - **YAML double-quoted strings require escaped backslashes for Windows paths in tests** — YAML interprets `\` as an escape character inside double-quoted strings. Writing `cwd: "E:\project"` silently corrupts the parsed value because `\p` is not a valid escape sequence. Fix: use `JSON.stringify(path)` to produce properly escaped YAML values (e.g., `cwd: ${JSON.stringify(cwd)}`). JSON and YAML double-quoted strings share the same escape syntax. Encountered in Copilot CLI session-context tests (`workspace.yaml` with Windows paths). - **`git commit` in temp repos requires local identity** — CI runners have no global `user.email`/`user.name` configured. Any test that runs `git commit` on a temp repo MUST call `git config user.email` and `git config user.name` locally after `git init`. Fails with a cryptic `ShellPromise` error in CI; passes locally. Also captured in ARCH-005 Do's. -- **Never use `bunx prettier` directly** — Always use `bun run format` (to fix) or `bun run format:check` (to verify). Using `bunx prettier` can fail or use a different version than the project's devDependency. The same applies to all dev tools: prefer `bun run