diff --git a/TODO.md b/TODO.md index 82cbaa3..8552a04 100644 --- a/TODO.md +++ b/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 diff --git a/src/commands/audit.zig b/src/commands/audit.zig index 16ab99f..3578ba5 100644 --- a/src/commands/audit.zig +++ b/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