fix(backup-all): catch orphan-manifest errors without swallowing real failures - #5819
Merged
Conversation
… failures Closes #5734. Supersedes #5740. When `loadAgent()` throws for a sandbox whose agent manifest is missing (orphan from a previous higher-version install), the prior backup-all loop aborted the entire batch and blocked the installer's pre-upgrade backup step. #5740 attempted this fix by wrapping backupSandboxState() in a broad try/catch and counting any throw as 'skipped'. That swallowed real failures (disk full mid tar write, SSH timeout, permission denied on the backup directory, programming bugs in the backup logic) and exited 0, so the installer would march forward with the upgrade and the restore path would later read a manifest pointing at a corrupt or absent backup. This commit ships the same outcome with a narrow catch: it only treats `Agent '...' not found` errors (the exact pattern thrown by loadAgent in src/lib/agent/defs.ts:367) as recoverable orphans. Anything else re-throws so the installer aborts as it did before any wrapping existed. Also renames the log line to 'Skipped (orphan manifest)' so the batch summary distinguishes that case from 'Skipping (not running)' above it. Adds a regression test that backupAll() rejects on a non-orphan error (EACCES) instead of counting it as skipped. Keeps the orphan manifest tests from #5740. Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
cjagwani
added
area: cli
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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesBackup-all orphan manifest handling
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 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
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: Auto-dispatched E2E: 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. |
![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/actions/maintenance.ts`:
- Around line 89-90: The orphan-manifest matcher in maintenance handling is too
broad and catches more than the actual loadAgent() orphan case. Narrow the regex
in maintenance.ts to match the exact loadAgent() error contract from loadAgent()
in src/lib/agent/defs.ts, including the manifest path portion after Agent
'<name>' not found:. Keep non-matching errors flowing to the throw path so
unrelated failures still abort the backup instead of being skipped.
🪄 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: 6e182319-41f0-4bdf-bb4b-990af2b83f5a
📒 Files selected for processing (2)
src/lib/actions/maintenance.test.tssrc/lib/actions/maintenance.ts
Advisor PRA-2 + CodeRabbit on #5819 both pointed out the orphan-manifest regex `^Agent '[^']*' not found` is looser than the actual loadAgent() throw shape at src/lib/agent/defs.ts:365-372 (`Agent '<name>' not found: <manifestPath>`). A bare `Agent '...' not found` could plausibly surface from a different layer (registry lookup, manifest index, future code) and should still abort the backup batch instead of being silently skipped. Tighten the regex to require the `: <path>` suffix that loadAgent emits, document the source-line anchor in a code comment, and update the second orphan-skip test fixture so it reflects the real shape. Add a regression test that a bare `Agent '<name>' not found` message without the suffix is re-thrown, so a future widening of the matcher fails CI. Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
…PRA-1) Advisor PRA-1 on #5819 asked for the five source-of-truth dimensions to be documented for the localized orphan-manifest catch: invalid state, source boundary, source-fix constraint, regression test, and removal condition. Expand the inline comment to name each dimension explicitly. No code behavior change — pure documentation pass so the catch's source boundary is auditable at the site instead of in a separate doc. Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
…aml (PRA-1) Advisor PRA-1 on #5819 (033462d) asked the matcher to specifically identify the manifest.yaml path so a future error that wraps `Agent '...' not found:` with a different artifact (binary, config, registry entry) keeps aborting the batch instead of being treated as an orphan manifest skip. loadAgent at src/lib/agent/defs.ts:367 emits the path via `path.join(AGENTS_DIR, name, "manifest.yaml")`, so the path always ends in `/manifest.yaml`. Tighten the regex to that exact suffix and add a regression test that a non-manifest-yaml path is re-thrown. Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 28199869787
|
cjagwani
added a commit
that referenced
this pull request
Jun 25, 2026
Address advisor PRA-2 PRA-3 PRA-5 on #5455. PRA-2 (required): wipeSandboxState() lived inside the destroy.ts monolith. Move it to a sibling module src/lib/actions/sandbox/wipe-state.ts so the wipe contract has its own surface, the destroy hotspot stays focused on lifecycle orchestration, and future evolution of the wipe logic (e.g. typed manifest schema) doesn't drag the destroy file around. Re-export wipeSandboxState and WipeSandboxStateDeps from destroy.ts so callers (tests, downstream code) keep working. PRA-3 (warning): the path-escape boundary check used Node's host-OS `path.resolve()` + `path.sep`. The remote sandbox shell that executes the rm-rf script is Linux, but the CLI can run on Windows or macOS, so the boundary semantics could diverge from what the sandbox actually does (e.g. `\` separators or different normalization on Windows). Switch to `path.posix` so the boundary check matches the sandbox shell that will run the script. PRA-5 (warning): expand the wipeSandboxState docstring to name the five source-of-truth dimensions explicitly: invalid state, source boundary, source-fix constraint, regression test, and removal condition. Same pattern that converged the advisor on #5819. No behavior change for the common in-sandbox path (configPaths.dir is already a POSIX absolute path); the POSIX switch hardens the check against future Windows-host callers. Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
12 tasks
cv
approved these changes
Jun 26, 2026
cv
added a commit
that referenced
this pull request
Jun 27, 2026
…5455) ## Summary `nemoclaw <name> destroy` deleted the sandbox pod but left its per-sandbox persistent volume intact, so re-onboarding with the same name resurrected old workspace files (`USER.md`, `SOUL.md`, ...). This makes destroy actually wipe that persistent state, restoring the documented "clean workspace on re-onboard" contract. ## Related Issue Fixes #5449 ## Changes - Add `wipeSandboxState()` in `src/lib/actions/sandbox/destroy.ts`: while the sandbox is still live (before `openshell sandbox delete`), it removes the agent-manifest state dirs/files plus discovered multi-agent `workspace-*` dirs via `openshell sandbox exec -- sh -c 'rm -rf ...'`. This is the inverse of `backupSandboxState`, so it targets exactly the set snapshot/backup treat as durable state. - Call `wipeSandboxState()` from `destroySandbox()` after the confirmation gate and before the delete. - Best-effort and non-fatal: a non-live sandbox (e.g. gateway down) warns and lets destroy proceed, mirroring the existing `removeShieldsState` pattern from #3114. - Add `test/destroy-wipe-sandbox-state.test.ts` reproducing the issue: asserts the wipe targets the `workspace/` dir under the agent config dir, includes `workspace-*` (#1260), passes `ignoreError`, and never throws on a failed exec. ### Root cause `openshell sandbox delete` tears down the pod but the workspace lives in a k3s local-path PVC keyed by sandbox name (inside the shared `openshell-cluster-nemoclaw` Docker volume), which `delete` leaves intact. `openshell sandbox delete --help` exposes no storage-wipe flag, and the cluster volume is only removed on opt-in gateway teardown (#2166). Re-onboarding with the same name rebinds the PVC. Same bug class as #3114. ## 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 - [ ] `npx prek run --all-files` passes - [ ] `npm test` passes - [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 - [ ] `make 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) <!-- Verification notes: this branch builds (`npm run build:cli`) and type-checks (`npm run typecheck:cli`) cleanly; `biome check` is clean on both changed files; and the destroy suites (`test/destroy-wipe-sandbox-state.test.ts`, `test/destroy-cleanup-sandbox-services.test.ts`, `src/lib/domain/sandbox/destroy.test.ts`) pass deterministically. The full `npm test` and `npx prek run --all-files` were not run because the local working tree carries heavy unrelated modifications and the full suite is flaky in this environment; behavior was verified via the targeted suites above. No docs change is needed — the fix makes code match the existing contract in docs/manage-sandboxes/backup-restore.md. --> ## Advisor state 1 required finding that contradicts the advisor's own original required PRA-5 from this PR. PRA-5 required the wipe run AFTER gateway-select-before-delete (the bug we fixed). A later round's PRA-2 asks the wipe defer until after delete proves destroy can complete — physically impossible because `sandbox delete` unmounts the PVC and the in-sandbox `rm -rf` can no longer reach it. The code keeps PRA-5's ordering; the contradiction is named at `src/lib/actions/sandbox/destroy.ts:386-389`. Plus 3 recurring advisory warnings (source-of-truth recursion pattern — same plateau as #5712 and #5819). Justifications in `wipeSandboxState()` docstring. --- Signed-off-by: jason-ma-nv <jama@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes * **Bug Fixes** * Improved sandbox destruction to best-effort wipe persisted agent state before deletion, including the agent manifest state plus `workspace` and `workspace-*` multi-agent data. * Added safer cleanup execution: failures are non-blocking, and warnings are logged (e.g., “Could not wipe workspace state”). * Strengthened security checks to prevent path escaping when generating the cleanup command. * **Tests** * Expanded regression coverage for correct exec invocation, warning behavior on failures, and secure `rm -rf` script generation (including `cd` boundary and traversal/absolute-path protections). <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Jason Ma <jama@nvidia.com> Signed-off-by: Charan Jagwani <cjagwani@nvidia.com> Co-authored-by: Test User <test@example.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: Carlos Villela <cvillela@nvidia.com> Co-authored-by: cjagwani <cjagwani@nvidia.com>
24 tasks
Hadar301
pushed a commit
to Hadar301/NemoClaw-OpenShift
that referenced
this pull request
Jul 12, 2026
… failures (NVIDIA#5819) ## Summary Catches the `loadAgent()` orphan-manifest case from NVIDIA#5734 so the pre-upgrade backup loop survives a sandbox whose agent manifest is missing, **without** silently swallowing real failures (disk full, SSH timeout, permission denied, programming bugs) the way NVIDIA#5740's broad catch did. ## Related Issue Closes NVIDIA#5734. **Supersedes NVIDIA#5740**. ## Why a new PR NVIDIA#5740 (kagura-agent) implemented the right outcome with a broad try/catch that swallowed every error and counted it as 'skipped', then exited 0. That lets the installer march forward with an upgrade after a real disk-full or SSH-timeout failure, and the restore path later reads a manifest pointing at a corrupt or absent backup. Data loss scenario. That PR is on a fork I cannot push to, so this is a fresh PR on the NVIDIA upstream so the PR Advisor and full CI run. ## Changes - `src/lib/actions/maintenance.ts`: narrow try/catch on `backupSandboxState()` that only matches the exact orphan-manifest pattern (`Agent '...' not found`) thrown by `loadAgent()` at `src/lib/agent/defs.ts:367`. Anything else re-throws so the installer aborts as it did before any wrapping existed. - `src/lib/actions/maintenance.test.ts`: new test file with 3 cases: - Orphan manifest in one sandbox does not abort the batch - Orphan manifest is counted as `skipped`, not `failed` - **Non-orphan errors (e.g. EACCES) are re-thrown so the installer aborts the upgrade** ## Verification - `npx vitest run src/lib/actions/maintenance.test.ts` — 3/3 pass - The narrow regex `/^Agent '[^']*' not found/` matches the exact error shape from `loadAgent` at `defs.ts:367` ## 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) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved batch backup reliability: if a sandbox fails due to a missing/orphan agent manifest (matching the expected “not found: <path>…manifest.yaml” pattern), the batch now logs a warning, marks the sandbox as skipped, updates counters, and continues processing remaining sandboxes. * Non-matching or unrecoverable errors still abort the batch as before. * **Tests** * Expanded maintenance test coverage for partial success/continuation, skipped behavior, and correct abort behavior for non-matching “not found” cases. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
Hadar301
pushed a commit
to Hadar301/NemoClaw-OpenShift
that referenced
this pull request
Jul 12, 2026
…5449) (NVIDIA#5455) ## Summary `nemoclaw <name> destroy` deleted the sandbox pod but left its per-sandbox persistent volume intact, so re-onboarding with the same name resurrected old workspace files (`USER.md`, `SOUL.md`, ...). This makes destroy actually wipe that persistent state, restoring the documented "clean workspace on re-onboard" contract. ## Related Issue Fixes NVIDIA#5449 ## Changes - Add `wipeSandboxState()` in `src/lib/actions/sandbox/destroy.ts`: while the sandbox is still live (before `openshell sandbox delete`), it removes the agent-manifest state dirs/files plus discovered multi-agent `workspace-*` dirs via `openshell sandbox exec -- sh -c 'rm -rf ...'`. This is the inverse of `backupSandboxState`, so it targets exactly the set snapshot/backup treat as durable state. - Call `wipeSandboxState()` from `destroySandbox()` after the confirmation gate and before the delete. - Best-effort and non-fatal: a non-live sandbox (e.g. gateway down) warns and lets destroy proceed, mirroring the existing `removeShieldsState` pattern from NVIDIA#3114. - Add `test/destroy-wipe-sandbox-state.test.ts` reproducing the issue: asserts the wipe targets the `workspace/` dir under the agent config dir, includes `workspace-*` (NVIDIA#1260), passes `ignoreError`, and never throws on a failed exec. ### Root cause `openshell sandbox delete` tears down the pod but the workspace lives in a k3s local-path PVC keyed by sandbox name (inside the shared `openshell-cluster-nemoclaw` Docker volume), which `delete` leaves intact. `openshell sandbox delete --help` exposes no storage-wipe flag, and the cluster volume is only removed on opt-in gateway teardown (NVIDIA#2166). Re-onboarding with the same name rebinds the PVC. Same bug class as NVIDIA#3114. ## 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 - [ ] `npx prek run --all-files` passes - [ ] `npm test` passes - [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 - [ ] `make 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) <!-- Verification notes: this branch builds (`npm run build:cli`) and type-checks (`npm run typecheck:cli`) cleanly; `biome check` is clean on both changed files; and the destroy suites (`test/destroy-wipe-sandbox-state.test.ts`, `test/destroy-cleanup-sandbox-services.test.ts`, `src/lib/domain/sandbox/destroy.test.ts`) pass deterministically. The full `npm test` and `npx prek run --all-files` were not run because the local working tree carries heavy unrelated modifications and the full suite is flaky in this environment; behavior was verified via the targeted suites above. No docs change is needed — the fix makes code match the existing contract in docs/manage-sandboxes/backup-restore.md. --> ## Advisor state 1 required finding that contradicts the advisor's own original required PRA-5 from this PR. PRA-5 required the wipe run AFTER gateway-select-before-delete (the bug we fixed). A later round's PRA-2 asks the wipe defer until after delete proves destroy can complete — physically impossible because `sandbox delete` unmounts the PVC and the in-sandbox `rm -rf` can no longer reach it. The code keeps PRA-5's ordering; the contradiction is named at `src/lib/actions/sandbox/destroy.ts:386-389`. Plus 3 recurring advisory warnings (source-of-truth recursion pattern — same plateau as NVIDIA#5712 and NVIDIA#5819). Justifications in `wipeSandboxState()` docstring. --- Signed-off-by: jason-ma-nv <jama@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes * **Bug Fixes** * Improved sandbox destruction to best-effort wipe persisted agent state before deletion, including the agent manifest state plus `workspace` and `workspace-*` multi-agent data. * Added safer cleanup execution: failures are non-blocking, and warnings are logged (e.g., “Could not wipe workspace state”). * Strengthened security checks to prevent path escaping when generating the cleanup command. * **Tests** * Expanded regression coverage for correct exec invocation, warning behavior on failures, and secure `rm -rf` script generation (including `cd` boundary and traversal/absolute-path protections). <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Jason Ma <jama@nvidia.com> Signed-off-by: Charan Jagwani <cjagwani@nvidia.com> Co-authored-by: Test User <test@example.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: Carlos Villela <cvillela@nvidia.com> Co-authored-by: cjagwani <cjagwani@nvidia.com>
This was referenced Jul 13, 2026
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
3 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
Catches the
loadAgent()orphan-manifest case from #5734 so the pre-upgrade backup loop survives a sandbox whose agent manifest is missing, without silently swallowing real failures (disk full, SSH timeout, permission denied, programming bugs) the way #5740's broad catch did.Related Issue
Closes #5734. Supersedes #5740.
Why a new PR
#5740 (kagura-agent) implemented the right outcome with a broad try/catch that swallowed every error and counted it as 'skipped', then exited 0. That lets the installer march forward with an upgrade after a real disk-full or SSH-timeout failure, and the restore path later reads a manifest pointing at a corrupt or absent backup. Data loss scenario.
That PR is on a fork I cannot push to, so this is a fresh PR on the NVIDIA upstream so the PR Advisor and full CI run.
Changes
src/lib/actions/maintenance.ts: narrow try/catch onbackupSandboxState()that only matches the exact orphan-manifest pattern (Agent '...' not found) thrown byloadAgent()atsrc/lib/agent/defs.ts:367. Anything else re-throws so the installer aborts as it did before any wrapping existed.src/lib/actions/maintenance.test.ts: new test file with 3 cases:skipped, notfailedVerification
npx vitest run src/lib/actions/maintenance.test.ts— 3/3 pass/^Agent '[^']*' not found/matches the exact error shape fromloadAgentatdefs.ts:367Type of Change
Summary by CodeRabbit
Bug Fixes
Tests