Skip to content

Commit 05953a0

Browse files
fix(npm): Show publish stderr at info level (#877)
## Summary npm sends publish notices and warnings to stderr. Craft currently logs that stream only at Trace, so normal Info-level publish runs hide those diagnostics even when npm exits successfully. - Add an opt-in `showStderr` option to `spawnProcess`, matching the existing `showStdout` behavior. - Enable it for the npm target's token and OIDC publish paths (including the existing Yarn fallback). - Keep other subprocesses trace-only by default. Do not change stdout return values, exit-code handling, auth, or publish commands. - Extend the subprocess tests for default logging, visible stderr on success/failure, and the npm target tests for both auth paths. This exposes future publish output. It cannot recover stderr from earlier runs and does not retry or verify a registry publish. The publishing image must pick up this change after merge/release before it affects runs. ## Validation - Frozen pnpm install, 54 focused tests, typecheck, formatting, and diff checks pass. - Lint passes with seven existing warnings outside these files. - Local build and CLI version smoke test pass. The first `pnpm build` exited 0 but its optional Sentry upload reported HTTP 413; rebuilding with `env -u SENTRY_AUTH_TOKEN pnpm build` succeeded without the upload. - `env -u SENTRY_AUTH_TOKEN pnpm test` exits 1: 1,200 passed, three failed, one skipped. The three `prepare-dry-run.e2e.test.ts` failures report `Git author was not set to the Junior identity` because the fixtures set `Test User`. Two also encounter GitHub's unresolved `test-owner/test-repo` fixture. A snapshot becomes obsolete because the failing test never reaches its assertion. No runtime identity guards or fixture behavior were changed. <!-- junior-request-attribution:start --> via **David Cramer**. <!-- junior-request-attribution:end --> <!-- junior-session-footer:start --> <!-- junior-conversation-id:slack%3AC0B595QDZLL%3A1790001522.417909 --> -- [View Junior Session](https://junior-prod.sentry.dev/conversations/slack%3AC0B595QDZLL%3A1790001522.417909) [[Sentry]](https://sentry.sentry.io/explore/conversations/slack%3AC0B595QDZLL%3A1790001522.417909/?project=4510944073809921) <!-- junior-session-footer:end --> Co-authored-by: sentry-junior[bot] <264270552+sentry-junior[bot]@users.noreply.github.com> Co-authored-by: David Cramer <david@sentry.io>
1 parent c17a278 commit 05953a0

4 files changed

Lines changed: 83 additions & 3 deletions

File tree

‎src/targets/__tests__/npm.test.ts‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -518,6 +518,39 @@ describe('NpmTarget OIDC configuration', () => {
518518
expect(target.npmConfig.token).toBe('my-token');
519519
});
520520

521+
it.each([false, true])(
522+
'shows publish stdout and stderr with oidc=%s',
523+
async oidc => {
524+
vi.stubEnv('NPM_TOKEN', 'my-token');
525+
vi.stubEnv('USE_YARN', '');
526+
const spawnProcessMock = vi
527+
.spyOn(system, 'spawnProcess')
528+
.mockResolvedValue(Buffer.from('+ @sentry/browser@1.0.0\n'));
529+
const provider = {
530+
filterArtifactsForRevision: vi
531+
.fn()
532+
.mockResolvedValue([{ filename: 'sentry-browser-1.0.0.tgz' }]),
533+
downloadArtifact: vi
534+
.fn()
535+
.mockResolvedValue('/tmp/sentry-browser-1.0.0.tgz'),
536+
} as unknown as BaseArtifactProvider;
537+
538+
try {
539+
const target = new TestNpmTarget({ name: 'npm', oidc }, provider);
540+
await target.publish('1.0.0', 'release-sha');
541+
542+
expect(spawnProcessMock).toHaveBeenCalledExactlyOnceWith(
543+
NPM_BIN,
544+
['publish', '--ignore-scripts', '/tmp/sentry-browser-1.0.0.tgz'],
545+
expect.any(Object),
546+
{ showStdout: true, showStderr: true },
547+
);
548+
} finally {
549+
spawnProcessMock.mockRestore();
550+
}
551+
},
552+
);
553+
521554
it('throws when oidc: true and npm version is too old', () => {
522555
TestNpmTarget.mockVersion = { major: 5, minor: 6, patch: 0 };
523556

‎src/targets/npm.ts‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -848,7 +848,10 @@ export class NpmTarget extends BaseTarget {
848848
spawnOptions.env.NPM_CONFIG_OTP = options.otp;
849849
}
850850
// Disable output buffering because npm can ask for one-time passwords
851-
return spawnProcess(bin, args, spawnOptions, { showStdout: true });
851+
return spawnProcess(bin, args, spawnOptions, {
852+
showStdout: true,
853+
showStderr: true,
854+
});
852855
}
853856

854857
return withTempFile(filePath => {
@@ -869,6 +872,7 @@ export class NpmTarget extends BaseTarget {
869872
// Disable output buffering because NPM/Yarn can ask us for one-time passwords
870873
return spawnProcess(bin, args, spawnOptions, {
871874
showStdout: true,
875+
showStderr: true,
872876
});
873877
});
874878
}

‎src/utils/__tests__/system.test.ts‎

Lines changed: 38 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -78,9 +78,15 @@ describe('spawnProcess', () => {
7878
test('does not write to output by default', async () => {
7979
const mockedLogInfo = logger.info as Mock;
8080

81-
await spawnProcess(process.execPath, ['-e', 'console.log("test-string")']);
81+
await spawnProcess(process.execPath, [
82+
'-e',
83+
'console.log("test-string"); process.stderr.write("test-warning")',
84+
]);
8285

8386
expect(mockedLogInfo).toHaveBeenCalledTimes(0);
87+
expect(logger.trace).toHaveBeenCalledWith(
88+
`${process.execPath}: test-warning`,
89+
);
8490
});
8591

8692
test('writes to output if told so', async () => {
@@ -97,6 +103,37 @@ describe('spawnProcess', () => {
97103
expect(mockedLogInfo.mock.calls[0][0]).toMatch(/test-string/);
98104
});
99105

106+
test.each([0, 1])(
107+
'shows stderr without changing stdout or exit code %i',
108+
async exitCode => {
109+
const result = spawnProcess(
110+
process.execPath,
111+
[
112+
'-e',
113+
`process.stdout.write("result"); process.stderr.write("publish-notice"); process.exitCode = ${exitCode}`,
114+
],
115+
{},
116+
{ showStderr: true },
117+
);
118+
119+
if (exitCode === 0) {
120+
expect((await result)?.toString()).toBe('result');
121+
} else {
122+
await expect(result).rejects.toMatchObject({
123+
code: exitCode,
124+
message: expect.stringContaining('publish-notice'),
125+
});
126+
}
127+
128+
expect(logger.info).toHaveBeenCalledWith(
129+
`${process.execPath}: publish-notice`,
130+
);
131+
expect(logger.info).not.toHaveBeenCalledWith(
132+
`${process.execPath}: result`,
133+
);
134+
},
135+
);
136+
100137
describe('env sanitisation (defence-in-depth)', () => {
101138
const savedEnv = { ...process.env };
102139

‎src/utils/system.ts‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -106,6 +106,8 @@ export function replaceEnvVariable(
106106
export interface SpawnProcessOptions {
107107
/** Do not buffer standard output */
108108
showStdout?: boolean;
109+
/** Log standard error at info level instead of trace */
110+
showStderr?: boolean;
109111
/** Force the process to run in dry-run mode */
110112
enableInDryRunMode?: boolean;
111113
/** Data to write to stdin (process will receive 'pipe' for stdin instead of 'inherit') */
@@ -210,7 +212,11 @@ export async function spawnProcess(
210212
});
211213
child.stderr.pipe(split()).on('data', (data: any) => {
212214
const output = `${command}: ${data}`;
213-
logger.trace(output);
215+
if (spawnProcessOptions.showStderr) {
216+
logger.info(output);
217+
} else {
218+
logger.trace(output);
219+
}
214220
stderr += `${output}\n`;
215221
});
216222
} catch (e) {

0 commit comments

Comments
 (0)