fix(cli): report inference health from a served request - #8731
fix(cli): report inference health from a served request#8731laitingsheng wants to merge 2 commits into
Conversation
The in-sandbox route probe reads any final HTTP 200-499 as reachable, so a rejected provider credential answered 401 while status reported healthy inference and exited 0. Status and start now send one inference request over the same route and report its result, and start exits nonzero when the request fails. Signed-off-by: Tinson Lai <tinsonl@nvidia.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 (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughSandbox inference health now includes an in-sandbox inference request after route probing. Startup readiness, status output, failure labels, diagnostics, tests, and documentation now reflect request results. ChangesSandbox inference health
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant SandboxStatus
participant GatewayProbe
participant InferenceProbe
participant RouteHealth
CLI->>SandboxStatus: request status
SandboxStatus->>GatewayProbe: probe sandbox route
GatewayProbe-->>SandboxStatus: reachability result
SandboxStatus->>InferenceProbe: send provider/model inference request
InferenceProbe-->>SandboxStatus: response status and invocation result
SandboxStatus->>RouteHealth: combine probe results
RouteHealth-->>CLI: inference health and diagnostic subprobes
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
3 terminology differences from the second opinionAdvisory only. These are normalized differences from the primary terminology receipt.
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. 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 against this exact revision. Recommended E2E: 2 optional E2E recommendations
1 warning · 0 suggestionsWarningsWarnings do not block.
|
Summary
nemoclaw <name> statusreportedinferenceHealth.ok: trueand exited0while the in-sandbox inference route answered HTTP 401, andstartexited0on the same route. The route probe reads any final HTTP 200-499 as reachable, so a rejected provider credential passed it. Status and start now send one inference request over the same route and report its result, so a rejected credential reportsunauthorizedand exits nonzero.Related Issue
Fixes #8705
Changes
src/lib/actions/sandbox/rebuild-inference-preflight.tstoinference-invocation-probe.tsand its exports tobuildSandboxInferenceInvocationCommandandprobeSandboxInferenceInvocation. The module already sent one minimal completion throughinference.localon the sandbox's stored credential, but only Deep Agents Code rebuild preflight consumed it. Status and start are the second and third consumers, so the name now describes the behaviour rather than the first caller. The failure result addshttpStatus, and the timeout is a parameter: rebuild keeps 100 seconds, status and start use 30.src/lib/actions/sandbox/inference-route-health.ts. When the route probe reports the route reachable,statussends one inference request and its result drivesinferenceHealth.ok; HTTP 401 or 403 setsfailureLabel: "unauthorized". The route probe result becomes aroute reachabilitysubprobe so a failing verdict still shows that the route itself answered. A host-side upstream probe stays a diagnostic and does not changeok, keeping the sandbox route authoritative.startSandboxreadiness on the same request insideverifyStarted, after the existing gateway and forward checks, and exit1when it fails. A sandbox with no recorded provider or model has nothing to request, so start skips it and exits0.docs/reference/commands.mdx, including theInferencestate table, and the pages whose acceptance criteria or failing-state lists this change invalidates:docs/inference/choose-local-inference-server.mdx,docs/inference/set-up-llama-cpp.mdx,docs/inference/set-up-ollama.mdx,docs/inference/set-up-vllm-on-two-dgx-stations.mdx,docs/deployment/deploy-to-headless-server.mdx,docs/monitoring/monitor-sandbox-activity.mdx,docs/reference/troubleshooting.mdx, anddocs/security/credential-rotation.mdx.inference healthentries in.agents/skills/_shared/controlled-words.md, which defined the term as the/v1/modelsclassification alone.Contract changes maintainers should weigh
nemoclaw statusreports inference healthy on endpoint reachability, not model invocability — a green status can mask an unreachable model #6846. Two tests asserted the reported behaviour and are rewritten: the unit test expecting{ ok: true, okLabel: "reachable" }beside a failedunauthorizedsubprobe, and the CLI test named "treats an inference.local HTTP %s as healthy", which asserted exit0for 401 and 403.statusandstartnow send a 16-token request through the stored provider credential on every run, so both wait up to 30 seconds for it and consume provider tokens on a hosted route.doctoris unchanged and still classifies an HTTP 401 or 403 route response as reachable, so it exits0wherestatusnow reportsunauthorized. The difference is documented; makingdoctorconsistent needs its own change.phaseto stop reportingReadywhen inference is broken.phaseis OpenShell's own container lifecycle value, parsed insrc/lib/state/gateway.tsand shared withTERMINAL_SANDBOX_PHASES, so this change leaves it alone and reports the condition throughinferenceHealthand the exit codes instead.statusinference health can only attest a fresh exec's environment, never the serving process's — surface process-level (self-reported) health #7003 remains open. The request runs in a freshsandbox exec, so it attests the route and credential, not the long-running gateway process's own environment.Type of Change
Quality Gates
Documentation Writer Review
docs-updateddocs/reference/commands.mdx,docs/inference/choose-local-inference-server.mdx,docs/inference/set-up-llama-cpp.mdx,docs/inference/set-up-ollama.mdx,docs/inference/set-up-vllm-on-two-dgx-stations.mdx,docs/deployment/deploy-to-headless-server.mdx,docs/monitoring/monitor-sandbox-activity.mdx,docs/reference/troubleshooting.mdx,docs/security/credential-rotation.mdx,.agents/skills/_shared/controlled-words.mdDGX 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 src/lib/actions/sandbox/status-snapshot-inference-health.test.ts src/lib/actions/sandbox/status-inference.test.ts src/lib/actions/sandbox/start.test.ts src/lib/actions/sandbox/inference-invocation-probe.test.ts src/lib/actions/sandbox/status-snapshot-recovery.test.ts— 5 files, 71 tests passed;npx vitest run test/cli— 35 files, 283 tests passed; the new CLI cases fail against a pre-fix build withexpected +0 to be 1npm 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: Tinson Lai tinsonl@nvidia.com
Summary by CodeRabbit
New Features
Documentation