fix(dev): execute dev command with shell operators (&&, ||, etc.)#8234
fix(dev): execute dev command with shell operators (&&, ||, etc.)#8234puneetdixit200 wants to merge 1 commit intonetlify:mainfrom
&&, ||, etc.)#8234Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis pull request fixes a parsing issue where shell operators (such as Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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
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'`