Open
Conversation
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Contributor
📝 WalkthroughWalkthroughStrengthens TypeScript typing and runtime validation across scripts and libs: removes suppression directives, converts CommonJS to ESM imports, adds explicit types/return annotations and exported interfaces, introduces parsing/type-guard helpers, tightens error handling, and refactors timer/state control flow. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 (4)
scripts/ts-migration-bulk-fix-prs.ts (2)
282-423: Consider extracting PR processing logic to reduce complexity.The
mainfunction handles many responsibilities: argument parsing, PR iteration, checkout, migration, merge, validation, commit, push, and commenting. While the logic is clear, the function has high cyclomatic complexity (estimated ~18-20 branches) approaching the guideline limit of 20.Consider extracting the per-PR processing loop body (lines 294-412) into a dedicated
processPr(prNumber, options, summary)function for improved readability and testability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/ts-migration-bulk-fix-prs.ts` around lines 282 - 423, Extract the per-PR logic inside main (the loop handling each prNumber) into a new function processPr(prNumber, options, summary) so main only parses args, gets targets via listTargetPrs(), iterates and calls processPr, and finalizes cleanup; move all checks and actions that reference getPrMetadata, touchesLegacyPaths, maintainerCanModify handling, gh checkout/run/npm assist/merge/validateBranch/git add/commit/push and commentOnPr into processPr, return status updates (fixed/skipped/manual) or mutate summary there, and ensure main still performs the final checkout to startingBranch in the finally block and forwards options.base and options.dryRun where needed.
82-96: Consider validating required fields inparsePrMetadata.The
numberfield defaults to0when missing or invalid (line 88). Since PR numbers are always positive, a0value indicates malformed data. Consider throwing an error similar to howparsePrListhandles malformed data, rather than silently returning an invalid metadata object that could cause confusing behavior downstream.💡 Proposed improvement
function parsePrMetadata(value: unknown): PullRequestMetadata { if (!isRecord(value)) { throw new Error("GitHub CLI returned malformed PR metadata."); } + const number = readNumber(value, "number"); + if (number === null || number <= 0) { + throw new Error("GitHub CLI returned PR metadata with invalid number."); + } + return { - number: readNumber(value, "number") ?? 0, + number, title: readString(value, "title"), headRefName: readString(value, "headRefName"), baseRefName: readString(value, "baseRefName"), url: readString(value, "url"), maintainerCanModify: readBoolean(value, "maintainerCanModify"), files: parsePrFiles(value.files), }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/ts-migration-bulk-fix-prs.ts` around lines 82 - 96, The parsePrMetadata function currently defaults number to 0 which can hide malformed input; update parsePrMetadata to validate required fields (at minimum ensure readNumber(value, "number") returns a positive integer) and throw an Error if the number is missing or <= 0 (mirroring parsePrList behavior) instead of using 0; adjust error text to indicate malformed PR metadata and keep the existing parsing of title, headRefName, baseRefName, url, maintainerCanModify, and files (parsePrFiles) intact so callers receive a guaranteed-valid PullRequestMetadata object.scripts/check-legacy-migrated-paths.ts (1)
83-95: Verify the fallback logic for rename/copy entries.For rename (
R) and copy (C) status entries, git outputs the formatR100\toldPath\tnewPath. IfsecondPathis empty for a rename/copy entry, this would indicate malformed git output. The fallbacksecondPath || firstPathwould incorrectly setnewPathto the old path in that scenario.Consider returning
nullfor R/C entries with missingsecondPathrather than silently using the old path as the new path:💡 Proposed stricter handling
const isRenameOrCopy = /^[RC]/.test(status); + if (isRenameOrCopy && !secondPath) { + return null; // Malformed R/C entry + } return { status, oldPath: isRenameOrCopy ? firstPath : null, - newPath: isRenameOrCopy ? secondPath || firstPath : firstPath, + newPath: isRenameOrCopy ? secondPath : firstPath, };🤖 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 83 - 95, parseChangedFileLine currently treats rename/copy entries (status matching /^[RC]/) as valid even when the secondPath is missing by falling back to firstPath; change this to treat such malformed R/C lines as invalid and return null instead. Specifically, in parseChangedFileLine check if isRenameOrCopy and secondPath is empty — if so return null instead of using secondPath || firstPath; otherwise keep the existing behavior (oldPath = firstPath, newPath = secondPath). This ensures we don't silently treat oldPath as newPath for R/C entries and makes the function fail fast on malformed git output.src/lib/config-io.ts (1)
124-126: Consider adding runtime validation for type safety.While this change makes the unsafe cast more explicit (which is good),
parsed as Tstill lacks runtime validation. If the config file contains JSON that doesn't match typeT, the caller will receive incorrectly typed data, potentially causing runtime errors elsewhere.🛡️ Consider adding a validator parameter for runtime safety
-export function readConfigFile<T>(filePath: string, fallback: T): T { +export function readConfigFile<T>( + filePath: string, + fallback: T, + validator?: (data: unknown) => data is T +): T { try { const parsed: unknown = JSON.parse(fs.readFileSync(filePath, "utf-8")); + if (validator && !validator(parsed)) { + return fallback; + } return parsed as T; } catch (error: unknown) {Alternatively, use a schema validation library like
zodfor robust type checking:import { z } from 'zod'; export function readConfigFile<T>( filePath: string, fallback: T, schema: z.ZodSchema<T> ): T { try { const parsed: unknown = JSON.parse(fs.readFileSync(filePath, "utf-8")); return schema.parse(parsed); } catch (error: unknown) { // ...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/config-io.ts` around lines 124 - 126, The readConfigFile function currently returns parsed as T without runtime checks; update its signature (readConfigFile) to accept a validator (either a predicate function or a zod/schema object) and use that validator to validate/parse the unknown JSON before returning, e.g., call schema.parse(parsed) or validator(parsed) and throw or fallback when validation fails; ensure the catch block and return path use the validated value (not the raw parsed) and preserve existing fallback behavior so callers receive a runtime-checked T.
🤖 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/config-io.ts`:
- Line 13: The current assignment to the local variable `home` uses the nullish
coalescing operator and will not fall back when `process.env.HOME` is an empty
string; change the expression that initializes `home` (currently `const home =
process.env.HOME ?? os.homedir();`) to use a logical OR so an empty string falls
back to `os.homedir()`, matching the handling used in `src/lib/credentials.ts`.
---
Nitpick comments:
In `@scripts/check-legacy-migrated-paths.ts`:
- Around line 83-95: parseChangedFileLine currently treats rename/copy entries
(status matching /^[RC]/) as valid even when the secondPath is missing by
falling back to firstPath; change this to treat such malformed R/C lines as
invalid and return null instead. Specifically, in parseChangedFileLine check if
isRenameOrCopy and secondPath is empty — if so return null instead of using
secondPath || firstPath; otherwise keep the existing behavior (oldPath =
firstPath, newPath = secondPath). This ensures we don't silently treat oldPath
as newPath for R/C entries and makes the function fail fast on malformed git
output.
In `@scripts/ts-migration-bulk-fix-prs.ts`:
- Around line 282-423: Extract the per-PR logic inside main (the loop handling
each prNumber) into a new function processPr(prNumber, options, summary) so main
only parses args, gets targets via listTargetPrs(), iterates and calls
processPr, and finalizes cleanup; move all checks and actions that reference
getPrMetadata, touchesLegacyPaths, maintainerCanModify handling, gh
checkout/run/npm assist/merge/validateBranch/git add/commit/push and commentOnPr
into processPr, return status updates (fixed/skipped/manual) or mutate summary
there, and ensure main still performs the final checkout to startingBranch in
the finally block and forwards options.base and options.dryRun where needed.
- Around line 82-96: The parsePrMetadata function currently defaults number to 0
which can hide malformed input; update parsePrMetadata to validate required
fields (at minimum ensure readNumber(value, "number") returns a positive
integer) and throw an Error if the number is missing or <= 0 (mirroring
parsePrList behavior) instead of using 0; adjust error text to indicate
malformed PR metadata and keep the existing parsing of title, headRefName,
baseRefName, url, maintainerCanModify, and files (parsePrFiles) intact so
callers receive a guaranteed-valid PullRequestMetadata object.
In `@src/lib/config-io.ts`:
- Around line 124-126: The readConfigFile function currently returns parsed as T
without runtime checks; update its signature (readConfigFile) to accept a
validator (either a predicate function or a zod/schema object) and use that
validator to validate/parse the unknown JSON before returning, e.g., call
schema.parse(parsed) or validator(parsed) and throw or fallback when validation
fails; ensure the catch block and return path use the validated value (not the
raw parsed) and preserve existing fallback behavior so callers receive a
runtime-checked T.
🪄 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 Plus
Run ID: 5f757ef4-9be3-4d6d-8a8e-b50d83c9c935
📒 Files selected for processing (6)
scripts/check-legacy-migrated-paths.tsscripts/ts-migration-bulk-fix-prs.tssrc/lib/config-io.tssrc/lib/nim.test.tssrc/lib/sandbox-build-context.tssrc/lib/shields-timer.ts
|
|
||
| function buildRemediation(): string { | ||
| const home = process.env.HOME || os.homedir(); | ||
| const home = process.env.HOME ?? os.homedir(); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check for HOME environment variable handling patterns
# Search for other HOME handling patterns
rg -n --type=ts -C3 'process\.env\.HOME' -g '!node_modules'
# Check if empty string HOME is explicitly handled anywhere
rg -n --type=ts 'HOME.*==.*""' -g '!node_modules'Repository: NVIDIA/NemoClaw
Length of output: 34338
Use || instead of ?? to handle empty process.env.HOME.
The nullish coalescing operator ?? only handles null or undefined, not empty strings. If process.env.HOME is set to an empty string, the code will use the empty string rather than falling back to os.homedir(). This contradicts the PR summary's intent to "handle process.env.HOME being an empty string."
For consistency with similar handling elsewhere in the codebase (e.g., src/lib/credentials.ts), use the logical OR operator || instead:
const home = process.env.HOME || os.homedir();
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/config-io.ts` at line 13, The current assignment to the local
variable `home` uses the nullish coalescing operator and will not fall back when
`process.env.HOME` is an empty string; change the expression that initializes
`home` (currently `const home = process.env.HOME ?? os.homedir();`) to use a
logical OR so an empty string falls back to `os.homedir()`, matching the
handling used in `src/lib/credentials.ts`.
This was referenced Apr 20, 2026
wscurran
added
refactor
ericksoa
added a commit
that referenced
this pull request
Apr 21, 2026
…erations (#2117) ## Summary Adds a shared `sandbox-session-state.ts` module that detects active SSH connections to OpenShell sandboxes before destructive operations proceed. This addresses the core issue in #992: no destructive operation in the CLI checks for active SSH sessions before proceeding, leading to abrupt Broken pipe errors with no graceful warning. ## Architectural Context This is part of the broader onboard.ts decomposition effort and JS→TS migration arc (see #2113 for the type-safety direction). The fix creates reusable infrastructure that improves **destroy, rebuild, status, list, and connect**. The module follows the `gateway-state.ts` pattern: pure typed classifiers that parse CLI output are separated from the I/O layer with dependency injection for testability. ## Changes - **New module: `src/lib/sandbox-session-state.ts`** — ESM TypeScript, no `@ts-nocheck`, explicit types, dependency-injected I/O - `parseForwardList()` — parses `openshell forward list` output - `parseSshProcesses()` — parses `pgrep -a ssh` output - `classifySessionState()` — combines both sources - `getActiveSandboxSessions()` — high-level entry point - `createSystemDeps()` — factory for real system deps - **`src/nemoclaw.ts`** — wires session detection into 6 consumers: - `sandboxDestroy()`:⚠️ "Active SSH session detected. Destroy anyway?" - `sandboxRebuild()`:⚠️ Same warning pattern - `cleanupGatewayAfterLastSandbox()`: Note about active port forwards - `sandboxConnect()`: "Note: existing session detected (another terminal)" - `sandboxStatus()`: Shows "Connected: yes (N sessions)" / "Connected: no" - `nemoclaw list`: ● indicator next to connected sandboxes - **`src/lib/inventory-commands.ts`** — adds optional `getActiveSessionCount` to `ListSandboxesCommandDeps` ## Type of Change - [x] Code change (feature, bug fix, or refactor) ## Test plan - [x] 27 unit tests in `src/lib/sandbox-session-state.test.ts` covering all pure classifiers and integration function - [x] `npx vitest run --project cli` passes (existing tests unaffected) - [x] `npx tsc -p tsconfig.src.json --noEmit` clean - [x] All guards wrapped in try/catch — if detection fails, commands behave identically to before (zero regression risk) ## Verification - [x] `npm test` passes (pre-existing failure in install-preflight.test.ts unrelated) - [x] No secrets, API keys, or credentials committed - [ ] Tests added or updated for new or changed behavior ✅ (27 new tests) ## AI Disclosure - [x] AI-assisted — tool: pi coding agent Closes #992 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Sandbox list shows a connection indicator when active SSH sessions are detected. * Status command reports active session counts per sandbox. * Connect alerts if a sandbox already has active sessions. * Destroy and rebuild prompt warn about active sessions and require confirmation when present. * **Tests** * Added comprehensive tests for session detection, parsing, and classification. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Test User <test@example.com> Co-authored-by: Aaron Erickson 🦞 <aerickson@nvidia.com>
13 tasks
…s-quick-wins # Conflicts: # src/lib/shields-timer.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/shields-timer.ts (1)
88-96: Consider ensuring STATE_DIR exists before writing.Unlike the pattern in
shields.ts(lines 92-93), this version ofupdateStatedoesn't callfs.mkdirSync(STATE_DIR, { recursive: true })before writing. While the directory should exist (created when shields were lowered), adding the defensive creation would improve robustness if the directory is removed between timer spawn and restore.♻️ Optional: defensive directory creation
function updateState(stateFile: string, patch: ShieldsStatePatch): void { try { const current = readStateFile(stateFile); const updated = { ...current, ...patch, updatedAt: new Date().toISOString() }; + fs.mkdirSync(path.dirname(stateFile), { recursive: true, mode: 0o700 }); fs.writeFileSync(stateFile, JSON.stringify(updated, null, 2), { mode: 0o600 }); } catch { // Best effort } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/shields-timer.ts` around lines 88 - 96, The updateState function should defensively ensure the state directory exists before writing: inside updateState (function name) call fs.mkdirSync(STATE_DIR, { recursive: true }) prior to fs.writeFileSync so the write won't fail if the directory was removed; keep the existing try/catch behavior and make sure you reference the same STATE_DIR symbol used in shields.ts to create the directory before serializing the updated object (ShieldsStatePatch and readStateFile remain unchanged).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/lib/shields-timer.ts`:
- Around line 88-96: The updateState function should defensively ensure the
state directory exists before writing: inside updateState (function name) call
fs.mkdirSync(STATE_DIR, { recursive: true }) prior to fs.writeFileSync so the
write won't fail if the directory was removed; keep the existing try/catch
behavior and make sure you reference the same STATE_DIR symbol used in
shields.ts to create the directory before serializing the updated object
(ShieldsStatePatch and readStateFile remain unchanged).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8ac237e4-8784-4a0b-a71d-0a4b3953a1e6
📒 Files selected for processing (2)
src/lib/sandbox-build-context.tssrc/lib/shields-timer.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/sandbox-build-context.ts
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
This is the first of two follow-up typing PRs driven by the new hotspot finder. It focuses on lower-risk helpers and maintainer scripts, removing several
@ts-nocheckislands and tightening parsing/type boundaries without changing user-facing behavior.Changes
@ts-nocheckand add explicit types insrc/lib/sandbox-build-context.tsandsrc/lib/shields-timer.tsscripts/ts-migration-bulk-fix-prs.tsandscripts/check-legacy-migrated-paths.tswith structured helpers and typed result shapessrc/lib/config-io.tsto reduce unsafe casts while preserving the existing APIsrc/lib/nim.test.tsto use a namespace import sotsconfig.cli.jsonstays green after the newerdist/lib/nimtype surface landed onmain95/74/70/57/47to0/8/5/0/29forsandbox-build-context,shields-timer,ts-migration-bulk-fix-prs,check-legacy-migrated-paths, andconfig-ioType of Change
Verification
npx prek run --all-filespassesnpm testpassesmake docsbuilds without warnings (doc changes only)AI Disclosure
Signed-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit