feat: support 2026-09-01 gateway API - #378
Open
alexbouchardd wants to merge 28 commits into
Open
Conversation
Collaborator
|
Claude-assisted review - I ran Checklist so we can track what gets picked up. Tick as they land, or push back on any of them. Edited after posting: the Blocking
Worth fixing
Tests
Not blocking
Open question on namingNot blocking, and I may be missing context from the API side. Verified while reviewing: I can put the non-blocking fixes and the test gaps into a PR against this branch if that's easier than folding them in yourself. |
Findings from reviewing #378. The `/projects` question is not addressed here - that is a deploy-ordering decision, not a code change. Only offer --delivery-group where the API accepts it. The flag was added to every metrics subcommand, but delivery_group exists only on the events, attempts and queue-depth filter schemas, and those filters are additionalProperties:false. So `metrics requests --delivery-group` and `metrics transformations --delivery-group` were a guaranteed 422, as were the two `metrics events` routes that land on events-pending-timeseries or events-by-issue. The flag is now omitted where it cannot work and rejected client-side on the two routes that share a command with routes where it can. metricsFlagOpts replaces the positional skipIssueID bool so a second exclusion does not turn every call site into unreadable booleans. Fall back to team_mode when team_product is absent. The cutover left no fallback: an empty product blanks ProjectProduct, ProjectMode and ProjectType at once, IsGatewayProject("") is false, and every gateway command then fails with an empty project type in the message. The live API does return team_product today - verified against prod with the type, mode and product stripped from a config - so this is insurance, not a repair. Stop ListProjects swallowing a shape mismatch. The unmarshal error was discarded, so a renamed field or a wrapped envelope would return an empty list and a nil error: "you have no projects" rather than a failure. Exactly the risk an endpoint rename introduces. Collapse five copies of the Type -> Product -> Mode ladder into Profile.ResolveProjectType. Same precedence, one place to get it wrong. Make the two case-sensitive mappers case-insensitive like their three siblings in the same file. Nothing documented which was which. Add ,omitempty to Event.DeliveryGroup so `event list --output json` does not start emitting "delivery_group": null on every event. Document --delivery-group in REFERENCE.md, including where it does not apply. Run gofmt on the files this change touches; profile.go and profile_credentials_test.go were unformatted on the branch. Tests. The upgrade path had none: a config written before this release has no project_product, and nothing asserted it gets derived from the legacy mode - which is what every existing user hits on first run. The mapping tests covered roughly a third of the matrix; they are now table-driven per function and include the empty-product case, case-insensitivity, and a round trip that pins the deliberate outbound-to-inbound flattening. The acceptance test now asserts project_product actually reaches config.toml, which nothing did before: every other test on this path reads a mock this repo also writes. Refs #378 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BnrKWZQASV7bFJ4oGWwmo9
Collaborator
|
Follow-up PR with the fixes from the review above: #379 (targets this branch, so merge / cherry-pick / close as you prefer). Covers everything on the checklist except the
Left alone: the 99 hardcoded version literals in tests, and the CLI-destination delivery-policy validation - both judgement calls that are yours rather than mine. |
leggetter
added this pull request to stack #380
September 10, 2026 10:00
core `origin/staging` now names the field `type` on GET /projects and
`team_type` on the CLI auth endpoints, with the same event_gateway | console |
outpost values. The CLI followed the wire rename and the internal vocabulary
with it, so it speaks the same word as the API it calls.
That word was already taken. The CLI used ProjectType for the display label -
"Gateway", "Console", "Outpost" - so the two meanings had to be separated:
ProjectType event_gateway | console | outpost what the API calls type
TypeLabel() Gateway | Console | Outpost derived at print time
The label is presentation and is no longer stored. project_type on disk now
holds the API value, project_product is gone entirely (it only ever existed on
this unreleased branch, so nothing has written it), and project_mode is still
written for older CLIs reading the same file.
NormalizeProjectType is the single door every value goes through. It accepts an
API type, a display label written by an older CLI, or a legacy mode, and returns
the API type - so the three vocabularies converge in one place instead of at
each call site.
The auth structs read team_type, then team_product, then team_mode. Prod
currently serves team_product while staging serves team_type, so without that
chain a CLI shipping ahead of the deploy would blank the project type and fail
every gateway command. Verified against prod, which still serves the old field:
whoami resolves Gateway from a config stripped of all type information.
Two things deliberately unchanged, both user-facing: `--output json` still emits
gateway | outpost | console, and the `--type` filter still accepts them. The
API type is not used there - changing it would break anyone parsing that output.
Tests caught two regressions worth naming: the gateway error message printed the
wire value ("current project type is outpost") instead of the label the user was
shown, and saving local config dropped an unrecognized mode instead of carrying
it through. Both fixed.
Refs #378
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BnrKWZQASV7bFJ4oGWwmo9
…prod The comments said the API rejects the unknown filter because the metrics filter schemas are additionalProperties:false. It does not. Tested against production with the pre-fix binary: `metrics requests --delivery-group` over a 14 day window returned count 87, identical to the same call with no filter, while a bogus --source-id on that call returned 0. So the filter key is recognized and applied when the endpoint supports it, and silently dropped when it does not. That makes withholding the flag more important than the original reasoning suggested, not less. An opaque 422 at least tells the caller something is wrong. Returning unfiltered totals under a flag that says they are filtered is the silent-wrong-answer shape, and the caller has no way to notice. No behaviour change: the flag was already withheld on requests and transformations, and rejected client-side on the two `metrics events` routes that cannot use it. Only the stated reason was wrong. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BnrKWZQASV7bFJ4oGWwmo9
Collaborator
|
Prod testing update, now that Confirmed working against prod
Correction to my earlier reviewI said Ran the pre-fix binary against prod, 14 day window: The API does not reject the unknown filter. It drops it and returns unfiltered totals. Debug logging confirms the CLI really did send This makes withholding the flag more important than I argued, not less. A 422 at least tells you something is wrong; unfiltered numbers under a flag that says they are filtered is the silent-wrong-answer shape, and there is no way for the caller to notice. No behaviour change needed - the flag was already withheld in the right places. I have pushed 4f6bfd3 correcting the comments and the test rationale, which previously asserted the 422 story. Gap worth knowing before releaseNothing in CI exercises Worth noting CI authenticates with |
The delivery-group fix was one instance of a general bug. metricsCommonFlags added six filter flags to all four metrics subcommands, but each endpoint declares its own filter schema and the API drops keys it does not recognize instead of rejecting them. So the flags that did not apply returned unfiltered totals under a flag saying they were filtered. Measured against production, 14 day window: attempts, no filter count: 120 attempts --source-id src_bogus count: 120 <- source_id not in schema attempts --destination-id des_bogus count: 0 <- destination_id is Nine flag/endpoint pairs were affected: requests offered destination-id, connection-id and issue-id; attempts offered source-id, connection-id and issue-id; transformations offered source-id, destination-id and status. All silently no-ops. Each subcommand now registers only the filters its endpoint honours, driven by a metricsFilters set per endpoint. `metrics events` is the exception: it fans out over four endpoints depending on measures and dimensions, so it offers the union and validates per route at run time, naming the flag and saying why it matters rather than just refusing it. TestMetricsFlagsMatchTheEndpointSchemas asserts the whole matrix, so adding a filter API-side fails the test rather than passing unnoticed. Also: replace the hardcoded API version in tests with hookdeck.APIPathPrefix, down from 99 occurrences to 4 - a genuine date value and three comments. The constant's own comment says to change the version in one place; it was being copied into 87 lines of pkg/gateway/mcp/server_test.go alone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BnrKWZQASV7bFJ4oGWwmo9
TestMetricsAttemptsWithConnectionID ran `metrics attempts --connection-id web_placeholder`, asserted the command succeeded and stopped there. It did succeed - because the attempts endpoint has no webhook_id filter and the API drops keys it does not recognize, so the call returned unfiltered totals under a flag that said otherwise. The test was pinning the bug. Measured against production over 14 days, with a control to rule out the filter simply matching nothing: attempts, no filter count: 150 attempts --connection-id web_bogus count: 150 <- ignored events --connection-id web_bogus count: 0 <- honoured (145 unfiltered) Same flag, same filters[webhook_id] mapping, different endpoints. The test now asserts the flag is refused. It reads the message from stdout, because that is where cobra writes flag errors - stderr is empty and the error is only "exit status 1", so asserting on stderr would have passed regardless of what the CLI printed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BnrKWZQASV7bFJ4oGWwmo9
ProjectMode was threaded from config through listen, proxy, the renderers and the TUI, only ever to answer "is this a console project?". That is a project type question, and mode is the vocabulary the API dropped. Behaviour is identical: the only test anywhere was `== "console"`, and the console type is the string "console" in both vocabularies. The other values differ (inbound vs event_gateway) but neither is console, so every branch resolves the same way. The comparison now names config.ProjectTypeConsole rather than a bare string. Renamed through links, listen, printer, proxy.Config, RendererConfig, both renderers and the TUI model. What is left called mode is deliberate and now documented on the field: the pre-2026-09-01 config key, written so a CLI older than this one reading the same config file still resolves a project, and the team_mode wire field read as the last fallback in resolveType. Neither is vocabulary this codebase should reason in. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BnrKWZQASV7bFJ4oGWwmo9
The config file is shared between CLI versions - a repo-local .hookdeck/config.toml most obviously - and versions before 2026-09-01 only understand the display label. Writing the API type there broke every `hookdeck gateway ...` command for them: $ hookdeck gateway source list # older CLI, config written by this one this command requires a Gateway project; current project type is event_gateway Worse than one-off: both versions rewrite the file, so two people on different versions would ping-pong it and the older one would break again each time. Measured against production, not reasoned about. So project_type on disk stays exactly what v2.5.0 writes - Gateway, Console, Outpost - and project_mode keeps its legacy value. Internally nothing changes: ProjectType still holds the API type, and NormalizeProjectType turns the label back into it on read, which it already had to do for configs written before this release. Verified the full round trip with both binaries against production: new CLI selects a project, old CLI runs gateway commands against the same file, old CLI switches project, new CLI reads it back. Every step resolves correctly. The label and the API type are one-to-one, so persisting the label loses nothing. It also puts the two on-disk keys on the same footing: both are compatibility surfaces speaking the vocabulary the most versions understand, rather than one of them speaking the wire. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BnrKWZQASV7bFJ4oGWwmo9
Nothing exercised this endpoint for real. Every other test on the path uses an httptest mock this repo also writes, so they verify our parsing and nothing about the endpoint: not that it exists, not that it authorizes the credential the CLI carries, not that its response still matches what we unmarshal. The tests that do need it sit behind //go:build manual, and they are manual because of the browser login flow, not because of the endpoint. That mattered here. /teams moved to /projects in 2026-09-01 and the type field was renamed twice during the release, and the mocks were updated alongside each rename, so they would have passed whatever the API did. On the credential: the acceptance runner bootstraps with a project API key, but `hookdeck ci` exchanges it at /cli-auth/ci for a CLI client key, and that is what lands in the config and goes on the wire. So this covers the CLI-key path a real user has. ListProjects also drops the project scoping header via clientForCLIAuthValidate, which is only observable against the real API. Asserts the three things worth pinning, all verified against production first: the endpoint answers a CLI key at all, `--output json` still reports gateway | outpost | console rather than following the API's event_gateway rename, and the --type filter speaks the same vocabulary as that output. Runs in the existing project_use tag, which is already in CI slice 0. No new secret needed - I had thought this needed one, which was wrong. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BnrKWZQASV7bFJ4oGWwmo9
My previous commit asserted the CI runner could list projects. CI proved
otherwise, with the clearest possible error:
403 GET /2026-09-01/projects
"This credential is scoped to a single project and cannot list all projects.
Keys from hookdeck ci are project-scoped. Run hookdeck login for
account-wide CLI access."
There are two kinds of CLI key. `hookdeck login` issues one bound to a user and
it can list projects; `hookdeck ci --api-key` issues a project-scoped key with
no user, and core rejects it:
if (!req.context.user?.id) { throw new APICLIProjectScopedError() }
I missed this because my own key comes from `hookdeck login`, so every manual
check against production passed. The runner authenticates with `ci`, which is
also the real reason the project-listing tests were behind //go:build manual -
not the browser login flow, as their comment implies.
Not a regression: the guard landed for GET /teams in core 90e38ee395 and the
/projects rename inherited it. So `project list` has never worked with ci
credentials.
The CI-runnable test now asserts the restriction itself, which is more useful
than asserting the happy path anyway: it checks the 403 surfaces with the reason
and names the command that fixes it, so a change to that rule fails a test
instead of arriving as a support ticket.
The vocabulary assertions - that --output json and --type still speak
gateway | outpost | console rather than following the API's rename - move to the
manual suite, where an account-wide key exists to run them.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BnrKWZQASV7bFJ4oGWwmo9
The coverage I said was missing already existed, in the same file I edited. TestProjectListFailsWithCIKeyAcceptance asserts the 403, and TestProjectListShowsType / TestProjectListJSONOutput / TestProjectListFilterByType assert the display labels and the gateway | outpost | console json vocabulary. I missed them because I grepped for ListProjects and /projects, and their bodies only say "project list". Their skip message already states the rule I reported as a discovery: "CLI key required for listing projects; API and CI keys cannot list or switch projects" So both halves were covered. Removed my duplicates in both the CI and manual files. What survives is one assertion the existing rejection test lacked: that the error names `hookdeck login`. Checking the reason is not enough, because the two kinds of CLI key are invisible to the user - without the command that fixes it, the message says what went wrong and not what to do. The real gap is not code: HOOKDECK_CLI_TESTING_CLI_KEY is not set in CI, so the five tests gated on it skip on every run. They are written and correct; they have never executed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BnrKWZQASV7bFJ4oGWwmo9
Five project list/use tests are gated on this variable and skip on every run because it was never set, so they have been written and correct but never executed. That is why the /teams to /projects move and two renames of the project type field could all land without a test noticing. It has to be an account-wide CLI key from `hookdeck login`. The key the runner gets from `hookdeck ci` is project-scoped with no user attached, and core returns 403 for that on this endpoint by design, so it cannot stand in. The secret still needs adding to the repository; this only wires it through. Until then the tests skip exactly as they do now. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BnrKWZQASV7bFJ4oGWwmo9
…sting Reverting c94662e. That commit passed HOOKDECK_CLI_TESTING_CLI_KEY to the acceptance jobs so five long-skipping tests would run. They did run, and they passed - but the key is account-wide, and an account-wide key reaches every org its owner belongs to, including Hookdeck Prod. This repository is public, so Actions logs are world-readable. Nothing leaked in the run that happened: I checked the log for org and project names and found none, because assertions only print on failure. But the failure path was one red build away from publishing the lot: RunExpectSuccess -> require.NoError(t, err, "...stdout: %s...", stdout) TestProjectListShowsType -> assert.Contains(t, stdout, "|") and `project list` on such a key returns every project in every org the owner can see. We have already had two unrelated 502 flakes in this slice today, so "only on failure" is not much of a guard. So the variable goes back out of the workflow, with a comment saying what has to be true before it returns: the key should belong to a test-only account in its own org, so that what a failure can disclose is worth nothing. Independently of that, the tests no longer put a listing in any failure message. All nine call sites move from RunExpectSuccess, which formats stdout into its error, to Run plus an assertion that carries no payload. Worth having whatever key is used later - the listing is the credential's reach made legible, and it does not belong in a public log. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BnrKWZQASV7bFJ4oGWwmo9
Re-applies what 8638838 reverted. The key behind the secret is no longer a person's: it belongs to a test-only account, so the project inventory a failing test could disclose is worth nothing, and it no longer reaches Hookdeck Prod. Five tests have been skipping since they were written because this variable was never passed through - which is how the /teams to /projects move and two renames of the project type field all landed with nothing checking them end to end. The assertion hardening from 8638838 stays: all nine project list call sites use Run rather than RunExpectSuccess, so no failure message carries a listing. The scoped account is the control that matters, but there is no reason to print the thing either. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BnrKWZQASV7bFJ4oGWwmo9
`hookdeck login --api-key <rejected>` announced "Starting browser sign-in...", walked straight past the Enter prompt because there is nothing to read from, and then polled for a confirmation nobody could give. CI spent 248 seconds on a mistyped key before giving up; reproduced locally with stdin=/dev/null. This is the shape #337 fixed in v2.5.0 - commands hanging instead of failing where there is no terminal - and the same guard already exists a few lines below for a key that is valid but project-scoped. The rejected-key branch simply never got it. The error also says what to check, because "invalid or expired" is often untrue here. A project API key is a valid key that the CLI auth endpoints do not accept; an organization API key is not accepted by any of them. Both arrive at this branch, and telling their owner to re-authenticate a working key helps nobody: the API key was rejected, and browser sign-in needs an interactive terminal; check the key is a CLI key from hookdeck login rather than a project or organization API key, or use hookdeck ci --api-key with a project API key TestLogin_unauthorizedValidateStartsBrowserFlow had to start stubbing stdinIsTerminal to true. It passed before without doing so, which is the bug stated as a test: the browser flow ran whether or not anyone could complete it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BnrKWZQASV7bFJ4oGWwmo9
TestCIFailsFastWithInvalidAPIKeyAcceptance sends a deliberately invalid key and asserts the CLI answers with a friendly authentication failure. When POST /cli-auth/ci returns 502 instead, the assertion fails and the board goes red for something the CLI did not do. That happened three times today. A gateway error is not an authentication outcome, so the test cannot read it either way. It now retries on 502 the way CLIRunner.Run already does, and skips if every attempt is a 502 rather than reporting a product regression it has no evidence for. This test builds its own exec.Cmd rather than going through CLIRunner, which is why it inherited none of the existing retry behaviour. The context deadline goes from 60s to 150s so it actually covers the retries - sized for one attempt, it would cut them short and reintroduce the flake. Also drops a throwaway exec.Cmd that was only being used to carry args, dir and env into the loop. Refs #382 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BnrKWZQASV7bFJ4oGWwmo9
The README distinguished a CLI key from a project API key and then said "within
the CLI both are stored and used the same way", which is not true of the thing
people hit: only a CLI key can list or switch projects. Nothing mentioned
organization API keys at all.
Verified against production rather than read off the code:
/cli-auth/validate CLI keys only - both API key types get 401
/cli-auth/ci project API key 200, organization API key 401
/projects project key -> 1 project; org key -> its organization's
projects given projects.read; CLI key -> every org the
user belongs to
So an organization API key cannot authenticate the CLI by any route, and
`hookdeck ci --api-key` needs a project key specifically. Neither was written
down anywhere, and the error you get is "your API key is invalid or expired" on
a key that is neither.
Refs #376
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BnrKWZQASV7bFJ4oGWwmo9
The catch-all 401 handler replaced whatever the API said with "your API key is invalid or expired". That is an inference, and often a wrong one: a project API key is valid but not accepted by the CLI auth endpoints, and an organization API key is not accepted by any of them. Both land here, and both owners get told to re-authenticate a working key. We cannot do better by inspecting the key - validators.APIKey checks length and nothing else, so a bare 401 cannot tell an expired CLI key from an API key from a typo. So rather than guess more precisely, stop overriding the one source that does know. When the response carries an explanation, lead with it; otherwise fall back to the existing guidance. Worth being honest about the immediate effect: /cli-auth/validate and /cli-auth/ci currently answer with a bare "Unauthorized" body, which says nothing the status code did not, so most users will still see the fallback. The helper filters that case out deliberately rather than printing "Authentication failed: Unauthorized". What changes is that a message the server does send now reaches the user instead of being discarded - including from any endpoint that already returns one, and from these two if core ever improves them. Refs #283 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BnrKWZQASV7bFJ4oGWwmo9
b04a99f fixed this for `hookdeck gateway metrics` and touched pkg/gateway/mcp only to substitute a constant in its tests. The MCP tool kept building params from every argument and handing them to whichever endpoint the action routed to, so `hookdeck_metrics` still offered 22 argument/endpoint pairs the API silently drops - returning unfiltered totals to an agent with no way to notice. The release note would have been untrue for anyone using MCP. The matrix now lives in pkg/hookdeck beside the client, because both callers reach the same endpoints and a matrix only one of them consults is one the other drifts from. pkg/cmd and pkg/gateway/mcp both consult it; filter names are supplied per caller so a CLI user reads --source-id and an MCP client reads source_id. Three further places where MCP diverged from the CLI in the same routing function, all fixed here: - The pending timeseries route sent measures[]=pending. The API expects count there; "pending" only selects the route, which the CLI has always rewritten. - The by-issue route sent a request with no filters[issue_id]. The endpoint filters on it, so the route is meaningless without one; the CLI rejects this. - dimensions did not map connection_id to webhook_id, though the tool schema tells callers it does. True of the filter, not of the dimension. TestMetricsEvents_ByIssueRoute asserted a call with dimensions issue_id and no issue_id was a success, pinning the second of those. It now passes an issue_id, and a sibling covers the rejection. TestMetricsToolRejectsFiltersTheEndpointIgnores mirrors the pkg/cmd guard, so a filter added API-side now fails on both sides rather than being fixed for one. Refs #382 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BnrKWZQASV7bFJ4oGWwmo9
Two places still told people the opposite of what the code now does. REFERENCE.md listed --source-id, --destination-id, --connection-id and --status under "Common flags (all metrics subcommands)". They are not common: each endpoint accepts a different set, which is the whole point of the change. The list now holds only the flags that really are shared, with a table for the rest. The MCP tool schema described every filter with no indication of where it applies, so a model reading the schema was invited to pass source_id to attempts and find out by being refused. Each argument now names the actions that honour it, and the tool description says why passing one elsewhere is an error rather than a no-op. The runtime guard is the safety net. The schema is the contract, and it was still wrong. Refs #382 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BnrKWZQASV7bFJ4oGWwmo9
team_product was the API's name for this field between team_mode and team_type. The CLI read it as a middle step in resolveType because, when that code was written, production served team_product while staging served team_type - a CLI shipping into that window would otherwise have blanked the project type and failed every gateway command. The window is closed. Production now returns team_type and nothing else: neither team_product nor team_mode appears in the response. And no released CLI ever read team_product - it was added on this branch, so there is no installed base to keep working. So the chain is back to two: the current team_type, and team_mode for an API still serving the pre-2026-09-01 shape. Verified after removing it, against production, with every type value stripped from the config so the answer can only come from the API: whoami still reports "Project type: Gateway".
… a browser This is the CI failure from 9ff211a, and it is the acceptance-level twin of the unit test that commit already updated. I fixed one and missed the other. TestLoginAfterValidate401StartsBrowserFlowAcceptance asserted that a stale key drops into browser sign-in. It cannot: there is no terminal in CI, so nobody can press Enter or finish the flow in a browser, and the CLI now refuses rather than polling for a confirmation that will never arrive. The test was describing behaviour that could not have worked where it was running. Renamed to say what it checks, and it now asserts the refusal - including that the poll endpoint is never touched. The config-persistence assertions moved out to TestCIWritesTheProjectTypeAcceptance, because the login flow now stops before anything is written. They go through `ci`, which is the path that works without a terminal, and they still cover what they were added for: that project_type reaches config.toml as the label older CLIs understand, that the legacy mode is written beside it, and that project_product is not. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BnrKWZQASV7bFJ4oGWwmo9
TestTelemetryGatewayIssueGetProxy failed in CI and passed on a re-run against the same commit, and the log said only: Error: Received unexpected error: exit status 1 That is the CLI exiting non-zero and nothing about why. require.NoError was given the error alone while the command's stdout and stderr were discarded, so the only available response was to re-run it and hope. Both streams and the issue id now go into the failure message. The id matters because this test, unlike its two siblings, lists issues without a status filter, so it can pick up a resolved or dismissed one where they would skip. Deliberately not narrowing the filter to OPENED to match the siblings. If `gateway issue get` is supposed to work for an issue in any state, narrowing it would hide a real defect rather than fix a flake, and nothing currently establishes which it is. The next failure will now say. Refs #387 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BnrKWZQASV7bFJ4oGWwmo9
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
API-contract, cache-versioning, and delivery-policy validation issues can cause incorrect project detection or silently ignored configuration.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Migrates the CLI to the 2026-09-01 Gateway API and adds delivery-group support across CLI, metrics, MCP, configuration, and tests.
Changes:
- Updates API versioning and project-type handling.
- Moves rate limits into delivery policies and adds delivery-group options.
- Adds delivery-group filters and endpoint-specific metrics validation.
File summaries
| File | Description |
|---|---|
.github/workflows/acceptance.yml |
Adds account-wide CLI test credentials. |
AGENTS.md |
Updates the OpenAPI reference. |
README.md |
Documents credentials, delivery groups, and project types. |
REFERENCE.md |
Regenerates CLI flag documentation. |
test-scripts/test-api-upsert-behavior.sh |
Updates API paths. |
test/acceptance/README.md |
Updates acceptance-test documentation. |
test/acceptance/connection_test.go |
Verifies nested delivery policies. |
test/acceptance/destination_config_json_test.go |
Tests delivery-policy JSON. |
test/acceptance/guest_login_acceptance_test.go |
Updates auth response fixtures. |
test/acceptance/helpers_retry_test.go |
Uses the shared API prefix. |
test/acceptance/login_auth_acceptance_test.go |
Tests headless login and persisted type behavior. |
test/acceptance/metrics_test.go |
Rejects unsupported attempt filters. |
test/acceptance/project_use_test.go |
Updates project tests and output safety. |
test/acceptance/telemetry_test.go |
Updates telemetry expectations and diagnostics. |
pkg/cmd/actionable_error_test.go |
Tests unauthorized server messages. |
pkg/cmd/connection_create.go |
Adds inline destination delivery groups. |
pkg/cmd/connection_upsert.go |
Merges delivery policies during upsert. |
pkg/cmd/connection_upsert_test.go |
Tests connection delivery policies. |
pkg/cmd/destination_common.go |
Builds and validates delivery policies. |
pkg/cmd/destination_config_json_test.go |
Tests delivery-policy construction. |
pkg/cmd/destination_create.go |
Registers delivery-policy flags. |
pkg/cmd/destination_update.go |
Adds delivery-policy updates. |
pkg/cmd/destination_upsert.go |
Adds delivery-policy upserts. |
pkg/cmd/event_list.go |
Adds delivery-group filtering. |
pkg/cmd/gateway.go |
Uses normalized project types. |
pkg/cmd/gateway_test.go |
Updates gateway type tests. |
pkg/cmd/listen_cli_key_test.go |
Updates login fixtures. |
pkg/cmd/login.go |
Preserves project identity in local config. |
pkg/cmd/metrics.go |
Adds endpoint-specific metric filters. |
pkg/cmd/metrics_attempts.go |
Restricts attempt filters. |
pkg/cmd/metrics_events.go |
Adds delivery-group dimensions and route validation. |
pkg/cmd/metrics_filters_test.go |
Tests metrics filter matrices. |
pkg/cmd/metrics_requests.go |
Restricts request filters. |
pkg/cmd/metrics_transformations.go |
Restricts transformation filters. |
pkg/cmd/project_list.go |
Renders project-type labels. |
pkg/cmd/project_use.go |
Persists normalized project types. |
pkg/cmd/request_events.go |
Adds delivery-group filtering. |
pkg/cmd/root.go |
Improves unauthorized errors. |
pkg/cmd/sources/types.go |
Updates the OpenAPI source URL. |
pkg/cmd/whoami.go |
Resolves and displays API project types. |
pkg/cmd/whoami_test.go |
Updates project-resolution tests. |
pkg/config/clear_active_profile_credentials_test.go |
Updates credential-clearing assertions. |
pkg/config/config.go |
Normalizes and persists project identities. |
pkg/config/config_test.go |
Tests project-type compatibility. |
pkg/config/load_config_file_test.go |
Updates legacy-config expectations. |
pkg/config/profile.go |
Adds project-type resolution and persistence. |
pkg/config/profile_credentials.go |
Maps authentication responses to project types. |
pkg/config/profile_credentials_test.go |
Tests current and legacy response fields. |
pkg/config/project_type.go |
Defines project-type normalization. |
pkg/config/project_type_test.go |
Tests labels, modes, and normalization. |
pkg/gateway/mcp/project_display_test.go |
Updates project-list fixtures. |
pkg/gateway/mcp/telemetry_test.go |
Uses current API paths. |
pkg/gateway/mcp/tool_events.go |
Passes delivery-group filters. |
pkg/gateway/mcp/tool_help.go |
Updates MCP help text. |
pkg/gateway/mcp/tool_metrics.go |
Validates filters and maps dimensions. |
pkg/gateway/mcp/tool_metrics_filters_test.go |
Tests MCP metrics filtering. |
pkg/gateway/mcp/tool_projects_errors.go |
Updates project authentication guidance. |
pkg/gateway/mcp/tool_requests.go |
Filters and paginates request events. |
pkg/gateway/mcp/tools.go |
Exposes new MCP arguments. |
pkg/hookdeck/auth.go |
Updates authentication response fields. |
pkg/hookdeck/auth_test.go |
Tests project-type decoding. |
pkg/hookdeck/ci.go |
Updates CI client project fields. |
pkg/hookdeck/client.go |
Advances the API path prefix. |
pkg/hookdeck/events.go |
Adds event delivery groups. |
pkg/hookdeck/metrics.go |
Serializes delivery-group filters. |
pkg/hookdeck/metrics_filters.go |
Defines supported metrics filters. |
pkg/hookdeck/metrics_test.go |
Tests metrics query serialization. |
pkg/hookdeck/projects.go |
Uses the projects endpoint and strict decoding. |
pkg/hookdeck/projects_test.go |
Tests project listing behavior. |
pkg/listen/links/links.go |
Uses project types for dashboard links. |
pkg/listen/listen.go |
Passes project types into listeners. |
pkg/listen/printer.go |
Builds links from project types. |
pkg/listen/proxy/proxy.go |
Renames proxy project configuration. |
pkg/listen/proxy/renderer.go |
Renames renderer project configuration. |
pkg/listen/proxy/renderer_interactive.go |
Uses project types for interactive links. |
pkg/listen/proxy/renderer_simple.go |
Uses project types for simple links. |
pkg/listen/tui/links.go |
Builds TUI links from project types. |
pkg/listen/tui/links_test.go |
Updates TUI link tests. |
pkg/listen/tui/model.go |
Renames TUI project configuration. |
pkg/login/claimed_cli_key.go |
Uses normalized console detection. |
pkg/login/claimed_cli_key_test.go |
Updates claimed-key fixtures. |
pkg/login/client_login.go |
Fails fast for rejected headless credentials. |
pkg/login/client_login_test.go |
Tests terminal-aware login behavior. |
pkg/login/interactive_login.go |
Uses normalized console detection. |
pkg/project/credentials_test.go |
Updates credential fixtures. |
pkg/project/normalize.go |
Normalizes API project types. |
pkg/project/normalize_test.go |
Tests project normalization. |
Review details
Suppressed comments (2)
pkg/cmd/destination_common.go:94
- Using
groupRate > 0here means an explicit negative--delivery-group-rateby itself is treated as if no group flags were supplied and silently ignored. UsegroupRate != 0so the existinggroupRate <= 0validation rejects it.
hasGroups := groupKey != "" || groupRate > 0 || groupRatePeriod != "" || overridesJSON != ""
README.md:228
- This row conflates two distinct credentials:
hookdeck ci --api-keyconsumes a dashboard Project API key but returns/stores a project-scoped CLI client key. Split these into separate rows so users do not infer that the Project API key itself is the credential used by subsequent CLI commands.
| **Project API key** | dashboard, or `hookdeck ci --api-key` | the one project it belongs to | no |
- Files reviewed: 88/88 changed files
- Comments generated: 6
- 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
+5
to
+6
| // Project types as the API names them: `type` on GET /projects, and `team_type` | ||
| // on the CLI auth endpoints. These are the values stored in config and passed |
| cmd.Flags().StringVar(&flags.DestinationDeliveryGroupOverrides, "destination-delivery-group-overrides", "", "JSON object of group-specific delivery rate overrides") | ||
| } | ||
|
|
||
| func (f *destinationConfigFlags) validateDeliveryPolicyFlags(flagPrefix string) error { |
Comment on lines
+83
to
+85
| if period != "" && rate <= 0 { | ||
| return nil, fmt.Errorf("--%srate-limit must be a positive integer when rate limiting is configured", flagPrefix) | ||
| } |
| var ( | ||
| openapiURL = "https://api.hookdeck.com/2025-07-01/openapi" | ||
| openapiURL = "https://api.hookdeck.com/2026-09-01/openapi" | ||
| cacheFileName = "hookdeck_source_types.json" |
| ``` | ||
|
|
||
| Both flags are global, so they work with any command. A **CLI key** is tied to your user account and can navigate across projects; a **project API key** is scoped to a single project. Within the CLI both are stored and used the same way (see [Credential Types](#security-config-files-and-source-control)). | ||
| Both flags are global, so they work with any command. Which key you hold decides what the CLI can do: |
| api_key = "api_key_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" | ||
| project_id = "tm_5JxTelcYxOJy" | ||
| project_mode = "inbound" | ||
| project_type = "event_gateway" |
Two of my own changes were regressions. Both were covered by tests that passed. The 401 handler printed raw HTTP boilerplate to every user with an expired key. unauthorizedServerMessage suppressed a message of exactly "Unauthorized", but checkAndPrintError only produces that from a JSON body; these endpoints answer text/plain, so the message it actually stores is "unexpected http status code: 401, raw response body: Unauthorized" - which then got printed as the explanation. Reproduced with the real binary: "Authentication failed: unexpected http status code: 401, raw response body: Unauthorized" where the guidance used to be. Now suppressed, and the test drives a real httptest 401 through the client rather than hand-building an APIError the client never emits, which is why the original test passed while the code was broken. The non-TTY login guard removed a flow that works. waitForLoginSession prints the URL and polls when isSSH(), never reading stdin, so a remote session with a human in it completes fine without a terminal. Refusing on "no terminal" alone took that out - and left a user with an expired key worse off than one with no key, since an empty key skips the block entirely. It now refuses only when there is also no SSH session, which still fixes the CI hang. TestLoginAfterValidate401StartsBrowserFlowAcceptance sets SSH_CONNECTION to exercise exactly that branch. I had rewritten it to assert the refusal, which recorded the regression instead of catching it. Restored. An unrecognized project type no longer blanks the profile. resolveType returning "" meant both project_type and project_mode were written empty, so a project type this CLI has not heard of produced "current project type is ." on every gateway command and discarded the value another CLI version could have used. Each field now keeps what the API sent and is derived only to fill a gap, which is the position setProjectIdentity already took for command-line values. From Copilot, in the delivery-policy code: - A negative --rate-limit with no period passed both guards and was silently dropped, so an explicitly invalid value succeeded. Any negative rate is now configured-and-invalid. Same for --delivery-group-rate. - Delivery-policy flags were validated even when --config or --config-file was given, so an incomplete flag combination could reject a command whose flags would have been ignored anyway. - The source-types cache was not keyed by API version: after upgrading, source create validated against the previous spec's types for up to 24 hours. Both the URL and the cache filename now derive from APIPathPrefix so they cannot drift apart again - which is how this happened. - README showed project_type values the CLI never writes, and leaned on the hidden root --api-key/--cli-key flags that AGENTS.md says not to document as user-facing. The credential table now describes the supported flows. Refs #382, #283 Co-Authored-By: Claude Opus 5 (1M context) <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.
Summary
2026-09-01mode/team_modehandling withproduct/team_product, while retaining local config compatibilityconfig.delivery_policyand add delivery-group flags for standalone and inline destinationsTesting
env -u GOMODCACHE go test ./...env -u GOMODCACHE go run ./tools/generate-reference --checkCredentialed destination and connection acceptance tests were not run because
HOOKDECK_CLI_TESTING_API_KEYis not configured in this environment.