fix(inference): refresh agent primary model on rebuild after provider switch - #7063
fix(inference): refresh agent primary model on rebuild after provider switch#7063jason-ma-nv wants to merge 2 commits into
Conversation
… switch When a sandbox is rebuilt, the OpenClaw config restore merge overlays the durable backup over the freshly generated config. The `agents` section is backup-durable, so `agents.defaults.model.primary` (and the matching main/default `agents.list[].model`) was inherited from the snapshot — which still pointed at the model selected before an inference provider switch. The `models.providers` routing was already re-owned from the fresh rebuild, so the routing table was correct while the agent kept calling the old model, producing the stale TUI route and "provider rejected the request schema or tool payload" failures reported in the DGX Station switch scenario. Re-own the agent's primary model routing from the fresh rebuild after the overlay, mirroring updateAgentPrimary/updatePrimaryAgentListModel in the inference-set path. Only override when the fresh config carries a primary, so backups with no rebuild-owned routing are left untouched. Closes #7011 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Jason Ma <jama@nvidia.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)
📝 WalkthroughWalkthroughOpenClaw restored-config merging now reconciles agent primary model routing with the freshly rebuilt configuration and preserves backup agent settings when no fresh primary model is provided. ChangesAgent model reconciliation
Estimated code review effort: 3 (Moderate) | ~20 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 |
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/state/openclaw-config-merge.test.ts (1)
90-142: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the no-
maindefault-agent fallback.The provider-switch test exercises only
id: "main"; thedefault: truefallback is untested. Add a fresh-primary case with nomainagent and assert that the default agent’s string model is updated.Proposed regression case
+ it("refreshes the default agent model when no main agent exists (`#7011`)", () => { + const merged = mergeOpenClawRestoredConfig( + { + agents: { + defaults: { model: { primary: "inference/old-model" } }, + list: [{ id: "researcher", default: true, model: "inference/old-model" }], + }, + }, + { + agents: { + defaults: { model: { primary: "inference/new-model" } }, + }, + }, + ); + + expect(merged).toMatchObject({ + agents: { + list: [{ id: "researcher", default: true, model: "inference/new-model" }], + }, + }); + });🤖 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/state/openclaw-config-merge.test.ts` around lines 90 - 142, Add a regression test alongside the provider-switch test for a configuration with no agent having id "main" but with a default: true agent. Use a fresh primary model from the rebuild and assert that the default agent’s string model is updated to that fresh primary while preserving the existing merge behavior.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/state/openclaw-config-merge.test.ts`:
- Around line 90-142: Add a regression test alongside the provider-switch test
for a configuration with no agent having id "main" but with a default: true
agent. Use a fresh primary model from the rebuild and assert that the default
agent’s string model is updated to that fresh primary while preserving the
existing merge behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a11410a1-a740-43e7-8bf0-7c5595bcdf9d
📒 Files selected for processing (2)
src/lib/state/openclaw-config-merge.test.tssrc/lib/state/openclaw-config-merge.ts
PR Review Advisor — InformationalAdvisor assessment: Informational / high confidence 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: 3 optional E2E recommendations
This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
|
Heads-up on ownership: issue #7011 is assigned to @yanyunl1991 (who commented "Will take a look"). This PR was opened by an automated fix loop that picked the issue up in the brief window before that assignment was visible, so it was not coordinated with the assignee — apologies for stepping on claimed work. @yanyunl1991 / maintainers, please treat this PR as entirely optional and defer to the assignee: take it over, review/merge it, or supersede it with your own fix, whatever fits work already in progress. The change is a localized stale-field fix in the rebuild config-restore merge ( |
…l reconcile The #7011 regression only exercised the `main`-agent branch of the merge's `updateMainAgentListModel`. Add a case with no `main` agent and a `default: true` agent to cover the fallback selection: the fresh primary re-owns only the selected default agent's string model, while other durable agents are left unchanged. Addresses the CodeRabbit nitpick and PR review advisor PRA-1 warning. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Jason Ma <jama@nvidia.com>
|
Addressed the CodeRabbit nitpick and PR review advisor Verified: Merge decision still deferred to @yanyunl1991 / maintainers per the ownership note above. |
|
Same diagnosis here, and your scoping (re-own exactly Adding an end-to-end reproduction that confirms your fix, in case it's useful — the bug is provider/model-agnostic, so it does not need the reporter's DGX Station / DeepSeek setup. On a DGX Spark with the
Durable agent tuning ( |
wscurran
added
area: inference
|
✨ Related open issues: |
sandl99
left a comment
sandl99
left a comment
There was a problem hiding this comment.
Requesting changes because the current evidence is not sufficient to close #7011.
The regression tests prove that the restore merge rewrites agents.defaults.model.primary and the main/default agents.list[].model. The manual confirmation in this PR likewise stops at comparing inference get with openclaw.json. Neither demonstrates the user-visible acceptance condition from #7011: after switching provider and rebuilding, the agent must send a request to the newly selected model and receive a successful response.
A model-not-found result is not valid evidence for this fix when the target model is not actually served. In a follow-up check, the local vLLM /v1/models endpoint exposed only nvidia/nemotron-3-ultra-550b-a55b, while the route had been changed to deepseek-ai/DeepSeek-V4-Flash. nemoclaw inference set updates the route/configuration; it does not make an already-running vLLM server load an unavailable model.
Before this PR closes #7011, please add reproducible patched-run evidence with all endpoints genuinely available:
- Confirm the starting local vLLM model through
curl http://127.0.0.1:8000/v1/modelsand successfully send it a direct completion request. - Reproduce the issue's provider switch from loaded
vllm-local / deepseek-ai/DeepSeek-V4-Flashto availablenvidia-prod / nvidia/nemotron-3-ultra-550b-a55b(or justify an equivalent provider-agnostic path). - Run the state-preserving sandbox rebuild on the patched branch.
- Verify both agent model bindings use the new model.
- Send a fresh TUI/agent prompt and show a successful response through the new provider, not only the updated
openclaw.jsonvalue.
The code-level diagnosis remains plausible, but closure should wait for this end-to-end acceptance evidence.
|
Closing this PR without merging because the runtime workflow reported in #7011 could not be reproduced. We re-tested a valid model change from vllm-local / nvidia/nemotron-3-ultra-550b-a55b to nvidia-prod / nvidia/nemotron-3-super-120b-a12b. Onboarding regenerated the OpenClaw agent binding, a TUI request succeeded through the new hosted model before rebuild, nemoclaw tm rebuild --yes backed up and restored the sandbox, and a fresh TUI request still succeeded through the new hosted model afterward. The model substitution does not remove the relevant condition: the old and new model IDs and providers were different. The observed workflow never produced the stale backup-model/new-provider split that this PR repairs. The later managed-MCP/progressive-rebuild description introduces an additional prerequisite and a different controlled path that is not present in the original reproduction steps. If that path still fails, it should be reported with a deterministic supported workflow and pre-patch/post-patch runtime evidence. The current unit test demonstrates a constructed merge state, but it does not establish that #7011 reaches that state. |
) (#7216) <!-- markdownlint-disable MD041 --> ## Summary Switching a managed model on a sandbox that has MCP servers completes via `rebuild --yes --tool-disclosure progressive` (the generic onboard recreate is refused to preserve MCP state). After that rebuild the gateway route, the vLLM backend, and `status` show the new model, but `openclaw.json` → `agents.defaults.model.primary` (and `openclaw agents list`) stay on the OLD model. This PR re-owns the agent's primary model routing from the fresh rebuild. Closes #7210. ## Reproduction Reproduced on our DGX Spark aarch64 test host (GB10 GPU) with managed vLLM. The reporter's Station-only models (Nemotron-3-Ultra 550B, DeepSeek-V4-Flash) don't fit a GB10, so two Spark-servable managed profiles are used — `nemotron-3-nano-4b` → `qwen3.6-27b`; the mechanism (config-restore merge, MCP-forced rebuild path) is model-agnostic. Steps: 1. `onboard` managed vLLM model A (`nemotron-3-nano-4b`). 2. `nemoclaw <sb> mcp add deepwiki --url https://mcp.deepwiki.com/mcp --env DEEPWIKI_TOKEN`. 3. `docker stop nemoclaw-vllm` → `onboard` managed vLLM model B (`qwen3.6-27b`), reuse the sandbox → recreate is refused because MCP servers are present → run the directed `nemoclaw <sb> rebuild --yes --tool-disclosure progressive`. 4. Inspect the agent binding. **Observed on `main` (before fix)** — route/backend new, agent binding stale: ``` nemoclaw <sb> status -> Model: Qwen/Qwen3.6-27B-FP8 (NEW, correct) curl :8000/v1/models -> Qwen/Qwen3.6-27B-FP8 (NEW, correct) openclaw agents list (main) -> Model: inference/...Nemotron-3-Nano-4B-FP8 (STALE) openclaw.json agents.defaults.model.primary = inference/...Nemotron-3-Nano-4B-FP8 (STALE) ``` **Observed on `fix/...` (after fix)** — agent binding follows the switch: ``` openclaw agents list (main) -> Model: inference/Qwen/Qwen3.6-27B-FP8 (NEW) openclaw.json agents.defaults.model.primary = inference/Qwen/Qwen3.6-27B-FP8 (NEW) # route/backend unchanged (NEW); MCP server 'deepwiki' still present after rebuild ``` ## Analysis The rebuild config-restore merge (`src/lib/state/openclaw-config-merge.ts` → `mergeOpenClawRestoredConfig`) overlays the backup onto the freshly generated config and then re-owns individual runtime sections (`gateway`/`proxy`/`diagnostics`, `channels`, `models`, `plugins`, `tools`). But `agents` is listed in `backupDurableSections` and is inherited **wholesale** from the pre-switch backup snapshot — including `agents.defaults.model.primary`. So even though `models.providers` routing is refreshed from the registry (new model), the agent's own routing reference (`agents.defaults.model.primary`, and the main/default `agents.list[].model`) stays on the pre-switch model. Restore never reconciled that reference against the fresh rebuild. On the MCP-forced `rebuild --tool-disclosure progressive` path there is no subsequent onboard-recreate to rewrite the config, so the stale primary persists. ## Fix After the section merges, `reconcileAgentPrimaryModel` re-owns the agent's primary model routing from the fresh config: it rewrites `agents.defaults.model.primary` and the matching main/default `agents.list[].model` (agent id `main` wins, else the first `default: true`, and only when its `model` is a string routing reference) from the freshly generated config — and only when the fresh config carries a primary, so backups with no rebuild-owned routing are left untouched. All other agent config (durable tuning like `thinkingDefault`, `customAgents`, and intentional **non-default** per-agent model pins) is still inherited from the backup. A `removalCondition` is documented on the helper. This revives the approach proposed in the previously-closed #7063. Tests lock the whole contract: the stale primary + main list model are re-owned to the fresh model; a non-default per-agent pin and durable tuning are left untouched; and a fresh config with no agent primary leaves the backup routing intact. ## Changes - `src/lib/state/openclaw-config-merge.ts`: add `agentPrimaryModelPath` ownership + `reconcileAgentPrimaryModel` (with `readAgentPrimaryModelRef`/`updateMainAgentListModel`), called at the end of `mergeOpenClawRestoredConfig`. - `src/lib/state/openclaw-config-merge.test.ts`: cover the re-own contract and its boundaries. ## Type of Change - [x] Code change (feature, bug fix, or refactor) ## Verification - [x] Unit tests for the merge contract pass (`openclaw-config-merge.test.ts`, 24 tests) - [x] `npm run typecheck:cli` and `npm run build:cli` pass - [x] Reproduced end-to-end on our DGX Spark aarch64 test host (managed vLLM, MCP present, `rebuild --tool-disclosure progressive`): before the fix the agent primary stayed on the old model; after the fix it follows the switch, consistent with the route/backend, and the MCP server survives the rebuild. - [x] No secrets, API keys, or credentials committed ## AI Disclosure - [x] AI-assisted — tool: Claude Code Signed-off-by: Yanyun Liao <yanyunl@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Fixed configuration restoration to re-assign agent model routing to the newly selected managed primary model after a rebuild. * Preserved durable agent settings and intentional per-agent model assignments during restore. * If the fresh config doesn’t provide a primary model, restoration now keeps the backed-up primary model routing unchanged. * **Tests** * Added coverage for primary-model routing behavior during sandbox re-creation and config merge scenarios. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Yanyun Liao <yanyunl@nvidia.com> Signed-off-by: Aaron Erickson <aerickson@nvidia.com> Co-authored-by: Aaron Erickson <aerickson@nvidia.com> Co-authored-by: cjagwani <cjagwani@nvidia.com>
5 participants
Summary
On sandbox rebuild, the OpenClaw config restore merge overlays the durable snapshot backup over the freshly generated config. The
agentssection is backup-durable, soagents.defaults.model.primary(and the matching main/defaultagents.list[].model) was inherited from the snapshot, which still pointed at the model selected before an inference provider switch.models.providersrouting was already re-owned from the fresh rebuild, so the routing table was correct while the agent kept calling the old model. After this change the merge re-owns the agent's primary model routing from the fresh rebuild, so a provider switch followed by rebuild routes the sandbox agent to the newly selected model.Related Issue
Closes #7011
Changes
src/lib/state/openclaw-config-merge.ts: after the backup overlay,reconcileAgentPrimaryModelre-ownsagents.defaults.model.primaryand the main/defaultagents.list[].modelfrom the fresh rebuild config, mirroringupdateAgentPrimary/updatePrimaryAgentListModelin the inference-set path. It only overrides when the fresh config carries a primary, so backups with no rebuild-owned routing are left untouched.agentsremains backup-durable for all other (genuinely user-owned) fields; the primary model reference is the one carve-out and is documented inOPENCLAW_CONFIG_RESTORE_OWNERSHIP.agentPrimaryModelPath.src/lib/state/openclaw-config-merge.test.ts: regression coverage — the agent primary (and main-agent list model) follows the fresh rebuild after a provider switch, and backup agent config is left unchanged when the rebuild carries no agent primary. The merge unit previously had zeroagentscoverage, which let this stale-field bug escape.Type of Change
Quality Gates
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/state/openclaw-config-merge.test.ts→ 24 passed (incl. 2 new [DGX Station][Inference] OpenClaw agent routes to previous model after switching provider from vllm-local to NVIDIA Endpoints #7011 cases); confirmed red before the fix, green after.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: Jason Ma jama@nvidia.com
Summary by CodeRabbit