diff --git a/docs/reference/config/projections-srf.md b/docs/reference/config/projections-srf.md index b2afc24..4f53fc2 100644 --- a/docs/reference/config/projections-srf.md +++ b/docs/reference/config/projections-srf.md @@ -67,13 +67,14 @@ sleeve, and would not be comparable against the portfolio row (which is renormalized). Both rows therefore describe invested assets only. The row labels say so, and they are renormalized to match: a leg reads -`SPYM (89.4% of benchmark)`, not its raw portfolio weight, so that +`SPY (84.2% of benchmark)`, not its raw portfolio weight, so that multiplying the printed weights by the printed returns reproduces the printed blend. The blend row names the share of the portfolio it covers, -`Benchmark (94.9% of portfolio)`, and omits that clause entirely once +`Benchmark (95.0% of portfolio)`, and omits that clause entirely once coverage reaches 99.5%. A leg's raw portfolio weight is still recoverable -as `of benchmark x of portfolio` -- here `0.894 x 0.949 = 84.9%`, which -is the figure the `Target allocation` line reports. +as `of benchmark x of portfolio` -- for an 80/15 split that is +`0.842 x 0.950 = 80.0%`, which is the figure the `Target allocation` line +reports. The year columns are **total return** (dividend-reinvested). The `Week` column is price-only, because dividends over seven days are negligible; diff --git a/src/commands/contributions.zig b/src/commands/contributions.zig index 30bf9ce..a956852 100644 --- a/src/commands/contributions.zig +++ b/src/commands/contributions.zig @@ -734,9 +734,45 @@ fn resolveEndpoints( try maybeSnapNote(io, arena, env, repo, before, range.before_rev, "before"); } + // Deliberately NOT gated on `verbosity`. `.silent` exists because + // `computeAttributionSpec` calls this speculatively and an unresolvable window + // is an ordinary null return - but that is the `compare` path, and `compare`'s + // headline is exactly what a straddled anchor corrupts. This note only fires + // when a snapshot WAS resolved and disagreed with the copy on disk, so it can + // never fire on the case `.silent` was introduced for. + maybeSnapshotAnchorNote(io, range.snapshot_anchor); + return .{ .range = range, .label = label }; } +/// Say something when the snapshot anchor was not clean. +/// +/// Both cases mean the same thing operationally: the value side of a comparison +/// reads `history/-portfolio.srf` off disk while the attribution side reads +/// `portfolio.srf` at a commit, and those two can describe different portfolios. +/// When they do, `gains = delta - contributions` mixes windows and the error is +/// invisible in the output - it looks like a plausible number. The motivating case +/// had a review's contributions and a five-figure outflow counted in two +/// consecutive reports because the anchor predated the reconcile. +/// +/// `corrected_from` is a note rather than a warning: the anchor moved and the +/// answer is now right, but the operator should know their history has a commit +/// where the snapshot and the portfolio disagreed, because that is a staging +/// slip that will recur. +fn maybeSnapshotAnchorNote(io: std.Io, anchor: ?git.SnapshotAnchor) void { + const a = anchor orelse return; + var buf: [420]u8 = undefined; + if (a.corrected_from) |from| { + const msg = std.fmt.bufPrint(&buf, "Note: the {f} snapshot was regenerated after it was first committed; anchoring attribution on {s} rather than {s}, which described different positions.\n", .{ a.date, shortSha(a.commit), shortSha(from) }) catch return; + cli.stderrPrint(io, msg); + return; + } + if (a.unmatched) { + const msg = std.fmt.bufPrint(&buf, "Warning: no commit of the {f} snapshot matches the copy on disk, so the value and attribution sides describe different portfolios. Contributions and gains for this window are NOT reliable - re-run `zfin snapshot --force` for that date and commit it alongside portfolio.srf.\n", .{a.date}) catch return; + cli.stderrPrint(io, msg); + } +} + /// Render a `CommitSpec` for user-facing error messages. Dates and /// working-copy sentinels get formatted; refs are passed through. /// When `spec` is null, returns "(unset)". @@ -3429,12 +3465,13 @@ fn printNone(out: *std.Io.Writer, color: bool, muted: [3]u8) !void { try cli.printFg(out, color, muted, " (none)\n", .{}); } -/// Symbol column. 16 because `Engagement Ring` is 15 - illiquid assets carry -/// free-form names rather than tickers, so the old 14 was sized for a population -/// this report no longer only contains. +/// Symbol column. 16 rather than 14, because a portfolio may carry illiquid assets +/// under free-form names instead of tickers, and 15-character names are ordinary. +/// The old width was sized for a population this report no longer only contains. const sym_w = 16; -/// Account column. 30 because `Fidelity Emil 401(k) Roth BL` is 28. +/// Account column. 30, because a plan account's full name runs to about 28 +/// characters once it carries a plan type and a sub-account qualifier. const acct_w = 30; /// Pad `s` into a `w`-wide field, always leaving at least one space behind it. @@ -3442,10 +3479,10 @@ const acct_w = 30; /// The guarantee is the point. `{s:= w) 1 else w - s.len); @@ -3477,16 +3514,17 @@ test "padTo: pads short values to width and never welds a long one" { try padTo(&w1, "CASH", 16); try std.testing.expectEqualStrings("CASH ", w1.buffered()); - // Exactly one under: still a gutter, and it is the last width that aligns. + // Exactly one under the width: still a gutter, and the last length that aligns. var w2 = std.Io.Writer.fixed(&buf); - try padTo(&w2, "Engagement Ring", 16); - try std.testing.expectEqualStrings("Engagement Ring ", w2.buffered()); + try padTo(&w2, "Fifteen Chars15", 16); + try std.testing.expectEqualStrings("Fifteen Chars15 ", w2.buffered()); // Over: alignment is gone, whitespace is not. `{s:<16}` produced no space - // here at all, which ran the value into the next column. + // here at all, which ran the value into the next column. An OCC option + // description is the realistic overflow. var w3 = std.Io.Writer.fixed(&buf); - try padTo(&w3, "AMZN 09/18/2026 280.00 C", 16); - try std.testing.expectEqualStrings("AMZN 09/18/2026 280.00 C ", w3.buffered()); + try padTo(&w3, "ZZZZ 01/01/2030 100.00 C", 16); + try std.testing.expectEqualStrings("ZZZZ 01/01/2030 100.00 C ", w3.buffered()); } fn printTotalLine(out: *std.Io.Writer, label: []const u8, v: f64, color: bool, hdr: [3]u8) !void { diff --git a/src/git.zig b/src/git.zig index 14ca4fd..e8852e3 100644 --- a/src/git.zig +++ b/src/git.zig @@ -116,6 +116,27 @@ pub const CommitRange = struct { before_rev: []const u8, /// null = working copy; non-null = a concrete git revision. after_rev: ?[]const u8, + /// How the before-side anchor was chosen, when it came from a `.snapshot_add` + /// spec. Non-null only in that case, and only worth reading to WARN - the + /// resolution has already happened. See `snapshotAnchor`. + snapshot_anchor: ?SnapshotAnchor = null, +}; + +/// The outcome of resolving a `.snapshot_add` anchor, so the caller can say +/// something when it was not clean. +pub const SnapshotAnchor = struct { + commit: []const u8, + /// The snapshot's own date, for the message. + date: Date, + /// Set when the anchor moved FORWARD off `commitThatAdded` because that + /// commit's snapshot described different positions than the snapshot on disk. + /// Names the commit we moved off. + corrected_from: ?[]const u8 = null, + /// Set when NO commit touching the snapshot describes the same positions as + /// the copy on disk. `commit` is then a best effort and the value side and the + /// attribution side are KNOWN to disagree - which is the one situation that + /// silently corrupts a weekly headline, so it must be said out loud. + unmatched: bool = false, }; // ── Implementation ─────────────────────────────────────────── @@ -428,9 +449,17 @@ pub fn lastCommitTimestampForPath( /// The weekly review writes `history/-portfolio.srf` for each market day and /// commits the whole batch together with the reconciled `portfolio.srf`. So the /// commit that ADDED a given Friday's snapshot IS that week's reconciliation -/// commit, by construction. Resolving it needs no commit-message convention, is -/// unaffected by how many other commits landed that day, and survives later -/// rewrites of the file's contents. +/// commit, *provided the batch really was one commit*. Resolving it needs no +/// commit-message convention and is unaffected by how many other commits landed +/// that day. +/// +/// That proviso is load-bearing and it has been violated: on 2026-08-29 one commit +/// added the snapshot and touched no portfolio, and the whole reconcile landed the +/// next day. Callers wanting the attribution anchor should therefore go through +/// `snapshotAnchor`, which wraps this and verifies the answer against the snapshot +/// on disk. This function stays narrow - "when did this path first appear" - and +/// is deliberately still the STARTING point, because the alternatives below are +/// all worse in the common case. /// /// The alternatives all failed in practice: /// @@ -671,6 +700,113 @@ pub fn commitTimestamp( /// 3. Both null -> full legacy mode: HEAD~1..HEAD (clean) or /// HEAD..working-copy (dirty). Back-compat with pre-flag /// `zfin contributions` invocations. +/// Which commit's `portfolio.srf` corresponds to the snapshot being used as the +/// "then" side. +/// +/// `commitThatAdded` answers a subtly different question - "when did this file +/// first appear" - and those diverge whenever the snapshot is REGENERATED in a +/// later commit that also moved `portfolio.srf`. That happens when a review is +/// committed in two passes: the first commit carries the snapshot, the second +/// carries the reconcile and force-refreshes the snapshot to match. Anchoring on +/// the first commit then puts the window's start BEFORE a whole review's worth of +/// contributions, so the next window re-counts them and both reports show the same +/// money. The value side reads the snapshot off DISK and stays right, so only the +/// attribution side straddles - which is why the totals look plausible. +/// +/// The rule is deliberately conservative: **keep `commitThatAdded` unless its +/// snapshot disagrees with the one on disk about POSITIONS.** Two failure modes +/// have to be told apart and only share counts do it: +/// +/// - A regenerated snapshot changes `shares`, because the portfolio changed. +/// That is the case above, and the anchor must move forward. +/// - A retroactive RESTATEMENT changes prices, tickers and values across many +/// snapshots at once while every share count stays put. One such commit +/// rewrote 97 snapshots and also touched `portfolio.srf`, so "last commit +/// touching the snapshot" and "last commit touching both" BOTH resolve to it +/// and both collapse the window. Comparing shares ignores it, correctly. +/// +/// When the anchor does move, it moves to the EARLIEST matching commit rather +/// than the latest: the latest would land on a restatement in the mixed case. +/// +/// Returns null when the snapshot was never committed, so the caller can fall back. +fn snapshotAnchor( + io: std.Io, + arena: std.mem.Allocator, + env: *const std.process.Environ.Map, + root: []const u8, + snap_rel: []const u8, + date: Date, +) Error!?SnapshotAnchor { + const first = (try commitThatAdded(io, arena, env, root, snap_rel)) orelse return null; + + // The on-disk copy is the reference because it is what the value side reads. + const disk_path = std.fs.path.join(arena, &.{ root, snap_rel }) catch return error.OutOfMemory; + const disk_text = std.Io.Dir.cwd().readFileAlloc(io, disk_path, arena, .limited(32 * 1024 * 1024)) catch + // No working copy to compare against (a bare checkout, or the file was + // deleted). Nothing to verify, so keep the historical behaviour. + return .{ .commit = first, .date = date }; + const want = positionFingerprint(disk_text); + + const first_text = show(io, arena, env, root, first, snap_rel) catch return .{ .commit = first, .date = date }; + if (positionFingerprint(first_text) == want) return .{ .commit = first, .date = date }; + + // It disagrees. Walk every commit that touched the snapshot, oldest first, and + // take the first whose positions match what the value side will read. + const touches = listCommitsTouching(io, arena, env, root, snap_rel, null) catch + return .{ .commit = first, .date = date, .unmatched = true }; + var i = touches.len; + while (i > 0) { + i -= 1; + const sha = touches[i].commit; + const text = show(io, arena, env, root, sha, snap_rel) catch continue; + if (positionFingerprint(text) != want) continue; + if (std.mem.eql(u8, sha, first)) return .{ .commit = first, .date = date }; + return .{ .commit = sha, .date = date, .corrected_from = first }; + } + return .{ .commit = first, .date = date, .unmatched = true }; +} + +/// Order-insensitive hash of the (account, lot_symbol, shares) triples in a snapshot. +/// +/// Deliberately a scan and NOT an srf parse. This is a change DETECTOR, not a +/// reader: it decides whether two snapshots describe the same positions, and the +/// asymmetry of the costs sets the bar. A false positive moves the anchor forward +/// past commits it should not have, silently shrinking a window. A false negative +/// leaves a week's contributions counted twice in a headline. Both are bad, which +/// is why the three fields below are chosen precisely rather than generously. +/// +/// `lot_symbol` and NOT `symbol`. `symbol` is the RESOLVED market ticker, and that +/// is precisely the field a ticker restatement rewrites - one such commit updated +/// the resolved ticker on a couple of synthetic lots across ~100 snapshots while +/// every share count and every price stayed identical. Hashing `symbol` read that +/// as a position change and moved the anchor forward past a reconcile, which is +/// the exact regression `commitThatAdded` was chosen to avoid. `lot_symbol` is the +/// portfolio's own lot identity, equal to `symbol` for ordinary holdings and +/// stable across a restatement. +/// +/// Prices, values and cost bases are ignored for the same reason. +fn positionFingerprint(text: []const u8) u64 { + var total: u64 = 0; + var lines = std.mem.splitScalar(u8, text, '\n'); + while (lines.next()) |line| { + if (std.mem.indexOf(u8, line, "kind::lot") == null) continue; + var h: u64 = 0xcbf29ce484222325; + for ([_][]const u8{ "account::", "lot_symbol::", "shares:num:" }) |key| { + const at = std.mem.indexOf(u8, line, key) orelse continue; + const rest = line[at + key.len ..]; + const end = std.mem.indexOfScalar(u8, rest, ',') orelse rest.len; + for (std.mem.trim(u8, rest[0..end], " \t\r")) |c| { + h = (h ^ c) *% 0x100000001b3; + } + h = (h ^ 0xff) *% 0x100000001b3; + } + // Summed rather than chained, so record ORDER cannot change the result - + // a re-serialised snapshot may legitimately reorder lots. + total +%= h; + } + return total; +} + pub fn resolveCommitRangeSpec( io: std.Io, arena: std.mem.Allocator, @@ -687,6 +823,20 @@ pub fn resolveCommitRangeSpec( } // Resolve each endpoint independently. + // + // The before side is asked TWICE when it is a `.snapshot_add`: once here for + // the sha and once for the anchor detail the caller warns on. Both hit the + // same git plumbing, and paying for it keeps `resolveSpec` returning a plain + // sha for every other spec rather than threading an optional through all six. + var anchor: ?SnapshotAnchor = null; + if (before) |b| switch (b) { + .snapshot_add => |d| { + const snap_rel = try snapshotRelPath(arena, rel_paths, d); + anchor = try snapshotAnchor(io, arena, env, repo.root, snap_rel, d); + }, + else => {}, + }; + const before_rev: []const u8 = if (before) |b| try resolveSpec(io, arena, env, repo, rel_paths, b) else if (dirty) @@ -704,7 +854,7 @@ pub fn resolveCommitRangeSpec( else "HEAD"; - return .{ .before_rev = before_rev, .after_rev = after_rev }; + return .{ .before_rev = before_rev, .after_rev = after_rev, .snapshot_anchor = anchor }; } /// Resolve one non-working `CommitSpec` to a string git can consume. @@ -723,7 +873,7 @@ fn resolveSpec(io: std.Io, arena: std.mem.Allocator, env: *const std.process.Env }, .snapshot_add => |d| blk: { const snap_rel = try snapshotRelPath(arena, rel_paths, d); - if (try commitThatAdded(io, arena, env, repo.root, snap_rel)) |sha| break :blk sha; + if (try snapshotAnchor(io, arena, env, repo.root, snap_rel, d)) |a| break :blk a.commit; // No committed snapshot for that date. Fall back to the date behaviour // rather than erroring: a portfolio kept without a history directory is // a legitimate configuration, and this spec should degrade to the old @@ -1109,3 +1259,116 @@ test "snapshotRelPath places history beside the portfolio, not at the repo root" const empty = try snapshotRelPath(arena, &.{}, d); try std.testing.expectEqualStrings("history/2026-08-21-portfolio.srf", empty); } + +test "positionFingerprint: shares matter, resolved ticker and prices do not" { + const base = + \\kind::lot,symbol::AAA,lot_symbol::SYNTH-A,account::Acct One,shares:num:100.5,price:num:10.00 + \\kind::lot,symbol::BBB,lot_symbol::BBB,account::Acct Two,shares:num:2000,price:num:20.00 + \\ + ; + // A ticker restatement: the RESOLVED symbol moves while `lot_symbol`, shares + // and account do not. Prices move too. Must read as unchanged. + const restated = + \\kind::lot,symbol::AAAX,lot_symbol::SYNTH-A,account::Acct One,shares:num:100.5,price:num:10.00 + \\kind::lot,symbol::BBB,lot_symbol::BBB,account::Acct Two,shares:num:2000,price:num:31.00 + \\ + ; + try std.testing.expectEqual(positionFingerprint(base), positionFingerprint(restated)); + + // A withdrawal: shares move. This is the case that has to be caught. + const withdrawn = + \\kind::lot,symbol::AAA,lot_symbol::SYNTH-A,account::Acct One,shares:num:100.5,price:num:10.00 + \\kind::lot,symbol::BBB,lot_symbol::BBB,account::Acct Two,shares:num:1500,price:num:20.00 + \\ + ; + try std.testing.expect(positionFingerprint(base) != positionFingerprint(withdrawn)); + + // Record order is not content: a re-serialised snapshot may reorder lots. + const reordered = + \\kind::lot,symbol::BBB,lot_symbol::BBB,account::Acct Two,shares:num:2000,price:num:20.00 + \\kind::lot,symbol::AAA,lot_symbol::SYNTH-A,account::Acct One,shares:num:100.5,price:num:10.00 + \\ + ; + try std.testing.expectEqual(positionFingerprint(base), positionFingerprint(reordered)); + + // Same lot and shares in a different account is not the same portfolio. + const here = "kind::lot,symbol::BBB,lot_symbol::BBB,account::Acct Two,shares:num:2000\n"; + const there = "kind::lot,symbol::BBB,lot_symbol::BBB,account::Acct Three,shares:num:2000\n"; + try std.testing.expect(positionFingerprint(here) != positionFingerprint(there)); +} + +test "snapshotAnchor moves off the add-commit only when positions changed" { + // The two-pass-commit shape, reproduced: commit A adds the snapshot, commit B + // regenerates it with a withdrawal applied. Anchoring on A puts a whole + // review's contributions inside the FOLLOWING window. + const allocator = std.testing.allocator; + if (!test_git.available(allocator)) return; + + var tmp = std.testing.tmpDir(.{}); + defer tmp.cleanup(); + var path_buf: [std.fs.max_path_bytes]u8 = undefined; + const dir_len = try tmp.dir.realPathFile(std.testing.io, ".", &path_buf); + const dir = path_buf[0..dir_len]; + + try tmp.dir.createDirPath(std.testing.io, "history"); + const snap = "history/2026-08-28-portfolio.srf"; + const v_added = "kind::lot,symbol::BBB,lot_symbol::BBB,account::Acct Two,shares:num:2000,price:num:20.00\n"; + const v_final = "kind::lot,symbol::BBB,lot_symbol::BBB,account::Acct Two,shares:num:1500,price:num:20.00\n"; + + try tmp.dir.writeFile(std.testing.io, .{ .sub_path = snap, .data = v_added }); + try test_git.run(allocator, dir, null, &.{ "init", "-q" }); + try test_git.run(allocator, dir, null, &.{ "config", "user.email", "test@example.com" }); + try test_git.run(allocator, dir, null, &.{ "config", "user.name", "Test" }); + try test_git.run(allocator, dir, null, &.{ "config", "commit.gpgsign", "false" }); + try test_git.run(allocator, dir, null, &.{ "add", "." }); + try test_git.run(allocator, dir, "2026-08-29T12:00:00", &.{ "commit", "-q", "-m", "adds snapshot only" }); + + var env = gitTestEnv(allocator); + defer env.deinit(); + + const first = (try commitThatAdded(std.testing.io, allocator, &env, dir, snap)).?; + defer allocator.free(first); + + var arena_state = std.heap.ArenaAllocator.init(allocator); + defer arena_state.deinit(); + const arena = arena_state.allocator(); + const d = Date.fromYmd(2026, 8, 28); + + // While the on-disk copy still matches the add-commit, nothing moves and + // nothing is reported. + const clean = (try snapshotAnchor(std.testing.io, arena, &env, dir, snap, d)).?; + try std.testing.expectEqualStrings(first, clean.commit); + try std.testing.expect(clean.corrected_from == null); + try std.testing.expect(!clean.unmatched); + + // Now regenerate the snapshot with the withdrawal applied, and commit. + try tmp.dir.writeFile(std.testing.io, .{ .sub_path = snap, .data = v_final }); + try test_git.run(allocator, dir, null, &.{ "add", "." }); + try test_git.run(allocator, dir, "2026-08-30T12:00:00", &.{ "commit", "-q", "-m", "regenerate with reconcile" }); + + const moved = (try snapshotAnchor(std.testing.io, arena, &env, dir, snap, d)).?; + try std.testing.expect(!std.mem.eql(u8, first, moved.commit)); + try std.testing.expect(moved.corrected_from != null); + try std.testing.expectEqualStrings(first, moved.corrected_from.?); + try std.testing.expect(!moved.unmatched); + + // A pure ticker restatement on top must NOT move it again - that is the + // bulk-restatement case, and moving would swallow a reconcile. + const v_restated = "kind::lot,symbol::BBBX,lot_symbol::BBB,account::Acct Two,shares:num:1500,price:num:31.00\n"; + try tmp.dir.writeFile(std.testing.io, .{ .sub_path = snap, .data = v_restated }); + try test_git.run(allocator, dir, null, &.{ "add", "." }); + try test_git.run(allocator, dir, "2026-08-31T12:00:00", &.{ "commit", "-q", "-m", "restate ticker" }); + + const after_restate = (try snapshotAnchor(std.testing.io, arena, &env, dir, snap, d)).?; + try std.testing.expectEqualStrings(moved.commit, after_restate.commit); + + // And when the working copy describes positions no commit ever held, say so + // rather than picking one - the two sides genuinely disagree. + try tmp.dir.writeFile(std.testing.io, .{ + .sub_path = snap, + .data = "kind::lot,symbol::BBB,lot_symbol::BBB,account::Acct Two,shares:num:1.0,price:num:20.00\n", + }); + const orphan = (try snapshotAnchor(std.testing.io, arena, &env, dir, snap, d)).?; + try std.testing.expect(orphan.unmatched); + try std.testing.expectEqualStrings(first, orphan.commit); +} diff --git a/src/views/projections.zig b/src/views/projections.zig index 751bfdc..fc7a5a7 100644 --- a/src/views/projections.zig +++ b/src/views/projections.zig @@ -152,11 +152,11 @@ pub fn fmtAllocationNote(buf: []u8, target_stock_pct: ?f64, current_stock_pct: f /// weight present (`benchmark.blendOptional`), which is what stops a cash sleeve /// dragging the benchmark down against a portfolio return that excludes it too. /// -/// The labels have to agree with that arithmetic. Printing the raw 84.9%/10.0% -/// beside a row computed from 89.5%/10.5% invites the reader to multiply out and -/// get 17.14% where the row says 18.06%. Both numbers are correct and the -/// mismatch is entirely in the caption - the same trap that already cost an hour -/// on the `--as-of` table and is why `review` prints that table's date. +/// The labels have to agree with that arithmetic. On an 80/15 split, printing the +/// raw 80.0%/15.0% beside a row computed from 84.2%/15.8% invites the reader to +/// multiply out and get 16.15% where the row says 17.00%. Both numbers are correct +/// and the mismatch is entirely in the caption - the same trap that already cost an +/// hour on the `--as-of` table, which is why that table now prints its own date. /// /// Nothing is lost by renormalizing the caption: the raw portfolio weight of /// either leg is `stock/100 * covered`.