fix(inference): show registered providers and onboard tip on provider-not-found - #5983
fix(inference): show registered providers and onboard tip on provider-not-found#5983kagura-agent wants to merge 4 commits into
Conversation
…-not-found (NVIDIA#5924) When `openshell inference set` fails with a provider-not-found error, the error message now lists registered providers and suggests running `nemoclaw onboard` to register a new one. Co-Authored-By: Claude Opus 4 <noreply@anthropic.com> Signed-off-by: kagura-agent <kagura.agent.ai@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesProvider-not-found error enrichment
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/lib/actions/inference-set.test.ts (1)
1113-1180: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a regression case for
listSandboxes()throwing.The new fallback branch in
runInferenceSetis still untested. A case wherelistSandboxes()throws would lock in the intended public error output for registry failures instead of only the happy-path lookup variants. As per path instructions, “Review tests for behavioral confidence rather than implementation lock-in.”🤖 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/inference-set.test.ts` around lines 1113 - 1180, Add a regression test in inference-set.test.ts for the new fallback path in runInferenceSet when listSandboxes() throws during provider lookup. Use createDeps and mock the deps call that powers listSandboxes to throw, then assert the surfaced Error message matches the intended public registry-failure output and still includes the onboard tip when appropriate. Keep the test alongside the existing runInferenceSet provider-not-found cases so it covers the fallback branch without locking into implementation details.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/inference-set.ts`:
- Around line 620-639: In the InferenceSetError construction inside the
inference-set flow, avoid defaulting to “No providers registered” when
deps.listSandboxes() fails. Track whether the provider registry lookup succeeded
in the try block, and only populate providerList from the registeredProviders
list when it does; if the lookup throws, leave out provider details entirely and
keep the fallback tip text. Use the existing deps.listSandboxes,
registeredProviders, and InferenceSetError block to locate the fix.
---
Nitpick comments:
In `@src/lib/actions/inference-set.test.ts`:
- Around line 1113-1180: Add a regression test in inference-set.test.ts for the
new fallback path in runInferenceSet when listSandboxes() throws during provider
lookup. Use createDeps and mock the deps call that powers listSandboxes to
throw, then assert the surfaced Error message matches the intended public
registry-failure output and still includes the onboard tip when appropriate.
Keep the test alongside the existing runInferenceSet provider-not-found cases so
it covers the fallback branch without locking into implementation details.
🪄 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: 3cfcd6b2-0113-4a48-ae6b-b4750208f12b
📒 Files selected for processing (2)
src/lib/actions/inference-set.test.tssrc/lib/actions/inference-set.ts
Address CodeRabbit review: - Don't report 'No providers registered' when listSandboxes() throws - Add regression test for listSandboxes() throwing Signed-off-by: kagura-agent <kagura.agent.ai@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/inference-set.test.ts`:
- Around line 1186-1195: The test setup in runInferenceSet is using a mocked
missing provider that does not match the provider under test, so the failure
path is not validating the intended behavior. Update the stderr fixture and the
runInferenceSet call to use the same provider name, referencing the
runInferenceSet test case and its provider/model inputs, so the assertion
exercises the public error enrichment for the requested provider directly.
🪄 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: 2c5b7b90-942e-4f9a-99c5-1ce544c9d33c
📒 Files selected for processing (2)
src/lib/actions/inference-set.test.tssrc/lib/actions/inference-set.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/actions/inference-set.ts
|
✨ Thanks for the fix. The enhanced error message with registered providers list and onboard tip is a clear UX improvement for Related open issues: |
|
Superseded by #6023, which preserves this exact verified head SHA in NVIDIA/NemoClaw so the mandatory PR Review Advisor can run. No commits were rewritten or cherry-picked. |
…-not-found (#6023) <!-- markdownlint-disable MD041 --> ## Summary <!-- 1-3 sentences: what this PR does and why. --> When `nemoclaw inference set` fails because the requested provider is not registered, preserve a bounded, fully redacted failure diagnostic and add the registered provider list plus an onboarding recovery tip. This same-repository replacement preserves #5983's verified contributor history so the mandatory PR Review Advisor can run; fork-origin advisor jobs are skipped by policy. ## Related Issue <!-- Fixes #NNN or Closes #NNN. Remove this section if none. --> Closes #5924 Supersedes #5983 ## Changes <!-- Bullet list of key changes. --> - Capture `openshell inference set` output and enrich only a quoted provider-not-found diagnostic naming the provider actually requested. - Fully redact, whitespace-compact, and cap generic and enhanced OpenShell failure details at 500 characters so diagnostics remain useful without exposing credentials or unbounded subprocess output. - Query the sandbox registry for known providers without allowing registry lookup failures to hide the original route-update error; emit only a static safe warning when that lookup fails. - Isolate parsing and message formatting in `inference-set-error.ts`, with focused security/correctness tests separate from the already-large action test. - Cover matching, mismatched, malformed, stderr, and stdout diagnostics; registered and empty registries; registry failures; full secret-shape redaction; enhanced-path output bounds; and generic failures. - Compatibility boundary: OpenShell 0.0.71 exposes this condition only as subprocess text. Unknown or drifted formats stay generic; replace this parser when OpenShell provides a structured provider-not-found error carrying the missing provider as a field. - Preserve #5983's exact four GitHub-Verified commits without rewriting or cherry-picking, then add two signed maintainer hardening/refactor commits on top. ## 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 all that apply. For any "covered by existing tests", "not applicable", or waiver entry, add a brief justification on the same line or in the Changes section. --> - [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 enriches an existing failure path without changing commands, configuration, or successful behavior. - [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: full redaction, exact-provider correlation, bounded output, malformed-input behavior, safe registry-warning text, and focused regressions were manually reviewed. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Verification <!-- Check each item you ran and confirmed. Leave unchecked items you skipped. Doc-only changes do not require npm test unless you ran it. --> - [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 — 38/38 focused tests - [x] `npm run typecheck:cli` passes - [ ] 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 (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) Exact-head E2E: https://github.com/NVIDIA/NemoClaw/actions/runs/28425898821 --- <!-- 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: kagura-agent <kagura.agent.ai@gmail.com> Signed-off-by: Carlos Villela <cvillela@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Enhanced “provider not found” failure diagnostics when switching inference providers, including a registered-provider list (or “No providers registered”) and an onboarding tip to run `nemoclaw onboard`. * Improved visibility into command output while redacting sensitive details. * **Bug Fixes** * More reliable detection of provider-missing failures and clearer, actionable error reporting; non-provider-related failures remain generic. * When the requested provider can’t be found, existing route/sandbox state remains unchanged. * **Documentation** * Added prerequisites and troubleshooting guidance for cross-provider switching, including what happens when OpenShell can’t locate the requested provider. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: kagura-agent <kagura.agent.ai@gmail.com> Signed-off-by: Carlos Villela <cvillela@nvidia.com> Signed-off-by: Prek Shiv <prekshiv@nvidia.com> Co-authored-by: kagura-agent <kagura.agent.ai@gmail.com> Co-authored-by: Claude Opus 4 <noreply@anthropic.com> Co-authored-by: Prekshi Vyas <prekshiv@nvidia.com>
…-not-found (NVIDIA#6023) <!-- markdownlint-disable MD041 --> ## Summary <!-- 1-3 sentences: what this PR does and why. --> When `nemoclaw inference set` fails because the requested provider is not registered, preserve a bounded, fully redacted failure diagnostic and add the registered provider list plus an onboarding recovery tip. This same-repository replacement preserves NVIDIA#5983's verified contributor history so the mandatory PR Review Advisor can run; fork-origin advisor jobs are skipped by policy. ## Related Issue <!-- Fixes #NNN or Closes #NNN. Remove this section if none. --> Closes NVIDIA#5924 Supersedes NVIDIA#5983 ## Changes <!-- Bullet list of key changes. --> - Capture `openshell inference set` output and enrich only a quoted provider-not-found diagnostic naming the provider actually requested. - Fully redact, whitespace-compact, and cap generic and enhanced OpenShell failure details at 500 characters so diagnostics remain useful without exposing credentials or unbounded subprocess output. - Query the sandbox registry for known providers without allowing registry lookup failures to hide the original route-update error; emit only a static safe warning when that lookup fails. - Isolate parsing and message formatting in `inference-set-error.ts`, with focused security/correctness tests separate from the already-large action test. - Cover matching, mismatched, malformed, stderr, and stdout diagnostics; registered and empty registries; registry failures; full secret-shape redaction; enhanced-path output bounds; and generic failures. - Compatibility boundary: OpenShell 0.0.71 exposes this condition only as subprocess text. Unknown or drifted formats stay generic; replace this parser when OpenShell provides a structured provider-not-found error carrying the missing provider as a field. - Preserve NVIDIA#5983's exact four GitHub-Verified commits without rewriting or cherry-picking, then add two signed maintainer hardening/refactor commits on top. ## 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 all that apply. For any "covered by existing tests", "not applicable", or waiver entry, add a brief justification on the same line or in the Changes section. --> - [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 enriches an existing failure path without changing commands, configuration, or successful behavior. - [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: full redaction, exact-provider correlation, bounded output, malformed-input behavior, safe registry-warning text, and focused regressions were manually reviewed. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Verification <!-- Check each item you ran and confirmed. Leave unchecked items you skipped. Doc-only changes do not require npm test unless you ran it. --> - [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 — 38/38 focused tests - [x] `npm run typecheck:cli` passes - [ ] 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 (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) Exact-head E2E: https://github.com/NVIDIA/NemoClaw/actions/runs/28425898821 --- <!-- 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: kagura-agent <kagura.agent.ai@gmail.com> Signed-off-by: Carlos Villela <cvillela@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Enhanced “provider not found” failure diagnostics when switching inference providers, including a registered-provider list (or “No providers registered”) and an onboarding tip to run `nemoclaw onboard`. * Improved visibility into command output while redacting sensitive details. * **Bug Fixes** * More reliable detection of provider-missing failures and clearer, actionable error reporting; non-provider-related failures remain generic. * When the requested provider can’t be found, existing route/sandbox state remains unchanged. * **Documentation** * Added prerequisites and troubleshooting guidance for cross-provider switching, including what happens when OpenShell can’t locate the requested provider. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: kagura-agent <kagura.agent.ai@gmail.com> Signed-off-by: Carlos Villela <cvillela@nvidia.com> Signed-off-by: Prek Shiv <prekshiv@nvidia.com> Co-authored-by: kagura-agent <kagura.agent.ai@gmail.com> Co-authored-by: Claude Opus 4 <noreply@anthropic.com> Co-authored-by: Prekshi Vyas <prekshiv@nvidia.com>
Description
When
nemoclaw inference setfails because the requested provider is not registered in the gateway, the error message now includes the list of registered providers and a tip to runnemoclaw onboard.Before
After
Changes
openshell inference setoutput (pipe stdio instead of inherit) to detect provider-not-found errorsTesting
inference-set.test.ts:Closes #5924
Signed-off-by: kagura-agent kagura.agent.ai@gmail.com
Summary by CodeRabbit
Bug Fixes
Tests