[cristim]

Summary

Part of #133 (backend). The frontend editor change follows in a separate PR, which will resolve the issue.

Owner decision on #133 (2026-10-16):

• An absent cap keeps meaning "no cap".
• The UI refuses 0.
• The API also rejects an explicit max_amount: 0 with a 400.

Before this change, an admin who sent 0 meaning "may not spend" got an unlimited cap. PermissionConstraints.MaxPurchaseAmount is a plain float64, and 0 means no cap in every reader.

#940 (12c2663) already rejects negative and NaN caps on writes and fails closed on stored ones. This PR reuses its helpers in internal/auth/purchase_amount_cap.go and adds only the explicit-zero case:

• APIPermissionConstraint gets an unexported maxAmountSet flag, set by a custom UnmarshalJSON. The flag separates an explicit max_amount (including 0, 0.0, -0, 0e5) from an absent field or null. Marshal output is unchanged, so responses are identical. Go literals never set the flag and are never rejected.
• validateExplicitZeroCap runs in the shared apiPermissionsToPermissions, which now returns an error. It runs before checkGrantCeiling. ErrInvalidPermission is already a 400 via mapGroupAuthError. apiPermissionToPermission keeps its signature.
• UpdateGroupAPI is split into an updateRequestPermissions helper to stay under gocyclo 10. The order is unchanged: system-managed refusal, empty-permissions check, conversion and zero check, then the ceilings.
• openapi.yaml: PermissionConstraint.max_amount gets minimum: 0 and exclusiveMinimum: true (the OpenAPI 3.0.3 form). The description says 0 is rejected on group writes and that absent means no cap.
• CHANGELOG entry.

Scope: group writes only. The HTTP API-key create path decodes into api.Permission, which erases an explicit 0 before the auth layer sees it. That gap is tracked in #980, and the wording here does not claim key coverage.

Tests

• internal/auth/explicit_zero_cap_test.go:
  • 0, 0.0, -0 and 0e5 are rejected on create and update.
  • Absent, null, 5000 and 1234.5 are accepted.
  • Marshal never emits the flag.
  • All 8 rejection subtests failed on unfixed code.
• internal/api/group_explicit_zero_cap_test.go: drives the real createGroup and updateGroup handlers (the router calls them directly; the body decode is in the handler) with a real auth.Service on MockStore.
  • POST and PUT with 0 or 0.0 return 400 with the message.
  • Absent and 5000 are accepted.
  • 4 subtests failed before the fix.
• Mutations, all caught:
  • drop the set-flag conjunct;
  • treat null as set;
  • skip the check on update or on create;
  • reject positive values too.

Verification (local, macOS)

• go build ./... and go vet ./... pass.
• go test -race -count=1 ./internal/auth/... ./internal/api/... ./internal/server/... passes, and the test(auth): fail when an API adapter drops a request field #923 field-coverage tests still pass.
• GOTOOLCHAIN=go1.26.9 go run github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.10.1 run on auth and api: 0 issues. The pre-commit gocyclo hook passes.

Review

• Plan: plans/133-spend-cap-zero.md, reviewed independently through several revisions. It was re-scoped after fix(auth): fail closed on a negative MaxPurchaseAmount spend cap #940 and fix(auth): keep API key permission constraints on create #957.
• Diff: independent non-author review, two passes, approved at 0ad75eb.

🤖 Generated with Claude Code

[cristim]

@coderabbitai review

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 7 billable files and costs up to $1.75.}

• Ask an admin to make reviews automatic

Open in CodeRabbit

Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing.

Or wait 3 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available. Your 64 included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

• Configuration used: Repository: LeanerCloud/cloud-commitments-platform/.coderabbit.yaml
• Review profile: CHILL
• Plan: Essentials
• Run ID: c262853c-f9d6-4f09-8c4e-20d9f90bc24f

📥 Commits

Reviewing files that changed from the base of the PR and between 4a3fa9c and 84a8218.

📒 Files selected for processing: 7

• CHANGELOG.md
• internal/api/group_explicit_zero_cap_test.go
• internal/api/openapi.yaml
• internal/auth/explicit_zero_cap_test.go
• internal/auth/purchase_amount_cap.go
• internal/auth/service_api.go
• internal/auth/service_apikeys_api.go

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

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[cristim]

Independent review of 0ad75eb: changes requested

Findings

1. BLOCKING (gate): PR body line "The frontend editor change follows in a separate PR, which closes #133" makes GitHub list #133 under closingIssuesReferences (verified via gh pr view). Merging this backend-only PR auto-closes #133 before the UI half lands. Fix: reword the body to "Refs #133" / "Part of #133" with no closing keyword, then re-check that closingIssuesReferences is empty. No code change, so no new SHA.
2. NON-BLOCKING: HTTP API-key create is not covered (api.Permission erases the explicit 0 in handler_apikeys.go:60-74 before auth sees it). Honestly disclosed in the body, CHANGELOG and openapi, tracked in #980. Note the openapi exclusiveMinimum is on the group schema only; key creation uses a different schema, so no false contract claim.

What I attacked

• Explicit vs absent survives decode: UnmarshalJSON (service_api.go:69) sets maxAmountSet from a *float64 probe, so 0, 0.0, -0, 0e5 are set; absent and null are not. Quoted "0" fails the float64 decode as before (400). No handler uses DisallowUnknownFields or UseNumber on these types, so the inner json.Unmarshal loses no decoder options. Marshal is untouched (omitempty), so GET output re-PUT round-trips.
• Paths: create (service_api.go:341), update (updateRequestPermissions, order unchanged: system-managed, empty-permissions, zero check, then checkGrantCeiling) and CreateAPIKeyAPI (service_apikeys_api.go:213) all go through the one converter. Handlers decode with json.Unmarshal into the auth types (handler_groups.go:47,110).
• Stored rows: no migration and no read-path change; stored 0 still means no cap on reads (validateMaxPurchaseAmount and #940's negative/NaN handling untouched, same ErrInvalidPermission 400 mapping). Intended per the owner decision; existing zero-cap groups stay unlimited and only a re-write is refused.
• Fail-before/pass-after: with production files reset to origin/main, all 8 TestGroupWrite_RejectsExplicitZeroMaxAmount subtests and the API-key test fail; at the SHA go test ./internal/auth/... ./internal/api/... passes (4319 tests).

Gate status

• Independent review: this comment, names the SHA, one blocking gate item above.
• CI on the SHA: still pending at review time (Unit, Lint, pre-commit, E2E); CodeRabbit is rate limited with no threads. Not yet green.
• Labels: priority/p2, urgency/this-sprint, severity/high, impact/few, effort/s, type/bug, triaged. I did not diff them against #133, so that is unverified here.
• Local verification: auth and api tests with fail-before proof, run on macOS. No real-account verification.

[cristim]

CI on 0ad75eb is red for a reason in this PR. Unit and Integration both fail TestNoUnfailableMockAssertions (internal/mocks):

• internal/api/group_explicit_zero_cap_test.go:76:4: store.AssertNotCalled(t, "CreateGroup") -- receiver type could not be r[esolved]
• internal/api/group_explicit_zero_cap_test.go:77:4: store.AssertNotCalled(t, "UpdateGroup") -- ...

The guard rejects AssertNotCalled on a receiver whose mock type it can't resolve, because such an assertion can never fail. Declare store with the concrete mock type that the other tests in internal/api use, or use the repo's existing pattern for not-called assertions. Then confirm go test ./internal/mocks/ ./internal/api/ passes locally.

Separately, as noted on the bus: GitHub still lists #133 as a closing issue. The sentence "...in a separate PR, which closes #133" triggers that. Please reword it, e.g. "Refs #133; the frontend PR will close it", so that merging this backend half doesn't close #133.

The independent code review (no code defects) covers 0ad75eb. A new commit gets a delta review.

[cristim]

CI fix at 84a8218 (test-only, +6/-1): TestNoUnfailableMockAssertions could not resolve the receiver of two store.AssertNotCalled sites in the new group_explicit_zero_cap_test.go, because the guard only resolves new(T) with a bare type name. The test now declares type MockStore = auth.MockStore and uses new(MockStore). Both assertions are kept, and CreateGroup and UpdateGroup exist on auth.MockStore. Locally the guard passes with and without -tags integration, and go test -race ./internal/api/... passes. The production diff is unchanged from the reviewed 0ad75eb. [claude-cc-impl]

[cristim]

Independent delta review of 84a8218 (previous: 0ad75eb): changes requested

OLD_SHA is an ancestor of NEW_SHA. The delta is one file, internal/api/group_explicit_zero_cap_test.go (+6 -1): type MockStore = auth.MockStore and new(MockStore) at line 61 (was new(auth.MockStore)).

Findings

• BLOCKING (gate): gh pr view 983 --json closingIssuesReferences still lists #133. The body still says "The frontend editor change follows in a separate PR, which closes #133", so merging this backend-only PR would auto-close #133 before the frontend work lands. Fix: reword so it does not use a closing keyword, for example "Part of #133; the frontend PR will close it", then re-check closingIssuesReferences is empty.
• RESOLVED: TestNoUnfailableMockAssertions failure. The alias gives the guard a bare type name to resolve for the AssertNotCalled sites at lines 76-77. go test ./internal/mocks/ passes.
• Code review at OLD_SHA found no defects; the delta changes no production code.

What I attacked

• Weakening: the assertions are unchanged and the alias is the same type as auth.MockStore, so semantics are identical.
• Probe 1: disabled validateExplicitZeroCap in internal/auth/purchase_amount_cap.go. All four explicit-zero subtests (POST and PUT, 0 and 0.0) fail (error expected but got nil).
• Probe 2: inserted direct store.CreateGroup and store.UpdateGroup calls before the assertions. Both AssertNotCalled calls fail ("Should not have called with given arguments") in every explicit-zero subtest. Both probes reverted; worktree clean.
• go test -count=1 ./internal/mocks/ ./internal/api/ ./internal/auth/... passes.

Gate status

• CI on NEW_SHA: most checks pass (pre-commit, terraform, Snyk and the other scope checks); Unit Tests, Lint Code, Integration Tests, E2E, Docker build and Security Scanning were still pending when I looked. No action_required runs seen. I did not wait.
• CodeRabbit: rate limited, no threads; not pinged.
• Closing reference: fails, see above. Verdict and this review are recorded here against NEW_SHA.

[cristim]

Merge driver status (claude-cc-pr-merge, 2026-10-17T15:25:27Z): code gate met, merge BLOCKED on the author: the body still says the frontend PR "closes #133", so GitHub lists #133 as a closing issue and would close it on merge; reword without a closing keyword (for example "Part of #133; the frontend PR will resolve it").

[cristim]

Body reworded: it now says "Part of #133" with no closing keyword, and GitHub's closing-issues list for this PR is empty. The head is unchanged at 84a8218 and all 24 checks are green. [claude-cc-impl]