fix(cli): handle top-level command failures - #8222
Conversation
Signed-off-by: San Dang <sdang@nvidia.com>
📝 WalkthroughWalkthroughThe CLI now handles rejected ChangesCLI error handling
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit e26a050 in the TypeScript / code-coverage/cliThe overall coverage in commit e26a050 in the Show a code coverage summary of the most impacted files.
Updated |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
bin/nemoclaw.js (1)
14-16: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winVerify and enforce the one-line error contract.
error.messageandString(error)can contain\ror\n. This code passes both values directly tolog.error. Confirm thatlog.errorremoves line breaks. If it does not, normalize the message before logging and add a multiline rejection case.🤖 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 14 - 16, Verify the line-break handling in the mainPromise rejection handler and the log.error implementation. If log.error does not remove carriage returns and newlines, normalize the message derived from error.message or String(error) before logging so the emitted error remains one line, and add a rejection test covering multiline error text.test/package-contract/cli/top-level-error.test.ts (2)
20-25: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the non-
Errorrejection path.The launcher has a separate
String(error)branch, but this test injects onlynew Error("Command failed."). Add a second subprocess case that rejects with a string and verifies status 1 and the expected clean stderr.🤖 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 `@test/package-contract/cli/top-level-error.test.ts` around lines 20 - 25, Extend the top-level CLI error tests around the existing subprocess case to cover a non-Error rejection: inject a string rejection from dispatchCli, then assert the process exits with status 1 and stderr contains the expected clean string error output. Keep the existing Error rejection case unchanged.Source: Path instructions
27-31: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMake logger configuration deterministic.
The child process inherits
NEMOCLAW_LOG_LEVELandNEMOCLAW_DEBUGfromprocess.env. The logger documents both variables as configuration inputs. Set them to known values or remove them before spawning so the exactstderrassertion does not depend on the environment running Vitest.🤖 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 `@test/package-contract/cli/top-level-error.test.ts` around lines 27 - 31, Update the child-process environment in the top-level error test to explicitly set or remove NEMOCLAW_LOG_LEVEL and NEMOCLAW_DEBUG alongside NEMOCLAW_DISABLE_AUTO_DISPATCH. Ensure the spawned logger uses deterministic configuration so the exact stderr assertion is independent of the Vitest environment.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@bin/nemoclaw.js`:
- Around line 14-16: Verify the line-break handling in the mainPromise rejection
handler and the log.error implementation. If log.error does not remove carriage
returns and newlines, normalize the message derived from error.message or
String(error) before logging so the emitted error remains one line, and add a
rejection test covering multiline error text.
In `@test/package-contract/cli/top-level-error.test.ts`:
- Around line 20-25: Extend the top-level CLI error tests around the existing
subprocess case to cover a non-Error rejection: inject a string rejection from
dispatchCli, then assert the process exits with status 1 and stderr contains the
expected clean string error output. Keep the existing Error rejection case
unchanged.
- Around line 27-31: Update the child-process environment in the top-level error
test to explicitly set or remove NEMOCLAW_LOG_LEVEL and NEMOCLAW_DEBUG alongside
NEMOCLAW_DISABLE_AUTO_DISPATCH. Ensure the spawned logger uses deterministic
configuration so the exact stderr assertion is independent of the Vitest
environment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 721e740b-a9e3-4a10-b015-098beb44c3b2
📒 Files selected for processing (2)
bin/nemoclaw.jstest/package-contract/cli/top-level-error.test.ts
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
Second-opinion terminology and E2E selections are advisory. They do not change the primary assessment or E2E / PR Gate. 1 semantic terminology decisionTerminology 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: None This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
wscurran
added
area: cli
cjagwani
left a comment
cjagwani
left a comment
There was a problem hiding this comment.
Reviewed exact head 50eb4425126c047115acc568b5d83f3528682aec against the uncaught-exception portion of #8202 and the current logger implementation.
Changes requested: #8202's observable contract is a clean single-line error, but log.error does not remove carriage returns or newlines. Passing error.message or String(error) directly therefore allows a rejected multiline diagnostic to emit multiple lines. Normalize CR/LF (and repeated whitespace as appropriate) at this executable boundary before logging, and add a package-contract case that rejects with multiline text and proves exactly one redacted error line is emitted.
Please also cover the separate non-Error branch and make the child logger configuration deterministic by clearing or fixing NEMOCLAW_LOG_LEVEL and NEMOCLAW_DEBUG. Those findings were independently raised by both automated reviewers and are valid for this small boundary contract.
All ordinary exact-head code checks are otherwise green. The primary advisor failure preserved only the same non-Error coverage warning and did not report another blocker; do not retry it unchanged merely to clear the red receipt.
cjagwani
left a comment
cjagwani
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 96b7d247e32beeb3c94409194bc7dbfe6b3e268a. The executable boundary now normalizes CR/LF before logging, and the package-contract helper fixes the child logger environment while covering Error, non-Error, multiline, redaction, one-line stderr, exit status, and absence of a Node stack. This closes the requested contract gaps. Exact-head CI and automated review remain required.
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
The advisor warning about rejected values with throwing conversion hooks is addressed in commit |
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
5 participants
Summary
NemoClaw now catches a rejected top-level CLI dispatch. The launcher prints one error line, exits with status 1, and does not expose an uncaught Node.js stack.
Related Issue
Addresses the uncaught-exception cases in #8202.
Changes
mainPromiserejection.Error, non-Error, multiline, and throwing-coercion rejections with deterministic logger configuration.Type of Change
Quality Gates
Proportional security review: PASS at exact head
e26a050dbdccdb88b9c55c91f380ac2ceec66c66against base4f76eea5c3bde2bc35a5c748232025a618aa4d27; no findings across all nine categories. The effective patch remains limited to the launcher and its package-contract regression test. The existing redacting logger remains authoritative, CR and LF are normalized before output, conversion failures use a stable default message, and the package-contract canary verifies credential redaction without stack or path disclosure. Current-main changes do not alter this launcher boundary. No injection, dependency, endpoint-authentication, cryptography, permission, secret-handling, network, or resource-control regression was introduced.Documentation Writer Review
no-docs-needede26a050dbdccdb88b9c55c91f380ac2ceec66c66against base4f76eea5c3bde2bc35a5c748232025a618aa4d27. The change corrects generic top-level failure presentation without changing a documented command, workflow, configuration, recovery action, or supported behavior contract. Nodocs/or Fern files changed. Focused package-contract tests passed 4/4 through all three mainline refreshes,git diff --checkpassed, and normal pre-push CLI TypeScript and version-sync hooks passed. Changed code commentary is concise, and test titles are behavior-oriented with the#8202reference in the final suffix.DGX Station Hardware Evidence
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHub — GitHub reports no unverified commit at exact heade26a050dbdccdb88b9c55c91f380ac2ceec66c66.pre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailable — each refresh merge passed commitlint and the source-age check; the published head passed normal pre-push CLI TypeScript and version-sync checks. The earlier exact-head fallback validation, plugin build, and CLI build passed before the mainline-only refreshes.git diff --checkpassed and the effective patch remains the intended two-file change.npm run docsbuilds without warnings (doc changes only)Signed-off-by: San Dang sdang@nvidia.com
Summary by CodeRabbit
Bug Fixes
Tests