From 5706eed3c1581b13177f417519983b8653c8d06f Mon Sep 17 00:00:00 2001 From: Emil Lerch Date: Tue, 18 Aug 2026 01:38:57 -0700 Subject: [PATCH] fix regression in contribution --- src/commands/contributions.zig | 269 ++++++++++++++++++++++++++++++--- 1 file changed, 249 insertions(+), 20 deletions(-) diff --git a/src/commands/contributions.zig b/src/commands/contributions.zig index 2e3bf34..fa3a108 100644 --- a/src/commands/contributions.zig +++ b/src/commands/contributions.zig @@ -2508,30 +2508,50 @@ fn matchCashDestination( }); } -/// Best-effort: find a negative cash_delta or lot_removed on the -/// `from` account with |value| >= amount - tolerance; reclassify to -/// transfer_out. No-op (silent) if no such Change exists - the -/// sending side may not be in portfolio.srf. +/// Best-effort: find the cash that left the `from` account and +/// reclassify it to `transfer_out`. No-op (silent) if no such Change +/// exists - the sending side may not be in portfolio.srf at all, which +/// the format explicitly allows. +/// +/// Restricted to CASH outflows: a negative `cash_delta`, or a removed / +/// closed cash lot (a line drained in full). A `type::cash` record says +/// dollars moved, so a security sale is never its sending leg - selling +/// to raise the cash is a separate event, and the cash decrease is the +/// transfer. Securities moving between accounts is what `type::in_kind` +/// and `matchInKindTransfer` are for, matched by symbol on both sides. +/// +/// The type gate is load-bearing rather than tidy-up. This scan was +/// unreachable while `value()` was 0 for every removal; once outflows +/// carried dollars it went live, and an unrelated sale in the same +/// account became a candidate. Consuming it here would flip it out of +/// `lot_removed`, drop it from the sale-proceeds budget in +/// `matchIntraAccountPurchases`, and make the repurchase it funded read +/// as new money - reintroducing the bug that budget exists to fix. fn tryMatchFromSide( changes: *std.ArrayList(Change), rec: transaction_log.TransferRecord, ) void { - for (changes.items) |*c| switch (c.kind) { - .cash_delta, .lot_removed, .position_closed => { - if (!std.mem.eql(u8, c.account, rec.from)) continue; - const abs_val = @abs(c.value()); - if (abs_val < rec.amount - transfer_amount_tolerance) continue; - // Match: flip to transfer_out. We don't track - // transfer_attributed on the from side (it's - // decorative) but we record the counterpart date so - // the Transfers section can cross-reference. - c.kind = .transfer_out; - c.transfer_attributed = rec.amount; - c.transfer_date = rec.transfer; - return; - }, - else => {}, - }; + for (changes.items) |*c| { + if (c.security_type != .cash) continue; + if (!std.mem.eql(u8, c.account, rec.from)) continue; + // Must be money leaving. `cash_delta` is signed, so an account + // that GAINED cash must not be read as a sending leg; removals + // and closes are outflows by construction. + const outflow: f64 = switch (c.kind) { + .cash_delta => if (c.value() < 0) -c.value() else continue, + .lot_removed, .position_closed => c.face_value, + else => continue, + }; + if (outflow < rec.amount - transfer_amount_tolerance) continue; + // Match: flip to transfer_out. We don't track + // transfer_attributed on the from side (it's decorative) but we + // record the counterpart date so the Transfers section can + // cross-reference. + c.kind = .transfer_out; + c.transfer_attributed = rec.amount; + c.transfer_date = rec.transfer; + return; + } } /// Match a `type::in_kind` transfer record: securities moved between @@ -4291,6 +4311,19 @@ test "computeReport: transfer record takes priority over intra-account netting" } } +/// Sum of everything a window reports as new money. Mirrors +/// `summarizeAttribution`'s buckets without needing a ReportContext, so +/// classifier-level tests can assert on the same figure the CLI prints. +fn attributionTotalForTest(report: Report) f64 { + var total: f64 = 0; + for (report.changes) |c| switch (c.kind) { + .new_stock, .new_cash, .new_cd, .new_option, .cash_contribution, .partial_transfer_in => total += c.attributedValue(), + .new_drip_lot, .drip_confirmed, .rollup_delta => total += c.value(), + else => {}, + }; + return total; +} + // ── Security-sale funding (intra-account reallocation) ─────── // // Case labels A-F below match the scenario table worked out when this @@ -4867,6 +4900,202 @@ test "printCollapsedSales: many retired lots render as one line" { try std.testing.expect(std.mem.indexOf(u8, text, "$4,800.00") != null); } +test "computeReport: a declared cash transfer out does not consume a security sale" { + // `tryMatchFromSide` looks for the sending leg of a declared + // `type::cash` transfer. Its removal arm was written for a fully + // drained CASH lot, whose dollars live in `face_value` - but it was + // unreachable while `value()` was 0 for every removal, so nothing + // pinned that intent. + // + // Now that outflows carry dollars the arm is live, and an unrelated + // security sale in the same account is a candidate. If it wins, the + // sale flips to `transfer_out`, drops out of the sale-proceeds + // budget, and the rebuy it paid for reads as new money - the exact + // bug this whole change set exists to remove. + // + // Here: $50k of cash is declared as moving out of Sample IRA, and + // separately SYM is sold for $100k and NEWSYM bought with the + // proceeds. The transfer must take the cash; the reallocation must + // still net to zero. + var arena_state = std.heap.ArenaAllocator.init(std.testing.allocator); + defer arena_state.deinit(); + const allocator = arena_state.allocator(); + var prices = std.StringHashMap(f64).init(allocator); + defer prices.deinit(); + try prices.put("SYM", 100.0); + + const before = [_]Lot{ + .{ .symbol = "SYM", .shares = 1_000, .open_date = Date.fromYmd(2025, 1, 2), .open_price = 60.0, .account = "Sample IRA" }, + .{ .symbol = "", .shares = 50_000, .open_date = Date.fromYmd(2026, 1, 1), .open_price = 1.0, .security_type = .cash, .account = "Sample IRA" }, + }; + const after = [_]Lot{ + // SYM sold, proceeds straight into NEWSYM. + .{ .symbol = "NEWSYM", .shares = 2_000, .open_date = Date.fromYmd(2026, 5, 3), .open_price = 50.0, .account = "Sample IRA" }, + // The $50k cash left, as declared. + .{ .symbol = "", .shares = 0, .open_date = Date.fromYmd(2026, 1, 1), .open_price = 1.0, .security_type = .cash, .account = "Sample IRA" }, + .{ .symbol = "", .shares = 50_000, .open_date = Date.fromYmd(2026, 1, 1), .open_price = 1.0, .security_type = .cash, .account = "Sample Roth IRA" }, + }; + + const tlog = try transaction_log.parseTransactionLogFile(allocator, + \\#!srfv1 + \\transfer::2026-05-02,type::cash,amount:num:50000,from::Sample IRA,to::Sample Roth IRA,dest_lot::cash + \\ + ); + + const report = try computeReport(allocator, &before, &after, &prices, Date.fromYmd(2026, 5, 4), .{ + .transfer_log = tlog.transfers, + }); + + // The SYM sale must remain a sale, not be eaten as the transfer's + // sending leg. + var sale: ?Change = null; + var buy: ?Change = null; + for (report.changes) |c| { + if (std.mem.eql(u8, c.symbol, "SYM") and isSaleKind(c)) sale = c; + if (c.kind == .new_stock and std.mem.eql(u8, c.symbol, "NEWSYM")) buy = c; + } + try std.testing.expect(sale != null); + try std.testing.expectApproxEqAbs(@as(f64, 100_000.0), sale.?.face_value, 0.01); + + // ...so its proceeds still fund the rebuy, which is not new money. + try std.testing.expect(buy != null); + try std.testing.expectApproxEqAbs(@as(f64, 100_000.0), buy.?.internal_funded, 0.01); + try std.testing.expectApproxEqAbs(@as(f64, 0.0), buy.?.attributedValue(), 0.01); +} + +test "computeReport: an untracked transfer source does not consume a security sale" { + // The dangerous shape. A `type::cash` transfer's sending account is + // allowed to be unmodelled - the docs say a missing source side is + // not an error. But `tryMatchFromSide` scans for ANY qualifying + // outflow on that account, so with no cash change to claim, an + // unrelated security sale is the only candidate left. + // + // Flipping it to `transfer_out` removes it from the sale-proceeds + // budget, and the rebuy it paid for reads as new money again. The + // sending leg of a CASH transfer must be cash: a fully drained cash + // lot (whose dollars live in `face_value`) or a negative + // `cash_delta`, never a security. + var arena_state = std.heap.ArenaAllocator.init(std.testing.allocator); + defer arena_state.deinit(); + const allocator = arena_state.allocator(); + var prices = std.StringHashMap(f64).init(allocator); + defer prices.deinit(); + try prices.put("SYM", 100.0); + + const before = [_]Lot{ + .{ .symbol = "SYM", .shares = 1_000, .open_date = Date.fromYmd(2025, 1, 2), .open_price = 60.0, .account = "Sample IRA" }, + }; + const after = [_]Lot{ + // Reallocation inside Sample IRA: SYM out, NEWSYM in. + .{ .symbol = "NEWSYM", .shares = 2_000, .open_date = Date.fromYmd(2026, 5, 3), .open_price = 50.0, .account = "Sample IRA" }, + // The declared transfer's destination. Its source cash is not + // modelled in the portfolio at all. + .{ .symbol = "", .shares = 50_000, .open_date = Date.fromYmd(2026, 5, 2), .open_price = 1.0, .security_type = .cash, .account = "Sample Roth IRA" }, + }; + + const tlog = try transaction_log.parseTransactionLogFile(allocator, + \\#!srfv1 + \\transfer::2026-05-02,type::cash,amount:num:50000,from::Sample IRA,to::Sample Roth IRA,dest_lot::cash + \\ + ); + + const report = try computeReport(allocator, &before, &after, &prices, Date.fromYmd(2026, 5, 4), .{ + .transfer_log = tlog.transfers, + }); + + var sale: ?Change = null; + var buy: ?Change = null; + for (report.changes) |c| { + if (std.mem.eql(u8, c.symbol, "SYM")) sale = c; + if (c.kind == .new_stock and std.mem.eql(u8, c.symbol, "NEWSYM")) buy = c; + } + + // The sale stays a sale. + try std.testing.expect(sale != null); + try std.testing.expectEqual(ChangeKind.lot_removed, sale.?.kind); + + // And still funds the rebuy, so no new money is reported. + try std.testing.expect(buy != null); + try std.testing.expectApproxEqAbs(@as(f64, 100_000.0), buy.?.internal_funded, 0.01); + try std.testing.expectApproxEqAbs(@as(f64, 0.0), buy.?.attributedValue(), 0.01); +} + +test "computeReport: a fully drained cash lot is the transfer's sending leg" { + // The shape `tryMatchFromSide`'s removal arm was written for, and + // which never worked: when the cash line is spent to the cent the + // user deletes it, so the outflow arrives as a removed cash lot + // rather than a negative `cash_delta`. Its dollars live in + // `face_value` because `value()` was 0 for removals - which is + // exactly why the arm could never fire. + var arena_state = std.heap.ArenaAllocator.init(std.testing.allocator); + defer arena_state.deinit(); + const allocator = arena_state.allocator(); + var prices = std.StringHashMap(f64).init(allocator); + defer prices.deinit(); + + const before = [_]Lot{ + .{ .symbol = "", .shares = 50_000, .open_date = Date.fromYmd(2026, 1, 1), .open_price = 1.0, .security_type = .cash, .account = "Sample IRA" }, + }; + const after = [_]Lot{ + .{ .symbol = "", .shares = 50_000, .open_date = Date.fromYmd(2026, 5, 2), .open_price = 1.0, .security_type = .cash, .account = "Sample Roth IRA" }, + }; + + const tlog = try transaction_log.parseTransactionLogFile(allocator, + \\#!srfv1 + \\transfer::2026-05-02,type::cash,amount:num:50000,from::Sample IRA,to::Sample Roth IRA,dest_lot::cash + \\ + ); + + const report = try computeReport(allocator, &before, &after, &prices, Date.fromYmd(2026, 5, 4), .{ + .transfer_log = tlog.transfers, + }); + + var n_out: usize = 0; + for (report.changes) |c| { + if (c.kind == .transfer_out and std.mem.eql(u8, c.account, "Sample IRA")) n_out += 1; + } + try std.testing.expectEqual(@as(usize, 1), n_out); + + // And the whole move contributes nothing. + try std.testing.expectApproxEqAbs(@as(f64, 0.0), attributionTotalForTest(report), 0.01); +} + +test "computeReport: an account that GAINED cash is not a transfer's sending leg" { + // `cash_delta` is signed and the match used to take its magnitude, + // so an account whose cash went UP could be reported as the source + // of a transfer out. Money leaving is the whole point, so the sign + // has to be checked. + var arena_state = std.heap.ArenaAllocator.init(std.testing.allocator); + defer arena_state.deinit(); + const allocator = arena_state.allocator(); + var prices = std.StringHashMap(f64).init(allocator); + defer prices.deinit(); + + const before = [_]Lot{ + .{ .symbol = "", .shares = 10_000, .open_date = Date.fromYmd(2026, 1, 1), .open_price = 1.0, .security_type = .cash, .account = "Sample IRA" }, + }; + const after = [_]Lot{ + // Sample IRA's cash rose by $60k - it cannot be what funded a + // transfer out of it. + .{ .symbol = "", .shares = 70_000, .open_date = Date.fromYmd(2026, 1, 1), .open_price = 1.0, .security_type = .cash, .account = "Sample IRA" }, + .{ .symbol = "", .shares = 50_000, .open_date = Date.fromYmd(2026, 5, 2), .open_price = 1.0, .security_type = .cash, .account = "Sample Roth IRA" }, + }; + + const tlog = try transaction_log.parseTransactionLogFile(allocator, + \\#!srfv1 + \\transfer::2026-05-02,type::cash,amount:num:50000,from::Sample IRA,to::Sample Roth IRA,dest_lot::cash + \\ + ); + + const report = try computeReport(allocator, &before, &after, &prices, Date.fromYmd(2026, 5, 4), .{ + .transfer_log = tlog.transfers, + }); + + for (report.changes) |c| { + try std.testing.expect(c.kind != .transfer_out); + } +} + test "prepareReport: a lot archived into a sibling portfolio file is one sale" { // End-to-end pin on the multi-file diff. The documented workflow for // a sale is to move the closed lot out of `portfolio.srf` into a