[allanbatista]

What changed

• Detect native POSIX test expressions passed through rtk test, such as rtk test -d graphify-out.
• Route those expressions to the system test command via passthrough.
• Keep regular RTK test filtering intact for commands like rtk test cargo test.
• Add regression coverage and a changelog entry.

Problem

rtk test -d graphify-out failed with:

sh: 0: Illegal option -d

Running test -d graphify-out directly worked.

Root cause

test is also an RTK subcommand. Clap correctly routed rtk test -d graphify-out to Commands::Test, but that handler joined the remaining args into a string and executed it through sh -c for RTK's test-output filtering.

That produced sh -c "-d graphify-out". POSIX sh interpreted -d as an option to the shell command itself, not as an argument to test, which caused Illegal option -d.

Proposed solution

When Commands::Test receives a native test expression (-d, -f, !, (, etc.), bypass the RTK test-output wrapper and run the system test command directly with the original arguments. This keeps shell parsing out of the path and preserves the native test exit semantics.

Validation

• rtk cargo +1.91.0 fmt --all && rtk cargo +1.91.0 clippy --all-targets && rtk cargo +1.91.0 test --all
• rtk proxy ./target/debug/rtk test -d .
• rtk proxy ./target/debug/rtk test -d graphify-out
• rtk proxy ./target/debug/rtk test true

[CLAassistant]

All committers have signed the CLA.

[KuSh]

Round 1 of 3 — APPROVED, follow-up filed

Claim: rtk test should evaluate a native POSIX test expression (rtk test -d dir) instead of joining it into sh -c, where -d became a shell option and produced sh: 0: Illegal option -d.
Scope: accept. (round 1, frozen)
Ran: /usr/bin/test × 526 generated expressions × merge-base (39cbb96) vs head (b02b567), differential on exit codes; plus your three repros, --/--help/--version, missing-operand, -a/-o, the paren form, spaces in operands, an empty PATH, a test shim on PATH, and rtk rewrite. No savings figure applies — this routes to passthrough, and tracking honestly records 0%, so the 20% floor (CONTRIBUTING.md) is not in play.

Good fix, and the numbers are not close. rtk test <expr> against real test, same 526 cases: 411 mismatches → 212. 199 fixed.

The best of them is not the one in the title. rtk test ! -d <existing dir> returned exit 0 on develop — -d was a missing command, ! inverted its 127, and the answer came back "true" for a directory that plainly exists. Silent, and wrong. This returns 1.

Blocks merge (0)

Nothing outstanding — the one item I found is fixed in 98e66e0 below.

Fixups pushed

I pushed two commits rather than spending a round on them. Revert either if you disagree; nothing here is precious.

98e66e0 — ! and ( are shell syntax as well as test syntax. Matching them on the first token alone captured invocations the old path ran correctly:

                                develop   before fixup   after
rtk test ! false                   0           1           0     <- silent exit flip
rtk test ! cargo --version         1           2           1     <- stopped running cargo

rtk test ! false was the one that worried me: no output either way, just an exit code silently going 0 → 1. The fix makes ! and ( mark a native expression only when what they apply to is one:

match command.first().map(String::as_str) {
    Some("!") | Some("(") => is_native_test_expression(&command[1..]),
    Some(arg) => arg.starts_with('-'),
    None => false,
}

Same 199 fixes, still 212/526, shell-command forms back to their previous exit codes.

80e4568 — a routing test. Your two unit tests assert on is_native_test_expression directly, so they pin the predicate but not the wiring. I checked by mutation: inverting the if in the Commands::Test arm — which fully reintroduces sh: 0: Illegal option -d — left all 2210 tests green, and deleting the branch outright did too. tests/native_test_expression_test.rs runs the binary instead and fails on both mutations.

cargo fmt --all --check, cargo clippy --all-targets (0 warnings), cargo test --all (2212 + 5) green on the result, and on it merged onto current develop (d701ad96, clean merge).

Optional — will not hold merge; threads resolved by me

• resolved_command("test") resolves through PATH, so a test executable earlier on PATH runs instead of the utility (PATH=./shim:$PATH rtk test -d dir → my shim, exit 7). Only reachable on a path that was fully broken before, and it is RTK's standard passthrough mechanism.
• rtk test with no arguments exits 0 (sh -c ""), where real test exits 1. Identical on develop.
• The description says it adds "a changelog entry"; the diff has none — which is correct, since CONTRIBUTING.md says CHANGELOG.md belongs to release-please and must never be hand-edited. Only the sentence is wrong. Worth trimming so the next reader is not hunting for a file that should not exist.

Filed as follow-up

• #3932 — remove the rtk test subcommand and let native test fall through to RTK's existing passthrough. Measured: that takes the same 526 cases to 0/526, versus 212 here, because it is exact rather than a heuristic. It subsumes both gaps left in this PR — operand-first expressions (rtk test 1 -eq 1 → exit 127, 205 of the 212) and rtk test -h FILE (swallowed by clap's help flag, always 0) — and deletes the predicate entirely. It supersedes this PR rather than blocking it, which is why this is merging first.

Checked and correct — no need to re-verify

• Exit codes match /usr/bin/test for every unary operator, (, -a/-o, and the missing-operand form.
• Argument boundaries survive: rtk test -f "a file" works where develop failed. The passthrough passes argv, so nothing is re-split and nothing is shell-interpreted — strictly safer than the string it replaces.
• -- still routes to the test runner (rtk test -- cargo --version), so the escape hatch is intact.
• rtk test --version exits 0 with no output, which is exactly what /usr/bin/test --version does.
• No test on PATH: both builds exit 1 with a stderr diagnostic. No regression, including the Windows case where there is no test binary at all.
• Tracking records the passthrough at 0% saved. No inflated-savings problem.
• CI green on ubuntu, macos and windows.

Hypotheses built and dropped

• "The siblings need the same fix." Commands::Err, Summary and Run join argv into a shell string the same way — but none of them shadows a real utility, so there is no native invocation to detect. test is the only member of this class. Their shared quoting defect is #2985, and the else branch here is the same bug on a line this PR does not touch.
• "This is reachable through the hook, so the blast radius is large." No — rtk rewrite emits nothing for test -d foo, /usr/bin/test -d foo, [ -d foo ], or the compound and if forms. There is no test rule. Only a hand-typed rtk test reaches this.
• "Routing to /usr/bin/test breaks Windows." Checked by emptying PATH: both builds fail identically with exit 1 and a message. Not made worse.
• "rtk test --version now reports test's version instead of RTK's." No — real test --version prints nothing and exits 0; this matches it.

Your questions

None asked since you opened it — the only comment on the thread was the CLA bot. Thanks for the clear write-up; the root-cause section was accurate and saved me the diagnosis.

Next

Nothing — approving. 98e66e0 and 80e4568 are on your branch; revert either if you disagree with it.

Rounds: 1/3. Threads: 0 open, 3 resolved this round (fixup-closed included).