fix(rebuild): warn when rebuild drops user-managed sandbox files - #5765
Merged
Conversation
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
laitingsheng
added
area: sandbox
Contributor
|
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:
📝 WalkthroughWalkthroughThis PR adds ChangesUser-managed file rebuild flow
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in the Show a code coverage summary of the most covered files.
TypeScript / code-coverage/cliThe overall coverage in the Show a code coverage summary of the most covered files.
Updated |
Contributor
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Contributor
Vitest E2E Scenario RecommendationRequired Vitest E2E scenarios: Dispatch required Vitest E2E scenarios:
Full Vitest E2E advisor summaryVitest E2E Scenario AdvisorBase: Required Vitest E2E scenarios
Optional Vitest E2E scenarios
Relevant changed files
|
Contributor
PR Review Advisor — No blocking findingsMerge posture: No blocking advisor findings Action checklist
Test follow-ups to resolve or justifyIf these cover changed behavior, prefer adding them in this PR; otherwise state why existing coverage is enough or link the follow-up.
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. |
laitingsheng
added
the
integration: dcode
…ifest contract 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: 2
🧹 Nitpick comments (1)
src/lib/state/sandbox.ts (1)
469-495: 🩺 Stability & Availability | 🔵 TrivialPlease run the sandbox lifecycle E2Es for this export change.
These helpers are now a public dependency for rebuild probing, so I’d still run
state-backup-restore-e2e,snapshot-commands-e2e, andrebuild-openclaw-e2ebefore merge to make sure the rebuild path stays clean. As per path instructions,src/lib/state/sandbox.ts“manages sandbox state (backup, restore, rebuild, snapshot)” and those selective E2Es are the recommended coverage.🤖 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/sandbox.ts` around lines 469 - 495, This export change in getSshConfig and sshArgs should be validated with the sandbox lifecycle E2Es before merge. Run state-backup-restore-e2e, snapshot-commands-e2e, and rebuild-openclaw-e2e to confirm the rebuild probing path still works cleanly and that the public sandbox helpers behave correctly under lifecycle coverage.Source: Path instructions
🤖 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.test.ts`:
- Around line 211-217: The temp SSH config test is only checking os.tmpdir() and
not the actual directory used by the probe. Update the test around loadProbe() /
probeUserManagedFiles to assert the intercepted mkdtempSync base path or spy on
createTempSshConfig, so it verifies the SSH config is created under the OS temp
directory rather than just rechecking Node’s tmpdir contract.
In `@src/lib/state/user-managed-files-probe.ts`:
- Around line 47-53: The probe command built in user-managed-files-probe should
not fail when none of the declared files exist; the current `[ -f ... ] &&
printf ...` chain returns a non-zero status on missing files, which then
triggers the error path in the probe result handling. Update the command
construction and the success check in the probe flow so “no matches” is treated
as a valid empty result, while still surfacing real shell errors; use the
probeCmd builder and the lines that throw on non-zero status to locate the fix.
---
Nitpick comments:
In `@src/lib/state/sandbox.ts`:
- Around line 469-495: This export change in getSshConfig and sshArgs should be
validated with the sandbox lifecycle E2Es before merge. Run
state-backup-restore-e2e, snapshot-commands-e2e, and rebuild-openclaw-e2e to
confirm the rebuild probing path still works cleanly and that the public sandbox
helpers behave correctly under lifecycle coverage.
🪄 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: 3016b988-ffba-44a6-b98c-33465f15c966
📒 Files selected for processing (7)
src/lib/actions/sandbox/rebuild-flow-helpers.test.tssrc/lib/actions/sandbox/rebuild-flow-helpers.tssrc/lib/agent/defs.test.tssrc/lib/agent/defs.tssrc/lib/state/sandbox.tssrc/lib/state/user-managed-files-probe.test.tssrc/lib/state/user-managed-files-probe.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/actions/sandbox/rebuild-flow-helpers.test.ts
- src/lib/actions/sandbox/rebuild-flow-helpers.ts
5 tasks
…clared Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/lib/state/user-managed-files-probe.ts (1)
48-67:⚠️ Potential issue | 🟠 MajorStill valid: “no files present” is treated as a probe failure.
Lines 48-52 build the remote command with
[ -f ... ] && printf ..., which exits1when a declared file is absent. If every declared file is missing,stdoutstays empty and Lines 61-67 throw, so the rebuild flow falls back to the generic probe-failed warning instead of staying quiet for the intended “nothing to warn about” case.🤖 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 48 - 67, The probe in user-managed-files-probe.ts is treating “no declared files exist” as an SSH failure instead of a valid empty result. Update the command built in the user-managed files probe flow so that the remote check in the probe path does not rely on [ -f ... ] returning a failure exit when files are missing; instead, have the probe return success with empty output when none of the declared paths exist, and only throw from the spawnSync/ssh handling when SSH itself fails. Keep the logic around probeCmd, spawnSync, and the stdout/status check in sync so the rebuild flow stays quiet for the no-files-present case.
🤖 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.
Duplicate comments:
In `@src/lib/state/user-managed-files-probe.ts`:
- Around line 48-67: The probe in user-managed-files-probe.ts is treating “no
declared files exist” as an SSH failure instead of a valid empty result. Update
the command built in the user-managed files probe flow so that the remote check
in the probe path does not rely on [ -f ... ] returning a failure exit when
files are missing; instead, have the probe return success with empty output when
none of the declared paths exist, and only throw from the spawnSync/ssh handling
when SSH itself fails. Keep the logic around probeCmd, spawnSync, and the
stdout/status check in sync so the rebuild flow stays quiet for the
no-files-present case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5bc34852-00d7-4f6a-8037-d9d3948da706
📒 Files selected for processing (5)
src/lib/actions/sandbox/rebuild-flow-helpers.test.tssrc/lib/agent/defs.test.tssrc/lib/agent/defs.tssrc/lib/state/user-managed-files-probe.test.tssrc/lib/state/user-managed-files-probe.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/lib/agent/defs.test.ts
- src/lib/actions/sandbox/rebuild-flow-helpers.test.ts
- src/lib/agent/defs.ts
…g on absent files Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
prekshivyas
previously requested changes
Jun 25, 2026
prekshivyas
left a comment
prekshivyas
left a comment
Collaborator
There was a problem hiding this comment.
Hermes is missing. The PR adds .env/.mcp.json to user_managed_files for OpenClaw and dcode, but agents/hermes/manifest.yaml has no declaration — the [] in defs.test.ts is the schema default, not an intentional choice. Hermes users keep .env for API keys (Anthropic, OpenAI, etc.) and will silently lose them on rebuild with no warning. This is the exact bug the PR claims to fix, left open for Hermes. Either add .env/.mcp.json to the Hermes manifest with the same declaration, or add a comment explaining why Hermes is excluded (different credential lifecycle, managed differently, etc.).
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
agents/hermes/manifest.yaml (1)
93-96: 🩺 Stability & Availability | 🔵 TrivialRun the Hermes rebuild E2E lanes for this manifest change.
This new
user_managed_filesentry feeds the rebuild-warning path, and the Hermes rebuild coverage only runs in nightly/workflow_dispatch. Please validaterebuild-hermes-e2eandrebuild-hermes-stale-base-e2ebefore merge. As per path instructions, "the rebuild warning behavior added in this PR should be validated via the existing Hermes rebuild E2E lanes rather than expecting it to run in PR CI."🤖 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 `@agents/hermes/manifest.yaml` around lines 93 - 96, Validate the new user_managed_files manifest entry by running the existing Hermes rebuild E2E coverage for rebuild-hermes-e2e and rebuild-hermes-stale-base-e2e, since the rebuild-warning path is only exercised in those nightly/workflow_dispatch lanes. Use the Hermes manifest handling around user_managed_files and the rebuild warning flow to confirm the .hermes/.env entry is picked up correctly before merging.Source: Path instructions
🤖 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 `@agents/hermes/manifest.yaml`:
- Around line 93-96: Validate the new user_managed_files manifest entry by
running the existing Hermes rebuild E2E coverage for rebuild-hermes-e2e and
rebuild-hermes-stale-base-e2e, since the rebuild-warning path is only exercised
in those nightly/workflow_dispatch lanes. Use the Hermes manifest handling
around user_managed_files and the rebuild warning flow to confirm the
.hermes/.env entry is picked up correctly before merging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 6870cc0b-a8a5-48ea-9767-fc945c47d584
📒 Files selected for processing (3)
agents/hermes/manifest.yamlsrc/lib/agent/defs.test.tssrc/lib/state/user-managed-files-probe.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/state/user-managed-files-probe.test.ts
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 28203973460
|
cv
approved these changes
Jun 25, 2026
Hadar301
pushed a commit
to Hadar301/NemoClaw-OpenShift
that referenced
this pull request
Jul 12, 2026
…DIA#5765) ## Summary Rebuild silently destroyed user-created `.env` / `.mcp.json` files inside Deep Agents sandboxes because the backup engine only enumerates `state_dirs` / `state_files` from the agent manifest, which intentionally omits user credential files. The CLI gave no hint that those files were about to be dropped. This change adds a declarative `user_managed_files` manifest field, probes the sandbox during rebuild for files declared but not preserved, and emits a clear warning naming the affected files. ## Related Issue Fixes NVIDIA#5750 ## Changes - Added `user_managed_files: string[]` to the agent manifest schema and an `AgentDefinition.userManagedFiles` accessor. - Declared `.env` and `.mcp.json` under `user_managed_files` in the LangChain Deep Agents Code manifest. - New `probeUserManagedFiles(sandboxName)` in `src/lib/state/sandbox.ts` — opens a short SSH session, runs a `test -f` probe per declared file, and reports which actually exist inside the live sandbox. - `backupSandboxStateForRebuild` now invokes the probe after `State backed up (…)` and prints a yellow `⚠ User-managed files in sandbox not preserved by rebuild: …` line followed by `Re-add them after rebuild, or manage them from the host.` when any declared file is present. - Probe is defensive: tolerates absent `userManagedFiles`, missing SSH config, and non-zero probe exit codes; rebuild flow swallows probe errors so the warning never blocks a rebuild. - Added `userManagedFiles: []` to existing `AgentDefinition` test fixtures required by the tightened type, plus assertions on the new field for OpenClaw, Hermes, and Deep Agents. - New `src/lib/actions/sandbox/rebuild-flow-helpers.test.ts` covers: warning emit, no-emit-when-none-present, no-emit-when-nothing-declared, staleRecovery short-circuit, swallowed probe errors. ## Type of Change - [x] 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) ## Verification - [x] PR description includes the DCO sign-off declaration and every commit appears as `Verified` in GitHub - [x] Git hooks passed during commit and push, or `npx prek run --from-ref main --to-ref HEAD` passes - [x] Targeted tests pass for changed behavior - [ ] Full `npm test` passes (broad runtime changes only) - [x] Tests added or updated for new or changed behavior - [x] No secrets, API keys, or credentials committed - [ ] Docs updated for user-facing behavior changes - [ ] `npm run docs` builds without warnings (doc changes only) - [ ] 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) --- Signed-off-by: Tinson Lai <tinsonl@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added `user_managed_files` support in agent manifests, exposed as `userManagedFiles`, with safe relative-path validation. * Added user-managed file probing to detect which declared files already exist in the sandbox. * Built-in agents now declare their default user-managed files (including `.env` / `.mcp.json`, and Hermes `.hermes/.env`). * **Bug Fixes** * Sandbox rebuild recovery now surfaces guidance when declared user-managed files aren’t preserved; recovery still succeeds even if probing can’t run. * **Tests** * Expanded test coverage for manifest validation, probing behavior, and rebuild warning flows. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Tinson Lai <tinsonl@nvidia.com> Co-authored-by: Carlos Villela <cvillela@nvidia.com>
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
5 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
Rebuild silently destroyed user-created
.env/.mcp.jsonfiles inside Deep Agents sandboxes because the backup engine only enumeratesstate_dirs/state_filesfrom the agent manifest, which intentionally omits user credential files. The CLI gave no hint that those files were about to be dropped. This change adds a declarativeuser_managed_filesmanifest field, probes the sandbox during rebuild for files declared but not preserved, and emits a clear warning naming the affected files.Related Issue
Fixes #5750
Changes
user_managed_files: string[]to the agent manifest schema and anAgentDefinition.userManagedFilesaccessor..envand.mcp.jsonunderuser_managed_filesin the LangChain Deep Agents Code manifest.probeUserManagedFiles(sandboxName)insrc/lib/state/sandbox.ts— opens a short SSH session, runs atest -fprobe per declared file, and reports which actually exist inside the live sandbox.backupSandboxStateForRebuildnow invokes the probe afterState backed up (…)and prints a yellow⚠ User-managed files in sandbox not preserved by rebuild: …line followed byRe-add them after rebuild, or manage them from the host.when any declared file is present.userManagedFiles, missing SSH config, and non-zero probe exit codes; rebuild flow swallows probe errors so the warning never blocks a rebuild.userManagedFiles: []to existingAgentDefinitiontest fixtures required by the tightened type, plus assertions on the new field for OpenClaw, Hermes, and Deep Agents.src/lib/actions/sandbox/rebuild-flow-helpers.test.tscovers: warning emit, no-emit-when-none-present, no-emit-when-nothing-declared, staleRecovery short-circuit, swallowed probe errors.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
user_managed_filessupport in agent manifests, exposed asuserManagedFiles, with safe relative-path validation..env/.mcp.json, and Hermes.hermes/.env).Bug Fixes
Tests