feat: Add ACP (Agent Client Protocol) job mode for delegating to any compatible coding agent - #1600
Conversation
…compatible coding agent Add a third container job mode (`JobMode::Acp`) that spawns any ACP-compliant agent (Goose, Codex, Gemini CLI, Cline, Copilot, etc.) as a subprocess inside a Docker container and communicates via the standard ACP protocol (JSON-RPC over stdio). **Bridge runtime** (`src/worker/acp_bridge.rs`): - Spawns agent subprocess, performs ACP handshake (initialize → session → prompt) - Translates ACP SessionNotification events to IronClaw's JobEventPayload stream - Auto-approves permissions (Docker container is the security boundary) - Supports follow-up prompts from the orchestrator - Detects agent process exit via oneshot channel to prevent infinite polling **User configuration** (mirrors MCP server pattern): - `ironclaw acp add/list/remove/toggle/test` CLI commands - DB-backed persistence with `~/.ironclaw/acp-agents.json` disk fallback - Per-agent `enabled` flag + global `ACP_ENABLED` toggle - `ironclaw acp test` spawns agent, verifies ACP handshake, reports capabilities **System integration**: - `ExtensionKind::AcpAgent` in extension manager (12 match arms) - `agent_name` parameter in CreateJobTool resolves agent from AcpAgentsFile - Mode stored as `"acp:<agent_name>"` for restart support - Doctor validation, status display, boot screen, app startup logging - Web UI: extension install mapping, job restart, follow-up prompt support Closes nearai#1506
Bridge: ToolCall, ToolCallUpdate, thought-image, max_turn_requests, session_id propagation, text_from_content_block, multibyte truncation. Config: AcpModeConfig defaults, settings resolution, env overrides. Job tool: schema includes "acp" mode + agent_name, mode="acp" requires agent_name parameter, JobMode::Acp as_str/display. Job manager: JobMode::Acp as_str/display, acp_memory_limit_mb default. CLI: parse_env_var valid/invalid/equals-in-value, command variants.
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive support for ACP (Agent Client Protocol) agents, enabling the system to integrate and manage external coding agents. Key changes include adding new configuration options for ACP agents, updating dependencies, implementing a dedicated CLI for agent management (add, remove, list, toggle, test), and integrating ACP agent support into the application's job management, extension system, and boot processes. The new AcpBridge worker facilitates communication between the orchestrator and ACP-compliant agents. Review comments highlight opportunities to improve error handling by validating agent status before use, refactor duplicated agent loading logic into a helper function, use anyhow::bail! for cleaner error returns, extract configuration resolution logic for better readability, and add checks for empty session IDs and agent commands to prevent unexpected runtime issues.
| if !agent.enabled { | ||
| return Err(ToolError::InvalidParameters(format!( | ||
| "ACP agent '{}' is disabled. Enable it with 'ironclaw acp toggle {}'.", | ||
| agent_name, agent_name | ||
| ))); |
There was a problem hiding this comment.
If agent.enabled is false, the code should return an error to the user, indicating that the agent is disabled and cannot be used. This provides better feedback to the user and prevents unexpected behavior.
| if !agent.enabled { | |
| return Err(ToolError::InvalidParameters(format!( | |
| "ACP agent '{}' is disabled. Enable it with 'ironclaw acp toggle {}'.", | |
| agent_name, agent_name | |
| ))); | |
| if !agent.enabled { | |
| return Err(ToolError::InvalidParameters(format( | |
| "ACP agent '{}' is disabled. Enable it with 'ironclaw acp toggle {}'.", | |
| agent_name, agent_name | |
| ))); | |
| } | |
There was a problem hiding this comment.
Already implemented — see lines 945-949 in the same diff hunk. The if !agent.enabled check with the actionable error message is present.
| crate::orchestrator::job_manager::JobMode::ClaudeCode | ||
| } | ||
| _ => crate::orchestrator::job_manager::JobMode::Worker, | ||
| let stored_mode = store |
There was a problem hiding this comment.
This code duplicates logic for loading ACP agents. Consider creating a helper function to avoid redundancy.
| let stored_mode = store | |
| let agent = match state.store { | |
| Some(ref s) => crate::config::acp::load_acp_agent_from_store(s.as_ref(), &state.user_id, agent_name).await, | |
| None => crate::config::acp::load_acp_agent(agent_name).await, | |
| }; | |
References
- When an issue is found in duplicated code, prefer refactoring into a shared function over applying localized fixes.
There was a problem hiding this comment.
Acknowledged — this mirrors the existing MCP pattern where cli/mcp.rs has the same DB-or-disk branching. The duplication is ~5 lines in 2 call sites. Deferring extraction to a follow-up that consolidates both MCP and ACP loading helpers.
| impl AcpModeConfig { | ||
| pub(crate) fn resolve(settings: &crate::settings::Settings) -> Result<Self, ConfigError> { | ||
| let defaults = Self::default(); | ||
| Ok(Self { | ||
| enabled: parse_bool_env("ACP_ENABLED", settings.sandbox.acp_enabled)?, | ||
| memory_limit_mb: parse_optional_env("ACP_MEMORY_LIMIT_MB", defaults.memory_limit_mb)?, | ||
| timeout_secs: parse_optional_env("ACP_TIMEOUT_SECS", defaults.timeout_secs)?, | ||
| }) |
There was a problem hiding this comment.
Consider extracting the logic for resolving AcpModeConfig from settings and environment variables into a separate function to improve readability and testability.
References
- Consolidate related sequences of operations, such as creating, persisting, and scheduling a job, into a single reusable method to improve code consistency and maintainability.
There was a problem hiding this comment.
The resolve() method is the extracted function — it's the standard pattern used by all config types in this codebase (ClaudeCodeConfig::resolve(), SandboxModeConfig::resolve(), HeartbeatConfig::resolve(), etc.). Each is a small self-contained method that reads env vars with settings fallback.
| // Report prompt result | ||
| let result_payload = | ||
| stop_reason_to_result(&prompt_response.stop_reason, &session_id.to_string()); | ||
| client_for_acp.post_event(&result_payload).await; |
There was a problem hiding this comment.
session_id is a SessionId type from the ACP SDK, not a raw string. The SDK guarantees it's populated when new_session() returns Ok. An empty-check on an opaque type would be defensive against something the type system already prevents.
| } | ||
| } | ||
|
|
||
| // ==================== ACP Client trait implementation ==================== |
There was a problem hiding this comment.
Already validated upstream — AcpAgentConfig::validate() rejects empty commands and is called both on config load (disk/DB) and during agent resolution in CreateJobTool::execute(). The bridge cannot receive an empty command through the normal code path.
Extract AcpEventSink trait so the same Client implementation (permission auto-approval, event translation) is shared between the container bridge (posts to orchestrator HTTP API) and the CLI test command (prints to stdout). Also extracts ironclaw_init_request() to avoid duplicating the ACP handshake parameters between bridge and test command.
…tor URL The orchestrator host was hardcoded to 172.17.0.1 on Linux, which is only correct for the default Docker bridge network. Environments with custom bridge IPs break container-to-host connectivity. Since all containers already set extra_hosts with host-gateway, using host.docker.internal works on all platforms and network configurations.
| .map(|db| db as Arc<dyn Database>) | ||
| } | ||
|
|
||
| async fn load_agents(db: Option<&Arc<dyn Database>>) -> Result<AcpAgentsFile, anyhow::Error> { |
There was a problem hiding this comment.
Medium Severity
The ACP CLI is hard-coded to the "default" DB scope for both load and save.
Runtime ACP resolution uses the actual owner/user scope elsewhere, so in a non-default-owner deployment ironclaw acp add/list/toggle/test will manage a different record set than the runtime reads from. That makes ACP appear misconfigured even when the CLI succeeded.
Please thread the resolved owner ID into ACP CLI persistence, or expose a --user-id flag similar to MCP. A test covering a non-default-owner scope would catch this.
| .unwrap_or_default(); | ||
|
|
||
| // Parse mode string — ACP jobs are stored as "acp:<agent_name>". | ||
| let (mode, acp_agent) = if stored_mode == "claude_code" { |
There was a problem hiding this comment.
High Severity
ACP restart can recreate a job without any ACP agent configuration.
This path reloads the agent with state.user_id, not old_job.user_id. In non-default-owner cases that lookup can fail even though the original ACP job was valid. The handler then still calls create_job(..., JobMode::Acp, None), so the new container starts in ACP mode without ACP_AGENT_COMMAND and immediately fails in the bridge.
Please resolve the agent with the original job owner and reject restart if the named ACP agent cannot be loaded or is disabled. A regression test covering ACP restart would be useful here.
| } | ||
| } | ||
|
|
||
| impl AcpModeConfig { |
There was a problem hiding this comment.
Medium Severity
ACP_TIMEOUT_SECS is parsed and documented, but it is not actually used by the runtime.
The config layer resolves this setting, but ContainerJobConfig never carries it forward and run_acp_bridge() still hard-codes a 30 minute timeout. That means operators can set ACP_TIMEOUT_SECS and see no behavioral change.
Please propagate the resolved ACP timeout through container/job config into AcpBridgeConfig, and add a test that proves the configured timeout overrides the default.
…compatible coding agent (#1600) * feat: add ACP (Agent Client Protocol) job mode for delegating to any compatible coding agent Add a third container job mode (`JobMode::Acp`) that spawns any ACP-compliant agent (Goose, Codex, Gemini CLI, Cline, Copilot, etc.) as a subprocess inside a Docker container and communicates via the standard ACP protocol (JSON-RPC over stdio). **Bridge runtime** (`src/worker/acp_bridge.rs`): - Spawns agent subprocess, performs ACP handshake (initialize → session → prompt) - Translates ACP SessionNotification events to IronClaw's JobEventPayload stream - Auto-approves permissions (Docker container is the security boundary) - Supports follow-up prompts from the orchestrator - Detects agent process exit via oneshot channel to prevent infinite polling **User configuration** (mirrors MCP server pattern): - `ironclaw acp add/list/remove/toggle/test` CLI commands - DB-backed persistence with `~/.ironclaw/acp-agents.json` disk fallback - Per-agent `enabled` flag + global `ACP_ENABLED` toggle - `ironclaw acp test` spawns agent, verifies ACP handshake, reports capabilities **System integration**: - `ExtensionKind::AcpAgent` in extension manager (12 match arms) - `agent_name` parameter in CreateJobTool resolves agent from AcpAgentsFile - Mode stored as `"acp:<agent_name>"` for restart support - Doctor validation, status display, boot screen, app startup logging - Web UI: extension install mapping, job restart, follow-up prompt support Closes #1506 * test: add comprehensive ACP test coverage (22 new tests) Bridge: ToolCall, ToolCallUpdate, thought-image, max_turn_requests, session_id propagation, text_from_content_block, multibyte truncation. Config: AcpModeConfig defaults, settings resolution, env overrides. Job tool: schema includes "acp" mode + agent_name, mode="acp" requires agent_name parameter, JobMode::Acp as_str/display. Job manager: JobMode::Acp as_str/display, acp_memory_limit_mb default. CLI: parse_env_var valid/invalid/equals-in-value, command variants. * refactor: make IronClawAcpClient reusable for CLI test command Extract AcpEventSink trait so the same Client implementation (permission auto-approval, event translation) is shared between the container bridge (posts to orchestrator HTTP API) and the CLI test command (prints to stdout). Also extracts ironclaw_init_request() to avoid duplicating the ACP handshake parameters between bridge and test command. * fix(sandbox): use host.docker.internal on all platforms for orchestrator URL The orchestrator host was hardcoded to 172.17.0.1 on Linux, which is only correct for the default Docker bridge network. Environments with custom bridge IPs break container-to-host connectivity. Since all containers already set extra_hosts with host-gateway, using host.docker.internal works on all platforms and network configurations. * fix ACP PR review feedback * fix ACP DB error fallback * fix clippy after staging merge --------- Co-authored-by: Rajul Bhatnagar <brajul@amazon.com> Co-authored-by: Firat Sertgoz <f@nuff.tech>
…compatible coding agent (nearai#1600) * feat: add ACP (Agent Client Protocol) job mode for delegating to any compatible coding agent Add a third container job mode (`JobMode::Acp`) that spawns any ACP-compliant agent (Goose, Codex, Gemini CLI, Cline, Copilot, etc.) as a subprocess inside a Docker container and communicates via the standard ACP protocol (JSON-RPC over stdio). **Bridge runtime** (`src/worker/acp_bridge.rs`): - Spawns agent subprocess, performs ACP handshake (initialize → session → prompt) - Translates ACP SessionNotification events to IronClaw's JobEventPayload stream - Auto-approves permissions (Docker container is the security boundary) - Supports follow-up prompts from the orchestrator - Detects agent process exit via oneshot channel to prevent infinite polling **User configuration** (mirrors MCP server pattern): - `ironclaw acp add/list/remove/toggle/test` CLI commands - DB-backed persistence with `~/.ironclaw/acp-agents.json` disk fallback - Per-agent `enabled` flag + global `ACP_ENABLED` toggle - `ironclaw acp test` spawns agent, verifies ACP handshake, reports capabilities **System integration**: - `ExtensionKind::AcpAgent` in extension manager (12 match arms) - `agent_name` parameter in CreateJobTool resolves agent from AcpAgentsFile - Mode stored as `"acp:<agent_name>"` for restart support - Doctor validation, status display, boot screen, app startup logging - Web UI: extension install mapping, job restart, follow-up prompt support Closes nearai#1506 * test: add comprehensive ACP test coverage (22 new tests) Bridge: ToolCall, ToolCallUpdate, thought-image, max_turn_requests, session_id propagation, text_from_content_block, multibyte truncation. Config: AcpModeConfig defaults, settings resolution, env overrides. Job tool: schema includes "acp" mode + agent_name, mode="acp" requires agent_name parameter, JobMode::Acp as_str/display. Job manager: JobMode::Acp as_str/display, acp_memory_limit_mb default. CLI: parse_env_var valid/invalid/equals-in-value, command variants. * refactor: make IronClawAcpClient reusable for CLI test command Extract AcpEventSink trait so the same Client implementation (permission auto-approval, event translation) is shared between the container bridge (posts to orchestrator HTTP API) and the CLI test command (prints to stdout). Also extracts ironclaw_init_request() to avoid duplicating the ACP handshake parameters between bridge and test command. * fix(sandbox): use host.docker.internal on all platforms for orchestrator URL The orchestrator host was hardcoded to 172.17.0.1 on Linux, which is only correct for the default Docker bridge network. Environments with custom bridge IPs break container-to-host connectivity. Since all containers already set extra_hosts with host-gateway, using host.docker.internal works on all platforms and network configurations. * fix ACP PR review feedback * fix ACP DB error fallback * fix clippy after staging merge --------- Co-authored-by: Rajul Bhatnagar <brajul@amazon.com> Co-authored-by: Firat Sertgoz <f@nuff.tech>
Summary
JobMode::Acpas a third container job mode alongsideWorkerandClaudeCodeironclaw acp add/list/remove/toggle/testCLI commands with DB-backed persistence and disk fallbackJobEventPayloadstream with follow-up prompt support and agent process exit detectionhost.docker.internalon all platformsCloses #1506
Verified against Kiro CLI
Test plan
cargo fmt— no changescargo clippy --all --benches --tests --examples --all-features— zero warningscargo test --lib— 3,576 passed, 0 failedironclaw acp add kiro --command kiro-cli --arg acp→ironclaw acp list→ironclaw acp test kiro