fix(snapshot): refuse restore --to existing destination; add --force opt-in#3796
Open
laitingsheng wants to merge 9 commits into
Open
fix(snapshot): refuse restore --to existing destination; add --force opt-in#3796laitingsheng wants to merge 9 commits into
laitingsheng wants to merge 9 commits into
Conversation
…opt-in Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
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 (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughAdds ChangesCross-sandbox snapshot restore with force/yes flags
Sequence Diagram(s)sequenceDiagram
participant CLI as SnapshotRestoreCommand
participant Action as SnapshotRestoreAction
participant OpenShell as OpenShell
participant FS as HostFilesystem
CLI->>Action: request restore {selector,to,force,yes}
Action->>Action: preflight (resolve selector, resolve source image, check targetExists)
alt target exists and not force
Action->>CLI: error "destination exists"
else force
Action->>CLI: interactive confirm (skippable by yes/ENV)
CLI->>Action: confirmation
Action->>OpenShell: sandbox delete <dst>
OpenShell-->>Action: delete outcome
Action->>FS: remove per-sandbox artifacts
Action->>Action: autoCreateSandboxFromSource(...)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
E2E Advisor RecommendationRequired E2E: Dispatch hint: Auto-dispatched E2E: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
…force destination Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
|
🌿 Preview your docs: https://nvidia-preview-pr-3796.docs.buildwithfern.com/nemoclaw |
…existing-destination Signed-off-by: Tinson Lai <tinsonl@nvidia.com> # Conflicts: # docs/reference/commands.md # src/lib/actions/sandbox/snapshot.ts # src/lib/commands/sandbox/snapshot.test.ts # src/lib/commands/sandbox/snapshot/restore.ts
…rtifacts on --force Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
![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
🧹 Nitpick comments (1)
docs/reference/commands.mdx (1)
838-842: ⚡ Quick winReplace colon with period or em dash.
Line 839 uses a colon between two independent clauses rather than to introduce a list. As per coding guidelines, colons should only introduce lists, not serve as general punctuation between clauses.
✏️ Suggested revision
Replace the colon with a period to form two sentences, or use an em dash if you want to preserve the close relationship between the clauses:
Option 1 (two sentences):
-To overwrite an existing destination, pass `--force`: the command deletes `dst`, then recreates it from the source's image and restores the snapshot into the fresh copy. +To overwrite an existing destination, pass `--force`. The command deletes `dst`, then recreates it from the source's image and restores the snapshot into the fresh copy.Option 2 (em dash):
-To overwrite an existing destination, pass `--force`: the command deletes `dst`, then recreates it from the source's image and restores the snapshot into the fresh copy. +To overwrite an existing destination, pass `--force` — the command deletes `dst`, then recreates it from the source's image and restores the snapshot into the fresh copy.As per coding guidelines: "Colons should only introduce a list. Flag colons used as general punctuation between clauses."
🤖 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/reference/commands.mdx` around lines 838 - 842, In the sentence that begins "To overwrite an existing destination, pass `--force`: the command deletes `dst`, then recreates it..." replace the colon after "`--force`" with either a period (split into two sentences) or an em dash to avoid using a colon between independent clauses; update the text that follows "`--force`" accordingly so it reads either "To overwrite an existing destination, pass `--force`. The command deletes `dst`, then recreates it..." or "To overwrite an existing destination, pass `--force` — the command deletes `dst`, then recreates it..." keeping the rest of the wording unchanged.
🤖 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/snapshot-restore-existing-dest.test.ts`:
- Around line 28-51: Refactor runCli to avoid command-injection by changing its
signature to accept args: string[] and use child_process.execFileSync (or
execFileSync) with CLI as the executable and the args array (preserving
encoding, timeout, and env merging including NEMOCLAW_* vars); update the
function body to handle the thrown error shape exactly as before but using
execFileSync's invocation, and update every call site in this test file that
passes a single string to runCli to instead pass an array of argument strings
(e.g., ["snapshot", "restore", ...]) so no shell interpolation is used.
---
Nitpick comments:
In `@docs/reference/commands.mdx`:
- Around line 838-842: In the sentence that begins "To overwrite an existing
destination, pass `--force`: the command deletes `dst`, then recreates it..."
replace the colon after "`--force`" with either a period (split into two
sentences) or an em dash to avoid using a colon between independent clauses;
update the text that follows "`--force`" accordingly so it reads either "To
overwrite an existing destination, pass `--force`. The command deletes `dst`,
then recreates it..." or "To overwrite an existing destination, pass `--force` —
the command deletes `dst`, then recreates it..." keeping the rest of the wording
unchanged.
🪄 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: 494004ba-7fa4-4781-9ada-07eeb065f46a
📒 Files selected for processing (5)
docs/reference/commands.mdxsrc/commands/sandbox/snapshot.test.tssrc/commands/sandbox/snapshot/restore.tssrc/lib/actions/sandbox/snapshot.tstest/snapshot-restore-existing-dest.test.ts
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 26140579903
|
…dest helper Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 26140953271
|
laitingsheng
added
v0.0.47
- src/commands/sandbox/snapshot/restore.ts: drop the local publicDisplay static (main moved this metadata into the central public-display-defaults registry); keep --force/--yes flags and examples. - src/lib/cli/public-display-defaults.ts: update sandbox:snapshot:restore flags string to include --force and --yes|-y. - src/lib/actions/sandbox/snapshot.ts: keep deleteSandboxForRestore from this branch, drop the orphaned probeDockerDriverGatewayRunning, and take main's probeGatewayMetadataHealth (uses isGatewayHealthy). Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
PR Review AdvisorRecommendation: blocked This is an automated advisory review. A human maintainer must make the final merge decision. Limitations: Review is based on trusted metadata, repository read-only inspection, and the provided diff; no scripts, tests, package-manager commands, or workflows were executed.; CI and E2E statuses may change after this review; current assessment is for head SHA fd2acb2 only.; No live sandbox was created or inspected, so filesystem, OpenShell registry, provider cleanup, and gateway behavior in real environments remain dependent on E2E results.; Review-thread state is taken from trusted GitHub metadata; the current file content no longer shows the stripAnsi import, but the thread is still reported unresolved. Full advisor summaryPR Review AdvisorBase: PR addresses the linked snapshot restore safety issue, but merge is blocked by pending CI, BLOCKED merge state, an unresolved review thread, missing required E2E for the latest head SHA, and monolith growth in sandbox lifecycle code. Gate status
🔴 Blockers
🟡 Warnings
🔵 Suggestions
Acceptance coverage
Security review
Test / E2E status
✅ What looks good
Review completeness
|
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 26201035574
|
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 26201767027
|
12 tasks
laitingsheng
added
v0.0.48
12 tasks
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
2 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
snapshot restore --to <dst>used to overlay onto an existing destination's filesystem silently. Now it refuses by default;--forcedeletes the destination and recreates it from the source's image, with interactive confirmation unless--yesorNEMOCLAW_NON_INTERACTIVE=1is set.Related Issue
Fixes #3756
Changes
snapshot restore --to <dst>whendstalready exists. The refusal fires before source-side preflight, so the user always sees the precise "destination exists" error even when the source is also missing or its image cannot be resolved.--force(delete-then-recreate) and--yes/-y(skip prompt) flags to the typedrunSandboxSnapshot({ kind: "restore", ... })action and to the oclif command.dstintact.openshell sandbox delete, then performs the destination-only cleanupssandboxDestroydoes —/tmp/nemoclaw-services-<dst>, per-sandbox messaging providers (<dst>-telegram-bridge,-discord-bridge,-slack-bridge,-slack-app,-wechat-bridge), and shields state — before removing the registry entry. Host-shared cleanups (Ollama proxy, Ollama model unload, host services, gateway teardown) are intentionally skipped because they would also affect the source sandbox we are about to clone from.docs/reference/commands.mdxwith the new flags, the refuse-by-default behaviour, and a fix to the long-standing inaccurate "exact or prefix timestamp" claim (findBackupmatches exact timestamps only).snapshot helpfallback usage text inrunSandboxSnapshotso it lists--force/--yes.test/snapshot-restore-existing-dest.test.tscovering refuse-default; refuse-default-even-when-source-image-broken (locks the ordering);--force --yesreaches the delete step;--forceunderNEMOCLAW_NON_INTERACTIVE=1skips the prompt; no-delete when no snapshot, bad selector, or unresolvable source image. The--forceassertions verify the delete step ran; the subsequentsandbox createis mocked to exit non-zero rather than mocking the full create stream.Type of Change
Verification
npx prek run --all-filespassesnpm testpasses (one pre-existing flake ine2e-scenario-frameworkunrelated to this PR)make docsbuilds without warnings (doc changes only)Signed-off-by: Tinson Lai tinsonl@nvidia.com
Summary by CodeRabbit
Documentation
New Features
Tests