Repository navigation
fix(azure): invalidate stale recommendation prices before purchase - #655
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (11)
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. 📝 WalkthroughWalkthroughMigration 106 clears stored Azure cost fields. The recommendation store can check migration readiness. Before purchase execution, the manager compares Azure recommendation prices with stored values and refuses stale prices. The API maps stale-pricing errors to HTTP 409. ChangesAzure pricing validation
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant executeAndFinalize
participant staleAzurePricingRefusal
participant ListStoredRecommendations
participant PurchaseExecution
executeAndFinalize->>staleAzurePricingRefusal: check Azure recommendation prices
staleAzurePricingRefusal->>ListStoredRecommendations: load stored Azure recommendations
ListStoredRecommendations-->>staleAzurePricingRefusal: return stored recommendations
staleAzurePricingRefusal-->>executeAndFinalize: return validation result
executeAndFinalize->>PurchaseExecution: execute when validation succeeds
Merge Risk: ⚪ Minimal · up to The change invalidates stale Azure prices and refuses purchases when approved costs differ from the current recommendation, returning HTTP 409. No actionable merge-blocking risk is evident from the supplied context. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Add Carrot credits or activate Agent usage billing to use Autopilot |
|
Current head: 1b15d74. The lint follow-up replaced large range-value copies with indexed reads and removed an unused test initializer. Pinned golangci-lint v2.10.1, Go 1.26.6 build, purchase/API race-short tests and normal commit hooks passed. All CI jobs and the pre-commit workflow passed on this head. CodeRabbit's completed walkthrough covers all 12 changed files from 3131779 to this head and reports no actionable comments; the inline review-comment list is empty. Its advisory docstring-coverage warning is declined: boilerplate comments on private pricing helpers and regression tests would not clarify their behavior. No review threads were resolved or suppressed. The later Autopilot billing prompt is not a review finding; no paid action was attempted. The published follow-up is LeanerCloud/cloud-commitments-mcp#44. The rollout/history correction is recorded on #612. This remains a draft. Local verification is mock/fixture based. The required real Azure scenario and independent exact claude-opus-5-5 reviewed-SHA verdict remain pending. No merge, deployment or purchase has been performed. |
|
Independent final-head review: 1b15d74, base 3131779. A fresh Codex reviewer inspected all 12 changed files through the live GitHub diff and local source. No actionable findings across completeness, correctness, security, bugs, duplication, over-engineering, reuse and scope. The reviewer traced approval/direct/scheduled/redrive execution, scaled pricing, nil versus zero, failed-status persistence, audit-error precedence, spend caps, migration and recollection. Verification is local fixture/mock evidence, permitted by the owner's latest merge-gate override. Earlier fail-first checks exercise the manager's actual provider-dispatch path; unchanged migration SQL has successful up/reapply/down evidence. Fresh build passed. Fresh full suite passed: Go 1.26.6, GOMAXPROCS=2, go test -race -short -count=1 -p=1 ./..., exit 0. Three unrelated timing failures in the initial parallel run passed unchanged in a focused controlled-load run and this full rerun. The reviewer's own runtime attempts stopped before assertions with transient Go cache/standard-library lookup errors, so those attempts are not counted as behavioral proof. Fresh migration up106 and recommendation upsert passed; the rollback repeat hit a Docker read-only-filesystem error. Earlier rollback proof uses identical migration files. No live Azure account acceptance, purchase or deployment is claimed. Deployment still requires the fixed collector rollout through #475 and quiescing old collectors before migration. CodeRabbit completed normally on this head with zero actionable comments, so no CR quota waiver is used. The removed exact-model and mandatory live-account gates are not merge blockers. Main-merge sequencing is with the coordinator: active push-to-main deployment workflows conflict with the retained no-deployment instruction. Merge must use an authorized path respecting that constraint, followed by a fresh unchanged-head, green-CI and mergeability check. No deployment workflow or repository setting has been changed by this worker. |
Refuse Azure execution while migration 106 is missing or dirty, so a failed or skipped migration cannot leave matching old prices purchasable. Preserve database failures and failed-execution audit handling. Real PostgreSQL approval fixtures fail on the previous head with one mocked provider call and pass after the readiness gate and recollection.
4f8b96b to
6464a13
Compare
|
Independent adversarial review: no actionable findings at The reviewer independently replayed Main subsequently advanced to CodeRabbit is optional under the project review gate and the owner's current instruction; skipped or throttled bot checks are not counted as independent review evidence. Merge remains conditional on fresh green exact-head CI, unchanged SHA, clean mergeability and normal branch protections. Existing collectors must be quiesced during rollout before migration 106; no live cloud purchase was performed. |
|
Post-merge verification at The AWS Lambda (37755443364), Azure Container Apps (37755443511), and GCP Cloud Run (37755443305) development deployment runs remain pending. Their preparation jobs passed; build/deploy jobs have no assigned runner. No pending environment approval request was returned. Read-only workflow watchers remain active. Deployment success and deployed purchase behavior are not claimed. The actual merged tree equals the independently reviewed combined tree; the real PostgreSQL approval regression and affected race suites passed before merge. No cloud purchase was made. Existing #612 retains the historical execution limitation, and MCP #44 retains its separate preview follow-up. No new follow-up issue was needed for this fix. |
Azure recommendation costs collected before the upstream units fix can understate purchase spend caps. This change invalidates stored Azure pricing and refuses a purchase when approved costs differ from the current recommendation. Azure purchases also require migration 106 to be cleanly applied: a failed migration cannot leave positive same-ID legacy prices eligible for purchase.
Refusals persist a failed execution and return HTTP 409 through the approval/direct API paths. Storage errors and audit-save failures remain server errors.
Rollout
Validation at
6464a133e3bddd9c6d520e775fda966dbcae60e7go test -race -short -count=1 -p=1 ./internal/config ./internal/purchase ./internal/api; pinned golangci-lint reported zero issues. Normal commit hooks passed.Sibling audit
The CLI CSV importer ignores upfront/monthly price columns and uses AWS-only purchase clients; the proposed stale Azure CSV replay defect was not reproduced. The MCP known-zero upfront preview follow-up is tracked in LeanerCloud/cloud-commitments-mcp#44.
Refs #612