From 08497a327041a4d91666fa3d1066ed3d0d2447a1 Mon Sep 17 00:00:00 2001 From: Emil Lerch Date: Thu, 27 Aug 2026 09:19:01 -0700 Subject: [PATCH] mixed-share treatment in compare --- .pre-commit-config.yaml | 2 +- docs/reference/cli/compare.md | 51 ++++ src/analytics/valuation.zig | 13 + src/commands/compare.zig | 125 ++++++++- src/compare.zig | 338 +++++++++++++++++++++-- src/tui/history_tab.zig | 105 +++++++- src/views/compare.zig | 493 ++++++++++++++++++++++++++++++---- 7 files changed, 1058 insertions(+), 69 deletions(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 31f8e6e..d1cf399 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -21,7 +21,7 @@ repos: language: pygrep entry: ' — ' files: '\.(zig|zon|md|srf|txt|toml|ya?ml)$' - exclude: '^(\.pre-commit-config\.yaml|src/format\.zig|src/views/projections\.zig|docs/reference/cli/milestones\.md)$' + exclude: '^(\.pre-commit-config\.yaml|src/format\.zig|src/views/projections\.zig|docs/reference/cli/milestones\.md|docs/reference/cli/compare\.md)$' - repo: https://github.com/batmac/pre-commit-zig rev: v0.3.0 hooks: diff --git a/docs/reference/cli/compare.md b/docs/reference/cli/compare.md index b5b39ea..e882293 100644 --- a/docs/reference/cli/compare.md +++ b/docs/reference/cli/compare.md @@ -40,6 +40,57 @@ Liquid: $2,350,000.00 -> $2,580,000.00 +$230,000.00 +9.79% With symbols held on both dates, a per-symbol price-change table appears, sorted by percentage move. +### Mixed share classes + +A symbol can cover holdings in more than one share class -- for example +a direct-indexing sleeve, a 401(k) collective trust and plain retail +shares all priced through the same `ticker::` alias with different +`price_ratio` values. Those trade at different per-share prices, so the +group has no single price to show and the price columns render as +`—`. The percentage and dollar figures come from the change in market +value instead. + +Whether that percentage is trustworthy depends on one thing: **did the +share count move?** + +**Share count unchanged** -- the percentage is a real return, and the row +sorts inline with everything else. A group's value is its underlying +price times a fixed basket, so with the basket held still the change in +value *is* the change in price. You just don't get a per-share price +beside it: + +``` + SPYM — -> — +9.40% +$110,000.00 +``` + +**Shares bought or sold** -- the figure now mixes market movement with +your own cash flow and there is no way to separate them. Those rows are +tallied separately, named in a footnote, and always sorted last so you +can disregard them as a block: + +``` + SPYM — -> — +30.69% +$419,605.63 + 18 gainers, 3 losers, 1 share count changed + SPYM: spans share classes AND the share count moved, so the figure is total value change - part market, part shares bought or sold. +``` + +In that second example much of the `+30.69%` is a retail purchase made +partway through the window, not market movement. + +Ordinary (non-mixed) rows are never affected by this: their percentage is +a pure price ratio, so buying or selling during the window cannot +distort it. + +One caveat the tool cannot detect: restating a `price_ratio` without +changing shares also breaks the equivalence, because the ratio itself is +not recorded in a snapshot. If you have just re-based a proxied holding, +treat percentages spanning that date with suspicion. + +This split is a limitation of what snapshots record rather than a display +choice: `price_ratio` is folded into each lot's stored price and the +underlying base price is never written, so a single meaningful price for +the group cannot be reconstructed after the fact. + ## See also - [Snapshots and history](../../guides/snapshots-and-history.md) diff --git a/src/analytics/valuation.zig b/src/analytics/valuation.zig index bb06dbc..a42cad2 100644 --- a/src/analytics/valuation.zig +++ b/src/analytics/valuation.zig @@ -247,6 +247,18 @@ pub const Allocation = struct { account: []const u8 = "", /// Price ratio applied (for display context; 1.0 means no ratio). price_ratio: f64 = 1.0, + /// True when `mergeAllocsBySymbol` folded two or more ratio variants + /// of one ticker into this row. Distinguishes the two ways + /// `price_ratio == 1.0` can arise - a plain unratioed position versus + /// a merged group - which `price_ratio` alone cannot express and + /// which determines whether `current_price` is the lot's effective + /// price or the raw base-ticker price. + /// + /// Consumers that only need "is `current_price` raw?" can keep using + /// `price_ratio == 1.0`; this exists for consumers that must know the + /// price's UNITS, e.g. deciding whether it is comparable against a + /// snapshot's ratio-scaled per-lot price. + merged: bool = false, }; /// Net worth = liquid (stocks + cash + CDs + options) + illiquid assets. @@ -443,6 +455,7 @@ fn mergeAllocsBySymbol(allocs: *std.ArrayList(Allocation), allocator: std.mem.Al .is_manual_price = is_manual, .account = "Multiple", .price_ratio = 1.0, // normalized to base units + .merged = true, }); } diff --git a/src/commands/compare.zig b/src/commands/compare.zig index 81811e9..1231cb2 100644 --- a/src/commands/compare.zig +++ b/src/commands/compare.zig @@ -775,9 +775,34 @@ fn renderGainerLoserSummary(out: *std.Io.Writer, color: bool, cv: view.CompareVi if (cv.loser_count == 1) "" else "s", }); if (cv.flat_count > 0) { - try cli.printFg(out, color, cli.CLR_MUTED, ", {d} flat\n", .{cv.flat_count}); - } else { - try out.print("\n", .{}); + try cli.printFg(out, color, cli.CLR_MUTED, ", {d} flat", .{cv.flat_count}); + } + // Rows whose percentage can't be trusted as a return get their own + // tally rather than hiding inside "flat": a mixed-class group whose + // share count moved has a figure that is part market, part cash flow. + if (cv.unreliable_count > 0) { + try cli.printFg(out, color, cli.CLR_MUTED, ", {d} share count changed", .{cv.unreliable_count}); + } + try out.print("\n", .{}); + + // Name them and say why. Without this the reader has no way to know + // that the last row(s) answer a different question from the rest - + // the columns are unlabelled. + if (cv.unreliable_count > 0) { + try cli.printFg(out, color, cli.CLR_MUTED, " ", .{}); + var written: usize = 0; + for (cv.symbols) |s| { + if (s.pct_reliable) continue; + try cli.printFg(out, color, cli.CLR_MUTED, "{s}{s}", .{ if (written == 0) "" else ", ", s.symbol }); + written += 1; + } + try cli.printFg( + out, + color, + cli.CLR_MUTED, + ": spans share classes AND the share count moved, so {s} total value change - part market, part shares bought or sold.\n", + .{if (cv.unreliable_count == 1) "the figure is" else "the figures are"}, + ); } } @@ -1344,6 +1369,100 @@ test "renderCompare: gainer/loser summary includes flat when present" { try testing.expect(std.mem.indexOf(u8, out, "2 flat") != null); } +test "renderCompare: unreliable rows are tallied and footnoted" { + // The footnote is the only thing telling the reader that the last + // row's percentage answers a different question from the rest - the + // columns are unlabelled - so it needs a test. + const symbols = [_]view.SymbolChange{ + .{ + .symbol = "UP", + .price_then = 100, + .price_now = 110, + .shares_held_throughout = 1, + .pct_change = 0.10, + .dollar_change = 10, + .style = .positive, + }, + .{ + .symbol = "BENCH", + .price_then = 700, + .price_now = 90, + .shares_held_throughout = 100, + .pct_change = 0.25, // VALUE basis, share count moved + .dollar_change = 50_000, + .price_comparable = false, + .pct_reliable = false, + .style = .positive, + }, + }; + const cv = view.CompareView{ + .then_date = Date.fromYmd(2024, 1, 15), + .now_date = Date.fromYmd(2024, 1, 22), + .days_between = 7, + .now_is_live = true, + .liquid = view.buildTotalsRow(300, 310), + .symbols = @constCast(&symbols), + .held_count = 2, + .added_count = 0, + .removed_count = 0, + .gainer_count = 1, + .loser_count = 0, + .flat_count = 0, + .unreliable_count = 1, + }; + + var buf: [4096]u8 = undefined; + var stream = std.Io.Writer.fixed(&buf); + try renderCompare(&stream, false, cv, null); + const out = stream.buffered(); + + // Separate tally, not folded into gainers/losers/flat. + try testing.expect(std.mem.indexOf(u8, out, "1 share count changed") != null); + // Footnote names the symbol and states why it is set aside. + try testing.expect(std.mem.indexOf(u8, out, "BENCH: spans share classes") != null); + try testing.expect(std.mem.indexOf(u8, out, "the share count moved") != null); + try testing.expect(std.mem.indexOf(u8, out, "part market, part shares bought or sold") != null); + // Singular verb for one symbol. + try testing.expect(std.mem.indexOf(u8, out, "the figure is") != null); + // The value-basis percentage IS rendered; the prices are not. + try testing.expect(std.mem.indexOf(u8, out, "+25.00%") != null); + try testing.expect(std.mem.indexOf(u8, out, "$700.00") == null); + try testing.expect(std.mem.indexOf(u8, out, "$90.00") == null); +} + +test "renderCompare: no unreliable rows means no footnote at all" { + const symbols = [_]view.SymbolChange{.{ + .symbol = "UP", + .price_then = 100, + .price_now = 110, + .shares_held_throughout = 1, + .pct_change = 0.10, + .dollar_change = 10, + .style = .positive, + }}; + const cv = view.CompareView{ + .then_date = Date.fromYmd(2024, 1, 15), + .now_date = Date.fromYmd(2024, 1, 22), + .days_between = 7, + .now_is_live = true, + .liquid = view.buildTotalsRow(300, 310), + .symbols = @constCast(&symbols), + .held_count = 1, + .added_count = 0, + .removed_count = 0, + .gainer_count = 1, + .loser_count = 0, + .flat_count = 0, + }; + + var buf: [4096]u8 = undefined; + var stream = std.Io.Writer.fixed(&buf); + try renderCompare(&stream, false, cv, null); + const out = stream.buffered(); + try testing.expect(std.mem.indexOf(u8, out, "share count changed") == null); + try testing.expect(std.mem.indexOf(u8, out, "spans share classes") == null); +} + // ── run() entry-point validation tests ───────────────────────── fn makeTestSvc() zfin.DataService { diff --git a/src/compare.zig b/src/compare.zig index 1a4c94d..0f5fc72 100644 --- a/src/compare.zig +++ b/src/compare.zig @@ -103,8 +103,22 @@ pub fn loadSnapshotSide( /// brings forward. For a plain pre-feature snapshot `value == shares x /// price`, so this degrades exactly to the raw share count. /// -/// Price is taken from the first lot seen (all stock lots of a symbol -/// share the same `price` field in a given snapshot). +/// Price is taken from the first lot seen. That is only meaningful when +/// every stock lot of a symbol shares one `price` in the snapshot, which +/// is USUALLY but not always true: `snapshot.buildSnapshot` keys +/// `LotRow.symbol` on `priceSymbol()` (no `price_ratio` in the key) while +/// writing a ratio-scaled per-lot `.price`, so two lots aliasing one +/// `ticker::` at different `price_ratio`s land under the same symbol with +/// different prices - a direct-indexing sleeve at $775.34 next to retail +/// shares at $90.17. When that happens the group is flagged +/// `mixed_class`, `shares` is a sum across share classes, and the compare +/// view refuses to derive a price move from it. +/// +/// A correct single price is NOT recoverable here: `price_ratio` is +/// folded into `.price` and the base price is never recorded, so +/// base-equivalent share counts cannot be reconstructed from a snapshot. +/// Hence flag-and-abstain rather than a silent wrong answer. `value` is +/// exact either way, which is what the mixed-class rows fall back to. /// /// Lives here rather than in `history.zig` because it emits a /// `view.HoldingMap` - a compare-view-shaped type. The projection- @@ -126,18 +140,59 @@ pub fn aggregateSnapshotStocks( const eff_shares = if (price != 0) lot.value / price else lot.shares; if (out_map.getPtr(lot.symbol)) |h| { h.shares += eff_shares; - // price is already set from first-seen; leave it. + h.value += lot.value; + // A second lot at a DIFFERENT price means this symbol spans + // share classes: the accumulated `shares` is now a sum of + // counts in different units and `h.price` describes only one + // of them. Keep first-seen (nothing better exists) but mark + // the group so no price comparison is drawn from it. + if (h.price != price) h.mixed_class = true; } else { - try out_map.put(lot.symbol, .{ .shares = eff_shares, .price = price }); + try out_map.put(lot.symbol, .{ + .shares = eff_shares, + .price = price, + .value = lot.value, + }); } } } // ── Live-portfolio aggregation ─────────────────────────────── -/// Walk the live portfolio's stock lots, group by `priceSymbol()`, -/// and look up the current price from `prices`. Mirrors the snapshot -/// aggregation so the two sides are apples-to-apples. +/// Walk the live portfolio's stock lots, group by `priceSymbol()`, and +/// look up the current price from `prices`. +/// +/// - `price` = the raw base-ticker price, identical for every lot of +/// the symbol regardless of share class. +/// - `shares` = `SUM(effectiveShares)` - each lot's OWN split-adjusted +/// count, NOT normalized by `price_ratio`. +/// - `value` = `SUM(lot.marketValue(raw_price, false))` - ratio-correct. +/// +/// `shares` is deliberately in own-lot units so it matches what the +/// snapshot side recovers (`value / price` per lot is that lot's own +/// count). That makes the two sides' share counts COMPARABLE, which is +/// what lets `buildSymbolChange` ask "did composition change?" - the +/// question that decides whether a mixed group's value-based return can +/// be trusted as a price return. +/// +/// Consequence: `shares * price == value` holds only when no share-class +/// conversion is involved (every `price_ratio` 1.0), which is exactly the +/// case where `price` is used. For a `mixed_class` group `value` is +/// authoritative and `shares` is a comparability signal, not a quantity +/// to multiply. +/// +/// This used to keep the FIRST lot's ratio-scaled price while summing raw +/// share counts - neither factor coherent, and disagreeing with +/// `mergeAllocsBySymbol` on the same portfolio. +/// +/// ## Why a ratio'd symbol is flagged `mixed_class` here +/// +/// This side's `price` is the BASE price; the snapshot side's is a +/// ratio-SCALED per-lot price (`price_ratio` is folded into +/// `LotRow.price` and the base is never recorded). Those two are the same +/// number only when every ratio involved is 1.0, so any symbol with a +/// non-unit ratio is flagged and the per-share price columns are +/// suppressed for it. /// /// `out_map` keys borrow from the portfolio's lot data (via /// `priceSymbol()`). Caller must keep the portfolio alive as long as @@ -153,11 +208,20 @@ pub fn aggregateLiveStocks( if (!lot.lotIsOpenAsOf(as_of)) continue; const sym = lot.priceSymbol(); const raw_price = prices.get(sym) orelse continue; - const eff_price = lot.effectivePrice(raw_price, false); + const own_shares = lot.effectiveShares(); + const value = lot.marketValue(raw_price, false); + const ratioed = lot.price_ratio != 1.0; if (out_map.getPtr(sym)) |h| { - h.shares += lot.effectiveShares(); + h.shares += own_shares; + h.value += value; + if (ratioed) h.mixed_class = true; } else { - try out_map.put(sym, .{ .shares = lot.effectiveShares(), .price = eff_price }); + try out_map.put(sym, .{ + .shares = own_shares, + .price = raw_price, + .value = value, + .mixed_class = ratioed, + }); } } } @@ -228,7 +292,7 @@ pub fn adjustThenForSplits( const testing = std.testing; -test "aggregateSnapshotStocks: sums shares, filters non-stock, takes first price" { +test "aggregateSnapshotStocks: sums shares and value, filters non-stock" { var map: view.HoldingMap = .init(testing.allocator); defer map.deinit(); @@ -304,6 +368,145 @@ test "aggregateSnapshotStocks: sums shares, filters non-stock, takes first price try testing.expectEqual(@as(f64, 150), (map.get("AAPL") orelse unreachable).shares); try testing.expectEqual(@as(f64, 150.0), (map.get("AAPL") orelse unreachable).price); try testing.expectEqual(@as(f64, 25), (map.get("MSFT") orelse unreachable).shares); + // Values accumulate, and both symbols are single-share-class. + try testing.expectEqual(@as(f64, 22500), (map.get("AAPL") orelse unreachable).value); + try testing.expectEqual(@as(f64, 10000), (map.get("MSFT") orelse unreachable).value); + try testing.expect(!(map.get("AAPL") orelse unreachable).mixed_class); + try testing.expect(!(map.get("MSFT") orelse unreachable).mixed_class); +} + +test "aggregateSnapshotStocks: two prices under one symbol flags mixed_class" { + // The bug. `snapshot.buildSnapshot` keys `LotRow.symbol` on + // `priceSymbol()` with NO `price_ratio` in the key, while writing a + // ratio-scaled per-lot `.price`. So two lots aliasing one `ticker::` at + // different ratios land under one symbol at different prices - and the + // old code silently kept whichever came FIRST (i.e. portfolio file + // order) while summing `value/price` across share classes. + // + // The resulting `shares` is a mixed-unit sum and the retained `price` + // describes ~31% of the value. Compared against another date it + // reported a -17.8% move on a position whose underlying rose 9.4%. + var map: view.HoldingMap = .init(testing.allocator); + defer map.deinit(); + + const lots = [_]snapshot_model.LotRow{ + // Direct-indexing sleeve: institutional NAV. + .{ + .symbol = "BENCH", + .lot_symbol = "DI-IDX", + .account = "Sample Trust", + .security_type = "Stock", + .shares = 709.235272, + .open_price = 461.240208, + .cost_basis = 327_127.82, + .value = 549_901.00, + .price = 775.34, + .quote_date = Date.fromYmd(2026, 8, 26), + }, + // 401k core fund: a different share class of the same index. + .{ + .symbol = "BENCH", + .lot_symbol = "AGG-LC", + .account = "Sample 401(k)", + .security_type = "Stock", + .shares = 5075.077, + .open_price = 97.50, + .cost_basis = 494_820.01, + .value = 928_030.02, + .price = 182.86, + .quote_date = Date.fromYmd(2026, 8, 26), + }, + // Plain retail shares of the same ticker. + .{ + .symbol = "BENCH", + .lot_symbol = "BENCH", + .account = "Sample Roth", + .security_type = "Stock", + .shares = 3426, + .open_price = 90.42, + .cost_basis = 309_778.92, + .value = 308_922.42, + .price = 90.17, + .quote_date = Date.fromYmd(2026, 8, 26), + }, + }; + const snap = snapshot_model.Snapshot{ + .meta = .{ + .snapshot_version = 1, + .as_of_date = Date.fromYmd(2026, 8, 26), + .captured_at = 0, + .zfin_version = "test", + .stale_count = 0, + }, + .totals = &.{}, + .tax_types = &.{}, + .accounts = &.{}, + .lots = @constCast(&lots), + }; + + try aggregateSnapshotStocks(&snap, &map); + const got = map.get("BENCH") orelse return error.TestUnexpectedResult; + + try testing.expect(got.mixed_class); + // `value` is exact regardless - it is what the mixed rows fall back to. + try testing.expectApproxEqAbs(@as(f64, 1_786_853.44), got.value, 0.02); + // ...whereas shares * price is not the value, which is precisely why + // the price columns must not be rendered for this row. + try testing.expect(@abs(got.shares * got.price - got.value) > 1000); +} + +test "aggregateSnapshotStocks: same symbol at the SAME price is not mixed" { + // Blast-radius guard: two lots of one ordinary holding across two + // accounts share a price and must stay comparable. + var map: view.HoldingMap = .init(testing.allocator); + defer map.deinit(); + + const lots = [_]snapshot_model.LotRow{ + .{ + .symbol = "ABC", + .lot_symbol = "ABC", + .account = "Sample IRA", + .security_type = "Stock", + .shares = 100, + .open_price = 50, + .cost_basis = 5000, + .value = 6000, + .price = 60.0, + .quote_date = Date.fromYmd(2026, 8, 26), + }, + .{ + .symbol = "ABC", + .lot_symbol = "ABC", + .account = "Sample Roth", + .security_type = "Stock", + .shares = 50, + .open_price = 55, + .cost_basis = 2750, + .value = 3000, + .price = 60.0, + .quote_date = Date.fromYmd(2026, 8, 26), + }, + }; + const snap = snapshot_model.Snapshot{ + .meta = .{ + .snapshot_version = 1, + .as_of_date = Date.fromYmd(2026, 8, 26), + .captured_at = 0, + .zfin_version = "test", + .stale_count = 0, + }, + .totals = &.{}, + .tax_types = &.{}, + .accounts = &.{}, + .lots = @constCast(&lots), + }; + + try aggregateSnapshotStocks(&snap, &map); + const got = map.get("ABC") orelse return error.TestUnexpectedResult; + try testing.expect(!got.mixed_class); + try testing.expectApproxEqAbs(@as(f64, 150.0), got.shares, 1e-9); + try testing.expectApproxEqAbs(@as(f64, 9000.0), got.value, 1e-9); + try testing.expectApproxEqAbs(got.value, got.shares * got.price, 1e-9); } test "aggregateSnapshotStocks: derives effective shares from value/price (split-captured snapshot)" { @@ -498,12 +701,13 @@ test "aggregateLiveStocks: skips lots with no price in map" { try testing.expect(map.get("OBSCURE") == null); } -test "aggregateLiveStocks: applies price_ratio via effectivePrice" { +test "aggregateLiveStocks: a ratio'd lot keeps own-unit shares and the base price" { var map: view.HoldingMap = .init(testing.allocator); defer map.deinit(); const today = Date.fromYmd(2026, 5, 8); - // CUSIP-style lot with price_ratio: raw price * ratio = effective. + // CUSIP-style lot with price_ratio: 100 institutional shares whose NAV + // is 5x the retail sibling's $30 quote. const lots = [_]zfin.Lot{ .{ .symbol = "02315N600", @@ -518,15 +722,113 @@ test "aggregateLiveStocks: applies price_ratio via effectivePrice" { var prices: std.StringHashMap(f64) = .init(testing.allocator); defer prices.deinit(); - try prices.put("VTTHX", 30.0); // raw price + try prices.put("VTTHX", 30.0); // raw base-ticker price try aggregateLiveStocks(today, &portfolio, &prices, &map); // priceSymbol() returns "VTTHX" (the ticker), not the CUSIP. - const h = map.get("VTTHX") orelse return error.TestUnexpectedResult; - try testing.expectApproxEqAbs(@as(f64, 100), h.shares, 0.01); - // effective price = raw * price_ratio = 30 * 5 = 150 - try testing.expectApproxEqAbs(@as(f64, 150.0), h.price, 0.01); + const got = map.get("VTTHX") orelse return error.TestUnexpectedResult; + // `shares` is the lot's OWN count (100), matching what the snapshot + // side recovers as value/price - that is what makes the two sides' + // counts comparable, and hence what lets the view ask whether + // composition changed. + try testing.expectApproxEqAbs(@as(f64, 100.0), got.shares, 0.01); + // `value` is ratio-correct: 100 * 30 * 5. + try testing.expectApproxEqAbs(@as(f64, 15_000.0), got.value, 0.01); + // `price` is the raw base price, not the institutional NAV. + try testing.expectApproxEqAbs(@as(f64, 30.0), got.price, 0.01); + // Non-unit ratio -> flagged, so the price columns are suppressed and + // `shares * price != value` here is harmless (nothing multiplies them). + try testing.expect(got.mixed_class); + try testing.expect(@abs(got.shares * got.price - got.value) > 1.0); +} + +test "aggregateLiveStocks: three share classes under one ticker sum by value" { + // The shape that motivated the fix: a direct-indexing sleeve, a 401k + // core fund and plain retail shares all aliased to one ticker at + // wildly different ratios. `value` is the ratio-correct total; `shares` + // is the own-unit sum, kept comparable with the snapshot side. + var map: view.HoldingMap = .init(testing.allocator); + defer map.deinit(); + + const today = Date.fromYmd(2026, 8, 26); + const base: f64 = 90.17; + const lots = [_]zfin.Lot{ + .{ + .symbol = "DI-IDX", + .ticker = "BENCH", + .price_ratio = 8.598686503842965, + .shares = 709.235272, + .open_date = Date.fromYmd(2026, 2, 25), + .open_price = 461.240208, + .account = "Sample Trust", + }, + .{ + .symbol = "AGG-LC", + .ticker = "BENCH", + .price_ratio = 2.0279503665333483, + .shares = 5075.077, + .open_date = Date.fromYmd(2026, 2, 26), + .open_price = 97.50, + .account = "Sample 401(k)", + }, + .{ + .symbol = "BENCH", + .shares = 3426, + .open_date = Date.fromYmd(2026, 8, 6), + .open_price = 90.42, + .account = "Sample Roth", + }, + }; + const portfolio: zfin.Portfolio = .{ .lots = @constCast(&lots), .allocator = testing.allocator }; + + var prices: std.StringHashMap(f64) = .init(testing.allocator); + defer prices.deinit(); + try prices.put("BENCH", base); + + try aggregateLiveStocks(today, &portfolio, &prices, &map); + try testing.expectEqual(@as(usize, 1), map.count()); + const got = map.get("BENCH") orelse return error.TestUnexpectedResult; + + var want_shares: f64 = 0; + var want_value: f64 = 0; + for (lots) |lot| { + want_shares += lot.effectiveShares(); // own units + want_value += lot.marketValue(base, false); // ratio-correct + } + try testing.expectApproxEqRel(want_shares, got.shares, 1e-9); + try testing.expectApproxEqRel(want_value, got.value, 1e-9); + try testing.expectApproxEqRel(base, got.price, 1e-9); + try testing.expect(got.mixed_class); + // 1,786,853 of value against 9,210 own-unit shares - the product is + // meaningless, which is why `mixed_class` suppresses the price cells + // and the view works from `value`. + try testing.expect(@abs(got.shares * got.price - got.value) > 1000); +} + +test "aggregateLiveStocks: an unratioed symbol is NOT flagged mixed" { + // Regression guard on the flag's blast radius: the sentinel must not + // leak onto the ~20 ordinary holdings in a real portfolio. + var map: view.HoldingMap = .init(testing.allocator); + defer map.deinit(); + + const today = Date.fromYmd(2026, 5, 8); + const lots = [_]zfin.Lot{ + .{ .symbol = "ABC", .shares = 100, .open_date = Date.fromYmd(2024, 1, 1), .open_price = 50, .account = "Sample IRA" }, + .{ .symbol = "ABC", .shares = 50, .open_date = Date.fromYmd(2024, 6, 1), .open_price = 55, .account = "Sample Roth" }, + }; + const portfolio: zfin.Portfolio = .{ .lots = @constCast(&lots), .allocator = testing.allocator }; + + var prices: std.StringHashMap(f64) = .init(testing.allocator); + defer prices.deinit(); + try prices.put("ABC", 60.0); + + try aggregateLiveStocks(today, &portfolio, &prices, &map); + const got = map.get("ABC") orelse return error.TestUnexpectedResult; + try testing.expect(!got.mixed_class); + try testing.expectApproxEqAbs(@as(f64, 150.0), got.shares, 1e-9); + try testing.expectApproxEqAbs(@as(f64, 60.0), got.price, 1e-9); + try testing.expectApproxEqAbs(@as(f64, 9000.0), got.value, 1e-9); } test "aggregateLiveStocks: empty portfolio yields empty map" { diff --git a/src/tui/history_tab.zig b/src/tui/history_tab.zig index ee89fd6..4296ae3 100644 --- a/src/tui/history_tab.zig +++ b/src/tui/history_tab.zig @@ -803,7 +803,19 @@ fn aggregateFromSummary( for (summary.allocations) |a| { if (a.shares == 0) continue; const price = a.market_value / a.shares; - try out.put(a.symbol, .{ .shares = a.shares, .price = price }); + try out.put(a.symbol, .{ + .shares = a.shares, + .price = price, + .value = a.market_value, + // Mirrors `compare.aggregateLiveStocks`: this side's price is + // only comparable against a snapshot's ratio-scaled per-lot + // price when no share-class conversion is in play. A merged + // group's price is the raw base price; an unmerged ratio'd + // position's is its own effective price. Neither lines up + // with the snapshot convention reliably, so abstain and let + // the exact value delta carry the row. + .mixed_class = a.merged or a.price_ratio != 1.0, + }); } } @@ -1686,6 +1698,97 @@ const testing = std.testing; const Date = zfin.Date; const snapshot = @import("../models/snapshot.zig"); +test "aggregateFromSummary: carries value and flags share-class conversions" { + // This is the TUI's live side of a snapshot-vs-live compare. It had no + // tests at all, and it constructs `compare_view.Holding` by hand - so + // when `Holding` grew `value`, forgetting to set it here would have + // made every mixed row's dollar delta `0 - then_value`. + const allocs = [_]zfin.valuation.Allocation{ + // Ordinary holding: comparable. + .{ + .symbol = "ABC", + .display_symbol = "ABC", + .shares = 100, + .avg_cost = 50, + .current_price = 60, + .market_value = 6000, + .cost_basis = 5000, + .weight = 0.5, + .unrealized_gain_loss = 1000, + .unrealized_return = 0.2, + }, + // Merged group: price is the RAW base price, shares are + // base-equivalent - not the convention snapshots record. + .{ + .symbol = "BENCH", + .display_symbol = "BENCH", + .shares = 200, + .avg_cost = 25, + .current_price = 30, + .market_value = 6000, + .cost_basis = 5000, + .weight = 0.5, + .unrealized_gain_loss = 1000, + .unrealized_return = 0.2, + .merged = true, + }, + // Unmerged but ratio'd: price is its own effective price. + .{ + .symbol = "CIT", + .display_symbol = "CIT", + .shares = 10, + .avg_cost = 100, + .current_price = 150, + .market_value = 1500, + .cost_basis = 1000, + .weight = 0.1, + .unrealized_gain_loss = 500, + .unrealized_return = 0.5, + .price_ratio = 5.0, + }, + // Zero shares: skipped (would divide by zero). + .{ + .symbol = "GONE", + .display_symbol = "GONE", + .shares = 0, + .avg_cost = 0, + .current_price = 0, + .market_value = 0, + .cost_basis = 0, + .weight = 0, + .unrealized_gain_loss = 0, + .unrealized_return = 0, + }, + }; + const summary = zfin.valuation.PortfolioSummary{ + .allocations = @constCast(&allocs), + .total_value = 13500, + .total_cost = 11000, + .unrealized_gain_loss = 2500, + .unrealized_return = 0.227, + .realized_gain_loss = 0, + }; + + var map: compare_view.HoldingMap = .init(testing.allocator); + defer map.deinit(); + try aggregateFromSummary(summary, &map); + + try testing.expectEqual(@as(u32, 3), map.count()); // GONE skipped + + const abc = map.get("ABC") orelse return error.TestUnexpectedResult; + try testing.expectApproxEqAbs(@as(f64, 6000), abc.value, 1e-9); + try testing.expectApproxEqAbs(abc.value, abc.shares * abc.price, 1e-9); + try testing.expect(!abc.mixed_class); + + const bench = map.get("BENCH") orelse return error.TestUnexpectedResult; + try testing.expectApproxEqAbs(@as(f64, 6000), bench.value, 1e-9); + try testing.expect(bench.mixed_class); + + const cit = map.get("CIT") orelse return error.TestUnexpectedResult; + try testing.expectApproxEqAbs(@as(f64, 1500), cit.value, 1e-9); + try testing.expect(cit.mixed_class); +} + test "formatSelectionStatus: count 1 includes commit key" { var buf: [128]u8 = undefined; const msg = try formatSelectionStatus(&buf, 1, "c"); diff --git a/src/views/compare.zig b/src/views/compare.zig index 0bcc644..392242e 100644 --- a/src/views/compare.zig +++ b/src/views/compare.zig @@ -37,7 +37,31 @@ //! don't contribute (matching the "don't count adds" intent), //! shares sold don't either. //! -//! Sorted by `pct_change` descending - biggest winners first. +//! ### Mixed share classes +//! +//! A symbol whose lots span share classes (a `ticker::` alias shared by +//! holdings at different `price_ratio`s) has no single meaningful +//! per-share price. Such a row is marked `price_comparable = false`: the +//! two price cells render as the no-data sentinel, and `pct_change` / +//! `dollar_change` switch to a VALUE basis - `value_now / value_then - 1` +//! and `value_now - value_then`. +//! +//! Whether that percentage is a real return then depends on one thing: +//! did the share count move? A group's value is +//! `base_price * SUM(shares_i * ratio_i)`, so with shares and ratios +//! fixed the value ratio reduces EXACTLY to the underlying price ratio. +//! Such a row is `pct_reliable` and sorts inline with everything else - +//! its percentage is as good as any price-derived one, it just has no +//! single price to show alongside. +//! +//! If shares were bought or sold, the figure absorbs that flow and no +//! longer isolates the market. Those rows are `pct_reliable = false`: +//! tallied separately, sorted last, and named in a footnote, so they are +//! easy to set aside as a block. `CompareView.unreliable_count` exists so +//! renderers can surface that. +//! +//! Sorted by `pct_change` descending - biggest winners first, with +//! unreliable rows last. //! //! ## Contract //! @@ -68,15 +92,57 @@ pub const SymbolChange = struct { price_then: f64, price_now: f64, /// `min(shares_then, shares_now)` - the continuously-held floor. - /// Drives `dollar_change`. + /// Drives `dollar_change` when `price_comparable`. shares_held_throughout: f64, /// Ratio, NOT percentage. `0.05` means +5%. Renderers multiply by /// 100 at format time via `fmtSignedPercentBuf` or similar. + /// + /// BASIS DEPENDS ON `price_comparable`: + /// - true: `price_now / price_then - 1` - price-only, unaffected by + /// share-count changes between the dates. + /// - false: `value_now / value_then - 1` - total value change, which + /// ALSO includes anything bought or sold in the window. + /// No price-only figure is derivable for such a group. pct_change: f64, - /// `shares_held_throughout * (price_now - price_then)`. Signed. + /// Signed. Meaning depends on `price_comparable`: + /// - true: `shares_held_throughout * (price_now - price_then)` - + /// the price-only impact on continuously-held shares. + /// - false: `value_now - value_then` - the TOTAL value change, on + /// the same basis as `pct_change` above. dollar_change: f64, - /// `.positive` when pct_change > 0, `.negative` when < 0, - /// `.muted` when exactly zero. + /// False when either side of the comparison is a `mixed_class` + /// holding, meaning no single per-share price describes the group. + /// `buildSymbolRowCells` emits the no-data sentinel for the two price + /// cells, and `pct_change` / `dollar_change` switch to a VALUE basis. + /// + /// Gates on EITHER side on purpose: a snapshot's per-lot price and the + /// live side's base-ticker price are not comparable quantities, so one + /// mixed side poisons the pair even if the other is clean. + price_comparable: bool = true, + /// Whether `pct_change` can be read as a genuine return. + /// + /// Always true for `price_comparable` rows - a price ratio is + /// share-count-independent by construction. + /// + /// For a mixed-class row it is true exactly when the share count did + /// NOT move between the two dates. That is not a heuristic: a group's + /// value is `base_price * SUM(shares_i * ratio_i)`, so with the shares + /// and ratios fixed, `value_now / value_then` reduces exactly to + /// `base_now / base_then` - the real underlying return. Once shares + /// move, the ratio also absorbs whatever was bought or sold and no + /// longer isolates the market. + /// + /// Renderers use this to decide sort position and whether to warn: + /// reliable rows sort inline with everything else and count toward + /// gainers/losers, unreliable ones sort last and are called out. + /// + /// Caveat: a `price_ratio` RESTATEMENT (rewriting the ratio without + /// changing shares) also breaks the reduction, and this cannot detect + /// that - the ratio is not recorded in a snapshot. + pct_reliable: bool = true, + /// `.positive` when the driving figure is > 0, `.negative` when < 0, + /// `.muted` when zero. Driven by `pct_change` when comparable, by + /// `dollar_change` otherwise. style: StyleIntent, }; @@ -141,12 +207,26 @@ pub const CompareView = struct { removed_count: usize, /// Number of held-throughout symbols with `pct_change > flat_threshold`. /// Intended for the per-symbol summary footer ("21 gainers, 5 losers"). + /// Counts only `price_comparable` rows. gainer_count: usize = 0, /// Number of held-throughout symbols with `pct_change < -flat_threshold`. + /// Counts only `price_comparable` rows. loser_count: usize = 0, /// Number of held-throughout symbols with `|pct_change| <= flat_threshold`. - /// `gainer_count + loser_count + flat_count == held_count`. + /// Counts only `price_comparable` rows. flat_count: usize = 0, + /// Number of held-throughout symbols whose percentage cannot be read + /// as a return (`SymbolChange.pct_reliable == false`): a mixed-class + /// group whose share count moved, so the figure mixes market movement + /// with whatever was bought or sold. + /// + /// Its own bucket rather than folded into `flat_count`, which would + /// understate the gainers/losers tally. Note a mixed-class row with a + /// STATIC share count is NOT counted here - it buckets as a normal + /// gainer/loser/flat, because its value ratio is a genuine return. + /// + /// `gainer_count + loser_count + flat_count + unreliable_count == held_count`. + unreliable_count: usize = 0, /// Optional contributions-vs-gains breakdown of `liquid.delta`. /// Populated by the CLI from `computeAttributionSpec` when a git repo /// is available; always null in unit-tested / TUI flows. @@ -221,12 +301,34 @@ pub fn buildBucketLabel( /// table crosses the threshold. pub const flat_threshold: f64 = 0.0001; -/// One entry in a holdings snapshot - total shares held of `symbol` and -/// the per-share price at that moment. Caller-populated; the view model -/// doesn't know or care where the numbers came from. +/// One entry in a holdings snapshot - total shares held of `symbol`, the +/// per-share price at that moment, and the total value. Caller-populated; +/// the view model doesn't know or care where the numbers came from. +/// +/// `shares * price == value` must hold. That is not decoration: it is the +/// only thing that makes `price` meaningful, and it is exactly what broke +/// when a symbol's lots spanned share classes. See `mixed_class`. pub const Holding = struct { shares: f64, price: f64, + /// Total market value. Carried explicitly rather than recomputed as + /// `shares * price`, because for a `mixed_class` group that product + /// is meaningless while the value is still exact. + value: f64 = 0, + /// True when this symbol's lots did NOT share a single per-share + /// price on this side of the comparison - i.e. the aggregation summed + /// across share classes (a direct-indexing sleeve at $775.34, a 401k + /// CIT at $182.86 and retail shares at $90.17 can all sit under one + /// `ticker::`). For such a group `shares` is a sum of counts in + /// different units and no single `price` describes it, so + /// `buildSymbolChange` refuses to compare prices and the renderer + /// shows the no-data sentinel instead of a fabricated move. + /// + /// A single price genuinely cannot be recovered for these: snapshot + /// `LotRow` folds `price_ratio` into its per-lot `price` and never + /// records the base price, so base-equivalent share counts are not + /// derivable from an existing snapshot at all. + mixed_class: bool = false, }; /// Symbol -> Holding. String keys are caller-owned; keep them alive as @@ -235,27 +337,50 @@ pub const HoldingMap = std.StringHashMap(Holding); // ── Pure builders ──────────────────────────────────────────── -/// Compute a single per-symbol change from the raw inputs. +/// Compute a single per-symbol change from the two sides' holdings. /// -/// The pct-change denominator is `price_then`. If `price_then` is zero -/// (shouldn't happen for stocks but guards against bad data), the -/// pct_change is reported as 0 rather than a NaN/Inf leaking into the -/// sort comparator. -pub fn buildSymbolChange( - symbol: []const u8, - shares_then: f64, - price_then: f64, - shares_now: f64, - price_now: f64, -) SymbolChange { - const held = @min(shares_then, shares_now); - const pct = if (price_then != 0) (price_now / price_then - 1.0) else 0.0; - const dollar = held * (price_now - price_then); +/// The pct-change denominator is `then.price`. If it is zero (shouldn't +/// happen for stocks but guards against bad data), the pct_change is +/// reported as 0 rather than a NaN/Inf leaking into the sort comparator. +/// +/// When EITHER side is `mixed_class` no single per-share price describes +/// the group, so the price cells are suppressed and both figures switch +/// to a VALUE basis: `pct_change = value_now / value_then - 1`, +/// `dollar_change = value_now - value_then`. +/// +/// Whether that percentage is a trustworthy return then depends on +/// whether the share count moved - see `SymbolChange.pct_reliable`. With +/// composition fixed the value ratio reduces exactly to the underlying +/// price ratio, so the row belongs inline with the price-basis rows. If +/// shares were bought or sold, the figure absorbs that flow and the row +/// is marked unreliable so a renderer can set it aside. +pub fn buildSymbolChange(symbol: []const u8, then: Holding, now: Holding) SymbolChange { + const held = @min(then.shares, now.shares); + const comparable = !then.mixed_class and !now.mixed_class; + + if (!comparable) { + const dollar = now.value - then.value; + const pct = if (then.value != 0) (now.value / then.value - 1.0) else 0.0; + return .{ + .symbol = symbol, + .price_then = then.price, + .price_now = now.price, + .shares_held_throughout = held, + .pct_change = pct, + .dollar_change = dollar, + .price_comparable = false, + .pct_reliable = sharesUnchanged(then.shares, now.shares), + .style = if (dollar > 0) .positive else if (dollar < 0) .negative else .muted, + }; + } + + const pct = if (then.price != 0) (now.price / then.price - 1.0) else 0.0; + const dollar = held * (now.price - then.price); const style: StyleIntent = if (pct > 0) .positive else if (pct < 0) .negative else .muted; return .{ .symbol = symbol, - .price_then = price_then, - .price_now = price_now, + .price_then = then.price, + .price_now = now.price, .shares_held_throughout = held, .pct_change = pct, .dollar_change = dollar, @@ -263,6 +388,22 @@ pub fn buildSymbolChange( }; } +/// Whether two share counts are the same position rather than a +/// purchase, sale or DRIP. +/// +/// Relative tolerance, because both sides arrive through float +/// arithmetic: the snapshot side recovers each lot's count as +/// `value / price` and the live side sums `effectiveShares()`, so a +/// genuinely static holding can differ in the last bits. The absolute +/// floor covers counts near zero. +/// +/// A real purchase is orders of magnitude above this - the smallest +/// meaningful move is a fractional DRIP share, not 1e-6 of a share. +fn sharesUnchanged(a: f64, b: f64) bool { + const diff = @abs(a - b); + return diff <= @max(1e-6, @abs(a) * 1e-6); +} + /// Compute the liquid totals row. Safe when `then == 0` (pct -> 0 rather /// than NaN). pub fn buildTotalsRow(then: f64, now: f64) TotalsRow { @@ -298,6 +439,11 @@ pub fn forwardAdjustThen(then_map: *HoldingMap, factors: *const std.StringHashMa if (f == 1.0 or f == 0.0) continue; e.value_ptr.shares *= f; e.value_ptr.price /= f; + // `value` is deliberately untouched: a split changes the share + // count and the per-share price by reciprocal factors, so the + // holding's total value is unchanged. This keeps the + // `shares * price == value` invariant intact through the + // adjustment. } } @@ -338,35 +484,43 @@ pub fn buildCompareView( const sym = e.key_ptr.*; const then_h = e.value_ptr.*; if (now_map.get(sym)) |now_h| { - try changes.append(allocator, buildSymbolChange( - sym, - then_h.shares, - then_h.price, - now_h.shares, - now_h.price, - )); + try changes.append(allocator, buildSymbolChange(sym, then_h, now_h)); } else { removed += 1; } } - // Sort by pct_change descending. Stable is fine; stability isn't - // semantically relevant here but is cheaper in the not-all-unique case. + // Sort by pct_change descending, with rows whose percentage can't be + // trusted as a return pushed to the bottom. Those are mixed-class + // groups whose share count moved, so the figure absorbs whatever was + // bought or sold - parking them last keeps them out of the "biggest + // mover" slot and makes them easy to disregard as a block. std.mem.sort(SymbolChange, changes.items, {}, struct { fn lt(_: void, a: SymbolChange, b: SymbolChange) bool { + if (a.pct_reliable != b.pct_reliable) return a.pct_reliable; return a.pct_change > b.pct_change; } }.lt); - // Bucket held-throughout rows into gainers / losers / flat using - // `flat_threshold` so that cent-rounding noise on a high-priced - // position doesn't get counted as a win or a loss. Computed after - // the sort purely for locality - buckets are independent of order. + // Bucket rows into gainers / losers / flat using `flat_threshold` so + // that cent-rounding noise on a high-priced position doesn't get + // counted as a win or a loss. Computed after the sort purely for + // locality - buckets are independent of order. + // + // A mixed-class row with a STATIC share count buckets normally: its + // value ratio reduces exactly to the underlying price ratio, so it is + // a real return even though no single per-share price exists to + // display. Only rows whose share count moved are set aside, because + // there the percentage is part market and part cash flow with no way + // to separate them. var gainers: usize = 0; var losers: usize = 0; var flats: usize = 0; + var unreliable: usize = 0; for (changes.items) |c| { - if (c.pct_change > flat_threshold) { + if (!c.pct_reliable) { + unreliable += 1; + } else if (c.pct_change > flat_threshold) { gainers += 1; } else if (c.pct_change < -flat_threshold) { losers += 1; @@ -389,6 +543,7 @@ pub fn buildCompareView( .gainer_count = gainers, .loser_count = losers, .flat_count = flats, + .unreliable_count = unreliable, }; } @@ -470,8 +625,20 @@ test "buildBucketLabel: missing bucket_start on non-daily tier falls back to ISO try testing.expectEqualStrings("2025-03-28", lbl); } +/// A single-share-class `Holding` for tests: `value` derived so the +/// `shares * price == value` invariant holds, `mixed_class` false. +fn h(shares: f64, price: f64) Holding { + return .{ .shares = shares, .price = price, .value = shares * price }; +} + +/// A mixed-share-class `Holding`: `value` given explicitly because +/// `shares * price` is meaningless for such a group. +fn hMixed(shares: f64, price: f64, value: f64) Holding { + return .{ .shares = shares, .price = price, .value = value, .mixed_class = true }; +} + test "buildSymbolChange: positive price move, shares stable" { - const c = buildSymbolChange("AAPL", 100, 150.0, 100, 165.0); + const c = buildSymbolChange("AAPL", h(100, 150.0), h(100, 165.0)); try testing.expectEqualStrings("AAPL", c.symbol); try testing.expectApproxEqAbs(@as(f64, 0.10), c.pct_change, 1e-9); try testing.expectApproxEqAbs(@as(f64, 1500.0), c.dollar_change, 1e-9); @@ -480,14 +647,14 @@ test "buildSymbolChange: positive price move, shares stable" { } test "buildSymbolChange: negative move" { - const c = buildSymbolChange("NFLX", 10, 500.0, 10, 400.0); + const c = buildSymbolChange("NFLX", h(10, 500.0), h(10, 400.0)); try testing.expectApproxEqAbs(@as(f64, -0.20), c.pct_change, 1e-9); try testing.expectApproxEqAbs(@as(f64, -1000.0), c.dollar_change, 1e-9); try testing.expectEqual(StyleIntent.negative, c.style); } test "buildSymbolChange: zero price move -> muted style, zero dollar" { - const c = buildSymbolChange("VTI", 50, 240.0, 50, 240.0); + const c = buildSymbolChange("VTI", h(50, 240.0), h(50, 240.0)); try testing.expectApproxEqAbs(@as(f64, 0.0), c.pct_change, 1e-9); try testing.expectApproxEqAbs(@as(f64, 0.0), c.dollar_change, 1e-9); try testing.expectEqual(StyleIntent.muted, c.style); @@ -495,7 +662,7 @@ test "buildSymbolChange: zero price move -> muted style, zero dollar" { test "buildSymbolChange: shares_held is min (added shares between dates)" { // Started with 100, added 50, now at 150. Held-throughout floor = 100. - const c = buildSymbolChange("MSFT", 100, 400.0, 150, 420.0); + const c = buildSymbolChange("MSFT", h(100, 400.0), h(150, 420.0)); try testing.expectEqual(@as(f64, 100), c.shares_held_throughout); // dollar = 100 * (420-400) = 2000, not 150 * 20 = 3000 try testing.expectApproxEqAbs(@as(f64, 2000.0), c.dollar_change, 1e-9); @@ -503,14 +670,14 @@ test "buildSymbolChange: shares_held is min (added shares between dates)" { test "buildSymbolChange: shares_held is min (sold shares between dates)" { // Started with 200, sold down to 50. Held-throughout floor = 50. - const c = buildSymbolChange("GOOG", 200, 160.0, 50, 180.0); + const c = buildSymbolChange("GOOG", h(200, 160.0), h(50, 180.0)); try testing.expectEqual(@as(f64, 50), c.shares_held_throughout); // dollar = 50 * (180-160) = 1000 try testing.expectApproxEqAbs(@as(f64, 1000.0), c.dollar_change, 1e-9); } test "buildSymbolChange: zero price_then doesn't NaN" { - const c = buildSymbolChange("BAD", 10, 0.0, 10, 50.0); + const c = buildSymbolChange("BAD", h(10, 0.0), h(10, 50.0)); try testing.expectEqual(@as(f64, 0.0), c.pct_change); try testing.expectEqual(StyleIntent.muted, c.style); // dollar_change is still 10 * 50 = 500 - that's the true held-throughout @@ -518,6 +685,210 @@ test "buildSymbolChange: zero price_then doesn't NaN" { try testing.expectApproxEqAbs(@as(f64, 500.0), c.dollar_change, 1e-9); } +test "buildSymbolChange: a mixed side abstains from the price comparison" { + // The two prices here are the real ones from the SPYM case: the "then" + // snapshot's first-seen lot was the direct-index sleeve at $708.72, the + // "now" side's base price is $90.17. Subtracting those is meaningless - + // it reported -87% on a position whose underlying rose. + const then = hMixed(5793.5618, 708.72, 1_367_247.82); + const now = hMixed(19_816.496, 90.17, 1_786_853.45); + const c = buildSymbolChange("BENCH", then, now); + + try testing.expect(!c.price_comparable); + // Both figures switch to a VALUE basis - exact, but answering a + // different question than the price-basis rows (it includes the 3,426 + // retail shares bought partway through the window). + try testing.expectApproxEqAbs(@as(f64, 419_605.63), c.dollar_change, 0.02); + try testing.expectApproxEqAbs(@as(f64, 0.306898), c.pct_change, 1e-5); + try testing.expectEqual(StyleIntent.positive, c.style); + // The old price-basis math would have produced this instead: + const old_dollar = @min(then.shares, now.shares) * (now.price - then.price); + try testing.expect(old_dollar < 0); // wrong sign, on a position that GAINED + const old_pct = now.price / then.price - 1.0; + try testing.expect(old_pct < 0); // and a wrong-signed percentage +} + +test "buildSymbolChange: one mixed side is enough to abstain" { + // Gates on EITHER side: a snapshot's ratio-scaled per-lot price and the + // live side's base price are not comparable quantities, so one mixed + // side poisons the pair. + const clean = h(100, 50.0); + const mixed = hMixed(100, 500.0, 12_345.0); + + const a = buildSymbolChange("X", mixed, clean); + try testing.expect(!a.price_comparable); + const b = buildSymbolChange("X", clean, mixed); + try testing.expect(!b.price_comparable); + // Both clean -> normal comparison restored. + const ok = buildSymbolChange("X", clean, h(100, 55.0)); + try testing.expect(ok.price_comparable); + try testing.expectApproxEqAbs(@as(f64, 0.10), ok.pct_change, 1e-9); +} + +test "buildSymbolChange: a mixed row with a value loss reads negative" { + const c = buildSymbolChange("BENCH", hMixed(100, 700.0, 200_000.0), hMixed(200, 90.0, 150_000.0)); + try testing.expect(!c.price_comparable); + try testing.expectApproxEqAbs(@as(f64, -50_000.0), c.dollar_change, 1e-9); + try testing.expectApproxEqAbs(@as(f64, -0.25), c.pct_change, 1e-9); + try testing.expectEqual(StyleIntent.negative, c.style); +} + +test "buildSymbolChange: static shares make a mixed row's percentage reliable" { + // The whole justification: value = base_price * SUM(shares_i * ratio_i). + // Hold the shares and ratios still and value scales EXACTLY with the + // underlying, so value_now/value_then IS the price return - even though + // no single per-share price exists to display. + // + // Here the group is worth 1000 then and 1100 now on an unchanged 10 + // shares, so +10% is the real move. + const c = buildSymbolChange("BENCH", hMixed(10, 100.0, 1000.0), hMixed(10, 800.0, 1100.0)); + try testing.expect(!c.price_comparable); // still no price to show + try testing.expect(c.pct_reliable); // ...but the return is sound + try testing.expectApproxEqAbs(@as(f64, 0.10), c.pct_change, 1e-9); +} + +test "buildSymbolChange: a moved share count makes it unreliable" { + const bought = buildSymbolChange("BENCH", hMixed(10, 100.0, 1000.0), hMixed(25, 800.0, 1500.0)); + try testing.expect(!bought.pct_reliable); + const sold = buildSymbolChange("BENCH", hMixed(25, 100.0, 2500.0), hMixed(10, 800.0, 1100.0)); + try testing.expect(!sold.pct_reliable); + // Even a fractional DRIP share counts - it is still money in. + const drip = buildSymbolChange("BENCH", hMixed(10, 100.0, 1000.0), hMixed(10.25, 800.0, 1100.0)); + try testing.expect(!drip.pct_reliable); +} + +test "buildSymbolChange: float noise in a static share count stays reliable" { + // The two sides reach their counts by different arithmetic - the + // snapshot side divides value by price, the live side sums + // effectiveShares - so a genuinely static holding can differ in the + // last bits. That must not read as a purchase. + const c = buildSymbolChange( + "BENCH", + hMixed(709.235272, 100.0, 1000.0), + hMixed(709.2352720000001, 800.0, 1100.0), + ); + try testing.expect(c.pct_reliable); +} + +test "buildSymbolChange: a price-comparable row is always reliable" { + // A price ratio is share-count-independent by construction, so buying + // more of an ordinary holding must NOT demote it. + const c = buildSymbolChange("MSFT", h(100, 400.0), h(150, 420.0)); + try testing.expect(c.price_comparable); + try testing.expect(c.pct_reliable); + try testing.expectApproxEqAbs(@as(f64, 0.05), c.pct_change, 1e-9); +} + +test "buildSymbolChange: a mixed row with zero then-value doesn't NaN" { + // Value basis divides by `then.value`; guard it the same way the price + // basis guards `then.price`. + const c = buildSymbolChange("BENCH", hMixed(0, 0, 0), hMixed(10, 5, 500.0)); + try testing.expect(!c.price_comparable); + try testing.expectEqual(@as(f64, 0), c.pct_change); + try testing.expectApproxEqAbs(@as(f64, 500.0), c.dollar_change, 1e-9); +} + +test "buildSymbolRowCells: a non-comparable row renders sentinels but a real dollar" { + var p_then: [24]u8 = undefined; + var p_now: [24]u8 = undefined; + var p_pct: [16]u8 = undefined; + var p_dollar: [32]u8 = undefined; + + const s = buildSymbolChange("BENCH", hMixed(100, 700.0, 200_000.0), hMixed(200, 90.0, 250_000.0)); + const cells = buildSymbolRowCells(s, &p_then, &p_now, &p_pct, &p_dollar); + + // The two PRICE cells are sentinels, padded to the column's DISPLAY + // width rather than its byte width - the sentinel is one column in + // three bytes, and the row templates pad by bytes, so an unpadded + // sentinel would skew every column to its right. + try testing.expectEqual(@as(usize, price_w), fmt.displayCols(cells.price_then)); + try testing.expectEqual(@as(usize, price_w), fmt.displayCols(cells.price_now)); + try testing.expect(std.mem.indexOf(u8, cells.price_then, fmt.no_data_sentinel) != null); + try testing.expect(std.mem.indexOf(u8, cells.price_now, fmt.no_data_sentinel) != null); + // Justification matches the spec each cell is fed to. + try testing.expect(std.mem.endsWith(u8, cells.price_then, fmt.no_data_sentinel)); + try testing.expect(std.mem.startsWith(u8, cells.price_now, fmt.no_data_sentinel)); + // The PERCENT cell is a real value-basis figure, rendered exactly like + // any other row (the row template pads it), NOT a sentinel. + // 250,000 / 200,000 - 1 = +25%. + try testing.expectEqualStrings("+25.00%", cells.pct); + // The dollar cell stays real - it is the exact value delta. + try testing.expectEqualStrings("+$50,000.00", cells.dollar); + // And no fabricated price leaks through. + try testing.expect(std.mem.indexOf(u8, cells.price_then, "700") == null); + try testing.expect(std.mem.indexOf(u8, cells.price_now, "90") == null); +} + +test "buildCompareView: mixed rows get their own bucket and sort last" { + var then_map: HoldingMap = .init(testing.allocator); + defer then_map.deinit(); + var now_map: HoldingMap = .init(testing.allocator); + defer now_map.deinit(); + + try then_map.put("WIN", h(10, 100.0)); + try now_map.put("WIN", h(10, 120.0)); // +20% + try then_map.put("LOSE", h(10, 100.0)); + try now_map.put("LOSE", h(10, 80.0)); // -20% + try then_map.put("FLAT", h(10, 100.0)); + try now_map.put("FLAT", h(10, 100.0)); // 0% + // Mixed class, share count STATIC. Its value ratio reduces exactly to + // the underlying price ratio (1100/1000 = +10%), so it is a real + // return and belongs inline with the price-basis rows - it just has no + // single per-share price to display beside it. + try then_map.put("MIXSTATIC", hMixed(10, 100.0, 1000.0)); + try now_map.put("MIXSTATIC", hMixed(10, 800.0, 1100.0)); + // Mixed class, share count MOVED (10 -> 25). Its +50% is part market + // and part purchase with no way to separate them, so it is unreliable. + try then_map.put("MIXBOUGHT", hMixed(10, 100.0, 1000.0)); + try now_map.put("MIXBOUGHT", hMixed(25, 800.0, 1500.0)); + + var view = try buildCompareView( + testing.allocator, + Date.fromYmd(2026, 1, 1), + Date.fromYmd(2026, 8, 1), + false, + 100_000, + 110_000, + &then_map, + &now_map, + ); + defer view.deinit(testing.allocator); + + try testing.expectEqual(@as(usize, 5), view.held_count); + // MIXSTATIC counts as a normal gainer alongside WIN. + try testing.expectEqual(@as(usize, 2), view.gainer_count); + try testing.expectEqual(@as(usize, 1), view.loser_count); + try testing.expectEqual(@as(usize, 1), view.flat_count); + try testing.expectEqual(@as(usize, 1), view.unreliable_count); + // The documented invariant. + try testing.expectEqual( + view.held_count, + view.gainer_count + view.loser_count + view.flat_count + view.unreliable_count, + ); + + // Reliable rows first, descending by pct - MIXSTATIC sorts INLINE at + // +10%, between WIN (+20%) and FLAT (0%). Only the share-count change + // banishes a row to the bottom. + try testing.expectEqualStrings("WIN", view.symbols[0].symbol); + try testing.expectEqualStrings("MIXSTATIC", view.symbols[1].symbol); + try testing.expectEqualStrings("FLAT", view.symbols[2].symbol); + try testing.expectEqualStrings("LOSE", view.symbols[3].symbol); + try testing.expectEqualStrings("MIXBOUGHT", view.symbols[4].symbol); + + // MIXSTATIC: no price to show, but a trustworthy return. + try testing.expect(!view.symbols[1].price_comparable); + try testing.expect(view.symbols[1].pct_reliable); + try testing.expectApproxEqAbs(@as(f64, 0.10), view.symbols[1].pct_change, 1e-9); + + // MIXBOUGHT: pinned last despite the largest percentage in the table, + // which is exactly the point - it is not comparable with the rest. + try testing.expect(!view.symbols[4].price_comparable); + try testing.expect(!view.symbols[4].pct_reliable); + try testing.expectApproxEqAbs(@as(f64, 0.50), view.symbols[4].pct_change, 1e-9); + try testing.expectApproxEqAbs(@as(f64, 500.0), view.symbols[4].dollar_change, 1e-9); + try testing.expect(view.symbols[4].pct_change > view.symbols[0].pct_change); +} + test "buildTotalsRow: positive delta" { const t = buildTotalsRow(1_000_000.0, 1_050_000.0); try testing.expectApproxEqAbs(@as(f64, 50_000.0), t.delta, 1e-6); @@ -830,6 +1201,36 @@ pub fn buildSymbolRowCells( pct_buf: *[16]u8, dollar_buf: *[32]u8, ) SymbolRowCells { + // A mixed-share-class row has no meaningful per-share price on either + // side, so those two cells get the no-data sentinel rather than two + // numbers that invite subtraction. The percent and dollar cells DO + // render - on a value basis (see `SymbolChange.pct_change`) - because + // a total value change is exactly computable, and when the share count + // held still it IS the underlying return. `SymbolChange.pct_reliable` + // and `CompareView.unreliable_count` let the renderer flag the rows + // where it is not. + // + // Pad the sentinel to display columns here. The row templates use + // byte-based `{s:>N}` specs, and the sentinel is one display column in + // three bytes, so leaving it unpadded under-pads the cell by two + // columns and skews every column to its right. Padding to exactly + // `price_w` makes the byte-based spec a no-op and keeps the table + // square. Justification matches the spec each cell is fed to: + // `price_right_fmt` right-justifies, `price_left_fmt` left. + if (!s.price_comparable) { + // `padRightToCols` appends in place and requires its content to + // already sit at the start of the buffer, so stage the sentinel + // there first. (`padLeftToCols` copies, hence the asymmetry.) + const staged = std.fmt.bufPrint(price_now_buf, "{s}", .{fmt.no_data_sentinel}) catch fmt.no_data_sentinel; + return .{ + .symbol = s.symbol, + .price_then = fmt.padLeftToCols(price_then_buf, fmt.no_data_sentinel, price_w), + .price_now = fmt.padRightToCols(price_now_buf, staged, price_w), + .pct = view_hist.fmtSignedPercentBuf(pct_buf, s.pct_change), + .dollar = std.fmt.bufPrint(dollar_buf, "{f}", .{Money.from(s.dollar_change).signed()}) catch "$?", + .style = s.style, + }; + } return .{ .symbol = s.symbol, .price_then = std.fmt.bufPrint(price_then_buf, "{f}", .{Money.from(s.price_then)}) catch "$?",