From aa391bf513151d09fcb645f3ba1010058ca5d30c Mon Sep 17 00:00:00 2001 From: Nathan DeMoss Date: Thu, 3 Sep 2026 17:34:16 -0400 Subject: [PATCH 1/2] fix(cli): validate logs --level and --source values Both flags took any string and forwarded it to the gateway, where an unrecognized value is ignored rather than rejected. The two fail in opposite directions: level_matches ranks an unknown level below every real one, so `--level warning` returns every level instead of warn and above, while source_matches compares exact strings, so a mistyped `--source` returns nothing. Neither reports an error. Make both flags value enums so a typo is an argument error that names the valid values. Matching stays case-insensitive, `--source` is still repeatable, and the defaults are unchanged. The gateway's permissiveness is deliberate for a log line's own level, since a sandbox emitting an unusual level should not disappear. The same ranking is applied to the caller's threshold, where it only disables the filter. Validating in the CLI leaves the log-line behavior alone and rejects the value a user can actually get wrong. Signed-off-by: Nathan DeMoss --- crates/openshell-cli/src/main.rs | 121 +++++++++++++++++++++++++++++-- 1 file changed, 114 insertions(+), 7 deletions(-) diff --git a/crates/openshell-cli/src/main.rs b/crates/openshell-cli/src/main.rs index befac54759..ee191b36df 100644 --- a/crates/openshell-cli/src/main.rs +++ b/crates/openshell-cli/src/main.rs @@ -539,12 +539,12 @@ enum Commands { /// Filter by log source: "gateway", "sandbox", or "all" (default). /// Can be specified multiple times: --source gateway --source sandbox - #[arg(long, default_value = "all")] - source: Vec, + #[arg(long, value_enum, ignore_case = true, default_value = "all")] + source: Vec, - /// Minimum log level to display: error, warn, info (default), debug, trace. - #[arg(long, default_value = "")] - level: String, + /// Minimum log level to display: error, warn, info, debug, trace. + #[arg(long, value_enum, ignore_case = true)] + level: Option, }, /// Manage sandbox policy. @@ -753,6 +753,54 @@ fn normalize_completion_script(output: Vec, executable: &std::path::Path) -> Ok(script.replace(executable.to_string_lossy().as_ref(), "openshell")) } +/// Log source accepted by `openshell logs --source`. +/// +/// The server matches sources by exact string, so an unrecognized value +/// silently filters every line out. Restricting the flag to known values makes +/// a typo an argument error instead of an empty result. +#[derive(Clone, Copy, Debug, PartialEq, Eq, ValueEnum)] +enum LogSource { + Gateway, + Sandbox, + All, +} + +impl LogSource { + const fn as_str(self) -> &'static str { + match self { + Self::Gateway => "gateway", + Self::Sandbox => "sandbox", + Self::All => "all", + } + } +} + +/// Minimum severity accepted by `openshell logs --level`. +/// +/// The server ranks an unrecognized level below every real one, so a typo +/// silently disables the filter and returns all levels. Restricting the flag to +/// known values makes that a visible argument error. +#[derive(Clone, Copy, Debug, PartialEq, Eq, ValueEnum)] +enum LogLevel { + Error, + Warn, + Info, + Debug, + Trace, +} + +impl LogLevel { + const fn as_str(self) -> &'static str { + match self { + Self::Error => "error", + Self::Warn => "warn", + Self::Info => "info", + Self::Debug => "debug", + Self::Trace => "trace", + } + } +} + #[derive(Clone, Debug, PartialEq, Eq, ValueEnum)] enum OutputFormat { Table, @@ -2794,14 +2842,19 @@ async fn run_async() -> Result<()> { let mut tls = tls.with_gateway_name(&ctx.name); apply_auth(&mut tls, &ctx.name); let name = resolve_sandbox_name(name, &ctx.name, &cli.workspace)?; + let sources = source + .iter() + .map(|value| value.as_str().to_string()) + .collect::>(); + let level = level.map_or("", LogLevel::as_str); run::sandbox_logs( &ctx.endpoint, &name, n, tail, since.as_deref(), - &source, - &level, + &sources, + level, &cli.workspace, &tls, ) @@ -4486,6 +4539,60 @@ mod tests { assert_eq!(dest.get_value_hint(), ValueHint::AnyPath); } + #[test] + fn logs_level_accepts_known_values_any_case() { + for value in ["error", "warn", "info", "debug", "trace", "ERROR", "Warn"] { + Cli::try_parse_from(["openshell", "logs", "sb", "--level", value]) + .unwrap_or_else(|err| panic!("--level {value} should parse: {err}")); + } + } + + /// The server ranks an unrecognized level below every real one, so a typo + /// used to disable the filter and return all levels instead of erroring. + #[test] + fn logs_level_rejects_unknown_values() { + for value in ["warning", "critical", "wanr", ""] { + Cli::try_parse_from(["openshell", "logs", "sb", "--level", value]) + .expect_err(&format!("--level {value} should be rejected")); + } + } + + /// The server matches sources by exact string, so a typo used to filter + /// every line out and return nothing instead of erroring. + #[test] + fn logs_source_rejects_unknown_values() { + Cli::try_parse_from(["openshell", "logs", "sb", "--source", "gatewy"]) + .expect_err("--source gatewy should be rejected"); + } + + #[test] + fn logs_source_accepts_repeated_known_values() { + let cli = Cli::try_parse_from([ + "openshell", + "logs", + "sb", + "--source", + "gateway", + "--source", + "sandbox", + ]) + .expect("repeated --source should parse"); + let Some(Commands::Logs { source, .. }) = cli.command else { + panic!("expected logs command"); + }; + assert_eq!(source, vec![LogSource::Gateway, LogSource::Sandbox]); + } + + #[test] + fn logs_defaults_to_all_sources_and_no_level_filter() { + let cli = Cli::try_parse_from(["openshell", "logs", "sb"]).expect("logs should parse"); + let Some(Commands::Logs { source, level, .. }) = cli.command else { + panic!("expected logs command"); + }; + assert_eq!(source, vec![LogSource::All]); + assert_eq!(level, None); + } + #[test] fn parse_upload_spec_without_remote() { let (local, remote) = parse_upload_spec("./src"); From b0acf82ba60f0bc32d1aaad53d8eade4524b7e72 Mon Sep 17 00:00:00 2001 From: Nathan DeMoss Date: Fri, 4 Sep 2026 14:23:13 -0400 Subject: [PATCH 2/2] fix(cli): keep ocsf accepted as a logs level The gateway ranks OCSF alongside INFO, so `--level ocsf` selected the same lines as `--level info` and was a working filter rather than a string that happened to pass validation. Keep it accepted. It stays hidden in --help because it selects exactly what info does, and listing both spellings for one threshold would imply a distinction that does not exist. Also cover mixed-case --source values, which the flag accepted but no test exercised. Signed-off-by: Nathan DeMoss --- crates/openshell-cli/src/main.rs | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/crates/openshell-cli/src/main.rs b/crates/openshell-cli/src/main.rs index ee191b36df..edcccc1c35 100644 --- a/crates/openshell-cli/src/main.rs +++ b/crates/openshell-cli/src/main.rs @@ -785,6 +785,12 @@ enum LogLevel { Error, Warn, Info, + /// Accepted because the gateway ranks OCSF alongside INFO, so this has + /// always worked as a threshold. Hidden rather than advertised: it selects + /// exactly the same lines as `info`, and offering two spellings for one + /// threshold in `--help` would suggest a distinction that does not exist. + #[value(hide = true)] + Ocsf, Debug, Trace, } @@ -795,6 +801,7 @@ impl LogLevel { Self::Error => "error", Self::Warn => "warn", Self::Info => "info", + Self::Ocsf => "ocsf", Self::Debug => "debug", Self::Trace => "trace", } @@ -4541,12 +4548,22 @@ mod tests { #[test] fn logs_level_accepts_known_values_any_case() { - for value in ["error", "warn", "info", "debug", "trace", "ERROR", "Warn"] { + for value in [ + "error", "warn", "info", "debug", "trace", "ERROR", "Warn", "ocsf", "OCSF", + ] { Cli::try_parse_from(["openshell", "logs", "sb", "--level", value]) .unwrap_or_else(|err| panic!("--level {value} should parse: {err}")); } } + #[test] + fn logs_source_accepts_known_values_any_case() { + for value in ["gateway", "sandbox", "all", "Gateway", "SANDBOX"] { + Cli::try_parse_from(["openshell", "logs", "sb", "--source", value]) + .unwrap_or_else(|err| panic!("--source {value} should parse: {err}")); + } + } + /// The server ranks an unrecognized level below every real one, so a typo /// used to disable the filter and return all levels instead of erroring. #[test]