feat(acp): Add ACP protocol support for OpenCode integration #85
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Implement ACP client for standardized communication with OpenCode agent. This provides a cleaner alternative to the custom JSON format parsing. New files: - src/main/acp/types.ts - ACP protocol type definitions - src/main/acp/acp-client.ts - JSON-RPC client for ACP agents - src/main/acp/acp-adapter.ts - Convert ACP events to ParsedEvent format - src/main/acp/index.ts - Module exports Tests: - src/__tests__/integration/acp-opencode.integration.test.ts - Connect and initialize with OpenCode ACP - Create new sessions - Send prompts and receive streaming updates - Adapter converts ACP events to Maestro's internal format Key features: - JSON-RPC 2.0 over stdio - Session management (new/load) - Streaming message chunks - Tool call support - Permission request handling - Terminal and file system methods All 8 ACP integration tests pass with OpenCode v1.0.190.
Add 'Use ACP Protocol (Experimental)' checkbox in OpenCode configuration: - agent-detector.ts: Add useACP config option (default: false) - agent-capabilities.ts: Add supportsACP capability flag When enabled, Maestro will use the ACP client (JSON-RPC over stdio) instead of the standard 'opencode run --format json' interface. This is behind a feature flag because: 1. ACP is still experimental 2. Some features may behave differently 3. Users can choose their preferred interface OpenCode is the only agent with native ACP support (supportsACP: true). Claude Code and Codex have ACP adapters available but not integrated yet.
Complete ACP integration so the 'Use ACP Protocol' checkbox in OpenCode settings actually switches between modes: - OFF (default): Standard mode - 'opencode run --format json' + stdout parsing - ON: ACP mode - 'opencode acp' + JSON-RPC over stdio Changes: - process-manager.ts: Add ACP process spawning alongside PTY/child process - New ACPProcess type in ManagedProcess - spawn() checks useACP flag and creates ACPProcess - write/interrupt/kill methods handle ACP processes - ipc/handlers/process.ts: Read useACP from agent config and pass to spawn - acp/acp-process.ts: New ACPProcess wrapper that emits ProcessManager events The ACPProcess: - Connects to OpenCode via ACP protocol - Creates/resumes sessions - Sends prompts and streams responses - Auto-approves permission requests (YOLO mode) - Handles file system read/write requests from agent All 8 ACP integration tests pass with OpenCode v1.0.190.
Fix type mismatches between ACP adapter and ParsedEvent interface: - Use valid ParsedEvent types (init, text, tool_use, result, error, usage, system) - Add required 'raw' field to all ParsedEvent returns - Map thinking chunks to 'text' type with [thinking] prefix - Map plan updates to 'system' type - Use toolState object instead of custom properties - Remove unused imports and variables
Add 7 new ACP provider tests that replace legacy stdout JSON format:
- should send initial message and receive session ID via ACP
- should resume session with follow-up message via ACP
- should stream text updates via ACP
- should handle tool calls via ACP
- should convert ACP updates to ParsedEvent format
- should handle multiple sessions via ACP
- should report available modes via ACP
Also fix content format handling in:
- acp-adapter.ts: Handle both ACP spec and OpenCode's actual format
- Tests: Handle { type: 'text', text: '...' } format from OpenCode
All 15 ACP OpenCode tests pass (3 connection + 5 adapter + 7 provider).
Fix issue where checkbox settings (like useACP) weren't being persisted because React state updates are async. The blur handler was reading stale state. Solution: Pass the new config directly to onConfigBlur for checkbox changes, bypassing the async state update.
Add a new debug menu entry to view ACP protocol communication: - Shows the ACP server initialization command - Displays all inbound/outbound JSON-RPC messages - Filter by direction (all/inbound/outbound) - Click to expand individual message payloads - Stats showing message counts by type - Clear log button to reset Access via Quick Actions (Cmd+K) → type 'debug' → 'Debug: ACP Protocol Log'
When spawning from Electron, the PATH doesn't include user paths like Homebrew, nvm, volta, etc. This caused 'env: node: No such file or directory' errors when OpenCode's ACP mode tried to use node. Now the ACP client extends PATH with: - /opt/homebrew/bin (Homebrew on Apple Silicon) - /usr/local/bin (Homebrew on Intel) - ~/.nvm/versions/node/vX.X.X/bin (nvm default version) - ~/.volta/bin (Volta) - ~/.fnm (fnm) - Standard system paths
The renderer expects string data from process:data events, but ACP was sending ParsedEvent objects, resulting in '[object Object]' display. Now the ProcessManager converts ACP events properly: - text events → emit text content directly - result events → emit result text - init events with sessionId → emit session-id event - other events → emit as JSON string Also added debug logging to ACP adapter to help diagnose issues.
Add new checkbox option 'Show Streaming Output (ACP)' next to 'Use ACP Protocol': - Default: disabled (shows only final response, avoids duplicate text) - Enabled: shows text as it streams from the agent (may cause duplicates) This gives users control over whether to see real-time streaming output or wait for the complete response in ACP mode.
Fixed two issues in ACP mode: 1. Result JSON was being displayed raw to users 2. Text was duplicated when streaming was enabled Now: - Streaming disabled: only result text is emitted (once) - Streaming enabled: only streaming chunks emitted, result ignored - Empty results and internal events (usage, etc.) are not shown
Ensures accumulated text from previous prompts doesn't carry over.
Removed ACP Debug Log modal and all related code: - ACPDebugModal.tsx component - ACP debug IPC handlers - ACP debug preload API - Quick action menu entry - Debug logging to in-memory buffer (acpDebugLog) ACP protocol messages are still logged to system logs at debug level.
Fixed two issues with ACP mode: 1. Disabled synopsis toasts that were appearing on every message completion - Synopsis feature needs rework for ACP compatibility - Prevented 'Ungrouped' and 'Synopsis' toast spam 2. Fixed message accumulation/duplication - OpenCode's ACP sends cumulative text in each chunk - Now track emitted text length and only emit deltas - Prevents previous messages from repeating
Added detailed transport-level logging for all ACP JSON-RPC messages: - [ACP Transport] log category in system logs - INBOUND: requests, responses, notifications - OUTBOUND: requests, responses, notifications, error responses - Full message data logged for debugging This will help diagnose the message accumulation issue by showing exactly what OpenCode is sending vs what we're processing.
ACPProcess now returns the actual OpenCode process PID instead of -1: - Added getProcess() method to ACPClient to expose child process - ACPProcess.pid getter now returns actual PID from spawned process - Process Monitor can now display and kill ACP server processes Users can now see the OpenCode process in Process Monitor with real PID and terminate it if needed (e.g., hung processes).
…n load - Add isLoadingSession flag to ignore session/update notifications during session/load - Historical messages from OpenCode were being processed as new streaming content - Update createResultEvent to accept and forward usage stats from ACP responses - Add logging for prompt responses to track usage data availability - Add MAESTRO_LOG_LEVEL env var support for easier debugging (dev:main:debug script) - Fix spawnBackgroundSynopsis to properly pass ACP config for synopsis generation - Add TODO comments for agent session cleanup on Maestro session deletion Note: OpenCode currently doesn't report token usage in ACP responses (usage field is optional per spec)
Contributor
There was a problem hiding this comment.
Pull request overview
This PR implements the Agent Client Protocol (ACP) to enable standardized communication with ACP-compatible agents like OpenCode, replacing custom JSON parsing with a protocol-based approach. The implementation includes bug fixes for message accumulation and improvements to configuration management.
Key Changes:
- Implemented complete ACP protocol stack (client, process wrapper, adapter, types) supporting JSON-RPC 2.0 over stdio
- Added per-agent ACP configuration options (
useACP,acpShowStreaming) with UI controls - Fixed checkbox state synchronization bug in agent config panels by passing immediate config values to async save handlers
- Enhanced debugging capabilities with
MAESTRO_LOG_LEVELenvironment variable support
Reviewed changes
Copilot reviewed 21 out of 22 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/acp/types.ts | Complete TypeScript type definitions for ACP protocol specification |
| src/main/acp/acp-client.ts | JSON-RPC 2.0 client implementation for stdio communication with ACP agents |
| src/main/acp/acp-adapter.ts | Converts ACP session updates to Maestro's ParsedEvent format |
| src/main/acp/acp-process.ts | ProcessManager-compatible wrapper for ACP agents with deduplication logic |
| src/main/acp/index.ts | Module exports for ACP components |
| src/main/process-manager.ts | Added ACP mode support alongside standard stdout parsing |
| src/main/ipc/handlers/process.ts | Added ACP configuration detection and routing logic |
| src/main/agent-capabilities.ts | Marked OpenCode as supporting ACP protocol |
| src/main/agent-detector.ts | Added useACP and acpShowStreaming configuration options for OpenCode |
| src/main/index.ts | Added environment variable override for log level configuration |
| src/renderer/hooks/useAgentExecution.ts | Added async agent config fetch to enable ACP for background synopsis |
| src/renderer/components/shared/AgentConfigPanel.tsx | Fixed checkbox config persistence by accepting immediate config parameter |
| src/renderer/components/Wizard/screens/AgentSelectionScreen.tsx | Updated to use immediate config for checkbox saves |
| src/renderer/components/NewInstanceModal.tsx | Updated to use immediate config for checkbox saves |
| src/renderer/components/EditGroupChatModal.tsx | Updated to use immediate config for checkbox saves |
| src/renderer/components/NewGroupChatModal.tsx | Updated to use immediate config for checkbox saves |
| src/renderer/App.tsx | Added AutoRun folder cleanup and TODO for agent session deletion |
| src/tests/main/agent-capabilities.test.ts | Updated test to include new supportsACP capability |
| src/tests/integration/acp-opencode.integration.test.ts | Comprehensive integration tests for ACP communication with OpenCode |
| package.json | Added dev:debug and dev:main:debug scripts for easier debugging |
| package-lock.json | Version bump to 0.12.0 |
| docs/acp-message-flow.md | Detailed sequence diagram documenting ACP message flow |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| agentConfig: Record<string, any>; | ||
| onConfigChange: (key: string, value: any) => void; | ||
| onConfigBlur: () => void; | ||
| onConfigBlur: (immediateConfig?: Record<string, any>) => void; |
There was a problem hiding this comment.
The callback parameter signature change should be validated against the function implementation. The immediateConfig parameter is optional but the implementation always passes either the provided config or the state/ref. Consider whether this parameter should actually be required since it's always provided by the callers, or if there are other code paths that call this without the parameter.
Suggested change
| onConfigBlur: (immediateConfig?: Record<string, any>) => void; | |
| onConfigBlur: (immediateConfig: Record<string, any>) => void; |
| env: config.customEnvVars, | ||
| clientInfo: { | ||
| name: 'maestro', | ||
| version: '0.12.0', |
There was a problem hiding this comment.
The hardcoded version string '0.12.0' should be dynamically retrieved from package.json or a version constant to avoid version mismatches. This appears in multiple places in the ACP implementation and will need manual updates when the version changes.
| protocolVersion: CURRENT_PROTOCOL_VERSION, | ||
| clientInfo: this.config.clientInfo || { | ||
| name: 'maestro', | ||
| version: '0.12.0', |
There was a problem hiding this comment.
The hardcoded version string '0.12.0' should be dynamically retrieved from package.json or a version constant to avoid version mismatches when the application version changes.
Comment on lines
+358
to
+366
| this.client.on('fs:read', async (request, respond) => { | ||
| try { | ||
| const fs = await import('fs'); | ||
| const content = fs.readFileSync(request.path, 'utf-8'); | ||
| respond({ content }); | ||
| } catch { | ||
| logger.error(`Failed to read file: ${request.path}`, LOG_CONTEXT); | ||
| respond({ content: '' }); | ||
| } |
There was a problem hiding this comment.
The event handler for 'fs:read' swallows errors silently by returning an empty string. This could mask legitimate file access issues. Consider logging the error with more details (e.g., error message, error type) or providing a more informative response to help with debugging when file operations fail.
Comment on lines
+307
to
+319
| * | ||
| * Note: ACP ContentBlock format is { type: 'text', text: 'content' } | ||
| * not { text: { text: 'content' } } as the union type suggests | ||
| */ | ||
| async prompt(sessionId: SessionId, text: string): Promise<PromptResponse> { | ||
| // ACP uses a simpler content block format for text | ||
| const contentBlock = { | ||
| type: 'text', | ||
| text, | ||
| }; | ||
| const request: PromptRequest = { | ||
| sessionId, | ||
| prompt: [contentBlock as unknown as ContentBlock], |
There was a problem hiding this comment.
The comment mentions using a 'simpler content block format for text' and then casts to 'unknown as ContentBlock'. However, the actual object created uses { type: 'text', text } which doesn't match the ContentBlock union type definition. According to the types in acp/types.ts, ContentBlock should be { text: TextContent } where TextContent is { text: string; annotations?: Annotations }. This type mismatch could cause issues. Either update the type definition to match OpenCode's actual format, or adjust the code to match the spec.
Suggested change
| * | |
| * Note: ACP ContentBlock format is { type: 'text', text: 'content' } | |
| * not { text: { text: 'content' } } as the union type suggests | |
| */ | |
| async prompt(sessionId: SessionId, text: string): Promise<PromptResponse> { | |
| // ACP uses a simpler content block format for text | |
| const contentBlock = { | |
| type: 'text', | |
| text, | |
| }; | |
| const request: PromptRequest = { | |
| sessionId, | |
| prompt: [contentBlock as unknown as ContentBlock], | |
| * | |
| * Uses the standard ContentBlock text format: { text: { text: 'content' } }. | |
| */ | |
| async prompt(sessionId: SessionId, text: string): Promise<PromptResponse> { | |
| const contentBlock: ContentBlock = { | |
| text: { text }, | |
| }; | |
| const request: PromptRequest = { | |
| sessionId, | |
| prompt: [contentBlock], |
Comment on lines
+4359
to
+4369
| // TODO: Delete agent's session storage (OpenCode/Codex sessions) | ||
| // Currently these sessions remain in the agent's storage (~/.local/share/opencode/storage/) | ||
| // even after the Maestro session is deleted, causing them to appear in Agent Sessions Browser. | ||
| // Once ACP supports session deletion, we should: | ||
| // if (session.agentSessionId && session.toolType) { | ||
| // try { | ||
| // await window.maestro.agentSessions.delete(session.toolType, session.agentSessionId); | ||
| // } catch (error) { | ||
| // console.error('Failed to delete agent session:', error); | ||
| // } | ||
| // } |
There was a problem hiding this comment.
The TODO comment indicates incomplete functionality for agent session deletion. While this is documented as a known limitation in the PR description, consider whether orphaned agent sessions could accumulate over time and cause storage issues. It might be worth adding a warning log when sessions are deleted without cleaning up the agent storage, or tracking this in an issue for future implementation.
Suggested change
| // TODO: Delete agent's session storage (OpenCode/Codex sessions) | |
| // Currently these sessions remain in the agent's storage (~/.local/share/opencode/storage/) | |
| // even after the Maestro session is deleted, causing them to appear in Agent Sessions Browser. | |
| // Once ACP supports session deletion, we should: | |
| // if (session.agentSessionId && session.toolType) { | |
| // try { | |
| // await window.maestro.agentSessions.delete(session.toolType, session.agentSessionId); | |
| // } catch (error) { | |
| // console.error('Failed to delete agent session:', error); | |
| // } | |
| // } | |
| // Agent sessions cannot yet be deleted via ACP; log a warning so we can | |
| // track potential orphaned sessions in the underlying agent storage | |
| // (~/.local/share/opencode/storage/) and in the Agent Sessions Browser. | |
| if (session.agentSessionId && session.toolType) { | |
| console.warn( | |
| 'Agent session was not deleted because ACP does not yet support agent session deletion. ' + | |
| 'This may leave an orphaned agent session in storage.', | |
| { | |
| toolType: session.toolType, | |
| agentSessionId: session.agentSessionId, | |
| } | |
| ); | |
| } |
Comment on lines
+375
to
+376
| } catch { | ||
| logger.error(`Failed to write file: ${request.path}`, LOG_CONTEXT); |
There was a problem hiding this comment.
The error handler for 'fs:write' silently catches all errors without providing any error information. This makes it difficult to diagnose file write failures. Consider logging the error details or returning an error response to the agent so it can handle the failure appropriately.
Suggested change
| } catch { | |
| logger.error(`Failed to write file: ${request.path}`, LOG_CONTEXT); | |
| } catch (error: unknown) { | |
| const errorMessage = error instanceof Error ? error.message : String(error); | |
| logger.error(`Failed to write file: ${request.path}: ${errorMessage}`, LOG_CONTEXT); |
Comment on lines
+46
to
+66
| if (typeof content === 'string') { | ||
| return content; | ||
| } | ||
|
|
||
| if ('image' in content) { | ||
| return '[image]'; | ||
| } | ||
| if ('resource_link' in content) { | ||
| return `[resource: ${content.resource_link.name}]`; | ||
| } | ||
| if ('resource' in content) { | ||
| const res = content.resource.resource; | ||
| if ('text' in res) { | ||
| return res.text; | ||
| } | ||
| return '[binary resource]'; | ||
| } | ||
|
|
||
| // Fallback: try to stringify | ||
| logger.warn('Unknown content block format', LOG_CONTEXT, { content }); | ||
| return typeof content === 'object' ? JSON.stringify(content) : String(content); |
There was a problem hiding this comment.
There's an inconsistency in how the fallback text is constructed. Line 66 uses JSON.stringify(content) while line 46 uses String(content). For consistency and better debugging, both cases should probably use JSON.stringify when dealing with object types, since String(content) will just produce '[object Object]' for objects.
| * instead of the custom JSON format. | ||
| */ | ||
|
|
||
| import { describe, it, expect, beforeAll, afterAll, vi } from 'vitest'; |
There was a problem hiding this comment.
Unused imports afterAll, beforeAll, vi.
Suggested change
| import { describe, it, expect, beforeAll, afterAll, vi } from 'vitest'; | |
| import { describe, it, expect } from 'vitest'; |
| import { acpUpdateToParseEvent, createSessionIdEvent } from '../../main/acp/acp-adapter'; | ||
| import type { SessionUpdate } from '../../main/acp/types'; | ||
| import { execSync } from 'child_process'; | ||
| import * as path from 'path'; |
There was a problem hiding this comment.
Unused import path.
Suggested change
| import * as path from 'path'; |
Owner
|
@oliveiraantoniocc can i trouble you to rebase this on latest refactored main? because it's feature-gated, i think we can just merge this in from there and get feedback from those who are using it. can you ensure it has a 
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR adds support for the Agent Client Protocol (ACP) to enable native integration with OpenCode and other ACP-compatible agents.
Key Changes
ACP Protocol Implementation
acp-client.ts): JSON-RPC 2.0 client for stdio communication with ACP agentsacp-process.ts): Wrapper that provides ProcessManager-compatible interface for ACP agentsacp-adapter.ts): Converts ACP session updates to Maestro's ParsedEvent formattypes.ts): Complete TypeScript definitions for ACP protocol specBug Fixes
isLoadingSessionflag to ignore session updates duringsession/load.Configuration & Debugging
useACPandacpShowStreamingoptions to agent configs for per-agent ACP mode controlMAESTRO_LOG_LEVELenvironment variable support anddev:main:debugscript for easier debuggingOpenCode Integration
session/loadmethodTesting
Tested with OpenCode v1.0.190:
Known Limitations
Checklist