Skip to content

fix(cli): version and updater checks #155

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
coryrylan wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(cli): version and updater checks #155

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion projects/cli/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ npm install -g @nvidia-elements/cli
| ---------------------------------------------------------------- | -------------------------------------------------------------------------------------- |
| `nve` | Show About and help output. |
| `nve api.list [format]` | Get a list of all available Elements (`nve-*`) APIs and components. |
| `nve api.get <names> [format]` | Get documentation for known components or attributes by name (`nve-*`). |
| `nve api.get <names..> [--format <format>]` | Get documentation for one to five known components or attributes (`nve-*`). |
| `nve api.template.validate <template>` | Check HTML templates using Elements APIs and components (`nve-*`). |
| `nve api.imports.get <template>` | Get ESM imports for an HTML template using Elements APIs (`nve-*`). |
| `nve api.tokens.list [format] [query]` | Get available semantic CSS custom properties and design tokens for theming. |
Expand Down
2 changes: 1 addition & 1 deletion projects/cli/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@
"dist/**/*.js"
],
"scripts": {
"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",
"ci": "wireit",
"build": "wireit",
"lint": "wireit",
Expand Down
18 changes: 13 additions & 5 deletions projects/cli/scripts/write-install-manifest.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
// SPDX-License-Identifier: Apache-2.0

import { createHash } from 'node:crypto';
import { existsSync, readFileSync, writeFileSync } from 'node:fs';
import { readFileSync, writeFileSync } from 'node:fs';
import { resolve } from 'node:path';

const platformBinaries = [
Expand All @@ -14,14 +14,22 @@ const platformBinaries = [
];

const manifestPath = resolve('dist/manifest.json');
const existingManifest = existsSync(manifestPath) ? JSON.parse(readFileSync(manifestPath, 'utf-8')) : {};
const indexContent = readFileSync(resolve('dist/index.js'));
const sha = existingManifest.sha || createHash('sha256').update(indexContent).digest('hex').slice(0, 12);
const indexContent = readFileSync(resolve('dist/index.js'), 'utf-8');
const sha = getBundledConstant(indexContent, 'BUILD_SHA');
const version = getBundledConstant(indexContent, 'VERSION');
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') }];
})
);

writeFileSync(manifestPath, `${JSON.stringify({ sha, platforms }, null, 2)}\n`);
writeFileSync(manifestPath, `${JSON.stringify({ sha, version, platforms }, null, 2)}\n`);

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;
Comment on lines +29 to +34

@coderabbitai coderabbitai Bot Jun 26, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 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.

}
55 changes: 55 additions & 0 deletions projects/cli/src/index.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,11 @@ describe('index', () => {
expect(output).toContain('nve examples.get <id> [format]');
});

it('should provide api.get with variadic names', () => {
expect(output).toContain('nve api.get <names..>');
expect(output).not.toContain('nve api.get <names> [format]');
});

it('should provide skills.list', () => {
expect(output).toContain('nve skills.list [format]');
});
Expand Down Expand Up @@ -120,6 +125,56 @@ describe('index', () => {
expect(combined).toContain('nve-foo');
expect(combined).toContain('nve-bar');
});

it('should pass space-separated API names as one array argument', () => {
const result = spawnSync(process.execPath, ['dist/index.js', 'api.get', 'nve-card', 'nve-input'], {
timeout: 10000,
encoding: 'utf-8',
input: '',
env: { ...process.env, CI: 'true' }
});

expect(result.status).toBe(0);
expect(result.stderr).toBe('');
expect(result.stdout).toContain('nve-card');
expect(result.stdout).toContain('nve-input');
});

it('should accept an explicit format option after space-separated API names', () => {
const result = spawnSync(
process.execPath,
['dist/index.js', 'api.get', 'nve-card', 'nve-input', '--format', 'json'],
{
timeout: 10000,
encoding: 'utf-8',
input: '',
env: { ...process.env, CI: 'true' }
}
);

expect(result.status).toBe(0);
expect(result.stderr).toBe('');
expect(JSON.parse(result.stdout)).toEqual([
expect.objectContaining({ name: 'nve-card' }),
expect.objectContaining({ name: 'nve-input' })
]);
});

it('should reject array arguments that exceed the schema limit', () => {
const result = spawnSync(
process.execPath,
['dist/index.js', 'api.get', 'nve-card', 'nve-input', 'nve-button', 'nve-badge', 'nve-alert', 'nve-link'],
{
timeout: 10000,
encoding: 'utf-8',
input: '',
env: { ...process.env, CI: 'true' }
}
);

expect(result.status).toBe(1);
expect(result.stderr).toContain('api.get accepts at most 5 names.');
});
});

describe('tool errors', () => {
Expand Down
72 changes: 43 additions & 29 deletions projects/cli/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,9 @@ process.env.ELEMENTS_ENV = 'cli';
import yargs from 'yargs';
import { hideBin } from 'yargs/helpers';
import { performance } from 'perf_hooks';
import { type ManagedToolMethod, tools, ToolSupport, type Schema, isDebug, MAX_CONTEXT_TOKENS } from '@internals/tools';
import { type ManagedToolMethod, tools, ToolSupport, type Schema } from '@internals/tools';
import { installNve } from './install.js';
import { banner, colors, getArgValue, progressBar, renderResult, runAsyncTool } from './utils.js';
import { banner, colors, exitWithCompleteToolResult, exitWithToolError, getArgValue, runAsyncTool } from './utils.js';
import { notifyIfUpdateAvailable } from './update.js';

export const VERSION = '0.0.0';
Expand Down Expand Up @@ -46,11 +46,6 @@ yargsInstance.middleware(argv => {
}
});

async function exitWithToolError(result: unknown, message: string | undefined): Promise<never> {
console.error(result === undefined ? colors.error(message ?? 'unknown error') : await renderResult(result));
process.exit(1);
}

yargsInstance.command(
'install [source]',
false,
Expand All @@ -73,10 +68,10 @@ yargsInstance.command(
async () => {
if (process.argv.includes('--upgrade')) {
const upgradeTool = tools.find(tool => tool.metadata.command === 'cli.upgrade') as ManagedToolMethod<unknown>;
const { result, status, message } = await runAsyncTool({}, upgradeTool);
console.log(colors.info('Upgrading Elements CLI...'));
const { result, status, message } = await runAsyncTool({}, upgradeTool, { interactiveProgress: false });
if (status === 'complete') {
await renderResult(result);
process.exit(0);
await exitWithCompleteToolResult({ result });
} else {
await exitWithToolError(result, message);
}
Expand All @@ -100,9 +95,13 @@ tools
const optionalArgs = Object.keys(properties ?? {}).filter(
key => !required?.includes(key) || properties?.[key]?.default
);
const hasVariadicArg = requiredArgs.some(key => properties?.[key]?.type === 'array');
const positionalArgs = requiredArgs.map(key => (properties?.[key]?.type === 'array' ? `<${key}..>` : `<${key}>`));

const command =
`${tool.metadata.command} ${[...requiredArgs.map(key => `<${key}>`), ...optionalArgs.map(key => `[${key}]`)].join(' ')}`.trim();
const command = `${tool.metadata.command} ${[
...positionalArgs,
...(hasVariadicArg ? [] : optionalArgs.map(key => `[${key}]`))
].join(' ')}`.trim();
Comment on lines +101 to +104

@coderabbitai coderabbitai Bot Jun 30, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '--- index.ts around command generation ---\n'
sed -n '80,130p' projects/cli/src/index.ts

printf '\n--- index.test.ts around help assertions ---\n'
sed -n '1,220p' projects/cli/src/index.test.ts

printf '\n--- docs references for api.get / --format ---\n'
rg -n "api\.get|--format|<names\.\.>" projects -g '!**/node_modules/**'

Repository: NVIDIA/elements

Length of output: 244

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo '--- index.ts around command generation ---'
sed -n '80,130p' projects/cli/src/index.ts

echo
echo '--- index.test.ts around help assertions ---'
sed -n '1,220p' projects/cli/src/index.test.ts

echo
echo '--- docs references for api.get / --format ---'
rg -n "api\.get|--format|<names\.\.>" projects -g '!**/node_modules/**'

Repository: NVIDIA/elements

Length of output: 12946

Keep --format in variadic command usage.
The variadic branch drops every optional arg from the displayed synopsis, so nve --help no longer advertises api.get’s --format flag. Keep flag-style options visible, e.g. nve api.get <names..> [--format <format>].

🤖 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/index.ts` around lines 101 - 104, The command synopsis
builder in index.ts is incorrectly hiding all optional args when
`hasVariadicArg` is true, which removes flag-style options like `--format` from
help output. Update the `command` construction logic so the variadic path still
appends optional flag arguments from `optionalArgs` while excluding only extra
positional placeholders, keeping `tool.metadata.command` help text consistent
for commands such as `api.get`.


yargsInstance.command(
command,
Expand All @@ -113,7 +112,8 @@ tools

const argOptions = (prop: Schema) => ({
describe: prop.description,
type: prop.type as 'string' | 'number' | 'boolean',
type: (prop.type === 'array' ? 'string' : prop.type) as 'string' | 'number' | 'boolean',
...(prop.type === 'array' ? { array: true } : {}),
choices: prop.enum ?? undefined,
default: prop.default
});
Expand All @@ -128,17 +128,13 @@ tools
const end = performance.now();

if (status === 'complete') {
let formattedResult = await renderResult(result);
if (isDebug()) {
const tokens = formattedResult.length / 4;
const pct = (tokens / MAX_CONTEXT_TOKENS) * 100;
formattedResult += `[debug]\n[command]: ${tool.metadata.command}`;
formattedResult += `\n[execution time]: ${((end - start) / 1000).toFixed(2)} seconds`;
formattedResult += `\n[token usage]: ${progressBar(pct)} ${tokens.toLocaleString()} / ${MAX_CONTEXT_TOKENS.toLocaleString()} (${(100 - pct).toFixed(1)}% remaining)`;
}
console.log(formattedResult);
await notifyIfUpdateAvailable(BUILD_SHA);
process.exit(0);
await exitWithCompleteToolResult({
result,
tool,
start,
end,
notifyUpdate: () => notifyIfUpdateAvailable(BUILD_SHA)
});
} else {
await exitWithToolError(result, message);
}
Expand All @@ -155,13 +151,31 @@ tools
const propertySchema = properties?.[argName] ?? {};
const v = await getArgValue(argName, propertySchema);
argv[argName] = v;
} else if (properties?.[argName]?.type === 'array' && typeof argv[argName] === 'string') {
argv[argName] = (argv[argName] as string)
.split(',')
.map(s => s.trim())
.filter(Boolean);
}
}

Object.entries(properties ?? {})
.filter(([, property]) => property.type === 'array')
.forEach(([argName, property]) => {
if (argv[argName] === undefined) return;
const values = (Array.isArray(argv[argName]) ? argv[argName] : [argv[argName]])
.flatMap(value => (typeof value === 'string' ? value.split(',') : []))
.map(value => value.trim())
.filter(Boolean);
if (property.minItems !== undefined && values.length < property.minItems) {
console.error(
colors.error(`${tool.metadata.command} accepts at least ${property.minItems} ${argName}.`)
);
process.exit(1);
}
if (property.maxItems !== undefined && values.length > property.maxItems) {
console.error(
colors.error(`${tool.metadata.command} accepts at most ${property.maxItems} ${argName}.`)
);
process.exit(1);
}
argv[argName] = values;
});
}
]
);
Expand Down
64 changes: 62 additions & 2 deletions projects/cli/src/install.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -328,15 +328,25 @@ describe('installNve', () => {
expect(await readFile(skillPath, 'utf-8')).toBe(getExpectedElementsSkillMarkdown());
});

it('should overwrite the global Elements skill when accepted', async () => {
it('should overwrite the global Elements skill without prompting when it already exists', async () => {
const context = await createInstallerContext();
const source = await writeFakeNve(join(context.root, 'source-nve'));
const skillPath = join(context.home, '.agents/skills/elements/SKILL.md');
let prompted = false;
await mkdir(dirname(skillPath), { recursive: true });
await writeFile(skillPath, 'stale skill');

await installNve({ env: createUnixEnv(context), log: () => {}, prompt: async () => 'y', source });
await installNve({
env: createUnixEnv(context),
log: () => {},
prompt: async () => {
prompted = true;
return 'N';
},
source
});

expect(prompted).toBe(false);
expect(await readFile(skillPath, 'utf-8')).toBe(getExpectedElementsSkillMarkdown());
});

Expand Down Expand Up @@ -393,6 +403,28 @@ describe('installNve', () => {
expect(existsSync(join(context.home, '.agents/skills/elements/SKILL.md'))).toBe(false);
});

it('should update an existing global Elements skill in CI without prompting', async () => {
const context = await createInstallerContext();
const source = await writeFakeNve(join(context.root, 'source-nve'));
const skillPath = join(context.home, '.agents/skills/elements/SKILL.md');
let prompted = false;
await mkdir(dirname(skillPath), { recursive: true });
await writeFile(skillPath, 'stale skill');

await installNve({
env: { ...createUnixEnv(context), CI: 'true' },
log: () => {},
prompt: async () => {
prompted = true;
return 'yes';
},
source
});

expect(prompted).toBe(false);
expect(await readFile(skillPath, 'utf-8')).toBe(getExpectedElementsSkillMarkdown());
});

it('should keep the CLI install when global skill installation fails', async () => {
const context = await createInstallerContext();
const source = await writeFakeNve(join(context.root, 'source-nve'));
Expand Down Expand Up @@ -724,6 +756,30 @@ describe('write-install-manifest', () => {

expect(source).toContain("{ key: 'windows-x64', filename: 'nve-windows-x64.exe', source: 'nve-windows-x64.exe' }");
});

it('should use the embedded build sha and version', async () => {
const context = await createInstallerContext();
const dist = join(context.root, 'dist');
await mkdir(dist, { recursive: true });
await writeFile(join(dist, 'index.js'), 'var VERSION = "2.1.0";\nvar BUILD_SHA = "abc123def456";\n');
await Promise.all(
['nve-macos-arm64', 'nve-macos-x64', 'nve-linux-x64', 'nve-linux-arm64', 'nve-windows-x64.exe'].map(
async filename => writeFile(join(dist, filename), filename)
)
);

const result = spawnSync(process.execPath, [writeInstallManifest], { cwd: context.root, encoding: 'utf-8' });
const manifest = JSON.parse(await readFile(join(dist, 'manifest.json'), 'utf-8')) as {
sha: string;
version: string;
platforms: { 'macos-arm64': { checksum: string } };
};

expect(result.status).toBe(0);
expect(manifest.sha).toBe('abc123def456');
expect(manifest.version).toBe('2.1.0');
expect(manifest.platforms['macos-arm64'].checksum).toBe(getContentChecksum('nve-macos-arm64'));
});
Comment thread
coderabbitai[bot] marked this conversation as resolved.
});

async function createInstallerContext(): Promise<InstallerContext> {
Expand Down Expand Up @@ -954,6 +1010,10 @@ function getFileChecksum(path: string): string {
return createHash('sha256').update(readFileSync(path)).digest('hex');
}

function getContentChecksum(value: string): string {
return createHash('sha256').update(value).digest('hex');
}

function countOccurrences(value: string, search: string): number {
return value.split(search).length - 1;
}
11 changes: 6 additions & 5 deletions projects/cli/src/install.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -389,19 +389,20 @@ function getManifestPlatform(platform: NodeJS.Platform): string {
}

async function installGlobalElementsSkill(context: InstallContext): Promise<void> {
if (!(await shouldInstallGlobalElementsSkill(context))) {
return;
}

const skillPath = getGlobalElementsSkillPath(context.env, context.platform);
if (!skillPath) {
context.warn('Could not install Elements agent skill. HOME is not set.');
return;
}

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}`);
Comment on lines +398 to +405

@coderabbitai coderabbitai Bot Jun 26, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ 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.

Suggested change
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.

} catch (e) {
const message = e instanceof Error ? e.message : String(e);
context.warn(`Could not install Elements agent skill at ${skillPath}. ${message}`);
Expand Down
Loading