fix(cli): stop detached Hermes forward watcher during uninstall - #7184
Conversation
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughUninstall now reads and validates managed Hermes forward watcher state, stops owned watcher processes and sandbox-scoped forwards, preserves state on failure, and reports unsuccessful cleanup. Regression tests cover ownership, command matching, stale state, gateway selection, process reuse, and failure cases. ChangesHermes watcher cleanup
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant runUninstallPlan
participant stopHermesForwardWatchers
participant stopHermesForwardWatcherProcess
participant stopHermesSandboxForward
runUninstallPlan->>stopHermesForwardWatchers: clean up watcher state
stopHermesForwardWatchers->>stopHermesForwardWatcherProcess: stop verified watcher PID
stopHermesForwardWatcherProcess-->>stopHermesForwardWatchers: process stop result
stopHermesForwardWatchers->>stopHermesSandboxForward: stop sandbox forward
stopHermesSandboxForward-->>stopHermesForwardWatchers: forward stop result
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage remains at 96%, unchanged from the TypeScript / code-coverage/cliThe overall coverage in the Show a code coverage summary of the most impacted files.
Updated |
PR Review Advisor — InformationalAdvisor assessment: Informational / high confidence Model lanes
Nemotron output stays in workflow artifacts and does not change the assessment above. E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: 3 optional E2E recommendations
1 warning · 0 suggestionsWarningsWarnings do not block.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@src/lib/actions/uninstall/run-plan.ts`:
- Around line 727-806: Move Hermes watcher cleanup responsibilities out of
stopHermesForwardWatchers in run-plan.ts: place ps/kill and OpenShell forwarding
operations in adapters, PID-file discovery and parsing in state, and the pure
ownership/argv classification in domain. Update the run-plan.ts call site and
stopHermesForwardWatchers to only orchestrate these helpers and propagate the
aggregate success result, removing the current direct fs, process, and OpenShell
interactions without leaving duplicate implementations.
🪄 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: 9f54c467-a3da-4b1c-a44b-3b94988dc9e7
📒 Files selected for processing (2)
src/lib/actions/uninstall/run-plan-hermes-forward-watcher.test.tssrc/lib/actions/uninstall/run-plan.ts
laitingsheng
added
bug-fix
Keep selected-gateway cleanup scoped and verify exact process identity. Preserve retry state when cleanup cannot be confirmed. Co-authored-by: Tinson Lai <tinsonl@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
🌿 Preview your docs: https://nvidia-preview-pr-7184.docs.buildwithfern.com/nemoclaw |
Describe per-watcher cleanup, sibling gateway isolation, and retry behavior when watcher or forward cleanup cannot be confirmed. Co-authored-by: Tinson Lai <tinsonl@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/lib/adapters/openshell/hermes-forward-watcher.ts (1)
99-118: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRedundant re-check between SIGTERM and SIGKILL.
waitForWatcherExit's own return value already reflects!isManagedWatcherRunning(...)at the same instant; the extraif (!isManagedWatcherRunning(watcher, host))right after immediately repeats that same check (3 morepsspawns) and will practically never diverge from whatwaitForWatcherExitjust returned. It's dead code that muddies the SIGTERM→SIGKILL escalation logic.♻️ Proposed simplification
host.kill(pid); if (waitForWatcherExit(watcher, host, 1000)) { host.log(`Stopped Hermes forward watcher ${pid}`); return true; } - if (!isManagedWatcherRunning(watcher, host)) { - host.log(`Stopped Hermes forward watcher ${pid}`); - return true; - } host.kill(pid, "SIGKILL");🤖 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/adapters/openshell/hermes-forward-watcher.ts` around lines 99 - 118, Remove the redundant isManagedWatcherRunning check after the first waitForWatcherExit call in the watcher shutdown flow. Let a successful waitForWatcherExit return immediately, and proceed directly to SIGKILL escalation when it returns false; preserve the existing final wait, logging, and failure behavior.
🤖 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 `@docs/reference/commands.mdx`:
- Around line 3076-3077: Reformat the paragraph near the Hermes uninstall
documentation so each sentence starts on its own source line. Keep the wording
and paragraph structure unchanged, including the sentences describing watcher
and sandbox-forward cleanup and the nonzero return behavior.
---
Nitpick comments:
In `@src/lib/adapters/openshell/hermes-forward-watcher.ts`:
- Around line 99-118: Remove the redundant isManagedWatcherRunning check after
the first waitForWatcherExit call in the watcher shutdown flow. Let a successful
waitForWatcherExit return immediately, and proceed directly to SIGKILL
escalation when it returns false; preserve the existing final wait, logging, and
failure behavior.
🪄 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: 66a54d9c-9c43-4a9b-a326-de435c846ef2
📒 Files selected for processing (7)
docs/reference/commands.mdxsrc/lib/actions/uninstall/hermes-forward-watcher-cleanup.tssrc/lib/actions/uninstall/run-plan-hermes-forward-watcher.test.tssrc/lib/actions/uninstall/run-plan.tssrc/lib/adapters/openshell/hermes-forward-watcher.tssrc/lib/domain/uninstall/hermes-forward-watcher.tssrc/lib/state/hermes-forward-watcher.ts
Verify that a transient watcher-stop failure preserves retry state. Confirm a successful second attempt removes the watcher, forward, and state. Co-authored-by: Tinson Lai <tinsonl@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
apurvvkumaria
left a comment
apurvvkumaria
left a comment
There was a problem hiding this comment.
Reviewed exact head 0958a8f. The cleanup is gateway-scoped, validates watcher ownership and exact argv before signaling, fails closed on unreadable state or incomplete cleanup, and has focused retry/foreign-PID coverage. Required CI, advisors, and protected E2E are green. No blocking findings.
Co-authored-by: Tinson Lai <tinsonl@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
|
Review result at head Blocking findings
Validation
Security-review verdict: FAIL pending fixes. Argument-array execution, PID/port validation, symlink-resistant PID reading, secrets, serialization, dependencies, and logging look safe; the selected-gateway authorization boundary and managed-process identity contract do not yet satisfy the required lifecycle guarantees. |
<!-- markdownlint-disable MD041 --> ## Summary Add the canonical dated changelog entry for NemoClaw `v0.0.89` before the release plan captures the tag commit. The entry summarizes the user-visible Station preparation, inference, recovery, policy-disclosure, and CLI-containment changes merged since `v0.0.88`. ## Changes - Add `docs/changelog/2026-07-20.mdx` with the exact `## v0.0.89` release heading, parser-safe SPDX comment, summary, and detailed bullets. - Link each shipped theme to the most specific published OpenClaw documentation routes. - Keep internal E2E, advisory-registry, and review-workflow refactors out of the user-facing release summary. Source summary: - #7214, #7241, #7237, #7223, #7204, #7202, #7183, and #7090 -> `docs/changelog/2026-07-20.mdx`: Summarize qualified DGX Station identity, package-state, PackageKit, DKMS, and reboot-handoff fixes. - #7242, #7221, #7186, #7164, and #6874 -> `docs/changelog/2026-07-20.mdx`: Summarize inference endpoint provenance, provider attachment, managed vLLM cleanup and progress, and Ollama selection guidance. - #7225, #7216, #7192, #7136, #7096, and #6910 -> `docs/changelog/2026-07-20.mdx`: Summarize sandbox readiness, recovery guidance, rebuilt model routing, durable Hermes state, and gateway restart behavior. - #7187 -> `docs/changelog/2026-07-20.mdx`: Summarize complete effective messaging-preset egress disclosure before policy mutation. - #7218, #7165, and #7184 -> `docs/changelog/2026-07-20.mdx`: Summarize structured output containment, terminal-safe route display, and Hermes forward cleanup. ## 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 - [ ] Existing tests cover changed behavior — justification: - [x] Tests not applicable — justification: This PR adds release-history prose only; the focused changelog contract test validates its required structure 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 - [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 — `mise exec node@22.23.1 -- npx vitest run test/changelog-docs.test.ts` (6 passed). - [ ] 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) — `mise exec node@22.23.1 -- npm run docs` completed with 0 errors and 2 existing site-wide 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) — the native changelog entry uses the required parser-safe MDX SPDX comment and does not use frontmatter. --- Signed-off-by: Aaron Erickson <aerickson@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Expanded DGX Station installation support for qualified GB300 and OTA-upgraded environments. * Preserved selected inference providers, endpoints, model pins, and tuning settings during sandbox creation and rebuilds. * Improved sandbox recovery by validating availability and stability before restarting services. * Added clearer policy output showing the complete effective messaging egress configuration. * **Bug Fixes** * Hardened status and inference route displays by sanitizing terminal control characters. * Improved Hermes uninstall behavior by stopping detached dashboard forwarding. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
4 participants
Summary
A successful Hermes express onboard leaves a detached host-side Node watcher alive for the port 8642 forward (
install.shrestore_onboard_forward_after_post_checks), butnemohermes uninstallhad no cleanup path that matched it — the process shape isnode …/hermes-<sandbox>-8642.forward.pid.js <openshell> 8642 <sandbox>, which neither the dashboard-portpgrepnor theopenshell (sandbox create|ssh-proxy)scan catches. Uninstall printedNo local OpenShell forward processes foundand exited successfully while the watcher survived, then deleted its PID/script state, so the next Station express install failed its workload-quiescence preflight withAgent or inference workload is active. Uninstall now stops the managed watcher and its sandbox-scoped forward before removing state, and reports a non-zero exit if a watcher it owns cannot be stopped.Related Issue
Fixes #7163
Changes
stopHermesForwardWatcherstosrc/lib/actions/uninstall/run-plan.ts, wired into theStopping servicesstep (non-scoped uninstall path). It enumerates${nemoclawStateDir}/state/hermes-*-<port>.forward.pid, and for each entry only signals a PID that is both owned by the current user and whoseps -o args=contains the exact managed*.forward.pid.jsscript path, usingkill+waitForPidExitwith aSIGKILLfallback. Foreign-owned, reused (argv-mismatch), stale, and invalid PIDs are skipped without signalling. It also issuesopenshell forward stop <port> <sandbox>for the associated sandbox-scoped forward. Mirrors the existingstopOllamaAuthProxyownership-safe pattern.Uninstall completed with errorsmessage instead of the success banner, rather than silently reporting no forward processes.Type of Change
Quality Gates
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailablenpx vitest run --project cli src/lib/actions/uninstall/run-plan.test.ts src/lib/actions/uninstall/run-plan-hermes-forward-watcher.test.ts— 47 passed (40 existing + 7 new);npm run typecheck:cli— 0 errors.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: Tinson Lai tinsonl@nvidia.com
Summary by CodeRabbit
uninstallcommand reference with Hermes cleanup behavior.