fix(backup): preserve Shields during backup-all - #7557
Conversation
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@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:
📝 WalkthroughWalkthrough
ChangesBackup shields lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant backupAll
participant BackupShieldsWindow
participant SandboxBackup
participant Shields
backupAll->>BackupShieldsWindow: open window
BackupShieldsWindow->>Shields: shieldsDown
backupAll->>SandboxBackup: create snapshot
backupAll->>BackupShieldsWindow: relock window
BackupShieldsWindow->>Shields: shieldsUp
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 10954f5 in the TypeScript / code-coverage/cliThe overall coverage in commit 10954f5 in the Show a code coverage summary of the most impacted files.
Updated |
|
🌿 Preview your docs: https://nvidia-preview-pr-7557.docs.buildwithfern.com/nemoclaw |
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported 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: 2 optional E2E recommendations
This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
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/backup-restore.mdx`:
- Around line 187-192: Update the backup-all behavior description in
backup-restore.mdx to state that an initial Shields-unlock failure marks the
sandbox as failed and processing continues with the next sandbox. Also document
that the 30-minute Shields-down window remains fixed without owner-aware
deferral, so a long-running backup may trigger automatic lockdown before
completion.
In `@src/lib/actions/maintenance.ts`:
- Around line 91-119: Update the backup flow around the try/catch/finally block
so relock failure does not throw from finally or replace an existing backup
failure. Capture the relock failure, preserve and propagate the original
backupError or orphanManifestMessage when present, and only report the relock
error directly when backup succeeded; ensure both failure contexts remain
available when both operations fail.
🪄 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: 3e2f4d55-d076-43df-bfc2-41a3f5d2ab42
📒 Files selected for processing (6)
docs/manage-sandboxes/backup-restore.mdxsrc/lib/actions/maintenance.test.tssrc/lib/actions/maintenance.tssrc/lib/actions/sandbox/backup-shields-window.tssrc/lib/actions/sandbox/rebuild-shields.tstest/rebuild-shields-window.test.ts
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
Addressed the valid exact-head review findings in
On
This PR implements that remediation through the bounded Shields window, which makes the protected credentials state readable during backup and restores lockdown afterward. The hard-failure regression applies only if the backup still fails after the window opens. Incomplete backups therefore remain nonzero by design. |
|
The exact-head static failure is a clean-checkout CI prerequisite regression on current |
<!-- markdownlint-disable MD041 --> ## Summary Make the semantic E2E phase check build its generated shared policy dependency before Vitest collection. A clean static-check checkout fails before collecting tests when `nemoclaw/dist/shared/` does not exist. The canonical command now builds only that prerequisite and then runs the unchanged coverage check. This corrects the confirmed CI blocker for #7557. No other PR is claimed blocked by this prerequisite. ## Changes - Add one `build:policy-boundary` script for the shared TypeScript artifact. - Reuse that script from the existing CLI build. - Run it before semantic E2E phase collection. - Add a contract test for the required command order. - Add the reviewed contract to the source-shape compatibility budget. ## 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) ## Quality Gates - [x] Tests added or updated for changed behavior — justification: `test/e2e/support/e2e-semantic-phase-check.test.ts` locks the policy-boundary build before semantic collection and CLI compilation. - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [ ] Docs updated for user-facing behavior changes - [x] Docs not applicable — justification: this changes only the automatic build order for an existing validation command. Its documented command, purpose, and output contract are unchanged. - [ ] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [ ] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Documentation Writer Review - [x] Documentation writer subagent reviewed the completed changes - Result: `no-docs-needed` - Evidence: Reviewed `package.json`, `ci/source-shape-test-budget.json`, and `test/e2e/support/e2e-semantic-phase-check.test.ts` at `2852a517b`. The change adds an internal automatic prerequisite for the existing `npm run test:e2e-phases:check` and `npm run build:cli` commands; their documented invocation, purpose, and behavior remain unchanged. - Agent: Codex Desktop <!-- docs-review-head-sha: 2852a51 --> <!-- docs-review-agents-blob-sha: be20a09 --> ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: - Station profile/scenario: - Result: - Supporting evidence: ## Verification - [x] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run check:diff` passed when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — command/result or justification: the focused semantic-phase contract passed 15 tests. Clean-artifact `npm run test:e2e-phases:check` passed 125 tests across 82 files. - [x] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: not applicable because this scope changes one build prerequisite, its contract test, and its reviewed budget entry. `npm run build:cli` and `npm run check:diff` passed. - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `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: Apurv Kumaria <akumaria@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Added a dedicated policy-boundary build step. * Updated the CLI build flow to run the policy-boundary step first. * Updated end-to-end phase checks to compile the policy-boundary components before validation. * **Tests** * Added an end-to-end semantic phase checker that verifies the expected command ordering for the new build step. * **CI** * Extended the source-shape test budget exceptions to account for the new semantic check coverage. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Apurv Kumaria <akumaria@nvidia.com> Signed-off-by: Carlos Villela <cvillela@nvidia.com> Co-authored-by: Carlos Villela <cvillela@nvidia.com>
cv
left a comment
There was a problem hiding this comment.
Security blocker: backup-all derives authorization to unlock from !isShieldsDown(), but isShieldsDown() returns false for corrupt persisted Shields state. That lets a corrupt or tampered posture enter shieldsDown(), overwrite corruption markers, and later reseal an untrusted baseline. Fail closed before mutation when Shields state is corrupt (or establish authoritative physical posture), and add corrupt+locked regression coverage. I disabled auto-merge pending an exact-head repair and fresh gates.
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
<!-- markdownlint-disable MD041 --> ## Summary The maintainer merge-gate checker now accepts immutable E2E retry history under the same fail-closed rule already enforced by the controller and required observer. This unblocks a later successful check only when every older exact-identity check is a completed failure with a valid supported retry marker. ## Related Issue Unblocks #7582 and the dependent #7557 release sequence. ## Changes - Select the highest-ID exact coordination check after validating the full retry history. - Reject malformed markers, unsupported reasons, never-retry titles, active history, identity drift, and foreign GitHub Apps. - Add a focused regression suite for all supported retry reasons and fail-closed histories. ## 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) ## Quality Gates - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [ ] Docs updated for user-facing behavior changes - [x] Docs not applicable — justification: the canonical maintainer guide and E2E runbook already document this exact retry-history contract; this change aligns the checker implementation. - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: independent exact-diff review by Codex Desktop passed; the parser and selector mirror the existing controller and observer allowlist and fail-closed rules. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Documentation Writer Review - [x] Documentation writer subagent reviewed the completed changes - Result: `no-docs-needed` - Evidence: Existing `.agents/skills/nemoclaw-maintainer-day/MERGE-GATE.md` and `test/e2e/README.md` already define the supported reasons, immutable history, highest-ID selection, and fail-closed conditions. - Agent: Codex Desktop <!-- docs-review-head-sha: 7cd0ff7 --> <!-- docs-review-agents-blob-sha: be20a09 --> ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: Not applicable - Station profile/scenario: Not applicable - Result: Not applicable - Supporting evidence: Not applicable ## Verification - [x] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run check:diff` passed when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — `npx vitest run --project integration test/skills/check-gates-evidence.test.ts test/skills/check-gates-retry-history.test.ts test/skills/check-gates-fork-evidence.test.ts test/skills/check-gates-compliance.test.ts` passed 4 files and 160 tests; `npm run test-size:check` passed. - [x] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — not applicable to the two-file internal maintainer-checker change; the complete checker suite and normal hooks passed. - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `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: Carlos Villela <cvillela@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved E2E merge-gate validation for retryable failures and retry history. * Correctly recognizes supported retry markers and excludes non-retryable runs. * More reliably identifies the current E2E coordination check when multiple checks exist. * Fails closed when retry evidence is incomplete, malformed, unrelated, or includes active or unsupported checks. * **Tests** * Added coverage for successful retries following supported failures. * Added validation for incomplete, invalid, and non-retryable retry histories. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@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/troubleshooting.mdx`:
- Around line 1687-1694: Move the “A Shields command reports corrupt persisted
state” troubleshooting section out of the OpenClaw-only content block so it is
shared by both OpenClaw and Hermes pages. Preserve the existing recovery
guidance and placement within the general troubleshooting content.
🪄 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: d47cd31c-0bf6-421f-b94a-535a799d3c8a
📒 Files selected for processing (4)
docs/reference/troubleshooting.mdxsrc/lib/shields/index.test.tssrc/lib/shields/index.tstest/e2e/live/snapshot-commands.test.ts
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
prekshivyas
left a comment
There was a problem hiding this comment.
Exact-head maintainer review at 10954f50ec56bff21e6b829e977b70983995bb90: the implementation, focused tests, documentation review, ordinary CI, self-hosted image checks, security review, and automated review are clean. Protected E2E cannot validate this change yet: snapshot-commands failed in the existing cross-sandbox clone-restore phase before reaching the new protected-backup phase. State restore completed, but authenticated gateway pairing verification failed (test/e2e/live/snapshot-commands.test.ts:432; child run 30238525114, job 89890933833). This is the gateway self-dialback defect tracked by #7215 and addressed by excluded PR #7230. Per the protected merge gate, this assertion failure is not retry-eligible on the same SHA, so I cannot approve this head. Once #7230 lands, refresh this branch and rerun exact-head CI/E2E; approval can follow if the protected backup phase and full selected suite pass.
<!-- markdownlint-disable MD041 --> ## Summary `docs/changelog/2026-07-25.mdx` now includes the user-facing fixes that merged after #7607 and before the v0.0.96 tag. The follow-up covers safer bulk backup and clone restore behavior, policy and inference repairs, cleaner onboarding diagnostics, and OpenClaw base-image validation while leaving test-only and maintainer-internal merges out of the release entry. ## Changes - Document the Shields-safe `backup-all` flow from #7557 and the clone-specific restore pairing publication from #7608. - Record the Claude Code resolved-launcher policy repair from #7581, Hermes namespaced-model handling from #7604, and persisted Ollama proxy-token reuse from #7620. - Record OpenClaw immutable base-inventory validation from #7606, hidden route-only reservations from #7621, and clean invalid gateway-management errors from #7630. - Link the gateway lifecycle and snapshot authorities, retain #7622's already-merged Docker Engine wording, and exclude internal or test-only merges from the release entry. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [x] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates - [ ] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [x] Tests not applicable — justification: This PR changes release-entry prose only. The changelog contract test and Fern validation cover the dated entry, published routes, and rendering requirements. - [x] Docs updated for user-facing behavior changes - [ ] Docs not applicable — justification: - [ ] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [ ] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Documentation Writer Review - [x] Documentation writer subagent reviewed the completed changes - Result: `docs-updated` - Evidence: At exact PR head `29316da26`, a Codex Desktop documentation writer reviewed `docs/changelog/2026-07-25.mdx` against `AGENTS.md`, `WRITING.md`, and `docs/CONTRIBUTING.md`. The review confirmed that the full entry accurately reflects the merged user-visible behavior, retains #7622's existing wording, appropriately excludes internal and test-only PRs, and uses conforming terminology, structure, links, and release classification. It also confirmed that the review follow-ups use active third-person release-entry voice, name the actor and recovery requirement directly, and accurately preserve the trusted-backup, cached-release refresh, and local-build fallback constraints. The changelog test passed 6/6, and the docs build completed with 0 errors and 2 pre-existing hidden warnings. - Agent: Codex Desktop <!-- docs-review-head-sha: 29316da --> <!-- docs-review-agents-blob-sha: be20a09 --> ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: - Station profile/scenario: - Result: - Supporting evidence: ## Verification - [x] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run check:diff` passed when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — command/result or justification: `npx vitest run test/changelog-docs.test.ts` passed 6/6 tests after the final review fix. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: Not applicable to this prose-only changelog change. - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) — exited 0 with 0 errors and 2 pre-existing hidden warnings after the final review fix. - [x] 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: Carlos Villela <cvillela@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Expanded the changelog to clarify persistent `policy exclude`/`policy restore` behavior across rebuilds and snapshot restores, including reporting on removed endpoints and exclusion consistency. * Updated `claude-code` preset guidance to allow the npm-installed OpenShell launcher path while maintaining endpoint/HTTP method scope. * Documented hardened handling for invalid gateway-management declarations, improved gateway/agent-version diagnostics scope, and clarified onboarding/restore credential and reasoning precedence. * Tightened bulk backup/restore guidance (safety windows, approval limits, and failure recovery) and refined OpenClaw base selection to avoid incompatible cached releases and `:latest` fallback. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Summary
backup-allnow backs up intentionally protected state by opening one bounded Shields window per eligible sandbox and restoring the original locked posture before it processes the next sandbox.The change follows @Hokonoken's live root-cause analysis in #6455, keeps incomplete backups as hard failures, and now has protected live coverage for root-owned credential state.
Corrupt persisted Shields state fails closed before policy, config, timer, transition, state, or audit mutation; recovery requires restoring trusted host state.
Related Issue
Fixes #6455
Changes
backup-allattempt only when the sandbox starts with Shields UP.finallyafter success, a reported backup failure, or an exception.shields uprecovery command before the retry command.shields upcan skip restrictive-policy restoration or destroy timer and transition recovery authority.shields upnor ordinary rebuild replaces corrupt state.snapshot-commandscoverage for root-owned0700/0600credentials, sanitized backup contents, Shields relock, restored permissions, and timer removal.Type of Change
Quality Gates
10954f50ec56bff21e6b829e977b70983995bb90passed product scope and all nine security categories. The ten PR files are byte-identical to the prior passing security review. Corrupt state is rejected before config resolution, policy commands, state writes, timer kill, transition clear, or audit append. The 30-minute no-owner-deferral window and batch abort on relock failure remain intentional documented contracts. Trusted exact-headsnapshot-commandsE2E is still required before merge.Documentation Writer Review
docs-updated10954f50ec56bff21e6b829e977b70983995bb90against base0b185498155a0a51a3f682a3e2b57f80c95eeaaa, reviewed the complete ten-file diff and generated OpenClaw, Hermes, and Deep Agents variants. The ten PR files are byte-unchanged from passing reviewed headb2c642425.docs/manage-sandboxes/backup-restore.mdxaccurately documents the bounded per-sandboxbackup-allShields window and failure behavior.docs/reference/troubleshooting.mdxdocuments fail-closed corrupt-state preservation and trusted-state recovery for all three documented Shields variants; each generated troubleshooting page contains the section exactly once. Exact-treenpm run docspassed with 0 errors and two existing warnings; targeted tests passed 73/73; CLI and plugin typechecks and the merge pre-push CLI typecheck passed. GitHub reports the exact-head commit Verified.DGX Station Hardware Evidence
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 unavailablenpm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result: Exact-head GitHub CI and protectedsnapshot-commandsE2E are in progress; no result is waived.npm run docsbuilds without warnings (doc changes only) — 0 errors; two pre-existing hidden Fern warnings remain.Signed-off-by: Apurv Kumaria akumaria@nvidia.com
Signed-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit
backup-allnow performs a timed Shields-down window per eligible sandbox, then restores lockdown, while tracking whether the window opened successfully.