Repository navigation
fix(sandbox): preserve identity-free create recovery authority - #13170
Conversation
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>
|
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. |
Code coverage overviewLanguages: TypeScript TypeScript / code-coverage/plugin
View coverage of the 10 most impacted files.
TypeScript / code-coverage/cli
View coverage of the 10 most impacted files.
ℹ️ Calculations represent line coverage. Results may change as calculations are updated. Updated |
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>
|
🌿 Preview your docs: https://nvidia-preview-pr-13170.docs.buildwithfern.com/nemoclaw |
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>
|
Reviewed commit 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:
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 This assessment is bound to |
Signed-off-by: Hung Le <hple@nvidia.com>
|
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. |
|
PR Review Advisor finished for commit Request review only when Require no Advisor blockers is green. |
Signed-off-by: Hung Le <hple@nvidia.com>
|
Approval remains pending for
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. |
|
The requested E2E targets passed on |
sandl99
left a comment
There was a problem hiding this comment.
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.
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 explicitdestroy --forceto 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
--forcearchives eligible local tracking before retirement. Conflicting same-name sessions and sandbox-scoped resource ownership still block retirement; shared providers remain unchanged.Verification
c15e9218bf7ea7b064b6be1755eb4a64bfc10f2e; PR base and trustedmainworkflow 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.e2e-dispatch-38706144876-2, artifact11960959475, ZIP SHA-2563712b67cfa7e03d76c43051645ccf37bce11347d90b22f82f913075a953b6a9e. Correlation:84539ce4-bd3b-414e-b320-994937d0e79c. Aggregate resultreview-queue-e2e-result-38706144876-2reportspass. 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.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 usingvitest run --project cli --project integration --maxWorkers 1with 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.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.abf42f10a54617128c1464d33592164356660e43passed 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.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--yesand 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.ef5d55d19e2da33c2474db158588613828e1a9be) plus patch SHA-2567fb0da1fb249dbff9a2452c501f323f3619ccc7264acecb07c8efbc7336dae82. The first observer relocation, before commita43a46a5b2b8e174c2a40f257a13ce498713c893, 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
c15e9218bf7ea7b064b6be1755eb4a64bfc10f2eagainst base80491e92e2cda863d6bf42869a8f8a5b0a975628. The earlier CodeRabbit review reviewedb5124a72d911ed3a49234cb900d63895ab302a17against 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
b5124a72against 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
0c0bdb868bf002bbd475d4678107f9a424375bb6againste7840acd43451ae79191854aacafdf9e206983ed. 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
b5124a72requests latest-commit live E2E evidence and resolution of the separate Advisor blocker gate before approval. The selected E2E requirement is now supported onc15e9218by 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:
credentialEnvironmentVariablesis 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
--forceoption 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.Documentation
destroyand--force.