fix(cli): handle pnpm build approvals#125
Open
coryrylan wants to merge 2 commits into
Open
Conversation
coryrylan
force-pushed
the
topic-cli-install-fix
branch
from
4aceefd to
403ebe6
Compare
📝 WalkthroughWalkthroughAdds a ChangesMCP configuration and docs
Package manager install/update error handling
Select / Combobox UI and styles
Internals tools tests and utilities
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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 docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@projects/internals/tools/src/project/starters.ts`:
- Around line 280-297: Add JSDoc to the setupStarterNPM function documenting its
error-handling behavior: describe that it runs getRequiredNPMClient and
runPackageManagerInstall, throws an Error with combined stdout/stderr when the
install exits with a non-zero code, and explicitly documents the special-case
that if the combined output contains 'ERR_PNPM_IGNORED_BUILDS' the function will
log a warning and return instead of throwing; reference the npmClient, code,
stdout, stderr variables and the ERR_PNPM_IGNORED_BUILDS sentinel in the JSDoc
so maintainers understand this conditional flow.
- Around line 299-314: runPackageManagerInstall currently accumulates child
output in memory but doesn't stream it to the console, making installs look
frozen; modify the handler that creates the child (via execPackageManager) to
also pipe real‑time output by writing child.stdout data to process.stdout (e.g.
process.stdout.write(data.toString())) and child.stderr data to process.stderr
while still appending to the stdout/stderr buffers, and guard for nullable
child.stdout/child.stderr to avoid crashes.
In `@projects/internals/tools/src/project/update.ts`:
- Around line 94-97: The pnpm special-case currently checks
output.includes('ERR_PNPM_IGNORED_BUILDS') which is fine but should be made
slightly more robust: replace the substring check with a word-boundary regex
test (e.g., /\bERR_PNPM_IGNORED_BUILDS\b/). Update the check in the code paths
that inspect getExecFileSyncOutput(e) (see the packageManager === 'pnpm' branch
in the function in projects/internals/tools/src/project/update.ts and the
analogous branch in projects/internals/tools/src/project/starters.ts) to use the
regex test on the output string; keep the existing behavior (return null) when
the regex matches. Ensure tests in
projects/internals/tools/src/project/update.test.ts continue to pass or update
them to assert against the regex if necessary.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 83398f28-cb28-4920-a74c-3c12c98bce81
📒 Files selected for processing (8)
projects/cli/README.mdprojects/internals/tools/src/project/setup-agent.test.tsprojects/internals/tools/src/project/setup-agent.tsprojects/internals/tools/src/project/starters.tsprojects/internals/tools/src/project/update.test.tsprojects/internals/tools/src/project/update.tsprojects/internals/tools/src/skills/doctor.mdprojects/site/src/docs/mcp/index.md
Comment on lines
+280
to
297
| async function setupStarterNPM(cwd: string) { | ||
| console.log('📦 Installing dependencies...'); | ||
| const npmClient = await getRequiredNPMClient(); | ||
| const { code, stdout, stderr } = await runPackageManagerInstall(npmClient, cwd); | ||
|
|
||
| if (code === 0) { | ||
| return; | ||
| } | ||
|
|
||
| const output = `${stdout}\n${stderr}`; | ||
| if (output.includes('ERR_PNPM_IGNORED_BUILDS')) { | ||
| console.log('⚠️ Some dependency build scripts were skipped. Run "pnpm approve-builds" if needed.'); | ||
| return; | ||
| } | ||
|
|
||
| const message = output.trim() || `${npmClient} install exited with code ${code}`; | ||
| throw new Error(message); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Add JSDoc documentation for error handling behavior.
The function has special-case error handling for pnpm's ERR_PNPM_IGNORED_BUILDS. Consider adding JSDoc to document this behavior for maintainers.
📝 Suggested documentation
+/**
+ * Installs npm dependencies for a starter project.
+ * `@param` cwd - The working directory containing package.json
+ * `@throws` {Error} If installation fails (except for pnpm's ERR_PNPM_IGNORED_BUILDS)
+ * `@remarks` When using pnpm, build script approval errors are logged as warnings but do not fail the setup
+ */
async function setupStarterNPM(cwd: string) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async function setupStarterNPM(cwd: string) { | |
| console.log('📦 Installing dependencies...'); | |
| const npmClient = await getRequiredNPMClient(); | |
| const { code, stdout, stderr } = await runPackageManagerInstall(npmClient, cwd); | |
| if (code === 0) { | |
| return; | |
| } | |
| const output = `${stdout}\n${stderr}`; | |
| if (output.includes('ERR_PNPM_IGNORED_BUILDS')) { | |
| console.log('⚠️ Some dependency build scripts were skipped. Run "pnpm approve-builds" if needed.'); | |
| return; | |
| } | |
| const message = output.trim() || `${npmClient} install exited with code ${code}`; | |
| throw new Error(message); | |
| } | |
| /** | |
| * Installs npm dependencies for a starter project. | |
| * `@param` cwd - The working directory containing package.json | |
| * `@throws` {Error} If installation fails (except for pnpm's ERR_PNPM_IGNORED_BUILDS) | |
| * `@remarks` When using pnpm, build script approval errors are logged as warnings but do not fail the setup | |
| */ | |
| async function setupStarterNPM(cwd: string) { | |
| console.log('📦 Installing dependencies...'); | |
| const npmClient = await getRequiredNPMClient(); | |
| const { code, stdout, stderr } = await runPackageManagerInstall(npmClient, cwd); | |
| if (code === 0) { | |
| return; | |
| } | |
| const output = `${stdout}\n${stderr}`; | |
| if (output.includes('ERR_PNPM_IGNORED_BUILDS')) { | |
| console.log('⚠️ Some dependency build scripts were skipped. Run "pnpm approve-builds" if needed.'); | |
| return; | |
| } | |
| const message = output.trim() || `${npmClient} install exited with code ${code}`; | |
| throw new Error(message); | |
| } |
🤖 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 `@projects/internals/tools/src/project/starters.ts` around lines 280 - 297, Add
JSDoc to the setupStarterNPM function documenting its error-handling behavior:
describe that it runs getRequiredNPMClient and runPackageManagerInstall, throws
an Error with combined stdout/stderr when the install exits with a non-zero
code, and explicitly documents the special-case that if the combined output
contains 'ERR_PNPM_IGNORED_BUILDS' the function will log a warning and return
instead of throwing; reference the npmClient, code, stdout, stderr variables and
the ERR_PNPM_IGNORED_BUILDS sentinel in the JSDoc so maintainers understand this
conditional flow.
Comment on lines
+299
to
314
| function runPackageManagerInstall(npmClient: NPMClient, cwd: string) { | ||
| return new Promise<{ code: number; stdout: string; stderr: string }>((resolve, reject) => { | ||
| const child = execPackageManager(npmClient, ['install'], cwd); | ||
| let stdout = ''; | ||
| let stderr = ''; | ||
|
|
||
| child.stdout?.on('data', (data: Buffer) => { | ||
| stdout += data.toString(); | ||
| }); | ||
| child.stderr?.on('data', (data: Buffer) => { | ||
| stderr += data.toString(); | ||
| }); | ||
| child.on('close', code => resolve({ code: code ?? 1, stdout, stderr })); | ||
| child.on('error', reject); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 🏗️ Heavy lift
Consider streaming output to console for better user experience.
The runPackageManagerInstall function captures stdout/stderr in memory but doesn't pipe output to the console in real-time. Users won't see installation progress messages from npm/pnpm during the install, which could make the CLI appear frozen during long installations.
Consider piping child process stdio while still capturing output:
💡 Suggested improvement
function runPackageManagerInstall(npmClient: NPMClient, cwd: string) {
return new Promise<{ code: number; stdout: string; stderr: string }>((resolve, reject) => {
- const child = execPackageManager(npmClient, ['install'], cwd);
+ const child = execFile(npmClient === 'pnpm' ? 'pnpm' : 'npm', ['install'], {
+ cwd,
+ stdio: ['inherit', 'pipe', 'pipe']
+ });
let stdout = '';
let stderr = '';
child.stdout?.on('data', (data: Buffer) => {
stdout += data.toString();
+ process.stdout.write(data);
});
child.stderr?.on('data', (data: Buffer) => {
stderr += data.toString();
+ process.stderr.write(data);
});
child.on('close', code => resolve({ code: code ?? 1, stdout, stderr }));
child.on('error', reject);
});
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function runPackageManagerInstall(npmClient: NPMClient, cwd: string) { | |
| return new Promise<{ code: number; stdout: string; stderr: string }>((resolve, reject) => { | |
| const child = execPackageManager(npmClient, ['install'], cwd); | |
| let stdout = ''; | |
| let stderr = ''; | |
| child.stdout?.on('data', (data: Buffer) => { | |
| stdout += data.toString(); | |
| }); | |
| child.stderr?.on('data', (data: Buffer) => { | |
| stderr += data.toString(); | |
| }); | |
| child.on('close', code => resolve({ code: code ?? 1, stdout, stderr })); | |
| child.on('error', reject); | |
| }); | |
| } | |
| function runPackageManagerInstall(npmClient: NPMClient, cwd: string) { | |
| return new Promise<{ code: number; stdout: string; stderr: string }>((resolve, reject) => { | |
| const child = execPackageManager(npmClient, ['install'], cwd); | |
| let stdout = ''; | |
| let stderr = ''; | |
| child.stdout?.on('data', (data: Buffer) => { | |
| stdout += data.toString(); | |
| process.stdout.write(data); | |
| }); | |
| child.stderr?.on('data', (data: Buffer) => { | |
| stderr += data.toString(); | |
| process.stderr.write(data); | |
| }); | |
| child.on('close', code => resolve({ code: code ?? 1, stdout, stderr })); | |
| child.on('error', reject); | |
| }); | |
| } |
🤖 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 `@projects/internals/tools/src/project/starters.ts` around lines 299 - 314,
runPackageManagerInstall currently accumulates child output in memory but
doesn't stream it to the console, making installs look frozen; modify the
handler that creates the child (via execPackageManager) to also pipe real‑time
output by writing child.stdout data to process.stdout (e.g.
process.stdout.write(data.toString())) and child.stderr data to process.stderr
while still appending to the stdout/stderr buffers, and guard for nullable
child.stdout/child.stderr to avoid crashes.
403ebe6 to
9814a63
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@projects/internals/tools/src/project/setup-agent.test.ts`:
- Around line 174-176: Add the same description assertion to the overwrite-case
test ("should overwrite existing elements entry in TOML config"): in that test
where the variable written (the TOML output) is asserted, add
expect(written).toContain('description = "NVIDIA Elements UI Design System
(nve-*), custom element schemas, APIs and examples"') so the overwrite path
validates the description replacement; locate the test block by its name and the
existing assertions on written and insert this additional expect alongside them.
In `@projects/internals/tools/src/skills/doctor.md`:
- Around line 50-53: The MCP command snippets are inconsistent: some use the
split form command = "nve" with args = ["mcp"] while the Cursor snippet uses the
combined string "command": "nve mcp"; update the Cursor snippet to the canonical
split shape by replacing the combined "command": "nve mcp" entry with separate
command = "nve" and args = ["mcp"] (or conversely convert all split snippets to
the single-string form if you prefer that canonical style) so every snippet uses
the same format (match the existing command = "nve" and args = ["mcp"] pattern).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: a9cdf18a-12e6-4578-b536-8e7c488fb4e9
📒 Files selected for processing (11)
projects/cli/README.mdprojects/core/src/combobox/combobox.tsprojects/core/src/select/select.global.cssprojects/core/src/select/select.tsprojects/internals/tools/src/project/setup-agent.test.tsprojects/internals/tools/src/project/setup-agent.tsprojects/internals/tools/src/project/starters.tsprojects/internals/tools/src/project/update.test.tsprojects/internals/tools/src/project/update.tsprojects/internals/tools/src/skills/doctor.mdprojects/site/src/docs/mcp/index.md
coryrylan
force-pushed
the
topic-cli-install-fix
branch
from
9814a63 to
29dce7b
Compare
Signed-off-by: Cory Rylan <crylan@nvidia.com>
Signed-off-by: Cory Rylan <crylan@nvidia.com>
coryrylan
force-pushed
the
topic-cli-install-fix
branch
from
29dce7b to
b01a0dc
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
projects/internals/tools/src/skills/doctor.md (1)
50-53:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStandardize MCP command format across all snippets.
The past review comment remains valid. The Cursor snippet (line 22) uses
"command": "nve mcp"as a single string, while Claude Code (lines 37-38) and Codex (lines 51-52) use the split form with separatecommandandargs. This inconsistency can confuse users. Update the Cursor snippet to use the canonical split form:"command": "nve"with"args": ["mcp"].♻️ Proposed fix
"mcpServers": { "elements": { "description": "NVIDIA Elements UI Design System (nve-*), custom element schemas, APIs and examples", - "command": "nve mcp" + "command": "nve", + "args": ["mcp"] } }🤖 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 `@projects/internals/tools/src/skills/doctor.md` around lines 50 - 53, Update the Cursor snippet so its command is split into the canonical form: replace the single-string entry "command": "nve mcp" with "command": "nve" and add an "args": ["mcp"] array; ensure the snippet matches the other examples (Claude Code and Codex) by using the separate command and args fields.projects/internals/tools/src/project/setup-agent.test.ts (1)
197-209:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd description assertion to the overwrite test.
The past review comment is still valid. The "should overwrite existing elements entry in TOML config" test validates
commandbut notdescription. Add the same description assertion here to ensure the overwrite path correctly replaces the description field.✅ Proposed fix
const written = vi.mocked(writeFileSync).mock.calls[0][1] as string; expect(written).toContain('command = "nve"'); expect(written).not.toContain('command = "old-cmd"'); + expect(written).toContain( + 'description = "NVIDIA Elements UI Design System (nve-*), custom element schemas, APIs and examples"' + );🤖 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 `@projects/internals/tools/src/project/setup-agent.test.ts` around lines 197 - 209, The test "should overwrite existing elements entry in TOML config" in setup-agent.test.ts currently asserts the command but not the description; update the test around the writeMcpTomlConfig('/project/.codex/config.toml') call to also assert that the written TOML contains the new description (e.g., expect(written).toContain('description = "nve"') or the actual new description value used by writeMcpTomlConfig) and does not contain the old description (expect(written).not.toContain('description = "old-desc"')), using the same vi.mocked(writeFileSync).mock.calls[0][1] string you already read for command assertions.
🤖 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 `@projects/internals/tools/src/internal/node.test.ts`:
- Around line 217-227: The test calls vi.stubEnv('PATH', ...) but calls
vi.unstubAllEnvs() inline, which won't run if the assertion fails; update the
test for findExecutablesOnPath so environment restoration always runs (e.g.,
wrap the test body in try { ... } finally { vi.unstubAllEnvs(); } or use an
afterEach to call vi.unstubAllEnvs()) ensuring the stubbed PATH is always
cleared even on failures; reference the test that imports and calls
findExecutablesOnPath and the use of vi.stubEnv/vi.unstubAllEnvs.
- Around line 190-202: Wrap the PATHEXT test so vi.unstubAllEnvs() always runs:
in the test that imports findExecutablesOnPath and stubs PATHEXT, either wrap
the test body in a try-finally where vi.unstubAllEnvs() is called in finally, or
move the cleanup to an afterEach scoped to this describe block (ensuring
vi.stubEnv('PATHEXT', ...) remains in the test). This ensures environment
cleanup via vi.unstubAllEnvs() even if the assertion on result.some(...) fails.
In `@projects/internals/tools/src/packages/utils.test.ts`:
- Around line 221-227: The test for getVersions is too weak; update it to assert
specific behavior when ProjectsService.getData returns undefined by either (A)
mocking getLatestPublishedVersions so when called with an empty array it returns
an empty object and then asserting result equals that empty object, or (B)
asserting that getLatestPublishedVersions was called with [] and that
getVersions returns the expected empty-package structure; adjust mocks for
ProjectsService.getData (use ProjectsService.getData -> undefined) and the
global getLatestPublishedVersions mock accordingly and replace
expect(result).toBeDefined() with an assertion that checks the exact returned
value or call arguments.
In `@projects/internals/tools/src/project/starters.ts`:
- Around line 289-292: The code currently treats any output containing
'ERR_PNPM_IGNORED_BUILDS' as non-fatal regardless of package manager; change the
check that uses the constructed output variable (output) so it only suppresses
the error when the installer is pnpm. Specifically, in the function that builds
output (where output = `${stdout}\n${stderr}`) and returns early on
'ERR_PNPM_IGNORED_BUILDS', add a guard that verifies the package manager is
pnpm—either by checking a local packageManager parameter/option if one exists or
by inspecting process.env.npm_config_user_agent (contains 'pnpm')—and only log
the warning and return when that pnpm check is true. Ensure the comparison
targets the exact token 'ERR_PNPM_IGNORED_BUILDS' and does not change behavior
for npm or yarn.
---
Duplicate comments:
In `@projects/internals/tools/src/project/setup-agent.test.ts`:
- Around line 197-209: The test "should overwrite existing elements entry in
TOML config" in setup-agent.test.ts currently asserts the command but not the
description; update the test around the
writeMcpTomlConfig('/project/.codex/config.toml') call to also assert that the
written TOML contains the new description (e.g.,
expect(written).toContain('description = "nve"') or the actual new description
value used by writeMcpTomlConfig) and does not contain the old description
(expect(written).not.toContain('description = "old-desc"')), using the same
vi.mocked(writeFileSync).mock.calls[0][1] string you already read for command
assertions.
In `@projects/internals/tools/src/skills/doctor.md`:
- Around line 50-53: Update the Cursor snippet so its command is split into the
canonical form: replace the single-string entry "command": "nve mcp" with
"command": "nve" and add an "args": ["mcp"] array; ensure the snippet matches
the other examples (Claude Code and Codex) by using the separate command and
args fields.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: d5054230-eee8-4d82-8cb7-1504c3b41312
📒 Files selected for processing (18)
projects/cli/README.mdprojects/core/src/combobox/combobox.test.tsprojects/core/src/combobox/combobox.tsprojects/core/src/select/select.global.cssprojects/core/src/select/select.tsprojects/internals/tools/src/api/utils.test.tsprojects/internals/tools/src/distill/examples.test.tsprojects/internals/tools/src/internal/node.test.tsprojects/internals/tools/src/internal/utils.test.tsprojects/internals/tools/src/packages/utils.test.tsprojects/internals/tools/src/project/setup-agent.test.tsprojects/internals/tools/src/project/setup-agent.tsprojects/internals/tools/src/project/starters.test.tsprojects/internals/tools/src/project/starters.tsprojects/internals/tools/src/project/update.test.tsprojects/internals/tools/src/project/update.tsprojects/internals/tools/src/skills/doctor.mdprojects/site/src/docs/mcp/index.md
Comment on lines
+190
to
+202
| it('should fall back to the PATHEXT environment variable on win32', async () => { | ||
| const { existsSync, statSync, realpathSync } = await import('node:fs'); | ||
| vi.mocked(existsSync).mockReturnValue(true); | ||
| vi.mocked(statSync).mockReturnValue({ isFile: () => true } as ReturnType<typeof statSync>); | ||
| vi.mocked(realpathSync).mockImplementation(path => path.toString()); | ||
| vi.stubEnv('PATHEXT', '.BAT'); | ||
|
|
||
| const { findExecutablesOnPath } = await import('./node.js'); | ||
| const result = findExecutablesOnPath('nve', { envPath: 'C:/bin', platform: 'win32' }); | ||
|
|
||
| expect(result.some(commandPath => commandPath.toLowerCase().endsWith('nve.bat'))).toBe(true); | ||
| vi.unstubAllEnvs(); | ||
| }); |
There was a problem hiding this comment.
Consider moving environment cleanup to ensure it always executes.
The inline vi.unstubAllEnvs() call at line 201 won't execute if the assertion at line 200 fails, potentially leaving environment pollution for subsequent tests.
🧪 Recommended pattern for guaranteed cleanup
Move the cleanup to a try-finally block or use Vitest's afterEach:
Option 1: try-finally pattern
it('should fall back to the PATHEXT environment variable on win32', async () => {
const { existsSync, statSync, realpathSync } = await import('node:fs');
vi.mocked(existsSync).mockReturnValue(true);
vi.mocked(statSync).mockReturnValue({ isFile: () => true } as ReturnType<typeof statSync>);
vi.mocked(realpathSync).mockImplementation(path => path.toString());
vi.stubEnv('PATHEXT', '.BAT');
- const { findExecutablesOnPath } = await import('./node.js');
- const result = findExecutablesOnPath('nve', { envPath: 'C:/bin', platform: 'win32' });
-
- expect(result.some(commandPath => commandPath.toLowerCase().endsWith('nve.bat'))).toBe(true);
- vi.unstubAllEnvs();
+ try {
+ const { findExecutablesOnPath } = await import('./node.js');
+ const result = findExecutablesOnPath('nve', { envPath: 'C:/bin', platform: 'win32' });
+
+ expect(result.some(commandPath => commandPath.toLowerCase().endsWith('nve.bat'))).toBe(true);
+ } finally {
+ vi.unstubAllEnvs();
+ }
});Option 2: Scoped afterEach (if multiple tests need the same cleanup)
describe('win32 PATHEXT tests', () => {
afterEach(() => {
vi.unstubAllEnvs();
});
it('should fall back to the PATHEXT environment variable on win32', async () => {
// ... test without inline cleanup
});
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it('should fall back to the PATHEXT environment variable on win32', async () => { | |
| const { existsSync, statSync, realpathSync } = await import('node:fs'); | |
| vi.mocked(existsSync).mockReturnValue(true); | |
| vi.mocked(statSync).mockReturnValue({ isFile: () => true } as ReturnType<typeof statSync>); | |
| vi.mocked(realpathSync).mockImplementation(path => path.toString()); | |
| vi.stubEnv('PATHEXT', '.BAT'); | |
| const { findExecutablesOnPath } = await import('./node.js'); | |
| const result = findExecutablesOnPath('nve', { envPath: 'C:/bin', platform: 'win32' }); | |
| expect(result.some(commandPath => commandPath.toLowerCase().endsWith('nve.bat'))).toBe(true); | |
| vi.unstubAllEnvs(); | |
| }); | |
| it('should fall back to the PATHEXT environment variable on win32', async () => { | |
| const { existsSync, statSync, realpathSync } = await import('node:fs'); | |
| vi.mocked(existsSync).mockReturnValue(true); | |
| vi.mocked(statSync).mockReturnValue({ isFile: () => true } as ReturnType<typeof statSync>); | |
| vi.mocked(realpathSync).mockImplementation(path => path.toString()); | |
| vi.stubEnv('PATHEXT', '.BAT'); | |
| try { | |
| const { findExecutablesOnPath } = await import('./node.js'); | |
| const result = findExecutablesOnPath('nve', { envPath: 'C:/bin', platform: 'win32' }); | |
| expect(result.some(commandPath => commandPath.toLowerCase().endsWith('nve.bat'))).toBe(true); | |
| } finally { | |
| vi.unstubAllEnvs(); | |
| } | |
| }); |
🤖 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 `@projects/internals/tools/src/internal/node.test.ts` around lines 190 - 202,
Wrap the PATHEXT test so vi.unstubAllEnvs() always runs: in the test that
imports findExecutablesOnPath and stubs PATHEXT, either wrap the test body in a
try-finally where vi.unstubAllEnvs() is called in finally, or move the cleanup
to an afterEach scoped to this describe block (ensuring vi.stubEnv('PATHEXT',
...) remains in the test). This ensures environment cleanup via
vi.unstubAllEnvs() even if the assertion on result.some(...) fails.
Comment on lines
+217
to
+227
| it('should default to process PATH and platform when options are omitted', async () => { | ||
| const { existsSync } = await import('node:fs'); | ||
| vi.mocked(existsSync).mockReturnValue(false); | ||
| vi.stubEnv('PATH', '/usr/bin:/bin'); | ||
|
|
||
| const { findExecutablesOnPath } = await import('./node.js'); | ||
| const result = findExecutablesOnPath('definitely-not-a-real-command'); | ||
|
|
||
| expect(result).toEqual([]); | ||
| vi.unstubAllEnvs(); | ||
| }); |
There was a problem hiding this comment.
Consider moving environment cleanup to ensure it always executes.
The inline vi.unstubAllEnvs() call at line 226 won't execute if the assertion at line 225 fails, potentially leaving environment pollution for subsequent tests. This is the same pattern as the test at lines 190-202.
🧪 Recommended pattern for guaranteed cleanup
Use a try-finally block:
it('should default to process PATH and platform when options are omitted', async () => {
const { existsSync } = await import('node:fs');
vi.mocked(existsSync).mockReturnValue(false);
vi.stubEnv('PATH', '/usr/bin:/bin');
- const { findExecutablesOnPath } = await import('./node.js');
- const result = findExecutablesOnPath('definitely-not-a-real-command');
-
- expect(result).toEqual([]);
- vi.unstubAllEnvs();
+ try {
+ const { findExecutablesOnPath } = await import('./node.js');
+ const result = findExecutablesOnPath('definitely-not-a-real-command');
+
+ expect(result).toEqual([]);
+ } finally {
+ vi.unstubAllEnvs();
+ }
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it('should default to process PATH and platform when options are omitted', async () => { | |
| const { existsSync } = await import('node:fs'); | |
| vi.mocked(existsSync).mockReturnValue(false); | |
| vi.stubEnv('PATH', '/usr/bin:/bin'); | |
| const { findExecutablesOnPath } = await import('./node.js'); | |
| const result = findExecutablesOnPath('definitely-not-a-real-command'); | |
| expect(result).toEqual([]); | |
| vi.unstubAllEnvs(); | |
| }); | |
| it('should default to process PATH and platform when options are omitted', async () => { | |
| const { existsSync } = await import('node:fs'); | |
| vi.mocked(existsSync).mockReturnValue(false); | |
| vi.stubEnv('PATH', '/usr/bin:/bin'); | |
| try { | |
| const { findExecutablesOnPath } = await import('./node.js'); | |
| const result = findExecutablesOnPath('definitely-not-a-real-command'); | |
| expect(result).toEqual([]); | |
| } finally { | |
| vi.unstubAllEnvs(); | |
| } | |
| }); |
🤖 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 `@projects/internals/tools/src/internal/node.test.ts` around lines 217 - 227,
The test calls vi.stubEnv('PATH', ...) but calls vi.unstubAllEnvs() inline,
which won't run if the assertion fails; update the test for
findExecutablesOnPath so environment restoration always runs (e.g., wrap the
test body in try { ... } finally { vi.unstubAllEnvs(); } or use an afterEach to
call vi.unstubAllEnvs()) ensuring the stubbed PATH is always cleared even on
failures; reference the test that imports and calls findExecutablesOnPath and
the use of vi.stubEnv/vi.unstubAllEnvs.
Comment on lines
+221
to
+227
| it('should default to an empty package list when metadata is missing', async () => { | ||
| const { ProjectsService } = await import('@internals/metadata'); | ||
| vi.mocked(ProjectsService.getData).mockResolvedValueOnce(undefined as never); | ||
|
|
||
| const result = await getVersions(); | ||
| expect(result).toBeDefined(); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Consider a more specific assertion.
The assertion expect(result).toBeDefined() is weak and doesn't validate the actual behavior when metadata is missing. The global mock for getLatestPublishedVersions (lines 41-45) returns a hardcoded object regardless of input, so this test passes but doesn't confirm that passing an empty array [] to getLatestPublishedVersions produces the expected result.
💡 Suggested improvement
it('should default to an empty package list when metadata is missing', async () => {
const { ProjectsService } = await import('`@internals/metadata`');
vi.mocked(ProjectsService.getData).mockResolvedValueOnce(undefined as never);
const result = await getVersions();
- expect(result).toBeDefined();
+ expect(result).toEqual({});
});Or mock getLatestPublishedVersions to return an empty object when called with an empty array in this specific test.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it('should default to an empty package list when metadata is missing', async () => { | |
| const { ProjectsService } = await import('@internals/metadata'); | |
| vi.mocked(ProjectsService.getData).mockResolvedValueOnce(undefined as never); | |
| const result = await getVersions(); | |
| expect(result).toBeDefined(); | |
| }); | |
| it('should default to an empty package list when metadata is missing', async () => { | |
| const { ProjectsService } = await import('`@internals/metadata`'); | |
| vi.mocked(ProjectsService.getData).mockResolvedValueOnce(undefined as never); | |
| const result = await getVersions(); | |
| expect(result).toEqual({}); | |
| }); |
🤖 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 `@projects/internals/tools/src/packages/utils.test.ts` around lines 221 - 227,
The test for getVersions is too weak; update it to assert specific behavior when
ProjectsService.getData returns undefined by either (A) mocking
getLatestPublishedVersions so when called with an empty array it returns an
empty object and then asserting result equals that empty object, or (B)
asserting that getLatestPublishedVersions was called with [] and that
getVersions returns the expected empty-package structure; adjust mocks for
ProjectsService.getData (use ProjectsService.getData -> undefined) and the
global getLatestPublishedVersions mock accordingly and replace
expect(result).toBeDefined() with an assertion that checks the exact returned
value or call arguments.
Comment on lines
+289
to
+292
| const output = `${stdout}\n${stderr}`; | ||
| if (output.includes('ERR_PNPM_IGNORED_BUILDS')) { | ||
| console.log('⚠️ Some dependency build scripts were skipped. Run "pnpm approve-builds" if needed.'); | ||
| return; |
There was a problem hiding this comment.
Gate ignored-build suppression to pnpm only.
This branch currently suppresses failure solely by matching output text. In npm mode, a failed install can be incorrectly treated as success if that token appears in logs.
💡 Proposed fix
- if (output.includes('ERR_PNPM_IGNORED_BUILDS')) {
+ if (npmClient === 'pnpm' && output.includes('ERR_PNPM_IGNORED_BUILDS')) {
console.log('⚠️ Some dependency build scripts were skipped. Run "pnpm approve-builds" if needed.');
return;
}🤖 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 `@projects/internals/tools/src/project/starters.ts` around lines 289 - 292, The
code currently treats any output containing 'ERR_PNPM_IGNORED_BUILDS' as
non-fatal regardless of package manager; change the check that uses the
constructed output variable (output) so it only suppresses the error when the
installer is pnpm. Specifically, in the function that builds output (where
output = `${stdout}\n${stderr}`) and returns early on 'ERR_PNPM_IGNORED_BUILDS',
add a guard that verifies the package manager is pnpm—either by checking a local
packageManager parameter/option if one exists or by inspecting
process.env.npm_config_user_agent (contains 'pnpm')—and only log the warning and
return when that pnpm check is true. Ensure the comparison targets the exact
token 'ERR_PNPM_IGNORED_BUILDS' and does not change behavior for npm or yarn.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Documentation
Bug Fixes
Style
Tests