docs: add an agent skill for reviewing CLI changes

[leggetter]

What this adds

AGENTS.md covers how to build, test and lint this repository, thoroughly. Nothing in the repo covers what to look for in a diff — there's no CONTRIBUTING.md, no PR template, no CODEOWNERS, and neither existing skill is about review. So an agent asked to review a change here infers the standard from scratch each time.

This adds .agents/skills/hookdeck-cli-review/SKILL.md, alongside hookdeck-cli-release, plus one line in AGENTS.md § Agent skills. 300 lines; frontmatter is name and description only; nothing in it references a skill, role or repository a reader of hookdeck-cli cannot see.

Second review round: four findings, all addressed

A review of 9b7e068 found no false statements but four things to change. The commit was amended, so diff 9b7e068..2340843 (the force-push is in the timeline) to see only this round.

1. The tag rule was not a two-place rule — it's five. The file said a new acceptance tag has to be registered in run_parallel.sh and acceptance.yml. test/acceptance/README.md carries the same list in three more hand-maintained spots: the "Run all automated tests" command, the three per-slice commands, and the Slice N features bullets. All five are listed now. The section calls out the "Run all" one as the worst to miss, because the documented way to run everything then quietly stops running everything and still exits 0. I re-checked the sync claim by parsing //go:build out of every test/acceptance/*_test.go: 22 tags, 21 once manual is excluded, and the "Run all" string is exactly that set.

2. "The part no other file in the repository covers" was overstated, and is gone. test/acceptance/README.md § Parallelisation already documents the slice-to-tag mapping, the per-slice accounts and the separate telemetry job. The section now points at the README for the mechanism and claims only what is actually uncovered: that the five lists drift independently. The "Which slice a tag lands in" section was trimmed to the judgement it's there for — the 12-tags-against-a-12-minute-timeout imbalance — instead of restating the accounts.

Included in that fix: the README misattributes the matrix to .github/workflows/test-acceptance.yml. That file only declares triggers and calls acceptance.yml, which is where the slices live. The skill says so, since it sends readers to the README.

3. The one case where acceptance genuinely does not run was missing. test-acceptance.yml documents it in a comment above its triggers: pull_request doesn't fire when a PR's head branch is updated by merging another PR into it, so a stacked release branch can reach main having never run acceptance. workflow_dispatch is the remedy, with the catch that GitHub reads the workflow definition from the dispatched ref — so a branch cut before that block landed has to be rebased first. The file now says all of this. It was the "skipped" case the section tells you to flag, with no action attached.

I added one sentence the review didn't ask for: dispatch it on its own. test/acceptance/README.md § Rate limits warns that a push also triggers the pull_request run, and two concurrent runs exhaust the API rate limit and fail at the timeout with zero assertion failures. Advising a dispatch without that produces exactly that confusing failure.

4. Broken cross-reference fixed. The go test ./... exclusion was cited as AGENTS.md § Acceptance tests. No such heading exists — it's at AGENTS.md:291, under § Running go test after code changes. Corrected. I then re-checked all seven citations in the file by grepping the headings: AGENTS.md § Acceptance tests and feature tags, § Command help text, § Cobra Example and output for website docs, and test/acceptance/README.md § Running Tests, § Parallelisation, § Rate limits all resolve.

Third review round: two findings, both addressed

A review of 2340843 confirmed every claim in the file against the tree and returned two. Both are in dcf4a7c, a follow-up commit rather than an amend, so 2340843..dcf4a7c is exactly this round. (The task asked for an amend; the force-push was blocked by a tooling guard, and a separate commit keeps the reviewed commit readable anyway.)

1. "All five agree today, on 21 distinct tags" was wrong, and wrong in the direction that manufactures a false finding. telemetry is registered separately from the slice lists, so four of the five carry 20: SLICE0/1/2_TAGS in run_parallel.sh (telemetry is in run_telemetry()), matrix.include in acceptance.yml (telemetry is its own acceptance-telemetry job), the three per-slice README commands (there is a fourth, -tags=telemetry), and the Slice N features bullets (the fourth job is described separately at README:63). Only the "Run all automated tests" string carries all 21 in one list. A reviewer doing what the section told them to do would open run_parallel.sh, count 20, and report drift that is not there — the same failure the manual exception was added to prevent, one subsection earlier. The file now says so explicitly, and points a telemetry-tagged addition at the three separate sites instead.

The review scoped this to three of the five; it is four. The Slice N features bullets carry 12 + 2 + 6 = 20 as well.

2. The required-check list had no source in the tree. It is correct today and it is the one claim in the file that lives in GitHub's branch-protection settings rather than in a file, so it rots without a commit — in the section most likely to be trusted. It now carries the query to re-read it:

gh api repos/hookdeck/hookdeck-cli/branches/main/protection \
  --jq '.required_status_checks.contexts'

Run just now: ["unit-test","build-linux","build-mac","build-windows"] — unchanged.

Fourth review round: three findings, all addressed

A review of dcf4a7c returned three, one substantive. All are in b2b2ed6, so dcf4a7c..b2b2ed6 is exactly this round.

1. The CLITelemetry claim was false — the one kind of error this file can least afford. The section said a renamed JSON tag "compiles, passes unit tests, passes acceptance, and silently empties the dashboards". It does not. pkg/hookdeck/telemetry_test.go marshals the struct into a map[string]interface{} and asserts the tags by literal key name; neither test carries a build tag or a testing.Short() guard, and .github/workflows/test.yml runs go test -short ./pkg/..., so both run under the required unit-test check. Seven of the eight renames turn a required check red:

tag	asserted by	caught by
source, environment, command_path, invocation_id, device_name, mcp_client	TestTelemetryJSONSerialization	unit-test (required)
generated_resource	TestTelemetryJSONWithGeneratedResource	unit-test (required)
command_flags	test/acceptance/telemetry_test.go	acceptance-telemetry

The review attributed seven tags to TestTelemetryJSONSerialization; it is six. The generated_resource check in that test asserts absence (the field is omitempty and false there), so a rename still passes it — the positive assertion lives in TestTelemetryJSONWithGeneratedResource. Same conclusion, different test; the file names both.

The wire-contract point survives, and the trap is sharper than the original: the obvious way to get the build green again is to update the expected key in the test, which restores the signal while the dashboards built on the old name stay empty. The file now says to flag a diff that edits telemetry.go and its expected key names together.

2. "Three surfaces ... have no test or CI job guarding them" was two. Follows from the above. REFERENCE.md and the website generator are genuinely unguarded (grep -rn generate-reference .github/ → no hits); telemetry is guarded, and the guard is what makes it dangerous. The premise says so now.

3. The credential rule was attributed too broadly. The file said acceptance.yml is why "the test credentials" must belong to a test-only account. The comment there is scoped to HOOKDECK_CLI_TESTING_CLI_KEY, and its reason — "reaches every org its owner belongs to" — is a property of that account-wide key, not of the per-slice HOOKDECK_CLI_TESTING_API_KEY* secrets, which are project-scoped. Scoped to the account-wide key, with the world-readable log kept as the part that applies to all of them.

Also in this round: the line count above was stale (236 → 268), and the telemetry and credentials bullets below repeated the two corrected claims.

Fifth review round: three findings, all addressed

A review of b2b2ed6 returned three, one substantive. All are in dfc13fe, so b2b2ed6..dfc13fe is exactly this round.

1. command_flags is not pinned in CI — round four's correction was itself wrong. Round four replaced a false claim ("a rename silently empties the dashboards") with a narrower false one: that command_flags is "pinned only by the acceptance telemetry job". The assertion exists; it never runs. TestTelemetryLoginCommandFlagsProxy skips unless HOOKDECK_CLI_TESTING_CLI_KEY is set (test/acceptance/telemetry_test.go:84-87), and the acceptance-telemetry job's env: block supplies ACCEPTANCE_SLICE, HOOKDECK_CLI_TESTING_API_KEY and HOOKDECK_CLI_TELEMETRY_DISABLED — not that key (acceptance.yml:54-60). The matrix slices do set it (acceptance.yml:37) but do not build -tags=telemetry. Confirmed in this PR's own green run on b2b2ed6, job 106696045003:

$ gh run view --job=106696045003 --log | grep -E -- '--- SKIP:'
--- SKIP: TestTelemetryLoginProxy (0.00s)
--- SKIP: TestTelemetryLoginCommandFlagsProxy (0.00s)
... 6 more

So for that one tag, the scenario round four deleted as false is the truth: renaming it goes green and empties the dashboard.

I did not take the suggested sentence. The review offered a replacement that keeps the per-tag breakdown ("seven of the eight are caught that way; the eighth ..."). Three of the last four rounds' findings came out of that one enumeration, and it has now been wrong twice — the shape of the error is a coverage claim verified one level short of what it asserts, and a more precise enumeration gives it more places to be wrong. So the section now states the rule and hands over the command that re-derives it for whichever tag is in the diff, modelled on how the branch-protection section already handles a fact that rots:

grep -rn '<the json tag>' --include='*_test.go' .

No hit means nothing guards it; hits only under test/acceptance/ mean go and check whether that test runs. command_flags stays in as the worked example, flagged as a thing to re-derive rather than to trust. Re-run just now: command_flags → test/acceptance/telemetry_test.go only; the other seven tags → pkg/hookdeck/telemetry_test.go, all as positive require.Equal assertions on literal keys (telemetry_test.go:121-125,148-150,278).

2. A green acceptance job is not a run. The headline section stopped at job colour, and finding 1 is a third failure mode it could not see: a green job whose tests skipped themselves for a missing credential. New subsection A green acceptance job is not a run, with the three gates between an assertion existing and a red check — the file's build tag being in a slice list or the telemetry job's -tags, the test's own testing.Short() / missing-env t.Skip guards, and the job's env: block — plus the log grep. On b2b2ed6, acceptance-telemetry was green at 62 pass / 8 skip; slice 0 was 207 pass / 0 skip.

3. The "a third is guarded" premise inherited the defect. It now reads "guarded only in part", which is what the section below it says.

Not a finding, but someone should fix it: acceptance-telemetry not setting HOOKDECK_CLI_TESTING_CLI_KEY is a repo gap, not a PR gap — TestTelemetryLoginProxy and TestTelemetryLoginCommandFlagsProxy have never run in CI. One line in that job's env:, mirroring acceptance.yml:37. Out of scope for a docs PR, so it is filed separately as #422.

What the file covers

• Acceptance runs on every PR to main/next with no path filter, but is not a required check — branch protection requires exactly unit-test, build-linux, build-mac, build-windows — so a red or skipped run merges silently, and then fails at the next release tag, because release.yml gates its builds on the same acceptance workflow. Read gh pr checks; nothing else will.
• Five hand-maintained tag lists — four carrying the 20 slice tags, with telemetry registered separately in the same files and only the README "Run all" string carrying all 21 — plus //go:build manual as a deliberate exception that a literal reading would report as drift.
• REFERENCE.md is generated, --check exits 1 when stale, and no CI job runs it. Hand-edits inside GENERATE blocks are destroyed on the next regeneration.
• Credentials and world-readable logs. Every acceptance log is public; the account-wide HOOKDECK_CLI_TESTING_CLI_KEY is why the test account must not be a person's. The concrete edge: the CLI writes single-quoted TOML, so a redaction regex written against api_key = "..." — as the pkg/config/testdata fixtures are — leaks the key verbatim.
• CLITelemetry JSON tags are a wire contract. Whether a rename goes red depends on the tag — most are pinned by literal key in pkg/hookdeck/telemetry_test.go under the required unit-test check, but command_flags is asserted only by an acceptance test that CI never runs. The file gives the grep that settles it for a given tag rather than a breakdown to trust, and names the trap either way: fixing a red check by editing the expected key restores the signal and leaves the analytics built on the old name empty.
• The website is a second consumer of the reference generator and needs Annotations["cli.arguments"] for positional args.
• KNOWN_ISSUES.md moves with behaviour fixes.
• What not to raise: CHANGELOG.md entries, the machine-written package.json version, and unenforced style.

Deviations I'm keeping, unless told otherwise

• The telemetry section says "Hookdeck's usage analytics" rather than naming the vendor. This is a public repository and the reviewer-facing point lands without it.
• The release gate is described as transitive (build-* declare needs: [acceptance], publish-npm needs those builds) rather than as a direct needs: [acceptance] on publish-npm, because that's what release.yml actually says.

Verification

Re-run in this worktree on dfc13fe:

• go build ./... — clean, exit 0
• go vet ./... — clean, exit 0
• go test ./... — 17 packages ok, no failures
• go run ./tools/generate-reference --check — REFERENCE.md is up to date, exit 0

golangci-lint run reports 116 pre-existing issues (50 errcheck, 29 staticcheck, 29 unused, 7 govet, 1 ineffassign), all in Go files this branch does not touch — git diff origin/main --stat is one Markdown file plus one line in AGENTS.md. gofmt -l not re-run, same reason.

No tests added, and none apply — the change is documentation. The file's claims are what is under test, and round five's were re-derived from the tree rather than from the review write-up: the skip guard at test/acceptance/telemetry_test.go:84-87, the env: block at acceptance.yml:54-60, grep -rn command_flags --include='*.go' (two hits: that test and telemetry.go:38), each of the eight JSON tags grepped separately against *_test.go, and pkg/hookdeck/telemetry_test.go confirmed to carry no build tag and no testing.Short() against go test -short ./pkg/... in test.yml:32.

CI on b2b2ed6 — the previous round's run, now finished green. 12 checks pass, read per the file's own new advice rather than at job-colour level:

• acceptance / acceptance (0, ...) pass, 7m56s, 207 pass / 0 skip — the slice the file calls out as carrying 12 tags against a 12m timeout. 8m34s on dcf4a7c, 7m35s on 2340843; roughly four minutes of margin, on a branch that changes no Go code.
• acceptance / acceptance (1, request event) pass, 4m51s
• acceptance / acceptance (2, ...) pass, 3m35s
• acceptance / acceptance-telemetry pass, 3m2s — 62 pass / 8 skip, two of them the login tests that want HOOKDECK_CLI_TESTING_CLI_KEY. This is the green-is-not-a-run case, on this pull request.
• unit-test, build-linux, build-mac, build-windows, govulncheck, test-npm-build, test-homebrew-build — all pass

[code]smith and check-dependabot report skipping; neither is an acceptance job and neither is required.

CI on dfc13fe has not been read yet — it changes one Markdown file and no Go code, but per the section above, read it before merge rather than assuming.

Still worth challenging

Drafted from reading the repo, not from having maintained it. The judgement calls are the part to push on — particularly whether the slice-balance advice matches how you actually allocate tags, and whether the what-not-to-raise list is missing anything that wastes reviewer time today.

The five-list tag registration is arguably a finding about the repo rather than about review: if those lists were generated from one source, the section would shrink to a sentence. I didn't do that here — it's a change to the test tooling, not to a review skill.

Draft until someone who knows the CLI has read it.