[IAmJSD]

Summary

Test-only. ladderTestStore.inFlightErr (in internal/server/handler_ladder_test.go) was declared and honored by the fake PlanAndSaveLadderRun, but no test set it. So the executeLadderRun branch that handles a failed in-flight commitment read had no coverage. In production that read is PostgresStore.PlanAndSaveLadderRun -> lockedLadderCommit, inside the per-config lock.

Root cause: a coverage gap, not a behavior bug. On current main the handler already fails loudly on this error: it returns persist ladder run: %w and processOneLadderConfig counts the config Errored. No production code changes.

New file internal/server/handler_ladder_inflight_test.go. It is a sibling file, so it does not touch the hunks in handler_ladder.go / handler_ladder_test.go that open PR #1199 edits. It adds:

• TestExecuteLadderRun_InFlightFetchError_FailsLoudAndPersistsNothing: executeLadderRun returns an error, errors.Is finds the store's error in the chain, and the message is exactly persist ladder run: <store error>.

• TestRunLadderConfigs_InFlightFetchError_CountsErrored: the same error sent through the per-config loop gives exactly LadderRunResult{Errored: 1}, never Planned.

• Both tests check that nothing was saved or submitted:

  • no ladder_runs row (savedRun / savedRuns) and no ladder_tranches rows, so there is nothing for the fire task to submit later;
  • no SavePurchaseExecution / SavePurchaseHistory;
  • zero PurchaseLayer / ReshapeBuffer calls.

  Small counting wrappers record these, so the checks are counted assertions rather than the absence of a t.Fatal.

• The fixture would otherwise plan a positive purchase (low-water 10, target 8, nothing existing). That means a swallowed error would visibly save a run with an 8 $/hr tranche.

Closes #1180

Verification

All evidence is unit/mock-store level. There is no real Postgres, cloud or purchase path. Tests ran with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null because this machine's default AWS profile is invalid.

go build ./...
go vet ./internal/server/
go test ./internal/server/ -count=1          # ok
go test ./internal/mocks/ -count=1           # ok (TestNoUnfailableMockAssertions etc.)
golangci-lint v2.10.1 run --allow-parallel-runners ./internal/server/   # 0 issues
gocyclo -over 10 -ignore "_test\.go" internal/server/                   # silent
bash scripts/gosec-hook.sh internal/server/handler_ladder_inflight_test.go  # clean
git-secrets --scan (fresh git init repo, isolated HOME, scripts/setup-git-secrets.sh + .gitallowed)  # clean
bash scripts/test-git-secrets-allowlist.sh   # 109 PASS, 0 FAIL

Mutation proof. Each mutation replaced only the if err != nil { return fmt.Errorf("persist ladder run: %w", err) } branch after PlanAndSaveLadderRun in executeLadderRun. Each was run with go test ./internal/server/ -run InFlightFetchError -count=1, then the original was restored (git diff on handler_ladder.go is empty):

Mutation	Result
none (main)	both PASS
M1: swallow the error and treat in-flight as empty: in.plan(nil), then SaveLadderRun + SaveLadderTranches, return nil	both FAIL ("An error is expected but got nil"; Planned:1, Errored:0; run and tranches saved)
M2: log and return nil	both FAIL (no error; Planned:1)
M3: return the error but still persist an un-netted plan	both FAIL (savedRun / savedRuns / savedTranches non-empty)
M4: replace the error with an unwrapped "persist ladder run failed"	executeLadderRun test FAILs (ErrorIs)

The tests passed against the unchanged handler as soon as they were written. That is expected, because this PR adds coverage and does not fix a bug. The fail-before evidence is the mutation table above.

Out of scope / notes

• handleLadderRun returns (result, nil) even when every config is Errored. Per-config isolation is the documented design (runLadderConfigs), and the Errored counter is the failure signal. Unlike handleProcessScheduledPurchases (chore(iac): no alert reaches anyone; instantiate monitoring and wire failure notifications #714), the scheduler therefore sees a successful task. I left this alone: changing it would affect every per-config error kind, not only this branch. A maintainer may want a separate issue for it.
• The real PostgresStore.PlanAndSaveLadderRun in-flight read error (rollback under the lock) is not exercised here. This PR covers the handler's branch with the mock store only.

Maintainer: please mirror the issue's triage labels onto this PR: impact/internal, type/chore, triaged, priority/p3, severity/low, urgency/this-quarter, effort/xs.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 1 billable file and costs up to $0.25.}

Or wait 8 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: 5d4ab6b
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: a73dc08e-6c58-41d7-953e-15bbc4cbef01

Commits

Reviewing files that changed from the base of the PR and between 5d4ab6b and 1591a6d.

Files selected for processing: 1

• internal/server/handler_ladder_inflight_test.go

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

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

[cristim]

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

Findings (none blocking):

• [non-blocking] internal/server/handler_ladder_inflight_test.go:30-62,72-75 - Part of the fixture can never fail.
  • executeLadderRun only plans. handler_ladder.go:82 says "no PurchaseLayer, ReshapeBuffer, email, or approval tokens".
  • Nothing on that path calls PurchaseLayer, ReshapeBuffer, SavePurchaseExecution or SavePurchaseHistory, so the four counters and their wrapper types never catch anything.
  • The savedRun, savedRuns and savedTranches assertions are the ones that matter, and probe M4 shows they work.
• [non-blocking] What is pinned: the test covers the generic PlanAndSaveLadderRun error branch (handler_ladder.go:345-348).
  • The fake store returns inFlightErr before calling build (handler_ladder_test.go:207-208).
  • So the real store's in-flight read and how it passes errors up are not exercised (internal/config/store_postgres_ladder_lock.go:125-127, :192).
  • That fits a handler unit test, and the PR does not claim store coverage.
  • The "in-flight" name is a little narrow: lock, read and insert errors all go through this branch.
• Fail-before is not possible for a test-only PR. Both tests pass on base 5d4ab6b, because the handler already fails loudly. The probes below stand in for it.

Attacked: mutation probes on the production branch (handler_ladder.go:345-348 and processOneLadderConfig:270-273), in a scratch copy, each restored and confirmed identical with cmp. All 7 were caught:

• M1, the error swallowed (return nil): both tests FAIL.
• M2, %v instead of %w: test 1 FAILS on ErrorIs.
• M3, the error replaced with a new error: test 1 FAILS on ErrorIs.
• M4, in-flight treated as empty (plan(nil), then save the run and tranches directly): both FAIL, including the "persists nothing" assertions, so those checks really catch a regression.
• M5, the fetch skipped entirely: both FAIL.
• M6, outcomePlanned returned instead of outcomeErrored: test 2 FAILS.
• M7, the wrap prefix changed: test 1 FAILS on the exact-string assertion.

Also checked:

• The fixture plans a real purchase when the error is swallowed. Under M4, the run has GapUSDHr 8 and one scheduled tranche.
• The account gate passes (ExternalID equals ownAccountID), so the run reaches the persist step.
• Flakiness and races: none found.

Verified locally, with GOTOOLCHAIN=go1.26.10 and no cloud credentials, under the heavy lock:

• Head:
  • go build ./... and go vet ./internal/server: OK. gofmt -l: clean.
  • Both tests PASS, also with -count=20 -race.
  • Full go test -count=1 ./internal/server: ok.
• Merged tree: git merge-tree --write-tree origin/main 1591a6d7 against origin/main 00a3f40 is clean. vet and both tests pass there.
• No migration, frontend or terraform changes.

Reviewed by an independent agent with no author context (second reviewer). Review only: nothing was pushed, and the GitHub review state is unchanged.

[cristim]

VERDICT: APPROVE 1591a6d

Independent adversarial review of PR #1218 (closes #1180) at 1591a6d, base main. Test-only: it adds internal/server/handler_ladder_inflight_test.go and changes no production code.

The PR meets #1180's acceptance criteria. A test sets ladderTestStore.inFlightErr. The fake PlanAndSaveLadderRun returns that error before it builds a plan (internal/server/handler_ladder_test.go:207). The test asserts that executeLadderRun fails loudly (ErrorIs plus the exact persist ladder run: <err> message, matching internal/server/handler_ladder.go:345-347). It also asserts that nothing is saved: no run, no tranches, no execution or history, no provider writes. A second test drives the same error through runLadderConfigs and gets exactly LadderRunResult{Errored: 1}. Mutating the branch makes the tests fail, as shown below.

Findings: none blocking.

• nit internal/server/handler_ladder_inflight_test.go:32-41 - executeLadderRun never calls the PurchaseLayer / ReshapeBuffer counters at all, so on this path those zero-count asserts cannot fail. They are harmless defensive checks.

Attacked:

• Fixture reachability: the in-flight error is the only error in the chain (the ErrorIs passes), so the run really reaches the persist step and does not exit early on some other gate.
• go vet ./internal/server/ is clean. go test ./internal/server/ ./internal/mocks/ passes, including the repo-wide mock assertion guards.
• Mutation probes on the PlanAndSaveLadderRun error branch (each reverted, file restored byte-identical):
  • return nil: both tests FAIL (killed).
  • Replace with an unwrapped fmt.Errorf("persist ladder run failed"): the executeLadderRun test FAILS (killed).
  • Wrap with %v instead of %w: the executeLadderRun test FAILS on ErrorIs (killed).

[IAmJSD]

Local verification at 1591a6d71ef4c854d733f04285813d693e87205c merged onto main (26004a6d), together with #974, #1106, #1151, #1193, #1199, #1212, #1217, #1218, #1219, #1221, #1223, #1224, #1225, #1226, #1227, #1256, #1262, #1266, #1272, #1275, #1283, #1286, #1293, #1299, #1300, #1313 in that order (combined tree d3d95c83, 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 1 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: 1 approving independent review(s) on 1591a6d7, green CI, and the combined-tree verification above. Squash-merged at exactly 1591a6d71ef4c854d733f04285813d693e87205c.