[cristim]

Follow-up to #1531. #1542 made the Settings form refuse a save that leaves no provider enabled. The API still accepted one. Per the owner's decision, PUT /api/config now refuses it too.

What changes

• updateConfig (internal/api/handler_config.go) returns 400 enabled_providers must list at least one provider when the body carries enabled_providers and it decodes to an empty list. That covers [], null, and any key casing json.Unmarshal accepts. The refusal runs inside the locked read-modify-write, so nothing is saved, and it comes before the arming-grant checks.
• The rule depends on whether the key is in the body, and it is not part of GlobalConfig.Validate. A fresh install stores [], and every partial save that leaves the key out keeps working. The same reasoning is why ri_exchange_mode is checked only when sent.
• The OpenAPI description of enabled_providers states the write rule. It has no minItems, because a GET on a fresh install can still return [].

Why the two meanings matter: an empty list means "every provider" to the recommendation readers (handler_recommendations.go ~195-216), but "collect from none" to the scheduler (scheduler.go ~334).

Breaking change

An API client that cleared every provider with "enabled_providers": [] now gets a 400.

Tests

TestHandler_updateConfig_EnabledProvidersWrite covers five cases:

• [], null and a mixed-case key are refused with no save.
• An omitted key on a stored [] row is accepted.
• ["aws"] is accepted.

With the handler change reverted, the three refusal cases fail.

🤖 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 47 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: fix/config-refuse-empty-enabled-providers
• File: .coderabbit.yaml
• Commit: a64c9e6
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: fa8b33ab-3c4f-439b-822d-b1647730a785

Commits

Reviewing files that changed from the base of the PR and between b139db1 and a64c9e6.

Files selected for processing: 3

• internal/api/handler_config.go
• internal/api/handler_config_test.go
• internal/api/openapi.yaml

• 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

CodeRabbit Verify

• Run CodeRabbit Verify to test the changes in this pull request.

[cristim]

Independent review 2 (claude-fleet, cold): APPROVE 86b8966

I reviewed this cold, on exports of 86b8966 and its merge base b139db1, treating the PR body as a claim. handler_config.go is correct for the owner's 2026-11-03 rule. A test-only follow-up, a64c9e6, gets its own delta verdict.

Checked

• Presence detection (internal/api/handler_config.go:77, bodyHasKey at :151)

  • The key is matched with strings.EqualFold over the parsed top-level keys, the same matching json.Unmarshal uses for struct fields.

  • The check uses the merged value after decoding (:184), so the decoder settles duplicate keys in its own order: the last one wins.

  • I ran scratch cases through updateConfig at head:

    Body	Stored	Result
    {"enabled_providers":["aws"],"enabled_providers":[]}	["aws"]	400, no save
    {"enabled_providers":[],"enabled_providers":["gcp"]}	["aws"]	saved ["gcp"]
    {"enabled_providers":["aws"],"ENABLED_PROVIDERS":[]}	["aws"]	400, no save
    {"ENABLED_PROVIDERS":[],"enabled_providers":["gcp"]}	["aws"]	saved ["gcp"]
    {"enabled_providers":["aws"],"Enabled_Providers":null}	["aws"]	400, no save
    {"ENABLED_PROVIDERS":["aws"]}	[]	saved ["aws"]
    {}	[]	saved, providers unchanged
    {"laddering_enabled":true}	nil	saved, providers unchanged

  • A key spelled with the Unicode long s (enabled_providerſ) also folds to the name and is refused. That is the conservative side.

• Ordering
  • requirePermission (:66) and requireGlobalConfigScope (:78) run before the closure, so a caller without update:config or global scope gets 403 before any body validation. TestHandler_updateConfig_RequiresUpdateConfigPermission already covers the permission case.
  • Inside the closure, the new check runs in validateGlobalConfigWrite before requireRIExchangeAutoModeGrant and requireSwitchGrants (:118-124). A malformed write therefore never reveals grant state, as the function's comment requires.
  • Scratch case {"enabled_providers":[],"auto_purchase_enabled":true}: 400 from the providers check, no save.
• Nothing saved on refusal
  • PostgresStore.UpdateGlobalConfigAtomic (internal/config/store_postgres.go:209) returns the closure's error before saveGlobalConfigWith and propagateChangedDefaults, and WithTx rolls back.
  • The PR test asserts no SaveGlobalConfig call through the mock's Get/apply/Save fallback (internal/mocks/stores.go:121).
• Fresh-install compatibility
  • The rule depends on the key being present and is deliberately not in GlobalConfig.Validate. A stored [] (store_postgres.go:265) still accepts partial writes that omit the key.
  • The merged-not-presence probe below breaks omitted_key_on_stored_empty_row_accepted and TestHandler_updateConfig_DefaultsPUTDoesNotRewriteServiceConfigs.
• Blast radius: every PUT /api/config writer
  • frontend/src/settings.ts:3655-3662 already refuses to send an empty list after rebaseMultiFieldKeys (fix(settings): an empty enabled_providers means all providers on the backend but none on the page #1531). The other api.updateConfig callers, ladder.ts:203 and recommendations.ts:848, never send the key.
  • The other UpdateGlobalConfigAtomic writer, handler_ri_exchange.go:2286, sets only the ri_exchange_* fields and is not affected.
  • The frontend tests that use enabled_providers: [] are GET fixtures. No e2e test or script writes it.
  • The full go test ./internal/api/ suite passes at head.
• The test fails on base. With head's handler_config_test.go on b139db1, TestHandler_updateConfig_EnabledProvidersWrite fails: base calls SaveGlobalConfig with the empty list, which the mock doesn't expect, so the binary panics in the first subtest.
• Mutation probes on an export copy, each restored and checked with cmp. All were killed:
  • Exact-case key match: killed by mixed-case_key_refused.
  • Treating null as not sent: killed by null_refused.
  • providersSent forced false: killed by empty_list_refused.
  • Checking the merged value regardless of presence: killed by omitted_key_on_stored_empty_row_accepted and the defaults-propagation test.
• OpenAPI: the enabled_providers description matches the behaviour, including that a GET on a fresh install can return [].
• go vet ./internal/api/ is clean.

Non-blocking

None.

Main's red checks (the frontend date test and the x/crypto scan) are unrelated to this diff. Not merging.

[cristim]

Independent cold review A (claude-cc-platform's reviewer agent, did not write the change): APPROVE at a64c9e6d6f2a890833ade84da7bee552050a3da2 (pass 1 at 86b8966, pass 2 at the test-only delta).

What I attacked:

• Presence detection. I tried a mixed-case key, duplicate keys in both orders (the last one wins, as in json.Unmarshal), the U+017F long-s spelling, null, a non-array value, and an empty list combined with a bad ri_exchange_mode. Each result matches what the decoder does.
• Ordering. A caller without update:config gets 403, not 400. A refusal inside the closure stops UpdateGlobalConfigAtomic (store_postgres.go:209) before it saves, so the transaction rolls back.
• Blast radius. None of the other writers can send an empty list:
  • settings.ts:3664 refuses an empty list before it sends (line 3659).
  • ladder.ts:203 and recommendations.ts:848 send other keys only.
  • handler_ri_exchange.go:2286 sets only the ri_exchange_* fields.
  • cmd/, scripts/ and tests/ never send the key.
• OpenAPI. openapi.yaml:6634 matches the behaviour.
• Mutation probes on scratch copies, all caught by a failing test:
  • removing the check;
  • matching only the exact key case;
  • dropping the presence gate;
  • not counting null as present;
  • returning 500 instead of 400;
  • running the check before requirePermission.

Pass 1 found that the refusal tests didn't assert the 400 code and didn't cover the duplicate-key or no-permission cases. a64c9e6 adds all three. One older issue the diff doesn't touch is filed as #1658: the grace_period_days presence check uses the exact key case. updateConfig is at gocyclo 10, which is exactly the limit.

Local verification (macOS, at 86b8966; a64c9e6 adds tests only): go test -race ./internal/api/ and go test -tags integration ./internal/api/ (real Postgres) pass. At a64c9e6, go test ./internal/api -run TestHandler_updateConfig passes. Nothing was applied to any cloud.

[cristim]

Local verification at a64c9e6d6f2a890833ade84da7bee552050a3da2, merged onto current main ad19db5e (macOS). This re-checks the PR on the newer base, because PR CI ran on an older one.

• go build ./..., go vet -tags integration ./... and go test -race -short ./... pass on the full suite.
• go test -tags integration ./internal/api passes on real Postgres (testcontainers).
• Nothing was applied to any cloud.

CodeRabbit hit its review rate limit on this PR and left no findings.

[cristim]

Independent review 2 (claude-fleet, cold): APPROVE a64c9e6

Delta review of 86b8966..a64c9e6, on top of my APPROVE at 86b8966. The delta is test-only: git diff --name-only lists only internal/api/handler_config_test.go, and handler_config.go has the same blob at both SHAs.

Checked (at a64c9e6)

• The new assertions are real. On every refusal, TestHandler_updateConfig_EnabledProvidersWrite now requires a ClientError with code 400. A probe that returns 422 instead fails all four refusal subtests.
• The duplicate-key case {"enabled_providers":["aws"],"ENABLED_PROVIDERS":[]} passes at head and is refused with no save.
  • On its own it adds no kill power over the existing cases: an exact-case key match still sees enabled_providers in that body.
  • The mixed-case and null subtests are what fail the exact-key and first-key probes.
  • It does pin the last-key-wins decode order, which my scratch cases on 86b8966 also covered.
• The 403 case: TestHandler_updateConfig_EmptyEnabledProvidersWithoutPermissionIs403 passes. A caller with no permission sending [] gets 403 with no save, which pins that the permission check comes before body validation.
• The new tests fail on base. With head's test file on b139db1, TestHandler_updateConfig_EnabledProvidersWrite fails: base saves the empty list, which the mock doesn't expect.
• Probes, on an export copy, each restored and checked with cmp. All were killed:
  • 400 changed to 422;
  • exact-case key match;
  • presence counted only when the exact key's raw value is [].
• go vet ./internal/api/ is clean.

Non-blocking

None. Not merging.