chore: remove unreachable!() and improve error messages (#139)

* remove unreachable!(), improve panic messages, fix tautological test

* revert debug_assert

* format

---------

Co-authored-by: Ben Brandt <benjamin.j.brandt@gmail.com>

Author: anaslimem

src/agent-client-protocol-conductor/src/mcp_bridge.rs

@@ -95,28 +95,31 @@ pub async fn run_mcp_bridge(port: u16) -> Result<(), agent_client_protocol::Erro
 async fn connect_with_retry(port: u16) -> Result<TcpStream, agent_client_protocol::Error> {
     let max_retries = 10;
     let mut retry_delay_ms = 50;
+    let mut last_error = None;
 
     for attempt in 1..=max_retries {
         match TcpStream::connect(format!("127.0.0.1:{port}")).await {
             Ok(stream) => {
                 tracing::info!("Connected to localhost:{} on attempt {}", port, attempt);
                 return Ok(stream);
             }
-            Err(e) if attempt < max_retries => {
+            Err(e) => {
                 tracing::debug!(
                     "Connection attempt {} failed: {}, retrying in {}ms",
                     attempt,
                     e,
                     retry_delay_ms
                 );
-                tokio::time::sleep(tokio::time::Duration::from_millis(retry_delay_ms)).await;
-                retry_delay_ms = (retry_delay_ms * 2).min(1000); // Exponential backoff, max 1s
-            }
-            Err(e) => {
-                return Err(agent_client_protocol::Error::into_internal_error(e));
+                last_error = Some(e);
+                if attempt < max_retries {
+                    tokio::time::sleep(tokio::time::Duration::from_millis(retry_delay_ms)).await;
+                    retry_delay_ms = (retry_delay_ms * 2).min(1000);
+                }
             }
         }
     }
 
-    unreachable!()
+    Err(agent_client_protocol::Error::into_internal_error(
+        last_error.expect("loop ran at least once"),
+    ))
 }

src/agent-client-protocol-trace-viewer/src/lib.rs

@@ -36,19 +36,25 @@ pub struct TraceHandle {
 impl TraceHandle {
     /// Push a new event to the trace.
     pub fn push(&self, event: serde_json::Value) {
-        self.events.lock().unwrap().push(event);
+        self.events
+            .lock()
+            .expect("events mutex poisoned")
+            .push(event);
     }
 
     /// Get the current number of events.
     #[must_use]
     pub fn len(&self) -> usize {
-        self.events.lock().unwrap().len()
+        self.events.lock().expect("events mutex poisoned").len()
     }
 
     /// Check if empty.
     #[must_use]
     pub fn is_empty(&self) -> bool {
-        self.events.lock().unwrap().is_empty()
+        self.events
+            .lock()
+            .expect("events mutex poisoned")
+            .is_empty()
     }
 }
 
@@ -192,7 +198,7 @@ async fn serve_events_from_file(path: &PathBuf) -> Response {
 }
 
 fn serve_events_from_memory(events: &Arc<Mutex<Vec<serde_json::Value>>>) -> Response {
-    let events = events.lock().unwrap();
+    let events = events.lock().expect("events mutex poisoned");
     match serde_json::to_string(&*events) {
         Ok(json) => (StatusCode::OK, [("content-type", "application/json")], json).into_response(),
         Err(e) => (

src/agent-client-protocol/examples/simple_agent.rs

@@ -19,7 +19,10 @@ async fn main() -> Result<()> {
         .on_receive_dispatch(
             async move |message: Dispatch, cx: ConnectionTo<Client>| {
                 // Respond to any other message with an error
-                message.respond_with_error(agent_client_protocol::util::internal_error("TODO"), cx)
+                message.respond_with_error(
+                    agent_client_protocol::util::internal_error("unhandled message"),
+                    cx,
+                )
             },
             agent_client_protocol::on_receive_dispatch!(),
         )

src/agent-client-protocol/tests/jsonrpc_error_handling.rs

@@ -219,8 +219,11 @@ async fn test_incomplete_line() {
     // The server should handle EOF mid-message gracefully
     let result = connection.connect_to(transport).await;
 
-    // Server should terminate cleanly when hitting EOF
-    assert!(result.is_ok() || result.is_err());
+    // Server should terminate cleanly (not hang) when EOF is hit mid-message
+    assert!(
+        result.is_ok(),
+        "expected clean shutdown on EOF, got: {result:?}"
+    );
 }
 
 // ============================================================================