Skip to content

fix(uninstall): exit nonzero when openshell is unavailable (#7628) - #7631

Merged
senthilr-nv merged 5 commits into
NVIDIA:mainfrom
kagura-agent:fix/uninstall-openshell-exit-nonzero
Jul 27, 2026
Merged

fix(uninstall): exit nonzero when openshell is unavailable (#7628)#7631
senthilr-nv merged 5 commits into
NVIDIA:mainfrom
kagura-agent:fix/uninstall-openshell-exit-nonzero

Conversation

@kagura-agent

@kagura-agent kagura-agent commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Summary

nemoclaw uninstall now exits nonzero when the openshell command is unavailable.
The check runs after confirmation and before any cleanup mutation, so the user can restore the command and retry without a partial uninstall.

Related Issue

Fixes #7628

Product Scope

Product scope is approved independently of GitHub merge state.
This PR restores the existing supported uninstall contract documented in docs/manage-sandboxes/uninstall-nemoclaw.mdx and docs/reference/commands.mdx.
It adds no integration, recipe, image, third-party stack, configuration, or product surface.

Changes

  • Check for the openshell command after uninstall confirmation and before the cleanup plan mutates services, processes, or files.
  • Retain the check inside removeOpenShellResources() so the plan fails if the command disappears after the first check.
  • Report an actionable error that tells the user to restore openshell to PATH and run uninstall again.
  • Extend the regression test to prove that the missing-command path runs no command, kill, or remove operation.
  • Resolve PR Review Advisor finding PRA-1, which reported that the original check ran after the "Stopping services" step.

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 canonical uninstall and command-reference pages already state the missing-OpenShell nonzero restore-and-retry 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: Codex Desktop reviewed all nine security categories for PR SHA 190841bc against base SHA 07553d70; all categories passed with no findings. This is agent evidence, not human approval.
  • 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: docs/manage-sandboxes/uninstall-nemoclaw.mdx and docs/reference/commands.mdx already state the missing-OpenShell nonzero restore-and-retry contract. No documentation or documentation-build input changed. Review covers PR SHA 190841bc against base SHA 07553d70.
  • Agent: Codex Desktop documentation-writer subagent

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 check:diff passed 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: npx vitest run --project cli src/lib/actions/uninstall/run-plan-gateway-segregation.test.ts passed 27 tests; npm run build:cli passed.
  • Applicable broad gate passed — npm test for broad runtime/test-harness changes; npm run check for repo-wide validation/coverage changes — command/result: Not applicable; the diff changes one uninstall guard and its focused regression.
  • 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)

Exact-state evidence:

  • PR SHA: 190841bc46ba8b64ee9811b9c00930e106eb262f
  • Base SHA: 07553d70b7721e83f18362e7b172b3f11c321af7
  • Refresh: signed, non-force merge from upstream/main
  • Displayed commits: four; GitHub reports all four as Verified
  • Cross-issue sweep: no adjacent fix or contradiction above the medium-confidence floor
  • Human Files changed review and GitHub approval: pending for this exact PR SHA
  • DGX Station or other hardware review: not performed and not required by the changed paths

Signed-off-by: Kagura kagurachen@agent.qq.com
Signed-off-by: Senthil Ravichandran senthilr@nvidia.com

removeOpenShellResources() conditionally returned success when
openshell was not on PATH, allowing the uninstall plan to continue
through remaining steps and exit 0.  Always return false and emit
an actionable error so the caller exits nonzero before OpenShell
cleanup, matching the documented contract.

Signed-off-by: Kagura <kagurachen@agent.qq.com>
Signed-off-by: kagura-agent <kagura.agent.ai@gmail.com>
@copy-pr-bot

copy-pr-bot Bot commented Jul 27, 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 Jul 27, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The uninstall flow now exits nonzero when openshell is unavailable, logs an actionable error, and prevents further cleanup. The gateway segregation test adds service-file setup and verifies no commands or resource-removal operations run.

Changes

OpenShell uninstall failure handling

Layer / File(s) Summary
Missing OpenShell command handling
src/lib/actions/uninstall/run-plan.ts, src/lib/actions/uninstall/run-plan-gateway-segregation.test.ts
The uninstall implementation returns failure with a shared error message when openshell is missing, while the test verifies the updated message and confirms that commands, process termination, and file removal are not invoked.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested reviewers: apurvvkumaria, cv, laitingsheng

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The code now stops uninstall and returns failure when openshell is missing, matching #7628's expected nonzero exit and retry guidance.
Out of Scope Changes check ✅ Passed Changes are limited to the uninstall flow and its test, with no unrelated feature work.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely matches the main change: uninstall now exits nonzero when openshell is unavailable.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@github-actions

github-actions Bot commented Jul 27, 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): Failed after a partial review · low confidence · 0 blockers · 5 warnings · 0 suggestions

Nemotron output stays in workflow artifacts and does not change the assessment above.

E2E guidance

Advisory only. E2E / PR Gate selects and runs jobs independently.

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.

@cv cv added the v0.0.97 label Jul 27, 2026
@senthilr-nv senthilr-nv self-assigned this Jul 27, 2026
@senthilr-nv senthilr-nv added bug-fix PR fixes a bug or regression area: install Install, setup, prerequisites, or uninstall flow labels Jul 27, 2026

@senthilr-nv senthilr-nv left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Approved for exact head 9e6cc74 against base 0b722a4. The uninstall guard now fails before cleanup mutations, retains the TOCTOU check, and tests verify no command, kill, or removal occurs. No blocking issues found.

@senthilr-nv

Copy link
Copy Markdown
Collaborator

Automated-review disposition for exact head 190841bc46ba8b64ee9811b9c00930e106eb262f against base 07553d70b7721e83f18362e7b172b3f11c321af7:

The primary PR Review Advisor completed with high confidence and reported 0 blockers, 0 warnings, and 0 suggestions. The optional Nemotron lane stopped after a partial, low-confidence review and preserved five warning-shaped records. Each is dispositioned below:

  1. Fail-fast behavior change: intended and accepted. This restores issue nemoclaw uninstall silently skips OpenShell cleanup when openshell binary is unavailable instead of exiting nonzero #7628's binding acceptance criteria and the already-published uninstall contract. Adding a new --force surface would be out of scope and lacks product-scope approval.
  2. Guard placement: positive confirmation, not an actionable finding. The guard returns before executePlan.
  3. Acceptance coverage: positive confirmation, not an actionable finding. The implementation and regression test satisfy nemoclaw uninstall silently skips OpenShell cleanup when openshell binary is unavailable instead of exiting nonzero #7628.
  4. Documentation alignment: positive confirmation, not an actionable finding. The exact-head documentation-writer review found no-docs-needed; the existing uninstall and command-reference pages already state the behavior.
  5. --keep-openshell interaction: not actionable. That flag preserves the OpenShell binary, managed service files, and gateway process, but uninstall still removes OpenShell sandboxes, providers, and the selected gateway registration. Those operations require the openshell command, so the unconditional availability guard is correct.

The regression test verifies exit code 1, the actionable error, and that no command, process kill, or file removal occurs. No code or documentation change is required from these partial-lane records.

@senthilr-nv
senthilr-nv merged commit 10fc8b1 into NVIDIA:main Jul 27, 2026
60 of 65 checks passed
@cjagwani cjagwani added v0.0.98 and removed v0.0.97 labels Jul 28, 2026
@cjagwani cjagwani mentioned this pull request Jul 28, 2026
23 tasks
cv pushed a commit that referenced this pull request Jul 28, 2026
<!-- markdownlint-disable MD041 -->
## Summary

Add the canonical dated changelog entry for NemoClaw v0.0.97 before the
release plan captures `origin/main`.
The entry groups the user-visible and maintainer-facing changes since
v0.0.96 while preserving the Deferred dual-Station status, experimental
runtime-identity boundary, and pending physical IGX validation.

## Changes

- Add `docs/changelog/2026-07-28.mdx` with the parser-safe MDX SPDX
comment and exact `## v0.0.97` heading.
- Summarize the 43 merged PRs in the release range, omitting
internal-only changes from the public entry and linking each grouped
change to its most specific published documentation.
- Keep the experimental Okta reference explicitly opt-in and outside
normal onboarding, keep the two-Station path Deferred, and state that
physical IGX Orin validation remains pending.

### Source summary

- [#7440](#7440),
[#7443](#7443), and
[#7445](#7445) ->
`docs/changelog/2026-07-28.mdx`: Document read-only host readiness
reports and fail-closed platform qualification.
- [#7030](#7030) ->
`docs/changelog/2026-07-28.mdx`: Document the Deferred trusted
two-Station vLLM evaluation.
- [#7265](#7265) ->
`docs/changelog/2026-07-28.mdx`: Document the bounded experimental
direct-runner Okta runtime-identity reference.
- [#7711](#7711) and
[#7648](#7648) ->
`docs/changelog/2026-07-28.mdx`: Document compatible-endpoint reasoning
effort and retired NVIDIA Build model paths.
- [#7746](#7746),
[#7763](#7763), and
[#7681](#7681) ->
`docs/changelog/2026-07-28.mdx`: Document safe compatible-provider
creation, replacement refusal, and narrow OpenShell bridge URL handling.
- [#7641](#7641),
[#7690](#7690),
[#7631](#7631), and
[#7710](#7710) ->
`docs/changelog/2026-07-28.mdx`: Document paused-container recovery,
recreation journaling, pre-mutation uninstall checks, and
source-checkout OpenShell selection.
- [#7624](#7624) and
[#7762](#7762) ->
`docs/changelog/2026-07-28.mdx`: Document Jetson release diagnostics and
bounded render-device group propagation.
- [#7639](#7639),
[#7760](#7760),
[#7721](#7721), and
[#7761](#7761) ->
`docs/changelog/2026-07-28.mdx`: Document Telegram, MCP media-type,
Hermes image-mode, and locked-restart fixes.
- [#7653](#7653) and
[#7680](#7680) ->
`docs/changelog/2026-07-28.mdx`: Document Deep Agents policy tasks and
the bounded Claude Code OAuth path.
- [#7679](#7679) ->
`docs/changelog/2026-07-28.mdx`: Document the checksum-bound libssh2 and
Python HTMLParser backports.
- [#7655](#7655),
[#7651](#7651),
[#7664](#7664),
[#7666](#7666),
[#7670](#7670),
[#7719](#7719), and
[#7741](#7741) ->
`docs/changelog/2026-07-28.mdx`: Document exact candidate E2E evidence,
Launchable selection, diagnostic consolidation, and trusted WSL
validation.

## Type of Change

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

## Quality Gates

- [ ] Tests added or updated for changed behavior
- [x] Existing tests cover changed behavior — justification:
`test/changelog-docs.test.ts` validates the dated changelog contract,
MDX header, heading uniqueness, and release-entry structure.
- [ ] Tests not applicable — justification:
- [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:

## Documentation Writer Review

- [x] Documentation writer subagent reviewed the completed changes
- Result: `docs-updated`
- Evidence: The committed `docs/changelog/2026-07-28.mdx` blob exactly
matches the reviewed file. Completeness, factual accuracy, link shape,
parser-safe MDX header, one-sentence-per-line style, `.docs-skip`
compliance, and bounded product claims passed.
- Agent: Codex Desktop documentation writer subagent
<!-- docs-review-head-sha: da6aa27 -->
<!-- docs-review-agents-blob-sha: be20a09 -->

## DGX Station Hardware Evidence

- [ ] Tested on DGX Station
- Tested commit: Not applicable; this PR changes only the dated
changelog.
- Station profile/scenario: Not applicable.
- Result: Not applicable.
- Supporting evidence: Not applicable.

## Verification

- [x] PR description includes a `Signed-off-by:` line 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 — `npx vitest run
test/changelog-docs.test.ts` passed 6/6.
- [ ] Applicable broad gate passed — `npm test` for broad
runtime/test-harness changes; `npm run check` for repo-wide
validation/coverage changes — not applicable to this doc-only release
entry.
- [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) —
completed with 0 errors and 2 pre-existing Fern warnings.
- [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)
— native changelog entries use the required parser-safe MDX SPDX comment
and intentionally have no frontmatter.

---
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

* **New Features**
* Added improved host readiness reporting and Jetson onboarding
guidance.
* Added controls for reasoning effort with compatible endpoints and
enhanced managed MCP discovery.
  * Improved Deep Agents task publication and preset support.
* **Bug Fixes**
* Hardened provider switching, sandbox recovery, uninstall behavior, and
Telegram connectivity.
* Improved container image integrity checks, media-type handling, and
checksum validation.
  * Enhanced vLLM evaluation behavior and release diagnostics.
* **Documentation**
  * Added the NemoClaw v0.0.97 changelog.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: install Install, setup, prerequisites, or uninstall flow bug-fix PR fixes a bug or regression

Projects

None yet

Development

Successfully merging this pull request may close these issues.

nemoclaw uninstall silently skips OpenShell cleanup when openshell binary is unavailable instead of exiting nonzero

4 participants