diff --git a/.archgate/PRDs/ast-aware-rule-context.md b/.archgate/PRDs/ast-aware-rule-context.md new file mode 100644 index 00000000..4c0426aa --- /dev/null +++ b/.archgate/PRDs/ast-aware-rule-context.md @@ -0,0 +1,75 @@ +# PRD: AST-Aware Rule Context + +**Status:** Draft +**Related ADR:** [ARCH-022 — AST-Aware Rule Context](../adrs/ARCH-022-ast-aware-rule-context.md) + +This document covers the _what_ and _for whom_ of exposing AST inspection to `.rules.ts` authors. Architectural decisions — dependency choices, sandboxing, subprocess execution, error semantics — are governed by ARCH-022 and are not restated here. Where this PRD and the ADR appear to overlap, the ADR is authoritative for "how it must be built"; this document is authoritative for "what it must do for users." + +## Problem + +Rule authors writing `.rules.ts` files that need to check code _structure_ (not just text patterns) have no good option today. They fall back to line-based heuristics and regex over raw source, which are fragile against formatting variance, multi-line statements, and string-escaping edge cases. This is visible in the project's own ADR rules (`ARCH-004`'s barrel-file heuristic, `ARCH-008`'s option-shape regex checks) and would presumably affect any user writing similar structural rules for their own project. + +There is no path today for a rule author to write a structural check against Python or Ruby source at all — `RuleContext` has no language-aware capability beyond generic text search. + +## Goals + +- Let a `.rules.ts` author write a structural (AST-based) check against a TypeScript or JavaScript file using a single, discoverable `RuleContext` method. +- Extend that same method to Python and Ruby source files, using each language's own standard-library AST facility, gated on that interpreter being available on the machine running `archgate check`. +- Make the capability discoverable and usable without requiring the rule author to understand the internal dispatch mechanism (subprocess vs. in-process parser) — that is an implementation detail per ARCH-022. +- Make failure states (missing interpreter, unparseable file) visible and actionable to whoever is running `archgate check`, not silently swallowed into a false pass. + +## Non-Goals + +- **A common AST vocabulary across languages.** This PRD does not require that a rule written against Python's AST "look like" one written against TypeScript's AST. Per ARCH-022, `ctx.ast()` unifies the call site and failure contract, not the returned node shape. A rule author targeting multiple languages is expected to know each target language's native AST grammar. +- **Bundled/native language parsers** (tree-sitter, WASM grammars, or similar). Out of scope for this PRD's v1; see ARCH-022's "Exceptions" section for the process to propose this later if Python/Ruby coverage via system interpreters proves insufficient. +- **Guaranteeing Python/Ruby support works on every machine.** A user running `archgate check` without `python3`/`ruby` installed cannot use a Python/Ruby structural rule — the product requirement is that this fails clearly (see "Failure Behavior" below), not that it works everywhere unconditionally. +- **Editor/IDE integration, autocomplete for AST node types, or a rule-authoring DX layer beyond documentation.** Future work, not v1. + +## Users and Use Cases + +Primary user: a developer or team writing `.rules.ts` files to enforce project-specific conventions via `archgate check` — the same audience already writing ADR rules today, extended to teams whose codebase includes Python or Ruby alongside (or instead of) TypeScript/JavaScript. + +Representative use cases: + +1. A TypeScript-only team rewrites an existing fragile regex-based rule (e.g., a barrel-file or call-shape check) to use `ctx.ast()` instead, for correctness rather than new capability. +2. A team with a Python backend wants an ADR rule enforcing a convention only expressible structurally (e.g., "no bare `except:` clauses," "all Django views subclass a specific base class") — something currently impossible to check reliably via `grep`/`grepFiles`. +3. A team with a Ruby codebase wants an equivalent structural check for a Ruby-specific convention. + +## Requirements + +### Functional + +- `RuleContext` MUST expose the AST capability as a single method taking a file path and a language identifier, returning the parsed tree or throwing (see ARCH-022 for the exact signature and throw semantics). +- Supported languages overall: TypeScript, JavaScript, Python, Ruby. TypeScript/JavaScript ship in v1; Python/Ruby follow in a later release per the rollout sequencing below. +- The method MUST work against any file within the rule's `scopedFiles`/`changedFiles`, subject to the same sandboxing already applied to `readFile`/`glob`. + +### Rollout Sequencing + +TypeScript/JavaScript support ships first — it requires no new runtime dependency (reuses the existing in-process parser) and directly replaces two existing fragile rules (`ARCH-004`, `ARCH-008`) as a validating first use case, per ARCH-022. Python and Ruby support ship as a follow-on once TS/JS usage has validated the API shape and failure-reporting UX; they should not block on each other and can land independently of one another. + +### Failure Behavior (product-facing) + +- If the interpreter required for a requested language is unavailable, or the target file fails to parse, the rule invoking `ctx.ast()` MUST surface as a distinct, visible failure category in `archgate check` output — not as "0 violations" and not as a crash of the entire `check` run. (The mechanism for this is defined in ARCH-022; the product requirement is only that the user-visible outcome is "clearly told something is wrong," not silence.) +- The failure message shown to the user MUST make it possible to distinguish "this rule found a violation," "this rule's target file doesn't parse," and "this rule can't run at all because a required interpreter is missing" — a user debugging a red `check` run should not have to read the rule's source to tell these apart. + +### Documentation Requirements + +- The CLI's rule-authoring documentation MUST document, per supported language, which AST facility backs it (meriyah/ESTree for TS/JS, the standard `ast` module for Python, `Ripper` for Ruby) and link to that facility's own reference documentation, since this PRD and ARCH-022 both explicitly decline to normalize the shapes. +- Documentation MUST state plainly, near the `ctx.ast()` reference, that Python/Ruby rules require the corresponding interpreter on PATH wherever `archgate check` runs (local machines and CI), since this is a real environmental requirement introduced by this feature and not something the tool works around. +- At least one example rule per supported language SHOULD ship in documentation or the rule-authoring skill, since the lack of a shared AST vocabulary means an example in one language does not transfer to another. + +## Success Criteria + +- `ARCH-004`'s barrel-file rule and `ARCH-008`'s option-shape rules are rewritten to use `ctx.ast()` and pass the project's own test suite, demonstrating the TS/JS path in production use within this repository. +- At least one Python or Ruby structural rule can be authored and run successfully against a real target file, with a deliberately-broken environment (interpreter removed from PATH) producing a clear, distinguishable failure rather than a silent pass. + +## Open Questions + +- Should the documentation-required example rules (per language) live in this repository's own `.archgate/adrs/` as dogfooding, in the public docs site, or both? +- What is the minimum interpreter version this PRD should claim support for (Python 3.x floor, Ruby version floor)? Needs a decision before the Python/Ruby documentation ships, not before TS/JS ships. + +## References + +- [ARCH-022 — AST-Aware Rule Context](../adrs/ARCH-022-ast-aware-rule-context.md) — architectural decision record for the mechanism described here +- [ARCH-004 — No Barrel Files or Re-Exports](../adrs/ARCH-004-no-barrel-files.md) — first intended consumer of `ctx.ast()` +- [ARCH-008 — Typed Command Options](../adrs/ARCH-008-typed-command-options.md) — second intended consumer of `ctx.ast()` diff --git a/.archgate/adrs/ARCH-022-ast-aware-rule-context.md b/.archgate/adrs/ARCH-022-ast-aware-rule-context.md new file mode 100644 index 00000000..593a4716 --- /dev/null +++ b/.archgate/adrs/ARCH-022-ast-aware-rule-context.md @@ -0,0 +1,128 @@ +--- +id: ARCH-022 +title: AST-Aware Rule Context +domain: architecture +rules: false +--- + +## Context + +`RuleContext` (`src/formats/rules.ts`, mirrored in `src/helpers/rules-shim.ts` for `.rules.ts` authors) is the only interface a `.rules.ts` file has to inspect a target project. Today it exposes exclusively text/regex/glob primitives: `glob`, `grep`, `grepFiles`, `readFile`, `readJSON`, and `report`. There is no structural, syntax-aware inspection capability. + +This is a real limitation, visible in the project's own rules. `ARCH-004/no-barrel-files` implements `isBarrelFile()` as a line-stripping heuristic — it strips comments and pattern-matches each remaining line to guess whether a file "only re-exports," rather than checking whether the file's top-level statements are actually `ExportNamedDeclaration`/`ExportAllDeclaration` nodes. `ARCH-008`'s option-shape rules (`use-add-option-for-choices`, `use-add-option-for-arg-parser`) regex-match `.option(...)` call text to detect a specific three-argument call shape. Both are exactly the class of check that is fragile with regex — multi-line calls, incidental whitespace, string escaping inside arguments — and would be direct and robust with a parsed AST (e.g., "does this file's only top-level statements have type `ExportNamedDeclaration`?" or "does this `CallExpression` targeting `.option()` have a third argument of type `ArrowFunctionExpression`?"). + +The codebase already parses an AST, but only defensively. `src/engine/rule-scanner.ts` uses `meriyah` (`parseModule`, currently a `devDependency`) to sandbox `.rules.ts` source files themselves before they execute: `Bun.Transpiler({ loader: "ts" }).transformSync(source)` strips TypeScript syntax, then `parseModule()` produces an ESTree-shaped tree that `scanRuleSource()`/`scanImportedRuleSource()` walk to block banned imports (`BANNED_MODULES`), dangerous `Bun.*` property access (`BLOCKED_BUN_PROPS = spawn, spawnSync, write, $, file`), `eval`/`Function`, non-literal dynamic `import()`, and `globalThis`/`process.env` mutation. This capability is private to `rule-scanner.ts` — it has no exported "parse this source" primitive, the `parseModule()` call is duplicated inline across both scanning functions, and none of it is reachable from `RuleContext`. + +**Alternatives considered for adding multi-language structural inspection:** + +- **Per-language native tree-sitter bindings** (`tree-sitter` + `tree-sitter-python`, `tree-sitter-ruby`, etc.) — Gives a single, uniform node interface (`{type, children, text, startPosition}`) across every supported language, which is genuinely attractive. Rejected because these are native Node addons distributed as prebuilt binaries per OS/architecture. This is precisely the supply-chain and install-size profile [ARCH-006](./ARCH-006-dependency-policy.md) exists to prevent, multiplied by one binary matrix per supported language, and it does not fit the CLI's single-file `bun build --compile` distribution model. +- **WASM tree-sitter grammars** (`web-tree-sitter` + a `.wasm` grammar per language) — Avoids the native-binary-per-platform problem since Bun has built-in `WebAssembly` support and a `.wasm` grammar is a portable data file, not a platform artifact. Deferred rather than rejected outright: it still adds a new production dependency and one or more multi-megabyte bundled assets requiring ARCH-006 review, and it is unverified whether `bun build --compile` can embed and load a `.wasm` grammar from the compiled binary rather than the filesystem. If Python/Ruby usage under this ADR's approach proves insufficient, this is the most likely next escalation and should be evaluated in a follow-up ADR once the two open questions above are answered. +- **Shelling out to the target project's own linter** (invoke `pylint`/`rubocop` and parse their JSON output) — Rejected as the general mechanism: it depends on the _target_ project having that tooling installed and configured, which cannot be assumed, and it couples `RuleContext` to third-party CLI output formats rather than to a language's own AST representation. +- **Do nothing; keep structural checks as regex heuristics** — Rejected because it does not scale past superficial patterns (see the ARCH-004/ARCH-008 examples above) and blocks any Python/Ruby structural check entirely, since regex-over-text has no notion of syntax at all for those languages. + +For Archgate specifically, the CLI already ships as a single compiled binary with a small, deliberately vetted dependency tree ([ARCH-006](./ARCH-006-dependency-policy.md)) and already has a working, in-process AST parser for TypeScript/JavaScript sitting unused outside of `rule-scanner.ts`. The lowest-cost path that adds real capability without expanding the dependency tree is to expose that existing parser through `RuleContext`, and to reach Python/Ruby by invoking each language's own standard-library AST facility as a subprocess — capability that ships with the interpreter itself, requiring zero new packages. + +## Decision + +`RuleContext` MUST expose a single method: + +```typescript +ast(path: string, language: "typescript" | "javascript" | "python" | "ruby"): Promise; +``` + +This method dispatches internally based on `language`, and the dispatch mechanism MUST be invisible to rule authors — a `.rules.ts` file calls `ctx.ast(path, language)` and receives a parsed tree or an exception; it never sees which mechanism produced it. + +- **`"typescript"` / `"javascript"`** MUST reuse the in-process `meriyah` parser already used by `src/engine/rule-scanner.ts`. No subprocess is spawned for this branch. The inline `parseModule()` invocation currently duplicated in `scanRuleSource()` and `scanImportedRuleSource()` MUST be factored into a shared, exported parse helper that both `rule-scanner.ts` and the new `ctx.ast()` implementation call, rather than introducing a third inline copy. +- **`"python"` / `"ruby"`** MUST invoke the language's own standard-library AST facility as a subprocess via `Bun.spawn`, per [ARCH-007](./ARCH-007-cross-platform-subprocess-execution.md): Python's built-in `ast` module (` -c "..."`, serializing the tree to JSON), Ruby's built-in `Ripper` (`ruby -rripper -rjson -e "..."`, serializing its s-expression output to JSON). `` is whichever candidate name (`python3`/`python`) the interpreter availability probe below resolved for this platform — never hardcoded. No third-party parser, native binding, or WASM grammar is introduced for these languages under this decision. + +**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). +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` on Windows, 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. + +**Failure semantics.** `ctx.ast()` MUST throw — it MUST NOT return `null` or any other sentinel — both when the required interpreter is unavailable and when the target file fails to parse. This is a deliberate choice, not an oversight: this ADR does not introduce any new error-boundary or exit-code behavior, and none is needed, because `ctx.ast()`'s failure mode composes directly with contracts `src/engine/runner.ts` and `src/engine/reporter.ts` already implement. Every rule's `check(ctx)` call already runs inside a per-rule `try/catch` (`runner.ts`, the loop over `Object.entries(ruleSet.rules)`) that isolates a thrown error to that single rule — other ADRs and rules in the same `check` run continue and report normally. `reporter.ts`'s `getExitCode()` already reserves exit code `2` specifically for rule execution errors, distinct from exit `1` (ADR violations found) and exit `0` (pass). A thrown `ctx.ast()` error therefore surfaces as a visible, correctly-categorized failure through machinery that already exists; a `null` return would instead let a rule silently no-op and report as a false "0 violations," masking a real capability gap as a pass. The exit-code/reporter distinction is coarse by design (exit `2` means "a rule could not complete," full stop) — the two throw cases MUST still be distinguishable from each other in the thrown error's message text (e.g. "Python interpreter not found on PATH" vs. "Failed to parse ``: ``"), since a user reading `check` output needs to tell "this environment can't run this rule" apart from "this specific file has a syntax error" even though both land on the same exit code. + +**Explicit non-goal: cross-language AST shape unification.** `ctx.ast()` unifies the call site and the failure contract across languages. It does NOT unify the shape of the returned tree. TypeScript/JavaScript returns ESTree-shaped nodes (via `meriyah`); Python returns whatever the standard `ast` module's own node schema produces; Ruby returns `Ripper`'s native s-expression shape. A rule author writing a Python check and a rule author writing a Ruby check are working against two different, language-native grammars, and must know the target language's own AST vocabulary. This ADR accepts that trade explicitly in exchange for avoiding the dependency and distribution cost of a unifying parser (see the tree-sitter alternatives above); it is not a limitation to be silently discovered later. + +**Scope.** This ADR covers the `RuleContext.ast()` method signature, its internal dispatch and guardrail ordering, and its failure semantics. It does not cover: which languages ship in which release, rollout sequencing, or example rule authoring guidance — those are product decisions tracked separately, not architectural constraints. + +## Do's and Don'ts + +### Do + +- **DO** implement `ast(path, language)` as a single method on `RuleContext` with dispatch entirely internal to `createRuleContext()` in `src/engine/runner.ts` +- **DO** reuse the existing `meriyah`-based parser for `"typescript"`/`"javascript"`, factoring the duplicated `parseModule()` call in `rule-scanner.ts` into one shared helper used by both the scanner and `ctx.ast()` +- **DO** run the path-safety, language-plausibility, interpreter-availability, and guarded-invocation checks in exactly that order, before any subprocess is spawned, for the `"python"`/`"ruby"` branches +- **DO** use `Bun.spawn` with array-based arguments for the Python/Ruby subprocess invocations, per [ARCH-007](./ARCH-007-cross-platform-subprocess-execution.md) +- **DO** cache the interpreter-availability probe once per `check` invocation +- **DO** throw from `ctx.ast()` on missing interpreter or parse failure, and let it propagate to the existing per-rule `try/catch` in `runner.ts` +- **DO** document, in the type signature or accompanying JSDoc, that the returned node shape differs per language + +### Don't + +- **DON'T** expose `Bun.spawn`, `child_process`, or any other raw subprocess primitive on `RuleContext` — `ctx.ast()` is the only sanctioned path to language tooling +- **DON'T** return `null` or any other silent-failure sentinel from `ctx.ast()` — this would hide a capability gap as a false passing check +- **DON'T** invoke the Python/Ruby interpreter on a file before the language-plausibility check has run +- **DON'T** add `tree-sitter`, `web-tree-sitter`, or any other new production dependency under this decision — Python/Ruby support MUST use only the interpreter's own standard-library AST facility +- **DON'T** attempt to normalize Python/Ruby output into an ESTree-like shape as part of this ADR — that is explicitly out of scope +- **DON'T** re-probe interpreter availability on every file — cache it per `check` run + +## Consequences + +### Positive + +- **Structural checks become possible for TypeScript/JavaScript without new dependencies** — `ctx.ast()`'s TS/JS branch reuses `meriyah`, already present in the tree, closing the gap that forces `ARCH-004` and `ARCH-008` into regex heuristics today. +- **Python/Ruby structural checks become possible with zero new production dependencies** — using each language's own standard-library AST facility means no native binding, no WASM asset, and no ARCH-006 dependency review is required to ship this. +- **Consistent, auditable sandbox boundary** — extending, rather than bypassing, the existing `rule-scanner.ts`/`RuleContext` sandboxing model means the security posture of `.rules.ts` execution does not change in kind, only in the set of capabilities exposed through the same narrow door. +- **Failure visibility reuses proven machinery** — no new exit code, no new reporter branch, no new error-boundary design; `ctx.ast()`'s throw-on-failure behavior rides on `runner.ts`'s existing per-rule isolation and `reporter.ts`'s existing exit-code-2 category. +- **Incremental adoption** — TS/JS support ships using zero new capability surface beyond what already exists internally; Python/Ruby support can follow independently since the guardrail and failure-semantics design is identical for both. + +### Negative + +- **No cross-language AST vocabulary** — a rule author supporting both Python and Ruby structural checks must learn two unrelated grammars (the standard `ast` module's schema and `Ripper`'s s-expression shape), unlike a tree-sitter-based approach which would have offered one vocabulary across languages. +- **Environmental dependency for Python/Ruby rules** — `ctx.ast()` for those languages depends on a Python or Ruby interpreter being present on the machine running `archgate check`. This is not a package the project controls or bundles; a rule targeting Python will correctly fail (via the throw-based contract above) on a machine without a Python interpreter, which is a real limitation, not just a theoretical one. +- **`meriyah` gains a runtime execution path it did not previously have** — today `meriyah` runs only inside the `check` engine's rule-scanning step; after this decision it also runs, via `ctx.ast()`, at rule-execution time inside the compiled binary shipped to end users. This does not require a new ARCH-006 review (no new package is added), but it changes the package's practical scope from "internal scanning tool" to "runtime capability," and maintainers should be aware of that shift when evaluating future `meriyah` upgrades. +- **Language-specific grammar drift is inherited, not controlled** — standard-library AST facilities are not immune to internal restructuring across language versions (e.g., Python's `ast` module deprecating `ast.Str`/`ast.Num` in favor of `ast.Constant` in 3.8). `ctx.ast()`'s own contract does not change when this happens, but a rule author's language-specific pattern matching can still break; this ADR does not attempt to insulate rule authors from upstream grammar changes. + +### Risks + +- **A future contributor bypasses the guardrail ordering and spawns the Python/Ruby interpreter directly from inside a `ctx.ast()` code path without the path-safety or language-plausibility checks.** + - **Mitigation:** the four-step guardrail ordering in the Decision section is mandatory and reviewable; `rule-scanner.ts`'s existing `BLOCKED_BUN_PROPS` sandbox continues to prevent `.rules.ts` files themselves from reaching `Bun.spawn`, so the only code path capable of spawning a subprocess for this feature is `createRuleContext()` itself, which code review MUST verify follows the ordering exactly. +- **Interpreter-version skew between the machine authoring a Python/Ruby rule and machines running `archgate check` produces inconsistent AST shapes for the same source file.** + - **Mitigation:** this is inherent to shelling out to system-installed interpreters rather than bundling a pinned parser version, and is accepted as part of choosing Option A over tree-sitter. Rule authors targeting Python/Ruby structural checks should keep patterns tolerant of minor version-specific node shape differences, and the interpreter-availability probe surfaces the interpreter's version so this can be logged for diagnosis. +- **The duplicated inline `parseModule()` calls in `rule-scanner.ts` are not factored out before `ctx.ast()`'s TS/JS branch is implemented, leaving three near-identical parse call sites instead of two.** + - **Mitigation:** the Decision section explicitly mandates factoring this into one shared helper as part of implementing this ADR, not as optional cleanup. + +## Compliance and Enforcement + +### Automated Enforcement + +- None at this time. `rules: false` — this ADR documents an engine/API design decision made ahead of implementation. Once `ctx.ast()` ships, a follow-up amendment to this ADR (or a new companion ADR) MUST introduce `rules: true` with an automated check that flags any direct `Bun.spawn`/`child_process` usage inside `src/engine/runner.ts`'s `createRuleContext()` implementation that bypasses the mandated guardrail ordering, mirroring how `ARCH-007/no-bun-shell` scans for banned subprocess patterns today. + +### Manual Enforcement + +Code reviewers MUST verify, for any PR implementing or modifying `ctx.ast()`: + +1. `RuleContext` exposes exactly one `ast(path, language)` method — no per-language method variants (`ctx.pythonAst()`, `ctx.rubyAst()`, etc.) +2. The four-step guardrail ordering (path safety, language plausibility, interpreter probe, guarded invocation) is implemented in full and in order for the `"python"`/`"ruby"` branches +3. No new production dependency appears in `package.json` as part of this feature +4. `ctx.ast()` throws (never returns `null` or another sentinel) on missing interpreter or parse failure +5. The `meriyah` `parseModule()` call is shared between `rule-scanner.ts` and the `ctx.ast()` TS/JS branch, not duplicated a third time +6. No subprocess invocation for this feature uses `Bun.$` or any shell-interpolated command string, per [ARCH-007](./ARCH-007-cross-platform-subprocess-execution.md) + +### Exceptions + +Any proposal to add a bundled multi-language parser (tree-sitter, WASM grammars, or otherwise) to broaden `ctx.ast()`'s guarantees beyond this ADR's scope MUST be documented as a separate ADR, reviewed against [ARCH-006](./ARCH-006-dependency-policy.md)'s dependency-approval process, and approved by the project maintainer before implementation begins. + +## References + +- [ARCH-006 — Dependency Policy](./ARCH-006-dependency-policy.md) — This decision requires no new production dependency; Python/Ruby support relies entirely on system-installed interpreters rather than an approved-list addition +- [ARCH-007 — Cross-Platform Subprocess Execution](./ARCH-007-cross-platform-subprocess-execution.md) — Governs the `Bun.spawn` array-argument pattern used for the Python/Ruby subprocess branches +- [ARCH-004 — No Barrel Files or Re-Exports](./ARCH-004-no-barrel-files.md) — The `isBarrelFile()` line-heuristic is a concrete example of the regex-over-text limitation this ADR addresses for TypeScript/JavaScript +- [ARCH-008 — Typed Command Options](./ARCH-008-typed-command-options.md) — The `.option()` call-shape regex checks are a second concrete example of the same limitation +- `src/engine/rule-scanner.ts` — The existing `meriyah`-based AST sandbox this decision extends; note this mechanism itself is not currently documented by a formal ADR, which is a documentation gap outside this ADR's scope +- [Python `ast` module documentation](https://docs.python.org/3/library/ast.html) +- [Ruby `Ripper` documentation](https://docs.ruby-lang.org/en/master/Ripper.html) +- [meriyah (npm)](https://www.npmjs.com/package/meriyah) diff --git a/.archgate/adrs/CI-002-validate-workflow-syntax-with-actionlint.md b/.archgate/adrs/CI-002-validate-workflow-syntax-with-actionlint.md new file mode 100644 index 00000000..45790b63 --- /dev/null +++ b/.archgate/adrs/CI-002-validate-workflow-syntax-with-actionlint.md @@ -0,0 +1,118 @@ +--- +id: CI-002 +title: Validate Workflow Syntax with Actionlint +domain: ci +rules: false +--- + +## Context + +### Problem Statement + +`.github/workflows/*.yml` files are hand-written YAML with a schema GitHub enforces only at execution time — a malformed `permissions:` key, an invalid expression, or a shellcheck-flagged `run:` block is invisible until the workflow actually runs (or, worse, silently does nothing and never runs at all in the way the author intended). This repository's only workflow-file static analysis is `zizmor` (`.github/workflows/code-pull-request.yml`'s `zizmor` job, config at `zizmor.yml`), which is a **security**-focused scanner: template-injection, credential persistence, unpinned actions. It has no concept of GitHub's actual permission-scope schema, expression syntax, or `run:`-block shell correctness — those are a different class of defect entirely. + +### Pain Points + +- A workflow file can contain a syntactically well-formed but semantically invalid key (e.g., a `permissions:` scope name that does not exist) and pass every check this repository runs, because no check in the pipeline validates workflow YAML against GitHub's schema +- Such an error can sit unnoticed indefinitely if the specific job never runs during ordinary development (release-only jobs, in particular, execute rarely — sometimes only once per release) +- Code review by a human or an AI reviewer without workflow-schema expertise cannot reliably catch this class of error by reading the YAML — the key looks plausible, resembles a real permission name, and the mistake is only obvious against GitHub's actual documented scope list +- **Concrete incident**: [PR #451](https://github.com/archgate/cli/pull/451) added `workflows: write` to `publish-shims.yml`'s `publish-go-tag` job `permissions:` block, based on a previous incident's fix (recorded in `.claude/agent-memory/archgate-developer/project_release_pipeline_gotchas.md`) intended to resolve a GitHub push-rejection error: `refusing to allow a GitHub App to create or update workflow ... without workflows permission`. The change passed code review and merged. `workflows` is not, and has never been, a valid `permissions:`-key scope — confirmed independently against GitHub's own live workflow-syntax documentation. The invalid key had been silently doing nothing since it was added. It surfaced only when a later PR's CodeRabbit review happened to run `actionlint` internally and flagged `unknown permission scope "workflows"` — this repository's own CI never would have caught it, because zizmor performs no schema validation of this kind. + +### Alternatives Analysis + +- **Rely on zizmor alone**: Already in place and valuable, but explicitly out of scope for schema correctness — see Problem Statement. Expanding zizmor's own rule set is not an option; schema validation is not its design goal, and its maintainers do not position it as a schema linter. +- **Rely on third-party review tooling (CodeRabbit, Cursor Bugbot) to catch this class of error**: This is what actually caught the PR #451 incident, but it is not a dependable control — third-party review tools are not guaranteed to run actionlint internally, their internal tooling is not something this project controls or can pin, and relying on an external reviewer's implementation detail to catch a class of bug this project could check directly is not a real enforcement strategy. +- **`reviewdog/action-actionlint`**: A maintained GitHub Action wrapping actionlint with reviewdog-style PR annotations. Rejected in favor of installing the actionlint binary directly: the reviewdog wrapper runs as a Docker container action, adding both a new third-party Action to this repository's trust surface (subject to [CI-001](./CI-001-pin-github-actions-by-hash.md)'s SHA-pinning requirement) and Docker-image execution overhead, for what is fundamentally a single static-check binary invocation. The direct-binary approach needs no `uses:` reference at all. +- **Manual periodic audits of workflow files**: Does not scale and has no enforcement mechanism — exactly the failure mode that let the PR #451 defect merge in the first place. + +### Project-Specific Motivation + +For the Archgate CLI, the release pipeline (`publish-shims.yml`, `release-binaries.yml`) is exercised far less frequently than the pull-request pipeline — some jobs run only once per release, weeks apart. A schema defect in a release-only job can sit dormant through many PR merges before it is ever executed for real, at which point it fails during an actual release rather than during routine development. Catching this class of defect on every PR, before merge, is strictly better than discovering it during a release. + +## Decision + +`.github/workflows/code-pull-request.yml` MUST run `actionlint` as a dedicated `actionlint` job, included as a required dependency of the `status` gate job (the single required status check for branch protection) — a hard blocker, not an advisory-only check. + +**Installation**: `actionlint` MUST be installed by downloading the maintainer's prebuilt release binary via `rhysd/actionlint`'s own `scripts/download-actionlint.bash`, fetched from a specific 40-character commit SHA of that script (not a mutable branch reference), with an explicit pinned version argument (not `latest`): + +```yaml +- name: Install actionlint + run: | + bash <(curl -fsSL "https://raw.githubusercontent.com/rhysd/actionlint/<40-char-sha>/scripts/download-actionlint.bash") +- name: Run actionlint + run: ./actionlint -color +``` + +This is a raw script fetch, not a `uses:` action reference — [CI-001](./CI-001-pin-github-actions-by-hash.md)'s automated `no-unpinned-actions` rule does not scan it — but the same reproducibility principle applies voluntarily: both the download script's commit and the actionlint version are pinned, exactly as CI-001 requires for actual `uses:` references. + +**Scope**: This ADR covers only the decision to run `actionlint` as a hard-blocking CI job and how it is installed. It does not cover `zizmor` (governed by its own inline comments in `code-pull-request.yml`, not a formal ADR) and does not revise CI-001's `uses:`-pinning requirements. + +**Relationship to [GEN-003](./GEN-003-tool-invocation-via-scripts.md)**: GEN-003 requires linting/formatting/validation to run through `package.json` scripts, but its Decision text and Do/Don't examples (`bunx prettier`, `bunx oxfmt`, `npx eslint`, `oxlint .`) — and its automated rule's own tool list (`prettier`, `oxfmt`, `oxlint`, `eslint`, `biome`) — are specifically about this project's own JS/TS toolchain. `actionlint` is a standalone external Go binary with no npm or `package.json` involvement at all, invoked directly in a CI job exactly as the pre-existing `zizmor` job invokes its own tool (via direct execution, not an npm script). GEN-003 does NOT apply to CI-only, non-npm-ecosystem static analysis tooling; no `package.json` wrapper script for `actionlint` is required or expected. + +## Do's and Don'ts + +### Do + +- **DO** run `actionlint` as its own job in `.github/workflows/code-pull-request.yml`, listed in the `status` gate job's `needs:` array and result check +- **DO** pin the `download-actionlint.bash` fetch to a specific 40-character commit SHA in the raw.githubusercontent.com URL +- **DO** pin the actionlint version explicitly (e.g. `1.7.12`) — never `latest` +- **DO** set `persist-credentials: false` on the job's `actions/checkout` step, consistent with the `zizmor` job's pattern +- **DO** treat `actionlint` findings as hard blockers — unlike `zizmor`'s advisory carve-outs for fork PRs and its pre-existing findings backlog, `actionlint` starts from a clean slate and should stay that way +- **DO** re-resolve and update both the pinned script SHA and the actionlint version together when upgrading, the same way CI-001 requires for `uses:` references + +### Don't + +- **DON'T** add a `reviewdog/action-actionlint`-style wrapper Action — it adds Docker execution overhead and a new `uses:` trust surface for no capability this project needs beyond pass/fail +- **DON'T** treat `actionlint` findings as advisory-only — this ADR exists specifically because an advisory-only signal (a third-party reviewer's internal tooling) was the only thing that caught the motivating incident, and that is not a dependable control +- **DON'T** add a `package.json` script to wrap `actionlint` invocation under the belief that GEN-003 requires it — GEN-003 governs this project's own JS/TS toolchain, not external CI-only binaries +- **DON'T** fetch the `download-actionlint.bash` script from a branch ref (`main`) or omit the version argument (defaulting to `latest`) — both reintroduce the same class of non-reproducibility CI-001 exists to prevent for `uses:` references + +## Consequences + +### Positive + +- **Catches the exact defect class that caused the motivating incident**: `actionlint` flags invalid `permissions:` scopes, malformed expressions, and shellcheck issues in `run:` blocks before merge, independent of whether a third-party review tool happens to run it +- **Complements, not duplicates, zizmor**: zizmor's security-pattern scanning and actionlint's schema validation cover disjoint failure classes; running both closes a real gap rather than adding redundant signal +- **No new third-party Action trust surface**: the direct-binary installation avoids adding a `uses:` reference, keeping CI-001's SHA-pinning surface unchanged +- **Reproducible tooling**: pinned script commit + pinned actionlint version means the exact same binary runs on every CI invocation until deliberately upgraded +- **Catches defects in rarely-executed release-pipeline jobs before they ever run for real**, closing the specific gap that let the PR #451 defect merge undetected + +### Negative + +- **Another CI job, another few seconds of pipeline time**: adds a small, fixed cost to every PR run (binary download + lint pass), though this is minor relative to the existing pipeline's total duration +- **Manual version bumps**: unlike a `uses:`-pinned Action, Renovate/Dependabot do not automatically propose updates for a pinned raw-script-URL-plus-version-argument pattern; upgrading `actionlint` requires a manual PR + +### Risks + +- **Stale actionlint version**: without automated dependency-update tooling watching this pattern, the pinned version can fall behind new actionlint releases (and their bug fixes or new schema checks). + - **Mitigation:** Treat `actionlint` version bumps the same way CI-001 treats `uses:` SHA bumps — periodic manual review, checked during any broader CI/workflow maintenance pass. +- **A future contributor reintroduces the same class of error in a different workflow file added after this ADR**: `actionlint` runs against all `.github/workflows/*.yml` files by default (via `./actionlint` with no path argument), so this is unlikely, but a future refactor of the job's invocation could accidentally scope it to a subset of files. + - **Mitigation:** Code review of any change to the `actionlint` job step MUST verify the invocation still covers the entire `.github/workflows/` directory with no path restriction. + +## Compliance and Enforcement + +### Automated Enforcement + +- The `actionlint` job in `.github/workflows/code-pull-request.yml`, required by the `status` gate job, fails the pipeline on any `actionlint` finding. + +### Manual Enforcement + +Code reviewers MUST verify, for any change to the `actionlint` job: + +1. The `scripts/download-actionlint.bash` fetch remains pinned to a specific 40-character commit SHA, not a branch reference +2. The actionlint version argument remains an explicit version, not `latest` +3. `actionlint` remains listed in the `status` gate job's `needs:` array and result check — removing it silently downgrades this from a hard blocker to a no-op +4. The job's invocation still scans the entire `.github/workflows/` directory, not a restricted subset + +### Exceptions + +None. If `actionlint` produces a false positive for a legitimate, GitHub-supported syntax it does not yet recognize, resolve by upgrading to a newer `actionlint` version first; if the false positive persists on the current version, escalate to the project maintainer and document the specific suppression (if any) in this ADR rather than silently disabling the job. + +## References + +- [CI-001: Pin GitHub Actions by Commit SHA](./CI-001-pin-github-actions-by-hash.md) — governs `uses:` reference pinning; this ADR applies the same reproducibility principle to a non-`uses:` script fetch +- [GEN-003: Tool Invocation via Package Scripts](./GEN-003-tool-invocation-via-scripts.md) — governs this project's own JS/TS toolchain invocation; does not apply to external CI-only tooling like `actionlint` +- [ARCH-006: Dependency Policy](./ARCH-006-dependency-policy.md) — general project minimalism philosophy informing the rejection of a wrapper Action in favor of direct binary installation +- `.claude/agent-memory/archgate-developer/project_release_pipeline_gotchas.md` — records the motivating incident and its correction +- [rhysd/actionlint](https://github.com/rhysd/actionlint) — the tool itself +- [GitHub Actions workflow syntax — `permissions`](https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-syntax) — the authoritative schema `actionlint` validates against diff --git a/.claude/agent-memory/archgate-developer/MEMORY.md b/.claude/agent-memory/archgate-developer/MEMORY.md index 42787702..01da7a7c 100644 --- a/.claude/agent-memory/archgate-developer/MEMORY.md +++ b/.claude/agent-memory/archgate-developer/MEMORY.md @@ -4,18 +4,15 @@ Every work loop MUST end with these steps — no exceptions, even for trivial changes: -1. **`bun run validate`** — lint, typecheck, format, test, ADR check, knip, build check (fail-fast) +1. **`bun run validate`** — lint, typecheck, format:check, test, ADR check, knip, build check (fail-fast) 2. **`@reviewer` skill** — Invoke via `Skill tool` with skill `"archgate:reviewer"`. Validates structural ADR compliance beyond automated rules. 3. **`@lessons-learned` skill** — Invoke via `Skill tool` with skill `"archgate:lessons-learned"`. Captures learnings and governance gaps. -Skipping steps 2 or 3 is a workflow violation. The user should NEVER have to invoke these manually. (`.claude/settings.json` permissions now correctly allowlist `archgate:reviewer`/`archgate:lessons-learned`/`archgate:adr-author` — the older `archgate:architect`/`archgate:quality-manager` naming issue noted here previously has been fixed; verified 2026-07-01.) +Skipping steps 2 or 3 is a workflow violation. The user should NEVER have to invoke these manually. ## Version References -- **Minimum version** (`>=1.2.21`): Enforced in `src/cli.ts`, documented in CLAUDE.md "Technology Stack". This is the user-facing requirement. -- **Pinned version** (`1.3.14`): Set in `.prototools`, referenced in ADR risk sections (ARCH-005, ARCH-006) and CLAUDE.md "Toolchain" section. This is the dev toolchain version. -- **Pre-1.0 release bump policy**: breaking changes bump MINOR, not major — enforced by the `getNextVersion` cap in `.simple-release.js`. v1.0.0 must be an explicit decision (force via the `version` bump option), never an automatic consequence of a `feat!` commit. -- These are intentionally different. When upgrading the pinned version, update `.prototools` + ADR risk sections + CLAUDE.md toolchain. Do NOT change the minimum unless a new Bun API is required. +- **Minimum version** (`>=1.2.21`) in `src/cli.ts`/CLAUDE.md "Technology Stack" is the user-facing floor; **pinned version** (`1.3.14`) in `.prototools`/CLAUDE.md "Toolchain" is the dev toolchain version — these are intentionally different. Pre-1.0 breaking changes bump MINOR, not major (`.simple-release.js` cap); v1.0.0 requires an explicit forced bump. ## Git Workflow @@ -23,11 +20,10 @@ Skipping steps 2 or 3 is a workflow violation. The user should NEVER have to inv ## Approach Guidance -- [No prod changes for testability](feedback_no_prod_changes_for_tests.md) — mock implementations in tests (spyOn os.homedir works cross-module); never alter prod semantics for test isolation -- [Pick the right enforcement layer](feedback_prefer_tests_over_adr_rules.md) — static syntactic invariants → custom oxlint rule in `.archgate/lint/oxlint.ts`; tests are for executable behavior; ADR .rules.ts for cross-file/governance checks - -- [This repo is PUBLIC — no private sibling-repo internals in memory/PRs](feedback_public_repo_privacy.md) — split captures: public-safe summary here, full detail in the private repo's own memory -- [Keep code comments and memory entries concise](feedback_concise_comments.md) — no multi-paragraph rationale blocks; one line + terse why, link out for detail +- [No prod changes for testability](feedback_no_prod_changes_for_tests.md) — mock in tests (e.g. spyOn), never alter prod semantics for test isolation +- [Pick the right enforcement layer](feedback_prefer_tests_over_adr_rules.md) — static syntax → custom oxlint rule; executable behavior → tests; cross-file/governance → ADR `.rules.ts` +- [This repo is PUBLIC — no private sibling-repo internals in memory/PRs](feedback_public_repo_privacy.md) +- [Keep code comments and memory entries concise](feedback_concise_comments.md) — one line + terse why, link out for detail ## Known Bugs @@ -35,55 +31,31 @@ Skipping steps 2 or 3 is a workflow violation. The user should NEVER have to inv ## Platform Limitations -- **Content filtering on policy/legal text** — Writing files containing Contributor Covenant, license text, or similar legal boilerplate (e.g., `CODE_OF_CONDUCT.md`) may trigger API content filtering and block output. Do NOT attempt to auto-generate these files. Instead, tell the user to copy the content manually from the official source (e.g., https://www.contributor-covenant.org/version/2/1/code_of_conduct/). +- **Content filtering on policy/legal text** — Auto-generating Contributor Covenant/license boilerplate (e.g. `CODE_OF_CONDUCT.md`) can trigger API content filtering. Tell the user to copy it manually from the official source instead. ## Patterns & Fixes -Non-enforceable lessons — environment/CI/platform quirks no static rule can reliably catch. (Conventions that ARE machine-checked live in their ADRs under `.archgate/adrs/`; the agent reads those before coding, so they are intentionally not duplicated here.) - -- **Custom oxlint JS plugins live in `lint/*.ts`, registered via `jsPlugins` in `.oxlintrc.json`** — The repo has a custom plugin `lint/expect-expect.ts` (rule id `bun-test/expect-expect`) that fails the build for any runnable `bun:test` `test()`/`it()` (incl. `test.skipIf(...)()`, `test.each(...)()`) whose body has no `expect()` call; it ignores `test.skip`/`test.todo`. It exists because oxlint's built-in `jest/expect-expect` only recognizes `jest`/`vitest` imports and silently skips `bun:test`. The plugin uses the ESLint-compatible default-export shape (`{ meta:{name}, rules:{ "x": { create(ctx){ return { CallExpression(node){...} } } } } }`) and runs as native TS under Bun (no build step). Important scoping: `lint/` is NOT in `tsconfig.json` `include` (so `tsc` does not typecheck it) and NOT in `knip.json` `project` (so knip ignores it) — but `oxlint --deny-warnings .` DOES lint it, so the plugin file must itself pass all oxlint rules. To enable a jsPlugin rule only for tests, list it under an `overrides` entry for `tests/**/*.test.ts` (the plugin is loaded top-level via `jsPlugins`, the rule is turned on in the override). The convention is now documented in ARCH-005. -- **oxlint `unicorn/no-array-callback-reference`** — Don't pass a bare function reference to `.map()`/`.find()`/`.filter()` etc. (e.g. `args.map(asNode)`); the rule wants the element-only contract made explicit. Wrap in an arrow: `args.map((x) => asNode(x)).find((x) => isFn(x))`. -- **oxlint `require-unicode-regexp`** — Regex literals need the `u` flag: `/Git is not installed/u`, not `/Git is not installed/`. Applies in tests too (e.g. `expect(...).rejects.toThrow(/.../u)`). -- **`oxfmt` formats markdown too — run `bun run format` after editing any `.md`, including ADRs** — `format:check` (part of `bun run validate` and the CI "Lint, Test & Check" job) runs `oxfmt --check .` over ALL files, not just `.ts`. It normalizes markdown (e.g., emphasis `*word*` → `_word_`). The `adr-author` skill writes ADR markdown but does NOT auto-format it, so ADR edits frequently fail CI `format:check` even when the TS changes are clean. Always run `bun run format` before committing ADR/markdown edits. Tripped CI on PR #372 (ARCH-005 edit). -- **YAML double-quoted strings require escaped backslashes for Windows paths in tests** — YAML interprets `\` as an escape character inside double-quoted strings. Writing `cwd: "E:\project"` silently corrupts the parsed value because `\p` is not a valid escape sequence. Fix: use `JSON.stringify(path)` to produce properly escaped YAML values (e.g., `cwd: ${JSON.stringify(cwd)}`). JSON and YAML double-quoted strings share the same escape syntax. Encountered in Copilot CLI session-context tests (`workspace.yaml` with Windows paths). -- **`Bun.Glob.match()` triggers oxlint `prefer-regexp-test`** — `Bun.Glob.match()` returns a boolean (not a RegExp), but oxlint can't tell. Suppress with `// oxlint-disable-next-line prefer-regexp-test -- Bun.Glob.match() returns boolean, not RegExp`. -- **Live `opencode.db` can't be opened `readonly: true` while opencode runs** — `new Database(path, { readonly: true })` fails with `SQLITE_CANTOPEN` (errno 14) on the live WAL-mode DB (`~/.local/share/opencode/opencode.db`). To inspect real data, copy `opencode.db` + `.db-wal` + `.db-shm` to a temp dir and open the copy. Data model facts load-bearing for `session-context opencode`: sub-agent runs are child sessions (`parent_id` set) sharing the parent's `directory`, and opencode skills run INLINE in the calling session (no own session row) — recency selection must filter `parent_id IS NULL` (fixed 2026-07-01; `--root` resolves a `--session-id` child to its top-level ancestor). -- [session-context --skip 1 inline-skill bug](project_session_context_skip_root_fix.md) — opencode fixed via top-level default + `--root`; claude-code/cursor/copilot guidance fixed with plain command — the "skill runs as a sub-agent" premise was false everywhere -- **The distributed opencode lessons-learned skill references session-context CLI flags — sequence releases** — the skill instructs the plain `archgate session-context opencode` (was `--skip 1`, which read sub-agent transcripts; the escape hatch references `session-context list`/`show`). When changing session-context CLI semantics, update the shipped skill in the same effort AND sequence releases. Flag ADDITIONS: CLI ships first (a skill referencing a flag the installed CLI lacks dies with "unknown option"). Flag REMOVALS (e.g. --skip, removed 2026-07-02): already-installed skills still reference the dead flag and their command errors on the new CLI — ship the plugin release promptly after the CLI release and keep an error-fallback path in the skill text. Don't hand-edit the installed copy under `~/.config/opencode/skills/` before the CLI release. -- **Verify haiku reviewer findings against the cited ADR's text before blocking** — A haiku review sub-agent reported "await on a synchronous helper" as an ARCH-012 violation; ARCH-012 only mandates try-catch boundaries/exit codes and its automated rules passed. Re-read the cited ADR's Decision/Do's before accepting a FAIL; overrule misattributed style nits but still surface them as non-ADR notes. Recurred 2026-07-01: the same await-on-sync-helper nit came back self-labeled "ARCH-NONE" — a finding citing no ADR can never block. -- **Commander hoists parent-known options away from nested subcommands** — if a parent command and its child subcommand declare the same option (e.g. `session-context ` and ` show` both taking `--max-entries`), commander (without positional-options mode) parses the flag onto the PARENT no matter where it appears in argv — the child's `opts` silently gets `undefined`. Read merged values in the child action via `command.optsWithGlobals()` (third action parameter), and add an in-process regression test that passes the flag through the full `parseAsync` path. Shipped broken in v0.46.0, fixed in PR #448. -- **Bun `mock.module` state is process-global across test FILES** — a helper mock registered in one command test file leaks into other files' imports of the same module (live bindings get re-bound to the mock), and module-level constants that capture function references freeze whatever binding existed at load time. Symptom: tests pass in isolation, fail (or silently hit mocks) in the full run. When a command test needs REAL helper behavior while sibling files mock those helpers, spawn the CLI via `tests/integration/cli-harness` `runCli` with `HOME`/`USERPROFILE`/`XDG_DATA_HOME` redirected to a temp dir (fresh subprocess = env-based homedir works). This is also why ARCH-005 prefers `spyOn` over `mock.module`. Hit in session-context list/show tests (PR #446). -- **Git Bash `/tmp` is invisible to Windows-native tools** — A bash redirect to `/tmp/x` writes into Git Bash's virtual mount, but Windows-native python/node then fail with `FileNotFoundError` on that path (gh.exe survives only because Git Bash rewrites path-looking arguments). When piping a file between bash and Windows-native tools, use a repo-relative path (and clean it up) or `$TMPDIR`. -- **oxlint `no-negated-condition`** — Always write ternaries with the positive condition first: `x === null ? A : B` not `x !== null ? B : A`. Applies to both `if/else` blocks and ternary expressions. -- **oxlint `no-unused-vars` on catch parameters** — Use bare `catch { }` (no parameter) when the caught error is not used. `catch (err) { }` with unused `err` triggers the rule. -- **oxlint `no-await-in-loop`** — Sequential `await` inside a `for` loop is flagged (warning). When the sequential order is intentional (e.g., build steps with per-step output), suppress with `// oxlint-disable-next-line no-await-in-loop -- `. -- **Git credential tests need system-level isolation on Windows** — Overriding `Bun.env.HOME` is NOT sufficient to isolate `git credential fill/approve` calls in tests. Windows Credential Manager is a system-level API, not file-based. Tests MUST set `Bun.env.GIT_CONFIG_NOSYSTEM = "1"` and `Bun.env.GIT_CONFIG_GLOBAL = ` to prevent git from reading the real credential helper config. Without this, tests on machines with stored credentials will pick up real tokens. -- **GCM prompt suppression requires 5 env vars** — `GIT_TERMINAL_PROMPT=0` alone does NOT prevent Git Credential Manager (GCM) from showing GUI prompts on Windows or askpass prompts on Linux. The full set for `gitCredentialEnv()` in `src/helpers/credential-store.ts` is: `GIT_TERMINAL_PROMPT=0`, `GCM_INTERACTIVE=never`, `GCM_GUI_PROMPT=false`, `GIT_ASKPASS=""`, `SSH_ASKPASS=""`. Omitting any one can trigger unexpected prompts in editor contexts where the CLI runs as a subprocess. -- **Module-level `{ ...Bun.env }` captures env at import time** — Spreading `Bun.env` into a module-level constant freezes the env snapshot. Tests that override `Bun.env.HOME` after import won't affect the constant. Fix: use a function that returns `{ ...Bun.env, ... }` on each call so it picks up test-time overrides. Applied in `src/helpers/credential-store.ts`. -- **Windows binary upgrade: never use detached child processes for `.old` cleanup** — On Windows, `replaceBinary()` renames the running exe to `.old` because the OS file-locks it. Cleaning up the `.old` via a detached `cmd /c ping -n 2 ... & del` process is unreliable (process may not spawn, `del` may fail silently, timing races). Instead, `cleanupStaleBinary()` runs at the next CLI startup as a fire-and-forget `unlink()` — the file is guaranteed unlocked by then. The cleanup is platform-agnostic (uses `getArtifactInfo()` to resolve the binary name), so it works on any supported platform even though only Windows currently creates `.old` files. The sync `unlinkSync` in `replaceBinary()` is kept as defense-in-depth for leftover `.old` files from previous upgrades. Do NOT reintroduce detached cleanup processes. -- **`bun:sqlite` file handles persist after `db.close()` on Windows — wrap test cleanup in try/catch** — Tests that create temp SQLite databases via `new Database(path)` will fail with `EBUSY: resource busy or locked` when `rmSync` tries to remove the temp directory in `afterEach`, even after calling `db.close()`. Windows holds the file handle briefly. Fix: (1) set `PRAGMA journal_mode = DELETE` in test DBs to avoid creating WAL/SHM files, and (2) wrap `rmSync` in `afterEach` with `try { rmSync(...) } catch { /* SQLite handles may persist */ }`. Each test must use a unique temp dir name so leftover files don't collide. -- **`GITHUB_TOKEN`-authored pushes do NOT trigger downstream workflows** — When an Actions workflow pushes commits or opens PRs using `${{ github.token }}` / `secrets.GITHUB_TOKEN`, GitHub suppresses the resulting `push`/`pull_request` events (anti-recursion). Symptom: required PR checks never run, so branch protection treats the PR as missing checks. Fix: author such pushes with a GitHub App installation token (`actions/create-github-app-token`) passed to both `actions/checkout` and the pushing step. Applies to any new workflow that pushes to a branch whose downstream CI must run. -- **`Bun.env` modifications in parallel test files leak into integration test subprocesses** — Bun test runner runs all test files in a single process sharing `Bun.env`. Tests that set `Bun.env.HOME`, `Bun.env.GIT_CONFIG_NOSYSTEM`, or `Bun.env.GIT_CONFIG_GLOBAL` (e.g., `auth.test.ts`, `credential-store.test.ts`) modify the shared environment. Integration tests that spawn CLI subprocesses via `runCli()` spread `process.env` (which IS `Bun.env`) into the child, inheriting the leaked values. Symptom: git operations in the subprocess fail with "not a git repo" or similar, but the test passes in isolation. Fix: integration tests that rely on git must explicitly reset git-related env vars in the `runCli` call: `runCli(args, dir, { GIT_CONFIG_NOSYSTEM: "", GIT_CONFIG_GLOBAL: "" })`. Applied in `tests/integration/check.test.ts` for the `--base` tests. -- **Cross-command I/O sharing: export from the existing command file, don't create shared files** — When two commands need to share I/O functions (console.log with styleText), you CANNOT put them in `src/helpers/` (ARCH-002 forbids console.log in helpers) or create a new file under `src/commands//` without a register function (ARCH-001 requires register\*Command export, ARCH-016 requires docs heading). The correct pattern: export the shared functions from the command file that already defines them (e.g., `plugin/install.ts` exports `installForEditor()` and `printManualInstructions()`) and import them in the other command. Applied in `upgrade.ts` importing from `./plugin/install`. -- **macOS `/var` → `/private/var` symlink breaks temp dir path comparisons in tests** — On macOS, `/var` is a symlink to `/private/var`. `mkdtempSync(join(tmpdir(), ...))` returns `/var/folders/...` but `process.cwd()` after `chdir()` resolves the symlink to `/private/var/folders/...`. Tests that compare `tempDir` against paths derived from `process.cwd()` or `findProjectRoot()` will fail. Fix: always wrap `mkdtempSync` with `realpathSync` in test setup: `tempDir = realpathSync(mkdtempSync(join(tmpdir(), "archgate-test-")))`. This normalizes the path upfront. Discovered in v0.38.0/v0.39.0 release builds — PR CI runs on ubuntu-latest only, so macOS-specific issues are invisible until the release workflow. -- **jq on Windows Git Bash emits CRLF line endings** — `jq -r` output ends lines with `\r\n`. In sh scripts, command substitution strips only trailing newlines, so parsed values carry a trailing `\r` (and multi-line lists get `\r` on every entry except the last). Symptom: charset validations reject valid values, or URLs get an embedded CR. Fix: pipe jq (and grep/sed fallbacks) through `tr -d '\r'`. Bit us in `install.sh` resolve_version — the release-walk skipped every tag except the last one. -- **Don't test that well-known tools exist on PATH** — Tests like `expect(resolveCommand("bun")).toBe("bun")` assert CI environment state, not application logic. They fail when the runner installs tools via shims (e.g., proto on macOS ARM64 where `Bun.which` returns null). Delete such tests entirely — the "returns null for non-existent command" tests already cover `resolveCommand`'s actual logic, and WSL-specific tests cover the `.exe` fallback path. -- **`Bun.Glob.scan()` silently fails for brace patterns with path separators** — `new Bun.Glob("svc/{src/env.ts,env.ts}").scan(...)` returns zero results (no error), while `.match()` works correctly for the same pattern. The match engine was rewritten in Bun 1.2.3 (PR #16824) to expand braces recursively, but the scanner (`GlobWalker.zig`) was not updated. Filed upstream as [oven-sh/bun#32596](https://github.com/oven-sh/bun/issues/32596). Workaround: `expandBracePattern()` in `src/engine/runner.ts` pre-expands brace groups containing `/` before scanning. Applied in `ctx.glob()`, `ctx.grepFiles()`, and `resolveScopedFiles()`. -- **ARCH-020 `glob-scan-dot` rule triggers on `.scan()` in comments** — The rule uses regex `/\.scan\(([^)]*)\)/gu` which matches `.scan()` text in JSDoc/inline comments (e.g., `Bun.Glob.scan() silently...`). Workaround: rephrase comments to avoid the exact `.scan()` text — e.g., "Bun.Glob scanning silently..." instead of "Bun.Glob.scan() silently...". -- **GitHub Actions `secrets.*` and `vars.*` are separate namespaces — configuring a value as one does NOT make it readable via the other** — `release.yml`'s "Annotate release in PostHog" step read `POSTHOG_PROJECT_ID` via `${{ vars.POSTHOG_PROJECT_ID }}`, but the value was only ever configured as a repo **secret** (confirmed with `gh secret list` vs `gh variable list` — zero repo variables existed). `vars.POSTHOG_PROJECT_ID` therefore always resolved to an empty string. Combined with `continue-on-error: true`, an internal guard that does `exit 0` (not a failure) when required config is missing, and a low-visibility `::notice::` log line, the step silently no-opped on every release for ~3 weeks (9 releases, v0.45.0–v0.45.7) with zero CI failures to flag it — confirmed via `gh run view --log` showing `POSTHOG_PROJECT_ID: ` blank in the step's masked env dump. Before wiring a new `secrets.X`/`vars.X` reference, verify with `gh secret list`/`gh variable list` which namespace actually holds the value. For any `continue-on-error` step with a skip-on-missing-config guard, use `::warning::` (not `::notice::`) so misconfiguration surfaces in the Actions UI instead of being invisible indefinitely. Also documented in the project's `CLAUDE.md`. -- **Release workflow chaining:** `release-binaries.yml` dispatches `publish-shims.yml` after binaries + provenance succeed, avoiding a `release: published` race. See [[project_release_pipeline_gotchas]]. -- **`moonrepo/setup-toolchain` cache can break PATH** after a `.prototools` bump; self-heals on retry. See [[project_release_pipeline_gotchas]]. -- **CLI update-check notice can pollute stdout** — gated via `shouldPerformUpdateCheck()`. See [[project_release_pipeline_gotchas]]. +Non-enforceable lessons — environment/CI/platform quirks no static rule can reliably catch. (Conventions that ARE machine-checked live in their ADRs under `.archgate/adrs/`, so they're not duplicated here.) + +- [oxlint rule gotchas + custom jsPlugins convention](project_oxlint_gotchas.md) — expect-expect plugin, array-callback-reference, unicode-regexp, prefer-regexp-test, no-negated-condition, catch-param, no-await-in-loop, ARCH-020 comment trigger, oxfmt-on-markdown +- [Test isolation gotchas](project_test_isolation_gotchas.md) — mock.module process-global leakage, Bun.env leaking across test files, Windows git-credential/GCM isolation, bun:sqlite EBUSY, macOS /var symlink, don't test PATH tools +- [Windows subprocess/path gotchas](project_windows_subprocess_gotchas.md) — Git Bash /tmp invisible to native tools, YAML backslash escaping, binary-upgrade `.old` cleanup, module-level `Bun.env` spread capture +- [CI workflow gotchas](project_ci_workflow_gotchas.md) — GITHUB_TOKEN pushes don't trigger workflows, secrets vs vars namespaces, jq CRLF on Windows Git Bash +- [Rules engine / command internals](project_rules_engine_internals.md) — Bun.Glob brace-pattern scan bug, commander option hoisting, cross-command I/O sharing pattern, verifying reviewer sub-agent ADR citations +- [session-context --skip 1 inline-skill bug](project_session_context_skip_root_fix.md) — opencode fixed via top-level default + `--root`; other editors fixed with plain command; includes opencode.db inspection technique +- [CLI-skill flag sequencing across releases](project_cli_skill_flag_sequencing.md) — ship CLI first for flag additions, ship plugin promptly after for removals; installed lessons-learned skill v0.13.1 confirmed still broken +- [PR review thread triage](project_pr_review_thread_triage.md) — REST API doesn't expose resolved state; use GraphQL `reviewThreads.isResolved` to find genuinely outstanding comments +- [Release pipeline gotchas](project_release_pipeline_gotchas.md) — workflow-trigger race, moonrepo/setup-toolchain cache bug, update-check stdout pollution, publish-go-tag permissions ## Claude Code Harness Config -- [WorktreeCreate hook contract](project_worktree_create_hook_contract.md) — stdin JSON in, path-only stdout out; hook owns the entire worktree creation once configured, not just post-setup +- [Hooks config (`.claude/settings.json`)](project_claude_code_hooks_config.md) — WorktreeCreate contract (stdin JSON in, path-only stdout out) + the `"shell": "bash"` requirement for POSIX hooks +- [WorktreeCreate hook bug history](project_worktree_create_hook_contract.md) — 5 rounds of fixes, re-test all of them if this hook changes - [Cursor Approval Agent is external, not in-repo](reference_cursor_approval_agent.md) — "Archgate CLI Approver" automation lives on cursor.com; no APPROVAL_POLICY.md/ROUTING.md exist in this repo ## Translation Quality -- **Docs have TWO locales: `nb/` AND `pt-br/`** — Editing any English docs page requires updating BOTH `docs/src/content/docs/nb/` and `docs/src/content/docs/pt-br/` in the same changeset (GEN-002 `i18n-translation-drift` is an error-severity rule). Don't stop at nb. Also: locale pages can silently lack whole sections present in English (e.g., a section might exist in English but be entirely absent from a locale page) — the drift rule only checks that the file was touched, not content parity, so compare section structure when editing. -- **Norwegian (nb/) diacritical patterns to scan for** — When reviewing or editing `docs/src/content/docs/nb/` translations, scan for three corruption patterns: (1) stripped diacriticals (`monster` for `mønster`, `a` for `å`), (2) ASCII approximations (`aa` for `å`, `oe` for `ø`, `ae` for `æ`), (3) HTML entities (`å` for `å`, `ø` for `ø`, `æ` for `æ`). GEN-002 mandates correct characters but automated rules only check structural i18n (page parity, link prefixes, translation drift) — diacritical correctness requires manual/AI review. Common Norwegian words to watch: må, når, på, også, både, får, bør, før, første, følger, kjører, mønster, nøkkel, verktøy, nødvendig, støtter, foreslår, påvirkning, forårsaker, primær, erklæring, miljø, overføring. +- [i18n translation quality checks](project_i18n_translation_quality.md) — nb/ + pt-br/ dual-locale requirement, Norwegian diacritical corruption patterns to scan for ## Validation Pipeline @@ -93,12 +65,10 @@ Non-enforceable lessons — environment/CI/platform quirks no static rule can re ## CLI Repo Quirk -- **`archgate` command = `bun run cli`** — This is the CLI repo itself, so the `archgate` binary is not installed in PATH. Use `bun run cli ` (e.g., `bun run cli check`, `bun run cli adr list`) instead of `archgate `. The `bun run cli` script maps to `bun run src/cli.ts`. +- **`archgate` command = `bun run cli`** — This is the CLI repo itself, so the `archgate` binary is not installed in PATH. Use `bun run cli ` instead of `archgate `. ## Distribution / Packaging -- **npm shim + GitHub Releases** — The npm package is a thin shim (`bin/archgate.cjs`). On first run, the shim downloads the platform binary from GitHub Releases and caches it to `~/.archgate/bin/`. No platform-specific npm packages. -- **`.cjs` extension is mandatory** — Root `package.json` has `"type": "module"`. Any Node.js CJS wrapper script placed at the package root MUST use `.cjs`, not `.js`, or Node.js will attempt to parse it as ESM and fail. -- [Shim publishing pipeline gotchas](project_shim_publishing.md) — PyPI README, RubyGem Rakefile/working-dir, Maven waitUntil; build reqs not caught by `archgate check` -- **Advertised version != installable version** — `docs/public/version.json` is committed in the release PR and deployed by Cloudflare on merge to main, BEFORE `release.yml` creates the GitHub release and `release-binaries.yml` uploads assets (~15-25 min gap; indefinite if the release job fails, as in the v0.44 incident). `install.sh`/`install.ps1` therefore verify the platform asset exists (HEAD request) before trusting any advertised version, and fall back to walking `releases?per_page=10` for the newest release whose asset exists. The shims (npm/pypi/go/etc.) pin version constants at release time and share this exposure — if they get the same hardening, codify the rule in ARCH-017. -- **Registering a subdir Go module on pkg.go.dev** — A subdir Go module's zip only contains files under its subtree, so the repo-root `LICENSE.md` is excluded and pkg.go.dev shows "no license" until `shims/go/LICENSE.md` exists (the shim LICENSE sync is enforced by ARCH-013). To trigger registration, hit the proxy: `curl https://proxy.golang.org//@v/.info`. +- **npm shim + GitHub Releases** — The npm package is a thin shim (`bin/archgate.cjs`) that downloads the platform binary on first run and caches it to `~/.archgate/bin/`. +- **`.cjs` extension is mandatory** for any root-level Node.js CJS wrapper — root `package.json` has `"type": "module"`, so `.js` gets parsed as ESM and fails. +- [Shim publishing pipeline gotchas](project_shim_publishing.md) — PyPI README, RubyGem Rakefile/working-dir, Maven waitUntil, advertised-vs-installable version lag, Go module registration on pkg.go.dev diff --git a/.claude/agent-memory/archgate-developer/feedback_prefer_tests_over_adr_rules.md b/.claude/agent-memory/archgate-developer/feedback_prefer_tests_over_adr_rules.md index 2b6eed85..9e37815c 100644 --- a/.claude/agent-memory/archgate-developer/feedback_prefer_tests_over_adr_rules.md +++ b/.claude/agent-memory/archgate-developer/feedback_prefer_tests_over_adr_rules.md @@ -7,7 +7,7 @@ metadata: Pick the enforcement layer by the nature of the invariant — don't default to ADR rules. -**Why:** After the inquirer v14 `"list"` → `"select"` crash, I first drafted an ARCH-019 `.rules.ts` allowlist rule — user rejected it ("making an adr rule is stupid. we should make a proper test"). I then wrote a bun test scanning source files — user rejected that too ("if we have no tty, then this is more a linting rule than proper testing"). The final shape is a custom oxlint JS plugin rule (`archgate/valid-inquirer-prompt-type` in `.archgate/lint/oxlint.ts`), which gets real AST access instead of line-scanning and runs in the existing `bun run lint` gate. +**Why:** After the inquirer v14 `"list"` → `"select"` crash, a draft ADR `.rules.ts` allowlist rule and a bun test scanning source files were both rejected by the user (an untestable-in-CI check isn't a real test; a per-file syntax check isn't an ADR governance check). The draft rule was never assigned an ADR ID — it landed instead as a custom oxlint JS plugin rule (`archgate/valid-inquirer-prompt-type` in `.archgate/lint/oxlint.ts`) — real AST access, runs in the existing `bun run lint` gate. **How to apply:** diff --git a/.claude/agent-memory/archgate-developer/project_ci_workflow_gotchas.md b/.claude/agent-memory/archgate-developer/project_ci_workflow_gotchas.md new file mode 100644 index 00000000..4ceb080b --- /dev/null +++ b/.claude/agent-memory/archgate-developer/project_ci_workflow_gotchas.md @@ -0,0 +1,10 @@ +--- +name: project-ci-workflow-gotchas +description: GitHub Actions gotchas outside the release pipeline (token permissions, config namespaces, shell encoding) +metadata: + type: project +--- + +- **`GITHUB_TOKEN`-authored pushes do NOT trigger downstream workflows.** A workflow pushing commits/PRs with `github.token`/`secrets.GITHUB_TOKEN` suppresses the resulting `push`/`pull_request` events (anti-recursion), so required PR checks never run. Fix: author such pushes with a GitHub App installation token (`actions/create-github-app-token`) on both `actions/checkout` and the pushing step. +- **`secrets.*` and `vars.*` are separate, non-overlapping namespaces** — configuring a value as one does not make it readable via the other. `release.yml`'s PostHog annotation step once read `vars.POSTHOG_PROJECT_ID` when the value only existed as a **secret**, silently no-opping for weeks behind `continue-on-error: true` + a low-visibility `::notice::`. Confirm the actual location with `gh secret list`/`gh variable list` before writing a reference. For any `continue-on-error` step with a skip-on-missing-config guard, use `::warning::` (not `::notice::`) so misconfiguration is visible in the Actions UI. +- **jq on Windows Git Bash emits CRLF line endings.** `jq -r` output carries a trailing `\r` after command-substitution newline-stripping (and mid-list entries too). Pipe through `tr -d '\r'`. Broke `install.sh`'s `resolve_version` release-walk (skipped every tag but the last). diff --git a/.claude/agent-memory/archgate-developer/project_claude_code_hooks_config.md b/.claude/agent-memory/archgate-developer/project_claude_code_hooks_config.md new file mode 100644 index 00000000..8705b5c6 --- /dev/null +++ b/.claude/agent-memory/archgate-developer/project_claude_code_hooks_config.md @@ -0,0 +1,12 @@ +--- +name: project-claude-code-hooks-config +description: How this repo's .claude/settings.json hooks work — WorktreeCreate contract and the shell:bash requirement for POSIX hooks +metadata: + type: project +--- + +`hooks.WorktreeCreate` is **not** a post-creation setup step — once configured, the harness defers the _entire_ worktree creation to it. The hook gets `{ "cwd", "name", ... }` on stdin and **must** create the worktree itself, printing _only_ the final absolute path as its last stdout line — any other stdout (unsilenced `bun install`/`git worktree add` output) gets misread as the path, breaking `EnterWorktree`/`ExitWorktree` (`path contains control characters`, `ENOENT: ... chdir`). Redirect all setup output to `>&2`. Don't simplify this back to a bare `bun install` — see [[project_worktree_create_hook_contract]] for the full bug history (5 rounds of fixes, all worth re-testing if this hook changes). + +**Command-type hooks with POSIX syntax MUST set `"shell": "bash"` explicitly**, even on Windows with Git Bash installed — hooks have a separate shell-detection path from the interactive `Bash` tool and can silently fall back to `cmd.exe` without it (symptom: `'x' is not recognized as an internal or external command...`). If Git Bash still isn't found, Claude Code checks `CLAUDE_CODE_GIT_BASH_PATH` → `C:\Program Files\Git\bin\bash.exe` (or x86) → `git` on PATH resolved to `../../bin/bash.exe`. + +**How to apply:** read this before touching any hook in `.claude/settings.json`, not just WorktreeCreate — the `"shell": "bash"` requirement applies to every command-type hook using POSIX syntax. diff --git a/.claude/agent-memory/archgate-developer/project_cli_skill_flag_sequencing.md b/.claude/agent-memory/archgate-developer/project_cli_skill_flag_sequencing.md new file mode 100644 index 00000000..ade46395 --- /dev/null +++ b/.claude/agent-memory/archgate-developer/project_cli_skill_flag_sequencing.md @@ -0,0 +1,16 @@ +--- +name: project-cli-skill-flag-sequencing +description: General rule for sequencing releases when a shipped editor skill references a CLI flag that's being added or removed +metadata: + type: project +--- + +Distributed editor skills (e.g. the opencode lessons-learned skill) reference specific `archgate` CLI flags in their instructions. When changing session-context (or any) CLI flag surface, sequence releases deliberately: + +- **Flag ADDITIONS**: ship the CLI release first. A skill referencing a flag the installed CLI lacks dies with "unknown option." +- **Flag REMOVALS** (e.g. `--skip`, removed 2026-07-02): already-installed skills still reference the dead flag and error on the new CLI. Ship the plugin release promptly after the CLI release, and keep an error-fallback path in the skill text for the gap in between. +- Don't hand-edit the installed copy under `~/.config/opencode/skills/` (or other editors' skill dirs) before the CLI release — edit the canonical source and let the plugin release distribute it. + +**How to apply:** treat "update a CLI flag referenced by a shipped skill" as a two-release coordination problem, not a single-PR change. + +**Confirmed still outstanding (2026-07-04):** the installed Claude Code `archgate:lessons-learned` skill, plugin version 0.13.1, still instructs `archgate session-context claude-code --skip 1` and still asserts the false "runs as a sub-agent with its own session file" premise — the exact issue [[project_session_context_skip_root_fix]] documents as resolved for the CLI/canonical skill source. The command fails outright (`error: unknown option '--skip'`) since the flag was removed 2026-07-02. This means the plugin release carrying the fix has not reached this installed version (or a newer plugin version exists but wasn't installed here — check `~/.claude/plugins/cache/archgate/archgate/` for available versions vs. the one actually active). If this recurs, don't just work around it locally — it means the two-release sequencing above didn't fully land for the claude-code skill variant specifically. diff --git a/.claude/agent-memory/archgate-developer/project_i18n_translation_quality.md b/.claude/agent-memory/archgate-developer/project_i18n_translation_quality.md new file mode 100644 index 00000000..6f3b6258 --- /dev/null +++ b/.claude/agent-memory/archgate-developer/project_i18n_translation_quality.md @@ -0,0 +1,9 @@ +--- +name: project-i18n-translation-quality +description: Norwegian/pt-br docs translation quality checks not covered by GEN-002's automated structural rules +metadata: + type: project +--- + +- **Docs have TWO locales: `nb/` AND `pt-br/`.** Editing any English docs page requires updating BOTH locale trees in the same changeset (GEN-002 `i18n-translation-drift` is error-severity). Locale pages can also silently lack whole sections present in English — the drift rule only checks that the file was touched, not content parity, so compare section structure manually. +- **Norwegian (nb/) diacritical corruption patterns to scan for:** (1) stripped diacriticals (`monster` for `mønster`, `a` for `å`), (2) ASCII approximations (`aa`/`oe`/`ae` for `å`/`ø`/`æ`), (3) HTML entities (`å`/`ø`/`æ`). GEN-002 only checks structural i18n, not diacritical correctness — that needs manual/AI review. Watch words: må, når, på, også, både, får, bør, før, første, følger, kjører, mønster, nøkkel, verktøy, nødvendig, støtter, foreslår, påvirkning, forårsaker, primær, erklæring, miljø, overføring. diff --git a/.claude/agent-memory/archgate-developer/project_oxlint_gotchas.md b/.claude/agent-memory/archgate-developer/project_oxlint_gotchas.md new file mode 100644 index 00000000..4b4b92db --- /dev/null +++ b/.claude/agent-memory/archgate-developer/project_oxlint_gotchas.md @@ -0,0 +1,16 @@ +--- +name: project-oxlint-gotchas +description: oxlint rule-specific gotchas and the custom jsPlugins convention hit repeatedly in this repo +metadata: + type: project +--- + +- **Custom oxlint JS plugins live in `lint/*.ts`, registered via `jsPlugins` in `.oxlintrc.json`.** `lint/expect-expect.ts` (rule `bun-test/expect-expect`) fails any runnable `bun:test` `test()`/`it()` (incl. `.skipIf()()`, `.each()()`) with no `expect()` call, because oxlint's built-in `jest/expect-expect` doesn't recognize `bun:test`. ESLint-compatible default-export shape, runs as native TS under Bun. `lint/` is excluded from `tsconfig.json` and `knip.json` but IS linted by oxlint itself, so the plugin file must pass all oxlint rules. Enable a rule only for tests via an `overrides` entry for `tests/**/*.test.ts`. Documented in ARCH-005. +- **`unicorn/no-array-callback-reference`** — don't pass a bare function reference to `.map()`/`.find()`/`.filter()`; wrap in an arrow: `args.map((x) => asNode(x))`. +- **`require-unicode-regexp`** — regex literals need the `u` flag, including in test `toThrow(/.../u)`. +- **`prefer-regexp-test`** — `Bun.Glob.match()` returns a boolean but oxlint can't tell; suppress with `// oxlint-disable-next-line prefer-regexp-test -- Bun.Glob.match() returns boolean, not RegExp`. +- **`no-negated-condition`** — write ternaries/`if-else` with the positive condition first: `x === null ? A : B`, not `x !== null ? B : A`. +- **`no-unused-vars` on catch params** — use bare `catch { }` when the error is unused, not `catch (err) { }`. +- **`no-await-in-loop`** — sequential `await` in a `for` loop is flagged; suppress with a reason comment when the sequential order is intentional. +- **ARCH-020's `glob-scan-dot` rule matches `.scan()` inside comments too** (regex `/\.scan\(([^)]*)\)/gu`) — rephrase comments to avoid the literal `.scan()` text. +- **`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. diff --git a/.claude/agent-memory/archgate-developer/project_pr_review_thread_triage.md b/.claude/agent-memory/archgate-developer/project_pr_review_thread_triage.md new file mode 100644 index 00000000..6fc83b99 --- /dev/null +++ b/.claude/agent-memory/archgate-developer/project_pr_review_thread_triage.md @@ -0,0 +1,33 @@ +--- +name: project-pr-review-thread-triage +description: How to distinguish already-resolved vs genuinely outstanding PR review comments — REST API doesn't expose resolution state +metadata: + type: project +--- + +**`gh api repos///pulls//comments` (REST) does NOT expose whether a review comment thread is resolved.** A stale CodeRabbit/reviewer comment from an earlier commit stays in that endpoint's output forever, indistinguishable from a live, unaddressed one — reading it naively re-litigates already-fixed findings. + +**Fix: use the GraphQL `reviewThreads` field**, which has `isResolved` and `isOutdated`: + +```bash +gh api graphql -f query=' +query { + repository(owner: "OWNER", name: "REPO") { + pullRequest(number: N) { + reviewThreads(first: 50) { + nodes { + isResolved + isOutdated + path + line + comments(first: 5) { nodes { author { login } body createdAt } } + } + } + } + } +}' +``` + +Filter to `isResolved: false` for what actually still needs addressing. `isOutdated: true` alone does NOT mean resolved — a thread can be outdated (the line moved) but still unresolved if nobody marked it fixed. + +**How to apply:** before acting on "there are still outstanding comments," run this query first. Don't re-fix findings a prior commit already addressed, and don't miss ones marked outdated-but-unresolved. diff --git a/.claude/agent-memory/archgate-developer/project_release_pipeline_gotchas.md b/.claude/agent-memory/archgate-developer/project_release_pipeline_gotchas.md index 6bf62908..05810fe6 100644 --- a/.claude/agent-memory/archgate-developer/project_release_pipeline_gotchas.md +++ b/.claude/agent-memory/archgate-developer/project_release_pipeline_gotchas.md @@ -5,12 +5,12 @@ metadata: type: project --- -Three distinct issues caused shim/binary publishing failures across v0.45.1–v0.46.1, fixed in `fix/publish-shims-race`: +Four distinct issues caused shim/binary publishing failures across v0.45.1–v0.46.1, fixed in `fix/publish-shims-race`: 1. **Workflow race**: `publish-shims.yml` and `release-binaries.yml` both triggered on `release: published`. If binaries needed a retry, `publish-shims.yml`'s fixed-budget `wait-for-binaries` poll timed out and went `cancelled` (terminal) before the retry finished. Fixed: `publish-shims.yml` is now `workflow_dispatch`-only; `release-binaries.yml`'s `trigger-shim-publish` job dispatches it via `gh workflow run` after binaries + provenance succeed. 2. **`moonrepo/setup-toolchain` cache bug**: right after a `.prototools` bump, the first macOS/Windows CI run can restore a stale `restore-key` cache fallback instead of an exact hit — the action reports success but `bun` isn't on PATH. Log signature: `Cache hit for restore-key:` (vs. exact `Cache hit for:`). Self-heals on retry (the failing job still saves a fresh exact-key cache in post-job cleanup). No code fix — just don't chase it as flakiness. -3. **Update-check stdout pollution**: `src/cli.ts` printed a background "update available" notice to stdout after every command, unconditionally. Broke `JSON.parse(stdout)` in CLI-subprocess tests (and would break real `| jq` usage) whenever a newer release existed. Fixed via `shouldPerformUpdateCheck()` in `src/helpers/update-check.ts` — only checks in a genuine TTY, non-CI session. -4. **`publish-go-tag` missing `workflows: write`**: its `git push origin shims/go/$TAG` was rejected — GitHub blocks any ref push reachable through commits touching `.github/workflows/*` without that permission, even for an unrelated tag. Fixed by adding `workflows: write` to the job's `permissions:`. Confirmed on v0.45.7's job log. +3. **Update-check stdout pollution**: `src/cli.ts` printed a background "update available" notice to stdout after every command, unconditionally. Broke `JSON.parse(stdout)` in CLI-subprocess tests (and would break real `| jq` usage) whenever a newer release existed. Fixed via `shouldPerformUpdateCheck()` in `src/helpers/update-check.ts` — only checks in a genuine TTY, non-CI, non-`upgrade` session. +4. **`publish-go-tag`'s `git push origin shims/go/$TAG` was rejected** with "refusing to allow a GitHub App to create or update workflow ... without workflows permission" (GitHub blocks ref pushes reachable through commits touching `.github/workflows/*`, even for an unrelated tag) on v0.45.7. **Correction (2026-07-03):** the original fix — adding `workflows: write` to the job's `permissions:` — does nothing: `workflows` is not a valid `permissions:`-key scope (confirmed against GitHub's own workflow syntax docs and actionlint's schema), so it was silently ignored rather than granting anything. Removed the invalid key; `contents: write` alone remains. Whether the underlying push rejection is actually resolved is **unverified** — if it recurs, the real fix needs a PAT with the classic `workflow` OAuth scope (as a secret) or a GitHub App installation token whose App has the Workflows permission explicitly configured, since neither is expressible via the workflow's own `permissions:` key. Watch the next real release's `publish-go-tag` job log. **Why:** Diagnosed while investigating why v0.46.0/v0.46.1 kept failing to publish despite repeated manual retries. -**How to apply:** If a release build fails and a retry "just works," check which of these three it was before assuming generic flakiness. +**How to apply:** If a release build fails and a retry "just works," check which of these four it was before assuming generic flakiness. diff --git a/.claude/agent-memory/archgate-developer/project_rules_engine_internals.md b/.claude/agent-memory/archgate-developer/project_rules_engine_internals.md new file mode 100644 index 00000000..23f2b4f9 --- /dev/null +++ b/.claude/agent-memory/archgate-developer/project_rules_engine_internals.md @@ -0,0 +1,11 @@ +--- +name: project-rules-engine-internals +description: Gotchas in the ADR rules engine and command-option parsing internals (glob, commander, code sharing, review verification) +metadata: + type: project +--- + +- **`Bun.Glob.scan()` silently fails for brace patterns with path separators.** `new Bun.Glob("svc/{src/env.ts,env.ts}").scan(...)` returns zero results (no error) while `.match()` works — the scanner wasn't updated when the match engine's brace expansion was rewritten in Bun 1.2.3. Filed upstream: [oven-sh/bun#32596](https://github.com/oven-sh/bun/issues/32596). Workaround: `expandBracePattern()` in `src/engine/runner.ts` pre-expands `/`-containing brace groups before scanning; applied in `ctx.glob()`, `ctx.grepFiles()`, `resolveScopedFiles()`. +- **Commander hoists parent-known options away from nested subcommands.** If a parent and child subcommand declare the same option (e.g. `session-context ` and ` show` both taking `--max-entries`), commander parses the flag onto the PARENT regardless of argv position — the child's `opts` silently gets `undefined`. Read merged values via `command.optsWithGlobals()` (third action param) and add a regression test through the full `parseAsync` path. Shipped broken in v0.46.0, fixed in PR #448. +- **Cross-command I/O sharing: export from the existing command file, don't create a new shared file.** ARCH-002 forbids `console.log` in helpers; ARCH-001/ARCH-016 require a `register*Command` export + docs heading for any new command file. Correct pattern: export the shared functions from the command file that already defines them (e.g. `plugin/install.ts` exports `installForEditor()`/`printManualInstructions()`) and import them elsewhere. Applied in `upgrade.ts`. +- **Verify a reviewer sub-agent's ADR citation against the ADR's actual text before blocking.** A haiku review agent flagged "await on a synchronous helper" as an ARCH-012 violation; ARCH-012 only mandates try-catch boundaries/exit codes, and automated rules passed. Re-read the cited ADR's Decision/Do's before accepting a FAIL. A finding citing no ADR (self-labeled "ARCH-NONE") can never block — recurred 2026-07-01 with the same nit. diff --git a/.claude/agent-memory/archgate-developer/project_session_context_skip_root_fix.md b/.claude/agent-memory/archgate-developer/project_session_context_skip_root_fix.md index 969bfe30..f14e6c9c 100644 --- a/.claude/agent-memory/archgate-developer/project_session_context_skip_root_fix.md +++ b/.claude/agent-memory/archgate-developer/project_session_context_skip_root_fix.md @@ -1,18 +1,16 @@ --- name: project-session-context-skip-root-fix -description: session-context --skip 1 inline-skill bug — opencode fixed 2026-07-01 (top-level default + --root); claude-code/cursor/copilot guidance fixed 2026-07-02 (plain command; --skip 1 premise was false everywhere) +description: session-context --skip was removed entirely (2026-07-02) after two rounds of bugs — replaced with per-editor list/show subcommands metadata: type: project --- -Fixed a real, reproduced bug in `archgate session-context opencode --skip 1`: it could return a completely unrelated sibling sub-agent's session transcript instead of the true parent/development session. Fix shipped 2026-07-01 (see `src/helpers/session-context-opencode.ts` and `src/commands/session-context/opencode.ts`): recency selection now only considers **top-level sessions** (`parent_id IS NULL`) by default, and a new `--root` flag walks the `parent_id` chain from a `--session-id` child session up to its top-level ancestor (cycle-guarded). Without `--session-id`, `--root` is an explicit alias for the new default. +**Current shape (final, 2026-07-02):** `--skip` was removed from all four editor subcommands. Explicit selection instead: `archgate session-context list` / `show `. `--root` exists only on `opencode show` (resolves a child session to its top-level ancestor via `parent_id`; opencode `list` is top-level-only). Bare editor subcommands take only `--max-entries` and always read the current conversation. -**Why:** `readOpencodeSession`'s `--skip N` selected the Nth-most-recently-updated session sharing a project directory, ignoring opencode's real `session.parent_id` column entirely. This breaks in two ways: (1) the opencode `Skill` tool runs inline in the current session (no new session row), so "skip past my own session" skips past the actual parent instead; (2) sibling sub-agent sessions fanned out from the same parent (e.g. the `archgate:reviewer` skill's parallel domain-review agents) interleave in recency order with the parent, so `--skip 1` can land on any sibling. Verified this concretely against the user's real `~/.local/share/opencode/opencode.db`: a live `archgate-lessons-learned` skill invocation's `--skip 1` call returned the "General process ADR review" sub-agent's private transcript instead of the parent session. +**Why it got here:** `--skip N` originally picked the Nth-most-recently-updated session sharing a project directory, ignoring opencode's real `parent_id` column. Two independent bugs: (1) opencode's `Skill` tool runs inline (no new session row), so "skip past my own session" skipped the actual parent instead, and sibling sub-agent sessions (e.g. `archgate:reviewer`'s parallel domain agents) could interleave ahead of it in recency order — reproduced against a real `opencode.db` (a lessons-learned skill run returned an unrelated sub-agent's transcript). Fixed 2026-07-01 via top-level (`parent_id IS NULL`) filtering by default + `--root`. (2) The SAME `--skip 1` guidance was then found wrong for claude-code/cursor/copilot too, but for a different reason: skills run inline there as well, and Agent-tool sub-agents don't write their own session files at all — so the plain command (skip 0) was always correct for those editors; no `parent_id`-style fix was needed. Since the flag's only purpose was this false premise, it was removed everywhere rather than fixed per-editor. -Top-level filtering fixes the common case regardless of nesting depth or sibling count; `--session-id --root` gives deterministic ancestry resolution when the caller knows a session inside the right conversation tree (relevant when several top-level sessions share a directory — recency alone can pick a different conversation's root). The distributed archgate editor plugin's opencode lessons-learned skill was updated to use `--root` instead of `--skip 1`. +**Remaining caveat (all editors):** with several concurrent conversations for the same project, most-recent-by-mtime can pick the wrong live conversation. `opencode show --session-id --root` is deterministic; claude-code/cursor have no equivalent linkage. -**Resolved for the other editors (2026-07-02):** The `--skip 1` guidance was wrong for ALL editors, not just opencode, but for a different reason — the premise "this skill runs as a sub-agent with its own session file" is simply false. Verified for Claude Code on real data: (1) skills run inline, so the current conversation is the most recent session file and `--skip 1` reads an unrelated previous conversation (live-reproduced: got an unrelated "ExitWorktree" session); (2) Agent-tool sub-agents do NOT write session files into `~/.claude/projects//` (4 sub-agents spawned, zero new .jsonl files) and modern sessions contain zero `isSidechain: true` entries — so even from a sub-agent, the most recent project session is the main conversation. Conclusion: the plain command (skip 0) is correct in every case; no `parent_id`-style CLI fix is needed for Claude Code. Cursor/Copilot skill variants had the same false guidance (inherited from the canonical claude-code SKILL.md source); their sub-agent storage behavior is unverified but the plain command + transcript sanity check is the right default there too. Fixed at two levels: the distributed lessons-learned skill's canonical source now instructs the plain command, and — since the flag's only advertised purpose was this false premise — `--skip` was REMOVED from all four subcommands entirely (2026-07-02, user decision). Explicit selection replaced it as verbs nested under each editor (user decision): `archgate session-context list` and `archgate session-context show ` (`--root` exists only on `opencode show`, resolving a child to its top-level ancestor; opencode list is top-level sessions only). The bare editor subcommands take only `--max-entries` and always read the current conversation. +**Inspecting real opencode data:** the live `opencode.db` can't be opened `readonly: true` while opencode runs (`SQLITE_CANTOPEN`). Copy `opencode.db` + `.db-wal` + `.db-shm` to a temp dir first. -Remaining caveat (all editors): when several conversations for the same project run concurrently, most-recent-by-mtime can pick the other live conversation — for opencode, `--session-id --root` is deterministic; claude-code/cursor have no equivalent linkage. - -**How to apply:** If asked to investigate "session context returns wrong data" for claude-code, cursor, or copilot, start here — this is the same architectural flaw already fixed once for opencode, not a new mystery. +**How to apply:** if "session context returns wrong data" comes up again for claude-code, cursor, or copilot, start here — same class of flaw, already resolved once. See also [[project_cli_skill_flag_sequencing]] for the release-sequencing rule this fix's `--skip` removal triggered. diff --git a/.claude/agent-memory/archgate-developer/project_shim_publishing.md b/.claude/agent-memory/archgate-developer/project_shim_publishing.md index 73af813a..d6e6aa60 100644 --- a/.claude/agent-memory/archgate-developer/project_shim_publishing.md +++ b/.claude/agent-memory/archgate-developer/project_shim_publishing.md @@ -9,8 +9,12 @@ metadata: **When editing any shim under `shims/` or `publish-shims.yml`:** -- **PyPI** (`shims/pypi/`): `pyproject.toml` declares `readme = "README.md"`, so `shims/pypi/README.md` MUST exist or the build fails with `OSError: Readme file does not exist`. The job builds with **`uv build --python 3.12`** (via `astral-sh/setup-uv`, SHA-pinned) — uv provisions its own version-pinned, isolated build env, so there is no `pip install build` line for Scorecard Pinned-Dependencies to flag. Do NOT reintroduce the `pip install build==X --hash=...` form: **`--hash` is NOT a valid `pip install` command-line option** (only valid inside a requirements file), so it fails with `no such option: --hash` — that broke the v0.41.0 release (introduced unverified in #361 since the workflow only runs at release time). -- **RubyGem** (`shims/rubygem/`): `rubygems/release-gem` runs `bundle exec rake release` from its `working-directory`. Requires (1) `working-directory: shims/rubygem` on BOTH `ruby/setup-ruby` (with `bundler-cache: true`) and `rubygems/release-gem`; (2) a `shims/rubygem/Rakefile` with `require "bundler/gem_tasks"` for the `release` task; (3) **`gem "rake"` declared in `shims/rubygem/Gemfile`** — Ruby 4.0 no longer ships rake as a bundled default gem, so `bundle exec rake` fails with `rake is not currently included in the bundle` (broke the v0.41.0 release when the runner moved to Ruby 4.0.5). Do NOT commit `Gemfile.lock` — bundler-cache generates it untracked, keeping `release:guard_clean` happy. +- **PyPI** (`shims/pypi/`): `pyproject.toml` needs `shims/pypi/README.md` to exist (`readme = "README.md"`) or the build fails with `OSError: Readme file does not exist`. Builds via `uv build --python 3.12` (isolated, version-pinned env — no `pip install build` line to flag). Never reintroduce `pip install build==X --hash=...`: `--hash` isn't a valid `pip install` CLI flag (requirements-file only); broke v0.41.0. +- **RubyGem** (`shims/rubygem/`): needs `working-directory: shims/rubygem` on both `ruby/setup-ruby` and `rubygems/release-gem`, a `Rakefile` requiring `bundler/gem_tasks`, and `gem "rake"` in the `Gemfile` (Ruby 4.0 dropped rake as a default gem; broke v0.41.0 on the Ruby 4.0.5 runner bump). Don't commit `Gemfile.lock` — bundler-cache generates it untracked. - **Maven** (`shims/maven/pom.xml`): use `validated` with `true`, NOT `published` — the latter blocks until Sonatype finishes publishing, which routinely exceeds the job timeout (upload succeeds, then the build hangs on "Waiting until Deployment ... is published"). **Re-runs are not idempotent:** `publish-go-tag` (creates a git tag), `publish-nuget`, and an already-uploaded Maven deploy fail on "already exists" on a second run. After a partial failure, apply the fix to the next version bump or `workflow_dispatch` only the failed ecosystems. + +**Advertised version can lag the installable version.** `docs/public/version.json` deploys on merge to main BEFORE `release.yml`/`release-binaries.yml` finish creating the release and uploading assets (~15-25 min gap, longer if the release job fails, as in the v0.44 incident). `install.sh`/`install.ps1` verify the platform asset exists (HEAD request) before trusting the advertised version, falling back to walking `releases?per_page=10`. The shims pin version constants at release time and share this exposure — see ARCH-017 if hardening them. + +**Registering a subdir Go module on pkg.go.dev:** a subdir module's zip only contains files under its subtree, so the repo-root `LICENSE.md` is excluded and pkg.go.dev shows "no license" until `shims/go/LICENSE.md` exists (enforced by ARCH-013). Trigger registration via `curl https://proxy.golang.org//@v/.info`. diff --git a/.claude/agent-memory/archgate-developer/project_test_isolation_gotchas.md b/.claude/agent-memory/archgate-developer/project_test_isolation_gotchas.md new file mode 100644 index 00000000..35da6b87 --- /dev/null +++ b/.claude/agent-memory/archgate-developer/project_test_isolation_gotchas.md @@ -0,0 +1,14 @@ +--- +name: project-test-isolation-gotchas +description: Bun test isolation pitfalls — mock.module leakage, shared env across test files, platform-specific flakiness +metadata: + type: project +--- + +- **`mock.module` state is process-global across test files.** A helper mock registered in one file leaks into other files' imports of the same module. When a command test needs REAL helper behavior while sibling files mock those helpers, spawn the CLI via `tests/integration/cli-harness`'s `runCli` with `HOME`/`USERPROFILE`/`XDG_DATA_HOME` redirected to a temp dir instead. This is why ARCH-005 prefers `spyOn` over `mock.module`. Hit in session-context list/show tests (PR #446). +- **`Bun.env` overrides leak across parallel test files.** Bun test runner shares one process, so tests setting `Bun.env.HOME`/`GIT_CONFIG_NOSYSTEM`/`GIT_CONFIG_GLOBAL` leak into integration tests that spawn CLI subprocesses via `runCli()` (which spreads `process.env`). Fix: explicitly reset git-related env vars in the `runCli` call, e.g. `runCli(args, dir, { GIT_CONFIG_NOSYSTEM: "", GIT_CONFIG_GLOBAL: "" })`. +- **Git credential tests need system-level isolation on Windows.** Overriding `Bun.env.HOME` is not enough — Windows Credential Manager is a system API, not file-based. Set `GIT_CONFIG_NOSYSTEM=1` and `GIT_CONFIG_GLOBAL=` or tests on machines with stored credentials will pick up real tokens. +- **GCM prompt suppression needs 5 env vars**, not just `GIT_TERMINAL_PROMPT=0`: also `GCM_INTERACTIVE=never`, `GCM_GUI_PROMPT=false`, `GIT_ASKPASS=""`, `SSH_ASKPASS=""` (see `gitCredentialEnv()` in `src/helpers/credential-store.ts`). +- **`bun:sqlite` file handles persist after `db.close()` on Windows.** `rmSync` on the temp dir in `afterEach` can throw `EBUSY`. Set `PRAGMA journal_mode = DELETE` to avoid WAL/SHM files, and wrap `rmSync` in try/catch. +- **macOS `/var` → `/private/var` symlink breaks temp dir path comparisons.** `mkdtempSync` returns `/var/folders/...` but `process.cwd()` after `chdir()` resolves to `/private/var/...`. Always wrap with `realpathSync`. Invisible on ubuntu-only PR CI — only surfaces in release builds. +- **Don't test that well-known tools exist on PATH** (e.g. `expect(resolveCommand("bun")).toBe("bun")`) — asserts CI environment state, not logic, and fails when tools are installed via shims. Delete such tests; the null-return and `.exe`-fallback tests already cover the real logic. diff --git a/.claude/agent-memory/archgate-developer/project_windows_subprocess_gotchas.md b/.claude/agent-memory/archgate-developer/project_windows_subprocess_gotchas.md new file mode 100644 index 00000000..418d3f1f --- /dev/null +++ b/.claude/agent-memory/archgate-developer/project_windows_subprocess_gotchas.md @@ -0,0 +1,11 @@ +--- +name: project-windows-subprocess-gotchas +description: Windows-specific subprocess, path, and env-capture gotchas outside the test suite +metadata: + type: project +--- + +- **Git Bash `/tmp` is invisible to Windows-native tools.** A bash redirect to `/tmp/x` writes into Git Bash's virtual mount; Windows-native python/node then get `FileNotFoundError` on that path. Use a repo-relative path or `$TMPDIR` when piping files between bash and Windows-native tools. +- **YAML double-quoted strings need escaped backslashes for Windows paths.** `cwd: "E:\project"` silently corrupts (`\p` isn't a valid escape). Use `JSON.stringify(path)` to produce correctly escaped YAML (JSON and YAML double-quoted escaping match). Hit in Copilot CLI session-context tests (`workspace.yaml`). +- **Windows binary upgrade: never use detached child processes for `.old` cleanup.** `replaceBinary()` renames the locked running exe to `.old`; a detached `cmd /c ping ... & del` cleanup is unreliable (spawn/timing races). `cleanupStaleBinary()` instead does a fire-and-forget `unlink()` at next CLI startup, when the file is guaranteed unlocked. Platform-agnostic via `getArtifactInfo()`. Do not reintroduce detached cleanup processes. +- **Module-level `{ ...Bun.env }` captures env at import time.** Spreading into a module constant freezes the snapshot; test-time `Bun.env.HOME` overrides won't affect it. Use a function returning `{ ...Bun.env, ... }` per call instead. Applied in `src/helpers/credential-store.ts`. diff --git a/.claude/agent-memory/archgate-developer/project_worktree_create_hook_contract.md b/.claude/agent-memory/archgate-developer/project_worktree_create_hook_contract.md index 2f998e20..b156c70b 100644 --- a/.claude/agent-memory/archgate-developer/project_worktree_create_hook_contract.md +++ b/.claude/agent-memory/archgate-developer/project_worktree_create_hook_contract.md @@ -1,35 +1,20 @@ --- name: project-worktree-create-hook-contract -description: How the Claude Code WorktreeCreate hook contract works — stdin JSON in, path-only stdout out — and how the repo's hook was broken/fixed +description: WorktreeCreate hook contract (stdin JSON in, path-only stdout out) and the 5 rounds of bugs found fixing it metadata: type: project --- -`.claude/settings.json`'s `hooks.WorktreeCreate` hook was broken from ~Feb 2026 (or earlier) until fixed on 2026-07-01: it only ran `bun install`, which is not enough — once a `WorktreeCreate` hook is configured, the harness defers the **entire** worktree creation to it (no automatic git worktree creation happens in parallel). Symptom reported by the user: `WorktreeCreate hook failed: path contains control characters`; empirically also produced `ENOENT: ... chdir 'E:\archgate\cli\' -> ''`. +Once `hooks.WorktreeCreate` is configured in `.claude/settings.json`, the harness defers the **entire** worktree creation to it — no parallel automatic git worktree creation happens. Contract: hook receives `{"cwd","name","session_id",...}` on **stdin**, and must print ONLY the final absolute worktree path as its last stdout line — any other stdout (unsilenced `bun install`/`git worktree add` banners) gets misread as the path and corrupts session state. Redirect all setup output to `>&2`. -**Why:** Diagnosed by dumping the hook's stdin/env/args to a side-channel file (`{ echo ...; cat; env; } > /tmp/debug.txt; pwd`) and by calling `EnterWorktree` directly to observe raw harness errors. Found: (1) the hook receives a JSON payload on **stdin** — `{"cwd", "name", "session_id", ...}` — same pattern as the existing `PostToolUse` hook that reads `.tool_input.file_path` via `jq`; (2) the harness error `hook succeeded but returned no worktree path (command: echo the path to stdout; ...)` reveals the hook **must** print only the resulting absolute worktree path as its final stdout line; (3) any other stdout (unsilenced `bun install`, `git worktree add` banner text) gets misread by the harness as the path, corrupting session state (`ExitWorktree` later reported removing a worktree "at Checked 177 installs... [28ms]" — the literal bun install stdout). +See [CLAUDE.md](../../../CLAUDE.md) "Claude Code Harness Config" for the current, correct hook implementation. Do not simplify it back to a bare `bun install` — that regression is exactly what broke it originally (21 stale empty worktree dirs found from the old broken version). -**Evidence of prior breakage:** `.claude/worktrees/` had 21 stale, completely empty leftover directories (no `.git` file, no content) alongside orphaned local branches like `claude/wonderful-bose-0f7e45` — confirming `git worktree add` used to run (via some earlier, more complete hook version) but the directory was left behind when path corruption broke cleanup. +**Bugs found fixing it (all true positives, all confirmed by reproduction before fixing — reproduce Bugbot/reviewer findings against the real script, don't just reason about the code):** -**How to apply:** The fixed hook (see `CLAUDE.md` "Claude Code Harness Config" section) parses `.name` from stdin via `jq`, creates `git worktree add "$CLAUDE_PROJECT_DIR/.claude/worktrees/$name" -B "claude/$name"`, redirects ALL setup command output to stderr (`>&2`), and ends with `printf '%s\n' "$dir"` as the only real stdout. Verified end-to-end via `EnterWorktree`/`ExitWorktree` — worktree created with full file content + `node_modules`, session cwd matched, clean removal. Note: `git worktree remove` does not delete the branch — `claude/` branches accumulate locally over time; periodic `git branch -D` cleanup of merged/abandoned `claude/*` branches is a reasonable manual chore, not automated by anything. +1. Stale non-worktree leftover dir at the target path silently skipped `git worktree add`. Fix: check for `"$dir/.git"`, `rm -rf` first if it's a stale non-worktree dir. +2. `jq -r` can emit CRLF on Windows Git Bash; the trailing `\r` alone reproduces "path contains control characters." Fix: `tr -d '\r'` on `name`. (`cat -A` or byte-length comparison reliably detects an embedded `\r`; `grep -q '\r'` against `od -c` output gives false positives.) +3. `bun install --silent` failures were silently swallowed (no exit check). Deliberately NOT hard-failed — by then the worktree may be a reused one with real uncommitted work, so hard-fail-without-cleanup risks orphaning dirs again, and cleanup risks deleting real work on a transient failure. Fix: explicit `warning: bun install failed...` to stderr instead. +4. A worktree whose directory was deleted without `git worktree remove` leaves git's internal registration (`prunable`) behind, so `git worktree add` at the same path/branch later fails with `fatal: '' is already used by worktree`. Fix: `git worktree prune >&2` immediately before `git worktree add`. +5. (2026-07-01, different machine, user-reported) Hook fell back to cmd.exe despite Git Bash being on PATH and the interactive `Bash` tool correctly using it — hooks have a separate, independently-implemented shell-detection path from the interactive tool. Fix: set `"shell": "bash"` explicitly on the hook object; don't rely on autodetection when mixing POSIX syntax with Windows. Without it, Claude Code resolves Git Bash via `CLAUDE_CODE_GIT_BASH_PATH` → `C:\Program Files\Git\bin\bash.exe` (or x86) → `git` on PATH resolved to `../../bin/bash.exe`, in that order. -If this hook is ever "simplified" back to a bare `bun install`, the exact same bug returns. - -**Round 2 (same PR, caught by Cursor Bugbot on [PR #441](https://github.com/archgate/cli/pull/441)):** My first fix had two more bugs, both confirmed by direct reproduction before fixing: - -1. **Stale-dir skip** — `if [ ! -d "$dir" ]` skipped `git worktree add` whenever the target path already existed, even as an empty non-worktree leftover (exactly the kind of debris this same PR documents finding 21 of). Repro: `mkdir -p .claude/worktrees/x; echo '{"name":"x"}' | bash -c "$HOOK_CMD"` produced exit 0 and a printed path, but no real checkout. Fix: check for `"$dir/.git"` and `rm -rf "$dir"` first if it's a stale non-worktree directory, before the create-if-missing check. -2. **CRLF in `jq -r` output** — on Windows Git Bash, `jq -r` can emit CRLF; `$(...)` command substitution strips only the trailing `\n`, leaving a `\r` in `name`/`dir`/the final stdout path — reproducing the exact "path contains control characters" bug this PR set out to fix. This is the same pattern already documented under "jq on Windows Git Bash emits CRLF line endings" in the top-level MEMORY.md (bit `install.sh` before). Repro: wrapped `jq` in a PATH-shadowing stub that appends `\r` to its output, ran the hook end-to-end, verified via `cat -A`/byte-length comparison (not naive `grep '\r'` — that gave a false positive against `od -c` text output). Fix: `name=$(jq -r '.name // empty' | tr -d '\r')`. - -**Lesson: reproduce Bugbot/reviewer findings against the actual script before trusting or dismissing them.** Both were true positives, verified by running the real hook command through simulated failure conditions (pre-existing stale dir; CRLF-emitting jq stub) rather than just reasoning about the code. One dead end worth noting: `grep -q '\r'` against `od -c` output is NOT a reliable way to detect an embedded carriage return byte (od's own text formatting can produce a false match) — use `cat -A` (shows `^M`) or compare `wc -c` byte length to the expected clean string instead. - -**Round 3 (same PR, Bugbot re-ran on the round-2 fix commit):** Flagged that `bun install --silent` failures were silently swallowed — no `|| exit 1` check, so the hook always ended with `printf` and exit 0 regardless of install outcome. Repro: created a worktree, corrupted its `package.json` to force a parse error, re-ran the hook — confirmed exit 0 despite `bun install`'s own error printing to stderr. **Deliberately did not hard-fail** (`|| exit 1`) here, unlike the `git worktree add` step: by the time `bun install` runs, the git worktree already exists and may be a _reused_ existing worktree with real uncommitted work in it (not just a fresh one) — hard-failing without cleanup would orphan directories again (round-2's bug #1), and adding cleanup would risk deleting real work on a transient `bun install` failure (e.g., a network blip) if the worktree was being reused rather than freshly created. Instead added an explicit `warning: bun install failed in $dir -- ...` line to stderr on failure (`bun install --silent >&2 || echo "warning: ..." >&2`), verified it appears via the same corrupt-package.json repro, and verified the happy path produces no spurious warning. This is a case of fixing the underlying "silently swallowed" problem the finding correctly identified, while explicitly disagreeing with the literal "hook should fail" framing — explained the reasoning in the reply rather than just complying. - -**Round 4 (same PR, Bugbot re-ran again):** Flagged that `git worktree add` runs without a preceding `git worktree prune`, so if a worktree's directory was ever deleted without going through `git worktree remove` (crash, manual `rm -rf`, disk cleanup) git still has it registered internally (shows as `prunable` in `git worktree list`) and `git worktree add` at that path/branch fails outright with `fatal: '' is already used by worktree at ''` — even though `[ ! -d "$dir" ]` is true (directory is gone) and my round-2 stale-dir check doesn't catch it either (that check only fires when the directory _exists_ but lacks `.git`; here the directory doesn't exist at all, only git's internal metadata does). Repro: created a worktree, `rm -rf`'d just the directory (not `git worktree remove`), re-ran the hook with the same name — confirmed exit 1 with that exact fatal error. Fix: `git worktree prune >&2` immediately before `git worktree add`, cheap and safe to run unconditionally every time. Verified the repro now succeeds, plus regression-tested round-2's stale-dir case and the plain happy path still work. - -**Running tally: 4 rounds of Bugbot findings on one hook, all true positives, all found by actually reproducing before fixing.** This hook is deceptively easy to get subtly wrong — small shell script, but git worktree state has more failure modes (missing dir, stale dir, stale git registration, failed dep install) than are obvious up front. If touching this hook again, re-run all four repros above as a regression suite before pushing, not just the happy path. - -**Round 5 (2026-07-01, different machine, reported directly by the user, not Bugbot):** `WorktreeCreate hook failed: name=$(jq -r '.name // empty' | tr -d '\r'); ...: 'name' is not recognized as an internal or external command, operable program or batch file.` That exact phrasing is cmd.exe's own stderr text (PowerShell's equivalent says "is not recognized as the name of a cmdlet, function, script file, or operable program"), and cmd.exe tokenizes on `=` — explaining why it complained about bare `name` rather than `name=$(jq`. Confirmed via `where bash` that Git Bash was installed and on `PATH`, and confirmed via the project's own `Bash` tool description that it uses Git Bash — yet the **hook** runner still fell back to cmd.exe. Root cause (confirmed by extracting and grepping the real shipped `claude.exe` binary for source strings, not guessing): the hook object had no `"shell"` field, and on this machine the platform-default resolution for hooks specifically did not land on bash the way the interactive Bash tool's own Windows detection does — the two have separate detection paths inside the CLI. The binary's source strings confirmed hooks support an explicit `"shell": "bash" | "powershell"` field, and that when `shell` resolves to anything other than `"powershell"`, Claude Code resolves a Git Bash path via, in order: (1) `CLAUDE_CODE_GIT_BASH_PATH` env var (hard-errors if set but invalid), (2) hardcoded `C:\Program Files\Git\bin\bash.exe` / `C:\Program Files (x86)\Git\bin\bash.exe`, (3) `git` resolved from `PATH` with `bash.exe` derived as `/../../bin/bash.exe`. If none resolve, it throws a distinct `Hook "..." requires bash but Git Bash was not found` error (different from the cmd.exe fallback the user actually saw) — so that explicit-shell path is not what silently failed; the _default resolution when `shell` is unset_ is what picked cmd.exe on this box. - -**Fix:** add `"shell": "bash"` directly to the hook object in `.claude/settings.json` (sibling of `"command"`/`"timeout"`). **Verified by actually triggering the hook** — not just reasoning about the binary strings — via `EnterWorktree({name: "hook-shell-fix-test"})`, confirmed a real git worktree with `.git` file + `package.json` + correct branch via `git worktree list`, then `ExitWorktree({action: "remove", discard_changes: true})` and `git branch -D` to clean up the leftover branch (per the existing note above, `ExitWorktree`/`git worktree remove` never deletes the branch). - -**Lesson:** never assume a documented "sane default" (e.g. "defaults to bash when Git Bash is installed") actually applies uniformly across every code path in the same CLI — the interactive `Bash` tool and the `hooks` runner turned out to have independently-implemented Windows-shell detection in this binary, and only one of them worked correctly on this machine. When a hook mixes POSIX-only syntax (`$(...)`, `[ -z ]`, `&&`, `;`) with Windows, set `"shell": "bash"` explicitly rather than relying on autodetection — it's one extra JSON field and removes an entire class of platform-dependent failure. See also [CLAUDE.md](../../../CLAUDE.md) "Claude Code Harness Config" for the user-facing version of this note. +**Lesson:** `git worktree remove` never deletes the branch — `claude/` branches accumulate locally; periodic `git branch -D` cleanup of merged/abandoned ones is a manual chore. If touching this hook again, regression-test all 5 repros above, not just the happy path — small script, but more git-worktree-state failure modes than are obvious up front. diff --git a/.claude/agent-memory/archgate-developer/reference_cursor_approval_agent.md b/.claude/agent-memory/archgate-developer/reference_cursor_approval_agent.md index 27d2b3a4..3ade11f2 100644 --- a/.claude/agent-memory/archgate-developer/reference_cursor_approval_agent.md +++ b/.claude/agent-memory/archgate-developer/reference_cursor_approval_agent.md @@ -1,14 +1,12 @@ --- name: reference-cursor-approval-agent -description: PRs go through an external Cursor.com "Archgate CLI Approver" automation not implemented anywhere in this repo — where to look when its approval fails +description: PRs go through an external Cursor.com "Archgate CLI Approver" automation, not implemented anywhere in this repo metadata: type: reference --- -PRs in `archgate/cli` are evaluated by a custom **Cursor Automation** named "Archgate CLI Approver" (visible on PR checks as "Cursor Approval Agent: Archgate CLI Approver", linked to `https://cursor.com/automations/`). It is entirely external/hosted on cursor.com — **not** implemented as a GitHub Actions workflow or any file in this repository. Confirmed by exhaustive search: no `APPROVAL_POLICY.md`, no `cursor/approval-policies/ROUTING.md`, no `.cursor/` policy files, and no workflow under `.github/workflows/` references "cursor" or "bugbot" (checked 2026-07-01). +PRs in `archgate/cli` are evaluated by a Cursor Automation called "Archgate CLI Approver" (PR check "Cursor Approval Agent," linked to `cursor.com/automations/`). It's entirely external/hosted — not a GitHub Actions workflow or any file in this repo. It reads [APPROVAL_POLICY.md](../../../APPROVAL_POLICY.md) at the repo root to customize its behavior (bespoke logic in the automation's own prompt, not a Cursor platform feature — confirmed no built-in policy-file mechanism exists). -**Why:** The agent's own PR comments say it looks for `APPROVAL_POLICY.md` / `cursor/approval-policies/ROUTING.md` in-repo to customize its behavior, and falls back to a default heuristic otherwise: block auto-approval unless it finds a `cursor[bot]` review comment containing the marker ``. Observed failure mode on [PR #439](https://github.com/archgate/cli/pull/439): Cursor Bugbot's check passed with a clean SUCCESS conclusion but posted **no** review comment at all (plausibly because it found zero issues to flag — nothing to say). The Approval Agent's fallback couldn't distinguish "Bugbot ran clean" from "Bugbot's comment hasn't posted yet," so it conservatively left a non-approving comment instead of approving. The PR still merged fine (author is sole CODEOWNER); this is a soft/non-blocking signal, not a broken merge. +**Current policy:** a `success` Bugbot check alone is sufficient for approval, even with no review comment — Cursor's docs confirm `success` legitimately means "no issues found," Bugbot doesn't always post a comment. Human review is still required when the check is missing/pending/failed or Bugbot left unresolved comments. -**Resolution (2026-07-01):** Confirmed via Cursor's own docs (`cursor.com/docs/bugbot`) that a `success` Bugbot check conclusion legitimately means "no issues found, no unresolved comments" — Bugbot does NOT always post a comment, so "check passed, no comment" is the documented happy path, not an ambiguous/unverified signal. Also confirmed via `cursor.com/docs/cloud-agent/automations` that Cursor Automations have no built-in/documented repo-policy-file mechanism — the `APPROVAL_POLICY.md` lookup is bespoke logic this specific automation's author coded into its own prompt (not a Cursor platform feature), so it's genuinely repo-controllable. Created [APPROVAL_POLICY.md](../../../APPROVAL_POLICY.md) at the repo root (after asking the user, per their explicit "read the docs and bring a solution" request) documenting that a `success` Bugbot check alone is sufficient for approval regardless of comment presence, while still requiring human review when the check is missing/pending/failed or Bugbot left unresolved comments. Not yet verified against a live automation run — the automation's prompt might not actually read this file (its "policy file" search was self-reported, not independently confirmed); watch the next PR's approval-agent comment to see if it picks this up, and if not, the fallback is editing the automation's prompt directly on cursor.com (only the account owner has access). - -**How to apply:** If a future "Cursor approval failing" report comes in for a DIFFERENT failure mode (not the missing-comment case above), don't assume `APPROVAL_POLICY.md` covers it — re-check what specific signal the automation says it's missing, since its policy-reading behavior is unverified/undocumented and may not generalize. +**How to apply:** if a future "Cursor approval failing" report is a different failure mode, don't assume `APPROVAL_POLICY.md` covers it — the automation's policy-reading behavior is self-reported by the automation, not independently verified against its actual prompt. Only the cursor.com account owner can edit the automation's prompt directly if the policy file isn't picked up. diff --git a/.github/workflows/code-pull-request.yml b/.github/workflows/code-pull-request.yml index 71ec72b6..767b79e0 100644 --- a/.github/workflows/code-pull-request.yml +++ b/.github/workflows/code-pull-request.yml @@ -85,16 +85,20 @@ jobs: fetch-depth: 0 - name: Check for shim changes id: changes + env: + # Passed via env, not inlined into the script, to avoid template-injection + # (zizmor: code injection via template expansion). + BASE_REF: ${{ github.base_ref }} run: | # Detect changes to shim source code AND workflow files that configure # shim test/publish infrastructure (e.g., action version bumps, runtime # version changes). Without checking workflow files, a Renovate PR that # bumps Ruby 3.3→3.4 in the shim-tests job would skip tests entirely. - PATHS="shims/ .github/workflows/publish-shims.yml .github/workflows/code-pull-request.yml" + PATHS=(shims/ .github/workflows/publish-shims.yml .github/workflows/code-pull-request.yml) if [ "${{ github.event_name }}" = "pull_request" ]; then - CHANGED=$(git diff --name-only origin/${{ github.base_ref }}...HEAD -- $PATHS || true) + CHANGED=$(git diff --name-only "origin/$BASE_REF...HEAD" -- "${PATHS[@]}" || true) else - CHANGED=$(git diff --name-only HEAD~1 -- $PATHS || true) + CHANGED=$(git diff --name-only HEAD~1 -- "${PATHS[@]}" || true) fi if [ -n "$CHANGED" ]; then echo "shims_changed=true" >> "$GITHUB_OUTPUT" @@ -211,6 +215,26 @@ jobs: config: zizmor.yml advanced-security: ${{ github.event.pull_request.head.repo.fork != true }} + # Validates workflow schema (zizmor covers security patterns only). See CI-002. + actionlint: + name: Actionlint (Workflow Schema) + runs-on: ubuntu-latest + timeout-minutes: 5 + if: github.event_name != 'pull_request' || github.event.pull_request.draft == false + permissions: + contents: read # actions/checkout + steps: + - name: Checkout code + uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7 + with: + persist-credentials: false + - name: Install actionlint + # Pinned script commit + pinned version, not `latest` — see CI-002. + run: | + bash <(curl -fsSL "https://raw.githubusercontent.com/rhysd/actionlint/011a6d15e749bb3f2d771eed9c7aa0e7e3e10ee7/scripts/download-actionlint.bash") 1.7.12 + - name: Run actionlint + run: ./actionlint -color + coverage: name: Coverage Report runs-on: ubuntu-latest @@ -272,6 +296,7 @@ jobs: smoke-windows, smoke-linux, zizmor, + actionlint, coverage, ] steps: @@ -286,6 +311,7 @@ jobs: [[ "${{ needs.smoke-windows.result }}" != "success" ]] || \ [[ "${{ needs.smoke-linux.result }}" != "success" ]] || \ [[ "${{ needs.zizmor.result }}" != "success" ]] || \ + [[ "${{ needs.actionlint.result }}" != "success" ]] || \ [[ "${{ needs.coverage.result }}" != "success" ]]; then echo "::error::One or more jobs failed:" echo " validate: ${{ needs.validate.result }}" @@ -293,6 +319,7 @@ jobs: echo " smoke-windows: ${{ needs.smoke-windows.result }}" echo " smoke-linux: ${{ needs.smoke-linux.result }}" echo " zizmor: ${{ needs.zizmor.result }}" + echo " actionlint: ${{ needs.actionlint.result }}" echo " coverage: ${{ needs.coverage.result }}" exit 1 fi diff --git a/.github/workflows/publish-shims.yml b/.github/workflows/publish-shims.yml index 6c3b54bf..51cf7c79 100644 --- a/.github/workflows/publish-shims.yml +++ b/.github/workflows/publish-shims.yml @@ -108,12 +108,10 @@ jobs: needs: wait-for-binaries runs-on: ubuntu-latest timeout-minutes: 5 - # workflows: write is required to push a new ref — GitHub rejects any - # ref push reachable through commits that touch .github/workflows/* - # without it, even for an unrelated tag like this one. + # contents: write is required to push a tag. See + # project_release_pipeline_gotchas.md for why there's no "workflows" scope. permissions: contents: write - workflows: write steps: - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7 with: diff --git a/.github/workflows/release-binaries.yml b/.github/workflows/release-binaries.yml index 4c4aedac..de299427 100644 --- a/.github/workflows/release-binaries.yml +++ b/.github/workflows/release-binaries.yml @@ -152,9 +152,9 @@ jobs: id: combine run: | echo "=== Individual digests ===" - cat *.sha256 + cat ./*.sha256 echo "" - DIGESTS=$(cat *.sha256 | tr -d '\r' | base64 -w0) + DIGESTS=$(cat ./*.sha256 | tr -d '\r' | base64 -w0) echo "digests=$DIGESTS" >> "$GITHUB_OUTPUT" # SLSA provenance — generates .intoto.jsonl and uploads it to the release. diff --git a/.github/workflows/smoke-test-linux.yml b/.github/workflows/smoke-test-linux.yml index 2b7e7995..ad78d472 100644 --- a/.github/workflows/smoke-test-linux.yml +++ b/.github/workflows/smoke-test-linux.yml @@ -94,7 +94,8 @@ jobs: fi echo "Testing install.sh with version $ARCHGATE_VERSION" - export ARCHGATE_INSTALL_DIR="$(mktemp -d)" + ARCHGATE_INSTALL_DIR="$(mktemp -d)" + export ARCHGATE_INSTALL_DIR sh install.sh diff --git a/CLAUDE.md b/CLAUDE.md index c02aee7d..f4602075 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -19,14 +19,14 @@ bun run format # oxfmt --write bun run format:check # oxfmt --check bun run test # all tests (not bare `bun test` — picks up --timeout; see GEN-003) bun run knip # dead export detection -bun run validate # MANDATORY: lint + typecheck + format + test + ADR check + knip + build check +bun run validate # MANDATORY: lint + typecheck + format:check + test + ADR check + knip + build check bun run build:check # verify build compiles (CI builds binaries via release workflow) bun run commit # conventional commit wizard ``` ## Validation Gate -**`bun run validate` must pass before any task is considered complete.** Fail-fast pipeline: lint → typecheck → format → test → ADR check → knip → build check. Mirrors CI in `.github/workflows/code-pull-request.yml`. +**`bun run validate` must pass before any task is considered complete.** Fail-fast pipeline: lint → typecheck → format:check → test → ADR check → knip → build check. Mirrors CI in `.github/workflows/code-pull-request.yml`. ## Git Hooks (Git 2.54+) @@ -43,23 +43,15 @@ git config --local include.path ../.githooks Opt out of a specific hook: `git config --local hook..enabled false`. Skip all hooks for a single commit: `git commit --no-verify`. -## GitHub Actions: `secrets.*` vs `vars.*` +## Agent Memory -These are two distinct, non-overlapping namespaces in workflow expressions — configuring a value as a repo **secret** does NOT make it readable via `vars.*`, and vice versa. `.github/workflows/release.yml`'s "Annotate release in PostHog" step read `POSTHOG_PROJECT_ID` via `${{ vars.POSTHOG_PROJECT_ID }}` while the value only ever existed as a **secret** (`gh secret list`), so `vars.POSTHOG_PROJECT_ID` always resolved empty. Combined with a guard clause that does `exit 0` (not a failure) when required config is missing, plus `continue-on-error: true` and a low-visibility `::notice::` log line, the step silently no-opped on every release for weeks — annotations simply stopped appearing in PostHog with no CI failure to flag it. Fixed by reading `secrets.POSTHOG_PROJECT_ID` to match where the value actually lives. +Claude Code sessions in this repo maintain persistent memory at `.claude/agent-memory/archgate-developer/` (index: `MEMORY.md`). Operational gotchas that are incident history rather than standing conventions live there instead of here, including: -When adding any workflow step that reads repo-level config: confirm the value's actual location with `gh secret list` / `gh variable list` before writing `secrets.X` vs `vars.X`, and if the step is `continue-on-error: true` with an internal "not configured, skipping" guard, use `::warning::` (or higher) rather than `::notice::` so a misconfiguration is visible in the Actions UI instead of silently invisible indefinitely. +- GitHub Actions `secrets.*` vs `vars.*` namespace confusion +- Release pipeline gotchas (workflow-trigger races, toolchain cache bugs, update-check stdout pollution) +- Claude Code hooks config for `.claude/settings.json` (the `WorktreeCreate` contract, the `"shell": "bash"` requirement for POSIX hooks) -## Release Pipeline Gotchas - -- **Chain downstream release workflows, don't parallel-trigger them.** `publish-shims.yml` has no `release: published` trigger — `release-binaries.yml`'s `trigger-shim-publish` job dispatches it (`gh workflow run`) after binaries + provenance succeed. Two workflows both listening to `release: published` races: if the build needs a retry, a fixed-budget wait job can time out (`cancelled`, terminal) before the retry finishes. -- **`moonrepo/setup-toolchain`'s cache can silently break PATH.** Right after a `.prototools` bump, the first macOS/Windows CI run often restores a stale `restore-key` cache instead of an exact hit (check the log for `Cache hit for restore-key:`) — the action reports success but `bun` isn't wired onto PATH. Self-heals on retry (the failed job still saves a fresh exact-key cache). -- **The CLI's background update-check notice can pollute stdout.** `checkForUpdatesIfNeeded()` prints to stdout after command output; `shouldPerformUpdateCheck()` in `src/helpers/update-check.ts` gates it to TTY-only, non-CI, non-`upgrade` sessions so piped/agent JSON output isn't corrupted. - -## Claude Code Harness Config (`.claude/settings.json`) - -The `hooks.WorktreeCreate` entry is **not** a post-creation setup step — once it's configured, the Claude Code harness defers the _entire_ worktree creation to it (it does not also create a git worktree on its own). The hook receives a JSON payload on stdin (`{ "cwd", "name", ... }`, same pattern as the `PostToolUse` hook reading `.tool_input.file_path` via `jq`) and **must** create the worktree itself and echo _only_ the resulting absolute path as its final stdout line — any other stdout (e.g. unsilenced `bun install` or `git worktree add` output) gets misread as the path and breaks `EnterWorktree`/`ExitWorktree` with errors like `path contains control characters` or `ENOENT: ... chdir`. Redirect all setup-command output to stderr (`>&2`) and keep the trailing `printf` as the only real stdout. Do not simplify this hook back down to a bare `bun install` — that regression is exactly what caused the worktree-creation bug fixed here. - -**Command-type hooks with POSIX shell syntax MUST set `"shell": "bash"` explicitly — do not rely on the platform default, even on Windows with Git Bash installed.** Without it, on at least one confirmed Windows setup, the hook runner fell back to spawning via `cmd.exe` (Node `child_process.spawn` with `shell: true` defaults to `%ComSpec%`) instead of detecting Git Bash — even though the interactive `Bash` tool on the same machine correctly used Git Bash. Symptom: ` failed: : 'x' is not recognized as an internal or external command, operable program or batch file.` (that exact phrasing is cmd.exe's, not PowerShell's — PowerShell says `is not recognized as the name of a cmdlet, function, script file, or operable program`). Fix: add `"shell": "bash"` to the hook object (sibling of `"command"`). If Git Bash still isn't found (error becomes `Hook "..." requires bash but Git Bash was not found`), Claude Code checks, in order: the `CLAUDE_CODE_GIT_BASH_PATH` env var, then `C:\Program Files\Git\bin\bash.exe` / the `(x86)` variant, then a `git` on `PATH` resolved to `..\..\bin\bash.exe` — set `CLAUDE_CODE_GIT_BASH_PATH` if none of those apply. Verified end-to-end via `EnterWorktree`/`ExitWorktree` on `hooks.WorktreeCreate` on 2026-07-01. +If you're a memory-equipped agent, consult that index when working in these areas. If you're a fresh session, contributor, or tool without access to it, the same facts are recoverable from git history and the referenced source files (`.github/workflows/release.yml`, `publish-shims.yml`, `release-binaries.yml`, `src/helpers/update-check.ts`, `.claude/settings.json`). ## Architecture @@ -117,6 +109,6 @@ Editor integrations share the `EditorTarget` union. Adding a new editor requires User-scope editors (e.g., opencode) write to a path resolved in `paths.ts` rather than the project tree — `configureEditorSettings` returns that path for the init summary and the real work happens in `tryInstallPlugin`. -**Match the target editor's actual path resolution — don't assume Windows conventions.** opencode uses the `xdg-basedir` npm package, which falls back to `~/.config` on **all platforms** (including Windows, where it resolves to `C:\Users\\.config\…`, not `%APPDATA%\…`). `opencodeAgentsDir()` must mirror that exact logic or the CLI writes files the editor can't find. When adding a user-scope editor, verify the editor's path helper in its source before writing the resolver. +**Match the target editor's actual path resolution — don't assume Windows conventions.** opencode uses `xdg-basedir`, which falls back to `~/.config` on **all platforms** (Windows: `C:\Users\\.config\…`, not `%APPDATA%\…`). `opencodeAgentsDir()` must mirror that exactly. Verify the editor's own path helper before writing a resolver for a new user-scope editor. -**opencode ships two distributions — CLI detection alone misses the Desktop app.** The `opencode` CLI is one distribution; the opencode Desktop app (Electron-based, e.g. `@opencode-aidesktop` on Windows) is another, and it ships **no CLI binary at all** — `isOpencodeCliAvailable()` (a PATH check via `resolveCommand`) can never detect it. Both distributions read/write the same `opencodeConfigDir()` (`~/.config/opencode/`), so `isOpencodeAvailable()` in `plugin-install.ts` also treats that directory's existence as a valid installed-opencode signal. All three call sites (`editor-detect.ts`, `init-project.ts`, `commands/plugin/install.ts`) use `isOpencodeAvailable()`, not the narrower CLI-only check — use the broader one for any new opencode-gated behavior too. +**opencode ships two distributions — CLI detection alone misses the Desktop app.** The Electron-based Desktop app (`@opencode-aidesktop` on Windows) ships **no CLI binary**, so `isOpencodeCliAvailable()` (PATH check) can't detect it. Both distributions share `opencodeConfigDir()` (`~/.config/opencode/`), so `isOpencodeAvailable()` in `plugin-install.ts` also treats that directory's existence as installed. All three call sites (`editor-detect.ts`, `init-project.ts`, `commands/plugin/install.ts`) use the broader `isOpencodeAvailable()` — use it for any new opencode-gated behavior too.