test(coverage): assert CE lookback window width + add --rec-lookback-period flag (refs #360)

[cristim]

Summary

Completes the follow-up gaps left after #794 (merged), which implemented --coverage-lookback-days but left test coverage gaps and the second flag from issue #360 unimplemented.

• Gap 1 (CE window-width test): mockCoverageCE.GetReservationCoverage now captures the last TimePeriod from the input. TestGetRICoverageMap_LookbackWindowWidth calls GetRICoverageMap with 14 and 60 days, parses the YYYY-MM-DD Start/End strings, and asserts end-start == N days. This test fails if the window math regresses to a hardcoded value (e.g. reverting to 30).
• Gap 2 (remove tautological test): The TestFetchExistingCoverage_LookbackDays "custom lookback stored in Config" subcase, which only asserted cfg.CoverageLookbackDays == 60 (trivially true), is replaced by a comment pointing to the new discriminating test above.
• Gap 3 (second requested flag): Adds --rec-lookback-period (values: 7d/30d/60d, default 7d) controlling LookbackPeriodInDays in GetReservationPurchaseRecommendation. Wired through Config.RecLookbackPeriod -> fetchRecommendationsForRegion (CLI path) and Client.SetRecLookbackPeriod (discovery/UI path). Validated in validateFlags. TestSetRecLookbackPeriod_ReachesGetReservationPurchaseRecommendation asserts all three enum values reach the actual CE input. TestValidateRecLookbackPeriod guards the validator.

References #794. refs #360 (already closed).

Test plan

• go build ./... passes from root and from providers/aws/
• cd providers/aws && go test ./recommendations/ - 296 tests pass (includes new window-width and rec-lookback-period tests)
• go test ./cmd/... - 758 tests pass (includes new validator test)
• Gap 1 is discriminating: if GetRICoverageMap used a hardcoded 30 instead of lookbackDays, calling with 14 would produce spanDays=30 != 14 and the test would fail

Summary by CodeRabbit

• New Features

  • Added a configurable recommendation lookback period for AWS reservation recommendations.
  • Supported lookback windows are 7, 30, or 60 days.
  • Added the --rec-lookback-period option, defaulting to 7 days.
  • The selected period now applies consistently when retrieving reservation recommendations and coverage data.

• Bug Fixes

  • Invalid lookback-period values now produce a clear validation error before processing begins.

[cristim]

@coderabbitai review

[coderabbitai]

✅ 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.

[coderabbitai]

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 43 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: b50bd1a9-2592-45a3-83d8-a93f8606182c

📥 Commits

Reviewing files that changed from the base of the PR and between 7ac4490 and bee2aa5.

📒 Files selected for processing (6)

• cmd/main.go
• cmd/multi_service.go
• cmd/multi_service_coverage_test.go
• cmd/multi_service_helpers.go
• cmd/validators.go
• cmd/validators_test.go

📝 Walkthrough

Walkthrough

The CLI now accepts validated reservation-recommendation lookback periods of 7, 30, or 60 days. The value flows through multi-service execution and AWS adapters into Cost Explorer requests, with tests covering request periods and coverage-window calculations.

Changes

Configurable reservation recommendation lookback

Layer / File(s)	Summary
CLI option and validation cmd/main.go, cmd/validators.go, cmd/validators_test.go	Adds RecLookbackPeriod, the --rec-lookback-period flag, supported-value validation, and table-driven validation tests.
Recommendations request plumbing providers/aws/recommendations/client.go, providers/aws/recommendations/client_test.go, providers/aws/service_client.go	Stores and forwards the configured period to Cost Explorer recommendation requests, applies the 7d default, and preserves cancellation-aware combination processing.
Multi-service propagation and purchase orchestration cmd/multi_service.go, cmd/multi_service_helpers.go, cmd/multi_service_coverage_test.go	Forwards the period through multi-service recommendation fetching and moves confirmation, purchase, and reporting into runPurchaseAndReport.
Cost Explorer window verification providers/aws/recommendations/coverage_test.go	Captures coverage request periods and verifies their calendar-day span for multiple lookback values.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant CLI
  participant runToolMultiService
  participant RecommendationsClientAdapter
  participant recommendations.Client
  participant CostExplorer

  CLI->>runToolMultiService: RecLookbackPeriod
  runToolMultiService->>RecommendationsClientAdapter: SetRecLookbackPeriod(period)
  RecommendationsClientAdapter->>recommendations.Client: SetRecLookbackPeriod(period)
  runToolMultiService->>recommendations.Client: GetRecommendationsForService
  recommendations.Client->>CostExplorer: GetReservationPurchaseRecommendation with LookbackPeriodInDays
  CostExplorer-->>recommendations.Client: Recommendation response

Loading

Possibly related PRs

• LeanerCloud/CUDly#195: Overlaps with the recommendation client’s per-combination Cost Explorer request loop.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately captures the main changes: coverage lookback window assertions and the new rec-lookback-period flag.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
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.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/360-coverage-window-tests-and-rec-lookback

---

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

[cristim]

Rebased on feat/multicloud-web-frontend. Resolved conflict in providers/aws/recommendations/client.go (additive merge: kept both ec2API/instanceTypePagerFactory/skuCatalog fields from base and recLookbackPeriod field from this PR; same for SetInstanceTypePagerFactory/instanceTypeLookup and SetRecLookbackPeriod methods). CR BILLING_BLOCKED; no re-trigger sent.

[cristim]

@coderabbitai review

[coderabbitai]

✅ 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]

Adversarial review (PR #992)

Reviewed against the brief's risk surfaces. The window-width test and --rec-lookback-period flag are both correctly wired; one magic-value-duplication finding was fixable in scope and is now pushed.

Verified

• Window-width test is discriminating. TestGetRICoverageMap_LookbackWindowWidth parses the YYYY-MM-DD Start/End strings the production code emits, computes spanDays = end.Sub(start).Hours()/24, and asserts spanDays == lookbackDays for both 14 and 60. The production code (coverage.go:181-184) computes end = time.Now().UTC() then start = end.AddDate(0, 0, -lookbackDays) — purely UTC arithmetic so there's no DST/calendar drift, and both timestamps are formatted from the same time.Now() base so day-rollover skew between the captured Start and End is impossible. Mutation check: reverting to start := end.AddDate(0, 0, -30) (hardcoded 30) makes the 14-day case fail with spanDays=30 != 14. ✓
• SetRecLookbackPeriod test is discriminating. TestSetRecLookbackPeriod_ReachesGetReservationPurchaseRecommendation parameterises all three valid values, captures riCalls on the mock, and asserts call.LookbackPeriodInDays == tc.wantEnum against the typed SDK enum (types.LookbackPeriodInDaysSevenDays/ThirtyDays/SixtyDays). Mutation check: hardcoding lookback = "7d" in fetchSingleComboRecs would fail the 30d and 60d cases. ✓
• Validator rejects negative / fractional / unknown (per feedback_strict_int_parse.md spirit). validateRecLookbackPeriod is a map-membership check on {"7d", "30d", "60d"} so "", "14d", "90d", "SEVEN_DAYS", fractions, and arbitrary text all produce "invalid rec-lookback-period: must be one of 7d, 30d, 60d". Wired into validateFlags (cobra PreRunE on rootCmd) so it always fires on the CLI path before any work runs. ✓
• Flag value range matches the AWS SDK enum exactly: types.LookbackPeriodInDays only admits SEVEN_DAYS/THIRTY_DAYS/SIXTY_DAYS, mapped 1:1 by convertLookbackPeriodE. There is no 14d/45d/90d in the SDK; the validator is correctly tight. ✓
• Independent from --target-coverage and --coverage-lookback-days. The three flags are wired through distinct fields (TargetCoverage, CoverageLookbackDays, RecLookbackPeriod) and consume distinct APIs (GetReservationCoverage for the first two, GetReservationPurchaseRecommendation for --rec-lookback-period), confirming project_target_coverage_uses_coverage_api.md. ✓
• Negative/edge tests are present. TestValidateRecLookbackPeriod covers "", "14d", "90d", "SEVEN_DAYS" rejections plus the three valid values. ✓
• Tautological test removed. The PR replaces the "custom lookback stored in Config" subcase (which only asserted cfg.CoverageLookbackDays == 60 after assigning 60 to it) with an inline comment pointing at the discriminating window-width test. ✓
• Tests pass locally: providers/aws && go test ./recommendations/ → 338 pass; go test ./cmd/ → 740 pass. Build is clean (go build ./..., go vet).

Fix pushed (aa555f53c)

• Magic-value duplication: "7d" literal in three production locations (feedback_no_hardcoded_magic_values.md). The default appeared as a raw "7d" in cmd/main.go flag default, cmd/multi_service_helpers.go:426 fallback (CLI path), and providers/aws/recommendations/client.go:251 fallback (discovery path). Hoisted into recommendations.DefaultRecLookbackPeriod = "7d" next to maxRecommendationPages, and all three call sites now reference the constant. No behavioural change; a future change to the default flips one value instead of three.

Out of scope (no follow-up issue filed)

• Validator and convertLookbackPeriodE independently enumerate {7d, 30d, 60d} (cmd/validators.go:41 map; providers/aws/recommendations/converters.go:88-95 switch). Drift risk if AWS ever adds a fourth lookback window. Could be tightened by having validateRecLookbackPeriod call convertLookbackPeriodE directly; refactor is wider than this PR's mandate and matches the existing pattern used for validatePaymentAndTerm. Not filing a follow-up — the practical drift risk is low because AWS hasn't extended this enum in ~5 years.
• convertLookbackPeriod (the silent wrapper) docstring claims "silently defaults to SevenDays" but actually returns the empty enum "" on the error path (converters.go:106-109). Pre-existing, not touched by this PR.

UNSTABLE state (pre-existing on main)

• Lint Code: ~30 pre-existing errcheck errors across cmd/configure_azure.go, cmd/configure_gcp.go, internal/api/handler*.go, cmd/cleanup-lambda/main.go, etc. None in files this PR touches.
• Integration Tests: RDS DescribeDBMajorEngineVersions: no EC2 IMDS role found — CI runner lacks AWS credentials, unrelated to the diff.
• Security Scanning: pre-existing govulncheck failure on main (last 5 main runs all failure).
• CI Success: aggregator failure tracking the above.

None are caused by this PR.

Stale-rebase note

gh pr view 992 reports mergeable=MERGEABLE against baseRefOid=451a70f73 (the stale main snapshot from when the PR was opened), but git merge-tree main HEAD against current main (11005399c, the #692 paginate commit) reports 25 add/add conflicts — the branch will need a rebase before merge. Not blocking the review.

Verdict

LGTM. CR re-ping posted (@coderabbitai review).

[cristim]

@coderabbitai review

[coderabbitai]

✅ 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.

[coderabbitai]

Tip

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

@cristim Thanks for the thorough adversarial pass.

This all sounds solid to me: the discriminating test coverage for both the CE window width and --rec-lookback-period wiring is exactly the kind of evidence that reduces regression risk, and hoisting the default into recommendations.DefaultRecLookbackPeriod is a nice cleanup for the in-scope magic-value duplication.

I also agree the remaining points are reasonable to leave out of scope for this PR:

• the validator/converter duplication is low-risk and pre-existing
• the convertLookbackPeriod doc mismatch is separate cleanup
• the unstable CI items you called out shouldn’t be attributed to this change

Given your verification and the pushed constant cleanup, this looks non-blocking from my side.
🐇✅

[coderabbitai]

Caution

Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted.

Error details

{"name":"HttpError","status":500,"request":{"method":"PATCH","url":"https://api.github.com/repos/LeanerCloud/CUDly/issues/comments/4631221181","headers":{"accept":"application/vnd.github.v3+json","user-agent":"octokit.js/0.0.0-development octokit-core.js/7.0.6 Node.js/24","content-type":"application/json; charset=utf-8"},"body":{"body":"<!-- This is an auto-generated comment: summarize by coderabbit.ai -->\n<!-- review_stack_entry_start -->\n\n[![Review Change Stack](https://storage.googleapis.com/coderabbit_public_assets/review-stack-in-coderabbit-ui.svg)](https://app.coderabbit.ai/change-stack/LeanerCloud/CUDly/pull/992?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack)\n\n<!-- review_stack_entry_end -->\n<!-- This is an auto-generated comment: review in progress by coderabbit.ai -->\n\n> [!NOTE]\n> Currently processing new changes in this PR. This may take a few minutes, please wait...\n> \n> <details>\n> <summary>⚙️ Run configuration</summary>\n> \n> **Configuration used**: defaults\n> \n> **Review profile**: CHILL\n> \n> **Plan**: Pro\n> \n> **Run ID**: `69abede6-265c-4b92-b077-e680c3bd2d3e`\n> \n> </details>\n> \n> <details>\n> <summary>📥 Commits</summary>\n> \n> Reviewing files that changed from the base of the PR and between 451a70f736464337a4df34472f24ecb42241f518 and aa555f53cf2ef4cbebb4182991dbf45d2ea43569.\n> \n> </details>\n> \n> <details>\n> <summary>📒 Files selected for processing (10)</summary>\n> \n> * `cmd/main.go`\n> * `cmd/multi_service.go`\n> * `cmd/multi_service_coverage_test.go`\n> * `cmd/multi_service_helpers.go`\n> * `cmd/validators.go`\n> * `cmd/validators_test.go`\n> * `providers/aws/recommendations/client.go`\n> * `providers/aws/recommendations/client_test.go`\n> * `providers/aws/recommendations/coverage_test.go`\n> * `providers/aws/service_client.go`\n> \n> </details>\n> \n> ```ascii\n>  __________________________________________\n> < My GPUs are ready. Let's find some bugs. >\n>  ------------------------------------------\n>   \\\n>    \\   \\\n>         \\ /\\\n>         ( )\n>       .( o ).\n> ```\n\n<!-- end of auto-generated comment: review in progress by coderabbit.ai -->\n\n<!-- finishing_touch_checkbox_start -->\n\n<details>\n<summary>✨ Finishing Touches</summary>\n\n<details>\n<summary>📝 Generate docstrings</summary>\n\n- [ ] <!-- {\"checkboxId\": \"7962f53c-55bc-4827-bfbf-6a18da830691\"} --> Create stacked PR\n- [ ] <!-- {\"checkboxId\": \"3e1879ae-f29b-4d0d-8e06-d12b7ba33d98\"} --> Commit on current branch\n\n</details>\n<details>\n<summary>🧪 Generate unit tests (beta)</summary>\n\n- [ ] <!-- {\"checkboxId\": \"f47ac10b-58cc-4372-a567-0e02b2c3d479\", \"radioGroupId\": \"utg-output-choice-group-unknown_comment_id\"} -->   Create PR with unit tests\n- [ ] <!-- {\"checkboxId\": \"6ba7b810-9dad-11d1-80b4-00c04fd430c8\", \"radioGroupId\": \"utg-output-choice-group-unknown_comment_id\"} -->   Commit unit tests in branch `fix/360-coverage-window-tests-and-rec-lookback`\n\n</details>\n\n</details>\n\n<!-- finishing_touch_checkbox_end -->\n<!-- tips_start -->\n\n---\n\n\n\n\n<sub>Comment `@coderabbitai help` to get the list of available commands.</sub>\n\n<!-- tips_end -->"},"request":{"retryCount":3,"signal":{},"retries":3,"retryAfter":16}}}

[cristim]

@coderabbitai review

[coderabbitai]

✅ 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]

@coderabbitai review

[coderabbitai]

✅ 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]

Rebased onto origin/main (132 commits behind). Two struct-comment conflicts in cmd/main.go resolved by keeping main's Config layout and adding the PR's new RecLookbackPeriod field on top; all other files auto-merged cleanly.

Gates (root module + providers/aws module):

• build=0 (both modules)
• vet=0 (both modules)
• gocyclo=0 (touched files, threshold 10)
• lintnew=0 root; lintnew=0 providers/aws (5 misspell/gocritic warnings in new code, severity=warning, exit 0)
• test-cmd=0 (761 passed, 7 packages)
• test-recs=0 (398 passed, 1 package)

Branch force-pushed: fix/360-coverage-window-tests-and-rec-lookback

@coderabbitai review

[coderabbitai]

@cristim Thanks for rebasing and validating both modules. I’ll review the updated branch, including the post-rebase conflict resolution in cmd/main.go.

✅ 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-rebased onto origin/main post-#1208 merge (ed288c4). DefaultRecLookbackPeriod const is confirmed defined in providers/aws/recommendations/client.go:33 and referenced in cmd/main.go:156 and cmd/multi_service_helpers.go:428. Both modules compile and test clean.

@coderabbitai review

[coderabbitai]

@cristim Thanks for validating the post-#1208 rebase and the centralized default references. I’ll review the updated branch.

✅ 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]

Merged to main (rebased onto current main, CLEAN + all CI green). Adds CE lookback-window width assertions + rec-lookback coverage tests; DefaultRecLookbackPeriod constant confirmed defined. Test-only.