mirror of
https://github.com/openclaw/openclaw.git
synced 2026-05-06 05:20:43 +00:00
fix(active-memory): clarify modelFallbackPolicy deprecation warning text
Closes #74587. AI-assisted, fully tested. The previous deprecation warning ("set config.modelFallback explicitly if you want a fallback model") read naturally as runtime failover — model A errors → switch to model B. The actual semantics in `getModelRef` are different: `modelFallback` is the **last candidate in the chain-resolution walk**, consulted only when `config.model`, the current run's model, AND the agent's configured default have all resolved to nothing. There is no error-recovery / retry-with-different-model path. The mismatch wastes real debug time. The issue filer reports ~1 hour of cycles before reading source revealed the gap; users without source access can debug for much longer assuming runtime failover exists. ## Fix Rewrite the warning string to: 1. State the deprecation (preserved). 2. Describe `modelFallback`'s actual semantics — chain-resolution last-resort, gated on the three earlier candidates resolving to nothing. 3. Explicitly disclaim the wrong mental model — "it is NOT a runtime failover that substitutes a different model when the resolved model errors out" — so a quick read can't lead the operator astray. No behavior change, only operator-facing copy. Surrounding code paths (`getModelRef`, `hasDeprecatedModelFallbackPolicy`, the warn caller in `register()`) are untouched. ## Tests `extensions/active-memory/index.test.ts` extends the existing deprecation-warning assertion to pin both the positive copy (`chain-resolution`, `last-resort`) and the negative disclaimer (`NOT a runtime failover`), so a future "let's reword this" change that reintroduces the failover-implying language fails the test instead of silently regressing. `pnpm test extensions/active-memory/index.test.ts` — 94 passed. `pnpm exec oxfmt --check` — clean. `pnpm exec oxlint` — 0 warnings, 0 errors. ## AI-assisted PR - [x] Mark as AI-assisted (Claude). Lightly tested via the targeted Vitest extension shard; not exercised against a live Ollama / AM rollout because the change is a log-string update, not behavior. - [x] Confirm I understand what the code does: yes — `getModelRef` walks four candidates (`config.model`, `currentRunModel`, `configuredDefaultModel`, `config.modelFallback`) and returns the first non-null parse; `modelFallback` is purely a default-when-empty selector, not a runtime failover.
This commit is contained in:
committed by
Peter Steinberger
parent
395ad91323
commit
c894dbf0ae
@@ -1384,6 +1384,22 @@ describe("active-memory plugin", () => {
|
||||
expect(api.logger.warn).toHaveBeenCalledWith(
|
||||
expect.stringContaining("config.modelFallbackPolicy is deprecated"),
|
||||
);
|
||||
// #74587: deprecation warning must spell out the chain-resolution
|
||||
// semantics so operators don't read it as a promise of runtime failover.
|
||||
// The previous wording ("set config.modelFallback if you want a fallback
|
||||
// model") cost real users hours of debug time before they hit the source
|
||||
// and saw `getModelRef` only walks candidates once.
|
||||
const warnCalls = (api.logger.warn as ReturnType<typeof vi.fn>).mock.calls;
|
||||
const deprecationMessage = warnCalls
|
||||
.map(([first]) => (typeof first === "string" ? first : ""))
|
||||
.find((message) => message.includes("config.modelFallbackPolicy is deprecated"));
|
||||
expect(deprecationMessage).toBeDefined();
|
||||
// Positive: the warning describes chain-resolution last-resort behavior.
|
||||
expect(deprecationMessage).toContain("chain-resolution");
|
||||
expect(deprecationMessage).toContain("last-resort");
|
||||
// Negative: the warning explicitly disclaims runtime failover, since
|
||||
// that's the wrong mental model the previous wording invited.
|
||||
expect(deprecationMessage).toMatch(/NOT a runtime failover/i);
|
||||
});
|
||||
|
||||
it("does not use a built-in fallback model even when default-remote is configured", async () => {
|
||||
|
||||
@@ -2431,8 +2431,19 @@ export default definePluginEntry({
|
||||
let config = normalizePluginConfig(api.pluginConfig);
|
||||
const warnDeprecatedModelFallbackPolicy = (pluginConfig: unknown) => {
|
||||
if (hasDeprecatedModelFallbackPolicy(pluginConfig)) {
|
||||
// Wording matters here: the previous text ("set config.modelFallback
|
||||
// explicitly if you want a fallback model") read naturally as runtime
|
||||
// failover (model A errors → switch to model B), but `getModelRef`
|
||||
// only consults `modelFallback` as the *last candidate* in the
|
||||
// resolution chain after `config.model`, the current run's model,
|
||||
// and the agent's configured default have all resolved to nothing.
|
||||
// Surface the chain-resolution semantics directly so operators
|
||||
// don't waste debug cycles assuming runtime failover (#74587).
|
||||
api.logger.warn?.(
|
||||
"active-memory: config.modelFallbackPolicy is deprecated and no longer changes runtime behavior; set config.modelFallback explicitly if you want a fallback model",
|
||||
"active-memory: config.modelFallbackPolicy is deprecated and no longer changes runtime behavior. " +
|
||||
"config.modelFallback is a chain-resolution last-resort (consulted only when config.model, " +
|
||||
"the current run's model, and the agent's configured default all resolve to nothing) — " +
|
||||
"it is NOT a runtime failover that substitutes a different model when the resolved model errors out.",
|
||||
);
|
||||
}
|
||||
};
|
||||
|
||||
Reference in New Issue
Block a user