Skip to content

fix(onboard): allow packaged service teardown fallback - #8239

Merged
jyaunches merged 2 commits into
NVIDIA:mainfrom
VinayBhagavath:codex/fix-hermes-uninstall-authority
Aug 5, 2026
Merged

fix(onboard): allow packaged service teardown fallback#8239
jyaunches merged 2 commits into
NVIDIA:mainfrom
VinayBhagavath:codex/fix-hermes-uninstall-authority

Conversation

@VinayBhagavath

@VinayBhagavath VinayBhagavath commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Summary

nemohermes uninstall --yes no longer fails when teardown revalidates a default NemoClaw-managed gateway after the packaged gateway service has already been removed. Uninstall opts into that expected packaged-service to standalone resolution, while other teardown callers and credential mutation still use the default fail-closed authority check.

Related Issue

Fixes #8215

Changes

  • Add an explicit uninstall opt-in that allows gateway teardown to proceed when the recorded default NemoClaw-managed authority was packaged-service and the current default resolution is standalone because the packaged service has already been removed.
  • Keep the default teardown and provider credential mutation paths strict for the same transition so non-uninstall gateway effects still require a fresh onboarding run.
  • Add regression coverage for the uninstall teardown path and the credential-mutation fail-closed boundary.
  • Update the gateway lifecycle authority docs to describe the narrow full-uninstall exception.

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:
  • 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: Local security-sensitive review kept the exception opt-in for uninstall teardown of the same default NemoClaw-managed gateway; default teardown and credential mutation remain fail-closed and are covered by regression tests.
  • 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: docs-updated
  • Evidence: docs/deployment/gateway-lifecycle-authority.mdx; writing rules and documentation style reviewed.
  • Agent: Codex Desktop documentation writer reviewer

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/onboard/gateway-teardown-authority.test.ts src/lib/onboard/gateway-authority-migration.test.ts src/lib/actions/uninstall/run-plan-gateway-service.test.ts src/lib/actions/uninstall/run-plan.test.ts passed.
  • Applicable broad gate passed — npm test for broad runtime/test-harness changes; npm run check for repo-wide validation/coverage changes — command/result: npm run check:diff passed; npm run docs passed with Fern reporting 0 errors and 2 unrelated warnings. fern check --warnings reported an unauthenticated redirects check and a global light-mode accent color contrast warning.
  • 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: Vinay Bhagavath bhagavathvinay@gmail.com

Summary by CodeRabbit

  • Bug Fixes

    • Full uninstall now completes when a packaged gateway service has already been removed.
    • Remaining gateway registration and runtime resources are cleaned up correctly during this scenario.
    • Credential changes remain protected when teardown authority cannot be verified.
  • Documentation

    • Clarified gateway authority migration rules and the limited exception available during full uninstall.

@copy-pr-bot

copy-pr-bot Bot commented Aug 4, 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 4, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The change allows full uninstall teardown to accept a packaged-service authority that resolves as standalone after service removal. Credential mutation and other authority changes remain rejected. Tests and lifecycle documentation cover both outcomes.

Changes

Gateway authority teardown

Layer / File(s) Summary
Packaged-service teardown compatibility
src/lib/onboard/gateway-teardown-authority.ts, src/lib/onboard/gateway-teardown-authority.test.ts, docs/deployment/gateway-lifecycle-authority.mdx
Teardown accepts the packaged-service-to-standalone transition only when explicitly enabled. Credential mutation still rejects the transition without gateway effects. Tests and documentation define the full-uninstall exception.
Full-uninstall authority opt-in
src/lib/actions/uninstall/run-plan.ts, src/lib/actions/uninstall/run-plan-gateway-service.test.ts
Full uninstall passes allowMissingPackagedServiceTeardown: true while preserving the runtime environment. The regression test verifies the gateway identity, port, and opt-in flag.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

  • NVIDIA/NemoClaw#8098: Introduces the packaged-service fallback scenario addressed by this change.
  • NVIDIA/NemoClaw#8118: Also changes gateway authority migration handling in gateway-teardown-authority.ts.

Suggested labels: bug-fix, area: onboarding, area: docs

Suggested reviewers: sandl99, yanyunl1991, apurvvkumaria

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 20.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 The changes address issue [#8215] by allowing the expected packaged-service to standalone transition during full uninstall while preserving fail-closed checks elsewhere.
Out of Scope Changes check ✅ Passed The implementation, regression tests, and documentation directly support issue [#8215] and contain no unrelated changes.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: allowing packaged-service teardown fallback during uninstall.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@VinayBhagavath
VinayBhagavath force-pushed the codex/fix-hermes-uninstall-authority branch 2 times, most recently from 4004db3 to 6e2814f Compare August 4, 2026 17:03

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

🧹 Nitpick comments (1)
docs/deployment/gateway-lifecycle-authority.mdx (1)

154-160: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Update the section heading to include full uninstall.

This section now documents both managed authority rebuilds and full uninstall. The current heading names only rebuilds. Rename it to ### Migrate Managed Authority During Rebuild and Full Uninstall, or split the two exceptions into separate sections.

As per coding guidelines, documentation must be clear, accurate, and task-oriented.

🤖 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 `@docs/deployment/gateway-lifecycle-authority.mdx` around lines 154 - 160,
Update the section heading associated with “Migrate Managed Authority During
Rebuild” to explicitly include “Full Uninstall,” reflecting that the section
documents both exception cases. Preserve the existing content and heading level.

Source: Coding guidelines

🤖 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 `@docs/deployment/gateway-lifecycle-authority.mdx`:
- Around line 154-160: Update the section heading associated with “Migrate
Managed Authority During Rebuild” to explicitly include “Full Uninstall,”
reflecting that the section documents both exception cases. Preserve the
existing content and heading level.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: d9957e67-9cb4-4183-be33-e596984b1903

📥 Commits

Reviewing files that changed from the base of the PR and between 0d67bf3 and 6e2814f.

📒 Files selected for processing (1)
  • docs/deployment/gateway-lifecycle-authority.mdx

@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor — Informational

Advisor assessment: Informational / low confidence
Next action: No advisor follow-up needed.
Findings: 0 blockers · 0 warnings · 0 suggestions
Status: PR review advisor failed: PR review advisor SDK execution failed: session: 429 status code (no body); turn: terminology-review: 429 status code (no body)

Model lanes

  • GPT-5.6 Terra (primary): Failed
  • Nemotron 3 Ultra (second opinion): Failed

Second-opinion terminology and E2E selections are advisory. They do not change the primary assessment or E2E / PR Gate.

E2E guidance

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

Recommended E2E: onboard-repair, onboard-resume, cloud-onboard

Workflow run details

This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge.

@wscurran wscurran added area: onboarding Onboarding FSM, provider setup, sandbox launch, or first-run flow area: sandbox OpenShell sandbox lifecycle, runtime, config, or recovery bug-fix PR fixes a bug or regression integration: hermes Hermes integration behavior labels Aug 4, 2026
@wscurran

wscurran commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

✨ Thanks for the fix. This resolves the uninstall failure when the packaged gateway service has already been removed.


Related open issues:


Related open issues:

@apurvvkumaria

Copy link
Copy Markdown
Collaborator

Maintainer follow-up for the current PR revision:

  • Cleared the normal fork workflow authorization. CI / Pull Request passes, including all eight CLI shards, static checks, build and type checks, installer integration, plugin tests, and dependency audits.
  • DCO, commit verification, documentation writer receipt, Fern preview, macOS E2E, installer hash, and code scanning checks pass.
  • Reviewed all five changed files against the nine-category security checklist. Result: PASS with no findings. The opt-in only accepts the same NemoClaw-managed gateway's one-way packaged-service to standalone transition during full uninstall. External authority, reverse transitions, ordinary teardown, and provider credential mutation remain fail-closed. Regression tests cover the allowed uninstall path and the rejected credential-mutation path.
  • No unresolved review thread remains. The documentation heading requested by CodeRabbit is present.
  • Approved the selected fork E2E scope (onboard-repair and onboard-resume) after reviewing the diff and risk plan. E2E run 30998060273 is queued.

Remaining blockers:

  • The GPT-5.6 Terra and Nemotron advisor lanes failed on workflow attempt 2 because the advisor service returned HTTP 429. See Terra and Nemotron. These are external service failures, not failures in the PR. A third rerun is not justified.
  • The selected E2E run must pass.
  • GitHub still reports REVIEW_REQUIRED; an independent approval is required before merge.

No merge action will occur until every repository gate passes.

@apurvvkumaria

Copy link
Copy Markdown
Collaborator

The selected E2E run passed. Both advisor lanes failed again on the already-permitted second attempt because their inference service returned HTTP 429 during analysis; this is external rate limiting, not a PR test or implementation failure. I will not issue a third rerun without new evidence. The documented advisor-service failure and independent approval remain the merge blockers; there are no unresolved review threads.

@jyaunches
jyaunches merged commit f5892b8 into NVIDIA:main Aug 5, 2026
48 of 52 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: onboarding Onboarding FSM, provider setup, sandbox launch, or first-run flow area: sandbox OpenShell sandbox lifecycle, runtime, config, or recovery bug-fix PR fixes a bug or regression integration: hermes Hermes integration behavior v0.0.103 Release target

Projects

None yet

Development

Successfully merging this pull request may close these issues.

nemohermes uninstall blocked by gateway lifecycle authority mismatch packaged-service vs standalone

5 participants