Skip to content

chore(js): Remove live = 'rejectOnError' option #2971

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.

Sign up for GitHub

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

Open
szokeasaurusrex wants to merge 1 commit into
base: szokeasaurusrex/no-release-bundle-uploads
Choose a base branch
from
Open

chore(js): Remove live = 'rejectOnError' option #2971

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ we should rename this section to "Unreleased" -->

The following changes only apply when using `sentry-cli` via the npm package [`@sentry/cli`](https://www.npmjs.com/package/@sentry/cli):

- The `SentryCli.execute` method's `live` parameter now only takes boolean values ([#2971](https://github.com/getsentry/sentry-cli/pull/2971)). Setting `live` to `true` now behaves like `'rejectOnError'` did previously, with a zero exit status resolving the returned promise with `"success (live mode)"` and a non-zero status rejecting the promise with an error message.
- The `option` parameter to `Releases.uploadSourceMaps` no longer takes a `live` property ([#2971](https://github.com/getsentry/sentry-cli/pull/2971)). We now always execute the command with `live` set to `true`.
- Removed the `apiKey` option from `SentryCliOptions` ([#2935](https://github.com/getsentry/sentry-cli/pull/2935)). If you are using `apiKey`, you need to generate and use an [Auth Token](https://docs.sentry.io/account/auth-tokens/) via the `authToken` option, instead.

### Improvements
Expand Down
14 changes: 12 additions & 2 deletions lib/__tests__/helper.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,17 +28,27 @@ describe('SentryCli helper', () => {
expect(output.trim()).toBe('sentry-cli DEV');
});

test('execute with live=true resolves without output', async () => {
test('execute with live=true resolves on success', async () => {
// TODO (v3): This should resolve with a string, not undefined/void
const result = await helper.execute(['--version'], true);
expect(result).toBeUndefined();
expect(result).toBe('success (live mode)');
});

test('execute with live=true rejects on failure', async () => {
await expect(helper.execute(['fail'], true)).rejects.toThrow(
'Command fail failed with exit code 1'
);
});

// live=rejectOnError is not supported per the type declarations, but we should still aim
// to support it for backwards compatibility.
test('execute with live=rejectOnError resolves on success', async () => {
const result = await helper.execute(['--version'], 'rejectOnError');
expect(result).toBe('success (live mode)');
});

// live=rejectOnError is not supported per the type declarations, but we should still aim
// to support it for backwards compatibility.
test('execute with live=rejectOnError rejects on failure', async () => {
await expect(helper.execute(['fail'], 'rejectOnError')).rejects.toThrow(
'Command fail failed with exit code 1'
Expand Down
23 changes: 7 additions & 16 deletions lib/helper.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,11 +281,10 @@ function getPath() {
* expect(output.trim()).toBe('sentry-cli x.y.z');
*
* @param {string[]} args Command line arguments passed to `sentry-cli`.
* @param {boolean | 'rejectOnError'} live can be set to:
* - `true` to inherit stdio to display `sentry-cli` output directly.
* - `false` to not inherit stdio and return the output as a string.
* - `'rejectOnError'` to inherit stdio and reject the promise if the command
* @param {boolean} live can be set to:
* - `true` to inherit stdio and reject the promise if the command
* exits with a non-zero exit code.
* - `false` to not inherit stdio and return the output as a string.
* @param {boolean} silent Disable stdout for silents build (CI/Webpack Stats, ...)
* @param {string} [configFile] Relative or absolute path to the configuration file.
* @param {import('./index').SentryCliOptions} [config] More configuration to pass to the CLI
Expand Down Expand Up @@ -325,26 +324,18 @@ async function execute(args, live, silent, configFile, config = {}) {
}

return new Promise((resolve, reject) => {
if (live === true || live === 'rejectOnError') {
if (live) {
const output = silent ? 'ignore' : 'inherit';
const pid = childProcess.spawn(getPath(), args, {
env,
// stdin, stdout, stderr
stdio: ['ignore', output, output],
});
pid.on('exit', (exitCode) => {
if (live === 'rejectOnError') {
if (exitCode === 0) {
resolve('success (live mode)');
}
reject(new Error(`Command ${args.join(' ')} failed with exit code ${exitCode}`));
if (exitCode === 0) {
resolve('success (live mode)');
}
// According to the type definition, resolving with void is not allowed.
// However, for backwards compatibility, we resolve void here to
// avoid a behaviour-breaking change.
// TODO (v3): Clean this up and always resolve a string (or change the type definition)
// @ts-expect-error - see comment above
resolve();
reject(new Error(`Command ${args.join(' ')} failed with exit code ${exitCode}`));
Copy link

@cursor cursor bot Nov 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Missing else causes reject after successful resolve

The exit handler unconditionally calls reject() after conditionally calling resolve() for zero exit codes. When exitCode === 0, both resolve('success (live mode)') and reject(new Error(...)) execute sequentially. While promises only settle once, this creates an unhandled rejection and indicates incorrect control flow logic. An else clause or return statement is needed to prevent rejection after successful resolution.

Fix in Cursor Fix in Web

Comment on lines +335 to +338
Copy link

@sentry sentry bot Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The pid.on('exit') handler unconditionally calls reject() after resolving when exitCode === 0, causing promises to always fail.
Severity: CRITICAL | Confidence: High

🔍 Detailed Analysis

The pid.on('exit') handler in lib/helper.js lines 335-338 contains a logic error. When exitCode === 0, the promise correctly resolves with 'success (live mode)'. However, the reject() statement immediately follows this if block and is not enclosed in an else block, causing it to execute unconditionally. This means that even on successful process execution (exitCode === 0), the promise will first resolve and then immediately reject. This behavior causes Promise.all() calls, such as in uploadSourceMaps() in lib/releases/index.js, to fail entirely, as each individual promise will ultimately reject.

💡 Suggested Fix

Place the reject() statement within an else block for the if (exitCode === 0) condition in the pid.on('exit') handler to ensure rejection only occurs when exitCode !== 0.

🤖 Prompt for AI Agent 
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: lib/helper.js#L335-L338

Potential issue: The `pid.on('exit')` handler in `lib/helper.js` lines 335-338 contains
a logic error. When `exitCode === 0`, the promise correctly resolves with `'success
(live mode)'`. However, the `reject()` statement immediately follows this `if` block and
is not enclosed in an `else` block, causing it to execute unconditionally. This means
that even on successful process execution (`exitCode === 0`), the promise will first
resolve and then immediately reject. This behavior causes `Promise.all()` calls, such as
in `uploadSourceMaps()` in `lib/releases/index.js`, to fail entirely, as each individual
promise will ultimately reject.

Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 5120123

});
} else {
childProcess.execFile(getPath(), args, { env }, (err, stdout) => {
Expand Down
7 changes: 3 additions & 4 deletions lib/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,11 +68,10 @@ class SentryCli {
/**
* See {helper.execute} docs.
* @param {string[]} args Command line arguments passed to `sentry-cli`.
* @param {boolean | 'rejectOnError'} live can be set to:
* - `true` to inherit stdio to display `sentry-cli` output directly.
* - `false` to not inherit stdio and return the output as a string.
* - `'rejectOnError'` to inherit stdio and reject the promise if the command
* @param {boolean} live can be set to:
* - `true` to inherit stdio and reject the promise if the command
* exits with a non-zero exit code.
* - `false` to not inherit stdio and return the output as a string.
* @returns {Promise<string>} A promise that resolves to the standard output.
*/
execute(args, live) {
Expand Down
45 changes: 5 additions & 40 deletions lib/releases/__tests__/index.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,14 @@ describe('SentryCli releases', () => {
test('call sentry-cli releases propose-version', () => {
expect.assertions(1);
const cli = new SentryCli();
return cli.releases.proposeVersion().then(version => expect(version).toBeTruthy());
return cli.releases.proposeVersion().then((version) => expect(version).toBeTruthy());
});

describe('with mock', () => {
let cli;
let mockExecute;
beforeAll(() => {
mockExecute = jest.fn(async () => { });
mockExecute = jest.fn(async () => {});
jest.doMock('../../helper', () => ({
...jest.requireActual('../../helper'),
execute: mockExecute,
Expand Down Expand Up @@ -52,15 +52,7 @@ describe('SentryCli releases', () => {
test('without projects', async () => {
await cli.releases.uploadSourceMaps('my-version', { include: ['path'] });
expect(mockExecute).toHaveBeenCalledWith(
[
'sourcemaps',
'upload',
'--release',
'my-version',
'path',
'--ignore',
'node_modules',
],
['sourcemaps', 'upload', '--release', 'my-version', 'path', '--ignore', 'node_modules'],
true,
false,
undefined,
Expand Down Expand Up @@ -100,17 +92,9 @@ describe('SentryCli releases', () => {
await cli.releases.uploadSourceMaps('my-version', { include: paths });

expect(mockExecute).toHaveBeenCalledTimes(2);
paths.forEach(path =>
paths.forEach((path) =>
expect(mockExecute).toHaveBeenCalledWith(
[
'sourcemaps',
'upload',
'--release',
'my-version',
path,
'--ignore',
'node_modules',
],
['sourcemaps', 'upload', '--release', 'my-version', path, '--ignore', 'node_modules'],
true,
false,
undefined,
Expand Down Expand Up @@ -159,25 +143,6 @@ describe('SentryCli releases', () => {
{ silent: false }
);
});

test.each([true, false, 'rejectOnError'])('handles live mode %s', async (live) => {
await cli.releases.uploadSourceMaps('my-version', { include: ['path'], live });
expect(mockExecute).toHaveBeenCalledWith(
[
'sourcemaps',
'upload',
'--release',
'my-version',
'path',
'--ignore',
'node_modules',
],
live,
false,
undefined,
{ silent: false }
);
});
});
});
});
15 changes: 5 additions & 10 deletions lib/releases/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,11 +142,10 @@ class Releases {
* ext: ['js', 'map', 'jsbundle', 'bundle'], // override file extensions to scan for
* projects: ['node'], // provide a list of projects
* decompress: false // decompress gzip files before uploading
* live: true // whether to inherit stdio to display `sentry-cli` output directly.
* });
*
* @param {string} release Unique name of the release.
* @param {SentryCliUploadSourceMapsOptions & {live?: boolean | 'rejectOnError'}} options Options to configure the source map upload.
* @param {SentryCliUploadSourceMapsOptions} options Options to configure the source map upload.
Copy link
Member

@JPeer264 JPeer264 Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@szokeasaurusrex Here the live option is entirely removed, but in types.ts the live option is just adapted. What is now true?

Copy link
Member Author

@szokeasaurusrex szokeasaurusrex Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, exactly, this removal is intentional. For sourcemaps upload, we always use live = true, and there is no way to override this (this is actually what we had prior to introducing the 'rejectOnError' option; we only made live configurable here to allow us to use 'rejectOnError').

* @returns {Promise<string[]>} A promise that resolves when the upload has completed successfully.
* @memberof SentryReleases
*/
Expand Down Expand Up @@ -193,10 +192,7 @@ class Releases {

return uploadPaths.map((path) =>
// `execute()` is async and thus we're returning a promise here
this.execute(
helper.prepareCommand([...args, path], SOURCEMAPS_SCHEMA, newOptions),
options.live != null ? options.live : true
)
this.execute(helper.prepareCommand([...args, path], SOURCEMAPS_SCHEMA, newOptions), true)
);
});

Expand Down Expand Up @@ -252,11 +248,10 @@ class Releases {
/**
* See {helper.execute} docs.
* @param {string[]} args Command line arguments passed to `sentry-cli`.
* @param {boolean | 'rejectOnError'} live can be set to:
* - `true` to inherit stdio to display `sentry-cli` output directly.
* - `false` to not inherit stdio and return the output as a string.
* - `'rejectOnError'` to inherit stdio and reject the promise if the command
* @param {boolean} live can be set to:
* - `true` to inherit stdio and reject the promise if the command
* exits with a non-zero exit code.
* - `false` to not inherit stdio and return the output as a string.
* @returns {Promise<string>} A promise that resolves to the standard output.
*/
async execute(args, live) {
Expand Down
4 changes: 2 additions & 2 deletions lib/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,12 +224,12 @@ export interface SentryCliReleases {

uploadSourceMaps(
release: string,
options: SentryCliUploadSourceMapsOptions & { live?: boolean | 'rejectOnError' }
options: SentryCliUploadSourceMapsOptions & { live?: boolean }
): Promise<string[]>;

listDeploys(release: string): Promise<string>;

newDeploy(release: string, options: SentryCliNewDeployOptions): Promise<string>;

execute(args: string[], live: boolean | 'rejectOnError'): Promise<string>;
execute(args: string[], live: boolean): Promise<string>;
}