fix(onboard): prune dangling extra providers before sandbox create - #6518
Closed
nvshaxie wants to merge 4 commits into
Closed
fix(onboard): prune dangling extra providers before sandbox create#6518nvshaxie wants to merge 4 commits into
nvshaxie wants to merge 4 commits into
Conversation
Onboarding fails with "provider 'tavily-search' not found" when the host registry still records an extra provider that no longer exists on the reused gateway, even though the user declined web search (#6501). Reconcile the recorded set against the gateway's provider list at plan time: names the gateway confirms are kept, provably dangling records are skipped, warned about, and removed; if the list is unreadable the recorded set passes through unchanged so a real gateway outage still surfaces through the existing create diagnostics. Fixes #6501 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Shawn Xie <shaxie@nvidia.com>
Contributor
📝 WalkthroughWalkthroughSandbox creation now reconciles recorded extra providers against the gateway before building the create plan. Stale provider records are pruned on provider-not-found diagnostics, and the helper is re-exported through cleanup plumbing. Tests cover the new reconciliation and onboarding path. ChangesProvider reconciliation
Estimated code review effort: 3 (Moderate) | ~25 minutes 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
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
|
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 — No blocking findingsMerge posture: No blocking advisor findings 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. |
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 |
Merged
21 tasks
…ng-extra-providers
The initial reconcile queried 'provider list --names' and treated any exit-0 response as an authoritative snapshot, which pruned legitimately configured providers whenever a stub or degraded gateway answered success with an empty list — cli-test-shards (5) caught exactly that against the always-OK openshell test fixture. Probe each recorded name with 'provider get -g <gateway> <name>' instead (the same existence check upsertProvider relies on), prune only on an explicit provider-not-found diagnostic, and fail open on any other failure so a gateway outage can never silently drop a healthy provider. Also adds a spawn-level onboard integration test covering the prune path end-to-end: a dangling tavily-search record is skipped, warned about, and removed while sandbox create proceeds without the flag. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Shawn Xie <shaxie@nvidia.com>
… module PR Review Advisor flagged sandbox-provider-cleanup.ts monolith growth (PRA-2, required): reconciliation is a fourth responsibility next to detach, pre-delete cleanup, and delete recovery. Move reconcileRegisteredExtraProviders to src/lib/onboard/extra-provider-reconciliation.ts with a re-export from the cleanup module so the onboard entrypoint import stays net-zero; the cleanup module shrinks below its pre-PR line count. Also folds in the advisor's hardening asks: SOURCE_OF_TRUTH_REVIEW block documenting the drift's origin and removal condition (PRA-6), a debug note when the fail-open path keeps a record after a non-not-found probe failure (PRA-4), and a forget() guard so a registry write failure cannot abort onboarding (PRA-5). Unit tests move to test/extra-provider-reconciliation.test.ts alongside the module. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Shawn Xie <shaxie@nvidia.com>
Contributor
Author
|
Pushed two follow-up commits addressing the shard-5 CI failure and the PR Review Advisor findings. CI failure ( Advisor findings —
Verification: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/onboard/sandbox-provider-cleanup.ts (1)
401-417: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDiagnostics are console-only; consider surfacing as a state result.
warndefaults toconsole.warn, so the pruning outcome is only observable via stdout/stderr. Persrc/lib/onboard/machine/README.md, the target architecture "Prefer[s] explicit state results returned by handlers and applied byOnboardRuntime... rather than relying on console output as the only observable diagnostic." If this helper is invoked from within onboarding state-handler code (rather than purely ad-hoc CLI plumbing), consider returning pruned/kept provider names as part of a state result soOnboardRuntimecan persist/redact them, instead of console output being the sole record.🤖 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/onboard/sandbox-provider-cleanup.ts` around lines 401 - 417, The provider cleanup in the sandbox helper is currently only observable through warn/console output, which does not match the explicit-state-result pattern used by onboarding handlers. Update the logic around the runOpenshell check, warn, and forgetExtraProvider flow so the pruning decision is returned as structured state (for example, pruned vs kept provider names) that OnboardRuntime can consume and persist/redact, instead of relying on console.warn as the only diagnostic channel.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.
Nitpick comments:
In `@src/lib/onboard/sandbox-provider-cleanup.ts`:
- Around line 401-417: The provider cleanup in the sandbox helper is currently
only observable through warn/console output, which does not match the
explicit-state-result pattern used by onboarding handlers. Update the logic
around the runOpenshell check, warn, and forgetExtraProvider flow so the pruning
decision is returned as structured state (for example, pruned vs kept provider
names) that OnboardRuntime can consume and persist/redact, instead of relying on
console.warn as the only diagnostic channel.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f616200d-844b-47dd-90a6-86b5ff277a67
📒 Files selected for processing (4)
src/lib/onboard.tssrc/lib/onboard/sandbox-provider-cleanup.tstest/onboard-extra-provider-prune.test.tstest/sandbox-provider-cleanup.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/onboard.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/lib/onboard/extra-provider-reconciliation.ts (1)
115-120: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSilent swallow on
forget()failure — consider a debug note.Other fail-open branches in this file log via
console.debugfor diagnosability (lines 106-109), but a registry write failure here is swallowed with only a code comment. Not blocking, but adding a similar debug line would keep the diagnosability story consistent.♻️ Optional
try { forget(name); - } catch { + } catch (err) { // A registry write failure must not abort onboarding — the dangling // record is still skipped for this run and re-pruned on the next one. + console.debug( + `reconcileRegisteredExtraProviders: failed to forget stale provider '${name}': ${err}`, + ); }🤖 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/onboard/extra-provider-reconciliation.ts` around lines 115 - 120, Add a non-blocking debug log in the `forget(name)` failure branch inside `extra-provider-reconciliation` so this fail-open path is diagnosable like the nearby `console.debug` handling; update the `try/catch` around `forget(name)` to capture the thrown error and emit a concise `console.debug` message with the provider name and error details, while keeping onboarding uninterrupted.test/extra-provider-reconciliation.test.ts (1)
24-132: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMissing test for the
forget()-throws hardening.The commit description calls out a new guard so that a registry write failure inside
forget()doesn't abort onboarding (production lines 115-120), but no test here exercises that path. Given this was explicitly added as hardening for a real failure mode, a regression test would lock in the guarantee.✅ Suggested addition
it("keeps onboarding running when forgetting a stale provider throws (registry write failure)", () => { const responses = new Map<string, RunResult>([ ["provider get tavily-search", { status: 1, stderr: "provider 'tavily-search' not found\n" }], ]); const { runOpenshell } = buildRunOpenshell(responses); const forget = vi.fn(() => { throw new Error("disk full"); }); const warn = vi.fn(); const result = reconcileRegisteredExtraProviders({ runOpenshell, listExtraProviders: () => ["tavily-search"], forgetExtraProvider: forget, warn, }); expect(result).toEqual([]); expect(forget).toHaveBeenCalledWith("tavily-search"); });Want me to open this as a follow-up or add it directly?
🤖 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/extra-provider-reconciliation.test.ts` around lines 24 - 132, The reconciliation test suite for reconcileRegisteredExtraProviders is missing coverage for the new forget()-throws hardening path. Add a test that stubs forgetExtraProvider to throw during stale-provider cleanup, then verify reconcileRegisteredExtraProviders still returns the pruned result and does not abort onboarding; use the existing buildRunOpenshell, listExtraProviders, warn, and forget setup patterns to exercise the failure mode.
🤖 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/onboard/extra-provider-reconciliation.ts`:
- Around line 20-27: The provider-not-found detection in PROVIDER_NOT_FOUND_RE
is still too broad because the second branch can match gateway-related
diagnostics; tighten the regex in extra-provider-reconciliation so it only
matches when the “provider” subject is the thing reported as not found, or add
an explicit guard to exclude gateway/sandbox diagnostics. Update the matching
logic around PROVIDER_NOT_FOUND_RE and verify the reconciliation flow that uses
it only prunes providers for true provider-not-found errors.
---
Nitpick comments:
In `@src/lib/onboard/extra-provider-reconciliation.ts`:
- Around line 115-120: Add a non-blocking debug log in the `forget(name)`
failure branch inside `extra-provider-reconciliation` so this fail-open path is
diagnosable like the nearby `console.debug` handling; update the `try/catch`
around `forget(name)` to capture the thrown error and emit a concise
`console.debug` message with the provider name and error details, while keeping
onboarding uninterrupted.
In `@test/extra-provider-reconciliation.test.ts`:
- Around line 24-132: The reconciliation test suite for
reconcileRegisteredExtraProviders is missing coverage for the new
forget()-throws hardening path. Add a test that stubs forgetExtraProvider to
throw during stale-provider cleanup, then verify
reconcileRegisteredExtraProviders still returns the pruned result and does not
abort onboarding; use the existing buildRunOpenshell, listExtraProviders, warn,
and forget setup patterns to exercise the failure mode.
🪄 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: cccbb1c7-944b-4333-bd2f-b83f9cf03cb7
📒 Files selected for processing (4)
src/lib/onboard/extra-provider-reconciliation.tssrc/lib/onboard/sandbox-provider-cleanup.tstest/extra-provider-reconciliation.test.tstest/sandbox-provider-cleanup.test.ts
💤 Files with no reviewable changes (1)
- test/sandbox-provider-cleanup.test.ts
Comment on lines
+20
to
+27
| /** | ||
| * Diagnostic shapes for "the probed provider does not exist": both the CLI's | ||
| * `provider 'X' not found` and the gRPC-style `NotFound: provider "X"` | ||
| * orderings. Anchored to the word "provider" on the same line so missing- | ||
| * sandbox or missing-gateway errors never count as a provider-not-found. | ||
| */ | ||
| const PROVIDER_NOT_FOUND_RE = | ||
| /provider[^\n]{0,200}?(?:\bNotFound\b|\bnot\s+found\b)|(?:\bNotFound\b|\bnot\s+found\b)(?::|\s)[^\n]{0,200}?\bprovider\b/i; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file and nearby tests/usages first.
git ls-files 'src/lib/onboard/extra-provider-reconciliation.ts' \
'src/lib/onboard/**' \
'src/**/__tests__/**' \
'src/**/test/**' \
'test/**' | sed -n '1,200p'
echo
echo "== outline =="
ast-grep outline src/lib/onboard/extra-provider-reconciliation.ts --view expanded || true
echo
echo "== search for regex/tests/usages =="
rg -n "PROVIDER_NOT_FOUND_RE|provider not found|NotFound: provider|not found.*provider|gateway|sandbox" src/lib/onboard src test -S || true
echo
echo "== file excerpt =="
wc -l src/lib/onboard/extra-provider-reconciliation.ts
sed -n '1,220p' src/lib/onboard/extra-provider-reconciliation.tsRepository: NVIDIA/NemoClaw
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== extra-provider-reconciliation.ts =="
sed -n '1,220p' src/lib/onboard/extra-provider-reconciliation.ts
echo
echo "== nearby tests/usages =="
rg -n "PROVIDER_NOT_FOUND_RE|reconcileRegisteredExtraProviders|forget\\(|provider-not-found|provider not found|NotFound: provider|not found.*provider" \
src/lib/onboard src/lib -g '!**/dist/**' -g '!**/build/**' -S
echo
echo "== regex behavior probe =="
python3 - <<'PY'
import re
pattern = re.compile(r'provider[^\n]{0,200}?(?:\bNotFound\b|\bnot\s+found\b)|(?:\bNotFound\b|\bnot\s+found\b)(?::|\s)[^\n]{0,200}?\bprovider\b', re.I)
samples = [
'provider "alpha" not found',
'NotFound: provider "alpha"',
'gateway "nemoclaw" not found while listing provider records',
'sandbox "foo" not found; provider "alpha" is cached',
'missing-gateway: provider "alpha" not found',
'NotFound: gateway "nemoclaw" not found while listing provider records',
'provider "alpha" NotFound',
]
for s in samples:
print(f"{pattern.search(s) is not None!s:5} {s}")
PYRepository: NVIDIA/NemoClaw
Length of output: 9941
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== extra-provider reconciliation tests =="
fd -a 'extra-provider-reconciliation.test.ts|onboard-extra-provider-prune.test.ts|rebuild-provider-preflight.test.ts' test src || true
echo
echo "== extra-provider-reconciliation.test.ts =="
sed -n '1,240p' test/extra-provider-reconciliation.test.ts
echo
echo "== onboard-extra-provider-prune.test.ts =="
sed -n '1,260p' test/onboard-extra-provider-prune.test.ts
echo
echo "== gateway / outage related samples in tests =="
rg -n "gateway.*not found|not found.*gateway|provider not found|provider-not-found|fail-open|keep.*provider|skip.*provider|reconcileRegisteredExtraProviders" \
test src/lib -SRepository: NVIDIA/NemoClaw
Length of output: 27725
Tighten the provider-not-found match The second branch still matches diagnostics like NotFound: gateway ... not found while listing provider records, so a gateway outage can prune a healthy provider record. Narrow it to the provider subject itself or add a gateway-down regression case.
🤖 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/onboard/extra-provider-reconciliation.ts` around lines 20 - 27, The
provider-not-found detection in PROVIDER_NOT_FOUND_RE is still too broad because
the second branch can match gateway-related diagnostics; tighten the regex in
extra-provider-reconciliation so it only matches when the “provider” subject is
the thing reported as not found, or add an explicit guard to exclude
gateway/sandbox diagnostics. Update the matching logic around
PROVIDER_NOT_FOUND_RE and verify the reconciliation flow that uses it only
prunes providers for true provider-not-found errors.
This was referenced Jul 9, 2026
cv
pushed a commit
that referenced
this pull request
Jul 9, 2026
<!-- markdownlint-disable MD041 --> ## Summary Reconcile registry-recorded extra providers against the authoritative OpenShell gateway list before sandbox creation. This prevents a stale `tavily-search` record from breaking onboarding while preserving every healthy gateway-wide user provider. The PR replaces #6531, preserves Ho Lim's original issue work, and credits Shawn Xie's provider-reconciliation direction from #6518. ## Related Issue Fixes #6501. ## Changes - Query `provider list -g <gateway> --names` once when registry extras exist. - Attach only exact provider names returned by a successful authoritative list. - Preserve all recorded providers if the gateway query fails or throws, avoiding silent user-state loss during an outage. - Keep local registry state unchanged; reconciliation affects only the current sandbox-create plan. - Preserve healthy bare `brave-search`, `tavily-search`, custom, and bridge providers without treating their names as NemoClaw-owned. - Add focused and spawn-level regressions for healthy providers, stale records, exact-name matching, gateway scoping, and fail-open behavior. ## 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 - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [ ] Docs updated for user-facing behavior changes - [x] Docs not applicable — justification: this silently restores the documented provider source-of-truth contract without adding or changing a command, option, output, configuration, or remediation step. - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — exact-name gateway reconciliation was independently audited; it avoids diagnostic regexes, preserves healthy user-owned providers, fails open on gateway-list failure, and does not mutate registry state. - [ ] 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 — 17 focused reconciliation tests and all 65 onboard integration tests passed. - [ ] 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) Additional exact-head validation: CLI type-check, file/commit/push hooks, commitlint, test-size/source-shape budgets, secret scan, and `npm run check:diff` passed. Documentation review found no update required. `src/lib/onboard.ts` is net-neutral. --- Signed-off-by: Apurv Kumaria <akumaria@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Sandbox creation now better matches configured providers to what’s actually available, reducing unexpected provider options during onboarding. * **Bug Fixes** * Fixed cases where stale or unavailable extra providers could still appear in sandbox setup. * Improved behavior when provider lookup fails, helping preserve previously configured choices instead of removing them unexpectedly. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Apurv Kumaria <akumaria@nvidia.com> Co-authored-by: Ho Lim <subhoya@gmail.com> Co-authored-by: Shawn Xie <shaxie@nvidia.com>
21 tasks
Collaborator
|
Superseded by #6596. The replacement carries forward Shawn Xie’s per-provider probe design and credits @nvshaxie as co-author, while preserving the non-destructive registry behavior from merged #6587. It also closes the remaining gaps found during comparison: byte-exact provider identity, whole-diagnostic fail-open handling, provider counts beyond 100, bounded aggregate latency, and gateway/transport/authentication regressions. Thank you, Shawn, for the core reconciliation design and tests preserved in the replacement. |
cv
added a commit
that referenced
this pull request
Jul 10, 2026
<!-- markdownlint-disable MD041 --> ## 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 - 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 #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>
Hadar301
pushed a commit
to Hadar301/NemoClaw-OpenShift
that referenced
this pull request
Jul 12, 2026
<!-- markdownlint-disable MD041 --> ## Summary Reconcile registry-recorded extra providers against the authoritative OpenShell gateway list before sandbox creation. This prevents a stale `tavily-search` record from breaking onboarding while preserving every healthy gateway-wide user provider. The PR replaces NVIDIA#6531, preserves Ho Lim's original issue work, and credits Shawn Xie's provider-reconciliation direction from NVIDIA#6518. ## Related Issue Fixes NVIDIA#6501. ## Changes - Query `provider list -g <gateway> --names` once when registry extras exist. - Attach only exact provider names returned by a successful authoritative list. - Preserve all recorded providers if the gateway query fails or throws, avoiding silent user-state loss during an outage. - Keep local registry state unchanged; reconciliation affects only the current sandbox-create plan. - Preserve healthy bare `brave-search`, `tavily-search`, custom, and bridge providers without treating their names as NemoClaw-owned. - Add focused and spawn-level regressions for healthy providers, stale records, exact-name matching, gateway scoping, and fail-open behavior. ## 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 - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [ ] Docs updated for user-facing behavior changes - [x] Docs not applicable — justification: this silently restores the documented provider source-of-truth contract without adding or changing a command, option, output, configuration, or remediation step. - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — exact-name gateway reconciliation was independently audited; it avoids diagnostic regexes, preserves healthy user-owned providers, fails open on gateway-list failure, and does not mutate registry state. - [ ] 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 — 17 focused reconciliation tests and all 65 onboard integration tests passed. - [ ] 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) Additional exact-head validation: CLI type-check, file/commit/push hooks, commitlint, test-size/source-shape budgets, secret scan, and `npm run check:diff` passed. Documentation review found no update required. `src/lib/onboard.ts` is net-neutral. --- Signed-off-by: Apurv Kumaria <akumaria@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Sandbox creation now better matches configured providers to what’s actually available, reducing unexpected provider options during onboarding. * **Bug Fixes** * Fixed cases where stale or unavailable extra providers could still appear in sandbox setup. * Improved behavior when provider lookup fails, helping preserve previously configured choices instead of removing them unexpectedly. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Apurv Kumaria <akumaria@nvidia.com> Co-authored-by: Ho Lim <subhoya@gmail.com> Co-authored-by: Shawn Xie <shaxie@nvidia.com>
Hadar301
pushed a commit
to Hadar301/NemoClaw-OpenShift
that referenced
this pull request
Jul 12, 2026
<!-- 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>
wscurran
added
area: onboarding
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
3 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
nemoclaw onboardfails at sandbox create withprovider 'tavily-search' not foundwhenever the host registry still records an extra provider that no longer exists on the (reused) gateway — even when the user explicitly declined web search. This PR reconciles the registry-recorded extra providers against the gateway before they are passed tosandbox create --provider.Related Issue
Fixes #6501
Changes
reconcileRegisteredExtraProvidersinsrc/lib/onboard/extra-provider-reconciliation.ts(re-exported throughsandbox-provider-cleanup.tsso the onboard entrypoint import stays net-zero): when the recorded set is non-empty, probe each recorded name with a gateway-scopedopenshell provider get -g <gateway> <name>(the same existence checkupsertProviderrelies on). A record is pruned — skipped, warned about (with anemoclaw credentials addrecreate hint), and removed from the registry — only when the gateway explicitly answers with a provider-not-found diagnostic. Any other failure (gateway down, timeout, unexpected error) keeps the record — fail-open — so a real outage still surfaces through the existing sandbox-create diagnostics and a healthy provider is never silently dropped.extraProviders:production call site (net-zero line change insrc/lib/onboard.ts).test/extra-provider-reconciliation.test.ts: empty-set short-circuit (no gateway probe), gateway-scoped confirm passthrough, not-found prune+warn+forget in both CLI and gRPC diagnostic orderings (the [NemoClaw][onboard][All Platforms] nemoclaw onboard fails creating sandbox when web search is disabled — tries to configure nonexistent tavily-search provider #6501 scenario), non-not-found fail-open, and Buffer diagnostic handling.test/onboard-extra-provider-prune.test.ts: a spawn-levelcreateSandboxintegration test where the gateway reports the recordedtavily-searchas not found — asserts the create command omits--provider tavily-search, the probe is gateway-scoped, the stale record is removed from the real registry state, and the user-facing warning is emitted. (Kept as a focused file to respect thetest/onboard.test.tssize budget.)Root cause note:
credentials addrecords the provider name host-side (addExtraProvider), but gateway-side deletion paths can leave that record dangling; every subsequent onboard then attaches the stale name and OpenShell rejects the create. Probing at consumption time recovers regardless of how the desync happened.Design history: the first revision consulted
provider list --namesand treated any exit-0 output as an authoritative snapshot. That pruned legitimately configured providers when a stub or degraded gateway answered success with an empty list —cli-test-shards (5)caught exactly that against the always-OK openshell test fixture. The per-name probe with an explicit not-found requirement removes that failure mode.Type of Change
Quality Gates
Verification
Verifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailablenpx vitest run --project integration test/extra-provider-reconciliation.test.ts test/sandbox-provider-cleanup.test.ts test/onboard-extra-provider-prune.test.ts→ 34 tests passed;npx vitest run --project integration test/onboard.test.ts→ 66 tests passed;npm run typecheck:cliclean.npm 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: Shawn Xie shaxie@nvidia.com
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests