From f07a58965e1213f3a004f37e536b3efc1f873756 Mon Sep 17 00:00:00 2001 From: Robby Date: Thu, 22 Jan 2026 20:31:06 +0000 Subject: [PATCH 1/2] fix: only show model switch success when persist succeeds (fixes #1435) Previously, the /model command would display 'Model set to X' even when the session state wasn't actually persisted (when sessionEntry, sessionStore, or sessionKey were missing). This caused confusion as users saw success messages but the model didn't actually change. This fix: - Tracks whether the model override was actually persisted - Only shows success message when persist happened - Shows a clear error message when persist fails AI-assisted: Claude Opus 4.5 via Clawdbot Testing: lightly tested (code review, no runtime test) --- src/auto-reply/reply/directive-handling.impl.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/auto-reply/reply/directive-handling.impl.ts b/src/auto-reply/reply/directive-handling.impl.ts index 7f056e6cd..63b1baa8c 100644 --- a/src/auto-reply/reply/directive-handling.impl.ts +++ b/src/auto-reply/reply/directive-handling.impl.ts @@ -288,6 +288,7 @@ export async function handleDirectiveOnly(params: { nextThinkLevel === "xhigh" && !supportsXHighThinking(resolvedProvider, resolvedModel); + let didPersistModel = false; if (sessionEntry && sessionStore && sessionKey) { const prevElevatedLevel = currentElevatedLevel ?? @@ -346,6 +347,7 @@ export async function handleDirectiveOnly(params: { selection: modelSelection, profileOverride, }); + didPersistModel = true; } if (directives.hasQueueDirective && directives.queueReset) { delete sessionEntry.queueMode; @@ -447,7 +449,7 @@ export async function handleDirectiveOnly(params: { `Thinking level set to high (xhigh not supported for ${resolvedProvider}/${resolvedModel}).`, ); } - if (modelSelection) { + if (modelSelection && didPersistModel) { const label = `${modelSelection.provider}/${modelSelection.model}`; const labelWithAlias = modelSelection.alias ? `${modelSelection.alias} (${label})` : label; parts.push( @@ -458,6 +460,8 @@ export async function handleDirectiveOnly(params: { if (profileOverride) { parts.push(`Auth profile set to ${profileOverride}.`); } + } else if (modelSelection && !didPersistModel) { + parts.push(`Model switch to ${modelSelection.provider}/${modelSelection.model} failed (session state unavailable).`); } if (directives.hasQueueDirective && directives.queueMode) { parts.push(formatDirectiveAck(`Queue mode set to ${directives.queueMode}.`)); From 784ea4f7d504209d44539e9d40330c845c9120df Mon Sep 17 00:00:00 2001 From: Robby Date: Thu, 22 Jan 2026 20:33:44 +0000 Subject: [PATCH 2/2] test: add unit tests for model switch persist behavior Tests verify: - Success message shown when session state available - Error message shown when sessionEntry missing - Error message shown when sessionStore missing - No model message when no /model directive Covers edge cases for #1435 fix. --- ...ective-handling.impl.model-persist.test.ts | 174 ++++++++++++++++++ .../reply/directive-handling.impl.ts | 4 +- 2 files changed, 177 insertions(+), 1 deletion(-) create mode 100644 src/auto-reply/reply/directive-handling.impl.model-persist.test.ts diff --git a/src/auto-reply/reply/directive-handling.impl.model-persist.test.ts b/src/auto-reply/reply/directive-handling.impl.model-persist.test.ts new file mode 100644 index 000000000..8ac0f12fb --- /dev/null +++ b/src/auto-reply/reply/directive-handling.impl.model-persist.test.ts @@ -0,0 +1,174 @@ +import { describe, expect, it, vi } from "vitest"; + +import type { ModelAliasIndex } from "../../agents/model-selection.js"; +import type { ClawdbotConfig } from "../../config/config.js"; +import type { SessionEntry } from "../../config/sessions.js"; +import { parseInlineDirectives } from "./directive-handling.js"; +import { handleDirectiveOnly } from "./directive-handling.impl.js"; + +// Mock dependencies +vi.mock("../../agents/agent-scope.js", () => ({ + resolveAgentConfig: vi.fn(() => ({})), + resolveAgentDir: vi.fn(() => "/tmp/agent"), + resolveSessionAgentId: vi.fn(() => "main"), +})); + +vi.mock("../../agents/sandbox.js", () => ({ + resolveSandboxRuntimeStatus: vi.fn(() => ({ sandboxed: false })), +})); + +vi.mock("../../config/sessions.js", () => ({ + updateSessionStore: vi.fn(async () => {}), +})); + +vi.mock("../../infra/system-events.js", () => ({ + enqueueSystemEvent: vi.fn(), +})); + +function baseAliasIndex(): ModelAliasIndex { + return { byAlias: new Map(), byKey: new Map() }; +} + +function baseConfig(): ClawdbotConfig { + return { + commands: { text: true }, + agents: { defaults: {} }, + } as unknown as ClawdbotConfig; +} + +describe("handleDirectiveOnly model persist behavior (fixes #1435)", () => { + const allowedModelKeys = new Set(["anthropic/claude-opus-4-5", "openai/gpt-4o"]); + const allowedModelCatalog = [ + { provider: "anthropic", id: "claude-opus-4-5" }, + { provider: "openai", id: "gpt-4o" }, + ]; + + it("shows success message when session state is available", async () => { + const directives = parseInlineDirectives("/model openai/gpt-4o"); + const sessionEntry: SessionEntry = { + sessionId: "s1", + updatedAt: Date.now(), + }; + const sessionStore = { "agent:main:dm:1": sessionEntry }; + + const result = await handleDirectiveOnly({ + cfg: baseConfig(), + directives, + sessionEntry, + sessionStore, + sessionKey: "agent:main:dm:1", + storePath: "/tmp/sessions.json", + elevatedEnabled: false, + elevatedAllowed: false, + defaultProvider: "anthropic", + defaultModel: "claude-opus-4-5", + aliasIndex: baseAliasIndex(), + allowedModelKeys, + allowedModelCatalog, + resetModelOverride: false, + provider: "anthropic", + model: "claude-opus-4-5", + initialModelLabel: "anthropic/claude-opus-4-5", + formatModelSwitchEvent: (label) => `Switched to ${label}`, + }); + + expect(result?.text).toContain("Model set to"); + expect(result?.text).toContain("openai/gpt-4o"); + expect(result?.text).not.toContain("failed"); + }); + + it("shows error message when sessionEntry is missing", async () => { + const directives = parseInlineDirectives("/model openai/gpt-4o"); + const sessionStore = {}; + + const result = await handleDirectiveOnly({ + cfg: baseConfig(), + directives, + sessionEntry: undefined, // Missing! + sessionStore, + sessionKey: "agent:main:dm:1", + storePath: "/tmp/sessions.json", + elevatedEnabled: false, + elevatedAllowed: false, + defaultProvider: "anthropic", + defaultModel: "claude-opus-4-5", + aliasIndex: baseAliasIndex(), + allowedModelKeys, + allowedModelCatalog, + resetModelOverride: false, + provider: "anthropic", + model: "claude-opus-4-5", + initialModelLabel: "anthropic/claude-opus-4-5", + formatModelSwitchEvent: (label) => `Switched to ${label}`, + }); + + expect(result?.text).toContain("failed"); + expect(result?.text).toContain("session state unavailable"); + }); + + it("shows error message when sessionStore is missing", async () => { + const directives = parseInlineDirectives("/model openai/gpt-4o"); + const sessionEntry: SessionEntry = { + sessionId: "s1", + updatedAt: Date.now(), + }; + + const result = await handleDirectiveOnly({ + cfg: baseConfig(), + directives, + sessionEntry, + sessionStore: undefined, // Missing! + sessionKey: "agent:main:dm:1", + storePath: "/tmp/sessions.json", + elevatedEnabled: false, + elevatedAllowed: false, + defaultProvider: "anthropic", + defaultModel: "claude-opus-4-5", + aliasIndex: baseAliasIndex(), + allowedModelKeys, + allowedModelCatalog, + resetModelOverride: false, + provider: "anthropic", + model: "claude-opus-4-5", + initialModelLabel: "anthropic/claude-opus-4-5", + formatModelSwitchEvent: (label) => `Switched to ${label}`, + }); + + expect(result?.text).toContain("failed"); + expect(result?.text).toContain("session state unavailable"); + }); + + it("shows no model message when no /model directive", async () => { + const directives = parseInlineDirectives("hello world"); + const sessionEntry: SessionEntry = { + sessionId: "s1", + updatedAt: Date.now(), + }; + const sessionStore = { "agent:main:dm:1": sessionEntry }; + + const result = await handleDirectiveOnly({ + cfg: baseConfig(), + directives, + sessionEntry, + sessionStore, + sessionKey: "agent:main:dm:1", + storePath: "/tmp/sessions.json", + elevatedEnabled: false, + elevatedAllowed: false, + defaultProvider: "anthropic", + defaultModel: "claude-opus-4-5", + aliasIndex: baseAliasIndex(), + allowedModelKeys, + allowedModelCatalog, + resetModelOverride: false, + provider: "anthropic", + model: "claude-opus-4-5", + initialModelLabel: "anthropic/claude-opus-4-5", + formatModelSwitchEvent: (label) => `Switched to ${label}`, + }); + + // No model directive = no model message + expect(result?.text ?? "").not.toContain("Model set to"); + expect(result?.text ?? "").not.toContain("failed"); + }); +}); diff --git a/src/auto-reply/reply/directive-handling.impl.ts b/src/auto-reply/reply/directive-handling.impl.ts index 63b1baa8c..a753085dc 100644 --- a/src/auto-reply/reply/directive-handling.impl.ts +++ b/src/auto-reply/reply/directive-handling.impl.ts @@ -461,7 +461,9 @@ export async function handleDirectiveOnly(params: { parts.push(`Auth profile set to ${profileOverride}.`); } } else if (modelSelection && !didPersistModel) { - parts.push(`Model switch to ${modelSelection.provider}/${modelSelection.model} failed (session state unavailable).`); + parts.push( + `Model switch to ${modelSelection.provider}/${modelSelection.model} failed (session state unavailable).`, + ); } if (directives.hasQueueDirective && directives.queueMode) { parts.push(formatDirectiveAck(`Queue mode set to ${directives.queueMode}.`));