From 12e131fef82fe2e52929b412ee305e7ce8a2281f Mon Sep 17 00:00:00 2001 From: Emil Lerch Date: Mon, 3 Aug 2026 21:54:43 -0700 Subject: [PATCH] add observation for missing dividend history --- src/analytics/observations.zig | 111 +++++++++++++++++++++++++++ src/views/review.zig | 135 ++++++++++++++++++++++++++++++++- 2 files changed, 242 insertions(+), 4 deletions(-) diff --git a/src/analytics/observations.zig b/src/analytics/observations.zig index 1e9ebbf..08e8b34 100644 --- a/src/analytics/observations.zig +++ b/src/analytics/observations.zig @@ -357,6 +357,11 @@ pub const default_checks = [_]Check{ .label = "Tiny position", .run = checkTinyPosition, }, + .{ + .name = "dividend_data_missing", + .label = "Dividend data", + .run = checkDividendData, + }, .{ .name = "drift", .label = "Drift since last view", @@ -731,6 +736,59 @@ fn checkTinyPosition(ctx: CheckCtx) CheckResult { return .pass; } +/// Holdings whose trailing returns are price-only because no dividend +/// history was available for them. +/// +/// This is a data-quality check, not a portfolio-risk one: the numbers in +/// the table are wrong (understated by roughly the yield), and for an +/// income-oriented holding that is most of the return. It exists because +/// the failure is otherwise completely silent - a missing dividend cache +/// entry renders as a perfectly plausible total-return figure. +/// +/// Only fires on genuine absence. A holding present in the dividend map +/// with zero records pays no dividends, which is a correct answer and gets +/// no finding. See `ReviewRow.dividends_missing`. +fn checkDividendData(ctx: CheckCtx) CheckResult { + if (ctx.rows.len == 0) return .pass; + + var findings = std.ArrayList(Observation).empty; + errdefer { + for (findings.items) |o| { + ctx.allocator.free(o.kind); + ctx.allocator.free(o.target); + ctx.allocator.free(o.text); + } + findings.deinit(ctx.allocator); + } + + for (ctx.rows) |row| { + if (!row.dividends_missing) continue; + const text = std.fmt.allocPrint(ctx.allocator, "{s} has no dividend history - its returns are price-only and understate total return. Check POLYGON_API_KEY and `zfin divs {s}`", .{ + row.symbol, + row.symbol, + }) catch return errResult(ctx.allocator, "alloc failed"); + const target = ctx.allocator.dupe(u8, row.symbol) catch { + ctx.allocator.free(text); + return errResult(ctx.allocator, "alloc failed"); + }; + const kind = ctx.allocator.dupe(u8, "dividend_data_missing") catch { + ctx.allocator.free(text); + ctx.allocator.free(target); + return errResult(ctx.allocator, "alloc failed"); + }; + findings.append(ctx.allocator, .{ + .severity = .warn, + .kind = kind, + .target = target, + .text = text, + }) catch return errResult(ctx.allocator, "alloc failed"); + } + + if (findings.items.len == 0) return .pass; + const slice = findings.toOwnedSlice(ctx.allocator) catch return errResult(ctx.allocator, "alloc failed"); + return .{ .warn = slice }; +} + /// Drift since last view. Currently a placeholder - returns `skipped` /// until temporal observations ship in a follow-up. The forward-compat /// slot in the status grid stays visible (rendered as ➖) so users @@ -1058,6 +1116,59 @@ test "checkTinyPosition: positions below thresholds flag" { } } +test "checkDividendData: warns only on genuine absence, not on a holding that pays nothing" { + var rows = [_]review_view.ReviewRow{ + makeRow("AAA", "X", 0.50), + makeRow("BBB", "Y", 0.50), + }; + + // Neither row is missing data. BBB may well pay no dividends at all - + // that is a real answer and must stay silent, which is the entire point + // of tracking absence separately from emptiness. + { + const result = checkDividendData(.{ + .allocator = testing.allocator, + .rows = &rows, + .totals = emptyTotals(), + }); + defer freeResult(testing.allocator, result); + try testing.expect(result == .pass); + } + + // BBB's dividend fetch never landed: its returns are price-only. + rows[1].dividends_missing = true; + { + const result = checkDividendData(.{ + .allocator = testing.allocator, + .rows = &rows, + .totals = emptyTotals(), + }); + defer freeResult(testing.allocator, result); + switch (result) { + .warn => |obs| { + try testing.expectEqual(@as(usize, 1), obs.len); + try testing.expectEqualStrings("BBB", obs[0].target); + try testing.expectEqualStrings("dividend_data_missing", obs[0].kind); + // The text must say which way the number is wrong, not just + // that something is missing. + try testing.expect(std.mem.indexOf(u8, obs[0].text, "price-only") != null); + try testing.expect(std.mem.indexOf(u8, obs[0].text, "understate") != null); + }, + else => return error.TestUnexpectedResult, + } + } +} + +test "checkDividendData: no rows is a pass" { + const result = checkDividendData(.{ + .allocator = testing.allocator, + .rows = &.{}, + .totals = emptyTotals(), + }); + defer freeResult(testing.allocator, result); + try testing.expect(result == .pass); +} + test "checkDrift: returns skipped (placeholder)" { const ctx: CheckCtx = .{ .allocator = testing.allocator, diff --git a/src/views/review.zig b/src/views/review.zig index a11383e..bcc8a5e 100644 --- a/src/views/review.zig +++ b/src/views/review.zig @@ -108,6 +108,17 @@ pub const ReviewRow = struct { sharpe_10y: ?f64, /// 5Y max drawdown (positive decimal, e.g. 0.30 = 30%). maxdd_5y: ?f64, + /// True when a dividend map was supplied but had no entry for this + /// symbol, so the trailing returns above fell back to the + /// adj_close-only path and understate total return by roughly the + /// yield. + /// + /// Deliberately distinct from "present with zero records", which is a + /// real answer (the holding pays nothing) and needs no warning. Absence + /// means the fetch never succeeded. Collapsing the two is what let a + /// cold dividend cache masquerade as a correct total return for as long + /// as the review table has existed. + dividends_missing: bool = false, }; /// Bottom totals row. @@ -204,10 +215,20 @@ pub fn buildReview( for (summary.allocations) |a| { const candles = candle_map.get(a.symbol) orelse &.{}; - const dividends: ?[]const zfin.Dividend = if (dividend_map) |dm| - (dm.get(a.symbol) orelse null) - else - null; + + // Absent from the map and present-but-empty are different facts and + // must not collapse: the first means the fetch failed and the + // returns below are price-only; the second means the holding pays + // nothing, which is a correct and unremarkable answer. + var dividends: ?[]const zfin.Dividend = null; + var dividends_missing = false; + if (dividend_map) |dm| { + if (dm.get(a.symbol)) |d| { + dividends = d; + } else { + dividends_missing = true; + } + } const bucket = bucketForSymbol(a.symbol, classifications); const tax_pct = computeTaxPct(a.symbol, portfolio, account_map, as_of); @@ -229,6 +250,7 @@ pub fn buildReview( .sharpe_3y = if (tr_risk.three_year) |m| m.sharpe else null, .sharpe_10y = if (tr_risk.ten_year) |m| m.sharpe else null, .maxdd_5y = if (tr_risk.five_year) |m| m.max_drawdown else null, + .dividends_missing = dividends_missing, }); try positions.append(allocator, .{ @@ -1186,6 +1208,111 @@ test "returnIntent: signs map to gain/loss colors" { try testing.expectEqual(format.StyleIntent.normal, returnIntent(0.0)); } +test "buildReview: dividends_missing separates a failed fetch from a holding that pays nothing" { + // Regression: this used to be `dm.get(sym) orelse null`, which collapsed + // "never fetched" into "pays nothing". Both took the adj_close-only + // path, so a cold dividend cache rendered as a plausible total return + // with no indication anywhere that the number was price-only. + const D = zfin.Date; + const Lot = @import("../models/portfolio.zig").Lot; + + var allocs = [_]valuation.Allocation{ + .{ + .symbol = "AAA", + .display_symbol = "AAA", + .shares = 10, + .avg_cost = 10, + .current_price = 12, + .market_value = 120, + .cost_basis = 100, + .weight = 0.5, + .unrealized_gain_loss = 20, + .unrealized_return = 0.2, + .account = "Sample Brokerage", + }, + .{ + .symbol = "BBB", + .display_symbol = "BBB", + .shares = 10, + .avg_cost = 10, + .current_price = 12, + .market_value = 120, + .cost_basis = 100, + .weight = 0.5, + .unrealized_gain_loss = 20, + .unrealized_return = 0.2, + .account = "Sample Brokerage", + }, + }; + const summary: valuation.PortfolioSummary = .{ + .total_value = 240, + .total_cost = 200, + .unrealized_gain_loss = 40, + .unrealized_return = 0.2, + .realized_gain_loss = 0, + .allocations = allocs[0..], + }; + var lots = [_]Lot{ + .{ .symbol = "AAA", .shares = 10, .open_date = D.fromYmd(2022, 1, 10), .open_price = 10, .account = "Sample Brokerage" }, + .{ .symbol = "BBB", .shares = 10, .open_date = D.fromYmd(2022, 1, 10), .open_price = 10, .account = "Sample Brokerage" }, + }; + const portfolio: zfin.Portfolio = .{ .lots = lots[0..], .allocator = testing.allocator }; + + var candle_map = std.StringHashMap([]const zfin.Candle).init(testing.allocator); + defer candle_map.deinit(); + const cm: classification.ClassificationMap = .{ .entries = &.{}, .allocator = testing.allocator }; + + // AAA: present with zero records -> genuinely pays nothing. + // BBB: absent entirely -> the fetch never landed. + var dividend_map = std.StringHashMap([]const zfin.Dividend).init(testing.allocator); + defer dividend_map.deinit(); + try dividend_map.put("AAA", &.{}); + + var view = try buildReview( + testing.allocator, + std.testing.io, + summary, + &candle_map, + ÷nd_map, + portfolio, + cm, + null, + D.fromYmd(2026, 6, 4), + "test_portfolio.srf", + ); + defer view.deinit(testing.allocator); + + var saw_aaa = false; + var saw_bbb = false; + for (view.rows) |r| { + if (std.mem.eql(u8, r.symbol, "AAA")) { + saw_aaa = true; + try testing.expect(!r.dividends_missing); + } else if (std.mem.eql(u8, r.symbol, "BBB")) { + saw_bbb = true; + try testing.expect(r.dividends_missing); + } + } + try testing.expect(saw_aaa and saw_bbb); + + // A null map means dividends were never requested at all. That is not a + // per-symbol failure, so it must not flag every holding. + var view2 = try buildReview( + testing.allocator, + std.testing.io, + summary, + &candle_map, + null, + portfolio, + cm, + null, + D.fromYmd(2026, 6, 4), + "test_portfolio.srf", + ); + defer view2.deinit(testing.allocator); + for (view2.rows) |r| try testing.expect(!r.dividends_missing); +} + test "buildReview: end-to-end with testing allocator (leak check)" { // Allocator detects leaks: every allocation must be freed for the // test to pass. Exercises the full buildReview lifecycle including