fix(models): honor replace mode in list output#103103
Conversation
|
Codex review: needs real behavior proof before merge. Reviewed July 10, 2026, 3:25 AM ET / 07:25 UTC. Summary PR surface: Source +20, Tests +296. Total +316 across 9 files. Reproducibility: yes. The exact-head source deterministically exposes both failures with Review metrics: 1 noteworthy metric.
Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Risk before merge
Maintainer options:
Next step before merge
Security Review findings
Review detailsBest possible solution: Normalize configured model IDs using the raw provider first, then canonicalize only the rendered key while carrying raw and canonical provider candidates separately for auth availability checks. Do we have a high-confidence way to reproduce the issue? Yes. The exact-head source deterministically exposes both failures with Is this the best way to solve the issue? No. The replace-mode source boundary is the right approach, but the current candidate shape conflates configured, display, and auth identities that have different contracts. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 51c1c08227db. Label changesLabel justifications:
Evidence reviewedPR surface: Source +20, Tests +296. Total +316 across 9 files. View PR surface stats
Acceptance criteria:
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
Review history (3 earlier review cycles)
|
clawsweeper
Bot
added
rating: 🦪 silver shellfish
7d01ae6 to
c45ceb8
Compare
openclaw-barnacle
Bot
added
commands
c45ceb8 to
6483ffd
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6483ffd019
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| params.context.cfg.models?.mode !== "replace" && | ||
| !shouldListConfiguredProviderModel({ providerConfig, model: configuredModel }) |
There was a problem hiding this comment.
When models.mode: "replace" is used with a manifest provider alias in models.providers (for example "z.ai") and the model relies on plugin transport inference with no api, this new branch lets the row through but the code below still builds the row key/model from the raw provider key. resolveConfiguredEntries canonicalizes the same default/allowlist ref to zai/..., so seenKeys will not dedupe it and the default list can render both zai/glm-4.7 and z.ai/glm-4.7 (or expose only the raw alias for non-default configured rows), regressing the canonical configured-key behavior covered by the z.ai list tests. Canonicalize the provider before computing key/model for these replace-mode rows.
Useful? React with 👍 / 👎.
6483ffd to
5a3fb92
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5a3fb92c36
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| continue; | ||
| } | ||
| const key = modelKey(provider, configuredModel.id); | ||
| const key = modelKey(canonicalProvider, configuredModel.id); |
There was a problem hiding this comment.
When models.providers is keyed by an alias or non-normalized provider id (for example z.ai or XIAOMI) and the default/allowlist entry resolves to the canonical provider, appendConfiguredRows runs first and can only build a synthetic fallback because findConfiguredProviderModel looks up cfg.models.providers[entry.ref.provider] exactly. Fresh evidence in this version is that the provider-row key is now canonicalized here, so seenKeys treats the real configured provider row as a duplicate and skips it, leaving the default list with the fallback name/input/context instead of the configured model metadata in replace mode.
Useful? React with 👍 / 👎.
5a3fb92 to
3ea2f68
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3ea2f68fd8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| continue; | ||
| } | ||
| const key = modelKey(provider, configuredModel.id); | ||
| const key = modelKey(canonicalProvider, configuredModel.id); |
There was a problem hiding this comment.
When models.mode: "replace" is used with a configured provider model that relies on transport inference and has a model id normalized elsewhere (for example google/gemini-3-pro-preview, which parseModelRef/configured catalogs normalize to gemini-3.1-pro-preview), this new replace-mode path adds a row keyed with the raw configuredModel.id. appendConfiguredRows has already emitted the canonical default/allowlist key, so seenKeys will not de-dupe it and the list can show both the canonical fallback row and the raw configured row, or expose non-default rows under an unselectable stale key. Normalize the configured model id before building key/model here.
Useful? React with 👍 / 👎.
Replace the stale fork PR with the reviewed fix on current main. Refs: #94735 Co-authored-by: 毛宇豪0668001457 <[email protected]>
3ea2f68 to
f7bb38f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f7bb38f2a0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| seenKeys: Set<string>; | ||
| }): Promise<void> { | ||
| // Replace narrows default configured lists; explicit browse paths skip this helper. | ||
| if (params.context.cfg.models?.mode === "replace") { |
There was a problem hiding this comment.
When models.mode: "replace" is set but agents.defaults.model.primary is absent or still points at a provider not present in models.providers, this guard only skips the authenticated catalog after appendConfiguredRows has already emitted that default/fallback row. The default models list output can still show something like openai/gpt-5.4 alongside the explicit provider rows, even though replace-mode CLI listings are supposed to be limited to models.providers.*.models (docs/concepts/models.md:67); filter configured entries against the configured provider models before appending them in replace mode.
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
rating: 🧂 unranked krab
Co-authored-by: 毛宇豪0668001457 <[email protected]>
|
Holding this replacement at exact head Two final P2 defects remain:
Exact-head CI run 29057561329, focused Testbox tests, The next design must keep canonical row identity separate from raw/source auth-provider candidates, and must collapse the raw provider/model key before applying provider-alias canonicalization. Add focused coverage for both states before this leaves draft. |
clawsweeper
Bot
added
rating: 🦐 gold shrimp
1 participant
What Problem This Solves
Fixes #94705. Supersedes #94735 because its stale fork branch could not safely accept the current-main maintainer refactor.
With
models.mode: "replace", the defaultopenclaw models listoutput still appended models from authenticated providers that were absent frommodels.providers. That contradicted replace-mode configuration semantics and made the configured-model list look broader than the effective configuration.Why This Change Was Made
The authenticated-catalog append helper now reads the canonical config directly and returns early in replace mode. This keeps the policy at the row source that introduces the leak, avoids threading a derived boolean through the row-building context, and leaves explicit
--alland--providerbrowsing on their existing source-plan paths.The regression coverage exercises both merge and replace semantics, proves replace mode skips the authenticated catalog entirely, and separately protects explicit all/provider browsing. The replacement commit preserves original contributor credit for @Monkey-wusky.
User Impact
openclaw models listnow shows only explicitly configured provider models whenmodels.modeisreplace. Users can still browse the complete catalog with--allor narrow catalog browsing with--provider.Evidence
bb6a70e0c257cd2213670478156a638cc1c5ac97:src/commands/models.list.e2e.test.tspassed 20/20 (run_59f7ce52973a).custom/only, while--alland--provider xiaomiretained three Xiaomi catalog rows (run_c2b46c7f05e9).c45ceb843c0c045ca594eae0af5c47affb5f9d04, Testbox-through-Crabboxtbx_01kx4c0dgee775a7n3cvyy5512: forward-compat tests passed 29/29 and model-list E2E passed 20/20.pnpm check:changedpassed on Testbox, including core/core-test typechecks, changed-file lint, and repository guards.--allretained it plus three Xiaomi rows;--provider xiaomireturned only those three Xiaomi rows.patch is correct, confidence 0.97).