Skip to content

Commit dfc13fe

Browse files
leggetterclaude
andcommitted
docs: replace the telemetry coverage list with the rule that derives it
The `CLITelemetry` section claimed `command_flags` was "pinned by the acceptance telemetry job". The assertion exists, but it never runs: TestTelemetryLoginCommandFlagsProxy skips unless HOOKDECK_CLI_TESTING_CLI_KEY is set, and the acceptance-telemetry job does not set it. Renaming that tag really does go green and empty the dashboard. Rather than correct the per-tag breakdown a third time, state the rule and hand over the grep that re-derives it for whichever tag is in the diff, mirroring how the branch-protection section already works. Adds a section on the failure mode that hid this: an acceptance job can be green because its tests skipped themselves for a missing credential, which `gh pr checks` cannot show. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent b2b2ed6 commit dfc13fe

1 file changed

Lines changed: 47 additions & 15 deletions

File tree

  • .agents/skills/hookdeck-cli-review

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

Lines changed: 47 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,27 @@ gh api repos/hookdeck/hookdeck-cli/branches/main/protection \
4545
nothing else in the process will. That is the single most useful thing this
4646
review does that the required checks do not.
4747

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+
4869
### The one case where it genuinely does not run
4970

5071
`pull_request` does not fire when a pull request's head branch is updated by
@@ -190,8 +211,8 @@ against real output, not against a fixture.
190211
## Things that compile, pass, and are still broken
191212

192213
Two surfaces in this repository have no test or CI job guarding them, so a green
193-
run means nothing about them. A third is guarded — and the guard is what makes it
194-
dangerous.
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.
195216

196217
### `REFERENCE.md` is generated
197218

@@ -224,19 +245,30 @@ in the `X-Hookdeck-CLI-Telemetry` header. Its `json:"..."` tags —
224245
`command_path`, `invocation_id`, `command_flags`, `mcp_client` and the rest —
225246
are field names that Hookdeck's usage analytics query by name.
226247

227-
A rename does **not** pass silently. `pkg/hookdeck/telemetry_test.go` marshals
228-
the struct into a `map[string]interface{}` and asserts by literal key name —
229-
`TestTelemetryJSONSerialization` covers six of the eight tags,
230-
`TestTelemetryJSONWithGeneratedResource` covers `generated_resource`. Neither
231-
carries a build tag or a `testing.Short()` guard, so both run under the
232-
**required** `unit-test` check. The eighth, `command_flags`, is pinned only by
233-
the acceptance telemetry job (`test/acceptance/telemetry_test.go`), and
234-
`AssertTelemetryConsistent` separately pins `command_path` and `invocation_id`
235-
on the wire.
236-
237-
The trap is the next move. The obvious way to get the build green again is to
238-
update the expected key in the test — which restores the signal while the
239-
dashboards built on the old name stay empty. **Treat a diff that edits both
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
240272
`telemetry.go` and its expected key names as a breaking change to a consumer
241273
outside this repository**, and say so explicitly. Adding a field is safe;
242274
renaming or removing one is not.

0 commit comments

Comments
 (0)