docs(claude): review-pr lanes + cost metering, and correct the host policy

[WilcoLouwerse]

Companion to hydra#671, which changed how /review-pr works. These docs described the previous shape.

/review-pr (commands.md)

The step list was rewritten. The substantive additions:

• Lanes — every PR is classified settled / delta / full before any diff, gate, guide, clone or sub-agent runs. A merged PR already carrying your APPROVE with no new commits does no work at all and never gets a second APPROVE. This is what makes a third review round cost a fraction of the first, and it was entirely undocumented.
• Batch scope pre-check — detects a PR whose head already contains another's commits (a retargeted stacked branch) before staging anything, and reads baseRefName from the API rather than the PR body.
• CI gate consumption — a green gate job pinned to the exact head SHA is taken as evidence instead of re-running it locally. A red one still gets a local run.
• Per-mode sub-agent budgets and Quick/delta grouping per repo.
• Thread handling split by author — the old text said the skill "marks threads closed". It now resolves only threads you opened and posts an acknowledgment reply on a co-reviewer's, leaving theirs for them to close.
• Measuring cost — the skill-metrics.py commands, plus an explicit note on what the meter does not cover (sub-agent internals), so the number is not read as a total.

skill-evals.md

New section on a distinction the suite does not draw for itself: evals measure quality, the metrics meter measures cost, and a change that makes a skill cheaper can quietly make it worse. The rule stated there is that a cost reduction is not acceptable without re-running the evals, with both numbers recorded in the same commit message.

It also flags that several timing.json files in the review-pr eval workspace are byte-identical across different evals — hand-entered rather than measured — so they cannot serve as a cost baseline. That is part of why a transcript-derived meter was needed.

Note on the host-policy edit (dropped)

This branch originally also corrected the shared platform-policy paragraph and three per-command Requires lines, which still claimed Codeberg was primary and GitHub the fallback.

main landed the same correction first (ac9437e), with its own wording. The merge conflict was resolved by taking main on all four — my phrasing added nothing, and two different sentences for one fact is worse than one. The net diff here is now only the /review-pr section and the new skill-evals.md section.

Review notes

Documentation only. One judgement call worth a second opinion: whether the Lanes table belongs in commands.md or is too much implementation detail for a command reference.

🤖 Generated with Claude Code

[WilcoLouwerse]

Self-review (Quick). 1 🟢 minor. Verdict follows separately.

[WilcoLouwerse]

Verdict: APPROVE (Quick) — self-review posted as COMMENT (GitHub blocks self-APPROVE)

No blockers, one 🟢 styling nit inline. Accurate, well-scoped docs PR.

Every load-bearing claim was checked against hydra#671's source at c99d569 rather than taken on trust, and all of them held:

• The lane table (settled/delta/full) and "never a second APPROVE" match SKILL.md.
• CI-gate consumption really does require both an exact head-SHA match and a green conclusion, and a red check really does still get a local run (SKILL.md:284-290).
• The sub-agent grouping cap ("at most 4 per agent") and its exclusions — full lane, Thorough/Strict, security-sensitive — match SKILL.md:393-402.
• The thread author-split matches SKILL.md:513-534.
• All four skill-metrics.py subcommands and their flags exist verbatim in the script's argparse, and the default store path ~/.claude/metrics/skill-runs.jsonl is correct.

The timing.json claim in the new skill-evals.md section is the one I most expected to be overstated, and it isn't — two different evals share an identical blob SHA:

eval-line-number-accuracy/with_skill/run-1/timing.json
eval-no-duplicate-comments/with_skill/run-1/timing.json
  → both f15b95928b5c92de8fadc41aaedfd9ed6b36f89b

The host-policy account also checks out: the net diff contains no Codeberg/platform-policy edit, and commands.md at head never claims Codeberg is primary.

On your open question — whether the Lanes table belongs in commands.md or is too much implementation detail for a command reference: keep it. Lanes change what a user gets (a settled PR is silently skipped), not just how the skill works internally. Someone who doesn't know the rule will read a six-of-nine-skipped batch as a bug. That is user-facing behaviour, not implementation detail.

Note this PR is behind_by=5 against main.

[WilcoLouwerse]

Re-review (Quick, delta). 1 🟡 inline on the link this round introduced. Verdict follows separately.

[WilcoLouwerse]

🟡 This link 404s today — metrics.md does not exist on hydra main yet

The previous round's 🟢 asked for this pointer to be a markdown link rather than backtick text, and it now is. But the target only ships with hydra#671, which is still open against development — so the URL resolves on neither branch:

$ gh api repos/ConductionNL/hydra/contents/.claude/skills/review-pr/references/metrics.md?ref=main
404 Not Found
$ gh api repos/ConductionNL/hydra/contents/.claude/skills/review-pr/references/metrics.md?ref=development
404 Not Found

The two sibling cross-repo links in this same file do resolve on main today, which is what makes this one inconsistent rather than merely early:

.claude/skills/PLATFORM-POLICY.md         OK
openspec/architecture/adr-003-backend.md  OK

So the fix traded unfollowable but honest for followable but broken. That is a real regression for anyone who clicks it before the chain lands.

Not a blocker: the whole Measuring-cost section documents behaviour that ships in #671, so this doc is inherently ahead of main and self-corrects once #671 merges to development and development promotes to main. Leaving the link as-is deliberately, with this comment as the record of the merge-order dependency — #671 must land on main before this link works. If #671 is ever abandoned or retargeted, this line needs to go back to backticks.

[WilcoLouwerse]

Verdict: APPROVE (Quick, re-review) — posted as COMMENT (GitHub blocks self-APPROVE)

The previous round's 🟢 is addressed — the metrics.md pointer is now a markdown link in the same shape as the two sibling cross-repo links in this file.

One 🟡 inline, and it is one the fix itself introduced: the link target does not exist on hydra main or development yet, because metrics.md ships with hydra#671, which is still open. It 404s today and self-corrects once #671 lands on main. Deliberately left as-is per the inline comment, which is the record of that merge-order dependency.

Not a blocker in Quick mode, and the section it sits in documents #671's behaviour throughout — this doc is inherently ahead of main.

On the failing required check — Package invariants is red here, and it is not this PR's doing. This diff is two docs files; the failure is test_gate_mainline_push_audits_everything.sh inside the hydra-gates package, which also fails on main:

$ gh api "repos/ConductionNL/.github/actions/runs?branch=main&per_page=10" \
    --jq '.workflow_runs[] | "\(.name) | \(.conclusion) | \(.head_sha[0:7])"'
hydra-gates package | failure | d192db3

The suite itself reports a bash defect in the test, not in the runner: line 95: [: 0 0: integer expression expected. That needs fixing on main; it is a merge gate on this PR only incidentally.

Note this PR is behind_by its base — worth an Update branch before anyone approves, since a ConductionNL automation dismisses approvals on a PR that is behind.

[WilcoLouwerse]

Verdict: APPROVE (Quick, re-review) — posted as COMMENT (GitHub blocks self-APPROVE)

Two failing checks cleared since the last round, neither of them caused by this PR's docs content.

Package invariants — the mainline-push test was asserting against a context that never triggers. test_gate_mainline_push_audits_everything.sh ran the runner with neither GITHUB_EVENT_NAME=push nor HYDRA_GATE_PUSH_BEFORE set. The runner distinguishes those two cases deliberately — a push whose previous tip is unresolvable gets the full-tree fallback; a caller who passed --base equal to HEAD gets a refusal — and the test predates that narrowing. Reproduced both sides before touching anything:

$ (unset HYDRA_GATE_PUSH_BEFORE; run-hydra-gates.sh --scope-to-diff --base development .)
EXIT=99   gate lines: 0

$ (unset HYDRA_GATE_PUSH_BEFORE; GITHUB_EVENT_NAME=push run-hydra-gates.sh --scope-to-diff --base development .)
EXIT=7    gate lines: 101   FULL-TREE AUDIT announced   gate-25: FAIL on the controller

So #183's guarantee was intact the whole time; the test was pointing at the wrong context. Also fixed the bash defect CI was printing — _lines=$(grep -c … || echo 0) emits "0\n0", because grep -c prints 0 and exits 1, so [ "0\n0" -gt 0 ] died with "integer expression expected" and the assertion it guarded never ran. 8 of 8 assertions pass locally.

ShellCheck — SC2016 on global-settings/tests/test-block-polling.sh:76. The single quotes are the point: the 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 already carry the identical directive on line 2; the file added in #763 just missed it. No VERSION bump — a lint annotation on a test changes nothing for anything consuming global-settings.

🟡 One finding in my own first attempt, fixed in f1ceb68

The refusal assertion I added accepted any exit 99. The runner also exits 99 on setup failures — no git, no app dir, unreadable tree — so deleting the refusal while breaking something else in the same run would have left this test green. That is the precise failure mode this suite exists to catch, reintroduced by the fix for it. It now requires the refusal to name itself and reports a bare 99 separately.

On scope

This PR is now two docs files plus two CI fixes, which is wider than the repo's own one-PR-per-coherent-change guidance. Deliberate: the gates test was the required check blocking this PR, and splitting it would have left this one red while the split PR waited. Worth splitting if you would rather land them separately.

The merge-order note from the previous round still stands: the metrics.md link 404s until hydra#671's content reaches hydra main.