refactor(cli): migrate onboard-session.js to TypeScript

[cv]

Summary

• Convert bin/lib/onboard-session.js (440 lines) to src/lib/onboard-session.ts
• Typed interfaces: Session, StepState, SessionFailure, LockResult, SessionUpdates, LockInfo, SessionMetadata
• Full migration including fs I/O (session persistence, file locking) and pure helpers (redaction, validation, normalization)
• Co-locate tests (14 tests) with updated cache-clearing for dist/ path

Stacked on #1240. 615 CLI tests pass. Coverage ratchet passes.

Relates to #924 (shell consolidation).

🤖 Generated with Claude Code

Summary by CodeRabbit

• Refactor
  • Reorganized onboarding session management module structure and improved distribution of the codebase. Session tracking, step-level state management, lock handling, and sensitive data redaction functionality have been consolidated and optimized for better maintainability.

[coderabbitai]

📝 Walkthrough

Walkthrough

Reorganized session management code from a JavaScript implementation in bin/lib/ to a new TypeScript implementation in src/lib/, with the original bin/ file converted to a thin re-export shim of the compiled dist/ module. Updated tests to handle module caching for both entrypoints.

Changes

Cohort / File(s)	Summary
Shim Re-export bin/lib/onboard-session.js	Replaced entire implementation with single re-export from compiled dist module; all previously defined constants, functions, and helpers now sourced from build output.
Test Module Loading src/lib/onboard-session.test.ts	Updated module cache resolution to handle both shim and dist paths; switched test to require compiled module from dist while maintaining cache cleanup for both entrypoints.
Session Management Implementation src/lib/onboard-session.ts	New TypeScript module providing file-backed session persistence at ~/.nemoclaw/onboard-session.json, including session lifecycle (create/load/save/clear), file-based locking with process liveness detection, step-level state tracking, atomic updates, and sensitive data redaction utilities.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Poem

🐰 Hoppy refactoring times,
From JavaScript to TypeScript rhymes,
Sessions saved with lock and key,
Sensitive data we shall be,
Built from source to dist they go—
Onboarding flows now steal the show! 🔐✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'refactor(cli): migrate onboard-session.js to TypeScript' clearly and specifically describes the primary change—converting a JavaScript module to TypeScript.

_{✏️ 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 cv/migrate-onboard-session-ts

---

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

[coderabbitai]

Actionable comments posted: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/lib/onboard-session.test.ts (1)

19-35: ⚠️ Potential issue | 🟡 Minor

The shared temp HOME makes these lock tests order-dependent.

These hooks reuse one tmpDir for the whole file, but releaseOnboardLock() intentionally leaves malformed lock files behind. After the malformed-lock cases, later tests inherit that state because the hooks only clear the module cache and the session file. Recreate tmpDir per test, or wipe it in the hooks.

One simple cleanup option

 beforeEach(() => {
+  fs.rmSync(tmpDir, { recursive: true, force: true });
+  fs.mkdirSync(tmpDir, { recursive: true, mode: 0o700 });
   process.env.HOME = tmpDir;
   delete require.cache[shimPath];
   delete require.cache[distPath];
   session = require("../../dist/lib/onboard-session");
   session.clearSession();
   session.releaseOnboardLock();
 });
 
 afterEach(() => {
   delete require.cache[shimPath];
   delete require.cache[distPath];
+  fs.rmSync(tmpDir, { recursive: true, force: true });
   if (originalHome === undefined) {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/lib/onboard-session.test.ts` around lines 19 - 35, The tests reuse a
single tmpDir across the file which causes order-dependent failures because
session.releaseOnboardLock() leaves malformed lock files; update the
beforeEach/afterEach hooks to create a fresh temporary directory for each test
(or delete all files inside tmpDir) and set process.env.HOME to that fresh
directory, ensuring you still delete require.cache for shimPath and distPath and
call session.clearSession() and session.releaseOnboardLock() as before; target
the beforeEach/afterEach functions and symbols tmpDir, process.env.HOME,
session.clearSession(), and session.releaseOnboardLock() when applying the
change.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@bin/lib/onboard-session.js`:
- Line 7: Replace the shim export in bin/lib/onboard-session.js (currently
"module.exports = require('../../dist/lib/onboard-session')") with the actual
CommonJS implementation so the CLI can run without a build; locate the logic in
dist/lib/onboard-session (or the source TS/JS equivalent), convert/inline that
implementation into bin/lib/onboard-session.js as a plain CommonJS module
(exports or module.exports) and remove any dependency on dist/, ensuring
bin/nemoclaw.js can require('./lib/onboard-session') in a fresh checkout/npm
install.

In `@src/lib/onboard-session.ts`:
- Around line 204-251: normalizeSession is too complex and currently bypasses
the complexity rule; split its responsibilities into small helpers. Create
helper functions such as coerceSessionFields(data: Record<string, unknown>) ->
Partial<Session> to handle top-level field coercion and redaction (use
redactUrl, sanitizeFailure), normalizeMetadata(metadata: unknown) ->
SessionMetadata|undefined, and normalizeSteps(steps: unknown, currentSteps:
Session['steps']) -> Session['steps'] to validate and populate individual step
entries (use validateStep and redactSensitiveText). Replace the inline logic in
normalizeSession to call these helpers, remove the eslint-disable-next-line
complexity waiver, and keep behavior identical (set normalized.resumable and
normalized.status as before). Ensure the new helpers are exported or file-scoped
so tests/other code can be updated if needed.
- Around line 336-365: The current stale-lock cleanup unlinks LOCK_FILE directly
which allows a race where a concurrent process recreates the lock and the slower
process then deletes it; change to an atomic stale-claim approach: when you
detect a stale lock (parseLockFile(...) returned existing and
isProcessAlive(...) is false), write a temporary file (e.g.,
`${LOCK_FILE}.${process.pid}.tmp`) with your PID/startedAt/command and then
attempt to atomically create the real lock using fs.linkSync(tempPath,
LOCK_FILE) (this call will succeed only if LOCK_FILE does not exist); if
linkSync succeeds you have acquired the lock (remove tempPath and return
acquired=true), if it fails because LOCK_FILE now exists remove tempPath and
continue retrying; do not call fs.unlinkSync(LOCK_FILE) unconditionally—only
remove stale files after successfully claiming the lock or when you can safely
determine nobody else replaced it. Use parseLockFile, LOCK_FILE, isProcessAlive,
and fs.linkSync/fs.writeFileSync/fs.unlinkSync to implement this.
- Around line 227-229: The metadata assignment currently copies gatewayName
regardless of type, allowing non-string values to persist; update the
normalization in the metadata block (used by loadSession/createSession flows) to
only set metadata.gatewayName when isObject(d.metadata) is true and (d.metadata
as Record<string, unknown>).gatewayName is a string (typeof === 'string'),
otherwise leave metadata undefined or omit gatewayName; ensure you reference the
existing isObject check and the SessionMetadata type so only string gatewayName
values are propagated.

---

Outside diff comments:
In `@src/lib/onboard-session.test.ts`:
- Around line 19-35: The tests reuse a single tmpDir across the file which
causes order-dependent failures because session.releaseOnboardLock() leaves
malformed lock files; update the beforeEach/afterEach hooks to create a fresh
temporary directory for each test (or delete all files inside tmpDir) and set
process.env.HOME to that fresh directory, ensuring you still delete
require.cache for shimPath and distPath and call session.clearSession() and
session.releaseOnboardLock() as before; target the beforeEach/afterEach
functions and symbols tmpDir, process.env.HOME, session.clearSession(), and
session.releaseOnboardLock() when applying the change.

🪄 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: 356d71e7-e42a-411b-862f-93c6eab61708

📥 Commits

Reviewing files that changed from the base of the PR and between 2804b74 and 3e67453.

📒 Files selected for processing (3)

• bin/lib/onboard-session.js
• src/lib/onboard-session.test.ts
• src/lib/onboard-session.ts

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "Tracked onboard-session entrypoints:"
git ls-files -- bin/lib/onboard-session.js dist/lib/onboard-session.js src/lib/onboard-session.ts

echo
echo "Packaging/build hooks that mention bin/dist:"
rg -n -C2 '"(bin|files|prepare|prepack|postinstall|build:cli)"|dist/lib/onboard-session|bin/lib/onboard-session' \
  package.json README.md .npmignore .gitignore 2>/dev/null || true

Repository: NVIDIA/NemoClaw

Length of output: 1400

---

Remove the shim and keep the implementation in bin/lib/onboard-session.js.

The shim at line 7 delegates to dist/lib/onboard-session.js, which is not tracked in git and not included in the npm package. This breaks the bin/ contract that requires working without a build step—bin/nemoclaw.js will fail to load this module in a clean checkout or fresh npm install.

Either restore the implementation directly in bin/lib/onboard-session.js (as CommonJS) or restructure to avoid the dist/ dependency in the CLI path.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@bin/lib/onboard-session.js` at line 7, Replace the shim export in
bin/lib/onboard-session.js (currently "module.exports =
require('../../dist/lib/onboard-session')") with the actual CommonJS
implementation so the CLI can run without a build; locate the logic in
dist/lib/onboard-session (or the source TS/JS equivalent), convert/inline that
implementation into bin/lib/onboard-session.js as a plain CommonJS module
(exports or module.exports) and remove any dependency on dist/, ensuring
bin/nemoclaw.js can require('./lib/onboard-session') in a fresh checkout/npm
install.

[coderabbitai]

🛠️ Refactor suggestion | 🟠 Major

Split normalizeSession instead of waiving the complexity check.

This deserializer is already doing schema coercion, redaction, defaults, and step normalization in one place. Please extract smaller helpers rather than landing the new complexity waiver here.

As per coding guidelines, **/*.{js,ts,tsx}: Maintain cyclomatic complexity limit of 20 (target: ratchet down to 15) for all functions.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/lib/onboard-session.ts` around lines 204 - 251, normalizeSession is too
complex and currently bypasses the complexity rule; split its responsibilities
into small helpers. Create helper functions such as coerceSessionFields(data:
Record<string, unknown>) -> Partial<Session> to handle top-level field coercion
and redaction (use redactUrl, sanitizeFailure), normalizeMetadata(metadata:
unknown) -> SessionMetadata|undefined, and normalizeSteps(steps: unknown,
currentSteps: Session['steps']) -> Session['steps'] to validate and populate
individual step entries (use validateStep and redactSensitiveText). Replace the
inline logic in normalizeSession to call these helpers, remove the
eslint-disable-next-line complexity waiver, and keep behavior identical (set
normalized.resumable and normalized.status as before). Ensure the new helpers
are exported or file-scoped so tests/other code can be updated if needed.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Validate metadata.gatewayName before copying it.

A persisted payload like { "metadata": { "gatewayName": 123 } } currently survives normalization. Because createSession() copies truthy gatewayName values through, loadSession() can return a Session whose metadata.gatewayName is not a string.

Suggested fix

-    metadata: isObject(d.metadata)
-      ? ({ gatewayName: (d.metadata as Record<string, unknown>).gatewayName } as SessionMetadata)
-      : undefined,
+    metadata:
+      isObject(d.metadata) &&
+      typeof (d.metadata as Record<string, unknown>).gatewayName === "string"
+        ? { gatewayName: (d.metadata as Record<string, unknown>).gatewayName as string }
+        : undefined,

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/lib/onboard-session.ts` around lines 227 - 229, The metadata assignment
currently copies gatewayName regardless of type, allowing non-string values to
persist; update the normalization in the metadata block (used by
loadSession/createSession flows) to only set metadata.gatewayName when
isObject(d.metadata) is true and (d.metadata as Record<string,
unknown>).gatewayName is a string (typeof === 'string'), otherwise leave
metadata undefined or omit gatewayName; ensure you reference the existing
isObject check and the SessionMetadata type so only string gatewayName values
are propagated.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

The stale-lock cleanup can let two processes acquire the lock.

If two contenders read the same stale file, the slower one can reach Line 360 after the faster one has already recreated LOCK_FILE, unlink that fresh lock, and then win on its next retry. That breaks the mutual-exclusion guarantee. Please use an atomic stale-lock claim strategy here, or switch this path to a proven lockfile implementation.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/lib/onboard-session.ts` around lines 336 - 365, The current stale-lock
cleanup unlinks LOCK_FILE directly which allows a race where a concurrent
process recreates the lock and the slower process then deletes it; change to an
atomic stale-claim approach: when you detect a stale lock (parseLockFile(...)
returned existing and isProcessAlive(...) is false), write a temporary file
(e.g., `${LOCK_FILE}.${process.pid}.tmp`) with your PID/startedAt/command and
then attempt to atomically create the real lock using fs.linkSync(tempPath,
LOCK_FILE) (this call will succeed only if LOCK_FILE does not exist); if
linkSync succeeds you have acquired the lock (remove tempPath and return
acquired=true), if it fails because LOCK_FILE now exists remove tempPath and
continue retrying; do not call fs.unlinkSync(LOCK_FILE) unconditionally—only
remove stale files after successfully claiming the lock or when you can safely
determine nobody else replaced it. Use parseLockFile, LOCK_FILE, isProcessAlive,
and fs.linkSync/fs.writeFileSync/fs.unlinkSync to implement this.

[ericksoa]

Faithful migration of the largest module in the series. Pre-existing issues tracked separately:

• #1281 — stale-lock race condition in acquireOnboardLock
• #1282 — normalizeSession cyclomatic complexity
• #1283 — metadata.gatewayName not type-validated
• #1284 — lock tests share tmpDir causing order dependence

5 behind main — needs a merge.