[cristim]

Summary

Closes #1146

Owner decision (2026-10-20): Savings Plans are exempt from the per-service min-count filter. A Savings Plan recommendation's Count is always 1 and is not an instance count. This matches cloud-commitments-go#536 (the scorer exempts CommitmentSavingsPlan) and the CLI change, so the GUI, CLI and scorer agree.

• 12bd41d9 fix(scheduler): filterRecsByResolvedConfigs skips meetsMinCount when common.IsSavingsPlan(common.ServiceType(rec.Service)) is true. That existing library helper covers the umbrella savingsplans slug (also Azure's) and the four savings-plans-* slugs. config.RecommendationRecord has no commitment type field, so service-based classification is the closest match to the scorer's rule.
• ed48b78d fix(frontend): Savings Plans service cards label the field "Min count (not applied to Savings Plans)". The field stays visible because hiding it would make the next save overwrite a stored min_count with 0.

Tests

• TestFilterRecsByResolvedConfigs_MinCount_ExemptsSavingsPlans covers six Savings Plans service types (aws savingsplans and savings-plans-compute, -ec2instance, -sagemaker and -database, plus azure savingsplans). Each has Count 1 under MinCount 5 and is kept. An aws ec2 rec with Count 1 is dropped. With the exemption removed, all six Savings Plans subtests fail.
• A jest test in serviceFilters.test.ts checks the Savings Plans label and that other services keep the old one.

Verification evidence is posted as PR comments.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes
  • Minimum-count filtering no longer removes Savings Plans recommendations, whose counts represent dollar amounts rather than instances. Other services continue to follow the selected minimum-count threshold.
• User Interface
  • The minimum-count label now clarifies that the filter does not apply to Savings Plans services. For other services, the existing “0 = no filter” label remains.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Repository: LeanerCloud/cloud-commitments-platform/.coderabbit.yaml
• Review profile: CHILL
• Plan: Essentials
• Run ID: 0b81f7ac-279b-4e1a-bb41-20d014b95f23

📥 Commits

Reviewing files that changed from the base of the PR and between 2d822f3 and ed48b78.

📒 Files selected for processing: 4

• frontend/src/__tests__/serviceFilters.test.ts
• frontend/src/serviceFilters.ts
• internal/scheduler/scheduler_overrides.go
• internal/scheduler/scheduler_overrides_test.go

Included review availability: This review used your included allowance. 0 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

📝 Walkthrough

The scheduler now skips minimum-count filtering for Savings Plan services. The frontend indicates that the minimum-count setting does not apply to Savings Plans.

Changes: Savings Plans minimum-count handling

Layer / File(s)	Summary
Scheduler filtering and coverage internal/scheduler/scheduler_overrides.go, internal/scheduler/scheduler_overrides_test.go	The resolved-config filter skips the minimum-count check for Savings Plan services. Tests verify that AWS and Azure Savings Plans recommendations with count 1 remain when the minimum is 5, while an AWS EC2 recommendation is filtered.
Minimum-count settings label frontend/src/serviceFilters.ts, frontend/src/__tests__/serviceFilters.test.ts	The minimum-count label identifies the Savings Plans exemption. A test checks that the Savings Plans label contains “not applied to Savings Plans” and the RDS label does not.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to ed48b

The Savings Plans cards accurately indicate that minimum count does not apply. No outstanding issue from this review prevents merging.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 4 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly and concisely describes the main change: exempting Savings Plans from the scheduler minimum-count filter.
Linked Issues check		Issue #1146 requires the min-count filter to exempt all Savings Plans services, retain a Savings Plan recommendation with Count 1 and MinCount 5, filter an EC2 recommendation with the same count, and …
Out of Scope Changes check		The changes are limited to the scheduler exemption, its focused unit test, and the related Savings Plans settings label. These changes directly implement Issue #1146 and its automated test requirement…

Full details: Linked Issues check

Explanation

Issue #1146 requires the min-count filter to exempt all Savings Plans services, retain a Savings Plan recommendation with Count 1 and MinCount 5, filter an EC2 recommendation with the same count, and note the exemption in the settings UI. filterRecsByResolvedConfigs now skips meetsMinCount when common.IsSavingsPlan(common.ServiceType(rec.Service)) is true. The added Go test covers AWS and Azure Savings Plans services and confirms that the EC2 case is filtered. serviceFilters.ts labels the Savings Plans control as Min count (not applied to Savings Plans).

Full details: Out of Scope Changes check

Explanation

The changes are limited to the scheduler exemption, its focused unit test, and the related Savings Plans settings label. These changes directly implement Issue #1146 and its automated test requirements. No unrelated behavior is shown in the reviewed diff.

• Fix all pre-merge checks with AI

✨ Finishing Touches: 💡 1

📝 Generate docstrings: 💡

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• 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]

Merge gate at ed48b78dcd5a0725ee0677db1aac785707c10d30

Independent review 1 (cold brief, non-author): APPROVE ed48b78dcd5a0725ee0677db1aac785707c10d30

Attacked:

• Classification: common.IsSavingsPlan (pinned pkg types.go:159) accepts exactly the slugs every CommitmentSavingsPlan producer emits:
  • AWS parser_sp.go:259 emits the per-type slug.
  • Azure benefit_recommendations.go:241 and savingsplans/client.go:241 emit ServiceSavingsPlansAll.
  • Azure Advisor Savings Plans recs get ServiceSavingsPlansAll through serviceFromExtendedProperties.
• No over-exemption: Azure reservations and GCP CUDs use slugs outside that set, so no non-Savings Plans rec is exempted.
• Other min_count sites: scheduler_overrides.go:79 is the only place min_count filters recs; every other reference only stores or validates config.
• Frontend: only the label changes, and the stored min_count is still read and written.
• Mutation: an always-false guard fails all six Savings Plans subtests, and the ec2 case still drops its rec. The jest label probe was inconclusive (compile error); the test asserts the Savings Plans text present and the rds text absent.

Nits:

• internal/planstep/precheck.go:61 refuses plan services with any non-zero MinCount, including Savings Plans. That path refuses every filter, so it is outside fix(scheduler): exempt Savings Plans from the min-count filter #1146.
• The frontend Savings Plans check is a separate implementation from the Go one.

Independent review 2 (cold brief, non-author): APPROVE ed48b78dcd5a0725ee0677db1aac785707c10d30

Fixes #1146: the floor is skipped for every slug common.IsSavingsPlan accepts (pinned pkg common/types.go:159), and non-Savings Plans recs still hit it.

Attacked:

• Legacy and alias slugs: migration 000040 rewrote savings-plans/savingsplans to the split AWS slugs. savingsplans stays valid for Azure and is accepted. No other Savings Plans alias exists in internal/config.
• Include/exclude filters: inListRule still runs first, unchanged.
• Frontend vs backend: both agree on all five real slugs.
• Complexity: gocyclo is now exactly 10, which still passes the -over 10 hook.

Mutation probes:

• Removing the IsSavingsPlan guard fails all six Savings Plans subtests, while the ec2 subtest still passes.
• Making the jest Savings Plans branch dead fails the new label test.

Nits:

• The frontend check also matches the bare legacy savings-plans slug, which no stored data uses after migration 000040.
• The next branch added to this function will hit the gocyclo hook.

Local verification (macOS, exact head, worktree clean):

• go build ./..., go test -race -short -count=1 ./... and make lint all pass.
• Frontend: npm ci, npm run build, npm run lint (0 errors) and npx jest (108 suites, 3430 passed, 1 skipped) all pass.

CI: all checks are green at this SHA.

Threads: none open. CodeRabbit is waived as a merge gate.

[cristim]

Combined check before merge: current main (e304ebea, which now includes #1133 and #1138) merged with this PR gives tree 1342933f. On that tree npx tsc --noEmit, npm run build and npx jest pass (108 suites, 3453 passed, 1 skipped). The backend change only touches internal/scheduler, which nothing on main has changed since this branch's base.