From 0e1a4862db2aff3673f544778b9950adbdcec32c Mon Sep 17 00:00:00 2001 From: Emil Lerch Date: Sat, 25 Jul 2026 10:45:12 -0700 Subject: [PATCH] add stale harvesting date checks to audit command --- docs/guides/set-up-accounts.md | 18 +- docs/reference/cli/audit.md | 17 +- docs/reference/config/accounts-srf.md | 14 ++ src/commands/audit/hygiene.zig | 347 +++++++++++++++++++++++++- 4 files changed, 380 insertions(+), 16 deletions(-) diff --git a/docs/guides/set-up-accounts.md b/docs/guides/set-up-accounts.md index 274b03d..acfa4ff 100644 --- a/docs/guides/set-up-accounts.md +++ b/docs/guides/set-up-accounts.md @@ -124,9 +124,21 @@ and the TUI Portfolio tab header when you filter to that account: The date is required -- an undated hand-copied number is indistinguishable from a stale one, so zfin won't display it and -[`zfin doctor`](../reference/cli/doctor.md) will tell you. Entries more -than 12 months old stop displaying on purpose. The sign is ignored, so -`-45300` works too. +[`zfin doctor`](../reference/cli/doctor.md) will tell you. The sign is +ignored, so `-45300` works too. + +You don't have to remember to refresh it. A flagless +[`zfin audit`](../reference/cli/audit.md) lists anything more than 90 +days old, so it rides along with your normal maintenance sweep: + +``` + Stale harvested figures (>90 days - refresh 'harvested' in accounts.srf) + Sample Tax Loss last updated 146 days ago +``` + +Past 12 months the annotation retires itself -- data that old isn't +decision-useful. The audit nag keeps going and says +`no longer displayed`, so the figure never disappears silently. **Don't put the number in the account name.** It's tempting -- the name already prints in a lot of places -- but the name is a join key, not a diff --git a/docs/reference/cli/audit.md b/docs/reference/cli/audit.md index def2c2f..558a259 100644 --- a/docs/reference/cli/audit.md +++ b/docs/reference/cli/audit.md @@ -8,10 +8,11 @@ Usage: zfin audit [opts] ``` **Flagless** runs the hygiene check -- stale manual prices, -accounts overdue for update, unhandled stock splits, and -auto-discovered brokerage-file candidates. **With brokerage flags**, it -reconciles your portfolio against the export (treating the brokerage as -source of truth) and reports discrepancies. +accounts overdue for update, stale tax-loss-harvested figures, +unhandled stock splits, and auto-discovered brokerage-file candidates. +**With brokerage flags**, it reconciles your portfolio against the +export (treating the brokerage as source of truth) and reports +discrepancies. ## Options @@ -41,6 +42,14 @@ purchase date but haven't opted into automatic split adjustment, in an date on its `metadata.srf` row -- without it, that holding's shares (and every value derived from them) are misstated across the split. +Accounts carrying a hand-declared +[`harvested`](../config/accounts-srf.md#harvested-and-harvested_date) +figure get a **Stale harvested figures** section once the figure is more +than 90 days old, or if it has no `harvested_date` at all. The section +is omitted entirely for portfolios where no account declares one. Past +12 months the annotation itself retires, and the nag says +`no longer displayed` so it can't vanish without explanation. + ## Example (hygiene check) ```bash diff --git a/docs/reference/config/accounts-srf.md b/docs/reference/config/accounts-srf.md index fda43ec..87f8f6d 100644 --- a/docs/reference/config/accounts-srf.md +++ b/docs/reference/config/accounts-srf.md @@ -120,10 +120,21 @@ It appears in four places: figure with no date is indistinguishable from a stale one, so zfin refuses to show it. [`zfin doctor`](../cli/doctor.md) warns when `harvested` is set without a date, and when a date is in the future. +- **`zfin audit` nags after 90 days.** A flagless + [`zfin audit`](../cli/audit.md) lists any account whose figure is more + than 90 days old under **Stale harvested figures**, so it's part of + your normal maintenance sweep. The threshold isn't configurable, and + doesn't need to be: only accounts that declare `harvested` are ever + considered, so not wanting the nag and not wanting the field are the + same choice. - **Entries older than 12 months stop displaying.** Harvest data that old isn't decision-useful. This is designed behavior, not an error -- `doctor` stays quiet about it. Refresh the figure and the date to bring it back. + + The audit nag keeps firing past 12 months, and says + `no longer displayed` once the annotation has retired -- otherwise the + annotation would simply vanish with nothing to explain why. - **The sign doesn't matter.** `harvested:num:45300` and `harvested:num:-45300` are equivalent; the annotation's parentheses already carry the accounting "this is a loss" convention. @@ -131,6 +142,9 @@ It appears in four places: breakdown value, or the contributions attribution. It is an annotation, not an input. +The two thresholds together: fresh for 90 days, nagged from 90 to 365, +nagged *and* hidden past 365. + ### Why not put it in the account name? Because the account name is a **join key**, not a label. It is matched diff --git a/src/commands/audit/hygiene.zig b/src/commands/audit/hygiene.zig index fdfc73e..0c2ca22 100644 --- a/src/commands/audit/hygiene.zig +++ b/src/commands/audit/hygiene.zig @@ -3,10 +3,10 @@ //! When `zfin audit` runs with no brokerage flag, this is what //! executes: stale-manual-price detection, working-tree-vs-HEAD //! price/date mismatch detection, account-cadence overdue checks, -//! automatic brokerage-file discovery + reconciliation, and the -//! large-new-lot transfer nudge. +//! stale hand-declared `harvested` figures, automatic brokerage-file +//! discovery + reconciliation, and the large-new-lot transfer nudge. //! -//! The reconciliation in Section 4 delegates to the per-broker +//! The reconciliation in Section 5 delegates to the per-broker //! modules (`fidelity.zig`, `schwab.zig`) and the shared display in //! `common.zig`; this file owns only the hygiene logic, so a future //! `zfin doctor` can reuse it without pulling in the reconciler @@ -28,6 +28,7 @@ const test_git = @import("../../testutil/git.zig"); const common = @import("common.zig"); const fidelity = @import("fidelity.zig"); const schwab = @import("schwab.zig"); +const fmt = cli.fmt; // ── Hygiene check (flagless audit) ────────────────────────── @@ -43,6 +44,20 @@ const stale_warning_multiplier: u32 = 2; // yellow -> red at 2× threshold /// pathological case where an account never changed in tracked history /// (so it never resolves) from forcing a scan of the entire repo. const max_history_commits_scanned: usize = 500; +/// Age at which `zfin audit` starts nagging that an account's +/// hand-declared `harvested` figure (see `AccountTaxEntry.harvested`) +/// needs a refresh. +/// +/// Not configurable, and deliberately so: unlike `update_cadence` - +/// which nags every account by default and therefore *needs* a +/// per-account silence knob - this section only ever considers accounts +/// that explicitly declare `harvested`. Not wanting the nag and not +/// wanting the field are the same choice, so the opt-out already exists. +/// +/// 90 days is the same interval as `UpdateCadence.quarterly`, but is +/// deliberately an independent constant: retuning the account +/// reconciliation cadence shouldn't silently move this. +const harvested_stale_days: u32 = 90; /// Type of a discovered brokerage file. const BrokerFileKind = enum { @@ -421,6 +436,98 @@ fn staleLessThan(_: void, a: StaleManualPrice, b: StaleManualPrice) bool { return std.mem.order(u8, a.symbol, b.symbol) == .lt; } +/// One account whose hand-declared `harvested` figure has gone stale +/// (or was never dated). `account` borrows from the `AccountMap` it was +/// collected from and is valid for that map's lifetime. +const StaleHarvest = struct { + account: []const u8, + /// Days since `harvested_date`; `null` when the entry declares + /// `harvested` with no `harvested_date` - the worst case, since an + /// undated figure can't be aged and never renders at all. + age_days: ?i32, + /// True when the figure no longer renders anywhere. + /// + /// Derived by asking `format.fmtHarvestAnnotation` whether it would + /// emit anything, NOT by comparing `age_days` against 365. Those two + /// disagree at leap-year boundaries, because the formatter gates on + /// the calendar-exact `as_of.subtractYears(1)`. Going through the + /// formatter makes the "no longer displayed" message definitionally + /// true - the nag cannot contradict what the user sees. + hidden: bool, +}; + +/// Collect accounts whose `harvested` figure is older than +/// `harvested_stale_days`, or that declare `harvested` with no +/// `harvested_date`, for the "Stale harvested figures" hygiene section. +/// +/// Only accounts that explicitly declare `harvested` are considered - +/// that declaration IS the opt-in to being nagged, which is why the +/// threshold needs no per-account override. +/// +/// Future-dated entries fall out as fresh (their age is negative). +/// That's deliberate: `zfin doctor` already reports a future +/// `harvested_date` as the config typo it is, and surfacing the same +/// mistake a second time in staleness clothing would be worse than +/// reporting it once. +/// +/// Caller owns the returned list; `account` fields borrow from +/// `account_map`. +fn collectStaleHarvested( + allocator: std.mem.Allocator, + account_map: analysis.AccountMap, + as_of: Date, +) !std.ArrayList(StaleHarvest) { + var out = std.ArrayList(StaleHarvest).empty; + errdefer out.deinit(allocator); + + const threshold: i32 = @intCast(harvested_stale_days); + for (account_map.entries) |e| { + if (e.harvested == null) continue; + + // SAFETY: immediately overwritten by fmtHarvestAnnotation below. + var ann_buf: [fmt.harvest_annotation_max_len]u8 = undefined; + const hidden = fmt.fmtHarvestAnnotation(&ann_buf, e.harvested, e.harvested_date, as_of).len == 0; + + if (e.harvested_date) |on| { + const age = as_of.days - on.days; + if (age <= threshold) continue; // fresh enough (or future-dated) + try out.append(allocator, .{ .account = e.account, .age_days = age, .hidden = hidden }); + } else { + try out.append(allocator, .{ .account = e.account, .age_days = null, .hidden = hidden }); + } + } + return out; +} + +/// Sort stale harvested figures worst-first: undated entries (which +/// never render), then oldest, then account name as a stable tiebreak. +/// +/// Deliberately unlike `staleLessThan`, which sorts alphabetically: +/// that section emits many rows per account and needs them grouped, +/// while this one emits exactly one row per account, so severity order +/// is strictly more useful. +fn harvestLessThan(_: void, a: StaleHarvest, b: StaleHarvest) bool { + if (a.age_days == null and b.age_days != null) return true; + if (a.age_days != null and b.age_days == null) return false; + if (a.age_days) |ad| { + const bd = b.age_days.?; + if (ad != bd) return ad > bd; + } + return std.mem.order(u8, a.account, b.account) == .lt; +} + +/// How many accounts declare a `harvested` figure at all. Lets the +/// display distinguish "nothing to report because you don't use this +/// feature" (print nothing) from "nothing to report because everything +/// is current" (print a reassuring `(none)`). +fn countHarvestedAccounts(account_map: analysis.AccountMap) usize { + var n: usize = 0; + for (account_map.entries) |e| { + if (e.harvested != null) n += 1; + } + return n; +} + /// A lot whose manual `price` moved between HEAD and the working tree /// while its `price_date` stayed identical - the "bumped the price, /// forgot the date" mistake. String fields borrow from the working- @@ -840,7 +947,48 @@ pub fn runHygieneCheck( } } - // ── Section 3: Discover brokerage files ── + // ── Section 3: Stale harvested figures ── + // + // Accounts whose hand-declared `harvested` figure (accounts.srf) is + // older than `harvested_stale_days`, or that declare it with no + // `harvested_date`. Unlike Section 2 this ages an explicit date + // field rather than walking git history, so the two are separate + // sections - one "days ago" column with two meanings would mislead. + // + // The nag has no upper bound on purpose. Past 12 months the + // annotation stops rendering (see `format.fmtHarvestAnnotation`), + // and this section is then the only thing that explains where it + // went - so it escalates its wording instead of going quiet. + { + const declared = countHarvestedAccounts(account_map); + // Silent for portfolios that don't use the feature at all; once + // an account declares a figure, say so either way. + if (declared > 0) { + var stale_harvest = try collectStaleHarvested(allocator, account_map, as_of); + defer stale_harvest.deinit(allocator); + std.mem.sort(StaleHarvest, stale_harvest.items, {}, harvestLessThan); + + try out.print("\n", .{}); + try cli.printFg(out, color, cli.CLR_MUTED, " Stale harvested figures (>{d} days - refresh 'harvested' in accounts.srf)\n", .{harvested_stale_days}); + + if (stale_harvest.items.len == 0) { + try cli.printFg(out, color, cli.CLR_POSITIVE, " (none)\n", .{}); + } else { + for (stale_harvest.items) |e| { + try out.print(" {s:<32} ", .{e.account}); + if (e.age_days) |ad| { + const clr = stalenessColor(ad, harvested_stale_days); + const suffix: []const u8 = if (e.hidden) " - no longer displayed" else ""; + try cli.printFg(out, color, clr, "last updated {d} days ago{s}\n", .{ @as(u32, @intCast(ad)), suffix }); + } else { + try cli.printFg(out, color, cli.CLR_NEGATIVE, "no harvested_date set\n", .{}); + } + } + } + } + } + + // ── Section 4: Discover brokerage files ── // Resolve audit directories const portfolio_dir = std.fs.path.dirnamePosix(portfolio_path) orelse "."; @@ -883,7 +1031,7 @@ pub fn runHygieneCheck( } } - // ── Section 4: Auto-reconcile discovered files ── + // ── Section 5: Auto-reconcile discovered files ── if (all_files.items.len > 0) { // Build prices map (shared by all reconciliations) @@ -1022,7 +1170,7 @@ pub fn runHygieneCheck( } } - // ── Section 5: Large new lots - confirm source ── + // ── Section 6: Large new lots - confirm source ── // // Cross-check any new_* Change with value >= threshold against // `transaction_log.srf` (via the shared contributions pipeline). @@ -1048,7 +1196,7 @@ pub fn runHygieneCheck( } } - // ── Section 6: Unhandled stock splits ── + // ── Section 7: Unhandled stock splits ── // // Held symbols with a split AFTER a lot's purchase date that haven't // opted into automatic split adjustment. Each needs a @@ -1373,6 +1521,187 @@ test "collectStaleManualPrices: lot without a manual price is skipped" { try std.testing.expectEqual(@as(usize, 0), stale.items.len); } +// ── collectStaleHarvested ──────────────────────────────────── + +/// Build an in-memory AccountMap from a literal SRF string, mirroring +/// the helper in `analytics/analysis.zig`. Exercises the real parse +/// path so these tests can't drift from how `accounts.srf` is read. +fn testAccountMap(comptime data: []const u8) !analysis.AccountMap { + return analysis.parseAccountsFile(std.testing.allocator, data); +} + +test "collectStaleHarvested: no account declares harvested -> empty" { + const allocator = std.testing.allocator; + var am = try testAccountMap( + \\#!srfv1 + \\account::Sample Brokerage,tax_type::taxable + ); + defer am.deinit(); + + var stale = try collectStaleHarvested(allocator, am, Date.fromYmd(2026, 7, 25)); + defer stale.deinit(allocator); + try std.testing.expectEqual(@as(usize, 0), stale.items.len); + try std.testing.expectEqual(@as(usize, 0), countHarvestedAccounts(am)); +} + +test "collectStaleHarvested: figure within the window is fresh" { + const allocator = std.testing.allocator; + var am = try testAccountMap( + \\#!srfv1 + \\account::Sample Tax Loss,tax_type::taxable,harvested:num:45300,harvested_date::2026-06-24 + ); + defer am.deinit(); + + // 31 days old. + var stale = try collectStaleHarvested(allocator, am, Date.fromYmd(2026, 7, 25)); + defer stale.deinit(allocator); + try std.testing.expectEqual(@as(usize, 0), stale.items.len); + // ...but it IS declared, so the section still prints "(none)". + try std.testing.expectEqual(@as(usize, 1), countHarvestedAccounts(am)); +} + +test "collectStaleHarvested: exactly at the threshold is still fresh" { + const allocator = std.testing.allocator; + var am = try testAccountMap( + \\#!srfv1 + \\account::Sample Tax Loss,tax_type::taxable,harvested:num:45300,harvested_date::2026-01-01 + ); + defer am.deinit(); + + const on = Date.fromYmd(2026, 1, 1); + const at_threshold = on.addDays(@intCast(harvested_stale_days)); + var stale = try collectStaleHarvested(allocator, am, at_threshold); + defer stale.deinit(allocator); + try std.testing.expectEqual(@as(usize, 0), stale.items.len); + + // One day later it trips. + var stale2 = try collectStaleHarvested(allocator, am, at_threshold.addDays(1)); + defer stale2.deinit(allocator); + try std.testing.expectEqual(@as(usize, 1), stale2.items.len); + try std.testing.expectEqual(@as(?i32, @intCast(harvested_stale_days + 1)), stale2.items[0].age_days); + try std.testing.expect(!stale2.items[0].hidden); +} + +test "collectStaleHarvested: undated figure is flagged with a null age" { + const allocator = std.testing.allocator; + var am = try testAccountMap( + \\#!srfv1 + \\account::Sample Tax Loss,tax_type::taxable,harvested:num:45300 + ); + defer am.deinit(); + + var stale = try collectStaleHarvested(allocator, am, Date.fromYmd(2026, 7, 25)); + defer stale.deinit(allocator); + try std.testing.expectEqual(@as(usize, 1), stale.items.len); + try std.testing.expectEqualStrings("Sample Tax Loss", stale.items[0].account); + try std.testing.expectEqual(@as(?i32, null), stale.items[0].age_days); + // An undated figure never renders, so it's hidden too. + try std.testing.expect(stale.items[0].hidden); +} + +test "collectStaleHarvested: future-dated figure is left to doctor" { + // A future `harvested_date` is a config typo, and `zfin doctor` + // already reports it as one. Surfacing the same mistake here in + // staleness clothing would double-nag for a single error. + const allocator = std.testing.allocator; + var am = try testAccountMap( + \\#!srfv1 + \\account::Sample Tax Loss,tax_type::taxable,harvested:num:45300,harvested_date::2027-06-24 + ); + defer am.deinit(); + + var stale = try collectStaleHarvested(allocator, am, Date.fromYmd(2026, 7, 25)); + defer stale.deinit(allocator); + try std.testing.expectEqual(@as(usize, 0), stale.items.len); +} + +test "collectStaleHarvested: hidden agrees with fmtHarvestAnnotation across the 12-month edge" { + // The load-bearing invariant: the "no longer displayed" message must + // be true. `hidden` is derived from the formatter rather than from a + // day count precisely so the two can't disagree at a leap-year + // boundary. Walk a window that straddles the cutoff and assert + // agreement on every day. + const allocator = std.testing.allocator; + var am = try testAccountMap( + \\#!srfv1 + \\account::Sample Tax Loss,tax_type::taxable,harvested:num:45300,harvested_date::2024-02-29 + ); + defer am.deinit(); + + const on = Date.fromYmd(2024, 2, 29); + var offset: i32 = 300; + while (offset <= 430) : (offset += 1) { + const as_of = on.addDays(offset); + var stale = try collectStaleHarvested(allocator, am, as_of); + defer stale.deinit(allocator); + try std.testing.expectEqual(@as(usize, 1), stale.items.len); // always past 90d here + + // SAFETY: immediately overwritten by fmtHarvestAnnotation. + var buf: [fmt.harvest_annotation_max_len]u8 = undefined; + const rendered = fmt.fmtHarvestAnnotation(&buf, 45_300, on, as_of); + try std.testing.expectEqual(rendered.len == 0, stale.items[0].hidden); + } +} + +test "collectStaleHarvested: hidden is set once the annotation retires" { + const allocator = std.testing.allocator; + var am = try testAccountMap( + \\#!srfv1 + \\account::Sample Tax Loss,tax_type::taxable,harvested:num:45300,harvested_date::2024-06-24 + ); + defer am.deinit(); + + // ~400 days later: stale AND no longer rendered. + var stale = try collectStaleHarvested(allocator, am, Date.fromYmd(2025, 7, 29)); + defer stale.deinit(allocator); + try std.testing.expectEqual(@as(usize, 1), stale.items.len); + try std.testing.expectEqual(@as(?i32, 400), stale.items[0].age_days); + try std.testing.expect(stale.items[0].hidden); +} + +test "harvestLessThan: undated first, then oldest, then account name" { + const undated_b: StaleHarvest = .{ .account = "B", .age_days = null, .hidden = true }; + const undated_a: StaleHarvest = .{ .account = "A", .age_days = null, .hidden = true }; + const old: StaleHarvest = .{ .account = "C", .age_days = 400, .hidden = true }; + const newer: StaleHarvest = .{ .account = "D", .age_days = 100, .hidden = false }; + + // Undated outranks any dated entry, however old. + try std.testing.expect(harvestLessThan({}, undated_b, old)); + try std.testing.expect(!harvestLessThan({}, old, undated_b)); + // Among dated, older first. + try std.testing.expect(harvestLessThan({}, old, newer)); + try std.testing.expect(!harvestLessThan({}, newer, old)); + // Ties break on account name, so output is stable. + try std.testing.expect(harvestLessThan({}, undated_a, undated_b)); + try std.testing.expect(!harvestLessThan({}, undated_b, undated_a)); +} + +test "collectStaleHarvested + sort: mixed accounts come out worst-first" { + const allocator = std.testing.allocator; + var am = try testAccountMap( + \\#!srfv1 + \\account::Sample Fresh,tax_type::taxable,harvested:num:1000,harvested_date::2026-07-01 + \\account::Sample Mild,tax_type::taxable,harvested:num:2000,harvested_date::2026-01-01 + \\account::Sample Ancient,tax_type::taxable,harvested:num:3000,harvested_date::2024-01-01 + \\account::Sample Undated,tax_type::taxable,harvested:num:4000 + \\account::Sample None,tax_type::taxable + ); + defer am.deinit(); + + var stale = try collectStaleHarvested(allocator, am, Date.fromYmd(2026, 7, 25)); + defer stale.deinit(allocator); + std.mem.sort(StaleHarvest, stale.items, {}, harvestLessThan); + + try std.testing.expectEqual(@as(usize, 4), countHarvestedAccounts(am)); + try std.testing.expectEqual(@as(usize, 3), stale.items.len); // Fresh and None excluded + try std.testing.expectEqualStrings("Sample Undated", stale.items[0].account); + try std.testing.expectEqualStrings("Sample Ancient", stale.items[1].account); + try std.testing.expectEqualStrings("Sample Mild", stale.items[2].account); + // Only the ancient one has retired from display. + try std.testing.expect(stale.items[1].hidden); + try std.testing.expect(!stale.items[2].hidden); +} + // ── findPriceDateMismatches ────────────────────────────────── test "findPriceDateMismatches: price moved but date unchanged is flagged" { @@ -1719,7 +2048,7 @@ test "findPriceDateMismatches: duplicate HEAD identity collapses to one entry" { try std.testing.expectApproxEqAbs(@as(f64, 160), mismatches.items[0].new_price, 0.01); } -test "runHygieneCheck: Section 6 flags an un-opted-in symbol's split, not an opted-in one" { +test "runHygieneCheck: Section 7 flags an un-opted-in symbol's split, not an opted-in one" { const allocator = std.testing.allocator; const io = std.testing.io; var tmp = std.testing.tmpDir(.{}); @@ -1773,7 +2102,7 @@ test "runHygieneCheck: Section 6 flags an un-opted-in symbol's split, not an opt const output = aw.written(); try std.testing.expect(std.mem.indexOf(u8, output, "Portfolio hygiene") != null); - // Assert against Section 6 specifically (it's the last section, so + // Assert against Section 7 specifically (it's the last section, so // slice from its header to end) - robust even if NVDA/AMZN surface // in an earlier section. The un-opted-in NVDA is listed; the // opted-in AMZN is not.