From fa5b4f61840d7f9a4827bcd170c2cd2e6ef7b076 Mon Sep 17 00:00:00 2001 From: Mykhailo Chalyi Date: Thu, 22 Oct 2026 09:38:08 +0000 Subject: [PATCH 1/2] fix(builtins): read shuf's input range under either generated value type MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The weekly Coreutils Args Drift run has been red since upstream dropped `.value_parser(parse_range)` from `shuf`'s `input-range` arg and moved range parsing into its own `uumain`. The regenerated surface therefore stores a plain `String` where the pin stores a `RangeInclusive`. `shuf.rs` named only the pre-parsed type, so `get_one::>` mismatched on the regenerated surface. bashkit catches builtin panics, so the only symptom was `shuf -i 1-5` printing nothing: 3960/3989 spec cases passed and `shuf_input_range` failed. Run 39753862402 died there, at *Run cat/tac spec tests* — after regeneration but before the patch upload, so no drift patch and no PR, which is why the drift has stayed invisible. This is the documented failure mode from the `od` case (#2751/#2754): the drift PR enforces a generated-only diff and cannot repair its own consumers, so the consumer fix has to land first and tolerate both surfaces. The new variant is that the two types are not the `String`/`OsString` pair `helpers::string_value` already covers — upstream added and then dropped a value parser yielding a non-string type, moving the arg between "clap pre-parsed it" and "clap handed back raw text". `read_input_range` tries `try_get_one::>` first and falls back to `helpers::string_value` plus a local `parse_input_range`; an `Err` from the typed read means the other surface, not a bad value. The typed read is tried first so the string fallback never trips the `debug_assert` guarding a third, unhandled value type. Verified against both surfaces. Regenerating `shuf_args.rs` from upstream HEAD (eb7d426) shows the dropped value parser is the only semantic change in shuf's whole surface. On that regenerated surface the full spec suite, the shuf unit tests and the shuf resource tests pass; reverting to the pinned surface, all three pass again. Before the fix, the same regenerated surface failed `shuf_input_range` with empty stdout. Behaviour note, recorded in the knowledge: a malformed `-i` exits 2 with clap's rendering under the pinned surface, because clap rejects the value before shuf sees it. On the regenerated surface `parse_input_range` reports it instead and exits 1 with GNU's `shuf: invalid input range: ''`. The pin bump moves this toward GNU, not away; no spec case pins the old shape. --- crates/bashkit/src/builtins/shuf.rs | 149 +++++++++++++++++++++- knowledge/runtimes/coreutils-args-port.md | 28 ++++ 2 files changed, 174 insertions(+), 3 deletions(-) diff --git a/crates/bashkit/src/builtins/shuf.rs b/crates/bashkit/src/builtins/shuf.rs index b0f1c9f82..2959e3c97 100644 --- a/crates/bashkit/src/builtins/shuf.rs +++ b/crates/bashkit/src/builtins/shuf.rs @@ -67,13 +67,16 @@ impl Builtin for Shuf { let zero_terminated = matches.get_flag("zero-terminated"); // The generated args declare value_parsers that pre-parse these // values for us — we read the typed result, not the raw String. + // `input-range` is the exception; see `read_input_range`. let head_count: Option = matches .get_many::("head-count") .and_then(|mut vs| vs.next_back().copied()); let output_path = matches.get_one::("output").cloned(); - let input_range = matches - .get_one::>("input-range") - .cloned(); + let input_range = match read_input_range(&matches) { + Some(Ok(range)) => Some(range), + Some(Err(stderr)) => return Ok(ExecResult::err(stderr, 1)), + None => None, + }; let positionals: Vec = matches .get_many::("file-or-args") .map(|vs| vs.map(|v| v.to_string_lossy().into_owned()).collect()) @@ -137,6 +140,63 @@ impl Builtin for Shuf { } } +/// Read `-i LO-HI` under either value type the generated surface carries. +/// +/// Decision: at the pinned rev the ported surface declares +/// `.value_parser(parse_range)` on `input-range`, so clap hands back a +/// `RangeInclusive`. Upstream has since dropped that value parser and +/// parses the raw string in its own `uumain`, so the regenerated surface +/// stores a plain `String` and a consumer naming only the pre-parsed type +/// reads nothing — `shuf -i 1-5` printed empty output and failed the weekly +/// drift run (run 39753862402). The drift PR is generated-only and cannot +/// fix its own consumers (`knowledge/runtimes/coreutils-args-port.md` § The +/// drift PR cannot fix its own consumers), so this reads both and stays +/// valid against the pin and upstream HEAD alike — the same rationale as +/// `helpers::string_value`. +/// +/// `None` means no `-i` was given; `Some(Err(_))` is a GNU-shaped diagnostic +/// for a malformed range. +fn read_input_range( + matches: &clap::ArgMatches, +) -> Option, String>> { + // Pinned surface: clap already parsed the range. An `Err` here is a + // value-type mismatch, i.e. the regenerated surface, not a bad value. + if let Ok(range) = matches.try_get_one::>("input-range") { + return range.cloned().map(Ok); + } + super::helpers::string_value(matches, "input-range").map(|raw| parse_input_range(&raw)) +} + +/// Parse `LO-HI` the way GNU `shuf` does, including its diagnostics. +/// +/// Mirrors upstream's own `parse_range`, which is body code rather than +/// argument surface and so is not vendored: GNU reports the range as a +/// whole rather than the offending bound, and distinguishes only the +/// does-not-fit-in-`u64` case. Exit status is 1, matching upstream's +/// `USimpleError::new(1, ...)` — clap's own rejection under the pinned +/// surface exits 2 instead, so this path is strictly closer to GNU. +fn parse_input_range(raw: &str) -> std::result::Result, String> { + let invalid = || format!("shuf: invalid input range: '{raw}'\n"); + let Some((from, to)) = raw.split_once('-') else { + return Err(invalid()); + }; + let parse = |bound: &str| match bound.parse::() { + Ok(n) => Ok(n), + Err(e) if *e.kind() == std::num::IntErrorKind::PosOverflow => Err(format!( + "shuf: invalid input range: '{raw}': Value too large to be stored in data type\n" + )), + Err(_) => Err(invalid()), + }; + let begin = parse(from)?; + let end = parse(to)?; + // `begin == end + 1` is GNU's spelling of an empty range, e.g. `-i 5-4`. + if begin <= end || begin == end + 1 { + Ok(begin..=end) + } else { + Err(invalid()) + } +} + fn split_separated(raw: &str, sep: char) -> Vec { if raw.is_empty() { return Vec::new(); @@ -437,6 +497,89 @@ mod tests { ); } + /// The regenerated surface: upstream dropped `.value_parser(parse_range)`, + /// so `-i` stores a plain `String`. Built by hand rather than by + /// regenerating, so the guard holds without a live upstream checkout. + fn head_shaped_cmd() -> clap::Command { + clap::Command::new("shuf").no_binary_name(true).arg( + clap::Arg::new("input-range") + .short('i') + .long("input-range") + .value_name("LO-HI"), + ) + } + + /// The pinned surface: clap pre-parses `-i` into a `RangeInclusive`. + fn pinned_shaped_cmd() -> clap::Command { + clap::Command::new("shuf").no_binary_name(true).arg( + clap::Arg::new("input-range") + .short('i') + .long("input-range") + .value_name("LO-HI") + .value_parser(|s: &str| parse_input_range(s).map_err(|e| e.trim().to_string())), + ) + } + + #[test] + fn reads_input_range_under_both_value_types() { + for cmd in [pinned_shaped_cmd(), head_shaped_cmd()] { + let m = cmd.try_get_matches_from(["-i", "1-5"]).unwrap(); + assert_eq!(read_input_range(&m), Some(Ok(1..=5))); + } + } + + #[test] + fn absent_input_range_reads_none_under_both_value_types() { + for cmd in [pinned_shaped_cmd(), head_shaped_cmd()] { + let m = cmd.try_get_matches_from::<_, &str>([]).unwrap(); + assert_eq!(read_input_range(&m), None); + } + } + + #[test] + fn malformed_range_on_regenerated_surface_reports_like_gnu() { + let m = head_shaped_cmd() + .try_get_matches_from(["-i", "1x5"]) + .unwrap(); + assert_eq!( + read_input_range(&m), + Some(Err("shuf: invalid input range: '1x5'\n".to_string())) + ); + } + + #[test] + fn parse_input_range_accepts_gnu_empty_range() { + // GNU spells the empty range `LO-HI` with `LO == HI + 1`. Asserted + // through the bounds rather than a `5..=4` literal, which clippy + // reads as a reversed-range typo. + let empty = parse_input_range("5-4").expect("LO == HI + 1 is the empty range"); + assert_eq!((*empty.start(), *empty.end()), (5, 4)); + assert!(empty.is_empty()); + // One further apart is a genuine inversion and is rejected. + assert!(parse_input_range("6-4").is_err()); + } + + #[test] + fn parse_input_range_rejects_malformed_input() { + for raw in ["", "5", "a-b", "1-", "-", "1-2-3"] { + assert!( + parse_input_range(raw).is_err(), + "expected {raw:?} to be rejected" + ); + } + } + + #[test] + fn parse_input_range_distinguishes_overflow() { + let too_big = "1-18446744073709551616"; + assert_eq!( + parse_input_range(too_big), + Err(format!( + "shuf: invalid input range: '{too_big}': Value too large to be stored in data type\n" + )) + ); + } + #[test] fn small_rng_bounded_in_range() { let mut rng = SmallRng::seed_from_now(); diff --git a/knowledge/runtimes/coreutils-args-port.md b/knowledge/runtimes/coreutils-args-port.md index 53f821fac..af60a38af 100644 --- a/knowledge/runtimes/coreutils-args-port.md +++ b/knowledge/runtimes/coreutils-args-port.md @@ -404,6 +404,34 @@ real generated surface twice, once with `OsStringValueParser` forced onto every flippable arg via `Command::mut_arg`, and asserts both yield the same options. +The rule is not limited to the `String`/`OsString` pair. Upstream may +also **add or drop a value parser that yields a non-string type**, which +moves an arg between "clap pre-parsed it" and "clap handed back the raw +text". `string_value` covers only the second state, so an arg in the +first needs its own tolerant read. + +Worked example (2026-10, drift run 39753862402): upstream dropped +`.value_parser(parse_range)` from `shuf`'s `input-range` and moved range +parsing into its own `uumain`, so the regenerated surface stores a plain +`String` where the pin stores a `RangeInclusive`. `shuf.rs` read +only the pre-parsed type, so `shuf -i 1-5` produced no output and +`shuf_input_range` failed — that run stayed red at *Run cat/tac spec +tests*, again after regeneration, so no patch uploaded and no PR +opened. Fix: `shuf::read_input_range` tries +`try_get_one::>` first and falls back to +`helpers::string_value` plus a local `parse_input_range`; an `Err` from +the typed read means the other surface, not a bad value. Guards: +`shuf::tests::reads_input_range_under_both_value_types` and +`absent_input_range_reads_none_under_both_value_types` build both arg +shapes by hand, so they hold without a live upstream checkout. + +Behaviour note: a malformed `-i` exits 2 with clap's rendering under the +pinned surface, because clap rejects the value before `shuf` sees it. +On the regenerated surface `parse_input_range` reports it instead, so it +exits 1 with GNU's `shuf: invalid input range: ''` — the GNU +behaviour. The pin bump therefore moves this diagnostic closer to GNU +rather than away from it; no spec case pins the old shape. + 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 From 590d35a4f7e73352fc50f5f3e306fb0e59b910e1 Mon Sep 17 00:00:00 2001 From: Mykhailo Chalyi Date: Thu, 22 Oct 2026 09:48:54 +0000 Subject: [PATCH 2/2] fix(builtins): avoid Debug formatting in shuf range test message The TM-INF-022 static scan rejects `{raw:?}` anywhere under `src/builtins/`, test asserts included unless marked `// debug-ok`. Here Debug only added the surrounding quotes, so the message spells them itself instead of claiming an exemption. --- crates/bashkit/src/builtins/shuf.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bashkit/src/builtins/shuf.rs b/crates/bashkit/src/builtins/shuf.rs index 2959e3c97..efb489aca 100644 --- a/crates/bashkit/src/builtins/shuf.rs +++ b/crates/bashkit/src/builtins/shuf.rs @@ -564,7 +564,7 @@ mod tests { for raw in ["", "5", "a-b", "1-", "-", "1-2-3"] { assert!( parse_input_range(raw).is_err(), - "expected {raw:?} to be rejected" + "expected '{raw}' to be rejected" ); } }