[cristim]

Summary

Part of #202: part 2 plus A04-020. Part 1 (the container-flake retry) stays open in #202, alongside the env-guarded skips noted there.

internal/config/store_postgres_db_test.go had a false green. GetExecutionByID success read generatedExecID, a variable that GetPendingExecutions sets only inside if len(pending) > 0, and called t.Skip("no execution ID available from prior test") when it was empty. If GetPendingExecutions regressed to return nothing, the dependent test skipped and the suite stayed green.

• GetPendingExecutions now calls require.NotEmpty on the result and on pending[0].ExecutionID.
• GetExecutionByID success saves its own pending execution and then looks it up. The cross-subtest generatedExecID variable and the t.Skip are gone. The file now has no t.Skip. The existing Docker-unavailable skip in RequirePostgresContainer is unchanged.
• A04-020: removed the test-only idx_purchase_executions_execution_id_unique DDL and its inaccurate comment. Migration 000008 already adds unique_execution_id UNIQUE (execution_id), and no later migration drops it. The test DB runs the real migrations (migrations.RunMigrations). The ON CONFLICT (execution_id) upsert (store_postgres.go:1048) and its subtests pass without the extra index.

This is a test-only change in one file, +13/-20.

Evidence

• Mutations:
  • Changing GetPendingExecutions's WHERE clause to match nothing now fails the test ("Should NOT be empty") instead of skipping.
  • Changing GetExecutionByID's WHERE clause to AND 1=0 fails GetExecutionByID success.
• Order independence: -race -shuffle=on -run TestPostgresStoreDB_PurchaseExecutions$ passes all 9 subtests.
• Runtime: ./internal/config/... with CI's flags (-race -tags=integration) passes with 0 failures and 622 results, in 904.7s on this change and 882.0s on its parent. The difference is noise. The package's roughly 15-minute runtime against CI's 20-minute timeout is pre-existing and tracked in ci: internal/config integration tests take ~15 min against a 20 min job timeout #982.
• go vet -tags integration is clean. golangci-lint v2.10.1 reports 0 issues.

Review

An independent non-author ran two passes and found no blocking findings at 853bafe.

Non-blocking:

• GetPendingExecutions still relies on earlier subtests having saved pending rows. Shuffle reorders only top-level tests, and the test now fails loudly if those rows are missing.
• The saved execution row is never deleted. No later subtest counts pending rows, so it has no effect.

🤖 Generated with Claude Code

[cristim]

@coderabbitai review

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

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

• Ask an admin to make reviews automatic

Open in CodeRabbit

Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing.

Or wait 34 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available. Your 64 included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

• Configuration used: Repository: LeanerCloud/cloud-commitments-platform/.coderabbit.yaml
• Review profile: CHILL
• Plan: Essentials
• Run ID: e47184a3-d6eb-4d5d-9999-cf8c7dda4c01

📥 Commits

Reviewing files that changed from the base of the PR and between 06c5109 and 853bafe.

📒 Files selected for processing: 1

• internal/config/store_postgres_db_test.go

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

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

[coderabbitai]

⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[cristim]

Independent review verdict: CLEAN at 853bafe

Reviewer: fresh Claude agent (Sonnet, high effort), not the author, cold brief. The PR body was treated as a claim. Coordinator: claude-cc-reviewer.

Blocking findings: none.

Non-blocking notes:

1. The "false green" is overstated. With GetPendingExecutions mutated to return nothing, main's old test already failed at GetPendingExecutions: its assert.GreaterOrEqual is non-fatal, but it still fails the suite. GetExecutionByID success then skipped. The change is still a real hardening: the skip is gone, and the dependent subtest is now self-contained.
2. GetPendingExecutions (around store_postgres_db_test.go:556) still depends on earlier subtests having saved pending rows. It now fails loudly if they are missing. Shuffle only reorders top-level tests, so this is not a flake.
3. The row saved in GetExecutionByID success is not cleaned up. No later subtest counts pending rows.

The DDL removal is truly redundant:

• migrations/000008_add_execution_id_unique.up.sql:3 creates unique_execution_id UNIQUE (execution_id).
• 000081 (lines 16-17) relies on it for the ladder_tranches FK.
• The ON CONFLICT (execution_id) upsert (store_postgres.go:1048) passes.
• t.Skip no longer appears in the file.

What was attacked and how (local macOS, Docker Postgres via testcontainers):

• go vet -tags integration ./internal/config/ is clean.
• go test -tags integration -count=1 -v -run 'TestPostgresStoreDB_PurchaseExecutions$' ./internal/config/: 9/9 subtests pass, with 0 SKIP.
• Mutation 1: making the pending query return nothing (store_postgres.go:1663) makes the new test fail at GetPendingExecutions. The old file fails there too and skips GetExecutionByID_success.
• Mutation 2: making the GetExecutionByID query return nothing makes GetExecutionByID_success, SavePurchaseExecution_with_all_timestamps and ..._upsert_updates_existing fail.
• Not run by the reviewer: shuffle/-race and the full-package runtime. The body reports the runtime went from 904s to 882s, and the CI timeout risk is tracked in ci: internal/config integration tests take ~15 min against a 20 min job timeout #982.

Gate: labels are mirrored from #202. CodeRabbit is optional because exact-revision local verification plus this review cover the SHA. Merge waits on CI being green at this SHA.