fix(dev): execute dev command with shell operators (&&, ||, etc.)#8234
fix(dev): execute dev command with shell operators (&&, ||, etc.)#8234puneetdixit200 wants to merge 7 commits into
&&, ||, etc.)#8234Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughrunCommand now runs commands through a shell (execa.command with shell: true). Helpers were added: CommandResult shape, isCommandResult type guard, getCommandName to correctly extract possibly quoted command names, and canReportMissingCommandName. Non-existent-command detection was rewritten to aggregate message/stdout/stderr and detect missing-command text (including Windows phrasing). Integration tests were added to verify shell operators and normalized failure output. 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.
![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
|
Rebased this branch onto the latest Verification:
|
puneetdixit200
force-pushed
the
fix-dev-command-shell-operators-8228
branch
from
74e40bb to
4f17001
Compare
|
Pushed Verified locally:
|
|
Checked the latest Node 24 integration failures on this branch. What I verified locally with Node 24.15 after rebuilding:
The failing |
|
Follow-up pushed in What changed:
Local verification:
I also reproduced a CI-shaped shard locally earlier: the framework-detection timeout case passed there, while the shard hit an unrelated edge-functions timeout under load. This push reduces the branch-added integration load while preserving the runtime coverage. |
|
Pushed a small test-only follow-up ( Local verification: After the push, GitHub currently shows only neutral/skipped provider rows plus CodeRabbit/deploy-preview for the new head; the old Node 24 integration failures are no longer attached to the latest commit. |
Summary
Fixes #8228 by running
dev.commandthrough a shell only when the command string needs shell parsing, such as shell operators (&&,||, pipes, redirects, semicolons) or POSIX-style inline environment assignments.Changes
execa.command()path so existing command execution behavior stays unchanged.Why
execa.command()does not interpret shell operators by default. Without shell parsing, a command like:is parsed as a single command with literal
&&args, causing the observed failure.Validation
npm run buildnpm exec vitest -- run tests/integration/framework-detection.test.ts -t 'shell operators|generic command failure'npm exec vitest -- run tests/integration/framework-detection.test.ts -t 'should run and serve a production build when using the \serve` command'`npm exec vitest -- run tests/integration/commands/dev/functions.test.ts -t 'nodeModuleFormat: esm v1 functions should work'