Skip to content

fix(aws): scope ladder baseline observations to account - #413

Merged
cristim merged 1 commit into
mainfrom
fix/282-account-scoped-residual
Oct 15, 2026
Merged

cristim merged 1 commit into
mainfrom
fix/282-account-scoped-residual

Conversation

@cristim

@cristim cristim commented Oct 15, 2026 •

Copy link
Copy Markdown
Member

The AWS ladder's residual baseline now queries DAILY Cost Explorer observations for its configured linked account. Each page retains the account, region, service and purchase-type filters. Scoped responses reject malformed periods and duplicate days; baseline construction requires a contiguous in-window prefix while preserving existing count and reporting-lag guards.

The legacy public unscoped collector remains available. Missing days are not filled with zero, and explicit zero observations keep their existing behavior.

Reviewed head: 1bf6d0e4a54c7aaf7e72c37ff1069cf024e2c69c, tree 5022c3249df61174064eecda03077cb36289b98d.

Validation includes native macOS build/race tests, actual-parent fail-before proof, seven killed/restored scratch mutations, and a fresh published AWS-only consumer with natural shared-module selection. The renewed independent two-pass review and exact-head evidence cover this revision. SDK transport evidence uses constructed loopback fixtures rather than live AWS billing responses. CI retry history and its verification limits are recorded in the final gate comment.

Refs #282 and #349. This ships the scoped residual-observation prerequisite. It does not reconstruct total demand, normalize historical E/k, update platform module pins, or enable an executor or purchases.

@cristim cristim added priority/p2 Backlog-worthy severity/medium Moderate harm triaged Item has been triaged urgency/this-sprint Within the current sprint impact/few Limited audience effort/m Days type/bug Defect labels Oct 15, 2026
@cristim

cristim commented Oct 15, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor
⚠️ Action not completed

Review rate limited.

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

coderabbitai Bot commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: LeanerCloud/cloud-commitments-go/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Essentials
  • Run ID: f5b82c17-e972-4a45-8784-42bac628e7ca
📥 Commits

Reviewing files that changed from the base of the PR and between c2a461a and 1bf6d0e.

📒 Files selected for processing: 8
  • providers/aws/ladder/adapters.go
  • providers/aws/ladder/adapters_test.go
  • providers/aws/ladder/baseline.go
  • providers/aws/ladder/baseline_sdk_test.go
  • providers/aws/ladder/factory.go
  • providers/aws/ladder/ladder_test.go
  • providers/aws/recommendations/ondemand_series.go
  • providers/aws/recommendations/ondemand_series_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 recommendations client now supports account-scoped on-demand cost series and validates scoped daily periods. The ladder adapter passes the configured account ID. Usage baseline validation now requires ordered, consecutive daily points from the window start while allowing trailing reporting lag.

Changes: Account-Scoped On-Demand Series

Layer / File(s) Summary
Account-scoped query and period validation
providers/aws/recommendations/ondemand_series.go, providers/aws/recommendations/ondemand_series_test.go
The recommendations client adds a linked-account filter and validates periods and duplicate dates in scoped responses. Tests cover invalid inputs and response periods.
Ladder adapter account wiring
providers/aws/ladder/adapters.go, providers/aws/ladder/factory.go, providers/aws/ladder/adapters_test.go
The ladder adapter passes the configured account ID to the recommendations client. Adapter tests use account IDs and daily Cost Explorer periods.

Usage Baseline Coverage

Layer / File(s) Summary
Daily coverage rules and baseline tests
providers/aws/ladder/baseline.go, providers/aws/ladder/baseline_sdk_test.go, providers/aws/ladder/ladder_test.go
Coverage validation rejects unordered, duplicate, out-of-window, and nonconsecutive points. Tests cover Cost Explorer requests and responses, freshness boundaries, and minimum coverage.

Priority: ➖ Normal

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

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant Adapter as onDemandSeriesAdapter
  participant Client as recommendations.Client
  participant CE as AWS Cost Explorer
  Adapter->>Client: GetOnDemandSeriesForAccount(accountID, region, lookbackDays)
  Client->>CE: Query with LINKED_ACCOUNT filter
  CE-->>Client: Cost and period rows
  Client-->>Adapter: Validated daily costs
Loading
Merge Risk: ⚪ Minimal · up to 1bf6d

No actionable issue remains identified in this change; it is mergeable after normal checks.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 16 functions across 8 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed 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 clearly and concisely describes the primary change: scoping AWS ladder baseline observations to the requested account.
  • 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

cristim commented Oct 15, 2026 •

Copy link
Copy Markdown
Member Author

Independent cold review: no actionable findings in either of two adversarial passes, performed by the same gpt-6.1-sol reviewer at MEDIUM effort.

Reviewed head 152fe913ad4dca83fbf3ef76b44900d48e5aa1f4, tree 8b23c1a1738d3bc9e577684d4d5bc2bdb64acc6b, against parent 498b74f31f01aca9b2c795d1dce9d85c790eabea. Rechecked the PR head and unchanged source checkout after verification. This verdict covers the full eight-file diff, not CI or merge permission.

Pass 1 attacked the actual production caller, payer/member scope leakage, filter/page preservation, malformed/duplicate periods before map overwrite, missing observations versus explicit zero, and the independent sample/count/age guards. Pass 2 rechecked those conclusions against the pinned SDK, legacy API behavior, concrete parent failures and isolated mutations, plus completeness, correctness, security, bugs, reuse, scope and unnecessary machinery. The new scoped API has a current factory caller and reuses the existing numeric parser and sorter. It adds no purchases or cloud-backed verification.

Checked source fields:

Contract Implementation evidence
Configured account reaches the production baseline providers/aws/ladder/factory.go:89, adapters.go:224
DAILY, CamelCase UnblendedCost, inclusive-start/exclusive-end request, account/service/purchase/region AND on every page providers/aws/recommendations/ondemand_series.go:118, :126, :131, :148, :246
Start and End parse, exact one-day in-window interval, duplicate rejection before accumulation providers/aws/recommendations/ondemand_series.go:175, :188, :191, :194
Minimum samples, contiguous prefix, count and trailing age remain separate checks providers/aws/ladder/baseline.go:89, :146, :165, :197
Legacy unscoped entry point, numeric parsing, chronological sorting, explicit zero and empty/all-zero errors providers/aws/recommendations/ondemand_series.go:101, :276, :322

Source of truth: pinned Cost Explorer SDK v1.61.0 api_op_GetCostAndUsage.go:42-112, types/types.go:975-994 (DateInterval), :1494-1502 (MetricValue), :2061-2075 (ResultByTime), types/enums.go:345 (LINKED_ACCOUNT), and AWS GetCostAndUsage, GetDimensionValues, ResultByTime, and MetricValue. The docs establish request/response structure and supported dimensions, not this account's live economics.

Fresh macOS verification, all with GOTOOLCHAIN=go1.26.9 GOWORK=off, ambient credentials disabled and retained-inode shared runner locks:

  • From the exact head's AWS module: go build ./...; go test -race -short -count=1 -run '^TestScopedBaselineParentCompatible$|TestGetOnDemandSeriesForAccount' ./ladder ./recommendations; then both affected package suites via go test -race -short -count=1 ./ladder ./recommendations. All passed.
  • Independently archived actual parent 498b74f, transferred only the new baseline_sdk_test.go, and compiled successfully. Its regression run failed the intended account, duplicate/page-duplicate, hole/leading, outside/future, nil/bad End and short/long/backward period cases. Positive reverse-order, explicit-zero and lag controls passed, as did the pre-existing malformed-amount/empty/all-zero error controls.
  • Independently mutated seven separate head archives: wrong account dimension, wrong factory account, dropped filters on page two, disabled exact-day validation, disabled collector window validation, disabled duplicate validation, and disabled contiguous-prefix validation. Each selected assertion failed for its intended behavior, with no compilation failures. Every changed scratch source was restored immediately after its probe; the live source was never edited.
  • Fresh external consumer initially required only published AWS revision 152fe913..., with no shared pin, replace, or workspace. Normal go mod tidy naturally resolved AWS v0.0.0-20261015093120-152fe913ad4d and shared v0.0.0-20261015071854-5b254484d5e4; go list -m -json recorded module provenance. Published production files matched the reviewed archive byte-for-byte. Its public NewFromConfig SDK-loopback suite and independently written legacy/scoped collector probes passed go test -race -count=1 ./.... These assert every filter on each page, legacy unscoped/duplicate behavior, sorting, zero, empty/all-zero, parse/provider errors and missing-account rejection before I/O.

Evidence is constructed CE-shaped HTTP/SDK-loopback fixtures, not recorded billing data or live AWS economic acceptance. Total-demand reconstruction, native normalization and Compute purchase enablement are outside this residual-only verdict. No new follow-up defect was found.

Keep other accounts and incomplete daily observations out of the
ladder residual percentile while preserving measured zero days.

Refs #282
@cristim
cristim force-pushed the fix/282-account-scoped-residual branch from 152fe91 to 1bf6d0e Compare October 15, 2026 10:06
@cristim

cristim commented Oct 15, 2026

Copy link
Copy Markdown
Member Author

Rebased 152fe91 to 1bf6d0e on parent c2a461a. The complete intervening main delta contains 11 files, all under providers/gcp, including its module pin update.

AWS and shared pkg subtree bytes are unchanged, and range-diff confirms the AWS patch is identical. The original reviewed commit is preserved at backup/go413-reviewed-152fe913.

Normal hooks on the eight changed AWS files, the standalone AWS build, and focused public SDK/collector race tests passed on the rebased candidate. The retained independent cold reviewer completed two clean candidate rebind passes.

New-head CI and published-candidate consumer proof remain pending. Prior-head successful checks do not establish new-head CI success.

@cristim

cristim commented Oct 15, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@cristim

cristim commented Oct 15, 2026

Copy link
Copy Markdown
Member Author

Renewed independent verdict after the normal rebase: two adversarial passes, no actionable findings for head 1bf6d0e4a54c7aaf7e72c37ff1069cf024e2c69c, tree 5022c3249df61174064eecda03077cb36289b98d, parent c2a461a801e3541860893f62a05e825364ac74a2. Same retained gpt-6.1-sol MEDIUM reviewer. The prior 152fe913 verdict is historical; this comment binds the review to the current head.

Pass 1 independently re-read the entire eight-file PR delta against the new parent and retraced configured account -> public AWS factory -> scoped collector -> every DAILY Cost Explorer page -> strict daily periods/duplicate rejection -> contiguous-prefix baseline. Pass 2 attacked rebase integration assumptions, dependency/base changes, legacy behavior, chronology/count/age boundaries, reuse, scope, security and unnecessary machinery. No new implementation defect was found.

Independent rebind proof:

  • The complete original and rebased AWS patch bytes compare equal, and git range-diff reports 152fe91 = 1bf6d0e.
  • Every AWS file is unchanged: both revisions have AWS subtree 7a8d0c8cb2ff4e481f5fb653258f203cde3d77a8. Every shared package file is unchanged: both have pkg subtree c6d44b41055cd555ec344de8950397658222ffea.
  • The complete intervening parent delta has exactly eleven changed paths, all under providers/gcp/. There is no AWS/shared dependency or implementation change hidden by the rebase. Original reviewed commit remains preserved at backup/go413-reviewed-152fe913.
  • Source/index were clean at the candidate inspection; expected committed head/tree/parent and current PR head were checked directly.

Checked implementation remains providers/aws/ladder/factory.go:89 and adapters.go:224 for the configured account; providers/aws/recommendations/ondemand_series.go:118, :126, :131, :148, :175, :188, :191 and :246 for daily request/filter/page/period/duplicate behavior; providers/aws/ladder/baseline.go:89, :146, :165 and :197 for sample/count/prefix/age guards; and providers/aws/recommendations/ondemand_series.go:101, :276 and :322 for legacy unscoped parsing/sorting/zero/error behavior. The unchanged SDK v1.61.0 and AWS GetCostAndUsage, GetDimensionValues, ResultByTime and MetricValue contracts remain the source of truth, as recorded in the original detailed evidence.

Fresh proof at this revision:

  • Independently inspected the author's exact-candidate receipt and raw terminal logs: normal hooks passed; standalone AWS go build ./... exited 0; go test -race -short -count=1 -run '^(TestScopedBaselineParentCompatible|TestGetOnDemandSeries.*)$' ./ladder ./recommendations passed on macOS. These are freshly inspected author runs, not reviewer-authored runs.
  • After confirmed normal lease publication, I independently created a new external module initially requiring only AWS 1bf6d0e.... Preserved initial go.mod has no shared pin, no replace and no workspace. With GOTOOLCHAIN=go1.26.9 GOWORK=off, normal go mod tidy naturally selected AWS v0.0.0-20261015095956-1bf6d0e4a54c and shared v0.0.0-20261015071854-5b254484d5e4; go list -m -json confirmed published module provenance without replacements.
  • That new published consumer passed go test -race -count=1 ./...: public factory SDK-loopback account/pagination/period/gap/lag/zero/error cases plus independently written direct legacy/scoped collector checks, including every filter on every page. Published changed production files plus AWS go.mod/go.sum matched the exact candidate byte-for-byte.
  • The original independently compiled parent fail-before proof, seven killed and restored scratch mutations, and affected-package race suite still apply to the identical AWS/pkg bytes. Those unchanged probes were not redundantly rerun.

All fresh runs disabled ambient credentials, used synthetic credentials and constructed CE-shaped SDK-loopback responses, and serialized through retained-inode shared runner locks. This is local fixture integration evidence, not live AWS economic acceptance. No purchases, deployments or cloud calls were performed. CI/CR completion and the final normal merge gate remain separate checks. No new follow-up defect was found.

@coderabbitai

coderabbitai Bot commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor
⚠️ Action not completed

Already reviewed the last commit. Use @coderabbitai full review to rerun a review of the entire changeset.

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

cristim commented Oct 15, 2026

Copy link
Copy Markdown
Member Author

Final gate evidence for head 1bf6d0e4a54c7aaf7e72c37ff1069cf024e2c69c, tree 5022c3249df61174064eecda03077cb36289b98d, base c2a461a801e3541860893f62a05e825364ac74a2.

The renewed independent two-pass MEDIUM review found no actionable findings at this exact head. It independently verified identical AWS/shared bytes across the rebase and a fresh published AWS-only consumer with natural shared-module selection, without replacements or an initial shared pin. Native macOS build/race, actual-parent fail-before proof and seven killed/restored scratch mutations are recorded in the linked evidence. SDK responses are constructed loopback fixtures, not live billing observations.

The completed CodeRabbit review explicitly covers the current head and all eight changed files, with no actionable comments. There are no inline review threads. The generated docstring-coverage warning is advisory: the new public scoped collector is documented; concise internal helpers and test functions follow the repository's comment policy.

CI history is retained rather than described as uniformly green. The initial AWS lint failure was a schema-download timeout before code analysis; its bounded same-head retry passed. A separately carried integration failure read an HTTP 200 loopback response through a closed socket. Independent source tracing found no configured timeout, premature cleanup or shared transport mutation. Native 100 full-fixture race runs, 1,000 account race/coverage runs, the full affected package integration command, and a scratch 300ms delayed-body probe passed. The socket-closing actor remains unknown. A successful bounded Linux retry establishes nonrecurrence, not a proven root cause.

Both workflows now have actual terminal SUCCESS at the unchanged reviewed head: CI - Build & Test, attempt 3 and pre-commit. The bounded integration retry and dependent CI Success gate succeeded; earlier successful jobs retained their actual successful conclusions. The failure did not recur on that Linux retry, and its root cause remains unknown. Final server inspection reports CLEAN/MERGEABLE, all checks SUCCESS, unchanged head/base, and no review threads.

This is the scoped DAILY residual-observation prerequisite. Total-demand reconstruction, historical E/k unit provenance and compatible platform/executor adoption remain tracked by #282 and #349. This PR does not complete either issue or enable purchases.

@cristim
cristim merged commit 8b27158 into main Oct 15, 2026
32 of 37 checks passed
@cristim

cristim commented Oct 15, 2026

Copy link
Copy Markdown
Member Author

Merged #413 normally as 8b27158582fe0034c1b0800522579d9f7fde785e.

Fresh published-main verification passed on macOS Go1.26.9: a new external module initially required only AWS v0.0.0-20261015103505-8b27158582fe; normal module resolution selected shared v0.0.0-20261015071854-5b254484d5e4, without a replacement, workspace or explicit initial shared pin. The unchanged independently reviewed public-factory and direct collector consumers passed a fresh race run. All 47 published AWS production/module files match exact merged main; the AWS/shared source equals reviewed 1bf6d0e. SDK responses remain constructed loopback fixtures, not live billing observations.

Post-merge status is not all green: pre-commit passed, but main CI failed its unit job with the recurring HTTP 200 body-read socket error, this time in TestScopedBaselineParentCompatible/lag2. #414 is active main-CI repair. Retries stopped; connection-lifetime instrumentation is being reviewed. The closing actor remains unknown, and cleanup is held.

This merged leaf supplies scoped DAILY residual observations, not reconstructed total demand. Historical E/k units and cohort rates, total-demand observations, persisted tranche rates, compatible platform pin/in-flight accounting and executor adoption remain open under #282 and #349. No real purchases, deployments or runtime enablement were performed.

Follow-up issues filed: #414 for the observed recurring Linux fixture failure. The remaining ladder work stays explicitly tracked by #282 and #349; neither parent issue is complete.

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

Labels

effort/m Days impact/few Limited audience priority/p2 Backlog-worthy 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