Skip to content

feat: add experimental ACP mode (--experimental-acp) - #186

Closed
johnstcn wants to merge 19 commits into
mainfrom
cj/exp/acp
Closed

feat: add experimental ACP mode (--experimental-acp)#186
johnstcn wants to merge 19 commits into
mainfrom
cj/exp/acp

Conversation

@johnstcn

@johnstcnjohnstcn commented Feb 13, 2026

Copy link
Copy Markdown
Member

Depends on #185
Fixescoder/internal#1333

Add support for Agent Control Protocol (ACP) as an alternative to terminal emulation. ACP uses JSON-RPC over stdin/stdout pipes.

  • Introduce AgentIO interface to abstract PTY vs ACP transports
  • Add ACPConversation implementing Conversation interface
  • Add --experimental-acp flag (mutually exclusive with --print-openapi)
  • Add e2e test with mock ACP agent

Created using Mux (Opus 4.5)

- Add Emitter interface to screentracker package
- Remove OnSnapshot from PTYConversationConfig, accept Emitter in NewPTY
- Rename EventEmitter methods: EmitMessages, EmitStatus, EmitScreen
- Accept agentType at NewEventEmitter construction instead of per-call
- Update server.go wiring, all tests pass
- Add Emitter interface to screentracker package
- Remove OnSnapshot from PTYConversationConfig, accept Emitter in NewPTY
- Rename EventEmitter methods: EmitMessages, EmitStatus, EmitScreen
- Accept agentType at NewEventEmitter construction instead of per-call
- Update server.go wiring, all tests pass
@johnstcnjohnstcn self-assigned this Feb 13, 2026
@github-actions

Copy link
Copy Markdown

✅ Preview binaries are ready!

To test with modules: agentapi_version = "agentapi_186" or download from: https://github.com/coder/agentapi/releases/tag/agentapi_186

Add support for Agent Control Protocol (ACP) as an alternative to
terminal emulation. ACP uses JSON-RPC over stdin/stdout pipes.
- Introduce AgentIO interface to abstract PTY vs ACP transports
- Add ACPConversation implementing Conversation interface
- Add --experimental-acp flag (mutually exclusive with --print-openapi)
- Add e2e test with mock ACP agent
- Block `attach` when using --experimental-acp (no terminal)
- Update chat UI to show ACP tool calls
Other changes:
- chat: Fix redundant draft filtering from finally block
Created using Mux (Opus 4.5)
Comment on lines -307 to -309
setMessages((prevMessages) =>
prevMessages.filter((m) => !isDraftMessage(m))
);

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

review: this was causing a 'flicker' when sending a message in the UI

Comment threadcmd/attach/attach.go
}

if status.ACPMode {
return xerrors.New("attach is not supported in ACP mode. The server is running with --experimental-acp which uses JSON-RPC instead of terminal emulation.")

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

review: It should be eventually supported but I'm not sure what form it should take yet.

Comment threadx/acpio/acpio.go
Comment on lines +89 to +96
func (c *acpClient) RequestPermission(ctx context.Context, params acp.RequestPermissionRequest) (acp.RequestPermissionResponse, error) {
// Auto-approve all permissions for Phase 1
return acp.RequestPermissionResponse{
Outcome: acp.RequestPermissionOutcome{
Selected: &acp.RequestPermissionOutcomeSelected{OptionId: "allow"},
},
}, nil
}

@johnstcnjohnstcnFeb 16, 2026

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

review: we need to add support for allow/deny; will be handled in a future PR

@johnstcn
johnstcn marked this pull request as ready for review February 17, 2026 17:57
CopilotAI review requested due to automatic review settings February 17, 2026 17:57

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds experimental support for Agent Control Protocol (ACP) as an alternative transport to PTY-based terminal emulation. ACP uses JSON-RPC over stdin/stdout pipes for cleaner communication without terminal escape sequences.

Changes:

  • Introduces AgentIO interface abstraction for PTY vs ACP transports
  • Implements ACPConversation and ACPAgentIO for ACP protocol support
  • Adds --experimental-acp CLI flag with validation against --print-openapi
  • Adds Backend field to status API response and blocks attach command in ACP mode

Reviewed changes

Copilot reviewed 16 out of 17 changed files in this pull request and generated 6 comments.

Show a summary per file
FileDescription
x/acpio/acpio.goCore ACP I/O implementation with JSON-RPC connection handling
x/acpio/acp_conversation.goACP conversation tracker with async message handling
x/acpio/acp_conversation_test.goComprehensive unit tests for ACP conversation
lib/httpapi/setup.goACP process setup and lifecycle management
lib/httpapi/server.goServer integration with transport abstraction
lib/httpapi/server_test.goUpdated tests to use AgentIO interface
lib/httpapi/models.goAdded Backend field to status response
cmd/server/server.goCLI flag and transport selection logic
cmd/attach/attach.goACP mode detection and rejection for attach command
openapi.jsonAPI schema update with backend field
go.mod/go.sumAdded acp-go-sdk dependency
lib/acp/doc.goPackage documentation for ACP support
e2e/acp_echo.goMock ACP agent for e2e testing
e2e/echo_test.goE2E test for ACP mode
e2e/testdata/acp_basic.jsonTest script data
chat/src/components/chat-provider.tsxRemoved draft message cleanup (appears to be a bug)
Comments suppressed due to low confidence (1)

lib/httpapi/server_test.go:37

  • The tests don't set the Transport field in ServerConfig, which means it will be an empty string. The server code will default to PTY transport when config.Transport != "acp", but the Backend field in the status response will be an empty string instead of "pty". This could cause issues for clients that check the backend field.

Consider setting a default value "pty" for config.Transport when it's empty, or explicitly setting Transport in all tests.

	srv, err := httpapi.NewServer(ctx, httpapi.ServerConfig{
AgentType: msgfmt.AgentTypeClaude,
AgentIO: nil,
Port: 0,
ChatBasePath: "/chat",
AllowedHosts: []string{"*"},
AllowedOrigins: []string{"*"},
})

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment threadlib/httpapi/setup.go
Comment on lines +104 to +120
go func() {
select {
case <-ctx.Done():
logger.Info("Context done, closing ACP agent")
_ = stdin.Close()
_ = stdout.Close()
// Try graceful shutdown first
_ = cmd.Process.Signal(syscall.SIGTERM)
// Force kill after timeout
time.AfterFunc(5*time.Second, func() {
_ = cmd.Process.Kill()
})
case <-done:
// Process exited normally, nothing to clean up
return
}
}()

CopilotAIFeb 17, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cleanup goroutine in SetupACP has a potential race condition with the AfterFunc timer. If the context is cancelled, SIGTERM is sent, and then exactly 5 seconds later the cleanup goroutine receives from the done channel (normal exit), the AfterFunc will still fire and call Kill() on an already-exited process.

Consider using a sync mechanism to cancel the AfterFunc timer when the process exits normally, or checking if the process has exited before calling Kill().

Copilot uses AI. Check for mistakes.
Comment threadchat/src/components/chat-provider.tsx
Comment threadlib/httpapi/setup.go
Comment on lines +106 to +115
case <-ctx.Done():
logger.Info("Context done, closing ACP agent")
_ = stdin.Close()
_ = stdout.Close()
// Try graceful shutdown first
_ = cmd.Process.Signal(syscall.SIGTERM)
// Force kill after timeout
time.AfterFunc(5*time.Second, func() {
_ = cmd.Process.Kill()
})

CopilotAIFeb 17, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the context is cancelled, the cleanup goroutine closes stdin/stdout pipes and signals SIGTERM to the process. However, the main goroutine (lines 185-194) is simultaneously calling acpResult.Wait() which calls cmd.Wait(). Closing the pipes while the ACP connection might still be active could cause the ACP SDK to encounter unexpected EOF errors or other I/O errors.

Consider coordinating the shutdown sequence so that the ACP connection can be gracefully closed before the pipes are closed, or ensure that the ACP SDK properly handles pipe closure during context cancellation.

Copilot uses AI. Check for mistakes.
Comment threadx/acpio/acp_conversation.go Outdated
Comment threadcmd/server/server.go
Comment threadx/acpio/acpio.go
Previously, when Send() failed, only empty agent messages were removed.
If the agent had streamed partial content before the error, that partial
message would incorrectly remain in the conversation.
Now we remove the agent message on error regardless of whether it has
content, ensuring the conversation state stays consistent.
Reorder the shutdown sequence to send SIGTERM first, giving the agent
a chance to shutdown gracefully before losing I/O.
- Add Clock field to SetupACPConfig
- Default to quartz.NewReal() if Clock is nil
- Replace time.AfterFunc with config.Clock.AfterFunc for testability
@johnstcn
johnstcn changed the base branch from cj/refactor/event-emitter to mainFebruary 18, 2026 11:13
The cleanup goroutine in SetupACPProcess was leaking because it
didn't return after handling ctx.Done(). After scheduling the kill
timer, the goroutine would block forever on the done channel.
- Add ctx and cancel fields to ACPConversation struct
- Update NewACPConversation to accept context.Context as first param
- Create cancellable context in constructor using context.WithCancel
- Add Stop() method that calls c.cancel()
- Check c.ctx.Err() in executePrompt before each message part
- Update server.go to pass ctx to NewACPConversation
- Update all test calls to pass context.Background()
@johnstcn
johnstcn marked this pull request as draft February 19, 2026 09:32
@johnstcn

Copy link
Copy Markdown
MemberAuthor

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add experimental ACP support to coder/agentapi

2 participants

@johnstcn