chore(test): medium/low findings from the 2026-07-28 full review #1601

Reviewed commit: be11bdcb5. Note: origin/main moved to 3e9660d06 during the review; re-verify against current main before changing code, since a finding may have been fixed or moved.

Grouped MEDIUM and LOW findings from the test-integrity and docs-accuracy slice of the 2026-07-28 full review. The CRITICAL and HIGH findings from the same slice are filed separately; one LOW (internal/api/ri_utilization_cache_test.go wall-clock sleeps) is omitted here because LeanerCloud/cloud-commitments-platform#36 already names the exact lines.

Method, for context on what the absence of a finding does and does not mean: the review swept all of internal/mocks/, internal/testutil/, internal/api/mocks_test.go, tests/e2e/, and ci_cd_sanity_tests/**; then cross-referenced, repo-wide across 322 *_test.go files, every AssertNotCalled call site (263) against the mock short-circuit list, every .On( versus AssertExpectations pairing per file, every time.Sleep in a test (24 sites), every t.Skip / t.Skipf (60+ sites), every panic( in test-support code, every require.* / assert.* reachable from a go func, and constant-versus-itself assertions. Frontend: all 60+ frontend/src/__tests__/*.test.ts for try/catch-swallowed assertions and assertion-free bodies. Docs: every --flag token in README.md, SECURITY.md, CONTRIBUTING.md, known-issues.md, docs/**, specs/**, runbooks/** and .github/runbooks/** was extracted and diffed against the flags actually registered in cmd/*.go, then each behavioural claim hand-verified against the implementation. No tests were executed, so every item below is a static proof about what the code can assert, not an observed run.

MEDIUM

• README.md documents three flags that do not exist

  • Where: README.md:236-238 (--rds-coverage 50, --elasticache-coverage 80, --ec2-coverage 100, under "Example 2: Multi-Service with Different Coverage") versus cmd/main.go:95-158, which registers no per-service coverage flag - only --coverage (cmd/main.go:101) and --target-coverage (cmd/main.go:102). git grep finds these three strings nowhere outside README.md.
  • What: the README presents a copy-pasteable example for a documented feature ("Apply different coverage percentages per service") that the binary does not implement.
  • Failure scenario: a user copies the example and cobra exits with unknown flag: --rds-coverage. The worse money-path version: an operator who believes per-service coverage exists assumes a lower per-service cap is in force, then drops the flags to make the command parse and runs a broader --purchase than intended, against the single global --coverage value.
  • Fix direction: delete Example 2, or replace it with three separate --coverage invocations, and open a feature issue if per-service coverage is still wanted.
  • Related: HYG-08: README/CONTRIBUTING describe a CLI-only project on Go 1.23; web platform and Go 1.25 absent or contradicted #1177 (HYG-08: README/CONTRIBUTING describe a CLI-only project on Go 1.23) covers other README inaccuracies but not these flags.

• docs/cli/purchase-safety.md says --idempotency-window is not validated; it is

  • Where: docs/cli/purchase-safety.md:82 versus pkg/config/load.go:356-363.
  • What: the doc states: "This flag is accepted as a Go duration string ... The CLI does not validate the string at startup; it stores the raw value ...". load.go:356-363 calls time.ParseDuration(v) and returns fmt.Errorf("--idempotency-window: %w", err) on failure, so an invalid duration aborts the run at startup. This is a factually wrong statement about a startup-failure mode, in the safety document. (The paragraph's other claim - that the parsed value never reaches the CLI purchase loop - is correct: cfg.IdempotencyWindow is set at load.go:362 and read by nothing in cmd/; internal/api/handler_purchases.go:2380 uses an unrelated purchaseIdempotencyWindow constant.)
  • Failure scenario: an operator scripting an unattended run reads "not validated", assumes a typo'd --idempotency-window 24hr degrades silently, and does not guard the exit code. The run in fact aborts at startup and the scheduled purchase silently never happens - the operator learns about it from a coverage report weeks later.
  • Fix direction: change the sentence to "the value is parsed at startup and an invalid duration aborts the run, but the parsed value is not consumed by the CLI purchase loop".
  • Related: fix(cli): --idempotency-window silently ignored in CLI direct-purchase path #1262 tracks the second half (the value is never consumed).

• frontend/src/__tests__/api.test.ts puts an assertion inside a catch with no expect.assertions()

  • Where: frontend/src/__tests__/api.test.ts:370-382, test('includes status code in error').
  • What:

    try { await apiRequest('/test'); } catch (error) { expect((error as {status?: number}).status).toBe(500); }

    There is no expect.assertions(1) and no unreachable-fail() after the await.
  • Failure scenario: if apiRequest stopped throwing on a 500 whose body fails to parse - resolving null instead, which is exactly what the neighbouring test at line 384 makes it do for 2xx - the catch block never executes, zero assertions run, and Jest reports a pass. The test that exists to prove "5xx surfaces as a rejection carrying .status" cannot detect the regression where 5xx stops rejecting at all.
  • Fix direction: await expect(apiRequest('/test')).rejects.toMatchObject({ status: 500 }), or add expect.assertions(1). api-mfa.test.ts:44-52 is already the correct pattern (it throws a sentinel after the await) and needs no change.

• MockConfigStore mixes two incompatible unregistered-method policies

  • Where: internal/mocks/stores.go - 28 methods short-circuit via isExpected (e.g. WithTx, CancelExecutionAtomic, GetGlobalConfig at :54-67) while the rest call m.Called unconditionally (e.g. GetAllPurchaseHistory at :375-385, SaveGlobalConfig at :70-73).
  • What: for the second group, an unregistered call panics inside testify, which makes AssertNotCalled genuinely load-bearing. For the first group it silently returns a default, which makes AssertNotCalled vacuous. Nothing at the call site distinguishes the two.
  • Failure scenario: a reviewer sees two adjacent lines - mockStore.AssertNotCalled(t, "GetAllPurchaseHistory", ...) (real) and mockStore.AssertNotCalled(t, "WithTx") (vacuous), as at internal/api/handler_purchases_test.go:2725-2726 - and reasonably concludes both are guarding. That is how the vacuity stayed invisible across 263 AssertNotCalled call sites.
  • Fix direction: pick one policy for the whole mock. If defaults must stay, make them opt-in and explicit (WithDefaultWithTx()) so a bare mock panics on unregistered calls and the call site shows which policy is in force.
  • Related: the isExpected CRITICAL filed separately from this review; test: consolidate hand-rolled ConfigStore mocks (3 copies, taxes every new store method) cloud-commitments-platform#76 (mock consolidation).

• internal/mocks type-assertion failures panic instead of t.Fatal

  • Where: 40+ sites: internal/mocks/stores.go:64, 88, 116, 135, 163, 206, 229, 276, 289, 302, 350, 369, ...; internal/mocks/secretsmanager.go:24, 37, 50; internal/mocks/ses.go:24; internal/mocks/sns.go:24. Also ci_cd_sanity_tests/pkg/sanity/azure/azure_test.go:47, where the accountJSON helper panics on a json.Marshal error.
  • What: a mis-typed .Return(...) - the single most common mock-authoring mistake, e.g. returning []config.PurchaseHistoryRecord where []*config.PurchaseHistoryRecord is expected - aborts the goroutine via panic rather than failing the test cleanly.
  • Failure scenario: the panic bypasses t.Cleanup, so the Postgres testcontainer started by a neighbouring integration test is never terminated and leaks on the CI runner. The Go test output shows a raw stack trace instead of naming the offending test and method, turning a two-line fix into a bisect.
  • Fix direction: thread *testing.T into the mock (a t field set by a constructor) and use t.Fatalf plus t.Helper(). azure_test.go:47 should take t and use require.NoError. Matches the repo's feedback_no_panic_in_test_setup convention.

• docs/ci-required-checks.md prescribes a required check that would block every non-frontend PR

  • Where: docs/ci-required-checks.md:30-76 (Steps 2 and 3 add Build frontend to required_status_checks.contexts) versus .github/workflows/frontend-build.yml:20-27, whose trigger is on: pull_request: paths: ["frontend/**", ".github/workflows/frontend-build.yml"].
  • What: the doc's job-name claim is correct (jobs.build.name: Build frontend, line 38), but a path-filtered workflow does not report a status at all on PRs that touch no matching path.
  • Failure scenario: an admin follows the runbook verbatim. Every backend-only, Terraform-only or docs-only PR then sits permanently at "Expected - Waiting for status to be reported" and cannot be merged, with no failing check to point at. The runbook's own Step 5 smoke test (introduce a TS error in frontend/) passes and would not surface this, because that PR does touch frontend/.
  • Fix direction: add a paths-less companion job that reports Build frontend as a no-op skip (the standard "required check on a path-filtered workflow" workaround) and document the reason in the doc, so the next person does not remove it as redundant.
  • Related: Do not add the CodeRabbit status to required checks: it reports success while throttled cloud-commitments-platform#85 (Do not add the CodeRabbit status to required checks) - same document, same class of "this status will not always report" trap.

LOW

• known-issues.md names an ARM role that does not exist in the template

  • Where: known-issues.md:15-17 says arm/CUDly-CrossSubscription/template.json "has been updated (fix/731-arm-roles) to add a custom role "CUDly Reservation and Savings Plan Purchaser"", versus arm/CUDly-CrossSubscription/template.json:40, where roleName is "CUDly Reservation Purchaser (custom)". git grep "CUDly Reservation and Savings Plan Purchaser" over arm/ returns nothing.
  • What: the substance is on main - the template at lines 46-57 does enumerate Microsoft.Capacity/calculatePrice/action, reservationOrders/write, Microsoft.BillingBenefits/savingsPlanOrderAliases/write and the rest - so only the role name and the "on branch fix/731-arm-roles" framing are stale.
  • Failure scenario: an operator verifying the remediation searches the Azure portal or az role definition list for the documented role name, finds nothing, and concludes the fix was never deployed. They then re-run the ARM deployment or file a duplicate 403 report.
  • Fix direction: correct the role name in known-issues.md, drop the branch reference, and move the entry to the Resolved section per the repo's known-issues convention.

• tests/e2e /version test asserts only that the body is non-empty

  • Where: tests/e2e/e2e_test.go:100-124 (TestVersionEndpoint) decodes into map[string]any and asserts len(body) != 0.
  • What: the test is titled "serves JSON build metadata" but names no field.
  • Failure scenario: if the version handler regressed to emitting {"error":"not configured"} or a stub {"ok":true}, losing the commit SHA that deploy verification and the rollback runbooks rely on, the test still passes. The endpoint exists specifically so a deploy can be verified; a test that cannot tell whether it carries a SHA does not protect that.
  • Fix direction: assert the specific keys the deploy runbooks read (version, commit).
  • Related: feat(api): /version endpoint exposing git SHA + build time for deploy verification #900 (feat(api): /version endpoint exposing git SHA + build time for deploy verification) - the purpose this test is meant to guard.

Related: LeanerCloud/cloud-commitments-platform#35, LeanerCloud/cloud-commitments-platform#36, #1177, #1262, LeanerCloud/cloud-commitments-platform#76, LeanerCloud/cloud-commitments-platform#85, #900, #1294.

Scope after the split

Re-verified against each repo's current tree (2026-09-28):

• The README.md phantom-flags finding is now moot: this repo's README.md was completely rewritten during the split (down to ~57 lines) and no longer contains the "Example 2" section or any mention of --rds-coverage/--elasticache-coverage/--ec2-coverage. No action needed.
• The frontend/, internal/mocks/, docs/ci-required-checks.md, known-issues.md, and tests/e2e/ findings went to chore(test): medium/low findings from the 2026-07-28 full review [platform part] cloud-commitments-platform#353 (none of those paths exist in this repo).
• The ci_cd_sanity_tests half of the mock-panic finding went to chore(test): medium/low findings from the 2026-07-28 full review [go part] cloud-commitments-go#122.

This issue stays open, scoped to the one finding that is still this repo's own: the docs/cli/purchase-safety.md --idempotency-window validation claim, re-verified still incorrect as of this split:

• docs/cli/purchase-safety.md:75-88 still says "This flag is accepted as a Go duration string ... The CLI does not validate the string at startup; it stores the raw value...". That is still factually wrong: pkg/config/load.go:356-363 (in cloud-commitments-go, this repo's dependency) calls time.ParseDuration(v) and returns fmt.Errorf("--idempotency-window: %w", err) on failure, so an invalid duration still aborts the run at startup. The doc's other claim, that the parsed value never reaches the CLI purchase loop, is correct and is already stated accurately elsewhere in the same doc and in this repo's README.md ("The CLI's --idempotency-window flag does not prevent duplicate purchases in the CLI path").
  • Fix direction: change the sentence to "the value is parsed at startup and an invalid duration aborts the run, but the parsed value is not consumed by the CLI purchase loop."