Skip to content

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

Merged
chaliy merged 2 commits into
mainfrom
eve/clever-mccarthy-yvt41j
Oct 19, 2026
Merged

chaliy merged 2 commits into
mainfrom
eve/clever-mccarthy-yvt41j

Conversation

@chaliy

@chaliy chaliy commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

What changed

od now reads its clap-parsed arguments without naming the value type upstream chose for them. Two new helpers, helpers::string_values and helpers::string_value, try OsString then String through try_get_many / try_get_one (which return Err rather than panicking), and od uses them for FILENAME, -t/--format, --address-radix, --skip-bytes, --read-bytes, --width and --endian.

No user-visible change on the current pin. What changes is that od keeps working when upstream flips an arg's value parser.

Why

The weekly Coreutils Args Drift run (39202465007) is red on main. Upstream gave od's FILENAME positional an OsStringValueParser; od_options_from_matches read it with get_many::<String>, and clap panics on a value-type mismatch.

bashkit catches builtin panics, so od just produced nothing and the only symptom was one spec case losing one line. The run fails after regeneration, so no patch is uploaded and no drift PR opens — the job stays red until someone reads the log.

The drift PR enforces a generated-only diff and so can never fix its own consumers (knowledge/runtimes/coreutils-args-port.md § The drift PR cannot fix its own consumers). Per that contract the consumer fix lands first, in its own PR, tolerating both value types so the surface is valid against the current pin and upstream HEAD — the only state in which the drift run can get past its build and test steps. The regenerated surfaces follow in the drift PR.

Before / After

Reproduced locally by regenerating od_args.rs against upstream HEAD (bbf2bfff2) and bumping the pin.

Before — cargo test -p bashkit --test integration -- spec_tests::bash_spec_tests:

thread 'spec_tests::bash_spec_tests' panicked at crates/bashkit/src/builtins/hextools.rs:306:10:
Mismatch between definition and access of `FILENAME`. Could not downcast to
alloc::string::String, need to downcast to std::ffi::os_str::OsString
Total: 3989 | Passed: 3960 | Failed: 1 | Skipped: 28

[process-env-builtins.test] dd_zero_device_and_seek
  Expected stdout: "3072\n  \0   X   Y  \0  \0 ...\n32\n"
  Got stdout:      "3072\n32\n"

After — same regenerated surface, unchanged otherwise:

test spec_tests::bash_spec_tests ... ok
test result: ok. 1 passed; 0 failed

Both surfaces verified:

Surface Spec suite od unit tests
pinned 8613dffab (String) 3989 pass 48 pass
upstream HEAD bbf2bfff2 (OsString) 3989 pass 48 pass

The guard test reproduces the CI panic when the fix is reverted:

test builtins::hextools::tests::od_options_read_either_value_parser ... FAILED
panicked at crates/bashkit/src/builtins/hextools.rs:304:10:
Mismatch between definition and access of `FILENAME`.

Risk

  • Low.
  • The helpers are strictly more permissive than the reads they replace: same to_string_lossy text conversion already used by cat, ls and truncate, and an absent or unknown arg yields None / empty instead of panicking.
  • Scoped to od. Nine sibling builtins still name OsString directly and carry the mirror-image exposure (an upstream flip back to String); all only want text, so they are mechanically convertible. Recorded in the knowledge doc as "convert on touch" rather than widening this PR.

Checklist

  • Tests added or updated
  • Backward compatibility considered

The weekly Coreutils Args Drift run (39202465007) went red at *Run
cat/tac spec tests*: upstream gave `od`'s `FILENAME` positional an
`OsStringValueParser`, and `od_options_from_matches` read it with
`get_many::<String>`, which panics on a value-type mismatch
("Mismatch between definition and access of `FILENAME`").

bashkit catches builtin panics, so the only visible symptom was `od`
producing no output: the `dd_zero_device_and_seek` spec case lost its
`od -An -c` line and nothing else. The failure lands after regeneration,
so no patch is uploaded and no drift PR opens — the job just stays red.

Whether a generated arg carries clap's default `String` parser or an
explicit `OsStringValueParser` is upstream's choice and flips across
revisions, and the drift PR is generated-only so it cannot fix its own
consumers. `helpers::string_values` / `string_value` try `OsString` then
`String` through `try_get_many` / `try_get_one`, which return `Err`
instead of panicking, so the consumer stays valid against the current
pin and upstream HEAD at once — the only state in which the drift run
can get past its build and test steps. Applied to every `od` arg whose
value type upstream owns, not just the one that flipped.

Verified against both surfaces: the pinned rev's `od_args.rs` and the
one regenerated from upstream HEAD (bbf2bfff2) each pass the full spec
suite (3989 cases) and od's unit tests. Without the fix, the new
`od_options_read_either_value_parser` test reproduces the CI panic
verbatim.
@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Oct 19, 2026 •

Copy link
Copy Markdown

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Preview URL Updated (UTC)
✅ Deployment successful!
View logs
bashkit 2123d5f Commit Preview URL

Branch Preview URL
Oct 19 2026, 09:48 AM

…dled

Trying `OsString` then `String` and giving up turns clap's panic into a
silent "no value": an arg the user did pass would be quietly ignored,
with no diagnostic at all. For `od` that would mean `-w`, `-j`, `-N` or
`--endian` reverting to defaults rather than erroring — worse than the
panic this helper exists to remove, 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. Both helpers now `debug_assert!` on that
case: loud under `cargo test`, and so in the weekly drift run's own test
steps where it is actionable, while an embedder's release build degrades
instead of aborting.
@chaliy
chaliy merged commit d199685 into main Oct 19, 2026
50 checks passed
@chaliy
chaliy deleted the eve/clever-mccarthy-yvt41j branch October 19, 2026 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant