Skip to content

fix(sandbox): preserve identity-free create recovery authority - #13170

Merged
sandl99 merged 9 commits into
mainfrom
codex/fix-identity-free-recovery-13071
Oct 15, 2026
Merged

sandl99 merged 9 commits into
mainfrom
codex/fix-identity-free-recovery-13071

Conversation

@hunglp6d

@hunglp6d hunglp6d commented Oct 14, 2026 •

Copy link
Copy Markdown
Collaborator

Outcome

On native Linux with default Docker, failed creates with no sandbox identity can release their local name reservation after confirmed absence, even after a gateway restart or onboarding for another sandbox. New records preserve the authority needed for destroy --yes; eligible legacy records with lost authority require explicit destroy --force to archive and discard local tracking. Forced discard does not establish original runtime cleanup.

Reason

The recovery path added in #12839 depended on the original session and a gateway marker predating the recovery record. Restarting the gateway or starting another session can remove that evidence while the retained name remains blocked. Shared inference providers alone do not cause this refusal.

Related issues

Fixes #13071. Follows #12839 and #12260.

Changes

  • Capture the reservation owner, Docker daemon ID, and gateway state directory before create on a qualified native Linux default-Docker gateway. Preserve them through the failure writer and session reconstruction so recovery can qualify the original create after the session changes.
  • Store records containing this authority as schema 2 so older CLIs refuse them instead of dropping the evidence. Continue reading schema 1 and preserve authority during legacy port migration.
  • Use captured authority for metadata retirement while retaining absence checks, locking, revalidation, and reservation compare-and-swap. Explicit --force archives eligible local tracking before retirement. Conflicting same-name sessions and sandbox-scoped resource ownership still block retirement; shared providers remain unchanged.
  • Share the native authority observer through the existing gateway state-ownership module. Destroy imports that owner directly; the create boundary re-exports the same helper for its existing caller. The persisted parser and type remain with the state owner. This dependency cleanup preserves the observer's function body and adds no source file, package dependency, or budget exception.

Verification

  • Selected live E2E: run 38706144876, attempt 2, passed on GitHub-hosted Ubuntu Linux x64 with Docker. Tested commit: c15e9218bf7ea7b064b6be1755eb4a64bfc10f2e; PR base and trusted main workflow revision: 80491e92e2cda863d6bf42869a8f8a5b0a975628. Selectors: onboard-repair,onboard-resume,rebuild-hermes,rebuild-openclaw. All five executions ran one test and passed, with zero skipped tests, unhandled errors, or recorded cleanup failures.
  • Selected jobs: OpenClaw repair, OpenClaw resume, Hermes resume, OpenClaw rebuild, and Hermes rebuild. OpenClaw rebuild also passed its Docker kill/Error-state recovery check with preserved workspace and native-root markers.
  • Evidence: dispatch receipt e2e-dispatch-38706144876-2, artifact 11960959475, ZIP SHA-256 3712b67cfa7e03d76c43051645ccf37bce11347d90b22f82f913075a953b6a9e. Correlation: 84539ce4-bd3b-414e-b320-994937d0e79c. Aggregate result review-queue-e2e-result-38706144876-2 reports pass. The receipt, aggregate, and all five execution manifests, test counts, phase results, and cleanup records were checked against the same source/workflow/run attempt; artifact digests were verified. Attempt 1 stopped before live execution because the candidate image producer was still running. Attempt 2 passed after candidate image publication completed, without a source change. This is focused E2E evidence, not full-suite or all-platform qualification.
  • Latest local validation: c15e9218bf7ea7b064b6be1755eb4a64bfc10f2e, Linux x86_64 / Node.js 22.22.2. The retained-recovery CLI test file and fresh-create integration file passed 65 tests with no skips using vitest run --project cli --project integration --maxWorkers 1 with those two file paths. CLI typecheck, scoped formatting/lint, and 13 selected repository checks passed. The committed diff and source hashes match the validated two-file change. Normal commit hooks passed, including growth and source-shape checks. No full local suite was run for this test-only reduction. Current-commit live E2E results are recorded above.
  • The producer-to-destroy integration case runs real create-authority capture, orchestration, registry, and retained-record persistence. A separate process reloads the record after a simulated gateway restart and session replacement, calls destroySandbox, and verifies selected metadata retirement, unrelated-state preservation, and no ordinary resource cleanup. External Docker/OpenShell observations are mocked. The latest commit removes one unit case already covered by that integration case and two lines comparing file bytes around a read-only load. It keeps the fresh-process record comparison, authority/nonce checks, unrelated-state assertions, and cleanup guards; it adds no test cases.
  • Earlier implementation evidence remains revision-bound: the candidate based on abf42f10a54617128c1464d33592164356660e43 passed 253 focused CLI tests plus native CLI/plugin builds and type checks. Current focused local validation supplements those checks without rerunning the full local suite.
  • Live evidence: Linux x86_64, Docker 29.2.1, OpenShell 0.0.116, Ollama 0.32.9 / qwen3.5:9b, sandbox GPU disabled. A control using an earlier candidate before schema fencing onboarded without fault injection. Fault runs killed their dedicated gateway after a real Docker create. After native runtime removal, new-authority recovery passed --yes; a stock v0.0.133 schema-1 record refused --yes and passed explicit --force. Both names then onboarded with new identities. Presence and wrong-endpoint cases refused retirement; shared providers and unrelated state remained unchanged. Recovery records came from the real failure path.
  • Those live recovery consumers used v0.0.133 (ef5d55d19e2da33c2474db158588613828e1a9be) plus patch SHA-256 7fb0da1fb249dbff9a2452c501f323f3619ccc7264acecb07c8efbc7336dae82. The first observer relocation, before commit a43a46a5b2b8e174c2a40f257a13ce498713c893, passed build, focused tests, and a compiled read-only daemon probe. That probe does not qualify the current relocation. The current relocation has the local validation and selected live E2E above. The original identity-free create-failure timing was not rerun on this commit, and no new agent tool-call result is claimed. Original logs, manifests, snapshots, and the legacy archive remain available locally.

Review notes

Publication commit: c15e9218bf7ea7b064b6be1755eb4a64bfc10f2e; PR base: 80491e92e2cda863d6bf42869a8f8a5b0a975628.

CI run 38703642625 passed on publication commit c15e9218bf7ea7b064b6be1755eb4a64bfc10f2e against base 80491e92e2cda863d6bf42869a8f8a5b0a975628. The earlier CodeRabbit review reviewed b5124a72d911ed3a49234cb900d63895ab302a17 against the recorded base and generated no actionable comments. Its 26.32% docstring-coverage result is a separate advisory warning; no docstrings were added solely to satisfy that metric. That CodeRabbit result predates the test reduction and does not establish the current automated-review disposition.

Advisor run 38689464981 completed all nine specialists on b5124a72 against the recorded base: eight clear, including Architecture and Verification, with one repeated archive-retention P1 (F-operability-recovery-7cecf90ed7d07a2f09a5). The gate failed on that finding; there was no missing specialist analysis. The test reduction does not resolve that gate. The archive writer is unchanged from the commit San reviewed. Resolution of the Advisor blocker gate remains a separate repository review-process requirement.

San's review found no confirmed code defect in 0c0bdb868bf002bbd475d4678107f9a424375bb6 against e7840acd43451ae79191854aacafdf9e206983ed. His macOS / Node.js 25.9.0 validation passed 98 focused tests and skipped 39 Linux-only recovery tests. His assessment remains bound to that revision and does not approve the publication commit.

San's follow-up on b5124a72 requests latest-commit live E2E evidence and resolution of the separate Advisor blocker gate before approval. The selected E2E requirement is now supported on c15e9218 by attempt 2 and the evidence linked above. San reiterates that the stated retention P1 is unsubstantiated and needs a separate policy; he does not request automatic archive pruning. His assessment remains recorded here. This E2E update does not claim that the Advisor gate is cleared or that maintainer approval has been granted.

The author's retained-state choices remain:

  • Keep private archives after forced local discard while original-runtime cleanup is unverified. A retry reuses the same record's archive; distinct discarded records can accumulate. Automatic pruning needs a separate retention policy and removal conditions.
  • Keep the existing ownership checks without refusing solely because credentialEnvironmentVariables is nonempty. That list contains configured names/references; staged sandbox-provider receipts are recorded separately and block retirement. Forced discard archives the original record and reservation. This does not establish cleanup of every credential-bearing resource or of the original daemon.

San's source assessment supports treating the archive-retention and credential-reference P1 claims as unsubstantiated as stated; it also confirms the earlier same-name-session guard and regression case. His comment contains the source references. These choices do not assert maintainer approval of the publication commit.

Scope remains identity-free local recovery on native Linux default Docker. Cross-root work (#12520/#12452), inference failures addressed by #12826, runtime deletion without an identity, and support for other runtime providers remain outside this PR. Forced local discard cannot reconstruct lost historical ownership or prove absence from the original daemon.


Signed-off-by: Hung Le hple@nvidia.com

Summary by CodeRabbit

  • Bug Fixes

    • Improved recovery for Linux systems using default Docker when a sandbox identity is unavailable. Recovery verifies that the gateway and Docker confirm the sandbox is absent and that ownership evidence meets the applicable checks before removing local tracking.
    • The --force option can archive and remove eligible local recovery records after absence checks. It preserves runtime resources and shared providers; cleanup on the original runtime remains unverified.
    • Recovery remains blocked when conflicting active sessions, sandbox-scoped providers, or other resource ownership are detected.
  • Documentation

    • Clarified recovery requirements, limits, and archive retention behavior for destroy and --force.

Capture the reservation owner and native Docker binding before create
requests, and retain them across gateway restarts and session changes.
Version records with this evidence so older CLIs cannot drop it.

Allow explicit archived local discard for eligible legacy records after
current absence checks. Keep ownership, resource, locking, and retry
guards for automatic recovery, and preserve unrelated session state.

Fixes #13071

Signed-off-by: Hung Le <hple@nvidia.com>
@copy-pr-bot

copy-pr-bot Bot commented Oct 14, 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 Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: NVIDIA/NemoClaw/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Enterprise
  • Run ID: a5b3dbf6-8a7a-442d-93bf-fc70de29699f
📥 Commits

Reviewing files that changed from the base of the PR and between b5124a7 and c15e921.

📒 Files selected for processing: 2
  • src/lib/actions/sandbox/lifecycle/retained-recovery.test.ts
  • test/onboarding/onboard-fresh-create-identity.test.ts
💤 Files with no reviewable changes: 2
  • test/onboarding/onboard-fresh-create-identity.test.ts
  • src/lib/actions/sandbox/lifecycle/retained-recovery.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.

📝 Walkthrough

Eligible native Linux sandbox creates can record default-Docker and gateway authority with retained recovery data. Destroy checks ownership and absence before retiring recovery. With --force, it archives and removes eligible local tracking without asserting cleanup on the original runtime.

Changes: Identity-free sandbox recovery

Layer / File(s) Summary
Capture create authority
src/lib/onboard/gateway/state-ownership.ts, src/lib/onboard/host-gateway-process.ts, src/lib/onboard/sandbox-create/*, src/lib/onboard/sandbox-gpu-create-*
The create flow captures eligible native Docker authority before each create attempt and associates it with the attempt nonce for retained recovery.
Persist and validate recovery authority
src/lib/state/onboard-session/retained-sandbox-recovery.ts, src/lib/state/onboard-session.ts, src/lib/state/legacy-port-migration.ts, src/lib/state/*test.ts
Recovery records can store validated create authority in schema version 2; records without authority remain schema version 1. Session persistence, migration, and archival carry or validate the authority.
Qualify and retire recovery
src/lib/actions/sandbox/destroy*.ts, src/lib/actions/sandbox/lifecycle/retained-recovery*, src/commands/sandbox/destroy.ts, docs/reference/commands.mdx, src/lib/onboard/lifecycle-contracts.md, test/onboarding/onboard-fresh-create-identity.test.ts
Destroy checks ownership and absence before retirement. Forced local discard archives eligible recovery data and leaves original-runtime cleanup unverified. Tests cover authority changes, conflicts, archival, retries, and preservation of unrelated state.

Priority: ➖ Normal

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

Change: Bug fix · Severity of issue fixed: Medium

Sequence Diagram(s)

sequenceDiagram
  participant CreateFlow
  participant AuthorityCapture
  participant DockerDaemon
  participant RecoveryStore
  CreateFlow->>AuthorityCapture: capture authority before create attempt
  AuthorityCapture->>DockerDaemon: query default daemon identity
  DockerDaemon-->>AuthorityCapture: daemon identity
  AuthorityCapture-->>CreateFlow: authority associated with attempt nonce
  CreateFlow->>RecoveryStore: persist retained recovery with authority
Loading

Suggested reviewers: ericksoa, afourniernv

Merge Risk: ⚪ Minimal · up to c15e9

The incremental changes only add tests for identity-free recovery after a gateway restart and for forced local discard. No concrete code risk was found in them. Live E2E evidence and the maintainer disposition of the Advisor gate are still pending.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 26.32% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 38 functions across 19 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Linked Issues check Passed The PR satisfies the coding requirements in directly linked issue #13071. reconcileIdentityFreeRecovery still requires confirmed OpenShell and Docker absence, but it no longer rejects a record only …
Out of Scope Changes check Passed The changes stay within #13071. Authority capture, schema handling, session propagation, recovery checks, force archival, documentation, and regression tests directly support identity-free local recov…
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely describes the main change: preserving recovery authority for identity-free sandbox creation failures.
  • Fix all pre-merge checks with AI
✨ Finishing Touches: 💡 1
📝 Generate docstrings: 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

@github-code-quality

github-code-quality Bot commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

Code coverage overview

Languages: TypeScript

TypeScript / code-coverage/plugin

main 63002cd codex/fix-identity-f... c15e921 +/-
96% 97% +1%
View coverage of the 10 most impacted files.
File main 63002cd codex/fix-identity-f... c15e921 +/-
nemoclaw/src/onboard/config.ts 98% 96% -2%
nemoclaw/src/index.ts 94% 93% -1%
nemoclaw/src/bl...print/runner.ts 95% 95% 0%
nemoclaw/src/bl...ime-identity.ts 97% 97% 0%
nemoclaw/src/bl...t-management.ts 100% 100% 0%
nemoclaw/src/co.../config-show.ts 100% 100% 0%
nemoclaw/src/commands/slash.ts 100% 100% 0%
nemoclaw/src/ru...time-context.ts 100% 100% 0%
nemoclaw/src/sh...er-boundary.cts 100% 100% 0%
nemoclaw/src/on...native-route.ts 0% 100% +100%

TypeScript / code-coverage/cli

main 63002cd codex/fix-identity-f... c15e921 +/-
84% 86% +2%
View coverage of the 10 most impacted files.
File main 63002cd codex/fix-identity-f... c15e921 +/-
src/lib/onboard.ts 62% 47% -15%
src/lib/actions...ess-recovery.ts 65% 78% +13%
src/lib/state/l...diness-lease.ts 66% 83% +17%
src/lib/inferen...ollama/proxy.ts 41% 61% +20%
src/lib/onboard.../application.ts 55% 84% +29%
src/lib/domain/...y/dimensions.ts 0% 71% +71%
src/lib/actions...ry/operation.ts 0% 76% +76%
src/lib/actions...try/snapshot.ts 0% 78% +78%
src/lib/inferen...ovider/index.ts 0% 88% +88%
src/lib/onboard...ternal-image.ts 0% 91% +91%

ℹ️ Calculations represent line coverage. Results may change as calculations are updated.


Updated October 15, 2026 08:29 UTC

hunglp6d and others added 2 commits October 14, 2026 19:57
Exercise the real create orchestration and recovery writer when the
gateway disconnects before returning a sandbox identity. Check that
Docker authority is observed before create and retained for its nonce.

Clarify normal recovery and forced local discard in the command docs.

Signed-off-by: Hung Le <hple@nvidia.com>
@github-actions

Copy link
Copy Markdown
Contributor

hunglp6d and others added 4 commits October 14, 2026 20:40
Describe normal recovery without listing a destroy flag in the onboard
section. Keep the explicit force behavior in the destroy reference.

Signed-off-by: Hung Le <hple@nvidia.com>
Consume the persisted record in a fresh process after a simulated
gateway restart. Preserve unrelated state and forbid resource cleanup.

Signed-off-by: Hung Le <hple@nvidia.com>
@sandl99

sandl99 commented Oct 15, 2026

Copy link
Copy Markdown
Collaborator

Reviewed commit 0c0bdb868bf002bbd475d4678107f9a424375bb6 against base e7840acd43451ae79191854aacafdf9e206983ed.

I found no confirmed code defects in the 20-file diff. The create-authority capture, schema fencing, session reconstruction, locking, revalidation, and reservation compare-and-swap preserve the intended local recovery boundary. Forced discard archives the evidence and leaves original-runtime cleanup unverified.

My assessment of the findings collected from Advisor attempt 2:

  • Credential references (F-security-built-in-quality-f14bdae804607d7aa786): P1 not substantiated. The record producer records configured inference, web-search, and messaging environment-variable names. These names alone do not establish an outstanding sandbox-owned provider. Staged provider receipts populate sandboxScopedProviders, which the recovery guard rejects. A blanket refusal for credential references would also block shared-inference recovery. A valid P1 needs a concrete credential resource that escapes this ownership tracking.
  • Archive retention (F-operability-recovery-0028b7f27e8da0d541eb): follow-up policy concern; P1 not substantiated. The archive writer uses one file per record and reuses it on retry. Distinct forced discards can accumulate archives. Those archives preserve evidence while original cleanup remains unverified. A retention policy needs removal conditions before automatic pruning is introduced.
  • Earlier same-name replacement-session finding: not substantiated. The ownership guard still requires matching recovery evidence for a same-name session under --force. The existing regression case asserts refusal and preserved state.

Local validation on macOS / Node.js 25.9.0: 98 focused CLI tests passed; 39 Linux-only recovery tests were skipped. The shared TypeScript boundary build and git diff --check passed. I did not independently rerun live Linux recovery or add a forced-recovery test with credential references.

This assessment is bound to 0c0bdb868b. The PR now points to 8801c7225c7d1fba119a172cee43601a34869219; I have not revalidated that commit. Since the original review, Advisor attempt 3 completed all nine specialists but failed Require no Advisor blockers. The earlier incomplete-migration status describes attempt 2 only. This comment does not resolve the current gate or constitute merge approval.

Signed-off-by: Hung Le <hple@nvidia.com>
@hunglp6d

Copy link
Copy Markdown
Collaborator Author

Thanks, @sandl99 . I've recorded your assessment and the rationale in the PR body, keeping the existing ownership guards and evidence-preserving archives.

In b5124a7, I also moved the unchanged authority observer into the shared gateway module to address the Advisor dependency finding. All 73 existing focused Linux tests passed without skips; no test cases were added. CI is running, and I'll check the new Advisor results when available.

@github-actions

Copy link
Copy Markdown
Contributor

PR Review Advisor finished for commit b5124a7. Include the Advisor findings in the complete PR feedback collection. Verify and group valid findings before repair.

Request review only when Require no Advisor blockers is green.

All previous runs

@hunglp6d
hunglp6d marked this pull request as ready for review October 15, 2026 07:00
Signed-off-by: Hung Le <hple@nvidia.com>
@sandl99

sandl99 commented Oct 15, 2026

Copy link
Copy Markdown
Collaborator

Approval remains pending for b5124a72d911ed3a49234cb900d63895ab302a17. Please complete these requirements:

  1. Provide passing live E2E evidence for the latest PR commit. Run the selected recovery and rebuild targets through the trusted main E2E workflow: onboard-repair, onboard-resume, rebuild-hermes, and rebuild-openclaw. Link the runs and relevant jobs. Include the tested commit, base commit, workflow revision, selectors, and dispatch receipt. If a selected target cannot run, identify the blocker; a skipped target does not establish a pass. The green portable-profile and managed-startup checks cover different paths.
  2. Resolve the remaining Advisor finding and blocker gate. Advisor run 38689464981 completed all nine specialists, but Require no Advisor blockers failed on archive retention (F-operability-recovery-7cecf90ed7d07a2f09a5). My assessment remains that the stated P1 is unsubstantiated and retention needs a separate policy. Record the maintainer disposition and resolve the gate through the repository process. This request does not require automatic archive pruning.

The PR is now marked ready for review, so the earlier draft-state blocker is cleared. Keep the verification summary current. If the source changes, provide evidence for the new commit before approval.

@hunglp6d

Copy link
Copy Markdown
Collaborator Author

The requested E2E targets passed on c15e9218 in run 38706144876 (attempt 2): all five executions passed with no skips or recorded cleanup failures. The PR body now includes the tested commit, base/workflow revision, dispatch receipt, and job links.

@sandl99 sandl99 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Approved commit c15e9218bf7ea7b064b6be1755eb4a64bfc10f2e against base 80491e92e2cda863d6bf42869a8f8a5b0a975628.

I reviewed the shared authority-observer relocation and the subsequent test reduction. The observer logic is unchanged. The retained producer-to-destroy integration case still verifies recovery after a gateway restart and a different-name session, including preservation of unrelated state. I found no unresolved correctness or security defect.

The requested live E2E requirement is satisfied by run 38706144876, attempt 2. All four selectors passed across five executions: onboard-repair, onboard-resume for OpenClaw and Hermes, rebuild-hermes, and rebuild-openclaw. I verified that the dispatch receipt and all five execution manifests match this commit and the trusted workflow revision. Cleanup records report no failures. Required CI and the trusted approval checker also passed.

Maintainer disposition of F-operability-recovery-7cecf90ed7d07a2f09a5: archive retention is a non-blocking policy follow-up; the stated P1 is unsubstantiated. Forced local discard leaves original-runtime cleanup unverified, so preserving ownership evidence is appropriate. Retries reuse the same record's archive. Distinct forced discards can accumulate archives; any removal policy must define when that evidence may be deleted. Automatic pruning is not required in this recovery fix. The historical Advisor blocker result remains recorded; this approval records my disposition of that finding.

@sandl99
sandl99 merged commit 4e64d22 into main Oct 15, 2026
@sandl99
sandl99 deleted the codex/fix-identity-free-recovery-13071 branch October 15, 2026 13:22
@wscurran wscurran added area: sandbox OpenShell sandbox lifecycle, runtime, config, or recovery area: security Security controls, permissions, secrets, or hardening bug-fix PR fixes a bug or regression platform: container Affects Docker, containerd, Podman, or images labels Oct 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: sandbox OpenShell sandbox lifecycle, runtime, config, or recovery area: security Security controls, permissions, secrets, or hardening bug-fix PR fixes a bug or regression platform: container Affects Docker, containerd, Podman, or images

Projects

None yet

3 participants