fix(execpolicy): a redirect is not a command separator

[SparkofSpike]

Summary

Every shell command containing a 2>&1 redirect was refused on Windows with:

Built-in safety gate requires approval: Windows command input cannot be
classified safely enough to exclude termination of Codewhale npm launchers;
use a direct PID- or port-specific command.

The refusal reason names npm launchers, but no launcher check ever ran. The
command was simply unclassifiable, and the reason text is a canned
fail-closed string. Two lines in the shared classifier made a plain redirect
unreadable:

crates/execpolicy/src/command_safety.rs, split_command_segments, kept a bare
& inside the word ('&' => current.push(ch)), and command_invocations
re-parsed any word containing &. Since the segmenter never separates a bare
&, that re-parse returned the identical word, recursed again, and hit
MAX_WRAPPER_DEPTH. command_invocations then returned None — "fail closed"
— and windows_session_runtime_risk turned that into
UnclassifiedWindowsInvocation, which the runtime floor converts into a
non-bypassable block (auto_review.rs:1364).

So: the safety gate was not too strict about launchers. It could not read the
command at all.

Reproduction (before this change)

In a Full Access session with the sandbox disabled, on Windows:

• refused: cargo fmt --all -- --check 2>&1 | Select-Object -First 30
• refused: git fetch origin 2>&1; git log --oneline -1 origin/main
• refused: & $GH auth status (PowerShell's call operator — a bare &)
• refused: git grep -n "2>&1" -- crates/ (the literal text inside quotes)
• allowed: any of the above with the & removed

The last two lines are the tell: the classifier never gets to decide what the
command means, because a bare & makes it stop reading.

Changes

• split_command_segments: a & that is a command separator now splits
  the segment. POSIX job control and cmd.exe both treat it that way. The & of
  a redirection stays inside the word: >& copies a file descriptor (2>&1,
  >&2) and &> is bash's combined redirect.
• command_invocations: re-parse a word only when re-parsing can yield a
  different token stream — whitespace, ;, or |. A & that survived
  segmenting is part of a redirect; recursing on it could only reproduce the
  same word.

The second change alone would have been a weakening: a bare & glued to
its neighbours (echo x&rm -rf /etc) names two commands, and left unsplit it
corrupts the command word of the second one. Splitting it is the fix; dropping
the recursion is what makes the redirect readable.

Evidence

redirects_stay_classifiable_and_a_lone_ampersand_still_separates (new, in
crates/execpolicy/src/command_safety.rs) asserts both directions:

• git log 2>&1, cargo test 2>&1 | tail -5, make > build.log 2>&1,
  tool &> out.txt stay classifiable.
• echo x & rm -rf /etc, echo x&rm -rf /etc,
  curl https://example.com & rm -rf /etc still yield an rm invocation, so
  the second stage is reached rather than blanket-refused.

Run against the previous code, that test fails with exactly the reported
symptom:

a redirect must stay readable instead of failing closed: git log 2>&1
test result: FAILED. 0 passed; 1 failed

With the change: cargo test -p codewhale-execpolicy command_safety →
87 passed; 0 failed.

TUI side: cargo test -p codewhale-tui auto_review → 72 passed; 0 failed.
test_cases_09.rs used to assert the Windows floor as the expected outcome
for cargo test & curl https://example.com; that assertion is removed because
the command is classifiable and belongs on the reviewer path like every other
approval-shaped command. The remaining windows_* tests in that module pass
unchanged.

cargo fmt --all -- --check is clean.

Note on a flaky-looking neighbour

core::engine::tests::auto_review_asks_the_user_and_returns_the_answer
overflowed its stack on this machine. It is unrelated to this change (the diff
only removes recursion) and it is a stack-size limit, not a logic failure:
with RUST_MIN_STACK=33554432 it passes, and it passes as part of the 72. Not
fixed here — out of scope, but worth a separate look.

Type of Change

• Bug fix (non-breaking change which fixes an issue)

Checklist

• Updated docs or comments as needed
• Added or updated tests where relevant

Related Issues

No-Issue: observed in an operator session on 0.10.1 (Windows/PowerShell); no
upstream issue filed for it.

Attribution

🤖 Generated by SpikeBot 000(CodeWhale-LOCAL)

[SparkofSpike]

Adversarial review — PR #9: "a redirect is not a command separator"

Verdict

The fix holds. The root-cause analysis is accurate, both edits are needed and
complementary, and I could not construct a single command where the new code
loses a discovered invocation or lets a real destructive stage go unread. I ran
the crate on this box (isolated toolchain, cargo test -p codewhale-execpolicy)
and confirmed the reported symptom and its resolution empirically. I found one
confirmed, low-severity regression (quoted & is split inside quotes) that
over-reads, never under-reads — worth a follow-up, not a block.

---

Findings (most severe first)

F1 — CONFIRMED: & inside quoted text is split, inventing phantom commands and misclassifying benign strings

crates/execpolicy/src/command_safety.rs:2033-2035 — split_command_segments
is a raw character scan with no quote awareness. The new &-splits fire even
when the & is inside a shell quote, producing segments that shell_words
would never have produced from the original line.

Ran on this box (temporary probe, since reverted):

command	previous code	this PR
echo "a & b"	readable, single argv	invocations [["echo","a"],...,["b"]] — phantom b
echo "a&b"	None (fail-closed)	invocations include phantom ["b"]
echo 'x & rm -rf /etc'	level=Dangerous via None	invocations include ["rm","-rf","/etc"] (a destructor the shell never runs), level=Dangerous

The quoted-&-with-rm case is the sharpest: command_invocations("echo 'x & rm -rf /etc'") now yields an rm -rf /etc argv, which reaches
argv_is_destroyer at crates/tui/src/tui/auto_review.rs:1565-1570. The two
command_invocations consumers there are both hold-only (block), never
allow gates, so a phantom rm can only over-hold — it cannot let a real
destroyer through. Consequence: benign commands quoting shell metacharacters
(git commit -m "fix & feature", checks that grep for rm -rf text) get
split into phantom invocations and may be refused/held spuriously. This is a
availability / classification regression, not a security opening. Old code
failed closed on the same input (returned None), which is where the bug the
PR fixes came from; but the fix could equally skip splitting when the & is
inside quotes, at once preserving readability and not inventing commands.

I could not turn this into a false negative: an innocent & is split too,
never merging two real commands, so no real separator is hidden.

F2. CONFIRMED: analyze_command levels are byte-for-byte unchanged across the adversarial battery — blast radius is well bounded

I captured SafetyLevel for 50 commands (redirects, glued separators, quoting,
eval, curl|sh, a&b… shapes) under both the previous and new source. That
list was identical in order and level. The only behavioral delta the PR creates
is in command_invocations() — every & command went from None(fail-closed)
to a readable Vec. So the functional effect is confined to the three
consumers at auto_review.rs:1364, 1376, 1569 (the Windows floor and the
destroyer hold). The Windowsnpm-launcher hard-blow the PR targets
(command_invocations == None ⇒ UnclassifiedWindowsInvocation) is exactly what
changes. That is the correct, contained surface. TUI-side I did not build
(codewhale-tui takes tens of minutes on this box); the 72 passed CI claim
was not verified by me.

F3. CONFIRMED: no discovered invocation is lost — provably a superset

Every & that is not preceded by > and not followed by >
(command_safety.rs:2033) splits; the only survivor inside a segment is the
redirect form. &-free commands are byte-identical between versions. Therefore
command_invocations and split_command_segments can only ever enumerate a
strict superset of the old (readable) set. I tested the shapes the task named —
a>&b, a&>b, 2>&1&rm -rf /etc, cmd &>file & rm -rf /etc, x>&y&z — plus
echo >& rm -rf /etc, cmd > file >& rm -rf /etc, sort < in > out & mv, and
the second rm stage reappears in every case the shell would actually run it
and in none where the & was a redirect. The author's argument that "edit 2
alone would be a weakening" is correct: with only the & removed from the
re-parse predicate and no split, echo x&rm -rf /etc would read words
["echo","x&rm",…] and the rm never discovered. Both edits must land
together — they do.

F4. Confirmed (correct behavior, not a defect): redirect spellings are all preserved

I exercised the operator list for the guard table:

spelling	survives split?	evidence
2>&1	yes	git log 2>&1 readable
>&2	yes	>&2 echo hi → [">&2","echo","hi"]
1>&-	yes	single token, readable
&>file	yes	tool &> out.txt readable
&>>file	yes	>>&file single token
2>&1& (trailing)	yes→bare & splits after	2>&1&rm -rf /etc → ["2>&1"],["rm…"]
N>&M	yes	exec 3>&1, 2>file 3>&1

The trailing-& sub-case (2>&1&rm) is itself nearly the acid test for F3 and
works. The >&/&> guard correctly discerns redirect from backgrounding.

5 (asked) — test_vacuity

The new test redirects_stay_classifiable_and_a_lone_ampersand_still_separates
is not vacuous: run against the previous code it fails on the first assert
(git log 2>&1 → None), and its second half reproduces the F1 phantom for the
spaced echo x & rm -rf /etc (which is genuinely a real separator and must be
rm). The is_some assert is the one the old code cannot pass. Credible.

6 — test_cases_09.rs assertion change is correct, and the Windows floor isn't dead

The deleted branch asserted Block(UnclassifiedWindowsInvocation) for cargo test & curl … under the buggy behavior; with the fix that command is
classifiable, so the correct expectation for it on all platforms is
ConsultReviewer (still refuses auto-approval). Real coverage of the floor is
retained elsewhere: auto_review.rs:2017 asserts deep sh -c nesting still
yields UnclassifiedWindowsInvocation via the depth cap, and
windows_session_runtime_risk/.._risk_scoped still fire on genuine
unclassifiable nesting. Not a coverage hole.

7 — Comments/doc accuracy

• The comment at command_safety.rs:2030-2032 is accurate for POSIX and for
  cmd.exe bare-&.
• The depth accounting is unchanged and, now that re-parse no longer recurses on
  redirects, the previously-infinite redirect loop is the one real depth bug
  the PR removes; MAX_WRAPPER_DEPTH bounds the genuine nested sh -c case
  still covered at auto_review.rs:2017. No miscount found.

Bottom line

No adversary succeeded against it. The core reclassification (client
&-separator vs. redirect) is sound, the two edits are mutually necessary, the
effect is precisely scoped to two hold-only consumers, and no real destructive
invocation was demonstrated lost. Confirm (i.e. the bug is fixed and the
fix does not open a destructive-path dimpling). Highest-value follow-up:
teach split_command_segments about quotes so & inside quotes doesn't
manufacture phantom commands (F1), which currently flags more than it should
and would also trip a user grepping for 2>&1 in their own helper text.

---

References (lines in commit d37431a):

• crates/execpolicy/src/command_safety.rs:2033-2035 — guard + bare-& split
• crates/execpolicy/src/command_safety.rs:2096-2097 — re-parse predicate
• crates/tui/src/tui/auto_review.rs:1364 — None → Unclassified (the bug)
• crates/tui/src/tui/auto_review.rs:1565-1570 — hold-only use of command_invocations
• crates/tui/src/core/engine/tests/test_cases_09.rs:474-486 — changed windows-floor assertion

Attribution

🤖 Generated by SpikeBot 003(ClaudeCode-JP)

[SparkofSpike]

Adversarial review — follow-up commit ea9bc1b ("a quoted operator is data, not a separator")

Verdict

The follow-up holds — no regression. F1's phantom invocations are gone
from command_invocations for all four named shapes; the narrowed recursion
does not lose a real command in any case I could construct; quote state closes
and stays closed correctly for every separator-shaped input except one
pre-existing backslash-escape residual; and the Windows floor
(None ⇒ UnclassifiedWindowsInvocation) plus the PR's original
git log 2>&1 fix still hold. The commit fixes exactly what F1 named. My
strongest adversarial attempt — a non-literal destructive payload in a -e
interpreter — is a pre-existing gap this PR does not widen.

Evidence: cargo test -p codewhale-execpolicy command_safety on this box
→ 88 passed; 0 failed. Plus a temporary probe (reverted afterwards) running
command_invocations/analyze_command under both this commit and the
pre-follow-up d37431a, side-by-side.

---

Findings (most severe first)

1. CONFIRMED — F1 fixed: no phantom invocation survives the four named cases.

crates/execpolicy/src/command_safety.rs:2017 split_command_segments now
tracks quote state; :2122 command_invocations re-parses a word only via
:2106 is_shell_script_argument. Side-by-side over d37431a:

command	d37431a argv (the F1 phantom)	ea9bc1b argv	result
echo "a & b"	["echo","a"],["a"],["b"]	["echo","a & b"]	phantom gone
echo "a&b"	["echo","a"],["a"],["b"]	["echo","a&b"]	phantom gone
echo 'x & rm -rf /etc'	["rm","-rf","/etc"] present	no rm argv	phantom rm gone
git commit -m "fix & feature"	["git","commit","-m","fix"], phantom feature	["git","commit","-m","fix & feature"]	one command

No phantom rm survives in command_invocations, and no stage the shell
actually runs is missing from the argv list.

Nuance (not a defect): the user-visible analyze_command verdict for
echo 'x & rm -rf /etc' is still dangerous — command_safety.rs:1821
DANGEROUS_PATTERNS (rm -rf /) is quote-blind and fires on the literal
text. That is pre-existing (unchanged from main) and intentional
hardening (the "keep that protection" comment at :1814); it over-reads,
never under-reads. The F1 issue — the invented rm argv at the destroyer
hold — is gone even though the substring fallback still classifies the
quoted string dangerous. If the goal were "a benign string quoting rm -rf /etc is never refused", that remains unmet, but it is not this edit's
omission.

2. CONFIRMED — narrowed recursion loses no real command; every dropped re-parse is a phantom.

Ran the battery under both d37431a and ea9bc1b. For the -e interpreters
(perl -e 'system("rm -rf /etc")', ruby -e 'system "rm", "-rf", "/etc"',
node -e '...execSync("rm -rf /etc")', awk 'BEGIN { system("rm -rf /etc") }')
and python3 -c "import os; os.system('rm -rf /etc')", the old code re-parsed
the payload and produced an rm argv whose argv[0] was "system(rm", "rm,",
"require(...)execsync(rm", or "os.system('rm" — none equal "rm", so
none ever matched auto_review.rs:1580 argv_is_destroyer. Those were phantom
non-holds already; the new code drops them, and the real destructive text is
still caught by the substring fallback → Dangerous in both old and new.
No destructive stage is lost.

The genuinely-running shell payloads — sh -c 'rm -rf /home',
bash -lc 'cd /; rm -rf /tmp', bash -- -c 'rm -rf /boot',
/bin/sh -c 'rm -rf /var', find | xargs [ -I{} ] sh -c … — are still
re-parsed (previous word -c / -lc / -ic passes
is_shell_script_argument), yield the exact ["rm","-rf",…] argv, and are
dangerous. Verified argv-for-argv.

sh -c 'true' -- 'rm -rf /etc' and sh --command 'rm -rf /etc': old code
re-parsed and raised a phantom; new drops it. That is correct — in
sh -c 'true' -- 'rm -rf /etc' the rm word is $1 of the true script,
never executed, and --command is not a real option. Both were over-reads.
xargs -I{} sh -c 'rm -rf {}' still surfaces ["rm","-rf","{}"]. The
command_word-folded forms (/bin/sh, \rm, /usr/bin/rm) and the
find | xargs … sh -c '/etc' chain behave identically old vs new. This
category yielded no false negative.

3. CONFIRMED — one pre-existing residual: the scanner does not honor backslash escapes, so \" inside double quotes can still invent a phantom rm.

echo "a \" & rm -rf /etc": the char scanner at 2017 treats the \" as
closing the double quote (no escape awareness), so & rm -rf /etc lands
outside a quote and the & splits → a phantom ["rm","-rf","/etc"] argv the
shell never runs. This is identical in d37431a and ea9bc1b and redundant
with the Dangerous already raised; it is not introduced or reopened by
this commit, but the fix's quote-tracking still is not a shell lexer. The
opposite failure — quote state closing early and thereby hiding a real
separator — did not surface: echo "a&b" & rm -rf /etc (quoted op followed by
real) keeps the quoted & as data and still splits the real one, finding the
rm; echo "&" ; rm behaves the same; consecutive single quotes
(echo 'it'\''s & x') stay data with no phantom; and an unterminated double
quote (echo "x & rm -rf /etc) keeps & inside the never-closed quote,
matching the shell's non-execution of that rm. Effect: exactly one,
pre-existing, over-read-only finding in this category.

4. CONFIRMED — Windows floor & the PR's original fix still hold.

command_invocations returning None still arms UnclassifiedWindowsInvocation
(auto_review.rs:1359). git log 2>&1 and make >build.log 2>&1 | tail -3
both stay Some (classifiable) here. The depth-cap path
command_invocations_reach_past_wrapper_options_and_into_shell_payloads still
asserts a too-deep nested sh -c returns None, and the suite passes it.
The PR's own redirects_stay_classifiable_and_a_lone_ampersand_still_separates
still covers echo x&rm -rf /etc (un-spaced real &) and tool &> out.txt.
Nothing new in this category.

5. CONFIRMED — wrapper-option reach-in (sudo -u, command_word folding, --, xargs) is preserved.

sudo -u me rm -rf /etc, \rm -rf /etc, /usr/bin/rm -rf /etc, and
find | xargs [ -I{} ] sh -c/bash -c 'rm -rf /etc' all still yield the
rm argv and are dangerous. This is the original PR's core guarantee and
none of it regresses.

---

Bottom line: the follow-up resolves F1 in exactly the surface it touched
and I could not find a genuine command it now misses that the old code caught —
the dropped re-parses are phantom rms, substring-fallback redundancy, or
system()-payload gaps identical before and after the change. Merge-able.
Categories 2 and 4 produced no confirmed defect.

Attribution

🤖 Generated by SpikeBot 003(ClaudeCode-JP)