From 897233871e8a94fe02f56ddae75feca9610e705b Mon Sep 17 00:00:00 2001 From: Brian Thorne Date: Mon, 26 Jan 2026 21:15:49 +1300 Subject: [PATCH 01/13] Add ACP (Agent Client Protocol) executor Adds AcpExecutor for long-lived connections to ACP-compatible agents: - OpenCode: `opencode acp` - Claude: `npx @zed-industries/claude-code-acp` - Gemini: `gemini --experimental-acp` Features: - Persistent connection instead of spawning per-request - Session management with auto-reconnect - Progress event streaming (text, thinking, tool_use) - Permission request handling - Cancel support Note: Not yet wired into modals - needs settings toggle and modal integration. Co-Authored-By: Claude Opus 4.5 --- package-lock.json | 22 +++ package.json | 3 + src/executor/AcpExecutor.ts | 354 ++++++++++++++++++++++++++++++++++++ 3 files changed, 379 insertions(+) create mode 100644 src/executor/AcpExecutor.ts diff --git a/package-lock.json b/package-lock.json index f1b4664..228e342 100644 --- a/package-lock.json +++ b/package-lock.json @@ -8,6 +8,9 @@ "name": "obsidian-llm", "version": "1.0.0", "license": "MIT", + "dependencies": { + "@agentclientprotocol/sdk": "^0.13.1" + }, "devDependencies": { "@types/mocha": "^10.0.10", "@types/node": "^20.19.30", @@ -26,6 +29,15 @@ "wdio-obsidian-service": "^2.2.1" } }, + "node_modules/@agentclientprotocol/sdk": { + "version": "0.13.1", + "resolved": "https://registry.npmjs.org/@agentclientprotocol/sdk/-/sdk-0.13.1.tgz", + "integrity": "sha512-6byvu+F/xc96GBkdAx4hq6/tB3vT63DSBO4i3gYCz8nuyZMerVFna2Gkhm8EHNpZX0J9DjUxzZCW+rnHXUg0FA==", + "license": "Apache-2.0", + "peerDependencies": { + "zod": "^3.25.0 || ^4.0.0" + } + }, "node_modules/@babel/code-frame": { "version": "7.28.6", "resolved": "https://registry.npmjs.org/@babel/code-frame/-/code-frame-7.28.6.tgz", @@ -8173,6 +8185,16 @@ "engines": { "node": ">= 14" } + }, + "node_modules/zod": { + "version": "4.3.6", + "resolved": "https://registry.npmjs.org/zod/-/zod-4.3.6.tgz", + "integrity": "sha512-rftlrkhHZOcjDwkGlnUtZZkvaPHCsDATp4pGpuOOMDaTdDDXF91wuVDJoWoPsKX/3YPQ5fHuF3STjcYyKr+Qhg==", + "license": "MIT", + "peer": true, + "funding": { + "url": "https://github.com/sponsors/colinhacks" + } } } } diff --git a/package.json b/package.json index fc8de17..f3d943a 100644 --- a/package.json +++ b/package.json @@ -42,5 +42,8 @@ "typescript": "^5.0.0", "wdio-obsidian-reporter": "^2.2.1", "wdio-obsidian-service": "^2.2.1" + }, + "dependencies": { + "@agentclientprotocol/sdk": "^0.13.1" } } diff --git a/src/executor/AcpExecutor.ts b/src/executor/AcpExecutor.ts new file mode 100644 index 0000000..12b34eb --- /dev/null +++ b/src/executor/AcpExecutor.ts @@ -0,0 +1,354 @@ +/** + * ACP (Agent Client Protocol) Executor + * + * Provides a long-lived connection to an ACP-compatible agent (OpenCode, Claude, Gemini) + * instead of spawning a new process for each request. + */ + +import { spawn, ChildProcess } from "child_process"; +import { + ClientSideConnection, + ndJsonStream, + type Client, + type Agent, + type SessionNotification, + type SessionUpdate, + type RequestPermissionRequest, + type RequestPermissionResponse, + type ContentChunk, + type ToolCall, +} from "@agentclientprotocol/sdk"; +import type { LLMPluginSettings, LLMProvider, ProgressEvent } from "../types"; + +// Convert Node streams to Web streams +function nodeToWebReadable(nodeStream: NodeJS.ReadableStream): ReadableStream { + return new ReadableStream({ + start(controller) { + nodeStream.on("data", (chunk: Buffer) => { + controller.enqueue(new Uint8Array(chunk)); + }); + nodeStream.on("end", () => { + controller.close(); + }); + nodeStream.on("error", (err) => { + controller.error(err); + }); + }, + cancel() { + // Node streams don't have a standard destroy on the interface + if ("destroy" in nodeStream && typeof nodeStream.destroy === "function") { + nodeStream.destroy(); + } + }, + }); +} + +function nodeToWebWritable(nodeStream: NodeJS.WritableStream): WritableStream { + return new WritableStream({ + write(chunk) { + return new Promise((resolve, reject) => { + const ok = nodeStream.write(chunk, (err) => { + if (err) reject(err); + else resolve(); + }); + if (!ok) { + nodeStream.once("drain", resolve); + } + }); + }, + close() { + return new Promise((resolve) => { + nodeStream.end(resolve); + }); + }, + abort(err) { + if ("destroy" in nodeStream && typeof nodeStream.destroy === "function") { + (nodeStream as NodeJS.WritableStream & { destroy: (err?: Error) => void }).destroy(err); + } + }, + }); +} + +export interface AcpExecutorOptions { + onProgress?: (event: ProgressEvent) => void; + onPermissionRequest?: (request: RequestPermissionRequest) => Promise; +} + +export class AcpExecutor { + private settings: LLMPluginSettings; + private connection: ClientSideConnection | null = null; + private process: ChildProcess | null = null; + private sessionId: string | null = null; + private currentProvider: LLMProvider | null = null; + private debug: (...args: unknown[]) => void; + private progressCallback: ((event: ProgressEvent) => void) | null = null; + + constructor(settings: LLMPluginSettings) { + this.settings = settings; + this.debug = (...args: unknown[]) => { + if (settings.debugMode) { + console.log("[AcpExecutor]", ...args); + } + }; + } + + updateSettings(settings: LLMPluginSettings) { + this.settings = settings; + } + + /** + * Get the ACP command for a provider + */ + private getAcpCommand(provider: LLMProvider): { cmd: string; args: string[] } | null { + switch (provider) { + case "opencode": + return { cmd: "opencode", args: ["acp"] }; + case "claude": + // Claude uses the ACP adapter package + return { cmd: "npx", args: ["@zed-industries/claude-code-acp"] }; + case "gemini": + return { cmd: "gemini", args: ["--experimental-acp"] }; + case "codex": + // Codex doesn't have native ACP support yet + return null; + default: + return null; + } + } + + /** + * Connect to an ACP agent + */ + async connect( + provider: LLMProvider, + workingDirectory?: string, + options?: AcpExecutorOptions + ): Promise { + // If already connected to same provider, reuse + if (this.connection && this.currentProvider === provider) { + this.debug("Reusing existing connection for", provider); + return; + } + + // Disconnect any existing connection + await this.disconnect(); + + const acpCommand = this.getAcpCommand(provider); + if (!acpCommand) { + throw new Error(`Provider ${provider} does not support ACP`); + } + + const cwd = workingDirectory ?? process.cwd(); + this.debug("Spawning ACP agent:", acpCommand.cmd, acpCommand.args, "cwd:", cwd); + + // Spawn the ACP agent process + this.process = spawn(acpCommand.cmd, acpCommand.args, { + cwd, + stdio: ["pipe", "pipe", "pipe"], + env: { ...process.env }, + }); + + if (!this.process.stdin || !this.process.stdout) { + throw new Error("Failed to create stdio streams for ACP agent"); + } + + // Log stderr for debugging + this.process.stderr?.on("data", (data: Buffer) => { + this.debug("Agent stderr:", data.toString()); + }); + + this.process.on("error", (err) => { + this.debug("Agent process error:", err); + }); + + this.process.on("exit", (code, signal) => { + this.debug("Agent process exited:", code, signal); + this.connection = null; + this.process = null; + this.sessionId = null; + }); + + // Create the ACP stream from stdio + const stream = ndJsonStream( + nodeToWebWritable(this.process.stdin), + nodeToWebReadable(this.process.stdout) + ); + + // Store the progress callback for use in session updates + this.progressCallback = options?.onProgress ?? null; + + // Create the client handler + const createClient = (_agent: Agent): Client => ({ + sessionUpdate: async (params: SessionNotification) => { + this.debug("Session update:", params.update.sessionUpdate); + this.handleSessionUpdate(params.update); + }, + requestPermission: async (params: RequestPermissionRequest) => { + this.debug("Permission request:", params); + if (options?.onPermissionRequest) { + return options.onPermissionRequest(params); + } + // Default: allow with first option selected + return { + outcome: { + outcome: "selected", + optionId: params.options?.[0]?.optionId ?? "allow", + }, + }; + }, + }); + + // Create the client-side connection + this.connection = new ClientSideConnection(createClient, stream); + this.currentProvider = provider; + + // Initialize the connection + this.debug("Initializing ACP connection..."); + const initResponse = await this.connection.initialize({ + protocolVersion: 1, + clientInfo: { + name: "obsidian-llm-plugin", + version: "1.0.0", + }, + clientCapabilities: {}, + }); + + this.debug("ACP initialized:", initResponse); + + // Create a new session + this.debug("Creating new session..."); + const sessionResponse = await this.connection.newSession({ + cwd, + mcpServers: [], + }); + + this.sessionId = sessionResponse.sessionId; + this.debug("Session created:", this.sessionId); + } + + /** + * Handle session update notifications and convert to ProgressEvents + */ + private handleSessionUpdate(update: SessionUpdate) { + if (!this.progressCallback) return; + + switch (update.sessionUpdate) { + case "agent_message_chunk": + case "user_message_chunk": { + // ContentChunk has a single content block, not an array + const chunk = update as ContentChunk & { sessionUpdate: string }; + if (chunk.content && chunk.content.type === "text") { + const textContent = chunk.content as { type: "text"; text: string }; + this.progressCallback({ + type: "text", + content: textContent.text, + }); + } + break; + } + + case "agent_thought_chunk": { + const chunk = update as ContentChunk & { sessionUpdate: string }; + if (chunk.content && chunk.content.type === "text") { + const textContent = chunk.content as { type: "text"; text: string }; + this.progressCallback({ + type: "thinking", + content: textContent.text, + }); + } + break; + } + + case "tool_call": { + // ToolCall has title and toolCallId, not name/arguments + const toolCall = update as ToolCall & { sessionUpdate: string }; + this.progressCallback({ + type: "tool_use", + tool: toolCall.title ?? "unknown", + input: toolCall.toolCallId, + }); + break; + } + + default: + this.debug("Unhandled session update type:", update.sessionUpdate); + } + } + + /** + * Send a prompt to the agent + */ + async prompt( + message: string, + options?: AcpExecutorOptions + ): Promise<{ content: string; error?: string }> { + if (!this.connection || !this.sessionId) { + throw new Error("Not connected to an ACP agent. Call connect() first."); + } + + // Update progress callback if provided + if (options?.onProgress) { + this.progressCallback = options.onProgress; + } + + this.debug("Sending prompt:", message.slice(0, 100)); + + try { + const response = await this.connection.prompt({ + sessionId: this.sessionId, + prompt: [{ type: "text", text: message }], + }); + + this.debug("Prompt response:", response); + + // The response indicates completion - actual content comes via sessionUpdate + // Return empty content for now (content was streamed via callbacks) + return { content: "" }; + } catch (err) { + const error = err instanceof Error ? err.message : String(err); + this.debug("Prompt error:", error); + return { content: "", error }; + } + } + + /** + * Cancel any ongoing request + */ + async cancel(): Promise { + if (this.connection && this.sessionId) { + this.debug("Cancelling session:", this.sessionId); + await this.connection.cancel({ sessionId: this.sessionId }); + } + } + + /** + * Disconnect from the agent + */ + async disconnect(): Promise { + this.debug("Disconnecting..."); + + if (this.process) { + this.process.kill(); + this.process = null; + } + + this.connection = null; + this.sessionId = null; + this.currentProvider = null; + this.progressCallback = null; + } + + /** + * Check if connected + */ + isConnected(): boolean { + return this.connection !== null && this.sessionId !== null; + } + + /** + * Get current provider + */ + getProvider(): LLMProvider | null { + return this.currentProvider; + } +} From 711c45e808f38bb0c475d7b4a2919434e6f95f65 Mon Sep 17 00:00:00 2001 From: Brian Thorne Date: Mon, 26 Jan 2026 21:21:30 +1300 Subject: [PATCH 02/13] Wire up ACP executor with settings toggle - Add useAcp setting to ProviderConfig for claude, opencode, gemini - Add ACP_SUPPORTED_PROVIDERS constant - Add "Use ACP Mode (Experimental)" toggle in provider settings - Integrate AcpExecutor in ChatView: - Checks if ACP is enabled for current provider - Connects on first message, reuses connection for subsequent - Falls back to regular CLI executor when ACP disabled - Properly disconnects on view close All 39 tests passing. Co-Authored-By: Claude Opus 4.5 --- src/settings/SettingsTab.ts | 16 +++++++++++- src/types.ts | 7 +++++ src/views/ChatView.ts | 51 ++++++++++++++++++++++++++++++++----- 3 files changed, 66 insertions(+), 8 deletions(-) diff --git a/src/settings/SettingsTab.ts b/src/settings/SettingsTab.ts index db4faff..9b30dd2 100644 --- a/src/settings/SettingsTab.ts +++ b/src/settings/SettingsTab.ts @@ -1,7 +1,7 @@ import { App, FuzzySuggestModal, PluginSettingTab, Setting, TFile } from "obsidian"; import type LLMPlugin from "../../main"; import type { LLMProvider } from "../types"; -import { PROVIDER_MODELS } from "../types"; +import { PROVIDER_MODELS, ACP_SUPPORTED_PROVIDERS } from "../types"; /** * Modal for selecting a markdown file from the vault @@ -268,6 +268,20 @@ export class LLMSettingTab extends PluginSettingTab { }); } + // ACP mode for supported providers + if (ACP_SUPPORTED_PROVIDERS.includes(provider)) { + new Setting(settingsContainer) + .setName("Use ACP Mode (Experimental)") + .setDesc("Use Agent Client Protocol for persistent connection. Faster for multiple messages but may be less stable.") + .addToggle((toggle) => { + toggle.setValue(providerConfig.useAcp ?? false); + toggle.onChange(async (value) => { + this.plugin.settings.providers[provider].useAcp = value; + await this.plugin.saveSettings(); + }); + }); + } + // Timeout override (optional) const timeoutSetting = new Setting(settingsContainer) .setName("Timeout Override (seconds)") diff --git a/src/types.ts b/src/types.ts index d088408..228728b 100644 --- a/src/types.ts +++ b/src/types.ts @@ -21,8 +21,15 @@ export interface ProviderConfig { timeout?: number; /** Gemini: Enable yolo mode (auto-confirm dangerous operations) */ yoloMode?: boolean; + /** Use ACP (Agent Client Protocol) for persistent connection (supported: claude, opencode, gemini) */ + useAcp?: boolean; } +/** + * Providers that support ACP (Agent Client Protocol) + */ +export const ACP_SUPPORTED_PROVIDERS: LLMProvider[] = ["claude", "opencode", "gemini"]; + /** * Common model options per provider */ diff --git a/src/views/ChatView.ts b/src/views/ChatView.ts index cb8e3eb..0c699cc 100644 --- a/src/views/ChatView.ts +++ b/src/views/ChatView.ts @@ -11,7 +11,9 @@ import { } from "obsidian"; import type LLMPlugin from "../../main"; import type { LLMProvider, ConversationMessage, ProgressEvent } from "../types"; +import { ACP_SUPPORTED_PROVIDERS } from "../types"; import { LLMExecutor } from "../executor/LLMExecutor"; +import { AcpExecutor } from "../executor/AcpExecutor"; export const CHAT_VIEW_TYPE = "llm-chat-view"; @@ -25,6 +27,7 @@ const PROVIDER_DISPLAY_NAMES: Record = { export class ChatView extends ItemView { plugin: LLMPlugin; private executor: LLMExecutor; + private acpExecutor: AcpExecutor; private messages: ConversationMessage[] = []; private currentProvider: LLMProvider; private isLoading = false; @@ -44,6 +47,7 @@ export class ChatView extends ItemView { super(leaf); this.plugin = plugin; this.executor = new LLMExecutor(plugin.settings); + this.acpExecutor = new AcpExecutor(plugin.settings); this.currentProvider = plugin.settings.defaultProvider; } @@ -74,6 +78,7 @@ export class ChatView extends ItemView { async onClose() { this.executor.cancel(); + await this.acpExecutor.disconnect(); // Clean up markdown components this.markdownComponents.forEach((c) => c.unload()); this.markdownComponents = []; @@ -495,13 +500,45 @@ export class ChatView extends ItemView { // eslint-disable-next-line @typescript-eslint/no-explicit-any const vaultPath = (this.app.vault.adapter as any).basePath as string | undefined; - const response = await this.executor.execute( - contextPrompt, - this.currentProvider, - this.plugin.settings.streamOutput ? onStream : undefined, - onProgress, - vaultPath - ); + // Check if ACP mode is enabled for this provider + const providerConfig = this.plugin.settings.providers[this.currentProvider]; + const useAcp = providerConfig.useAcp && ACP_SUPPORTED_PROVIDERS.includes(this.currentProvider); + + let response; + if (useAcp) { + // Use ACP executor for persistent connection + try { + // Connect if not already connected or provider changed + if (!this.acpExecutor.isConnected() || this.acpExecutor.getProvider() !== this.currentProvider) { + onProgress({ type: "status", message: "Connecting to ACP agent..." }); + await this.acpExecutor.connect(this.currentProvider, vaultPath, { onProgress }); + } + + const acpResponse = await this.acpExecutor.prompt(contextPrompt, { onProgress }); + response = { + content: streamedContent || acpResponse.content, // Prefer streamed content + provider: this.currentProvider, + durationMs: 0, + error: acpResponse.error, + }; + } catch (err) { + response = { + content: "", + provider: this.currentProvider, + durationMs: 0, + error: err instanceof Error ? err.message : String(err), + }; + } + } else { + // Use regular CLI executor + response = await this.executor.execute( + contextPrompt, + this.currentProvider, + this.plugin.settings.streamOutput ? onStream : undefined, + onProgress, + vaultPath + ); + } if (response.error) { this.showError(response.error); From 32c76f82b92c2f51037ad8ea7238349b7ec111c3 Mon Sep 17 00:00:00 2001 From: Brian Thorne Date: Mon, 26 Jan 2026 21:30:56 +1300 Subject: [PATCH 03/13] Add ACP mode tests - Settings toggle test: verifies useAcp setting persists - ACP live test: sends message via ACP mode with OpenCode - Benchmark test: measures first vs second message timing Tests tagged: - @acp - all ACP tests - @acp-live - requires real ACP connection - @acp-benchmark - timing measurements Co-Authored-By: Claude Opus 4.5 --- test/specs/providers.e2e.ts | 235 ++++++++++++++++++++++++++++++++++++ 1 file changed, 235 insertions(+) diff --git a/test/specs/providers.e2e.ts b/test/specs/providers.e2e.ts index f5b0ee3..51ac982 100644 --- a/test/specs/providers.e2e.ts +++ b/test/specs/providers.e2e.ts @@ -878,3 +878,238 @@ describe("Vault File Interactions @files @provider", () => { } }); }); + +/** + * ACP (Agent Client Protocol) Tests + * Tests for the experimental ACP mode which uses persistent connections + */ +describe("ACP Mode Tests @acp @provider", () => { + before(async () => { + await browser.waitUntil( + async () => { + const workspace = await browser.$(".workspace"); + return workspace.isExisting(); + }, + { timeout: 10000 } + ); + + // Close any existing chat views + await browser.execute(() => { + const app = (window as any).app; + app?.workspace?.detachLeavesOfType?.("llm-chat-view"); + }); + await browser.pause(200); + }); + + afterEach(async () => { + // Close chat view between tests + await browser.execute(() => { + const app = (window as any).app; + app?.workspace?.detachLeavesOfType?.("llm-chat-view"); + }); + await browser.pause(200); + }); + + /** + * Helper to enable ACP mode for a provider + */ + async function enableAcpMode(provider: string): Promise { + await browser.execute((p) => { + const plugin = (window as any).app?.plugins?.plugins?.["obsidian-llm"]; + if (plugin?.settings?.providers?.[p]) { + plugin.settings.providers[p].enabled = true; + plugin.settings.providers[p].useAcp = true; + plugin.settings.defaultProvider = p; + plugin.saveSettings(); + } + }, provider); + await browser.pause(200); + } + + /** + * Helper to disable ACP mode for a provider + */ + async function disableAcpMode(provider: string): Promise { + await browser.execute((p) => { + const plugin = (window as any).app?.plugins?.plugins?.["obsidian-llm"]; + if (plugin?.settings?.providers?.[p]) { + plugin.settings.providers[p].useAcp = false; + plugin.saveSettings(); + } + }, provider); + await browser.pause(200); + } + + /** + * Helper to check if ACP mode is enabled + */ + async function isAcpEnabled(provider: string): Promise { + return await browser.execute((p) => { + const plugin = (window as any).app?.plugins?.plugins?.["obsidian-llm"]; + return plugin?.settings?.providers?.[p]?.useAcp === true; + }, provider); + } + + it("should show ACP toggle in settings for supported providers", async () => { + // Verify ACP setting exists in the plugin settings via execute + const acpSettingExists = await browser.execute(() => { + const plugin = (window as any).app?.plugins?.plugins?.["obsidian-llm"]; + // Check that the useAcp property is defined in the type (settings schema) + // And that ACP_SUPPORTED_PROVIDERS includes claude + return plugin !== undefined; + }); + + expect(acpSettingExists).toBe(true); + + // Enable ACP for Claude and verify it persists + await browser.execute(() => { + const plugin = (window as any).app?.plugins?.plugins?.["obsidian-llm"]; + if (plugin?.settings?.providers?.claude) { + plugin.settings.providers.claude.useAcp = true; + plugin.saveSettings(); + } + }); + await browser.pause(200); + + const claudeAcpEnabled = await browser.execute(() => { + const plugin = (window as any).app?.plugins?.plugins?.["obsidian-llm"]; + return plugin?.settings?.providers?.claude?.useAcp === true; + }); + + expect(claudeAcpEnabled).toBe(true); + + // Clean up + await browser.execute(() => { + const plugin = (window as any).app?.plugins?.plugins?.["obsidian-llm"]; + if (plugin?.settings?.providers?.claude) { + plugin.settings.providers.claude.useAcp = false; + plugin.saveSettings(); + } + }); + }); + + it("should persist ACP mode setting", async () => { + // Enable ACP for OpenCode + await enableAcpMode("opencode"); + + // Verify it's enabled + const isEnabled = await isAcpEnabled("opencode"); + expect(isEnabled).toBe(true); + + // Disable it + await disableAcpMode("opencode"); + + // Verify it's disabled + const isDisabled = await isAcpEnabled("opencode"); + expect(isDisabled).toBe(false); + }); + + it("should send message with ACP mode enabled @slow @acp-live", async () => { + // Enable ACP for OpenCode (has native ACP support) + await enableAcpMode("opencode"); + + // Open chat + await browser.executeObsidianCommand("obsidian-llm:open-llm-chat"); + await browser.pause(500); + + const chatView = await browser.$(".llm-chat-view"); + expect(await chatView.isExisting()).toBe(true); + + // Send a simple message + const input = await browser.$(".llm-chat-input"); + await input.click(); + await input.setValue("Say 'ACP works' and nothing else."); + + const sendBtn = await browser.$(".llm-chat-send"); + await sendBtn.click(); + + // Wait for response (ACP might show "Connecting to ACP agent..." first) + await browser.waitUntil( + async () => { + const response = await browser.$(".llm-message-assistant"); + return response.isExisting(); + }, + { timeout: 60000, timeoutMsg: "No response from ACP agent" } + ); + + const responseEl = await browser.$(".llm-message-assistant"); + const responseText = await responseEl.getText(); + console.log("ACP response:", responseText); + + expect(responseText.length).toBeGreaterThan(0); + + // Clean up - disable ACP mode + await disableAcpMode("opencode"); + }); + + it("should measure ACP connection reuse @slow @acp-benchmark", async () => { + // This test measures if ACP connection reuse is working + // The second message should be faster than the first (no connection overhead) + + const provider = "opencode"; + const testPrompt = "Say 'hi' and nothing else."; + + // Enable ACP mode + await enableAcpMode(provider); + + await browser.executeObsidianCommand("obsidian-llm:open-llm-chat"); + await browser.pause(500); + + // First message (includes connection time) + const startTime1 = Date.now(); + + let input = await browser.$(".llm-chat-input"); + await input.click(); + await input.setValue(testPrompt); + + let sendBtn = await browser.$(".llm-chat-send"); + await sendBtn.click(); + + await browser.waitUntil( + async () => { + const responses = await browser.$$(".llm-message-assistant"); + return responses.length >= 1; + }, + { timeout: 60000, timeoutMsg: "First ACP message timed out" } + ); + + const time1 = Date.now() - startTime1; + console.log(`ACP first message (with connection): ${time1}ms`); + + // Second message (reuses connection) + const startTime2 = Date.now(); + + input = await browser.$(".llm-chat-input"); + await input.click(); + await input.setValue(testPrompt); + + sendBtn = await browser.$(".llm-chat-send"); + await sendBtn.click(); + + await browser.waitUntil( + async () => { + const responses = await browser.$$(".llm-message-assistant"); + return responses.length >= 2; + }, + { timeout: 60000, timeoutMsg: "Second ACP message timed out" } + ); + + const time2 = Date.now() - startTime2; + console.log(`ACP second message (reusing connection): ${time2}ms`); + + // Log results + console.log("\n=== ACP Benchmark Results ==="); + console.log(`First message: ${time1}ms`); + console.log(`Second message: ${time2}ms`); + if (time2 < time1) { + console.log(`Connection reuse saved: ${time1 - time2}ms (${((time1 - time2) / time1 * 100).toFixed(1)}%)`); + } + + // Verify both messages got responses + const responses = await browser.$$(".llm-message-assistant"); + expect(responses.length).toBeGreaterThanOrEqual(2); + + // Clean up + await disableAcpMode(provider); + }); +}); From c9fd22461b515218dc02a3298efc5e5142552091 Mon Sep 17 00:00:00 2001 From: Brian Thorne Date: Mon, 26 Jan 2026 21:36:53 +1300 Subject: [PATCH 04/13] Add model selection and tests for all ACP providers AcpExecutor: - Add unstable_setSessionModel call after session creation - Uses configured model from provider settings - Gracefully handles if model selection not supported Tests added: - @acp-model: Verifies model is configured for ACP session - @acp-claude: Tests Claude via @zed-industries/claude-code-acp - @acp-gemini: Tests Gemini via --experimental-acp All three ACP providers verified working: - OpenCode: opencode acp - Claude: npx @zed-industries/claude-code-acp - Gemini: gemini --experimental-acp Co-Authored-By: Claude Opus 4.5 --- src/executor/AcpExecutor.ts | 16 ++++ test/specs/providers.e2e.ts | 162 ++++++++++++++++++++++++++++++++++++ 2 files changed, 178 insertions(+) diff --git a/src/executor/AcpExecutor.ts b/src/executor/AcpExecutor.ts index 12b34eb..45fffcf 100644 --- a/src/executor/AcpExecutor.ts +++ b/src/executor/AcpExecutor.ts @@ -224,6 +224,22 @@ export class AcpExecutor { this.sessionId = sessionResponse.sessionId; this.debug("Session created:", this.sessionId); + + // Set model if configured + const providerConfig = this.settings.providers[provider]; + if (providerConfig.model) { + this.debug("Setting model:", providerConfig.model); + try { + await this.connection.unstable_setSessionModel({ + sessionId: this.sessionId, + modelId: providerConfig.model, + }); + this.debug("Model set successfully"); + } catch (err) { + // Model selection is experimental - log but don't fail + this.debug("Failed to set model (may not be supported):", err); + } + } } /** diff --git a/test/specs/providers.e2e.ts b/test/specs/providers.e2e.ts index 51ac982..7facf15 100644 --- a/test/specs/providers.e2e.ts +++ b/test/specs/providers.e2e.ts @@ -1042,6 +1042,168 @@ describe("ACP Mode Tests @acp @provider", () => { await disableAcpMode("opencode"); }); + it("should use configured model with ACP @slow @acp-model", async () => { + // Enable ACP for OpenCode with a specific model + const testModel = "gpt-4o-mini"; + + await browser.execute((model) => { + const plugin = (window as any).app?.plugins?.plugins?.["obsidian-llm"]; + if (plugin?.settings?.providers?.opencode) { + plugin.settings.providers.opencode.enabled = true; + plugin.settings.providers.opencode.useAcp = true; + plugin.settings.providers.opencode.model = model; + plugin.settings.defaultProvider = "opencode"; + plugin.settings.debugMode = true; // Enable debug to see model selection + plugin.saveSettings(); + } + }, testModel); + await browser.pause(200); + + // Verify model is set + const configuredModel = await browser.execute(() => { + const plugin = (window as any).app?.plugins?.plugins?.["obsidian-llm"]; + return plugin?.settings?.providers?.opencode?.model; + }); + expect(configuredModel).toBe(testModel); + + // Open chat and send a message + await browser.executeObsidianCommand("obsidian-llm:open-llm-chat"); + await browser.pause(500); + + const input = await browser.$(".llm-chat-input"); + await input.click(); + await input.setValue("What model are you? Reply with just your model name."); + + const sendBtn = await browser.$(".llm-chat-send"); + await sendBtn.click(); + + // Wait for response + await browser.waitUntil( + async () => { + const response = await browser.$(".llm-message-assistant"); + return response.isExisting(); + }, + { timeout: 60000, timeoutMsg: "No response from ACP agent" } + ); + + const responseEl = await browser.$(".llm-message-assistant"); + const responseText = await responseEl.getText(); + console.log("Model response:", responseText); + console.log("Configured model:", testModel); + + // The response should mention something about GPT-4 or the model + // (exact response depends on what the model says about itself) + expect(responseText.length).toBeGreaterThan(0); + + // Clean up + await browser.execute(() => { + const plugin = (window as any).app?.plugins?.plugins?.["obsidian-llm"]; + if (plugin?.settings?.providers?.opencode) { + plugin.settings.providers.opencode.useAcp = false; + plugin.settings.providers.opencode.model = ""; + plugin.settings.debugMode = false; + plugin.saveSettings(); + } + }); + }); + + it("should work with Claude ACP @slow @acp-claude", async () => { + // Test Claude via ACP adapter (@zed-industries/claude-code-acp) + await browser.execute(() => { + const plugin = (window as any).app?.plugins?.plugins?.["obsidian-llm"]; + if (plugin?.settings?.providers?.claude) { + plugin.settings.providers.claude.enabled = true; + plugin.settings.providers.claude.useAcp = true; + plugin.settings.providers.claude.model = "claude-3-5-haiku-latest"; + plugin.settings.defaultProvider = "claude"; + plugin.saveSettings(); + } + }); + await browser.pause(200); + + await browser.executeObsidianCommand("obsidian-llm:open-llm-chat"); + await browser.pause(500); + + const input = await browser.$(".llm-chat-input"); + await input.click(); + await input.setValue("Say 'Claude ACP works' and nothing else."); + + const sendBtn = await browser.$(".llm-chat-send"); + await sendBtn.click(); + + await browser.waitUntil( + async () => { + const response = await browser.$(".llm-message-assistant"); + return response.isExisting(); + }, + { timeout: 90000, timeoutMsg: "No response from Claude ACP" } + ); + + const responseEl = await browser.$(".llm-message-assistant"); + const responseText = await responseEl.getText(); + console.log("Claude ACP response:", responseText); + + expect(responseText.length).toBeGreaterThan(0); + + // Clean up + await browser.execute(() => { + const plugin = (window as any).app?.plugins?.plugins?.["obsidian-llm"]; + if (plugin?.settings?.providers?.claude) { + plugin.settings.providers.claude.useAcp = false; + plugin.saveSettings(); + } + }); + }); + + it("should work with Gemini ACP @slow @acp-gemini", async () => { + // Test Gemini with --experimental-acp flag + await browser.execute(() => { + const plugin = (window as any).app?.plugins?.plugins?.["obsidian-llm"]; + if (plugin?.settings?.providers?.gemini) { + plugin.settings.providers.gemini.enabled = true; + plugin.settings.providers.gemini.useAcp = true; + plugin.settings.providers.gemini.yoloMode = true; + plugin.settings.providers.gemini.model = "gemini-2.5-flash"; + plugin.settings.defaultProvider = "gemini"; + plugin.saveSettings(); + } + }); + await browser.pause(200); + + await browser.executeObsidianCommand("obsidian-llm:open-llm-chat"); + await browser.pause(500); + + const input = await browser.$(".llm-chat-input"); + await input.click(); + await input.setValue("Say 'Gemini ACP works' and nothing else."); + + const sendBtn = await browser.$(".llm-chat-send"); + await sendBtn.click(); + + await browser.waitUntil( + async () => { + const response = await browser.$(".llm-message-assistant"); + return response.isExisting(); + }, + { timeout: 90000, timeoutMsg: "No response from Gemini ACP" } + ); + + const responseEl = await browser.$(".llm-message-assistant"); + const responseText = await responseEl.getText(); + console.log("Gemini ACP response:", responseText); + + expect(responseText.length).toBeGreaterThan(0); + + // Clean up + await browser.execute(() => { + const plugin = (window as any).app?.plugins?.plugins?.["obsidian-llm"]; + if (plugin?.settings?.providers?.gemini) { + plugin.settings.providers.gemini.useAcp = false; + plugin.saveSettings(); + } + }); + }); + it("should measure ACP connection reuse @slow @acp-benchmark", async () => { // This test measures if ACP connection reuse is working // The second message should be faster than the first (no connection overhead) From b1b74a4e524ec55c9823459ca9420424d75368f9 Mon Sep 17 00:00:00 2001 From: Brian Thorne Date: Mon, 26 Jan 2026 22:47:23 +1300 Subject: [PATCH 05/13] Update model lists and add dynamic model fetching - Update PROVIDER_MODELS with current model IDs (Jan 2026): - Claude: 4.5 Opus/Sonnet/Haiku, 4 Sonnet, 3.7 Sonnet - Gemini: 3 Pro/Flash Preview, 2.5 Pro/Flash - OpenCode: Claude 4.5 models, GPT-5 via Copilot - Codex: o3, o4-mini, GPT-5 - Add dynamic model fetching for OpenCode via `opencode models` CLI - Add "Custom model..." option to dropdown for custom model IDs - Show custom text input when custom option selected - Cache fetched models for 5 minutes to avoid repeated CLI calls --- src/settings/SettingsTab.ts | 144 ++++++++++++++++++++++++++++++++---- src/types.ts | 29 ++++---- src/utils/modelFetcher.ts | 116 +++++++++++++++++++++++++++++ 3 files changed, 260 insertions(+), 29 deletions(-) create mode 100644 src/utils/modelFetcher.ts diff --git a/src/settings/SettingsTab.ts b/src/settings/SettingsTab.ts index 9b30dd2..eacea33 100644 --- a/src/settings/SettingsTab.ts +++ b/src/settings/SettingsTab.ts @@ -1,7 +1,8 @@ -import { App, FuzzySuggestModal, PluginSettingTab, Setting, TFile } from "obsidian"; +import { App, DropdownComponent, FuzzySuggestModal, PluginSettingTab, Setting, TFile } from "obsidian"; import type LLMPlugin from "../../main"; import type { LLMProvider } from "../types"; import { PROVIDER_MODELS, ACP_SUPPORTED_PROVIDERS } from "../types"; +import { fetchModelsForProvider, type ModelOption } from "../utils/modelFetcher"; /** * Modal for selecting a markdown file from the vault @@ -226,21 +227,8 @@ export class LLMSettingTab extends PluginSettingTab { }); }); - // Model selection - const modelOptions = PROVIDER_MODELS[provider]; - new Setting(settingsContainer) - .setName("Model") - .setDesc("Select which model to use for this provider") - .addDropdown((dropdown) => { - modelOptions.forEach((option) => { - dropdown.addOption(option.value, option.label); - }); - dropdown.setValue(providerConfig.model ?? ""); - dropdown.onChange(async (value) => { - this.plugin.settings.providers[provider].model = value || undefined; - await this.plugin.saveSettings(); - }); - }); + // Model selection - with dynamic fetching and custom input + this.addModelSetting(settingsContainer, provider, providerConfig.model ?? ""); new Setting(settingsContainer) .setName("Custom Command") @@ -322,6 +310,130 @@ export class LLMSettingTab extends PluginSettingTab { }); } + /** + * Add model selection setting with dropdown + custom input + * Fetches available models dynamically for providers that support it + */ + private addModelSetting(container: HTMLElement, provider: LLMProvider, currentValue: string): void { + const setting = new Setting(container) + .setName("Model") + .setDesc("Select a model or enter a custom model ID"); + + let dropdown: DropdownComponent | null = null; + let customInput: HTMLInputElement | null = null; + let isCustomMode = false; + + // Check if current value is in the static list (to determine if using custom) + const staticModels = PROVIDER_MODELS[provider]; + const isCurrentValueInList = staticModels.some((m) => m.value === currentValue); + isCustomMode = currentValue !== "" && !isCurrentValueInList; + + // Add dropdown + setting.addDropdown((dd) => { + dropdown = dd; + + // Add static options first (will be updated with dynamic ones) + this.populateModelDropdown(dd, staticModels, currentValue, isCustomMode); + + dd.onChange(async (value) => { + if (value === "__custom__") { + // Switch to custom mode + if (customInput) { + customInput.style.display = "inline-block"; + customInput.focus(); + } + isCustomMode = true; + } else { + // Use selected model + if (customInput) { + customInput.style.display = "none"; + customInput.value = ""; + } + isCustomMode = false; + this.plugin.settings.providers[provider].model = value || undefined; + await this.plugin.saveSettings(); + } + }); + + // Fetch dynamic models in the background + if (provider === "opencode") { + this.fetchAndUpdateModels(dd, provider, currentValue, isCustomMode); + } + }); + + // Add custom input (hidden by default unless in custom mode) + customInput = setting.controlEl.createEl("input", { + type: "text", + cls: "llm-custom-model-input", + attr: { + placeholder: "Enter model ID...", + }, + }); + customInput.style.display = isCustomMode ? "inline-block" : "none"; + customInput.style.marginLeft = "8px"; + customInput.style.width = "150px"; + + if (isCustomMode) { + customInput.value = currentValue; + } + + customInput.addEventListener("change", async () => { + const value = customInput!.value.trim(); + this.plugin.settings.providers[provider].model = value || undefined; + await this.plugin.saveSettings(); + }); + } + + /** + * Populate dropdown with model options + */ + private populateModelDropdown( + dropdown: DropdownComponent, + models: ModelOption[], + currentValue: string, + isCustomMode: boolean + ): void { + // Clear existing options + dropdown.selectEl.empty(); + + // Add model options + models.forEach((option) => { + dropdown.addOption(option.value, option.label); + }); + + // Add custom option at the end + dropdown.addOption("__custom__", "Custom model..."); + + // Set current value + if (isCustomMode) { + dropdown.setValue("__custom__"); + } else { + dropdown.setValue(currentValue); + } + } + + /** + * Fetch models dynamically and update dropdown + */ + private async fetchAndUpdateModels( + dropdown: DropdownComponent, + provider: LLMProvider, + currentValue: string, + isCustomMode: boolean + ): Promise { + try { + const models = await fetchModelsForProvider(provider); + + // Check if current value is in the new list + const isInList = models.some((m) => m.value === currentValue); + const shouldUseCustom = isCustomMode || (currentValue !== "" && !isInList); + + this.populateModelDropdown(dropdown, models, currentValue, shouldUseCustom); + } catch { + // Keep static models on error + } + } + private getDefaultCommand(provider: LLMProvider): string { switch (provider) { case "claude": diff --git a/src/types.ts b/src/types.ts index 228728b..1d50718 100644 --- a/src/types.ts +++ b/src/types.ts @@ -32,35 +32,38 @@ export const ACP_SUPPORTED_PROVIDERS: LLMProvider[] = ["claude", "opencode", "ge /** * Common model options per provider + * Updated January 2026 - see each provider's documentation for latest models */ export const PROVIDER_MODELS: Record = { claude: [ { value: "", label: "Default (CLI default)" }, - { value: "claude-sonnet-4-20250514", label: "Claude Sonnet 4" }, - { value: "claude-opus-4-20250514", label: "Claude Opus 4" }, - { value: "claude-3-5-sonnet-latest", label: "Claude 3.5 Sonnet" }, - { value: "claude-3-5-haiku-latest", label: "Claude 3.5 Haiku (fast)" }, + { value: "claude-opus-4-5-20251101", label: "Claude 4.5 Opus (most capable)" }, + { value: "claude-sonnet-4-5-20250929", label: "Claude 4.5 Sonnet" }, + { value: "claude-haiku-4-5-20251001", label: "Claude 4.5 Haiku (fast)" }, + { value: "claude-sonnet-4-20250514", label: "Claude 4 Sonnet" }, + { value: "claude-3-7-sonnet-20250219", label: "Claude 3.7 Sonnet" }, ], gemini: [ { value: "", label: "Default (CLI default)" }, - { value: "gemini-3-flash-preview", label: "Gemini 3 Flash (fast)" }, - { value: "gemini-3-pro-preview", label: "Gemini 3 Pro" }, + { value: "gemini-3-pro-preview", label: "Gemini 3 Pro (preview)" }, + { value: "gemini-3-flash-preview", label: "Gemini 3 Flash (preview, fast)" }, { value: "gemini-2.5-pro", label: "Gemini 2.5 Pro" }, - { value: "gemini-2.5-flash", label: "Gemini 2.5 Flash" }, + { value: "gemini-2.5-flash", label: "Gemini 2.5 Flash (fast)" }, ], opencode: [ { value: "", label: "Default (CLI default)" }, - { value: "claude-sonnet", label: "Claude Sonnet" }, - { value: "claude-haiku", label: "Claude Haiku (fast)" }, - { value: "gpt-4o", label: "GPT-4o" }, - { value: "gpt-4o-mini", label: "GPT-4o Mini (fast)" }, + { value: "anthropic/claude-sonnet-4-5", label: "Claude 4.5 Sonnet" }, + { value: "anthropic/claude-opus-4-5", label: "Claude 4.5 Opus" }, + { value: "anthropic/claude-haiku-4-5", label: "Claude 4.5 Haiku (fast)" }, + { value: "github-copilot/gpt-5", label: "GPT-5 (Copilot)" }, + { value: "github-copilot/gpt-5-mini", label: "GPT-5 Mini (Copilot, fast)" }, ], codex: [ { value: "", label: "Default (CLI default)" }, + { value: "o3", label: "o3 (reasoning)" }, + { value: "o4-mini", label: "o4-mini (reasoning, fast)" }, { value: "gpt-5", label: "GPT-5" }, { value: "gpt-5-mini", label: "GPT-5 Mini (fast)" }, - { value: "gpt-5-nano", label: "GPT-5 Nano (fastest)" }, - { value: "gpt-4.1", label: "GPT-4.1" }, ], }; diff --git a/src/utils/modelFetcher.ts b/src/utils/modelFetcher.ts new file mode 100644 index 0000000..9c4430b --- /dev/null +++ b/src/utils/modelFetcher.ts @@ -0,0 +1,116 @@ +/** + * Utility for dynamically fetching available models from CLI tools + */ +import { exec } from "child_process"; +import { promisify } from "util"; +import type { LLMProvider } from "../types"; +import { PROVIDER_MODELS } from "../types"; + +const execAsync = promisify(exec); + +export interface ModelOption { + value: string; + label: string; +} + +// Cache fetched models to avoid repeated CLI calls +const modelCache: Map = new Map(); +const CACHE_TTL_MS = 5 * 60 * 1000; // 5 minutes + +/** + * Get models for OpenCode by calling `opencode models` + */ +async function fetchOpenCodeModels(): Promise { + try { + const { stdout } = await execAsync("opencode models", { timeout: 10000 }); + const models: ModelOption[] = [{ value: "", label: "Default (CLI default)" }]; + + // Parse the output - each line has "provider/model" format + // Skip lines that don't look like model IDs (e.g., INFO logs) + const lines = stdout.trim().split("\n"); + for (const line of lines) { + const model = line.trim(); + // Skip empty lines, INFO/WARN/ERROR logs, and lines without / + if (!model || model.startsWith("INFO") || model.startsWith("WARN") || model.startsWith("ERROR")) { + continue; + } + if (model.includes("/")) { + // Create a friendly label from the model ID + const [provider, name] = model.split("/", 2); + const label = `${name} (${provider})`; + models.push({ value: model, label }); + } + } + + return models.length > 1 ? models : PROVIDER_MODELS.opencode; + } catch { + // CLI not available or failed - use static fallback + return PROVIDER_MODELS.opencode; + } +} + +/** + * Get models for Claude (currently static, could add `claude --list-models` if available) + */ +async function fetchClaudeModels(): Promise { + // Claude Code CLI doesn't have a list-models command yet + // Return static list + return PROVIDER_MODELS.claude; +} + +/** + * Get models for Gemini (currently static) + */ +async function fetchGeminiModels(): Promise { + // Gemini CLI doesn't have a list-models command that we know of + return PROVIDER_MODELS.gemini; +} + +/** + * Get models for Codex (currently static) + */ +async function fetchCodexModels(): Promise { + return PROVIDER_MODELS.codex; +} + +/** + * Fetch available models for a provider + * Uses caching to avoid repeated CLI calls + */ +export async function fetchModelsForProvider(provider: LLMProvider): Promise { + // Check cache first + const cached = modelCache.get(provider); + if (cached && Date.now() - cached.timestamp < CACHE_TTL_MS) { + return cached.models; + } + + let models: ModelOption[]; + switch (provider) { + case "opencode": + models = await fetchOpenCodeModels(); + break; + case "claude": + models = await fetchClaudeModels(); + break; + case "gemini": + models = await fetchGeminiModels(); + break; + case "codex": + models = await fetchCodexModels(); + break; + default: + models = [{ value: "", label: "Default" }]; + } + + // Update cache + modelCache.set(provider, { models, timestamp: Date.now() }); + + return models; +} + +/** + * Clear the model cache (useful after settings changes) + */ +export function clearModelCache(): void { + modelCache.clear(); +} From cfad3b80635c68b0456d512f34b842427db12953 Mon Sep 17 00:00:00 2001 From: Brian Thorne Date: Mon, 26 Jan 2026 22:50:04 +1300 Subject: [PATCH 06/13] Add thinking mode support for ACP connections - Add thinkingMode setting to ProviderConfig for extended thinking levels - Capture config options from ACP session response - Add methods to AcpExecutor for thinking mode: - getThinkingOptions(): Get available thinking levels from agent - getCurrentThinkingMode(): Get current thinking level - setThinkingMode(value): Set thinking level via unstable_setSessionConfigOption - supportsThinkingMode(): Check if agent supports thought_level config - Apply thinking mode after session creation if configured - Add thinking mode text input in settings (for ACP providers) - Common values: none, low, medium, high (agent-specific) --- src/executor/AcpExecutor.ts | 126 ++++++++++++++++++++++++++++++++++++ src/settings/SettingsTab.ts | 13 ++++ src/types.ts | 2 + 3 files changed, 141 insertions(+) diff --git a/src/executor/AcpExecutor.ts b/src/executor/AcpExecutor.ts index 45fffcf..3cbd9e5 100644 --- a/src/executor/AcpExecutor.ts +++ b/src/executor/AcpExecutor.ts @@ -17,9 +17,15 @@ import { type RequestPermissionResponse, type ContentChunk, type ToolCall, + type SessionConfigOption, } from "@agentclientprotocol/sdk"; import type { LLMPluginSettings, LLMProvider, ProgressEvent } from "../types"; +export interface ThinkingOption { + id: string; + name: string; +} + // Convert Node streams to Web streams function nodeToWebReadable(nodeStream: NodeJS.ReadableStream): ReadableStream { return new ReadableStream({ @@ -82,6 +88,7 @@ export class AcpExecutor { private currentProvider: LLMProvider | null = null; private debug: (...args: unknown[]) => void; private progressCallback: ((event: ProgressEvent) => void) | null = null; + private configOptions: SessionConfigOption[] = []; constructor(settings: LLMPluginSettings) { this.settings = settings; @@ -225,6 +232,10 @@ export class AcpExecutor { this.sessionId = sessionResponse.sessionId; this.debug("Session created:", this.sessionId); + // Store config options from session response + this.configOptions = sessionResponse.configOptions ?? []; + this.debug("Config options available:", this.configOptions.map((o) => o.id)); + // Set model if configured const providerConfig = this.settings.providers[provider]; if (providerConfig.model) { @@ -240,6 +251,11 @@ export class AcpExecutor { this.debug("Failed to set model (may not be supported):", err); } } + + // Set thinking mode if configured and available + if (providerConfig.thinkingMode) { + await this.setThinkingMode(providerConfig.thinkingMode); + } } /** @@ -337,6 +353,115 @@ export class AcpExecutor { } } + /** + * Get available thinking/reasoning options from the agent + * Returns null if thinking mode is not supported + */ + getThinkingOptions(): ThinkingOption[] | null { + const thoughtLevelOption = this.configOptions.find( + (opt) => opt.category === "thought_level" + ); + + if (!thoughtLevelOption) { + return null; + } + + // Extract options from the config (handles both flat options and groups) + const options: ThinkingOption[] = []; + const selectOptions = (thoughtLevelOption as { options?: unknown }).options; + + if (Array.isArray(selectOptions)) { + for (const opt of selectOptions) { + if (typeof opt === "object" && opt !== null) { + // Could be a direct option or a group + if ("group" in opt && "options" in opt) { + // It's a group - extract options from it + const groupOpts = (opt as { options: unknown[] }).options; + for (const groupOpt of groupOpts) { + if (typeof groupOpt === "object" && groupOpt !== null && "id" in groupOpt) { + const typedOpt = groupOpt as { id: string; name?: string }; + options.push({ + id: typedOpt.id, + name: typedOpt.name ?? typedOpt.id, + }); + } + } + } else if ("id" in opt) { + // Direct option + const typedOpt = opt as { id: string; name?: string }; + options.push({ + id: typedOpt.id, + name: typedOpt.name ?? typedOpt.id, + }); + } + } + } + } + + return options.length > 0 ? options : null; + } + + /** + * Get the current thinking mode value + */ + getCurrentThinkingMode(): string | null { + const thoughtLevelOption = this.configOptions.find( + (opt) => opt.category === "thought_level" + ); + + if (!thoughtLevelOption) { + return null; + } + + return (thoughtLevelOption as { currentValue?: string }).currentValue ?? null; + } + + /** + * Set the thinking/reasoning mode + */ + async setThinkingMode(value: string): Promise { + if (!this.connection || !this.sessionId) { + this.debug("Cannot set thinking mode - not connected"); + return false; + } + + const thoughtLevelOption = this.configOptions.find( + (opt) => opt.category === "thought_level" + ); + + if (!thoughtLevelOption) { + this.debug("Thinking mode not supported by this agent"); + return false; + } + + try { + this.debug("Setting thinking mode to:", value); + const response = await this.connection.unstable_setSessionConfigOption({ + sessionId: this.sessionId, + configId: thoughtLevelOption.id, + value, + }); + + // Update local config options with response + if (response.configOptions) { + this.configOptions = response.configOptions; + } + + this.debug("Thinking mode set successfully"); + return true; + } catch (err) { + this.debug("Failed to set thinking mode:", err); + return false; + } + } + + /** + * Check if thinking mode is supported + */ + supportsThinkingMode(): boolean { + return this.getThinkingOptions() !== null; + } + /** * Disconnect from the agent */ @@ -352,6 +477,7 @@ export class AcpExecutor { this.sessionId = null; this.currentProvider = null; this.progressCallback = null; + this.configOptions = []; } /** diff --git a/src/settings/SettingsTab.ts b/src/settings/SettingsTab.ts index eacea33..4b91512 100644 --- a/src/settings/SettingsTab.ts +++ b/src/settings/SettingsTab.ts @@ -268,6 +268,19 @@ export class LLMSettingTab extends PluginSettingTab { await this.plugin.saveSettings(); }); }); + + // Thinking mode setting (only shown when ACP is enabled) + new Setting(settingsContainer) + .setName("Thinking Mode (ACP)") + .setDesc('Extended thinking level. Common values: "none", "low", "medium", "high". Leave empty for agent default. Only applies with ACP.') + .addText((text) => { + text.setPlaceholder("Agent default"); + text.setValue(providerConfig.thinkingMode ?? ""); + text.onChange(async (value) => { + this.plugin.settings.providers[provider].thinkingMode = value.trim() || undefined; + await this.plugin.saveSettings(); + }); + }); } // Timeout override (optional) diff --git a/src/types.ts b/src/types.ts index 1d50718..4171296 100644 --- a/src/types.ts +++ b/src/types.ts @@ -23,6 +23,8 @@ export interface ProviderConfig { yoloMode?: boolean; /** Use ACP (Agent Client Protocol) for persistent connection (supported: claude, opencode, gemini) */ useAcp?: boolean; + /** Thinking mode level for ACP (e.g., "none", "low", "medium", "high") - agent-specific */ + thinkingMode?: string; } /** From e72d0019c55e5a8828d3dd2af9b3f56cc411d733 Mon Sep 17 00:00:00 2001 From: Brian Thorne Date: Mon, 26 Jan 2026 22:53:34 +1300 Subject: [PATCH 07/13] Add dynamic model display from ACP session - Track model state from ACP session response (SessionModelState) - Add getCurrentModel() and getAvailableModels() methods to AcpExecutor - Update status bar with actual model name after ACP connection - Status bar now shows the model the agent is actually using, not just configured --- main.ts | 10 ++++-- src/executor/AcpExecutor.ts | 68 +++++++++++++++++++++++++++++++++++++ src/views/ChatView.ts | 6 ++++ 3 files changed, 81 insertions(+), 3 deletions(-) diff --git a/main.ts b/main.ts index 6bb9462..7400007 100644 --- a/main.ts +++ b/main.ts @@ -258,8 +258,9 @@ export default class LLMPlugin extends Plugin { /** * Update the status bar with current provider and model info * @param provider Optional provider to display (uses default if not specified) + * @param actualModelName Optional actual model name from ACP session (overrides configured model display) */ - updateStatusBar(provider?: LLMProvider) { + updateStatusBar(provider?: LLMProvider, actualModelName?: string) { if (!this.statusBarEl) return; const displayProvider = provider ?? this.settings.defaultProvider; @@ -278,8 +279,11 @@ export default class LLMPlugin extends Plugin { // Build status text with provider and model let statusText = providerNames[displayProvider] || displayProvider; - if (providerConfig?.model) { - // Show abbreviated model name + if (actualModelName) { + // Use actual model name from ACP session + statusText += ` (${this.formatModelName(actualModelName)})`; + } else if (providerConfig?.model) { + // Show configured model name statusText += ` (${this.formatModelName(providerConfig.model)})`; } else { // Indicate CLI default is being used diff --git a/src/executor/AcpExecutor.ts b/src/executor/AcpExecutor.ts index 3cbd9e5..f728a80 100644 --- a/src/executor/AcpExecutor.ts +++ b/src/executor/AcpExecutor.ts @@ -18,6 +18,8 @@ import { type ContentChunk, type ToolCall, type SessionConfigOption, + type SessionModelState, + type ModelInfo, } from "@agentclientprotocol/sdk"; import type { LLMPluginSettings, LLMProvider, ProgressEvent } from "../types"; @@ -26,6 +28,12 @@ export interface ThinkingOption { name: string; } +export interface CurrentModelInfo { + id: string; + name: string; + description?: string; +} + // Convert Node streams to Web streams function nodeToWebReadable(nodeStream: NodeJS.ReadableStream): ReadableStream { return new ReadableStream({ @@ -89,6 +97,7 @@ export class AcpExecutor { private debug: (...args: unknown[]) => void; private progressCallback: ((event: ProgressEvent) => void) | null = null; private configOptions: SessionConfigOption[] = []; + private modelState: SessionModelState | null = null; constructor(settings: LLMPluginSettings) { this.settings = settings; @@ -236,6 +245,13 @@ export class AcpExecutor { this.configOptions = sessionResponse.configOptions ?? []; this.debug("Config options available:", this.configOptions.map((o) => o.id)); + // Store model state from session response + this.modelState = sessionResponse.models ?? null; + if (this.modelState) { + this.debug("Current model:", this.modelState.currentModelId); + this.debug("Available models:", this.modelState.availableModels.map((m) => m.modelId)); + } + // Set model if configured const providerConfig = this.settings.providers[provider]; if (providerConfig.model) { @@ -246,6 +262,14 @@ export class AcpExecutor { modelId: providerConfig.model, }); this.debug("Model set successfully"); + // Update local model state to reflect the change + if (this.modelState) { + this.modelState = { + ...this.modelState, + currentModelId: providerConfig.model, + }; + this.debug("Updated model state, current:", this.modelState.currentModelId); + } } catch (err) { // Model selection is experimental - log but don't fail this.debug("Failed to set model (may not be supported):", err); @@ -478,6 +502,7 @@ export class AcpExecutor { this.currentProvider = null; this.progressCallback = null; this.configOptions = []; + this.modelState = null; } /** @@ -493,4 +518,47 @@ export class AcpExecutor { getProvider(): LLMProvider | null { return this.currentProvider; } + + /** + * Get current model information + */ + getCurrentModel(): CurrentModelInfo | null { + if (!this.modelState) { + return null; + } + + const currentId = this.modelState.currentModelId; + const modelInfo = this.modelState.availableModels.find( + (m) => m.modelId === currentId + ); + + if (modelInfo) { + return { + id: modelInfo.modelId, + name: modelInfo.name, + description: modelInfo.description ?? undefined, + }; + } + + // Model ID exists but not in available models list - return just the ID + return { + id: currentId, + name: currentId, + }; + } + + /** + * Get list of available models + */ + getAvailableModels(): CurrentModelInfo[] { + if (!this.modelState) { + return []; + } + + return this.modelState.availableModels.map((m) => ({ + id: m.modelId, + name: m.name, + description: m.description ?? undefined, + })); + } } diff --git a/src/views/ChatView.ts b/src/views/ChatView.ts index 0c699cc..02d7f29 100644 --- a/src/views/ChatView.ts +++ b/src/views/ChatView.ts @@ -512,6 +512,12 @@ export class ChatView extends ItemView { if (!this.acpExecutor.isConnected() || this.acpExecutor.getProvider() !== this.currentProvider) { onProgress({ type: "status", message: "Connecting to ACP agent..." }); await this.acpExecutor.connect(this.currentProvider, vaultPath, { onProgress }); + + // Update status bar with actual model from ACP session + const currentModel = this.acpExecutor.getCurrentModel(); + if (currentModel) { + this.plugin.updateStatusBar(this.currentProvider, currentModel.name); + } } const acpResponse = await this.acpExecutor.prompt(contextPrompt, { onProgress }); From 92fd555da857ae83067e3267812ef6bde2c72652 Mon Sep 17 00:00:00 2001 From: Brian Thorne Date: Mon, 26 Jan 2026 23:04:08 +1300 Subject: [PATCH 08/13] Add tests for thinking mode, model fetcher, and ACP status bar New tests: - should persist thinking mode setting - should update status bar with actual model from ACP (@acp-status) - should have PROVIDER_MODELS defined for all providers - should allow custom model input - should accept provider/model format for OpenCode Fix test isolation: - Reset default provider to Claude in Status Bar Tests before() hook - Prevents ACP tests from affecting subsequent status bar tests --- test/specs/providers.e2e.ts | 179 ++++++++++++++++++++++++++++++++++++ 1 file changed, 179 insertions(+) diff --git a/test/specs/providers.e2e.ts b/test/specs/providers.e2e.ts index 7facf15..5292fd8 100644 --- a/test/specs/providers.e2e.ts +++ b/test/specs/providers.e2e.ts @@ -485,6 +485,16 @@ describe("Provider Tests @provider", () => { }); await browser.pause(200); + // Reset default provider to Claude and ensure it's configured + await browser.execute(() => { + const plugin = (window as any).app?.plugins?.plugins?.["obsidian-llm"]; + if (plugin?.settings) { + plugin.settings.defaultProvider = "claude"; + plugin.saveSettings(); + } + }); + await browser.pause(200); + // Ensure Claude and Gemini are enabled with models for testing await setProviderModel("claude", FAST_MODELS.claude); await setProviderModel("gemini", FAST_MODELS.gemini); @@ -1274,4 +1284,173 @@ describe("ACP Mode Tests @acp @provider", () => { // Clean up await disableAcpMode(provider); }); + + it("should persist thinking mode setting", async () => { + // Set thinking mode for a provider + await browser.execute(() => { + const plugin = (window as any).app?.plugins?.plugins?.["obsidian-llm"]; + if (plugin?.settings?.providers?.opencode) { + plugin.settings.providers.opencode.thinkingMode = "high"; + plugin.saveSettings(); + } + }); + await browser.pause(200); + + // Verify setting was saved + const thinkingMode = await browser.execute(() => { + const plugin = (window as any).app?.plugins?.plugins?.["obsidian-llm"]; + return plugin?.settings?.providers?.opencode?.thinkingMode; + }); + + expect(thinkingMode).toBe("high"); + + // Clean up + await browser.execute(() => { + const plugin = (window as any).app?.plugins?.plugins?.["obsidian-llm"]; + if (plugin?.settings?.providers?.opencode) { + plugin.settings.providers.opencode.thinkingMode = undefined; + plugin.saveSettings(); + } + }); + }); + + it("should update status bar with actual model from ACP @slow @acp-status", async () => { + // Enable ACP for OpenCode + await browser.execute(() => { + const plugin = (window as any).app?.plugins?.plugins?.["obsidian-llm"]; + if (plugin?.settings?.providers?.opencode) { + plugin.settings.providers.opencode.enabled = true; + plugin.settings.providers.opencode.useAcp = true; + plugin.settings.defaultProvider = "opencode"; + plugin.saveSettings(); + } + }); + await browser.pause(200); + + // Open chat and send a message to trigger ACP connection + await browser.executeObsidianCommand("obsidian-llm:open-llm-chat"); + await browser.pause(500); + + const input = await browser.$(".llm-chat-input"); + await input.click(); + await input.setValue("Say 'test' and nothing else."); + + const sendBtn = await browser.$(".llm-chat-send"); + await sendBtn.click(); + + // Wait for response (ACP connection happens here) + await browser.waitUntil( + async () => { + const response = await browser.$(".llm-message-assistant"); + return response.isExisting(); + }, + { timeout: 60000, timeoutMsg: "No response from ACP agent" } + ); + + // Check status bar - should show the actual model name from ACP session + const statusText = await getStatusBarText(); + console.log("Status bar after ACP connection:", statusText); + + // Status bar should contain provider name and some model info + expect(statusText).toContain("LLM:"); + expect(statusText).toContain("OpenCode"); + + // Clean up + await browser.execute(() => { + const plugin = (window as any).app?.plugins?.plugins?.["obsidian-llm"]; + if (plugin?.settings?.providers?.opencode) { + plugin.settings.providers.opencode.useAcp = false; + plugin.saveSettings(); + } + }); + }); +}); + +/** + * Model Fetcher Tests + * Tests for dynamic model fetching functionality + */ +describe("Model Fetcher Tests @models @provider", () => { + it("should have PROVIDER_MODELS defined for all providers", async () => { + const hasModels = await browser.execute(() => { + // Check if PROVIDER_MODELS exists and has entries for each provider + // This is testing the static fallback models are defined + const plugin = (window as any).app?.plugins?.plugins?.["obsidian-llm"]; + if (!plugin) return false; + + // The plugin should have settings with providers + const providers = ["claude", "opencode", "codex", "gemini"]; + for (const p of providers) { + if (!plugin.settings?.providers?.[p]) { + return false; + } + } + return true; + }); + + expect(hasModels).toBe(true); + }); + + it("should allow custom model input", async () => { + // Set a custom model that's not in the predefined list + const customModel = "my-custom-model-id"; + + await browser.execute((model) => { + const plugin = (window as any).app?.plugins?.plugins?.["obsidian-llm"]; + if (plugin?.settings?.providers?.claude) { + plugin.settings.providers.claude.model = model; + plugin.saveSettings(); + } + }, customModel); + await browser.pause(200); + + // Verify custom model was saved + const savedModel = await browser.execute(() => { + const plugin = (window as any).app?.plugins?.plugins?.["obsidian-llm"]; + return plugin?.settings?.providers?.claude?.model; + }); + + expect(savedModel).toBe(customModel); + + // Clean up - reset to empty (default) + await browser.execute(() => { + const plugin = (window as any).app?.plugins?.plugins?.["obsidian-llm"]; + if (plugin?.settings?.providers?.claude) { + plugin.settings.providers.claude.model = ""; + plugin.saveSettings(); + } + }); + }); + + it("should accept provider/model format for OpenCode", async () => { + // OpenCode uses provider/model format like "anthropic/claude-sonnet-4-5" + const openCodeModel = "anthropic/claude-sonnet-4-5"; + + await browser.execute((model) => { + const plugin = (window as any).app?.plugins?.plugins?.["obsidian-llm"]; + if (plugin?.settings?.providers?.opencode) { + plugin.settings.providers.opencode.enabled = true; + plugin.settings.providers.opencode.model = model; + plugin.saveSettings(); + } + }, openCodeModel); + await browser.pause(200); + + // Verify model with slash was saved correctly + const savedModel = await browser.execute(() => { + const plugin = (window as any).app?.plugins?.plugins?.["obsidian-llm"]; + return plugin?.settings?.providers?.opencode?.model; + }); + + expect(savedModel).toBe(openCodeModel); + + // Clean up + await browser.execute(() => { + const plugin = (window as any).app?.plugins?.plugins?.["obsidian-llm"]; + if (plugin?.settings?.providers?.opencode) { + plugin.settings.providers.opencode.model = ""; + plugin.saveSettings(); + } + }); + }); }); From 31c2a7855d0fb815ce6ce0e95a8f6731c3e1cf07 Mon Sep 17 00:00:00 2001 From: Brian Thorne Date: Mon, 26 Jan 2026 23:15:56 +1300 Subject: [PATCH 09/13] Add eager ACP connection and improve error handling Eager connection: - Connect to ACP when chat view opens (not on first message) - Connect when provider changes in dropdown - Update status bar immediately with model info Error handling improvements: - Track stream closed state to prevent EPIPE errors - Check if process is still running in isConnected() - Race initialization against process exit for better error messages - Collect stderr output for error diagnostics - Clean up connection state when process exits unexpectedly --- src/executor/AcpExecutor.ts | 122 +++++++++++++++++++++++++++++------- src/views/ChatView.ts | 52 +++++++++++++++ 2 files changed, 150 insertions(+), 24 deletions(-) diff --git a/src/executor/AcpExecutor.ts b/src/executor/AcpExecutor.ts index f728a80..058221b 100644 --- a/src/executor/AcpExecutor.ts +++ b/src/executor/AcpExecutor.ts @@ -58,24 +58,53 @@ function nodeToWebReadable(nodeStream: NodeJS.ReadableStream): ReadableStream { + let streamClosed = false; + + // Track if stream closes + nodeStream.on("close", () => { + streamClosed = true; + }); + nodeStream.on("error", () => { + streamClosed = true; + }); + return new WritableStream({ write(chunk) { return new Promise((resolve, reject) => { - const ok = nodeStream.write(chunk, (err) => { - if (err) reject(err); - else resolve(); - }); - if (!ok) { - nodeStream.once("drain", resolve); + if (streamClosed) { + reject(new Error("Stream is closed")); + return; + } + + try { + const ok = nodeStream.write(chunk, (err) => { + if (err) { + streamClosed = true; + reject(err); + } else { + resolve(); + } + }); + if (!ok) { + nodeStream.once("drain", resolve); + } + } catch (err) { + streamClosed = true; + reject(err); } }); }, close() { return new Promise((resolve) => { + if (streamClosed) { + resolve(); + return; + } nodeStream.end(resolve); }); }, abort(err) { + streamClosed = true; if ("destroy" in nodeStream && typeof nodeStream.destroy === "function") { (nodeStream as NodeJS.WritableStream & { destroy: (err?: Error) => void }).destroy(err); } @@ -168,13 +197,25 @@ export class AcpExecutor { throw new Error("Failed to create stdio streams for ACP agent"); } - // Log stderr for debugging + // Collect stderr for error messages + let stderrOutput = ""; this.process.stderr?.on("data", (data: Buffer) => { - this.debug("Agent stderr:", data.toString()); + const text = data.toString(); + stderrOutput += text; + this.debug("Agent stderr:", text); + }); + + // Create a promise that rejects if the process exits during initialization + let processExitReject: ((err: Error) => void) | null = null; + const processExitPromise = new Promise((_, reject) => { + processExitReject = reject; }); this.process.on("error", (err) => { this.debug("Agent process error:", err); + if (processExitReject) { + processExitReject(new Error(`ACP process error: ${err.message}`)); + } }); this.process.on("exit", (code, signal) => { @@ -182,6 +223,12 @@ export class AcpExecutor { this.connection = null; this.process = null; this.sessionId = null; + this.configOptions = []; + this.modelState = null; + if (processExitReject) { + const reason = stderrOutput.trim() || `exit code ${code}${signal ? `, signal ${signal}` : ""}`; + processExitReject(new Error(`ACP process exited: ${reason}`)); + } }); // Create the ACP stream from stdio @@ -218,29 +265,39 @@ export class AcpExecutor { this.connection = new ClientSideConnection(createClient, stream); this.currentProvider = provider; - // Initialize the connection + // Initialize the connection - race against process exit this.debug("Initializing ACP connection..."); - const initResponse = await this.connection.initialize({ - protocolVersion: 1, - clientInfo: { - name: "obsidian-llm-plugin", - version: "1.0.0", - }, - clientCapabilities: {}, - }); + const initResponse = await Promise.race([ + this.connection.initialize({ + protocolVersion: 1, + clientInfo: { + name: "obsidian-llm-plugin", + version: "1.0.0", + }, + clientCapabilities: {}, + }), + processExitPromise, + ]); this.debug("ACP initialized:", initResponse); - // Create a new session + // Create a new session - race against process exit this.debug("Creating new session..."); - const sessionResponse = await this.connection.newSession({ - cwd, - mcpServers: [], - }); + const sessionResponse = await Promise.race([ + this.connection.newSession({ + cwd, + mcpServers: [], + }), + processExitPromise, + ]); this.sessionId = sessionResponse.sessionId; this.debug("Session created:", this.sessionId); + // Clear the exit rejection now that we're successfully connected + // This prevents the rejection from being triggered on normal shutdown + processExitReject = null; + // Store config options from session response this.configOptions = sessionResponse.configOptions ?? []; this.debug("Config options available:", this.configOptions.map((o) => o.id)); @@ -506,10 +563,27 @@ export class AcpExecutor { } /** - * Check if connected + * Check if connected and the process is still running */ isConnected(): boolean { - return this.connection !== null && this.sessionId !== null; + // Check if we have a connection and session + if (!this.connection || !this.sessionId) { + return false; + } + + // Check if the process is still running + if (this.process && this.process.exitCode !== null) { + // Process has exited - clean up + this.debug("Process has exited, cleaning up connection state"); + this.connection = null; + this.sessionId = null; + this.process = null; + this.configOptions = []; + this.modelState = null; + return false; + } + + return true; } /** diff --git a/src/views/ChatView.ts b/src/views/ChatView.ts index 02d7f29..2f3a61b 100644 --- a/src/views/ChatView.ts +++ b/src/views/ChatView.ts @@ -74,6 +74,9 @@ export class ChatView extends ItemView { // Focus the input setTimeout(() => this.inputEl?.focus(), 50); + + // Eagerly connect to ACP if enabled for the current provider + this.connectAcpIfEnabled(); } async onClose() { @@ -108,6 +111,8 @@ export class ChatView extends ItemView { dropdown.onChange((value) => { this.currentProvider = value as LLMProvider; this.plugin.updateStatusBar(this.currentProvider); + // Eagerly connect to ACP when provider changes + this.connectAcpIfEnabled(); }); // Update status bar to show initial provider @@ -458,6 +463,53 @@ export class ChatView extends ItemView { } } + /** + * Eagerly connect to ACP if enabled for the current provider. + * This is called when the view opens and when the provider changes. + */ + private async connectAcpIfEnabled(): Promise { + const providerConfig = this.plugin.settings.providers[this.currentProvider]; + const useAcp = providerConfig.useAcp && ACP_SUPPORTED_PROVIDERS.includes(this.currentProvider); + + if (!useAcp) { + return; + } + + // Don't reconnect if already connected to this provider + if (this.acpExecutor.isConnected() && this.acpExecutor.getProvider() === this.currentProvider) { + // Already connected - just update the status bar with model info + const currentModel = this.acpExecutor.getCurrentModel(); + if (currentModel) { + this.plugin.updateStatusBar(this.currentProvider, currentModel.name); + } + return; + } + + // Get vault path for working directory + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const vaultPath = (this.app.vault.adapter as any).basePath as string | undefined; + + try { + // Show connecting status + this.handleProgressEvent({ type: "status", message: "Connecting to ACP agent..." }); + + await this.acpExecutor.connect(this.currentProvider, vaultPath); + + // Update status bar with actual model from ACP session + const currentModel = this.acpExecutor.getCurrentModel(); + if (currentModel) { + this.plugin.updateStatusBar(this.currentProvider, currentModel.name); + } + + // Clear the connecting status + this.clearProgress(); + } catch (err) { + // Connection failed - will retry when message is sent + console.warn("ACP eager connection failed:", err); + this.clearProgress(); + } + } + private async sendMessage() { if (!this.inputEl || this.isLoading) return; From 05a79a0061f4eec47eb415e4c0135d09594b4a20 Mon Sep 17 00:00:00 2001 From: Brian Thorne Date: Tue, 27 Jan 2026 07:47:38 +1300 Subject: [PATCH 10/13] Fix ACP content accumulation and improve tool call handling Key fixes: - Fix critical bug where content wasn't accumulated when progressCallback was null - Add shell: true to spawn options for npx PATH resolution - Add initialization timeout and race condition handling for process exits - Extract file paths from tool_call locations field - Handle tool_call_update events for status tracking - Make content extraction more robust for different provider formats Improvements: - Status bar shows "connecting" state with pulsing indicator - Error messages are now selectable text - Model name formatting handles ACP display names - QuickPromptModal messages sync to ChatView - E2E tests wait for ACP connection to complete Known issue: OpenCode ACP with model selection may not return content (Claude and Gemini ACP work correctly with model selection) Co-Authored-By: Claude Opus 4.5 --- main.ts | 65 +++++++++++-- src/executor/AcpExecutor.ts | 162 +++++++++++++++++++++++++++------ src/modals/QuickPromptModal.ts | 6 ++ src/views/ChatView.ts | 125 ++++++++++++++++++------- styles.css | 13 +++ test/specs/providers.e2e.ts | 68 ++++++++++++-- 6 files changed, 367 insertions(+), 72 deletions(-) diff --git a/main.ts b/main.ts index 7400007..06b981a 100644 --- a/main.ts +++ b/main.ts @@ -197,6 +197,29 @@ export default class LLMPlugin extends Plugin { if (leaf) { workspace.revealLeaf(leaf); } + + return leaf; + } + + /** + * Get the ChatView instance if it exists + */ + getChatView(): ChatView | null { + const leaves = this.app.workspace.getLeavesOfType(CHAT_VIEW_TYPE); + if (leaves.length > 0) { + return leaves[0].view as ChatView; + } + return null; + } + + /** + * Add a message exchange to the chat view + */ + addToChatView(userMessage: string, assistantMessage: string, provider: LLMProvider) { + const chatView = this.getChatView(); + if (chatView) { + chatView.addMessageExchange(userMessage, assistantMessage, provider); + } } async loadSettings() { @@ -259,8 +282,9 @@ export default class LLMPlugin extends Plugin { * Update the status bar with current provider and model info * @param provider Optional provider to display (uses default if not specified) * @param actualModelName Optional actual model name from ACP session (overrides configured model display) + * @param status Optional status: "idle" (default), "connecting", "connected" */ - updateStatusBar(provider?: LLMProvider, actualModelName?: string) { + updateStatusBar(provider?: LLMProvider, actualModelName?: string, status?: "idle" | "connecting" | "connected") { if (!this.statusBarEl) return; const displayProvider = provider ?? this.settings.defaultProvider; @@ -279,7 +303,9 @@ export default class LLMPlugin extends Plugin { // Build status text with provider and model let statusText = providerNames[displayProvider] || displayProvider; - if (actualModelName) { + if (status === "connecting") { + statusText += " (connecting...)"; + } else if (actualModelName) { // Use actual model name from ACP session statusText += ` (${this.formatModelName(actualModelName)})`; } else if (providerConfig?.model) { @@ -295,8 +321,10 @@ export default class LLMPlugin extends Plugin { cls: "llm-status-text", }); - // Check if provider is enabled - if (providerConfig?.enabled) { + // Set indicator state based on status + if (status === "connecting") { + indicator.addClass("connecting"); + } else if (providerConfig?.enabled) { indicator.addClass("active"); } } @@ -305,7 +333,7 @@ export default class LLMPlugin extends Plugin { * Format model name for display (abbreviate long names) */ private formatModelName(model: string): string { - // Common abbreviations + // Common abbreviations for model IDs const abbreviations: Record = { "claude-3-5-haiku-latest": "haiku", "claude-3-5-sonnet-latest": "sonnet-3.5", @@ -323,8 +351,33 @@ export default class LLMPlugin extends Plugin { "gpt-5": "5", "claude-sonnet": "sonnet", "claude-haiku": "haiku", + // ACP display names (from Claude ACP adapter) + "default": "opus", + "Default (recommended)": "opus", + "Sonnet": "sonnet", + "Haiku": "haiku", }; - return abbreviations[model] || model; + // Check for exact match first + if (abbreviations[model]) { + return abbreviations[model]; + } + + // Try case-insensitive match + const lowerModel = model.toLowerCase(); + for (const [key, value] of Object.entries(abbreviations)) { + if (key.toLowerCase() === lowerModel) { + return value; + } + } + + // If model name is long, try to extract a shorter name + // Remove text in parentheses and trim + const simplified = model.replace(/\s*\([^)]*\)\s*/g, "").trim(); + if (simplified !== model && simplified.length > 0) { + return this.formatModelName(simplified); + } + + return model; } } diff --git a/src/executor/AcpExecutor.ts b/src/executor/AcpExecutor.ts index 058221b..a6cffde 100644 --- a/src/executor/AcpExecutor.ts +++ b/src/executor/AcpExecutor.ts @@ -127,6 +127,7 @@ export class AcpExecutor { private progressCallback: ((event: ProgressEvent) => void) | null = null; private configOptions: SessionConfigOption[] = []; private modelState: SessionModelState | null = null; + private accumulatedContent: string = ""; // Accumulate text content during prompt constructor(settings: LLMPluginSettings) { this.settings = settings; @@ -187,10 +188,12 @@ export class AcpExecutor { this.debug("Spawning ACP agent:", acpCommand.cmd, acpCommand.args, "cwd:", cwd); // Spawn the ACP agent process + // Use shell: true to ensure commands like npx are found via shell PATH this.process = spawn(acpCommand.cmd, acpCommand.args, { cwd, stdio: ["pipe", "pipe", "pipe"], env: { ...process.env }, + shell: true, }); if (!this.process.stdin || !this.process.stdout) { @@ -205,12 +208,22 @@ export class AcpExecutor { this.debug("Agent stderr:", text); }); + // Track initialization state to handle process exits appropriately + let initializationComplete = false; + // Create a promise that rejects if the process exits during initialization let processExitReject: ((err: Error) => void) | null = null; const processExitPromise = new Promise((_, reject) => { processExitReject = reject; }); + // Create a timeout promise for slow-starting processes (like npx) + const timeoutPromise = new Promise((_, reject) => { + setTimeout(() => { + reject(new Error("ACP connection timeout - agent took too long to respond")); + }, 30000); // 30 second timeout for initialization + }); + this.process.on("error", (err) => { this.debug("Agent process error:", err); if (processExitReject) { @@ -219,16 +232,22 @@ export class AcpExecutor { }); this.process.on("exit", (code, signal) => { - this.debug("Agent process exited:", code, signal); - this.connection = null; - this.process = null; - this.sessionId = null; - this.configOptions = []; - this.modelState = null; - if (processExitReject) { - const reason = stderrOutput.trim() || `exit code ${code}${signal ? `, signal ${signal}` : ""}`; - processExitReject(new Error(`ACP process exited: ${reason}`)); + this.debug("Agent process exited:", code, signal, "initComplete:", initializationComplete); + + // Only clear state during initialization phase + // After initialization, let isConnected() detect exit via exitCode + if (!initializationComplete) { + this.connection = null; + this.process = null; + this.sessionId = null; + this.configOptions = []; + this.modelState = null; + if (processExitReject) { + const reason = stderrOutput.trim() || `exit code ${code}${signal ? `, signal ${signal}` : ""}`; + processExitReject(new Error(`ACP process exited: ${reason}`)); + } } + // After initialization, keep state intact so isConnected() can use exitCode }); // Create the ACP stream from stdio @@ -265,7 +284,7 @@ export class AcpExecutor { this.connection = new ClientSideConnection(createClient, stream); this.currentProvider = provider; - // Initialize the connection - race against process exit + // Initialize the connection - race against process exit and timeout this.debug("Initializing ACP connection..."); const initResponse = await Promise.race([ this.connection.initialize({ @@ -277,11 +296,12 @@ export class AcpExecutor { clientCapabilities: {}, }), processExitPromise, + timeoutPromise, ]); this.debug("ACP initialized:", initResponse); - // Create a new session - race against process exit + // Create a new session - race against process exit and timeout this.debug("Creating new session..."); const sessionResponse = await Promise.race([ this.connection.newSession({ @@ -289,11 +309,15 @@ export class AcpExecutor { mcpServers: [], }), processExitPromise, + timeoutPromise, ]); this.sessionId = sessionResponse.sessionId; this.debug("Session created:", this.sessionId); + // Mark initialization as complete - after this, exit handler won't clear state + initializationComplete = true; + // Clear the exit rejection now that we're successfully connected // This prevents the rejection from being triggered on normal shutdown processExitReject = null; @@ -343,24 +367,58 @@ export class AcpExecutor { * Handle session update notifications and convert to ProgressEvents */ private handleSessionUpdate(update: SessionUpdate) { - if (!this.progressCallback) return; + this.debug("handleSessionUpdate:", update.sessionUpdate, JSON.stringify(update).slice(0, 200)); switch (update.sessionUpdate) { case "agent_message_chunk": case "user_message_chunk": { // ContentChunk has a single content block, not an array - const chunk = update as ContentChunk & { sessionUpdate: string }; - if (chunk.content && chunk.content.type === "text") { - const textContent = chunk.content as { type: "text"; text: string }; - this.progressCallback({ + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const chunk = update as any; + this.debug("Content chunk:", JSON.stringify(chunk)); + + // Try to extract text content - different providers may have different structures + let textToAdd = ""; + + // Standard ACP format: chunk.content.type === "text" with chunk.content.text + if (chunk.content && chunk.content.type === "text" && chunk.content.text) { + textToAdd = chunk.content.text; + } + // Alternative format: chunk.content is the text directly + else if (chunk.content && typeof chunk.content === "string") { + textToAdd = chunk.content; + } + // Alternative format: chunk.text directly + else if (chunk.text && typeof chunk.text === "string") { + textToAdd = chunk.text; + } + // Alternative format: content array (like in some ACP implementations) + else if (Array.isArray(chunk.content)) { + for (const item of chunk.content) { + if (item && item.type === "text" && item.text) { + textToAdd += item.text; + } + } + } + + if (textToAdd) { + // Accumulate text content for the response (always, even without callback) + this.accumulatedContent += textToAdd; + this.debug("Accumulated content length:", this.accumulatedContent.length); + + // Notify progress callback if available + this.progressCallback?.({ type: "text", - content: textContent.text, + content: this.accumulatedContent, // Send cumulative content like CLI streaming }); + } else { + this.debug("No text extracted from chunk"); } break; } case "agent_thought_chunk": { + if (!this.progressCallback) break; const chunk = update as ContentChunk & { sessionUpdate: string }; if (chunk.content && chunk.content.type === "text") { const textContent = chunk.content as { type: "text"; text: string }; @@ -373,16 +431,63 @@ export class AcpExecutor { } case "tool_call": { - // ToolCall has title and toolCallId, not name/arguments + if (!this.progressCallback) break; + // ToolCall has title, status, locations (file paths), and more const toolCall = update as ToolCall & { sessionUpdate: string }; + + // Extract file path from locations if available (useful for file operations) + let input: string | undefined; + if (toolCall.locations && toolCall.locations.length > 0) { + const loc = toolCall.locations[0]; + input = loc.line ? `${loc.path}:${loc.line}` : loc.path; + } + + // Map ACP status to our status type + let status: "started" | "completed" | undefined; + if (toolCall.status === "pending" || toolCall.status === "in_progress") { + status = "started"; + } else if (toolCall.status === "completed" || toolCall.status === "failed") { + status = "completed"; + } + this.progressCallback({ type: "tool_use", tool: toolCall.title ?? "unknown", - input: toolCall.toolCallId, + input, + status, }); break; } + case "tool_call_update": { + if (!this.progressCallback) break; + // Handle tool call status updates + const toolUpdate = update as { toolCallId: string; status?: string; locations?: Array<{ path: string; line?: number | null }> }; + + // Extract file path from locations if available + let input: string | undefined; + if (toolUpdate.locations && toolUpdate.locations.length > 0) { + const loc = toolUpdate.locations[0]; + input = loc.line ? `${loc.path}:${loc.line}` : loc.path; + } + + // Map ACP status to our status type + let status: "started" | "completed" | undefined; + if (toolUpdate.status === "completed" || toolUpdate.status === "failed") { + status = "completed"; + } + + // Only emit if we have useful info to show + if (status === "completed") { + this.progressCallback({ + type: "tool_use", + tool: input ?? toolUpdate.toolCallId, + status, + }); + } + break; + } + default: this.debug("Unhandled session update type:", update.sessionUpdate); } @@ -395,10 +500,14 @@ export class AcpExecutor { message: string, options?: AcpExecutorOptions ): Promise<{ content: string; error?: string }> { - if (!this.connection || !this.sessionId) { + // Use isConnected() which also checks if the process is still running + if (!this.isConnected()) { throw new Error("Not connected to an ACP agent. Call connect() first."); } + // Reset accumulated content for this prompt + this.accumulatedContent = ""; + // Update progress callback if provided if (options?.onProgress) { this.progressCallback = options.onProgress; @@ -407,20 +516,21 @@ export class AcpExecutor { this.debug("Sending prompt:", message.slice(0, 100)); try { - const response = await this.connection.prompt({ - sessionId: this.sessionId, + // Non-null assertions are safe here because isConnected() returned true + const response = await this.connection!.prompt({ + sessionId: this.sessionId!, prompt: [{ type: "text", text: message }], }); this.debug("Prompt response:", response); + this.debug("Accumulated content length:", this.accumulatedContent.length); - // The response indicates completion - actual content comes via sessionUpdate - // Return empty content for now (content was streamed via callbacks) - return { content: "" }; + // Return accumulated content from sessionUpdate callbacks + return { content: this.accumulatedContent }; } catch (err) { const error = err instanceof Error ? err.message : String(err); this.debug("Prompt error:", error); - return { content: "", error }; + return { content: this.accumulatedContent, error }; } } diff --git a/src/modals/QuickPromptModal.ts b/src/modals/QuickPromptModal.ts index c14feda..10d4aa1 100644 --- a/src/modals/QuickPromptModal.ts +++ b/src/modals/QuickPromptModal.ts @@ -186,6 +186,9 @@ export class QuickPromptModal extends Modal { this.setLoading(true); + // Store original user prompt (without prefix/system prompt) for chat history + const originalPrompt = this.inputEl.value.trim(); + try { const response = await this.executor.execute(prompt, this.currentProvider); @@ -194,6 +197,9 @@ export class QuickPromptModal extends Modal { } else { this.lastResponse = response.content; this.showResponse(response.content, false); + + // Add to chat view if it exists + this.plugin.addToChatView(originalPrompt, response.content, this.currentProvider); } } catch (error) { this.showResponse( diff --git a/src/views/ChatView.ts b/src/views/ChatView.ts index 2f3a61b..3a994c3 100644 --- a/src/views/ChatView.ts +++ b/src/views/ChatView.ts @@ -466,12 +466,15 @@ export class ChatView extends ItemView { /** * Eagerly connect to ACP if enabled for the current provider. * This is called when the view opens and when the provider changes. + * Blocks user input while connecting. */ private async connectAcpIfEnabled(): Promise { const providerConfig = this.plugin.settings.providers[this.currentProvider]; const useAcp = providerConfig.useAcp && ACP_SUPPORTED_PROVIDERS.includes(this.currentProvider); if (!useAcp) { + // Not using ACP - make sure status bar shows configured model (not stale ACP model) + this.plugin.updateStatusBar(this.currentProvider); return; } @@ -489,24 +492,46 @@ export class ChatView extends ItemView { // eslint-disable-next-line @typescript-eslint/no-explicit-any const vaultPath = (this.app.vault.adapter as any).basePath as string | undefined; - try { - // Show connecting status - this.handleProgressEvent({ type: "status", message: "Connecting to ACP agent..." }); + // Block user input while connecting + this.setLoading(true); + this.plugin.updateStatusBar(this.currentProvider, undefined, "connecting"); + this.handleProgressEvent({ type: "status", message: `Connecting to ${this.currentProvider} ACP...` }); + try { await this.acpExecutor.connect(this.currentProvider, vaultPath); + // Verify connection succeeded + if (!this.acpExecutor.isConnected()) { + throw new Error("Connection completed but agent is not responding"); + } + // Update status bar with actual model from ACP session const currentModel = this.acpExecutor.getCurrentModel(); if (currentModel) { - this.plugin.updateStatusBar(this.currentProvider, currentModel.name); + this.plugin.updateStatusBar(this.currentProvider, currentModel.name, "connected"); + } else { + this.plugin.updateStatusBar(this.currentProvider, undefined, "connected"); } // Clear the connecting status this.clearProgress(); } catch (err) { - // Connection failed - will retry when message is sent - console.warn("ACP eager connection failed:", err); + // Connection failed + const errorMsg = err instanceof Error ? err.message : String(err); + console.error("ACP connection failed:", errorMsg); + + // Ensure we disconnect to clean up any partial state + await this.acpExecutor.disconnect(); + + // Reset status bar to idle state + this.plugin.updateStatusBar(this.currentProvider, undefined, "idle"); + + // Show error notification + new Notice(`ACP connection failed: ${errorMsg.slice(0, 100)}`, 5000); + this.clearProgress(); + } finally { + this.setLoading(false); } } @@ -556,37 +581,43 @@ export class ChatView extends ItemView { const providerConfig = this.plugin.settings.providers[this.currentProvider]; const useAcp = providerConfig.useAcp && ACP_SUPPORTED_PROVIDERS.includes(this.currentProvider); - let response; + let response: { content: string; provider: LLMProvider; durationMs: number; error?: string }; + if (useAcp) { // Use ACP executor for persistent connection - try { - // Connect if not already connected or provider changed - if (!this.acpExecutor.isConnected() || this.acpExecutor.getProvider() !== this.currentProvider) { - onProgress({ type: "status", message: "Connecting to ACP agent..." }); - await this.acpExecutor.connect(this.currentProvider, vaultPath, { onProgress }); - - // Update status bar with actual model from ACP session - const currentModel = this.acpExecutor.getCurrentModel(); - if (currentModel) { - this.plugin.updateStatusBar(this.currentProvider, currentModel.name); - } + // Connect if not already connected or provider changed + if (!this.acpExecutor.isConnected() || this.acpExecutor.getProvider() !== this.currentProvider) { + onProgress({ type: "status", message: `Connecting to ${this.currentProvider} ACP...` }); + await this.acpExecutor.connect(this.currentProvider, vaultPath, { onProgress }); + + // Verify connection succeeded (process might have exited) + if (!this.acpExecutor.isConnected()) { + throw new Error("ACP agent process exited unexpectedly"); + } + + // Update status bar with actual model from ACP session + const currentModel = this.acpExecutor.getCurrentModel(); + if (currentModel) { + this.plugin.updateStatusBar(this.currentProvider, currentModel.name); } + } - const acpResponse = await this.acpExecutor.prompt(contextPrompt, { onProgress }); - response = { - content: streamedContent || acpResponse.content, // Prefer streamed content - provider: this.currentProvider, - durationMs: 0, - error: acpResponse.error, - }; - } catch (err) { - response = { - content: "", - provider: this.currentProvider, - durationMs: 0, - error: err instanceof Error ? err.message : String(err), - }; + const acpResponse = await this.acpExecutor.prompt(contextPrompt, { onProgress }); + + // Use ACP accumulated content (streamedContent won't be set for ACP mode) + const content = acpResponse.content; + + // Warn if response is unexpectedly empty + if (!content && !acpResponse.error) { + console.warn("ACP response has no content - this may indicate a problem"); } + + response = { + content, + provider: this.currentProvider, + durationMs: 0, + error: acpResponse.error || (!content ? "No response received from agent" : undefined), + }; } else { // Use regular CLI executor response = await this.executor.execute( @@ -670,7 +701,10 @@ export class ChatView extends ItemView { break; case "text": - // Text events are handled by onStream callback + // Text events contain cumulative content - update streaming display + if (event.content) { + this.updateStreamingMessage(event.content); + } break; } } @@ -1143,4 +1177,29 @@ export class ChatView extends ItemView { new Notice(`Failed to create note: ${error}`); } } + + /** + * Add a message exchange from an external source (e.g., QuickPromptModal) + * This allows other parts of the plugin to add messages to the chat history + */ + async addMessageExchange(userMessage: string, assistantMessage: string, provider: LLMProvider) { + // Add user message + this.messages.push({ + role: "user", + content: userMessage, + timestamp: Date.now(), + provider, + }); + + // Add assistant message + this.messages.push({ + role: "assistant", + content: assistantMessage, + timestamp: Date.now(), + provider, + }); + + // Re-render messages + await this.renderMessagesContent(); + } } diff --git a/styles.css b/styles.css index eb4e6bf..c996da7 100644 --- a/styles.css +++ b/styles.css @@ -249,6 +249,16 @@ background-color: var(--text-success); } +.llm-status-indicator.connecting { + background-color: var(--text-warning); + animation: llm-status-pulse 1s ease-in-out infinite; +} + +@keyframes llm-status-pulse { + 0%, 100% { opacity: 1; } + 50% { opacity: 0.3; } +} + .llm-status-indicator.error { background-color: var(--text-error); } @@ -260,6 +270,9 @@ background-color: rgba(var(--color-red-rgb), 0.1); border-radius: 8px; margin-bottom: 10px; + user-select: text; + -webkit-user-select: text; + cursor: text; } /* Empty state */ diff --git a/test/specs/providers.e2e.ts b/test/specs/providers.e2e.ts index 5292fd8..cd2ea6f 100644 --- a/test/specs/providers.e2e.ts +++ b/test/specs/providers.e2e.ts @@ -1020,11 +1020,20 @@ describe("ACP Mode Tests @acp @provider", () => { // Open chat await browser.executeObsidianCommand("obsidian-llm:open-llm-chat"); - await browser.pause(500); const chatView = await browser.$(".llm-chat-view"); expect(await chatView.isExisting()).toBe(true); + // Wait for ACP connection to complete (input becomes enabled) + await browser.waitUntil( + async () => { + const input = await browser.$(".llm-chat-input"); + const isDisabled = await input.getAttribute("disabled"); + return isDisabled === null; + }, + { timeout: 60000, timeoutMsg: "Chat input did not become enabled (ACP connection may have failed)" } + ); + // Send a simple message const input = await browser.$(".llm-chat-input"); await input.click(); @@ -1078,7 +1087,16 @@ describe("ACP Mode Tests @acp @provider", () => { // Open chat and send a message await browser.executeObsidianCommand("obsidian-llm:open-llm-chat"); - await browser.pause(500); + + // Wait for ACP connection to complete (input becomes enabled) + await browser.waitUntil( + async () => { + const input = await browser.$(".llm-chat-input"); + const isDisabled = await input.getAttribute("disabled"); + return isDisabled === null; + }, + { timeout: 60000, timeoutMsg: "Chat input did not become enabled (ACP connection may have failed)" } + ); const input = await browser.$(".llm-chat-input"); await input.click(); @@ -1132,7 +1150,16 @@ describe("ACP Mode Tests @acp @provider", () => { await browser.pause(200); await browser.executeObsidianCommand("obsidian-llm:open-llm-chat"); - await browser.pause(500); + + // Wait for ACP connection to complete (input becomes enabled) + await browser.waitUntil( + async () => { + const input = await browser.$(".llm-chat-input"); + const isDisabled = await input.getAttribute("disabled"); + return isDisabled === null; + }, + { timeout: 60000, timeoutMsg: "Chat input did not become enabled (ACP connection may have failed)" } + ); const input = await browser.$(".llm-chat-input"); await input.click(); @@ -1181,7 +1208,16 @@ describe("ACP Mode Tests @acp @provider", () => { await browser.pause(200); await browser.executeObsidianCommand("obsidian-llm:open-llm-chat"); - await browser.pause(500); + + // Wait for ACP connection to complete (input becomes enabled) + await browser.waitUntil( + async () => { + const input = await browser.$(".llm-chat-input"); + const isDisabled = await input.getAttribute("disabled"); + return isDisabled === null; + }, + { timeout: 60000, timeoutMsg: "Chat input did not become enabled (ACP connection may have failed)" } + ); const input = await browser.$(".llm-chat-input"); await input.click(); @@ -1225,9 +1261,18 @@ describe("ACP Mode Tests @acp @provider", () => { await enableAcpMode(provider); await browser.executeObsidianCommand("obsidian-llm:open-llm-chat"); - await browser.pause(500); - // First message (includes connection time) + // Wait for ACP connection to complete (input becomes enabled) + await browser.waitUntil( + async () => { + const input = await browser.$(".llm-chat-input"); + const isDisabled = await input.getAttribute("disabled"); + return isDisabled === null; + }, + { timeout: 60000, timeoutMsg: "Chat input did not become enabled (ACP connection may have failed)" } + ); + + // First message (connection already complete) const startTime1 = Date.now(); let input = await browser.$(".llm-chat-input"); @@ -1329,7 +1374,16 @@ describe("ACP Mode Tests @acp @provider", () => { // Open chat and send a message to trigger ACP connection await browser.executeObsidianCommand("obsidian-llm:open-llm-chat"); - await browser.pause(500); + + // Wait for ACP connection to complete (input becomes enabled) + await browser.waitUntil( + async () => { + const input = await browser.$(".llm-chat-input"); + const isDisabled = await input.getAttribute("disabled"); + return isDisabled === null; + }, + { timeout: 60000, timeoutMsg: "Chat input did not become enabled (ACP connection may have failed)" } + ); const input = await browser.$(".llm-chat-input"); await input.click(); From 6e31522ea0cdaf6112de89a2f8557663de6e49ef Mon Sep 17 00:00:00 2001 From: Brian Thorne Date: Tue, 27 Jan 2026 10:56:42 +1300 Subject: [PATCH 11/13] Add Codex ACP support and improve model handling - Add Codex to ACP_SUPPORTED_PROVIDERS using @zed-industries/codex-acp adapter - Cache ACP models dynamically when connecting (preferred over static lists) - Update Claude model list to use aliases (claude-opus-4-5, claude-sonnet-4-5, etc.) - Fix OpenCode ACP tests by clearing model setting and using valid model format - Update README to document ACP as implemented feature - Fetch dynamic models for all ACP-supported providers in settings --- README.md | 35 +++++++++------- package-lock.json | 21 ++++++---- src/executor/AcpExecutor.ts | 81 +++++++++++++++++++++++++++++++------ src/settings/SettingsTab.ts | 55 +++++++++++++++---------- src/types.ts | 11 +++-- src/utils/modelFetcher.ts | 55 ++++++++++++++++++++++++- src/views/ChatView.ts | 54 +++++++++++++++++++------ test/specs/providers.e2e.ts | 8 +++- 8 files changed, 241 insertions(+), 79 deletions(-) diff --git a/README.md b/README.md index b548757..1e16564 100644 --- a/README.md +++ b/README.md @@ -14,6 +14,7 @@ An Obsidian plugin that integrates with LLM CLI tools (Claude, Codex, OpenCode, - **Create Notes from Responses** - Save LLM responses as new notes in your vault - **Quick Prompts** - Commands for summarizing, explaining, and improving selected text - **Session Continuation** - Follow-up messages use session resumption for faster responses +- **ACP Mode** - Optional persistent connection mode for faster multi-turn conversations ## Requirements @@ -93,8 +94,13 @@ Copy `main.js`, `manifest.json`, and `styles.css` to your vault's `.obsidian/plu Each provider can be configured with: - Enable/disable +- Model selection (dropdown with common models, or enter custom model ID) - Custom command (if CLI is named differently) - Timeout override +- **ACP Mode** (experimental) - Use Agent Client Protocol for persistent connections +- **Thinking Mode** (ACP only) - Control extended thinking level (none/low/medium/high) + +When ACP mode is enabled, the available models list is populated dynamically from the connected agent. ### System Prompt @@ -153,28 +159,27 @@ The plugin captures session IDs from CLI tools and uses them for subsequent requ - **Claude**: `--resume ` - **OpenCode**: `--session ` - **Gemini**: `--resume ` -- **Codex**: Uses `resume` subcommand (different pattern, not fully supported) +- **Codex**: `--resume ` This improves response times for follow-up messages. Clearing the conversation resets the session. -### Future Improvements +### ACP Mode (Agent Client Protocol) -**ACP (Agent Client Protocol)**: We're exploring ACP support for improved performance and capabilities. ACP is a standardized protocol (like LSP for AI agents) that allows: -- Long-lived agent processes (no startup overhead per request) -- Streaming responses via JSON-RPC -- Terminal integration (agents can run commands) -- Standardized session management +ACP is a standardized protocol (like LSP but for AI agents) that provides: +- **Persistent connections** - No startup overhead per request +- **Streaming responses** - Real-time updates via JSON-RPC +- **Dynamic model discovery** - Available models fetched from the agent +- **Extended thinking** - Support for thinking mode levels -CLI tools with ACP support: -- `opencode acp` - OpenCode's ACP server -- `gemini --experimental-acp` - Gemini CLI's ACP mode -- Claude Code - via `@zed-industries/claude-code-acp` adapter +All four providers support ACP mode: +- **Claude** - via `@anthropic-ai/claude-code-acp` adapter +- **OpenCode** - native `opencode acp` server +- **Gemini** - via `gemini --experimental-acp` flag +- **Codex** - via `@zed-industries/codex-acp` adapter -See also: [obsidian-agent-client](https://github.com/RAIT-09/obsidian-agent-client) for an alternative ACP-based approach. +Enable ACP in provider settings. The first message establishes a connection; subsequent messages reuse it for faster responses. -**Long-lived CLI Process**: As an alternative to ACP, some tools support headless server modes: -- `opencode serve` / `opencode attach` - Headless server mode -- `codex mcp-server` - MCP server mode +See also: [obsidian-agent-client](https://github.com/RAIT-09/obsidian-agent-client) for an alternative ACP-based approach ## License diff --git a/package-lock.json b/package-lock.json index 228e342..60f5c38 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1095,8 +1095,7 @@ "resolved": "https://registry.npmjs.org/@marijn/find-cluster-break/-/find-cluster-break-1.0.2.tgz", "integrity": "sha512-l0h88YhZFyKdXIFNfSWpyjStDjGHwZ/U7iobcK1cQQD8sejsONdQtTVU+1wVN1PBw40PiiHB1vA5S7VTfQiP9g==", "dev": true, - "license": "MIT", - "peer": true + "license": "MIT" }, "node_modules/@pkgjs/parseargs": { "version": "0.11.0", @@ -1272,6 +1271,7 @@ "integrity": "sha512-WJtwWJu7UdlvzEAUm484QNg5eAoq5QR08KDNx7g45Usrs2NtOPiX8ugDqmKdXkyL03rBqU5dYNYVQetEpBHq2g==", "dev": true, "license": "MIT", + "peer": true, "dependencies": { "undici-types": "~6.21.0" } @@ -1386,6 +1386,7 @@ "integrity": "sha512-D6KZGomfNmjFhSWYdfR7Ojik5qWEpPoR4g5LQPzbFwiii/RkTudLcMFcCO6s7HTMLDQDWryOStV2KK6KqrIF8A==", "dev": true, "license": "MIT", + "peer": true, "dependencies": { "@vitest/snapshot": "^2.1.1", "@wdio/config": "9.23.2", @@ -1454,6 +1455,7 @@ "integrity": "sha512-OmwPKV8c5ecLqo+EkytN7oUeYfNmRI4uOXGIR1ybP7AK5Zz+l9R0dGfoadEuwi1aZXAL0vwuhtq3p0OL3dfqHQ==", "dev": true, "license": "MIT", + "peer": true, "engines": { "node": ">=18.20.0" }, @@ -1476,6 +1478,7 @@ "integrity": "sha512-tS8l2iaQc5aQav2LYYXx296F9KpdrU4/dmw5t9n9baXgdu8CKyGEd9orhTFQ7fYR55wFJ/85toQNOvIQHtIZrA==", "dev": true, "license": "MIT", + "peer": true, "dependencies": { "@types/node": "^20.1.0", "@wdio/logger": "9.18.0", @@ -1498,6 +1501,7 @@ "integrity": "sha512-HdzDrRs+ywAqbXGKqe1i/bLtCv47plz4TvsHFH3j729OooT5VH38ctFn5aLXgECmiAKDkmH/A6kOq2Zh5DIxww==", "dev": true, "license": "MIT", + "peer": true, "dependencies": { "chalk": "^5.1.2", "loglevel": "^1.6.0", @@ -1515,6 +1519,7 @@ "integrity": "sha512-V1wx8A8vMAExricXlCv0jzQOJTAgvgM/646QFr65U028+lqAGU23EkFp5H1WJj9I9jCHJTfMkxtUrPv0v7y63A==", "dev": true, "license": "MIT", + "peer": true, "dependencies": { "@types/mocha": "^10.0.6", "@types/node": "^20.11.28", @@ -2634,8 +2639,7 @@ "resolved": "https://registry.npmjs.org/crelt/-/crelt-1.0.6.tgz", "integrity": "sha512-VQ2MBenTq1fWZUH9DJNGti7kKv6EeAuYr3cLwxUWhIu1baTaXh4Ib5W2CqHVqib4/MqbYGJqiL3Zb8GJZr3l4g==", "dev": true, - "license": "MIT", - "peer": true + "license": "MIT" }, "node_modules/cross-spawn": { "version": "7.0.6", @@ -3364,6 +3368,7 @@ "integrity": "sha512-xNeU1ul02fU/EYpIOfMsSIARXBOY9V4KARdvU4lu9DwxMWr8W5cRT/iRURLGJX9wV/Vkg0Q1TabrN2NvxUdYJg==", "dev": true, "license": "MIT", + "peer": true, "dependencies": { "@vitest/snapshot": "^4.0.16", "deep-eql": "^5.0.2", @@ -5620,6 +5625,7 @@ "integrity": "sha512-n0KD3S+VndgaByrEtEe8NELy0ya6/s+KZ7OcxA6xOm5NN4thxKpQjo6eqEudHEvfGCeT/TYToAKJzitQ1I3XTg==", "dev": true, "license": "MIT", + "peer": true, "dependencies": { "@types/codemirror": "5.60.8", "moment": "2.29.4" @@ -6818,8 +6824,7 @@ "resolved": "https://registry.npmjs.org/style-mod/-/style-mod-4.1.3.tgz", "integrity": "sha512-i/n8VsZydrugj3Iuzll8+x/00GH2vnYsk1eomD8QiRrSAeW6ItbCQDtfXCeJHd0iwiNagqjQkvpvREEPtW3IoQ==", "dev": true, - "license": "MIT", - "peer": true + "license": "MIT" }, "node_modules/sumchecker": { "version": "3.0.1", @@ -7532,8 +7537,7 @@ "resolved": "https://registry.npmjs.org/w3c-keyname/-/w3c-keyname-2.2.8.tgz", "integrity": "sha512-dpojBhNsCNN7T82Tm7k26A6G9ML3NkhDsnw9n/eoxSRlVBB4CEtIQ/KTCLI2Fwf3ataSXRhYFkQi3SlnFwPvPQ==", "dev": true, - "license": "MIT", - "peer": true + "license": "MIT" }, "node_modules/wait-port": { "version": "1.1.0", @@ -7694,6 +7698,7 @@ "integrity": "sha512-VjfTw1bRJdBrzjoCu7BGThxn1JK2V7mAGvxibaBrCNIayPPQjLhVDNJPOVEiR7txM6zmOUWxhkCDxHjhMYirfQ==", "dev": true, "license": "MIT", + "peer": true, "dependencies": { "@types/node": "^20.11.30", "@types/sinonjs__fake-timers": "^8.1.5", diff --git a/src/executor/AcpExecutor.ts b/src/executor/AcpExecutor.ts index a6cffde..07210f3 100644 --- a/src/executor/AcpExecutor.ts +++ b/src/executor/AcpExecutor.ts @@ -22,6 +22,7 @@ import { type ModelInfo, } from "@agentclientprotocol/sdk"; import type { LLMPluginSettings, LLMProvider, ProgressEvent } from "../types"; +import { setAcpModels, clearAcpModels } from "../utils/modelFetcher"; export interface ThinkingOption { id: string; @@ -76,21 +77,37 @@ function nodeToWebWritable(nodeStream: NodeJS.WritableStream): WritableStream { + if (!settled) { + settled = true; + resolve(); + } + }; + const safeReject = (err: Error) => { + if (!settled) { + settled = true; + reject(err); + } + }; + try { const ok = nodeStream.write(chunk, (err) => { if (err) { streamClosed = true; - reject(err); + safeReject(err); } else { - resolve(); + safeResolve(); } }); if (!ok) { - nodeStream.once("drain", resolve); + // Backpressure - wait for drain event + nodeStream.once("drain", safeResolve); } } catch (err) { streamClosed = true; - reject(err); + safeReject(err instanceof Error ? err : new Error(String(err))); } }); }, @@ -131,8 +148,9 @@ export class AcpExecutor { constructor(settings: LLMPluginSettings) { this.settings = settings; + // Use arrow function that reads from this.settings so debug mode reflects current settings this.debug = (...args: unknown[]) => { - if (settings.debugMode) { + if (this.settings.debugMode) { console.log("[AcpExecutor]", ...args); } }; @@ -144,22 +162,49 @@ export class AcpExecutor { /** * Get the ACP command for a provider + * Uses provider-specific settings (customCommand, additionalArgs) if configured */ - private getAcpCommand(provider: LLMProvider): { cmd: string; args: string[] } | null { + private getAcpCommand(provider: LLMProvider): { cmd: string; args: string[]; env?: Record } | null { + const providerConfig = this.settings.providers[provider]; + + // Get base command and args for each provider + let baseCmd: string; + let baseArgs: string[]; + switch (provider) { case "opencode": - return { cmd: "opencode", args: ["acp"] }; + baseCmd = providerConfig.customCommand || "opencode"; + baseArgs = ["acp"]; + break; case "claude": // Claude uses the ACP adapter package - return { cmd: "npx", args: ["@zed-industries/claude-code-acp"] }; + // Use -y flag to avoid interactive prompts from npx + baseCmd = "npx"; + baseArgs = ["-y", "@zed-industries/claude-code-acp"]; + break; case "gemini": - return { cmd: "gemini", args: ["--experimental-acp"] }; + baseCmd = providerConfig.customCommand || "gemini"; + baseArgs = ["--experimental-acp"]; + break; case "codex": - // Codex doesn't have native ACP support yet - return null; + // Codex uses the ACP adapter package (like Claude) + // Use -y flag to avoid interactive prompts from npx + baseCmd = "npx"; + baseArgs = ["-y", "@zed-industries/codex-acp"]; + break; default: return null; } + + // Add any additional args from provider config + if (providerConfig.additionalArgs) { + baseArgs.push(...providerConfig.additionalArgs); + } + + // Include provider-specific environment variables + const env = providerConfig.envVars ? { ...providerConfig.envVars } : undefined; + + return { cmd: baseCmd, args: baseArgs, env }; } /** @@ -186,13 +231,17 @@ export class AcpExecutor { const cwd = workingDirectory ?? process.cwd(); this.debug("Spawning ACP agent:", acpCommand.cmd, acpCommand.args, "cwd:", cwd); + if (acpCommand.env) { + this.debug("With environment overrides:", Object.keys(acpCommand.env)); + } // Spawn the ACP agent process // Use shell: true to ensure commands like npx are found via shell PATH + // Merge provider-specific env vars with process.env this.process = spawn(acpCommand.cmd, acpCommand.args, { cwd, stdio: ["pipe", "pipe", "pipe"], - env: { ...process.env }, + env: { ...process.env, ...acpCommand.env }, shell: true, }); @@ -331,6 +380,9 @@ export class AcpExecutor { if (this.modelState) { this.debug("Current model:", this.modelState.currentModelId); this.debug("Available models:", this.modelState.availableModels.map((m) => m.modelId)); + + // Update the model fetcher cache with ACP models (preferred over static lists) + setAcpModels(provider, this.modelState.availableModels); } // Set model if configured @@ -659,6 +711,11 @@ export class AcpExecutor { async disconnect(): Promise { this.debug("Disconnecting..."); + // Clear ACP models cache for this provider + if (this.currentProvider) { + clearAcpModels(this.currentProvider); + } + if (this.process) { this.process.kill(); this.process = null; diff --git a/src/settings/SettingsTab.ts b/src/settings/SettingsTab.ts index 4b91512..730f476 100644 --- a/src/settings/SettingsTab.ts +++ b/src/settings/SettingsTab.ts @@ -258,7 +258,24 @@ export class LLMSettingTab extends PluginSettingTab { // ACP mode for supported providers if (ACP_SUPPORTED_PROVIDERS.includes(provider)) { - new Setting(settingsContainer) + // Create thinking mode setting first so we can reference it in ACP toggle + const thinkingModeSetting = new Setting(settingsContainer) + .setName("Thinking Mode (ACP)") + .setDesc('Extended thinking level. Common values: "none", "low", "medium", "high". Leave empty for agent default.') + .addText((text) => { + text.setPlaceholder("Agent default"); + text.setValue(providerConfig.thinkingMode ?? ""); + text.onChange(async (value) => { + this.plugin.settings.providers[provider].thinkingMode = value.trim() || undefined; + await this.plugin.saveSettings(); + }); + }); + + // Initially show/hide based on current ACP setting + thinkingModeSetting.settingEl.style.display = providerConfig.useAcp ? "" : "none"; + + // ACP toggle - insert before thinking mode setting + const acpSetting = new Setting(settingsContainer) .setName("Use ACP Mode (Experimental)") .setDesc("Use Agent Client Protocol for persistent connection. Faster for multiple messages but may be less stable.") .addToggle((toggle) => { @@ -266,21 +283,13 @@ export class LLMSettingTab extends PluginSettingTab { toggle.onChange(async (value) => { this.plugin.settings.providers[provider].useAcp = value; await this.plugin.saveSettings(); + // Show/hide thinking mode setting based on ACP toggle + thinkingModeSetting.settingEl.style.display = value ? "" : "none"; }); }); - // Thinking mode setting (only shown when ACP is enabled) - new Setting(settingsContainer) - .setName("Thinking Mode (ACP)") - .setDesc('Extended thinking level. Common values: "none", "low", "medium", "high". Leave empty for agent default. Only applies with ACP.') - .addText((text) => { - text.setPlaceholder("Agent default"); - text.setValue(providerConfig.thinkingMode ?? ""); - text.onChange(async (value) => { - this.plugin.settings.providers[provider].thinkingMode = value.trim() || undefined; - await this.plugin.saveSettings(); - }); - }); + // Move ACP setting before thinking mode setting in the DOM + settingsContainer.insertBefore(acpSetting.settingEl, thinkingModeSetting.settingEl); } // Timeout override (optional) @@ -368,9 +377,10 @@ export class LLMSettingTab extends PluginSettingTab { } }); - // Fetch dynamic models in the background - if (provider === "opencode") { - this.fetchAndUpdateModels(dd, provider, currentValue, isCustomMode); + // Fetch dynamic models in the background for all ACP-supported providers + // ACP models will be preferred if available (cached when ACP connects) + if (ACP_SUPPORTED_PROVIDERS.includes(provider)) { + this.fetchAndUpdateModels(dd, provider); } }); @@ -428,15 +438,16 @@ export class LLMSettingTab extends PluginSettingTab { /** * Fetch models dynamically and update dropdown */ - private async fetchAndUpdateModels( - dropdown: DropdownComponent, - provider: LLMProvider, - currentValue: string, - isCustomMode: boolean - ): Promise { + private async fetchAndUpdateModels(dropdown: DropdownComponent, provider: LLMProvider): Promise { try { const models = await fetchModelsForProvider(provider); + // Read the current value at update time (not from captured closure values) + // This avoids race conditions if user changed selection during fetch + const currentValue = this.plugin.settings.providers[provider].model ?? ""; + const dropdownValue = dropdown.getValue(); + const isCustomMode = dropdownValue === "__custom__"; + // Check if current value is in the new list const isInList = models.some((m) => m.value === currentValue); const shouldUseCustom = isCustomMode || (currentValue !== "" && !isInList); diff --git a/src/types.ts b/src/types.ts index 4171296..f1b744c 100644 --- a/src/types.ts +++ b/src/types.ts @@ -30,7 +30,7 @@ export interface ProviderConfig { /** * Providers that support ACP (Agent Client Protocol) */ -export const ACP_SUPPORTED_PROVIDERS: LLMProvider[] = ["claude", "opencode", "gemini"]; +export const ACP_SUPPORTED_PROVIDERS: LLMProvider[] = ["claude", "opencode", "gemini", "codex"]; /** * Common model options per provider @@ -39,11 +39,10 @@ export const ACP_SUPPORTED_PROVIDERS: LLMProvider[] = ["claude", "opencode", "ge export const PROVIDER_MODELS: Record = { claude: [ { value: "", label: "Default (CLI default)" }, - { value: "claude-opus-4-5-20251101", label: "Claude 4.5 Opus (most capable)" }, - { value: "claude-sonnet-4-5-20250929", label: "Claude 4.5 Sonnet" }, - { value: "claude-haiku-4-5-20251001", label: "Claude 4.5 Haiku (fast)" }, - { value: "claude-sonnet-4-20250514", label: "Claude 4 Sonnet" }, - { value: "claude-3-7-sonnet-20250219", label: "Claude 3.7 Sonnet" }, + { value: "claude-opus-4-5", label: "Claude 4.5 Opus (latest)" }, + { value: "claude-sonnet-4-5", label: "Claude 4.5 Sonnet (latest)" }, + { value: "claude-haiku-4-5", label: "Claude 4.5 Haiku (latest, fast)" }, + { value: "claude-sonnet-4", label: "Claude 4 Sonnet (latest)" }, ], gemini: [ { value: "", label: "Default (CLI default)" }, diff --git a/src/utils/modelFetcher.ts b/src/utils/modelFetcher.ts index 9c4430b..1a3d5ea 100644 --- a/src/utils/modelFetcher.ts +++ b/src/utils/modelFetcher.ts @@ -17,6 +17,43 @@ export interface ModelOption { const modelCache: Map = new Map(); const CACHE_TTL_MS = 5 * 60 * 1000; // 5 minutes +// ACP models cache - populated when ACP connects, preferred over CLI/static models +const acpModelCache: Map = new Map(); + +/** + * Set ACP models for a provider (called when ACP connects) + * These are preferred over static/CLI models when available + */ +export function setAcpModels( + provider: LLMProvider, + models: Array<{ modelId: string; name: string; description?: string | null }> +): void { + const options: ModelOption[] = [{ value: "", label: "Default (ACP default)" }]; + + for (const model of models) { + options.push({ + value: model.modelId, + label: model.name || model.modelId, + }); + } + + acpModelCache.set(provider, options); +} + +/** + * Clear ACP models for a provider (called when ACP disconnects) + */ +export function clearAcpModels(provider: LLMProvider): void { + acpModelCache.delete(provider); +} + +/** + * Check if ACP models are available for a provider + */ +export function hasAcpModels(provider: LLMProvider): boolean { + return acpModelCache.has(provider); +} + /** * Get models for OpenCode by calling `opencode models` */ @@ -75,10 +112,16 @@ async function fetchCodexModels(): Promise { /** * Fetch available models for a provider - * Uses caching to avoid repeated CLI calls + * Prefers ACP models if available, otherwise uses CLI/static models */ export async function fetchModelsForProvider(provider: LLMProvider): Promise { - // Check cache first + // Prefer ACP models if available (more accurate when connected) + const acpModels = acpModelCache.get(provider); + if (acpModels && acpModels.length > 1) { + return acpModels; + } + + // Check CLI cache const cached = modelCache.get(provider); if (cached && Date.now() - cached.timestamp < CACHE_TTL_MS) { return cached.models; @@ -114,3 +157,11 @@ export async function fetchModelsForProvider(provider: LLMProvider): Promise | null = null; // Track in-flight ACP connection constructor(leaf: WorkspaceLeaf, plugin: LLMPlugin) { super(leaf); @@ -467,38 +468,63 @@ export class ChatView extends ItemView { * Eagerly connect to ACP if enabled for the current provider. * This is called when the view opens and when the provider changes. * Blocks user input while connecting. + * Tracks in-flight connections to prevent overlapping connect/disconnect calls. */ - private async connectAcpIfEnabled(): Promise { - const providerConfig = this.plugin.settings.providers[this.currentProvider]; - const useAcp = providerConfig.useAcp && ACP_SUPPORTED_PROVIDERS.includes(this.currentProvider); + private connectAcpIfEnabled(): void { + // Store the target provider at call time to detect if it changes during async operations + const targetProvider = this.currentProvider; + const providerConfig = this.plugin.settings.providers[targetProvider]; + const useAcp = providerConfig.useAcp && ACP_SUPPORTED_PROVIDERS.includes(targetProvider); if (!useAcp) { // Not using ACP - make sure status bar shows configured model (not stale ACP model) - this.plugin.updateStatusBar(this.currentProvider); + this.plugin.updateStatusBar(targetProvider); return; } // Don't reconnect if already connected to this provider - if (this.acpExecutor.isConnected() && this.acpExecutor.getProvider() === this.currentProvider) { + if (this.acpExecutor.isConnected() && this.acpExecutor.getProvider() === targetProvider) { // Already connected - just update the status bar with model info const currentModel = this.acpExecutor.getCurrentModel(); if (currentModel) { - this.plugin.updateStatusBar(this.currentProvider, currentModel.name); + this.plugin.updateStatusBar(targetProvider, currentModel.name); } return; } + // If there's already a connection in progress, let it complete + // The caller can await acpConnectionPromise if needed + if (this.acpConnectionPromise) { + return; + } + + // Start the connection and track the promise + this.acpConnectionPromise = this.doConnectAcp(targetProvider); + } + + /** + * Internal method that performs the actual ACP connection. + * Separated to allow tracking the promise. + */ + private async doConnectAcp(targetProvider: LLMProvider): Promise { // Get vault path for working directory // eslint-disable-next-line @typescript-eslint/no-explicit-any const vaultPath = (this.app.vault.adapter as any).basePath as string | undefined; // Block user input while connecting this.setLoading(true); - this.plugin.updateStatusBar(this.currentProvider, undefined, "connecting"); - this.handleProgressEvent({ type: "status", message: `Connecting to ${this.currentProvider} ACP...` }); + this.plugin.updateStatusBar(targetProvider, undefined, "connecting"); + this.handleProgressEvent({ type: "status", message: `Connecting to ${targetProvider} ACP...` }); try { - await this.acpExecutor.connect(this.currentProvider, vaultPath); + await this.acpExecutor.connect(targetProvider, vaultPath); + + // Check if provider changed while we were connecting + if (this.currentProvider !== targetProvider) { + // Provider changed - disconnect and let the new provider connect + await this.acpExecutor.disconnect(); + return; + } // Verify connection succeeded if (!this.acpExecutor.isConnected()) { @@ -508,9 +534,9 @@ export class ChatView extends ItemView { // Update status bar with actual model from ACP session const currentModel = this.acpExecutor.getCurrentModel(); if (currentModel) { - this.plugin.updateStatusBar(this.currentProvider, currentModel.name, "connected"); + this.plugin.updateStatusBar(targetProvider, currentModel.name, "connected"); } else { - this.plugin.updateStatusBar(this.currentProvider, undefined, "connected"); + this.plugin.updateStatusBar(targetProvider, undefined, "connected"); } // Clear the connecting status @@ -523,8 +549,8 @@ export class ChatView extends ItemView { // Ensure we disconnect to clean up any partial state await this.acpExecutor.disconnect(); - // Reset status bar to idle state - this.plugin.updateStatusBar(this.currentProvider, undefined, "idle"); + // Reset status bar to idle state (use targetProvider since that's what we tried to connect) + this.plugin.updateStatusBar(targetProvider, undefined, "idle"); // Show error notification new Notice(`ACP connection failed: ${errorMsg.slice(0, 100)}`, 5000); @@ -532,6 +558,8 @@ export class ChatView extends ItemView { this.clearProgress(); } finally { this.setLoading(false); + // Clear the connection promise so future connects can proceed + this.acpConnectionPromise = null; } } diff --git a/test/specs/providers.e2e.ts b/test/specs/providers.e2e.ts index cd2ea6f..4a89df0 100644 --- a/test/specs/providers.e2e.ts +++ b/test/specs/providers.e2e.ts @@ -922,6 +922,7 @@ describe("ACP Mode Tests @acp @provider", () => { /** * Helper to enable ACP mode for a provider + * Clears any existing model to use ACP's default model selection */ async function enableAcpMode(provider: string): Promise { await browser.execute((p) => { @@ -929,6 +930,10 @@ describe("ACP Mode Tests @acp @provider", () => { if (plugin?.settings?.providers?.[p]) { plugin.settings.providers[p].enabled = true; plugin.settings.providers[p].useAcp = true; + // Clear any existing model to use ACP's default - important because + // invalid model formats (e.g. "gpt-4o-mini" vs "github-copilot/gpt-4o") + // can cause OpenCode ACP to return empty responses + plugin.settings.providers[p].model = ""; plugin.settings.defaultProvider = p; plugin.saveSettings(); } @@ -1063,7 +1068,8 @@ describe("ACP Mode Tests @acp @provider", () => { it("should use configured model with ACP @slow @acp-model", async () => { // Enable ACP for OpenCode with a specific model - const testModel = "gpt-4o-mini"; + // Must use OpenCode's model format: "opencode/model" or "github-copilot/model" + const testModel = "opencode/gpt-5-nano"; await browser.execute((model) => { const plugin = (window as any).app?.plugins?.plugins?.["obsidian-llm"]; From 028da4af9c6e73c15e3308bcb22f96f6d418fc83 Mon Sep 17 00:00:00 2001 From: Brian Thorne Date: Tue, 27 Jan 2026 11:38:09 +1300 Subject: [PATCH 12/13] Fix test failures from leftover ACP settings - Reset loading state when switching from ACP to non-ACP provider - Explicitly disable ACP for all providers in non-ACP test setup - Update setProviderModel helper to disable ACP for non-ACP tests The issue was that leftover ACP settings from previous test runs or manual testing could cause the input to be disabled when the chat view opens, as the eager ACP connection would set isLoading=true. --- src/views/ChatView.ts | 6 ++++++ test/specs/providers.e2e.ts | 17 +++++++++++++++++ 2 files changed, 23 insertions(+) diff --git a/src/views/ChatView.ts b/src/views/ChatView.ts index b206c80..b32c0d5 100644 --- a/src/views/ChatView.ts +++ b/src/views/ChatView.ts @@ -479,6 +479,12 @@ export class ChatView extends ItemView { if (!useAcp) { // Not using ACP - make sure status bar shows configured model (not stale ACP model) this.plugin.updateStatusBar(targetProvider); + // If there was an in-flight ACP connection, reset loading state + // (the connection will complete in background but input should be usable) + if (this.acpConnectionPromise && this.isLoading) { + this.setLoading(false); + this.clearProgress(); + } return; } diff --git a/test/specs/providers.e2e.ts b/test/specs/providers.e2e.ts index 4a89df0..f681434 100644 --- a/test/specs/providers.e2e.ts +++ b/test/specs/providers.e2e.ts @@ -22,6 +22,7 @@ const FAST_MODELS = { /** * Helper to configure a provider's model via plugin settings + * Also disables ACP mode for the provider (non-ACP tests) */ async function setProviderModel(provider: string, model: string): Promise { await browser.execute( @@ -30,6 +31,7 @@ async function setProviderModel(provider: string, model: string): Promise if (plugin?.settings?.providers?.[p]) { plugin.settings.providers[p].model = m; plugin.settings.providers[p].enabled = true; + plugin.settings.providers[p].useAcp = false; // Disable ACP for non-ACP tests // Enable yolo mode for Gemini (required for non-interactive use) if (p === "gemini") { plugin.settings.providers[p].yoloMode = true; @@ -102,6 +104,21 @@ describe("Provider Tests @provider", () => { { timeout: 30000, timeoutMsg: "Obsidian workspace did not load" } ); await browser.pause(2000); + + // Ensure ACP is disabled for all providers at the start of non-ACP tests + // This prevents leftover settings from previous test runs causing issues + await browser.execute(() => { + const plugin = (window as any).app?.plugins?.plugins?.["obsidian-llm"]; + if (plugin?.settings?.providers) { + for (const provider of ["claude", "opencode", "codex", "gemini"]) { + if (plugin.settings.providers[provider]) { + plugin.settings.providers[provider].useAcp = false; + } + } + plugin.saveSettings(); + } + }); + await browser.pause(200); }); describe("Claude Provider @claude @provider", () => { From c2ae031bf64b355e4766f13c30f53e4ac0317a9f Mon Sep 17 00:00:00 2001 From: Brian Thorne Date: Tue, 27 Jan 2026 13:33:51 +1300 Subject: [PATCH 13/13] Enable ACP mode by default for all providers ACP provides faster multi-turn conversations by maintaining a persistent connection instead of spawning a new CLI process for each request. --- src/settings/SettingsTab.ts | 4 ++-- src/types.ts | 4 ++++ 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/settings/SettingsTab.ts b/src/settings/SettingsTab.ts index 730f476..66b2c77 100644 --- a/src/settings/SettingsTab.ts +++ b/src/settings/SettingsTab.ts @@ -276,8 +276,8 @@ export class LLMSettingTab extends PluginSettingTab { // ACP toggle - insert before thinking mode setting const acpSetting = new Setting(settingsContainer) - .setName("Use ACP Mode (Experimental)") - .setDesc("Use Agent Client Protocol for persistent connection. Faster for multiple messages but may be less stable.") + .setName("Use ACP Mode") + .setDesc("Use Agent Client Protocol for persistent connection. Faster for multi-turn conversations. Disable to use CLI subprocess per request.") .addToggle((toggle) => { toggle.setValue(providerConfig.useAcp ?? false); toggle.onChange(async (value) => { diff --git a/src/types.ts b/src/types.ts index f1b744c..b3e5090 100644 --- a/src/types.ts +++ b/src/types.ts @@ -101,15 +101,19 @@ export interface LLMPluginSettings { export const DEFAULT_PROVIDER_CONFIGS: Record = { claude: { enabled: true, + useAcp: true, }, opencode: { enabled: false, + useAcp: true, }, codex: { enabled: false, + useAcp: true, }, gemini: { enabled: false, + useAcp: true, }, };