fix(cli): emit docker_unreachable header upfront in sandbox status#4388
Open
laitingsheng wants to merge 2 commits into
Open
fix(cli): emit docker_unreachable header upfront in sandbox status#4388laitingsheng wants to merge 2 commits into
laitingsheng wants to merge 2 commits into
Conversation
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
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 (4)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughImplements upfront Docker daemon reachability detection in ChangesDocker Unreachable Detection
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Comment |
Contributor
E2E Advisor RecommendationRequired E2E: Dispatch hint: Auto-dispatched E2E: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Contributor
E2E Scenario Advisor RecommendationRequired scenario E2E: None Full scenario advisor summaryE2E Scenario AdvisorBase: Required scenario E2E
Optional scenario E2E
Relevant changed files
|
Contributor
PR Review AdvisorFindings: 1 needs attention, 0 worth checking, 0 nice ideas Review findings🛠️ Needs attention
🔎 Worth checking
🌱 Nice ideas
Since last review detailsCurrent findings:
This is an automated advisory review. A human maintainer must make the final merge decision. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/cli.test.ts (1)
1003-1052: ⚡ Quick winStrengthen the non-docker-driver assertion coverage.
This test currently proves only header absence, not that inference probing remains enabled. Please also assert normal (non-error) status behavior for this path.
Suggested diff
const r = runWithEnv("alpha status", { HOME: home, PATH: `${localBin}:${process.env.PATH || ""}`, }); + expect(r.code).toBe(0); + expect(r.out).toContain("Inference:"); expect(r.out).not.toContain( "Failure layer: docker_unreachable", );🤖 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/cli.test.ts` around lines 1003 - 1052, The test "sandbox <name> status preserves Inference probe when openshellDriver is not docker" currently only asserts the absence of "Failure layer: docker_unreachable"; update the test (around runWithEnv("alpha status") / writeSandboxRegistry) to also assert that inference probing and normal status output are present by checking r.out contains the inference probe lines (e.g., "Gateway inference:" and "Provider: openai-api" or "Model: gpt-4o-mini") and that normal gateway status appears (e.g., "Status: Connected"), using the same runWithEnv result and existing expect APIs so you verify probing remains enabled in non-docker openshellDriver cases.
🤖 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.
Nitpick comments:
In `@test/cli.test.ts`:
- Around line 1003-1052: The test "sandbox <name> status preserves Inference
probe when openshellDriver is not docker" currently only asserts the absence of
"Failure layer: docker_unreachable"; update the test (around runWithEnv("alpha
status") / writeSandboxRegistry) to also assert that inference probing and
normal status output are present by checking r.out contains the inference probe
lines (e.g., "Gateway inference:" and "Provider: openai-api" or "Model:
gpt-4o-mini") and that normal gateway status appears (e.g., "Status:
Connected"), using the same runWithEnv result and existing expect APIs so you
verify probing remains enabled in non-docker openshellDriver cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8b9c4ead-b564-4518-bbce-7e09c2c07300
📒 Files selected for processing (3)
src/lib/actions/sandbox/status.test.tssrc/lib/actions/sandbox/status.tstest/cli.test.ts
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 26551292928
|
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
|
🌿 Preview your docs: https://nvidia-preview-pr-4388.docs.buildwithfern.com/nemoclaw |
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 26552024714
|
laitingsheng
added
Sandbox
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
1 participant
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 <name> statusnow probes the host Docker daemon upfront on docker-driver sandboxes and printsFailure layer: docker_unreachable — Docker daemon is not reachable.as the first line of stdout when the daemon is stopped. The misleading host-sideInference: healthyline (which hits the remote provider directly and bypasses the local stack) is suppressed in that case, and the command exits with code1. The same layer header is no longer emitted twice when the downstream gateway-state branches also classify the failure asdocker_unreachable.Related Issue
Fixes #4313.
Changes
src/lib/actions/sandbox/gateway-failure-classifier.tsexports a newisDockerDaemonReachable()helper that wraps the existingdockerInfo({ ignoreError, timeout: 3000 })probe.src/lib/actions/sandbox/status.tsaddsisDockerDaemonUnreachableForStatus(sb, probe?)(gated onsb.openshellDriver === "docker") which delegates to the shared helper.showSandboxStatusevaluates it after the sandbox-registry lookup, prints the layer header verbatim as the first stdout line, setsprocess.exitCode = 1, and suppresses the host-side inference probe.printGatewayFailureLayerHeadertakes analreadyPrintedDockerUnreachableflag so downstream gateway-state failure branches do not duplicate the header when the classifier also returnsdocker_unreachable.src/lib/actions/sandbox/status.test.tscovers the new helper with null sandbox, non-docker driver, docker driver with reachable probe, and docker driver with unreachable probe.test/cli.test.tsadds two integration tests: one drivesalpha statuswith a docker stub that exits 1 and asserts exit code1, that stdout starts with the verbatim layer header, thatInference: healthyis absent, that the header appears exactly once, and that the header precedes theSandbox: alphaline; the other registers the sandbox withopenshellDriver: "vm"and asserts exit code0, presence of theProvider/Model/Inferencelines, and absence of the docker header.docs/reference/commands.mdxdocuments the docker-driver stopped-daemon behaviour, including the verbatim layer header, exit-code contract, and the inference-probe suppression.Type of Change
Verification
npx prek run --all-filespassesnpm testpassesnpm run docsbuilds without warnings (doc changes only)Signed-off-by: Tinson Lai tinsonl@nvidia.com
Summary by CodeRabbit
Bug Fixes
Documentation
Tests