Repository navigation
ARCH-06: Term and PaymentOption stringly-typed; 175+ bare literals and 5+ divergent term parsers #12
Description
Severity: P2. Confidence: high.
Affected files:
pkg/common/types.go:153-154providers/gcp/services/computeengine/client.go:43-52providers/aws/services/savingsplans/client.go:376-385providers/azure/services/internal/reservations/purchase.go:62providers/azure/internal/recommendations/converter.go:329internal/scheduler/scheduler.go:1306
Evidence:
types.go declares Term string // 1yr, 3yr and PaymentOption string on the central Recommendation struct, while sibling fields (Provider, Service, CommitmentType) are typed string enums in the same file. Grep finds ~181 term/payment literal occurrences across non-test code. Term parsing is independently re-implemented with different vocabularies: GCP accepts "1yr"/"1"/"12mo", AWS savingsplans only "1yr"/"1", memorydb additionally "36", Azure has ParseTermYears plus a separate termToMonths, and the scheduler re-parses "3yr" -> 3 with a default-to-3 fallback.
Impact:
Exactly the bug class the memory garden documents (feedback_prefer_typed_enums): each parser's accepted set differs, so a term valid in one layer is silently mis-handled in another. Live silent-default pockets remain: memorydb falls back to "1yr" for any unrecognized term, Azure ParseTermYears maps empty term to a 1-year purchase, termToMonths defaults unknown terms to 12 months in cost math, and scheduler.go defaults invalid terms to 3 in ID derivation. 181 call sites make any vocabulary change a shotgun edit.
Recommendation:
Introduce common.Term and common.PaymentOption typed string enums with a single ParseTerm/ParsePaymentOption (error on unknown) in pkg/common, validate at boundaries (CSV, HTTP, DB read-back), and migrate provider switches to the typed value.
Verifier verdict: confirmed - existing mitigations (NormalizePaymentOption for LeanerCloud/cloud-commitments-cli#698, per-provider whitelists) are compensating shims that are themselves evidence for the debt; the class has bitten repeatedly (LeanerCloud/cloud-commitments-cli#698, PR LeanerCloud/cloud-commitments-cli#1047).
Source: docs/reviews/codebase-review-2026-06-10.md (automated multi-dimension code review, adversarially verified for P1/P2)
Findings from the 2026-09-02 codebase audit
Added by an automated audit of 3c0f8ac94048a2c36fce5ccddee54e6c4849a5cd (tip of origin/main). Each item below was reported by one reviewer and independently confirmed by a second that did not write it. Full report: docs/audits/codebase-audit-2026-09-02.md.
A09-019 (medium)
One more divergent term parser, and the only one whose failure mode is a recorded zero rather than a wrong default. termMonths (pkg/common/audit.go:84-99) matches two bare literals and returns 0 for everything else with a log.Printf warning, and NewAuditRecord writes that 0 into AuditRecord.Term unconditionally (:68). The JSONL audit log is the artifact reconciled against purchase_history, so a record saying a real commitment was purchased carries a zero-month term with no way to recover the real value. A non-canonical term reaches it in practice: normaliseTerm in providers/azure/internal/recommendations/converter.go:350-352 passes any Azure term other than P1Y/P3Y through verbatim onto Recommendation.Term. The single production caller is cmd/multi_service.go:401. A shared ParseTerm returning an error would make NewAuditRecord refuse the record instead. Audit finding A09-019.
Activity
23 remaining items
refactor(gcp): parse terms in skumatch through common.ParseTermSpelling
refactor(aws): parse terms in purchasecfg through common.ParseTermSpe…
refactor(aws): parse terms in savingsplans through common.ParseTermSp…
refactor(azure): parse terms in reservations through common.ParseTerm…
refactor(azure): parse terms in recommendations through common.ParseT…
refactor(common): type Recommendation.Term and PaymentOption as enums
fix(build): pin the provider modules to the pkg with typed Term and P…
fix(lint): drop conversions made redundant by the typed fields
cristim commentedon Nov 13, 2026
ARCH-06 slice 6 (#12) landed on main, issue left open. Commits: 8a3a00d.. rebased as ccaf670 chain (common ParseTermSpelling + Term.Years/Months; skumatch, purchasecfg, savingsplans, azure reservations and recommendations parsers migrated), ccaf670 (Recommendation.Term and PaymentOption now common.Term and common.PaymentOption, wire values unchanged; about 520 changed lines, one commit because a field type change cannot build if split across the go.work modules), 42e8dce (provider modules pinned to the new pkg for standalone CI), 46fafca (unconvert lint fix). CI on 46fafca is green; ccaf670 and 42e8dce were red (pin, then lint) and are fixed.
Review: two adversarial passes by one non-author opus reviewer; a differential run of old vs new parsers over 24 inputs showed 0 diffs. The message of the ParseTermSpelling commit overstates ("every spelling accepted anywhere"): the table covers the callers migrated so far only.
Still open:
refactor(common): add ISO and bare-month spellings and route reservat…
refactor(aws): parse Cost Explorer terms with the shared term parser
refactor(aws): parse MemoryDB month-count terms with the shared term …
refactor(azure): parse term strings with the shared term parser
cristim commentedon Nov 13, 2026
ARCH-06 (#12) finished on main. Commits: a9f4505 (common: SpellBareMonths and SpellISO flags, normalizeReservationTerm migrated), 1b2fabc (aws: convertTermInYearsE, riRequestTerm), 6bf49eb (aws memorydb "12"/"36"), 917e54b (azure: normalizeTerm, advisorTerm, normaliseTerm), 5525eb3 (audit decision comment, closes this issue).
Each migrated parser keeps its exact accepted set via a per-caller mask, with a differential test against the old switch. An independent opus reviewer ran a 15.2M-input differential (0 mismatches), 17 mask mutation probes (the 4 survivors got tests) and a per-commit vet of all five modules; two passes, no open findings. test-unit, vet and lint pass locally.
Decisions recorded: