Skip to content

fix(installer): recover sandboxes before onboarding - #6132

Merged
cv merged 7 commits into
mainfrom
fix/upgrade-sandbox-recovery-6114
Jul 1, 2026
Merged

fix(installer): recover sandboxes before onboarding#6132
cv merged 7 commits into
mainfrom
fix/upgrade-sandbox-recovery-6114

Conversation

@ericksoa

@ericksoa ericksoa commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Summary

This change runs validated prepared-backup recovery before the installer starts generic onboarding.
It reuses the existing installer recovery signal and rebuild machinery, fails closed on identity or managed-image evidence mismatches, and stops onboarding if any recovery fails.
Focused verification passes. However, v0.0.55 recorded no trustworthy per-sandbox managed/custom image provenance, so this PR now rejects those ambiguous entries and does not by itself resolve #6114.

Related Issue

Related to #6114

Changes

  • Run upgrade-sandboxes --auto for pre-existing sandboxes before generic onboarding.
  • Under NEMOCLAW_RESTORE_LATEST_BACKUP_ON_RECREATE=1, include registered non-Ready sandboxes only when their latest backup validates against the sandbox and agent identity.
  • Require an explicitly observed, known non-Ready live phase so an absent registry entry—such as a Ready sandbox on another gateway—is never selected for prepared recovery.
  • Isolate backup-assessment failures per sandbox, attempt every later eligible recovery, and return aggregate nonzero when any eligible recovery is blocked or fails.
  • Require a non-empty NemoClaw-managed image fingerprint and reject pre-fingerprint, missing, mismatched, or custom-image recovery inputs before deletion; matching agent versions are explicitly not accepted as provenance.
  • Revalidate the registry entry and latest manifest immediately before the destructive phase.
  • Reuse the validated manifest in the existing rebuild path instead of creating a second unreachable backup.
  • Preserve the existing registry rollback and state-restore behavior when recreation fails.
  • Attempt every eligible recovery and return nonzero before onboarding if any recovery fails.
  • Route recovery failure through the installer completion summary so preserved-backup guidance is shown before the nonzero exit.
  • Add focused rebuild, manifest-validation, upgrade classification, installer-ordering, and rollback coverage.
  • Document the installer's pre-onboarding recovery behavior.
  • Local validation passed for CLI build/typecheck, focused CLI and integration tests, shell checks, Biome, project membership, source-shape, and test-size checks. The Fern docs check completed with 0 errors and 2 warnings.
  • Live v0.0.55 upgrade acceptance is blocked by missing trustworthy legacy image provenance: safely recovering raw v0.0.55 managed images while rejecting custom images requires a separate design. No v0.0.55 recovery success is claimed here.

Type of Change

  • 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

  • Tests added or updated for changed behavior
  • Existing tests cover changed behavior — justification:
  • Tests not applicable — justification:
  • 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: Independent review through 35ab003bbba4159cb630b9e04566261ce5401c8b confirmed the fingerprint-only rule blocks the probed-custom-image deletion path and also confirmed raw v0.0.55 recovery remains unresolved.
  • Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue:

Verification

  • PR description includes the DCO sign-off declaration and every commit appears as Verified in GitHub
  • Git hooks passed during commit and push, or npx prek run --from-ref main --to-ref HEAD passes
  • Targeted tests pass for changed behavior
  • Full npm test passes (broad runtime changes only)
  • Quality Gates section completed with required justifications or waivers
  • No secrets, API keys, or credentials committed
  • npm run docs builds without warnings (doc changes only)
  • Doc pages follow the style guide (doc changes only)
  • New doc pages include SPDX header and frontmatter (new pages only)

Signed-off-by: Aaron Erickson aerickson@nvidia.com

Summary by CodeRabbit

  • New Features
    • Installer/upgrade workflows now recover pre-existing non-Ready sandboxes before starting generic onboarding, including prepared recovery from the latest validated pre-upgrade backup when eligible and evidenced.
  • Bug Fixes
    • Recovery is fail-closed: if recovery can’t be completed for any eligible sandbox, the process stops and does not proceed with generic onboarding.
    • Added stronger identity, manifest, and managed-image evidence checks to prevent unsafe recreate/recovery.
    • Upgrade output now clearly distinguishes “prepared” vs “rejected” recovery candidates.
  • Documentation
    • Updated quickstart and lifecycle upgrade guides to match the recovery-first flow.
  • Tests
    • Expanded coverage for recovery ordering and prepared-backup recovery validation.

Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
@ericksoa ericksoa added bug-fix PR fixes a bug or regression area: install Install, setup, prerequisites, or uninstall flow area: sandbox OpenShell sandbox lifecycle, runtime, config, or recovery v0.0.72 labels Jul 1, 2026
@coderabbitai

coderabbitai Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

This PR adds prepared-backup recovery for pre-existing sandboxes during upgrades. It validates recovery manifests and managed-image evidence, threads recovery through rebuild and upgrade flows, moves installer recovery before onboarding, and updates docs and tests for the new behavior.

Changes

Prepared Backup Recovery

Layer / File(s) Summary
Recovery manifest validation
src/lib/state/sandbox.ts, test/snapshot-recovery-validation.test.ts
Adds validateRebuildRecoveryManifest() and hasPositiveManagedImageEvidence() with identity/version checks, and covers them with dedicated tests.
Prepared recovery rebuild flow
src/lib/actions/sandbox/rebuild.ts
Adds RebuildSandboxExecutionOptions.recoveryManifest, pre-delete revalidation, generalized recoveryRecreate handling, registry restore-on-failure, and a post-restore completeness bail-out.
Prepared recovery rebuild tests
src/lib/actions/sandbox/rebuild-flow.test.ts
Extends the rebuild test harness with manifest/registry override hooks and adds tests covering recovery and registry drift failure modes.
Prepared recovery classification
src/lib/actions/upgrade-sandboxes.ts, src/lib/actions/upgrade-sandboxes-recovery.test.ts
Classifies sandboxes into prepared/rejected recovery candidates, updates CLI/check reporting and the rebuild worklist, and calls rebuildSandbox with recoveryManifest; adds new recovery-focused tests.
Installer pre-onboarding recovery gate
scripts/install.sh, test/install-preexisting-sandbox-recovery.test.ts, docs/get-started/quickstart.mdx, docs/manage-sandboxes/lifecycle.mdx
Adds recover_preexisting_sandboxes_before_onboard(), reorders onboarding to recover sandboxes first, adjusts sandbox-count detection ordering, updates related messaging, adds installer tests, and documents the recovery flow.

Estimated code review effort: 4 (Complex) | ~60 minutes

Suggested labels: area: onboarding

Suggested reviewers: cv

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the main installer change: recovering sandboxes before onboarding.
Linked Issues check ✅ Passed The changes implement pre-onboarding recovery, preserve sandbox state, and add validation/rollback behavior matching #6114.
Out of Scope Changes check ✅ Passed The modified code, tests, and docs all support the sandbox-recovery upgrade flow and do not introduce clear unrelated changes.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/upgrade-sandbox-recovery-6114

Comment @coderabbitai help to get the list of available commands.

@github-code-quality

github-code-quality Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

The overall coverage in the fix/upgrade-sandbox-... branch is 96%. Coverage data for the main branch is not yet available.

Show a code coverage summary of the most covered files.
File main fix/upgrade-sandbox-... f6d6080 +/-
nemoclaw/src/se...cret-scanner.ts 100%
nemoclaw/src/commands/slash.ts 100%
nemoclaw/src/li...bprocess-env.ts 100%
nemoclaw/src/bl...eprint/state.ts 98%
nemoclaw/src/onboard/config.ts 98%
nemoclaw/src/bl...int/snapshot.ts 97%
nemoclaw/src/bl...print/runner.ts 95%
nemoclaw/src/co...ration-state.ts 94%
nemoclaw/src/bl...ate-networks.ts 94%
nemoclaw/src/index.ts 94%

TypeScript / code-coverage/cli

The overall coverage in the fix/upgrade-sandbox-... branch is 68%. Coverage data for the main branch is not yet available.

Show a code coverage summary of the most covered files.
File main fix/upgrade-sandbox-... f6d6080 +/-
src/lib/shields...nsition-lock.ts 86%
src/lib/actions...dbox/rebuild.ts 82%
src/lib/actions...all/run-plan.ts 80%
src/lib/state/o...oard-session.ts 80%
src/lib/shields/index.ts 75%
src/lib/state/sandbox.ts 73%
src/lib/onboard/preflight.ts 69%
src/lib/onboard...er-gpu-patch.ts 59%
src/lib/actions...licy-channel.ts 58%
src/lib/onboard.ts 20%

Updated July 01, 2026 22:31 UTC
Code Coverage is in Public Preview. Learn more and provide us with your feedback.

@github-actions

github-actions Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

@github-actions

github-actions Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor — Changes requested

Merge posture: Do not merge yet
Primary next action: Resolve or justify PRA-1: Source-of-truth review needed: Prepared-backup installer recovery bridge.
Open items: 0 required · 3 warnings · 0 suggestions · 8 test follow-ups
Since last review: 0 prior items resolved · 2 still apply · 0 new items found

Action checklist

  • PRA-1 Resolve or justify: Source-of-truth review needed: Prepared-backup installer recovery bridge
  • PRA-2 Resolve or justify: Prepared-backup recovery still lacks live runtime validation
  • PRA-3 Resolve or justify: Prepared-recovery trust boundary remains spread through large lifecycle hotspots in src/lib/actions/sandbox/rebuild.ts:584
  • PRA-T1 Add or justify test follow-up: Runtime validation
  • PRA-T2 Add or justify test follow-up: Runtime validation
  • PRA-T3 Add or justify test follow-up: Runtime validation
  • PRA-T4 Add or justify test follow-up: Runtime validation
  • PRA-T5 Add or justify test follow-up: Prepared-backup recovery still lacks live runtime validation
  • PRA-T6 Add or justify test follow-up: Acceptance clause
  • PRA-T7 Add or justify test follow-up: Acceptance clause
  • PRA-T8 Add or justify test follow-up: Prepared-backup installer recovery bridge

Findings index

ID Severity Category Location Required action
PRA-1 Resolve/justify architecture Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
PRA-2 Resolve/justify tests Add or identify targeted runtime/integration validation for the changed behavior before relying on the focused unit and shell-snippet coverage. Keep the current negative-path unit tests; supplement them with live prepared-backup recovery checks for the eligible managed-image case and a destructive-safety rejection check for ambiguous/pre-fingerprint inputs.
PRA-3 Resolve/justify architecture src/lib/actions/sandbox/rebuild.ts:584 Extract the cohesive prepared-recovery eligibility, manifest validation, pre-delete revalidation, and rollback snapshot selection into a small module used by `upgrade-sandboxes.ts` and `rebuild.ts`. Keep the installer shell as the handoff/orchestration layer, and preserve every current fail-closed check.
Review findings by urgency: 0 required fixes, 3 items to resolve/justify, 0 in-scope improvements

⚠️ Resolve or justify before merge

Investigate these in the current review; either fix them, explain why they are not applicable, or document the accepted risk.

PRA-1 Resolve/justify — Source-of-truth review needed: Prepared-backup installer recovery bridge

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: Focused tests cover installer handoff/order, non-Ready selection, blocked recovery without managed-image evidence, manifest validation, latest-backup and registry drift before delete, aggregate failure, and registry rollback. Live runtime validation is still missing and is covered by the test-depth finding.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: `upgrade-sandboxes.ts` contains the source-boundary comment for [All Platforms][Upgrade] v0.0.55 → v0.0.76 leaves pre-existing sandboxes stuck in Provisioning/Error — user data inaccessible until manual rebuild #6114 and uses `NEMOCLAW_RESTORE_LATEST_BACKUP_ON_RECREATE`; `install.sh` exports the recovery signal after pre-upgrade backup/prepared state and runs recovery before onboarding; `state/sandbox.ts` rejects ambiguous provenance through `hasPositiveManagedImageEvidence`.

PRA-2 Resolve/justify — Prepared-backup recovery still lacks live runtime validation

  • Location: not file-specific
  • Category: tests
  • Problem: The changed tests cover the decision logic well, including installer ordering, non-Ready selection, malformed or mismatched manifests, managed-image fingerprint rejection, pre-delete registry/latest-backup drift, aggregate failure, and rollback. However, the user-visible contract still is not proven across a real OpenShell sandbox boundary: an eligible recovered sandbox should become Ready, support `nemoclaw <name> exec`, and preserve the state/policy/config surfaces that recovery promises to restore.
  • Impact: Because this path can delete and recreate pre-existing sandboxes before generic onboarding, mocked success can still hide a real restore, gateway, policy, or agent-readiness failure that leaves an operator with a backup-only or partially restored sandbox.
  • Recommended action: Add or identify targeted runtime/integration validation for the changed behavior before relying on the focused unit and shell-snippet coverage. Keep the current negative-path unit tests; supplement them with live prepared-backup recovery checks for the eligible managed-image case and a destructive-safety rejection check for ambiguous/pre-fingerprint inputs.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Shortest read-only check: inspect `src/lib/actions/upgrade-sandboxes-recovery.test.ts`, `src/lib/actions/sandbox/rebuild-flow.test.ts`, and `test/install-preexisting-sandbox-recovery.test.ts`; `captureSandboxListWithGatewayRecovery`, `rebuildSandbox`, `runOpenshell`, `onboard`, restore behavior, registry reads, and installer functions are mocked or stubbed rather than exercising a real installer-to-OpenShell recovery followed by `nemoclaw <name> exec`.
  • Missing regression test: Add or identify behavior coverage named like `installer prepared recovery recreates an eligible managed OpenClaw sandbox before generic onboarding and leaves nemoclaw <name> exec working`, `prepared recovery restores OpenClaw workspace marker, .env, credentials, skills, config, and policy-list from the validated latest backup`, and, if Hermes is in scope, `prepared recovery preserves Hermes state files, tool gateway/dashboard configuration, and leaves nemohermes <name> exec working`. Also add a live or near-live destructive-safety check that `prepared recovery rejects an ambiguous pre-fingerprint/custom-image registry entry without deleting the non-Ready sandbox`.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Shortest read-only check: inspect `src/lib/actions/upgrade-sandboxes-recovery.test.ts`, `src/lib/actions/sandbox/rebuild-flow.test.ts`, and `test/install-preexisting-sandbox-recovery.test.ts`; `captureSandboxListWithGatewayRecovery`, `rebuildSandbox`, `runOpenshell`, `onboard`, restore behavior, registry reads, and installer functions are mocked or stubbed rather than exercising a real installer-to-OpenShell recovery followed by `nemoclaw <name> exec`.
  • Evidence: The static inventory lists only focused changed tests: `upgrade-sandboxes-recovery.test.ts` mocks OpenShell list, backup discovery/evidence checks, and `rebuildSandbox`; `rebuild-flow.test.ts` mocks OpenShell runtime, registry, backup, restore, and onboard; `install-preexisting-sandbox-recovery.test.ts` sources `install.sh` with stubbed installer functions and a fake CLI. The validation context verdict is `runtime_validation_recommended` for `install.sh`, `rebuild.ts`, `upgrade-sandboxes.ts`, and `state/sandbox.ts`.

PRA-3 Resolve/justify — Prepared-recovery trust boundary remains spread through large lifecycle hotspots

  • Location: src/lib/actions/sandbox/rebuild.ts:584
  • Category: architecture
  • Problem: The PR adds important fail-closed checks, but the single prepared-recovery contract is still split across installer handoff, upgrade eligibility, manifest/provenance validation, pre-delete revalidation, delete/recreate, restore, and registry rollback code. Those pieces now live in `install.sh`, `upgrade-sandboxes.ts`, `state/sandbox.ts`, and the already-large `rebuild.ts` flow, with a large require-cache-heavy test harness mirroring the split.
  • Impact: Future lifecycle edits can accidentally weaken or bypass one part of the destructive recovery boundary, such as fingerprint-only managed-image evidence, latest-backup identity, registry-drift rejection, selected-gateway narrowing, or registry restore after recreate failure. That is a security and data-loss regression risk even though the current checks are fail-closed.
  • Recommended action: Extract the cohesive prepared-recovery eligibility, manifest validation, pre-delete revalidation, and rollback snapshot selection into a small module used by `upgrade-sandboxes.ts` and `rebuild.ts`. Keep the installer shell as the handoff/orchestration layer, and preserve every current fail-closed check.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Shortest read-only check: inspect `src/lib/actions/upgrade-sandboxes.ts` around `prepareBackupRecovery`, `src/lib/state/sandbox.ts` around `validateRebuildRecoveryManifest` and `hasPositiveManagedImageEvidence`, and `src/lib/actions/sandbox/rebuild.ts` around `revalidatePreparedRecoveryBeforeDelete`; the prepared-recovery authority is split across all three TypeScript files plus installer ordering in `scripts/install.sh`.
  • Missing regression test: If extracted, keep or move the existing coverage for `fails closed for a probed v0.0.55 custom image with matching backup agent version`, `rejects a latest-backup change immediately before deleting the sandbox`, `rejects same-agent registry configuration drift before deleting the sandbox`, `uses the single refreshed registry snapshot for recreate rollback`, and `restores the registry entry when prepared-backup recreation fails`; add module-level tests for the extracted planner if any logic moves.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Shortest read-only check: inspect `src/lib/actions/upgrade-sandboxes.ts` around `prepareBackupRecovery`, `src/lib/state/sandbox.ts` around `validateRebuildRecoveryManifest` and `hasPositiveManagedImageEvidence`, and `src/lib/actions/sandbox/rebuild.ts` around `revalidatePreparedRecoveryBeforeDelete`; the prepared-recovery authority is split across all three TypeScript files plus installer ordering in `scripts/install.sh`.
  • Evidence: Drift analysis reports large hotspot growth: `src/lib/actions/sandbox/rebuild.ts` grows by 147 lines to 1395 lines, `src/lib/state/sandbox.ts` grows by 76 lines to 1832 lines, and `src/lib/actions/sandbox/rebuild-flow.test.ts` grows by 231 lines to 1052 lines. The current diff adds `RebuildSandboxExecutionOptions.recoveryManifest`, `revalidatePreparedRecoveryBeforeDelete`, `prepareBackupRecovery`, and `validateRebuildRecoveryManifest` in separate files.

💡 In-scope improvements

These are lower-risk, not throwaway. Prefer fixing them in this PR when they are local to changed code; defer only with rationale or a linked follow-up.

  • None.
Simplification opportunities: 1 possible cut

These are safe simplification checks only. Do not remove validation, security controls, data-loss prevention, or required tests.

  • PRA-3 shrink (src/lib/actions/sandbox/rebuild.ts:584): Inline prepared-recovery eligibility, manifest/provenance validation, latest-backup and registry revalidation, and rollback snapshot selection spread across `upgrade-sandboxes.ts`, `rebuild.ts`, and `state/sandbox.ts`.
    • Replacement: A focused prepared-recovery helper/module with small pure functions called by `upgrade-sandboxes.ts` and `rebuild.ts`, plus targeted module tests and the existing end-to-end flow tests kept as callers.
    • Safety boundary: Do not remove or weaken fingerprint-only managed-image evidence, persisted manifest re-read, backup path/timestamp/sandbox/agent identity checks, latest-backup revalidation, registry deep-equality drift rejection, selected-gateway/non-Ready narrowing, aggregate failure behavior, registry rollback on recreate failure, or state restore failure reporting.
Test follow-ups to resolve or justify

If these cover changed behavior, prefer adding them in this PR; otherwise state why existing coverage is enough or link the follow-up.

  • PRA-T1 Runtime validation — installer prepared recovery recreates an eligible managed OpenClaw sandbox before generic onboarding and leaves nemoclaw <name> exec working. The PR changes installer/bootstrap and destructive sandbox lifecycle recovery paths where OpenShell state, gateway readiness, snapshot restore, policies, credentials, and agent runtime behavior matter. The added tests are valuable and cover many negative paths, but most changed behavior is exercised with mocks or shell snippets rather than a real sandbox boundary.
  • PRA-T2 Runtime validation — prepared recovery restores OpenClaw workspace marker, .env, credentials, skills, config, and policy-list from the validated latest backup. The PR changes installer/bootstrap and destructive sandbox lifecycle recovery paths where OpenShell state, gateway readiness, snapshot restore, policies, credentials, and agent runtime behavior matter. The added tests are valuable and cover many negative paths, but most changed behavior is exercised with mocks or shell snippets rather than a real sandbox boundary.
  • PRA-T3 Runtime validation — prepared recovery preserves Hermes state files, tool gateway/dashboard configuration, and leaves nemohermes <name> exec working when Hermes recovery is in scope. The PR changes installer/bootstrap and destructive sandbox lifecycle recovery paths where OpenShell state, gateway readiness, snapshot restore, policies, credentials, and agent runtime behavior matter. The added tests are valuable and cover many negative paths, but most changed behavior is exercised with mocks or shell snippets rather than a real sandbox boundary.
  • PRA-T4 Runtime validation — prepared recovery rejects an ambiguous pre-fingerprint/custom-image registry entry without deleting the observed non-Ready sandbox. The PR changes installer/bootstrap and destructive sandbox lifecycle recovery paths where OpenShell state, gateway readiness, snapshot restore, policies, credentials, and agent runtime behavior matter. The added tests are valuable and cover many negative paths, but most changed behavior is exercised with mocks or shell snippets rather than a real sandbox boundary.
  • PRA-T5 Prepared-backup recovery still lacks live runtime validation — Add or identify targeted runtime/integration validation for the changed behavior before relying on the focused unit and shell-snippet coverage. Keep the current negative-path unit tests; supplement them with live prepared-backup recovery checks for the eligible managed-image case and a destructive-safety rejection check for ambiguous/pre-fingerprint inputs.
  • PRA-T6 Acceptance clause — Related to [All Platforms][Upgrade] v0.0.55 → v0.0.76 leaves pre-existing sandboxes stuck in Provisioning/Error — user data inaccessible until manual rebuild #6114 — add test evidence or identify existing coverage. The deterministic validation context reports `linkedIssues: []`; no linked issue body or comments were available to extract literal acceptance clauses. The PR body references [All Platforms][Upgrade] v0.0.55 → v0.0.76 leaves pre-existing sandboxes stuck in Provisioning/Error — user data inaccessible until manual rebuild #6114, but PR-provided text is untrusted and no literal issue clauses/comments were available for clause-by-clause acceptance mapping.
  • PRA-T7 Acceptance clause — Add focused rebuild, manifest-validation, upgrade classification, installer-ordering, and rollback coverage. — add test evidence or identify existing coverage. Focused coverage exists in the changed test files listed by static inventory, including manifest validation, installer ordering, upgrade recovery, and rebuild rollback cases. Runtime validation remains partial because no changed test exercises a real installer/OpenShell/rebuild/restore boundary followed by `nemoclaw <name> exec`.
  • PRA-T8 Prepared-backup installer recovery bridge — Focused tests cover installer handoff/order, non-Ready selection, blocked recovery without managed-image evidence, manifest validation, latest-backup and registry drift before delete, aggregate failure, and registry rollback. Live runtime validation is still missing and is covered by the test-depth finding.. `upgrade-sandboxes.ts` contains the source-boundary comment for [All Platforms][Upgrade] v0.0.55 → v0.0.76 leaves pre-existing sandboxes stuck in Provisioning/Error — user data inaccessible until manual rebuild #6114 and uses `NEMOCLAW_RESTORE_LATEST_BACKUP_ON_RECREATE`; `install.sh` exports the recovery signal after pre-upgrade backup/prepared state and runs recovery before onboarding; `state/sandbox.ts` rejects ambiguous provenance through `hasPositiveManagedImageEvidence`.
Since last review details

Current findings, using the urgency labels above:

PRA-1 Resolve/justify — Source-of-truth review needed: Prepared-backup installer recovery bridge

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: Focused tests cover installer handoff/order, non-Ready selection, blocked recovery without managed-image evidence, manifest validation, latest-backup and registry drift before delete, aggregate failure, and registry rollback. Live runtime validation is still missing and is covered by the test-depth finding.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: `upgrade-sandboxes.ts` contains the source-boundary comment for [All Platforms][Upgrade] v0.0.55 → v0.0.76 leaves pre-existing sandboxes stuck in Provisioning/Error — user data inaccessible until manual rebuild #6114 and uses `NEMOCLAW_RESTORE_LATEST_BACKUP_ON_RECREATE`; `install.sh` exports the recovery signal after pre-upgrade backup/prepared state and runs recovery before onboarding; `state/sandbox.ts` rejects ambiguous provenance through `hasPositiveManagedImageEvidence`.

PRA-2 Resolve/justify — Prepared-backup recovery still lacks live runtime validation

  • Location: not file-specific
  • Category: tests
  • Problem: The changed tests cover the decision logic well, including installer ordering, non-Ready selection, malformed or mismatched manifests, managed-image fingerprint rejection, pre-delete registry/latest-backup drift, aggregate failure, and rollback. However, the user-visible contract still is not proven across a real OpenShell sandbox boundary: an eligible recovered sandbox should become Ready, support `nemoclaw <name> exec`, and preserve the state/policy/config surfaces that recovery promises to restore.
  • Impact: Because this path can delete and recreate pre-existing sandboxes before generic onboarding, mocked success can still hide a real restore, gateway, policy, or agent-readiness failure that leaves an operator with a backup-only or partially restored sandbox.
  • Recommended action: Add or identify targeted runtime/integration validation for the changed behavior before relying on the focused unit and shell-snippet coverage. Keep the current negative-path unit tests; supplement them with live prepared-backup recovery checks for the eligible managed-image case and a destructive-safety rejection check for ambiguous/pre-fingerprint inputs.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Shortest read-only check: inspect `src/lib/actions/upgrade-sandboxes-recovery.test.ts`, `src/lib/actions/sandbox/rebuild-flow.test.ts`, and `test/install-preexisting-sandbox-recovery.test.ts`; `captureSandboxListWithGatewayRecovery`, `rebuildSandbox`, `runOpenshell`, `onboard`, restore behavior, registry reads, and installer functions are mocked or stubbed rather than exercising a real installer-to-OpenShell recovery followed by `nemoclaw <name> exec`.
  • Missing regression test: Add or identify behavior coverage named like `installer prepared recovery recreates an eligible managed OpenClaw sandbox before generic onboarding and leaves nemoclaw <name> exec working`, `prepared recovery restores OpenClaw workspace marker, .env, credentials, skills, config, and policy-list from the validated latest backup`, and, if Hermes is in scope, `prepared recovery preserves Hermes state files, tool gateway/dashboard configuration, and leaves nemohermes <name> exec working`. Also add a live or near-live destructive-safety check that `prepared recovery rejects an ambiguous pre-fingerprint/custom-image registry entry without deleting the non-Ready sandbox`.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Shortest read-only check: inspect `src/lib/actions/upgrade-sandboxes-recovery.test.ts`, `src/lib/actions/sandbox/rebuild-flow.test.ts`, and `test/install-preexisting-sandbox-recovery.test.ts`; `captureSandboxListWithGatewayRecovery`, `rebuildSandbox`, `runOpenshell`, `onboard`, restore behavior, registry reads, and installer functions are mocked or stubbed rather than exercising a real installer-to-OpenShell recovery followed by `nemoclaw <name> exec`.
  • Evidence: The static inventory lists only focused changed tests: `upgrade-sandboxes-recovery.test.ts` mocks OpenShell list, backup discovery/evidence checks, and `rebuildSandbox`; `rebuild-flow.test.ts` mocks OpenShell runtime, registry, backup, restore, and onboard; `install-preexisting-sandbox-recovery.test.ts` sources `install.sh` with stubbed installer functions and a fake CLI. The validation context verdict is `runtime_validation_recommended` for `install.sh`, `rebuild.ts`, `upgrade-sandboxes.ts`, and `state/sandbox.ts`.

PRA-3 Resolve/justify — Prepared-recovery trust boundary remains spread through large lifecycle hotspots

  • Location: src/lib/actions/sandbox/rebuild.ts:584
  • Category: architecture
  • Problem: The PR adds important fail-closed checks, but the single prepared-recovery contract is still split across installer handoff, upgrade eligibility, manifest/provenance validation, pre-delete revalidation, delete/recreate, restore, and registry rollback code. Those pieces now live in `install.sh`, `upgrade-sandboxes.ts`, `state/sandbox.ts`, and the already-large `rebuild.ts` flow, with a large require-cache-heavy test harness mirroring the split.
  • Impact: Future lifecycle edits can accidentally weaken or bypass one part of the destructive recovery boundary, such as fingerprint-only managed-image evidence, latest-backup identity, registry-drift rejection, selected-gateway narrowing, or registry restore after recreate failure. That is a security and data-loss regression risk even though the current checks are fail-closed.
  • Recommended action: Extract the cohesive prepared-recovery eligibility, manifest validation, pre-delete revalidation, and rollback snapshot selection into a small module used by `upgrade-sandboxes.ts` and `rebuild.ts`. Keep the installer shell as the handoff/orchestration layer, and preserve every current fail-closed check.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Shortest read-only check: inspect `src/lib/actions/upgrade-sandboxes.ts` around `prepareBackupRecovery`, `src/lib/state/sandbox.ts` around `validateRebuildRecoveryManifest` and `hasPositiveManagedImageEvidence`, and `src/lib/actions/sandbox/rebuild.ts` around `revalidatePreparedRecoveryBeforeDelete`; the prepared-recovery authority is split across all three TypeScript files plus installer ordering in `scripts/install.sh`.
  • Missing regression test: If extracted, keep or move the existing coverage for `fails closed for a probed v0.0.55 custom image with matching backup agent version`, `rejects a latest-backup change immediately before deleting the sandbox`, `rejects same-agent registry configuration drift before deleting the sandbox`, `uses the single refreshed registry snapshot for recreate rollback`, and `restores the registry entry when prepared-backup recreation fails`; add module-level tests for the extracted planner if any logic moves.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Shortest read-only check: inspect `src/lib/actions/upgrade-sandboxes.ts` around `prepareBackupRecovery`, `src/lib/state/sandbox.ts` around `validateRebuildRecoveryManifest` and `hasPositiveManagedImageEvidence`, and `src/lib/actions/sandbox/rebuild.ts` around `revalidatePreparedRecoveryBeforeDelete`; the prepared-recovery authority is split across all three TypeScript files plus installer ordering in `scripts/install.sh`.
  • Evidence: Drift analysis reports large hotspot growth: `src/lib/actions/sandbox/rebuild.ts` grows by 147 lines to 1395 lines, `src/lib/state/sandbox.ts` grows by 76 lines to 1832 lines, and `src/lib/actions/sandbox/rebuild-flow.test.ts` grows by 231 lines to 1052 lines. The current diff adds `RebuildSandboxExecutionOptions.recoveryManifest`, `revalidatePreparedRecoveryBeforeDelete`, `prepareBackupRecovery`, and `validateRebuildRecoveryManifest` in separate files.

Workflow run details

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.

Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
@github-actions

github-actions Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

E2E Advisor Recommendation

Required E2E: cloud-onboard, openshell-gateway-upgrade, upgrade-stale-sandbox, sandbox-rebuild
Optional E2E: state-backup-restore, snapshot-commands, onboard-resume, onboard-repair

Dispatch hint: cloud-onboard,openshell-gateway-upgrade,upgrade-stale-sandbox,sandbox-rebuild

Workflow run

Full advisor summary

E2E Recommendation Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required E2E

  • cloud-onboard (high): The installer/onboarding path changed in scripts/install.sh. This hosted live job exercises the real install.sh + OpenShell + cloud OpenClaw onboarding boundary and ensures normal generic onboarding still succeeds after the phase ordering changes.
  • openshell-gateway-upgrade (high): This PR specifically changes installer handling of pre-existing sandboxes around backup-all, OpenShell upgrade, upgrade-sandboxes --auto, and survivor recovery. This existing live job preserves the old-installer/current-installer upgrade boundary with a real survivor sandbox.
  • upgrade-stale-sandbox (high): upgrade-sandboxes.ts and rebuild.ts changed the stale/recovery rebuild path. This job exercises real Docker/OpenShell stale sandbox detection and rebuild through the CLI.
  • sandbox-rebuild (high): rebuildSandbox changed destructive pre-delete checks, backup selection, recreate recovery, registry rollback, shields cleanup, and restore behavior. This live job validates real OpenClaw rebuild state preservation and registry refresh.

Optional E2E

  • state-backup-restore (medium): Useful adjacent confidence for backup/restore state handling, although the changed manifest validation is specifically for rebuild/pre-upgrade recovery rather than the basic workspace backup script flow.
  • snapshot-commands (medium): Optional coverage for snapshot lifecycle behavior adjacent to the changed rebuild backup manifest validation and recovery documentation.
  • onboard-resume (medium): Rebuild still recreates through onboard --resume, so this is useful adjacent coverage for resume behavior. The mandatory onboarding resume rule is not triggered because the PR does not modify src/lib/onboard/machine resume-state orchestration.
  • onboard-repair (medium): Useful adjacent confidence for onboarding repair flows after installer and recovery changes, but not merge-blocking because the touched runtime changes are in installer/rebuild/upgrade rather than the onboard machine repair state machine.

New E2E recommendations

  • installer pre-existing sandbox prepared-backup recovery (high): Existing live jobs cover normal stale rebuild and legacy gateway survivor upgrade, but there is no clear live target that creates a pre-existing registered sandbox, makes it explicitly non-Ready after the host upgrade, verifies install.sh runs upgrade-sandboxes --auto before generic onboarding, restores from the validated latest rebuild backup, and confirms generic onboarding is skipped on recovery failure.
    • Suggested test: Add a live installer-preexisting-nonready-recovery E2E that stages a managed-image sandbox with a validated latest backup, forces a non-Ready OpenShell phase, reruns install.sh, and asserts recovery succeeds before any new generic onboarding; include a failure subcase that exits non-zero and preserves diagnostics.
  • automatic recovery fail-closed provenance (high): The PR adds a security boundary requiring NemoClaw-managed image fingerprint evidence before automatic recreate. Unit tests cover this, but live coverage would better prove custom/pre-fingerprint registered sandboxes are not deleted by installer recovery.
    • Suggested test: Extend the pre-existing sandbox recovery live coverage with a custom-image or pre-fingerprint registry entry and assert install.sh/upgrade-sandboxes refuses automatic recovery without deleting the sandbox or registry entry.
  • prepared backup pre-delete race guard (medium): rebuildSandbox now revalidates registry configuration and latest backup identity immediately before deletion. The current coverage appears unit-level; a live disruption target would catch regressions at the real filesystem/registry boundary.
    • Suggested test: Add a focused live rebuild recovery race test that swaps the latest backup or mutates the registry between initial assessment and destructive delete, then asserts rebuild aborts with the sandbox untouched.

Dispatch hint

  • Workflow: E2E
  • jobs input: cloud-onboard,openshell-gateway-upgrade,upgrade-stale-sandbox,sandbox-rebuild

@github-actions

github-actions Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

E2E Target Recommendation

Required E2E targets: openshell-gateway-upgrade, upgrade-stale-sandbox, rebuild-openclaw
Optional E2E targets: state-backup-restore, sandbox-rebuild

Dispatch required E2E targets:

  • gh workflow run e2e.yaml --ref <pr-head-ref> --field jobs=openshell-gateway-upgrade
  • gh workflow run e2e.yaml --ref <pr-head-ref> --field jobs=upgrade-stale-sandbox
  • gh workflow run e2e.yaml --ref <pr-head-ref> --field jobs=rebuild-openclaw

Workflow run

Full E2E target advisor summary

E2E Target Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required E2E targets

  • openshell-gateway-upgrade: The installer path now runs pre-existing sandbox recovery/upgrade before generic onboarding and changes the OpenShell gateway upgrade handoff. This free-standing live job exercises the real old-install/current-install gateway upgrade path, pre-upgrade backup, registry preservation, and restored survivor sandbox behavior.
    • Dispatch: gh workflow run e2e.yaml --ref <pr-head-ref> --field jobs=openshell-gateway-upgrade
  • upgrade-stale-sandbox: The upgrade-sandboxes implementation and prepared backup recovery path changed, including stale/non-Ready classification and rebuild dispatch. This job is the smallest live E2E coverage for upgrade-sandboxes detection and rebuild of registered stale OpenClaw sandboxes.
    • Dispatch: gh workflow run e2e.yaml --ref <pr-head-ref> --field jobs=upgrade-stale-sandbox
  • rebuild-openclaw: The rebuild flow now validates prepared recovery manifests before destructive delete, skips redundant backup in recovery mode, and restores registry entries on recreate failure. This job exercises the real OpenClaw rebuild boundary with Docker/OpenShell, registry/session state, backup/restore, and recreated sandbox validation.
    • Dispatch: gh workflow run e2e.yaml --ref <pr-head-ref> --field jobs=rebuild-openclaw

Optional E2E targets

  • state-backup-restore: Adjacent coverage for sandbox backup/restore and manifest handling after changes to rebuild recovery manifest validation in src/lib/state/sandbox.ts.
    • Dispatch: gh workflow run e2e.yaml --ref <pr-head-ref> --field jobs=state-backup-restore
  • sandbox-rebuild: Additional OpenClaw rebuild coverage on the standard onboard-then-rebuild path; useful as a lower-complexity companion to rebuild-openclaw for registry metadata refresh and backup hygiene.
    • Dispatch: gh workflow run e2e.yaml --ref <pr-head-ref> --field jobs=sandbox-rebuild

Relevant changed files

  • scripts/install.sh
  • src/lib/actions/sandbox/rebuild.ts
  • src/lib/actions/upgrade-sandboxes.ts
  • src/lib/state/sandbox.ts

@github-actions

github-actions Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor (Nemotron Ultra) — Changes requested

Merge posture: Do not merge yet
Primary next action: Fix PRA-6: Blueprint digest not validated during prepared-backup recovery; then add or justify PRA-T1.
Open items: 6 required · 12 warnings · 3 suggestions · 8 test follow-ups
Since last review: 0 prior items resolved · 14 still apply · 3 new items found

Action checklist

  • PRA-6 Fix: Blueprint digest not validated during prepared-backup recovery in src/lib/state/sandbox.ts:1659
  • PRA-7 Fix: Monolith growth: rebuild-flow.test.ts grew by 231 lines (821→1052) in src/lib/actions/sandbox/rebuild-flow.test.ts:233
  • PRA-8 Fix: Monolith growth: rebuild.ts grew by 147 lines (1248→1395) in src/lib/actions/sandbox/rebuild.ts:1
  • PRA-9 Fix: Monolith growth: sandbox.ts grew by 76 lines (1756→1832) in src/lib/state/sandbox.ts:1566
  • PRA-10 Fix: Installer recovery failure message does not name affected sandbox in scripts/install.sh:2690
  • PRA-11 Fix: Stopped sandboxes with prepared backups are not recovered in src/lib/actions/upgrade-sandboxes.ts:195
  • PRA-1 Resolve or justify: Source-of-truth review needed: install.sh → CLI recovery signal (NEMOCLAW_RESTORE_LATEST_BACKUP_ON_RECREATE env var)
  • PRA-2 Resolve or justify: Source-of-truth review needed: upgrade-sandboxes.ts work array coupling rebuild + recovery paths
  • PRA-3 Resolve or justify: Source-of-truth review needed: rebuild.ts RebuildSandboxExecutionOptions.recoveryManifest public interface
  • PRA-4 Resolve or justify: Source-of-truth review needed: upgrade-sandboxes.ts stopped sandbox warning only when recovery signal unset
  • PRA-5 Resolve or justify: Source-of-truth review needed: validateRebuildRecoveryManifest blueprintDigest validation
  • PRA-12 Resolve or justify: Recovery signal uses environment variable instead of typed internal API in src/lib/actions/upgrade-sandboxes.ts:199
  • PRA-13 Resolve or justify: Public interface exposes internal recovery option in src/lib/actions/sandbox/rebuild.ts:85
  • PRA-14 Resolve or justify: Mixed work array couples two recovery paths with different trust assumptions in src/lib/actions/upgrade-sandboxes.ts:247
  • PRA-15 Resolve or justify: Stopped sandboxes without prepared backups silently skipped when recovery signal set in src/lib/actions/upgrade-sandboxes.ts:210
  • PRA-16 Resolve or justify: Extensive mock scaffolding duplicated across rebuild flow tests in src/lib/actions/sandbox/rebuild-flow.test.ts:233
  • PRA-T1 Add or justify test follow-up: Runtime validation
  • PRA-T2 Add or justify test follow-up: Runtime validation
  • PRA-T3 Add or justify test follow-up: Runtime validation
  • PRA-T4 Add or justify test follow-up: Runtime validation
  • PRA-T5 Add or justify test follow-up: Runtime validation
  • PRA-T6 Add or justify test follow-up: Extensive mock scaffolding duplicated across rebuild flow tests
  • PRA-T7 Add or justify test follow-up: Missing blueprintDigest validation test guardrail
  • PRA-T8 Add or justify test follow-up: install.sh → CLI recovery signal (NEMOCLAW_RESTORE_LATEST_BACKUP_ON_RECREATE env var)
  • PRA-19 In-scope improvement: Single-use config knob NEMOCLAW_RESTORE_LATEST_BACKUP_ON_RECREATE in src/lib/actions/upgrade-sandboxes.ts:199
  • PRA-20 In-scope improvement: Documentation doesn't mention blueprintDigest validation in docs/get-started/quickstart.mdx:113
  • PRA-21 In-scope improvement: Make RebuildSandboxExecutionOptions internal or add runtime assertion in src/lib/actions/sandbox/rebuild.ts:85

Findings index

ID Severity Category Location Required action
PRA-1 Resolve/justify architecture Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
PRA-2 Resolve/justify architecture Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
PRA-3 Resolve/justify architecture Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
PRA-4 Resolve/justify architecture Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
PRA-5 Resolve/justify architecture Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
PRA-6 Required security src/lib/state/sandbox.ts:1659 Add blueprintDigest validation in validateRebuildRecoveryManifest: when persisted.blueprintDigest !== null, require candidate.blueprintDigest === persisted.blueprintDigest. If registry gains a blueprint fingerprint field in future, cross-validate against that as well. Fail closed on mismatch.
PRA-7 Required architecture src/lib/actions/sandbox/rebuild-flow.test.ts:233 Extract shared test utilities to src/lib/actions/sandbox/rebuild-test-helpers.ts with composable factory functions for common mock configurations (registry, sandboxState, session, shields, messaging). Use the helper in rebuild-flow.test.ts and new test files.
PRA-8 Required architecture src/lib/actions/sandbox/rebuild.ts:1 Extract recovery validation to dedicated module src/lib/actions/sandbox/rebuild-recovery.ts with functions: revalidatePreparedRecoveryBeforeDelete, failPreparedRecoveryPreDelete, validateRecoveryManifestIntegrity. Keep rebuild.ts focused on the rebuild orchestration.
PRA-9 Required architecture src/lib/state/sandbox.ts:1566 Extract recovery manifest validation to src/lib/state/rebuild-manifest.ts (or alongside rebuild-recovery.ts) with validateRebuildRecoveryManifest, hasPositiveManagedImageEvidence, RebuildRecoveryManifestValidation type. Keep sandbox.ts focused on backup/restore/list operations.
PRA-10 Required correctness scripts/install.sh:2690 In recover_preexisting_sandboxes_before_onboard, capture stdout/stderr from upgrade-sandboxes call. Parse failed sandbox names from output (pattern "Failed to recover '([^']+)'") and store in array. Pass array to finalize_install/print_done to include in error message: "Failed sandboxes: alpha, beta".
PRA-11 Required correctness src/lib/actions/upgrade-sandboxes.ts:195 Extend backup recovery assessment to include stopped sandboxes (from splitRebuildableSandboxes.stopped) that have validated latest backups and positive managed-image evidence. Assess them with the same prepareBackupRecovery logic. Update notObservedReadyOrNonReady warning to only apply to stopped sandboxes WITHOUT prepared backups.
PRA-12 Resolve/justify security src/lib/actions/upgrade-sandboxes.ts:199 Replace env var with internal TypeScript option: add recoverPreparedBackups?: boolean to UpgradeSandboxesOptions (or create internal upgradeSandboxesWithRecovery function). Installer calls with typed option; CLI --auto path keeps env var for backward compat but internal path uses typed API.
PRA-13 Resolve/justify security src/lib/actions/sandbox/rebuild.ts:85 Make recoveryManifest internal: prefix with underscore (_recoveryManifest) in RebuildSandboxExecutionOptions, or add runtime assertion that it's only used when called from upgrade-sandboxes (check call stack or pass explicit internal flag). Document as @internal in JSDoc.
PRA-14 Resolve/justify architecture src/lib/actions/upgrade-sandboxes.ts:247 Split into separate loops or extract executeRecovery(sandbox, manifest?) with explicit path separation. First loop: stale rebuildable sandboxes (normal rebuild). Second loop: preparedRecoveries (recovery manifest). Keep validation and logging distinct per path.
PRA-15 Resolve/justify correctness src/lib/actions/upgrade-sandboxes.ts:210 Always warn about stopped sandboxes without prepared backups (current behavior when recoverPreparedBackups=false). When recoverPreparedBackups=true, additionally attempt prepared-backup recovery for stopped sandboxes that have validated backups + managed-image evidence. Only skip truly unassessable sandboxes.
PRA-16 Resolve/justify tests src/lib/actions/sandbox/rebuild-flow.test.ts:233 Extract to src/lib/actions/sandbox/rebuild-test-helpers.ts with factory functions: createMockRegistry(), createMockSandboxState(), createMockOnboardSession(), createMockShields(), createHarness(overrides). Use composable overrides instead of monolithic harness.
PRA-17 Resolve/justify security test/snapshot-recovery-validation.test.ts:48 Add test case: candidate manifest pointing to non-existent backup directory (simulating manifest deletion between discovery and revalidation), verify validateRebuildRecoveryManifest returns ok: false with reason "latest backup manifest is missing, malformed, or unsupported".
PRA-18 Resolve/justify tests test/snapshot-recovery-validation.test.ts:1 Add test in test/snapshot-recovery-validation.test.ts after blueprintDigest validation is implemented: candidate with different blueprintDigest than persisted manifest should fail validation.
PRA-19 Improvement architecture src/lib/actions/upgrade-sandboxes.ts:199 Replace with constant: const RECOVER_PREPARED_BACKUPS = true (or internal typed option per PRA-12). Remove env var read. When a second recovery mode is needed, introduce proper enum/config.
PRA-20 Improvement docs docs/get-started/quickstart.mdx:113 Update quickstart.mdx line 113 and lifecycle.mdx line 267 to include "and the backup's blueprint digest matches the persisted manifest" once validation is implemented.

🚨 Required before merge

Address these before merging unless a maintainer explicitly overrides the advisor with rationale.

PRA-6 Required — Blueprint digest not validated during prepared-backup recovery

  • Location: src/lib/state/sandbox.ts:1659
  • Category: security
  • Problem: validateRebuildRecoveryManifest validates sandbox name, agent type, timestamp, and backup path identity but does NOT validate blueprintDigest. A tampered or swapped manifest could substitute a different blueprint (defining base image/Dockerfile) while passing all other checks, enabling supply-chain compromise of the recreated sandbox.
  • Impact: An attacker with write access to the backup directory could replace a manifest's blueprintDigest with one pointing to a malicious blueprint, causing the recovery to rebuild a sandbox with an unintended base image while all identity checks pass.
  • Required action: Add blueprintDigest validation in validateRebuildRecoveryManifest: when persisted.blueprintDigest !== null, require candidate.blueprintDigest === persisted.blueprintDigest. If registry gains a blueprint fingerprint field in future, cross-validate against that as well. Fail closed on mismatch.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Read src/lib/state/sandbox.ts:1659-1720 validateRebuildRecoveryManifest; confirm blueprintDigest is read from persisted manifest but never compared to candidate or registry
  • Missing regression test: Add test in test/snapshot-recovery-validation.test.ts: candidate manifest with different blueprintDigest than persisted manifest should return ok: false with reason mentioning blueprint digest mismatch
  • Done when: The required change is committed and verification passes: Read src/lib/state/sandbox.ts:1659-1720 validateRebuildRecoveryManifest; confirm blueprintDigest is read from persisted manifest but never compared to candidate or registry.
  • Evidence: sandbox.ts:1659 validateRebuildRecoveryManifest checks sandboxName, agentType, timestamp, backupPath, manifest version but never compares blueprintDigest. computeBlueprintDigest() at line 497 computes SHA256 of blueprint.yaml; backup writes it at line 1100; readManifest returns it at line 1637. No validation occurs.

PRA-7 Required — Monolith growth: rebuild-flow.test.ts grew by 231 lines (821→1052)

  • Location: src/lib/actions/sandbox/rebuild-flow.test.ts:233
  • Category: architecture
  • Problem: The test file added 7 new recovery tests with extensive mock scaffolding, pushing it over 1000 lines. Shared mock setup for registry, sandboxState, onboardSession, shields, messaging is duplicated across tests and will be duplicated in new test files.
  • Impact: Maintenance burden increases; any change to rebuild dependencies requires updating mocks in many places. New test files (upgrade-sandboxes-recovery.test.ts, snapshot-recovery-validation.test.ts) already replicate similar patterns.
  • Required action: Extract shared test utilities to src/lib/actions/sandbox/rebuild-test-helpers.ts with composable factory functions for common mock configurations (registry, sandboxState, session, shields, messaging). Use the helper in rebuild-flow.test.ts and new test files.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Count lines in src/lib/actions/sandbox/rebuild-flow.test.ts (currently 1052); verify rebuild-test-helpers.ts does not exist yet
  • Missing regression test: N/A - architecture finding; no regression test needed. Extraction should not change test behavior.
  • Done when: The required change is committed and verification passes: Count lines in src/lib/actions/sandbox/rebuild-flow.test.ts (currently 1052); verify rebuild-test-helpers.ts does not exist yet.
  • Evidence: driftEvidence shows baseLines: 821, headLines: 1052, delta: 231, severity: blocker. PRA-8 from previous review unresolved.

PRA-8 Required — Monolith growth: rebuild.ts grew by 147 lines (1248→1395)

  • Location: src/lib/actions/sandbox/rebuild.ts:1
  • Category: architecture
  • Problem: Recovery validation logic (revalidatePreparedRecoveryBeforeDelete, failPreparedRecoveryPreDelete, recoveryManifest handling, pre-delete revalidation) added ~150 lines to an already large file. Recovery validation is a distinct concern from normal rebuild flow.
  • Impact: Cognitive complexity increases; recovery logic intertwined with rebuild flow makes both harder to audit. Security-critical validation mixed with operational rebuild steps.
  • Required action: Extract recovery validation to dedicated module src/lib/actions/sandbox/rebuild-recovery.ts with functions: revalidatePreparedRecoveryBeforeDelete, failPreparedRecoveryPreDelete, validateRecoveryManifestIntegrity. Keep rebuild.ts focused on the rebuild orchestration.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Count lines in src/lib/actions/sandbox/rebuild.ts (currently 1395); verify rebuild-recovery.ts does not exist
  • Missing regression test: N/A - architecture finding; extraction should preserve all existing test behavior
  • Done when: The required change is committed and verification passes: Count lines in src/lib/actions/sandbox/rebuild.ts (currently 1395); verify rebuild-recovery.ts does not exist.
  • Evidence: driftEvidence shows baseLines: 1248, headLines: 1395, delta: 147, severity: blocker. PRA-9 from previous review unresolved.

PRA-9 Required — Monolith growth: sandbox.ts grew by 76 lines (1756→1832)

  • Location: src/lib/state/sandbox.ts:1566
  • Category: architecture
  • Problem: Added validateRebuildRecoveryManifest, hasPositiveManagedImageEvidence, and related types to an already large state module. Recovery validation is a distinct domain from general sandbox state management.
  • Impact: sandbox.ts becomes a catch-all for backup, restore, manifest, validation, listing, snapshot matching. Recovery validation should live alongside rebuild recovery logic.
  • Required action: Extract recovery manifest validation to src/lib/state/rebuild-manifest.ts (or alongside rebuild-recovery.ts) with validateRebuildRecoveryManifest, hasPositiveManagedImageEvidence, RebuildRecoveryManifestValidation type. Keep sandbox.ts focused on backup/restore/list operations.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Count lines in src/lib/state/sandbox.ts (currently 1832); verify rebuild-manifest.ts does not exist
  • Missing regression test: N/A - architecture finding; extraction should preserve all existing test behavior
  • Done when: The required change is committed and verification passes: Count lines in src/lib/state/sandbox.ts (currently 1832); verify rebuild-manifest.ts does not exist.
  • Evidence: driftEvidence shows baseLines: 1756, headLines: 1832, delta: 76, severity: blocker. PRA-10 from previous review unresolved.

PRA-10 Required — Installer recovery failure message does not name affected sandbox

  • Location: scripts/install.sh:2690
  • Category: correctness
  • Problem: recover_preexisting_sandboxes_before_onboard calls upgrade-sandboxes --auto but only checks exit code. upgrade-sandboxes.ts logs failed sandbox names (e.g., "Failed to recover 'alpha': ...") to stderr, but installer doesn't capture this output. print_done() shows generic "See the messages above for the affected sandbox name" without actually including the name.
  • Impact: Operators must scroll through noisy CI output to identify which sandbox failed recovery, delaying incident response and manual recovery.
  • Required action: In recover_preexisting_sandboxes_before_onboard, capture stdout/stderr from upgrade-sandboxes call. Parse failed sandbox names from output (pattern "Failed to recover '([^']+)'") and store in array. Pass array to finalize_install/print_done to include in error message: "Failed sandboxes: alpha, beta".
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Run installer with pre-existing non-Ready sandbox that fails recovery; verify final error output includes sandbox name(s). Check install.sh:2670-2690 recover_preexisting_sandboxes_before_onboard and print_done at line 555
  • Missing regression test: Add test in test/install-preexisting-sandbox-recovery.test.ts asserting failed sandbox name appears in installer stderr/output when upgrade-sandboxes fails
  • Done when: The required change is committed and verification passes: Run installer with pre-existing non-Ready sandbox that fails recovery; verify final error output includes sandbox name(s). Check install.sh:2670-2690 recover_preexisting_sandboxes_before_onboard and print_done at line 555.
  • Evidence: install.sh:2770 recover_preexisting_sandboxes_before_onboard only checks exit code; print_done at line ~555 outputs generic message. upgrade-sandboxes.ts:270 logs failed sandbox names per-item. PRA-6 from previous review unresolved.

PRA-11 Required — Stopped sandboxes with prepared backups are not recovered

  • Location: src/lib/actions/upgrade-sandboxes.ts:195
  • Category: correctness
  • Problem: Only sandboxes explicitly observed in live list with non-Ready phase (nonReadyLiveNames) are assessed for prepared-backup recovery. Stopped sandboxes (absent from live list entirely) with validated latest backups and positive managed-image evidence are filtered into notObservedReadyOrNonReady and skipped with generic warning, even when they have all required evidence.
  • Impact: Pre-upgrade sandboxes that were stopped before upgrade cannot auto-recover, leaving user data inaccessible until manual rebuild. This contradicts the PR goal of recovering pre-existing sandboxes.
  • Required action: Extend backup recovery assessment to include stopped sandboxes (from splitRebuildableSandboxes.stopped) that have validated latest backups and positive managed-image evidence. Assess them with the same prepareBackupRecovery logic. Update notObservedReadyOrNonReady warning to only apply to stopped sandboxes WITHOUT prepared backups.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Read upgrade-sandboxes.ts:167 nonReadyLiveNames filter and 240 splitRebuildableSandboxes; trace how stopped sandboxes flow to notObservedReadyOrNonReady at line 210
  • Missing regression test: Add test in src/lib/actions/upgrade-sandboxes-recovery.test.ts: stopped sandbox with validated backup and managed-image evidence should be recovered (rebuildSandbox called with recoveryManifest)
  • Done when: The required change is committed and verification passes: Read upgrade-sandboxes.ts:167 nonReadyLiveNames filter and 240 splitRebuildableSandboxes; trace how stopped sandboxes flow to notObservedReadyOrNonReady at line 210.
  • Evidence: upgrade-sandboxes.ts:167 nonReadyLiveNames only includes explicitly observed non-Ready phases. Line 210 notObservedReadyOrNonReady = stopped.filter(s => !assessedRecoveryNames.has(s.name)) skips all stopped sandboxes. PRA-7/PRA-15 from previous review unresolved.
Review findings by urgency: 6 required fixes, 12 items to resolve/justify, 3 in-scope improvements

⚠️ Resolve or justify before merge

Investigate these in the current review; either fix them, explain why they are not applicable, or document the accepted risk.

PRA-1 Resolve/justify — Source-of-truth review needed: install.sh → CLI recovery signal (NEMOCLAW_RESTORE_LATEST_BACKUP_ON_RECREATE env var)

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: upgrade-sandboxes-recovery.test.ts (7 tests) + install-preexisting-sandbox-recovery.test.ts (3 tests) guard the handoff
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: upgrade-sandboxes.ts:188-198 comment documents source boundary. PRA-1 from previous review unresolved.

PRA-2 Resolve/justify — Source-of-truth review needed: upgrade-sandboxes.ts work array coupling rebuild + recovery paths

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: upgrade-sandboxes-recovery.test.ts covers both paths in same loop
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: upgrade-sandboxes.ts:247 work array; PRA-2/PRA-14 from previous review

PRA-3 Resolve/justify — Source-of-truth review needed: rebuild.ts RebuildSandboxExecutionOptions.recoveryManifest public interface

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: rebuild-flow.test.ts uses recoveryManifest in 7 new tests
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: rebuild.ts:85 interface; PRA-3/PRA-13 from previous review

PRA-4 Resolve/justify — Source-of-truth review needed: upgrade-sandboxes.ts stopped sandbox warning only when recovery signal unset

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: upgrade-sandboxes-recovery.test.ts warns for absent-from-gateway but not for stopped-without-backup
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: upgrade-sandboxes.ts:210 notObservedReadyOrNonReady; PRA-4/PRA-15 from previous review

PRA-5 Resolve/justify — Source-of-truth review needed: validateRebuildRecoveryManifest blueprintDigest validation

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as missing.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: snapshot-recovery-validation.test.ts has no blueprintDigest tests (PRA-24)
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: sandbox.ts:1659 validateRebuildRecoveryManifest reads blueprintDigest but never validates. PRA-5/PRA-11 from previous review.

PRA-12 Resolve/justify — Recovery signal uses environment variable instead of typed internal API

  • Location: src/lib/actions/upgrade-sandboxes.ts:199
  • Category: security
  • Problem: recoverPreparedBackups = process.env.NEMOCLAW_RESTORE_LATEST_BACKUP_ON_RECREATE === "1" uses an environment variable as the control signal between installer and upgrade-sandboxes. Any process setting this env var could trigger the recovery path, bypassing the installer's controlled flow.
  • Impact: Trust boundary bypass: a compromised or misconfigured process could set this env var and cause upgrade-sandboxes to attempt prepared-backup recovery on arbitrary sandboxes without the installer's preflight checks (backup-all, legacy gateway retirement).
  • Recommended action: Replace env var with internal TypeScript option: add recoverPreparedBackups?: boolean to UpgradeSandboxesOptions (or create internal upgradeSandboxesWithRecovery function). Installer calls with typed option; CLI --auto path keeps env var for backward compat but internal path uses typed API.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read upgrade-sandboxes.ts:199 and normalizeUpgradeSandboxesOptions in domain/lifecycle/options.ts; confirm env var is the only signal
  • Missing regression test: N/A - architecture finding; add unit test that upgradeSandboxes with recoverPreparedBackups=true behaves identically to env var path
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read upgrade-sandboxes.ts:199 and normalizeUpgradeSandboxesOptions in domain/lifecycle/options.ts; confirm env var is the only signal.
  • Evidence: upgrade-sandboxes.ts:199 uses process.env directly. install.sh exports NEMOCLAW_RESTORE_LATEST_BACKUP_ON_RECREATE=1 before calling. PRA-12 from previous review unresolved.

PRA-13 Resolve/justify — Public interface exposes internal recovery option

  • Location: src/lib/actions/sandbox/rebuild.ts:85
  • Category: security
  • Problem: RebuildSandboxExecutionOptions.recoveryManifest is documented as "Internal installer recovery input; never exposed as a CLI option" but is part of the exported public interface. External callers could pass a crafted manifest to bypass normal rebuild validation.
  • Impact: API surface leak: malicious or buggy caller could supply a recoveryManifest that skips backup creation and validation, potentially deleting a sandbox and recreating from an attacker-controlled manifest.
  • Recommended action: Make recoveryManifest internal: prefix with underscore (_recoveryManifest) in RebuildSandboxExecutionOptions, or add runtime assertion that it's only used when called from upgrade-sandboxes (check call stack or pass explicit internal flag). Document as @internal in JSDoc.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read rebuild.ts:85 RebuildSandboxExecutionOptions interface and line 703 where it's used; confirm no internal guard
  • Missing regression test: Add test in rebuild-flow.test.ts: calling rebuildSandbox with recoveryManifest from non-upgrade-sandboxes context should fail or be ignored (depending on chosen fix)
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read rebuild.ts:85 RebuildSandboxExecutionOptions interface and line 703 where it's used; confirm no internal guard.
  • Evidence: rebuild.ts:85 exports RebuildSandboxExecutionOptions with recoveryManifest. Called from upgrade-sandboxes.ts:255 with recoveryManifest. PRA-13 from previous review unresolved.

PRA-14 Resolve/justify — Mixed work array couples two recovery paths with different trust assumptions

  • Location: src/lib/actions/upgrade-sandboxes.ts:247
  • Category: architecture
  • Problem: work array combines rebuildable (manifest: null, trust live sandbox state) and preparedRecoveries (manifest: RebuildManifest, trust pre-upgrade backup). Single loop with verb="Rebuild"/"Recover" conflates two distinct trust boundaries: live sandbox backup vs. pre-upgrade backup.
  • Impact: Harder to audit: a bug in one path could affect the other. Different validation requirements (live backup vs. pre-validated manifest) are handled by same code path with conditional logic. Violates separation of concerns for security-critical operations.
  • Recommended action: Split into separate loops or extract executeRecovery(sandbox, manifest?) with explicit path separation. First loop: stale rebuildable sandboxes (normal rebuild). Second loop: preparedRecoveries (recovery manifest). Keep validation and logging distinct per path.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read upgrade-sandboxes.ts:247 work array construction and 250-270 loop; trace how manifest null vs object changes behavior
  • Missing regression test: N/A - architecture finding; refactoring should preserve behavior
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read upgrade-sandboxes.ts:247 work array construction and 250-270 loop; trace how manifest null vs object changes behavior.
  • Evidence: upgrade-sandboxes.ts:247 work = [...rebuildable.map(manifest: null), ...preparedRecoveries.map(manifest: recovery.manifest)]. PRA-14 from previous review unresolved.

PRA-15 Resolve/justify — Stopped sandboxes without prepared backups silently skipped when recovery signal set

  • Location: src/lib/actions/upgrade-sandboxes.ts:210
  • Category: correctness
  • Problem: When recoverPreparedBackups=true, stopped sandboxes without prepared backups are filtered into notObservedReadyOrNonReady and only warned about. The warning message "verify their recorded gateway or start them first" is misleading — these sandboxes have no backup to recover. The warning should always appear regardless of recovery signal; the signal should only control whether prepared-backup recoveries are attempted.
  • Impact: Operators see warning about "not observed on selected gateway" for stopped sandboxes but don't understand these are permanently skipped from recovery. No indication that they need manual rebuild.
  • Recommended action: Always warn about stopped sandboxes without prepared backups (current behavior when recoverPreparedBackups=false). When recoverPreparedBackups=true, additionally attempt prepared-backup recovery for stopped sandboxes that have validated backups + managed-image evidence. Only skip truly unassessable sandboxes.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read upgrade-sandboxes.ts:210 notObservedReadyOrNonReady filter and 212 console.log; trace how recoverPreparedBackups flag affects this path
  • Missing regression test: Add test in upgrade-sandboxes-recovery.test.ts: stopped sandbox without backup should warn regardless of NEMOCLAW_RESTORE_LATEST_BACKUP_ON_RECREATE value
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read upgrade-sandboxes.ts:210 notObservedReadyOrNonReady filter and 212 console.log; trace how recoverPreparedBackups flag affects this path.
  • Evidence: PRA-15 from previous review: "Always warn about stopped sandboxes without prepared backups regardless of recoverPreparedBackups flag. The flag should only affect whether prepared-backup recoveries are attempted."

PRA-16 Resolve/justify — Extensive mock scaffolding duplicated across rebuild flow tests

  • Location: src/lib/actions/sandbox/rebuild-flow.test.ts:233
  • Category: tests
  • Problem: createRebuildFlowHarness mocks 20+ modules (registry, sandboxState, onboardSession, shields, messaging, etc.) with 100+ lines of setup. Each new test file (upgrade-sandboxes-recovery.test.ts, snapshot-recovery-validation.test.ts) replicates similar mocking patterns.
  • Impact: High maintenance cost; changes to rebuild dependencies require updating mocks in multiple files. Inconsistent mock configurations can mask integration bugs.
  • Recommended action: Extract to src/lib/actions/sandbox/rebuild-test-helpers.ts with factory functions: createMockRegistry(), createMockSandboxState(), createMockOnboardSession(), createMockShields(), createHarness(overrides). Use composable overrides instead of monolithic harness.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Count vi.spyOn/mockImplementation lines in createRebuildFlowHarness (lines 233-380); compare with mock setup in upgrade-sandboxes-recovery.test.ts:60-120
  • Missing regression test: N/A - test architecture finding
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Count vi.spyOn/mockImplementation lines in createRebuildFlowHarness (lines 233-380); compare with mock setup in upgrade-sandboxes-recovery.test.ts:60-120.
  • Evidence: PRA-16 from previous review. simplificationSignals shows test_over_scaffold at lines 233, 235, 251, 583, 601, 610, 654 in rebuild-flow.test.ts.

PRA-17 Resolve/justify — Missing negative test for readManifest null/parse-error in validateRebuildRecoveryManifest

  • Location: test/snapshot-recovery-validation.test.ts:48
  • Category: security
  • Problem: validateRebuildRecoveryManifest calls readManifest(candidateBackupPath) which can return null if manifest disappears or becomes malformed between getLatestBackup and revalidation. Tests cover getLatestBackup returning null (line 56) and persisted manifest disappearing after discovery (line 84), but NOT the revalidation path in validateRebuildRecoveryManifest where readManifest returns null for the candidate path.
  • Impact: TOCTOU gap: if backup manifest is deleted/corrupted after getLatestBackup but before validateRebuildRecoveryManifest's readManifest call, behavior is untested. Could return ok: true with null manifest or throw.
  • Recommended action: Add test case: candidate manifest pointing to non-existent backup directory (simulating manifest deletion between discovery and revalidation), verify validateRebuildRecoveryManifest returns ok: false with reason "latest backup manifest is missing, malformed, or unsupported".
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Run test/snapshot-recovery-validation.test.ts; confirm no test exercises validateRebuildRecoveryManifest with candidate whose backup path has no manifest file
  • Missing regression test: test/snapshot-recovery-validation.test.ts: it('rejects candidate manifest when persisted manifest missing at revalidation time')
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Run test/snapshot-recovery-validation.test.ts; confirm no test exercises validateRebuildRecoveryManifest with candidate whose backup path has no manifest file.
  • Evidence: test/snapshot-recovery-validation.test.ts:56 tests getLatestBackup with missing manifest; line 84 tests persisted manifest disappearing after getLatestBackup. validateRebuildRecoveryManifest's readManifest call at line 1685 is not tested for null return. PRA-17/T7 from previous review.

PRA-18 Resolve/justify — Missing blueprintDigest validation test guardrail

  • Location: test/snapshot-recovery-validation.test.ts:1
  • Category: tests
  • Problem: No test exists for blueprintDigest validation because the validation doesn't exist (PRA-11). Once blueprintDigest validation is added, a test must verify mismatch detection.
  • Impact: Without test, blueprintDigest validation regression would go undetected. Supply-chain trust boundary unguarded.
  • Recommended action: Add test in test/snapshot-recovery-validation.test.ts after blueprintDigest validation is implemented: candidate with different blueprintDigest than persisted manifest should fail validation.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Search test/snapshot-recovery-validation.test.ts for blueprintDigest; only appears in test fixture at line 36 as null
  • Missing regression test: test/snapshot-recovery-validation.test.ts: it('rejects blueprintDigest mismatch between candidate and persisted manifest')
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Search test/snapshot-recovery-validation.test.ts for blueprintDigest; only appears in test fixture at line 36 as null.
  • Evidence: PRA-24/T8 from previous review. blueprintDigest validation missing entirely (S1 finding).

💡 In-scope improvements

These are lower-risk, not throwaway. Prefer fixing them in this PR when they are local to changed code; defer only with rationale or a linked follow-up.

PRA-19 Improvement — Single-use config knob NEMOCLAW_RESTORE_LATEST_BACKUP_ON_RECREATE

  • Location: src/lib/actions/upgrade-sandboxes.ts:199
  • Category: architecture
  • Problem: Environment variable used only once in codebase as boolean flag. No second value planned. Adds surface area for misconfiguration (typo in env var name silently disables recovery).
  • Impact: Unnecessary configuration complexity; env var can be set by any process, not just installer.
  • Suggested action: Replace with constant: const RECOVER_PREPARED_BACKUPS = true (or internal typed option per PRA-12). Remove env var read. When a second recovery mode is needed, introduce proper enum/config.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: grep -r NEMOCLAW_RESTORE_LATEST_BACKUP_ON_RECREATE in repo; only appears in install.sh export and upgrade-sandboxes.ts read
  • Missing regression test: N/A - simplification
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: PRA-18 from previous review. simplificationSignals shows single_use_config at upgrade-sandboxes.ts:199.

PRA-20 Improvement — Documentation doesn't mention blueprintDigest validation

  • Location: docs/get-started/quickstart.mdx:113
  • Category: docs
  • Problem: Quickstart docs describe recovery validation as "backup identity matches and registry entry has NemoClaw-managed image fingerprint" but omit blueprintDigest check (which doesn't exist yet). After blueprintDigest validation is added (PRA-11), docs must be updated.
  • Impact: Docs drift from implementation; operators don't know full validation criteria.
  • Suggested action: Update quickstart.mdx line 113 and lifecycle.mdx line 267 to include "and the backup's blueprint digest matches the persisted manifest" once validation is implemented.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Read quickstart.mdx:113 and lifecycle.mdx:267; confirm blueprintDigest not mentioned
  • Missing regression test: N/A - doc finding
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: PRA-19 from previous review. quickstart.mdx:115 says 'backup identity matches (sandbox name, agent, timestamp, path) and positive managed-image evidence'. No blueprintDigest.

PRA-21 Improvement — Make RebuildSandboxExecutionOptions internal or add runtime assertion

  • Location: src/lib/actions/sandbox/rebuild.ts:85
  • Category: architecture
  • Problem: RebuildSandboxExecutionOptions.recoveryManifest is public but documented as internal. Two options: prefix with underscore (_recoveryManifest) to signal internal-only, or add runtime assertion that it's only used when called from upgrade-sandboxes (check call stack or pass explicit internal flag).
  • Impact: Public API surface includes internal-only option, encouraging misuse.
  • Suggested action: Prefix with underscore: _recoveryManifest?. Add JSDoc @internal. Or add internal flag: internalRecovery?: boolean and assert _recoveryManifest only when internalRecovery=true.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Read rebuild.ts:85 interface and line 703 usage; confirm no external consumers
  • Missing regression test: N/A - API design finding
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: PRA-13 from previous review. Interface exported from rebuild.ts.
Simplification opportunities: 2 possible cuts, net -5 lines possible

These are safe simplification checks only. Do not remove validation, security controls, data-loss prevention, or required tests.

  • PRA-19 yagni (src/lib/actions/upgrade-sandboxes.ts:199): process.env.NEMOCLAW_RESTORE_LATEST_BACKUP_ON_RECREATE === "1" env var check
    • Replacement: const RECOVER_PREPARED_BACKUPS = true (or internal typed option)
    • Net: -5 lines
    • Safety boundary: Must preserve installer→upgrade-sandboxes signal; replace with typed internal API per PRA-12
  • PRA-21 delete (src/lib/actions/sandbox/rebuild.ts:85): recoveryManifest?: sandboxState.RebuildManifest from public RebuildSandboxExecutionOptions
    • Replacement: _recoveryManifest?: sandboxState.RebuildManifest (internal only) + runtime assertion
    • Net: 0 lines
    • Safety boundary: Must not break upgrade-sandboxes.ts call site; internal flag or call-stack check required
Test follow-ups to resolve or justify

If these cover changed behavior, prefer adding them in this PR; otherwise state why existing coverage is enough or link the follow-up.

  • PRA-T1 Runtime validation — test/snapshot-recovery-validation.test.ts: validateRebuildRecoveryManifest rejects candidate with missing persisted manifest at revalidation time. Runtime/sandbox/infrastructure paths need behavioral runtime validation: docs/get-started/quickstart.mdx, docs/manage-sandboxes/lifecycle.mdx, scripts/install.sh, src/lib/actions/sandbox/rebuild.ts, src/lib/actions/upgrade-sandboxes.ts, src/lib/state/sandbox.ts. Unit tests cover validation logic well but installer/CLI integration requires runtime verification.
  • PRA-T2 Runtime validation — test/snapshot-recovery-validation.test.ts: validateRebuildRecoveryManifest rejects blueprintDigest mismatch between candidate and persisted manifest. Runtime/sandbox/infrastructure paths need behavioral runtime validation: docs/get-started/quickstart.mdx, docs/manage-sandboxes/lifecycle.mdx, scripts/install.sh, src/lib/actions/sandbox/rebuild.ts, src/lib/actions/upgrade-sandboxes.ts, src/lib/state/sandbox.ts. Unit tests cover validation logic well but installer/CLI integration requires runtime verification.
  • PRA-T3 Runtime validation — test/install-preexisting-sandbox-recovery.test.ts: installer final error message includes failed sandbox name from upgrade-sandboxes stderr. Runtime/sandbox/infrastructure paths need behavioral runtime validation: docs/get-started/quickstart.mdx, docs/manage-sandboxes/lifecycle.mdx, scripts/install.sh, src/lib/actions/sandbox/rebuild.ts, src/lib/actions/upgrade-sandboxes.ts, src/lib/state/sandbox.ts. Unit tests cover validation logic well but installer/CLI integration requires runtime verification.
  • PRA-T4 Runtime validation — src/lib/actions/upgrade-sandboxes-recovery.test.ts: recovers stopped sandbox with validated backup and managed-image evidence. Runtime/sandbox/infrastructure paths need behavioral runtime validation: docs/get-started/quickstart.mdx, docs/manage-sandboxes/lifecycle.mdx, scripts/install.sh, src/lib/actions/sandbox/rebuild.ts, src/lib/actions/upgrade-sandboxes.ts, src/lib/state/sandbox.ts. Unit tests cover validation logic well but installer/CLI integration requires runtime verification.
  • PRA-T5 Runtime validation — src/lib/actions/upgrade-sandboxes-recovery.test.ts: warns about stopped sandboxes without prepared backups regardless of recovery signal. Runtime/sandbox/infrastructure paths need behavioral runtime validation: docs/get-started/quickstart.mdx, docs/manage-sandboxes/lifecycle.mdx, scripts/install.sh, src/lib/actions/sandbox/rebuild.ts, src/lib/actions/upgrade-sandboxes.ts, src/lib/state/sandbox.ts. Unit tests cover validation logic well but installer/CLI integration requires runtime verification.
  • PRA-T6 Extensive mock scaffolding duplicated across rebuild flow tests — Extract to src/lib/actions/sandbox/rebuild-test-helpers.ts with factory functions: createMockRegistry(), createMockSandboxState(), createMockOnboardSession(), createMockShields(), createHarness(overrides). Use composable overrides instead of monolithic harness.
  • PRA-T7 Missing blueprintDigest validation test guardrail — Add test in test/snapshot-recovery-validation.test.ts after blueprintDigest validation is implemented: candidate with different blueprintDigest than persisted manifest should fail validation.
  • PRA-T8 install.sh → CLI recovery signal (NEMOCLAW_RESTORE_LATEST_BACKUP_ON_RECREATE env var) — upgrade-sandboxes-recovery.test.ts (7 tests) + install-preexisting-sandbox-recovery.test.ts (3 tests) guard the handoff. upgrade-sandboxes.ts:188-198 comment documents source boundary. PRA-1 from previous review unresolved.
Since last review details

Current findings, using the urgency labels above:

PRA-1 Resolve/justify — Source-of-truth review needed: install.sh → CLI recovery signal (NEMOCLAW_RESTORE_LATEST_BACKUP_ON_RECREATE env var)

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: upgrade-sandboxes-recovery.test.ts (7 tests) + install-preexisting-sandbox-recovery.test.ts (3 tests) guard the handoff
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: upgrade-sandboxes.ts:188-198 comment documents source boundary. PRA-1 from previous review unresolved.

PRA-2 Resolve/justify — Source-of-truth review needed: upgrade-sandboxes.ts work array coupling rebuild + recovery paths

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: upgrade-sandboxes-recovery.test.ts covers both paths in same loop
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: upgrade-sandboxes.ts:247 work array; PRA-2/PRA-14 from previous review

PRA-3 Resolve/justify — Source-of-truth review needed: rebuild.ts RebuildSandboxExecutionOptions.recoveryManifest public interface

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: rebuild-flow.test.ts uses recoveryManifest in 7 new tests
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: rebuild.ts:85 interface; PRA-3/PRA-13 from previous review

PRA-4 Resolve/justify — Source-of-truth review needed: upgrade-sandboxes.ts stopped sandbox warning only when recovery signal unset

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: upgrade-sandboxes-recovery.test.ts warns for absent-from-gateway but not for stopped-without-backup
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: upgrade-sandboxes.ts:210 notObservedReadyOrNonReady; PRA-4/PRA-15 from previous review

PRA-5 Resolve/justify — Source-of-truth review needed: validateRebuildRecoveryManifest blueprintDigest validation

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as missing.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: snapshot-recovery-validation.test.ts has no blueprintDigest tests (PRA-24)
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: sandbox.ts:1659 validateRebuildRecoveryManifest reads blueprintDigest but never validates. PRA-5/PRA-11 from previous review.

PRA-6 Required — Blueprint digest not validated during prepared-backup recovery

  • Location: src/lib/state/sandbox.ts:1659
  • Category: security
  • Problem: validateRebuildRecoveryManifest validates sandbox name, agent type, timestamp, and backup path identity but does NOT validate blueprintDigest. A tampered or swapped manifest could substitute a different blueprint (defining base image/Dockerfile) while passing all other checks, enabling supply-chain compromise of the recreated sandbox.
  • Impact: An attacker with write access to the backup directory could replace a manifest's blueprintDigest with one pointing to a malicious blueprint, causing the recovery to rebuild a sandbox with an unintended base image while all identity checks pass.
  • Required action: Add blueprintDigest validation in validateRebuildRecoveryManifest: when persisted.blueprintDigest !== null, require candidate.blueprintDigest === persisted.blueprintDigest. If registry gains a blueprint fingerprint field in future, cross-validate against that as well. Fail closed on mismatch.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Read src/lib/state/sandbox.ts:1659-1720 validateRebuildRecoveryManifest; confirm blueprintDigest is read from persisted manifest but never compared to candidate or registry
  • Missing regression test: Add test in test/snapshot-recovery-validation.test.ts: candidate manifest with different blueprintDigest than persisted manifest should return ok: false with reason mentioning blueprint digest mismatch
  • Done when: The required change is committed and verification passes: Read src/lib/state/sandbox.ts:1659-1720 validateRebuildRecoveryManifest; confirm blueprintDigest is read from persisted manifest but never compared to candidate or registry.
  • Evidence: sandbox.ts:1659 validateRebuildRecoveryManifest checks sandboxName, agentType, timestamp, backupPath, manifest version but never compares blueprintDigest. computeBlueprintDigest() at line 497 computes SHA256 of blueprint.yaml; backup writes it at line 1100; readManifest returns it at line 1637. No validation occurs.

PRA-7 Required — Monolith growth: rebuild-flow.test.ts grew by 231 lines (821→1052)

  • Location: src/lib/actions/sandbox/rebuild-flow.test.ts:233
  • Category: architecture
  • Problem: The test file added 7 new recovery tests with extensive mock scaffolding, pushing it over 1000 lines. Shared mock setup for registry, sandboxState, onboardSession, shields, messaging is duplicated across tests and will be duplicated in new test files.
  • Impact: Maintenance burden increases; any change to rebuild dependencies requires updating mocks in many places. New test files (upgrade-sandboxes-recovery.test.ts, snapshot-recovery-validation.test.ts) already replicate similar patterns.
  • Required action: Extract shared test utilities to src/lib/actions/sandbox/rebuild-test-helpers.ts with composable factory functions for common mock configurations (registry, sandboxState, session, shields, messaging). Use the helper in rebuild-flow.test.ts and new test files.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Count lines in src/lib/actions/sandbox/rebuild-flow.test.ts (currently 1052); verify rebuild-test-helpers.ts does not exist yet
  • Missing regression test: N/A - architecture finding; no regression test needed. Extraction should not change test behavior.
  • Done when: The required change is committed and verification passes: Count lines in src/lib/actions/sandbox/rebuild-flow.test.ts (currently 1052); verify rebuild-test-helpers.ts does not exist yet.
  • Evidence: driftEvidence shows baseLines: 821, headLines: 1052, delta: 231, severity: blocker. PRA-8 from previous review unresolved.

PRA-8 Required — Monolith growth: rebuild.ts grew by 147 lines (1248→1395)

  • Location: src/lib/actions/sandbox/rebuild.ts:1
  • Category: architecture
  • Problem: Recovery validation logic (revalidatePreparedRecoveryBeforeDelete, failPreparedRecoveryPreDelete, recoveryManifest handling, pre-delete revalidation) added ~150 lines to an already large file. Recovery validation is a distinct concern from normal rebuild flow.
  • Impact: Cognitive complexity increases; recovery logic intertwined with rebuild flow makes both harder to audit. Security-critical validation mixed with operational rebuild steps.
  • Required action: Extract recovery validation to dedicated module src/lib/actions/sandbox/rebuild-recovery.ts with functions: revalidatePreparedRecoveryBeforeDelete, failPreparedRecoveryPreDelete, validateRecoveryManifestIntegrity. Keep rebuild.ts focused on the rebuild orchestration.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Count lines in src/lib/actions/sandbox/rebuild.ts (currently 1395); verify rebuild-recovery.ts does not exist
  • Missing regression test: N/A - architecture finding; extraction should preserve all existing test behavior
  • Done when: The required change is committed and verification passes: Count lines in src/lib/actions/sandbox/rebuild.ts (currently 1395); verify rebuild-recovery.ts does not exist.
  • Evidence: driftEvidence shows baseLines: 1248, headLines: 1395, delta: 147, severity: blocker. PRA-9 from previous review unresolved.

PRA-9 Required — Monolith growth: sandbox.ts grew by 76 lines (1756→1832)

  • Location: src/lib/state/sandbox.ts:1566
  • Category: architecture
  • Problem: Added validateRebuildRecoveryManifest, hasPositiveManagedImageEvidence, and related types to an already large state module. Recovery validation is a distinct domain from general sandbox state management.
  • Impact: sandbox.ts becomes a catch-all for backup, restore, manifest, validation, listing, snapshot matching. Recovery validation should live alongside rebuild recovery logic.
  • Required action: Extract recovery manifest validation to src/lib/state/rebuild-manifest.ts (or alongside rebuild-recovery.ts) with validateRebuildRecoveryManifest, hasPositiveManagedImageEvidence, RebuildRecoveryManifestValidation type. Keep sandbox.ts focused on backup/restore/list operations.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Count lines in src/lib/state/sandbox.ts (currently 1832); verify rebuild-manifest.ts does not exist
  • Missing regression test: N/A - architecture finding; extraction should preserve all existing test behavior
  • Done when: The required change is committed and verification passes: Count lines in src/lib/state/sandbox.ts (currently 1832); verify rebuild-manifest.ts does not exist.
  • Evidence: driftEvidence shows baseLines: 1756, headLines: 1832, delta: 76, severity: blocker. PRA-10 from previous review unresolved.

PRA-10 Required — Installer recovery failure message does not name affected sandbox

  • Location: scripts/install.sh:2690
  • Category: correctness
  • Problem: recover_preexisting_sandboxes_before_onboard calls upgrade-sandboxes --auto but only checks exit code. upgrade-sandboxes.ts logs failed sandbox names (e.g., "Failed to recover 'alpha': ...") to stderr, but installer doesn't capture this output. print_done() shows generic "See the messages above for the affected sandbox name" without actually including the name.
  • Impact: Operators must scroll through noisy CI output to identify which sandbox failed recovery, delaying incident response and manual recovery.
  • Required action: In recover_preexisting_sandboxes_before_onboard, capture stdout/stderr from upgrade-sandboxes call. Parse failed sandbox names from output (pattern "Failed to recover '([^']+)'") and store in array. Pass array to finalize_install/print_done to include in error message: "Failed sandboxes: alpha, beta".
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Run installer with pre-existing non-Ready sandbox that fails recovery; verify final error output includes sandbox name(s). Check install.sh:2670-2690 recover_preexisting_sandboxes_before_onboard and print_done at line 555
  • Missing regression test: Add test in test/install-preexisting-sandbox-recovery.test.ts asserting failed sandbox name appears in installer stderr/output when upgrade-sandboxes fails
  • Done when: The required change is committed and verification passes: Run installer with pre-existing non-Ready sandbox that fails recovery; verify final error output includes sandbox name(s). Check install.sh:2670-2690 recover_preexisting_sandboxes_before_onboard and print_done at line 555.
  • Evidence: install.sh:2770 recover_preexisting_sandboxes_before_onboard only checks exit code; print_done at line ~555 outputs generic message. upgrade-sandboxes.ts:270 logs failed sandbox names per-item. PRA-6 from previous review unresolved.

PRA-11 Required — Stopped sandboxes with prepared backups are not recovered

  • Location: src/lib/actions/upgrade-sandboxes.ts:195
  • Category: correctness
  • Problem: Only sandboxes explicitly observed in live list with non-Ready phase (nonReadyLiveNames) are assessed for prepared-backup recovery. Stopped sandboxes (absent from live list entirely) with validated latest backups and positive managed-image evidence are filtered into notObservedReadyOrNonReady and skipped with generic warning, even when they have all required evidence.
  • Impact: Pre-upgrade sandboxes that were stopped before upgrade cannot auto-recover, leaving user data inaccessible until manual rebuild. This contradicts the PR goal of recovering pre-existing sandboxes.
  • Required action: Extend backup recovery assessment to include stopped sandboxes (from splitRebuildableSandboxes.stopped) that have validated latest backups and positive managed-image evidence. Assess them with the same prepareBackupRecovery logic. Update notObservedReadyOrNonReady warning to only apply to stopped sandboxes WITHOUT prepared backups.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Read upgrade-sandboxes.ts:167 nonReadyLiveNames filter and 240 splitRebuildableSandboxes; trace how stopped sandboxes flow to notObservedReadyOrNonReady at line 210
  • Missing regression test: Add test in src/lib/actions/upgrade-sandboxes-recovery.test.ts: stopped sandbox with validated backup and managed-image evidence should be recovered (rebuildSandbox called with recoveryManifest)
  • Done when: The required change is committed and verification passes: Read upgrade-sandboxes.ts:167 nonReadyLiveNames filter and 240 splitRebuildableSandboxes; trace how stopped sandboxes flow to notObservedReadyOrNonReady at line 210.
  • Evidence: upgrade-sandboxes.ts:167 nonReadyLiveNames only includes explicitly observed non-Ready phases. Line 210 notObservedReadyOrNonReady = stopped.filter(s => !assessedRecoveryNames.has(s.name)) skips all stopped sandboxes. PRA-7/PRA-15 from previous review unresolved.

PRA-12 Resolve/justify — Recovery signal uses environment variable instead of typed internal API

  • Location: src/lib/actions/upgrade-sandboxes.ts:199
  • Category: security
  • Problem: recoverPreparedBackups = process.env.NEMOCLAW_RESTORE_LATEST_BACKUP_ON_RECREATE === "1" uses an environment variable as the control signal between installer and upgrade-sandboxes. Any process setting this env var could trigger the recovery path, bypassing the installer's controlled flow.
  • Impact: Trust boundary bypass: a compromised or misconfigured process could set this env var and cause upgrade-sandboxes to attempt prepared-backup recovery on arbitrary sandboxes without the installer's preflight checks (backup-all, legacy gateway retirement).
  • Recommended action: Replace env var with internal TypeScript option: add recoverPreparedBackups?: boolean to UpgradeSandboxesOptions (or create internal upgradeSandboxesWithRecovery function). Installer calls with typed option; CLI --auto path keeps env var for backward compat but internal path uses typed API.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read upgrade-sandboxes.ts:199 and normalizeUpgradeSandboxesOptions in domain/lifecycle/options.ts; confirm env var is the only signal
  • Missing regression test: N/A - architecture finding; add unit test that upgradeSandboxes with recoverPreparedBackups=true behaves identically to env var path
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read upgrade-sandboxes.ts:199 and normalizeUpgradeSandboxesOptions in domain/lifecycle/options.ts; confirm env var is the only signal.
  • Evidence: upgrade-sandboxes.ts:199 uses process.env directly. install.sh exports NEMOCLAW_RESTORE_LATEST_BACKUP_ON_RECREATE=1 before calling. PRA-12 from previous review unresolved.

PRA-13 Resolve/justify — Public interface exposes internal recovery option

  • Location: src/lib/actions/sandbox/rebuild.ts:85
  • Category: security
  • Problem: RebuildSandboxExecutionOptions.recoveryManifest is documented as "Internal installer recovery input; never exposed as a CLI option" but is part of the exported public interface. External callers could pass a crafted manifest to bypass normal rebuild validation.
  • Impact: API surface leak: malicious or buggy caller could supply a recoveryManifest that skips backup creation and validation, potentially deleting a sandbox and recreating from an attacker-controlled manifest.
  • Recommended action: Make recoveryManifest internal: prefix with underscore (_recoveryManifest) in RebuildSandboxExecutionOptions, or add runtime assertion that it's only used when called from upgrade-sandboxes (check call stack or pass explicit internal flag). Document as @internal in JSDoc.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read rebuild.ts:85 RebuildSandboxExecutionOptions interface and line 703 where it's used; confirm no internal guard
  • Missing regression test: Add test in rebuild-flow.test.ts: calling rebuildSandbox with recoveryManifest from non-upgrade-sandboxes context should fail or be ignored (depending on chosen fix)
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read rebuild.ts:85 RebuildSandboxExecutionOptions interface and line 703 where it's used; confirm no internal guard.
  • Evidence: rebuild.ts:85 exports RebuildSandboxExecutionOptions with recoveryManifest. Called from upgrade-sandboxes.ts:255 with recoveryManifest. PRA-13 from previous review unresolved.

PRA-14 Resolve/justify — Mixed work array couples two recovery paths with different trust assumptions

  • Location: src/lib/actions/upgrade-sandboxes.ts:247
  • Category: architecture
  • Problem: work array combines rebuildable (manifest: null, trust live sandbox state) and preparedRecoveries (manifest: RebuildManifest, trust pre-upgrade backup). Single loop with verb="Rebuild"/"Recover" conflates two distinct trust boundaries: live sandbox backup vs. pre-upgrade backup.
  • Impact: Harder to audit: a bug in one path could affect the other. Different validation requirements (live backup vs. pre-validated manifest) are handled by same code path with conditional logic. Violates separation of concerns for security-critical operations.
  • Recommended action: Split into separate loops or extract executeRecovery(sandbox, manifest?) with explicit path separation. First loop: stale rebuildable sandboxes (normal rebuild). Second loop: preparedRecoveries (recovery manifest). Keep validation and logging distinct per path.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read upgrade-sandboxes.ts:247 work array construction and 250-270 loop; trace how manifest null vs object changes behavior
  • Missing regression test: N/A - architecture finding; refactoring should preserve behavior
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read upgrade-sandboxes.ts:247 work array construction and 250-270 loop; trace how manifest null vs object changes behavior.
  • Evidence: upgrade-sandboxes.ts:247 work = [...rebuildable.map(manifest: null), ...preparedRecoveries.map(manifest: recovery.manifest)]. PRA-14 from previous review unresolved.

PRA-15 Resolve/justify — Stopped sandboxes without prepared backups silently skipped when recovery signal set

  • Location: src/lib/actions/upgrade-sandboxes.ts:210
  • Category: correctness
  • Problem: When recoverPreparedBackups=true, stopped sandboxes without prepared backups are filtered into notObservedReadyOrNonReady and only warned about. The warning message "verify their recorded gateway or start them first" is misleading — these sandboxes have no backup to recover. The warning should always appear regardless of recovery signal; the signal should only control whether prepared-backup recoveries are attempted.
  • Impact: Operators see warning about "not observed on selected gateway" for stopped sandboxes but don't understand these are permanently skipped from recovery. No indication that they need manual rebuild.
  • Recommended action: Always warn about stopped sandboxes without prepared backups (current behavior when recoverPreparedBackups=false). When recoverPreparedBackups=true, additionally attempt prepared-backup recovery for stopped sandboxes that have validated backups + managed-image evidence. Only skip truly unassessable sandboxes.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read upgrade-sandboxes.ts:210 notObservedReadyOrNonReady filter and 212 console.log; trace how recoverPreparedBackups flag affects this path
  • Missing regression test: Add test in upgrade-sandboxes-recovery.test.ts: stopped sandbox without backup should warn regardless of NEMOCLAW_RESTORE_LATEST_BACKUP_ON_RECREATE value
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read upgrade-sandboxes.ts:210 notObservedReadyOrNonReady filter and 212 console.log; trace how recoverPreparedBackups flag affects this path.
  • Evidence: PRA-15 from previous review: "Always warn about stopped sandboxes without prepared backups regardless of recoverPreparedBackups flag. The flag should only affect whether prepared-backup recoveries are attempted."

PRA-16 Resolve/justify — Extensive mock scaffolding duplicated across rebuild flow tests

  • Location: src/lib/actions/sandbox/rebuild-flow.test.ts:233
  • Category: tests
  • Problem: createRebuildFlowHarness mocks 20+ modules (registry, sandboxState, onboardSession, shields, messaging, etc.) with 100+ lines of setup. Each new test file (upgrade-sandboxes-recovery.test.ts, snapshot-recovery-validation.test.ts) replicates similar mocking patterns.
  • Impact: High maintenance cost; changes to rebuild dependencies require updating mocks in multiple files. Inconsistent mock configurations can mask integration bugs.
  • Recommended action: Extract to src/lib/actions/sandbox/rebuild-test-helpers.ts with factory functions: createMockRegistry(), createMockSandboxState(), createMockOnboardSession(), createMockShields(), createHarness(overrides). Use composable overrides instead of monolithic harness.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Count vi.spyOn/mockImplementation lines in createRebuildFlowHarness (lines 233-380); compare with mock setup in upgrade-sandboxes-recovery.test.ts:60-120
  • Missing regression test: N/A - test architecture finding
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Count vi.spyOn/mockImplementation lines in createRebuildFlowHarness (lines 233-380); compare with mock setup in upgrade-sandboxes-recovery.test.ts:60-120.
  • Evidence: PRA-16 from previous review. simplificationSignals shows test_over_scaffold at lines 233, 235, 251, 583, 601, 610, 654 in rebuild-flow.test.ts.

PRA-17 Resolve/justify — Missing negative test for readManifest null/parse-error in validateRebuildRecoveryManifest

  • Location: test/snapshot-recovery-validation.test.ts:48
  • Category: security
  • Problem: validateRebuildRecoveryManifest calls readManifest(candidateBackupPath) which can return null if manifest disappears or becomes malformed between getLatestBackup and revalidation. Tests cover getLatestBackup returning null (line 56) and persisted manifest disappearing after discovery (line 84), but NOT the revalidation path in validateRebuildRecoveryManifest where readManifest returns null for the candidate path.
  • Impact: TOCTOU gap: if backup manifest is deleted/corrupted after getLatestBackup but before validateRebuildRecoveryManifest's readManifest call, behavior is untested. Could return ok: true with null manifest or throw.
  • Recommended action: Add test case: candidate manifest pointing to non-existent backup directory (simulating manifest deletion between discovery and revalidation), verify validateRebuildRecoveryManifest returns ok: false with reason "latest backup manifest is missing, malformed, or unsupported".
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Run test/snapshot-recovery-validation.test.ts; confirm no test exercises validateRebuildRecoveryManifest with candidate whose backup path has no manifest file
  • Missing regression test: test/snapshot-recovery-validation.test.ts: it('rejects candidate manifest when persisted manifest missing at revalidation time')
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Run test/snapshot-recovery-validation.test.ts; confirm no test exercises validateRebuildRecoveryManifest with candidate whose backup path has no manifest file.
  • Evidence: test/snapshot-recovery-validation.test.ts:56 tests getLatestBackup with missing manifest; line 84 tests persisted manifest disappearing after getLatestBackup. validateRebuildRecoveryManifest's readManifest call at line 1685 is not tested for null return. PRA-17/T7 from previous review.

PRA-18 Resolve/justify — Missing blueprintDigest validation test guardrail

  • Location: test/snapshot-recovery-validation.test.ts:1
  • Category: tests
  • Problem: No test exists for blueprintDigest validation because the validation doesn't exist (PRA-11). Once blueprintDigest validation is added, a test must verify mismatch detection.
  • Impact: Without test, blueprintDigest validation regression would go undetected. Supply-chain trust boundary unguarded.
  • Recommended action: Add test in test/snapshot-recovery-validation.test.ts after blueprintDigest validation is implemented: candidate with different blueprintDigest than persisted manifest should fail validation.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Search test/snapshot-recovery-validation.test.ts for blueprintDigest; only appears in test fixture at line 36 as null
  • Missing regression test: test/snapshot-recovery-validation.test.ts: it('rejects blueprintDigest mismatch between candidate and persisted manifest')
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Search test/snapshot-recovery-validation.test.ts for blueprintDigest; only appears in test fixture at line 36 as null.
  • Evidence: PRA-24/T8 from previous review. blueprintDigest validation missing entirely (S1 finding).

PRA-19 Improvement — Single-use config knob NEMOCLAW_RESTORE_LATEST_BACKUP_ON_RECREATE

  • Location: src/lib/actions/upgrade-sandboxes.ts:199
  • Category: architecture
  • Problem: Environment variable used only once in codebase as boolean flag. No second value planned. Adds surface area for misconfiguration (typo in env var name silently disables recovery).
  • Impact: Unnecessary configuration complexity; env var can be set by any process, not just installer.
  • Suggested action: Replace with constant: const RECOVER_PREPARED_BACKUPS = true (or internal typed option per PRA-12). Remove env var read. When a second recovery mode is needed, introduce proper enum/config.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: grep -r NEMOCLAW_RESTORE_LATEST_BACKUP_ON_RECREATE in repo; only appears in install.sh export and upgrade-sandboxes.ts read
  • Missing regression test: N/A - simplification
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: PRA-18 from previous review. simplificationSignals shows single_use_config at upgrade-sandboxes.ts:199.

PRA-20 Improvement — Documentation doesn't mention blueprintDigest validation

  • Location: docs/get-started/quickstart.mdx:113
  • Category: docs
  • Problem: Quickstart docs describe recovery validation as "backup identity matches and registry entry has NemoClaw-managed image fingerprint" but omit blueprintDigest check (which doesn't exist yet). After blueprintDigest validation is added (PRA-11), docs must be updated.
  • Impact: Docs drift from implementation; operators don't know full validation criteria.
  • Suggested action: Update quickstart.mdx line 113 and lifecycle.mdx line 267 to include "and the backup's blueprint digest matches the persisted manifest" once validation is implemented.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Read quickstart.mdx:113 and lifecycle.mdx:267; confirm blueprintDigest not mentioned
  • Missing regression test: N/A - doc finding
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: PRA-19 from previous review. quickstart.mdx:115 says 'backup identity matches (sandbox name, agent, timestamp, path) and positive managed-image evidence'. No blueprintDigest.

Workflow run details

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 coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🧹 Nitpick comments (3)
src/lib/actions/upgrade-sandboxes.ts (2)

82-123: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Classification logic reads well; consider a discriminant field over duck-typing.

isPreparedBackupRecovery distinguishes the union via "manifest" in candidate. Works today, but a PreparedBackupRecovery/RejectedBackupRecovery shape without a shared narrowing property (e.g. an explicit status: "prepared" | "rejected") is fragile if a future field addition accidentally introduces manifest on the rejected variant.

♻️ Optional refactor: explicit discriminant
 type PreparedBackupRecovery = {
+  status: "prepared";
   sandbox: registry.SandboxEntry;
   manifest: sandboxState.RebuildManifest;
 };

 type RejectedBackupRecovery = {
+  status: "rejected";
   sandbox: registry.SandboxEntry;
   reason: string;
 };
 ...
 function isPreparedBackupRecovery(
   candidate: PreparedBackupRecovery | RejectedBackupRecovery,
 ): candidate is PreparedBackupRecovery {
-  return "manifest" in candidate;
+  return candidate.status === "prepared";
 }
🤖 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/actions/upgrade-sandboxes.ts` around lines 82 - 123, The backup
recovery union in prepareBackupRecovery/isPreparedBackupRecovery relies on
duck-typing via the manifest property, which is fragile. Add an explicit
discriminant field to PreparedBackupRecovery and RejectedBackupRecovery (for
example, a status or kind literal), set it in prepareBackupRecovery for each
return path, and update isPreparedBackupRecovery to narrow on that discriminant
instead of checking "manifest" in candidate. Keep the existing recovery/reason
fields unchanged so callers can continue to use the same data.

264-294: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Failure/success messaging doesn't distinguish recovery from ordinary rebuild.

Line 276 correctly picks Recover/Rebuild for the confirmation prompt, but the catch-block error (line 291, Failed to rebuild '${sandbox.name}') and the check-only summary line (line 242, to rebuild them) always say "rebuild" even for prepared-recovery items. Since this is the installer's operator-facing status for a data-recovery path (per PR objective of surfacing recovery failures clearly), reusing the same verb distinction used at line 276 would make failures easier to triage.

♻️ Suggested fix
     } catch (err) {
       const errorMessage = err instanceof Error ? err.message : String(err);
-      console.error(`  ${YW}⚠${R} Failed to rebuild '${sandbox.name}': ${errorMessage}`);
+      console.error(
+        `  ${YW}⚠${R} Failed to ${manifest ? "recover" : "rebuild"} '${sandbox.name}': ${errorMessage}`,
+      );
       failed++;
     }
🤖 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/actions/upgrade-sandboxes.ts` around lines 264 - 294, The recovery
workflow still logs and summarizes all outcomes as “rebuild,” even for prepared
recovery items. Update the operator-facing messages in upgrade-sandboxes.ts to
reuse the same verb selection already used in the askPrompt branch (Recover vs
Rebuild), including the catch block around rebuildSandbox and the check-only
summary text, so recovery failures and summaries are clearly distinguished from
ordinary rebuilds.
test/install-preexisting-sandbox-recovery.test.ts (1)

43-63: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

ensure_supported_runtime (and command_exists) are left unstubbed in an otherwise fully-mocked main() harness.

Since _CLI_PATH is set, command_exists is unreachable, but ensure_supported_runtime runs for real right after install_nodejs (stubbed). It likely passes harmlessly in CI, but it weakens the isolation of this harness and could make the test environment-dependent.

🤖 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 `@test/install-preexisting-sandbox-recovery.test.ts` around lines 43 - 63, The
fully mocked main() harness still leaves ensure_supported_runtime (and
indirectly command_exists) unstubbed, so the test is not fully isolated. Add a
no-op stub for ensure_supported_runtime in this test harness alongside the other
mocked helpers, and only keep command_exists real if it is truly unreachable
through _CLI_PATH; otherwise stub it too. Use the existing shell helper names in
the harness to place the stubs consistently with install_nodejs,
verify_nemoclaw, and run_onboard.
🤖 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 `@scripts/install.sh`:
- Around line 2747-2754: The recover_preexisting_sandboxes_before_onboard
failure path is exiting too early and bypassing the install finalizer. In
scripts/install.sh, update the error handling around
recover_preexisting_sandboxes_before_onboard in the onboarding flow so it
records the failure but does not abort before finalize_install() runs; keep the
existing run_onboard/error and ONBOARD_RAN logic intact so print_done() can emit
the _UPGRADE_SANDBOXES_FAILED recovery guidance with the affected sandbox and
backup details.

In `@src/lib/actions/upgrade-sandboxes-recovery.test.ts`:
- Line 12: The test cleanup logic in upgrade-sandboxes-recovery.test.ts adds a
new conditional branch around restoring
NEMOCLAW_RESTORE_LATEST_BACKUP_ON_RECREATE in afterEach, which triggers the
conditional scan. Replace the manual save/restore pattern around
originalRecoverySignal with Vitest’s environment helpers in the test setup,
using vi.stubEnv for the variable and vi.unstubAllEnvs in cleanup so the
original value is restored automatically. Keep the change localized to the
test’s beforeEach/afterEach flow and remove the explicit if branch tied to
originalRecoverySignal.

In `@test/install-preexisting-sandbox-recovery.test.ts`:
- Line 63: The failing-recovery test uses a no-op finalize_install stub, which
lets the test pass without proving the recovery path actually reaches
completion. Update the finalize_install stub in the
install-preexisting-sandbox-recovery test to record a call-log entry (matching
the other stubs), then assert that entry is present so the test verifies
finalize_install is invoked after recovery. Use the existing finalize_install,
calls, and result assertions in the test to locate the change.

In `@test/snapshot-recovery-validation.test.ts`:
- Around line 47-54: The teardown in afterAll is using a manual conditional to
restore HOME, which the test guardrail wants avoided. Update the
snapshot-recovery-validation test to use Vitest’s vi.stubEnv for setting HOME
and vi.unstubAllEnvs for cleanup, and keep the existing TMP_HOME removal in the
same teardown path. Use the afterAll block and the ORIGINAL_HOME /
process.env.HOME handling as the place to switch to env stubbing.

---

Nitpick comments:
In `@src/lib/actions/upgrade-sandboxes.ts`:
- Around line 82-123: The backup recovery union in
prepareBackupRecovery/isPreparedBackupRecovery relies on duck-typing via the
manifest property, which is fragile. Add an explicit discriminant field to
PreparedBackupRecovery and RejectedBackupRecovery (for example, a status or kind
literal), set it in prepareBackupRecovery for each return path, and update
isPreparedBackupRecovery to narrow on that discriminant instead of checking
"manifest" in candidate. Keep the existing recovery/reason fields unchanged so
callers can continue to use the same data.
- Around line 264-294: The recovery workflow still logs and summarizes all
outcomes as “rebuild,” even for prepared recovery items. Update the
operator-facing messages in upgrade-sandboxes.ts to reuse the same verb
selection already used in the askPrompt branch (Recover vs Rebuild), including
the catch block around rebuildSandbox and the check-only summary text, so
recovery failures and summaries are clearly distinguished from ordinary
rebuilds.

In `@test/install-preexisting-sandbox-recovery.test.ts`:
- Around line 43-63: The fully mocked main() harness still leaves
ensure_supported_runtime (and indirectly command_exists) unstubbed, so the test
is not fully isolated. Add a no-op stub for ensure_supported_runtime in this
test harness alongside the other mocked helpers, and only keep command_exists
real if it is truly unreachable through _CLI_PATH; otherwise stub it too. Use
the existing shell helper names in the harness to place the stubs consistently
with install_nodejs, verify_nemoclaw, and run_onboard.
🪄 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: 0b167b90-14ef-4b1a-bc93-11e0afaae9d3

📥 Commits

Reviewing files that changed from the base of the PR and between 96aacaa and 20f42f6.

📒 Files selected for processing (10)
  • docs/get-started/quickstart.mdx
  • docs/manage-sandboxes/lifecycle.mdx
  • scripts/install.sh
  • src/lib/actions/sandbox/rebuild-flow.test.ts
  • src/lib/actions/sandbox/rebuild.ts
  • src/lib/actions/upgrade-sandboxes-recovery.test.ts
  • src/lib/actions/upgrade-sandboxes.ts
  • src/lib/state/sandbox.ts
  • test/install-preexisting-sandbox-recovery.test.ts
  • test/snapshot-recovery-validation.test.ts

Comment thread scripts/install.sh
Comment thread src/lib/actions/upgrade-sandboxes-recovery.test.ts Outdated
Comment thread test/install-preexisting-sandbox-recovery.test.ts Outdated
Comment thread test/snapshot-recovery-validation.test.ts
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
src/lib/actions/upgrade-sandboxes-recovery.test.ts (1)

168-183: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Test doesn't actually exercise the "different gateway" scenario it claims.

The test title says the sandbox is "registered on a different gateway," but liveOutput: "No sandboxes found." makes the sandbox simply absent from the live listing entirely — indistinguishable from any other "not observed live" case. The gatewayNames option only feeds registry.listSandboxes, which isn't consulted by the nonReadyLiveNames gating logic under test (per upgrade-sandboxes.ts Context snippet 2, eligibility is derived purely from parsed live entries, not registry gatewayName). This passes without exercising cross-gateway matching at all — a scenario that matters for the underlying bug (#6114).

To actually test gateway-based exclusion, the live output should show the sandbox as Ready/present on a different gateway's listing than the one being checked, or the harness needs to support per-gateway live output.

As per path instructions: "Flag ... conditionals that make a test pass without exercising its claim."

🤖 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/actions/upgrade-sandboxes-recovery.test.ts` around lines 168 - 183,
The test in upgrade-sandboxes-recovery.test.ts is claiming a cross-gateway Ready
sandbox case but currently only proves the sandbox is missing from live output.
Update the harness setup in createRecoveryHarness/upgradeSandboxes test so the
sandbox appears in live entries on a different gateway (not just in
registry.listSandboxes via gatewayNames), and assert the recovery logic still
skips it; this ensures the nonReadyLiveNames gating in upgradeSandboxes.ts is
actually exercised.

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 `@src/lib/actions/upgrade-sandboxes-recovery.test.ts`:
- Around line 168-183: The test in upgrade-sandboxes-recovery.test.ts is
claiming a cross-gateway Ready sandbox case but currently only proves the
sandbox is missing from live output. Update the harness setup in
createRecoveryHarness/upgradeSandboxes test so the sandbox appears in live
entries on a different gateway (not just in registry.listSandboxes via
gatewayNames), and assert the recovery logic still skips it; this ensures the
nonReadyLiveNames gating in upgradeSandboxes.ts is actually exercised.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 86d02ad6-c96b-426c-a1c6-75599a71eda4

📥 Commits

Reviewing files that changed from the base of the PR and between 1309bf4 and 5688e5f.

📒 Files selected for processing (2)
  • src/lib/actions/upgrade-sandboxes-recovery.test.ts
  • src/lib/actions/upgrade-sandboxes.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/lib/actions/upgrade-sandboxes.ts

@github-actions

github-actions Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Vitest E2E Target Results — ✅ All requested jobs passed

Run: 28536356354
Workflow ref: fix/upgrade-sandbox-recovery-6114
Requested targets: (default — all supported)
Requested jobs: cloud-onboard,upgrade-stale-sandbox,sandbox-rebuild,state-backup-restore,openshell-gateway-upgrade,rebuild-openclaw,snapshot-commands,onboard-resume,onboard-repair
Summary: 9 passed, 0 failed, 0 cancelled, 0 skipped

Job Result
cloud-onboard ✅ success
onboard-repair ✅ success
onboard-resume ✅ success
openshell-gateway-upgrade ✅ success
rebuild-openclaw ✅ success
sandbox-rebuild ✅ success
snapshot-commands ✅ success
state-backup-restore ✅ success
upgrade-stale-sandbox ✅ success

Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
@github-actions

github-actions Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Vitest E2E Target Results — ✅ All requested jobs passed

Run: 28537782550
Workflow ref: fix/upgrade-sandbox-recovery-6114
Requested targets: (default — all supported)
Requested jobs: cloud-onboard,upgrade-stale-sandbox,sandbox-rebuild,state-backup-restore,openshell-gateway-upgrade,rebuild-openclaw,snapshot-commands,onboard-resume,onboard-repair
Summary: 9 passed, 0 failed, 0 cancelled, 0 skipped

Job Result
cloud-onboard ✅ success
onboard-repair ✅ success
onboard-resume ✅ success
openshell-gateway-upgrade ✅ success
rebuild-openclaw ✅ success
sandbox-rebuild ✅ success
snapshot-commands ✅ success
state-backup-restore ✅ success
upgrade-stale-sandbox ✅ success

ericksoa added 2 commits July 1, 2026 14:47
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
@ericksoa

ericksoa commented Jul 1, 2026

Copy link
Copy Markdown
Contributor Author

Safety correction at 35ab003bbba4159cb630b9e04566261ce5401c8b:

  • v0.0.55 initially registers --from images with agentVersion=null, but ordinary connect / doctor version probing can populate that same field.
  • backup-all then copies the probed value into the manifest, so the previous registry/manifest version match could misclassify a custom image as NemoClaw-managed and permit destructive default-image recreation.
  • Recovery now requires a non-empty nemoclawVersion fingerprint. A regression exercises the real predicate with the probed-custom shape and proves exit 1 with zero rebuild calls.

Focused verification is 31/31 green, with typecheck, Biome, source-shape, test-size, docs, and diff checks passing. The PR body no longer says Fixes #6114: raw v0.0.55 entries have no trustworthy per-sandbox managed/custom provenance, so they now fail closed. A separate design is required to recover those entries without risking custom-image loss.

Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
@cv
cv merged commit 3f7f4d8 into main Jul 1, 2026
43 checks passed
@cv
cv deleted the fix/upgrade-sandbox-recovery-6114 branch July 1, 2026 22:51
ericksoa pushed a commit that referenced this pull request Jul 2, 2026
## Summary
- Add the `v0.0.72` release-note section with links to the deeper docs
pages for installer recovery, command diagnostics, inference, policy,
and sandbox repair changes.
- Document the custom preset `allowed_ips` guard for user-authored
policy files.

## Related Issue
None.

## Source summary
- #6132 -> `docs/about/release-notes.mdx`: Summarizes installer and
upgrade recovery before generic onboarding, with links to quickstart and
lifecycle docs.
- #6087 -> `docs/network-policy/customize-network-policy.mdx`: Documents
that user-authored custom presets reject `allowed_ips` for ordinary
endpoints; also summarized in release notes.
- #5975 -> `docs/about/release-notes.mdx`: Summarizes safer curl-based
inference probes that keep API keys out of process arguments.
- #6044 -> `docs/about/release-notes.mdx`: Summarizes compact `channels
status` configuration reporting.
- #6096 -> `docs/about/release-notes.mdx`: Summarizes OpenClaw EC2
metadata discovery disablement and links to security guidance.
- #5980 and #5991 -> `docs/about/release-notes.mdx`: Summarizes `exec`
multiline argument rejection and recovery guidance.
- #6023 -> `docs/about/release-notes.mdx`: Summarizes
registered-provider diagnostics for `inference set` failures.
- #6074 -> `docs/about/release-notes.mdx`: Summarizes the refreshed
NVIDIA Endpoints featured-model selection behavior.
- #5969 -> `docs/about/release-notes.mdx`: Summarizes `credentials add`
provider credential registration.
- #6060 -> `docs/about/release-notes.mdx`: Summarizes mutable OpenClaw
config permission restoration after `exec`.
- #6134 -> `docs/about/release-notes.mdx`: Summarizes restored Tavily
access for managed Python workflows.
- #6089 -> `docs/about/release-notes.mdx`: Summarizes Hermes runtime
version-scheme comparison during upgrade checks.
- #6131 -> `docs/about/release-notes.mdx`: Summarizes OpenClaw gateway
watchdog recovery behavior.
- #5976 and #5990 -> `docs/about/release-notes.mdx`: Summarizes prompt
stdin EOF cancellation behavior during onboarding.
- #5540 -> `docs/about/release-notes.mdx`: Summarizes clarified
host-level and per-sandbox status command scope.
- #5978 and #6018 -> `docs/about/release-notes.mdx`: Summarizes
policy-denial log breadcrumbs in connect shells.

## Testing
- `npm run docs:sync-agent-variants`
- `npm run docs`
- Commit hooks passed during `git commit`, including commitlint and
gitleaks.
- Pre-push hook passed during `git push`, including TypeScript CLI and
package/tag version sync.

## Checklist
- [x] Documentation updated.
- [x] `npm run docs` completed with 0 errors and 1 existing Fern
warning.
- [x] No source code or generated build artifacts committed.

Signed-off-by: Miyoung Choi <miyoungc@nvidia.com>

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

* **Documentation**
* Added release notes for v0.0.72 covering improved installer recovery,
clearer CLI diagnostics, safer inference setup and provider switching,
better credential handling, stronger policy boundaries, and more robust
runtime repair behavior.
* Updated network policy guidance to clarify when `allowed_ips` can be
used, including a specific exception for the sandbox-to-host bridge
endpoint.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->
Hadar301 pushed a commit to Hadar301/NemoClaw-OpenShift that referenced this pull request Jul 12, 2026
<!-- markdownlint-disable MD041 -->
## Summary

This change runs validated prepared-backup recovery before the installer
starts generic onboarding.
It reuses the existing installer recovery signal and rebuild machinery,
fails closed on identity or managed-image evidence mismatches, and stops
onboarding if any recovery fails.
Focused verification passes. However, v0.0.55 recorded no trustworthy
per-sandbox managed/custom image provenance, so this PR now rejects
those ambiguous entries and does not by itself resolve NVIDIA#6114.

## Related Issue

Related to NVIDIA#6114

## Changes

- Run `upgrade-sandboxes --auto` for pre-existing sandboxes before
generic onboarding.
- Under `NEMOCLAW_RESTORE_LATEST_BACKUP_ON_RECREATE=1`, include
registered non-Ready sandboxes only when their latest backup validates
against the sandbox and agent identity.
- Require an explicitly observed, known non-Ready live phase so an
absent registry entry—such as a Ready sandbox on another gateway—is
never selected for prepared recovery.
- Isolate backup-assessment failures per sandbox, attempt every later
eligible recovery, and return aggregate nonzero when any eligible
recovery is blocked or fails.
- Require a non-empty NemoClaw-managed image fingerprint and reject
pre-fingerprint, missing, mismatched, or custom-image recovery inputs
before deletion; matching agent versions are explicitly not accepted as
provenance.
- Revalidate the registry entry and latest manifest immediately before
the destructive phase.
- Reuse the validated manifest in the existing rebuild path instead of
creating a second unreachable backup.
- Preserve the existing registry rollback and state-restore behavior
when recreation fails.
- Attempt every eligible recovery and return nonzero before onboarding
if any recovery fails.
- Route recovery failure through the installer completion summary so
preserved-backup guidance is shown before the nonzero exit.
- Add focused rebuild, manifest-validation, upgrade classification,
installer-ordering, and rollback coverage.
- Document the installer's pre-onboarding recovery behavior.
- Local validation passed for CLI build/typecheck, focused CLI and
integration tests, shell checks, Biome, project membership,
source-shape, and test-size checks. The Fern docs check completed with 0
errors and 2 warnings.
- Live v0.0.55 upgrade acceptance is blocked by missing trustworthy
legacy image provenance: safely recovering raw v0.0.55 managed images
while rejecting custom images requires a separate design. No v0.0.55
recovery success is claimed here.

## Type of Change

- [ ] Code change (feature, bug fix, or refactor)
- [x] Code change with doc updates
- [ ] Doc only (prose changes, no code sample modifications)
- [ ] Doc only (includes code sample changes)

## Quality Gates
<!-- Check all that apply. For any "covered by existing tests", "not
applicable", or waiver entry, add a brief justification on the same line
or in the Changes section. -->
- [x] Tests added or updated for changed behavior
- [ ] Existing tests cover changed behavior — justification:
- [ ] Tests not applicable — justification:
- [x] Docs updated for user-facing behavior changes
- [ ] Docs not applicable — justification:
- [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 review
through `35ab003bbba4159cb630b9e04566261ce5401c8b` confirmed the
fingerprint-only rule blocks the probed-custom-image deletion path and
also confirmed raw v0.0.55 recovery remains unresolved.
- [ ] Non-success, skipped, or missing CI check accepted by maintainer —
check name, approval link, and follow-up issue:

## Verification
<!-- Check each item you ran and confirmed. Leave unchecked items you
skipped. Doc-only changes do not require npm test unless you ran it. -->
- [x] PR description includes the DCO sign-off declaration and every
commit appears as `Verified` in GitHub
- [ ] 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] 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)

---
<!-- DCO sign-off is required in this PR description, and every commit
must appear as Verified in GitHub. Run: git config user.name && git
config user.email -->
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

* **New Features**
* Installer/upgrade workflows now recover pre-existing non-Ready
sandboxes before starting generic onboarding, including prepared
recovery from the latest validated pre-upgrade backup when eligible and
evidenced.
* **Bug Fixes**
* Recovery is fail-closed: if recovery can’t be completed for any
eligible sandbox, the process stops and does not proceed with generic
onboarding.
* Added stronger identity, manifest, and managed-image evidence checks
to prevent unsafe recreate/recovery.
* Upgrade output now clearly distinguishes “prepared” vs “rejected”
recovery candidates.
* **Documentation**
* Updated quickstart and lifecycle upgrade guides to match the
recovery-first flow.
* **Tests**
* Expanded coverage for recovery ordering and prepared-backup recovery
validation.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Hadar301 pushed a commit to Hadar301/NemoClaw-OpenShift that referenced this pull request Jul 12, 2026
## Summary
- Add the `v0.0.72` release-note section with links to the deeper docs
pages for installer recovery, command diagnostics, inference, policy,
and sandbox repair changes.
- Document the custom preset `allowed_ips` guard for user-authored
policy files.

## Related Issue
None.

## Source summary
- NVIDIA#6132 -> `docs/about/release-notes.mdx`: Summarizes installer and
upgrade recovery before generic onboarding, with links to quickstart and
lifecycle docs.
- NVIDIA#6087 -> `docs/network-policy/customize-network-policy.mdx`: Documents
that user-authored custom presets reject `allowed_ips` for ordinary
endpoints; also summarized in release notes.
- NVIDIA#5975 -> `docs/about/release-notes.mdx`: Summarizes safer curl-based
inference probes that keep API keys out of process arguments.
- NVIDIA#6044 -> `docs/about/release-notes.mdx`: Summarizes compact `channels
status` configuration reporting.
- NVIDIA#6096 -> `docs/about/release-notes.mdx`: Summarizes OpenClaw EC2
metadata discovery disablement and links to security guidance.
- NVIDIA#5980 and NVIDIA#5991 -> `docs/about/release-notes.mdx`: Summarizes `exec`
multiline argument rejection and recovery guidance.
- NVIDIA#6023 -> `docs/about/release-notes.mdx`: Summarizes
registered-provider diagnostics for `inference set` failures.
- NVIDIA#6074 -> `docs/about/release-notes.mdx`: Summarizes the refreshed
NVIDIA Endpoints featured-model selection behavior.
- NVIDIA#5969 -> `docs/about/release-notes.mdx`: Summarizes `credentials add`
provider credential registration.
- NVIDIA#6060 -> `docs/about/release-notes.mdx`: Summarizes mutable OpenClaw
config permission restoration after `exec`.
- NVIDIA#6134 -> `docs/about/release-notes.mdx`: Summarizes restored Tavily
access for managed Python workflows.
- NVIDIA#6089 -> `docs/about/release-notes.mdx`: Summarizes Hermes runtime
version-scheme comparison during upgrade checks.
- NVIDIA#6131 -> `docs/about/release-notes.mdx`: Summarizes OpenClaw gateway
watchdog recovery behavior.
- NVIDIA#5976 and NVIDIA#5990 -> `docs/about/release-notes.mdx`: Summarizes prompt
stdin EOF cancellation behavior during onboarding.
- NVIDIA#5540 -> `docs/about/release-notes.mdx`: Summarizes clarified
host-level and per-sandbox status command scope.
- NVIDIA#5978 and NVIDIA#6018 -> `docs/about/release-notes.mdx`: Summarizes
policy-denial log breadcrumbs in connect shells.

## Testing
- `npm run docs:sync-agent-variants`
- `npm run docs`
- Commit hooks passed during `git commit`, including commitlint and
gitleaks.
- Pre-push hook passed during `git push`, including TypeScript CLI and
package/tag version sync.

## Checklist
- [x] Documentation updated.
- [x] `npm run docs` completed with 0 errors and 1 existing Fern
warning.
- [x] No source code or generated build artifacts committed.

Signed-off-by: Miyoung Choi <miyoungc@nvidia.com>

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

* **Documentation**
* Added release notes for v0.0.72 covering improved installer recovery,
clearer CLI diagnostics, safer inference setup and provider switching,
better credential handling, stronger policy boundaries, and more robust
runtime repair behavior.
* Updated network policy guidance to clarify when `allowed_ips` can be
used, including a specific exception for the sandbox-to-host bridge
endpoint.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: install Install, setup, prerequisites, or uninstall flow area: sandbox OpenShell sandbox lifecycle, runtime, config, or recovery bug-fix PR fixes a bug or regression

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[All Platforms][Upgrade] v0.0.55 → v0.0.76 leaves pre-existing sandboxes stuck in Provisioning/Error — user data inaccessible until manual rebuild

3 participants