Repository navigation
loadRecommendationsFromCSV silently tolerates missing required columns + CSV edge-case coverage #1327
Description
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 (
beforeService,Region,...) -> first column name becomesServiceand the column-index lookup misses it. Fix: strip BOM before parsing. - CRLF vs LF line endings ->
encoding/csvhandles both, but lock it in. - UTF-16 / Latin-1 encoded files -> should be rejected with a clear error (probably already happens via
csv.Readerchoking on the BOM). - Quoted commas / quoted newlines inside fields ->
encoding/csvhandles both; lock it in. Countoverflow / non-numeric ->strconv.Atoicurrently has a silent zero-on-error path somewhere inparseCSVRecord; trace and fix (memoryfeedback_strict_int_parse.md).- The trailing
TOTALsummary row currently uses string match onService == "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.
Activity
cristim commentedon Jul 28, 2026
Reviewed commit:
be11bdcb5. Note:origin/mainmoved to3e9660d06during the review; re-verify against currentmainbefore changing code, since a finding may have been fixed or moved.This issue's CSV edge-case list already flags "
Countoverflow / non-numeric ->strconv.Atoicurrently has a silent zero-on-error path somewhere inparseCSVRecord; trace and fix". The 2026-07-28 full review ofcmd/traced it. Posting the result here rather than filing a duplicate.It is not
strconv.Atoi. It isfmt.Sscanf, which is worse.Where:
cmd/multi_service_csv.go:166-190(parseCSVInt,parseCSVFloat), called forCountandEstimatedSavingsatcmd/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 returnerr == nil:Count1.913,000312abc12-5-5(no bound check rejects it)Additionally, an empty cell returns
nilwithout writingtarget(cmd/multi_service_csv.go:169-171), so a blankCountsilently becomes0rather 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-discardstrconv.Atoierrors in Go").Failure scenario: an operator edits a CSV in a spreadsheet that renders counts with a thousands separator. A row reading
Count=1,200parses asCount=1, so the run buys 1 instance instead of 1200 and reports success. The inverse mistake is equally silent: a row hand-edited toCount=2.9buys 2 with no warning that the value was not an integer.A negative
Countsurvives parsing entirely. The only thing that catches it is the incidentaladjusted.Count <= 0guard inprocessRecommendation(cmd/multi_service_filters.go:73), and that guard is skipped entirely when--include-extended-supportis passed.Fix direction (extends this issue's fix): alongside the header validation, replace
parseCSVInt/parseCSVFloatwithstrconv.Atoi/strconv.ParseFloaton the trimmed cell, and return an error on any parse failure, on a negativeCount, and on aCountcell 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-emptyCountcell. Each of those is currently accepted.Related: #379 covers the sibling gap on the same parse path,
TermandPaymentOptionbypassing the allow-listsvalidatePaymentAndTermapplies to the flag equivalents.cristim commentedon Oct 7, 2026
claimed by cc-cli-w2
cristim commentedon Oct 7, 2026
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.