fix(run): make non-recursive --no-bail exit non-zero when a script fails#12263
Conversation
Review Summary by QodoAdd
WalkthroughsDescription• Add --report-failure for pnpm run • Exit non-zero when --no-bail scripts fail • Preserve existing --no-bail behavior by default • Add coverage for success and failure cases Diagramflowchart LR
A["CLI options"] --> B["`--report-failure` added"]
B --> C["`run` command execution"]
C --> D["`Promise.allSettled` in `--no-bail` mode"]
D --> E["Aggregate failed scripts"]
E --> F["Throw `PnpmError` when requested"]
C --> G["Keep zero exit by default"]
File Changes1. exec/commands/src/run.ts
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthrough
Changes--no-bail Exit Code Fix
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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 |
|
Is there any steps that I need to do that I haven't done in this PR? Is the docstring the only stopper? |
5a3b48c to
e6e8b82
Compare
Code Review by Qodo
Context used✅ Tickets:
🎫 --no-bail behavior inconsistent 1. --no-bail semantics diverge
|
e6e8b82 to
f9c6b94
Compare
|
Code review by qodo was updated up to the latest commit f9c6b94 |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
exec/commands/test/index.ts (2)
68-84: ⚡ Quick winGuard caught errors before asserting
code.These new assertions read
err.codefromanycatches. Useutil.types.isNativeError(err)before checking the pnpm error code to follow the repo’s cross-realm Jest guidance. As per coding guidelines, “Jest/TS error checks: avoidinstanceof Error; useutil.types.isNativeError(err)when asserting error types/code across Jest realms.”♻️ Proposed test assertion shape
import fs from 'node:fs' import path from 'node:path' +import { types as utilTypes } from 'node:util'- let err!: PnpmError + let err: unknown try { await run.handler({ ...DEFAULT_OPTS, @@ - } catch (_err: any) { // eslint-disable-line + } catch (_err: unknown) { err = _err } - expect(err).toBeDefined() - expect(err.code).toBe('ERR_PNPM_RUN_FAILED') + expect(utilTypes.isNativeError(err)).toBe(true) + expect((err as PnpmError).code).toBe('ERR_PNPM_RUN_FAILED')Apply the same pattern to the regex failure test.
Also applies to: 99-115
🤖 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 `@exec/commands/test/index.ts` around lines 68 - 84, The error caught in the try-catch block is typed as any, and the code directly accesses err.code without validation, which violates cross-realm Jest error handling guidelines. Add a guard check using util.types.isNativeError(err) before asserting on err.code to ensure the error is a native error before accessing its properties. Apply the same guard pattern to the regex failure test mentioned in the "Also applies to" section (lines 99-115) to maintain consistency across all error assertions in this test file.Source: Coding guidelines
90-116: ⚡ Quick winMake the regex failure test prove every matched script ran.
The test name says every matched script runs, but the scripts only exit; there is no side effect proving
lint:aandlint:ccompleted whenlint:bfails. Add output assertions so a regression back to fail-fast behavior is caught.✅ Proposed coverage improvement
prepare({ scripts: { - 'lint:a': 'node -e "process.exit(0)"', - 'lint:b': 'node -e "process.exit(1)"', - 'lint:c': 'node -e "process.exit(0)"', + 'lint:a': 'node -e "require(\'fs\').writeFileSync(\'./output-lint-a.txt\', \'a\', \'utf8\')"', + 'lint:b': 'node -e "require(\'fs\').writeFileSync(\'./output-lint-b.txt\', \'b\', \'utf8\'); process.exit(1)"', + 'lint:c': 'node -e "require(\'fs\').writeFileSync(\'./output-lint-c.txt\', \'c\', \'utf8\')"', }, }) @@ expect(err).toBeDefined() expect(err.code).toBe('ERR_PNPM_RUN_FAILED') + expect(fs.readFileSync('output-lint-a.txt', { encoding: 'utf-8' })).toBe('a') + expect(fs.readFileSync('output-lint-b.txt', { encoding: 'utf-8' })).toBe('b') + expect(fs.readFileSync('output-lint-c.txt', { encoding: 'utf-8' })).toBe('c') })🤖 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 `@exec/commands/test/index.ts` around lines 90 - 116, The test `pnpm run with regex and --no-bail runs every matched script but exits non-zero when one fails` does not actually prove that all three matched scripts (lint:a, lint:b, lint:c) executed. Currently the test only verifies that an error was thrown with the correct code, but there is no observable output proving each script ran when one failed. Modify each script in the prepare block to produce distinct output (for example, echoing a unique identifier), then add assertions in the test that verify each expected output is present in the command's output. This will demonstrate that execution continued through all scripts despite the --no-bail flag and lint:b failing.
🤖 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 `@exec/commands/test/index.ts`:
- Around line 68-84: The error caught in the try-catch block is typed as any,
and the code directly accesses err.code without validation, which violates
cross-realm Jest error handling guidelines. Add a guard check using
util.types.isNativeError(err) before asserting on err.code to ensure the error
is a native error before accessing its properties. Apply the same guard pattern
to the regex failure test mentioned in the "Also applies to" section (lines
99-115) to maintain consistency across all error assertions in this test file.
- Around line 90-116: The test `pnpm run with regex and --no-bail runs every
matched script but exits non-zero when one fails` does not actually prove that
all three matched scripts (lint:a, lint:b, lint:c) executed. Currently the test
only verifies that an error was thrown with the correct code, but there is no
observable output proving each script ran when one failed. Modify each script in
the prepare block to produce distinct output (for example, echoing a unique
identifier), then add assertions in the test that verify each expected output is
present in the command's output. This will demonstrate that execution continued
through all scripts despite the --no-bail flag and lint:b failing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: cc2c1be8-9f41-4e3e-b6a1-ab89c9f6d453
📒 Files selected for processing (3)
.changeset/no-bail-exit-code.mdexec/commands/src/run.tsexec/commands/test/index.ts
✅ Files skipped from review due to trivial changes (1)
- .changeset/no-bail-exit-code.md
Previously `pnpm run --no-bail` (without -r) always exited with code 0 even when a matched script failed, while recursive runs failed at the end via throwOnCommandFail. --no-bail now runs every matched script to completion and exits non-zero if any failed, making the behavior consistent across recursive and non-recursive runs. Closes pnpm#8013.
f9c6b94 to
9055482
Compare
|
Code review by qodo was updated up to the latest commit 9055482 |
|
Addressed the latest review feedback in 9055482:
All Written by an agent (Claude Code, claude-opus-4-8). |
|
@zkochan 🙇 |
When running a non-recursive
pnpm run --no-bailthat matches multiple scripts (e.g. via a/regex/selector), pnpm always exited with code0regardless of whether any script failed. This is inconsistent with recursive runs, which aggregate failures and exit non-zero at the end (viathrowOnCommandFail).This PR fixes
--no-baildirectly so its exit-code behavior is consistent across recursive and non-recursive runs, as requested in #8013:--no-bailstill runs every matched script to completion (it no longer short-circuits on the first failure — execution switched fromPromise.alltoPromise.allSettled).ERR_PNPM_RUN_FAILED) if any of them failed.This is a behavior change: previously a non-recursive
pnpm run --no-bailwith a failing script exited0. No new flag is introduced — per the issue discussion, a separate flag "would just add confusion without benefit".Closes #8013
Written by an agent (Claude Code, claude-opus-4-8).
Summary by CodeRabbit
pnpm run --no-bailnow returns a non-zero exit code if any matched scripts fail, while still running all matched scripts to completion.--no-bailhelp text to clarify that failures don’t stop execution, but the command can still fail overall.