fix(platform): skip an unidentifiable Docker socket instead of aborting detection - #10253
Merged
Merged
Conversation
…ng detection detectDockerHost returned null for the whole function the moment it found a reachable-but-unclassified socket, instead of trying the remaining candidates. On macOS, a stale Colima socket that answers but can't be classified broke Docker auto-detection entirely, even when a later candidate (e.g. Docker Desktop) would have resolved cleanly. Skip an unknown-identity candidate rather than aborting (#10248). Signed-off-by: Jason Ma <jama@nvidia.com>
Contributor
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall line coverage in commit 1012a71 in the TypeScript / code-coverage/cliThe overall line coverage in commit 1012a71 in the Updated |
Contributor
|
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)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 12 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthrough
ChangesDocker host detection
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The fix lets Docker auto-detection continue past a reachable but unidentifiable socket so a later valid engine can still be selected, with a regression test covering the scenario; no actionable merge-blocking risk remains at the current head. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation The PR satisfies issue ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
…on test The codebase-growth-guardrails check flags new if statements added to test files. Rewrite the mock as a ternary chain instead, keeping the same probe behavior (#10248). Signed-off-by: Jason Ma <jama@nvidia.com>
Contributor
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported 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: Apurv Kumaria <akumaria@nvidia.com>
apurvvkumaria
approved these changes
Aug 25, 2026
apurvvkumaria
left a comment
apurvvkumaria
left a comment
Collaborator
There was a problem hiding this comment.
Security review: PASS for the current revision.
- Secrets and sensitive data: PASS. No credential or diagnostic-content changes.
- Input validation and injection: PASS. Candidate socket paths still come from the existing trusted discovery list; no command construction changes.
- Authentication and authorization: PASS. No permission, identity, or daemon-access expansion.
- Dependencies and supply chain: PASS. No dependency, workflow, or artifact-source changes.
- Error handling and information disclosure: PASS. An unidentifiable reachable socket is skipped, while two conflicting identified engines still fail closed.
- Cryptography and data protection: PASS. No cryptographic or persisted-data changes.
- Configuration and deployment: PASS. Explicit Docker host and context precedence are unchanged.
- Security testing: PASS. All 35 platform integration tests, repository checks, CLI build/type checking, and normal hooks pass.
- System security: PASS. The change selects only a later candidate whose identity probe succeeds and does not weaken the conflicting-engine guard.
No documentation update is needed for this internal detection correction. The change adds 29 lines and removes one, below the large-change threshold. Cross-issue review found only issue #10248 and this PR, with no competing open implementation. Jason Ma remains the primary contributor; Apurv Kumaria’s merge commit only refreshes the branch.
Collaborator
|
Refreshed the branch from current main without conflicts. The signed merge commit is Verified. Post-refresh validation passes: 35 platform integration tests, repository checks, CLI build and type checking, and normal push hooks. Jason Ma remains the sole substantive contributor; the maintainer commit is branch synchronization only. |
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
detectDockerHostreturnednullfor the whole function the moment it found a reachable-but-unidentifiable socket in its candidate loop, instead of skipping to the next candidate. On macOS, a stale Colima socket that answers but can't be classified broke Docker auto-detection entirely, even when a later candidate (e.g. Docker Desktop) would have resolved cleanly.Related Issue
Fixes #10248
Changes
src/lib/platform.ts: changedreturn nulltocontinueon theobservation.identity === "unknown"branch insidedetectDockerHost's socket-candidate loop, so one ambiguous candidate is skipped rather than aborting detection of all remaining candidates — matching the reporter's suggested fix exactly.test/e2e-runtime/platform.test.ts: added a regression test for the exact repro — an earlier reachable-but-unidentifiable socket (stale Colima) must not prevent a later valid candidate (Docker Desktop) from being selected.Note: the reporter also flagged the next line's conflicting-identity check (
if (selectedIdentity && observation.identity !== selectedIdentity) return null) as "worth reviewing" for a similar concern. Left that unchanged — the issue only asks to review it, not fix it, and that check's abort-on-conflict behavior may be intentional (refusing to guess between two differently-identified reachable engines rather than picking one). Flagging it as a possible separate follow-up rather than bundling an unrequested behavior change into this fix.Type of Change
Quality Gates
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 unavailablenpx vitest run --project integration test/e2e-runtime/platform.test.ts— 35/35 passed locally and again on a clean host checkout (fresh clone +npm ci) on the auto-fix loop's ubuntu verify host.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: Jason Ma jama@nvidia.com
Summary by CodeRabbit
Bug Fixes
Tests