feat(nvidia): add NVIDIA provider with onboarding flow#71204
Conversation
Greptile SummaryThis PR adds the NVIDIA provider plugin — onboarding flow, catalog entries, plugin JSON manifest, and the new Confidence Score: 5/5Safe to merge; only a minor P2 UX regression with no functional impact. All findings are P2. The sole issue is the unconditional removal of the 'resolves to' hint on the 'Keep current' option, which is a cosmetic UX regression for non-literal-prefix providers using aliases. No logic errors, security issues, or data integrity problems. src/flows/model-picker.ts — review the hint removal around line 540. Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/flows/model-picker.ts
Line: 540-545
Comment:
**"resolves to" hint unconditionally removed**
The old "Keep current" option showed a secondary hint (`resolves to {resolvedKey}`) whenever `configuredRaw !== resolvedKey` — i.e., when the user had stored an alias or shortened ref that maps to a different canonical key. That hint has been dropped entirely by this change, not just for literal-prefix providers. Non-literal-prefix providers where a user stored an alias (e.g. `anthropic/claude`) are now silently missing the disambiguation that previously told them what the ref resolves to.
How can I resolve this? If you propose a fix, please make it concise.Reviews (3): Last reviewed commit: "fix(nvidia): drop redundant hint from Ke..." | Re-trigger Greptile |
💡 Codex Reviewopenclaw/extensions/nvidia/onboard.ts Line 7 in 6c6a5c3
https://github.com/openclaw/openclaw/blob/6c6a5c3cf62dca19cb5242bfc6c3c2abf5a98c91/docs/.generated/config-baseline.json#L1 This commit adds full baseline JSON artifacts under ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
Fixed in 4890410 — |
🔒 Aisle Security AnalysisWe found 1 potential security issue(s) in this PR:
1. 🔵 Terminal escape/control-sequence injection via untrusted model IDs in interactive picker labels and notes
DescriptionModel/provider identifiers coming from provider catalogs (potentially network-sourced) are interpolated directly into terminal UI strings without neutralizing ANSI/control characters. Impacted behaviors introduced/expanded in this change:
Because
Vulnerable code (example): const label = params.literalPrefixProviders.has(normalizeProviderId(params.entry.provider))
? `${params.entry.provider}/${params.entry.id}`
: key;
params.options.push({ value: key, label });RecommendationSanitize any model/provider IDs before rendering them in terminal UI labels/notes.
Example fix: import { sanitizeTerminalText } from "../terminal/safe-text.js";
const rawLabel = literalPrefixProviders.has(normalizeProviderId(entry.provider))
? `${entry.provider}/${entry.id}`
: key;
const label = sanitizeTerminalText(rawLabel);
options.push({ value: key, label, hint });Similarly, sanitize Analyzed PR: #71204 at commit Last updated on: 2026-04-29T16:21:13Z |
|
All findings addressed in new commits:
|
|
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
|
Following up on the aisle-research-bot report: we are not addressing these as part of this PR. Reason, point by point:
Given the above, we are keeping this PR scoped to the NVIDIA provider work and not patching pre-existing repo-wide patterns or non-actionable security framing that was not introduced by this change. |
|
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
5df2720 to
f1a7860
Compare
9a407f8 to
53a57ef
Compare
|
Codex review: needs maintainer review before merge. What this changes: The PR branch proposes adding NVIDIA provider onboarding/catalog support plus model-picker/provider-prefix behavior, but its current diff also includes broad CI, Docker, docs, skills, doctor, plugin-registry, and test changes. Maintainer follow-up before merge: Not a safe automated repair candidate: the PR has a protected maintainer label, is very broad, includes security-sensitive workflow/Docker surfaces, and needs maintainer judgment rather than a single focused fix PR. Review detailsBest possible solution: Keep the PR in maintainer review. The clean path is to either reduce it to a narrow, current-main NVIDIA/model-picker change with focused tests, or have a maintainer close/replace it manually if the already-shipped NVIDIA provider work fully supersedes the intended contribution. Workflow, Docker, release, and security-sensitive changes should be split into separate reviewed PRs. What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against c6b269154a39. |
|
Codex review: needs maintainer review before merge. What this changes: The PR branch adds a bundled NVIDIA provider with onboarding/catalog/docs, model-reference display behavior for NVIDIA namespaced IDs, diagnostic redaction updates, and many additional CI, Docker, docs, skill, doctor, plugin-registry, and test changes. Maintainer follow-up before merge: Protected maintainer label plus broad security-sensitive workflow/Docker scope make this a maintainer decision, not a safe automated repair or replacement PR candidate. Review detailsBest possible solution: Keep this PR in maintainer review. The clean path is to reduce it to a narrow NVIDIA/model-picker change against current main with focused tests, or have a maintainer close and replace it manually if the already-shipped NVIDIA provider work fully supersedes the intended contribution. Workflow, Docker, release, and broad registry/doctor changes should be split into separately reviewed PRs. What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 240362bf6d0e. |
Add the NVIDIA build.nvidia.com API as a bundled provider. Default model is nvidia/nvidia/nemotron-3-super-120b-a12b: first segment is the provider id, remaining "nvidia/nemotron-3-super-120b-a12b" is the literal upstream model id (which happens to start with "nvidia/" because NVIDIA is also the model maker). Supporting core change: introduce a provider capability flag nativeIdsIncludeProviderPrefix so providers whose native catalog ids intentionally include their provider prefix (OpenRouter) opt into self-prefix dedupe in modelKey, without hardcoding provider names in core. Providers whose ids merely happen to start with their own name (NVIDIA) leave the flag unset and get the full <provider>/<model-id> concatenation. - extensions/nvidia/*: new plugin, catalog, onboarding, tests, docs - extensions/openrouter/index.ts: declare nativeIdsIncludeProviderPrefix - src/plugins/types.ts: add field to ProviderPlugin - src/plugins/registry.ts: populate self-prefix set on registration - src/agents/provider-self-prefix.ts: sync accessor used by modelKey - src/agents/model-ref-shared.ts: modelKey consults the flag - test updates for affected surfaces Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Codex review: found issues before merge. What this changes: The PR updates the bundled NVIDIA provider docs, onboarding/config helpers and tests, and adds provider-prefix display behavior for NVIDIA model refs in model-picker and provider-auth flows. Maintainer follow-up before merge: Protected maintainer label plus plugin/runtime control-plane implications make this a maintainer review item, not a safe automated repair or replacement PR candidate. Review findings:
Review detailsBest possible solution: Keep this PR in maintainer review. The desired end state is a narrow NVIDIA/model-picker change that either stores the literal-prefix display flag in manifest/metadata or otherwise keeps label decisions on a manifest-only/lightweight path, with focused default-model and allowlist picker coverage against current main. Full review comments:
Overall correctness: patch is incorrect What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against c99d6807141e. |
|
Codex review: found issues before merge. What this changes: The PR updates NVIDIA provider onboarding, docs, catalog tests, and API exports, and adds a Maintainer follow-up before merge: Protected maintainer label plus plugin control-plane/runtime-loading implications make this a maintainer review item, not a safe automated repair or replacement PR lane. Review findings:
Review detailsBest possible solution: Keep this in maintainer review and make the desired end state a narrow NVIDIA/model-picker change that keeps canonical config storage while sourcing literal-prefix display metadata from a manifest or other lightweight control-plane surface, with focused coverage for NVIDIA onboarding, default-model labels, and allowlist picker labels. Full review comments:
Overall correctness: patch is incorrect What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against e46dccb35374. |
Summary
Add NVIDIA as a bundled provider with onboarding, docs, and catalog wiring.
Problem
When NVIDIA is the model maker, the code base is collapsing nvidia/nvidia / into nvidia/, mistakenly assuming we have double providers listed. This makes our model picker selection inconsistent with the rest of the nvidia provider models when offering our own models.
Changes
extensions/nvidiaprovider plugin, onboarding flow, catalog, and contract coveragepreserveLiteralProviderPrefixflag onProviderPlugin— NVIDIA sets it so picker labels shownvidia/nvidia/nemotron-...(literal vendor namespace) while config storage stays canonical (nvidia/nemotron-...)--<prefix>-api-keyin diagnostic support redaction, covering--nvidia-api-keyand future provider-scoped flagsNotes
feat/nvidia-providermainaddModelSelectOptioncomputes a label frompreserveLiteralProviderPrefix; option value and persisted config remain canonical. No dual-key system, no migration helpers.