[cristim]

Closes #1465.

Problem: scanExecutionRows (internal/config/store_postgres.go) scanned the nullable purchase_executions.error column straight into the string field exec.Error. One row with a NULL error failed the scan for the whole result set (cannot scan NULL into *string). The history page then lost every actionable execution (pending, notified, paused, scheduled) for every viewer, and the approval queue went empty.

Change: the scan goes through a sql.NullString and sets exec.Error to "" when the column is NULL. Every purchase-execution reader uses this scanner: GetExecutionsByStatuses, ListHistoryExecutionsPage, GetPendingExecutions, GetExecutionByID, and the ramp and scheduled readers. The only other error scan, for RI exchanges, already uses NullString. Writes are unchanged, and there is no migration.

Test: a real-Postgres integration test sets one execution's error to NULL with raw SQL. It then checks that all four readers return every row, with Error set to "" for the NULL one. Before the fix it fails with failed to scan execution: can't scan into dest[11] (col: error): cannot scan NULL into *string.

Local:

• go build ./... passes, and go test -race passes on config and api.
• -tags integration passes on config and api (testcontainers Postgres).
• pre-commit passes.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 2 billable files and costs up to $0.50.}

• 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 28 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 62 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

• Source mode: Auto
• Repository: LeanerCloud/cloud-commitments-platform
• Branch: fix/execution-null-error-scan
• File: .coderabbit.yaml
• Commit: 9abec09
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Essentials
• Run ID: ba1a94d1-93c1-4514-acdd-b66ba06bc337

Commits

Reviewing files that changed from the base of the PR and between 529b6e3 and 9abec09.

Files selected for processing: 2

• internal/config/store_postgres.go
• internal/config/store_postgres_execution_null_error_integration_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 1 (cold, fresh context):

VERDICT: APPROVE 9abec09
Findings (none blocking):

• nit internal/config/store_postgres_execution_null_error_integration_test.go:54 - require.NoError on the first reader aborts the test, so pre-fix only GetExecutionsByStatuses is shown failing; the other three readers are covered transitively (same scanExecutionRows). No gap in practice.
• nit same file:62-66 - GetPendingExecutions only checks len==3 and empty Error, not IDs; acceptable since order/filtering is not under test.
• info reachability - no Go writer stores NULL: SavePurchaseExecution binds execution.Error (string) at store_postgres.go:1158 / upsert :1105; ramp_target_met.go:66 binds a string; only other INSERT is :1087. NULL arrives only via legacy/manual rows (column is error TEXT nullable, 000001_initial_schema.up.sql:89; no later NOT NULL). So this is defensive hardening; a backfill + NOT NULL DEFAULT '' migration (issue's optional item) is not done and not required.
  Attacked:
• Completeness: grepped every SELECT/RETURNING over purchase_executions in internal/ and cmd/. All full-row readers (GetExecutionsByStatuses ~:1586, GetPlannedExecutions, GetStaleApproved, ListStuck, GetPendingExecutions, ListRecentSubmitsTx, getExecutionByIDOn, GetExecutionByPlanAndDate, GetScheduledExecutionsDue :2176, TransitionExecutionStatus RETURNING :1330, ListHistoryExecutionsPage history_page.go:212, ramp_step.go:335, ramp_baseline.go:117) route through queryExecutions(On)/scanExecutionRows. Other scanners (history_summary.go:167/190, managed_guard.go:135, ladder_reconcile.go:112, execution_guarded.go:29, plan_schedule_clock.go:66, CountExecutionsByPlanAndStatus, cleanup-lambda COUNT) never read error.
• Other nullable columns in the same row: approved_by, cancelled_by(COALESCE), cloud_account_id, created_by_user_id, retry_execution_id, executed_by_user_id, pre_approval_skip_reason are *string; plan_id/idempotency_key/approval_token_hash already NullString; source, capacity_percent, retry_attempt_n (000044 enforces NOT NULL), no_submission, created_by_system, step_number, status are NOT NULL. No sibling bug. RI exchange error already scanned via NullString (store_postgres.go:3690).
• NULL→"" semantics: SQL already folds NULL to '' (historyExecutionsPredicate, history_summary.go:20 COALESCE(error,'') = ''); Error is json:"error,omitempty", so API output identical. No consumer distinguishes NULL from "".
• Tests: go test -race ./internal/config ./internal/api: 5570 passed. Full -tags integration ./internal/config under heavy.lock: ok (44s). New test passes.
• Mutation 1 (scan back into &exec.Error): new test FAILS with "cannot scan NULL into *string". Mutation 2 (keep NullString, drop exec.Error = errText.String): TestRecoverReapedExecution_FailedRowBecomesCompleted fails, so non-NULL error round-trip is guarded. Restored via cp; cmp vs git show HEAD:file byte-identical.
• git merge-tree --write-tree origin/main HEAD: clean (820123ce).
• Six dims: correctness ok; security n/a; no races (local var per row); no duplication (reuses existing NullString pattern); no over-engineering; scope = 2 files, +76/-1; gocyclo unchanged (no new branch).

[cristim]

Independent review 2 (claude-fleet, cold): APPROVE at 9abec09

Blocking findings: none.

Non-blocking findings:

1. internal/config/types.go:1042 and internal/config/store_postgres.go:2816: the same bug class exists on purchase_history. ramp_step is a nullable INTEGER (000001_initial_schema.up.sql:118), and scanPurchaseHistoryRow scans it into a plain int. A scratch test that set ramp_step to NULL on one row broke GetAllPurchaseHistory with "can't scan into dest[15] (col: ramp_step): cannot scan NULL into *int". The app cannot reach this today, because SavePurchaseHistory (store_postgres_history_save.go:91) always binds an int, so only raw SQL can store a NULL there. Worth a follow-up issue: use sql.NullInt32, or a migration to NOT NULL DEFAULT 0.
2. The NULL error the PR fixes cannot be reached through the app either. The only production INSERT (store_postgres.go:1087-1146) and the ramp-target-met UPDATE (store_postgres_ramp_target_met.go:66) bind a Go string. scripts/mark_orphan_executions_failed.sql:40 writes a literal. So NULL comes only from raw SQL or old data. The issue's optional migration (backfill to '' plus NOT NULL DEFAULT '') would close this at the root. Fine as a follow-up.
3. Nothing distinguishes NULL from "". The only SQL reader uses COALESCE(error,'') (store_postgres_history_summary.go:20), and Go code reads exec.Error as a string. Mapping NULL to "" keeps every reader's semantics.
4. The test (store_postgres_execution_null_error_integration_test.go:52-70) covers 4 readers but not the transition RETURNING path (store_postgres.go:1328), GetScheduledExecutionsDue (store_postgres.go:2194) or the ramp readers. All of them share scanExecutionRows, so one scanner covers them. My scratch test also proved TransitionExecutionStatus fails on base and passes on head.
5. Already on base, not from this PR: golangci-lint reports prealloc at store_postgres_history_page_integration_test.go:141.

Scan-site table (site / columns / NULL-safe?):

• scanExecutionRows (store_postgres.go:2090). Callers: queryExecutions/queryExecutionsOn at 1599, 1710, 1733, 1779, 1800, 1914 (GetExecutionByID), 1941; transition RETURNING at 1338; GetScheduledExecutionsDue at 2194; history page at store_postgres_history_page.go:225; store_postgres_ramp_baseline.go:123; store_postgres_ramp_step.go:342. All 29 columns / yes after the fix.
• History summary (store_postgres_history_summary.go:190). status, cloud_account_id and created_by_user_id (*string), plus recs_key, which the query COALESCEs. Does not read error. Yes.
• readChainNode (store_postgres_ladder_reconcile.go:107). retry_execution_id goes to *string. Yes.
• lockExecutionRow (store_postgres_execution_guarded.go:28), RescheduleAwaitingExecution (store_postgres.go:1283), plan_schedule_clock:58, managed_guard:128, store_postgres.go:2017, RETURNING execution_id/status at 907, 1451 and 1522. Each reads only NOT NULL columns or scans into a pointer. Yes.
• No pgx RowToStruct use for executions. No other package queries the table except cmd/cleanup-lambda (COUNT/DELETE) and testhelpers/lockwait.go (execution_id).

Nullable-column table (column / type / nullable / destination / NULL reachable? / scratch result):
The schema comes from information_schema after all migrations up to 000139. Every column below is nullable, and its scratch result is OK on all 6 readers. The readers are GetExecutionsByStatuses, ListHistoryExecutionsPage, GetPendingExecutions, GetExecutionByID, HistorySummaryAggregate and TransitionExecutionStatus.

• plan_id uuid / NullString / yes, direct-execute rows (000033)
• notification_sent, completed_at, expires_at, approval_token_expires_at, executed_at, scheduled_execution_at timestamptz / NullTime / yes
• error text / NullString (was string) / raw SQL only / base FAIL on 5 readers, head OK
• cloud_account_id, created_by_user_id, retry_execution_id, executed_by_user_id uuid / *string / yes
• approved_by, pre_approval_skip_reason text / *string / yes
• cancelled_by, canceled_by text / COALESCE into *string / yes
• idempotency_key text / NullString / yes, pre-000066 rows
• approval_token_hash varchar(64) / NullString / yes, 000100
• approval_token varchar, transitioned_by uuid, transitioned_at timestamptz, ladder_run_id uuid / not scanned / n.a. / OK
• The NOT NULL columns (source, capacity_percent, retry_attempt_n, no_submission, created_by_system, recommendations, costs, step_number, status) scan into value types correctly.
• Open PR feat(plans): settle a failed-with-submission row so a managed plan can be deleted #1241 migration 000143 adds outcome_settled_at, outcome_settled_by and outcome_settlement, all nullable. Its readers scan them into pointers, so they are safe. PR fix(config/db): seed-group rollbacks, partition-safe 000027 dedupe, fail-closed plan providers, revocation_in_flight clear #1268 migration 000146 only updates purchase_history.
• purchase_history (17 nullable columns, each set to NULL in the scratch test): only ramp_step fails (int, see finding 1). The other 16 are OK.

Probes (in the head export, each restored with cp and checked with cmp):

• Fail-before: head's test on base fails at dest[11] "cannot scan NULL into *string" on all 4 readers.
• Probe 1, revert to a plain string: FAIL, same scan error.
• Probe 2, map NULL to the sentinel "": FAIL on the assert.Empty checks.
• Probe 3, skip rows whose error is NULL: FAIL on the ID-list assert (expected 3 rows, got 2).
• Scratch test that sets NULL on every nullable column: results above. I deleted it afterwards and diffed internal/config against the head SHA: identical.

Commands (GOTOOLCHAIN=go1.26.10, cloud credentials nulled):

• go build ./...: OK.
• go vet ./internal/config/: OK. go vet -tags integration ./internal/config/ ./internal/api/: OK.
• gofmt -l internal/config/: clean.
• go test -count=1 ./internal/config/ ./internal/api/: ok.
• golangci-lint run ./internal/config/... (with and without --build-tags integration): 1 issue, the prealloc in finding 5. Nothing in the diff.
• go test -tags integration ./internal/config/ (full, under nice + lockf heavy.lock): ok (44s). Same for -run History ./internal/api/: ok.
• git merge-tree --write-tree origin/main (529b6e3) 9abec09: clean (820123ce). store_postgres.go is unchanged from base to main. No migration.
• Cleanup: I removed the scratch test, logs and scripts. There was no build output beyond the Go cache.

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

[cristim]

Local verification at 9abec09df3c751722513c380fa6fdc042eb24236 merged onto current main (08105c87; merge-tree 9cc5b59e; 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.