refactor(doctor): separate diagnostic orchestration - #5910
Conversation
Signed-off-by: Carlos Villela <cvillela@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:
📝 WalkthroughWalkthroughSplits sandbox doctor reporting, messaging, and system checks into dedicated modules, then rewires ChangesDoctor Report Extraction and Diagnostics Refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in the Show a code coverage summary of the most covered files.
TypeScript / code-coverage/cliThe overall coverage in the Show a code coverage summary of the most covered files.
Updated |
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. |
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Vitest E2E Scenario RecommendationRequired Vitest E2E scenarios: Dispatch required Vitest E2E scenarios:
Full Vitest E2E advisor summaryVitest E2E Scenario AdvisorBase: Required Vitest E2E scenarios
Optional Vitest E2E scenarios
Relevant changed files
|
PR Review Advisor (Nemotron Ultra) — InformationalMerge posture: Informational / low confidence Action checklist
Findings index
Review findings by urgency: 0 required fixes, 1 item to resolve/justify, 0 in-scope improvements
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/lib/actions/sandbox/doctor.ts (1)
768-770: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGate the in-sandbox inference gateway probe on sandbox reachability.
probeSandboxInferenceGatewayHealthexecutes inside the sandbox, butcollectInferenceChecksruns it for local providers even afterliveSandboxCheckhas marked the sandbox unreachable. Passsandbox.reachableinto inference collection and skip this subprobe when the sandbox is not ready.Suggested direction
async function collectInferenceChecks( sandboxName: string, route: InferenceRoute, + sandboxReachable: boolean, ): Promise<DoctorCheck[]> { @@ - if (isLocalInferenceProvider(route.provider)) { + if (sandboxReachable && isLocalInferenceProvider(route.provider)) { const gateway = await probeSandboxInferenceGatewayHealth(sandboxName);- ...(await collectInferenceChecks(sandboxName, route)), + ...(await collectInferenceChecks(sandboxName, route, sandbox.reachable)),Also applies to: 902-908
🤖 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/doctor.ts` around lines 768 - 770, Gate the in-sandbox inference gateway probe on sandbox reachability: `collectInferenceChecks` currently calls `probeSandboxInferenceGatewayHealth` for local providers even when `liveSandboxCheck` has already determined the sandbox is unreachable. Pass `sandbox.reachable` into `collectInferenceChecks` (and any related inference-check helper) and skip the `probeSandboxInferenceGatewayHealth` path when the sandbox is not reachable, while keeping the existing local-provider logic intact.
🤖 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.
Inline comments:
In `@src/lib/actions/sandbox/doctor-report.ts`:
- Around line 17-24: `DoctorReport.status` is inconsistent with
`buildDoctorReport()` and `renderSummary()`, because the exported type still
permits `"info"` even though it is not produced and is rendered as a generic
fallback. Update the `DoctorReport` type and any related `DoctorStatus` usage in
`doctor-report` so the status contract matches actual behavior, either by
narrowing it to `"ok" | "warn" | "fail"` or by adding explicit `"info"` handling
in `renderSummary()` and the report-building path.
In `@src/lib/actions/sandbox/doctor.ts`:
- Around line 608-610: The gateway probe in collectDoctorChecks still allows
recovery behavior for doctor --json because intent.asJson is dropped before
calling probeOpenShellGateway. Update collectDoctorChecks and
probeOpenShellGateway to accept and forward a read-only/no-recovery flag based
on the JSON intent, and ensure recoverNamedGatewayRuntime is not invoked for
JSON runs so gateway probing remains non-destructive.
- Around line 107-113: The gateway port validation in gatewayPortCheck is using
the process-global GATEWAY_PORT instead of the sandbox’s persisted gateway port,
which can cause false mismatches. Update the Docker port check to use the
sandbox binding-derived port from the same source used to resolve gatewayName,
and keep the expected host port hint aligned with that persisted value. Also
remove the duplicate source of truth in the related occurrences referenced by
the same check so all gateway validation paths compare against the
sandbox-specific port consistently.
---
Nitpick comments:
In `@src/lib/actions/sandbox/doctor.ts`:
- Around line 768-770: Gate the in-sandbox inference gateway probe on sandbox
reachability: `collectInferenceChecks` currently calls
`probeSandboxInferenceGatewayHealth` for local providers even when
`liveSandboxCheck` has already determined the sandbox is unreachable. Pass
`sandbox.reachable` into `collectInferenceChecks` (and any related
inference-check helper) and skip the `probeSandboxInferenceGatewayHealth` path
when the sandbox is not reachable, while keeping the existing local-provider
logic intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 979b5bd2-82ef-4e04-85e5-c15a2e9b8e4e
📒 Files selected for processing (3)
biome.jsonsrc/lib/actions/sandbox/doctor-report.tssrc/lib/actions/sandbox/doctor.ts
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/lib/actions/sandbox/doctor-flow.test.ts (1)
244-246: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert doctor output, not helper wiring.
These cases are pinned to
recoverNamedGatewayRuntime,captureOpenshell, andbuildToolScopeCheckscall patterns instead of the observablerunSandboxDoctorresult. That makes the tests brittle across equivalent refactors and weakens the migration proof. Prefer asserting the emitted checks / JSON report (for example, no live-sandbox or tool-scope entries, and no repair result) rather than whether private helpers were called or which boolean was passed to them. As per path instructions, "Prefer observable outcomes through the public boundary over source-text, private-shape, or mock-call assertions."Also applies to: 259-260, 286-291, 308-308
🤖 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/doctor-flow.test.ts` around lines 244 - 246, The doctor-flow tests are asserting private helper wiring instead of the observable `runSandboxDoctor` output, making them brittle. Update the affected cases in `doctor-flow.test.ts` to verify the emitted checks/JSON report from `runSandboxDoctor` (for example, absence of live-sandbox or tool-scope entries and no repair result) rather than `recoverNamedGatewayRuntime`, `captureOpenShell`, or `buildToolScopeChecks` spy calls or their boolean arguments. Use the public result shape as the assertion target so refactors of internal helpers do not break the tests.Source: Path instructions
src/lib/actions/sandbox/doctor-system-checks.ts (1)
190-198: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBound this legacy gateway fallback with an explicit retirement plan.
This preserves the recorded-driver path and the older platform-detection fallback, but the migration guidance requires retained compatibility paths to name the GitHub retirement work and the observable condition for removing them.
As per path instructions, “Retain an old path only for a demonstrated external/persisted-data contract or a bounded confidence/rollback window… link the retirement issue or PR in GitHub, and state observable exit criteria.”
🤖 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/doctor-system-checks.ts` around lines 190 - 198, Update shouldInspectLegacyGatewayContainer to keep the legacy platform-detection fallback, but document it with an explicit retirement reference and exit criteria. Add a comment near the recorded-driver logic and the isLinuxDockerDriverGatewayEnabled fallback that names the GitHub issue/PR tracking removal of the legacy gateway path and states the observable condition for deleting it. Preserve the existing behavior for SandboxEntry.openshellDriver while making the legacy path clearly time-bounded and tied to the retirement work.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.
Inline comments:
In `@src/lib/actions/sandbox/doctor-messaging.ts`:
- Around line 100-119: channelRuntimeDoctorCheck currently re-reads the sandbox
from registry and swallows registry/loadAgent failures by returning null, which
hides broken agent/config states. Update collectMessagingDoctorChecks and
channelRuntimeDoctorCheck to use the already-available SandboxEntry sb as the
source of truth, and when sandbox resolution or loadAgent fails, return a
warning DoctorCheck instead of omitting the diagnostic. Keep the runtime probe
flow intact for valid agents, and reference channelRuntimeDoctorCheck,
collectMessagingDoctorChecks, and loadAgent when making the change.
---
Nitpick comments:
In `@src/lib/actions/sandbox/doctor-flow.test.ts`:
- Around line 244-246: The doctor-flow tests are asserting private helper wiring
instead of the observable `runSandboxDoctor` output, making them brittle. Update
the affected cases in `doctor-flow.test.ts` to verify the emitted checks/JSON
report from `runSandboxDoctor` (for example, absence of live-sandbox or
tool-scope entries and no repair result) rather than
`recoverNamedGatewayRuntime`, `captureOpenShell`, or `buildToolScopeChecks` spy
calls or their boolean arguments. Use the public result shape as the assertion
target so refactors of internal helpers do not break the tests.
In `@src/lib/actions/sandbox/doctor-system-checks.ts`:
- Around line 190-198: Update shouldInspectLegacyGatewayContainer to keep the
legacy platform-detection fallback, but document it with an explicit retirement
reference and exit criteria. Add a comment near the recorded-driver logic and
the isLinuxDockerDriverGatewayEnabled fallback that names the GitHub issue/PR
tracking removal of the legacy gateway path and states the observable condition
for deleting it. Preserve the existing behavior for SandboxEntry.openshellDriver
while making the legacy path clearly time-bounded and tied to the retirement
work.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ea65b6aa-e09e-4f54-9e2e-edcae770e608
📒 Files selected for processing (6)
biome.jsonsrc/lib/actions/sandbox/doctor-flow.test.tssrc/lib/actions/sandbox/doctor-messaging.tssrc/lib/actions/sandbox/doctor-report.test.tssrc/lib/actions/sandbox/doctor-system-checks.tssrc/lib/actions/sandbox/doctor.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/actions/sandbox/doctor.ts
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@src/lib/actions/sandbox/doctor-system-checks.test.ts`:
- Around line 20-26: The test for doctor system checks is using branching logic
inside the subprocess mock, which lets the test decide behavior instead of
verifying it linearly. Update the mock in doctor-system-checks.test.ts to return
the two expected subprocess results in a fixed order for the relevant calls, and
remove the conditional handling from the mockImplementation. Keep the assertions
tied to the existing doctor-system-checks flow so the test stays a straight
behavior check without internal branching.
- Line 13: The test cleanup in doctor-system-checks.test.ts is using the
unavailable global require, which can throw inside afterEach and break later
tests. Update the cache invalidation logic to use the existing
createRequire-based requireDist reference instead of require, and keep the cache
deletion scoped to the modulePath resolved via requireDist so the test teardown
remains safe.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: aac754dc-fcae-4c21-a238-a4235ae43397
📒 Files selected for processing (5)
src/lib/actions/sandbox/doctor-flow.test.tssrc/lib/actions/sandbox/doctor-report.tssrc/lib/actions/sandbox/doctor-system-checks.test.tssrc/lib/actions/sandbox/doctor-system-checks.tssrc/lib/actions/sandbox/doctor.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/lib/actions/sandbox/doctor-report.ts
- src/lib/actions/sandbox/doctor-flow.test.ts
- src/lib/actions/sandbox/doctor-system-checks.ts
- src/lib/actions/sandbox/doctor.ts
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
|
Automated-review follow-up for
|
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
|
Resolved PRA-1 and PRA-2 in 6424638. The invalid state was a disconnected named gateway or non-ready sandbox followed by sandbox-exec inference and messaging probes. The authoritative boundary is the gateway lifecycle plus live sandbox-list result collected once by collectDoctorChecks; that reachability value is now threaded into both collectors. Registry-only diagnostics still run, while live probes render explicit skipped checks. The flow regression configures ollama-local plus enabled Telegram behind a disconnected named gateway and proves neither sandbox-exec path runs. This is a permanent orchestration invariant rather than a compatibility workaround, so there is no later removal condition. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
|
Resolved GPT advisor PRA-T1 in 0f67897. The new public-flow regression starts plain doctor with a missing named gateway, returns a healthy recovered gateway, and verifies sandbox discovery plus inference, messaging runtime, and tool-scope probes run only after recovery. |
|
Final advisor follow-ups: the positive and negative reachability contracts are covered at the exported runSandboxDoctor flow boundary, including recovery ordering, JSON non-recovery, and proof that sandbox-exec probes stay gated. A live fault-injection test that deliberately removes and recovers a named gateway would expand this refactor into environment-reliability E2E work; the existing process scenarios plus the full self-hosted sandbox, gateway-isolation, port-override, and non-root matrix are green, so PRA-T1/PRA-T2 are justified as unnecessary for this PR. Nemotron reported its own advisor analysis unavailable, but GPT-5.5 completed on the same final head with merge_as_is, CodeRabbit completed, and the manual review plus all CI gates are green. |
<!-- markdownlint-disable MD041 --> ## Summary Replaces the 114-complexity sandbox doctor command with a narrow orchestration layer and cohesive reporting, messaging, and system-diagnostic modules. `doctor.ts` shrinks from 884 lines to 531, while a file-scoped Biome ratchet caps every function in the doctor surface at cognitive complexity 10. ## Changes - isolate doctor report aggregation, text rendering, and JSON rendering in `doctor-report.ts` - extract messaging/runtime-channel diagnostics and host/gateway/local-service diagnostics into focused modules - express host, gateway, sandbox, inference, registered-sandbox, tool-scope, and local-service execution as ordered collectors - keep `doctor --json` read-only by probing gateway state without invoking recovery - keep registry diagnostics available offline while gating inference-gateway and messaging-runtime sandbox probes on collected reachability - validate legacy gateway port mappings against each sandbox's persisted gateway binding - add negative tests for missing OpenShell, disconnected gateways, read-only diagnostics, and non-OpenClaw tool-scope gating - verify report aggregation/rendering and ensure local inference diagnostics do not mutate provider-health results - enforce a cognitive-complexity ceiling of 10 across all doctor modules ## Type of Change - [x] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates - [x] Tests added or updated for changed behavior - [x] Existing tests cover changed behavior — justification: the existing doctor helper suites and 12 CLI process scenarios continue to cover end-to-end diagnostics; new focused tests cover the extracted boundaries, report contract, persisted-port matching, and negative orchestration gates. - [ ] Tests not applicable — justification: - [ ] Docs updated for user-facing behavior changes - [x] Docs not applicable — justification: the command reference already defines `--json` as report-only and mutually exclusive with `--fix`; this change makes the implementation honor that existing contract. - [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: self-review confirmed diagnostic ordering, repair/recovery gating, process exits, and output contracts; the full repository hook and coverage suite passes. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Verification - [x] PR description includes the DCO sign-off declaration and every commit appears as `Verified` in GitHub - [x] Git hooks passed during commit and push, or `npx prek run --from-ref main --to-ref HEAD` passes - [x] Targeted tests pass for changed behavior - [ ] Full `npm test` passes (broad runtime changes only) - [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) - [ ] 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) --- Signed-off-by: Carlos Villela <cvillela@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Sandbox “doctor” diagnostics now generate a structured, grouped report with colored status labels, optional per-check hints, and a JSON output mode. * Added dedicated diagnostics for messaging channels/runtime registry, gateway/container health, local tunnel/service state, and Ollama reachability. * **Bug Fixes** * Improved gating so recovery/repairs and certain probes run only when prerequisites are met; JSON runs remain read-only. * Messaging runtime registry failures now surface as warnings without hiding related messaging checks. * **Tests** * Expanded coverage for report formatting, system checks, and doctor flow gating scenarios. * **Chores** * Tightened complexity linting for sandbox doctor action modules. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Carlos Villela <cvillela@nvidia.com>
wscurran
added
the
area: cli
wscurran
added
area: sandbox
2 participants
Summary
Replaces the 114-complexity sandbox doctor command with a narrow orchestration layer and cohesive reporting, messaging, and system-diagnostic modules.
doctor.tsshrinks from 884 lines to 531, while a file-scoped Biome ratchet caps every function in the doctor surface at cognitive complexity 10.Changes
doctor-report.tsdoctor --jsonread-only by probing gateway state without invoking recoveryType of Change
Quality Gates
--jsonas report-only and mutually exclusive with--fix; this change makes the implementation honor that existing contract.Verification
Verifiedin GitHubnpx prek run --from-ref main --to-ref HEADpassesnpm testpasses (broad runtime changes only)npm run docsbuilds without warnings (doc changes only)Signed-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit