[IAmJSD]

First slice of #339 (the 2026-09-02 audit's low-severity correctness findings). It fixes A05-015 and A06-009. A06-019 is deferred to a follow-up PR; the reasons are below.

Refs #339

Per-finding status (re-verified against main at a14dc8f)

Finding	Status	Notes
A05-015: an execution with nothing selected still sends a purchase confirmation	Fixed here	Partly addressed already. d000285 (#618) refuses a plan step with no recommendations (ErrPlanStepNoRecommendations), which covers the cron/notification row with an empty list that the audit described. Still reproducible on main: a run whose recommendations are all unselected (a plan step or a direct execute). It goes through executeSingleAccount and sends a confirmation for zero commitments and $0. The multi-account path (executeForAccount) did the same, because it gated on len(purchaseErrors) == 0 || partial. Both paths now gate on whether any rec was purchased.
A06-009: /health config_store reports healthy after a failed post-connect reinit	Fixed here	Still reproducible on main (integration test below).
A06-019: every non-SavingsPlans commitment is labelled "RI"	Deferred to a follow-up PR (Refs #339)	Still reproducible on main, and worse than the audit says. commitmentTypeFor matches only "SavingsPlans", which purchase_history.service never stores (Savings Plans are stored as savingsplans or savings-plans-<type>), so AWS Savings Plans are labelled "RI" as well as GCP CUDs. The fix needs a migration that widens the savings_snapshots CHECK and relabels existing rows. I left it out of this slice for two reasons. First, the full set came to about 665 changed lines, well over our 400-line slice budget. Second, a migration PR has to wait until open migrations 000139 to 000144 (#1201, #1239, #1240, #1241, fix/issue-47) merge, and these two fixes don't need to wait. The follow-up is written and tested: commit 73bdb19 on local branch fix/issue-339-commitment-type (not pushed yet). It maps (provider, service) to the pkg/common CommitmentType values, adds migration 000145, and includes fail-before/pass-after unit, migration and Postgres tests.

In-flight overlap check: I diffed open PRs #1099, #1111, #1237, #1245 and #1246, the pushed branches fix/issue-47, -54, -532 and -337, and the working trees of the unpushed fix/issue-243 and fix/issue-326 against main. None of them touch executeSingleAccount, executeForAccount, internal/server/health.go, readiness_test.go, health_test.go, or the edited lines in approvals_test.go / coverage_extra_test.go. #1111 (fix/issue-835) edits approvals_test.go at about line 1333, far from the change here. Nothing had to be deferred because of overlap.

Root cause and fix

A05-015: confirmation email for a run that bought nothing (internal/purchase/execution.go)

processPurchaseRecommendations returns (0, 0, nil, nil) when no recommendation is selected. In executeSingleAccount the failure guard is len(purchaseErrors) > 0 && !anyRecPurchased(...), which is false here, so sendPurchaseNotification fired unconditionally. executeForAccount gated on "no per-rec errors or partial", which is also true when nothing ran.

Fix: send the confirmation only when anyRecPurchased(exec.Recommendations) is true (single-account) or committed is true (per-account; it is the same anyRecPurchased result that applyAccountOutcome already computes). Full and partial successes are unchanged: they always have at least one Purchased rec, and buildPurchaseConfirmationData only ever listed Purchased recs.

A06-009: config_store healthy over a closed pool (internal/server/health.go)

reinitializeAfterConnect sets app.Config and can still fail afterwards (encryption key, analytics collector, LoadDefaultConfig). ensureDB then closes the pool and sets app.DB = nil, but leaves app.Config in place and dbConnected false. checkConfigStore saw a non-nil store with a nil DB and returned healthy.

Fix: collectHealth already snapshots the store pointers under dbMu. It now also snapshots app.dbConnected and passes it to checkConfigStore, which reports unhealthy ("Database initialization did not complete; config store is not connected") when PostgreSQL is configured but lazy init has not completed. In production this state (Postgres configured, Config set, not connected, dbMu free) only happens after a failed reinit. app.Config is nil until reinitializeAfterConnect sets it, and while ensureDB holds dbMu the existing TryLock path already reports pending. /health stays HTTP 200 (liveness contract), and /ready was already 503 in this state because auth_store fails, so no probe verdict changes. Only the sub-check stops lying. The flag is read inside the existing TryLock critical section, so the snapshot has no new race.

Three readiness tests built a "fully wired" app with dbConfig set but never set dbConnected. They now set dbConnected: true, which is the real post-ensureDB state. Without it, two of them would be not-ready for the wrong reason.

Tests that pinned the A05-015 bug

TestManager_ApproveAndExecute_EmptyPlanID, TestProcessMessage_ApproveHappyPath and TestProcessMessage_ApproveFourEyesOn_DifferentApproverSucceeds run executions with no selected recs and registered a SendPurchaseConfirmation expectation that required the email. They now assert that no confirmation is sent. In the approve test the assertion is the missing mock expectation: an unexpected call panics the mock. The internal/mocks vacuous-assertion guard can't resolve a mock returned by newApproveManager, so an explicit AssertNotCalled there would be unverifiable.

Verification

Commands ran on this branch with Go 1.26.9 and AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true. The A05-015 evidence is unit/mock evidence. The A06-009 evidence includes a real-Postgres integration test (testcontainers) that drives the real ensureDB → reinitializeAfterConnect failure path. No cloud APIs were called. The reinit fails on a deliberately missing AWS_PROFILE, before any AWS client is used.

New tests:

• internal/purchase/empty_selection_notify_test.go
  • TestExecutePurchase_SingleAccount_NothingSelected_SendsNoConfirmation: plan step, all recs unselected, through executePurchase.
  • TestExecuteForAccount_NothingSelected_SendsNoConfirmation: multi-account per-account path.
• internal/server/health_reinit_failure_integration_test.go, TestHealth_ConfigStoreAfterFailedReinitialization (integration tag): real Postgres. ensureDB fails in reinitializeAfterConnect after app.Config is set. The test asserts that app.Config is non-nil, app.DB is nil, config_store is unhealthy, the overall status is degraded, and the app is not ready.
• TestCheckConfigStore gains "config store left over a failed lazy init" (unhealthy) and "config store after a completed lazy init" (healthy).

Fail before (internal/purchase/execution.go restored from origin/main, each test run on its own):

TestExecutePurchase_SingleAccount_NothingSelected_SendsNoConfirmation: FAIL
    empty_selection_notify_test.go:52: (AssertNotCalled SendPurchaseConfirmation)
TestExecuteForAccount_NothingSelected_SendsNoConfirmation: FAIL
    empty_selection_notify_test.go:105
TestManager_ApproveAndExecute_EmptyPlanID: FAIL (unexpected SendPurchaseConfirmation call)
TestProcessMessage_ApproveHappyPath: FAIL
TestProcessMessage_ApproveFourEyesOn_DifferentApproverSucceeds: FAIL

Fail before (the new if app.dbConfig != nil && !connected guard disabled):

$ go test -tags integration -count=1 -timeout 20m -run 'TestHealth_ConfigStoreAfterFailedReinitialization|TestCheckConfigStore' ./internal/server/
--- FAIL: TestHealth_ConfigStoreAfterFailedReinitialization (8.39s)
        expected: "unhealthy"
        actual  : "healthy"
--- FAIL: TestCheckConfigStore
    --- FAIL: TestCheckConfigStore/config_store_left_over_a_failed_lazy_init
FAIL

Pass after:

$ go build ./... && go vet ./... && go vet -tags integration ./...          # clean
$ go test -count=1 ./internal/purchase/                                      # ok (32.8s)
$ go test -count=1 ./internal/server/                                        # ok (9.4s)
$ go test -count=1 ./internal/mocks/                                         # ok (vacuous-assertion guard included)
$ go test -count=1 ./internal/scheduler/ ./internal/api/                     # ok (callers of the purchase manager)
$ go test -tags integration -count=1 -timeout 20m -run 'TestHealth_ConfigStoreAfterFailedReinitialization' ./internal/server/   # ok (10.1s)
$ golangci-lint v2.10.1 run --allow-parallel-runners --build-tags integration ./internal/purchase/... ./internal/server/...    # 0 issues
$ gocyclo -over 10 -ignore "_test\.go" internal/purchase/execution.go internal/server/health.go                                 # nothing over 10

Not run here: whole integration packages, -race ./... and git-secrets. They run on the combined tree below.

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

Branch	Tested head
fix/issue-337	ff3420c
fix/issue-169	7d75eb5
fix/issue-342	66b221f
fix/issue-310	5e11c21
fix/issue-326	5f0ea59
fix/issue-243	365e959
fix/issue-343	6c94036
fix/issue-312	d2848a8
fix/issue-316	acd60dd
fix/issue-339	75ea9b9
fix/issue-339-commitment-type	73bdb19
fix/issue-353	5d25fcd
fix/issue-313	3156bc4
fix/issue-357	53d9e58
fix/issue-368	0ba727d
fix/issue-419	61648c9
fix/issue-111	b04c372
fix/issue-686	0ad90b7
fix/issue-674	793095d
fix/issue-1055	4ec96f1
fix/issue-559	dfa9525
fix/issue-558	034eed4
fix/issue-874	1720539
fix/issue-666	1e120da
fix/issue-668	fb26345
fix/issue-568	ce04173
fix/issue-311	e54f250
fix/issue-177	0cbb88a
fix/issue-151	c21377c
fix/issue-166	41fb77b
fix/issue-27	2d67622
fix/issue-323	69288ae
fix/issue-59	36463e8
fix/issue-43	35e4069
fix/issue-158	6f76f8a
fix/issue-104	bb00491
fix/issue-55	971ef1b
fix/issue-361	89ac220
fix/issue-120	b458719
fix/issue-36	42bac6e
fix/issue-525	57c97dd
fix/issue-712	e5bcda0
fix/issue-72	6cc63ed

fix/issue-72 and fix/issue-312 conflict on one line in internal/api/handler_dashboard.go (both change the calculateCommitmentMetrics call). The tree used #72's side with #312's validated provider variable; whichever PR merges second takes that same resolution.

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:

• Passed at 936996ad: go build ./..., go vet ./..., go vet -tags integration ./... and the full go test -race -short -count=1 ./....
• Static checks at 936996ad: golangci-lint v2.10.1 found 0 issues on every changed package, with and without --build-tags integration. tests/e2e is a separate module, so it was vetted under its own e2e tag. gocyclo -over 10 is silent on the changed non-test files.
• Integration tests, every changed package, one package at a time (-tags integration -count=1, 60-minute local timeout because the VM was shared):
  • At 936996ad, these pass: internal/server, internal/credentials, internal/scheduler, internal/database/postgres/migrations, internal/purchase and internal/api.
  • internal/config passes except TestPostgresStoreDB_ResolveStaleRIExchange_ValidatesCloudCheck, which fails the same way on unmodified main (fixed by fix(config): compare resolve checked_at at the database's microsecond precision #1252).
  • These were run on the first build of this tree (26795376, same branches at earlier heads): internal/analytics, internal/auth, internal/commitmentopts, internal/database and cmd/lambda/clear-rate-limit all pass. The later fixes don't touch those packages.
• What the combined run caught, each fixed on its own branch before this PR opened:
  • a compile break between fix/issue-104's test and fix/issue-326's httpToLambdaRequest signature change;
  • fix/issue-316 rejecting legacy-keyed plans;
  • fix/issue-120 versus main's global-filter test.
• Secrets: 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: 137 PASS, 0 FAIL.
• Nothing was applied to any cloud.

Left out of scope / for a maintainer

• A06-019 is deferred to the follow-up described in the table.
• "Lands completed" (owner decision). The audit title also says such a run "lands completed". With this PR the no-selection run still finalizes as completed, and on a plan step it still calls CompletePlanStep and advances the ramp. TestProcessMessage_ApproveHappyPath shows that path. fix(plans): fail loudly on plan steps that carry no recommendations #618 made a plan step with no recommendations fail for exactly this reason. Whether a step whose recs were all deselected should also fail, or should count as a deliberate skip, is a product decision, so I didn't change it. The issue's acceptance criteria only ask for the email gate.
• applyAccountOutcome still returns partial, which its only caller now ignores (_, committed = ...). I left the signature alone to keep the diff small.

No frontend change is needed.

Maintainer: please mirror the issue's triage labels onto this PR: triaged, priority/p3, severity/low, urgency/eventually, impact/few, effort/m, type/bug.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 8 billable files and costs up to $2.00.}

Or wait 59 minutes for your next included review.

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: 503eb2e
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: eee59482-8c1f-4774-98d9-8d3ff34d65a1

Commits

Reviewing files that changed from the base of the PR and between 503eb2e and 63adf28.

Files selected for processing: 8

• internal/purchase/approvals_test.go
• internal/purchase/coverage_extra_test.go
• internal/purchase/empty_selection_notify_test.go
• internal/purchase/execution.go
• internal/server/health.go
• internal/server/health_reinit_failure_integration_test.go
• internal/server/health_test.go
• internal/server/readiness_test.go

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

_{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 63adf280c907a0249e48b04e2093ccabf4dbe097

Blocking:

• none

Non-blocking:

• internal/server/health.go:196: main has since added stable Reason codes for failed checks (chore(server/scheduler): medium/low findings from the 2026-07-28 full review #111: healthReasonDBUnreachable, etc.). The merge is clean, but the new unhealthy result has only a Message and no Reason. Adding a code such as db_init_incomplete would match that convention. The message is a fixed string, so nothing leaks.
• internal/purchase/execution.go:335: applyAccountOutcome's partial return now has no reader. Dropping it, or leaving it with a comment, can wait for a follow-up.
• Product note, already stated in the body: a step whose recs were all deselected still finalizes as completed and advances the ramp. With this PR it also sends no email, so a skipped step now goes completely silent. That is consistent with the issue's acceptance criteria. Whether such a step should fail or send a "skipped" notice is an owner call.

Attacked:

• Money path. Both sendPurchaseNotification call sites are gated (the grep found no other sites). Every successful per-rec outcome sets Purchased=true in aggregatePurchaseOutcomes, so a real purchase can't lose its confirmation. Full and partial runs behave the same as before, and the zero-committed, zero-error case is the only one that changes.
• Health. dbConnected is written only under dbMu in ensureDB, which is the only production path that sets dbConfig (NewApplication), so a healthy replica can't get stuck reporting unhealthy. app.Auth is set before the later failure points (analytics collector, LoadDefaultConfig), but its ping goes over the closed pool, so /ready was already 503 there. The PR body's "no probe verdict changes" claim holds.
• Fail-before. With execution.go restored from main, TestExecutePurchase_SingleAccount_NothingSelected_SendsNoConfirmation (:52), TestExecuteForAccount_NothingSelected_SendsNoConfirmation (:106) and TestProcessMessage_ApproveHappyPath (mock panic) fail. With the health.go hunk reverted, TestCheckConfigStore/config_store_left_over_a_failed_lazy_init fails, and so does TestHealth_ConfigStoreAfterFailedReinitialization (real Postgres; expected unhealthy, got healthy). All pass with the fix restored.
• Commands: go build ./...; go vet with and without -tags integration; go test -race -short and go test -race -tags integration on ./internal/server/ and ./internal/purchase/ (ok, 50s and 113s); golangci-lint 2.10.1 with and without the integration tag (0 issues); gocyclo -over 10 on the two changed files (silent). I also merged current main (ff7d191) into a local scratch tree, never pushed, and re-ran build, vet, race tests and the health/readiness integration tests there. All ok.

Body-stale:

• "re-verified against main at a14dc8f": main is now ff7d191, and the branch base is 503eb2e.
• The deferred A06-019 follow-up "adds migration 000145", and the body names "open migrations 000139 to 000144". Main is now at 000139, and 000140-000148 are held by open PRs (000145 by fix(analytics): label snapshot commitment types from provider and service #1302). The follow-up needs 000149 or higher.
• The combined-tree table lists fix/issue-339 at 75ea9b9. The reviewed head is 63adf28.

_{Reviewer: rB14. 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 63adf280c907a0249e48b04e2093ccabf4dbe097

Blocking:

• none

Non-blocking:

• internal/purchase/execution.go:141: the gate is anyRecPurchased(exec.Recommendations). It is true for any rec already flagged Purchased, not only recs this run bought. A re-driven row that carries a Purchased flag from an earlier attempt, but has nothing newly selected, would still send a confirmation. That is an edge case and narrower than before this PR, so it isn't blocking. Gating on "this run committed at least one rec" (for example, a count returned by aggregatePurchaseOutcomes) would be exact.
• Owner decision, already called out in the body: a plan step whose recs were all deselected still finalizes completed and advances the ramp. Before this PR the operator at least got a $0 email. Now nothing is sent at all, so the only trace is the History row. That makes the "deselected step = skip or fail" decision a little more pressing.
• applyAccountOutcome now returns a partial value that no caller uses. Dropping it can come later.

Attacked:

• A05-015: I traced processPurchaseRecommendations → aggregatePurchaseOutcomes. Every rec that doesn't error gets Purchased = true (execution.go:739). So a full or partial success always passes the new gate, and only "nothing selected" or "everything failed" skips the confirmation. In the per-account path, committed is anyRecPurchased(acctExec.Recommendations) from applyAccountOutcome, so the two paths agree.
• A06-009: dbConnected is set only at the end of a successful ensureDB (app.go:707). Production wires ConfigStore: nil with a dbConfig, so Config != nil && !dbConnected happens only after a failed reinitializeAfterConnect. The new check can't wrongly mark a healthy app unhealthy. The flag is read inside the existing TryLock section, so there's no new race.
• Fail-before: I reverted only execution.go to origin/main and ran each test on its own. TestExecutePurchase_SingleAccount_NothingSelected_SendsNoConfirmation, TestExecuteForAccount_NothingSelected_SendsNoConfirmation, TestProcessMessage_ApproveHappyPath, TestProcessMessage_ApproveFourEyesOn_DifferentApproverSucceeds and TestManager_ApproveAndExecute_EmptyPlanID all fail. I then disabled only the new !connected guard in health.go, and TestCheckConfigStore/config_store_left_over_a_failed_lazy_init fails. Files restored afterwards.
• Ran go build ./..., go vet (plain and -tags integration) on purchase and server, go test -race -short -count=1 on purchase, server and scheduler, go test -race -tags integration -count=1 ./internal/server/ ./internal/purchase/ (testcontainers Postgres, including TestHealth_ConfigStoreAfterFailedReinitialization), and golangci-lint --build-tags integration (0 issues). All green.

Body-stale:

• The A06-019 row says the follow-up adds "migration 000145" and waits on "open migrations 000139 to 000144". Main is now at 000139, and 000140-000148 are taken by open PRs (000145 by fix(analytics): label snapshot commitment types from provider and service #1302), so the follow-up needs 000149 or above.

_{Reviewer: rB15. 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 63adf280c907a0249e48b04e2093ccabf4dbe097 merged onto main (5f717aa4), together with #1260, #1261, #1263, #1264, #1269, #1271, #1273, #1308, #1416 in that order (combined tree 62127c84, 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 63adf280 (two required: money, auth or security path), green CI, and the combined-tree verification above. Squash-merged at exactly 63adf280c907a0249e48b04e2093ccabf4dbe097.