[IAmJSD]

Root cause

PUT /api/accounts/{id} (updateAccount) built the stored row from the request alone through cloudAccountFromRequest, copying only ID, CreatedAt and CreatedBy from the existing row. CloudAccountRequest.Enabled was already a *bool, but a nil pointer still mapped to false. AWSIsOrgRoot was a plain bool, so "omitted" couldn't be told apart from "false" at all. An API client or Terraform provider that sent a PUT with a name and credentials but no enabled key therefore disabled the account without any error, and the same PUT cleared aws_is_org_root. The scheduler then skipped the account. The web UI always sends enabled, which hid this from UI users. This is audit finding A02-013 in the issue.

Fix

• internal/api/handler_accounts.go:
  • CloudAccountRequest.AWSIsOrgRoot is now *bool, like Enabled.
  • cloudAccountFromRequest applies it only when the request sets it.
  • A new keepOmittedAccountFlags helper, called only from updateAccount, copies the stored Enabled and AWSIsOrgRoot when the request omits them or sends null.
  • An explicit false still clears either flag.
• Create defaults are unchanged. createAccount and registration approval still treat an absent flag as false. Registration approval still overrides enabled from its credential check, and createSelfAccount still forces enabled=true.
• internal/api/openapi.yaml: the updateAccount description no longer says "enabled defaults to false". Both properties are now marked nullable and documented as "create: absent means false; update: absent or null keeps the stored value".

Consumers: the CLI and MCP sibling checkouts (cloud-commitments-cli at c07edc53, cloud-commitments-mcp at d49fb20) never call /api/accounts and never reference aws_is_org_root (grep, read-only), so nothing depended on the old clearing behaviour. frontend/src/settings.ts always sends enabled, and sends aws_is_org_root for AWS accounts, so UI saves behave as before.

Refs #151

Concurrency

No DB or store change. UpdateCloudAccount is still a whole-row overwrite, so two concurrent PUTs to one account are last-writer-wins on every column, the same as on main. With this change, a PUT that omits enabled writes back the value it read a moment earlier. If a concurrent PUT flips enabled in between, the omitting PUT can undo that flip, just as it would already undo a concurrent rename. Before this change the omitting PUT always wrote false, which is strictly worse. Making the fallback atomic would need a COALESCE-style store signature (*bool parameters in UpdateCloudAccount) or optimistic concurrency on updated_at. That is a wider change than this finding, so it is not done here; this note is so a reviewer can decide.

Verification

Unit/mock evidence. The handler is exercised through the real updateAccount / createAccount code against a capturing config-store mock that records the row passed to UpdateCloudAccount / CreateCloudAccount. This bug is in the request-to-row mapping, so no Postgres test was added. The SQL already writes whatever value the handler hands it.

New tests in internal/api/handler_accounts_update_keep_test.go:

• TestUpdateAccount_OmittedFlagsKeepStoredValues covers seven cases: omitted keeps true, omitted keeps false, null keeps true, explicit false clears, explicit true sets, and each flag omitted while the other is explicit false.
• TestUpdateAccount_ExplicitFalseAndOmittedDiverge follows the issue's verification rule: a real false and a genuine absence through the same path must store different values.
• TestCreateAccount_FlagDefaultsUnchanged checks that omitted and null give false/false on create, and explicit true gives true/true.

Fail-before, on main 7b86e38 with only the test file added:

$ go test ./internal/api/ -count=1 -run 'TestUpdateAccount_OmittedFlagsKeepStoredValues|TestUpdateAccount_ExplicitFalseAndOmittedDiverge|TestCreateAccount_FlagDefaultsUnchanged'
--- FAIL: TestUpdateAccount_OmittedFlagsKeepStoredValues (0.03s)
    --- FAIL: TestUpdateAccount_OmittedFlagsKeepStoredValues/omitted_keeps_true (0.01s)
    --- FAIL: TestUpdateAccount_OmittedFlagsKeepStoredValues/null_keeps_true (0.01s)
    --- FAIL: TestUpdateAccount_OmittedFlagsKeepStoredValues/enabled_omitted,_org_root_explicit_false (0.00s)
    --- FAIL: TestUpdateAccount_OmittedFlagsKeepStoredValues/enabled_explicit_false,_org_root_omitted (0.00s)
--- FAIL: TestUpdateAccount_ExplicitFalseAndOmittedDiverge (0.00s)
FAIL	github.com/LeanerCloud/cloud-commitments-platform/internal/api

Pass-after, on this branch: all three tests and all 10 subtests pass.

Other commands run on this branch, all clean (run with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true):

• go build ./...
• go vet ./...
• go vet -tags integration ./...
• go test ./internal/api/ -count=1: ok (51s)
• go test ./internal/mocks/ -count=1: ok
• golangci-lint run --allow-parallel-runners ./internal/api/ (v2.10.1): 0 issues
• gocyclo -over 10 -ignore "_test\.go" internal/api/handler_accounts.go: no output

Combined-tree verification. This branch was tested together with the other queued branches on a local, never-pushed tree: queue/platform @ 400d256b = origin/main 7b86e386 plus these heads, merged in this order:

Branch	Tested head
fix/issue-243-rest	007c163
fix/issue-1206	312072f
fix/issue-111-email	6d422e2
fix/issue-111-fail-closed	3817e29
fix/issue-712-age-cap	a91120b
fix/issue-712-fargate	aeaf0d4
fix/issue-613-legacy-ids	350e837
fix/issue-120-guards	6f38ebc
fix/issue-120-shared-predicate	cb25735
fix/issue-151-account-update-keep	d20cf01
fix/issue-713-upcoming-filter	e73fdfb
fix/issue-263-bastion-opts	7861f32
fix/issue-43-fixture-ids	8cee203
fix/issue-177-makefile-walk	365a9d6
fix/issue-27-seller-not-registered	d845f66
fix/issue-313-user-columns	6d759dd
fix/issue-353-mock-panics	aac2edf
fix/issue-8-config-stamps	b548ae5
fix/issue-30-monthly-totals	f637921

Linux, Go 1.26.9, AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true; Docker Postgres via testcontainers. The full suite ran on f7c48020, which differs from this tree only by test-only commits on fix/issue-30, fix/issue-713 and fix/issue-263 that replace placeholder account-number literals with named constants:

• Passed: go build ./..., go vet ./..., go vet -tags integration ./... and the full go test -race -short -count=1 ./....
• Static checks: golangci-lint v2.10.1 found 0 issues on every changed package, with and without --build-tags integration. gocyclo -over 10 is silent on the changed non-test files.
• Integration tests, every changed package that has them, one package at a time (-tags integration -count=1): internal/analytics, internal/api, internal/auth, internal/config, internal/database/postgres/migrations, internal/purchase, internal/scheduler and internal/server all pass. Migrations 000149, 000150 and 000151 apply in order on this tree.
• Frontend: the changed jest files (approval-details, settings-accounts, api) pass (221 tests).
• Secrets: the first run flagged 9 new fixture lines (placeholder account numbers next to an account key); fixed with named constants. Re-run on this tree: git-secrets, using the repo's scripts/setup-git-secrets.sh patterns in a fresh repo with an isolated HOME, found nothing in the changed files, and a planted fake key was flagged. scripts/test-git-secrets-allowlist.sh: 183 PASS, 0 FAIL.
• Terraform .tftest.hcl files were not run locally (CI runs them). Nothing was applied to any cloud.

Out of scope

• The rest of fix(money): zero-vs-absent conflation across frontend/internal money paths [platform part] #151 stays open:
  • A06-027 (assembleLadderPlan fabricating a $0 existing_usd_hr), which is held in fix(server): report the ladder plan's net existing commitment #1461.
  • The frontend items, which belong to the UI owner.
• Making the account update atomic against concurrent PUTs (see Concurrency above).

No frontend change is needed. The account form (frontend/src/settings.ts) always sends enabled, and it sends aws_is_org_root for AWS accounts, so UI saves behave exactly as before. For Azure and GCP accounts the form omits aws_is_org_root, which now keeps the stored value (normally false) instead of forcing it to false.

Maintainer: please mirror the issue's triage labels on this PR: priority/p2, severity/high, urgency/this-quarter, impact/many, effort/l, type/bug, triaged.

🤖 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 59 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: main
• File: .coderabbit.yaml
• Commit: 7b86e38
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: b1e679b0-2ba9-4c83-949b-9a60c1fb852b

Commits

Reviewing files that changed from the base of the PR and between 7b86e38 and 74cc208.

Files ignored due to path filters: 1

• go.sum is excluded by !**/*.sum

Files selected for processing: 4

• go.mod
• internal/api/handler_accounts.go
• internal/api/handler_accounts_update_keep_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

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

[IAmJSD]

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE 74cc208a005f98484deff3b31a96230701827731

Blocking:

• none

Non-blocking:

• keepOmittedAccountFlags keeps the stored aws_is_org_root even when the same PUT points the record at a different AWS account (external_id / aws_role_arn change) and omits the flag. Org discovery (handler_accounts.go:1623, the only reader of the flag) would then be attempted against the new account and fail at AWS Organizations. The caller already holds update accounts scope and could set the flag explicitly, so this is not a privilege issue. Optionally, clear the flag when external_id changes and the request omits it, or note it in the OpenAPI description.
• Concurrent PUTs are still last-writer-wins on the whole row. The body discusses this, and it is strictly better than main's "omitted means false".
• The head moved to 74cc208 since the listed d20cf01. The added commit is the x/net v0.62.0 bump, byte-identical to fix(deps): bump golang.org/x/net to v0.62.0 (GO-2026-6860, GO-2026-6866) #1564 (go.mod/go.sum only). I reviewed and tested 74cc208.

Attacked:

• Scope: only updateAccount calls keepOmittedAccountFlags, and it runs after requirePermission("update","accounts") and requireAccountAccess. createAccount, createSelfAccount and registration approval (handler_registrations.go:297) still go through cloudAccountFromRequest alone, so an absent flag stays false there. JSON null decodes to a nil *bool and keeps the stored value; an explicit false still clears it. Both are covered. frontend/src/settings.ts:2385 always sends aws_is_org_root for AWS accounts and always sends enabled, so UI saves are unchanged. openapi.yaml is 3.0.3, so nullable: true is valid.
• Fail-before: with handler_accounts.go reverted to origin/main and the new test file kept, TestUpdateAccount_OmittedFlagsKeepStoredValues (4 of 7 subtests) and TestUpdateAccount_ExplicitFalseAndOmittedDiverge fail. After restoring, all 13 tests and subtests pass. The tests assert on the row passed to UpdateCloudAccount, not only on the response.
• Ran on 74cc208: go build ./.... go vet ./internal/api/, with and without -tags integration. go test -race -short -count=1 ./internal/api/: ok. go test -race -tags integration -count=1 ./internal/api/: ok (284s; no integration test exercises updateAccount, and the store SQL is unchanged). golangci-lint v2.10.1, with and without the integration tag: 0 issues. gocyclo -over 10 on handler_accounts.go: silent.

Body-stale:

• none

_{Reviewer: rG3. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths.}

[IAmJSD]

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE 74cc208a005f98484deff3b31a96230701827731

Blocking:

• none

Non-blocking:

• With this change, a PUT that changes an AWS account's role, external ID or bastion and omits aws_is_org_root keeps true on the stored row. The only thing that reads the flag is parseDiscoverOrgRoot. It still requires update/create permission on the row and unrestricted account scope before it uses the root's credentials, so this is no privilege change, just something worth knowing: re-pointing an org-root row now keeps it discoverable as a root unless the caller sends false.
• The concurrency note in the body is accurate. The omit-keeps fallback reads existing and then writes the whole row, so it is last-writer-wins, the same as main. A COALESCE/updated_at CAS is reasonable follow-up scope.

Attacked:

• Checked every cloudAccountFromRequest caller. Create (createAccount), registration approval and createSelfAccount still map a nil flag to false, approval still overrides enabled from its credential check, and only updateAccount calls keepOmittedAccountFlags, after requireAccountAccess. No other Go code builds CloudAccountRequest.AWSIsOrgRoot. The frontend always sends enabled and sends aws_is_org_root for AWS accounts, so UI saves are unchanged. validateCloudAccountRequest doesn't read either flag, so a nil value skips no validation.
• Ran go build ./..., go vet ./internal/api/ with and without -tags integration, go test -race -short -count=1 ./internal/api/ (ok) and go test -race -tags integration -run 'Account|Registration|OrgRoot|Discover' ./internal/api/ (ok). golangci-lint v2.10.1 found 0 issues with and without the integration tag, and gocyclo -over 10 is silent on handler_accounts.go.
• Fail-before: removing only the keepOmittedAccountFlags(...) call from updateAccount fails TestUpdateAccount_OmittedFlagsKeepStoredValues (4 of 7 subtests: omitted/null keeps true, and each flag omitted next to an explicit false) and TestUpdateAccount_ExplicitFalseAndOmittedDiverge, which is the issue's real-zero vs absence rule. Restored afterwards.

Body-stale:

• none

_{Reviewer: rH1. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths.}

[IAmJSD]

Local verification at 74cc208a005f98484deff3b31a96230701827731 merged onto main (bbf0a839), together with #1552, #1553, #1551, #1246, #1548, #1549, #1554, #1560 in that order (combined tree 7f9817d4, Linux, Go 1.26.10): go build ./..., go vet with and without -tags integration, the full go test -race -short -count=1 ./..., and go test -race -tags integration -count=1 on every changed package, against Docker Postgres. All pass. CI Success and pre-commit are green on this head, with 2 approving independent review(s) on it. Nothing was run against any cloud.

[IAmJSD]

Merged by IAmJSD's agent at IAmJSD's request (IAmJSD is acting owner while the maintainers are away), under the bar IAmJSD set: 2 approving independent review(s) on 74cc208a (two required: money, auth or security path), green CI, and the combined-tree verification above. Squash-merged at exactly 74cc208a005f98484deff3b31a96230701827731.