[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]

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

• Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

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

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