fix(azure): coerce web partial-upfront to monthly, not upfront (#1503) - #1504
Conversation
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAzure ChangesPayment adjustment disclosure
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
mcp/tools/aws_rds_ri.go (1)
154-162: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated dry_run/confirm defaulting.
Same identical defaulting block as in
aws_ec2_ri.go,aws_elasticache_ri.go,aws_savingsplans.go, andaws_simple_ri.go; candidate for a shared helper (see consolidated comment).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcp/tools/aws_rds_ri.go` around lines 154 - 162, Extract the duplicated dryRun and confirm defaulting logic from the current function into a shared helper, then reuse that helper here and in the corresponding AWS RI and savings-plan tools. Preserve the defaults of dryRun=true and confirm=false, while allowing non-nil args.DryRun and args.Confirm values to override them.mcp/tools/aws_elasticache_ri.go (1)
147-155: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated dry_run/confirm defaulting.
This 6-line block is identical to the same defaulting logic in
aws_ec2_ri.go,aws_rds_ri.go,aws_savingsplans.go, andaws_simple_ri.go. Worth extracting into a single shared helper (see consolidated comment).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcp/tools/aws_elasticache_ri.go` around lines 147 - 155, Extract the duplicated dryRun and confirm defaulting logic from the current function into a shared helper reusable by aws_ec2_ri.go, aws_rds_ri.go, aws_savingsplans.go, and aws_simple_ri.go. Update each caller, including the current Elasticache RI path, to use the helper while preserving the existing defaults and pointer override behavior.mcp/tools/aws_simple_ri.go (1)
186-194: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated dry_run/confirm defaulting.
Same identical defaulting block as in
aws_ec2_ri.go,aws_elasticache_ri.go,aws_rds_ri.go, andaws_savingsplans.go; candidate for a shared helper (see consolidated comment).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcp/tools/aws_simple_ri.go` around lines 186 - 194, The dryRun and confirm defaulting logic at the end of the relevant tool flow duplicates the same behavior across aws_ec2_ri.go, aws_elasticache_ri.go, aws_rds_ri.go, and aws_savingsplans.go. Extract or reuse a shared helper for applying these optional argument defaults, then update this flow and the other identified tools to call it while preserving true/false defaults and explicit argument overrides.mcp/tools/aws_ec2_ri.go (2)
107-158: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the dry_run/confirm defaulting into a shared helper.
effectiveDryRunConfirmhere is the same 3-branch defaulting logic (dryRun=true,confirm=false, overridden by non-nil pointers) that is copy-pasted inline inaws_elasticache_ri.go,aws_rds_ri.go,aws_savingsplans.go, andaws_simple_ri.go. Since this file already took the step of extracting it into a named function, it's the natural place to generalize it (e.g. take*bool, *boolinstead of the whole args struct) so all 5 tools call one shared helper instead of maintaining 5 copies.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcp/tools/aws_ec2_ri.go` around lines 107 - 158, Generalize effectiveDryRunConfirm into a shared helper that accepts the dry-run and confirm boolean pointers, applies dryRun=true and confirm=false defaults, and preserves non-nil overrides. Update effectiveDryRunConfirm and the callers in aws_elasticache_ri.go, aws_rds_ri.go, aws_savingsplans.go, and aws_simple_ri.go so all five tools use the single helper without duplicating the defaulting logic.
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the dry_run/confirm defaulting boilerplate into one shared helper. All five AWS purchase tools duplicate the identical logic for resolving
dryRun/confirmfrom optional*boolargs (defaulttrue/false, overridden only when the caller's pointer is non-nil). One shared helper eliminates five copies and guarantees the default behavior can't silently drift between tools as they evolve.
mcp/tools/aws_ec2_ri.go#L145-158: generalizeeffectiveDryRunConfirmto take(dryRun, confirm *bool) (bool, bool)instead of the wholeec2RIPurchaseArgs, and have this file call the generalized version.mcp/tools/aws_elasticache_ri.go#L147-155: replace the inlinedryRun, confirm = true, false; if args.DryRun != nil {...}; if args.Confirm != nil {...}block with a call to the shared helper.mcp/tools/aws_rds_ri.go#L154-162: replace the same inline block with a call to the shared helper.mcp/tools/aws_savingsplans.go#L193-201: replace the same inline block with a call to the shared helper.mcp/tools/aws_simple_ri.go#L186-194: replace the same inline block with a call to the shared helper.♻️ Proposed shared helper
// resolveDryRunConfirm applies the dry_run=true / confirm=false defaults: // bool zero values cannot distinguish "caller omitted the field" from // "caller explicitly set it false", so both flags are pointers and this is // the single place that resolves them to concrete booleans. func resolveDryRunConfirm(dryRun, confirm *bool) (bool, bool) { d := true if dryRun != nil { d = *dryRun } c := false if confirm != nil { c = *confirm } return d, c }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcp/tools/aws_ec2_ri.go` at line 1, Extract the duplicated dryRun/confirm default resolution into a shared resolveDryRunConfirm helper accepting (*bool, *bool) and returning (bool, bool), preserving defaults of true and false and non-nil overrides. Update effectiveDryRunConfirm in the EC2 purchase flow to use the generalized helper, then replace the inline defaulting blocks in the ElastiCache, RDS, Savings Plans, and Simple RI purchase tools with calls to resolveDryRunConfirm.mcp/tools/aws_savingsplans.go (1)
193-201: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated dry_run/confirm defaulting.
Same identical defaulting block as in
aws_ec2_ri.go,aws_elasticache_ri.go,aws_rds_ri.go, andaws_simple_ri.go; candidate for a shared helper (see consolidated comment).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcp/tools/aws_savingsplans.go` around lines 193 - 201, Extract the duplicated dryRun and confirm defaulting logic from the current function into a shared helper, reusing the same helper across aws_ec2_ri.go, aws_elasticache_ri.go, aws_rds_ri.go, aws_simple_ri.go, and this Savings Plans flow. Preserve the defaults of true for dryRun and false for confirm, while allowing non-nil argument values to override them.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@mcp/server_test.go`:
- Around line 173-223: Update mcp/server_test.go lines 173-223 in
TestAzureComputeRIPurchaseSchemaExcludesPartialUpfront to verify Azure
advertises and accepts partial-upfront, and assert the resulting billing plan is
normalized to monthly/no-upfront rather than rejected. Update mcp/README.md line
107 to document this Azure partial-upfront normalization behavior. Rename or
replace the test to reflect the new contract.
In `@mcp/tools/search_recommendations.go`:
- Around line 128-145: Validate non-empty lookback_period in the search
recommendations handler before provider creation, accepting only 7d, 30d, or 60d
and returning an error for other values. Add a regression test in
mcp/tools/search_recommendations_test.go covering an invalid lookback value and
asserting the handler returns an error.
---
Nitpick comments:
In `@mcp/tools/aws_ec2_ri.go`:
- Around line 107-158: Generalize effectiveDryRunConfirm into a shared helper
that accepts the dry-run and confirm boolean pointers, applies dryRun=true and
confirm=false defaults, and preserves non-nil overrides. Update
effectiveDryRunConfirm and the callers in aws_elasticache_ri.go, aws_rds_ri.go,
aws_savingsplans.go, and aws_simple_ri.go so all five tools use the single
helper without duplicating the defaulting logic.
- Line 1: Extract the duplicated dryRun/confirm default resolution into a shared
resolveDryRunConfirm helper accepting (*bool, *bool) and returning (bool, bool),
preserving defaults of true and false and non-nil overrides. Update
effectiveDryRunConfirm in the EC2 purchase flow to use the generalized helper,
then replace the inline defaulting blocks in the ElastiCache, RDS, Savings
Plans, and Simple RI purchase tools with calls to resolveDryRunConfirm.
In `@mcp/tools/aws_elasticache_ri.go`:
- Around line 147-155: Extract the duplicated dryRun and confirm defaulting
logic from the current function into a shared helper reusable by aws_ec2_ri.go,
aws_rds_ri.go, aws_savingsplans.go, and aws_simple_ri.go. Update each caller,
including the current Elasticache RI path, to use the helper while preserving
the existing defaults and pointer override behavior.
In `@mcp/tools/aws_rds_ri.go`:
- Around line 154-162: Extract the duplicated dryRun and confirm defaulting
logic from the current function into a shared helper, then reuse that helper
here and in the corresponding AWS RI and savings-plan tools. Preserve the
defaults of dryRun=true and confirm=false, while allowing non-nil args.DryRun
and args.Confirm values to override them.
In `@mcp/tools/aws_savingsplans.go`:
- Around line 193-201: Extract the duplicated dryRun and confirm defaulting
logic from the current function into a shared helper, reusing the same helper
across aws_ec2_ri.go, aws_elasticache_ri.go, aws_rds_ri.go, aws_simple_ri.go,
and this Savings Plans flow. Preserve the defaults of true for dryRun and false
for confirm, while allowing non-nil argument values to override them.
In `@mcp/tools/aws_simple_ri.go`:
- Around line 186-194: The dryRun and confirm defaulting logic at the end of the
relevant tool flow duplicates the same behavior across aws_ec2_ri.go,
aws_elasticache_ri.go, aws_rds_ri.go, and aws_savingsplans.go. Extract or reuse
a shared helper for applying these optional argument defaults, then update this
flow and the other identified tools to call it while preserving true/false
defaults and explicit argument overrides.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 47216eb8-9e44-466d-9e0e-e306012bd3bc
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumgo.work.sumis excluded by!**/*.sum
📒 Files selected for processing (41)
README.mdcmd/cudly-mcp/main.gocmd/cudly-mcp/main_test.gogo.modinternal/api/handler_purchases_guards_test.gointernal/config/validation.gointernal/config/validation_test.gomcp/README.mdmcp/server.gomcp/server_test.gomcp/tools/aws_ec2_ri.gomcp/tools/aws_ec2_ri_test.gomcp/tools/aws_elasticache_ri.gomcp/tools/aws_elasticache_ri_test.gomcp/tools/aws_rds_ri.gomcp/tools/aws_rds_ri_test.gomcp/tools/aws_savingsplans.gomcp/tools/aws_savingsplans_test.gomcp/tools/aws_simple_ri.gomcp/tools/aws_simple_ri_test.gomcp/tools/azure_compute_ri.gomcp/tools/azure_compute_ri_test.gomcp/tools/enums.gomcp/tools/enums_test.gomcp/tools/gcp_computeengine_cud.gomcp/tools/gcp_computeengine_cud_test.gomcp/tools/list_commitment_actions.gomcp/tools/list_commitment_actions_test.gomcp/tools/purchase.gomcp/tools/purchase_test.gomcp/tools/registry.gomcp/tools/schema.gomcp/tools/schema_test.gomcp/tools/search_recommendations.gomcp/tools/search_recommendations_test.gopkg/common/types.gopkg/common/types_test.goproviders/azure/services/compute/client.goproviders/azure/services/compute/client_test.goproviders/azure/services/internal/reservations/purchase.goproviders/azure/services/internal/reservations/purchase_test.go
303dde8 to
820ecb4
Compare
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/config/validation.go`:
- Around line 114-118: Update the caller of validatePurchaseRecommendation to
log a warning containing the raw and canonical payment values whenever
normalization changes rec.Payment, before overwriting it; otherwise remove or
revise the validation comment’s promise that callers emit this warning.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7dea9abd-669d-40b7-9c9c-b8ec1d6638d7
📒 Files selected for processing (3)
internal/api/handler_purchases_guards_test.gointernal/config/validation.gointernal/config/validation_test.go
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
14ae1b1 to
fbf05f9
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
The coercion-rationale comments assert the normalized payment option is "not a billingPlan request field". That is accurate on main today (the reservation purchase body sends no billingPlan), but becomes false once the billingPlan wiring lands (#1495/#1502), and that wiring does not touch this file, so the comment would silently go stale on a money-path rationale. Rephrase to state the forward-compatible fact instead: the wiring maps only upfront/monthly tokens and hard-errors on partial-upfront, which is exactly why normalizing to monthly here keeps web-path Azure purchases valid. Comment-only change; no behavior difference.
The web execute path silently normalizes cross-provider payment tokens (most notably Azure partial-upfront -> monthly, #1503); until now only operators saw the WARN log while the caller's response gave no hint the billing schedule they requested was changed. Return the coercion to the caller: validatePurchaseRecommendation now also yields a PaymentAdjustment (rec index, provider, service, requested token, applied token, reason) whenever normalization changes the token, and executePurchase attaches the collected list to all three response bodies (approval-pending, duplicate-collapse, direct-execute) under payment_adjustments. The key is omitted entirely when every option was already canonical, so existing clients are unaffected on the common case, and the coercion policy itself (config.NormalizePaymentOption) plus the operator WARN log are unchanged. Tests: the guards table now cross-checks every coerced row surfaces a matching adjustment and every canonical row surfaces none; new field-level unit test for the Azure partial-upfront case; new handler-level tests assert the response body carries the adjustment for a mixed canonical+coerced batch (verified to fail with the attach stubbed out) and omits the key for a canonical-only batch, both with mock.AssertExpectations registered. frontend PurchaseResult gains the matching optional payment_adjustments field.
The Go side of #1503 was fixed to map Azure partial-upfront onto "monthly", but normalizePaymentValue in the frontend still mapped it onto "upfront", so the two halves of the web path disagreed. That function decides which option the plan and purchase dropdowns pre-select (plans.ts:1166 on plan edit, plans.ts:1562 when prefilling from a selected commitment), so a rec carrying the legacy partial-upfront token pre-selected "Pay Upfront". Saving from that state submits an already-canonical "upfront", which the backend accepts verbatim, so the backend coercion never gets a chance to run and the user is committed to a full upfront charge they never chose. Azure offers exactly two billing plans and the total cost is identical either way, but the plan cannot be changed after purchase, so landing on monthly is the only direction that cannot surprise the user with an irreversible upfront charge. The jest case that pinned the old expectation is updated and now documents why monthly is required, mirroring the Go-side assertion in TestNormalizePaymentOption. Verified failing before this change and passing after. Refs #1503
The doc comments justifying the partial-upfront to monthly coercion cited providers/azure/services/compute/client.go's GetOfferingDetails as the consumer that "drives the upfront-vs-monthly cost split". GetOfferingDetails has no non-test callers anywhere in the repo; it exists only to satisfy the ServiceClient interface, so the stated mechanism does not run. On a money path a wrong rationale is worse than none, since the next reader reasons from it. Replace it with what actually consumes the canonical token today (persisted on the execution, then copied into common.Recommendation.PaymentOption in internal/purchase/execution.go) and the billingPlan wiring arriving in #1495/#1502. Also record the two Microsoft-documented facts the decision rests on: upfront and monthly cost the same total, and the billing frequency cannot be changed after purchase. Note the documented exception that monthly is unavailable for SUSE Linux, Red Hat, Azure Red Hat OpenShift and pre-purchase plans, where the coerced token makes Azure reject the purchase; that loud rejection is the intended outcome. Adds the cross-reference to the frontend mirror of this mapping so the two stay in lockstep. Comments only, no behaviour change. Refs #1503
The backend already returns `payment_adjustments` when it normalizes a requested payment option onto a different provider-canonical token (Azure has exactly two billing plans, Upfront and Monthly, so an inherited AWS-style `partial-upfront` has nowhere to land and is coerced to monthly). The field was declared in `frontend/src/api/types.ts` and read by nothing, so the coercion was disclosed to API clients and to the operator WARN log but never to the web user whose billing schedule actually changed. Coercing instead of rejecting is only defensible if the user is told, so this closes that gap: - Add `formatPaymentAdjustmentNotice` next to `normalizePaymentValue`, the mapping it discloses. Reuses the existing `getPaymentLabel` so the copy shows "Partial Upfront"/"Pay Monthly" rather than raw API tokens, and collapses a batch to its distinct requested -> applied pairs. - Render it as a separate non-expiring warning toast on both purchase submit paths (single and fan-out) so an irreversible billing-schedule change cannot scroll away inside a success message. The fan-out path collects from every fulfilled response, including buckets whose approval email failed, since those still created a pending execution carrying the coerced schedule. - Extract the inline `payment_adjustments` element type into a named `PaymentAdjustment` interface so the formatter is typed against the same shape the API returns. Regression coverage: the two disclosure tests in purchase-execution-toast.test.ts fail with the wiring removed and pass with it, exercising the real handler through to the rendered toast; a formatter-only unit test could not prove the notice reaches the user.
The #1503 disclosure fired on every raw != canonical rewrite, including the cross-provider renames that leave the customer's cash flow untouched. Azure spells AWS's all-upfront "upfront" and AWS's no-upfront "monthly", so both of those rewrites are bookkeeping, not a billing change. That mattered on the ordinary path, not an edge case: the fan-out purchase modal builds its per-bucket Payment dropdown from paymentOptionsFor (frontend/src/lib/purchase-compatibility.ts), whose candidate list has no "upfront" entry. An Azure bucket the user chooses to pay upfront therefore ALWAYS submits "all-upfront", so every ordinary Azure upfront purchase raised a sticky "Billing schedule adjusted" warning claiming Azure "does not offer All Upfront" and that the purchase was applied as something else. Both claims are false, and the noise trains users to dismiss the one notice that is real. Classify a payment token by the cash flow it implies (config.PaymentScheduleFor) and surface a PaymentAdjustment only when the rewrite crosses schedules (config.PaymentCoercionChangesSchedule). Azure partial-upfront -> monthly and GCP upfront -> monthly still disclose; Azure all-upfront -> upfront and no-upfront -> monthly no longer do. The operator WARN keeps firing on every rewrite: a non-canonical token on the wire is an upstream input bug worth auditing even when it costs the customer nothing. Only the user-facing notice is narrowed. Adjustment construction moves into paymentAdjustmentFor so validatePurchaseRecommendation stays at gocyclo 10 (the pre-commit threshold), not 11. Regression coverage: the three rename cases assert nil adjustment plus a still-emitted WARN, and the GCP upfront-shaped tokens assert a surfaced adjustment. Both new tests fail against the previous raw != canonical condition and pass after (verified by reverting the condition).
8e0c580 to
84a55ac
Compare
Rebased onto
|
|
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 49 seconds. |
|
Requesting a full review (not incremental) now that a window may be open. Triggers are serialised centrally across the queue because CodeRabbit is under an account-level adaptive throttle and parallel pings hold it in backoff. @coderabbitai full review |
|
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 42 minutes. |
|
Requesting a full review (not incremental) now that a window may be open. Triggers are serialised centrally across the queue because CodeRabbit is under an account-level adaptive throttle and parallel pings hold it in backoff. @coderabbitai full review |
|
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 27 minutes. |
|
Requesting a full review (not incremental) now that a window may be open. Triggers are serialised centrally across the queue because CodeRabbit is under an account-level adaptive throttle and parallel pings hold it in backoff. @coderabbitai full review |
|
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 11 minutes. |
|
Requesting a full review (not incremental) now that a window may be open. Triggers are serialised centrally across the queue because CodeRabbit is under an account-level adaptive throttle and parallel pings hold it in backoff. @coderabbitai full review |
|
✅ Action performedFull review finished. |
|
Requesting a full review (not incremental) now that a window may be open. Triggers are serialised centrally across the queue because CodeRabbit is under an account-level adaptive throttle and parallel pings hold it in backoff. @coderabbitai full review |
|
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 48 minutes. |
|
Requesting a full review (not incremental) now that a window may be open. Triggers are serialised centrally across the queue because CodeRabbit is under an account-level adaptive throttle and parallel pings hold it in backoff. @coderabbitai full review |
|
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 38 minutes. |
|
Requesting a full review (not incremental) now that a window may be open. Triggers are serialised centrally across the queue because CodeRabbit is under an account-level adaptive throttle and parallel pings hold it in backoff. @coderabbitai full review |
|
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 32 minutes. |
|
Requesting a full review (not incremental) now that a window may be open. Triggers are serialised centrally across the queue because CodeRabbit is under an account-level adaptive throttle and parallel pings hold it in backoff. @coderabbitai full review |
|
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 19 minutes. |
|
Requesting a full review (not incremental) now that a window may be open. Triggers are serialised centrally across the queue because CodeRabbit is under an account-level adaptive throttle and parallel pings hold it in backoff. @coderabbitai full review |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/api/handler_purchases_test.go (1)
4113-4113: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the direct-execute
payment_adjustmentscasedirectExecutePurchasealready forwardspaymentAdjustmentsintowithPaymentAdjustments(...), but the direct-execute tests still only exercise thenilpath. Add one case with a populated slice so this response shape is covered on the direct branch too.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/api/handler_purchases_test.go` at line 4113, Add a direct-execution test case around directExecutePurchase that supplies a populated paymentAdjustments slice, ensuring the forwarded value reaches withPaymentAdjustments and the resulting response shape is asserted. Retain the existing nil-path coverage and follow the surrounding direct-execute test setup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/api/handler_purchases_test.go`:
- Line 4113: Add a direct-execution test case around directExecutePurchase that
supplies a populated paymentAdjustments slice, ensuring the forwarded value
reaches withPaymentAdjustments and the resulting response shape is asserted.
Retain the existing nil-path coverage and follow the surrounding direct-execute
test setup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e5be3d93-60db-4e6f-bcf7-2b86a2034852
📒 Files selected for processing (12)
frontend/src/__tests__/commitmentOptions.test.tsfrontend/src/__tests__/purchase-execution-toast.test.tsfrontend/src/api/types.tsfrontend/src/app.tsfrontend/src/commitmentOptions.tsinternal/api/executed_notification_flow_test.gointernal/api/handler_purchases.gointernal/api/handler_purchases_guards_test.gointernal/api/handler_purchases_test.gointernal/api/validation.gointernal/config/validation.gointernal/config/validation_test.go
Summary
internal/api/validation.go->config.NormalizePaymentOption->internal/config/validation.go:crossProviderPaymentAlias) coerced Azurepartial-upfronttoupfront. On currentmainthe normalized token drives the Azure upfront-vs-monthly cost split (GetOfferingDetails) and the persisted rec token; once feat(mcp): CUDly MCP server for RI/SP/CUD purchases across AWS, Azure, GCP #1495's AzurebillingPlanwiring lands it also selects the actual reservation billing plan, so the old coercion silently billed an all-upfront schedule the caller never chose.partial-upfrontmapping tomonthly(no-upfront, CUDly's default Azure schedule) instead ofupfront. The rec still survives validation, but lands on the default schedule rather than silently flipping to upfront. Azure charges the same total for both schedules, so this is a cash-flow-timing fix, not an overcharge fix.partial-upfront -> monthly) and AWS (no coercion; AWS supportspartial-upfrontnatively) are unchanged, verified by the existing/updated regression tests.monthly(consistent with the GCP precedent and the CUDly-wide no-upfront default) rather than failing loud.Rebase status: originally stacked on #1495 (
docs/mcp-server-design); since rebased ontomain, and the PR base ismain. No longer blocked on anything: it is fully effective onmaintoday, and once #1495'sbillingPlanwiring lands (that mapping hard-errors onpartial-upfront), this normalization is also what keeps web-path Azurepartial-upfrontpurchases valid.Closes #1503
Test plan
crossProviderPaymentAlias("azure", "partial-upfront")now returnsmonthly(wasupfront); confirmed the updated test fails against the pre-change code and passes post-change.upfrontandno-upfront/monthlymappings unchanged.monthly).internal/api/handler_purchases_guards_test.go's Azure legacy-partial-upfrontcase to assert the coerced value ismonthly, notupfront.go build ./...,go vet ./...clean.go test ./internal/config/... ./internal/api/... ./internal/scheduler/... ./providers/azure/...: 3363 passed, 0 failed.golangci-lintv2.10.1 (pinned, matching CI'sci.yml) run at repo root: 0 issues.gocyclo -over 10 internal/config/: clean.Summary by CodeRabbit
partial-upfrontnow maps tomonthly(notupfront);all-upfrontremainsupfront.