Two follow-ups deferred from LeanerCloud/cloud-commitments-cli#1826 (which closes LeanerCloud/cloud-commitments-cli#1597), both surfaced by that PR's adversarial review.
1. Container-lifecycle flakes now land red instead of skipping
internal/database/postgres/testhelpers/postgres.go:110-113
LeanerCloud/cloud-commitments-cli#1597 was a false green: a t.Skipf on migration failure turned a real failure into a PASS. LeanerCloud/cloud-commitments-cli#1826 correctly converts that to a hard failure. The consequence, which is correct by design but previously undisclosed, is that a transient container-lifecycle error now fails CI too.
Reproduced during the review, roughly 1 in 10 container starts under churn:
Error Trace: testhelpers/postgres.go:111
migrations/000087_purchase_history_marketplace_listing_test.go:29
Error: failed to get container port: port "5432/tcp" not found
Messages: Docker is healthy but the PostgreSQL test container did not come up
The container logged Container is ready and then MappedPort failed. Confirmed a flake rather than a regression: the same test passes 3/3 when run alone on the branch, and passes on main.
This is not an argument to revert. You cannot distinguish "host exhausted" from "something is genuinely broken" at that point, and choosing red is the fail-loud answer. But CI runs -race across six modules, which is exactly the churn that produced it, so expect intermittent reds.
Deliberately not fixed pre-emptively. A retry layer is its own bug surface, and the proportionate move is to measure the rate first. Act only if it proves painful, and then the narrow fix is one retry of SetupPostgresContainer on a container-lifecycle error specifically, not a blanket retry.
What to do here: watch the rate. If it is material, implement the narrow retry with both directions asserted, i.e. a genuine setup failure must still fail after the retry is exhausted.
internal/config/store_postgres_db_test.go:576-578
t.Run("GetExecutionByID success", func(t *testing.T) {
if generatedExecID == "" {
t.Skip("no execution ID available from prior test")
}
If the earlier subtest fails to produce an ID, this one reports skipped rather than failed. That is the same "answer a failure with no verdict" shape LeanerCloud/cloud-commitments-cli#1597 was filed about. It is not a guarded call, so the new guard from LeanerCloud/cloud-commitments-cli#1826 does not see it.
Out of LeanerCloud/cloud-commitments-cli#1826's stated scope, hence deferred rather than folded in.
Fix direction: fail rather than skip when the precondition was supposed to be produced by a prior step in the same test. A skip is honest only when the environment genuinely cannot provide something; here the environment was expected to provide it and did not.
Worth a sweep for the same shape: a t.Skip whose condition is a value an earlier subtest was responsible for producing. That is distinguishable from an environment skip and is always a false green.
Verification bar for both
Both directions. The genuine failure must fail, and the genuinely-unavailable environment must still skip. A change that makes everything fail breaks every developer machine without Docker; one that makes everything skip restores the bug.
Findings from the 2026-09-02 codebase audit
Added by an automated audit of 3c0f8ac94048a2c36fce5ccddee54e6c4849a5cd (tip of origin/main). Each item below was reported by one reviewer and independently confirmed by a second that did not write it. Full report: docs/audits/codebase-audit-2026-09-02.md.
A04-020 (low)
One more item in this file while it is open, same family of "the harness papers over something". store_postgres_db_test.go:53-59 creates a second unique index on execution_id under the comment "the migration schema does not create a UNIQUE constraint on execution_id". That is false: migration 000008_add_execution_id_unique.up.sql:3 adds CONSTRAINT unique_execution_id UNIQUE (execution_id) and every ON CONFLICT (execution_id) in production depends on it. The harness has already applied the full migration chain at :49-51 before layering the redundant index at :55-56, so the integration suite runs against a schema with two unique indexes and would keep passing if 000008 were ever dropped -- the only regression the extra DDL could have caught. Deleting the DDL and the comment makes the suite run against exactly the migrated schema. (audit finding A04-020)
Two follow-ups deferred from LeanerCloud/cloud-commitments-cli#1826 (which closes LeanerCloud/cloud-commitments-cli#1597), both surfaced by that PR's adversarial review.
1. Container-lifecycle flakes now land red instead of skipping
internal/database/postgres/testhelpers/postgres.go:110-113LeanerCloud/cloud-commitments-cli#1597 was a false green: a
t.Skipfon migration failure turned a real failure into a PASS. LeanerCloud/cloud-commitments-cli#1826 correctly converts that to a hard failure. The consequence, which is correct by design but previously undisclosed, is that a transient container-lifecycle error now fails CI too.Reproduced during the review, roughly 1 in 10 container starts under churn:
The container logged
Container is readyand thenMappedPortfailed. Confirmed a flake rather than a regression: the same test passes 3/3 when run alone on the branch, and passes on main.This is not an argument to revert. You cannot distinguish "host exhausted" from "something is genuinely broken" at that point, and choosing red is the fail-loud answer. But CI runs
-raceacross six modules, which is exactly the churn that produced it, so expect intermittent reds.Deliberately not fixed pre-emptively. A retry layer is its own bug surface, and the proportionate move is to measure the rate first. Act only if it proves painful, and then the narrow fix is one retry of
SetupPostgresContaineron a container-lifecycle error specifically, not a blanket retry.What to do here: watch the rate. If it is material, implement the narrow retry with both directions asserted, i.e. a genuine setup failure must still fail after the retry is exhausted.
2. A same-family false green in a file LeanerCloud/cloud-commitments-cli#1826 already edits
internal/config/store_postgres_db_test.go:576-578If the earlier subtest fails to produce an ID, this one reports skipped rather than failed. That is the same "answer a failure with no verdict" shape LeanerCloud/cloud-commitments-cli#1597 was filed about. It is not a guarded call, so the new guard from LeanerCloud/cloud-commitments-cli#1826 does not see it.
Out of LeanerCloud/cloud-commitments-cli#1826's stated scope, hence deferred rather than folded in.
Fix direction: fail rather than skip when the precondition was supposed to be produced by a prior step in the same test. A skip is honest only when the environment genuinely cannot provide something; here the environment was expected to provide it and did not.
Worth a sweep for the same shape: a
t.Skipwhose condition is a value an earlier subtest was responsible for producing. That is distinguishable from an environment skip and is always a false green.Verification bar for both
Both directions. The genuine failure must fail, and the genuinely-unavailable environment must still skip. A change that makes everything fail breaks every developer machine without Docker; one that makes everything skip restores the bug.
Findings from the 2026-09-02 codebase audit
Added by an automated audit of
3c0f8ac94048a2c36fce5ccddee54e6c4849a5cd(tip oforigin/main). Each item below was reported by one reviewer and independently confirmed by a second that did not write it. Full report:docs/audits/codebase-audit-2026-09-02.md.A04-020 (low)
One more item in this file while it is open, same family of "the harness papers over something".
store_postgres_db_test.go:53-59creates a second unique index onexecution_idunder the comment "the migration schema does not create a UNIQUE constraint on execution_id". That is false: migration000008_add_execution_id_unique.up.sql:3addsCONSTRAINT unique_execution_id UNIQUE (execution_id)and everyON CONFLICT (execution_id)in production depends on it. The harness has already applied the full migration chain at :49-51 before layering the redundant index at :55-56, so the integration suite runs against a schema with two unique indexes and would keep passing if 000008 were ever dropped -- the only regression the extra DDL could have caught. Deleting the DDL and the comment makes the suite run against exactly the migrated schema. (audit finding A04-020)