diff --git a/src/analytics/valuation.zig b/src/analytics/valuation.zig index ff84814..bb06dbc 100644 --- a/src/analytics/valuation.zig +++ b/src/analytics/valuation.zig @@ -205,16 +205,27 @@ pub const Allocation = struct { shares: f64, /// Weighted average cost per share across all lots (cost_basis / shares). avg_cost: f64, - /// Latest price from API (or manual fallback), before price_ratio adjustment. + /// Latest price from API (or manual fallback). WHETHER + /// `price_ratio` HAS BEEN APPLIED DEPENDS ON `price_ratio` ITSELF - + /// read this together with that field, never alone: /// - /// POSITION-level and RAW. Never feed this into a per-lot - /// calculation: `mergeAllocsBySymbol` folds every ratio variant of a - /// ticker into one row with base-ticker-normalized `shares`, so a - /// lot row multiplying its own raw shares by this price is off by - /// exactly the lot's ratio. Per-lot display sites go through - /// `views/portfolio_sections.zig:effectivePriceFor`. See the "Per-LOT - /// display rows" section of the pricing-model block in - /// `models/portfolio.zig`. + /// - `price_ratio != 1.0` (UNMERGED): this is the EFFECTIVE price, + /// `pos.effectivePrice(raw, is_manual)`, ratio already applied. + /// - `price_ratio == 1.0`: either a plain unratioed position, or a + /// group that `mergeAllocsBySymbol` folded - in which case this + /// is the RAW base-ticker price (`total_mv / norm_shares`) and + /// `shares` are in base-ticker-equivalent units. + /// + /// Either way `shares * current_price == market_value` holds, which + /// is what makes the position row self-consistent. + /// + /// Never feed this into a per-lot calculation: a lot row that + /// multiplies its own raw shares by this price is off by the lot's + /// ratio in the merged case, and squares it if you "fix" that by + /// keying on `is_manual_price` instead. Per-lot display sites go + /// through `views/portfolio_sections.zig:effectivePriceFor`, which + /// discriminates on `price_ratio`. See the "Per-LOT display rows" + /// section of the pricing-model block in `models/portfolio.zig`. current_price: f64, /// Total current value: shares * current_price * price_ratio. /// May be reduced by adjustForCoveredCalls for ITM sold calls diff --git a/src/models/portfolio.zig b/src/models/portfolio.zig index d06cc45..69bc859 100644 --- a/src/models/portfolio.zig +++ b/src/models/portfolio.zig @@ -69,27 +69,40 @@ const Split = split.Split; // ## Per-LOT display rows // // `valuation.Allocation` is a POSITION-level view, and its -// `current_price` is the RAW base-ticker price - not any lot's -// effective price. Worse, `valuation.mergeAllocsBySymbol` folds every -// ratio variant of one ticker into a single row whose `shares` are -// normalized into base-ticker-equivalent units. So the allocation is -// self-consistent while telling you nothing directly usable about the -// individual lots underneath it. +// `current_price` is NOT unconditionally raw. Which it is depends on +// whether `mergeAllocsBySymbol` folded the row: // -// A lot-detail row that multiplies its OWN raw shares by that raw price -// is therefore wrong by exactly the lot's ratio. That shipped: a -// direct-indexing sleeve with `price_ratio:num:0.2387` rendered a -// +$3.39M gain against a real +$433K, and the lot rows under a position -// summed to three times the position's own market value. Three sites had -// independently hand-rolled the same broken expression. +// - UNMERGED (`alloc.price_ratio != 1.0`): the allocation carries the +// lot's own ratio, and `portfolioSummary` already applied it - +// `current_price` is the EFFECTIVE price. +// - MERGED, or plainly unratioed (`alloc.price_ratio == 1.0`): +// `current_price` is the RAW base-ticker price and `shares` are in +// base-ticker-equivalent units. +// +// So a lot-detail row cannot just multiply its own raw shares by +// `current_price`: in the merged case it is wrong by exactly the lot's +// ratio. That shipped - a direct-indexing sleeve with +// `price_ratio:num:0.2387` rendered a +$3.39M gain against a real +// +$433K, and the lot rows under a position summed to three times the +// position's own market value. Three sites had independently +// hand-rolled the same broken expression. +// +// The first fix for that got the OTHER half wrong: it keyed provenance +// on `alloc.is_manual_price`, which is orthogonal to merging, so every +// unmerged live-priced ratio'd lot then had its ratio applied twice. +// That is the single-CIT-lot case this file's `ticker` + `price_ratio` +// docs describe as the primary use, and no test caught it because every +// fixture left `Allocation.price_ratio` at 1.0 - indistinguishable from +// a merged group. // // So: per-lot display sites MUST price through -// `views/portfolio_sections.zig:effectivePriceFor(allocations, lot)`, which -// resolves `close_price`, reads provenance off -// `Allocation.is_manual_price`, and applies the ratio via -// `Lot.effectivePrice`. Never read `Allocation.current_price` into a -// per-lot calculation. Current callers: the CLI holdings table, the TUI -// portfolio tab's lot rows, and the shared column-width pass. +// `views/portfolio_sections.zig:effectivePriceFor(allocations, lot)`, +// which resolves `close_price`, discriminates on `price_ratio`, and +// applies the ratio via `Lot.effectivePrice`. Never read +// `Allocation.current_price` into a per-lot calculation, and never +// reintroduce `is_manual_price` as the provenance signal. Current +// callers: the CLI holdings table, the TUI portfolio tab's lot rows, +// and the shared column-width pass. // ── Share model (split adjustment) ────────────────────────── // diff --git a/src/views/portfolio_sections.zig b/src/views/portfolio_sections.zig index c432b6c..3306358 100644 --- a/src/views/portfolio_sections.zig +++ b/src/views/portfolio_sections.zig @@ -187,30 +187,50 @@ fn gainLossCols(amount: f64) usize { /// position summed to three times the position's own market value. /// /// Provenance follows the `is_preadjusted` rule from the pricing-model -/// block in `models/portfolio.zig`: +/// block in `models/portfolio.zig`. The signal is `price_ratio`, and it +/// is answering ONE question: has this allocation been merged? /// -/// - `close_price` is preadjusted (already the lot's own NAV), so a -/// closed lot's ratio is NOT reapplied. Matches how the -/// contributions pipeline values closed lots. -/// - A manual `price::` is preadjusted too. `buildFallbackPrices` -/// stores the raw override and flags the allocation -/// `is_manual_price`, so that flag is the provenance signal here. -/// - Anything else is a candle close: raw, so the ratio applies. +/// - `close_price` short-circuits first. It is the price the lot +/// actually closed at, already in the lot's own terms, so the ratio +/// is NOT reapplied. Matches how the contributions pipeline values +/// closed lots (`effectivePrice(close_price, true)`). +/// - `a.price_ratio != 1.0` means the allocation is UNMERGED and +/// carries this lot's own ratio (`positionsAsOf` groups by +/// `(priceSymbol, price_ratio)` and propagates the ratio through). +/// `portfolioSummary` set its `current_price` to +/// `pos.effectivePrice(raw, is_manual)` - the ratio is ALREADY +/// APPLIED - so applying it again would square it. +/// - `a.price_ratio == 1.0` means either a plain unratioed position +/// (apply 1.0, a no-op) or a MERGED group, where +/// `mergeAllocsBySymbol` normalized shares into base-ticker units, +/// set `current_price` to the raw base price, and reset the ratio +/// to 1.0. Both want the lot's own ratio applied. +/// +/// Do NOT use `a.is_manual_price` for this. It looks like a provenance +/// flag and reads like the right answer, but it is orthogonal: a +/// manual `price::` is stored raw in the prices map and flagged, then +/// `portfolioSummary` folds it into `current_price` via +/// `effectivePrice(p, true)` - so by the time it reaches an +/// `Allocation` it has the same "already effective" shape as a live +/// unmerged price. Keying on it applied the ratio twice for every +/// unmerged, live-priced, ratio'd lot - i.e. the single-CIT-lot case +/// that `docs/reference/config/portfolio-srf.md` documents as the +/// primary use of `price_ratio`. /// /// Known limit, deliberately not handled: a MERGED group that mixes /// manual-priced and live-priced components gets a blended /// `current_price` (`total_mv / norm_shares`) that is neither raw nor -/// preadjusted, and a single `is_manual_price` bit cannot describe -/// both. That is an expressiveness gap in `mergeAllocsBySymbol` itself, -/// not something a lot-row accessor can repair. Merged groups whose -/// components share one provenance - the common case, and the only case -/// a ratio'd alias produces - are exact: with every component live off -/// the same raw price `r`, `total_mv / norm_shares` reduces to `r`. +/// preadjusted. That is an expressiveness gap in `mergeAllocsBySymbol` +/// itself, not something a lot-row accessor can repair. Merged groups +/// whose components share one provenance - the common case, and the +/// only case a ratio'd alias produces - are exact: with every +/// component live off the same raw price `r`, `total_mv / norm_shares` +/// reduces to `r`. pub fn effectivePriceFor(allocations: []const Allocation, lot: Lot) f64 { if (lot.close_price) |cp| return lot.effectivePrice(cp, true); for (allocations) |a| { if (!std.mem.eql(u8, a.symbol, lot.priceSymbol())) continue; - return lot.effectivePrice(a.current_price, a.is_manual_price); + return lot.effectivePrice(a.current_price, a.price_ratio != 1.0); } return 0; } @@ -705,12 +725,44 @@ test "effectivePriceFor: ratio 1.0 passes the raw price straight through" { try testing.expectEqual(@as(f64, 60), effectivePriceFor(&allocs, lot)); } -test "effectivePriceFor: manual price is preadjusted, so the ratio is NOT reapplied" { - // `buildFallbackPrices` stores the raw `price::` override and flags - // the allocation `is_manual_price`. A manual price is already the - // lot's own NAV by convention, so multiplying by the ratio would - // double-apply it. - var alloc = mkAlloc("ORCX", 100, 18.15, 19.01, 1901, 86); +test "effectivePriceFor: unmerged live price is already effective, ratio NOT reapplied" { + // THE REGRESSION. A single ratio'd lot whose ticker is shared with + // nobody produces an UNMERGED allocation: `positionsAsOf` groups by + // `(priceSymbol, price_ratio)`, so `Allocation.price_ratio` is the + // lot's own 5.0, and `portfolioSummary` already folded that ratio + // into `current_price`. Applying it again squares it. + // + // This is the documented primary use of `price_ratio` - a lone 401k + // CIT lot priced off its retail sibling (see + // docs/reference/config/portfolio-srf.md). Keying provenance on + // `is_manual_price` returned 144.04 * 5.0 = 720.20 here. + const allocs = [_]Allocation{mkAllocUnmerged("VTTHX", 1200, 106.99, 144.04, 5.0, 44_460)}; + const lot = Lot{ + .symbol = "02315N600", + .ticker = "VTTHX", + .price_ratio = 5.0, + .shares = 1200, + .open_date = Date.fromYmd(2026, 2, 26), + .open_price = 106.99, + }; + try testing.expectEqual(@as(f64, 144.04), effectivePriceFor(&allocs, lot)); + // And the lot row must reconcile with its own position row. + try testing.expectApproxEqRel( + allocs[0].market_value, + lot.effectiveShares() * effectivePriceFor(&allocs, lot), + 1e-9, + ); +} + +test "effectivePriceFor: unmerged manual price is also already effective" { + // Same unmerged shape, manual `price::` instead of a candle close. + // `buildFallbackPrices` stores the raw override and flags the + // allocation, then `portfolioSummary` folds it in via + // `effectivePrice(p, true)` - so by the time it reaches an + // Allocation it has the same "already effective" shape as the live + // case above. `price_ratio != 1.0` covers both; `is_manual_price` is + // not consulted. + var alloc = mkAllocUnmerged("ORCX", 100, 18.15, 19.01, 5.0, 86); alloc.is_manual_price = true; const allocs = [_]Allocation{alloc}; const lot = Lot{ @@ -723,6 +775,25 @@ test "effectivePriceFor: manual price is preadjusted, so the ratio is NOT reappl try testing.expectEqual(@as(f64, 19.01), effectivePriceFor(&allocs, lot)); } +test "effectivePriceFor: a merged group's raw price DOES get the lot's ratio" { + // The counterpart, and why the discriminator can't just be "always + // skip". `mergeAllocsBySymbol` normalizes shares to base-ticker + // units, sets `current_price` to the raw base price, and resets + // `price_ratio` to 1.0 - so the lot's own ratio must be applied. + // `is_manual_price` is false in both this case and the unmerged live + // case above, which is exactly why it cannot discriminate them. + const merged = [_]Allocation{mkAlloc("BENCH", 1929.2, 426.05, 765.91, 1_477_603, 655_655)}; + const lot = Lot{ + .symbol = "DI-IDX", + .ticker = "BENCH", + .price_ratio = 1.0120921601549708, + .shares = 709.235272, + .open_date = Date.fromYmd(2026, 2, 25), + .open_price = 461.240208, + }; + try testing.expectApproxEqRel(@as(f64, 775.1715), effectivePriceFor(&merged, lot), 1e-6); +} + test "effectivePriceFor: close_price wins over the allocation and skips the ratio" { // A closed lot's `close_price` is the price it actually closed at - // already in the lot's own units. Matches how the contributions @@ -839,12 +910,66 @@ test "effectivePriceFor: lot rows sum to their merged position's market value" { try testing.expectApproxEqRel(@as(f64, 655_655.23), lot_gl_total, 1e-6); } +test "effectivePriceFor: a LONE ratio'd lot's row reconciles with its position" { + // The unmerged half of the same invariant, driven through the real + // pipeline. One ratio'd lot, ticker shared with nobody, so + // `mergeAllocsBySymbol` leaves it alone: `price_ratio` stays 5.0 and + // `current_price` is already effective. + // + // Under the `is_manual_price` discriminator this test failed with + // lot_mv = shares * raw * ratio * ratio - the ratio squared, a 5x + // overstatement of a real position. The merged test above passed + // throughout, which is how the bug shipped. + const raw_price: f64 = 28.808; // retail sibling; institutional NAV = 144.04 + const ratio: f64 = 5.0; + var lots = [_]Lot{.{ + .symbol = "02315N600", + .ticker = "VTTHX", + .price_ratio = ratio, + .shares = 1200, + .open_date = Date.fromYmd(2026, 2, 26), + .open_price = 106.99, + .account = "Sample 401(k)", + }}; + const pf = portfolio_mod.Portfolio{ .lots = &lots, .allocator = testing.allocator }; + const as_of = Date.fromYmd(2026, 8, 26); + + const positions = try pf.positionsAsOf(testing.allocator, as_of); + defer testing.allocator.free(positions); + try testing.expectEqual(@as(usize, 1), positions.len); + + var prices = std.StringHashMap(f64).init(testing.allocator); + defer prices.deinit(); + try prices.put("VTTHX", raw_price); + + var summary = try valuation.portfolioSummary(as_of, testing.allocator, pf, positions, prices, null); + defer summary.deinit(testing.allocator); + + try testing.expectEqual(@as(usize, 1), summary.allocations.len); + const a = summary.allocations[0]; + // Unmerged: the allocation KEEPS the lot's ratio, and current_price + // is the effective (institutional) price, not the raw retail one. + try testing.expectEqual(ratio, a.price_ratio); + try testing.expectApproxEqRel(raw_price * ratio, a.current_price, 1e-9); + + const eff_price = effectivePriceFor(summary.allocations, lots[0]); + try testing.expectApproxEqRel(raw_price * ratio, eff_price, 1e-9); + try testing.expectApproxEqRel(a.market_value, lots[0].effectiveShares() * eff_price, 1e-9); + // Magnitude pin: 1200 * 144.04 = $172,848, NOT 1200 * 720.20 = $864,240. + try testing.expectApproxEqRel(@as(f64, 172_848.0), lots[0].effectiveShares() * eff_price, 1e-9); +} + // ── computeWidths ───────────────────────────────────────────── /// Build a minimal Allocation for the width and effective-price tests. /// `cost_basis` is derived so `unrealized_gain_loss` is consistent (not /// that computeWidths reads cost_basis, but it keeps the fixture /// honest). +/// +/// `price_ratio` defaults to 1.0, which is the MERGED shape (or a plain +/// unratioed position): `current_price` is the raw base-ticker price. +/// For the unmerged shape use `mkAllocUnmerged` - the distinction is +/// load-bearing, see `effectivePriceFor`. fn mkAlloc(symbol: []const u8, shares: f64, avg_cost: f64, current_price: f64, market_value: f64, gl: f64) Allocation { return .{ .symbol = symbol, @@ -860,6 +985,33 @@ fn mkAlloc(symbol: []const u8, shares: f64, avg_cost: f64, current_price: f64, m }; } +/// An UNMERGED allocation: one ratio'd position that shares its ticker +/// with nobody, so `mergeAllocsBySymbol` never touched it. It keeps its +/// lot's `price_ratio`, and `portfolioSummary` already folded that ratio +/// into `current_price` (`pos.effectivePrice(raw, is_manual)`), so +/// `current_price` here is the EFFECTIVE price, not the raw base price. +/// +/// This is the shape every fixture was missing: `mkAlloc` leaves +/// `price_ratio` at 1.0, which is indistinguishable from a merged group, +/// so a whole suite of tests can pass while the unmerged path squares +/// the ratio. +fn mkAllocUnmerged(symbol: []const u8, shares: f64, avg_cost: f64, effective_price: f64, price_ratio: f64, gl: f64) Allocation { + const mv = shares * effective_price; + return .{ + .symbol = symbol, + .display_symbol = symbol, + .shares = shares, + .avg_cost = avg_cost, + .current_price = effective_price, + .market_value = mv, + .cost_basis = mv - gl, + .weight = 1.0, + .unrealized_gain_loss = gl, + .unrealized_return = 0, + .price_ratio = price_ratio, + }; +} + test "computeWidths: a ratio'd lot's effective price widens the Price column" { // A ratio'd lot renders its own effective price in the // otherwise-blank Price cell, and at an 8.6x institutional ratio