refactor(cli): migrate remaining CJS modules to TypeScript#1306
Merged
Conversation
Migrate resolve-openshell.js to TypeScript and create two new TS modules (version.ts, chat-filter.ts), completing the "everything not blocked is converted" goal for #1298 / #924 shell consolidation. For each module: - TS implementation in nemoclaw/src/lib/ with typed interfaces - Thin CJS re-export shim in bin/lib/ pointing to compiled dist/ - Co-located tests importing from ../../dist/lib/ for coverage attribution Consumers updated: - bin/nemoclaw.js uses getVersion() for --version and help output - scripts/telegram-bridge.js uses parseAllowedChatIds/isChatAllowed Closes #1298 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThree CLI utility modules (chat-filter, resolve-openshell, version) were implemented in TypeScript under Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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.
Actionable comments posted: 1
🧹 Nitpick comments (2)
nemoclaw/src/lib/resolve-openshell.test.ts (1)
85-93: Consider consolidating with the similar test case above.This test appears functionally identical to "returns null when openshell not found anywhere" (lines 66-73). Both invoke
resolveOpenshellwithcommandVResult: nullandcheckExecutable: () => false; passinghome: undefinedexplicitly has no effect sinceopts.home ?? process.env.HOMEtreats missing and explicitundefinedidentically.If the intent is to document the explicit undefined case, a brief comment would clarify. Otherwise, this could be removed to reduce duplication.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nemoclaw/src/lib/resolve-openshell.test.ts` around lines 85 - 93, The two tests in resolve-openshell.test.ts are duplicated: both call resolveOpenshell with commandVResult: null and checkExecutable: () => false (the second also passes home: undefined which is redundant). Remove the redundant test ("returns null for null commandVResult with no executable found") or consolidate by keeping a single test (e.g., "returns null when openshell not found anywhere") and, if you want to document the explicit undefined case, add a one-line comment in that retained test noting that passing home: undefined is equivalent to omitting the option; ensure the retained test still calls resolveOpenshell and asserts toBeNull so resolveOpenshell behavior is covered once.bin/nemoclaw.js (1)
403-406: Skip git probing in plain semver CLI output paths.Line 403 and Line 475 only need
version, butgetVersion()default behavior executes a synchronousgit describe --tags --always --dirtycall. PassgitDescribeResult: nullat these call sites to keep help/version output fast and avoid unnecessary subprocess execution.Proposed fix
function help() { - const { version } = getVersion(); + const { version } = getVersion({ gitDescribeResult: null }); console.log(` ${B}${G}NemoClaw${R} ${D}v${version}${R} @@ case "--version": case "-v": { - const { version } = getVersion(); + const { version } = getVersion({ gitDescribeResult: null }); console.log(`nemoclaw v${version}`); break; }Also applies to: 475-476
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/nemoclaw.js` around lines 403 - 406, The CLI prints plain semver output but calls getVersion() which runs a synchronous git describe subprocess; to avoid git probing in these fast paths, call getVersion({ gitDescribeResult: null }) when extracting version (e.g., the lines using const { version } = getVersion() around the help/version output and the second occurrence near line 475) so only the version is returned without executing git describe; update both call sites to pass the gitDescribeResult: null option.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@nemoclaw/src/lib/version.test.ts`:
- Around line 4-5: Tests use hardcoded POSIX path strings like
'/test-dir/package.json' which breaks cross-platform behavior; update the mocks
in version.test.ts to construct expected storage keys using node:path join
(e.g., import { join } from "node:path") and use join("test-dir",
"package.json") wherever the test currently uses '/test-dir/package.json' so the
mocked keys match the implementation that uses join() (refer to the test file
and the getVersion import for where these mocks are set).
---
Nitpick comments:
In `@bin/nemoclaw.js`:
- Around line 403-406: The CLI prints plain semver output but calls getVersion()
which runs a synchronous git describe subprocess; to avoid git probing in these
fast paths, call getVersion({ gitDescribeResult: null }) when extracting version
(e.g., the lines using const { version } = getVersion() around the help/version
output and the second occurrence near line 475) so only the version is returned
without executing git describe; update both call sites to pass the
gitDescribeResult: null option.
In `@nemoclaw/src/lib/resolve-openshell.test.ts`:
- Around line 85-93: The two tests in resolve-openshell.test.ts are duplicated:
both call resolveOpenshell with commandVResult: null and checkExecutable: () =>
false (the second also passes home: undefined which is redundant). Remove the
redundant test ("returns null for null commandVResult with no executable found")
or consolidate by keeping a single test (e.g., "returns null when openshell not
found anywhere") and, if you want to document the explicit undefined case, add a
one-line comment in that retained test noting that passing home: undefined is
equivalent to omitting the option; ensure the retained test still calls
resolveOpenshell and asserts toBeNull so resolveOpenshell behavior is covered
once.
🪄 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: 80b5dc42-8876-4e4d-91e4-0f6a18bc9db2
📒 Files selected for processing (11)
bin/lib/chat-filter.jsbin/lib/resolve-openshell.jsbin/lib/version.jsbin/nemoclaw.jsnemoclaw/src/lib/chat-filter.test.tsnemoclaw/src/lib/chat-filter.tsnemoclaw/src/lib/resolve-openshell.test.tsnemoclaw/src/lib/resolve-openshell.tsnemoclaw/src/lib/version.test.tsnemoclaw/src/lib/version.tsscripts/telegram-bridge.js
cv
requested changes
Apr 2, 2026
Contributor
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
Thanks for picking this up @prekshivyas — the scope and approach are right.
One structural issue to fix, then this is close to ready:
TS files are in the wrong directory. All three modules are in nemoclaw/src/lib/ — that is the OpenClaw plugin directory (Commander-based, separate tsconfig, different runtime). CLI modules belong in src/lib/ at the repo root, compiled via tsconfig.src.json → dist/lib/. The bin/lib/ shims point at ../../dist/lib/, not into the plugin tree.
See any of the recently merged migrations for the pattern:
The pattern is:
src/lib/foo.ts ← TS source
src/lib/foo.test.ts ← co-located test (imports from ../../dist/lib/foo)
bin/lib/foo.js ← thin shim: module.exports = require("../../dist/lib/foo")
Also needs a rebase — there are merge conflicts with main.
The actual module implementations look correct. Once the directory is fixed and conflicts resolved, this should be straightforward to land.
This was referenced Apr 2, 2026
4 tasks
Migrate resolve-openshell.js to TypeScript and create two new TS modules (version.ts, chat-filter.ts), completing the "everything not blocked is converted" goal for #1298 / #924 shell consolidation. For each module: - TS implementation in nemoclaw/src/lib/ with typed interfaces - Thin CJS re-export shim in bin/lib/ pointing to compiled dist/ - Co-located tests importing from ../../dist/lib/ for coverage attribution Consumers updated: - bin/nemoclaw.js uses getVersion() for --version and help output - scripts/telegram-bridge.js uses parseAllowedChatIds/isChatAllowed Closes #1298 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Address review feedback from @cv and CodeRabbit: - Move TS files from nemoclaw/src/lib/ to src/lib/ (repo root), compiled via tsconfig.src.json to CJS in dist/lib/ - Preserve getVersion() string return type (matching existing API) - Preserve isChatAllowed(allowedChats, chatId) array-based signature - Update bin/lib/ shims to point to dist/lib/ (not nemoclaw/dist/) - Use platform-neutral paths in version tests (real temp dirs) - Restore bin/nemoclaw.js and telegram-bridge.js to main versions (no consumer changes needed — API is backward-compatible) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/lib/version.test.ts (1)
13-35: Cover the git-first branch with a regression test.Every case here bypasses git by construction, so tag-format regressions in
getVersion()can ship without this suite failing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/version.test.ts` around lines 13 - 35, Add a regression test that exercises the git-first branch of getVersion by initializing a real git repo in the existing testDir, committing the package.json, creating an annotated tag like "v2.0.0" (set user.name/user.email locally so commit succeeds), ensure no .version file is present, call getVersion({ rootDir: testDir }) and assert it returns "2.0.0" (or the tag without "v" if getVersion strips it), then clean up the .git directory; reference getVersion and the test file version.test.ts so the new test sits alongside the other cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/version.ts`:
- Around line 25-30: The current call to git describe strips the leading "v"
from any output, which turns a non-exact describe like "v1.2.3-4-gHASH" into
"1.2.3-4-gHASH" and changes development commit output; modify getVersion() so
you first run git describe --tags --match "v*" --exact-match (store e.g.
rawExact) and if that returns a value return rawExact.replace(/^v/, "");
otherwise run the fallback git describe --tags --match "v*" (the existing
command that may return the -N-gHASH form) and return that raw string unchanged;
update the execFileSync calls that produce raw to implement this two-step logic
(refer to getVersion, rawExact/raw variables).
---
Nitpick comments:
In `@src/lib/version.test.ts`:
- Around line 13-35: Add a regression test that exercises the git-first branch
of getVersion by initializing a real git repo in the existing testDir,
committing the package.json, creating an annotated tag like "v2.0.0" (set
user.name/user.email locally so commit succeeds), ensure no .version file is
present, call getVersion({ rootDir: testDir }) and assert it returns "2.0.0" (or
the tag without "v" if getVersion strips it), then clean up the .git directory;
reference getVersion and the test file version.test.ts so the new test sits
alongside the other cases.
🪄 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: ebd67ddc-049e-46a1-a880-1e09862abe5b
📒 Files selected for processing (9)
bin/lib/chat-filter.jsbin/lib/resolve-openshell.jsbin/lib/version.jssrc/lib/chat-filter.test.tssrc/lib/chat-filter.tssrc/lib/resolve-openshell.test.tssrc/lib/resolve-openshell.tssrc/lib/version.test.tssrc/lib/version.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- bin/lib/resolve-openshell.js
Comment on lines
+25
to
+30
| const raw = execFileSync("git", ["describe", "--tags", "--match", "v*"], { | ||
| cwd: root, | ||
| encoding: "utf-8", | ||
| stdio: ["ignore", "pipe", "ignore"], | ||
| }).trim(); | ||
| if (raw) return raw.replace(/^v/, ""); |
Contributor
There was a problem hiding this comment.
Keep the git path on the plain release version.
git describe --tags --match "v*" starts returning vX.Y.Z-N-gHASH as soon as HEAD moves past the last tag, so this branch changes getVersion() output on normal development commits.
Proposed fix
- const raw = execFileSync("git", ["describe", "--tags", "--match", "v*"], {
+ const raw = execFileSync("git", ["describe", "--tags", "--match", "v*", "--abbrev=0"], {#!/bin/bash
set -euo pipefail
printf 'git describe --exact-match: '
git describe --tags --match 'v*' --exact-match || true
printf 'git describe: '
git describe --tags --match 'v*' || true
printf 'git describe --abbrev=0: '
git describe --tags --match 'v*' --abbrev=0 || true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/version.ts` around lines 25 - 30, The current call to git describe
strips the leading "v" from any output, which turns a non-exact describe like
"v1.2.3-4-gHASH" into "1.2.3-4-gHASH" and changes development commit output;
modify getVersion() so you first run git describe --tags --match "v*"
--exact-match (store e.g. rawExact) and if that returns a value return
rawExact.replace(/^v/, ""); otherwise run the fallback git describe --tags
--match "v*" (the existing command that may return the -N-gHASH form) and return
that raw string unchanged; update the execFileSync calls that produce raw to
implement this two-step logic (refer to getVersion, rawExact/raw variables).
The "null commandVResult with home: undefined" test was functionally identical to "returns null when openshell not found anywhere" since opts.home ?? process.env.HOME treats undefined identically. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
cv
approved these changes
Apr 2, 2026
Contributor
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
Directory fixed, shims correct, CI green. The version format CodeRabbit concern is pre-existing behavior from the original JS, not a regression.
3 tasks
cjagwani
pushed a commit
to fdzdev/NemoClaw
that referenced
this pull request
Apr 3, 2026
## Summary Closes NVIDIA#1298. Part of NVIDIA#924 shell consolidation. - **resolve-openshell.js** → TS migration with `ResolveOpenshellOptions` interface; bin/lib shim re-exports from compiled dist - **version.ts** → new module reading root package.json + `git describe`; wired into `bin/nemoclaw.js` for `--version` and help (output unchanged — uses plain semver) - **chat-filter.ts** → new utility extracting `ALLOWED_CHAT_IDS` parsing from telegram-bridge; wired into `scripts/telegram-bridge.js` Each module follows the established pattern: 1. TS source in `nemoclaw/src/lib/` with flat interfaces 2. Thin CJS re-export shim in `bin/lib/` → `require("../../nemoclaw/dist/lib/...")` 3. Co-located tests in `src/lib/*.test.ts` importing from `../../dist/lib/` for coverage attribution ## Test plan - [x] `npx vitest run --project plugin` — 216 tests pass (includes new lib tests) - [x] `npx vitest run --project cli` — 336 tests pass (shims work transparently) - [x] `npx eslint nemoclaw/src/lib/` — clean - [x] `node bin/nemoclaw.js --version` — output unchanged (`nemoclaw v0.1.0`) - [x] Pre-commit hooks pass (prettier, eslint, gitleaks, vitest, tsc) 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added utilities for chat allowlist handling, openshell binary resolution, and robust version detection. * **Refactor** * Consolidated runtime implementations into the compiled distribution to streamline module surfaces. * **Tests** * Added comprehensive tests covering chat filtering, shell resolution fallbacks, and version-detection behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Prekshi Vyas <prekshivyas@nvidia.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: Carlos Villela <cvillela@nvidia.com>
lakamsani
pushed a commit
to lakamsani/NemoClaw
that referenced
this pull request
Apr 4, 2026
## Summary Closes NVIDIA#1298. Part of NVIDIA#924 shell consolidation. - **resolve-openshell.js** → TS migration with `ResolveOpenshellOptions` interface; bin/lib shim re-exports from compiled dist - **version.ts** → new module reading root package.json + `git describe`; wired into `bin/nemoclaw.js` for `--version` and help (output unchanged — uses plain semver) - **chat-filter.ts** → new utility extracting `ALLOWED_CHAT_IDS` parsing from telegram-bridge; wired into `scripts/telegram-bridge.js` Each module follows the established pattern: 1. TS source in `nemoclaw/src/lib/` with flat interfaces 2. Thin CJS re-export shim in `bin/lib/` → `require("../../nemoclaw/dist/lib/...")` 3. Co-located tests in `src/lib/*.test.ts` importing from `../../dist/lib/` for coverage attribution ## Test plan - [x] `npx vitest run --project plugin` — 216 tests pass (includes new lib tests) - [x] `npx vitest run --project cli` — 336 tests pass (shims work transparently) - [x] `npx eslint nemoclaw/src/lib/` — clean - [x] `node bin/nemoclaw.js --version` — output unchanged (`nemoclaw v0.1.0`) - [x] Pre-commit hooks pass (prettier, eslint, gitleaks, vitest, tsc) 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added utilities for chat allowlist handling, openshell binary resolution, and robust version detection. * **Refactor** * Consolidated runtime implementations into the compiled distribution to streamline module surfaces. * **Tests** * Added comprehensive tests covering chat filtering, shell resolution fallbacks, and version-detection behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Prekshi Vyas <prekshivyas@nvidia.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: Carlos Villela <cvillela@nvidia.com>
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
Closes #1298. Part of #924 shell consolidation.
ResolveOpenshellOptionsinterface; bin/lib shim re-exports from compiled distgit describe; wired intobin/nemoclaw.jsfor--versionand help (output unchanged — uses plain semver)ALLOWED_CHAT_IDSparsing from telegram-bridge; wired intoscripts/telegram-bridge.jsEach module follows the established pattern:
nemoclaw/src/lib/with flat interfacesbin/lib/→require("../../nemoclaw/dist/lib/...")src/lib/*.test.tsimporting from../../dist/lib/for coverage attributionTest plan
npx vitest run --project plugin— 216 tests pass (includes new lib tests)npx vitest run --project cli— 336 tests pass (shims work transparently)npx eslint nemoclaw/src/lib/— cleannode bin/nemoclaw.js --version— output unchanged (nemoclaw v0.1.0)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Refactor
Tests