From 4d4b04e798206499916125880f3f7e0607f029ee Mon Sep 17 00:00:00 2001 From: Rhuan Barreto Date: Fri, 3 Jul 2026 10:32:16 -0300 Subject: [PATCH 01/15] fix(ci): dispatch publish-shims from release-binaries instead of racing on release event publish-shims.yml and release-binaries.yml both triggered on `release: published`, racing each other: if the binary build needed a retry, publish-shims.yml's fixed-budget wait-for-binaries poll timed out and went `cancelled` (terminal) before the retry finished, silently skipping shim publishing for that release. publish-shims.yml is now workflow_dispatch-only; release-binaries.yml dispatches it via `gh workflow run` once binaries + provenance succeed. Signed-off-by: Rhuan Barreto --- .github/workflows/publish-shims.yml | 36 ++++++++++++++------------ .github/workflows/release-binaries.yml | 17 ++++++++++++ 2 files changed, 36 insertions(+), 17 deletions(-) diff --git a/.github/workflows/publish-shims.yml b/.github/workflows/publish-shims.yml index 34e26273..7169e38f 100644 --- a/.github/workflows/publish-shims.yml +++ b/.github/workflows/publish-shims.yml @@ -1,8 +1,9 @@ name: Publish Shims +# No `release: published` trigger — release-binaries.yml dispatches this via +# `gh workflow run` once binaries are actually uploaded. Avoids racing a +# fixed-budget wait against a build that may need a retry (see CLAUDE.md). on: - release: - types: [published] workflow_dispatch: inputs: tag: @@ -15,21 +16,20 @@ env: ARCHGATE_TELEMETRY: "0" jobs: - # Wait for platform binaries to be uploaded by release-binaries.yml. - # Shim packages download binaries on first use, but smoke tests need them. + # Sanity-check assets are present (covers GitHub API read-after-write lag). wait-for-binaries: runs-on: ubuntu-latest - timeout-minutes: 30 + timeout-minutes: 5 permissions: contents: read steps: - - name: Wait for release assets + - name: Verify release assets are present env: GH_TOKEN: ${{ github.token }} run: | - TAG="${{ github.event.release.tag_name || inputs.tag }}" + TAG="${{ inputs.tag }}" REQUIRED_ASSETS=("archgate-darwin-arm64.tar.gz" "archgate-linux-x64.tar.gz" "archgate-win32-x64.zip") - for i in $(seq 1 60); do + for i in $(seq 1 10); do ASSETS=$(gh release view "$TAG" --repo archgate/cli --json assets --jq '.assets[].name') ALL_FOUND=true for asset in "${REQUIRED_ASSETS[@]}"; do @@ -42,10 +42,12 @@ jobs: echo "All required release assets found" exit 0 fi - echo "Waiting for release assets... attempt $i/60" - sleep 30 + echo "Waiting for release assets... attempt $i/10" + sleep 15 done - echo "::error::Timed out waiting for release assets" + echo "::error::Required binaries for $TAG are still missing after 10 checks (~2.5 min)." + echo "::error::This workflow expects release-binaries.yml to have already succeeded for this tag." + echo "::error::Check the Release Binaries run for $TAG, then re-run: gh workflow run publish-shims.yml -f tag=$TAG" exit 1 publish-pypi: @@ -57,7 +59,7 @@ jobs: steps: - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7 with: - ref: ${{ github.event.release.tag_name || inputs.tag }} + ref: ${{ inputs.tag }} - uses: astral-sh/setup-uv@fac544c07dec837d0ccb6301d7b5580bf5edae39 # v8.2.0 with: python-version: "3.12" @@ -80,7 +82,7 @@ jobs: steps: - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7 with: - ref: ${{ github.event.release.tag_name || inputs.tag }} + ref: ${{ inputs.tag }} - uses: actions/setup-dotnet@26b0ec14cb23fa6904739307f278c14f94c95bf1 # v5 with: dotnet-version: "8.0.x" @@ -106,10 +108,10 @@ jobs: steps: - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7 with: - ref: ${{ github.event.release.tag_name || inputs.tag }} + ref: ${{ inputs.tag }} - name: Create Go module tag run: | - TAG="${{ github.event.release.tag_name || inputs.tag }}" + TAG="${{ inputs.tag }}" GO_TAG="shims/go/${TAG}" git tag "$GO_TAG" git push origin "$GO_TAG" @@ -122,7 +124,7 @@ jobs: steps: - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7 with: - ref: ${{ github.event.release.tag_name || inputs.tag }} + ref: ${{ inputs.tag }} - uses: actions/setup-java@1bcf9fb12cf4aa7d266a90ae39939e61372fe520 # v5 with: distribution: "temurin" @@ -151,7 +153,7 @@ jobs: steps: - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7 with: - ref: ${{ github.event.release.tag_name || inputs.tag }} + ref: ${{ inputs.tag }} - uses: ruby/setup-ruby@9eb537ca036ebaed86729dcb9309076e4c5c3b74 # v1.314.0 with: ruby-version: "4.0.5" diff --git a/.github/workflows/release-binaries.yml b/.github/workflows/release-binaries.yml index edb653b7..d0ca627f 100644 --- a/.github/workflows/release-binaries.yml +++ b/.github/workflows/release-binaries.yml @@ -171,3 +171,20 @@ jobs: base64-subjects: "${{ needs.combine-hashes.outputs.digests }}" upload-assets: true upload-tag-name: ${{ github.event.release.tag_name || inputs.tag }} + + # Dispatch publish-shims.yml once binaries + provenance are confirmed + # uploaded, instead of racing it via a shared `release: published` trigger. + trigger-shim-publish: + name: Trigger shim publishing + needs: provenance + runs-on: ubuntu-latest + timeout-minutes: 5 + permissions: + actions: write + steps: + - name: Dispatch publish-shims.yml + env: + GH_TOKEN: ${{ github.token }} + run: | + TAG="${{ github.event.release.tag_name || inputs.tag }}" + gh workflow run publish-shims.yml --repo archgate/cli -f tag="$TAG" From 68b46d649ba3439d9ac0bf434358d50b2fb4b057 Mon Sep 17 00:00:00 2001 From: Rhuan Barreto Date: Fri, 3 Jul 2026 10:32:21 -0300 Subject: [PATCH 02/15] fix(cli): gate background update-check notice to interactive TTY sessions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit main() unconditionally printed an "update available" notice to stdout after every command, with no gating. Any script, `| jq`, or agent parsing stdout as JSON would have it corrupted whenever a newer release happened to be available. Add shouldPerformUpdateCheck() and use it to skip the check entirely outside a genuine interactive terminal (piped output, CI, or the upgrade command itself) — same guard shape as the existing showFirstRunNoticeIfNeeded(). Signed-off-by: Rhuan Barreto --- src/cli.ts | 16 +++++++--- src/helpers/update-check.ts | 13 ++++++++ tests/helpers/update-check.test.ts | 50 ++++++++++++++++++++++++++++++ 3 files changed, 74 insertions(+), 5 deletions(-) diff --git a/src/cli.ts b/src/cli.ts index 1a0cdf53..f8ecdf44 100755 --- a/src/cli.ts +++ b/src/cli.ts @@ -39,7 +39,10 @@ import { trackCommand, } from "./helpers/telemetry"; import { showFirstRunNoticeIfNeeded } from "./helpers/telemetry-config"; -import { checkForUpdatesIfNeeded } from "./helpers/update-check"; +import { + checkForUpdatesIfNeeded, + shouldPerformUpdateCheck, +} from "./helpers/update-check"; // Pre-main environment guards — these are user-facing errors (exit 1), not bugs. // The Bun check must throw (logError requires Bun APIs). The rest use logError @@ -152,10 +155,13 @@ async function main() { registerDoctorCommand(program); registerTelemetryCommand(program); - const isUpgrade = process.argv.includes("upgrade"); - const updateCheckPromise = isUpgrade - ? Promise.resolve(null) - : checkForUpdatesIfNeeded(packageJson.version); + const updateCheckPromise = shouldPerformUpdateCheck({ + argv: process.argv, + isTTY: process.stdout.isTTY === true, + ci: Bun.env.CI, + }) + ? checkForUpdatesIfNeeded(packageJson.version) + : Promise.resolve(null); await program.parseAsync(process.argv); const notice = await updateCheckPromise; if (notice) console.log(notice); diff --git a/src/helpers/update-check.ts b/src/helpers/update-check.ts index 08f493f5..b6d406ca 100644 --- a/src/helpers/update-check.ts +++ b/src/helpers/update-check.ts @@ -9,6 +9,19 @@ import { internalPath } from "./paths"; const CACHE_FILE = "last-update-check"; const CACHE_TTL_MS = 24 * 60 * 60 * 1000; // 24 hours +/** + * Only check for updates in a genuine interactive terminal — never during + * `upgrade`, in CI, or when stdout is piped (avoids polluting parsed output). + */ +export function shouldPerformUpdateCheck(opts: { + argv: string[]; + isTTY: boolean; + ci: string | undefined; +}): boolean { + const isUpgrade = opts.argv.includes("upgrade"); + return !isUpgrade && opts.isTTY && !opts.ci; +} + /** * Checks GitHub Releases for a newer Archgate release (at most once per 24h). * Returns a human-readable notice string if an update is available, or null otherwise. diff --git a/tests/helpers/update-check.test.ts b/tests/helpers/update-check.test.ts index aa8ef2fa..030884c4 100644 --- a/tests/helpers/update-check.test.ts +++ b/tests/helpers/update-check.test.ts @@ -5,6 +5,56 @@ import { existsSync, mkdtempSync, rmSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; +describe("shouldPerformUpdateCheck", () => { + test("true in a genuine interactive terminal", async () => { + const { shouldPerformUpdateCheck } = + await import("../../src/helpers/update-check"); + expect( + shouldPerformUpdateCheck({ + argv: ["bun", "cli.ts", "session-context", "claude-code", "list"], + isTTY: true, + ci: undefined, + }) + ).toBe(true); + }); + + test("false when CI is set, even on a TTY", async () => { + const { shouldPerformUpdateCheck } = + await import("../../src/helpers/update-check"); + expect( + shouldPerformUpdateCheck({ + argv: ["bun", "cli.ts", "session-context", "claude-code", "list"], + isTTY: true, + ci: "1", + }) + ).toBe(false); + }); + + test("false when stdout is not a TTY (piped/redirected output)", async () => { + const { shouldPerformUpdateCheck } = + await import("../../src/helpers/update-check"); + expect( + shouldPerformUpdateCheck({ + argv: ["bun", "cli.ts", "session-context", "claude-code", "list"], + isTTY: false, + ci: undefined, + }) + ).toBe(false); + }); + + test("false for the upgrade command itself, even on an interactive TTY", async () => { + const { shouldPerformUpdateCheck } = + await import("../../src/helpers/update-check"); + expect( + shouldPerformUpdateCheck({ + argv: ["bun", "cli.ts", "upgrade"], + isTTY: true, + ci: undefined, + }) + ).toBe(false); + }); +}); + describe("checkForUpdatesIfNeeded", () => { let tempDir: string; let originalHome: string | undefined; From 49335cd725ed866ea2c0f0d24e68d7a4c948cd4d Mon Sep 17 00:00:00 2001 From: Rhuan Barreto Date: Fri, 3 Jul 2026 10:32:28 -0300 Subject: [PATCH 03/15] chore(memory): document release pipeline gotchas and comment-conciseness feedback Signed-off-by: Rhuan Barreto --- .../agent-memory/archgate-developer/MEMORY.md | 4 ++++ .../feedback_concise_comments.md | 17 +++++++++++++++++ .../project_release_pipeline_gotchas.md | 15 +++++++++++++++ CLAUDE.md | 6 ++++++ 4 files changed, 42 insertions(+) create mode 100644 .claude/agent-memory/archgate-developer/feedback_concise_comments.md create mode 100644 .claude/agent-memory/archgate-developer/project_release_pipeline_gotchas.md diff --git a/.claude/agent-memory/archgate-developer/MEMORY.md b/.claude/agent-memory/archgate-developer/MEMORY.md index 226bd1ca..b297c170 100644 --- a/.claude/agent-memory/archgate-developer/MEMORY.md +++ b/.claude/agent-memory/archgate-developer/MEMORY.md @@ -27,6 +27,7 @@ Skipping steps 2 or 3 is a workflow violation. The user should NEVER have to inv - [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 ## Known Bugs @@ -70,6 +71,9 @@ Non-enforceable lessons — environment/CI/platform quirks no static rule can re - **`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`. +- **Chain downstream release workflows via `gh workflow run`, not a parallel `release: published` trigger** — two workflows both listening for `release: published` race if the first needs a retry. See [[project_release_pipeline_gotchas]] and `CLAUDE.md` "Release Pipeline Gotchas". +- **`moonrepo/setup-toolchain` cache can silently leave `bun` off PATH after a `.prototools` bump** — check job log for `Cache hit for restore-key:`; self-heals on retry. See [[project_release_pipeline_gotchas]]. +- **CLI's background update-check notice can pollute stdout** — gated via `shouldPerformUpdateCheck()` (TTY + non-CI only). See [[project_release_pipeline_gotchas]]. ## Claude Code Harness Config diff --git a/.claude/agent-memory/archgate-developer/feedback_concise_comments.md b/.claude/agent-memory/archgate-developer/feedback_concise_comments.md new file mode 100644 index 00000000..0b3f6e6c --- /dev/null +++ b/.claude/agent-memory/archgate-developer/feedback_concise_comments.md @@ -0,0 +1,17 @@ +--- +name: feedback-concise-comments +description: Keep code comments and memory entries concise — do not overgenerate explanatory prose +metadata: + type: feedback +--- + +Code comments and memory entries must be concise. Do not write multi-paragraph explanatory comments in source/workflow files, and do not write long-winded memory bullets. + +**Why:** User feedback via `/feedback` (2026-07-03): "sonnet is overgenerating comments in code and memories. this is not good. those comments must be concise." Given directly as a correction after a session where I wrote long comment blocks in `.github/workflows/*.yml` and `src/cli.ts` (multi-sentence rationale blocks) and multi-sentence [[MEMORY.md]] bullets with full incident narratives. + +**How to apply:** + +- Code/workflow comments: one line stating _what_ and, if truly non-obvious, a terse _why_ — not a paragraph with incident timelines, confirmed-via citations, or full backstory. Link to a PR/issue/commit for detail instead of inlining it. +- Memory entries (`MEMORY.md` bullets and topic files): lead with the rule in one line; keep **Why:**/**How to apply:** to single short sentences, not multi-clause narratives with timestamps and evidence trails. +- If tempted to write a long comment or memory entry to "preserve context," prefer a short pointer (file/PR reference) over inlining the full story. +- Applies to all future sessions in this repo — re-check comment/memory length before writing. diff --git a/.claude/agent-memory/archgate-developer/project_release_pipeline_gotchas.md b/.claude/agent-memory/archgate-developer/project_release_pipeline_gotchas.md new file mode 100644 index 00000000..e55c01da --- /dev/null +++ b/.claude/agent-memory/archgate-developer/project_release_pipeline_gotchas.md @@ -0,0 +1,15 @@ +--- +name: project-release-pipeline-gotchas +description: Three causes behind archgate/cli release publishing failures found 2026-07-03 +metadata: + type: project +--- + +Three 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. + +**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. diff --git a/CLAUDE.md b/CLAUDE.md index 4e732cb8..a71dd152 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -49,6 +49,12 @@ These are two distinct, non-overlapping namespaces in workflow expressions — c 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. +## 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 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. From aec901cbe08de9c19abc5f511b78115bc91d6187 Mon Sep 17 00:00:00 2001 From: Rhuan Barreto Date: Fri, 3 Jul 2026 10:41:27 -0300 Subject: [PATCH 04/15] fix(ci): grant workflows:write to publish-go-tag so its tag push isn't rejected MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit git push origin "shims/go/\$TAG" was failing with "refusing to allow a GitHub App to create or update workflow .github/workflows/release.yml without workflows permission" — GitHub rejects any ref push reachable through commits touching .github/workflows/*, even for an unrelated tag, unless the token has workflows: write. Confirmed on v0.45.7's publish-go-tag job log. Signed-off-by: Rhuan Barreto --- .github/workflows/publish-shims.yml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.github/workflows/publish-shims.yml b/.github/workflows/publish-shims.yml index 7169e38f..940f6f34 100644 --- a/.github/workflows/publish-shims.yml +++ b/.github/workflows/publish-shims.yml @@ -103,8 +103,12 @@ 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. permissions: contents: write + workflows: write steps: - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7 with: From f45be44f608d1edfe4de01deb1dc1c42273bef9b Mon Sep 17 00:00:00 2001 From: Rhuan Barreto Date: Fri, 3 Jul 2026 10:41:42 -0300 Subject: [PATCH 05/15] chore(memory): note the publish-go-tag permissions fix Signed-off-by: Rhuan Barreto --- .../archgate-developer/project_release_pipeline_gotchas.md | 1 + 1 file changed, 1 insertion(+) 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 e55c01da..6bf62908 100644 --- a/.claude/agent-memory/archgate-developer/project_release_pipeline_gotchas.md +++ b/.claude/agent-memory/archgate-developer/project_release_pipeline_gotchas.md @@ -10,6 +10,7 @@ Three distinct issues caused shim/binary publishing failures across v0.45.1–v0 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. **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. From 367e6da1a789dffb08e4d6c10b5cbd9bf8c11417 Mon Sep 17 00:00:00 2001 From: Rhuan Barreto Date: Fri, 3 Jul 2026 11:33:47 -0300 Subject: [PATCH 06/15] docs(adr): add ARCH-022 and PRD for AST-aware RuleContext Document the decision to expose ctx.ast(path, language) on RuleContext, reusing the existing meriyah parser for TS/JS and shelling out to each language's own stdlib AST facility (Python's ast module, Ruby's Ripper) for Python/Ruby, gated behind path-safety, language-plausibility, and interpreter-availability guardrails so no raw subprocess access is ever exposed to rule authors. Requires no new production dependency. The PRD is kept separate to isolate product scope/rollout/success criteria from the architectural decision itself. Signed-off-by: Rhuan Barreto --- .archgate/PRDs/ast-aware-rule-context.md | 75 ++++++++++ .../adrs/ARCH-022-ast-aware-rule-context.md | 128 ++++++++++++++++++ 2 files changed, 203 insertions(+) create mode 100644 .archgate/PRDs/ast-aware-rule-context.md create mode 100644 .archgate/adrs/ARCH-022-ast-aware-rule-context.md diff --git a/.archgate/PRDs/ast-aware-rule-context.md b/.archgate/PRDs/ast-aware-rule-context.md new file mode 100644 index 00000000..3fefc1b5 --- /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 for v1: TypeScript, JavaScript, Python, Ruby. +- 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..a501d595 --- /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 (`python3 -c "..."`, serializing the tree to JSON), Ruby's built-in `Ripper` (`ruby -rripper -rjson -e "..."`, serializing its s-expression output to JSON). 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 `python3`. +3. **Interpreter availability probe** — for `"python"`/`"ruby"`, an availability check (e.g. `Bun.spawn(["python3", "--version"])` wrapped in `try/catch`, following the exact pattern `isClaudeCliAvailable()` uses in ARCH-007) MUST run before 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. + +**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 `python3`/`ruby` 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 `python3`/`ruby` 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 `python3`/`ruby` 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` sandbst 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) From 54419be47376902b32cab6d6919baacf691a7134 Mon Sep 17 00:00:00 2001 From: Rhuan Barreto Date: Fri, 3 Jul 2026 11:59:30 -0300 Subject: [PATCH 07/15] chore(memory): compress CLAUDE.md and agent-memory topic files MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Trim incident-log narrative down to actionable facts across CLAUDE.md and the largest memory topic files (worktree-create hook history, session-context --skip fix history, shim publishing, cursor approval agent, oxlint enforcement-layer feedback). No facts, paths, commands, or error strings were dropped — only prose and blow-by-blow verification narrative. Total agent-memory footprint: 49.4KB -> 36.3KB. CLAUDE.md: 12.0KB -> 10.0KB. Signed-off-by: Rhuan Barreto --- .../feedback_prefer_tests_over_adr_rules.md | 2 +- .../project_session_context_skip_root_fix.md | 18 ++++------ .../project_shim_publishing.md | 4 +-- .../project_worktree_create_hook_contract.md | 35 ++++++------------- .../reference_cursor_approval_agent.md | 10 +++--- CLAUDE.md | 12 +++---- 6 files changed, 28 insertions(+), 53 deletions(-) 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..ad3b94d6 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, an ARCH-019 `.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). Landed 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_session_context_skip_root_fix.md b/.claude/agent-memory/archgate-developer/project_session_context_skip_root_fix.md index 5ecb60dd..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,22 +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. - -**Inspecting real opencode data:** the live `opencode.db` can't be opened with `readonly: true` while opencode is running (`SQLITE_CANTOPEN`, errno 14, on the WAL-mode DB at `~/.local/share/opencode/opencode.db`). Copy `opencode.db` + `.db-wal` + `.db-shm` to a temp dir and open the copy instead. - -See also [[project_cli_skill_flag_sequencing]] for the general release-sequencing rule this bug's fix triggered (the `--skip` flag removal). +**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 84b49d1f..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,8 @@ 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. 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/CLAUDE.md b/CLAUDE.md index c02aee7d..06105901 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -45,9 +45,7 @@ Opt out of a specific hook: `git config --local hook..enabled false`. Skip ## GitHub Actions: `secrets.*` vs `vars.*` -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. - -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. +Distinct, non-overlapping namespaces — a repo **secret** is not readable via `vars.*`, and vice versa. (`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::`.) Before wiring a new reference, confirm the value's actual location with `gh secret list` / `gh variable list`. 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. ## Release Pipeline Gotchas @@ -57,9 +55,9 @@ When adding any workflow step that reads repo-level config: confirm the value's ## 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. +`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 [the full bug history](.claude/agent-memory/archgate-developer/project_worktree_create_hook_contract.md) (5 rounds of fixes, all worth re-testing if this hook changes). -**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. +**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`. ## Architecture @@ -117,6 +115,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. From d1b9b381ff1e7c5d10cf20008921788d0a01b955 Mon Sep 17 00:00:00 2001 From: Rhuan Barreto Date: Fri, 3 Jul 2026 12:06:10 -0300 Subject: [PATCH 08/15] chore(memory): move incident-history knowledge from CLAUDE.md into agent memory Moved three gotcha-style sections (GitHub Actions secrets.*/vars.* confusion, release pipeline gotchas, Claude Code hooks config) out of CLAUDE.md and into .claude/agent-memory/archgate-developer/, since they're incident history rather than standing project conventions. CLAUDE.md now points to the memory index instead of inlining the detail. Kept the "Adding a New Editor Target" checklist in CLAUDE.md since it's evergreen task reference any contributor or non-memory agent needs, not incident lore. CLAUDE.md: 10.0KB -> 7.9KB. Signed-off-by: Rhuan Barreto --- .../agent-memory/archgate-developer/MEMORY.md | 4 +++- .../project_ci_workflow_gotchas.md | 2 +- .../project_claude_code_hooks_config.md | 12 ++++++++++++ CLAUDE.md | 18 ++++++------------ 4 files changed, 22 insertions(+), 14 deletions(-) create mode 100644 .claude/agent-memory/archgate-developer/project_claude_code_hooks_config.md diff --git a/.claude/agent-memory/archgate-developer/MEMORY.md b/.claude/agent-memory/archgate-developer/MEMORY.md index 116de613..1b7da7de 100644 --- a/.claude/agent-memory/archgate-developer/MEMORY.md +++ b/.claude/agent-memory/archgate-developer/MEMORY.md @@ -44,10 +44,12 @@ Non-enforceable lessons — environment/CI/platform quirks no static rule can re - [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 +- [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 diff --git a/.claude/agent-memory/archgate-developer/project_ci_workflow_gotchas.md b/.claude/agent-memory/archgate-developer/project_ci_workflow_gotchas.md index 50de05cc..4ceb080b 100644 --- a/.claude/agent-memory/archgate-developer/project_ci_workflow_gotchas.md +++ b/.claude/agent-memory/archgate-developer/project_ci_workflow_gotchas.md @@ -6,5 +6,5 @@ metadata: --- - **`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. Confirm the actual location with `gh secret list`/`gh variable list` before writing a reference. Full incident writeup (PostHog annotation step silently no-opping for weeks) is in `CLAUDE.md`'s "GitHub Actions: `secrets.*` vs `vars.*`" section. +- **`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.md b/CLAUDE.md index 06105901..97b9c34e 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -43,21 +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 -Distinct, non-overlapping namespaces — a repo **secret** is not readable via `vars.*`, and vice versa. (`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::`.) Before wiring a new reference, confirm the value's actual location with `gh secret list` / `gh variable list`. 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. +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: -## Release Pipeline Gotchas +- 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) -- **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`) - -`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 [the full bug history](.claude/agent-memory/archgate-developer/project_worktree_create_hook_contract.md) (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`. +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 From c0e88a6448adb10438fe2de39010c92d426feb85 Mon Sep 17 00:00:00 2001 From: Rhuan Barreto Date: Fri, 3 Jul 2026 12:42:55 -0300 Subject: [PATCH 09/15] fix(docs): address CodeRabbit review comments on PR #451 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - project_release_pipeline_gotchas.md: fix "three issues" wording to match the actual four numbered items, and complete the shouldPerformUpdateCheck() gate description (was missing the non-upgrade exclusion). - PRD: reconcile the contradictory v1 scope statement — TS/JS ship in v1, Python/Ruby follow per the rollout sequencing section, not both in v1 as the requirements line previously implied. - ARCH-022: fix "sandbst" typo; require the interpreter-availability probe to try platform-appropriate candidate names (python3 is not a universal PATH alias on Windows) per ARCH-009; clarify that the two ctx.ast() throw cases (missing interpreter vs. parse failure) must be distinguishable in the error message text even though they share exit code 2. Workflow-file findings (persist-credentials, template-injection, TAG interpolation) from the same review were already resolved by the origin/main merge in a prior commit — verified against current file state, no further changes needed there. Signed-off-by: Rhuan Barreto --- .archgate/PRDs/ast-aware-rule-context.md | 2 +- .archgate/adrs/ARCH-022-ast-aware-rule-context.md | 6 +++--- .../archgate-developer/project_release_pipeline_gotchas.md | 6 +++--- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/.archgate/PRDs/ast-aware-rule-context.md b/.archgate/PRDs/ast-aware-rule-context.md index 3fefc1b5..4c0426aa 100644 --- a/.archgate/PRDs/ast-aware-rule-context.md +++ b/.archgate/PRDs/ast-aware-rule-context.md @@ -40,7 +40,7 @@ Representative use cases: ### 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 for v1: TypeScript, JavaScript, Python, Ruby. +- 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 diff --git a/.archgate/adrs/ARCH-022-ast-aware-rule-context.md b/.archgate/adrs/ARCH-022-ast-aware-rule-context.md index a501d595..7d1eaf33 100644 --- a/.archgate/adrs/ARCH-022-ast-aware-rule-context.md +++ b/.archgate/adrs/ARCH-022-ast-aware-rule-context.md @@ -39,10 +39,10 @@ This method dispatches internally based on `language`, and the dispatch mechanis 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 `python3`. -3. **Interpreter availability probe** — for `"python"`/`"ruby"`, an availability check (e.g. `Bun.spawn(["python3", "--version"])` wrapped in `try/catch`, following the exact pattern `isClaudeCliAvailable()` uses in ARCH-007) MUST run before the real invocation. This probe result MUST be cached once per `check` invocation, not re-run per file. +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. +**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. @@ -89,7 +89,7 @@ This method dispatches internally based on `language`, and the dispatch mechanis ### Risks - **A future contributor bypasses the guardrail ordering and spawns `python3`/`ruby` 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` sandbst 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. + - **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.** 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..e24bcf7a 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. +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` 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. **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. From 1a7ecf8939b16de2217588f31f389a37de19b55c Mon Sep 17 00:00:00 2001 From: Rhuan Barreto Date: Fri, 3 Jul 2026 13:09:27 -0300 Subject: [PATCH 10/15] fix(docs): make ARCH-022's interpreter references consistent with the probe CodeRabbit's follow-up review caught that the invocation description still hardcoded python3 in the Python example even after the prior fix required the guardrail probe to select from platform-appropriate candidate names. Replaced the hardcoded literal with a reference to the probed executable, and softened the remaining informal python3/ruby mentions elsewhere in the ADR to avoid implying a fixed command name. Signed-off-by: Rhuan Barreto --- .archgate/adrs/ARCH-022-ast-aware-rule-context.md | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/.archgate/adrs/ARCH-022-ast-aware-rule-context.md b/.archgate/adrs/ARCH-022-ast-aware-rule-context.md index 7d1eaf33..593a4716 100644 --- a/.archgate/adrs/ARCH-022-ast-aware-rule-context.md +++ b/.archgate/adrs/ARCH-022-ast-aware-rule-context.md @@ -33,12 +33,12 @@ ast(path: string, language: "typescript" | "javascript" | "python" | "ruby"): Pr 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 (`python3 -c "..."`, serializing the tree to JSON), Ruby's built-in `Ripper` (`ruby -rripper -rjson -e "..."`, serializing its s-expression output to JSON). No third-party parser, native binding, or WASM grammar is introduced for these languages under this decision. +- **`"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 `python3`. +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. @@ -64,7 +64,7 @@ This method dispatches internally based on `language`, and the dispatch mechanis - **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 `python3`/`ruby` on a file before the language-plausibility check has run +- **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 @@ -82,13 +82,13 @@ This method dispatches internally based on `language`, and the dispatch mechanis ### 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 `python3`/`ruby` 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. +- **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 `python3`/`ruby` directly from inside a `ctx.ast()` code path without the path-safety or language-plausibility checks.** +- **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. From 9703a8c900bf7a511ac57ee938fb63bf0b73fa1e Mon Sep 17 00:00:00 2001 From: Rhuan Barreto Date: Fri, 3 Jul 2026 19:09:01 -0300 Subject: [PATCH 11/15] fix(ci): remove invalid workflows:write permission, fix stale ADR citation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two remaining unresolved review threads: 1. publish-shims.yml's publish-go-tag job set `workflows: write` in its permissions block. Verified against GitHub's own workflow syntax docs and actionlint's schema: "workflows" is not a valid permissions-key scope and was never granting anything — the original v0.45.7 incident's "fix" was a no-op. Removed the invalid key (contents: write is the real requirement for pushing a tag) and replaced the comment with an accurate explanation plus the real workaround (PAT with the workflow OAuth scope, or a GitHub App token with the Workflows permission configured at the App level) if the underlying push rejection resurfaces. Corrected the memory file that had claimed this was fixed and confirmed working — that confirmation doesn't hold up; flagged as unverified pending the next real release. 2. feedback_prefer_tests_over_adr_rules.md cited "ARCH-019" as the rejected draft allowlist rule from the inquirer prompt-type incident, but ARCH-019 is actually "Interactive Prompts via withPromptFix" — an unrelated, already-shipped ADR. The draft rule was never assigned an ADR ID (it was rejected before merging); removed the incorrect citation. Signed-off-by: Rhuan Barreto --- .../feedback_prefer_tests_over_adr_rules.md | 2 +- .../project_release_pipeline_gotchas.md | 2 +- .github/workflows/publish-shims.yml | 14 ++++++++++---- 3 files changed, 12 insertions(+), 6 deletions(-) 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 ad3b94d6..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, an ARCH-019 `.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). Landed 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. +**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_release_pipeline_gotchas.md b/.claude/agent-memory/archgate-developer/project_release_pipeline_gotchas.md index e24bcf7a..05810fe6 100644 --- a/.claude/agent-memory/archgate-developer/project_release_pipeline_gotchas.md +++ b/.claude/agent-memory/archgate-developer/project_release_pipeline_gotchas.md @@ -10,7 +10,7 @@ Four distinct issues caused shim/binary publishing failures across v0.45.1–v0. 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, non-`upgrade` 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. +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 four it was before assuming generic flakiness. diff --git a/.github/workflows/publish-shims.yml b/.github/workflows/publish-shims.yml index 6c3b54bf..6caf6cd7 100644 --- a/.github/workflows/publish-shims.yml +++ b/.github/workflows/publish-shims.yml @@ -108,12 +108,18 @@ 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. NOTE: "workflows" is not a + # valid permissions-key scope (confirmed against GitHub's own workflow + # syntax docs and actionlint's schema) and has been removed — it was + # never actually granting anything. If GITHUB_TOKEN's push is rejected + # again with "refusing to allow a GitHub App to create or update + # workflow ... without workflows permission" (seen on v0.45.7), the + # real fix is a PAT with the classic `workflow` OAuth scope stored as a + # secret for this step, or a GitHub App installation token whose App is + # explicitly configured with the Workflows permission — neither is + # expressible via this YAML key. permissions: contents: write - workflows: write steps: - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7 with: From 6c1db779cefe0ba32275db8dcd5bfef1cbc8731d Mon Sep 17 00:00:00 2001 From: Rhuan Barreto Date: Sat, 4 Jul 2026 10:48:48 -0300 Subject: [PATCH 12/15] feat(ci): add actionlint as a hard-blocking workflow schema validator MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit zizmor (already in CI) covers security patterns in workflow files but has no concept of GitHub's actual permissions/expression schema. That gap is exactly what let PR #451's invalid `workflows: write` permission scope merge undetected — it was only caught later by a third-party reviewer's internal actionlint run, which is not a dependable control. Adds an `actionlint` job to code-pull-request.yml, required by the `status` gate. Installs the binary via the maintainer's own pinned download script (specific commit SHA + pinned version, not `latest`) rather than adding a reviewdog wrapper Action, avoiding a new `uses:` trust surface for a single static-check invocation. Verified locally: passes cleanly against the current workflows and correctly flags the old (already-fixed) invalid permission scope when run against the pre-fix file. Documents the decision as CI-002, since it's a distinct topic from CI-001's `uses:` SHA-pinning scope. Also fixes a pre-existing inaccuracy in CLAUDE.md/MEMORY.md ("format" -> "format:check" in the validate pipeline description, caught during reviewer sub-agent verification) and captures two session learnings: a GraphQL technique for distinguishing resolved vs. outstanding PR review threads (the REST API doesn't expose this), and a live-confirmed instance of the installed lessons-learned skill (v0.13.1) still referencing the removed --skip flag. Reviewer: APPROVED (automated checks 39/39, CI domain PASS, General domain PASS_WITH_WARNINGS - the format:check warning, now fixed). Signed-off-by: Rhuan Barreto --- ...alidate-workflow-syntax-with-actionlint.md | 118 ++++++++++++++++++ .../agent-memory/archgate-developer/MEMORY.md | 5 +- .../project_cli_skill_flag_sequencing.md | 2 + .../project_pr_review_thread_triage.md | 33 +++++ .github/workflows/code-pull-request.yml | 31 +++++ CLAUDE.md | 4 +- 6 files changed, 189 insertions(+), 4 deletions(-) create mode 100644 .archgate/adrs/CI-002-validate-workflow-syntax-with-actionlint.md create mode 100644 .claude/agent-memory/archgate-developer/project_pr_review_thread_triage.md 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 1b7da7de..01da7a7c 100644 --- a/.claude/agent-memory/archgate-developer/MEMORY.md +++ b/.claude/agent-memory/archgate-developer/MEMORY.md @@ -4,7 +4,7 @@ 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. @@ -43,7 +43,8 @@ Non-enforceable lessons — environment/CI/platform quirks no static rule can re - [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 +- [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 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 index 4f5af613..ade46395 100644 --- a/.claude/agent-memory/archgate-developer/project_cli_skill_flag_sequencing.md +++ b/.claude/agent-memory/archgate-developer/project_cli_skill_flag_sequencing.md @@ -12,3 +12,5 @@ Distributed editor skills (e.g. the opencode lessons-learned skill) reference sp - 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_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/.github/workflows/code-pull-request.yml b/.github/workflows/code-pull-request.yml index 71ec72b6..a1ea00a6 100644 --- a/.github/workflows/code-pull-request.yml +++ b/.github/workflows/code-pull-request.yml @@ -211,6 +211,34 @@ jobs: config: zizmor.yml advanced-security: ${{ github.event.pull_request.head.repo.fork != true }} + # Validates workflow YAML against GitHub's actual permissions/expression + # schema (invalid permission scopes, bad expression syntax, shellcheck + # issues in run: blocks). zizmor covers security patterns; neither tool + # covers the other's ground — actionlint would have caught ARCH-022's + # PR #451 `workflows: write` invalid-permission-scope bug before merge. + 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 to a specific commit of the maintainer's own download + # script (which verifies its own checksums) rather than a mutable + # branch ref, and to a specific actionlint version rather than + # "latest", for reproducibility — mirrors the spirit of CI-001 + # even though this is a script fetch, not a `uses:` reference. + 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 +300,7 @@ jobs: smoke-windows, smoke-linux, zizmor, + actionlint, coverage, ] steps: @@ -286,6 +315,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 +323,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/CLAUDE.md b/CLAUDE.md index 97b9c34e..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+) From 70043f5b711feb76e9540c7f905803345d517441 Mon Sep 17 00:00:00 2001 From: Rhuan Barreto Date: Sat, 4 Jul 2026 10:57:05 -0300 Subject: [PATCH 13/15] fix(ci): resolve pre-existing shellcheck findings surfaced by actionlint MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit My local verification of the new actionlint job used a Windows binary that silently skips shellcheck (not bundled/found on that platform), so it passed cleanly while the real Linux CI job failed against three pre-existing shellcheck findings in files this PR didn't otherwise touch. Re-verified with the actual Linux actionlint image via Docker (rhysd/actionlint:1.7.12) before pushing this time. - code-pull-request.yml (SC2086): PATHS was an unquoted space-joined string passed to `git diff -- $PATHS`, relying on word-splitting. Converted to a bash array (PATHS=(...)) expanded as "${PATHS[@]}" — preserves the intended multi-path behavior without relying on unquoted splitting. - release-binaries.yml (SC2035): `cat *.sha256` could misinterpret a glob result starting with `-` as an option. Changed to `cat ./*.sha256`. - smoke-test-linux.yml (SC2155): `export VAR="$(cmd)"` masks the command's exit status. Split into assignment then `export VAR`. Signed-off-by: Rhuan Barreto --- .github/workflows/code-pull-request.yml | 6 +++--- .github/workflows/release-binaries.yml | 4 ++-- .github/workflows/smoke-test-linux.yml | 3 ++- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/.github/workflows/code-pull-request.yml b/.github/workflows/code-pull-request.yml index a1ea00a6..648214ab 100644 --- a/.github/workflows/code-pull-request.yml +++ b/.github/workflows/code-pull-request.yml @@ -90,11 +90,11 @@ jobs: # 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/${{ github.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" 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 From a248c231adb7195330c81e0f6bc8eaf78ff726de Mon Sep 17 00:00:00 2001 From: Rhuan Barreto Date: Sat, 4 Jul 2026 12:01:16 -0300 Subject: [PATCH 14/15] chore(ci): trim overlong inline comments now redundant with CI-002/memory Per the project's own concise-comments convention: the workflows:write removal comment and the actionlint job comments restated detail that now lives in CI-002 and project_release_pipeline_gotchas.md. Cut to one-line pointers. Signed-off-by: Rhuan Barreto --- .github/workflows/code-pull-request.yml | 12 ++---------- .github/workflows/publish-shims.yml | 12 ++---------- 2 files changed, 4 insertions(+), 20 deletions(-) diff --git a/.github/workflows/code-pull-request.yml b/.github/workflows/code-pull-request.yml index 648214ab..50e83032 100644 --- a/.github/workflows/code-pull-request.yml +++ b/.github/workflows/code-pull-request.yml @@ -211,11 +211,7 @@ jobs: config: zizmor.yml advanced-security: ${{ github.event.pull_request.head.repo.fork != true }} - # Validates workflow YAML against GitHub's actual permissions/expression - # schema (invalid permission scopes, bad expression syntax, shellcheck - # issues in run: blocks). zizmor covers security patterns; neither tool - # covers the other's ground — actionlint would have caught ARCH-022's - # PR #451 `workflows: write` invalid-permission-scope bug before merge. + # Validates workflow schema (zizmor covers security patterns only). See CI-002. actionlint: name: Actionlint (Workflow Schema) runs-on: ubuntu-latest @@ -229,11 +225,7 @@ jobs: with: persist-credentials: false - name: Install actionlint - # Pinned to a specific commit of the maintainer's own download - # script (which verifies its own checksums) rather than a mutable - # branch ref, and to a specific actionlint version rather than - # "latest", for reproducibility — mirrors the spirit of CI-001 - # even though this is a script fetch, not a `uses:` reference. + # 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 diff --git a/.github/workflows/publish-shims.yml b/.github/workflows/publish-shims.yml index 6caf6cd7..51cf7c79 100644 --- a/.github/workflows/publish-shims.yml +++ b/.github/workflows/publish-shims.yml @@ -108,16 +108,8 @@ jobs: needs: wait-for-binaries runs-on: ubuntu-latest timeout-minutes: 5 - # contents: write is required to push a tag. NOTE: "workflows" is not a - # valid permissions-key scope (confirmed against GitHub's own workflow - # syntax docs and actionlint's schema) and has been removed — it was - # never actually granting anything. If GITHUB_TOKEN's push is rejected - # again with "refusing to allow a GitHub App to create or update - # workflow ... without workflows permission" (seen on v0.45.7), the - # real fix is a PAT with the classic `workflow` OAuth scope stored as a - # secret for this step, or a GitHub App installation token whose App is - # explicitly configured with the Workflows permission — neither is - # expressible via this YAML key. + # contents: write is required to push a tag. See + # project_release_pipeline_gotchas.md for why there's no "workflows" scope. permissions: contents: write steps: From 0273222ba5fae379e99f7dd3c9c89e381a7badda Mon Sep 17 00:00:00 2001 From: Rhuan Barreto Date: Sat, 4 Jul 2026 21:48:07 -0300 Subject: [PATCH 15/15] fix(ci): pass base_ref via env to avoid zizmor template-injection flag Inlining `${{ github.base_ref }}` directly into the shim-changes run: block tripped zizmor's template-injection check as a new error-level finding on the PR. Pass it through env instead. Signed-off-by: Rhuan Barreto --- .github/workflows/code-pull-request.yml | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/.github/workflows/code-pull-request.yml b/.github/workflows/code-pull-request.yml index 50e83032..767b79e0 100644 --- a/.github/workflows/code-pull-request.yml +++ b/.github/workflows/code-pull-request.yml @@ -85,6 +85,10 @@ 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 @@ -92,7 +96,7 @@ jobs: # 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) 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) fi