[IAmJSD]

Summary

Issue #243 tracks the platform half of the 2026-07-28 money-core review's medium/low findings plus five 2026-09-02 audit findings. I re-checked each one against current main (a14dc8f2). Three were already fixed by earlier PRs. This PR fixes the remaining platform-only ones. The ladder Existing display item is deferred until the go-side change lands.

Refs #243

Per-finding status

#	Finding	Status	Evidence / change
1	Reaper (10m) preempts the stranded-approval re-drive (15m)	Already fixed	d69c2024 (#1042): approved rows are reaped only after max(reapAfter, staleApprovedThreshold + sweep interval + slack), and reaper_ordering_test.go pins the ordering
2	Ladder run has no claim/CAS: overlapping ticks both append tranches	Already fixed	f606770e (#845) + 3c5a62b5 (#846): PlanAndSaveLadderRun plans and inserts under pg_advisory_xact_lock per config and nets in-flight tranches inside the lock. TestLadderLock_ConcurrentRunsPlanTheGapOnce (real Postgres) shows a second concurrent run plans a zero gap. Two ladder_runs rows can still exist for one tick, but only one carries tranches
3	recIsSafeToRedrive Azure SP exclusion covers 2 of 10 slugs, and its justification is stale	Slug part already resolved; stale comment fixed here	Slugs: LeanerCloud/cloud-commitments-cli#1713 (04e65fb7, after the review) added TestRedriveGuardReachMatchesDispatchReach. Azure only dispatches ServiceSavingsPlansAll to its SP client, and only savingsplans/savings-plans map to it. A rec with savingsplans-compute gets "unsupported service" and buys nothing, so the refusal set already equals the dispatch set. This PR rewrites the stale "timestamp alias, no idempotency key" comments in manager.go (3 places) and reaper.go. The pinned client derives the alias from the idempotency token, but nothing confirms that a repeat PUT is deduplicated, so the refusal stays as it is. Behavior is unchanged.
4	Canonical-slug (compute) EC2 recs never get offering_class	Already fixed	ba559138 (#509): savePurchaseHistory matches mapServiceType(rec.Service) against ServiceEC2 or ServiceCompute and stamps the resolved class
5	commitmentopts.Validate hard-rejects legit OpenSearch/Redshift combos cut off by the page cap	Fixed	walkPaginatedCapped reports whether maxPages stopped the walk while pages remained. The two client-side-filtered probers (OpenSearch, Redshift) record no combos for a capped walk, with a warning. Validate then takes its existing permissive "service absent" path, and the frontend keeps its hardcoded rules for that service. Probers with a server-side filter are unchanged
6	Pending direct-execute row (PlanID == "") records a failure every tick	Fixed	New autoPurchasePlan helper returns "not eligible" for PlanID == "" before calling the store, so the row is skipped without an error. The SQS path still dead-letters it as before
7	FinalizeInFlightRevocations sleeps without honoring ctx	Fixed	The backoff now uses a timer in a select on ctx.Done(). On cancel, the row being retried counts as Errored, the partial counts are logged, and (result, wrapped ctx.Err()) is returned
8	Ladder plan reports gross Existing instead of the net E used for the gap (cross-repo)	Deferred	The platform should consume the net figure Allocate computed rather than recompute it from layerStates. That needs NetExistingUSDPerHour on Allocate's result (LeanerCloud/cloud-commitments-go#83, not merged yet) and then a go pin bump here. A platform-only recompute of computeNetExisting would add the second copy the issue warns against, so it is left out
A16-005	finalize_revocations.go has no test	Fixed	New finalize_revocations_test.go: accounting table (first try / on retry / never, shared revokedAt), list error, cancel during backoff
A05-010	findAWSAccount probes a disabled account	Fixed	The filter now sets Enabled: true (as Scheduler.enabledAccounts does), and the loop re-checks Provider and Enabled
A02-022	checkCommitmentOptionCombo lets the save through on a Validate error	Fixed	ErrNoData stays permissive. Any other error is logged and returns a 503 without saving. Applies to both PUT /api/config/service/... and account service overrides
A05-012	Azure probeCombo records a combo as sold on an empty Benefits slice or a nil Valid flag	Fixed	A combo now needs at least one benefit with Valid=true and none with Valid=false. The two tests that pinned the old permissive behavior now assert the new rule. ProbeAzure still has no non-test caller
A05-013	executableByScheduler dereferences a nil plan	Fixed	A (nil, nil) plan returns a "plan not found" error (the caller fails closed and counts it), matching executePurchase

Root cause and fix (fixed items)

• Plan-less and missing-plan rows (6, A05-013): executableByScheduler passed exec.PlanID to GetPurchasePlan without checking it and dereferenced the result without a nil check. The new autoPurchasePlan helper (internal/purchase/manager.go) handles "" → not eligible, fetch error → wrapped error, nil → not-found error, and !AutoPurchase → not eligible. Splitting it out keeps executableByScheduler under the gocyclo budget.
• ctx-blind backoff (7): the retry loop moved into markRevokedWithRetry (internal/purchase/finalize_revocations.go), which returns (markErr, ctxErr).
• Page cap (5): internal/commitmentopts/probe.go. walkPaginated keeps its signature as a wrapper, so the other probers and the SP prober's call site are untouched.
• Disabled account (A05-010): internal/commitmentopts/service.go.
• Azure positive confirmation (A05-012): internal/commitmentopts/probe_azure.go.
• Fail closed on Validate error (A02-022): internal/api/handler_config.go.

internal/purchase/money_path_regression_test.go: TestSQSRedeliveryDoesNotDoubleExecute ran a notified, rec-bearing row with no PlanID through the SQS auto-execute gate. It relied on the mock answering GetPurchasePlan("") with an AutoPurchase plan, which real Postgres rejects with 22P02. The fixture now has PlanID: "plan-dup". What the test checks (a redelivery doesn't double-execute) is unchanged.

Verification

All evidence below is from unit tests with mocks and fakes (testify MockConfigStore, fake AWS SDK clients, stub Azure validate client). None of these changes is DB-level (no SQL or schema change), so I added no new integration test. Finding 2's race coverage is the existing real-Postgres TestLadderLock_ConcurrentRunsPlanTheGapOnce.

Fail-before: I checked out the origin/main versions of the changed non-test files, kept the new tests, ran them, then restored my versions:

• TestScheduler_DirectExecuteRowSkippedWithoutPlanLookup: FAIL. Failed=1, error failed to fetch plan for AutoPurchase gate: ... (SQLSTATE 22P02).
• TestScheduler_RefusesBeforeClaim/plan_missing and TestSQSExecutePath_RefusesBeforeClaim/plan_missing: FAIL with nil pointer dereference panic.
• TestFinalizeInFlightRevocations_CanceledDuringBackoffStopsSweep: FAIL. It took 3.00s, returned no error, and went on to the next row.
• TestFinalizeInFlightRevocations_Accounting / _ListErrorIsReturned: pass on both. These are the coverage tests A16-005 asked for, not regressions.
• TestService_FindAWSAccount_SkipsDisabled, TestAzureSPProber_EmptyBenefitsSlice, TestAzureSPProber_NilValidFlag: FAIL.
• TestHandler_updateServiceConfig_CommitmentOptsErrorRefusesSave: FAIL (save allowed).
• TestWalkPaginatedCapped_ReportsWhyTheWalkStopped, TestFilteredProbers_PageCapYieldsNoCombos/{opensearch,redshift}, TestService_Validate_CappedProbeStaysPermissive: FAIL. For this run, old probe.go got a compile shim, walkPaginatedCapped that never reports a cap, which is the old behavior.

Pass-after, on this branch:

export PATH=$HOME/.local/go/bin:$HOME/go/bin:$HOME/.local/bin:$PATH
export AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true
go build ./... && go vet ./... && go vet -tags integration ./...        # ok
go test ./internal/purchase/ -count=1                                   # ok
go test ./internal/commitmentopts/ -count=1                             # ok
go test ./internal/api/ -count=1                                        # ok
go test ./internal/mocks/ -count=1                                      # ok
golangci-lint (v2.10.1) run --allow-parallel-runners ./internal/purchase/... ./internal/commitmentopts/... ./internal/api/...   # 0 issues
gocyclo -over 10 -ignore "_test\.go" <changed non-test files>          # no output

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.

Out of scope / notes

• Finding 8 is deferred (see the table). Follow-up: once go#83 is released, bump the cloud-commitments-go/pkg pin and have assembleLadderPlan read NetExistingUSDPerHour from the Allocate result.
• Finding 3: whether an Azure SP re-drive can be marked safe when the idempotency token is non-empty is an owner decision. It needs confirmation that a repeat SavingsPlanOrderAlias PUT with the same alias name returns the existing order. Until then the refusal stays conservative.
• Finding 5, region scope: the probe snapshot is still taken from one account in one region and applied to every region. I didn't change that.
• Finding 5, other probers: probers that filter server-side (RDS, ElastiCache, MemoryDB, EC2, SP) still keep a capped walk's partial result. With a server-side instance filter, hitting 5×100 pages means pagination is broken, not that the matrix was truncated.
• A02-022 changes behavior: a commitment-options store outage now blocks AWS service-config and account-override saves that carry a term and payment, with a 503, until the store recovers.

Frontend follow-up for the UI owner

No change is required. With the 503 from A02-022, the Settings save can now fail with "could not validate the commitment term and payment option; try again". The UI should already show it like any other non-2xx save error. Worth a quick look that the message reads well there.

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

🤖 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.}