diff --git a/docs/claude/commands.md b/docs/claude/commands.md index 8a6ab1a7..692dc26d 100644 --- a/docs/claude/commands.md +++ b/docs/claude/commands.md @@ -202,15 +202,48 @@ Review one or more GitHub Pull Requests. Fetches the diff, detects prior reviews **What it does:** -1. **Detects re-reviews** — checks if anything has changed since your last review; skips if not -2. **Classifies sensitivity** — auto-detects auth/RBAC/CI code and recommends Strict mode -3. **Asks strictness** — Quick, Standard, Thorough, or Strict -4. **Analyzes the diff** — runs in parallel sub-agents for batch mode; looks for bugs, null-safety issues, SQL parity, test coverage gaps, and more -5. **Posts inline comments** — one finding per comment, severity marked with 🔴/🟡/🟢; never bundles multiple findings in one comment -6. **Offers local testing (optional)** — when the PR touches frontend or backend code, asks whether to verify the changes locally; locates or clones the target repo, checks Docker is running (starts it or asks the user to), maps detected layers to applicable `/test-*` skills, builds a test plan, gets your approval, then executes — any new issues join the existing findings before the verdict -7. **Checks CI** — blocks APPROVE if required CI checks are failing -8. **Submits formal review** — APPROVE (no blockers) or REQUEST_CHANGES (one or more 🔴 findings) -9. **Resolves addressed threads** — replies "✅ Resolved in {sha}" to previously raised comments now fixed, and marks threads closed +1. **Checks batch scope first** — when given several PRs, detects whether one PR's head already contains another's commits (a retargeted stacked branch) and asks how to scope before staging anything. Reads `baseRefName` from the API, never the PR body, which goes stale the moment a branch is retargeted +2. **Classifies each PR into a lane** — `settled` / `delta` / `full`, before any diff, gate, guide, clone or sub-agent runs. See *Lanes* below +3. **Classifies sensitivity** — auto-detects auth/RBAC/CI code and recommends Strict mode +4. **Asks strictness** — Quick, Standard, Thorough, or Strict. Always asked; never auto-selected +5. **Consumes CI instead of repeating it** — when the repo's own gate job is green on *exactly* the PR's head SHA, that is taken as the evidence. A red gate check still gets a local run +6. **Analyzes the diff** — parallel sub-agents in batch mode; Quick/delta PRs from one repo may share a single agent (max 4). Sub-agents work to a stated tool-call and report-length budget per strictness mode +7. **Posts inline comments** — one finding per comment, severity marked with 🔴/🟡/🟢; never bundles multiple findings in one comment +8. **Offers local testing (optional)** — when the PR touches frontend or backend code, asks whether to verify the changes locally; locates or clones the target repo, checks Docker is running (starts it or asks the user to), maps detected layers to applicable `/test-*` skills, builds a test plan, gets your approval, then executes — any new issues join the existing findings before the verdict +9. **Checks CI** — blocks APPROVE if required CI checks are failing +10. **Submits formal review** — APPROVE (no blockers) or REQUEST_CHANGES (one or more 🔴 findings) +11. **Handles prior threads by author** — resolves threads *you* opened that are now addressed; on a co-reviewer's thread it posts an acknowledgment reply and leaves the thread for its author to close. Resolving someone else's comment misrepresents their agreement +12. **Records what the run cost** — one line appended to `~/.claude/metrics/skill-runs.jsonl`, outside any repo. See *Measuring cost* below + +**Lanes** — decided per PR before any expensive step: + +| Lane | When | What runs | +| ---- | ---- | --------- | +| `settled` | Merged/closed **and** no commits since your last review | Nothing. One row in the summary table, nothing posted. A merged PR already carrying your APPROVE never gets a second one | +| `delta` | Re-review with new commits, or a non-open PR that moved | Analysis scoped to `compare(lastReviewedSha, headSha)` — not the full PR diff | +| `full` | Open PR you have not reviewed before | The whole pipeline | + +The lane is why a third review round costs a fraction of the first: the rest of the PR was already reviewed, and re-reading it is what made every round cost the same. + +**Measuring cost:** + +```bash +# what a run cost (after the review; --prs makes batches comparable) +python3 .claude/skills/review-pr/scripts/skill-metrics.py record --skill review-pr --prs 3 --mode Standard + +# compare runs, grouped by the skill's git SHA +python3 .claude/skills/review-pr/scripts/skill-metrics.py report --skill review-pr + +# measure a skill EDIT without running a review at all +python3 .claude/skills/review-pr/scripts/skill-metrics.py footprint + +# reconstruct past runs from transcripts already on disk +python3 .claude/skills/review-pr/scripts/skill-metrics.py baseline --skill review-pr --all-projects +``` + +Claude Code tags every assistant message with `attributionSkill`, so the runtime meter reads real usage rather than estimating it. Sub-agent-internal tokens are **not** measured — sub-agents write no local transcript — so the meter records agent count, model and tool-result volume as proxies instead of inventing a number. Method and caveats: [hydra/.claude/skills/review-pr/references/metrics.md](https://github.com/ConductionNL/hydra/blob/main/.claude/skills/review-pr/references/metrics.md). + +The measurement data lives outside the repo on purpose (per machine, per operator, churns every run). The tooling is in the repo; the measurements are not. **Model:** Requires Sonnet or Opus — stops immediately on Haiku. Batch mode lets you choose the model for parallel analysis agents (Sonnet default, Opus for security-sensitive batches). diff --git a/docs/claude/skill-evals.md b/docs/claude/skill-evals.md index 9a891b74..c8c551c1 100644 --- a/docs/claude/skill-evals.md +++ b/docs/claude/skill-evals.md @@ -114,6 +114,38 @@ The previous `local-mods.patch` mechanism is gone — the merge replaces it. If --- +## Evals measure quality, not cost + +An eval suite answers "is the skill still good?". It does not answer "what does the +skill cost to run?" — and a change that makes a skill cheaper can quietly make it +worse, which is exactly the failure an eval exists to catch. + +Treat the two as a pair. `review-pr` carries the reference implementation: + +```bash +# cost, from real runs — reads Claude Code's own transcripts +python3 .claude/skills//scripts/skill-metrics.py record --skill +python3 .claude/skills//scripts/skill-metrics.py report --skill + +# cost of an EDIT, without running the skill at all +python3 .claude/skills//scripts/skill-metrics.py footprint --skill-dir .claude/skills/ +``` + +The meter reads `attributionSkill`, which Claude Code writes on every assistant +message, so it measures rather than estimates. It does **not** measure sub-agent +internals — sub-agents write no local transcript — and it records agent count, model +and tool-result volume as proxies instead of inventing a figure. + +**The rule: never accept a cost reduction without re-running the evals.** Record the +cost delta and the eval pass rate in the same commit message, so a later reader can +see both halves of the trade. + +⚠️ **`timing.json` in `workspace/` is not a cost baseline.** In the `review-pr` suite +several of these files are byte-identical across *different* evals — hand-entered, not +measured. Use the metrics store for cost; use the eval workspace for quality. + +See `hydra/.claude/skills/review-pr/references/metrics.md` for the full method. + ## `baseline_score` — Regression Detection Even running evals manually (no CI), `baseline_score` is useful: it's the with-skill pass rate from the most recent successful eval run, recorded in `evals/evals.json` next to `last_validated`. When you re-run evals later, compare the new pass rate against `baseline_score`: diff --git a/global-settings/tests/test-block-polling.sh b/global-settings/tests/test-block-polling.sh index 8a744ecb..578ea0a5 100755 --- a/global-settings/tests/test-block-polling.sh +++ b/global-settings/tests/test-block-polling.sh @@ -1,4 +1,5 @@ #!/usr/bin/env bash +# shellcheck disable=SC2016,SC2088 # literal $VAR / ~ tokens are intentional test fixtures fed to the hook as data # test-block-polling.sh — synthetic test harness for block-polling.sh. # # Feeds PreToolUse envelopes to the hook and asserts exit 2 (deny) or 0 (allow). diff --git a/hydra-gates/scripts/lib/test_gate_mainline_push_audits_everything.sh b/hydra-gates/scripts/lib/test_gate_mainline_push_audits_everything.sh index 7b37580d..5d3021f4 100755 --- a/hydra-gates/scripts/lib/test_gate_mainline_push_audits_everything.sh +++ b/hydra-gates/scripts/lib/test_gate_mainline_push_audits_everything.sh @@ -78,13 +78,23 @@ _out="${_tmp}/run.txt" ( cd "${_app}" || exit 1 # HYDRA_GATE_PUSH_BEFORE deliberately unset — this is the unresolvable case. + # GITHUB_EVENT_NAME=push is what makes it a PUSH whose tip is unresolvable, + # rather than a caller who simply passed a --base equal to HEAD. The runner + # distinguishes the two deliberately (see run-hydra-gates.sh, "the fallback + # is now reserved for the situation it was written for"), and only the push + # gets the full-tree fallback. Without this variable the test was asserting + # #183's guarantee against a context that never triggers it. unset HYDRA_GATE_PUSH_BEFORE - HYDRA_GATE_LOG_DIR="${_logs}" bash "${_runner}" \ + GITHUB_EVENT_NAME=push HYDRA_GATE_LOG_DIR="${_logs}" bash "${_runner}" \ --scope-to-diff --base development . > "${_out}" 2>&1 ) _rc=$? -_lines=$(grep -cE '^\[gate-[0-9]+\]' "${_out}" 2>/dev/null || echo 0) +# `grep -c` already prints 0 when it matches nothing AND exits 1, so a +# `|| echo 0` appends a SECOND zero and `[ "0\n0" -gt 0 ]` dies with +# "integer expression expected" — the assertion below then never ran. +_lines=$(grep -cE '^\[gate-[0-9]+\]' "${_out}" 2>/dev/null || true) +_lines=${_lines:-0} if [ "${_rc}" -eq 99 ]; then _bad "the runner exited 99 on a mainline push — it gated nothing (#183)" @@ -138,6 +148,40 @@ else _ok "a resolvable base is still scoped narrowly (the fallback did not swallow it)" fi +# THE OTHER HALF OF THE SAME DECISION. The full-tree fallback is reserved for a +# push whose previous tip is unresolvable. With NO push context at all, a --base +# that equals HEAD is a caller mistake with two honest readings, and the runner +# refuses rather than silently auditing everything. That refusal is deliberate +# and was previously untested — so removing it would have turned this file green +# by widening the fallback, which is the failure this whole suite exists to catch. +_out3="${_tmp}/run3.txt" +_logs3="${_tmp}/logs3" +mkdir -p "${_logs3}" +( + cd "${_app}" || exit 1 + unset HYDRA_GATE_PUSH_BEFORE + unset GITHUB_EVENT_NAME + HYDRA_GATE_LOG_DIR="${_logs3}" bash "${_runner}" \ + --scope-to-diff --base development . > "${_out3}" 2>&1 +) +_rc3=$? +# 99 alone is not enough: the runner exits 99 for setup failures too (no git, no +# app dir, unreadable tree), so asserting only the code would keep this green if +# the refusal were deleted and something else broke instead. Require the refusal +# to NAME itself. +if [ "${_rc3}" -eq 99 ] && grep -q "resolves to HEAD" "${_out3}"; then + _ok "no push context + base == HEAD is refused (99) and says why, not silently widened" +elif [ "${_rc3}" -eq 99 ]; then + _bad "exited 99 without the refusal message — that is a setup failure wearing the refusal's exit code" +else + _bad "base == HEAD with no push context exited ${_rc3} — the refusal is gone, and a caller mistake now reads as a full audit" +fi +if grep -q 'FULL-TREE AUDIT' "${_out3}"; then + _bad "the refusal path still fell back to a full-tree audit — the two cases are no longer distinguished" +else + _ok "the refusal names the mistake instead of substituting an audit nobody asked for" +fi + echo if [ "${_failures}" -eq 0 ]; then echo "test_gate_mainline_push_audits_everything.sh: ALL PASS"