smashimo/oh-my-pi
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(coding-agent/acp): tighten ACP conformance per review feedback

Browse Source 
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
This commit is contained in:
Ogrodev
2026-05-12 16:45:33 -03:00
committed by can1357
parent 1a44cd2e36
commit 4e4e74be49
23 changed files with 431 additions and 81 deletions
Download Patch File Download Diff File Expand all files Collapse all files
packages/coding-agent/CHANGELOG.md
+20 -2
View File
@@ -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/<endpoint> -f q=… -F per_page=…` directly so the qualifiers reach GitHub's search API verbatim. `is:issue`/`is:pr` and `repo:<owner>/<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 <path>` 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:<name>` 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 <scheme>:// 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://<N>/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 <path>: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:<name>` 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
packages/coding-agent/src/edit/modes/patch.ts
+5 -1
View File
@@ -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,
packages/coding-agent/src/modes/acp/acp-agent.ts
+74 -15
View File
@@ -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<AuthenticateResponse> {
async authenticate(params: AuthenticateRequest): Promise<AuthenticateResponse> {
// 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<void> {
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<void> {
const sessionId = record.session.sessionId;
@@ -1217,14 +1274,15 @@ export class AcpAgent implements Agent {
}
async #replaySessionHistory(record: ManagedSessionRecord): Promise<void> {
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<Pick<ReplayableMessage, "toolCallId" | "toolName">> & 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 }),
];
}
packages/coding-agent/src/modes/acp/acp-event-mapper.ts
+45 -23
View File
@@ -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<PathContainer>(args, "path");
if (path) {
const seen = new Set<string>();
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<OldPathContainer>(args, "oldPath");
if (oldPath && oldPath !== path) {
locations.push({ path: oldPath });
}
const newPath = extractStringProperty<NewPathContainer>(args, "newPath");
if (newPath && newPath !== path && newPath !== oldPath) {
locations.push({ path: newPath });
}
pushPath(extractStringProperty<PathContainer>(args, "path"));
pushPath(extractStringProperty<OldPathContainer>(args, "oldPath"));
pushPath(extractStringProperty<NewPathContainer>(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<PathContainer>(entry, "path");
if (path && !seen.has(path)) {
seen.add(path);
locations.push({ path });
}
const raw = extractStringProperty<PathContainer>(entry, "path");
if (!raw) continue;
const path = toAcpLocationPath(raw, cwd);
if (seen.has(path)) continue;
seen.add(path);
locations.push({ path });
}
return locations;
}
packages/coding-agent/src/session/agent-session.ts
+15 -5
View File
@@ -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<string, unknown>;
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<T extends AgentTool>(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,
packages/coding-agent/src/slash-commands/acp-builtins/browser.ts
+10 -1
View File
@@ -19,7 +19,16 @@ export const browserCommand: AcpBuiltinCommandSpec = {
runtime.settings.set("browser.headless" as SettingPath, next as SettingValue<SettingPath>);
const tool = runtime.session.getToolByName("browser");
if (tool && "restartForModeChange" in tool) {
await (tool as { restartForModeChange: () => Promise<void> }).restartForModeChange();
try {
await (tool as { restartForModeChange: () => Promise<void> }).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();
packages/coding-agent/src/slash-commands/acp-builtins/compact.ts
+9 -2
View File
@@ -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) {
packages/coding-agent/src/slash-commands/acp-builtins/export.ts
+9 -1
View File
@@ -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) {
packages/coding-agent/src/slash-commands/acp-builtins/force.ts
+6 -2
View File
@@ -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:<tool-name> [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();
},
packages/coding-agent/src/slash-commands/acp-builtins/marketplace.ts
+4
View File
@@ -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] <name@marketplace>");
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")}`);
}
packages/coding-agent/src/slash-commands/acp-builtins/mcp.ts
+36 -3
View File
@@ -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<T>(
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}]`;
})
packages/coding-agent/src/slash-commands/acp-builtins/model.ts
+1
View File
@@ -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);
packages/coding-agent/src/slash-commands/acp-builtins/move.ts
+9 -2
View File
@@ -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();
packages/coding-agent/src/slash-commands/acp-builtins/plugins.ts
+1
View File
@@ -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();
}
packages/coding-agent/src/slash-commands/acp-builtins/reload-plugins.ts
+1 -1
View File
@@ -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();
},
packages/coding-agent/src/slash-commands/acp-builtins/session.ts
+9 -7
View File
@@ -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.`,
);
packages/coding-agent/src/slash-commands/acp-builtins/shared.ts
+2 -2
View File
@@ -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<AcpBuiltinSlashCommandResult> {
await runtime.output(text);
return commandConsumed();
}
packages/coding-agent/src/slash-commands/acp-builtins/ssh.ts
+15 -5
View File
@@ -40,8 +40,14 @@ const SSH_ADD_OPTION_PARSERS = new Map<string, SshAddOptionParser>([
"--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) {
packages/coding-agent/src/slash-commands/acp-builtins/types.ts
+8
View File
@@ -15,7 +15,15 @@ export interface AcpBuiltinCommandRuntime {
cwd: string;
output: (text: string) => Promise<void> | void;
refreshCommands: () => Promise<void> | 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<void>;
notifyTitleChanged?: () => Promise<void> | void;
notifyConfigChanged?: () => Promise<void> | void;
}
export type AcpBuiltinSlashCommandResult = false | { consumed: true } | { prompt: string };
packages/coding-agent/src/tools/bash.ts
+41 -9
View File
@@ -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<BashToolSchema, BashToolDetails> {
| { 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<void>();
const onAbortSignal = () => resolveAborted();
let killStarted = false;
const fireKill = (): Promise<void> => {
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<BashToolSchema, BashToolDetails> {
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<BashToolSchema, BashToolDetails> {
}
// 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 },
});
}
packages/coding-agent/test/acp-agent.test.ts
+12
View File
@@ -128,6 +128,10 @@ class FakeAgentSession {
this.thinkingLevel = level;
}
setSlashCommands(_commands: unknown[]): void {
// no-op for tests
}
async setModel(model: Model): Promise<void> {
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);
packages/coding-agent/test/acp-builtins.test.ts
+55
View File
@@ -142,7 +142,9 @@ function createRuntime() {
output.push(text);
},
refreshCommands: () => {},
reloadPlugins: async () => {},
notifyTitleChanged: undefined as (() => Promise<void> | void) | undefined,
notifyConfigChanged: undefined as (() => Promise<void> | 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();
}
packages/coding-agent/test/acp-stdout-hygiene.test.ts
+44
View File
@@ -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<Uint8Array>).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<Uint8Array>);
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);
});