fix(cli): improve failure diagnostics - #8218
Conversation
Signed-off-by: San Dang <sdang@nvidia.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe change improves sandbox creation diagnostics, adds trusted Python prerequisite handling for snapshot sanitization, preserves final failure output, expands diagnostic export, and prevents uncaught CLI promise rejections from printing Node.js stack traces. ChangesFailure handling and diagnostics
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 49d3cd6 in the TypeScript / code-coverage/cliThe overall coverage in commit 49d3cd6 in the Show a code coverage summary of the most impacted files.
Updated |
|
🌿 Preview your docs: https://nvidia-preview-pr-8218.docs.buildwithfern.com/nemoclaw |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/manage-sandboxes/backup-restore.mdx`:
- Line 51: Update the NemoClaw documentation sentence to distinguish Python’s
isolated interpreter execution from the helper’s descriptor-relative filesystem
anchoring. State that python3 -I provides interpreter isolation, while reads,
replacements, and removals remain anchored through opened directory descriptors.
In `@src/lib/onboard/created-sandbox-failure.ts`:
- Line 67: Update the error reporting path to pass a fully redacted version of
createOutput to deps.error() instead of the partially redacted value, while
preserving the existing redaction level for classification and recovery hint
logic. Ensure the redactSandboxCreateFailureOutput function or related redaction
is applied with full redaction for the error output path. Add test coverage that
validates a value matched only by FULL_REDACT_PATTERNS is properly handled
through the error path.
In `@src/nemoclaw.ts`:
- Around line 12-31: Remove reportTopLevelCliError and the automatic dispatch,
environment branching, and process exit handling from src/nemoclaw.ts; keep this
compatibility front controller limited to loading and exposing dispatchCli
through its existing exports. Move the executable-boundary dispatch, error
formatting/logging, and exit-state behavior to the executable launcher instead.
In `@test/package-contract/cli/top-level-error.test.ts`:
- Around line 34-38: The test subprocess inherits the parent's environment
variables, which can include NEMOCLAW_DISABLE_AUTO_DISPATCH set to 1, causing
non-deterministic test behavior. In the spawnSync call options, update the env
property to explicitly set NEMOCLAW_DISABLE_AUTO_DISPATCH to "0" alongside the
existing environment variable spread to ensure the child process always has
automatic dispatch enabled regardless of the parent's configuration.
🪄 Autofix
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: Enterprise
Run ID: dec35bb5-26de-4544-abf9-43f8313ae8fe
📒 Files selected for processing (17)
docs/manage-sandboxes/backup-restore.mdxdocs/reference/troubleshooting.mdxnemoclaw/src/shared/snapshot-sanitizer-boundary.ctsscripts/checks/export-managed-image-failure-diagnostics.tssrc/lib/onboard/created-sandbox-failure.test.tssrc/lib/onboard/created-sandbox-failure.tssrc/lib/onboard/sandbox-create-failure.tssrc/lib/onboard/sandbox-gpu-create-run-attempt.tssrc/lib/sandbox-base-image.test.tssrc/lib/sandbox-base-image.tssrc/lib/security/snapshot-sanitizer.tssrc/lib/state/sandbox-backup-sanitization.test.tssrc/lib/state/sandbox.tssrc/nemoclaw.tstest/managed-image-failure-diagnostics.test.tstest/onboard-sandbox-create-failure.test.tstest/package-contract/cli/top-level-error.test.ts
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
1 terminology difference from the second opinionAdvisory only. These are normalized differences from the primary terminology receipt.
2 additional E2E selections from the second opinionAdvisory only. The primary lane did not select these E2E jobs or targets.
Second-opinion terminology and E2E selections are advisory. They do not change the primary assessment or E2E / PR Gate. 2 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: 2 optional E2E recommendations
1 warning · 0 suggestionsWarningsWarnings do not block.
|
Signed-off-by: San Dang <sdang@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bin/nemoclaw.js`:
- Around line 26-27: Update the executable boundary around the require of
dist/nemoclaw to catch synchronous module-loading failures and route them
through reportTopLevelCliError, while preserving the existing mainPromise
rejection handling. Extend the top-level error coverage in
test/package-contract/cli/top-level-error.test.ts to verify startup failures are
reported through the same handler.
🪄 Autofix
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: Enterprise
Run ID: ffef0615-dbe9-4392-b164-0eb29ea27d33
📒 Files selected for processing (5)
bin/nemoclaw.jsdocs/manage-sandboxes/backup-restore.mdxsrc/lib/onboard/created-sandbox-failure.test.tssrc/lib/onboard/created-sandbox-failure.tstest/package-contract/cli/top-level-error.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/lib/onboard/created-sandbox-failure.test.ts
- src/lib/onboard/created-sandbox-failure.ts
- docs/manage-sandboxes/backup-restore.mdx
| const { mainPromise } = require("../dist/nemoclaw"); | ||
| Promise.resolve(mainPromise).catch(reportTopLevelCliError); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
For Node.js CommonJS entrypoints, does a synchronous require("../dist/nemoclaw") throw before a later Promise.resolve(mainPromise).catch(...) can handle the failure?
💡 Result:
Yes, a synchronous require call will throw an exception immediately if it fails [1][2], and this happens before any later code—including any Promise-based error handling—can execute [3]. Because require is a strictly synchronous operation [1], it operates within the main execution flow of the Node.js event loop [1]. If the module cannot be resolved, fails to load, or encounters an issue such as ERR_REQUIRE_ASYNC_MODULE (when attempting to load an ES module with top-level await) [4][5], require throws an exception immediately [1][2]. Because this throw is synchronous, it is not a Promise rejection [3]. Consequently: 1. A.catch method attached to a promise will not catch it, as.catch only handles rejected promises, not synchronous exceptions [3][6]. 2. If you have code structured like: require("../dist/nemoclaw"); Promise.resolve(mainPromise).catch(...); The require call will throw before the Promise is even created or the.catch handler is attached [3]. Unless the require call is wrapped in a try...catch block, the exception will bubble up as an uncaught exception, which may crash the Node.js process [3][2]. To handle such failures, you must use a standard try...catch block around the require call [3][2].
Citations:
- 1: https://www.thenodebook.com/modules/cjs-require
- 2: https://github.com/nodejs/node/blob/main/doc/api/errors.md
- 3: https://dev.to/shalvah/nodejs-error-handling-gotcha-synchronous-errors-from-an-asynchronous-function-3d8f
- 4: https://nodejs.org/api/modules.html
- 5: https://github.com/nodejs/node/blob/main/doc/api/modules.md
- 6: https://stackoverflow.com/questions/65350744/how-do-i-use-exceptions-and-promise-rejections-together-in-node-js
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- target launcher ---'
cat -n bin/nemoclaw.js
printf '%s\n' '--- error handler and related tests ---'
rg -n -C 5 'reportTopLevelCliError|top-level-error|mainPromise' bin test package.jsonRepository: NVIDIA/NemoClaw
Length of output: 5724
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- top-level error contract test ---'
cat -n test/package-contract/cli/top-level-error.test.ts
printf '%s\n' '--- launcher references and build/package configuration ---'
rg -n -C 4 'bin/nemoclaw|dist/nemoclaw|top-level errors|package-contract' package.json package-lock.json bin test --glob '!**/node_modules/**' || true
printf '%s\n' '--- synchronous CommonJS load probe ---'
node - <<'JS'
try {
require("./__definitely_missing_nemoclaw_entrypoint__");
console.log("unexpected: require returned");
} catch (error) {
console.log(JSON.stringify({
name: error && error.name,
code: error && error.code,
caughtSynchronously: true,
}));
}
let handlerAttached = false;
try {
const value = require("./__definitely_missing_nemoclaw_entrypoint__");
Promise.resolve(value).catch(() => { handlerAttached = true; });
} catch (error) {
console.log(JSON.stringify({
code: error && error.code,
promiseCatchAttached: handlerAttached,
uncaughtPathAvoided: false,
}));
}
JSRepository: NVIDIA/NemoClaw
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- focused probe output ---'
node - <<'JS'
let catchAttached = false;
try {
const value = require("./__definitely_missing_nemoclaw_entrypoint__");
Promise.resolve(value).catch(() => { catchAttached = true; });
} catch (error) {
console.log(JSON.stringify({
name: error.name,
code: error.code,
catchAttached,
reachesFollowingStatement: false,
}));
}
JS
printf '%s\n' '--- tracked build entrypoints ---'
git ls-files 'dist/nemoclaw*' 'src/**' 'bin/nemoclaw.js' | head -80
printf '%s\n' '--- logger implementation references ---'
rg -n -C 3 'export .*log|const log|log\.error' src bin test/package-contract/cli --glob '*.ts' --glob '*.js' --glob '*.cts'Repository: NVIDIA/NemoClaw
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -eu
LOG=/tmp/coderabbit-shell-logs/shell-output-yVCIts
sed -n '1,35p' "$LOG"Repository: NVIDIA/NemoClaw
Length of output: 1373
Catch synchronous startup failures at the executable boundary.
require("../dist/nemoclaw") executes before the promise rejection handler is registered. If the module or a dependency throws, reportTopLevelCliError is bypassed. Wrap the require() in try/catch and add this failure mode to test/package-contract/cli/top-level-error.test.ts.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bin/nemoclaw.js` around lines 26 - 27, Update the executable boundary around
the require of dist/nemoclaw to catch synchronous module-loading failures and
route them through reportTopLevelCliError, while preserving the existing
mainPromise rejection handling. Extend the top-level error coverage in
test/package-contract/cli/top-level-error.test.ts to verify startup failures are
reported through the same handler.
|
Superseded by #8222, which contains only the top-level CLI rejection handler and one focused regression test. |
1 participant
Summary
NemoClaw now catches command failures at the executable boundary and prints the error message without an uncaught Node.js stack trace. Snapshot failures identify the trusted Python 3 prerequisite, while sandbox build diagnostics preserve a redacted final-output tail and the available macOS or Linux gateway evidence.
Related Issue
Fixes #8202
Changes
dispatchCli()contract for in-process callers.sandbox-create-output.log, addcreate_outputandfailure_excerpttosummary.txt, and discover standard Homebrew gateway logs.Type of Change
Quality Gates
Documentation Writer Review
docs-updateddocs/manage-sandboxes/backup-restore.mdxanddocs/reference/troubleshooting.mdx; exact-head review passed with no blocking findings afternpm run docsvalidated 66 guarded routes.DGX Station Hardware Evidence
scripts/prepare-dgx-station-host.shis unchanged.Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailablenpm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result: Not applicable; the change does not modify the test harness or repo-wide validation configuration. The full plugin coverage gate passed (783 tests); the optional local CLI coverage run timed out on unrelated host-sensitive tests and produced no coverage result.npm run docsbuilds without warnings (doc changes only)Signed-off-by: San Dang sdang@nvidia.com
Summary by CodeRabbit
Bug Fixes
Documentation
Tests