diff --git a/src/analytics/valuation.zig b/src/analytics/valuation.zig index b185b5d..498f2d0 100644 --- a/src/analytics/valuation.zig +++ b/src/analytics/valuation.zig @@ -1152,6 +1152,96 @@ test "computeDayChange: positions without candles are counted but excluded" { try std.testing.expectApproxEqAbs(@as(f64, 100), dc.change, 0.01); } +test "buildFallbackPrices: a pre-inserted manual price loses its flag" { + // Why pre-multiplying a manual override into the shared map is unsound, + // demonstrated on the pure function rather than asserted in prose. + // + // The gap-fill is guarded by `if (!prices.contains(sym))`, so an entry + // someone else already inserted is skipped - and therefore never lands + // in `manual_price_set`. Downstream `effectivePrice(price, is_manual)` + // then sees `is_manual == false` and applies `price_ratio` to a value + // that already had it folded in, squaring it. + // + // `commands/snapshot.zig` did exactly that until the pre-multiply loop + // was deleted; `commands/audit.zig` still does, and cannot stop until + // the reconcile path grows a preadjusted set. See "The pre-multiply + // anti-pattern" in models/portfolio.zig. + const a = std.testing.allocator; + const ratio: f64 = 5.0; + const typed: f64 = 144.04; // what the user reads off the statement + const lots = [_]portfolio_mod.Lot{.{ + .symbol = "02315N600", + .ticker = "VTTHX", + .price_ratio = ratio, + .price = typed, + .shares = 100, + .open_date = Date.fromYmd(2026, 2, 26), + .open_price = 106.99, + }}; + const positions = [_]portfolio_mod.Position{dcPos("VTTHX", 100, ratio)}; + + // CORRECT: hand it an empty map. The override goes in untouched and the + // symbol is flagged, so the ratio is skipped and the value is exact. + { + var prices = std.StringHashMap(f64).init(a); + defer prices.deinit(); + var manual = try buildFallbackPrices(a, &lots, &positions, &prices); + defer manual.deinit(); + + try std.testing.expect(manual.contains("VTTHX")); + const p = prices.get("VTTHX").?; + try std.testing.expectApproxEqAbs(typed, p, 0.001); // stored RAW + const value = positions[0].marketValue(p, manual.contains("VTTHX")); + try std.testing.expectApproxEqAbs(@as(f64, 14_404.0), value, 0.01); + } + + // BROKEN: pre-multiply the same override in first. The gap-fill skips + // it, the flag never appears, and the ratio lands twice. + { + var prices = std.StringHashMap(f64).init(a); + defer prices.deinit(); + try prices.put("VTTHX", typed * ratio); // the pre-multiply + var manual = try buildFallbackPrices(a, &lots, &positions, &prices); + defer manual.deinit(); + + try std.testing.expect(!manual.contains("VTTHX")); // flag lost + const p = prices.get("VTTHX").?; + const value = positions[0].marketValue(p, manual.contains("VTTHX")); + // shares * (p * ratio) * ratio = 100 * 144.04 * 25 = 360,100. + // The error factor is the ratio SQUARED - 25x here, not 5x. + try std.testing.expectApproxEqAbs(@as(f64, 360_100.0), value, 0.01); + } +} + +test "buildFallbackPrices: a candle price is never overridden or flagged" { + // The guard's legitimate purpose: a live close already in the map wins + // over a manual override, and must NOT be flagged preadjusted - candle + // closes are raw and do need the ratio. + const a = std.testing.allocator; + const lots = [_]portfolio_mod.Lot{.{ + .symbol = "02315N600", + .ticker = "VTTHX", + .price_ratio = 5.0, + .price = 999.0, // stale override, should lose + .shares = 100, + .open_date = Date.fromYmd(2026, 2, 26), + .open_price = 106.99, + }}; + const positions = [_]portfolio_mod.Position{dcPos("VTTHX", 100, 5.0)}; + + var prices = std.StringHashMap(f64).init(a); + defer prices.deinit(); + try prices.put("VTTHX", 28.808); // raw candle close + var manual = try buildFallbackPrices(a, &lots, &positions, &prices); + defer manual.deinit(); + + try std.testing.expect(!manual.contains("VTTHX")); + try std.testing.expectApproxEqAbs(@as(f64, 28.808), prices.get("VTTHX").?, 0.001); + // Ratio applied once: 100 * 28.808 * 5 = 14,404. + const value = positions[0].marketValue(prices.get("VTTHX").?, false); + try std.testing.expectApproxEqAbs(@as(f64, 14_404.0), value, 0.01); +} + test "computeDayChange: a hand-priced holding is IMPOSSIBLE, not a gap" { // The headline behaviour. A 529 fund the user prices by hand on Saturdays // has no candle history and never will: the only price zfin will ever diff --git a/src/commands/audit.zig b/src/commands/audit.zig index 360367b..c705b00 100644 --- a/src/commands/audit.zig +++ b/src/commands/audit.zig @@ -175,7 +175,16 @@ pub fn run(ctx: *framework.RunCtx, parsed: ParsedArgs) !void { for (portfolio.lots) |lot| { if (lot.price) |p| { if (!prices.contains(lot.priceSymbol())) { - // Pre-multiply - see "Pricing model" in models/portfolio.zig. + // KNOWN GAP, see "The pre-multiply anti-pattern" in + // models/portfolio.zig. Pre-multiplying here is unsound + // - `resolvePositionValue` applies the ratio again - so + // a manual price on a lot with a non-unit `price_ratio` + // reads high by that ratio SQUARED (25x at ratio 5, not + // 5x). It stays for now because the + // reconcile path has no preadjusted-set to thread + // through; snapshot.zig fixed its copy by deleting the + // pre-multiply and letting `buildFallbackPrices` flag + // the symbol instead, which this path cannot do yet. try prices.put(lot.priceSymbol(), lot.effectivePrice(p, false)); } } diff --git a/src/commands/snapshot.zig b/src/commands/snapshot.zig index 61c6a3c..d9c3eec 100644 --- a/src/commands/snapshot.zig +++ b/src/commands/snapshot.zig @@ -294,26 +294,29 @@ pub fn run(ctx: *framework.RunCtx, parsed: ParsedArgs) !void { } } - // Build the flat-price map that `portfolioSummary` expects, plus - // apply manual `price::` overrides from portfolio.srf (which win - // over the candle lookup). + // Build the flat-price map that `portfolioSummary` expects: RAW + // base-ticker closes, keyed by symbol. + // + // Manual `price::` overrides are deliberately NOT applied here. + // `captureSnapshot` runs `buildFallbackPrices`, which gap-fills them + // correctly - it stores the raw value AND records the symbol in its + // `manual_set`, so the downstream `lot.effectivePrice(raw, is_manual)` + // knows to skip the share-class ratio (a hand-typed price is already in + // the lot's own share class). + // + // This used to pre-multiply overrides into the map here, which was wrong + // twice over. The map is keyed by SYMBOL while `price_ratio` is per LOT, + // so folding one lot's ratio in poisons every other lot sharing that + // ticker. And `buildFallbackPrices` gap-fills only when the symbol is + // absent, so the pre-inserted entry was skipped and never flagged - + // leaving `is_manual` false downstream and the ratio applied a second + // time, squaring it. var prices = std.StringHashMap(f64).init(allocator); defer prices.deinit(); var sp_it = symbol_prices.iterator(); while (sp_it.next()) |entry| { try prices.put(entry.key_ptr.*, entry.value_ptr.close); } - for (portfolio.lots) |lot| { - if (lot.price) |p| { - if (!prices.contains(lot.priceSymbol())) { - // Pre-multiply manual overrides so the shared `prices` - // map holds share-class-correct values - see the - // "Pricing model / caching pre-multiply pattern" note - // in models/portfolio.zig. - try prices.put(lot.priceSymbol(), lot.effectivePrice(p, false)); - } - } - } // Derive output path. var as_of_buf: [10]u8 = undefined; diff --git a/src/models/portfolio.zig b/src/models/portfolio.zig index 261ec3b..c914d07 100644 --- a/src/models/portfolio.zig +++ b/src/models/portfolio.zig @@ -46,15 +46,36 @@ const Split = split.Split; // snapshot.zig, audit/, and valuation.zig route through these - do // not reintroduce inline `price * price_ratio` expressions. // -// ## Caching pre-multiply pattern +// ## The pre-multiply anti-pattern (do not add new instances) // -// When manual overrides (2b) get folded into a shared `prices` map -// keyed by symbol, they're PRE-MULTIPLIED by `price_ratio` at insert -// time (see `commands/snapshot.zig:buildSnapshot` and -// `commands/audit/`). This normalizes the cached value so later -// readers can treat every entry uniformly as "price in whichever terms -// the lot needs." The `manual_set` (from `buildFallbackPrices`) then -// tells readers which entries are preadjusted. +// A tempting shortcut is to fold `price_ratio` into a shared `prices` map +// at insert time - "pre-multiplying" a manual override - so later readers +// can treat every entry uniformly. It is unsound, for two reasons: +// +// 1. The map is keyed by SYMBOL while `price_ratio` is per LOT. Folding +// one lot's ratio into a symbol-keyed entry corrupts every other lot +// sharing that ticker at a different ratio - which is exactly the +// `ticker::` aliasing this whole mechanism exists to support. +// 2. It collides with `buildFallbackPrices`, whose first pass gap-fills +// only `if (!prices.contains(sym))`. A pre-inserted entry is skipped, +// so it never lands in `manual_set`, so the downstream +// `effectivePrice(raw, is_manual)` sees `is_manual == false` and +// applies the ratio a SECOND time, squaring it. +// +// The correct shape is raw value + a companion "preadjusted" set: +// `buildFallbackPrices` stores the override untouched and records the +// symbol in `manual_set`; readers pass that through as `is_preadjusted`. +// `commands/snapshot.zig` does this (it used to pre-multiply, and squared +// the ratio for any manual-priced lot with a non-unit one). +// +// KNOWN GAP: `commands/audit.zig` still pre-multiplies, because the +// reconcile path has no `manual_set` equivalent - +// `reconcile/common.zig:resolvePositionValue` applies the ratio to +// whatever is in the map with no way to opt out. Fixing it properly means +// threading preadjusted-ness through the reconcile signatures. Until then +// a manual `price::` combined with a non-unit `price_ratio` reads high by +// the ratio in `zfin audit` only. Do not "fix" it by dividing the ratio +// out at insert - that just moves the corruption onto sibling lots. // // ## avg_cost fallback //