fix(sandbox): harden route probe - #6637
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughDCode sandbox route probes now run through a managed launcher with controlled startup environments, preserve stderr for classification, and identify missing helpers. Health and connection flows fail closed with rebuild guidance, supported by expanded tests and troubleshooting documentation. ChangesDCode route probing
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ConnectSandbox
participant OpenShell
participant RouteProbe as parseSandboxInferenceRouteProbeResult
participant RouteHealth as probeSandboxInferenceGatewayHealth
ConnectSandbox->>OpenShell: run inference.local probe with streams
OpenShell-->>RouteProbe: return status, output, and stderr
RouteProbe-->>RouteHealth: return route classification
RouteHealth-->>ConnectSandbox: return healthy or structured unhealthy result
ConnectSandbox-->>ConnectSandbox: fail closed for untrusted or missing-helper evidence
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
|
🌿 Preview your docs: https://nvidia-preview-pr-6637.docs.buildwithfern.com/nemoclaw |
Contributor
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage remains at 96%, unchanged from the TypeScript / code-coverage/cliThe overall coverage in the Show a code coverage summary of the most impacted files.
Updated |
Contributor
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Contributor
E2E Target RecommendationRequired E2E targets: Dispatch required E2E targets:
Full E2E target advisor summaryE2E Target AdvisorBase: Required E2E targets
Optional E2E targets
Relevant changed files
|
Contributor
PR Review Advisor (Nemotron Ultra) — Changes requestedMerge posture: Do not merge yet Action checklist
Findings index
🚨 Required before mergeAddress these before merging unless a maintainer explicitly overrides the advisor with rationale.
|
Contributor
PR Review Advisor — No blocking findingsMerge posture: No blocking advisor findings Action checklist
Test follow-ups to resolve or justifyIf these cover changed behavior, prefer adding them in this PR; otherwise state why existing coverage is enough or link the follow-up.
This is an automated, non-binding review; it still expects maintainers and agents to respond to each required or warning item. Treat suggestions as current-PR improvements when they touch changed code; defer only with maintainer rationale or a linked follow-up. 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/actions/sandbox/connect-inference-route-probe.ts (1)
109-134: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winNormalize stderr before returning probe detail. The early stderr return skips
isDcodeManagedExecMissingDetail, so a missing-launcher error on stderr won’t get the rebuild hint. It also preserves ANSI/control bytes indetail; strip them here before truncating, or run stderr through the same canonicalization path as stdout.🤖 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 `@src/lib/actions/sandbox/connect-inference-route-probe.ts` around lines 109 - 134, Normalize stderr through the same canonicalization used for probe output before returning it from the early stderr branch. Apply ANSI/control-byte stripping and whitespace normalization, then pass the cleaned detail through isDcodeManagedExecMissingDetail so missing-launcher errors receive DCODE_MANAGED_EXEC_MISSING_DETAIL before truncation.Source: Path instructions
🧹 Nitpick comments (2)
src/lib/actions/sandbox/connect-inference-route-probe.test.ts (1)
219-231: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd stderr-channel coverage for missing-helper detection.
This test only models the missing-launcher failure via
stdout/status: 127. SinceparseSandboxInferenceRouteProbeResultnow treats non-emptystderras an immediate, differently-handled failure path (see companion comment onconnect-inference-route-probe.tsLines 109-134), add a parallel case usingstderrto confirm the rebuild-guidance detail is still produced through that channel once the ordering issue is fixed.As per path instructions: "Flag copied production algorithms, broad mocks that bypass the behavior under test, and conditionals that make a test pass without exercising its claim."
🤖 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 `@src/lib/actions/sandbox/connect-inference-route-probe.test.ts` around lines 219 - 231, Add a parallel test for parseSandboxInferenceRouteProbeResult that supplies the missing DCode launcher message through non-empty stderr, then assert healthy is false, broken is false, httpStatus is 0, and detail equals DCODE_MANAGED_EXEC_MISSING_DETAIL with isDcodeManagedExecMissingDetail(parsed.detail) true. Ensure the test exercises the real stderr handling path without broad mocks or bypassing the parser logic.Source: Path instructions
src/lib/actions/sandbox/inference-route-health.test.ts (1)
99-111: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExtend
makeCaptureto cover thestderrchannel.
makeCaptureonly supportsoutput/status, so no test in this file can exercise thestderrpath thatincludeStreams: truewas added to support (Lines 45 ininference-route-health.ts). Add astderrparameter and a corresponding test case to confirmprobeSandboxInferenceGatewayHealthcorrectly classifies astderr-sourced missing-helper failure.As per path instructions: "Flag copied production algorithms, broad mocks that bypass the behavior under test, and conditionals that make a test pass without exercising its claim."
🤖 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 `@src/lib/actions/sandbox/inference-route-health.test.ts` around lines 99 - 111, The test helper makeCapture currently cannot produce stderr-based failures, leaving the includeStreams path untested. Extend makeCapture to accept and return a stderr value, then add a probeSandboxInferenceGatewayHealth test that supplies a stderr-sourced missing-helper error and asserts the same failed compatibility result and detail classification as the existing output-based case.Source: Path instructions
🤖 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.
Outside diff comments:
In `@src/lib/actions/sandbox/connect-inference-route-probe.ts`:
- Around line 109-134: Normalize stderr through the same canonicalization used
for probe output before returning it from the early stderr branch. Apply
ANSI/control-byte stripping and whitespace normalization, then pass the cleaned
detail through isDcodeManagedExecMissingDetail so missing-launcher errors
receive DCODE_MANAGED_EXEC_MISSING_DETAIL before truncation.
---
Nitpick comments:
In `@src/lib/actions/sandbox/connect-inference-route-probe.test.ts`:
- Around line 219-231: Add a parallel test for
parseSandboxInferenceRouteProbeResult that supplies the missing DCode launcher
message through non-empty stderr, then assert healthy is false, broken is false,
httpStatus is 0, and detail equals DCODE_MANAGED_EXEC_MISSING_DETAIL with
isDcodeManagedExecMissingDetail(parsed.detail) true. Ensure the test exercises
the real stderr handling path without broad mocks or bypassing the parser logic.
In `@src/lib/actions/sandbox/inference-route-health.test.ts`:
- Around line 99-111: The test helper makeCapture currently cannot produce
stderr-based failures, leaving the includeStreams path untested. Extend
makeCapture to accept and return a stderr value, then add a
probeSandboxInferenceGatewayHealth test that supplies a stderr-sourced
missing-helper error and asserts the same failed compatibility result and detail
classification as the existing output-based case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 88f5d645-370f-4e59-b092-7aa356d9f61c
📒 Files selected for processing (9)
docs/reference/troubleshooting.mdxsrc/lib/actions/sandbox/connect-flow-dcode-probe-preamble.test.tssrc/lib/actions/sandbox/connect-flow.test.tssrc/lib/actions/sandbox/connect-inference-route-probe.test.tssrc/lib/actions/sandbox/connect-inference-route-probe.tssrc/lib/actions/sandbox/connect.tssrc/lib/actions/sandbox/inference-route-health.test.tssrc/lib/actions/sandbox/inference-route-health.tstest/support/connect-flow-test-harness.ts
sandl99
added
bug-fix
Contributor
E2E Target Results — ✅ All requested jobs passedRun: 29095182896
|
jyaunches
approved these changes
Jul 10, 2026
cv
pushed a commit
that referenced
this pull request
Jul 11, 2026
## Summary Release-prep documentation for **v0.0.80**. Adds the `## v0.0.80` section to `docs/about/release-notes.mdx` summarizing user-facing changes since v0.0.79, each bullet linking to the relevant deeper page. Produced via `nemoclaw-contributor-update-docs` (pre-tag path): scanned `v0.0.79..HEAD`, applied the docs skip list (no violations), and confirmed the 8 commits that already shipped in-PR docs are complete. No new pages needed. ## Source summary - #6507 -> `docs/about/release-notes.mdx`: Hermes v0.18 + Slack Block Kit (rich rendering, digest-pinned base image). - #6584 / #6616 -> `docs/about/release-notes.mdx`: host-local OpenRouter runtime attribution adapter (port `11437`, `NEMOCLAW_OPENROUTER_RUNTIME_ADAPTER_PORT`) and native Deep Agents `openrouter` provider. - #6210 / #6292 -> `docs/about/release-notes.mdx`: host corporate proxy CA import into sandbox trust (`NEMOCLAW_CORPORATE_CA_BUNDLE`, `NEMOCLAW_CORPORATE_CA_IMPORT`). - #6624 / #6623 / #6656 -> `docs/about/release-notes.mdx`: release-matched base-image selection, surfaced cluster-image build diagnostics, preserved Nemotron profile registration. - #6629 / #6637 -> `docs/about/release-notes.mdx`: bare `connect` default-sandbox behavior and route-probe hardening. - #6634 / #6626 / #6596 / #5569 / #6610 / #6655 -> `docs/about/release-notes.mdx`: onboarding/recovery preservation, stale-gateway-PID fix, installer backup message, vLLM label on managed platforms. - #6578 / #5670 -> `docs/about/release-notes.mdx`: automatic Hermes light terminal skin and non-interactive `npx` MCP server startup. ## Verification `npm run docs`: 0 errors, all internal links resolve (2 pre-existing hidden-page warnings). `_build/` variants for OpenClaw, Hermes, and Deep Agents all regenerate with the v0.0.80 section. Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Added release notes for v0.0.80. * Documented Hermes upgrades, including Slack Block Kit rendering. * Added details on OpenRouter traffic routing and attribution headers. * Documented improved proxy certificate handling and sandbox reliability. * Highlighted enhanced connection defaults, route-probing safeguards, onboarding recovery, and terminal/MCP startup behavior. * Added references to relevant user-guide documentation. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Hadar301
pushed a commit
to Hadar301/NemoClaw-OpenShift
that referenced
this pull request
Jul 12, 2026
<!-- markdownlint-disable MD041 --> ## Summary See NVBug 6438202. ## Related Issue NVBug 6438202 ## Changes - See NVBug 6438202. ## 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) ## Quality Gates <!-- Check exactly one tests line and one docs line. Check other lines when applicable. Add every requested justification or approval reference. --> - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [x] Docs updated for user-facing behavior changes - [ ] Docs not applicable — justification: - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: NVBug 6438202 tracks internal security review; public details intentionally withheld. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Verification <!-- Check each applicable item only when supported by the requested evidence. Run targeted tests once per relevant change set and rerun after later edits or hook autofixes that can affect the tested behavior. Do not rerun hook-covered checks. --> - [x] PR description includes the DCO sign-off declaration and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run check:diff` passed 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 or justification: targeted CLI tests passed locally; details withheld, see NVBug 6438202. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: - [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) - [x] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) --- <!-- DCO sign-off is required in this PR description, and every commit must appear as Verified in GitHub. Run: git config user.name && git config user.email --> Signed-off-by: San Dang <sdang@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved sandbox connectivity checks to reject untrusted probe results, including unexpected startup errors. * Added clear failure handling and rebuild guidance when required sandbox helpers are missing. * Improved route health reporting for affected Deep Agents sandboxes. * **Documentation** * Added troubleshooting guidance for missing sandbox route-probe helpers, including rebuild and verification steps. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Hadar301
pushed a commit
to Hadar301/NemoClaw-OpenShift
that referenced
this pull request
Jul 12, 2026
## Summary Release-prep documentation for **v0.0.80**. Adds the `## v0.0.80` section to `docs/about/release-notes.mdx` summarizing user-facing changes since v0.0.79, each bullet linking to the relevant deeper page. Produced via `nemoclaw-contributor-update-docs` (pre-tag path): scanned `v0.0.79..HEAD`, applied the docs skip list (no violations), and confirmed the 8 commits that already shipped in-PR docs are complete. No new pages needed. ## Source summary - NVIDIA#6507 -> `docs/about/release-notes.mdx`: Hermes v0.18 + Slack Block Kit (rich rendering, digest-pinned base image). - NVIDIA#6584 / NVIDIA#6616 -> `docs/about/release-notes.mdx`: host-local OpenRouter runtime attribution adapter (port `11437`, `NEMOCLAW_OPENROUTER_RUNTIME_ADAPTER_PORT`) and native Deep Agents `openrouter` provider. - NVIDIA#6210 / NVIDIA#6292 -> `docs/about/release-notes.mdx`: host corporate proxy CA import into sandbox trust (`NEMOCLAW_CORPORATE_CA_BUNDLE`, `NEMOCLAW_CORPORATE_CA_IMPORT`). - NVIDIA#6624 / NVIDIA#6623 / NVIDIA#6656 -> `docs/about/release-notes.mdx`: release-matched base-image selection, surfaced cluster-image build diagnostics, preserved Nemotron profile registration. - NVIDIA#6629 / NVIDIA#6637 -> `docs/about/release-notes.mdx`: bare `connect` default-sandbox behavior and route-probe hardening. - NVIDIA#6634 / NVIDIA#6626 / NVIDIA#6596 / NVIDIA#5569 / NVIDIA#6610 / NVIDIA#6655 -> `docs/about/release-notes.mdx`: onboarding/recovery preservation, stale-gateway-PID fix, installer backup message, vLLM label on managed platforms. - NVIDIA#6578 / NVIDIA#5670 -> `docs/about/release-notes.mdx`: automatic Hermes light terminal skin and non-interactive `npx` MCP server startup. ## Verification `npm run docs`: 0 errors, all internal links resolve (2 pre-existing hidden-page warnings). `_build/` variants for OpenClaw, Hermes, and Deep Agents all regenerate with the v0.0.80 section. Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Added release notes for v0.0.80. * Documented Hermes upgrades, including Slack Block Kit rendering. * Added details on OpenRouter traffic routing and attribution headers. * Documented improved proxy certificate handling and sandbox reliability. * Highlighted enhanced connection defaults, route-probing safeguards, onboarding recovery, and terminal/MCP startup behavior. * Added references to relevant user-guide documentation. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.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
2 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
See NVBug 6438202.
Related Issue
NVBug 6438202
Changes
Type of Change
Quality Gates
Verification
Verifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailablenpm 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: San Dang sdang@nvidia.com
Summary by CodeRabbit
Bug Fixes
Documentation