From ec2e870c6ab16241aa32f17fdff777181197fb7b Mon Sep 17 00:00:00 2001 From: hermes Date: Sat, 17 Oct 2026 04:43:09 +0000 Subject: [PATCH] fix(cli): output contract, terminal-safe text, char-safe truncation, cache symlinks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From the full-repo Codex audit: - Provider and KSEI text reached the terminal raw, so an escape sequence in a news title or insight could clear or rewrite the screen. Every table cell, insight line, and graph label now has control characters and bidirectional overrides replaced. - Table rendering sliced strings by byte (a description at 200, a URL at 72, an earnings period), panicking when a multi-byte character such as "’" or "é" fell on the cut. Truncate by character. - In JSON mode, argument errors were clap's plain text and warnings were plain lines ahead of the JSON error. Argument errors are now the usual JSON error (INVALIDINPUT, exit 2) when JSON output is selected, and diagnostics are JSON lines; --help and --version are unchanged. - `config` commands ignored the output format; they follow it like every other command (JSON when piped or with -o json, text on a terminal). - Ownership argument mistakes (--limit 0, --format, missing or conflicting import sources, --family, an empty graph root) returned PARSEERROR / exit 1; they are INVALIDINPUT / exit 2 as documented. - `cache clear` followed symlinked directories and deleted files outside the cache; it now removes links without following them. Output change, intentional: goldens regenerated for the JSON argument error and the five config commands, which run with stdout piped. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01V3syn2fN2LNo5pvTSqc6eh --- crates/idx-cli/src/cli/config.rs | 30 +- crates/idx-cli/src/cli/ownership.rs | 126 ++++----- crates/idx-cli/src/main.rs | 59 +++- crates/idx-cli/src/output/mod.rs | 1 + crates/idx-cli/src/output/table.rs | 257 +++++++++--------- crates/idx-cli/src/output/text.rs | 70 +++++ crates/idx-cli/tests/golden/config-get.golden | 2 +- .../idx-cli/tests/golden/config-init.golden | 2 +- .../idx-cli/tests/golden/config-path.golden | 2 +- .../golden/config-set-output-jsonl.golden | 2 +- crates/idx-cli/tests/golden/config-set.golden | 2 +- .../error-usage-missing-symbol.json.golden | 7 +- crates/idx-core/src/cache.rs | 41 ++- crates/idx-core/src/investor.rs | 2 +- crates/idx-core/src/lib.rs | 27 +- crates/idx-core/src/ownership/graph.rs | 2 +- docs/ARCHITECTURE.md | 9 +- 17 files changed, 410 insertions(+), 231 deletions(-) create mode 100644 crates/idx-cli/src/output/text.rs diff --git a/crates/idx-cli/src/cli/config.rs b/crates/idx-cli/src/cli/config.rs index cd77da2..e4d7dac 100644 --- a/crates/idx-cli/src/cli/config.rs +++ b/crates/idx-cli/src/cli/config.rs @@ -1,6 +1,7 @@ use clap::{Args, Subcommand}; use crate::config::{config_path, ensure_default_config, get_config_value, set_config_value}; +use crate::output::{OutputFormat, json}; use idx_core::error::IdxError; #[derive(Debug, Args)] @@ -22,24 +23,39 @@ pub enum ConfigSubcommand { Path, } -pub fn handle(cmd: &ConfigCmd) -> Result<(), IdxError> { +pub fn handle(cmd: &ConfigCmd, format: &OutputFormat) -> Result<(), IdxError> { + // Each result as plain text, or one JSON document with -o json. + let print = |text: String, document: serde_json::Value| { + if format.is_json() { + json::print_json(&document) + } else { + println!("{text}"); + Ok(()) + } + }; match &cmd.command { ConfigSubcommand::Init => { - let path = ensure_default_config()?; - println!("{}", path.display()); + let path = ensure_default_config()?.display().to_string(); + print(path.clone(), serde_json::json!({ "path": path })) } ConfigSubcommand::Get { key } => { let value = get_config_value(key)? .ok_or_else(|| IdxError::ConfigError(format!("key not found: {key}")))?; - println!("{value}"); + print( + value.to_string(), + serde_json::json!({ "key": key, "value": value.to_string() }), + ) } ConfigSubcommand::Set { key, value } => { set_config_value(key, value)?; - println!("ok"); + print( + "ok".to_string(), + serde_json::json!({ "key": key, "value": value, "ok": true }), + ) } ConfigSubcommand::Path => { - println!("{}", config_path()?.display()); + let path = config_path()?.display().to_string(); + print(path.clone(), serde_json::json!({ "path": path })) } } - Ok(()) } diff --git a/crates/idx-cli/src/cli/ownership.rs b/crates/idx-cli/src/cli/ownership.rs index d9b4261..0d19885 100644 --- a/crates/idx-cli/src/cli/ownership.rs +++ b/crates/idx-cli/src/cli/ownership.rs @@ -1,13 +1,14 @@ use std::path::PathBuf; use clap::{Args, Subcommand}; -use comfy_table::{Cell, ContentArrangement, Table, presets::UTF8_FULL}; +use comfy_table::{ContentArrangement, Table, presets::UTF8_FULL}; use serde::Serialize; use crate::config::IdxConfig; use crate::output::json; use crate::output::style::Paint; use crate::output::table::format_idr; +use crate::output::text::{cell, terminal_safe}; use idx_core::error::IdxError; use idx_core::investor::{ ConcentrationRank, HolderSource, ImportSource, Investor, parse_report_family, @@ -262,7 +263,7 @@ fn handle_discover( config: &IdxConfig, ) -> Result<(), IdxError> { if args.limit == 0 { - return Err(IdxError::ParseError( + return Err(IdxError::InvalidInput( "--limit must be greater than 0".to_string(), )); } @@ -284,17 +285,17 @@ fn handle_discover( for report in reports { table.add_row(vec![ - Cell::new(report.publish_date.split('T').next().unwrap_or("-")), - Cell::new(report.family.label()), - Cell::new(report.status.label()), - Cell::new(if report.is_attachment { + cell(report.publish_date.split('T').next().unwrap_or("-")), + cell(report.family.label()), + cell(report.status.label()), + cell(if report.is_attachment { "attachment" } else { "main" }), - Cell::new(report.original_filename.unwrap_or_else(|| "-".to_string())), - Cell::new(report.title), - Cell::new(report.pdf_url), + cell(report.original_filename.unwrap_or_else(|| "-".to_string())), + cell(report.title), + cell(report.pdf_url), ]); } @@ -334,21 +335,21 @@ fn handle_ticker( for h in &data.holders { table.add_row(vec![ - Cell::new(h.rank), - Cell::new(match h.source { + cell(h.rank), + cell(match h.source { OwnershipSource::Ksei => "KSEI", OwnershipSource::Bing => "BING", }), - Cell::new(&h.name), - Cell::new(h.investor_type.clone().unwrap_or_else(|| "-".to_string())), - Cell::new(match h.locality { + cell(&h.name), + cell(h.investor_type.clone().unwrap_or_else(|| "-".to_string())), + cell(match h.locality { Some(idx_core::ownership::types::Locality::Local) => "L", Some(idx_core::ownership::types::Locality::Foreign) => "F", None => "-", }), - Cell::new(format_idr(h.shares)), - Cell::new(format_bps(h.percentage_bps)), - Cell::new(format_signal(h.signal)), + cell(format_idr(h.shares)), + cell(format_bps(h.percentage_bps)), + cell(format_signal(h.signal)), ]); } println!("{table}"); @@ -390,14 +391,14 @@ fn handle_entity( for row in &data.holdings { table.add_row(vec![ - Cell::new(&row.ticker.code), - Cell::new(match row.source { + cell(&row.ticker.code), + cell(match row.source { OwnershipSource::Ksei => "KSEI", OwnershipSource::Bing => "BING", }), - Cell::new(format_idr(row.shares)), - Cell::new(format_bps(row.percentage_bps)), - Cell::new(row.report_date.format("%Y-%m-%d").to_string()), + cell(format_idr(row.shares)), + cell(format_bps(row.percentage_bps)), + cell(row.report_date.format("%Y-%m-%d").to_string()), ]); } println!("{table}"); @@ -430,14 +431,14 @@ fn handle_search( for entity in &entities { table.add_row(vec![ - Cell::new(&entity.canonical_name), - Cell::new( + cell(&entity.canonical_name), + cell( entity .entity_type .clone() .unwrap_or_else(|| "-".to_string()), ), - Cell::new(entity.country.clone().unwrap_or_else(|| "-".to_string())), + cell(entity.country.clone().unwrap_or_else(|| "-".to_string())), ]); } println!("{table}"); @@ -463,10 +464,10 @@ fn handle_cross_holders( for (i, row) in rows.iter().enumerate() { table.add_row(vec![ - Cell::new(i + 1), - Cell::new(&row.entity.canonical_name), - Cell::new(row.ticker_count), - Cell::new(format_bps(row.total_bps)), + cell(i + 1), + cell(&row.entity.canonical_name), + cell(row.ticker_count), + cell(format_bps(row.total_bps)), ]); } @@ -502,13 +503,13 @@ fn handle_concentration( for (idx, (ticker, m)) in rows.iter().enumerate() { table.add_row(vec![ - Cell::new(idx + 1), - Cell::new(ticker), - Cell::new(format_bps(m.top1_bps)), - Cell::new(format_bps(m.top3_bps)), - Cell::new(m.hhi), - Cell::new(format_bps(m.free_float_bps)), - Cell::new(m.holder_count), + cell(idx + 1), + cell(ticker), + cell(format_bps(m.top1_bps)), + cell(format_bps(m.top3_bps)), + cell(m.hhi), + cell(format_bps(m.free_float_bps)), + cell(m.holder_count), ]); } @@ -556,11 +557,11 @@ fn print_flow_section(title: &str, rows: &[HolderRow]) { for (i, row) in rows.iter().enumerate() { table.add_row(vec![ - Cell::new(i + 1), - Cell::new(&row.name), - Cell::new(format_idr(row.shares)), - Cell::new(format_bps(row.percentage_bps)), - Cell::new(format_signal(row.signal)), + cell(i + 1), + cell(&row.name), + cell(format_idr(row.shares)), + cell(format_bps(row.percentage_bps)), + cell(format_signal(row.signal)), ]); } println!("{table}"); @@ -583,14 +584,17 @@ fn handle_graph(investor: &Investor, args: &GraphArgs, config: &IdxConfig) -> Re match args.format.trim().to_lowercase().as_str() { "table" => { - print!("{}", graph::format_graph_text(&nodes, &edges)); + print!( + "{}", + terminal_safe(&graph::format_graph_text(&nodes, &edges)) + ); Ok(()) } "dot" => { print!("{}", graph::format_graph_dot(&nodes, &edges)); Ok(()) } - other => Err(IdxError::ParseError(format!( + other => Err(IdxError::InvalidInput(format!( "invalid --format '{other}', expected table|dot" ))), } @@ -624,20 +628,20 @@ fn handle_changes( for row in changes { table.add_row(vec![ - Cell::new(row.ticker_code), - Cell::new(row.entity_name), - Cell::new(format_change_type(row.change_type)), - Cell::new( + cell(row.ticker_code), + cell(row.entity_name), + cell(format_change_type(row.change_type)), + cell( row.old_bps .map(format_bps) .unwrap_or_else(|| "-".to_string()), ), - Cell::new( + cell( row.new_bps .map(format_bps) .unwrap_or_else(|| "-".to_string()), ), - Cell::new(format_signed_bps(row.delta_bps)), + cell(format_signed_bps(row.delta_bps)), ]); } @@ -677,11 +681,11 @@ fn handle_resolve( for row in rows { table.add_row(vec![ - Cell::new(row.raw_name), - Cell::new(row.source), - Cell::new(row.ticker_code), - Cell::new(row.current_entity.unwrap_or_else(|| "-".to_string())), - Cell::new( + cell(row.raw_name), + cell(row.source), + cell(row.ticker_code), + cell(row.current_entity.unwrap_or_else(|| "-".to_string())), + cell( row.confidence .map(|v| format!("{v:.2}")) .unwrap_or_else(|| "-".to_string()), @@ -734,13 +738,13 @@ fn handle_import( config: &IdxConfig, ) -> Result<(), IdxError> { if args.file.is_none() && args.url.is_none() && args.fetch_bing.is_none() { - return Err(IdxError::ParseError( + return Err(IdxError::InvalidInput( "provide one of: --file, --url, or --fetch-bing".to_string(), )); } if args.file.is_some() && args.url.is_some() { - return Err(IdxError::ParseError( + return Err(IdxError::InvalidInput( "--file and --url are mutually exclusive".to_string(), )); } @@ -802,11 +806,11 @@ fn handle_releases(investor: &Investor, config: &IdxConfig) -> Result<(), IdxErr for r in releases { table.add_row(vec![ - Cell::new(r.as_of_date.format("%Y-%m-%d").to_string()), - Cell::new(r.row_count), - Cell::new(r.sha256), - Cell::new(r.source_url.unwrap_or_else(|| "-".to_string())), - Cell::new(r.imported_at), + cell(r.as_of_date.format("%Y-%m-%d").to_string()), + cell(r.row_count), + cell(r.sha256), + cell(r.source_url.unwrap_or_else(|| "-".to_string())), + cell(r.imported_at), ]); } diff --git a/crates/idx-cli/src/main.rs b/crates/idx-cli/src/main.rs index 91f75a1..c984112 100644 --- a/crates/idx-cli/src/main.rs +++ b/crates/idx-cli/src/main.rs @@ -20,7 +20,10 @@ fn main() { } fn run() -> Result<(), IdxError> { - let cli = Cli::parse(); + let cli = match Cli::try_parse() { + Ok(cli) => cli, + Err(err) => return Err(argument_error(err)), + }; runtime::set_quiet(cli.quiet); let config = match IdxConfig::load_with_cli(&cli) { Ok(config) => config, @@ -30,6 +33,7 @@ fn run() -> Result<(), IdxError> { } }; output::json::configure(config.output, config::stdout_is_terminal()); + runtime::set_json(config.output.is_json()); output::style::set_color(!config.no_color); match &cli.command { @@ -58,7 +62,7 @@ fn run() -> Result<(), IdxError> { } } Commands::Config(cfg) => { - if let Err(err) = cli::config::handle(cfg) { + if let Err(err) = cli::config::handle(cfg, &config.output) { emit_error(&err, &config.output); return Err(err); } @@ -82,6 +86,57 @@ fn run() -> Result<(), IdxError> { } /// Output format for errors raised before the configuration has loaded. +/// Argument errors honor the output contract: in JSON mode they are one JSON +/// error on stderr like any other failure (clap prints plain text). Help and +/// `--version` print as usual. +fn argument_error(err: clap::Error) -> IdxError { + use clap::error::ErrorKind; + if matches!( + err.kind(), + ErrorKind::DisplayHelp + | ErrorKind::DisplayVersion + | ErrorKind::DisplayHelpOnMissingArgumentOrSubcommand + ) || !requested_output_from_args().is_json() + { + err.exit(); + } + let rendered = err.render().to_string(); + let message = rendered + .trim() + .strip_prefix("error: ") + .unwrap_or(rendered.trim()) + .to_string(); + let error = IdxError::InvalidInput(message); + emit_error(&error, &requested_output_from_args()); + error +} + +/// The output format the raw arguments ask for, when they did not parse: +/// `-o`/`--output` (any spelling), then `IDX_OUTPUT`, then auto. +fn requested_output_from_args() -> OutputFormat { + let args: Vec = std::env::args().skip(1).collect(); + let flag = args.iter().enumerate().find_map(|(i, arg)| { + if arg == "-o" || arg == "--output" { + args.get(i + 1).cloned() + } else if let Some(value) = arg.strip_prefix("--output=") { + Some(value.to_string()) + } else if arg.starts_with("-o") && arg.len() > 2 { + Some(arg[2..].trim_start_matches('=').to_string()) + } else { + None + } + }); + let setting = flag + .and_then(|value| OutputSetting::parse(&value)) + .or_else(|| { + std::env::var("IDX_OUTPUT") + .ok() + .and_then(|value| OutputSetting::parse(&value)) + }) + .unwrap_or_default(); + setting.resolve(config::stdout_is_terminal()) +} + fn bootstrap_output_format(cli: &Cli) -> OutputFormat { let setting = cli .output diff --git a/crates/idx-cli/src/output/mod.rs b/crates/idx-cli/src/output/mod.rs index 2f1b8e4..055d08f 100644 --- a/crates/idx-cli/src/output/mod.rs +++ b/crates/idx-cli/src/output/mod.rs @@ -1,6 +1,7 @@ pub mod json; pub mod style; pub mod table; +pub mod text; use std::io::IsTerminal; diff --git a/crates/idx-cli/src/output/table.rs b/crates/idx-cli/src/output/table.rs index fb5df0c..961a7a9 100644 --- a/crates/idx-cli/src/output/table.rs +++ b/crates/idx-cli/src/output/table.rs @@ -1,4 +1,5 @@ use crate::output::style::Paint; +use crate::output::text::{cell, terminal_safe, truncate_chars}; use comfy_table::{Cell, Color, ContentArrangement, Table, presets::UTF8_FULL}; use idx_core::analysis::fundamental::{ @@ -56,27 +57,27 @@ pub fn print_quotes(quotes: &[Quote], no_color: bool) -> Result<(), IdxError> { for q in quotes { let pct_cell = match q.change_pct { - None => Cell::new("-"), - Some(pct) if no_color => Cell::new(format!("{pct:+.2}%")), - Some(pct) if pct >= 0.0 => Cell::new(format!("{pct:+.2}%")).fg(Color::Green), - Some(pct) => Cell::new(format!("{pct:+.2}%")).fg(Color::Red), + None => cell("-"), + Some(pct) if no_color => cell(format!("{pct:+.2}%")), + Some(pct) if pct >= 0.0 => cell(format!("{pct:+.2}%")).fg(Color::Green), + Some(pct) => cell(format!("{pct:+.2}%")).fg(Color::Red), }; table.add_row(vec![ - Cell::new(&q.symbol), - Cell::new(format_idr(q.price)), - Cell::new( + cell(&q.symbol), + cell(format_idr(q.price)), + cell( q.change .map(|c| format!("{c:+}")) .unwrap_or_else(|| "-".to_string()), ), pct_cell, - Cell::new(q.volume.map(format_u64).unwrap_or_else(|| "-".to_string())), - Cell::new( + cell(q.volume.map(format_u64).unwrap_or_else(|| "-".to_string())), + cell( q.market_cap .map(format_u64) .unwrap_or_else(|| "-".to_string()), ), - Cell::new(format_52w_range_bar(q.week52_position)), + cell(format_52w_range_bar(q.week52_position)), ]); } @@ -114,12 +115,12 @@ pub fn print_history(symbol: &str, history: &[Ohlc]) -> Result<(), IdxError> { .set_header(vec!["DATE", "OPEN", "HIGH", "LOW", "CLOSE", "VOLUME"]); for item in history { table.add_row(vec![ - Cell::new(item.date), - Cell::new(format_idr(item.open)), - Cell::new(format_idr(item.high)), - Cell::new(format_idr(item.low)), - Cell::new(format_idr(item.close)), - Cell::new(format_u64(item.volume)), + cell(item.date), + cell(format_idr(item.open)), + cell(format_idr(item.high)), + cell(format_idr(item.low)), + cell(format_idr(item.close)), + cell(format_u64(item.volume)), ]); } println!("{table}"); @@ -143,54 +144,54 @@ pub fn print_technical(report: &TechnicalReport, no_color: bool) -> Result<(), I .set_header(vec!["METRIC", "VALUE", "SIGNAL"]); table.add_row(vec![ - Cell::new("Current Price"), - Cell::new(format_idr(report.current_price)), - Cell::new("-"), + cell("Current Price"), + cell(format_idr(report.current_price)), + cell("-"), ]); table.add_row(vec![ - Cell::new("SMA 20"), - Cell::new(format_idr_option(report.sma20)), - Cell::new("-"), + cell("SMA 20"), + cell(format_idr_option(report.sma20)), + cell("-"), ]); table.add_row(vec![ - Cell::new("SMA 50"), - Cell::new(format_idr_option(report.sma50)), - Cell::new("-"), + cell("SMA 50"), + cell(format_idr_option(report.sma50)), + cell("-"), ]); table.add_row(vec![ - Cell::new("SMA 200"), - Cell::new(format_idr_option(report.sma200)), - Cell::new("-"), + cell("SMA 200"), + cell(format_idr_option(report.sma200)), + cell("-"), ]); table.add_row(vec![ - Cell::new("RSI (14)"), - Cell::new(format_float(report.rsi14, 2)), - Cell::new(format_signal(report.signals.rsi, no_color, false)), + cell("RSI (14)"), + cell(format_float(report.rsi14, 2)), + cell(format_signal(report.signals.rsi, no_color, false)), ]); table.add_row(vec![ - Cell::new("MACD (12,26,9)"), - Cell::new(format!( + cell("MACD (12,26,9)"), + cell(format!( "{}/{}/{}", format_float(report.macd.line, 2), format_float(report.macd.signal, 2), format_float(report.macd.histogram, 2) )), - Cell::new(format_signal(report.signals.macd, no_color, false)), + cell(format_signal(report.signals.macd, no_color, false)), ]); table.add_row(vec![ - Cell::new("Trend"), - Cell::new(trend_context(report)), - Cell::new(format_signal(report.signals.trend, no_color, false)), + cell("Trend"), + cell(trend_context(report)), + cell(format_signal(report.signals.trend, no_color, false)), ]); table.add_row(vec![ - Cell::new("Volume Ratio (20)"), - Cell::new(format_volume_ratio(report)), - Cell::new("-"), + cell("Volume Ratio (20)"), + cell(format_volume_ratio(report)), + cell("-"), ]); table.add_row(vec![ - Cell::new("Overall Signal"), - Cell::new("-"), - Cell::new(format_signal(report.signals.overall, no_color, true)), + cell("Overall Signal"), + cell("-"), + cell(format_signal(report.signals.overall, no_color, true)), ]); println!("{table}"); @@ -207,19 +208,19 @@ pub fn print_growth(symbol: &str, report: &GrowthReport, no_color: bool) -> Resu .set_header(vec!["METRIC", "VALUE", "SIGNAL"]); table.add_row(vec![ - Cell::new("Revenue Growth"), - Cell::new(format_pct(report.revenue_growth_pct)), - Cell::new(format_growth_signal(&report.revenue_signal, no_color)), + cell("Revenue Growth"), + cell(format_pct(report.revenue_growth_pct)), + cell(format_growth_signal(&report.revenue_signal, no_color)), ]); table.add_row(vec![ - Cell::new("Earnings Growth"), - Cell::new(format_pct(report.earnings_growth_pct)), - Cell::new(format_growth_signal(&report.earnings_signal, no_color)), + cell("Earnings Growth"), + cell(format_pct(report.earnings_growth_pct)), + cell(format_growth_signal(&report.earnings_signal, no_color)), ]); table.add_row(vec![ - Cell::new("Overall"), - Cell::new("-"), - Cell::new(format_growth_signal(&report.overall_signal, no_color)), + cell("Overall"), + cell("-"), + cell(format_growth_signal(&report.overall_signal, no_color)), ]); println!("{table}"); @@ -240,39 +241,39 @@ pub fn print_valuation( .set_header(vec!["METRIC", "VALUE", "SIGNAL"]); table.add_row(vec![ - Cell::new("P/E (Trailing)"), - Cell::new(format_opt_f64(report.pe_trailing, 2)), - Cell::new(format_valuation_signal(&report.pe_signal, no_color)), + cell("P/E (Trailing)"), + cell(format_opt_f64(report.pe_trailing, 2)), + cell(format_valuation_signal(&report.pe_signal, no_color)), ]); table.add_row(vec![ - Cell::new("P/E (Forward)"), - Cell::new(format_opt_f64(report.pe_forward, 2)), - Cell::new("-"), + cell("P/E (Forward)"), + cell(format_opt_f64(report.pe_forward, 2)), + cell("-"), ]); table.add_row(vec![ - Cell::new("Price/Book"), - Cell::new(format_opt_f64(report.pb, 2)), - Cell::new(format_valuation_signal(&report.pb_signal, no_color)), + cell("Price/Book"), + cell(format_opt_f64(report.pb, 2)), + cell(format_valuation_signal(&report.pb_signal, no_color)), ]); table.add_row(vec![ - Cell::new("ROE"), - Cell::new(format_pct(report.roe_pct)), - Cell::new(format_valuation_signal(&report.roe_signal, no_color)), + cell("ROE"), + cell(format_pct(report.roe_pct)), + cell(format_valuation_signal(&report.roe_signal, no_color)), ]); table.add_row(vec![ - Cell::new("Net Margin"), - Cell::new(format_pct(report.net_margin_pct)), - Cell::new(format_valuation_signal(&report.margin_signal, no_color)), + cell("Net Margin"), + cell(format_pct(report.net_margin_pct)), + cell(format_valuation_signal(&report.margin_signal, no_color)), ]); table.add_row(vec![ - Cell::new("EV/EBITDA"), - Cell::new(format_opt_f64(report.ev_ebitda, 2)), - Cell::new(format_valuation_signal(&report.ev_ebitda_signal, no_color)), + cell("EV/EBITDA"), + cell(format_opt_f64(report.ev_ebitda, 2)), + cell(format_valuation_signal(&report.ev_ebitda_signal, no_color)), ]); table.add_row(vec![ - Cell::new("Overall"), - Cell::new("-"), - Cell::new(format_valuation_signal(&report.overall_signal, no_color)), + cell("Overall"), + cell("-"), + cell(format_valuation_signal(&report.overall_signal, no_color)), ]); println!("{table}"); @@ -289,24 +290,24 @@ pub fn print_risk(symbol: &str, report: &RiskReport, no_color: bool) -> Result<( .set_header(vec!["METRIC", "VALUE", "SIGNAL"]); table.add_row(vec![ - Cell::new("Debt/Equity"), - Cell::new(format_opt_f64(report.debt_to_equity, 2)), - Cell::new(format_risk_signal(&report.de_signal, no_color)), + cell("Debt/Equity"), + cell(format_opt_f64(report.debt_to_equity, 2)), + cell(format_risk_signal(&report.de_signal, no_color)), ]); table.add_row(vec![ - Cell::new("Current Ratio"), - Cell::new(format_opt_f64(report.current_ratio, 2)), - Cell::new(format_risk_signal(&report.current_ratio_signal, no_color)), + cell("Current Ratio"), + cell(format_opt_f64(report.current_ratio, 2)), + cell(format_risk_signal(&report.current_ratio_signal, no_color)), ]); table.add_row(vec![ - Cell::new("ROA"), - Cell::new(format_pct(report.roa_pct)), - Cell::new("-"), + cell("ROA"), + cell(format_pct(report.roa_pct)), + cell("-"), ]); table.add_row(vec![ - Cell::new("Overall"), - Cell::new("-"), - Cell::new(format_risk_signal(&report.overall_signal, no_color)), + cell("Overall"), + cell("-"), + cell(format_risk_signal(&report.overall_signal, no_color)), ]); println!("{table}"); @@ -341,8 +342,8 @@ pub fn print_compare(reports: &[FundamentalReport], no_color: bool) -> Result<() .load_preset(UTF8_FULL) .set_content_arrangement(ContentArrangement::Dynamic); - let mut header = vec![Cell::new("METRIC")]; - header.extend(reports.iter().map(|report| Cell::new(&report.symbol))); + let mut header = vec![cell("METRIC")]; + header.extend(reports.iter().map(|report| cell(&report.symbol))); table.set_header(header); add_compare_row( @@ -519,10 +520,11 @@ fn format_earnings_period(period: &str) -> String { return format!("FY{trimmed}"); } + // ASCII only, so the byte slices below fall on character boundaries. if trimmed.len() == 6 + && trimmed.is_ascii() && trimmed.starts_with('Q') - && trimmed[1..2].chars().all(|ch| ch.is_ascii_digit()) - && trimmed[2..].chars().all(|ch| ch.is_ascii_digit()) + && trimmed[1..].bytes().all(|byte| byte.is_ascii_digit()) { return format!("{} {}", &trimmed[..2], &trimmed[2..]); } @@ -644,7 +646,7 @@ fn format_text_signal( } fn add_compare_row(table: &mut Table, label: &str, values: Vec) { - let mut row = vec![Cell::new(label)]; + let mut row = vec![cell(label)]; row.extend(values.into_iter().map(Cell::new)); table.add_row(row); } @@ -664,7 +666,7 @@ pub fn print_profile(profile: &CompanyProfile) -> Result<(), IdxError> { let add_if_present = |t: &mut Table, label: &str, value: &str| { if !value.is_empty() { - t.add_row(vec![Cell::new(label), Cell::new(value)]); + t.add_row(vec![cell(label), cell(value)]); } }; @@ -677,24 +679,17 @@ pub fn print_profile(profile: &CompanyProfile) -> Result<(), IdxError> { add_if_present(&mut table, "City", &profile.city); add_if_present(&mut table, "Phone", &profile.phone); if profile.employees > 0 { - table.add_row(vec![ - Cell::new("Employees"), - Cell::new(profile.employees.to_string()), - ]); + table.add_row(vec![cell("Employees"), cell(profile.employees.to_string())]); } if !profile.description.is_empty() { // Truncate long descriptions for table display - let desc = if profile.description.len() > 200 { - format!("{}...", &profile.description[..200]) - } else { - profile.description.clone() - }; - table.add_row(vec![Cell::new("Description"), Cell::new(desc)]); + let desc = truncate_chars(&profile.description, 200); + table.add_row(vec![cell("Description"), cell(desc)]); } if !profile.officers.is_empty() { table.add_row(vec![ - Cell::new("Executives"), - Cell::new( + cell("Executives"), + cell( profile .officers .iter() @@ -726,7 +721,7 @@ pub fn print_financials(fin: &FinancialStatements) -> Result<(), IdxError> { entries.sort_by(|(left, _), (right, _)| left.cmp(right)); for (label, value) in entries { - t.add_row(vec![Cell::new(label), Cell::new(format_idr(*value as i64))]); + t.add_row(vec![cell(label), cell(format_idr(*value as i64))]); } println!("{t}"); }; @@ -803,10 +798,10 @@ pub fn print_sentiment(data: &SentimentData) -> Result<(), IdxError> { .set_header(vec!["RANGE", "BULLISH", "BEARISH", "NEUTRAL"]); for row in &data.statistics { table.add_row(vec![ - Cell::new(&row.time_range), - Cell::new(row.bullish), - Cell::new(row.bearish), - Cell::new(row.neutral), + cell(&row.time_range), + cell(row.bullish), + cell(row.bearish), + cell(row.neutral), ]); } println!("{table}"); @@ -814,20 +809,20 @@ pub fn print_sentiment(data: &SentimentData) -> Result<(), IdxError> { } pub fn print_insights(data: &InsightData) -> Result<(), IdxError> { - println!("{}", data.summary); + println!("{}", terminal_safe(&data.summary)); if !data.last_updated.is_empty() { - println!("Last updated: {}", data.last_updated); + println!("Last updated: {}", terminal_safe(&data.last_updated)); } if !data.highlights.is_empty() { println!("Highlights:"); for h in &data.highlights { - println!("- {h}"); + println!("- {}", terminal_safe(h)); } } if !data.risks.is_empty() { println!("Risks:"); for r in &data.risks { - println!("- {r}"); + println!("- {}", terminal_safe(r)); } } Ok(()) @@ -840,11 +835,11 @@ pub fn print_news(items: &[NewsItem]) -> Result<(), IdxError> { .set_header(vec!["TITLE", "ABOUT", "PROVIDER", "DATE", "URL"]); for item in items { table.add_row(vec![ - Cell::new(&item.title), - Cell::new(item.relevance.as_str()), - Cell::new(&item.provider), - Cell::new(&item.published_at), - Cell::new(truncate_url(&item.url)), + cell(&item.title), + cell(item.relevance.as_str()), + cell(&item.provider), + cell(&item.published_at), + cell(truncate_url(&item.url)), ]); } println!("{table}"); @@ -853,13 +848,13 @@ pub fn print_news(items: &[NewsItem]) -> Result<(), IdxError> { fn add_earnings_history_row(table: &mut Table, row: &EarningsData) { table.add_row(vec![ - Cell::new(format_earnings_period(&row.period_type)), - Cell::new(format_float(row.eps_actual, 2)), - Cell::new(format_float(row.eps_forecast, 2)), - Cell::new(format_float(row.eps_surprise, 2)), - Cell::new(format_float(row.eps_surprise_pct, 2)), - Cell::new(format_idr_option_from_f64(row.revenue_actual)), - Cell::new(format_table_date_option( + cell(format_earnings_period(&row.period_type)), + cell(format_float(row.eps_actual, 2)), + cell(format_float(row.eps_forecast, 2)), + cell(format_float(row.eps_surprise, 2)), + cell(format_float(row.eps_surprise_pct, 2)), + cell(format_idr_option_from_f64(row.revenue_actual)), + cell(format_table_date_option( row.earning_release_date.as_deref(), )), ]); @@ -867,21 +862,17 @@ fn add_earnings_history_row(table: &mut Table, row: &EarningsData) { fn add_earnings_forecast_row(table: &mut Table, row: &EarningsData) { table.add_row(vec![ - Cell::new(format_earnings_period(&row.period_type)), - Cell::new(format_float(row.eps_forecast, 2)), - Cell::new(format_idr_option_from_f64(row.revenue_forecast)), - Cell::new(format_table_date_option( + cell(format_earnings_period(&row.period_type)), + cell(format_float(row.eps_forecast, 2)), + cell(format_idr_option_from_f64(row.revenue_forecast)), + cell(format_table_date_option( row.earning_release_date.as_deref(), )), ]); } fn truncate_url(url: &str) -> String { - if url.len() > 72 { - format!("{}...", &url[..72]) - } else { - url.to_string() - } + truncate_chars(url, 72) } #[cfg(test)] diff --git a/crates/idx-cli/src/output/text.rs b/crates/idx-cli/src/output/text.rs new file mode 100644 index 0000000..c3c92d1 --- /dev/null +++ b/crates/idx-cli/src/output/text.rs @@ -0,0 +1,70 @@ +//! Text from providers and KSEI files is untrusted: before it reaches a +//! terminal, control characters (which can start escape sequences that clear +//! the screen, rewrite output, or set the window title) and bidirectional +//! overrides (which can reorder what is displayed) are replaced. + +use comfy_table::Cell; + +/// `text` with control characters other than newline, and bidirectional +/// override characters, replaced by U+FFFD. +pub fn terminal_safe(text: &str) -> String { + text.chars() + .map(|ch| { + let bidi = matches!(ch, '\u{202A}'..='\u{202E}' | '\u{2066}'..='\u{2069}'); + if (ch.is_control() && ch != '\n') || bidi { + '\u{FFFD}' + } else { + ch + } + }) + .collect() +} + +/// A table cell whose content is made [`terminal_safe`]. +pub fn cell(value: impl ToString) -> Cell { + Cell::new(terminal_safe(&value.to_string())) +} + +/// At most `max` characters of `text` (never splitting one), with `...` +/// appended when it was cut. +pub fn truncate_chars(text: &str, max: usize) -> String { + match text.char_indices().nth(max) { + Some((end, _)) => format!("{}...", &text[..end]), + None => text.to_string(), + } +} + +#[cfg(test)] +mod tests { + use super::{terminal_safe, truncate_chars}; + + #[test] + fn escape_sequences_and_bidi_overrides_are_neutralized() { + assert_eq!( + terminal_safe("\u{1b}[2J\u{1b}[Hfake"), + "\u{FFFD}[2J\u{FFFD}[Hfake" + ); + assert_eq!( + terminal_safe("\u{1b}]0;title\u{7}x"), + "\u{FFFD}]0;title\u{FFFD}x" + ); + assert_eq!(terminal_safe("a\u{9b}31mb"), "a\u{FFFD}31mb"); + assert_eq!(terminal_safe("ab\u{202E}cd"), "ab\u{FFFD}cd"); + assert_eq!(terminal_safe("line one\nline two"), "line one\nline two"); + assert_eq!( + terminal_safe("Saham Grup Djarum’s 14,81%"), + "Saham Grup Djarum’s 14,81%" + ); + } + + #[test] + fn truncation_never_splits_a_character() { + let text = format!("{}é tail", "a".repeat(199)); + assert_eq!( + truncate_chars(&text, 200), + format!("{}é...", "a".repeat(199)) + ); + assert_eq!(truncate_chars("short", 200), "short"); + assert_eq!(truncate_chars("ééé", 2), "éé..."); + } +} diff --git a/crates/idx-cli/tests/golden/config-get.golden b/crates/idx-cli/tests/golden/config-get.golden index c9ca9de..a90ab07 100644 --- a/crates/idx-cli/tests/golden/config-get.golden +++ b/crates/idx-cli/tests/golden/config-get.golden @@ -1,4 +1,4 @@ exit: 0 --- stdout -msn +{"key":"general.provider","value":"msn"} --- stderr diff --git a/crates/idx-cli/tests/golden/config-init.golden b/crates/idx-cli/tests/golden/config-init.golden index ab029c5..28a0e2f 100644 --- a/crates/idx-cli/tests/golden/config-init.golden +++ b/crates/idx-cli/tests/golden/config-init.golden @@ -1,4 +1,4 @@ exit: 0 --- stdout -/config/idx/config.toml +{"path":"/config/idx/config.toml"} --- stderr diff --git a/crates/idx-cli/tests/golden/config-path.golden b/crates/idx-cli/tests/golden/config-path.golden index ab029c5..28a0e2f 100644 --- a/crates/idx-cli/tests/golden/config-path.golden +++ b/crates/idx-cli/tests/golden/config-path.golden @@ -1,4 +1,4 @@ exit: 0 --- stdout -/config/idx/config.toml +{"path":"/config/idx/config.toml"} --- stderr diff --git a/crates/idx-cli/tests/golden/config-set-output-jsonl.golden b/crates/idx-cli/tests/golden/config-set-output-jsonl.golden index aecf1eb..02653fc 100644 --- a/crates/idx-cli/tests/golden/config-set-output-jsonl.golden +++ b/crates/idx-cli/tests/golden/config-set-output-jsonl.golden @@ -1,4 +1,4 @@ exit: 0 --- stdout -ok +{"key":"general.output","ok":true,"value":"jsonl"} --- stderr diff --git a/crates/idx-cli/tests/golden/config-set.golden b/crates/idx-cli/tests/golden/config-set.golden index aecf1eb..0529834 100644 --- a/crates/idx-cli/tests/golden/config-set.golden +++ b/crates/idx-cli/tests/golden/config-set.golden @@ -1,4 +1,4 @@ exit: 0 --- stdout -ok +{"key":"general.provider","ok":true,"value":"yahoo"} --- stderr diff --git a/crates/idx-cli/tests/golden/error-usage-missing-symbol.json.golden b/crates/idx-cli/tests/golden/error-usage-missing-symbol.json.golden index e653302..60dd4a1 100644 --- a/crates/idx-cli/tests/golden/error-usage-missing-symbol.json.golden +++ b/crates/idx-cli/tests/golden/error-usage-missing-symbol.json.golden @@ -1,9 +1,4 @@ exit: 2 --- stdout --- stderr -error: the following required arguments were not provided: - ... - -Usage: idx stocks quote ... - -For more information, try '--help'. +{"code":"INVALIDINPUT","error":true,"message":"invalid input: the following required arguments were not provided:\n ...\n\nUsage: idx stocks quote ...\n\nFor more information, try '--help'."} diff --git a/crates/idx-core/src/cache.rs b/crates/idx-core/src/cache.rs index 5e1abb5..85669f8 100644 --- a/crates/idx-core/src/cache.rs +++ b/crates/idx-core/src/cache.rs @@ -185,13 +185,11 @@ impl Cache { } let mut removed = 0usize; let mut failed = Vec::new(); - Self::walk(&self.root, &mut |p| { - if p.is_file() { - match fs::remove_file(p) { - Ok(_) => removed += 1, - Err(_) => failed.push(p.to_path_buf()), - } - } + // `walk` yields files and symlinks, never entering a linked + // directory; remove_file on a symlink removes the link, not its target. + Self::walk(&self.root, &mut |p| match fs::remove_file(p) { + Ok(_) => removed += 1, + Err(_) => failed.push(p.to_path_buf()), })?; Ok((removed, failed)) } @@ -200,7 +198,10 @@ impl Cache { for entry in fs::read_dir(dir).map_err(|e| IdxError::Io(e.to_string()))? { let entry = entry.map_err(|e| IdxError::Io(e.to_string()))?; let path = entry.path(); - if path.is_dir() { + // file_type() does not follow symlinks, so a link to a directory + // outside the cache is never walked (or emptied by `clear`). + let file_type = entry.file_type().map_err(|e| IdxError::Io(e.to_string()))?; + if file_type.is_dir() { Self::walk(&path, f)?; } else { f(&path); @@ -297,6 +298,30 @@ pub fn cache_dir() -> Result { #[cfg(test)] mod tests { + + #[cfg(unix)] + #[test] + fn clear_removes_symlinks_without_touching_their_targets() { + let base = std::env::temp_dir().join(format!("idx-cache-link-{}", fastrand::u64(..))); + let outside = base.join("documents"); + let root = base.join("cache"); + std::fs::create_dir_all(&outside).unwrap(); + std::fs::create_dir_all(root.join("quote")).unwrap(); + std::fs::write(outside.join("keep.txt"), b"mine").unwrap(); + std::fs::write(root.join("quote").join("BBCA.json"), b"{}").unwrap(); + std::os::unix::fs::symlink(&outside, root.join("link")).unwrap(); + + let (removed, failed) = super::Cache::at(root.clone()).clear().unwrap(); + + assert!(failed.is_empty(), "{failed:?}"); + assert_eq!(removed, 2, "the cache file and the link itself"); + assert!( + outside.join("keep.txt").exists(), + "the link's target is left alone" + ); + assert!(!root.join("link").exists()); + std::fs::remove_dir_all(&base).unwrap(); + } use std::fs; use std::sync::atomic::{AtomicUsize, Ordering}; diff --git a/crates/idx-core/src/investor.rs b/crates/idx-core/src/investor.rs index b5012a2..6a13ca8 100644 --- a/crates/idx-core/src/investor.rs +++ b/crates/idx-core/src/investor.rs @@ -635,7 +635,7 @@ pub fn parse_report_family(raw: &str) -> Result, I "investor-type" | "investor_type" | "investortype" => { Ok(Some(OwnershipReportFamily::InvestorTypeBreakdown)) } - _ => Err(IdxError::ParseError( + _ => Err(IdxError::InvalidInput( "invalid --family, expected: all|above1|above5|investor-type".to_string(), )), } diff --git a/crates/idx-core/src/lib.rs b/crates/idx-core/src/lib.rs index bcc2e52..a89bc2f 100644 --- a/crates/idx-core/src/lib.rs +++ b/crates/idx-core/src/lib.rs @@ -23,6 +23,7 @@ pub mod runtime { use std::sync::atomic::{AtomicBool, Ordering}; static QUIET: AtomicBool = AtomicBool::new(false); + static JSON: AtomicBool = AtomicBool::new(false); pub fn set_quiet(quiet: bool) { QUIET.store(quiet, Ordering::Relaxed); @@ -32,15 +33,29 @@ pub mod runtime { QUIET.load(Ordering::Relaxed) } - pub fn info(message: impl Display) { - if !is_quiet() { - eprintln!("info: {message}"); + /// In JSON mode diagnostics are JSON lines, so stderr stays + /// machine-readable alongside a JSON error. + pub fn set_json(json: bool) { + JSON.store(json, Ordering::Relaxed); + } + + fn emit(level: &str, message: impl Display) { + if is_quiet() { + return; + } + if JSON.load(Ordering::Relaxed) { + let line = serde_json::json!({ "level": level, "message": message.to_string() }); + eprintln!("{line}"); + } else { + eprintln!("{level}: {message}"); } } + pub fn info(message: impl Display) { + emit("info", message); + } + pub fn warn(message: impl Display) { - if !is_quiet() { - eprintln!("warning: {message}"); - } + emit("warning", message); } } diff --git a/crates/idx-core/src/ownership/graph.rs b/crates/idx-core/src/ownership/graph.rs index d51fd12..274ec81 100644 --- a/crates/idx-core/src/ownership/graph.rs +++ b/crates/idx-core/src/ownership/graph.rs @@ -14,7 +14,7 @@ pub fn query_ownership_graph( ) -> Result<(Vec, Vec), IdxError> { let root = root.trim(); if root.is_empty() { - return Err(IdxError::ParseError( + return Err(IdxError::InvalidInput( "graph root cannot be empty".to_string(), )); } diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index d5c10ae..bb7d1dd 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -162,7 +162,12 @@ CLI flags > environment variables > config file > defaults ## Error Strategy - `IdxError` enum (thiserror) with structured error codes - Table mode: human-readable error on stderr -- JSON and JSONL modes: `{"error": true, "code": "...", "message": "..."}` on stderr, stdout empty +- JSON and JSONL modes: `{"error": true, "code": "...", "message": "..."}` on stderr, stdout empty. + This includes argument errors (`INVALIDINPUT`, exit 2) when `-o`/`--output`, + `IDX_OUTPUT`, or auto mode with stdout not a terminal selects JSON; `--help` + and `--version` print normally. +- Untrusted text (provider strings, KSEI names) has control characters and + bidirectional overrides replaced before it reaches a terminal table. - Exit codes by class: `0` success, `2` `INVALIDINPUT`/`CONFIGERROR` and clap usage errors, `3` `SYMBOLNOTFOUND`/`NOMARKETDATA`/`CACHEMISS`, `4` `HTTP`/`RATELIMITED`/ `PROVIDERUNAVAILABLE`/`OFFLINE`/`AUTHERROR`, `5` `UNSUPPORTED`, `1` everything else @@ -182,4 +187,6 @@ CLI flags > environment variables > config file > defaults - `stocks quote` and `stocks compare` require at least one symbol. - Offline reads warn on stderr when serving cache entries past their TTL, including the cache fetch timestamp. `--quiet` suppresses these warnings. + In JSON mode warnings and info are JSON lines, + `{"level": "warning", "message": "..."}`, so stderr stays parseable. Staleness is not yet represented by a versioned JSON result envelope.