fix(plugin): show actual configured model in banner (Closes #24) - #1819
Conversation
When the onboard config file is not available (e.g. inside the sandbox), the plugin banner hardcodes the model as nvidia/nemotron-3-super-120b-a12b and the endpoint as build.nvidia.com regardless of what is actually configured in OpenShell. Query the live OpenShell inference state via `openshell inference get --json` as a fallback before resorting to hardcoded defaults. The probe has a 3-second timeout and falls back to the existing defaults if anything goes wrong. Closes NVIDIA#24
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a synchronous probe of OpenShell inference state and updates provider/model resolution so registration uses onboard config first, then the probe, then original hardcoded defaults; tests were added to validate the probe-driven fallback when onboard config is absent. Changes
Sequence Diagram(s)sequenceDiagram
participant Register as Register()
participant Onboard as OnboardConfig
participant Probe as OpenShell CLI (execFileSync)
participant Defaults as HardcodedDefaults
participant Registry as ProviderRegistry
Register->>Onboard: read bannerEndpoint / bannerProvider / bannerModel
alt onboard has all values
Register->>Registry: register using onboard values
else missing any value
Register->>Probe: execFileSync("openshell inference get --json") (3s)
Probe-->>Register: { endpoint, provider, model } or error
alt probe supplies missing values
Register->>Registry: register using onboard + probed values
else
Register->>Defaults: use hardcoded defaults
Defaults-->>Register: { endpoint, provider, model }
Register->>Registry: register using onboard + defaults
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
nemoclaw/src/index.ts (1)
272-275:⚠️ Potential issue | 🟠 MajorResolve the effective model before
registerProvider().The probe only updates banner strings after
registerProvider()has already been called. WhenonboardCfgis missing,activeModelEntries()still publishes the baked-in catalog, so the registered provider/UI can disagree with the banner and with the live OpenShell model.Also applies to: 283-290
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nemoclaw/src/index.ts` around lines 272 - 275, The provider is being registered before the effective model/catalog is resolved, so the UI banner and registered provider can disagree; before calling api.registerProvider(registeredProviderForConfig(onboardCfg, providerCredentialEnv)) ensure the effective model is resolved by invoking the same resolution path used by the probe—e.g., call activeModelEntries() (or otherwise trigger the probe/model resolution logic) right after loadOnboardConfig() and before computing registeredProviderForConfig so the banner/catalog are up-to-date when api.registerProvider runs (also apply this ordering fix to the similar block around the code at lines 283–290).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@nemoclaw/src/index.ts`:
- Around line 25-40: probeOpenShellInference currently only returns endpoint and
model so callers (banner logic) always fall back to "NVIDIA Endpoints"; modify
probeOpenShellInference to also return the provider string by reading
parsed.provider (e.g., return { provider: parsed.provider ?? parsed.endpoint ??
"", endpoint: parsed.endpoint ?? parsed.provider ?? "", model: parsed.model ??
"" }) and update any banner/display code that uses probeOpenShellInference() to
prefer the returned provider when present (instead of hardcoding "NVIDIA
Endpoints"), e.g., use the provider value to construct the banner label.
- Around line 25-44: Add a Vitest unit test in the existing register.test.ts
that mocks node:child_process execFileSync to return a JSON string with
provider, model, and/or endpoint (e.g.
{"provider":"custom","model":"gpt-x","endpoint":"https://api"}), call
probeOpenShellInference and assert it returns the probed non-default
provider/model/endpoint; also add an integration-style assertion that the code
path which consumes probeOpenShellInference (the registration logic referenced
around probeOpenShellInference and the similar block at lines ~283-290) uses
those values instead of defaults. Use Vitest's vi.mock or vi.spyOn to stub
execFileSync, ensure you restore/reset the mock after the test, and cover both
provider->endpoint fallback and explicit endpoint cases.
---
Outside diff comments:
In `@nemoclaw/src/index.ts`:
- Around line 272-275: The provider is being registered before the effective
model/catalog is resolved, so the UI banner and registered provider can
disagree; before calling
api.registerProvider(registeredProviderForConfig(onboardCfg,
providerCredentialEnv)) ensure the effective model is resolved by invoking the
same resolution path used by the probe—e.g., call activeModelEntries() (or
otherwise trigger the probe/model resolution logic) right after
loadOnboardConfig() and before computing registeredProviderForConfig so the
banner/catalog are up-to-date when api.registerProvider runs (also apply this
ordering fix to the similar block around the code at lines 283–290).
🪄 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: Pro
Run ID: cef3faf3-a982-4de0-b744-e941f6c51da9
📒 Files selected for processing (1)
nemoclaw/src/index.ts
|
✨ Possibly related open PRs: Possibly related open issues: |
|
Good fix @WuKongAI-CMU — the probe fallback chain is clean and the 3-second timeout is well-chosen. One gap to address before merge: Provider still hardcoded. When CodeRabbit flagged this too — here's the minimal fix: -function probeOpenShellInference(): { endpoint: string; model: string } {
+function probeOpenShellInference(): { endpoint: string; provider: string; model: string } {
// ...
return {
endpoint: parsed.endpoint ?? parsed.provider ?? "",
+ provider: parsed.provider ?? "",
model: parsed.model ?? "",
};
// catch:
- return { endpoint: "", model: "" };
+ return { endpoint: "", provider: "", model: "" };
}
- const bannerProvider = onboardCfg ? describeOnboardProvider(onboardCfg) : "NVIDIA Endpoints";
+ let bannerProvider = onboardCfg ? describeOnboardProvider(onboardCfg) : "";
- if (!bannerEndpoint || !bannerModel) {
+ if (!bannerEndpoint || !bannerProvider || !bannerModel) {
const probed = probeOpenShellInference();
if (!bannerEndpoint) bannerEndpoint = probed.endpoint;
+ if (!bannerProvider) bannerProvider = probed.provider;
if (!bannerModel) bannerModel = probed.model;
}
+ if (!bannerProvider) bannerProvider = "NVIDIA Endpoints";This way Ollama users see "Ollama" instead of "NVIDIA Endpoints" in the banner. Also: DCO — add |
The in-sandbox plugin can lack the onboard config file, so the banner and registered provider catalog need to derive provider/model identity from the live OpenShell inference probe before falling back to NVIDIA defaults. Constraint: OpenShell probe is the only live source available inside sandbox execution.\nRejected: Keep provider defaulted to NVIDIA Endpoints | it leaves Ollama/OpenAI users with an incorrect banner.\nConfidence: high\nScope-risk: narrow\nTested: npm test -- nemoclaw/src/register.test.ts\nTested: npm run check --prefix nemoclaw\nTested: git diff --check\nSigned-off-by: Intern Dev <dev@wukongai.io>
|
Confirmed the requested follow-up is already in the latest head ( Verification:
|
cv
left a comment
There was a problem hiding this comment.
Looks good to me.
- banner now prefers onboard config, then live
openshell inference get --json, then hardcoded defaults - registered model catalog also reflects the probed active model when onboard config is unavailable
- regression coverage added for a non-default provider/model path
I also re-ran a local targeted check on the PR branch:
npx vitest run nemoclaw/src/register.test.tscd nemoclaw && npm run check
## Summary Bumps the published doc version to `0.0.22` and documents the user-visible CLI behavior changes to `nemoclaw <name> connect` that landed since v0.0.21. Drafted via the `nemoclaw-contributor-update-docs` skill against commits in `v0.0.21..origin/main`, filtered through `docs/.docs-skip`. ## Changes - **`docs/project.json`** and **`docs/versions1.json`**: bump the published version from `0.0.20` to `0.0.22`; insert a `0.0.21` entry into the version list so the history stays contiguous. - **`docs/reference/commands.md`** → `nemoclaw <name> connect`: document two new behaviors. - Readiness poll with `NEMOCLAW_CONNECT_TIMEOUT` (integer seconds; default `120`) that replaces the silent hang when the sandbox is not yet `Ready` — right after onboarding, while the 2.4 GB image is still pulling (#466). - Post-connect hint is now agent-aware, names the correct TUI command for the sandbox's agent, and tells you to use `/exit` to leave the chat before `exit` returns you to the host shell (#2080). Feature PRs that shipped their own docs in the same commit are intentionally not re-documented here: - `channels list/add/remove` (#2139) — command reference and the "`openclaw channels` blocked inside the sandbox" troubleshooting entry landed with the feature. - `nemoclaw gc` (#2176) — documented as part of the destroy/rebuild image cleanup PR. Skipped per `docs/.docs-skip`: - `e6bad533 fix(shields): verify config lock and fail hard on re-lock failure (#2066)` — matched `skip-features: src/lib/shields.ts`. Other commits in the range (#2141 OpenShell version bump, #1819 plugin banner live inference probe, #2085 / #2146 Slack Socket Mode fixes, #2110 axios proxy fix, #1818 NIM curl timeouts, #1824 onboard gateway bootstrap recovery, and assorted CI / test / install plumbing) are internal behavior refinements with no doc-relevant surface change. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [x] Doc only (includes code sample changes) ## Verification - [x] `npx prek run --all-files` passes for the modified files via the pre-commit hook, including `Regenerate agent skills from docs` (source ↔ generated parity confirmed) - [ ] `npm test` passes — skipped; the one pre-existing `test/cli.test.ts > unknown command exits 1` failure on `origin/main` is unrelated to these markdown/JSON-only changes - [ ] Tests added or updated for new or changed behavior — n/a, doc-only - [x] No secrets, API keys, or credentials committed - [x] Docs updated for user-facing behavior changes - [ ] `make docs` builds without warnings (doc changes only) — not run locally - [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) — n/a, no new pages ## AI Disclosure - [x] AI-assisted — tool: Claude Code --- Signed-off-by: Miyoung Choi <miyoungc@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * `connect` now displays the sandbox phase while waiting for readiness and honors a configurable timeout via NEMOCLAW_CONNECT_TIMEOUT (default 120s). * TTY hints are agent-aware and instruct using `/exit` before returning to the host shell. * **Documentation** * Command docs updated to describe polling, timeout, and TTY guidance. * Project/docs metadata updated for versions 0.0.21 and 0.0.22 (package version bumped to 0.0.22). <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
When the onboard config file is not available (e.g. when running inside the sandbox), the plugin banner in `nemoclaw/src/index.ts` hardcodes the model as `nvidia/nemotron-3-super-120b-a12b` and the endpoint as `build.nvidia.com` regardless of what is actually configured in OpenShell.
This PR queries the live OpenShell inference state via `openshell inference get --json` as a fallback before resorting to hardcoded defaults. The probe has a 3-second timeout and falls back cleanly to the existing defaults if anything goes wrong.
Closes #24.
Changes
Test plan
(Resubmitting — prior PR #1816 was auto-closed by the 10-PR-cap check when our open PR count briefly exceeded the limit; we're now under the cap.)
🤖 Generated with Claude Code
Summary by CodeRabbit
Signed-off-by: Intern Dev dev@wukongai.io