diff --git a/packages/coding-agent/src/sdk.ts b/packages/coding-agent/src/sdk.ts index 9568607aa..bab6ab2bd 100644 --- a/packages/coding-agent/src/sdk.ts +++ b/packages/coding-agent/src/sdk.ts @@ -3112,6 +3112,13 @@ export async function createAgentSession(options: CreateAgentSessionOptions = {} return id ? `${id}-advisor` : null; }, getAgentId: () => "advisor", + // The primary's availability signals are wrong for advisors: their tool + // slate is filtered separately at runtime (default read/grep/glob, no + // write transport), so xd:// devices are unreachable and read must never + // advertise inspect_image — images are inlined, and the provider + // boundary handles text-only advisor models. + xdevRegistry: undefined, + isToolActive: name => name !== "inspect_image" && toolSession.isToolActive?.(name) === true, }; const advisorToolBuilds: Array> = []; for (const name in BUILTIN_TOOLS) { diff --git a/packages/coding-agent/src/session/agent-session.ts b/packages/coding-agent/src/session/agent-session.ts index a250de9b3..385a10f9c 100644 --- a/packages/coding-agent/src/session/agent-session.ts +++ b/packages/coding-agent/src/session/agent-session.ts @@ -6493,7 +6493,7 @@ export class AgentSession { return true; } - #setModelWithProviderSessionReset(model: Model): void { + async #setModelWithProviderSessionReset(model: Model): Promise { const currentModel = this.model; if (currentModel) { this.#closeProviderSessionsForModelSwitch(currentModel, model); @@ -6508,11 +6508,13 @@ export class AgentSession { // inspect_image auto mode keys off model image capability. Reconcile // centrally here so retry-fallback model changes (turn-recovery.ts), - // which bypass syncAfterModelChange, cannot leave the tool set stale. - // Idempotent; syncAfterModelChange's own reconcile is then a no-op. - void this.#tools.reconcileInspectImageAfterModelChange().catch(error => { + // which bypass syncAfterModelChange, cannot leave the tool set stale — + // callers await, so a scheduled retry never races the reconciled slate. + try { + await this.#tools.reconcileInspectImageAfterModelChange(); + } catch (error) { logger.warn("inspect_image reconcile after model change failed", { error: String(error) }); - }); + } } #closeCodexProviderSessionsForHistoryRewrite(): void { @@ -7077,7 +7079,7 @@ export class AgentSession { currentModel.id !== match.id || currentModel.api !== match.api)); if (shouldResetProviderState) { - this.#setModelWithProviderSessionReset(match); + await this.#setModelWithProviderSessionReset(match); } else { this.agent.setModel(match); } diff --git a/packages/coding-agent/src/session/model-controls.ts b/packages/coding-agent/src/session/model-controls.ts index e0e47c4a3..364e76f67 100644 --- a/packages/coding-agent/src/session/model-controls.ts +++ b/packages/coding-agent/src/session/model-controls.ts @@ -52,7 +52,7 @@ export interface ModelControlsHost { promptGeneration(): number; resolveActiveEditMode(): EditMode; syncAfterModelChange(previousEditMode: EditMode): Promise; - setModelWithProviderSessionReset(model: Model): void; + setModelWithProviderSessionReset(model: Model): Promise; clearActiveRetryFallback(): void; clearInheritedProviderPromptCacheKey(): void; magicKeywordEnabled(keyword: "orchestrate" | "ultrathink" | "workflow"): boolean; @@ -220,7 +220,7 @@ export class ModelControls { this.#host.modelRegistry.clearSuppressedSelector(formatModelStringWithRouting(targetModel)); this.#host.clearActiveRetryFallback(); - this.#host.setModelWithProviderSessionReset(targetModel); + await this.#host.setModelWithProviderSessionReset(targetModel); this.#host.sessionManager.appendModelChange(`${targetModel.provider}/${targetModel.id}`, role); if (options?.persist) { this.#host.settings.setModelRole( @@ -265,7 +265,7 @@ export class ModelControls { this.#host.modelRegistry.clearSuppressedSelector(formatModelStringWithRouting(targetModel)); this.#host.clearActiveRetryFallback(); - this.#host.setModelWithProviderSessionReset(targetModel); + await this.#host.setModelWithProviderSessionReset(targetModel); this.#host.sessionManager.appendModelChange( `${targetModel.provider}/${targetModel.id}`, options?.ephemeral ? EPHEMERAL_MODEL_CHANGE_ROLE : "temporary", @@ -425,7 +425,7 @@ export class ModelControls { // Apply model this.#host.modelRegistry.clearSuppressedSelector(formatModelStringWithRouting(next.model)); this.#host.clearActiveRetryFallback(); - this.#host.setModelWithProviderSessionReset(next.model); + await this.#host.setModelWithProviderSessionReset(next.model); this.#host.sessionManager.appendModelChange(`${next.model.provider}/${next.model.id}`); this.#host.settings.getStorage()?.recordModelUsage(`${next.model.provider}/${next.model.id}`); @@ -456,7 +456,7 @@ export class ModelControls { this.#host.modelRegistry.clearSuppressedSelector(formatModelStringWithRouting(nextModel)); this.#host.clearActiveRetryFallback(); - this.#host.setModelWithProviderSessionReset(nextModel); + await this.#host.setModelWithProviderSessionReset(nextModel); this.#host.sessionManager.appendModelChange(`${nextModel.provider}/${nextModel.id}`); this.#host.settings.getStorage()?.recordModelUsage(`${nextModel.provider}/${nextModel.id}`); // Re-apply the current thinking level (or auto) for the newly selected model diff --git a/packages/coding-agent/src/session/turn-recovery.ts b/packages/coding-agent/src/session/turn-recovery.ts index 15efa4a94..a6a4d7705 100644 --- a/packages/coding-agent/src/session/turn-recovery.ts +++ b/packages/coding-agent/src/session/turn-recovery.ts @@ -118,7 +118,7 @@ export interface TurnRecoveryHost { waitForSessionMessagePersistence(message: AssistantMessage): Promise; appendSessionMessage(message: AssistantMessage): void; sessionMessageAlreadyPersisted(message: AssistantMessage): boolean; - setModelWithProviderSessionReset(model: Model): void; + setModelWithProviderSessionReset(model: Model): Promise; resetCurrentResponsesProviderSession(reason: string): void; maybeAutoRedeemCodexReset(): Promise; runAutoCompaction( @@ -1028,7 +1028,7 @@ export class TurnRecovery { ? requestedThinkingLevel : clampThinkingLevelToCeiling(candidate, requestedThinkingLevel, this.#host.thinkingLevelCeiling()); const candidateSelector = formatModelStringWithRouting(candidate); - this.#host.setModelWithProviderSessionReset(candidate); + await this.#host.setModelWithProviderSessionReset(candidate); this.#host.sessionManager.appendModelChange(candidateSelector, EPHEMERAL_MODEL_CHANGE_ROLE); this.#host.settings.getStorage()?.recordModelUsage(candidateSelector); this.#host.setThinkingLevel(nextThinkingLevel); @@ -1147,7 +1147,7 @@ export class TurnRecovery { const apiKey = await this.#host.modelRegistry.getApiKey(baseModel, this.#host.sessionId()); if (!apiKey) return false; const baseSelector = formatModelStringWithRouting(baseModel); - this.#host.setModelWithProviderSessionReset(baseModel); + await this.#host.setModelWithProviderSessionReset(baseModel); this.#host.sessionManager.appendModelChange(baseSelector, EPHEMERAL_MODEL_CHANGE_ROLE); this.#host.settings.getStorage()?.recordModelUsage(baseSelector); await this.#host.emitSessionEvent({ @@ -1201,7 +1201,7 @@ export class TurnRecovery { const thinkingToApply = currentThinkingLevel === lastAppliedFallbackThinkingLevel ? originalThinkingLevel : currentThinkingLevel; const primarySelector = formatModelStringWithRouting(primaryModel); - this.#host.setModelWithProviderSessionReset(primaryModel); + await this.#host.setModelWithProviderSessionReset(primaryModel); this.#host.sessionManager.appendModelChange(primarySelector, EPHEMERAL_MODEL_CHANGE_ROLE); this.#host.settings.getStorage()?.recordModelUsage(primarySelector); this.#host.setThinkingLevel(thinkingToApply); diff --git a/packages/coding-agent/src/tools/read.ts b/packages/coding-agent/src/tools/read.ts index 4cf544f79..338ecf810 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 = session.isToolActive?.("inspect_image") ?? isInspectImageToolActive(session); + this.#inspectImageActive = this.#resolveInspectImageAvailability(); this.description = this.#renderDescription(); } @@ -894,21 +894,33 @@ export class ReadTool implements AgentTool { }); } + /** + * Whether the agent can actually reach `inspect_image` right now: exposed + * top-level, or mounted as an `xd://` device while the effective mode wants + * it (mounted devices stay executable via `write xd://inspect_image`, so a + * metadata-only read remains actionable). Sessions with neither + * availability signal (tests, embedded use) fall back to the mode + * computation alone. Restricted slates (subagents without the tool and + * without xdev) resolve to unavailable, so those sessions get inline image + * blocks instead of guidance pointing at an absent tool. + */ + #resolveInspectImageAvailability(): boolean { + const topLevel = this.session.isToolActive?.("inspect_image"); + const registry = this.session.xdevRegistry; + if (topLevel === undefined && registry === undefined) return isInspectImageToolActive(this.session); + if (topLevel === true) return true; + return registry?.get("inspect_image") !== undefined && isInspectImageToolActive(this.session); + } + /** * Re-evaluate the effective inspect_image state; it can flip when the model * 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. + * per image read and by tool reconciliation before prompt rebuilds (which + * passes the post-change availability as `availableOverride`). */ syncInspectImageState(availableOverride?: boolean): boolean { - const active = - availableOverride ?? this.session.isToolActive?.("inspect_image") ?? isInspectImageToolActive(this.session); + const active = availableOverride ?? this.#resolveInspectImageAvailability(); 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 dbc2ae672..22c177815 100644 --- a/packages/coding-agent/test/inspect-image-mode.test.ts +++ b/packages/coding-agent/test/inspect-image-mode.test.ts @@ -105,28 +105,63 @@ describe("inspect_image.enabled migration", () => { }); describe("read tool follows actual tool availability", () => { - function readSession(inspectImageActive: boolean): ToolSession { + function readSession(options: { + inspectImageActive: boolean; + xdevMounted?: boolean; + settings?: Record; + }): ToolSession { return { cwd: os.tmpdir(), hasUI: false, - settings: Settings.isolated(), + settings: Settings.isolated(options.settings ?? {}), getSessionFile: () => null, getSessionSpawns: () => null, getActiveModel: () => textModel, - isToolActive: (name: string) => name === "inspect_image" && inspectImageActive, + isToolActive: (name: string) => name === "inspect_image" && options.inspectImageActive, + ...(options.xdevMounted === undefined + ? {} + : { + xdevRegistry: { + get: (name: string) => + name === "inspect_image" && options.xdevMounted ? { name: "inspect_image" } : undefined, + }, + }), } 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)); + const tool = new ReadTool(readSession({ inspectImageActive: 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)); + const tool = new ReadTool(readSession({ inspectImageActive: true })); expect(tool.syncInspectImageState()).toBe(true); }); + + test("xd://-mounted inspect_image counts as available", () => { + // Default sessions mount discoverable built-ins under xd://, removing them + // from the top-level predicate while they stay executable via + // `write xd://inspect_image` — read must keep pointing at the tool. + const tool = new ReadTool(readSession({ inspectImageActive: false, xdevMounted: true })); + expect(tool.syncInspectImageState()).toBe(true); + expect(tool.description).toContain("call `inspect_image`"); + }); + + test("mode off wins over a lingering xd:// mount", () => { + // Built-in devices are never reconciled out of the xdev registry, so after + // `/vision off` the device still resolves — the effective mode must gate it. + const tool = new ReadTool( + readSession({ + inspectImageActive: false, + xdevMounted: true, + settings: { "inspect_image.mode": "off" }, + }), + ); + expect(tool.syncInspectImageState()).toBe(false); + expect(tool.description).not.toContain("call `inspect_image`"); + }); });