Skip to content

Commit b2b2ed6

Browse files
leggetterclaude
andcommitted
docs: correct the telemetry claim, and scope the credential rule
Three review findings on the hookdeck-cli-review skill. The CLITelemetry 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 asserts the tags by literal key name against a marshalled map -- TestTelemetryJSONSerialization covers six of the eight, TestTelemetryJSONWithGeneratedResource covers generated_resource -- and neither carries a build tag or a testing.Short() guard, so both run under the required unit-test check. command_flags is pinned by the acceptance telemetry job. Seven of the eight renames turn a required check red. The wire-contract point survives, but the trap is the next move, not the rename: the obvious way to get the build green is to update the expected key in the test, which restores the signal while the dashboards built on the old name stay empty. A diff that edits telemetry.go and its expected key names together is the thing to flag. That also fixes the section premise. Two surfaces here are unguarded -- REFERENCE.md and the website generator -- not three. The credentials section attributed the test-only-account rule to "the test credentials" generally. The comment in acceptance.yml is scoped to HOOKDECK_CLI_TESTING_CLI_KEY, and its reason -- a key that reaches every org its owner belongs to -- is a property of that account-wide key, not of the per-slice project-scoped secrets. Scoped, with the world-readable log kept as the part that applies to all of them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent dcf4a7c commit b2b2ed6

1 file changed

Lines changed: 25 additions & 10 deletions

File tree

  • .agents/skills/hookdeck-cli-review

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

Lines changed: 25 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -157,10 +157,12 @@ A telemetry-tagged test added to a slice will not behave as intended.
157157
## Credentials, and logs anyone can read
158158

159159
This repository is public, so **every acceptance log is world-readable**.
160-
`.github/workflows/acceptance.yml` says so itself, which is why the test
161-
credentials have to belong to a test-only account rather than to a person: a key
162-
that reaches every org its owner belongs to must never be one a failing test can
163-
print.
160+
`.github/workflows/acceptance.yml` says so itself, next to
161+
`HOOKDECK_CLI_TESTING_CLI_KEY`: that key is account-wide, so it has to belong to
162+
the test-only account rather than to a person — a key that reaches every org its
163+
owner belongs to must never be one a failing test can print. The per-slice
164+
`HOOKDECK_CLI_TESTING_API_KEY*` secrets are project-scoped, but they go to the
165+
same public log.
164166

165167
The question worth asking of a diff is therefore: **does this print or commit a
166168
credential?**
@@ -187,8 +189,9 @@ against real output, not against a fixture.
187189

188190
## Things that compile, pass, and are still broken
189191

190-
Three surfaces in this repository have no test or CI job guarding them, so a
191-
green run means nothing about them.
192+
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.
192195

193196
### `REFERENCE.md` is generated
194197

@@ -221,10 +224,22 @@ in the `X-Hookdeck-CLI-Telemetry` header. Its `json:"..."` tags —
221224
`command_path`, `invocation_id`, `command_flags`, `mcp_client` and the rest —
222225
are field names that Hookdeck's usage analytics query by name.
223226

224-
Renaming a field compiles, passes unit tests, passes acceptance, and silently
225-
empties the dashboards built on the old name. Treat a change to those tags as a
226-
breaking change to a consumer outside this repository, and say so explicitly.
227-
Adding a field is safe; renaming or removing one is not.
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
240+
`telemetry.go` and its expected key names as a breaking change to a consumer
241+
outside this repository**, and say so explicitly. Adding a field is safe;
242+
renaming or removing one is not.
228243

229244
## Docs that move with the code
230245

0 commit comments

Comments
 (0)