Walkthrough

A new gateway-failure classifier identifies why a sandbox container's Docker gateway is unreachable by probing daemon reachability, container state, and port availability. The classifier is integrated into the status command fallback path and verified by unit and integration tests.

Changes

Gateway failure classifier

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related issues

• feat(status): classify failing layer when gateway probe fails (#2666 follow-up) #3271: PR directly implements the proposed gateway-failure classifier to diagnose and report specific failure layers when sandbox status verification fails.

Suggested labels

Sandbox, Docker

Suggested reviewers

• cv
• ericksoa

Poem

🐰 I hop through ports and Docker sighs,
I check the daemon, peer the skies.
If containers sleep or ports are tied,
I name the layer, show what's inside.
Small rabbit, big debug — here’s your guide.

❌ Failed checks (1 warning)

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

E2E Recommendation Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required E2E

• sandbox-operations-e2e (high): Best existing E2E coverage for this change. It exercises nemoclaw list, nemoclaw <sandbox> status, status fields, process recovery triggered by status, and gateway container kill recovery against a real Docker/OpenShell sandbox runtime.

Optional E2E

• sandbox-survival-e2e (high): Useful adjacent confidence for status and gateway restart behavior. It validates that nemoclaw <sandbox> status remains usable across gateway stop/start and that the sandbox survives restart, but it does not directly assert the new failure-layer headers.

New E2E recommendations

• gateway_failure_diagnostics (high): Existing E2E jobs cover healthy status and some recovery paths, but no existing E2E appears to assert the new user-visible Failure layer: ... headers for Docker unreachable, container missing, exited container, or exited-container-with-foreign-port cases in the real CLI.
  • Suggested test: Add a targeted negative gateway-status E2E that creates a registered sandbox, manipulates/stubs the gateway runtime into docker_unreachable, container_missing, container_exited, and container_exited_port_conflict states, then asserts nemoclaw <sandbox> status exits non-zero and prints the expected failure-layer header before recovery guidance.

Dispatch hint

• Workflow: nightly-e2e.yaml
• jobs input: sandbox-operations-e2e

PR Review Advisor

Base: origin/main
Head: HEAD
Analyzed SHA: eaf10d34576afaf1cdc1f477de390866e23352ef
Recommendation: blocked
Confidence: high

Implementation is narrow and well-tested at unit/subprocess level, but mergeStateStatus is BLOCKED and required sandbox E2E pass evidence is missing for the current head SHA.

Gate status

• CI: pass — Trusted context reports 5 required status contexts completed with no failures for eaf10d3: checks, commit-lint, dco-check, check-hash, changes. GraphQL also shows E2E recommendation completed successfully; non-required build-sandbox-images and build-sandbox-images-arm64 remain in progress.
• Mergeability: fail — GitHub metadata reports mergeStateStatus=BLOCKED and GraphQL reviewDecision=REVIEW_REQUIRED for PR feat(status): classify failing layer when gateway probe fails (#3271) #4020 at headRefOid eaf10d3.
• Review threads: pass — GraphQL reviewThreads.nodes is empty and reviewComments is empty; CodeRabbit reported no actionable comments in the latest review. Human review is still required by branch protection/reviewDecision.
• Risky code tested: fail — Changed files are sandbox gateway/status runtime paths. Unit and subprocess tests were added, but the E2E Advisor required sandbox-operations-e2e for eaf10d3; the only selective E2E pass comment provided targets old SHA 37e52fc, not the current head.

🔴 Blockers

• PR is currently blocked by GitHub merge state: The PR does not satisfy GitHub mergeability gates even though required status checks are passing. The trusted metadata reports mergeStateStatus=BLOCKED and reviewDecision=REVIEW_REQUIRED for the current head SHA.
  • Recommendation: Resolve the GitHub merge block, including required human review or other branch-protection requirements, then re-check gates on eaf10d3 before merge.
  • Evidence: GraphQL: headRefOid=eaf10d34576afaf1cdc1f477de390866e23352ef, mergeStateStatus=BLOCKED, reviewDecision=REVIEW_REQUIRED.
• Required sandbox E2E pass is missing for the current head SHA (src/lib/actions/sandbox/status.ts:214): This PR changes sandbox status behavior and gateway failure diagnostics. The E2E Advisor requires sandbox-operations-e2e and auto-dispatched it for eaf10d3, but the provided passing selective E2E result is for older SHA 37e52fc. Unit tests and fake-docker subprocess tests are useful but do not fully prove real Docker/OpenShell behavior for this head.
  • Recommendation: Confirm sandbox-operations-e2e passes for eaf10d3 before merging. If the auto-dispatched nightly run has completed, attach or verify its result against this exact SHA.
  • Evidence: E2E Advisor comment: Required E2E: sandbox-operations-e2e; Auto-dispatched at eaf10d3. Selective E2E Results comment reports sandbox-operations-e2e success only for target ref 37e52fc.

🟡 Warnings

• Linked proposal mentions gateway-state hook, but integration is status-only (src/lib/actions/sandbox/status.ts:214): The linked issue proposal asks for a classifier called from src/lib/actions/sandbox/status.ts and src/lib/actions/sandbox/gateway-state.ts:printGatewayLifecycleHint. This PR calls the classifier from status.ts before invoking printGatewayLifecycleHint in multiple fallback branches, but it does not modify gateway-state.ts itself. That may be acceptable if status is the only intended caller, but it is not a literal full implementation of that proposal clause.
  • Recommendation: Confirm that status-level integration before printGatewayLifecycleHint is sufficient, or move/shared-hook the layer-header emission into gateway-state.ts if all printGatewayLifecycleHint callers should receive the header.
  • Evidence: Changed files do not include src/lib/actions/sandbox/gateway-state.ts. Diff adds printGatewayFailureLayerHeader in src/lib/actions/sandbox/status.ts and calls it before printGatewayLifecycleHint/gateway guidance branches.
• Subprocess coverage covers the required conflict scenario but not every layer (test/repro-2666-silent-list-status.test.ts:247): The issue implementation notes say subprocess testing per layer. The PR adds subprocess assertions for gateway_unreachable, container_missing, and container_exited_port_conflict-related status paths, while docker_unreachable and container_exited are covered by unit tests only. This is probably reasonable given the Definition of Done specifically names the container-stopped plus foreign-port-holder scenario, but it is only partial against the broader implementation-notes wording.
  • Recommendation: Either consciously accept unit-only coverage for the remaining layers, or add lightweight subprocess cases for docker_unreachable and container_exited if maintainers want literal per-layer subprocess coverage.
  • Evidence: test/gateway-failure-classifier.test.ts covers every layer in isolation. test/repro-2666-silent-list-status.test.ts adds subprocess cases for gateway_unreachable, container_missing, and container_exited_port_conflict; no subprocess assertion for docker_unreachable or plain container_exited is shown.

🔵 Suggestions

• Small status.ts monolith growth remains acceptable but should be watched (src/lib/actions/sandbox/status.ts:99): status.ts is already a relatively large sandbox action file and grows by a few lines to add classifier integration. The extraction into gateway-failure-classifier.ts keeps most new logic out of the monolith, so this is not a blocker.
  • Recommendation: No immediate change required. Continue extracting future status/gateway diagnostic logic into focused helpers rather than expanding status.ts further.
  • Evidence: Trusted monolith delta reports src/lib/actions/sandbox/status.ts grew from 396 to 401 lines, delta 5, warning rationale: current monolith grew by 1-19 lines.

Acceptance coverage

• met — Print a clearly delimited block naming the failing layer (Docker daemon up but container exited; or container exit + foreign-port-conflict; or gateway not reachable): src/lib/actions/sandbox/status.ts calls printGatewayFailureLayerHeader before existing fallback guidance; gateway-failure-classifier.ts defines getLayerHeader strings beginning with 'Failure layer: ...' for docker_unreachable, container_missing, container_exited_port_conflict, container_exited, and gateway_unreachable.
• partial — Add a small failing-layer classifier called from src/lib/actions/sandbox/status.ts and src/lib/actions/sandbox/gateway-state.ts:printGatewayLifecycleHint that prints a layer-named header before the existing actionable hints.: classifyGatewayFailure is added in src/lib/actions/sandbox/gateway-failure-classifier.ts and called from status.ts before printGatewayLifecycleHint/gateway guidance. gateway-state.ts itself is not changed in this PR.
• met — 1. docker_unreachable — docker info fails or times out. Daemon down or socket inaccessible.: classifyGatewayFailure returns layer docker_unreachable when runners.dockerInfo() is false; unit test 'returns docker_unreachable when docker info fails' verifies this path and short-circuit behavior.
• met — 2. container_exited_port_conflict — docker ps --filter name=openshell-cluster-nemoclaw shows no running container, docker ps -a shows it exited, AND something is listening on the gateway port (host port held by a foreign process).: classifier checks dockerIsRunning false, dockerExists true, then portProbe true to return container_exited_port_conflict. Unit tests cover this, and repro-2666 subprocess test fakes docker ps/docker ps -a while holding a real localhost listener.
• met — 3. container_exited — same as 2 but no foreign listener on the gateway port.: classifier returns container_exited when dockerInfo succeeds, dockerIsRunning is false, dockerExists is true, and portProbe returns false; unit test 'returns container_exited when container exited AND port is free' covers this.
• met — 4. gateway_unreachable — container is running but the gateway API does not respond (current generic case).: classifier returns gateway_unreachable when dockerInfo succeeds and dockerIsRunning(DEFAULT_CONTAINER) is true; unit test covers it, and subprocess test asserts the gateway_unreachable header before after-restart guidance.
• met — For each layer, print a one-line "what's wrong" header followed by the existing recovery hints from printGatewayLifecycleHint.: LAYER_HEADERS contains one-line messages for all layers; status.ts logs getLayerHeader(failure.layer) before printGatewayLifecycleHint in the missing/fallback paths and before existing guidance in after-restart paths. Unit test asserts headers name each layer.
• met — New helper src/lib/actions/sandbox/gateway-failure-classifier.ts (or extend gateway-state.ts).: The PR adds src/lib/actions/sandbox/gateway-failure-classifier.ts with GatewayFailureLayer, GatewayFailureResult, GatewayFailureRunners, classifyGatewayFailure, and getLayerHeader.
• met — Port probe via Node's net.connect() with a short timeout — works cross-platform (Linux/macOS/WSL) without depending on ss / lsof / netstat.: defaultPortProbe uses net.connect({ host: '127.0.0.1', port }) and PORT_PROBE_TIMEOUT_MS=2000. No ss/lsof/netstat command is introduced.
• partial — Container probe via docker ps + docker ps -a with short timeouts; gracefully degrade to unknown if Docker is itself unreachable (already covered by step 1).: dockerContainerListed invokes dockerCapture(['ps', ...]) with DOCKER_TIMEOUT_MS=3000 and optionally '-a'. Docker unreachable maps to docker_unreachable via the initial dockerInfo check rather than a literal unknown layer.
• met — Unit-testable in isolation: classifier takes injected runners for docker info, docker ps, port-probe so tests can simulate each layer.: GatewayFailureRunners injects dockerInfo, dockerIsRunning, dockerExists, and portProbe. test/gateway-failure-classifier.test.ts uses makeRunners overrides to simulate each layer and short-circuit behavior.
• partial — Subprocess test (extending test/repro-2666-silent-list-status.test.ts) per layer.: The subprocess test file is extended and includes status header ordering plus the foreign-port-holder case. Not every layer has a subprocess test; docker_unreachable and plain container_exited are unit-tested only.
• met — The container name openshell-cluster-nemoclaw is hard-coded in NemoClaw's gateway start path; treat the same string as the fixed probe target. If we ever parameterize the gateway name, classifier can read from the same source.: gateway-failure-classifier.ts defines DEFAULT_CONTAINER = 'openshell-cluster-nemoclaw' and uses it for docker ps and docker ps -a probes and detail text.
• met — nemoclaw list does not need layer classification — its contract is "always show the registry," which fix(cli): keep status and list output visible when gateway probe fails (#2666) #3270 already delivers.: Diff only adds classifier integration to status-related fallback paths. Existing repro test for 'nemoclaw list never produces silent empty output' remains and asserts registry output with exit code 0.
• met — status prints a clearly-named layer header in each of the four classified states.: getLayerHeader returns headers for the original four states and the PR also adds container_missing as a fifth state. Unit tests assert every layer header contains its layer name.
• met — Classifier has unit tests per layer.: test/gateway-failure-classifier.test.ts covers docker_unreachable, gateway_unreachable, container_missing, container_exited_port_conflict, and container_exited.
• met — Repro subprocess test extended to assert the named layer appears for the (container-stopped + foreign-port-holder) scenario.: test/repro-2666-silent-list-status.test.ts adds an async subprocess test that binds a local port, fakes docker ps and docker ps -a for an exited openshell-cluster-nemoclaw container, runs the CLI, asserts output contains container_exited_port_conflict, and expects a non-zero exit.
• met — Surfaced from [Ubuntu 22.04][CLI&UX][Recovery] nemoclaw status and list return empty output, exit 0 when container is stopped and gateway port is held #2666 / fix(cli): keep status and list output visible when gateway probe fails (#2666) #3270.: The PR extends test/repro-2666-silent-list-status.test.ts and preserves the existing [Ubuntu 22.04][CLI&UX][Recovery] nemoclaw status and list return empty output, exit 0 when container is stopped and gateway port is held #2666 list/status non-silent-output regression coverage while adding feat(status): classify failing layer when gateway probe fails (#2666 follow-up) #3271 classifier checks.

Security review

• pass — 1. Secrets and Credentials: No hardcoded secrets, tokens, passwords, PEM material, credential JSON, or secret-bearing fixtures are introduced. The change adds diagnostic classification and tests with fake docker/openshell scripts.
• pass — 2. Input Validation and Data Sanitization: The new docker probes use fixed argument arrays through the existing docker adapter and compare exact container names; they do not concatenate user input into shell commands. The port probe connects only to 127.0.0.1 and uses the existing gateway port configuration, limiting SSRF exposure.
• pass — 3. Authentication and Authorization: No endpoints, authentication checks, authorization checks, token validation, scopes, or ownership decisions are changed. The classifier only emits diagnostic output after gateway verification failure paths.
• pass — 4. Dependencies and Third-Party Libraries: No new third-party dependencies or registry sources are added. The implementation uses Node's built-in net module and existing NemoClaw docker adapters.
• pass — 5. Error Handling and Logging: The added output is a static failure-layer header and non-sensitive detail text. It does not log credentials or trust material. Existing lookup.output printing behavior is unchanged.
• pass — 6. Cryptography and Data Protection: Not applicable — no cryptographic operations, key handling, encryption, hashing, or data-protection behavior is changed.
• pass — 7. Configuration and Security Headers: No HTTP server headers, CORS policy, Dockerfiles, container privileges, exposed ports, workflow permissions, or runtime policy defaults are changed. The diagnostic probe uses localhost and existing gateway port configuration.
• warning — 8. Security Testing: Unit tests cover each classifier layer and subprocess tests cover key fallback/header behavior including the foreign-port-holder case. However, this is sandbox/gateway runtime behavior and the E2E Advisor required sandbox-operations-e2e; passing evidence is missing for the current head SHA.
• warning — 9. Holistic Security Posture: The change appears fail-closed and diagnostic-only, with no obvious sandbox escape, SSRF bypass, policy bypass, credential leakage, blueprint tampering, installer trust issue, or workflow trusted-code boundary regression. Because it touches runtime gateway failure paths, required E2E confirmation remains necessary before merge.

Test / E2E status

• Test depth: e2e_required — Runtime/sandbox/infrastructure paths need real execution coverage: src/lib/actions/sandbox/gateway-failure-classifier.ts and src/lib/actions/sandbox/status.ts. The added unit tests and fake-docker subprocess tests are strong, but they cannot fully prove behavior against a real Docker/OpenShell gateway runtime for the current head SHA.
• E2E Advisor: missing
• Required E2E jobs: sandbox-operations-e2e
• Missing for analyzed SHA: sandbox-operations-e2e

✅ What looks good

• Codebase drift check: all four patched files exist in the current diff; trusted openPrOverlaps is empty, though status.ts has recent active history and should be reviewed carefully.
• The new classifier is extracted into a focused module rather than embedding most logic in status.ts.
• Docker probing goes through existing docker adapters instead of direct shell-string docker execution, aligning with the docker abstraction guard.
• The classifier distinguishes container_missing from container_exited by checking docker ps -a before considering port conflict, addressing the existence-check gap called out in earlier related work.
• Unit tests cover every classifier layer plus short-circuit behavior, and the subprocess regression uses a real localhost listener to exercise the foreign-port-holder classification without relying on a fixed host port.
• The implementation is diagnostic-only and does not alter sandbox credentials, policies, network policy, Dockerfiles, installers, or workflow permissions.

Review completeness

• This advisory review used the provided trusted PR metadata, linked issue body, issue comments, GraphQL/status evidence, and supplied diff; no tests, scripts, package-manager commands, or workflows were executed.
• The E2E Advisor auto-dispatched sandbox-operations-e2e for the current head SHA, but no passing result for eaf10d3 was present in the provided evidence.
• The linked issue feat(status): classify failing layer when gateway probe fails (#2666 follow-up) #3271 had no issue comments in the provided metadata, so acceptance extraction is based on the issue body only plus PR discussion comments.
• Review thread state was available via GraphQL as an empty node list, but GitHub still reports reviewDecision=REVIEW_REQUIRED.
• Human maintainer review required: yes