refactor(cli): remove legacy bin lib shims#1713
Merged
Conversation
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Contributor
|
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:
📝 WalkthroughWalkthroughRemoved many thin CommonJS shim entrypoints under Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 (2)
AGENTS.md (1)
1-1:⚠️ Potential issue | 🟡 MinorAdd SPDX header comments at the top of this Markdown file.
AGENTS.mdis changed in this PR and should include the required SPDX header block.Suggested patch
+<!-- SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. --> +<!-- SPDX-License-Identifier: Apache-2.0 --> + # Agent InstructionsAs per coding guidelines
**/*.{js,ts,tsx,jsx,sh,md}must include SPDX license headers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AGENTS.md` at line 1, Add the required SPDX license header block to the very top of AGENTS.md; insert the standard SPDX comment block used across the repo (e.g., lines like "SPDX-FileCopyrightText: <owner>" and "SPDX-License-Identifier: <license>") so the file matches the policy for **/*.{js,ts,tsx,jsx,sh,md}, ensuring the header appears before any other content in AGENTS.md..agents/skills/nemoclaw-maintainer-day/RISKY-AREAS.md (1)
1-1:⚠️ Potential issue | 🟡 MinorPlease add the SPDX header to this changed Markdown file.
This file is covered by the repository-wide SPDX rule for Markdown sources.
Suggested patch
+<!-- SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. --> +<!-- SPDX-License-Identifier: Apache-2.0 --> + # NemoClaw Risky Code AreasAs per coding guidelines
**/*.{js,ts,tsx,jsx,sh,md}must include SPDX license headers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/skills/nemoclaw-maintainer-day/RISKY-AREAS.md at line 1, This Markdown file (starts with the header "# NemoClaw Risky Code Areas") is missing the required SPDX header; add a single-line SPDX license comment as the very first line of the file in HTML-comment form, for example <!-- SPDX-License-Identifier: MIT --> (replace MIT with the repository's canonical SPDX identifier), ensure it's placed before the "# NemoClaw Risky Code Areas" title and followed by a blank line so the rest of the content remains unchanged.
🧹 Nitpick comments (2)
test/service-env.test.ts (1)
10-11: Inconsistent file extension in imports.Line 10 imports without
.jsextension while line 11 includes.js. For consistency and to avoid potential module resolution issues when importing from compileddist/artifacts, consider using the same pattern for both imports.♻️ Suggested fix for consistency
-import { resolveOpenshell } from "../dist/lib/resolve-openshell"; -import { parseAllowedChatIds, isChatAllowed } from "../dist/lib/chat-filter.js"; +import { resolveOpenshell } from "../dist/lib/resolve-openshell.js"; +import { parseAllowedChatIds, isChatAllowed } from "../dist/lib/chat-filter.js";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/service-env.test.ts` around lines 10 - 11, The two imports use inconsistent extensions—update them to the same pattern (either both with .js or both without) to avoid resolution issues: modify the import for resolveOpenshell or the import for parseAllowedChatIds/isChatAllowed so both imports use the identical extension style (refer to the symbols resolveOpenshell, parseAllowedChatIds, isChatAllowed and adjust their import statements accordingly).test/onboard.test.ts (1)
1030-1033: Consider a tiny helper for dist module path stringification.The repeated
JSON.stringify(path.join(repoRoot, "dist", "lib", "..."))pattern is copy/paste-heavy; a helper would reduce maintenance risk.♻️ Optional refactor sketch
+const distLib = (repoRoot, name) => + JSON.stringify(path.join(repoRoot, "dist", "lib", `${name}.js`)); -const onboardPath = JSON.stringify(path.join(repoRoot, "dist", "lib", "onboard.js")); -const runnerPath = JSON.stringify(path.join(repoRoot, "dist", "lib", "runner.js")); -const registryPath = JSON.stringify(path.join(repoRoot, "dist", "lib", "registry.js")); +const onboardPath = distLib(repoRoot, "onboard"); +const runnerPath = distLib(repoRoot, "runner"); +const registryPath = distLib(repoRoot, "registry");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/onboard.test.ts` around lines 1030 - 1033, The three variables onboardPath, runnerPath, and registryPath duplicate JSON.stringify(path.join(repoRoot, "dist", "lib", ...)) — extract a small helper (e.g., distModulePath or makeDistPath) that accepts the module filename and returns JSON.stringify(path.join(repoRoot, "dist", "lib", filename)), then replace the three assignments to call that helper (use the same helper wherever similar patterns appear) to reduce repetition and ease maintenance.
🤖 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 @.agents/skills/nemoclaw-maintainer-day/RISKY-AREAS.md:
- Line 1: This Markdown file (starts with the header "# NemoClaw Risky Code
Areas") is missing the required SPDX header; add a single-line SPDX license
comment as the very first line of the file in HTML-comment form, for example
<!-- SPDX-License-Identifier: MIT --> (replace MIT with the repository's
canonical SPDX identifier), ensure it's placed before the "# NemoClaw Risky Code
Areas" title and followed by a blank line so the rest of the content remains
unchanged.
In `@AGENTS.md`:
- Line 1: Add the required SPDX license header block to the very top of
AGENTS.md; insert the standard SPDX comment block used across the repo (e.g.,
lines like "SPDX-FileCopyrightText: <owner>" and "SPDX-License-Identifier:
<license>") so the file matches the policy for **/*.{js,ts,tsx,jsx,sh,md},
ensuring the header appears before any other content in AGENTS.md.
---
Nitpick comments:
In `@test/onboard.test.ts`:
- Around line 1030-1033: The three variables onboardPath, runnerPath, and
registryPath duplicate JSON.stringify(path.join(repoRoot, "dist", "lib", ...)) —
extract a small helper (e.g., distModulePath or makeDistPath) that accepts the
module filename and returns JSON.stringify(path.join(repoRoot, "dist", "lib",
filename)), then replace the three assignments to call that helper (use the same
helper wherever similar patterns appear) to reduce repetition and ease
maintenance.
In `@test/service-env.test.ts`:
- Around line 10-11: The two imports use inconsistent extensions—update them to
the same pattern (either both with .js or both without) to avoid resolution
issues: modify the import for resolveOpenshell or the import for
parseAllowedChatIds/isChatAllowed so both imports use the identical extension
style (refer to the symbols resolveOpenshell, parseAllowedChatIds, isChatAllowed
and adjust their import statements accordingly).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e1ea1326-9395-430f-bb04-fb6b63fe55b1
📒 Files selected for processing (54)
.agents/skills/nemoclaw-maintainer-day/RISKY-AREAS.mdAGENTS.mdCONTRIBUTING.mdbin/lib/chat-filter.jsbin/lib/config-io.jsbin/lib/debug.jsbin/lib/inference-config.jsbin/lib/local-inference.jsbin/lib/nim.jsbin/lib/onboard-session.jsbin/lib/onboard.jsbin/lib/platform.jsbin/lib/policies.jsbin/lib/preflight.jsbin/lib/registry.jsbin/lib/resolve-openshell.jsbin/lib/runner.jsbin/lib/runtime-recovery.jsbin/lib/sandbox-build-context.jsbin/lib/services.jsbin/lib/version.jspackage.jsonscripts/benchmark-sandbox-image-build.jsscripts/debug.shsrc/lib/debug.tssrc/lib/local-inference.tssrc/lib/model-prompts.test.tssrc/lib/model-prompts.tssrc/lib/nim.test.tssrc/lib/nim.tssrc/lib/onboard-session.test.tssrc/lib/onboard.tssrc/lib/preflight.tssrc/lib/provider-models.test.tssrc/lib/provider-models.tssrc/lib/usage-notice.tssrc/nemoclaw.tstest/credentials.test.tstest/e2e/e2e-cloud-experimental/skip/04-nemoclaw-openshell-status-parity.shtest/e2e/test-onboard-repair.shtest/e2e/test-onboard-resume.shtest/e2e/test-telegram-injection.shtest/onboard-readiness.test.tstest/onboard-selection.test.tstest/onboard.test.tstest/platform.test.tstest/policies.test.tstest/presets-checkbox.test.tstest/registry.test.tstest/resolve-openshell.test.tstest/runner.test.tstest/sandbox-build-context.test.tstest/service-env.test.tstest/ssh-known-hosts.test.ts
💤 Files with no reviewable changes (19)
- bin/lib/platform.js
- bin/lib/nim.js
- bin/lib/config-io.js
- bin/lib/services.js
- bin/lib/version.js
- bin/lib/onboard-session.js
- bin/lib/resolve-openshell.js
- bin/lib/preflight.js
- bin/lib/debug.js
- bin/lib/runtime-recovery.js
- bin/lib/chat-filter.js
- bin/lib/inference-config.js
- src/lib/onboard-session.test.ts
- bin/lib/runner.js
- bin/lib/registry.js
- bin/lib/sandbox-build-context.js
- bin/lib/local-inference.js
- bin/lib/policies.js
- bin/lib/onboard.js
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/check-legacy-migrated-paths.ts (1)
19-38: Consider reducing mapping drift risk between shim maps.
REMOVED_SHIM_MOVESduplicates some keys already present inRUNTIME_MOVES(e.g.,platform,runner,policies,onboard,sandbox-build-context). Consider deriving both from a single source-of-truth structure (or composing one map from another) to avoid future divergence.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/check-legacy-migrated-paths.ts` around lines 19 - 38, REMOVED_SHIM_MOVES duplicates keys that also exist in RUNTIME_MOVES which risks drift; refactor so these maps are derived from a single source-of-truth (e.g., a baseMap or canonicalMoves) or build REMOVED_SHIM_MOVES by composing/filtering RUNTIME_MOVES rather than duplicating entries; update code that references REMOVED_SHIM_MOVES and RUNTIME_MOVES to use the new shared structure and ensure identical key/value pairs for overlapping items like "bin/lib/platform.js"/"src/lib/platform.ts" and "bin/lib/runner.js"/"src/lib/runner.ts".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/check-legacy-migrated-paths.ts`:
- Around line 74-90: getChangedFiles currently only keeps the last path from git
--name-status lines, which misses the source path on rename/copy (status R/C)
and allows legacy guarded files to be bypassed; update getChangedFiles to parse
all path columns from the split line (for R* and C* events there will be a
source and destination) and emit ChangedFile entries for both the source and
destination paths (or otherwise ensure the source path is checked) instead of
only rest[rest.length - 1], referencing the getChangedFiles function and
handling statuses starting with "R" or "C" specially so the legacy guard catches
source-side renames/copies.
---
Nitpick comments:
In `@scripts/check-legacy-migrated-paths.ts`:
- Around line 19-38: REMOVED_SHIM_MOVES duplicates keys that also exist in
RUNTIME_MOVES which risks drift; refactor so these maps are derived from a
single source-of-truth (e.g., a baseMap or canonicalMoves) or build
REMOVED_SHIM_MOVES by composing/filtering RUNTIME_MOVES rather than duplicating
entries; update code that references REMOVED_SHIM_MOVES and RUNTIME_MOVES to use
the new shared structure and ensure identical key/value pairs for overlapping
items like "bin/lib/platform.js"/"src/lib/platform.ts" and
"bin/lib/runner.js"/"src/lib/runner.ts".
🪄 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: CHILL
Plan: Pro
Run ID: ea18f08b-3b0e-442f-8fd7-ae48d316fe34
📒 Files selected for processing (3)
.github/workflows/legacy-path-guard.yamlscripts/check-legacy-migrated-paths.tsscripts/ts-migration/README.md
✅ Files skipped from review due to trivial changes (2)
- .github/workflows/legacy-path-guard.yaml
- scripts/ts-migration/README.md
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/legacy-path-guard.test.ts (1)
33-61: Clean up temporary repos to avoid/tmpbuildup.Line 35 creates a temp repo, but it is never removed. Wrap the test body in
try/finallyand deleterepoDirat the end.♻️ Proposed cleanup refactor
describe("ts-migration:guard", () => { it("blocks renaming a removed shim by checking the source path in R entries", { timeout: 15000 }, () => { const repoDir = initTempRepo("nemoclaw-legacy-guard-"); const originalPath = path.join(repoDir, "bin", "lib", "runner.js"); const renamedPath = path.join(repoDir, "tmp", "runner.js"); + try { - fs.mkdirSync(path.dirname(originalPath), { recursive: true }); - fs.writeFileSync(originalPath, "module.exports = {};\n"); - run("git", ["add", "."], repoDir); - run("git", ["commit", "-m", "base"], repoDir); + fs.mkdirSync(path.dirname(originalPath), { recursive: true }); + fs.writeFileSync(originalPath, "module.exports = {};\n"); + run("git", ["add", "."], repoDir); + run("git", ["commit", "-m", "base"], repoDir); - run("git", ["checkout", "-b", "feature"], repoDir); - fs.mkdirSync(path.dirname(renamedPath), { recursive: true }); - run("git", ["mv", "bin/lib/runner.js", "tmp/runner.js"], repoDir); - run("git", ["commit", "-m", "rename shim"], repoDir); + run("git", ["checkout", "-b", "feature"], repoDir); + fs.mkdirSync(path.dirname(renamedPath), { recursive: true }); + run("git", ["mv", "bin/lib/runner.js", "tmp/runner.js"], repoDir); + run("git", ["commit", "-m", "rename shim"], repoDir); - const result = spawnSync(TSX, [GUARD_SCRIPT, "--base", "main", "--head", "HEAD"], { - cwd: repoDir, - encoding: "utf-8", - }); + const result = spawnSync(TSX, [GUARD_SCRIPT, "--base", "main", "--head", "HEAD"], { + cwd: repoDir, + encoding: "utf-8", + }); - expect(result.status).toBe(1); - expect(`${result.stdout}${result.stderr}`).toContain( - "Removed compatibility shims must not be reintroduced or edited directly:", - ); - expect(`${result.stdout}${result.stderr}`).toContain( - "bin/lib/runner.js -> src/lib/runner.ts", - ); + expect(result.status).toBe(1); + expect(`${result.stdout}${result.stderr}`).toContain( + "Removed compatibility shims must not be reintroduced or edited directly:", + ); + expect(`${result.stdout}${result.stderr}`).toContain( + "bin/lib/runner.js -> src/lib/runner.ts", + ); + } finally { + fs.rmSync(repoDir, { recursive: true, force: true }); + } }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/legacy-path-guard.test.ts` around lines 33 - 61, The test creates a temporary repository via initTempRepo assigned to repoDir but never removes it; wrap the test body (everything after repoDir is set) in a try/finally block and in the finally call a cleanup to remove repoDir (e.g. use fs.rmSync(repoDir, { recursive: true, force: true }) or equivalent) so the temp repo is always deleted even on failures; keep references to the existing symbols (repoDir, initTempRepo, and the spawnSync/GUARD_SCRIPT invocation) and only add the try/finally and cleanup call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/legacy-path-guard.test.ts`:
- Around line 33-61: The test creates a temporary repository via initTempRepo
assigned to repoDir but never removes it; wrap the test body (everything after
repoDir is set) in a try/finally block and in the finally call a cleanup to
remove repoDir (e.g. use fs.rmSync(repoDir, { recursive: true, force: true }) or
equivalent) so the temp repo is always deleted even on failures; keep references
to the existing symbols (repoDir, initTempRepo, and the spawnSync/GUARD_SCRIPT
invocation) and only add the try/finally and cleanup call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e4af5d0a-8445-49e5-8848-7f1b5b4edf7a
📒 Files selected for processing (2)
scripts/check-legacy-migrated-paths.tstest/legacy-path-guard.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/check-legacy-migrated-paths.ts
Contributor
Author
|
I merged the latest Focused validation run:
Those passed. CI is rerunning now. |
realkim93
added a commit
to realkim93/NemoClaw
that referenced
this pull request
Apr 12, 2026
Main underwent a large-scale refactoring while this PR was open: - JS→TS migration: bin/lib/onboard.js → src/lib/onboard.ts, bin/nemoclaw.js → src/nemoclaw.ts (NVIDIA#1673) - Legacy shim removal: 18 bin/lib/*.js files deleted (NVIDIA#1713) - Command handler extraction: src/lib/onboard-command.ts (NVIDIA#1710) - Test renames: .js → .ts across all test files Conflict resolution: - bin/nemoclaw.js: accept main's 6-line launcher (logic in src/nemoclaw.ts) - src/lib/onboard.ts: take main's build context staging (fromDockerfile + agent + stageOptimized), keep Jetson gateway patch functions - docs/get-started/quickstart.md: use main's table format, add Jetson row - test/onboard.test.js: accept deletion (migrated to .ts) Re-ported Jetson features onto new TS structure: - Add setup-jetson to GLOBAL_COMMANDS, help text, and switch dispatch in src/nemoclaw.ts (not a deprecated alias — runs setup-jetson.sh) - Add Jetson to ci/platform-matrix.json (source of truth for docs table) - Add getGatewayImageTag and patchGatewayImageForJetson to exports in src/lib/onboard.ts - Port 4 Jetson tests to test/onboard.test.ts (dist/lib/onboard paths) Verified on Jetson Orin Nano Super (8GB, JetPack 6.x, Node 22.22.0): - Build: tsc passes - 4 Jetson tests: all pass - GPU: jetson=true, 7619 MB unified memory - Gateway: iptables v1.8.10 (legacy), io.nemoclaw.jetson-patched=true - Sandbox: Phase Ready - Inference: openclaw agent → "2 + 2 = 4." - Path: sandbox → host.openshell.internal:11434 → Ollama → nemotron-3-nano:4b Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
realkim93
added a commit
to realkim93/NemoClaw
that referenced
this pull request
Apr 12, 2026
Main underwent a large-scale refactoring while this PR was open: - JS→TS migration: bin/lib/onboard.js → src/lib/onboard.ts, bin/nemoclaw.js → src/nemoclaw.ts (NVIDIA#1673) - Legacy shim removal: 18 bin/lib/*.js files deleted (NVIDIA#1713) - Command handler extraction: src/lib/onboard-command.ts (NVIDIA#1710) - Test renames: .js → .ts across all test files Conflict resolution: - bin/nemoclaw.js: accept main's 6-line launcher (logic in src/nemoclaw.ts) - src/lib/onboard.ts: take main's build context staging (fromDockerfile + agent + stageOptimized), keep Jetson gateway patch functions - docs/get-started/quickstart.md: use main's table format, add Jetson row - test/onboard.test.js: accept deletion (migrated to .ts) Re-ported Jetson features onto new TS structure: - Add setup-jetson to GLOBAL_COMMANDS, help text, and switch dispatch in src/nemoclaw.ts (not a deprecated alias — runs setup-jetson.sh) - Add Jetson to ci/platform-matrix.json (source of truth for docs table) - Add getGatewayImageTag and patchGatewayImageForJetson to exports in src/lib/onboard.ts - Port 4 Jetson tests to test/onboard.test.ts (dist/lib/onboard paths) Verified on Jetson Orin Nano Super (8GB, JetPack 6.x, Node 22.22.0): - Build: tsc passes - 4 Jetson tests: all pass - GPU: jetson=true, 7619 MB unified memory - Gateway: iptables v1.8.10 (legacy), io.nemoclaw.jetson-patched=true - Sandbox: Phase Ready - Inference: openclaw agent → "2 + 2 = 4." - Path: sandbox → host.openshell.internal:11434 → Ollama → nemotron-3-nano:4b Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
9 tasks
10 tasks
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
Remove the legacy
bin/libcompatibility wrappers that no longer carry behavior, and point the CLI/tests at the compileddistoutput or local TS modules instead. This trims the remaining root-level JavaScript surface while keeping the launcher and the two wrappers that still provide real compatibility behavior.Changes
bin/lib/*.jsshimsbin/nemoclaw.js,bin/lib/usage-notice.js, andbin/lib/credentials.jsbin/lib/*to localsrc/lib/*modules where safedist/lib/*dist/in the published packageType of Change
Testing
npx prek run --all-filespasses (or equivalentlymake check).npm testpasses.make docsbuilds without warnings. (for doc-only changes)Checklist
General
Code Changes
npx prek run --all-filesauto-fixes formatting (ormake formatfor targeted runs).Signed-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit