mirror of
https://github.com/openclaw/openclaw.git
synced 2026-07-01 03:33:42 +00:00
fix(doctor): merge colliding model-ref map keys instead of dropping (#96544)
* fix(doctor): merge colliding model-ref map keys instead of dropping 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 (#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. * fix(doctor): block prototype keys when merging colliding model-ref map 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. * fix(doctor): preserve canonical model migration values * fix(doctor): filter blocked keys on both merge sides and report retired source key * fix(doctor): preserve colliding retired model settings --------- Co-authored-by: Vincent Koc <vincentkoc@ieee.org> Co-authored-by: Peter Steinberger <steipete@golden-gate.local>
This commit is contained in:
Yuval Dinodia
@@ -2463,6 +2463,207 @@ describe("legacy model compat migrate", () => {
|
||||
]);
|
||||
});
|
||||
|
||||
it("deep-merges colliding retired model refs and reports only unequal fields", () => {
|
||||
const res = migrateLegacyConfigForTest({
|
||||
agents: {
|
||||
defaults: {
|
||||
models: {
|
||||
"openai/gpt-4o": {
|
||||
params: {
|
||||
reasoning: { effort: "high", budget: 100 },
|
||||
tags: ["stable"],
|
||||
},
|
||||
streaming: false,
|
||||
},
|
||||
"openai/gpt-4": {
|
||||
params: {
|
||||
reasoning: { effort: "low", summary: "auto" },
|
||||
tags: ["stable"],
|
||||
},
|
||||
alias: "legacy-four",
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
});
|
||||
|
||||
expect(res.config?.agents?.defaults?.models).toEqual({
|
||||
"openai/gpt-5.5": {
|
||||
params: {
|
||||
reasoning: { effort: "high", budget: 100, summary: "auto" },
|
||||
tags: ["stable"],
|
||||
},
|
||||
streaming: false,
|
||||
alias: "legacy-four",
|
||||
},
|
||||
});
|
||||
expect(res.changes.filter((change) => change.includes("Merged"))).toEqual([
|
||||
'Merged config.agents.defaults.models key "openai/gpt-4" into "openai/gpt-5.5"; kept existing values for conflicting fields: params.reasoning.effort.',
|
||||
]);
|
||||
});
|
||||
|
||||
it("does not report conflicts between model refs that normalize to the same value", () => {
|
||||
const res = migrateLegacyConfigForTest({
|
||||
agents: {
|
||||
defaults: {
|
||||
models: {
|
||||
"openai/gpt-5.5": { params: { fallbacks: ["openai/gpt-4"] } },
|
||||
"openai/gpt-4o": { params: { fallbacks: ["openai/gpt-5.5"] } },
|
||||
},
|
||||
},
|
||||
},
|
||||
});
|
||||
|
||||
expect(res.config?.agents?.defaults?.models).toEqual({
|
||||
"openai/gpt-5.5": { params: { fallbacks: ["openai/gpt-5.5"] } },
|
||||
});
|
||||
expect(res.changes.filter((change) => change.includes("Merged"))).toEqual([
|
||||
'Merged config.agents.defaults.models key "openai/gpt-4o" into "openai/gpt-5.5".',
|
||||
]);
|
||||
});
|
||||
|
||||
it.each(["first", "last"] as const)(
|
||||
"keeps canonical values when the canonical key appears %s",
|
||||
(canonicalPosition) => {
|
||||
const canonical = [
|
||||
"openai/gpt-5.5",
|
||||
{
|
||||
alias: "canonical-five",
|
||||
params: { reasoning: { effort: "medium", canonicalOnly: true } },
|
||||
agentRuntime: { id: "codex" },
|
||||
},
|
||||
] as const;
|
||||
const retired = [
|
||||
[
|
||||
"openai/gpt-4",
|
||||
{
|
||||
alias: "legacy-four",
|
||||
params: { reasoning: { effort: "low", fourOnly: true } },
|
||||
},
|
||||
],
|
||||
[
|
||||
"openai/gpt-4o",
|
||||
{
|
||||
streaming: false,
|
||||
params: { reasoning: { effort: "high", fourOOnly: true } },
|
||||
},
|
||||
],
|
||||
] as const;
|
||||
const entries =
|
||||
canonicalPosition === "first" ? [canonical, ...retired] : [...retired, canonical];
|
||||
const res = migrateLegacyConfigForTest({
|
||||
agents: { defaults: { models: Object.fromEntries(entries) } },
|
||||
});
|
||||
|
||||
expect(res.config?.agents?.defaults?.models).toEqual({
|
||||
"openai/gpt-5.5": {
|
||||
alias: "canonical-five",
|
||||
params: {
|
||||
reasoning: {
|
||||
effort: "medium",
|
||||
canonicalOnly: true,
|
||||
fourOnly: true,
|
||||
fourOOnly: true,
|
||||
},
|
||||
},
|
||||
agentRuntime: { id: "codex" },
|
||||
streaming: false,
|
||||
},
|
||||
});
|
||||
expect(res.changes.filter((change) => change.includes("Merged"))).toEqual([
|
||||
'Merged config.agents.defaults.models key "openai/gpt-4" into "openai/gpt-5.5"; kept existing values for conflicting fields: alias, params.reasoning.effort.',
|
||||
'Merged config.agents.defaults.models key "openai/gpt-4o" into "openai/gpt-5.5"; kept existing values for conflicting fields: params.reasoning.effort.',
|
||||
]);
|
||||
},
|
||||
);
|
||||
|
||||
it("merges colliding model refs in per-agent model maps", () => {
|
||||
const res = migrateLegacyConfigForTest({
|
||||
agents: {
|
||||
list: [
|
||||
{
|
||||
id: "research",
|
||||
models: {
|
||||
"openai/gpt-4": { alias: "legacy-four" },
|
||||
"openai/gpt-4o": { streaming: false },
|
||||
},
|
||||
},
|
||||
],
|
||||
},
|
||||
});
|
||||
|
||||
expect(res.config?.agents?.list?.[0]?.models).toEqual({
|
||||
"openai/gpt-5.5": { alias: "legacy-four", streaming: false },
|
||||
});
|
||||
expect(res.changes.filter((change) => change.includes("Merged"))).toEqual([
|
||||
'Merged config.agents.list.0.models key "openai/gpt-4o" into "openai/gpt-5.5".',
|
||||
]);
|
||||
});
|
||||
|
||||
it.each([
|
||||
{
|
||||
side: "canonical",
|
||||
raw: '{"agents":{"defaults":{"models":{"openai/gpt-5.5":{"__proto__":{"polluted":true},"alias":"canonical-five","params":{"nested":{"__proto__":{"polluted":true},"model":"openai/gpt-4o"}}},"openai/gpt-4":{"streaming":false}}}}}',
|
||||
},
|
||||
{
|
||||
side: "retired",
|
||||
raw: '{"agents":{"defaults":{"models":{"openai/gpt-5.5":{"alias":"canonical-five"},"openai/gpt-4":{"__proto__":{"polluted":true},"streaming":false,"params":{"nested":{"__proto__":{"polluted":true},"model":"openai/gpt-4o"}}}}}}}',
|
||||
},
|
||||
])("filters blocked keys recursively from the $side collision side", ({ raw }) => {
|
||||
const res = migrateLegacyConfigForTest(JSON.parse(raw));
|
||||
const merged = res.config?.agents?.defaults?.models?.["openai/gpt-5.5"] as Record<
|
||||
string,
|
||||
unknown
|
||||
>;
|
||||
const params = merged.params as Record<string, unknown>;
|
||||
const nested = params.nested as Record<string, unknown>;
|
||||
|
||||
for (const record of [merged, params, nested]) {
|
||||
expect(Object.getOwnPropertyNames(record)).not.toContain("__proto__");
|
||||
expect(Object.getPrototypeOf(record)).toBe(Object.prototype);
|
||||
expect(record.polluted).toBeUndefined();
|
||||
}
|
||||
expect(({} as Record<string, unknown>).polluted).toBeUndefined();
|
||||
expect(merged).toEqual({
|
||||
alias: "canonical-five",
|
||||
params: { nested: { model: "openai/gpt-5.5" } },
|
||||
streaming: false,
|
||||
});
|
||||
});
|
||||
|
||||
it("does not invoke prototype setters when copying the rewritten config root", () => {
|
||||
const raw = JSON.parse(
|
||||
'{"__proto__":{"polluted":true},"agents":{"defaults":{"model":"openai/gpt-4"}}}',
|
||||
) as Record<string, unknown>;
|
||||
const res = migrateLegacyConfigForTest(raw);
|
||||
const config = res.config as unknown as Record<string, unknown>;
|
||||
|
||||
expect(Object.getPrototypeOf(config)).toBe(Object.prototype);
|
||||
expect(Object.hasOwn(config, "__proto__")).toBe(true);
|
||||
expect(config.polluted).toBeUndefined();
|
||||
expect(res.config?.agents?.defaults?.model).toBe("openai/gpt-5.5");
|
||||
});
|
||||
|
||||
it("reports malformed scalar collisions without claiming equal values conflict", () => {
|
||||
const res = migrateLegacyConfigForTest({
|
||||
agents: {
|
||||
defaults: {
|
||||
models: {
|
||||
"openai/gpt-5.5": false,
|
||||
"openai/gpt-4": true,
|
||||
"openai/gpt-4o": false,
|
||||
},
|
||||
},
|
||||
},
|
||||
});
|
||||
|
||||
expect(res.config?.agents?.defaults?.models?.["openai/gpt-5.5"]).toBe(false);
|
||||
expect(res.changes.filter((change) => change.includes("Merged"))).toEqual([
|
||||
'Merged config.agents.defaults.models key "openai/gpt-4" into "openai/gpt-5.5"; kept existing values for conflicting fields: value.',
|
||||
'Merged config.agents.defaults.models key "openai/gpt-4o" into "openai/gpt-5.5".',
|
||||
]);
|
||||
});
|
||||
|
||||
it("removes unrecognized model compat thinkingFormat values", () => {
|
||||
const res = migrateLegacyConfigForTest({
|
||||
models: {
|
||||
|
||||
@@ -1,4 +1,5 @@
|
||||
// Legacy model runtime config migrations for stale model refs, compat fields, and catalog data.
|
||||
import { isDeepStrictEqual } from "node:util";
|
||||
import { normalizeProviderId } from "@openclaw/model-catalog-core/provider-id";
|
||||
import { splitTrailingAuthProfile } from "../../../agents/model-ref-profile.js";
|
||||
import {
|
||||
@@ -9,6 +10,7 @@ import {
|
||||
type LegacyConfigRule,
|
||||
} from "../../../config/legacy.shared.js";
|
||||
import { isModelThinkingFormat, type ModelDefinitionConfig } from "../../../config/types.models.js";
|
||||
import { isBlockedObjectKey } from "../../../infra/prototype-keys.js";
|
||||
import { isLegacyModelsAddCodexMetadataModel } from "./legacy-models-add-metadata.js";
|
||||
|
||||
const STALE_CONTEXT_WINDOW_FIXES: Record<string, { stale: number; correct: number }> = {
|
||||
@@ -847,6 +849,86 @@ function rewriteModelRefString(value: string, path: string, changes: string[]):
|
||||
return upgraded;
|
||||
}
|
||||
|
||||
function setRecordEntry(record: Record<string, unknown>, key: string, value: unknown): void {
|
||||
// Config dictionaries can contain hostile keys; define own properties so
|
||||
// rebuilding or copying them never invokes Object.prototype setters.
|
||||
Object.defineProperty(record, key, {
|
||||
configurable: true,
|
||||
enumerable: true,
|
||||
value,
|
||||
writable: true,
|
||||
});
|
||||
}
|
||||
|
||||
function sanitizeModelRefMapEntry(value: unknown): unknown {
|
||||
// Collisions combine both entries before recursive ref rewriting, so blocked
|
||||
// keys must be removed at every depth on both sides of the merge.
|
||||
if (Array.isArray(value)) {
|
||||
return value.map(sanitizeModelRefMapEntry);
|
||||
}
|
||||
const record = getRecord(value);
|
||||
if (!record) {
|
||||
return value;
|
||||
}
|
||||
const sanitized: Record<string, unknown> = {};
|
||||
for (const [field, child] of Object.entries(record)) {
|
||||
if (!isBlockedObjectKey(field)) {
|
||||
setRecordEntry(sanitized, field, sanitizeModelRefMapEntry(child));
|
||||
}
|
||||
}
|
||||
return sanitized;
|
||||
}
|
||||
|
||||
function modelRefValuesAreEqual(existing: unknown, incoming: unknown, path: string): boolean {
|
||||
if (isDeepStrictEqual(existing, incoming)) {
|
||||
return true;
|
||||
}
|
||||
const normalizedExisting = rewriteKnownModelRefs(existing, path, []).value;
|
||||
const normalizedIncoming = rewriteKnownModelRefs(incoming, path, []).value;
|
||||
return isDeepStrictEqual(normalizedExisting, normalizedIncoming);
|
||||
}
|
||||
|
||||
function mergeModelRefMapEntries(
|
||||
existing: unknown,
|
||||
incoming: unknown,
|
||||
path: string,
|
||||
): { value: unknown; conflicts: string[] } {
|
||||
const existingRecord = getRecord(existing);
|
||||
const incomingRecord = getRecord(incoming);
|
||||
if (!existingRecord || !incomingRecord) {
|
||||
return {
|
||||
value: sanitizeModelRefMapEntry(existing),
|
||||
conflicts: modelRefValuesAreEqual(existing, incoming, path) ? [] : ["value"],
|
||||
};
|
||||
}
|
||||
const merged = sanitizeModelRefMapEntry(existingRecord) as Record<string, unknown>;
|
||||
const conflicts: string[] = [];
|
||||
for (const [field, incomingValue] of Object.entries(incomingRecord)) {
|
||||
if (incomingValue === undefined || isBlockedObjectKey(field)) {
|
||||
continue;
|
||||
}
|
||||
if (!hasOwnDefinedProperty(existingRecord, field)) {
|
||||
setRecordEntry(merged, field, sanitizeModelRefMapEntry(incomingValue));
|
||||
continue;
|
||||
}
|
||||
const existingValue = existingRecord[field];
|
||||
const fieldPath = `${path}.${field}`;
|
||||
if (modelRefValuesAreEqual(existingValue, incomingValue, fieldPath)) {
|
||||
continue;
|
||||
}
|
||||
const existingField = getRecord(existingValue);
|
||||
const incomingField = getRecord(incomingValue);
|
||||
if (existingField && incomingField) {
|
||||
const nested = mergeModelRefMapEntries(existingField, incomingField, fieldPath);
|
||||
setRecordEntry(merged, field, nested.value);
|
||||
conflicts.push(...nested.conflicts.map((c) => `${field}.${c}`));
|
||||
continue;
|
||||
}
|
||||
conflicts.push(field);
|
||||
}
|
||||
return { value: merged, conflicts };
|
||||
}
|
||||
|
||||
function rewriteModelRefMapKeys(
|
||||
record: Record<string, unknown>,
|
||||
path: string,
|
||||
@@ -854,19 +936,40 @@ function rewriteModelRefMapKeys(
|
||||
): { value: Record<string, unknown>; changed: boolean } {
|
||||
let changed = false;
|
||||
const next: Record<string, unknown> = {};
|
||||
const consumedCanonicalKeys = new Set<string>();
|
||||
for (const [key, child] of Object.entries(record)) {
|
||||
const upgradedKey = upgradeRetiredModelRef(key);
|
||||
const nextKey = upgradedKey ?? key;
|
||||
if (!upgradedKey && consumedCanonicalKeys.has(key)) {
|
||||
continue;
|
||||
}
|
||||
if (upgradedKey) {
|
||||
changes.push(
|
||||
`Upgraded ${path} key from ${JSON.stringify(key)} to ${JSON.stringify(upgradedKey)}.`,
|
||||
);
|
||||
changed = true;
|
||||
}
|
||||
if (nextKey in next && upgradedKey) {
|
||||
if (upgradedKey && !Object.hasOwn(next, nextKey) && Object.hasOwn(record, nextKey)) {
|
||||
// Seed the canonical entry before its retired aliases so canonical conflict
|
||||
// precedence and per-alias change reporting do not depend on authored key order.
|
||||
setRecordEntry(next, nextKey, record[nextKey]);
|
||||
consumedCanonicalKeys.add(nextKey);
|
||||
}
|
||||
if (Object.hasOwn(next, nextKey)) {
|
||||
const existing = next[nextKey];
|
||||
const { value, conflicts } = mergeModelRefMapEntries(existing, child, `${path}.${nextKey}`);
|
||||
setRecordEntry(next, nextKey, value);
|
||||
const sortedConflicts = conflicts.toSorted();
|
||||
if (sortedConflicts.length > 0) {
|
||||
changes.push(
|
||||
`Merged ${path} key ${JSON.stringify(key)} into ${JSON.stringify(nextKey)}; kept existing values for conflicting fields: ${sortedConflicts.join(", ")}.`,
|
||||
);
|
||||
} else {
|
||||
changes.push(`Merged ${path} key ${JSON.stringify(key)} into ${JSON.stringify(nextKey)}.`);
|
||||
}
|
||||
continue;
|
||||
}
|
||||
next[nextKey] = child;
|
||||
setRecordEntry(next, nextKey, child);
|
||||
}
|
||||
return { value: changed ? next : record, changed };
|
||||
}
|
||||
@@ -915,7 +1018,7 @@ function rewriteKnownModelRefs(
|
||||
for (const [childKey, child] of Object.entries(working)) {
|
||||
const rewritten = rewriteKnownModelRefs(child, `${path}.${childKey}`, changes);
|
||||
changed ||= rewritten.changed;
|
||||
next[childKey] = rewritten.value;
|
||||
setRecordEntry(next, childKey, rewritten.value);
|
||||
}
|
||||
return { value: changed ? next : value, changed };
|
||||
}
|
||||
@@ -1333,13 +1436,16 @@ export const LEGACY_CONFIG_MIGRATIONS_RUNTIME_MODELS: LegacyConfigMigrationSpec[
|
||||
legacyRules: RETIRED_MODEL_REF_RULES,
|
||||
apply: (raw, changes) => {
|
||||
const rewritten = rewriteKnownModelRefs(raw, "config", changes);
|
||||
if (!rewritten.changed || !getRecord(rewritten.value)) {
|
||||
const rewrittenRecord = getRecord(rewritten.value);
|
||||
if (!rewritten.changed || !rewrittenRecord) {
|
||||
return;
|
||||
}
|
||||
for (const key of Object.keys(raw)) {
|
||||
delete raw[key];
|
||||
}
|
||||
Object.assign(raw, rewritten.value);
|
||||
for (const [key, value] of Object.entries(rewrittenRecord)) {
|
||||
setRecordEntry(raw, key, value);
|
||||
}
|
||||
},
|
||||
}),
|
||||
defineLegacyConfigMigration({
|
||||
|
||||
Reference in New Issue
Block a user