refactor(cli): migrate runtime-recovery.js to TypeScript

[cv]

Summary

• Convert bin/lib/runtime-recovery.js (84 lines) to src/lib/runtime-recovery.ts
• Typed StateClassification interface for sandbox/gateway state
• 5 pure classifiers/parsers + 1 I/O function (getRecoveryCommand)
• Co-locate tests with additional edge cases

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

Relates to #924 (shell consolidation).

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • Improved runtime recovery logic to better parse CLI output and determine recovery actions, including more reliable sandbox/gateway state classification and recovery command selection.

• Bug Fixes

  • Correctly treats "Disconnected" gateway status as inactive and handles error/empty lines when listing sandboxes.

• Tests

  • Expanded test coverage for edge cases, malformed input, and empty states.

• Refactor

  • Runtime recovery helpers consolidated to a shared compiled module.

[coderabbitai]

📝 Walkthrough

Walkthrough

The PR moves runtime-recovery logic into a new TypeScript module (src/lib/runtime-recovery.ts) and replaces the prior bin entrypoint implementation with a thin re-export of the compiled dist module; tests updated to target the dist build and add/adjust cases.

Changes

Cohort / File(s)	Summary
TypeScript Source Implementation src/lib/runtime-recovery.ts	New TypeScript module exporting StateClassification and functions: parseLiveSandboxNames, classifySandboxLookup, classifyGatewayStatus, shouldAttemptGatewayRecovery, and getRecoveryCommand. Implements ANSI stripping, parsing, classification rules, and onboard-session resume logic.
Node.js Shim (bin) bin/lib/runtime-recovery.js	Removed in-file helper implementations; now a thin re-export: module.exports = require("../../dist/lib/runtime-recovery").
Tests src/lib/runtime-recovery.test.ts	Updated imports to ../../dist/lib/runtime-recovery; added tests for parseLiveSandboxNames with error/empty/undefined inputs; extended classifyGatewayStatus assertions and simplified some test input formatting.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐇 I hopped through files, from bin to src I went,
I parsed the names and judged what each line meant,
If gateways sleep or sandboxes hide,
I'll nudge onboard commands back to stride,
A little rabbit's cheer for code well spent.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.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 runtime-recovery.js to TypeScript' accurately describes the primary change: converting a JavaScript implementation to TypeScript with a new typed interface and structured functions.

_{✏️ 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-runtime-recovery-ts

---

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

[coderabbitai]

Actionable comments posted: 3

🤖 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/runtime-recovery.js`:
- Line 7: The bin shims (e.g., bin/lib/runtime-recovery.js,
bin/lib/preflight.js, bin/lib/onboard.js) currently require compiled modules
from dist/lib/ which are missing on fresh checkouts; fix by converting these
shims back to true CommonJS sources that directly export the runtime logic
instead of proxying to dist—restore or reimplement the CommonJS exports for
runtime-recovery, preflight and onboard (and the dependent modules
gateway-state, validation, url-utils, build-context, dashboard) inside bin/lib
so module.exports contains the implementation (or requires the original source
CommonJS files) and no longer points at ../../dist; alternatively, if you prefer
keeping compiled artifacts, update package.json to include dist/ in "files" and
add the build:cli step to the prepare hook so dist gets produced on
install—choose the CommonJS conversion (preferred) and ensure the exported
identifiers and require paths used by bin/* match the original API.

In `@src/lib/runtime-recovery.test.ts`:
- Around line 6-12: The test imports runtime helpers (classifyGatewayStatus,
classifySandboxLookup, parseLiveSandboxNames, shouldAttemptGatewayRecovery) from
the compiled dist which can be missing locally; either update the test imports
to point at the TypeScript source module (change the import path to the src/lib
runtime-recovery module) so tests run against source, or add a pretest npm hook
to package.json (e.g. "pretest": "npm run build:cli") to ensure
dist/lib/runtime-recovery is built before running tests.

In `@src/lib/runtime-recovery.ts`:
- Around line 64-66: The condition in the runtime-recovery check uses
/Connected/i which also matches "Disconnected"; update the check to only match
the standalone word "Connected" (or at start of string) so "Disconnected" won't
match: change the regex in the if that tests the variable clean (currently if
(/Connected/i.test(clean)) ...) to something like /\bConnected\b/i or
/^Connected\b/i so only an exact/whole-word "Connected" (or beginning-with
"Connected") returns { state: "connected", reason: "ok" } in the
runtime-recovery logic.

🪄 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: ae93df98-dcae-4ca9-a429-e62650c60fa4

📥 Commits

Reviewing files that changed from the base of the PR and between 3f923a4 and 1fa2b13.

📒 Files selected for processing (3)

• bin/lib/runtime-recovery.js
• src/lib/runtime-recovery.test.ts
• src/lib/runtime-recovery.ts

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Check whether dist target exists without building =="
if [ -f dist/lib/runtime-recovery.js ]; then
  echo "FOUND: dist/lib/runtime-recovery.js"
else
  echo "MISSING: dist/lib/runtime-recovery.js"
fi

echo
echo "== Find references to runtime-recovery from bin/scripts =="
rg -n -C2 --type=js '\bruntime-recovery\b' bin scripts || true

echo
echo "== Inspect package scripts for enforced build-before-cli/test =="
rg -n '"(build|build:cli|test|test:cli|pretest|prepare|prepack|postinstall)"\s*:' package.json || true

Repository: NVIDIA/NemoClaw

Length of output: 1447

---

🏁 Script executed:

#!/bin/bash
# Get full package.json to see all scripts
cat -n package.json | head -50

echo "=== Check for build:all or other build targets ==="
rg -n '"(build|build:all|compile|prebuild|postinstall)"\s*:' package.json

echo "=== Check if dist/ is version controlled or generated ==="
git ls-files dist/ 2>/dev/null | head -20 || echo "dist/ not in git or git command failed"

echo "=== Check git ignore for dist/ ==="
cat .gitignore | grep -E 'dist|build' || echo "No dist/build in .gitignore"

echo "=== Check tsconfig files ==="
fd -t f 'tsconfig.*\.json$' -x basename {}

Repository: NVIDIA/NemoClaw

Length of output: 2384

---

🏁 Script executed:

#!/bin/bash
# Check the actual source file to understand the pattern
echo "=== Check src/lib/runtime-recovery.ts ==="
if [ -f src/lib/runtime-recovery.ts ]; then
  head -20 src/lib/runtime-recovery.ts
else
  echo "File not found, searching for it:"
  fd 'runtime-recovery' --type f
fi

echo
echo "=== Check other shims in bin/lib for pattern ==="
ls -la bin/lib/ | grep -E '\.js$' || true

echo
echo "=== Check content of other shim files ==="
for shim in bin/lib/*.js; do
  if [ -f "$shim" ]; then
    echo "--- $shim ---"
    head -10 "$shim"
  fi
done

Repository: NVIDIA/NemoClaw

Length of output: 7681

---

🏁 Script executed:

#!/bin/bash
# Verify tsconfig.src.json outputs to dist/
echo "=== tsconfig.src.json ==="
cat tsconfig.src.json

echo
echo "=== All files in bin/lib that require dist ==="
rg -n 'require.*dist' bin/lib/

echo
echo "=== Check if dist/ gets created during dev setup ==="
ls -la dist/ 2>/dev/null | head -5 || echo "dist/ directory does not exist"

Repository: NVIDIA/NemoClaw

Length of output: 1245

---

Multiple critical dependencies on missing dist/ directory — CLI and test flows broken in fresh checkout.

The shim in line 7 (bin/lib/runtime-recovery.js) and related shims (bin/lib/preflight.js, bin/lib/onboard.js) require compiled modules from dist/lib/ that do not exist without an explicit build step. Additionally:

• dist/lib/ is excluded from package.json "files" field (only "nemoclaw/dist/" is included), so npm install will not provide these files
• The prepare hook does not invoke build:cli, leaving no automatic build step
• test and CLI execution will fail with MODULE_NOT_FOUND on fresh checkout or after npm install

Affected imports:

• bin/lib/runtime-recovery.js:7 → dist/lib/runtime-recovery
• bin/lib/preflight.js:7 → dist/lib/preflight
• bin/lib/onboard.js:59-63 → dist/lib/gateway-state, validation, url-utils, build-context, dashboard

Either add dist/ to the "files" field and build:cli to prepare, or convert these modules back to CommonJS sources in bin/lib/ (preferred per guidelines: "bin/lib/**/*.js modules are expected to remain CommonJS").

🤖 Prompt for AI Agents

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

In `@bin/lib/runtime-recovery.js` at line 7, The bin shims (e.g.,
bin/lib/runtime-recovery.js, bin/lib/preflight.js, bin/lib/onboard.js) currently
require compiled modules from dist/lib/ which are missing on fresh checkouts;
fix by converting these shims back to true CommonJS sources that directly export
the runtime logic instead of proxying to dist—restore or reimplement the
CommonJS exports for runtime-recovery, preflight and onboard (and the dependent
modules gateway-state, validation, url-utils, build-context, dashboard) inside
bin/lib so module.exports contains the implementation (or requires the original
source CommonJS files) and no longer points at ../../dist; alternatively, if you
prefer keeping compiled artifacts, update package.json to include dist/ in
"files" and add the build:cli step to the prepare hook so dist gets produced on
install—choose the CommonJS conversion (preferred) and ensure the exported
identifiers and require paths used by bin/* match the original API.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Check test-related scripts for guaranteed build-before-test =="
rg -n '"(build|build:cli|test|test:cli|pretest|prepare)"\s*:' package.json || true

echo
echo "== Check CI workflows for build step before vitest/test commands =="
fd -HI -t f -i 'yml' .github/workflows -x rg -n -C2 '(vitest|npm run test|pnpm test|yarn test|build:cli|npm run build|pnpm build|yarn build)' {}

Repository: NVIDIA/NemoClaw

Length of output: 608

---

🏁 Script executed:

cat -n package.json

Repository: NVIDIA/NemoClaw

Length of output: 2418

---

🏁 Script executed:

find .github/workflows -type f -name "*.yml" -o -name "*.yaml" 2>/dev/null | head -20

Repository: NVIDIA/NemoClaw

Length of output: 481

---

🏁 Script executed:

cat -n src/lib/runtime-recovery.test.ts | head -40

Repository: NVIDIA/NemoClaw

Length of output: 1694

---

🏁 Script executed:

cat -n .github/workflows/pr.yaml | head -80

Repository: NVIDIA/NemoClaw

Length of output: 963

---

🏁 Script executed:

cat -n .github/workflows/main.yaml | head -80

Repository: NVIDIA/NemoClaw

Length of output: 1097

---

🏁 Script executed:

find . -maxdepth 1 -name "vitest.config.*" -o -name "vite.config.*" 2>/dev/null | xargs cat -n 2>/dev/null || echo "No vitest/vite config found"

Repository: NVIDIA/NemoClaw

Length of output: 1326

---

🏁 Script executed:

ls -la dist/lib/runtime-recovery.* 2>/dev/null || echo "dist/lib/runtime-recovery files not found"

Repository: NVIDIA/NemoClaw

Length of output: 100

---

🏁 Script executed:

cat -n .github/actions/basic-checks/action.yaml

Repository: NVIDIA/NemoClaw

Length of output: 1733

---

🏁 Script executed:

find . -name "runtime-recovery.ts" -not -path "*/node_modules/*" 2>/dev/null

Repository: NVIDIA/NemoClaw

Length of output: 88

---

🏁 Script executed:

cat -n src/lib/runtime-recovery.ts | head -50

Repository: NVIDIA/NemoClaw

Length of output: 1996

---

Either import directly from source TypeScript or add a pretest build hook.

The test script in package.json does not include a build step, so running npm test locally without first running npm run build:cli will fail since dist/lib/runtime-recovery.ts is not built. While the CI workflows (pr.yaml, main.yaml) enforce builds before testing via the basic-checks action, local development can run tests against non-existent or stale dist files. Either add a pretest hook ("pretest": "npm run build:cli") to guarantee build ordering, or import directly from the TypeScript source (src/lib/runtime-recovery.ts) for unit tests.

🤖 Prompt for AI Agents

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

In `@src/lib/runtime-recovery.test.ts` around lines 6 - 12, The test imports
runtime helpers (classifyGatewayStatus, classifySandboxLookup,
parseLiveSandboxNames, shouldAttemptGatewayRecovery) from the compiled dist
which can be missing locally; either update the test imports to point at the
TypeScript source module (change the import path to the src/lib runtime-recovery
module) so tests run against source, or add a pretest npm hook to package.json
(e.g. "pretest": "npm run build:cli") to ensure dist/lib/runtime-recovery is
built before running tests.

[coderabbitai]

♻️ Duplicate comments (1)

src/lib/runtime-recovery.ts (1)

64-73: ⚠️ Potential issue | 🟠 Major

Line 64 can still misclassify Not connected as connected.

The current \bConnected\b check matches negative phrases like Status: Not connected. Also, checking unavailable states first is safer when output contains mixed lines.

Proposed fix

 export function classifyGatewayStatus(output = ""): StateClassification {
   const clean = stripAnsi(output).trim();
   if (!clean) {
     return { state: "inactive", reason: "empty" };
   }
-  if (/\bConnected\b/i.test(clean) && !/\bDisconnected\b/i.test(clean)) {
-    return { state: "connected", reason: "ok" };
-  }
   if (
     /No active gateway|transport error|client error|Connection reset by peer|Connection refused|Gateway: .*Error/i.test(
       clean,
     )
   ) {
     return { state: "unavailable", reason: "gateway_unavailable" };
   }
+  if (/^\s*(?:Status:\s*)?Connected\s*$/im.test(clean)) {
+    return { state: "connected", reason: "ok" };
+  }
   return { state: "inactive", reason: "not_connected" };
 }

🤖 Prompt for AI Agents

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

In `@src/lib/runtime-recovery.ts` around lines 64 - 73, Swap the checks so the
unavailable regex block runs before the connected check and tighten the
connected detection to avoid matching negative phrases: in the function that
tests the string variable `clean` (the two regex branches shown), first test the
unavailable pattern as-is, then for the connected case require that `clean`
matches Connected but does NOT match /\bNot\s+connected\b/i (or use a negative
lookbehind /(?<!\bNot\s)\bConnected\b/i if supported), then return { state:
"connected", reason: "ok" } only when that stricter check passes.

🧹 Nitpick comments (2)

src/lib/runtime-recovery.test.ts (1)

70-77: Add a regression assertion for Status: Not connected.

Given the connected-status parsing sensitivity, add an explicit expectation for "Status: Not connected" to remain inactive.

Proposed test addition

   it("classifies gateway status output for restart recovery", () => {
     expect(classifyGatewayStatus("Gateway: nemoclaw\nStatus: Connected").state).toBe("connected");
     expect(classifyGatewayStatus("Error:   × No active gateway").state).toBe("unavailable");
     expect(classifyGatewayStatus("").state).toBe("inactive");
+    expect(classifyGatewayStatus("Gateway: nemoclaw\nStatus: Not connected").state).toBe(
+      "inactive",
+    );
     expect(classifyGatewayStatus("Gateway: nemoclaw\nStatus: Disconnected").state).toBe(
       "inactive",
     );
   });

🤖 Prompt for AI Agents

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

In `@src/lib/runtime-recovery.test.ts` around lines 70 - 77, Add a regression
assertion in the test that ensures classifyGatewayStatus treats "Status: Not
connected" as inactive: update the "classifies gateway status output for restart
recovery" test to include an expect call invoking
classifyGatewayStatus("Gateway: nemoclaw\nStatus: Not connected").state and
assert it toBe("inactive"); this references the classifyGatewayStatus function
in src/lib/runtime-recovery.test.ts used elsewhere in the test block.

src/lib/runtime-recovery.ts (1)

20-23: Tighten StateClassification with literal unions.

Using string for state/reason weakens compile-time guarantees and allows silent typos in classifier returns.

Proposed refactor

+export type RecoveryState = "missing" | "present" | "unavailable" | "inactive" | "connected";
+export type RecoveryReason =
+  | "empty"
+  | "not_found"
+  | "gateway_unavailable"
+  | "ok"
+  | "not_connected";
+
 export interface StateClassification {
-  state: string;
-  reason: string;
+  state: RecoveryState;
+  reason: RecoveryReason;
 }

🤖 Prompt for AI Agents

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

In `@src/lib/runtime-recovery.ts` around lines 20 - 23, The StateClassification
interface currently uses broad string types for state and reason which allows
silent typos; change StateClassification so state and reason are literal union
types (e.g., define a StateClassificationState = 'healthy' | 'degraded' |
'failed' | ... and a StateClassificationReason = 'timeout' | 'validation_error'
| 'resource_exhausted' | ... and use those unions on the state and reason
fields) and update any functions that construct or return StateClassification to
return one of the defined union members (locate usages by the symbol
StateClassification and any classifier functions that produce a
StateClassification) so the compiler will enforce valid values.

🤖 Prompt for all review comments with AI agents

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

Duplicate comments:
In `@src/lib/runtime-recovery.ts`:
- Around line 64-73: Swap the checks so the unavailable regex block runs before
the connected check and tighten the connected detection to avoid matching
negative phrases: in the function that tests the string variable `clean` (the
two regex branches shown), first test the unavailable pattern as-is, then for
the connected case require that `clean` matches Connected but does NOT match
/\bNot\s+connected\b/i (or use a negative lookbehind
/(?<!\bNot\s)\bConnected\b/i if supported), then return { state: "connected",
reason: "ok" } only when that stricter check passes.

---

Nitpick comments:
In `@src/lib/runtime-recovery.test.ts`:
- Around line 70-77: Add a regression assertion in the test that ensures
classifyGatewayStatus treats "Status: Not connected" as inactive: update the
"classifies gateway status output for restart recovery" test to include an
expect call invoking classifyGatewayStatus("Gateway: nemoclaw\nStatus: Not
connected").state and assert it toBe("inactive"); this references the
classifyGatewayStatus function in src/lib/runtime-recovery.test.ts used
elsewhere in the test block.

In `@src/lib/runtime-recovery.ts`:
- Around line 20-23: The StateClassification interface currently uses broad
string types for state and reason which allows silent typos; change
StateClassification so state and reason are literal union types (e.g., define a
StateClassificationState = 'healthy' | 'degraded' | 'failed' | ... and a
StateClassificationReason = 'timeout' | 'validation_error' |
'resource_exhausted' | ... and use those unions on the state and reason fields)
and update any functions that construct or return StateClassification to return
one of the defined union members (locate usages by the symbol
StateClassification and any classifier functions that produce a
StateClassification) so the compiler will enforce valid values.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: d231d260-9ee7-46ff-a91b-1f72eac58e9b

📥 Commits

Reviewing files that changed from the base of the PR and between 1fa2b13 and 354891b.

📒 Files selected for processing (2)

• src/lib/runtime-recovery.test.ts
• src/lib/runtime-recovery.ts

[ericksoa]

Clean migration — faithful to the original with the bonus Disconnected fix.

Pre-existing bug with "Status: Not connected" being misclassified tracked separately in #1279.