fix(onboard): preserve structured mounts during recreation#7135
Conversation
|
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 (5)
📝 WalkthroughWalkthroughStructured Docker mount inspection types, fixture data, clone argument reconstruction, and validation tests were added. Supported tmpfs and volume mounts are preserved during GPU container cloning, while unsupported mount types are rejected before modifying the original container. ChangesDocker structured mount preservation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant DockerInspect
participant MountSerializer
participant CloneArgsBuilder
DockerInspect->>MountSerializer: provide HostConfig.Mounts
MountSerializer->>MountSerializer: validate structured mount options
MountSerializer->>CloneArgsBuilder: return --tmpfs and --mount arguments
CloneArgsBuilder->>CloneArgsBuilder: append mount arguments to clone command
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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. TypeScript / code-coverage/cliThe overall coverage in the branch remains at 80%, unchanged from the branch. Show a code coverage summary of the most impacted files.
Updated |
PR Review Advisor — Blocking findings reportedAdvisor assessment: Blockers require maintainer review 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: 1 optional E2E recommendation
Blockers
|
|
Reviewed PRA-1 against the actual Docker Engine inspect contract and pinned OpenShell driver. No code change is needed: Docker container InspectResponse includes HostConfig, and HostConfig includes the original structured Mounts array. The top-level Mounts array is a separate runtime MountPoint summary; it uses Destination but drops the structured TmpfsOptions and VolumeOptions needed for exact recreation. OpenShell creates these mounts through HostConfig.mounts, and parseDockerInspectJson retains that raw field. The existing fixture intentionally models that create-time inspect field, while the live E2E uses top-level .Mounts only to verify the resulting runtime mount. Switching reconstruction to top-level Mounts would lose noexec, tmpfs size/mode, and volume options. |
Summary
Deep Agents Code startup-command recreation preserved legacy Docker binds but silently dropped OpenShell structured mounts, causing the exact-main MCP bridge proof to lose its tmpfs immediately after onboarding. Preserve the structured mount contract across recreation and reject unsupported inspect shapes before mutating the original container.
Changes
noexec, 16 MiB, mode1777tmpfs that failed in live E2E, and prove unsupported mounts are rejected before stop, rename, or clone.HostConfig.Mounts, while its source tests modeled onlyHostConfig.Binds; the corrected fixture now protects that boundary.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 unavailablenpx vitest run --project cli src/lib/onboard/docker-gpu-patch-clone.test.ts src/lib/onboard/docker-gpu-patch-validation.test.ts(27 passed);npx vitest run --project cli src/lib/onboard/docker-gpu-patch-recreate.test.ts(2 passed);npm run typecheck:clipassed.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:npm run docsbuilds without warnings (doc changes only)Signed-off-by: Julie Yaunches jyaunches@nvidia.com
Summary by CodeRabbit