fix(uninstall): add --destroy-user-data flag to purge preserved data#5784
fix(uninstall): add --destroy-user-data flag to purge preserved data#5784laitingsheng wants to merge 8 commits into
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:
📝 WalkthroughWalkthroughAdded ChangesUninstall destroy-user-data flag
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 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 in the branch is 96%. Coverage data for the branch is not yet available. Show a code coverage summary of the most covered files.
TypeScript / code-coverage/cliThe overall coverage in the branch is 47%. Coverage data for the branch is not yet available. Show a code coverage summary of the most covered files.
Updated |
laitingsheng
added
area: cli
|
🌿 Preview your docs: https://nvidia-preview-pr-5784.docs.buildwithfern.com/nemoclaw |
E2E Advisor RecommendationRequired E2E: Dispatch hint: Auto-dispatched E2E: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Vitest E2E Scenario RecommendationRequired Vitest E2E scenarios: None Full Vitest E2E advisor summaryVitest E2E Scenario AdvisorBase: Required Vitest E2E scenarios
Optional Vitest E2E scenarios
Relevant changed files
|
PR Review Advisor — No blocking findingsMerge posture: No blocking advisor findings 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/manage-sandboxes/lifecycle.mdx`:
- Around line 253-258: Add `--gateway <name>` to the uninstall quick-reference
and the hosted-script guidance in lifecycle.mdx so the documented uninstall
surface matches the actual command. Update the relevant uninstall section
entries alongside the existing `--yes`, `--keep-openshell`, `--delete-models`,
and `--destroy-user-data` flags, and make sure the `gateway` option is
explicitly mentioned wherever the uninstall behavior is summarized so users can
remove a non-default gateway correctly.
In `@src/lib/actions/uninstall/run-plan.test.ts`:
- Around line 1002-1004: The uninstall run-plan test is checking the wrong log
text and can pass even if the non-interactive preserve branch runs; update the
assertion in run-plan.test to target the actual preserve notice emitted by
resolvePreserveSet(), specifically verifying that no log line starts with
“Preserving ” rather than looking for “preserved:”. Keep the existing stateDir
and destroy-user-data expectations, and adjust the log assertion to match the
preserve path used by resolvePreserveSet.
🪄 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: 3077d88a-d34d-4a72-9dd4-34aafd42d503
📒 Files selected for processing (7)
docs/manage-sandboxes/lifecycle.mdxdocs/reference/commands-nemohermes.mdxdocs/reference/commands.mdxsrc/commands/internal/uninstall/run-plan.tssrc/lib/actions/uninstall/run-plan.test.tssrc/lib/actions/uninstall/run-plan.tsuninstall.sh
…o preserve Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/root-help.ts`:
- Around line 76-86: The help text in root-help’s banner is describing --yes as
removing preserved ~/.nemoclaw/ user data, which no longer matches the intended
safe default. Update the help strings built in the help-listing logic so --yes
is presented as non-destructive and the explicit destructive behavior is
attributed to --destroy-user-data, keeping --keep-user-data as the preservation
option. Make sure the descriptions around the root-help output stay consistent
with the CLI flags shown in this section.
In `@src/lib/actions/uninstall/run-plan.ts`:
- Around line 821-824: The `runPlan` flow in `run-plan.ts` is treating
`options.assumeYes` as permission to purge preserved `~/.nemoclaw/` data, which
is too destructive by default. Update the `assumeYes` branch so `--yes` only
confirms the uninstall plan and does not delete user data unless
`--destroy-user-data` (or its env equivalent) is explicitly enabled, and keep
the destructive path gated behind the existing destroy-user-data checks.
🪄 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: 36e72db2-0942-4028-b110-e4331238fdaf
📒 Files selected for processing (9)
docs/manage-sandboxes/lifecycle.mdxdocs/reference/commands-nemohermes.mdxdocs/reference/commands.mdxsrc/commands/internal/uninstall/run-plan.tssrc/lib/actions/root-help.tssrc/lib/actions/uninstall/run-plan.test.tssrc/lib/actions/uninstall/run-plan.tstest/uninstall.test.tsuninstall.sh
💤 Files with no reviewable changes (2)
- uninstall.sh
- test/uninstall.test.ts
✅ Files skipped from review due to trivial changes (1)
- docs/reference/commands.mdx
🚧 Files skipped from review as they are similar to previous changes (4)
- docs/manage-sandboxes/lifecycle.mdx
- src/commands/internal/uninstall/run-plan.ts
- docs/reference/commands-nemohermes.mdx
- src/lib/actions/uninstall/run-plan.test.ts
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
LGTM once coderabbit/advisor issues are addressed
…e-aware Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
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/uninstall.test.ts`:
- Around line 35-54: The runUninstall() helper is inheriting the full parent
environment, so uninstall control flags like
NEMOCLAW_UNINSTALL_DESTROY_USER_DATA can leak into tests and make the
preserve-by-default behavior flaky. Update runUninstall() to build the child
process env from a sanitized base instead of spreading process.env, then
explicitly add only the needed values (HOME, PATH, NEMOCLAW_NODE, TMPDIR) and
finally overlay extraEnv so test-specific overrides still work.
🪄 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: 3442b7eb-1748-4c55-87e0-097ecba0789c
📒 Files selected for processing (9)
docs/manage-sandboxes/lifecycle.mdxdocs/reference/commands-nemohermes.mdxdocs/reference/commands.mdxsrc/commands/internal/uninstall/run-plan.tssrc/lib/actions/root-help.tssrc/lib/actions/uninstall/run-plan.test.tssrc/lib/actions/uninstall/run-plan.tstest/uninstall.test.tsuninstall.sh
✅ Files skipped from review due to trivial changes (3)
- uninstall.sh
- docs/reference/commands-nemohermes.mdx
- docs/manage-sandboxes/lifecycle.mdx
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/reference/commands.mdx
Selective E2E Results —
|
| Job | Result |
|---|---|
| gpu-e2e | ⏭️ skipped |
…-data refs Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
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 `@docs/reference/commands.mdx`:
- Line 1986: The documentation sentence in the `--yes` description violates the
style guide by using a colon to join two clauses and by packing multiple
sentences onto one line. Rewrite the affected text in `commands.mdx` so `--yes`
is described with separate single-sentence lines, and make sure the colon only
introduces a list or is removed in favor of plain sentence structure.
🪄 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: 8eccda30-a98e-45d2-a4c5-8477bc21a703
📒 Files selected for processing (4)
docs/manage-sandboxes/lifecycle.mdxdocs/reference/commands-nemohermes.mdxdocs/reference/commands.mdxtest/uninstall.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- docs/reference/commands-nemohermes.mdx
- docs/manage-sandboxes/lifecycle.mdx
- test/uninstall.test.ts
Selective E2E Results —
|
| Job | Result |
|---|---|
| gpu-e2e | ⏭️ skipped |
…st env scrub Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
…st if-statement Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Selective E2E Results —
|
| Job | Result |
|---|---|
| gpu-e2e | ⏭️ skipped |
3 participants
Summary
nemoclaw uninstall --yesis non-destructive by design — it preserves~/.nemoclaw/rebuild-backups/,~/.nemoclaw/backups/, and~/.nemoclaw/sandboxes.json, leaving the state directory non-empty. The only escape hatch today is theNEMOCLAW_UNINSTALL_DESTROY_USER_DATA=1env var, which is not discoverable from--helpor the usage banner. Add a first-class--destroy-user-dataCLI flag that mirrors the env-var semantics so users who want a clean uninstall have a visible flag to reach for, without changing the safe--yesdefault.Related Issue
Fixes #5780
Changes
src/commands/internal/uninstall/run-plan.ts: declare adestroy-user-databoolean flag and forward it intoUninstallRunOptions.destroyUserData.src/lib/actions/uninstall/run-plan.ts: extendUninstallRunOptionswithdestroyUserData?: boolean;resolvePreserveSetnow checks the flag before the env var and logs--destroy-user-data set; purging user data under ~/.nemoclaw/.when it triggers; the non-interactive preserve notice now mentions both the flag and the env var. Note: the flag is opt-in precisely to avoid changing the existing safe--yesdefault — but the flag itself is destructive, so a user (or script) invoking it without intent will lose the preserved user data.uninstall.sh: extend the usage banner with the new flag.src/lib/actions/uninstall/run-plan.test.ts: four new tests — non-TTY plus--yesplus--destroy-user-datapurges, TTY plus flag purges without prompting, flag takes precedence overNEMOCLAW_UNINSTALL_DESTROY_USER_DATA=1, and the preserve hint mentions both the flag and the env var.docs/manage-sandboxes/lifecycle.mdx,docs/reference/commands.mdx,docs/reference/commands-nemohermes.mdx: add the new flag to the uninstall flags table, the usage line, and the user-data decision matrix.Type of Change
Verification
Verifiedin GitHubnpx prek run --from-ref main --to-ref HEADpassesnpm testpasses (broad runtime changes only)npm run docsbuilds without warnings (doc changes only)Signed-off-by: Tinson Lai tinsonl@nvidia.com
Summary by CodeRabbit
New Features
--destroy-user-dataoption to uninstall flows, including the CLI help and reference usage.~/.nemoclaw/user data.Bug Fixes
--yesonly confirms; preserved~/.nemoclaw/data remains unless--destroy-user-data(or the documented env override) is provided.Documentation
Tests