From cae8251c432fda5d76afc0d2f5e93d7ad1197e89 Mon Sep 17 00:00:00 2001 From: WilcoLouwerse Date: Mon, 14 Sep 2026 19:09:11 +0200 Subject: [PATCH 1/5] docs(claude): review-pr lanes + cost metering, and correct the host policy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The /review-pr entry described a pipeline that no longer exists in that shape. Brings it in line with skill version 1.2.0: - Lanes (settled / delta / full) decided per PR before any diff, gate, guide, clone or sub-agent. This is the change that makes a third review round cost a fraction of the first, and it was undocumented. - Batch scope pre-check for a PR whose head already contains another's commits. - CI gate consumption pinned to the exact head SHA. - Per-mode sub-agent budgets, and Quick/delta PR grouping per repo. - Thread handling split by author: resolve your own, reply-and-hand-off on a co-reviewer's. The old text said "marks threads closed" without that split. - A "Measuring cost" section for scripts/skill-metrics.py, including what it does NOT measure (sub-agent internals) so nobody reads the number as total. Also corrects the host policy, which said Codeberg was primary and GitHub the fallback. The 2026-05-29 migration was reversed (directive 2026-07-17, executed 2026-07-23): GitHub is the host for all ConductionNL work and the Gitea/Forgejo path serves customer-run instances. That claim appeared in the shared header and in three per-command "Requires" lines — all four corrected, since the two outside the /review-pr section carry the identical stale sentence. skill-evals.md gains a section on the distinction the suite does not draw itself: evals measure quality, the metrics meter measures cost, and a cost reduction is not acceptable without re-running the evals. It also flags that the timing.json files in the review-pr eval workspace are byte-identical across different evals, so they are not a usable cost baseline. Co-Authored-By: Claude Opus 5 (1M context) --- docs/claude/commands.md | 59 +++++++++++++++++++++++++++++--------- docs/claude/skill-evals.md | 32 +++++++++++++++++++++ 2 files changed, 78 insertions(+), 13 deletions(-) diff --git a/docs/claude/commands.md b/docs/claude/commands.md index edd45221..03af36c1 100644 --- a/docs/claude/commands.md +++ b/docs/claude/commands.md @@ -2,7 +2,7 @@ Complete reference for all commands available in the spec-driven development workflow. Commands are organized by domain — click through to the detailed reference for each area. -> **Platform policy.** Commands that interact with a git host (`/create-pr`, `/review-pr`, `/report-out`, `/opsx-plan-to-issues`, `/opsx-apply`, `/opsx-verify`, `/opsx-archive`, etc.) auto-detect the per-repo platform from `git remote get-url origin` and prefer **Codeberg / Gitea / Forgejo** *(primary)* over **GitHub** *(secondary/fallback)* over **GitLab** *(alternative)*. The `gh` calls shown below are the github form; on Codeberg they shell out to `tea` (or REST `POST /api/v1/...` when the operation requires a TTY that `tea` needs). Conduction is migrating to `codeberg.org/Conduction/*` (2026-05-29) — the chain is bidirectional. See [hydra/.claude/skills/PLATFORM-POLICY.md](https://github.com/ConductionNL/hydra/blob/main/.claude/skills/PLATFORM-POLICY.md) for the canonical reference. +> **Platform policy.** Commands that interact with a git host (`/create-pr`, `/review-pr`, `/report-out`, `/opsx-plan-to-issues`, `/opsx-apply`, `/opsx-verify`, `/opsx-archive`, etc.) auto-detect the per-repo platform from `git remote get-url origin`. For **ConductionNL work the host is GitHub** — the 2026-05-29 move to Codeberg was reversed (directive 2026-07-17, executed 2026-07-23), so a `codeberg.org` URL found inside a repo is stale data to be fixed, not evidence of where that repo lives. The Codeberg / Gitea / Forgejo path is retained for **customer-run self-hosted instances** and uses REST `POST /api/v1/...` where `tea` would need a TTY; GitLab is the alternative for non-Conduction work. The chain stays bidirectional deliberately. See [hydra/.claude/skills/PLATFORM-POLICY.md](https://github.com/ConductionNL/hydra/blob/main/.claude/skills/PLATFORM-POLICY.md) for the canonical reference. ## OpenSpec Commands @@ -172,7 +172,7 @@ Create a Pull Request from a branch in any repo. Handles the full flow interacti **Model:** Checked at run time — the command reads your active model from context and stops automatically if you're on Haiku (or anything weaker than Sonnet). Involves parsing CI workflows, detecting branch-protection rules, and reasoning about code diffs where mistakes have real consequences. **Sonnet** for most PRs. **Opus** when the repo uses reusable CI workflows, branch-protection rulesets, or a complex branching strategy — that's where it pays off most. -**Requires:** at least one platform CLI authenticated — `tea login add` (Codeberg, primary), `gh auth login` (GitHub, fallback), or `glab auth login` (GitLab, alternative) +**Requires:** the platform CLI for the repo's host authenticated — `gh auth login` (GitHub; all ConductionNL work), `glab auth login` (GitLab), or `tea login add` (a customer's self-hosted Gitea/Forgejo) --- @@ -202,19 +202,52 @@ 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`. + +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). -**Requires:** at least one platform CLI authenticated — `tea login add` (Codeberg, primary), `gh auth login` (GitHub, fallback), or `glab auth login` (GitLab, alternative) +**Requires:** the platform CLI for the repo's host authenticated — `gh auth login` (GitHub; all ConductionNL work), `glab auth login` (GitLab), or `tea login add` (a customer's self-hosted Gitea/Forgejo) --- @@ -266,7 +299,7 @@ Daily end-of-day report. Scans local git repos for the user's commits and uncomm **Maturity:** L6 (9 evals, learnings.md with consolidation pipeline). See `hydra/.claude/skills/report-out/SKILL.md`. -**Requires:** at least one platform CLI authenticated — `tea login add` (Codeberg, primary), `gh auth login` (GitHub, fallback), or `glab auth login` (GitLab, alternative), `git` configured with `user.name` and `user.email`. +**Requires:** the platform CLI for the repo's host authenticated — `gh auth login` (GitHub; all ConductionNL work), `glab auth login` (GitLab), or `tea login add` (a customer's self-hosted Gitea/Forgejo), `git` configured with `user.name` and `user.email`. --- 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`: From b611e309ab17f09da3f1cbec9382875ff07b652d Mon Sep 17 00:00:00 2001 From: WilcoLouwerse Date: Tue, 15 Sep 2026 11:12:22 +0200 Subject: [PATCH 2/5] docs(claude): linkify the metrics.md cross-reference MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every other cross-repo pointer in commands.md is a markdown link to the hydra blob URL (the PLATFORM-POLICY.md and ADR-003 references a few lines up); this one rendered as inline code, so a reader could not follow it. Addresses the 🟢 inline on #772. Co-Authored-By: Claude Opus 5 (1M context) --- docs/claude/commands.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/claude/commands.md b/docs/claude/commands.md index 47df5225..692dc26d 100644 --- a/docs/claude/commands.md +++ b/docs/claude/commands.md @@ -241,7 +241,7 @@ python3 .claude/skills/review-pr/scripts/skill-metrics.py footprint 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`. +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. From 84675b9639e760d6ea461f00676af8319f1dbcfc Mon Sep 17 00:00:00 2001 From: WilcoLouwerse Date: Tue, 15 Sep 2026 16:31:17 +0200 Subject: [PATCH 3/5] fix(gates): the mainline-push test never set a push context MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit test_gate_mainline_push_audits_everything.sh asserted #183's guarantee — a push to a mainline branch audits everything rather than exiting 99 — but ran the runner with neither GITHUB_EVENT_NAME=push nor HYDRA_GATE_PUSH_BEFORE set. The runner distinguishes those two cases on purpose. A push whose previous tip is unresolvable gets the full-tree fallback; a caller who simply passed --base equal to HEAD gets a refusal, because that request has two honest readings and substituting a 22-minute audit for one of them reads exactly like the gates ignoring --base. The test was written before that narrowing and had been asserting the fallback against the context that never triggers it, so five assertions were red on main. Three changes: 1. The run under test now carries GITHUB_EVENT_NAME=push, which is what a mainline push actually looks like. Measured: exit 7, 101 gate lines, FULL-TREE AUDIT announced, coverage summary reached, gate-25 FAIL on the controller — #183's guarantee, intact. 2. `_lines=$(grep -c ... || echo 0)` emitted "0\n0", because grep -c prints 0 AND exits 1 on no match. `[ "0\n0" -gt 0 ]` then died with "integer expression expected" (line 95 in CI), so the assertion it guarded never ran at all. 3. Added the other half of the same decision: no push context + base == HEAD must be refused with 99 and must NOT fall back to a full-tree audit. That path was deliberate and untested — without it, deleting the refusal would have turned this file green by widening the fallback, which is the exact failure this suite exists to catch. 8 of 8 assertions pass locally. Co-Authored-By: Claude Opus 5 (1M context) --- ...st_gate_mainline_push_audits_everything.sh | 42 ++++++++++++++++++- 1 file changed, 40 insertions(+), 2 deletions(-) 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..c29782c2 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,34 @@ 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=$? +if [ "${_rc3}" -eq 99 ]; then + _ok "no push context + base == HEAD is refused (99), not silently widened" +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" From f3a85d7abdbc0081fa60ac462cf2bd8587536bd6 Mon Sep 17 00:00:00 2001 From: WilcoLouwerse Date: Tue, 15 Sep 2026 16:45:18 +0200 Subject: [PATCH 4/5] fix(global-settings): the polling test was missing its shellcheck directive MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ShellCheck failed the branch on test-block-polling.sh:76: expect allow 'until_date=2026-09-12; echo $until_date' SC2016: Expressions don't expand in single quotes, use double quotes The single quotes are the point. This harness feeds literal command strings to the hook as DATA, so an expanded $until_date would test something the hook never sees. Its two sibling harnesses — test-block-write-commands.sh and test-block-config-tool-writes.sh — already carry the same directive on line 2 with the same reason; the file added in #763 just missed it. No VERSION bump: this is a lint annotation on a test, with no behaviour change for anything consuming global-settings. Co-Authored-By: Claude Opus 5 (1M context) --- global-settings/tests/test-block-polling.sh | 1 + 1 file changed, 1 insertion(+) 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). From f1ceb68adf237a6a55035a3796ff788da4f1e8f5 Mon Sep 17 00:00:00 2001 From: WilcoLouwerse Date: Tue, 15 Sep 2026 16:47:02 +0200 Subject: [PATCH 5/5] test(gates): 99 alone is a weak assertion for the refusal path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Self-review of the previous commit: the new refusal assertion accepted any exit 99, and the runner exits 99 for setup failures too — no git, no app dir, unreadable tree. Delete the refusal and break something else in the same run, and this test would have stayed green. It now requires the refusal to name itself ("resolves to HEAD") and reports a bare 99 as "a setup failure wearing the refusal's exit code", which is a different defect from the one the assertion guards. 8 of 8 assertions still pass. Co-Authored-By: Claude Opus 5 (1M context) --- .../lib/test_gate_mainline_push_audits_everything.sh | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) 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 c29782c2..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 @@ -165,8 +165,14 @@ mkdir -p "${_logs3}" --scope-to-diff --base development . > "${_out3}" 2>&1 ) _rc3=$? -if [ "${_rc3}" -eq 99 ]; then - _ok "no push context + base == HEAD is refused (99), not silently widened" +# 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