better brokerage detection for csvs

In early July fidelity changed the casing on the CSV headers. This broke
the file detection (Schwab/Fidelity) because zfin was doing a case
sensitive match on these headers. This quick fix addresses this problem,
and a wider fix to make detection less fragile will follow shortly.

.mise.toml

@@ -4,7 +4,4 @@ zls = "0.16.0"
 "github:j178/prek" = "0.4.1"
 
 [tools."github:DonIsaac/zlint"]
-version = "0.8.1"
+version = "0.9.0"
-
-[tools."github:DonIsaac/zlint".platforms]
-linux-riscv64 = { url = "https://github.com/elerch/zlint/releases/download/v0.8.1/zlint-linux-riscv64", checksum = "sha256:502d6a128631688ddf8bd8fad9ffac9f5dc51c963aaf36bbd5b56538bca8f4bd" }

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,13 +125,15 @@ pub fn parseCsv(allocator: std.mem.Allocator, data: []const u8) ![]BrokeragePosi
 
     var lines = std.mem.splitScalar(u8, content, '\n');
 
-    // Validate header row
+    // 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.mem.startsWith(u8, header_trimmed, "Account Number")) {
+    try validateHeader(header_trimmed);
-        return error.UnexpectedHeader;
-    }
 
     // Parse data rows
     while (lines.next()) |line| {
@@ -247,3 +293,64 @@ test "parseCsv cash account type is not cash position" {
     try std.testing.expect(!positions[0].is_cash);
     try std.testing.expectApproxEqAbs(@as(f64, 190), positions[0].quantity.?, 0.01);
 }
+
+test "parseCsv accepts Fidelity's lowercase (sentence-case) header" {
+    // Regression: Fidelity switched the export header from Title Case to
+    // sentence case ("Account number,Account name,...,Last price,...,Cost
+    // basis total,Average cost basis,Type"). Column order is unchanged, so
+    // positional parsing must still work. The trailing comma after the Type
+    // value and the blank-line-separated legal footer mirror the real export.
+    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\n" ++
+        "Z123,Individual - TOD,FZFXX**,HELD IN MONEY MARKET,,,,$24.03,,,,,0.12%,,,Cash,\n" ++
+        "Z123,Individual - TOD,AAPL,APPLE INC,100,$150.00,-$2.00,$15000.00,-$200.00,-1.3%,+$5000.00,+50%,99.88%,$10000.00,$100.00,Margin,\n" ++
+        "\n" ++
+        "\"Brokerage services are provided by Fidelity Brokerage Services LLC (FBS)...\"\n";
+
+    const allocator = std.testing.allocator;
+    const positions = try parseCsv(allocator, csv);
+    defer allocator.free(positions);
+
+    try std.testing.expectEqual(@as(usize, 2), positions.len);
+
+    // Money-market row -> cash, no quantity.
+    try std.testing.expectEqualStrings("FZFXX", positions[0].symbol);
+    try std.testing.expect(positions[0].is_cash);
+    try std.testing.expect(positions[0].quantity == null);
+    try std.testing.expectApproxEqAbs(@as(f64, 24.03), positions[0].current_value.?, 0.01);
+
+    // Stock row -> positional columns still resolve correctly.
+    try std.testing.expectEqualStrings("AAPL", positions[1].symbol);
+    try std.testing.expect(!positions[1].is_cash);
+    try std.testing.expectApproxEqAbs(@as(f64, 100), positions[1].quantity.?, 0.01);
+    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

@@ -23,6 +23,7 @@ const portfolio_mod = @import("../../models/portfolio.zig");
 const Date = @import("../../Date.zig");
 const srf = @import("srf");
 const git = @import("../../git.zig");
+const test_git = @import("../../testutil/git.zig");
 
 const common = @import("common.zig");
 const fidelity = @import("fidelity.zig");
@@ -36,6 +37,12 @@ const audit_file_max_age_hours = 24;
 const audit_file_max_size_non_csv = 512 * 1024; // 512KB, for non-CSV files only
 pub const default_stale_days: u32 = 3;
 const stale_warning_multiplier: u32 = 2; // yellow -> red at 2× threshold
+/// Safety cap on how many portfolio commits the account-cadence walk
+/// scans when resolving each account's last-update timestamp. The walk
+/// normally early-exits once every account resolves; this bounds the
+/// pathological case where an account never changed in tracked history
+/// (so it never resolves) from forcing a scan of the entire repo.
+const max_history_commits_scanned: usize = 500;
 
 /// Type of a discovered brokerage file.
 const BrokerFileKind = enum {
@@ -51,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)
@@ -59,26 +72,29 @@ fn detectBrokerFileKind(data: []const u8) ?BrokerFileKind {
     else
         data;
 
-    // Fidelity CSV: first line starts with "Account Number" or "Account Name"
+    // Fidelity CSV: identified by its legal-disclaimer footer, which names
-    if (std.mem.startsWith(u8, content, "Account Number") or
+    // the "Fidelity Brokerage Services LLC" legal entity. This is the file's
-        std.mem.startsWith(u8, content, "Account Name"))
+    // 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
+    // Schwab summary: the "Account number ending in" anchor is exactly what
-    const peek = content[0..@min(content.len, 4096)];
+    // parseSummary keys on to find account blocks, so detection matches what
-    if (std.mem.indexOf(u8, peek, "Account number ending in") != null) return .schwab_summary;
+    // the parser can actually handle. There is intentionally no looser
-    // Also match by account type labels + dollar amounts
+    // fallback (e.g. account-type labels + "$"): a file lacking this anchor
-    if ((std.mem.indexOf(u8, peek, "Brokerage") != null or
+    // cannot be parsed as a summary, so classifying it as one would only
-        std.mem.indexOf(u8, peek, "Roth IRA") != null or
+    // produce a guaranteed parse failure - and would misclassify unrelated
-        std.mem.indexOf(u8, peek, "Traditional IRA") != null or
+    // CSVs (Fidelity's legal footer contains "Brokerage", every export has "$").
-        std.mem.indexOf(u8, peek, "Rollover IRA") != null) and
+    if (std.mem.indexOf(u8, content, "Account number ending in") != null) return .schwab_summary;
-        std.mem.indexOf(u8, peek, "$") != null)
-    {
-        return .schwab_summary;
-    }
 
     return null;
 }
@@ -232,6 +248,89 @@ fn lotToString(allocator: std.mem.Allocator, lot: portfolio_mod.Lot) ![]const u8
     return std.fmt.allocPrint(allocator, "{f}", .{srf.fmt(portfolio_mod.Lot, &lots, .{ .emit_directives = false })});
 }
 
+/// Resolve, per account, the committer timestamp (Unix epoch seconds)
+/// of the newest commit in which that account's lots changed in
+/// `ri.rel_path`. Walks the file's git history newest-to-oldest,
+/// diffing adjacent commit pairs and attributing each change to the
+/// newer commit of the pair; results are written into `out`.
+///
+/// The walk covers full history (no cadence-derived cutoff): an
+/// account more than 2x its cadence overdue - exactly the ones we most
+/// want an age readout for - would otherwise always fall outside a
+/// 2x-cadence window and degrade to "no update history found". It
+/// stops early once every account in `all_accounts` has a timestamp,
+/// or after scanning `max_history_commits_scanned` commits (the safety
+/// valve for a never-changed account).
+///
+/// Accounts with no detectable change in tracked history are simply
+/// absent from `out` (e.g. an account present since before the first
+/// portfolio commit, whose initial add has no parent commit to diff
+/// against); the caller renders those as "no update history found".
+///
+/// Keys written into `out` are borrowed from `all_accounts` (stable
+/// working-copy account-name pointers), never from the transient
+/// historical portfolio strings. Git unavailability degrades to an
+/// empty result rather than an error.
+fn findLastUpdateTimestamps(
+    io: std.Io,
+    allocator: std.mem.Allocator,
+    env: *const std.process.Environ.Map,
+    ri: git.RepoInfo,
+    all_accounts: *const std.StringHashMap(void),
+    out: *std.StringHashMap(i64),
+) !void {
+    const commits = git.listCommitsTouching(io, allocator, env, ri.root, ri.rel_path, null) catch &.{};
+    defer git.freeCommitTouches(allocator, commits);
+
+    var prev_data: ?[]const u8 = null;
+    defer if (prev_data) |pd| allocator.free(pd);
+
+    for (commits, 0..) |ct, ci| {
+        // Stop once every account is resolved, or at the scan cap.
+        if (out.count() >= all_accounts.count()) break;
+        if (ci >= max_history_commits_scanned) break;
+
+        const rev_data = git.show(io, allocator, env, ri.root, ct.commit, ri.rel_path) catch continue;
+
+        if (ci > 0) {
+            if (prev_data) |pd| {
+                // rev_data is older, pd is newer (commits are newest-first).
+                var old_pf = zfin.cache.deserializePortfolio(allocator, rev_data) catch {
+                    allocator.free(rev_data);
+                    continue;
+                };
+                defer old_pf.deinit();
+                var new_pf = zfin.cache.deserializePortfolio(allocator, pd) catch {
+                    allocator.free(rev_data);
+                    continue;
+                };
+                defer new_pf.deinit();
+
+                var mods = findModifiedAccounts(allocator, old_pf, new_pf) catch {
+                    allocator.free(rev_data);
+                    continue;
+                };
+                defer mods.deinit();
+
+                // The newer commit's timestamp is when these accounts changed.
+                const update_ts = commits[ci - 1].timestamp;
+
+                // Match against stable working-copy account names.
+                var acct_iter = all_accounts.keyIterator();
+                while (acct_iter.next()) |stable_name| {
+                    if (out.contains(stable_name.*)) continue;
+                    if (mods.contains(stable_name.*)) {
+                        try out.put(stable_name.*, update_ts);
+                    }
+                }
+            }
+        }
+
+        if (prev_data) |pd| allocator.free(pd);
+        prev_data = rev_data;
+    }
+}
+
 /// Staleness color based on age vs threshold.
 /// Returns CLR_MUTED for within threshold, warning for 1-2x, negative for >2x.
 fn stalenessColor(age_days: i32, threshold: u32) [3]u8 {
@@ -665,68 +764,14 @@ pub fn runHygieneCheck(
         }
 
         // Find last update time for each account via git history.
-        // Walk commits newest-to-oldest, diffing adjacent pairs to find
+        // Walks the portfolio file's full history (no cadence-derived
-        // which accounts changed. Use working-copy account names as keys
+        // cutoff), keyed by stable working-copy account names. Absent
-        // (stable lifetime) rather than historical portfolio strings.
+        // entries render as "no update history found" below.
-        // Only walk back far enough to hit red status (2× max cadence).
         var last_update_ts = std.StringHashMap(i64).init(allocator);
         defer last_update_ts.deinit();
 
         if (repo_info) |ri| {
-            // Compute the furthest we need to look back: 2× the max cadence
+            try findLastUpdateTimestamps(io, allocator, env, ri, &all_accounts, &last_update_ts);
-            var max_threshold: u32 = 14; // 2× weekly default
-            for (account_map.entries) |entry| {
-                if (entry.update_cadence.thresholdDays()) |td| {
-                    const red = td * stale_warning_multiplier;
-                    if (red > max_threshold) max_threshold = red;
-                }
-            }
-            var since_buf: [32]u8 = undefined;
-            const since = std.fmt.bufPrint(&since_buf, "{d} days ago", .{max_threshold}) catch "30 days ago";
-
-            const commits = git.listCommitsTouching(io, allocator, env, ri.root, ri.rel_path, since) catch &.{};
-            defer git.freeCommitTouches(allocator, commits);
-
-            var prev_data: ?[]const u8 = null;
-            defer if (prev_data) |pd| allocator.free(pd);
-
-            for (commits, 0..) |ct, ci| {
-                // Stop early if every account already has a timestamp
-                if (last_update_ts.count() >= all_accounts.count()) break;
-
-                const rev_data = git.show(io, allocator, env, ri.root, ct.commit, ri.rel_path) catch continue;
-
-                if (ci > 0) {
-                    if (prev_data) |pd| {
-                        // rev_data is older, pd is newer (commits are newest-first)
-                        var old_pf = zfin.cache.deserializePortfolio(allocator, rev_data) catch {
-                            allocator.free(rev_data);
-                            continue;
-                        };
-                        defer old_pf.deinit();
-                        var new_pf = zfin.cache.deserializePortfolio(allocator, pd) catch continue;
-                        defer new_pf.deinit();
-
-                        var mods = findModifiedAccounts(allocator, old_pf, new_pf) catch continue;
-                        defer mods.deinit();
-
-                        // The newer commit's timestamp is when these accounts were updated
-                        const update_ts = commits[ci - 1].timestamp;
-
-                        // Match against stable working-copy account names
-                        var acct_iter = all_accounts.keyIterator();
-                        while (acct_iter.next()) |stable_name| {
-                            if (last_update_ts.contains(stable_name.*)) continue;
-                            if (mods.contains(stable_name.*)) {
-                                try last_update_ts.put(stable_name.*, update_ts);
-                            }
-                        }
-                    }
-                }
-
-                if (prev_data) |pd| allocator.free(pd);
-                prev_data = rev_data;
-            }
         }
 
         // Display overdue accounts
@@ -890,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)) {
@@ -912,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);
@@ -931,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);
@@ -1067,14 +1121,40 @@ test "accumulatePresent: result strings are owned copies (survive source free)"
     try std.testing.expectEqualStrings("9012", dst.items[0]);
 }
 
-test "detectBrokerFileKind: fidelity csv" {
+test "detectBrokerFileKind: fidelity csv identified by legal footer" {
-    const fidelity_header = "Account Number,Account Name,Symbol,Description";
+    // Detection keys on the self-identifying legal-entity footer, not the
-    try std.testing.expectEqual(BrokerFileKind.fidelity_csv, detectBrokerFileKind(fidelity_header).?);
+    // 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), 900 Salem Street, Smithfield, RI 02917. ...\"\n";
+    try std.testing.expectEqual(BrokerFileKind.fidelity_csv, detectBrokerFileKind(data).?);
 }
 
-test "detectBrokerFileKind: fidelity csv with BOM" {
+test "detectBrokerFileKind: fidelity detected by footer even with an unrecognized header" {
-    const fidelity_bom = "\xEF\xBB\xBFAccount Number,Account Name,Symbol";
+    // The footer is the identity signal, so a future header change (or a
-    try std.testing.expectEqual(BrokerFileKind.fidelity_csv, detectBrokerFileKind(fidelity_bom).?);
+    // 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" {
@@ -1394,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" {
@@ -1414,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)
@@ -1697,3 +1782,86 @@ test "runHygieneCheck: Section 6 flags an un-opted-in symbol's split, not an opt
     try std.testing.expect(std.mem.indexOf(u8, sec6, "NVDA") != null);
     try std.testing.expect(std.mem.indexOf(u8, sec6, "AMZN") == null);
 }
+
+/// Format a two-account portfolio.srf for the git-history test. Only
+/// `shares` varies between revisions, which is enough for
+/// findModifiedAccounts to flag the account.
+fn testPortfolioSrf(buf: []u8, ira_shares: []const u8, roth_shares: []const u8) ![]const u8 {
+    return std.fmt.bufPrint(buf, "#!srfv1\n" ++
+        "symbol::VOO,shares:num:{s},open_date::2026-01-01,open_price:num:1.00,account::Sample IRA\n" ++
+        "symbol::BND,shares:num:{s},open_date::2026-01-01,open_price:num:1.00,account::Sample Roth\n", .{ ira_shares, roth_shares });
+}
+
+test "findLastUpdateTimestamps: resolves an account changed in a non-newest commit" {
+    const allocator = std.testing.allocator;
+    const io = std.testing.io;
+
+    // Skip if `git` isn't on PATH (CI sandbox without git).
+    if (!test_git.available(allocator)) return;
+
+    var tmp = std.testing.tmpDir(.{});
+    defer tmp.cleanup();
+
+    var path_buf: [std.fs.max_path_bytes]u8 = undefined;
+    const dir_len = try tmp.dir.realPathFile(io, ".", &path_buf);
+    const dir = path_buf[0..dir_len];
+
+    var buf: [512]u8 = undefined;
+
+    // Commit A (oldest): IRA=100, Roth=50.
+    try tmp.dir.writeFile(io, .{ .sub_path = "portfolio.srf", .data = try testPortfolioSrf(&buf, "100", "50") });
+    try test_git.run(allocator, dir, null, &.{ "init", "-q" });
+    try test_git.run(allocator, dir, null, &.{ "config", "user.email", "test@example.com" });
+    try test_git.run(allocator, dir, null, &.{ "config", "user.name", "Test" });
+    try test_git.run(allocator, dir, null, &.{ "config", "commit.gpgsign", "false" });
+    try test_git.run(allocator, dir, null, &.{ "add", "portfolio.srf" });
+    try test_git.run(allocator, dir, "2026-01-10T12:00:00", &.{ "commit", "-q", "-m", "A" });
+
+    // Commit B: IRA changes to 200 (Roth unchanged). This is the change
+    // the old 2x-cadence lookback window could exclude, leaving IRA as
+    // "no update history found".
+    try tmp.dir.writeFile(io, .{ .sub_path = "portfolio.srf", .data = try testPortfolioSrf(&buf, "200", "50") });
+    try test_git.run(allocator, dir, null, &.{ "add", "portfolio.srf" });
+    try test_git.run(allocator, dir, "2026-02-15T12:00:00", &.{ "commit", "-q", "-m", "B" });
+
+    // Commit C (newest): Roth changes to 60 (IRA unchanged).
+    try tmp.dir.writeFile(io, .{ .sub_path = "portfolio.srf", .data = try testPortfolioSrf(&buf, "200", "60") });
+    try test_git.run(allocator, dir, null, &.{ "add", "portfolio.srf" });
+    try test_git.run(allocator, dir, "2026-07-01T12:00:00", &.{ "commit", "-q", "-m", "C" });
+
+    var env = try std.testing.environ.createMap(allocator);
+    defer env.deinit();
+
+    const pf_path = try std.fs.path.join(allocator, &.{ dir, "portfolio.srf" });
+    defer allocator.free(pf_path);
+
+    const ri = try git.findRepo(io, allocator, &env, pf_path);
+    defer {
+        allocator.free(ri.root);
+        allocator.free(ri.rel_path);
+    }
+
+    // Authoritative committer timestamps, newest-first: [0]=C, [1]=B, [2]=A.
+    const commits = try git.listCommitsTouching(io, allocator, &env, ri.root, ri.rel_path, null);
+    defer git.freeCommitTouches(allocator, commits);
+    try std.testing.expectEqual(@as(usize, 3), commits.len);
+
+    var all_accounts = std.StringHashMap(void).init(allocator);
+    defer all_accounts.deinit();
+    try all_accounts.put("Sample IRA", {});
+    try all_accounts.put("Sample Roth", {});
+
+    var out = std.StringHashMap(i64).init(allocator);
+    defer out.deinit();
+    try findLastUpdateTimestamps(io, allocator, &env, ri, &all_accounts, &out);
+
+    // Both accounts resolve. Critically "Sample IRA" - last changed in
+    // commit B, NOT the newest commit - is found rather than absent.
+    const ira_ts = out.get("Sample IRA") orelse return error.IraUnresolved;
+    const roth_ts = out.get("Sample Roth") orelse return error.RothUnresolved;
+
+    // Each change attributed to the commit that actually made it.
+    try std.testing.expectEqual(commits[1].timestamp, ira_ts); // B
+    try std.testing.expectEqual(commits[0].timestamp, roth_ts); // C
+    try std.testing.expect(ira_ts < roth_ts);
+}

src/git.zig

@@ -126,8 +126,11 @@ pub const CommitRange = struct {
 /// out by hooks that runs git against a different repo must explicitly
 /// opt out of the inherited env.
 ///
+/// Exposed (pub) so the git-in-test-repo helpers in `testutil/git.zig`
+/// reuse this exact scrubbing rather than re-implementing it.
+///
 /// Caller owns the returned map; free with `.deinit()`.
-fn scrubbedEnv(
+pub fn scrubbedEnv(
     allocator: std.mem.Allocator,
     base: *const std.process.Environ.Map,
 ) std.mem.Allocator.Error!std.process.Environ.Map {

src/portfolio_loader.zig

@@ -59,6 +59,7 @@ const zfin = @import("root.zig");
 const framework = @import("commands/framework.zig");
 const stderr = @import("stderr.zig");
 const git = @import("git.zig");
+const test_git = @import("testutil/git.zig");
 const enrichSplits = @import("models/portfolio.zig").enrichSplits;
 
 // ── Portfolio loading ────────────────────────────────────────
@@ -806,96 +807,11 @@ test "loadFromBytes: all-empty bytes returns empty portfolio" {
 // Kept minimal (one happy-path, one missing-file case); the
 // bulk of the logic is in `loadFromBytes`, covered above.
 
-/// Build an environment map from the parent process with the GIT_*
-/// variables stripped out.
-///
-/// When these tests run inside a git hook (pre-commit, prek, etc.),
-/// the hook runner sets `GIT_INDEX_FILE`, `GIT_DIR`, and
-/// `GIT_WORK_TREE` to point at the outer repo's staging state. Git
-/// inherits those env vars unconditionally - `git -C <tmpdir>`
-/// changes the CWD but does NOT clear these env vars. The result is
-/// that `git init` in our temp dir succeeds but subsequent `git
-/// add`/`git commit` operate against the OUTER repo's index, with
-/// blob references that don't exist in our temp dir's object store
-/// ("invalid object 100644 <hash> for '<outer-repo-path>'").
-///
-/// The hook runner can't fix this for us; per upstream guidance,
-/// hooks (and code shelled out by hooks) that run git against a
-/// different repo must explicitly opt out of the inherited env. See
-/// https://github.com/j178/prek/issues/1786 for the prek-specific
-/// instance and https://pre-commit.com/ for the analogous pre-commit
-/// guidance.
-fn buildScrubbedEnv(allocator: std.mem.Allocator) !std.process.Environ.Map {
-    var map = try testing.environ.createMap(allocator);
-    errdefer map.deinit();
-
-    // Strip every GIT_* variable. Iterating-while-removing isn't
-    // supported on the underlying ArrayHashMap, so collect first
-    // then remove. Keys are duped because `swapRemove` frees the
-    // map's owned key buffer, which would otherwise leave us with
-    // dangling pointers in `keys_to_remove`.
-    var keys_to_remove: std.ArrayList([]u8) = .empty;
-    defer {
-        for (keys_to_remove.items) |k| allocator.free(k);
-        keys_to_remove.deinit(allocator);
-    }
-
-    var it = map.iterator();
-    while (it.next()) |entry| {
-        if (std.mem.startsWith(u8, entry.key_ptr.*, "GIT_")) {
-            try keys_to_remove.append(allocator, try allocator.dupe(u8, entry.key_ptr.*));
-        }
-    }
-    for (keys_to_remove.items) |key| _ = map.swapRemove(key);
-
-    return map;
-}
-
-/// Run a one-shot git command in `cwd` for test setup. Panics on
-/// failure - these tests can't proceed without a working repo.
-///
-/// Uses `buildScrubbedEnv` to drop inherited `GIT_*` env vars; see
-/// that function's doc comment for why this matters under git
-/// hooks.
-fn gitInTestRepo(allocator: std.mem.Allocator, cwd: []const u8, argv: []const []const u8) !void {
-    const full_argv = try allocator.alloc([]const u8, argv.len + 3);
-    defer allocator.free(full_argv);
-    full_argv[0] = "git";
-    full_argv[1] = "-C";
-    full_argv[2] = cwd;
-    @memcpy(full_argv[3..], argv);
-
-    var env_map = try buildScrubbedEnv(allocator);
-    defer env_map.deinit();
-
-    const result = try std.process.run(allocator, testing.io, .{
-        .argv = full_argv,
-        .environ_map = &env_map,
-        .stdout_limit = .limited(64 * 1024),
-    });
-    defer allocator.free(result.stdout);
-    defer allocator.free(result.stderr);
-    switch (result.term) {
-        .exited => |code| if (code != 0) {
-            std.log.err("git command failed (code {d}): {s}\nstderr: {s}", .{ code, std.mem.join(allocator, " ", argv) catch "?", result.stderr });
-            return error.GitFailed;
-        },
-        else => return error.GitFailed,
-    }
-}
-
 test "loadPortfolioFromPathsAtRev: union of two committed files at HEAD" {
     const allocator = testing.allocator;
 
     // Skip if `git` isn't on PATH (CI sandbox without git).
-    {
+    if (!test_git.available(allocator)) return;
-        const probe = std.process.run(allocator, testing.io, .{
-            .argv = &.{ "git", "--version" },
-            .stdout_limit = .limited(1024),
-        }) catch return;
-        defer allocator.free(probe.stdout);
-        defer allocator.free(probe.stderr);
-    }
 
     var tmp = std.testing.tmpDir(.{});
     defer tmp.cleanup();
@@ -917,12 +833,12 @@ test "loadPortfolioFromPathsAtRev: union of two committed files at HEAD" {
     try tmp.dir.writeFile(testing.io, .{ .sub_path = "portfolio.srf", .data = file_a });
     try tmp.dir.writeFile(testing.io, .{ .sub_path = "portfolio_other.srf", .data = file_b });
 
-    try gitInTestRepo(allocator, dir, &.{ "init", "-q" });
+    try test_git.run(allocator, dir, null, &.{ "init", "-q" });
-    try gitInTestRepo(allocator, dir, &.{ "config", "user.email", "test@example.com" });
+    try test_git.run(allocator, dir, null, &.{ "config", "user.email", "test@example.com" });
-    try gitInTestRepo(allocator, dir, &.{ "config", "user.name", "Test" });
+    try test_git.run(allocator, dir, null, &.{ "config", "user.name", "Test" });
-    try gitInTestRepo(allocator, dir, &.{ "config", "commit.gpgsign", "false" });
+    try test_git.run(allocator, dir, null, &.{ "config", "commit.gpgsign", "false" });
-    try gitInTestRepo(allocator, dir, &.{ "add", "portfolio.srf", "portfolio_other.srf" });
+    try test_git.run(allocator, dir, null, &.{ "add", "portfolio.srf", "portfolio_other.srf" });
-    try gitInTestRepo(allocator, dir, &.{ "commit", "-q", "-m", "initial" });
+    try test_git.run(allocator, dir, null, &.{ "commit", "-q", "-m", "initial" });
 
     const p1 = try std.fs.path.join(allocator, &.{ dir, "portfolio.srf" });
     defer allocator.free(p1);
@@ -950,14 +866,7 @@ test "loadPortfolioFromPathsAtRev: file added later is silently skipped at earli
     const allocator = testing.allocator;
 
     // Skip if `git` isn't on PATH.
-    {
+    if (!test_git.available(allocator)) return;
-        const probe = std.process.run(allocator, testing.io, .{
-            .argv = &.{ "git", "--version" },
-            .stdout_limit = .limited(1024),
-        }) catch return;
-        defer allocator.free(probe.stdout);
-        defer allocator.free(probe.stderr);
-    }
 
     var tmp = std.testing.tmpDir(.{});
     defer tmp.cleanup();
@@ -973,12 +882,12 @@ test "loadPortfolioFromPathsAtRev: file added later is silently skipped at earli
     ;
     try tmp.dir.writeFile(testing.io, .{ .sub_path = "portfolio.srf", .data = file_a });
 
-    try gitInTestRepo(allocator, dir, &.{ "init", "-q" });
+    try test_git.run(allocator, dir, null, &.{ "init", "-q" });
-    try gitInTestRepo(allocator, dir, &.{ "config", "user.email", "test@example.com" });
+    try test_git.run(allocator, dir, null, &.{ "config", "user.email", "test@example.com" });
-    try gitInTestRepo(allocator, dir, &.{ "config", "user.name", "Test" });
+    try test_git.run(allocator, dir, null, &.{ "config", "user.name", "Test" });
-    try gitInTestRepo(allocator, dir, &.{ "config", "commit.gpgsign", "false" });
+    try test_git.run(allocator, dir, null, &.{ "config", "commit.gpgsign", "false" });
-    try gitInTestRepo(allocator, dir, &.{ "add", "portfolio.srf" });
+    try test_git.run(allocator, dir, null, &.{ "add", "portfolio.srf" });
-    try gitInTestRepo(allocator, dir, &.{ "commit", "-q", "-m", "initial" });
+    try test_git.run(allocator, dir, null, &.{ "commit", "-q", "-m", "initial" });
 
     // Add and commit a second portfolio file. After this commit,
     // commit 1 (where portfolio_other.srf doesn't exist yet) is
@@ -992,8 +901,8 @@ test "loadPortfolioFromPathsAtRev: file added later is silently skipped at earli
         \\
     ;
     try tmp.dir.writeFile(testing.io, .{ .sub_path = "portfolio_other.srf", .data = file_b });
-    try gitInTestRepo(allocator, dir, &.{ "add", "portfolio_other.srf" });
+    try test_git.run(allocator, dir, null, &.{ "add", "portfolio_other.srf" });
-    try gitInTestRepo(allocator, dir, &.{ "commit", "-q", "-m", "add second" });
+    try test_git.run(allocator, dir, null, &.{ "commit", "-q", "-m", "add second" });
 
     const p1 = try std.fs.path.join(allocator, &.{ dir, "portfolio.srf" });
     defer allocator.free(p1);

src/testutil/git.zig

@@ -0,0 +1,76 @@
+//! Test-only helpers for building throwaway git repositories.
+//!
+//! Consolidates the "run a git command in a temp repo" pattern shared by
+//! the portfolio-loader and audit-hygiene tests, so there is exactly one
+//! copy of the `GIT_*` scrubbing + `git -C <dir>` invocation + error
+//! handling. The scrubbing itself is delegated to `git.scrubbedEnv` (the
+//! same routine production `runGit` uses), so test setup and production
+//! agree on how the inherited hook environment is neutralized.
+
+const std = @import("std");
+const git = @import("../git.zig");
+
+/// Returns false when `git` isn't on PATH so a caller can skip (early
+/// return) rather than fail in a sandbox without git:
+///
+///     if (!test_git.available(allocator)) return;
+pub fn available(allocator: std.mem.Allocator) bool {
+    const probe = std.process.run(allocator, std.testing.io, .{
+        .argv = &.{ "git", "--version" },
+        .stdout_limit = .limited(1024),
+    }) catch return false;
+    allocator.free(probe.stdout);
+    allocator.free(probe.stderr);
+    return true;
+}
+
+/// Run `git -C <cwd> <argv...>` for test setup.
+///
+/// Inherited `GIT_*` vars are stripped (via `git.scrubbedEnv`) so the
+/// command works even when the test process runs inside a git hook whose
+/// `GIT_DIR` / `GIT_WORK_TREE` point at the outer repo.
+///
+/// When `date_iso` is non-null it pins both author and committer dates
+/// (e.g. `"2026-02-15T12:00:00"`), which makes commit timestamps
+/// deterministic for history-walk tests. Returns `error.GitFailed`
+/// (after logging the argv + stderr) on a non-zero exit.
+pub fn run(
+    allocator: std.mem.Allocator,
+    cwd: []const u8,
+    date_iso: ?[]const u8,
+    argv: []const []const u8,
+) !void {
+    const full_argv = try allocator.alloc([]const u8, argv.len + 3);
+    defer allocator.free(full_argv);
+    full_argv[0] = "git";
+    full_argv[1] = "-C";
+    full_argv[2] = cwd;
+    @memcpy(full_argv[3..], argv);
+
+    var base = try std.testing.environ.createMap(allocator);
+    defer base.deinit();
+    var env = try git.scrubbedEnv(allocator, &base);
+    defer env.deinit();
+
+    if (date_iso) |d| {
+        try env.put("GIT_AUTHOR_DATE", d);
+        try env.put("GIT_COMMITTER_DATE", d);
+    }
+
+    const result = try std.process.run(allocator, std.testing.io, .{
+        .argv = full_argv,
+        .environ_map = &env,
+        .stdout_limit = .limited(64 * 1024),
+    });
+    defer allocator.free(result.stdout);
+    defer allocator.free(result.stderr);
+    switch (result.term) {
+        .exited => |code| if (code != 0) {
+            const joined = std.mem.join(allocator, " ", argv) catch null;
+            defer if (joined) |j| allocator.free(j);
+            std.log.err("git command failed (code {d}): {s}\nstderr: {s}", .{ code, joined orelse "?", result.stderr });
+            return error.GitFailed;
+        },
+        else => return error.GitFailed,
+    }
+}