Skip to content

refactor(cli): migrate resolve-openshell, version, chat-filter to TypeScript#1357

Closed
prekshivyas wants to merge 1 commit intomainfrom
refactor/migrate-cjs-to-ts-1298-v2
Closed

refactor(cli): migrate resolve-openshell, version, chat-filter to TypeScript#1357
prekshivyas wants to merge 1 commit intomainfrom
refactor/migrate-cjs-to-ts-1298-v2

Conversation

@prekshivyas
Copy link
Copy Markdown
Contributor

@prekshivyas prekshivyas commented Apr 2, 2026

Summary

Closes #1298. Part of #924 shell consolidation. Supersedes #1306 (addresses all review feedback from @cv and CodeRabbit).

Changes from v1 (#1306):

Files changed

File Change
src/lib/resolve-openshell.ts New — TS source with typed DI interface
src/lib/version.ts New — TS source preserving existing getVersion() string API
src/lib/chat-filter.ts New — TS source preserving existing array-based API
src/lib/*.test.ts New — co-located tests importing from ../../dist/lib/
bin/lib/resolve-openshell.js Changed — thin shim replacing inline implementation
bin/lib/version.js Changed — thin shim replacing inline implementation
bin/lib/chat-filter.js Changed — thin shim replacing inline implementation

Test plan

  • npm test — all 874 tests pass (45 files)
  • npm run typecheck:cli — clean
  • Pre-commit hooks pass (prettier, eslint, gitleaks, vitest, commitlint)
  • Smoke-tested all 3 shims via node -e "require(...)" — backward compatible

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Added chat allowlist filtering to control which chats are accepted
    • Added openshell executable resolution with fallback detection across system directories
    • Added flexible version resolution supporting git tags, version files, and package metadata
  • Tests

    • Comprehensive test coverage added for chat filtering, shell resolution, and version detection

…eScript (#1298)

Move CJS implementations in `bin/lib/` to TypeScript in `src/lib/`,
compiled via `tsconfig.src.json` to `dist/lib/`. Replaces the inline
CJS with thin re-export shims following the pattern from #1262, #1265.

- Add `src/lib/resolve-openshell.ts` with typed DI options interface
- Add `src/lib/version.ts` preserving existing getVersion() string API
  (git describe → .version file → package.json fallback chain)
- Add `src/lib/chat-filter.ts` preserving existing array-based API
- Add co-located tests importing from `../../dist/lib/` for coverage
- Replace `bin/lib/` full implementations with thin shims
- Use platform-neutral paths in version tests (node:path join)

No API changes — all consumers (bin/nemoclaw.js, scripts/telegram-bridge.js)
continue to work without modification.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Apr 2, 2026

Caution

Review failed

Pull request was closed or merged during review

📝 Walkthrough

Walkthrough

Migrates three CommonJS modules (chat-filter.js, resolve-openshell.js, version.js) from bin/lib/ to TypeScript in src/lib/, replacing the originals with thin re-export shims pointing to compiled dist versions. Includes comprehensive test suites for each module.

Changes

Cohort / File(s) Summary
Chat filter module
src/lib/chat-filter.ts, src/lib/chat-filter.test.ts, bin/lib/chat-filter.js
New TypeScript implementation for chat ID allowlist parsing and validation; bin shim re-exports from dist. Tests verify parsing handles empty/whitespace inputs and comma-separated IDs, and allowlist checks support null (allow-all) and array membership logic.
Resolve openshell module
src/lib/resolve-openshell.ts, src/lib/resolve-openshell.test.ts, bin/lib/resolve-openshell.js
New TypeScript module for openshell binary resolution with two-stage fallback: command-v lookup (rejecting non-absolute paths/aliases) then probing ~/.local/bin, /usr/local/bin, /usr/bin; bin shim re-exports from dist. Tests validate absolute path passthrough, alias rejection, home-based fallback priority, and null handling.
Version module
src/lib/version.ts, src/lib/version.test.ts, bin/lib/version.js
New TypeScript implementation for project version resolution with prioritized fallback chain: git describe → .version file → package.json; bin shim re-exports from dist. Tests verify fallback behavior and confirm string output.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 From bin to src, the code takes flight,
Three modules dancing in TypeScript light,
With tests that frolic and types so tight,
Our cli toolbox shimmers bright! ✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: migrating three CLI modules from CommonJS to TypeScript.
Linked Issues check ✅ Passed The PR successfully implements all coding objectives from #1298: migrating resolve-openshell.js, version.js, and chat-filter.js to TypeScript sources under src/lib/, compiling to dist/lib/, replacing bin/lib/ with thin shims, adding co-located tests, and preserving existing APIs.
Out of Scope Changes check ✅ Passed All changes are directly related to the stated objective of migrating three CommonJS modules to TypeScript with minimal scope; no out-of-scope modifications detected.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch refactor/migrate-cjs-to-ts-1298-v2

Comment @coderabbitai help to get the list of available commands and usage tips.

@prekshivyas
Copy link
Copy Markdown
Contributor Author

Closing — will address feedback in the original PR #1306 instead.

@prekshivyas prekshivyas closed this Apr 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

refactor(cli): migrate remaining unblocked CJS modules to TypeScript

1 participant