Browse Source

fix: harden ACP agent bridge

alvinreal 1 month ago
parent
commit
4f060372d0
7 changed files with 175 additions and 34 deletions
  1. 10 1
      .slim/clonedeps.json
  2. 1 0
      AGENTS.md
  3. 4 0
      docs/configuration.md
  4. 18 0
      src/agents/custom.test.ts
  5. 22 3
      src/agents/index.ts
  6. 7 4
      src/index.ts
  7. 113 26
      src/tools/acp-run.ts

+ 10 - 1
.slim/clonedeps.json

@@ -1,6 +1,6 @@
 {
   "version": "1.0.0",
-  "updatedAt": "2026-05-25T00:00:00.000Z",
+  "updatedAt": "2026-06-15T00:00:00.000Z",
   "dependencies": [
     {
       "name": "@opencode-ai/plugin",
@@ -37,6 +37,15 @@
       "path": ".slim/clonedeps/repos/opencode",
       "packagePath": "packages/opencode",
       "reason": "Latest OpenCode TypeScript runtime source with experimental background subagent support."
+    },
+    {
+      "name": "agentclientprotocol/agent-client-protocol",
+      "resolvedVersion": "main",
+      "repoUrl": "https://github.com/agentclientprotocol/agent-client-protocol.git",
+      "ref": "main@8110fde4e8283b4bef1329d1ef7b074fd14cee1e",
+      "path": ".slim/clonedeps/repos/agentclientprotocol__agent-client-protocol",
+      "packagePath": ".",
+      "reason": "Authoritative ACP specification and schema source for implementing ACP client/server compatibility."
     }
   ]
 }

+ 1 - 0
AGENTS.md

@@ -284,3 +284,4 @@ Read-only dependency source repositories are available under
 - `.slim/clonedeps/repos/opencode-ai__opencode/` — `https://github.com/opencode-ai/opencode.git` at `main@73ee493265acf15fcd8caab2bc8cd3bd375b63cb`; inspect `packages/plugin` and `packages/sdk/js` for OpenCode plugin and SDK internals.
 - `.slim/clonedeps/repos/opencode/` — `https://github.com/anomalyco/opencode.git` at `dev@356f6841865d68adf6d0123c37357ad50814497a`; inspect `packages/opencode` for latest TypeScript runtime internals and experimental background subagent support.
 - `.slim/clonedeps/repos/modelcontextprotocol__typescript-sdk/` — `https://github.com/modelcontextprotocol/typescript-sdk.git` at `v1.29.0@e12cbd7078db388152f6e839abdbe09ba01f3f32`; inspect it for MCP protocol and server integration internals.
+- `.slim/clonedeps/repos/agentclientprotocol__agent-client-protocol/` — `https://github.com/agentclientprotocol/agent-client-protocol.git` at `main@8110fde4e8283b4bef1329d1ef7b074fd14cee1e`; inspect it for ACP protocol specification and schema details.

+ 4 - 0
docs/configuration.md

@@ -108,7 +108,10 @@ Presets can also be switched at runtime without restarting using the `/preset` c
 | `acpAgents.<name>.command` | string | — | Command for an external ACP-compatible agent; creates a wrapper subagent named `<name>` |
 | `acpAgents.<name>.args` | string[] | `[]` | Arguments for the ACP agent command |
 | `acpAgents.<name>.env` | object | `{}` | Extra environment variables for the ACP subprocess |
+| `acpAgents.<name>.cwd` | string | session directory | Working directory override for this ACP subprocess; protocol paths should be absolute |
 | `acpAgents.<name>.description` | string | — | Description shown to OpenCode and injected into the orchestrator routing prompt |
+| `acpAgents.<name>.prompt` | string | generated wrapper prompt | Optional full prompt for the lightweight wrapper subagent |
+| `acpAgents.<name>.orchestratorPrompt` | string | generated routing block | Optional exact routing block injected into the orchestrator prompt |
 | `acpAgents.<name>.wrapperModel` | string | fixer default | Cheap OpenCode model used by the wrapper subagent that calls `acp_run` |
 | `acpAgents.<name>.permissionMode` | string | `ask` | How ACP permission requests are handled: `ask`, `allow`, or `reject` |
 | `acpAgents.<name>.timeoutMs` | integer | `300000` | Timeout for a single ACP run in milliseconds |
@@ -166,6 +169,7 @@ Use `acpAgents` to expose external Agent Client Protocol servers as optional
 OpenCode subagents. The plugin creates a lightweight wrapper agent for each
 entry. The wrapper calls the built-in `acp_run` tool, which starts the ACP
 process, creates a session, sends the task, and returns the streamed result.
+`command` is only the executable; put flags and subcommands in `args`.
 
 ```jsonc
 {

+ 18 - 0
src/agents/custom.test.ts

@@ -172,4 +172,22 @@ describe('custom-agent creation', () => {
       "ACP agent 'bridge' conflicts with a custom agent of the same name",
     );
   });
+
+  test('rejects acpAgents that conflict with built-in agents', () => {
+    const config: PluginConfig = {
+      acpAgents: {
+        fixer: {
+          command: 'fixer-acp',
+          args: [],
+          env: {},
+          timeoutMs: 300000,
+          permissionMode: 'ask',
+        },
+      },
+    };
+
+    expect(() => createAgents(config)).toThrow(
+      "ACP agent 'fixer' conflicts with a built-in agent name or alias",
+    );
+  });
 });

+ 22 - 3
src/agents/index.ts

@@ -1,6 +1,7 @@
 import type { AgentConfig as SDKAgentConfig } from '@opencode-ai/sdk/v2';
 import { getSkillPermissionsForAgent } from '../cli/skills';
 import {
+  AGENT_ALIASES,
   type AgentOverrideConfig,
   ALL_AGENT_NAMES,
   DEFAULT_DISABLED_AGENTS,
@@ -74,6 +75,16 @@ function buildAcpAgentDefinition(
       temperature: 0,
       prompt,
       permission: {
+        read: 'deny',
+        edit: 'deny',
+        bash: 'deny',
+        task: 'deny',
+        glob: 'deny',
+        grep: 'deny',
+        list: 'deny',
+        webfetch: 'deny',
+        question: 'deny',
+        skill: 'deny',
         acp_run: 'allow',
       },
     },
@@ -339,8 +350,15 @@ export function createAgents(config?: PluginConfig): AgentDefinition[] {
     .map(normalizeCustomAgentName)
     .filter((name) => name.length > 0)
     .filter((name) => {
-      if (!isSafeCustomAgentName(name)) {
-        throw new Error(`Unsafe ACP agent name '${name}'`);
+      if (!SAFE_AGENT_ALIAS_RE.test(name)) {
+        throw new Error(
+          `ACP agent name '${name}' must match /^[a-z][a-z0-9_-]*$/i`,
+        );
+      }
+      if (isKnownAgentName(name) || AGENT_ALIASES[name] !== undefined) {
+        throw new Error(
+          `ACP agent '${name}' conflicts with a built-in agent name or alias`,
+        );
       }
       if (customAgentNames.includes(name)) {
         throw new Error(
@@ -470,7 +488,8 @@ export function createAgents(config?: PluginConfig): AgentDefinition[] {
   for (const displayName of usedDisplayNames) {
     if (
       (ALL_AGENT_NAMES as readonly string[]).includes(displayName) ||
-      customAgentNames.includes(displayName)
+      customAgentNames.includes(displayName) ||
+      acpAgentNames.includes(displayName)
     ) {
       throw new Error(
         `displayName '${displayName}' conflicts with an agent name`,

+ 7 - 4
src/index.ts

@@ -146,7 +146,7 @@ const OhMyOpenCodeLite: Plugin = async (ctx) => {
   let companionManager: CompanionManager;
   let councilTools: Record<string, unknown>;
   let cancelTaskTools: Record<string, unknown>;
-  let acpRunTool: ReturnType<typeof createAcpRunTool>;
+  let acpRunTools: Record<string, ReturnType<typeof createAcpRunTool>>;
   let webfetch: ReturnType<typeof createWebfetchTool>;
   let rewriteDisplayNameMentions: ReturnType<
     typeof createDisplayNameMentionRewriter
@@ -233,7 +233,10 @@ const OhMyOpenCodeLite: Plugin = async (ctx) => {
       : {};
 
     mcps = createBuiltinMcps(config.disabled_mcps, config.websearch);
-    acpRunTool = createAcpRunTool(config.acpAgents);
+    acpRunTools =
+      Object.keys(config.acpAgents ?? {}).length > 0
+        ? { acp_run: createAcpRunTool(config.acpAgents) }
+        : {};
     webfetch = createWebfetchTool(ctx);
     backgroundJobBoard = new BackgroundJobBoard({
       maxReusablePerAgent: config.backgroundJobs?.maxSessionsPerAgent ?? 2,
@@ -313,7 +316,7 @@ const OhMyOpenCodeLite: Plugin = async (ctx) => {
     toolCount =
       Object.keys(councilTools).length +
       Object.keys(cancelTaskTools).length +
-      1 + // acp_run
+      Object.keys(acpRunTools).length +
       1 + // webfetch
       2; // ast_grep_search, ast_grep_replace
   } catch (err) {
@@ -382,7 +385,7 @@ const OhMyOpenCodeLite: Plugin = async (ctx) => {
     tool: {
       ...councilTools,
       ...cancelTaskTools,
-      acp_run: acpRunTool,
+      ...acpRunTools,
       webfetch,
       ast_grep_search,
       ast_grep_replace,

+ 113 - 26
src/tools/acp-run.ts

@@ -10,7 +10,7 @@ type Json = null | boolean | number | string | Json[] | { [key: string]: Json };
 interface RpcResponse {
   id: number;
   result?: Json;
-  error?: { message?: string };
+  error?: { code?: number; message?: string; data?: Json };
 }
 
 interface RpcRequest {
@@ -35,6 +35,10 @@ class AcpClient {
   private pending = new Map<number, Pending>();
   private chunks: string[] = [];
   private errors: string[] = [];
+  private sessionId: string | undefined;
+  private lastUpdate = Date.now();
+  private authMethods: Array<Record<string, unknown>> = [];
+  private active = false;
 
   constructor(
     private name: string,
@@ -74,7 +78,7 @@ class AcpClient {
   }
 
   async run(prompt: string): Promise<string> {
-    await this.request('initialize', {
+    const init = await this.request('initialize', {
       protocolVersion: 1,
       clientCapabilities: {},
       clientInfo: {
@@ -82,19 +86,42 @@ class AcpClient {
         title: 'oh-my-opencode-slim ACP bridge',
       },
     });
-    const created = await this.request('session/new', {
-      cwd: this.cwd,
-      mcpServers: [],
-    });
+    this.authMethods = readAuthMethods(init);
+    const created = await this.newSession();
     const sessionId = readSessionId(created);
+    this.sessionId = sessionId;
+    this.active = true;
     await this.request('session/prompt', {
       sessionId,
       prompt: [{ type: 'text', text: prompt }],
     });
+    await this.drain();
+    this.active = false;
     return this.output();
   }
 
+  private async newSession(): Promise<Json | undefined> {
+    try {
+      return await this.request('session/new', {
+        cwd: this.cwd,
+        mcpServers: [],
+      });
+    } catch (error) {
+      if (!isAuthError(error) || this.authMethods.length === 0) throw error;
+      const method = this.authMethods[0];
+      if (typeof method.id !== 'string') throw error;
+      await this.request('authenticate', { methodId: method.id });
+      return await this.request('session/new', {
+        cwd: this.cwd,
+        mcpServers: [],
+      });
+    }
+  }
+
   close(): void {
+    if (this.active && this.sessionId && !this.child.killed) {
+      this.notify('session/cancel', { sessionId: this.sessionId });
+    }
     if (!this.child.killed) this.child.kill('SIGTERM');
   }
 
@@ -114,6 +141,18 @@ class AcpClient {
     });
   }
 
+  private notify(method: string, params: Record<string, unknown>): void {
+    this.child.stdin.write(
+      `${JSON.stringify({ jsonrpc: '2.0', method, params })}\n`,
+    );
+  }
+
+  private async drain(): Promise<void> {
+    while (Date.now() - this.lastUpdate < 100) {
+      await new Promise((resolve) => setTimeout(resolve, 25));
+    }
+  }
+
   private async receive(line: string): Promise<void> {
     if (!line.trim()) return;
     const message = JSON.parse(line) as
@@ -125,9 +164,7 @@ class AcpClient {
       if (!pending) return;
       this.pending.delete(message.id);
       if (message.error) {
-        pending.reject(
-          new Error(message.error.message ?? 'ACP request failed'),
-        );
+        pending.reject(rpcError(message.error));
         return;
       }
       pending.resolve(message.result);
@@ -143,14 +180,29 @@ class AcpClient {
   private async handleRequest(message: RpcRequest): Promise<void> {
     if (message.method === 'session/request_permission') {
       const title = readPermissionTitle(message.params);
-      if (this.config.permissionMode === 'ask') {
-        await this.ask(title, message.params ?? {});
+      try {
+        if (this.config.permissionMode === 'ask') {
+          await this.ask(title, message.params ?? {});
+        }
+        const optionId = selectPermissionOption(
+          message.params,
+          this.config.permissionMode,
+        );
+        if (!optionId)
+          throw new Error('ACP permission request had no usable option');
+        this.reply(message.id, {
+          outcome: { outcome: 'selected', optionId },
+        });
+      } catch {
+        const optionId = selectPermissionOption(message.params, 'reject');
+        if (optionId) {
+          this.reply(message.id, {
+            outcome: { outcome: 'selected', optionId },
+          });
+          return;
+        }
+        this.reply(message.id, { outcome: { outcome: 'cancelled' } });
       }
-      const optionId = selectPermissionOption(
-        message.params,
-        this.config.permissionMode,
-      );
-      this.reply(message.id, { outcome: { outcome: 'selected', optionId } });
       return;
     }
     this.replyError(
@@ -161,6 +213,7 @@ class AcpClient {
 
   private handleNotification(message: RpcNotification): void {
     if (message.method !== 'session/update') return;
+    this.lastUpdate = Date.now();
     const update = message.params?.update;
     if (!isRecord(update)) return;
     collectText(update, this.chunks);
@@ -208,6 +261,11 @@ export function createAcpRunTool(agents: AcpAgentsConfig = {}): ToolDefinition {
         .describe('Optional timeout override in milliseconds'),
     },
     async execute(args, ctx) {
+      if (ctx.agent !== args.agent) {
+        throw new Error(
+          `acp_run for '${args.agent}' can only be used by @${args.agent}`,
+        );
+      }
       const config = agents[args.agent];
       if (!config) {
         throw new Error(
@@ -277,6 +335,34 @@ function readSessionId(value: Json | undefined): string {
   return value.sessionId;
 }
 
+function readAuthMethods(
+  value: Json | undefined,
+): Array<Record<string, unknown>> {
+  if (!isRecord(value) || !Array.isArray(value.authMethods)) return [];
+  const methods: unknown[] = value.authMethods;
+  return methods.filter(isRecord);
+}
+
+function rpcError(error: NonNullable<RpcResponse['error']>): Error {
+  const err = new Error(error.message ?? 'ACP request failed') as Error & {
+    code?: number;
+    data?: Json;
+  };
+  err.code = error.code;
+  err.data = error.data;
+  return err;
+}
+
+function isAuthError(error: unknown): boolean {
+  if (!(error instanceof Error)) return false;
+  const meta = error as Error & { code?: number; data?: Json };
+  return (
+    meta.code === -32001 ||
+    error.message.toLowerCase().includes('auth_required') ||
+    error.message.toLowerCase().includes('auth required')
+  );
+}
+
 function isRecord(value: unknown): value is Record<string, unknown> {
   return typeof value === 'object' && value !== null && !Array.isArray(value);
 }
@@ -293,22 +379,23 @@ function readPermissionTitle(
 function selectPermissionOption(
   params: Record<string, unknown> | undefined,
   mode: AcpAgentConfig['permissionMode'],
-): string {
+): string | undefined {
   const options = Array.isArray(params?.options) ? params.options : [];
-  const ids = options
+  const choices = options
     .filter(isRecord)
-    .map((item) => item.optionId)
-    .filter((item): item is string => typeof item === 'string');
-  if (mode === 'reject')
-    return ids.find((id) => id.includes('reject')) ?? 'reject';
-  return (
-    ids.find((id) => id.includes('allow') || id === 'once') ??
-    ids.find((id) => !id.includes('reject')) ??
-    'allow'
+    .filter((item) => typeof item.optionId === 'string');
+  const reject = choices.find(
+    (item) => typeof item.kind === 'string' && item.kind.startsWith('reject'),
+  );
+  if (mode === 'reject') return reject?.optionId as string | undefined;
+  const allow = choices.find(
+    (item) => typeof item.kind === 'string' && item.kind.startsWith('allow'),
   );
+  return (allow?.optionId ?? reject?.optionId) as string | undefined;
 }
 
 function collectText(update: Record<string, unknown>, chunks: string[]): void {
+  if (update.sessionUpdate !== 'agent_message_chunk') return;
   for (const key of ['content', 'delta']) {
     const value = update[key];
     if (typeof value === 'string') chunks.push(value);