loadRecommendationsFromCSV silently tolerates missing required columns + CSV edge-case coverage

Background

PR #1225 (TEST-02 fix) demonstrated that the previous TestRunToolFromCSV fixture used CSV headers (Instance Type, Instance Count) that buildColumnIndexMap did not recognize, and the parser produced Count=0 recommendations without an error. The PR corrects the FIXTURE; the underlying PARSER still silently tolerates wrong / missing required columns.

The defect

cmd/multi_service_csv.go:

func buildColumnIndexMap(header []string) map[string]int {
    colIdx := make(map[string]int)
    for i, col := range header {
        colIdx[col] = i
    }
    return colIdx
}

getCSVField returns "" when a column is missing from colIdx; parseCSVRecord then turns the empty string into Count=0, ResourceType="", etc. and loadRecommendationsFromCSV returns (recs, nil) happily. The downstream effect is the very bug TEST-02 surfaced: an entire run no-ops because every row decoded to zero count, with no error message anywhere.

This violates feedback_no_silent_fallbacks.md ("no fallbacks/defaults/fabricated values on money paths; return an explicit error to the user when something is broken"). Reservation purchase IS a money path -- silently treating a mis-formatted CSV as "zero work to do" is precisely the failure mode that document forbids.

Fix

Validate the header up front against the required set, and fail loudly on missing columns:

var requiredCSVColumns = []string{"Service", "Region", "ResourceType", "Count", "Term", "PaymentOption"}

func buildColumnIndexMap(header []string) (map[string]int, error) {
    colIdx := make(map[string]int)
    for i, col := range header {
        colIdx[col] = i
    }
    var missing []string
    for _, req := range requiredCSVColumns {
        if _, ok := colIdx[req]; !ok {
            missing = append(missing, req)
        }
    }
    if len(missing) > 0 {
        return nil, fmt.Errorf("CSV header missing required columns: %v", missing)
    }
    return colIdx, nil
}

And surface the error in loadRecommendationsFromCSV. Optional/derived columns (Engine, RecommendedCount, ExistingCoverage, ...) stay optional.

CSV edge-case coverage (fold into this issue)

The PR's _EmptyFile only covers 0-byte input. Following are all uncovered today and should land as table-driven cases against loadRecommendationsFromCSV:

Headers-only-no-rows -> loads 0 recs, currently silently returns success with empty slice. After fix, this remains success but should be tested.
Wrong column count on a data row -> csv.ErrFieldCount. Should propagate as "failed to read CSV record".
BOM-prefixed file ( before Service,Region,...) -> first column name becomes Service and the column-index lookup misses it. Fix: strip BOM before parsing.
CRLF vs LF line endings -> encoding/csv handles both, but lock it in.
UTF-16 / Latin-1 encoded files -> should be rejected with a clear error (probably already happens via csv.Reader choking on the BOM).
Quoted commas / quoted newlines inside fields -> encoding/csv handles both; lock it in.
Count overflow / non-numeric -> strconv.Atoi currently has a silent zero-on-error path somewhere in parseCSVRecord; trace and fix (memory feedback_strict_int_parse.md).
The trailing TOTAL summary row currently uses string match on Service == "TOTAL"; if the column header is missing, the TOTAL detection ALSO fails. Tied to the header-validation fix above.

Triage

P2 / sev medium (money path / silent no-op) / impact internal / effort s -- ~30 LOC parser fix + table-driven test additions in cmd/multi_service_csv_test.go.

Reviewed commit: be11bdcb5. Note: origin/main moved to 3e9660d06 during the review; re-verify against current main before changing code, since a finding may have been fixed or moved.

This issue's CSV edge-case list already flags "Count overflow / non-numeric -> strconv.Atoi currently has a silent zero-on-error path somewhere in parseCSVRecord; trace and fix". The 2026-07-28 full review of cmd/ traced it. Posting the result here rather than filing a duplicate.

It is not strconv.Atoi. It is fmt.Sscanf, which is worse.

Where: cmd/multi_service_csv.go:166-190 (parseCSVInt, parseCSVFloat), called for Count and EstimatedSavings at cmd/multi_service_csv.go:115-122.

fmt.Sscanf(value, "%d", target) stops at the first non-digit and does not error on unconsumed input. Concretely, all of these return err == nil:

cell	parsed `Count`
`1.9`	`1`
`3,000`	`3`
`12abc`	`12`
`-5`	`-5` (no bound check rejects it)

Additionally, an empty cell returns nil without writing target (cmd/multi_service_csv.go:169-171), so a blank Count silently becomes 0 rather than an error. That is the zero-on-error path this issue asks about, and it is reachable independently of the missing-column defect: a file with a correct header and a blank cell hits it.

This is the exact pattern the project's own memory flags in feedback_strict_int_parse.md ("never blank-discard strconv.Atoi errors in Go").

Failure scenario: an operator edits a CSV in a spreadsheet that renders counts with a thousands separator. A row reading Count=1,200 parses as Count=1, so the run buys 1 instance instead of 1200 and reports success. The inverse mistake is equally silent: a row hand-edited to Count=2.9 buys 2 with no warning that the value was not an integer.

A negative Count survives parsing entirely. The only thing that catches it is the incidental adjusted.Count <= 0 guard in processRecommendation (cmd/multi_service_filters.go:73), and that guard is skipped entirely when --include-extended-support is passed.

Fix direction (extends this issue's fix): alongside the header validation, replace parseCSVInt / parseCSVFloat with strconv.Atoi / strconv.ParseFloat on the trimmed cell, and return an error on any parse failure, on a negative Count, and on a Count cell that is present but non-integral. Treat a blank required cell as a parse failure, not as zero.

Table-driven cases worth adding to the list already in this issue: 1,200, 2.9, 12abc, -5, and a present-but-empty Count cell. Each of those is currently accepted.

Related: #379 covers the sibling gap on the same parse path, Term and PaymentOption bypassing the allow-lists validatePaymentAndTerm applies to the flag equivalents.

claimed by cc-cli-w2

Implementation and verification are in PR #2145, head d75df92. Header corruption, BOM fidelity and invalid encodings have failing-first tests and fresh base/head full race verification; actual CLI dry runs reject malformed fixtures. This issue stays open: its proposed required set includes Term and PaymentOption, while existing minimal-CSV inputs omit those columns. Owner decision requested on whether to require all six or preserve four. The PR uses Refs #1327 until this contract is settled. #379 already tracks Term/PaymentOption value validation.

Navigation Menu

Repository navigation

loadRecommendationsFromCSV silently tolerates missing required columns + CSV edge-case coverage #1327

Background

The defect

Fix

CSV edge-case coverage (fold into this issue)

Triage

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

loadRecommendationsFromCSV silently tolerates missing required columns + CSV edge-case coverage #1327

Description

Background

The defect

Fix

CSV edge-case coverage (fold into this issue)

Triage

Activity

cristim commented on Jul 28, 2026

cristim commented on Oct 7, 2026

cristim commented on Oct 7, 2026

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions