diff --git a/.archgate/adrs/ARCH-004-no-barrel-files.rules.ts b/.archgate/adrs/ARCH-004-no-barrel-files.rules.ts index cde2a248..652313eb 100644 --- a/.archgate/adrs/ARCH-004-no-barrel-files.rules.ts +++ b/.archgate/adrs/ARCH-004-no-barrel-files.rules.ts @@ -1,16 +1,11 @@ /// /** - * A Program body is barrel-shaped when every top-level statement is purely - * import/re-export plumbing: - * - ImportDeclaration — `import { x } from "./y"` / `import "./y"` - * - ExportAllDeclaration — `export * from "./y"` - * - ExportNamedDeclaration with — `export { x } from "./y"` / `export { x }` - * declaration === null - * - * Anything else — `export const x = ...`, `export default ...`, function or - * class declarations, expression statements — is executable logic, so the - * file is not a barrel. + * A Program body is barrel-shaped when every top-level statement is pure + * import/re-export plumbing: ImportDeclaration, ExportAllDeclaration, or + * ExportNamedDeclaration with a null declaration. Anything else (`export + * const`, `export default`, function/class declarations, expression + * statements) is executable logic, so the file is not a barrel. */ function isReExportOnlyBody(body: EsTreeNode[]): boolean { return body.every((node) => { @@ -25,16 +20,10 @@ function isReExportOnlyBody(body: EsTreeNode[]): boolean { /** * Fallback for files whose transpiled Program body is EMPTY: ctx.ast() - * transpiles TypeScript before parsing, which erases type-only syntax - * (`export type { X } from "./y"`, `import type ...`), so a pure - * type-re-export barrel parses to an empty Program. Conservatively inspect - * the source: strip comments and blank space, then require every remaining - * statement to start with `import` or `export`. A comment-only/empty file - * is not a barrel. - * - * Handles multi-line statements (e.g. `export type {\n A,\n} from ...`) - * by tracking brace depth — continuation lines inside `{ }` are part of - * the enclosing import/export, not new statements. + * transpiles first, erasing type-only syntax, so a pure type-re-export + * barrel parses to an empty Program. Conservatively require every source + * statement (comments stripped, brace depth tracked across multi-line + * statements) to start with `import` or `export`; empty files are not barrels. */ function isTypeOnlyBarrel(source: string): boolean { const stripped = source diff --git a/.archgate/adrs/ARCH-005-testing-standards.rules.ts b/.archgate/adrs/ARCH-005-testing-standards.rules.ts index 25d15963..184c062f 100644 --- a/.archgate/adrs/ARCH-005-testing-standards.rules.ts +++ b/.archgate/adrs/ARCH-005-testing-standards.rules.ts @@ -9,7 +9,6 @@ export default { description: "Test directory structure should mirror src/ structure", severity: "error", async check(ctx) { - // Get all src modules (non-index, non-cli.ts) const srcFiles = await ctx.glob("src/**/*.ts"); const testFiles = await ctx.glob("tests/**/*.test.ts"); diff --git a/.archgate/adrs/ARCH-007-cross-platform-subprocess-execution.rules.ts b/.archgate/adrs/ARCH-007-cross-platform-subprocess-execution.rules.ts index 2fbb2038..b50ddf60 100644 --- a/.archgate/adrs/ARCH-007-cross-platform-subprocess-execution.rules.ts +++ b/.archgate/adrs/ARCH-007-cross-platform-subprocess-execution.rules.ts @@ -10,7 +10,6 @@ export default { (f) => !f.includes("tests/") && !f.includes(".archgate/") ); - // Check for Bun.$ template literal usage const bunShellMatches = await Promise.all( files.map((file) => ctx.grep(file, /Bun\.\$`/u)) ); @@ -26,7 +25,6 @@ export default { } } - // Check for $ import from "bun" (the shell API) const dollarImportMatches = await Promise.all( files.map((file) => ctx.grep(file, /import\s*\{[^}]*\$[^}]*\}\s*from\s*["']bun["']/u) @@ -44,7 +42,6 @@ export default { } } - // Check for await $` pattern (destructured $ usage) const destructuredMatches = await Promise.all( files.map((file) => ctx.grep(file, /await\s+\$`/u)) ); diff --git a/.archgate/adrs/ARCH-008-typed-command-options.rules.ts b/.archgate/adrs/ARCH-008-typed-command-options.rules.ts index a7a5e6e1..a66bc4df 100644 --- a/.archgate/adrs/ARCH-008-typed-command-options.rules.ts +++ b/.archgate/adrs/ARCH-008-typed-command-options.rules.ts @@ -1,21 +1,17 @@ /// /** - * ARCH-008 enforcement, rewritten on top of ctx.ast() (ARCH-022). - * - * The previous implementation grepped single lines for `.option(...)` and - * missed any call formatted across multiple lines — exactly the fragility - * ARCH-022 was introduced to fix. These rules now walk the ESTree produced - * by ctx.ast(file, "typescript") and inspect real CallExpression arguments, - * so formatting, whitespace, and string escaping no longer matter. + * ARCH-008 enforcement on top of ctx.ast() (ARCH-022): rules walk the ESTree + * produced by ctx.ast(file, "typescript") and inspect real CallExpression + * arguments, so formatting, whitespace, and string escaping do not matter — + * multi-line `.option(...)` calls match the same as single-line ones. */ /** * Description strings that enumerate a fixed set of values, e.g. * "editor integration to configure (claude, cursor, vscode, copilot)" or * "ADR domain: backend, frontend, data, architecture, general". - * Same heuristic as the previous regex rule, but applied to the parsed - * Literal VALUE rather than raw source text. + * Matched against the parsed Literal VALUE, not raw source text. */ const CHOICE_ENUMERATION = /(?:claude.*cursor|backend.*frontend)/u; @@ -60,14 +56,11 @@ function isStringLiteral( } /** - * Locate the 1-based line of an option's flag string in the ORIGINAL source. - * - * ctx.ast(file, "typescript") parses Bun-transpiled output, which reprints - * the module and collapses multi-line calls onto single lines — node.loc - * therefore refers to transpiled lines and is unusable for reporting. - * Searching the untranspiled source for the quoted flag literal gives an - * exact line instead; when the flag can't be found (e.g. built dynamically), - * the violation is reported file-only rather than with a wrong line. + * Locate the 1-based line of an option's flag string in the ORIGINAL source: + * ctx.ast(file, "typescript") parses Bun-transpiled output whose node.loc + * refers to transpiled lines, so the untranspiled source is searched for the + * quoted flag literal instead. A flag that can't be found (e.g. built + * dynamically) is reported file-only rather than with a wrong line. */ function findFlagLine(source: string, flag: string): number | undefined { const needles = [`"${flag}"`, `'${flag}'`, `\`${flag}\``]; diff --git a/.archgate/adrs/ARCH-012-command-error-boundaries.md b/.archgate/adrs/ARCH-012-command-error-boundaries.md index d5e4faff..0f7d427b 100644 --- a/.archgate/adrs/ARCH-012-command-error-boundaries.md +++ b/.archgate/adrs/ARCH-012-command-error-boundaries.md @@ -18,6 +18,8 @@ Async command actions that lack try-catch error boundaries produce poor user exp This was discovered during a repository-wide review where `review-context`, `session-context claude-code`, and `session-context cursor` all lacked error boundaries. +The failure mode is also documented by incident CLI-5: `src/commands/check.ts` once wrapped only `loadRuleAdrs()` in try-catch, so a `UserError` thrown later by `runChecks()` escaped to `main().catch()`, where it was miscaptured to Sentry and exited with code 2 instead of 1. A boundary that covers only part of the action body fails exactly like no boundary at all — the try-catch MUST span the entire action. + ARCH-002 defines the exit code convention and logging patterns, but does not require error boundaries in command actions. This ADR complements ARCH-002 by making error boundaries mandatory. **Why not a global Commander.js error handler?** Commander provides `.exitOverride()` and `.configureOutput()` for parsing errors (unknown options, missing arguments), but these do **not** cover errors thrown inside async `.action()` callbacks. Commander's `preAction`/`postAction` hooks could theoretically wrap actions, but they don't catch async errors from the action body. The `main().catch()` in `cli.ts` catches unhandled rejections as a safety net (exit 2), but per-command try-catch is needed to produce contextual error messages and exit with code 1 instead of 2. diff --git a/.archgate/adrs/ARCH-012-command-error-boundaries.rules.ts b/.archgate/adrs/ARCH-012-command-error-boundaries.rules.ts index 5c838737..b812afbe 100644 --- a/.archgate/adrs/ARCH-012-command-error-boundaries.rules.ts +++ b/.archgate/adrs/ARCH-012-command-error-boundaries.rules.ts @@ -1,17 +1,11 @@ /// /** - * ARCH-012 enforcement, rewritten on top of ctx.ast() (ARCH-022). - * - * The previous implementation only regex-detected the PRESENCE of a try-catch - * inside an async action. That let partial boundaries pass: src/commands/ - * check.ts once wrapped only loadRuleAdrs() in try/catch, and a UserError - * thrown later by runChecks() escaped to main().catch(), where it was - * miscaptured to Sentry with exit 2 (incident CLI-5). These rules now walk - * the ESTree produced by ctx.ast(file, "typescript"): the boundary rule - * additionally flags top-level awaited statements that sit OUTSIDE the - * action's try block — the exact statements whose rejections escape the - * boundary. + * ARCH-012 enforcement on top of ctx.ast() (ARCH-022). A try-catch that + * covers only part of an action is as leaky as none, so the boundary rule + * walks the ESTree and flags top-level awaited statements sitting OUTSIDE + * the action's try block, whose rejections escape to main().catch() and + * miscapture to Sentry. See ARCH-012 for the failure it prevents. */ /** Node types whose bodies run in their own context — awaits inside them are @@ -153,7 +147,6 @@ export default { "Async command actions must include try-catch error boundaries", severity: "warning", async check(ctx) { - // Only check non-index command files const files = ctx.scopedFiles.filter( (f) => f.includes("commands/") && !f.endsWith("index.ts") ); @@ -235,7 +228,6 @@ export default { description: "Catch blocks in async command actions must re-throw ExitPromptError for proper Ctrl+C handling (exit 130)", async check(ctx) { - // Only check non-index command files that have async actions const files = ctx.scopedFiles.filter( (f) => f.includes("commands/") && !f.endsWith("index.ts") ); @@ -243,7 +235,6 @@ export default { const checks = files.map(async (file) => { const content = await ctx.readFile(file); - // Only check files with async actions that have try-catch const hasAsyncActionWithTryCatch = /\.action\(\s*async\s[\s\S]*?\btry\s*\{/u.test(content); if (!hasAsyncActionWithTryCatch) return; diff --git a/.archgate/adrs/ARCH-015-cli-command-documentation-coverage.rules.ts b/.archgate/adrs/ARCH-015-cli-command-documentation-coverage.rules.ts index 194507f0..ffe75813 100644 --- a/.archgate/adrs/ARCH-015-cli-command-documentation-coverage.rules.ts +++ b/.archgate/adrs/ARCH-015-cli-command-documentation-coverage.rules.ts @@ -16,13 +16,10 @@ export default { "Every top-level CLI command (src/commands/.ts or src/commands//index.ts) must have a corresponding reference page at docs/src/content/docs/reference/cli/.mdx, and vice versa", severity: "error", async check(ctx) { - // Discover top-level command names from src/commands/. - // Per ARCH-001, top-level commands live at either - // src/commands/.ts — single-file command - // src/commands//index.ts — command group - // Nested files like src/commands//create.ts or - // src/commands///index.ts are subcommands and NOT - // independent top-level commands. + // Discover top-level command names from src/commands/. Per ARCH-001 + // they live at src/commands/.ts (single-file) or + // src/commands//index.ts (command group); nested files are + // subcommands, not independent top-level commands. const commandNames = new Set(); const topLevelFiles = await ctx.glob(`${COMMANDS_DIR}/*.ts`); @@ -38,7 +35,6 @@ export default { commandNames.add(name); } - // Collect docs stems. const docFiles = await ctx.glob(`${DOCS_DIR}/*.mdx`); const docStems = new Set(); for (const file of docFiles) { diff --git a/.archgate/adrs/ARCH-016-cli-subcommand-documentation-accuracy.rules.ts b/.archgate/adrs/ARCH-016-cli-subcommand-documentation-accuracy.rules.ts index e8b5778a..17b01984 100644 --- a/.archgate/adrs/ARCH-016-cli-subcommand-documentation-accuracy.rules.ts +++ b/.archgate/adrs/ARCH-016-cli-subcommand-documentation-accuracy.rules.ts @@ -11,27 +11,18 @@ export default { severity: "error", async check(ctx) { // ── 1. Discover subcommand names from src/commands/ ────────────── - // - // Top-level command groups live at src/commands//index.ts. - // Direct subcommands are either: - // src/commands//.ts (single-file subcommand) - // src/commands///index.ts (nested command group) - // - // We only look one level deep: /. Files like - // src/commands/adr/domain/add.ts are sub-subcommands of "adr domain" - // and are NOT checked by this rule (they are documented in the - // "adr domain" section as a table, not as separate headings). - - // Find all parent command groups (dirs with an index.ts). + // Direct subcommands of a group (src/commands//index.ts) are + // /.ts or //index.ts. Only one level deep + // is checked: deeper files like adr/domain/add.ts are sub-subcommands + // documented as a table in the parent's section, not as headings. + const groupIndexFiles = await ctx.glob(`${COMMANDS_DIR}/*/index.ts`); - // Extract parent names from index files. const parentNames = groupIndexFiles.map((indexFile) => { const rel = indexFile.slice(COMMANDS_DIR.length + 1); return rel.split("/")[0]; }); - // Discover subcommands for all parents in parallel. const subResults = await Promise.all( parentNames.map(async (parentName) => { const [subFiles, nestedGroupFiles] = await Promise.all([ @@ -41,7 +32,6 @@ export default { const subs = new Set(); - // Single-file subcommands for (const sf of subFiles) { const fileName = sf.slice( `${COMMANDS_DIR}/${parentName}/`.length @@ -50,7 +40,6 @@ export default { subs.add(fileName.slice(0, -".ts".length)); } - // Nested command groups for (const ngf of nestedGroupFiles) { const nestedRel = ngf.slice( `${COMMANDS_DIR}/${parentName}/`.length @@ -74,7 +63,6 @@ export default { // ### archgate adr domain const headingPattern = /^#{1,4}\s+.*archgate\s+(\S+)\s+(\S+)/giu; - // Read all docs files in parallel. const docsResults = await Promise.all( [...subcommandsByParent.entries()].map( async ([parentName, subNames]) => { @@ -91,7 +79,6 @@ export default { ) ); - // Report violations. for (const { parentName, subNames, @@ -100,7 +87,6 @@ export default { } of docsResults) { if (docsContent === null) continue; - // Extract documented subcommand names from headings const documentedSubs = new Set(); let match; headingPattern.lastIndex = 0; diff --git a/.archgate/adrs/ARCH-019-inquirer-prompt-fix.md b/.archgate/adrs/ARCH-019-inquirer-prompt-fix.md index 5f35c5e9..62f78f50 100644 --- a/.archgate/adrs/ARCH-019-inquirer-prompt-fix.md +++ b/.archgate/adrs/ARCH-019-inquirer-prompt-fix.md @@ -74,3 +74,4 @@ Code reviewers MUST verify any new interactive flow wraps its prompts in `withPr - [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 +- [Inquirer.js issue #2123](https://github.com/SBoudrias/Inquirer.js/issues/2123) — upstream report of the unrestored Windows console-mode flag diff --git a/.archgate/adrs/ARCH-022-ast-aware-rule-context.md b/.archgate/adrs/ARCH-022-ast-aware-rule-context.md index 744410da..abf2ce31 100644 --- a/.archgate/adrs/ARCH-022-ast-aware-rule-context.md +++ b/.archgate/adrs/ARCH-022-ast-aware-rule-context.md @@ -41,7 +41,7 @@ This method dispatches internally based on `language`, and the dispatch mechanis **Guardrail ordering — this is the core architectural constraint of this ADR.** A rule author MUST NEVER be able to reach `Bun.spawn`, `child_process`, or any other subprocess/filesystem primitive directly; `ctx.ast()` is the only door, exactly as `glob`/`grep`/`readFile` are today, and this is consistent with the sandbox `rule-scanner.ts` already enforces on `.rules.ts` source (which explicitly blocks `Bun.spawn` and `Bun.spawnSync` from rule code). All of the following MUST execute inside `createRuleContext()` in `src/engine/runner.ts`, in this order, before any subprocess is spawned: -1. **Path safety** — the requested `path` MUST pass through the same `safePath()` sandboxing already applied to `readFile`/`glob` (no traversal outside `scopedFiles`, no symlink escapes). +1. **Path safety** — the requested `path` MUST pass through the same `safePath()` sandboxing already applied to `readFile`/`glob` (no traversal outside `scopedFiles`, no symlink escapes). "No symlink escapes" means **every component below the project root**, not just the leaf: with `/docs` linked outside the project, `/docs/secret.txt` is an ordinary file, so a lexical containment check and an `lstat` of the leaf both pass while the OS resolves through the link and reads outside. `assertNoSymlinkInPath()` in `src/engine/safe-path.ts` walks each component and rejects any that is a link. Two deliberate limits on that walk: components at or above the root are NOT inspected (the root's own location is the user's business, and macOS's temp prefix is itself a symlink — `/var` → `/private/var` — which would otherwise reject every temp-dir root), and each component is tested with a boolean `lstat` rather than compared against `realpath`, which case-canonicalizes on Windows and macOS and would reject case-mismatched-but-legitimate paths. 2. **Language plausibility check** — the file's extension and/or leading content MUST be sanity-checked against the requested `language` before any interpreter is invoked on it. A rule calling `ctx.ast("config.json", "python")` MUST fail this check rather than hand arbitrary file content to a Python interpreter. 3. **Interpreter availability probe** — for `"python"`/`"ruby"`, an availability check (e.g. `Bun.spawn([candidate, "--version"])` wrapped in `try/catch`, following the exact pattern `isClaudeCliAvailable()` uses in ARCH-007) MUST run before the real invocation. `python3` is not a universal PATH alias on Windows (the common installer exposes `python`, not `python3`); the probe MUST try platform-appropriate candidate executable names in order (e.g. `python3` then `python` on non-Windows; `python`, then `python3`, then the `py` launcher on Windows — the python.org installer registers `py` even when "Add python.exe to PATH" is unchecked — using [ARCH-009](./ARCH-009-platform-detection-helper.md)'s `isWindows()`) and use the first one that resolves for both the probe and the real invocation. This probe result MUST be cached once per `check` invocation, not re-run per file. 4. **Guarded invocation** — the actual `Bun.spawn` call MUST use array-based arguments only, per ARCH-007, with no shell interpolation of file contents or paths. @@ -61,7 +61,7 @@ This method dispatches internally based on `language`, and the dispatch mechanis - The git reads (`git merge-base`, `git show :`) live in `src/engine/git-files.ts` — the same sanctioned git subprocess site the `no-unsanctioned-engine-subprocess` companion rule already permits. No new `Bun.spawn` site, no `child_process`. - All four guardrails still run, in order, inside `ast()` for a base parse: path safety (`safePath` on the original path — which also yields the repo-relative form `git show` needs), language plausibility (`AST_LANGUAGE_EXTENSIONS` on the original path), interpreter probe, guarded invocation. -- TypeScript/JavaScript base source is parsed in-process by the same `meriyah` path (`parseTsOrJsSource`, factored into `src/engine/js-parser.ts` and shared with the working-tree branch). Python/Ruby base content is not on disk, so it is written to a throwaway OS-temp file — outside the project tree, therefore outside any cwd-derived load path — and handed to the **same, unchanged** `PYTHON_AST_PROGRAM`/`RUBY_AST_PROGRAM` with the **same mandatory `-I` isolation**. The `python-subprocess-isolated` invariant is unchanged. +- TypeScript/JavaScript base source is parsed in-process by the same `meriyah` path (`parseTsOrJsSource`, factored into `src/engine/js-parser.ts` and shared with the working-tree branch). Python/Ruby base content is not on disk, so it is written to a throwaway OS-temp file — outside the project tree, therefore outside any cwd-derived load path — and handed to the **same, unchanged** `PYTHON_AST_PROGRAM`/`RUBY_AST_PROGRAM` with the **same mandatory `-I` isolation**. The `python-subprocess-isolated` invariant is unchanged. The temp write itself is hardened against shared-tmpdir symlink attacks (`writeTempSourceFile` in `ast-support.ts`): a world-shared `/tmp` lets an attacker pre-create a symlink at a predictable filename so a naive write follows it to an attacker-chosen destination. The helper defeats this by creating a fresh `mkdtemp` directory (mode `0700`, unpredictable suffix) and opening the file inside it with exclusive create (`wx`, mode `0600`) — the open fails rather than following any pre-existing path. **Comment access (`{ comments: true }`).** `AstOptions` carries a second field, `comments?: boolean`. With `{ comments: true }`, the returned tree carries a `comments` array of `CommentToken` (`{ type: "line" | "block"; value: string; loc: { start, end } }`) — structured comment data for comment-governance rules (comment length, style, content), in place of the line-by-line regex heuristics that were the only option while the AST exposed no comments at all. All four languages are supported (Ruby comments come from a second `Ripper.lex` pass — see below). This capability MUST fold into `ast()` — attaching `comments` to the returned tree — rather than a separate method, so it runs inside the same four-guardrail flow and `single-ast-method` stays satisfied (no new method, the catch-all signature unchanged). diff --git a/.archgate/adrs/ARCH-022-ast-aware-rule-context.rules.ts b/.archgate/adrs/ARCH-022-ast-aware-rule-context.rules.ts index e0ba8464..2440616b 100644 --- a/.archgate/adrs/ARCH-022-ast-aware-rule-context.rules.ts +++ b/.archgate/adrs/ARCH-022-ast-aware-rule-context.rules.ts @@ -2,12 +2,10 @@ /** * Identifiers that must appear, in this order, inside the `ast()` method of - * `createRuleContext()` (src/engine/runner.ts). Each anchors one of the four - * mandated guardrails: - * 1. safePath — path sandbox (same as readFile/glob) - * 2. AST_LANGUAGE_EXTENSIONS — language plausibility check - * 3. probeInterpreter — interpreter availability probe - * 4. runAstSubprocess — guarded array-args invocation + * `createRuleContext()` (src/engine/runner.ts). Each anchors one mandated + * guardrail: safePath (path sandbox), AST_LANGUAGE_EXTENSIONS (language + * plausibility), probeInterpreter (interpreter probe), runAstSubprocess + * (guarded array-args invocation). */ const GUARDRAIL_SEQUENCE = [ "safePath", @@ -74,8 +72,8 @@ export default { } return; } - // Fallback: inline `ast(path, language) { … }` object method, in - // case the implementation is ever moved back onto the object. + // Fallback: inline `ast(path, language) { … }` object method, for + // an implementation that lives directly on the returned object. if (n.type === "Property") { const key = n.key as (EsTreeNode & { name?: string }) | undefined; const value = n.value as EsTreeNode | undefined; diff --git a/.archgate/adrs/CI-001-pin-github-actions-by-hash.rules.ts b/.archgate/adrs/CI-001-pin-github-actions-by-hash.rules.ts index 2f75231f..f76a400f 100644 --- a/.archgate/adrs/CI-001-pin-github-actions-by-hash.rules.ts +++ b/.archgate/adrs/CI-001-pin-github-actions-by-hash.rules.ts @@ -42,7 +42,6 @@ export default { ); for (const m of matches) { - // Extract the full `uses:` value from the matched line const usesMatch = m.content.match( /uses:\s+(?!\.\/|docker:\/\/)(\S+@\S+)/u ); diff --git a/.archgate/adrs/GEN-002-docs-i18n.rules.ts b/.archgate/adrs/GEN-002-docs-i18n.rules.ts index 5d59d0c8..a8daa09e 100644 --- a/.archgate/adrs/GEN-002-docs-i18n.rules.ts +++ b/.archgate/adrs/GEN-002-docs-i18n.rules.ts @@ -9,7 +9,6 @@ const LOCALES = ["pt-br", "nb"]; const CONTENT_ROOT = "docs/src/content/docs"; -/** Patterns that match locale-prefixed internal links in MDX files. */ const LOCALE_LINK_PATTERNS = LOCALES.map( (locale) => new RegExp(`(?:href="|\\]\\()/${locale}/`, "gu") ); @@ -53,7 +52,6 @@ export default { async check(ctx) { const allMdxFiles = await ctx.glob(`${CONTENT_ROOT}/**/*.mdx`); - // Separate root files from locale files const rootFiles: string[] = []; const localeFiles = new Map(); @@ -128,7 +126,6 @@ export default { if (changedRootFiles.length === 0) return; - // Pre-build a set of all existing locale files for fast lookup const localeFileArrays = await Promise.all( LOCALES.map((locale) => ctx.glob(`${CONTENT_ROOT}/${locale}/**/*.mdx`) diff --git a/.archgate/adrs/GEN-003-tool-invocation-via-scripts.rules.ts b/.archgate/adrs/GEN-003-tool-invocation-via-scripts.rules.ts index 99a58a5c..ef2fe521 100644 --- a/.archgate/adrs/GEN-003-tool-invocation-via-scripts.rules.ts +++ b/.archgate/adrs/GEN-003-tool-invocation-via-scripts.rules.ts @@ -13,7 +13,6 @@ export default { "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`). diff --git a/.archgate/adrs/GEN-004-concise-forward-only-code-comments.md b/.archgate/adrs/GEN-004-concise-forward-only-code-comments.md new file mode 100644 index 00000000..476d47df --- /dev/null +++ b/.archgate/adrs/GEN-004-concise-forward-only-code-comments.md @@ -0,0 +1,153 @@ +--- +id: GEN-004 +title: Concise, Forward-Only Code Comments +domain: general +rules: true +files: + - "src/**/*.ts" + - "tests/**/*.ts" + - "lint/**/*.ts" + - "scripts/**/*.ts" + - "shims/**/*.ts" + - ".archgate/lint/**/*.ts" + - ".archgate/adrs/**/*.rules.ts" +--- + +# Concise, Forward-Only Code Comments + +## Context + +Code comments in this repository have drifted toward two failure modes, both produced disproportionately by AI agents narrating their own work: + +1. **Historical narration**: comments describing what the code "used to" do, what "previously" happened, or why an earlier approach was replaced (`// which used to add 3s of latency`, `* This used to be cached per process, but...`). The same information already lives in git history (commit messages, PR descriptions) and in ADRs; embedded in code it becomes a changelog entry that nobody maintains and that turns misleading after the next change. +2. **Oversized comment blocks**: multi-paragraph headers narrating an investigation, incident, or design discussion when 1-3 sentences describing current behavior — plus a pointer to the ADR or memory file that holds the full story — would serve the reader better. At the time this ADR was adopted, a repository-wide scan found 88 comment runs exceeding 5 lines of prose and 17 narration comments; all were fixed in the adopting change. + +A repository-wide guideline existed only as agent-memory feedback ("comments must be concise"), which binds a single memory-equipped agent, not humans or other tools. This repo's own experience (ARCH-019 with its companion rule, the custom oxlint plugins in `lint/`) shows conventions only stick once a check enforces them. + +### Alternatives considered + +1. **Agent memory / CLAUDE.md guidance only (status quo)**: Rejected — unenforced guidance is exactly what allowed an 88-violation backlog to accumulate. +2. **Companion `.rules.ts` heuristics only**: Portable and surfaced by `archgate check` / `archgate review-context`, but line-regex heuristics cannot distinguish comment tokens from string literals and miss block-comment interiors. +3. **Custom oxlint plugin rules only**: Real comment tokens via `sourceCode.getAllComments()` (no string-literal false positives, precise spans, IDE-visible), but invisible to agents that only run `archgate check`, and not portable to repos that don't share this lint setup. +4. **Both layers (chosen)**: The oxlint rules are the precise, developer-facing check in `bun run lint`; the companion `.rules.ts` mirrors the same invariants in `archgate check`, where agent workflows and `review-context` briefings surface them. Redundancy is deliberate — each layer covers the other's blind spot. + +For the Archgate CLI specifically, this ADR is also dogfooding: the product's premise is that governance needs machine-checkable rules, and comment discipline is a governance concern that both its enforcement layers can demonstrate. + +## Decision + +Comments in all project-authored TypeScript MUST be concise and MUST describe current behavior only, never the history of how that behavior came to be. + +### Conciseness + +- A contiguous run of whole-line comments MUST carry **at most 5 lines of prose**. The bound counts prose only: comment delimiters (`/**`, `*/`, a bare `*`), section dividers, SPDX headers (LEGAL-001), and tool directives (`oxlint-disable`, `@ts-expect-error`, `archgate-ignore`, and similar) keep a run contiguous but do not count. +- Longer explanations belong in a fuller reference — an ADR, agent-memory file, issue, or PR — with the comment pointing at it (`// See ARCH-019 for the Windows console-mode background`). Genuinely non-obvious, safety-critical invariants justify using the full 5 lines; nothing justifies exceeding them inline. + +### Structured documentation is exempt + +The bound measures **narrative**, so it applies to the untagged summary section of a doc comment only. A TSDoc block tag opens a section that does not count against the budget: + +- **Exempt (structural tags):** `@param`, `@arg`, `@typeParam`, `@template`, `@returns`, `@throws`, `@example`, `@see`, `@link`, `@defaultValue`, `@deprecated`, `@internal`, `@public`, `@alpha`, `@beta`, `@experimental`, `@module`, `@packageDocumentation`, `@typedef`, `@callback`, `@property`, `@overload`, `@inheritDoc`, `@label`. The tag line and every line under it, up to the next tag, is exempt. A twelve-parameter function legitimately carries twelve `@param` lines, and an `@example` is as long as the snippet it shows; neither is padding, and neither competes with the summary for budget. +- **NOT exempt (prose-container tags):** `@remarks`, `@description`, `@summary`, `@notes`, `@todo`, `@fixme`. Their content is narrative in a tag's clothing. Exempting them would reduce the entire bound to "write `@remarks` on line one", so they count exactly like untagged prose. + +Documenting a parameter, a return contract, a thrown error, or a usage snippet with the correct tag is therefore always preferable to compressing it into the summary: the tag carries the semantics into IDE tooltips and TSDoc tooling, and it frees the summary's five lines for the thing the summary is for. Length in a doc comment is only a problem when it is narrative. + +### Forward-only content + +- Comments MUST NOT narrate history: no "used to X, now Y", "previously", "no longer", "originally", "an earlier version", "was tried", references to `git blame`, or similar phrasing describing what changed rather than what the code does. +- Comments MUST NOT narrate relocations or refactors: no "Extracted from `X`", "was migrated", "has been moved to `Y`", "split out of", "renamed from", "formerly known as". A move is an event in git history, not a property of the code. +- **Present-tense location prose is NOT narration and remains encouraged.** `// The single sanctioned meriyah call site is src/engine/js-parser.ts` describes current structure. The test is grammatical: if the sentence's subject is the _change_ ("was migrated", "Extracted from"), it is narration; if the subject is the _code as it stands_ ("lives in", "is defined in"), it is a description. +- When behavior changes, rewrite the comment to state the new contract — never append "(update: now does Y)" on top of the old text. + +### Scope + +Applies to all project-authored TypeScript with no carve-outs: `src/`, `tests/` (including `tests/fixtures/`), `lint/`, `scripts/`, `shims/`, `.archgate/lint/`, and the ADR companion rules files (`.archgate/adrs/**/*.rules.ts`) — the code enforcing this decision is subject to it. Markdown, YAML, and JSON are governed by this ADR's prose but not by the automated checks. + +**Test code is held to the same bound as source.** Tests are read far more often than they are written, and a fixture-explaining block that narrates an investigation costs a reader exactly what it costs in `src/`. The structured-documentation exemption above already covers the legitimate reason a test comment grows — describing a scenario's inputs and expected outcome — so a size carve-out for `tests/**` would buy nothing but padding. Fixtures under `tests/fixtures/` are equally in scope: their _content_ is deliberately arbitrary, but the comments explaining what each fixture exercises are ordinary documentation and are held to the ordinary standard. + +**Agent memory (`.claude/agent-memory/**`) is exempt from the forward-only requirement.** Those files exist to record incident history — a memory entry's `**Why:**` line is deliberately a past-tense account of the failure that produced the rule, and that account is what lets a future agent judge edge cases instead of following the rule blindly. Applying forward-only prose there would delete the very content the memory system is for. Conciseness still applies by convention (memory is loaded into every session's context), but neither requirement is machine-checked in that directory. This exemption is what makes "move deep rationale to an ADR or memory file" a real remedy rather than a redirection to a second place the rationale is banned. + +## Do's and Don'ts + +### Do + +- **DO** keep comment runs to 1-3 sentences — at most 5 lines of prose. +- **DO** describe what the code currently does, not what it used to do or why an earlier approach was abandoned. +- **DO** document parameters, returns, thrown errors, and usage with `@param`, `@returns`, `@throws`, and `@example` rather than describing them in summary prose — the tags are exempt from the bound and carry the semantics into IDE tooltips. +- **DO** reach for `@see` when pointing at an ADR, test, or sibling symbol (`@see ARCH-022`), which is the tagged form of the "point to a fuller reference" remedy. +- **DO** write present-tense location pointers (`// Engine file listing is in-memory git-tracked matching, see ARCH-023`). +- **DO** move deep rationale (incidents, investigations, platform quirks) into an ADR or `.claude/agent-memory/` file and reference it from a one-line comment. +- **DO** rely on commit messages and PR descriptions to record why a change was made. +- **DO** rewrite a comment to the new contract when behavior changes. +- **DO** treat a `GEN-004/no-narration-in-comments`, `GEN-004/oversized-comment-blocks`, `archgate/no-narration-in-comments`, or `archgate/oversized-comment-blocks` failure as a merge blocker. + +### Don't + +- **DON'T** write comments containing "used to", "previously", "no longer", "originally", "an earlier version", "was tried", or references to `git blame` — reword to present tense (the reworded form is invariably clearer). +- **DON'T** narrate moves — "Extracted from X", "was migrated", "split out of", "renamed from" — even when the move explains why a module exists. Describe what the module _is_. +- **DON'T** replace a short comment with a multi-paragraph block narrating the investigation that led to the current code. +- **DON'T** park a violation behind `archgate-ignore` or `oxlint-disable` as a convenience — suppression is reserved for genuine false positives, with the reason stated in the suppression comment. +- **DON'T** dissolve `@param`/`@returns`/`@throws`/`@example` tags into summary prose to fit the bound — that trades machine-readable API documentation for a shorter comment and is a net loss. +- **DON'T** relabel narrative as `@remarks`, `@description`, `@notes`, `@todo`, or `@fixme` to escape the budget — those tags count as prose precisely to close that door. +- **DON'T** assume markdown, YAML, or JSON files are exempt just because the automated checks cover only TypeScript — the decision applies; enforcement there is manual review. +- **DON'T** treat "the automated check didn't flag it" as proof of compliance — both layers are heuristic about _content_; a comment can narrate history without any matched phrase, and a 5-line comment can still be padding. + +## Consequences + +### Positive + +- **Lower reading cost:** Comments describe only the code in front of the reader. +- **No drift:** A comment describing only current behavior cannot contradict a previous state it never mentions. +- **Two-layer backstop:** Violations surface in `bun run lint` (IDE-visible, AST-precise) and in `archgate check` (agent-facing briefings) — neither humans nor agents can miss them. +- **Dogfooding:** Demonstrates archgate rules and custom lint rules covering one decision from both sides. +- **Deep context survives:** Rationale moves to ADRs and memory files where it is maintained, instead of decaying inline. +- **Pressure toward structured documentation:** Because tagged sections cost nothing against the budget while summary prose does, the cheapest way to keep a long doc comment is to express it as `@param`/`@returns`/`@throws`/`@example`. The bound nudges authors toward machine-readable TSDoc rather than away from documenting. + +### Negative + +- **Heuristic false positives block:** Both layers run at `error`. Phrases like "used to" match purpose clauses ("is used to tailor error messages") as well as history. The remedy is rewording to present tense ("tailors error messages"), which is what this ADR wants anyway; genuine false positives can be suppressed with a reasoned `archgate-ignore` / `oxlint-disable-next-line` comment. +- **Heuristic false negatives:** The phrase lists are not semantic — narration that avoids the matched phrases passes automatically and still needs a reviewer. +- **The 5-line bound is a proxy:** It counts lines, not sentences; terse padding fits under it. Reviewers catch emptiness; the rule catches size. +- **The tag exemption is syntactic, not semantic:** Both layers trust the tag. Narrative parked under a structural tag — a `@see` followed by four paragraphs, or an `@example` whose "snippet" is prose — is exempted without being examined. Restricting the exemption to structural tags and excluding prose containers closes the obvious door, not every door; reviewers still verify that a tagged section contains what its tag claims. + +### Risks + +- **A bound is a target** — prose will cluster at 5 lines. **Mitigation:** the limit is a ceiling, not an allowance; reviewers push back on 5-line comments that say nothing. +- **Pattern lists go stale** — new narration phrasing won't be caught until someone extends the regexes. **Mitigation:** treat the lists as living; when a reviewer catches an unmatched pattern, extend both the oxlint rule and the companion `.rules.ts` in the same change, preferring grammar-based (past-tense/passive) patterns over vocabulary that also appears in encouraged location prose. +- **The two layers drift apart** — a pattern added to one but not the other silently narrows coverage. **Mitigation:** the shared patterns live in comments cross-referencing each other's file, and reviewers verify both files change together (see Manual enforcement). + +## Compliance and Enforcement + +### Automated enforcement + +Two layers, same invariants, both `error` severity: + +- **oxlint plugin rules** (`.archgate/lint/oxlint.ts`, run by `bun run lint`): `archgate/no-narration-in-comments` and `archgate/oversized-comment-blocks` operate on real comment tokens via `sourceCode.getAllComments()` — string literals never match, block-comment interiors always do. Both run repo-wide with no per-directory overrides in `.oxlintrc.json`. +- **Archgate rules** (companion `GEN-004-concise-forward-only-code-comments.rules.ts`, run by `archgate check`): `GEN-004/no-narration-in-comments` greps comment-looking lines for the narration and relocation patterns; `GEN-004/oversized-comment-blocks` counts prose lines in contiguous whole-line comment runs. Line-based heuristics — a match inside a string literal that starts a line with `//` is a known (rare) false positive, suppressible via `archgate-ignore` with a reason. + +Both layers implement the structured-documentation exemption identically: a line opening a structural TSDoc tag, and every line under it up to the next tag, is skipped when counting prose; prose-container tags are counted. The two tag lists MUST stay in sync — they are the one piece of logic duplicated across the layers. + +Both launched at `error` with the 88-block / 17-comment backlog fixed in the adopting change. Any future tightening MUST bring the codebase to zero in the same change. + +### Manual enforcement + +Reviewers MUST verify on every PR touching project TypeScript: + +1. New or modified comments describe current behavior only — no historical or relocation framing. +2. Comment runs stay within 5 prose lines, and longer rationale went to an ADR/memory file with a pointer. +3. Any suppression (`archgate-ignore`, `oxlint-disable`) carries a reason and marks a genuine false positive, not a parked violation. +4. Changes to the narration/relocation patterns touch both `.archgate/lint/oxlint.ts` and the companion `.rules.ts`. +5. Markdown/YAML/JSON prose follows the same rule even though no automated check covers it. + +### Exceptions + +- **Structured TSDoc sections**: exempt from the size bound as described in the Decision, never from the forward-only requirement — a `@param` line may not narrate history either. +- **`.claude/agent-memory/**`\*\*: exempt from the forward-only requirement, as described in Scope. +- **Suppressions**: genuine false positives only, always with a stated reason. +- **No directory-level exemptions**: there are no per-path carve-outs from either rule. Test files, fixtures, lint plugins, and the ADR companion rules files are all in scope, and any future exemption MUST be recorded here rather than added silently to `.oxlintrc.json`. + +## References + +- [ARCH-019: Interactive Prompts via withPromptFix](./ARCH-019-inquirer-prompt-fix.md) — the model this ADR generalizes: deep platform rationale lives in the ADR; call sites carry a one-line pointer +- [GEN-003: Tool Invocation via Package Scripts](./GEN-003-tool-invocation-via-scripts.md) — precedent for a GEN-domain decision enforced by companion rules +- [LEGAL-001: SPDX License Headers](./LEGAL-001-spdx-license-headers.md) — SPDX header lines are mandatory and do not count toward the prose bound +- [ARCH-005: Testing Standards](./ARCH-005-testing-standards.md) — governs the test files this ADR partially exempts diff --git a/.archgate/adrs/GEN-004-concise-forward-only-code-comments.rules.ts b/.archgate/adrs/GEN-004-concise-forward-only-code-comments.rules.ts new file mode 100644 index 00000000..4f0b0cea --- /dev/null +++ b/.archgate/adrs/GEN-004-concise-forward-only-code-comments.rules.ts @@ -0,0 +1,146 @@ +/// + +// Line-based heuristics mirroring the token-based oxlint rules in +// .archgate/lint/oxlint.ts — keep the two pattern sets in sync (GEN-004). + +// Historical-narration phrases. Grammar notes live in GEN-004's Decision. +const NARRATION_PATTERN = + /\b(used to|previously|no longer|originally|an earlier version|was tried|git blame|made things worse)\b/iu; + +// Past-tense/passive relocation constructions; present-tense location prose +// (encouraged by GEN-004) deliberately does not match. +const RELOCATION_PATTERN = + /\b(extracted (from|into|out of)|(was|were|has|have) been (moved|migrated|extracted|renamed|split|relocated|replaced|superseded|consolidated|refactored)|(was|were) (moved|migrated|extracted|renamed|split|relocated|superseded|consolidated|refactored)|moved (to|into) [`'"]?[\w./-]+\.tsx?\b|split out of|renamed from|used to live|formerly (lived|known|called|named)|has since|since been)\b/iu; + +// Delimiter/divider/directive lines keep a run contiguous but carry no prose. +const DIVIDER_ONLY = /^[─-╿=—\-_*\s]{3,}$/u; +const NON_PROSE = + /^( { + const content = await ctx.readFile(file); + content.split(/\r?\n/u).forEach((line, index) => { + const trimmed = line.trim(); + if (!looksLikeComment(trimmed)) return; + for (const [pattern, kind] of [ + [NARRATION_PATTERN, "narrates history"], + [RELOCATION_PATTERN, "narrates a relocation/refactor"], + ] as const) { + if (!pattern.test(trimmed)) continue; + ctx.report.violation({ + message: `Comment ${kind} instead of describing current behavior (GEN-004)`, + file, + line: index + 1, + fix: "Rewrite in present tense describing what the code does now — git history already records what changed", + }); + // One diagnostic per comment, even when both patterns match. + break; + } + }); + }); + await Promise.all(checks); + }, + }, + + "oversized-comment-blocks": { + description: + "Contiguous comment runs must carry at most 5 lines of prose — move deeper rationale to an ADR or memory file (GEN-004)", + severity: "error", + async check(ctx) { + const files = ctx.scopedFiles; + const checks = files.map(async (file) => { + const lines = (await ctx.readFile(file)).split(/\r?\n/u); + let runStart = -1; + let prose = 0; + let inStructuralSection = false; + + const flush = () => { + if (prose > OVERSIZED_BLOCK_THRESHOLD) { + ctx.report.violation({ + message: `Comment block carries ${prose} lines of prose; GEN-004 allows ${OVERSIZED_BLOCK_THRESHOLD} (roughly 1-3 sentences)`, + file, + line: runStart + 1, + fix: "Trim to the current-behavior essentials, or replace the inlined explanation with a pointer to an ADR or agent-memory file", + }); + } + runStart = -1; + prose = 0; + inStructuralSection = false; + }; + + lines.forEach((line, index) => { + const trimmed = line.trim(); + if (trimmed !== "" && looksLikeComment(trimmed)) { + if (runStart === -1) runStart = index; + const result = classifyLine(trimmed, inStructuralSection); + inStructuralSection = result.inStructuralSection; + if (result.counts) prose++; + } else { + flush(); + } + }); + flush(); + }); + await Promise.all(checks); + }, + }, + }, +} satisfies RuleSet; diff --git a/.archgate/lint/concise-comments.ts b/.archgate/lint/concise-comments.ts new file mode 100644 index 00000000..72ae2f48 --- /dev/null +++ b/.archgate/lint/concise-comments.ts @@ -0,0 +1,146 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 Archgate + +// Token-based comment rules for GEN-004, registered under the `archgate` +// plugin in oxlint.ts. Keep the pattern sets in sync with the line-based +// mirror in .archgate/adrs/GEN-004-concise-forward-only-code-comments.rules.ts. + +// Historical-narration phrases (GEN-004 "Forward-only content"). +const NARRATION_PATTERN = + /\b(used to|previously|no longer|originally|an earlier version|was tried|git blame|made things worse)\b/iu; + +// Past-tense/passive relocation constructions; present-tense location prose +// (encouraged by GEN-004) deliberately does not match. +const RELOCATION_PATTERN = + /\b(extracted (from|into|out of)|(was|were|has|have) been (moved|migrated|extracted|renamed|split|relocated|replaced|superseded|consolidated|refactored)|(was|were) (moved|migrated|extracted|renamed|split|relocated|superseded|consolidated|refactored)|moved (to|into) [`'"]?[\w./-]+\.tsx?\b|split out of|renamed from|used to live|formerly (lived|known|called|named)|has since|since been)\b/iu; + +// Divider/directive lines keep a run contiguous but carry no prose. +const DIVIDER_ONLY = /^[─-╿=—\-_*\s]{3,}$/u; +const NON_PROSE = + /^( l.replace(/^\s*\*+\s?/u, "")); + + const kept: string[] = []; + let inStructuralSection = false; + for (const line of raw) { + const text = line.trim(); + if (text.startsWith("@")) { + inStructuralSection = STRUCTURAL_TAG.test(text) && !PROSE_TAG.test(text); + continue; + } + if (inStructuralSection) continue; + if (text === "" || DIVIDER_ONLY.test(text) || NON_PROSE.test(text)) + continue; + kept.push(text); + } + return kept; +} + +// True when the comment occupies its lines alone — nothing but whitespace +// before it and nothing but whitespace after it, so `/* c */ const x = 1` +// stays out of block runs. +function isWholeLine(comment: CommentToken, lines: string[]): boolean { + const firstLine = lines[comment.loc.start.line - 1] ?? ""; + const lastLine = lines[comment.loc.end.line - 1] ?? ""; + return ( + firstLine.slice(0, comment.loc.start.column).trim() === "" && + lastLine.slice(comment.loc.end.column).trim() === "" + ); +} + +const noNarrationInComments = { + create(context: RuleContext) { + return { + Program() { + for (const comment of context.sourceCode.getAllComments()) { + for (const [pattern, kind] of [ + [NARRATION_PATTERN, "narrates history"], + [RELOCATION_PATTERN, "narrates a relocation/refactor"], + ] as const) { + if (!pattern.test(comment.value)) continue; + context.report({ + loc: comment.loc, + message: `Comment ${kind} instead of describing current behavior. Rewrite in present tense — git history already records what changed (GEN-004).`, + }); + // One diagnostic per comment, even when both patterns match. + break; + } + } + }, + }; + }, +}; + +const oversizedCommentBlocks = { + create(context: RuleContext) { + return { + Program() { + const { lines } = context.sourceCode; + const comments = context.sourceCode + .getAllComments() + .filter((c) => isWholeLine(c, lines)); + + let run: CommentToken[] = []; + const flush = () => { + const prose = run.reduce((n, c) => n + proseLines(c).length, 0); + if (prose > OVERSIZED_BLOCK_THRESHOLD && run[0]) { + context.report({ + loc: run[0].loc, + message: `Comment block carries ${prose} lines of prose; GEN-004 allows ${OVERSIZED_BLOCK_THRESHOLD} (roughly 1-3 sentences). Trim to current-behavior essentials or point to an ADR/memory file.`, + }); + } + run = []; + }; + + for (const comment of comments) { + const prev = run.at(-1); + if (prev && comment.loc.start.line > prev.loc.end.line + 1) flush(); + run.push(comment); + } + flush(); + }, + }; + }, +}; + +export const conciseCommentRules = { + "no-narration-in-comments": noNarrationInComments, + "oversized-comment-blocks": oversizedCommentBlocks, +}; diff --git a/.archgate/lint/oxlint.ts b/.archgate/lint/oxlint.ts index 34531794..0b376137 100644 --- a/.archgate/lint/oxlint.ts +++ b/.archgate/lint/oxlint.ts @@ -1,30 +1,17 @@ // SPDX-License-Identifier: Apache-2.0 // Copyright 2026 Archgate -// Custom oxlint JS plugin: validate `type:` literals passed to -// inquirer.prompt() against the prompt types registered by the installed -// inquirer version. -// -// Why this exists: inquirer v14 removed the legacy "list" prompt type -// (renamed "select" in v10), which crashed `archgate login` at runtime with -// 'Prompt type "list" is not registered'. Nothing else can catch this class -// of bug statically or in CI: -// - tsc cannot: inquirer's legacy prompt() types accept ANY `type: string` -// via the CustomQuestion escape hatch that exists for registerPrompt(). -// - tests cannot: interactive prompts need a TTY, so every test mocks the -// inquirer module entirely and the runtime prompt registry never runs. -// The invariant is purely syntactic, so it belongs in the linter. -// -// The registered set is read from the installed inquirer at plugin load, so -// the rule self-updates on dependency bumps — a future rename/removal makes -// stale call sites fail lint in the bump PR itself. -// -// The plugin runs natively as TypeScript under Bun, so there is no build step. +// Custom oxlint JS plugin (`archgate/*`), running natively as TypeScript +// under Bun. Validates `type:` literals passed to inquirer.prompt() against +// the prompt registry of the INSTALLED inquirer — neither tsc (CustomQuestion +// accepts any string) nor tests (prompts are mocked, no TTY in CI) can catch +// an unregistered prompt type; only the linter can. See ARCH-019 context. + +import { conciseCommentRules } from "./concise-comments.ts"; /** Minimal ESTree-ish node shape. The oxlint AST is ESLint-compatible. */ type AstNode = { type: string } & Record; -/** Narrow an unknown value to an AST node (an object with a string `type`). */ function asNode(value: unknown): AstNode | undefined { if ( value !== null && @@ -37,15 +24,10 @@ function asNode(value: unknown): AstNode | undefined { } /** - * Prompt types registered by the INSTALLED inquirer, read at plugin load. - * - * Deliberately not a hardcoded allowlist: when a future inquirer version - * renames or removes a prompt type (as v10 did with "list" -> "select"), - * the registry shrinks and stale call sites fail `bun run lint` immediately - * in the dependency-bump PR — no manual list maintenance. - * - * If inquirer ever changes the registry's API shape, the loud throw below - * fails the whole lint run rather than silently disabling the rule. + * Prompt types read from the installed inquirer at plugin load — not a + * hardcoded allowlist, so stale call sites fail lint in the dependency-bump + * PR itself. The loud throw below fails the whole lint run if inquirer ever + * changes the registry's API shape, rather than silently disabling the rule. */ const { default: inquirer } = await import("inquirer"); const REGISTERED_PROMPT_TYPES = new Set(Object.keys(inquirer.prompt.prompts)); @@ -55,7 +37,6 @@ if (REGISTERED_PROMPT_TYPES.size === 0) { ); } -/** True when the callee is exactly `inquirer.prompt`. */ function isInquirerPromptCallee(callee: AstNode | undefined): boolean { if (callee?.type !== "MemberExpression") return false; const object = asNode(callee.object); @@ -145,7 +126,10 @@ const validInquirerPromptType = { const plugin = { meta: { name: "archgate" }, - rules: { "valid-inquirer-prompt-type": validInquirerPromptType }, + rules: { + "valid-inquirer-prompt-type": validInquirerPromptType, + ...conciseCommentRules, + }, }; export default plugin; diff --git a/.claude/agent-memory/archgate-developer/MEMORY.md b/.claude/agent-memory/archgate-developer/MEMORY.md index 2b4aa5a6..b5a90bd6 100644 --- a/.claude/agent-memory/archgate-developer/MEMORY.md +++ b/.claude/agent-memory/archgate-developer/MEMORY.md @@ -23,7 +23,8 @@ 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`. Never write an ADR rule that only asserts implementation shape — `rules: false` is a valid outcome - [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 +- [Keep code comments and memory entries concise](feedback_concise_comments.md) — code side now machine-enforced by GEN-004 (oxlint `archgate/*` + `archgate check`); memory-entry conciseness still manual +- [Answer every review finding on its own thread](feedback_reply_on_review_threads.md) — especially declined ones; a summary PR comment does not close the loop (recurring miss) - [Throw UserError in boundary-wrapped guards](feedback_throw_usererror_in_guards.md) — not logError + exitWith(1); the action's handleCommandError boundary does that - [Docs are forward-only and version-independent](feedback_forward_only_docs.md) — describe current state; no "previously"/"rather than"/"shipped" framing, no pinned release version or drift-prone counts; git & package.json are the source of truth @@ -59,6 +60,10 @@ Non-enforceable lessons — environment/CI/platform quirks no static rule can re - [i18n translation quality checks](project_i18n_translation_quality.md) — nb/ + pt-br/ dual-locale requirement, Norwegian diacritical corruption patterns to scan for +## Performance + +- [CLI startup baselines](project_cli_perf_baselines.md) — measured numbers behind the `cli-perf.test.ts` budgets, and how to tell a real regression from a slow runner + ## Validation Pipeline - `bun run validate` is the mandatory gate: lint → typecheck → format:check → test → ADR check → knip → build:check diff --git a/.claude/agent-memory/archgate-developer/feedback_concise_comments.md b/.claude/agent-memory/archgate-developer/feedback_concise_comments.md index fed93d83..de4be702 100644 --- a/.claude/agent-memory/archgate-developer/feedback_concise_comments.md +++ b/.claude/agent-memory/archgate-developer/feedback_concise_comments.md @@ -11,7 +11,7 @@ Code comments and memory entries must be concise. Do not write multi-paragraph e **How to apply:** -- Code/workflow comments: one line stating _what_ and, if truly non-obvious, a terse _why_ — not a paragraph with timelines or backstory. Link to a PR/issue/commit for detail instead of inlining it. +- Code/workflow comments: one line stating _what_ and, if truly non-obvious, a terse _why_ — not a paragraph with timelines or backstory. Link to a PR/issue/commit for detail instead of inlining it. Since 2026-07-24 this is machine-enforced by GEN-004 (max 5 prose lines per run, no history/relocation narration) via both `bun run lint` and `archgate check`. - Memory entries (`MEMORY.md` bullets and topic files): lead with the rule in one line; keep **Why:**/**How to apply:** to single short sentences, not multi-clause narratives with timestamps and evidence trails. - If tempted to write a long comment or memory entry to "preserve context," prefer a short pointer (file/PR reference) over inlining the full story. - Applies to all future sessions in this repo — re-check comment/memory length before writing. diff --git a/.claude/agent-memory/archgate-developer/feedback_reply_on_review_threads.md b/.claude/agent-memory/archgate-developer/feedback_reply_on_review_threads.md new file mode 100644 index 00000000..a068ed25 --- /dev/null +++ b/.claude/agent-memory/archgate-developer/feedback_reply_on_review_threads.md @@ -0,0 +1,17 @@ +--- +name: reply-on-review-threads +description: Answer every review finding on its own thread — especially declined ones; a summary PR comment does not close the loop +metadata: + type: feedback +--- + +Reply to each PR review finding **on its own thread**, not only in a summary PR comment. Declined findings especially: the reviewer (human or bot) must see the reasoning attached to the code line they raised. + +**Why:** User feedback 2026-07-25 on PR #496: I addressed 14 CodeRabbit threads with one consolidated PR comment that explained three declines. The user flagged it as recurring — "the ones you declined you must answer at each thread. not the first time this is happening." An unanswered thread stays open and reads as ignored; the reviewer cannot resolve it, and a summary comment is not linked to the line. + +**How to apply:** + +- Find unresolved threads with the GraphQL `reviewThreads.isResolved` query (see [[project-pr-review-thread-triage]]) — REST does not expose resolved state. +- Reply per thread: `gh api repos///pulls//comments//replies -f body='...'` (the reply targets the thread's FIRST comment id). +- Declined → state the reason on the thread (out of scope / came from main / conflicts with an ADR). Accepted → state the fixing commit SHA. A summary comment is optional on top, never a substitute. +- Applies to bot reviewers too — CodeRabbit re-reviews per thread. diff --git a/.claude/agent-memory/archgate-developer/project_cli_perf_baselines.md b/.claude/agent-memory/archgate-developer/project_cli_perf_baselines.md new file mode 100644 index 00000000..f8625d3b --- /dev/null +++ b/.claude/agent-memory/archgate-developer/project_cli_perf_baselines.md @@ -0,0 +1,16 @@ +--- +name: project-cli-perf-baselines +description: Measured CLI startup baselines behind the cli-perf.test.ts budgets, and how to profile when one fires +metadata: + type: project +--- + +Baselines behind the budgets in `tests/integration/cli-perf.test.ts`, measured 2026-05-09 on Windows: `--help` ~260ms, `--version` ~250ms, `adr list` ~400ms, `check` ~750ms. Budgets sit at roughly 3-4x these numbers so they catch regressions without flaking on slow CI runners. + +**Why:** The raw measurements were inlined in the test file and removed by the GEN-004 comment sweep (the budget constants keep their own doc comments; the dated table does not belong in source). They are the only reference point for judging whether a budget failure is a real regression or an environment artifact. + +**How to apply:** + +- A budget firing at ~1.2x baseline is environmental (cold cache, loaded runner); at 2x+ suspect a real regression. +- Profile with `bun --inspect` or by bisecting module imports — startup cost in this CLI is dominated by module parse, which is why ARCH-018 lazy-loads heavy dependencies and why an eager top-level import in `src/cli.ts` is the usual culprit. +- Re-measure and update this entry when the baseline legitimately moves (new eager dependency, Bun upgrade); dated numbers are only useful if the date is honest. diff --git a/.claude/agent-memory/archgate-developer/project_oxlint_gotchas.md b/.claude/agent-memory/archgate-developer/project_oxlint_gotchas.md index e443c78b..9167d7db 100644 --- a/.claude/agent-memory/archgate-developer/project_oxlint_gotchas.md +++ b/.claude/agent-memory/archgate-developer/project_oxlint_gotchas.md @@ -13,5 +13,7 @@ metadata: - **`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. - **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. - **`oxfmt` formats markdown too** — `format:check` runs over ALL files, not just `.ts`, and normalizes markdown (e.g. `*word*` → `_word_`). The `adr-author` skill does not auto-format ADRs it writes; always run `bun run format` after editing ADR/markdown. Tripped CI on PR #372. - **oxfmt eats spaces after inline code spans when the line contains escaped backticks inside a code span** — e.g. `` `UserError("... Run \`archgate init\` first.")` `` mis-parses span boundaries and the re-print collapses `` `code` word`` → `` `code`word`` for the REST of the line. Re-adding the spaces gets re-eaten on the next format pass. Fix the root cause: never nest `\`` escaped backticks inside an inline code span in markdown — rephrase (quote the message as plain text, put commands in their own spans). Tripped CodeRabbit on PR #467 (ARCH-011). diff --git a/.oxlintrc.json b/.oxlintrc.json index cfd495fc..0eabd70a 100644 --- a/.oxlintrc.json +++ b/.oxlintrc.json @@ -15,6 +15,8 @@ "max-nested-callbacks": "off", "max-depth": "off", "archgate/valid-inquirer-prompt-type": "error", + "archgate/no-narration-in-comments": "error", + "archgate/oversized-comment-blocks": "error", "no-await-in-loop": "warn", "no-inline-comments": "off", "prefer-top-level-await": "off", diff --git a/.simple-release.js b/.simple-release.js index fc22ab3e..ca707ee7 100644 --- a/.simple-release.js +++ b/.simple-release.js @@ -4,14 +4,11 @@ import { NpmProject } from "@simple-release/npm"; class ArchgateProject extends NpmProject { /** - * Pre-1.0 semver policy: breaking changes bump the MINOR version, not - * the major. The project ships no support guarantees yet, so v1.0.0 - * must be an explicit decision — force it via the `version` (or `as`) - * bump option when the time comes — never an automatic consequence of - * a `feat!`/BREAKING CHANGE commit landing on main. - * - * `bump()` derives its version from this method, so capping here keeps - * the manifest writes, changelog, release PR title, and tag consistent. + * Pre-1.0 semver policy: breaking changes bump the MINOR version, not the + * major, so v1.0.0 only ships via an explicit `version`/`as` bump option — + * never as an automatic consequence of a `feat!`/BREAKING CHANGE commit. + * `bump()` derives its version from this method, so capping here keeps the + * manifest writes, changelog, release PR title, and tag consistent. */ async getNextVersion(options) { const next = await super.getNextVersion(options); @@ -200,13 +197,10 @@ class ArchgateProject extends NpmProject { } // --------------------------------------------------------------- - // Sync shim package LICENSE.md to the canonical root LICENSE.md - // - // The npm package publishes the root LICENSE directly, so it needs - // no copy. Every other ecosystem ships its own copy that must stay - // byte-identical to root (enforced by ARCH-013/shim-license-sync). - // Registries and pkg.go.dev detect the license from files inside - // the package, not from the repository root. + // Sync shim package LICENSE.md to the canonical root LICENSE.md. + // npm publishes the root LICENSE directly; every other ecosystem + // ships a byte-identical copy (ARCH-013/shim-license-sync) because + // registries detect the license from files inside the package. // --------------------------------------------------------------- const rootLicensePath = "LICENSE.md"; if (existsSync(rootLicensePath)) { diff --git a/lint/expect-expect.ts b/lint/expect-expect.ts index 9105e800..9e2acbe5 100644 --- a/lint/expect-expect.ts +++ b/lint/expect-expect.ts @@ -1,15 +1,11 @@ // SPDX-License-Identifier: Apache-2.0 // Copyright 2026 Archgate -// Custom oxlint JS plugin: enforce that every runnable bun:test test/it call -// contains at least one `expect()` assertion. -// -// Why this exists: oxlint ships `jest/expect-expect`, but its Rust -// implementation only recognizes `jest` and `vitest` imports — it silently -// ignores `bun:test`. This plugin reimplements the rule for the bun:test API -// using oxlint's JS plugins API (https://oxc.rs/docs/guide/usage/linter/js-plugins.html). -// -// The plugin runs natively as TypeScript under Bun, so there is no build step. +// Custom oxlint JS plugin: every runnable bun:test test/it call must contain +// at least one `expect()` assertion. Reimplements `jest/expect-expect` for +// bun:test — the built-in Rust rule only recognizes jest/vitest imports. +// Runs natively as TypeScript under Bun (no build step); JS plugins API: +// https://oxc.rs/docs/guide/usage/linter/js-plugins.html /** Minimal ESTree-ish node shape. The oxlint AST is ESLint-compatible. */ type AstNode = { type: string } & Record; @@ -36,12 +32,8 @@ function isFunctionNode(node: AstNode | undefined): boolean { /** * Resolve the leftmost identifier name of a callee chain. - * - * Examples (callee -> result): - * test -> "test" - * test.skip -> "test" - * test.skipIf(cond) -> "test" - * expect(x).toBe -> "expect" + * Examples: `test` -> "test"; `test.skip` -> "test"; + * `test.skipIf(cond)` -> "test"; `expect(x).toBe` -> "expect". */ function leftmostName(node: AstNode | undefined): string | undefined { let current = node; @@ -68,12 +60,8 @@ function leftmostName(node: AstNode | undefined): string | undefined { /** * Collect the member method names used in a callee chain. - * - * Examples (callee -> result): - * test.skip -> ["skip"] - * test.skipIf(cond) -> ["skipIf"] - * test.each(rows) -> ["each"] - * test -> [] + * Examples: `test.skip` -> ["skip"]; `test.skipIf(cond)` -> ["skipIf"]; + * `test.each(rows)` -> ["each"]; `test` -> []. */ function memberMethods(node: AstNode | undefined): string[] { const methods: string[] = []; diff --git a/lint/no-bare-env-restore.ts b/lint/no-bare-env-restore.ts index f40ca964..62d84ce6 100644 --- a/lint/no-bare-env-restore.ts +++ b/lint/no-bare-env-restore.ts @@ -1,34 +1,15 @@ // SPDX-License-Identifier: Apache-2.0 // Copyright 2026 Archgate -// Custom oxlint JS plugin: enforce that tests restore environment variables -// through `restoreEnv()` rather than a bare `Bun.env.X = original` assignment. -// -// Why this exists: `Bun.env.X = undefined` (and `process.env.X = undefined`) -// assigns the literal STRING "undefined" and leaves the key present — it does -// NOT unset. So the idiomatic-looking capture-and-restore -// const original = Bun.env.HOME; ...; Bun.env.HOME = original; -// silently leaks HOME="undefined" whenever the variable was unset to begin -// with, which is the normal case on Windows for HOME and GIT_CONFIG_GLOBAL. -// Bun's test runner shares ONE process across test files, so the bogus value -// escapes into every later test file and into every subprocess they spawn. -// Nothing else catches this class of bug: -// - tsc cannot: `Bun.env.X = original` is well-typed; the coercion to -// "undefined" happens at runtime. -// - tests cannot: the leak is invisible to the leaking file. It surfaces as -// an unrelated, order-dependent failure in some LATER file — the 2026-07-15 -// incident where a leaked HOME="undefined" made `review-context` report -// zero changed files. -// The invariant is purely syntactic, so it belongs in the linter. -// -// See ARCH-005 (Testing Standards) for the Do/Don't this rule enforces. -// -// The plugin runs natively as TypeScript under Bun, so there is no build step. +// Custom oxlint JS plugin: tests must restore environment variables via +// `restoreEnv()` (tests/test-utils.ts), never a bare `Bun.env.X = original` — +// assigning undefined coerces it to the STRING "undefined" instead of +// unsetting the key, and Bun's single-process test runner leaks that value +// into every later test file. See ARCH-005 and project_test_isolation_gotchas.md. /** Minimal ESTree-ish node shape. The oxlint AST is ESLint-compatible. */ type AstNode = { type: string } & Record; -/** Narrow an unknown value to an AST node (an object with a string `type`). */ function asNode(value: unknown): AstNode | undefined { if ( value !== null && @@ -69,12 +50,14 @@ function staticPropertyName(node: AstNode): string | undefined { } /** - * True when the node is a dotted env access — `Bun.env.NAME` or + * Read the variable name off a dotted env access — `Bun.env.NAME` or * `process.env.NAME`. * - * Computed access (`Bun.env[key]`) is deliberately NOT matched: a dynamic key - * is the shape of a generic helper such as `restoreEnv` itself, not the - * hand-rolled capture-and-restore idiom this rule targets. + * @returns The accessed name (`NAME`), or undefined when the node is not a + * dotted env access. Computed access (`Bun.env[key]`) deliberately returns + * undefined: a dynamic key is the shape of a generic helper such as + * `restoreEnv` itself, not the hand-rolled capture-and-restore idiom this + * rule targets. */ function envVarName(node: AstNode | undefined): string | undefined { if (node?.type !== "MemberExpression") return undefined; @@ -92,13 +75,10 @@ function envVarName(node: AstNode | undefined): string | undefined { /** * Names of local variables that captured an env value, e.g. `originalHome` in - * `const originalHome = Bun.env.HOME`. - * - * This is what separates a restore from an override. Both are spelled - * `Bun.env.HOME = `; only a restore assigns back a value that was - * itself read out of the environment. `Bun.env.HOME = tempDir` (an override - * onto a mkdtemp path) is therefore left alone, with no reliance on a naming - * convention like `original*`. + * `const originalHome = Bun.env.HOME`. Capture separates a restore from an + * override: only `Bun.env.X = ` is flagged, so overrides like + * `Bun.env.HOME = tempDir` are left alone with no `original*` naming + * convention required. */ function collectCapturedNames(root: AstNode): Set { const captured = new Set(); diff --git a/scripts/add-spdx-headers.ts b/scripts/add-spdx-headers.ts index bccae9ad..c9d99996 100644 --- a/scripts/add-spdx-headers.ts +++ b/scripts/add-spdx-headers.ts @@ -24,7 +24,6 @@ for (const pattern of patterns) { for (const match of glob.scanSync({ cwd: ROOT, absolute: true })) { const content = readFileSync(match, "utf-8"); - // Skip if already has SPDX header if (content.includes("SPDX-License-Identifier")) { skipped++; continue; diff --git a/src/cli.ts b/src/cli.ts index 4dbd1a56..6ddc6b91 100755 --- a/src/cli.ts +++ b/src/cli.ts @@ -80,15 +80,11 @@ cleanupStaleBinary(); async function main() { await installGit(); - // Start error tracking and telemetry initialization concurrently but do NOT - // await them here. Both SDKs are lazy-loaded via dynamic `import()` inside - // these functions, so the `ARCHGATE_TELEMETRY=0` path is a cheap no-op. - // - // The promise is awaited in the preAction hook — right before the first - // telemetry event fires — so `repo_id` is always present on `command_executed` - // events (see PR #211). This defers ~150ms of SDK parse + git subprocess cost - // off the critical path for --help, --version, and fast-exit commands that - // never trigger preAction. + // Start error tracking and telemetry initialization concurrently without + // awaiting: the preAction hook awaits this promise right before the first + // telemetry event fires, so `repo_id` is always present on `command_executed` + // events, while paths that never reach preAction (--help, --version) leave + // the ~150ms of SDK parse and repo_id resolution off the critical path. const telemetryReady = Promise.all([initSentry(), initTelemetry()]); const logLevelOption = new Option("--log-level ", "Set log verbosity") @@ -116,7 +112,6 @@ async function main() { // so this await is effectively free in practice. await telemetryReady; - // Apply log level from global option before any command runs const rootOpts = program.opts(); setLogLevel(rootOpts.logLevel); const fullCommand = getFullCommandName(actionCommand); @@ -160,7 +155,6 @@ async function main() { const notice = await updateCheckPromise; if (notice) console.log(notice); - // Flush telemetry and error tracking before exit await Promise.all([flushTelemetry(), flushSentry()]); // Belt-and-braces: force exit so any stray handle left by a third-party diff --git a/src/commands/adr/create.ts b/src/commands/adr/create.ts index 4f3719fd..2b360d5d 100644 --- a/src/commands/adr/create.ts +++ b/src/commands/adr/create.ts @@ -37,7 +37,6 @@ export function registerAdrCreateCommand(adr: Command) { let files: string[] | undefined; let body: string | undefined; - // Non-interactive mode when --title and --domain are provided if (opts.title && opts.domain) { domain = opts.domain; title = opts.title; @@ -54,7 +53,6 @@ export function registerAdrCreateCommand(adr: Command) { // needed for interactive prompts, not for scripted --title/--domain // invocations or --help/--version. const { default: inquirer } = await import("inquirer"); - // Interactive mode const answers = await withPromptFix(() => inquirer.prompt([ { diff --git a/src/commands/adr/import.ts b/src/commands/adr/import.ts index 2987a031..02db245f 100644 --- a/src/commands/adr/import.ts +++ b/src/commands/adr/import.ts @@ -25,8 +25,6 @@ import { import { withPromptFix } from "../../helpers/prompt"; import { ensureRulesShim } from "../../helpers/rules-shim"; -// ---------- Command registration ---------- - export function registerAdrImportCommand(adr: Command) { adr .command("import") @@ -43,7 +41,6 @@ export function registerAdrImportCommand(adr: Command) { const paths = resolvedProjectPaths(projectRoot); const useJson = opts.json || isAgentContext(); - // --list: show previously imported ADRs if (opts.list) { const manifest = await loadImportsManifest(projectRoot); if (useJson) { @@ -64,14 +61,10 @@ export function registerAdrImportCommand(adr: Command) { return; } - // ---------- Resolve & clone ---------- - const cloned = await resolveAndCloneSources(sources); const { resolved } = cloned; tempDirs = cloned.tempDirs; - // ---------- Collect ADR files ---------- - const adrsToImport = await collectAdrsToImport(resolved); if (adrsToImport.length === 0) { @@ -114,8 +107,6 @@ export function registerAdrImportCommand(adr: Command) { console.log(); } - // ---------- Dry run ---------- - if (opts.dryRun) { if (useJson) { console.log( @@ -130,8 +121,6 @@ export function registerAdrImportCommand(adr: Command) { return; } - // ---------- Confirmation ---------- - if (!opts.yes) { const { default: inquirer } = await import("inquirer"); const { confirm } = await withPromptFix(() => @@ -150,18 +139,12 @@ export function registerAdrImportCommand(adr: Command) { } } - // ---------- Atomic write ---------- - await writeImportedAdrs(adrsToImport, idMap, paths.adrsDir); - // ---------- Update imports.json ---------- - const manifest = await loadImportsManifest(projectRoot); updateImportsManifest(manifest, adrsToImport, idMap); saveImportsManifest(projectRoot, manifest); - // ---------- Ensure rules.d.ts ---------- - await ensureRulesShim(projectRoot, paths.adrsDir); // ---------- Summary ---------- diff --git a/src/commands/adr/list.ts b/src/commands/adr/list.ts index 67b69660..dc537ba0 100644 --- a/src/commands/adr/list.ts +++ b/src/commands/adr/list.ts @@ -49,7 +49,6 @@ export function registerAdrListCommand(adr: Command) { return; } - // Filter by domain if specified const filtered = options.domain ? adrs.filter((a) => a.frontmatter.domain === options.domain) : adrs; @@ -65,7 +64,6 @@ export function registerAdrListCommand(adr: Command) { return; } - // Table output const idWidth = 12; const domainWidth = 14; const rulesWidth = 7; diff --git a/src/commands/adr/sync.ts b/src/commands/adr/sync.ts index 1855a9ac..234833b3 100644 --- a/src/commands/adr/sync.ts +++ b/src/commands/adr/sync.ts @@ -26,7 +26,6 @@ interface AdrDiff { localPath: string; upstreamPath: string; hasChanges: boolean; - /** Human-readable summary of what changed */ summary: string; } @@ -58,9 +57,6 @@ function saveImportsManifest( // ---------- Diff helpers ---------- -/** - * Find the local ADR file by ID in the adrs directory. - */ function findLocalAdr(adrsDir: string, adrId: string): string | null { if (!existsSync(adrsDir)) return null; const files = readdirSync(adrsDir); @@ -82,7 +78,6 @@ function diffSummary(localContent: string, upstreamContent: string): string { const changedSections: string[] = []; - // Track which sections have differences const localSections = new Map(); const upstreamSections = new Map(); @@ -108,7 +103,6 @@ function diffSummary(localContent: string, upstreamContent: string): string { } } - // Check for new sections in upstream for (const section of upstreamSections.keys()) { if (!localSections.has(section) && !changedSections.includes(section)) { changedSections.push(section); @@ -162,7 +156,6 @@ export function registerAdrSyncCommand(adr: Command) { return; } - // Filter imports by source args if provided let imports = manifest.imports; if (sources.length > 0) { imports = imports.filter((entry) => @@ -226,7 +219,6 @@ export function registerAdrSyncCommand(adr: Command) { } } - // Compare each ADR in this import entry for (const adrId of entry.adrIds) { result.checked++; @@ -237,7 +229,6 @@ export function registerAdrSyncCommand(adr: Command) { continue; } - // Find the upstream ADR file const upstreamSubpath = resolved.subpath; const upstreamAdrsDir = join(cloneDir, upstreamSubpath, "adrs"); @@ -411,7 +402,6 @@ export function registerAdrSyncCommand(adr: Command) { } if (action === "take") { - // Read upstream content and rewrite the ID to match local const upstreamContent = readFileSync(diff.upstreamPath, "utf-8"); const rewritten = upstreamContent.replace( /^(id:\s*).*$/mu, @@ -431,7 +421,6 @@ export function registerAdrSyncCommand(adr: Command) { } } - // Update imports.json timestamps const updatedManifest = loadImportsManifest(projectRoot); for (const entry of updatedManifest.imports) { if ( diff --git a/src/commands/check.ts b/src/commands/check.ts index a2dd777a..6ff88402 100644 --- a/src/commands/check.ts +++ b/src/commands/check.ts @@ -128,12 +128,11 @@ export function registerCheckCommand(program: Command) { files: filterFiles.length > 0 ? filterFiles : undefined, }); - // Determine output format for telemetry const outputFormat = opts.ci ? "ci" : useJson ? "json" : "console"; // Build the summary once and share it with the reporters, telemetry, - // and exit-code resolver. Previously each of those built its own - // summary — 3 walks over the same result set. + // and exit-code resolver — one walk over the result set instead of + // one per consumer. const summary = buildSummary(result, { maxWarnings }); if (opts.ci) { diff --git a/src/commands/init.ts b/src/commands/init.ts index 332dbff3..81df774e 100644 --- a/src/commands/init.ts +++ b/src/commands/init.ts @@ -63,7 +63,6 @@ export function registerInitCommand(program: Command) { ) .action(async (opts) => { try { - // Resolve editors: explicit flag, interactive prompt, or default let editors: EditorTarget[]; if (opts.editor) { editors = [opts.editor]; @@ -79,8 +78,8 @@ export function registerInitCommand(program: Command) { ); let hasCredentials = (await loadCredentials()) !== null; - // If no credentials and --install-plugin not explicitly set, offer to log in - // Skip interactive prompts in non-TTY environments (agent-driven runs) + // Interactive prompts are skipped in non-TTY environments + // (agent-driven runs). if ( !hasCredentials && opts.installPlugin === undefined && @@ -131,7 +130,6 @@ export function registerInitCommand(program: Command) { } console.log(` ${dir.padEnd(13)}- ${label} settings configured`); - // Plugin install output if (result.plugin?.installed) { console.log(""); if (result.plugin.autoInstalled) { @@ -157,13 +155,10 @@ export function registerInitCommand(program: Command) { }); } - // One-time `project_initialized` event. The hashed `repo_id` ships in - // every event already via the common props; this richer event is the - // only place the raw remote URL / owner / name appear, and only for - // repositories we can confirm public via the host's unauthenticated - // API. Users who don't want the event at all disable telemetry - // (`ARCHGATE_TELEMETRY=0` / `archgate telemetry disable`) — no - // identity-specific knob is needed on top of that. + // One-time `project_initialized` event — the only place the raw remote + // URL / owner / name appear, and only for repositories confirmed public + // via the host's unauthenticated API. Disabling telemetry + // (`ARCHGATE_TELEMETRY=0` / `archgate telemetry disable`) opts out. const repo = await getRepoContext(); const repoPublic = await isPublicRepo(repo); const shareIdentity = shouldShareRepoIdentity(repoPublic); @@ -185,7 +180,6 @@ export function registerInitCommand(program: Command) { : {}), }); - // --- Greenfield wizard: offer starter packs when no ADRs exist --- if (process.stdin.isTTY && !hadExistingProject) { await runGreenfieldWizard(process.cwd()); } @@ -236,7 +230,6 @@ async function runGreenfieldWizard(projectRoot: string): Promise { const stack = await detectStack(projectRoot); - // Show detected stack summary const stackParts: string[] = []; if (stack.languages.length > 0) stackParts.push(...stack.languages); if (stack.runtimes.length > 0) stackParts.push(...stack.runtimes); diff --git a/src/commands/login.ts b/src/commands/login.ts index b3d0d06a..0e0b04a8 100644 --- a/src/commands/login.ts +++ b/src/commands/login.ts @@ -19,7 +19,6 @@ export function registerLoginCommand(program: Command) { login.action(async () => { try { - // Check if already logged in const existing = await loadCredentials(); if (existing) { logInfo( diff --git a/src/commands/plugin/install.ts b/src/commands/plugin/install.ts index a1f7a2aa..d0fa6dc3 100644 --- a/src/commands/plugin/install.ts +++ b/src/commands/plugin/install.ts @@ -36,13 +36,14 @@ const editorOption = new Option( ).choices(EDITOR_TARGETS); /** - * Install the archgate plugin for a single editor. + * Install the archgate plugin for a single editor. Dispatches to the + * editor-specific install function, checks CLI availability, and surfaces + * manual instructions when the CLI is missing. Exported for reuse by the + * `upgrade --plugins` flow. * - * Dispatches to the editor-specific install function, checks CLI availability, - * and surfaces manual instructions when the CLI is missing. Throws on failure - * so callers can collect errors and report them together. - * - * Exported for reuse by the `upgrade --plugins` flow. + * @throws On install failure, so callers can collect errors and report them + * together. + * @see runPluginInstalls */ export async function installForEditor( editor: EditorTarget, @@ -134,11 +135,7 @@ export async function installForEditor( } } -/** - * Print manual installation instructions for a given editor. - * - * Exported for reuse by the `upgrade --plugins` flow. - */ +/** Exported for reuse by the `upgrade --plugins` flow. */ export function printManualInstructions(editor: EditorTarget): void { switch (editor) { case "claude": { @@ -195,12 +192,11 @@ export function printManualInstructions(editor: EditorTarget): void { } /** - * Run plugin installs for a list of editors, collecting failures. - * - * Returns the failure list so callers can decide how to handle them - * (e.g., exit 1 for `plugin install`, or just report for `upgrade`). + * Run plugin installs for a list of editors, collecting failures. Exported + * for reuse by the `upgrade --plugins` flow. * - * Exported for reuse by the `upgrade --plugins` flow. + * @returns The failure list, so callers decide how to handle it — exit 1 for + * `plugin install`, or report only for `upgrade`. */ export async function runPluginInstalls( editors: EditorTarget[], diff --git a/src/commands/upgrade.ts b/src/commands/upgrade.ts index 6bc0fcb0..a968c14b 100644 --- a/src/commands/upgrade.ts +++ b/src/commands/upgrade.ts @@ -220,9 +220,11 @@ function formatBytes(bytes: number): string { /** * Create a progress callback that renders an updating line on stderr. - * Returns `undefined` when stderr is not a TTY (piped / CI) — in that case - * the download runs silently and the existing "Upgrading X -> Y..." message - * is sufficient feedback. Per ARCH-003: no progress output without a TTY. + * + * @returns The callback, or `undefined` when stderr is not a TTY (piped / + * CI) — the download then runs silently, and the existing + * "Upgrading X -> Y..." message is sufficient feedback. + * @see ARCH-003 — no progress output without a TTY. */ function createDownloadProgress(): DownloadProgressCallback | undefined { if (!process.stderr.isTTY) return undefined; @@ -438,7 +440,6 @@ export function registerUpgradeCommand(program: Command) { console.log(`Archgate upgraded to ${latestVersion} successfully.`); - // Offer plugin updates after a successful CLI upgrade await maybeUpdatePlugins(opts.plugins === true); } catch (err) { if (err instanceof Error && err.name === "ExitPromptError") throw err; diff --git a/src/engine/ast-support.ts b/src/engine/ast-support.ts index db5be83d..6d12a6a6 100644 --- a/src/engine/ast-support.ts +++ b/src/engine/ast-support.ts @@ -145,21 +145,11 @@ puts JSON.generate(sexp, max_nesting: false) `; /** - * Serializer used for Ruby `{ comments: true }`: prints the same - * `{"_tree", "comments"}` envelope as the Python with-comments program, with - * comments from a second `Ripper.lex` pass (`Ripper.sexp` carries none). - * `#` comments become `type: "line"` tokens (`#` stripped, newline chomped); - * each `=begin`/`=end` region becomes ONE `type: "block"` token whose value is - * the inner content (marker lines stripped, like TS/JS stripping the - * delimiters) and whose loc spans the `=begin` line through the `=end` line. - * Ripper reports columns as BYTE offsets; they are converted to CHARACTER - * columns (via a byteslice of the source line) so comment locs share the - * Python/TS unit — the sexp tree's own node positions stay byte-based, as - * Ripper emits them. Block values normalize CRLF to LF: Windows text-mode - * reads already strip the CR, so normalizing keeps the value identical - * across OSes. Lex errors on otherwise-parseable source degrade to an empty - * comment list rather than failing the parse, matching Python's - * tokenizer-error fallback. + * Serializer for Ruby `{ comments: true }`: the same `{"_tree", "comments"}` + * envelope as the Python program, with comments from a second `Ripper.lex` + * pass (`Ripper.sexp` carries none). Byte columns convert to character + * columns and block values normalize CRLF so comment locs match Python/TS; + * lex errors degrade to an empty comment list (ARCH-022 has the details). */ export const RUBY_AST_WITH_COMMENTS_PROGRAM = ` source = File.read(ARGV[0], mode: "r:bom|utf-8") @@ -211,12 +201,10 @@ puts JSON.generate({ _tree: sexp, comments: comments }, max_nesting: false) /** * Candidate executable names per language, in probe order. `python3` is not - * a universal PATH alias on Windows (the common installer exposes `python`), - * so the order flips per platform (ARCH-009's isWindows()). Windows also - * probes the `py` launcher last — the python.org installer registers it - * unconditionally even when "Add python.exe to PATH" is left unchecked, and - * the probe already rejects a stale launcher with no registered CPython - * (`py --version` exits non-zero). + * a universal PATH alias on Windows (installers expose `python`), so the + * order flips per platform (ARCH-009). Windows probes the `py` launcher + * last — python.org registers it even when PATH integration is unchecked, + * and the probe rejects a launcher with no registered CPython. */ export function interpreterCandidates(language: "python" | "ruby"): string[] { if (language === "ruby") return ["ruby"]; @@ -229,7 +217,9 @@ export function interpreterCandidates(language: "python" | "ruby"): string[] { * not enough on Windows — the Microsoft Store ships a `python.exe` App * Execution Alias stub that exists on PATH but exits non-zero. * - * Callers cache the returned promise once per `check` invocation. + * @param candidates - Executable names in probe order. + * @returns The first candidate that exits 0, or null when none does. Callers + * cache the returned promise once per `check` invocation. */ export async function probeInterpreter( candidates: string[] @@ -312,6 +302,8 @@ export async function runAstSubprocess( * Parse an AST subprocess's stdout as JSON, mapping malformed output to the * same throw contract as any other `ctx.ast()` failure. Subprocess stdout is * not a file read, so `Bun.file().json()` (ARCH-010) does not apply here. + * + * @throws When stdout is not valid JSON, naming the path and language. */ export function parseAstJson( stdout: string, @@ -329,11 +321,12 @@ export function parseAstJson( /** * Read a file's source at the comparison base revision for - * `ctx.ast(path, lang, { rev: "base" })`, throwing on the two cases the AST - * contract must never paper over: no base is resolvable, or the path did not - * exist at the base. Both throw rather than returning null — a silent miss - * would let a rule report a false "no change." `displayPath` is the - * caller-facing path used in error messages. + * `ctx.ast(path, lang, { rev: "base" })`. + * + * @param displayPath - Caller-facing path used in error messages. + * @throws When no base is resolvable, or the path is absent at the base — + * never returns null, because a silent miss would let a rule report a false + * "no change". */ export async function readBaseSourceOrThrow( projectRoot: string, @@ -356,23 +349,14 @@ export async function readBaseSourceOrThrow( } /** - * Write source to a throwaway temp file and return its path plus a cleanup - * thunk. Used only for `ctx.ast(path, lang, { rev: "base" })` on Python/Ruby: - * the base revision's content is not on disk, but the interpreter serializers - * read a file path from argv. Writing it to a temp file lets the existing, - * unchanged `PYTHON_AST_PROGRAM`/`RUBY_AST_PROGRAM` (and the mandatory `-I` - * isolation) parse it without a second code path. + * Write source to a throwaway temp file for `{ rev: "base" }` Python/Ruby + * parses — the interpreter serializers read a file path from argv. A private + * `mkdtempSync` dir (0700) plus exclusive create (`wx`, 0600) defeats + * shared-tmpdir symlink attacks and keeps base source owner-only (ARCH-022). * - * Security: the file goes in a per-call private directory created with - * `mkdtempSync` (0700, owner-only, unpredictable name), and is created - * exclusively (`wx`) with mode `0600`. This closes the shared-`tmpdir` attacks - * a predictable name would expose on a multi-user host: a pre-planted symlink - * at the path can no longer redirect the write (exclusive create fails on an - * existing entry, and the private parent is not writable by others), and the - * base source — which may be sensitive — is never world-readable or left where - * another user could read it. It also lives outside any interpreter's - * cwd-derived load path even before `-I` is considered. `cleanup()` removes the - * whole private directory, is best-effort, and never throws. + * @returns The temp file `path`, and a `cleanup()` thunk that removes the + * whole dir — best-effort, and never throws. + * @see ARCH-022 */ export function writeTempSourceFile( content: string, @@ -427,12 +411,11 @@ export async function materializeAstInput(args: { } /** - * Fold the `{ comments: true }` Python/Ruby subprocess output back into the - * shape `ctx.ast()` promises. Those serializers print `{ _tree, comments }`; - * unwrap it to the tree with `comments` attached, so the return shape matches - * the ESTree one (a tree carrying a `comments` array). Ruby's tree is an - * array, so `comments` rides on it as a non-index property. For every other - * case the subprocess output is already the tree — pass it through untouched. + * Fold the `{ comments: true }` Python/Ruby subprocess envelope + * (`{ _tree, comments }`) back into the shape `ctx.ast()` promises: the tree + * with `comments` attached, matching ESTree. Ruby's array tree carries + * `comments` as a non-index property; every other output is already the + * tree and passes through untouched. */ export function finalizeAstResult( parsed: Record | unknown[], @@ -450,16 +433,11 @@ export function finalizeAstResult( } /** - * `ctx.findAstNodes()`: collect every node in a parsed AST whose - * type-discriminant field matches one of `types`. Language-agnostic — each - * object node is checked against whichever discriminant field it carries: - * `_type` (Python) or `type` (ESTree TypeScript/JavaScript). Own-enumerable - * object values and arrays are traversed, and `tree` itself is a match - * candidate. Ruby's `Ripper.sexp` nodes are plain arrays with no object - * discriminant field, so a Ruby tree is traversed but its array-shaped nodes - * never match. The traversal is iterative (explicit stack, preorder) so a - * deeply nested tree cannot overflow the call stack, and a visited set - * guards against cycles — cheap insurance, real ASTs are acyclic. + * `ctx.findAstNodes()`: collect every node whose type-discriminant field + * (`_type` Python, `type` ESTree TS/JS) matches one of `types`; `tree` + * itself is a candidate. Ruby's array-shaped sexp nodes carry no + * discriminant, so a Ruby tree traverses but never matches. Iterative + * preorder with a visited set — deep trees cannot overflow the call stack. */ export function findAstNodes( tree: EsTreeNode, diff --git a/src/engine/context.ts b/src/engine/context.ts index 011ee398..975b1c2c 100644 --- a/src/engine/context.ts +++ b/src/engine/context.ts @@ -98,14 +98,9 @@ function truncateSection( /** * Identify an ADR, and — when `briefings` is set — include its Decision and - * Do's/Don'ts prose. - * - * That prose is ~78% of a review context on a repo of any size (62KB of 80KB - * here) and grows with the number of matched ADRs, which pushes the payload past - * the point where agent harnesses spill it to a file (ARCH-003 §7). It is - * therefore opt-in: the default identifies which ADRs apply, and the consumer - * drills into the ones it needs with `archgate adr show `. Skipping the - * prose also skips `extractAdrSections` entirely, so the lean path is cheaper. + * Do's/Don'ts prose. The prose dominates review-context payload size, so it + * is opt-in (ARCH-003 §7): the default identifies applicable ADRs and the + * consumer drills in via `archgate adr show `. */ export function briefAdr( adr: AdrDocument, diff --git a/src/engine/git-files.ts b/src/engine/git-files.ts index 67f73974..0abd10c7 100644 --- a/src/engine/git-files.ts +++ b/src/engine/git-files.ts @@ -48,15 +48,11 @@ export function getGitTrackedFiles( if (cached) return cached; const promise = (async () => { - // `--cached` lists files deleted from the worktree but not yet staged; - // a filesystem walk would never return those. Subtract `--deleted` so - // in-memory pattern matching (matchTrackedFiles) sees exactly the - // files that exist on disk. `--others` entries exist by definition. - // - // allSettled, not all: in a non-git directory the first spawn rejects, - // and Promise.all would return while the sibling git process is still - // running with projectRoot as its cwd — on Windows that live cwd handle - // locks the directory (EBUSY on removal). Both must fully exit first. + // Subtract `--deleted` from `--cached` so in-memory pattern matching + // (matchTrackedFiles) sees exactly the files on disk (ARCH-023). + // allSettled, not all: both git processes must fully exit before this + // returns — a still-running child's cwd handle locks the directory on + // Windows (EBUSY on removal). const [listed, deleted] = await Promise.allSettled([ runGit( ["ls-files", "--cached", "--others", "--exclude-standard"], @@ -165,7 +161,6 @@ export async function resolveScopedFiles( return all; } -/** Get changed files from git staging area. */ export async function getStagedFiles(projectRoot: string): Promise { try { const result = await runGit( @@ -199,13 +194,12 @@ export async function getChangedFiles(projectRoot: string): Promise { } /** - * Detect the base ref to compare against for branch-level change detection. + * Detect the base ref for branch-level change detection. * - * Resolution order: - * 1. Remote HEAD symref (e.g. `origin/main`) — fast, local, no network - * 2. `origin/main` or `origin/master` tracking refs - * 3. Local `main` or `master` branches (repos without remotes) - * 4. `null` — detection failed, caller falls back to empty changedFiles + * @returns The first ref that resolves, tried in order: remote HEAD symref + * (e.g. `origin/main`), `origin/main`/`origin/master` tracking refs, then + * local `main`/`master` for repos without remotes. Null when detection + * fails, and the caller falls back to empty `changedFiles`. */ export async function detectBaseRef( projectRoot: string @@ -285,17 +279,13 @@ export async function resolveBaseRef( } /** - * Get files changed between a base ref and the working tree. + * Get files changed between a base ref and the working tree. Unions committed + * branch changes (`git diff base...HEAD`), staged + unstaged edits, and + * untracked files so uncommitted work is never silently omitted + * (archgate/cli#403). * - * Unions three sources so uncommitted work is never silently omitted - * (see archgate/cli#403): - * 1. `git diff base...HEAD` — committed branch changes (three-dot diff - * finds the merge-base automatically) - * 2. staged + unstaged edits to tracked files - * 3. untracked (non-gitignored) files - * - * Returns an empty array when the ref cannot be diffed (bad ref or not - * a git repo), matching the previous behavior. + * @returns Changed file paths, or an empty array when the ref cannot be + * diffed (bad ref or not a git repo). */ export async function getFilesChangedSinceRef( projectRoot: string, @@ -345,14 +335,13 @@ async function runGitOrNull( } /** - * Resolve the merge base of `ref` and HEAD — the commit `changedFiles` - * compares against. + * Resolve the merge base of `ref` and HEAD — the same commit the three-dot + * `ref...HEAD` diff in `getFilesChangedSinceRef` resolves against, so + * base-revision reads compare against the exact base of the change set. * - * `getFilesChangedSinceRef` diffs `ref...HEAD` (three-dot), which git resolves - * against `merge-base(ref, HEAD)`. Base-revision reads MUST use that same - * commit, or a rule would compare the working tree against a different point - * than the change set it was handed. Returns null when no merge base exists - * (unrelated histories) or `ref` is unknown. + * @returns The merge-base SHA, or null when no merge base exists (unrelated + * histories) or `ref` is unknown. + * @see getFilesChangedSinceRef */ export async function getMergeBase( projectRoot: string, @@ -366,13 +355,14 @@ export async function getMergeBase( /** * Read a file's contents at a specific revision via `git show :`. * - * Returns null when the path did not exist at that revision (an added file) or - * the revision is unresolvable — callers distinguish "absent at base" from a - * present-but-empty file (which returns ""). - * - * `path` MUST be repo-relative with forward slashes, exactly as it appears in - * `changedFiles`/`scopedFiles`. Passed as an array argument (no shell), so a - * path with spaces or shell metacharacters is safe. + * @param projectRoot - Repository root the git command runs in. + * @param rev - Revision to read from, typically a merge-base SHA. + * @param path - Repo-relative path with forward slashes, exactly as it appears + * in `changedFiles`/`scopedFiles`. Passed as an array argument (no shell), so + * spaces and shell metacharacters are safe. + * @returns The file's content, or null when the path is absent at that + * revision (an added file) or the revision is unresolvable. A present but + * empty file returns `""`, which callers must distinguish from null. */ export function getFileAtRev( projectRoot: string, diff --git a/src/engine/glob-utils.ts b/src/engine/glob-utils.ts index 52497518..e5c3b770 100644 --- a/src/engine/glob-utils.ts +++ b/src/engine/glob-utils.ts @@ -10,7 +10,7 @@ import { UserError } from "../helpers/user-error"; /** * Find every line of `content` matching `pattern`, as 1-based line/column * `GrepMatch`es labelled with `file`. Shared by `ctx.grep` (one file) and - * `ctx.grepFiles` (many), which otherwise duplicated this scan. + * `ctx.grepFiles` (many). */ export function matchLines( content: string, @@ -20,7 +20,7 @@ export function matchLines( const lines = content.split("\n"); // Clone the pattern and drive it with `exec()`, resetting `lastIndex` per // line. `String.prototype.match` with a global (`/g`) pattern returns every - // match but strips the `index`, which collapsed the reported column to 1; + // match but strips the `index`, collapsing the reported column to 1; // `exec()` always carries `index`. Cloning also keeps a caller's stateful // `/g` regex from leaking `lastIndex` across our per-line scan. const linePattern = new RegExp(pattern.source, pattern.flags); @@ -42,6 +42,8 @@ export function matchLines( /** * Validate that a glob pattern cannot escape projectRoot via `..` segments. + * + * @throws {UserError} When the pattern contains `..` or is absolute. */ function safeGlob(pattern: string): void { if (pattern.includes("..")) { @@ -57,17 +59,12 @@ function safeGlob(pattern: string): void { } /** - * Expand brace patterns that contain path separators into separate patterns. - * - * Bun.Glob scanning silently returns empty results for brace groups whose - * alternatives contain `/` (e.g. `svc/{src/env.ts,env.ts}`). match() handles - * them correctly — only the scanner is broken. Filed upstream as - * https://github.com/oven-sh/bun/issues/32596. + * Expand brace groups whose alternatives contain `/` into separate patterns, + * since Bun.Glob scanning silently returns empty results for those. Braces + * with only simple alternatives are left for Bun.Glob to handle natively. * - * This function detects `{alt1,alt2,...}` groups where at least one alternative - * contains `/` and expands them into separate patterns so each one can be - * scanned individually. Braces whose alternatives are all simple names (no `/`) - * are left for Bun.Glob to handle natively. + * @see https://github.com/oven-sh/bun/issues/32596 — scanning only; `match()` + * is unaffected. */ export function expandBracePattern(pattern: string): string[] { const match = pattern.match(/^(.*?)\{([^{}]+)\}(.*)$/u); @@ -91,15 +88,12 @@ export function expandBracePattern(pattern: string): string[] { /** * Match glob patterns against the git-tracked file list in memory instead of - * walking the filesystem. On large projects a directory walk visits every - * entry under node_modules/, .venv/, data/, etc. only to discard them against - * `trackedFiles` afterwards — per pattern, per rule. Matching the (much - * smaller) tracked list directly eliminates that traversal entirely. + * walking the filesystem. `Bun.Glob#match()` matches dot-prefixed segments + * without options and handles `/`-containing brace groups, so patterns come + * in unexpanded. * - * `Bun.Glob#match()` matches dot-prefixed path segments without any option - * (unlike directory scanning, see ARCH-020) and handles brace groups with - * path separators correctly (oven-sh/bun#32596 only affects scanning), so - * callers may pass patterns unexpanded. + * @see ARCH-023 — why in-memory matching is both faster and simpler. + * @see ARCH-020 — scanning needs `dot: true`, matching does not. */ export function matchTrackedFiles( patterns: string[], @@ -115,16 +109,10 @@ export function matchTrackedFiles( /** * List project files matching a rule-supplied glob pattern, sorted and - * `/`-normalized. - * - * The pattern and every brace-expanded alternative are validated first — + * `/`-normalized. Every brace-expanded alternative is validated first — * expansion can surface absolute or `..` alternatives hidden inside a brace - * group (e.g. `{/etc/passwd,src/a.ts}`), and the sandbox contract must hold - * on both paths below. - * - * Fast path: match against the git-tracked file list in memory — avoids - * walking ignored trees (node_modules/, .venv/, ...) only to discard them - * afterwards. Fallback (no git repo): walk the filesystem. + * group, and the sandbox contract must hold on both paths below. Fast path: + * in-memory match against git-tracked files (ARCH-023); fallback: walk. */ export async function listMatchingFiles( projectRoot: string, diff --git a/src/engine/js-parser.ts b/src/engine/js-parser.ts index e5a54b8d..78a002f3 100644 --- a/src/engine/js-parser.ts +++ b/src/engine/js-parser.ts @@ -13,17 +13,10 @@ import type { CommentToken } from "../formats/rules"; export type MeriyahProgram = ReturnType; /** - * Parse JavaScript source into an ESTree AST via meriyah. - * - * This is the single sanctioned meriyah call site, shared by the rule-file - * sandbox scanner (`rule-scanner.ts`) and the `ctx.ast()` - * TypeScript/JavaScript branch in `runner.ts` — per ARCH-022, the parse - * call must not be duplicated inline at each consumer. - * - * `sourceType: "script"` parses sloppy-mode CommonJS (used for `.cjs` - * files, which cannot legally contain import/export in Node). It enables - * `globalReturn` because Node allows top-level `return` in CJS modules. - * + * Parse JavaScript source into an ESTree AST via meriyah. The single + * sanctioned meriyah call site per ARCH-022, shared by `rule-scanner.ts` and + * `ctx.ast()` in `runner.ts`. `sourceType: "script"` parses sloppy-mode + * CommonJS with `globalReturn` (Node allows top-level `return` in CJS). * Throws on syntax errors; callers decide how to surface them. */ export function parseJsModule( @@ -45,13 +38,9 @@ export function parseJsModule( /** * Extract `//` line and `/* … *​/` block comments from TypeScript/JavaScript * source, with delimiter-stripped text and original-source positions (0-based - * columns, matching ESTree `loc` and Python `col_offset`). - * - * String and template literals are skipped so a `//` or `/*` inside a string is - * not mistaken for a comment. Regular-expression literals are NOT tracked, so a - * comment delimiter inside a regex (e.g. `/foo\/\//`) is a known blind spot — - * acceptable for the comment-governance rules this serves, and consistent with - * the scanner in `source-positions.ts`. + * columns, matching ESTree `loc` and Python `col_offset`). String and + * template literals are skipped; regex literals are NOT tracked — a known + * blind spot shared with the scanner in `source-positions.ts`. */ export function extractJsComments(source: string): CommentToken[] { const comments: CommentToken[] = []; @@ -128,17 +117,11 @@ export function extractJsComments(source: string): CommentToken[] { } /** - * Parse TypeScript/JavaScript *source* into an ESTree AST, selecting the right - * transpile/parse mode from the file extension. Shared by `ctx.ast()`'s TS/JS - * branch for both working-tree and base-revision (`{ rev: "base" }`) source. - * - * TypeScript is transpiled by `Bun.Transpiler` first (which strips types and - * comments — see ARCH-022 on why `loc` is transpiled-relative for TS). `.cts` - * and `.cjs` are CommonJS and parse as sloppy-mode scripts; `.jsx` enables JSX. - * - * With `collectComments`, a `comments` array is attached to the returned tree, - * extracted from the ORIGINAL `source` (so it survives TS transpilation and - * carries original-source positions, unlike the tree's own `loc`). + * Parse TypeScript/JavaScript *source* into an ESTree AST for `ctx.ast()`, + * selecting the mode from the file extension: TS goes through + * `Bun.Transpiler` (ARCH-022: `loc` is transpiled-relative), `.cts`/`.cjs` + * parse as sloppy scripts, `.jsx` enables JSX. `collectComments` attaches + * `comments` read from the ORIGINAL source, carrying original positions. */ export function parseTsOrJsSource( language: "typescript" | "javascript", diff --git a/src/engine/loader.ts b/src/engine/loader.ts index d7bae445..5fcd5bb4 100644 --- a/src/engine/loader.ts +++ b/src/engine/loader.ts @@ -55,7 +55,6 @@ export type LoadResult = | { type: "loaded"; value: LoadedAdr } | { type: "blocked"; value: BlockedAdr }; -/** Convert a BlockedAdr into a RuleResult-shaped object for reporting. */ export function blockedToRuleResult(projectRoot: string, b: BlockedAdr) { const id = b.adr.frontmatter.id; const isSyntax = b.error.includes("syntax convention"); @@ -94,17 +93,13 @@ interface SyntaxViolation { /** * Check that a `.rules.ts` file follows the required syntax conventions: - * 1. Triple-slash reference directive: `/// ` - * pointing to `rules.d.ts` (provides ambient types without imports). - * 2. `satisfies RuleSet` on the default export (compile-time validation). - * - * These are authoring conventions that ensure rule files get proper - * type-checking and remain self-documenting. + * a triple-slash reference to `rules.d.ts` (ambient types without imports) + * and a `satisfies RuleSet` clause (compile-time validation). Both are + * presence checks over the source text, not placement checks. */ function checkRuleSyntax(source: string): SyntaxViolation[] { const violations: SyntaxViolation[] = []; - // Check for triple-slash reference to rules.d.ts const hasTripleSlash = /^\/\/\/\s*$/mu.test( source @@ -121,7 +116,6 @@ function checkRuleSyntax(source: string): SyntaxViolation[] { }); } - // Check for `satisfies RuleSet` on the default export const hasSatisfies = /\bsatisfies\s+RuleSet\b/u.test(source); if (!hasSatisfies) { // Point to the last line as a reasonable location for the missing satisfies @@ -146,20 +140,19 @@ interface ParsedAdrEntry { } /** - * Process-level cache of `readdir + read + parse` for each project root. - * `archgate review-context --run-checks` used to parse every ADR twice - * (once for briefings, once for rule loading); the cache lets both paths - * share the I/O. `archgate check` + `adr list` benefit too. - * - * Cache lifetime is per-process — consistent with other per-invocation - * caches in this codebase (git ls-files, repo context, install method). + * Process-level cache of `readdir + read + parse` per project root, so + * briefings and rule loading share one parse pass in the same invocation. + * Per-process lifetime is consistent with the other per-invocation caches + * here (git ls-files, repo context, install method). */ const parsedAdrsCache = new Map>(); /** * Read and parse every ADR markdown file in the project, caching the result - * per-process. Returns entries in directory order. Unparseable files are - * silently skipped (logged at debug level). + * per-process. + * + * @returns Entries in directory order. Unparseable files are silently + * skipped, logged at debug level. */ export function parseAllAdrs(projectRoot: string): Promise { const cached = parsedAdrsCache.get(projectRoot); @@ -245,7 +238,6 @@ export async function loadRuleAdrs( // Phase 1: Read and parse all ADR files in parallel (cached per process) const parsedAdrs = await parseAllAdrs(projectRoot); - // Filter to ADRs that have rules enabled const ruleAdrs = parsedAdrs.filter((entry) => { if (!entry.adr.frontmatter.rules) return false; if (filterAdrId && entry.adr.frontmatter.id !== filterAdrId) return false; diff --git a/src/engine/reporter.ts b/src/engine/reporter.ts index b27f8d6b..1917efb6 100644 --- a/src/engine/reporter.ts +++ b/src/engine/reporter.ts @@ -46,14 +46,10 @@ export interface ReportSummary { /** * Rules that have something to report: failures, rule errors, and anything - * carrying violations. - * - * The `violations.length` half is load-bearing — `buildSummary` sets status - * "fail" only for error-severity violations, so a warning- or info-only rule is - * status "pass" with a non-empty violations[]. Filtering on status alone would - * silently swallow every warning (ARCH-003: don't decide omission from a status - * field alone). Shared by every consumer that projects results for an agent, so - * the two call sites cannot drift apart on that subtlety. + * carrying violations. The `violations.length` half is load-bearing — a + * warning- or info-only rule is status "pass" with a non-empty violations[], + * so filtering on status alone would silently swallow every warning + * (ARCH-003: don't decide omission from a status field alone). */ export function resultsWithFindings( results: ReportSummary["results"] @@ -190,7 +186,6 @@ export function reportConsole( ); } - // Print violations for (const v of r.violations) { const loc = v.file ? (v.line ? `${v.file}:${v.line}` : v.file) : ""; const sevColor = @@ -217,7 +212,6 @@ export function reportConsole( } } - // Print suppression warnings for (const w of summary.suppressionWarnings) { const loc = w.line ? `${w.file}:${w.line}` : w.file; console.log( @@ -225,7 +219,6 @@ export function reportConsole( ); } - // Summary line console.log(); const parts: string[] = []; if (summary.passed > 0) @@ -264,23 +257,17 @@ export function reportConsole( } /** - * Output results as JSON. - * - * `results` carries only rules that have something to report — failures, rule - * errors, and anything with violations (including warning- and info-only rules, - * which are status "pass"). A clean rule's entry is pure restatement of static - * ADR text, and on a large project those entries are ~99% of the payload (25KB - * across 84 rules), which pushes the output past the threshold where agent - * harnesses spill a tool result to a file and stop showing it inline - * (ARCH-003 §7). Omitting them makes the payload scale with the number of - * findings rather than the number of rules. The summary counts above still - * report exactly how many rules passed, so no information is lost — and - * `--verbose` restores the full list, matching both `reportConsole` and the - * flag's documented meaning ("Show passing rules and timing info"). + * Output results as JSON. `results` carries only rules with something to + * report — clean-rule entries restate static ADR text and would push the + * payload past the spill threshold of ARCH-003 §7. * - * @param forcePretty - When true, always pretty-print (e.g., explicit --json flag). - * When omitted, format is auto-detected based on TTY/CI context. - * @param verbose - When true, include passing rules in `results`. + * @param result - Raw check result to summarize and print. + * @param forcePretty - Always pretty-print (explicit `--json` flag). When + * omitted, format is auto-detected from TTY/CI context. + * @param summary - Pre-built summary; defaults to `buildSummary(result)`. + * @param verbose - Include passing rules in `results` rather than only those + * with findings. + * @see resultsWithFindings */ export function reportJSON( result: CheckResult, @@ -323,7 +310,6 @@ export function reportCI( } } - // Suppression warnings for (const w of summary.suppressionWarnings) { const filePart = w.file ? ` file=${w.file}` : ""; const linePart = w.line ? `,line=${w.line}` : ""; @@ -332,7 +318,6 @@ export function reportCI( ); } - // Also output summary const status = summary.pass ? "check passed" : "check failed"; console.log( `\n${status}: ${summary.passed} passed, ${summary.failed} failed, ${summary.warnings} warnings` diff --git a/src/engine/rule-scanner.ts b/src/engine/rule-scanner.ts index 09d73d35..554b1268 100644 --- a/src/engine/rule-scanner.ts +++ b/src/engine/rule-scanner.ts @@ -5,24 +5,11 @@ import { z } from "zod"; import { parseJsModule, type MeriyahProgram } from "./js-parser"; /** - * Module specifiers a rule file is permitted to import. - * - * This is an ALLOWLIST, and deliberately so. A denylist of "dangerous" modules - * is not a viable boundary here: `.rules.ts` files are imported and executed - * in-process by `archgate check`, so reaching *any* module outside this set is - * arbitrary code execution, and the ways to name one are effectively unbounded - * — `data:text/javascript,...` URLs, relative paths to files the scanner never - * sees, bare npm packages, and `node:module`'s `createRequire` all resolve to - * executable code without naming a banned builtin. Enumerating those is - * unwinnable; enumerating the handful of safe modules is not. - * - * Only `node:`-prefixed specifiers are allowed. The bare forms (`path`) are - * shadowable by a `node_modules/path` package in the *target* project, which - * would hand execution straight back to the untrusted code this scanner exists - * to contain; the `node:` scheme always resolves to the built-in. - * - * Type-only imports need no entry here — `Bun.Transpiler` erases them before - * this scanner ever sees the AST. + * Module specifiers a rule file may import — an allowlist, because a denylist + * is unwinnable: rule files execute in-process, so reaching ANY module outside + * this set is arbitrary code execution (data: URLs, relative paths, bare npm + * packages, createRequire). Only `node:`-prefixed forms qualify — bare names + * are shadowable by a target-project `node_modules` package. See ARCH-024. */ const ALLOWED_MODULES = new Set([ "node:path", @@ -32,26 +19,11 @@ const ALLOWED_MODULES = new Set([ ]); /** - * Live globals in the rule runtime whose mere *naming* is blocked — in any - * position, code only (a same-named property key or string is fine). - * - * This is the module allowlist's logic (above) applied to globals. A rule file - * runs in-process, so `Bun`, `process`, and the global object are live and - * expose subprocess/filesystem/network/native capabilities directly — no import - * needed. Blocking specific *shapes* of reaching them (`Bun.spawn`, `Bun[x]`) - * is the same losing game as a module denylist: `const B = Bun; B.spawn(...)`, - * `const { spawn } = Bun`, `Reflect.get(Bun, "spawn")`, and `global.Bun.spawn` - * all reach the identical capability without matching any of those shapes. - * Enumerating the evasions is unwinnable; refusing to let rule code *name* the - * capability source is not. Rules touch the project only through `ctx`. - * - * Grouped by what each reaches: - * - the global object and its aliases, and reflection over it; - * - dynamic code execution (`eval`, and `Function` — the `Function` constructor - * is `eval`), which also subsumes the `.constructor` chain blocked below; - * - network; - * - module loading (`require`; `import.meta.require` is handled separately as it - * is a MetaProperty member, not a bare identifier). + * Live globals whose mere NAMING is blocked in rule code (a same-named + * property key or string is fine). Blocking specific access shapes + * (`Bun.spawn`, aliases, `Reflect.get`) is unwinnable, so rule code may not + * name a capability source at all — rules touch the project only through + * `ctx`. The grouping and the `import.meta.require` case live in ARCH-024. */ const BANNED_GLOBALS = new Set([ "globalThis", @@ -70,24 +42,11 @@ const BANNED_GLOBALS = new Set([ ]); /** - * Characters that let source render differently from how it parses. - * - * This is the one class of problem the AST cannot see, and the reason this - * scanner also inspects raw text. The parser resolves the true meaning, so a - * bidi override or an invisible character is invisible to it *by design*; the - * target of the attack is the human reading the diff -- a reviewer approving an - * imported rule pack -- not the parser. See "Trojan Source" (CVE-2021-42574). - * - * A denylist is legitimate here, unlike for module specifiers: this set is - * closed by the Unicode specification rather than by our imagination. - * - * Keyed by code point on purpose. Spelling these as literal characters would - * put the very things this scanner rejects into its own source, where no - * reviewer could see them -- and writing them as escapes is not enough either, - * since a formatter may normalise an escape back into the literal character. - * (Drafting this comment did exactly that: a U+202E written as an escape came - * back as an invisible override sitting in this paragraph.) A number cannot be - * made invisible, so the code points are spelled numerically and never as text. + * Characters that render differently from how they parse ("Trojan Source", + * CVE-2021-42574). The attack targets the human reviewing a rule pack, not the + * parser — hence the raw-text pass. A denylist is sound here: the Unicode spec + * closes the set. Keyed by NUMERIC code point on purpose: literal characters + * would hide in this very source, and formatters normalise escapes back. */ const INVISIBLE_CHARS = new Map([ [0x202a, "LEFT-TO-RIGHT EMBEDDING"], @@ -125,21 +84,11 @@ export interface ScanViolation { } /** - * Scan raw source text, before transpilation or parsing. - * - * This is deliberately NOT a text search for dangerous names. Such a search - * would be strictly worse than the AST walk that follows it: the parser - * resolves escapes, so `import("node:child_process")` is caught by the - * module allowlist even though its raw text never contains the string - * "node:child_process". A regex looking for that string would miss it. Text - * matching also cannot tell code from data, and rule files legitimately - * contain dangerous-looking strings as the patterns they search *for* — - * ARCH-007's and ARCH-022's own rules mention `child_process` by name. - * - * What this pass catches is the one thing the AST cannot: characters that make - * the rendered source differ from the parsed source. The AST is blind to them - * because it sees the true program, which is exactly the point — the target is - * the human reviewing an imported rule pack. + * Scan raw source text, before transpilation or parsing. Deliberately NOT a + * text search for dangerous names — the AST walk is stronger there (the parser + * resolves escapes, and rule files legitimately contain dangerous-looking + * strings as the patterns they search for). This pass catches the one thing + * the AST cannot see: characters that render differently from how they parse. */ function scanSourceText(source: string): ScanViolation[] { const violations: ScanViolation[] = []; @@ -183,12 +132,11 @@ function scanSourceText(source: string): ScanViolation[] { interface AstNode { type: string; name?: string; - // A literal's `value` can be a string, number, boolean, null, or — for exotic - // literals — a `bigint` (`123n`) or a plain object (a `RegExpLiteral` carries - // `value: {}`). It is only ever *read* through `typeof … === "string"` guards - // (`staticPropName`, `checkModuleSpecifier`), so its type is intentionally - // wide: the schema must never reject a node over a value shape it does not - // consume. See the schema note below on why a rejected node is a security bug. + // A literal's `value` may be string/number/boolean/null, a `bigint` (`123n`), + // or a plain object (`RegExpLiteral`). It is only read through + // `typeof … === "string"` guards, so the type stays intentionally wide — the + // schema must never reject a node over a value shape it does not consume + // (see the schema note below: a rejected node is a silently unscanned one). value?: unknown; computed?: boolean; source?: AstNode | null; @@ -203,24 +151,18 @@ const AstNodeSchema: z.ZodType = z .object({ type: z.string(), name: z.string().optional(), - // A node dropped by `safeParse` is dropped *with its entire subtree* from - // the walk — a silent false-negative, exactly the failure mode the null - // `source` fix (below) addresses. `type` is always present on an ESTree - // node and every typed child field recurses back into this schema, so a - // literal's `value` is the only leaf that can make a node fail to parse. - // Meriyah emits a `bigint` for `123n` and an object for a `RegExpLiteral` - // (`/x/.constructor.constructor` reaches the Function constructor = eval), - // neither of which a narrow union admits. `value` is read only through - // `typeof`-guarded string checks, so accept anything and never reject a - // node — and thus never skip scanning its children — over its value shape. + // A node `safeParse` rejects is dropped WITH ITS ENTIRE SUBTREE — a silent + // false negative (ARCH-024). `type` is always present and every typed + // child recurses into this schema, so a literal's `value` is the only leaf + // that can fail: meriyah emits `bigint` for `123n` and an object for a + // RegExpLiteral. Accept anything; never skip children over a value shape. value: z.unknown().optional(), computed: z.boolean().optional(), - // ESTree sets `source: null` on an `export` declaration that has no `from` - // clause (`export function`, `export const`, `export { local }`). Without - // `.nullable()` the whole node fails validation and `parseNode` drops it — - // silently skipping every child, so anything dangerous inside a top-level - // `export`-declaration would go unscanned. Tolerating null keeps the node - // in the walk; `checkModuleSpecifier` still correctly no-ops on a null src. + // ESTree sets `source: null` on an `export` declaration with no `from` + // clause. Without `.nullable()` the node fails validation and `parseNode` + // drops it — silently skipping every child inside a top-level export. + // Tolerating null keeps the node in the walk; `checkModuleSpecifier` + // still no-ops on a null source. source: z .lazy(() => AstNodeSchema) .nullable() @@ -232,7 +174,6 @@ const AstNodeSchema: z.ZodType = z }) .passthrough(); -/** Parse an unknown value into an AstNode, or return null. */ function parseNode(value: unknown): AstNode | null { const result = AstNodeSchema.safeParse(value); return result.success ? result.data : null; @@ -241,25 +182,10 @@ function parseNode(value: unknown): AstNode | null { import { remapViolations, type RawViolation } from "./source-positions"; /** - * Scan a `.rules.ts` source string for banned patterns. - * - * Two passes, in this order: - * - * 1. **Raw text** (`scanSourceText`) — the source exactly as written, before - * any transformation, checked for characters that make the rendered code - * differ from the parsed code. This must run on the untransformed text: - * transpiling normalises some of what it looks for. - * 2. **AST** — transpile TypeScript to JavaScript (`Bun.Transpiler`), parse to - * an ESTree tree (`meriyah`), and walk every node for blocked imports, - * globals, and escapes. - * - * The division is deliberate. The AST pass is the stronger of the two for - * anything semantic, *including* obfuscation: the parser resolves escapes and - * constant forms, so it sees `import("node:child_process")` however it is - * spelled. Text matching is reserved for the one thing a parser cannot report, - * because it is defined by what a human sees rather than what the code means. - * - * Returns an empty array if the rule is clean; violations if blocked patterns are found. + * Scan a `.rules.ts` source string for banned patterns; returns [] when clean. + * Raw text runs first (`scanSourceText`, on the untransformed source, which + * transpiling would normalise), then an AST walk (Bun.Transpiler + meriyah) + * blocks imports, globals, and escapes however they are spelled (ARCH-024). */ /** Shared transpiler — stateless, safe to reuse across calls. */ const tsTranspiler = new Bun.Transpiler({ loader: "ts" }); @@ -349,13 +275,11 @@ export function scanRuleSource( ) { return; } - // Advance the occurrence counter for EVERY code-position occurrence of the - // name — property-key slots included — so it stays aligned with the position - // remapper, which counts all code occurrences (it skips only strings and - // comments, not property keys). A property key (`{ Bun: 1 }`, `foo.Bun`) - // names a property, not the global, so it is counted but never emitted; - // skipping the count here would make a later *real* reference remap onto the - // earlier key. Bypasses `pushViolation` because it must count-without-emit. + // Advance the occurrence counter for EVERY code-position occurrence — + // property-key slots included — to stay aligned with the position remapper, + // which skips only strings and comments. A property key (`{ Bun: 1 }`, + // `foo.Bun`) names a property, not the global: counted, never emitted. + // Bypasses `pushViolation` because it must count-without-emit. const count = seenCounts.get(node.name) ?? 0; seenCounts.set(node.name, count + 1); if (!isPropertyKey) { @@ -400,14 +324,10 @@ export function scanRuleSource( } case "ObjectPattern": { // Destructuring `const { constructor: F } = obj` READS `obj.constructor` - // — the Function constructor (= eval), the same reach as `.constructor` - // member access, but through a binding pattern the MemberExpression case - // never sees. An object *literal* `{ constructor: 1 }` (ObjectExpression) - // only names a property and is fine; only the pattern form performs the - // read. `staticPropName` covers `{ constructor: F }`, `{ ["constructor"]: - // F }`, and the shorthand `{ constructor }`. A runtime-computed key - // (`{ [c]: F }`) is the same documented static-analysis residual as the - // computed-variable member case (see ARCH-024). + // — the Function constructor (= eval) — via a binding pattern the + // MemberExpression case never sees; an object LITERAL `{ constructor: 1 }` + // only names a property. Runtime-computed keys (`{ [c]: F }`) are the + // same static-analysis residual documented in ARCH-024. const props = Array.isArray(node.properties) ? node.properties : []; for (const raw of props) { const p = parseNode(raw); @@ -431,12 +351,11 @@ export function scanRuleSource( if (!obj || !prop) break; const computed = node.computed ?? false; - // Block `.constructor` (dotted or computed-literal), on ANY receiver. - // `(() => {}).constructor` is the `Function` constructor — i.e. eval — - // so `f = (() => {}).constructor; f("return import('node:fs')")()` - // would run arbitrary, unscanned code, bypassing every other check - // including the module allowlist. Naming `Function`/`eval` directly is - // already blocked as a global; this closes the property-chain route. + // Block `.constructor` (dotted or computed-literal) on ANY receiver: + // `(() => {}).constructor` is the Function constructor — i.e. eval — + // so it would run arbitrary unscanned code past the module allowlist. + // Naming `Function`/`eval` directly is already blocked as a global; + // this closes the property-chain route. if (staticPropName(prop, computed) === "constructor") { pushViolation( "Access to `.constructor` is blocked in rule files — it reaches the Function constructor, which is equivalent to eval.", @@ -474,10 +393,9 @@ export function scanRuleSource( ); break; } - // A *literal* specifier must still clear the allowlist. This case - // previously checked only for non-literal arguments, so the constant - // form — `await import("node:child_process")` — was a complete bypass - // of the module ban that ImportDeclaration enforces. + // A *literal* specifier must still clear the allowlist — without this + // check, the constant form `await import("node:child_process")` would + // bypass the module ban that ImportDeclaration enforces. const src = typeof node.source?.value === "string" ? node.source.value @@ -533,18 +451,11 @@ export function scanRuleSource( } /** - * Scan an imported (untrusted) `.rules.ts` source. - * - * Historically this added stricter checks than `scanRuleSource()` — imported - * rules were forbidden the environment reads (via `Bun` and `process`), - * `require()`, and `WebSocket` that first-party rules were allowed. Those are - * all now blocked for *every* rule file: naming `Bun`, `process`, `require`, or - * `WebSocket` - * (or any other runtime global) is refused by the banned-globals check in - * `scanRuleSource()`. First-party and imported scans have therefore converged, - * and this delegates. It remains a distinct export so the `adr import` call - * site reads intentionally, and so the two can diverge again if a future - * imported-only restriction is ever needed. + * Scan an imported (untrusted) `.rules.ts` source. The banned-globals check in + * `scanRuleSource()` applies to ALL rule files, so first-party and imported + * scans share one implementation and this delegates. It stays a distinct + * export so the `adr import` call site reads intentionally and the two can + * diverge if an imported-only restriction is ever needed. */ export function scanImportedRuleSource(source: string): ScanViolation[] { return scanRuleSource(source); diff --git a/src/engine/runner.ts b/src/engine/runner.ts index 939899b5..cf262572 100644 --- a/src/engine/runner.ts +++ b/src/engine/runner.ts @@ -52,16 +52,11 @@ import { applySuppressions, type SuppressionWarning } from "./suppressions"; const RULE_TIMEOUT_MS = 30_000; /** - * Per-invocation caches shared across every rule context in a check run. - * Rules overwhelmingly glob the same patterns and read the same files — - * without these caches, 40+ rules each repeat identical filesystem work. - * - * Values are promises so concurrent rules share in-flight work instead of - * racing to duplicate it. Glob results are copied on return, file contents - * are immutable strings. AST results are cached as shared trees — rules must - * treat them as read-only. `readJSON` is deliberately NOT cached — rules - * receive a mutable object, and sharing one instance would leak mutations - * between rules. + * Per-invocation caches shared across every rule context in a check run — + * without them, 40+ rules repeat identical glob/read/parse work. Values are + * promises so concurrent rules share in-flight work. Glob results are copied + * on return; file text is immutable; AST trees are shared and read-only. + * `readJSON` is NOT cached — sharing its mutable object would leak mutations. */ interface RunCaches { /** Glob results keyed by `tracked:`/`all:` + pattern. */ @@ -88,9 +83,6 @@ export interface CheckResult { suppressionWarnings?: SuppressionWarning[]; } -/** - * Create a RuleContext for a specific rule execution. - */ function createRuleContext( projectRoot: string, scopedFiles: string[], @@ -145,9 +137,8 @@ function createRuleContext( // ARCH-022: ctx.ast() implementation. Overload declarations match // RuleContext["ast"] so each language narrows to the correct return type. // The four guardrails below MUST run in this order before any subprocess. - // `{ rev: "base" }` parses the file's content at the comparison base commit - // instead of the working tree; the guardrails are identical, only the source - // acquisition differs. + // `{ rev: "base" }` swaps only the source acquisition (content at the + // comparison base commit); the guardrails are identical. async function astImpl( path: string, language: "typescript" | "javascript", @@ -194,10 +185,9 @@ function createRuleContext( /** * The uncached parse: TS/JS in-process, Python/Ruby via guardrails 3–4. * Errors below are CACHED (see the astResults lookup), so they - * interpolate the normalized `relPath` — never the raw `path` — - * otherwise a cached rejection would carry the first caller's path - * spelling (e.g. "src/./a.py") into every later caller's error, since - * aliased spellings resolve to the same cache entry. + * interpolate the normalized `relPath` — never the raw `path` — else a + * cached rejection would carry the first caller's path spelling into + * every later caller's error (aliased spellings share a cache entry). */ async function parseUncached(): Promise { // In-process branch: TypeScript/JavaScript via the shared meriyah @@ -246,11 +236,10 @@ function createRuleContext( try { // Guardrail 4: guarded invocation — array args only, path via argv. - // Python runs in isolated mode (-I): without it, `python -c` puts the - // cwd (the target project root) on sys.path, so a hostile project - // could shadow stdlib modules (ast.py, json.py) and execute arbitrary - // code when the serializer imports them. Ruby is safe as-is — its - // load path has not included the cwd since 1.9.2. + // Python runs isolated (-I): without it `python -c` puts the target + // project root on sys.path, letting a hostile project shadow stdlib + // modules (ast.py, json.py) for arbitrary code execution. Ruby's load + // path excludes the cwd (1.9.2+), so it is safe as-is. const pyProgram = wantComments ? PYTHON_AST_WITH_COMMENTS_PROGRAM : PYTHON_AST_PROGRAM; @@ -285,16 +274,11 @@ function createRuleContext( } } - // Per-run parse cache, mirroring cachedGlob/cachedFileText: keyed on the - // full tuple that determines the output, NUL-joined (NUL cannot appear in - // a path, so distinct tuples never collide). The PROMISE is cached, so - // concurrent identical calls share one in-flight parse/subprocess spawn. - // Rejected promises stay cached — a deliberate decision: ctx.ast() is - // fail-closed (ARCH-022), so every rule touching the same input fails - // fast with the identical error instead of re-paying the spawn. The - // cheap argument-validation guardrails above (path safety, language - // plausibility) still run per call, before this lookup, preserving - // ARCH-022's guardrail ordering on cache hits too. + // Per-run parse cache mirroring cachedGlob/cachedFileText, keyed on the + // full output-determining tuple, NUL-joined (NUL cannot appear in a path). + // The PROMISE is cached so concurrent identical calls share one in-flight + // parse; rejections stay cached — ctx.ast() is fail-closed (ARCH-022) — + // and the cheap guardrails above still run per call before this lookup. const cacheKey = astCacheKey(absPath, language, useBase, wantComments); let hit = caches.astResults.get(cacheKey); if (!hit) { @@ -360,11 +344,10 @@ function createRuleContext( /** * Read a file's source at the comparison base revision. Returns null when - * no base is resolved (no `--base`, or unrelated histories) or the path did - * not exist at the base (an added file) — the two "nothing to compare - * against" cases a caller checks with a single null test. Unlike - * `ctx.ast({ rev: "base" })`, this primitive reports absence as null rather - * than throwing. + * no base is resolved (no `--base`, or unrelated histories) or the path + * did not exist at the base — the two "nothing to compare against" cases, + * checkable with one null test. Unlike `ctx.ast({ rev: "base" })`, this + * primitive reports absence as null rather than throwing. */ fileAtBase(path: string): Promise { const absPath = safePath(resolvedRoot, path); @@ -399,16 +382,11 @@ export async function runChecks( // concurrently with the merge-base resolution below. const allTrackedFilesPromise = getGitTrackedFiles(projectRoot); - // Resolve the base commit ONCE per run — the merge base of `--base` and HEAD — - // and reuse that single SHA for BOTH `changedFiles` and base-revision reads - // (`ctx.fileAtBase()` / `ctx.ast({ rev: "base" })`). Resolving it separately - // for each (as `getFilesChangedSinceRef(options.base)` + `getMergeBase`) would - // let a branch that moves between the two git calls hand a rule a change set - // and a base AST computed against different commits (ARCH-022). Null for - // staged/default runs, so base-revision access reports "no base". Awaited here - // (concurrent with the tracked-file listing) so `changedFiles` can diff the - // resolved SHA: diffing the merge-base SHA three-dot against HEAD is identical - // to `options.base...HEAD`, since the merge base is an ancestor of HEAD. + // Resolve the base commit ONCE per run — the merge base of `--base` and HEAD + // — and reuse the single SHA for BOTH `changedFiles` and base-revision reads + // (`ctx.fileAtBase()` / `ctx.ast({ rev: "base" })`); separate resolution + // could hand a rule a change set and a base AST from different commits if + // the branch moves mid-run (ARCH-022). Null for staged/default runs. const baseRev: string | null = !options.staged && options.base ? await getMergeBase(projectRoot, options.base) @@ -469,7 +447,6 @@ export async function runChecks( astResults: new Map(), }; - // Run ADRs in parallel const adrResults = await Promise.allSettled( loadedAdrs.map(async ({ adr, ruleSet }) => { const respectGitignore = adr.frontmatter.respectGitignore !== false; @@ -481,19 +458,16 @@ export async function runChecks( { respectGitignore, adrId: adr.frontmatter.id } ); - // When files are specified, narrow scopedFiles to the intersection if (filterFiles) { scopedFiles = scopedFiles.filter((f) => filterFiles.has(f)); } - // Skip this ADR entirely if no specified files are in scope if (filterFiles && scopedFiles.length === 0) { return []; } const adrRuleResults: RuleResult[] = []; - // Run rules within an ADR sequentially for (const [ruleId, ruleConfig] of Object.entries(ruleSet.rules)) { const violations: ViolationDetail[] = []; const ruleStart = performance.now(); @@ -560,7 +534,6 @@ export async function runChecks( }) ); - // Collect results for (const result of adrResults) { if (result.status === "fulfilled") { for (const r of result.value) results.push(r); @@ -570,7 +543,6 @@ export async function runChecks( // Apply inline suppressions (archgate-ignore / archgate-ignore-file comments) const suppression = await applySuppressions(projectRoot, results); - // Filter suppressed violations from each rule result if (suppression.suppressedCount > 0) { for (const r of results) { r.violations = r.violations.filter((v) => diff --git a/src/engine/safe-path.ts b/src/engine/safe-path.ts index c481b768..33814aea 100644 --- a/src/engine/safe-path.ts +++ b/src/engine/safe-path.ts @@ -39,17 +39,13 @@ const verifiedRealDirs = new Set(); /** * Reject a symlink anywhere in the path below the project root — the leaf OR - * any ancestor. A leaf-only check is insufficient: with `/docs` linked - * outside the project, `/docs/secret.txt` is an ordinary file, so the - * lexical `isWithinRoot` and an lstat of the leaf both pass while the OS - * resolves through the link and reads outside. + * any ancestor, since a linked ancestor makes the leaf look like an ordinary + * file to both `isWithinRoot` and a leaf `lstat`. Components at or above the + * root are deliberately not inspected, and each test is a boolean `lstat` + * rather than a `realpath` comparison. * - * Components at or above the root are deliberately NOT inspected: the root's - * own location is the user's business, and macOS's temp prefix is itself a - * symlink (`/var` -> `/private/var`), which would reject every temp-dir root. - * Each component is a boolean lstat rather than a string comparison against - * `realpath`, which case-canonicalizes on Windows/macOS and would reject - * case-mismatched-but-legitimate paths. + * @throws {UserError} When any component below the root is a symbolic link. + * @see ARCH-022 — why the walk stops at the root and avoids `realpath` */ function assertNoSymlinkInPath( resolvedRoot: string, diff --git a/src/engine/source-positions.ts b/src/engine/source-positions.ts index 42dc31a8..c80ea90d 100644 --- a/src/engine/source-positions.ts +++ b/src/engine/source-positions.ts @@ -23,7 +23,7 @@ export interface RawViolation { /** * Build a set of character ranges that are inside comments or string literals. - * Used to filter out false matches when remapping violation positions. + * Filters out false matches when remapping violation positions. */ function buildNonCodeRanges(source: string): Array<[number, number]> { const ranges: Array<[number, number]> = []; @@ -93,9 +93,6 @@ function buildNonCodeRanges(source: string): Array<[number, number]> { return ranges; } -/** - * Check if a character offset falls inside any non-code range. - */ function isInNonCode(offset: number, ranges: Array<[number, number]>): boolean { for (const [start, end] of ranges) { if (offset >= start && offset < end) return true; diff --git a/src/engine/suppressions.ts b/src/engine/suppressions.ts index 6f98997c..3797fb43 100644 --- a/src/engine/suppressions.ts +++ b/src/engine/suppressions.ts @@ -43,16 +43,10 @@ export interface SuppressionResult { // --- Parsing --- /** - * Matches both `//` and `#` style comments: - * // archgate-ignore ARCH-006/no-unapproved-deps legacy dep, migration planned - * // archgate-ignore-file ARCH-005/test-mirrors-src generated file - * # archgate-ignore GEN-003/scripts-only Makefile target - * - * Capture groups: - * 1: "-file" or undefined (scope) - * 2: ADR ID (e.g. "ARCH-006") - * 3: rule ID (e.g. "no-unapproved-deps") - * 4: reason text or undefined + * Matches `//` and `#` style suppression comments, e.g. + * `# archgate-ignore ARCH-006/no-unapproved-deps legacy dep` or the + * `archgate-ignore-file` variant. Capture groups: 1 = "-file" scope or + * undefined, 2 = ADR ID, 3 = rule ID, 4 = reason text or undefined. */ const SUPPRESSION_RE = /^[ \t]*(?:\/\/|#)\s*archgate-ignore(-file)?\s+([\w-]+)\/([\w-]+)(?:\s+(.+))?$/u; @@ -61,12 +55,11 @@ const SUPPRESSION_RE = const FENCE_RE = /^[ \t]*(`{3,}|~{3,})/u; /** - * Parse suppression comments from file content. - * Returns one entry per matching comment line. + * Parse suppression comments from file content. In markdown files (.md, .mdx), + * lines inside fenced code blocks are skipped so that documented examples of + * `archgate-ignore` are not treated as real suppression directives. * - * In markdown files (.md, .mdx), lines inside fenced code blocks are skipped - * so that documented examples of `archgate-ignore` are not treated as real - * suppression directives. + * @returns One entry per matching comment line. */ export function parseSuppressions( content: string, @@ -78,7 +71,6 @@ export function parseSuppressions( let insideCodeBlock = false; for (let i = 0; i < lines.length; i++) { - // Track fenced code blocks in markdown so examples are not parsed if (isMarkdown && FENCE_RE.test(lines[i])) { insideCodeBlock = !insideCodeBlock; continue; @@ -117,20 +109,16 @@ export function parseSuppressions( // --- Filtering --- /** - * Apply inline suppressions to rule results. - * - * For each violation with a `file` and `line`, checks whether the source file - * contains an `archgate-ignore` comment on the preceding line (next-line scope) - * or an `archgate-ignore-file` comment anywhere in the file (file scope). - * - * Suppressions without a reason are ignored — a warning is emitted instead. - * Unused suppressions also produce warnings. + * Apply inline suppressions to rule results: a violation with `file`/`line` + * is dropped when an `archgate-ignore` comment precedes that line or an + * `archgate-ignore-file` comment appears anywhere in the file. A suppression + * missing its reason suppresses nothing and warns once it is scope-matched; + * an unused suppression that has a reason warns too. */ export async function applySuppressions( projectRoot: string, results: RuleResult[] ): Promise { - // Collect unique file paths referenced by violations const filePathsNeeded = new Set(); for (const r of results) { for (const v of r.violations) { @@ -150,7 +138,6 @@ export async function applySuppressions( }; } - // Read files in parallel and parse suppressions const fileSuppressions = new Map(); const readPromises = Array.from(filePathsNeeded, async (relPath) => { try { @@ -167,7 +154,6 @@ export async function applySuppressions( }); await Promise.all(readPromises); - // Filter violations const activeViolations = new Set(); const warnings: SuppressionWarning[] = []; let suppressedCount = 0; @@ -183,7 +169,6 @@ export async function applySuppressions( } } - // Detect unused suppressions for (const [, suppressions] of fileSuppressions) { for (const s of suppressions) { if (s.reason === null) continue; // already warned about missing reason @@ -206,7 +191,10 @@ export async function applySuppressions( /** * Check whether a single violation is suppressed by any comment in its file. - * Returns true if suppressed. + * + * @returns True when a scope-matching suppression carries a reason. A + * scope-matching suppression missing its reason pushes a warning and returns + * false, so the violation still reports. */ function checkSuppression( violation: ViolationDetail, diff --git a/src/formats/rules.ts b/src/formats/rules.ts index 1d7ca4e2..688802c0 100644 --- a/src/formats/rules.ts +++ b/src/formats/rules.ts @@ -1,11 +1,8 @@ // SPDX-License-Identifier: Apache-2.0 // Copyright 2026 Archgate -// --- Severity --- export type Severity = "error" | "warning" | "info"; -// --- Grep Match --- - export interface GrepMatch { file: string; line: number; @@ -13,8 +10,6 @@ export interface GrepMatch { content: string; } -// --- Violation Detail --- - export interface ViolationDetail { ruleId: string; adrId: string; @@ -37,8 +32,6 @@ export interface RuleReport { info(detail: Omit): void; } -// --- Package JSON --- - export interface PackageJson { name?: string; version?: string; @@ -68,17 +61,20 @@ export interface PackageJson { export type AstLanguage = "typescript" | "javascript" | "python" | "ruby"; /** - * A source comment, attached to the parsed tree's `comments` array when - * `ast()` is called with `{ comments: true }`. `value` is the comment text - * with its delimiters removed (`//`, `/* … *​/`, `#`). `loc` is a position in - * the ORIGINAL source — accurate even for `"typescript"`, where the tree's own - * `loc` is transpiled-relative (comments are extracted from the pre-transpile - * source). Python comments are always `"line"` (`#`); it has no block - * comments. Ruby `#` comments are `"line"`; each `=begin`/`=end` region is ONE - * `"block"` token whose `value` is the inner content (marker lines stripped, - * line endings normalized to LF) and whose `loc` spans the `=begin` line - * through the `=end` line. Ruby comment columns are character offsets like the - * other languages — the sexp tree's own node positions are byte offsets. + * A source comment, present on the tree's `comments` array when `ast()` is + * called with `{ comments: true }`. + * + * @property type - `"line"` for `//` and `#` comments; `"block"` for a + * C-style delimited comment and for a whole Ruby `=begin`/`=end` region. + * Python has no block comments, so its tokens are always `"line"`. + * @property value - Comment text with its delimiters removed. A Ruby block + * token carries the inner content only: marker lines stripped, line endings + * normalized to LF. + * @property loc - Position in the ORIGINAL source, accurate even for + * `"typescript"`, whose tree `loc` is transpiled-relative. Columns are + * character offsets in every language, including Ruby, whose sexp node + * positions are byte offsets. + * @see AstOptions */ export interface CommentToken { type: "line" | "block"; @@ -92,19 +88,27 @@ export interface CommentToken { /** * Options for `RuleContext.ast()`. * - * - `rev: "base"` parses the file's content at the comparison base commit (the - * merge base of `--base` and HEAD) instead of the working tree. Use it to ask - * "did the executable structure change?" — comments drop out of both the - * ESTree and Python `ast` shapes, but node positions do not, so compare a - * location-free projection (strip `loc`/`range`, `lineno`/`col_offset`): a - * comment-only edit is then equal, while a value change is not. Throws if no - * base is resolved or the file did not exist at the base; pair with - * `fileAtBase()` when you need to detect that first. - * - `comments: true` attaches a `comments` array of {@link CommentToken}s to - * the returned tree — the structured basis for comment-governance rules, in - * place of line-by-line regex. Supported for every {@link AstLanguage}; for - * `"ruby"` the array rides on the returned sexp array as a non-index - * property (see {@link RubyAstProgram}). + * @property rev - `"base"` parses the file as of the comparison base commit + * (merge base of `--base` and HEAD) rather than the working tree, so a rule + * can ask whether executable structure changed. Throws when no base resolves + * or the file is absent there; pair with `fileAtBase()` to detect that first. + * @property comments - Attaches a `comments` array of {@link CommentToken}s + * to the returned tree, the structured basis for comment-governance rules. + * Supported for every {@link AstLanguage}; for `"ruby"` the array rides on + * the returned sexp array as a non-index property. + * @example + * Detect a change in executable structure while ignoring comment-only edits. + * Comments drop out of both tree shapes but node positions do not, so compare + * a location-free projection: + * ```ts + * const strip = (tree: unknown) => + * JSON.stringify(tree, (key, value) => + * ["loc", "range", "lineno", "col_offset"].includes(key) ? undefined : value + * ); + * const changed = + * strip(await ctx.ast(file, "typescript")) !== + * strip(await ctx.ast(file, "typescript", { rev: "base" })); + * ``` */ export interface AstOptions { rev?: "base"; @@ -113,11 +117,10 @@ export interface AstOptions { /** * A node in the ESTree tree returned for `"typescript"`/`"javascript"`. - * `type` is the ESTree node discriminant (e.g. `"ImportDeclaration"`, - * `"CallExpression"`). Only the fields common to every node are typed; the - * rest of each node's grammar is reachable through the index signature — walk - * it against the ESTree spec. Note: for `"typescript"`, `loc` refers to the - * transpiled output (see `ast()`), not the original `.ts` source. + * Only the fields common to every node are typed; the rest of each node's + * grammar is reachable through the index signature — walk it against the + * ESTree spec. For `"typescript"`, `loc` refers to the transpiled output + * (see `ast()`), not the original `.ts` source. */ export interface EsTreeNode { type: string; @@ -202,10 +205,14 @@ export interface RuleContext { readFile(path: string): Promise; /** * Read a file's source at the comparison base revision (the merge base of - * `--base` and HEAD). Returns null when no base is resolved (no `--base`, or - * unrelated histories) or the path did not exist at the base — the two - * "nothing to compare against" cases, checkable with one null test. For a - * structural comparison prefer `ast(path, language, { rev: "base" })`. + * `--base` and HEAD). + * + * @param path - Project-relative path to read. + * @returns The file's content at the base, or null for both + * "nothing to compare against" cases — no base resolved (no `--base`, or + * unrelated histories) and path absent at the base — so one null test + * covers each. For a structural comparison prefer + * `ast(path, language, { rev: "base" })`. */ fileAtBase(path: string): Promise; readJSON(path: "package.json"): Promise; @@ -213,21 +220,19 @@ export interface RuleContext { /** * Parse a source file into its language-native AST. * - * The return type is selected by the `language` literal: an - * {@link EsTreeProgram} for `"typescript"`/`"javascript"`, a - * {@link PythonAstModule} for `"python"`, and a {@link RubyAstProgram} for - * `"ruby"`. The shapes are language-native and are NOT unified (see - * ARCH-022) — walk each against its own grammar. - * - * TypeScript/JavaScript parse in-process. Python and Ruby require the - * corresponding interpreter (`python3`/`python`, `ruby`) on PATH wherever - * `archgate check` runs — locally and in CI. - * - * Pass `{ rev: "base" }` (see {@link AstOptions}) to parse the file at the - * comparison base commit instead of the working tree. - * - * Throws (never returns null) when the file fails to parse or the required - * interpreter is missing; the error message distinguishes the two cases. + * @param path - Project-relative path to parse. + * @param language - Selects both the parser and the return shape. Shapes are + * language-native and NOT unified (ARCH-022) — walk each against its own + * grammar. + * @param opts - See {@link AstOptions} for `rev` and `comments`. + * @returns An {@link EsTreeProgram} for `"typescript"`/`"javascript"`, a + * {@link PythonAstModule} for `"python"`, or a {@link RubyAstProgram} for + * `"ruby"`. TypeScript and JavaScript parse in-process; Python and Ruby + * require their interpreter (`python3`/`python`, `ruby`) on PATH wherever + * `archgate check` runs, locally and in CI. + * @throws When the file fails to parse or the required interpreter is + * missing — never returns null. The message distinguishes the two cases. + * @see ARCH-022 */ ast( path: string, @@ -246,18 +251,16 @@ export interface RuleContext { ): Promise; ast(path: string, language: AstLanguage, opts?: AstOptions): Promise; /** - * Recursively collect every node in a parsed AST whose type-discriminant - * field matches one of `types`. Language-agnostic: each object node is - * checked against whichever discriminant field it carries — `_type` - * (Python) or `type` (ESTree TypeScript/JavaScript). Own-enumerable object - * values and arrays are recursed, and `tree` itself is a match candidate. - * - * Ruby: `Ripper.sexp` nodes are plain arrays with no object discriminant - * field, so a Ruby tree is recursed but its array-shaped nodes never - * match — only object nodes carrying a matching `_type`/`type` are - * collected. + * Collect every node in a parsed AST whose type-discriminant field matches + * one of `types`. Pure synchronous traversal — no I/O. * - * Pure tree traversal — synchronous, no I/O. + * @param tree - Any parsed tree or subtree; `tree` itself is a match + * candidate. Own-enumerable object values and arrays are traversed. + * @param types - Discriminant values to match against `_type` (Python) or + * `type` (ESTree TypeScript/JavaScript). + * @returns Matching nodes in preorder, empty when nothing matches. Ruby + * sexp nodes are plain arrays carrying no discriminant field, so only + * embedded object nodes can match. */ findAstNodes(tree: EsTreeNode, ...types: string[]): EsTreeNode[]; findAstNodes(tree: PythonAstNode, ...types: string[]): PythonAstNode[]; @@ -268,14 +271,10 @@ export interface RuleContext { report: RuleReport; } -// --- Rule Config --- - export interface RuleConfig { description: string; severity?: Severity; check: (ctx: RuleContext) => Promise; } -// --- Rule Set --- - export type RuleSet = { rules: Record }; diff --git a/src/helpers/adr-import.ts b/src/helpers/adr-import.ts index 36cc6ce6..db59166a 100644 --- a/src/helpers/adr-import.ts +++ b/src/helpers/adr-import.ts @@ -73,7 +73,6 @@ export function rewriteAdrId( newId: string ): string { // Replace id in frontmatter YAML only (between --- delimiters). - // We extract the frontmatter block, replace the id line, and reconstruct. const fmRegex = /^(---\r?\n)([\s\S]*?\r?\n)(---)/mu; const match = content.match(fmRegex); if (!match) return content; @@ -86,8 +85,8 @@ export function rewriteAdrId( // ---------- Source resolution & cloning ---------- /** - * Resolve and clone all sources. Uses a cache to avoid re-cloning the same repo. - * Sequential because clone N may share a repo with clone N+1 (dedup). + * Caches clones so a repo shared by several sources is fetched once, and runs + * sequentially because clone N may share a repo with clone N+1 (dedup). */ export async function resolveAndCloneSources( sources: string[] @@ -123,9 +122,6 @@ export async function resolveAndCloneSources( // ---------- ADR collection ---------- -/** - * Read all ADR files from resolved targets and build the import list. - */ export async function collectAdrsToImport( resolved: ResolvedImport[] ): Promise { @@ -176,9 +172,6 @@ export async function collectAdrsToImport( // ---------- ID remapping ---------- -/** - * Build an ID mapping for imported ADRs, assigning new IDs based on domain prefixes. - */ export function buildIdMap( adrsToImport: AdrToImport[], adrsDir: string, @@ -208,7 +201,13 @@ export function buildIdMap( /** * Write imported ADR files to disk with remapped IDs. - * Returns list of written file paths for rollback on failure. + * + * @param adrsToImport - The ADRs to write, each with its source path and + * optional companion rules path. + * @param idMap - Old-to-new ID mapping applied to filenames and content. + * @param adrsDir - Destination ADR directory. + * @returns Every written file path, in write order, so a caller can roll + * back the partial set when a later step fails. */ export async function writeImportedAdrs( adrsToImport: AdrToImport[], @@ -217,7 +216,6 @@ export async function writeImportedAdrs( ): Promise { const writtenFiles: string[] = []; - // Read all source files in parallel first const readTasks = adrsToImport.map((adr) => Bun.file(adr.sourcePath).text()); const ruleTasks = adrsToImport.map((adr) => adr.rulesPath ? Bun.file(adr.rulesPath).text() : Promise.resolve(null) @@ -227,16 +225,11 @@ export async function writeImportedAdrs( Promise.all(ruleTasks), ]); - // Security gate for third-party rule code. An imported `.rules.ts` is - // arbitrary code that `archgate check` will later import and execute - // in-process, so it is scanned with the stricter imported-rule ruleset - // before it is allowed onto disk. - // - // This has to happen here, at import time, rather than in the engine: - // once the file lands in `.archgate/adrs/` it is indistinguishable from a - // rule the project wrote itself, and the engine has no provenance to key - // the stricter checks off. Scanning before the first write also means a - // rejected pack needs no rollback — nothing has been written yet. + // Security gate for third-party rule code: an imported `.rules.ts` is + // arbitrary code `archgate check` will later execute in-process, so it is + // scanned with the stricter imported-rule ruleset BEFORE it lands on disk — + // once in `.archgate/adrs/` it is indistinguishable from a project-authored + // rule (no provenance), and scanning pre-write leaves nothing to roll back. const scanFailures = adrsToImport.flatMap((adr, i) => { const ruleSource = rulesContents[i]; if (ruleSource === null) return []; @@ -282,7 +275,6 @@ export async function writeImportedAdrs( } } } catch (err) { - // Rollback: delete all written files for (const file of writtenFiles) { try { unlinkSync(file); @@ -298,9 +290,6 @@ export async function writeImportedAdrs( // ---------- Manifest update ---------- -/** - * Update the imports manifest with newly imported ADRs. - */ export function updateImportsManifest( manifest: ImportsManifest, adrsToImport: AdrToImport[], diff --git a/src/helpers/adr-writer.ts b/src/helpers/adr-writer.ts index f77abf2e..b50f5535 100644 --- a/src/helpers/adr-writer.ts +++ b/src/helpers/adr-writer.ts @@ -107,7 +107,6 @@ export async function createAdrFile( const filePath = join(adrsDir, fileName); await Bun.write(filePath, content); - // Generate companion .rules.ts when rules are enabled if (opts.rules) { const rulesFileName = `${id}-${slug}.rules.ts`; const rulesFilePath = join(adrsDir, rulesFileName); diff --git a/src/helpers/auth.ts b/src/helpers/auth.ts index 9460d002..33a00aa2 100644 --- a/src/helpers/auth.ts +++ b/src/helpers/auth.ts @@ -89,8 +89,16 @@ export async function requestDeviceCode(): Promise { } /** - * Step 2: Poll GitHub until the user authorizes (or the code expires). - * Returns the GitHub access token on success. + * Step 2: Poll GitHub until the user authorizes, or the code expires. + * + * @param deviceCode - Device code from the step-1 authorization request. + * @param interval - Seconds to wait between polls, per RFC 8628. GitHub can + * widen this via a `slow_down` response. + * @param expiresIn - Lifetime of the device code in seconds, forming the + * polling deadline. + * @returns The GitHub access token. + * @throws {UserError} When GitHub rejects a poll, or the code expires or is + * denied before authorization completes. */ export async function pollForAccessToken( deviceCode: string, diff --git a/src/helpers/binary-upgrade.ts b/src/helpers/binary-upgrade.ts index 63e17440..49c4945d 100644 --- a/src/helpers/binary-upgrade.ts +++ b/src/helpers/binary-upgrade.ts @@ -13,10 +13,6 @@ import { internalPath } from "./paths"; import { isWindows } from "./platform"; import { UserError } from "./user-error"; -// --------------------------------------------------------------------------- -// Constants -// --------------------------------------------------------------------------- - const GITHUB_REPO = "archgate/cli"; // --------------------------------------------------------------------------- @@ -70,13 +66,13 @@ const GITHUB_RELEASES_API = `https://api.github.com/repos/${GITHUB_REPO}/release /** * Fetch the latest version tag from GitHub Releases. - * Returns the tag (e.g. "v0.13.1") or null on failure. * * @param timeoutMs Request timeout. Use a short value (e.g. 5s) for the * opportunistic background update check at CLI startup so * a slow network never delays the user's command. The * longer default (15s) is reserved for the explicit * `archgate upgrade` path where the user is waiting for it. + * @returns The tag (e.g. "v0.13.1"), or null on failure. */ export async function fetchLatestGitHubVersion( timeoutMs = 15_000 @@ -106,7 +102,6 @@ export async function fetchLatestGitHubVersion( // --------------------------------------------------------------------------- export interface DownloadProgress { - /** Bytes received so far. */ downloadedBytes: number; /** Total expected bytes (`null` when Content-Length is absent). */ totalBytes: number | null; @@ -124,7 +119,7 @@ export type DownloadProgressCallback = (progress: DownloadProgress) => void; * * When an `onProgress` callback is provided the response body is streamed * so the caller can display incremental progress. Without the callback the - * response is buffered in one shot (legacy behaviour). + * response is buffered in one shot. */ export async function downloadReleaseBinary( tag: string, @@ -138,9 +133,8 @@ export async function downloadReleaseBinary( logDebug("Downloading binary from:", archiveUrl); const response = await fetch(archiveUrl, { headers: { "User-Agent": "archgate-cli" }, - // 5 minutes — release binaries can exceed 100 MB which may take a - // while on slower connections. The previous 60 s limit caused - // timeouts for many users. + // 5 minutes — release binaries can exceed 100 MB, which takes a while + // on slower connections. signal: AbortSignal.timeout(300_000), }); @@ -186,7 +180,7 @@ export async function downloadReleaseBinary( logDebug("Downloaded", Math.round(buffer.byteLength / 1024), "KB"); - // Verify SHA256 checksum when available (releases after this change) + // Verify the SHA256 checksum when the release publishes one try { const checksumResponse = await fetch(checksumUrl, { headers: { "User-Agent": "archgate-cli" }, @@ -316,21 +310,11 @@ export function replaceBinary( // --------------------------------------------------------------------------- /** - * Attempt to delete the leftover `.old` binary from a previous upgrade. - * - * On Windows, `replaceBinary()` renames the running exe to `.old` because the - * OS file-locks the running binary. The `.old` file cannot be deleted during - * that same process — but it is guaranteed to be unlocked by the time the - * *next* CLI invocation starts. - * - * The cleanup is platform-agnostic: it resolves the correct binary name for - * the current platform and attempts to remove `.old` from the install - * directory. On Unix the `.old` file is unlikely to exist (rename is atomic), - * but running the check everywhere keeps the logic unified. - * - * Call this once at CLI startup (fire-and-forget, no `await`). Errors are - * silently swallowed — cleanup is best-effort and must never affect the - * user's command. + * Attempt to delete the leftover `.old` binary from a prior upgrade. On + * Windows the running exe is file-locked, so `replaceBinary()` renames it to + * `.old`; it is unlocked by the next CLI invocation, which is when this + * runs. Call once at CLI startup, fire-and-forget — errors are swallowed + * because cleanup is best-effort and must never affect the user's command. */ export function cleanupStaleBinary(): Promise { const artifact = getArtifactInfo(); @@ -342,9 +326,6 @@ export function cleanupStaleBinary(): Promise { }); } -/** - * Returns the manual install hint for the current platform. - */ export function getManualInstallHint(): string { return isWindows() ? "irm https://raw.githubusercontent.com/archgate/cli/main/install.ps1 | iex" diff --git a/src/helpers/claude-settings.ts b/src/helpers/claude-settings.ts index dbf5fc6d..f5dfa2dd 100644 --- a/src/helpers/claude-settings.ts +++ b/src/helpers/claude-settings.ts @@ -88,7 +88,6 @@ export async function configureClaudeSettings( const claudeDir = join(projectRoot, ".claude"); const settingsPath = join(claudeDir, "settings.local.json"); - // Read existing settings or start with empty object let existing: ClaudeSettings = {}; if (existsSync(settingsPath)) { try { @@ -103,7 +102,6 @@ export async function configureClaudeSettings( const merged = mergeClaudeSettings(existing, ARCHGATE_CLAUDE_SETTINGS); - // Ensure .claude/ directory exists if (!existsSync(claudeDir)) { mkdirSync(claudeDir, { recursive: true }); } diff --git a/src/helpers/copilot-settings.ts b/src/helpers/copilot-settings.ts index 7e3aafda..2c9370b2 100644 --- a/src/helpers/copilot-settings.ts +++ b/src/helpers/copilot-settings.ts @@ -4,8 +4,6 @@ import { existsSync, mkdirSync } from "node:fs"; import { join } from "node:path"; /** - * Configure Copilot CLI settings for archgate integration. - * * Creates the `.github/copilot/` directory if it does not exist. * Plugin installation is handled separately via `archgate init --install-plugin`. * diff --git a/src/helpers/credential-store.ts b/src/helpers/credential-store.ts index 849b5dc1..66facf1f 100644 --- a/src/helpers/credential-store.ts +++ b/src/helpers/credential-store.ts @@ -1,14 +1,10 @@ // SPDX-License-Identifier: Apache-2.0 // Copyright 2026 Archgate /** - * credential-store.ts — Secure credential storage using git's native credential helpers. - * - * Tokens are stored exclusively in the OS credential manager (macOS Keychain, - * Windows Credential Manager, libsecret) via `git credential approve/fill/reject`. - * Nothing is written to disk — no metadata files, no plaintext tokens. - * - * The `username` field in the git credential protocol carries the GitHub username, - * and the `password` field carries the archgate plugin token. + * Secure credential storage in the OS credential manager (macOS Keychain, + * Windows Credential Manager, libsecret) via `git credential + * approve/fill/reject` — nothing is written to disk. In the protocol, + * `username` carries the GitHub username and `password` the plugin token. * * @see https://git-scm.com/docs/git-credential */ @@ -22,16 +18,10 @@ const CREDENTIAL_HOST = "plugins.archgate.dev"; const CREDENTIAL_TIMEOUT_MS = 3_000; /** - * Build env for git credential commands at call time (not import time). - * - * Suppresses ALL interactive prompts — terminal, GUI, and askpass — across - * platforms and Git Credential Manager (GCM) versions: - * - * - GIT_TERMINAL_PROMPT=0 — git's own terminal prompt - * - GCM_INTERACTIVE=never — GCM interactive mode (terminal + GUI) - * - GCM_GUI_PROMPT=false — GCM GUI-only prompt (Windows toast/dialog) - * - GIT_ASKPASS="" — external askpass program - * - SSH_ASKPASS="" — SSH askpass fallback (some helpers reuse it) + * Build env for git credential commands at call time (not import time), + * suppressing every interactive prompt across platforms and Git Credential + * Manager versions: git's terminal prompt, GCM terminal/GUI modes, and the + * external/SSH askpass programs. */ function gitCredentialEnv(): Record { return { @@ -87,9 +77,9 @@ async function gitCredentialFill(): Promise<{ }); // The timeout MUST be cancelled when the spawn wins the race — - // `Bun.sleep` / `setTimeout` both keep the event loop alive for - // their full duration, which used to add 3s of latency to - // commands that call `loadCredentials()` (e.g. `archgate doctor`). + // `Bun.sleep` / `setTimeout` both keep the event loop alive for their + // full duration, adding 3s of latency to every `loadCredentials()` + // caller (e.g. `archgate doctor`) if left running. let timer: ReturnType | undefined; const result = await Promise.race([ (async () => { @@ -144,8 +134,9 @@ function legacyMetadataPath(): string { } /** - * Delete the legacy ~/.archgate/credentials file if it exists. - * Returns true if a file was found and deleted. + * Delete the legacy `~/.archgate/credentials` file if it exists. + * + * @returns `true` when a file was found and deleted, `false` when none existed. */ async function cleanupLegacyMetadata(): Promise { const file = Bun.file(legacyMetadataPath()); @@ -174,7 +165,6 @@ const CREDENTIAL_HELPER_HINT = export async function saveCredentials( credentials: StoredCredentials ): Promise { - // Clean up any legacy metadata file from previous versions. await cleanupLegacyMetadata(); const stored = await gitCredentialApprove( @@ -204,11 +194,11 @@ export async function saveCredentials( } /** - * Load stored archgate credentials from the OS credential manager. - * Returns null if no credentials are stored. + * Load stored archgate credentials from the OS credential manager. A legacy + * `~/.archgate/credentials` file is deleted on sight and the user is asked + * to re-login. * - * If a legacy ~/.archgate/credentials file exists, it is deleted and - * the user is asked to re-login. + * @returns The stored credentials, or `null` when none are stored. */ export async function loadCredentials(): Promise { // Delete legacy metadata file — force re-login for a clean slate. diff --git a/src/helpers/cursor-settings.ts b/src/helpers/cursor-settings.ts index c0034ef8..d7a5cdce 100644 --- a/src/helpers/cursor-settings.ts +++ b/src/helpers/cursor-settings.ts @@ -1,18 +1,11 @@ // SPDX-License-Identifier: Apache-2.0 // Copyright 2026 Archgate /** - * Cursor editor integration. - * - * Cursor has evolved from an IDE to an agent platform — users primarily - * use `cursor agent` (CLI) and cloud agents. Archgate components (skills, - * agents, hooks) are installed directly into Cursor's discovery - * directories (`~/.cursor/{skills,agents}/`) via an authenticated - * tarball download, bypassing Cursor's plugin subsystem which is - * unreliable in CLI mode and absent in cloud environments. - * - * `configureCursorSettings` writes a project-level hooks file - * (`.cursor/hooks.json`) for cloud agent compatibility — cloud VMs - * have no `~/.cursor/` user config. + * Cursor editor integration. Archgate components install directly into + * Cursor's discovery directories (`~/.cursor/{skills,agents}/`) via an + * authenticated tarball, bypassing the plugin subsystem (unreliable in CLI + * mode, absent in cloud environments). `configureCursorSettings` writes a + * project-level `.cursor/hooks.json` because cloud VMs have no `~/.cursor/`. */ import { existsSync, mkdirSync, writeFileSync } from "node:fs"; @@ -33,7 +26,6 @@ export function configureCursorSettings(projectRoot: string): string { const cursorDir = join(projectRoot, ".cursor"); mkdirSync(cursorDir, { recursive: true }); - // Write hooks.json const hooksPath = join(cursorDir, "hooks.json"); if (!existsSync(hooksPath)) { writeFileSync( diff --git a/src/helpers/doctor.ts b/src/helpers/doctor.ts index a85dc863..4ed9a8f8 100644 --- a/src/helpers/doctor.ts +++ b/src/helpers/doctor.ts @@ -98,7 +98,6 @@ export async function runDoctor(): Promise { const integrations = detectIntegrations(); const configDir = internalPath(); - // Run async checks in parallel const [editors, gitCmd, credentials] = await Promise.all([ detectEditors(), resolveCommand("git").then((r) => r !== null), diff --git a/src/helpers/editor-detect.ts b/src/helpers/editor-detect.ts index 6ea2bc03..18cc2593 100644 --- a/src/helpers/editor-detect.ts +++ b/src/helpers/editor-detect.ts @@ -19,17 +19,12 @@ import { } from "./plugin-install"; import { withPromptFix } from "./prompt"; -/** Result of editor availability detection. */ export interface DetectedEditor { id: EditorTarget; label: string; available: boolean; } -/** - * Detect which editor CLIs are available on PATH. - * Runs all checks in parallel for speed. - */ export async function detectEditors(): Promise { logDebug("Detecting available editor CLIs"); const [claude, cursor, vscode, copilot, opencode] = await Promise.all([ @@ -60,8 +55,11 @@ export async function detectEditors(): Promise { /** * Prompt the user to select one or more editors from the detected list. - * Detected editors are pre-checked; unavailable ones are shown but unchecked. - * Returns at least one selection (validation enforced). + * + * @param detected - Candidate editors; installed ones are pre-checked, the + * rest are listed unchecked. + * @returns At least one editor — the prompt's own validation rejects an + * empty selection. */ export async function promptEditorSelection( detected: DetectedEditor[] diff --git a/src/helpers/exit.ts b/src/helpers/exit.ts index 784d174f..bd375857 100644 --- a/src/helpers/exit.ts +++ b/src/helpers/exit.ts @@ -1,27 +1,11 @@ // SPDX-License-Identifier: Apache-2.0 // Copyright 2026 Archgate /** - * exit.ts — Centralized process-exit helper for the CLI. - * - * Every command that needs a non-zero exit (and some that need zero) must go - * through {@link exitWith} instead of calling `process.exit(code)` directly. - * The helper records a `command_completed` telemetry event with the real exit - * code + a high-level outcome tag, then flushes PostHog and Sentry before - * exiting. Calling `process.exit` directly skips the Commander `postAction` - * hook AND the `main()`-level flush, dropping the event on the floor — which - * is exactly why `exit_code` used to be stuck at 0 in the dashboard. - * - * Lifecycle: - * 1. Commander preAction hook calls {@link beginCommand} with the full - * command path so we know which command we're timing. - * 2. The action runs. On the happy path it returns and Commander's - * `postAction` hook calls {@link finalizeCommand}`(cmd, 0, "success")`. - * 3. On an expected failure, the action calls `await exitWith(1, ...)` which - * finalizes + flushes + exits. - * 4. On an unexpected crash, `main().catch()` calls `await exitWith(2, ...)`. - * - * The module-level guard prevents double-counting when both `exitWith` and the - * Commander `postAction` hook fire for the same invocation. + * Centralized process-exit helper. Exits go through {@link exitWith}, which + * records a `command_completed` telemetry event and flushes PostHog/Sentry + * before exiting; a direct `process.exit` skips both and drops the event. + * A module-level guard prevents double-counting when both `exitWith` and + * Commander's `postAction` hook fire for the same invocation. */ import { logError } from "./log"; @@ -83,18 +67,14 @@ export function finalizeCommand( } /** - * Terminate the process after recording + flushing telemetry. - * - * Use this instead of `process.exit(code)` anywhere inside a command action - * or the top-level error boundary. Safe to `await` — the returned promise is - * typed `Promise` because control never returns. + * Terminate the process after recording + flushing telemetry. Use instead of + * `process.exit(code)` in command actions and the top-level error boundary. * - * The outcome tag defaults to a sensible value derived from the exit code: - * - 0 → "success" - * - 1 → "user_error" - * - 2 → "internal_error" - * - 130 → "cancelled" - * Override via the `outcome` option when the default doesn't fit. + * @param code - Process exit code: 0 success, 1 user error, 2 internal + * error, 130 cancellation. + * @param opts - `outcome` overrides the telemetry outcome tag that otherwise + * derives from `code`; `errorKind` attaches a {@link classifyErrorKind} tag. + * @returns Typed `Promise` — control never returns to the caller. */ export async function exitWith( code: 0 | 1 | 2 | 130, @@ -119,18 +99,16 @@ export async function exitWith( } /** - * Centralized error handler for command catch blocks. + * Centralized error handler for command catch blocks (ARCH-012). Helpers + * throw {@link UserError} for expected failures: those are logged and never + * sent to Sentry. * - * Every async command action's catch block should delegate here instead of - * inlining `logError + exitWith`. The handler: - * - * 1. Re-throws `ExitPromptError` so `main().catch()` handles Ctrl+C (exit 130) - * 2. Captures **unexpected** errors (non-{@link UserError}) to Sentry - * 3. Logs the error message via `logError()` - * 4. Exits with code 1 (UserError) or code 2 (unexpected bug) - * - * Expected user-facing errors (validation, network, auth) should be thrown as - * {@link UserError} in helpers — these are logged but not sent to Sentry. + * @param err - The caught error, of any shape. + * @throws The original error when it is an `ExitPromptError`, so + * `main().catch()` handles Ctrl+C as exit 130. + * @returns Never returns: exits 1 for a {@link UserError}, or captures to + * Sentry and exits 2 for an unexpected bug. + * @see {@link exitWith} */ export function handleCommandError(err: unknown): Promise { if (err instanceof Error && err.name === "ExitPromptError") throw err; @@ -138,7 +116,6 @@ export function handleCommandError(err: unknown): Promise { const errorKind = classifyErrorKind(err); const isExpected = err instanceof UserError; - // Only capture unexpected errors to Sentry — UserError is expected if (!isExpected) { captureException(err, { command: currentCommand ?? "unknown", errorKind }); } @@ -154,7 +131,10 @@ export function handleCommandError(err: unknown): Promise { /** * Classify an error into a high-level bucket for telemetry. - * Returns a short tag — never the raw error message. + * + * @param err - The error to classify, of any shape. + * @returns A short tag such as `network` or `tls` — never the raw error + * message, which could carry user data. */ export function classifyErrorKind(err: unknown): string { if (!(err instanceof Error)) return "unknown"; diff --git a/src/helpers/git.ts b/src/helpers/git.ts index 9b6701ac..1e9c692d 100644 --- a/src/helpers/git.ts +++ b/src/helpers/git.ts @@ -13,7 +13,6 @@ import { UserError } from "./user-error"; * ceremony (and its WSL fallback subprocess on Windows) from the startup path. */ export async function installGit() { - // Fast path: git on PATH — no subprocess, no await, no WSL fallback. if (Bun.which("git")) { logDebug("Git is already installed"); return; diff --git a/src/helpers/init-project.ts b/src/helpers/init-project.ts index 1c18e1a9..2c13995f 100644 --- a/src/helpers/init-project.ts +++ b/src/helpers/init-project.ts @@ -44,7 +44,6 @@ export type SignupEditor = | "cursor" | "opencode"; -/** Map editor targets to signup API identifiers. */ export const SIGNUP_EDITORS: Record = { claude: "claude-code", cursor: "cursor", @@ -93,13 +92,11 @@ export async function initProject( // without requiring node_modules await writeRulesShim(projectRoot); - // Ensure generated shim files are gitignored await ensureGitignoreEntries(projectRoot); // Disable triple-slash-reference lint rule for .archgate/adrs/ if linter detected await ensureLinterOverrides(projectRoot); - // Only generate the example ADR when no ADRs exist yet const hasExistingAdrs = existsSync(paths.adrsDir) && readdirSync(paths.adrsDir).some((f) => f.endsWith(".md")); @@ -163,9 +160,6 @@ Archgate standardizes \`.archgate/lint/\` as the location for linter rules that }; } -/** - * Route editor settings configuration to the appropriate helper. - */ async function configureEditorSettings( projectRoot: string, editor: EditorTarget @@ -270,7 +264,10 @@ async function ensureEslintrcOverride(projectRoot: string): Promise { /** * Attempt to install the archgate plugin using stored credentials. - * Returns null-safe result — never throws. + * + * @param editor - The editor to install the plugin for. + * @returns A result describing success or the reason for skipping. Never + * throws, so a failed install cannot abort `archgate init`. */ async function tryInstallPlugin(editor: EditorTarget): Promise { const { loadCredentials } = await import("./credential-store"); @@ -313,12 +310,10 @@ async function tryInstallPlugin(editor: EditorTarget): Promise { const { isOpencodeAvailable, installOpencodePlugin } = await import("./plugin-install"); - // Writing agent markdown to `~/.config/opencode/agents/` is only useful - // if opencode is actually installed — otherwise we leave stale files in - // a directory nothing reads. `isOpencodeAvailable()` recognizes both the - // CLI (on PATH) and the Desktop app (no CLI, but shares the same - // user-scope config directory), mirroring the detect-before-install - // guard that every other editor's install path already uses. + // Install only when opencode exists — otherwise the agent markdown lands + // in a directory nothing reads. `isOpencodeAvailable()` recognizes both + // the CLI (on PATH) and the Desktop app (no CLI, same user-scope config + // dir), mirroring every other editor's detect-before-install guard. if (!(await isOpencodeAvailable())) { return { installed: true, diff --git a/src/helpers/install-info.ts b/src/helpers/install-info.ts index 409a2d3f..661b04cb 100644 --- a/src/helpers/install-info.ts +++ b/src/helpers/install-info.ts @@ -20,11 +20,11 @@ import { resolvedProjectPaths } from "./project-config"; let cachedInstallMethod: string | null = null; /** - * Detect how archgate was installed. - * Returns: "binary" | "proto" | "local" | "global-pm" + * Detect how archgate was installed, reading `process.execPath` for compiled + * binaries and `Bun.main` for `bun run` development mode (where + * `process.execPath` is the bun runtime rather than archgate). * - * Uses process.execPath for compiled binaries and Bun.main for `bun run` - * development mode (where process.execPath is the bun runtime, not archgate). + * @returns One of `"binary"`, `"proto"`, `"local"`, or `"global-pm"`. */ export function detectInstallMethod(): string { if (cachedInstallMethod) return cachedInstallMethod; @@ -69,14 +69,11 @@ export interface ProjectContext { } /** - * Scan the current working directory for an archgate project. - * - * This used to be cached per process, but the cache was a source of stale - * data: if the first call happened BEFORE `archgate init` created the project - * (during the Commander `preAction` hook), the post-init `init_completed` - * event reused the pre-init snapshot and incorrectly reported - * `has_project=false, adr_count=0`. The read is a single `readdirSync` — - * cheap enough to re-run on every event, and worth it for accuracy. + * Scan the current working directory for an archgate project. Deliberately + * uncached: a per-process cache goes stale when the first call precedes + * `archgate init` (the Commander `preAction` hook), making later events + * report `has_project=false`. The read is a single `readdirSync` — cheap + * enough to re-run on every event. */ export function getProjectContext(): ProjectContext { const cwd = process.cwd(); diff --git a/src/helpers/login-flow.ts b/src/helpers/login-flow.ts index e26aa1b2..b86c0619 100644 --- a/src/helpers/login-flow.ts +++ b/src/helpers/login-flow.ts @@ -45,8 +45,9 @@ export interface LoginFlowResult { * Run the full GitHub device flow: authenticate, claim token (or sign up * if the user is unregistered), and store credentials. * - * Returns `{ ok: true }` when credentials are stored, `{ ok: false }` on - * failure (error is already printed). + * @param options - Flow overrides, such as a pre-selected editor target. + * @returns `{ ok: true }` once credentials are stored, or `{ ok: false }` on + * failure — the error is printed before returning, so callers exit quietly. */ export async function runLoginFlow( options?: LoginFlowOptions @@ -113,8 +114,14 @@ export async function runLoginFlow( } /** - * Prompt for signup details, submit the request, and return the token. - * Returns null on failure (error is already printed). + * Prompt for signup details and submit the registration request. + * + * @param githubUser - GitHub login of the authenticated user. + * @param githubToken - GitHub access token proving that identity. + * @param githubEmail - Email from GitHub, or `null` to prompt for one. + * @param preselectedEditor - Skips the editor prompt when supplied. + * @returns The archgate token, or `null` on failure — the error is printed + * before returning. */ async function runSignupPrompt( githubUser: string, diff --git a/src/helpers/opencode-settings.ts b/src/helpers/opencode-settings.ts index c2898467..128ef3af 100644 --- a/src/helpers/opencode-settings.ts +++ b/src/helpers/opencode-settings.ts @@ -1,16 +1,10 @@ // SPDX-License-Identifier: Apache-2.0 // Copyright 2026 Archgate /** - * opencode-settings.ts — Configure opencode user-scope settings. - * - * Writes `opencode.json` to the XDG config directory - * (`~/.config/opencode/opencode.json`) with `default_agent` set to - * `archgate-developer`. Merges additively — existing user settings - * are preserved. - * - * opencode resolves its config via `xdg-basedir`, which falls back to - * `~/.config` on all platforms (including Windows). The path resolution - * uses `opencodeConfigDir()` from `paths.ts`. + * Configure opencode user-scope settings: writes `opencode.json` in the XDG + * config directory (resolved by `opencodeConfigDir()` in `paths.ts`) with + * `default_agent: archgate-developer`, merging additively so existing user + * settings are preserved. */ import { existsSync, mkdirSync } from "node:fs"; @@ -83,7 +77,6 @@ export async function configureOpencodeSettings(): Promise { const merged = mergeOpencodeSettings(existing); - // Ensure parent directory exists const dir = opencodeConfigDir(); if (!existsSync(dir)) { mkdirSync(dir, { recursive: true }); diff --git a/src/helpers/output.ts b/src/helpers/output.ts index c4e84cf7..601bc31c 100644 --- a/src/helpers/output.ts +++ b/src/helpers/output.ts @@ -10,17 +10,22 @@ */ /** - * Returns true when the CLI is likely being invoked by an AI agent: - * stdout is not a TTY AND not running in a CI environment. + * Detect whether the CLI is likely being driven by an AI agent. + * + * @returns `true` when stdout is not a TTY and no CI environment is detected. */ export function isAgentContext(): boolean { return !process.stdout.isTTY && !Bun.env.CI; } /** - * Serialize data to JSON with context-aware formatting: - * - Agent context (non-TTY, non-CI): compact (no whitespace) to minimize tokens - * - Human context (TTY or explicit --json): pretty-printed with 2-space indent + * Serialize data to JSON with context-aware formatting. + * + * @param data - The value to serialize. + * @param forcePretty - When true, always pretty-print (e.g. an explicit + * `--json` flag). Defaults to pretty-printing outside agent contexts. + * @returns Compact JSON in agent contexts (non-TTY, non-CI) to minimize + * tokens, otherwise JSON indented with 2 spaces. */ export function formatJSON(data: unknown, forcePretty?: boolean): string { const pretty = forcePretty ?? !isAgentContext(); diff --git a/src/helpers/pack-recommend.ts b/src/helpers/pack-recommend.ts index 8df9fbaf..b36a5893 100644 --- a/src/helpers/pack-recommend.ts +++ b/src/helpers/pack-recommend.ts @@ -18,7 +18,11 @@ export interface PackRecommendation { /** * Match a single pack tag against the detected stack. - * Returns the relevance level or null if no match. + * + * @param tag - A `namespace:value` pack tag. + * @param stack - The stack detected for the project. + * @returns The matched tag with its relevance level, or `null` when the tag + * is malformed or does not apply to this stack. */ function matchTag( tag: string, @@ -51,9 +55,6 @@ function matchTag( } } -/** - * Count ADR markdown files in a pack's adrs/ directory. - */ function countAdrs(packDir: string): number { const adrsDir = join(packDir, "adrs"); if (!existsSync(adrsDir)) return 0; @@ -108,7 +109,6 @@ export function recommendPacksFromDir( } } - // Only recommend packs that have at least one matching tag if (matchedTags.length === 0) continue; recommendations.push({ diff --git a/src/helpers/paths.ts b/src/helpers/paths.ts index d3430626..767936c4 100644 --- a/src/helpers/paths.ts +++ b/src/helpers/paths.ts @@ -43,18 +43,10 @@ function usableEnv(value: string | undefined): string | null { /** * Resolve the opencode user-scope config directory (`~/.config/opencode/`). - * - * Opencode uses the `xdg-basedir` package to locate its config root. That - * package reads `$XDG_CONFIG_HOME` when set and otherwise falls back to - * `~/.config` on **all platforms** — including Windows, where the resolved - * path is `C:\Users\\.config\opencode\` rather than anything under - * `%APPDATA%`. We mirror the same resolution here so the CLI writes to - * the exact directory opencode reads from. - * - * The path is resolved at call time, not cached — tests override `HOME` / - * `XDG_CONFIG_HOME` per-test and expect the helper to pick up the override. - * - * Used by `opencodeAgentsDir()` and `opencodeConfigPath()`. + * Mirrors opencode's own `xdg-basedir` resolution: `$XDG_CONFIG_HOME` when + * set, else `~/.config` on ALL platforms — including Windows (never + * `%APPDATA%`; see CLAUDE.md "Adding a New Editor Target"). Resolved at + * call time, not cached, so tests can override HOME / XDG_CONFIG_HOME. */ export function opencodeConfigDir(): string { const xdg = usableEnv(Bun.env.XDG_CONFIG_HOME); @@ -80,14 +72,10 @@ export function copilotSessionStateDir(): string { } /** - * Resolve the opencode SQLite database path. - * - * Opencode stores session/message/part data in a SQLite database at - * `$XDG_DATA_HOME/opencode/opencode.db` (defaulting to - * `~/.local/share/opencode/opencode.db`). - * - * Resolved at call time (not cached) so tests can override HOME / - * XDG_DATA_HOME. + * Resolve the opencode SQLite database path — session/message/part data at + * `$XDG_DATA_HOME/opencode/opencode.db` (default + * `~/.local/share/opencode/opencode.db`). Resolved at call time (not + * cached) so tests can override HOME / XDG_DATA_HOME. */ export function opencodeDbPath(): string { const xdg = usableEnv(Bun.env.XDG_DATA_HOME); @@ -132,22 +120,15 @@ export function createPathIfNotExists(path: string) { } /** - * Walk up from cwd to find the nearest directory containing an archgate - * project. A directory is a project root when it has either: - * - `.archgate/adrs/` — standard project layout - * - `.archgate/lint/` — also created by `archgate init` - * - * Both directories are created by `archgate init` and are project-specific. - * We cannot match on `.archgate/` alone because `~/.archgate/` is the - * CLI's user-level cache directory (binary installs, credentials, etc.) - * and would produce false positives. We also avoid matching on - * `.archgate/config.json` because `~/.archgate/config.json` stores - * telemetry settings. + * Walk up to the nearest directory containing `.archgate/adrs/` or + * `.archgate/lint/` (both created by `archgate init`). Matching `.archgate/` + * or `.archgate/config.json` alone would false-positive on the user-level + * `~/.archgate/` cache. * - * **Test isolation:** Set `ARCHGATE_PROJECT_CEILING` to a directory path - * to prevent the walk-up from escaping above it — analogous to git's - * `GIT_CEILING_DIRECTORIES`. The ceiling directory itself is still - * checked, but the walk stops there. + * @param startDir - Directory to start the walk from. Defaults to `cwd`. + * @returns The project root, or `null` when the walk reaches the filesystem + * root or the `ARCHGATE_PROJECT_CEILING` bound without a match. That ceiling + * isolates tests the way git's ceiling dirs do, and is itself still checked. */ export function findProjectRoot(startDir?: string): string | null { const ceilingEnv = Bun.env.ARCHGATE_PROJECT_CEILING; @@ -161,7 +142,6 @@ export function findProjectRoot(startDir?: string): string | null { return dir; } - // Don't walk above the ceiling directory if (ceiling && resolve(dir) === ceiling) { return null; } @@ -175,12 +155,14 @@ export function findProjectRoot(startDir?: string): string | null { } /** - * Like {@link findProjectRoot}, but throws a {@link UserError} when no - * project is found. For command actions whose body is wrapped in the - * ARCH-012 error boundary (handleCommandError): the boundary logs the - * message and exits 1 without Sentry. Commands that can operate without a - * project (e.g. `session-context` falling back to cwd) should keep using - * `findProjectRoot()` directly. + * Resolve the project root for commands that require one. + * + * @param startDir - Directory to start the walk from. Defaults to `cwd`. + * @returns The project root directory. + * @throws {UserError} When no project is found. The ARCH-012 boundary + * (`handleCommandError`) logs it and exits 1 without Sentry. + * @see {@link findProjectRoot} — used directly by commands that can operate + * without a project, such as `session-context` falling back to cwd. */ export function requireProjectRoot(startDir?: string): string { const projectRoot = findProjectRoot(startDir); diff --git a/src/helpers/platform.ts b/src/helpers/platform.ts index d1e656d4..438eb8f3 100644 --- a/src/helpers/platform.ts +++ b/src/helpers/platform.ts @@ -63,9 +63,6 @@ export function getPlatformInfo(): PlatformInfo { return cachedPlatformInfo; } -/** - * Shorthand: returns true if running inside WSL. - */ export function isWSL(): boolean { return getPlatformInfo().isWSL; } @@ -77,9 +74,6 @@ export function isWindows(): boolean { return getPlatformInfo().runtime === "win32"; } -/** - * Returns true if the process is running on macOS. - */ export function isMacOS(): boolean { return getPlatformInfo().runtime === "darwin"; } @@ -105,8 +99,15 @@ export function isSupportedPlatform(): boolean { // --------------------------------------------------------------------------- /** - * Convert a WSL path to a Windows path (e.g. /mnt/c/Users → C:\Users). - * Returns null if not in WSL or conversion fails. + * Convert a WSL path to a Windows path by delegating to `wslpath -w`. + * + * @param wslPath - A WSL-style absolute path. + * @returns The Windows equivalent, or `null` outside WSL and whenever the + * conversion fails. + * @example + * ```ts + * await toWindowsPath("/mnt/c/Users"); // "C:\\Users" + * ``` */ export async function toWindowsPath(wslPath: string): Promise { if (!isWSL()) return null; @@ -125,8 +126,15 @@ export async function toWindowsPath(wslPath: string): Promise { } /** - * Convert a Windows path to a WSL path (e.g. C:\Users → /mnt/c/Users). - * Returns null if not in WSL or conversion fails. + * Convert a Windows path to a WSL path by delegating to `wslpath -u`. + * + * @param windowsPath - A Windows-style absolute path. + * @returns The WSL equivalent, or `null` outside WSL and whenever the + * conversion fails. + * @example + * ```ts + * await toWslPath("C:\\Users"); // "/mnt/c/Users" + * ``` */ export async function toWslPath(windowsPath: string): Promise { if (!isWSL()) return null; diff --git a/src/helpers/plugin-install.ts b/src/helpers/plugin-install.ts index 22afe90f..6df5f83e 100644 --- a/src/helpers/plugin-install.ts +++ b/src/helpers/plugin-install.ts @@ -22,8 +22,11 @@ const CURSOR_MARKETPLACE_URL = "https://plugins.archgate.dev/archgate/cursor.git"; /** - * Run a command using Bun.spawn (cross-platform, no shell). - * Returns { exitCode, stdout, stderr }. + * Run a command using `Bun.spawn` (cross-platform, no shell — ARCH-007). + * + * @param cmd - Argv array, the executable first. + * @param opts - `cwd` sets the working directory for the child process. + * @returns The exit code plus captured `stdout` and `stderr`. */ async function run( cmd: string[], @@ -127,19 +130,11 @@ export async function installClaudePlugin(): Promise { // --------------------------------------------------------------------------- /** - * Install the archgate Cursor components into user-scope discovery dirs. - * - * Cursor discovers skills and agents from `~/.cursor/{skills,agents}/`. - * The tarball from /api/cursor contains these at its root: - * - skills/archgate-{name}/SKILL.md — skill definitions - * - agents/archgate-{name}.md — agent definitions - * - hooks.json — afterFileEdit hook for archgate check - * - * After extraction, `hooks.json` is merged into `~/.cursor/hooks.json` - * (rather than extracted as-is) to avoid overwriting existing user hooks. - * - * Throws on download or extraction failure so callers can surface a manual - * retry hint. + * Install the archgate Cursor components into user-scope discovery dirs + * (`~/.cursor/{skills,agents}/`). The /api/cursor tarball root holds + * per-skill SKILL.md directories, agent markdown files, and a `hooks.json` + * that is merged into `~/.cursor/hooks.json` to preserve existing user hooks. + * Throws on download or extraction failure so callers can surface a retry hint. */ export async function installCursorPlugin(token: string): Promise { const cursorDir = cursorUserDir(); @@ -170,12 +165,10 @@ async function mergeCursorHooks(cursorDir: string): Promise { const existing: { event: string; command?: string }[] = await Bun.file(hooksPath).json(); - // Remove any previous archgate hooks const filtered = existing.filter( (h) => !h.command?.includes("archgate check") ); - // Add our hooks const archgateHooks = [ { event: "afterFileEdit", @@ -227,22 +220,11 @@ async function downloadPluginAsset( // --------------------------------------------------------------------------- /** - * Install an archgate editor plugin bundle (agents + skills). - * - * Shared by Cursor and opencode — both follow the same pattern: - * 1. Ensure `agents/` and `skills/` subdirectories exist - * 2. Clean previous archgate files (avoids dangling/renamed artifacts) - * 3. Download and extract the authenticated tarball - * - * Old archgate files are removed via `Bun.Glob` before extraction so - * renamed or removed components don't linger. Only `archgate-*` entries - * are touched — other editors'/users' files are left untouched. - * - * Uses `tar` via `Bun.spawn` (ARCH-007) — `tar` is available on macOS, - * Linux, and modern Windows (bsdtar ships with Windows 10+). - * - * Editor-specific post-install steps (hooks merging, settings config) are - * handled by each editor's install function after this returns. + * Install an archgate editor plugin bundle (agents + skills), shared by + * Cursor and opencode: ensure `agents/`/`skills/` exist, delete stale + * `archgate-*` entries (only those — other files stay untouched), then + * download and extract the authenticated tarball with `tar` via ARCH-007's + * `run()`. Editor-specific post-install steps happen in each caller. */ async function installEditorPluginBundle(opts: { baseDir: string; @@ -275,7 +257,6 @@ async function installEditorPluginBundle(opts: { rmSync(join(skillsDir, dir), { recursive: true, force: true }); } - // Download and extract the tarball const tarballPath = internalPath(opts.tempFile); const buffer = await downloadPluginAsset(opts.apiPath, opts.token); logDebug( @@ -315,15 +296,10 @@ export async function isOpencodeCliAvailable(): Promise { /** * Check whether opencode is installed in any form — the CLI on PATH, or the - * opencode Desktop app (Electron-based GUI, ships no CLI binary at all). - * - * Both distributions read agents/skills from the same user-scope config - * directory (`opencodeConfigDir()` — see its doc comment for the exact - * resolution rules), so a directory that already exists there is reliable - * evidence opencode has been run and initialized on this machine, even when - * `isOpencodeCliAvailable()` finds nothing. This is what call sites should - * use to decide whether to attempt the plugin install, which itself never - * shells out to a CLI — it only writes files into that shared directory. + * Desktop app (Electron GUI, no CLI binary). Both distributions share + * `opencodeConfigDir()`, so that directory existing is reliable evidence of + * an opencode install even when the CLI probe finds nothing. Call sites use + * this to gate the plugin install, which only writes files into that dir. */ export async function isOpencodeAvailable(): Promise { if (await isOpencodeCliAvailable()) return true; @@ -332,14 +308,10 @@ export async function isOpencodeAvailable(): Promise { /** * Install archgate agents and skills into opencode's user-scope directories. - * - * Opencode has no plugin marketplace — agents and skills are plain markdown - * files. Archgate ships them as an authenticated tarball at `/api/opencode`. - * The tarball contains `agents/` and `skills/` directories which extract - * into the resolved `opencodeConfigDir()`. - * - * Throws on download or extraction failure so callers can surface a manual - * retry hint. + * Opencode has no plugin marketplace — the `/api/opencode` tarball ships + * plain-markdown `agents/` and `skills/` directories that extract into + * `opencodeConfigDir()`. Throws on download or extraction failure so + * callers can surface a manual retry hint. */ export async function installOpencodePlugin(token: string): Promise { const baseDir = opencodeConfigDir(); diff --git a/src/helpers/project-config.ts b/src/helpers/project-config.ts index 050e6e26..32e1ac4d 100644 --- a/src/helpers/project-config.ts +++ b/src/helpers/project-config.ts @@ -58,9 +58,6 @@ export function loadProjectConfig(projectRoot: string): ProjectConfig { } } -/** - * Write the project config to disk. - */ export async function saveProjectConfig( projectRoot: string, config: ProjectConfig @@ -109,7 +106,9 @@ export function resolveDomainPrefix( /** * Read the `baseBranch` value from `.archgate/config.json`. - * Returns `null` when unconfigured. + * + * @param projectRoot - Project root holding the `.archgate/` directory. + * @returns The configured base branch, or `null` when unconfigured. */ export function getConfiguredBaseBranch(projectRoot: string): string | null { const config = loadProjectConfig(projectRoot); @@ -241,13 +240,10 @@ export async function removeCustomDomain( } /** - * Resolve project paths with config-aware overrides. - * - * Reads `.archgate/config.json` and applies any custom `paths.adrs` or - * `paths.rules` overrides. When `paths.rules` is not set, rules are - * loaded from the same directory as ADRs (co-location convention). - * Falls back to the standard `.archgate/adrs/` and `.archgate/lint/` - * defaults when no `paths` config is present. + * Resolve project paths with config-aware overrides: reads + * `.archgate/config.json` and applies custom `paths.adrs` / `paths.rules`. + * Each key falls back independently to its standard default — `.archgate/adrs/` + * for ADRs and `.archgate/lint/` for rules — as does a missing `paths` block. */ export function resolvedProjectPaths(projectRoot: string): { root: string; diff --git a/src/helpers/prompt.ts b/src/helpers/prompt.ts index b3e97697..94e44b80 100644 --- a/src/helpers/prompt.ts +++ b/src/helpers/prompt.ts @@ -1,35 +1,11 @@ // SPDX-License-Identifier: Apache-2.0 // Copyright 2026 Archgate /** - * prompt.ts — Windows terminal fix for inquirer prompts. - * - * When inquirer creates a readline interface on Windows, the runtime enables - * Virtual Terminal Processing (VTP) on the console output handle. VTP mode - * sets the `DISABLE_NEWLINE_AUTO_RETURN` flag, which causes bare `\n` (LF) - * to move the cursor down WITHOUT returning to column 0. The runtime never - * restores the original console mode when the readline closes — so ALL - * subsequent output is affected, not just the prompt itself. - * - * This is a runtime bug (Node.js/Bun should restore the console mode in - * rl.close()), tracked upstream at: - * https://github.com/SBoudrias/Inquirer.js/issues/2123 - * - * Until the runtime fixes this, we work around it by: - * - * 1. Patching `process.stdout.write` / `process.stderr.write` to translate - * bare `\n` → `\r\n`. This fixes inquirer's own rendering (which writes - * through the JS stream API) and any code that uses the stream API. - * - * 2. Redirecting `console.log`, `.error`, `.warn`, `.info`, `.debug` through - * the patched stream writes. Bun's native console methods write directly - * to the file descriptor for performance, bypassing the JS stream API — - * so the stream-level patch alone cannot fix them. - * - * Both patches are applied once (idempotent) and persist for the lifetime - * of the process. - * - * `withPromptFix()` ensures the patches are active before running a prompt - * and resets the cursor to column 0 afterward. + * Windows terminal fix for inquirer prompts: patches stream writes (LF → CRLF) + * and redirects console methods through them, because a prompt permanently + * leaves the console in a mode where bare LF does not return the cursor to + * column 0. Full background and the wrapper contract live in ARCH-019 + * (.archgate/adrs/ARCH-019-inquirer-prompt-fix.md). */ import { cursorTo } from "node:readline"; @@ -41,10 +17,8 @@ import { isWindows } from "./platform"; // LF → CRLF translation // --------------------------------------------------------------------------- -/** Regex that matches bare LF (not preceded by CR). */ const BARE_LF = /(?(fn: () => Promise): Promise { if (!isWindows()) return fn(); diff --git a/src/helpers/registry.ts b/src/helpers/registry.ts index c604070f..c970d4ce 100644 --- a/src/helpers/registry.ts +++ b/src/helpers/registry.ts @@ -33,10 +33,8 @@ function stripRef(input: string): { base: string; ref?: string } { // git@ URLs use @ as part of the host syntax, not as a ref separator. // Only consider an @ that comes after the last `/` or `:` for git@ URLs. if (input.startsWith("git@")) { - // For git@ URLs, look for @ref only after the path portion const lastSlash = input.lastIndexOf("/"); const atIdx = input.lastIndexOf("@"); - // Only split on @ if it appears after the last path separator if (atIdx > lastSlash && lastSlash > 0) { return { base: input.slice(0, atIdx), ref: input.slice(atIdx + 1) }; } @@ -51,13 +49,19 @@ function stripRef(input: string): { base: string; ref?: string } { const OFFICIAL_REGISTRY_URL = "https://github.com/archgate/awesome-adrs.git"; /** - * Resolve a source string into a repo URL, optional ref, and subpath. + * Resolve a source string into a repo URL, optional ref, and subpath. An + * `@` suffix is stripped first, then the first matching form wins. * - * Resolution rules (first match wins): - * 1. Starts with "packs/" — official registry - * 2. Is a URL (https://, http://, git@) — parse GitHub /tree//, else pass-through - * 3. Has 3+ slash-separated segments — GitHub org/repo/path - * 4. None of the above — error + * @param input - Source string in one of the forms shown below. + * @returns The repo URL, ref, and subpath, tagged with the matched `kind`. + * @throws {UserError} When the input matches none of the supported forms. + * @example + * ```ts + * resolveSource("packs/security"); // kind: "official" + * resolveSource("acme/adrs/backend@v2"); // kind: "github-repo", ref "v2" + * resolveSource("https://github.com/acme/adrs/tree/main/backend"); + * resolveSource("git@github.com:acme/adrs.git"); // kind: "git-url", subpath "." + * ``` */ export function resolveSource(input: string): ResolvedSource { const { base, ref } = stripRef(input); @@ -207,8 +211,8 @@ function listAvailablePacks(cloneDir: string): string[] { * Detect whether the subpath within a cloned repo points to a full pack * (has archgate-pack.yaml) or a single ADR file (.md). * - * @param sourceKind - The kind of source (official, github-repo, git-url) - * used to tailor error messages. When "official", the error lists available + * @param sourceKind - The kind of source (official, github-repo, git-url); + * tailors error messages. When "official", the error lists available * packs from the registry. */ export async function detectTarget( @@ -222,7 +226,6 @@ export async function detectTarget( throw new UserError(`Path "${subpath}" escapes the repository root.`); } - // Check for a pack (directory with archgate-pack.yaml) const packYaml = join(fullPath, "archgate-pack.yaml"); if (existsSync(packYaml)) { const raw = await Bun.file(packYaml).text(); @@ -245,7 +248,6 @@ export async function detectTarget( return { kind: "pack", packMeta, adrFiles, rulesFiles, baseDir: adrsDir }; } - // Check for a single ADR file const mdPath = fullPath.endsWith(".md") ? fullPath : `${fullPath}.md`; if (existsSync(mdPath)) { const rulesPath = mdPath.replace(/\.md$/u, ".rules.ts"); diff --git a/src/helpers/repo-probe.ts b/src/helpers/repo-probe.ts index faa8941c..fccd2b5f 100644 --- a/src/helpers/repo-probe.ts +++ b/src/helpers/repo-probe.ts @@ -1,19 +1,11 @@ // SPDX-License-Identifier: Apache-2.0 // Copyright 2026 Archgate /** - * repo-probe.ts — Unauthenticated API probes to determine whether a Git - * repository is public on its host (GitHub, GitLab, Bitbucket, Azure DevOps). - * - * Why it's a separate module: the probe code is network-y and host-specific, - * while the rest of `repo.ts` is local-only git inspection. Keeping them apart - * keeps each file small and testable, and keeps the network surface out of - * the fast path for every command (the probe is only called from - * `archgate init`). - * - * Privacy rationale: the probe is the gate that decides whether a repo's - * owner / name / remote URL ship on the `project_initialized` event. Only - * repos that a random anonymous user of the host can already see get their - * identity shared. + * repo-probe.ts — Unauthenticated API probes deciding whether a repo is + * public on its host (GitHub, GitLab, Bitbucket, Azure DevOps). This is the + * privacy gate for sharing owner/name/URL on `project_initialized`: only + * repos an anonymous user can already see get their identity shared. Called + * only from `archgate init`; `repo.ts` stays local-only. */ import { z } from "zod"; @@ -41,16 +33,11 @@ let cachedPublicProbe: Promise | null = null; // --------------------------------------------------------------------------- /** - * Probe the host's unauthenticated API to determine whether the repo is - * public. Returns: - * - `true` — confirmed public on a recognised host - * - `false` — confirmed private / not visible to anonymous users - * - `null` — couldn't determine (self-hosted, network failure, timeout, - * rate-limited) - * - * Bounded by a 3s timeout — telemetry must not slow down the CLI when the - * network is misbehaving. Errors are swallowed; we never probe again after - * the first call in a given process. + * Probe the host's unauthenticated API for repo visibility: `true` = + * confirmed public, `false` = private/not anonymously visible, `null` = + * undetermined (self-hosted, network failure, timeout, rate-limited). + * Bounded by a 3s timeout with errors swallowed — telemetry must not slow + * the CLI down; probes once per process. */ export function isPublicRepo( repo: Pick @@ -179,13 +166,10 @@ async function probeBitbucket( } /** - * Azure DevOps owner is `{organization}/{project}`. We probe the project's - * visibility endpoint — a public Azure DevOps project returns the record - * unauthenticated, a private project responds with 401. - * - * Note: this doesn't try to prove the specific repository is public; Azure - * DevOps project visibility governs repo visibility, and individual repos - * aren't separately togglable to public within a private project. + * Azure DevOps owner is `{organization}/{project}`; the project visibility + * endpoint answers unauthenticated for public projects and 401 for private. + * Project visibility governs repo visibility, so the specific repository + * needs no separate check. */ async function probeAzureDevOps( owner: string, diff --git a/src/helpers/repo.ts b/src/helpers/repo.ts index 0cc44dab..e9bd390e 100644 --- a/src/helpers/repo.ts +++ b/src/helpers/repo.ts @@ -1,23 +1,11 @@ // SPDX-License-Identifier: Apache-2.0 // Copyright 2026 Archgate /** - * repo.ts — Detect the git repository context for telemetry enrichment. - * - * Every event carries: - * - `repo_host`: "github" | "gitlab" | "bitbucket" | "azure-devops" | "other" | null - * - `repo_id`: sha256 hash of the normalized remote URL, truncated to 16 - * hex chars. Stable per repo, but non-reversible — you can count distinct - * repos using the CLI without learning any identity. - * - `repo_is_git`: whether the CWD is a git working tree at all - * - `git_default_branch`: best-effort "main" / "master" / etc. - * - * The raw remote URL and parsed owner/name are *only* sent on the one-time - * `project_initialized` event, and *only* when the repository is confirmed - * public via the host's unauthenticated API. See `repo-probe.ts` for that - * logic; this module stays local-only (git + URL parsing). - * - * Cached per-process because the git remote and default branch are effectively - * immutable over the lifetime of a single CLI invocation. + * repo.ts — Local-only (git + URL parsing) repository context for telemetry: + * `repo_host`, `repo_id` (truncated sha256 of the normalized remote URL — + * non-reversible), `repo_is_git`, and `git_default_branch`, cached + * per-process. Raw URL/owner/name ship only on `project_initialized` for + * confirmed-public repos — that probe lives in `repo-probe.ts`. */ import { createHash } from "node:crypto"; @@ -160,17 +148,14 @@ function pickDefaultBranch( } /** - * Should the CLI include owner / name / full remote URL in the - * `project_initialized` event? - * - * Rule: share iff the repository is confirmed public on a recognised host. - * Private, unknown, and self-hosted repos always return false. + * Gate raw repo identity (owner / name / remote URL) in the + * `project_initialized` event. There is deliberately no identity-specific + * opt-out knob — disabling telemetry suppresses the whole event upstream. * - * There's no identity-specific opt-out knob — if a user doesn't want *any* - * telemetry, including the identity event, they disable telemetry itself - * (`ARCHGATE_TELEMETRY=0` or `archgate telemetry disable`). The whole event - * is then suppressed upstream. Adding a separate identity opt-out would be - * redundant and asymmetric with how every other field is gated. + * @param repoPublic - Whether the repo is confirmed public on a recognised + * host; `null` when that could not be determined. + * @returns `true` only for a confirmed-public repo, so private, unknown, and + * self-hosted repos all withhold identity. */ export function shouldShareRepoIdentity(repoPublic: boolean | null): boolean { return repoPublic === true; @@ -181,15 +166,15 @@ export function shouldShareRepoIdentity(repoPublic: boolean | null): boolean { // --------------------------------------------------------------------------- /** - * Parse a git remote URL into host + owner + name. + * Parse a git remote URL into host + owner + name. Handles GitHub / GitLab / + * Bitbucket HTTPS and SCP-style SSH URLs, GitLab subgroups, Azure DevOps + * HTTPS/SSH (including the `_git` infix and `v3` SSH prefix), and legacy + * `{org}.visualstudio.com` URLs with the org encoded in the subdomain. * - * Handles: - * - GitHub / GitLab / Bitbucket HTTPS and SCP-style SSH URLs - * - GitLab subgroups (`gitlab.com/foo/sub/bar` → owner=`foo/sub`, name=`bar`) - * - Azure DevOps (`dev.azure.com`) HTTPS and SSH URLs, including the - * `_git` path infix and the `v3` SSH prefix - * - Legacy Azure DevOps `{org}.visualstudio.com` URLs where the org is - * encoded in the subdomain rather than the path + * @param raw - A remote URL in any of the supported forms. + * @returns The parsed parts, each field `null` where it cannot be + * determined — an empty or unrecognised URL yields an all-`null` result + * rather than throwing. */ export function parseRemoteUrl(raw: string): ParsedRemote { const trimmed = raw.trim(); @@ -225,12 +210,10 @@ export function parseRemoteUrl(raw: string): ParsedRemote { path = path.replace(/\.git\/?$/u, "").replace(/\/$/u, ""); let segments = path.split("/").filter(Boolean); - // Azure DevOps URL quirks: - // - HTTPS (modern): /{org}/{project}/_git/{repo} - // - HTTPS (legacy): /{project}/_git/{repo} on {org}.visualstudio.com - // - SSH (v3 path): v3/{org}/{project}/{repo} - // Strip the structural markers (`_git`, `v3`) and, for legacy URLs, pull - // the org out of the subdomain. + // Azure DevOps quirks: /{org}/{project}/_git/{repo} (HTTPS), + // /{project}/_git/{repo} on {org}.visualstudio.com (legacy), and + // v3/{org}/{project}/{repo} (SSH). Strip the structural markers (`_git`, + // `v3`) and, for legacy URLs, pull the org out of the subdomain. if (classified === "azure-devops") { segments = segments.filter((s) => s !== "_git" && s !== "v3"); diff --git a/src/helpers/rules-shim.ts b/src/helpers/rules-shim.ts index 3c0c1ca7..02f35dd2 100644 --- a/src/helpers/rules-shim.ts +++ b/src/helpers/rules-shim.ts @@ -73,14 +73,20 @@ declare interface PackageJson { declare type AstLanguage = "typescript" | "javascript" | "python" | "ruby"; /** - * A source comment, attached to the parsed tree's \`comments\` array when - * \`ast()\` is called with \`{ comments: true }\`. \`value\` has delimiters - * removed; \`loc\` is a position in the ORIGINAL source (accurate even for - * "typescript"). Python comments are always "line". Ruby \`#\` comments are - * "line"; each \`=begin\`/\`=end\` region is ONE "block" token (marker lines - * stripped, line endings normalized to LF, loc spanning \`=begin\` through - * \`=end\`). Ruby comment columns are character offsets — the sexp tree's own - * node positions are byte offsets. + * A source comment, present on the tree's \`comments\` array when \`ast()\` is + * called with \`{ comments: true }\`. + * + * @property type - "line" for \`//\` and \`#\` comments; "block" for a C-style + * delimited comment and for a whole Ruby \`=begin\`/\`=end\` region. Python has + * no block comments, so its tokens are always "line". + * @property value - Comment text with its delimiters removed. A Ruby block + * token carries the inner content only: marker lines stripped, line endings + * normalized to LF. + * @property loc - Position in the ORIGINAL source, accurate even for + * "typescript", whose tree \`loc\` is transpiled-relative. Columns are + * character offsets in every language, including Ruby, whose sexp node + * positions are byte offsets. + * @see AstOptions */ declare interface CommentToken { type: "line" | "block"; @@ -92,13 +98,29 @@ declare interface CommentToken { } /** - * Options for \`RuleContext.ast()\`. \`rev: "base"\` parses the file at the - * comparison base commit (merge base of \`--base\` and HEAD) instead of the - * working tree — use it to detect whether the executable structure changed. - * Throws if no base is resolved or the file did not exist at the base. - * \`comments: true\` attaches a \`comments\` array of \`CommentToken\`s to the - * returned tree (all languages; for ruby it rides on the sexp array as a - * non-index property). + * Options for \`RuleContext.ast()\`. + * + * @property rev - "base" parses the file as of the comparison base commit + * (merge base of \`--base\` and HEAD) rather than the working tree, so a rule + * can ask whether executable structure changed. Throws when no base resolves + * or the file is absent there; pair with \`fileAtBase()\` to detect that first. + * @property comments - Attaches a \`comments\` array of \`CommentToken\`s to the + * returned tree, the structured basis for comment-governance rules. Supported + * for every \`AstLanguage\`; for "ruby" the array rides on the returned sexp + * array as a non-index property. + * @example + * Detect a change in executable structure while ignoring comment-only edits. + * Comments drop out of both tree shapes but node positions do not, so compare + * a location-free projection: + * \`\`\`ts + * const strip = (tree: unknown) => + * JSON.stringify(tree, (key, value) => + * ["loc", "range", "lineno", "col_offset"].includes(key) ? undefined : value + * ); + * const changed = + * strip(await ctx.ast(file, "typescript")) !== + * strip(await ctx.ast(file, "typescript", { rev: "base" })); + * \`\`\` */ declare interface AstOptions { rev?: "base"; @@ -106,12 +128,11 @@ declare interface AstOptions { } /** - * A node in the ESTree tree returned for "typescript"/"javascript". \`type\` - * is the ESTree node discriminant (e.g. "ImportDeclaration", - * "CallExpression"). Only the fields common to every node are typed; the rest - * of each node's grammar is reachable through the index signature. Note: for - * "typescript", \`loc\` refers to the transpiled output (see \`ast()\`), not - * the original .ts source. + * A node in the ESTree tree returned for "typescript"/"javascript". + * Only the fields common to every node are typed; the rest of each node's + * grammar is reachable through the index signature — walk it against the + * ESTree spec. For "typescript", \`loc\` refers to the transpiled output + * (see \`ast()\`), not the original .ts source. */ declare interface EsTreeNode { type: string; @@ -191,9 +212,14 @@ declare interface RuleContext { grepFiles(pattern: RegExp, fileGlob: string): Promise; readFile(path: string): Promise; /** - * Read a file's source at the comparison base revision (merge base of - * \`--base\` and HEAD). Returns null when no base is resolved or the path did - * not exist at the base. For a structural comparison prefer + * Read a file's source at the comparison base revision (the merge base of + * \`--base\` and HEAD). + * + * @param path - Project-relative path to read. + * @returns The file's content at the base, or null for both + * "nothing to compare against" cases — no base resolved (no \`--base\`, or + * unrelated histories) and path absent at the base — so one null test + * covers each. For a structural comparison prefer * \`ast(path, language, { rev: "base" })\`. */ fileAtBase(path: string): Promise; @@ -202,20 +228,19 @@ declare interface RuleContext { /** * Parse a source file into its language-native AST. * - * The return type is selected by the \`language\` literal: an - * \`EsTreeProgram\` for "typescript"/"javascript", a \`PythonAstModule\` for - * "python", and a \`RubyAstProgram\` for "ruby". The shapes are language-native - * and are NOT unified — walk each against its own grammar. - * - * TypeScript/JavaScript parse in-process. Python and Ruby require the - * corresponding interpreter (\`python3\`/\`python\`, \`ruby\`) on PATH - * wherever \`archgate check\` runs — locally and in CI. - * - * Pass \`{ rev: "base" }\` to parse the file at the comparison base commit - * instead of the working tree. - * - * Throws (never returns null) when the file fails to parse or the required - * interpreter is missing; the error message distinguishes the two cases. + * @param path - Project-relative path to parse. + * @param language - Selects both the parser and the return shape. Shapes are + * language-native and NOT unified (ARCH-022) — walk each against its own + * grammar. + * @param opts - See \`AstOptions\` for \`rev\` and \`comments\`. + * @returns An \`EsTreeProgram\` for "typescript"/"javascript", a + * \`PythonAstModule\` for "python", or a \`RubyAstProgram\` for "ruby". + * TypeScript and JavaScript parse in-process; Python and Ruby require their + * interpreter (\`python3\`/\`python\`, \`ruby\`) on PATH wherever + * \`archgate check\` runs, locally and in CI. + * @throws When the file fails to parse or the required interpreter is + * missing — never returns null. The message distinguishes the two cases. + * @see ARCH-022 */ ast( path: string, @@ -234,14 +259,16 @@ declare interface RuleContext { ): Promise; ast(path: string, language: AstLanguage, opts?: AstOptions): Promise; /** - * Recursively collect every node in a parsed AST whose type-discriminant - * field matches one of \`types\`. Language-agnostic: each object node is - * checked against whichever discriminant field it carries — \`_type\` - * (Python) or \`type\` (ESTree TypeScript/JavaScript). Own-enumerable - * object values and arrays are recursed, and \`tree\` itself is a match - * candidate. Ruby's \`Ripper.sexp\` nodes are plain arrays with no object - * discriminant field, so a Ruby tree is recursed but its array-shaped nodes - * never match. Pure tree traversal — synchronous, no I/O. + * Collect every node in a parsed AST whose type-discriminant field matches + * one of \`types\`. Pure synchronous traversal — no I/O. + * + * @param tree - Any parsed tree or subtree; \`tree\` itself is a match + * candidate. Own-enumerable object values and arrays are traversed. + * @param types - Discriminant values to match against \`_type\` (Python) or + * \`type\` (ESTree TypeScript/JavaScript). + * @returns Matching nodes in preorder, empty when nothing matches. Ruby + * sexp nodes are plain arrays carrying no discriminant field, so only + * embedded object nodes can match. */ findAstNodes(tree: EsTreeNode, ...types: string[]): EsTreeNode[]; findAstNodes(tree: PythonAstNode, ...types: string[]): PythonAstNode[]; @@ -272,10 +299,7 @@ export default { rules: { // "rule-name": { // description: "What this rule checks", - // async check(ctx) { - // // Use ctx.scopedFiles, ctx.readFile(), ctx.grep(), etc. - // // ctx.report.violation({ message: "..." }); - // }, + // async check(ctx) { ctx.report.violation({ message: "..." }); }, // }, }, } satisfies RuleSet; @@ -312,14 +336,10 @@ async function ensureShimAt(dtsPath: string, expected: string): Promise { } /** - * Ensure rules.d.ts exists and is up-to-date. Skips the disk write when the - * on-disk content already matches — `archgate check` calls this every run, - * and the content only changes when the CLI version changes. - * - * When `customAdrsDir` is provided and differs from the default - * `.archgate/adrs/`, a copy of `rules.d.ts` is also written to the parent - * of that directory so that companion `.rules.ts` files' triple-slash - * `/// ` resolves correctly. + * Ensure rules.d.ts exists and is up-to-date, skipping the disk write when + * the on-disk content already matches. When `customAdrsDir` differs from the + * default `.archgate/adrs/`, a copy also lands next to that directory so the + * companion files' triple-slash reference resolves. */ export async function ensureRulesShim( projectRoot: string, diff --git a/src/helpers/sentry.ts b/src/helpers/sentry.ts index 959efbb7..52a875c0 100644 --- a/src/helpers/sentry.ts +++ b/src/helpers/sentry.ts @@ -1,18 +1,11 @@ // SPDX-License-Identifier: Apache-2.0 // Copyright 2026 Archgate /** - * sentry.ts — Error tracking via @sentry/node-core light mode. - * - * Uses Sentry's lightweight "light" SDK variant which excludes all - * OpenTelemetry auto-instrumentation — ideal for a CLI that only needs - * error capture with breadcrumbs. This avoids pulling in 600+ modules - * of OTel instrumentation for MongoDB, Redis, Express, etc. - * - * IP anonymization: the Sentry project has "Prevent Storing of IP Addresses" - * enabled server-side. - * - * Sentry is only initialized when telemetry is enabled. All Sentry calls - * are wrapped to never affect CLI behavior or exit codes. + * sentry.ts — Error tracking via @sentry/node-core "light" mode, which + * excludes OTel auto-instrumentation (600+ modules the CLI never needs). + * The Sentry project stores no IP addresses (server-side setting). Sentry + * initializes only when telemetry is enabled, and every call is wrapped so + * it can never affect CLI behavior or exit codes. */ import type * as SentryNs from "@sentry/node-core/light"; @@ -91,7 +84,6 @@ export async function initSentry(): Promise { tunnel: SENTRY_TUNNEL, release: cliVersion, environment: Bun.env.NODE_ENV ?? "production", - // Disable sending events in test environments enabled: Bun.env.NODE_ENV !== "test", // Do not send default PII (hostnames, IPs, etc.) sendDefaultPii: false, @@ -148,9 +140,9 @@ export async function initSentry(): Promise { } /** - * Add a breadcrumb to the current Sentry scope. - * Breadcrumbs are attached to the next error event, providing context - * about the sequence of operations leading to a crash. + * Add a breadcrumb to the current Sentry scope. Breadcrumbs attach to the + * next error event, providing context about the sequence of operations + * leading to a crash. * * @param category Short category name (e.g., "command", "config", "check") * @param message Human-readable description diff --git a/src/helpers/session-context-copilot.ts b/src/helpers/session-context-copilot.ts index e039f469..2e2c78e1 100644 --- a/src/helpers/session-context-copilot.ts +++ b/src/helpers/session-context-copilot.ts @@ -65,15 +65,10 @@ type CopilotMatchResult = | { ok: false; error: string; path?: string; available?: string[] }; /** - * Find Copilot CLI sessions matching a project, most recent first. - * - * Copilot CLI stores sessions under `~/.copilot/session-state//`. - * Each session directory contains: - * - `workspace.yaml` — metadata with a `cwd` field for project matching - * - `events.jsonl` — JSONL event log with conversation events - * - * Sessions are matched by comparing the `cwd` field in workspace.yaml - * to the provided project root. + * Find Copilot CLI sessions matching a project, most recent first. Sessions + * live under `~/.copilot/session-state//`, each holding + * `workspace.yaml` (metadata) and `events.jsonl` (conversation events); a + * session matches when workspace.yaml's `cwd` equals the project root. */ async function findMatchingCopilotSessions( projectRoot: string | null @@ -162,8 +157,13 @@ export async function listCopilotSessions( /** * Read the most recent Copilot CLI session transcript for a project — - * normally the conversation that is running right now. Pass `sessionId` - * (from `listCopilotSessions`) to read an earlier session instead. + * normally the conversation that is running right now. + * + * @param projectRoot - Project to read sessions for; `null` falls back to cwd. + * @param options - `sessionId` (from {@link listCopilotSessions}) selects an + * earlier session; `maxEntries` caps returned transcript entries. + * @returns The transcript on success, or a result carrying the reason no + * session could be read. */ export async function readCopilotSession( projectRoot: string | null, diff --git a/src/helpers/session-context-opencode.ts b/src/helpers/session-context-opencode.ts index 16ef12b5..413e7d85 100644 --- a/src/helpers/session-context-opencode.ts +++ b/src/helpers/session-context-opencode.ts @@ -25,19 +25,11 @@ interface OpencodeSessionSummary { interface ReadOpencodeSessionOptions extends ReadSessionOptions { sessionId?: string; /** - * Resolve to the top-level (root) session. Without `sessionId` this is - * an explicit alias for the default behavior (recency selection already - * only considers top-level sessions); combined with a `sessionId` that - * names a sub-agent child session, walks the `parent_id` chain up to the - * top-level ancestor. - * - * Opencode is the only session-context backend with a real parent/child - * session graph, so this option lives here rather than in the shared - * `ReadSessionOptions`. A recency-based guess cannot distinguish the - * true parent from a sibling sub-agent session once more than one - * sibling exists — and an inline Skill invocation creates no session - * row at all. Ancestry via `parent_id` is correct regardless of nesting - * depth or sibling fan-out. + * Resolve to the top-level (root) session: without `sessionId`, an alias + * for the default recency selection (top-level sessions only); with a + * sub-agent child `sessionId`, walks the `parent_id` chain to the + * top-level ancestor. Opencode-specific — only opencode has a + * parent/child session graph. */ root?: boolean; } @@ -129,25 +121,11 @@ export function listOpencodeSessions( } /** - * Read an opencode session transcript for a project. - * - * Opencode stores data in a SQLite database at - * `$XDG_DATA_HOME/opencode/opencode.db` (default `~/.local/share/opencode/opencode.db`): - * - `session` table — session metadata with `directory` for project matching - * and `parent_id` linking sub-agent sessions to their parent - * - `message` table — messages with `role` in the `data` JSON column - * - `part` table — content parts with `type` and `text` in the `data` JSON column - * - * Sessions are matched by comparing the `directory` field in session rows - * to the provided project root. - * - * Sub-agent runs are stored as child sessions (`parent_id` set) that share - * the parent's `directory`, so recency selection only considers top-level - * sessions — otherwise sub-agent transcripts shadow the main session. Note - * that opencode skills run inline in the calling session (they do NOT - * create their own session), so the most recent top-level session is the - * current development session. An explicit `sessionId` can still read any - * session, including sub-agent children. + * Read an opencode session transcript for a project from the SQLite database + * at `$XDG_DATA_HOME/opencode/opencode.db` (`session`/`message`/`part` + * tables; sessions match on `directory` = project root). Recency selection + * considers only top-level sessions so sub-agent children don't shadow the + * main session; an explicit `sessionId` can read any session. */ export function readOpencodeSession( projectRoot: string | null, @@ -173,14 +151,12 @@ export function readOpencodeSession( } try { - // 1. Find all sessions, sorted by most recently updated first const allSessions = queryAllSessions(db); if (allSessions.length === 0) { return { ok: false, error: "No opencode sessions found", path: dbPath }; } - // 2. Filter sessions by project path const matching = allSessions.filter( (s) => s.directory && normalizePath(s.directory) === normalizedProjectRoot ); @@ -194,7 +170,6 @@ export function readOpencodeSession( }; } - // 3. Select session by ID or most recent. // Recency selection only considers top-level sessions: sub-agent runs // are child sessions (parent_id set) sharing the parent's directory, // and would otherwise shadow the main session. @@ -218,7 +193,7 @@ export function readOpencodeSession( }; } - // 3b. With --root, walk the parent_id chain up to the top-level + // With --root, walk the parent_id chain up to the top-level // ancestor (relevant when --session-id names a sub-agent child). let selected = target; if (options?.root === true) { @@ -232,7 +207,6 @@ export function readOpencodeSession( } } - // 4. Read messages for the session interface MessageRow { id: string; role: string; @@ -251,7 +225,6 @@ export function readOpencodeSession( }; } - // 5. Build transcript from text parts, skipping synthetic entries interface PartRow { type: string; text: string | null; diff --git a/src/helpers/session-context.ts b/src/helpers/session-context.ts index cf27c404..8c530ddd 100644 --- a/src/helpers/session-context.ts +++ b/src/helpers/session-context.ts @@ -10,25 +10,11 @@ import type { EditorTarget } from "./init-project"; import { isWSL, toWindowsPath } from "./platform"; /** - * Encode a project root path into the directory name used by Claude/Cursor - * for storing session files under `~/.claude/projects/` or `~/.cursor/projects/`. - * - * Replaces path separators (`\`, `/`) and dots (`.`) with dashes (`-`). - * Drive-letter colons (`:`) are handled per-tool: Claude Code replaces them - * with dashes while Cursor strips them entirely. - * - * Examples (target = "claude", the default): - * - `/home/user/project` → `-home-user-project` - * - `C:\Users\user\project` → `C--Users-user-project` - * - `E:\foo\.claude\worktrees\x` → `E--foo--claude-worktrees-x` - * - * Examples (target = "cursor"): - * - `/home/user/project` → `-home-user-project` - * - `C:\Users\user\project` → `C-Users-user-project` - * - `E:\foo\.claude\worktrees\x` → `E-foo--claude-worktrees-x` - * - * In WSL, converts to the Windows path first so the encoded name matches - * what the Windows-side editor uses. + * Encode a project root into the session-directory name under + * `~/.claude/projects/` or `~/.cursor/projects/`: separators (`\`, `/`) and + * dots become dashes; drive-letter colons become dashes for Claude Code + * (`C:\Users\x` → `C--Users-x`) but are stripped by Cursor (`C-Users-x`). + * In WSL, converts to the Windows path first to match the Windows-side editor. */ export async function encodeProjectPath( projectRoot: string, @@ -98,7 +84,6 @@ interface ClaudeSessionSummary { transcript: Array<{ type: string; role?: string; contentPreview: string }>; } -/** One entry in a list result: session id + last-update timestamp. */ export interface SessionListEntry { id: string; /** Session title — only populated by editors that store one (opencode). */ @@ -141,7 +126,6 @@ function parseContentBlock(block: ContentBlock): string | null { return null; } -/** Extract a concise content preview from a transcript entry. */ export function getContentPreview(entry: TranscriptEntry): string { const content = entry.message?.content; if (typeof content === "string") { @@ -170,7 +154,6 @@ interface ReadCursorSessionOptions extends ReadSessionOptions { sessionId?: string; } -/** Resolve the Claude Code projects dir for a project root. */ async function claudeProjectsDir(projectRoot: string | null): Promise { const encodedPath = await encodeProjectPath(projectRoot ?? process.cwd()); return join(homedir(), ".claude", "projects", encodedPath); @@ -218,9 +201,13 @@ export async function listClaudeCodeSessions( /** * Read the most recent Claude Code session transcript for a project — - * normally the conversation that is running right now. Pass `sessionId` - * (from `listClaudeCodeSessions`) to read an earlier session instead. - * Falls back to cwd when no project root is found. + * normally the conversation that is running right now. + * + * @param projectRoot - Project to read sessions for; `null` falls back to cwd. + * @param options - `sessionId` (from {@link listClaudeCodeSessions}) selects + * an earlier session; `maxEntries` caps returned transcript entries. + * @returns The transcript on success, or a result carrying the reason no + * session could be read. */ export async function readClaudeCodeSession( projectRoot: string | null, @@ -291,7 +278,6 @@ export async function readClaudeCodeSession( }; } -/** Resolve the Cursor agent-transcripts dir for a project root. */ async function cursorTranscriptsDir( projectRoot: string | null ): Promise { @@ -359,9 +345,13 @@ export async function listCursorSessions( /** * Read the most recent Cursor agent session transcript for a project — - * normally the conversation that is running right now. Pass `sessionId` - * (from `listCursorSessions`) to read an earlier session instead. - * Falls back to cwd when no project root is found. + * normally the conversation that is running right now. + * + * @param projectRoot - Project to read sessions for; `null` falls back to cwd. + * @param options - `sessionId` (from {@link listCursorSessions}) selects an + * earlier session; `maxEntries` caps returned transcript entries. + * @returns The transcript on success, or a result carrying the reason no + * session could be read. */ export async function readCursorSession( projectRoot: string | null, diff --git a/src/helpers/signup.ts b/src/helpers/signup.ts index 8bd2dc25..e2470ed6 100644 --- a/src/helpers/signup.ts +++ b/src/helpers/signup.ts @@ -21,9 +21,6 @@ export class SignupRequiredError extends Error { } } -/** - * Returns true if the error message indicates the user needs to sign up. - */ export function isSignupRequiredError(message?: string): boolean { if (!message) return false; const lower = message.toLowerCase(); diff --git a/src/helpers/stack-detect.ts b/src/helpers/stack-detect.ts index 62cd3ff5..0242884a 100644 --- a/src/helpers/stack-detect.ts +++ b/src/helpers/stack-detect.ts @@ -47,7 +47,6 @@ const PyprojectSchema = z.object({ .optional(), }); -/** Config file extensions commonly used by JS/TS frameworks. */ const JS_CONFIG_EXTENSIONS = ["js", "cjs", "mjs", "ts", "mts", "cts"]; /** Check whether any of `.` exists in `dir`. */ @@ -192,7 +191,6 @@ async function writeCache( export async function detectStack(projectRoot: string): Promise { const fingerprint = buildFingerprint(projectRoot); - // Check disk cache const cached = await readCache(projectRoot); if (cached && cached.fingerprint === fingerprint) { logDebug("Stack cache hit for", projectRoot); diff --git a/src/helpers/telemetry-config.ts b/src/helpers/telemetry-config.ts index 926c4a1a..8ff38ce2 100644 --- a/src/helpers/telemetry-config.ts +++ b/src/helpers/telemetry-config.ts @@ -2,13 +2,9 @@ // Copyright 2026 Archgate /** * telemetry-config.ts — Manages telemetry preferences in ~/.archgate/config.json. - * - * Telemetry is opt-out: enabled by default, users disable via: - * - ARCHGATE_TELEMETRY=0 environment variable - * - `archgate telemetry disable` command - * - * An anonymous installId (UUID v4) is generated on first use for aggregate - * counting — it is not derived from any user data. + * Telemetry is opt-out: disable via ARCHGATE_TELEMETRY=0 or + * `archgate telemetry disable`. An anonymous installId (UUID v4, not derived + * from user data) is generated on first use for aggregate counting. */ import { randomUUID } from "node:crypto"; @@ -113,9 +109,6 @@ export function loadTelemetryConfig(): TelemetryConfig { return cachedConfig; } -/** - * Update telemetry enabled/disabled state and persist to disk. - */ export async function setTelemetryEnabled(enabled: boolean): Promise { const config = loadTelemetryConfig(); config.telemetry = enabled; @@ -123,9 +116,6 @@ export async function setTelemetryEnabled(enabled: boolean): Promise { await saveTelemetryConfig(config); } -/** - * Get the anonymous install ID for this CLI installation. - */ export function getInstallId(): string { return loadTelemetryConfig().installId; } diff --git a/src/helpers/telemetry.ts b/src/helpers/telemetry.ts index c84e9adf..222cd773 100644 --- a/src/helpers/telemetry.ts +++ b/src/helpers/telemetry.ts @@ -1,17 +1,11 @@ // SPDX-License-Identifier: Apache-2.0 // Copyright 2026 Archgate /** - * telemetry.ts — Anonymous usage analytics via PostHog Node SDK. - * - * Uses the official posthog-node SDK for event capture with automatic - * batching and flush. Events are captured during command execution and - * flushed before process exit. - * - * IP anonymization: the CLI sends `$ip: null` on every event to signal - * PostHog to resolve geo server-side then discard the IP. The project - * also has "Discard client IP data" enabled in PostHog settings. - * - * See https://cli.archgate.dev/reference/telemetry for the full privacy policy. + * telemetry.ts — Anonymous usage analytics via the posthog-node SDK, with + * events captured during command execution and flushed before process exit. + * Every event sends `$ip: null` so PostHog resolves geo server-side and + * discards the IP. Full privacy policy: + * https://cli.archgate.dev/reference/telemetry */ import { basename } from "node:path"; @@ -106,16 +100,11 @@ function detectLocale(): string | null { // --------------------------------------------------------------------------- /** - * Cache of the slow-changing portion of the common event properties. Platform - * detection, install-method detection, CI detection, and locale resolution - * are all effectively constant for the lifetime of a CLI invocation, so we - * compute them once and reuse across events. - * - * The project context and repo snapshot are intentionally NOT cached here: - * - project context changes when `archgate init` creates the directory - * mid-command, so we always re-read it. - * - repo context lives in `repoContextSnapshot` which is written by - * `initTelemetry`. + * Cache of common event properties that stay constant for the lifetime of a + * CLI invocation (platform, install method, CI, locale). Project context is + * intentionally NOT cached (`archgate init` can create the directory + * mid-command); repo context lives in `repoContextSnapshot`, written by + * `initTelemetry`. */ let staticPropertiesSnapshot: Record | null = null; @@ -169,14 +158,10 @@ function getCommonProperties(): Record { // --------------------------------------------------------------------------- /** - * Initialize telemetry. Call once at CLI startup. - * If telemetry is disabled, this is a no-op and all subsequent calls are too. - * - * Returns a promise that resolves once the async repo-context lookup is done. - * Callers should `await` before emitting events so every event carries - * `repo_id` / `repo_host` — emitting before the await resolves means the - * event ships without repo identity. The repo lookup runs a handful of git - * subprocesses (cached per-process), so the added startup latency is small. + * Initialize telemetry. Call once at CLI startup; a no-op when telemetry is + * disabled. Callers must `await` the returned promise before emitting events + * so every event carries `repo_id` / `repo_host` from the async repo-context + * lookup (a few git subprocesses, cached per-process). */ export async function initTelemetry(): Promise { if (!isTelemetryEnabled()) { @@ -205,21 +190,16 @@ export async function initTelemetry(): Promise { const { PostHog } = await import("posthog-node"); client = new PostHog(POSTHOG_API_KEY, { host: POSTHOG_HOST, - // Disable polling for feature flags — we don't use them in the CLI disableGeoip: false, flushAt: 20, - // Disable automatic interval-based flushing. The CLI runs a single - // command and exits — we flush explicitly via `client.shutdown()` in - // `flushTelemetry()`. A 10s auto-flush timer is harmful: if the user - // is behind a corporate proxy with SSL inspection (self-signed cert), - // the timer fires mid-command and the PostHog SDK logs the TLS error - // via stderr, dumping an ugly stack trace into the CLI output. + // No interval-based auto-flush: the CLI flushes explicitly via + // `client.shutdown()` in `flushTelemetry()`. A timer firing mid-command + // would let the SDK dump TLS errors to stderr behind SSL-inspecting + // proxies. flushInterval: 0, - // Wrap fetch so network / TLS errors never reach the PostHog SDK's - // internal logFlushError → stderr path. Telemetry is - // non-critical: silently dropping events is preferable to printing - // unactionable errors (e.g. SELF_SIGNED_CERT_IN_CHAIN behind a - // corporate proxy). + // Wrap fetch so network/TLS errors never reach the SDK's stderr + // logging. Telemetry is non-critical: silently dropping events beats + // printing unactionable proxy/TLS errors. fetch: async (url, options) => { try { return await fetch(url, options); @@ -344,9 +324,6 @@ export function trackCheckResult(properties: { trackEvent("check_completed", properties); } -/** - * Track the outcome of `archgate init`. - */ export function trackInitResult(properties: { editor: string; plugin_installed: boolean; @@ -357,13 +334,11 @@ export function trackInitResult(properties: { } /** - * Track the `project_initialized` event on `archgate init`. - * - * Identity (raw remote URL / owner / name) ships only when the repo is - * confirmed public on a recognised host AND the user has not opted out via - * `--no-share-repo-identity` or `ARCHGATE_SHARE_REPO_IDENTITY=0`. The hashed - * `repo_id` is always included via common properties — it lets us count - * repos without learning names. + * Track the `project_initialized` event on `archgate init`. Raw repo + * identity (remote URL / owner / name) ships only when the repo is confirmed + * public on a recognised host AND the user has not opted out via + * `--no-share-repo-identity` or `ARCHGATE_SHARE_REPO_IDENTITY=0`; the hashed + * `repo_id` always ships via common properties. */ export function trackProjectInitialized(properties: { editors: string[]; @@ -387,9 +362,6 @@ export function trackProjectInitialized(properties: { trackEvent("project_initialized", properties); } -/** - * Track the outcome of `archgate upgrade`. - */ export function trackUpgradeResult(properties: { from_version: string; to_version: string; @@ -401,7 +373,6 @@ export function trackUpgradeResult(properties: { trackEvent("upgrade_completed", properties); } -/** Track the outcome of `archgate login`. */ export function trackLoginResult(properties: { subcommand: "login" | "logout" | "refresh" | "status"; success: boolean; @@ -417,7 +388,6 @@ export function trackTelemetryPreferenceChange(properties: { trackEvent("telemetry_preference_changed", properties); } -/** Track when the greenfield wizard prompt is displayed. */ export function trackGreenfieldWizardShown(): void { trackEvent("adoption.greenfield_wizard_shown"); } @@ -459,20 +429,22 @@ export function trackCustomDomainRemoved(properties: { } /** - * Flush pending events to PostHog. Call before process exit to ensure - * events are delivered. + * Flush pending events to PostHog. Call before process exit so events are + * delivered. + * + * @param timeoutMs - How long to wait for the flush before giving up, so a + * slow or blocked network cannot hang the exit path. + * @defaultValue 3000 */ export async function flushTelemetry(timeoutMs = 3000): Promise { if (!initialized || !client) return; try { logDebug("Flushing telemetry events"); - // Race shutdown against a timeout to prevent hanging on exit. - // - // The timeout MUST be cancelled when shutdown wins — a dangling - // `setTimeout` keeps the Bun/Node event loop alive for its full - // duration, which used to add 3s of latency to every command that - // exited via `main()` returning naturally (instead of `process.exit`). + // Race shutdown against a timeout to prevent hanging on exit. The + // timeout MUST be cancelled when shutdown wins — a dangling `setTimeout` + // keeps the event loop alive for its full duration, adding latency to + // every command that exits via `main()` returning naturally. let timer: ReturnType | undefined; try { await Promise.race([ diff --git a/src/helpers/update-check.ts b/src/helpers/update-check.ts index bf88929f..1e065561 100644 --- a/src/helpers/update-check.ts +++ b/src/helpers/update-check.ts @@ -7,7 +7,7 @@ import { logDebug } from "./log"; import { internalPath } from "./paths"; const CACHE_FILE = "last-update-check"; -const CACHE_TTL_MS = 24 * 60 * 60 * 1000; // 24 hours +const CACHE_TTL_MS = 24 * 60 * 60 * 1000; /** * Only check for updates in a genuine interactive terminal — never during diff --git a/src/helpers/user-error.ts b/src/helpers/user-error.ts index 70cf680b..64523c5b 100644 --- a/src/helpers/user-error.ts +++ b/src/helpers/user-error.ts @@ -1,17 +1,11 @@ // SPDX-License-Identifier: Apache-2.0 // Copyright 2026 Archgate /** - * user-error.ts — Typed error class for expected, user-facing failures. - * - * Helpers throw {@link UserError} for errors that are part of normal CLI - * operation: invalid input, missing config, network failures, auth - * rejections, etc. These are "exit-code-1" errors — the user (or their - * environment) needs to fix something, not us. - * - * Any error that is **not** a {@link UserError} is treated as an - * unexpected bug and captured to Sentry via {@link handleCommandError} - * in `exit.ts`. This keeps Sentry focused on genuine crashes rather - * than being flooded with routine validation noise. + * user-error.ts — Typed error class for expected, user-facing failures + * (invalid input, missing config, network/auth errors): "exit-code-1" + * errors the user fixes. {@link handleCommandError} in `exit.ts` captures + * to Sentry anything that is neither a {@link UserError} nor an + * `ExitPromptError` (Ctrl+C cancellation, exit 130). * * @example * ```ts diff --git a/src/helpers/vscode-settings.ts b/src/helpers/vscode-settings.ts index 9018a478..63428c1e 100644 --- a/src/helpers/vscode-settings.ts +++ b/src/helpers/vscode-settings.ts @@ -122,7 +122,6 @@ export async function configureVscodeSettings( ): Promise { const vscodeDir = join(projectRoot, ".vscode"); - // --- User-level: chat.plugins.marketplaces --- if (marketplaceUrl) { await addMarketplaceToUserSettings(marketplaceUrl); } diff --git a/tests/commands/adr.test.ts b/tests/commands/adr.test.ts index 06bbf811..ddeae1a1 100644 --- a/tests/commands/adr.test.ts +++ b/tests/commands/adr.test.ts @@ -180,12 +180,10 @@ describe("adr update", () => { title: "Completely Different Title", }); - // Filename should not change even when title changes expect(result.fileName).toBe(created.fileName); expect(result.filePath).toBe(created.filePath); expect(existsSync(result.filePath)).toBe(true); - // Only one file should exist (same path) const files = readdirSync(paths.adrsDir).filter((f) => f.endsWith(".md")); expect(files).toHaveLength(1); }); diff --git a/tests/commands/adr/create.test.ts b/tests/commands/adr/create.test.ts index bd485cd3..d900fcbb 100644 --- a/tests/commands/adr/create.test.ts +++ b/tests/commands/adr/create.test.ts @@ -285,7 +285,6 @@ describe("adr create action handler", () => { const adrsDir = join(tempDir, ".archgate", "adrs"); mkdirSync(adrsDir, { recursive: true }); - // Create a first ADR process.chdir(tempDir); const parent1 = makeProgram(); await parent1.parseAsync([ @@ -298,7 +297,6 @@ describe("adr create action handler", () => { "backend", ]); - // Create a second ADR in the same domain const parent2 = makeProgram(); await parent2.parseAsync([ "node", diff --git a/tests/commands/adr/import.test.ts b/tests/commands/adr/import.test.ts index e78f0ac6..0fd02037 100644 --- a/tests/commands/adr/import.test.ts +++ b/tests/commands/adr/import.test.ts @@ -299,7 +299,6 @@ describe("import action handler", () => { expect(output).toContain("TP-002"); expect(output).toContain("ARCH-"); expect(output).toContain("Dry run"); - // No files written const files = readdirSync(join(tempDir, ".archgate", "adrs")); expect(files.filter((f) => f.endsWith(".md"))).toHaveLength(0); }); @@ -357,7 +356,6 @@ describe("import action handler", () => { expect(content).toContain(`id: ${prefix}`); } - // Human-readable success message expect(allOutput()).toContain("Imported 2 ADR(s)"); }); @@ -379,7 +377,6 @@ describe("import action handler", () => { // TP-001 has a companion .rules.ts, TP-002 does not expect(rulesFiles).toHaveLength(1); expect(rulesFiles[0]).toMatch(/^ARCH-\d{3}-.*\.rules\.ts$/u); - // rules.d.ts shim created expect(existsSync(join(tempDir, ".archgate", "rules.d.ts"))).toBe(true); }); diff --git a/tests/commands/adr/sync.test.ts b/tests/commands/adr/sync.test.ts index cf28af1f..1eef53b4 100644 --- a/tests/commands/adr/sync.test.ts +++ b/tests/commands/adr/sync.test.ts @@ -420,7 +420,6 @@ describe("adr sync command", () => { expect(output()).toContain("Decision"); }); - // Non-interactive (no TTY, no --yes) skips updates test("non-interactive without --yes skips changes", async () => { const localPath = setupSync("Old.", "New."); await run(); diff --git a/tests/commands/check-security.test.ts b/tests/commands/check-security.test.ts index 24bf71c3..8c539cd3 100644 --- a/tests/commands/check-security.test.ts +++ b/tests/commands/check-security.test.ts @@ -147,7 +147,6 @@ describe("check command security", () => { }); test("blocks symlink to file outside project", async () => { - // Create a real file outside the project const outsideDir = mkdtempSync(join(tmpdir(), "archgate-outside-")); writeFileSync(join(outsideDir, "secret.txt"), "sensitive data"); diff --git a/tests/commands/check.test.ts b/tests/commands/check.test.ts index 1d7a486d..c596d31e 100644 --- a/tests/commands/check.test.ts +++ b/tests/commands/check.test.ts @@ -61,7 +61,6 @@ describe("registerCheckCommand", () => { const sub = program.commands.find((c) => c.name() === "check")!; const adrOpt = sub.options.find((o) => o.long === "--adr"); expect(adrOpt).toBeDefined(); - // The option takes a required value argument when used expect(adrOpt!.flags).toContain(""); }); @@ -77,7 +76,6 @@ describe("registerCheckCommand", () => { const program = new Command(); registerCheckCommand(program); const sub = program.commands.find((c) => c.name() === "check")!; - // Commander stores registered arguments const args = sub.registeredArguments; expect(args).toHaveLength(1); expect(args[0].name()).toBe("files"); diff --git a/tests/commands/clean.test.ts b/tests/commands/clean.test.ts index c7c7f0e7..2352bcaf 100644 --- a/tests/commands/clean.test.ts +++ b/tests/commands/clean.test.ts @@ -120,7 +120,6 @@ describe("clean action handler", () => { const program = makeProgram(); await program.parseAsync(["node", "test", "clean"]); - // cache/ should be removed, bin/ should be preserved expect(existsSync(binDir)).toBe(true); expect(existsSync(join(archgateDir, "cache"))).toBe(false); diff --git a/tests/commands/doctor.test.ts b/tests/commands/doctor.test.ts index cca9bd14..381a9f6f 100644 --- a/tests/commands/doctor.test.ts +++ b/tests/commands/doctor.test.ts @@ -159,7 +159,6 @@ describe("doctor action handler", () => { .map((c: unknown[]) => String(c[0])) .join("\n"); - // Section headers expect(output).toContain("System"); expect(output).toContain("Archgate"); expect(output).toContain("Project"); diff --git a/tests/commands/init.test.ts b/tests/commands/init.test.ts index 57aa779d..b22618d7 100644 --- a/tests/commands/init.test.ts +++ b/tests/commands/init.test.ts @@ -30,7 +30,6 @@ describe("init governance skeleton", () => { const exampleAdr = generateExampleAdr("test-project"); await Bun.write(`${paths.adrsDir}/GEN-001-example.md`, exampleAdr); - // Verify structure expect(existsSync(paths.root)).toBe(true); expect(existsSync(paths.adrsDir)).toBe(true); expect(existsSync(paths.lintDir)).toBe(true); diff --git a/tests/commands/login.test.ts b/tests/commands/login.test.ts index 1bbbf09c..44151215 100644 --- a/tests/commands/login.test.ts +++ b/tests/commands/login.test.ts @@ -213,7 +213,6 @@ describe("login action handlers", () => { .join("\n"); expect(allOutput).toContain("Already logged in"); expect(allOutput).toContain("octocat"); - // runLoginFlow should NOT have been called expect(runLoginFlowSpy).not.toHaveBeenCalled(); }); diff --git a/tests/commands/plugin/url.test.ts b/tests/commands/plugin/url.test.ts index 12189653..174cabe0 100644 --- a/tests/commands/plugin/url.test.ts +++ b/tests/commands/plugin/url.test.ts @@ -34,10 +34,6 @@ import { buildVscodeMarketplaceUrl, } from "../../../src/helpers/plugin-install"; -// --------------------------------------------------------------------------- -// Tests -// --------------------------------------------------------------------------- - describe("registerPluginUrlCommand", () => { test("registers 'url' as a subcommand", () => { const program = new Command(); diff --git a/tests/commands/review-context.test.ts b/tests/commands/review-context.test.ts index 7694d74f..08e3d48f 100644 --- a/tests/commands/review-context.test.ts +++ b/tests/commands/review-context.test.ts @@ -10,10 +10,6 @@ import { Command } from "@commander-js/extra-typings"; import { registerReviewContextCommand } from "../../src/commands/review-context"; import { safeRmSync } from "../test-utils"; -// --------------------------------------------------------------------------- -// Tests -// --------------------------------------------------------------------------- - describe("registerReviewContextCommand", () => { test("registers 'review-context' as a subcommand", () => { const program = new Command(); @@ -209,7 +205,6 @@ Test. .map((c: unknown[]) => String(c[0])) .join(""); const parsed = JSON.parse(output); - // All domains should only contain architecture entries for (const domain of parsed.domains) { expect(domain.domain).toBe("architecture"); } diff --git a/tests/commands/session-context/claude-code.test.ts b/tests/commands/session-context/claude-code.test.ts index 97bdb3ef..5a45ec9b 100644 --- a/tests/commands/session-context/claude-code.test.ts +++ b/tests/commands/session-context/claude-code.test.ts @@ -12,10 +12,6 @@ import * as sessionContextHelpers from "../../../src/helpers/session-context"; import { runCli } from "../../integration/cli-harness"; import { safeRmSync } from "../../test-utils"; -// --------------------------------------------------------------------------- -// Tests -// --------------------------------------------------------------------------- - describe("registerClaudeCodeSessionContextCommand", () => { test("registers 'claude-code' as a subcommand", () => { const parent = new Command("session-context"); diff --git a/tests/commands/session-context/copilot.test.ts b/tests/commands/session-context/copilot.test.ts index c129c69d..5790953b 100644 --- a/tests/commands/session-context/copilot.test.ts +++ b/tests/commands/session-context/copilot.test.ts @@ -11,10 +11,6 @@ import { registerCopilotSessionContextCommand } from "../../../src/commands/sess import * as copilotHelpers from "../../../src/helpers/session-context-copilot"; import { safeRmSync } from "../../test-utils"; -// --------------------------------------------------------------------------- -// Tests -// --------------------------------------------------------------------------- - describe("registerCopilotSessionContextCommand", () => { test("registers 'copilot' as a subcommand", () => { const parent = new Command("session-context"); diff --git a/tests/commands/session-context/cursor.test.ts b/tests/commands/session-context/cursor.test.ts index 596074a5..4ecb56c6 100644 --- a/tests/commands/session-context/cursor.test.ts +++ b/tests/commands/session-context/cursor.test.ts @@ -11,10 +11,6 @@ import { registerCursorSessionContextCommand } from "../../../src/commands/sessi import * as sessionContextHelpers from "../../../src/helpers/session-context"; import { safeRmSync } from "../../test-utils"; -// --------------------------------------------------------------------------- -// Tests -// --------------------------------------------------------------------------- - describe("registerCursorSessionContextCommand", () => { test("registers 'cursor' as a subcommand", () => { const parent = new Command("session-context"); diff --git a/tests/commands/session-context/opencode.test.ts b/tests/commands/session-context/opencode.test.ts index 0935a96f..d688178b 100644 --- a/tests/commands/session-context/opencode.test.ts +++ b/tests/commands/session-context/opencode.test.ts @@ -13,10 +13,6 @@ import * as opencodeHelpers from "../../../src/helpers/session-context-opencode" import { runCli } from "../../integration/cli-harness"; import { safeRmSync } from "../../test-utils"; -// --------------------------------------------------------------------------- -// Tests -// --------------------------------------------------------------------------- - describe("registerOpencodeSessionContextCommand", () => { test("registers 'opencode' as a subcommand", () => { const parent = new Command("session-context"); diff --git a/tests/commands/telemetry.test.ts b/tests/commands/telemetry.test.ts index 626350ae..96a0df51 100644 --- a/tests/commands/telemetry.test.ts +++ b/tests/commands/telemetry.test.ts @@ -205,7 +205,6 @@ describe("telemetry enable", () => { const output = collectOutput(logSpy); expect(output).toContain("ARCHGATE_TELEMETRY environment variable"); expect(output).toContain("Remove the environment variable"); - // Still calls setTelemetryEnabled expect(setTelemetryEnabledSpy).toHaveBeenCalledWith(true); }); @@ -231,7 +230,6 @@ describe("telemetry enable", () => { program.parseAsync(["node", "test", "telemetry", "enable"]) ).rejects.toThrow("prompt cancelled"); - // logError should NOT be called for ExitPromptError expect(logErrorSpy).not.toHaveBeenCalled(); }); }); diff --git a/tests/commands/upgrade.test.ts b/tests/commands/upgrade.test.ts index 2f217a78..4fcb73e7 100644 --- a/tests/commands/upgrade.test.ts +++ b/tests/commands/upgrade.test.ts @@ -201,15 +201,12 @@ describe("install method detection", () => { describe("_formatBytes", () => { test("formats bytes, KB, and MB ranges", () => { - // Bytes expect(_formatBytes(0)).toBe("0 B"); expect(_formatBytes(512)).toBe("512 B"); expect(_formatBytes(1023)).toBe("1023 B"); - // KB expect(_formatBytes(1024)).toBe("1.0 KB"); expect(_formatBytes(1536)).toBe("1.5 KB"); expect(_formatBytes(1024 * 100)).toBe("100.0 KB"); - // MB expect(_formatBytes(1024 * 1024)).toBe("1.0 MB"); expect(_formatBytes(1024 * 1024 * 5.5)).toBe("5.5 MB"); expect(_formatBytes(1024 * 1024 * 100)).toBe("100.0 MB"); @@ -264,7 +261,6 @@ describe("upgrade action handler", () => { return program; } - /** Mock fetch to return a GitHub release tag response. */ function mockGitHubRelease(tag: string | null) { globalThis.fetch = (() => Promise.resolve({ diff --git a/tests/engine/context.test.ts b/tests/engine/context.test.ts index 6e1121b9..39e1771d 100644 --- a/tests/engine/context.test.ts +++ b/tests/engine/context.test.ts @@ -124,7 +124,6 @@ describe("briefAdr", () => { expect(briefing.decision!.length).toBeLessThan(3000); expect(briefing.decision).toContain("[... truncated"); expect(briefing.decision).toContain("adr://ARCH-010"); - // Do's and Don'ts is short, should not be truncated expect(briefing.dosAndDonts).toBe("Short."); }); diff --git a/tests/engine/git-files.test.ts b/tests/engine/git-files.test.ts index 287a20a7..0ecbd6c6 100644 --- a/tests/engine/git-files.test.ts +++ b/tests/engine/git-files.test.ts @@ -72,7 +72,6 @@ describe("git-files", () => { writeFileSync(join(tempDir, "a.ts"), "export const a = 1;"); await git(["add", "a.ts"], tempDir); await git(["commit", "-m", "init"], tempDir); - // Stage a new file (staged change) writeFileSync(join(tempDir, "b.ts"), "export const b = 1;"); await git(["add", "b.ts"], tempDir); // Modify a committed file without staging (unstaged change) @@ -179,7 +178,6 @@ describe("git-files", () => { await git(["add", "file.ts"], tempDir); await git(["commit", "-m", "init"], tempDir); - // Pre-create config with a custom baseBranch mkdirSync(join(tempDir, ".archgate"), { recursive: true }); await Bun.write( join(tempDir, ".archgate", "config.json"), @@ -209,7 +207,6 @@ describe("git-files", () => { writeFileSync(join(tempDir, "base.ts"), "export const x = 1;"); await git(["add", "base.ts"], tempDir); await git(["commit", "-m", "init"], tempDir); - // Create feature branch and add files await git(["checkout", "-b", "feature"], tempDir); writeFileSync(join(tempDir, "new-file.ts"), "export const y = 2;"); await git(["add", "new-file.ts"], tempDir); @@ -231,8 +228,8 @@ describe("git-files", () => { }); // Regression: archgate/cli#403 — base...HEAD only sees committed - // changes, so uncommitted working-tree edits were silently omitted - // whenever a base ref was detected (i.e., almost always). + // changes, so uncommitted working-tree edits must be unioned in + // whenever a base ref is detected (i.e., almost always). test("includes uncommitted edits to tracked files (regression archgate/cli#403)", async () => { await git(["init", "--initial-branch=main"], tempDir); await git(["config", "user.email", "test@test.com"], tempDir); @@ -364,9 +361,9 @@ describe("git-files", () => { }); // Regression: archgate/cli#222 — ADR `files:` globs must match - // dot-prefixed source dirs like `.github/`. Bun.Glob with `dot: false` - // silently drops these on Windows, so ADRs scoped to `.github/**` had - // empty scopedFiles on Windows local-dev runs. + // dot-prefixed source dirs like `.github/`. Bun.Glob defaults to + // `dot: false`, which silently drops these on Windows, so the scan + // must opt in for ADRs scoped to `.github/**` to see any files. test("resolves dot-prefixed paths (regression archgate/cli#222)", async () => { await git(["init"], tempDir); mkdirSync(join(tempDir, ".github", "workflows"), { recursive: true }); diff --git a/tests/engine/glob-utils.test.ts b/tests/engine/glob-utils.test.ts index f5ebc7af..02df49db 100644 --- a/tests/engine/glob-utils.test.ts +++ b/tests/engine/glob-utils.test.ts @@ -15,8 +15,8 @@ import { git, safeRmSync } from "../test-utils"; describe("matchLines", () => { test("reports the true 1-based column with a global regex", () => { - // Regression: `String.match` with a `/g` pattern strips `.index`, which - // collapsed the column to 1. `matchLines` now uses `exec()` so the column + // Regression: `String.match` with a `/g` pattern strips `.index` and + // collapses the column to 1, so `matchLines` uses `exec()` and the column // reflects the real match offset. const matches = matchLines("const x = TODO;\n", /TODO/gu, "a.ts"); expect(matches).toHaveLength(1); diff --git a/tests/engine/loader.test.ts b/tests/engine/loader.test.ts index c2fa2f80..65eeb107 100644 --- a/tests/engine/loader.test.ts +++ b/tests/engine/loader.test.ts @@ -232,17 +232,14 @@ export default { invalid syntax here !!! } satisfies RuleSet; }); test("loads ADRs from custom directory configured in config.json", async () => { - // Create a custom ADR directory outside .archgate/ const customAdrsDir = join(tempDir, "docs", "adrs"); mkdirSync(customAdrsDir, { recursive: true }); - // Configure the custom path await saveProjectConfig(tempDir, { domains: {}, paths: { adrs: "docs/adrs" }, }); - // Place ADR + rules in the custom directory copyFileSync( join(fixturesDir, "TEST-001-sample.md"), join(customAdrsDir, "TEST-001-sample.md") @@ -276,7 +273,6 @@ export default { invalid syntax here !!! } satisfies RuleSet; }); test("parseAllAdrs reads from custom directory", async () => { - // Configure custom path const customAdrsDir = join(tempDir, "governance"); mkdirSync(customAdrsDir, { recursive: true }); diff --git a/tests/engine/reporter.test.ts b/tests/engine/reporter.test.ts index 4e72a649..ff898007 100644 --- a/tests/engine/reporter.test.ts +++ b/tests/engine/reporter.test.ts @@ -309,7 +309,6 @@ describe("reporter", () => { const summary = buildSummary(makeResult({ violations }), { maxViolationsPerRule: 5, }); - // errors count should reflect ALL 25 violations, not just the shown 5 expect(summary.errors).toBe(25); expect(summary.results[0].shownViolations).toBe(5); }); diff --git a/tests/engine/rule-scanner-escapes.test.ts b/tests/engine/rule-scanner-escapes.test.ts index c04477eb..81253a7e 100644 --- a/tests/engine/rule-scanner-escapes.test.ts +++ b/tests/engine/rule-scanner-escapes.test.ts @@ -3,15 +3,12 @@ /** * Sandbox-escape regression tests for the `.rules.ts` security scanner. * - * `archgate check` imports and executes every companion `.rules.ts` in-process, - * so `scanRuleSource()` is the only thing standing between a rule file and - * arbitrary code execution on the machine running the check. ARCH-022 depends - * on that boundary holding: it states a rule author "MUST NEVER be able to - * reach `Bun.spawn`, `child_process`, or any other subprocess/filesystem - * primitive directly." + * `archgate check` executes every companion `.rules.ts` in-process, so + * `scanRuleSource()` is the only barrier against arbitrary code execution. + * Each case below is one route around the invariant that a rule author never + * reaches `Bun.spawn`, `child_process`, or another subprocess/fs primitive. * - * Every case here is a way that boundary was, or could be, walked around. - * They are grouped in their own file so the list reads as one attack surface. + * @see ARCH-024 for the boundary definition, ARCH-022 for the rule context. */ import { describe, expect, test } from "bun:test"; @@ -21,10 +18,10 @@ import { } from "../../src/engine/rule-scanner"; describe("rule sandbox escapes", () => { - // Regression: the ImportExpression case rejected only *non-literal* - // arguments, so a constant specifier skipped the module ban that - // ImportDeclaration enforced. `await import("node:child_process")` executed - // at import time and `check` still reported the ADR as passing. + // The ImportExpression case must apply the module ban to *literal* + // specifiers too, not just non-literal ones: an unbanned + // `await import("node:child_process")` executes at import time and `check` + // still reports the ADR as passing. describe("dynamic import with a literal specifier", () => { for (const mod of [ "node:child_process", @@ -303,7 +300,7 @@ const b = 2${RLO};`); // `Bun`, `process`, and the global object are LIVE globals in the rule // runtime — reachable with no import at all. Blocking the syntactic shapes - // (`Bun.spawn`, `Bun[x]`) is the same losing game the module denylist was: + // (`Bun.spawn`, `Bun[x]`) is the same losing game as a module denylist: // aliasing, destructuring, reflection, and global-object aliases all reach // the identical capability. The scanner instead blocks *naming* the global. describe("reflective and aliased access to runtime globals", () => { @@ -330,7 +327,7 @@ const b = 2${RLO};`); } // Every eval-equivalent identifier is banned, and — crucially — so is - // aliasing it, which the old callee-name checks missed. + // aliasing it, which a callee-name check alone would miss. const codegen: Array<[string, string]> = [ ["eval()", `eval("x");`], ["aliased eval", `const e = eval;\ne("x");`], @@ -398,12 +395,11 @@ const b = 2${RLO};`); ).toHaveLength(0); }); - // A property name built at runtime (`obj[variable]`) is unknowable to a - // scanner that does not track values — the documented static-analysis limit - // (see ARCH-024). Blocking all computed access would reject ordinary - // `arr[i]`/`obj[key]`, so this residual is left to execution-time - // isolation, not chased with more pattern-matching. Asserted so the limit - // is explicit rather than an accidental gap. + // A runtime-built property name (`obj[variable]`) is unknowable to a + // scanner that does not track values — the documented limit in ARCH-024. + // Blocking all computed access would reject ordinary `arr[i]`/`obj[key]`, + // so this residual is left to execution-time isolation. Asserted so the + // limit stays explicit rather than becoming an accidental gap. test("does NOT catch .constructor via a runtime-computed key (known limit)", () => { expect( scanRuleSource(`const c = "constructor";\nconst F = (() => {})[c];`) @@ -417,15 +413,11 @@ const b = 2${RLO};`); }); }); - // Regression: a node the AST-node schema fails to validate is dropped by - // `parseNode` *with its entire subtree*, so anything dangerous underneath it - // goes unscanned — a silent false-negative `check` reports as a pass. The - // schema's only leaf that can fail is a Literal's `value`: `type` is always - // present and every other typed field recurses back into the schema. Meriyah - // emits shapes a narrow `value` union rejects — an object for a RegExp - // literal, a `bigint` for `123n` — so a payload hidden behind such a literal - // (`/x/.constructor.constructor`, a banned call to the right of `/re/ + …`) - // escaped the walk entirely. `value` is now tolerant of any shape. + // A node the AST-node schema rejects is dropped by `parseNode` with its + // entire subtree, so anything dangerous underneath goes unscanned — a silent + // false negative that `check` reports as a pass. A Literal's `value` is the + // only leaf that can fail validation (meriyah emits an object for a RegExp + // literal, a `bigint` for `123n`), so it stays tolerant of any shape. describe("payloads behind exotic-literal receivers stay in the walk", () => { const escapes: Array<[string, string]> = [ [ @@ -464,7 +456,7 @@ const b = 2${RLO};`); } // Positive controls: the literals themselves are perfectly legal in a rule - // file — the fix must keep the node in the walk, not start flagging it. + // file — the scanner keeps the node in the walk without flagging it. test("a clean RegExp literal still passes", () => { expect( scanRuleSource( diff --git a/tests/engine/rule-scanner.test.ts b/tests/engine/rule-scanner.test.ts index 88fb419a..b094caf6 100644 --- a/tests/engine/rule-scanner.test.ts +++ b/tests/engine/rule-scanner.test.ts @@ -114,12 +114,10 @@ describe("scanRuleSource", () => { }); // A specifier-only local export (`export { x }`, no `from`) also carries - // `source: null`, but it holds no scannable subtree: an export specifier - // must name a module-local binding, so `export { fetch as local }` naming - // the global is not valid ESM, and `Bun.Transpiler` erases the undeclared - // specifier down to `export {}` before the walk ever sees it. There is thus - // no reference to flag — the declaration-form cases above are what actually - // guard the `source: null` subtree against a schema regression. + // `source: null` but holds no scannable subtree: a specifier must name a + // module-local binding, and `Bun.Transpiler` erases an undeclared one to + // `export {}` before the walk sees it. The declaration-form cases above + // are what guard the `source: null` subtree against a schema regression. test("a local export without a `from` clause has nothing to scan", () => { expect(scanRuleSource(`export { fetch as local };`)).toHaveLength(0); }); @@ -248,11 +246,10 @@ describe("scanRuleSource", () => { }); describe("scanImportedRuleSource", () => { - // scanImportedRuleSource now delegates to scanRuleSource: the patterns that - // were once imported-only (Bun.env, process.env, require, WebSocket) are - // blocked for every rule file by the banned-globals check, because each names - // a banned global. These confirm the block reaches through the imported - // entry point. + // scanImportedRuleSource delegates to scanRuleSource: Bun.env, process.env, + // require, and WebSocket each name a banned global, so the banned-globals + // check blocks them for every rule file. These confirm the block reaches + // through the imported entry point. describe("previously imported-only patterns are now always blocked", () => { const cases: Array<[string, string, string]> = [ ["Bun.env read", `const token = Bun.env.FOO;`, "Bun"], diff --git a/tests/formats/rules.test.ts b/tests/formats/rules.test.ts index 61391903..c7d8133c 100644 --- a/tests/formats/rules.test.ts +++ b/tests/formats/rules.test.ts @@ -2,8 +2,6 @@ // Copyright 2026 Archgate import { describe, expect, test } from "bun:test"; -// Value import (not `import type`) to ensure the module is loaded at runtime, -// which is necessary for code coverage to register the file. import type { GrepMatch, PackageJson, @@ -20,8 +18,6 @@ import * as rulesModule from "../../src/formats/rules"; describe("formats/rules module", () => { test("module is loadable at runtime", () => { - // The module exports only types, but importing it as a value ensures the - // runtime evaluates the file, making it appear in coverage reports. expect(rulesModule).toBeDefined(); expect(typeof rulesModule).toBe("object"); }); diff --git a/tests/helpers/auth.test.ts b/tests/helpers/auth.test.ts index c80aa708..37831264 100644 --- a/tests/helpers/auth.test.ts +++ b/tests/helpers/auth.test.ts @@ -36,11 +36,10 @@ describe("auth", () => { afterEach(() => { // `Bun.env.X = undefined` assigns the STRING "undefined" and leaves the key - // present — it does not unset. Since HOME and GIT_CONFIG_GLOBAL are normally - // unset on Windows, a plain restore leaked HOME="undefined" into the shared - // process env, and every later test that spawned a subprocess inherited it. - // Bun.env is process-global and test files share one process, so this must - // delete when the original was absent. + // present — it does not unset. HOME and GIT_CONFIG_GLOBAL are normally unset + // on Windows, and Bun.env is process-global across test files, so a plain + // restore would publish HOME="undefined" to every later test and any + // subprocess it spawns. restoreEnv deletes when the capture was absent. restoreEnv("HOME", originalHome); restoreEnv("USERPROFILE", originalUserProfile); restoreEnv("GIT_CONFIG_NOSYSTEM", originalGitConfigNoSystem); diff --git a/tests/helpers/binary-upgrade.test.ts b/tests/helpers/binary-upgrade.test.ts index 4dbfa757..02b55390 100644 --- a/tests/helpers/binary-upgrade.test.ts +++ b/tests/helpers/binary-upgrade.test.ts @@ -143,7 +143,6 @@ describe("downloadReleaseBinary", () => { }); test("calls onProgress callback with streaming progress", async () => { - // Create a fake ReadableStream that yields two chunks const chunk1 = new Uint8Array([1, 2, 3, 4]); const chunk2 = new Uint8Array([5, 6, 7, 8, 9, 10]); const totalSize = chunk1.byteLength + chunk2.byteLength; @@ -168,7 +167,6 @@ describe("downloadReleaseBinary", () => { body: stream, } as Response); } - // Checksum fetch — not available return Promise.resolve({ ok: false, status: 404 } as Response); }) as unknown as typeof fetch; @@ -379,18 +377,16 @@ describe("replaceBinary", () => { const currentPath = join(tmpDir, "archgate"); const newBinaryPath = join(tmpDir, "archgate.new"); - // Create placeholder files writeFileSync(currentPath, "old binary content"); writeFileSync(newBinaryPath, "new binary content"); replaceBinary(currentPath, newBinaryPath); - // new binary should have been renamed to currentPath + // after replaceBinary, currentPath holds the new binary expect(existsSync(currentPath)).toBe(true); - // the new binary path should no longer exist (it was renamed) + // and the staging path must be gone (rename, not copy) expect(existsSync(newBinaryPath)).toBe(false); - // verify chmod 755 was applied const mode = statSync(currentPath).mode & 0o777; expect(mode).toBe(0o755); } @@ -450,7 +446,6 @@ describe("replaceBinary", () => { expect(existsSync(oldPath)).toBe(true); expect(existsSync(newBinaryPath)).toBe(false); - // The .old file should contain "old binary content" (the just-replaced binary) const oldContent = readFileSync(oldPath, "utf8"); expect(oldContent).toBe("old binary content"); } @@ -493,7 +488,6 @@ describe("cleanupStaleBinary", () => { const tmpDir = mkdtempSync(join(tmpdir(), "archgate-cleanup-test-")); Bun.env.HOME = tmpDir; - // No .old file — should not throw await expect(cleanupStaleBinary()).resolves.toBeUndefined(); }); }); diff --git a/tests/helpers/claude-settings.test.ts b/tests/helpers/claude-settings.test.ts index 21832b0a..52fddeae 100644 --- a/tests/helpers/claude-settings.test.ts +++ b/tests/helpers/claude-settings.test.ts @@ -135,13 +135,9 @@ describe("configureClaudeSettings", () => { const content = JSON.parse( await Bun.file(join(claudeDir, "settings.local.json")).text() ); - // Existing agent preserved expect(content.agent).toBe("my-custom-agent"); - // Custom key preserved expect(content.myCustomKey).toBe(true); - // Deny permissions preserved expect(content.permissions.deny).toEqual(["Bash(rm *)"]); - // Allow permissions appended expect(content.permissions.allow).toContain("Bash(git *)"); expect(content.permissions.allow).toContain("Skill(archgate:architect)"); }); diff --git a/tests/helpers/credential-store.test.ts b/tests/helpers/credential-store.test.ts index a9bb779b..cc448af8 100644 --- a/tests/helpers/credential-store.test.ts +++ b/tests/helpers/credential-store.test.ts @@ -32,10 +32,10 @@ describe("credential-store", () => { }); afterEach(() => { - // restoreEnv deletes when the original was unset. HOME and - // GIT_CONFIG_GLOBAL are normally unset on Windows, so a bare restore leaked - // them as the string "undefined" into every later test file and any - // subprocess those tests spawned (Bun.env is process-global). + // restoreEnv deletes when the captured value was unset, which matters + // because HOME and GIT_CONFIG_GLOBAL are normally unset on Windows: a + // bare assignment stores the string "undefined" and Bun.env is + // process-global, so it reaches every later test file and subprocess. restoreEnv("HOME", originalHome); restoreEnv("GIT_CONFIG_NOSYSTEM", originalGitConfigNoSystem); restoreEnv("GIT_CONFIG_GLOBAL", originalGitConfigGlobal); @@ -64,7 +64,6 @@ describe("credential-store", () => { test.skipIf(process.platform !== "win32")( "cleans up legacy metadata file on save", async () => { - // Create a legacy metadata file mkdirSync(join(tempDir, ".archgate"), { recursive: true }); const credPath = join(tempDir, ".archgate", "credentials"); await Bun.write( @@ -77,7 +76,6 @@ describe("credential-store", () => { github_user: "testuser", }); - // Legacy file should be removed. expect(await Bun.file(credPath).exists()).toBe(false); } ); @@ -212,13 +210,11 @@ describe("credential-store", () => { warnSpy.mockRestore(); } - // Load should return the saved credentials const loaded = await loadCredentials(); expect(loaded).not.toBeNull(); expect(loaded!.token).toBe("ag_beta_roundtrip"); expect(loaded!.github_user).toBe("rounduser"); - // Clear should remove them await clearCredentials(); const afterClear = await loadCredentials(); expect(afterClear).toBeNull(); diff --git a/tests/helpers/cursor-settings.test.ts b/tests/helpers/cursor-settings.test.ts index b39b3b6b..90a9bdc1 100644 --- a/tests/helpers/cursor-settings.test.ts +++ b/tests/helpers/cursor-settings.test.ts @@ -43,7 +43,6 @@ describe("configureCursorSettings", () => { configureCursorSettings(tempDir); const hooksPath = join(tempDir, ".cursor", "hooks.json"); await Bun.write(hooksPath, "custom hooks"); - // Re-run — should not overwrite configureCursorSettings(tempDir); expect(readFileSync(hooksPath, "utf-8")).toBe("custom hooks"); }); diff --git a/tests/helpers/doctor.test.ts b/tests/helpers/doctor.test.ts index ba559604..b52ed906 100644 --- a/tests/helpers/doctor.test.ts +++ b/tests/helpers/doctor.test.ts @@ -10,7 +10,6 @@ describe("doctor", () => { test("returns a complete DoctorReport structure", async () => { const report = await runDoctor(); - // System section expect(report.system).toBeDefined(); expect(typeof report.system.os).toBe("string"); expect(typeof report.system.arch).toBe("string"); @@ -18,7 +17,6 @@ describe("doctor", () => { expect(typeof report.system.bun_version).toBe("string"); expect(typeof report.system.node_version).toBe("string"); - // Archgate section expect(report.archgate).toBeDefined(); expect(typeof report.archgate.version).toBe("string"); expect(["binary", "proto", "local", "global-pm"]).toContain( @@ -28,17 +26,14 @@ describe("doctor", () => { expect(typeof report.archgate.telemetry_enabled).toBe("boolean"); expect(typeof report.archgate.logged_in).toBe("boolean"); - // Project section expect(report.project).toBeDefined(); expect(typeof report.project.has_project).toBe("boolean"); expect(typeof report.project.adr_count).toBe("number"); expect(Array.isArray(report.project.domains)).toBe(true); - // Editors section expect(report.editors).toBeDefined(); expect(typeof report.editors.git).toBe("boolean"); - // Integrations section expect(report.integrations).toBeDefined(); }); diff --git a/tests/helpers/editor-detect.test.ts b/tests/helpers/editor-detect.test.ts index 19037fc7..10b581de 100644 --- a/tests/helpers/editor-detect.test.ts +++ b/tests/helpers/editor-detect.test.ts @@ -26,10 +26,6 @@ import { promptSingleEditorSelection, } from "../../src/helpers/editor-detect"; -// --------------------------------------------------------------------------- -// Shared test data -// --------------------------------------------------------------------------- - const MOCK_DETECTED: DetectedEditor[] = [ { id: "claude", label: "Claude Code", available: true }, { id: "cursor", label: "Cursor", available: false }, @@ -38,10 +34,6 @@ const MOCK_DETECTED: DetectedEditor[] = [ { id: "opencode", label: "opencode", available: false }, ]; -// --------------------------------------------------------------------------- -// Tests -// --------------------------------------------------------------------------- - describe("editor-detect", () => { describe("detectEditors", () => { test("returns all five editors with availability status", async () => { diff --git a/tests/helpers/init-base-branch.test.ts b/tests/helpers/init-base-branch.test.ts index 7c4ca78c..07fb8bce 100644 --- a/tests/helpers/init-base-branch.test.ts +++ b/tests/helpers/init-base-branch.test.ts @@ -43,16 +43,13 @@ describe("initProject — baseBranch auto-detection", () => { await git(["add", "file.ts"], tempDir); await git(["commit", "-m", "init"], tempDir); - // First init saves baseBranch await initProject(tempDir); - // Manually change baseBranch to a custom value const configPath = join(tempDir, ".archgate", "config.json"); const config = JSON.parse(await Bun.file(configPath).text()); config.baseBranch = "develop"; await Bun.write(configPath, JSON.stringify(config, null, 2) + "\n"); - // Re-init should not overwrite the custom baseBranch await initProject(tempDir); const updatedConfig = JSON.parse(await Bun.file(configPath).text()); diff --git a/tests/helpers/init-project.test.ts b/tests/helpers/init-project.test.ts index f807b7fb..13672971 100644 --- a/tests/helpers/init-project.test.ts +++ b/tests/helpers/init-project.test.ts @@ -64,7 +64,6 @@ describe("initProject", () => { expect(second.adrsDir).toBe(first.adrsDir); expect(second.lintDir).toBe(first.lintDir); - // Directories and scaffolding files still exist after re-init expect(existsSync(join(tempDir, ".archgate", "lint", "README.md"))).toBe( true ); @@ -82,17 +81,14 @@ describe("initProject", () => { expect(existsSync(join(tempDir, ".cursor", "hooks.json"))).toBe(true); expect(existsSync(join(tempDir, ".cursor", "rules"))).toBe(false); - // Claude settings should NOT exist expect(existsSync(join(tempDir, ".claude", "settings.local.json"))).toBe( false ); - // Result should point to .cursor/ directory expect(result.editorSettingsPath).toBe(join(tempDir, ".cursor")); }); test("skips example ADR when ADRs already exist", async () => { - // Pre-create .archgate/adrs/ with an existing ADR const adrsDir = join(tempDir, ".archgate", "adrs"); mkdirSync(adrsDir, { recursive: true }); await Bun.write( @@ -102,9 +98,7 @@ describe("initProject", () => { await initProject(tempDir); - // Example ADR should NOT have been generated expect(existsSync(join(adrsDir, "GEN-001-example.md"))).toBe(false); - // Existing ADR should be untouched expect(existsSync(join(adrsDir, "PROJ-001-existing.md"))).toBe(true); }); @@ -191,7 +185,6 @@ describe("initProject", () => { expect(existsSync(copilotDir)).toBe(true); expect(result.editorSettingsPath).toBe(copilotDir); - // Claude settings should NOT exist expect(existsSync(join(tempDir, ".claude", "settings.local.json"))).toBe( false ); @@ -208,7 +201,6 @@ describe("initProject", () => { const expectedDir = join(tempDir, ".config", "opencode", "agents"); expect(result.editorSettingsPath).toBe(expectedDir); - // Claude settings should NOT exist expect(existsSync(join(tempDir, ".claude", "settings.local.json"))).toBe( false ); @@ -232,7 +224,6 @@ describe("initProject", () => { const vscodeDir = join(tempDir, ".vscode"); expect(result.editorSettingsPath).toBe(vscodeDir); - // Claude settings should NOT exist expect(existsSync(join(tempDir, ".claude", "settings.local.json"))).toBe( false ); @@ -292,9 +283,8 @@ describe("tryInstallPlugin via initProject", () => { credSpy.mockResolvedValue({ token: "tok", github_user: "user" }); // With credentials present, configureEditorSettings (vscode branch) writes // the REAL user-level VS Code settings.json — spy it out so the test never - // touches user state. (This previously polluted ~/.config/Code/User/ - // settings.json on CI runners and dev machines, causing order-dependent - // failures in vscode-settings.test.ts.) + // touches user state; an unspied write pollutes ~/.config/Code/User/ and + // causes order-dependent failures in vscode-settings.test.ts. const settingsSpy = spyOn( vscodeSettings, "configureVscodeSettings" diff --git a/tests/helpers/install-info.test.ts b/tests/helpers/install-info.test.ts index a88958fc..d641a628 100644 --- a/tests/helpers/install-info.test.ts +++ b/tests/helpers/install-info.test.ts @@ -51,7 +51,7 @@ describe("install-info", () => { }); test("returns equal (not identical) contexts across calls", () => { - // getProjectContext is no longer cached — each call re-reads the + // getProjectContext is not cached — each call re-reads the // filesystem so post-init events reflect newly-created ADRs. const first = getProjectContext(); const second = getProjectContext(); diff --git a/tests/helpers/login-flow.test.ts b/tests/helpers/login-flow.test.ts index b2d97f99..54d9a9bc 100644 --- a/tests/helpers/login-flow.test.ts +++ b/tests/helpers/login-flow.test.ts @@ -62,10 +62,6 @@ import { SignupRequiredError } from "../../src/helpers/signup"; let originalFetch: typeof globalThis.fetch; -// --------------------------------------------------------------------------- -// Tests -// --------------------------------------------------------------------------- - describe("login-flow", () => { beforeEach(() => { // Silence console output (restored via mock.restore() in afterEach). @@ -138,10 +134,6 @@ describe("login-flow", () => { expect(failure.githubUser).toBeUndefined(); }); - // ----------------------------------------------------------------------- - // Successful login flow - // ----------------------------------------------------------------------- - test("successful login: device code -> poll -> claim -> save", async () => { const result = await runLoginFlow(); @@ -163,10 +155,6 @@ describe("login-flow", () => { }); }); - // ----------------------------------------------------------------------- - // requestDeviceCode failure - // ----------------------------------------------------------------------- - test("requestDeviceCode throws -> propagates error", async () => { mockRequestDeviceCode.mockImplementation(() => Promise.reject(new Error("GitHub device code request failed (HTTP 500)")) @@ -179,10 +167,6 @@ describe("login-flow", () => { expect(mockSaveCredentials).not.toHaveBeenCalled(); }); - // ----------------------------------------------------------------------- - // pollForAccessToken failure - // ----------------------------------------------------------------------- - test("pollForAccessToken throws -> propagates error", async () => { mockPollForAccessToken.mockImplementation(() => Promise.reject(new Error("Device code expired")) @@ -193,10 +177,6 @@ describe("login-flow", () => { expect(mockSaveCredentials).not.toHaveBeenCalled(); }); - // ----------------------------------------------------------------------- - // getGitHubUser failure - // ----------------------------------------------------------------------- - test("getGitHubUser throws -> propagates error", async () => { mockGetGitHubUser.mockImplementation(() => Promise.reject(new Error("Failed to fetch GitHub user (HTTP 401)")) @@ -262,10 +242,6 @@ describe("login-flow", () => { }); }); - // ----------------------------------------------------------------------- - // Signup flow: no token from signup, fallback to claimArchgateToken - // ----------------------------------------------------------------------- - test("signup without auto-token falls back to second claim call", async () => { // First claimArchgateToken call throws, second succeeds let claimCallCount = 0; @@ -318,10 +294,6 @@ describe("login-flow", () => { }); }); - // ----------------------------------------------------------------------- - // Signup cancelled by user (confirmed = false) - // ----------------------------------------------------------------------- - test("signup cancelled (confirmed=false) -> returns ok:false", async () => { mockClaimArchgateToken.mockImplementation(() => Promise.reject(new SignupRequiredError()) @@ -351,10 +323,6 @@ describe("login-flow", () => { expect(mockSaveCredentials).not.toHaveBeenCalled(); }); - // ----------------------------------------------------------------------- - // Signup request fails (API returns non-201) - // ----------------------------------------------------------------------- - test("signup request fails -> returns ok:false", async () => { mockClaimArchgateToken.mockImplementation(() => Promise.reject(new SignupRequiredError()) @@ -397,10 +365,6 @@ describe("login-flow", () => { expect(mockSaveCredentials).not.toHaveBeenCalled(); }); - // ----------------------------------------------------------------------- - // Pre-selected editor in options -> skips editor prompt - // ----------------------------------------------------------------------- - test("pre-selected editor skips editor prompt in signup flow", async () => { mockClaimArchgateToken.mockImplementation(() => Promise.reject(new SignupRequiredError()) @@ -440,10 +404,6 @@ describe("login-flow", () => { expect(promptCallCount).toBe(3); }); - // ----------------------------------------------------------------------- - // claimArchgateToken throws non-SignupRequired error -> propagates - // ----------------------------------------------------------------------- - test("claimArchgateToken throws non-signup error -> propagates", async () => { mockClaimArchgateToken.mockImplementation(() => Promise.reject(new Error("Token claim failed (HTTP 500)")) diff --git a/tests/helpers/opencode-settings.test.ts b/tests/helpers/opencode-settings.test.ts index 196ed92e..048e04cd 100644 --- a/tests/helpers/opencode-settings.test.ts +++ b/tests/helpers/opencode-settings.test.ts @@ -114,9 +114,7 @@ describe("configureOpencodeSettings", () => { await configureOpencodeSettings(); const content = await Bun.file(join(configDir, "opencode.json")).json(); - // Existing default_agent preserved expect(content.default_agent).toBe("my-custom-agent"); - // Existing model preserved expect(content.model).toBe("anthropic/claude-sonnet-4-5"); }); diff --git a/tests/helpers/pack-recommend.test.ts b/tests/helpers/pack-recommend.test.ts index 5b3dd57a..084cfe5d 100644 --- a/tests/helpers/pack-recommend.test.ts +++ b/tests/helpers/pack-recommend.test.ts @@ -310,7 +310,6 @@ describe("recommendPacks", () => { expect(recs[0].adrCount).toBe(2); expect(recs[0].matchedTags).toContain("language:typescript"); - // The function should have cleaned up the cloned directory expect(existsSync(fakeCloneDir)).toBe(false); }); @@ -341,7 +340,6 @@ describe("recommendPacks", () => { frameworks: [], }; - // Even with no valid packs, the function returns empty and cleans up const recs = await recommendPacks(stack); expect(recs).toHaveLength(0); expect(existsSync(fakeCloneDir)).toBe(false); diff --git a/tests/helpers/paths.test.ts b/tests/helpers/paths.test.ts index 18407a15..75412aeb 100644 --- a/tests/helpers/paths.test.ts +++ b/tests/helpers/paths.test.ts @@ -58,7 +58,6 @@ describe("findProjectRoot", () => { const parent = join(tempDir, "parent"); const child = join(parent, "child"); mkdirSync(child, { recursive: true }); - // Create bare .archgate/ with no adrs/ or lint/ mkdirSync(join(parent, ".archgate"), { recursive: true }); const result = findProjectRoot(child); diff --git a/tests/helpers/platform.test.ts b/tests/helpers/platform.test.ts index 56fcf159..7300e327 100644 --- a/tests/helpers/platform.test.ts +++ b/tests/helpers/platform.test.ts @@ -55,7 +55,6 @@ describe("getPlatformInfo", () => { const first = getPlatformInfo(); _resetAllCaches(); const second = getPlatformInfo(); - // Values should be the same even though references differ expect(second.runtime).toBe(first.runtime); expect(second.isWSL).toBe(first.isWSL); expect(second.wslDistro).toBe(first.wslDistro); @@ -274,7 +273,7 @@ describe("_resetAllCaches", () => { // Call once to populate the cache. const before = await getWindowsHomeDirFromWSL(); // Reset and re-detect — the platform hasn't changed, so the freshly - // detected value must match the previously cached one. + // detected value must match the cached one from the first call. _resetAllCaches(); const after = await getWindowsHomeDirFromWSL(); expect(after).toBe(before); diff --git a/tests/helpers/plugin-install-cleanup.test.ts b/tests/helpers/plugin-install-cleanup.test.ts index 3c6964ba..513e27de 100644 --- a/tests/helpers/plugin-install-cleanup.test.ts +++ b/tests/helpers/plugin-install-cleanup.test.ts @@ -19,9 +19,6 @@ import { } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; -// --------------------------------------------------------------------------- -// Imports under test -// --------------------------------------------------------------------------- import * as platform from "../../src/helpers/platform"; import { @@ -186,7 +183,6 @@ describe("plugin install — stale file cleanup", () => { const targetIdx = callArgs.indexOf("-C"); expect(targetIdx).toBeGreaterThanOrEqual(0); const targetDir = callArgs[targetIdx + 1]; - // Must end with /opencode (config dir), not /opencode/agents expect(targetDir).toMatch(/opencode$/u); expect(targetDir).not.toMatch(/agents$/u); }); diff --git a/tests/helpers/plugin-install.test.ts b/tests/helpers/plugin-install.test.ts index 54386a67..14442bb6 100644 --- a/tests/helpers/plugin-install.test.ts +++ b/tests/helpers/plugin-install.test.ts @@ -12,9 +12,6 @@ import { import { mkdirSync, mkdtempSync, rmSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; -// --------------------------------------------------------------------------- -// Imports under test -// --------------------------------------------------------------------------- import { opencodeConfigDir } from "../../src/helpers/paths"; import * as platform from "../../src/helpers/platform"; @@ -105,11 +102,10 @@ beforeEach(() => { spawnSpy = spyOn(Bun, "spawn").mockImplementation(() => fakeSpawnResult(0)); // Redirect user-scope paths into a temp dir. The install functions create - // directories AND delete stale archgate-* files under cursorUserDir() / - // opencodeConfigDir() / internalPath() before the (mocked) tar extraction — - // without this override they destroy the developer's real installed plugin - // files in ~/.cursor and ~/.config/opencode. All three resolvers read - // Bun.env.HOME / XDG_CONFIG_HOME at call time, so an env override works. + // directories and delete stale archgate files under cursorUserDir() / + // opencodeConfigDir() / internalPath(), so without this override they wipe + // the developer's real plugins in ~/.cursor and ~/.config/opencode. All + // three resolvers read HOME / XDG_CONFIG_HOME at call time. tempHome = mkdtempSync(join(tmpdir(), "archgate-plugin-install-")); savedHome = Bun.env.HOME; savedXdg = Bun.env.XDG_CONFIG_HOME; @@ -300,7 +296,6 @@ describe("plugin-install", () => { await installClaudePlugin(); - // Two spawn calls: marketplace add + plugin install expect(spawnSpy).toHaveBeenCalledTimes(2); const firstCall = spawnSpy.mock.calls[0][0] as string[]; expect(firstCall).toContain("marketplace"); @@ -379,10 +374,8 @@ describe("plugin-install", () => { spawnSpy.mockImplementation(() => { callCount++; if (callCount === 1) { - // marketplace add fails with "already registered" return fakeSpawnResult(1, "already registered", ""); } - // plugin install succeeds return fakeSpawnResult(0); }); @@ -418,7 +411,6 @@ describe("plugin-install", () => { await installVscodeExtension("test-token"); - // fetch was called for the download expect(spawnSpy).toHaveBeenCalledTimes(1); const callArgs = spawnSpy.mock.calls[0][0] as string[]; expect(callArgs).toContain("--install-extension"); @@ -465,7 +457,6 @@ describe("plugin-install", () => { await installOpencodePlugin("test-token"); - // One spawn call for tar extraction expect(spawnSpy).toHaveBeenCalledTimes(1); const callArgs = spawnSpy.mock.calls[0][0] as string[]; expect(callArgs[0]).toBe("tar"); @@ -510,7 +501,6 @@ describe("plugin-install", () => { await installCursorPlugin("test-token"); - // One spawn call for tar extraction expect(spawnSpy).toHaveBeenCalledTimes(1); const callArgs = spawnSpy.mock.calls[0][0] as string[]; expect(callArgs[0]).toBe("tar"); diff --git a/tests/helpers/project-config.test.ts b/tests/helpers/project-config.test.ts index 8ccfa2ca..cd96edc7 100644 --- a/tests/helpers/project-config.test.ts +++ b/tests/helpers/project-config.test.ts @@ -137,10 +137,6 @@ describe("project-config", () => { expect(loadProjectConfig(projectRoot).domains.infra).toBe("INFRA"); }); - // ------------------------------------------------------------------------- - // resolvedProjectPaths - // ------------------------------------------------------------------------- - describe("resolvedProjectPaths", () => { test("returns defaults when no paths config is set", () => { const paths = resolvedProjectPaths(projectRoot); @@ -190,7 +186,6 @@ describe("project-config", () => { }); test("ignores invalid config and falls back to defaults", async () => { - // Write a malformed config file const configPath = join(projectRoot, ".archgate", "config.json"); await Bun.write(configPath, "not valid json"); const paths = resolvedProjectPaths(projectRoot); diff --git a/tests/helpers/rules-shim.test.ts b/tests/helpers/rules-shim.test.ts index 0cc462a9..ef7aeb75 100644 --- a/tests/helpers/rules-shim.test.ts +++ b/tests/helpers/rules-shim.test.ts @@ -54,7 +54,6 @@ describe("rules-shim", () => { const dtsContent = await Bun.file(dtsPath).text(); expect(dtsContent).toContain("declare interface RuleContext"); - // Should NOT create rules.js expect(existsSync(join(tempDir, ".archgate", "rules.js"))).toBe(false); }); @@ -73,7 +72,6 @@ describe("rules-shim", () => { const dtsPath = join(tempDir, ".archgate", "rules.d.ts"); - // Overwrite with stale content await Bun.write(dtsPath, "stale"); await ensureRulesShim(tempDir); diff --git a/tests/helpers/session-context-copilot.test.ts b/tests/helpers/session-context-copilot.test.ts index aa4679f9..5376d0be 100644 --- a/tests/helpers/session-context-copilot.test.ts +++ b/tests/helpers/session-context-copilot.test.ts @@ -11,8 +11,6 @@ import { } from "../../src/helpers/session-context-copilot"; import { restoreEnv } from "../test-utils"; -// This file covers readCopilotSession happy-path and error-case tests. - describe("readCopilotSession", () => { const uniqueId = `test-${Date.now()}-${Math.random().toString(36).slice(2, 8)}`; // Use a fake project root that we'll put in workspace.yaml cwd @@ -51,7 +49,6 @@ describe("readCopilotSession", () => { const yaml = `cwd: ${JSON.stringify(cwd)}\nid: ${JSON.stringify(sessionId)}\n`; writeFileSync(join(sessionDir, "workspace.yaml"), yaml); - // Write events.jsonl if provided if (events) { writeFileSync(join(sessionDir, "events.jsonl"), events.join("\n")); } @@ -261,7 +258,6 @@ describe("readCopilotSession", () => { const past = new Date(Date.now() - 60_000); utimesSync(join(stateDir, earlierId), past, past); - // Create the current session (newer) const currentId = `copilot-${uniqueId}-current`; makeSession(currentId, projectRoot, [ JSON.stringify({ diff --git a/tests/helpers/session-context-cursor.test.ts b/tests/helpers/session-context-cursor.test.ts index 9cb661a3..7faaa443 100644 --- a/tests/helpers/session-context-cursor.test.ts +++ b/tests/helpers/session-context-cursor.test.ts @@ -205,7 +205,6 @@ describe("readCursorSession", () => { }); test("sessionId reads an earlier session; default reads the most recent", async () => { - // Create an earlier session (make it older) makeSession("session-earlier", [ JSON.stringify({ role: "user", @@ -222,7 +221,6 @@ describe("readCursorSession", () => { const past = new Date(Date.now() - 60_000); utimesSync(join(transcriptsDir, "session-earlier"), past, past); - // Create the current session (newer) makeSession("session-current", [ JSON.stringify({ role: "user", @@ -292,7 +290,6 @@ describe("readCursorSession", () => { }); test("ignores non-directory entries in transcripts dir", async () => { - // Put a plain file in the transcripts dir — it should be skipped writeFileSync(join(transcriptsDir, "stray-file.txt"), "noise"); makeSession("session-good", [ JSON.stringify({ diff --git a/tests/helpers/session-context-opencode.test.ts b/tests/helpers/session-context-opencode.test.ts index e5ace4de..9cc11aad 100644 --- a/tests/helpers/session-context-opencode.test.ts +++ b/tests/helpers/session-context-opencode.test.ts @@ -12,10 +12,6 @@ import { } from "../../src/helpers/session-context-opencode"; import { restoreEnv } from "../test-utils"; -/** - * Tests for readOpencodeSession — reads session data from - * opencode's SQLite database. - */ describe("readOpencodeSession", () => { const uniqueId = `test-${Date.now()}-${Math.random().toString(36).slice(2, 8)}`; const projectRoot = resolve(`/__archgate_opencode_test_${uniqueId}`); @@ -46,7 +42,6 @@ describe("readOpencodeSession", () => { } }); - /** Create the opencode database schema. */ function createDb(): Database { const db = new Database(dbPath); // Use DELETE journal mode to avoid WAL/SHM files that lock on Windows @@ -238,7 +233,6 @@ describe("readOpencodeSession", () => { const db = createDb(); const sessionId = `ses_${uniqueId}_roles`; - // Insert messages with various roles — system and tool should be filtered out const now = Date.now(); makeSession(db, sessionId, projectRoot, undefined, now); @@ -284,7 +278,6 @@ describe("readOpencodeSession", () => { test("returns error when session has no messages", async () => { const db = createDb(); const sessionId = `ses_${uniqueId}_nomsg`; - // Create session but no messages makeSession(db, sessionId, projectRoot); db.close(); @@ -339,7 +332,6 @@ describe("readOpencodeSession", () => { }); test("returns error when database does not exist", async () => { - // Point to a non-existent directory Bun.env.XDG_DATA_HOME = join(tempDir, "nonexistent"); const result = await readOpencodeSession(projectRoot); @@ -375,7 +367,6 @@ describe("readOpencodeSession", () => { expect(result.data.transcript[0]?.contentPreview).toBe( "actual user question" ); - // The synthetic part should not appear expect(result.data.transcript[0]?.contentPreview).not.toContain( "system-reminder" ); @@ -451,12 +442,11 @@ describe("readOpencodeSession", () => { }); test("selects the true parent when sibling sub-agents fan out and are more recent", async () => { - // Real-world fan-out reproduced from a live incident: one parent session - // spawns several sibling sub-agent sessions against the same directory - // (e.g. the reviewer skill's parallel domain reviews). Every sibling - // sorts ahead of the parent by recency; none of them may shadow it. - // The old recency-based `--skip 1` landed on whichever sibling sat - // second in recency order instead of the parent. + // Real-world fan-out: one parent session spawns several sibling sub-agent + // sessions against the same directory (e.g. the reviewer skill's parallel + // domain reviews). Every sibling sorts ahead of the parent by recency, so + // a recency-based pick lands on a sibling instead; selection must resolve + // the parent no matter how many siblings outrank it. const db = createDb(); makeSimpleSession(db, "ses_parent", "parent development session", 1000); makeSimpleSession(db, "ses_sib_a", "domain review a", 2000, "ses_parent"); diff --git a/tests/helpers/session-context.test.ts b/tests/helpers/session-context.test.ts index 1ae28a38..5f522654 100644 --- a/tests/helpers/session-context.test.ts +++ b/tests/helpers/session-context.test.ts @@ -277,7 +277,6 @@ describe("readClaudeCodeSession", () => { ].join("\n") ); - // Backdate the earlier session's mtime const { utimesSync } = await import("node:fs"); const past = new Date(Date.now() - 60_000); utimesSync(olderFile, past, past); diff --git a/tests/helpers/stack-detect-frameworks.test.ts b/tests/helpers/stack-detect-frameworks.test.ts index 39b83fb2..1281f000 100644 --- a/tests/helpers/stack-detect-frameworks.test.ts +++ b/tests/helpers/stack-detect-frameworks.test.ts @@ -2,8 +2,8 @@ // Copyright 2026 Archgate // --------------------------------------------------------------------------- -// Framework detection and caching tests — split from stack-detect.test.ts to -// stay under the 500-line lint limit. +// Framework detection and caching tests. They live apart from +// stack-detect.test.ts so each file stays under the 500-line lint limit. // --------------------------------------------------------------------------- import { describe, expect, test, afterEach } from "bun:test"; diff --git a/tests/helpers/telemetry-config.test.ts b/tests/helpers/telemetry-config.test.ts index fcb67061..9916c5c2 100644 --- a/tests/helpers/telemetry-config.test.ts +++ b/tests/helpers/telemetry-config.test.ts @@ -22,8 +22,8 @@ describe("telemetry-config", () => { afterEach(async () => { // `env.X = undefined` assigns the string "undefined" rather than unsetting, - // so HOME (normally unset on Windows) leaked into every later test file. - // Bun.env and process.env are the same store, so restoreEnv covers both. + // which would leak HOME (normally unset on Windows) into every later test + // file. Bun.env and process.env are the same store, so restoreEnv covers both. restoreEnv("HOME", originalHome); restoreEnv("ARCHGATE_TELEMETRY", originalTelemetryEnv); rmSync(tempDir, { recursive: true, force: true }); diff --git a/tests/helpers/telemetry.test.ts b/tests/helpers/telemetry.test.ts index 524764c0..8255a033 100644 --- a/tests/helpers/telemetry.test.ts +++ b/tests/helpers/telemetry.test.ts @@ -27,8 +27,8 @@ describe("telemetry", () => { afterEach(async () => { // `env.X = undefined` assigns the string "undefined" rather than unsetting, - // so HOME (normally unset on Windows) leaked into every later test file. - // Bun.env and process.env are the same store, so restoreEnv covers both. + // which would leak HOME (normally unset on Windows) into every later test + // file. Bun.env and process.env are the same store, so restoreEnv covers both. restoreEnv("HOME", originalHome); restoreEnv("ARCHGATE_TELEMETRY", originalTelemetryEnv); restoreEnv("NODE_ENV", originalNodeEnv); @@ -67,7 +67,6 @@ describe("telemetry", () => { await initTelemetry(); expect(_getClient()).not.toBeNull(); - // Second init overwrites state — should not throw await initTelemetry(); expect(_getClient()).not.toBeNull(); }); diff --git a/tests/helpers/update-check.test.ts b/tests/helpers/update-check.test.ts index 60be27c9..a9aa4fe3 100644 --- a/tests/helpers/update-check.test.ts +++ b/tests/helpers/update-check.test.ts @@ -158,7 +158,6 @@ describe("checkForUpdatesIfNeeded", () => { }); test("skips check when cache is recent", async () => { - // Write a fresh cache timestamp const cacheDir = join(tempDir, ".archgate"); await Bun.write(join(cacheDir, "last-update-check"), String(Date.now())); @@ -194,17 +193,14 @@ describe("checkForUpdatesIfNeeded", () => { expect(result).toContain("0.2.0"); expect(existsSync(cacheFile)).toBe(true); - // Cache file should contain a numeric timestamp const content = await Bun.file(cacheFile).text(); const timestamp = Math.trunc(Number(content.trim())); expect(isNaN(timestamp)).toBe(false); - // Timestamp should be within the last 5 seconds expect(Date.now() - timestamp).toBeLessThan(5_000); }); test("rewrites cache file when cache is stale", async () => { const cacheFile = join(tempDir, ".archgate", "last-update-check"); - // Write a stale timestamp (25 hours ago) const staleTimestamp = Date.now() - 25 * 60 * 60 * 1000; await Bun.write(cacheFile, String(staleTimestamp)); @@ -224,7 +220,6 @@ describe("checkForUpdatesIfNeeded", () => { expect(result).toContain("0.3.0"); expect(mockFetch).toHaveBeenCalled(); - // Cache file should have been rewritten with a fresh timestamp const content = await Bun.file(cacheFile).text(); const newTimestamp = Math.trunc(Number(content.trim())); expect(newTimestamp).toBeGreaterThan(staleTimestamp); @@ -244,13 +239,11 @@ describe("checkForUpdatesIfNeeded", () => { `../../src/helpers/update-check?t=${Date.now()}` ); - // Pass a version string that semver cannot parse const result = await checkForUpdatesIfNeeded("not-a-version"); expect(result).toBeNull(); }); test("returns null when an error is thrown during execution", async () => { - // Simulate a disk write failure by making Bun.write throw Bun.write = (() => { throw new Error("simulated disk write failure"); }) as unknown as typeof Bun.write; diff --git a/tests/helpers/vscode-settings.test.ts b/tests/helpers/vscode-settings.test.ts index d59b5b67..d0917577 100644 --- a/tests/helpers/vscode-settings.test.ts +++ b/tests/helpers/vscode-settings.test.ts @@ -22,7 +22,7 @@ import { restoreEnv } from "../test-utils"; /** * Bulk form of `restoreEnv` for the snapshot-many-vars pattern used below: - * restores saved env vars, deleting keys that were originally unset. + * restores saved env vars, deleting keys that were unset at capture time. * * Neither `Object.assign(process.env, saved)` nor a plain `env[k] = v` loop * works — both stringify `undefined` into the literal "undefined", corrupting @@ -128,7 +128,6 @@ describe("configureVscodeSettings", () => { test("does not create user settings file when no marketplace URL is provided", async () => { await configureVscodeSettings(tempDir); - // The user settings file should not be created const path = await getVscodeUserSettingsPath(); expect(existsSync(path)).toBe(false); }); @@ -238,7 +237,6 @@ describe("addMarketplaceToUserSettings", () => { process.env.APPDATA = deepHome; // Windows homedirSpy.mockReturnValue(deepHome); // macOS/Linux - // addMarketplaceToUserSettings should create the entire directory tree await addMarketplaceToUserSettings(URL); const path = await settingsPath(); @@ -337,7 +335,6 @@ describe("getVscodeUserSettingsPath", () => { delete process.env.APPDATA; const path = await getVscodeUserSettingsPath(); const normalized = path.replaceAll("\\", "/"); - // Should fall back to homedir()/AppData/Roaming expect(normalized).toContain("AppData/Roaming/Code/User/settings.json"); } finally { restoreEnv("APPDATA", savedAppData); diff --git a/tests/integration/adr.test.ts b/tests/integration/adr.test.ts index fe73274e..90289d00 100644 --- a/tests/integration/adr.test.ts +++ b/tests/integration/adr.test.ts @@ -24,8 +24,6 @@ afterEach(() => { }); describe("adr integration", () => { - // adr create - describe("adr create", () => { test("creates ADR file", async () => { scaffoldProject(tempDir); @@ -147,8 +145,6 @@ describe("adr integration", () => { }); }); - // adr list - describe("adr list", () => { test("lists ADRs", async () => { scaffoldProject(tempDir); diff --git a/tests/integration/check.test.ts b/tests/integration/check.test.ts index 9a91df77..ca891af5 100644 --- a/tests/integration/check.test.ts +++ b/tests/integration/check.test.ts @@ -370,7 +370,6 @@ describe("check --base integration", () => { };` ); - // Commit everything on main await git(["add", "."], dir); await git(["commit", "-m", "initial"], dir); diff --git a/tests/integration/clean.test.ts b/tests/integration/clean.test.ts index bb5ee319..d1df4539 100644 --- a/tests/integration/clean.test.ts +++ b/tests/integration/clean.test.ts @@ -39,7 +39,6 @@ describe("clean integration", () => { const archgateDir = join(fakeHome, ".archgate"); seedUpdateCache(archgateDir); - // First clean removes the directory await runCli(["clean"], dir, { HOME: fakeHome, USERPROFILE: fakeHome }); // CLI startup re-creates ~/.archgate/cache; seed it again to prevent update-check writes seedUpdateCache(archgateDir); diff --git a/tests/integration/cli-harness.ts b/tests/integration/cli-harness.ts index 2035b9e8..eac228e5 100644 --- a/tests/integration/cli-harness.ts +++ b/tests/integration/cli-harness.ts @@ -18,10 +18,6 @@ export interface RunResult { stderr: string; } -/** - * Run a CLI command in the given project directory. - * Returns captured stdout, stderr, and exit code. - */ export async function runCli( args: string[], cwd: string, @@ -53,25 +49,15 @@ export async function runCli( return { exitCode, stdout: stdout.trim(), stderr: stderr.trim() }; } -/** - * Create an isolated temp project directory with optional .archgate scaffold. - */ export function createTempProject(prefix = "archgate-integ-"): string { return mkdtempSync(join(tmpdir(), prefix)); } -/** - * Initialize a minimal .archgate project in the given directory. - * Includes the adrs/ and lint/ directories. - */ export function scaffoldProject(dir: string): void { mkdirSync(join(dir, ".archgate", "adrs"), { recursive: true }); mkdirSync(join(dir, ".archgate", "lint"), { recursive: true }); } -/** - * Write an ADR markdown file to the project's adrs directory. - */ export function writeAdr(dir: string, filename: string, content: string): void { writeFileSync(join(dir, ".archgate", "adrs", filename), content); } @@ -96,9 +82,6 @@ export function writeRules( writeFileSync(join(dir, ".archgate", "adrs", filename), wrapped); } -/** - * Build a minimal ADR markdown string. - */ export function makeAdr(opts: { id: string; title: string; diff --git a/tests/integration/cli-perf.test.ts b/tests/integration/cli-perf.test.ts index 65506660..1b5ae85c 100644 --- a/tests/integration/cli-perf.test.ts +++ b/tests/integration/cli-perf.test.ts @@ -3,21 +3,10 @@ /** * Performance regression tests for CLI startup and exit latency. * - * Two concerns, two describe blocks: - * - * 1. **Exit tail guard** — catches leaked `setTimeout` / `Bun.sleep` that - * keep the event loop alive after the command completes (the ~3s tail - * from PR #213). Budget: 4000ms — generous, only fires on timer leaks. - * - * 2. **Startup latency budget** — catches import-time regressions like - * static `import inquirer` (costs ~200ms) or blocking telemetry init - * (~150ms). Budgets are ~3-4x the measured baseline so they don't - * flake on slow CI, but tight enough to catch a heavy dependency - * being pulled into the startup path. - * - * Strategy: run commands end-to-end via `Bun.spawn`, take the median of - * multiple runs to smooth out cold-start variance, and assert wall-clock - * time stays under the budget. + * One describe block guards the exit tail against leaked timers holding the + * event loop open; the other guards startup against import-time cost such as + * a static `inquirer` import. Commands run end-to-end via `Bun.spawn`, taking + * the median of several runs against a budget set well above baseline. */ import { describe, expect, test } from "bun:test"; @@ -26,20 +15,23 @@ import { resolve } from "node:path"; const CLI_PATH = resolve(import.meta.dir, "..", "..", "src", "cli.ts"); /** - * Ceiling for a trivially-fast command. The historical regression - * pushed this to 3.5–4s. Normal runs sit well under 2s even on slow - * CI. 4000ms catches the regression with plenty of headroom. + * Ceiling for a trivially-fast command. Normal runs sit well under 2s even on + * slow CI, while a leaked exit-path timer pushes them to 3.5-4s, so 4000ms + * separates the two with headroom. + * + * @see https://github.com/archgate/cli/pull/213 — the leak this budget guards */ const FAST_COMMAND_MAX_MS = 4000; /** * Run the CLI with the given args and return the wall-clock duration. - * `NODE_ENV=test` suppresses actual telemetry event capture so no real - * traffic is sent, but the telemetry / Sentry SDKs still initialize - * and flush — which is exactly the path the timer-leak regression - * lived on. Leaving `ARCHGATE_TELEMETRY` unset means we exercise the - * enabled path; the `_=test` env guard inside `trackEvent` / - * `Sentry.init`'s `enabled` flag prevents real event delivery. + * + * `NODE_ENV=test` suppresses event capture, so no real traffic is sent while + * the telemetry and Sentry SDKs still initialize and flush — the path a timer + * leak lives on. `ARCHGATE_TELEMETRY` stays unset to exercise the enabled path. + * + * @param args - Arguments passed to the CLI after the script path. + * @returns Wall-clock milliseconds from spawn to process exit. */ async function timeCli(args: string[]): Promise { const start = performance.now(); @@ -125,22 +117,10 @@ describe("CLI performance — exit tail regression guard", () => { // Startup latency budgets // --------------------------------------------------------------------------- // -// These budgets are tighter than the exit-tail guard above. They protect -// against import-time regressions: -// -// - Re-adding a static `import inquirer` (costs ~200ms) -// - Blocking on telemetry/sentry init before command parsing (~150ms) -// - Pulling a heavy new dependency into the top-level import chain -// -// Baseline (measured 2026-05-09 on Windows, subprocess via Bun.spawn): -// --help: ~260ms --version: ~250ms -// adr list: ~400ms check: ~750ms -// -// Budgets are set at ~3-4x the baseline to absorb CI variance (GitHub -// Actions Windows runners are typically 1.5-2x slower than local dev) -// without masking a real regression. If a budget fires, profile the -// startup with `bun -e "..."` import-time measurements (see the commit -// that introduced these tests for the technique). +// Tighter than the exit-tail guard above, these protect against import-time +// cost: a static `import inquirer` (~200ms), blocking telemetry init (~150ms), +// or a heavy dependency entering the top-level import chain. Each sits at +// ~3-4x its measured baseline so CI variance cannot mask a real regression. /** * Budget for commands that do zero project I/O — pure startup + parse + diff --git a/tests/integration/import.test.ts b/tests/integration/import.test.ts index 8a53d2ae..c72b94cb 100644 --- a/tests/integration/import.test.ts +++ b/tests/integration/import.test.ts @@ -81,23 +81,20 @@ describe("import integration (local fixtures)", () => { scaffoldProject(); process.chdir(tempDir); - // We need to bypass git clone for local testing. - // Simulate by directly calling the import action with local paths. - // Instead, let's use the detectTarget + manual write approach. + // Bypass git clone for local testing: resolve the fixture with + // detectTarget and write the files directly. const target = await detectTarget(FIXTURE_REGISTRY, "packs/test-pack"); expect(target.kind).toBe("pack"); if (target.kind === "pack") { const adrsDir = join(tempDir, ".archgate", "adrs"); - // Write ADR files with remapped IDs for (const adrFile of target.adrFiles) { const content = readFileSync(adrFile, "utf-8"); const filename = adrFile.split(/[\\/]/u).pop()!; writeFileSync(join(adrsDir, filename), content); } - // Write rules files for (const rulesFile of target.rulesFiles) { const content = readFileSync(rulesFile, "utf-8"); const filename = rulesFile.split(/[\\/]/u).pop()!; @@ -163,11 +160,9 @@ describe("import integration (local fixtures)", () => { scaffoldProject(); process.chdir(tempDir); - // Create a program that uses dry-run const parent = new Command("adr").exitOverride(); registerAdrImportCommand(parent); - // With --list and no imports, should succeed await parent.parseAsync([ "node", "adr", @@ -181,7 +176,6 @@ describe("import integration (local fixtures)", () => { .join("\n"); expect(allOutput).toContain("No ADRs have been imported yet."); - // No files should have been written to adrs dir const adrsDir = join(tempDir, ".archgate", "adrs"); const files = readdirSync(adrsDir); expect(files.length).toBe(0); diff --git a/tests/integration/review-context.test.ts b/tests/integration/review-context.test.ts index 0e9b089b..039770cb 100644 --- a/tests/integration/review-context.test.ts +++ b/tests/integration/review-context.test.ts @@ -246,7 +246,6 @@ describe("review-context integration", () => { writeFileSync(join(dir, "src", "base.ts"), "export const x = 1;\n"); await commitAll(dir, "initial commit"); - // Create feature branch and add a file await git(["checkout", "-b", "feature"], dir); writeFileSync(join(dir, "src", "new-feature.ts"), "export const y = 2;\n"); await commitAll(dir, "add feature"); @@ -266,9 +265,10 @@ describe("review-context integration", () => { expect(ctx.allChangedFiles).not.toContain("src/base.ts"); }, 30_000); - // Regression: archgate/cli#403 — with a base ref detected, review-context - // only listed committed branch changes and silently omitted uncommitted - // working-tree edits (the files actually under review in an agent session). + // Regression guard for archgate/cli#403: with a base ref detected, + // review-context must report uncommitted working-tree edits (the files + // actually under review in an agent session), not just committed branch + // changes. test("--base includes uncommitted working-tree changes (regression archgate/cli#403)", async () => { scaffoldProject(dir); writeAdr( @@ -337,11 +337,9 @@ describe("review-context integration", () => { writeFileSync(join(dir, "src", "committed.ts"), "export const c = 3;\n"); await commitAll(dir, "committed change"); - // Stage a different file (not committed yet) writeFileSync(join(dir, "src", "staged.ts"), "export const s = 4;\n"); await git(["add", "src/staged.ts"], dir); - // --staged should only show staged.ts, not committed.ts const { exitCode, stdout } = await runCli( ["review-context", "--staged"], dir, diff --git a/tests/test-utils.ts b/tests/test-utils.ts index 02a3c342..904647bc 100644 --- a/tests/test-utils.ts +++ b/tests/test-utils.ts @@ -28,16 +28,16 @@ export async function git(args: string[], cwd: string): Promise { } /** - * Restore an environment variable to a previously captured value, deleting it - * when that value was `undefined`. + * Restore an environment variable to a captured value, deleting the key when + * that value was `undefined`. A bare `env.X = original` assignment stores the + * literal string `"undefined"` instead of unsetting, and `Bun.env` is + * process-global across test files, so that leak reaches every later test and + * any subprocess inheriting the environment. * - * `Bun.env.X = undefined` assigns the literal string `"undefined"` rather than - * unsetting the key, so the common - * `const orig = Bun.env.X; ... Bun.env.X = orig` idiom silently leaks - * `X="undefined"` whenever the variable was unset to begin with — which is the - * normal case on Windows for HOME and GIT_CONFIG_GLOBAL. `Bun.env` is - * process-global and Bun shares one process across test files, so such a leak - * escapes into every later test, including subprocesses that inherit the env. + * @param key - Variable name. `Bun.env` and `process.env` are one store, so + * the capture works through either accessor. + * @param original - Captured value, or `undefined` when the key was unset. + * @see ARCH-005 for the rationale and the `no-bare-env-restore` lint rule. */ export function restoreEnv(key: string, original: string | undefined): void { if (original === undefined) delete Bun.env[key];