[cristim]

Part of #202. This PR doesn't close it: part 1, the container-flake retry, stays open as "measure first". First numbers are below.

Problem: three Postgres integration tests in internal/config read a dedicated database URL from an env var and t.Skip when it's unset. CI never sets these vars, so the tests never ran there, which is a false green:

• TestPostgresStoreDB_AutoPurchaseStamps_FreshMigrateAndRIWritePreserve
• TestPostgresStoreDB_StepCapStoredEqualsCompared (both in store_postgres_auto_purchase_stamps_integration_test.go, AUTO_PURCHASE_STAMPS_TEST_DATABASE_URL)
• TestPostgresStoreDB_LadderArming (store_postgres_ladder_arming_integration_test.go, LADDER_ARMING_TEST_DATABASE_URL)

Change:

• All three now get a fresh migrated database from testhelpers.RequireMigratedDatabase, the same way test(ri-exchange): run the daily-cap reservation Postgres tests in CI #1088 moved the RI-exchange reservation tests. The env read, the skip and the dedicated-database prefix checks are gone, and so is the now-redundant RunMigrations call, since the database arrives migrated. Every assertion is unchanged, including the "a fresh migrate leaves the switches OFF" checks.
• Both files are added to the parse-only guard in ri_exchange_reservation_integration_gate_test.go. It fails on any os.Getenv, os.LookupEnv or Skip call in a listed file, so an env gate can't come back unnoticed.

Local (macOS, Docker Postgres via testcontainers):

• go test -race -tags integration -count=1 -v passes all three tests with no SKIP lines.
• The guard passes without the integration tag.
• Mutation: adding os.Getenv("X") back into the ladder file fails the guard, with and without the tag.
• go vet (with and without -tags integration) and gofmt are clean.
• Pre-commit ran with SKIP=hadolint,actionlint,zizmor,cfn-lint, because those hooks' Python and Docker environments can't be built locally. They don't apply to Go test files, and CI runs them.

Part 1 (measure first): none of the last 25 failed CI - Build & Test runs (2026-10-25 to 2026-10-26) failed on a container start, readiness or Docker-daemon error. That's two days of data, so it's a first sample, not a reason to close part 1.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Tests
  • Integration tests for auto-purchase stamps and ladder arming now use fresh, migrated databases from the shared test environment.
  • Both test suites are included in the integration-test gate. This standardizes database setup across these tests and ensures they run as part of the gated integration checks.

[coderabbitai]

No actionable comments were generated in the recent review.

Recent review info

Run configuration

• Source mode: Auto
• Repository: LeanerCloud/cloud-commitments-platform
• Branch: test/202-env-gated-config-tests
• File: .coderabbit.yaml
• Commit: 01fbda9
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Essentials
• Run ID: ac84328e-f0b2-4a55-bcef-de1306c83468

Commits

Reviewing files that changed from the base of the PR and between 53f60c2 and 01fbda9.

Files selected for processing: 3

• internal/config/ri_exchange_reservation_integration_gate_test.go
• internal/config/store_postgres_auto_purchase_stamps_integration_test.go
• internal/config/store_postgres_ladder_arming_integration_test.go

Included review availability: This review used your included allowance. 1 included review remains after this review. Your included PR review attempts over the past 7 days set your current allowance at 2 reviews per hour.

Walkthrough

Three PostgreSQL integration tests now use a shared migrated test database. The environment-gate guard now includes the auto-purchase stamps and ladder arming test files.

Changes: PostgreSQL integration test setup

Layer / File(s)	Summary
Shared migrated database setup and gate coverage internal/config/store_postgres_auto_purchase_stamps_integration_test.go, internal/config/store_postgres_ladder_arming_integration_test.go, internal/config/ri_exchange_reservation_integration_gate_test.go	The auto-purchase stamps and ladder arming tests use the shared database helper instead of dedicated database URLs and direct pool and migration setup. The environment-gate guard checks both test files.

Priority: Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Other

Possibly related PRs

• LeanerCloud/cloud-commitments-platform#1402: It selects and runs integration-tagged tests; this PR removes environment-variable skips from three integration tests.

Suggested reviewers: iamjsd

Merge Risk: Minimal · up to 01fbd

The tests now use migrated test databases instead of dedicated URLs. No merge-blocking risk was identified.

Pre-merge checks: 5

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title "test(config): run the auto-purchase-stamp and ladder-arming Postgres tests in CI" matches the main change. The PR converts three Postgres integration tests in internal/config from env-var-g…
Docstring Coverage		Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within survi…
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

Full details: Title check

Explanation

The title "test(config): run the auto-purchase-stamp and ladder-arming Postgres tests in CI" matches the main change. The PR converts three Postgres integration tests in internal/config from env-var-gated skips to fresh databases from testhelpers.RequireMigratedDatabase, so they run without dedicated database URLs. It also adds the two test files to the parse-only guard. The title does not mention the guard, but it names the tests whose behavior changed. The title is specific and uses conventional commit format.

Full details: Docstring Coverage

Explanation

Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 4 functions across 3 files.

Finishing Touches

📝 Generate docstrings

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

• 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 01fbda9

Scope: the change is test-only. Three integration tests in two internal/config files move from an env-var DSN plus t.Skip to testhelpers.RequireMigratedDatabase, and both files are added to the existing env-gate AST guard. Base is 53f60c2.

Findings:

• internal/config/ri_exchange_reservation_integration_gate_test.go:45: the guard's error text still says "use testhelpers.RequirePostgresContainer instead", but the two files added here correctly use RequireMigratedDatabase. Naming either helper would be clearer. [non-blocking, nit]

Attacked:

• Fresh database per test: RequireMigratedDatabase (testhelpers/shared.go:61, requireSharedDatabase :87) clones a per-test database from the migrated template and registers its drop. So the "a fresh migrate leaves the kill switch / both ladder switches OFF with NULL stamps" assertions really see a fresh migration, not another test's writes. Dropping the dedicated r1a_/pr2_ name check loses nothing, because the database is now private to the test.
• Coverage was actually gained. On base, with the env vars unset (as in CI), all three tests SKIP. On head they run and pass: AutoPurchaseStamps_FreshMigrateAndRIWritePreserve 2.67s, StepCapStoredEqualsCompared 4.48s, LadderArming 0.05s.
• Guard probes, each restored and checked with cmp:
  • Running head's guard (with the new file list) against the base files flags 6 env-gate calls (os.Getenv plus t.Skip in each of the three tests), so the guard would have caught the old pattern.
  • Re-adding a t.Skip behind testing.Short() in the ladder file on head fails TestRIExchangeReservationIntegrationTestsAreNotEnvGated at :23:3.
• The guard file has no build tag, so it runs in the default go test, while the files it parses keep //go:build integration. That is right: the guard parses them rather than compiling them.

Verified locally (GOTOOLCHAIN=go1.26.10; AUTO_PURCHASE_STAMPS_TEST_DATABASE_URL and LADDER_ARMING_TEST_DATABASE_URL unset; heavy runs under the lock):

• gofmt -l internal/config: clean. go vet, with and without -tags integration: OK.
• go test -tags integration -count=1 -v for the guard and the three tests: 4 PASS, none skipped.

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 01fbda9
PR #1532 is OPEN (not merged). Part of #202, test-only, 1 commit, +14/-51 across 3 files.

Findings:

• nit internal/config/ri_exchange_reservation_integration_gate_test.go:24-30 - the comment the PR edits still says the listed tests "now get their database from testhelpers.RequirePostgresContainer", and the error at :45 says "use testhelpers.RequirePostgresContainer instead". All five listed files (and both new ones) use RequireMigratedDatabase, and RequirePostgresContainer's own doc says to prefer RequireMigratedDatabase. Someone fixing a guard failure is pointed at the per-test-container helper. The edit also leaves an odd line break ("ran nowhere. They now" / "get their ..."). The var and test names (riExchangeReservation...) no longer describe the list. All cosmetic.
• nit (existing, not introduced here) gate_test.go:66-77 callName - the parse-only guard misses an aliased import (goos "os"; goos.Getenv) and a function value (ge := os.Getenv; ge("X")); both probes below passed the guard. A deliberate evader is out of scope for a regression guard. Optional follow-up: resolve the import name from f.Imports.

No blocking findings.

Attacked:

• Semantics vs the old version: the old tests read AUTO_PURCHASE_STAMPS_/LADDER_ARMING_TEST_DATABASE_URL, checked the r1a_/pr2_ prefix, then ran RunMigrations on that DB. Now RequireMigratedDatabase (internal/database/postgres/testhelpers/shared.go:61-111) gives each test its own CREATE DATABASE ... TEMPLATE clone of a template built only by migrations.RunMigrations (shared.go:238-257). The template is created from pristine template1 and sealed with ALLOW_CONNECTIONS false, so no test can write to it. Nothing else seeds it, and the clone is dropped WITH (FORCE) at cleanup. The "fresh migrate" premise holds, more strictly than before: the old dedicated DB was only fresh on its first run, since the test sets the switches ON. Every assertion is byte-identical, and pool.Exec/QueryRow now run on *database.Connection, which compiles and runs.
• Could the "fresh" asserts pass for the wrong reason? A missing global_config row would make the later UPDATE ... WHERE id=1 a no-op, and assert.True(AutoPurchaseEnabled) would fail, so the row must exist. Probe P6 below shows the asserts track the template.
• Isolation and parallelism: there is no t.Parallel in any internal/config *integration_test.go. Each test gets a uniquely named database (seq under the mutex), so it shares only the server. -count=2 of the stamps and ladder tests passes, which shows the second iteration sees a fresh DB rather than the first one's writes. The same command would fail on the old shared dedicated DB.
• CI selection: these are //go:build integration files in internal/config, which is the "config" shard (ci.yml INTEGRATION_SHARDS). scripts/integration-test-selection.sh ./internal/config lists all three names in the -run regex. The job runs go test -count=1 -v -race -tags=integration and fails if fewer top-level results than selected. "Integration Tests (config)" was still pending when this review ran, so its result at this SHA is not verified here.
• Local run on macOS with Docker testcontainers, under lockf /tmp/claude/heavy.lock: go test -race -count=1 -tags integration -v -run '<3 tests>|TestRIExchange...NotEnvGated' ./internal/config/ gave PASS for all four (stamps 2.82s, StepCap 5.66s, LadderArming 0.06s, guard 0.01s), with no SKIP lines.
• Mutation probes, run on a scratch copy (files restored, cmp byte-identical; probe migration removed):
  P1 os.Getenv in the ladder file: guard FAIL at :25:6 (caught).
  P2 os.LookupEnv in the stamps file: guard FAIL at :24:9 (caught).
  P3 if false { t.Skip("x") } in the stamps file: guard FAIL at :23:13 (caught).
  P4 aliased goos "os" + goos.Getenv: guard PASS (missed; existing gap, nit above).
  P5 ge := os.Getenv; ge("X"): guard PASS (missed; same gap).
  P6 extra migration 000999 setting auto_purchase_enabled and ladder_execution_enabled TRUE: both tests FAIL on "a fresh migrate leaves ... OFF". The fresh assertions really read the migrated template.
• Remaining env-reading DB tests in internal/ (git grep TEST_DATABASE_URL / os.Getenv at the head); none skips without a database:
  internal/config/store_postgres_ladder_settled_integration_test.go:123 LADDER_SETTLED_TEST_DATABASE_URL: optional override, falls back to setupLadderStore (shared container). The plan file only mentions it in a comment.
  internal/server/handler_ladder_immediate_integration_test.go:37 LADDER_B1_TEST_DATABASE_URL: falls back to RequireMigratedDatabase.
  internal/database/postgres/migrations/migrate_cancel_integration_test.go:31 CUDLY_TEST_ADMIN_DSN: falls back to RequireEmptyDatabase.
  internal/analytics/postgres_analytics_db_test.go:39 SKIP_DB_TESTS: an explicit opt-out skip that CI never sets, already documented as deliberate.
  internal/api/federation_bundle_terraform_integration_test.go:37: skips on a missing terraform binary, not a DB.
  None needs converting for fix(test): container flakes now fail CI after #1826, and a same-family false green remains in store_postgres_db_test #202. The two env overrides with fallbacks could optionally join the guard list later.
• Scope and blast radius: test-only, with no production, migration or CI change. The only other caller of the guard list is this guard. Security: no error paths or secrets touched. Duplication and over-engineering: the change deletes code and reuses the test(ri-exchange): run the daily-cap reservation Postgres tests in CI #1088 pattern.

[cristim]

Local verification at 01fbda900f9959c0ab08389eb811eb12439d0c89 merged onto current main (53f60c27; merge-tree a9a078eb; macOS):

• go build ./..., go vet -tags integration ./... and go test -race -short ./... pass (full suite).
• go test -tags integration passes on real Postgres (testcontainers) for the touched packages: ./internal/config
• Nothing was applied to any cloud.