fix(host-mount): report the real reason a host directory is unusable - #9787
Conversation
parseReadOnlyHostMount reported "host directory does not exist" for any lstat/stat failure, so a directory that exists but sits under an unreadable ancestor was reported as missing. Forward the underlying error the way normalizePersistedSandboxHostMounts and verifyReadOnlyHostMountSources already do in this file. Signed-off-by: Udaya Tejas <udayatejas2004@gmail.com>
|
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 (2)
Included review availability: Your plan provides up to 12 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughHost-mount validation now preserves underlying ChangesHost-mount validation
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The change improves host-directory validation errors while preserving missing-directory behavior, and no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
PR Review Advisor — Blocking findings reportedAdvisor assessment: Blockers require maintainer review Model lanes
2 additional E2E selections from the second opinionAdvisory only. The primary lane did not select these E2E jobs or targets.
Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests. 1 semantic terminology decisionTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. A maintainer can dispatch the default E2E suite for the commit under review. Recommended E2E: None Manual-only E2E: Blockers
|
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
Reviewed commit 34aaa4878e606ca2e3834bdf4c8cfb0799805d42. The two source-stat failure paths now preserve the underlying filesystem error as cause, distinguishing a missing path from an unusable path without changing validation or authorization behavior. Focused host-mount validation passed 17 tests after building generated CLI artifacts. Both advisor lanes reported no findings. The initial shard 2 failure occurred after tests during artifact handling; its bounded failed-job rerun is in progress and no failed check remains.
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
Re-reviewed commit 2dccd03f2ea42c9c5774c11ce2c598cd10ca0718 after GitHub updated the branch from current main. The host-mount validation still preserves the underlying filesystem failure as the error cause without changing the user-facing classification or mount policy. After building generated CLI artifacts, the focused host-mount suite passed 12 tests.
## Summary Allows explicitly selected, registered native runtime providers to own host readiness without being blocked by Docker-only availability, reachability, runtime, or storage requirements. This restores the documented Docker-less native Podman installer path while keeping default Docker, platform, GPU, gateway, and unrelated blockers fail-closed. ## Related Issue Fixes #10891 ## Changes - Add an explicit, registry-validated provider-owned host-readiness boundary and use it in both CLI onboarding and installer preflight. - Keep default Docker and Docker-backed managed vLLM fail-closed; Docker-less native Podman still runs its own doctor, GPU, bridge, and DNS checks. - Prove the public installer path with Docker removed on the Podman E2E lane, post-install Docker absence, retained evidence, and fail-safe restoration ordering. - Align prerequisites, quickstarts, platform support, inference, security, and troubleshooting guidance with the implemented boundary. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [x] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Verification - [ ] `npx prek run --all-files` passes - [ ] `npm test` passes without unrelated infrastructure failures - [x] Tests added or updated for new or changed behavior - [x] No secrets, API keys, or credentials committed - [x] Docs updated for user-facing behavior changes - [x] Documentation validation passes with zero errors - [x] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) - [ ] New doc pages include SPDX header and frontmatter (new pages only) Exact-head evidence for `68893d8bfc36e2e30c2b9a18449d43e5ec26c3cd`: - [PR Review Advisor](https://github.com/NVIDIA/NemoClaw/actions/runs/33757032152): all nine specialists succeeded; every artifact was inspected and reports no remaining finding. - CodeRabbit reviewed the exact head, confirmed both requested fixes, accepted Podman `ownsHostReadiness: true`, and has zero unresolved threads. - [Rootless Podman CPU Qualification](https://github.com/NVIDIA/NemoClaw/actions/runs/33757034936): success with Docker disabled. - [Managed Images](https://github.com/NVIDIA/NemoClaw/actions/runs/33757034962): all ten jobs succeeded, including exact all-agent activation and both OpenClaw MCP discovery passes. - [Focused E2E](https://github.com/NVIDIA/NemoClaw/actions/runs/33760249117): `cloud-onboard / podman / public-nvidia` succeeded. The retained receipt records `dockerClientAvailable: false` at `/usr/bin/docker`; the post-install `command -v docker` probe exited 1; installer, sandbox, inference/security checks, restoration, and cleanup passed. - Code scanning, installer hashes, DCO, docs, CLI/installer parity, governance, local focused suites, mock/live parity, growth guardrails, CLI typecheck, repository checks, Fern validation, lint, and commit/push hooks passed. Known unrelated CI state: - The full Linux package-contract runner passed 1,238 tests but npm 10.9.8 crashed in `managed-image-registry-transport.test.ts` with upstream npm/cli issue [#9787](npm/cli#9787). The exact test passes standalone and the same npm crash is present on unrelated PRs; three exact-head attempts reproduced the npm defect. - Self-hosted image qualification has two pre-existing migration failures that reproduce on the prior head and are owned by separate PR #10941. This PR does not change those image/test surfaces. ## AI Disclosure - [x] AI-assisted — tool: Codex --- Signed-off-by: Aaron Erickson <aerickson@nvidia.com> --------- Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
4 participants
Summary
Both source-stat failure paths in
--host-mountvalidation used a barecatch {}and reportedhost directory does not exist, so a directory under an unreadable ancestor was reported as missing. They now forward the underlying error.Related Issue
Fixes #9786
Changes
failHostMounttakes an optionalcause; the two failure paths now reporthost directory is not usable: <path>plus the underlying error message, with that error attached ascause.normalizePersistedSandboxHostMountsandverifyReadOnlyHostMountSourcesalready use in this file. No new import, no new pattern.no such file or directory, so the two existing assertions moved to that text.Type of Change
Quality Gates
The existing
rejects missing, unreadable, relative, symlinked, non-normalized, and terminal-control pathscase gains a path whose ancestor is a regular file and assertscause.code === "ENOTDIR". Onmainthat cause isundefined.Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailablenpx vitest run --project cli src/lib/onboard/host-mount/passes; a sweep of all 17 test files that reference host mounts passes, 555 tests, no failures.Signed-off-by: Udaya Tejas udayatejas2004@gmail.com
Summary by CodeRabbit