wasip2: build `uucore` and `env` standalone, don't trap on empty argv by Aditya1404Sal · Pull Request #14562 · uutils/coreutils

Aditya1404Sal · 2026-09-14T14:44:05Z

Use stable byte conversions for env on WASI Preview 2 and restore it to feat_common_core. Handle empty WASI argv in embedded use without panicking, with regression tests and a Preview 2 CI step.

Upstream commit 17f1a5d already enables rustix/std through uucore's fs feature, so this PR no longer needs a dependency change.

Verified native uucore/env tests, Preview 2 conversion and argv tests, feat_wasm checks for both WASI targets, and a standalone Preview 2 uucore --features fs check.

…ries have none)

…wasip2

…encoded-bytes conversions)

…nd wasip2

Copilot

🟡 Changes recommended

Add WASI Preview 2 coverage and regression tests for the new conversion and empty-argv paths.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Enables standalone wasm32-wasip2 builds for uucore and env, while preventing empty-WASI-argv traps.

Changes:

Enables rustix/std for uucore.
Adds WASI Preview 2 encoded-byte conversions for env.
Restores env to common/WASI features and adds an empty-argv fallback.

File summaries

File	Description
`src/uucore/src/lib/lib.rs`	Prevents empty-argv indexing on WASI.
`src/uucore/Cargo.toml`	Enables required rustix standard-library APIs.
`src/uu/env/src/native_int_str.rs`	Adds WASI Preview 2 string conversions.
`Cargo.toml`	Includes `env` in common/WASI features.

Review details

Suppressed comments (1)

Cargo.toml:140

This move does not provide the wasip2 CI coverage claimed in the PR description: .github/workflows/wasi.yml gates the feat_wasm build to wasip1 and excludes env from the wasip2 package-test list. Add a wasip2 compile/test step or correct the description before relying on this feature move for regression protection.

  "env",

Files reviewed: 4/4 changed files
Comments generated: 2
Review effort level: Lite

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

sylvestre · 2026-09-19T09:30:15Z

please address what copilot said

Run env encoded-byte conversion tests under wasm32-wasip2, and factor the WASI argv fallback so its empty-input behavior is covered on host and target tests.

Copilot

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

Review effort: Lite
Findings: 1 · 2

Open (3)

The first commit described in the PR—enabling rustix/std for the optional uucore dependency—is… · New
Please add a regression test for an empty WASI argv. This branch is the only protection against the…
The new wasip2 conversion path is not exercised by the current tests: env is excluded from the…

  "dircolors",
  "dirname",
  "echo",
+  "env",


Copilot

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

Review effort: Lite
Findings: 1 · 2

Open (3)

The first commit described in the PR—enabling rustix/std for the optional uucore dependency—is…
Please add a regression test for an empty WASI argv. This branch is the only protection against the…
The new wasip2 conversion path is not exercised by the current tests: env is excluded from the…

sylvestre · 2026-09-19T20:44:35Z

i am sorry but nobody will read such a long comment 0
Please make it shorter

sylvestre · 2026-09-19T20:45:26Z

+// On wasm32-wasip2 `std::os::wasi::ffi` is behind the unstable `wasip2` library feature, so the
+// byte views come from the stable encoded-bytes API instead. The component model defines every
+// string the host hands a guest as UTF-8, so on this target the encoded bytes *are* the bytes, and
+// the reverse direction can go through `str` without `unsafe` — the same approach as
+// `uucore::os_str_from_bytes` / `os_string_from_vec` on this target.


please make this comment shorter

sylvestre · 2026-09-19T20:45:34Z

+    argv
+}
+
+// `std::env::args_os()` can be empty on wasi when the component is not invoked as a CLI


please make this comment shorter

github-actions · 2026-09-20T04:35:44Z

GNU testsuite comparison:

Skip an intermittent issue tests/date/resolution (fails in this run but passes in the 'main' branch)

Copilot

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

Review effort: Lite
Findings: 2 · 2

Open (4)

The PR description says commit 1 enables the std feature on uucore's optional rustix… · New
The first commit described in the PR—enabling rustix/std for the optional uucore dependency—is…
Please add a regression test for an empty WASI argv. This branch is the only protection against the…
The new wasip2 conversion path is not exercised by the current tests: env is excluded from the…

  "dircolors",
  "dirname",
  "echo",
+  "env",


Copilot

Copilot review overview

🟡 Changes recommended

The new Preview 2 CI step does not exercise the standalone uucore --features fs build that motivated the fix.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 2 · 1

Open (3)

The PR description says commit 1 enables the std feature on uucore's optional rustix…
The first commit described in the PR—enabling rustix/std for the optional uucore dependency—is…
Add standalone uucore fs feature build check · New

Resolved since last review (2)

Please add a regression test for an empty WASI argv. This branch is the only protection against the…
The new wasip2 conversion path is not exercised by the current tests: env is excluded from the…

Copilot

Copilot review overview

🟢 Approval recommended

The changes are scoped, tested, and align with the stated WASI Preview 2 build and runtime fixes.

Review effort: Lite
Findings: 2

Open (2)

The PR description says commit 1 enables the std feature on uucore's optional rustix…
The first commit described in the PR—enabling rustix/std for the optional uucore dependency—is…

Resolved since last review (1)

Add standalone uucore fs feature build check

sylvestre · 2026-09-21T17:19:02Z

+      run: |
+        cargo test --target wasm32-wasip2 --no-default-features -p uu_env
+        cargo check --target wasm32-wasip2 --no-default-features -p uucore --features fs
+        cargo test --target wasm32-wasip2 --no-default-features -p uucore --lib \


a dedicated CI step to run one test on a 4 lines helper seems a lot, the cargo check lines are the useful part here, no?

sylvestre · 2026-09-21T17:19:27Z

+            match String::from_utf8(vec) {
+                Ok(s) => s.into(),
+                // Cannot have come from the host; keep going rather than abort the component.
+                Err(e) => String::from_utf8_lossy(e.as_bytes()).into_owned().into(),


this silently replaces bytes with U+FFFD, so env would hand a corrupted value to the child. i'd rather keep the bytes than mangle them. also the comment doesn't really explain the why.

sylvestre · 2026-09-21T17:19:41Z

+#[cfg(any(test, target_os = "wasi"))]
+fn with_wasi_argv_fallback(mut argv: Vec<OsString>) -> Vec<OsString> {
+    if argv.is_empty() {
+        argv.push(OsString::from("uu"));


why "uu"? it shows up in error messages and --version, "coreutils" would be less cryptic.

Copilot

Copilot review overview

🟡 Changes recommended

Add the requested SAFETY comments before both unchecked conversions.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 2 · 1

Open (3)

The PR description says commit 1 enables the std feature on uucore's optional rustix…
The first commit described in the PR—enabling rustix/std for the optional uucore dependency—is…
Document safety rationale for unchecked OsStr conversion · New

codspeed · 2026-09-23T13:44:43Z

Merging this PR will regress 1 benchmark

⚠️

122 benchmarks spent significant time in system calls

System calls cannot be consistently instrumented, so they are not included in the measure, which understates the real cost. Please switch to the Walltime instrument to accurately measure system calls.

Measurement and system calls

cksum_multiple_files

du_wide_tree[(5000, 500)]

df_deep_directory

cksum_raw_output

cksum_crc

cksum_default

du_deep_tree[(100, 3)]

dd_copy_4k_blocks

du_all_wide_tree[(5000, 500)]

dd_copy_partial

…and 112 more

ℹ️ 1 benchmark has no flame graph

The flamegraph could not be generated because too much time was spent in runtime internals, making the visualization not useful for performance analysis.

sort_dictionary_order[500000]

⚡ 1 improved benchmark
❌ 1 regressed benchmark
✅ 399 untouched benchmarks
⏩ 70 skipped benchmarks¹

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

	Mode	Benchmark	`BASE`	`HEAD`	Efficiency
❌	Simulation	`factor_five_38_bit_primes`	1.8 s	2.1 s	-14.88%
⚡	Simulation	`factor_three_39_bit_primes`	942.2 ms	529.5 ms	+77.97%

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.

_{Comparing Aditya1404Sal:wasip2-fixes (b234415) with main (774cca5)²}

70 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
No successful run was found on main (fb68e6c) during the generation of this report, so 774cca5 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩

Copilot

Copilot review overview

🔵 Needs a closer look

Add the requested WASI-target regression test for the public argv path.

Review effort: Lite
Findings: 2

Open (2)

The PR description says commit 1 enables the std feature on uucore's optional rustix…
The first commit described in the PR—enabling rustix/std for the optional uucore dependency—is…

Resolved since last review (1)

Document safety rationale for unchecked OsStr conversion

eduardomourar

Just very small suggestions.

eduardomourar · 2026-10-09T21:09:45Z

 static UTIL_NAME: LazyLock<String> = LazyLock::new(|| {
-    let base_index = usize::from(get_utility_is_second_arg());
+    // `ARGV` may be shorter than the multicall layout assumes (e.g. the WASI fallback).
+    let last = ARGV.len().saturating_sub(1);
+    let base_index = usize::from(get_utility_is_second_arg()).min(last);
    let is_man = usize::from(ARGV[base_index].eq("manpage"));
-    let argv_index = base_index + is_man;
+    let argv_index = (base_index + is_man).min(last);


Previous fixes to this indexing (e.g. 6699eaad6) corrected the arithmetic to be exactly right rather than clamping around it. The .min(last) here means that if get_utility_is_second_arg() is true but ARGV only has one element (the WASI empty-argv fallback), base_index silently falls back to 0 instead of the intended 1 — i.e. a wrong-but-plausible value instead of a panic.

In the current call graph this looks unreachable: coreutils.rs's multicall path only sets get_utility_is_second_arg() after consuming an argv entry, so by the time it's true, ARGV already has ≥2 elements and the clamp is a no-op. But that's an invariant of the caller, not something enforced where the clamp lives — if that assumption ever changes, this would silently produce a misleading utility name instead of failing loudly. Would a comment noting why the clamp is always a no-op in practice (or asserting it) be worth adding here?

Went one step further and removed the clamp instead of documenting it, since it was dead code. UTIL_NAME is only read inside a utility's uumain, which the multicall path reaches only after consuming argv[1]; with the flag set and no second argument it exits with "missing argument" without reading the name. The indexing is back to upstream's exact arithmetic.

eduardomourar · 2026-10-09T21:13:53Z

 // args_os() can be expensive to call, it copies all of argv before iterating.
 // So if we want only the first arg or so it's overkill. We cache it.
 #[cfg(windows)]
 static ARGV: LazyLock<Vec<OsString>> = LazyLock::new(|| wild::args_os().collect());
-#[cfg(not(windows))]
+#[cfg(all(not(windows), not(target_os = "wasi")))]
 static ARGV: LazyLock<Vec<OsString>> = LazyLock::new(|| std::env::args_os().collect());

+#[cfg(any(test, target_os = "wasi"))]
+fn with_wasi_argv_fallback(mut argv: Vec<OsString>) -> Vec<OsString> {
+    if argv.is_empty() {
+        argv.push(OsString::from("coreutils"));
+    }
+    argv
+}
+
+// WASI embeddings may omit argv; provide a stable utility-name fallback.
+#[cfg(all(not(windows), target_os = "wasi"))]
+static ARGV: LazyLock<Vec<OsString>> =
+    LazyLock::new(|| with_wasi_argv_fallback(std::env::args_os().collect()));


with_wasi_argv_fallback guarantees ARGV is never empty on target_os = "wasi", which is what makes the existing unguarded ARGV[0] access in EXECUTION_PHRASE (L423-L433) safe on WASI too — a nice side benefit. But that "ARGV is always non-empty" guarantee isn't stated anywhere near the ARGV static itself, so a future change to with_wasi_argv_fallback (or a new platform cfg arm) could reintroduce the panic in EXECUTION_PHRASE or UTIL_NAME without anything pointing back at this invariant. A one-line comment on the ARGV static ("always non-empty — relied on by UTIL_NAME/EXECUTION_PHRASE") would make that dependency explicit for the next person touching this code.

Copilot

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

6 open findings

with_wasi_argv_fallback guarantees ARGV is non-empty, but UTIL_NAME still indexes… · New
The PR description says commit 1 enables the std feature on uucore's optional rustix…
The first commit described in the PR—enabling rustix/std for the optional uucore dependency—is…
The Preview 2 path is selected via not(target_env = \"p1\"), which will also match any WASI… · New
The Preview 2 path is selected via not(target_env = \"p1\"), which will also match any WASI… · New
The fallback value \"coreutils\" is a hard-coded magic string that becomes observable in… · New

🧠 Review effort: Lite

sylvestre · 2026-10-10T06:50:03Z

+}
+
+// Always non-empty on WASI (embeddings may omit argv); `UTIL_NAME`/`EXECUTION_PHRASE` rely on it.
+#[cfg(all(not(windows), target_os = "wasi"))]


not(windows) is redundant here, target_os = "wasi" is enough
can be done in a different PR

Copilot

🟢 Approval recommended

The targeted changes have regression tests and WASI CI coverage, with no unresolved findings.

2 open findings

The PR description says commit 1 enables the std feature on uucore's optional rustix…
The first commit described in the PR—enabling rustix/std for the optional uucore dependency—is…

4 resolved since last review

with_wasi_argv_fallback guarantees ARGV is non-empty, but UTIL_NAME still indexes…
The fallback value \"coreutils\" is a hard-coded magic string that becomes observable in…
The Preview 2 path is selected via not(target_env = \"p1\"), which will also match any WASI…
The Preview 2 path is selected via not(target_env = \"p1\"), which will also match any WASI…

🧠 Review effort: Lite

sylvestre · 2026-10-13T07:41:18Z

+
+// `std::os::wasi::ffi` is still unstable on WASI Preview 2, so provide the same traits here.
+// On WASI an `OsStr` is a plain byte string, so these conversions keep every byte.
+#[cfg(all(target_os = "wasi", not(target_env = "p1")))]


uucore's os_str_from_bytes/os_string_from_vec still reject non-UTF-8 on p2, so env and uucore now disagree.
could this shim live in uucore so both use it? can be done in a different PR

thats a fair point, I can migrate that in a follow-up PR very soon (today?)

Aditya1404Sal added 4 commits September 14, 2026 20:10

uucore: don't index an empty argv on wasip2 (guests embedded as libra…

0da5724

…ries have none)

uucore: enable rustix/std so its fs and io modules compile on wasm32-…

d61c139

…wasip2

env: build on wasm32-wasip2 without the unstable std::os::wasi::ffi (…

336894b

…encoded-bytes conversions)

env: return to feat_common_core now that it builds on wasm32-wasip1 a…

c4058d7

…nd wasip2

Aditya1404Sal force-pushed the wasip2-fixes branch from b7b4796 to c4058d7 Compare September 14, 2026 14:52

Aditya1404Sal marked this pull request as ready for review September 14, 2026 19:20

Copilot AI lite review requested due to automatic review settings September 14, 2026 19:20

Copilot started reviewing on behalf of Aditya1404Sal September 14, 2026 19:20

View session

Copilot AI reviewed Sep 14, 2026

View reviewed changes

Comment thread src/uu/env/src/native_int_str.rs

Comment thread src/uucore/src/lib/lib.rs Outdated

test(wasi): cover p2 conversions and empty argv

17db430

Run env encoded-byte conversion tests under wasm32-wasip2, and factor the WASI argv fallback so its empty-input behavior is covered on host and target tests.

Copilot AI review requested due to automatic review settings September 19, 2026 19:56

Merge upstream/main into wasip2-fixes

92ad886

Copilot AI reviewed Sep 19, 2026

View reviewed changes

Comment thread Cargo.toml

"dircolors",

"dirname",

"echo",

"env",

Copilot AI review requested due to automatic review settings September 19, 2026 20:14

Copilot AI reviewed Sep 19, 2026

View reviewed changes

sylvestre reviewed Sep 19, 2026

View reviewed changes

Copilot started reviewing on behalf of Aditya1404Sal September 19, 2026 23:32

View session

Copilot started reviewing on behalf of Aditya1404Sal September 19, 2026 23:38

View session

comment reduction

d0cde09

Copilot AI review requested due to automatic review settings September 20, 2026 11:38

Copilot AI reviewed Sep 20, 2026

View reviewed changes

Comment thread Cargo.toml

"dircolors",

"dirname",

"echo",

"env",

Copilot started reviewing on behalf of Aditya1404Sal September 20, 2026 12:47

View session

Merge upstream/main into wasip2-fixes

c1718a3

Copilot AI review requested due to automatic review settings September 21, 2026 07:06

Copilot started reviewing on behalf of Aditya1404Sal September 21, 2026 07:13

View session

Copilot AI reviewed Sep 21, 2026

View reviewed changes

Comment thread .github/workflows/wasi.yml Outdated

add fs feature ci check

abb54ec

Copilot AI review requested due to automatic review settings September 21, 2026 07:29

Copilot started reviewing on behalf of Aditya1404Sal September 21, 2026 07:43

View session

Copilot AI reviewed Sep 21, 2026

View reviewed changes

sylvestre reviewed Sep 21, 2026

View reviewed changes

review fixes

b2305fe

Copilot AI review requested due to automatic review settings September 23, 2026 12:52

Copilot started reviewing on behalf of Aditya1404Sal September 23, 2026 12:53

View session

Copilot AI reviewed Sep 23, 2026

View reviewed changes

Comment thread src/uu/env/src/native_int_str.rs

safety comment nits

49d9a86

Copilot AI review requested due to automatic review settings September 23, 2026 14:29

Copilot started reviewing on behalf of Aditya1404Sal September 23, 2026 14:31

View session

Copilot AI reviewed Sep 23, 2026

View reviewed changes

eduardomourar suggested changes Oct 9, 2026

View reviewed changes

Aditya1404Sal added 2 commits October 10, 2026 03:24

Merge upstream/main into wasip2-fixes

00f1914

review fixes: drop the argv clamp, document the non-empty invariant

2e54a2b

Copilot AI lite review requested due to automatic review settings October 9, 2026 22:07

Copilot AI reviewed Oct 9, 2026

View reviewed changes

Comment thread src/uucore/src/lib/lib.rs

Comment thread src/uu/env/src/native_int_str.rs

Comment thread src/uu/env/src/native_int_str.rs

Comment thread src/uucore/src/lib/lib.rs

Copilot stopped reviewing on behalf of Aditya1404Sal due to an error October 9, 2026 23:10
An unexpected error occurred. For more details, see the detailed logs in GitHub Actions.

sylvestre reviewed Oct 10, 2026

View reviewed changes

Aditya1404Sal added 2 commits October 13, 2026 03:09

Merge upstream/main into wasip2-fixes

a846d10

uucore: drop the redundant not(windows) from the wasi ARGV cfg

b234415

Copilot AI lite review requested due to automatic review settings October 12, 2026 21:42

Copilot started reviewing on behalf of Aditya1404Sal October 12, 2026 21:43

View session

Copilot AI reviewed Oct 12, 2026

View reviewed changes

sylvestre reviewed Oct 13, 2026

View reviewed changes

Conversation

Aditya1404Sal commented Sep 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (1)

Uh oh!

Uh oh!

Uh oh!

sylvestre commented Sep 19, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

Uh oh!

sylvestre commented Sep 19, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Sep 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟢 Approval recommended

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

codspeed Bot commented Sep 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging this PR will regress 1 benchmark

Performance Changes

Footnotes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🔵 Needs a closer look

Uh oh!

eduardomourar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

Aditya1404Sal commented Sep 14, 2026 •

edited

Loading

github-actions Bot commented Sep 20, 2026 •

edited

Loading

codspeed Bot commented Sep 23, 2026 •

edited

Loading