diff --git a/crates/bashkit/src/builtins/helpers.rs b/crates/bashkit/src/builtins/helpers.rs index 6306ea16d..ec62b82d5 100644 --- a/crates/bashkit/src/builtins/helpers.rs +++ b/crates/bashkit/src/builtins/helpers.rs @@ -39,6 +39,65 @@ pub(crate) fn invalid_option(cmd: &str, arg: &str, code: i32) -> ExecResult { ExecResult::err(msg, code) } +/// Read a multi-valued clap arg as owned strings under either value type +/// clap may have stored for it. +/// +/// Decision: the `generated/*_args.rs` surfaces are ported verbatim from +/// uutils, so whether an arg carries clap's default `String` parser or an +/// explicit `OsStringValueParser` is upstream's choice, and it flips +/// across revisions. `get_many::` *panics* on a type mismatch, so a +/// consumer that names one type turns an upstream parser change into a +/// panic — and the drift PR is generated-only, so it cannot fix its own +/// consumers (`knowledge/runtimes/coreutils-args-port.md` § The drift PR +/// cannot fix its own consumers). Asking for both keeps the consumer +/// valid against the current pin and upstream HEAD at the same time, +/// which is the only state in which the weekly drift run can get past its +/// build and test steps. +/// +/// Non-UTF-8 bytes are replaced, matching the `to_string_lossy` reads in +/// `cat`, `ls` and `truncate`. +pub(crate) fn string_values(matches: &clap::ArgMatches, id: &str) -> Vec { + if let Ok(Some(values)) = matches.try_get_many::(id) { + return values.map(|v| v.to_string_lossy().into_owned()).collect(); + } + if let Ok(Some(values)) = matches.try_get_many::(id) { + return values.cloned().collect(); + } + debug_assert_readable(matches, id, "string_values"); + Vec::new() +} + +/// Single-valued counterpart of [`string_values`]; same rationale. +pub(crate) fn string_value(matches: &clap::ArgMatches, id: &str) -> Option { + if let Ok(Some(value)) = matches.try_get_one::(id) { + return Some(value.to_string_lossy().into_owned()); + } + if let Ok(Some(value)) = matches.try_get_one::(id) { + return Some(value.clone()); + } + debug_assert_readable(matches, id, "string_value"); + None +} + +/// Guard the one hole the tolerant reads above open: a *third* value type. +/// +/// Trying two types and giving up turns clap's loud panic into a silent +/// "no value", which for an arg the user did pass would mean the flag is +/// quietly ignored — worse than a panic, because nothing reports it. Only +/// `String` and `OsString` are expected (upstream flips between exactly +/// those two), so an arg that is present and readable as neither is a +/// porting bug, not user input. Firing only under `debug_assertions` +/// keeps the loud signal everywhere it is actionable — `cargo test`, and +/// so the weekly drift run's own test steps — while an embedder's release +/// build degrades instead of aborting. +fn debug_assert_readable(matches: &clap::ArgMatches, id: &str, caller: &str) { + debug_assert!( + !matches.try_contains_id(id).unwrap_or(false), + "helpers::{caller}: arg `{id}` is present but is neither OsString \ + nor String; teach this helper the value type upstream now uses" + ); +} + /// Default helpers shared by builtin commands. /// /// Each impl binds the command name to the type, removing the need to @@ -113,6 +172,90 @@ mod tests { assert_eq!(r.stderr, "wc: unrecognized option '--foo'\n"); } + /// Two spellings of the same surface: clap's default `String` parser + /// (what the pinned uutils rev generates) and an explicit + /// `OsStringValueParser` (what upstream HEAD generates). + fn arg_surface(os_string: bool) -> clap::Command { + let many = clap::Arg::new("files") + .num_args(0..) + .action(clap::ArgAction::Append); + let one = clap::Arg::new("width").long("width"); + let (many, one) = if os_string { + ( + many.value_parser(clap::builder::OsStringValueParser::new()), + one.value_parser(clap::builder::OsStringValueParser::new()), + ) + } else { + (many, one) + }; + clap::Command::new("demo").arg(many).arg(one) + } + + #[test] + fn string_values_reads_either_value_parser() { + for os_string in [false, true] { + let matches = arg_surface(os_string) + .try_get_matches_from(["demo", "a", "b"]) + .unwrap(); + assert_eq!( + string_values(&matches, "files"), + vec!["a".to_string(), "b".to_string()], + "os_string={os_string}" + ); + } + } + + #[test] + fn string_value_reads_either_value_parser() { + for os_string in [false, true] { + let matches = arg_surface(os_string) + .try_get_matches_from(["demo", "--width", "16"]) + .unwrap(); + assert_eq!( + string_value(&matches, "width").as_deref(), + Some("16"), + "os_string={os_string}" + ); + } + } + + #[test] + fn string_reads_report_absent_args_as_empty() { + for os_string in [false, true] { + let matches = arg_surface(os_string) + .try_get_matches_from(["demo"]) + .unwrap(); + assert!(string_values(&matches, "files").is_empty()); + assert_eq!(string_value(&matches, "width"), None); + } + } + + /// A value type neither read handles must not pass silently: a flag the + /// user did supply would otherwise be dropped with no diagnostic. + #[cfg(debug_assertions)] + #[test] + #[should_panic(expected = "neither OsString nor String")] + fn string_value_rejects_an_unhandled_value_type() { + let matches = clap::Command::new("demo") + .arg( + clap::Arg::new("width") + .long("width") + .value_parser(clap::value_parser!(u64)), + ) + .try_get_matches_from(["demo", "--width", "16"]) + .unwrap(); + let _ = string_value(&matches, "width"); + } + + /// An id that is not in the surface at all must not panic the way + /// `get_many`/`get_one` would. + #[test] + fn string_reads_tolerate_unknown_ids() { + let matches = arg_surface(true).try_get_matches_from(["demo"]).unwrap(); + assert!(string_values(&matches, "nope").is_empty()); + assert_eq!(string_value(&matches, "nope"), None); + } + #[test] fn check_help_delegates() { let args = vec!["--help".to_string()]; diff --git a/crates/bashkit/src/builtins/hextools.rs b/crates/bashkit/src/builtins/hextools.rs index 6edf52478..6cfc9f83c 100644 --- a/crates/bashkit/src/builtins/hextools.rs +++ b/crates/bashkit/src/builtins/hextools.rs @@ -4,6 +4,7 @@ //! on the stack, and admits output growth through the shared request budget. use super::clap_cache::cached_command; +use super::helpers::{string_value, string_values}; use async_trait::async_trait; use super::{Builtin, Context}; @@ -226,7 +227,7 @@ fn od_options_from_matches( show_duplicates: matches.get_flag("output-duplicates"), }; - if let Some(radix) = matches.get_one::("address-radix") { + if let Some(radix) = string_value(matches, "address-radix") { opts.addr_radix = match radix.as_str() { "d" => AddrRadix::Decimal, "o" => AddrRadix::Octal, @@ -239,11 +240,8 @@ fn od_options_from_matches( // Specs apply in command-line order, so collect (index, TYPE) pairs from // both `-t` and the single-letter shorthands, then sort. let mut ordered: Vec<(usize, String)> = Vec::new(); - if let (Some(idx), Some(vals)) = ( - matches.indices_of("format"), - matches.get_many::("format"), - ) { - ordered.extend(idx.zip(vals.cloned())); + if let Some(idx) = matches.indices_of("format") { + ordered.extend(idx.zip(string_values(matches, "format"))); } const SHORTHANDS: &[(&str, &str)] = &[ ("a", "a"), @@ -285,27 +283,24 @@ fn od_options_from_matches( }); } - if let Some(s) = matches.get_one::("read-bytes") { - opts.count = Some(parse_od_num(s, "-N")?); + if let Some(s) = string_value(matches, "read-bytes") { + opts.count = Some(parse_od_num(&s, "-N")?); } - if let Some(s) = matches.get_one::("skip-bytes") { - opts.skip = parse_od_num(s, "-j")?; + if let Some(s) = string_value(matches, "skip-bytes") { + opts.skip = parse_od_num(&s, "-j")?; } - if let Some(s) = matches.get_one::("width") { - let width = parse_od_num(s, "-w")?; + if let Some(s) = string_value(matches, "width") { + let width = parse_od_num(&s, "-w")?; if width > OD_MAX_WIDTH { return Err(format!("od: width exceeds maximum of {OD_MAX_WIDTH} bytes")); } opts.width = Some(width); } - if let Some(e) = matches.get_one::("endian") { + if let Some(e) = string_value(matches, "endian") { opts.big_endian = e == "big"; } - let files: Vec = matches - .get_many::("FILENAME") - .map(|vs| vs.cloned().collect()) - .unwrap_or_default(); + let files = string_values(matches, "FILENAME"); Ok((opts, files)) } @@ -994,6 +989,73 @@ mod tests { Od.execute(ctx).await.unwrap() } + /// Upstream owns the value parser on every generated arg: the pinned + /// uutils rev leaves them on clap's default `String` parser, while + /// upstream HEAD gives `od`'s FILENAME an `OsStringValueParser`. Naming + /// one type in the consumer turned that flip into a panic + /// ("Mismatch between definition and access of `FILENAME`") that only + /// the weekly drift run saw. Both surfaces must yield the same options. + #[test] + fn od_options_read_either_value_parser() { + const FLIPPABLE: [&str; 7] = [ + "FILENAME", + "address-radix", + "skip-bytes", + "read-bytes", + "width", + "endian", + "format", + ]; + let argv = [ + "od", + "-An", + "-t", + "x2", + "-c", + "--endian=big", + "-w", + "8", + "-j", + "1", + "-N", + "4", + "z", + ]; + + for os_string in [false, true] { + let mut cmd = crate::builtins::generated::od_args::od_command(); + if os_string { + for id in FLIPPABLE { + cmd = cmd.mut_arg(id, |arg| { + arg.value_parser(clap::builder::OsStringValueParser::new()) + }); + } + } + let matches = cmd.try_get_matches_from(argv).unwrap(); + let (opts, files) = od_options_from_matches(&matches).unwrap(); + + assert_eq!(files, vec!["z".to_string()], "os_string={os_string}"); + assert_eq!(opts.skip, 1, "os_string={os_string}"); + assert_eq!(opts.count, Some(4), "os_string={os_string}"); + assert_eq!(opts.width, Some(8), "os_string={os_string}"); + assert!(opts.big_endian, "os_string={os_string}"); + assert!( + matches!(opts.addr_radix, AddrRadix::None), + "os_string={os_string}" + ); + // `-t x2` then `-c`, kept in command-line order. + assert_eq!(opts.specs.len(), 2, "os_string={os_string}"); + assert!( + opts.specs[0].kind == OdKind::Hex && opts.specs[0].size == 2, + "os_string={os_string}" + ); + assert!( + opts.specs[1].kind == OdKind::Char && opts.specs[1].size == 1, + "os_string={os_string}" + ); + } + } + async fn run_xxd(args: &[&str], stdin: Option<&str>) -> ExecResult { let fs = Arc::new(InMemoryFs::new()); let mut variables = HashMap::new(); diff --git a/knowledge/runtimes/coreutils-args-port.md b/knowledge/runtimes/coreutils-args-port.md index 3f498dade..eda1e404b 100644 --- a/knowledge/runtimes/coreutils-args-port.md +++ b/knowledge/runtimes/coreutils-args-port.md @@ -360,6 +360,55 @@ scans `generated/format/argument.rs` for the styles it actually names and fails if any escapes differently from that constant. A printf unit test covers a control-character argument, the case the spec suite misses. +#### Consumers must not name a generated arg's value type + +Args mode has the same straddle requirement as module mode, through +clap's value types. Whether a generated arg carries clap's default +`String` parser or an explicit `OsStringValueParser` is upstream's +choice and flips across revisions, and `ArgMatches::get_many::` / +`get_one::` **panic** on a type mismatch. A consumer that names one +type therefore turns an upstream parser change into a panic that only +the weekly drift run sees, and that the drift PR cannot repair. + +Rule: read generated args through `builtins::helpers::string_values` / +`string_value`, which try `OsString` then `String` via `try_get_many` / +`try_get_one` (these return `Err` instead of panicking) and so stay +valid against the current pin and upstream HEAD at once. Reading one +type directly is only safe where bashkit owns the surface. + +Trying two types and giving up would turn clap's loud panic into a +silent "no value", quietly dropping a flag the user did pass. Only +`String` and `OsString` are expected, so both helpers +`debug_assert!` that an arg which is present is readable as one of them: +loud under `cargo test` — and so in the drift run's own test steps — +while an embedder's release build degrades instead of aborting. Covered +by `helpers::tests::string_value_rejects_an_unhandled_value_type`. + +Known exposure: `cat`, `ls`, `truncate`, `shuf`, `tac` (`textrev`), +`realpath` (`path`), `tee` (`pipeline`), `stat` (`inspect`) and `mktemp` +(`fileops`) still name `OsString` directly. All of them only want text +(`to_string_lossy().into_owned()`), so they are mechanically +convertible, and each would panic the same way if upstream moved that +arg back to a `String` parser. Convert on touch. + +Worked example (2026-10, #2751): upstream gave `od`'s `FILENAME` +positional an `OsStringValueParser`. `od_options_from_matches` read +`get_many::("FILENAME")`, so the regenerated surface panicked +with "Mismatch between definition and access of `FILENAME`". bashkit +catches builtin panics, so `od` merely produced no output and the only +symptom was the `dd_zero_device_and_seek` spec case losing its `od -An +-c` line — drift run 39202465007 stayed red at *Run cat/tac spec +tests*, after regeneration, so no patch uploaded and no PR opened. +Guard: `hextools::tests::od_options_read_either_value_parser` parses the +real generated surface twice, once with `OsStringValueParser` forced +onto every flippable arg via `Command::mut_arg`, and asserts both yield +the same options. + +Note the asymmetry with the rename rule above: a value-type flip passes +*Verify build still passes* and fails later, in the spec-test or +differential step, because it is a runtime type check rather than a +compile error. + ### Output banner Module-mode files carry the same banner shape as args mode: `GENERATED by