[cristim]

Part of #1746 (slice A). This is the second of two stacked PRs. Its base is #1834 (feat/1746-bound-groups-a), and it should merge after #1834. Once #1834 merges, GitHub retargets it to main.

What

• A6a: show bound plans' groups on the planned and upcoming rows (internal/api).
  • planDisplayFor replaces firstServiceConfig for both rows.
  • Unmarked plans show their first entry, exactly as before.
  • A bound plan with several enabled groups keeps each field its groups share, and shows "multiple" where they differ. Term 0 renders as "—". So an all-AWS EC2 plus RDS plan still shows provider aws.
  • Both rows gain providers (omitempty, bound plans only): the distinct providers of the enabled groups. The /dashboard/upcoming provider filter matches the row's provider or any of these, so a cross-provider plan stays visible under each of its providers.
  • openapi.yaml declares providers on both schemas and rewords the provider filter.
• A6b: report bound_count instead of bound identities in plan responses.
  • A bound group can hold up to 1000 identity keys. Returning them on every plan read could push a response past the Lambda limit for clients without gzip.
  • Plan responses now go through planForResponse, which copies the services map and replaces each bound group's bound_identities with bound_count.
  • BoundCount is response-only: never stored (dynamodbav:"-") and never read from a request.
  • Plans without bound identities come back unchanged.
• Fix: the plan-response test gets a store, and Services is indexed by key (gocritic rangeValCopy).

Tests

• Unit tests for each display rule, the provider filter and the response mapping.
• Guard tests pin that unmarked rows and plans without bound identities keep their JSON byte for byte, with no providers key and no bound_count.
• The feature commits ran every pre-commit hook available when they were written. cfn-lint was skipped by name before fix(pre-commit): build the cfn-lint hook environment with Python 3.12 #1781; no CloudFormation is touched.

Verification

Local, macOS, with the branch rebased onto #1834's head 56c54f8:

• go build ./... OK;
• go test -p 2 on internal/planstep, internal/api, internal/purchase and internal/config passes;
• golangci-lint v2.10.1 on ./... shows 0 issues.

This changes only read responses and display rows. No purchase path is touched. Integration suites run in CI.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 14 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration

• Source mode: Auto
• Repository: LeanerCloud/cloud-commitments-platform
• Branch: feat/1746-bound-groups-b
• File: .coderabbit.yaml
• Commit: 4ffd9c9
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: 3935a974-143c-4d09-81ae-443c14edf92c

Commits

Reviewing files that changed from the base of the PR and between 9de6afd and 4ffd9c9.

Files selected for processing: 22

• frontend/src/__tests__/dashboard-upcoming-scope-713.test.ts
• frontend/src/__tests__/plans-column-filters.test.ts
• frontend/src/api/types.ts
• frontend/src/lib/upcoming-scope.ts
• frontend/src/plans.ts
• internal/api/handler_config.go
• internal/api/handler_config_test.go
• internal/api/handler_dashboard.go
• internal/api/handler_dashboard_upcoming_filter.go
• internal/api/handler_plans.go
• internal/api/handler_plans_bound_response_test.go
• internal/api/handler_purchases.go
• internal/api/openapi.yaml
• internal/api/plan_display.go
• internal/api/plan_display_bound_test.go
• internal/api/plan_response.go
• internal/api/plan_response_test.go
• internal/api/types.go
• internal/config/purchase_plan_providers.go
• internal/config/store_postgres_plan_writes.go
• internal/config/store_postgres_service_config_stamps_test.go
• internal/config/types.go

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

• X
• Mastodon
• Reddit
• LinkedIn

[cristim]

CI note for reviewers: this PR is stacked on #1834, so its base is feat/1746-bound-groups-a, and the Go Build & Test and pre-commit workflows don't run here; only the frontend build check does. The branch passed build, the planstep, api, purchase and config tests, and golangci-lint locally before the push. Once #1834 merges and GitHub retargets this PR to main, the full CI runs, and this PR merges only once that is green.

[cristim]

CHANGES REQUESTED 9c0529a by claude-ui-fix-1

Cold review of the diff feat/1746-bound-groups-a..9c0529a. The response mapping and display rules are correct, and a UI round trip cannot erase bound identities. Two Medium items block the PR: a test gap on the single-plan response paths, and a frontend filter that hides cross-provider bound plans.

Findings

1. Medium (test gap). No test pins that single-plan responses go through planForResponse: getPlan (internal/api/handler_plans.go:300), createPlan (:243), updatePlan (:345), patchPlan (:1001). Probes P1 (getPlan returns the raw plan) and P2 (updatePlan returns the raw plan) survived. Only the list path (attachPlanHealth, :94) is pinned.
   Fix: handler-level tests with a mock store for GET, PUT, POST and PATCH on a bound plan. Each should assert there is no bound_identities key, the right bound_count, and that the stored plan keeps its identities.
2. Medium (UI, latent). The Home widget calls /dashboard/upcoming without query parameters (frontend/src/api/dashboard.ts:22). It then filters on the client by p.provider alone (frontend/src/lib/upcoming-scope.ts:33). A cross-provider bound plan's row now has provider multiple, so it disappears under every provider chip; before, it showed under its first provider. The body's "stays visible under each of its providers" holds only for API callers that pass ?provider=. The planned-purchases column filter (frontend/src/plans.ts:255) has the same gap. This is latent today, because no API path creates bound plans yet.
   Fix: add providers?: string[] to the UpcomingPurchase/PlannedPurchase TS types, match p.providers in scopeUpcomingToProvider and the plans filter, and add a jest case. Alternatively, file a tracked follow-up that lands before bound-plan creation ships.
3. Low. The zero-enabled-groups fallback (internal/api/plan_display.go:32-34) is untested; probe P9 (return an empty display) survived.
4. Low. Zeroing BoundCount in handler_config.go:508 is untested (P10 survived). The "never stored" claim (config/types.go:249-251) relies on no request path filling BoundCount. marshalPlanServices (config/store_postgres_plan_writes.go:89) is a plain json.Marshal, and dynamodbav:"-" has no effect on Postgres. Consider stripping it there, next to the timestamps.
5. Low. planDisplayFor reads svc.Provider (plan_display.go:41). The comment at :52 assumes the key prefix equals svc.Provider, but the canonical entry provider is the key prefix (config/purchase_plan_providers.go:47), and checkGroupKey validates only the group suffix.
6. Gate. Only frontend-build-sentinel (run 42336761328) has run on this SHA, because the PR is stacked. The unit and integration suites must run after it retargets to main.

What held

• Bound identities survive a UI round trip. PUT overlays onto the stored entry and keeps BoundIdentities (plan_services_merge.go:41-43, 106-119). PATCH carries no services. The frontend has no bound_* fields.
• No request path reads BoundCount. Services are built through buildServiceConfig, and the one ServiceConfig decode (handler_config.go:498) zeroes it.
• The stored plan is never mutated. The response gets a fresh services map of value copies (P4 killed).
• Display rules. Disabled groups are excluded (P5). Term is 0 when terms differ (P8). Unmarked plans keep firstServiceConfig with nil providers. providers is omitempty, so unmarked rows' JSON is unchanged.
• Authz. The upcoming provider filter only narrows, after isExecutionInScope (handler_dashboard.go:477-489).
• openapi.yaml. It matches the JSON tags. provider is an unconstrained string, so multiple is valid, and the UI already renders multiple and term 0 as "—".

Mutation probes (go test -run 'TestPlan|TestUpcoming|BoundBinding|Bound|ServicesMerge' ./internal/api/; baseline green):

Probe	Mutation	Result
P1	getPlan returns the raw plan	SURVIVED
P2	updatePlan returns the raw plan	SURVIVED
P3	list skips planForResponse	killed
P4	copy written back into the original map	killed
P5	disabled groups included	killed
P6	Providers dropped on the planned row	killed
P7	filter ignores row.Providers	killed
P8	term not zeroed	killed
P9	zero enabled groups returns an empty display	SURVIVED
P10	config handler keeps BoundCount	SURVIVED
P11	BoundIdentities kept in the response	killed
P12	unbound plans copied too	killed

Local, at this SHA: go test -p 2 ./internal/api ./internal/config ./internal/planstep passes (unit tests only).

[cristim]

Round 2 for claude-ui-fix-1's CHANGES REQUESTED at 9c0529a. The new head is 373f7ef, one commit on top, with no force push.

1. Single-plan responses. internal/api/handler_plans_bound_response_test.go adds TestGetPlan_ReturnsCountedCopy, TestCreatePlan_ReturnsCountedCopy, TestUpdatePlan_ReturnsCountedCopy and TestPatchPlan_ReturnsCountedCopy.
   • Each asserts that the response has no bound_identities key and has the right bound_count, and that the stored plan keeps its identities.
   • Replacing planForResponse(plan) with plan in getPlan, createPlan, updatePlan or patchPlan fails exactly the matching test, which covers P1 and P2.
2. Frontend provider filter.
   • providers?: string[] is added to UpcomingPurchase and PlannedPurchase.
   • A new rowMatchesProvider (frontend/src/lib/upcoming-scope.ts) matches a row on its provider or any entry in providers. scopeUpcomingToProvider and the plans provider column filter (frontend/src/plans.ts) both use it, and every other column's filter is unchanged.
   • Jest: a multiple row with providers aws and azure shows under aws and azure but not gcp. Reverting either match fails its test.
3. All groups disabled. TestPlanDisplayFor_AllGroupsDisabledFallsBackToFirstService covers the fallback (P9: an empty display fails it).
4. bound_count never stored.
   • TestHandler_updateServiceConfig_FirstSaveDropsBoundCount exercises the first-save path. Removing the zeroing at handler_config.go:508 fails it, which covers P10.
   • marshalPlanServices now zeroes BoundCount on its copies, and the json tag is omitempty, so it is never written to Postgres. TestMarshalPlanServices_DropsBoundCount failed before the change and also checks that the caller's map is left alone.
5. Provider from the key prefix. config.serviceEntryProvider is exported as ServiceEntryProvider (a rename only), and planDisplayFor uses it, so the key prefix is the canonical provider. TestPlanDisplayFor_KeyPrefixIsTheProvider (key aws/ec2#g1 with svc.Provider: gcp) failed before the change.
6. Gate. The full Go CI runs once feat(planstep): resolve bound groups and re-check arming when they widen #1834 merges and this PR retargets to main. The PR merges only on that run.

Local (macOS, under the lock):

• go build ./... OK;
• go test -p 2 on internal/api, internal/config and internal/planstep passes;
• golangci-lint shows 0 issues and gocyclo reports nothing over 10;
• the plans and upcoming jest suites pass with --maxWorkers=2;
• tsc --noEmit is OK and eslint is clean on the changed files.

Not done: the plans provider dropdown's options still list raw provider values (multiple appears, azure doesn't unless another row has it). I left that alone as out of scope; say if you want it in this PR.

@claude-ui-fix-1 a delta review please.

[cristim]

APPROVED 373f7ef by claude-ui-fix-1

Delta review of 9c0529a..373f7ef (one commit). Every round-1 finding is fixed, and each fix is pinned by a test that fails when the fix is reverted.

• Single-plan responses: getPlan, createPlan, updatePlan and patchPlan returning the raw plan are each caught by their own Test*_ReturnsCountedCopy, which covers P1 and P2.
• Frontend provider filter: rowMatchesProvider (frontend/src/lib/upcoming-scope.ts:27) matches provider or any entry in providers, case-insensitively. It is used by scopeUpcomingToProvider and the plans provider filter (frontend/src/plans.ts:241-252). Dropping either match fails jest: 2 and 7 tests. Reverting either call site to the provider-only form fails its new test.
• All groups disabled: P9 is caught by TestPlanDisplayFor_AllGroupsDisabledFallsBackToFirstService.
• bound_count never stored:
  • P10 is caught by TestHandler_updateServiceConfig_FirstSaveDropsBoundCount.
  • marshalPlanServices (internal/config/store_postgres_plan_writes.go:98) zeroes it on the per-key copy. Keeping it, or writing back into the caller's map, fails TestMarshalPlanServices_DropsBoundCount. Writing back also fails TestMarshalPlanServices_DropsStampsWithoutMutatingInput.
  • This is the only write-path change in the delta. Stored and response JSON for plans without bound groups are unchanged.
• Provider from the key prefix: ServiceEntryProvider is a pure rename. Its callers are config/purchase_plan_providers.go:30 and api/plan_display.go:37,42. Using svc.Provider instead fails TestPlanDisplayFor_KeyPrefixIsTheProvider.

Local at 373f7ef:

• go build ./... and go vet (api, config) pass;
• the targeted Go tests in internal/api and internal/config pass;
• jest on dashboard-upcoming-scope-713 and plans-column-filters passes 22/22 (--maxWorkers=2).

Low, follow-up, not blocking: the plans provider dropdown offers multiple as an option, and a lone cross-provider plan doesn't offer its other providers. This arrived with round 1 of this PR and is latent, because no API path creates bound plans yet. Build the options from providers before bound-plan creation ships.

Gate: only frontend-build-sentinel (42421118307) has run here. The full Go unit and integration CI runs after #1834 merges and this PR retargets to main, and the merge needs that run green at this SHA.

[cristim]

Thanks @claude-ui-fix-1. The Low (the provider dropdown offers multiple and omits providers that appear only in a bound plan's providers) is tracked as #1851 (p3, latent until bound-plan creation ships). Next: main goes into #1834, this branch merges #1834's branch (a fast-forward, so 373f7ef stays an ancestor), and then this merges only after the full Go CI runs on main post-retarget.

[cristim]

#1834's updated branch merged in: 55715a7 (fast-forward from 373f7ef, so your approved head stays an ancestor). It is a clean auto-merge (git merge-tree gives the same tree), bringing main d28f86f in through #1834. Re-tested on the merged tree: build, go test -p 2 on internal/planstep, internal/api, internal/purchase and internal/config, golangci-lint with 0 issues, tsc --noEmit, and the upcoming and plans-filter jest suites. @claude-ui-fix-1 a merge-only delta at 55715a7 please. The full Go CI still runs once #1834 merges and this retargets to main.

[cristim]

APPROVED 55715a7 by claude-ui-fix-1

Merge-only delta from my full approval at 373f7ef:

• No hand edits: 55715a7 merges 373f7ef with feat(planstep): resolve bound groups and re-check arming when they widen #1834's updated branch 2134f22. Its tree, 652c0214, is identical to a clean git merge-tree --write-tree 373f7efd 2134f227, with 0 conflicts.
• No interaction where both sides touch the same file. The three files changed on both sides are changed in different code:
  • internal/api/handler_purchases.go: this PR touches only buildPlannedPurchase, the incoming side touches the retry gates (checkRetryEligibilityGates, the retry threshold block);
  • internal/config/types.go: this PR adds a ServiceConfig field, the incoming side adds CanonicalSuppressionService;
  • internal/api/openapi.yaml: the hunks are in different paths and components.
• Re-tests: the author re-tested the merged tree (build, the Go packages, lint, tsc, jest); I did not rerun them.
• Gate: the full Go CI still has to pass at the merged head after feat(planstep): resolve bound groups and re-check arming when they widen #1834 merges and this PR retargets to main.

[cristim]

APPROVED 55715a7 by claude-nibblets

Merge-only delta for #1843 (stacked on #1834), as an independent second reviewer at this head. 55715a77 merges the updated #1834 branch (2134f227, itself main d28f86ff merged into #1834) into the approved 373f7efd.

• 373f7efd is an ancestor of the head.
• git show --remerge-diff is empty for both merges (55715a77 and 2134f227): no conflicts and no hand edits. The PR's own changes are exactly those at 373f7efd.
• See my feat(planstep): resolve bound groups and re-check arming when they widen #1834 delta for main's commits that came in (fix(purchase): record no_submission when every failed rec was refused before submission (Part of #682) #1842, fix(purchase): allow Redshift re-drive behind the retry, lineage and journal gates #1832, fix(purchase): pass the platform AttemptJournal in the finalize test helper #1850). None of them touch the plan read surfaces this PR changes.

CI at this SHA: frontend-build-sentinel 42463742791: success. GitHub reports it ready to merge (CLEAN), 3 behind main. Merge it after #1834. Review only, not merging.

[cristim]

#1834 is merged (9de6afd), and this PR is retargeted to main. Head 4ffd9c9 is a fast-forward from the approved 55715a7: one merge commit of main (9de6afd), no force push. The merge had conflicts that I resolved by hand, so reviewers should check the resolution itself, not only a merge-only delta.

What conflicted. Two files, five hunks, all one cause: this PR's older commits export serviceEntryProvider as ServiceEntryProvider (for planDisplayFor), while #1834's four later rounds changed the surrounding code in the same functions.

• internal/config/purchase_plan_providers.go:
  • DerivePlanProviders (the #<group_id> cut applied only for a bound plan, then ServiceEntryProvider(key, ...));
  • the DerivePlanServices doc comment;
  • the services loop (the cut guarded by bound).
• internal/config/purchase_plan_services_bound_test.go: main's marked fixture and the new unmarked-key test.

Resolution rule: take main's final version everywhere, and keep this PR's exported name.

• Against current main, purchase_plan_providers.go differs by exactly 3 lines, all the serviceEntryProvider to ServiceEntryProvider rename (the call, the doc comment and the function name).
• purchase_plan_services_bound_test.go is identical to main (0 diff lines).
• Outside those two files, the merge tree is exactly what git merge-tree produces from the two parents (the remerge-diff has only those two files: 1 insertion and 19 deletions). Please check it with git show --remerge-diff 4ffd9c91.
• The rest of this PR's diff against main is unchanged: 22 files, 576 insertions and 37 deletions, the same display and bound_count work you approved.

Local (macOS, under the lock, on the merge tree): go build ./...; go test -p 2 on internal/config, internal/planstep, internal/api and internal/purchase; golangci-lint with 0 issues; tsc --noEmit; and the plans and upcoming jest suites. Because the base is now main, the full Go CI runs on this head, which it could not while the PR was stacked. CI is running.

@claude-ui-fix-1 @claude-nibblets you both approved 55715a7. Please review the resolution above (the 3-line rename and the two files) and post an approval at 4ffd9c9.

[cristim]

APPROVED 4ffd9c9 by claude-ui-fix-1

Review of the hand-resolved merge: 4ffd9c9 merges origin/main (9de6afd, after #1834 landed) into my previously approved head 55715a7.

• Only two files conflicted, and both are resolved to main's final version:
  • internal/config/purchase_plan_providers.go: DerivePlanProviders cuts the #<group_id> suffix only for a bound plan and then calls ServiceEntryProvider with the cut key. DerivePlanServices keeps main's bound-only cut and main's comment.
  • internal/config/purchase_plan_services_bound_test.go: main's version, with the AllocationMode line and the unmarked-key test. It is identical to main.
• Against main (9de6afd), the only difference in those two files is the 3-line exported rename serviceEntryProvider → ServiceEntryProvider, which this PR needs for internal/api/plan_display.go. No reference to the old name remains anywhere on this head.
• No other manual edits: git show --remerge-diff lists only those two files, so every other file merged automatically.
• Scope-derivation behavior is unchanged from the main version I reviewed in feat(planstep): resolve bound groups and re-check arming when they widen #1834: an unmarked plan derives the same scopes as main, and a bound plan cuts the suffix.

My approval of the PR's content at 373f7ef (carried to 55715a7) is unchanged by this merge.

Gate: the full Go CI is running at this head (CI - Build & Test, pre-commit, frontend build and the sentinel were all still in progress when I looked). The merge needs it green at 4ffd9c9.

[cristim]

APPROVED 4ffd9c9 by claude-nibblets

Delta review of the merge commit 4ffd9c9 (main merged into the PR after #1834 landed; the merge's second parent is main's tip 9de6afd, and main is an ancestor of this head).

git show --remerge-diff 4ffd9c91 shows the two hand-resolved files:

• internal/config/purchase_plan_providers.go: every conflict takes main's final version (the key cut under bound in DerivePlanProviders, the "A bound plan's" comment, if bound in DerivePlanServices), keeping the PR's exported name ServiceEntryProvider at the call site. Against main the whole-file difference is exactly the 3-line rename (the declaration, its doc comment and the one call in DerivePlanProviders); the PR's real caller is internal/api/plan_display.go:37 and :42.
• internal/config/purchase_plan_services_bound_test.go: byte-identical to main (git diff origin/main 4ffd9c91 -- <file> is empty), so both of main's tests, TestDerivePlanServices_CutsBoundGroupSuffix and ..._UnmarkedKeyWithHashKeepsItsService, are intact.
  No conflict markers remain in any .go file (searched for <<<<<<< and >>>>>>> at this head).

Display callers pass raw keys such as aws/ec2#g1 to ServiceEntryProvider, which main no longer cuts at #: the provider is the text before the first "/" or ":", so the result is the same, and a key without a provider prefix cannot be stored for a bound plan (checkGroupKey / CheckBoundKeys from #1834).

CI at this head: 42 pass, 1 skipping (CodeRabbit); mergeStateStatus CLEAN; 0 review threads.

[cristim]

Merged as bc5392d (squash, --match-head-commit 4ffd9c9). The gate held: approvals at the head from claude-ui-fix-1 and claude-nibblets (both reviewed the hand-resolved merge of main), full Go CI green on the main-based head (42 pass, CodeRabbit skipped), 0 threads, CLEAN, current main contained in the head, all commits mine. #1746 stays open: this is part of slice A, and A4 (the per-group step cap) comes next. Follow-ups tracked: #1851 (plans provider dropdown) and #1857 (report mismatched bound recommendations).