refactor: remove retry_count from ProviderConfig and enhance error handling in tool execution

- Removed the `retry_count` field from `ProviderConfig` as it is no longer needed.
- Introduced `argument_error` field in `ToolCall` to capture errors related to tool arguments.
- Updated various components to handle argument errors more gracefully, including in the `ToolDispatcher` and `ConversationOutput`.
- Enhanced tests to validate the new error handling and ensure proper functionality of tool calls.

server/tests/error_lifecycle.rs

@@ -51,10 +51,35 @@ async fn abort_command_cancels_the_run_and_closes_output() {
 }
 
 #[tokio::test]
-async fn provider_failure_keeps_the_initial_checkpoint_then_returns_structured_error() {
+async fn provider_failure_retries_from_the_current_checkpoint_without_hiding_partial_output() {
     let (_directory, store) = fixtures::temp_store().await;
     let provider = fake_provider::FakeProvider::default();
-    provider.push_error(Error::Provider("provider failed".into()));
+    provider.push_results(vec![
+        Ok(ModelEvent::Start {
+            model_call_id: "attempt-0".into(),
+        }),
+        Ok(ModelEvent::TextStart),
+        Ok(ModelEvent::TextDelta("partial ".into())),
+        Ok(ModelEvent::ToolCallStart {
+            index: 0,
+            call_id: "failed-tool".into(),
+            name: "Read".into(),
+        }),
+        Ok(ModelEvent::ToolCallArgumentsDelta {
+            index: 0,
+            delta: "{\"path\":".into(),
+        }),
+        Err(Error::Provider("stream disconnected".into())),
+    ]);
+    provider.push(vec![
+        ModelEvent::Start {
+            model_call_id: "attempt-1".into(),
+        },
+        ModelEvent::TextStart,
+        ModelEvent::TextDelta("completed".into()),
+        ModelEvent::TextEnd,
+        ModelEvent::Done(FinishReason::Stop),
+    ]);
     let assets = PromptAssets::load(
         std::path::Path::new(env!("CARGO_MANIFEST_DIR"))
             .join("prompt/cursor")
@@ -63,7 +88,106 @@ async fn provider_failure_keeps_the_initial_checkpoint_then_returns_structured_e
     .unwrap();
     let registry = TransportRegistry::new(
         store.clone(),
-        Arc::new(provider),
+        Arc::new(provider.clone()),
+        PromptCompiler::new(assets),
+    );
+    let handle = registry.get_or_create("retry-request").await.unwrap();
+    let mut output = handle.subscribe();
+    handle
+        .command(TransportCommand::Append {
+            seqno: 0,
+            message: Box::new(protocol_client_run("retry", "retry-user")),
+        })
+        .await
+        .unwrap();
+
+    let mut seqno = 1;
+    let mut text = String::new();
+    let mut failed_tool_completed = false;
+    loop {
+        let frame = tokio::time::timeout(std::time::Duration::from_secs(60), output.recv())
+            .await
+            .unwrap()
+            .unwrap();
+        let (flags, payload) = connect::decode_frames(&frame).unwrap().pop().unwrap();
+        if flags & connect::END_STREAM_FLAG != 0 {
+            assert_eq!(
+                serde_json::from_slice::<serde_json::Value>(&payload).unwrap(),
+                serde_json::json!({})
+            );
+            break;
+        }
+        let server = pb::AgentServerMessage::decode(payload).unwrap();
+        match server.message {
+            Some(pb::agent_server_message::Message::KvServerMessage(kv)) => {
+                handle
+                    .command(TransportCommand::Append {
+                        seqno,
+                        message: Box::new(kv_ack(kv.id)),
+                    })
+                    .await
+                    .unwrap();
+                seqno += 1;
+            }
+            Some(pb::agent_server_message::Message::InteractionUpdate(update)) => {
+                match update.message {
+                    Some(pb::interaction_update::Message::TextDelta(delta)) => {
+                        text.push_str(&delta.text);
+                    }
+                    Some(pb::interaction_update::Message::ToolCallCompleted(completed))
+                        if completed.call_id == "failed-tool" =>
+                    {
+                        let tool = completed.tool_call.expect("failed tool completion");
+                        failed_tool_completed = tool.completed_at_ms.is_some();
+                    }
+                    _ => {}
+                }
+            }
+            _ => {}
+        }
+    }
+
+    assert_eq!(text, "partial completed");
+    assert!(
+        failed_tool_completed,
+        "failed attempt must terminate its partial tool card"
+    );
+    let requests = provider.requests();
+    assert_eq!(requests.len(), 2);
+    assert_eq!(requests[0], requests[1]);
+    let messages = store
+        .load_current_messages(&cursor_server::model::ConversationId::new(
+            "protocol-failed-conversation",
+        ))
+        .await
+        .unwrap();
+    assert!(messages.iter().any(|message| {
+        matches!(&message.content, MessageContent::Assistant { text, .. } if text == "completed")
+    }));
+    assert!(!messages.iter().any(|message| {
+        matches!(&message.content, MessageContent::Assistant { text, .. } if text.contains("partial"))
+    }));
+}
+
+#[tokio::test]
+async fn provider_failure_keeps_the_initial_checkpoint_then_returns_structured_error() {
+    let (_directory, store) = fixtures::temp_store().await;
+    let provider = fake_provider::FakeProvider::default();
+    provider.push(vec![
+        ModelEvent::Start {
+            model_call_id: "length-limited".into(),
+        },
+        ModelEvent::Done(FinishReason::Length),
+    ]);
+    let assets = PromptAssets::load(
+        std::path::Path::new(env!("CARGO_MANIFEST_DIR"))
+            .join("prompt/cursor")
+            .as_path(),
+    )
+    .unwrap();
+    let registry = TransportRegistry::new(
+        store.clone(),
+        Arc::new(provider.clone()),
         PromptCompiler::new(assets),
     );
     let handle = registry.get_or_create("failed-request").await.unwrap();
@@ -148,6 +272,7 @@ async fn provider_failure_keeps_the_initial_checkpoint_then_returns_structured_e
         None
     );
 
+    assert_eq!(provider.requests().len(), 1, "Length must not retry");
     let messages = store
         .load_current_messages(&cursor_server::model::ConversationId::new(
             "failed-conversation",
@@ -164,7 +289,7 @@ async fn provider_failure_keeps_the_initial_checkpoint_then_returns_structured_e
 }
 
 #[tokio::test]
-async fn runtime_protocol_failure_returns_connect_error_end_stream_and_closes() {
+async fn unknown_tool_response_id_is_ignored_and_the_run_continues() {
     let (_directory, store) = fixtures::temp_store().await;
     let provider = fake_provider::FakeProvider::default();
     provider.push(vec![
@@ -183,6 +308,15 @@ async fn runtime_protocol_failure_returns_connect_error_end_stream_and_closes()
         ModelEvent::ToolCallEnd { index: 0 },
         ModelEvent::Done(FinishReason::ToolUse),
     ]);
+    provider.push(vec![
+        ModelEvent::Start {
+            model_call_id: "model-call-2".into(),
+        },
+        ModelEvent::TextStart,
+        ModelEvent::TextDelta("done".into()),
+        ModelEvent::TextEnd,
+        ModelEvent::Done(FinishReason::Stop),
+    ]);
     let assets = PromptAssets::load(
         std::path::Path::new(env!("CARGO_MANIFEST_DIR"))
             .join("prompt/cursor")
@@ -209,7 +343,7 @@ async fn runtime_protocol_failure_returns_connect_error_end_stream_and_closes()
 
     let mut append_seqno = 1;
     let mut saw_turn_ended = false;
-    let error_json = loop {
+    let end_stream = loop {
         let frame = tokio::time::timeout(std::time::Duration::from_secs(5), output.recv())
             .await
             .unwrap()
@@ -231,24 +365,42 @@ async fn runtime_protocol_failure_returns_connect_error_end_stream_and_closes()
                 append_seqno += 1;
             }
             Some(pb::agent_server_message::Message::ExecServerMessage(exec)) => {
-                // An unknown numeric bridge id is a runtime protocol error.
-                handle
-                    .command(TransportCommand::Append {
-                        seqno: append_seqno,
-                        message: Box::new(pb::AgentClientMessage {
-                            message: Some(pb::agent_client_message::Message::ExecClientMessage(
-                                pb::ExecClientMessage {
-                                    id: exec.id + 1_000,
-                                    exec_id: String::new(),
-                                    message: None,
+                // Unknown bridge ids are ignored; the valid response still completes the tool.
+                for message in [
+                    pb::ExecClientMessage {
+                        id: exec.id + 1_000,
+                        exec_id: String::new(),
+                        message: None,
+                        ..Default::default()
+                    },
+                    pb::ExecClientMessage {
+                        id: exec.id,
+                        exec_id: String::new(),
+                        message: Some(pb::exec_client_message::Message::ReadResult(
+                            pb::ReadResult {
+                                result: Some(pb::read_result::Result::Success(pb::ReadSuccess {
+                                    path: "/tmp/a".into(),
+                                    output: Some(pb::read_success::Output::Content("value".into())),
                                     ..Default::default()
-                                },
-                            )),
-                        }),
-                    })
-                    .await
-                    .unwrap();
-                append_seqno += 1;
+                                })),
+                            },
+                        )),
+                        ..Default::default()
+                    },
+                ] {
+                    handle
+                        .command(TransportCommand::Append {
+                            seqno: append_seqno,
+                            message: Box::new(pb::AgentClientMessage {
+                                message: Some(
+                                    pb::agent_client_message::Message::ExecClientMessage(message),
+                                ),
+                            }),
+                        })
+                        .await
+                        .unwrap();
+                    append_seqno += 1;
+                }
             }
             Some(pb::agent_server_message::Message::InteractionUpdate(update)) => {
                 if matches!(
@@ -266,12 +418,8 @@ async fn runtime_protocol_failure_returns_connect_error_end_stream_and_closes()
         }
     };
 
-    assert!(!saw_turn_ended);
-    assert_eq!(error_json["error"]["code"], "invalid_argument");
-    assert_eq!(
-        error_json["error"]["message"],
-        "unknown ExecClientMessage id: 1001"
-    );
+    assert!(saw_turn_ended);
+    assert_eq!(end_stream, serde_json::json!({}));
     assert_eq!(
         tokio::time::timeout(std::time::Duration::from_secs(1), output.recv())
             .await
@@ -279,28 +427,14 @@ async fn runtime_protocol_failure_returns_connect_error_end_stream_and_closes()
         None
     );
 
-    let deadline = tokio::time::Instant::now() + std::time::Duration::from_secs(1);
-    let (status, failure_summary) = loop {
-        let row: (String, Option<String>) =
-            sqlx::query_as("SELECT status, failure_summary FROM runs WHERE cursor_request_id = ?")
-                .bind("protocol-failed-request")
-                .fetch_one(store.pool())
-                .await
-                .unwrap();
-        if row.0 != "running" {
-            break row;
-        }
-        assert!(
-            tokio::time::Instant::now() < deadline,
-            "Run remained running after the Cursor session failed"
-        );
-        tokio::time::sleep(std::time::Duration::from_millis(10)).await;
-    };
-    assert_eq!(status, "failed");
-    assert_eq!(
-        failure_summary.as_deref(),
-        Some("unknown ExecClientMessage id: 1001")
-    );
+    let (status, failure_summary): (String, Option<String>) =
+        sqlx::query_as("SELECT status, failure_summary FROM runs WHERE cursor_request_id = ?")
+            .bind("protocol-failed-request")
+            .fetch_one(store.pool())
+            .await
+            .unwrap();
+    assert_eq!(status, "completed");
+    assert_eq!(failure_summary, None);
 }
 
 #[tokio::test]