fix(cli): version and updater checks#155
Conversation
📝 WalkthroughWalkthroughThe PR centralizes Elements environment injection, updates CLI build metadata generation, refactors CLI exit handling, tightens update checks, and changes global skill installation to overwrite existing files without prompting. ChangesRelease and CLI flow
Sequence Diagram(s)Build manifest flow sequenceDiagram
participant WriteInstallManifest
participant DistIndex
participant DistManifest
WriteInstallManifest->>DistIndex: read bundled BUILD_SHA and VERSION
WriteInstallManifest->>DistManifest: write { sha, version, platforms }
CLI completion flow sequenceDiagram
participant CLIEntrypoint
participant runAsyncTool
participant exitWithCompleteToolResult
participant notifyUpdate
participant ProcessExit
CLIEntrypoint->>runAsyncTool: upgrade command with interactiveProgress: false
runAsyncTool-->>CLIEntrypoint: result
CLIEntrypoint->>exitWithCompleteToolResult: result, tool, start, end, notifyUpdate
exitWithCompleteToolResult->>notifyUpdate: notifyUpdate() when exitCode is 0
exitWithCompleteToolResult->>ProcessExit: exit(exitCode)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Suggested reviewers
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 OpenGrep (1.23.0)projects/site/src/_11ty/utils/env.js┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.13][ERROR]: unable to find a config; path projects/internals/vite/src/configs/test.node.js┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.25][ERROR]: unable to find a config; path projects/internals/vite/src/configs/test.js┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.21][ERROR]: unable to find a config; path 🔧 ESLint
projects/cli/src/install.test.tsParsing error: error TS5012: Cannot read file '/tsconfig.json': ENOENT: no such file or directory, open '/tsconfig.json'. projects/cli/src/update.test.tsParsing error: error TS5012: Cannot read file '/tsconfig.json': ENOENT: no such file or directory, open '/tsconfig.json'. projects/cli/src/utils.test.tsParsing error: error TS5012: Cannot read file '/tsconfig.json': ENOENT: no such file or directory, open '/tsconfig.json'.
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: 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/src/utils.ts (1)
145-150: 🎯 Functional Correctness | 🟠 MajorTighten
isReportto validate report entries.
isReport()accepts any truthystatus/message, so malformed data can be narrowed toReportandexitCodeForResult()can return success. Require each entry to be an object,messageto be a string, andstatusto match the supported report statuses.🤖 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` around lines 145 - 150, The isReport guard is too permissive and can incorrectly narrow malformed data to Report. Update isReport to validate each entry in the object is itself an object, ensure message is a string, and verify status matches the supported report statuses instead of just being truthy. Keep the check localized in isReport so exitCodeForResult continues to rely on a correctly validated Report shape.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@projects/cli/src/install.test.ts`:
- Around line 760-782: In the install test that calls spawnSync for
writeInstallManifest, assert the subprocess result succeeded before reading and
parsing dist/manifest.json. Move the check on result.status (and include
result.stderr in the failure path if needed) ahead of the readFile/JSON.parse
step in the test using createInstallerContext and writeInstallManifest, so
failures surface the actual exit error instead of a file-not-found.
In `@projects/cli/src/utils.ts`:
- Line 185: The formatted result in the rendering logic is appending the debug
metadata directly after the prior content, so update the formatting in the utils
rendering path to prepend a separator before the debug block. Locate the
concatenation that adds the [debug] metadata in the result-building flow and
ensure it starts on a new line so the output is separated cleanly from the
rendered content.
---
Outside diff comments:
In `@projects/cli/src/utils.ts`:
- Around line 145-150: The isReport guard is too permissive and can incorrectly
narrow malformed data to Report. Update isReport to validate each entry in the
object is itself an object, ensure message is a string, and verify status
matches the supported report statuses instead of just being truthy. Keep the
check localized in isReport so exitCodeForResult continues to rely on a
correctly validated Report shape.
🪄 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: 872843f0-0461-4b73-9063-626b276e6d00
📒 Files selected for processing (20)
projects/cli/scripts/write-install-manifest.mjsprojects/cli/src/index.tsprojects/cli/src/install.test.tsprojects/cli/src/install.tsprojects/cli/src/update.test.tsprojects/cli/src/update.tsprojects/cli/src/utils.test.tsprojects/cli/src/utils.tsprojects/cli/vite.config.tsprojects/internals/tools/src/api/utils.tsprojects/internals/tools/src/cli/utils.test.tsprojects/internals/tools/src/cli/utils.tsprojects/internals/tools/src/playground/utils.test.tsprojects/internals/tools/src/playground/utils.tsprojects/internals/vite/src/configs/build.jsprojects/internals/vite/src/configs/build.node.jsprojects/internals/vite/src/configs/env.jsprojects/internals/vite/src/configs/test.jsprojects/internals/vite/src/configs/test.node.jsprojects/site/src/_11ty/utils/env.js
coryrylan
force-pushed
the
topic-cli-version-updater
branch
from
9cd318b to
b9fc8cd
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/cli/src/update.test.ts`:
- Around line 58-69: Add a regression test in fetchLatestSha coverage for a
manifest that returns an empty sha string, since the current test only covers a
missing sha shape. Update the existing test suite in update.test.ts alongside
the fetchLatestSha cases to stub fetch with a manifest containing sha: '' and
assert it rejects with the same failure path as the missing-sha case, so the
runtime guard in fetchLatestSha stays enforced for both invalid manifest shapes.
In `@projects/cli/src/update.ts`:
- Around line 24-28: The SHA guard in the manifest-fetching logic is too loose
because the `sha` value can be an empty string and still pass. Update the `sha`
validation in the update flow so the `sha` returned from `res.json()` must be a
non-empty string before returning it, and continue throwing the missing-SHA
error for blank values; this change should be applied in the manifest parsing
path used by `isUpdateAvailable()` so invalid manifests do not get treated as
“no update available.”
🪄 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: dc3c67f8-cb01-45f2-b49c-5749b00c7bd5
📒 Files selected for processing (20)
projects/cli/scripts/write-install-manifest.mjsprojects/cli/src/index.tsprojects/cli/src/install.test.tsprojects/cli/src/install.tsprojects/cli/src/update.test.tsprojects/cli/src/update.tsprojects/cli/src/utils.test.tsprojects/cli/src/utils.tsprojects/cli/vite.config.tsprojects/internals/tools/src/api/utils.tsprojects/internals/tools/src/cli/utils.test.tsprojects/internals/tools/src/cli/utils.tsprojects/internals/tools/src/playground/utils.test.tsprojects/internals/tools/src/playground/utils.tsprojects/internals/vite/src/configs/build.jsprojects/internals/vite/src/configs/build.node.jsprojects/internals/vite/src/configs/env.jsprojects/internals/vite/src/configs/test.jsprojects/internals/vite/src/configs/test.node.jsprojects/site/src/_11ty/utils/env.js
| it('should throw error when manifest is missing sha', async () => { | ||
| vi.stubGlobal( | ||
| 'fetch', | ||
| vi.fn().mockResolvedValue({ | ||
| ok: true, | ||
| json: () => Promise.resolve({ version: '2.1.0' }) | ||
| }) | ||
| ); | ||
|
|
||
| await expect(fetchLatestSha()).rejects.toThrow('Failed to fetch latest build info'); | ||
| }); | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add a blank-sha regression case.
This covers sha being absent, but '' is another invalid manifest shape that should be pinned down once the runtime guard is tightened. Otherwise the remaining bad-manifest path can regress unnoticed.
🤖 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/update.test.ts` around lines 58 - 69, Add a regression test
in fetchLatestSha coverage for a manifest that returns an empty sha string,
since the current test only covers a missing sha shape. Update the existing test
suite in update.test.ts alongside the fetchLatestSha cases to stub fetch with a
manifest containing sha: '' and assert it rejects with the same failure path as
the missing-sha case, so the runtime guard in fetchLatestSha stays enforced for
both invalid manifest shapes.
| const { sha } = (await res.json()) as { sha?: string }; | ||
| if (typeof sha !== 'string') { | ||
| throw new Error('Missing sha'); | ||
| } | ||
| return sha; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject blank manifest SHAs too.
typeof sha === 'string' still accepts '', which means an invalid manifest can pass this guard and then get treated downstream as "no update available" because isUpdateAvailable() only reports when latestSha !== ''. That leaves the missing-SHA failure mode only partially covered.
Proposed fix
- if (typeof sha !== 'string') {
+ if (typeof sha !== 'string' || sha.trim() === '') {
throw new Error('Missing sha');
}📝 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.
| const { sha } = (await res.json()) as { sha?: string }; | |
| if (typeof sha !== 'string') { | |
| throw new Error('Missing sha'); | |
| } | |
| return sha; | |
| const { sha } = (await res.json()) as { sha?: string }; | |
| if (typeof sha !== 'string' || sha.trim() === '') { | |
| throw new Error('Missing sha'); | |
| } | |
| return sha; |
🤖 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/update.ts` around lines 24 - 28, The SHA guard in the
manifest-fetching logic is too loose because the `sha` value can be an empty
string and still pass. Update the `sha` validation in the update flow so the
`sha` returned from `res.json()` must be a non-empty string before returning it,
and continue throwing the missing-SHA error for blank values; this change should
be applied in the manifest parsing path used by `isUpdateAvailable()` so invalid
manifests do not get treated as “no update available.”
- Added support for versioning output files in the build process, ensuring that the correct version is written to bundled files. - Introduced a new utility function to replace bundled constants in the output files, improving maintainability. - Updated the installation script to utilize the embedded build SHA and version, enhancing the manifest generation. - Refactored error handling in the update check to ensure proper feedback when the SHA is missing. Signed-off-by: Cory Rylan <crylan@nvidia.com>
coryrylan
force-pushed
the
topic-cli-version-updater
branch
from
b9fc8cd to
76d2980
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
projects/cli/src/utils.ts (1)
185-185: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMissing separator before
[debug]block.Line 185 still appends
[debug]directly onto the rendered result (...}[debug]), with no leading newline, despite the prior fix being marked as addressed. Prefix the debug block with\n.Suggested fix
- formattedResult += `[debug]\n[command]: ${tool.metadata.command}`; + formattedResult += `\n[debug]\n[command]: ${tool.metadata.command}`;🤖 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 185, The rendered output in utils.ts still appends the [debug] block directly after the main result, so update the formatting in the code that builds formattedResult to prefix the debug section with a leading newline before [debug]. Use the existing string assembly around formattedResult and tool.metadata.command to keep the debug block separated from the prior content.
🤖 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`:
- Around line 29-34: The getBundledConstant helper builds a RegExp from the
unescaped name argument, so update that pattern construction to safely escape
any regex metacharacters before interpolation. Keep the lookup logic in
getBundledConstant and preserve the missing-constant error path, but make the
guard/message reflect that this is a no-match case rather than a falsy value
check.
In `@projects/cli/src/install.ts`:
- Around line 398-405: The install flow in installSkill should keep the
CI/non-interactive policy check from shouldInstallGlobalElementsSkill(context)
separate from the existing-file shortcut. Call the policy gate first so CI still
blocks global installation, then use hasExistingSkill only to decide whether to
skip the prompt and whether to avoid overwriting an existing SKILL.md without
confirmation. Preserve the behavior around writeGlobalElementsSkill(skillPath)
and the Installed/Updated log message, but make sure reruns cannot bypass the
gate or silently replace a preexisting file.
---
Duplicate comments:
In `@projects/cli/src/utils.ts`:
- Line 185: The rendered output in utils.ts still appends the [debug] block
directly after the main result, so update the formatting in the code that builds
formattedResult to prefix the debug section with a leading newline before
[debug]. Use the existing string assembly around formattedResult and
tool.metadata.command to keep the debug block separated from the prior content.
🪄 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: 983fe57d-b011-4197-a3cc-586d20e1abac
📒 Files selected for processing (21)
projects/cli/scripts/write-install-manifest.mjsprojects/cli/src/index.tsprojects/cli/src/install.test.tsprojects/cli/src/install.tsprojects/cli/src/update.test.tsprojects/cli/src/update.tsprojects/cli/src/utils.test.tsprojects/cli/src/utils.tsprojects/cli/vite.config.tsprojects/internals/tools/src/api/utils.tsprojects/internals/tools/src/cli/utils.test.tsprojects/internals/tools/src/cli/utils.tsprojects/internals/tools/src/playground/utils.test.tsprojects/internals/tools/src/playground/utils.tsprojects/internals/vite/src/configs/build.jsprojects/internals/vite/src/configs/build.node.jsprojects/internals/vite/src/configs/env.jsprojects/internals/vite/src/configs/test.jsprojects/internals/vite/src/configs/test.node.jsprojects/site/src/_11ty/utils/env.jsprojects/site/src/_11ty/utils/env.test.ts
💤 Files with no reviewable changes (1)
- projects/site/src/_11ty/utils/env.test.ts
| function getBundledConstant(content, name) { | ||
| const value = content.match(new RegExp(`(?:var|const|let)\\s+${name}\\s*=\\s*["']([^"']+)["']`))?.[1]; | ||
| if (!value) { | ||
| throw new Error(`Missing bundled ${name} constant`); | ||
| } | ||
| return value; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Minor: escape name before building the RegExp.
name is interpolated unescaped into the pattern. Today the only callers pass BUILD_SHA/VERSION (regex-safe), but a future caller with regex metacharacters would silently produce a wrong pattern. Also consider that !value rejects a legitimately empty constant — though the [^"']+ group already requires at least one character, so the guard is effectively a missing-match check; a clearer message would help.
🤖 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 29 - 34, The
getBundledConstant helper builds a RegExp from the unescaped name argument, so
update that pattern construction to safely escape any regex metacharacters
before interpolation. Keep the lookup logic in getBundledConstant and preserve
the missing-constant error path, but make the guard/message reflect that this is
a no-match case rather than a falsy value check.
| const hasExistingSkill = existsSync(skillPath); | ||
| if (!hasExistingSkill && !(await shouldInstallGlobalElementsSkill(context))) { | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| await writeGlobalElementsSkill(skillPath); | ||
| context.log(`Installed agent skill at ${skillPath}`); | ||
| context.log(`${hasExistingSkill ? 'Updated' : 'Installed'} agent skill at ${skillPath}`); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Keep the CI/install-policy gate separate from the prompt shortcut.
Lines 398-400 now skip shouldInstallGlobalElementsSkill() whenever SKILL.md already exists, but that helper is also the only place that blocks global installs in CI. As written, a rerun in CI will overwrite an existing ~/.agents/skills/elements/SKILL.md without any prompt, and it will silently replace any preexisting file at that path. Preserve the non-interactive policy check before the hasExistingSkill fast path, then use the existence check only to suppress the prompt.
Suggested fix
async function installGlobalElementsSkill(context: InstallContext): Promise<void> {
const skillPath = getGlobalElementsSkillPath(context.env, context.platform);
if (!skillPath) {
context.warn('Could not install Elements agent skill. HOME is not set.');
return;
}
+ if (context.env.CI) {
+ return;
+ }
+
const hasExistingSkill = existsSync(skillPath);
if (!hasExistingSkill && !(await shouldInstallGlobalElementsSkill(context))) {
return;
}
try {📝 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.
| const hasExistingSkill = existsSync(skillPath); | |
| if (!hasExistingSkill && !(await shouldInstallGlobalElementsSkill(context))) { | |
| return; | |
| } | |
| try { | |
| await writeGlobalElementsSkill(skillPath); | |
| context.log(`Installed agent skill at ${skillPath}`); | |
| context.log(`${hasExistingSkill ? 'Updated' : 'Installed'} agent skill at ${skillPath}`); | |
| const skillPath = getGlobalElementsSkillPath(context.env, context.platform); | |
| if (!skillPath) { | |
| context.warn('Could not install Elements agent skill. HOME is not set.'); | |
| return; | |
| } | |
| if (context.env.CI) { | |
| return; | |
| } | |
| const hasExistingSkill = existsSync(skillPath); | |
| if (!hasExistingSkill && !(await shouldInstallGlobalElementsSkill(context))) { | |
| return; | |
| } | |
| try { | |
| await writeGlobalElementsSkill(skillPath); | |
| context.log(`${hasExistingSkill ? 'Updated' : 'Installed'} agent skill at ${skillPath}`); |
🤖 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 398 - 405, The install flow in
installSkill should keep the CI/non-interactive policy check from
shouldInstallGlobalElementsSkill(context) separate from the existing-file
shortcut. Call the policy gate first so CI still blocks global installation,
then use hasExistingSkill only to decide whether to skip the prompt and whether
to avoid overwriting an existing SKILL.md without confirmation. Preserve the
behavior around writeGlobalElementsSkill(skillPath) and the Installed/Updated
log message, but make sure reruns cannot bypass the gate or silently replace a
preexisting file.
1 participant
Summary by CodeRabbit