fix(cli): exit non-zero for user-error/startup surfaces riding oclif.exit === 0 (#5974) - #5986
Merged
Conversation
A command's run() that throws an error merely carrying `oclif.exit === 0` (not oclif's own graceful ExitError(0)) was surfaced by #2666 but still reported success, so `$?` stayed 0 on a real failure and broke scriptability. Treat only a genuine ExitError(0) as a silent graceful exit; any other error on that channel now surfaces its message AND exits non-zero, with the existing blank-message fallback preserved. Add a hermetic regression matrix that runs the real `nemoclaw` binary against fake openshell/docker shims and asserts the reported user-error / unknown-command surfaces (credentials reset without a provider, skill install without a path, unknown sandbox action, share mount / upload to a nonexistent sandbox) print their error text and return a non-zero code. Closes #5974 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Yimo Jiang <yimoj@nvidia.com>
Contributor
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesExit code fix for oclif error surfaces
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in the Show a code coverage summary of the most covered files.
TypeScript / code-coverage/cliThe overall coverage in the Show a code coverage summary of the most covered files.
Updated |
Contributor
PR Review Advisor (Nemotron Ultra) — Changes requestedMerge posture: Do not merge yet Action checklist
Findings index
🚨 Required before mergeAddress these before merging unless a maintainer explicitly overrides the advisor with rationale.
|
Collaborator
Author
Real-CLI E2E transcript (reporter workflow)Each of the five reporter surfaces was run through the worktree binary These are codified hermetically in Scope note on the code change vs. the matrix: the per-command surfaces already return non-zero on current |
Contributor
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Contributor
Vitest E2E Scenario RecommendationRequired Vitest E2E scenarios: Dispatch required Vitest E2E scenarios:
Full Vitest E2E advisor summaryVitest E2E Scenario AdvisorBase: Required Vitest E2E scenarios
Optional Vitest E2E scenarios
Relevant changed files
|
Contributor
PR Review Advisor — No blocking findingsMerge posture: No blocking advisor findings Action checklist
Test follow-ups to resolve or justifyIf these cover changed behavior, prefer adding them in this PR; otherwise state why existing coverage is enough or link the follow-up.
This is an automated, non-binding review; it still expects maintainers and agents to respond to each required or warning item. Treat suggestions as current-PR improvements when they touch changed code; defer only with maintainer rationale or a linked follow-up. A human maintainer must make the final merge decision. |
![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: 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/exit-code-user-error-surfaces.test.ts`:
- Line 108: The “skill install with no path” test case is currently passing on
sandbox resolution instead of exercising the missing-path validation in `skill
install`. Update the setup in `exit-code-user-error-surfaces.test.ts` so this
row uses a minimal sandbox entry or otherwise bypasses sandbox lookup, allowing
the `skill install` command path to reach its required-argument error. Keep the
assertion focused on the user-facing missing-path message surfaced by `skill
install`, not the sandbox “does not exist” failure.
🪄 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: Enterprise
Run ID: 40a922df-d474-4786-bfd9-d678b1cb347e
📒 Files selected for processing (3)
src/lib/cli/oclif-runner.test.tssrc/lib/cli/oclif-runner.tstest/exit-code-user-error-surfaces.test.ts
The skill-install and unknown-action rows previously used an empty
registry, so they stopped at the dispatcher's "sandbox does not exist"
boundary instead of the issue's command-specific surfaces. Seed a single
registered sandbox so those rows resolve it and reach `skill install`'s
required-arg parser ("Missing 1 required arg: skillPath", exit 2) and the
dispatcher's unknown-action branch ("Unknown action: dcode", exit 1).
Keep the literal nonexistent-sandbox share/upload surfaces, and note that
the share-mount bad-remote-path diagnostic (#3414) is covered by existing
unit tests since it needs a live sandbox + host sshfs to reach.
Addresses PR Review Advisor PRA-1 and CodeRabbit feedback on PR #5986.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Yimo Jiang <yimoj@nvidia.com>
Collaborator
Author
Resolution for PR Review Advisor items (922c9f2)PRA-1 / PRA-T4 (regression matrix misses reported branches) — resolved. The matrix now seeds a single registered sandbox (
PRA-T5 / PRA-T6 / PRA-T7 (acceptance clauses) — the five surfaces now each have a spawned-CLI row asserting non-zero exit + branch-specific text, and the diagnostic-preservation guarantee is held by the substring assertions plus the All 5 matrix rows pass locally against the rebuilt |
The spawned-CLI rows asserted only `code !== 0`, but a timeout or signal kill leaves `spawnSync().status === null` (and the old `?? -1` fallback was non-zero), so a killed process could satisfy the "non-zero exit" claim without ever reaching the user-error branch. Assert the process launched (`error` undefined), was not signal/timeout-killed (`signal === null`), and returned a real positive exit code (`status > 0`). Addresses PR Review Advisor PRA-1 (re-run) on PR #5986. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Yimo Jiang <yimoj@nvidia.com>
Collaborator
Author
Resolution for PR Review Advisor re-run PRA-1 (a1e6999)PRA-1 (spawned CLI matrix can pass on timeout/signal termination) — resolved. The rows asserted only
|
The #5974 fix initially covered only runOclifCommandById. runOclifArgv — the native route for `internal`, `sandbox`, and unknown-child commands — called executeOclif, whose internal handle() runs Exit.exit(oclif.exit ?? 1), so a non-ExitError error riding oclif.exit === 0 still exited 0 there. Reimplement runOclifArgv as run → flush → handle by hand (what execute() does) so oclif keeps owning command lookup, parsing, help, and pretty-print, while we intercept the one case: a non-ExitError carrying oclif.exit === 0 surfaces its message and forces exit 1 instead of delegating to handle() (which would exit 0). Genuine ExitError(0) (Command.exit(0)/--help) still delegates to handle() for the graceful exit. Add runOclifArgv unit tests for the weird-error, blank-message, and graceful-ExitError(0) cases. Addresses PR Review Advisor PRA-2 on PR #5986. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Yimo Jiang <yimoj@nvidia.com>
Collaborator
Author
Resolution for PR Review Advisor (Nemotron Ultra) items (aa41ca9)
The companion GPT-5.5 advisor on the same head returned |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/cli/oclif-runner.test.ts (1)
156-168: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the observable non-failure state for
ExitError(0).This test proves silence/delegation, but not that the runner avoided forcing
process.exitCode = 1. Capture the exact error and assert the exit code stays non-failure.Strengthen the graceful-exit assertion
- runMock.mockRejectedValue(new ExitError("EEXIT: 0")); + const exitError = new ExitError("EEXIT: 0"); + runMock.mockRejectedValue(exitError); const errorLine = vi.fn(); await runOclifArgv(["sandbox", "list"], { rootDir: "/repo", error: errorLine }); expect(errorLine).not.toHaveBeenCalled(); - expect(handleMock).toHaveBeenCalled(); + expect(process.exitCode).toBeUndefined(); + expect(handleMock).toHaveBeenCalledWith(exitError);As per path instructions, “Review tests for behavioral confidence rather than implementation lock-in.”
🤖 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 `@src/lib/cli/oclif-runner.test.ts` around lines 156 - 168, Strengthen the graceful-exit test in oclif-runner.test.ts so it verifies the runner stayed in the non-failure state for a native-route ExitError(0). In the runOclifArgv test around the ExitError mock, keep asserting silence and delegation via errorLine and handleMock, and also capture/assert that the exact ExitError instance does not cause process.exitCode to be set to 1. Use the existing runOclifArgv, runMock, and handleMock symbols to keep the check aligned with the runner behavior rather than implementation details.Source: Path instructions
🤖 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 `@src/lib/cli/oclif-runner.test.ts`:
- Around line 156-168: Strengthen the graceful-exit test in oclif-runner.test.ts
so it verifies the runner stayed in the non-failure state for a native-route
ExitError(0). In the runOclifArgv test around the ExitError mock, keep asserting
silence and delegation via errorLine and handleMock, and also capture/assert
that the exact ExitError instance does not cause process.exitCode to be set to
1. Use the existing runOclifArgv, runMock, and handleMock symbols to keep the
check aligned with the runner behavior rather than implementation details.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 60e5999b-c310-4ef7-b3f1-b0395ad3d450
📒 Files selected for processing (2)
src/lib/cli/oclif-runner.test.tssrc/lib/cli/oclif-runner.ts
Strengthen the runOclifArgv graceful-exit case to prove the runner does not force process.exitCode = 1 and that the exact ExitError instance is delegated to oclif's handle() (which performs the graceful exit 0). Addresses CodeRabbit nitpick on PR #5986. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Yimo Jiang <yimoj@nvidia.com>
wscurran
added
area: cli
Collaborator
Author
CodeRabbit nitpick resolution (e00ab90)Nitpick — The nitpick review was submitted at 12:17:48Z; commit const exitError = new ExitError("EEXIT: 0");
runMock.mockRejectedValue(exitError);
// ...
expect(errorLine).not.toHaveBeenCalled();
expect(process.exitCode).toBeUndefined(); // runner does NOT force a failure code
expect(handleMock).toHaveBeenCalledWith(exitError); // delegates the graceful exit 0 to handle()The subsequent No code changes needed; flagging the item as resolved/stale. |
Contributor
|
Tip For best results, initiate chat on the files or code changes.
|
The PR Review Advisor (PRA-1/PRA-2/PRA-T1-T4) asked for regression tests locking the non-zero exit of onboard dashboard-port exhaustion (#5974 instance 3) and Model Router Python preflight (instance 5). Reaching either surface through the real onboard flow hermetically requires emulating a chain of host openshell capability gates (verified by local probe: --version >= 0.0.44, then a request-body-credential-rewrite probe, and more) that a bash shim cannot stand in for reliably. Both surfaces already exit non-zero and, unlike the surfaces this PR fixes, neither rode the oclif.exit === 0 catch-all: instance 3 exits via explicit exitFn(1) (locked by onboard/dashboard-port.test.ts, asserting exit code 1 + canonical message); instance 5 throws a plain Error with no swallowing catch (locked by onboard/model-router-python.test.ts), whose thrown-error -> non-zero exit composition is locked by cli/oclif-runner.test.ts. Document this carve-out in the matrix header, mirroring the existing #3414 deferral to unit tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Yimo Jiang <yimoj@nvidia.com>
Collaborator
Author
Resolution for PR Review Advisor items PRA-1 / PRA-2 / PRA-T1–T4 (999eb13)These four items all reduce to the same ask: lock the non-zero exit of #5974 instance 3 (onboard dashboard-port exhaustion) and instance 5 (Model Router Python preflight) in Why not a spawn row in this matrixI empirically probed the real
Emulating that chain in a shim is brittle and high-maintenance for a path that is already locked elsewhere. This is the same reasoning the matrix already applies to the #3414 Both surfaces already exit non-zero and never rode the bug this PR fixesThe #5974 fix hardens the
Verification (local, this head)
No production code changed; the runner fix and its matrix remain as reviewed. PRA-T1–T4 are the test-follow-up framing of the same two items and are covered by the above. |
Contributor
E2E Target RecommendationRequired E2E targets: Dispatch required E2E targets:
Full E2E target advisor summaryE2E Target AdvisorBase: Required E2E targets
Optional E2E targets
Relevant changed files
|
The GPT-5.5 PR Review Advisor (PRA-1) asked for spawned coverage of the native oclif argv path (src/lib/cli/oclif-runner.ts): one native parse/user-error route that exits non-zero, and one native help route that stays a clean exit 0. The user-error direction is already locked by the existing 'credentials reset without a provider' row (oclif parse error, exit 2, native route). Add the missing counterpart: a spawned 'credentials --help' case that asserts a clean exit 0 through the real binary, so the #5974 hardening cannot over-correct a genuine ExitError(0) graceful exit. This is the real-CLI lock for the ExitError(0) unit test in src/lib/cli/oclif-runner.test.ts. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Yimo Jiang <yimoj@nvidia.com>
Collaborator
Author
Resolution for GPT-5.5 advisor PRA-1 / PRA-T1–T3 — native oclif argv spawned coverage (fd26d4a)PRA-1 (tests): "Add a small spawned CLI regression that uses the real Both directions of the native oclif argv route (
I verified the native route exit codes directly against the real binary:
Verification (local, head fd26d4a)
PRA-T1–T3 are the test-follow-up framing of this same native-route coverage and are covered by the above. |
The prior 'credentials --help' case went through the by-id dispatcher
(runOclifCommandById), not the native argv route the advisor asked to
cover. dispatchCli routes a leading 'sandbox'/'internal' token straight
to runOclifArgv (src/lib/cli/oclif-runner.ts), so switch the spawned
coverage to that route and lock both directions:
- 'sandbox bogus-subcmd' -> oclif 'command not found', exit non-zero
(exit 2), exercising the native-path hardening this PR adds; and
- 'sandbox --help' -> clean exit 0, a genuine ExitError(0) the
hardening must not over-correct.
Both resolve at oclif command lookup before any gateway probe, so they
stay hermetic under the existing fakes.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Yimo Jiang <yimoj@nvidia.com>
Collaborator
Author
Correction for GPT-5.5 advisor PRA-1 — now targets the real native argv route (9663903)The advisor correctly still flagged PRA-1 on
Both resolve at oclif's command lookup before any gateway probe, so they stay hermetic under the existing fakes. Verification (local, head 9663903)
This directly exercises |
The long it() title pushed the call past biome's line width; wrap it as biome's formatter does so static-checks (Files were modified by hooks) passes. No behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Yimo Jiang <yimoj@nvidia.com>
Nemotron advisor PRA-1 asked for a regression test locking the non-zero exit of onboard dashboard-port exhaustion (issue #5974 instance 3). Add a hermetic spawn that binds the whole dashboard port range (18789-18799) and drives the real onboard preflight to its fail-fast 'All dashboard ports in range ... are occupied' exit, asserting a non-zero status. The fake openshell shim reports a supported version and embeds the request-body/websocket credential-rewrite capability markers the installer greps for with 'strings', so preflight neither attempts a network reinstall nor fails the capability gate before reaching the port check. Verified hermetic (no download) and stable across repeated runs (~1.5-3s). This surface exits via an explicit process.exit(1) and never rode the oclif.exit === 0 catch-all, so the test simply locks the end-to-end non-zero exit. Instance 5 (Model Router Python preflight) runs only behind live gateway+provider+sandbox provisioning and stays covered by its unit tests, as documented in the file header. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Yimo Jiang <yimoj@nvidia.com>
Collaborator
Author
Update for Nemotron advisor PRA-1 / PRA-2 — instance 3 now has a hermetic spawn test (900e83d)Reworked the response after finding the onboard port-exhaustion path is hermetically reachable:
Verification (local, head 900e83d)
PRA-T1–T4 (the test follow-ups for instances 3 & 5) are covered by the above. |
… condition (#5974) Address Nemotron advisor improvement items on the #5974 hardening: - PRA-4: explain why runOclifArgv forces a non-zero exit via process.exitCode + return (it mirrors oclif execute() and must not re-enter handle(), which would Exit.exit(0)) versus runOclifCommandById, which maps errors to codes by hand and never routes through handle(). - PRA-6 / PRA-1: state the removal condition — drop the guard once @oclif/core's handle() no longer exits 0 for a non-ExitError carrying oclif.exit === 0. - PRA-5: cross-reference the runtime E2E counterpart (test/exit-code-user-error-surfaces.test.ts native --help case) from the mocked ExitError(0) unit test. Comments only; no behavior or coverage change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Yimo Jiang <yimoj@nvidia.com>
Collaborator
Author
Resolution for Nemotron advisor PRA-1 … PRA-6 (e935388)The advisor's last pass reviewed Improvements — now implemented as code docs
Acceptance items
Verification (local, head e935388)
PRA-T1–T6 are the test-follow-up framing of the above and are covered. |
PRA-3: the Model Router Python preflight throw (#5974 instance 5) was asserted only at the message level (model-router-python.test.ts); the catch→exitProcess(1) composition in the routed provider/inference handler was unlocked, and the E2E matrix comment misattributed it to oclif-runner.test.ts (a path this plain-Error never traverses). Add a provider-inference handler test asserting reconcileModelRouter's throw is caught and converted to a non-zero exit, and correct the instance-5 justification comment to reference the real chain. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Yimo Jiang <yimoj@nvidia.com>
wscurran
added
integration: dcode
Contributor
Vitest E2E Scenario Results — ❌ Some jobs failedRun: 28612082640
|
Contributor
Vitest E2E Target Results — ✅ All requested jobs passedRun: 28612470340
|
prekshivyas
approved these changes
Jul 2, 2026
prekshivyas
left a comment
prekshivyas
left a comment
Collaborator
There was a problem hiding this comment.
LGTM. Nemotron PRA-1 and PRA-2 are verification-only (comment present at oclif-runner.ts:183-189, port-exhaustion test passes in CI). Standard advisor is merge_as_is. E2E green on onboard-negative-paths and sandbox-operations.
Contributor
Vitest E2E Scenario Results — ✅ All requested jobs passedRun: 28636444972
|
21 tasks
ericksoa
added a commit
that referenced
this pull request
Jul 4, 2026
<!-- markdownlint-disable MD041 --> ## Summary This PR prepares the user-facing documentation for v0.0.74 before the release plan is frozen. It expands the release notes across the 56-commit train and closes durable documentation gaps found during the pre-tag commit scan. ## Changes - Expand the `v0.0.74` release notes to cover OpenShell 0.0.72, managed MCP, progressive tool disclosure, LangChain Deep Agents Code, onboarding, local inference, messaging, recovery, and contributor workflows. - Correct the `destroy` contract for retained per-name volumes, gateway-unreachable `--force` cleanup, managed MCP ownership, and same-name recovery. - Document separate remediation for an unreachable container DNS resolver versus one that answers with `NXDOMAIN` or `REFUSED`. - Document the Windows on Arm N1X automatic Ollama safeguard and its remaining large-model limitations. - State that messaging conflicts abort rebuild before backup or deletion, leaving the original sandbox intact. - Link the agent-runnable value benchmark from the contributor task index. - Synchronize generated agent command variants. - Validate with `npm run docs:sync-agent-variants` and `npm run docs`; Fern completed with 0 errors and 2 existing warnings. - Source summary: - [#6020](#6020) and [#5876](#5876) -> `docs/about/release-notes.mdx`: Consolidate the OpenShell 0.0.72 policy boundary and managed MCP lifecycle. - [#6251](#6251) and [#5989](#5989) -> `docs/about/release-notes.mdx`: Summarize progressive tool disclosure and sandbox-first inference controls. - [#6232](#6232), [#6082](#6082), [#6219](#6219), [#6214](#6214), [#6215](#6215), [#6230](#6230), and [#6260](#6260) -> `docs/about/release-notes.mdx`: Summarize the experimental LangChain Deep Agents Code status, secret, version, rebuild, snapshot, and MCP boundaries. - [#6166](#6166), [#6254](#6254), [#6265](#6265), [#6164](#6164), and [#6017](#6017) -> `docs/about/release-notes.mdx`: Summarize BuildKit prebuild, validated image reuse, bounded readiness, and preflight improvements. - [#6150](#6150) -> `docs/about/release-notes.mdx` and `docs/reference/troubleshooting.mdx`: Separate unreachable-resolver remediation from reachable-but-rejected DNS responses. - [#6234](#6234) -> `docs/about/release-notes.mdx`, `docs/inference/use-local-inference.mdx`, and `docs/get-started/windows-preparation.mdx`: Document N1X automatic 9B selection and the remaining explicit-large-model boundary. - [#6129](#6129), [#5987](#5987), [#5955](#5955), and [#6220](#6220) -> `docs/about/release-notes.mdx`, `docs/manage-sandboxes/messaging-channels.mdx`, `docs/reference/commands.mdx`, and `docs/reference/commands-nemohermes.mdx`: Document messaging policy persistence, status, and the pre-destructive conflict check. - [#5963](#5963), [#6050](#6050), [#6094](#6094), [#6238](#6238), [#5988](#5988), [#6235](#6235), [#6181](#6181), and [#5986](#5986) -> `docs/about/release-notes.mdx`, `docs/reference/commands.mdx`, and `docs/reference/commands-nemohermes.mdx`: Summarize day-two recovery and clarify retained-volume and local-only destroy semantics. - [#6200](#6200), [#6248](#6248), [#6168](#6168), [#6270](#6270), and [#5649](#5649) -> `docs/about/release-notes.mdx` and `CONTRIBUTING.md`: Summarize contributor setup and verification improvements and expose the advisory value benchmark. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [x] Doc only (includes code sample changes) ## Quality Gates <!-- Check exactly one tests line and one docs line. Check other lines when applicable. Add every requested justification or approval reference. --> - [ ] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [x] Tests not applicable — justification: documentation-only release preparation; generated-variant synchronization and the Fern docs build validate the changed pages and routes. - [x] Docs updated for user-facing behavior changes - [ ] Docs not applicable — justification: - [ ] 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: - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Verification <!-- Check each applicable item only when supported by the requested evidence. Run targeted tests once per relevant change set and rerun after later edits or hook autofixes that can affect the tested behavior. Do not rerun hook-covered checks. --> - [x] PR description includes the DCO sign-off declaration and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run check:diff` passed when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — command/result or justification: tests are not applicable to this documentation-only change; `npm run docs` validates the source and generated routes. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) - [x] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) --- <!-- DCO sign-off is required in this PR description, and every commit must appear as Verified in GitHub. Run: git config user.name && git config user.email --> Signed-off-by: Aaron Erickson <aerickson@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Expanded setup guidance for Windows on Arm devices with safer default local model selection. * Clarified local inference and sandbox messaging behavior, including conflict checks before rebuilds and safer recovery steps. * Updated destroy/rebuild/reference docs with more detailed warnings, failure handling, and volume-retention guidance. * Improved troubleshooting instructions for Docker DNS issues with clearer paths for unreachable vs. blocked resolvers. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
This was referenced Jul 6, 2026
Hadar301
pushed a commit
to Hadar301/NemoClaw-OpenShift
that referenced
this pull request
Jul 12, 2026
…exit === 0 (NVIDIA#5974) (NVIDIA#5986) ## Summary Several `nemoclaw` user-error and unknown-command surfaces returned exit `0` even though they printed correct error text, which breaks `$?`-based scriptability (a watchdog or CI step wrapping the CLI could not detect the failure). This hardens the last structural exit-`0` hole in the oclif runner and locks the reported surfaces with a regression matrix. ## Related Issue Closes NVIDIA#5974 ## Changes - `src/lib/cli/oclif-runner.ts`: an error that merely happens to carry `oclif.exit === 0` (propagated out of a command's `run()`, not oclif's own graceful `ExitError(0)`) is now treated as a genuine failure — its message is surfaced **and** `process.exitCode` is set to `1`. Only a real `ExitError(0)` (e.g. `Command.exit(0)` / `--help`, whose synthetic `EEXIT: 0` message must stay silent) keeps exit `0`. The blank-message fallback line from NVIDIA#2666 is preserved. - `test/exit-code-user-error-surfaces.test.ts`: new hermetic regression matrix that runs the real `nemoclaw` binary against fake `openshell`/`docker` shims with an isolated `HOME`. A single sandbox is seeded so the command-specific rows resolve it and reach their exact branches: `credentials reset` (missing provider) and `<existing> skill install` (missing path) both hit the missing-required-arg parser, `<existing> dcode --help` hits the unknown-action branch, and `share mount` / `upload` against a nonexistent sandbox hit the literal reporter surfaces. - `src/lib/cli/oclif-runner.test.ts`: updated the two NVIDIA#2666 unit tests to assert the corrected non-zero exit while keeping the surfaced-message intent. Scope note: the per-command surfaces were re-tested on current `main` and already return non-zero (release drift since the v0.0.68 report); the matrix guards them against future regression, while the runner change closes the remaining catch-all path. The onboard startup paths (dashboard-port exhaustion, Python preflight) were verified to propagate as thrown errors through onboard's `try/finally` (no swallowing catch) and already exit non-zero, so they are left untouched. The `share mount` bad-remote-path diagnostic (NVIDIA#3414) needs a live sandbox + host `sshfs` to reach, so it stays covered by `src/lib/share-command.test.ts` / `test/share-command-remote-path.test.ts` rather than the hermetic spawn matrix. ## Type of Change - [x] Code change (feature, bug fix, or refactor) ## Quality Gates - [x] Tests added or updated for changed behavior - [x] Docs not applicable — justification: no user-facing behavior or flag changes; only exit codes are corrected to be non-zero on already-documented error messages. - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: CLI runner change is additive (only converts a wrongly-successful failure into a non-zero exit) and preserves legitimate graceful `ExitError(0)`; covered by unit + integration tests. ## Verification - [x] PR description includes the DCO sign-off declaration and every commit appears as `Verified` in GitHub - [x] Git hooks passed during commit and push, or `npx prek run --from-ref main --to-ref HEAD` passes - [x] Targeted tests pass for changed behavior - [x] No secrets, API keys, or credentials committed ### Reporter-workflow E2E (real worktree CLI) Ran each reporter surface through the worktree binary `./bin/nemoclaw.js` (Node entry → `dist/nemoclaw.js`) with an isolated `HOME`, a registry seeded with one sandbox (`bug5974-alpha`), and fake `openshell`/`docker` shims so no live gateway is contacted. Every command prints its error text and exits **non-zero**: ``` $ node ./bin/nemoclaw.js credentials reset Missing 1 required arg: provider OpenShell provider name => exit 2 $ node ./bin/nemoclaw.js bug5974-alpha skill install # existing sandbox, missing path Missing 1 required arg: skillPath Skill directory or direct path to SKILL.md => exit 2 $ node ./bin/nemoclaw.js bug5974-alpha dcode --help # existing sandbox, unknown action Unknown action: dcode Valid actions: agent, agents, channels, ... skill, snapshot, status, upload => exit 1 $ node ./bin/nemoclaw.js bug5974-missing-sb share mount /sandbox/bad-typo-path Sandbox 'bug5974-missing-sb' does not exist. => exit 1 $ node ./bin/nemoclaw.js bug5974-missing-sb upload some-file.txt Sandbox 'bug5974-missing-sb' does not exist. => exit 1 ``` This exact reporter workflow is codified hermetically in `test/exit-code-user-error-surfaces.test.ts`, which spawns the same `bin/nemoclaw.js` and asserts non-zero exit + branch-specific error text for each row. The behavioral fix itself lives in `src/lib/cli/oclif-runner.ts` (the `oclif.exit === 0` catch-all that previously surfaced a message but reported success). That path is a defensive catch-all not directly reachable from a fixed user command, so it is covered by unit tests in `src/lib/cli/oclif-runner.test.ts` rather than a CLI transcript. Other tests run locally: - `vitest run --project cli src/lib/cli/oclif-runner.test.ts` (11 passed) - `vitest run --project integration test/exit-code-user-error-surfaces.test.ts` (5 passed) - `vitest run --project integration test/repro-2666-silent-list-status.test.ts` (9 passed, no regression) - `npm run typecheck:cli`, `npm run typecheck`, biome lint + format, test title/size/overlap checks, `prek run --from-ref main --to-ref HEAD` — all pass. --- Signed-off-by: Yimo Jiang <yimoj@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Hardened CLI handling so errors that merely *carry* an exit code of `0` are treated as failures: they now surface a non-empty error message and exit with a non-zero status. * Preserved quiet behavior for genuine successful `ExitError(0)` exits. * **Tests** * Updated CLI runner tests to reflect the updated oclif mocking and the success-vs-failure exit/output distinctions. * Added end-to-end regression coverage for CLI error surfaces and dashboard port exhaustion. * Added a `NVIDIA#5974` provider inference regression test to confirm failure propagation and logging. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Yimo Jiang <yimoj@nvidia.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Hadar301
pushed a commit
to Hadar301/NemoClaw-OpenShift
that referenced
this pull request
Jul 12, 2026
<!-- markdownlint-disable MD041 --> ## Summary This PR prepares the user-facing documentation for v0.0.74 before the release plan is frozen. It expands the release notes across the 56-commit train and closes durable documentation gaps found during the pre-tag commit scan. ## Changes - Expand the `v0.0.74` release notes to cover OpenShell 0.0.72, managed MCP, progressive tool disclosure, LangChain Deep Agents Code, onboarding, local inference, messaging, recovery, and contributor workflows. - Correct the `destroy` contract for retained per-name volumes, gateway-unreachable `--force` cleanup, managed MCP ownership, and same-name recovery. - Document separate remediation for an unreachable container DNS resolver versus one that answers with `NXDOMAIN` or `REFUSED`. - Document the Windows on Arm N1X automatic Ollama safeguard and its remaining large-model limitations. - State that messaging conflicts abort rebuild before backup or deletion, leaving the original sandbox intact. - Link the agent-runnable value benchmark from the contributor task index. - Synchronize generated agent command variants. - Validate with `npm run docs:sync-agent-variants` and `npm run docs`; Fern completed with 0 errors and 2 existing warnings. - Source summary: - [NVIDIA#6020](NVIDIA#6020) and [NVIDIA#5876](NVIDIA#5876) -> `docs/about/release-notes.mdx`: Consolidate the OpenShell 0.0.72 policy boundary and managed MCP lifecycle. - [NVIDIA#6251](NVIDIA#6251) and [NVIDIA#5989](NVIDIA#5989) -> `docs/about/release-notes.mdx`: Summarize progressive tool disclosure and sandbox-first inference controls. - [NVIDIA#6232](NVIDIA#6232), [NVIDIA#6082](NVIDIA#6082), [NVIDIA#6219](NVIDIA#6219), [NVIDIA#6214](NVIDIA#6214), [NVIDIA#6215](NVIDIA#6215), [NVIDIA#6230](NVIDIA#6230), and [NVIDIA#6260](NVIDIA#6260) -> `docs/about/release-notes.mdx`: Summarize the experimental LangChain Deep Agents Code status, secret, version, rebuild, snapshot, and MCP boundaries. - [NVIDIA#6166](NVIDIA#6166), [NVIDIA#6254](NVIDIA#6254), [NVIDIA#6265](NVIDIA#6265), [NVIDIA#6164](NVIDIA#6164), and [NVIDIA#6017](NVIDIA#6017) -> `docs/about/release-notes.mdx`: Summarize BuildKit prebuild, validated image reuse, bounded readiness, and preflight improvements. - [NVIDIA#6150](NVIDIA#6150) -> `docs/about/release-notes.mdx` and `docs/reference/troubleshooting.mdx`: Separate unreachable-resolver remediation from reachable-but-rejected DNS responses. - [NVIDIA#6234](NVIDIA#6234) -> `docs/about/release-notes.mdx`, `docs/inference/use-local-inference.mdx`, and `docs/get-started/windows-preparation.mdx`: Document N1X automatic 9B selection and the remaining explicit-large-model boundary. - [NVIDIA#6129](NVIDIA#6129), [NVIDIA#5987](NVIDIA#5987), [NVIDIA#5955](NVIDIA#5955), and [NVIDIA#6220](NVIDIA#6220) -> `docs/about/release-notes.mdx`, `docs/manage-sandboxes/messaging-channels.mdx`, `docs/reference/commands.mdx`, and `docs/reference/commands-nemohermes.mdx`: Document messaging policy persistence, status, and the pre-destructive conflict check. - [NVIDIA#5963](NVIDIA#5963), [NVIDIA#6050](NVIDIA#6050), [NVIDIA#6094](NVIDIA#6094), [NVIDIA#6238](NVIDIA#6238), [NVIDIA#5988](NVIDIA#5988), [NVIDIA#6235](NVIDIA#6235), [NVIDIA#6181](NVIDIA#6181), and [NVIDIA#5986](NVIDIA#5986) -> `docs/about/release-notes.mdx`, `docs/reference/commands.mdx`, and `docs/reference/commands-nemohermes.mdx`: Summarize day-two recovery and clarify retained-volume and local-only destroy semantics. - [NVIDIA#6200](NVIDIA#6200), [NVIDIA#6248](NVIDIA#6248), [NVIDIA#6168](NVIDIA#6168), [NVIDIA#6270](NVIDIA#6270), and [NVIDIA#5649](NVIDIA#5649) -> `docs/about/release-notes.mdx` and `CONTRIBUTING.md`: Summarize contributor setup and verification improvements and expose the advisory value benchmark. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [x] Doc only (includes code sample changes) ## Quality Gates <!-- Check exactly one tests line and one docs line. Check other lines when applicable. Add every requested justification or approval reference. --> - [ ] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [x] Tests not applicable — justification: documentation-only release preparation; generated-variant synchronization and the Fern docs build validate the changed pages and routes. - [x] Docs updated for user-facing behavior changes - [ ] Docs not applicable — justification: - [ ] 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: - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Verification <!-- Check each applicable item only when supported by the requested evidence. Run targeted tests once per relevant change set and rerun after later edits or hook autofixes that can affect the tested behavior. Do not rerun hook-covered checks. --> - [x] PR description includes the DCO sign-off declaration and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run check:diff` passed when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — command/result or justification: tests are not applicable to this documentation-only change; `npm run docs` validates the source and generated routes. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) - [x] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) --- <!-- DCO sign-off is required in this PR description, and every commit must appear as Verified in GitHub. Run: git config user.name && git config user.email --> Signed-off-by: Aaron Erickson <aerickson@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Expanded setup guidance for Windows on Arm devices with safer default local model selection. * Clarified local inference and sandbox messaging behavior, including conflict checks before rebuilds and safer recovery steps. * Updated destroy/rebuild/reference docs with more detailed warnings, failure handling, and volume-retention guidance. * Improved troubleshooting instructions for Docker DNS issues with clearer paths for unreachable vs. blocked resolvers. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Aaron Erickson <aerickson@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
4 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
Several
nemoclawuser-error and unknown-command surfaces returned exit0even though they printed correct error text, which breaks$?-based scriptability (a watchdog or CI step wrapping the CLI could not detect the failure). This hardens the last structural exit-0hole in the oclif runner and locks the reported surfaces with a regression matrix.Related Issue
Closes #5974
Changes
src/lib/cli/oclif-runner.ts: an error that merely happens to carryoclif.exit === 0(propagated out of a command'srun(), not oclif's own gracefulExitError(0)) is now treated as a genuine failure — its message is surfaced andprocess.exitCodeis set to1. Only a realExitError(0)(e.g.Command.exit(0)/--help, whose syntheticEEXIT: 0message must stay silent) keeps exit0. The blank-message fallback line from [Ubuntu 22.04][CLI&UX][Recovery] nemoclaw status and list return empty output, exit 0 when container is stopped and gateway port is held #2666 is preserved.test/exit-code-user-error-surfaces.test.ts: new hermetic regression matrix that runs the realnemoclawbinary against fakeopenshell/dockershims with an isolatedHOME. A single sandbox is seeded so the command-specific rows resolve it and reach their exact branches:credentials reset(missing provider) and<existing> skill install(missing path) both hit the missing-required-arg parser,<existing> dcode --helphits the unknown-action branch, andshare mount/uploadagainst a nonexistent sandbox hit the literal reporter surfaces.src/lib/cli/oclif-runner.test.ts: updated the two [Ubuntu 22.04][CLI&UX][Recovery] nemoclaw status and list return empty output, exit 0 when container is stopped and gateway port is held #2666 unit tests to assert the corrected non-zero exit while keeping the surfaced-message intent.Scope note: the per-command surfaces were re-tested on current
mainand already return non-zero (release drift since the v0.0.68 report); the matrix guards them against future regression, while the runner change closes the remaining catch-all path. The onboard startup paths (dashboard-port exhaustion, Python preflight) were verified to propagate as thrown errors through onboard'stry/finally(no swallowing catch) and already exit non-zero, so they are left untouched. Theshare mountbad-remote-path diagnostic (#3414) needs a live sandbox + hostsshfsto reach, so it stays covered bysrc/lib/share-command.test.ts/test/share-command-remote-path.test.tsrather than the hermetic spawn matrix.Type of Change
Quality Gates
ExitError(0); covered by unit + integration tests.Verification
Verifiedin GitHubnpx prek run --from-ref main --to-ref HEADpassesReporter-workflow E2E (real worktree CLI)
Ran each reporter surface through the worktree binary
./bin/nemoclaw.js(Node entry →dist/nemoclaw.js) with an isolatedHOME, a registry seeded with one sandbox (bug5974-alpha), and fakeopenshell/dockershims so no live gateway is contacted. Every command prints its error text and exits non-zero:This exact reporter workflow is codified hermetically in
test/exit-code-user-error-surfaces.test.ts, which spawns the samebin/nemoclaw.jsand asserts non-zero exit + branch-specific error text for each row.The behavioral fix itself lives in
src/lib/cli/oclif-runner.ts(theoclif.exit === 0catch-all that previously surfaced a message but reported success). That path is a defensive catch-all not directly reachable from a fixed user command, so it is covered by unit tests insrc/lib/cli/oclif-runner.test.tsrather than a CLI transcript.Other tests run locally:
vitest run --project cli src/lib/cli/oclif-runner.test.ts(11 passed)vitest run --project integration test/exit-code-user-error-surfaces.test.ts(5 passed)vitest run --project integration test/repro-2666-silent-list-status.test.ts(9 passed, no regression)npm run typecheck:cli,npm run typecheck, biome lint + format, test title/size/overlap checks,prek run --from-ref main --to-ref HEAD— all pass.Signed-off-by: Yimo Jiang yimoj@nvidia.com
Summary by CodeRabbit
0are treated as failures: they now surface a non-empty error message and exit with a non-zero status.ExitError(0)exits.#5974provider inference regression test to confirm failure propagation and logging.