From b3239eefda597d42553e7e83b30dc26fafd65621 Mon Sep 17 00:00:00 2001 From: Emil Lerch Date: Thu, 24 Sep 2026 08:54:47 -0700 Subject: [PATCH] make sure large new lots section remains without open_date --- src/commands/audit/hygiene.zig | 64 +++++++++++++++++++++++++- src/commands/contributions.zig | 82 +++++++++++++++++++++++++++++----- 2 files changed, 134 insertions(+), 12 deletions(-) diff --git a/src/commands/audit/hygiene.zig b/src/commands/audit/hygiene.zig index 0fb58ab..f949616 100644 --- a/src/commands/audit/hygiene.zig +++ b/src/commands/audit/hygiene.zig @@ -1244,7 +1244,7 @@ pub fn runHygieneCheck( // (not in a git repo). Threshold is per-account: an account's // `audit_large_lot_threshold` in accounts.srf wins, otherwise the // filter's built-in default applies. - if (contributions.findUnmatchedLargeLots(io, allocator, env, svc, portfolio_paths, &account_map, as_of, color, refresh)) |found| { + if (try contributions.findUnmatchedLargeLots(io, allocator, env, svc, portfolio_paths, &account_map, as_of, color, refresh)) |found| { var found_mut = found; defer found_mut.deinit(); @@ -2318,6 +2318,68 @@ test "runHygieneCheck: Section 7 flags an un-opted-in symbol's split, not an opt try std.testing.expect(std.mem.indexOf(u8, sec6, "AMZN") == null); } +test "runHygieneCheck: a large deposit on a cash_is_contribution account doesn't blank Large new lots" { + // End-to-end regression for the `MissingOpenDate` bug: the deposit + // becomes a `cash_contribution` with no open_date, which used to + // error out of `collectUnmatchedLargeLots` and suppress the whole + // section - taking the unrelated VTI lot down with it. + const allocator = std.testing.allocator; + const io = std.testing.io; + 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(io, ".", &path_buf); + const dir = path_buf[0..dir_len]; + + // Committed state: $10k cash on an account whose cash deposits are + // contributions. + try tmp.dir.writeFile(io, .{ + .sub_path = "portfolio.srf", + .data = "#!srfv1\nsecurity_type::cash,shares:num:10000,open_date::2025-01-01,open_price:num:1,account::Sample IRA\n", + }); + try tmp.dir.writeFile(io, .{ + .sub_path = "accounts.srf", + .data = "#!srfv1\naccount::Sample IRA,tax_type::traditional,cash_is_contribution:bool:true\n", + }); + 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", "portfolio.srf", "accounts.srf" }); + try test_git.run(allocator, dir, "2026-01-10T12:00:00", &.{ "commit", "-q", "-m", "A" }); + + // Working copy: a $50k deposit, plus a $25k new stock lot. + try tmp.dir.writeFile(io, .{ + .sub_path = "portfolio.srf", + .data = + \\#!srfv1 + \\security_type::cash,shares:num:60000,open_date::2025-01-01,open_price:num:1,account::Sample IRA + \\symbol::VTI,shares:num:100,open_date::2026-05-10,open_price:num:250,account::Sample IRA + \\ + , + }); + + var svc = zfin.DataService.init(io, allocator, .{ .cache_dir = dir }); + defer svc.deinit(); + var env = try std.testing.environ.createMap(allocator); + defer env.deinit(); + const pf_path = try std.fs.path.join(allocator, &.{ dir, "portfolio.srf" }); + defer allocator.free(pf_path); + + var aw: std.Io.Writer.Allocating = .init(allocator); + defer aw.deinit(); + try runHygieneCheck(io, allocator, &env, &svc, pf_path, &.{pf_path}, 3, false, zfin.Date.fromYmd(2026, 5, 11), 1_778_500_000, false, .never, &aw.writer); + + const out = aw.written(); + const start = std.mem.indexOf(u8, out, "Large new lots") orelse return error.LargeLotSectionMissing; + const sec = out[start..]; + try std.testing.expect(std.mem.indexOf(u8, sec, "VTI") != null); + try std.testing.expect(std.mem.indexOf(u8, sec, "+$50,000.00 (date unknown)") != null); + try std.testing.expect(std.mem.indexOf(u8, sec, "transfer::,type::cash,amount:num:50000.00") != null); +} + /// Format a two-account portfolio.srf for the git-history test. Only /// `shares` varies between revisions, which is enough for /// findModifiedAccounts to flag the account. diff --git a/src/commands/contributions.zig b/src/commands/contributions.zig index 5725059..bebb2e6 100644 --- a/src/commands/contributions.zig +++ b/src/commands/contributions.zig @@ -311,7 +311,7 @@ pub const meta: framework.Meta = .{ \\ , .uppercase_first_arg = false, - .user_errors = error{ DuplicateEndpoint, InvalidArg, MissingOpenDate, PrepareFailed, ResolveFailed, UnexpectedArg }, + .user_errors = error{ DuplicateEndpoint, InvalidArg, PrepareFailed, ResolveFailed, UnexpectedArg }, }; pub fn parseArgs(ctx: *framework.RunCtx, cmd_args: []const []const u8) !ParsedArgs { @@ -1199,7 +1199,10 @@ pub const UnmatchedLargeLotSet = struct { /// Mirrors the `zfin contributions` zero-flag path - uses /// `prepareReport`'s shared git + portfolio + transfer plumbing so /// the classification is identical. Returns null if the pipeline -/// can't resolve a window (not in a git repo, etc.). +/// can't resolve a window (not in a git repo, etc.) - the expected +/// reason for audit to skip the section, logged at debug level. Any +/// other failure is returned, never folded into that null: doing so +/// once made the whole section disappear over one undated lot. /// /// The threshold is resolved per lot: an account's /// `audit_large_lot_threshold` (from `account_map`) wins, otherwise @@ -1221,7 +1224,7 @@ pub fn findUnmatchedLargeLots( as_of: Date, color: bool, refresh: framework.RefreshPolicy, -) ?UnmatchedLargeLotSet { +) !?UnmatchedLargeLotSet { var arena_state = std.heap.ArenaAllocator.init(allocator); errdefer arena_state.deinit(); const arena = arena_state.allocator(); @@ -1234,16 +1237,14 @@ pub fn findUnmatchedLargeLots( // // Separate allocator here so we can tear the whole thing down // via `arena_state.deinit` once we've copied out the descriptors. - var ctx = prepareReport(io, allocator, arena, env, svc, paths, null, null, as_of, color, refresh, .silent) catch { + var ctx = prepareReport(io, allocator, arena, env, svc, paths, null, null, as_of, color, refresh, .silent) catch |err| { + std.log.scoped(.contributions).debug("large-lot check skipped: no comparable window ({t})", .{err}); arena_state.deinit(); return null; }; defer ctx.deinit(); - const lots = collectUnmatchedLargeLots(arena, ctx.report.changes, account_map) catch { - arena_state.deinit(); - return null; - }; + const lots = try collectUnmatchedLargeLots(arena, ctx.report.changes, account_map); return .{ .lots = lots, .arena = arena_state }; } @@ -1297,9 +1298,18 @@ fn collectUnmatchedLargeLots( const residual = c.attributedValue(); if (residual < threshold) continue; - // open_date is populated in Pass 1's new-lot branch; absence - // here would be a pipeline bug. - const od = c.open_date orelse return error.MissingOpenDate; + // Pass 1's new-lot branch sets open_date. A `cash_contribution` + // never has one: it is a balance INCREASE on an existing cash + // lot, and that lot's open_date is when the account was set + // up, not when this money arrived. `Date.epoch` is the + // codebase's "no date" (import writes it; `Lot.fromParsed` + // fills it), and `printLargeLotWarning` renders it as "date + // unknown" with a `` placeholder rather than a real date. + // + // This used to `return error.MissingOpenDate`, which the caller + // collapsed into "no section" - one large deposit to a + // `cash_is_contribution` account blanked the whole list. + const od = c.open_date orelse Date.epoch; const account_copy = try arena.dupe(u8, c.account); const symbol_copy = try arena.dupe(u8, c.symbol); try out.append(arena, .{ @@ -8272,3 +8282,53 @@ test "printReport: full report renders every section and sub-printer" { try printReport(&cw, &report, "portfolio.srf", true); try testing.expect(std.mem.indexOf(u8, cw.buffered(), "\x1b[") != null); } + +test "collectUnmatchedLargeLots: a large cash_contribution surfaces, undated, alongside the rest" { + // Regression. A positive balance change on a `cash_is_contribution` + // account is reclassified to `cash_contribution`, which carries no + // open_date. That returned error.MissingOpenDate, the caller turned + // it into "no section", and audit's entire "Large new lots" list - + // including the unrelated stock lot below - silently vanished. + var arena_state = std.heap.ArenaAllocator.init(std.testing.allocator); + defer arena_state.deinit(); + const allocator = arena_state.allocator(); + var prices = std.StringHashMap(f64).init(allocator); + defer prices.deinit(); + + var am = try analysis.parseAccountsFile( + allocator, + "#!srfv1\naccount::Sample IRA,tax_type::traditional,cash_is_contribution:bool:true\n", + ); + const before = [_]Lot{ + .{ .symbol = "cash", .shares = 10_000, .open_date = Date.fromYmd(2025, 1, 1), .open_price = 1.0, .security_type = .cash, .account = "Sample IRA" }, + }; + const after = [_]Lot{ + .{ .symbol = "cash", .shares = 60_000, .open_date = Date.fromYmd(2025, 1, 1), .open_price = 1.0, .security_type = .cash, .account = "Sample IRA" }, + .{ .symbol = "VTI", .shares = 100, .open_date = Date.fromYmd(2026, 5, 10), .open_price = 250.0, .account = "Sample IRA" }, + }; + + const report = try computeReport(allocator, &before, &after, &prices, Date.fromYmd(2026, 5, 11), .{ .account_map = &am }); + // Precondition: the balance change really was reclassified. + var saw_contribution = false; + for (report.changes) |c| { + if (c.kind == .cash_contribution) saw_contribution = true; + } + try std.testing.expect(saw_contribution); + + const lots = try collectUnmatchedLargeLots(allocator, report.changes, null); + try std.testing.expectEqual(@as(usize, 2), lots.len); + var cash_seen = false; + var stock_seen = false; + for (lots) |l| { + switch (l.security_type) { + .cash => { + cash_seen = true; + try std.testing.expectApproxEqAbs(@as(f64, 50_000.0), l.value, 0.01); + try std.testing.expect(l.open_date.eql(Date.epoch)); + }, + .stock => stock_seen = true, + else => {}, + } + } + try std.testing.expect(cash_seen and stock_seen); +}