Skip to content

fix(doctor): merge colliding model-ref map keys instead of dropping#96544

Merged
steipete merged 5 commits into
openclaw:mainfrom
yetval:fix/doctor-model-map-key-collision-merge
Jul 1, 2026
Merged

fix(doctor): merge colliding model-ref map keys instead of dropping#96544
steipete merged 5 commits into
openclaw:mainfrom
yetval:fix/doctor-model-map-key-collision-merge

Conversation

@yetval

@yetval yetval commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

What Problem This Solves

openclaw doctor --fix upgrades retired model refs in map keys such as agents.defaults.models and agents.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 lose params, alias, streaming, or agentRuntime tuning while doctor reported success.

Both openai/gpt-4 and openai/gpt-4o, for example, currently upgrade to openai/gpt-5.5. The skip behavior entered in b6530beb05da7441f1cbd832d470feb8a06dd4ee and shipped in stable releases beginning with v2026.5.26.

Why This Change Was Made

The fix stays in the doctor-owned legacy migration boundary. It now:

  • gives an authored canonical key precedence regardless of whether it appears before or after retired aliases;
  • recursively fills only missing fields from every converging retired alias;
  • reports sorted leaf conflict paths only when values remain different after model-ref normalization;
  • attributes each merge to the actual retired source key;
  • recursively filters blocked prototype keys from both collision inputs; and
  • rebuilds records and the final config root with safe own-property definitions so hostile keys cannot invoke prototype setters.

Existing merge helpers were audited but do not match this contract: mergeMissing lacks conflict reporting and recursive sanitization, RFC merge-patch treats null as 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 --fix preserves 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: 9c9fe8dd1c

  • node scripts/run-vitest.mjs src/commands/doctor/shared/legacy-config-migrate.test.ts — 124/124 passed.
  • Real isolated openclaw doctor --fix --non-interactive persisted-config proof — Blacksmith Testbox tbx_01kwdegncjbcjhv1m0jcetfx66, Actions run 28483061881, passed.
  • Remote corepack pnpm check:changed — Blacksmith Testbox tbx_01kwdeh9qh34esmmqmrfx09eb1, Actions run 28483076398, passed core/coreTests typecheck, lint, import-cycle, and policy gates.
  • oxfmt --check on both changed files and git diff --check — passed.
  • Fresh autoreview after all fixes — no accepted/actionable findings, overall confidence 0.97.

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.

@openclaw-barnacle openclaw-barnacle Bot added commands Command implementations size: S labels Jun 24, 2026
@clawsweeper

clawsweeper Bot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed June 30, 2026, 8:04 PM ET / 00:04 UTC.

Summary
The PR changes the legacy doctor retired-model-ref migration to deep-merge colliding models map entries, preserve canonical precedence, filter prototype keys, and add focused regression coverage.

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 openai/gpt-5.5 key and skips a later colliding entry; the PR discussion also includes before/after output from the real migration runner.

Review metrics: 1 noteworthy metric.

  • Persisted migration behavior: 1 doctor model-ref map merge path changed. The diff changes how openclaw doctor --fix rewrites persisted model-map keys during upgrades, so maintainers should review the compatibility semantics before merge.

Stored data model
Persistent data-model change detected: migration/backfill/repair: src/commands/doctor/shared/legacy-config-migrate.test.ts, migration/backfill/repair: src/commands/doctor/shared/legacy-config-migrations.runtime.models.ts, unknown-data-model-change: src/commands/doctor/shared/legacy-config-migrate.test.ts, unknown-data-model-change: src/commands/doctor/shared/legacy-config-migrations.runtime.models.ts. Confirm migration or upgrade compatibility proof before merge.

Merge readiness
Overall: 🦞 diamond lobster
Proof: 🦞 diamond lobster
Patch quality: 🦞 diamond lobster
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Risk before merge

  • [P1] The PR changes conflict precedence for persisted agents.defaults.models and agents.list[].models entries during openclaw doctor --fix; maintainers should explicitly accept canonical/existing-wins semantics before merge.
  • [P1] The merge helper combines user-controlled config objects during migration; the patch uses blocked-key filtering and own-property writes, but the path remains security-boundary-sensitive.

Maintainer options:

  1. Land After Semantic Acceptance (recommended)
    Accept the canonical/existing-wins doctor merge policy because the patch preserves more user config and reports conflicts instead of silently dropping entries.
  2. Require A Different Conflict Policy
    Pause the PR if maintainers want a different precedence rule for canonical, existing, and retired alias values before this migration ships.

Next step before merge

  • No automated repair is needed; the remaining action is maintainer acceptance of the compatibility- and security-sensitive doctor merge semantics.

Security
Cleared: No concrete supply-chain issue was found; the diff changes no dependencies or workflows and hardens prototype-key handling while merging config objects.

Review details

Best 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 openai/gpt-5.5 key and skips a later colliding entry; the PR discussion also includes before/after output from the real migration runner.

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 changes

Label justifications:

  • P2: This is a normal-priority doctor migration bug fix with limited code surface, but it affects persisted user config during upgrades.
  • merge-risk: 🚨 compatibility: The PR changes doctor conflict precedence for persisted agents.defaults.models and agents.list[].models entries when retired refs collide.
  • merge-risk: 🚨 security-boundary: The PR merges user-controlled config objects and relies on prototype-pollution guards while rewriting migration output.
  • rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The PR body and comments include copied before/after output from the real doctor migration path showing dropped tuning before and preserved tuning after the fix.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body and comments include copied before/after output from the real doctor migration path showing dropped tuning before and preserved tuning after the fix.
Evidence reviewed

PR surface:

Source +106, Tests +201. Total +307 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 1 111 5 +106
Tests 1 201 0 +201
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 312 5 +307

What I checked:

Likely related people:

  • Peter Steinberger: Authored the commit that added the retired model-ref migration, authored adjacent OpenAI Codex doctor-only migration work, and authored the latest PR-head refinement. (role: introduced behavior and recent area contributor; confidence: high; commits: b6530beb05da, 12c6c588eead, 88a3a2a55ae9; files: src/commands/doctor/shared/legacy-config-migrations.runtime.models.ts, src/commands/doctor/shared/legacy-config-migrate.test.ts)
  • Vincent Koc: Authored the branch revision that made canonical model migration values win in the collision path, which is central to the proposed merge semantics. (role: recent PR-branch contributor; confidence: medium; commits: ea385840f672; files: src/commands/doctor/shared/legacy-config-migrations.runtime.models.ts, src/commands/doctor/shared/legacy-config-migrate.test.ts)
  • luyifan: Added Codex migration coverage linked from the related closed migration issue; this is adjacent context rather than ownership of the exact map-key helper. (role: adjacent migration coverage contributor; confidence: low; commits: 906e342e7d2c; files: src/commands/doctor/shared/codex-route-warnings.test.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

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
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 security-boundary 🚨 May affect sandboxing, authorization, credentials, or sensitive data. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. merge-risk: 🚨 security-boundary 🚨 May affect sandboxing, authorization, credentials, or sensitive data. labels Jun 24, 2026
@openclaw-barnacle openclaw-barnacle Bot added docs Improvements or additions to documentation channel: imessage Channel integration: imessage channel: signal Channel integration: signal channel: whatsapp-web Channel integration: whatsapp-web gateway Gateway runtime extensions: open-prose Extension: open-prose scripts Repository scripts docker Docker and sandbox tooling agents Agent runtime and tooling extensions: tavily extensions: duckduckgo channel: qqbot extensions: qa-lab extensions: firecrawl extensions: ollama extensions: openshell extensions: perplexity labels Jun 25, 2026
@yetval

yetval commented Jun 26, 2026

Copy link
Copy Markdown
Contributor Author

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

@clawsweeper

clawsweeper Bot commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@yetval

yetval commented Jun 26, 2026

Copy link
Copy Markdown
Contributor Author

Refreshed the real before/after behavior proof and re-pinned it to the current head ea385840f672cbbf20b2abdb4f799f7112d96fc0 (base: pristine main 6830aa39eaa16429daec9e03688e6e04e9e75841).

Both runs drove the real applyLegacyDoctorMigrations runner (src/commands/doctor/shared/legacy-config-compat.ts) on identical input, with no external seam mocked:

  • BEFORE (pristine main): the two retired keys openai/gpt-4o and openai/gpt-4 both upgrade to openai/gpt-5.5, the surviving entry is { params: { reasoning_effort: "high" }, streaming: false }, and alias: "legacy-four" from the second key is silently dropped. Only two Upgraded ... key ... lines are logged, falsely reporting the dropped key as upgraded.
  • AFTER (this head, identical input): the surviving entry is { params: { reasoning_effort: "high" }, streaming: false, alias: "legacy-four" }, and the change log adds Merged config.agents.defaults.models key "openai/gpt-4" into "openai/gpt-5.5"; kept existing values for conflicting fields: params.reasoning_effort.

One-line difference: the merged openai/gpt-5.5 entry keeps alias: "legacy-four" (and a truthful merge change-log line) instead of losing it.

Prototype safety reverified: an unguarded Object.assign merge of a colliding entry carrying a JSON __proto__ field swaps the merged entry prototype (getPrototypeOf !== Object.prototype, merged.polluted === true); this head's guarded merge keeps getPrototypeOf === Object.prototype and merged.polluted === undefined.

Regression suite at this head: node scripts/run-vitest.mjs src/commands/doctor/shared/legacy-config-migrate.test.ts -> 119 passed.

@clawsweeper re-review

@yetval

yetval commented Jun 26, 2026

Copy link
Copy Markdown
Contributor Author

Added a third real before/after proof scenario for the canonical-vs-retired branch (the new mergeModelRefMapEntries + canonical-key tracking path), re-pinned to head ea385840f672cbbf20b2abdb4f799f7112d96fc0 (base: pristine main 6830aa39eaa16429daec9e03688e6e04e9e75841). The existing retired/retired collision and prototype-safety scenarios are unchanged.

Scenario: the models map already contains a canonical openai/gpt-5.5 ({ params: { reasoning_effort: "medium" }, streaming: true }) AND a retired openai/gpt-4 that upgrades onto it ({ params: { reasoning_effort: "low" }, alias: "legacy-four" }). Driven through the real applyLegacyDoctorMigrations on identical input:

  • BEFORE (pristine main): the retired entry hits the continue branch and is silently dropped. Surviving openai/gpt-5.5 = { params: { reasoning_effort: "medium" }, streaming: true }; the disjoint alias: "legacy-four" is lost and only the lone Upgraded ... key from "openai/gpt-4" to "openai/gpt-5.5". line is logged.
  • AFTER (this head, identical input): surviving openai/gpt-5.5 = { params: { reasoning_effort: "medium" }, streaming: true, alias: "legacy-four" }. The canonical params.reasoning_effort: "medium" is kept on conflict, the retired entry's disjoint alias is merged in, and the log adds Merged config.agents.defaults.models key "openai/gpt-4" into "openai/gpt-5.5"; kept existing values for conflicting fields: params.reasoning_effort.

One-line difference: AFTER keeps the canonical params.reasoning_effort: "medium" on conflict and merges the retired entry's disjoint alias: "legacy-four" into openai/gpt-5.5; BEFORE drops the retired entry entirely (alias lost) with no merge line.

Regression suite at this head: node scripts/run-vitest.mjs src/commands/doctor/shared/legacy-config-migrate.test.ts -> 119 passed. No fix-code or test changes in this refresh, only the PR body Evidence block.

@clawsweeper re-review

@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. labels Jun 26, 2026
@yetval

yetval commented Jun 26, 2026

Copy link
Copy Markdown
Contributor Author

Addressed both review findings on the merge helper and re-pinned proof to head ac1bd125ad6e28507eb6d65064b19e9bf229c1e0 (base: pristine main 6830aa39eaa16429daec9e03688e6e04e9e75841).

P1 (filter blocked keys from both sides): mergeModelRefMapEntries no longer spreads the existing entry wholesale. It now filters isBlockedObjectKey fields when copying the existing side too, matching the incoming side. A blocked key on the FIRST/existing colliding entry can no longer survive into the merged object or mutate its prototype.

  • Driven through the real applyLegacyDoctorMigrations with __proto__ on the first entry plus a retired fallback (which forces the entry rewrite that realizes the mutation):
    • BEFORE (prior single-sided head ea385840f672): merged entry prototype is Object.prototype: false; merged.polluted: true.
    • AFTER (this head): merged entry prototype is Object.prototype: true; merged.polluted: undefined; merged = { fallback:["openai/gpt-5.5"], streaming:false, alias:"legacy-four" }.
  • New regression test does not pollute prototypes when the first colliding entry carries a blocked key is RED on the prior revision, GREEN now.

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.

  • Input { "openai/gpt-4": {...}, "openai/gpt-5.5": {...} }:
    • BEFORE (prior head): Merged ... key "openai/gpt-5.5" into "openai/gpt-5.5"; kept ...
    • AFTER (this head): Merged ... key "openai/gpt-4" into "openai/gpt-5.5"; kept ...
  • New regression test names the retired source key when the canonical key follows the retired key is RED on the prior revision, GREEN now.

Gates at this head: legacy-config-migrate.test.ts -> 121 passed; oxlint, oxfmt --check, and tsgo -p tsconfig.core.json / -p test/tsconfig/tsconfig.core.test.json all clean on the changed files. PR is MERGEABLE. Source diff is +19/-7; the rest is regression coverage.

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@steipete

steipete commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Land-ready maintainer proof for exact head 9c9fe8dd1c0f680b06f0e4f356c6b3fd1a4b7a39:

  • Maintainer semantic decision: accept authored-canonical-wins precedence. It is independent of key order; every retired alias contributes disjoint fields; conflicts are reported only when final normalized values differ.
  • Focused regression suite: node scripts/run-vitest.mjs src/commands/doctor/shared/legacy-config-migrate.test.ts — 124/124 passed.
  • Real user path: isolated OPENCLAW_CONFIG_PATH=... corepack pnpm openclaw doctor --fix --non-interactive — Blacksmith Testbox tbx_01kwdegncjbcjhv1m0jcetfx66, run 28483061881. Persisted JSON assertion passed with canonical fields plus disjoint tuning from both retired aliases; doctor emitted correctly attributed leaf-conflict lines.
  • Broad changed gate: env CI=true OPENCLAW_CHECK_CHANGED_REMOTE_CHILD=1 OPENCLAW_CHANGED_LANES_RAW_SYNC=1 corepack pnpm check:changed (after restoring the delegated shallow checkout's origin/main ancestry) — Blacksmith Testbox tbx_01kwdeh9qh34esmmqmrfx09eb1, run 28483076398. Core/coreTests typecheck, lint, import-cycle, and policy gates passed.
  • Fresh .agents/skills/autoreview/scripts/autoreview --mode local --stream-engine-output after all fixes — no accepted/actionable findings, confidence 0.97.
  • Exact-head hosted CI run 28484156406 is complete with zero pending/failed checks.
  • OPENCLAW_TESTBOX=1 scripts/pr prepare-run 96544 verified the hosted exact-head gates and remote tree; prep head and PR head both resolve to 9c9fe8dd1c0f680b06f0e4f356c6b3fd1a4b7a39.
  • Review artifacts validate, and there are no unresolved review threads or known proof gaps.

Ready to merge.

yetval and others added 5 commits June 30, 2026 17:04
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.
@steipete

steipete commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Merged via squash.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

commands Command implementations merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. merge-risk: 🚨 security-boundary 🚨 May affect sandboxing, authorization, credentials, or sensitive data. P2 Normal backlog priority with limited blast radius. proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. size: M status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants