teach compare to use the git commit that added the snapshot for comparison

docs/guides/periodic-review.md

@@ -66,7 +66,7 @@ the attribution will lie to you.
 One command gives you the whole "since last review" picture:
 
 ```bash
-zfin compare 1W --projections --commit-before HEAD
+zfin compare 1W --projections
 ```
 
 - **`1W`** is the point of comparison -- the snapshot from one week ago.
@@ -75,8 +75,8 @@ zfin compare 1W --projections --commit-before HEAD
 - **`--projections`** folds in projected-return and safe-withdrawal
   (SWR@99%) deltas, then vs. now. (Costs ~1-2s per endpoint for the
   Monte Carlo search; add `--no-events` to exclude life events.)
-- **`--commit-before HEAD`** pins the contributions/gains attribution to
+- **No `--commit-before` needed.** Attribution now anchors itself on the
-  your latest reconciliation commit. This matters -- see
+  commit that *recorded* the comparison snapshot. See
   [Attribution and commit timing](#attribution-and-commit-timing).
 
 Read off the liquid-total delta, the contributions-vs-gains split, the
@@ -122,16 +122,51 @@ your backup and your audit trail.
 
 `compare` and `contributions` work out "contributions vs. gains" by
 walking the git history of `portfolio.srf`. The positional date (`1W`)
-picks the *snapshot* whose prices you compare against; **`--commit-before`**
+picks the *snapshot* whose prices you compare against; the attribution
-picks which *commit* anchors the attribution. Those two can drift apart.
+needs a *commit* to anchor on. If those two describe different periods,
+the split is wrong -- and it is wrong silently, because gains are derived
+as a residual (`delta - contributions`), so anything the attribution
+window misses is reported as market performance.
 
-If you reconcile on Saturday but don't commit until Monday, next
+**This is now handled for you.** The default anchor is the commit that
-Saturday's `1W` snapshot lands *before* your last commit -- so a bare
+ADDED `history/<date>-portfolio.srf` for the snapshot being compared
-`compare 1W` would attribute **two weeks** of contributions to one week.
+against. Because a review commits its snapshots together with the
-`--commit-before HEAD` sidesteps this by pinning attribution to your
+reconciled `portfolio.srf`, that commit *is* that week's
-most recent reconciliation commit regardless of the snapshot date. When
+reconciliation -- no naming convention, no dependence on how many times
-the dates line up anyway, the flag is harmless -- which is why it's
+you committed, and unaffected by later edits to the file.
-worth making a habit.
+
+**Do not pass `--commit-before HEAD`.** It used to be the recommended
+workaround for reconciling on Saturday and committing on Monday, and it
+worked only while `HEAD` happened to be the *previous* week's
+reconciliation commit. Commit anything during a review -- a data fix, a
+metadata tweak -- and the attribution window collapses to "uncommitted
+edits only" while the value window still spans the week. One observed
+case reported ~$23k of vested shares as market gains.
+
+The flag remains for pinning a window deliberately; it is no longer a
+habit worth having.
+
+### Re-snapshot after reconciling
+
+One ordering detail matters more than it looks. If a scheduled job
+writes daily snapshots, the one for the last market day is captured
+*before* you reconcile -- so it holds pre-reconcile share counts while
+`portfolio.srf` ends up holding post-reconcile ones. The two then
+disagree at the same commit, permanently, by one reconciliation:
+
+```
+history/2026-08-21-portfolio.srf   AMZN 1569   (pre-reconcile)
+portfolio.srf @ that same commit   AMZN 1656   (post-reconcile)
+```
+
+`compare` reads the snapshot for "then" and the live portfolio for
+"now", so those 87 shares appear in the value delta. Attribution walks
+`portfolio.srf` across commits, where they were already present -- so no
+contribution is found and the residual absorbs them.
+
+Fix: run `zfin snapshot --force` after reconciling and before
+committing. On a non-trading day it rewrites the last trading day's
+file, so it never invents a weekend snapshot.
 
 ## Relative dates
 

docs/reference/cli/compare.md

@@ -23,7 +23,7 @@ silently.
 | `--projections`                                        | Add projected-return and 99% safe-withdrawal deltas (adds ~1-2s per endpoint).                                                        |
 | `--no-events`                                          | With `--projections`, exclude life events.                                                                                            |
 | `--snapshot-before <DATE>` / `--snapshot-after <DATE>` | Override a side's snapshot (`--snapshot-after live` for the current portfolio).                                                       |
-| `--commit-before <SPEC>` / `--commit-after <SPEC>`     | Pin the git commit for the attribution block (`HEAD`, `HEAD~N`, SHA, or `working`). Useful when a review date and its commit diverge. |
+| `--commit-before <SPEC>` / `--commit-after <SPEC>`     | Pin the git commit for the attribution block (`HEAD`, `HEAD~N`, SHA, or `working`). Rarely needed: by default each side anchors on the commit that *recorded* its snapshot. Passing `HEAD` is usually wrong -- see [Attribution and commit timing](../../guides/periodic-review.md#attribution-and-commit-timing). |
 
 ## Example
 

src/commands/compare.zig

@@ -353,19 +353,19 @@ pub fn run(ctx: *framework.RunCtx, parsed: ParsedArgs) !void {
 
     // ── Snap dates to nearest-earlier snapshots ──────────────
     //
-    // The requested date may not correspond to an actual snapshot
+    // The requested date may not correspond to an actual snapshot file. Snap to the
-    // file. Snap to the most recent snapshot at-or-before. Record
+    // most recent snapshot at-or-before.
-    // the original requested date so we can pass it to the
+    //
-    // attribution git-window independently (liquid uses the snap'd
+    // The snapped dates are then used for BOTH the value window and the attribution
-    // date; attribution uses the requested date unless --commit-*
+    // window. They used to diverge - values on the snapped date, attribution on the
-    // overrides).
+    // requested one - which quietly guaranteed the two windows described different
+    // periods whenever a snap occurred, and `gains` (a residual) absorbed the
+    // difference. Snapping both keeps them aligned; see the `attr_before`
+    // construction below.
     var arena_state = std.heap.ArenaAllocator.init(allocator);
     defer arena_state.deinit();
     const arena = arena_state.allocator();
 
-    const then_date_requested = then_date;
-    const now_date_requested = now_date;
-
     const then_resolved = cli.resolveSnapshotOrExplain(io, arena, hist_dir, then_date) catch return error.SnapshotNotFound;
     if (!then_resolved.exact) {
         var stderr_buf: [256]u8 = undefined;
@@ -445,18 +445,29 @@ pub fn run(ctx: *framework.RunCtx, parsed: ParsedArgs) !void {
         }
     }
 
-    // Build the CommitSpecs for attribution. Explicit --commit-*
+    // Build the CommitSpecs for attribution. Explicit --commit-* overrides win;
-    // overrides win; otherwise fall back to the requested snapshot
+    // otherwise anchor on the commit that RECORDED each side's snapshot.
-    // date. now_is_live implies `null` for after (= working copy
+    //
-    // default handling downstream).
+    // `snapshot_add` rather than `date_at_or_before`, and it matters. The value
+    // window's endpoints are snapshot files, so the attribution window has to be
+    // "the commit that recorded that snapshot" for the two to describe the same
+    // period. A date lands on whatever commit happened to be nearest, which drifts
+    // whenever a review is done on one day and committed on another - and `HEAD`
+    // (the previous workaround for that drift) collapses the window entirely as
+    // soon as anything is committed mid-review. See `git.commitThatAdded`.
+    //
+    // Uses the SNAPPED dates, not the requested ones: the snapped date is the
+    // snapshot that actually supplied the values, so it is the one whose recording
+    // commit bounds them. `snapshot_add` degrades to the old date behaviour when no
+    // snapshot for that date was committed.
     const attr_before: git.CommitSpec = commit_before_override orelse
-        .{ .date_at_or_before = then_date_requested };
+        .{ .snapshot_add = then_date };
     const attr_after_opt: ?git.CommitSpec = if (commit_after_override) |s|
         s
     else if (now_is_live)
         null
     else
-        .{ .date_at_or_before = now_date_requested };
+        .{ .snapshot_add = now_date };
 
     // Opt-in: bring the "then" snapshot into the current split basis so
     // a split between the two dates doesn't read as a phantom position

src/commands/contributions.zig

@@ -744,7 +744,7 @@ fn specDisplayString(spec: ?git.CommitSpec, date_buf: *[10]u8) []const u8 {
     const s = spec orelse return "(unset)";
     return switch (s) {
         .git_ref => |r| r,
-        .date_at_or_before => |d| std.fmt.bufPrint(date_buf, "{f}", .{d}) catch "????-??-??",
+        .date_at_or_before, .snapshot_add => |d| std.fmt.bufPrint(date_buf, "{f}", .{d}) catch "????-??-??",
         .working_copy => "working",
     };
 }
@@ -845,6 +845,10 @@ fn specLabel(arena: std.mem.Allocator, spec: ?git.CommitSpec, resolved_ref: []co
     return switch (s) {
         .git_ref => |r| arena.dupe(u8, r),
         .date_at_or_before => |d| std.fmt.allocPrint(arena, "commit at-or-before {f}", .{d}),
+        // Names the anchor, because "the commit that recorded this snapshot" is a
+        // materially different claim from "some commit near this date" and the
+        // header is where a reader checks which window they got.
+        .snapshot_add => |d| std.fmt.allocPrint(arena, "the commit recording {f}", .{d}),
         .working_copy => arena.dupe(u8, "working copy"),
     };
 }

src/git.zig

@@ -78,6 +78,13 @@ pub const RepoInfo = struct {
 pub const CommitSpec = union(enum) {
     git_ref: []const u8,
     date_at_or_before: Date,
+    /// The commit that ADDED `history/<date>-portfolio.srf`.
+    ///
+    /// The stable anchor for a weekly window - see `commitThatAdded` for why this
+    /// beats a bare date or `HEAD`. Falls back to `date_at_or_before` when no
+    /// snapshot for that date was ever committed, so a portfolio with no history
+    /// directory keeps working unchanged.
+    snapshot_add: Date,
     working_copy,
 };
 
@@ -408,6 +415,68 @@ pub fn lastCommitTimestampForPath(
     return std.fmt.parseInt(i64, trimmed, 10) catch return null;
 }
 
+/// Return the SHA of the commit that ADDED `rel_path`, or null if the path was
+/// never added (or does not exist in history).
+///
+/// `--diff-filter=A` is the whole point. It matches only the commit that
+/// introduced the file, ignoring every later modification - which is what makes
+/// this a stable anchor where a date or `HEAD` is not.
+///
+/// ## Why anchoring on a snapshot's add-commit is the right window
+///
+/// The weekly review writes `history/<date>-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.
+///
+/// The alternatives all failed in practice:
+///
+///   - `HEAD` assumes HEAD is the PREVIOUS week's reconcile commit. Commit anything
+///     during a review - a data fix, a metadata tweak - and the attribution window
+///     silently collapses to "uncommitted edits only" while the value window still
+///     spans the week. Observed: five commits landed during one review, and ~$23k
+///     of vested shares got reported as market gains.
+///   - A date (`--until=<DATE> -- portfolio.srf`) is close, but drifts whenever a
+///     review is done on one day and committed on another, which is the documented
+///     "committed mid-week after the review date" edge case.
+///   - "Last commit touching history/" picks up retroactive restatements. One
+///     commit rewrote 97 historical snapshots at once; `--diff-filter=A` ignored it
+///     correctly, a plain path filter would not have.
+///
+/// Caller owns the returned string.
+pub fn commitThatAdded(
+    io: std.Io,
+    allocator: std.mem.Allocator,
+    env: *const std.process.Environ.Map,
+    root: []const u8,
+    rel_path: []const u8,
+) Error!?[]const u8 {
+    // `--follow` is deliberately NOT used. It would chase renames and could walk
+    // back into a differently-named predecessor, which is exactly the kind of
+    // silent window-widening this function exists to prevent.
+    const result = runGit(io, allocator, env, &.{
+        "git",         "-C",              root,
+        "log",         "--diff-filter=A", "-1",
+        "--format=%H", "--",              rel_path,
+    }, .limited(64 * 1024)) catch return error.GitUnavailable;
+    defer allocator.free(result.stdout);
+    defer allocator.free(result.stderr);
+
+    switch (result.term) {
+        .exited => |code| if (code != 0) return error.GitLogFailed,
+        else => return error.GitLogFailed,
+    }
+
+    const trimmed = std.mem.trim(u8, result.stdout, " \t\r\n");
+    if (trimmed.len == 0) return null;
+    // Same hash-shape guard as `commitAtOrBeforeDate`.
+    if (trimmed.len < 40) return error.GitLogFailed;
+    for (trimmed) |c| if (!std.ascii.isHex(c)) return error.GitLogFailed;
+    return try allocator.dupe(u8, trimmed);
+}
+
 /// Return the SHA of the most recent commit that touched any of
 /// `rel_paths` at or before `date_iso` (YYYY-MM-DD, inclusive
 /// end-of-day semantics via `git log --until`).
@@ -651,10 +720,36 @@ fn resolveSpec(io: std.Io, arena: std.mem.Allocator, env: *const std.process.Env
                 return error.NoCommitAtOrBefore;
             break :blk sha;
         },
+        .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;
+            // 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
+            // resolution instead of breaking it.
+            var buf: [10]u8 = undefined;
+            const date_str = std.fmt.bufPrint(&buf, "{f}", .{d}) catch buf[0..];
+            const sha = (try commitAtOrBeforeDate(io, arena, env, repo.root, rel_paths, date_str)) orelse
+                return error.NoCommitAtOrBefore;
+            break :blk sha;
+        },
         .working_copy => error.InvalidArg,
     };
 }
 
+/// `history/<date>-portfolio.srf`, relative to the repo root.
+///
+/// Derived from the portfolio's own repo-relative directory so a portfolio kept in
+/// a subdirectory resolves its sibling history dir rather than one at the root.
+fn snapshotRelPath(arena: std.mem.Allocator, rel_paths: []const []const u8, d: Date) Error![]const u8 {
+    const first = if (rel_paths.len > 0) rel_paths[0] else "portfolio.srf";
+    const dir = std.fs.path.dirname(first);
+    return if (dir) |dd|
+        std.fmt.allocPrint(arena, "{s}/history/{f}-portfolio.srf", .{ dd, d }) catch error.OutOfMemory
+    else
+        std.fmt.allocPrint(arena, "history/{f}-portfolio.srf", .{d}) catch error.OutOfMemory;
+}
+
 /// Back-compat wrapper for the original `Date`-based API. Existing
 /// callers (legacy `zfin contributions --since / --until`) keep
 /// working unchanged. New callers using explicit commit refs go
@@ -913,3 +1008,61 @@ test "resolveCommitRange: --since with no earlier commit -> NoCommitAtOrBefore"
     );
     try std.testing.expectError(error.NoCommitAtOrBefore, result);
 }
+
+test "commitThatAdded finds the introducing commit and ignores later edits" {
+    // Uses the ambient zfin checkout: `build.zig` was added once and modified many
+    // times since, which is exactly the shape `--diff-filter=A` must see through.
+    const allocator = std.testing.allocator;
+    var env = gitTestEnv(allocator);
+    defer env.deinit();
+    const info = findRepo(std.testing.io, allocator, &env, "build.zig") catch return;
+    defer allocator.free(info.root);
+    defer allocator.free(info.rel_path);
+
+    const add_sha = (commitThatAdded(std.testing.io, allocator, &env, info.root, info.rel_path) catch return) orelse return;
+    defer allocator.free(add_sha);
+    try std.testing.expect(add_sha.len == 40 or add_sha.len == 64);
+    for (add_sha) |c| try std.testing.expect(std.ascii.isHex(c));
+
+    // The add-commit must be at-or-older than the newest commit touching the file.
+    // If `--diff-filter=A` were being ignored these would be equal for a file with
+    // any edit history, which `build.zig` certainly has.
+    const newest = (commitAtOrBeforeDate(std.testing.io, allocator, &env, info.root, &.{info.rel_path}, "2099-01-01") catch return) orelse return;
+    defer allocator.free(newest);
+    try std.testing.expect(!std.mem.eql(u8, add_sha, newest));
+}
+
+test "commitThatAdded returns null for a path never committed" {
+    const allocator = std.testing.allocator;
+    var env = gitTestEnv(allocator);
+    defer env.deinit();
+    const info = findRepo(std.testing.io, allocator, &env, "build.zig") catch return;
+    defer allocator.free(info.root);
+    defer allocator.free(info.rel_path);
+
+    // Null, not an error: `resolveSpec` relies on this to fall back to the date
+    // behaviour for a portfolio kept without a committed history directory.
+    const sha_opt = commitThatAdded(std.testing.io, allocator, &env, info.root, "history/1970-01-01-portfolio.srf") catch return;
+    try std.testing.expect(sha_opt == null);
+}
+
+test "snapshotRelPath places history beside the portfolio, not at the repo root" {
+    var arena_state = std.heap.ArenaAllocator.init(std.testing.allocator);
+    defer arena_state.deinit();
+    const arena = arena_state.allocator();
+    const d = Date.fromYmd(2026, 8, 21);
+
+    // Portfolio at the repo root.
+    const at_root = try snapshotRelPath(arena, &.{"portfolio.srf"}, d);
+    try std.testing.expectEqualStrings("history/2026-08-21-portfolio.srf", at_root);
+
+    // Portfolio in a subdirectory: the history dir is its SIBLING. Resolving to a
+    // root-level `history/` would silently find nothing and fall back to the date
+    // behaviour, reintroducing the drift this spec exists to remove.
+    const nested = try snapshotRelPath(arena, &.{"accounts/portfolio.srf"}, d);
+    try std.testing.expectEqualStrings("accounts/history/2026-08-21-portfolio.srf", nested);
+
+    // Empty path list is defensive, not expected; must not crash.
+    const empty = try snapshotRelPath(arena, &.{}, d);
+    try std.testing.expectEqualStrings("history/2026-08-21-portfolio.srf", empty);
+}