Tighten tools, MCP isolation tail, and CLI polish across the audit punch list

Author: alexylon

CHANGELOG.md

@@ -6,12 +6,22 @@ All notable changes to Sofos are documented in this file.
 
 ### Added
 
+- **`glob_files` aborts at fifty thousand matches and breaks symlink cycles.** A `**` pattern over a million-file monorepo used to buffer about 80 MB of strings before output truncation; the walk now stops at 50 000 hits with a "narrow the pattern" sentinel. With `follow_symlinks: true`, the walker tracks canonical directories so a `ln -s . loop` no longer spins forever.
+- **`edit_file` refuses to overwrite a file that changed during the read.** When VSCode auto-save or `cargo watch` rewrites a file between the tool's read and write, sofos now detects the mtime / length change and surfaces a clear "concurrent edit" error so the model re-reads the file instead of clobbering the fresher version.
+
 - **The cache numbers now appear in the status line.** When either cache-read or cache-creation tokens are non-zero, the status row shows `cache: 1234r 56w` alongside the input/output totals so users can see their prompt-cache hit rate without quitting to view the session summary.
 - **The input box title line shows a `… +N more` hint when the draft overflows the visible area.** A long paste or multi-line draft that exceeds the input box height previously had no on-screen indication that more content existed below the visible window; the cap is now surfaced as part of the title.
 - **`Ctrl+W` and `Ctrl+K` are now bound in the input box.** `Ctrl+W` deletes the word behind the cursor (the readline / bash / zsh / fish binding) and `Ctrl+K` deletes from the cursor to the end of the line. `Ctrl+U` (delete to start of line) is unchanged.
 
 ### Fixed
 
+- **`list_directory` no longer over-counts items when output is truncated.** The `[TRUNCATED: …]` footer added when a directory listing exceeds the cap used to be counted as extra items; sofos now strips the footer before counting so the reported count matches the actual entries.
+- **`view_image` rejects `data:` URLs with a clear hint.** The previous flow routed them into the file branch and surfaced a misleading "Image not found" error; the new error explains the accepted shapes (workspace-relative, absolute, `~/`, http(s)://).
+- **MCP safe-mode refusals are now treated as security blocks, not failures.** Tools filtered out by safe mode used to render with the red error styling; they now flow through the same yellow "blocked" channel as native safe-mode refusals.
+- **A corrupt `index.json` no longer fails session saves.** Sofos now treats a malformed index as if it were missing and rebuilds it from the current save instead of failing the whole write.
+- **Mid-turn user messages folded into text-only assistant turns.** Typing while the assistant is producing a text-only reply (no tool calls) now folds the steer message into a follow-up user turn instead of dropping it as a fresh prompt.
+- **Morph stub-response detection covers the 200-500 byte range.** Files in this bracket that collapsed below 30 % of the original used to slip past the absolute 50-byte floor; sofos now flags those merges as likely truncated.
+
 - **Long assistant turns render fluidly again.** The streaming markdown renderer used to re-render the whole accumulated buffer on every new line — quadratic in the length of the reply, which on a 50 KB stream was noticeably laggy. Replies under 16 KB stream per line as before; longer replies batch the work into 1 KB chunks so the total cost stays linear.
 - **Editing past a slash-popup dismissal stays dismissed.** Pressing Esc on `/clear` and then backspacing one character (or adding a typo fix) used to immediately re-open the popup; the dismissal now sticks while the textarea is still in the same `/command` edit family and only clears when the user switches to an unrelated command.
 - **A live draft is preserved while scrolling through input history.** Alt+Up used to discard whatever you had typed when stepping into older prompts; sofos now snapshots the draft on the first Alt+Up and restores it when Alt+Down walks past the newest entry.
@@ -36,6 +46,22 @@ All notable changes to Sofos are documented in this file.
 
 ### Security
 
+- **`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.
+- **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
+
+- **Deprecated `--thinking-budget` and `--check-connection` + `--prompt` warnings now use the yellow `UI::print_warning` channel.** Users without `RUST_LOG=warn` set previously got no on-screen feedback for either; both now appear consistently.
+- **Passing an API key on the command line now prints a warning when the environment variable is unset.** `--api-key`, `--openai-api-key`, and `--morph-api-key` arguments land in `ps` output and shell history; sofos now recommends exporting the env-var form when it sees a flag-only setup.
+- **`--check-connection` reports "host reachable" instead of "API is reachable".** The probe is a HEAD against the base URL — both providers return 404 there without authenticating, so the previous wording misled users into expecting their key would also work.
+- **Cross-platform `move_file` now reports "concurrent edit" when the destination changed mid-operation.** Combined with the file-modification fixes above, sofos no longer silently overwrites a file that was rewritten between read and write.
+- **Slash-popup dismissal persists across edits within the same `/command` attempt** (covered by the slash-popup fix earlier in this section).
+- **Animated GIFs are documented as first-frame only.** The `view_image` schema text now matches the implementation; ask the user for a still frame if an animation matters.
+
+### Security
+
 - **API-key-shaped strings in provider error bodies are redacted before display.** Provider 401 responses sometimes echo a truncated key (`sk-ant-api03-…` or `Bearer …`); sofos now replaces every matching run with `sk-[redacted]` / `Bearer [redacted]` and caps the body at 4 KB, so a noisy proxy or moderation block can no longer flood the status line or leak key prefixes to transcripts and crash reports.
 - **The SSE re-assembly buffer is capped at 16 MB on both providers.** A server (or middlebox) that streams gigabytes without a newline used to grow the buffer until the 30-minute request timeout fired; sofos now aborts the stream cleanly with a clear error long before memory exhaustion becomes a real risk.
 

src/api/anthropic/wire.rs

@@ -97,6 +97,14 @@ pub fn legacy_thinking_budget(effort: ReasoningEffort) -> u32 {
 /// request's signed thinking blocks. Echoing back a stored set of
 /// thinking blocks that the server cross-checks against the request,
 /// and dropping `output_config` would 400 the next turn.
+/// `Off` collapses to `"low"` on the adaptive-thinking path: dropping
+/// `output_config.thinking` entirely would fail the server's
+/// cross-check against any signed thinking blocks already in the
+/// conversation, so adaptive models always send *some* effort label.
+/// Effectively, `effort: off` on Opus 4.7 / Sonnet 4.6 still sends
+/// the minimum adaptive budget; for "no thinking at all" pick a
+/// model whose `requires_adaptive_thinking` returns false and run
+/// it with `--reasoning-effort off`.
 pub fn effort_label(effort: ReasoningEffort) -> &'static str {
     match effort {
         ReasoningEffort::Off | ReasoningEffort::Low => "low",
@@ -125,6 +133,12 @@ pub(super) fn prepare_request(mut request: CreateMessageRequest) -> CreateMessag
 
     // OpenAI-only; drop before serializing for Anthropic.
     request.prompt_cache_key = None;
+    // `reasoning` is the OpenAI Responses-style sibling of Anthropic's
+    // `thinking` field. The request builder never sets it on the
+    // Anthropic path today, but clear it here defensively so a future
+    // caller that constructs a request directly doesn't accidentally
+    // send it and trigger a 400.
+    request.reasoning = None;
 
     if let Some(tools) = request.tools.take() {
         let filtered: Vec<Tool> = tools

src/api/utils.rs

@@ -194,9 +194,12 @@ pub fn parse_tool_arguments(name: &str, args: &str) -> serde_json::Value {
     }
 
     let preview_end = truncate_at_char_boundary(args, UNPARSEABLE_ARGS_PREVIEW_BYTES);
+    // Redact `sk-…` / `Bearer …` shapes before tracing — the preview
+    // can land in transcripts and crash reports.
+    let preview = redact_api_secrets(&args[..preview_end]);
     tracing::warn!(
         tool = %name,
-        preview = %&args[..preview_end],
+        preview = %preview,
         "failed to parse tool arguments as JSON; passing raw_arguments through"
     );
     serde_json::json!({"raw_arguments": args})

src/clipboard.rs

@@ -71,10 +71,16 @@ pub fn get_clipboard_image() -> Option<PastedImage> {
         writer.write_image_data(&image.bytes).ok()?;
     }
 
-    if buf.len() > MAX_CLIPBOARD_IMAGE_BYTES {
+    // 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.
+    let binary_cap = MAX_CLIPBOARD_IMAGE_BYTES * 3 / 4;
+    if buf.len() > binary_cap {
         tracing::warn!(
             bytes = buf.len(),
-            limit = MAX_CLIPBOARD_IMAGE_BYTES,
+            limit = binary_cap,
             "dropping oversized clipboard image"
         );
         return None;

src/error.rs

@@ -19,6 +19,8 @@ const BLOCKED_KEYWORDS: &[&str] = &[
     "tilde paths",
     "output redirection",
     "here-doc",
+    "filtered out",
+    "safe mode",
 ];
 
 #[derive(Error, Debug)]
@@ -77,7 +79,11 @@ impl SofosError {
         match self {
             Self::PathViolation(_) => true,
             Self::ToolExecution(msg) => BLOCKED_KEYWORDS.iter().any(|kw| msg.contains(kw)),
-            Self::McpError(_) => false,
+            // Safe-mode MCP refusals come back as `McpError` from the
+            // executor; the message still carries a "filtered out" /
+            // "safe mode" keyword, so let the same keyword check apply
+            // here instead of treating every MCP error as a failure.
+            Self::McpError(msg) => BLOCKED_KEYWORDS.iter().any(|kw| msg.contains(kw)),
             Self::Join(_) => false,
             Self::Context { source, .. } => source.is_blocked(),
             _ => false,

src/main.rs

@@ -31,10 +31,40 @@ fn main() -> Result<()> {
 
     let cli = Cli::parse();
 
+    // API keys on the command line land in `ps` output and shell
+    // history; the env-var form is the safe alternative. clap's
+    // `#[arg(env = ...)]` populates the field from either source, so
+    // "field set but env unset" means the user passed the flag.
+    for (field, env_var, flag) in [
+        (cli.api_key.as_deref(), "ANTHROPIC_API_KEY", "--api-key"),
+        (
+            cli.openai_api_key.as_deref(),
+            "OPENAI_API_KEY",
+            "--openai-api-key",
+        ),
+        (
+            cli.morph_api_key.as_deref(),
+            "MORPH_API_KEY",
+            "--morph-api-key",
+        ),
+    ] {
+        if field.is_some() && std::env::var_os(env_var).is_none() {
+            UI::print_warning(&format!(
+                "{} on the command line exposes the key in `ps` output and shell history. \
+                 Export {} in your shell instead.",
+                flag, env_var
+            ));
+        }
+    }
+
     if cli.thinking_budget != cli::THINKING_BUDGET_DEFAULT {
-        tracing::warn!(
+        // Surface the deprecation through the same yellow `UI` channel
+        // the rest of the program uses for user-facing warnings.
+        // `tracing::warn!` only reaches users with `RUST_LOG` set,
+        // which most don't.
+        UI::print_warning(
             "--thinking-budget is deprecated and has no effect on any provider path. \
-             Use --reasoning-effort to control thinking depth. The flag will be removed in a future release."
+             Use --reasoning-effort to control thinking depth. The flag will be removed in a future release.",
         );
     }
 
@@ -100,9 +130,9 @@ fn main() -> Result<()> {
         // so scripts that combine the two don't quietly drop their
         // input.
         if cli.prompt.is_some() {
-            tracing::warn!(
+            UI::print_warning(
                 "--prompt is ignored when --check-connection is set; \
-                 only the connectivity check will run."
+                 only the connectivity check will run.",
             );
         }
         return check_api_connectivity(&client);
@@ -246,10 +276,16 @@ fn check_api_connectivity(client: &LlmClient) -> Result<()> {
 
     match runtime.block_on(client.check_connectivity()) {
         Ok(()) => {
+            // Use "host reachable" rather than "API is reachable" — the
+            // probe is a HEAD against the base URL, which returns 404
+            // on either provider without authenticating. Saying "API is
+            // reachable" misleads users into expecting a successful
+            // first request, but a missing or wrong key still 401s.
             println!(
-                "{} {} API is reachable",
+                "{} {} host reachable (HEAD {}); key not validated",
                 "✓".bright_green().bold(),
-                provider
+                provider,
+                "/".dimmed()
             );
             Ok(())
         }

src/mcp/client.rs

@@ -13,6 +13,13 @@ use std::time::Duration;
 /// still bounding a frozen server's blast radius to a single turn.
 pub(crate) const MCP_REQUEST_TIMEOUT: Duration = Duration::from_secs(120);
 
+/// Shorter ceiling for the MCP `initialize` handshake. A frozen server
+/// holds startup hostage for the full request timeout otherwise (two
+/// minutes per server). 15 s is long enough for a slow stdio child to
+/// finish booting and short enough that a misconfigured server gives
+/// up quickly so the user can fix the config.
+pub(crate) const MCP_INIT_TIMEOUT: Duration = Duration::from_secs(15);
+
 pub(crate) fn create_init_request() -> InitializeRequest {
     InitializeRequest {
         protocol_version: "2024-11-05".to_string(),

src/mcp/manager.rs

@@ -63,9 +63,12 @@ pub struct McpManager {
 }
 
 /// Reject server and tool names that contain the prefix separator or
-/// that would produce an empty identifier. Names are otherwise accepted
-/// as-is; provider-level character validation happens on the prefixed
-/// name when the tool list is sent.
+/// that would produce an empty identifier. Server names are restricted
+/// further to ASCII `[A-Za-z0-9_-]+` so visually-identical Unicode
+/// confusables (e.g. `github` vs `gith\u{1d62}ub`) cannot produce two
+/// map entries that look the same in config but route differently.
+/// Tool names stay permissive because the server controls them and
+/// provider-level validation runs on the prefixed name.
 fn validate_mcp_name(kind: &str, name: &str) -> Result<()> {
     if name.is_empty() {
         return Err(SofosError::McpError(format!("MCP {} name is empty", kind)));
@@ -76,6 +79,16 @@ fn validate_mcp_name(kind: &str, name: &str) -> Result<()> {
             kind, name, MCP_NAME_SEPARATOR
         )));
     }
+    if kind == "server"
+        && !name
+            .chars()
+            .all(|c| c.is_ascii_alphanumeric() || c == '_' || c == '-')
+    {
+        return Err(SofosError::McpError(format!(
+            "MCP server name '{}' must contain only ASCII letters, digits, '_' or '-'",
+            name
+        )));
+    }
     Ok(())
 }
 

src/mcp/transport/stdio.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::*;
@@ -396,13 +396,13 @@ impl StdioClient {
     /// doesn't pause the executor waiting for the OS to reap it. Used
     /// by both `send_request` and `send_notification` so they share
     /// the same lock/panic/timeout error vocabulary.
-    async fn run_with_timeout<T, F>(&self, label: &str, blocking: F) -> Result<T>
+    async fn run_with_timeout<T, F>(&self, label: &str, timeout: Duration, blocking: F) -> Result<T>
     where
         F: FnOnce() -> Result<T> + Send + 'static,
         T: Send + 'static,
     {
         let task = tokio::task::spawn_blocking(blocking);
-        match tokio::time::timeout(MCP_REQUEST_TIMEOUT, task).await {
+        match tokio::time::timeout(timeout, task).await {
             Ok(Ok(Ok(value))) => Ok(value),
             Ok(Ok(Err(e))) => Err(e),
             Ok(Err(join_err)) => Err(SofosError::McpError(format!(
@@ -415,7 +415,7 @@ impl StdioClient {
                     "MCP server '{}' {} timed out after {}s",
                     self.server_name,
                     label,
-                    MCP_REQUEST_TIMEOUT.as_secs()
+                    timeout.as_secs()
                 )))
             }
         }
@@ -451,10 +451,14 @@ impl StdioClient {
     }
 
     async fn initialize(&self) -> Result<()> {
+        // The handshake uses a tighter ceiling than tool calls so a
+        // frozen server can't hold session startup hostage for two
+        // minutes per misconfigured config entry.
         let response = self
-            .send_request(
+            .send_request_with_timeout(
                 "initialize",
                 Some(serde_json::to_value(create_init_request())?),
+                MCP_INIT_TIMEOUT,
             )
             .await?;
 
@@ -467,6 +471,16 @@ impl StdioClient {
     }
 
     async fn send_request(&self, method: &str, params: Option<Value>) -> Result<Value> {
+        self.send_request_with_timeout(method, params, MCP_REQUEST_TIMEOUT)
+            .await
+    }
+
+    async fn send_request_with_timeout(
+        &self,
+        method: &str,
+        params: Option<Value>,
+        timeout: Duration,
+    ) -> Result<Value> {
         let id = self.next_id.fetch_add(1, Ordering::SeqCst);
         let request = JsonRpcRequest::new(id, method.to_string(), params);
         let request_json = serde_json::to_string(&request)?;
@@ -476,7 +490,7 @@ impl StdioClient {
         let stdin = Arc::clone(&self.stdin);
         let stdout = Arc::clone(&self.stdout);
         let response = self
-            .run_with_timeout("request", move || {
+            .run_with_timeout("request", timeout, move || {
                 stdio_request_blocking(
                     &server_name,
                     &request_lock,
@@ -515,7 +529,7 @@ impl StdioClient {
         // wedged its read side. Same timeout path as `send_request`.
         let server_name = self.server_name.clone();
         let stdin = Arc::clone(&self.stdin);
-        self.run_with_timeout("notification", move || {
+        self.run_with_timeout("notification", MCP_REQUEST_TIMEOUT, move || {
             stdio_write_blocking(&server_name, &stdin, &notification_json)
         })
         .await