Skip to content

fix(cli): catch launcher initialization errors - #8547

Open
danielpolimac wants to merge 5 commits into
NVIDIA:mainfrom
danielpolimac:issue-8202-clean-reserved-port-help
Open

fix(cli): catch launcher initialization errors#8547
danielpolimac wants to merge 5 commits into
NVIDIA:mainfrom
danielpolimac:issue-8202-clean-reserved-port-help

Conversation

@danielpolimac

@danielpolimac danielpolimac commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

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 --help now exits with a specific single-line Error: diagnostic, matching the existing async top-level error path.

Related Issue

Related to #8202

Changes

  • Move bin/nemoclaw.js module loading into the top-level error handler so synchronous require("../dist/nemoclaw") failures are reported cleanly.
  • Load the logger before the application entrypoint and retain logger-backed redaction for normal failures.
  • Use the shared redactor for fallback stderr; if it cannot load, print only tightly constrained built-in port diagnostics and fail closed to a generic message for all other errors.
  • Add package-contract regressions for the reserved-port --help path, logger-fallback credential redaction, an untrusted invalid port value, and simultaneous logger/redactor failure.

Type of Change

  • Code change (feature, bug fix, or refactor)
  • Code change with doc updates
  • Doc only (prose changes, no code sample modifications)
  • Doc only (includes code sample changes)

Quality Gates

  • Tests added or updated for changed behavior
  • Existing tests cover changed behavior — justification:
  • Tests not applicable — justification:
  • Docs updated for user-facing behavior changes
  • Docs not applicable — justification: the invalid port behavior and reserved 8081 value are already documented; this PR changes only the launcher's error presentation for that existing contract.
  • Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging)
  • Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: maintainer security review passed: fix(cli): catch launcher initialization errors #8547 (comment)
  • Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue:

Documentation Writer Review

  • Documentation writer subagent reviewed the completed changes
  • Result: no-docs-needed
  • Evidence: bin/nemoclaw.js, test/package-contract/cli/top-level-error.test.ts, existing docs at docs/reference/commands.mdx and docs/reference/troubleshooting.mdx
  • Agent: Codex CLI

DGX Station Hardware Evidence

  • Tested on DGX Station
  • Tested commit:
  • Station profile/scenario:
  • Result:
  • Supporting evidence:

Verification

  • PR description includes a Signed-off-by: line and every commit appears as Verified in GitHub
  • Normal pre-commit, commit-msg, and pre-push hooks passed, or npm run validate:pr passed after refreshing origin/main when hooks were skipped or unavailable
  • Targeted behavior tests pass for the current change set, or tests are marked not applicable above — command/result or justification: npm run build:cli passed; npx vitest run --project package-contract test/package-contract/cli/top-level-error.test.ts passed, 1 file and 8 tests; branch-diff hooks and normal pre-push checks passed.
  • Applicable broad gate passed — npm test for broad runtime/test-harness changes; npm run check for repo-wide validation/coverage changes — command/result:
  • Quality Gates section completed with required justifications or waivers
  • No secrets, API keys, or credentials committed
  • npm run docs builds without warnings (doc changes only)
  • Doc pages follow the style guide (doc changes only)
  • New doc pages include SPDX header and frontmatter (new pages only)

Signed-off-by: danielpolimac danielpolimac@gmail.com

Signed-off-by: danielpolimac <danielpolimac@gmail.com>
@copy-pr-bot

copy-pr-bot Bot commented Aug 7, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@coderabbitai

coderabbitai Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The 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.

Changes

CLI error handling

Layer / File(s) Summary
Launcher error path
bin/nemoclaw.js
The launcher safely loads the logger and application entry point. It sanitizes startup errors, reports through the logger when available, falls back to redacted stderr output, and sets exit code 1.
Launcher failure validation
test/package-contract/cli/top-level-error.test.ts
Tests verify exit status, empty stdout, stderr formatting, absent stack traces, invalid gateway-port handling, and credential redaction when logger or redactor loading fails.

Estimated code review effort: 3 (Moderate) | ~15–30 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: handling launcher initialization errors in the CLI.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between d8fbb6e and 26f8c32.

📒 Files selected for processing (2)
  • bin/nemoclaw.js
  • test/package-contract/cli/top-level-error.test.ts

Comment thread test/package-contract/cli/top-level-error.test.ts Outdated
Signed-off-by: danielpolimac <danielpolimac@gmail.com>
@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor — No blocking findings reported

Advisor assessment: No blocking advisor findings reported
Next action: No advisor follow-up needed.
Findings: 0 blockers · 0 warnings · 0 suggestions

Model lanes

  • GPT-5.6 Terra (primary): Completed · high confidence · 0 blockers · 0 warnings · 0 suggestions
  • Nemotron 3 Ultra (second opinion): Completed · medium confidence · 0 blockers · 3 warnings · 0 suggestions
  • Model comparison: normalized findings differ; normalized terminology decisions match; normalized E2E selections match; Nemotron reported the same number of blockers, 3 more warnings, the same number of suggestions.

Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests.

E2E guidance

Advisory only. A maintainer can dispatch the default E2E suite against this exact revision.

Recommended E2E: None

Workflow run details

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>
@apurvvkumaria apurvvkumaria self-assigned this Aug 7, 2026
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
@apurvvkumaria

Copy link
Copy Markdown
Collaborator

Security review — PASS

Reviewed branch revision 5afeb270d after syncing current main (c31724f4e). No security findings remain.

  1. Input validation and trust boundaries — PASS. The emergency launcher fallback exposes only a tightly constrained set of built-in port diagnostics: fixed NEMOCLAW_*_PORT names, one-to-five decimal digits, and fixed validation reasons. All other text fails closed to Command failed.
  2. Authentication and authorization — PASS / not applicable. This changes only local CLI initialization-error reporting and adds no privileged action or authorization boundary.
  3. Secrets and sensitive data — PASS. Normal fallback output uses the shared redactor. If that redactor cannot load, arbitrary error text is never echoed. Regressions cover both an untrusted credential-shaped port value and failure of the logger plus shared redactor.
  4. Injection and command execution — PASS. Error line breaks are flattened before output. The fail-closed path permits only fixed diagnostic grammar and decimal digits; it adds no command construction or execution.
  5. Network and SSRF — PASS / not applicable. No network request, URL parsing, or endpoint behavior changed.
  6. Filesystem and path safety — PASS / not applicable. No path handling or filesystem mutation changed.
  7. Dependencies and supply chain — PASS. No dependency, lockfile, workflow, or package-source changes were introduced.
  8. Cryptography and integrity — PASS / not applicable. No cryptographic behavior changed. Contributor and maintainer commits retain signed-off declarations and signatures.
  9. Errors, availability, and observability — PASS. Known reserved-port failures remain actionable and single-line. Unexpected loader, redactor, coercion, logger, or stderr failures remain bounded and do not expose a stack trace or untrusted message.

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 no-docs-needed because the existing command and troubleshooting pages already own the port contract and remediation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants