fix(onboard): preserve structured Docker tmpfs mounts#7137
Conversation
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@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. |
|
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)
📝 WalkthroughWalkthroughThe changes switch Docker tmpfs recreation to structured mounts, tighten tmpfs option validation, centralize exact-main MCP environment overlays, strengthen runtime probes, add namespace-aware nft inspection, and install pinned nftables packages across agent base images. ChangesSandbox runtime and exact-main validation
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage remains at 96%, unchanged from the branch. Updated |
PR Review Advisor — InformationalAdvisor assessment: Informational / high confidence Model lanes
Nemotron output stays in workflow artifacts and does not change the assessment above. E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
E2E Target Results — ❌ Some tests failedRun: 29626746071
|
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
E2E Target Results — ❌ Some tests failedRun: 29627004991
|
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
E2E Target Results — ❌ Some tests failedRun: 29627277222
|
E2E Target Results — ❌ Some tests failedRun: 29627515601
|
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
E2E Target Results — ❌ Some tests failedRun: 29627940692
|
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
E2E Target Results — ❌ Some tests failedRun: 29628183802
|
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
E2E Target Results — ❌ Some tests failedRun: 29628405201
|
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
E2E Target Results — ❌ Some tests failedRun: 29628609846
|
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
E2E Target Results — ❌ Some tests failedRun: 29628853604
|
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
E2E Target Results — ❌ Some tests failedRun: 29629079209
|
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
E2E Target Results — ❌ Some tests failedRun: 29629276579
|
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
E2E Target Results — ❌ Some tests failedRun: 29629789840
|
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
E2E Target Results — ❌ Some tests failedRun: 29630211337
|
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
E2E Target Results — ✅ All requested tests passedRun: 29630429324
|
jyaunches
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
test/e2e/support/openshell-exact-main-child-contracts.test.ts (1)
67-67: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftTest generated probes by executing their behavior.
These assertions lock tests to source fragments and can pass while the generated probe’s relevant branch is broken.
test/e2e/support/openshell-exact-main-child-contracts.test.ts#L67-L67: replace the substring assertion with fixture-driven execution for the applicable generated probes.test/e2e/support/openshell-exact-main-runtime-contracts.test.ts#L138-L158: execute the nft script against controlled command fixtures and assert namespace selection and emitted nft invocation.test/e2e/support/openshell-exact-main-runtime-contracts.test.ts#L177-L182: exercise the generated CONNECT script against controlled status responses and assert retry/status outcomes.As per path instructions, tests should prefer observable public-boundary outcomes and flag source-text assertions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/support/openshell-exact-main-child-contracts.test.ts` at line 67, Replace the source-text assertion at test/e2e/support/openshell-exact-main-child-contracts.test.ts:67-67 with fixture-driven execution of the applicable generated probe and assert its observable behavior. In test/e2e/support/openshell-exact-main-runtime-contracts.test.ts:138-158, execute the generated nft script with controlled command fixtures and verify namespace selection plus the emitted nft invocation; at :177-182, execute the generated CONNECT script with controlled status responses and verify retry and status outcomes. Prefer public-boundary behavior assertions at all three sites.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/e2e/live/openshell-exact-main-driver-config.ts`:
- Around line 348-359: Update the exact-main-driver-config test around the
inspectJson calls to retain and assert both HostConfig results instead of
discarding them. Verify HostConfig.Mounts contains the expected tmpfs target
with its required size and mode, and verify HostConfig.Tmpfs does not contain
that target, proving the legacy --tmpfs path is not used.
In `@test/e2e/live/openshell-exact-main-runtime-contracts.ts`:
- Around line 76-79: Preserve config_revision losslessly in the runtime contract
validation and status-report flow, using its exact decimal-string representation
rather than a JavaScript number; update
test/e2e/live/openshell-exact-main-runtime-contracts.ts at lines 76-79 to
validate and retain that representation, and update
test/e2e/support/openshell-exact-main-runtime-contracts.test.ts at lines 119-126
to assert exact round-trip equality with the emitted revision instead of only
integer validity.
In `@test/sandbox-provisioning.test.ts`:
- Around line 1035-1044: Update the parameterized test around the “installs
pinned nftables” case to extract the active apt-get install block from each
Dockerfile and assert that the nftables pin appears within that block. Avoid
asserting against the entire Dockerfile so comments or unrelated text cannot
satisfy the test.
---
Nitpick comments:
In `@test/e2e/support/openshell-exact-main-child-contracts.test.ts`:
- Line 67: Replace the source-text assertion at
test/e2e/support/openshell-exact-main-child-contracts.test.ts:67-67 with
fixture-driven execution of the applicable generated probe and assert its
observable behavior. In
test/e2e/support/openshell-exact-main-runtime-contracts.test.ts:138-158, execute
the generated nft script with controlled command fixtures and verify namespace
selection plus the emitted nft invocation; at :177-182, execute the generated
CONNECT script with controlled status responses and verify retry and status
outcomes. Prefer public-boundary behavior assertions at all three sites.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3998a0e5-9146-4bb5-8f33-f05aa7ea83f7
📒 Files selected for processing (15)
Dockerfile.baseagents/hermes/Dockerfile.baseagents/langchain-deepagents-code/Dockerfile.basesrc/lib/onboard/docker-gpu-patch-clone.test.tssrc/lib/onboard/docker-gpu-patch-clone.tssrc/lib/onboard/docker-gpu-patch-validation.test.tstest/e2e/live/mcp-bridge-onboard-env.tstest/e2e/live/mcp-bridge.test.tstest/e2e/live/openshell-exact-main-driver-config.tstest/e2e/live/openshell-exact-main-exec.tstest/e2e/live/openshell-exact-main-runtime-contracts.tstest/e2e/support/mcp-bridge-onboard-env.test.tstest/e2e/support/openshell-exact-main-child-contracts.test.tstest/e2e/support/openshell-exact-main-runtime-contracts.test.tstest/sandbox-provisioning.test.ts
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/e2e/support/openshell-exact-main-runtime-contracts.test.ts (1)
137-157: 🎯 Functional Correctness | 🔵 Trivial | 🏗️ Heavy liftExercise generated probes at their behavioral boundary.
These assertions only check source substrings and shell syntax, so dead or incorrectly wired branches can still pass. Add focused fixture-based execution for the nft inspection, bypass probe, and CONNECT transition scripts, asserting their observable outputs/statuses rather than exact implementation text.
Also applies to: 176-181
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/support/openshell-exact-main-runtime-contracts.test.ts` around lines 137 - 157, Replace the source-substring checks in the tests for buildExactMainNftInspectionScript, DIRECT_BYPASS_PROBE_CODE, and the CONNECT transition script with focused fixture-based executions. Assert each generated probe’s observable output and exit status across the supported nft topologies, bypass outcomes, and CONNECT transition cases, while retaining only syntax validation where appropriate.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/sandbox-provisioning.test.ts`:
- Around line 63-77: Update dockerRunCommandStartingWith to search Dockerfile
lines individually and select only a trimmed line that begins with startMarker,
rather than using indexOf on the full text. Start collecting the matched
instruction and its continuation lines from that line, preserving the existing
validation and error behavior.
---
Nitpick comments:
In `@test/e2e/support/openshell-exact-main-runtime-contracts.test.ts`:
- Around line 137-157: Replace the source-substring checks in the tests for
buildExactMainNftInspectionScript, DIRECT_BYPASS_PROBE_CODE, and the CONNECT
transition script with focused fixture-based executions. Assert each generated
probe’s observable output and exit status across the supported nft topologies,
bypass outcomes, and CONNECT transition cases, while retaining only syntax
validation where appropriate.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: aee22754-4668-4daa-a86c-c4a3ef5f4ce3
📒 Files selected for processing (4)
test/e2e/live/openshell-exact-main-driver-config.tstest/e2e/live/openshell-exact-main-runtime-contracts.tstest/e2e/support/openshell-exact-main-runtime-contracts.test.tstest/sandbox-provisioning.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- test/e2e/live/openshell-exact-main-driver-config.ts
3 participants
Summary
Deep Agents Code recreation converted OpenShell's structured tmpfs mount into Docker's legacy
--tmpfsrepresentation, so the recreated container no longer exposed the mount through.HostConfig.Mounts. Keep supported tmpfs mounts structured across recreation, fail closed on unsupported options, and ensure every sandbox base image contains thenftruntime OpenShell uses to enforce direct-bypass rejection.Changes
--mount type=tmpfsarguments while preserving target, size, mode, and the supportednoexec,nosuid, andnodevoptions.nftables=1.1.3-1in the OpenClaw, Hermes, and Deep Agents base images so OpenShell can install its bypass-enforcement rules in each agent topology./proc/<pid>/exeidentity across on-disk replacement./procfields, stale namespaces, sidecar namespaces, nft JSON family variants, immediate denial errno values, exact component paths during rebuild, and bounded transient proxy reconnects.Type of Change
Quality Gates
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailable30/30mount clone/validation/recreation tests;61/61sandbox provisioning tests;37/37focused MCP/exact-main E2E-support tests;npm run typecheck:cli;bash scripts/check-production-build-args.shnpm run docsbuilds without warnings (doc changes only)Live E2E evidence: MCP bridge run 29630429324 passed for Deep Agents, Hermes, and OpenClaw.
Signed-off-by: Julie Yaunches jyaunches@nvidia.com
Summary by CodeRabbit
New Features
nftablestooling to supported sandbox base images.--mountsyntax.Bug Fixes
Tests