diff --git a/TODO.md b/TODO.md index fe01bb9..0e2c4e3 100644 --- a/TODO.md +++ b/TODO.md @@ -597,28 +597,6 @@ likely culprits: No reproducer yet. When the user has a more specific lead (which tab, which interaction), this entry should narrow. -## CLI dispatch / arg-parsing bugs (found May 2026) - -Found during a post-framework-refactor sanity check of all 20 -commands plus interactive. The framework dispatch itself is -working correctly; these are gaps in command-level behavior or in -the global `--refresh-data` flag's coverage. - -### `zfin interactive --default-keys`/`--default-theme` swallow trailing flags — priority LOW - -`zfin interactive --default-keys --bogus-flag` is silently -accepted: `--default-keys` prints its output and `return`s from -`tui.run` before the rest of the args are parsed. The flag -parser itself now rejects unknown flags (`error.InvalidArgs` -+ exit 1), but only when control reaches the parsing loop — -which it doesn't if `--default-keys` or `--default-theme` -short-circuits first. - -Fix: validate the entire arg list before honoring the -print-and-exit flags, or restructure so the parser runs first -and the print-and-exit flags fire from inside the loop after -all args have been validated. - ### `etf ` warns `failed to serialize ETF profile: WriteFailed` — priority LOW Every `zfin etf VTI` invocation prints diff --git a/src/commands/audit.zig b/src/commands/audit.zig index 1376acf..5981074 100644 --- a/src/commands/audit.zig +++ b/src/commands/audit.zig @@ -2002,26 +2002,41 @@ pub const meta: framework.Meta = .{ \\ , .uppercase_first_arg = false, - .user_errors = error{ UnexpectedArg, EmptyFile, NoAccountsFound, UnexpectedHeader }, + .user_errors = error{ UnexpectedArg, EmptyFile, NoAccountsFound, UnexpectedHeader, MissingFlagValue }, }; -pub fn parseArgs(_: *framework.RunCtx, cmd_args: []const []const u8) !ParsedArgs { +pub fn parseArgs(ctx: *framework.RunCtx, cmd_args: []const []const u8) !ParsedArgs { var parsed: ParsedArgs = .{}; var i: usize = 0; while (i < cmd_args.len) : (i += 1) { - if (std.mem.eql(u8, cmd_args[i], "--fidelity") and i + 1 < cmd_args.len) { - i += 1; - parsed.fidelity_csv = cmd_args[i]; - } else if (std.mem.eql(u8, cmd_args[i], "--schwab") and i + 1 < cmd_args.len) { - i += 1; - parsed.schwab_csv = cmd_args[i]; - } else if (std.mem.eql(u8, cmd_args[i], "--schwab-summary")) { + const a = cmd_args[i]; + if (std.mem.eql(u8, a, "--fidelity")) { + parsed.fidelity_csv = try cli.requireFlagValue(ctx.io, cmd_args, &i, a); + } else if (std.mem.eql(u8, a, "--schwab")) { + parsed.schwab_csv = try cli.requireFlagValue(ctx.io, cmd_args, &i, a); + } else if (std.mem.eql(u8, a, "--schwab-summary")) { parsed.schwab_summary = true; - } else if (std.mem.eql(u8, cmd_args[i], "--verbose")) { + } else if (std.mem.eql(u8, a, "--verbose")) { parsed.verbose = true; - } else if (std.mem.eql(u8, cmd_args[i], "--stale-days") and i + 1 < cmd_args.len) { - i += 1; - parsed.stale_days = std.fmt.parseInt(u32, cmd_args[i], 10) catch default_stale_days; + } else if (std.mem.eql(u8, a, "--stale-days")) { + const v = try cli.requireFlagValue(ctx.io, cmd_args, &i, a); + // Reject a non-integer value loudly instead of silently + // falling back to the default; a typo'd threshold that + // silently reverts to 3 is a foot-gun. + parsed.stale_days = std.fmt.parseInt(u32, v, 10) catch { + cli.stderrPrint(ctx.io, "Error: --stale-days requires a non-negative integer, got: "); + cli.stderrPrint(ctx.io, v); + cli.stderrPrint(ctx.io, "\n"); + return error.UnexpectedArg; + }; + } else { + // Unknown flag or stray positional: reject explicitly + // rather than silently ignoring it (which previously let + // `audit --bogus` run flagless hygiene mode with no error). + cli.stderrPrint(ctx.io, "Error: unknown argument to 'audit': "); + cli.stderrPrint(ctx.io, a); + cli.stderrPrint(ctx.io, "\nRun 'zfin audit --help' for usage.\n"); + return error.UnexpectedArg; } } return parsed; @@ -2233,6 +2248,58 @@ test "parseArgs: --stale-days parses integer" { try std.testing.expectEqual(@as(u32, 5), parsed.stale_days); } +test "parseArgs: unknown flag is rejected (not silently ignored)" { + var ctx: framework.RunCtx = undefined; + ctx.io = std.testing.io; + const args = [_][]const u8{"--bogus"}; + try std.testing.expectError(error.UnexpectedArg, parseArgs(&ctx, &args)); +} + +test "parseArgs: stray positional is rejected" { + var ctx: framework.RunCtx = undefined; + ctx.io = std.testing.io; + const args = [_][]const u8{"AAPL"}; + try std.testing.expectError(error.UnexpectedArg, parseArgs(&ctx, &args)); +} + +test "parseArgs: --fidelity without a value is rejected (no silent mode switch)" { + var ctx: framework.RunCtx = undefined; + ctx.io = std.testing.io; + const args = [_][]const u8{"--fidelity"}; + try std.testing.expectError(error.MissingFlagValue, parseArgs(&ctx, &args)); +} + +test "parseArgs: --fidelity followed by a flag does not swallow the flag" { + var ctx: framework.RunCtx = undefined; + ctx.io = std.testing.io; + const args = [_][]const u8{ "--fidelity", "--verbose" }; + try std.testing.expectError(error.MissingFlagValue, parseArgs(&ctx, &args)); +} + +test "parseArgs: --schwab without a value is rejected" { + var ctx: framework.RunCtx = undefined; + ctx.io = std.testing.io; + const args = [_][]const u8{"--schwab"}; + try std.testing.expectError(error.MissingFlagValue, parseArgs(&ctx, &args)); +} + +test "parseArgs: --stale-days with a non-integer value is rejected (not a silent default)" { + var ctx: framework.RunCtx = undefined; + ctx.io = std.testing.io; + const args = [_][]const u8{ "--stale-days", "abc" }; + try std.testing.expectError(error.UnexpectedArg, parseArgs(&ctx, &args)); +} + +test "parseArgs: combined valid flags parse together" { + var ctx: framework.RunCtx = undefined; + ctx.io = std.testing.io; + const args = [_][]const u8{ "--fidelity", "/tmp/fid.csv", "--verbose", "--stale-days", "7" }; + const parsed = try parseArgs(&ctx, &args); + try std.testing.expectEqualStrings("/tmp/fid.csv", parsed.fidelity_csv.?); + try std.testing.expect(parsed.verbose); + try std.testing.expectEqual(@as(u32, 7), parsed.stale_days); +} + test "consolidateBySymbol: distinct symbols pass through unchanged" { const allocator = std.testing.allocator; const rows = [_]BrokeragePosition{ diff --git a/src/commands/common.zig b/src/commands/common.zig index 3f56381..c2ee40f 100644 --- a/src/commands/common.zig +++ b/src/commands/common.zig @@ -537,6 +537,56 @@ pub fn parseRequiredDateOrStderr( }; } +/// Consume the value argument for a value-taking flag during a +/// command's `parseArgs` loop. +/// +/// Call with `i` pointing at the flag token. On success, advances +/// `i.*` past the value (so the caller's `: (i += 1)` loop step +/// lands on the token after the value) and returns the value slice. +/// +/// Enforces the two invariants every value-flag wants: +/// 1. a value must follow the flag (not end-of-args), and +/// 2. the value must not be flag-shaped (a leading `-` with more +/// characters after it), which almost always means the user +/// forgot the value and the next flag got silently swallowed +/// as the "value". +/// +/// The lone `-` is deliberately ALLOWED through: it's the +/// conventional stdin/stdout sentinel (e.g. `import --wells-fargo +/// -`), not a flag. An empty-string value (`--flag ""`) is allowed +/// too (not flag-shaped); a deliberate empty argument is the +/// caller's to interpret. +/// +/// On violation, prints a specific stderr message naming `flag` +/// (and the offending token) and returns `error.MissingFlagValue`; +/// `i.*` is left unchanged. Callers list `MissingFlagValue` in +/// their `meta.user_errors` so the dispatcher maps it to a clean +/// exit 1. +pub fn requireFlagValue( + io: std.Io, + cmd_args: []const []const u8, + i: *usize, + flag: []const u8, +) error{MissingFlagValue}![]const u8 { + if (i.* + 1 >= cmd_args.len) { + stderrPrint(io, "Error: "); + stderrPrint(io, flag); + stderrPrint(io, " requires a value\n"); + return error.MissingFlagValue; + } + const value = cmd_args[i.* + 1]; + if (value.len > 1 and value[0] == '-') { + stderrPrint(io, "Error: "); + stderrPrint(io, flag); + stderrPrint(io, " requires a value, got flag: "); + stderrPrint(io, value); + stderrPrint(io, "\n"); + return error.MissingFlagValue; + } + i.* += 1; + return value; +} + // ── Commit-spec parsing (shared by contributions / compare) ── /// Re-export of `git.CommitSpec` so call sites already using `cli.*` @@ -716,6 +766,51 @@ test "parseCommitSpec: trims whitespace" { try std.testing.expect((try parseCommitSpec(" working ", today)) == .working_copy); } +test "requireFlagValue: returns value and advances index past it" { + const args = [_][]const u8{ "--out", "snap.srf" }; + var i: usize = 0; + const v = try requireFlagValue(std.testing.io, &args, &i, "--out"); + try std.testing.expectEqualStrings("snap.srf", v); + try std.testing.expectEqual(@as(usize, 1), i); +} + +test "requireFlagValue: missing value at end of args is rejected, index unchanged" { + const args = [_][]const u8{"--out"}; + var i: usize = 0; + try std.testing.expectError(error.MissingFlagValue, requireFlagValue(std.testing.io, &args, &i, "--out")); + try std.testing.expectEqual(@as(usize, 0), i); +} + +test "requireFlagValue: flag-shaped value is rejected (next flag not swallowed), index unchanged" { + const args = [_][]const u8{ "--out", "--force" }; + var i: usize = 0; + try std.testing.expectError(error.MissingFlagValue, requireFlagValue(std.testing.io, &args, &i, "--out")); + try std.testing.expectEqual(@as(usize, 0), i); +} + +test "requireFlagValue: empty-string value is accepted (not flag-shaped)" { + const args = [_][]const u8{ "--out", "" }; + var i: usize = 0; + const v = try requireFlagValue(std.testing.io, &args, &i, "--out"); + try std.testing.expectEqualStrings("", v); + try std.testing.expectEqual(@as(usize, 1), i); +} + +test "requireFlagValue: a value containing a non-leading dash is fine" { + const args = [_][]const u8{ "--out", "year-end.srf" }; + var i: usize = 0; + const v = try requireFlagValue(std.testing.io, &args, &i, "--out"); + try std.testing.expectEqualStrings("year-end.srf", v); +} + +test "requireFlagValue: lone '-' stdin sentinel is accepted" { + const args = [_][]const u8{ "--wells-fargo", "-" }; + var i: usize = 0; + const v = try requireFlagValue(std.testing.io, &args, &i, "--wells-fargo"); + try std.testing.expectEqualStrings("-", v); + try std.testing.expectEqual(@as(usize, 1), i); +} + /// Snap a requested snapshot date to the nearest earlier snapshot /// that exists in `hist_dir`, printing CLI-friendly stderr messages /// when resolution fails. diff --git a/src/commands/import.zig b/src/commands/import.zig index 55274f4..cb84d37 100644 --- a/src/commands/import.zig +++ b/src/commands/import.zig @@ -227,6 +227,7 @@ pub const meta: framework.Meta = .{ .uppercase_first_arg = false, .user_errors = error{ UnexpectedArg, + MissingFlagValue, MissingSource, ConflictingSources, MissingPortfolioPath, @@ -254,33 +255,13 @@ pub fn parseArgs(ctx: *framework.RunCtx, cmd_args: []const []const u8) !ParsedAr while (i < cmd_args.len) : (i += 1) { const a = cmd_args[i]; if (std.mem.eql(u8, a, "--fidelity")) { - if (i + 1 >= cmd_args.len) { - cli.stderrPrint(ctx.io, "Error: --fidelity requires a CSV path\n"); - return error.UnexpectedArg; - } - i += 1; - fidelity_path = cmd_args[i]; + fidelity_path = try cli.requireFlagValue(ctx.io, cmd_args, &i, a); } else if (std.mem.eql(u8, a, "--schwab")) { - if (i + 1 >= cmd_args.len) { - cli.stderrPrint(ctx.io, "Error: --schwab requires a CSV path\n"); - return error.UnexpectedArg; - } - i += 1; - schwab_path = cmd_args[i]; + schwab_path = try cli.requireFlagValue(ctx.io, cmd_args, &i, a); } else if (std.mem.eql(u8, a, "--wells-fargo")) { - if (i + 1 >= cmd_args.len) { - cli.stderrPrint(ctx.io, "Error: --wells-fargo requires a path (or '-' for stdin)\n"); - return error.UnexpectedArg; - } - i += 1; - wells_fargo_path = cmd_args[i]; + wells_fargo_path = try cli.requireFlagValue(ctx.io, cmd_args, &i, a); } else if (std.mem.eql(u8, a, "--account")) { - if (i + 1 >= cmd_args.len) { - cli.stderrPrint(ctx.io, "Error: --account requires a name\n"); - return error.UnexpectedArg; - } - i += 1; - account_override = cmd_args[i]; + account_override = try cli.requireFlagValue(ctx.io, cmd_args, &i, a); } else if (std.mem.eql(u8, a, "-y") or std.mem.eql(u8, a, "--yes")) { yes = true; } else { @@ -1418,7 +1399,25 @@ test "parseArgs: --fidelity without value errors" { var ctx: framework.RunCtx = undefined; ctx.io = testing.io; const args = [_][]const u8{"--fidelity"}; - try testing.expectError(error.UnexpectedArg, parseArgs(&ctx, &args)); + try testing.expectError(error.MissingFlagValue, parseArgs(&ctx, &args)); +} + +test "parseArgs: --fidelity followed by a flag does not swallow the flag" { + var ctx: framework.RunCtx = undefined; + ctx.io = testing.io; + const args = [_][]const u8{ "--fidelity", "--yes" }; + try testing.expectError(error.MissingFlagValue, parseArgs(&ctx, &args)); +} + +test "parseArgs: --wells-fargo accepts the lone '-' stdin sentinel" { + var ctx: framework.RunCtx = undefined; + ctx.io = testing.io; + const args = [_][]const u8{ "--wells-fargo", "-" }; + const parsed = try parseArgs(&ctx, &args); + switch (parsed.source) { + .wells_fargo => |wf| try testing.expectEqualStrings("-", wf.path), + else => try testing.expect(false), + } } test "Source.label: returns broker name" { diff --git a/src/commands/projections.zig b/src/commands/projections.zig index 4550690..6945dd7 100644 --- a/src/commands/projections.zig +++ b/src/commands/projections.zig @@ -140,12 +140,7 @@ pub fn parseArgs(ctx: *framework.RunCtx, cmd_args: []const []const u8) !ParsedAr } else if (std.mem.eql(u8, a, "--real")) { real_mode = true; } else if (std.mem.eql(u8, a, "--export-chart")) { - if (i + 1 >= cmd_args.len) { - cli.stderrPrint(io, "Error: --export-chart requires a path argument.\n"); - return error.MissingFlagValue; - } - export_chart = cmd_args[i + 1]; - i += 1; + export_chart = try cli.requireFlagValue(io, cmd_args, &i, a); } else if (std.mem.eql(u8, a, "--as-of") or std.mem.eql(u8, a, "--vs")) { if (i + 1 >= cmd_args.len) { cli.stderrPrint(io, "Error: "); @@ -1642,6 +1637,18 @@ test "parseArgs: unknown flag errors" { try testing.expectError(error.UnexpectedArg, parseArgsForTest(today, &args)); } +test "parseArgs: --export-chart without a value is rejected" { + const today = Date.fromYmd(2026, 5, 9); + const args = [_][]const u8{"--export-chart"}; + try testing.expectError(error.MissingFlagValue, parseArgsForTest(today, &args)); +} + +test "parseArgs: --export-chart followed by a flag does not swallow the flag" { + const today = Date.fromYmd(2026, 5, 9); + const args = [_][]const u8{ "--export-chart", "--real" }; + try testing.expectError(error.MissingFlagValue, parseArgsForTest(today, &args)); +} + test "parseArgs: --overlay-actuals carries into bands" { const today = Date.fromYmd(2026, 5, 9); const args = [_][]const u8{ "--as-of", "2026-04-01", "--overlay-actuals" }; diff --git a/src/commands/quote.zig b/src/commands/quote.zig index 9b05143..27f5fe3 100644 --- a/src/commands/quote.zig +++ b/src/commands/quote.zig @@ -67,13 +67,12 @@ pub fn parseArgs(ctx: *framework.RunCtx, cmd_args: []const []const u8) !ParsedAr while (i < cmd_args.len) : (i += 1) { const a = cmd_args[i]; if (std.mem.eql(u8, a, "--export-chart")) { - if (i + 1 >= cmd_args.len) { - cli.stderrPrint(ctx.io, "Error: --export-chart requires a path argument.\n"); - return error.MissingFlagValue; - } - export_chart = cmd_args[i + 1]; - i += 1; - } else if (std.mem.startsWith(u8, a, "--")) { + export_chart = try cli.requireFlagValue(ctx.io, cmd_args, &i, a); + } else if (a.len > 0 and a[0] == '-') { + // Reject ANY leading-dash token we don't recognize, + // including single-dash ones like `-x`. Previously only + // `--`-prefixed flags were caught, so `-x` slipped through + // and became the symbol. cli.stderrPrint(ctx.io, "Error: 'quote': unexpected flag "); cli.stderrPrint(ctx.io, a); cli.stderrPrint(ctx.io, "\n"); @@ -323,6 +322,36 @@ test "parseArgs: extra args error" { try std.testing.expectError(error.UnexpectedArg, parseArgs(&ctx, &args)); } +test "parseArgs: --export-chart captures the path" { + var ctx: framework.RunCtx = undefined; + ctx.io = std.testing.io; + const args = [_][]const u8{ "AAPL", "--export-chart", "aapl.png" }; + const parsed = try parseArgs(&ctx, &args); + try std.testing.expectEqualStrings("AAPL", parsed.symbol); + try std.testing.expectEqualStrings("aapl.png", parsed.export_chart.?); +} + +test "parseArgs: single-dash unknown flag is rejected (not swallowed as symbol)" { + var ctx: framework.RunCtx = undefined; + ctx.io = std.testing.io; + const args = [_][]const u8{"-x"}; + try std.testing.expectError(error.UnexpectedArg, parseArgs(&ctx, &args)); +} + +test "parseArgs: --export-chart without a value is rejected" { + var ctx: framework.RunCtx = undefined; + ctx.io = std.testing.io; + const args = [_][]const u8{ "AAPL", "--export-chart" }; + try std.testing.expectError(error.MissingFlagValue, parseArgs(&ctx, &args)); +} + +test "parseArgs: --export-chart followed by a flag does not swallow the flag" { + var ctx: framework.RunCtx = undefined; + ctx.io = std.testing.io; + const args = [_][]const u8{ "AAPL", "--export-chart", "--bogus" }; + try std.testing.expectError(error.MissingFlagValue, parseArgs(&ctx, &args)); +} + test "display with candles only" { var buf: [8192]u8 = undefined; var w: std.Io.Writer = .fixed(&buf); diff --git a/src/commands/snapshot.zig b/src/commands/snapshot.zig index 3e849c3..911439d 100644 --- a/src/commands/snapshot.zig +++ b/src/commands/snapshot.zig @@ -108,7 +108,7 @@ pub const meta: framework.Meta = .{ \\ , .uppercase_first_arg = false, - .user_errors = error{ UnexpectedArg, BadMetadata, NoCommitBeforeDate, NoMetadata, PathMissingInRev, WriteFailed }, + .user_errors = error{ UnexpectedArg, BadMetadata, NoCommitBeforeDate, NoMetadata, PathMissingInRev, WriteFailed, MissingFlagValue }, }; pub fn parseArgs(ctx: *framework.RunCtx, cmd_args: []const []const u8) !ParsedArgs { @@ -121,12 +121,7 @@ pub fn parseArgs(ctx: *framework.RunCtx, cmd_args: []const []const u8) !ParsedAr } else if (std.mem.eql(u8, a, "--dry-run")) { parsed.dry_run = true; } else if (std.mem.eql(u8, a, "--out")) { - i += 1; - if (i >= cmd_args.len) { - cli.stderrPrint(ctx.io, "Error: --out requires a path argument\n"); - return error.UnexpectedArg; - } - parsed.out_override = cmd_args[i]; + parsed.out_override = try cli.requireFlagValue(ctx.io, cmd_args, &i, a); } else if (std.mem.eql(u8, a, "--as-of")) { i += 1; if (i >= cmd_args.len) { @@ -1010,7 +1005,15 @@ test "parseArgs: --out without value errors" { ctx.io = std.testing.io; ctx.now_s = 0; const args = [_][]const u8{"--out"}; - try std.testing.expectError(error.UnexpectedArg, parseArgs(&ctx, &args)); + try std.testing.expectError(error.MissingFlagValue, parseArgs(&ctx, &args)); +} + +test "parseArgs: --out followed by a flag does not swallow the flag" { + var ctx: framework.RunCtx = undefined; + ctx.io = std.testing.io; + ctx.now_s = 0; + const args = [_][]const u8{ "--out", "--force" }; + try std.testing.expectError(error.MissingFlagValue, parseArgs(&ctx, &args)); } test "parseArgs: --as-of with explicit date" { diff --git a/src/main.zig b/src/main.zig index 27d31b4..f6046e1 100644 --- a/src/main.zig +++ b/src/main.zig @@ -231,7 +231,14 @@ fn parseGlobals(allocator: std.mem.Allocator, args: []const []const u8) GlobalPa } if (std.mem.eql(u8, a, "-p") or std.mem.eql(u8, a, "--portfolio")) { if (i + 1 >= args.len) return error.MissingValue; - try patterns.append(allocator, args[i + 1]); + // Reject a flag-shaped value (leading '-' with more after + // it): almost certainly the user forgot the value and the + // next flag would otherwise be silently consumed as the + // pattern. The lone '-' is left alone (harmless; resolves + // to nothing). + const value = args[i + 1]; + if (value.len > 1 and value[0] == '-') return error.MissingValue; + try patterns.append(allocator, value); // Detect the unquoted-glob shape: we just consumed `-p VALUE`, // and the next args are more `.srf` files with no flag in // between. That's almost always the shell expanding `-p @@ -244,7 +251,9 @@ fn parseGlobals(allocator: std.mem.Allocator, args: []const []const u8) GlobalPa } if (std.mem.eql(u8, a, "-w") or std.mem.eql(u8, a, "--watchlist")) { if (i + 1 >= args.len) return error.MissingValue; - g.watchlist_path = args[i + 1]; + const value = args[i + 1]; + if (value.len > 1 and value[0] == '-') return error.MissingValue; + g.watchlist_path = value; i += 2; continue; } @@ -600,6 +609,27 @@ test "parseGlobals: flag missing value errors" { try std.testing.expectError(error.MissingValue, parseGlobals(allocator, &argv)); } +test "parseGlobals: -p with a flag-shaped value errors (next flag not swallowed)" { + const allocator = std.testing.allocator; + const argv = [_][]const u8{ "zfin", "-p", "--no-color", "portfolio" }; + try std.testing.expectError(error.MissingValue, parseGlobals(allocator, &argv)); +} + +test "parseGlobals: -w with a flag-shaped value errors" { + const allocator = std.testing.allocator; + const argv = [_][]const u8{ "zfin", "-w", "--no-color", "portfolio" }; + try std.testing.expectError(error.MissingValue, parseGlobals(allocator, &argv)); +} + +test "parseGlobals: -p accepts the lone '-' (not treated as a flag)" { + const allocator = std.testing.allocator; + const argv = [_][]const u8{ "zfin", "-p", "-", "portfolio" }; + const g = try parseGlobals(allocator, &argv); + defer allocator.free(g.portfolio_patterns); + try std.testing.expectEqual(@as(usize, 1), g.portfolio_patterns.len); + try std.testing.expectEqualStrings("-", g.portfolio_patterns[0]); +} + test "parseGlobals: --help stops scanning" { const allocator = std.testing.allocator; const argv = [_][]const u8{ "zfin", "--help" }; diff --git a/src/tui.zig b/src/tui.zig index e40b257..5a08893 100644 --- a/src/tui.zig +++ b/src/tui.zig @@ -2214,31 +2214,54 @@ fn printDefaultKeys(io: std.Io) !void { try out.flush(); } -/// Entry point for the interactive TUI. -/// `args` contains only command-local tokens (everything after `interactive`). -pub fn run( - io: std.Io, - allocator: std.mem.Allocator, - config: zfin.Config, - portfolio_patterns: []const []const u8, - global_watchlist_path: ?[]const u8, - args: []const []const u8, - today: zfin.Date, -) !void { - const watchlist_path: ?[]const u8 = global_watchlist_path; - var symbol: []const u8 = ""; - var symbol_upper_buf: [32]u8 = undefined; - var has_explicit_symbol = false; - var skip_watchlist = false; - var chart_config: chart.ChartConfig = .{}; +/// Result of parsing the `interactive` command's local args +/// (everything after `interactive`). Produced by `parseArgs`, +/// consumed by `run`. The symbol is uppercased into an inline +/// buffer so the struct owns its storage and stays copy-safe (no +/// borrow into the caller's `args`). +const ParsedArgs = struct { + /// Set when a print-and-exit flag (`--default-keys` / + /// `--default-theme`) was seen. Recorded on the FIRST + /// occurrence so `--default-keys --default-theme` keeps the + /// historical first-token-wins behavior. `run` honors this + /// only after the whole arg list has validated, so trailing + /// bogus flags are still rejected (`--default-keys --nope` + /// exits non-zero rather than silently printing). + print_and_exit: ?PrintAndExit = null, + /// Uppercased symbol storage; `symbol()` returns the live slice. + symbol_buf: [32]u8 = @splat(0), + symbol_len: usize = 0, + has_explicit_symbol: bool = false, + skip_watchlist: bool = false, + chart_config: chart.ChartConfig = .{}, + + const PrintAndExit = enum { keys, theme }; + + /// The uppercased symbol, or "" when none was supplied. + fn symbol(self: *const ParsedArgs) []const u8 { + return self.symbol_buf[0..self.symbol_len]; + } +}; + +/// Parse the `interactive` command's local args. Validates the +/// ENTIRE list before returning: unknown flags, missing flag +/// values, and flag-shaped values all yield `error.InvalidArgs` +/// even when they trail a print-and-exit flag. This is the fix for +/// `interactive --default-keys --bogus-flag` silently succeeding: +/// the print-and-exit flags no longer short-circuit the loop, so +/// the bogus flag reaches the catch-all reject branch. +/// +/// Error messages go to stderr (suppressed under test). Pure apart +/// from those best-effort writes, so it's unit-testable without a +/// terminal. +fn parseArgs(io: std.Io, args: []const []const u8) error{InvalidArgs}!ParsedArgs { + var result: ParsedArgs = .{}; var i: usize = 0; while (i < args.len) : (i += 1) { if (std.mem.eql(u8, args[i], "--default-keys")) { - try printDefaultKeys(io); - return; + if (result.print_and_exit == null) result.print_and_exit = .keys; } else if (std.mem.eql(u8, args[i], "--default-theme")) { - try theme.printDefaults(io); - return; + if (result.print_and_exit == null) result.print_and_exit = .theme; } else if (std.mem.eql(u8, args[i], "--symbol") or std.mem.eql(u8, args[i], "-s")) { // -s / --symbol require a non-flag value. Bare `-s` and // `-s --chart …` are both user errors — surface them @@ -2260,11 +2283,11 @@ pub fn run( stderr.print(io, "\n"); return error.InvalidArgs; } - const len = @min(value.len, symbol_upper_buf.len); - _ = std.ascii.upperString(symbol_upper_buf[0..len], value[0..len]); - symbol = symbol_upper_buf[0..len]; - has_explicit_symbol = true; - skip_watchlist = true; + const len = @min(value.len, result.symbol_buf.len); + _ = std.ascii.upperString(result.symbol_buf[0..len], value[0..len]); + result.symbol_len = len; + result.has_explicit_symbol = true; + result.skip_watchlist = true; } else if (std.mem.eql(u8, args[i], "--chart")) { // Same shape as -s / --symbol: require a value, reject // flag-shaped values. @@ -2281,7 +2304,7 @@ pub fn run( return error.InvalidArgs; } if (chart.ChartConfig.parse(value)) |cc| { - chart_config = cc; + result.chart_config = cc; } else { stderr.print(io, "Error: --chart value is not a valid WIDTHxHEIGHT spec: "); stderr.print(io, value); @@ -2297,12 +2320,43 @@ pub fn run( stderr.print(io, "\nRun 'zfin interactive --help' for usage.\n"); return error.InvalidArgs; } else if (args[i].len > 0) { - const len = @min(args[i].len, symbol_upper_buf.len); - _ = std.ascii.upperString(symbol_upper_buf[0..len], args[i][0..len]); - symbol = symbol_upper_buf[0..len]; - has_explicit_symbol = true; + const len = @min(args[i].len, result.symbol_buf.len); + _ = std.ascii.upperString(result.symbol_buf[0..len], args[i][0..len]); + result.symbol_len = len; + result.has_explicit_symbol = true; } } + return result; +} + +/// Entry point for the interactive TUI. +/// `args` contains only command-local tokens (everything after `interactive`). +pub fn run( + io: std.Io, + allocator: std.mem.Allocator, + config: zfin.Config, + portfolio_patterns: []const []const u8, + global_watchlist_path: ?[]const u8, + args: []const []const u8, + today: zfin.Date, +) !void { + const watchlist_path: ?[]const u8 = global_watchlist_path; + + // Parse + validate the full arg list first. Print-and-exit + // flags (`--default-keys` / `--default-theme`) are honored only + // after validation, so trailing bogus flags still exit non-zero. + const parsed = try parseArgs(io, args); + if (parsed.print_and_exit) |which| { + switch (which) { + .keys => try printDefaultKeys(io), + .theme => try theme.printDefaults(io), + } + return; + } + const symbol = parsed.symbol(); + const has_explicit_symbol = parsed.has_explicit_symbol; + const skip_watchlist = parsed.skip_watchlist; + const chart_config = parsed.chart_config; var keymap = blk: { const home_opt = if (config.environ_map) |em| em.get("HOME") else null; @@ -2668,6 +2722,109 @@ test "formatStatusHint: single fragment has no separator" { try testing.expectEqualStrings("ctrl+s save", out); } +// ── interactive parseArgs ───────────────────────────────────── + +test "parseArgs: no args yields defaults" { + const p = try parseArgs(std.testing.io, &.{}); + try testing.expect(p.print_and_exit == null); + try testing.expectEqualStrings("", p.symbol()); + try testing.expect(!p.has_explicit_symbol); + try testing.expect(!p.skip_watchlist); + try testing.expectEqual(chart.ChartConfig{}, p.chart_config); +} + +test "parseArgs: positional symbol is uppercased and marked explicit" { + const p = try parseArgs(std.testing.io, &.{"vti"}); + try testing.expectEqualStrings("VTI", p.symbol()); + try testing.expect(p.has_explicit_symbol); + // A bare positional does NOT skip the watchlist (only -s/--symbol does). + try testing.expect(!p.skip_watchlist); +} + +test "parseArgs: --symbol sets symbol and skips watchlist" { + const p = try parseArgs(std.testing.io, &.{ "--symbol", "aapl" }); + try testing.expectEqualStrings("AAPL", p.symbol()); + try testing.expect(p.has_explicit_symbol); + try testing.expect(p.skip_watchlist); +} + +test "parseArgs: -s short form behaves like --symbol" { + const p = try parseArgs(std.testing.io, &.{ "-s", "msft" }); + try testing.expectEqualStrings("MSFT", p.symbol()); + try testing.expect(p.skip_watchlist); +} + +test "parseArgs: --default-keys requests the keys template" { + const p = try parseArgs(std.testing.io, &.{"--default-keys"}); + try testing.expectEqual(ParsedArgs.PrintAndExit.keys, p.print_and_exit.?); +} + +test "parseArgs: --default-theme requests the theme template" { + const p = try parseArgs(std.testing.io, &.{"--default-theme"}); + try testing.expectEqual(ParsedArgs.PrintAndExit.theme, p.print_and_exit.?); +} + +test "parseArgs: --default-keys with a trailing bogus flag is rejected" { + // Regression: print-and-exit flags used to short-circuit before + // the rest of the args were validated, so `--default-keys + // --bogus-flag` silently printed and exited 0. + try testing.expectError(error.InvalidArgs, parseArgs(std.testing.io, &.{ "--default-keys", "--bogus-flag" })); +} + +test "parseArgs: --default-theme with a trailing bogus flag is rejected" { + try testing.expectError(error.InvalidArgs, parseArgs(std.testing.io, &.{ "--default-theme", "--bogus-flag" })); +} + +test "parseArgs: --default-keys with a valid trailing positional still requests keys (no error)" { + // A trailing *positional* is harmless: print-and-exit takes + // precedence and the positional is ignored. Only bogus flags error. + const p = try parseArgs(std.testing.io, &.{ "--default-keys", "VTI" }); + try testing.expectEqual(ParsedArgs.PrintAndExit.keys, p.print_and_exit.?); +} + +test "parseArgs: first print-and-exit flag wins" { + const p = try parseArgs(std.testing.io, &.{ "--default-theme", "--default-keys" }); + try testing.expectEqual(ParsedArgs.PrintAndExit.theme, p.print_and_exit.?); +} + +test "parseArgs: unknown flag is rejected" { + try testing.expectError(error.InvalidArgs, parseArgs(std.testing.io, &.{"--nope"})); +} + +test "parseArgs: --symbol without a value is rejected" { + try testing.expectError(error.InvalidArgs, parseArgs(std.testing.io, &.{"--symbol"})); +} + +test "parseArgs: --symbol followed by a flag is rejected" { + try testing.expectError(error.InvalidArgs, parseArgs(std.testing.io, &.{ "-s", "--chart" })); +} + +test "parseArgs: --chart parses a valid WxH spec" { + const p = try parseArgs(std.testing.io, &.{ "--chart", "800x600" }); + try testing.expectEqual(chart.ChartMode.kitty, p.chart_config.mode); + try testing.expectEqual(@as(u32, 800), p.chart_config.max_width); + try testing.expectEqual(@as(u32, 600), p.chart_config.max_height); +} + +test "parseArgs: --chart without a value is rejected" { + try testing.expectError(error.InvalidArgs, parseArgs(std.testing.io, &.{"--chart"})); +} + +test "parseArgs: --chart with an invalid spec is rejected" { + try testing.expectError(error.InvalidArgs, parseArgs(std.testing.io, &.{ "--chart", "nonsense" })); +} + +test "parseArgs: --chart with a flag-shaped value is rejected" { + try testing.expectError(error.InvalidArgs, parseArgs(std.testing.io, &.{ "--chart", "-x" })); +} + +test "parseArgs: a symbol longer than the buffer is truncated, not overflowed" { + const long = "ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789"; // 36 chars > 32 + const p = try parseArgs(std.testing.io, &.{long}); + try testing.expectEqual(@as(usize, 32), p.symbol().len); + try testing.expectEqualStrings(long[0..32], p.symbol()); +} + test "writeDefaultKeys: includes preamble, global section, and per-tab sections" { var aw: std.Io.Writer.Allocating = .init(testing.allocator); defer aw.deinit();