fix issue with single security reported twice on fidelity accounts

TODO.md

@@ -300,31 +300,6 @@ gain. Possible fixes are discussed in the "Contributions diff" TODO
 below — option C there (per-account `cash_is_contribution`) would
 make manually-entered ESPP-style cash additions count correctly.
 
-## Investigate: audit phantom discrepancy on freshly-added lots — priority MEDIUM
-
-**Observed during a weekly audit run:** `zfin audit` flagged a
-discrepancy on a newly-added single-symbol lot that, on inspection,
-was correctly entered in `portfolio.srf` and matched the brokerage.
-The flag was a false positive.
-
-Possible causes (not yet diagnosed):
-
-- Audit logic mishandles lots whose `open_date::` is the audit's
-  reference date (today) — share count is "new" but no transaction
-  history yet supports it from the audit's perspective.
-- The broker's positions CSV was internally inconsistent the morning
-  after a high-movement day. Other lots on the same export may
-  have been affected; only the new lot was noticed because it was new.
-- Some interaction with rollup/DRIP classification on the new lot.
-
-Repro: next time a brand-new lot is added on a heavy-movement day,
-check whether `zfin audit` flags it without cause. Compare the
-audit's reported brokerage figure against the raw CSV value to
-isolate audit-logic bug from CSV-input bug.
-
-Workaround for now: visual inspection. If `portfolio.srf` matches
-the brokerage UI, ignore the audit flag for this cycle and move on.
-
 ## Investigate: `compare --projections` block disagrees with standalone `projections`
 
 **Observed during a weekly review:** The embedded projections block

src/commands/audit.zig

@@ -764,6 +764,62 @@ pub const AccountComparison = struct {
     has_discrepancies: bool,
 };
 
+/// Consolidate broker rows that share a symbol within the same
+/// account into a single position. Some brokers split a single
+/// stock holding into separate "Cash" and "Margin" rows for the
+/// same ticker in the same account — Fidelity does this when a
+/// freshly-credited lot (e.g. an RSU distribution) hasn't yet
+/// cleared settlement (T+1 / T+2) and is therefore considered
+/// un-marginable, while the older settled shares stay in the
+/// margin sub-account. Without consolidation, the audit would
+/// double-count when matching against the portfolio's
+/// account-level aggregate.
+///
+/// Aggregation rules:
+///   - `quantity` and `current_value` are summed across rows
+///     (treating null as 0 for the sum, but preserving null when
+///     no row supplied a value).
+///   - `cost_basis` is summed the same way.
+///   - `is_cash` is OR-ed across rows: any cash row in the group
+///     marks the consolidated entry as cash. In practice a single
+///     symbol is either always-cash (money market) or never (stock),
+///     so this is just defensive.
+///   - `account_number`, `account_name`, `description` are taken
+///     from the first row in the group.
+///
+/// Caller owns the returned ArrayList.
+fn consolidateBySymbol(
+    allocator: std.mem.Allocator,
+    rows: []const BrokeragePosition,
+) !std.ArrayList(BrokeragePosition) {
+    var by_symbol = std.StringHashMap(usize).init(allocator);
+    defer by_symbol.deinit();
+
+    var out: std.ArrayList(BrokeragePosition) = .empty;
+    errdefer out.deinit(allocator);
+
+    for (rows) |bp| {
+        if (by_symbol.get(bp.symbol)) |idx| {
+            const existing = &out.items[idx];
+            // Sum quantity (null + value = value; null + null = null).
+            existing.quantity = sumOptional(existing.quantity, bp.quantity);
+            existing.current_value = sumOptional(existing.current_value, bp.current_value);
+            existing.cost_basis = sumOptional(existing.cost_basis, bp.cost_basis);
+            existing.is_cash = existing.is_cash or bp.is_cash;
+        } else {
+            try by_symbol.put(bp.symbol, out.items.len);
+            try out.append(allocator, bp);
+        }
+    }
+
+    return out;
+}
+
+fn sumOptional(a: ?f64, b: ?f64) ?f64 {
+    if (a == null and b == null) return null;
+    return (a orelse 0) + (b orelse 0);
+}
+
 /// Build per-account comparisons between portfolio.srf and brokerage data.
 pub fn compareAccounts(
     allocator: std.mem.Allocator,
@@ -793,8 +849,38 @@ pub fn compareAccounts(
         try entry.value_ptr.append(allocator, bp);
     }
 
+    // Aggregate same-symbol rows within each account. Some brokers
+    // report a single security as multiple rows when a position
+    // straddles sub-account contexts. The motivating case is
+    // Fidelity's margin-eligible accounts: when a freshly-credited
+    // lot (e.g. an RSU distribution) hasn't yet cleared settlement
+    // (T+1 / T+2), Fidelity classifies the new shares as
+    // un-marginable "Cash" and the older settled shares as
+    // "Margin", reporting them as two CSV rows for the same
+    // ticker in the same account number. Once settlement clears,
+    // the rows usually consolidate back into one — but until
+    // then, the audit needs to consolidate them itself, otherwise
+    // it'd match each broker row independently against the
+    // (already-aggregated) portfolio total and report a phantom
+    // discrepancy on every duplicate. Aggregating here lets the
+    // rest of the comparator stay (account, symbol)-keyed
+    // regardless of how the broker chose to slice the rows.
+    var consolidated_accounts = std.StringHashMap(std.ArrayList(BrokeragePosition)).init(allocator);
+    defer {
+        var it = consolidated_accounts.valueIterator();
+        while (it.next()) |v| v.deinit(allocator);
+        consolidated_accounts.deinit();
+    }
+    {
+        var acct_it = brokerage_accounts.iterator();
+        while (acct_it.next()) |kv| {
+            const consolidated = try consolidateBySymbol(allocator, kv.value_ptr.items);
+            try consolidated_accounts.put(kv.key_ptr.*, consolidated);
+        }
+    }
+
     // For each brokerage account, find the matching portfolio account and compare
-    var acct_iter = brokerage_accounts.iterator();
+    var acct_iter = consolidated_accounts.iterator();
     while (acct_iter.next()) |kv| {
         const acct_num = kv.key_ptr.*;
         const broker_positions = kv.value_ptr.items;
@@ -2434,6 +2520,72 @@ test "parseFidelityCsv cash account type is not cash position" {
     try std.testing.expectApproxEqAbs(@as(f64, 190), positions[0].quantity.?, 0.01);
 }
 
+test "consolidateBySymbol: distinct symbols pass through unchanged" {
+    const allocator = std.testing.allocator;
+    const rows = [_]BrokeragePosition{
+        .{ .account_number = "A", .account_name = "Acct", .symbol = "AMZN", .description = "", .quantity = 39, .current_value = 10300, .cost_basis = 10000, .is_cash = false },
+        .{ .account_number = "A", .account_name = "Acct", .symbol = "QTUM", .description = "", .quantity = 100, .current_value = 14000, .cost_basis = 13000, .is_cash = false },
+    };
+    var out = try consolidateBySymbol(allocator, &rows);
+    defer out.deinit(allocator);
+
+    try std.testing.expectEqual(@as(usize, 2), out.items.len);
+    try std.testing.expectEqualStrings("AMZN", out.items[0].symbol);
+    try std.testing.expectApproxEqAbs(@as(f64, 39), out.items[0].quantity.?, 0.01);
+    try std.testing.expectEqualStrings("QTUM", out.items[1].symbol);
+}
+
+test "consolidateBySymbol: same-symbol rows aggregate quantity and value" {
+    // Reproduces the Fidelity Cash + Margin double-row scenario
+    // (newly-credited shares pre-settlement live in the cash
+    // sub-account; older settled shares live in the margin
+    // sub-account; both rows share the ticker). Both rows are
+    // AMZN in the same account; consolidation must sum to one
+    // entry of 40 shares total.
+    const allocator = std.testing.allocator;
+    const rows = [_]BrokeragePosition{
+        .{ .account_number = "A", .account_name = "Acct", .symbol = "AMZN", .description = "Cash row", .quantity = 39, .current_value = 10301.46, .cost_basis = 10244.55, .is_cash = false },
+        .{ .account_number = "A", .account_name = "Acct", .symbol = "AMZN", .description = "Margin row", .quantity = 1, .current_value = 264.14, .cost_basis = null, .is_cash = false },
+    };
+    var out = try consolidateBySymbol(allocator, &rows);
+    defer out.deinit(allocator);
+
+    try std.testing.expectEqual(@as(usize, 1), out.items.len);
+    try std.testing.expectEqualStrings("AMZN", out.items[0].symbol);
+    try std.testing.expectApproxEqAbs(@as(f64, 40), out.items[0].quantity.?, 0.01);
+    try std.testing.expectApproxEqAbs(@as(f64, 10565.60), out.items[0].current_value.?, 0.01);
+    // cost_basis was null on the margin row but present on cash row;
+    // null + value = value preserves the cash row's basis.
+    try std.testing.expectApproxEqAbs(@as(f64, 10244.55), out.items[0].cost_basis.?, 0.01);
+    try std.testing.expect(!out.items[0].is_cash);
+}
+
+test "consolidateBySymbol: null quantities collapse to null sum" {
+    // Two cash rows for the same money-market symbol — Fidelity reports
+    // these with quantity null and a dollar value. Sum the values, leave
+    // quantity null.
+    const allocator = std.testing.allocator;
+    const rows = [_]BrokeragePosition{
+        .{ .account_number = "A", .account_name = "Acct", .symbol = "FZFXX", .description = "", .quantity = null, .current_value = 100, .cost_basis = null, .is_cash = true },
+        .{ .account_number = "A", .account_name = "Acct", .symbol = "FZFXX", .description = "", .quantity = null, .current_value = 50, .cost_basis = null, .is_cash = true },
+    };
+    var out = try consolidateBySymbol(allocator, &rows);
+    defer out.deinit(allocator);
+
+    try std.testing.expectEqual(@as(usize, 1), out.items.len);
+    try std.testing.expectEqual(@as(?f64, null), out.items[0].quantity);
+    try std.testing.expectApproxEqAbs(@as(f64, 150), out.items[0].current_value.?, 0.01);
+    try std.testing.expect(out.items[0].is_cash);
+}
+
+test "consolidateBySymbol: empty input returns empty" {
+    const allocator = std.testing.allocator;
+    const rows = [_]BrokeragePosition{};
+    var out = try consolidateBySymbol(allocator, &rows);
+    defer out.deinit(allocator);
+    try std.testing.expectEqual(@as(usize, 0), out.items.len);
+}
+
 test "parseSchwabSummary basic" {
     const data =
         \\Emil Roth