Skip to content

fix(sandbox): preserve ownership for existing read_write paths#827

Merged
johntmyers merged 1 commit intomainfrom
fix/783-prepare-filesystem-respect-existing-ownership/johntmyers
Apr 15, 2026
Merged

fix(sandbox): preserve ownership for existing read_write paths#827
johntmyers merged 1 commit intomainfrom
fix/783-prepare-filesystem-respect-existing-ownership/johntmyers

Conversation

@johntmyers
Copy link
Copy Markdown
Collaborator

@johntmyers johntmyers commented Apr 14, 2026

🏗️ build-from-issue-agent

Summary

Preserve image-defined ownership for existing sandbox read_write paths during filesystem preparation. The supervisor now only chowns paths it creates at startup, while keeping the existing symlink safety check and adding focused regression coverage.

Related Issue

Closes #783

Changes

  • crates/openshell-sandbox/src/lib.rs: extracted prepare_read_write_path(), kept the symlink guard, and limited chown() to newly-created read_write paths.
  • crates/openshell-sandbox/src/lib.rs tests: added Unix-focused coverage for creating missing paths, preserving existing paths, rejecting symlinks, and skipping ownership changes on pre-existing paths.
  • architecture/sandbox.md, architecture/security-policy.md: updated the architecture notes so they match the new ownership-preservation behavior.

Deviations from Plan

Updated architecture docs to keep the documented filesystem-preparation behavior aligned with the implementation.

Testing

  • mise run pre-commit passes
  • Unit tests added/updated
  • E2E tests added/updated (if applicable)

Tests added:

  • Unit: crates/openshell-sandbox/src/lib.rs tests covering missing-path creation, existing-path preservation, symlink rejection, and skipping chown() on existing paths.
  • Integration: N/A
  • E2E: N/A

Checklist

  • Follows Conventional Commits
  • Commits are signed off (DCO)
  • Architecture docs updated (if applicable)

Documentation updated:

  • architecture/sandbox.md: clarified that only newly-created read_write paths are re-owned.
  • architecture/security-policy.md: documented that existing image paths keep their ownership and symlinks are rejected.

Closes #783

Only chown read_write paths that the supervisor creates at startup, and leave pre-existing image paths with their original ownership. Add sandbox tests for creation, symlink rejection, and existing-path ownership preservation, and update architecture docs to match the new behavior.
@johntmyers johntmyers self-assigned this Apr 14, 2026
@johntmyers johntmyers requested a review from a team as a code owner April 14, 2026 05:19
@copy-pr-bot
Copy link
Copy Markdown

copy-pr-bot Bot commented Apr 14, 2026

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@johntmyers johntmyers merged commit e0db01e into main Apr 15, 2026
10 checks passed
@johntmyers johntmyers deleted the fix/783-prepare-filesystem-respect-existing-ownership/johntmyers branch April 15, 2026 04:51
ericksoa pushed a commit to NVIDIA/NemoClaw that referenced this pull request Apr 23, 2026
## Summary
Bumps the pinned OpenShell version range from `0.0.29` → `0.0.32` so
fresh NemoClaw installs pick up sandbox hardening and TLS improvements
from the last three OpenShell releases.

## Notable upstream changes

**0.0.30**
([NVIDIA/OpenShell@v0.0.29...v0.0.30](NVIDIA/OpenShell@v0.0.29...v0.0.30))
- Network policy deny rules
([OpenShell#822](NVIDIA/OpenShell#822))
- Preserve ownership on existing `read_write` paths
([OpenShell#827](NVIDIA/OpenShell#827))
- Disable child core dumps
([OpenShell#821](NVIDIA/OpenShell#821))
- Escape control characters in SSE error formatting
([OpenShell#842](NVIDIA/OpenShell#842))
- Fix silent truncation of large streaming inference responses
([OpenShell#834](NVIDIA/OpenShell#834))

**0.0.31**
([NVIDIA/OpenShell@v0.0.30...v0.0.31](NVIDIA/OpenShell@v0.0.30...v0.0.31))
- Inference routed-request header allowlist
([OpenShell#826](NVIDIA/OpenShell#826))

**0.0.32**
([NVIDIA/OpenShell@v0.0.31...v0.0.32](NVIDIA/OpenShell@v0.0.31...v0.0.32))
- **Load system CA certificates for upstream TLS connections**
([OpenShell#862](NVIDIA/OpenShell#862))
- Publish standalone `openshell-gateway` binaries
([OpenShell#853](NVIDIA/OpenShell#853))

## Changes
- `nemoclaw-blueprint/blueprint.yaml`: `min_openshell_version` and
`max_openshell_version` → `0.0.32`
- `scripts/install-openshell.sh`: `MIN_VERSION` and `MAX_VERSION` →
`0.0.32` (`PIN_VERSION` follows `MAX`)
- `scripts/brev-launchable-ci-cpu.sh`: default `OPENSHELL_VERSION` →
`v0.0.32`
- `src/lib/onboard.ts`: blueprint-fallback min version → `0.0.32`
- `test/onboard.test.ts`,
`test/install-openshell-version-check.test.ts`: fixtures updated; "above
MAX" test case moved from `0.0.30` to `0.0.33`

Historical `m-dev` comments referencing `0.0.29` left in place — they
describe a self-report quirk the sidecar fallback still handles.

## Why not 0.0.33+?
`0.0.34` introduced incremental sandbox policy updates and L7
request-target canonicalization — changes with larger surface area
against how NemoClaw delivers policy via gRPC. Worth a follow-up PR
rather than bundling here. `0.0.35` released hours before this PR was
cut — too fresh.

## Type of Change
- [x] Code change for a new feature, bug fix, or refactor.

## Testing
- [x] `npx vitest run test/install-openshell-version-check.test.ts` — 9
passed
- [x] pre-commit hooks (prek) clean: shellcheck, commitlint, gitleaks,
YAML validator, CLI test suite
- [ ] Nightly E2E on this branch — will be kicked off after PR opens

## Notes
- No user-facing CLI behavior changes — just the pinned version range.
- Two pre-existing failures in `test/onboard.test.ts` reproduce on clean
`main` and are unrelated to this bump.

Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>

🤖 Generated with [Claude Code](https://claude.com/claude-code)

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

* **Chores**
* Updated OpenShell version constraints and default pinned version to
v0.0.32 across configuration, install, and onboarding flows.

* **Tests**
* Updated test fixtures and expectations to match the new OpenShell
version (v0.0.32).
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

prepare_filesystem() should not chown existing directories in read_write

2 participants