fix(dev): execute dev command with shell operators (&&, ||, etc.)#8234
fix(dev): execute dev command with shell operators (&&, ||, etc.)#8234puneetdixit200 wants to merge 3 commits into
&&, ||, etc.)#8234Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughrunCommand now runs commands through a shell (execa.command with shell: true). Helpers were added: CommandResult shape, isCommandResult type guard, and getCommandName to correctly extract possibly quoted command names. Non-existent-command detection was rewritten to use the result shape and check exitCode === 127 or platform “not recognized” messages instead of relying on Error.message. An integration test verifies commands with shell operators (e.g., "echo first && echo second") produce separate stdout lines and are not passed as literal arguments. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
There was a problem hiding this comment.
Pull request overview
This PR fixes netlify dev execution of dev.command values that include shell operators (e.g., &&, ||) by running the configured command through a shell, matching the behavior users expect from netlify.toml command strings.
Changes:
- Updated
runCommandto execute viaexeca.command(..., { shell: true })so shell operators are interpreted. - Added an integration regression test verifying
dev.commandsupports&&and does not treat it as a literal argument.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/utils/shell.ts |
Enables shell mode for netlify dev command execution to support shell operators in command strings. |
tests/integration/framework-detection.test.ts |
Adds an integration test to confirm dev.command supports && semantics. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const commandProcess = execa.command(command, { | ||
| preferLocal: true, | ||
| // Command strings in netlify.toml may use shell operators like `&&`. | ||
| // execa.command() does not interpret these unless shell mode is enabled. | ||
| shell: true, |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/utils/shell.ts (1)
115-134:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
isNonExistingCommandErroris broken byshell: true— non-existing command errors now show the wrong message.With
shell: true, execa spawns the system shell (/bin/sh,cmd.exe) rather than the user command directly. The shell binary always starts successfully, socode: 'ENOENT'is never set on the result when the user's command doesn't exist on POSIX. On POSIX, the shell simply exits with code 127 instead.Similarly on Windows, when cmd.exe reports
"is not recognized..."it writes that to stdout/stderr — it does not appear incommandError.message, so the string-match branch also misses it.The practical consequence: the user-friendly message
"Failed running command: X. Please verify 'X' exists"(line 119) will never be shown for any non-existing command after this change. Instead, the genericshortMessagepath (line 128) fires. The snapshot test attests/integration/__snapshots__/framework-detection.test.ts.snapshows the friendly error message as expected output, but the current implementation cannot produce it.To fix, check
result.exitCode === 127on POSIX (the shell's canonical "command not found" exit code) as a complement to the existing ENOENT check:🐛 Proposed fix
const isNonExistingCommandError = ({ command, error: commandError }: { command: string; error: unknown }) => { // `ENOENT` is only returned for non Windows systems // See https://github.com/sindresorhus/execa/pull/447 if (isErrnoException(commandError) && commandError.code === 'ENOENT') { return true } + // When shell: true is used, the shell itself starts successfully. + // On POSIX, a missing command causes the shell to exit with code 127. + if ( + commandError instanceof Error && + 'exitCode' in commandError && + (commandError as { exitCode: unknown }).exitCode === 127 + ) { + return true + } + // if the command is a package manager we let it report the error if (['yarn', 'npm', 'pnpm'].includes(command)) { return false }🤖 Prompt for 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. In `@src/utils/shell.ts` around lines 115 - 134, The non-existing-command detection (used where commandProcess resolves and isNonExistingCommandError is called) fails when execa is run with shell: true because shells succeed but return exit code 127 on POSIX or emit human text on Windows; update isNonExistingCommandError (and the call-site logic around result in the commandProcess.then block) to treat result.exitCode === 127 as a "command not found" on POSIX and also inspect result.stderr/result.stdout for Windows phrases like "is not recognized as an internal or external command" (or "is not recognized") for cmd.exe, so that the friendly log path (the branch that logs Failed running command: ... Please verify 'X' exists) is triggered when shell-based command resolution fails.
🧹 Nitpick comments (2)
tests/integration/framework-detection.test.ts (1)
133-150: 💤 Low valueNew integration test LGTM — one minor regex note.
The test structure correctly mirrors the existing
"should run \command`..."pattern and validates both the positive (shell operators interpreted) and negative (literal&&` not echoed) cases.One minor note on Line 146:
\s*between the two capture groups includes\n, so the regex would match even if arbitrary blank lines appear betweenfirstandsecond. Using[^\S\r\n]*(horizontal whitespace only) is stricter:🔧 Optional tightening
- t.expect(output).toMatch(/\nfirst\s*\r?\n\s*second\r?\n/i) + t.expect(output).toMatch(/\nfirst[^\S\r\n]*\r?\n[^\S\r\n]*second\r?\n/i)🤖 Prompt for 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. In `@tests/integration/framework-detection.test.ts` around lines 133 - 150, The regex in the test "should support shell operators in `command`" is too permissive because the `\s*` between the capture groups can match newlines; update the assertion that checks the output (the t.expect(output).toMatch(...) call) to use horizontal-whitespace-only matching (e.g., replace `\s*` between `first` and `second` with `[^\S\r\n]*`) so blank lines won't satisfy the test; locate this in the test using withSiteBuilder/withNetlifyToml/withDevServer and normalizeSnapshot to adjust the pattern accordingly.src/utils/shell.ts (1)
71-73: 💤 Low valueRemove code-explaining comments.
The two comments describe what
execa.command()does withoutshell: true, which is implementation documentation rather than essential context. The option name is self-explanatory and the PR description/commit message is the right place for this rationale.♻️ Proposed change
- // Command strings in netlify.toml may use shell operators like `&&`. - // execa.command() does not interpret these unless shell mode is enabled. shell: true,As per coding guidelines: "Never write comments on what the code does; make the code clean and self-explanatory instead."
🤖 Prompt for 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. In `@src/utils/shell.ts` around lines 71 - 73, Remove the explanatory comment lines above the shell option so the code only contains the self-explanatory option "shell: true" in the execa command invocation; locate the object where execa.command() is configured (the block that sets shell: true in src/utils/shell.ts) and delete the two comment lines describing why shell mode is needed, leaving the option as-is.
🤖 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.
Outside diff comments:
In `@src/utils/shell.ts`:
- Around line 115-134: The non-existing-command detection (used where
commandProcess resolves and isNonExistingCommandError is called) fails when
execa is run with shell: true because shells succeed but return exit code 127 on
POSIX or emit human text on Windows; update isNonExistingCommandError (and the
call-site logic around result in the commandProcess.then block) to treat
result.exitCode === 127 as a "command not found" on POSIX and also inspect
result.stderr/result.stdout for Windows phrases like "is not recognized as an
internal or external command" (or "is not recognized") for cmd.exe, so that the
friendly log path (the branch that logs Failed running command: ... Please
verify 'X' exists) is triggered when shell-based command resolution fails.
---
Nitpick comments:
In `@src/utils/shell.ts`:
- Around line 71-73: Remove the explanatory comment lines above the shell option
so the code only contains the self-explanatory option "shell: true" in the execa
command invocation; locate the object where execa.command() is configured (the
block that sets shell: true in src/utils/shell.ts) and delete the two comment
lines describing why shell mode is needed, leaving the option as-is.
In `@tests/integration/framework-detection.test.ts`:
- Around line 133-150: The regex in the test "should support shell operators in
`command`" is too permissive because the `\s*` between the capture groups can
match newlines; update the assertion that checks the output (the
t.expect(output).toMatch(...) call) to use horizontal-whitespace-only matching
(e.g., replace `\s*` between `first` and `second` with `[^\S\r\n]*`) so blank
lines won't satisfy the test; locate this in the test using
withSiteBuilder/withNetlifyToml/withDevServer and normalizeSnapshot to adjust
the pattern accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3f2281fc-868f-4dda-a909-89e6fc361732
📒 Files selected for processing (2)
src/utils/shell.tstests/integration/framework-detection.test.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/utils/shell.ts (1)
23-23: 💤 Low valueType guard is overly permissive.
isCommandResultaccepts any non-null object rather than verifying the expected shape. This works in practice becausecommandErroris an execa result, but the guard doesn't actually narrow toCommandResult.♻️ Optional: Strengthen the type guard
-const isCommandResult = (value: unknown): value is CommandResult => typeof value === 'object' && value !== null +const isCommandResult = (value: unknown): value is CommandResult => + typeof value === 'object' && + value !== null && + ('exitCode' in value || 'message' in value || 'shortMessage' in value || 'stderr' in value || 'stdout' in value)🤖 Prompt for 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. In `@src/utils/shell.ts` at line 23, The type guard isCommandResult is too permissive (it only checks for a non-null object) and doesn't ensure the shape of CommandResult; update isCommandResult to verify required properties from CommandResult (e.g., check for string/number/boolean properties and fields you rely on such as stderr, stdout, exitCode or signal as appropriate) so that narrowing actually identifies an execa CommandResult used in commandError handling; locate the function isCommandResult in src/utils/shell.ts and add checks for those specific keys and types before returning true.
🤖 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.
Nitpick comments:
In `@src/utils/shell.ts`:
- Line 23: The type guard isCommandResult is too permissive (it only checks for
a non-null object) and doesn't ensure the shape of CommandResult; update
isCommandResult to verify required properties from CommandResult (e.g., check
for string/number/boolean properties and fields you rely on such as stderr,
stdout, exitCode or signal as appropriate) so that narrowing actually identifies
an execa CommandResult used in commandError handling; locate the function
isCommandResult in src/utils/shell.ts and add checks for those specific keys and
types before returning true.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dd9ef1f3-623b-434c-b741-c50daad944e0
📒 Files selected for processing (2)
src/utils/shell.tstests/integration/framework-detection.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/integration/framework-detection.test.ts
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 `@src/utils/shell.ts`:
- Around line 32-35: The current getCommandName logic and the 127-handling
branch should not always map exit code 127 to the first token; update the code
that emits the "Please verify ..." message (the branch handling exit code 127
around the existing message emission) to first detect shell operators and
leading env assignments and only emit the specialized message when safe: use a
shell-syntax check (/(?:&&|\|\||[|;<>])/) to skip compound commands and an
env-assignment check (/^\s*[\w.-]+=/) to skip commands with leading VAR=
assignments; keep getCommandName as-is for simple cases but do not rely on it
when either check matches (fall back to a generic error message), and add
regression tests that run a failing compound command (e.g., "echo ok &&
missing-cmd") and a failing env-assigned command (e.g., "FOO=1 missing-cmd") to
ensure the specialized message is not emitted incorrectly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| const getCommandName = (command: string) => { | ||
| const match = /^(?:"([^"]+)"|'([^']+)'|(\S+))/.exec(command.trim()) | ||
|
|
||
| return match?.[1] ?? match?.[2] ?? match?.[3] ?? command |
There was a problem hiding this comment.
Don't map every shell 127 back to the first token.
When shell execution is enabled (shell: true), exit code 127 can result from any command in a compound command chain or from a missing executable preceded by environment variable assignments. For example:
echo ok && missing-cmdwill currently ask users to verify'echo'exists (incorrect)FOO=1 missing-cmdwill point at'FOO=1'(not a valid executable)
The current implementation only extracts the first token via regex, missing the actual failing command. Only emit the specialized "Please verify ..." message when you can safely attribute the failure to the first executable. Skip it for:
- Compound commands containing shell operators (
&&,||,|,;,<,>) - Commands preceded by leading environment variable assignments
Consider applying checks at lines 138-143 (where the message is emitted):
Shell syntax check: /(?:&&|\|\||[|;<>])/
Env assignment check: /^\s*[\w.-]+=/ Also add regression tests covering a missing command after && and one with a leading environment assignment to prevent this issue from recurring.
🧰 Tools
🪛 OpenGrep (1.21.0)
[ERROR] 33-33: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🤖 Prompt for 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.
In `@src/utils/shell.ts` around lines 32 - 35, The current getCommandName logic
and the 127-handling branch should not always map exit code 127 to the first
token; update the code that emits the "Please verify ..." message (the branch
handling exit code 127 around the existing message emission) to first detect
shell operators and leading env assignments and only emit the specialized
message when safe: use a shell-syntax check (/(?:&&|\|\||[|;<>])/) to skip
compound commands and an env-assignment check (/^\s*[\w.-]+=/) to skip commands
with leading VAR= assignments; keep getCommandName as-is for simple cases but do
not rely on it when either check matches (fall back to a generic error message),
and add regression tests that run a failing compound command (e.g., "echo ok &&
missing-cmd") and a failing env-assigned command (e.g., "FOO=1 missing-cmd") to
ensure the specialized message is not emitted incorrectly.
Summary
Fixes #8228 by running
dev.commandthrough a shell so command strings with shell operators (like&&) are interpreted correctly innetlify dev.Changes
runCommandto callexeca.command(..., { shell: true }).should support shell operators in \command`intests/integration/framework-detection.test.ts`.Why
execa.command()does not interpret shell operators by default. Withoutshell: true, a command like:is parsed as a single command with literal
&&args, causing the observed failure.Validation
npm run buildnpx vitest run tests/integration/framework-detection.test.ts -t 'should support shell operators in \command`'`npx vitest run tests/integration/framework-detection.test.ts -t "should print specific error when command doesn't exist"npx vitest run tests/integration/framework-detection.test.ts -t 'should run \command` when both `command` and `targetPort` are configured'`