diff --git a/packages/coding-agent/src/config/settings.ts b/packages/coding-agent/src/config/settings.ts index 472e03200..d8e391096 100644 --- a/packages/coding-agent/src/config/settings.ts +++ b/packages/coding-agent/src/config/settings.ts @@ -39,6 +39,7 @@ import { isLightTheme, setAutoThemeMapping, setColorBlindMode, setSymbolPreset } import { AgentStorage } from "../session/agent-storage"; import { AUTO_IMAGE_PROVIDER_ORDER, isImageProviderId } from "../tools/image-providers"; import { type EditMode, normalizeEditMode } from "../utils/edit-mode"; +import { INSPECT_IMAGE_MODES } from "../utils/inspect-image-mode"; import { isSearchProviderId, SEARCH_PROVIDER_ORDER } from "../web/search/types"; import { withFileLock } from "./file-lock"; import { @@ -1384,11 +1385,20 @@ export class Settings { raw.inspect_image = {}; } const target = raw.inspect_image as Record; - if (target.mode === undefined && raw["inspect_image.mode"] === undefined) { - target.mode = legacyEnabled ? "on" : "off"; + const flatMode = raw["inspect_image.mode"]; + if (target.mode === undefined) { + // A quoted-dotted explicit mode wins over the legacy boolean but + // must be normalized into the nested form the resolver reads. + target.mode = + typeof flatMode === "string" && (INSPECT_IMAGE_MODES as readonly string[]).includes(flatMode) + ? flatMode + : legacyEnabled + ? "on" + : "off"; } delete target.enabled; delete raw["inspect_image.enabled"]; + delete raw["inspect_image.mode"]; } // task.isolation.enabled (boolean) -> task.isolation.mode (enum) diff --git a/packages/coding-agent/src/session/session-tools.ts b/packages/coding-agent/src/session/session-tools.ts index 8118031e4..7129a629c 100644 --- a/packages/coding-agent/src/session/session-tools.ts +++ b/packages/coding-agent/src/session/session-tools.ts @@ -887,15 +887,23 @@ export class SessionTools { getActiveModel: () => this.#host.model(), getInspectImageModeOverride: () => this.#host.getInspectImageModeOverride(), }); - // Keep the read tool's advertised description in sync with the effective - // state BEFORE any prompt rebuild below; its per-read behavior syncs - // lazily, which would otherwise leave stale guidance in the system prompt. - const readTool = this.#toolRegistry.get("read") as { syncInspectImageState?: () => boolean } | undefined; - readTool?.syncInspectImageState?.(); + // Keep the read tool's advertised description in sync BEFORE any prompt + // rebuild below, passing the post-change availability so the prompt never + // lags a flip in either direction. Per-read lazy sync is the backstop. + const syncReadDescription = (available: boolean): void => { + const readTool = this.#toolRegistry.get("read") as + | { syncInspectImageState?: (available?: boolean) => boolean } + | undefined; + readTool?.syncInspectImageState?.(available); + }; const active = this.getEnabledToolNames(); const isActive = active.includes("inspect_image"); - if (expected === isActive) return true; + if (expected === isActive) { + syncReadDescription(isActive); + return true; + } if (!expected) { + syncReadDescription(false); await this.applyActiveToolsByName(active.filter(name => name !== "inspect_image")); return true; } @@ -905,12 +913,14 @@ export class SessionTools { logger.warn("inspect_image tool could not be created", { model: this.#host.model()?.id, }); + syncReadDescription(false); return false; } const wrapped = this.#wrapRuntimeTool(tool); this.#toolRegistry.set(wrapped.name, wrapped); this.#builtInToolNames.add(wrapped.name); } + syncReadDescription(true); await this.applyActiveToolsByName([...active, "inspect_image"]); return true; } diff --git a/packages/coding-agent/src/tools/read.ts b/packages/coding-agent/src/tools/read.ts index 1ae86e50b..4cf544f79 100644 --- a/packages/coding-agent/src/tools/read.ts +++ b/packages/coding-agent/src/tools/read.ts @@ -874,7 +874,7 @@ export class ReadTool implements AgentTool { 1, Math.min(session.settings.get("read.defaultLimit") ?? DEFAULT_MAX_LINES, DEFAULT_MAX_LINES), ); - this.#inspectImageActive = isInspectImageToolActive(session); + this.#inspectImageActive = session.isToolActive?.("inspect_image") ?? isInspectImageToolActive(session); this.description = this.#renderDescription(); } @@ -899,9 +899,16 @@ export class ReadTool implements AgentTool { * or the `/vision` override changes after this tool was constructed. Keeps * the behavior branch and the advertised description in lockstep. Called * per image read and by tool reconciliation before prompt rebuilds. + * + * Actual tool availability wins over the mode computation: restricted + * sessions (explicit tool slates without `inspect_image`, e.g. subagents) + * must never see metadata-only reads pointing at an absent tool. Sessions + * without an `isToolActive` predicate (tests, embedded use) fall back to + * the mode check. */ - syncInspectImageState(): boolean { - const active = isInspectImageToolActive(this.session); + syncInspectImageState(availableOverride?: boolean): boolean { + const active = + availableOverride ?? this.session.isToolActive?.("inspect_image") ?? isInspectImageToolActive(this.session); if (active !== this.#inspectImageActive) { this.#inspectImageActive = active; this.description = this.#renderDescription(); diff --git a/packages/coding-agent/test/inspect-image-mode.test.ts b/packages/coding-agent/test/inspect-image-mode.test.ts index ae0053fe4..dbc2ae672 100644 --- a/packages/coding-agent/test/inspect-image-mode.test.ts +++ b/packages/coding-agent/test/inspect-image-mode.test.ts @@ -9,6 +9,8 @@ import * as os from "node:os"; import * as path from "node:path"; import type { Model } from "@oh-my-pi/pi-ai"; import { Settings } from "../src/config/settings"; +import type { ToolSession } from "../src/tools/index"; +import { ReadTool } from "../src/tools/read"; import { isInspectImageToolActive } from "../src/utils/inspect-image-mode"; const visionModel = { provider: "kimi-code", id: "k3", input: ["text", "image"] } as unknown as Model; @@ -89,5 +91,42 @@ describe("inspect_image.enabled migration", () => { const settings = await Settings.loadReadOnly({ agentDir, cwd: agentDir }); expect(settings.get("inspect_image.mode")).toBe("off"); }); + + test("flat explicit mode survives alongside a flat legacy key", async () => { + agentDir = fs.mkdtempSync(path.join(os.tmpdir(), "omp-vision-migration-")); + fs.writeFileSync( + path.join(agentDir, "config.yml"), + '"inspect_image.enabled": true\n"inspect_image.mode": "off"\n', + ); + const settings = await Settings.loadReadOnly({ agentDir, cwd: agentDir }); + expect(settings.get("inspect_image.mode")).toBe("off"); + }); + }); +}); + +describe("read tool follows actual tool availability", () => { + function readSession(inspectImageActive: boolean): ToolSession { + return { + cwd: os.tmpdir(), + hasUI: false, + settings: Settings.isolated(), + getSessionFile: () => null, + getSessionSpawns: () => null, + getActiveModel: () => textModel, + isToolActive: (name: string) => name === "inspect_image" && inspectImageActive, + } as unknown as ToolSession; + } + + test("restricted session (tool absent) never advertises inspect_image", () => { + // auto mode + text-only model would compute active=true, but the tool is + // not in this session's slate, so read must serve inline image blocks. + const tool = new ReadTool(readSession(false)); + expect(tool.description).not.toContain("call `inspect_image`"); + expect(tool.syncInspectImageState()).toBe(false); + }); + + test("session with the tool registered advertises it", () => { + const tool = new ReadTool(readSession(true)); + expect(tool.syncInspectImageState()).toBe(true); }); });