fix(models): prefer exact configured provider refs before aliases#88232
Conversation
|
Codex review: needs maintainer review before merge. Reviewed May 30, 2026, 1:08 PM ET / 17:08 UTC. Summary PR surface: Source +80, Tests +182. Total +262 across 2 files. Reproducibility: yes. at source level. Current main checks reverse alias candidates before slash-form parsing, and the linked issue plus PR proof exercise the exact alias-target collision without requiring a live provider call. Review metrics: 1 noteworthy metric.
Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Land one canonical resolver fix that preserves explicit provider/model refs ahead of bare reverse aliases while keeping slash-form alias behavior and auth-profile alias coverage, then close the duplicate implementation path after merge. Do we have a high-confidence way to reproduce the issue? Yes, at source level. Current main checks reverse alias candidates before slash-form parsing, and the linked issue plus PR proof exercise the exact alias-target collision without requiring a live provider call. Is this the best way to solve the issue? Yes, mostly. Moving exact configured provider refs and bare slash-form primaries ahead of bare reverse aliases is the narrow fix; the remaining maintainer decision is which duplicate PR should become canonical. AGENTS.md: found and applied where relevant. Codex review notes: model gpt-5.5, reasoning high; reviewed against 522da25932b3. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source +80, Tests +182. Total +262 across 2 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
c28ca80 to
0f30cfb
Compare
0f30cfb to
2886e87
Compare
|
Updated this branch for the alias-precedence review feedback.
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
Refreshed this branch on upstream/main at Validation:
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
Refreshed this branch on upstream/main at Validation:
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
538b40e to
ac8d391
Compare
ac8d391 to
b21fad2
Compare
|
Verification for head b21fad2. Behavior addressed: exact configured provider/model defaults now win before reverse alias matches, while slash-form aliases and auth-profile alias stripping keep their previous precedence. Real environment tested: local source checkout, GitHub CI on run 26689708781. Exact steps or command run after this patch:
Evidence after fix: local focused model-selection tests passed, 115 tests in src/agents/model-selection.test.ts. Autoreview reported clean with no accepted/actionable findings, overall patch correct 0.83. GitHub CI run 26689708781 passed the changed-surface and model-relevant checks, including Real behavior proof, checks-node-agentic-commands-models, check-prod-types, check-test-types, check-lint, check-guards, security-fast, actionlint, build-artifacts, CodeQL, and Opengrep OSS. Observed result after fix: configured exact provider refs such as provider/model are preserved instead of being collapsed through bare alias reverse lookup; profile-qualified alias behavior remains covered. What was not tested: no live provider call against an actual self-hosted Nemotron gateway backend. Two broad unrelated CI shards, checks-node-agentic-commands-doctor and checks-node-core-runtime-infra-state, failed twice by Vitest no-output timeout only, with no assertion failure in the touched model-selection surface. |
steipete
left a comment
There was a problem hiding this comment.
Reviewed the model-selection precedence change and follow-up fixups. Focused regression coverage now covers exact configured provider refs, slash-form aliases, and auth-profile alias stripping.
Summary
What problem does this PR solve?
Why does this matter now?
What is the intended outcome?
What is intentionally out of scope?
What does success look like?
What should reviewers focus on?
Linked context
Which issue does this close?
Closes #88218
Which issues, PRs, or discussions are related?
Related #88218
Was this requested by a maintainer or owner?
Real behavior proof (required for external PRs)
Behavior or issue addressed: exact configured custom provider refs were being reverse-matched by agents.defaults.models alias values before the ref itself was parsed.
Real environment tested: local OpenClaw source checkout on Linux with Node/pnpm, running the actual resolver implementation from src/agents/model-selection.ts.
Exact steps or command run after this patch:
COREPACK_HOME=/tmp/corepack pnpm exec tsx -e '
import { resolveConfiguredModelRef } from "./src/agents/model-selection.ts";
const cfg = {
agents: { defaults: { model: { primary: "nemotron-bolt/nemotron-3-super-120b" }, models: { nemotron: { alias: "nemotron-bolt/nemotron-3-super-120b" } } } },
models: { providers: { "nemotron-bolt": { api: "openai-completions", baseUrl: "http://127.0.0.1:8080/v1", models: [{ id: "nemotron-3-super-120b", name: "Nemotron" }] } } },
};
console.log(JSON.stringify(resolveConfiguredModelRef({ cfg: cfg as any, defaultProvider: "openai", defaultModel: "gpt-5.4" })));
'
Evidence after fix: Terminal console output copied from the command above:
{"provider":"nemotron-bolt","model":"nemotron-3-super-120b"}
Observed result after fix: the configured primary resolves to the custom provider/model instead of openai/nemotron.
What was not tested: a live gateway boot with a real self-hosted Nemotron backend.
Proof limitations or environment constraints: the issue is deterministic in the resolver and does not require contacting the model provider; the proof executes the production resolver path directly rather than starting a full gateway.
Before evidence (optional but encouraged): the added regression test failed before the implementation with Received { provider: "openai", model: "nemotron" }.
Tests and validation
Which commands did you run?
What regression coverage was added or updated?
What failed before this fix, if known?
If no test was added, why not?
Risk checklist
Did user-visible behavior change? (Yes/No)
Yes.
Did config, environment, or migration behavior change? (Yes/No)
No migration or environment change. Existing config resolution behavior changes for the reported alias-target collision.
Did security, auth, secrets, network, or tool execution behavior change? (Yes/No)
No.
What is the highest-risk area?
How is that risk mitigated?
Current review state
What is the next action?
What is still waiting on author, maintainer, CI, or external proof?
Which bot or reviewer comments were addressed?