[IAmJSD]

Root cause

ReplaceRecommendations (internal/config/store_postgres_recommendations.go) ran DELETE FROM recommendations with no WHERE on provider, account or collection time, then reinserted. Any caller handing it a partial or empty snapshot would have wiped every account's recommendations, not just its own. It had no production caller: the only non-test reference was the ConfigStore interface declaration.

Fix

This takes the issue's preferred option 1 and removes the function.

Before removing it I checked origin/main for any caller:

• Grepping the whole repo (code, tests, docs) found only the store, the interface, three fakes and tests.
• The PR searches for ReplaceRecommendations (open and all states) return nothing that depends on it.
• None of the open PRs that touch these files adds or removes a call to it in its diff.
• No document mentions a planned consumer.

Changes:

• Store: ReplaceRecommendations is removed. It was the only caller of the batch insert with onConflict=false, so that parameter is gone too and the natural-key ON CONFLICT ... DO UPDATE clause is now unconditional. That is exactly what UpsertRecommendations already sent; leaving a parameter that is always true would also trip unparam.
• Interface and fakes: the method is removed from ConfigStore (internal/config/interfaces.go, with its comments updated), internal/mocks/stores.go, internal/server/test_helpers_test.go and internal/analytics/collector_test.go.
• Tests:
  • Deleted the two that only exercised the removed function: TestPostgresStore_ReplaceRecommendations and the ReplaceRecommendations stores the same AZ pair subtest of TestPostgresStore_UpsertRecommendations_MultiAZBatch.
  • Removed the Replace entry from the rollback-site table and the replace branch from TestPostgresRecommendationSourceEncodingRollback. The Upsert half of each still runs.
  • The filter-pushdown and freshness round-trip tests, plus both RI exchange reshape integration seeds, now write through UpsertRecommendations(..., nil).
  • The five subtests that called Replace only to empty the table now use a test-only wipeRecommendations helper.
• Clearing last_collection_error and the partial marker: TestPostgresStore_CollectionPartial_RecordedAndCleared still covers this for UpsertRecommendations. It sets an error and a partial marker, upserts, and asserts both are nil. Only the Replace half of that test was removed.
• Guard test: the new internal/config/recommendations_scoped_delete_guard_test.go adds TestNoUnscopedRecommendationsDelete.
  • It parses every non-test Go file under internal/ and cmd/, and fails on any string literal that deletes from recommendations without a WHERE in the same literal (SQL comments are stripped first), or that truncates it. Schema-qualified and quoted table names count.
  • A self-test, TestUnscopedRecommendationsDeletesMatcher, checks that the matcher flags the unscoped, lowercase, schema-qualified, quoted, WHERE-only-in-a-comment and TRUNCATE forms, and ignores scoped deletes and recommendations_state.
  • It is a best-effort tripwire, not a guarantee: SQL assembled from separate literals ("DELETE FROM " + table) gets past it.
• Comments: two stale ones in internal/api/purchase_pricing.go and its test now refer to the refresh's stale-row eviction.

Concurrency: this PR changes no write path's behavior. It removes a function and inlines a constant true, so I found no new race to test.

Verification

All evidence is local unit, mock and real-Postgres (testcontainers) on macOS. Tests ran with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null because this machine's default AWS profile is invalid.

The guard fails before the fix and passes after. With origin/main's store_postgres_recommendations.go swapped back in:

$ go test ./internal/config/ -run 'TestNoUnscopedRecommendationsDelete|TestUnscopedRecommendationsDeletesMatcher' -count=1
--- FAIL: TestNoUnscopedRecommendationsDelete
    recommendations_scoped_delete_guard_test.go:56: .../internal/config/store_postgres_recommendations.go:38:28: unscoped DELETE FROM recommendations; scope it to the accounts being written (see UpsertRecommendations)
FAIL

With the fix, both tests pass.

Build, vet and tests:

• go build ./... passes.
• go vet passes on config, mocks, server, analytics and api, and again with -tags integration on config and api.
• go test -count=1 passes for each package, run one at a time: ./internal/config/, ./internal/mocks/ (the repo-wide guards), ./internal/analytics/, ./internal/server/, ./internal/api/ and ./internal/scheduler/.
• Integration tests on real Postgres:
  • go test -tags integration ./internal/config/ -run 'TestPostgresStore_UpsertRecommendations|TestPostgresStore_ListStoredRecommendations|TestPostgresStore_Freshness|TestPostgresStore_CollectionPartial|TestPostgresRecommendationSourceEncodingRollback|TestTxRollbackFailureIsLogged|TestNoUnscoped|TestUnscoped' passes (17 top-level tests).
  • go test -tags integration ./internal/api/ -run TestReshapeRecommendations_Integration passes (4 tests).

Other checks:

• golangci-lint v2.10.1 with --allow-parallel-runners, with and without --build-tags integration, reports 0 issues.
• gocyclo -over 10 -ignore "_test\.go" on the changed non-test files is silent.
• git-secrets ran on all changed files in a fresh git init temp repo with an isolated HOME, using --register-aws plus the patterns from scripts/setup-git-secrets.sh and the repo's .gitallowed. The scan is clean, and a planted account ID was caught.
• gofmt -l is clean and git diff --check finds no whitespace errors.
• gosec isn't installed here, so it didn't run locally.

Not covered: nothing here needs real cloud credentials. The change is DB-layer only, and nothing was deployed.

Out of scope / notes

• Migration comment left as is: a SQL comment in internal/database/postgres/migrations/000129_recommendations_state_partial.up.sql still says "UpsertRecommendations/ReplaceRecommendations reset it to NULL". I didn't edit it because the migration is already applied.
• last_collection_error design is unchanged: it is still a single global row (recommendations_state WHERE id = 1). As the issue says, that's a separate schema/UX decision.
• test(config): share one migrated Postgres container across integration tests #1165 (shared Postgres container) merged first: origin/main has been merged into this branch and the one conflict, in internal/config/store_postgres_recommendations_test.go, is resolved. The tests use main's RequireMigratedDatabase, and TestPostgresStore_ReplaceRecommendations is dropped along with the method.

Could a maintainer please mirror the issue's triage labels onto this PR? They are impact/internal, triaged, priority/p3, severity/low, urgency/eventually, effort/xs and type/bug.

Closes #159

🤖 Generated with Claude Code

[coderabbitai]

Caution

Review failed

The pull request is closed.

Finishing Touches

🧪 Generate unit tests (beta)

• 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 e9350f8

Against #159 (option 1):

• The PR removes ReplaceRecommendations, the only unscoped DELETE FROM recommendations in production code (base internal/config/store_postgres_recommendations.go:38).
• It also removes the method from ConfigStore (internal/config/interfaces.go:539-550), from the three fakes and from the mock.
• On head and on the merged tree, no .go file references it any more, apart from the new guard test's comment.
• The comment in migration 000129_recommendations_state_partial.up.sql:10 is the only SQL reference left. The body states this, and it is reasonable to keep.
• Dropping onConflict is a pure simplification: Upsert was the only remaining caller and always passed true (store_postgres_recommendations.go:54,184-193).
• No migration.

Findings (none blocking):

• [non-blocking] internal/config/recommendations_scoped_delete_guard_test.go:16-17,101 - The guard only matches a single string literal that contains DELETE FROM recommendations and no WHERE. It passes these evasions:
  • "DELETE FROM " + "recommendations";
  • DELETE FROM public.recommendations;
  • TRUNCATE recommendations;
  • DELETE FROM recommendations -- no WHERE here.
  • It is a best-effort tripwire, not a guarantee. A comment saying so, or covering TRUNCATE and schema-qualified forms, would help.
• [non-blocking, pre-existing] store_postgres_recommendations.go:192 - cloud_account_id = EXCLUDED.cloud_account_id in DO UPDATE has no test (probe B survives).
• No coverage is lost. The removed Replace halves (the rollback, encoding, partial-clear and MultiAZ tests) only exercised the deleted function, and the Upsert halves still cover the partial-marker clear and the error. Test seeds moved to UpsertRecommendations(..., nil) seed once per fresh container, and nil means no eviction.

Attacked:

• Probes on a scratch copy, each restored and confirmed identical with cmp:
  • A, the ON CONFLICT clause removed: 8 Upsert integration tests FAIL.
  • B, cloud_account_id removed from DO UPDATE SET: all pass (the gap above).
  • C, a production file with an unscoped DELETE added: TestNoUnscopedRecommendationsDelete FAILS with file:line.
  • D, the matcher made to ignore WHERE: both guard tests FAIL.
  • E, the evasion forms: the guard passes (the gap above).
• Blast radius:
  • No implementers or callers of ReplaceRecommendations remain, including .On("ReplaceRecommendations") expectations.
  • The DELETE FROM recommendations at scheduler.go:1042 is inside a comment, and the guard correctly ignores it.
  • The guard walks internal/ and cmd/. The Go files outside them (iac/, scripts/) do not touch the recommendations table.
• Drift:
  • None of main's 14 newer commits touch the PR's files.
  • merge-tree with origin/main 7eac792 is clean, and internal/config is identical between head and the merged tree.

Verified locally, with no cloud credentials, under the heavy lock, with testcontainers Postgres:

• Head:
  • gofmt -l: clean. go build ./...: ok.
  • go vet on config, mocks, analytics, server, api and scheduler, plus -tags integration on config and api: ok.
  • go test -count=1 on the six packages: ok.
  • The config integration subset (Upsert, ListStored, Freshness, CollectionPartial, encoding rollback, TxRollback and the guard): ok.
  • api TestReshapeRecommendations_Integration: 4/4 PASS.
• Fail-before: the guard test on base FAILS at store_postgres_recommendations.go:38:28.
• Merged tree: build, vet, the six-package unit tests, the config integration subset and the api reshape integration tests: ok.

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

[IAmJSD]

Merged origin/main to resolve the conflict in internal/config/store_postgres_recommendations_test.go, and addressed the non-blocking findings from Independent review 2 (on e9350f8).

Conflict: internal/config/store_postgres_recommendations_test.go (with #1165, c8bdd46, shared migrated Postgres container)

• Kept main's setupRecommendationsStore on testhelpers.RequireMigratedDatabase. It is used by every test in the file.
• Dropped TestPostgresStore_ReplaceRecommendations, which main had converted to the new helper. This branch removes it along with the method.
• Kept this branch's wipeRecommendations helper (:61). Its comment now says subtests share one test database rather than one container.
• Every other file auto-merged. After the merge, no .go file outside the guard test's comment references ReplaceRecommendations.

Findings

• [non-blocking] The guard misses public.recommendations, TRUNCATE, a WHERE that only appears in a comment, and concatenation. Fixed in 7bb8719, internal/config/recommendations_scoped_delete_guard_test.go:
  • Patterns (:19-22): the delete pattern now accepts ONLY and schema-qualified or quoted names. A new pattern flags TRUNCATE [TABLE] [ONLY] of the table, including when it appears in a table list.
  • Matching (:133-137): the Go literal is unquoted and its SQL comments are stripped before the check looks for WHERE.
  • Self-test (:83): it gained a positive row for each of those forms, plus a double-quoted Go literal, and negatives for recommendations_state. The doc comment (:38) now says the guard is a best-effort tripwire and that SQL assembled from separate literals gets past it.
  • Probes: a scratch production file with TRUNCATE public.recommendations and "DELETE FROM recommendations -- WHERE" failed the guard at both lines. With base's store_postgres_recommendations.go swapped back in, it still fails at :38:28. Both were reverted.
• [non-blocking, pre-existing] Probe B (cloud_account_id = EXCLUDED.cloud_account_id, store_postgres_recommendations.go:193). Left as is: it is out of scope and, as far as I can tell, an equivalent mutant.
  • account_key is GENERATED ... COALESCE(cloud_account_id, zero-uuid) (migration 000030) and is part of the conflict target.
  • So a conflicting row already has the same cloud_account_id, and the SET is a no-op that no test can observe.
• PR body: the stale "possible conflict with test(config): share one migrated Postgres container across integration tests #1165" note now says the merge is done, and the guard bullets describe the wider matcher and its limits.

New head: 7bb8719 (merge 7374a26 plus the guard commit)

Verification on the merged tree (Linux, no cloud credentials, AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null):

• Build and vet: gofmt -l is clean, go build ./... is ok, and git diff --check is clean. go vet passes on config, mocks, analytics, server, api and scheduler, and with -tags integration on config, api and analytics.
• Unit tests: go test -count=1 passes on config, mocks, analytics, server, api and scheduler, one package at a time.
• Integration, config: go test -tags integration ./internal/config/ gives 699 top-level PASS, 3 env-gated SKIP and 0 FAIL, with one exclusion. TestPostgresStoreDB_ResolveStaleRIExchange_ValidatesCloudCheck (subtests "checked at the change" and "checked in the future") fails the same way on a clean git archive of origin/main a14dc8f. It is unrelated to this PR and came in with fix(exchange): alarm on stale processing RI exchanges and let an operator resolve them #1109.
• Integration, api: -run TestReshapeRecommendations_Integration passes 4/4.
• Integration, analytics: the full package ran with -timeout=20m. The only failure was a testcontainers reaper start error (unexpected container status "removing") in TestPostgresAnalyticsStore_RefreshMaterializedViews_DB, which passed when rerun alone. The host's Docker was shared with other test runs at the time.
• Lint: golangci-lint v2.10.1 --allow-parallel-runners reports 0 issues, with and without --build-tags integration.
• Complexity: gocyclo -over 10 on the changed non-test files is silent.
• Secrets: git-secrets ran in a fresh git init repo with an isolated HOME, using scripts/setup-git-secrets.sh and .gitallowed, with the 12 changed files added. The scan was clean, and a planted fake AWS key was flagged. scripts/test-git-secrets-allowlist.sh reports 137 PASS, 0 FAIL.

The PR is still a draft.

[cristim]

VERDICT: APPROVE d1d93f9

PR #1209 (closes #159), head d1d93f9, reviewed against origin/main 6776e27.
Merge-tree: clean (tree 006fbaa18eb6ca3fe4a31ec6dd209265d0dc7978). All results below are on that merged tree.

Findings: none blocking.

• nit internal/config/recommendations_scoped_delete_guard_test.go:146-160 - a literal counts as scoped if WHERE appears anywhere in it (e.g. DELETE FROM recommendations; SELECT .. WHERE ..), and concatenated SQL bypasses it. Documented as best-effort at :38-41; acceptable.
• nit internal/database/postgres/migrations/000129_recommendations_state_partial.up.sql:10 still names ReplaceRecommendations in a comment; migrations are immutable, leave it.
• process: largest commit e9350f8 = 330 changed lines (cap 400), 7bb8719 = 53; the two merge commits are main syncs.

Attacked:

• Callers: grep of the merged tree (code, tests, mocks, scheduler, cmd/CLI, docs) finds no ReplaceRecommendations call; only comments/guard-test text. Sibling cli/go/mcp repos: none (internal/ is not importable anyway). Remaining production DELETE FROM recommendations is only UpsertRecommendations' eviction (store_postgres_recommendations.go:63-70), scoped by collected_at < $1 AND (provider, account_key) IN unnest(successfulCollects); nil successfulCollects skips eviction entirely. Down-migration DELETEs are pre-existing and out of scope.
• Behavior: insertRecommendationsBatch now always emits ON CONFLICT, identical to what Upsert already sent; Replace was the only onConflict=false caller.
• Tests on merged tree: go build ./..., go vet (incl. -tags integration) OK; unit config/scheduler/api/mocks/server/analytics 6048 pass; -tags integration config + scheduler ok, api 13/13 Integration tests pass (incl. reshape scoped-account seed now via Upsert).
• Mutation probes: (1) add const x = \DELETE FROM recommendations`` in internal/config -> TestNoUnscopedRecommendationsDelete FAILS; (2) drop the ON CONFLICT clause -> 5 Upsert integration tests FAIL; (3) widen eviction to provider-only (drop account_key) -> AccountScopedEviction and AmbientAndRegisteredCoexist FAIL (cross-tenant wipe is caught). All reverted.
• CI (gh pr checks): all pass at this head, Snyk skipping; CodeRabbit skipped (draft). Base 503eb2e predates 6776e27 but the merged tree passes locally.

[cristim]

Independent review 2 (claude-fleet, cold), re-review: APPROVE d1d93f9

Delta (e9350f8..d1d93f9, first-parent): 7374a26 is a merge with a hand resolution in internal/config/store_postgres_recommendations_test.go. 7bb8719 is test-only (+45/-8, guard). d1d93f9 is a clean merge.

Findings:

• [non-blocking] internal/config/recommendations_scoped_delete_guard_test.go:36-41 says outright that string concatenation and Sprintf get past the guard. Concat and Sprintf are still not caught (probes E1, E6). A trivially true WHERE (WHERE true, E5) also gets past it. This is acceptable for a tripwire.

Attacked:

• The four evasions I reported, each tested with a probe file under internal/ against TestNoUnscopedRecommendationsDelete:
  • string concat: NOT caught (documented);
  • schema-qualified: caught, both public.recommendations and "public"."recommendations";
  • TRUNCATE: caught, both plain and TRUNCATE TABLE ONLY "recommendations" RESTART IDENTITY;
  • trailing comment: caught, both -- WHERE ... and /* WHERE ... */.
  • An alias with no WHERE is also caught. The recommendations_state negatives stay unflagged.
• Guard mutations, each restored and checked with cmp, each making TestUnscopedRecommendationsDeletesMatcher FAIL:
  • G1: drop the comment stripping;
  • G2: drop the TRUNCATE check;
  • G3: drop the schema-qualifier group.
• Resolution in 7374a26: it keeps the PR's wipeRecommendations and drops main's TestPostgresStore_ReplaceRecommendations, which tests the method this PR removes. It updates the comment to say "test database" to match main's shared-server harness. Nothing in internal/ or cmd/ references ReplaceRecommendations any more, apart from an old comment in applied migration 000129, which correctly stays unedited.
• Main's newer code: grepping non-test Go for DELETE or TRUNCATE on recommendations finds only the scoped eviction at internal/config/store_postgres_recommendations.go:64 (WHERE collected_at < $1 AND (provider, account_key) IN ...) and a comment quoting it in internal/scheduler/scheduler.go:1042. No concat or Sprintf form exists.

Verified locally (merged tree 2c19f9a, GOTOOLCHAIN=go1.26.10, no credentials, heavy lock):

• gofmt -l: clean.
• go build ./... and go vet -tags integration ./internal/config/: rc=0.
• go test ./internal/config/: ok. Both guard tests PASS.
• go test -tags integration -run Recommendations ./internal/config/: 20 PASS, no skips.

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

[IAmJSD]

Local verification at d1d93f94cdbaaedd4ee3c2f9f28a80817e9e77ae merged onto current main (6a4a506e), together with #1140, #1170, #1197, #1201, #1202, #1204, #1205, #1208 and #1209 in that order (combined tree 8b907eb6, Linux, Go 1.26.10):

• go build ./..., go vet ./..., go vet -tags integration ./..., (cd tests/e2e && go vet -tags e2e ./...): clean.
• go test -race -short -count=1 ./...: 36 packages ok, no failures.
• go test -race -tags integration -count=1 on every package the nine PRs change (analytics, api, auth, config, database/postgres/migrations, mocks, server), against Docker Postgres: all ok.

CI on this head: CI Success and pre-commit green. cristim's independent review approved this head. Migrations in this batch (000136-000139) are applied in numeric order. Nothing was run against any cloud.

[IAmJSD]

Marked ready and merged by IAmJSD's agent at IAmJSD's request (IAmJSD is acting owner while the maintainers are away), after cristim's independent review approving d1d93f94 and the combined-tree verification above. Squash-merged at exactly d1d93f94cdbaaedd4ee3c2f9f28a80817e9e77ae.