Skip to content

refactor(cli): lower cognitive complexity ratchet below 150 - #5432

Merged
cv merged 119 commits into
mainfrom
ratchet/149-rebuild-snapshot-onboard-stack
Jun 16, 2026
Merged

refactor(cli): lower cognitive complexity ratchet below 150#5432
cv merged 119 commits into
mainfrom
ratchet/149-rebuild-snapshot-onboard-stack

Conversation

@cv

@cv cv commented Jun 15, 2026

Copy link
Copy Markdown
Collaborator

Summary

Lower the cognitive-complexity ratchet from 184 to 149 by extracting the remaining high-complexity rebuild, snapshot, and setupNim paths. This brings the configured limit below 150 without adding suppressions or per-file exceptions.

Changes

  • Lowered complexity/noExcessiveCognitiveComplexity from 184 to 149 in biome.json.
  • Extracted rebuild live-state resolution, base-image preflight, backup handling, and related helpers out of the large rebuild.ts hotspot into src/lib/actions/sandbox/rebuild-flow-helpers.ts.
  • Extracted snapshot restore substeps for OpenClaw permission repair and policy reconciliation.
  • Moved Ollama setupNim branch handling into src/lib/onboard/setup-nim-ollama.ts, keeping src/lib/onboard.ts net smaller.
  • Preserved allowToolsIncompatible through running, Windows-host, and installed Ollama selection paths.
  • Added focused unit/regression coverage for extracted Ollama handlers and the multi-agent rebuild guard.

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)

Verification

  • npx @biomejs/biome lint src/lib/onboard/setup-nim-ollama.test.ts src/lib/onboard/setup-nim-ollama.ts src/lib/onboard.ts src/lib/onboard/setup-nim-selection.ts

  • npx @biomejs/biome lint test/rebuild-credential-preflight.test.ts src/lib/onboard/setup-nim-ollama.test.ts

  • npx @biomejs/biome lint src/lib/actions/sandbox/rebuild.ts src/lib/actions/sandbox/rebuild-flow-helpers.ts test/rebuild-credential-preflight.test.ts

  • npx @biomejs/biome lint --only=complexity/noExcessiveCognitiveComplexity --max-diagnostics=none .

  • npm run typecheck:cli

  • npx vitest run --project cli test/rebuild-credential-preflight.test.ts test/rebuild-stale-recovery.test.ts src/lib/actions/sandbox/rebuild-gateway-drift.test.ts test/snapshot.test.ts test/openclaw-config-snapshot.test.ts test/security-sandbox-tar-traversal.test.ts src/lib/onboard/providers.test.ts src/lib/onboard/vllm-menu.test.ts src/lib/onboard/windows-host-ollama.test.ts src/lib/onboard/install-ollama-linux-upgrade.test.ts

  • npx vitest run --project cli src/lib/onboard/setup-nim-ollama.test.ts src/lib/onboard/windows-host-ollama.test.ts src/lib/onboard/install-ollama-linux-upgrade.test.ts src/lib/onboard/vllm-menu.test.ts

  • npx vitest run --project cli src/lib/onboard/setup-nim-ollama.test.ts src/lib/onboard/windows-host-ollama.test.ts src/lib/onboard/install-ollama-linux-upgrade.test.ts src/lib/onboard/vllm-menu.test.ts test/rebuild-credential-preflight.test.ts

  • npx vitest run --project cli test/rebuild-credential-preflight.test.ts test/rebuild-stale-recovery.test.ts src/lib/actions/sandbox/rebuild-gateway-drift.test.ts

  • Git hooks passed during commit and push, or npx prek run --from-ref main --to-ref HEAD passes

  • Targeted tests pass for changed behavior

  • Tests added or updated for new or changed behavior

  • Full npm test passes (broad runtime changes only)

  • No secrets, API keys, or credentials committed

  • Docs updated for user-facing behavior changes

  • npm run docs builds without warnings (doc changes only)

  • Doc pages follow the style guide (doc changes only)

  • New doc pages include SPDX header and frontmatter (new pages only)


Signed-off-by: Carlos Villela cvillela@nvidia.com

Summary by CodeRabbit

Release Notes

  • New Features
    • Expanded onboarding Ollama support across Windows and Linux/macOS with improved model selection and provisioning flow.
  • Bug Fixes
    • Refined sandbox rebuild live/stale recovery, including safer base-image preflight, shield-window handling, and backup behavior.
    • Enhanced snapshot restores with best-effort OpenClaw permission repair and preset/custom-policy reconciliation (warnings on issues).
  • Refactor
    • Modularized rebuild and restore workflows for clearer live recovery, backup, and shield relock handling.
  • Tests
    • Expanded coverage for Ollama selection paths, multi-agent rebuild safety, and snapshot restore reconciliation.
  • Style/Chores
    • Tightened linting complexity thresholds.

cv added 14 commits June 14, 2026 11:27
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
(cherry picked from commit 41fcd6a)
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
## Summary
Continue the stacked cognitive-complexity ratchet by lowering the
threshold from 185 to 184. The previous stack step brought all remaining
offenders to 184 or below, so this PR only tightens the Biome
configuration.

## Changes
- Lowered `complexity/noExcessiveCognitiveComplexity` from `185` to
`184` in `biome.json`.

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

## Verification
- `npx @biomejs/biome lint
--only=complexity/noExcessiveCognitiveComplexity --max-diagnostics=none
.`

- [x] Git hooks passed during commit and push, or `npx prek run
--from-ref main --to-ref HEAD` passes
- [x] Targeted tests pass for changed behavior
- [ ] Tests added or updated for new or changed behavior
- [ ] Full `npm test` passes (broad runtime changes only)
- [x] No secrets, API keys, or credentials committed
- [ ] Docs updated for user-facing behavior changes
- [ ] `npm run docs` builds without warnings (doc changes only)
- [ ] Doc pages follow the [style
guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md)
(doc changes only)
- [ ] New doc pages include SPDX header and frontmatter (new pages only)

---
<!-- DCO sign-off required by CI. Run: git config user.name && git
config user.email -->
Signed-off-by: Carlos Villela <cvillela@nvidia.com>

Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
(cherry picked from commit 167e6e3)
@cv cv self-assigned this Jun 15, 2026
@copy-pr-bot

copy-pr-bot Bot commented Jun 15, 2026

Copy link
Copy Markdown

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

@coderabbitai

coderabbitai Bot commented Jun 15, 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

Three parallel refactors extract rebuild preflight logic, snapshot post-restore handling, and Ollama onboarding selection into dedicated helpers and factories. New allowToolsIncompatible field added to SetupNimSelectionState for tracking tool compatibility across Ollama selection paths. Biome cognitive-complexity threshold lowered from 184 to 149 to enforce complexity reduction.

Changes

Rebuild flow helper extraction

Layer / File(s) Summary
rebuild-flow-helpers.ts: types and core implementations
src/lib/actions/sandbox/rebuild-flow-helpers.ts
Introduces RebuildSandboxEntry and RebuildLiveState types. Implements resolveRebuildLiveState to query OpenShell gateway, reconcile sandbox registry state, and return normal rebuild eligibility or stale-recovery with snapshot. Adds openRebuildShieldsWindowForState for stale-safe shields configuration. Adds ensureRebuildAgentBaseImage for agent base-image preflight with error handling. Adds backupSandboxStateForRebuild to perform state backup before rebuild, skip during stale recovery, and abort on failure.
rebuild.ts: imports and Step 1 delegation
src/lib/actions/sandbox/rebuild.ts
Adds imports from ./rebuild-flow-helpers including resolveRebuildLiveState, openRebuildShieldsWindowForState, ensureRebuildAgentBaseImage, backupSandboxStateForRebuild, and type RebuildSandboxEntry. Removes file-local RebuildSandboxEntry type alias. Updates imports for OpenShell gateway/runtime handling and policy resolution. Refactors rebuild Step 1 preflight to call the four new helpers in sequence instead of inline liveness querying and backup setup.
rebuild-credential-preflight.test.ts: multi-agent early-abort
test/rebuild-credential-preflight.test.ts
Extends createFixture options with optional agents field and destructures it with default null. Updates sandboxes.json generation to conditionally include agents property. Adds test case asserting that multi-agent sandbox rebuild aborts early with "multi-agent sandbox rebuild is not yet supported" before any prompt, credential preflight check, or backup step.

Snapshot post-restore helper extraction and policy reconciliation

Layer / File(s) Summary
snapshot.ts: runSnapshotRestore and post-restore reconciliation
src/lib/actions/sandbox/snapshot.ts
Updates autoCreateSandboxFromSource to accept resolved fromImage as explicit parameter. Adds repairRestoredOpenClawConfigPerms to repair OpenClaw mutable config permissions when openclaw.json is restored. Adds reconcileSnapshotPolicyPresets and reconcileSnapshotCustomPolicies to remove/apply snapshot-recorded policy presets and custom policies with warnings on failure. Introduces runSnapshotRestore encapsulating preflight resolution, destination deletion, snapshot execution, and post-restore reconciliation. Simplifies restore switch-case to delegate to runSnapshotRestore. Applies multi-line formatting to captureOpenshell, fs.rmSync, and backupSandboxState calls.
snapshot.test.ts: mock expansion and test assertions
src/lib/actions/sandbox/snapshot.test.ts
Introduces named, reusable mock functions for captureOpenshellMock, getAppliedPresetsMock, getCustomPoliciesMock, getLatestBackupMock, and policy application mocks. Updates vi.mock declarations to wire these mocks for ../../policy, ../../state/registry, and ../../state/sandbox. Sets mockReturnValue defaults in beforeEach. Reformats runSandboxSnapshot call and toHaveBeenCalledWith assertions to multi-line object-literal form. Expands restore/policy reconciliation test assertions to verify removal/addition of preset policies and re-application of custom policies.

Setup NIM Ollama handler extraction and state

Layer / File(s) Summary
SetupNimSelectionState: type extension and Biome threshold
src/lib/onboard/setup-nim-selection.ts, src/lib/onboard/setup-nim-selection.test.ts, biome.json
Adds allowToolsIncompatible: boolean field to SetupNimSelectionState type. Updates applyCloudFallbackSelection to reset the flag to false during cloud fallback. Updates test fixture makeState() and cloud fallback assertion. Tightens biome.json noExcessiveCognitiveComplexity.maxAllowedComplexity from 184 to 149.
setup-nim-ollama.ts: createSetupNimOllamaHandlers factory
src/lib/onboard/setup-nim-ollama.ts
Introduces SetupNimOllamaDeps dependency contract covering port checks, OS-specific Ollama install/switch/binding, systemd loopback override, startup gating with fallback, proxy configuration, base URL resolution, and model selection/validation. Implements shared helpers: selectModel, startProxyOrAnnounceDirect, configureOllamaState, applyOllamaFallbackState. Exports createSetupNimOllamaHandlers factory returning three async handlers: handleWindowsHostOllamaSelection, handleRunningOllamaSelection, handleInstallOllamaSelection.
onboard.ts: handler factory integration and state propagation
src/lib/onboard.ts
Imports and instantiates createSetupNimOllamaHandlers with wired dependencies. Replaces inline Ollama selection branches with calls to three handler functions. Adds allowToolsIncompatible to all SetupNimSelectionState constructions across remote-provider, nim-local, vLLM, and routed selection paths for consistent propagation across provider selection cycles.
setup-nim-ollama.test.ts: handler behavior verification
src/lib/onboard/setup-nim-ollama.test.ts
Adds Vitest scaffolding with makeState() and makeDeps() factories. Verifies three "happy path" handlers return "selected", set provider to "ollama-local", and preserve allowToolsIncompatible: true. Adds edge-case tests: "fail closed" verifies unknown Ollama startup outcomes abort without mutating state; "safe fallback" verifies fallback-state overwrite and allowToolsIncompatible normalization to false.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

  • NVIDIA/NemoClaw#5427: Both PRs tighten cognitive-complexity thresholds in biome.json and refactor src/lib/actions/sandbox/rebuild.ts by extracting rebuild preflight/live-state logic into helper functions.
  • NVIDIA/NemoClaw#5104: Main PR's src/lib/actions/sandbox/snapshot.ts refactor adds post-restore policy reconciliation that removes/applies snapshot-recorded custom policies, similar to retrieved PR's custom-policy-preservation logic in the restore path.
  • NVIDIA/NemoClaw#5420: Modifies onboarding selection paths and shared SetupNimSelectionState, and adjusts the same Biome cognitive-complexity threshold.

Suggested labels

refactor, area: cli, area: sandbox

Poem

🐇 Hop hop, the helpers arrive,
Each function now earns its own hive!
No more inline stacks—
Just clean factored tracks,
And complexity scores take a dive! 🌿

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 12.90% 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
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title clearly summarizes the main objective: lowering the cognitive complexity ratchet below 150 through refactoring, which is the primary focus of the changeset.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch ratchet/149-rebuild-snapshot-onboard-stack

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

@github-code-quality

github-code-quality Bot commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

The overall coverage in the ratchet/149-rebuild-... 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 ratchet/149-rebuild-... 6eab9bb +/-
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 ratchet/149-rebuild-... branch is 46%. Coverage data for the main branch is not yet available.

Show a code coverage summary of the most covered files.
File main ratchet/149-rebuild-... 6eab9bb +/-
src/lib/state/o...oard-session.ts 90%
src/lib/inference/local.ts 76%
src/lib/sandbox/config.ts 72%
src/lib/actions...dbox/rebuild.ts 67%
src/lib/onboard/preflight.ts 64%
src/lib/actions...licy-channel.ts 56%
src/lib/state/sandbox.ts 55%
src/lib/policy/index.ts 49%
src/lib/onboard...er-gpu-patch.ts 44%
src/lib/onboard.ts 18%

Updated June 16, 2026 01:56 UTC
Code Coverage is in Public Preview. Learn more and provide us with your feedback.

@github-actions

github-actions Bot commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

E2E Advisor Recommendation

Required E2E: rebuild-openclaw-e2e, upgrade-stale-sandbox-e2e, snapshot-commands-e2e, cloud-onboard-e2e, gpu-e2e
Optional E2E: rebuild-hermes-e2e, state-backup-restore-e2e, gpu-double-onboard-e2e, onboard-resume-e2e

Dispatch hint: rebuild-openclaw-e2e,upgrade-stale-sandbox-e2e,snapshot-commands-e2e,cloud-onboard-e2e,gpu-e2e

Auto-dispatched E2E: gpu-e2e via nightly-e2e.yaml at 6eab9bb7ca0b9e7e5b5eabf497829d43429b8bcanightly run

Workflow run

Full advisor summary

E2E Recommendation Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required E2E

  • rebuild-openclaw-e2e (high): Required because the PR changes the destructive OpenClaw rebuild path: liveness checks, shield handling, base-image preflight, backup, delete, recreate, restore, token rotation, and policy preservation all need a real sandbox lifecycle proof.
  • upgrade-stale-sandbox-e2e (high): Required as the closest existing coverage for stale sandbox/version upgrade flows through rebuild --yes; the new rebuild live-state helper and stale-recovery branching can break this path.
  • snapshot-commands-e2e (medium): Required because snapshot restore now performs policy reconciliation and OpenClaw config permission repair after state restore; the existing snapshot create/list/restore flow should verify no regression in real restored sandbox state.
  • cloud-onboard-e2e (high): Required because setupNim/onboard provider-selection state was changed; this is the standard non-interactive hosted-provider onboard flow and catches regressions in model/provider/credential state propagation.
  • gpu-e2e (very high): Required when available because the new Ollama setup handlers directly affect local Ollama onboarding and inference routing, which this job validates with a real GPU-backed local Ollama sandbox.

Optional E2E

  • rebuild-hermes-e2e (high): Useful adjacent confidence because the refactored rebuild helpers are agent-agnostic and include agent base-image preflight behavior that also affects Hermes rebuilds.
  • state-backup-restore-e2e (medium): Useful adjacent coverage for the lower-level backup/restore primitives used by both rebuild and snapshot restore, especially after snapshot restore policy/security changes.
  • gpu-double-onboard-e2e (very high): Useful adjacent coverage for re-onboard with local Ollama/proxy token persistence after the Ollama onboarding handler refactor.
  • onboard-resume-e2e (medium): Useful adjacent coverage because rebuild recreates via onboard resume and setupNim/onboard state propagation changed, though rebuild-openclaw-e2e already exercises the primary rebuild-resume path.

New E2E recommendations

  • sandbox lifecycle / stale rebuild recovery (high): Existing stale coverage focuses on stale versions/upgrades, not the explicit case added in rebuild live-state handling where a sandbox is registered locally but absent from the healthy OpenShell gateway and rebuild recreates from preserved registry metadata without backup/restore.
    • Suggested test: Add an E2E scenario that onboards/registers a sandbox, deletes only the live OpenShell sandbox while preserving NemoClaw registry/session metadata, runs nemoclaw <name> rebuild --yes, and verifies the registry/defaultSandbox metadata is preserved and the sandbox is recreated safely.
  • snapshot restore / custom policy reconciliation (medium): Unit tests cover policy reconciliation, but the existing snapshot-commands E2E primarily validates marker-file state restore and credential absence. It does not appear to prove real built-in and custom policy add/remove reconciliation after snapshot restore.
    • Suggested test: Extend the snapshot E2E to apply built-in and custom policy presets before snapshot, mutate the target policies, restore the snapshot, and verify the target's policy presets and custom policy content exactly match the snapshot manifest.
  • onboarding / Ollama handler fallback state (medium): The new Ollama handler code has multiple local-only branches including ready, fallback, Windows-host, install, and upgrade paths; existing GPU E2E covers the common local Ollama path but not all state transitions.
    • Suggested test: Add a hermetic E2E or live scenario for Ollama startup fallback/retry that verifies provider, credentialEnv, endpointUrl, preferredInferenceApi, nimContainer, and allowToolsIncompatible are reset correctly when switching away from local Ollama.

Dispatch hint

  • Workflow: .github/workflows/nightly-e2e.yaml
  • jobs input: rebuild-openclaw-e2e,upgrade-stale-sandbox-e2e,snapshot-commands-e2e,cloud-onboard-e2e,gpu-e2e

@github-actions

github-actions Bot commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

Vitest E2E Scenario Recommendation

Required Vitest E2E scenarios: sandbox-rebuild-vitest, rebuild-openclaw-vitest, ubuntu-repo-cloud-openclaw
Optional Vitest E2E scenarios: state-backup-restore-vitest

Dispatch required Vitest E2E scenarios:

  • gh workflow run e2e-vitest-scenarios.yaml --ref <pr-head-ref> --field jobs=sandbox-rebuild-vitest
  • gh workflow run e2e-vitest-scenarios.yaml --ref <pr-head-ref> --field jobs=rebuild-openclaw-vitest
  • gh workflow run e2e-vitest-scenarios.yaml --ref <pr-head-ref> --field scenarios=ubuntu-repo-cloud-openclaw

Workflow run

Full Vitest E2E advisor summary

Vitest E2E Scenario Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required Vitest E2E scenarios

  • sandbox-rebuild-vitest: The PR refactors the sandbox rebuild flow, including live-state resolution, shields handling, base-image preflight, and backup behavior. This free-standing live Vitest job runs a real OpenClaw sandbox rebuild and validates state preservation plus registry/backup hygiene.
    • Dispatch: gh workflow run e2e-vitest-scenarios.yaml --ref <pr-head-ref> --field jobs=sandbox-rebuild-vitest
  • rebuild-openclaw-vitest: The rebuild helper extraction also affects the OpenClaw rebuild path that preserves workspace, policies, gateway-token state, and base-image refresh behavior. This wired free-standing job is the most specific live Vitest coverage for that surface.
    • Dispatch: gh workflow run e2e-vitest-scenarios.yaml --ref <pr-head-ref> --field jobs=rebuild-openclaw-vitest
  • ubuntu-repo-cloud-openclaw: The onboarding/setupNim refactor changes provider selection and normal cloud OpenClaw onboarding flow. This live-supported typed scenario exercises repo-current Ubuntu Docker onboarding with NVIDIA cloud inference and credentials validation.
    • Dispatch: gh workflow run e2e-vitest-scenarios.yaml --ref <pr-head-ref> --field scenarios=ubuntu-repo-cloud-openclaw

Optional Vitest E2E scenarios

  • state-backup-restore-vitest: Adjacent coverage for backup/restore state preservation after snapshot/restore logic changes. It does not directly drive the snapshot CLI path, so it is optional rather than required.
    • Dispatch: gh workflow run e2e-vitest-scenarios.yaml --ref <pr-head-ref> --field jobs=state-backup-restore-vitest

Relevant changed files

  • src/lib/actions/sandbox/rebuild-flow-helpers.ts
  • src/lib/actions/sandbox/rebuild.ts
  • src/lib/actions/sandbox/snapshot.ts
  • src/lib/onboard.ts
  • src/lib/onboard/setup-nim-ollama.ts
  • src/lib/onboard/setup-nim-selection.ts

@github-actions

github-actions Bot commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor

Findings: 0 needs attention, 2 worth checking, 0 nice ideas
Since last review: 0 prior items resolved, 2 still apply, 0 new items found

Review findings

🛠️ Needs attention

  • None.

🔎 Worth checking

  • Source-of-truth review needed: Snapshot restore post-restore permission and policy reconciliation: The advisor marked localized patch analysis as needs_followup.
    • Recommendation: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
    • Evidence: `runSnapshotRestore()` prints `✓ Restored ...` before calling warning-only reconciliation helpers at the end of the restore flow.
  • Snapshot restore success can mask unreconciled policy/config posture (src/lib/actions/sandbox/snapshot.ts:659): After `restoreSandboxState()` succeeds, `runSnapshotRestore()` prints `✓ Restored ...` and then performs OpenClaw config permission repair plus built-in/custom policy reconciliation as best-effort warning-only follow-up. For a sandbox security boundary, a target can be left with mutable config permissions or egress/custom policy state that differs from the snapshot while the command's primary success signal has already been emitted. The added comments and custom-policy split test reduce ambiguity, but they do not establish an explicit partial-security-posture result or cover all warning-only branches.
    • Recommendation: Either make post-restore policy/config reconciliation failures affect the restore result, or surface a clear partial-security-reconciliation state after filesystem restore so users know restored files are not equivalent to restored security posture. Add negative coverage for built-in preset apply failure, built-in preset remove failure, and OpenClaw config permission repair throw/unreadable/incomplete outcomes.
    • Evidence: `runSnapshotRestore()` logs filesystem restore success, then calls `repairRestoredOpenClawConfigPerms(targetSandbox, result)`, `reconcileSnapshotPolicyPresets(targetSandbox, resolvedSnapshot)`, and `reconcileSnapshotCustomPolicies(targetSandbox, resolvedSnapshot)`. Those helpers catch/collect failures and only `console.warn(...)`; `snapshot.test.ts` covers warning-only custom-policy removal but not built-in apply/remove or config repair failure branches.

🌱 Nice ideas

  • None.
Consider writing more tests for
  • **Runtime validation** — Snapshot restore where built-in policy preset apply fails after filesystem restore surfaces an unmistakable partial-security-posture warning or state.. The PR changes runtime/sandbox/infrastructure paths that cross OpenShell, Docker, gateway policy, process.exit interception, host installers, and local inference proxy boundaries. Unit and fake-CLI coverage improved, but runtime validation would materially improve confidence in destructive and security-posture behavior.
  • **Runtime validation** — Snapshot restore where built-in policy preset removal fails after filesystem restore surfaces an unmistakable partial-security-posture warning or state.. The PR changes runtime/sandbox/infrastructure paths that cross OpenShell, Docker, gateway policy, process.exit interception, host installers, and local inference proxy boundaries. Unit and fake-CLI coverage improved, but runtime validation would materially improve confidence in destructive and security-posture behavior.
  • **Runtime validation** — Snapshot restore where OpenClaw config permission repair throws, returns unreadable, or returns applied-but-unverified still clearly communicates partial security posture after `✓ Restored`.. The PR changes runtime/sandbox/infrastructure paths that cross OpenShell, Docker, gateway policy, process.exit interception, host installers, and local inference proxy boundaries. Unit and fake-CLI coverage improved, but runtime validation would materially improve confidence in destructive and security-posture behavior.
  • **Runtime validation** — Snapshot restore `--to --force` with an existing destination and an unresolvable source image leaves the destination live sandbox and registry entry untouched.. The PR changes runtime/sandbox/infrastructure paths that cross OpenShell, Docker, gateway policy, process.exit interception, host installers, and local inference proxy boundaries. Unit and fake-CLI coverage improved, but runtime validation would materially improve confidence in destructive and security-posture behavior.
  • **Runtime validation** — Rebuild stale-sandbox recovery clears stale shields state only after recreate success and preserves the original shields, registry, and defaultSandbox state when recreate fails.. The PR changes runtime/sandbox/infrastructure paths that cross OpenShell, Docker, gateway policy, process.exit interception, host installers, and local inference proxy boundaries. Unit and fake-CLI coverage improved, but runtime validation would materially improve confidence in destructive and security-posture behavior.
  • **Acceptance clause:** No linked issue acceptance clauses were available in the deterministic GitHub context. — add test evidence or identify existing coverage. `linkedIssues` was empty in the validation context, so there were no issue body/comment clauses to map literally.
  • **Snapshot restore post-restore permission and policy reconciliation** — `snapshot.test.ts` covers a warning-only custom-policy removal failure and custom-name/built-in split, but does not cover built-in preset apply failure, built-in preset removal failure, or OpenClaw config permission repair throw/unreadable/incomplete outcomes.. `runSnapshotRestore()` prints `✓ Restored ...` before calling warning-only reconciliation helpers at the end of the restore flow.
Since last review details

Current findings:

  • Source-of-truth review needed: Snapshot restore post-restore permission and policy reconciliation: The advisor marked localized patch analysis as needs_followup.
    • Recommendation: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
    • Evidence: `runSnapshotRestore()` prints `✓ Restored ...` before calling warning-only reconciliation helpers at the end of the restore flow.
  • Snapshot restore success can mask unreconciled policy/config posture (src/lib/actions/sandbox/snapshot.ts:659): After `restoreSandboxState()` succeeds, `runSnapshotRestore()` prints `✓ Restored ...` and then performs OpenClaw config permission repair plus built-in/custom policy reconciliation as best-effort warning-only follow-up. For a sandbox security boundary, a target can be left with mutable config permissions or egress/custom policy state that differs from the snapshot while the command's primary success signal has already been emitted. The added comments and custom-policy split test reduce ambiguity, but they do not establish an explicit partial-security-posture result or cover all warning-only branches.
    • Recommendation: Either make post-restore policy/config reconciliation failures affect the restore result, or surface a clear partial-security-reconciliation state after filesystem restore so users know restored files are not equivalent to restored security posture. Add negative coverage for built-in preset apply failure, built-in preset remove failure, and OpenClaw config permission repair throw/unreadable/incomplete outcomes.
    • Evidence: `runSnapshotRestore()` logs filesystem restore success, then calls `repairRestoredOpenClawConfigPerms(targetSandbox, result)`, `reconcileSnapshotPolicyPresets(targetSandbox, resolvedSnapshot)`, and `reconcileSnapshotCustomPolicies(targetSandbox, resolvedSnapshot)`. Those helpers catch/collect failures and only `console.warn(...)`; `snapshot.test.ts` covers warning-only custom-policy removal but not built-in apply/remove or config repair failure branches.

Workflow run details

This is an automated advisory review. A human maintainer must make the final merge decision.

cv added 5 commits June 14, 2026 18:19
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Comment thread src/lib/actions/sandbox/rebuild-flow-helpers.ts Fixed
cv and others added 3 commits June 15, 2026 08:22
…ort, function or class'

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Signed-off-by: Carlos Villela <cvillela@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.

Actionable comments posted: 1

🧹 Nitpick comments (1)
src/lib/actions/sandbox/rebuild.ts (1)

639-673: Run the rebuild channel lifecycle E2E before merge.

Given this file controls rebuild flow behavior, please run channels-stop-start-e2e to validate channel disable/enable persistence across rebuild with cached credentials.

As per coding guidelines: src/lib/actions/sandbox/rebuild.ts changes should be validated with channels-stop-start-e2e.

🤖 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/sandbox/rebuild.ts` around lines 639 - 673, The changes to
the rebuild flow in the openRebuildShieldsWindowForState,
relockRebuildShieldsWindow, and backupSandboxStateForRebuild code paths need
validation through end-to-end testing. Run the channels-stop-start-e2e E2E test
to confirm that channel disable/enable persistence is properly maintained across
rebuild operations with cached credentials before merging this change.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/lib/actions/sandbox/snapshot.ts`:
- Around line 600-635: The preflight validation of fromImage at lines 601-608 is
decoupled from the actual recreate operation at line 634. The validated
fromImage value is not passed to autoCreateSandboxFromSource, causing it to
re-probe the image internally — if this re-probe fails after the destination has
been deleted, the operation aborts in a destructive state. Modify the call to
autoCreateSandboxFromSource to accept and use the pre-validated fromImage value
instead of re-probing, ensuring the preflight validation is directly bound to
the actual recreate step and no transient failures can occur after destination
deletion.

---

Nitpick comments:
In `@src/lib/actions/sandbox/rebuild.ts`:
- Around line 639-673: The changes to the rebuild flow in the
openRebuildShieldsWindowForState, relockRebuildShieldsWindow, and
backupSandboxStateForRebuild code paths need validation through end-to-end
testing. Run the channels-stop-start-e2e E2E test to confirm that channel
disable/enable persistence is properly maintained across rebuild operations with
cached credentials before merging this change.
🪄 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: 461ff0b2-585b-4444-b12f-2c3030379d6d

📥 Commits

Reviewing files that changed from the base of the PR and between b490f8b and 28bf3f9.

📒 Files selected for processing (11)
  • biome.json
  • src/lib/actions/sandbox/rebuild-flow-helpers.ts
  • src/lib/actions/sandbox/rebuild.ts
  • src/lib/actions/sandbox/snapshot.test.ts
  • src/lib/actions/sandbox/snapshot.ts
  • src/lib/onboard.ts
  • src/lib/onboard/setup-nim-ollama.test.ts
  • src/lib/onboard/setup-nim-ollama.ts
  • src/lib/onboard/setup-nim-selection.test.ts
  • src/lib/onboard/setup-nim-selection.ts
  • test/rebuild-credential-preflight.test.ts

Comment thread src/lib/actions/sandbox/snapshot.ts
@github-actions

Copy link
Copy Markdown
Contributor

Selective E2E Results — ✅ All requested jobs passed

Run: 27587386845
Target ref: 28bf3f9bfdf987757a919158c9284e2fd7bad0d3
Workflow ref: main
Requested jobs: double-onboard-e2e,gpu-e2e
Summary: 1 passed, 0 failed, 0 cancelled, 1 skipped

Job Result
double-onboard-e2e ✅ success
gpu-e2e ⏭️ skipped

Comment thread src/lib/actions/sandbox/snapshot.test.ts Fixed
@github-actions

Copy link
Copy Markdown
Contributor

Selective E2E Results — ⚠️ Run cancelled — no signal

Run: 27588267532
Target ref: 89f1d4d993c78311c54dac06307dabc8d5d5fc47
Workflow ref: main
Requested jobs: double-onboard-e2e,gpu-e2e
Summary: 0 passed, 0 failed, 1 cancelled, 1 skipped

Job Result
double-onboard-e2e ⚠️ cancelled
gpu-e2e ⏭️ skipped

@github-actions

Copy link
Copy Markdown
Contributor

Selective E2E Results — ⚠️ No requested jobs ran

Run: 27588687406
Target ref: 51d9dc1b64859be0eca9b98afb2b3d9e24407c13
Workflow ref: main
Requested jobs: gpu-e2e
Summary: 0 passed, 0 failed, 0 cancelled, 1 skipped

Job Result
gpu-e2e ⏭️ skipped

@github-actions

Copy link
Copy Markdown
Contributor

Selective E2E Results — ⚠️ No requested jobs ran

Run: 27588930949
Target ref: 6eab9bb7ca0b9e7e5b5eabf497829d43429b8bca
Workflow ref: main
Requested jobs: gpu-e2e
Summary: 0 passed, 0 failed, 0 cancelled, 1 skipped

Job Result
gpu-e2e ⏭️ skipped

@cv
cv merged commit 6c0fb04 into main Jun 16, 2026
33 checks passed
@cv
cv deleted the ratchet/149-rebuild-snapshot-onboard-stack branch June 16, 2026 02:19
@wscurran wscurran added area: cli Command line interface, flags, terminal UX, or output area: sandbox OpenShell sandbox lifecycle, runtime, config, or recovery refactor PR restructures code without intended behavior change labels Aug 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: cli Command line interface, flags, terminal UX, or output area: sandbox OpenShell sandbox lifecycle, runtime, config, or recovery refactor PR restructures code without intended behavior change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants