Skip to content

fix(recommendations): use pool demand for expiry coverage - #2133

Open
cristim wants to merge 2 commits into
mainfrom
codex/go70-cli-expiry-consumer
Open

cristim wants to merge 2 commits into
mainfrom
codex/go70-cli-expiry-consumer

Conversation

@cristim

@cristim cristim commented Oct 4, 2026 •

Copy link
Copy Markdown
Member

Scope

Use authoritative pool demand when adjusting expiry coverage, and report how many recommendations retain their coverage because pool demand is unavailable. Pin the published shared-package and AWS prerequisites.

Refs LeanerCloud/cloud-commitments-go#70.

This is partial consumer prerequisite A: four files, 153 changed lines. The separate B verification layer will add the six root-command/TLS-SDK/CSV regressions. This PR does not close issue 70 or independently establish CLI delivery.

Exact revision and dependencies

  • Head: 6007c4f4fdc767a2a296c09e70999d0f97c6aaae
  • Parent: 652fc94af4593273946ad8d5eafc2ad430496f1e
  • Tree: 7d68b349ec0e71203c426842c636bd30d84449f7
  • pkg: v0.0.0-20261004010532-e6c7eb87968a, full commit e6c7eb87968a5bdc05e604cbc1e922d5ce97ce36 (prerequisite)
  • AWS: v0.0.0-20261004034603-d4b69ab4f8b1, full commit d4b69ab4f8b10b241ad93d4e78171a597364a5cc (combined producer)

Both modules were resolved from published commits with normal checksum verification, GOWORK=off, and no replace directive.

Local evidence

Native macOS arm64, Go 1.26.6, published dependencies. The exact four source blobs were verified before and after the precommit run; normal hooks committed the unchanged reviewed tree.

  • Helper expiry matrix: 17 passed. This is helper-only evidence, not the B command scenarios.
  • Retained original command completeness suite: 51 passed.
  • Full and integration-tag race suites: each 1,036 passed, zero failures, three existing cloud tests skipped.
  • Build, vet, lint, tidy and format checks passed. Lint executable: /Users/cristi/go/bin/golangci-lint; version: 2.10.1.
  • Native artifact metadata binds both published versions and sums, with no replacements. Linked-worktree evidence uses -buildvcs=false and external source/tree binding, not a misleading embedded ancestor revision. Artifact SHA-256: df769e8440da24cc143cd3e9608ec2e701de43472a89fa4cea2f8445509b5211; --help passed.
  • Two local source reviews and two exact staged reviews were clean. All applicable normal commit hooks passed, including gosec and Trivy. Hook log SHA-256: 02735c8da9750fa803febdaae3221e22fea737839fe5d68cdc389d6076df4851.

The three uncovered existing tests are TestRunTool, TestGetAccountAliasRealFunction, and TestGetAllAWSRegions/Integration_test. No real credentials, cloud calls, purchases, deployments or Windows runs were used.

The preserved aggregate checkout separately passed six synthetic SDK-to-CSV cases and seven isolated mutation probes. Those B tests are not in A. Parent-compatible aggregate tests reproduced incorrect baseline counts/costs, and the unchanged treatment expectations passed. This is synthetic local evidence, not live acceptance or committed-B qualification.

Independent committed-head review

Fresh gpt-6-astra review of exact commit 6007c4f4fdc767a2a296c09e70999d0f97c6aaae found no actionable findings. The reviewer checked the published AWS/pkg source contracts and independently ran five synthetic sizing scenarios, the 17 committed helper tests, build, vet and lint on native macOS with Go 1.26.6. Published module versions, sums and origins matched, with GOWORK=off and no replacements.

The independent baseline overlaid the parent CLI helper onto the new dependencies: missing-demand assertions failed while four controls passed. This isolates the consumer call and warning, not historical old-dependency behavior. Three connected mutations independently failed the intended denominator, warning and precision assertions while their controls passed. Treatment passed. The initial sandbox cache-access failure was retained as a setup failure; the approved retry passed using the same existing caches.

The reviewer inspected, but did not independently rerun, the author's broad suites, artifact/help and hook evidence above. Independent probes were helper-only, not command or real-cloud acceptance. Verdict SHA-256: b554fe320841dc56a64b159043030176fd82e0f946a30241a2110af8f3a2bbec.

Holds

  • Exact-head Linux CI - Build & Test and pre-commit both passed for 6007c4f4fdc767a2a296c09e70999d0f97c6aaae. Both owned watchers terminated successfully. This does not resolve the remaining holds below.
  • B's command verification layer and exact committed qualification remain pending.
  • Real-scenario acceptance remains unresolved. Keep this PR and the dependency stack open if that proof is unavailable.
  • After accepted producer integration, resolve actual final main commits and repin/retest/review the CLI. These feature-commit pins are not final rollout evidence.
  • CodeRabbit is explicitly waived in favor of exact-revision local verification and independent review. Do not trigger CodeRabbit or treat the waiver as an acceptance waiver.

Existing separate CLI issues #2131 (linked inventory) and #2132 (RDS family expiry overwrite) are unchanged. Remaining issue-70 delivery is tracked on the parent issue and approved stack; no duplicate follow-up issue is needed.

Summary by CodeRabbit

  • Improvements
    • Coverage adjustments for expiring commitments now account for available pool demand. When demand data is missing or invalid, the existing coverage remains unchanged, and a warning is logged.
    • Adjustments continue to report partial coverage when only some recommendations can be covered.

Use the coverage-aware AWS API and report missing-demand skips.
Pin the published exact-coverage prerequisites and test helper sizing.

This is the consumer prerequisite for issue 70; command-level proof
is retained in the separate verification layer. Real-scenario
acceptance and final dependency repins remain outstanding.
@cristim cristim added triaged Item has been triaged priority/p1 Next up; this sprint severity/medium Moderate harm urgency/this-sprint Within the current sprint impact/many Affects most users effort/s Hours type/bug Defect labels Oct 4, 2026
@coderabbitai

coderabbitai Bot commented Oct 4, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: LeanerCloud/cloud-commitments-cli/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Essentials
  • Run ID: 74ae8cd7-818e-4efb-aa9b-f5cdf7d96c13
📥 Commits

Reviewing files that changed from the base of the PR and between 6007c4f and 24dc60f.

⛔ Files ignored due to path filters (1)
  • go.sum is excluded by !**/*.sum
📒 Files selected for processing (1)
  • go.mod

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

Walkthrough

Expiry sizing now uses coverage data when adjusting recommendations. When pool demand is unavailable, the adjustment is skipped and coverage remains unchanged. Tests cover sizing cases, missing demand, and zero-demand rows. Two Go dependencies use newer pseudo-versions.

Changes

Reservation expiry sizing

Layer / File(s) Summary
Expiry adjustment
cmd/multi_service_helpers.go
Expiry adjustment uses the coverage-aware helper and logs recommendations skipped because pool demand is unavailable.
Sizing validation and dependencies
cmd/reservation_expiry_test.go, go.mod
Tests cover sizing boundaries, account exclusions, missing demand, and zero-demand rows. go.mod updates two dependency versions.

Priority: ⬇️ Low

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

Change: Bug fix · Unblocks: 1 PR

Merge Risk: ⚪ Minimal · up to 24dc6

No actionable defect is established for this change. Final dependency and real-scenario qualification remain outstanding before delivery can be claimed.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 14.29% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 2 files. (1 skipped: 1… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: using pool demand to adjust expiry coverage.
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.
Full details: Docstring Coverage

Explanation

Docstring coverage is 14.29% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 2 files. (1 skipped: 1 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

@cristim

cristim commented Oct 5, 2026

Copy link
Copy Markdown
Member Author

Coordination: I am preparing the final merged dependency pins for this PR (pkg de46f760cdcf, AWS 945a4045d11f), followed by an additive parent merge into #2134 and renewed exact-head macOS/Astra/CI evidence. Existing commits and tests will be preserved. Please flag overlapping ownership before editing these two branches. This is not a merge-ready claim; real-scenario acceptance remains outstanding.

@cristim

cristim commented Oct 5, 2026

Copy link
Copy Markdown
Member Author

Published 24dc60f with the merged pkg de46f760cdcf and AWS 945a4045d11f module pins. A fresh independent gpt-6-astra agent reviewed the full PR and effective dependency delta at this exact SHA: no actionable findings. Independent native macOS replay passed expiry helper and retained completeness tests, all ten affected dependency packages without skips, build/vet/tidy/format, and the native artifact module/checksum binding plus help. Artifact SHA256: 61d0a08352414b17d217d3bc726ee1cce2ce1dec65d1ffc0c381d71e625246c7. Author full race and integration suites passed with three explicitly excluded real-cloud tests; lint and normal commit hooks passed. Evidence is synthetic SDK-boundary testing, not real multi-account acceptance. Historical baseline/mutation evidence was audited, not rerun by the final reviewer. Fresh CI runs 37257116910 and 37257116939 are being watched. This is not merge-ready: final-SHA CI and real affected-scenario acceptance remain required. Child #2134 dependency integration is staged separately. Using the user-approved independent review path; no new CodeRabbit request.

@cristim

cristim commented Oct 5, 2026

Copy link
Copy Markdown
Member Author

Final-SHA CI update: both workflows for 24dc60f completed successfully: CI - Build & Test https://github.com/LeanerCloud/cloud-commitments-cli/actions/runs/37257116910 and pre-commit https://github.com/LeanerCloud/cloud-commitments-cli/actions/runs/37257116939. Both background watchers have exited and been reaped. Independent Astra and native verification evidence are in the preceding comment. This PR remains open solely pending the applicable real affected-scenario acceptance; no cloud access or purchase was performed by this verification.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/s Hours impact/many Affects most users priority/p1 Next up; this sprint severity/medium Moderate harm triaged Item has been triaged type/bug Defect urgency/this-sprint Within the current sprint

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant