From 29228cfb1921061e768748b96ff06403304741fd Mon Sep 17 00:00:00 2001 From: Eric Andrechek Date: Fri, 15 May 2026 13:59:34 -0400 Subject: [PATCH 01/18] chore: clean drift in AI rules + meta; remove TODO.md - 17 doc-path references corrected to docs/src/content/docs/*.md (actual Astro Starlight content collection location) across AGENTS.md and CONTRIBUTING.md - .github/copilot-instructions.md shrunk to pointer (was drifting on Go 1.25 vs go.mod 1.26.3; "60% coverage" vs .testcoverage.yml 80% total / 70% unit) - .gemini/styleguide.md: stale #67/60% reference fixed (#67 closed, 70% restored); duplicated doc-sync list collapsed to defer to AGENTS.md table - .github/labeler.yml: dropped non-existent cmd/wavehouse-{api, worker}/** entries; fixed tests/compose.yaml -> tests/e2e/ compose.yaml; tests/sdk/** -> tests/e2e/sdk/**; added cmd/wavehouse/** to area/infra - .github/prompts/pr-review.md: doc-sync list collapsed; path fix; vestigial "tenant" wording dropped (no tenant model); hard-wrap reflowed (180 -> 81 lines) - AGENTS.md: cmd/*/main.go (plural) -> cmd/wavehouse/main.go (one binary); tests/fixtures/ -> tests/e2e/fixtures/; removed stale "update triage.yml area enumeration" step (workflow discovers area/* labels dynamically via gh label list); package count fixed; tooling notes + make-help intro reflowed - TODO.md deleted (audited against open + closed issues; orphan items split into follow-up issues; Projects #7 board + triage automation is now the single canonical backlog) Co-Authored-By: Claude Opus 4.7 (1M context) --- .gemini/styleguide.md | 12 +- .github/copilot-instructions.md | 23 +--- .github/labeler.yml | 7 +- .github/prompts/pr-review.md | 209 +++++++++----------------------- AGENTS.md | 60 ++++----- CONTRIBUTING.md | 10 +- TODO.md | 61 ---------- 7 files changed, 91 insertions(+), 291 deletions(-) delete mode 100644 TODO.md diff --git a/.gemini/styleguide.md b/.gemini/styleguide.md index 5806a9b9..640ae289 100644 --- a/.gemini/styleguide.md +++ b/.gemini/styleguide.md @@ -57,19 +57,11 @@ Severity-tag every security finding `CRITICAL` / `HIGH` / `MEDIUM` / `LOW`. - Missing edge-case coverage (nil inputs, empty batches, cancelled contexts, invalid JWT) - Mocks where an integration test would catch more (per `AGENTS.md` testing conventions) - Tests that don't actually exercise the path they claim -- Coverage ≥60% is CI-enforced (interim; #67 tracks restoring 70%); flag drops below threshold as `[MUST]` +- CI gates coverage against the thresholds in `.testcoverage.yml` (project-wide total + per-suite). Flag drops below threshold as `[MUST]` ### 5. Documentation sync — `[MUST]` when missed -`AGENTS.md` §"Documentation & Consistency Sync (MANDATORY)" lists exactly which docs must update for which code changes. Diff the changed files against that table: - -- Handler / router changes → `docs/api.md`, README if user-facing -- Config struct / env var → `docs/configuration.md`, `config.yaml`, compose files, `docs/deployment.md` -- Architecture / package changes → `docs/architecture.md`, `AGENTS.md` -- Notable changes → `CHANGELOG.md` under `[Unreleased]` -- Build / test process → `docs/development.md`, `Makefile` - -Flag every missed sync. +`AGENTS.md` §"Documentation Sync" lists exactly which docs must update for which code changes (handler/router, config, architecture, build/test, etc.). Diff the changed files against that table and flag every missed sync. ### 6. Go idiom — `[SHOULD]` for quality diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 03b18644..f93bb1f7 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -1,22 +1,3 @@ -# GitHub Copilot Instructions +See [AGENTS.md](../AGENTS.md) for project conventions, architecture notes, and AI agent instructions. -For detailed project context, architecture, code conventions, and common tasks, see [AGENTS.md](../AGENTS.md) in the project root. - -## Quick Reference - -- **Language**: Go 1.25, strict formatting (`gofumpt`) -- **Router**: Chi v5 (`github.com/go-chi/chi/v5`) -- **Logging**: `log/slog` with JSON handler -- **Testing**: `testing` + `testify` for assertions -- **Linting**: golangci-lint v2 (see `.golangci.yml`) - -## Key Rules - -1. **Tenant ID comes only from JWT** — never read `tenant_id` from request body, query params, or headers. -2. **Interface-first** — define interfaces where consumed, implement separately for standalone and (future) clustered mode. -3. **Return errors, don't panic** — wrap with `fmt.Errorf("context: %w", err)`. -4. **MANDATORY: Keep ALL docs, configs, and examples in sync** — every code change MUST include updates to all affected documentation, config files, Docker Compose files, examples, and CHANGELOG.md. Do NOT wait for the user to ask. See the full "Documentation & Consistency Sync" section in [AGENTS.md](../AGENTS.md) for the complete checklist and cross-referencing rules. A code change without its documentation counterpart is considered incomplete. -5. **Verify before finishing** — before completing any task, search docs for identifiers you touched (field names, env vars, endpoints, struct names) and fix any stale references. -6. **Every new function must have tests** — use table-driven tests, shared mocks from `internal/testutil/`, and aim for 80%+ coverage. Run `make lint` and `make test` before considering work complete. -7. **Use testutil helpers** — use `MockPublisher`, `MockCache`, `MockDeduplicator`, `MockSubscriber` from `testutil/mocks.go` instead of creating ad-hoc mocks. Use `testutil.MakeJWT()` for auth tests. Use `policy.NewMemoryStore(p)` for policy tests. Use `pipes.NewMemoryStore(queries...)` for pipes tests. -8. **Validate locally before pushing** — run `make ci` (or at minimum `make coverage`) in your sandbox and confirm it passes before opening or updating a PR. The repo runs on a shared self-hosted runner pool with finite throughput and bills AI-reviewer credits per push; a speculative commit to "see what CI says" costs real minutes and dollars. If a test passes locally but flakes on CI, investigate the runner environment before patching the test — see the "Local-First Validation" section in [AGENTS.md](../AGENTS.md) for full guidance. +AGENTS.md is the single source of truth. This file exists as a pointer for GitHub Copilot, which looks for `.github/copilot-instructions.md` by convention. diff --git a/.github/labeler.yml b/.github/labeler.yml index 88dd50a7..4df42c01 100644 --- a/.github/labeler.yml +++ b/.github/labeler.yml @@ -7,13 +7,11 @@ - changed-files: - any-glob-to-any-file: - "internal/api/**" - - "cmd/wavehouse-api/**" "area/ingest": - changed-files: - any-glob-to-any-file: - "internal/ingest/**" - - "cmd/wavehouse-worker/**" "area/query": - changed-files: @@ -45,7 +43,7 @@ - changed-files: - any-glob-to-any-file: - "clients/ts/**" - - "tests/sdk/**" + - "tests/e2e/sdk/**" "area/docs": - changed-files: @@ -67,12 +65,13 @@ - "Dockerfile*" - ".goreleaser.yaml" - "deployments/**" - - "tests/compose.yaml" + - "tests/e2e/compose.yaml" - ".golangci.yml" - ".air.toml" - ".markdownlint.json" - "go.mod" - "go.sum" + - "cmd/wavehouse/**" "area/observability": - changed-files: diff --git a/.github/prompts/pr-review.md b/.github/prompts/pr-review.md index 8c5bd1ef..0d31ed44 100644 --- a/.github/prompts/pr-review.md +++ b/.github/prompts/pr-review.md @@ -1,180 +1,81 @@ -You are reviewing a pull request on the WaveHouse project. -Read AGENTS.md at the repo root first — it has the -architectural context, code conventions, and documentation -sync rules that inform every review. +You are reviewing a pull request on the WaveHouse project. Read AGENTS.md at the repo root first — it has the architectural context, code conventions, and documentation sync rules that inform every review. ## What to read before reviewing -The PR number you're reviewing is in the header above. Before -forming an opinion, read the **whole PR**, not just the most -recent commit: - -1. **The full PR diff (HEAD vs the merge base with the target - branch)** — `gh pr diff ` or read each changed file at - the PR head SHA. **Do NOT only review the latest commit's - diff.** Reviews focused on the latest push miss issues that - prior commits introduced and that the latest commit didn't - touch. The merge-base diff is the unit of review; a commit - is just a checkpoint inside it. - -2. **All prior comments and reviews on this PR** — - `gh pr view --json comments,reviews` (and the inline - review-comment endpoint - `gh api repos//pulls//comments` for line-level - comments). If a reviewer (human or other bot) already - flagged something, don't re-flag it; either acknowledge - their finding briefly or add nuance. If the author replied - to a concern, factor in their reply — don't re-raise a - resolved point. - -3. **Linked issues** — anything referenced in the PR body via - `Closes #N` / `Fixes #N` / `Resolves #N`. The acceptance - criteria for the PR live in the linked issue; review against - those, not just against the diff in isolation. - -4. **CI run logs** — only when the diff touches workflow / build - / test infrastructure (`.github/`, `Makefile`, `Dockerfile*`, - `tests/compose.yaml`). Read the most recent CI run on this - PR's head SHA; flag anything that passed CI but looks - suspicious in the logs. - -The action's environment provides `gh` CLI authenticated to -this repo, so the commands above all work directly. +The PR number you're reviewing is in the header above. Before forming an opinion, read the **whole PR**, not just the most recent commit: + +1. **The full PR diff (HEAD vs the merge base with the target branch)** — `gh pr diff ` or read each changed file at the PR head SHA. **Do NOT only review the latest commit's diff.** Reviews focused on the latest push miss issues that prior commits introduced and that the latest commit didn't touch. The merge-base diff is the unit of review; a commit is just a checkpoint inside it. + +2. **All prior comments and reviews on this PR** — `gh pr view --json comments,reviews` (and the inline review-comment endpoint `gh api repos//pulls//comments` for line-level comments). If a reviewer (human or other bot) already flagged something, don't re-flag it; either acknowledge their finding briefly or add nuance. If the author replied to a concern, factor in their reply — don't re-raise a resolved point. + +3. **Linked issues** — anything referenced in the PR body via `Closes #N` / `Fixes #N` / `Resolves #N`. The acceptance criteria for the PR live in the linked issue; review against those, not just against the diff in isolation. + +4. **CI run logs** — only when the diff touches workflow / build / test infrastructure (`.github/`, `Makefile`, `Dockerfile*`, `tests/e2e/compose.yaml`). Read the most recent CI run on this PR's head SHA; flag anything that passed CI but looks suspicious in the logs. + +The action's environment provides `gh` CLI authenticated to this repo, so the commands above all work directly. ## Tone -Be a rigorous, skeptical staff engineer. Assume the worst -about every change until the diff convinces you otherwise. -"Could this break in production?" "What's the failure -mode?" "What about on a restart, during a deploy, under -load?" Err on the side of flagging a concern — a false -positive is cheap (reply with a rebuttal), a missed real -issue is expensive. +Be a rigorous, skeptical staff engineer. Assume the worst about every change until the diff convinces you otherwise. "Could this break in production?" "What's the failure mode?" "What about on a restart, during a deploy, under load?" Err on the side of flagging a concern — a false positive is cheap (reply with a rebuttal), a missed real issue is expensive. -Be specific and constructive. Cite file/line and suggest -a concrete remediation whenever possible; don't leave -vague "consider refactoring" notes. If the code is -genuinely good, say so briefly — don't invent complaints. +Be specific and constructive. Cite file/line and suggest a concrete remediation whenever possible; don't leave vague "consider refactoring" notes. If the code is genuinely good, say so briefly — don't invent complaints. ## Focus areas Review against each of these, in this order: -1. **Correctness** — Go concurrency (goroutine leaks, data - races, missing context propagation, channel leaks, - `sync.Once` / `sync.Map` misuse, handlers ignoring - `r.Context()`), error wrapping with `%w`, resource - cleanup on every error path (`defer` that ignores Close - errors is OK when intentional, but flag if the error - could mask data loss), invariants preserved (schema - validation before DB writes, policy enforcement, - tenant/role isolation). +1. **Correctness** — Go concurrency (goroutine leaks, data races, missing context propagation, channel leaks, `sync.Once` / `sync.Map` misuse, handlers ignoring `r.Context()`), error wrapping with `%w`, resource cleanup on every error path (`defer` that ignores Close errors is OK when intentional, but flag if the error could mask data loss), invariants preserved (schema validation before DB writes, policy enforcement, role isolation). 2. **Security** — walk the OWASP Top 10 against the diff: - - SQL injection in any ClickHouse-bound path - (`BindParams`, query builders, dynamic table names) - - Broken authentication / authorization (JWT claim - handling, role extraction, policy templating) - - Sensitive data exposure (secrets in logs, error - messages leaking internal state) - - Broken access control (policy bypass, raw-SQL - without `raw_sql: true` permission) - - Security misconfiguration (CORS, TLS, default - credentials, permissive defaults) - - Insufficient logging / monitoring - - SSRF, XXE, deserialization flaws if touched - - Hardcoded secrets or credentials - - TOCTOU / race conditions in auth or policy paths - Rate every security finding with severity: `CRITICAL`, - `HIGH`, `MEDIUM`, `LOW`, or `NONE` and include it in the - comment. Flag `CRITICAL` / `HIGH` prominently in the - summary. - -3. **Performance** — hot-path allocation in ingest / query / - cache, unbounded goroutine spawns, unbatched DB work, - missing caching where cost is high, locks held across - I/O, N+1 query patterns, singleflight misuse. - -4. **Testing** — new code without tests (especially on - critical paths: auth middleware, ingest pipeline, policy - evaluation, structured query builder, cache coherence, - dedup). Missing edge-case coverage. Mocks where a real - integration test would catch more (per the "no mocking - DB" rule in the test conventions). Unit tests that - don't actually exercise the code path they claim to. - -5. **Documentation & doc-sync** — AGENTS.md has a hard rule - that code changes affecting API / config / architecture - / event format must update `docs/api.md`, - `docs/configuration.md`, `docs/architecture.md`, - `docs/deployment.md`, `CHANGELOG.md` (under - `[Unreleased]`), and the compose files / `config.yaml` - defaults. Flag every missed sync. The table in - AGENTS.md §"Documentation & Consistency Sync" is - authoritative — diff changed files against that map. + + - SQL injection in any ClickHouse-bound path (`BindParams`, query builders, dynamic table names) + - Broken authentication / authorization (JWT claim handling, role extraction, policy templating) + - Sensitive data exposure (secrets in logs, error messages leaking internal state) + - Broken access control (policy bypass, raw-SQL without `raw_sql: true` permission) + - Security misconfiguration (CORS, TLS, default credentials, permissive defaults) + - Insufficient logging / monitoring + - SSRF, XXE, deserialization flaws if touched + - Hardcoded secrets or credentials + - TOCTOU / race conditions in auth or policy paths + + Rate every security finding with severity: `CRITICAL`, `HIGH`, `MEDIUM`, `LOW`, or `NONE` and include it in the comment. Flag `CRITICAL` / `HIGH` prominently in the summary. + +3. **Performance** — hot-path allocation in ingest / query / cache, unbounded goroutine spawns, unbatched DB work, missing caching where cost is high, locks held across I/O, N+1 query patterns, singleflight misuse. + +4. **Testing** — new code without tests (especially on critical paths: auth middleware, ingest pipeline, policy evaluation, structured query builder, cache coherence, dedup). Missing edge-case coverage. Mocks where a real integration test would catch more (per the "no mocking DB" rule in the test conventions). Unit tests that don't actually exercise the code path they claim to. + +5. **Documentation & doc-sync** — AGENTS.md has a hard rule that code changes affecting API / config / architecture / event format / deployment must update the corresponding docs, `CHANGELOG.md` (under `[Unreleased]`), and the compose files / `config.yaml` defaults. The table in AGENTS.md §"Documentation Sync" is authoritative — diff changed files against that map and flag every missed sync. ## Output discipline -**Use inline review comments for specific line-level -findings.** Call `mcp__github_inline_comment__create_inline_comment` -with `confirmed: true` for each concrete issue. These become -real PR review threads that show next to the line in the diff, -must be resolved before merge (the repo's ruleset has -`required_review_thread_resolution: true`), and are the same -mechanism Gemini Code Assist uses. *Do not* dump every -finding into one giant prose blob — that pattern caused the -sticky comment to bloat. - -**Tag every inline comment with exactly one severity** at the -start of the body: `[MUST]`, `[SHOULD]`, or `[MAY]`. This -matches `.gemini/styleguide.md` so both reviewers speak the -same language and the author can filter on tag. - - - `[MUST]` — correctness bug, security issue, broken - invariant, missing required documentation sync. The PR - can't merge until this is addressed. - - `[SHOULD]` — quality / maintainability issue the author - should fix, but isn't a release blocker if they push back - with reasoning. - - `[MAY]` — minor suggestion, style nit, alternative - approach. Take or leave. - -**Use the sticky summary comment for the verdict only.** One -short top-level comment with: - - A one-line headline grouping findings by severity (e.g. - "2 [MUST], 1 [SHOULD], 0 [MAY]"). - - A pointer to read the inline threads for detail. - - The verdict line, **exactly one of**: `Ship it`, - `Iterate`, or `Block`, followed by the single most - important thing the author must address. +**Use inline review comments for specific line-level findings.** Call `mcp__github_inline_comment__create_inline_comment` with `confirmed: true` for each concrete issue. These become real PR review threads that show next to the line in the diff, must be resolved before merge (the repo's ruleset has `required_review_thread_resolution: true`), and are the same mechanism Gemini Code Assist uses. *Do not* dump every finding into one giant prose blob — that pattern caused the sticky comment to bloat. + +**Tag every inline comment with exactly one severity** at the start of the body: `[MUST]`, `[SHOULD]`, or `[MAY]`. This matches `.gemini/styleguide.md` so both reviewers speak the same language and the author can filter on tag. + +- `[MUST]` — correctness bug, security issue, broken invariant, missing required documentation sync. The PR can't merge until this is addressed. +- `[SHOULD]` — quality / maintainability issue the author should fix, but isn't a release blocker if they push back with reasoning. +- `[MAY]` — minor suggestion, style nit, alternative approach. Take or leave. + +**Use the sticky summary comment for the verdict only.** One short top-level comment with: + +- A one-line headline grouping findings by severity (e.g. "2 [MUST], 1 [SHOULD], 0 [MAY]"). +- A pointer to read the inline threads for detail. +- The verdict line, **exactly one of**: `Ship it`, `Iterate`, or `Block`, followed by the single most important thing the author must address. Verdict rules (matched to the styleguide): - - `Block` — a `[MUST]` finding that's a CRITICAL/HIGH - security issue, data-loss risk, or broken core - invariant. Cannot proceed without addressing. - - `Iterate` — one or more `[MUST]` findings that aren't - `Block`-level, or multiple `[SHOULD]` findings that - collectively need a pass. - - `Ship it` — no `[MUST]`s and few or no `[SHOULD]`s. - `[MAY]` findings alone don't preclude `Ship it`. + +- `Block` — a `[MUST]` finding that's a CRITICAL/HIGH security issue, data-loss risk, or broken core invariant. Cannot proceed without addressing. +- `Iterate` — one or more `[MUST]` findings that aren't `Block`-level, or multiple `[SHOULD]` findings that collectively need a pass. +- `Ship it` — no `[MUST]`s and few or no `[SHOULD]`s. `[MAY]` findings alone don't preclude `Ship it`. What not to comment on: - - Anything the linter already catches (gofumpt, govet, - staticcheck, gosec, gocritic, errcheck, etc. — see - `.golangci.yml`). CI enforces those. - - Self-explanatory code — this project prefers well-named - identifiers over explanatory comments. - - Don't post `gh pr comment` floors of prose for findings - that belong on a specific line — use an inline comment - there instead. + +- Anything the linter already catches (gofumpt, govet, staticcheck, gosec, gocritic, errcheck, etc. — see `.golangci.yml`). CI enforces those. +- Self-explanatory code — this project prefers well-named identifiers over explanatory comments. +- Don't post `gh pr comment` floors of prose for findings that belong on a specific line — use an inline comment there instead. ## Noise filter (important) -Before posting, re-read every finding you wrote and drop -the ones you wouldn't personally ask the author to change -if you were reviewing in-person. Quality of feedback beats -quantity. Follow this rule from Anthropic's `/review-pr`: -*"Review the feedback and post only the feedback that you -also deem noteworthy. Keep feedback concise."* +Before posting, re-read every finding you wrote and drop the ones you wouldn't personally ask the author to change if you were reviewing in-person. Quality of feedback beats quantity. Follow this rule from Anthropic's `/review-pr`: *"Review the feedback and post only the feedback that you also deem noteworthy. Keep feedback concise."* Do not make code changes — this is a review only. diff --git a/AGENTS.md b/AGENTS.md index 3232dfb2..71b0cf0c 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -12,7 +12,7 @@ One binary: - **`cmd/wavehouse/`** — Standalone mode (all-in-one with embedded NATS, optional Pebble dedup) -Twelve internal packages under `internal/`: +Eleven internal packages under `internal/` (plus `internal/testutil/` for shared test helpers): - **`api/`** — Chi HTTP router, JWT/JWKS middleware, ingest/query/structured-query/SSE/WS/schema/DLQ/policy/pipes handlers, Hub - **`cache/`** — `Cache` interface → `LocalCache` (Ristretto) + `SharedCache` (TBD) + `TieredCache` (singleflight) @@ -56,8 +56,7 @@ Twelve internal packages under `internal/`: ## Build & Test Commands -`make help` is the source of truth — run it to see every target with its -one-line description. Common targets, grouped: +`make help` is the source of truth — run it to see every target with its one-line description. Common targets, grouped: ```bash # Setup @@ -121,20 +120,10 @@ Build tags: `make build TAGS="foo bar"`. Tooling notes: -- Most dev tools (`gotestsum`, `gofumpt`, `goimports`, `govulncheck`, - `go-test-coverage`, `deadcode`, `gsa`, `goda`) are pinned in `go.mod` via - native `tool` directives and invoked with `go tool ` — no manual - install needed. -- `golangci-lint` is pinned in the Makefile (currently v2.11.4) and - auto-installed to `.bin/_/` on first `make lint` (or via - `make tools`). Not in `go.mod` — its dependency tree conflicts with the - main module. -- `pnpm` (>= 10.33) and `Node.js` (>= 20) must be on your PATH; the SDK and - E2E test harnesses both shell out to `pnpm`. `make tools` runs - `pnpm install --frozen-lockfile` in `clients/ts/` and `tests/e2e/sdk/`. -- `GNU Make 4+` is required (uses `--output-sync=target`); macOS ships BSD - Make 3.81 which will not parse the Makefile. See `docs/development.md` § - Prerequisites for the full setup checklist. +- Most dev tools (`gotestsum`, `gofumpt`, `goimports`, `govulncheck`, `go-test-coverage`, `deadcode`, `gsa`, `goda`) are pinned in `go.mod` via native `tool` directives and invoked with `go tool ` — no manual install needed. +- `golangci-lint` is pinned in the Makefile (currently v2.11.4) and auto-installed to `.bin/_/` on first `make lint` (or via `make tools`). Not in `go.mod` — its dependency tree conflicts with the main module. +- `pnpm` (>= 10.33) and `Node.js` (>= 20) must be on your PATH; the SDK and E2E test harnesses both shell out to `pnpm`. `make tools` runs `pnpm install --frozen-lockfile` in `clients/ts/` and `tests/e2e/sdk/`. +- `GNU Make 4+` is required (uses `--output-sync=target`); macOS ships BSD Make 3.81 which will not parse the Makefile. See `docs/src/content/docs/development.md` § Prerequisites for the full setup checklist. ## Testing Conventions @@ -207,20 +196,20 @@ Every code change should update the corresponding docs in the same PR. A code ch | Change | Files to update | | ------ | --------------- | -| Add/modify API endpoint | `docs/api.md`, `README.md` (if user-facing) | -| Add/modify config option | `docs/configuration.md`, `config.yaml`, `deployments/compose/*` env blocks, `docs/deployment.md` | -| Change architecture / add a package | `docs/architecture.md`, `AGENTS.md` | -| Change ingest / event format | `docs/api.md`, `docs/deployment.md` (CH schema) | -| Change deployment / Docker | `docs/deployment.md`, compose files | -| Change build / test process | `docs/development.md`, `Makefile` | +| Add/modify API endpoint | `docs/src/content/docs/api.md`, `README.md` (if user-facing) | +| Add/modify config option | `docs/src/content/docs/configuration.md`, `config.yaml`, `deployments/compose/*` env blocks, `docs/src/content/docs/deployment.md` | +| Change architecture / add a package | `docs/src/content/docs/architecture.md`, `AGENTS.md` | +| Change ingest / event format | `docs/src/content/docs/api.md`, `docs/src/content/docs/deployment.md` (CH schema) | +| Change deployment / Docker | `docs/src/content/docs/deployment.md`, compose files | +| Change build / test process | `docs/src/content/docs/development.md`, `Makefile` | | Any notable change | `CHANGELOG.md` under `[Unreleased]` | Source-of-truth pairs that must agree: -- Config struct tags in `internal/config/config.go` ↔ `docs/configuration.md`, `config.yaml`, compose env blocks -- `EventMessage` JSON tags ↔ `docs/api.md` event format, SSE/WS examples, ClickHouse INSERT columns -- Route registrations in `router.go` ↔ `docs/api.md` endpoint list -- Handler error responses ↔ `docs/api.md` error tables +- Config struct tags in `internal/config/config.go` ↔ `docs/src/content/docs/configuration.md`, `config.yaml`, compose env blocks +- `EventMessage` JSON tags ↔ `docs/src/content/docs/api.md` event format, SSE/WS examples, ClickHouse INSERT columns +- Route registrations in `router.go` ↔ `docs/src/content/docs/api.md` endpoint list +- Handler error responses ↔ `docs/src/content/docs/api.md` error tables Before finishing a task, grep for the identifiers you touched (field names, env var names, endpoint paths) across docs to catch staleness. @@ -231,24 +220,23 @@ Before finishing a task, grep for the identifiers you touched (field names, env 1. Create or modify a handler in `internal/api/` (follow existing patterns like `ingest.go`). 2. Register the route in `internal/api/router.go`. 3. If it needs new dependencies, add to the `Dependencies` struct in `router.go`. -4. Wire dependencies in the relevant `cmd/*/main.go` file(s). +4. Wire dependencies in `cmd/wavehouse/main.go`. 5. Add tests. -6. Document in `docs/api.md`. +6. Document in `docs/src/content/docs/api.md`. ### Adding a new config option 1. Add the field to the appropriate struct in `internal/config/config.go` with `yaml`, `env`, and `env-default` tags. -2. Use the new config value in the relevant `cmd/*/main.go` or internal package. -3. Document in `docs/configuration.md`. +2. Use the new config value in `cmd/wavehouse/main.go` or the relevant internal package. +3. Document in `docs/src/content/docs/configuration.md`. ### Adding a new internal package 1. Create the package under `internal/`. 2. Define an interface if there will be multiple implementations. -3. Wire it into the appropriate `cmd/*/main.go`. -4. Document in `docs/architecture.md`. -5. **Add a matching `area/` repo label** (e.g. `area/foo` for `internal/foo/`) so the issue triage workflow can route issues to it. -6. **Update the area enumeration** in `.github/workflows/triage.yml` (the `system-prompt:` block lists every legal area the LLM is allowed to return). Without this, the triager can't categorize issues about the new package. +3. Wire it into `cmd/wavehouse/main.go`. +4. Document in `docs/src/content/docs/architecture.md`. +5. **Add a matching `area/` repo label** (e.g. `area/foo` for `internal/foo/`) so the issue triage workflow can route issues to it. `triage.yml` discovers `area/*` labels at runtime via `gh label list`, so the new label is picked up automatically — no workflow edit needed. ### Writing tests @@ -278,8 +266,8 @@ internal/query/ → Structured query AST + SQL builder internal/testutil/ → Shared test helpers (NopLogger, etc.) tests/ → Integration & E2E tests tests/integration/ → Go integration tests (//go:build integration; ClickHouse testcontainer) -tests/fixtures/ → Idempotent ClickHouse DDL scripts for test tables tests/e2e/ → E2E test stack +tests/e2e/fixtures/ → Idempotent ClickHouse DDL scripts for test tables tests/e2e/compose.yaml → Docker Compose with profiles (ClickHouse always; WaveHouse via --profile app) tests/e2e/sdk/ → E2E integration tests via TypeScript SDK (Vitest) deployments/compose/ → Docker Compose files diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 70425148..c0abfd98 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -5,7 +5,7 @@ Thank you for your interest in contributing to WaveHouse! This guide will help y ## Getting Started 1. [Fork the repository](https://github.com/Wave-RF/WaveHouse/fork) and clone your fork. -2. Follow the [Development Guide](docs/development.md) to set up your local environment. +2. Follow the [Development Guide](docs/src/content/docs/development.md) to set up your local environment. 3. Create a feature branch: `git checkout -b feat/my-feature` ## How to Contribute @@ -40,10 +40,10 @@ Open a [feature request issue](https://github.com/Wave-RF/WaveHouse/issues/new?t 2. Write tests for new functionality. Unit tests go alongside the code in `internal/`. Integration tests go in `tests/` with the `//go:build integration` tag. 3. Update documentation if your change affects: - - API endpoints → update `docs/api.md` - - Configuration options → update `docs/configuration.md` - - Deployment → update `docs/deployment.md` - - Architecture → update `docs/architecture.md` + - API endpoints → update `docs/src/content/docs/api.md` + - Configuration options → update `docs/src/content/docs/configuration.md` + - Deployment → update `docs/src/content/docs/deployment.md` + - Architecture → update `docs/src/content/docs/architecture.md` 4. Follow the commit message format (see below). diff --git a/TODO.md b/TODO.md deleted file mode 100644 index 7f0fd66d..00000000 --- a/TODO.md +++ /dev/null @@ -1,61 +0,0 @@ - -- [ ] Observability: - - [ ] Metrics: - - [ ] Configure WaveHouse to be the single source of truth for metrics. - - [ ] Write lightweight adapter code in WaveHouse that periodically reads stats from the embedded NATS server (`server.Varz()`), Pebble (`db.Metrics()`), and Bento, and registers them into your central OpenTelemetry `MeterProvider` or Prometheus registry. - - [ ] Expose a single `/metrics` endpoint on port `8080` (or push via a single OTLP pipeline). - - [ ] Prefix the metrics cleanly so users know what they are looking at: `wavehouse_nats_connections`, `wavehouse_pebble_wal_size`, `wavehouse_bento_events_processed`. - - [ ] Potentially do this via otel (`go.opentelemetry.io/otel/metric`) and support exporting via push to otel collector, or via a Prometheus scrape endpoint. TBD on whether we want `prometheus/client_golang` for `promhttp` or if we can just use `otel` with a Prometheus exporter. - - [ ] Export go runtime metrics (e.g. GC pause times, goroutine count) as well for additional insight into system health. - - [ ] Work out exactly what custom metrics we want to emit from each component (API, worker, Bento) that would be useful for monitoring and alerting, and ensure those are included in the WaveHouse metrics adapter.cd - - [ ] Traces: - - [ ] Instrument the main API and worker code paths with OpenTelemetry spans. - - [ ] Add spans around key operations like schema discovery, batch flushes, and NATS message processing. - - [ ] Ensure trace context is propagated across API and worker boundaries via NATS messages. - - [ ] Ensure Bento uses the same trace context when processing events and writing to ClickHouse, with a child span under the original API trace. - - [ ] Pass context to `clickhouse-go/v2` so that ClickHouse queries are also traced and show up in the same trace. - - [ ] Logs: - - [ ] Use a structured logging library (slog) to emit JSON logs with consistent fields. - - [ ] Include key context in logs (e.g. request ID, trace ID, batch ID) to correlate with metrics and traces. - - [ ] Include log levels (INFO, WARN, ERROR) and ensure that errors are logged with stack traces for easier debugging. - - [ ] Include component fields in logs (e.g. "api", "worker", "bento") to differentiate log sources. Potentially include a "source" field for more granularity (e.g. "api/ingest_handler", "worker/batch_processor") and maybe even specific function names, file names, or line numbers if it doesn't add too much overhead. - - [ ] Potentially support log sampling or log levels that can be configured at runtime to avoid overwhelming log storage in high-throughput scenarios, while still ensuring that critical errors are always logged. - - [ ] Consider adding support to push logs to a collector through OTLP or a similar protocol, in addition to or instead of writing to stdout, for better integration with centralized logging systems. Could potentially use `slogotel` for this or `slog-multi` to fan out logs to multiple destinations in different more readable (non-JSON) formats for local development. - - [ ] Profiling: - - [ ] Setup `pprof` (expose standard `net/http/pprof` endpoints on admin/internal http port) - - [ ] Health Checks: - - [ ] Implement `/healthz` and `/readyz` endpoints on the API and worker for Kubernetes health checks vs normal `/health` and `/ready` for general health monitoring. - - [ ] Ensure that the health checks verify connectivity to dependencies (e.g. ClickHouse, NATS) and return appropriate status codes. - - [ ] Consider adding more detailed health check endpoints that can provide insights into the status of internal components (e.g. `/healthz/nats`, `/healthz/clickhouse`) for easier debugging in production. - - [ ] Ensure that the health check endpoints are lightweight and do not add significant overhead to the system, especially under high load. - - [ ] Setup dashboard/viewing to see this data locally during development, potentially via a docker container like `grafana/otel-lgtm` or maybe even Signoz for full ClickHouse usage and native OTLP, or `jaegertracing/all-in-one` for tracing or something – likely need more research or to try a couple options here. - - [ ] Build dashboards for given monitoring system for development - - [ ] Document how to run and use all of this - - [ ] Build configuration options for users to customize what ports metrics and health checks are exposed on, whether to push metrics via OTLP or expose a Prometheus endpoint, log levels, etc. -- [ ] CORS setup (allow from whitelisted origins via config, also allow localhost for development), protect CSRF -- [ ] RequireRoles auth middleware needs strict config, not fallback to allow (see comment in code for more) -- [ ] Native SQL templating for named pipes (e.g. `{{ param }}`) with proper escaping and type handling, instead of just doing string replacement on the final SQL -- [ ] Pipes need to use KV Watcher to update in real-time across cluster instead of just on startup -- [ ] Consider custom buffer instead of Bento for smaller binary and dependencies -- [ ] Ack and Nak methods in mq should have context for timeouts and cancellation -- [ ] Move API handlers into their own package for better organization and separation of concerns, and to avoid the `internal/api` package becoming too large and unwieldy as we add more endpoints and features. -- [ ] Get Delve setup working for local development with `make dev` and VS Code Go extension for easier debugging. -- [ ] Add more integration tests covering the full end-to-end flow from API ingestion to ClickHouse, including edge cases and failure scenarios. -- [ ] Add benchmarks for critical code paths (e.g. batch processing, NATS message handling) to monitor performance and catch regressions. -- [x] Setup one docker image with multiple entrypoints for API and worker instead of two separate images, to simplify deployment and reduce maintenance overhead. -- [ ] Docker image multiple architecture support (amd64 + arm64) -- [ ] Docker compose docs for running standalone vs clustered, and for running with different ClickHouse configurations (e.g. single node vs cluster) -- [ ] Docker composes with cleaner deps for standalone vs clustered -- [ ] Gemini in CI for auto code review comments and suggestions, especially for Go code quality and best practices, security issues, and potential bugs. Ideally also have it run some checklist to ensure docs are up to date and comprehensive, changelog is updated, agent files, readmes, etc are updated, and maybe even have it suggest release notes based on the PR description and code changes. -- [ ] Add ldflags (-X) for version and build info in the binary, and expose an endpoint (e.g. `/version`) that returns this information for easier debugging and tracking of deployed versions. -- [ ] Update the Makefile to include targets for building and pushing the Docker image, running the application in different modes (standalone vs clustered), and running tests and benchmarks. Also include a `make release` target that builds the binaries with the appropriate ldflags and creates a GitHub release using goreleaser. -- [ ] Update CI pipeline to run tests, build binaries, and create releases on GitHub when new tags are pushed. Also consider adding a step to build and push Docker images to a registry like Docker Hub or GitHub Container Registry. -- [ ] Update README and dev docs with instructions for running the application, using the new features, and contributing to the project. Include examples of how to run in standalone vs clustered mode, how to configure dependencies, and how to use the new observability features. -- [ ] Add a roadmap or project board to track these tasks and future features, and to provide visibility into the project's direction and priorities for contributors and users. -- [ ] Consider adding support for graceful shutdown and cleanup of resources (e.g. closing database connections, flushing metrics, etc) when the application receives termination signals, to ensure a clean shutdown and avoid potential data loss or corruption. -- [ ] Add support for configuration via environment variables in addition to config files, for easier deployment in containerized environments. Potentially use a library like `envconfig` or `viper` to handle this in a clean way. -- [ ] Add support for hot reloading of configuration without needing to restart the application, especially for things like log levels and metrics settings, to allow for easier tuning and debugging in production without downtime. This could potentially be done via SIGHUP signal handling or by watching config files for changes. -- [ ] Add support for graceful handling of NATS server restarts or connectivity issues, including automatic reconnection and backoff strategies, to improve the robustness of the system in production environments where network issues can occur. -- [ ] Add support for batching and retrying failed events in the worker, with exponential backoff and a dead letter queue for events that fail repeatedly, to improve reliability and ensure that transient issues do not result in data loss. -- [ ] Fuzzing and security testing, especially around the API endpoints, to identify and fix potential vulnerabilities before they can be exploited in production. This could include testing for SQL injection, authentication bypass, and other common web vulnerabilities. -- [ ] From 3156c889717c66b0006617b6e43be505d9d4a091 Mon Sep 17 00:00:00 2001 From: Eric Andrechek Date: Fri, 15 May 2026 14:24:07 -0400 Subject: [PATCH 02/18] docs(contributing): drop vestigial "tenant isolation" wording MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit WaveHouse has no tenant model — auth is JWT-based with role-based access control (Hasura-style per-table/per-column/row-level policies). Matches SECURITY.md and the parallel pr-review.md edit in the previous commit on this branch. Flagged by Gemini Code Assist on #147. Co-Authored-By: Claude Opus 4.7 (1M context) --- CONTRIBUTING.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index c0abfd98..9995e096 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -100,7 +100,7 @@ All submissions require review before merging. Reviewers will look for: - Correctness and test coverage - Adherence to existing architecture patterns - Documentation updates where applicable -- No security regressions (tenant isolation, input validation) +- No security regressions (role-based access control, input validation) ## License From b8a7a255017db83f90b2d7bc188a287c55de5652 Mon Sep 17 00:00:00 2001 From: Eric Andrechek Date: Tue, 19 May 2026 09:34:15 -0400 Subject: [PATCH 03/18] feat(claude): add agent PR discipline hooks + review gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Layers Claude Code-specific guardrails on top of the existing universal git hooks. Pre-existing git hooks already enforce `make verify` on commit and `make ci` passing before push for everyone uniformly. These additions are agent-only and live in `.claude/hooks/`. ## Hooks - `.claude/hooks/agent-bash-gate.sh` (PreToolUse Bash): - Agent-created PRs must use the draft flag (`gh pr create` without `--draft` is blocked) - `gh pr ready` / `--approve` / `--request-changes` blocked (humans only) - `gh pr edit --add-reviewer` / `--add-assignee` blocked (humans only — bot re-triggers via `gh pr comment` mention are fine; `@coderabbitai review`, `@gemini-code-assist`, `@claude` / `/review` all work since they don't touch the reviewer API) - `gh api requested_reviewers` POST blocked (API form of reviewer-add) - Bypass flags on `git push` / `git commit` blocked for agents - Direct marker forgery blocked (`touch`, redirect, `sh -c` wrappers, `cp`/`mv` targeting `tmp/(ci|review)-passed-*`) - `git push` to a PR branch blocked without BOTH `ci-passed` and `review-passed` markers for the current HEAD - `.claude/hooks/review-marker.sh` (PostToolUse Agent) writes `tmp/review-passed-` when the `pre-push-reviewer` subagent returns `VERDICT: ship_it`. Hook runs at Claude Code privilege so it bypasses the deny list — the only path to the review marker is via the subagent's verdict. The orchestrator agent can't fake it because the subagent runs in fresh context with the canonical system prompt that can't be overridden. ## Agent + skills - `.claude/agents/pre-push-reviewer.md`: strengthened to fetch PR context (top-level + inline comments, reviews, CI status, linked issue acceptance criteria) when on a PR branch, and emit a parseable `VERDICT: ship_it|iterate|block` line consumed by the marker hook. - `.claude/skills/pr-review-locally/SKILL.md`: documents "review someone else's PR locally" workflow — `wt switch pr:N` (gh-CLI-backed) + `pre-push-reviewer` subagent, no PR comments posted. CI claude-review via `gh workflow run` is the remote-comment path. ## Permissions Force-prefix denies covering obvious bypass attempts. The agent-bash-gate hook handles creative attempts that prefix patterns can't match. ## Docs - `AGENTS.md`: new "## Agent PR Discipline" section with the rules + bot-mention table. - `docs/src/content/docs/claude-code.md`: updated to reflect new gates, plural agent/skill tables, the bot-vs-human reviewer- request distinction, the Daily Workflow showing the full pre-push-reviewer loop on PR branches. ## Tested locally End-to-end gating flow verified: 1. No markers → push blocked (no ci-passed) 2. ci-passed only on PR branch → push blocked (no review-passed) 3. Marker hook fires with ship_it → review-passed written → push ALLOWED agent-bash-gate.sh: 41 test cases covering bypass-flag attempts, gh pr discipline, marker forgery, innocuous commands — all pass. review-marker.sh: 7 test cases covering ship_it / iterate / block / wrong agent type / missing verdict / case variations / last-verdict-wins — all pass. ## Known limitation The agent-bash-gate regex matches on whole-command text. If a commit message body contains the literal flag substring, the hook can false-positive. Workaround: pass commit messages via `git commit -F ` (this commit uses that pattern). Honest- agent defense, not adversarial; AGENTS.md makes the rule explicit. Co-Authored-By: Claude Opus 4.7 (1M context) --- .claude/agents/pre-push-reviewer.md | 100 +++++++++ .claude/commands/cover.md | 24 +++ .claude/hooks/agent-bash-gate.sh | 172 ++++++++++++++++ .claude/hooks/gofumpt-on-save.sh | 25 +++ .claude/hooks/review-marker.sh | 47 +++++ .claude/settings.json | 96 +++++++++ .claude/skills/pr-review-locally/SKILL.md | 98 +++++++++ .claude/skills/pr-sync-with-main/SKILL.md | 60 ++++++ .config/wt.toml | 21 ++ .githooks/pre-commit | 73 +++++++ .githooks/pre-push | 40 ++++ .gitignore | 4 + AGENTS.md | 111 +++++++++- Makefile | 13 +- README.md | 14 +- docs/src/config/sidebar.ts | 1 + docs/src/content/docs/claude-code.md | 237 ++++++++++++++++++++++ docs/src/content/docs/development.md | 7 + 18 files changed, 1136 insertions(+), 7 deletions(-) create mode 100644 .claude/agents/pre-push-reviewer.md create mode 100644 .claude/commands/cover.md create mode 100755 .claude/hooks/agent-bash-gate.sh create mode 100755 .claude/hooks/gofumpt-on-save.sh create mode 100755 .claude/hooks/review-marker.sh create mode 100644 .claude/settings.json create mode 100644 .claude/skills/pr-review-locally/SKILL.md create mode 100644 .claude/skills/pr-sync-with-main/SKILL.md create mode 100644 .config/wt.toml create mode 100755 .githooks/pre-commit create mode 100755 .githooks/pre-push create mode 100644 docs/src/content/docs/claude-code.md diff --git a/.claude/agents/pre-push-reviewer.md b/.claude/agents/pre-push-reviewer.md new file mode 100644 index 00000000..24b145dd --- /dev/null +++ b/.claude/agents/pre-push-reviewer.md @@ -0,0 +1,100 @@ +--- +description: Reviews the current branch's full delta against main using the canonical WaveHouse review prompt (.github/prompts/pr-review.md). Use before pushing to any PR branch (mandatory per AGENTS.md §Agent PR Discipline) or to audit someone else's PR after `wt switch pr:N` / `gh pr checkout N`. Considers full PR diff, latest commit, all open PR comments + reviews, and CI / failing-test status. Runs in fresh context for objectivity. Returns [MUST]/[SHOULD]/[MAY] findings plus a parseable verdict line that drives the pre-push marker. +tools: Bash, Read, Glob, Grep +model: opus +--- + +You are reviewing the current branch's delta against main, using the same prompt as the CI Claude review action — but locally, on the working state, before push (or on someone else's PR after checking it out locally). + +## Source of truth + +Read `.github/prompts/pr-review.md` first. That file is the canonical WaveHouse review prompt and applies here verbatim — including the focus areas (correctness → security → performance → testing → docs/sdk-sync), the severity tags `[MUST]`/`[SHOULD]`/`[MAY]`, the noise filter, and the verdict rules. + +The diff source here is the local working state, computed as `git diff $(git merge-base HEAD main)` (or equivalently `git diff main...HEAD`). + +## Process + +1. Read `.github/prompts/pr-review.md` and `AGENTS.md` (especially §Documentation Sync, §SDK Sync, §Branch Maintenance, §Agent PR Discipline). + +2. Compute the branch diff: + + ```bash + git diff $(git merge-base HEAD main) + ``` + +3. For each changed file, read its current state. Don't just look at the diff — context matters. + +4. **If this branch has an open PR, fetch PR context.** Get the PR number from the branch name (`gh pr view --json number,state,comments,reviews,statusCheckRollup`). When there's a PR, the review must consider: + + - **All open PR comments and reviews** — top-level comments (`gh pr view --json comments,reviews`) AND inline review comments (`gh api repos//pulls//comments`). If a reviewer already flagged something, don't re-flag; either acknowledge and add nuance, or skip. If the author replied to a concern, factor in the reply. + - **Failing CI checks** — `gh pr checks ` and `gh pr view --json statusCheckRollup`. Surface failures that look like real bugs (not env flakes). + - **Linked issues** — `Closes #N` / `Fixes #N` in PR body. Acceptance criteria live in the issue. + - **Latest commit specifically** — `git show HEAD` — sometimes the most recent push introduced a regression worth highlighting. + + If there's no open PR for this branch (e.g., pre-PR self-review), skip the PR-context fetch but still review the merge-base diff thoroughly. + +5. Apply the focus areas from `pr-review.md` in order: + + - **Correctness** — Go concurrency (goroutine leaks, data races, missing context propagation, channel leaks, `sync.Once` / `sync.Map` misuse, handlers ignoring `r.Context()`), error wrapping with `%w`, resource cleanup on every error path, broken invariants per AGENTS.md §Key Design Decisions. + - **Security** — OWASP Top 10 walked against the diff (SQL injection in CH paths, JWT/role handling, sensitive data exposure, CORS, hardcoded secrets, TOCTOU). Severity-tag CRITICAL / HIGH / MEDIUM / LOW. + - **Performance** — hot-path allocations, unbounded goroutines, unbatched DB work, locks across I/O, N+1, singleflight misuse. + - **Testing** — new code on critical paths without tests, missing edge cases, mocks where integration would catch more. + - **Documentation sync** — per AGENTS.md §Documentation Sync table. + - **SDK sync** — per AGENTS.md §SDK Sync table. Did `internal/api/` change without `clients/ts/src/` consideration? + +6. Apply the noise filter from `pr-review.md` before finalizing: drop findings you wouldn't personally ask the author to change in-person. + +7. Tag each finding `[MUST]` / `[SHOULD]` / `[MAY]` per the styleguide. + +8. End with a verdict per the styleguide (`Ship it` / `Iterate` / `Block`), **followed immediately by the parseable verdict line** on its own line: + + ``` + VERDICT: ship_it + ``` + + or `VERDICT: iterate` or `VERDICT: block`. The line is consumed by `.claude/hooks/review-marker.sh` to gate the pre-push marker — incorrect formatting means no marker, no push. + +## Output format + +``` +## Pre-push review — vs main + +(Optional: brief paragraph on PR scope + linked issues if applicable.) + +### [MUST] Findings + +- `internal/api/handler.go:42` — + Severity: CRITICAL/HIGH/MEDIUM/LOW (security findings only) + +### [SHOULD] Findings + +- ... + +### [MAY] Findings + +- ... + +## Verdict + +**Ship it** / **Iterate** / **Block** — + +VERDICT: ship_it +``` + +(or `VERDICT: iterate` / `VERDICT: block`) + +## Verdict mapping (matches `.gemini/styleguide.md` + `.github/prompts/pr-review.md`) + +- **`Ship it`** + `VERDICT: ship_it` — no `[MUST]`s; few or no `[SHOULD]`s. Pre-push marker auto-writes, push proceeds. +- **`Iterate`** + `VERDICT: iterate` — one or more `[MUST]`s that aren't `Block`-level, or multiple `[SHOULD]`s. Fix and re-invoke (always in fresh context). +- **`Block`** + `VERDICT: block` — a `[MUST]` that's CRITICAL/HIGH security, data-loss risk, or broken core invariant. Cannot proceed without addressing. + +`[MAY]` findings alone don't preclude `Ship it`. + +## Framing + +This is a SELF-review or PR-audit run by an agent. Frame findings as "things to consider fixing before pushing / before this PR merges" — direct and skeptical, but constructive. The user reads these and decides what to act on. + +**Do not make code changes.** Review only. The orchestrator agent (or a human) decides what to fix; you just surface the findings. + +**Do not post comments on the PR.** This is a local review. To make a bot comment on a PR remotely, the workflow is `gh workflow run "Claude PR review" -f pr_number=` (which fires the CI claude-review action — that's the bot that comments). diff --git a/.claude/commands/cover.md b/.claude/commands/cover.md new file mode 100644 index 00000000..5ec63b2a --- /dev/null +++ b/.claude/commands/cover.md @@ -0,0 +1,24 @@ +--- +description: Render coverage HTML for a suite and surface drops below threshold +argument-hint: [unit|integration|e2e|sdk|all] (default: merge whatever exists) +--- + +Generate the coverage report and surface anything below threshold from `.testcoverage.yml`. + +Suite to run: $ARGUMENTS + +Behavior: +- **no argument or "merge"**: just `make cov` (merges whatever covdata exists in `tmp/coverage/*/data/` and gates against `threshold.total`) +- **unit**: `make test-unit` (gates per-suite + writes `tmp/coverage/unit/`) +- **integration**: `make test-integration` (requires Docker) +- **e2e**: `make test-e2e` (requires Docker; orchestrator + cover binary) +- **sdk**: `make test-sdk` (vitest) +- **all**: `make test-all` (all four suites sequentially + `make cov`) + +After the run completes: +1. Parse `tmp/coverage//coverage.txt` (Go) or `tmp/coverage/sdk/index.html` (SDK) for per-package coverage. +2. Identify packages below the suite's threshold from `.testcoverage.yml`. Report as a sorted list. +3. Surface the suite total + delta vs. the threshold. +4. Print the path to the HTML report — don't auto-open (lets the user open it themselves if they want). + +If the suite errors before producing covdata, report what failed without trying to render coverage. diff --git a/.claude/hooks/agent-bash-gate.sh b/.claude/hooks/agent-bash-gate.sh new file mode 100755 index 00000000..da99f455 --- /dev/null +++ b/.claude/hooks/agent-bash-gate.sh @@ -0,0 +1,172 @@ +#!/usr/bin/env bash +# PreToolUse Bash gate — enforces AGENTS.md §"Agent PR Discipline". +# +# This hook is what the deny-list can't be (deny patterns are prefix-glob only). +# Here we regex over the whole command to catch: +# +# 1. --no-verify on git push/commit (no agent bypass; humans can use it intentionally) +# 2. gh pr create without --draft (only humans publish ready-for-review PRs) +# 3. gh pr ready (humans only — draft→ready is a deliberate human signal) +# 4. gh pr edit --add-reviewer / --add-assignee (human reviewer assignment is humans-only; +# bot reviewers are re-triggered via PR comments, not the reviewer API) +# 5. gh api .../requested_reviewers POST (the API form of --add-reviewer) +# 6. gh pr review --approve / --request-changes (only humans take formal review actions; +# bot reviewers use inline review comments + sticky summaries instead) +# 7. Direct marker forgery (touch / redirect / sh -c wrappers writing to +# tmp/(ci|review)-passed-*) +# 8. git push without required markers (ci-passed always; review-passed on PR branches) +# +# Anything blocked here can typically be re-run by a human from terminal — these +# rules are for the agent path, not the underlying git/gh capabilities. + +set -uo pipefail + +input=$(cat) +cmd=$(printf '%s' "$input" | jq -r '.tool_input.command // empty' 2>/dev/null) +[ -z "$cmd" ] && exit 0 + +cd "${CLAUDE_PROJECT_DIR:-.}" 2>/dev/null || exit 0 + +# --- Helper: block with a structured reason --------------------------------- +block() { + local reason="$1" + cat >&2 <` invocation anywhere in the +# command (including after && / ; / | / cd ... &&). Used by multiple checks. +git_subcmd() { + printf '%s\n' "$cmd" | grep -qE "(^|[[:space:];|&]+)git[[:space:]]+$1\b" +} + +# --- 1. --no-verify on git push/commit -------------------------------------- +if git_subcmd 'push' || git_subcmd 'commit'; then + if printf '%s\n' "$cmd" | grep -qE '(^|[[:space:]])--no-verify\b'; then + block "git push/commit with --no-verify is not permitted for agents. Run the gates." + fi +fi + +# --- 2. gh pr create without --draft ---------------------------------------- +if printf '%s\n' "$cmd" | grep -qE '(^|[[:space:];|&]+)gh[[:space:]]+pr[[:space:]]+create\b'; then + if ! printf '%s\n' "$cmd" | grep -qE '(^|[[:space:]])(--draft|-d)\b'; then + block "Agent-opened PRs must be created with --draft. Only humans publish ready-for-review PRs." + fi +fi + +# --- 3. gh pr ready --------------------------------------------------------- +if printf '%s\n' "$cmd" | grep -qE '(^|[[:space:];|&]+)gh[[:space:]]+pr[[:space:]]+ready\b'; then + block "Only humans transition PRs from draft to ready-for-review. Ask the user to do this manually when the PR is ready." +fi + +# --- 4. gh pr edit with reviewer/assignee changes --------------------------- +if printf '%s\n' "$cmd" | grep -qE '(^|[[:space:];|&]+)gh[[:space:]]+pr[[:space:]]+edit\b'; then + if printf '%s\n' "$cmd" | grep -qE '(^|[[:space:]])--(add|remove)-(reviewer|assignee)\b'; then + block "Adding/removing reviewers or assignees is humans-only. To re-trigger bot reviewers, post a PR comment mentioning them (@coderabbitai review, @gemini-code-assist /gemini review, @claude / /review)." + fi +fi + +# --- 5. gh api .../requested_reviewers POST (humans-only reviewer-add API) -- +if printf '%s\n' "$cmd" | grep -qE '(^|[[:space:];|&]+)gh[[:space:]]+api\b' && \ + printf '%s\n' "$cmd" | grep -qE 'requested_reviewers'; then + if printf '%s\n' "$cmd" | grep -qE '(-X[[:space:]]*POST|--method[[:space:]]*POST|[[:space:]]-f[[:space:]]+reviewers=|[[:space:]]-F[[:space:]]+reviewers=)'; then + block "POST to /requested_reviewers is the API form of --add-reviewer; humans-only. For bot reviewers, post a PR comment mentioning them." + fi +fi + +# --- 6. gh pr review --approve / --request-changes -------------------------- +if printf '%s\n' "$cmd" | grep -qE '(^|[[:space:];|&]+)gh[[:space:]]+pr[[:space:]]+review\b'; then + if printf '%s\n' "$cmd" | grep -qE '(^|[[:space:]])(--approve|-a)\b'; then + block "Only humans approve PRs (--approve)." + fi + if printf '%s\n' "$cmd" | grep -qE '(^|[[:space:]])(--request-changes|-r)\b'; then + block "Agents don't use --request-changes. Post inline review comments via the GitHub inline-comment MCP tool, or use gh pr comment for top-level comments." + fi +fi + +# --- 7. Marker forgery ------------------------------------------------------ +# Block any *write-like* operation targeting tmp/(ci|review)-passed-*. +# Read operations (ls/cat/find) on the markers are fine. +if printf '%s\n' "$cmd" | grep -qE 'tmp/(ci|review)-passed-'; then + if printf '%s\n' "$cmd" | grep -qE '(touch|install)[[:space:]]+[^&|;]*tmp/(ci|review)-passed-'; then + block "Direct marker creation is forbidden. Markers are written by 'make ci' (ci-passed) and the pre-push-reviewer agent's PostToolUse hook (review-passed)." + fi + if printf '%s\n' "$cmd" | grep -qE '(cp|mv)[[:space:]]+[^&|;]+[[:space:]]+[^&|;]*tmp/(ci|review)-passed-'; then + block "Direct marker creation via cp/mv is forbidden." + fi + if printf '%s\n' "$cmd" | grep -qE '>[[:space:]]*tmp/(ci|review)-passed-'; then + block "Direct marker creation via output redirect is forbidden." + fi + if printf '%s\n' "$cmd" | grep -qE '\b(sh|bash|zsh|env)\b[[:space:]]+-c[[:space:]]+.*tmp/(ci|review)-passed-'; then + block "Wrapping marker creation in a shell -c to bypass the gate is forbidden." + fi +fi + +# --- 8. git push: check markers --------------------------------------------- +# Only on actual `git push` invocations (not `git push --help`, not `gh pr push`). +if git_subcmd 'push'; then + head_sha=$(git rev-parse HEAD 2>/dev/null || echo "") + if [ -n "$head_sha" ]; then + short_sha="${head_sha:0:8}" + ci_marker="tmp/ci-passed-${head_sha}" + review_marker="tmp/review-passed-${head_sha}" + + # 8a. ci-passed required for every push (mirrors the universal git pre-push hook; + # firing here too gives Claude a more actionable error inside its session). + if [ ! -f "$ci_marker" ]; then + cat >&2 </dev/null || echo "") + if [ -n "$branch" ] && [ "$branch" != "main" ]; then + pr_state=$(gh pr view "$branch" --json state --jq .state 2>/dev/null || echo "") + if [ "$pr_state" = "OPEN" ]; then + if [ ! -f "$review_marker" ]; then + cat >&2 </dev/null) + +# Only Go files +[[ "$file_path" == *.go ]] || exit 0 +[ -f "$file_path" ] || exit 0 + +cd "${CLAUDE_PROJECT_DIR:-.}" 2>/dev/null || exit 0 + +go tool gofumpt -w "$file_path" 2>/dev/null || true +go tool goimports -w "$file_path" 2>/dev/null || true + +exit 0 diff --git a/.claude/hooks/review-marker.sh b/.claude/hooks/review-marker.sh new file mode 100755 index 00000000..1dd4777a --- /dev/null +++ b/.claude/hooks/review-marker.sh @@ -0,0 +1,47 @@ +#!/usr/bin/env bash +# PostToolUse Agent hook — writes the pre-push review marker. +# +# When the pre-push-reviewer subagent finishes its review and ends its response +# with `VERDICT: ship_it`, this hook writes tmp/review-passed-. The +# pre-push gate (in .claude/hooks/agent-bash-gate.sh) reads that marker to allow +# the subsequent `git push`. +# +# Why this hook exists: the orchestrator agent is denied direct writes to +# tmp/(ci|review)-passed-* (permission deny list + agent-bash-gate). Hooks run at +# Claude Code privilege level, NOT subject to the permission system, so this +# is the only path to creating the marker. The subagent's verdict is the gate; +# the orchestrator can't fake it because the subagent runs in fresh context +# with the canonical system prompt from .claude/agents/pre-push-reviewer.md. + +set -uo pipefail + +input=$(cat) +agent_type=$(printf '%s' "$input" | jq -r '.tool_input.subagent_type // empty' 2>/dev/null) + +# Only act on pre-push-reviewer completions +[ "$agent_type" = "pre-push-reviewer" ] || exit 0 + +response=$(printf '%s' "$input" | jq -r '.tool_response // empty' 2>/dev/null) +[ -z "$response" ] && exit 0 + +# Parse the parseable verdict line. Format (per .claude/agents/pre-push-reviewer.md): +# VERDICT: ship_it | VERDICT: iterate | VERDICT: block +# We take the LAST such line in the response (in case the agent quoted the +# enum earlier in its body). Case-insensitive on the value. +verdict=$(printf '%s\n' "$response" \ + | grep -oiE 'VERDICT:[[:space:]]*(ship_it|iterate|block)\b' \ + | tail -1 \ + | sed -E 's/.*:[[:space:]]*([a-zA-Z_]+).*/\1/' \ + | tr '[:upper:]' '[:lower:]') + +[ "$verdict" = "ship_it" ] || exit 0 + +cd "${CLAUDE_PROJECT_DIR:-.}" 2>/dev/null || exit 0 +head_sha=$(git rev-parse HEAD 2>/dev/null) +[ -z "$head_sha" ] && exit 0 + +mkdir -p tmp +touch "tmp/review-passed-${head_sha}" +echo "📝 Pre-push review marker written: tmp/review-passed-${head_sha:0:8}" >&2 + +exit 0 diff --git a/.claude/settings.json b/.claude/settings.json new file mode 100644 index 00000000..9004dc6d --- /dev/null +++ b/.claude/settings.json @@ -0,0 +1,96 @@ +{ + "$schema": "https://json.schemastore.org/claude-code-settings.json", + + "permissions": { + "deny": [ + "Bash(rm -rf /:*)", + "Bash(rm -rf $HOME:*)", + "Bash(rm -rf ~:*)", + "Bash(sudo rm:*)", + "Bash(git push --force:*)", + "Bash(git push -f:*)", + "Bash(git push --force-with-lease:*)", + "Bash(git reset --hard origin:*)", + "Bash(git filter-branch:*)", + "Bash(git update-ref -d:*)", + "Bash(gh pr merge:*)", + "Bash(gh repo delete:*)", + "Bash(gh release delete:*)", + "Bash(gh workflow disable:*)", + "Bash(gh secret delete:*)", + "Bash(make deps-wipe:*)", + "Bash(make clean-all:*)", + + "Bash(gh pr ready:*)", + "Bash(gh pr review --approve:*)", + "Bash(gh pr review -a:*)", + "Bash(gh pr review --request-changes:*)", + "Bash(gh pr review -r:*)", + + "Bash(git push --no-verify:*)", + "Bash(git commit --no-verify:*)", + + "Bash(touch tmp/ci-passed:*)", + "Bash(touch tmp/review-passed:*)", + "Write(./tmp/ci-passed-*)", + "Write(./tmp/review-passed-*)", + "Edit(./tmp/ci-passed-*)", + "Edit(./tmp/review-passed-*)", + "Write(tmp/ci-passed-*)", + "Write(tmp/review-passed-*)", + "Edit(tmp/ci-passed-*)", + "Edit(tmp/review-passed-*)", + + "Read(./.env)", + "Read(./.env.*)", + "Read(./secrets/**)" + ] + }, + + "worktree": { + "baseRef": "fresh", + "symlinkDirectories": [ + "node_modules", + ".bin", + "tmp", + "data" + ] + }, + + "enableAllProjectMcpServers": false, + + "hooks": { + "PreToolUse": [ + { + "matcher": "Bash", + "hooks": [ + { + "type": "command", + "command": "$CLAUDE_PROJECT_DIR/.claude/hooks/agent-bash-gate.sh", + "timeout": 30 + } + ] + } + ], + "PostToolUse": [ + { + "matcher": "Edit|Write|MultiEdit", + "hooks": [ + { + "type": "command", + "command": "$CLAUDE_PROJECT_DIR/.claude/hooks/gofumpt-on-save.sh" + } + ] + }, + { + "matcher": "Agent", + "hooks": [ + { + "type": "command", + "command": "$CLAUDE_PROJECT_DIR/.claude/hooks/review-marker.sh" + } + ] + } + ] + } +} diff --git a/.claude/skills/pr-review-locally/SKILL.md b/.claude/skills/pr-review-locally/SKILL.md new file mode 100644 index 00000000..ecd30213 --- /dev/null +++ b/.claude/skills/pr-review-locally/SKILL.md @@ -0,0 +1,98 @@ +--- +name: pr-review-locally +description: Use when a user wants to review someone else's open PR locally without commenting on the PR. Triggers on phrases like "review PR ", "look at PR ", "audit PR ", "pull down PR and review", "check out PR for me". Covers worktrunk's `wt switch pr:` syntax (gh-CLI-backed), the `gh pr checkout` fallback, and how to optionally fire the CI claude-review for the bot to comment on the PR remotely. Pairs with the pre-push-reviewer subagent. +--- + +# Reviewing someone else's PR locally + +For "review PR 120 locally" / "audit this PR" / "pull down PR and tell me what you think" — pull the PR's content into a worktree, run the `pre-push-reviewer` subagent against it in fresh context, surface findings to the user. Don't comment on the PR. + +## Procedure + +### 1. Get the PR into a local worktree + +Worktrunk supports `pr:N` syntax when `gh` CLI is installed: + +```bash +wt switch pr:120 # creates a worktree on PR 120's branch + switches into it +``` + +Fallback if worktrunk isn't installed or `pr:` syntax fails: + +```bash +gh pr checkout 120 # in-place checkout (no worktree) +``` + +Verify after switching: + +```bash +git rev-parse HEAD # should match the PR's head SHA +gh pr view --json number,state,headRefName --jq . # confirm we're on PR 120's branch +``` + +### 2. Invoke the `pre-push-reviewer` subagent + +Use the `Agent` tool: + +``` +Agent({ + subagent_type: "pre-push-reviewer", + description: "Review PR locally", + prompt: "Review the current branch (PR ) against main. Use the canonical + .github/prompts/pr-review.md workflow — full PR diff vs merge-base, + latest commit, all open PR comments and reviews, CI status. Return + [MUST]/[SHOULD]/[MAY] findings and a parseable verdict line." +}) +``` + +The subagent runs in **fresh context** — no contamination from your current session's assumptions. That's the whole point: the review should be cold-eyed. + +### 3. Surface findings to the user + +Present the subagent's output. Don't auto-fix anything — the PR belongs to someone else. The user decides what to do with the findings. + +If the user asks "should I approve?", that's their call — you can summarize the verdict (`Ship it` / `Iterate` / `Block`) and highlight the highest-severity findings, but the actual approval decision is theirs. + +## Two modes + +### Local-only (default) + +The above procedure. Findings stay in your local session. Nothing is posted to the PR. Use for "I want a thorough read of this PR before I review it manually," "what would Claude flag here?", or "audit this PR for me." + +### Make the bot comment on the PR remotely + +If the user explicitly asks for the bot to post on the PR (not just local feedback), the mechanism is the **CI claude-review workflow** — NOT manual comments from the local session. Trigger it: + +```bash +gh workflow run "Claude PR review" -R Wave-RF/WaveHouse -f pr_number=120 +``` + +That fires `.github/workflows/claude-review.yml` against PR 120. The workflow: + +- Runs the same `.github/prompts/pr-review.md` prompt +- Posts inline review comments at line-level +- Edits a sticky verdict summary comment (one per PR) +- Gates on trust (HEAD's author / committer must have ≥read on the repo) + +This is the same path as posting `@claude` or `/review` as a PR comment — both end up firing the workflow. + +## What NOT to do + +- ❌ **Don't run the reviewer locally and then post comments on the PR manually.** The CI workflow exists for that and handles trust gating + sticky-comment editing properly. Manual comments fragment the dialog and don't match the bot's conventions. +- ❌ **Don't `--add-reviewer` yourself or others to the PR.** Blocked by `.claude/hooks/agent-bash-gate.sh` anyway. Bot reviewers re-trigger via comment mentions; humans add themselves. +- ❌ **Don't switch back to your own branch before presenting findings.** Stay in the PR's worktree until you've reported to the user. +- ❌ **Don't approve, request-changes, or merge the PR.** Approval is humans-only (`gh pr review --approve` is denied). Merge is denied. Request-changes is denied. Surface findings, let the human reviewer act. + +## Cleaning up + +After the review, the user might want to switch back. If they used `wt switch`, the worktree persists — they can `wt switch ` to return, or `wt remove ` to tear down. If they used `gh pr checkout`, their previous branch state was modified; `git switch -` returns to it. Either way, don't tear down the worktree without asking — the user might want to keep poking at the PR. + +## If the PR can't be checked out + +Common failures: + +- **Auth**: `gh auth status` to confirm you're authenticated; `gh auth login` if not. +- **Closed/merged PR**: `gh pr view 120 --json state` — if not `OPEN`, you're reviewing historical state, which is fine but worth telling the user. +- **Permission denied**: PR may be from a fork you can't push to. Reviewing the PR head SHA still works (`git fetch origin pull/120/head:pr-120` then `git checkout pr-120`). + +Surface failures to the user with a clear next-step rather than silently bailing. diff --git a/.claude/skills/pr-sync-with-main/SKILL.md b/.claude/skills/pr-sync-with-main/SKILL.md new file mode 100644 index 00000000..62e0a465 --- /dev/null +++ b/.claude/skills/pr-sync-with-main/SKILL.md @@ -0,0 +1,60 @@ +--- +name: pr-sync-with-main +description: Use when a PR shows "This branch is out-of-date with the base branch" on GitHub, when a user asks to "fix the PR", "sync with main", "merge main into this branch", or whenever a feature branch needs to absorb upstream main commits. Covers the exact procedure (merge, never rebase, never force-push) plus the WaveHouse-specific reason this matters. +--- + +# Syncing a PR branch with main + +When the GitHub UI says "This branch is out-of-date with the base branch" — or a teammate asks to "fix the PR" / "sync with main" — follow this workflow exactly. + +## Procedure + +```bash +# Working tree must be clean before merge +git status + +# Fetch latest main without touching the working tree +git fetch origin main + +# Merge into the current branch (the default merge commit message is fine) +git merge origin/main --no-edit + +# If conflicts: STOP, surface to the user. Don't auto-resolve. + +# Push the merge commit normally — no force, no force-with-lease +git push +``` + +After the merge: + +- `make ci` for the merge commit is invalidated (HEAD SHA changed). Re-run `make ci` before pushing — the pre-push hook will block you otherwise. +- CI will fire on the merge commit. Verify it passes. +- The "out-of-date" banner on the PR clears once GitHub sees the merged base. + +## Why merge, not rebase + +WaveHouse has historically lost `pull_request` event firing on long-lived branches (symptom: only `pull_request_target` checks appear, regular CI doesn't trigger). The recovery is **merging origin/main into the branch**, not: + +- ❌ `git rebase origin/main` — changes commit SHAs, requires force-push +- ❌ `git push --force` / `--force-with-lease` — blocked by `.claude/settings.json` deny rules anyway; also loses inline review-comment anchors +- ❌ Closing + reopening the PR — doesn't fix the underlying event-firing issue +- ❌ Pushing an empty commit (`git commit --allow-empty`) — superstitious +- ❌ Toggling draft ↔ ready — same; doesn't help + +The merge approach preserves everything: inline review-thread anchors, the PR's review history, and re-triggers the standard `pull_request` events. + +## If `git merge origin/main` fails with conflicts + +**Stop and ask the user.** Don't make autonomous conflict-resolution decisions — the conflict might involve subtle semantic differences (two parallel changes to `internal/api/router.go`, for example, that look mechanically resolvable but break runtime behavior). + +Surface the conflicting files and the conflict markers. Let the user decide how to resolve, or escalate to a teammate. + +## If the user reports the merge "didn't fix the PR" + +The merge is the correct action; if GitHub still shows out-of-date, possible causes: + +- The push didn't actually land (check `git log origin/` vs local HEAD) +- GitHub's cache is stale (typically clears within a minute) +- A protected ref or rule is blocking the push + +Don't reach for force-push as a fix. Verify the push succeeded and wait briefly. diff --git a/.config/wt.toml b/.config/wt.toml new file mode 100644 index 00000000..33e4a3cc --- /dev/null +++ b/.config/wt.toml @@ -0,0 +1,21 @@ +# Worktrunk project config — committed, applies to all team members. +# Personal overrides go in ~/.config/worktrunk/config.toml (gitignored upstream). +# See https://worktrunk.dev/config/ for full schema. + +[post-start] +# Bootstrap a fresh worktree: install golangci-lint, air, Go modules, and the +# three pnpm projects (SDK, E2E harness, docs). Matches `make tools` — the +# same target a new clone runs once. +tools = "make tools" + +[pre-merge] +# Fast pre-merge gate: tidy + fmt + vulncheck + lint. Mirrors CI's `make ci` +# Phase 1 (parallel checks). Full `make ci` (integration + e2e, ~5-10min) is +# left for explicit runs before risky merges — too slow as a default gate. +verify = "make verify" + +[pre-remove] +# Surface uncommitted work before tearing down a worktree. Informational — +# users can still force-remove. If status output is non-empty, worktrunk +# prompts (or blocks, depending on its policy). +status = "git status --short" diff --git a/.githooks/pre-commit b/.githooks/pre-commit new file mode 100755 index 00000000..ab3782b9 --- /dev/null +++ b/.githooks/pre-commit @@ -0,0 +1,73 @@ +#!/usr/bin/env bash +# pre-commit: runs `make verify` (blocks on failure), then surfaces doc-sync +# and sdk-sync nudges (informational, doesn't block). +# +# Bypass: `git commit --no-verify` (per AGENTS.md, only when explicitly intentional). +# Installed by `make tools` via `git config core.hooksPath .githooks`. + +set -uo pipefail + +cd "$(git rev-parse --show-toplevel)" || exit 1 + +# --- Block: make verify ------------------------------------------------------ +# tidy + fmt + vulncheck + lint. Fast enough (~30s) for every commit. +if ! make verify; then + echo "" >&2 + echo "🛑 pre-commit: 'make verify' failed." >&2 + echo " Fix the reported issues and re-stage, or bypass intentionally with" >&2 + echo " 'git commit --no-verify' (only for WIP / draft commits)." >&2 + exit 1 +fi + +# --- Nudge: doc-sync + sdk-sync --------------------------------------------- +# Informational stderr only. Commit proceeds. AGENTS.md §Documentation Sync +# and §SDK Sync are the source-of-truth tables; this hook applies the most +# common rules deterministically against staged files. +staged=$(git diff --cached --name-only 2>/dev/null) +[ -z "$staged" ] && exit 0 + +has() { echo "$staged" | grep -qE "$1"; } +nudges=() + +# Non-test files in internal/api/ → docs/api.md + SDK +if echo "$staged" | grep -E '^internal/api/.*\.go$' | grep -qvE '_test\.go$'; then + has '^docs/src/content/docs/api\.md$' \ + || nudges+=("API handler/router staged but docs/src/content/docs/api.md is not (AGENTS.md §Documentation Sync)") + has '^clients/ts/src/' \ + || nudges+=("API handler/router staged but clients/ts/src/ is not — SDK may need a corresponding method (AGENTS.md §SDK Sync)") +fi + +# Config struct → configuration.md + config.yaml + compose env blocks +if has '^internal/config/config\.go$'; then + has '^docs/src/content/docs/configuration\.md$' \ + || nudges+=("internal/config/config.go staged but docs/src/content/docs/configuration.md is not") + has '^config\.yaml$' \ + || nudges+=("internal/config/config.go staged but config.yaml is not") + echo "$staged" | grep -qE '^deployments/compose/' \ + || nudges+=("internal/config/config.go staged but no deployments/compose/* file — env blocks may need update") +fi + +# EventMessage / ingest types → api.md (event format) + SDK types +if has '^internal/ingest/types\.go$'; then + has '^docs/src/content/docs/api\.md$' \ + || nudges+=("internal/ingest/types.go staged but docs/src/content/docs/api.md (event format section) is not") + has '^clients/ts/src/' \ + || nudges+=("internal/ingest/types.go staged but clients/ts/src/ is not — SDK payload types may need update") +fi + +# Any non-test code change → CHANGELOG.md [Unreleased] +if echo "$staged" | grep -E '^(internal|cmd|clients/ts/src)/' | grep -qvE '_test\.(go|ts)$'; then + has '^CHANGELOG\.md$' \ + || nudges+=("Code changed but CHANGELOG.md is not staged — add an [Unreleased] entry") +fi + +if [ ${#nudges[@]} -gt 0 ]; then + echo "" >&2 + echo "📋 AGENTS.md sync nudges (informational — commit allowed):" >&2 + for n in "${nudges[@]}"; do echo " • $n" >&2; done + echo "" >&2 + echo " If these are real misses, address before pushing. If false positives" >&2 + echo " (internal-only refactor, intentional multi-commit split, etc.), proceed." >&2 +fi + +exit 0 diff --git a/.githooks/pre-push b/.githooks/pre-push new file mode 100755 index 00000000..ee53432a --- /dev/null +++ b/.githooks/pre-push @@ -0,0 +1,40 @@ +#!/usr/bin/env bash +# pre-push: enforces AGENTS.md §Local-First Validation — `make ci` must have +# passed locally for the exact HEAD being pushed. Checked via marker file +# `tmp/ci-passed-` which is written by the `ci` Makefile target. +# +# Bypass: `git push --no-verify` (per AGENTS.md, only when explicitly intentional — +# e.g., draft PR push where you'll iterate; CI will still gate the merge). +# Installed by `make tools` via `git config core.hooksPath .githooks`. + +set -uo pipefail + +cd "$(git rev-parse --show-toplevel)" || exit 1 + +head_sha=$(git rev-parse HEAD 2>/dev/null) +marker="tmp/ci-passed-${head_sha}" + +if [ -f "$marker" ]; then + exit 0 +fi + +short_sha=${head_sha:0:8} + +cat >&2 <` marker — written by `make ci` on success. Blocks the push if `make ci` hasn't been run for the exact HEAD being pushed. + +Bypass with `git commit --no-verify` / `git push --no-verify` only when explicitly intentional (WIP / draft pushes where you accept the consequences). Don't disable the hooks globally; that defeats the gate. + ### If local passes but CI fails Treat as environment mismatch first, test bug second. Reproduce the failure locally (`go test -race -run TestFoo ./...`); if it passes, try concurrent copies to simulate runner contention; only then look at the runner itself. Masking environment issues with longer timeouts compounds — today's 5s bump becomes tomorrow's 30s bump. @@ -190,6 +199,87 @@ Every review comment gets a substantive reply, and every thread gets resolved be > **Known limitation**: Gemini Code Assist silently ignores all files under `.github/workflows/**` — a hardcoded Google default that `.gemini/config.yaml`'s `ignore_patterns` can't remove. For workflow-heavy PRs, Claude review is the primary AI reviewer. Gemini still covers `CHANGELOG.md`, docs, source code, and configuration outside `.github/`. +## Branch Maintenance + +### Syncing a PR branch with main + +When the GitHub UI shows "This branch is out-of-date with the base branch" — or a feature branch needs to absorb upstream `main` commits — **merge, don't rebase**: + +```bash +git fetch origin main +git merge origin/main --no-edit +git push +``` + +Force-pushes (`--force`, `--force-with-lease`) are blocked by `.claude/settings.json`'s `deny` rules and would lose inline review-thread anchors. Rebase changes commit SHAs and requires force-push, so it's wrong for the same reason. Long-lived WaveHouse branches have historically lost `pull_request` event firing (symptom: only `pull_request_target` checks appear) — the recovery is `git merge origin/main`, not close+reopen, not empty commits, not toggling draft/ready. + +The `pre-push` hook will block until `make ci` re-runs after the merge (HEAD changed). That's the point — the merge commit itself needs CI to have passed locally. + +If merge introduces conflicts: surface them to a human reviewer. Don't auto-resolve — collisions in `internal/api/router.go`, `internal/config/config.go`, or `internal/ingest/types.go` can look mechanically resolvable but break runtime behavior. + +See also: `.claude/skills/pr-sync-with-main/SKILL.md` for the same workflow in Claude-Code-skill form. + +## Agent PR Discipline + +These rules apply to AI agents (Claude Code etc.) working on WaveHouse PRs. Humans keep the standard git/gh affordances; agents have additional gating, enforced by `.claude/hooks/agent-bash-gate.sh` (PreToolUse Bash) plus deny rules in `.claude/settings.json`. + +### Drafts only + +Agents must create PRs with `gh pr create --draft`. Only humans transition draft → ready-for-review (`gh pr ready` is blocked for agents). Only humans approve or request changes (`gh pr review --approve` / `--request-changes` are blocked). + +### Human reviewer assignment is humans-only + +Adding/removing human reviewers (`gh pr edit --add-reviewer `, `gh pr edit --add-assignee `, or `POST /repos/.../pulls//requested_reviewers`) is blocked for agents. The `housekeeping.yml` workflow auto-assigns the non-author admin on PR open / ready-for-review; humans handle anything else. + +### Bot reviewer re-triggers go through PR comments + +Agents CAN re-request bot reviewers by mentioning them in PR comments (`gh pr comment` is allowed). This bypasses the reviewer-assignment API entirely: + +| Bot | Re-trigger via PR comment | +| --- | -------------------------- | +| Claude review | `@claude` or `/review` | +| Gemini Code Assist | `@gemini-code-assist` or `/gemini review` | +| CodeRabbit | `@coderabbitai review` | +| Copilot Pull Request Reviewer | No comment-mention; humans use the GitHub UI's re-request button | + +### Pre-push self-review is mandatory on PR branches + +Before pushing to any branch with an open PR, agents must invoke the `pre-push-reviewer` subagent in fresh context. The subagent reviews: + +- The full PR diff against `main` (merge-base) +- The latest commit specifically +- All open PR comments and reviews (top-level + inline) +- CI status / failing checks +- Linked issues' acceptance criteria + +When the subagent's response ends with the parseable line `VERDICT: ship_it`, `.claude/hooks/review-marker.sh` writes `tmp/review-passed-` and the next `git push` succeeds. On `VERDICT: iterate` or `VERDICT: block`, no marker — fix the findings and re-invoke the subagent (always fresh context — never reuse the same session) until ship_it. + +The orchestrator agent cannot override the subagent's system prompt (it's the fixed file content of `.claude/agents/pre-push-reviewer.md`), and the subagent runs in a clean conversation context, so it doesn't share the orchestrator's bias toward its own work. + +### No bypass for agents + +- `git push --no-verify` and `git commit --no-verify` are blocked. +- Writing `tmp/ci-passed-*` or `tmp/review-passed-*` directly (via `touch`, redirect, `cp`, `sh -c` wrapper, `Write`/`Edit` tools) is blocked. +- Markers are written exclusively by `make ci` (ci-passed) and the `pre-push-reviewer` PostToolUse hook (review-passed). Any other path is a discipline violation. + +Humans retain `--no-verify` for explicit intentional bypass — see §"Local-First Validation". + +### Reviewing someone else's PR locally + +For "review PR " workflows, use `.claude/skills/pr-review-locally/SKILL.md`. Procedure: + +```bash +wt switch pr: # worktrunk + gh CLI; or `gh pr checkout ` fallback +``` + +Then invoke `pre-push-reviewer`. Findings stay local — agents must not post comments on the PR manually. To make the bot comment on the PR remotely, fire the CI workflow: + +```bash +gh workflow run "Claude PR review" -f pr_number= +``` + +That's the canonical path (also reachable via `@claude` / `/review` in a PR comment). + ## Documentation Sync Every code change should update the corresponding docs in the same PR. A code change without its doc update is incomplete. @@ -213,6 +303,25 @@ Source-of-truth pairs that must agree: Before finishing a task, grep for the identifiers you touched (field names, env var names, endpoint paths) across docs to catch staleness. +## SDK Sync + +The TypeScript SDK (`@wavehouse/sdk` in `clients/ts/`) is the canonical client and ships from this repo. When backend changes alter the public API surface, the SDK needs corresponding updates. The `pre-commit` git hook flags likely misses informationally; consult this table when deciding what to update. + +| Backend change | SDK considerations | +| -------------- | ------------------ | +| New user-facing API endpoint | Add a typed client method (in `clients/ts/src/client.ts` or the relevant subsystem file: `query-builder.ts`, `pipes.ts`, `policy.ts`, `stream/`, etc.); update `docs/src/content/docs/sdk.md` | +| Change to JWT auth / role extraction | Update auth handling in `clients/ts/src/http.ts` and types in `clients/ts/src/client.ts` | +| Change to `EventMessage` / ingest event format | Update payload types in `clients/ts/src/` (some are codegen-regenerated — re-run the SDK codegen CLI) | +| New / changed structured query AST | Update `clients/ts/src/query-builder.ts` types + builder methods | +| Change to live-query aggregation classification | Update live-query helpers in `clients/ts/src/stream/` | +| Named pipes API change | Update `clients/ts/src/pipes.ts` | +| Policy / access-control change | Update `clients/ts/src/policy.ts` | +| ClickHouse schema-driven type changes | Re-run the SDK codegen CLI; commit regenerated types | + +Internal-only backend changes (middleware refactors, observability internals, dedup implementation, sweeper logic, NATS plumbing) generally don't need SDK updates. The `pre-commit` hook can't tell internal-only from public-surface from staged paths alone, so it'll nudge on anything in `internal/api/`. Ignore the nudge for internal-only changes; act on it for anything user-visible. + +**The decision test**: would a `@wavehouse/sdk` user's *code* need to change to take advantage of (or be compatible with) this change? If yes, SDK update needed. If no (purely internal optimization), no. + ## Common Tasks ### Adding a new API endpoint diff --git a/Makefile b/Makefile index 1f7b28bc..a32326fd 100644 --- a/Makefile +++ b/Makefile @@ -484,6 +484,10 @@ ci: ## Full pipeline — parallel checks, then sequential heavy suites + coverag @$(MAKE) test-integration @$(MAKE) test-e2e @$(MAKE) cov + @# Marker file for the pre-push git hook: confirms `make ci` passed for the + @# exact HEAD being pushed. tmp/ is gitignored. New commits invalidate the + @# marker (different SHA), so `make ci` must re-run before the next push. + @mkdir -p tmp && touch "tmp/ci-passed-$$(git rev-parse HEAD)" @echo "$(GREEN)$(BOLD)✔ All CI checks passed$(RESET)" ##@ Analysis @@ -586,8 +590,13 @@ clean-all: clean clean-test clean-tools ## Full reset — clean + clean-test + c # Go's build cache makes subsequent invocations near-instant. If you need # them pre-compiled (offline CI image baking), run them once with --help. .PHONY: tools -tools: $(GOLANGCI_LINT) $(AIR) go-mod-download install-sdk install-e2e-sdk install-docs ## Install pinned tools, Go modules, and pnpm deps - @echo "$(GREEN)==> Tools cached; Go modules + pnpm packages installed$(RESET)" +tools: $(GOLANGCI_LINT) $(AIR) go-mod-download install-sdk install-e2e-sdk install-docs ## Install pinned tools, Go modules, pnpm deps, and git hooks + @# Install team-wide git hooks via core.hooksPath. Idempotent — running + @# `make tools` repeatedly just re-asserts the config. The .githooks/ + @# directory is committed; this line plumbs git to it. Users can opt out + @# locally by unsetting the config (`git config --unset core.hooksPath`). + @git config core.hooksPath .githooks + @echo "$(GREEN)==> Tools cached; Go modules + pnpm packages installed; git hooks active (.githooks/)$(RESET)" @echo " (go.mod tool deps compile on first \`go tool \` invocation)" # File-target rules: only run when the versioned binary is missing. Bumping diff --git a/README.md b/README.md index a437c934..168aa70f 100644 --- a/README.md +++ b/README.md @@ -110,19 +110,21 @@ go run github.com/Wave-RF/WaveHouse/cmd/wavehouse@latest go install github.com/Wave-RF/WaveHouse/cmd/wavehouse@v0.1.0 ``` -You'll still need ClickHouse running somewhere — point WaveHouse at it via `WH_CH_ADDR`. See [Configuration](docs/configuration.md). +You'll still need ClickHouse running somewhere — point WaveHouse at it via `WH_CH_ADDR`. See [Configuration](docs/src/content/docs/configuration.md). -WaveHouse is built as an application, not a library — `internal/` packages are not importable from outside the module. Use the binary or container; if you need programmatic access, the [TypeScript SDK](docs/sdk.md) is the supported integration surface. +WaveHouse is built as an application, not a library — `internal/` packages are not importable from outside the module. Use the binary or container; if you need programmatic access, the [TypeScript SDK](docs/src/content/docs/sdk.md) is the supported integration surface. ## 💻 Local Development -You'll need **Go 1.26+, GNU Make 4+, Docker (or Podman) with Compose v2, Node.js 20+, and pnpm 10+** on your PATH — see [docs/development.md § Prerequisites](docs/development.md#prerequisites) for the full list, version requirements, and macOS gotchas (BSD Make 3.81 won't work). +You'll need **Go 1.26+, GNU Make 4+, Docker (or Podman) with Compose v2, Node.js 20+, and pnpm 10+** on your PATH — see [docs/development.md § Prerequisites](docs/src/content/docs/development.md#prerequisites) for the full list, version requirements, and macOS gotchas (BSD Make 3.81 won't work). For building and testing WaveHouse locally with hot-reload: ```bash # One-time bootstrap: installs golangci-lint into .bin/, downloads Go modules, -# and runs pnpm install for the SDK + E2E harness. +# runs pnpm install for the SDK + E2E harness, and configures git hooks +# (.githooks/pre-commit runs `make verify`; .githooks/pre-push enforces that +# `make ci` has passed for HEAD before publishing). make tools # Start ClickHouse @@ -134,6 +136,10 @@ make dev WaveHouse will automatically recompile and restart whenever you save a `.go` file. +## 🤖 Working with Claude Code + +The repo ships minimal team-wide [Claude Code](https://claude.com/claude-code) configuration — safety guardrails, a couple of slash commands / a subagent, an auto-format hook, and [worktrunk](https://worktrunk.dev) project hooks for parallel agent workflows. Personal preferences (status line, model, allow lists) stay user-level. See [Claude Code & AI agents](docs/src/content/docs/claude-code.md) for setup + reference. `AGENTS.md` at the repo root is the canonical source of truth for project conventions. + ## 🤝 Contributing We welcome issues, pull requests, and feedback! Please see our [CONTRIBUTING.md](CONTRIBUTING.md) for guidelines on how to structure your code and run the integration test suites. diff --git a/docs/src/config/sidebar.ts b/docs/src/config/sidebar.ts index d2351f50..36d1eafb 100644 --- a/docs/src/config/sidebar.ts +++ b/docs/src/config/sidebar.ts @@ -28,6 +28,7 @@ export const sidebar: StarlightUserConfig["sidebar"] = [ label: "Contributing", items: [ { label: "Development", slug: "development" }, + { label: "Claude Code & AI agents", slug: "claude-code" }, { label: "Contributing Guide", link: "https://github.com/Wave-RF/WaveHouse/blob/main/CONTRIBUTING.md", diff --git a/docs/src/content/docs/claude-code.md b/docs/src/content/docs/claude-code.md new file mode 100644 index 00000000..5d2e288a --- /dev/null +++ b/docs/src/content/docs/claude-code.md @@ -0,0 +1,237 @@ +--- +title: "Claude Code & AI agents" +description: "Working with Claude Code in the WaveHouse repo — what ships in .claude/ and .githooks/, how enforcement is layered, and recommended user-level setup." +sidebar: + order: 10 +--- + +WaveHouse ships minimal team-wide [Claude Code](https://claude.com/claude-code) configuration. The repo commits only what's distinctly useful to every contributor; cosmetic and personal choices stay at the user level. + +If you're new to Claude Code itself, the [official docs](https://code.claude.com/docs) cover the basics. This page is WaveHouse-specific. + +## Quick setup + +1. **Install Claude Code**: follow the [official install guide](https://code.claude.com/docs/en/quickstart). On macOS: `brew install --cask claude-code`. + +2. **Authenticate**: run `claude setup-token` once. The CI Claude review workflow uses `CLAUDE_CODE_OAUTH_TOKEN`; locally you just need to be logged in to your Max subscription. + +3. **Bootstrap the repo**: `make tools`. This installs Go tools, pnpm deps, and **also configures git hooks** (`git config core.hooksPath .githooks`). Without this step, the team's pre-commit / pre-push gates won't fire. + +4. **Optional — worktrunk**: install [worktrunk](https://worktrunk.dev) for parallel-agent worktree management. The team's project hooks live in `.config/wt.toml`. + +## How enforcement is layered + +The team has two distinct gate layers, plus optional Claude-Code-only ergonomics: + +| Layer | Lives in | Applies to | Purpose | +| ----- | -------- | ---------- | ------- | +| **Git hooks** | `.githooks/` (installed by `make tools`) | Humans + Claude uniformly | Hard enforcement: `make verify` on commit, `make ci` passed before push | +| **Claude Code hook** | `.claude/hooks/gofumpt-on-save.sh` (wired in `.claude/settings.json`) | Claude only | UX: auto-format on file edits (humans get this from their IDE) | +| **Claude Code skills / agents / commands** | `.claude/skills/`, `.claude/agents/`, `.claude/commands/` | Claude only (when relevant) | Workflow guidance and on-demand helpers — not gates | + +Git hooks are the source of truth for "must pass before merge." Claude Code config layers on agentic affordances; it doesn't substitute for the gates. + +## Git hooks (`.githooks/`) + +Two scripts, both committed to the repo: + +| Hook | Behavior | +| ---- | -------- | +| `pre-commit` | Runs `make verify` (tidy + fmt + vulncheck + lint, ~30s) — **blocks on failure**. Then emits informational stderr nudges for likely doc-sync and SDK-sync misses (see AGENTS.md §Documentation Sync and §SDK Sync). Nudges don't block. | +| `pre-push` | Checks for `tmp/ci-passed-` marker. The `make ci` target writes this marker on success. **Blocks** if absent — Claude (or you) runs `make ci`, sees output, fixes failures, retries push. | + +**Humans** can bypass with `git commit --no-verify` / `git push --no-verify` for intentional WIP / draft pushes. **Agents cannot** — `--no-verify` and direct marker writes are blocked (see [Agent PR Discipline](#agent-pr-discipline)). + +The marker invalidates on every commit (HEAD SHA changes), so `make ci` re-runs after each new commit before pushing. That's the AGENTS.md rule made literal. + +## What's in `.claude/` and `.config/` + +| Path | Purpose | +| ---- | ------- | +| `.claude/settings.json` | Team-wide: `deny` permissions (force-push, gh pr merge / ready / approve, --no-verify, marker forgery, secrets), `worktree.baseRef: "fresh"` + symlinkDirectories, all four hooks wired | +| `.claude/hooks/gofumpt-on-save.sh` | PostToolUse Edit/Write/MultiEdit: auto-formats `.go` files | +| `.claude/hooks/agent-bash-gate.sh` | PreToolUse Bash: enforces Agent PR Discipline (drafts only, no human reviewer adds, no `--no-verify`, marker required on PR pushes) | +| `.claude/hooks/review-marker.sh` | PostToolUse Agent: writes `tmp/review-passed-` when `pre-push-reviewer` returns `VERDICT: ship_it` | +| `.claude/commands/cover.md` | `/cover [suite]` — suite dispatch + coverage threshold analysis | +| `.claude/agents/pre-push-reviewer.md` | `pre-push-reviewer` subagent — canonical pre-push review, also used for auditing others' PRs locally | +| `.claude/skills/pr-sync-with-main/SKILL.md` | "Fix this stale PR" workflow — merge origin/main, never rebase or force-push | +| `.claude/skills/pr-review-locally/SKILL.md` | "Review PR locally" workflow — `wt switch pr:` + `pre-push-reviewer`, no PR comments | +| `.claude/settings.local.json` | **Your personal overrides** — gitignored; put model choice, status line, allow lists, etc. here | +| `.config/wt.toml` | Worktrunk project hooks (post-start, pre-merge, pre-remove) | + +Notably absent: no `.mcp.json`, no committed status line, no `permissions.allow` list. See [GitHub access](#github-access-gh-cli-vs-mcp) and [Permission posture](#permission-posture) below. + +## The one slash command + +| Command | What it does | +| ------- | ------------ | +| `/cover [suite]` | Renders coverage for a suite (unit / integration / e2e / sdk / all / merge) and surfaces drops below threshold | + +To add a command: drop a `.md` file in `.claude/commands/`. Filename becomes the slash command. Frontmatter: `description` and `argument-hint`; body is the prompt with `$ARGUMENTS`. + +## Subagents + +| Subagent | When to use | +| -------- | ----------- | +| `pre-push-reviewer` | **Mandatory before pushing to a PR branch** (enforced by `.claude/hooks/agent-bash-gate.sh`). Also used for auditing someone else's PR after `wt switch pr:`. Runs the canonical `.github/prompts/pr-review.md` workflow against the local branch in fresh context. Fetches PR comments + CI status + linked-issue acceptance criteria when on a PR branch. Returns `[MUST]`/`[SHOULD]`/`[MAY]` findings + a parseable `VERDICT: ship_it\|iterate\|block` line that drives the pre-push marker. | + +Invoke via the `Agent` tool with `subagent_type: pre-push-reviewer`, or via `/agents`. + +To add a subagent: drop a `.md` file in `.claude/agents/`. Frontmatter: `description` (used by main Claude to decide when to delegate), `tools`, `model`. Body is the system prompt. + +## Skills + +Skills load automatically into Claude's context when conversation patterns match their `description`. + +| Skill | Triggers on | +| ----- | ----------- | +| `pr-sync-with-main` | When a PR shows "out-of-date with base branch", or a user asks to "fix the PR" / "sync with main". Documents the merge-not-rebase procedure and the WaveHouse-specific reason long-lived branches need it. | +| `pr-review-locally` | When a user asks to "review PR ", "audit PR ", "look at PR " — pulls the PR down via `wt switch pr:` (or `gh pr checkout`), runs `pre-push-reviewer` in fresh context, surfaces findings without commenting on the PR. Documents the local-only vs CI-claude-review-comment-mode distinction. | + +To add a skill: create `.claude/skills//SKILL.md` with frontmatter `name` + `description` and the workflow body. Description quality matters — that's what Claude matches against to load the skill. + +## Agent PR Discipline + +Agents (Claude Code etc.) have additional gating beyond what humans face — enforced by `.claude/hooks/agent-bash-gate.sh` (PreToolUse Bash) + deny rules in `.claude/settings.json`. Humans keep full git/gh affordances; agents have these extra constraints: + +- **Drafts only.** `gh pr create` must include `--draft`. Only humans transition draft → ready (`gh pr ready` is blocked), approve (`gh pr review --approve` is blocked), or request changes (`gh pr review --request-changes` is blocked). +- **No human reviewer assignment.** `gh pr edit --add-reviewer / --add-assignee` and `POST /requested_reviewers` are blocked. The `housekeeping.yml` workflow auto-assigns; humans handle the rest. +- **Bot re-triggers via comments.** Agents CAN mention bots in PR comments to re-trigger reviews — `@coderabbitai review`, `@gemini-code-assist /gemini review`, `@claude` or `/review`, etc. This goes through `gh pr comment` (allowed), not the reviewer API. +- **Pre-push review required on PR branches.** Before `git push` to a branch with an open PR, the agent must invoke `pre-push-reviewer` (fresh context). On `VERDICT: ship_it`, the marker auto-writes and push proceeds. On iterate/block, fix and re-invoke (always fresh context) until clean. +- **No bypass.** `--no-verify`, direct marker writes, and creative wrappers (`sh -c "touch ..."`, redirects, etc.) are all blocked. Markers come from `make ci` and the review-marker hook — nowhere else. +- **PR reviews on others' PRs stay local by default.** Use `pr-review-locally` skill for local-only audits. To make the bot comment on the PR remotely, fire the CI workflow: `gh workflow run "Claude PR review" -f pr_number=` — that's the canonical bot-comment path. + +Full ruleset and rationale: AGENTS.md §"Agent PR Discipline". + +## Worktrunk integration + +We use [worktrunk](https://worktrunk.dev) as the recommended worktree manager. It creates real `git worktree add` directories, so the committed `.claude/` config Just Works in every worktree. + +Project hooks in `.config/wt.toml`: + +| Hook | Command | Why | +| ---- | ------- | --- | +| `post-start` | `make tools` | Bootstraps the new worktree: tools, modules, pnpm deps, **and git hooks** | +| `pre-merge` | `make verify` | Fast pre-merge gate (same as pre-commit) | +| `pre-remove` | `git status --short` | Surfaces uncommitted work before tearing down | + +Common workflow: + +```bash +wt switch -c feat/new-thing # fresh worktree off origin/main +wt switch -x claude -c feat/x # spawn an agent in a new worktree +wt list # status of all worktrees +wt merge # squash + rebase + merge + clean up +wt remove feat/new-thing # tear down (warns on uncommitted work) +``` + +User-specific worktrunk config goes in `~/.config/worktrunk/config.toml`; the committed `.config/wt.toml` has team-wide hooks only. + +## GitHub access: gh CLI vs MCP + +**We use `gh` CLI as the canonical GitHub access path**, not a GitHub MCP server. Reasons: + +- `gh` is already a hard dev requirement +- Works identically in Claude Code, terminal, and shell scripts +- No extra auth / approval / npx cold-start + +The GitHub MCP server is useful for cross-repo code search and bulk graph queries, but neither is a daily WaveHouse pattern. If you want it, add at user level: + +```jsonc +// ~/.claude.json — your user-level config +{ + "mcpServers": { + "github": { + "command": "npx", + "args": ["-y", "@modelcontextprotocol/server-github"], + "env": { "GITHUB_PERSONAL_ACCESS_TOKEN": "${GITHUB_TOKEN}" } + } + } +} +``` + +Then `export GITHUB_TOKEN=$(gh auth token)` in your shell rc. + +## Other useful MCP servers (user-level, optional) + +None of these are committed at project level — pick what you actually use. + +- **[Grafana MCP](https://github.com/grafana/mcp-grafana)** — for querying Prometheus / Loki / Tempo / Pyroscope from Claude when debugging observability work. Useful if you touch `internal/observability/`. +- **ClickHouse MCP** (community) — direct schema introspection + query against your local `make dev` ClickHouse. Useful for `internal/discovery/` and ingest work, but `make deps-shell` (clickhouse-client REPL) is often enough. + +When [issue #121](https://github.com/Wave-RF/WaveHouse/issues/121) lands a SigNoz dev stack with `make dev-obs`, Grafana MCP pointed at that dev environment will become a natural choice for trace / log inspection. + +## Permission posture + +`.claude/settings.json` is structured around the reality that most contributors run Claude Code in `bypassPermissions` (yolo). In that mode: + +- ✅ `permissions.deny` **still fires** — the only programmatic guardrail +- ❌ `permissions.allow` / `permissions.ask` are moot (everything's already allowed) +- ✅ Hooks (Claude Code AND git) **still fire** — the behavioral layer + +So the file is `deny`-only by design. Personal allow / ask lists go in `.claude/settings.local.json` (gitignored). + +`defaultMode` is intentionally not set in committed config. Each user picks their preferred mode. + +The deny list blocks: + +| Blocked | Why | +| ------- | --- | +| `git push --force`, `-f`, `--force-with-lease` | Force-pushing is destructive; also loses inline review-comment anchors | +| `git reset --hard origin:*` | Throws away local work | +| `gh pr merge`, `gh repo delete`, `gh release delete` | Irreversible / shared-state | +| `gh workflow disable`, `gh secret delete` | Operational footguns | +| `make deps-wipe`, `make clean-all` | Wipes data / docker volumes / installed tools | +| `rm -rf /`, `rm -rf $HOME`, `sudo rm` | Filesystem destruction | +| `Read(./.env)`, `Read(./.env.*)`, `Read(./secrets/**)` | Secrets shouldn't enter Claude's context | + +## Status line, output style, model + +Not committed at project level. Personal preference — put in `.claude/settings.local.json`: + +```jsonc +{ + "statusLine": { "type": "command", "command": "~/.config/claude/statusline.sh" }, + "outputStyle": "default", + "model": "claude-opus-4-7" +} +``` + +## Daily workflow + +1. Write code (gofumpt-on-save formats Go files as you go). +2. `git commit` → pre-commit hook runs `make verify` + sync nudges. Fix anything that fails. +3. `git push` (first time on a feature branch) → pre-push hook blocks until `make ci` passed for HEAD. Run `make ci`, fix, retry push. +4. Open the PR with `gh pr create --draft` (agents required to use `--draft`; humans flip to ready when ready). +5. **Subsequent pushes** → agent-bash-gate hook ALSO requires `pre-push-reviewer` subagent to have run (fresh context) and returned `VERDICT: ship_it`. Invoke the subagent → on Ship it, marker auto-writes → push succeeds. On Iterate/Block, fix, re-invoke (fresh context each time), repeat. +6. CI workflows fire on the new HEAD. Address review comments per AGENTS.md §Review Response. + +Helpers along the way: + +- Stale PR ("out-of-date with base branch") → ask Claude to "fix the PR" (loads `pr-sync-with-main` skill — merges main, doesn't rebase or force-push). +- Reviewing someone else's PR → "review PR 120 locally" / "audit PR 120" (loads `pr-review-locally` skill — `wt switch pr:120` + `pre-push-reviewer`, no PR comments). +- Re-trigger a bot reviewer → ask Claude to "ping coderabbit again" / "re-request claude review" — Claude posts the appropriate `@` comment on the PR. +- Coverage check on a specific suite → `/cover unit` (or `integration`, `e2e`, `sdk`, `all`). +- Make the bot comment on a PR remotely → ask Claude to "fire the CI claude-review on PR 120" — `gh workflow run "Claude PR review" -f pr_number=120`. + +**Memory**: Claude Code maintains per-project memory in `~/.claude/projects//memory/`. `AGENTS.md` is the SHARED source of truth; memory is for personal observations / preferences that don't belong in committed config. + +## Extending + +The `.claude/` layout is intentionally small. Add things when they earn their keep: + +- New slash commands as recurring workflows emerge +- New subagents for specialized one-shot tasks +- New skills for context-loaded workflow patterns +- New Claude Code hooks for edit-time UX gaps (NOT for enforcement — gates go in `.githooks/` so they apply to humans too) +- Adjust `.githooks/pre-commit` / `pre-push` if the gates feel wrong + +If you build something useful, commit it and update this doc. + +## Reference + +- [AGENTS.md](https://github.com/Wave-RF/WaveHouse/blob/main/AGENTS.md) — project conventions, architecture, code style, doc-sync rules, SDK-sync rules, branch maintenance. Source of truth for all AI agent work. +- [Claude Code docs](https://code.claude.com/docs) — official Claude Code reference. +- [worktrunk.dev](https://worktrunk.dev) — worktree manager. +- `.github/prompts/pr-review.md` — the prompt the CI Claude review runs (and what `pre-push-reviewer` mirrors locally). +- `.gemini/styleguide.md` — Gemini Code Assist review style. diff --git a/docs/src/content/docs/development.md b/docs/src/content/docs/development.md index 9e3d8c25..3dcdb327 100644 --- a/docs/src/content/docs/development.md +++ b/docs/src/content/docs/development.md @@ -42,6 +42,13 @@ pnpm --version # 10.33+ If any of those are wrong/missing, the Makefile recipes will fail with confusing errors (e.g. `--output-sync` is unrecognized on Make 3.81; `pnpm: command not found` on `make test-sdk`). +### Optional but recommended + +| Tool | Why | Install | +| ---- | --- | ------- | +| **[Claude Code](https://claude.com/claude-code)** | The repo ships team-wide configuration in `.claude/` — slash commands, subagents, hooks, status line. See [Claude Code & AI agents](claude-code.md) for setup. | `brew install --cask claude-code` (macOS) or follow [official install](https://code.claude.com/docs/en/quickstart) | +| **[worktrunk](https://worktrunk.dev)** | Wraps `git worktree` for parallel-agent workflows. Project hooks live in `.config/wt.toml` (auto-runs `make tools` on new worktrees, `make verify` on pre-merge). | `brew install worktrunk && wt config shell install` | + ## Quick Start This is the fastest way to get a fully functional local environment: From 6c79315c7e7b975a3d2f0c73675ad095e8847a5f Mon Sep 17 00:00:00 2001 From: Eric Andrechek Date: Tue, 19 May 2026 10:59:28 -0400 Subject: [PATCH 04/18] fix(claude): tighten no-verify regex to skip quoted message bodies MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous check matched the literal flag substring anywhere in the command, false-positiving when a commit message body documented the rule (e.g., "this hook blocks `git push --no-verify`"). The new regex only matches when the flag appears in the args of `git push|commit` BEFORE any quote / command-substitution / heredoc character — so the same flag appearing inside a quoted -m message or a heredoc body no longer trips the gate. Still honest-agent defense, not adversarial: eval / sh -c wrappers around `git push` with the bypass flag could still slip through. AGENTS.md §"Agent PR Discipline" makes the rule explicit so the honest-agent path stays correct. Co-Authored-By: Claude Opus 4.7 (1M context) --- .claude/hooks/agent-bash-gate.sh | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/.claude/hooks/agent-bash-gate.sh b/.claude/hooks/agent-bash-gate.sh index da99f455..9884bf4b 100755 --- a/.claude/hooks/agent-bash-gate.sh +++ b/.claude/hooks/agent-bash-gate.sh @@ -47,10 +47,15 @@ git_subcmd() { } # --- 1. --no-verify on git push/commit -------------------------------------- -if git_subcmd 'push' || git_subcmd 'commit'; then - if printf '%s\n' "$cmd" | grep -qE '(^|[[:space:]])--no-verify\b'; then - block "git push/commit with --no-verify is not permitted for agents. Run the gates." - fi +# Match --no-verify only when it appears in the args of `git push|commit` +# BEFORE any quote (" or '), command substitution ($), or heredoc marker (<). +# That excludes false positives where the literal string sits inside a quoted +# commit message body (e.g., when documenting the rule). Honest-agent defense, +# not adversarial — eval / sh -c wrappers around `git push --no-verify` could +# still bypass; AGENTS.md §"Agent PR Discipline" makes the rule explicit. +no_verify_re='(^|[[:space:];|&])git[[:space:]]+(push|commit)\b[^"'\''$<]*[[:space:]]--no-verify\b' +if printf '%s\n' "$cmd" | grep -qE "$no_verify_re"; then + block "git push/commit with --no-verify is not permitted for agents. Run the gates." fi # --- 2. gh pr create without --draft ---------------------------------------- From 94e6fd7792aaa14f175628678a7de41f36e576cc Mon Sep 17 00:00:00 2001 From: Eric Andrechek Date: Tue, 19 May 2026 11:25:33 -0400 Subject: [PATCH 05/18] fix(claude): add name field so pre-push-reviewer agent registers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Without `name:` in the YAML frontmatter, Claude Code's agent loader skips the file silently — no warning, no registration. Result: `pre-push-reviewer` never appears as an available `subagent_type`, the Agent tool falls back to `general-purpose`, and `.claude/hooks/review-marker.sh` never writes the `tmp/review-passed-` marker (it gates on `subagent_type == "pre-push-reviewer"` specifically) — so the pre-push gate stays closed. The `name:` field is what the loader uses as the registered slug; every working subagent in the marketplace plugins (`code-reviewer`, `code-explorer`, etc.) has it as the first frontmatter key. Bringing this file in line. Co-Authored-By: Claude Opus 4.7 (1M context) --- .claude/agents/pre-push-reviewer.md | 1 + 1 file changed, 1 insertion(+) diff --git a/.claude/agents/pre-push-reviewer.md b/.claude/agents/pre-push-reviewer.md index 24b145dd..1e4994c9 100644 --- a/.claude/agents/pre-push-reviewer.md +++ b/.claude/agents/pre-push-reviewer.md @@ -1,4 +1,5 @@ --- +name: pre-push-reviewer description: Reviews the current branch's full delta against main using the canonical WaveHouse review prompt (.github/prompts/pr-review.md). Use before pushing to any PR branch (mandatory per AGENTS.md §Agent PR Discipline) or to audit someone else's PR after `wt switch pr:N` / `gh pr checkout N`. Considers full PR diff, latest commit, all open PR comments + reviews, and CI / failing-test status. Runs in fresh context for objectivity. Returns [MUST]/[SHOULD]/[MAY] findings plus a parseable verdict line that drives the pre-push marker. tools: Bash, Read, Glob, Grep model: opus From c03edcd8a93c0cba75e9eb82a252a727a6c90fd4 Mon Sep 17 00:00:00 2001 From: Eric Andrechek Date: Tue, 19 May 2026 12:20:09 -0400 Subject: [PATCH 06/18] chore(claude): tighten ship_it rule + drop porous marker-forgery regex MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two policy shifts in the agent PR discipline layer: 1. **ship_it now requires zero findings.** Previously [MAY]s alone didn't preclude ship_it. The rubric in .claude/agents/pre-push-reviewer.md and AGENTS.md §"Agent PR Discipline" now treats any [MUST]/[SHOULD]/[MAY] finding as iterate — the orchestrator loops review → fix → review in fresh context until findings list is empty. [MAY] becomes a real commitment ("actually do before merge") rather than optional polish; genuinely-optional observations go in the prose preamble or get dropped. 2. **Marker-forgery is honest-agent, not regex-enforced.** Dropped section 7 of agent-bash-gate.sh (the touch/redirect/cp/mv/sh -c regex around tmp/(ci|review)-passed-*) and the redundant ./-prefixed Write/Edit denies in settings.json. Bash can write a file by a dozen paths (tee, dd, python -c, perl, awk, go run, etc.) and the regex was a porous game of whack-a-mole that oversold what it delivered. Kept the cheap settings.json denies (Bash(touch tmp/ci-passed:*), Write/Edit on the canonical paths) for the obvious idioms; the rest is an explicit honest-agent rule in AGENTS.md ("you do not write a marker file by any other means — period"). Plus the [SHOULD]/[MAY] cleanups from the prior pre-push review (which under the new rule would have forced iteration before the last push): - Tightened requested_reviewers gate to cover PUT/PATCH in addition to POST (idempotent reviewer-replace verb was previously slipping through). - Fixed pre-push-reviewer.md's git-diff doc — main...HEAD (three dots) is the correct merge-base-vs-HEAD form. - Dropped hardcoded -R Wave-RF/WaveHouse from pr-review-locally SKILL.md (gh picks up the worktree's repo, consistent with AGENTS.md examples). - Dropped the WAVEHOUSE_SKIP_PUSH_GATE hint from .githooks/pre-push (the comment admitted it wasn't implemented; either implement or drop, and --no-verify is the right human bypass anyway). - Added the missing [Unreleased] CHANGELOG entry covering the .claude/ + .githooks/ + AGENTS.md infrastructure that landed in this branch. Co-Authored-By: Claude Opus 4.7 (1M context) --- .claude/agents/pre-push-reviewer.md | 18 +++++--- .claude/hooks/agent-bash-gate.sh | 54 ++++++++++------------- .claude/settings.json | 4 -- .claude/skills/pr-review-locally/SKILL.md | 4 +- .githooks/pre-push | 8 ++-- AGENTS.md | 12 ++--- CHANGELOG.md | 1 + 7 files changed, 46 insertions(+), 55 deletions(-) diff --git a/.claude/agents/pre-push-reviewer.md b/.claude/agents/pre-push-reviewer.md index 1e4994c9..dedd3ddd 100644 --- a/.claude/agents/pre-push-reviewer.md +++ b/.claude/agents/pre-push-reviewer.md @@ -11,7 +11,7 @@ You are reviewing the current branch's delta against main, using the same prompt Read `.github/prompts/pr-review.md` first. That file is the canonical WaveHouse review prompt and applies here verbatim — including the focus areas (correctness → security → performance → testing → docs/sdk-sync), the severity tags `[MUST]`/`[SHOULD]`/`[MAY]`, the noise filter, and the verdict rules. -The diff source here is the local working state, computed as `git diff $(git merge-base HEAD main)` (or equivalently `git diff main...HEAD`). +The diff source here is the local working state, computed as `git diff main...HEAD` (three dots — equivalent to `git diff $(git merge-base main HEAD) HEAD`, i.e. merge-base vs HEAD). Pre-push self-review wants the same range so uncommitted edits are NOT included (commit them first; markers are SHA-pinned anyway). ## Process @@ -20,7 +20,7 @@ The diff source here is the local working state, computed as `git diff $(git mer 2. Compute the branch diff: ```bash - git diff $(git merge-base HEAD main) + git diff main...HEAD ``` 3. For each changed file, read its current state. Don't just look at the diff — context matters. @@ -84,13 +84,17 @@ VERDICT: ship_it (or `VERDICT: iterate` / `VERDICT: block`) -## Verdict mapping (matches `.gemini/styleguide.md` + `.github/prompts/pr-review.md`) +## Verdict mapping -- **`Ship it`** + `VERDICT: ship_it` — no `[MUST]`s; few or no `[SHOULD]`s. Pre-push marker auto-writes, push proceeds. -- **`Iterate`** + `VERDICT: iterate` — one or more `[MUST]`s that aren't `Block`-level, or multiple `[SHOULD]`s. Fix and re-invoke (always in fresh context). -- **`Block`** + `VERDICT: block` — a `[MUST]` that's CRITICAL/HIGH security, data-loss risk, or broken core invariant. Cannot proceed without addressing. +WaveHouse uses a stricter rule than `.gemini/styleguide.md` / `.github/prompts/pr-review.md`: **`ship_it` requires zero findings at any severity**. If there is anything left to do, the PR isn't shippable — "ship it, just do this one thing first" is iteration, not shipping. -`[MAY]` findings alone don't preclude `Ship it`. +- **`Ship it`** + `VERDICT: ship_it` — `[MUST]`, `[SHOULD]`, and `[MAY]` sections are all empty. Pre-push marker auto-writes, push proceeds. +- **`Iterate`** + `VERDICT: iterate` — any `[MUST]` / `[SHOULD]` / `[MAY]` finding exists, but none are block-level. The orchestrator fixes the findings and re-invokes this subagent (always in fresh context) until ship_it. +- **`Block`** + `VERDICT: block` — a `[MUST]` that's CRITICAL/HIGH security, data-loss risk, broken core invariant, or otherwise needs human/maintainer attention (architectural disagreement, missing CI signal, etc.). Cannot proceed without addressing. + +### What this means for `[MAY]` + +Under this rubric, **`[MAY]` is a real commitment** — "I'd actually do this before merge," not "optional polish." If you're tempted to raise a finding because it's nice-to-have but you wouldn't ask the author to act on it before merge, drop it from the findings list. Put it in the prose preamble as an observation, or leave it out entirely. The noise filter from `pr-review.md` is even stronger here: any finding in the list is a blocker to ship_it. ## Framing diff --git a/.claude/hooks/agent-bash-gate.sh b/.claude/hooks/agent-bash-gate.sh index 9884bf4b..498196e8 100755 --- a/.claude/hooks/agent-bash-gate.sh +++ b/.claude/hooks/agent-bash-gate.sh @@ -9,12 +9,17 @@ # 3. gh pr ready (humans only — draft→ready is a deliberate human signal) # 4. gh pr edit --add-reviewer / --add-assignee (human reviewer assignment is humans-only; # bot reviewers are re-triggered via PR comments, not the reviewer API) -# 5. gh api .../requested_reviewers POST (the API form of --add-reviewer) +# 5. gh api .../requested_reviewers POST/PUT (the API form of --add-reviewer) # 6. gh pr review --approve / --request-changes (only humans take formal review actions; # bot reviewers use inline review comments + sticky summaries instead) -# 7. Direct marker forgery (touch / redirect / sh -c wrappers writing to -# tmp/(ci|review)-passed-*) -# 8. git push without required markers (ci-passed always; review-passed on PR branches) +# 7. git push without required markers (ci-passed always; review-passed on PR branches) +# +# Marker forgery (writing tmp/(ci|review)-passed-* by any means) is NOT blocked +# here. The .claude/settings.json deny list catches the obvious tool-level +# attempts (Bash(touch tmp/...:*), Write/Edit on the paths); everything else +# relies on the honest-agent model documented in AGENTS.md §"Agent PR +# Discipline" — Bash can write a file by a dozen paths and regex enforcement +# becomes a porous game of whack-a-mole that oversells what it delivers. # # Anything blocked here can typically be re-run by a human from terminal — these # rules are for the agent path, not the underlying git/gh capabilities. @@ -77,11 +82,14 @@ if printf '%s\n' "$cmd" | grep -qE '(^|[[:space:];|&]+)gh[[:space:]]+pr[[:space: fi fi -# --- 5. gh api .../requested_reviewers POST (humans-only reviewer-add API) -- +# --- 5. gh api .../requested_reviewers any write verb (humans-only API) ----- +# Both POST (add) and PUT (replace) on /pulls//requested_reviewers are +# reviewer-write operations. Neither has a legitimate agent use case — bot +# reviewers are re-triggered via PR comments. Match any reviewer-write idiom. if printf '%s\n' "$cmd" | grep -qE '(^|[[:space:];|&]+)gh[[:space:]]+api\b' && \ printf '%s\n' "$cmd" | grep -qE 'requested_reviewers'; then - if printf '%s\n' "$cmd" | grep -qE '(-X[[:space:]]*POST|--method[[:space:]]*POST|[[:space:]]-f[[:space:]]+reviewers=|[[:space:]]-F[[:space:]]+reviewers=)'; then - block "POST to /requested_reviewers is the API form of --add-reviewer; humans-only. For bot reviewers, post a PR comment mentioning them." + if printf '%s\n' "$cmd" | grep -qE '(-X[[:space:]]*(POST|PUT|PATCH)|--method[[:space:]]*(POST|PUT|PATCH)|[[:space:]]-f[[:space:]]+reviewers=|[[:space:]]-F[[:space:]]+reviewers=)'; then + block "Write requests to /requested_reviewers are the API form of --add-reviewer; humans-only. For bot reviewers, post a PR comment mentioning them." fi fi @@ -95,25 +103,7 @@ if printf '%s\n' "$cmd" | grep -qE '(^|[[:space:];|&]+)gh[[:space:]]+pr[[:space: fi fi -# --- 7. Marker forgery ------------------------------------------------------ -# Block any *write-like* operation targeting tmp/(ci|review)-passed-*. -# Read operations (ls/cat/find) on the markers are fine. -if printf '%s\n' "$cmd" | grep -qE 'tmp/(ci|review)-passed-'; then - if printf '%s\n' "$cmd" | grep -qE '(touch|install)[[:space:]]+[^&|;]*tmp/(ci|review)-passed-'; then - block "Direct marker creation is forbidden. Markers are written by 'make ci' (ci-passed) and the pre-push-reviewer agent's PostToolUse hook (review-passed)." - fi - if printf '%s\n' "$cmd" | grep -qE '(cp|mv)[[:space:]]+[^&|;]+[[:space:]]+[^&|;]*tmp/(ci|review)-passed-'; then - block "Direct marker creation via cp/mv is forbidden." - fi - if printf '%s\n' "$cmd" | grep -qE '>[[:space:]]*tmp/(ci|review)-passed-'; then - block "Direct marker creation via output redirect is forbidden." - fi - if printf '%s\n' "$cmd" | grep -qE '\b(sh|bash|zsh|env)\b[[:space:]]+-c[[:space:]]+.*tmp/(ci|review)-passed-'; then - block "Wrapping marker creation in a shell -c to bypass the gate is forbidden." - fi -fi - -# --- 8. git push: check markers --------------------------------------------- +# --- 7. git push: check markers --------------------------------------------- # Only on actual `git push` invocations (not `git push --help`, not `gh pr push`). if git_subcmd 'push'; then head_sha=$(git rev-parse HEAD 2>/dev/null || echo "") @@ -122,7 +112,7 @@ if git_subcmd 'push'; then ci_marker="tmp/ci-passed-${head_sha}" review_marker="tmp/review-passed-${head_sha}" - # 8a. ci-passed required for every push (mirrors the universal git pre-push hook; + # 7a. ci-passed required for every push (mirrors the universal git pre-push hook; # firing here too gives Claude a more actionable error inside its session). if [ ! -f "$ci_marker" ]; then cat >&2 </dev/null || echo "") if [ -n "$branch" ] && [ "$branch" != "main" ]; then pr_state=$(gh pr view "$branch" --json state --jq .state 2>/dev/null || echo "") @@ -163,9 +153,11 @@ If the agent returns VERDICT: iterate or VERDICT: block, address the findings and re-invoke the agent (always in fresh context — never the same session) until ship_it. -Per AGENTS.md §"Agent PR Discipline", you cannot bypass this with --no-verify -or by writing the marker file directly. CI's claude-review will also fire on -push, but pre-push review catches issues before consuming shared capacity. +Per AGENTS.md §"Agent PR Discipline", agents do not bypass this with +--no-verify, and you do not write the marker file directly by any means — +the marker is wrong-shaped if you're the one writing it. CI's claude-review +will also fire on push, but pre-push review catches issues before consuming +shared capacity. EOF exit 2 fi diff --git a/.claude/settings.json b/.claude/settings.json index 9004dc6d..2b982569 100644 --- a/.claude/settings.json +++ b/.claude/settings.json @@ -32,10 +32,6 @@ "Bash(touch tmp/ci-passed:*)", "Bash(touch tmp/review-passed:*)", - "Write(./tmp/ci-passed-*)", - "Write(./tmp/review-passed-*)", - "Edit(./tmp/ci-passed-*)", - "Edit(./tmp/review-passed-*)", "Write(tmp/ci-passed-*)", "Write(tmp/review-passed-*)", "Edit(tmp/ci-passed-*)", diff --git a/.claude/skills/pr-review-locally/SKILL.md b/.claude/skills/pr-review-locally/SKILL.md index ecd30213..72014def 100644 --- a/.claude/skills/pr-review-locally/SKILL.md +++ b/.claude/skills/pr-review-locally/SKILL.md @@ -64,10 +64,10 @@ The above procedure. Findings stay in your local session. Nothing is posted to t If the user explicitly asks for the bot to post on the PR (not just local feedback), the mechanism is the **CI claude-review workflow** — NOT manual comments from the local session. Trigger it: ```bash -gh workflow run "Claude PR review" -R Wave-RF/WaveHouse -f pr_number=120 +gh workflow run "Claude PR review" -f pr_number= ``` -That fires `.github/workflows/claude-review.yml` against PR 120. The workflow: +That fires `.github/workflows/claude-review.yml` against PR ``. `gh` picks up the repo from the current worktree, matching the convention used elsewhere in AGENTS.md. The workflow: - Runs the same `.github/prompts/pr-review.md` prompt - Posts inline review comments at line-level diff --git a/.githooks/pre-push b/.githooks/pre-push index ee53432a..dee6ae70 100755 --- a/.githooks/pre-push +++ b/.githooks/pre-push @@ -31,10 +31,8 @@ CI for the exact commit being published. Run: …then push again. (\`make ci\` takes ~2min warm, ~5-8min cold.) -Bypass options: - • For an intentional WIP / draft push: git push --no-verify - • To skip a single push: WAVEHOUSE_SKIP_PUSH_GATE=1 git push (not currently - honored — use --no-verify instead; this line is a reminder if you find - yourself wanting it often, that's a signal to revisit the gate.) +Bypass: for an intentional WIP / draft push, use `git push --no-verify` +(humans only — agents are blocked from --no-verify by +.claude/hooks/agent-bash-gate.sh). EOF exit 1 diff --git a/AGENTS.md b/AGENTS.md index 108ca785..5d06873b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -253,17 +253,17 @@ Before pushing to any branch with an open PR, agents must invoke the `pre-push-r - CI status / failing checks - Linked issues' acceptance criteria -When the subagent's response ends with the parseable line `VERDICT: ship_it`, `.claude/hooks/review-marker.sh` writes `tmp/review-passed-` and the next `git push` succeeds. On `VERDICT: iterate` or `VERDICT: block`, no marker — fix the findings and re-invoke the subagent (always fresh context — never reuse the same session) until ship_it. +The subagent's verdict is one of `ship_it`, `iterate`, or `block`. **`ship_it` requires zero findings at any severity** (`[MUST]`, `[SHOULD]`, `[MAY]` sections all empty). Anything in the findings list — including `[MAY]` — forces `iterate`. The rule is: if there's anything left to do, the PR isn't shippable. "Ship it, just do this one thing first" is iteration, not shipping. + +When the subagent's response ends with the parseable line `VERDICT: ship_it`, `.claude/hooks/review-marker.sh` writes `tmp/review-passed-` and the next `git push` succeeds. On `VERDICT: iterate` or `VERDICT: block`, no marker — the orchestrator agent **loops**: address every finding, commit, re-invoke `pre-push-reviewer` in fresh context, repeat until `ship_it`. Never push with open findings. The orchestrator agent cannot override the subagent's system prompt (it's the fixed file content of `.claude/agents/pre-push-reviewer.md`), and the subagent runs in a clean conversation context, so it doesn't share the orchestrator's bias toward its own work. ### No bypass for agents -- `git push --no-verify` and `git commit --no-verify` are blocked. -- Writing `tmp/ci-passed-*` or `tmp/review-passed-*` directly (via `touch`, redirect, `cp`, `sh -c` wrapper, `Write`/`Edit` tools) is blocked. -- Markers are written exclusively by `make ci` (ci-passed) and the `pre-push-reviewer` PostToolUse hook (review-passed). Any other path is a discipline violation. - -Humans retain `--no-verify` for explicit intentional bypass — see §"Local-First Validation". +- `git push --no-verify` and `git commit --no-verify` are blocked at the `.claude/hooks/agent-bash-gate.sh` PreToolUse layer for agents. Humans retain `--no-verify` for explicit intentional bypass (see §"Local-First Validation"). +- The obvious tool-level writes to `tmp/ci-passed-*` or `tmp/review-passed-*` are denied at the `.claude/settings.json` permission layer (`Bash(touch tmp/ci-passed:*)`, `Write(tmp/ci-passed-*)`, `Edit(tmp/ci-passed-*)`, and the review-passed equivalents). +- **Markers are written exclusively by `make ci` (ci-passed) and the `pre-push-reviewer` PostToolUse hook (review-passed). You do not write a marker file by any other means — period.** Bash can write a file by a dozen paths and the deny list does not enumerate all of them; this is an honest-agent rule, not an adversarial gate. If you ever feel tempted to write a marker, stop: the marker is wrong-shaped if you're the one writing it. Run `make ci`, invoke the subagent, get the verdict — that's the path. ### Reviewing someone else's PR locally diff --git a/CHANGELOG.md b/CHANGELOG.md index e0aced6e..a3bafd2a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -41,6 +41,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **Dependabot auto-merge no longer slipped past CI on consolidated-pipeline PRs** (`main branch protection` ruleset; `.github/workflows/project-orchestrator.yml`; `.github/workflows/claude-review.yml`; `AGENTS.md`): on commit `93b3206` the previously six-job CI was collapsed into a single job named `CI`, but three places still referenced the old multi-job names — the ruleset's `required_status_checks` (`Validate` + `Admin approval` only — `CI` not listed), the `bot-clean` check in `project-orchestrator.yml` (looking for `Build`/`Validate`/`Lint`/`Test`/`Integration Tests`/`SDK Tests`), and `REQUIRED_CHECKS` in `claude-review.yml` (same six). Net effect: a Dependabot PR opened at T+0 would have its `Validate` and `Admin approval` checks satisfied within ~30s (PR-title workflow + the auto-approval workflow), `gh pr merge --auto` would fire, GitHub would see all *required* checks green, and the PR would squash-merge before `ci.yml` even started running. Pre-consolidation runs (PR #90 etc.) didn't expose this because GitHub's auto-merge waits for in-flight checks as a courtesy, and the multi-job CI was producing checks before auto-merge fired — but with one collapsed `CI` check that didn't start until later, the courtesy wait disappeared. Fix: added `CI` to the ruleset's required-checks (now `Admin approval` / `CI` / `Validate`) via the `gh api PUT /repos/Wave-RF/WaveHouse/rulesets/15353356` round-trip, and updated both bot-clean lists to look for `CI` + `Validate`. AGENTS.md note about the required-check set updated to match. Diagnosis steps recorded inline as code comments in both workflow files so future-me knows where to look if the check name changes again. ### Added +- **Team-wide Claude Code configuration + agent PR discipline gates** (`.claude/agents/pre-push-reviewer.md`, `.claude/hooks/agent-bash-gate.sh`, `.claude/hooks/review-marker.sh`, `.claude/hooks/gofumpt-on-save.sh`, `.claude/settings.json`, `.claude/skills/pr-review-locally/SKILL.md`, `.claude/skills/pr-sync-with-main/SKILL.md`, `.claude/commands/cover.md`, `.githooks/pre-commit`, `.githooks/pre-push`, `.config/wt.toml`, `AGENTS.md` §"Agent PR Discipline" + §"Local-First Validation", `docs/src/content/docs/claude-code.md`). Two distinct enforcement layers: universal git hooks (`.githooks/`, installed by `make tools` via `git config core.hooksPath .githooks`) run `make verify` on commit and require `tmp/ci-passed-` before push for everyone (humans + agents); agent-only gates layer on top in `.claude/hooks/agent-bash-gate.sh` (PreToolUse Bash) and block: `--no-verify` on git push/commit; `gh pr create` without `--draft`; `gh pr ready`, `--approve`, `--request-changes` (humans-only state transitions); `gh pr edit --add-reviewer/--add-assignee` and `gh api .../requested_reviewers` POST/PUT/PATCH (humans-only reviewer assignment — bot reviewers re-triggered via PR comments instead); and `git push` to an open-PR branch without both ci-passed and review-passed markers. The `pre-push-reviewer` subagent runs the canonical `.github/prompts/pr-review.md` in fresh context against the branch's full merge-base diff (`git diff main...HEAD`) plus latest commit, open PR comments + reviews, CI status, and linked-issue acceptance criteria, then emits a parseable `VERDICT: ship_it|iterate|block` consumed by `review-marker.sh` (PostToolUse Agent) which writes `tmp/review-passed-` only on `ship_it`. **`ship_it` is gated to zero findings at any severity** — any `[MUST]`/`[SHOULD]`/`[MAY]` forces `iterate` and the orchestrator loops review → fix → review until clean before pushing. Marker writes are NOT regex-blocked beyond the cheap settings.json deny patterns (`Bash(touch tmp/ci-passed:*)`, `Write(tmp/ci-passed-*)`, `Edit(tmp/ci-passed-*)`, and review-passed equivalents); the rest is an explicit honest-agent rule in AGENTS.md ("you do not write a marker file by any other means — period") because Bash can write a file by a dozen paths and regex enforcement is a porous game of whack-a-mole. Humans retain `--no-verify` for explicit intentional bypass; `.config/wt.toml` ships worktrunk hooks so parallel-agent worktrees install `.githooks/` correctly. `docs/src/content/docs/claude-code.md` is the contributor-facing page that explains the layering, quick-setup, and discipline rules. - **Astro / Starlight documentation site at `wavehouse.dev`** (`docs/`, `Makefile`, `.github/dependabot.yml`, `.github/workflows/ci.yml`, `.gitignore`, `.vscode/launch.json`): full content + tooling for the public docs (Getting Started, Why WaveHouse, Architecture, API Reference, TypeScript SDK, Configuration, Deployment, Development). Build-time mermaid via `rehype-mermaid` (inline-svg strategy, no client-side JS), LaTeX via `remark-math` + `rehype-katex`, image zoom via `starlight-image-zoom`, expressiveCode with github dark/light + `env`/`dns` Shiki language aliases. PostHog frontend telemetry (key is public by design — embedded in every visitor's browser). `editLink` pointed at `main`; `lastUpdated` on. Sidebar lives in `docs/src/config/sidebar.ts` as the single source of truth, consumed by both Starlight rendering and the LLM-friendly outputs below. Cloudflare Workers + Static Assets deployment via `docs/wrangler.jsonc` (no auto-deploy yet — wired up when the GH Actions workflow lands separately). Make targets stay minimal: `dev-docs` (Astro dev server on :4321), `build-docs`, `preview-docs` — the last serves the production build through `wrangler dev`, so the Cloudflare Worker's content negotiation (`Accept: text/markdown` / LLM-bot User-Agent → `.md` twin) is exercised end-to-end the same way it will be in production. - **Per-page `.md` twin + `llms.txt` family via `starlight-llm-tools`** (`docs/astro.config.mjs`, `docs/worker/index.ts`, `docs/wrangler.jsonc`): every doc is also served as raw markdown at `.md` with a navigation header (Section / Subpages / Related / HTML version pointers), plus three concatenated views — `/llms.txt` manifest, `/llms-full.txt` (every page, sidebar-ordered), `/llms-small.txt` (overview pages only). Page-title slot gains a **Copy Markdown** button and **Open with AI** dropdown (Claude / ChatGPT / Cursor). Content negotiation is handled by a Cloudflare Worker that re-exports the `cloudflare-md-router` package's default handler in one line — `Accept: text/markdown` or known LLM-bot User-Agent (GPTBot, ClaudeBot, PerplexityBot, Applebot-Extended, etc.) gets the `.md` twin transparently, with a fall-through to the HTML response when the twin doesn't exist. - **Two extracted plugin packages, MIT-licensed**: [`github.com/Wave-RF/cloudflare-md-router`](https://github.com/Wave-RF/cloudflare-md-router) (Workers handler + `createMdRouter()` factory + extensible bot-UA regex; consumed in `docs/worker/index.ts` as a one-line re-export) and [`github.com/Wave-RF/starlight-llm-tools`](https://github.com/Wave-RF/starlight-llm-tools) (Starlight plugin that auto-injects the four routes, the two components, and the `PageTitle` override; optionally calls into `starlight-glossary/transform` when present). Both pinned via `github:Wave-RF/...` in `docs/package.json`; lockfile pins the resolved commit SHAs. Pre-npm-publish state — switch to normal version specifiers when the packages land on npm. From ab2f4148c0288cc99d447cf118b029a98f4081c4 Mon Sep 17 00:00:00 2001 From: Eric Andrechek Date: Tue, 19 May 2026 12:26:22 -0400 Subject: [PATCH 07/18] fix(claude): doc-sync the honest-agent + strict-ship_it policy shifts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two doc-sync misses surfaced by pre-push-reviewer against c03edcd: - docs/src/content/docs/claude-code.md:101 (and the settings.json table row at line 51) still claimed regex enforcement of touch / redirect / sh -c wrappers around marker writes. That regex was removed in c03edcd in favor of an honest-agent rule. Rewrote the "No bypass" bullet to state what's actually enforced (--no-verify + obvious tool-level denies) and where the rest comes from (AGENTS.md honest-agent rule). Also added the new ship_it-requires-zero-findings rule to the same bullet block so the contributor-facing doc matches AGENTS.md. - .claude/agents/pre-push-reviewer.md:12 (§Source of truth) said the CI prompt applies here "verbatim — including the verdict rules", which contradicts §Verdict mapping at line 89 ("WaveHouse uses a stricter rule"). A fresh-context agent reading linearly could anchor on line 12 and emit ship_it with [MAY] findings present, defeating the c03edcd rule. Amended line 12 to scope "verbatim" to focus areas + severity tags + noise filter, and explicitly hand off the verdict rules to §Verdict mapping below. Co-Authored-By: Claude Opus 4.7 (1M context) --- .claude/agents/pre-push-reviewer.md | 2 +- docs/src/content/docs/claude-code.md | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/.claude/agents/pre-push-reviewer.md b/.claude/agents/pre-push-reviewer.md index dedd3ddd..57a7c6bc 100644 --- a/.claude/agents/pre-push-reviewer.md +++ b/.claude/agents/pre-push-reviewer.md @@ -9,7 +9,7 @@ You are reviewing the current branch's delta against main, using the same prompt ## Source of truth -Read `.github/prompts/pr-review.md` first. That file is the canonical WaveHouse review prompt and applies here verbatim — including the focus areas (correctness → security → performance → testing → docs/sdk-sync), the severity tags `[MUST]`/`[SHOULD]`/`[MAY]`, the noise filter, and the verdict rules. +Read `.github/prompts/pr-review.md` first. That file is the canonical WaveHouse review prompt and applies here verbatim **for the focus areas (correctness → security → performance → testing → docs/sdk-sync), the severity tags `[MUST]`/`[SHOULD]`/`[MAY]`, and the noise filter**. The verdict rules below override the CI prompt's — WaveHouse pre-push runs a stricter rubric (any finding forces iterate; see §Verdict mapping below). The diff source here is the local working state, computed as `git diff main...HEAD` (three dots — equivalent to `git diff $(git merge-base main HEAD) HEAD`, i.e. merge-base vs HEAD). Pre-push self-review wants the same range so uncommitted edits are NOT included (commit them first; markers are SHA-pinned anyway). diff --git a/docs/src/content/docs/claude-code.md b/docs/src/content/docs/claude-code.md index 5d2e288a..0ed1a01a 100644 --- a/docs/src/content/docs/claude-code.md +++ b/docs/src/content/docs/claude-code.md @@ -48,7 +48,7 @@ The marker invalidates on every commit (HEAD SHA changes), so `make ci` re-runs | Path | Purpose | | ---- | ------- | -| `.claude/settings.json` | Team-wide: `deny` permissions (force-push, gh pr merge / ready / approve, --no-verify, marker forgery, secrets), `worktree.baseRef: "fresh"` + symlinkDirectories, all four hooks wired | +| `.claude/settings.json` | Team-wide: `deny` permissions (force-push, gh pr merge / ready / approve, --no-verify, the obvious marker-write idioms, secrets), `worktree.baseRef: "fresh"` + symlinkDirectories, all four hooks wired | | `.claude/hooks/gofumpt-on-save.sh` | PostToolUse Edit/Write/MultiEdit: auto-formats `.go` files | | `.claude/hooks/agent-bash-gate.sh` | PreToolUse Bash: enforces Agent PR Discipline (drafts only, no human reviewer adds, no `--no-verify`, marker required on PR pushes) | | `.claude/hooks/review-marker.sh` | PostToolUse Agent: writes `tmp/review-passed-` when `pre-push-reviewer` returns `VERDICT: ship_it` | @@ -97,8 +97,8 @@ Agents (Claude Code etc.) have additional gating beyond what humans face — enf - **Drafts only.** `gh pr create` must include `--draft`. Only humans transition draft → ready (`gh pr ready` is blocked), approve (`gh pr review --approve` is blocked), or request changes (`gh pr review --request-changes` is blocked). - **No human reviewer assignment.** `gh pr edit --add-reviewer / --add-assignee` and `POST /requested_reviewers` are blocked. The `housekeeping.yml` workflow auto-assigns; humans handle the rest. - **Bot re-triggers via comments.** Agents CAN mention bots in PR comments to re-trigger reviews — `@coderabbitai review`, `@gemini-code-assist /gemini review`, `@claude` or `/review`, etc. This goes through `gh pr comment` (allowed), not the reviewer API. -- **Pre-push review required on PR branches.** Before `git push` to a branch with an open PR, the agent must invoke `pre-push-reviewer` (fresh context). On `VERDICT: ship_it`, the marker auto-writes and push proceeds. On iterate/block, fix and re-invoke (always fresh context) until clean. -- **No bypass.** `--no-verify`, direct marker writes, and creative wrappers (`sh -c "touch ..."`, redirects, etc.) are all blocked. Markers come from `make ci` and the review-marker hook — nowhere else. +- **Pre-push review required on PR branches.** Before `git push` to a branch with an open PR, the agent must invoke `pre-push-reviewer` (fresh context). `ship_it` requires zero findings at any severity — any `[MUST]` / `[SHOULD]` / `[MAY]` forces iterate. On iterate, fix the findings and re-invoke (always fresh context) — loop until clean. +- **No bypass.** `--no-verify` is blocked on `git push` / `git commit` for agents. The obvious marker-write idioms (`Bash(touch tmp/ci-passed:*)`, `Write`/`Edit` on `tmp/ci-passed-*` and `tmp/review-passed-*`) are denied at the permission layer. Everything else relies on the honest-agent rule in AGENTS.md §"Agent PR Discipline": markers come from `make ci` and the `review-marker.sh` hook — nowhere else, by any means. Bash can write a file by a dozen paths and regex enforcement is a porous game of whack-a-mole; the rule is documented, not regex-enforced. - **PR reviews on others' PRs stay local by default.** Use `pr-review-locally` skill for local-only audits. To make the bot comment on the PR remotely, fire the CI workflow: `gh workflow run "Claude PR review" -f pr_number=` — that's the canonical bot-comment path. Full ruleset and rationale: AGENTS.md §"Agent PR Discipline". From 849ffce6d1f927a72f8458dd54c9ae04b47e87a2 Mon Sep 17 00:00:00 2001 From: Eric Andrechek Date: Tue, 19 May 2026 12:30:45 -0400 Subject: [PATCH 08/18] fix(claude): drop residual marker-write overclaim in claude-code.md:43 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The contributor-facing "Humans can bypass / Agents cannot" callout in the Git hooks section still claimed "--no-verify and direct marker writes are blocked" without the honest-agent qualifier ab2f414 added to line 101. Tightened to match: --no-verify blocked, obvious marker-write idioms denied at the permission layer, the rest is an honest-agent rule (with cross-reference to §Agent PR Discipline for the full text). Same correction pattern as line 101; line 43 was missed in the first sweep and surfaced by the next pre-push-reviewer iteration. Co-Authored-By: Claude Opus 4.7 (1M context) --- docs/src/content/docs/claude-code.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/src/content/docs/claude-code.md b/docs/src/content/docs/claude-code.md index 0ed1a01a..52b65a8f 100644 --- a/docs/src/content/docs/claude-code.md +++ b/docs/src/content/docs/claude-code.md @@ -40,7 +40,7 @@ Two scripts, both committed to the repo: | `pre-commit` | Runs `make verify` (tidy + fmt + vulncheck + lint, ~30s) — **blocks on failure**. Then emits informational stderr nudges for likely doc-sync and SDK-sync misses (see AGENTS.md §Documentation Sync and §SDK Sync). Nudges don't block. | | `pre-push` | Checks for `tmp/ci-passed-` marker. The `make ci` target writes this marker on success. **Blocks** if absent — Claude (or you) runs `make ci`, sees output, fixes failures, retries push. | -**Humans** can bypass with `git commit --no-verify` / `git push --no-verify` for intentional WIP / draft pushes. **Agents cannot** — `--no-verify` and direct marker writes are blocked (see [Agent PR Discipline](#agent-pr-discipline)). +**Humans** can bypass with `git commit --no-verify` / `git push --no-verify` for intentional WIP / draft pushes. **Agents cannot** — `--no-verify` is blocked, the obvious marker-write idioms are denied at the permission layer, and the rest is an honest-agent rule (see [Agent PR Discipline](#agent-pr-discipline)). The marker invalidates on every commit (HEAD SHA changes), so `make ci` re-runs after each new commit before pushing. That's the AGENTS.md rule made literal. From f3018ed73feb8d63f9c3b9f43d39bfeaf7c86a31 Mon Sep 17 00:00:00 2001 From: Eric Andrechek Date: Tue, 19 May 2026 12:35:34 -0400 Subject: [PATCH 09/18] fix(claude-code docs): three hooks wired, not four MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit claude-code.md:51 in the .claude/settings.json row said "all four hooks wired" but only three are configured (PreToolUse Bash → agent-bash-gate; PostToolUse Edit|Write|MultiEdit → gofumpt-on-save; PostToolUse Agent → review-marker). Off-by-one count overclaim, same shape as the marker- write overclaims fixed in earlier iterations. Co-Authored-By: Claude Opus 4.7 (1M context) --- docs/src/content/docs/claude-code.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/src/content/docs/claude-code.md b/docs/src/content/docs/claude-code.md index 52b65a8f..c6be6b21 100644 --- a/docs/src/content/docs/claude-code.md +++ b/docs/src/content/docs/claude-code.md @@ -48,7 +48,7 @@ The marker invalidates on every commit (HEAD SHA changes), so `make ci` re-runs | Path | Purpose | | ---- | ------- | -| `.claude/settings.json` | Team-wide: `deny` permissions (force-push, gh pr merge / ready / approve, --no-verify, the obvious marker-write idioms, secrets), `worktree.baseRef: "fresh"` + symlinkDirectories, all four hooks wired | +| `.claude/settings.json` | Team-wide: `deny` permissions (force-push, gh pr merge / ready / approve, --no-verify, the obvious marker-write idioms, secrets), `worktree.baseRef: "fresh"` + symlinkDirectories, all three hooks wired | | `.claude/hooks/gofumpt-on-save.sh` | PostToolUse Edit/Write/MultiEdit: auto-formats `.go` files | | `.claude/hooks/agent-bash-gate.sh` | PreToolUse Bash: enforces Agent PR Discipline (drafts only, no human reviewer adds, no `--no-verify`, marker required on PR pushes) | | `.claude/hooks/review-marker.sh` | PostToolUse Agent: writes `tmp/review-passed-` when `pre-push-reviewer` returns `VERDICT: ship_it` | From 8fbd7db46d365a222aa99a1cefd52ab93d73d68a Mon Sep 17 00:00:00 2001 From: Eric Andrechek Date: Tue, 19 May 2026 12:44:17 -0400 Subject: [PATCH 10/18] fix(claude): strip quotes before gate matching + correct enforcement table Two issues surfaced by the fourth strict-ship_it iteration: 1. **agent-bash-gate.sh false-positives on quoted mentions of blocked patterns.** Commit 6c79315 fixed this specifically for --no-verify by adding a [^"'\$<]* quote-traversal segment to that one regex, but the same fix wasn't applied to the other six checks (gh pr create / ready / edit, gh api requested_reviewers, gh pr review, the push-marker gate). Demonstrated live: `echo "this mentions git push in quotes"` tripped the marker check. Generalized the fix: strip single- and double-quoted segments from the command once at the top into $stripped, and use $stripped for all subsequent regex checks. Simplified check #1's regex accordingly (the in-regex quote traversal is no longer needed). Sanity-tested: false positives on `echo`, `gh pr comment -b "..."`, `git commit -m "..."` all pass through; real `git push --no-verify` / `git commit --no-verify` still block. 2. **claude-code.md "How enforcement is layered" table mislabels the Claude Code hooks layer.** The two-row "two distinct gate layers" framing put `gofumpt-on-save.sh` in a row labeled "UX: auto-format" and omitted `agent-bash-gate.sh` (which is enforcement) and `review-marker.sh` (which is the marker writer). Restructured to four rows: git hooks (universal enforcement), Claude Code agent gate (agent-only enforcement), Claude Code ergonomic hooks (formatter + marker writer), and Claude Code skills/agents/commands (workflow guidance). Matches the more detailed sections later in the doc and the actual settings.json wiring. Co-Authored-By: Claude Opus 4.7 (1M context) --- .claude/hooks/agent-bash-gate.sh | 47 ++++++++++++++++------------ docs/src/content/docs/claude-code.md | 7 ++--- 2 files changed, 30 insertions(+), 24 deletions(-) diff --git a/.claude/hooks/agent-bash-gate.sh b/.claude/hooks/agent-bash-gate.sh index 498196e8..ce800964 100755 --- a/.claude/hooks/agent-bash-gate.sh +++ b/.claude/hooks/agent-bash-gate.sh @@ -32,6 +32,15 @@ cmd=$(printf '%s' "$input" | jq -r '.tool_input.command // empty' 2>/dev/null) cd "${CLAUDE_PROJECT_DIR:-.}" 2>/dev/null || exit 0 +# Strip single- and double-quoted segments before matching, so legitimate +# commands that *mention* a blocked pattern inside a string (e.g. +# `gh pr comment -b "we will git push after CI"`, `echo "use --no-verify"`) +# don't false-positive. Same intent as commit 6c79315 (which fixed the +# no-verify regex specifically), generalized to every check below. Doesn't +# cover escaped quotes or heredocs — accept the corner case; agents +# constructing such commands are doing something weird. +stripped=$(printf '%s' "$cmd" | sed -E "s/'[^']*'//g; s/\"[^\"]*\"//g") + # --- Helper: block with a structured reason --------------------------------- block() { local reason="$1" @@ -46,38 +55,36 @@ EOF } # Boundary helper: detects a `git ` invocation anywhere in the -# command (including after && / ; / | / cd ... &&). Used by multiple checks. +# (quote-stripped) command (including after && / ; / | / cd ... &&). Used +# by multiple checks. git_subcmd() { - printf '%s\n' "$cmd" | grep -qE "(^|[[:space:];|&]+)git[[:space:]]+$1\b" + printf '%s\n' "$stripped" | grep -qE "(^|[[:space:];|&]+)git[[:space:]]+$1\b" } # --- 1. --no-verify on git push/commit -------------------------------------- -# Match --no-verify only when it appears in the args of `git push|commit` -# BEFORE any quote (" or '), command substitution ($), or heredoc marker (<). -# That excludes false positives where the literal string sits inside a quoted -# commit message body (e.g., when documenting the rule). Honest-agent defense, -# not adversarial — eval / sh -c wrappers around `git push --no-verify` could +# Quote-stripping above already excludes false positives where the literal +# string sits inside a quoted commit-message body. Honest-agent defense, not +# adversarial — eval / sh -c wrappers around `git push --no-verify` could # still bypass; AGENTS.md §"Agent PR Discipline" makes the rule explicit. -no_verify_re='(^|[[:space:];|&])git[[:space:]]+(push|commit)\b[^"'\''$<]*[[:space:]]--no-verify\b' -if printf '%s\n' "$cmd" | grep -qE "$no_verify_re"; then +if printf '%s\n' "$stripped" | grep -qE '(^|[[:space:];|&])git[[:space:]]+(push|commit)\b[[:space:]][^&|;]*--no-verify\b'; then block "git push/commit with --no-verify is not permitted for agents. Run the gates." fi # --- 2. gh pr create without --draft ---------------------------------------- -if printf '%s\n' "$cmd" | grep -qE '(^|[[:space:];|&]+)gh[[:space:]]+pr[[:space:]]+create\b'; then - if ! printf '%s\n' "$cmd" | grep -qE '(^|[[:space:]])(--draft|-d)\b'; then +if printf '%s\n' "$stripped" | grep -qE '(^|[[:space:];|&]+)gh[[:space:]]+pr[[:space:]]+create\b'; then + if ! printf '%s\n' "$stripped" | grep -qE '(^|[[:space:]])(--draft|-d)\b'; then block "Agent-opened PRs must be created with --draft. Only humans publish ready-for-review PRs." fi fi # --- 3. gh pr ready --------------------------------------------------------- -if printf '%s\n' "$cmd" | grep -qE '(^|[[:space:];|&]+)gh[[:space:]]+pr[[:space:]]+ready\b'; then +if printf '%s\n' "$stripped" | grep -qE '(^|[[:space:];|&]+)gh[[:space:]]+pr[[:space:]]+ready\b'; then block "Only humans transition PRs from draft to ready-for-review. Ask the user to do this manually when the PR is ready." fi # --- 4. gh pr edit with reviewer/assignee changes --------------------------- -if printf '%s\n' "$cmd" | grep -qE '(^|[[:space:];|&]+)gh[[:space:]]+pr[[:space:]]+edit\b'; then - if printf '%s\n' "$cmd" | grep -qE '(^|[[:space:]])--(add|remove)-(reviewer|assignee)\b'; then +if printf '%s\n' "$stripped" | grep -qE '(^|[[:space:];|&]+)gh[[:space:]]+pr[[:space:]]+edit\b'; then + if printf '%s\n' "$stripped" | grep -qE '(^|[[:space:]])--(add|remove)-(reviewer|assignee)\b'; then block "Adding/removing reviewers or assignees is humans-only. To re-trigger bot reviewers, post a PR comment mentioning them (@coderabbitai review, @gemini-code-assist /gemini review, @claude / /review)." fi fi @@ -86,19 +93,19 @@ fi # Both POST (add) and PUT (replace) on /pulls//requested_reviewers are # reviewer-write operations. Neither has a legitimate agent use case — bot # reviewers are re-triggered via PR comments. Match any reviewer-write idiom. -if printf '%s\n' "$cmd" | grep -qE '(^|[[:space:];|&]+)gh[[:space:]]+api\b' && \ - printf '%s\n' "$cmd" | grep -qE 'requested_reviewers'; then - if printf '%s\n' "$cmd" | grep -qE '(-X[[:space:]]*(POST|PUT|PATCH)|--method[[:space:]]*(POST|PUT|PATCH)|[[:space:]]-f[[:space:]]+reviewers=|[[:space:]]-F[[:space:]]+reviewers=)'; then +if printf '%s\n' "$stripped" | grep -qE '(^|[[:space:];|&]+)gh[[:space:]]+api\b' && \ + printf '%s\n' "$stripped" | grep -qE 'requested_reviewers'; then + if printf '%s\n' "$stripped" | grep -qE '(-X[[:space:]]*(POST|PUT|PATCH)|--method[[:space:]]*(POST|PUT|PATCH)|[[:space:]]-f[[:space:]]+reviewers=|[[:space:]]-F[[:space:]]+reviewers=)'; then block "Write requests to /requested_reviewers are the API form of --add-reviewer; humans-only. For bot reviewers, post a PR comment mentioning them." fi fi # --- 6. gh pr review --approve / --request-changes -------------------------- -if printf '%s\n' "$cmd" | grep -qE '(^|[[:space:];|&]+)gh[[:space:]]+pr[[:space:]]+review\b'; then - if printf '%s\n' "$cmd" | grep -qE '(^|[[:space:]])(--approve|-a)\b'; then +if printf '%s\n' "$stripped" | grep -qE '(^|[[:space:];|&]+)gh[[:space:]]+pr[[:space:]]+review\b'; then + if printf '%s\n' "$stripped" | grep -qE '(^|[[:space:]])(--approve|-a)\b'; then block "Only humans approve PRs (--approve)." fi - if printf '%s\n' "$cmd" | grep -qE '(^|[[:space:]])(--request-changes|-r)\b'; then + if printf '%s\n' "$stripped" | grep -qE '(^|[[:space:]])(--request-changes|-r)\b'; then block "Agents don't use --request-changes. Post inline review comments via the GitHub inline-comment MCP tool, or use gh pr comment for top-level comments." fi fi diff --git a/docs/src/content/docs/claude-code.md b/docs/src/content/docs/claude-code.md index c6be6b21..e6517e5d 100644 --- a/docs/src/content/docs/claude-code.md +++ b/docs/src/content/docs/claude-code.md @@ -21,15 +21,14 @@ If you're new to Claude Code itself, the [official docs](https://code.claude.com ## How enforcement is layered -The team has two distinct gate layers, plus optional Claude-Code-only ergonomics: - | Layer | Lives in | Applies to | Purpose | | ----- | -------- | ---------- | ------- | | **Git hooks** | `.githooks/` (installed by `make tools`) | Humans + Claude uniformly | Hard enforcement: `make verify` on commit, `make ci` passed before push | -| **Claude Code hook** | `.claude/hooks/gofumpt-on-save.sh` (wired in `.claude/settings.json`) | Claude only | UX: auto-format on file edits (humans get this from their IDE) | +| **Claude Code agent gate** | `.claude/hooks/agent-bash-gate.sh` (PreToolUse Bash) + `.claude/settings.json` deny rules | Agents only | Enforces [Agent PR Discipline](#agent-pr-discipline): drafts only, no human reviewer adds, no `--no-verify`, marker required on PR pushes | +| **Claude Code ergonomic hooks** | `.claude/hooks/gofumpt-on-save.sh` (PostToolUse Edit/Write/MultiEdit), `.claude/hooks/review-marker.sh` (PostToolUse Agent) | Claude only | gofumpt: auto-format on file edits (humans get this from their IDE). review-marker: writes `tmp/review-passed-` on `VERDICT: ship_it` | | **Claude Code skills / agents / commands** | `.claude/skills/`, `.claude/agents/`, `.claude/commands/` | Claude only (when relevant) | Workflow guidance and on-demand helpers — not gates | -Git hooks are the source of truth for "must pass before merge." Claude Code config layers on agentic affordances; it doesn't substitute for the gates. +Git hooks are the source of truth for "must pass before merge." `.claude/` layers agent-specific gates and ergonomic hooks on top; it doesn't substitute for the universal gates. ## Git hooks (`.githooks/`) From f5e03bb64354aa400508daa4cff6b3aae3a6b4b6 Mon Sep 17 00:00:00 2001 From: Eric Andrechek Date: Tue, 19 May 2026 13:33:52 -0400 Subject: [PATCH 11/18] fix(claude): address coderabbit findings on PR #147 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - agent-bash-gate.sh: fail closed on missing jq / malformed JSON (silent exit 0 would have disabled every discipline check below) - agent-bash-gate.sh: skip marker check for `git push --help` / `-h` via new git_subcmd_is_help helper — matches the in-comment doc - review-marker.sh: anchor VERDICT parse to ^...$ so quoted mentions in prose can't accidentally produce ship_it - settings.json: add bare-relative Read deny patterns (.env, .env.*, secrets/**) alongside existing ./-prefixed ones — Claude Code matches each path format separately - .githooks/pre-push: read refspec lines from stdin and validate the marker for the actual pushed SHA(s), not just HEAD - markdownlint: tag fenced code blocks in pre-push-reviewer.md and pr-review-locally/SKILL.md (MD040) - cover.md: list `merge` in argument-hint to match documented modes Co-Authored-By: Claude Opus 4.7 (1M context) --- .claude/agents/pre-push-reviewer.md | 4 +- .claude/commands/cover.md | 2 +- .claude/hooks/agent-bash-gate.sh | 49 ++++++++++++++++------- .claude/hooks/review-marker.sh | 12 +++--- .claude/settings.json | 5 ++- .claude/skills/pr-review-locally/SKILL.md | 2 +- .githooks/pre-push | 21 ++++++++-- 7 files changed, 66 insertions(+), 29 deletions(-) diff --git a/.claude/agents/pre-push-reviewer.md b/.claude/agents/pre-push-reviewer.md index 57a7c6bc..2b17b45e 100644 --- a/.claude/agents/pre-push-reviewer.md +++ b/.claude/agents/pre-push-reviewer.md @@ -49,7 +49,7 @@ The diff source here is the local working state, computed as `git diff main...HE 8. End with a verdict per the styleguide (`Ship it` / `Iterate` / `Block`), **followed immediately by the parseable verdict line** on its own line: - ``` + ```text VERDICT: ship_it ``` @@ -57,7 +57,7 @@ The diff source here is the local working state, computed as `git diff main...HE ## Output format -``` +```markdown ## Pre-push review — vs main (Optional: brief paragraph on PR scope + linked issues if applicable.) diff --git a/.claude/commands/cover.md b/.claude/commands/cover.md index 5ec63b2a..b49119f4 100644 --- a/.claude/commands/cover.md +++ b/.claude/commands/cover.md @@ -1,6 +1,6 @@ --- description: Render coverage HTML for a suite and surface drops below threshold -argument-hint: [unit|integration|e2e|sdk|all] (default: merge whatever exists) +argument-hint: [unit|integration|e2e|sdk|merge|all] (default: merge whatever exists) --- Generate the coverage report and surface anything below threshold from `.testcoverage.yml`. diff --git a/.claude/hooks/agent-bash-gate.sh b/.claude/hooks/agent-bash-gate.sh index ce800964..f3c07114 100755 --- a/.claude/hooks/agent-bash-gate.sh +++ b/.claude/hooks/agent-bash-gate.sh @@ -26,8 +26,32 @@ set -uo pipefail +# --- Helper: block with a structured reason --------------------------------- +# Declared early because the parse step below may need it. +block() { + local reason="$1" + cat >&2 </dev/null 2>&1; then + block "jq is required for the PR discipline gate but is not installed. Install jq (brew install jq) or remove the PreToolUse hook from .claude/settings.json." +fi + input=$(cat) -cmd=$(printf '%s' "$input" | jq -r '.tool_input.command // empty' 2>/dev/null) +if ! cmd=$(printf '%s' "$input" | jq -r '.tool_input.command // ""' 2>/dev/null); then + block "Could not parse hook payload as JSON; refusing to fail open." +fi [ -z "$cmd" ] && exit 0 cd "${CLAUDE_PROJECT_DIR:-.}" 2>/dev/null || exit 0 @@ -41,19 +65,6 @@ cd "${CLAUDE_PROJECT_DIR:-.}" 2>/dev/null || exit 0 # constructing such commands are doing something weird. stripped=$(printf '%s' "$cmd" | sed -E "s/'[^']*'//g; s/\"[^\"]*\"//g") -# --- Helper: block with a structured reason --------------------------------- -block() { - local reason="$1" - cat >&2 <` invocation anywhere in the # (quote-stripped) command (including after && / ; / | / cd ... &&). Used # by multiple checks. @@ -61,6 +72,14 @@ git_subcmd() { printf '%s\n' "$stripped" | grep -qE "(^|[[:space:];|&]+)git[[:space:]]+$1\b" } +# True if `git ` is followed by -h / --help anywhere before a +# separator (so `git push --help`, `git push origin main --help`, `git push -h` +# all return true). Help invocations don't actually run the subcommand, so +# discipline gates should skip them. +git_subcmd_is_help() { + printf '%s\n' "$stripped" | grep -qE "(^|[[:space:];|&]+)git[[:space:]]+$1([[:space:]]+[^;|&]*)?[[:space:]]+(-h|--help)([[:space:]]|\$|[;|&])" +} + # --- 1. --no-verify on git push/commit -------------------------------------- # Quote-stripping above already excludes false positives where the literal # string sits inside a quoted commit-message body. Honest-agent defense, not @@ -112,7 +131,7 @@ fi # --- 7. git push: check markers --------------------------------------------- # Only on actual `git push` invocations (not `git push --help`, not `gh pr push`). -if git_subcmd 'push'; then +if git_subcmd 'push' && ! git_subcmd_is_help 'push'; then head_sha=$(git rev-parse HEAD 2>/dev/null || echo "") if [ -n "$head_sha" ]; then short_sha="${head_sha:0:8}" diff --git a/.claude/hooks/review-marker.sh b/.claude/hooks/review-marker.sh index 1dd4777a..3f773c38 100755 --- a/.claude/hooks/review-marker.sh +++ b/.claude/hooks/review-marker.sh @@ -26,13 +26,15 @@ response=$(printf '%s' "$input" | jq -r '.tool_response // empty' 2>/dev/null) # Parse the parseable verdict line. Format (per .claude/agents/pre-push-reviewer.md): # VERDICT: ship_it | VERDICT: iterate | VERDICT: block -# We take the LAST such line in the response (in case the agent quoted the -# enum earlier in its body). Case-insensitive on the value. +# Anchored to line start so an inline mention like "do not write VERDICT: ship_it" +# inside prose won't accidentally produce ship_it. We take the LAST matching +# line in the response (in case the agent emits the parseable line more than +# once for any reason). Case-insensitive on the keyword and value. verdict=$(printf '%s\n' "$response" \ - | grep -oiE 'VERDICT:[[:space:]]*(ship_it|iterate|block)\b' \ + | grep -iE '^[[:space:]]*VERDICT:[[:space:]]*(ship_it|iterate|block)[[:space:]]*$' \ | tail -1 \ - | sed -E 's/.*:[[:space:]]*([a-zA-Z_]+).*/\1/' \ - | tr '[:upper:]' '[:lower:]') + | tr '[:upper:]' '[:lower:]' \ + | sed -E 's/^[[:space:]]*verdict:[[:space:]]*([a-z_]+)[[:space:]]*$/\1/') [ "$verdict" = "ship_it" ] || exit 0 diff --git a/.claude/settings.json b/.claude/settings.json index 2b982569..c04c5d8b 100644 --- a/.claude/settings.json +++ b/.claude/settings.json @@ -39,7 +39,10 @@ "Read(./.env)", "Read(./.env.*)", - "Read(./secrets/**)" + "Read(./secrets/**)", + "Read(.env)", + "Read(.env.*)", + "Read(secrets/**)" ] }, diff --git a/.claude/skills/pr-review-locally/SKILL.md b/.claude/skills/pr-review-locally/SKILL.md index 72014def..72974bd3 100644 --- a/.claude/skills/pr-review-locally/SKILL.md +++ b/.claude/skills/pr-review-locally/SKILL.md @@ -34,7 +34,7 @@ gh pr view --json number,state,headRefName --jq . # confirm we're on PR 120's Use the `Agent` tool: -``` +```js Agent({ subagent_type: "pre-push-reviewer", description: "Review PR locally", diff --git a/.githooks/pre-push b/.githooks/pre-push index dee6ae70..3911de4a 100755 --- a/.githooks/pre-push +++ b/.githooks/pre-push @@ -11,13 +11,26 @@ set -uo pipefail cd "$(git rev-parse --show-toplevel)" || exit 1 -head_sha=$(git rev-parse HEAD 2>/dev/null) -marker="tmp/ci-passed-${head_sha}" - -if [ -f "$marker" ]; then +# Git invokes pre-push with refspec lines on stdin: ` `. +# Validate the marker for each commit actually being pushed (which may not be HEAD +# if the user is pushing a non-current branch like `git push origin other-branch`). +# Deletion pushes (local_sha = 40 zeros) and missing-SHA lines are skipped. +missing_sha="" +while read -r local_ref local_sha remote_ref remote_sha; do + [ -z "${local_sha:-}" ] && continue + [ "$local_sha" = "0000000000000000000000000000000000000000" ] && continue + if [ ! -f "tmp/ci-passed-${local_sha}" ]; then + missing_sha="$local_sha" + break + fi +done + +# All pushed refs are gated, or stdin was empty (some test invocations). +if [ -z "$missing_sha" ]; then exit 0 fi +head_sha="$missing_sha" short_sha=${head_sha:0:8} cat >&2 < Date: Tue, 19 May 2026 14:02:04 -0400 Subject: [PATCH 12/18] fix(claude): move review-marker to SubagentStop + 2 coderabbit findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The PR #147 review surfaced a real plumbing bug: the marker hook on PostToolUse:Agent never wrote its file because `.tool_response` for the Agent tool is a structured object (`{ content: [{type, text}], ... }`), not a flat string. Instrumented both events live; confirmed that SubagentStop exposes a clean `.last_assistant_message` string and an `agent_type` field — moved the hook there and used those fields. Also: - agent-bash-gate.sh: distinguish "no PR for this branch" (benign, skip review-marker check) from "gh auth/network error" (NOT benign — block rather than silently bypass the review gate). Captures gh stderr and regex-matches "no pull request(s) found"; any other failure mode blocks with a clear remediation pointer. Also blocks if gh is missing. - settings.json: deny `MultiEdit(tmp/{ci,review}-passed-*)` alongside existing Write/Edit denies (MultiEdit is evaluated as a separate tool by Claude Code). - claude-code.md: doc-sync the hook event change. Tested live: triggered an Agent (Explore) to confirm PostToolUse:Agent and SubagentStop both fire (hot-reload of settings.json works), dumped both payloads, and verified the new hook script writes a marker only on agent_type=="pre-push-reviewer" with a `VERDICT: ship_it` last message. Co-Authored-By: Claude Opus 4.7 (1M context) --- .claude/hooks/agent-bash-gate.sh | 17 ++++++++++++- .claude/hooks/review-marker.sh | 38 ++++++++++++++++++---------- .claude/settings.json | 7 +++-- docs/src/content/docs/claude-code.md | 4 +-- 4 files changed, 47 insertions(+), 19 deletions(-) diff --git a/.claude/hooks/agent-bash-gate.sh b/.claude/hooks/agent-bash-gate.sh index f3c07114..94cf2d0a 100755 --- a/.claude/hooks/agent-bash-gate.sh +++ b/.claude/hooks/agent-bash-gate.sh @@ -159,7 +159,22 @@ EOF # 7b. review-passed required when HEAD branch has an open PR. branch=$(git symbolic-ref --short HEAD 2>/dev/null || echo "") if [ -n "$branch" ] && [ "$branch" != "main" ]; then - pr_state=$(gh pr view "$branch" --json state --jq .state 2>/dev/null || echo "") + if ! command -v gh >/dev/null 2>&1; then + block "gh CLI is required to determine PR state for branch '${branch}' (needed to enforce pre-push review on PR branches). Install gh or push from main." + fi + # gh pr view exits 1 for both "no PR for this branch" (benign — no review-marker + # enforcement needed) and "auth/network error" (NOT benign — would silently bypass + # the review gate). Differentiate by capturing stderr and grepping for the + # well-known "no pull requests found" message; anything else is treated as a + # real failure and blocks. + pr_state="" + if pr_view_out=$(gh pr view "$branch" --json state --jq .state 2>&1); then + pr_state="$pr_view_out" + elif printf '%s' "$pr_view_out" | grep -qiE 'no (open )?pull request'; then + pr_state="" + else + block "Could not determine PR state for branch '${branch}': ${pr_view_out}. Refusing to silently skip review-marker enforcement. Run 'gh auth status', 'gh auth login', or retry." + fi if [ "$pr_state" = "OPEN" ]; then if [ ! -f "$review_marker" ]; then cat >&2 <. The -# pre-push gate (in .claude/hooks/agent-bash-gate.sh) reads that marker to allow -# the subsequent `git push`. +# When the pre-push-reviewer subagent finishes its review and its last assistant +# message ends with `VERDICT: ship_it`, this hook writes +# tmp/review-passed-. The pre-push gate (in +# .claude/hooks/agent-bash-gate.sh) reads that marker to allow the subsequent +# `git push`. # -# Why this hook exists: the orchestrator agent is denied direct writes to -# tmp/(ci|review)-passed-* (permission deny list + agent-bash-gate). Hooks run at -# Claude Code privilege level, NOT subject to the permission system, so this -# is the only path to creating the marker. The subagent's verdict is the gate; -# the orchestrator can't fake it because the subagent runs in fresh context -# with the canonical system prompt from .claude/agents/pre-push-reviewer.md. +# Why SubagentStop (not PostToolUse:Agent): the PostToolUse:Agent payload puts +# the subagent's final text in `.tool_response.content[].text` (array of +# content blocks), which is brittle to parse and to schema changes. +# SubagentStop exposes `.last_assistant_message` as a flat string, which is +# both stable and what we actually need. Both events do fire on subagent +# completion; we just use the one with the friendlier schema. +# +# Why this hook exists at all: the orchestrator agent is denied direct writes +# to tmp/(ci|review)-passed-* (permission deny list + agent-bash-gate). Hooks +# run at Claude Code privilege level, NOT subject to the permission system, so +# this is the only path to creating the marker. The subagent's verdict is the +# gate; the orchestrator can't fake it because the subagent runs in fresh +# context with the canonical system prompt from +# .claude/agents/pre-push-reviewer.md. set -uo pipefail input=$(cat) -agent_type=$(printf '%s' "$input" | jq -r '.tool_input.subagent_type // empty' 2>/dev/null) -# Only act on pre-push-reviewer completions +# SubagentStop fires for every subagent completion (no matcher support per +# Claude Code docs), so we filter by `agent_type` in-script. +agent_type=$(printf '%s' "$input" | jq -r '.agent_type // empty' 2>/dev/null) [ "$agent_type" = "pre-push-reviewer" ] || exit 0 -response=$(printf '%s' "$input" | jq -r '.tool_response // empty' 2>/dev/null) +response=$(printf '%s' "$input" | jq -r '.last_assistant_message // empty' 2>/dev/null) [ -z "$response" ] && exit 0 # Parse the parseable verdict line. Format (per .claude/agents/pre-push-reviewer.md): diff --git a/.claude/settings.json b/.claude/settings.json index c04c5d8b..5ee0792d 100644 --- a/.claude/settings.json +++ b/.claude/settings.json @@ -36,6 +36,8 @@ "Write(tmp/review-passed-*)", "Edit(tmp/ci-passed-*)", "Edit(tmp/review-passed-*)", + "MultiEdit(tmp/ci-passed-*)", + "MultiEdit(tmp/review-passed-*)", "Read(./.env)", "Read(./.env.*)", @@ -80,9 +82,10 @@ "command": "$CLAUDE_PROJECT_DIR/.claude/hooks/gofumpt-on-save.sh" } ] - }, + } + ], + "SubagentStop": [ { - "matcher": "Agent", "hooks": [ { "type": "command", diff --git a/docs/src/content/docs/claude-code.md b/docs/src/content/docs/claude-code.md index e6517e5d..8c4492d7 100644 --- a/docs/src/content/docs/claude-code.md +++ b/docs/src/content/docs/claude-code.md @@ -25,7 +25,7 @@ If you're new to Claude Code itself, the [official docs](https://code.claude.com | ----- | -------- | ---------- | ------- | | **Git hooks** | `.githooks/` (installed by `make tools`) | Humans + Claude uniformly | Hard enforcement: `make verify` on commit, `make ci` passed before push | | **Claude Code agent gate** | `.claude/hooks/agent-bash-gate.sh` (PreToolUse Bash) + `.claude/settings.json` deny rules | Agents only | Enforces [Agent PR Discipline](#agent-pr-discipline): drafts only, no human reviewer adds, no `--no-verify`, marker required on PR pushes | -| **Claude Code ergonomic hooks** | `.claude/hooks/gofumpt-on-save.sh` (PostToolUse Edit/Write/MultiEdit), `.claude/hooks/review-marker.sh` (PostToolUse Agent) | Claude only | gofumpt: auto-format on file edits (humans get this from their IDE). review-marker: writes `tmp/review-passed-` on `VERDICT: ship_it` | +| **Claude Code ergonomic hooks** | `.claude/hooks/gofumpt-on-save.sh` (PostToolUse Edit/Write/MultiEdit), `.claude/hooks/review-marker.sh` (SubagentStop) | Claude only | gofumpt: auto-format on file edits (humans get this from their IDE). review-marker: writes `tmp/review-passed-` on `VERDICT: ship_it` | | **Claude Code skills / agents / commands** | `.claude/skills/`, `.claude/agents/`, `.claude/commands/` | Claude only (when relevant) | Workflow guidance and on-demand helpers — not gates | Git hooks are the source of truth for "must pass before merge." `.claude/` layers agent-specific gates and ergonomic hooks on top; it doesn't substitute for the universal gates. @@ -50,7 +50,7 @@ The marker invalidates on every commit (HEAD SHA changes), so `make ci` re-runs | `.claude/settings.json` | Team-wide: `deny` permissions (force-push, gh pr merge / ready / approve, --no-verify, the obvious marker-write idioms, secrets), `worktree.baseRef: "fresh"` + symlinkDirectories, all three hooks wired | | `.claude/hooks/gofumpt-on-save.sh` | PostToolUse Edit/Write/MultiEdit: auto-formats `.go` files | | `.claude/hooks/agent-bash-gate.sh` | PreToolUse Bash: enforces Agent PR Discipline (drafts only, no human reviewer adds, no `--no-verify`, marker required on PR pushes) | -| `.claude/hooks/review-marker.sh` | PostToolUse Agent: writes `tmp/review-passed-` when `pre-push-reviewer` returns `VERDICT: ship_it` | +| `.claude/hooks/review-marker.sh` | SubagentStop: writes `tmp/review-passed-` when `pre-push-reviewer` returns `VERDICT: ship_it`. Filters by `agent_type` in-script (SubagentStop has no matcher). Reads `.last_assistant_message` (flat string) rather than PostToolUse:Agent's structured `tool_response` | | `.claude/commands/cover.md` | `/cover [suite]` — suite dispatch + coverage threshold analysis | | `.claude/agents/pre-push-reviewer.md` | `pre-push-reviewer` subagent — canonical pre-push review, also used for auditing others' PRs locally | | `.claude/skills/pr-sync-with-main/SKILL.md` | "Fix this stale PR" workflow — merge origin/main, never rebase or force-push | From 34494789292b461d6318cc283f48b09c30987906 Mon Sep 17 00:00:00 2001 From: Eric Andrechek Date: Tue, 19 May 2026 14:07:21 -0400 Subject: [PATCH 13/18] chore(claude): doc-sync the SubagentStop hook event rename MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit (`dc91aa7`) moved review-marker.sh from PostToolUse:Agent to SubagentStop but only updated claude-code.md. Pre-push-reviewer's strict-rubric scan caught three stale references: - AGENTS.md §"No bypass for agents" — the markers-exclusively bullet still named the old hook event. - CHANGELOG.md [Unreleased] — the rollout entry parenthetical. - agent-bash-gate.sh:190 — the user-facing error message shown when an agent hits the missing-review-marker branch. All three now say SubagentStop. No behavior change. Co-Authored-By: Claude Opus 4.7 (1M context) --- .claude/hooks/agent-bash-gate.sh | 2 +- AGENTS.md | 2 +- CHANGELOG.md | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.claude/hooks/agent-bash-gate.sh b/.claude/hooks/agent-bash-gate.sh index 94cf2d0a..13202169 100755 --- a/.claude/hooks/agent-bash-gate.sh +++ b/.claude/hooks/agent-bash-gate.sh @@ -187,7 +187,7 @@ Invoke the pre-push-reviewer subagent in fresh context before pushing: asking it to review the current branch's full diff vs main, the latest commit, open PR comments, and CI status. -When the agent's response ends with "VERDICT: ship_it", a PostToolUse hook +When the agent's response ends with "VERDICT: ship_it", a SubagentStop hook auto-writes tmp/review-passed-${short_sha} and this push will succeed. If the agent returns VERDICT: iterate or VERDICT: block, address the findings diff --git a/AGENTS.md b/AGENTS.md index 5d06873b..98b8e72e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -263,7 +263,7 @@ The orchestrator agent cannot override the subagent's system prompt (it's the fi - `git push --no-verify` and `git commit --no-verify` are blocked at the `.claude/hooks/agent-bash-gate.sh` PreToolUse layer for agents. Humans retain `--no-verify` for explicit intentional bypass (see §"Local-First Validation"). - The obvious tool-level writes to `tmp/ci-passed-*` or `tmp/review-passed-*` are denied at the `.claude/settings.json` permission layer (`Bash(touch tmp/ci-passed:*)`, `Write(tmp/ci-passed-*)`, `Edit(tmp/ci-passed-*)`, and the review-passed equivalents). -- **Markers are written exclusively by `make ci` (ci-passed) and the `pre-push-reviewer` PostToolUse hook (review-passed). You do not write a marker file by any other means — period.** Bash can write a file by a dozen paths and the deny list does not enumerate all of them; this is an honest-agent rule, not an adversarial gate. If you ever feel tempted to write a marker, stop: the marker is wrong-shaped if you're the one writing it. Run `make ci`, invoke the subagent, get the verdict — that's the path. +- **Markers are written exclusively by `make ci` (ci-passed) and the `pre-push-reviewer` SubagentStop hook (review-passed). You do not write a marker file by any other means — period.** Bash can write a file by a dozen paths and the deny list does not enumerate all of them; this is an honest-agent rule, not an adversarial gate. If you ever feel tempted to write a marker, stop: the marker is wrong-shaped if you're the one writing it. Run `make ci`, invoke the subagent, get the verdict — that's the path. ### Reviewing someone else's PR locally diff --git a/CHANGELOG.md b/CHANGELOG.md index a3bafd2a..26e319c8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -41,7 +41,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **Dependabot auto-merge no longer slipped past CI on consolidated-pipeline PRs** (`main branch protection` ruleset; `.github/workflows/project-orchestrator.yml`; `.github/workflows/claude-review.yml`; `AGENTS.md`): on commit `93b3206` the previously six-job CI was collapsed into a single job named `CI`, but three places still referenced the old multi-job names — the ruleset's `required_status_checks` (`Validate` + `Admin approval` only — `CI` not listed), the `bot-clean` check in `project-orchestrator.yml` (looking for `Build`/`Validate`/`Lint`/`Test`/`Integration Tests`/`SDK Tests`), and `REQUIRED_CHECKS` in `claude-review.yml` (same six). Net effect: a Dependabot PR opened at T+0 would have its `Validate` and `Admin approval` checks satisfied within ~30s (PR-title workflow + the auto-approval workflow), `gh pr merge --auto` would fire, GitHub would see all *required* checks green, and the PR would squash-merge before `ci.yml` even started running. Pre-consolidation runs (PR #90 etc.) didn't expose this because GitHub's auto-merge waits for in-flight checks as a courtesy, and the multi-job CI was producing checks before auto-merge fired — but with one collapsed `CI` check that didn't start until later, the courtesy wait disappeared. Fix: added `CI` to the ruleset's required-checks (now `Admin approval` / `CI` / `Validate`) via the `gh api PUT /repos/Wave-RF/WaveHouse/rulesets/15353356` round-trip, and updated both bot-clean lists to look for `CI` + `Validate`. AGENTS.md note about the required-check set updated to match. Diagnosis steps recorded inline as code comments in both workflow files so future-me knows where to look if the check name changes again. ### Added -- **Team-wide Claude Code configuration + agent PR discipline gates** (`.claude/agents/pre-push-reviewer.md`, `.claude/hooks/agent-bash-gate.sh`, `.claude/hooks/review-marker.sh`, `.claude/hooks/gofumpt-on-save.sh`, `.claude/settings.json`, `.claude/skills/pr-review-locally/SKILL.md`, `.claude/skills/pr-sync-with-main/SKILL.md`, `.claude/commands/cover.md`, `.githooks/pre-commit`, `.githooks/pre-push`, `.config/wt.toml`, `AGENTS.md` §"Agent PR Discipline" + §"Local-First Validation", `docs/src/content/docs/claude-code.md`). Two distinct enforcement layers: universal git hooks (`.githooks/`, installed by `make tools` via `git config core.hooksPath .githooks`) run `make verify` on commit and require `tmp/ci-passed-` before push for everyone (humans + agents); agent-only gates layer on top in `.claude/hooks/agent-bash-gate.sh` (PreToolUse Bash) and block: `--no-verify` on git push/commit; `gh pr create` without `--draft`; `gh pr ready`, `--approve`, `--request-changes` (humans-only state transitions); `gh pr edit --add-reviewer/--add-assignee` and `gh api .../requested_reviewers` POST/PUT/PATCH (humans-only reviewer assignment — bot reviewers re-triggered via PR comments instead); and `git push` to an open-PR branch without both ci-passed and review-passed markers. The `pre-push-reviewer` subagent runs the canonical `.github/prompts/pr-review.md` in fresh context against the branch's full merge-base diff (`git diff main...HEAD`) plus latest commit, open PR comments + reviews, CI status, and linked-issue acceptance criteria, then emits a parseable `VERDICT: ship_it|iterate|block` consumed by `review-marker.sh` (PostToolUse Agent) which writes `tmp/review-passed-` only on `ship_it`. **`ship_it` is gated to zero findings at any severity** — any `[MUST]`/`[SHOULD]`/`[MAY]` forces `iterate` and the orchestrator loops review → fix → review until clean before pushing. Marker writes are NOT regex-blocked beyond the cheap settings.json deny patterns (`Bash(touch tmp/ci-passed:*)`, `Write(tmp/ci-passed-*)`, `Edit(tmp/ci-passed-*)`, and review-passed equivalents); the rest is an explicit honest-agent rule in AGENTS.md ("you do not write a marker file by any other means — period") because Bash can write a file by a dozen paths and regex enforcement is a porous game of whack-a-mole. Humans retain `--no-verify` for explicit intentional bypass; `.config/wt.toml` ships worktrunk hooks so parallel-agent worktrees install `.githooks/` correctly. `docs/src/content/docs/claude-code.md` is the contributor-facing page that explains the layering, quick-setup, and discipline rules. +- **Team-wide Claude Code configuration + agent PR discipline gates** (`.claude/agents/pre-push-reviewer.md`, `.claude/hooks/agent-bash-gate.sh`, `.claude/hooks/review-marker.sh`, `.claude/hooks/gofumpt-on-save.sh`, `.claude/settings.json`, `.claude/skills/pr-review-locally/SKILL.md`, `.claude/skills/pr-sync-with-main/SKILL.md`, `.claude/commands/cover.md`, `.githooks/pre-commit`, `.githooks/pre-push`, `.config/wt.toml`, `AGENTS.md` §"Agent PR Discipline" + §"Local-First Validation", `docs/src/content/docs/claude-code.md`). Two distinct enforcement layers: universal git hooks (`.githooks/`, installed by `make tools` via `git config core.hooksPath .githooks`) run `make verify` on commit and require `tmp/ci-passed-` before push for everyone (humans + agents); agent-only gates layer on top in `.claude/hooks/agent-bash-gate.sh` (PreToolUse Bash) and block: `--no-verify` on git push/commit; `gh pr create` without `--draft`; `gh pr ready`, `--approve`, `--request-changes` (humans-only state transitions); `gh pr edit --add-reviewer/--add-assignee` and `gh api .../requested_reviewers` POST/PUT/PATCH (humans-only reviewer assignment — bot reviewers re-triggered via PR comments instead); and `git push` to an open-PR branch without both ci-passed and review-passed markers. The `pre-push-reviewer` subagent runs the canonical `.github/prompts/pr-review.md` in fresh context against the branch's full merge-base diff (`git diff main...HEAD`) plus latest commit, open PR comments + reviews, CI status, and linked-issue acceptance criteria, then emits a parseable `VERDICT: ship_it|iterate|block` consumed by `review-marker.sh` (SubagentStop) which writes `tmp/review-passed-` only on `ship_it`. **`ship_it` is gated to zero findings at any severity** — any `[MUST]`/`[SHOULD]`/`[MAY]` forces `iterate` and the orchestrator loops review → fix → review until clean before pushing. Marker writes are NOT regex-blocked beyond the cheap settings.json deny patterns (`Bash(touch tmp/ci-passed:*)`, `Write(tmp/ci-passed-*)`, `Edit(tmp/ci-passed-*)`, and review-passed equivalents); the rest is an explicit honest-agent rule in AGENTS.md ("you do not write a marker file by any other means — period") because Bash can write a file by a dozen paths and regex enforcement is a porous game of whack-a-mole. Humans retain `--no-verify` for explicit intentional bypass; `.config/wt.toml` ships worktrunk hooks so parallel-agent worktrees install `.githooks/` correctly. `docs/src/content/docs/claude-code.md` is the contributor-facing page that explains the layering, quick-setup, and discipline rules. - **Astro / Starlight documentation site at `wavehouse.dev`** (`docs/`, `Makefile`, `.github/dependabot.yml`, `.github/workflows/ci.yml`, `.gitignore`, `.vscode/launch.json`): full content + tooling for the public docs (Getting Started, Why WaveHouse, Architecture, API Reference, TypeScript SDK, Configuration, Deployment, Development). Build-time mermaid via `rehype-mermaid` (inline-svg strategy, no client-side JS), LaTeX via `remark-math` + `rehype-katex`, image zoom via `starlight-image-zoom`, expressiveCode with github dark/light + `env`/`dns` Shiki language aliases. PostHog frontend telemetry (key is public by design — embedded in every visitor's browser). `editLink` pointed at `main`; `lastUpdated` on. Sidebar lives in `docs/src/config/sidebar.ts` as the single source of truth, consumed by both Starlight rendering and the LLM-friendly outputs below. Cloudflare Workers + Static Assets deployment via `docs/wrangler.jsonc` (no auto-deploy yet — wired up when the GH Actions workflow lands separately). Make targets stay minimal: `dev-docs` (Astro dev server on :4321), `build-docs`, `preview-docs` — the last serves the production build through `wrangler dev`, so the Cloudflare Worker's content negotiation (`Accept: text/markdown` / LLM-bot User-Agent → `.md` twin) is exercised end-to-end the same way it will be in production. - **Per-page `.md` twin + `llms.txt` family via `starlight-llm-tools`** (`docs/astro.config.mjs`, `docs/worker/index.ts`, `docs/wrangler.jsonc`): every doc is also served as raw markdown at `.md` with a navigation header (Section / Subpages / Related / HTML version pointers), plus three concatenated views — `/llms.txt` manifest, `/llms-full.txt` (every page, sidebar-ordered), `/llms-small.txt` (overview pages only). Page-title slot gains a **Copy Markdown** button and **Open with AI** dropdown (Claude / ChatGPT / Cursor). Content negotiation is handled by a Cloudflare Worker that re-exports the `cloudflare-md-router` package's default handler in one line — `Accept: text/markdown` or known LLM-bot User-Agent (GPTBot, ClaudeBot, PerplexityBot, Applebot-Extended, etc.) gets the `.md` twin transparently, with a fall-through to the HTML response when the twin doesn't exist. - **Two extracted plugin packages, MIT-licensed**: [`github.com/Wave-RF/cloudflare-md-router`](https://github.com/Wave-RF/cloudflare-md-router) (Workers handler + `createMdRouter()` factory + extensible bot-UA regex; consumed in `docs/worker/index.ts` as a one-line re-export) and [`github.com/Wave-RF/starlight-llm-tools`](https://github.com/Wave-RF/starlight-llm-tools) (Starlight plugin that auto-injects the four routes, the two components, and the `PageTitle` override; optionally calls into `starlight-glossary/transform` when present). Both pinned via `github:Wave-RF/...` in `docs/package.json`; lockfile pins the resolved commit SHAs. Pre-npm-publish state — switch to normal version specifiers when the packages land on npm. From 92e2c6a67a17657c195623981d9976597006befb Mon Sep 17 00:00:00 2001 From: Eric Andrechek Date: Tue, 19 May 2026 14:22:03 -0400 Subject: [PATCH 14/18] fix(claude): emit diagnostic stderr from review-marker.sh on jq failure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-3 CodeRabbit finding on PR #147: the two `jq -r ... 2>/dev/null` calls (one for `.agent_type`, one for `.last_assistant_message`) silently swallow errors, leaving the orchestrator to push, hit the missing review marker, and have no idea why. Same posture as the round-2 fix to `agent-bash-gate.sh`, except this hook exits 0 on its own failures rather than blocking — it's the marker writer, not the push gate; the absence of a marker is itself the downstream enforcement signal: - `command -v jq` check at the top — emits "jq not found" diagnostic, exits 0. - Each `jq -r` call captured via `if ! var=$(...); then`, emitting a field-specific diagnostic on parse failure, exits 0. Tested all three failure modes: malformed JSON → diagnostic; non- pre-push-reviewer subagent → silent exit 0 (legitimate non-match); valid `pre-push-reviewer` with `VERDICT: ship_it` → marker written. Co-Authored-By: Claude Opus 4.7 (1M context) --- .claude/hooks/review-marker.sh | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/.claude/hooks/review-marker.sh b/.claude/hooks/review-marker.sh index 92b93a98..2e334b61 100755 --- a/.claude/hooks/review-marker.sh +++ b/.claude/hooks/review-marker.sh @@ -26,12 +26,29 @@ set -uo pipefail input=$(cat) +# Failure modes for this hook (jq missing, malformed JSON) should leave +# stderr breadcrumbs rather than silently no-op — otherwise the orchestrator +# pushes, gets blocked by the missing review marker, and has no clue why. +# Mirrors agent-bash-gate.sh's posture, except this hook exits 0 on its own +# failures (it's the marker writer, not the push gate; the absence of a +# marker is itself the enforcement signal downstream). +if ! command -v jq >/dev/null 2>&1; then + echo "review-marker: jq not found; cannot parse SubagentStop payload — no marker written." >&2 + exit 0 +fi + # SubagentStop fires for every subagent completion (no matcher support per # Claude Code docs), so we filter by `agent_type` in-script. -agent_type=$(printf '%s' "$input" | jq -r '.agent_type // empty' 2>/dev/null) +if ! agent_type=$(printf '%s' "$input" | jq -r '.agent_type // empty' 2>/dev/null); then + echo "review-marker: malformed SubagentStop payload; could not parse .agent_type — no marker written." >&2 + exit 0 +fi [ "$agent_type" = "pre-push-reviewer" ] || exit 0 -response=$(printf '%s' "$input" | jq -r '.last_assistant_message // empty' 2>/dev/null) +if ! response=$(printf '%s' "$input" | jq -r '.last_assistant_message // empty' 2>/dev/null); then + echo "review-marker: malformed SubagentStop payload; could not parse .last_assistant_message — no marker written." >&2 + exit 0 +fi [ -z "$response" ] && exit 0 # Parse the parseable verdict line. Format (per .claude/agents/pre-push-reviewer.md): From c8731531aaefa1932b4846b3e9db6bbe400d4feb Mon Sep 17 00:00:00 2001 From: Eric Andrechek Date: Tue, 19 May 2026 14:32:32 -0400 Subject: [PATCH 15/18] fix(claude): guard the marker write so failures surface as errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-4 CodeRabbit finding on PR #147: the success message at the tail of review-marker.sh was emitted unconditionally after `mkdir -p tmp && touch ...`, so a write failure (full disk, permission denied, etc.) would print "📝 Pre-push review marker written" while the file didn't actually exist. The orchestrator would then push, get blocked by the missing marker, and have no clue why the success line lied. Now: if mkdir -p tmp && touch "tmp/review-passed-${head_sha}"; then echo "📝 Pre-push review marker written: …" >&2 else echo "review-marker: failed to write … — no marker written." >&2 fi Same posture as the other failure paths in this hook: stderr diagnostic on failure, exit 0 (the missing marker is the enforcement signal downstream — exit 2 would conflate hook misbehaviour with verdict). Co-Authored-By: Claude Opus 4.7 (1M context) --- .claude/hooks/review-marker.sh | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/.claude/hooks/review-marker.sh b/.claude/hooks/review-marker.sh index 2e334b61..e1b5f7aa 100755 --- a/.claude/hooks/review-marker.sh +++ b/.claude/hooks/review-marker.sh @@ -69,8 +69,10 @@ cd "${CLAUDE_PROJECT_DIR:-.}" 2>/dev/null || exit 0 head_sha=$(git rev-parse HEAD 2>/dev/null) [ -z "$head_sha" ] && exit 0 -mkdir -p tmp -touch "tmp/review-passed-${head_sha}" -echo "📝 Pre-push review marker written: tmp/review-passed-${head_sha:0:8}" >&2 +if mkdir -p tmp && touch "tmp/review-passed-${head_sha}"; then + echo "📝 Pre-push review marker written: tmp/review-passed-${head_sha:0:8}" >&2 +else + echo "review-marker: failed to write tmp/review-passed-${head_sha:0:8} — no marker written." >&2 +fi exit 0 From 42d08f5a89383c4be1c75633cda79f33e2fbb000 Mon Sep 17 00:00:00 2001 From: Eric Andrechek Date: Tue, 19 May 2026 15:07:50 -0400 Subject: [PATCH 16/18] chore(claude-review): drop auto-trigger, manual-only via comment or dispatch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Removes the `pull_request` trigger so the workflow only fires on `@claude` / `/review` PR comments from trusted reviewers, or `workflow_dispatch`. Falls out: - Header comment no longer needs the `pull_request_target` OIDC paragraph — that tradeoff only applied to the auto-trigger path. - Concurrency: `cancel-in-progress` is now unconditionally `false`; manual triggers should always queue, never cancel an in-flight run. - Workflow-level `if:` drops the `pull_request` branch and its dependabot-author exclusion. - Gate step drops `PR_EVENT_NUM` / `PR_EVENT_HEAD_SHA` and the `pull_request)` case; both remaining paths need the same `gh pr view --jq .headRefOid` call so it's hoisted out of the case. Trust gate stays — it's actually more load-bearing under manual-only, since `issue_comment` runs with full repo secrets and a trusted reviewer commenting `@claude` on a fork PR would otherwise check out untrusted fork code with write tokens. Updated the header comment to lead with that reasoning. AGENTS.md synced in three places: the "Re-request review" step, the review tooling reference table, and the Repository Automation section's Claude PR review entry. Co-Authored-By: Claude Opus 4.7 (1M context) --- .github/workflows/claude-review.yml | 52 ++++++++--------------------- AGENTS.md | 6 ++-- 2 files changed, 17 insertions(+), 41 deletions(-) diff --git a/.github/workflows/claude-review.yml b/.github/workflows/claude-review.yml index 0c2f419e..2707e63d 100644 --- a/.github/workflows/claude-review.yml +++ b/.github/workflows/claude-review.yml @@ -1,25 +1,17 @@ name: Claude PR review -# Auto-reviews PRs whose HEAD commit was authored or pushed by a user -# with repo permission. Manual re-runs via `@claude` / `/review` -# comments (trusted reviewers only) or workflow_dispatch. -# -# Triggers use `pull_request` rather than `pull_request_target`: the -# action's OIDC token exchange rejects pull_request_target claims -# (anthropics/claude-code-action#713). The cost is that fork-PR -# secrets are unavailable, so drive-by contributor PRs aren't -# auto-reviewed — the benefit is that the action can never run with -# fork-supplied code holding write tokens. +# Manual-only PR review. Triggered by `@claude` or `/review` +# comments from trusted reviewers, or by workflow_dispatch. # # Trust gate: HEAD commit's author OR committer must have ≥read on -# the repo. Accepting either covers the admin-fixup-onto-fork-PR case -# (author = external, committer = admin) that author-only gates miss. -# When the gate skips, the job exits 0 so the "Claude review" check -# stays visible on the PR. +# the repo. Without this, a trusted reviewer commenting `@claude` on +# a fork PR would check out untrusted fork code with write tokens. +# Accepting either author or committer covers the +# admin-fixup-onto-fork-PR case (author = external, committer = admin) +# that author-only gates miss. When the gate skips, the job exits 0 +# so the "Claude review" check stays visible on the PR. on: - pull_request: - types: [opened, synchronize, reopened, ready_for_review] issue_comment: types: [created] workflow_dispatch: @@ -29,16 +21,11 @@ on: required: true type: string -# Dedup per PR. Only `pull_request` events cancel the in-flight run — -# a new HEAD SHA invalidates any review mid-flight. Other triggers -# (issue_comment, workflow_dispatch) queue behind it instead, so: -# - `@claude` / `/review` comments don't kill the in-flight review -# - manual retries don't kill the previous attempt -# - Claude's own sticky-comment edits (which fire `issue_comment`) -# can't cancel themselves +# Dedup per PR. Manual triggers (comments, dispatches) queue rather +# than cancel — back-to-back retries don't kill an in-flight review. concurrency: - group: claude-review-${{ github.event.pull_request.number || github.event.issue.number || inputs.pr_number || github.run_id }} - cancel-in-progress: ${{ github.event_name == 'pull_request' }} + group: claude-review-${{ github.event.issue.number || inputs.pr_number || github.run_id }} + cancel-in-progress: false jobs: review: @@ -48,9 +35,6 @@ jobs: # and would miss admin-pushed fixups. if: | ( - github.event_name == 'pull_request' - && github.event.pull_request.user.login != 'dependabot[bot]' - ) || ( github.event_name == 'issue_comment' && github.event.issue.pull_request != null && (contains(github.event.comment.body, '@claude') || contains(github.event.comment.body, '/review')) @@ -72,29 +56,21 @@ jobs: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} REPO: ${{ github.repository }} EVENT_NAME: ${{ github.event_name }} - PR_EVENT_NUM: ${{ github.event.pull_request.number }} - PR_EVENT_HEAD_SHA: ${{ github.event.pull_request.head.sha }} ISSUE_COMMENT_PR: ${{ github.event.issue.number }} DISPATCH_PR: ${{ inputs.pr_number }} run: | set -euo pipefail - # Resolve PR number + HEAD SHA. pull_request has both in the - # webhook payload; other paths need a `gh pr view` round-trip. + # Resolve PR number from the trigger, then look up HEAD SHA. case "$EVENT_NAME" in - pull_request) - pr_num="$PR_EVENT_NUM" - head_sha="$PR_EVENT_HEAD_SHA" - ;; issue_comment) pr_num="$ISSUE_COMMENT_PR" - head_sha=$(gh pr view "$pr_num" -R "$REPO" --json headRefOid --jq '.headRefOid') ;; workflow_dispatch) pr_num="$DISPATCH_PR" - head_sha=$(gh pr view "$pr_num" -R "$REPO" --json headRefOid --jq '.headRefOid') ;; esac + head_sha=$(gh pr view "$pr_num" -R "$REPO" --json headRefOid --jq '.headRefOid') # Self-modification gate. The action's validation step # auto-fails when its own workflow or prompt file is in the diff --git a/AGENTS.md b/AGENTS.md index 98b8e72e..e9d45289 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -182,7 +182,7 @@ Every review comment gets a substantive reply, and every thread gets resolved be Without the mention, the bot never sees the reply and the dialog silently terminates. 4. **Fix in this PR** if the suggestion is right and in scope. Out-of-scope but valid: link a tracking issue before resolving. 5. **Resolve the thread** once the reply addresses the concern and no counter-reply is pending. Bot threads are safe to resolve after a substantive reply (bots only re-engage on mention); human threads — wait for them. -6. **Re-request review** from humans after substantive changes. Bot reviewers re-run on `synchronize` (Claude, Gemini) or via a re-request button (Copilot). +6. **Re-request review** from humans after substantive changes. Bot reviewers re-run on `synchronize` (Gemini), via PR-comment mention (Claude), or via a re-request button (Copilot). ### What not to do @@ -193,7 +193,7 @@ Every review comment gets a substantive reply, and every thread gets resolved be | Reviewer | How it runs | Re-runs on new commits | Blocks merge | | -------- | ----------- | ---------------------- | ------------ | -| Claude (`.github/workflows/claude-review.yml`) | Our workflow, fires on every PR push (open/sync/reopen/ready). Manual re-trigger via `@claude` / `/review` in a PR comment, or `gh workflow run "Claude PR review" -f pr_number=` | Yes — posts inline review comments plus a sticky verdict-summary comment that edits in place | Yes for inline comments — `required_review_thread_resolution: true` blocks merge until each `claude[bot]` thread is resolved. The workflow's check itself is advisory | +| Claude (`.github/workflows/claude-review.yml`) | Manual-only. Comment `@claude` or `/review` on the PR (trusted reviewers), or run `gh workflow run "Claude PR review" -f pr_number=`. Findings post as inline review comments plus a sticky verdict-summary comment that edits in place | No — re-trigger by mention after pushing new commits | Yes for inline comments — `required_review_thread_resolution: true` blocks merge until each `claude[bot]` thread is resolved. The workflow's check itself is advisory | | Gemini Code Assist | Marketplace App at repo level | Yes on synchronize. **Silently skips `.github/workflows/**`** (built-in exclusion, can't be overridden) — Gemini rarely sees infra PRs | No (advisory) | | Copilot | GitHub-native, requires a reviewer with Copilot Pro | Yes if enabled | No (advisory) | | Human admins | Review requested from a non-author admin by `housekeeping.yml` on PR open / ready-for-review (not on every push). Selection picks the other admin if the author is one, otherwise round-robins. The composite also sets `assignees`. | Not on synchronize. Manual re-request via the GitHub UI's "Re-request review" if `dismiss_stale_reviews_on_push` clears the request. | Yes — `admin-approval.yml` is a required status check that fails unless an admin has approved. Dependabot patch/minor bypasses (auto-merge handles those); major bumps fall through to admin review. | @@ -400,7 +400,7 @@ docs/ → Project documentation - **Issue triage** (`triage.yml`): GitHub Models classifies new/edited issues and applies `area/*` + `security` + `breaking-change` labels. - **Code review** (advisory; the `Admin approval` required status check + the ruleset are the actual merge gate): - **Gemini Code Assist App** configured via `.gemini/styleguide.md`. - - **Claude PR review** (`claude-review.yml`) runs on every PR open or push, gated on the HEAD commit's author or committer having ≥read permission. Dependabot is filtered at workflow level. Findings post as inline review comments (blocked by `required_review_thread_resolution`) plus a sticky verdict summary. Manual re-trigger via `@claude` / `/review` from a trusted commenter or via `workflow_dispatch`. Review-only — Claude can comment but not push. Requires the `CLAUDE_CODE_OAUTH_TOKEN` secret (`claude setup-token`). + - **Claude PR review** (`claude-review.yml`) runs only on manual trigger: `@claude` or `/review` from a trusted commenter on a PR, or `workflow_dispatch`. Gated on the HEAD commit's author or committer having ≥read permission so a comment on a fork PR can't run untrusted code with write tokens. Findings post as inline review comments (blocked by `required_review_thread_resolution`) plus a sticky verdict summary. Review-only — Claude can comment but not push. Requires the `CLAUDE_CODE_OAUTH_TOKEN` secret (`claude setup-token`). - **Dependabot auto-merge** (`dependabot-automerge.yml`): patch/minor bumps auto-approve + auto-merge; major bumps hold for human review. CI still gates the actual merge. Patch/minor bypass `Admin approval` (the workflow + CI passing is the trust model); major bumps fall through to admin review like any human PR — this closed a hole where a bot's APPROVED review (e.g. CodeRabbit) could merge a major bump without admin involvement (see #130). ## Governance Files From bc81178936d42819173448f35036d986a45bffe9 Mon Sep 17 00:00:00 2001 From: Eric Andrechek Date: Tue, 19 May 2026 15:13:01 -0400 Subject: [PATCH 17/18] docs(changelog): record the claude-review manual-only switch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds an Unreleased § Changed bullet for the previous commit (42d08f5) — pre-push reviewer flagged it as a [MAY], and the section already tracks workflow-tuning entries of similar shape, so it's a clearer entry point than letting the change live only in the commit log. Co-Authored-By: Claude Opus 4.7 (1M context) --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 26e319c8..cf624faf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- **Claude PR review is now manual-only** (`.github/workflows/claude-review.yml`, `AGENTS.md`): dropped the `pull_request` trigger (was firing on opened/synchronize/reopened/ready_for_review) so the workflow only runs on `@claude` or `/review` comments from trusted reviewers, or `workflow_dispatch`. "Claude review" was never a required status check (the ruleset gates on `Admin approval` / `CI` / `PR housekeeping`), so this changes who pays the action budget — every push no longer triggers a review automatically; reviewers explicitly opt in via mention after they've looked at the diff themselves. Falls out: the workflow header's `pull_request_target` OIDC paragraph dropped (only applied to the auto-trigger path), concurrency's `cancel-in-progress` is now unconditionally `false` (manual triggers should always queue, never cancel an in-flight review), the workflow-level `if:` loses its `pull_request` branch + dependabot-author exclusion, and the gate-step `case` drops the `pull_request)` arm + its two `PR_EVENT_*` env vars (`gh pr view --jq .headRefOid` is hoisted out of the case since both remaining paths need it identically). The trust gate stays — it's *more* load-bearing under manual-only, since `issue_comment` runs with full repo secrets and a trusted reviewer commenting `@claude` on a fork PR would otherwise check out untrusted fork code with write tokens. AGENTS.md synced in three places: §"Review Response" step 6 (Claude moved from synchronize-rerun to PR-comment-mention), the review tooling reference table (Claude row now "Manual-only" with "No" in the re-runs column), and §"Repository Automation" §"Code review" entry (rewritten to lead with manual-only). - **BREAKING:** SSE/WS streaming endpoints replaced `?topic=` with `?table=` (issue #100, option B). The NATS subject convention no longer leaks into the public HTTP API; the server builds `ingest.` internally. SSE returns `400 Bad Request` when `?table=` is missing or fails the `^[a-zA-Z_][a-zA-Z0-9_]*$` safe-identifier check (the same regex `internal/ingest` and `internal/query` use for SQL identifiers — crucially this rejects NATS wildcard characters `*` and `>` before they reach the gap-fill `FilterSubject`). WS `?table=` is optional — clients can still defer to in-band subscribe commands — but is validated by the same regex when present. - **BREAKING:** WebSocket in-band subscribe/unsubscribe commands use `"table"` (raw name) instead of `"topic"` (NATS subject): - Before: `{"action":"subscribe","topic":"ingest.clicks"}` From acd6c044a3f02f5d19cd26a15c6fcb662655fa0e Mon Sep 17 00:00:00 2001 From: Eric Andrechek Date: Tue, 19 May 2026 15:21:44 -0400 Subject: [PATCH 18/18] fix(docs): use absolute path for the claude-code link MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `docs/src/content/docs/development.md:49` linked to `claude-code.md` relatively, which the starlight-links-validator (newly stricter in the docs site rebuild merged from #142) rejects. Repo convention for sibling links in `src/content/docs/` is the absolute, suffix-less form `/claude-code` — switching to that. Confirmed locally with `pnpm run build`: "All internal links are valid." Co-Authored-By: Claude Opus 4.7 (1M context) --- docs/src/content/docs/development.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/src/content/docs/development.md b/docs/src/content/docs/development.md index c0a79368..e6138d10 100644 --- a/docs/src/content/docs/development.md +++ b/docs/src/content/docs/development.md @@ -46,7 +46,7 @@ If any of those are wrong/missing, the Makefile recipes will fail with confusing | Tool | Why | Install | | ---- | --- | ------- | -| **[Claude Code](https://claude.com/claude-code)** | The repo ships team-wide configuration in `.claude/` — slash commands, subagents, hooks, status line. See [Claude Code & AI agents](claude-code.md) for setup. | `brew install --cask claude-code` (macOS) or follow [official install](https://code.claude.com/docs/en/quickstart) | +| **[Claude Code](https://claude.com/claude-code)** | The repo ships team-wide configuration in `.claude/` — slash commands, subagents, hooks, status line. See [Claude Code & AI agents](/claude-code) for setup. | `brew install --cask claude-code` (macOS) or follow [official install](https://code.claude.com/docs/en/quickstart) | | **[worktrunk](https://worktrunk.dev)** | Wraps `git worktree` for parallel-agent workflows. Project hooks live in `.config/wt.toml` (auto-runs `make tools` on new worktrees, `make verify` on pre-merge). | `brew install worktrunk && wt config shell install` | ## Quick Start