diff --git a/src/commands/audit/hygiene.zig b/src/commands/audit/hygiene.zig index e588063..b746308 100644 --- a/src/commands/audit/hygiene.zig +++ b/src/commands/audit/hygiene.zig @@ -23,6 +23,7 @@ const portfolio_mod = @import("../../models/portfolio.zig"); const Date = @import("../../Date.zig"); const srf = @import("srf"); const git = @import("../../git.zig"); +const test_git = @import("../../testutil/git.zig"); const common = @import("common.zig"); const fidelity = @import("fidelity.zig"); @@ -36,6 +37,12 @@ const audit_file_max_age_hours = 24; const audit_file_max_size_non_csv = 512 * 1024; // 512KB, for non-CSV files only pub const default_stale_days: u32 = 3; const stale_warning_multiplier: u32 = 2; // yellow -> red at 2× threshold +/// Safety cap on how many portfolio commits the account-cadence walk +/// scans when resolving each account's last-update timestamp. The walk +/// normally early-exits once every account resolves; this bounds the +/// 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; /// Type of a discovered brokerage file. const BrokerFileKind = enum { @@ -237,6 +244,89 @@ fn lotToString(allocator: std.mem.Allocator, lot: portfolio_mod.Lot) ![]const u8 return std.fmt.allocPrint(allocator, "{f}", .{srf.fmt(portfolio_mod.Lot, &lots, .{ .emit_directives = false })}); } +/// Resolve, per account, the committer timestamp (Unix epoch seconds) +/// of the newest commit in which that account's lots changed in +/// `ri.rel_path`. Walks the file's git history newest-to-oldest, +/// diffing adjacent commit pairs and attributing each change to the +/// newer commit of the pair; results are written into `out`. +/// +/// The walk covers full history (no cadence-derived cutoff): an +/// account more than 2x its cadence overdue - exactly the ones we most +/// want an age readout for - would otherwise always fall outside a +/// 2x-cadence window and degrade to "no update history found". It +/// stops early once every account in `all_accounts` has a timestamp, +/// or after scanning `max_history_commits_scanned` commits (the safety +/// valve for a never-changed account). +/// +/// Accounts with no detectable change in tracked history are simply +/// absent from `out` (e.g. an account present since before the first +/// portfolio commit, whose initial add has no parent commit to diff +/// against); the caller renders those as "no update history found". +/// +/// Keys written into `out` are borrowed from `all_accounts` (stable +/// working-copy account-name pointers), never from the transient +/// historical portfolio strings. Git unavailability degrades to an +/// empty result rather than an error. +fn findLastUpdateTimestamps( + io: std.Io, + allocator: std.mem.Allocator, + env: *const std.process.Environ.Map, + ri: git.RepoInfo, + all_accounts: *const std.StringHashMap(void), + out: *std.StringHashMap(i64), +) !void { + const commits = git.listCommitsTouching(io, allocator, env, ri.root, ri.rel_path, null) catch &.{}; + defer git.freeCommitTouches(allocator, commits); + + var prev_data: ?[]const u8 = null; + defer if (prev_data) |pd| allocator.free(pd); + + for (commits, 0..) |ct, ci| { + // Stop once every account is resolved, or at the scan cap. + if (out.count() >= all_accounts.count()) break; + if (ci >= max_history_commits_scanned) break; + + const rev_data = git.show(io, allocator, env, ri.root, ct.commit, ri.rel_path) catch continue; + + if (ci > 0) { + if (prev_data) |pd| { + // rev_data is older, pd is newer (commits are newest-first). + var old_pf = zfin.cache.deserializePortfolio(allocator, rev_data) catch { + allocator.free(rev_data); + continue; + }; + defer old_pf.deinit(); + var new_pf = zfin.cache.deserializePortfolio(allocator, pd) catch { + allocator.free(rev_data); + continue; + }; + defer new_pf.deinit(); + + var mods = findModifiedAccounts(allocator, old_pf, new_pf) catch { + allocator.free(rev_data); + continue; + }; + defer mods.deinit(); + + // The newer commit's timestamp is when these accounts changed. + const update_ts = commits[ci - 1].timestamp; + + // Match against stable working-copy account names. + var acct_iter = all_accounts.keyIterator(); + while (acct_iter.next()) |stable_name| { + if (out.contains(stable_name.*)) continue; + if (mods.contains(stable_name.*)) { + try out.put(stable_name.*, update_ts); + } + } + } + } + + if (prev_data) |pd| allocator.free(pd); + prev_data = rev_data; + } +} + /// Staleness color based on age vs threshold. /// Returns CLR_MUTED for within threshold, warning for 1-2x, negative for >2x. fn stalenessColor(age_days: i32, threshold: u32) [3]u8 { @@ -670,68 +760,14 @@ pub fn runHygieneCheck( } // Find last update time for each account via git history. - // Walk commits newest-to-oldest, diffing adjacent pairs to find - // which accounts changed. Use working-copy account names as keys - // (stable lifetime) rather than historical portfolio strings. - // Only walk back far enough to hit red status (2× max cadence). + // Walks the portfolio file's full history (no cadence-derived + // cutoff), keyed by stable working-copy account names. Absent + // entries render as "no update history found" below. var last_update_ts = std.StringHashMap(i64).init(allocator); defer last_update_ts.deinit(); if (repo_info) |ri| { - // Compute the furthest we need to look back: 2× the max cadence - var max_threshold: u32 = 14; // 2× weekly default - for (account_map.entries) |entry| { - if (entry.update_cadence.thresholdDays()) |td| { - const red = td * stale_warning_multiplier; - if (red > max_threshold) max_threshold = red; - } - } - var since_buf: [32]u8 = undefined; - const since = std.fmt.bufPrint(&since_buf, "{d} days ago", .{max_threshold}) catch "30 days ago"; - - const commits = git.listCommitsTouching(io, allocator, env, ri.root, ri.rel_path, since) catch &.{}; - defer git.freeCommitTouches(allocator, commits); - - var prev_data: ?[]const u8 = null; - defer if (prev_data) |pd| allocator.free(pd); - - for (commits, 0..) |ct, ci| { - // Stop early if every account already has a timestamp - if (last_update_ts.count() >= all_accounts.count()) break; - - const rev_data = git.show(io, allocator, env, ri.root, ct.commit, ri.rel_path) catch continue; - - if (ci > 0) { - if (prev_data) |pd| { - // rev_data is older, pd is newer (commits are newest-first) - var old_pf = zfin.cache.deserializePortfolio(allocator, rev_data) catch { - allocator.free(rev_data); - continue; - }; - defer old_pf.deinit(); - var new_pf = zfin.cache.deserializePortfolio(allocator, pd) catch continue; - defer new_pf.deinit(); - - var mods = findModifiedAccounts(allocator, old_pf, new_pf) catch continue; - defer mods.deinit(); - - // The newer commit's timestamp is when these accounts were updated - const update_ts = commits[ci - 1].timestamp; - - // Match against stable working-copy account names - var acct_iter = all_accounts.keyIterator(); - while (acct_iter.next()) |stable_name| { - if (last_update_ts.contains(stable_name.*)) continue; - if (mods.contains(stable_name.*)) { - try last_update_ts.put(stable_name.*, update_ts); - } - } - } - } - - if (prev_data) |pd| allocator.free(pd); - prev_data = rev_data; - } + try findLastUpdateTimestamps(io, allocator, env, ri, &all_accounts, &last_update_ts); } // Display overdue accounts @@ -1716,3 +1752,86 @@ test "runHygieneCheck: Section 6 flags an un-opted-in symbol's split, not an opt try std.testing.expect(std.mem.indexOf(u8, sec6, "NVDA") != null); try std.testing.expect(std.mem.indexOf(u8, sec6, "AMZN") == 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. +fn testPortfolioSrf(buf: []u8, ira_shares: []const u8, roth_shares: []const u8) ![]const u8 { + return std.fmt.bufPrint(buf, "#!srfv1\n" ++ + "symbol::VOO,shares:num:{s},open_date::2026-01-01,open_price:num:1.00,account::Sample IRA\n" ++ + "symbol::BND,shares:num:{s},open_date::2026-01-01,open_price:num:1.00,account::Sample Roth\n", .{ ira_shares, roth_shares }); +} + +test "findLastUpdateTimestamps: resolves an account changed in a non-newest commit" { + const allocator = std.testing.allocator; + const io = std.testing.io; + + // Skip if `git` isn't on PATH (CI sandbox without git). + 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]; + + var buf: [512]u8 = undefined; + + // Commit A (oldest): IRA=100, Roth=50. + try tmp.dir.writeFile(io, .{ .sub_path = "portfolio.srf", .data = try testPortfolioSrf(&buf, "100", "50") }); + 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" }); + try test_git.run(allocator, dir, "2026-01-10T12:00:00", &.{ "commit", "-q", "-m", "A" }); + + // Commit B: IRA changes to 200 (Roth unchanged). This is the change + // the old 2x-cadence lookback window could exclude, leaving IRA as + // "no update history found". + try tmp.dir.writeFile(io, .{ .sub_path = "portfolio.srf", .data = try testPortfolioSrf(&buf, "200", "50") }); + try test_git.run(allocator, dir, null, &.{ "add", "portfolio.srf" }); + try test_git.run(allocator, dir, "2026-02-15T12:00:00", &.{ "commit", "-q", "-m", "B" }); + + // Commit C (newest): Roth changes to 60 (IRA unchanged). + try tmp.dir.writeFile(io, .{ .sub_path = "portfolio.srf", .data = try testPortfolioSrf(&buf, "200", "60") }); + try test_git.run(allocator, dir, null, &.{ "add", "portfolio.srf" }); + try test_git.run(allocator, dir, "2026-07-01T12:00:00", &.{ "commit", "-q", "-m", "C" }); + + 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); + + const ri = try git.findRepo(io, allocator, &env, pf_path); + defer { + allocator.free(ri.root); + allocator.free(ri.rel_path); + } + + // Authoritative committer timestamps, newest-first: [0]=C, [1]=B, [2]=A. + const commits = try git.listCommitsTouching(io, allocator, &env, ri.root, ri.rel_path, null); + defer git.freeCommitTouches(allocator, commits); + try std.testing.expectEqual(@as(usize, 3), commits.len); + + var all_accounts = std.StringHashMap(void).init(allocator); + defer all_accounts.deinit(); + try all_accounts.put("Sample IRA", {}); + try all_accounts.put("Sample Roth", {}); + + var out = std.StringHashMap(i64).init(allocator); + defer out.deinit(); + try findLastUpdateTimestamps(io, allocator, &env, ri, &all_accounts, &out); + + // Both accounts resolve. Critically "Sample IRA" - last changed in + // commit B, NOT the newest commit - is found rather than absent. + const ira_ts = out.get("Sample IRA") orelse return error.IraUnresolved; + const roth_ts = out.get("Sample Roth") orelse return error.RothUnresolved; + + // Each change attributed to the commit that actually made it. + try std.testing.expectEqual(commits[1].timestamp, ira_ts); // B + try std.testing.expectEqual(commits[0].timestamp, roth_ts); // C + try std.testing.expect(ira_ts < roth_ts); +} diff --git a/src/git.zig b/src/git.zig index 0404bed..d95ccb2 100644 --- a/src/git.zig +++ b/src/git.zig @@ -126,8 +126,11 @@ pub const CommitRange = struct { /// out by hooks that runs git against a different repo must explicitly /// opt out of the inherited env. /// +/// Exposed (pub) so the git-in-test-repo helpers in `testutil/git.zig` +/// reuse this exact scrubbing rather than re-implementing it. +/// /// Caller owns the returned map; free with `.deinit()`. -fn scrubbedEnv( +pub fn scrubbedEnv( allocator: std.mem.Allocator, base: *const std.process.Environ.Map, ) std.mem.Allocator.Error!std.process.Environ.Map { diff --git a/src/portfolio_loader.zig b/src/portfolio_loader.zig index 23f84c7..778fdeb 100644 --- a/src/portfolio_loader.zig +++ b/src/portfolio_loader.zig @@ -59,6 +59,7 @@ const zfin = @import("root.zig"); const framework = @import("commands/framework.zig"); const stderr = @import("stderr.zig"); const git = @import("git.zig"); +const test_git = @import("testutil/git.zig"); const enrichSplits = @import("models/portfolio.zig").enrichSplits; // ── Portfolio loading ──────────────────────────────────────── @@ -806,96 +807,11 @@ test "loadFromBytes: all-empty bytes returns empty portfolio" { // Kept minimal (one happy-path, one missing-file case); the // bulk of the logic is in `loadFromBytes`, covered above. -/// Build an environment map from the parent process with the GIT_* -/// variables stripped out. -/// -/// When these tests run inside a git hook (pre-commit, prek, etc.), -/// the hook runner sets `GIT_INDEX_FILE`, `GIT_DIR`, and -/// `GIT_WORK_TREE` to point at the outer repo's staging state. Git -/// inherits those env vars unconditionally - `git -C ` -/// changes the CWD but does NOT clear these env vars. The result is -/// that `git init` in our temp dir succeeds but subsequent `git -/// add`/`git commit` operate against the OUTER repo's index, with -/// blob references that don't exist in our temp dir's object store -/// ("invalid object 100644 for ''"). -/// -/// The hook runner can't fix this for us; per upstream guidance, -/// hooks (and code shelled out by hooks) that run git against a -/// different repo must explicitly opt out of the inherited env. See -/// https://github.com/j178/prek/issues/1786 for the prek-specific -/// instance and https://pre-commit.com/ for the analogous pre-commit -/// guidance. -fn buildScrubbedEnv(allocator: std.mem.Allocator) !std.process.Environ.Map { - var map = try testing.environ.createMap(allocator); - errdefer map.deinit(); - - // Strip every GIT_* variable. Iterating-while-removing isn't - // supported on the underlying ArrayHashMap, so collect first - // then remove. Keys are duped because `swapRemove` frees the - // map's owned key buffer, which would otherwise leave us with - // dangling pointers in `keys_to_remove`. - var keys_to_remove: std.ArrayList([]u8) = .empty; - defer { - for (keys_to_remove.items) |k| allocator.free(k); - keys_to_remove.deinit(allocator); - } - - var it = map.iterator(); - while (it.next()) |entry| { - if (std.mem.startsWith(u8, entry.key_ptr.*, "GIT_")) { - try keys_to_remove.append(allocator, try allocator.dupe(u8, entry.key_ptr.*)); - } - } - for (keys_to_remove.items) |key| _ = map.swapRemove(key); - - return map; -} - -/// Run a one-shot git command in `cwd` for test setup. Panics on -/// failure - these tests can't proceed without a working repo. -/// -/// Uses `buildScrubbedEnv` to drop inherited `GIT_*` env vars; see -/// that function's doc comment for why this matters under git -/// hooks. -fn gitInTestRepo(allocator: std.mem.Allocator, cwd: []const u8, argv: []const []const u8) !void { - const full_argv = try allocator.alloc([]const u8, argv.len + 3); - defer allocator.free(full_argv); - full_argv[0] = "git"; - full_argv[1] = "-C"; - full_argv[2] = cwd; - @memcpy(full_argv[3..], argv); - - var env_map = try buildScrubbedEnv(allocator); - defer env_map.deinit(); - - const result = try std.process.run(allocator, testing.io, .{ - .argv = full_argv, - .environ_map = &env_map, - .stdout_limit = .limited(64 * 1024), - }); - defer allocator.free(result.stdout); - defer allocator.free(result.stderr); - switch (result.term) { - .exited => |code| if (code != 0) { - std.log.err("git command failed (code {d}): {s}\nstderr: {s}", .{ code, std.mem.join(allocator, " ", argv) catch "?", result.stderr }); - return error.GitFailed; - }, - else => return error.GitFailed, - } -} - test "loadPortfolioFromPathsAtRev: union of two committed files at HEAD" { const allocator = testing.allocator; // Skip if `git` isn't on PATH (CI sandbox without git). - { - const probe = std.process.run(allocator, testing.io, .{ - .argv = &.{ "git", "--version" }, - .stdout_limit = .limited(1024), - }) catch return; - defer allocator.free(probe.stdout); - defer allocator.free(probe.stderr); - } + if (!test_git.available(allocator)) return; var tmp = std.testing.tmpDir(.{}); defer tmp.cleanup(); @@ -917,12 +833,12 @@ test "loadPortfolioFromPathsAtRev: union of two committed files at HEAD" { try tmp.dir.writeFile(testing.io, .{ .sub_path = "portfolio.srf", .data = file_a }); try tmp.dir.writeFile(testing.io, .{ .sub_path = "portfolio_other.srf", .data = file_b }); - try gitInTestRepo(allocator, dir, &.{ "init", "-q" }); - try gitInTestRepo(allocator, dir, &.{ "config", "user.email", "test@example.com" }); - try gitInTestRepo(allocator, dir, &.{ "config", "user.name", "Test" }); - try gitInTestRepo(allocator, dir, &.{ "config", "commit.gpgsign", "false" }); - try gitInTestRepo(allocator, dir, &.{ "add", "portfolio.srf", "portfolio_other.srf" }); - try gitInTestRepo(allocator, dir, &.{ "commit", "-q", "-m", "initial" }); + 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", "portfolio_other.srf" }); + try test_git.run(allocator, dir, null, &.{ "commit", "-q", "-m", "initial" }); const p1 = try std.fs.path.join(allocator, &.{ dir, "portfolio.srf" }); defer allocator.free(p1); @@ -950,14 +866,7 @@ test "loadPortfolioFromPathsAtRev: file added later is silently skipped at earli const allocator = testing.allocator; // Skip if `git` isn't on PATH. - { - const probe = std.process.run(allocator, testing.io, .{ - .argv = &.{ "git", "--version" }, - .stdout_limit = .limited(1024), - }) catch return; - defer allocator.free(probe.stdout); - defer allocator.free(probe.stderr); - } + if (!test_git.available(allocator)) return; var tmp = std.testing.tmpDir(.{}); defer tmp.cleanup(); @@ -973,12 +882,12 @@ test "loadPortfolioFromPathsAtRev: file added later is silently skipped at earli ; try tmp.dir.writeFile(testing.io, .{ .sub_path = "portfolio.srf", .data = file_a }); - try gitInTestRepo(allocator, dir, &.{ "init", "-q" }); - try gitInTestRepo(allocator, dir, &.{ "config", "user.email", "test@example.com" }); - try gitInTestRepo(allocator, dir, &.{ "config", "user.name", "Test" }); - try gitInTestRepo(allocator, dir, &.{ "config", "commit.gpgsign", "false" }); - try gitInTestRepo(allocator, dir, &.{ "add", "portfolio.srf" }); - try gitInTestRepo(allocator, dir, &.{ "commit", "-q", "-m", "initial" }); + 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" }); + try test_git.run(allocator, dir, null, &.{ "commit", "-q", "-m", "initial" }); // Add and commit a second portfolio file. After this commit, // commit 1 (where portfolio_other.srf doesn't exist yet) is @@ -992,8 +901,8 @@ test "loadPortfolioFromPathsAtRev: file added later is silently skipped at earli \\ ; try tmp.dir.writeFile(testing.io, .{ .sub_path = "portfolio_other.srf", .data = file_b }); - try gitInTestRepo(allocator, dir, &.{ "add", "portfolio_other.srf" }); - try gitInTestRepo(allocator, dir, &.{ "commit", "-q", "-m", "add second" }); + try test_git.run(allocator, dir, null, &.{ "add", "portfolio_other.srf" }); + try test_git.run(allocator, dir, null, &.{ "commit", "-q", "-m", "add second" }); const p1 = try std.fs.path.join(allocator, &.{ dir, "portfolio.srf" }); defer allocator.free(p1); diff --git a/src/testutil/git.zig b/src/testutil/git.zig new file mode 100644 index 0000000..c6ceae2 --- /dev/null +++ b/src/testutil/git.zig @@ -0,0 +1,76 @@ +//! Test-only helpers for building throwaway git repositories. +//! +//! Consolidates the "run a git command in a temp repo" pattern shared by +//! the portfolio-loader and audit-hygiene tests, so there is exactly one +//! copy of the `GIT_*` scrubbing + `git -C ` invocation + error +//! handling. The scrubbing itself is delegated to `git.scrubbedEnv` (the +//! same routine production `runGit` uses), so test setup and production +//! agree on how the inherited hook environment is neutralized. + +const std = @import("std"); +const git = @import("../git.zig"); + +/// Returns false when `git` isn't on PATH so a caller can skip (early +/// return) rather than fail in a sandbox without git: +/// +/// if (!test_git.available(allocator)) return; +pub fn available(allocator: std.mem.Allocator) bool { + const probe = std.process.run(allocator, std.testing.io, .{ + .argv = &.{ "git", "--version" }, + .stdout_limit = .limited(1024), + }) catch return false; + allocator.free(probe.stdout); + allocator.free(probe.stderr); + return true; +} + +/// Run `git -C ` for test setup. +/// +/// Inherited `GIT_*` vars are stripped (via `git.scrubbedEnv`) so the +/// command works even when the test process runs inside a git hook whose +/// `GIT_DIR` / `GIT_WORK_TREE` point at the outer repo. +/// +/// When `date_iso` is non-null it pins both author and committer dates +/// (e.g. `"2026-02-15T12:00:00"`), which makes commit timestamps +/// deterministic for history-walk tests. Returns `error.GitFailed` +/// (after logging the argv + stderr) on a non-zero exit. +pub fn run( + allocator: std.mem.Allocator, + cwd: []const u8, + date_iso: ?[]const u8, + argv: []const []const u8, +) !void { + const full_argv = try allocator.alloc([]const u8, argv.len + 3); + defer allocator.free(full_argv); + full_argv[0] = "git"; + full_argv[1] = "-C"; + full_argv[2] = cwd; + @memcpy(full_argv[3..], argv); + + var base = try std.testing.environ.createMap(allocator); + defer base.deinit(); + var env = try git.scrubbedEnv(allocator, &base); + defer env.deinit(); + + if (date_iso) |d| { + try env.put("GIT_AUTHOR_DATE", d); + try env.put("GIT_COMMITTER_DATE", d); + } + + const result = try std.process.run(allocator, std.testing.io, .{ + .argv = full_argv, + .environ_map = &env, + .stdout_limit = .limited(64 * 1024), + }); + defer allocator.free(result.stdout); + defer allocator.free(result.stderr); + switch (result.term) { + .exited => |code| if (code != 0) { + const joined = std.mem.join(allocator, " ", argv) catch null; + defer if (joined) |j| allocator.free(j); + std.log.err("git command failed (code {d}): {s}\nstderr: {s}", .{ code, joined orelse "?", result.stderr }); + return error.GitFailed; + }, + else => return error.GitFailed, + } +}