-
-
Notifications
You must be signed in to change notification settings - Fork 8.7k
fix: avoid dev-hook false positives in non-dev commands #356
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,140 @@ | |
| const MAX_STDIN = 1024 * 1024; | ||
| const { splitShellSegments } = require('../lib/shell-split'); | ||
|
|
||
| const DEV_COMMAND_WORDS = new Set([ | ||
| 'npm', | ||
| 'pnpm', | ||
| 'yarn', | ||
| 'bun', | ||
| 'npx', | ||
| 'bash', | ||
| 'sh', | ||
| 'zsh', | ||
| 'fish', | ||
| 'tmux' | ||
| ]); | ||
| const SKIPPABLE_PREFIX_WORDS = new Set(['env', 'command', 'builtin', 'exec', 'noglob', 'sudo']); | ||
| const PREFIX_OPTION_VALUE_WORDS = { | ||
| env: new Set(['-u', '-C', '-S', '--unset', '--chdir', '--split-string']), | ||
|
Comment on lines
+20
to
+21
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
💡 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 |
||
| sudo: new Set([ | ||
| '-u', | ||
| '-g', | ||
| '-h', | ||
| '-p', | ||
| '-r', | ||
| '-t', | ||
| '-C', | ||
| '--user', | ||
| '--group', | ||
| '--host', | ||
| '--prompt', | ||
| '--role', | ||
| '--type', | ||
| '--close-from' | ||
| ]) | ||
| }; | ||
|
|
||
| function readToken(input, startIndex) { | ||
| let index = startIndex; | ||
| while (index < input.length && /\s/.test(input[index])) index += 1; | ||
| if (index >= input.length) return null; | ||
|
|
||
| let token = ''; | ||
| let quote = null; | ||
|
|
||
| while (index < input.length) { | ||
| const ch = input[index]; | ||
| if (quote) { | ||
| if (ch === quote) { | ||
| quote = null; | ||
| index += 1; | ||
| continue; | ||
| } | ||
| if (ch === '\\' && quote === '"' && index + 1 < input.length) { | ||
| token += input[index + 1]; | ||
| index += 2; | ||
| continue; | ||
| } | ||
| token += ch; | ||
| index += 1; | ||
| continue; | ||
| } | ||
|
|
||
| if (ch === '"' || ch === "'") { | ||
| quote = ch; | ||
| index += 1; | ||
| continue; | ||
| } | ||
|
|
||
| if (/\s/.test(ch)) break; | ||
|
|
||
| if (ch === '\\' && index + 1 < input.length) { | ||
| token += input[index + 1]; | ||
| index += 2; | ||
| continue; | ||
| } | ||
|
|
||
| token += ch; | ||
| index += 1; | ||
| } | ||
|
|
||
| return { token, end: index }; | ||
| } | ||
|
|
||
| function shouldSkipOptionValue(wrapper, optionToken) { | ||
| if (!wrapper || !optionToken || optionToken.includes('=')) return false; | ||
| const optionSet = PREFIX_OPTION_VALUE_WORDS[wrapper]; | ||
| if (!optionSet) return false; | ||
| return optionSet.has(optionToken); | ||
| } | ||
|
|
||
| function isOptionToken(token) { | ||
| return token.startsWith('-') && token.length > 1; | ||
| } | ||
|
|
||
| function getLeadingCommandWord(segment) { | ||
| let index = 0; | ||
| let activeWrapper = null; | ||
| let skipNextValue = false; | ||
|
|
||
| while (index < segment.length) { | ||
| const parsed = readToken(segment, index); | ||
| if (!parsed) return null; | ||
| index = parsed.end; | ||
|
|
||
| const token = parsed.token; | ||
| if (!token) continue; | ||
|
|
||
| if (skipNextValue) { | ||
| skipNextValue = false; | ||
| continue; | ||
| } | ||
|
|
||
| if (token === '--') { | ||
| activeWrapper = null; | ||
| continue; | ||
| } | ||
|
|
||
| if (/^[A-Za-z_][A-Za-z0-9_]*=.*/.test(token)) continue; | ||
|
|
||
| if (SKIPPABLE_PREFIX_WORDS.has(token)) { | ||
| activeWrapper = token; | ||
| continue; | ||
| } | ||
|
|
||
| if (activeWrapper && isOptionToken(token)) { | ||
| if (shouldSkipOptionValue(activeWrapper, token)) { | ||
| skipNextValue = true; | ||
| } | ||
| continue; | ||
| } | ||
|
|
||
| return token; | ||
| } | ||
|
|
||
| return null; | ||
| } | ||
|
|
||
| let raw = ''; | ||
| process.stdin.setEncoding('utf8'); | ||
| process.stdin.on('data', chunk => { | ||
|
|
@@ -23,7 +157,13 @@ process.stdin.on('end', () => { | |
| const tmuxLauncher = /^\s*tmux\s+(new|new-session|new-window|split-window)\b/; | ||
| const devPattern = /\b(npm\s+run\s+dev|pnpm(?:\s+run)?\s+dev|yarn\s+dev|bun\s+run\s+dev)\b/; | ||
|
|
||
| const hasBlockedDev = segments.some(segment => devPattern.test(segment) && !tmuxLauncher.test(segment)); | ||
| const hasBlockedDev = segments.some(segment => { | ||
| const commandWord = getLeadingCommandWord(segment); | ||
| if (!commandWord || !DEV_COMMAND_WORDS.has(commandWord)) { | ||
| return false; | ||
| } | ||
| return devPattern.test(segment) && !tmuxLauncher.test(segment); | ||
| }); | ||
|
|
||
| if (hasBlockedDev) { | ||
| console.error('[Hook] BLOCKED: Dev server must run in tmux for log access'); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrapper commands can still false-positive on quoted text.
Because
bash,sh,zsh,fish, andtmuxare inDEV_COMMAND_WORDS, Line 165 still runsdevPatternon the full raw segment after the wrapper is identified. Cases likebash -lc 'echo "npm run dev"'ortmux 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