feat(cli): improve installer script#131
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds manifest-driven shell and PowerShell bootstrap installers, a runtime CLI install command that manages paths/shims and diagnostics, comprehensive installer tests and packaging changes, standardized skill-markdown formatting and registry/docs updates, and switches Windows upgrade/install snippets to PowerShell; Playwright runner port selection now persists to disk. ChangesCLI installer delivery and runtime install flow
Skills markdown formatting and registry expansion
Windows upgrade command switch & Playwright runner persistence
Sequence Diagram(s)sequenceDiagram
participant User
participant PagesHost
participant InstallerScript
participant InstallerBinary
User->>InstallerScript: curl/irm manifest or install script URL
InstallerScript->>PagesHost: fetch dist/manifest.json
PagesHost-->>InstallerScript: platform filename + checksum
InstallerScript->>InstallerBinary: download binary to temp
InstallerScript->>InstallerScript: verify SHA256
InstallerScript->>InstallerBinary: run binary with "install"
InstallerBinary-->>User: perform install actions
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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 unit tests (beta)
Comment |
github-actions
Bot
added
scope(internals)
scope(cli)
scope(docs)
dependencies
coryrylan
force-pushed
the
topic-cli-skill-fix
branch
from
d7690a9 to
7d8746f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
projects/pages/index.js (1)
17-24:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd the missing
nve-macos-x64artifact to distribution copy list.Line 17 omits
nve-macos-x64, while the generated manifest includesmacos-x64. This breaks the publish contract and can make macOS x64 installs fail at download time.Suggested fix
- for (const bin of ['nve-macos-arm64', 'nve-linux-x64', 'nve-linux-arm64', 'nve-windows-x64']) { + for (const bin of ['nve-macos-arm64', 'nve-macos-x64', 'nve-linux-x64', 'nve-linux-arm64', 'nve-windows-x64']) {🤖 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/pages/index.js` around lines 17 - 24, The for-loop that builds the list of CLI artifacts (the array iterated by "for (const bin of [...] )" that checks existsSync(...) and copies with cpSync(...)) is missing the macOS x64 artifact name; add "nve-macos-x64" to that array so the loop will check and copy the nve-macos-x64 artifact (using the existing existsSync and cpSync logic) alongside the other platform bins.
🤖 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/cli/install.ps1`:
- Around line 19-23: The installer is downloading $entry.filename as-is with
Invoke-WebRequest but Windows artifact names may be emitted without the .exe
suffix (manifest uses 'nve-windows-x64' while actual file is
'nve-windows-x64.exe'), causing 404s; update the script around
Invoke-WebRequest/Get-FileHash to detect Windows targets (eg. using $env:OS or
$IsWindows/$PSVersionTable.OS) and, if the OS is Windows and $entry.filename
does not end with '.exe', append '.exe' to a local download name (e.g.
$downloadName = if ($entry.filename -notlike '*.exe' -and $IsWindows) {
$entry.filename + '.exe' } else { $entry.filename }) and use $downloadName for
Invoke-WebRequest and checksum verification, keeping $entry.filename unchanged
for manifest semantics.
In `@projects/cli/src/install.test.ts`:
- Around line 727-729: The createUnixEnv helper inherits CI from process.env
which can alter test behavior; update createUnixEnv ({ fakeBin, home }:
InstallerContext) to explicitly remove or override CI when building the returned
NodeJS.ProcessEnv (e.g., spread process.env but set CI to undefined or 'false')
so the test environment is deterministic and prompt-dependent branches (like
shouldInstallGlobalElementsSkill) are not short-circuited by the CI variable.
In `@projects/cli/src/utils.test.ts`:
- Around line 543-558: The test "should preserve leading frontmatter when
rendering markdown" only covers LF line endings; add a second assertion (or a
new test case) that uses a markdown string with CRLF line endings (use "\r\n"
between lines in the frontmatter and before the content) and verify
renderResult(markdownWithCRLF) calls marked.parse with '# Authoring' and returns
the same preserved frontmatter + 'parsed markdown' output; update references to
renderResult and the existing expect(marked.parse) and expect(result) checks to
also run against the CRLF variant to lock cross-platform behavior.
In `@projects/cli/src/utils.ts`:
- Line 193: The current frontmatterPattern only matches LF newlines and fails on
CRLF; update the regex used to detect YAML frontmatter (frontmatterPattern) to
accept both LF and CRLF by using \r?\n in place of \n (e.g., change occurrences
like /^(---\n[\s\S]*?\n---)(?:\n+|$)/ to a pattern using \r?\n such as
/^(---\r?\n[\s\S]*?\r?\n---)(?:\r?\n+|$)/) and apply the same \r?\n adjustment
to any other frontmatter/newline regexes in this file (the other patterns
referenced around lines 216-228) so detection/preservation works cross-platform.
In `@projects/internals/tools/src/skills/about.md`:
- Line 1: Change the document title from a level 2 heading to level 1 by
replacing "## NVIDIA Elements Design System" with a top-level heading "# NVIDIA
Elements Design System" so the markdown starts with an H1 as required by the
Vale prose linter and project guidelines; ensure no other heading hierarchy
changes are needed elsewhere in the file.
- Line 3: Update the sentence "Give a high level overview of the NVIDIA Elements
Design System." in about.md by hyphenating the compound adjective: change "high
level overview" to "high-level overview" so it follows the Google style guide
and write-good rules.
In `@projects/internals/tools/src/skills/artifact.md`:
- Line 1: The document heading "Creating an Artifact" is using a level-2
heading; change it to a level-1 heading by replacing "## Creating an Artifact"
with "# Creating an Artifact" so the markdown starts with a top-level title and
complies with the Vale prose linter and project heading conventions.
In `@projects/internals/tools/src/skills/authoring.md`:
- Line 1: Change the document title heading from level 2 to level 1: replace the
"## Authoring Guidelines & Frontend Tasks" heading with "# Authoring Guidelines
& Frontend Tasks" so the file (the heading line containing "Authoring Guidelines
& Frontend Tasks") uses a top-level H1 per the Vale prose linter and project
markdown rules.
In `@projects/internals/tools/src/skills/integration.md`:
- Line 1: Change the first line heading "Creating Starter Project" from an H2 to
an H1 (make it the document title) so the file starts with a top-level heading;
update the heading marker to a single '#' and ensure any other headings remain
properly nested, and add any new technical terms or component names introduced
in this document to the Vale vocabulary
(config/vale/styles/config/vocabularies/Elements/accept.txt) per the project
docs style rules.
---
Outside diff comments:
In `@projects/pages/index.js`:
- Around line 17-24: The for-loop that builds the list of CLI artifacts (the
array iterated by "for (const bin of [...] )" that checks existsSync(...) and
copies with cpSync(...)) is missing the macOS x64 artifact name; add
"nve-macos-x64" to that array so the loop will check and copy the nve-macos-x64
artifact (using the existing existsSync and cpSync logic) alongside the other
platform bins.
🪄 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: fc7f010b-e2c7-4c9f-bafe-a72804609f07
📒 Files selected for processing (30)
projects/cli/README.mdprojects/cli/install.cmdprojects/cli/install.ps1projects/cli/install.shprojects/cli/package.jsonprojects/cli/scripts/write-install-manifest.mjsprojects/cli/src/index.test.tsprojects/cli/src/index.tsprojects/cli/src/install.test.tsprojects/cli/src/install.tsprojects/cli/src/utils.test.tsprojects/cli/src/utils.tsprojects/internals/tools/src/cli/service.tsprojects/internals/tools/src/cli/utils.test.tsprojects/internals/tools/src/cli/utils.tsprojects/internals/tools/src/project/setup-agent.test.tsprojects/internals/tools/src/project/setup-agent.tsprojects/internals/tools/src/skills/about.mdprojects/internals/tools/src/skills/artifact.mdprojects/internals/tools/src/skills/authoring.mdprojects/internals/tools/src/skills/index.test.tsprojects/internals/tools/src/skills/index.tsprojects/internals/tools/src/skills/integration.mdprojects/internals/tools/src/skills/markdown.tsprojects/internals/tools/src/skills/registry.tsprojects/internals/tools/src/skills/service.test.tsprojects/internals/tools/src/skills/service.tsprojects/pages/index.jsprojects/pages/package.jsonprojects/site/src/docs/integrations/shortcodes.js
💤 Files with no reviewable changes (1)
- projects/cli/install.cmd
coryrylan
force-pushed
the
topic-cli-skill-fix
branch
from
7d8746f to
9eefc0e
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (6)
projects/internals/tools/src/skills/artifact.md (1)
1-1:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse level 1 heading for the document title.
Markdown documents should start with a level 1 heading (
#) rather than level 2 (##). As per coding guidelines, markdown files should follow the Google developer documentation style guide and Vale prose linter rules, which require proper heading hierarchy.📝 Proposed fix
-## Creating an Artifact +# Creating an Artifact🤖 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/artifact.md` at line 1, The document currently uses a level 2 heading ("## Creating an Artifact"); update the top-level title to a level 1 heading by replacing "## Creating an Artifact" with "# Creating an Artifact" so the file follows the required heading hierarchy and Google/Vale prose linting rules (ensure the first non-empty line is the level 1 title).Source: Coding guidelines
projects/internals/tools/src/skills/about.md (2)
1-1:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse level 1 heading for the document title.
Markdown documents should start with a level 1 heading (
#) rather than level 2 (##). As per coding guidelines, markdown files should follow the Google developer documentation style guide and Vale prose linter rules, which require proper heading hierarchy.📝 Proposed fix
-## NVIDIA Elements Design System +# NVIDIA Elements Design System🤖 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/about.md` at line 1, Replace the level 2 heading with a level 1 heading by changing the document title line "## NVIDIA Elements Design System" to use a single leading '#' so the file's top line reads "# NVIDIA Elements Design System" to comply with the required heading hierarchy and prose linter rules.Source: Coding guidelines
3-3:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHyphenate compound adjective.
"High level" should be hyphenated as "high-level" when used as a compound adjective modifying "overview". As per coding guidelines, markdown files should follow the Google developer documentation style guide and write-good rules.
📝 Proposed fix
-Give a high level overview of the NVIDIA Elements Design System. +Give a high-level overview of the NVIDIA Elements Design System.🤖 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/about.md` at line 3, Edit the sentence "Give a high level overview of the NVIDIA Elements Design System." and hyphenate the compound adjective by changing "high level" to "high-level" so it reads "Give a high-level overview of the NVIDIA Elements Design System."; update any other occurrences of the unhyphenated phrase in the same file (projects/internals/tools/src/skills/about.md) to follow the same style.Source: Coding guidelines
projects/internals/tools/src/skills/authoring.md (1)
1-1:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse level 1 heading for the document title.
Markdown documents should start with a level 1 heading (
#) rather than level 2 (##). As per coding guidelines, markdown files should follow the Google developer documentation style guide and Vale prose linter rules, which require proper heading hierarchy.📝 Proposed fix
-## Authoring Guidelines & Frontend Tasks +# Authoring Guidelines & Frontend Tasks🤖 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/authoring.md` at line 1, The document uses a level-2 heading ("## Authoring Guidelines & Frontend Tasks") but must start with a level-1 heading; replace the existing "## Authoring Guidelines & Frontend Tasks" heading with a level-1 heading ("# Authoring Guidelines & Frontend Tasks") at the top of the file and ensure the rest of the heading hierarchy follows (subsections remain ## or lower) so the file complies with the Google documentation style and Vale prose linter.Source: Coding guidelines
projects/cli/src/utils.test.ts (1)
543-558:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd a CRLF frontmatter regression case.
Line 543 currently validates only LF input. Add a
\r\nvariant to ensure Windows-origin markdown keeps frontmatter preservation behavior stable.Suggested test addition
it('should preserve leading frontmatter when rendering markdown', async () => { @@ expect(result).toBe(`--- name: "authoring" description: "Authoring guidance" --- parsed markdown`); }); + + it('should preserve leading frontmatter with CRLF newlines', async () => { + const markdown = `---\r +name: "authoring"\r +description: "Authoring guidance"\r +---\r +\r +# Authoring`; + const result = await renderResult(markdown); + expect(marked.parse).toHaveBeenCalledWith('# Authoring'); + expect(result).toContain('parsed markdown'); + });🤖 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/cli/src/utils.test.ts` around lines 543 - 558, The test 'should preserve leading frontmatter when rendering markdown' only checks LF line endings; add a parallel regression case that uses CRLF frontmatter (replace instances of "\n" with "\r\n") to verify renderResult preserves leading frontmatter for Windows-origin files and that marked.parse is called with the stripped markdown body (e.g., '# Authoring'); you can either add a new it(...) block mirroring the existing test or extend the existing one to run both variants, referencing the test name and the renderResult and marked.parse expectations so the assertion still expects the full frontmatter + "\r\n\r\n" + "parsed markdown" output.projects/cli/src/utils.ts (1)
193-193:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFrontmatter detection is LF-only and breaks on CRLF markdown.
Line 193 only matches
\n, so CRLF frontmatter is not detected. That causes Lines 218-228 to skip preservation and parse the whole block incorrectly for Windows-style input.Suggested patch
-const frontmatterPattern = /^(---\n[\s\S]*?\n---)(?:\n+|$)/; +const frontmatterPattern = /^(---\r?\n[\s\S]*?\r?\n---)(?:\r?\n+|$)/;Also applies to: 216-228
🤖 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/cli/src/utils.ts` at line 193, The frontmatter regex frontmatterPattern only matches LF (\\n) and fails on CRLF; update its pattern to accept optional CR before newlines (use \\r?\\n in place of \\n) so it matches both LF and CRLF. Also update any other regexes or string operations in the same area that assume literal "\\n" (the logic around preservation/processing that references frontmatterPattern and the block handling between the frontmatter and the body) to use the CRLF-tolerant pattern or normalize newlines before processing to ensure Windows-style files are detected and preserved correctly.
🤖 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/cli/install.sh`:
- Around line 85-89: The current get_manifest_field function uses fragile sed
parsing; update it to prefer jq when available and fall back to the existing sed
approach: detect jq with command -v jq, and if present parse MANIFEST_JSON using
jq with --arg platform "$PLATFORM_KEY" and --arg field "$field" to extract
.[$platform][$field] (and handle null/empty results), otherwise run the existing
sed extraction as a fallback; keep the function name get_manifest_field and
ensure behavior remains the same when jq is absent.
In `@projects/cli/scripts/write-install-manifest.mjs`:
- Line 13: The manifest entry for the Windows build is missing the .exe
extension and must be updated: locate the object with key 'windows-x64' in
write-install-manifest.mjs and change the filename (and if present the source)
from 'nve-windows-x64' to 'nve-windows-x64.exe' so the emitted manifest matches
the actual hosted artifact and install.ps1 download URLs resolve correctly.
In `@projects/cli/src/install.ts`:
- Around line 299-318: The PowerShell path-comparison in setupWindowsPath only
trims backslashes (`$_.TrimEnd('\\')`) but should remove both backslashes and
forward slashes like the TypeScript normalizePathEntry does; update the
PowerShell script inside setupWindowsPath to trim both characters (e.g., use
TrimEnd('\\','/') for both $_ and $entry) so PATH entries compare correctly
across environments, then run the spawnSync call as before and keep
prependPathEntry(binDir, env, 'win32') intact.
---
Duplicate comments:
In `@projects/cli/src/utils.test.ts`:
- Around line 543-558: The test 'should preserve leading frontmatter when
rendering markdown' only checks LF line endings; add a parallel regression case
that uses CRLF frontmatter (replace instances of "\n" with "\r\n") to verify
renderResult preserves leading frontmatter for Windows-origin files and that
marked.parse is called with the stripped markdown body (e.g., '# Authoring');
you can either add a new it(...) block mirroring the existing test or extend the
existing one to run both variants, referencing the test name and the
renderResult and marked.parse expectations so the assertion still expects the
full frontmatter + "\r\n\r\n" + "parsed markdown" output.
In `@projects/cli/src/utils.ts`:
- Line 193: The frontmatter regex frontmatterPattern only matches LF (\\n) and
fails on CRLF; update its pattern to accept optional CR before newlines (use
\\r?\\n in place of \\n) so it matches both LF and CRLF. Also update any other
regexes or string operations in the same area that assume literal "\\n" (the
logic around preservation/processing that references frontmatterPattern and the
block handling between the frontmatter and the body) to use the CRLF-tolerant
pattern or normalize newlines before processing to ensure Windows-style files
are detected and preserved correctly.
In `@projects/internals/tools/src/skills/about.md`:
- Line 1: Replace the level 2 heading with a level 1 heading by changing the
document title line "## NVIDIA Elements Design System" to use a single leading
'#' so the file's top line reads "# NVIDIA Elements Design System" to comply
with the required heading hierarchy and prose linter rules.
- Line 3: Edit the sentence "Give a high level overview of the NVIDIA Elements
Design System." and hyphenate the compound adjective by changing "high level" to
"high-level" so it reads "Give a high-level overview of the NVIDIA Elements
Design System."; update any other occurrences of the unhyphenated phrase in the
same file (projects/internals/tools/src/skills/about.md) to follow the same
style.
In `@projects/internals/tools/src/skills/artifact.md`:
- Line 1: The document currently uses a level 2 heading ("## Creating an
Artifact"); update the top-level title to a level 1 heading by replacing "##
Creating an Artifact" with "# Creating an Artifact" so the file follows the
required heading hierarchy and Google/Vale prose linting rules (ensure the first
non-empty line is the level 1 title).
In `@projects/internals/tools/src/skills/authoring.md`:
- Line 1: The document uses a level-2 heading ("## Authoring Guidelines &
Frontend Tasks") but must start with a level-1 heading; replace the existing "##
Authoring Guidelines & Frontend Tasks" heading with a level-1 heading ("#
Authoring Guidelines & Frontend Tasks") at the top of the file and ensure the
rest of the heading hierarchy follows (subsections remain ## or lower) so the
file complies with the Google documentation style and Vale prose linter.
🪄 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: d8e584f6-3e05-425c-9963-a8ba0ba66d3a
📒 Files selected for processing (30)
projects/cli/README.mdprojects/cli/install.cmdprojects/cli/install.ps1projects/cli/install.shprojects/cli/package.jsonprojects/cli/scripts/write-install-manifest.mjsprojects/cli/src/index.test.tsprojects/cli/src/index.tsprojects/cli/src/install.test.tsprojects/cli/src/install.tsprojects/cli/src/utils.test.tsprojects/cli/src/utils.tsprojects/internals/tools/src/cli/service.tsprojects/internals/tools/src/cli/utils.test.tsprojects/internals/tools/src/cli/utils.tsprojects/internals/tools/src/project/setup-agent.test.tsprojects/internals/tools/src/project/setup-agent.tsprojects/internals/tools/src/skills/about.mdprojects/internals/tools/src/skills/artifact.mdprojects/internals/tools/src/skills/authoring.mdprojects/internals/tools/src/skills/index.test.tsprojects/internals/tools/src/skills/index.tsprojects/internals/tools/src/skills/integration.mdprojects/internals/tools/src/skills/markdown.tsprojects/internals/tools/src/skills/registry.tsprojects/internals/tools/src/skills/service.test.tsprojects/internals/tools/src/skills/service.tsprojects/pages/index.jsprojects/pages/package.jsonprojects/site/src/docs/integrations/shortcodes.js
💤 Files with no reviewable changes (1)
- projects/cli/install.cmd
Comment on lines
+299
to
+318
| function setupWindowsPath(binDir: string, env: NodeJS.ProcessEnv, warn: (message: string) => void): void { | ||
| const escapedBinDir = powershellSingleQuote(binDir); | ||
| const script = [ | ||
| `$entry = ${escapedBinDir}`, | ||
| "$current = [Environment]::GetEnvironmentVariable('Path', 'User')", | ||
| "$entries = @($current -split ';' | Where-Object { $_ })", | ||
| "if (-not ($entries | Where-Object { $_.TrimEnd('\\') -ieq $entry.TrimEnd('\\') })) {", | ||
| " [Environment]::SetEnvironmentVariable('Path', (@($entry) + $entries) -join ';', 'User')", | ||
| '}' | ||
| ].join('; '); | ||
| const result = spawnSync('powershell', ['-NoProfile', '-ExecutionPolicy', 'Bypass', '-Command', script], { | ||
| encoding: 'utf-8' | ||
| }); | ||
|
|
||
| if (result.status !== 0) { | ||
| warn(`Could not update the user PATH. Add ${binDir} manually.`); | ||
| } | ||
|
|
||
| prependPathEntry(binDir, env, 'win32'); | ||
| } |
There was a problem hiding this comment.
Consider both slash types in PowerShell PATH comparison.
Line 305 uses $_.TrimEnd('\\') to normalize PATH entries before comparison, but on Windows, forward slashes can also appear in PATH (WSL, Git Bash, etc.). The TypeScript normalizePathEntry (line 345) correctly handles both with regex /[\\/]+$/.
🔧 Suggested fix
"$entries = @($current -split ';' | Where-Object { $_ })",
- "if (-not ($entries | Where-Object { $_.TrimEnd('\\') -ieq $entry.TrimEnd('\\') })) {",
+ "if (-not ($entries | Where-Object { ($_ -replace '[/\\\\]+$', '') -ieq ($entry -replace '[/\\\\]+$', '') })) {",
" [Environment]::SetEnvironmentVariable('Path', (@($entry) + $entries) -join ';', 'User')",🤖 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/cli/src/install.ts` around lines 299 - 318, The PowerShell
path-comparison in setupWindowsPath only trims backslashes (`$_.TrimEnd('\\')`)
but should remove both backslashes and forward slashes like the TypeScript
normalizePathEntry does; update the PowerShell script inside setupWindowsPath to
trim both characters (e.g., use TrimEnd('\\','/') for both $_ and $entry) so
PATH entries compare correctly across environments, then run the spawnSync call
as before and keep prependPathEntry(binDir, env, 'win32') intact.
9eefc0e to
79d6179
Compare
coryrylan
commented
Jun 11, 2026
|
|
||
| it('should not show hidden install command', () => { | ||
| expect(output).not.toContain('nve install'); | ||
| }); |
Collaborator
Author
There was a problem hiding this comment.
The nve install command is not something a user would execute but the install.sh script which is essentially a bootstrap step for the install command. This allows the complexity of platform specifics and env setups to be in the cli source code which becomes more testable and maintainable.
coryrylan
commented
Jun 11, 2026
| readPrompt, | ||
| spawnVersion, | ||
| setupWindowsPath | ||
| }; |
Collaborator
Author
There was a problem hiding this comment.
This was a interesting pattern I found, not 100% sold yet but it groups internals for easier testing and a signal for things not exported to other module usage. We typically have done this by creating utils.ts style files
coryrylan
commented
Jun 11, 2026
| - Unix/macOS: \`$NVE_HOME/bin/nve\`, else \`$HOME/.nve/bin/nve\` | ||
| - Windows: \`%NVE_HOME%\\bin\\nve.exe\`, else \`%LOCALAPPDATA%\\nve\\bin\\nve.exe\` | ||
|
|
||
| When a canonical path exists, call it directly, for example \`$HOME/.nve/bin/nve api.list\`. Fall back to \`nve\` on \`PATH\` only for interactive convenience. |
Collaborator
Author
There was a problem hiding this comment.
This helps agents find and use the binary when placed in a sandbox and not reliant on user env settings
coryrylan
commented
Jun 11, 2026
| \`\`\`shell | ||
| curl -fsSL ${ELEMENTS_PAGES_BASE_URL}/install.cmd -o install.cmd && install.cmd && del install.cmd | ||
| \`\`\`powershell | ||
| irm ${ELEMENTS_PAGES_BASE_URL}/install.ps1 | iex |
Collaborator
Author
There was a problem hiding this comment.
improving windows dx switching to powershell
coryrylan
force-pushed
the
topic-cli-skill-fix
branch
from
79d6179 to
8fe0a14
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
projects/cli/src/install.ts (1)
299-318:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPowerShell PATH comparison should trim both slash types.
Line 305 uses
$_.TrimEnd('\\')to normalize PATH entries, but forward slashes can also appear in Windows PATH (from WSL, Git Bash, MSYS2). The TypeScriptnormalizePathEntry(line 345) correctly handles both with regex/[\\/]+$/.🔧 Suggested fix
"$entries = @($current -split ';' | Where-Object { $_ })", - "if (-not ($entries | Where-Object { $_.TrimEnd('\\') -ieq $entry.TrimEnd('\\') })) {", + "if (-not ($entries | Where-Object { ($_ -replace '[/\\\\]+$', '') -ieq ($entry -replace '[/\\\\]+$', '') })) {", " [Environment]::SetEnvironmentVariable('Path', (@($entry) + $entries) -join ';', 'User')",🤖 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/cli/src/install.ts` around lines 299 - 318, The PowerShell comparison in setupWindowsPath uses $_.TrimEnd('\\') which only removes backslashes and misses forward slashes; change the TrimEnd calls in the script built in setupWindowsPath so both PATH entries are normalized by trimming both slash types (i.e., TrimEnd for '/' and '\' on both $_ and $entry) to match normalizePathEntry's behavior; update the single script element that contains "$_.TrimEnd('\\')" and "$entry.TrimEnd('\\')" inside setupWindowsPath to trim both slash characters.projects/cli/install.ps1 (1)
24-24:⚠️ Potential issue | 🟠 Major | ⚡ Quick winChecksum comparison may fail due to case mismatch.
$actualChecksumis lowercased via.ToLowerInvariant(), but$entry.checksumfrom the manifest is compared as-is. If the manifest contains uppercase hex digits, the comparison will incorrectly fail.🐛 Proposed fix
- if ($actualChecksum -ne $entry.checksum) { + if ($actualChecksum -ne $entry.checksum.ToLowerInvariant()) {🤖 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/cli/install.ps1` at line 24, The checksum comparison can fail if the manifest's checksum has uppercase hex digits because only $actualChecksum is lowercased; normalize both sides before comparing by applying ToLowerInvariant() (and optionally Trim()) to $entry.checksum and then use the existing if ($actualChecksum -ne $entry.checksum) check (i.e., replace that comparison with one that compares $actualChecksum to $entry.checksum.ToLowerInvariant() or compute a normalized $manifestChecksum = $entry.checksum.ToLowerInvariant().Trim() and compare $actualChecksum -ne $manifestChecksum).
🤖 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/cli/install.ps1`:
- Line 19: The script assigns to the automatic variable $IsWindows
(case-insensitive), causing runtime errors on PowerShell 6+; rename the local
variable (for example to $runningOnWindows or $is_windows) and compute its value
without reassigning the automatic variable: use ($env:OS -eq 'Windows_NT') -or
($IsWindows -eq $true) -or ($PSVersionTable.OS -like '*Windows*') as the RHS,
assign that to the new variable (e.g., $runningOnWindows) and update any
subsequent references from $isWindows to the new name to avoid clobbering the
read-only automatic variable.
In `@projects/cli/install.sh`:
- Line 68: Replace the non-POSIX fractional sleep call `sleep 0.1` in
projects/cli/install.sh with a portable alternative: either use an
integer-second sleep (e.g., `sleep 1`) for spinner cosmetics, or call a
ubiquitous interpreter to sleep fractional seconds (e.g., `python -c "import
time; time.sleep(0.1)"` or `python3`/`perl` variant) if sub-second timing is
required; update the line containing `sleep 0.1` accordingly and ensure the
chosen interpreter is checked/available before use.
In `@projects/cli/scripts/write-install-manifest.mjs`:
- Line 13: The manifest and build scripts disagree on Windows binary naming:
update write-install-manifest.mjs to emit filename 'nve-windows-x64.exe' (for
the entry with key 'windows-x64'), remove the logic in install.ps1 that appends
".exe" to the filename when constructing the download URL, and change the copy
in pages/index.js so it preserves the .exe (use the existing src path for the
Windows artifact and copy it to ./dist/cli/nve-windows-x64.exe) so all three
places consistently reference the .exe name.
In `@projects/internals/vite/src/runners/playwright.js`:
- Around line 102-103: The startup always calls preview({ ..., preview: { port:
DEFAULT_PORT }}) which ignores the persisted port in this.#portFile; change the
startup logic in the method that assigns this.#server (where preview() is
invoked) to read the stored port from this.#portFile, parse/validate it, and
pass that value as the preview.port option (falling back to DEFAULT_PORT if
invalid or missing), then continue to write the actual bound port back to
this.#portFile (using this.port or server.port) so persisted port reuse works
across runs.
- Around line 50-54: The getter `#storedPort` should validate the parsed port from
this.#portFile before returning it: in get `#storedPort`(), after parsing port
with Number.parseInt(...) and checking Number.isNaN(port), also ensure port is
an integer within the valid TCP port range (1 through 65535); if it is outside
that range (including 0, negative, or >65535) return undefined so callers like
open() don't receive invalid ports. Keep the existing try/catch behavior and
only change the acceptance condition for the parsed port.
---
Duplicate comments:
In `@projects/cli/install.ps1`:
- Line 24: The checksum comparison can fail if the manifest's checksum has
uppercase hex digits because only $actualChecksum is lowercased; normalize both
sides before comparing by applying ToLowerInvariant() (and optionally Trim()) to
$entry.checksum and then use the existing if ($actualChecksum -ne
$entry.checksum) check (i.e., replace that comparison with one that compares
$actualChecksum to $entry.checksum.ToLowerInvariant() or compute a normalized
$manifestChecksum = $entry.checksum.ToLowerInvariant().Trim() and compare
$actualChecksum -ne $manifestChecksum).
In `@projects/cli/src/install.ts`:
- Around line 299-318: The PowerShell comparison in setupWindowsPath uses
$_.TrimEnd('\\') which only removes backslashes and misses forward slashes;
change the TrimEnd calls in the script built in setupWindowsPath so both PATH
entries are normalized by trimming both slash types (i.e., TrimEnd for '/' and
'\' on both $_ and $entry) to match normalizePathEntry's behavior; update the
single script element that contains "$_.TrimEnd('\\')" and
"$entry.TrimEnd('\\')" inside setupWindowsPath to trim both slash characters.
🪄 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: 22c51dc6-59ec-482c-8f1c-ad7ebe928d6a
📒 Files selected for processing (31)
projects/cli/README.mdprojects/cli/install.cmdprojects/cli/install.ps1projects/cli/install.shprojects/cli/package.jsonprojects/cli/scripts/write-install-manifest.mjsprojects/cli/src/index.test.tsprojects/cli/src/index.tsprojects/cli/src/install.test.tsprojects/cli/src/install.tsprojects/cli/src/utils.test.tsprojects/cli/src/utils.tsprojects/internals/tools/src/cli/service.tsprojects/internals/tools/src/cli/utils.test.tsprojects/internals/tools/src/cli/utils.tsprojects/internals/tools/src/project/setup-agent.test.tsprojects/internals/tools/src/project/setup-agent.tsprojects/internals/tools/src/skills/about.mdprojects/internals/tools/src/skills/artifact.mdprojects/internals/tools/src/skills/authoring.mdprojects/internals/tools/src/skills/index.test.tsprojects/internals/tools/src/skills/index.tsprojects/internals/tools/src/skills/integration.mdprojects/internals/tools/src/skills/markdown.tsprojects/internals/tools/src/skills/registry.tsprojects/internals/tools/src/skills/service.test.tsprojects/internals/tools/src/skills/service.tsprojects/internals/vite/src/runners/playwright.jsprojects/pages/index.jsprojects/pages/package.jsonprojects/site/src/docs/integrations/shortcodes.js
💤 Files with no reviewable changes (1)
- projects/cli/install.cmd
| esac | ||
| printf "\r%s %s" "$frame" "$message" >&2 | ||
| frame_index=$(((frame_index + 1) % 4)) | ||
| sleep 0.1 |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Fractional sleep is not POSIX-compliant.
sleep 0.1 requires GNU coreutils or a compatible implementation. On minimal POSIX systems (e.g., BusyBox ash, some BSD variants), this may fail or be ignored. Since this is only for spinner cosmetics and the script targets macOS/Linux where GNU sleep is typical, this is low risk.
🤖 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/cli/install.sh` at line 68, Replace the non-POSIX fractional sleep
call `sleep 0.1` in projects/cli/install.sh with a portable alternative: either
use an integer-second sleep (e.g., `sleep 1`) for spinner cosmetics, or call a
ubiquitous interpreter to sleep fractional seconds (e.g., `python -c "import
time; time.sleep(0.1)"` or `python3`/`perl` variant) if sub-second timing is
required; update the line containing `sleep 0.1` accordingly and ensure the
chosen interpreter is checked/available before use.
8fe0a14 to
e8e8788
Compare
johnyanarella
approved these changes
Jun 11, 2026
There was a problem hiding this comment.
♻️ Duplicate comments (2)
projects/cli/src/utils.ts (1)
193-193:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFrontmatter detection is not cross-platform due to LF-only regex.
The regex only matches
\n, so markdown with Windows newlines (\r\n) won't match and frontmatter won't be preserved.🔧 Suggested fix
-const frontmatterPattern = /^(---\n[\s\S]*?\n---)(?:\n+|$)/; +const frontmatterPattern = /^(---\r?\n[\s\S]*?\r?\n---)(?:\r?\n+|$)/;🤖 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/cli/src/utils.ts` at line 193, The frontmatterPattern regex only accounts for LF (`\n`) and fails on CRLF files; update the pattern used by the frontmatterPattern constant to accept both LF and CRLF (use `\r?\n` where newlines are matched) so the leading `---` block and trailing separator are detected and preserved on Windows as well as Unix. Ensure all `\n` occurrences in the pattern (the opening line break after `---`, the internal content boundary, and the `---` closing line plus the following blank lines group) are replaced with `\r?\n` so the regex works cross-platform.projects/cli/src/utils.test.ts (1)
543-558: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd a CRLF frontmatter test case to lock cross-platform behavior.
The test only covers LF input. Add a
\r\nvariant so Windows-origin markdown is validated and regressions are caught.🧪 Suggested CRLF test case
it('should preserve leading frontmatter when rendering markdown', async () => { const markdown = `--- name: "authoring" description: "Authoring guidance" --- # Authoring`; const result = await renderResult(markdown); expect(marked.parse).toHaveBeenCalledWith('# Authoring'); expect(result).toBe(`--- name: "authoring" description: "Authoring guidance" --- parsed markdown`); }); + + it('should preserve frontmatter with CRLF line endings', async () => { + const markdown = `---\r\nname: "authoring"\r\ndescription: "Authoring guidance"\r\n---\r\n\r\n# Authoring`; + const result = await renderResult(markdown); + expect(marked.parse).toHaveBeenCalledWith('# Authoring'); + expect(result).toContain('name: "authoring"'); + expect(result).toContain('parsed markdown'); + });🤖 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/cli/src/utils.test.ts` around lines 543 - 558, Add a parallel test to the existing "should preserve leading frontmatter when rendering markdown" that uses CRLF line endings: create a new it block (e.g., "should preserve leading frontmatter with CRLF when rendering markdown") that supplies markdown with `\r\n` frontmatter and body to renderResult, assert marked.parse was called with '# Authoring' and assert the returned result preserves the frontmatter using `\r\n` line endings; reference renderResult and marked.parse to locate where to add the CRLF variant so Windows-origin markdown is validated.
🤖 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.
Duplicate comments:
In `@projects/cli/src/utils.test.ts`:
- Around line 543-558: Add a parallel test to the existing "should preserve
leading frontmatter when rendering markdown" that uses CRLF line endings: create
a new it block (e.g., "should preserve leading frontmatter with CRLF when
rendering markdown") that supplies markdown with `\r\n` frontmatter and body to
renderResult, assert marked.parse was called with '# Authoring' and assert the
returned result preserves the frontmatter using `\r\n` line endings; reference
renderResult and marked.parse to locate where to add the CRLF variant so
Windows-origin markdown is validated.
In `@projects/cli/src/utils.ts`:
- Line 193: The frontmatterPattern regex only accounts for LF (`\n`) and fails
on CRLF files; update the pattern used by the frontmatterPattern constant to
accept both LF and CRLF (use `\r?\n` where newlines are matched) so the leading
`---` block and trailing separator are detected and preserved on Windows as well
as Unix. Ensure all `\n` occurrences in the pattern (the opening line break
after `---`, the internal content boundary, and the `---` closing line plus the
following blank lines group) are replaced with `\r?\n` so the regex works
cross-platform.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: eb877aeb-c81a-417c-a542-53587c7dc374
📒 Files selected for processing (32)
projects/cli/README.mdprojects/cli/install.cmdprojects/cli/install.ps1projects/cli/install.shprojects/cli/package.jsonprojects/cli/scripts/write-install-manifest.mjsprojects/cli/src/index.test.tsprojects/cli/src/index.tsprojects/cli/src/install.test.tsprojects/cli/src/install.tsprojects/cli/src/utils.test.tsprojects/cli/src/utils.tsprojects/cli/vite.config.tsprojects/internals/tools/src/cli/service.tsprojects/internals/tools/src/cli/utils.test.tsprojects/internals/tools/src/cli/utils.tsprojects/internals/tools/src/project/setup-agent.test.tsprojects/internals/tools/src/project/setup-agent.tsprojects/internals/tools/src/skills/about.mdprojects/internals/tools/src/skills/artifact.mdprojects/internals/tools/src/skills/authoring.mdprojects/internals/tools/src/skills/index.test.tsprojects/internals/tools/src/skills/index.tsprojects/internals/tools/src/skills/integration.mdprojects/internals/tools/src/skills/markdown.tsprojects/internals/tools/src/skills/registry.tsprojects/internals/tools/src/skills/service.test.tsprojects/internals/tools/src/skills/service.tsprojects/internals/vite/src/runners/playwright.jsprojects/pages/index.jsprojects/pages/package.jsonprojects/site/src/docs/integrations/shortcodes.js
💤 Files with no reviewable changes (2)
- projects/cli/install.cmd
- projects/cli/vite.config.ts
coryrylan
force-pushed
the
topic-cli-skill-fix
branch
from
e8e8788 to
2d6e830
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
projects/cli/package.json (1)
38-38:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winBroken script reference:
nve:installwas removed.The
devscript invokespnpm run nve:install, but according to the AI summary,nve:installwas removed in this PR (replaced bycli:install). This will cause the dev script to fail.🐛 Proposed fix
- "dev": "pnpm run nve:install && npx `@modelcontextprotocol/inspector`@0.22.0 node ./dist/index.js mcp", + "dev": "pnpm run cli:install && npx `@modelcontextprotocol/inspector`@0.22.0 node ./dist/index.js 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/cli/package.json` at line 38, The dev script currently references the removed script name "nve:install"; update the "dev" npm script to call the new "cli:install" script instead (replace occurrences of nve:install with cli:install in the "dev" entry) so the command invoking npx `@modelcontextprotocol/inspector` still runs after installation.
♻️ Duplicate comments (3)
projects/internals/vite/src/runners/playwright.js (2)
50-53:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate persisted port range before accepting file contents.
At Lines 52-53, any parsed integer is accepted. Invalid values (
0, negatives,>65535) can becomethis.portwhen no server is active, yielding invalid local URLs for consumers.Suggested fix
get `#storedPort`() { try { const port = Number.parseInt(fs.readFileSync(this.#portFile, 'utf-8'), 10); - return Number.isNaN(port) ? undefined : port; + return Number.isInteger(port) && port >= 1 && port <= 65535 ? port : undefined; } catch { return undefined; } }🤖 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/vite/src/runners/playwright.js` around lines 50 - 53, The getter get `#storedPort`() currently accepts any parsed integer from fs.readFileSync(this.#portFile), which can yield invalid ports like 0, negatives, or >65535; update it to validate the parsed port is an integer within the valid TCP port range (1–65535) and return undefined if out of range or NaN so that this.port cannot pick up invalid values from the file; modify the logic inside get `#storedPort`() (referencing this.#portFile and the parsed port variable) to perform this range check before returning the port.
102-103:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPersisted port is still not used as startup bind preference.
At Line 102,
preview()is always started withDEFAULT_PORT, so the previously saved port is ignored during server startup. That breaks the persisted-port reuse behavior and can reintroduce churn across runs.Suggested fix
- this.#server = await preview({ root: this.#root, preview: { port: DEFAULT_PORT, open: false } }); + const preferredPort = this.#storedPort ?? DEFAULT_PORT; + this.#server = await preview({ root: this.#root, preview: { port: preferredPort, open: false } }); fs.writeFileSync(this.#portFile, String(this.port));🤖 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/vite/src/runners/playwright.js` around lines 102 - 103, The preview server is always started with DEFAULT_PORT, ignoring any previously persisted port; update the preview(...) call to pass the persisted port (use the existing this.port getter or read from this.#portFile) as the preview.port value falling back to DEFAULT_PORT if absent, so replace preview({ root: this.#root, preview: { port: DEFAULT_PORT, ... } }) with preview({ root: this.#root, preview: { port: Number(this.port) || DEFAULT_PORT, ... } }) and then continue to write the chosen port to this.#portFile as currently done.projects/cli/src/install.ts (1)
305-306:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNormalize both slash styles in Windows PATH de-dup comparison.
Line 305 only trims trailing
\. If a PATH entry ends with/, this comparison treats equivalent paths as different and can prepend duplicates.🔧 Proposed fix
- "if (-not ($entries | Where-Object { $_.TrimEnd('\\') -ieq $entry.TrimEnd('\\') })) {", + "if (-not ($entries | Where-Object { ($_ -replace '[/\\\\]+$', '') -ieq ($entry -replace '[/\\\\]+$', '') })) {",🤖 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/cli/src/install.ts` around lines 305 - 306, The PATH de-dup check only trims trailing backslashes, so entries ending with '/' are considered different; update the comparison in the Where-Object predicate (the expression containing "$_.TrimEnd('\\') -ieq $entry.TrimEnd('\\')") to normalize both slash styles before comparing (e.g., trim both '\' and '/' and/or replace '/' with '\' and use case-insensitive compare) and apply the same normalization when constructing the value passed into SetEnvironmentVariable (the line that builds "(@($entry) + $entries) -join ';'") so duplicates are correctly detected and not prepended.
🤖 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/cli/scripts/write-install-manifest.mjs`:
- Line 13: The manifest lists Windows binaries with filenames that include the
.exe extension (e.g. filename: 'nve-windows-x64.exe') but the distribution step
strips that extension; update the distribution packager so the copied Windows
binary preserves the .exe suffix. Specifically, in the code that calls
cpSync(srcExe, ...) in pages/index.js, change the destination to include `.exe`
(so the distributed file is ./dist/cli/${bin}.exe) to match the manifest entry
emitted by write-install-manifest.mjs.
- Around line 20-25: The code building platforms calls readFileSync on each
platform binary (platformBinaries -> platforms) without checking existence; add
a defensive check around readFileSync (e.g., use existsSync or a try/catch) when
resolving the path with resolve('dist', source) and handle missing files by
logging a clear, actionable error (including the key/source and path) and either
skipping that entry or exiting with a non‑zero status, rather than letting
readFileSync throw; update the platforms creation logic to only include entries
that successfully produce a checksum via createHash('sha256') or to fail fast
with a helpful message.
---
Outside diff comments:
In `@projects/cli/package.json`:
- Line 38: The dev script currently references the removed script name
"nve:install"; update the "dev" npm script to call the new "cli:install" script
instead (replace occurrences of nve:install with cli:install in the "dev" entry)
so the command invoking npx `@modelcontextprotocol/inspector` still runs after
installation.
---
Duplicate comments:
In `@projects/cli/src/install.ts`:
- Around line 305-306: The PATH de-dup check only trims trailing backslashes, so
entries ending with '/' are considered different; update the comparison in the
Where-Object predicate (the expression containing "$_.TrimEnd('\\') -ieq
$entry.TrimEnd('\\')") to normalize both slash styles before comparing (e.g.,
trim both '\' and '/' and/or replace '/' with '\' and use case-insensitive
compare) and apply the same normalization when constructing the value passed
into SetEnvironmentVariable (the line that builds "(@($entry) + $entries) -join
';'") so duplicates are correctly detected and not prepended.
In `@projects/internals/vite/src/runners/playwright.js`:
- Around line 50-53: The getter get `#storedPort`() currently accepts any parsed
integer from fs.readFileSync(this.#portFile), which can yield invalid ports like
0, negatives, or >65535; update it to validate the parsed port is an integer
within the valid TCP port range (1–65535) and return undefined if out of range
or NaN so that this.port cannot pick up invalid values from the file; modify the
logic inside get `#storedPort`() (referencing this.#portFile and the parsed port
variable) to perform this range check before returning the port.
- Around line 102-103: The preview server is always started with DEFAULT_PORT,
ignoring any previously persisted port; update the preview(...) call to pass the
persisted port (use the existing this.port getter or read from this.#portFile)
as the preview.port value falling back to DEFAULT_PORT if absent, so replace
preview({ root: this.#root, preview: { port: DEFAULT_PORT, ... } }) with
preview({ root: this.#root, preview: { port: Number(this.port) || DEFAULT_PORT,
... } }) and then continue to write the chosen port to this.#portFile as
currently done.
🪄 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: d8567cb6-4841-44dc-a149-15b8ff17baef
📒 Files selected for processing (32)
projects/cli/README.mdprojects/cli/install.cmdprojects/cli/install.ps1projects/cli/install.shprojects/cli/package.jsonprojects/cli/scripts/write-install-manifest.mjsprojects/cli/src/index.test.tsprojects/cli/src/index.tsprojects/cli/src/install.test.tsprojects/cli/src/install.tsprojects/cli/src/utils.test.tsprojects/cli/src/utils.tsprojects/cli/vite.config.tsprojects/internals/tools/src/cli/service.tsprojects/internals/tools/src/cli/utils.test.tsprojects/internals/tools/src/cli/utils.tsprojects/internals/tools/src/project/setup-agent.test.tsprojects/internals/tools/src/project/setup-agent.tsprojects/internals/tools/src/skills/about.mdprojects/internals/tools/src/skills/artifact.mdprojects/internals/tools/src/skills/authoring.mdprojects/internals/tools/src/skills/index.test.tsprojects/internals/tools/src/skills/index.tsprojects/internals/tools/src/skills/integration.mdprojects/internals/tools/src/skills/markdown.tsprojects/internals/tools/src/skills/registry.tsprojects/internals/tools/src/skills/service.test.tsprojects/internals/tools/src/skills/service.tsprojects/internals/vite/src/runners/playwright.jsprojects/pages/index.jsprojects/pages/package.jsonprojects/site/src/docs/integrations/shortcodes.js
💤 Files with no reviewable changes (2)
- projects/cli/vite.config.ts
- projects/cli/install.cmd
Comment on lines
+20
to
+25
| const platforms = Object.fromEntries( | ||
| platformBinaries.map(({ key, filename, source }) => { | ||
| const content = readFileSync(resolve('dist', source)); | ||
| return [key, { filename, checksum: createHash('sha256').update(content).digest('hex') }]; | ||
| }) | ||
| ); |
There was a problem hiding this comment.
Missing error handling for non-existent platform binaries.
If a platform binary doesn't exist in dist/, readFileSync will throw an unhandled exception. During development or incomplete builds, this could produce cryptic errors instead of actionable feedback.
🛡️ Proposed defensive check
const platforms = Object.fromEntries(
platformBinaries.map(({ key, filename, source }) => {
+ const binaryPath = resolve('dist', source);
+ if (!existsSync(binaryPath)) {
+ throw new Error(`Platform binary not found: ${binaryPath}`);
+ }
- const content = readFileSync(resolve('dist', source));
+ const content = readFileSync(binaryPath);
return [key, { filename, checksum: createHash('sha256').update(content).digest('hex') }];
})
);🤖 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/cli/scripts/write-install-manifest.mjs` around lines 20 - 25, The
code building platforms calls readFileSync on each platform binary
(platformBinaries -> platforms) without checking existence; add a defensive
check around readFileSync (e.g., use existsSync or a try/catch) when resolving
the path with resolve('dist', source) and handle missing files by logging a
clear, actionable error (including the key/source and path) and either skipping
that entry or exiting with a non‑zero status, rather than letting readFileSync
throw; update the platforms creation logic to only include entries that
successfully produce a checksum via createHash('sha256') or to fail fast with a
helpful message.
4fcc14e to
330fd15
Compare
coryrylan
force-pushed
the
topic-cli-skill-fix
branch
from
330fd15 to
5a3ad01
Compare
- Refactors installer logic out of sh file and into testable cli installer command - Added a new `install.ps1` script for Windows installations using PowerShell. - Updated `install.sh` for improved error handling and user feedback. - Modified `package.json` to reflect new installation commands and paths. - Enhanced README with instructions for the new PowerShell installer. - Updated tests to ensure compatibility with the new installation process. Signed-off-by: Cory Rylan <crylan@nvidia.com>
coryrylan
force-pushed
the
topic-cli-skill-fix
branch
from
5a3ad01 to
06ff33a
Compare
Collaborator
Author
|
🎉 This issue has been resolved in version 2.0.1 🎉 |
Collaborator
Author
|
🎉 This issue has been resolved in version 2.1.0 🎉 |
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
2 participants
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.
install.ps1script for Windows installations using PowerShell.install.shfor improved error handling and user feedback.package.jsonto reflect new installation commands and paths.Summary by CodeRabbit
New Features
Documentation
Tests