fix(cli): preserve healthy Docker authority - #8823
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
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughDocker host detection now probes the CLI default authority and validates Docker or Podman identity before selecting socket fallbacks. Runner environments preserve or clear Docker context settings by command type. Tests cover mixed runtimes, explicit hosts, unknown identities, readiness, profile compatibility, and probe filtering. ChangesDocker authority detection
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant NemoClaw
participant DockerCLI
participant SocketCandidates
participant Readiness
NemoClaw->>DockerCLI: Probe default docker version authority
DockerCLI-->>NemoClaw: Return reachability and server identity
NemoClaw->>SocketCandidates: Probe fallback sockets when required
SocketCandidates-->>NemoClaw: Return validated endpoint identity
NemoClaw->>Readiness: Project selected runtime
Readiness-->>NemoClaw: Resolve Docker-only profile compatibility
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Contributor
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 982a17d in the TypeScript / code-coverage/cliThe overall coverage in commit 982a17d in the Show a code coverage summary of the most impacted files.
Updated |
Contributor
|
🌿 Preview your docs: https://nvidia-preview-pr-8823.docs.buildwithfern.com/nemoclaw |
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Contributor
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests. 3 semantic terminology decisionsTerminology 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: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
wscurran
added
area: cli
senthilr-nv
approved these changes
Aug 12, 2026
24 tasks
prekshivyas
added a commit
that referenced
this pull request
Sep 9, 2026
…10379) ## Summary NemoClaw probes Docker reachability by running `docker version` in a four-name environment (`HOME`, `USER`, `LOGNAME`, `PATH`), but every Docker command it runs afterwards gets the full subprocess allowlist. On a host whose daemon answers only through one of the dropped names, the probe reports the host's own default authority unreachable, detection falls through to the socket candidates, and the CLI pins `DOCKER_HOST` to Podman's rootless socket. Preflight then reports `Docker is not reachable` and points at the docker group, so onboarding stops at its first step on a host whose Docker is healthy. After this change the probe asks the same question the later commands answer, and detection never redirects the CLI on no evidence. ## Related Issue Closes #10367 This removes the mechanisms that produce the reported outcome: a probe environment narrower than the one the predicted commands run in, and a probe that reaches no verdict counting as a refusal. Either can send `DOCKER_HOST` to Podman's socket on a host whose Docker daemon is live. One honest caveat for whoever merges this. The reporter runs a DGX Spark with Docker and Podman installed; I have no such host and they have not yet answered the two diagnostic commands I asked for on the issue, so the cure is reasoned from the code path, not observed on their machine. If their `docker version` under the old four-name environment turns out to exit `0` quickly, neither fix explains their failure and the issue should be reopened rather than left closed. Two details from the report stay out of scope either way: the docker-group remediation text that names the wrong cause, and the `docker info` versus `docker version` disagreement on an unhealthy daemon. ## Changes - `buildDockerProbeEnv` now selects names with `isSubprocessEnvNameAllowed`, the same allowlist `buildSubprocessEnv` gives real Docker commands, and drops an ambient `DOCKER_HOST` so the probe still pins the authority under test. The probe predicts whether those commands reach a daemon, so it must not ask under a narrower environment: `SSH_AUTH_SOCK` authenticates an `ssh://` Docker context and the proxy names decide how a `tcp://` one is routed. (An earlier revision of this description claimed `XDG_RUNTIME_DIR` selects a rootless daemon socket for the Docker CLI. I tested that and it is false — the CLI ignores a listening `docker.sock` in the runtime directory — so the justification is corrected here and in the code comment.) - `probeDockerHost` reports `inconclusive` when the Docker CLI cannot be spawned or the 3-second probe timeout kills it, and `detectDockerHost` holds the host default in that case. A probe that never answered is not an observed refusal, so it must not move the whole CLI to a fallback socket. - Linux socket candidates are now ordered `/run/docker.sock`, `/var/run/docker.sock`, `/run/user/<uid>/docker.sock`, then Podman's. Rootless Docker's socket sits beside Podman's in the same runtime directory and was never a candidate. - `buildDockerProbeEnv` also applies `withLocalNoProxy`, which `buildSubprocessEnv` gives every real Docker command. Without it, forwarding the proxy names could route a probe of a local `tcp://` authority through a host proxy that the real commands bypass — the same defect class, reintroduced by the fix. - `ci/source-architecture-budget.json`: reading the shared allowlist raises the recorded fan-in of `src/lib/subprocess-env.ts` from 24 to 25. - Onboarding now bounds the existing `docker info` and `docker version` preflight calls at 15 seconds, so preserving an inconclusive default authority cannot leave onboarding waiting without a limit. ## Risk family `src/lib/platform.ts` puts this PR in the tier-3 `platform-install` family, whose required job is `cloud-onboard`. That workflow has no `pull_request` trigger, so it selects on the post-merge push to `main` rather than here. Say the word if you want a `cloud-onboard` run before merge and I will arrange it. ## Not in this PR CodeRabbit's merge-risk note and the PR Review Advisor both point at the mixed-identity bail: when the default authority is dead and both a Docker socket and a Podman socket answer, `detectDockerHost` returns `null` and the CLI keeps its dead default. That path is pre-existing and unchanged here, and removing it reverses a decision recorded in #8823 and #10253, whose security review cited it as a pass criterion. It is a maintainer call, so it is a separate stacked PR — #10387 — with the reversal argued. This PR leaves the guard exactly as it was. ## Type of Change - [x] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [ ] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: pending review on this PR - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: The probe environment stays an allowlist. `test/e2e-runtime/platform.test.ts` fails the probe binary when `NVIDIA_INFERENCE_API_KEY` crosses the boundary, in the new test and in the existing `#8816` one. ## Verification - [x] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run validate:pr` passed after refreshing `origin/main` when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — command/result: `npx vitest run test/e2e-runtime/platform.test.ts` gives 38 passed, and a focused sweep over the Docker-authority files (`platform`, `runner`, `preflight-docker-host`, `domain/docker-host`, `subprocess-env`, `readiness/host`, `container-engine`, `docker-authority-profile`) gives 187 passed. `npm run typecheck:cli` and `npm run lint` pass. The focused platform and Docker-preflight timeout suites cover 2 files and 40 tests, and the codebase growth guardrails cover 33 tests. All three original probe changes were confirmed red first: without the probe-environment change the default-authority test returns `unix:///run/user/1000/podman/podman.sock` where `null` is expected; without `withLocalNoProxy` that same test fails on the proxy-exclusion guard; and without the no-verdict branch, the test whose Docker CLI dies without an exit status selects the Podman socket. - [ ] Applicable broad gate passed — command/result: not run. The change set is two source functions and their tests. - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) - [ ] Doc pages follow the style guide (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) --- Signed-off-by: Dongni Yang <dongniy@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved Docker environment detection across Linux setups, including rootless Docker and Podman installations. * Prioritized native Docker sockets for more accurate runtime detection. * Prevented incorrect Docker or Podman classification when the Docker CLI is unavailable or unresponsive. * Preserved relevant runtime and proxy settings while excluding ambient configuration that could cause misleading results. * **Tests** * Expanded coverage for socket prioritization, environment handling, proxy behavior, and inconclusive Docker probes. <!-- end of auto-generated comment: release notes by coderabbit.ai --> ## Merge with `main` (`7d1476a7a1`, 2026-09-09) `reviewed-npm-audit` failed on `b20e7fcd22` with `Tencent WeChat plugin 2.4.3 locked runtime graph lock SHA-256 mismatch`: the branch carried the pre-#11023/#11253 expected hash in `ci/reviewed-npm-audit.json` while the trusted action computes the current one. `main` already records the current hash, so this is a clean merge of `main` (104 commits, no conflicts) with no change to the fix itself. It also picks up the patched `js-yaml` pin from #11264. --------- Signed-off-by: Dongni Yang <dongniy@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com> Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Co-authored-by: Apurv Kumaria <akumaria@nvidia.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Prekshi Vyas <prekshiv@nvidia.com> Co-authored-by: Prekshi Vyas <34834085+prekshivyas@users.noreply.github.com>
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
3 participants
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.
Summary
NemoClaw now preserves a reachable Docker CLI default, including a custom
DOCKER_CONFIG, whenDOCKER_HOSTis unset instead of replacing it with the first existing socket.If the default is unreachable, the CLI probes only its bounded local-socket candidates and selects a fallback only when server-version evidence identifies one runtime.
Related Issue
Fixes #8816
Changes
DOCKER_HOSTwithout probing or replacing its authority.DOCKER_CONTEXTand itsDOCKER_CONFIGdirectory for Docker commands, and clear the context only when a verified socket fallback must take precedence.Type of Change
Quality Gates
DOCKER_CONTEXTand itsDOCKER_CONFIGpath; fallback probes and host overrides omit both; selecting a verified fallback clears the conflicting context; and non-Docker commands receive neither value. Regression coverage confirmsNVIDIA_INFERENCE_API_KEYremains filtered at both boundaries. The maintainer explicitly approved this Docker-only configuration boundary.Documentation Writer Review
docs-updateddocs/reference/system-readiness.mdxdocuments Docker default-authority selection withDOCKER_CONTEXTandDOCKER_CONFIG, verified local socket fallback, and conflicting context removal.DGX Station Hardware Evidence
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 unavailableupstream/main.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result: Not required for this focused CLI change.npm run docsbuilds without warnings (doc changes only) — result: the build passed; Fern reported two baseline warnings that it suppresses unless run with--warnings.Signed-off-by: Prekshi Vyas prekshiv@nvidia.com
Summary by CodeRabbit
Enhancements
Documentation