fix(upgrade): protect legacy sandbox state - #7107
Closed
ericksoa wants to merge 1 commit into
Closed
Conversation
Contributor
📝 WalkthroughWalkthroughChangesLegacy sandbox upgrade preservation
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested labels: Suggested reviewers: Sequence Diagram(s)Strict pre-upgrade backup validationsequenceDiagram
participant Installer
participant backupAll
participant Sandbox
Installer->>backupAll: run strict pre-upgrade backup
backupAll->>Sandbox: probe top-level /sandbox entries
Sandbox-->>backupAll: return unpreserved paths or probe error
backupAll-->>Installer: continue, warn, or fail upgrade
Legacy OpenClaw restoresequenceDiagram
participant SandboxFinalization
participant restoreRecreatedSandboxState
participant restoreStateFile
participant mergeOpenClawRestoredConfig
SandboxFinalization->>restoreRecreatedSandboxState: restore pre-upgrade backup
restoreRecreatedSandboxState->>restoreStateFile: pass restoreMissingManagedChannels
restoreStateFile->>mergeOpenClawRestoredConfig: merge restored configuration
mergeOpenClawRestoredConfig-->>restoreStateFile: retain fresh blocks and restore missing managed channels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
|
🌿 Preview your docs: https://nvidia-preview-pr-7107.docs.buildwithfern.com/nemoclaw |
Contributor
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 |
![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
🤖 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/state/user-managed-files-probe.ts`:
- Around line 100-107: Restrict the `${configEntry}-data` exclusion in the probe
logic to the explicit legacy OpenClaw/Hermes agent or manifest contract, rather
than applying it to every agent. Keep the canonical config entry managed for all
applicable agents, and add a regression test proving a non-legacy agent’s
sibling data directory remains unmanaged and protected from deletion.
🪄 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: 79b6d0a1-bcca-4ae4-8813-ae1db62bf3f7
📒 Files selected for processing (15)
docs/manage-sandboxes/backup-restore.mdxdocs/manage-sandboxes/update-sandboxes.mdxsrc/lib/actions/maintenance.test.tssrc/lib/actions/maintenance.tssrc/lib/onboard/created-sandbox-finalization.test.tssrc/lib/onboard/created-sandbox-finalization.tssrc/lib/state/openclaw-config-merge.test.tssrc/lib/state/openclaw-config-merge.tssrc/lib/state/openclaw-config-restore-input.tssrc/lib/state/sandbox-recreated-openclaw-restore.test.tssrc/lib/state/sandbox.tssrc/lib/state/state-file-restore.tssrc/lib/state/user-managed-files-probe.test.tssrc/lib/state/user-managed-files-probe.tstest/onboard-installer-restore-intent.test.ts
Comment on lines
+100
to
+107
| const configDir = path.posix.normalize(agent.configPaths.dir.replace(/\\/g, "/")); | ||
| if (path.posix.dirname(configDir) === USER_MANAGED_FILES_BASE) { | ||
| const configEntry = path.posix.basename(configDir); | ||
| managedEntries.add(configEntry); | ||
| // Older OpenClaw and Hermes images used a sibling durable data directory | ||
| // behind the canonical config path. It belongs to the same manifest-backed | ||
| // state and must not be reported as arbitrary root data. | ||
| managedEntries.add(`${configEntry}-data`); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not exclude <configEntry>-data for every agent.
Line 107 treats this sibling as managed even though the comment only establishes ownership for legacy OpenClaw and Hermes images. For another agent, user data with that name bypasses the strict gate and may be deleted during recreation.
Derive exclusions from an explicit agent or manifest contract, and add a non-legacy-agent regression test.
🤖 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/state/user-managed-files-probe.ts` around lines 100 - 107, Restrict
the `${configEntry}-data` exclusion in the probe logic to the explicit legacy
OpenClaw/Hermes agent or manifest contract, rather than applying it to every
agent. Keep the canonical config entry managed for all applicable agents, and
add a regression test proving a non-legacy agent’s sibling data directory
remains unmanaged and protected from deletion.
Contributor
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
3 warnings · 0 suggestionsWarningsWarnings do not block.
|
ericksoa
added
v0.0.86
bug-fix
Contributor
Author
|
Closing in favor of #7093, which owns the issue-scoped warning fix and is already exact-head green and approved. |
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
1 participant
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
Protect legacy sandbox state during installer-driven upgrades.
The upgrade now recovers sanitized managed-channel configuration that legacy registry rows cannot regenerate and stops before recreation when top-level
/sandboxdata falls outside the snapshot contract.Related Issue
Fixes #7073
Changes
openclaw.jsononly during installer-driven legacy recreation.Freshly generated channel configuration still wins, and ordinary rebuilds retain the existing do-not-resurrect behavior.
/sandboxentries after a successful strict pre-upgrade snapshot and fail before destructive recreation when unpreserved paths are present.NEMOCLAW_ACCEPT_UNPRESERVED_SANDBOX_DATA=1explicitly acknowledges their loss without adding arbitrary, potentially credential-bearing data to the snapshot.Root Cause and Detection Gap
Legacy registry rows predate persisted messaging intent, so recreation generated a fresh config without Slack and the normal selective merge correctly interpreted that omission as a removed managed channel.
Separately, the snapshot contract preserves declared agent state but did not surface arbitrary top-level
/sandboxpaths before the installer deleted the old sandbox.Existing coverage proved normal rebuild ownership and manifest-declared restore behavior, but did not exercise an installer-selected legacy backup with missing messaging intent or undeclared top-level sandbox data.
The new tests cover both installer boundaries while retaining the ordinary rebuild behavior.
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 unavailablevitestCLI projects: 79 passed; installer integration: 2 passed;npm run typecheckpassed.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)npm run docscompleted with 0 errors and the repository's 2 current Fern warnings.Signed-off-by: Aaron Erickson aerickson@nvidia.com
Summary by CodeRabbit
New Features
/sandboxdata before installer-driven upgrades.NEMOCLAW_ACCEPT_UNPRESERVED_SANDBOX_DATA=1.Documentation