Fix: claude bash-chain-guard quoting fix and split error messages - #193
Conversation
|
Warning Review limit reached
Next review available in: 42 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe Bash hook now removes quoted strings and escaped characters before scanning commands. It blocks chaining operators and standalone backgrounding while allowing supported redirection forms. It reports each violation type with a specific remediation message. ChangesBash command validation
Estimated code review effort: 3 (Moderate) | ~15–30 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/hooks/bash-chain-guard.js:
- Around line 12-13: Update stripQuotedAndEscaped so each removed quoted or
escaped span is replaced with whitespace or another non-operator sentinel rather
than deleted, preserving token separators and preventing adjacent shell
operators from becoming standalone. Add regression tests covering redirection
followed by an escaped character and by a quoted character before &, ensuring
BACKGROUND_AMPERSAND does not allow either form.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4290d225-0de2-43d6-8ec8-386f5079ed6b
📒 Files selected for processing (1)
.claude/hooks/bash-chain-guard.js
stripQuotedAndEscaped deleted quoted/escaped spans, which could join previously-separate shell tokens. `printf hi >\x&` collapsed to `printf hi >&`, so the BACKGROUND_AMPERSAND lookbehind read the `&` as part of a redirection and let a real backgrounding operator bypass the guard. Replace each stripped span with a single space instead of deleting it, keeping tokens separated. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Why is this change necessary?
The chain-detection regex was
/\s(&&|\|\||;)\s|\s&(\s|$)/, which requires whitespace on both sides of the operator. That makes it both too narrow and too broad, and every repo on this template has the same holes today.Missed (chaining that should be blocked, but ran through):
cd /some/path; git diff— no space before the semicoloncd a&&ls— no spaces at allFalse-positived (legitimate single commands that got blocked):
find . -name x -exec rm {} \;— the trailing\;offind -execcat x.json | jq -r ".a; .b"— a semicolon inside a string literalThe regex never accounted for shell quoting, so it could not tell a chain operator from the same character inside a quoted or escaped span.
How does this change address the issue?
Strip quoted spans and backslash-escapes first, then test what remains:
Because the operators are matched against the stripped string, they no longer need whitespace anchors — which is what fixes the missed cases.
Chaining and backgrounding are also split into two separate checks so each error message names its own fix. The backgrounding message now points at the Bash tool's
run_in_backgroundoption, which the single combined message could not do.BACKGROUND_AMPERSANDuses lookaround so2>&1and&>fileare treated as redirections rather than backgrounding.What side effects does this change have?
cd x; yforms that used to run.find -exec ... \;and quoted semicolons stop being blocked, so anything that had worked around the false positive no longer needs to.|) remain allowed, unchanged.How is this change tested?
Smoke-tested by piping hook-shaped JSON into the real hook and checking the exit code:
cd /some/path; git diffcd a&&lspnpm dev &run_in_backgroundfind . -name x -exec rm {} \;cat x.json | jq -r ".a; .b"uv run pytest tests/unit 2>&1 | tail -20Invocation used:
Also, I played around with it for a while in a downstream repo
Summary by CodeRabbit