fix: review follow-ups for the 2026-09-01 API migration - #379
Open
leggetter wants to merge 2 commits into
Open
Conversation
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
leggetter
added this pull request to stack #380
September 10, 2026 10:00
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.
Follow-ups from the review on #378. Targets
feat/api-2026-09-01so you can merge, cherry-pick, or close it - whichever is least disruptive.Claude-assisted, same as the review comment. Every claim below was checked against
coreorigin/stagingor a real run, not just read off the diff.The
/projectsitem is deliberately not here:origin/stagingregisters the route on both/projectsand/teams, so that one is a deploy-ordering call rather than a code change.Behaviour
--delivery-grouponly where the API accepts it. It was added to every metrics subcommand, but resolving the filter schemas incoreorigin/staginggivesdelivery_grouponevent_filters_schema,attempt_filters_schemaandqueue_depth_filters_schemaonly.request_filters_schema,transformation_filters_schema,events_pending_timeseries_filters_schemaandevents_by_issue_filters_schemado not carry it, and those filters areadditionalProperties: false- so it was a guaranteed 422 rather than an ignored field.metrics requestsandmetrics transformationsno longer register the flag.metrics eventskeeps it, but errors client-side on the two routes that cannot use it (--measures pending, and per-issue).metricsFlagOptsreplaces the positionalskipIssueIDbool, so a second exclusion does not turn call sites into(cmd, f, true, false).team_modefallback. The cutover left none, and an empty product blanksProjectProduct,ProjectModeandProjectTypetogether - after whichIsGatewayProject("")is false and every gateway command fails with an empty type in the message. To be clear about the risk: the live API does returnteam_producttoday. I checked by stripping type, mode and product from a config and runningwhoamiagainst prod - stillProject type: Gateway. So this is insurance rather than a repair.ListProjectsno longer swallows a shape mismatch. The unmarshal error was discarded, so a renamed field or a wrapped envelope returns an empty list and a nil error - "you have no projects" instead of a failure. That is the specific risk an endpoint rename carries.,omitemptyonEvent.DeliveryGroup, soevent list --output jsonstops emitting"delivery_group": nullon every event.Consistency
One
Profile.ResolveProjectType()replaces the five open-coded Type -> Product -> Mode ladders (gateway.go,whoami.go,SaveProfile,setProfileFieldsInViper,constructConfig). Same precedence, one place to get it wrong.Case handling.
ProjectTypeToProductandIsGatewayProjectare now case-insensitive like the three mappers beside them.gofmt on the files touched here;
profile.goandprofile_credentials_test.gowere unformatted on the branch. I have not reformatted the ~40 files that were already unformatted onmain- that is not this PR's job.Tests
The upgrade path had no coverage.
config_test.gowas untouched, yetconstructConfiggained the back-fill every existing user hits on first run: a config written before this release has noproject_productand it has to come from the legacy mode. Now asserted, along withproject_productactually being persisted byUseProject.The mapping tests covered about a third of the matrix. Now table-driven per function, including the empty-product case, case-insensitivity, and a round trip that pins the deliberate outbound-to-inbound flattening that was previously only described in a comment.
TestProfile_LegacyModeFallbackcovers the new fallback in all threeApply*paths, product-wins-over-mode, and both-absent.The acceptance test now checks
project_productreachesconfig.toml. Nothing did before - every other test on this path reads a mock this repo also writes, so the persisted field was never verified against a real CLI run.Verification
go build ./...,go test ./pkg/...,go test -tags basic ./test/acceptance/ -run TestLoginAfterValidate401StartsBrowserFlowAcceptanceandgenerate-reference --checkall pass.One thing I noticed but did not touch:
go vetreportspkg/login/interactive_login.go:162: misuse of unbuffered os.Signal channel as argument to signal.Notify. Pre-existing on your branch and unrelated to the migration, but worth someone picking up - an unbuffered channel there can drop the signal.