fix(inference): reject unsafe provider replacement - #7763
Conversation
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
📝 WalkthroughWalkthroughCompatible-provider switching now validates recorded direct bindings before mutation, reuses matching providers without updates, cleans up newly created providers after verification failure, and documents endpoint and restoration behavior. Tests add stateful provider lifecycle coverage and updated compatible-provider fixtures. ChangesCompatible provider binding safeguards
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
🌿 Preview your docs: https://nvidia-preview-pr-7763.docs.buildwithfern.com/nemoclaw |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 9768615 in the TypeScript / code-coverage/cliThe overall coverage in commit 9768615 in the Show a code coverage summary of the most impacted files.
Updated |
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/lib/actions/inference-set.ts (1)
644-654: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider moving this pure comparison to the domain layer.
matchesRecordedDirectProviderBindingis a pure decision function (no I/O, no side effects) defined inline ininference-set.ts(an actions module). As per path instructions,src/lib/{actions,domain,adapters,state}/**ownership should keep "pure decisions" insrc/lib/domain/**while actions orchestrate. Extracting this comparison into a domain module would keep the layering consistent and make it independently unit-testable.🤖 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.ts` around lines 644 - 654, Move the pure comparison implemented by matchesRecordedDirectProviderBinding from the actions module into an appropriate src/lib/domain module, export it there, and update the action code to import and use the domain function while preserving the existing matching behavior.Source: Path instructions
src/lib/actions/inference-set-compatible-provider.test.ts (1)
301-337: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate mock shape vs. shared
createCompatibleProviderCapture.This inline mock re-implements the
provider:get/provider:create/provider:deletecases already increateCompatibleProviderCapture(test-support.ts), only to add a failinginference:setcase and dropprovider:update. Consider extending the shared helper to accept optional command overrides (e.g., aninference:setfailure injector) so this scenario reuses the shared lifecycle mock instead of duplicating it.🤖 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-compatible-provider.test.ts` around lines 301 - 337, Extend createCompatibleProviderCapture in test-support.ts to accept optional command overrides, then update the test “removes an absent direct provider when verified route selection fails (`#7725`)” to use that shared helper for provider:get/create/delete lifecycle behavior while injecting only the failing inference:set response and omitting provider:update as needed. Remove the duplicated inline captureOpenshell mock and preserve the scenario’s existing assertions and state transitions.
🤖 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 980-993: Refine the error handling in the direct-provider update
branch around matchesRecordedDirectProviderBinding so the InferenceSetError
message accurately identifies whether the mismatch is in provider, endpointUrl,
or credentialEnv. Preserve the existing rollback guidance for endpoint/provider
replacements, while providing credentialEnv-specific guidance when the endpoint
remains unchanged.
---
Nitpick comments:
In `@src/lib/actions/inference-set-compatible-provider.test.ts`:
- Around line 301-337: Extend createCompatibleProviderCapture in test-support.ts
to accept optional command overrides, then update the test “removes an absent
direct provider when verified route selection fails (`#7725`)” to use that shared
helper for provider:get/create/delete lifecycle behavior while injecting only
the failing inference:set response and omitting provider:update as needed.
Remove the duplicated inline captureOpenshell mock and preserve the scenario’s
existing assertions and state transitions.
In `@src/lib/actions/inference-set.ts`:
- Around line 644-654: Move the pure comparison implemented by
matchesRecordedDirectProviderBinding from the actions module into an appropriate
src/lib/domain module, export it there, and update the action code to import and
use the domain function while preserving the existing matching behavior.
🪄 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: b02c1192-933f-4ba5-9b1e-d858ac661f9b
📒 Files selected for processing (6)
docs/inference/switch-providers.mdxsrc/lib/actions/inference-set-compatible-provider.test.tssrc/lib/actions/inference-set-degraded-state.test.tssrc/lib/actions/inference-set-provider-alias.test.tssrc/lib/actions/inference-set.test-support.tssrc/lib/actions/inference-set.ts
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
Nemotron output stays in workflow artifacts and does not change the assessment above. E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: 1 optional E2E recommendation
This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.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-compatible-provider.test.ts`:
- Around line 428-430: Align the parameterized expectations around
prepareInferenceSetProviderBinding with the provider guard’s implemented error
contract: assert the “Refusing to replace provider ... does not match this
sandbox's durable custom-endpoint provenance” message for both cases, or change
the guard to emit the binding-specific diagnostic while preserving the intended
behavior.
🪄 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: d8f84383-cf06-4da3-be93-0523f17b227f
📒 Files selected for processing (3)
docs/inference/switch-providers.mdxsrc/lib/actions/inference-set-compatible-provider.test.tssrc/lib/actions/inference-set.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/actions/inference-set.ts
- docs/inference/switch-providers.mdx
<!-- markdownlint-disable MD041 --> ## Summary Add the canonical dated changelog entry for NemoClaw v0.0.97 before the release plan captures `origin/main`. The entry groups the user-visible and maintainer-facing changes since v0.0.96 while preserving the Deferred dual-Station status, experimental runtime-identity boundary, and pending physical IGX validation. ## Changes - Add `docs/changelog/2026-07-28.mdx` with the parser-safe MDX SPDX comment and exact `## v0.0.97` heading. - Summarize the 43 merged PRs in the release range, omitting internal-only changes from the public entry and linking each grouped change to its most specific published documentation. - Keep the experimental Okta reference explicitly opt-in and outside normal onboarding, keep the two-Station path Deferred, and state that physical IGX Orin validation remains pending. ### Source summary - [#7440](#7440), [#7443](#7443), and [#7445](#7445) -> `docs/changelog/2026-07-28.mdx`: Document read-only host readiness reports and fail-closed platform qualification. - [#7030](#7030) -> `docs/changelog/2026-07-28.mdx`: Document the Deferred trusted two-Station vLLM evaluation. - [#7265](#7265) -> `docs/changelog/2026-07-28.mdx`: Document the bounded experimental direct-runner Okta runtime-identity reference. - [#7711](#7711) and [#7648](#7648) -> `docs/changelog/2026-07-28.mdx`: Document compatible-endpoint reasoning effort and retired NVIDIA Build model paths. - [#7746](#7746), [#7763](#7763), and [#7681](#7681) -> `docs/changelog/2026-07-28.mdx`: Document safe compatible-provider creation, replacement refusal, and narrow OpenShell bridge URL handling. - [#7641](#7641), [#7690](#7690), [#7631](#7631), and [#7710](#7710) -> `docs/changelog/2026-07-28.mdx`: Document paused-container recovery, recreation journaling, pre-mutation uninstall checks, and source-checkout OpenShell selection. - [#7624](#7624) and [#7762](#7762) -> `docs/changelog/2026-07-28.mdx`: Document Jetson release diagnostics and bounded render-device group propagation. - [#7639](#7639), [#7760](#7760), [#7721](#7721), and [#7761](#7761) -> `docs/changelog/2026-07-28.mdx`: Document Telegram, MCP media-type, Hermes image-mode, and locked-restart fixes. - [#7653](#7653) and [#7680](#7680) -> `docs/changelog/2026-07-28.mdx`: Document Deep Agents policy tasks and the bounded Claude Code OAuth path. - [#7679](#7679) -> `docs/changelog/2026-07-28.mdx`: Document the checksum-bound libssh2 and Python HTMLParser backports. - [#7655](#7655), [#7651](#7651), [#7664](#7664), [#7666](#7666), [#7670](#7670), [#7719](#7719), and [#7741](#7741) -> `docs/changelog/2026-07-28.mdx`: Document exact candidate E2E evidence, Launchable selection, diagnostic consolidation, and trusted WSL validation. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [x] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates - [ ] Tests added or updated for changed behavior - [x] Existing tests cover changed behavior — justification: `test/changelog-docs.test.ts` validates the dated changelog contract, MDX header, heading uniqueness, and release-entry structure. - [ ] Tests not applicable — justification: - [x] Docs updated for user-facing behavior changes - [ ] Docs not applicable — justification: - [ ] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [ ] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Documentation Writer Review - [x] Documentation writer subagent reviewed the completed changes - Result: `docs-updated` - Evidence: The committed `docs/changelog/2026-07-28.mdx` blob exactly matches the reviewed file. Completeness, factual accuracy, link shape, parser-safe MDX header, one-sentence-per-line style, `.docs-skip` compliance, and bounded product claims passed. - Agent: Codex Desktop documentation writer subagent <!-- docs-review-head-sha: da6aa27 --> <!-- docs-review-agents-blob-sha: be20a09 --> ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: Not applicable; this PR changes only the dated changelog. - Station profile/scenario: Not applicable. - Result: Not applicable. - Supporting evidence: Not applicable. ## Verification - [x] PR description includes a `Signed-off-by:` line 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 — `npx vitest run test/changelog-docs.test.ts` passed 6/6. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — not applicable to this doc-only release entry. - [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) — completed with 0 errors and 2 pre-existing Fern warnings. - [x] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) — native changelog entries use the required parser-safe MDX SPDX comment and intentionally have no frontmatter. --- Signed-off-by: Charan Jagwani <cjagwani@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added improved host readiness reporting and Jetson onboarding guidance. * Added controls for reasoning effort with compatible endpoints and enhanced managed MCP discovery. * Improved Deep Agents task publication and preset support. * **Bug Fixes** * Hardened provider switching, sandbox recovery, uninstall behavior, and Telegram connectivity. * Improved container image integrity checks, media-type handling, and checksum validation. * Enhanced vLLM evaluation behavior and release diagnostics. * **Documentation** * Added the NemoClaw v0.0.97 changelog. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Summary
This follow-up prevents the behavior reported by PR Review Advisor on #7746: OpenShell verified an existing direct provider's old endpoint before NemoClaw replaced it with the requested endpoint. NemoClaw now reuses only the exact direct binding recorded for the sandbox and refuses endpoint replacement that cannot be rolled back.
Related Issue
Follow-up to #7746 (
PRA-1); related to #7725.Changes
Type of Change
Quality Gates
97686155bpassed all nine categories with no findings. The change fails before an unverifiable direct-provider mutation; endpoint validation, invocation-local credential handling, mutation locking, and HTTPS Pin Runtime rollback remain intact.Documentation Writer Review
docs-updateddocs/inference/switch-providers.mdx; verified direct-provider refusal and exact-binding reuse, absent-provider verification and cleanup, and the unchanged DNS-backed HTTPS Pin Runtime update and rollback guidance. The focused inference tests passed 77/77, including endpoint and credential-binding mismatch regressions; the test-conditional scan passed.npm run docspassed with 0 errors and 2 pre-existing warnings.DGX Station Hardware Evidence
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailablenpx vitest run --project cli src/lib/actions/inference-set-compatible-provider.test.ts src/lib/actions/inference-set-degraded-state.test.ts src/lib/actions/inference-set-provider-alias.test.ts src/lib/actions/inference-set-provider.test.ts src/lib/actions/inference-set-gateway-route-containment.test.ts: 77 passed;npm run test-conditionals:scan -- --top 25: passednpm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — not applicable to this isolated inference correctionnpm run docsbuilds without warnings (doc changes only) — passed with 0 errors and 2 pre-existing warningsSigned-off-by: Prekshi Vyas prekshiv@nvidia.com
Summary by CodeRabbit