Harden REPL state on tool-cap, /clear, /effort, resume, and atomic writes

Author: alexylon

CHANGELOG.md

@@ -4,6 +4,16 @@ All notable changes to Sofos are documented in this file.
 
 ## [Unreleased]
 
+### Fixed
+
+- **Hitting the tool-iteration cap no longer corrupts the saved session.** The recovery turn used to ship with the tools list still attached, so the assistant could answer with a fresh `tool_use` block that was never executed; the next request 400'd and `--resume` was dead on arrival. Sofos now strips tools from that final summary request and removes any tool-related blocks it returns defensively, so the session resumes cleanly afterwards.
+- **`/clear` keeps safe mode visible to the model.** Clearing the history used to strip the "you are running in safe mode" preamble even when the executor was still in safe mode, so the model proposed writes and bash and was met with opaque denials. The preamble is re-added automatically when safe mode is still on.
+- **Switching reasoning effort mid-session refuses combinations the next request would reject.** On the Anthropic legacy thinking models, `/effort high` (or any enabled level) requires `--max-tokens` above 16 384; the gate that startup enforces now also fires from `/effort`, so the next turn doesn't 400 with no clear hint at the cause.
+- **Resuming a session that was killed mid-tool-call no longer fails the first request.** A hard kill between writing the assistant's `tool_use` to disk and writing the matching `tool_result` used to leave the saved history in a shape the provider rejects on resume; sofos now strips the orphan tool-use block on load and replaces it with a short "[Tool call interrupted before execution]" placeholder.
+- **Slash commands now save their effect to the session.** Running `/compact`, `/clear`, `/safe`, or `/normal` and quitting via Ctrl+C without another prompt used to lose the change; the worker now persists the session after every slash command, not only after free-text turns.
+- **`write_file` / `edit_file` survive a power loss between the rename and writeback.** The atomic-write helper now `fsync`s the temp file before the rename and (on Unix) the parent directory after, so ext4/xfs in their default mount options can no longer leave the target with the new inode but zero data after a kernel crash.
+- **Moving a file across filesystems no longer fails outright.** `move_file` used to surface a raw "Invalid cross-device link" error when source and destination lived on different filesystems (macOS APFS volumes, Linux `/tmp` mounts, external drives). Sofos now detects the cross-device case and falls back to a copy + delete so the operation completes.
+
 ### Security
 
 - **MCP server children no longer inherit the sofos environment.** Stdio MCP servers used to start with every variable the parent shell exported, so `ANTHROPIC_API_KEY`, `OPENAI_API_KEY`, `MORPH_API_KEY`, ssh agent sockets, and AWS credentials all reached the child. The launcher now clears the environment first and forwards a small allowlist (`PATH`, `HOME` / `USERPROFILE`, `TMPDIR` / `TEMP` / `TMP`, `LANG`, every `LC_*` locale variable, plus Windows essentials like `SYSTEMROOT` and `PATHEXT`); anything else needs to be listed under the server's `env` config field. Existing servers that already declare their required vars in config are unaffected.

src/repl/conversation/lifecycle.rs

@@ -213,6 +213,51 @@ impl ConversationHistory {
         )
     }
 
+    /// Drop tool-use blocks from the tail assistant message when no
+    /// matching tool-result follows. Triggered by `restore_messages`
+    /// to guard against a session file written between
+    /// `add_assistant_with_blocks` and `add_tool_results` — a hard
+    /// kill at that moment would otherwise leave the resumed
+    /// conversation with an orphan that the provider 400s on the
+    /// next request.
+    pub(super) fn drop_tail_orphaned_tool_uses(&mut self) -> bool {
+        let Some(last) = self.messages.last_mut() else {
+            return false;
+        };
+        if last.role != "assistant" {
+            return false;
+        }
+        let crate::api::MessageContent::Blocks { content } = &mut last.content else {
+            return false;
+        };
+        let had_initiator = content.iter().any(|b| {
+            matches!(
+                b,
+                crate::api::MessageContentBlock::ToolUse { .. }
+                    | crate::api::MessageContentBlock::ServerToolUse { .. }
+                    | crate::api::MessageContentBlock::WebSearchToolResult { .. }
+            )
+        });
+        if !had_initiator {
+            return false;
+        }
+        content.retain(|b| {
+            !matches!(
+                b,
+                crate::api::MessageContentBlock::ToolUse { .. }
+                    | crate::api::MessageContentBlock::ServerToolUse { .. }
+                    | crate::api::MessageContentBlock::WebSearchToolResult { .. }
+            )
+        });
+        if content.is_empty() {
+            content.push(crate::api::MessageContentBlock::Text {
+                text: "[Tool call interrupted before execution]".to_string(),
+                cache_control: None,
+            });
+        }
+        true
+    }
+
     /// Build a brief summary of messages about to be dropped (no LLM, just key facts).
     pub(super) fn build_drop_summary(messages: &[Message]) -> String {
         let mut tools_used = Vec::new();

src/repl/conversation/messages.rs

@@ -91,6 +91,11 @@ impl ConversationHistory {
         // any inherited anchor index is meaningless content-wise.
         self.invalidate_cache_anchor();
         self.messages = messages;
+        // A session file written between `add_assistant_with_blocks`
+        // and `add_tool_results` carries a tail assistant message with
+        // unmatched tool_use blocks. Resuming that history would 400 on
+        // the next request, so strip the orphan blocks now.
+        self.drop_tail_orphaned_tool_uses();
         self.trim_if_needed();
     }
 }

src/repl/mod.rs

@@ -354,6 +354,15 @@ 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.
+        if self.safe_mode {
+            self.session_state
+                .conversation
+                .add_user_message(SAFE_MODE_MESSAGE.to_string());
+        }
         self.session_state
             .conversation
             .add_user_message("The session history has been cleared".to_string());
@@ -408,6 +417,27 @@ 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.
+        if matches!(self.client, Anthropic(_))
+            && !self.uses_adaptive_thinking()
+            && effort.is_enabled()
+            && self.model_config.max_tokens <= crate::api::anthropic::LEGACY_THINKING_BUDGET_HIGH
+        {
+            println!();
+            UI::print_error(&format!(
+                "Cannot enable extended thinking on the legacy budget — max_tokens \
+                 ({}) must exceed {}. Relaunch with a higher --max-tokens or pick a \
+                 lower effort.",
+                self.model_config.max_tokens,
+                crate::api::anthropic::LEGACY_THINKING_BUDGET_HIGH
+            ));
+            println!();
+            return;
+        }
         self.model_config.set_reasoning_effort(effort);
         self.print_reasoning_state();
     }

src/repl/response_handler.rs

@@ -232,7 +232,7 @@ impl ResponseHandler {
             }
 
             let (tool_results, user_cancelled) =
-                self.execute_tools(&tool_uses, display_messages).await?;
+                self.execute_tools(&tool_uses, display_messages).await;
 
             if !tool_results.is_empty() {
                 if std::env::var("SOFOS_DEBUG").is_ok() {
@@ -367,11 +367,19 @@ impl ResponseHandler {
         (text_output, tool_uses, had_reasoning)
     }
 
+    /// Run every tool in the assistant's `tool_use` batch and return
+    /// a `ToolResult` for each one — including a synthesised note for
+    /// any tool we skip after a cancellation. Returns `(results,
+    /// user_cancelled)` directly, without a `Result`, so a caller
+    /// cannot accidentally short-circuit with `?` and leave a
+    /// half-batch on the wire: every `ToolUse` MUST be paired with a
+    /// matching `ToolResult` on the immediate next user turn or the
+    /// provider 400s, and the saved session loads dead.
     async fn execute_tools(
         &self,
         tool_uses: &[(String, String, serde_json::Value)],
         display_messages: &mut Vec<DisplayMessage>,
-    ) -> Result<(Vec<crate::api::MessageContentBlock>, bool)> {
+    ) -> (Vec<crate::api::MessageContentBlock>, bool) {
         let mut tool_results = Vec::new();
         let mut user_cancelled = false;
 
@@ -514,7 +522,7 @@ impl ResponseHandler {
             }
         }
 
-        Ok((tool_results, user_cancelled))
+        (tool_results, user_cancelled)
     }
 
     async fn get_next_response(&mut self) -> Result<crate::api::CreateMessageResponse> {
@@ -624,7 +632,14 @@ impl ResponseHandler {
         // 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.
-        let request = self.build_request();
+        //
+        // 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`.
+        let mut request = self.build_request();
+        request.tools = None;
         let client = self.client.clone();
         let response_result = self
             .run_interruptible(async move { client.create_message(request).await })
@@ -654,10 +669,23 @@ 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.
                 let message_blocks: Vec<crate::api::MessageContentBlock> = response
                     .content
                     .iter()
                     .map(crate::api::MessageContentBlock::from_content_block_for_api)
+                    .filter(|block| {
+                        !matches!(
+                            block,
+                            crate::api::MessageContentBlock::ToolUse { .. }
+                                | crate::api::MessageContentBlock::ServerToolUse { .. }
+                                | crate::api::MessageContentBlock::WebSearchToolResult { .. }
+                        )
+                    })
                     .collect();
                 if !message_blocks.is_empty() {
                     self.conversation.add_assistant_with_blocks(message_blocks);

src/repl/tui/worker.rs

@@ -151,6 +151,14 @@ fn run(
                         }
                     }
                 }
+                // Slash commands can mutate the conversation (`/compact`
+                // rewrites the history, `/clear` resets it, `/safe` /
+                // `/normal` toggle the mode preamble). Persist after the
+                // command runs so a `/exit` or Ctrl+C before the next
+                // prompt doesn't lose the change.
+                if let Err(e) = repl.save_current_session() {
+                    UI::print_warning(&format!("Failed to save session: {}", e));
+                }
                 flush_captured_streams();
                 let _ = ui_tx.send(UiEvent::Status(repl.status_snapshot()));
                 let _ = ui_tx.send(UiEvent::WorkerIdle);

src/tools/executor.rs

@@ -241,12 +241,13 @@ fn move_between(
         return fs_tool.move_file(source, destination);
     }
     ensure_parent_dir(&dst.canonical)?;
-    std::fs::rename(&src.canonical, &dst.canonical).map_err(|e| {
-        SofosError::ToolExecution(format!(
-            "Failed to move '{}' to '{}': {}",
-            source, destination, e
-        ))
-    })
+    crate::tools::filesystem::rename_with_cross_device_fallback(&src.canonical, &dst.canonical)
+        .map_err(|e| {
+            SofosError::ToolExecution(format!(
+                "Failed to move '{}' to '{}': {}",
+                source, destination, e
+            ))
+        })
 }
 
 /// Copy-file counterpart to [`move_between`]. Same inside/outside matrix;

src/tools/filesystem.rs

@@ -55,6 +55,15 @@ fn write_atomic(path: &Path, content: &str) -> std::io::Result<()> {
         let _ = fs::remove_file(&tmp_path);
         return Err(e);
     }
+    // Flush the file content to stable storage BEFORE the rename. On a
+    // kernel crash between rename and writeback ext4/xfs default mount
+    // options can otherwise leave the target file with the new inode
+    // but zero data, replacing valid content with an empty file.
+    if let Err(e) = tmp_file.sync_all() {
+        drop(tmp_file);
+        let _ = fs::remove_file(&tmp_path);
+        return Err(e);
+    }
     drop(tmp_file);
 
     // Preserve the existing file's permission bits. Best-effort: if
@@ -74,9 +83,64 @@ fn write_atomic(path: &Path, content: &str) -> std::io::Result<()> {
         let _ = fs::remove_file(&tmp_path);
         return Err(e);
     }
+
+    // Flush the directory entry on Unix so the rename itself survives a
+    // crash. Without this, ext4/xfs in `data=ordered` mode can lose
+    // the directory update on power-loss even though the file content
+    // already made it to disk. Best-effort: `sync_all` on a closed
+    // directory handle isn't supported on every platform, and on
+    // Windows the call would be inappropriate.
+    #[cfg(unix)]
+    if let Some(parent) = target.parent() {
+        if let Ok(dir) = fs::File::open(parent) {
+            let _ = dir.sync_all();
+        }
+    }
     Ok(())
 }
 
+/// True when `e` describes a rename that crossed a filesystem
+/// boundary. Uses the stable `ErrorKind::CrossesDevices` mapping
+/// first; falls back to the platform-specific raw code so a future
+/// Rust that drops the kind mapping still works.
+fn is_cross_device_error(e: &std::io::Error) -> bool {
+    if e.kind() == std::io::ErrorKind::CrossesDevices {
+        return true;
+    }
+    #[cfg(unix)]
+    {
+        e.raw_os_error() == Some(libc::EXDEV)
+    }
+    #[cfg(windows)]
+    {
+        // ERROR_NOT_SAME_DEVICE
+        e.raw_os_error() == Some(17)
+    }
+    #[cfg(not(any(unix, windows)))]
+    {
+        false
+    }
+}
+
+/// Move `src` to `dst`, falling back to copy + delete when the rename
+/// crosses a filesystem boundary (`EXDEV` on Unix,
+/// `ERROR_NOT_SAME_DEVICE` on Windows). The fallback isn't atomic —
+/// a crash between the copy and the delete leaves both files — but
+/// the alternative is a hard error for what is otherwise a common
+/// operation (move into `/tmp` on macOS APFS, move across mounts on
+/// Linux), so the tradeoff is worth it.
+pub fn rename_with_cross_device_fallback(src: &Path, dst: &Path) -> std::io::Result<()> {
+    match fs::rename(src, dst) {
+        Ok(()) => Ok(()),
+        Err(e) if is_cross_device_error(&e) => {
+            fs::copy(src, dst)?;
+            fs::remove_file(src)?;
+            Ok(())
+        }
+        Err(e) => Err(e),
+    }
+}
+
 /// Reserve a fresh `<target>.sofos.tmp.<hex>` sibling file using
 /// `O_EXCL`-style exclusive create, returning both the path and the
 /// open handle so the caller writes through the handle we just owned
@@ -431,7 +495,7 @@ impl FileSystemTool {
             }
         }
 
-        fs::rename(&source_path, &dest_path)?;
+        rename_with_cross_device_fallback(&source_path, &dest_path)?;
         Ok(())
     }
 
@@ -493,6 +557,67 @@ mod tests {
         assert!(fs_tool.validate_path("foo..bar/baz..qux.txt").is_ok());
     }
 
+    #[test]
+    fn write_atomic_replaces_existing_content_durably() {
+        // Smoke test: write_atomic must produce a readable file with the
+        // exact bytes passed in, with no leftover `.sofos.tmp.*` siblings
+        // in the directory after a successful write. `sync_all` runs
+        // before the rename — failing to flush would leave the file
+        // empty on a crash, which is the regression we're guarding
+        // against.
+        let (_temp, path) = test_support::workspace();
+        let fs_tool = FileSystemTool::new(path.clone()).unwrap();
+        fs_tool.write_file("doc.md", "first").unwrap();
+        fs_tool.write_file("doc.md", "second pass").unwrap();
+        let contents = fs_tool.read_file("doc.md").unwrap();
+        assert_eq!(contents, "second pass");
+
+        // No tmp file should survive a successful write.
+        let leftover: Vec<_> = std::fs::read_dir(&path)
+            .unwrap()
+            .filter_map(|e| e.ok())
+            .filter(|e| e.file_name().to_string_lossy().contains(".sofos.tmp."))
+            .collect();
+        assert!(
+            leftover.is_empty(),
+            "no .sofos.tmp.* artefact should remain"
+        );
+    }
+
+    #[cfg(unix)]
+    #[test]
+    fn cross_device_move_falls_back_to_copy_remove() {
+        // `fs::rename` returns `EXDEV` across filesystems. Modern macOS
+        // (every `/tmp` is a separate APFS volume from the user's home)
+        // and Linux containers (`/dev/shm` vs `/tmp`) both hit this in
+        // practice. The fallback must succeed where rename can't.
+        //
+        // We can't reliably stage two devices in unit-tests, so we go
+        // straight at `is_cross_device_error` + `rename_with_cross_device_fallback`
+        // on a same-device move and verify it still works (the
+        // fast-path) — the slow-path is exercised by hand on macOS
+        // before each release.
+        let (_temp, path) = test_support::workspace();
+        let fs_tool = FileSystemTool::new(path).unwrap();
+        fs_tool.write_file("src.txt", "hello").unwrap();
+        fs_tool.move_file("src.txt", "dst.txt").unwrap();
+        let contents = fs_tool.read_file("dst.txt").unwrap();
+        assert_eq!(contents, "hello");
+    }
+
+    #[test]
+    fn cross_device_error_classifier_matches_kind() {
+        // `ErrorKind::CrossesDevices` is the canonical signal from
+        // 1.85+; any future toolchain that drops the mapping is caught
+        // by the platform-specific raw-error fallback.
+        let e = std::io::Error::from(std::io::ErrorKind::CrossesDevices);
+        assert!(is_cross_device_error(&e));
+
+        // Unrelated IO errors must NOT trigger the copy+remove path.
+        let other = std::io::Error::from(std::io::ErrorKind::PermissionDenied);
+        assert!(!is_cross_device_error(&other));
+    }
+
     #[test]
     fn append_file_creates_then_appends_across_calls() {
         let (_temp, path) = test_support::workspace();