fix: avoid dev-hook false positives in non-dev commands#356
fix: avoid dev-hook false positives in non-dev commands#356tsubasakong wants to merge 3 commits intoaffaan-m:mainfrom
Conversation
📝 WalkthroughWalkthroughToken-level parsing is added to the dev-server-block hook: it extracts the leading command word of each shell segment (skipping recognized prefixes) and only applies the dev-pattern/tmux checks when that leading word is a known package-manager command, preventing false positives from heredoc or prose content. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="scripts/hooks/pre-bash-dev-server-block.js">
<violation number="1" location="scripts/hooks/pre-bash-dev-server-block.js:111">
P2: Wrapper option tokens are treated as the command word, which lets `env`/`sudo`-prefixed dev commands bypass blocking.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/hooks/pre-bash-dev-server-block.js`:
- Around line 20-21: The parsing currently lets env -S/--split-string bypass
because the split-string argument (the following token) is ignored; update the
parser so PREFIX_OPTION_VALUE_WORDS handling treats '-S' and '--split-string' as
options that consume the next token as the command string and skips that
consumed token when computing the leading command; modify getLeadingCommandWord
to recognize and advance past the split-string value (same change for the other
occurrences referenced around lines 128-133) so env -S "npm run dev" yields
"npm" as the leading command.
- Around line 13-17: When a wrapper command from DEV_COMMAND_WORDS is detected,
we should not run devPattern against the entire raw segment because quoted
arguments (e.g., bash -lc 'echo "npm run dev"' or tmux display-message "npm run
dev") cause false positives; instead, extract the inner command string passed to
the wrapper (strip wrapper name and common flags like -c, -lc, --, and
tmux/display-message argument wrappers) or skip quoted/heredoc sections, then
run devPattern only on that extracted/unquoted command; update the logic that
currently calls devPattern on the full raw segment so it first detects wrapper
commands in DEV_COMMAND_WORDS, extracts the actual child command, unquotes or
removes heredoc content, and only then applies devPattern to avoid
wrapper-induced false positives.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d7146ffb-9e1e-4191-b02e-e608fa2114cd
📒 Files selected for processing (2)
scripts/hooks/pre-bash-dev-server-block.jstests/integration/hooks.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/integration/hooks.test.js
| 'bash', | ||
| 'sh', | ||
| 'zsh', | ||
| 'fish', | ||
| 'tmux' |
There was a problem hiding this comment.
Wrapper commands can still false-positive on quoted text.
Because bash, sh, zsh, fish, and tmux are in DEV_COMMAND_WORDS, Line 165 still runs devPattern on the full raw segment after the wrapper is identified. Cases like bash -lc 'echo "npm run dev"' or tmux display-message "npm run dev" will still be blocked even though they never start a dev server, so the quoted/heredoc false-positive path remains for wrapper commands.
Also applies to: 160-166
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/hooks/pre-bash-dev-server-block.js` around lines 13 - 17, When a
wrapper command from DEV_COMMAND_WORDS is detected, we should not run devPattern
against the entire raw segment because quoted arguments (e.g., bash -lc 'echo
"npm run dev"' or tmux display-message "npm run dev") cause false positives;
instead, extract the inner command string passed to the wrapper (strip wrapper
name and common flags like -c, -lc, --, and tmux/display-message argument
wrappers) or skip quoted/heredoc sections, then run devPattern only on that
extracted/unquoted command; update the logic that currently calls devPattern on
the full raw segment so it first detects wrapper commands in DEV_COMMAND_WORDS,
extracts the actual child command, unquotes or removes heredoc content, and only
then applies devPattern to avoid wrapper-induced false positives.
| const PREFIX_OPTION_VALUE_WORDS = { | ||
| env: new Set(['-u', '-C', '-S', '--unset', '--chdir', '--split-string']), |
There was a problem hiding this comment.
env -S currently creates a bypass.
-S / --split-string hold the command string that env will execute, but this path skips that value entirely. env -S "npm run dev" therefore returns null from getLeadingCommandWord() and never reaches the blocker, which is the opposite of the wrapper support this PR is adding.
💡 Minimal direction
if (activeWrapper && isOptionToken(token)) {
if (shouldSkipOptionValue(activeWrapper, token)) {
+ if (activeWrapper === 'env' && (token === '-S' || token === '--split-string')) {
+ const value = readToken(segment, index);
+ return value ? getLeadingCommandWord(value.token) : null;
+ }
skipNextValue = true;
}
continue;
}Also applies to: 128-133
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/hooks/pre-bash-dev-server-block.js` around lines 20 - 21, The parsing
currently lets env -S/--split-string bypass because the split-string argument
(the following token) is ignored; update the parser so PREFIX_OPTION_VALUE_WORDS
handling treats '-S' and '--split-string' as options that consume the next token
as the command string and skips that consumed token when computing the leading
command; modify getLeadingCommandWord to recognize and advance past the
split-string value (same change for the other occurrences referenced around
lines 128-133) so env -S "npm run dev" yields "npm" as the leading command.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
|
Hey @tsubasakong — this has merge conflicts now. Could you rebase when you get a chance? Thanks! |
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
2 participants
Description
env npm run devwhile allowing commands likegh pr createthat mention dev commands inside quoted or heredoc textFixes #352
Validation:
Type of Change
fix:Bug fixfeat:New featurerefactor:Code refactoringdocs:Documentationtest:Testschore:Maintenance/toolingci:CI/CD changesChecklist
node tests/run-all.js)