Core: Fix npm dependency detection

[fallintoplace]

Fixes #35098

What changed

This removes the fake shell redirection argument from npm ls --json in npm dependency detection. stderr is now suppressed through process stdio options, while stdout stays piped so the JSON output can still be parsed.

The regression test now uses a fake npm executable on PATH that writes valid JSON to stdout and warnings to stderr. That keeps the test focused on the observed dependency-detection outcome rather than only inspecting the command options.

Why

executeCommand passes an argv array directly to the child process. That means 2>/dev/null and 2>NUL are not shell redirections here; npm receives them as literal positional arguments/package filters, which can make installed dependency detection return incorrect results.

Validation

• node .yarn/releases/yarn-4.10.3.cjs vitest run --config code/core/vitest.config.ts code/core/src/common/js-package-manager/NPMProxy.test.ts
• node .yarn/releases/yarn-4.10.3.cjs exec oxfmt --check code/core/src/common/js-package-manager/NPMProxy.ts code/core/src/common/js-package-manager/NPMProxy.test.ts
• node ../.yarn/releases/yarn-4.10.3.cjs lint:js:cmd core/src/common/js-package-manager/NPMProxy.ts core/src/common/js-package-manager/NPMProxy.test.ts --quiet

Manual testing

Not applicable. This change only affects how the npm dependency detection command is invoked, and the regression is covered by the targeted unit test above.

Summary by CodeRabbit

• Tests

  • Added new test coverage for npm package installation discovery, including handling of npm warnings on stderr, retry behavior when the initial npm listing fails, and returning undefined when stdout is missing.

• Refactor

  • Improved consistency of npm command execution by using explicit environment and stdio handling, while ignoring npm stderr output to ensure reliable JSON-based discovery across environments.

[coderabbitai]

📝 Walkthrough

Walkthrough

findInstallations now spawns npm with env.FORCE_COLOR='false' and stdio configured to pipe stdout and ignore stderr instead of using shell stderr redirection; the unused platform import was removed. Three test cases validate stdout JSON parsing with stderr suppressed, retry behavior on failure, and undefined handling when stdout is empty.

Changes

NPM installation discovery stderr handling

Layer / File(s)	Summary
Configure npm ls with explicit stdio and env code/core/src/common/js-package-manager/NPMProxy.ts	findInstallations now spawns npm ls --json ... with env.FORCE_COLOR = 'false' and stdio set to ['pipe', 'pipe', 'ignore'] so stdout is piped and stderr is ignored; removed unused platform import from node:os.
findInstallations test cases code/core/src/common/js-package-manager/NPMProxy.test.ts	Adds three test cases validating that stdout JSON is parsed into dependencies while stderr npm warnings are ignored, that retry with --depth=0 occurs after failure and the successful stdout is parsed, and that undefined is returned when stdout is empty/undefined even if stderr contains JSON.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[Sidnioulz]

Thanks for this PR @fallintoplace!

Could you please help us out with a few followup actions?

• The tests don't help a lot; could you look for ways to make them more focused on outcomes?
• Could you link the PR to an open issue so we understand what impact it has on end users?

Thanks

[coderabbitai]

🧹 Nitpick comments (2)

code/core/src/common/js-package-manager/NPMProxy.test.ts (2)

252-282: ⚡ Quick win

Consider adding explanatory comments for complex test setup.

This integration test creates a sophisticated fake npm environment (temporary executables, PATH modification, stderr/stdout simulation) without inline documentation. Per the coding guideline "Document complex mock behaviors in Vitest tests," adding comments would improve maintainability.

Consider adding comments like:

// Create fake npm that writes warnings to stderr while emitting valid JSON to stdout
// Validates that no shell redirection tokens (e.g., '2>/dev/null') are passed as args

🤖 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 `@code/core/src/common/js-package-manager/NPMProxy.test.ts` around lines 252 -
282, Add concise inline comments above the fake npm test setup (around the
writeFileSync/chmodSync/process.env.PATH block in NPMProxy.test.ts) explaining
the mock behavior: that fake-npm.cjs writes a warning to stderr while outputting
valid JSON to stdout, that it validates incoming argv to ensure no shell
redirection tokens are passed (the args.some check), and that npm and npm.cmd
stubs are placed in tempDir and PATH is adjusted (using process.execPath) so the
test invokes these executables; keep comments short and specific to help future
maintainers understand why writeFileSync, chmodSync, and PATH manipulation are
required.

Source: Coding guidelines

---

251-251: 💤 Low value

Mock implementation should be in beforeEach block.

This line sets up the mock implementation inline within the test case, which violates the coding guideline requiring mock implementations in beforeEach blocks. While this is consistent with the existing pattern in this file (where other tests also configure executeCommand mocks per-test), consider refactoring to follow the guideline more strictly.

However, this integration-style test requires the real executeCommand implementation (unlike other tests that need specific return values), so the inline approach is pragmatic for this specific case.

🤖 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 `@code/core/src/common/js-package-manager/NPMProxy.test.ts` at line 251, The
mockedExecuteCommand.mockImplementation(actualCommand.executeCommand) call is
placed inline in the test; move this mock setup into the file's beforeEach so
all tests configure executeCommand there (locate mockedExecuteCommand and
actualCommand.executeCommand in NPMProxy.test.ts), and for this special
integration-style test either (a) allow the beforeEach to set the real
implementation by default or (b) add a per-test override flag that the test can
set before execution to keep the inline real implementation; adjust beforeEach
to use that flag or the real implementation so the majority of tests follow the
guideline while preserving this test's need for the real executeCommand.

Source: Coding guidelines

🤖 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 `@code/core/src/common/js-package-manager/NPMProxy.test.ts`:
- Around line 252-282: Add concise inline comments above the fake npm test setup
(around the writeFileSync/chmodSync/process.env.PATH block in NPMProxy.test.ts)
explaining the mock behavior: that fake-npm.cjs writes a warning to stderr while
outputting valid JSON to stdout, that it validates incoming argv to ensure no
shell redirection tokens are passed (the args.some check), and that npm and
npm.cmd stubs are placed in tempDir and PATH is adjusted (using
process.execPath) so the test invokes these executables; keep comments short and
specific to help future maintainers understand why writeFileSync, chmodSync, and
PATH manipulation are required.
- Line 251: The
mockedExecuteCommand.mockImplementation(actualCommand.executeCommand) call is
placed inline in the test; move this mock setup into the file's beforeEach so
all tests configure executeCommand there (locate mockedExecuteCommand and
actualCommand.executeCommand in NPMProxy.test.ts), and for this special
integration-style test either (a) allow the beforeEach to set the real
implementation by default or (b) add a per-test override flag that the test can
set before execution to keep the inline real implementation; adjust beforeEach
to use that flag or the real implementation so the majority of tests follow the
guideline while preserving this test's need for the real executeCommand.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 67cab0b3-69f8-492b-b15c-4ba97bec0359

📥 Commits

Reviewing files that changed from the base of the PR and between c6ef1ba and 47af599.

📒 Files selected for processing (2)

• code/core/src/common/js-package-manager/NPMProxy.test.ts
• code/core/src/common/js-package-manager/NPMProxy.ts

🚧 Files skipped from review as they are similar to previous changes (1)

• code/core/src/common/js-package-manager/NPMProxy.ts

[storybook-app-bot]

Package Benchmarks

^{Commit: 886f74d, ran on 17 June 2026 at 18:20:52 UTC}

No significant changes detected, all good. 👏

[coderabbitai]

🧹 Nitpick comments (1)

code/core/src/common/js-package-manager/NPMProxy.test.ts (1)

242-250: ⚡ Quick win

Move executeCommand mock behavior setup into beforeEach for this describe block.

The new tests configure/reset mockedExecuteCommand inline inside each it(...), which conflicts with the Vitest mock-setup rules for this repo. Please centralize reset/default behavior in a local beforeEach, and keep per-test overrides minimal.

As per coding guidelines, "Implement mock behaviors in beforeEach blocks in Vitest tests" and "Avoid inline mock implementations within test cases in Vitest tests".

Also applies to: 270-279, 299-305

🤖 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 `@code/core/src/common/js-package-manager/NPMProxy.test.ts` around lines 242 -
250, Move the mockedExecuteCommand.mockResolvedValue setup out of the individual
test cases and into a beforeEach block at the describe block level. Extract the
common mock behavior configuration (the default response with stdout containing
dependencies and stderr with npm warnings) into the beforeEach, then override
only the specific aspects that differ between individual tests within their
respective it blocks. This ensures mock setup follows the repository's Vitest
conventions where mock behaviors are centralized in beforeEach rather than
configured inline within test cases.

Source: Coding guidelines

🤖 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 `@code/core/src/common/js-package-manager/NPMProxy.test.ts`:
- Around line 242-250: Move the mockedExecuteCommand.mockResolvedValue setup out
of the individual test cases and into a beforeEach block at the describe block
level. Extract the common mock behavior configuration (the default response with
stdout containing dependencies and stderr with npm warnings) into the
beforeEach, then override only the specific aspects that differ between
individual tests within their respective it blocks. This ensures mock setup
follows the repository's Vitest conventions where mock behaviors are centralized
in beforeEach rather than configured inline within test cases.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: bad6a3dd-6891-47d7-a936-511bc791d81a

📥 Commits

Reviewing files that changed from the base of the PR and between 47af599 and 653731a.

📒 Files selected for processing (1)

• code/core/src/common/js-package-manager/NPMProxy.test.ts