diff --git a/src/analytics/analysis.zig b/src/analytics/analysis.zig index 9d0aaaa..b61e46f 100644 --- a/src/analytics/analysis.zig +++ b/src/analytics/analysis.zig @@ -5,6 +5,7 @@ const std = @import("std"); const builtin = @import("builtin"); const srf = @import("srf"); +const srf_opts = @import("../srf_opts.zig"); const Allocation = @import("valuation.zig").Allocation; const ClassificationMap = @import("../models/classification.zig").ClassificationMap; const ClassificationEntry = @import("../models/classification.zig").ClassificationEntry; @@ -546,7 +547,7 @@ pub fn parseAccountsFile(allocator: std.mem.Allocator, data: []const u8) !Accoun defer it.deinit(); while (try it.next()) |fields| { - const entry = fields.to(AccountTaxEntry, .{}) catch continue; + const entry = fields.to(AccountTaxEntry, srf_opts.user_edited) catch continue; // A zero/negative large-lot threshold is nonsensical (zero // flags every new lot; negative is meaningless). Reject it and @@ -1271,6 +1272,28 @@ test "parseAccountsFile: cash_is_contribution default false, opt-in true" { try std.testing.expect(!am.cashIsContribution("Nonexistent")); } +test "parseAccountsFile: a hand-typed string separator on a numeric field still parses" { + // `accounts.srf` is hand-edited, so `harvested::5000` instead of + // `harvested:num:5000` is a slip rather than different intent. Under + // SRF's strict default that string reaches an unchecked + // `val.?.number` - a panic in Debug and undefined behaviour in + // ReleaseFast - which is why this parser opts into + // `srf_opts.user_edited`. Pinned because the option is easy to drop + // and the failure is silent in the build zfin actually ships. + const data = + \\#!srfv1 + \\account::Sample Brokerage,tax_type::taxable,harvested::5000,harvested_date::2026-06-01 + \\account::Sample IRA,tax_type::traditional,audit_large_lot_threshold::25000 + ; + const allocator = std.testing.allocator; + var am = try parseAccountsFile(allocator, data); + defer am.deinit(); + + try std.testing.expectEqual(@as(usize, 2), am.entries.len); + try std.testing.expectApproxEqAbs(@as(f64, 5000), am.entries[0].harvested.?, 0.001); + try std.testing.expectApproxEqAbs(@as(f64, 25000), am.entries[1].audit_large_lot_threshold.?, 0.001); +} + test "parseAccountsFile: direct_indexing default false, opt-in true" { const data = \\#!srfv1 diff --git a/src/analytics/projections.zig b/src/analytics/projections.zig index 4228529..c63552e 100644 --- a/src/analytics/projections.zig +++ b/src/analytics/projections.zig @@ -14,6 +14,7 @@ const builtin = @import("builtin"); const log = std.log.scoped(.projections); const shiller = @import("../data/shiller.zig"); const srf = @import("srf"); +const srf_opts = @import("../srf_opts.zig"); const Date = @import("../Date.zig"); /// `log.warn` wrapper that no-ops under `zig build test`. Used for @@ -762,7 +763,7 @@ pub fn parseProjectionsConfig(data: ?[]const u8) UserConfig { var annotation_count: u8 = 0; while (it.next() catch null) |field_it| { - const rec = field_it.to(SrfProjection, .{}) catch continue; + const rec = field_it.to(SrfProjection, srf_opts.user_edited) catch continue; switch (rec) { .config => |c| { config.target_stock_pct = c.target_stock_pct orelse config.target_stock_pct; diff --git a/src/cache/store.zig b/src/cache/store.zig index 0668952..47b630e 100644 --- a/src/cache/store.zig +++ b/src/cache/store.zig @@ -1,6 +1,7 @@ const std = @import("std"); const log = std.log.scoped(.cache); const srf = @import("srf"); +const srf_opts = @import("../srf_opts.zig"); const atomic = @import("../atomic.zig"); const version = @import("../version.zig"); const Date = @import("../Date.zig"); @@ -1343,7 +1344,7 @@ pub const Store = struct { const created = it.created orelse std.Io.Timestamp.now(self.io, .real).toSeconds(); const fields = (it.next() catch return null) orelse return null; - const meta = fields.to(CandleMeta, .{}) catch return null; + const meta = fields.to(CandleMeta, srf_opts.machine_written) catch return null; return .{ .meta = meta, .created = created }; } @@ -1889,7 +1890,7 @@ pub const Store = struct { } // Per-record coercion. Most types use SRF's generalized - // `fields.to(T, .{})` - correct for any struct shape but + // `fields.to(T, ...)` - correct for any struct shape but // pays a per-field abstraction cost (coerce() boundary, // found-bitmap bookkeeping, inline-for dispatch chain). // @@ -1904,7 +1905,7 @@ pub const Store = struct { var item: T = if (comptime T == Candle) coerceCandleSpecialized(fields) catch continue else - fields.to(T, .{}) catch continue; + fields.to(T, srf_opts.machine_written) catch continue; if (comptime postProcess) |pp| { pp(&item, allocator) catch { if (comptime @hasDecl(T, "deinit")) item.deinit(allocator); @@ -1968,7 +1969,7 @@ pub const Store = struct { defer it.deinit(); const fields = (try it.next()) orelse return error.InvalidData; - return fields.to(CandleMeta, .{}) catch error.InvalidData; + return fields.to(CandleMeta, srf_opts.machine_written) catch error.InvalidData; } // ── Private serialization: options (bespoke) ───────────────── @@ -2038,7 +2039,7 @@ pub const Store = struct { } while (try it.next()) |fields| { - const opt_rec = fields.to(OptionsRecord, .{}) catch continue; + const opt_rec = fields.to(OptionsRecord, srf_opts.machine_written) catch continue; switch (opt_rec) { .chain => |ch| { const idx = chains.items.len; @@ -2111,18 +2112,10 @@ pub fn deserializePortfolio(allocator: std.mem.Allocator, data: []const u8) !Por var skipped: usize = 0; while (try it.next()) |fields| { const line = it.state.line; - // `strings_to_numbers` because these are HUMAN-EDITED files, which is - // exactly the case srf's default strict coercion is not for - its own - // doc says "if you want to use this for human-edited files, turn this - // on". Strict mode assumes the writer was a machine, so a numeric - // field spelled with a string separator (`close_price::200.00` instead - // of `close_price:num:200.00`) reaches an unchecked `val.?.number` and - // takes the whole process down. One such typo was enough to panic every - // `zfin portfolio` run. - // - // The `catch` below still handles genuinely unparseable values; this - // only stops a hand-typed separator from being fatal. - var lot = fields.to(Lot, .{ .strings_to_numbers = true }) catch { + // `user_edited` coercion: see `srf_opts.zig` for why hand-edited + // files get different options from cache files. The `catch` + // below still handles genuinely unparseable values. + var lot = fields.to(Lot, srf_opts.user_edited) catch { std.log.warn("portfolio: could not parse record at line {d}", .{line}); skipped += 1; continue; diff --git a/src/commands/common.zig b/src/commands/common.zig index 3b24ee7..ed22375 100644 --- a/src/commands/common.zig +++ b/src/commands/common.zig @@ -2,6 +2,7 @@ const std = @import("std"); const builtin = @import("builtin"); const zfin = @import("../root.zig"); const srf = @import("srf"); +const srf_opts = @import("../srf_opts.zig"); const history = @import("../history.zig"); const git = @import("../git.zig"); const framework = @import("framework.zig"); @@ -1007,7 +1008,7 @@ pub fn loadWatchlist(io: std.Io, allocator: std.mem.Allocator, path: []const u8) var syms: std.ArrayList([]const u8) = .empty; while (it.next() catch null) |fields| { - const entry = fields.to(WatchEntry, .{}) catch continue; + const entry = fields.to(WatchEntry, srf_opts.user_edited) catch continue; const duped = allocator.dupe(u8, entry.symbol) catch continue; syms.append(allocator, duped) catch { allocator.free(duped); diff --git a/src/data/Journal.zig b/src/data/Journal.zig index 33347ca..589c21a 100644 --- a/src/data/Journal.zig +++ b/src/data/Journal.zig @@ -66,6 +66,7 @@ const std = @import("std"); const srf = @import("srf"); +const srf_opts = @import("../srf_opts.zig"); const Date = @import("../Date.zig"); const atomic = @import("../atomic.zig"); @@ -214,7 +215,7 @@ pub fn parse(allocator: std.mem.Allocator, data: []const u8) !Journal { defer it.deinit(); while (try it.next()) |fields| { - const rec = try fields.to(JournalRecord, .{}); + const rec = try fields.to(JournalRecord, srf_opts.user_edited); switch (rec) { .acknowledgment => |a| { try entries.append(allocator, .{ diff --git a/src/data/imported_values.zig b/src/data/imported_values.zig index dbd6ff5..f838a54 100644 --- a/src/data/imported_values.zig +++ b/src/data/imported_values.zig @@ -35,6 +35,7 @@ const std = @import("std"); const srf = @import("srf"); +const srf_opts = @import("../srf_opts.zig"); const Date = @import("../Date.zig"); // ── Types ──────────────────────────────────────────────────── @@ -176,7 +177,7 @@ pub fn parseImportedValues( errdefer points.deinit(allocator); while (it.next() catch return error.InvalidSrf) |fields| { - const point = fields.to(HistoryPoint, .{}) catch return error.InvalidSrf; + const point = fields.to(HistoryPoint, srf_opts.user_edited) catch return error.InvalidSrf; try points.append(allocator, point); } diff --git a/src/history.zig b/src/history.zig index e3a161e..2aaa99e 100644 --- a/src/history.zig +++ b/src/history.zig @@ -32,6 +32,7 @@ const std = @import("std"); const builtin = @import("builtin"); const srf = @import("srf"); +const srf_opts = @import("srf_opts.zig"); const snapshot = @import("models/snapshot.zig"); const Date = @import("Date.zig"); const timeline = @import("analytics/timeline.zig"); @@ -99,7 +100,7 @@ pub fn parseSnapshotBytes( // record kind we don't know about). Every other srf error // indicates malformed data in a record we SHOULD understand, so // we propagate it up rather than silently losing rows. - const rec = field_it.to(SnapshotRecord, .{}) catch |err| switch (err) { + const rec = field_it.to(SnapshotRecord, srf_opts.machine_written) catch |err| switch (err) { error.ActiveTagDoesNotExist => continue, else => return error.InvalidSrf, }; diff --git a/src/models/classification.zig b/src/models/classification.zig index a3fc18a..dd73c4c 100644 --- a/src/models/classification.zig +++ b/src/models/classification.zig @@ -11,6 +11,7 @@ /// symbol::02315N600,asset_class::Bonds,pct:num:15 const std = @import("std"); const srf = @import("srf"); +const srf_opts = @import("../srf_opts.zig"); const Date = @import("../Date.zig"); /// A single classification entry for a symbol. @@ -89,7 +90,7 @@ pub fn parseClassificationFile(allocator: std.mem.Allocator, data: []const u8) ! defer it.deinit(); while (try it.next()) |fields| { - const entry = fields.to(ClassificationEntry, .{}) catch continue; + const entry = fields.to(ClassificationEntry, srf_opts.user_edited) catch continue; // Pre-fill `bucket` if the user didn't curate one. This // shifts the cost of `deriveBucket` to parse time and // makes downstream code free to read `entry.bucket` @@ -269,6 +270,24 @@ test "parse classification file: missing name field stays null (backwards compat try std.testing.expectEqualStrings("Technology", cm.entries[0].sector.?); } +test "parse classification file: a hand-typed string separator on pct still parses" { + // `metadata.srf` is hand-edited, so `pct::60` instead of `pct:num:60` + // must not be fatal. See `srf_opts.user_edited` - under strict + // coercion this string reaches an unchecked `val.?.number`, which is + // undefined behaviour in ReleaseFast. + const data = + \\#!srfv1 + \\symbol::SYM,sector::Technology,pct::60 + \\symbol::SYM,sector::Healthcare,pct:num:40 + ; + var map = try parseClassificationFile(std.testing.allocator, data); + defer map.deinit(); + + try std.testing.expectEqual(@as(usize, 2), map.entries.len); + try std.testing.expectApproxEqAbs(@as(f64, 60), map.entries[0].pct, 0.001); + try std.testing.expectApproxEqAbs(@as(f64, 40), map.entries[1].pct, 0.001); +} + test "parse classification file: bucket round-trips" { const data = \\#!srfv1 diff --git a/src/models/transaction_log.zig b/src/models/transaction_log.zig index 33f8a8a..349f3a6 100644 --- a/src/models/transaction_log.zig +++ b/src/models/transaction_log.zig @@ -60,6 +60,7 @@ const std = @import("std"); const builtin = @import("builtin"); const srf = @import("srf"); +const srf_opts = @import("../srf_opts.zig"); const Date = @import("../Date.zig"); const logger = std.log.scoped(.transaction_log); @@ -248,7 +249,7 @@ pub fn parseTransactionLogFile( defer it.deinit(); while (try it.next()) |fields| { - const parsed = fields.to(TransferRecord, .{}) catch |err| { + const parsed = fields.to(TransferRecord, srf_opts.user_edited) catch |err| { // Tests intentionally feed malformed records to exercise the // skip path; real parse failures stay visible outside tests. if (!builtin.is_test) { @@ -560,6 +561,21 @@ test "parseTransactionLogFile: single cash transfer" { try testing.expect(r.note == null); } +test "parseTransactionLogFile: a hand-typed string separator on amount still parses" { + // Hand-edited file, so `amount::50000` instead of `amount:num:50000` + // must not be fatal. See `srf_opts.user_edited` - under strict + // coercion this string reaches an unchecked `val.?.number`, which is + // undefined behaviour in ReleaseFast. + var log = try parseTransactionLogFile(testing.allocator, + \\#!srfv1 + \\transfer::2026-05-02,type::cash,amount::50000,from::Acct A,to::Acct B,dest_lot::cash + \\ + ); + defer log.deinit(); + try testing.expectEqual(@as(usize, 1), log.transfers.len); + try testing.expectEqual(@as(f64, 50000), log.transfers[0].amount); +} + test "parseTransactionLogFile: single lot-destination transfer" { var log = try parseTransactionLogFile(testing.allocator, \\#!srfv1 diff --git a/src/service.zig b/src/service.zig index 45bd7ca..5883dc7 100644 --- a/src/service.zig +++ b/src/service.zig @@ -25,6 +25,7 @@ const Config = @import("Config.zig"); const cache = @import("cache/store.zig"); const freshness = @import("cache/freshness.zig"); const srf = @import("srf"); +const srf_opts = @import("srf_opts.zig"); const analysis = @import("analytics/analysis.zig"); const transaction_log = @import("models/transaction_log.zig"); const TwelveData = @import("providers/twelvedata.zig").TwelveData; @@ -2987,7 +2988,7 @@ pub const DataService = struct { defer it.deinit(); while (it.next() catch return result) |fields| { - const entry = fields.to(CusipEntry, .{}) catch continue; + const entry = fields.to(CusipEntry, srf_opts.machine_written) catch continue; if (entry.cusip.len == 0 or entry.ticker.len == 0) continue; // First occurrence wins; getOrPut stores the borrowed // slices directly - they live in `backing`, no dupe. @@ -3153,7 +3154,7 @@ pub const DataService = struct { var it = srf.iterator(&reader, arena, .{ .parse_allocator = .none }) catch return; defer it.deinit(); while (it.next() catch return) |fields| { - const e = fields.to(CusipEntry, .{}) catch continue; + const e = fields.to(CusipEntry, srf_opts.machine_written) catch continue; if (e.cusip.len == 0 or e.ticker.len == 0) continue; if (have.contains(e.cusip) or out.contains(e.cusip)) continue; const kc = arena.dupe(u8, e.cusip) catch continue; diff --git a/src/srf_opts.zig b/src/srf_opts.zig new file mode 100644 index 0000000..58be8d6 --- /dev/null +++ b/src/srf_opts.zig @@ -0,0 +1,56 @@ +//! SRF coercion policy, in one place. +//! +//! SRF encodes a field's type in its separator: `key::v` is a string, +//! `key:num:v` a number, `key:bool:v` a boolean. Coercion into a typed +//! struct therefore depends on the writer having picked the right one. +//! +//! That assumption holds for files zfin writes and breaks for files +//! people write, so the two get different options - and which one a +//! parser wants is a property of where the file came from, not of the +//! struct being parsed. Naming the two policies keeps that decision +//! visible at the call site instead of buried in whichever comment +//! happened to explain it first. + +const srf = @import("srf"); + +/// For files a HUMAN edits: `portfolio.srf`, `accounts.srf`, +/// `metadata.srf`, `watchlist.srf`, `transaction_log.srf`, +/// `projections.srf`, `imported_values.srf`, `acknowledgments.srf`, and +/// the keybind config. +/// +/// Accepts a string where a number was declared, because a hand-typed +/// `close_price::200.00` instead of `close_price:num:200.00` is a +/// slip, not a different intent. SRF's own doc says as much: strict +/// coercion is "intended for performant access for cache use cases... +/// if you want to use this for human-edited files, turn this on". +/// +/// It is also a safety measure, which is the part worth not +/// forgetting. Under strict coercion a string reaching a numeric field +/// falls through to an unchecked `val.?.number` inside SRF - a panic +/// in Debug/ReleaseSafe and undefined behaviour in ReleaseFast, which +/// is how zfin is built. One `close_price::200.00` once took down +/// every `zfin portfolio` run; the fix was to turn this on for +/// `portfolio.srf` alone, which left every other hand-edited file +/// exposed to the same typo. +/// +/// This is mitigation, not a cure. Two holes remain, both needing an +/// upstream fix in SRF's `coerce`: +/// +/// - a non-string, non-number value in a numeric field (say +/// `harvested:bool:true`) still reaches the unchecked access; +/// - enum fields ignore this option entirely, so a typo like +/// `security_type::stok` still hits `stringToEnum(...).?`. +pub const user_edited: srf.CoercionOptions = .{ .strings_to_numbers = true }; + +/// For files ZFIN writes: the candle, quote, and options caches, +/// `cusip_tickers.srf` under `cache_dir`, server responses, and the +/// `history/-portfolio.srf` snapshots produced by +/// `zfin snapshot`. +/// +/// Keeps SRF's strict default. The writer is a machine that always +/// emits `:num:` for numbers, so accepting a string instead would only +/// mask a serializer bug rather than tolerate a human slip - these +/// files are not edited after the fact. Strictness is also free +/// performance on the hot paths, where a cached candle file is +/// millions of records. +pub const machine_written: srf.CoercionOptions = .{}; diff --git a/src/tui/keybinds.zig b/src/tui/keybinds.zig index c822d98..3c90e74 100644 --- a/src/tui/keybinds.zig +++ b/src/tui/keybinds.zig @@ -1,6 +1,7 @@ const std = @import("std"); const vaxis = @import("vaxis"); const srf = @import("srf"); +const srf_opts = @import("../srf_opts.zig"); pub const Action = enum { quit, @@ -486,7 +487,7 @@ pub fn loadFromDataChecked(allocator: std.mem.Allocator, data: []const u8) LoadO var idx: usize = 0; while (ri.next() catch return .fallback) |fields| : (idx += 1) { - const raw = fields.to(RawRecord, .{}) catch |err| { + const raw = fields.to(RawRecord, srf_opts.user_edited) catch |err| { // Per-record parse failure (missing field, bad key // string, unknown action). Don't drop the whole file - // skip the record and warn the user. Record index is