Fix audit follow-up bugs and apply MCP init timeout to HTTP transport

Author: alexylon

CHANGELOG.md

@@ -48,7 +48,7 @@ All notable changes to Sofos are documented in this file.
 
 - **`web_fetch` follows at most three redirects, and only to http(s) targets.** The default `reqwest` redirect policy would let an LLM-supplied URL chain through up to ten hops including non-http(s) schemes (`file://`, `data:`, custom). Sofos now caps the hop count and rejects any redirect with a non-http(s) scheme so a content URL can't slip into a different protocol mid-chain.
 - **MCP server names are restricted to `[A-Za-z0-9_-]+`.** Two visually-identical Unicode names (`github` vs `gith\u{1d62}ub`) used to produce distinct map entries and route differently while looking the same in config; sofos now refuses non-ASCII server names at load time.
-- **MCP `initialize` uses a 15-second timeout.** Tool calls keep the existing two-minute ceiling, but the handshake no longer holds session startup hostage for two minutes per misconfigured server.
+- **MCP `initialize` uses a 15-second timeout on both stdio and HTTP transports.** Tool calls keep the existing two-minute ceiling, but the handshake no longer holds session startup hostage for two minutes per misconfigured server.
 - **The clipboard image binary check is the effective cap.** Base64 expansion (~33 %) used to make the post-encode check the actual gate, while the binary check at the same limit was dead. Sofos now caps the binary side at three-quarters of the limit so a marginally oversized image fails fast with a clearer reason.
 
 ### Changed

src/clipboard.rs

@@ -71,11 +71,9 @@ pub fn get_clipboard_image() -> Option<PastedImage> {
         writer.write_image_data(&image.bytes).ok()?;
     }
 
-    // Base64 expands binary by ~33%, so a binary blob just under
-    // MAX_CLIPBOARD_IMAGE_BYTES becomes ~1.33x that after encoding and
-    // then trips the second check anyway. Cap the binary side at three
-    // quarters of the limit so the binary check is the effective gate
-    // and a marginally-oversize blob fails fast with a clearer reason.
+    // Binary cap is 3/4 of the API limit because base64 inflates by
+    // ~33% — without it the binary check is dead and the post-encode
+    // check is the effective gate.
     let binary_cap = MAX_CLIPBOARD_IMAGE_BYTES * 3 / 4;
     if buf.len() > binary_cap {
         tracing::warn!(

src/mcp/transport/http.rs

@@ -1,7 +1,7 @@
 use crate::error::{Result, SofosError};
 use crate::mcp::client::{
-    MCP_REQUEST_TIMEOUT, create_call_tool_request, create_init_request, parse_call_tool_response,
-    parse_list_tools_response,
+    MCP_INIT_TIMEOUT, MCP_REQUEST_TIMEOUT, create_call_tool_request, create_init_request,
+    parse_call_tool_response, parse_list_tools_response,
 };
 use crate::mcp::config::McpServerConfig;
 use crate::mcp::protocol::*;
@@ -73,23 +73,28 @@ impl HttpClient {
     }
 
     async fn initialize(&self) -> Result<()> {
-        let response = self
-            .send_request(
-                "initialize",
-                Some(serde_json::to_value(create_init_request())?),
-            )
-            .await?;
-
-        let _init_result: InitializeResult = serde_json::from_value(response)?;
-
-        // The MCP spec requires the client to confirm the handshake
-        // with a `notifications/initialized` message before sending any
-        // other request. The stdio transport already did this; HTTP
-        // didn't, which caused strict servers to reject every later
-        // request as "not initialized".
-        self.send_notification_initialized().await?;
-
-        Ok(())
+        // Handshake under a tighter ceiling than tool calls so a
+        // frozen server can't hold session startup hostage.
+        let handshake = async {
+            let response = self
+                .send_request(
+                    "initialize",
+                    Some(serde_json::to_value(create_init_request())?),
+                )
+                .await?;
+            let _init_result: InitializeResult = serde_json::from_value(response)?;
+            self.send_notification_initialized().await?;
+            Ok::<(), SofosError>(())
+        };
+        tokio::time::timeout(MCP_INIT_TIMEOUT, handshake)
+            .await
+            .map_err(|_| {
+                SofosError::McpError(format!(
+                    "MCP server '{}' initialize timed out after {}s",
+                    self.server_name,
+                    MCP_INIT_TIMEOUT.as_secs()
+                ))
+            })?
     }
 
     async fn send_notification_initialized(&self) -> Result<()> {

src/mcp/transport/stdio.rs

@@ -256,11 +256,8 @@ fn stdio_request_blocking(
         )));
     }
 
-    // Parse first as a generic Value so we can tell a server-initiated
-    // request (`method`) apart from a response. We don't speak the
-    // request side of the spec — `roots/list`, `sampling/createMessage`
-    // — so a frame carrying `method` is rejected here rather than
-    // confusing it with a response to our outgoing request.
+    // Parse as Value first; a `method` field means a server-initiated
+    // request, which sofos doesn't implement — reject explicitly.
     let raw: Value = serde_json::from_str(&response_line).map_err(|e| {
         SofosError::McpError(format!(
             "Failed to parse response from MCP server '{}': {}",
@@ -282,11 +279,8 @@ fn stdio_request_blocking(
         ))
     })?;
 
-    // JSON-RPC requires the response id to match the request id. With
-    // the in-flight `request_lock` this shouldn't fail today, but
-    // checking guards against future concurrent variants and against
-    // servers that emit an unsolicited frame. Accepts both numeric
-    // and string-shaped echoes of our outgoing numeric id.
+    // Reject id mismatches; accept either numeric or string echoes
+    // of the outgoing numeric id (spec lets servers reshape).
     if !response.id.matches_outgoing(request_id) {
         return Err(SofosError::McpError(format!(
             "MCP server '{}' returned response with id {:?}, expected {}",
@@ -308,12 +302,10 @@ pub struct StdioClient {
     next_id: Arc<AtomicU64>,
 }
 
-/// Send SIGKILL / `TerminateProcess` then poll `try_wait` for up to
-/// [`STDIO_DROP_WAIT_TOTAL`] before giving up. Shared by `Drop` and
-/// the timeout-recovery `kill_child_detached` path so the same
-/// bounded shutdown shape applies whether the reap runs inline or on
-/// a blocking task. Returns once the child has been reaped or the
-/// budget expires; the OS reaps any survivor when sofos exits.
+/// Kill and reap with a bounded `try_wait` loop. Shared by `Drop`
+/// and the detached kill path so neither blocks the executor on a
+/// child stuck in uninterruptible IO. The OS reaps any survivor
+/// when sofos exits.
 fn kill_and_reap_bounded(child: &mut Child) {
     let _ = child.kill();
     let start = std::time::Instant::now();
@@ -351,10 +343,8 @@ impl StdioClient {
         let args = config.args.unwrap_or_default();
         let env_vars = config.env.unwrap_or_default();
 
-        // Spawn the child on a blocking worker. `Command::spawn` performs
-        // synchronous syscalls (fork+exec / CreateProcess) that on a slow
-        // filesystem or under load can pause the tokio executor for tens
-        // to hundreds of milliseconds.
+        // Spawn off the executor — `Command::spawn` is synchronous and
+        // can pause tokio noticeably on slow filesystems.
         let spawn_name = server_name.clone();
         let process = tokio::task::spawn_blocking(move || -> std::io::Result<Child> {
             spawn_stdio_child(&command, &args, &env_vars)

src/repl/mod.rs

@@ -364,10 +364,8 @@ impl Repl {
         let new_session_id = self.history_manager.generate_unique_session_id();
         self.session_state.conversation.clear();
         self.session_state.clear(new_session_id);
-        // The executor stays in safe mode across `/clear`, so the model
-        // also needs to keep seeing the safe-mode preamble. Without
-        // this it confidently proposes write/bash and gets opaque
-        // denials from the tool layer.
+        // Safe mode survives `/clear`, so the preamble has to ride
+        // along too — otherwise the model proposes blocked tools.
         if self.safe_mode {
             self.session_state
                 .conversation
@@ -427,11 +425,8 @@ impl Repl {
             println!();
             return;
         }
-        // Anthropic's legacy thinking model rejects requests where
-        // `max_tokens` is below the configured budget ceiling. Startup
-        // refuses that combination outright (see `new` above); the same
-        // gate must fire on mid-session `/effort high` so the next
-        // turn doesn't 400 with no clear hint at the cause.
+        // Mirror the startup gate: legacy Anthropic thinking needs
+        // max_tokens above the budget ceiling.
         if matches!(self.client, Anthropic(_))
             && !self.uses_adaptive_thinking()
             && effort.is_enabled()

src/repl/response_handler.rs

@@ -217,13 +217,9 @@ impl ResponseHandler {
             }
 
             if tool_uses.is_empty() {
-                // Drain any in-flight steer messages even when the
-                // assistant only produced text. The follow-up branch
-                // below folds them into the tool-results turn; without
-                // mirroring the drain here, a text-only turn leaves
-                // the steer buffered and the next prompt fires as a
-                // fresh user turn — losing the "mid-turn delivery"
-                // association the user expected.
+                // Drain pending steer messages on a text-only turn so
+                // they aren't lost between this reply and the next
+                // user prompt.
                 if let Some(steer_text) = self.drain_steer_messages() {
                     println!(
                         "{} {}",
@@ -641,15 +637,9 @@ impl ResponseHandler {
             content: "[System: Maximum tool iterations reached]".to_string(),
         });
 
-        // Let the assistant respond to the interruption. Use
-        // `run_interruptible` so ESC during this final summary cancels
-        // the HTTP call instead of blocking on the server.
-        //
-        // The recovery request is built without tools. Sending the
-        // tools array would let the assistant come back with another
-        // `tool_use` block; persisting that block without the matching
-        // `tool_result` puts the session in a shape the provider
-        // rejects on the next request and breaks `--resume`.
+        // Recovery turn is text-only: clearing `tools` stops the
+        // assistant from emitting a `tool_use` we'd have to discard
+        // or pair with a synthetic result before saving.
         let mut request = self.build_request();
         request.tools = None;
         let client = self.client.clone();
@@ -681,11 +671,8 @@ impl ResponseHandler {
                     }
                 }
 
-                // Even though the recovery request carried no tools,
-                // strip any tool-related blocks defensively before
-                // appending. A provider that ignores `tools = None`
-                // (or returns a cached tool_use from a previous turn)
-                // would otherwise leave an orphan in the saved session.
+                // Defensive strip in case the provider returns a cached
+                // tool_use despite `tools = None`.
                 let message_blocks: Vec<crate::api::MessageContentBlock> = response
                     .content
                     .iter()

src/repl/tui/event_loop.rs

@@ -221,15 +221,9 @@ pub(super) async fn event_loop(
                 UiEvent::WorkerShutdown(summary) => {
                     app.exit_summary = Some(summary);
                     app.should_quit = true;
-                    // Unblock a worker that is parked on `reply_rx.recv()`
-                    // for a confirmation modal. Without this, the
-                    // `worker_handle.thread.join()` in `mod.rs` would
-                    // hang forever: the worker is waiting for the
-                    // modal's responder, but the event loop is about
-                    // to drop `app` (and the responder with it) only
-                    // after the join returns. Sending the default
-                    // index back unblocks the worker exactly as if
-                    // the user had pressed Esc.
+                    // Unblock any worker parked on a confirmation modal
+                    // — without this, `thread.join()` would deadlock
+                    // because the responder lives inside `app`.
                     if let Some(prompt) = app.confirmation.take() {
                         let _ = prompt.responder.send(prompt.default_index);
                     }

src/repl/tui/mod.rs

@@ -43,21 +43,14 @@ use input::spawn_input_reader;
 use keymap::install_confirm_handler;
 use output::OutputCapture;
 
-/// Real-tty handle saved by [`install_panic_hook`] so the hook can
-/// route restoration sequences past any `OutputCapture` redirection
-/// still in place at panic time. `OnceLock` because the hook is
-/// installed once per process and the handle never needs to change
-/// afterwards.
+/// Real-tty handle saved for the panic hook so its restoration
+/// sequences bypass any `OutputCapture` pipe in place at panic time.
 static PANIC_TTY: OnceLock<Mutex<std::fs::File>> = OnceLock::new();
 
-/// Install a process-global panic hook that restores the terminal
-/// before chaining to the previous hook. Best-effort on every step:
-/// raw mode comes off, bracketed paste is disabled, kitty keyboard
-/// enhancement flags are popped, and the cursor is shown — written
-/// through [`PANIC_TTY`] (when set) so output reaches the real
-/// terminal even if `OutputCapture` has redirected stdout into a
-/// pipe. Chaining to the previous hook keeps the backtrace flowing
-/// to stderr so the user can still file a useful bug report.
+/// Process-global panic hook: disable raw mode, disable bracketed
+/// paste, pop kitty keyboard flags, show the cursor — all through
+/// [`PANIC_TTY`] when set — then chain to the previous hook so the
+/// backtrace still prints.
 fn install_panic_hook() {
     static INIT: std::sync::Once = std::sync::Once::new();
     INIT.call_once(|| {
@@ -233,10 +226,8 @@ pub fn run(mut repl: Repl) -> Result<()> {
     #[cfg(windows)]
     let tty = OpenOptions::new().write(true).open("CONOUT$")?;
     let tty_for_backend = tty.try_clone()?;
-    // Stash a real-tty handle for the panic hook so a panic between
-    // here and the normal teardown can still write disable-raw-mode +
-    // disable-bracketed-paste + show-cursor through a writer that
-    // bypasses the upcoming output-capture redirection.
+    // Save a real-tty handle for the panic hook before output capture
+    // redirects stdout.
     if let Ok(tty_for_panic) = tty.try_clone() {
         let _ = PANIC_TTY.set(Mutex::new(tty_for_panic));
     }
@@ -343,11 +334,9 @@ pub fn run(mut repl: Repl) -> Result<()> {
 
     if let Some(summary) = app.exit_summary.take() {
         if summary.panicked {
-            // The worker exited via panic rather than the normal
-            // shutdown path. The counts in `summary` are zeroed
-            // placeholders from `ShutdownSender::send_now`, so the
-            // session-summary table would be misleading; surface the
-            // explicit notice and skip straight to the goodbye line.
+            // Counts are zeroed placeholders on the panic path, so
+            // surface the cause explicitly before the (suppressed)
+            // summary table.
             println!();
             UI::print_warning("Session ended unexpectedly. See backtrace above for details.");
         }

src/repl/tui/output.rs

@@ -311,11 +311,20 @@ fn spawn_reader(
             // before sending so the batch-joiner sees a self-contained
             // styled line.
             let mut state = SgrState::default();
+            // True when the previous read returned without a newline
+            // (cap reached). The next read prepends a `[…continued]`
+            // marker so the user can tell two consecutive captured
+            // lines logically belong to the same original output.
+            let mut previous_was_capped = false;
             loop {
                 line.clear();
                 match read_until_or_cap(&mut buf, b'\n', &mut line, READER_CHUNK_BYTES) {
                     Ok(0) => break,
                     Ok(_) => {
+                        // Whether THIS read ended at the cap rather
+                        // than on a real line terminator. Checked
+                        // before the trailing-newline strip below.
+                        let this_capped = !matches!(line.last(), Some(b'\n') | Some(b'\r'));
                         // Log the *raw* bytes — including the `\n` /
                         // `\r` suffix — before any stripping. That
                         // way the log shows exactly what arrived in
@@ -341,6 +350,9 @@ fn spawn_reader(
                         // resolved-state prefix is still accurate
                         // across the drop.
                         strip_trailing_incomplete_csi(&mut text);
+                        if previous_was_capped {
+                            text.insert_str(0, "[…continued] ");
+                        }
                         let prefix = state.to_ansi_prefix();
                         state.apply(&text);
                         let payload = if prefix.is_empty() {
@@ -360,6 +372,7 @@ fn spawn_reader(
                         {
                             break;
                         }
+                        previous_was_capped = this_capped;
                     }
                     Err(_) => break,
                 }

src/repl/tui/slash_popup.rs

@@ -109,9 +109,16 @@ impl SlashPopup {
         // text begins with the current input (backspaced). Switching
         // to an unrelated command (e.g. dismiss `/clear`, then type
         // `/list`) breaks both prefix tests and re-opens the popup.
-        if let Some(prev) = self.dismissed_for.as_deref() {
-            if prev == input || input.starts_with(prev) || prev.starts_with(input) {
-                return;
+        //
+        // An empty input fully resets the dismissal: every dismissed
+        // string trivially starts with "", which would otherwise keep
+        // the popup suppressed even after the textarea has been
+        // emptied and a fresh `/` is typed.
+        if !input.is_empty() {
+            if let Some(prev) = self.dismissed_for.as_deref() {
+                if prev == input || input.starts_with(prev) || prev.starts_with(input) {
+                    return;
+                }
             }
         }
         self.dismissed_for = None;