fix(builtins): read od's generated args under either clap value type

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::<T>` *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<String> {
+    if let Ok(Some(values)) = matches.try_get_many::<std::ffi::OsString>(id) {
+        return values.map(|v| v.to_string_lossy().into_owned()).collect();
+    }
+    if let Ok(Some(values)) = matches.try_get_many::<String>(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<String> {
+    if let Ok(Some(value)) = matches.try_get_one::<std::ffi::OsString>(id) {
+        return Some(value.to_string_lossy().into_owned());
+    }
+    if let Ok(Some(value)) = matches.try_get_one::<String>(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()];

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::<String>("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::<String>("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::<String>("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::<String>("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::<String>("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::<String>("endian") {
+    if let Some(e) = string_value(matches, "endian") {
         opts.big_endian = e == "big";
     }
 
-    let files: Vec<String> = matches
-        .get_many::<String>("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();

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::<T>` /
+`get_one::<T>` **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::<String>("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