fix(cli): catch launcher initialization errors - #8547
Conversation
Signed-off-by: danielpolimac <danielpolimac@gmail.com>
📝 WalkthroughWalkthroughThe CLI launcher now handles synchronous module-load failures and rejected startup promises through shared top-level error handling. It sanitizes messages, uses logger or stderr fallback reporting, preserves exit codes, and adds coverage for logger failures and credential redaction. ChangesCLI error handling
Estimated code review effort: 3 (Moderate) | ~15–30 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
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 `@test/package-contract/cli/top-level-error.test.ts`:
- Around line 61-65: Update the stderr assertions in the top-level error test to
split non-empty stderr lines and require them to equal only expectedMessage.
Replace the permissive toContain and separate negative checks while preserving
the status assertion and the launcher’s single-line Error: diagnostic contract.
🪄 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: f170778f-2a8d-4eee-ad35-0cf853021de2
📒 Files selected for processing (2)
bin/nemoclaw.jstest/package-contract/cli/top-level-error.test.ts
Signed-off-by: danielpolimac <danielpolimac@gmail.com>
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests. E2E guidanceAdvisory only. A maintainer can dispatch the default E2E suite against this exact revision. Recommended E2E: None This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
Signed-off-by: danielpolimac <danielpolimac@gmail.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Security review — PASSReviewed branch revision
Validation: CLI build passed; the focused package-contract suite passed 8/8; changed-file and branch-diff hooks passed, including formatting, lint, repository policy checks, environment-variable documentation, secret scan, source-shape, and test-size gates; normal pre-push TypeScript and version checks passed. The independent documentation review concluded |
Summary
Catches synchronous launcher initialization failures before Node.js can print a raw stack trace. Invalid require-time configuration such as
NEMOCLAW_GATEWAY_PORT=8081 nemoclaw --helpnow exits with a specific single-lineError:diagnostic, matching the existing async top-level error path.Related Issue
Related to #8202
Changes
bin/nemoclaw.jsmodule loading into the top-level error handler so synchronousrequire("../dist/nemoclaw")failures are reported cleanly.--helppath, logger-fallback credential redaction, an untrusted invalid port value, and simultaneous logger/redactor failure.Type of Change
Quality Gates
8081value are already documented; this PR changes only the launcher's error presentation for that existing contract.Documentation Writer Review
no-docs-neededbin/nemoclaw.js,test/package-contract/cli/top-level-error.test.ts, existing docs atdocs/reference/commands.mdxanddocs/reference/troubleshooting.mdxDGX Station Hardware Evidence
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 run build:clipassed;npx vitest run --project package-contract test/package-contract/cli/top-level-error.test.tspassed, 1 file and 8 tests; branch-diff hooks and normal pre-push checks passed.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:npm run docsbuilds without warnings (doc changes only)Signed-off-by: danielpolimac danielpolimac@gmail.com