Skip to content

ARCH-04 followup: SP calculateHoursInTerm and normalizeTermString still have silent 1yr defaults #24

Closed

Description

@cristim

Problem

PR LeanerCloud/cloud-commitments-cli#1085 fixed convertTermToSeconds in the Savings Plans client to fail loud on unrecognized term strings, and PR LeanerCloud/cloud-commitments-cli#1207 extended the same pattern to ElastiCache / MemoryDB / OpenSearch / RDS / Redshift. Two helpers in the SP client retain the old silent-default pattern:

// providers/aws/services/savingsplans/client.go:522-529
func calculateHoursInTerm(term string) float64 {
    if term == "3yr" || term == "3" {
        return 3 * 365 * 24 // 3 years (26280 hours)
    }
    return 365 * 24 // 1 year - silent default
}

// providers/aws/services/savingsplans/client.go:545-551
func normalizeTermString(term string) string {
    if term == "3yr" || term == "3" {
        return "3yr"
    }
    return "1yr" // silent default
}

Both are called from GetOfferingDetails (lines 491, 498), after findOfferingID has already validated the term via convertTermToSeconds, so the current call graph protects them. That makes this a defensive-pattern issue rather than an active money bug.

feedback_no_silent_fallbacks.md (the project memory rule from PR review history) treats silent money-affecting defaults as defects regardless of upstream protection: any new caller, or a refactor that drops the upstream validation, would silently mis-cost or mis-label the offering details (UpfrontCost / RecurringCost / Term in OfferingDetails).

Fix

Mirror the convertTermToSeconds pattern: both helpers return (value, error) on unrecognized input, callers propagate. Same error message style as the other AWS clients: unsupported Savings Plans term %q: must be one of 1yr, 1, 3yr, 3.

Optionally collapse to a single internal termInYears(term string) (int, error) and derive both hours and the canonical "Nyr" form from it, removing the duplicated "3yr" / "3" switch.

Why this is lower priority

Currently unreachable on the production call graph - GetOfferingDetails always calls findOfferingID first, which fails loud. The bug only materializes if a future caller wires calculateHoursInTerm / normalizeTermString directly, or findOfferingID is refactored. Filing as p3 / effort/xs.

Refs: LeanerCloud/cloud-commitments-cli#1085 (SP convertTermToSeconds fix), LeanerCloud/cloud-commitments-cli#1207 (five-client extension), feedback_no_silent_fallbacks.md.

Activity

  1. self-assigned this
    on Oct 16, 2026
  2. cristim commented on Oct 16, 2026

    @cristim
    MemberAuthor

    Claiming: working on this.

  3. added a commit that references this issue on Oct 16, 2026
    7b02f1c
  4. added a commit that references this issue on Oct 16, 2026
    1771442
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions