From d850a3b77a5097431b560dc22336871589a6b461 Mon Sep 17 00:00:00 2001 From: Nathan Flurry Date: Sun, 15 Mar 2026 18:52:59 -0700 Subject: [PATCH] fix: normalize Pi ACP bootstrap payloads (#227) * fix: normalize pi ACP bootstrap payloads * docs(cli): document custom pi binary override * docs(quickstart): list all supported agent IDs * docs(code): clarify Pi payload normalization rationale --- docs/cli.mdx | 33 +++++++ docs/quickstart.mdx | 2 + .../sandbox-agent/src/acp_proxy_runtime.rs | 62 +++++++++++- .../tests/v1_api/acp_transport.rs | 94 +++++++++++++++++++ 4 files changed, 190 insertions(+), 1 deletion(-) diff --git a/docs/cli.mdx b/docs/cli.mdx index a3cd839..6177fb3 100644 --- a/docs/cli.mdx +++ b/docs/cli.mdx @@ -59,6 +59,39 @@ sandbox-agent install-agent claude --reinstall sandbox-agent install-agent --all ``` +### Custom Pi implementation path + +If you use a forked/custom `pi` binary with `pi-acp`, you can override what executable gets launched. + +#### Option 1: explicit command override (recommended) + +Set `PI_ACP_PI_COMMAND` in the environment where `sandbox-agent` runs: + +```bash +PI_ACP_PI_COMMAND=/absolute/path/to/your/pi-fork sandbox-agent server +``` + +This is forwarded to `pi-acp`, which uses it instead of looking up `pi` on `PATH`. + +#### Option 2: PATH override + +Put your custom `pi` first on `PATH` before starting `sandbox-agent`: + +```bash +export PATH="/path/to/custom-pi-dir:$PATH" +sandbox-agent server +``` + +#### Option 3: symlink override + +Point `pi` to your custom binary via symlink in a directory that is early on `PATH`: + +```bash +ln -sf /absolute/path/to/your/pi-fork /usr/local/bin/pi +``` + +Then start `sandbox-agent` normally. + ## opencode (experimental) Start/reuse daemon and run `opencode attach` against `/opencode`. diff --git a/docs/quickstart.mdx b/docs/quickstart.mdx index 7b5beed..caf2c21 100644 --- a/docs/quickstart.mdx +++ b/docs/quickstart.mdx @@ -217,6 +217,8 @@ icon: "rocket" + Supported agent IDs: `claude`, `codex`, `opencode`, `amp`, `pi`, `cursor`, `mock`. + To preinstall agents: ```bash diff --git a/server/packages/sandbox-agent/src/acp_proxy_runtime.rs b/server/packages/sandbox-agent/src/acp_proxy_runtime.rs index 47bc2b0..3710e2f 100644 --- a/server/packages/sandbox-agent/src/acp_proxy_runtime.rs +++ b/server/packages/sandbox-agent/src/acp_proxy_runtime.rs @@ -11,7 +11,7 @@ use futures::Stream; use sandbox_agent_agent_management::agents::{AgentId, AgentManager, InstallOptions}; use sandbox_agent_error::SandboxError; use sandbox_agent_opencode_adapter::{AcpDispatch, AcpDispatchResult, AcpPayloadStream}; -use serde_json::Value; +use serde_json::{Number, Value}; use tokio::sync::{Mutex, RwLock}; const DEFAULT_REQUEST_TIMEOUT_MS: u64 = 120_000; @@ -134,6 +134,8 @@ impl AcpProxyRuntime { "acp_proxy: instance resolved" ); + let payload = normalize_payload_for_agent(instance.agent, payload); + match instance.runtime.post(payload).await { Ok(PostOutcome::Response(value)) => { let total_ms = start.elapsed().as_millis() as u64; @@ -510,6 +512,64 @@ fn map_adapter_error(err: AdapterError, agent: Option) -> SandboxError } } +fn normalize_payload_for_agent(agent: AgentId, payload: Value) -> Value { + if agent != AgentId::Pi { + return payload; + } + + // Pi's ACP adapter is stricter than other adapters for a couple of bootstrap + // fields. Normalize here so older/raw ACP clients still work against Pi. + normalize_pi_payload(payload) +} + +fn normalize_pi_payload(mut payload: Value) -> Value { + let method = payload + .get("method") + .and_then(Value::as_str) + .unwrap_or_default(); + + match method { + "initialize" => { + // Some clients send ACP protocolVersion as a string ("1.0"), but + // pi-acp expects a numeric JSON value and rejects strings. + if let Some(protocol) = payload.pointer_mut("/params/protocolVersion") { + if let Some(raw) = protocol.as_str() { + if let Some(number) = parse_json_number(raw) { + *protocol = Value::Number(number); + } + } + } + } + "session/new" => { + // The TypeScript SDK and opencode adapter already send mcpServers: [], + // but raw /v1/acp callers may omit it. pi-acp currently validates + // mcpServers as required, so default it here for compatibility. + if let Some(params) = payload.get_mut("params").and_then(Value::as_object_mut) { + params + .entry("mcpServers".to_string()) + .or_insert_with(|| Value::Array(Vec::new())); + } + } + _ => {} + } + + payload +} + +fn parse_json_number(raw: &str) -> Option { + let trimmed = raw.trim(); + + if let Ok(unsigned) = trimmed.parse::() { + return Some(Number::from(unsigned)); + } + + if let Ok(signed) = trimmed.parse::() { + return Some(Number::from(signed)); + } + + trimmed.parse::().ok().and_then(Number::from_f64) +} + /// Inspect JSON-RPC error responses from agent processes and add helpful hints /// when we can infer the root cause from a known error pattern. fn annotate_agent_error(agent: AgentId, mut value: Value) -> Value { diff --git a/server/packages/sandbox-agent/tests/v1_api/acp_transport.rs b/server/packages/sandbox-agent/tests/v1_api/acp_transport.rs index 46b0198..147d844 100644 --- a/server/packages/sandbox-agent/tests/v1_api/acp_transport.rs +++ b/server/packages/sandbox-agent/tests/v1_api/acp_transport.rs @@ -33,6 +33,51 @@ done write_executable(path, &script); } +fn write_strict_pi_agent_process(path: &Path) { + // This stub intentionally mirrors the strict bootstrap validation behavior + // observed in pi-acp: + // - initialize.params.protocolVersion must be numeric + // - session/new.params.mcpServers must be present (array) + // + // The proxy normalization layer should adapt legacy/raw client payloads so + // requests still succeed against this stricter contract. + let script = r#"#!/usr/bin/env sh +if [ "${1:-}" = "--help" ] || [ "${1:-}" = "--version" ] || [ "${1:-}" = "version" ] || [ "${1:-}" = "-V" ]; then + echo "pi-agent-process 0.0.1" + exit 0 +fi + +while IFS= read -r line; do + method=$(printf '%s\n' "$line" | sed -n 's/.*"method"[[:space:]]*:[[:space:]]*"\([^"]*\)".*/\1/p') + id=$(printf '%s\n' "$line" | sed -n 's/.*"id"[[:space:]]*:[[:space:]]*\([^,}]*\).*/\1/p') + + if [ "$method" = "initialize" ] && [ -n "$id" ]; then + if printf '%s\n' "$line" | grep -Eq '"protocolVersion"[[:space:]]*:[[:space:]]*"'; then + printf '{"jsonrpc":"2.0","id":%s,"error":{"code":-32603,"message":"Internal error","data":[{"expected":"number","code":"invalid_type","path":["protocolVersion"],"message":"Invalid input: expected number, received string"}]}}\n' "$id" + else + printf '{"jsonrpc":"2.0","id":%s,"result":{"ok":true,"echoedMethod":"initialize"}}\n' "$id" + fi + continue + fi + + if [ "$method" = "session/new" ] && [ -n "$id" ]; then + if printf '%s\n' "$line" | grep -Eq '"mcpServers"[[:space:]]*:[[:space:]]*\['; then + printf '{"jsonrpc":"2.0","id":%s,"result":{"sessionId":"pi-session","echoedMethod":"session/new"}}\n' "$id" + else + printf '{"jsonrpc":"2.0","id":%s,"error":{"code":-32603,"message":"Internal error","data":[{"expected":"array","code":"invalid_type","path":["mcpServers"],"message":"Invalid input: expected array, received undefined"}]}}\n' "$id" + fi + continue + fi + + if [ -n "$method" ] && [ -n "$id" ]; then + printf '{"jsonrpc":"2.0","id":%s,"result":{"ok":true,"echoedMethod":"%s"}}\n' "$id" "$method" + fi +done +"#; + + write_executable(path, script); +} + fn setup_stub_artifacts(install_dir: &Path, agent: &str) { let native = install_dir.join(agent); write_stub_native(&native, agent); @@ -47,6 +92,17 @@ fn setup_stub_artifacts(install_dir: &Path, agent: &str) { write_stub_agent_process(&launcher, agent); } +fn setup_strict_pi_agent_process_only(install_dir: &Path) { + let agent_processes = install_dir.join("agent_processes"); + fs::create_dir_all(&agent_processes).expect("create agent processes dir"); + let launcher = if cfg!(windows) { + agent_processes.join("pi-acp.cmd") + } else { + agent_processes.join("pi-acp") + }; + write_strict_pi_agent_process(&launcher); +} + #[tokio::test] async fn acp_bootstrap_requires_agent_query() { let test_app = TestApp::new(AuthConfig::disabled()); @@ -115,6 +171,44 @@ async fn acp_round_trip_and_replay() { assert!(second_event_id > first_event_id); } +#[cfg(unix)] +#[tokio::test] +async fn pi_initialize_and_session_new_are_normalized() { + let test_app = TestApp::with_setup(AuthConfig::disabled(), |install_dir| { + setup_strict_pi_agent_process_only(install_dir); + }); + + let (status, _, body) = send_request( + &test_app.app, + Method::POST, + "/v1/acp/server-pi?agent=pi", + Some(initialize_payload()), + &[], + ) + .await; + assert_eq!(status, StatusCode::OK); + assert_eq!(parse_json(&body)["result"]["echoedMethod"], "initialize"); + + let session_new = json!({ + "jsonrpc": "2.0", + "id": 2, + "method": "session/new", + "params": { + "cwd": "/tmp" + } + }); + let (status, _, body) = send_request( + &test_app.app, + Method::POST, + "/v1/acp/server-pi", + Some(session_new), + &[], + ) + .await; + assert_eq!(status, StatusCode::OK); + assert_eq!(parse_json(&body)["result"]["echoedMethod"], "session/new"); +} + #[cfg(unix)] #[tokio::test] async fn acp_agent_mismatch_returns_conflict() {