diff --git a/src/brokerage/fidelity.zig b/src/brokerage/fidelity.zig index 3004833..741f42e 100644 --- a/src/brokerage/fidelity.zig +++ b/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); +} diff --git a/src/commands/audit/hygiene.zig b/src/commands/audit/hygiene.zig index b746308..fdfc73e 100644 --- a/src/commands/audit/hygiene.zig +++ b/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)