refactor(sandbox): retire network-interface compatibility guard - #11400
Merged
Merged
Conversation
Signed-off-by: Aaron Erickson <aerickson@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. |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 12 included reviews per hour; 10 remain after this review. 📝 WalkthroughWalkthroughThe change retires the Ciao network guard. Runtime installation, recovery, fixtures, and tests now use only the sandbox safety-net preload. Qualification artifacts and full E2E tests validate native network-interface discovery. ChangesNetwork guard retirement
Estimated code review effort: 4 (Complex) | ~45 minutes Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant FullE2E as full-e2e.test.ts
participant Sandbox
participant Node
participant Gateway
FullE2E->>Sandbox: Start or recover sandbox
FullE2E->>Node: Inspect native network interfaces
Node->>Gateway: Discover interfaces and report errors
FullE2E->>FullE2E: Validate network and gateway state
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The obsolete Ciao compatibility layer is removed while native discovery, recovery, and the remaining sandbox safety net are retained. Validation passed and no actionable merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 11.11% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 18 functions across 16 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall line coverage in commit 45d1b44 in the TypeScript / code-coverage/cliThe overall line coverage in commit 45d1b44 in the Show a line coverage summary of the most impacted files.
Updated |
Preserve both native permission and network-interface lifecycle assertions. Refresh the authenticated Pi receipt pair without changing its image inputs. Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
wscurran
added
area: networking
Contributor
Author
|
@coderabbitai review Please review candidate |
Contributor
|
✅ Action performedReview finished.
|
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Contributor
Author
|
@coderabbitai review Current candidate: |
Contributor
|
✅ Action performedReview finished.
|
ericksoa
marked this pull request as ready for review
September 10, 2026 18:56
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
ericksoa
added a commit
that referenced
this pull request
Sep 12, 2026
## Outcome OpenShell-managed OpenClaw uses native home locations for Git, XDG configuration/data/state, and Python user installs. npm user installs move from `/tmp/npm-global` to `/sandbox/.local`, with `~/.local/bin` available to agent commands and connect sessions. ## Reason Ordinary preferences and installed tools were redirected to temporary directories to avoid home dotfiles. OpenShell already provides the writable sandbox home. ### Related issues Closes #11258. Part of #11255. ## Changes - Remove obsolete configuration redirects and temporary-directory provisioning. Keep disposable caches and existing GnuPG, Claude, and history locations. - Set HOME and user-tool PATH after the root one-shot command switches to the sandbox user; preserve the supervisor's trusted PATH. - Keep the temporary Git override private to the direct topology’s separate gateway process and retain its trusted PATH. Shared shells add the user-bin directory only for the sandbox account. Connect sessions and one-shot commands use native Git configuration; HOME permissions are unchanged. - Allow the native npm-installed Claude executable while retaining existing temporary-install paths. Refresh the existing Pi receipts because Pi images include this shared policy. - Document restart persistence and the existing rebuild/snapshot boundary. No migration, new volume, snapshot expansion, or state registry. - Reuse existing tests for native Git/Python behavior and both user topologies. The config fixture creates a private temporary directory for its files and wrapper. Production code is 10 lines smaller. The complete change adds 33 lines overall, including tests, documentation, and receipt metadata. The final PATH repair changes two existing expressions and extends two existing fixtures by 28 lines; it adds no runtime mechanism or test file. ## Verification - Final repair: 35 existing regression cases passed, with 2 Linux-only process-inspection cases skipped on macOS. The gateway launcher and sh/bash shared-environment fixtures exercise a harmless user-bin sentinel, retaining the Git and credential assertions. - A Linux AMD64 before/after probe used actual root 0, sandbox 998 and gateway 999 identities on the authenticated 322bd OpenClaw image. Before, direct gateway launch and gateway sh/bash shells executed the sandbox-owned sentinel as 999. After, gateway/root could not resolve it while sandbox sh/bash still executed it as 998. Both exact isolated containers were removed. This is function/UID evidence, not full PID 1 or OpenShell startup qualification. - `npm run validate:pr` passed on `b7d908167` in an isolated checkout using canonical validator code, authenticated SDK 0.0.116 dependencies and generated artifacts from that commit. Formatting, lint, ShellCheck, schemas, secret scanning, repository/growth checks, commitlint and CLI type-checking passed with every applicable gate enabled. No publication exception was needed for this repair. - Both Pi receipt files are the original authenticated bytes from [image run 34634582790](https://github.com/NVIDIA/NemoClaw/actions/runs/34634582790), source `322bd367`, attempt 1. All 25 Pi image-input paths remain identical on `b7d908167`; the source-parity gate passed. - [Core CI](https://github.com/NVIDIA/NemoClaw/actions/runs/34643362121) passed on `b7d908167`, including all 12 test shards, type-checking, static checks and Pi receipt freshness. - [Managed-image qualification, attempt 2](https://github.com/NVIDIA/NemoClaw/actions/runs/34643362076/attempts/2) passed on the same commit. All-agent activation and both MCP discovery tests passed after rsliter initiated the rerun. Attempt 1 remains recorded: both MCP fixtures failed before onboarding when cloudflared exited; Hermes failed its 90-second readiness check. Retained logs do not establish those failures’ underlying causes. - One-off Linux AMD64 validation used OpenShell 0.0.116 and the authenticated `b7d908167` OpenClaw image. Native state diagnostics reported zero findings before installation and after normal stop/start. Git/XDG preferences and npm/Python user tools persisted in the same container and sandbox. Both native exec and NemoClaw connect sessions, all bare-tool and exit-code checks, and both authenticated gateway-backed agent turns passed. - Full credential-boundary proof remains incomplete. The first one-off stopped at coverage failure 97. A separate diagnostic follow-up preserved that failure and collected the independent persistence checks. Both credential scans read the primary config and more than 30,000 nonempty files under managed configuration roots, but matching process-environment reads returned EACCES. No upstream credential appeared in the inspected files, in-sandbox probe environment, Docker configuration or exported logs. Neither scan returned exposure code 98; both returned 97, keeping the overall local result failed. - Normal cleanup removed both test workloads, state volumes, gateway processes and synthetic responders. After verifying their absence, the two empty test networks and private host staging area were removed. The prior failed evidence and corrected, hash-verified structured evidence are retained. No product source, permanent E2E test, runtime permission or timeout was changed for these probes. - The original [Advisor run 34644796886](https://github.com/NVIDIA/NemoClaw/actions/runs/34644796886) failed during shell-profile access. [PR #11593](#11593) independently added the two read-only profile paths and removed an unsupported OpenShell exec flag on main. The authorized [rerun 34653195935](https://github.com/NVIDIA/NemoClaw/actions/runs/34653195935) used that controller (`33d57021`) to review unchanged `b7d908167`. Preparation passed; all nine specialists failed with an exec relay closure before their review command reported an exit status. The old profile-denial and unsupported-option errors were absent. No specialist review artifacts were collected; Advisor collection remains incomplete. - The diff contains no secrets, API keys or credentials. Credential injection and snapshot sanitization are unchanged. Earlier docs validation passed with 0 errors and 5 warnings; the final repair does not change the documentation patch. ## Review notes The Git finding is resolved. [Rsliter’s latest review](#11475 (review)) requested the gateway PATH restriction and a negative sentinel regression. Both are addressed in `b7d908167`, including the shared-shell route. [CodeRabbit accepted the repair](#11475 (comment)); all four review threads are resolved. Independent review found no blockers. Rsliter’s changes-requested decision remains pending reassessment. CodeRabbit’s pre-first-use executable concern describes an existing OpenShell trust limitation: the base preset already allows sandbox-writable npm and Linuxbrew executables plus Node. The new entries are exact alternate npm locations with unchanged destination/HTTP/credential permissions. The pinned OpenShell 0.0.116 runtime retains the same identity implementation and offers no expected-hash/pre-enrollment policy field. This PR does not claim first-use publisher verification or add an installation/identity registry. [CodeRabbit accepted the source-backed disposition](#11475 (comment)) and [withdrew the repeated finding on this commit](#11475 (comment)). The new paths add no capability beyond the existing writable install locations. ## Historical permission and lifecycle E2E qualification These results use the earlier 0.0.106 runtime and do not qualify the 0.0.116 image-user or session transition. [Run 34541663125](https://github.com/NVIDIA/NemoClaw/actions/runs/34541663125), attempt 1, tested `b3479a958788e97494d95b5f61b1dcb21cfee953` with controller `dab48f58c094d78c017458f4d72d6f5ca217578c` and recorded PR base `9e07bcfa2bd944c84582ed787a7c1d15a5d6e4b7`. The trusted controller authenticated the PR's immutable image catalog. **10 target/runtime executions passed; 3 failed.** All 13 execution identities and evidence artifacts were authenticated. There were 17 Vitest tests: 13 passed, 4 failed, and none skipped. - Passed on both Docker and Podman: security posture/native state diagnostics, hosted inference, and state backup/restore. - Passed on Docker: gateway survival, sandbox operations, snapshots, and the historical OpenClaw rebuild. - Both runtimes had zero native state-integrity findings on first run, after doctor repair, and after recovery: six clean reports. Broad doctor still reports 34 findings; every finding object matches the retained #11400 qualification, including its four security messages. This is not a claim of a warning-free general doctor. - Docker operations internally skipped its gateway-recovery action when the gateway was absent; other executed recovery coverage remains separately identified. | Failed Podman execution | Observed result and classification | | --- | --- | | [Sandbox operations](https://github.com/NVIDIA/NemoClaw/actions/runs/34541663125/job/103086043533) | Registry recovery omits the stored runtime driver, which defaults to Docker and invokes its unavailable CLI. A separate credential fixture also requires Docker. These paths are unchanged at candidate, PR base, and merge-base. The resulting cleanup provider mismatch is retained in the evidence. | | [Gateway survival](https://github.com/NVIDIA/NemoClaw/actions/runs/34541663125/job/103086043589) | Initial status fails because the in-sandbox models probe returns curl failure/BROKEN000, before restart. Raw transport diagnostics were not retained; the cause is unresolved. | | [Snapshot commands](https://github.com/NVIDIA/NemoClaw/actions/runs/34541663125/job/103086043765) | After successful destroy, fresh onboarding stops in host gateway ownership/version preflight before creating a new sandbox. The exact missing listener observation is unresolved. | A supplemental local probe ran the unchanged published image: native Git/XDG configuration, offline npm/Python installation, login-shell discovery, legacy-canary preservation, and same-container restart passed as the sandbox user. Root mode failed closed because QEMU changed PID 1 identity; this does not qualify native root startup. Both local test containers were removed and existing user resources/context were unchanged. These E2E results precede the gateway-only Git repair. No E2E job was retried. The complete matrix is not green; the two unresolved Podman failures still need attribution before a clean no-regression conclusion. Current-repair evidence is listed separately above. --- Signed-off-by: Aaron Erickson <aerickson@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Improved Claude Code compatibility for standard user installations and temporary npm-based installations. - Non-root sessions can use sandbox-local command-line tools consistently. - **Bug Fixes** - Git configuration is preserved correctly across gateway, connect, and one-shot command sessions. - Existing legacy temporary files and settings remain unchanged. - Refined npm and Python user-environment handling for more predictable tool behavior. - **Documentation** - Clarified Git configuration behavior and which non-secret preferences may be transferred between sessions. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Aaron Erickson <aerickson@nvidia.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Outcome
Use native Node.js network-interface discovery in the sandbox. Remove the obsolete ciao preload and its startup, connect-session, recovery, and packaging wiring.
Reason
Pinned OpenShell 0.0.106 permits the route queries used by native interface discovery. The old preload masks failures by returning an empty interface map and catching gateway exceptions.
Related issues
Closes #11260. Part of #11255.
Changes
ciao-network-guard.jsand its exclusive consumers. Keep the remaining preloads, managed recovery order, and broader safety-net behavior.The current diff changes 30 files: 122 additions, 327 deletions (205 fewer lines). Production code remains 11 additions and 147 deletions (136 fewer lines). No new runtime state, dependency, registry, permission rule, compatibility path, retry, timeout extension or live test target.
Verification
eb679d2; 61 existing focused fixture/reconnect tests passed for the correction ond6ce89e.8d6643bwas merged without conflicts as45d1b44. Canonicalnpm run validate:pr, both builds, Pi source parity, all 13 MCP fixture tests and all 42 legacy-fixture support tests passed on that merged commit.eb679d2images, passed legacy fixture creation, handoff and Docker restart. Actual legacy recovery then failed in the unchanged privileged Docker target selector because multiple labeled containers matched; late inference was not reached. The exact matching rows were not retained, so the backup/replacement explanation remains a source-supported inference. Both cleanup actions passed. The other passing cases are retained as ancestor evidence; their dependency graphs do not import the changed fixture.doctor --lint --jsonreported 34 security/optional-skill warnings per runtime. Native-state diagnostics reported zero findings; no warning-free broad-doctor result is claimed.45d1b44. Both live MCP passes and all-agent activation passed. The diff contains no secrets, API keys, or credentials.Review notes
Independent source and merge reviews found no actionable issue in this change. CodeRabbit completed its exact
45d1b44review with no actionable findings. Advisor run 34520676176 could not produce reviews: all nine specialists exhausted the provider budget (HTTP 429); no specialist artifacts or code findings were produced. The maintainer-requested admin merge was performed after CI passed, pinned to45d1b44, producing GitHub-verified main commit15c71dd.Main's #11460 fixes the inherited MCP unit regression and is now included. The formerly failing case passes locally; this PR introduces no MCP-specific repair.
OpenShell policy, capability restrictions, intentional discovery settings, managed Docker/Podman recovery authority, and upstream credential custody remain unchanged. The host-test prerequisite from #11309 landed independently on main before this branch consumed it to resolve conflicts. This PR targets main independently.
Signed-off-by: Aaron Erickson aerickson@nvidia.com
Summary by CodeRabbit
Bug Fixes
Tests
Chores