[IAmJSD]

This PR takes the issue's option 2 (remove the dead key). It matches how execution retention works today and it is the smaller change. Option 1 (have the cleanup path read a setting) is still open if the owner wants retention to be tunable per install. That would be a new, wired setting. It wouldn't revive this seed entry, and planHealthLookbackDays would have to read the same value, as the issue notes.

Root cause

retention.execution_history_days (value 90) was an entry in the Go slice config.DefaultSettings, and nothing read it. Execution retention is actually set by the compile-time constant config.DefaultExecutionTTLDays (30). internal/server/handler.go passes that constant to CleanupOldExecutions, and the plan health score's planHealthLookbackDays (internal/api/plan_health.go) reuses it. The config README listed the 90-day key as the retention setting, which is how it came to be cited as policy in review.

Fix

• internal/config/defaults.go: remove the entry. A short comment in its place points at DefaultExecutionTTLDays and its two consumers.
• internal/config/defaults_test.go: remove the key's row from TestDefaultSettings_Retention. Add TestDefaultSettings_NoExecutionRetentionKey, which asserts the key is absent and that DefaultExecutionTTLDays is 30.
• internal/config/README.md: remove the key from "Data Retention". Replace it with a paragraph saying that execution retention is DefaultExecutionTTLDays (30 days), what the sweep deletes and on which column (completed by scheduled_date, canceled by updated_at, plus the expires_at branch; failed executions are never deleted), and that the plan health lookback is the same constant.

No migration: there is no row to delete

The issue (and the plan for this PR) assumed the key is stored as a row in a settings table on existing installs, so a migration would be needed to delete it. That assumption is wrong for the current code, so this PR adds no migration:

• DefaultSettings is an in-memory slice. Outside defaults.go it is referenced only by defaults_test.go and by one comment in store_postgres.go. GetDefaultValue, GetDefaultSetting, GetDefaultsByCategory and GetAllCategories have no non-test callers. Nothing writes these entries to Postgres or anywhere else.
• Every CREATE TABLE in internal/database/postgres/migrations/*.up.sql was checked. No key/value settings table exists. Global settings live in typed columns on the singleton global_config row, and there is no retention column. No migration inserts any retention.* key.
• So a DELETE migration would have nothing to delete, and a down migration would have no table to re-insert into. Adding a no-op one would only use up a migration number (numbers collide often in this repo). For the same reason there is no Postgres migration test.

A related point: the issue says an operator "can change it in the settings UI". No API, openapi schema, validation list or frontend code mentions the key, so it was never editable. The harm was limited to the misleading README and code entry.

Other references checked

• frontend/: no reference to execution_history_days, retention or historyDays in frontend/src or frontend/tests-e2e. No frontend follow-up is needed.
• internal/api/openapi.yaml: the key is absent. The plan-health code description already says the "trailing execution-retention window (30 days)", which is still correct.
• internal/api/plan_health.go (planHealthLookbackDays) and the internal/server/handler.go call site: both comments already cite config.DefaultExecutionTTLDays, not the dead key, so they needed no change.
• No settings validation or allowed-key list mentions the key.

Verification

This is unit-test and static evidence only. The change has no runtime behavior, because the entry had no reader.

Fail-before: the new test was run with only the test edit applied, against unmodified defaults.go:

$ AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null go test ./internal/config/ -run TestDefaultSettings -count=1
--- FAIL: TestDefaultSettings_NoExecutionRetentionKey (0.00s)
    Error: Expected nil, but got: &config.ConfigSetting{Key:"retention.execution_history_days", Value:90, Type:"int", Category:"retention", ...}
FAIL

Pass-after:

$ go build ./...
$ go vet ./internal/config/
$ AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null go test ./internal/config/ -count=1
ok  	github.com/LeanerCloud/cloud-commitments-platform/internal/config
$ AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null go test ./internal/mocks/ -count=1
ok  	github.com/LeanerCloud/cloud-commitments-platform/internal/mocks
$ golangci-lint run --allow-parallel-runners ./internal/config/...   # v2.10.1
0 issues.
$ gocyclo -over 10 -ignore "_test\.go" internal/config/defaults.go   # silent
$ markdownlint -c .markdownlint.yaml internal/config/README.md        # markdownlint-cli 0.47.0, clean
$ gofmt -l internal/config/ ; git diff --check                       # clean
$ git secrets --scan   # in a fresh `git init` repo with isolated HOME, patterns from scripts/setup-git-secrets.sh: clean
$ bash scripts/test-git-secrets-allowlist.sh
42 PASS, 0 FAIL

The tests were run with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null because the default AWS profile on the local machine is invalid. This PR doesn't depend on it.

Out of scope (noted, not changed)

• The rest of DefaultSettings has the same problem. Nothing reads any of it, including retention.purchase_history_days and retention.recommendation_cache_hours. Much of internal/config/README.md also describes a DynamoDB-backed ConfigDBClient that no longer exists, including an "Initialize database with defaults" example. This PR only fixes the key and section the issue covers. A wider cleanup could be its own issue.
• internal/purchase/execution.go (the TODO(#95) comment) says purchase_executions rows have "a ~6-month retention window". That doesn't match the 30-day sweep. It belongs to sec(api): createPlan accepts target_accounts outside the caller's allowed_accounts #95's alias-removal decision, so it was left alone.

Closes #88

Maintainers: please mirror the issue's triage labels onto this PR: impact/internal, type/chore, triaged, priority/p3, severity/low, urgency/eventually, effort/s.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 3 billable files and costs up to $0.75.}

Or wait 59 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration

• Source mode: Auto
• Repository: LeanerCloud/cloud-commitments-platform
• Branch: main
• File: .coderabbit.yaml
• Commit: b6355a8
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: bfdf1bb3-ddec-4bbc-8ad6-023c76d9f9ce

Commits

Reviewing files that changed from the base of the PR and between b6355a8 and b936fe1.

Files selected for processing: 3

• internal/config/README.md
• internal/config/defaults.go
• internal/config/defaults_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 2 (claude-fleet, cold): APPROVE b936fe1

Findings (none blocking):

• [non-blocking, follow-up issue] internal/config/README.md:213-214 - "Data Retention" still lists retention.purchase_history_days and retention.recommendation_cache_hours as settings, though nothing reads them either. The body calls this out of scope, but a reader could still treat them as live policy.
• [non-blocking] internal/config/defaults_test.go:244 - assert.Equal(t, 30, DefaultExecutionTTLDays) only detects a change to the constant. It does nothing for chore(config): retention.execution_history_days is never read at runtime #88, and a deliberate TTL change would have to edit this test too.
• [non-blocking] defaults_test.go:242 - The guard checks the exact key string only, so a dead key under another name (for example execution.retention_days) would pass. That is acceptable for the scope.
• [cosmetic] README.md:222 - One line of the new paragraph is far longer than the rest. MD013 is disabled in .markdownlint.yaml.

Against #88: option 2 is done fully, and "no migration needed" holds.

• The only config tables are global_config, ladder_configs and service_configs. There is no key/value settings table.
• No migration mentions retention.*. The only "retention" hit is a comment in 000003.
• GetDefaultSetting, GetDefaultValue, GetDefaultsByCategory, GetAllCategories and DefaultSettings have no non-test Go callers. The other reference is a comment at store_postgres.go:145.
• Nothing else mentions execution_history, frontend included.
• The README claims match store_postgres.go:2243-2276:
  • completed rows are deleted by scheduled_date;
  • canceled rows (both spellings) by updated_at;
  • the expires_at branch excludes HealthScoredExecutionStatuses (types.go:538), so failed rows are never deleted;
  • handler.go:303 and plan_health.go:91 both use config.DefaultExecutionTTLDays.

Drift: none of main's 14 newer commits (origin/main 7eac792) touch internal/config, plan_health.go or server/handler.go. merge-tree --write-tree origin/main <head> is clean, and the merged tree was built and tested anyway.

Attacked:

• Fail-before: with the head defaults_test.go on base, TestDefaultSettings_NoExecutionRetentionKey FAILS.
• Probes, each restored and confirmed identical:
  1. The key put back in defaults.go: the new test FAILS.
  2. DefaultExecutionTTLDays changed to 31: the new test FAILS.
  3. The key put back under Category "execution": the new test FAILS.
  4. recommendation_cache_hours renamed, which simulates deleting too much: TestDefaultSettings_Retention FAILS.

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

• Head:
  • go build ./... and go vet ./internal/config/ ./internal/mocks/: OK.
  • go test -count=1 ./internal/config/ ./internal/mocks/: ok.
  • gofmt -l: clean.
• Merged tree: go build ./... and the same tests: ok.
• No frontend, terraform, DB code or migration changes.

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 b936fe1

Findings:

• nit internal/config/README.md:222 - one unwrapped long line ("...the health score counts). It never deletes failed executions. The plan health score's lookback window") inside an otherwise hard-wrapped paragraph. MD013 is disabled, so lint passes.
• nit (scope, already noted in the PR) - the rest of DefaultSettings and the README's DynamoDB ConfigDBClient text are also dead. That cleanup can be its own issue.

Attacked:

• Unread proof: on the merged tree, grepped every file (Go, TS, terraform, yaml/json, md, migrations) for execution_history, executionHistory, EXECUTION_HISTORY, history_days and historyDays. The only hits are the new test and comment. DefaultSettings, GetDefaultValue, GetDefaultSetting, GetDefaultsByCategory and GetAllCategories have no non-test callers; store_postgres.go:145 is a comment only. No migration creates a key/value settings table or inserts retention.* rows. The issue's "seeded into the settings table / editable in UI" premise is false, so no migration is correct.
• Backward compat: the key was never a GlobalConfig field, an openapi property or an env var. Stored global_config rows and API request bodies never carried it, so decoding is unchanged. No API ever accepted this key, so a client that sends it gets the same result as before.
• README claims checked against store_postgres.go:2278-2293 and types.go:538 (HealthScoredExecutionStatuses = failed, canceled, cancelled): completed rows are deleted by scheduled_date, canceled by updated_at, and the expires_at branch excludes the health-scored statuses, so failed rows are never deleted. Correct. DefaultExecutionTTLDays = 30 is used by handler.go:310 and plan_health.go:91. Correct.
• Merge-tree against origin/main 6af022f: clean (tree 42ac7d01). On the merged tree with Go 1.26.10: go build ./... ok, go vet ./internal/config ok, go test ./internal/config ./internal/mocks: 900 passed.
• Mutation probe: re-added the retention.execution_history_days entry. TestDefaultSettings_NoExecutionRetentionKey FAILs. Restored.
• CI: 24 pass, 1 skipping (Snyk). The checks ran on the old base b6355a8 (134 commits behind main). The merged-tree local run covers the gap.
• Commit size: 1 commit, 36 changed lines (under the 400 cap).
• Issue chore(config): retention.execution_history_days is never read at runtime #88 option 2 is fully addressed, including the README.

[IAmJSD]

Local verification at b936fe1932a8b4acfbb7cf9f45e03ce28815544a 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 b936fe19 and the combined-tree verification above. Squash-merged at exactly b936fe1932a8b4acfbb7cf9f45e03ce28815544a.