fix(onboard): probe registered extra providers exactly - #6596
Conversation
Replace the first-page snapshot with bounded, gateway-scoped per-provider probes. Remove only exact absence; preserve indeterminate results and local registry state. Co-authored-by: Shawn Xie <shaxie@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
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 |
|
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:
📝 WalkthroughWalkthroughExtra-provider reconciliation now probes each recorded provider, removes only exact not-found matches, preserves indeterminate results, and bounds diagnostics and probe time. Onboarding resume flows accept explicit sandbox recreation, while unit, integration, and live tests cover stale-provider pruning and filtered sandbox creation. ChangesExtra-provider reconciliation
Sandbox recreation and stale-provider filtering
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Onboard
participant Reconciliation
participant OpenShell
participant Gateway
Onboard->>Reconciliation: reconcile recorded providers
Reconciliation->>OpenShell: provider get for each name
OpenShell->>Gateway: query provider status
Gateway-->>OpenShell: status and diagnostics
OpenShell-->>Reconciliation: probe result
Reconciliation-->>Onboard: filtered provider names
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
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
|
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
E2E Target Results —
|
| Job | Result |
|---|---|
| cloud-onboard | |
| onboard-repair | |
| onboard-resume |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/onboard-extra-provider-reconciliation.test.ts (1)
60-67: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueVestigial
provider listmock branch.Lines 66-67 still stub
provider list -g nemoclaw --names, but the assertion at Lines 170-175 asserts this command is never invoked. This branch is now dead code in this test — harmless, but worth pruning or adding a comment explaining it's a defensive fallback in case of unexpected calls.🤖 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 `@test/onboard-extra-provider-reconciliation.test.ts` around lines 60 - 67, The test has a dead mock branch for provider list -g nemoclaw --names that is no longer exercised because the existing assertions in the same test expect that command to never run. Remove the obsolete branch from the test setup, or if you want to keep it as a safety net, add a short comment near the mocked command handling in onboard-extra-provider-reconciliation.test.ts explaining that it is a defensive fallback and not part of the expected flow.
🤖 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.
Nitpick comments:
In `@test/onboard-extra-provider-reconciliation.test.ts`:
- Around line 60-67: The test has a dead mock branch for provider list -g
nemoclaw --names that is no longer exercised because the existing assertions in
the same test expect that command to never run. Remove the obsolete branch from
the test setup, or if you want to keep it as a safety net, add a short comment
near the mocked command handling in
onboard-extra-provider-reconciliation.test.ts explaining that it is a defensive
fallback and not part of the expected flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b8cb7512-d4a7-42e8-9aef-2663f8fc5d53
📒 Files selected for processing (3)
src/lib/onboard/extra-provider-reconciliation.test.tssrc/lib/onboard/extra-provider-reconciliation.tstest/onboard-extra-provider-reconciliation.test.ts
Co-authored-by: Shawn Xie <shaxie@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
E2E Target Results —
|
| Job | Result |
|---|---|
| cloud-onboard | |
| onboard-repair | |
| onboard-resume |
E2E Target Results — ✅ All requested jobs passedRun: 29046258405
|
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. |
PR Review Advisor (Nemotron Ultra) — Changes requestedMerge posture: Do not merge yet Action checklist
Findings index
Review findings by urgency: 0 required fixes, 2 items to resolve/justify, 0 in-scope improvements
|
Co-authored-by: Shawn Xie <shaxie@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Co-authored-by: Shawn Xie <shaxie@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
E2E Target Results — ✅ All requested jobs passedRun: 29047798544
|
E2E Target Results — ✅ All requested jobs passedRun: 29048610487
|
Co-authored-by: Shawn Xie <shaxie@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
Advisor follow-up for
The commit is signed, GitHub-verified, and retains Shawn Xie as co-author. |
E2E Target Results —
|
| Job | Result |
|---|---|
| cloud-onboard | |
| onboard-repair | |
| onboard-resume |
|
Exact-head maintainer sweep (cb7977f): keeping this held for the two substantive gaps already identified by the advisors: split/table-drive the 94→412-line reconciliation test body, and add checked-in stale-plus-live extra-provider resume/repair convergence coverage. The required exact-head live run was cancelled; the prior green run was on ancestor a0ea6e6, so please rerun onboard-repair, onboard-resume, and cloud-onboard after the fixes, together with both advisors and CodeRabbit. One disposition to avoid redundant work: Nemotron PRA-3 (missing retry idempotency) appears already covered by test/onboard-extra-provider-reconciliation.test.ts, which performs two create attempts and asserts the filtered provider arguments remain identical. The structure and lifecycle-coverage findings remain blocking. |
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
E2E Target Results — ❌ Some jobs failedRun: 29114463308
|
E2E Target Results — ❌ Some jobs failedRun: 29115029731
|
E2E Target Results —
|
| Job | Result |
|---|---|
| agent-turn-latency | |
| bedrock-runtime-compatible-anthropic | |
| brave-search | |
| channels-add-remove | |
| channels-stop-start | |
| cloud-inference | |
| cloud-onboard | |
| common-egress-agent | |
| concurrent-gateway-ports | |
| credential-migration | |
| credential-sanitization | |
| cron-preflight-inference-local | |
| device-auth-health | |
| diagnostics | |
| docs-validation | |
| double-onboard | |
| full-e2e | |
| gateway-drift-preflight | |
| gateway-guard-recovery | |
| gateway-health-honest | |
| gpu-double-onboard | |
| gpu-e2e | |
| hermes-dashboard | |
| hermes-discord | |
| hermes-e2e | |
| hermes-gpu-startup | |
| hermes-inference-switch | |
| hermes-shields-config | |
| hermes-slack | |
| inference-routing | |
| issue-2478-crash-loop-recovery | |
| issue-4434-tui-unreachable-inference | |
| issue-4462-scope-upgrade-approval | |
| jetson-nvmap-gpu | |
| kimi-inference-compat | |
| launchable-smoke | |
| live | |
| mcp-bridge | |
| mcp-bridge-dev | |
| messaging-compatible-endpoint | |
| messaging-providers | |
| model-router-provider-routed-inference | |
| network-policy | |
| ollama-auth-proxy | |
| onboard-negative-paths | |
| onboard-repair | |
| onboard-resume | |
| openclaw-discord-pairing | |
| openclaw-inference-switch | |
| openclaw-plugin-runtime-exdev | |
| openclaw-skill-cli | |
| openclaw-slack-pairing | |
| openclaw-tui-chat-correlation | |
| openshell-gateway-auth-contract | |
| openshell-gateway-upgrade | |
| openshell-version-pin | |
| overlayfs-autofix | |
| rebuild-hermes | |
| rebuild-hermes-stale-base | |
| rebuild-openclaw | |
| sandbox-operations | |
| sandbox-rebuild | |
| sandbox-rlimits-connect | |
| sandbox-survival | |
| security-posture | |
| sessions-agents-cli | |
| shields-config | |
| skill-agent | |
| snapshot-commands | |
| spark-install | |
| state-backup-restore | |
| telegram-injection | |
| token-rotation | |
| tunnel-lifecycle | |
| upgrade-stale-sandbox |
E2E Target Results — ❌ Some jobs failedRun: 29115969006
|
E2E Target Results — ✅ All requested jobs passedRun: 29116257938
|
E2E Target Results — ✅ All requested jobs passedRun: 29116735646
|
E2E Target Results — ✅ All requested jobs passedRun: 29117568248
|
E2E Target Results — ✅ All requested jobs passedRun: 29118167293
|
E2E Target Results — ✅ All requested jobs passedRun: 29119108431
|
E2E Target Results — ✅ All requested jobs passedRun: 29119308633
|
|
Exact-head follow-up for
One audit-record blocker remains: the PR description still says reconciliation “never mutate[s] the local registry,” is intentionally non-destructive, and that The branch is also two commits behind |
## 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>
<!-- markdownlint-disable MD041 --> ## Summary Fixes the post-merge NVIDIA#6587 gap where one paginated provider-list page could omit healthy registry extras beyond entry 100. Reconciliation now uses bounded, gateway-scoped per-provider probes, recognizes the exact wrapped OpenShell NVIDIA#6501 diagnostic, and preserves every indeterminate result. ## Related Issue Follow-up to NVIDIA#6501 and NVIDIA#6587. Supersedes NVIDIA#6518. ## Changes - Probe every recorded extra with argv-safe `provider get -g <gateway> <name>` instead of treating one list page as authoritative. - Omit only exit-1 diagnostics that bind the exact, case-sensitive quoted provider name to a tightly anchored not-found shape, including the wrapped not-found-and-unrecognized form reported in NVIDIA#6501. - Fail open on gateway, transport, authentication, timeout, signal, ambiguous, name-mismatched, or command-spoofed output. - Read canonical stderr/stdout without duplicating Node's composite `output` array, and treat process errors or capture-limit diagnostics as indeterminate. - Bound each probe to 5 seconds and 64 KiB, cap total reconciliation time at 15 seconds, and never mutate the local registry. - Cover stale providers after index 100, the literal wrapped issue diagnostic in a realistic `spawnSync` result, capture truncation, mixed-case names, multi-line diagnostic spoofing, gateway failures, aggregate timeout, and stable repeated sandbox-create provider arguments. The create-time filter is intentionally non-destructive. A provider omitted from one sandbox create remains in the user-owned registry for a later retry; `--fresh` does not purge it. Indeterminate probes preserve the attachment, and the final `sandbox create` remains authoritative and reports any concrete attachment failure. This avoids deleting healthy user configuration because of a transient gateway or transport failure. ## 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 <!-- 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: - [ ] Docs updated for user-facing behavior changes - [x] Docs not applicable — justification: internal provider-state reconciliation only; no CLI, prompt, configuration, policy, or required user action changes. - [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: independent nine-category security review passed after exact-name, whole-diagnostic, subprocess-bound, and fail-open checks. - [ ] 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: CLI unit 14/14 and spawn integration 1/1 passed; the spawn test uses a realistic composite process result, executes two create attempts, and proves identical filtered provider arguments. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: not applicable to this focused reconciliation change; `npm run typecheck:cli`, `npm run check:diff`, source-shape, test-size, and test-conditional scans passed. - [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) Exact-head live validation for `onboard-repair`, `onboard-resume`, and `cloud-onboard` passed at [workflow run 29048610487](https://github.com/NVIDIA/NemoClaw/actions/runs/29048610487) for head `a0ea6e60f98c6e15a6084dba7a5ba9ff1e89215a`. --- <!-- 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: Apurv Kumaria <akumaria@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Onboarding and repair now detect and remove stale extra-provider records. * Resume operations can explicitly recreate an existing sandbox while preserving its registry entry. * Sandbox creation and retries now use a consistent, deduplicated provider list. * **Bug Fixes** * Providers are retained when availability checks are inconclusive, preventing accidental removal. * Reconciliation now avoids relying on potentially stale provider-list snapshots and handles diagnostic variations more reliably. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Apurv Kumaria <akumaria@nvidia.com> Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Signed-off-by: Carlos Villela <cvillela@nvidia.com> Co-authored-by: Shawn Xie <shaxie@nvidia.com> Co-authored-by: Prekshi Vyas <prekshiv@nvidia.com> Co-authored-by: Carlos Villela <cvillela@nvidia.com>
## 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>
Summary
Fixes the post-merge #6587 gap where one paginated provider-list page could omit healthy registry extras beyond entry 100. Reconciliation now uses bounded, gateway-scoped per-provider probes, recognizes the exact wrapped OpenShell #6501 diagnostic, and preserves every indeterminate result.
Related Issue
Follow-up to #6501 and #6587. Supersedes #6518.
Changes
provider get -g <gateway> <name>instead of treating one list page as authoritative.outputarray, and treat process errors or capture-limit diagnostics as indeterminate.spawnSyncresult, capture truncation, mixed-case names, multi-line diagnostic spoofing, gateway failures, aggregate timeout, and stable repeated sandbox-create provider arguments.The create-time filter is intentionally non-destructive. A provider omitted from one sandbox create remains in the user-owned registry for a later retry;
--freshdoes not purge it. Indeterminate probes preserve the attachment, and the finalsandbox createremains authoritative and reports any concrete attachment failure. This avoids deleting healthy user configuration because of a transient gateway or transport failure.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: not applicable to this focused reconciliation change;npm run typecheck:cli,npm run check:diff, source-shape, test-size, and test-conditional scans passed.npm run docsbuilds without warnings (doc changes only)Exact-head live validation for
onboard-repair,onboard-resume, andcloud-onboardpassed at workflow run 29048610487 for heada0ea6e60f98c6e15a6084dba7a5ba9ff1e89215a.Signed-off-by: Apurv Kumaria akumaria@nvidia.com
Summary by CodeRabbit
New Features
Bug Fixes