Skip to content

Commit d757476

Browse files
leggetterleggetter
authored
Merge pull request #421 from hookdeck/docs/agent-review-skill
docs: add an agent skill for reviewing CLI changes
2 parents 5c6948c + dfc13fe commit d757476

2 files changed

Lines changed: 301 additions & 0 deletions

File tree

‎.agents/skills/hookdeck-cli-review/SKILL.md‎

Lines changed: 300 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,300 @@
1+
---
2+
name: hookdeck-cli-review
3+
description: >-
4+
Reviews changes to the Hookdeck CLI: why a green pull request still needs its
5+
acceptance run read, where an acceptance-test build tag must be registered,
6+
which generated files and wire contracts break silently, which docs move with
7+
the code, and what not to raise. Use when reviewing a pull request, a diff or
8+
a branch in hookdeck-cli, or when deciding whether a change is safe to merge.
9+
---
10+
11+
# Reviewing changes to the Hookdeck CLI
12+
13+
`AGENTS.md` is the reference for building, testing and linting this repository,
14+
and for the rules it already states. Read it first. This file is the review
15+
delta: what to look at in a diff, and the traps that a green pull request
16+
hides. Where a rule already lives in `AGENTS.md`, this points at it rather than
17+
restating it.
18+
19+
## Acceptance runs on the pull request, but nothing enforces it
20+
21+
`.github/workflows/test-acceptance.yml` triggers on every `pull_request`
22+
targeting `main` or `next`, with no path filter. So the acceptance suite — the
23+
three matrix slices plus the separate telemetry job — **does run on the pull
24+
request in front of you**. Assuming it did not is the wrong call.
25+
26+
What it is not is *required*. Branch protection on `main` requires exactly:
27+
28+
```
29+
unit-test build-linux build-mac build-windows
30+
```
31+
32+
Acceptance is not in that list. A pull request with a **red or skipped**
33+
acceptance run is still mergeable, and the merge button will not tell you.
34+
35+
That list is the one claim here with no source in the tree — it lives in
36+
GitHub's branch-protection settings, so it can change without a commit and
37+
without this file noticing. Re-read it rather than trusting it:
38+
39+
```
40+
gh api repos/hookdeck/hookdeck-cli/branches/main/protection \
41+
--jq '.required_status_checks.contexts'
42+
```
43+
44+
**So read `gh pr checks` and flag a red or skipped acceptance run**, because
45+
nothing else in the process will. That is the single most useful thing this
46+
review does that the required checks do not.
47+
48+
### A green acceptance job is not a run
49+
50+
A tick is not coverage either. An acceptance test that needs a credential skips
51+
itself when the job's `env:` block does not supply one, and a job whose tests
52+
all skipped still reports green. `gh pr checks` cannot show the difference.
53+
`acceptance-telemetry` is the live example — see *`CLITelemetry` JSON tags are a
54+
wire contract* below.
55+
56+
Three gates sit between an assertion existing and a red check, and all three
57+
have to hold: the file's build tag has to be in a slice list or the telemetry
58+
job's `-tags`; the test's own guards (`testing.Short()`, a `t.Skip` on a missing
59+
environment variable) have to pass; and the job's `env:` block has to supply
60+
what those guards need.
61+
62+
So when a diff touches behaviour an acceptance test covers, read the log rather
63+
than the colour:
64+
65+
```
66+
gh run view --job=<id> --log | grep -E -- '--- (SKIP|PASS): <TestName>'
67+
```
68+
69+
### The one case where it genuinely does not run
70+
71+
`pull_request` does not fire when a pull request's head branch is updated by
72+
**merging another pull request into it**. `test-acceptance.yml` states this in
73+
a comment above its triggers. A stacked branch — a release branch that collects
74+
other pull requests — can therefore reach `main` having never run acceptance at
75+
all, and `gh pr checks` shows that as an absence rather than as a failure.
76+
77+
The remedy is `workflow_dispatch`, which the same workflow accepts against any
78+
ref. One catch, also in that comment: GitHub reads the workflow definition from
79+
the ref you dispatch, so a branch can only be dispatched once it already
80+
carries the `workflow_dispatch` block. A branch cut before that block landed
81+
has to be rebased first.
82+
83+
Dispatch it on its own, though. `test/acceptance/README.md` § Rate limits
84+
explains why: a push to the branch also triggers the `pull_request` run, and
85+
two concurrent runs against the same projects exhaust the API rate limit and
86+
fail at the timeout with no assertion failures at all — which reads exactly
87+
like a code failure.
88+
89+
So "acceptance never ran" is a finding with an action attached, not just an
90+
observation.
91+
92+
### Why a reviewer should care, not just the release engineer
93+
94+
`.github/workflows/release.yml` calls the same acceptance workflow as a gate:
95+
`build-mac`, `build-linux` and `build-windows` each declare
96+
`needs: [acceptance]`, and `publish-npm` needs those three builds. A red
97+
acceptance therefore blocks the whole release pipeline.
98+
99+
Merging a red acceptance does not fail loudly at merge time. It fails at the
100+
next release tag, on someone else's change, and the bisect starts from the
101+
wrong commit.
102+
103+
### Running it yourself
104+
105+
`go test ./...` does not include the acceptance suite — see `AGENTS.md`
106+
§ Running `go test` after code changes. A local run needs real credentials in
107+
`test/acceptance/.env`, which is gitignored. Without them, "not verified" is the
108+
honest report, not a pass.
109+
110+
## A new acceptance tag has to be registered in five places
111+
112+
Every `*_test.go` under `test/acceptance/` carries exactly one feature build
113+
tag, and untagged files leak into every tag build — `AGENTS.md` § Acceptance
114+
tests and feature tags states that rule; do not repeat it in a review, check it.
115+
116+
`test/acceptance/README.md` § Parallelisation covers the rest of the mechanism:
117+
which tags are in which slice, that each slice has its own test account, and
118+
that telemetry runs separately. Read it rather than relying on this file.
119+
120+
What no file states is that the same set of tags is maintained **by hand in
121+
five places**, and they drift independently:
122+
123+
- `test/acceptance/run_parallel.sh` — `SLICE0_TAGS`, `SLICE1_TAGS`,
124+
`SLICE2_TAGS`, which is how the suite runs locally
125+
- `.github/workflows/acceptance.yml` — the `matrix.include` `tags` strings,
126+
which is how it runs in CI
127+
- `test/acceptance/README.md` § Running Tests — the "Run all automated tests"
128+
command, a single `-tags="..."` string that has to carry every tag
129+
- `test/acceptance/README.md` § Running Tests — the three per-slice commands
130+
- `test/acceptance/README.md` § Parallelisation — the `Slice N features` bullets
131+
132+
Registered in CI but not in the script, the tag never runs locally. In the
133+
script but not in CI, it never runs on a pull request. Left out of the "run all"
134+
command, the documented way to run everything quietly stops running everything —
135+
and that one is the worst of the five, because the command still exits 0.
136+
137+
All five agree today — but count before reporting drift, because **four of the
138+
five carry 20 tags, not 21.** `telemetry` is the twenty-first, and it is
139+
registered *separately* in each of those same files: `run_telemetry()` in
140+
`run_parallel.sh`, the `acceptance-telemetry` job in `acceptance.yml`, and its
141+
own `-tags=telemetry` command in the README. Only the "Run all automated tests"
142+
string carries all 21 in a single list.
143+
144+
So a slice list that reads 20 is correct, not stale. Check all five whenever a
145+
diff adds a tag — and if the new tag is a telemetry one, check those three
146+
separate sites instead.
147+
148+
One thing the README gets wrong, before you trust it on this: it attributes the
149+
matrix to `.github/workflows/test-acceptance.yml`. That file only declares the
150+
triggers and calls `.github/workflows/acceptance.yml`, which is where the slices
151+
and their tags actually live.
152+
153+
### The `manual` tag is a deliberate exception
154+
155+
`test/acceptance/project_use_manual_test.go` is `//go:build manual`, and
156+
`manual` appears in **none** of those five lists. That is intentional: the test
157+
needs an interactive browser login, which no CI job can perform, and
158+
`test/acceptance/README.md` gives it its own `-tags=manual` command instead.
159+
Applying the registration rule literally will report it as drift. It is not.
160+
161+
### Which slice a tag lands in is a decision, not bookkeeping
162+
163+
The README describes the slices; what it does not say is that choosing between
164+
them is a judgement call. They are not interchangeable buckets:
165+
166+
- a different test account per slice means a test that assumes resources
167+
created by another slice will not find them
168+
- slice 0 already carries 12 tags against a 12-minute timeout, while slice 1
169+
carries 2
170+
171+
Adding a slow tag to slice 0 is how the suite starts timing out. If a diff adds
172+
a tag, the slice it went into is worth a sentence.
173+
174+
The telemetry job is separate for a reason worth checking against a diff: it
175+
forces `HOOKDECK_CLI_TELEMETRY_DISABLED=0`, where the matrix jobs set it to `1`.
176+
A telemetry-tagged test added to a slice will not behave as intended.
177+
178+
## Credentials, and logs anyone can read
179+
180+
This repository is public, so **every acceptance log is world-readable**.
181+
`.github/workflows/acceptance.yml` says so itself, next to
182+
`HOOKDECK_CLI_TESTING_CLI_KEY`: that key is account-wide, so it has to belong to
183+
the test-only account rather than to a person — a key that reaches every org its
184+
owner belongs to must never be one a failing test can print. The per-slice
185+
`HOOKDECK_CLI_TESTING_API_KEY*` secrets are project-scoped, but they go to the
186+
same public log.
187+
188+
The question worth asking of a diff is therefore: **does this print or commit a
189+
credential?**
190+
191+
- a new `t.Logf`, `fmt.Print` or error string that interpolates a key, a config
192+
file or a whole HTTP request
193+
- a test fixture or `.env`-shaped file added to the tree
194+
- a redaction helper whose pattern is narrower than the data it guards
195+
196+
That last one has a concrete edge here. **The CLI writes single-quoted TOML.**
197+
Viper's writer emits:
198+
199+
```toml
200+
profile = 'default'
201+
202+
[default]
203+
api_key = 'cli_secret_abc123'
204+
```
205+
206+
A redaction regex written against `api_key = "..."` — double quotes, as in the
207+
`pkg/config/testdata/*.toml` fixtures — matches nothing against a config the CLI
208+
actually wrote, and the key goes to the log verbatim. Check redaction patterns
209+
against real output, not against a fixture.
210+
211+
## Things that compile, pass, and are still broken
212+
213+
Two surfaces in this repository have no test or CI job guarding them, so a green
214+
run means nothing about them. A third is guarded only in part — and both halves,
215+
the guarded one and the unguarded one, are dangerous in their own way.
216+
217+
### `REFERENCE.md` is generated
218+
219+
It is produced by `tools/generate-reference`, and the generated regions are
220+
fenced by `<!-- GENERATE... -->` / `<!-- GENERATE_END -->` markers.
221+
222+
- `go run ./tools/generate-reference --check` regenerates to a temp file, diffs,
223+
and **exits 1 when `REFERENCE.md` is stale**
224+
- **no CI job runs it.** Nothing catches a command-surface change that leaves
225+
the file behind
226+
227+
So run it yourself when a diff touches commands, flags or examples. And flag a
228+
hand-edit *inside* a `GENERATE` block: it is not wrong today, it is deleted on
229+
the next regeneration, silently.
230+
231+
### The website is a second consumer
232+
233+
The same generator feeds the docs website, and it emits positional arguments
234+
only when `Annotations["cli.arguments"]` is set on the command (`AGENTS.md`
235+
§ Cobra Example and output for website docs). A new command that takes
236+
positional arguments without that annotation documents them nowhere.
237+
238+
The review consequence: a command-surface change is not finished inside this
239+
repository. Say so, so the docs change is not discovered after release.
240+
241+
### `CLITelemetry` JSON tags are a wire contract
242+
243+
`pkg/hookdeck/telemetry.go` defines `CLITelemetry`, sent on every API request
244+
in the `X-Hookdeck-CLI-Telemetry` header. Its `json:"..."` tags —
245+
`command_path`, `invocation_id`, `command_flags`, `mcp_client` and the rest —
246+
are field names that Hookdeck's usage analytics query by name.
247+
248+
Whether a rename goes red depends on which tag it is, and the split is not
249+
uniform. Most are pinned by literal key name in `pkg/hookdeck/telemetry_test.go`,
250+
which marshals the struct into a `map[string]interface{}`; that file carries no
251+
build tag and no `testing.Short()` guard, so it runs under the **required**
252+
`unit-test` check and renaming one of those tags goes red. At least one is not:
253+
`command_flags` is asserted only by `TestTelemetryLoginCommandFlagsProxy` in
254+
`test/acceptance/telemetry_test.go`, which skips itself unless
255+
`HOOKDECK_CLI_TESTING_CLI_KEY` is set — and the `acceptance-telemetry` job does
256+
not set it. Renaming that one goes green and empties the dashboard.
257+
258+
Do not carry that split around in your head, and do not trust the sentence above
259+
to still be true: it rots every time a test moves. Derive it for the tag in
260+
front of you.
261+
262+
```
263+
grep -rn '<the json tag>' --include='*_test.go' .
264+
```
265+
266+
No hit means nothing guards it. Hits only under `test/acceptance/` mean check
267+
whether that test runs at all — see *A green acceptance job is not a run* above.
268+
269+
The trap, when it does go red, is the next move. The obvious way to get the
270+
build green again is to update the expected key in the test — which restores the
271+
signal while the dashboards built on the old name stay empty. **Treat a diff that edits both
272+
`telemetry.go` and its expected key names as a breaking change to a consumer
273+
outside this repository**, and say so explicitly. Adding a field is safe;
274+
renaming or removing one is not.
275+
276+
## Docs that move with the code
277+
278+
A change to a command's surface is incomplete without:
279+
280+
- **`REFERENCE.md`** — regenerated, per above
281+
- **Cobra `Short` and `Long`**, via the shared helpers in `pkg/cmd/helptext.go`
282+
rather than per-command literals (`AGENTS.md` § Command help text)
283+
- **`KNOWN_ISSUES.md`**, when the change *fixes* user-visible behaviour. The
284+
file documents each issue by symptom, cause, affected versions and
285+
recommended fix. A fix that ships while its entry still says "Affected:
286+
v2.5.0 and earlier" with no resolution tells users to keep working around
287+
something that no longer exists.
288+
289+
## What not to raise
290+
291+
- **A `CHANGELOG.md` entry.** The file states plainly that it is no longer
292+
maintained; release notes come from the GitHub Releases page and are drafted
293+
at release time. Asking for an entry asks for something the repository
294+
deliberately stopped doing.
295+
- **The `version` field in `package.json`.** It is machine-written:
296+
`release.yml` sets it from the tag and commits it during `publish-npm`. It is
297+
expected to lag the tag in the tree, and a human editing it by hand is noise,
298+
not a finding.
299+
- **Style the repository does not enforce**, or a refactor nobody asked for. A
300+
review that returns twenty comments buries the one that mattered.

‎AGENTS.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -596,6 +596,7 @@ Summary for code and docs work:
596596
- **Never copy a skill into a second location.** `.agents/skills/` and a root `skills/` briefly held byte-identical copies, so a fix applied to one left the other wrong (see #336). One canonical directory, symlinks everywhere else.
597597
- **Windows:** Git must create symlinks correctly (`core.symlinks` / Developer Mode). If symlinks are missing after clone, recreate them (`mklink /D` on Windows, or copy `.agents/skills/` into `.cursor/skills` and `.claude/skills` as a fallback).
598598
- **Releases:** For cutting GitHub releases, tags, npm/beta publish flow, and drafting release notes, use **`.agents/skills/hookdeck-cli-release/SKILL.md`**; human-facing steps remain in **README.md § Releasing**.
599+
- **Reviewing:** For reviewing a pull request, diff or branch, use **`.agents/skills/hookdeck-cli-review/SKILL.md`**: why a green pull request still needs its acceptance run read, where a new acceptance-test build tag has to be registered, which generated files and wire contracts break silently, which docs move with a command's surface, and what not to raise.
599600
600601
---
601602

0 commit comments

Comments
 (0)