fix: delivery-group data loss and metrics defects, plus the missing acceptance coverage - #392
Merged
Merged
Conversation
Two defects found by exercising a release-candidate build against the API. A CLI destination carries no delivery_policy in the API schema. Passing --rate-limit or --delivery-group-* with --type CLI was accepted, sent, and discarded server-side: exit 0, and a read-back showing no policy at all. The flags looked applied and never took effect. Reject the combination instead, naming the --destination- prefixed spelling when it came from a connection. Separately, --cli-path is declared with a default of "/" on create (but "" on update and upsert), so the "explicit flag wins" branch fired even when the user never passed it — overwriting a path supplied via --config. Compare against cobra's Changed rather than "", so the advertised default stays in --help and only the --config case changes. The path defaulting is now one helper shared by create and upsert, since the asymmetry between them is what produced the bug. Verified on the wire by pointing the build at a local capture server via --api-base and reading the request bytes, rather than inferring from read-backs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BnrKWZQASV7bFJ4oGWwmo9
Three defects found by exercising a release-candidate build against the API. --measures pending only routed to the pending-timeseries endpoint when --granularity was also set; otherwise it fell through to the default events route, which rejects the measure. The API treats granularity as optional on that route, so gating the routing on it was wrong. Route on the measure alone. --measures queue_depth is advertised in --help but is not in the endpoint's enum, which accepts max_depth and max_age only, so it could never succeed. Translate it to max_depth on the wire, mirroring the existing pending -> count translation, so the documented flag works. (If the intent was to drop the spelling instead, removing it from the measures list is the one-line alternative.) --dimensions and --status advertised one generic vocabulary on all four subcommands, and it is wrong on three: passing a suggested dimension returned a raw 422. Each route now advertises the set it accepts, alongside the filter matrix that already gates which flags exist. The metrics example in `gateway --help` was also missing the required --measures, so copying it verbatim failed. Vocabularies cross-checked against the API's OpenAPI document per route. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BnrKWZQASV7bFJ4oGWwmo9
Four defects the review caught in the first two commits, plus tests that actually pin the fixes. connection upsert has a second destination-building path, taken when the connection already exists, which built a delivery policy with no CLI guard. The bug this PR set out to fix was still reachable through it. The status vocabulary added for --help was itself wrong. Events accept SCHEDULED, QUEUED, HOLD, SUCCESSFUL, FAILED and CANCELLED; the constant advertised PAUSED, which the API rejects, and omitted three real values. The repo already had the right list in event_list.go and the MCP tools. Attempts accept SUCCESSFUL and FAILED only, so they now get their own constant rather than sharing the events one — a single shared vocabulary across routes with different enums is the defect this commit series is meant to remove. Transformations register no --status flag at all, so they are passed none. Verified against the live API: all six event values query, PAUSED 422s. The MCP layer carried both metrics bugs the CLI had just fixed, contradicting the "shared so they cannot drift apart" comment above the filter matrix. The queue-depth translation moves to pkg/hookdeck and both callers use it, and the granularity gate is gone from the MCP routing too. Also: metrics events printed two conflicting dimension lists on one help page, so the local constant now derives from the shared one; --measures is marked required, since every endpoint rejects a request without it and MCP already enforced it; and connection upsert no longer resets an existing CLI destination's path to "/", which its own comment said it should not do. Every fix in this PR was checked by reverting it and confirming the relevant test fails. Five previously had no such coverage. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BnrKWZQASV7bFJ4oGWwmo9
Collaborator
Author
Review round 1 addressedA coding agent reviewed this PR and found four must-fix defects, three of them in the fixes themselves. All are now fixed in 6827c44, plus the test gap it identified. Must-fix
Also fixed
The test gapThe review reverted each fix and re-ran the suite. Five of them still passed — the tests pinned pure helpers, not behaviour anyone can observe. Now corrected: the routing tests drive Not changedThe reviewer noted Verification: Review and fixes by Claude, on Phil's behalf. |
Closes #393. Bumping a delivery group's rate wiped the per-group overrides, silently. The API merges delivery_policy one level deep but replaces groups wholesale, so a groups object sent without overrides takes the stored ones with it. The CLI requires --delivery-group-key and --delivery-group-rate-period whenever --delivery-group-rate is given, so "just change the rate" always sends a full groups object -- the one command a user would reach for was the one that lost data, with no warning and nothing in --dry-run to reveal it. Carry the stored overrides forward when the caller did not supply their own. An explicit --delivery-group-overrides still wins, and '{}' still clears, so deliberately emptying them is unaffected. Three paths needed it: destination upsert, which now fetches the existing destination when a groups object would otherwise go out bare; and both connection upsert paths, which already hold the existing destination and so cost no extra request. This is a read-modify-write and races a concurrent edit of the same destination, which is the same exposure upsert already had for every other field it preserves. Verified against the live API: created a destination carrying overrides, bumped the group rate, and confirmed the overrides survived and the rate changed. The pre-fix binary run against the same destination drops them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BnrKWZQASV7bFJ4oGWwmo9
Collaborator
Author
Added: the #393 data-loss fix (f05ca9e)Scope change — this now also fixes #393, the Stored overrides are carried forward when the caller doesn't supply their own. Explicit Verified live, A/B on the same destination: fixed binary preserves the overrides while bumping the rate; the pre-fix binary drops them. Known caveat, recorded on #393: this is a read-modify-write and races a concurrent edit — the same exposure Note on CI for this PRNone of the normal checks run here — (Fixes and verification by Claude, on Phil's behalf.) |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Several update paths can still discard policies or stored configuration, and mixed metrics can fail or silently omit requested measures.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Fixes release-candidate defects in destination configuration and metrics routing. It also implements #393 override preservation despite the PR description stating it was deferred.
Changes:
- Validates CLI destination policies and preserves CLI paths.
- Preserves delivery-group overrides during upserts.
- Corrects metrics routing, translations, help text, and tests.
File summaries
| File | Description |
|---|---|
REFERENCE.md |
Adds the required metrics measure to the example. |
pkg/hookdeck/metrics_filters.go |
Adds route vocabularies and queue-depth translation. |
pkg/gateway/mcp/tool_metrics.go |
Updates MCP event metrics routing. |
pkg/cmd/metrics.go |
Adds route-specific help and requires measures. |
pkg/cmd/metrics_transformations.go |
Supplies transformation dimensions. |
pkg/cmd/metrics_requests.go |
Supplies request dimensions and statuses. |
pkg/cmd/metrics_events.go |
Fixes pending and queue-depth routing. |
pkg/cmd/metrics_events_routing_test.go |
Tests event routing and translation. |
pkg/cmd/metrics_attempts.go |
Supplies attempt dimensions and statuses. |
pkg/cmd/gateway.go |
Corrects the gateway metrics example. |
pkg/cmd/destination_upsert.go |
Preserves paths and delivery-group overrides. |
pkg/cmd/destination_create.go |
Prevents config paths from being overwritten. |
pkg/cmd/destination_common.go |
Adds shared validation and preservation helpers. |
pkg/cmd/destination_cli_type_test.go |
Tests CLI destination behavior. |
pkg/cmd/delivery_group_overrides_test.go |
Tests override preservation. |
pkg/cmd/connection_upsert.go |
Preserves destination settings during upserts. |
pkg/cmd/connection_create.go |
Rejects CLI delivery policies. |
Review details
Suppressed comments (1)
pkg/cmd/connection_upsert.go:488
- This still misses full declarative upserts: when both destination name and type are supplied, the earlier
needsExistingexpression stays false, so an existing connection reaches here withisUpdate == false. The stored destination is therefore unavailable and a group-rate update still sends no overrides, reproducing the silent data loss. Ensure any groups update without explicit overrides fetches the existing connection first.
if isUpdate && existing != nil && existing.Destination != nil {
preserveDeliveryGroupOverrides(destinationInput.Config, existing.Destination.Config)
- Files reviewed: 17/17 changed files
- Comments generated: 7
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+85
to
+87
| // Granularity is optional on this route, so it must not gate the routing: | ||
| // gating it sent "pending" to the default endpoint, which rejects the measure. | ||
| if hasMeasure(params, map[string]bool{"pending": true}) { |
| queueParams := params | ||
| queueParams.Measures = hookdeck.TranslateQueueDepthMeasures(params.Measures) | ||
| result, err = client.QueryQueueDepth(ctx, queueParams) | ||
| case containsAny(params.Measures, "pending"): |
Comment on lines
+475
to
+477
| existingCLIDest := isUpdate && existing != nil && existing.Destination != nil && | ||
| strings.ToUpper(existing.Destination.Type) == "CLI" | ||
| if strings.ToUpper(cu.destinationType) == "CLI" && cu.destinationCliPath == "" && !existingCLIDest { |
Comment on lines
+326
to
+327
| if err := rejectDeliveryPolicyForCLI(destType, policy, ""); err != nil { | ||
| return nil, err |
Comment on lines
135
to
140
| if req.Config == nil || len(req.Config) == 0 || needsOverrides { | ||
| params := map[string]string{"name": dc.name} | ||
| listResp, err := client.ListDestinations(ctx, params) | ||
| if err == nil && listResp.Models != nil && len(listResp.Models) > 0 { | ||
| existing, err := client.GetDestination(ctx, listResp.Models[0].ID, nil) | ||
| if err == nil && existing.Config != nil { |
Comment on lines
+117
to
+121
| if seen[m] { | ||
| continue | ||
| } | ||
| seen[m] = true | ||
| out = append(out, m) |
Comment on lines
+132
to
+134
| // A groups object sent without overrides also needs the stored config, because | ||
| // the API replaces groups wholesale and would drop the overrides with it. | ||
| needsOverrides := deliveryGroupsNeedOverrides(req.Config) |
Five pkg/ defects and the acceptance gap that let them through. Copilot's four findings, plus a fifth the new acceptance tests caught: 1. Mixed measures across routes silently dropped data. Routing on "pending" alone also captured --measures pending,failed_count, and the pending branch replaces the whole measure list with count -- exit 0, failed_count gone. RejectMixedMeasureRoutes now refuses the combination, shared by the CLI and MCP so the two cannot drift. 2. The CLI delivery-policy guard missed the common form. update and upsert normally omit --type, so destType was "" and the guard returned nil while the API silently discarded the policy. The stored type is now resolved, with the lookup skipped whenever it cannot change the outcome. 3. Overrides preservation failed open. A transient lookup error left a bare groups object going out, reintroducing the #393 data loss through the error path. It now refuses rather than proceeding. 4. The CLI-path fix did not fire for --destination-name plus --destination-type CLI, the ordinary idempotent form, because the existence lookup was skipped. Suppressing the default alone would have sent path:"" instead, trading one silent clobber for another, so the unconditional assignment went too. 5. destination update wiped delivery group overrides -- the same #393 loss on a third command, which neither the unit tests nor two code reviews caught. It reuses the lookup the type resolution already performs. Acceptance coverage: delivery groups had none at all, on the feature this release exists for. 13 test functions and 28 subtests now cover the standalone and inline forms, CLI-type rejection, partial and invalid flag sets, and -- the case that matters -- that bumping a group rate preserves the stored overrides on update, upsert and connection upsert. Defect 5 was found by writing them. Also adds the two metrics cases whose absence let real bugs ship: --measures pending with no --granularity, and --measures queue_depth. The existing tests were written around the broken behaviour, passing --granularity 1h and using max_depth, so they could never have failed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BnrKWZQASV7bFJ4oGWwmo9
leggetter
changed the title
Every gateway MCP response carried meta.active_project_name: "" whenever the CLI was authenticated with a project-scoped credential (hookdeck ci keys, dashboard/single-project API keys). The profile on disk stores only project_id, so fillProjectDisplayNameIfNeeded has to recover the name from the API, and its only source was GET /projects — which 403s for those credentials. The error was swallowed, so the meta block an LLM client shows the user had no readable project name, including before the pause/unpause write actions. Fall back to /cli-auth/validate, which does return team_name_no_org and organization_name for those keys (this is where whoami gets them). Its names are only applied when the key's project matches the active project id, mirroring resolveActiveProject in whoami, so the meta block can never name the wrong project. Refs #405 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BnrKWZQASV7bFJ4oGWwmo9
This was referenced Sep 14, 2026
Two ways `hookdeck login` failed a caller with no terminal, both in pkg/login. #400: with an EMPTY config, `hookdeck login </dev/null` printed "Press Enter to open the browser", read EOF instantly, opened a real browser window on the user's desktop, and then polled forever. Killed at 60s with no sign of stopping. The guard added in 9ff211a mirrors the browser branch of waitForLoginSession, but it sits inside `if config.Profile.APIKey != ""`. An empty config skips that block entirely, so the fresh-login path reached waitForLoginSession unguarded - the branch was known about and only the rejected-key case was covered. The same condition now runs before StartLogin, so no session is created either: no saved credentials, and browser sign-in needs an interactive terminal; use hookdeck ci --api-key with a project API key, hookdeck login --cli-key with a CLI key, or set HOOKDECK_API_KEY to a project API key The `isSSH() || !canOpenBrowser()` branch prints a URL and polls without reading stdin. That works headlessly and is still allowed; the shared condition is now browserSignInNeedsStdin() rather than two copies of it. The Enter prompt also had no trailing newline, so it ran straight into "Waiting for confirmation...". Fixed. Its "^C to quit" is now true rather than aspirational, because the branch is only reachable with a terminal. #401: `hookdeck login -i </dev/null` printed "Enter your CLI API key: " and then "operation not supported by device" - term.GetState's termios error, surfaced verbatim. It exited 1 immediately, so only the message was wrong. It now refuses before printing a prompt nobody can answer, and names the same ways in. Neither of these is a regression. #337's "unauthenticated commands no longer hang" is resolveAuthFallback in pkg/cmd/root.go, which covers a command that fails for want of credentials and would drop into login. Typing `hookdeck login` never went through it, so the claim was true as scoped and never covered this. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BnrKWZQASV7bFJ4oGWwmo9
…n-in URL #373. The third copy of the Enter-then-browser branch, in waitForGuestUpgrade. A guest profile whose key still validates skipped every guard already added, so `hookdeck login </dev/null` printed the Enter prompt, read EOF, opened a browser window unasked, and then polled for four minutes (120 attempts, 2s apart). Same browserSignInNeedsStdin() guard as #400, placed before RefreshGuestSigninLink so no sign-up link is minted for a flow nobody can finish. The isSSH() || !canOpenBrowser() branch prints the URL and polls without reading stdin, and is still allowed. Same missing newline on its prompt, fixed. Unlike the other two this refusal has no headless equivalent - a permanent account is created in the browser - so it names signing in with one that exists: creating a permanent account needs browser sign-up, and browser sign-up needs an interactive terminal; run hookdeck login in a terminal to keep this sandbox's data, or sign in to an account you already have with hookdeck ci --api-key or hookdeck login --cli-key Both browser branches now print the sign-in URL on its own line before starting the spinner. It used to be carried only by the openBrowser error path, via a stop-spinner/restart-spinner dance, and that path is the detectable half only: open.Browser is exec.Command(...).Start(), which returns nil the moment the child is spawned. A browser that dies straight after - WSL, containers, VS Code Remote - reported success, so the user got a bare spinner and no link. Printing it unconditionally also lets the failure message shrink to one line that does not repeat the URL. Audit of every instance of this shape in pkg/login, since three surfaced separately. Two openBrowser call sites, both in client_login.go, both now guarded; two Fscanln reads, the same two branches; one echo-suppressed stdin read in InteractiveLogin, guarded by #401. No fourth. GuestLogin polls but prompts for nothing and opens nothing. The only other openBrowser in the repo, pkg/listen/tui/update.go:283, is an "o" keypress in the interactive renderer, which #333 already downgrades to compact without a terminal. Not touched: RefreshGuestSigninLink's stale-URL/TTL behaviour, which #373 also tracks and which is a larger behavioural question. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BnrKWZQASV7bFJ4oGWwmo9
Two commands reported success while answering a different question. #406: destination update/upsert build their config by switching on the destination type, and --type is normally omitted, so the switch fell to the empty-type default and --url and --cli-path were never copied into the request. The PUT went out without them and the command exited 0. PR #392 resolved the stored type for the delivery-policy guard but deliberately kept it out of config building, because wiring it there opportunistically would have made --url work only when a rate-limit flag happened to be present too. So resolve it generally instead: the lookup now fires for any flag whose handling depends on the type, and the resolved type is what config building gets. It reuses the memoised GetDestination the policy guard already pays for, so a typeless update still costs one GET. A type-specific flag the type has no field for is now refused rather than dropped, in both directions: --url on a stored CLI destination, and --url with no type to resolve at all (an upsert that is really a create). The empty-type default stays tolerant, because auth and delivery-policy flags mean the same thing whatever the type. #407: metrics events picks one endpoint from ordered conditions, so a queue-depth measure matched first and the issue_id dimension went to /metrics/queue-depth, which does not group by issue. "pending" shadowed it the same way, and the --issue-id filter was reported as an unsupported filter rather than as the second route it is. RejectCrossRouteEventQuery extends the #392 mixed-measure rule to measure/dimension conflicts and names both routes. It lives in pkg/hookdeck beside RejectMixedMeasureRoutes, which it subsumes; the MCP layer still calls the narrower one and needs the same one-line swap. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BnrKWZQASV7bFJ4oGWwmo9
Four output bugs found in v2.6.0 RC testing. #376 fixed readiness never being announced without a TTY; #399 is the same bug inverted, and is the one that matters most here. #399 — interactive mode looked connected before it was. The TUI drew its complete layout immediately — brand header, "Listening on …", "Requests to →", "Forwards to →" — and drew the status bar only once the websocket was up. A session that never connected was therefore identical to a working one apart from a line that was absent, for the whole 40-second attempt budget, before the alt-screen was torn down and an error printed. Absence is not a signal a user reads. The model now carries an explicit connection state whose zero value is "connecting", the status bar is drawn on every frame, and it leads with that state: "● Connecting…", "● Connecting… (attempt N)" while retries are in flight, "● Connected." on success, "● Reconnecting…" after a drop, and "● Connection failed: <reason>" when the CLI gives up — held briefly so it is visible inside the alt-screen rather than only after it. A failed attempt before the first connect is counted rather than reported as reconnecting, because the CLI cannot claim a connection it never had. #402 — compact output printed the bare preposition "Listening on". The counts the interactive header shows now live in pkg/listen/summary, a leaf shared by both renderers (as pkg/listen/links already is), so the two modes cannot drift apart again. Compact is the automatic no-TTY fallback, so this is the line most CI logs keep. #403 — OSC 8 hyperlinks were hand-rolled and emitted unconditionally, so redirected output carried the escape bytes and a real terminal — the only thing that can render them — carried a plain URL. They are now gated on the same check colour uses, and the plain-text fallback prints the full URL including team_id, which the hyperlink label deliberately omits. The CLI also now honours NO_COLOR, which it never had. #404 — --color off reached only pkg/ansi, and the TUI draws with lipgloss, so a controlling-pty run with the flag set still emitted 48 SGR sequences. The interactive renderer now threads the same answer into the TUI styles, which renders every frame with no SGR bytes at all. Verified across five output environments with a real openpty + TIOCSCTTY harness: the non-TTY readiness line of #376 is unchanged in compact and quiet, --color off on a TTY drops from 48 SGR sequences to 0, and compact on a TTY now emits the hyperlink while a pipe does not. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BnrKWZQASV7bFJ4oGWwmo9
The upsert builder asserted the resolved type back onto the request. The type
resolution is needed -- it decides whether --url is even a valid field for this
destination -- but sending it makes the command a read-modify-write: if the
destination's type changes between the lookup and the PUT, we silently revert
it. Omitting the field leaves the stored type alone.
Verified against the live API: an upsert carrying a config and no type field is
accepted, applies the change, and keeps the stored type.
This also makes upsert agree with update, which already asserted exactly this
rule ("resolving the stored type must not start sending a type the user did not
pass") -- the two builders had opposite behaviour for the same situation.
An explicit --type is still sent, and is covered by a new case.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BnrKWZQASV7bFJ4oGWwmo9
…ot honour Found by driving the MCP server over JSON-RPC against the live API. The shape throughout: MCP flattens several CLI subcommands into one tool with one flat schema, and the per-subcommand precision the CLI has is lost. hookdeck_requests accepted delivery_group on action "list" and the API silently ignored it -- a bogus value returned rows byte-identical to the unfiltered baseline, while every other filter narrows to zero. That is the same silent-wrong-answer hazard the metrics tool already guards against, unguarded one tool over, on the feature this release ships for. Arguments an action does not support are now refused, for hookdeck_requests and hookdeck_events alike. ignored_events was passing nil for limit/next/prev, so pagination was dropped. Dimensions received no client-side gating at all, unlike filters, so known-bad combinations fell through to raw 422s -- including the delivery_group dimension, which requires a destination_id filter. Gated per route from the shared matrix, in the CLI as well as MCP. The metrics schema advertised four "common" measures, three of which fail on most routes, and gave measures, dimensions and status no per-action values at all -- the three parameters that decide whether a call succeeds. All three are now accurate per action and derived from shared constants, so the CLI's hand-maintained lists and the MCP schema cannot drift again. 422 bodies surfaced verbatim with internal fields, burying the useful message. Also applies the #407 cross-route guard to the MCP routing, which the CLI-side change could not reach. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BnrKWZQASV7bFJ4oGWwmo9
hookdeck_requests action "events" dropped source_id before the request was
built, so "the events of req_X that came from source Y" answered with every
event of the request. That was briefed as "the API ignores it" and the previous
commit acted on it, refusing the argument. The brief was wrong:
GET /requests/{id}/events declares the whole /events filter set and honours it
-- verified live, a bogus source returns zero rows and the real one returns the
matching row.
So the refusal is replaced with forwarding for everything the route declares,
and `gateway request events` gains the matching flags. It offered five; it now
offers the flag set of `gateway event list`, same names, same wording, because
it queries the same collection narrowed to one request. --id is the one flag
left off: this command already takes the request ID as its argument, and a
second --id meaning "event IDs" beside it reads as the request's.
Still refused on events, and still tested: verified, rejection_cause and
ingested_* describe the edge decision, which the sub-resource has no parameter
for. Confirmed against the live route -- it answers 200 with unfiltered rows for
a parameter it does not declare, which is exactly the silent-wrong-answer the
guard exists for.
status was the one real design problem. It means ACCEPTED/REJECTED on list and
SCHEDULED/QUEUED/.../CANCELLED on events, and MCP has one flat property per
tool. The description now names both vocabularies, the shape hookdeck_metrics
already uses for the four its own status argument carries, rather than inventing
a second spelling the CLI has no equivalent of. The handler checks the value
against the action's own list: the API does 422 on an out-of-enum status, but
that message names only the enum of the route it was sent to, never the sibling
action that takes the value. Matching ignores case and sends the API's own
spelling, which the API itself will not do -- it 422s "successful" against the
upper-case enum.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BnrKWZQASV7bFJ4oGWwmo9
# Conflicts: # pkg/cmd/metrics_events.go
A revert audit of #392 found fixes that can be deleted with the whole suite green. The code is correct today; the problem is that nothing would notice if a refactor undid it, and several of these guard the silent-wrong-answer class the release exists to fix. Each test below was written first, then checked by reverting the fix it covers and confirming it fails. - MCP: gate the filters each events route ignores (queue depth, pending and by-issue). Only requests/attempts/transformations were covered, so the delivery_group family of bugs was unprotected on the tool side. - MCP: pin queue_depth -> max_depth at the request, mirroring the CLI's TestQueueDepthMeasureIsTranslatedOnTheWire. Without it the tool sends a 422 for a measure its own schema advertises. - MCP: gate the by-issue route's dimensions, the one events route missing from the dimension table. - `destination create`: extract buildCreateRequest, in the style of buildUpdateRequest and buildUpsertRequest, so both cliPathFromFlags call sites are covered through the command's own wiring. Testing the helper alone could not tell whether the command still called it, and raw dc.cliPath reinstates --config's path being overwritten with "/". - CLI: gate the dimensions of `metrics attempts`, `metrics requests` and `metrics transformations`, driven through each command's RunE. - Delete the default events route's filter guard in both layers: it can never fire, because DefaultEventRouteFilters differs from the union only by issue_id and any issue_id selects the by-issue route above it. A new hookdeck test pins that invariant, so a filter the route does not honour brings the guard back rather than passing unnoticed. - listen: cover Proxy.Run's give-up path end to end, asserting the renderer is told why before it is torn down (#399). The retry budget and its backoff become vars so the test runs in milliseconds rather than twenty seconds; the CLI never changes them. - listen: give InteractiveRenderer an injectable message sink and its first unit tests, covering a session-level OnError surfacing as a ConnectionFailedMsg the model acts on. Two tests that failed for the wrong reason: - TestSetColorEnabledKeepsTheWords asserted on the status bar, so a #399 regression failed as a #404 colour bug. It now asserts on the frame; position belongs to TestStatusBarAlwaysReportsConnectionState. - The #399 acceptance test passed with the status bar removed, because renderConnectingStatus writes the same words into the viewport body. It now matches the status bar line specifically. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BnrKWZQASV7bFJ4oGWwmo9
This was referenced Sep 14, 2026
Eight findings from a review of the merged PR #392, most of them one layer fixed and the other missed. - RejectUnsupportedDimensions: say that the delivery_group/destination_id rule is the API's and applies on every route offering the dimension, not just events. Verified live against `metrics attempts`. - Report a refused dimension in the caller's spelling. Both callers rewrite connection_id to webhook_id before validating, so the refusal named a token the caller never typed, beside an allowed list that spelled it connection_id. The filter path was already correct. - hookdeck_events action "list" now canonicalises `status` like hookdeck_requests action "events" does. Same collection, same enum, and only one of them accepted "failed". - apiErrorMessage: hold "data" as a raw value. Decoding it straight into a []json.RawMessage failed the whole unmarshal on an object or string data and threw the top-level "message" away with it, which is the raw body dump the function exists to prevent. - The CLI canonicalises --status too: `request list` against the request-log enum, `event list` and `request events` against the event enum, and --help now names the vocabulary each one takes. MCP has done this since status.go landed; the CLI had not, so ACCEPTED worked through one surface and 422'd through the other. - --config/--config-file and the individual destination config flags are now a refused conflict on create, update and upsert alike. They disagreed three ways before, and `upsert --config ... --url ...` without --type dropped the URL and exited 0. - One routing table: hookdeck.RouteForMeasures replaces the CLI's map and MCP's containsAny list, and the route-name constants replace the hand-written strings that gave one route two names in adjacent errors. - REFERENCE.md documented `metrics queue-depth`, `metrics pending` and `metrics events-by-issue`, none of which exist; the per-route dimension gating was undocumented. Both fixed, and a test now pins every `gateway metrics <sub>` in the prose to a real subcommand. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BnrKWZQASV7bFJ4oGWwmo9
faa742f made --config/--config-file a refused conflict with the individual destination config flags on `destination create`, `update` and `upsert`. That is a breaking change: before it, destination create --type HTTP --config '{"url":"https://from-config"}' \ --url https://from-flag succeeded and sent url=https://from-flag; after it, the command errors. 2.6.0 is not the release for that, so the change comes out here and will be re-proposed against 3.0.0 on its own. Restored from faa742f^, hunk for hunk: - rejectConfigJSONWithIndividualFlags and destinationIndividualConfigFlags in destination_common.go, and the three validateFlags call sites in destination_create.go, destination_update.go and destination_upsert.go. - The two overlays the rejection made unreachable: the --url overlay in buildCreateRequest (its applyCLIPath call was never removed and stays), and the --url/--cli-path overlay in buildUpsertRequest. - The four tests that pinned the refusal: TestDestinationConfigJSONRefusesIndividualFlags, TestDestinationConfigFileRefusesIndividualFlagsToo, TestDestinationConfigJSONAloneIsStillAccepted and TestDestinationUpsertAndUpdateAgreeOnConfigJSON, with the helpers added for them. The five builder tests that predate faa742f are untouched. - The --config/--config-file help text on all three commands, and REFERENCE.md regenerated to match. This restores the pre-faa742f behaviour as it was, three-way disagreement included: `create` and `upsert --type HTTP` let --url win, `update` and `upsert` without --type let --config win. Fixing that is the 3.0.0 proposal, not this commit. The other seven findings in faa742f stay: the RejectUnsupportedDimensions comment, hookdeck.DimensionName and the caller's-spelling refusal, canonicalEventsStatus, the apiErrorMessage raw-data fix, status_flag.go and its three callers, hookdeck.RouteForMeasures and the route-name constants, and REFERENCE.md's corrected metrics prose with its test. metrics_filters.go was picked over by hand rather than reverted, so the comment correction it shares with this file set survives. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BnrKWZQASV7bFJ4oGWwmo9
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #378 (base:
feat/api-2026-09-01). 20 commits, 75 files, +8848/−460. Defects found by building #378 and exercising it against the live API — plus the acceptance coverage whose absence let them through.It opened as a 96-line PR fixing five defects. It is much larger now, because every round of checking found more, and the last round found the thing worth the whole exercise.
How it grew
That fourth row is the argument for the whole PR:
gateway destination updatedestroyed delivery group overrides, and neither the unit tests nor two code reviews caught it. The first acceptance test written for the feature caught it immediately.Delivery groups and destinations
upsertdestroyedgroups.overrides(#393)updatedestroyed them toogroupsobject anyway, reintroducing the loss through the error path--type CLI--rate-limit/--delivery-group-*exited 0 and applied nothingupdate/upsertomit--type, so the type was""and it never firedconnection upsertbypassed it entirely--configpath overwritten (#392)--config '{"path":"/webhooks"}'on a CLI destination stored/connection upsertreset a stored CLI path--destination-cli-pathrewrote it to/update/upsertdropped--url(#406)--typewas omitted — the normal way to run an updateupsertasserted a type nobody asked forTwo of these — the
--configclobber and theconnection upsertpath reset — are byte-identical in v2.5.0, so they are pre-existing shipped bugs, not regressions from #378.Metrics
--measures pending,failed_countexited 0 having silently discardedfailed_countissue_iddimension. The audit found three such pairs, not the one reported--measures pendingneeded--granularity--measures queue_depthnever worked--help, absent from the endpoint's enum--dimensions/--statushelp wrong on 3 of 4 subcommands--measureswas optionalgateway --helpdid not runVocabularies are cross-checked against the API's OpenAPI document route by route, and verified live — the six event statuses all query, and
PAUSED, which an earlier commit in this PR wrongly advertised, 422s.MCP
hookdeck_requestsaccepteddelivery_grouponlistand the API silently ignored it — a bogus value returned rows identical to unfiltered, while every other filter narrows to zero. The same hazard the metrics tool already guarded against, one tool over, on the feature this release ships for.requests eventsfilters now forwarded, not dropped — ~22 args, with matching CLI flags ongateway request events(5 → the fullevent listset). Correction to an earlier claim in this PR: that route genuinely honourssource_id; MCP was dropping it, not the API ignoring it.ignored_eventspagination, per-action measure/dimension/status vocabularies derived from shared constants, status canonicalisation,meta.active_project_namefor project-scoped keys (#405), and 422 bodies reduced to the useful line.listen and login
listenlooked connected before it was (#399) — the full TUI rendered with a blank status bar for ~40s while nothing was connected. This is listen connects but never prints "Connected": readiness line is dropped when output is piped or --color off #376 inverted: piped output under-reported readiness, interactive over-reported it. NowConnecting…→Connected.→Connection failed: <reason>, affirmative in every mode.Listening on(#402), OSC-8 emitted only where it cannot render (#403),--color offignored by the TUI (#404), plusNO_COLORsupport the CLI never had.hookdeck loginopened a browser and hung forever with no terminal (#400), and the same on the guest-upgrade path (#373). The sign-in URL is now always printed, so a browser that fails after spawning is visible rather than silent.login -iexplains itself instead of leaking a termios error (#401).Test coverage
Delivery groups had none (
grep -rn "delivery-group" test/acceptance/→ 0 hits) on the feature this release exists for.27 new test files, 16 acceptance test functions, 115 unit test functions.
Every fix here was verified by reverting it and confirming the relevant test fails. A later audit did that mechanically across 80 individual reverts on the merged tree and found 9 fixes that were not actually pinned — all since closed, along with two guards that could never fire.
The two metrics ATs added are the ones whose absence let real bugs ship:
TestMetricsEventsPendingpassed--granularity 1handTestMetricsEventsQueueDepthusedmax_depth, so both were written around the broken behaviour and could never have failed.Verification of the merged result
Also clean:
go build,go test ./...(17 packages),go vetuntagged and underbasic/listen/destination/connection_upsert/metrics,generate-reference --check, and nogofmtregression.Delivery-group and metrics behaviour was verified against a project with real grouped production traffic — 4.68M events across five delivery groups — because the CI test project has no delivery groups at all. Real group returned 19,688; bogus group returned 0.
Deliberately not included
--configmutual-exclusion fix is reverted and deferred to 3.0.0 (#415) — it is a breaking change and this is a minor release. So 2.6.0 still ships the existing wart: two fields overlay--config, about ten flags are silently discarded, andcreate/update/upsertdisagree. Unchanged from today, now tracked.groupsobject still will, and the client-side fix is a read-modify-write that races a concurrent edit.connection upsertcarries overrides forward from the connection's current destination even when--destination-namenames a different one. Pre-existing; forcing the lookup widens its reach. A semantic call about what that flag means.Filed rather than fixed: #394, #396, #397, #410–#414 — all pre-existing, none introduced here.
This PR gets no automatic checks —
test.ymlonly triggers on PRs targetingmain(#395). It was triggered manually and passes. Acceptance tests cannot run on this branch at all, so the new delivery-group ATs will first execute in CI when this lands in #378. They pass locally and in a full 391-test local run.🤖 Generated with Claude Code
https://claude.ai/code/session_01BnrKWZQASV7bFJ4oGWwmo9