diff --git a/.claude/agents/pre-push-reviewer.md b/.claude/agents/pre-push-reviewer.md new file mode 100644 index 00000000..2b17b45e --- /dev/null +++ b/.claude/agents/pre-push-reviewer.md @@ -0,0 +1,105 @@ +--- +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 +--- + +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 **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). + +## 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 main...HEAD + ``` + +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: + + ```text + 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 + +```markdown +## 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 + +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. + +- **`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 + +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..b49119f4 --- /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|merge|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..13202169 --- /dev/null +++ b/.claude/hooks/agent-bash-gate.sh @@ -0,0 +1,210 @@ +#!/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/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. 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. + +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) +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 + +# 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") + +# Boundary helper: detects a `git ` invocation anywhere in the +# (quote-stripped) command (including after && / ; / | / cd ... &&). Used +# by multiple checks. +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 +# adversarial — eval / sh -c wrappers around `git push --no-verify` could +# still bypass; AGENTS.md §"Agent PR Discipline" makes the rule explicit. +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' "$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' "$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' "$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 + +# --- 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' "$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' "$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' "$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 + +# --- 7. git push: check markers --------------------------------------------- +# Only on actual `git push` invocations (not `git push --help`, not `gh pr push`). +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}" + ci_marker="tmp/ci-passed-${head_sha}" + review_marker="tmp/review-passed-${head_sha}" + + # 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 + 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 </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..e1b5f7aa --- /dev/null +++ b/.claude/hooks/review-marker.sh @@ -0,0 +1,78 @@ +#!/usr/bin/env bash +# SubagentStop hook — writes the pre-push review marker. +# +# 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 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) + +# 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. +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 + +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): +# VERDICT: ship_it | VERDICT: iterate | VERDICT: block +# 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 -iE '^[[:space:]]*VERDICT:[[:space:]]*(ship_it|iterate|block)[[:space:]]*$' \ + | tail -1 \ + | tr '[:upper:]' '[:lower:]' \ + | sed -E 's/^[[:space:]]*verdict:[[:space:]]*([a-z_]+)[[:space:]]*$/\1/') + +[ "$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 + +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 diff --git a/.claude/settings.json b/.claude/settings.json new file mode 100644 index 00000000..5ee0792d --- /dev/null +++ b/.claude/settings.json @@ -0,0 +1,98 @@ +{ + "$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-*)", + "MultiEdit(tmp/ci-passed-*)", + "MultiEdit(tmp/review-passed-*)", + + "Read(./.env)", + "Read(./.env.*)", + "Read(./secrets/**)", + "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" + } + ] + } + ], + "SubagentStop": [ + { + "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..72974bd3 --- /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: + +```js +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" -f pr_number= +``` + +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 +- 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/.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/.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..3911de4a --- /dev/null +++ b/.githooks/pre-push @@ -0,0 +1,51 @@ +#!/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 + +# 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 <` 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/.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/.gitignore b/.gitignore index 2cd1becc..3d1b613c 100644 --- a/.gitignore +++ b/.gitignore @@ -51,6 +51,10 @@ node_modules/ *.swo *~ +# Claude Code local state (per-user; not for committing) +.claude/settings.local.json +.claude/worktrees/ + # Local git worktrees .worktrees/ diff --git a/AGENTS.md b/AGENTS.md index 77021005..e9d45289 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) @@ -57,12 +57,11 @@ 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 -make tools # Install pinned tools, Go modules, and pnpm deps +make tools # Install pinned tools, Go modules, pnpm deps, and git hooks make help # Show all targets with descriptions # Static checks (parallel-safe: `make -j verify`) @@ -122,21 +121,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` (>= 11.1) and `Node.js` (22 LTS — pinned via `.nvmrc` at the repo - root, matches CI) 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` (>= 11.1) and `Node.js` (22 LTS — pinned via `.nvmrc` at the repo root, matches CI) 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 @@ -163,6 +151,15 @@ make ci # Full parity with CI: parallel verify + builds + unit/SDK tests, then If `make ci` passes locally, your commit has crossed the same gates CI will run. For workflow-only changes, read the YAML diff carefully and run `actionlint` if you have it installed. +### Enforced via git hooks + +`make tools` installs team-wide git hooks via `git config core.hooksPath .githooks`. They apply to humans and Claude Code alike: + +- **`.githooks/pre-commit`** runs `make verify` (~30s) on every commit; blocks on failure. Also emits informational nudges for likely doc-sync / SDK-sync misses (see §Documentation Sync, §SDK Sync). +- **`.githooks/pre-push`** checks for a `tmp/ci-passed-` 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. @@ -185,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 @@ -196,36 +193,136 @@ 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. | > **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 + +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 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` 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 + +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. | 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. +## 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 @@ -233,24 +330,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 @@ -280,8 +376,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 @@ -304,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 diff --git a/CHANGELOG.md b/CHANGELOG.md index 861bd940..415181d2 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"}` @@ -41,6 +42,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` (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. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 70425148..9995e096 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). @@ -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 diff --git a/Makefile b/Makefile index 7f150f45..f994b8dd 100644 --- a/Makefile +++ b/Makefile @@ -502,6 +502,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 @@ -604,8 +608,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 98fe76e8..a38f9351 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 22 LTS (pinned via `.nvmrc`, matches CI), and pnpm 11.1+** 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 22 LTS (pinned via `.nvmrc`, matches CI), and pnpm 11.1+** 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/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. -- [ ] 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..8c4492d7 --- /dev/null +++ b/docs/src/content/docs/claude-code.md @@ -0,0 +1,236 @@ +--- +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 + +| 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 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` (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. + +## 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` 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. + +## What's in `.claude/` and `.config/` + +| 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 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` | 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 | +| `.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). `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". + +## 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 ed661423..e6138d10 100644 --- a/docs/src/content/docs/development.md +++ b/docs/src/content/docs/development.md @@ -42,6 +42,13 @@ pnpm --version # 11.1+ 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) 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: