fix(acp): return JSON-RPC errors from dispatch handlers instead of killing the connection (#10499)

Signed-off-by: Matt Toohey <contact@matttoohey.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>

crates/goose/src/acp/server/dispatch.rs

@@ -109,9 +109,15 @@ impl HandleDispatchFrom<Client> for GooseAcpHandler {
                         let cx_spawn = cx.clone();
                         cx.spawn(async move {
                             let cx = cx_spawn;
-                            let value_id = req.value.as_value_id()
-                                .ok_or_else(|| agent_client_protocol::Error::invalid_params().data("Expected a value ID"))?
-                                .clone();
+                            let value_id = match req.value.as_value_id() {
+                                Some(value_id) => value_id.clone(),
+                                None => {
+                                    responder.respond_with_error(
+                                        agent_client_protocol::Error::invalid_params().data("Expected a value ID")
+                                    )?;
+                                    return Ok(());
+                                }
+                            };
                             let session_id = req.session_id.clone();
                             let sid = sid_short(session_id.0.as_ref());
                             let config_id = req.config_id.0.to_string();
@@ -150,7 +156,19 @@ impl HandleDispatchFrom<Client> for GooseAcpHandler {
                                 }
                             }
                             // Respond immediately using the current provider inventory snapshot.
-                            let (notification, config_options) = agent.build_config_update(&session_id).await?;
+                            let (notification, config_options) = match agent.build_config_update(&session_id).await {
+                                Ok(update) => update,
+                                Err(e) => {
+                                    warn!(
+                                        sid = %sid,
+                                        config_id = %config_id,
+                                        error = ?e,
+                                        "failed to build config update after config change"
+                                    );
+                                    responder.respond_with_error(e)?;
+                                    return Ok(());
+                                }
+                            };
                             cx.send_notification(notification)?;
                             responder.respond(SetSessionConfigOptionResponse::new(config_options))?;
 
@@ -346,7 +364,10 @@ impl HandleDispatchFrom<Client> for GooseAcpHandler {
                     let cx = cx.clone();
                     |req: CloseSessionRequest, responder: Responder<CloseSessionResponse>| async move {
                         cx.spawn(async move {
-                            responder.respond(agent.on_close_session(&req.session_id.0).await?)?;
+                            match agent.on_close_session(&req.session_id.0).await {
+                                Ok(response) => responder.respond(response)?,
+                                Err(e) => responder.respond_with_error(e)?,
+                            }
                             Ok(())
                         })?;
                         Ok(())

crates/goose/tests/acp_server_test.rs

@@ -525,6 +525,41 @@ fn test_config_option_thinking_effort_set() {
     });
 }
 
+#[test]
+fn test_config_option_non_value_id_returns_error_and_keeps_connection() {
+    run_test(async {
+        let openai = OpenAiFixture::new(
+            vec![],
+            <AcpServerConnection as Connection>::expected_session_id(),
+        )
+        .await;
+        let mut conn =
+            <AcpServerConnection as Connection>::new(TestConnectionConfig::default(), openai).await;
+        let data = conn.new_session().await.unwrap();
+
+        let err = conn
+            .cx()
+            .send_request(SetSessionConfigOptionRequest::new(
+                data.session.session_id().clone(),
+                "mode".to_string(),
+                SessionConfigOptionValue::boolean(true),
+            ))
+            .block_task()
+            .await
+            .unwrap_err();
+        assert_eq!(
+            err,
+            agent_client_protocol::Error::invalid_params().data("Expected a value ID")
+        );
+
+        conn.cx()
+            .send_request(ListSessionsRequest::new())
+            .block_task()
+            .await
+            .expect("connection should survive an invalid set_config_option");
+    });
+}
+
 #[test]
 fn test_delete_session() {
     run_test(async { run_delete_session::<AcpServerConnection>().await });