better brokerage detection for csvs

src/brokerage/fidelity.zig

@@ -65,6 +65,50 @@ const Col = struct {
     const type_col = 15;
 };
 
+/// Canonical name of each column `parseCsv` actually reads, at its
+/// expected index. The header is validated against these
+/// case-insensitively (Fidelity re-cases labels - Title Case ->
+/// sentence case) so a reorder, insertion, or removal that would shift a
+/// value we depend on is caught as `error.UnexpectedHeader` instead of
+/// silently reading the wrong column.
+///
+/// Columns the parser ignores are intentionally absent here: Fidelity
+/// adding, renaming, or appending an unused column is tolerated. Keep
+/// this in sync with `Col`.
+const ExpectedColumn = struct { idx: usize, name: []const u8 };
+const expected_header = [_]ExpectedColumn{
+    .{ .idx = Col.account_number, .name = "Account number" },
+    .{ .idx = Col.account_name, .name = "Account name" },
+    .{ .idx = Col.symbol, .name = "Symbol" },
+    .{ .idx = Col.description, .name = "Description" },
+    .{ .idx = Col.quantity, .name = "Quantity" },
+    .{ .idx = Col.last_price, .name = "Last price" },
+    .{ .idx = Col.current_value, .name = "Current value" },
+    .{ .idx = Col.cost_basis_total, .name = "Cost basis total" },
+    .{ .idx = Col.avg_cost_basis, .name = "Average cost basis" },
+    .{ .idx = Col.type_col, .name = "Type" },
+};
+
+/// Validate that the header carries the columns `parseCsv` indexes into,
+/// at their expected positions (case-insensitive). Trailing extra columns
+/// are tolerated; a missing, renamed, or reordered read-column is
+/// rejected with `error.UnexpectedHeader`. See `expected_header`.
+fn validateHeader(header: []const u8) error{UnexpectedHeader}!void {
+    // Fidelity does not quote fields containing commas, so a plain comma
+    // split matches how the data rows are parsed below.
+    var fields: [expected_columns][]const u8 = undefined;
+    var n: usize = 0;
+    var it = std.mem.splitScalar(u8, header, ',');
+    while (it.next()) |f| : (n += 1) {
+        if (n < expected_columns) fields[n] = std.mem.trim(u8, f, &.{ ' ', '"' });
+    }
+    // Need at least the columns we index into (highest index is type_col).
+    if (n < expected_columns) return error.UnexpectedHeader;
+    inline for (expected_header) |col| {
+        if (!std.ascii.eqlIgnoreCase(fields[col.idx], col.name)) return error.UnexpectedHeader;
+    }
+}
+
 /// Parse a Fidelity CSV positions export into BrokeragePosition slices.
 /// All string fields in the returned positions are slices into `data`,
 /// so the caller must keep `data` alive for as long as the positions are used.
@@ -81,16 +125,15 @@ pub fn parseCsv(allocator: std.mem.Allocator, data: []const u8) ![]BrokeragePosi
 
     var lines = std.mem.splitScalar(u8, content, '\n');
 
-    // Validate header row. Case-insensitive: Fidelity switched the export
-    // header from Title Case to sentence case ("Account number,Account
-    // name,..."). The column order is unchanged, so the positional parsing
-    // below still holds; only this guard needed to loosen.
+    // Validate the header row. We check the specific columns parseCsv reads,
+    // by name at their expected index (case-insensitive), so a column
+    // reorder / insertion / removal that would shift a value we depend on is
+    // caught here as UnexpectedHeader rather than silently mis-reading a
+    // value. See `expected_header` / `validateHeader`.
     const header_line = lines.next() orelse return error.EmptyFile;
     const header_trimmed = std.mem.trimEnd(u8, header_line, &.{ '\r', ' ' });
     if (header_trimmed.len == 0) return error.EmptyFile;
-    if (!std.ascii.startsWithIgnoreCase(header_trimmed, "Account Number")) {
-        return error.UnexpectedHeader;
-    }
+    try validateHeader(header_trimmed);
 
     // Parse data rows
     while (lines.next()) |line| {
@@ -283,3 +326,31 @@ test "parseCsv accepts Fidelity's lowercase (sentence-case) header" {
     try std.testing.expectApproxEqAbs(@as(f64, 15000.00), positions[1].current_value.?, 0.01);
     try std.testing.expectApproxEqAbs(@as(f64, 10000.00), positions[1].cost_basis.?, 0.01);
 }
+
+test "parseCsv rejects a header whose read-columns are reordered" {
+    // Quantity (idx 4) and Last price (idx 5) are swapped. Positional parsing
+    // would silently read price-as-quantity, so validateHeader must reject it
+    // rather than let a shifted column through.
+    const csv =
+        "Account number,Account name,Symbol,Description,Last price,Quantity,Last price change,Current value,Today's gain/loss dollar,Today's gain/loss percent,Total gain/loss dollar,Total gain/loss percent,Percent of account,Cost basis total,Average cost basis,Type\n" ++
+        "Z123,Individual,AAPL,APPLE INC,$150.00,100,-$2.00,$15000.00,,,,,99%,$10000.00,$100.00,Margin,\n";
+    const allocator = std.testing.allocator;
+    try std.testing.expectError(error.UnexpectedHeader, parseCsv(allocator, csv));
+}
+
+test "parseCsv tolerates an unknown trailing column" {
+    // Fidelity appending a column the parser doesn't read (a 17th, here
+    // "Accrued interest") must not break parsing: the columns we index into
+    // are unshifted, so validateHeader passes and values still resolve.
+    const csv =
+        "Account number,Account name,Symbol,Description,Quantity,Last price,Last price change,Current value,Today's gain/loss dollar,Today's gain/loss percent,Total gain/loss dollar,Total gain/loss percent,Percent of account,Cost basis total,Average cost basis,Type,Accrued interest\n" ++
+        "Z123,Individual,AAPL,APPLE INC,100,$150.00,-$2.00,$15000.00,,,,,99%,$10000.00,$100.00,Margin,$0.00\n";
+    const allocator = std.testing.allocator;
+    const positions = try parseCsv(allocator, csv);
+    defer allocator.free(positions);
+    try std.testing.expectEqual(@as(usize, 1), positions.len);
+    try std.testing.expectEqualStrings("AAPL", positions[0].symbol);
+    try std.testing.expect(!positions[0].is_cash);
+    try std.testing.expectApproxEqAbs(@as(f64, 100), positions[0].quantity.?, 0.01);
+    try std.testing.expectApproxEqAbs(@as(f64, 15000.00), positions[0].current_value.?, 0.01);
+}

src/commands/audit/hygiene.zig

@@ -58,7 +58,13 @@ const DiscoveredFile = struct {
     dir_label: []const u8, // e.g. "audit/" or "$ZFIN_AUDIT_FILES"
 };
 
-/// Detect the brokerage type from file contents by inspecting the first few lines.
+/// Detect the brokerage type from file contents.
+///
+/// Detection answers only "which brokerage produced this file?" - it is
+/// deliberately separate from "can we parse it?", which each parser
+/// enforces via its own header/column validation. Keying detection on a
+/// stable self-identifying marker (rather than the column header) means a
+/// format tweak surfaces as a specific parse error, not a misroute.
 fn detectBrokerFileKind(data: []const u8) ?BrokerFileKind {
     // Strip optional UTF-8 BOM
     const content = if (data.len >= 3 and data[0] == 0xEF and data[1] == 0xBB and data[2] == 0xBF)
@@ -66,31 +72,29 @@ fn detectBrokerFileKind(data: []const u8) ?BrokerFileKind {
     else
         data;
 
-    // Fidelity CSV: first line starts with "Account Number"/"Account Name".
-    // Case-insensitive: Fidelity switched the export header from Title Case
-    // to sentence case ("Account number,Account name,...") and could change
-    // it again. Matching this loosely also keeps the file from falling
-    // through to the broad schwab-summary heuristic below, which would
-    // otherwise misclassify it via the "Brokerage services..." legal footer.
-    if (std.ascii.startsWithIgnoreCase(content, "Account Number") or
-        std.ascii.startsWithIgnoreCase(content, "Account Name"))
+    // Fidelity CSV: identified by its legal-disclaimer footer, which names
+    // the "Fidelity Brokerage Services LLC" legal entity. This is the file's
+    // true self-identification and is stable boilerplate - unlike the column
+    // header, which Fidelity has already re-cased (Title Case -> sentence
+    // case) and could change again. Keying on the legal-entity phrase also
+    // avoids false positives that bare "Fidelity" would cause: a *Schwab* CSV
+    // holding a Fidelity fund shows "FIDELITY ..." as a ticker description,
+    // but never the legal-entity name. Whether the columns are still
+    // parseable is a separate concern, enforced by the parser's header check.
+    if (std.mem.indexOf(u8, content, "Fidelity Brokerage Services LLC") != null)
         return .fidelity_csv;
 
     // Schwab per-account CSV: starts with a quoted title line like "Positions for ..."
     if (std.mem.startsWith(u8, content, "\"Positions for")) return .schwab_csv;
 
-    // Schwab summary: contains "Account number ending in" pattern
-    const peek = content[0..@min(content.len, 4096)];
-    if (std.mem.indexOf(u8, peek, "Account number ending in") != null) return .schwab_summary;
-    // Also match by account type labels + dollar amounts
-    if ((std.mem.indexOf(u8, peek, "Brokerage") != null or
-        std.mem.indexOf(u8, peek, "Roth IRA") != null or
-        std.mem.indexOf(u8, peek, "Traditional IRA") != null or
-        std.mem.indexOf(u8, peek, "Rollover IRA") != null) and
-        std.mem.indexOf(u8, peek, "$") != null)
-    {
-        return .schwab_summary;
-    }
+    // Schwab summary: the "Account number ending in" anchor is exactly what
+    // parseSummary keys on to find account blocks, so detection matches what
+    // the parser can actually handle. There is intentionally no looser
+    // fallback (e.g. account-type labels + "$"): a file lacking this anchor
+    // cannot be parsed as a summary, so classifying it as one would only
+    // produce a guaranteed parse failure - and would misclassify unrelated
+    // CSVs (Fidelity's legal footer contains "Brokerage", every export has "$").
+    if (std.mem.indexOf(u8, content, "Account number ending in") != null) return .schwab_summary;
 
     return null;
 }
@@ -931,7 +935,10 @@ pub fn runHygieneCheck(
 
             switch (f.kind) {
                 .schwab_summary => {
-                    const results = schwab.reconcileSummary(allocator, portfolio, file_data, account_map, prices, as_of) catch continue;
+                    const results = schwab.reconcileSummary(allocator, portfolio, file_data, account_map, prices, as_of) catch |err| {
+                        try cli.printFg(out, color, cli.CLR_WARNING, "    {s}: detected as schwab summary but could not parse ({s}); skipped\n", .{ f.path, @errorName(err) });
+                        continue;
+                    };
                     defer allocator.free(results);
 
                     if (verbose or schwab.hasSchwabDiscrepancies(results)) {
@@ -953,7 +960,10 @@ pub fn runHygieneCheck(
                     try accumulatePresent(allocator, &schwab_present, schwab.SchwabAccountComparison, results);
                 },
                 .fidelity_csv => {
-                    const results = fidelity.reconcile(allocator, portfolio, file_data, account_map, prices, as_of) catch continue;
+                    const results = fidelity.reconcile(allocator, portfolio, file_data, account_map, prices, as_of) catch |err| {
+                        try cli.printFg(out, color, cli.CLR_WARNING, "    {s}: detected as fidelity CSV but could not parse ({s}); skipped\n", .{ f.path, @errorName(err) });
+                        continue;
+                    };
                     defer {
                         for (results) |r| allocator.free(r.comparisons);
                         allocator.free(results);
@@ -972,7 +982,10 @@ pub fn runHygieneCheck(
                     try accumulatePresent(allocator, &fidelity_present, common.AccountComparison, results);
                 },
                 .schwab_csv => {
-                    const results = schwab.reconcileCsv(allocator, portfolio, file_data, account_map, prices, as_of) catch continue;
+                    const results = schwab.reconcileCsv(allocator, portfolio, file_data, account_map, prices, as_of) catch |err| {
+                        try cli.printFg(out, color, cli.CLR_WARNING, "    {s}: detected as schwab CSV but could not parse ({s}); skipped\n", .{ f.path, @errorName(err) });
+                        continue;
+                    };
                     defer {
                         for (results) |r| allocator.free(r.comparisons);
                         allocator.free(results);
@@ -1108,30 +1121,42 @@ test "accumulatePresent: result strings are owned copies (survive source free)"
     try std.testing.expectEqualStrings("9012", dst.items[0]);
 }
 
-test "detectBrokerFileKind: fidelity csv" {
-    const fidelity_header = "Account Number,Account Name,Symbol,Description";
-    try std.testing.expectEqual(BrokerFileKind.fidelity_csv, detectBrokerFileKind(fidelity_header).?);
-}
-
-test "detectBrokerFileKind: fidelity csv with BOM" {
-    const fidelity_bom = "\xEF\xBB\xBFAccount Number,Account Name,Symbol";
-    try std.testing.expectEqual(BrokerFileKind.fidelity_csv, detectBrokerFileKind(fidelity_bom).?);
-}
-
-test "detectBrokerFileKind: fidelity csv with lowercase header is not misclassified as schwab" {
-    // Regression: Fidelity switched the header from Title Case to sentence
-    // case ("Account number,..."). The old case-sensitive check failed and
-    // the file fell through to the schwab-summary heuristic, which
-    // false-matched on the "Brokerage services are provided by..." legal
-    // footer plus the "$" amounts. Must classify as fidelity_csv.
+test "detectBrokerFileKind: fidelity csv identified by legal footer" {
+    // Detection keys on the self-identifying legal-entity footer, not the
+    // column header (which Fidelity has already re-cased). A realistic
+    // export: header, a data row, blank line, then the disclaimer footer.
     const data =
         "Account number,Account name,Symbol,Description,Quantity,Last price,Last price change,Current value,Today's gain/loss dollar,Today's gain/loss percent,Total gain/loss dollar,Total gain/loss percent,Percent of account,Cost basis total,Average cost basis,Type\n" ++
         "Z123,Individual - TOD,AAPL,APPLE INC,100,$150.00,-$2.00,$15000.00,,,,,99.88%,$10000.00,$100.00,Margin,\n" ++
         "\n" ++
-        "\"Brokerage services are provided by Fidelity Brokerage Services LLC (FBS)...\" $\n";
+        "\"Brokerage services are provided by Fidelity Brokerage Services LLC (FBS), 900 Salem Street, Smithfield, RI 02917. ...\"\n";
     try std.testing.expectEqual(BrokerFileKind.fidelity_csv, detectBrokerFileKind(data).?);
 }
 
+test "detectBrokerFileKind: fidelity detected by footer even with an unrecognized header" {
+    // The footer is the identity signal, so a future header change (or a
+    // leading BOM, or no header at all) does not defeat detection. Whether
+    // the columns are parseable is a separate concern enforced by the
+    // parser's header validation.
+    const data =
+        "\xEF\xBB\xBFsome future header layout we do not recognize\n" ++
+        "...data...\n\n" ++
+        "\"... Both are Fidelity Brokerage Services LLC companies and members SIPC ...\"\n";
+    try std.testing.expectEqual(BrokerFileKind.fidelity_csv, detectBrokerFileKind(data).?);
+}
+
+test "detectBrokerFileKind: a Fidelity fund inside a Schwab CSV is not mis-detected as Fidelity" {
+    // Bare "Fidelity" would false-match here (FIDELITY STOCK SELECTOR is a
+    // holding), but the legal-entity phrase never appears as a ticker
+    // description - so this is correctly detected as a Schwab CSV.
+    const data =
+        "\"Positions for account Sample IRA ...1234 as of 10:00 AM ET, 2026/06/27\"\n" ++
+        "\n" ++
+        "\"Symbol\",\"Description\",\"Price\"\n" ++
+        "\"FDSCX\",\"FIDELITY STOCK SELECTOR SMALL CAP\",\"51.65\"\n";
+    try std.testing.expectEqual(BrokerFileKind.schwab_csv, detectBrokerFileKind(data).?);
+}
+
 test "detectBrokerFileKind: schwab csv" {
     const schwab_header = "\"Positions for account Roth IRA ...1234 as of\"";
     try std.testing.expectEqual(BrokerFileKind.schwab_csv, detectBrokerFileKind(schwab_header).?);
@@ -1449,9 +1474,14 @@ test "detectBrokerFileKind: schwab csv with Positions header" {
     try std.testing.expectEqual(BrokerFileKind.schwab_csv, detectBrokerFileKind(data).?);
 }
 
-test "detectBrokerFileKind: schwab summary with Roth IRA" {
+test "detectBrokerFileKind: summary-shaped text without the anchor is not detected" {
+    // Regression guard against re-introducing the old "account-type label +
+    // $" fallback. This blob looks summary-ish (account-type words, dollar
+    // amounts) but lacks the "Account number ending in" anchor that
+    // parseSummary requires - so classifying it as a summary would only
+    // yield a guaranteed NoAccountsFound. Detection must return null.
     const data = "Roth IRA ...1234\nSome text\n$50,000.00\n";
-    try std.testing.expectEqual(BrokerFileKind.schwab_summary, detectBrokerFileKind(data).?);
+    try std.testing.expect(detectBrokerFileKind(data) == null);
 }
 
 test "UpdateCadence label" {
@@ -1469,10 +1499,10 @@ test "discoverBrokerFiles: finds files in temp directory" {
     var tmp = std.testing.tmpDir(.{});
     defer tmp.cleanup();
 
-    // Write a fidelity CSV
+    // Write a fidelity CSV (identified by its legal-entity footer)
     tmp.dir.writeFile(io, .{
         .sub_path = "fidelity.csv",
-        .data = "Account Number,Account Name,Symbol,Description,Quantity,Last Price,Current Value\nZ123,Test,AAPL,Apple,100,200,20000\n",
+        .data = "Account number,Account name,Symbol,Description,Quantity,Last price,Current value\nZ123,Test,AAPL,Apple,100,200,20000\n\n\"Brokerage services are provided by Fidelity Brokerage Services LLC (FBS), 900 Salem Street, Smithfield, RI 02917. ...\"\n",
     }) catch unreachable;
 
     // Write a schwab summary (non-CSV)