Skip to content

fix(aws/recommendations): parseRecommendations silently drops unparseable offers, so a search can present an incomplete menu as complete #54

Closed
Closed
fix(aws/recommendations): parseRecommendations silently drops unparseable offers, so a search can present an incomplete menu as complete#54
Labels
effort/mDaysimpact/manyAffects most userspriority/p1Next up; this sprintseverity/highSignificant harmtriagedItem has been triagedtype/bugDefecturgency/this-sprintWithin the current sprint

Description

@cristim
cristim
opened on Jul 28, 2026

Summary

parseRecommendations in providers/aws/recommendations/parser_ri.go:30 logs a
warning to stderr and continues when a single recommendation detail fails to
parse. The failed detail is dropped from the returned slice and no signal of
the drop reaches the caller. That is defensible graceful degradation for a
batch job, but it became load-bearing when cudly_search_recommendations
started promising a complete offer menu, so it should now fail loud (at least
for the MCP caller).

// providers/aws/recommendations/parser_ri.go
rec, err := c.parseRecommendationDetail(ctx, &details, params)
if err != nil {
    log.Printf("Warning: Failed to parse recommendation detail %d: %v", i, err)
    continue
}

Why the search fan-out made this load-bearing

mcp/tools/search_recommendations.go fans an AWS reservation search out over
every (term, payment option) combination when the caller omits term_years
and/or payment_option, because GetReservationPurchaseRecommendation returns
only one cell per request. fetchSearchCombos deliberately fails the WHOLE
search when any one combo's API call errors, and its own comment says why:

silently returning 5 of 6 offers is indistinguishable from "these are all
your options" and recreates the exact defect this fan-out exists to fix.

That guarantee only holds at the combo boundary. Inside a combo, the shared
parser silently drops individual offers, so the same defect the fan-out was
built to remove is reproduced one level down, where nothing checks for it.

Failure scenario

  1. A model calls cudly_search_recommendations with provider="aws",
    service="ec2" and no term_years / payment_option, so the tool issues
    all 6 combos.
  2. For the 3yr/all-upfront combo, Cost Explorer returns several recommendation
    details. The best one carries a quantity or cost field the parser cannot
    handle (unparseable numeric string, a field AWS newly leaves unset, an
    instance type the sizing path rejects).
  3. parseRecommendations logs to stderr and continues. The MCP client never
    sees stderr; the JSON-RPC response carries the remaining offers only.
  4. The tool returns success with a menu the model presents as complete. The
    model recommends the best SURVIVING offer, and the human buys it. The
    dropped offer may have been the cheapest one.

This is silent under-reporting on a purchase-decision path: the caller cannot
distinguish "AWS had nothing better" from "we could not parse the better one".
The same drop also reaches the scheduler's discovery sweep and the CLI's CSV
output, which is why the fix is not a one-line change here.

Why this is filed rather than fixed in PR LeanerCloud/cloud-commitments-cli#1495

parseRecommendations is shared. internal/scheduler's discovery sweep calls
into the same path and legitimately prefers partial progress over none in a
batch job, so flipping continue to a hard error changes behaviour for a
consumer that wants the current semantics. The fix needs a per-caller policy
(strict for the interactive/purchase-decision path, tolerant for the batch
sweep) or a returned skipped-count the MCP tool can refuse on, plus tests on
both sides. That is outside the scope of PR LeanerCloud/cloud-commitments-cli#1495, which only touches
mcp/tools.

Suggested direction (not prescriptive)

  • Have parseRecommendations report what it dropped (count plus per-detail
    reason) instead of only logging it.
  • Let the caller decide: cudly_search_recommendations refuses the search with
    an error naming the unparseable detail, matching fetchSearchCombos'
    existing all-or-nothing contract; the scheduler keeps tolerating and records
    the skipped count.
  • Regression test with a fixture whose detail genuinely fails
    parseRecommendationDetail, asserting the MCP search errors rather than
    returning a short menu, and that the scheduler path still returns the
    survivors.

References

Activity

  1. cristim
    added
    triagedItem has been triaged
    priority/p1Next up; this sprint
    severity/highSignificant harm
    urgency/this-sprintWithin the current sprint
    impact/manyAffects most users
    effort/mDays
    type/bugDefect
    on Jul 28, 2026
  2. cristim
    added a commit that references this issue on Jul 28, 2026

    fix(mcp): stop Azure subscription-ID case forking purchase identity

    707cd14
  3. cristim
    transferred this issue fromLeanerCloud/cloud-commitments-clion Sep 27, 2026
  4. cristim
    added 2 commits that reference this issue on Oct 2, 2026

    fix(aws): report incomplete recommendation collections (#169)

    006ef5c

    fix(search): reject incomplete AWS recommendation menus (#37)

    e073769

  5. cristim commented on Oct 3, 2026

    @cristim
    cristim
    on Oct 3, 2026
    MemberAuthor

    Refs LeanerCloud/cloud-commitments-mcp#37. The MCP rollout portion is merged at e073769ac91505956c5deab733208a264e215fde (2026-10-03), with reviewed head d04d15bb6fec29e99ee7a3a7d21a5ecdf3b44a4a.

    The merged tree is exactly 1d39ebab8f9e54713389d14e7c684e033da9201d, identical to the independently reviewed and locally verified head. A narrow additive fetch and full tree comparison found no byte differences, so the native macOS proof applies to the merged source.

    Independent gpt-6-astra review covered the full MCP PR and the published AWS producer contract. All eight registered MCP protocol cases passed at the final head; fresh parent-dependency and diagnostic-drop controls each failed the four malformed cases and passed the four valid/empty controls. Full race tests, actual command build, and frozen-database source/binary scans passed. These are connected synthetic-fixture checks, with no live cloud calls or purchases. The dependency repair preserves all four producer pins and selects the patched OTEL SDK closure.

    Both exact-merge-SHA workflows passed: pre-commit and CI - Build & Test. The final PR verification summary records the independent review and the original head's checks.

    This records the MCP rollout portion. Keep this issue open for the coordinated CLI and Platform rollout scope; their state is preserved by this update.

  6. cristim
    added a commit that references this issue on Oct 3, 2026

    fix(recommendations): retain incomplete AWS survivors (#2129)

    1ce4658

  7. cristim commented on Oct 3, 2026

    @cristim
    cristim
    on Oct 3, 2026
    MemberAuthor

    CLI rollout verified: cloud-commitments-cli PR 2129 merged as 1ce4658a0c29ca3f8bd289a428166883a0b0d1de on 2026-10-03 at 19:21:15 UTC.

    The additively fetched merge tree is exactly ac116220c0c1adaf74a230f9ec847c614d061fb6, identical to independently reviewed head 6abdb1469c04b0876bb983d22eb6af9ca92b4b20. Native Go 1.26.6 proof therefore applies unchanged: full race suite, 15 actual root-command/config-loader/published-SDK/CSV scenarios plus five helper cases, actual command build, and fixed source/binary vulnerability scans. Provider boundaries used synthetic fixtures; no live purchases or cloud execution are claimed. All published provider pins are preserved, including AWS v0.0.0-20261002152209-006ef5c8d0a2.

    Both exact merge-SHA workflows passed: pre-commit 37147583239 and CI 37147583241. Their watcher handles are terminal and reaped. Source checkout and existing branches were preserved.

    CLI rollout is complete. Platform rollout remains pending, so this shared issue stays open.

  8. cristim commented on Oct 7, 2026

    @cristim
    cristim
    on Oct 7, 2026
    MemberAuthor

    claimed by cc-go-w3

  9. cristim commented on Oct 7, 2026

    @cristim
    cristim
    on Oct 7, 2026
    MemberAuthor

    Worker-2 shard pickup: the cloud-commitments-go side of this is complete. PR #169 (merged 2026-10-02, exact 8d4be0d) makes parseRecommendations surface dropped details via IncompleteRecommendationsError with failed-detail/failed-scope counts, preserved through combo/service aggregation and the SDK adapter. Remaining consumer integration (MCP search refusal, scheduler tolerance + skipped count, CLI) lives in the cli/mcp/platform repos; in-progress worktrees exist for all three (go54-cli-consumer, go54-mcp-completeness, go54-platform-consumer) with commits but no open PRs yet. Leaving open until the consumers land.

  10. cristim
    added a commit that references this issue on Oct 10, 2026

    fix(aws/recommendations): fail the detail on an invalid average-insta…

    d9ead42

  11. cristim commented on Oct 10, 2026

    @cristim
    cristim
    on Oct 10, 2026
    MemberAuthor

    Closing as completed. Checked against current origin/main of each repo:

    • go: fix(aws): report incomplete recommendation collections #169 made parseRecommendations and the SP parser return survivors plus IncompleteRecommendationsError (failed details and scopes, per-detail causes). fix(aws/recommendations): fail the detail on an invalid average-instances or SP utilization #327 (d9ead42) closes the last silent-zero gap: a present-but-invalid RI AverageNumberOfInstancesUsedPerHour or SP EstimatedAverageUtilization now drops the detail and is reported, instead of becoming "no signal" (which left the rec unsized under --target-coverage and bypassed --min-pool-size via IncludesPoolSize avg<=0).
    • mcp: tools/search_recommendations.go fetchSearchCombos returns an error on any error from GetRecommendations, so an incomplete collection refuses the search. Test: tools/search_recommendations_completeness_test.go (result.IsError on incomplete responses).
    • cli: cmd/multi_service_helpers.go:112 and :476 handle the typed error. Test: cmd/recommendation_completeness_test.go.
    • platform: internal/scheduler/scheduler.go tolerateIncompleteSweep keeps the collected rows and withholds stale-row eviction, so a dropped rec leaves that pool's previous row in place and no new unsized row appears. Tests: internal/scheduler/partial_sweep_eviction_test.go.

    Note: consumers pick up the #327 behaviour (the avg/utilization change) when they bump their providers/aws pin; the typed-error handling they rely on is already in their pins. Not covered here, by decision: AttachDailyUsageHistory failures (display-only sparkline) and Count=0 recs from fractional quantities.

  12. cristim
    closed this as completedon Oct 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    effort/mDaysimpact/manyAffects most userspriority/p1Next up; this sprintseverity/highSignificant harmtriagedItem has been triagedtype/bugDefecturgency/this-sprintWithin the current sprint

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions