Merged
Conversation
Replace `spawnSync("bash", ["scripts/debug.sh", ...])` with a direct
call to `src/lib/debug.ts` compiled module, following the established
migration pattern from #924.
- Add `src/lib/debug.ts` with `runDebug()` entry point, `redact()`
secret filtering, platform-aware diagnostics (macOS / Linux), and
tarball creation using execa for subprocess execution
- Add `src/lib/debug.test.ts` testing all redaction patterns
- Add `bin/lib/debug.js` thin CJS re-export shim
- Update `bin/nemoclaw.js` debug() to parse --quick, --output,
--sandbox, --help flags and call runDebug() directly
- Preserve `scripts/debug.sh` for standalone curl-pipe usage
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)
📝 WalkthroughWalkthroughPorts the diagnostic collection functionality from Changes
Sequence DiagramsequenceDiagram
participant User as User / CLI
participant Nemoclaw as bin/nemoclaw.js
participant Debug as src/lib/debug.ts
participant SubProc as Subprocess Execution
participant FS as Filesystem
User->>Nemoclaw: nemoclaw debug [options]
Nemoclaw->>Nemoclaw: Parse CLI options (--sandbox, --output, --quick)
Nemoclaw->>Debug: runDebug(opts)
Debug->>Debug: Resolve target sandbox name
Debug->>FS: Create temp collection directory
Debug->>SubProc: Execute multiple diagnostic commands<br/>(OS info, Docker, GPU, OpenShell, etc.)
SubProc-->>Debug: Capture stdout/stderr (30s timeout)
Debug->>Debug: Apply secret redaction patterns
Debug->>FS: Write labeled .txt files to temp dir
Debug->>User: Print colorized output to console
alt opts.output provided
Debug->>FS: Create gzipped tarball
Debug->>User: Print archive path & usage instructions
end
Debug->>FS: Clean up temp directory & files
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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: 4
🧹 Nitpick comments (1)
nemoclaw/src/lib/debug.test.ts (1)
5-5: Import the co-located source module here.Pointing this test at
../../dist/lib/debug.jsmakes the suite depend on a built artifact and can hide a source/dist mismatch. If this is meant to be a unit test for the new TypeScript implementation, import the local source module instead; if it is meant to smoke-test the packaged output, it should live with the integration/build checks instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nemoclaw/src/lib/debug.test.ts` at line 5, The test currently imports the built artifact (dist) which creates a dependency on a packaged output; update the import in the test so it imports the co-located source TypeScript module that exports redact (the repo's debug module) instead of ../../dist/lib/debug.js so the unit test exercises the source implementation rather than the built artifact.
🤖 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/nemoclaw.js`:
- Around line 241-247: The CLI argument parsing block that sets opts.output and
opts.sandboxName accepts any next token (args[++i]) including flag-shaped values
like "--quick"; update the parsing for the branches handling "--output"/"-o" and
"--sandbox" to validate the next argument is not another flag (e.g., starts with
'-') and exists, and if it is missing or flag-shaped emit the same error path
(console.error + process.exit(1)) instead of assigning it to opts.output or
opts.sandboxName; locate the code that checks args[i] and assigns to opts.output
and opts.sandboxName and add a guard rejecting values that begin with '-' so
typos don’t become valid names.
In `@nemoclaw/src/lib/debug.ts`:
- Around line 250-254: The execa(...) calls used for probing Docker/OpenShell
can hang and bypass collect()'s timeout; update the ad-hoc probes (the execa
invocation shown and the similar call at the 313-317 block) to include a timeout
option (e.g., timeout: <milliseconds>) and propagate the same timeout value used
by collect() (or introduce a shared timeoutMs constant) so the probe is killed
after the collection timeout; ensure the option is passed alongside
reject/stdout/stderr and handle the timeout result consistently (reject: false)
to avoid throwing on timeout.
- Around line 324-333: The code currently builds sshHost and sshBase (variables
sshHost and sshBase) and then rebuilds them into a shell string (via
collectShell/round-tripping), which allows a crafted sandboxName to break
quoting; instead keep sshBase as an argv array and stop converting it to a
single shell string: pass the sshBase array (with sshHost) directly to the child
process/spawn API or any function that accepts argv, and update the places that
previously used collectShell()/stringified argv (also the similar block around
lines 338-342) to accept an array instead of concatenating into a shell command
so sandboxName is never interpolated into sh -c.
- Around line 50-52: Update the GitHub PAT redact regex in
nemoclaw/src/lib/debug.ts to also match fine-grained tokens prefixed with
"github_pat_" (in addition to "ghp_") by extending the existing
[/ghp_[A-Za-z0-9]{30,}/g, "<REDACTED>"] entry (or replacing it with a single
regex that matches both prefixes), and add a unit test that verifies tokens
starting with "github_pat_" are redacted the same way as "ghp_" tokens.
---
Nitpick comments:
In `@nemoclaw/src/lib/debug.test.ts`:
- Line 5: The test currently imports the built artifact (dist) which creates a
dependency on a packaged output; update the import in the test so it imports the
co-located source TypeScript module that exports redact (the repo's debug
module) instead of ../../dist/lib/debug.js so the unit test exercises the source
implementation rather than the built artifact.
🪄 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: 685019af-01b4-4196-8d9e-827712605a49
📒 Files selected for processing (4)
bin/lib/debug.jsbin/nemoclaw.jsnemoclaw/src/lib/debug.test.tsnemoclaw/src/lib/debug.ts
Comment on lines
+50
to
+52
| [/nvapi-[A-Za-z0-9_-]{10,}/g, "<REDACTED>"], | ||
| [/ghp_[A-Za-z0-9]{30,}/g, "<REDACTED>"], | ||
| [/(Bearer )\S+/gi, "$1<REDACTED>"], |
Contributor
There was a problem hiding this comment.
Redact fine-grained GitHub PATs too.
ghp_ only covers classic personal access tokens. GitHub also issues fine-grained PATs with the github_pat_ prefix, so those can still be written to disk and echoed back unredacted by this collector. Please extend the pattern and add a matching test case. (docs.github.com)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@nemoclaw/src/lib/debug.ts` around lines 50 - 52, Update the GitHub PAT redact
regex in nemoclaw/src/lib/debug.ts to also match fine-grained tokens prefixed
with "github_pat_" (in addition to "ghp_") by extending the existing
[/ghp_[A-Za-z0-9]{30,}/g, "<REDACTED>"] entry (or replacing it with a single
regex that matches both prefixes), and add a unit test that verifies tokens
starting with "github_pat_" are redacted the same way as "ghp_" tokens.
cv
requested changes
Apr 2, 2026
Contributor
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
Nice work porting the debug collector. A few issues to fix, one of which is a security concern:
1. Wrong directory — same as #1306 and #1307. nemoclaw/src/lib/debug.ts should be src/lib/debug.ts.
2. Shell injection via sandbox name (critical) — sandboxName is interpolated into a sh -c command string for the SSH diagnostic collection. A sandbox name containing shell metacharacters (quotes, semicolons, backticks) can execute arbitrary commands locally. Use execa array args instead of string interpolation:
// Instead of:
await execa("sh", ["-c", `ssh ${sshBase} "command"`])
// Use:
await execa("ssh", [...sshArgs, "command"])3. Missing redaction for fine-grained GitHub PATs — the ghp_ pattern only catches classic tokens. GitHub also issues github_pat_ prefixed tokens. Add that pattern.
4. No timeouts on ad-hoc execa probes — the Docker and OpenShell health checks outside of collect() have no timeout. If Docker is wedged, nemoclaw debug hangs forever. Add { timeout: 10_000 } or similar.
5. Flag-shaped values accepted for --output and --sandbox — nemoclaw debug --output --quick writes to a file literally named --quick. Reject values starting with - for these flags, or use a flag parsing library that handles this.
6. Needs rebase — merge conflicts with main.
The overall structure (platform-aware collectors, secret redaction, tarball output) is solid.
Replace `spawnSync("bash", ["scripts/debug.sh", ...])` with a direct
call to `src/lib/debug.ts` compiled module, following the established
migration pattern from #924.
- Add `src/lib/debug.ts` with `runDebug()` entry point, `redact()`
secret filtering, platform-aware diagnostics (macOS / Linux), and
tarball creation using execa for subprocess execution
- Add `src/lib/debug.test.ts` testing all redaction patterns
- Add `bin/lib/debug.js` thin CJS re-export shim
- Update `bin/nemoclaw.js` debug() to parse --quick, --output,
--sandbox, --help flags and call runDebug() directly
- Preserve `scripts/debug.sh` for standalone curl-pipe usage
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…redaction Address review feedback: - Move TS source from nemoclaw/src/lib/ to src/lib/ (correct directory pattern) - Replace execa with child_process (execFileSync/spawnSync) for CJS compat - Fix shell injection: use collect() with array args for SSH sandbox commands instead of collectShell() with string interpolation of sandboxName - Add github_pat_ fine-grained token redaction pattern - Add timeouts (30s) to all ad-hoc Docker/OpenShell probes - Reject flag-shaped values for --output/--sandbox in argument parsing - Add Onboard Session diagnostic section - Update bin/lib/debug.js shim to point to dist/lib/debug - Make all functions synchronous (no more async/execa dependency) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…moClaw into refactor/port-debug-to-ts-1296 # Conflicts: # bin/lib/debug.js # bin/nemoclaw.js
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/debug.ts (1)
70-77: Consider usingexecFileSyncfor command existence check.
command -v ${cmd}interpolates into a shell string. While all current callers pass hardcoded command names, usingexecFileSyncwithwhich(or spawning a shell with array args) would be safer for defense in depth.♻️ Optional: Safer implementation
function commandExists(cmd: string): boolean { try { - execSync(`command -v ${cmd}`, { stdio: ["ignore", "ignore", "ignore"] }); + spawnSync("sh", ["-c", `command -v "$1"`, "_", cmd], { + stdio: "ignore", + }).status === 0; return true; } catch { return false; } }Or simply document that this function expects only trusted/hardcoded command names.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/debug.ts` around lines 70 - 77, The commandExists function currently calls execSync with an interpolated shell string; replace it to avoid shell interpolation by using execFileSync (or spawnSync) to run the platform command directly (e.g., call execFileSync or spawnSync with the executable name "which" and an args array containing cmd) and check exit status/throwing behavior to return true/false; update the function named commandExists accordingly and ensure stdio/options mirror previous behavior for silent execution.
🤖 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/debug.ts`:
- Around line 70-77: The commandExists function currently calls execSync with an
interpolated shell string; replace it to avoid shell interpolation by using
execFileSync (or spawnSync) to run the platform command directly (e.g., call
execFileSync or spawnSync with the executable name "which" and an args array
containing cmd) and check exit status/throwing behavior to return true/false;
update the function named commandExists accordingly and ensure stdio/options
mirror previous behavior for silent execution.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 18349c7e-7076-49c4-9d74-1af7fa1a41b8
📒 Files selected for processing (6)
bin/lib/debug.jsbin/nemoclaw.jsinstall.shsrc/lib/debug.test.tssrc/lib/debug.tstest/install-preflight.test.js
✅ Files skipped from review due to trivial changes (1)
- bin/lib/debug.js
🚧 Files skipped from review as they are similar to previous changes (1)
- bin/nemoclaw.js
Replace `execSync(`command -v ${cmd}`)` with `execFileSync("sh", ["-c",
'command -v "$1"', "--", cmd])` to avoid shell injection. Also remove the
now-unused `execSync` import.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/lib/debug.ts (2)
105-107: Consider distinguishing timeouts from other failures.When
spawnSynctimes out,result.signalwill be'SIGTERM'but the current message says "non-zero status". For better diagnostics, consider checkingresult.signalto display a more informative message like "(command timed out after 30s)".💡 Optional enhancement
if (result.status !== 0) { - console.log(" (command exited with non-zero status)"); + if (result.signal === "SIGTERM") { + console.log(` (command timed out after ${TIMEOUT_MS / 1000}s)`); + } else { + console.log(` (command exited with status ${result.status ?? "unknown"})`); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/debug.ts` around lines 105 - 107, The current console message for non-zero exits uses result.status only; update the check around the spawnSync result (variable name result) to detect timeouts by inspecting result.signal (e.g., === 'SIGTERM') and print a timeout-specific message like "(command timed out after 30s)" instead of the generic "(command exited with non-zero status)"; keep the original non-zero/exit case for other failures and include the status or signal in the log for clearer diagnostics.
306-306: Minor: Consider usingmkdtempSyncfor the SSH config.The timestamp-based filename is predictable, which creates a small TOCTOU window. For hardened environments, consider creating a temp directory with
mkdtempSyncand placing the config inside, or usingcrypto.randomUUID()for the filename suffix.💡 Optional hardening
- const sshConfigPath = join(tmpdir(), `nemoclaw-ssh-${String(Date.now())}`); + const sshConfigPath = join(tmpdir(), `nemoclaw-ssh-${crypto.randomUUID()}`);Add to imports:
import { randomUUID } from "node:crypto";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/debug.ts` at line 306, The sshConfigPath currently uses a predictable timestamp-based name; update the code that defines sshConfigPath (the variable assigned with join(tmpdir(), `nemoclaw-ssh-${String(Date.now())}`)) to create a secure temp location instead—either create a unique temp directory with fs.mkdtempSync and place the SSH config file inside it, or append a cryptographically strong suffix such as crypto.randomUUID() to the filename (add the import for randomUUID from node:crypto if chosen); ensure any downstream uses of sshConfigPath still point to the new file path.
🤖 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/debug.ts`:
- Around line 105-107: The current console message for non-zero exits uses
result.status only; update the check around the spawnSync result (variable name
result) to detect timeouts by inspecting result.signal (e.g., === 'SIGTERM') and
print a timeout-specific message like "(command timed out after 30s)" instead of
the generic "(command exited with non-zero status)"; keep the original
non-zero/exit case for other failures and include the status or signal in the
log for clearer diagnostics.
- Line 306: The sshConfigPath currently uses a predictable timestamp-based name;
update the code that defines sshConfigPath (the variable assigned with
join(tmpdir(), `nemoclaw-ssh-${String(Date.now())}`)) to create a secure temp
location instead—either create a unique temp directory with fs.mkdtempSync and
place the SSH config file inside it, or append a cryptographically strong suffix
such as crypto.randomUUID() to the filename (add the import for randomUUID from
node:crypto if chosen); ensure any downstream uses of sshConfigPath still point
to the new file path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f233e8ab-307c-4a07-936c-5b469d12a5ff
📒 Files selected for processing (1)
src/lib/debug.ts
cv
approved these changes
Apr 2, 2026
Contributor
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
Shell injection fixed (execa array args), github_pat_ redaction added, timeouts added, directory structure corrected. Remaining CodeRabbit comments are nitpicks. Good work.
cjagwani
pushed a commit
to fdzdev/NemoClaw
that referenced
this pull request
Apr 3, 2026
## Summary Closes NVIDIA#1296. Part of NVIDIA#924 shell consolidation. - Port `scripts/debug.sh` (355 lines) to `src/lib/debug.ts` using execa for subprocess execution - Add `redact()` function with 4 regex patterns for secret filtering (API keys, nvapi tokens, GitHub PATs, Bearer tokens) - Platform-aware diagnostics: System, Processes, GPU, Docker, OpenShell, Sandbox Internals (SSH), Network, Kernel/IO, Kernel Messages - Replace `bin/nemoclaw.js` `spawnSync("bash", ["scripts/debug.sh", ...])` with direct `runDebug()` call - Parse `--quick`, `--output`, `--sandbox`, `--help` flags in CLI with unknown-flag rejection - Add `bin/lib/debug.js` thin CJS re-export shim following established migration pattern - Add `src/lib/debug.test.ts` with 7 tests covering all redaction patterns - Preserve `scripts/debug.sh` for standalone curl-pipe usage ## Test plan - [x] `npx vitest run --project plugin` — all 200 tests pass (9 files) - [x] `npx tsc --noEmit` — no type errors - [x] `npx eslint src/lib/debug.ts src/lib/debug.test.ts` — clean - [x] Pre-commit hooks pass (gitleaks, prettier, eslint, vitest, commitlint) - [ ] Manual: `nemoclaw debug --quick` collects minimal diagnostics - [ ] Manual: `nemoclaw debug --output /tmp/diag.tar.gz` creates tarball - [ ] Manual: `nemoclaw debug --help` shows usage text 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes * **New Features** * Enhanced the debug command with CLI options: `--help`, `--quick`, `--output`, and `--sandbox` for customized diagnostic collection. * Debug command now automatically redacts sensitive information (API keys, tokens, passwords) from diagnostic output. * Added support for exporting collected diagnostics as a gzipped archive. * **Tests** * Added comprehensive test coverage for secret redaction functionality. <!-- 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#1296. Part of NVIDIA#924 shell consolidation. - Port `scripts/debug.sh` (355 lines) to `src/lib/debug.ts` using execa for subprocess execution - Add `redact()` function with 4 regex patterns for secret filtering (API keys, nvapi tokens, GitHub PATs, Bearer tokens) - Platform-aware diagnostics: System, Processes, GPU, Docker, OpenShell, Sandbox Internals (SSH), Network, Kernel/IO, Kernel Messages - Replace `bin/nemoclaw.js` `spawnSync("bash", ["scripts/debug.sh", ...])` with direct `runDebug()` call - Parse `--quick`, `--output`, `--sandbox`, `--help` flags in CLI with unknown-flag rejection - Add `bin/lib/debug.js` thin CJS re-export shim following established migration pattern - Add `src/lib/debug.test.ts` with 7 tests covering all redaction patterns - Preserve `scripts/debug.sh` for standalone curl-pipe usage ## Test plan - [x] `npx vitest run --project plugin` — all 200 tests pass (9 files) - [x] `npx tsc --noEmit` — no type errors - [x] `npx eslint src/lib/debug.ts src/lib/debug.test.ts` — clean - [x] Pre-commit hooks pass (gitleaks, prettier, eslint, vitest, commitlint) - [ ] Manual: `nemoclaw debug --quick` collects minimal diagnostics - [ ] Manual: `nemoclaw debug --output /tmp/diag.tar.gz` creates tarball - [ ] Manual: `nemoclaw debug --help` shows usage text 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes * **New Features** * Enhanced the debug command with CLI options: `--help`, `--quick`, `--output`, and `--sandbox` for customized diagnostic collection. * Debug command now automatically redacts sensitive information (API keys, tokens, passwords) from diagnostic output. * Added support for exporting collected diagnostics as a gzipped archive. * **Tests** * Added comprehensive test coverage for secret redaction functionality. <!-- 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 #1296. Part of #924 shell consolidation.
scripts/debug.sh(355 lines) tosrc/lib/debug.tsusing execa for subprocess executionredact()function with 4 regex patterns for secret filtering (API keys, nvapi tokens, GitHub PATs, Bearer tokens)bin/nemoclaw.jsspawnSync("bash", ["scripts/debug.sh", ...])with directrunDebug()call--quick,--output,--sandbox,--helpflags in CLI with unknown-flag rejectionbin/lib/debug.jsthin CJS re-export shim following established migration patternsrc/lib/debug.test.tswith 7 tests covering all redaction patternsscripts/debug.shfor standalone curl-pipe usageTest plan
npx vitest run --project plugin— all 200 tests pass (9 files)npx tsc --noEmit— no type errorsnpx eslint src/lib/debug.ts src/lib/debug.test.ts— cleannemoclaw debug --quickcollects minimal diagnosticsnemoclaw debug --output /tmp/diag.tar.gzcreates tarballnemoclaw debug --helpshows usage text🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
--help,--quick,--output, and--sandboxfor customized diagnostic collection.Tests