fix(cli): report inference health from a served request - #8731
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>
|
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:
📝 WalkthroughWalkthroughSandbox inference health now combines route reachability with an authenticated inference request. Startup readiness, status output, failure labels, diagnostics, tests, and documentation now distinguish healthy, unauthorized, unreachable, and unhealthy results. ChangesSandbox inference health
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant statusSnapshot
participant Gateway
participant SandboxInference
participant RouteHealth
User->>statusSnapshot: request sandbox status
statusSnapshot->>Gateway: probe inference route
Gateway-->>statusSnapshot: route reachability
statusSnapshot->>SandboxInference: send authenticated inference request
SandboxInference-->>statusSnapshot: response and HTTP status
statusSnapshot->>RouteHealth: combine route and invocation results
RouteHealth-->>User: request-aware inference health
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
laitingsheng
added
area: cli
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: Manual-only E2E: 1 optional E2E recommendation
This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit a36cc95 in the TypeScript / code-coverage/cliThe overall coverage in commit a36cc95 in the Show a code coverage summary of the most impacted files.
Updated |
|
🌿 Preview your docs: https://nvidia-preview-pr-8731.docs.buildwithfern.com/nemoclaw |
|
Maintainer decision required before readiness work continues This PR changes the product contract in ways that need explicit maintainer direction:
The sensitive-path and documentation receipts must also be refreshed for the current branch revision after the direction is settled. I am deferring implementation, approval, and merge because resolving the contract requires product and architecture judgment. |
prekshivyas
left a comment
prekshivyas
left a comment
There was a problem hiding this comment.
Blocking correctness finding on latest PR commit 4a5cbca0d.
status-snapshot.ts now takes the provider/model pair from the usable live gateway route, but still passes sb?.preferredInferenceApi from the recorded sandbox into runSandboxInferenceInvocationProbe. When live route drift changes the API family, status can send the wrong request shape/endpoint and report an otherwise usable route as unhealthy.
Derive provider, model, and API family from one route authority. If the live route is selected, use its compatible API family; otherwise use the complete recorded route contract. Add a status regression where the recorded route is Responses-based and the live route requires Chat Completions, then assert the served request uses the live route's endpoint and reports healthy.
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Addressed by a4b22d8: live and recorded inference routes now keep provider/model/API coherent; 13 focused status-inference tests and the exact-head pre-push typecheck passed.
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
Security review completed for the current #8731 revision. Scope:
Result: no actionable security finding. The request uses the fixed Validation on commit
Fresh repository CI is running. The prior review request is resolved by the contributor's two route-coherence commits. The follow-up harness commit repairs the module rename that caused the earlier CI shard failures. |
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
The failed CLI recovery check was caused by its OpenShell fixture not answering the new inference invocation. Revision 282a38b updates that fixture without changing production behavior. Five source-test files passed 84 tests, and three CLI integration files passed 29 tests. The Station pair-preparation failure was unrelated to the inference changes and passed twice in isolation, so the new revision provides a safe fresh CI run. The signed commit is Verified. |
prekshivyas
left a comment
prekshivyas
left a comment
There was a problem hiding this comment.
Blocking inference-health correctness finding on latest PR commit 2bf953663.
src/lib/actions/sandbox/status-snapshot.ts:567-576 clears preferredInferenceApi whenever the live RPC returns a provider/model pair, including when that pair exactly matches the recorded sandbox route. For a compatible endpoint recorded with preferredInferenceApi: "openai-responses", status therefore discards the only API-family metadata and probes the default Chat Completions path, producing a false unhealthy result against a Responses-only endpoint. The new test covers a changed live route, but not the unchanged-pair case.
Please retain the recorded API family when the live provider/model pair exactly equals the recorded pair, and clear it only when the pair has drifted (or expose the API family through the live RPC). Add a same-pair Responses regression that asserts /v1/responses is probed.
Security rubric: all nine categories PASS; this is a correctness blocker rather than a security vulnerability. Files reviewed: the complete 27-file diff.
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
Addressed the unchanged-route inference-health blocker and refreshed the branch from current
Validation on the pushed revision:
Security review remains clean across the nine-category rubric. The change preserves the route-authority boundary by retaining API-family metadata only for an unchanged provider/model pair. The effective PR diff is 27 files with 730 changed lines, below the large-change threshold used in this readiness pass. Fresh repository checks and re-review are now pending. I did not change reviewer routing. |
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
The failed CLI shard exposed a stale documentation-contract assertion after the reviewed wording change. Updated that assertion to match the existing source page. Validation:
Fresh checks and human re-review are pending. Reviewer routing was not changed. |
The latest PR commit retains the recorded API family when the live route matches and tests the Responses endpoint. Dismissing this resolved review before a full current-commit review.
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
Addressed the response-validation blocker on this branch.
|
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
The failed CLI shards were PR-related: their status and recovery fixtures returned a successful HTTP status without a response body, which the new readiness validation correctly rejects. Updated the shared fixture to return structurally valid bodies for Chat Completions, Responses, and Anthropic Messages, and aligned the recovery fixture. The five affected test files now pass locally (58 tests), CLI type-checking passes, repository hooks pass, and the documentation follow-up confirmed no additional docs change is needed. The earlier local loopback-listener error was environmental; the same tests passed with their required host access. |
prekshivyas
left a comment
prekshivyas
left a comment
There was a problem hiding this comment.
Reviewed the current head, served-request health behavior across status/start/rebuild, required CI, and advisor feedback. Required checks are green and no blocking findings remain.
…9307) <!-- markdownlint-disable MD041 --> ## Summary Sandbox status probes the live shared inference route with one real inference request, and that request's result decides `status --json`'s exit code. The probe dropped the sandbox's recorded API family whenever the live route was not exactly aligned — including when the shared route drifted by **model alone** and the provider was unchanged. For a compatible endpoint that family is the only signal that the route speaks `openai-responses` or `anthropic-messages`, so a healthy route was probed with the wrong API and reported unhealthy, making the command exit nonzero. Refs #9302. ## Reproduction **Environment** - Test machine: our Ubuntu 24.04 x86_64 test host (no GPU) - NemoClaw `main` at `588bb6db9b1132266840e4604fa16c2a4912cbfc` (`v0.0.109-96-g588bb6db9`), OpenShell CLI 0.0.101 - Sandbox: OpenClaw, provider NVIDIA Endpoints (`nvidia-prod`) - The reporter's platform is Ubuntu 26.04 GPU; this path is CLI/JSON exit-code logic with no GPU or OS-version dependency, but see **Platform scope** below. First, what already works on `main`. Recorded route `nvidia-prod/nvidia/nemotron-3-ultra-550b-a55b`; the shared route was changed externally with `openshell inference set` to another route that genuinely serves: ```text ### drift to a healthy route exit=0 | drift=yes | live=openai/gpt-oss-20b | inferenceHealth.ok=True | recordedRoute/liveRoute/routeDrift all present ``` Route drift on its own already exits `0` and already returns all three documented fields, so the drift itself is not the trigger. The trigger is the API family. With the recorded route on a compatible endpoint that speaks `openai-responses`, and the shared route drifting by model only, the probe on `main` is handed no API family at all: ```json { "provider": "compatible-endpoint", "model": "live/model", "preferredInferenceApi": null } ``` `getSandboxInferenceConfig` resolves `null` to `openai-completions`, which for that provider is the wrong endpoint: ```text compatible-endpoint recorded=openai-responses kept=openai-responses dropped=openai-completions <-- family changes compatible-endpoint recorded=anthropic-messages kept=anthropic-messages dropped=openai-completions <-- family changes compatible-endpoint recorded=openai-completions kept=openai-completions dropped=openai-completions nvidia-prod recorded=openai-responses kept=openai-completions dropped=openai-completions ``` The request then fails against a route that is actually healthy, `inferenceHealth.ok` goes false, and `status --json` exits `1`. **Observed on `main` (before fix)** — invocation probe input for a model-only drift on a compatible endpoint: ```json { "provider": "compatible-endpoint", "model": "live/model", "preferredInferenceApi": null } ``` **Observed on `fix/...` (after fix)** ```json { "provider": "compatible-endpoint", "model": "live/model", "preferredInferenceApi": "openai-responses" } ``` Live re-verification on the test host, with the same sandbox, after the fix: ```text ### drift to a healthy route -> exit=0 | inferenceHealth.ok=True | all three fields present ### route aligned -> exit=0 | inferenceHealth.ok=True | all three fields present ### drift to an unusable route -> exit=1 | inferenceHealth.ok=False | all three fields present ``` The third row is unchanged on purpose — see **Scope** below. ## Analysis `src/lib/actions/sandbox/status-snapshot.ts` builds the route for the in-sandbox invocation probe. It takes provider and model as one pair from the live gateway route, which #8731 introduced deliberately, and then set: ```ts preferredInferenceApi: routeDriftPlan?.kind === "aligned" ? (sb?.preferredInferenceApi ?? null) : null, ``` `aligned` means provider **and** model both match. The stated reason for dropping the family is sound but only covers the provider case: a family recorded for one provider must not be carried onto a different provider, because `getSandboxInferenceConfig` would otherwise route e.g. a persisted `openai-responses` onto a provider with no `/v1/responses` endpoint and 404 every request — the same hazard its own comment describes. That reason does not hold when only the model drifted. The recorded family describes the recorded *provider*, and that provider is unchanged, so the family still describes the live route exactly. Dropping it there falls back to `openai-completions` and probes an endpoint the provider does not serve, so a healthy route is reported unhealthy and the command exits nonzero. Built-in providers hide this: `getSandboxInferenceConfig` forces the family from the provider for `anthropic-prod` and for every provider matching `shouldSkipResponsesProbe`, so `nvidia-prod` resolves to `openai-completions` either way. Only providers whose family is carried in the sandbox record — the compatible endpoints — are affected. ## Fix Gate on the provider rather than on full alignment: ```diff - routeDriftPlan?.kind === "aligned" ? (sb?.preferredInferenceApi ?? null) : null, + live.provider === sb?.provider ? (sb?.preferredInferenceApi ?? null) : null, ``` This is strictly wider than `aligned` (which already required the provider to match), so the aligned case is unchanged and the cross-provider guard is preserved verbatim: the family is still dropped whenever the provider itself drifted. Three tests enforce the contract: the recorded API family survives a model-only drift and an aligned route, while a provider change removes it. The first test fails against the earlier logic and passes after the fix. ## Scope This PR uses `Refs`, not `Fixes`, because it covers one of two ways the reported symptom can arise and I could not confirm which one the reporter hit. Measured on `main`, `status --json` exits nonzero after a shared-route change in exactly two situations: 1. **The live route is healthy but probed with the wrong API family.** That is the defect this PR fixes. 2. **The live route is genuinely unusable** (for example the shared route was repointed at a placeholder model to manufacture the mismatch). There `inferenceHealth.ok` is legitimately false and the nonzero exit is the contract #8731 established for #8705 — status sends one real inference request and reports its result. Worth noting for the second case: the exit code does not withhold the payload. In every failing run above, stdout still carried the complete JSON document with `recordedRoute`, `liveRoute` and `routeDrift` populated, so a caller can read the drift fields even when the command exits `1`. If the validation job treats a nonzero exit as "no JSON to parse", it will report the fields as unavailable when they are in fact present. To confirm whether case 1 is what the pipeline hit, the reporter's provider and `--inference-api` for the sandbox under test, plus the captured stdout of the failing `status --json`, would settle it. ## Platform scope Reproduced and verified on our Ubuntu 24.04 x86_64 test host; the reporter's runner is Ubuntu 26.04 GPU. The changed code is provider/API-family resolution in the status snapshot, with no GPU, kernel, or OS-version dependency, but this was not re-run on Ubuntu 26.04. ## Changes - `src/lib/actions/sandbox/status-snapshot.ts`: keep the recorded API family while the live provider matches the recorded provider; drop it only on a provider change. - `src/lib/actions/sandbox/status-snapshot-route-drift.test.ts`: cover model-only drift, aligned routes, and provider changes at the public health-result boundary. - `docs/reference/commands.mdx` and `docs/inference/verify-inference-route.mdx`: describe API-family selection during route drift. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [x] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Verification - [x] `npx prek run` passes on the changed files - [x] `npm test` passes (3105 tests across `src/lib/actions/sandbox/` and `test/cli/sandbox-status-json.test.ts`) - [x] Tests added or updated for new or changed behavior - [x] Focused route-drift tests pass (11 of 11) - [x] No secrets, API keys, or credentials committed - [x] Docs updated for the user-facing route-validation behavior - [x] `npm run docs` passes with 0 errors and 2 existing warnings - [x] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) - [ ] New doc pages include SPDX header and frontmatter (new pages only) ## Documentation Writer Review - [x] Documentation writer subagent reviewed the completed changes - Result: `docs-updated` - Evidence: `docs/reference/commands.mdx` and `docs/inference/verify-inference-route.mdx` - Agent: Codex Desktop <!-- docs-review-head-sha: 3f3f6cd --> <!-- docs-review-agents-blob-sha: b9fb6a9 --> ## AI Disclosure - [x] AI-assisted — tool: Claude Code Signed-off-by: Yanyun Liao <yanyunl@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Improved inference health checks when only the selected model changes. - Preserved the recorded API family for the same provider, including compatible response-based routes. - Prevented outdated API routing from being reused when the provider changes. - Health is now reported only for structurally valid responses. - **Documentation** - Updated sandbox status and inference route documentation to reflect provider and model handling. - **Tests** - Added regression coverage for provider and model routing changes. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Yanyun Liao <yanyunl@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com> Co-authored-by: Apurv Kumaria <akumaria@nvidia.com>
4 participants
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/verify-inference-route.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 1; follow-up rebuild harness validation passed 2 files and 9 tests after the probe rename; the runtime-recovery compatibility test and its related CLI suites passed 3 files and 29 tests after the fixture learned the agent-request probenpm 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