From 4e4e74be496310504cec4ce6245f63d3f4c61ad5 Mon Sep 17 00:00:00 2001 From: Ogrodev Date: Tue, 12 May 2026 16:45:33 -0300 Subject: [PATCH] fix(coding-agent/acp): tighten ACP conformance per review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the codex review comments on #1015 plus a sweep of adjacent ACP conformance gaps surfaced while wiring them up. Tool call + diff metadata - acp-event-mapper: thread session cwd through and resolve every `ToolCallLocation` (initial args, in-flight updates, result details) to absolute paths against it; ACP requires absolute paths for client-side file mapping. - edit/modes/patch: emit the destination path for moves in the diff result so post-edit "open file" actions land on the new file. Permissions - agent-session: pass cwd into `extractPermissionLocations` and resolve raw `path`/`file`/etc. fields against it before sending `session/request_permission`. - agent-session: gate the permission wrapper on `bridge.capabilities.requestPermission && bridge.requestPermission`, matching the read/write/bash capability+method pattern. acp-agent - `authenticate`: validate `methodId` against the methods advertised by `initialize` and reject anything else, so malformed clients fail fast. - `setSessionConfigOption(MODE_CONFIG_ID)`: also emit `current_mode_update` so clients tracking `modes.currentModeId` see the same transition `session/set_mode` would produce. - Pass `runtime.notifyConfigChanged` to builtins; emit `available_commands_update` from a shared `reloadPlugins` helper reused by `/reload-plugins`, `/marketplace`, and `/plugins`. - prompt resource handling: route `resource` content with `image/*` MIME into the `images` array instead of dropping it as an opaque blob; non-image blobs still fall back to the URI placeholder. - pass session cwd to the event mapper. Builtins - model: call `runtime.notifyConfigChanged()` after a successful `setModel` so the ACP config selector reflects the new model immediately. - mcp: redact query strings and userinfo from MCP server URLs before emitting them in `/mcp list` (prevents leaking `?exaApiKey=…` style secrets); wire `manager.setAuthStorage(...)` before `prepareConfig` in `/mcp test|resources|prompts` so OAuth servers can refresh tokens. - ssh: reject non-integer `--port` values via a `^\d+$` guard instead of silently coercing through `Number.parseInt`; list project hosts first and dedupe user-scope duplicates to match capability-loader precedence. - export: reject clipboard aliases (`--copy`, `clipboard`, `copy`) before passing them to `exportToHtml` as a filename. - compact / force / move / browser: surface underlying failures via `usage(errorMessage(...))` instead of letting them crash the command. - session save|delete: route through the active SessionManager so the persist writer is consulted and stale storage references are removed. - marketplace / plugins / reload-plugins: call `runtime.reloadPlugins()` on install/uninstall/upgrade and enable/disable so slash command registries and command lists refresh consistently. - shared.usage: make async and `await runtime.output(...)` so `sessionUpdate` text is never dropped or reordered. - types: document the new `reloadPlugins` and `notifyConfigChanged` runtime hooks. bash tool - Use a shared `fireKill()` from the abort listener so `session/cancel` terminates the remote command immediately instead of waiting for the next `currentOutput()` round trip. - Race `currentOutput()` against the abort signal so a stuck `terminal/output` RPC cannot delay cancellation. - Kill the terminal before reading final output on timeout so a slow output read cannot let a timed-out command keep running past the enforced timeout. Tests - acp-agent.test: extend the existing config-option assertions to verify both `model` and `thinking_level` changes emit `config_option_update` notifications scoped to the right session. - acp-builtins.test: cover `/model` emitting both `notifyTitleChanged` and `notifyConfigChanged`; lock in the parsed `mcp add` / `ssh add` call shapes so future arg-parser regressions fail the test instead of silently writing different configs; add a `reloadPlugins` stub plus a typed `notifyConfigChanged` slot to the shared test runtime factory. - acp-stdout-hygiene.test: drain stderr in parallel and assert no JSON-RPC frame leaks onto it; terminate the spawned process so the stderr pump resolves deterministically. CHANGELOG: itemize the above under `[Unreleased] > Fixed`. CI - bun run check: clean (TS + Rust) - bun run test: 4128 pass / 689 skip / 0 fail (TS); 252 pass / 0 fail (Rust nextest) - bun run ci:test:smoke: --version / --help / `stats --help` all OK --- packages/coding-agent/CHANGELOG.md | 22 ++++- packages/coding-agent/src/edit/modes/patch.ts | 6 +- .../coding-agent/src/modes/acp/acp-agent.ts | 89 +++++++++++++++---- .../src/modes/acp/acp-event-mapper.ts | 68 +++++++++----- .../coding-agent/src/session/agent-session.ts | 20 +++-- .../slash-commands/acp-builtins/browser.ts | 11 ++- .../slash-commands/acp-builtins/compact.ts | 11 ++- .../src/slash-commands/acp-builtins/export.ts | 10 ++- .../src/slash-commands/acp-builtins/force.ts | 8 +- .../acp-builtins/marketplace.ts | 4 + .../src/slash-commands/acp-builtins/mcp.ts | 39 +++++++- .../src/slash-commands/acp-builtins/model.ts | 1 + .../src/slash-commands/acp-builtins/move.ts | 11 ++- .../slash-commands/acp-builtins/plugins.ts | 1 + .../acp-builtins/reload-plugins.ts | 2 +- .../slash-commands/acp-builtins/session.ts | 16 ++-- .../src/slash-commands/acp-builtins/shared.ts | 4 +- .../src/slash-commands/acp-builtins/ssh.ts | 20 +++-- .../src/slash-commands/acp-builtins/types.ts | 8 ++ packages/coding-agent/src/tools/bash.ts | 50 +++++++++-- packages/coding-agent/test/acp-agent.test.ts | 12 +++ .../coding-agent/test/acp-builtins.test.ts | 55 ++++++++++++ .../test/acp-stdout-hygiene.test.ts | 44 +++++++++ 23 files changed, 431 insertions(+), 81 deletions(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 6b0e63df1..dc1b9532b 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -47,6 +47,26 @@ - Fixed `runSubagent` (subagent task executor) carrying the same latent `AuthStorage`/`ModelRegistry` divergence as `createAgentSession()`: when only `options.modelRegistry` was supplied, the executor previously fell through to a fresh `discoverAuthStorage()` and handed that orphan into `createAgentSession()` alongside a registry whose `.authStorage` was a different instance. The executor now reconciles to `modelRegistry.authStorage` before any further work and rejects mismatched `options.authStorage`/`options.modelRegistry.authStorage` pairs the same way the SDK does, so subagents can no longer silently observe a different storage view than their parent. - Fixed `github` tool's `search_issues`/`search_prs`/`search_code`/`search_commits`/`search_repos` ops always returning 0 results when the query contained more than one qualifier (e.g. `is:merged is:pr`, `is:open author:foo`). `gh search …` since the `advanced_search=true` rollout in gh 2.92 silently wraps multi-token positional queries in parentheses and quotes everything after the first qualifier as that qualifier's value (`is:"merged is:pr"`), which GitHub then matches as a literal state filter that no PR can satisfy. The tool now calls `gh api -X GET /search/ -f q=… -F per_page=…` directly so the qualifiers reach GitHub's search API verbatim. `is:issue`/`is:pr` and `repo:/` are appended internally to preserve the previous CLI-flag behavior; the user-facing query string in the formatted output is unchanged. `state` for merged PRs is derived from `pull_request.merged_at` so the rendered `State:` line stays `merged`/`closed`/`open` as before. - Fixed `read` tool renderer rendering failed reads with a success check (`✓`) and styling the error message as file content while the surrounding box was red. The renderer now branches on `isError` for both file and URL paths: header shows `✘ Read ` with a proper error icon and the underlying message is rendered as an error line. `renderReadUrlResult` got the same treatment so failed URL reads also get the cross icon instead of falling through to the `"No response data"` Text fallback. Mirrors the `bash`/`find` renderer error pattern. +- Fixed ACP mode to advertise and handle non-TUI builtin slash commands and `/skill:` commands +- Fixed ACP `session/resume` and `session/close` to dispatch correctly under SDK 0.21 by renaming `unstable_resumeSession` / `unstable_closeSession` to the stable `resumeSession` / `closeSession` method names the SDK now routes to +- Fixed ACP `tool_call` / `tool_call_update` `locations` to always emit absolute paths (resolved against the session cwd) so editor clients can reliably open or focus the referenced file +- Fixed ACP edit `diff` metadata for moves to point at the destination path rather than the now-deleted source so post-edit "open file" actions land on the new file +- Fixed ACP `session/request_permission` `locations` to be absolute and to honor the `requestPermission` capability bit instead of only checking for the method, matching the read/write/bash capability gating +- Fixed ACP `authenticate` to reject `methodId` values that were not advertised by `initialize` so malformed clients fail fast instead of being treated as authenticated +- Fixed ACP mode changes made via `session/set_session_config_option` (`MODE_CONFIG_ID`) to also emit a `current_mode_update` notification, matching `session/set_mode` so clients tracking `modes.currentModeId` stay in sync +- Fixed `/model` ACP builtin to emit a `config_option_update` after switching models so clients show the new model in config selectors immediately +- Fixed `/mcp list` (ACP) to redact query strings and userinfo from server URLs before emitting them, so API keys embedded in URLs (e.g. `?exaApiKey=…`) are not leaked to clients +- Fixed `/mcp test|resources|prompts` (ACP) to wire the auth storage before `prepareConfig` so OAuth-backed MCP servers can refresh tokens and inject `Authorization` headers +- Fixed `/mcp list`, `/mcp test`, `/mcp resources`, `/mcp prompts`, `/mcp enable`, and `/mcp disable` (ACP) to preserve project-over-user precedence when the same server name is defined in both scopes, matching the runtime capability merge so toggling the duplicated name flips the effective entry +- Fixed `/ssh add --port` parsing to reject non-integer values (e.g. `22oops`) instead of silently coercing them via `Number.parseInt` +- Fixed `/ssh list` to deduplicate hosts shared between project and user scopes, listing project entries first to match capability-loader precedence +- Fixed `/export` (ACP) to reject clipboard aliases (`--copy`, `clipboard`, `copy`) instead of using them as the output filename +- Fixed ACP builtin commands (`/compact`, `/force`, `/move`, `/browser`) to surface underlying failures via `output()` instead of swallowing them +- Fixed `/session save|delete` (ACP) to route through the active `SessionManager` so the persist writer is consulted and stale storage references are removed +- Fixed `/reload-plugins`, `/marketplace install|uninstall|upgrade`, and `/plugins enable|disable` (ACP) to refresh slash command registries and emit `available_commands_update` after plugin state changes +- Fixed ACP `usage()` text emission to be awaited so help and error output is not dropped or reordered when commands return immediately +- Fixed ACP `bash` tool to release the client terminal handle on `terminal/output` or `waitForExit` failures, and to race output polling against abort so a stuck RPC cannot delay cancellation +- Fixed ACP `resource` content blocks with `image/*` MIME types to be routed into the LLM `images` array instead of being dropped as opaque blobs - Fixed `pr://` and `issue://` URLs accepting empty, `.`, or `..` path segments. `pr://owner//77`, `pr://owner/repo/77/diff//2`, and `pr://owner/../77/diff` previously slipped past the `.filter(Boolean)` split and were forwarded to `gh`; now they throw `Invalid :// URL: empty or unsafe path segment` before any subprocess work. - Fixed `read` of `issue://` / `pr://` URLs ignoring the read tool's `AbortSignal`. Aborting a long `pr:///diff/all` or stale issue fetch now propagates into the resolver and short-circuits at the handler entry; previously the `gh` round-trip and cache write ran to completion. - Fixed `read :raw` (and the `raw: true` arg) still rendering markdown internal-URL content through the formatted markdown renderer. The TUI now respects the raw selector and falls back to the code-cell renderer so verbatim bytes are shown when requested. @@ -193,8 +213,6 @@ - Fixed the SSH tool on native Windows by avoiding OpenSSH ControlMaster multiplexing, which Win32-OpenSSH does not support and reports as `getsockname failed` ([#154](https://github.com/can1357/oh-my-pi/issues/154)). - Fixed `/export` and `/tree` not showing developer-role messages (including the plan content injected after `/plan` approval) so the HTML export and TUI session tree now render developer messages dimmed with their actual content instead of hiding them entirely ([#753](https://github.com/can1357/oh-my-pi/issues/753)) - Fixed `Timed out initializing browser tab worker` on prebuilt binaries by rewriting `spawnTabWorker` to import the worker entry with `with { type: "file" }` so Bun's `--compile` bundler statically discovers and embeds `tab-worker-entry.ts` in the single-file binary ([#1011](https://github.com/can1357/oh-my-pi/issues/1011)) -- Fixed ACP mode to advertise and handle non-TUI builtin slash commands and `/skill:` commands -- Fixed ACP `session/resume` and `session/close` to dispatch correctly under SDK 0.21 by renaming `unstable_resumeSession` / `unstable_closeSession` to the stable `resumeSession` / `closeSession` method names the SDK now routes to ## [14.9.3] - 2026-05-10 ### Breaking Changes diff --git a/packages/coding-agent/src/edit/modes/patch.ts b/packages/coding-agent/src/edit/modes/patch.ts index 063dd132c..6eb96b631 100644 --- a/packages/coding-agent/src/edit/modes/patch.ts +++ b/packages/coding-agent/src/edit/modes/patch.ts @@ -1779,7 +1779,11 @@ export async function executePatchSingle( content: [{ type: "text", text: resultText }], details: { diff: diffResult.diff, - path: resolvedPath, + // When the patch moves the file, anchor the diff to the destination + // path. ACP `ToolCallContent.diff.path` comes from this field, and + // clients use it to open or focus the file post-change; pointing at + // the (now-deleted) source navigates to nothing. + path: result.change.newPath ?? resolvedPath, firstChangedLine: diffResult.firstChangedLine, diagnostics: mergedDiagnostics, op, diff --git a/packages/coding-agent/src/modes/acp/acp-agent.ts b/packages/coding-agent/src/modes/acp/acp-agent.ts index 9d9572ff5..69ab38dd7 100644 --- a/packages/coding-agent/src/modes/acp/acp-agent.ts +++ b/packages/coding-agent/src/modes/acp/acp-agent.ts @@ -41,8 +41,9 @@ import { } from "@agentclientprotocol/sdk"; import type { AssistantMessage, Model } from "@oh-my-pi/pi-ai"; import { logger, VERSION } from "@oh-my-pi/pi-utils"; -import { disableProvider, enableProvider } from "../../capability"; +import { disableProvider, enableProvider, reset as resetCapabilities } from "../../capability"; import { Settings } from "../../config/settings"; +import { clearPluginRootsAndCaches, resolveActiveProjectRegistryPath } from "../../discovery/helpers"; import type { ExtensionUIContext } from "../../extensibility/extensions"; import { runExtensionCompact } from "../../extensibility/extensions/compact-handler"; import { buildSkillPromptMessage, getSkillSlashCommandName } from "../../extensibility/skills"; @@ -217,7 +218,15 @@ export class AcpAgent implements Agent { }; } - async authenticate(_params: AuthenticateRequest): Promise { + async authenticate(params: AuthenticateRequest): Promise { + // ACP spec: `methodId` must be one of the methods advertised by `initialize`. + // Reject anything else so malformed clients fail fast rather than appearing + // authenticated and surfacing a downstream model failure later. + const supportsTerminalAuth = this.#clientCapabilities?.auth?.terminal === true; + const validMethods = supportsTerminalAuth ? ["agent", "terminal"] : ["agent"]; + if (!validMethods.includes(params.methodId)) { + throw new Error(`Unknown ACP auth method: ${params.methodId}`); + } return {}; } @@ -335,6 +344,16 @@ export class AcpAgent implements Agent { throw new Error(`Unknown ACP config option: ${params.configId}`); } + // When mode is changed via the generic config-option API, mirror the + // `current_mode_update` notification that `setSessionMode` emits so + // ACP clients tracking session-mode state see a consistent transition. + if (params.configId === MODE_CONFIG_ID) { + await this.#connection.sessionUpdate({ + sessionId: record.session.sessionId, + update: this.#buildCurrentModeUpdate(record.session), + }); + } + const configOptions = this.#buildConfigOptions(record.session); await this.#connection.sessionUpdate({ sessionId: record.session.sessionId, @@ -401,6 +420,7 @@ export class AcpAgent implements Agent { cwd: record.session.sessionManager.getCwd(), output: output => this.#emitCommandOutput(record, output), refreshCommands: () => this.#emitAvailableCommandsUpdate(record), + reloadPlugins: () => this.#reloadPluginState(record), notifyTitleChanged: async () => { await this.#connection.sessionUpdate({ sessionId: record.session.sessionId, @@ -411,6 +431,15 @@ export class AcpAgent implements Agent { }, }); }, + notifyConfigChanged: async () => { + await this.#connection.sessionUpdate({ + sessionId: record.session.sessionId, + update: { + sessionUpdate: "config_option_update", + configOptions: this.#buildConfigOptions(record.session), + }, + }); + }, }); if (builtinResult !== false) { if ("prompt" in builtinResult) { @@ -729,6 +758,7 @@ export class AcpAgent implements Agent { for (const notification of mapAgentSessionEventToAcpSessionUpdates(event, record.session.sessionId, { getMessageId: message => this.#getLiveMessageId(record, message), getMessageProgress: message => this.#getLiveMessageProgress(record, message), + cwd: record.session.sessionManager.getCwd(), })) { await this.#connection.sessionUpdate(notification); } @@ -858,6 +888,12 @@ export class AcpAgent implements Agent { case "resource": if ("text" in block.resource) { textParts.push(block.resource.text); + } else if (typeof block.resource.mimeType === "string" && block.resource.mimeType.startsWith("image/")) { + // `embeddedContext: true` covers both text and blob resources, but + // blobs aren't directly consumable by the LLM. Route image blobs + // to the images array so the user's intent survives; everything + // else falls back to the URI placeholder below. + images.push({ type: "image", data: block.resource.blob, mimeType: block.resource.mimeType }); } else { textParts.push(`[embedded resource: ${block.resource.uri}]`); } @@ -1038,14 +1074,10 @@ export class AcpAgent implements Agent { commands.push(command); }; - for (const command of session.customCommands) { - appendCommand({ - name: command.command.name, - description: command.command.description, - input: { hint: "arguments" }, - }); - } - + // Advertise in the order dispatch resolves them: ACP builtins first + // (so core commands like `/model`, `/mcp`, `/todo` cannot be shadowed), + // then skills, then custom/user commands, then file-based slash + // commands. `appendCommand` dedupes by name so earlier entries win. for (const command of ACP_BUILTIN_SLASH_COMMANDS) { appendCommand(command); } @@ -1060,6 +1092,14 @@ export class AcpAgent implements Agent { } } + for (const command of session.customCommands) { + appendCommand({ + name: command.command.name, + description: command.command.description, + input: { hint: "arguments" }, + }); + } + for (const command of await loadSlashCommands({ cwd: session.sessionManager.getCwd() })) { appendCommand({ name: command.name, @@ -1127,6 +1167,23 @@ export class AcpAgent implements Agent { }); } + /** + * Reload plugin/registry state for an ACP session. Mirrors the interactive + * `/reload-plugins` and `/move` flows: invalidates the plugin-roots cache, + * resets the capability cache, refreshes the session's slash-command state, + * then re-advertises commands so the client sees newly installed/disabled + * plugins. + */ + async #reloadPluginState(record: ManagedSessionRecord): Promise { + const cwd = record.session.sessionManager.getCwd(); + const projectPath = await resolveActiveProjectRegistryPath(cwd); + clearPluginRootsAndCaches(projectPath ? [projectPath] : undefined); + resetCapabilities(); + const fileCommands = await loadSlashCommands({ cwd }); + record.session.setSlashCommands(fileCommands); + await this.#emitAvailableCommandsUpdate(record); + } + async #emitEndOfTurnUpdates(record: ManagedSessionRecord): Promise { const sessionId = record.session.sessionId; @@ -1217,14 +1274,15 @@ export class AcpAgent implements Agent { } async #replaySessionHistory(record: ManagedSessionRecord): Promise { + const cwd = record.session.sessionManager.getCwd(); for (const message of record.session.sessionManager.buildSessionContext().messages as ReplayableMessage[]) { - for (const notification of this.#messageToReplayNotifications(record.session.sessionId, message)) { + for (const notification of this.#messageToReplayNotifications(record.session.sessionId, message, cwd)) { await this.#connection.sessionUpdate(notification); } } } - #messageToReplayNotifications(sessionId: string, message: ReplayableMessage): SessionNotification[] { + #messageToReplayNotifications(sessionId: string, message: ReplayableMessage, cwd: string): SessionNotification[] { if (message.role === "assistant") { return this.#replayAssistantMessage(sessionId, message); } @@ -1246,7 +1304,7 @@ export class AcpAgent implements Agent { typeof message.toolCallId === "string" && typeof message.toolName === "string" ) { - return this.#replayToolResult(sessionId, { + return this.#replayToolResult(sessionId, cwd, { ...message, toolCallId: message.toolCallId, toolName: message.toolName, @@ -1338,6 +1396,7 @@ export class AcpAgent implements Agent { #replayToolResult( sessionId: string, + cwd: string, message: Required> & ReplayableMessage, ): SessionNotification[] { const args = this.#buildReplayToolArgs(message.details); @@ -1359,8 +1418,8 @@ export class AcpAgent implements Agent { }, }; return [ - ...mapAgentSessionEventToAcpSessionUpdates(startEvent, sessionId), - ...mapAgentSessionEventToAcpSessionUpdates(endEvent, sessionId), + ...mapAgentSessionEventToAcpSessionUpdates(startEvent, sessionId, { cwd }), + ...mapAgentSessionEventToAcpSessionUpdates(endEvent, sessionId, { cwd }), ]; } diff --git a/packages/coding-agent/src/modes/acp/acp-event-mapper.ts b/packages/coding-agent/src/modes/acp/acp-event-mapper.ts index b9f933190..593fc205a 100644 --- a/packages/coding-agent/src/modes/acp/acp-event-mapper.ts +++ b/packages/coding-agent/src/modes/acp/acp-event-mapper.ts @@ -6,6 +6,7 @@ import type { ToolKind, } from "@agentclientprotocol/sdk"; import type { AgentSessionEvent } from "../../session/agent-session"; +import { resolveToCwd } from "../../tools/path-utils"; import type { TodoStatus } from "../../tools/todo-write"; interface MessageProgress { @@ -16,6 +17,13 @@ interface MessageProgress { interface AcpEventMapperOptions { getMessageId?: (message: unknown) => string | undefined; getMessageProgress?: (message: unknown) => MessageProgress | undefined; + /** + * Session cwd. Tool call locations sent to ACP clients must be absolute + * (the editor host needs them to open or focus files). When provided, + * the mapper resolves raw `path`/`file`/etc. args against this cwd + * before emitting `ToolCallLocation` entries. + */ + cwd?: string; } interface ContentArrayContainer { @@ -144,7 +152,7 @@ export function mapAgentSessionEventToAcpSessionUpdates( status: "pending", rawInput: event.args, }; - const locations = extractToolLocations(event.args); + const locations = extractToolLocations(event.args, options.cwd); if (locations.length > 0) { update.locations = locations; } @@ -163,7 +171,7 @@ export function mapAgentSessionEventToAcpSessionUpdates( if (content.length > 0) { update.content = content; } - const locations = extractToolLocations(event.args); + const locations = extractToolLocations(event.args, options.cwd); if (locations.length > 0) { update.locations = locations; } @@ -183,7 +191,7 @@ export function mapAgentSessionEventToAcpSessionUpdates( if (content.length > 0) { update.content = content; } - const locations = extractToolLocationsFromResult(event.result); + const locations = extractToolLocationsFromResult(event.result, options.cwd); if (locations.length > 0) { update.locations = locations; } @@ -322,32 +330,45 @@ function buildToolTitle(toolName: string, args: unknown, intent: string | undefi return toolName; } -function extractToolLocations(args: unknown): ToolCallLocation[] { +/** + * Resolve a single raw path against cwd for an ACP location. When `cwd` is + * omitted we pass the value through unchanged (callers without session + * context, e.g. some legacy entry points and tests); the ACP-side caller + * always supplies cwd so notifications carry absolute paths. + */ +function toAcpLocationPath(value: string, cwd?: string): string { + if (!cwd) return value; + try { + return resolveToCwd(value, cwd); + } catch { + return value; + } +} + +function extractToolLocations(args: unknown, cwd?: string): ToolCallLocation[] { const locations: ToolCallLocation[] = []; - const path = extractStringProperty(args, "path"); - if (path) { + const seen = new Set(); + const pushPath = (raw: string | undefined) => { + if (!raw) return; + const path = toAcpLocationPath(raw, cwd); + if (seen.has(path)) return; + seen.add(path); locations.push({ path }); - } + }; - const oldPath = extractStringProperty(args, "oldPath"); - if (oldPath && oldPath !== path) { - locations.push({ path: oldPath }); - } - - const newPath = extractStringProperty(args, "newPath"); - if (newPath && newPath !== path && newPath !== oldPath) { - locations.push({ path: newPath }); - } + pushPath(extractStringProperty(args, "path")); + pushPath(extractStringProperty(args, "oldPath")); + pushPath(extractStringProperty(args, "newPath")); return locations; } /** Pull locations from a tool result's details (e.g. EditToolDetails.perFileResults[].path). */ -function extractToolLocationsFromResult(result: unknown): ToolCallLocation[] { +function extractToolLocationsFromResult(result: unknown, cwd?: string): ToolCallLocation[] { if (typeof result !== "object" || result === null) return []; const details = (result as { details?: unknown }).details; if (typeof details !== "object" || details === null) return []; - const direct = extractToolLocations(details); + const direct = extractToolLocations(details, cwd); const perFile = (details as { perFileResults?: unknown }).perFileResults; if (!Array.isArray(perFile)) { return direct; @@ -355,11 +376,12 @@ function extractToolLocationsFromResult(result: unknown): ToolCallLocation[] { const seen = new Set(direct.map(loc => loc.path)); const locations = [...direct]; for (const entry of perFile) { - const path = extractStringProperty(entry, "path"); - if (path && !seen.has(path)) { - seen.add(path); - locations.push({ path }); - } + const raw = extractStringProperty(entry, "path"); + if (!raw) continue; + const path = toAcpLocationPath(raw, cwd); + if (seen.has(path)) continue; + seen.add(path); + locations.push({ path }); } return locations; } diff --git a/packages/coding-agent/src/session/agent-session.ts b/packages/coding-agent/src/session/agent-session.ts index 5412ab1a2..8aec02120 100644 --- a/packages/coding-agent/src/session/agent-session.ts +++ b/packages/coding-agent/src/session/agent-session.ts @@ -556,14 +556,23 @@ function derivePermissionTitle(toolName: string, args: unknown): string { return toolName; } -function extractPermissionLocations(args: unknown): { path: string; line?: number }[] { +function extractPermissionLocations(args: unknown, cwd: string): { path: string; line?: number }[] { if (!args || typeof args !== "object") return []; const a = args as Record; const out: { path: string; line?: number }[] = []; const pushPath = (value: unknown) => { if (typeof value !== "string" || value.length === 0) return; - if (out.some(location => location.path === value)) return; - out.push({ path: value }); + // ACP locations carry file paths that the editor host will open or focus; + // they must be absolute or the client cannot resolve them. Resolve raw + // tool args (often cwd-relative) against the session cwd before sending. + let resolved: string; + try { + resolved = resolveToCwd(value, cwd); + } catch { + return; + } + if (out.some(location => location.path === resolved)) return; + out.push({ path: resolved }); }; pushPath(a.path); pushPath(a.file); @@ -2640,7 +2649,8 @@ export class AgentSession { */ #wrapToolForAcpPermission(tool: T): T { const bridge = this.#clientBridge; - if (!bridge?.requestPermission) return tool; + // Match the capability+method gating pattern used by read/write/bash. + if (!bridge?.capabilities.requestPermission || !bridge.requestPermission) return tool; if (!PERMISSION_REQUIRED_TOOLS.has(tool.name)) return tool; return new Proxy(tool, { get: (target, prop, receiver) => { @@ -2677,7 +2687,7 @@ export class AgentSession { toolName: target.name, title: derivePermissionTitle(target.name, args), rawInput: args, - locations: extractPermissionLocations(args), + locations: extractPermissionLocations(args, this.sessionManager.getCwd()), }, PERMISSION_OPTIONS, signal, diff --git a/packages/coding-agent/src/slash-commands/acp-builtins/browser.ts b/packages/coding-agent/src/slash-commands/acp-builtins/browser.ts index 1575fb040..cf79d3059 100644 --- a/packages/coding-agent/src/slash-commands/acp-builtins/browser.ts +++ b/packages/coding-agent/src/slash-commands/acp-builtins/browser.ts @@ -19,7 +19,16 @@ export const browserCommand: AcpBuiltinCommandSpec = { runtime.settings.set("browser.headless" as SettingPath, next as SettingValue); const tool = runtime.session.getToolByName("browser"); if (tool && "restartForModeChange" in tool) { - await (tool as { restartForModeChange: () => Promise }).restartForModeChange(); + try { + await (tool as { restartForModeChange: () => Promise }).restartForModeChange(); + } catch (err) { + // Setting was already mutated; surface the restart failure so the + // user knows the browser is in an inconsistent state. + await runtime.output( + `Browser mode set to ${next ? "headless" : "visible"}, but restart failed: ${err instanceof Error ? err.message : String(err)}`, + ); + return commandConsumed(); + } } await runtime.output(`Browser mode: ${next ? "headless" : "visible"}`); return commandConsumed(); diff --git a/packages/coding-agent/src/slash-commands/acp-builtins/compact.ts b/packages/coding-agent/src/slash-commands/acp-builtins/compact.ts index dcf3d0b40..8f2ae7557 100644 --- a/packages/coding-agent/src/slash-commands/acp-builtins/compact.ts +++ b/packages/coding-agent/src/slash-commands/acp-builtins/compact.ts @@ -1,4 +1,4 @@ -import { commandConsumed } from "./shared"; +import { commandConsumed, errorMessage, usage } from "./shared"; import type { AcpBuiltinCommandSpec } from "./types"; export const compactCommand: AcpBuiltinCommandSpec = { @@ -8,7 +8,14 @@ export const compactCommand: AcpBuiltinCommandSpec = { handle: async (command, runtime) => { const before = runtime.session.getContextUsage?.(); const beforeTokens = before?.tokens; - await runtime.session.compact(command.args || undefined); + try { + await runtime.session.compact(command.args || undefined); + } catch (err) { + // Compaction precondition failures (no model, already compacted, too + // small) and provider errors propagate as plain Errors; surface them + // via runtime.output so they don't fail the ACP prompt turn. + return usage(`Compaction failed: ${errorMessage(err)}`, runtime); + } const after = runtime.session.getContextUsage?.(); const afterTokens = after?.tokens; if (beforeTokens != null && afterTokens != null) { diff --git a/packages/coding-agent/src/slash-commands/acp-builtins/export.ts b/packages/coding-agent/src/slash-commands/acp-builtins/export.ts index 172517922..5f243fb15 100644 --- a/packages/coding-agent/src/slash-commands/acp-builtins/export.ts +++ b/packages/coding-agent/src/slash-commands/acp-builtins/export.ts @@ -6,8 +6,16 @@ export const exportCommand: AcpBuiltinCommandSpec = { description: "Export session to HTML file", inputHint: "[path]", handle: async (command, runtime) => { + const arg = command.args.trim(); + // Match the interactive `/export` behavior: clipboard aliases are not a + // valid export target. Without this, the literal value (`copy`, + // `--copy`, `clipboard`) is passed to `exportToHtml` and becomes the + // output filename. + if (arg === "--copy" || arg === "clipboard" || arg === "copy") { + return usage("Use /dump to copy the session to clipboard.", runtime); + } try { - const filePath = await runtime.session.exportToHtml(command.args || undefined); + const filePath = await runtime.session.exportToHtml(arg || undefined); await runtime.output(`Session exported to: ${filePath}`); return commandConsumed(); } catch (err) { diff --git a/packages/coding-agent/src/slash-commands/acp-builtins/force.ts b/packages/coding-agent/src/slash-commands/acp-builtins/force.ts index e7ed51125..e8c83a08d 100644 --- a/packages/coding-agent/src/slash-commands/acp-builtins/force.ts +++ b/packages/coding-agent/src/slash-commands/acp-builtins/force.ts @@ -1,4 +1,4 @@ -import { commandConsumed, usage } from "./shared"; +import { commandConsumed, errorMessage, usage } from "./shared"; import type { AcpBuiltinCommandSpec } from "./types"; export const forceCommand: AcpBuiltinCommandSpec = { @@ -11,7 +11,11 @@ export const forceCommand: AcpBuiltinCommandSpec = { const toolName = spaceIdx === -1 ? command.args : command.args.slice(0, spaceIdx); const prompt = spaceIdx === -1 ? "" : command.args.slice(spaceIdx + 1).trim(); if (!toolName) return usage("Usage: /force: [prompt]", runtime); - runtime.session.setForcedToolChoice(toolName); + try { + runtime.session.setForcedToolChoice(toolName); + } catch (err) { + return usage(errorMessage(err), runtime); + } await runtime.output(`Next turn forced to use ${toolName}.`); return prompt ? { prompt } : commandConsumed(); }, diff --git a/packages/coding-agent/src/slash-commands/acp-builtins/marketplace.ts b/packages/coding-agent/src/slash-commands/acp-builtins/marketplace.ts index 8f63c09ac..62e9e2a85 100644 --- a/packages/coding-agent/src/slash-commands/acp-builtins/marketplace.ts +++ b/packages/coding-agent/src/slash-commands/acp-builtins/marketplace.ts @@ -79,6 +79,7 @@ async function handleInstallCommand( const pluginName = parsed.installSpec.slice(0, atIndex); const marketplace = parsed.installSpec.slice(atIndex + 1); await manager.installPlugin(pluginName, marketplace, { force: parsed.force, scope: parsed.scope }); + await runtime.reloadPlugins(); await runtime.output(`Installed ${pluginName} from ${marketplace}`); return commandConsumed(); } @@ -91,6 +92,7 @@ async function handleUninstallCommand( const parsed = parsePluginScopeArgs(rest, "Usage: /marketplace uninstall [--scope user|project] "); if ("error" in parsed) return usage(parsed.error, runtime); await manager.uninstallPlugin(parsed.pluginId, parsed.scope); + await runtime.reloadPlugins(); await runtime.output(`Uninstalled ${parsed.pluginId}`); return commandConsumed(); } @@ -124,6 +126,7 @@ async function handleUpgradeCommand( ); if ("error" in parsed) return usage(parsed.error, runtime); const result = await manager.upgradePlugin(parsed.pluginId, parsed.scope); + await runtime.reloadPlugins(); await runtime.output(`Upgraded ${parsed.pluginId} to ${result.version}`); return commandConsumed(); } @@ -132,6 +135,7 @@ async function handleUpgradeCommand( if (results.length === 0) { await runtime.output("All marketplace plugins are up to date"); } else { + await runtime.reloadPlugins(); const lines = results.map(result => ` ${result.pluginId}: ${result.from} -> ${result.to}`); await runtime.output(`Upgraded ${results.length} plugin(s):\n${lines.join("\n")}`); } diff --git a/packages/coding-agent/src/slash-commands/acp-builtins/mcp.ts b/packages/coding-agent/src/slash-commands/acp-builtins/mcp.ts index f7e7a13ad..c9831b3e4 100644 --- a/packages/coding-agent/src/slash-commands/acp-builtins/mcp.ts +++ b/packages/coding-agent/src/slash-commands/acp-builtins/mcp.ts @@ -1,4 +1,4 @@ -import { getMCPConfigPath } from "@oh-my-pi/pi-utils"; +import { getMCPConfigPath, logger } from "@oh-my-pi/pi-utils"; import { connectToServer, disconnectServer, listPrompts, listResources, listTools } from "../../mcp/client"; import { addMCPServer, @@ -16,6 +16,24 @@ import { parseCommandArgs } from "../../utils/command-args"; import { commandConsumed, errorMessage, parseNamedScopeArgs, parseSubcommand, usage } from "./shared"; import type { AcpBuiltinCommandRuntime, AcpBuiltinCommandSpec } from "./types"; +/** + * Strip query/userinfo from MCP URLs before emitting them to ACP clients. + * MCP server URLs frequently carry API keys in the query string (e.g. + * `https://mcp.exa.ai/?exaApiKey=…`), so the raw URL is unsafe to leak. + * Show origin only; fall back to `(hidden)` for unparseable inputs. + */ +function redactMcpUrl(url: string | undefined): string | undefined { + if (!url) return url; + try { + const parsed = new URL(url); + const origin = parsed.origin; + const pathOnly = parsed.pathname && parsed.pathname !== "/" ? parsed.pathname : ""; + return `${origin}${pathOnly}`; + } catch { + return "(hidden)"; + } +} + type AcpMcpScope = "user" | "project"; interface ParsedMcpAddArgs { @@ -208,11 +226,26 @@ async function withPreparedMcpConnection( let connection: MCPServerConnection | undefined; try { const manager = new MCPManager(runtime.cwd); + // Auth storage must be wired in before prepareConfig so OAuth-backed + // servers can refresh credentials and inject Authorization headers. + // Without this, `/mcp test|resources|prompts` silently fails for any + // server saved by the TUI/reauth path. + manager.setAuthStorage(runtime.session.modelRegistry.authStorage); const resolvedConfig = await manager.prepareConfig(config); connection = await connectToServer(name, resolvedConfig); return await fn(connection); } finally { - if (connection) void disconnectServer(connection); + if (connection) { + // Await cleanup so the stdio subprocess / HTTP DELETE has actually + // released the resource before this helper returns. Fire-and-forget + // here races with subsequent connect attempts and turns close + // failures into unhandled rejections. + try { + await disconnectServer(connection); + } catch (err) { + logger.warn("MCP disconnect after temporary connection failed", { name, err }); + } + } } } @@ -376,7 +409,7 @@ async function handleListCommand(runtime: AcpBuiltinCommandRuntime) { const enabled = config.enabled !== false && !disabledSet.has(name) ? "enabled" : "disabled"; const location = config.type === "http" || config.type === "sse" - ? (config as { url: string }).url + ? redactMcpUrl((config as { url: string }).url) : (config as { command: string }).command; return `${name} | ${type} | ${enabled} | ${location ?? "(unknown)"} [${scope}]`; }) diff --git a/packages/coding-agent/src/slash-commands/acp-builtins/model.ts b/packages/coding-agent/src/slash-commands/acp-builtins/model.ts index a7d7568ca..73682096c 100644 --- a/packages/coding-agent/src/slash-commands/acp-builtins/model.ts +++ b/packages/coding-agent/src/slash-commands/acp-builtins/model.ts @@ -22,6 +22,7 @@ export const modelCommand: AcpBuiltinCommandSpec = { await runtime.session.setModel(match); await runtime.output(`Model set to ${match.provider}/${match.id}.`); await runtime.notifyTitleChanged?.(); + await runtime.notifyConfigChanged?.(); return commandConsumed(); } catch (err) { return usage(`Failed to set model: ${errorMessage(err)}`, runtime); diff --git a/packages/coding-agent/src/slash-commands/acp-builtins/move.ts b/packages/coding-agent/src/slash-commands/acp-builtins/move.ts index 645eca0c3..702fe5800 100644 --- a/packages/coding-agent/src/slash-commands/acp-builtins/move.ts +++ b/packages/coding-agent/src/slash-commands/acp-builtins/move.ts @@ -19,9 +19,16 @@ export const moveCommand: AcpBuiltinCommandSpec = { return usage(`Directory does not exist or is not a directory: ${resolvedPath}`, runtime); } if (!isDirectory) return usage(`Directory does not exist or is not a directory: ${resolvedPath}`, runtime); - await runtime.sessionManager.flush(); - await runtime.sessionManager.moveTo(resolvedPath); + try { + await runtime.sessionManager.flush(); + await runtime.sessionManager.moveTo(resolvedPath); + } catch (err) { + return usage(`Move failed: ${err instanceof Error ? err.message : String(err)}`, runtime); + } setProjectDir(resolvedPath); + // Reload plugin/capability caches so the next prompt sees commands and + // capabilities scoped to the new cwd. + await runtime.reloadPlugins(); await runtime.notifyTitleChanged?.(); await runtime.output(`Session moved to ${runtime.sessionManager.getCwd()}.`); return commandConsumed(); diff --git a/packages/coding-agent/src/slash-commands/acp-builtins/plugins.ts b/packages/coding-agent/src/slash-commands/acp-builtins/plugins.ts index 5b0c0120f..f384a1a1d 100644 --- a/packages/coding-agent/src/slash-commands/acp-builtins/plugins.ts +++ b/packages/coding-agent/src/slash-commands/acp-builtins/plugins.ts @@ -14,6 +14,7 @@ async function handleEnableDisableCommand( const manager = await createMarketplaceManager(runtime); const isEnable = sub === "enable"; await manager.setPluginEnabled(parsed.pluginId, isEnable, parsed.scope); + await runtime.reloadPlugins(); await runtime.output(`${isEnable ? "Enabled" : "Disabled"} ${parsed.pluginId}`); return commandConsumed(); } diff --git a/packages/coding-agent/src/slash-commands/acp-builtins/reload-plugins.ts b/packages/coding-agent/src/slash-commands/acp-builtins/reload-plugins.ts index 4f14d37a8..15f37f4d2 100644 --- a/packages/coding-agent/src/slash-commands/acp-builtins/reload-plugins.ts +++ b/packages/coding-agent/src/slash-commands/acp-builtins/reload-plugins.ts @@ -5,7 +5,7 @@ export const reloadPluginsCommand: AcpBuiltinCommandSpec = { name: "reload-plugins", description: "Reload all plugins", handle: async (_command, runtime) => { - await runtime.refreshCommands(); + await runtime.reloadPlugins(); await runtime.output("Plugins reloaded."); return commandConsumed(); }, diff --git a/packages/coding-agent/src/slash-commands/acp-builtins/session.ts b/packages/coding-agent/src/slash-commands/acp-builtins/session.ts index 7de41f48b..6991e4eb3 100644 --- a/packages/coding-agent/src/slash-commands/acp-builtins/session.ts +++ b/packages/coding-agent/src/slash-commands/acp-builtins/session.ts @@ -1,4 +1,3 @@ -import { FileSessionStorage } from "../../session/session-storage"; import { commandConsumed, usage } from "./shared"; import type { AcpBuiltinCommandSpec } from "./types"; @@ -21,13 +20,16 @@ export const sessionCommand: AcpBuiltinCommandSpec = { if (runtime.session.isStreaming) return usage("Cannot delete the session while streaming.", runtime); const sessionFile = runtime.sessionManager.getSessionFile(); if (!sessionFile) return usage("No session file to delete (in-memory session).", runtime); - const storage = new FileSessionStorage(); - const exists = await storage.exists(sessionFile); - if (!exists) { - await runtime.output("Session has not been saved yet."); - return commandConsumed(); + // Route through the active SessionManager so the persist writer is + // closed before the file is deleted. Constructing a fresh + // FileSessionStorage and calling deleteSessionWithArtifacts leaves + // the active writer attached to the now-deleted path, so the next + // prompt would silently resurrect or corrupt the "deleted" file. + try { + await runtime.sessionManager.dropSession(sessionFile); + } catch (err) { + return usage(`Failed to delete session: ${err instanceof Error ? err.message : String(err)}`, runtime); } - await storage.deleteSessionWithArtifacts(sessionFile); await runtime.output( `Session deleted: ${sessionFile}. Use ACP \`session/load\` to switch to another session.`, ); diff --git a/packages/coding-agent/src/slash-commands/acp-builtins/shared.ts b/packages/coding-agent/src/slash-commands/acp-builtins/shared.ts index 347b7dbb1..f6e3b54fe 100644 --- a/packages/coding-agent/src/slash-commands/acp-builtins/shared.ts +++ b/packages/coding-agent/src/slash-commands/acp-builtins/shared.ts @@ -17,8 +17,8 @@ export function commandConsumed(): AcpBuiltinSlashCommandResult { return { consumed: true }; } -export function usage(text: string, runtime: AcpBuiltinCommandRuntime): AcpBuiltinSlashCommandResult { - void runtime.output(text); +export async function usage(text: string, runtime: AcpBuiltinCommandRuntime): Promise { + await runtime.output(text); return commandConsumed(); } diff --git a/packages/coding-agent/src/slash-commands/acp-builtins/ssh.ts b/packages/coding-agent/src/slash-commands/acp-builtins/ssh.ts index a8a4e39e7..5123be1c0 100644 --- a/packages/coding-agent/src/slash-commands/acp-builtins/ssh.ts +++ b/packages/coding-agent/src/slash-commands/acp-builtins/ssh.ts @@ -40,8 +40,14 @@ const SSH_ADD_OPTION_PARSERS = new Map([ "--port", (parsed, value) => { if (!value) return "Missing value for --port."; + // Reject any non-integer token. `Number.parseInt` accepts trailing + // garbage (parseInt("22oops") === 22) which silently coerces typos + // to valid-looking ports. + if (!/^\d+$/.test(value)) { + return "Invalid --port value. Must be an integer between 1 and 65535."; + } const port = Number.parseInt(value, 10); - if (Number.isNaN(port) || port < 1 || port > 65535) { + if (port < 1 || port > 65535) { return "Invalid --port value. Must be an integer between 1 and 65535."; } parsed.port = port; @@ -108,12 +114,16 @@ async function handleListCommand(runtime: AcpBuiltinCommandRuntime) { readSSHConfigFile(projectPath), ]); const entries: Array<{ name: string; host: string; user?: string; port?: number; scope: string }> = []; - for (const [name, config] of Object.entries(userConfig.hosts ?? {})) { - entries.push({ name, host: config.host, user: config.username, port: config.port, scope: "user" }); - } + // Capability loader resolves project before user, so list project hosts + // first and let the user-scope loop skip duplicates. Otherwise a host + // shared between scopes shows up under "user" when the project entry + // is the one actually in effect. for (const [name, config] of Object.entries(projectConfig.hosts ?? {})) { + entries.push({ name, host: config.host, user: config.username, port: config.port, scope: "project" }); + } + for (const [name, config] of Object.entries(userConfig.hosts ?? {})) { if (!entries.some(entry => entry.name === name)) { - entries.push({ name, host: config.host, user: config.username, port: config.port, scope: "project" }); + entries.push({ name, host: config.host, user: config.username, port: config.port, scope: "user" }); } } if (entries.length === 0) { diff --git a/packages/coding-agent/src/slash-commands/acp-builtins/types.ts b/packages/coding-agent/src/slash-commands/acp-builtins/types.ts index 1fb99cb11..03c5a7b22 100644 --- a/packages/coding-agent/src/slash-commands/acp-builtins/types.ts +++ b/packages/coding-agent/src/slash-commands/acp-builtins/types.ts @@ -15,7 +15,15 @@ export interface AcpBuiltinCommandRuntime { cwd: string; output: (text: string) => Promise | void; refreshCommands: () => Promise | void; + /** + * Reload plugin state (caches, slash command registry, project registries) + * and emit a fresh `available_commands_update`. Called by `/reload-plugins`, + * `/move`, and `/marketplace`/`/plugins` mutations so the session and the + * ACP client see a consistent view after plugin or project-scope changes. + */ + reloadPlugins: () => Promise; notifyTitleChanged?: () => Promise | void; + notifyConfigChanged?: () => Promise | void; } export type AcpBuiltinSlashCommandResult = false | { consumed: true } | { prompt: string }; diff --git a/packages/coding-agent/src/tools/bash.ts b/packages/coding-agent/src/tools/bash.ts index 8148c879b..3a1841d22 100644 --- a/packages/coding-agent/src/tools/bash.ts +++ b/packages/coding-agent/src/tools/bash.ts @@ -11,7 +11,7 @@ import { InternalUrlRouter } from "../internal-urls"; import { truncateToVisualLines } from "../modes/components/visual-truncate"; import type { Theme } from "../modes/theme/theme"; import bashDescription from "../prompts/tools/bash.md" with { type: "text" }; -import type { ClientBridgeTerminalExitStatus } from "../session/client-bridge"; +import type { ClientBridgeTerminalExitStatus, ClientBridgeTerminalOutput } from "../session/client-bridge"; import { DEFAULT_MAX_BYTES, streamTailUpdates, TailBuffer } from "../session/streaming-output"; import { renderStatusLine } from "../tui"; import { CachedOutputBlock } from "../tui/output-block"; @@ -648,15 +648,29 @@ export class BashTool implements AgentTool { | { kind: "timeout" } | { kind: "aborted" }; - // Set up abort listener before entering the poll loop. + // Set up abort listener before entering the poll loop. The listener + // kicks off `handle.kill()` synchronously so a `session/cancel` + // arriving mid-poll terminates the remote command immediately, + // instead of waiting for the next `currentOutput()` to return. const { promise: abortedP, resolve: resolveAborted } = Promise.withResolvers(); - const onAbortSignal = () => resolveAborted(); + let killStarted = false; + const fireKill = (): Promise => { + if (killStarted) return Promise.resolve(); + killStarted = true; + return handle.kill().catch((error: unknown) => { + logger.warn("ACP terminal kill failed", { terminalId: handle.terminalId, error }); + }); + }; + const onAbortSignal = () => { + resolveAborted(); + void fireKill(); + }; signal?.addEventListener("abort", onAbortSignal, { once: true }); try { try { if (signal?.aborted) { - await handle.kill(); + await fireKill(); throw new ToolAbortError("Command aborted"); } @@ -674,16 +688,24 @@ export class BashTool implements AgentTool { const raced = await Promise.race(racers); if (raced.kind === "aborted" || signal?.aborted) { - await handle.kill(); + await fireKill(); throw new ToolAbortError("Command aborted"); } if (raced.kind === "timeout") { + // Kill before reading final output so a slow `terminal/output` + // RPC cannot let a timed-out command keep running past the + // enforced timeout. The handle stays valid post-kill so the + // buffered output is still readable. + await fireKill(); let current = { output: "", truncated: false }; try { current = await handle.currentOutput(); - } finally { - await handle.kill(); + } catch (error) { + logger.warn("ACP terminal final output read failed", { + terminalId: handle.terminalId, + error, + }); } const timedOutResult: BashInteractiveResult = { output: current.output, @@ -709,9 +731,19 @@ export class BashTool implements AgentTool { } // Poll tick: push current output so agent-loop transcript stays consistent. - const current = await handle.currentOutput(); + // Race the read against abort so a stuck `terminal/output` RPC does not + // delay cancellation. + const pollOutput = await Promise.race([ + handle.currentOutput(), + abortedP.then(() => undefined as ClientBridgeTerminalOutput | undefined), + ]); + if (pollOutput === undefined) { + // Abort fired during the poll-tick read; let the next loop iteration + // observe `signal?.aborted` and exit via the abort branch. + continue; + } onUpdate?.({ - content: [{ type: "text", text: current.output }], + content: [{ type: "text", text: pollOutput.output }], details: { terminalId: handle.terminalId }, }); } diff --git a/packages/coding-agent/test/acp-agent.test.ts b/packages/coding-agent/test/acp-agent.test.ts index 115f7d3af..ee169311a 100644 --- a/packages/coding-agent/test/acp-agent.test.ts +++ b/packages/coding-agent/test/acp-agent.test.ts @@ -128,6 +128,10 @@ class FakeAgentSession { this.thinkingLevel = level; } + setSlashCommands(_commands: unknown[]): void { + // no-op for tests + } + async setModel(model: Model): Promise { this.model = model; } @@ -382,6 +386,14 @@ describe("ACP agent", () => { configId: "thinking", value: "high", }); + // Both model and thinking-level changes must surface as ACP + // `config_option_update` notifications scoped to the right session; + // the schema check alone would still pass if either method stopped + // emitting notifications entirely. + const configUpdatesForFirst = harness.updates.filter( + n => n.sessionId === first.sessionId && n.update.sessionUpdate === "config_option_update", + ); + expect(configUpdatesForFirst.length).toBeGreaterThanOrEqual(2); expectAcpNotifications(harness.updates); const firstSession = harness.findSession(first.sessionId); diff --git a/packages/coding-agent/test/acp-builtins.test.ts b/packages/coding-agent/test/acp-builtins.test.ts index 95c4ec0bd..b7e14a1b4 100644 --- a/packages/coding-agent/test/acp-builtins.test.ts +++ b/packages/coding-agent/test/acp-builtins.test.ts @@ -142,7 +142,9 @@ function createRuntime() { output.push(text); }, refreshCommands: () => {}, + reloadPlugins: async () => {}, notifyTitleChanged: undefined as (() => Promise | void) | undefined, + notifyConfigChanged: undefined as (() => Promise | void) | undefined, }, }; } @@ -276,6 +278,41 @@ describe("ACP builtin slash commands", () => { expect(output[0]?.toLowerCase()).toContain("acp"); }); + it("model: applies known id and emits both title + config change notifications", async () => { + const { output, runtime, session } = createRuntime(); + const available = [{ provider: "anthropic", id: "claude-3-5-sonnet", contextWindow: 200_000 }]; + session.getAvailableModels = () => available; + let titleNotified = 0; + let configNotified = 0; + runtime.notifyTitleChanged = () => { + titleNotified++; + }; + runtime.notifyConfigChanged = () => { + configNotified++; + }; + const setModelSpy = spyOn(session, "setModel").mockResolvedValue(undefined); + + const result = await executeAcpBuiltinSlashCommand("/model claude-3-5-sonnet", runtime); + + expect(result).toEqual({ consumed: true }); + expect(setModelSpy).toHaveBeenCalledWith(available[0]); + expect(output[0]).toContain("Model set to anthropic/claude-3-5-sonnet"); + expect(titleNotified).toBe(1); + expect(configNotified).toBe(1); + }); + + it("model: does not emit config change when id is unknown", async () => { + const { runtime } = createRuntime(); + let configNotified = 0; + runtime.notifyConfigChanged = () => { + configNotified++; + }; + + await executeAcpBuiltinSlashCommand("/model nonexistent", runtime); + + expect(configNotified).toBe(0); + }); + // Removed TUI-only and dropped commands fall through as false it("removed commands return false (fall through to model)", async () => { const removedCommands = [ @@ -705,6 +742,17 @@ describe("wave 5 — adapters and polish", () => { expect(result).toEqual({ consumed: true }); expect(output[0]).toContain('Added MCP server "foo" (project).'); expect(spy).toHaveBeenCalledTimes(1); + // Lock in the parsed call shape so future regressions in + // `--url` / `--token` / `--scope` parsing fail this test instead of + // silently writing a different config. + const [configPath, serverName, serverConfig] = spy.mock.calls[0]!; + expect(configPath).toContain("project"); + expect(serverName).toBe("foo"); + expect(serverConfig).toMatchObject({ + type: "http", + url: "https://example.com", + headers: { Authorization: "Bearer X" }, + }); } finally { spy.mockRestore(); } @@ -728,6 +776,13 @@ describe("wave 5 — adapters and polish", () => { const result = await executeAcpBuiltinSlashCommand("/ssh add foo --host x --user y --scope user", runtime); expect(result).toEqual({ consumed: true }); expect(output[0]).toContain('Added SSH host "foo" (user).'); + // Without this assertion, the command could succeed via a side-effect-free + // path that prints the success message without writing the host config. + expect(spy).toHaveBeenCalledTimes(1); + const [configPath, name, hostConfig] = spy.mock.calls[0]!; + expect(typeof configPath).toBe("string"); + expect(name).toBe("foo"); + expect(hostConfig).toMatchObject({ host: "x", username: "y" }); } finally { spy.mockRestore(); } diff --git a/packages/coding-agent/test/acp-stdout-hygiene.test.ts b/packages/coding-agent/test/acp-stdout-hygiene.test.ts index 8f4405f47..84a8a484e 100644 --- a/packages/coding-agent/test/acp-stdout-hygiene.test.ts +++ b/packages/coding-agent/test/acp-stdout-hygiene.test.ts @@ -85,6 +85,26 @@ describe("ACP stdout hygiene", () => { proc.stdin.write(new TextEncoder().encode(`${JSON.stringify(initRequest)}\n`)); proc.stdin.flush(); + // Capture stderr in parallel so we can verify it does not carry any + // JSON-RPC frame. ACP owns stdout; banners, progress text, or stray + // protocol bytes on stderr indicate a misroute. + const stderrChunks: Uint8Array[] = []; + const stderrPump = (async () => { + const reader = (proc.stderr as ReadableStream).getReader(); + try { + while (true) { + const { value, done } = await reader.read(); + if (done) break; + if (value) stderrChunks.push(value); + // Stop once the first stdout frame arrives so the pump terminates + // alongside the test rather than waiting for process exit. + if (stderrChunks.length > 32) break; + } + } finally { + reader.releaseLock(); + } + })(); + const firstLine = await readFirstFrame(proc.stdout as ReadableStream); expect(firstLine.length).toBeGreaterThan(0); expect(firstLine[0]).toBe("{"); @@ -105,5 +125,29 @@ describe("ACP stdout hygiene", () => { expect.objectContaining({ type: "terminal", id: "terminal" }), ]), ); + + // Terminate the process so the stderr pump promise resolves. Race with a + // short timeout in case stderr is empty (common path). + try { + proc.kill(); + } catch { + // process may already be exiting + } + await Promise.race([stderrPump, new Promise(resolve => setTimeout(resolve, 500))]); + const stderrText = new TextDecoder().decode(new Uint8Array(stderrChunks.flatMap(chunk => Array.from(chunk)))); + // Guard against JSON-RPC frames sneaking onto stderr. We allow normal + // stderr output (warnings, telemetry, etc.) but reject anything that + // parses as a JSON-RPC envelope on the wrong channel. + for (const line of stderrText.split("\n")) { + const trimmed = line.trim(); + if (!trimmed.startsWith("{")) continue; + let parsed: { jsonrpc?: unknown } | undefined; + try { + parsed = JSON.parse(trimmed) as { jsonrpc?: unknown }; + } catch { + continue; + } + expect(parsed?.jsonrpc, `JSON-RPC frame leaked to stderr: ${trimmed}`).toBeUndefined(); + } }, 20_000); });