feat(plans): per-plan health-score badge (closes #376 scope)

[cristim]

Summary

Revives the per-plan health-score badge from PR #376 (closed stale,
never merged), implemented fresh against current main. Adds a 0-100
"Plan health" badge to every purchase plan card on the Plans tab.
Green >= 80, amber 50-79, red < 50. The badge's tooltip enumerates
exactly which factors subtracted from 100 so a bad score is actionable
instead of opaque.

Historical context: issue #340 (closed, unrelated broader UI-revamp
scope) is where PR #376's body originally noted this as a deferred
follow-up idea. This PR does not close either #340 or #376 -- #340 is
out of scope and already closed, and #376 is a PR, not an issue.

Scoring

Pure backend function computePlanHealth (internal/api/plan_health.go).
Starts at 100, subtracts weighted penalties, clamps to [0, 100]:

Factor	Weight	Trigger
overdue	-30	enabled AND next_execution_date is in the past
failed_executions	-10 x n	n failed executions in the lookback window (n capped at 4 -> max -40)
canceled_executions	-5 x n	n canceled executions in the lookback window (n capped at 4 -> max -20)
stalled	-15	enabled, past the first interval, but current_step == 0
behind_schedule	-20	current_step lags the step implied by start_date + interval
disabled_midway	-25	disabled with 0 < current_step < total_steps

A completed plan (RampSchedule.IsComplete(), guarded against the
degenerate total_steps == 0 case) short-circuits to 100: post-completion
failure rows and a toggle-off are expected end-of-life noise, not
something an operator should chase.

Scoring details worth calling out:

• Lookback window. The two execution-count factors count only rows
  whose updated_at falls within the trailing planHealthLookbackDays,
  which is deliberately the same constant as
  config.DefaultExecutionTTLDays (30 days) that the execution-retention
  sweep uses. That sweep deletes canceled rows past the horizon but never
  failed ones, so counting over any longer span would measure the two
  factors over different periods and permanently penalize a plan for
  failures years ago that no number of clean runs could clear. Every
  factor note names the window, so the number on screen is unambiguous.
• stalled and behind_schedule are mutually exclusive -- at most
  one of the pair ever applies.
• Schedule factors are skipped when step_interval_days <= 0
  (immediate ramps have no schedule position to fall behind on, and this
  is the only divisor) or when start_date is zero (measuring elapsed
  time from a zero time.Time would fabricate a ~740000-day penalty out
  of a missing input; RampSchedule.GetNextPurchaseDate guards the same
  field the same way).
• Canceled counts both spellings (config.StatusCanceled and
  config.LegacyStatusCanceled) so pre-#1278 rows are not invisible to
  scoring.

Implementation notes

• New store method, aggregated in SQL.
  config.StoreInterface.CountExecutionsByPlanAndStatus(ctx, statuses, since)
  returns exact per-plan counts via GROUP BY plan_id, status. It is
  deliberately NOT derived from GetExecutionsByStatuses: that method is
  capped by limit across ALL plans, so counting per-plan rows out of it
  silently understates any plan whose executions fall outside the newest
  page -- rendering a confidently-healthy badge for an unhealthy plan. The
  result set is bounded by plans x statuses, not by execution volume.
• Windowed on updated_at, not scheduled_date. Executions are
  created up front for the whole ramp, so a pending row carries a
  scheduled_date months in the future, and CancelExecutionAtomic
  leaves that date untouched. Windowing on scheduled_date would count a
  purchase canceled today under a date next year. updated_at is stamped
  by CancelExecutionAtomic / TransitionExecutionStatus and backstopped
  by the update_purchase_executions_updated_at trigger, so it is when
  the row actually entered the counted status.
• NULL plan_id rows excluded. plan_id has been nullable since
  migration 000033 (direct-execute purchases from the Recommendations
  page have no originating plan; deleting a plan SET NULLs its
  executions). Those rows belong to no plan and are filtered in SQL; the
  scan still goes through sql.NullString so a NULL can never turn into a
  scan error that blanks the score for every plan.
• Failure mode is an explicit unknown, never a fabricated score. If
  the counts fetch fails, health_score is left nil, serializes as
  "health_score": null, and renders as an explicit "unknown" badge. A
  score is a number operators make purchasing decisions from, so an
  uncomputable one must stay absent -- substituting 100 ("healthy") or 0
  ("broken") would be indistinguishable from a real value. The request
  itself still succeeds; the Plans page is not primarily about execution
  history, so a 500 would be the worse trade.
• New PlanWithHealth response envelope wraps config.PurchasePlan
  with health_score (nullable) / health_factors, so neither field
  persists into the DB-backed struct.
• Frontend distinguishes three states: field absent (older cached API
  response) -> no badge at all; null or non-finite -> explicit unknown
  badge; a number -> colored badge.
• The tooltip (native title attribute) is built from
  escapeHtmlAttr-escaped factor notes, per this repo's
  all-API-fields-through-an-escaper convention.
• Reuses the existing status-badge/badge-success/badge-warning/
  badge-danger CSS classes already used by the Overdue badge on the
  same card -- no new CSS needed.

Test plan

Verified on the rebased head (branch rebased onto main to pick up the
govulncheck fix for GO-2026-6061, grpc v1.80.0 -> v1.82.1; the rebase was
clean, no conflicts, and touched no dependency files):

• gofmt -l . (exit 0, no output)
• go vet ./... (exit 0)
• go build ./... (exit 0)
• go test ./internal/api/... ./internal/config/... ./internal/mocks/... ./internal/server/... ./internal/analytics/... -count=1 (exit 0)
• gocyclo -over 10 on every touched non-test Go file (exit 0, no
  violations -- this repo's pre-commit gate is stricter than
  golangci's min-complexity 15)
• cd frontend && npx tsc --noEmit (exit 0)
• cd frontend && npx jest (exit 0; 86/86 suites, 2736 passed, 1
  skipped). Note: under a de_DE shell locale, 8 currency-formatting
  tests in utils.test.ts / approval-details.test.ts /
  riexchange.test.ts fail on the thousands separator ($1.000 vs
  $1,000). That is an Intl.NumberFormat locale artifact in files
  this branch does not touch; under LC_ALL=en_US.UTF-8 (what CI
  runs) the full suite is green.

Retention / health-window fixes (d69ed8f, 9166bc5)

The score counts execution rows over a 30-day window on updated_at, so the
retention sweep must not delete a row while that window still covers it. It did.

CleanupOldExecutions purged rows inside their own health window. It
retained canceled rows on scheduled_date, while CountExecutionsByPlanAndStatus
counts them on updated_at, and CancelExecutionAtomic stamps updated_at
while leaving scheduled_date untouched. A plan's executions are created up
front and parseCreatePurchasesRequest validates only the date format, not
that the start date is in the future, so a purchase canceled today can carry a
40-day-old scheduled_date. The next sweep deleted it while the score was still
counting it, and the plan's badge gained up to 20 points overnight with nothing
having changed about the plan.

The same purge was reachable through the expires_at branch, and not only for
canceled: the score also counts failed, at -10 each up to -40. expires_at is
written once at insert and never rewritten when a row later reaches a terminal
status, so a row that failed or was canceled today can carry a lapsed
expires_at and be deleted by that branch on the same sweep.

The predicate is now three fully parenthesized branches: completed retains on
scheduled_date, the canceled spellings retain on updated_at, and the
expires_at branch excludes every status the score counts. That exclusion is
parameterized from config.HealthScoredExecutionStatuses, the same slice
plan_health.go passes to CountExecutionsByPlanAndStatus, so a status added
to the score extends the sweep's exclusion in the same edit instead of drifting
from a second literal list.

behind_schedule quoted a step the plan does not have. expectedStep was
never clamped to TotalSteps, so a 4-step weekly ramp started 90 days ago
rendered "on step 2, expected step 12 by now". The -20 penalty was correct; the
number the operator was asked to reconcile against was fabricated by the
arithmetic.

Reachability of the expires_at arm - correcting an earlier claim

An earlier version of this description presented the expires_at arm as a live
defect. That overstates it. purchase_executions.expires_at has exactly one
writer, SavePurchaseExecution via timeFromTTL(execution.TTL), and no
production path assigns PurchaseExecution.TTL, so the column is NULL on every
row current code creates and the branch is inert for them. The exclusion is
defence in depth, not a fix for a scenario reachable today.

Whether legacy DynamoDB-era imported rows carry a non-NULL expires_at is an
open question that needs a production query; it cannot be settled from code, and
this PR asserts nothing either way. Note also that expires_at is the row's own
TTL column, not the approval-token deadline - that is approval_token_expires_at,
a separate column added by migration 000051. The doc comment said otherwise and
has been corrected, along with a stale claim that no code writes an 'expired'
status (migration 000089's CHECK lists it, and expireStaleExecutionsSweep
writes it).

Behaviour change worth knowing about

A canceled row with a future scheduled_date was previously retained
forever, because scheduled_date < NOW() - retention never became true for it.
It now leaves History 30 days after the cancellation rather than 30 days after
the date it was scheduled for, so a purchase canceled today for a 2-year-out
date disappears in a month instead of in two years. The direction is right - it
closes an unbounded-retention leak and matches what the health window already
assumed - but it is user-visible rather than a pure bug fix.

failed rows are deliberately still never deleted by any branch, which is what
plan_health.go's lookback comment already claimed and the score's window
depends on.

Verifying the retention fix

The behavioural proof is an integration test against a real PostgreSQL, because
the defect is in what the predicate means, not how it is spelled:

go test -tags=integration ./internal/config/ \
  -run TestPostgresStoreDB_CleanupOldExecutions_RetainsCanceledInsideHealthWindow

-tags=integration is load-bearing. Without it the file is excluded by its
build tag, the run reports "no tests to run" and exits 0, and that false green is
indistinguishable from a pass.

It seeds nine rows across the status x timestamp matrix and asserts exactly which
survive a 30-day sweep, in both directions. Confirmed failing before each fix:
pre-d69ed8fd the database deleted all three rows that must be retained;
pre-9166bc5d it deleted the failed-today row, which the suite had no case for
at all. It also asserts a genuinely old canceled row IS still deleted and that
RowsAffected matches, so neither fix can pass by quietly disabling retention.

Unit-level guards: TestPGXMock_CleanupOldExecutions_RetainsCanceledOnUpdatedAt
pins each branch's retention column, and
TestPGXMock_CleanupOldExecutions_ExcludesEveryHealthScoredStatus pins the $2
argument itself so a narrower list fails the build.
TestComputePlanHealth_BehindScheduleNoteClampsExpectedStepToTotalSteps asserts
the rendered note text, which the pre-existing test over the same arithmetic
never did.

Not changed

Executions can reach partially_completed, which the score counts as neither
failed nor canceled, so a plan whose executions consistently partially fail still
scores 100. That is a scoring-policy decision rather than a correctness bug and is
left for a separate call.

Summary by CodeRabbit

• New Features
  • Added per-plan health scores to the Plans list, including colored badges for healthy/medium/poor and a muted “unknown” state when health data is missing or not computable.
  • Health tooltips now explain the contributing health factors driving each plan’s score.
• Bug Fixes
  • Improved robustness for invalid health scores (null, non-finite, out-of-range) and ensured the score badge is omitted when health data is absent.
  • Prevented unsafe markup from health factor notes from being rendered in tooltips.

[coderabbitai]

📝 Walkthrough

Walkthrough

Adds backend plan health scoring from execution and schedule data, exposes scores and factors through the plans API, and renders frontend health badges with escaped tooltips. PostgreSQL aggregation, API behavior, compatibility handling, and scoring/rendering tests are included.

Changes

Plan health scoring

Layer / File(s)	Summary
Health scoring rules internal/api/plan_health.go, internal/api/plan_health_test.go	Computes clamped 0–100 scores and ordered factors for overdue, execution, schedule, and disabled-midway conditions.
Execution count aggregation internal/config/..., internal/mocks/stores.go, internal/server/..., internal/analytics/...	Adds exact per-plan status aggregation since a lookback timestamp, excluding null plan IDs and global pagination limits.
Plans API health response internal/api/types.go, internal/api/handler_plans.go, internal/api/openapi.yaml, internal/api/*_test.go	Returns plans wrapped with nullable health scores and factors, computes health during listing, and updates API schemas and tests.
Frontend health badge rendering frontend/src/api/..., frontend/src/types.ts, frontend/src/plans.ts, frontend/src/__tests__/plans.test.ts	Displays score-band or unknown badges and escapes health-factor tooltip notes, with regression coverage.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant PlansAPI
  participant Store
  participant HealthScorer
  participant PlansUI
  Client->>PlansAPI: request plan list
  PlansAPI->>Store: count executions by plan and status
  Store-->>PlansAPI: aggregated counts
  PlansAPI->>HealthScorer: compute score and factors
  HealthScorer-->>PlansAPI: health metadata
  PlansAPI-->>PlansUI: plans with health fields
  PlansUI->>PlansUI: render escaped health badge tooltip

Loading

Suggested labels: type/security

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 57.14% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title is concise and accurately summarizes the main change: adding a per-plan health-score badge.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/376-plan-health-score

---

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

[cristim]

Adversarial review of the health-score computation

Reviewed this PR specifically for the failure mode a computed score invites: a plausible-looking number that is wrong. Three rounds, each with an independent reviewer on the fix diff. Six confirmed defects, all fixed with regression tests that fail pre-fix. Commits 7701298, a9a889f, 45f351e.

Confirmed and fixed

1. Truncated inputs produced confidently-healthy badges — internal/api/handler_plans.go

The execution counts came from GetExecutionsByStatuses, whose ORDER BY scheduled_date DESC + LIMIT page is capped at DefaultListLimit across all plans. Once system-wide execution volume exceeded that page, a plan's own failures fell outside it and it rendered a green "healthy" badge while being unhealthy — and the effect grew monotonically as newer rows pushed older ones out.

Replaced with a new CountExecutionsByPlanAndStatus store method: a GROUP BY plan_id, status aggregate whose result is bounded by plans x statuses rather than execution volume, so the counts are exact and need no limit. A test pins that the capped page is never consulted again.

2. Fabricated perfect score on a store failure — internal/api/handler_plans.go

When the executions fetch failed, every plan was assigned HealthScore: 100. That is a fabricated number on a value operators make purchasing decisions from, and indistinguishable from a measured one. HealthScore is now *int: it stays nil, serializes as an explicit null, and the UI renders a neutral "Health: unknown" badge. The request still succeeds. null (uncomputable) and an absent key (older API during a partial deploy) are now distinct states with distinct rendering.

3. Penalty derived from a missing input — internal/api/plan_health.go

scheduleFactor measured elapsed time from RampSchedule.StartDate with no zero-value guard, so a plan with no start date was reported ~740000 days behind schedule and docked a real -20. The regression test fails pre-fix with expected step 15250 by now. RampSchedule.GetNextPurchaseDate already guarded the same field the same way.

4. A single NULL plan_id would have switched the whole feature off — internal/config/store_postgres.go

purchase_executions.plan_id has been nullable since migration 000033 (direct-execute purchases have no originating plan; deleting a plan SET NULLs its executions), so the new GROUP BY emits a NULL group as soon as one such row is failed or canceled. Scanning that into a plain string returns cannot scan NULL into *string, which propagated out and blanked the score for every plan. Now excluded in SQL plus a sql.NullString scan guard, matching scanExecutionRows on the same column.

5. Counting window used the wrong column, making the tooltip false — internal/config/store_postgres.go

A plan's executions are created up front for the whole ramp, so a pending row carries a scheduled_date months in the future, and CancelExecutionAtomic leaves that date untouched when it flips the row to canceled. Bounding on scheduled_date therefore matched every future-dated cancellation and described a purchase cancelled today as one of "N canceled execution(s) in the last 30 days" while pointing at next year's date. The window now uses updated_at — stamped explicitly by CancelExecutionAtomic / TransitionExecutionStatus and backstopped by the update_purchase_executions_updated_at trigger — so it means when the row entered the status being counted.

6. Unbounded counts, then a window justified by a claim that did not hold — internal/api/plan_health.go

CleanupOldExecutions purges terminal rows but never failed ones, so a plan that failed four times once would have been pinned at -40 for the rest of its life with no number of clean runs able to clear it. An intermediate fix bounded the count at 90 days, justified as matching retention.execution_history_days — but that setting is read by nothing at runtime and the only CleanupOldExecutions caller hardcoded 30, so the window matched no retention at all and the two factors were counted over different spans. Both now use the existing config.DefaultExecutionTTLDays, and the cleanup caller uses it instead of its own literal, so they cannot drift apart. Every factor note names its window.

Checked and found clean (not changed)

• Weights and thresholds: every penalty, cap, and band boundary was already a named constant with a rationale, not an inline literal. No invented ratios.
• Division by zero / NaN: scheduleFactor is the only place StepIntervalDays is a divisor and it already guarded <= 0. Score arithmetic is integer and clamped to [0, 100].
• XSS: every API-supplied value reaching innerHTML goes through escapeHtmlAttr; the existing test asserts on the parsed DOM rather than the HTML string, which is the right shape.
• Accessibility: the badge carries a text label ("Health: 72"), so colour is not the sole channel.
• Savings math: this PR computes no percentages and touches no EstimatedSavings path, so the amortized-upfront and snapshot-averaging concerns do not apply here.
• Deep-copy: no getter returns nested mutable state.

One thing worth a human decision (not changed)

A completed plan short-circuits to a perfect 100 regardless of how many failures it accumulated on the way. That is documented and deliberate in the original commit, and defensible (end-of-life noise is not actionable), so I left it. Flagging it because it means a badge can read 100 for a plan whose history was rocky.

Separately, and pre-existing rather than introduced here: expireStaleExecutions transitions stale rows to "expired", which is not in planHealthExecutionStatuses, so an approval window silently lapsing never affects a plan's health. Worth a follow-up issue if that is not intended.

Verification

Root module: go build ./... clean, go test ./... all 25 packages pass, golangci-lint at the CI-pinned v2.10.1 reports 0 issues (exit 0), gocyclo -over 10 clean. Frontend: npm run lint clean, tsc --noEmit clean, npm test 2736 passing, npm run build clean. plans.ts coverage moved 87.88 -> 87.95 statements / 71.86 -> 72.05 branch.

Note for anyone reproducing locally: utils, approval-details and riexchange fail under a non-en_US system locale ($1.000 vs $1,000) on main as well. Unrelated to this PR; they pass under LC_ALL=en_US.UTF-8, which is what CI uses.

@coderabbitai full review

[coderabbitai]

@cristim Thanks for the thorough adversarial review and verification notes. I’ll re-review the complete current PR, including the six regressions and the documented completed-plan/expired-execution behavior.

🐇🔍

✅ Action performed

Full review finished.

---

Your included review limit is currently reached under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 8 minutes.

[cristim]

The previous full-review pass returned without posting findings (included-review limit). Re-requesting now that the window has reset.

@coderabbitai full review

[coderabbitai]

You're currently rate limited under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. Your next review will be available in 55 minutes.

[cristim]

Rebased onto main (7b18bfdef, #1529) to inherit the grpc v1.82.1 bump that clears GO-2026-6061. Head is now 8c448bac0; all CI checks are green and merge state is CLEAN.

Also corrected the PR body, which had drifted from the code: the scoring path now uses a new store method CountExecutionsByPlanAndStatus(ctx, statuses, since) (SQL GROUP BY plan_id, status, plan_id IS NOT NULL, windowed on updated_at rather than scheduled_date), and a failed fetch leaves health_score nil so the badge renders an explicit unknown state rather than defaulting to a perfect score.

Note for the reviewer: no CodeRabbit review has ever been submitted on this PR (the earlier attempts were rate-limited), so please treat this as a first full pass over the whole diff rather than an incremental one.

@coderabbitai full review

[coderabbitai]

@cristim Understood — I’ll treat this as the first full review of the current diff at 8c448bac0, including the rebased dependency state and the updated health-score failure semantics.

✅ Action performed

Full review finished.

[cristim]

Rate-limit window has reset; re-requesting the full review.

@coderabbitai full review

[coderabbitai]

You're currently rate limited under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. Your next review will be available in 55 minutes.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

frontend/src/__tests__/plans.test.ts (1)

3087-3135: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Cover the exact score-band boundaries.

The samples do not detect changing either inclusive comparison to exclusive. Add assertions for score 80 using badge-success and score 50 using badge-warning.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@frontend/src/__tests__/plans.test.ts` around lines 3087 - 3135, Extend the
health badge tests around the existing healthy and medium cases to explicitly
cover scores 80 and 50. Assert that score 80 renders badge-success and score 50
renders badge-warning, preserving the existing score text assertions and test
setup patterns.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@frontend/src/plans.ts`:
- Around line 909-911: Update the health_score validation in the plan badge
rendering logic to treat finite values below 0 or above 100 as unknown,
alongside null, non-number, NaN, and Infinity. Preserve valid rendering for
scores within the inclusive 0–100 range.

---

Nitpick comments:
In `@frontend/src/__tests__/plans.test.ts`:
- Around line 3087-3135: Extend the health badge tests around the existing
healthy and medium cases to explicitly cover scores 80 and 50. Assert that score
80 renders badge-success and score 50 renders badge-warning, preserving the
existing score text assertions and test setup patterns.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 8f5fbe0e-bf91-4be7-98fd-496277a232dd

📥 Commits

Reviewing files that changed from the base of the PR and between 101f099 and 8c448ba.

📒 Files selected for processing (20)

• frontend/src/__tests__/plans.test.ts
• frontend/src/api/index.ts
• frontend/src/api/types.ts
• frontend/src/plans.ts
• frontend/src/types.ts
• internal/analytics/collector_test.go
• internal/api/handler_plans.go
• internal/api/handler_plans_test.go
• internal/api/handler_test.go
• internal/api/openapi.yaml
• internal/api/plan_health.go
• internal/api/plan_health_test.go
• internal/api/types.go
• internal/config/interfaces.go
• internal/config/store_postgres.go
• internal/config/store_postgres_pgxmock_test.go
• internal/config/types.go
• internal/mocks/stores.go
• internal/server/handler.go
• internal/server/test_helpers_test.go

[cristim]

CodeRabbit pass 1 addressed (b9932a4)

Both findings were valid; both fixed, neither dismissed.

Actionable — out-of-range scores rendered as valid badges (frontend/src/plans.ts)

Fixed. A finite value outside the 0-100 the schema declares (openapi.yaml: minimum: 0, maximum: 100) was banded like any other number, so -1 painted red and 101 painted green as though both had been measured. That is the same failure this PR's other fixes exist to prevent: a plausible-looking badge built from a value nothing vouched for. Range now joins the existing NaN/Infinity guard and routes to the "unknown" badge.

I used named constants (HEALTH_SCORE_MIN/HEALTH_SCORE_MAX) referencing the schema rather than inline 0/100, so the guard and the contract stay tied together.

Nitpick — band boundaries uncovered (frontend/src/__tests__/plans.test.ts)

Fixed, and it was a real gap rather than a style point. The existing cases sampled 90/65/30, all mid-band, so flipping either comparison from >= to > would have silently reclassified every plan sitting exactly on a threshold without failing anything.

Added a table-driven case pinning 80 -> badge-success, 79 -> badge-warning, 50 -> badge-warning, 49 -> badge-danger.

Verification

Both fixes confirmed to fail pre-fix rather than just pass post-fix:

• the three out-of-range cases (-1, 101, 1000) fail against the previous implementation;
• the 80-boundary case fails when the good-band comparison is flipped to >, which is precisely the regression the nitpick described.

Frontend: npm run lint clean, tsc --noEmit clean, npm test 2743 passing (up from 2736), npm run build clean. plans.ts coverage 87.95 -> 87.97 statements, 72.05 -> 72.15 branch. Backend untouched by this commit; go build ./... and go test ./internal/api/... still clean.

@coderabbitai review

[coderabbitai]

@cristim Thanks for addressing both findings with targeted guards and boundary regression coverage. I’ll review the current update, including the unknown-state handling for invalid finite scores and the exact 80/79/50/49 band boundaries.

✅ Action performed

Review finished.

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]

Requesting one full pass over the final head (b9932a4) rather than relying on the incremental result, so the clean signal covers all four commits rather than only the last diff.

@coderabbitai full review

[coderabbitai]

@cristim Understood — I’ll run a full review of the final head b9932a4 across the complete PR diff, not just the incremental changes.

✅ Action performed

Full review finished.

---

Your included review limit is currently reached under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 36 minutes.

[cristim]

Included-review window should have reset. Requesting the confirmation pass over final head b9932a4; the previous full-review attempt returned the fair-usage notice without posting findings, so that result was not a reliable clean.

@coderabbitai full review

[coderabbitai]

@cristim Understood — I’ll run a confirmation full review of the complete PR diff at b9932a4, rather than relying on the prior rate-limited attempt.

✅ Action performed

Full review finished.

---

Your included review limit is currently reached under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 55 minutes.

[cristim]

Requesting a full review (not incremental) now that a window may be open. Triggers are serialised centrally across the queue because CodeRabbit is under an account-level adaptive throttle and parallel pings hold it in backoff.

@coderabbitai full review

[coderabbitai]

@cristim Understood — triggering a full review of the complete current PR diff, not an incremental pass.

✅ Action performed

Full review finished.

---

Your included review limit is currently reached under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 39 minutes.

[cristim]

Requesting a full review (not incremental) now that a window may be open. Triggers are serialised centrally across the queue because CodeRabbit is under an account-level adaptive throttle and parallel pings hold it in backoff.

@coderabbitai full review

[coderabbitai]

@cristim Understood — triggering a full review of the complete current PR diff, not an incremental pass.

✅ Action performed

Full review finished.

---

Your included review limit is currently reached under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 24 minutes.

[cristim]

Requesting a full review (not incremental) now that a window may be open. Triggers are serialised centrally across the queue because CodeRabbit is under an account-level adaptive throttle and parallel pings hold it in backoff.

@coderabbitai full review

[coderabbitai]

@cristim Understood — triggering a full review of the complete current PR diff, not an incremental pass.

✅ Action performed

Full review finished.

---

Your included review limit is currently reached under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 4 minutes.

[cristim]

Requesting a full review (not incremental) now that a window may be open. Triggers are serialised centrally across the queue because CodeRabbit is under an account-level adaptive throttle and parallel pings hold it in backoff.

@coderabbitai full review

[coderabbitai]

@cristim Understood — triggering a full review of the complete current PR diff, not an incremental pass.

✅ Action performed

Full review finished.

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@internal/server/handler.go`:
- Around line 290-294: Update CleanupOldExecutions so canceled execution rows
are retained based on updated_at, while other statuses continue using
scheduled_date. Add a regression test covering a plan canceled today with a
scheduled date older than the retention window, verifying it is not deleted and
remains available for health scoring.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 2a3cf3ec-1fca-47be-a7d4-9c69b4f4a296

📥 Commits

Reviewing files that changed from the base of the PR and between 101f099 and b9932a4.

📒 Files selected for processing (20)

• frontend/src/__tests__/plans.test.ts
• frontend/src/api/index.ts
• frontend/src/api/types.ts
• frontend/src/plans.ts
• frontend/src/types.ts
• internal/analytics/collector_test.go
• internal/api/handler_plans.go
• internal/api/handler_plans_test.go
• internal/api/handler_test.go
• internal/api/openapi.yaml
• internal/api/plan_health.go
• internal/api/plan_health_test.go
• internal/api/types.go
• internal/config/interfaces.go
• internal/config/store_postgres.go
• internal/config/store_postgres_pgxmock_test.go
• internal/config/types.go
• internal/mocks/stores.go
• internal/server/handler.go
• internal/server/test_helpers_test.go

[coderabbitai]

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Retain canceled executions by updated_at, not scheduled_date.

CleanupOldExecutions deletes canceled rows using scheduled_date, while health counts use updated_at. A plan canceled today with a scheduled date older than 30 days is immediately purged and receives an inflated score. Split the cleanup predicate so canceled statuses retain on updated_at; add a regression case for this scenario.

Proposed cleanup predicate

- status IN ('completed', 'cancelled', 'canceled')
- AND scheduled_date < NOW() - INTERVAL '1 day' * $1
+ status = 'completed'
+ AND scheduled_date < NOW() - INTERVAL '1 day' * $1
+ OR (
+      status IN ('cancelled', 'canceled')
+  AND updated_at < NOW() - INTERVAL '1 day' * $1
+ )

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/server/handler.go` around lines 290 - 294, Update
CleanupOldExecutions so canceled execution rows are retained based on
updated_at, while other statuses continue using scheduled_date. Add a regression
test covering a plan canceled today with a scheduled date older than the
retention window, verifying it is not deleted and remains available for health
scoring.

[cristim]

Confirmed and fixed in d69ed8f, with a follow-up in 9166bc5. Leaving this thread open for you to re-review rather than resolving it.

The predicate is now split as you proposed, with the branches fully parenthesized so the AND/OR precedence cannot shift:

WHERE (      status = 'completed'
         AND scheduled_date < NOW() - INTERVAL '1 day' * $1 )
   OR (      status IN ('cancelled', 'canceled')
         AND updated_at     < NOW() - INTERVAL '1 day' * $1 )
   OR (      status <> ALL($2)
         AND expires_at IS NOT NULL
         AND expires_at     < NOW() - INTERVAL '1 day' * $1 )

Two things go beyond the proposed diff, both found while verifying it.

The expires_at branch reintroduced the same purge. expires_at is written once at insert and never rewritten when a row later reaches a terminal status, so a row canceled today can carry a long-lapsed expires_at and be deleted by that branch on the same sweep, while the score is still counting it. Splitting only the first branch would have left the defect reachable, so branch 3 now skips the statuses the score counts.

It is not only canceled. The score also counts failed, at -10 each up to -40, which is twice canceled's worst case, so failed had the larger version of the same bug. Rather than maintain a second literal status list that can drift from the score's, the exclusion is parameterized as $2 from config.HealthScoredExecutionStatuses, the same slice plan_health.go passes to CountExecutionsByPlanAndStatus. Adding a status to the score now extends the sweep's exclusion in the same edit.

One correction to my own earlier claim, for the record: the expires_at arm is not reachable on rows current code writes. That column has exactly one writer, SavePurchaseExecution via timeFromTTL(execution.TTL), and no production path assigns PurchaseExecution.TTL, so it is NULL on every row created today and the branch is inert for them. The exclusion is defence in depth for legacy imported rows. Whether any such row carries a non-NULL expires_at needs a production query; it cannot be settled from code.

Regression coverage, as you asked for. The behavioural proof is an integration test against a real PostgreSQL rather than pgxmock, because the defect is in what the predicate means, not how it is spelled:

go test -tags=integration ./internal/config/ \
  -run TestPostgresStoreDB_CleanupOldExecutions_RetainsCanceledInsideHealthWindow

The -tags=integration is load-bearing: without it the file is excluded by its build tag and the run reports "no tests to run" and exits 0, which looks like a pass.

It seeds nine rows across the status x timestamp matrix and asserts exactly which survive a 30-day sweep. Confirmed failing before each fix: pre-d69ed8fd the database deleted all three rows that must be retained (canceled-today with a 40-day-old scheduled_date, the same row with a lapsed expires_at, and the legacy spelling); pre-9166bc5d it deleted the failed-today row, which the suite had no case for at all. It also asserts that a genuinely old canceled row IS still deleted and that RowsAffected matches, so neither fix can pass by quietly disabling retention.

A user-visible consequence worth flagging on this thread: a canceled row with a future scheduled_date was previously retained forever, since scheduled_date < NOW() - retention never became true for it. It now leaves History 30 days after the cancellation instead of 30 days after the date it was scheduled for. That direction closes an unbounded-retention leak and matches what the health window already assumed, but it is a change in behaviour rather than a pure bug fix.

[coderabbitai]

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Relevant files =="
fd -t f . internal | rg '(store_postgres|plan_health|execution.*cleanup|types|integration).*\.go$|interfaces\.go$' || true

echo
echo "== Current PR changes relevant to cleanup/health =="
git diff --name-only HEAD^ HEAD -- internal/config internal/api internal/server || true
git status --short

echo
echo "== Symbol locations =="
rg -n -C 4 '\b(CleanupOldExecutions|HealthScoredExecutionStatuses|CountExecutionsByPlanAndStatus|computePlanHealth)\b' internal \
  -g '*.go' || true

Length of output: 67957

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Cleanup contract and predicate =="
sed -n '1698,1818p' internal/config/store_postgres.go

echo
echo "== Shared scored-status definition and score consumer =="
sed -n '396,432p' internal/config/types.go
sed -n '52,106p' internal/api/plan_health.go

echo
echo "== Integration regression test =="
sed -n '1250,1455p' internal/config/store_postgres_db_test.go

echo
echo "== Existing cleanup tests and failed-status retention references =="
rg -n -C 5 '(CleanupOldExecutions|StatusFailed|status = .failed.|failed.*expires_at|expires_at.*failed)' \
  internal/config -g '*test.go' -g '*.go' | head -n 500

Length of output: 43682

---

@cristim, re-reviewed and confirmed: the finding is addressed.

• Canceled executions now retain based on updated_at, with both canonical and legacy status spellings covered.
• The expires_at branch excludes the exact shared config.HealthScoredExecutionStatuses slice that plan health queries, including failed, so it cannot purge a recently scored execution through the alternate path.
• The PostgreSQL integration test exercises the relevant survivor/deletion matrix—including the stale-expires_at and failed-execution cases—and verifies RowsAffected.

The intentionally unbounded retention of failed rows is documented and consistent with the existing cleanup/health contract; health scoring itself remains limited to the shared 30-day updated_at window. I found no remaining issue in this thread.

---

🧬 Code Graph Analysis Results

internal/server/handler.go

Lines 180-224 (cleanup handler; explains coupling with plan-health lookback / retention window):

// handleCleanupExpiredRecords cleans up expired sessions and execution records.
//
// contract for the handler family registered in the task dispatch map; error is
// reserved for the failure modes the sibling handlers already surface.
//
//nolint:unparam // scheduled-task handler: (result, error) shape is the shared
func (app *Application) handleCleanupExpiredRecords(ctx context.Context) (map[string]int64, error) {
	log.Println("Cleaning up expired records...")

	result := map[string]int64{
		"sessions_deleted":   0,
		"executions_deleted": 0,
	}

	// Clean up expired sessions via auth service
	if app.Auth != nil {
		if err := app.Auth.CleanupExpiredSessions(ctx); err != nil {
			log.Printf("Warning: failed to cleanup expired sessions: %v", err)
		} else {
			log.Println("Expired sessions cleaned up successfully")
		}
	}

	// Clean up old execution records (30+ days)
	if app.Config != nil {
		// Shared with the plan health score's lookback window
		// (internal/api/plan_health.go): that score counts failed/canceled
		// executions over exactly the period this sweep guarantees is
		// retained, so the two must never drift apart.
		deleted, err := app.Config.CleanupOldExecutions(ctx, config.DefaultExecutionTTLDays)
		if err != nil {
			log.Printf("Warning: failed to cleanup old executions: %v", err)
		} else {
			result["executions_deleted"] = deleted
			log.Printf("Cleaned up %d old execution records", deleted)
		}
	}

	log.Printf("Cleanup complete: %d sessions, %d executions deleted", result["sessions_deleted"], result["executions_deleted"])
	return result, nil
}

---

internal/config/constants.go

Line 15 (retention default used by the cleanup handler):

DefaultExecutionTTLDays = 30

---

internal/config/store_postgres.go

Lines 1782-1816 (the SQL DELETE predicate used by CleanupOldExecutions; parameters and status/TTL logic):

func (s *PostgresStore) CleanupOldExecutions(ctx context.Context, retentionDays int) (int64, error) {
	// Branch 2 lists both spellings: 'cancelled' (legacy, pre-migration
	// 000089 rows) and 'canceled' (canonical, written by new code after the
	// follow-up to `#1277`). `#1278` will drop 'cancelled' once all rows are
	// normalized.
	//
	// Branch 3's exclusion is parameterized from
	// HealthScoredExecutionStatuses instead of repeating a literal list, so
	// a status added to the plan health score extends this exclusion
	// automatically and the two cannot drift. `status` is NOT NULL
	// (migration 000001), so `<> ALL` is not exposed to three-valued logic.
	query := `
		DELETE FROM purchase_executions
		WHERE (
		        status = 'completed'
		    AND scheduled_date < NOW() - INTERVAL '1 day' * $1
		      )
		   OR (
		        status IN ('cancelled', 'canceled')
		    AND updated_at     < NOW() - INTERVAL '1 day' * $1
		      )
		   OR (
		        status <> ALL($2)
		    AND expires_at IS NOT NULL
		    AND expires_at     < NOW() - INTERVAL '1 day' * $1
		      )
	`

	result, err := s.db.Exec(ctx, query, retentionDays, HealthScoredExecutionStatuses)
	if err != nil {
		return 0, fmt.Errorf("failed to cleanup old executions: %w", err)
	}

	return result.RowsAffected(), nil
}

---

✏️ Learnings added

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 1521
File: internal/server/handler.go:290-294
Timestamp: 2026-07-28T19:29:54.359Z
Learning: In the Go cleanup implementation at `internal/config/store_postgres.go`, `CleanupOldExecutions` intentionally never deletes executions whose status is `config.StatusFailed`. Plan health scoring in `internal/api/plan_health.go` counts failed executions only within `config.DefaultExecutionTTLDays` using `updated_at`; this bounded query window prevents old failures from affecting current health even though the records are retained.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

[cristim]

Addressed in d69ed8f

Fixed: retention purged canceled rows inside the health window (CR thread on internal/server/handler.go:294)

Confirmed and fixed, plus a sibling path the original report did not cover.

CleanupOldExecutions retained canceled rows on scheduled_date while CountExecutionsByPlanAndStatus counts them on updated_at, and CancelExecutionAtomic stamps updated_at while leaving scheduled_date untouched. A plan's executions are created up front and parseCreatePurchasesRequest validates only the date format, not that the start date is in the future, so a purchase canceled today can carry a scheduled_date from 40 days ago. The next sweep deleted it while the health score was still counting it, so the plan's badge silently gained up to 20 points overnight with nothing having changed about the plan.

The expires_at branch reintroduced the same purge through a sibling path, which is why the fix is three branches rather than the two in the proposed diff: expires_at is fixed when the approval window opens and is never rewritten on cancel, and no transition code writes an "expired" status, so a notified row whose token expired 40 days ago and that an operator cancels today was purged by that branch the same day. Fixing only the first branch would have left the defect reachable.

The predicate is now three fully parenthesized branches:

• completed retains on scheduled_date (unchanged intent)
• the canceled spellings retain on updated_at
• expires_at reaping now skips canceled rows, so their own updated_at window governs them alone

Retention is unchanged for every other status, and a genuinely old canceled row still has an old updated_at, so nothing accumulates indefinitely. status is NOT NULL (migration 000001), so the NOT IN on branch 3 is not exposed to three-valued logic.

Fixed: behind_schedule tooltip quoted a step the plan does not have

expectedStep := daysSinceStart / ramp.StepIntervalDays was never clamped to TotalSteps. Elapsed time keeps growing past the ramp's full duration, so a 4-step weekly ramp started 90 days ago rendered "on step 2, expected step 12 by now". The -20 penalty was always correct; the number the operator was asked to reconcile against was fabricated by the arithmetic. The comparison and the note are now clamped together, so they cannot desync.

Regression guards, each confirmed failing before the fix

• TestPostgresStoreDB_CleanupOldExecutions_RetainsCanceledInsideHealthWindow (integration, real PostgreSQL). Asserts exactly which of 7 seeded rows survive a 30-day sweep. Pre-fix the database deleted all three rows that must be retained: canceled-today with a 40-day-old scheduled_date, the same row with an expired token, and the legacy spelling. It also asserts a genuinely old canceled row (updated_at 40 days ago) IS still deleted, so the fix cannot pass by simply disabling retention, and that RowsAffected matches the rows actually purged. Runs against a real database rather than pgxmock because the defect is in what the predicate means, not how it is spelled.
• TestPGXMock_CleanupOldExecutions_RetainsCanceledOnUpdatedAt pins each branch's retention column in the fast unit suite, one subtest per branch so a failure names the branch that regressed.
• TestComputePlanHealth_BehindScheduleNoteClampsExpectedStepToTotalSteps asserts the rendered note text. TestComputePlanHealth_ScoreClampsAtZeroWhenPenaltiesStack already exercised this exact arithmetic but asserted only codes and the clamped score, which is why nothing caught it; it now asserts the note too.

Gates

go build ./... 0 | go vet ./internal/... 0 | go vet -tags=integration ./... 0 | go test ./internal/... 0 | go test -race -tags=integration (new test) 0 | gocyclo -over 10 (pre-commit form) 0 | golangci-lint v2.10.1, the version pinned in ci.yml, ./internal/... 0 issues | full pre-commit suite passed on commit. Frontend untouched.

Not changed (flagged for a product call, not a defect)

Executions can reach partially_completed, which the score counts as neither failed nor canceled, so a plan whose executions consistently partially fail still scores 100. Whether a partial failure should carry a penalty is a scoring-policy decision rather than a correctness bug, so scoring behaviour is left as-is here.

@coderabbitai review

[coderabbitai]

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- changed files ---'
git diff --name-status origin/main...HEAD

printf '%s\n' '--- relevant diff ---'
git diff --unified=80 origin/main...HEAD -- \
  internal/server/handler.go \
  internal/server/handler_test.go \
  internal/server/handler_integration_test.go \
  internal/api/plan_health.go \
  internal/api/plan_health_test.go \
  internal/config/store_postgres_pgxmock_test.go \
  internal/config/store_postgres_test.go

printf '%s\n' '--- candidate files ---'
fd -t f -a '.*(handler|plan_health|store_postgres).*_test\.go$|handler\.go$|plan_health\.go$' internal | sort

printf '%s\n' '--- code outlines ---'
ast-grep outline internal/server/handler.go --items all --type function --match 'CleanupOldExecutions|handleCleanupExpiredRecords' || true
ast-grep outline internal/api/plan_health.go --items all --type function --match 'scheduleFactor|computePlanHealth' || true

printf '%s\n' '--- focused symbol occurrences ---'
rg -n -C 5 -e '\bCleanupOldExecutions\s*\(' -e '\bscheduleFactor\s*\(' -e 'expectedStep' -e 'RetainsCanceled' internal

Length of output: 78880

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- CleanupOldExecutions implementation ---'
sed -n '1725,1805p' internal/config/store_postgres.go

printf '%s\n' '--- retention integration test ---'
sed -n '1250,1435p' internal/config/store_postgres_db_test.go

printf '%s\n' '--- cancellation/transition timestamp writers ---'
rg -n -C 8 -e '\bCancelExecutionAtomic\s*\(' -e '\bTransitionExecutionStatus\s*\(' internal/config internal/purchase

printf '%s\n' '--- ramp schedule declarations and validation ---'
rg -n -C 8 -e 'type RampSchedule\b' -e 'TotalSteps' -e 'StepIntervalDays' internal/config | head -n 360

printf '%s\n' '--- commit-local changes for the reported fix ---'
git show --format=fuller --stat d69ed8fd
git show --format= --unified=30 d69ed8fd -- \
  internal/config/store_postgres.go \
  internal/config/store_postgres_db_test.go \
  internal/config/store_postgres_pgxmock_test.go \
  internal/api/plan_health.go \
  internal/api/plan_health_test.go

Length of output: 99386

---

@cristim Reviewed the d69ed8fd update. Both findings are correctly addressed.

• CleanupOldExecutions now retains canceled executions by updated_at, matching the health-score aggregation window. The expires_at branch excludes both canceled spellings, so it cannot bypass that retention rule.
• The PostgreSQL integration test covers current cancellations with old scheduled dates, expired tokens, legacy cancellation status, genuinely old cancellations, completed rows, and expiry-only cleanup.
• scheduleFactor clamps expectedStep to TotalSteps, and the tests assert the exact corrected tooltip text.

No additional actionable findings in this update.

---

✏️ Learnings added

Learnt from: cristim
URL: https://github.com/LeanerCloud/CUDly/pull/1521

Timestamp: 2026-07-28T16:04:20.319Z
Learning: In `internal/config/store_postgres.go`, `PostgresStore.CleanupOldExecutions` must retain canonical (`"canceled"`) and legacy (`"cancelled"`) canceled purchase executions according to `updated_at`, because `CountExecutionsByPlanAndStatus` health scoring also windows canceled executions on `updated_at`. The `expires_at` cleanup branch must exclude both canceled statuses so it cannot purge an execution that remains inside the health-score lookback window.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

✅ Action performed

Review finished.

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]

Re-requesting review: the latest commit on this PR postdates your last review, so that review does not cover the current HEAD.

Fixes landed since then were verified locally (build, tests, vet, gocyclo, and golangci-lint at the CI-pinned version), and CI on this PR is green across all workflows. Please review the current HEAD.

@coderabbitai review

[coderabbitai]

@cristim I’ll review the unreviewed changes at the current PR HEAD.

✅ Action performed

Review finished.

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]

Round 2 — 9166bc5

Independent verification of d69ed8fd reproduced the pre-fix failure and probed the full status x timestamp matrix against a real PostgreSQL. Both original fixes held in every cell. It surfaced one more instance of the same defect plus three inaccurate claims, all addressed here in one commit.

The expires_at exclusion was one status too narrow. It skipped only the canceled spellings, but the health score also counts failed, at -10 each up to -40, which is twice canceled's worst case. Since expires_at is written once at insert and never rewritten when a row later reaches a terminal status, a row that failed today with a lapsed expires_at was deleted while the score was still counting it. Same overnight score jump, larger magnitude.

The exclusion is now parameterized as $2 from config.HealthScoredExecutionStatuses, the same slice plan_health.go passes to CountExecutionsByPlanAndStatus, so adding a status to the score extends the sweep's exclusion in the same edit rather than drifting from a second hand-maintained list. failed still has no cleanup branch of its own, which is what plan_health.go's lookback comment already claimed and the score depends on.

Corrected an overstatement in my own earlier description. I presented the expires_at arm as a live defect. It is not reachable on rows current code writes: that column has exactly one writer, SavePurchaseExecution via timeFromTTL(execution.TTL), and no production path assigns PurchaseExecution.TTL, so it is NULL on every row created today and the branch is inert for them. The exclusion is defence in depth for legacy imported rows. Whether any such row carries a non-NULL expires_at needs a production query and cannot be settled from code; the PR body now says that instead of implying a live bug.

Two stale doc claims fixed. expires_at is the row's own TTL column, not the approval-token deadline — that is approval_token_expires_at, added by migration 000051. And the comment justifying the branch said no code writes an 'expired' status and the CHECK excludes it; both are false (migration 000089's purchase_executions_status_check lists 'expired', and expireStaleExecutionsSweep writes it). Behaviourally harmless, since an 'expired' row is unscored and still governed by that branch, but it was the branch's stated justification.

Behaviour change now disclosed in the PR body: a canceled row with a future scheduled_date was previously retained forever, and now leaves History 30 days after cancellation rather than 30 days after its scheduled date.

Regression guards, both confirmed failing first

The failed case is new; the suite had no failed-row case at all, so it stayed green with the defect present. Reverting only the exclusion widening fails exactly failed-today-with-lapsed-expires-at plus the RowsAffected assertion. A second pgxmock guard pins the $2 argument itself rather than just the <> ALL($2) spelling, so passing a narrower list fails the build.

Verify with the tag — without it the file is excluded by its build tag and the run exits 0 having executed nothing:

go test -tags=integration ./internal/config/ \
  -run TestPostgresStoreDB_CleanupOldExecutions_RetainsCanceledInsideHealthWindow

Gates

go build ./... 0 | go vet ./internal/... 0 | go vet -tags=integration ./... 0 | go test ./internal/... 0 (19 pkgs) | integration test 0, and 0 under -race | gocyclo -over 10 (pre-commit form) 0 | golangci-lint v2.10.1, the version pinned in ci.yml, ./internal/... → 0 issues | full pre-commit suite passed on commit. All 6 CI workflows on the previous commit d69ed8fd finished green.

Two existing pgxmock cases were updated for the second query argument.

@coderabbitai full review

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- HEAD and PR diff summary ---'
git rev-parse --short HEAD
git status --short
git diff --name-status origin/main...HEAD
git log --oneline origin/main..HEAD

printf '%s\n' '--- relevant commit diff (if present) ---'
git show --format=fuller --stat 9166bc5d || true
git show --format= --unified=50 9166bc5d -- \
  internal/config/store_postgres.go \
  internal/config/store_postgres_pgxmock_test.go \
  internal/config/store_postgres_db_test.go \
  internal/api/plan_health.go \
  internal/api/plan_health_test.go \
  internal/config/types.go \
  internal/config/interfaces.go || true

printf '%s\n' '--- symbol outlines ---'
ast-grep outline internal/config/store_postgres.go --items all --type function --match 'CleanupOldExecutions|SavePurchaseExecution|CountExecutionsByPlanAndStatus' || true
ast-grep outline internal/api/plan_health.go --items all --type function --match 'computePlanHealth|planHealthExecutionStatuses' || true
ast-grep outline internal/config/types.go --items all --type type,variable --match 'HealthScoredExecutionStatuses|PurchaseExecution' || true

printf '%s\n' '--- focused definitions, usage, and test locations ---'
rg -n -C 8 \
  -e '\bHealthScoredExecutionStatuses\b' \
  -e '\bCleanupOldExecutions\s*\(' \
  -e '\bCountExecutionsByPlanAndStatus\s*\(' \
  -e '\bexpires_at\b' \
  -e '\btimeFromTTL\s*\(' \
  -e '\bexpireStaleExecutionsSweep\s*\(' \
  internal

printf '%s\n' '--- relevant test names ---'
rg -n -C 4 \
  -e 'RetainsCanceledInsideHealthWindow' \
  -e 'RetainsCanceledOnUpdatedAt' \
  -e 'HealthScoredExecutionStatuses' \
  internal/config

Length of output: 201862

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- current PR commit/file summary ---'
git rev-parse --short HEAD
git diff --stat origin/main...HEAD
git diff --name-only origin/main...HEAD

printf '%s\n' '--- status-list declarations and every relevant call site ---'
rg -n -C 6 \
  -e '^\s*(var|const)\s+planHealthExecutionStatuses\b' \
  -e '^\s*(var|const)\s+HealthScoredExecutionStatuses\b' \
  -e '\bCountExecutionsByPlanAndStatus\s*\(' \
  -e '\bCleanupOldExecutions\s*\(' \
  internal/api internal/config internal/server internal/mocks

printf '%s\n' '--- exact implementation regions ---'
sed -n '1,115p' internal/api/plan_health.go
sed -n '45,100p' internal/api/handler_plans.go
sed -n '1215,1275p' internal/config/store_postgres.go
sed -n '1710,1820p' internal/config/store_postgres.go
sed -n '385,435p' internal/config/types.go

printf '%s\n' '--- exact current incremental diff ---'
git diff --unified=30 d69ed8fd..HEAD -- \
  internal/config/store_postgres.go \
  internal/config/store_postgres_pgxmock_test.go \
  internal/config/store_postgres_db_test.go \
  internal/config/types.go \
  internal/api/plan_health.go \
  internal/api/handler_plans.go

printf '%s\n' '--- all direct cleanup callers, excluding declarations and test names ---'
rg -n -P '\.\s*CleanupOldExecutions\s*\(' \
  -g '*.go' \
  -g '!**/*_test.go' \
  internal

Length of output: 87604

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- diff hygiene ---'
git diff --check origin/main...HEAD
git diff --find-conflicts origin/main...HEAD || true

printf '%s\n' '--- changed production symbol map ---'
ast-grep outline internal/api/handler_plans.go --items all --type function --match 'listPlans|attachPlanHealth' || true
ast-grep outline internal/api/plan_health.go --items all --type function --match 'computePlanHealth|collectPlanHealthFactors|overdueFactor|failedExecutionsFactor|canceledExecutionsFactor|scheduleFactor|disabledMidwayFactor' || true
ast-grep outline frontend/src/plans.ts --items all --type function --match 'healthBadgeHtml|renderPlanCard' || true

printf '%s\n' '--- API/data-model and frontend rendering slices ---'
sed -n '1,135p' internal/api/types.go
rg -n -C 8 -e '^    PlanWithHealth:' -e '^    PlanHealthFactor:' -e '^    PlansResponse:' internal/api/openapi.yaml
sed -n '1,230p' frontend/src/plans.ts
sed -n '1,150p' frontend/src/api/types.ts
sed -n '1,130p' frontend/src/types.ts

printf '%s\n' '--- health-score test case index and assertions ---'
rg -n -C 3 \
  -e '^func TestComputePlanHealth' \
  -e '^func TestHandler_listPlans_Health' \
  -e "describe\\('plan health-score badge'" \
  internal/api/plan_health_test.go \
  internal/api/handler_plans_test.go \
  frontend/src/__tests__/plans.test.ts

Length of output: 46119

---

@cristim Full review of current HEAD 9166bc5d completed.

No additional actionable findings.

Confirmed in particular:

• Health counting and expires_at cleanup now use the same config-owned HealthScoredExecutionStatuses slice, including failed and both canceled spellings.
• The only production CleanupOldExecutions caller uses config.DefaultExecutionTTLDays, matching the health-score lookback.
• The failed-today/lapsed-expires_at PostgreSQL regression covers the newly identified score-jump path.
• The pgxmock guard verifies both the <> ALL($2) predicate and that $2 receives the full shared status list.
• API schema, nullable unknown-score behavior, frontend range handling, score-band boundaries, and tooltip escaping remain consistent with the backend contract.
• git diff --check is clean. I did not execute the test suite in this sandbox.

🐇🔎

[full_review]

---

✏️ Learnings added

Learnt from: cristim
URL: https://github.com/LeanerCloud/CUDly/pull/1521

Timestamp: 2026-07-28T19:30:45.477Z
Learning: For CUDly plan health scoring, `internal/api/plan_health.go` must use `config.HealthScoredExecutionStatuses` as the exact status list passed to `CountExecutionsByPlanAndStatus`. In `internal/config/store_postgres.go`, `PostgresStore.CleanupOldExecutions` must exclude every status in that same list from its `expires_at` cleanup branch, because a terminal transition updates `updated_at` but does not rewrite `expires_at`; otherwise recently failed or canceled executions can be deleted during the health-score lookback window and incorrectly raise a plan’s score.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.