Dead plugin modules (4 TypeScript files ship in dist/ but are unreachable at runtime) #1002
Closed
deepujain wants to merge 1 commit intoNVIDIA:mainfrom
Closed
Dead plugin modules (4 TypeScript files ship in dist/ but are unreachable at runtime) #1002deepujain wants to merge 1 commit intoNVIDIA:mainfrom
deepujain wants to merge 1 commit intoNVIDIA:mainfrom
Conversation
…to snapshot and migration-state runner.ts, ssrf.ts, snapshot.ts, and migration-state.ts shipped in dist/ but were unreachable at runtime (issue NVIDIA#977). This PR addresses the first two by wiring runner.ts as a CLI binary: - Add shebang and ESM main-module guard to runner.ts so it runs correctly when invoked directly (no-op when imported as a module in tests) - Register nemoclaw-runner bin in package.json pointing to dist/blueprint/runner.js - Add TODO(NVIDIA#977) comments to snapshot.ts and migration-state.ts noting they are ready and await a `nemoclaw migrate` command entry point ssrf.ts is now reachable transitively via the runner binary. Fixes NVIDIA#977 Signed-off-by: Deepak Jain <deepujain@gmail.com>
Contributor
📝 WalkthroughWalkthroughAdded a CLI entry point for the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
nemoclaw/src/blueprint/runner.ts (3)
292-302:⚠️ Potential issue | 🟠 MajorFail
applywhen provider setup commands fail.Lines 292-302 discard the exit codes from
openshell provider createandopenshell inference set. If either command fails, the code still writesplan.jsonand emits “Apply complete”, which records a partially configured run as healthy.🔧 Proposed fix
- await execa(providerArgs[0], providerArgs.slice(1), { + const providerResult = await execa(providerArgs[0], providerArgs.slice(1), { reject: false, stdout: "pipe", stderr: "pipe", env: { ...process.env, ...credEnv }, }); + if (providerResult.exitCode !== 0) { + throw new Error(`Failed to create provider: ${providerResult.stderr || providerResult.stdout}`); + } progress(70, "Setting inference route"); - await runCmd(["openshell", "inference", "set", "--provider", providerName, "--model", model], { - reject: false, - }); + const routeResult = await runCmd( + ["openshell", "inference", "set", "--provider", providerName, "--model", model], + { reject: false }, + ); + if (routeResult.exitCode !== 0) { + throw new Error(`Failed to set inference route: ${routeResult.stderr || routeResult.stdout}`); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nemoclaw/src/blueprint/runner.ts` around lines 292 - 302, The provider setup commands currently ignore failures; capture and check the exit status/return value from the execa call that runs providerArgs and from runCmd(["openshell","inference","set",...]) and if either returns a non-zero exit code (or indicates failure) throw an error or return a rejected promise to abort apply before writing plan.json or emitting the "Apply complete" message; update the section around the execa(providerArgs...) call and the subsequent runCmd invocation (referencing providerArgs, providerName, model, execa, and runCmd) to inspect their results, log the failure via progress/processLogger, and propagate an error so the apply fails on command failure.
429-436:⚠️ Potential issue | 🟠 MajorLoad the blueprint only for actions that actually use it.
Line 429 eagerly loads
blueprint.yamlbefore the action switch. Now that this file is executable as a real CLI,statusandrollbackfail outside a blueprint directory even though neither path needs the blueprint.🔧 Proposed fix
- const blueprint = loadBlueprint(); - switch (action) { case "plan": - await actionPlan(profile, blueprint, { dryRun, endpointUrl }); + await actionPlan(profile, loadBlueprint(), { dryRun, endpointUrl }); break; case "apply": - await actionApply(profile, blueprint, { planPath, endpointUrl }); + await actionApply(profile, loadBlueprint(), { planPath, endpointUrl }); break;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nemoclaw/src/blueprint/runner.ts` around lines 429 - 436, The code currently calls loadBlueprint() unconditionally (const blueprint = loadBlueprint()) before the action switch, causing commands like status and rollback to fail outside a blueprint directory; move the call into only the cases that require it (e.g., inside the "plan" and "apply" branches) so loadBlueprint() is invoked lazily when needed and pass the loaded blueprint to actionPlan and actionApply; ensure any references to the blueprint variable are updated to use the locally loaded value in those case blocks.
17-17:⚠️ Potential issue | 🟠 MajorUse
import.meta.mainfor symlink-safe CLI entry detection.npm installs
binentries as symlinks on Unix-like systems, and Node.js resolves the main module's real path by default. When nemoclaw-runner is launched through the installed wrapper,process.argv[1]points to the symlink path whilefileURLToPath(import.meta.url)resolves to the real file path, causing the raw equality check on line 455 to fail and preventingmain()from executing.For Node.js v22+ (where
import.meta.mainwas backported), use:if (import.meta.main) { void main().catch((e: unknown) => { process.stderr.write((e instanceof Error ? e.message : String(e)) + "\n"); process.exit(1); }); }This is the official ESM equivalent to CommonJS
require.main === moduleand works reliably regardless of symlinks. For older Node.js versions, resolve both paths before comparing:Fallback for older Node.js
-import { mkdirSync, readFileSync, readdirSync, writeFileSync } from "node:fs"; +import { mkdirSync, readFileSync, readdirSync, realpathSync, writeFileSync } from "node:fs"; @@ import { fileURLToPath } from "node:url"; -if (process.argv[1] === fileURLToPath(import.meta.url)) { - main().catch((e: unknown) => { +let isDirectExecution = false; +if (process.argv[1]) { + try { + isDirectExecution = + realpathSync(process.argv[1]) === realpathSync(fileURLToPath(import.meta.url)); + } catch { + isDirectExecution = false; + } +} + +if (isDirectExecution) { + void main().catch((e: unknown) => { process.stderr.write((e instanceof Error ? e.message : String(e)) + "\n"); process.exit(1); }); }Also applies to: 454-460
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nemoclaw/src/blueprint/runner.ts` at line 17, Replace the fragile raw path equality check that compares process.argv[1] and fileURLToPath(import.meta.url) to decide whether to run main(), and instead use the ESM entry detection import.meta.main to invoke main(); update the code around the main() bootstrap (the block currently using process.argv[1] and fileURLToPath(import.meta.url) in runner.ts) to call main() inside an import.meta.main conditional and attach a .catch that writes the error message to stderr and exits with non-zero status, and for compatibility with older Node versions consider adding a fallback that resolves both paths before comparing if import.meta.main is not available.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@nemoclaw/src/blueprint/runner.ts`:
- Around line 292-302: The provider setup commands currently ignore failures;
capture and check the exit status/return value from the execa call that runs
providerArgs and from runCmd(["openshell","inference","set",...]) and if either
returns a non-zero exit code (or indicates failure) throw an error or return a
rejected promise to abort apply before writing plan.json or emitting the "Apply
complete" message; update the section around the execa(providerArgs...) call and
the subsequent runCmd invocation (referencing providerArgs, providerName, model,
execa, and runCmd) to inspect their results, log the failure via
progress/processLogger, and propagate an error so the apply fails on command
failure.
- Around line 429-436: The code currently calls loadBlueprint() unconditionally
(const blueprint = loadBlueprint()) before the action switch, causing commands
like status and rollback to fail outside a blueprint directory; move the call
into only the cases that require it (e.g., inside the "plan" and "apply"
branches) so loadBlueprint() is invoked lazily when needed and pass the loaded
blueprint to actionPlan and actionApply; ensure any references to the blueprint
variable are updated to use the locally loaded value in those case blocks.
- Line 17: Replace the fragile raw path equality check that compares
process.argv[1] and fileURLToPath(import.meta.url) to decide whether to run
main(), and instead use the ESM entry detection import.meta.main to invoke
main(); update the code around the main() bootstrap (the block currently using
process.argv[1] and fileURLToPath(import.meta.url) in runner.ts) to call main()
inside an import.meta.main conditional and attach a .catch that writes the error
message to stderr and exits with non-zero status, and for compatibility with
older Node versions consider adding a fallback that resolves both paths before
comparing if import.meta.main is not available.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f789304b-e16f-4dc9-80fc-1f23eb663931
📒 Files selected for processing (4)
nemoclaw/package.jsonnemoclaw/src/blueprint/runner.tsnemoclaw/src/blueprint/snapshot.tsnemoclaw/src/commands/migration-state.ts
Contributor
|
#982 beat you to it, @deepujain - ask yer clanker to read through the linked PRs and existing issues before firing off a new one, please! ;) |
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.
Summary
runner.ts,ssrf.ts,snapshot.ts, andmigration-state.tsall compile and ship in the npm package but are never reachable at runtime (#977). This PR fixes the two modules that have a natural CLI entry point and documents the other two for a follow-up migration command.runner.tsis the TypeScript port of the Python blueprint runner. It has amain()function designed for CLI use but was never exposed as a bin. This PR wires it in.ssrf.tsis now reachable transitively (imported byrunner.ts).snapshot.tsandmigration-state.tsimplement thenemoclaw migrateflow. They are tested and ready, but need a future CLI subcommand to hook into;TODO(#977)comments added to mark this.Changes
nemoclaw/src/blueprint/runner.ts— add#!/usr/bin/env nodeshebang and ESM main-module guard (process.argv[1] === fileURLToPath(import.meta.url)) so the compiled binary auto-invokesmain()when run directly but is still importable as a module (tests unaffected)nemoclaw/package.json— add"bin": { "nemoclaw-runner": "./dist/blueprint/runner.js" }so the binary is installed when the package is installednemoclaw/src/blueprint/snapshot.ts— addTODO(#977)comment pointing to the pendingnemoclaw migrateentry pointnemoclaw/src/commands/migration-state.ts— sameTesting
All 542 tests pass (2 pre-existing skips). Build is clean.
The shebang is preserved in compiled output (
dist/blueprint/runner.js) and the main-module guard ensuresmain()runs when invoked as a CLI binary but not when imported by tests or other modules.Fixes #977
Signed-off-by: Deepak Jain deepujain@gmail.com
Summary by CodeRabbit
New Features
nemoclaw-runnerCLI command, enabling direct command-line execution of the nemoclaw runner.Chores