Repository navigation
fix(advisor): restore stable OpenShell exec - #11593
Merged
Merged
Conversation
|
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 (3)
💤 Files with no reviewable changes (1)
Included review availability: Your plan provides up to 12 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughThe OpenShell policy adds read-only shell profile paths. Sandbox execution removes the ChangesOpenShell policy and execution
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to The narrowly scoped OpenShell compatibility changes have focused validation and no unresolved merge-blocking risk. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
cjagwani
marked this pull request as ready for review
September 11, 2026 21:44
cjagwani
marked this pull request as draft
September 11, 2026 21:46
cjagwani
marked this pull request as ready for review
September 11, 2026 21:46
prekshivyas
approved these changes
Sep 11, 2026
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>
wscurran
added
area: ci
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
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
PR Review Advisor specialists can start on the repository-pinned OpenShell v0.0.116 release again. Stable OpenShell may read the pinned image's login files, while the Advisor sandbox keeps them read-only and retains its existing write boundary.
Reason
The runtime passed
--no-login-shell, an option available on unreleased OpenShell main but absent from pinned v0.0.116. Stable OpenShell treated it as the sandbox command and failed every specialist with/bin/bash: --: invalid optionbefore review began.Related issues
Part of #10791
Changes
--no-login-shellargument from sandbox execution./sandbox/.profileand/sandbox/.bashrcas additional read-only policy paths required by stable OpenShell login execution.Verification
npx vitest run --project integration test/automation/pull-requests/pr-review-advisor-openshell.test.ts test/generation/post-merge-docs.test.ts— 108/108 passed on current main.npx prek run --hook-stage pre-commit --all-files— passed.NODE_OPTIONS=--max-old-space-size=8192 npm run validate:pr— passed, including CLI typecheck.Review notes
This touches the Advisor sandbox policy. The two added paths are pinned-image login files and remain read-only; no broader
/sandboxread or write permission is granted.Signed-off-by: Charan Jagwani cjagwani@nvidia.com
Summary by CodeRabbit
Improvements
.bashrcand.profileis now read-only, helping protect shell configuration while preserving access to these files.Tests