fix(doctor): merge colliding model-ref map keys instead of dropping#96544
Conversation
|
Codex review: needs maintainer review before merge. Reviewed June 30, 2026, 8:04 PM ET / 00:04 UTC. Summary PR surface: Source +106, Tests +201. Total +307 across 2 files. Reproducibility: yes. Source inspection shows current main upgrades multiple retired refs to the same Review metrics: 1 noteworthy metric.
Stored data model Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Land the doctor-only migration after maintainer acceptance of canonical/existing-wins conflict semantics, keeping the focused regression coverage and exact-head green CI. Do we have a high-confidence way to reproduce the issue? Yes. Source inspection shows current main upgrades multiple retired refs to the same Is this the best way to solve the issue? Yes, subject to maintainer acceptance of the conflict precedence. Doctor migration is the right boundary because runtime should consume canonical config, and the PR keeps the repair in legacy normalization with focused tests and blocked-key filtering. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 5e572dcf781a. Label changesLabel justifications:
Evidence reviewedPR surface: Source +106, Tests +201. Total +307 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
|
|
Rebased onto current origin/main (6830aa3) to clear the merge conflict. The only conflicts were two auto-generated plugin-SDK surface files (docs/.generated/plugin-sdk-api-baseline.sha256 and scripts/plugin-sdk-surface-report.mjs); this fix adds no public SDK exports, so both were taken from main and the collision-merge change is unchanged. New head ea38584. Re-verified RED to GREEN on the new head via the real applyLegacyDoctorMigrations runner: pristine main drops the colliding entry's alias (data loss, no warning), patched tree merges and keeps it with an explicit merge change-log line. Regression suite 119 passed; oxlint, oxfmt --check, and tsgo:core all clean on the changed files. PR is MERGEABLE. Proof block in the description is refreshed to the new head. @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
Refreshed the real before/after behavior proof and re-pinned it to the current head Both runs drove the real
One-line difference: the merged Prototype safety reverified: an unguarded Regression suite at this head: @clawsweeper re-review |
|
Added a third real before/after proof scenario for the canonical-vs-retired branch (the new Scenario: the models map already contains a canonical
One-line difference: AFTER keeps the canonical Regression suite at this head: @clawsweeper re-review |
|
Addressed both review findings on the merge helper and re-pinned proof to head P1 (filter blocked keys from both sides):
P3 (report the retired key when the canonical entry follows it): the migration now tracks the retired source key per upgraded slot. When a retired key is processed before the canonical key it upgrades onto, the change-log names the retired source key instead of the canonical key against itself.
Gates at this head: @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
Land-ready maintainer proof for exact head
Ready to merge. |
When two retired model-ref keys in a models map upgrade to the same current ref, rewriteModelRefMapKeys dropped the second entry silently and still logged a success line, so doctor --fix lost one entry's params/streaming/alias/agentRuntime tuning while falsely reporting it as upgraded. Mirror the sibling provider-models-array merge path (openclaw#90047): deep-merge the colliding entries preserving disjoint fields, keep the existing value deterministically on a true field conflict, and push a merge or collision change-log line instead of dropping.
…p entries mergeModelRefMapEntries copied arbitrary incoming config fields into a plain object, so a colliding model entry carrying a __proto__ field could mutate the merged entry prototype during legacy config migration. Skip isBlockedObjectKey fields before assigning or recursing, matching the shared config merge guard. Disjoint-field merging is unchanged.
|
Merged via squash.
|
What Problem This Solves
openclaw doctor --fixupgrades retired model refs in map keys such asagents.defaults.modelsandagents.list[].models. When multiple retired keys upgrade to the same canonical ref—or a retired key upgrades onto an already-authored canonical key—the shipped migration logged each upgrade but silently skipped every later collision. Users could therefore loseparams,alias,streaming, oragentRuntimetuning while doctor reported success.Both
openai/gpt-4andopenai/gpt-4o, for example, currently upgrade toopenai/gpt-5.5. The skip behavior entered inb6530beb05da7441f1cbd832d470feb8a06dd4eeand shipped in stable releases beginning withv2026.5.26.Why This Change Was Made
The fix stays in the doctor-owned legacy migration boundary. It now:
Existing merge helpers were audited but do not match this contract:
mergeMissinglacks conflict reporting and recursive sanitization, RFC merge-patch treatsnullas deletion, and the include merge concatenates arrays/source-wins. The focused helper here preserves arbitrary tuning values while keeping one canonical migration path.Issue #90047 is a sibling precedent for collision-safe provider-array migration, not the report for this map-key bug.
User Impact
After an upgrade,
openclaw doctor --fixpreserves disjoint tuning from every retired alias that converges on a canonical model. Explicit canonical values win deterministically, and the doctor report names only genuine conflicts instead of claiming upgrades for discarded or effectively equal values.No new config key, runtime fallback, or compatibility surface is added.
Evidence
Final head:
9c9fe8dd1cnode scripts/run-vitest.mjs src/commands/doctor/shared/legacy-config-migrate.test.ts— 124/124 passed.openclaw doctor --fix --non-interactivepersisted-config proof — Blacksmith Testboxtbx_01kwdegncjbcjhv1m0jcetfx66, Actions run 28483061881, passed.corepack pnpm check:changed— Blacksmith Testboxtbx_01kwdeh9qh34esmmqmrfx09eb1, Actions run 28483076398, passed core/coreTests typecheck, lint, import-cycle, and policy gates.oxfmt --checkon both changed files andgit diff --check— passed.Focused coverage includes canonical-first and canonical-last order, multiple aliases converging on one key, nested conflicts, normalized-equality logs, per-agent maps, malformed scalar entries, recursive blocked keys on both sides, and root prototype-setter safety.