diff --git a/CHANGELOG.md b/CHANGELOG.md index ae5291fa..4611c2fc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -67,14 +67,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **`deepseek doctor --json`** now surfaces a `memory` block (`enabled` / `path` / `file_present`) so operators can verify memory configuration without booting the TUI. -- **Tool-output spillover** (#422 + #423) — large tool outputs spill - to `~/.deepseek/tool_outputs/.txt` (helpers in - `crates/tui/src/tools/truncate.rs`, 7-day boot prune wired into - `run_interactive`). Tool cells with a spilled path render an - inline `full output: ` annotation in live mode so the user - and the model can find the elided tail; transcript-mode replay - omits the annotation because the full output is inline. #500 - (preview pane) is now unblocked and just needs the pager glue. +- **Tool-output spillover** (#422 + #423 + #500) — tool outputs over + 100 KiB now spill to `~/.deepseek/tool_outputs/.txt` from the + engine's tool-execution path. The model receives a 32 KiB head plus + a footer pointing at the spillover file (`Use read_file path=…`), + the tool cell renders an inline `full output: ` annotation in + live mode, and a 7-day boot prune keeps the directory bounded. + Spillover is skipped on error results so the model still sees the + failure message verbatim. The existing tool-details pager surfaces + the truncated head so the user can verify what the model saw. ### Changed - **Sub-agent concurrency cap raised to 10 by default** (#509) — diff --git a/crates/tui/src/core/engine/turn_loop.rs b/crates/tui/src/core/engine/turn_loop.rs index 9208bb48..de40f7a9 100644 --- a/crates/tui/src/core/engine/turn_loop.rs +++ b/crates/tui/src/core/engine/turn_loop.rs @@ -1078,7 +1078,7 @@ impl Engine { let started_at = Instant::now(); tool_tasks.push(async move { - let result = Engine::execute_tool_with_lock( + let mut result = Engine::execute_tool_with_lock( lock, plan.supports_parallel, plan.interactive, @@ -1091,6 +1091,12 @@ impl Engine { ) .await; + // #500: spill outsized output before fanout (mirror + // of the sequential path below). + if let Ok(tool_result) = result.as_mut() { + crate::tools::truncate::apply_spillover(tool_result, &plan.id); + } + let _ = tx_event .send(Event::ToolCallComplete { id: plan.id.clone(), @@ -1347,7 +1353,7 @@ impl Engine { } let started_at = Instant::now(); - let result = if let Some(result_override) = result_override { + let mut result = if let Some(result_override) = result_override { result_override } else { Self::execute_tool_with_lock( @@ -1364,6 +1370,14 @@ impl Engine { .await }; + // #500: spill outsized tool outputs to disk before the + // result fans out to the model context and the UI cell. + // Both consumers see the same truncated content + the + // `spillover_path` metadata pointing at the full file. + if let Ok(tool_result) = result.as_mut() { + crate::tools::truncate::apply_spillover(tool_result, &tool_id); + } + let _ = self .tx_event .send(Event::ToolCallComplete { diff --git a/crates/tui/src/tools/truncate.rs b/crates/tui/src/tools/truncate.rs index ff0e633f..a5f9793f 100644 --- a/crates/tui/src/tools/truncate.rs +++ b/crates/tui/src/tools/truncate.rs @@ -19,27 +19,28 @@ //! (7 days). Prune failures are logged and never fatal — the user //! shouldn't see startup wedge because of a stale tool-output file. //! -//! ## What's NOT here +//! ## Live callers //! -//! Wiring `maybe_spillover` into the actual tool-execution path is -//! tracked by **#423** (UI annotation) and **#500** (preview pane); -//! both want the spillover bytes to exist. This module ships the -//! plumbing so those follow-ups land cleanly without re-litigating -//! the storage decisions. +//! * [`apply_spillover`] — invoked from the engine's tool-execution +//! path (`turn_loop.rs`) so any successful tool result over +//! [`SPILLOVER_THRESHOLD_BYTES`] spills to disk and the model +//! receives a [`SPILLOVER_HEAD_BYTES`] head plus a pointer footer. +//! * Boot prune in `main.rs` deletes files older than +//! [`SPILLOVER_MAX_AGE`]. //! -//! Today the only live caller is the boot prune in `main.rs`. The -//! storage helpers (`write_spillover`, `maybe_spillover`, -//! `spillover_path`) are unused outside of this module's own tests -//! and the `#[allow(dead_code)]` markers below mark them deferred — -//! they get callers when #423 / #500 land. - -#![allow(dead_code)] // storage surface used by #423/#500 follow-ups; tests pin the contract +//! UI-side rendering of the inline `full output: ` annotation +//! is owned by `tui/history.rs::render_spillover_annotation`. The +//! tool-details pager opens the spillover file when the user +//! presses `Alt+V` (or plain `v` with empty composer) on a spilled +//! tool cell. use std::fs; use std::io; use std::path::PathBuf; use std::time::{Duration, SystemTime}; +use crate::tools::spec::ToolResult; + // `Path` is only referenced from helpers gated to test builds. #[cfg(test)] use std::path::Path; @@ -174,6 +175,88 @@ pub fn maybe_spillover( Ok(Some((content[..cut].to_string(), path))) } +/// Inline head retained when [`apply_spillover`] truncates a tool +/// result. 32 KiB is large enough for the model to keep meaningful +/// context (a long stack trace, a `git diff` head, a directory +/// listing of typical depth) without consuming the lion's share of +/// the per-turn context budget. The full output is preserved on +/// disk; the model can `read_file` it back if it needs the tail. +pub const SPILLOVER_HEAD_BYTES: usize = 32 * 1024; + +/// Apply spillover to a tool result in place. If the result's +/// content exceeds [`SPILLOVER_THRESHOLD_BYTES`], writes the full +/// content to a sibling file under `~/.deepseek/tool_outputs/`, +/// replaces `result.content` with a [`SPILLOVER_HEAD_BYTES`] head +/// plus a footer pointing the model at the spillover file, and +/// stamps `metadata.spillover_path` so the UI can render its +/// "full output: …" annotation. +/// +/// Returns the spillover path on success, `None` if no spillover +/// happened (content small enough, error result, write failure). +/// Failures are logged but never bubble up — a tool that produced a +/// result shouldn't be marked failed because the spillover writer +/// couldn't reach disk; we degrade to no-op and the model gets the +/// original (large) content. +/// +/// Error results (`success == false`) are skipped: error messages +/// are typically short, and turning them into a "see file" pointer +/// would just hide the error from the model's reasoning. +pub fn apply_spillover(result: &mut ToolResult, tool_id: &str) -> Option { + if !result.success { + return None; + } + if result.content.len() <= SPILLOVER_THRESHOLD_BYTES { + return None; + } + let total = result.content.len(); + let outcome = match maybe_spillover( + tool_id, + &result.content, + SPILLOVER_THRESHOLD_BYTES, + SPILLOVER_HEAD_BYTES, + ) { + Ok(Some(pair)) => pair, + Ok(None) => return None, + Err(err) => { + tracing::warn!( + target: "spillover", + ?err, + tool_id, + "spillover write failed; passing original content through" + ); + return None; + } + }; + let (head, path) = outcome; + let path_str = path.display().to_string(); + let footer = format!( + "\n\n[Output truncated: {head_kib} KiB of {total_kib} KiB shown. \ + Full output saved to {path_str}. Use `read_file path={path_str}` \ + if you need the elided tail.]", + head_kib = head.len() / 1024, + total_kib = total / 1024, + ); + result.content = format!("{head}{footer}"); + let metadata = result.metadata.get_or_insert_with(|| serde_json::json!({})); + if let Some(obj) = metadata.as_object_mut() { + obj.insert("spillover_path".into(), serde_json::Value::String(path_str)); + } else { + // Pre-existing metadata that wasn't a JSON object (rare, + // possibly an array). Replace with an object so we can + // attach our key without losing prior data — wrap it under + // a `_prior` field so callers that introspect can recover. + let prior = std::mem::replace(metadata, serde_json::json!({})); + if let Some(obj) = metadata.as_object_mut() { + obj.insert("_prior".into(), prior); + obj.insert( + "spillover_path".into(), + serde_json::Value::String(path.display().to_string()), + ); + } + } + Some(path) +} + /// Sanitise a tool call id for use as a filename. Keeps ASCII /// alphanumerics, `-`, and `_`; rejects `.` to keep `..` traversal /// out, rejects empty results. Returns `None` if the input contains @@ -385,4 +468,94 @@ mod tests { // Not exercised in CI on Windows; prune semantics are the same // and the per-cycle stress test lives on the Unix path. } + + #[test] + fn apply_spillover_is_noop_below_threshold() { + let _g = setup(); + let tmp = tempdir().unwrap(); + with_test_home(tmp.path(), || { + let mut result = ToolResult::success("small payload"); + let path = apply_spillover(&mut result, "call-small"); + assert!(path.is_none()); + assert_eq!(result.content, "small payload"); + assert!(result.metadata.is_none()); + }); + } + + #[test] + fn apply_spillover_is_noop_for_error_results() { + let _g = setup(); + let tmp = tempdir().unwrap(); + with_test_home(tmp.path(), || { + // Even very large error messages are passed through — + // truncating an error would hide it from the model. + let big_err = "boom\n".repeat(50_000); + let mut result = ToolResult::error(big_err.clone()); + let path = apply_spillover(&mut result, "call-err"); + assert!(path.is_none()); + assert_eq!(result.content, big_err); + }); + } + + #[test] + fn apply_spillover_truncates_and_stamps_metadata_above_threshold() { + let _g = setup(); + let tmp = tempdir().unwrap(); + with_test_home(tmp.path(), || { + // 200 KiB body — well above the 100 KiB threshold. + let big = "X".repeat(200 * 1024); + let mut result = ToolResult::success(big.clone()); + let path = apply_spillover(&mut result, "call-big").expect("should spill"); + + // Inline content shrunk to head + footer. + assert!(result.content.len() < big.len()); + assert!( + result.content.contains("Output truncated:"), + "footer missing: {}", + &result.content[result.content.len().saturating_sub(200)..] + ); + assert!(result.content.contains("read_file path=")); + + // Full bytes are on disk at the returned path. + assert!(path.exists(), "spillover file missing: {path:?}"); + let body = fs::read_to_string(&path).unwrap(); + assert_eq!(body.len(), 200 * 1024); + + // metadata.spillover_path stamped for the UI to find. + let metadata = result.metadata.expect("metadata stamped"); + let stamped = metadata + .get("spillover_path") + .and_then(serde_json::Value::as_str) + .expect("spillover_path key present"); + assert_eq!(stamped, path.display().to_string()); + }); + } + + #[test] + fn apply_spillover_preserves_existing_metadata() { + let _g = setup(); + let tmp = tempdir().unwrap(); + with_test_home(tmp.path(), || { + let big = "Y".repeat(200 * 1024); + let mut result = ToolResult::success(big) + .with_metadata(serde_json::json!({"prior_key": "prior_value"})); + let path = apply_spillover(&mut result, "call-meta").expect("should spill"); + + let metadata = result.metadata.expect("metadata present"); + // Prior keys survive. + assert_eq!( + metadata + .get("prior_key") + .and_then(serde_json::Value::as_str), + Some("prior_value") + ); + // New key added alongside. + assert_eq!( + metadata + .get("spillover_path") + .and_then(serde_json::Value::as_str), + Some(path.display().to_string().as_str()) + ); + }); + } } diff --git a/crates/tui/src/tui/ui.rs b/crates/tui/src/tui/ui.rs index 0cd9f5db..0467900f 100644 --- a/crates/tui/src/tui/ui.rs +++ b/crates/tui/src/tui/ui.rs @@ -6655,6 +6655,33 @@ fn open_tool_details_pager(app: &mut App) -> bool { open_details_pager_for_cell(app, cell_index) } +/// Build the trailing "Spillover" section for the tool-details pager +/// (#500). Returns `None` when the cell at `cell_index` is not a +/// `GenericToolCell` with a recorded spillover path, or when the +/// spillover file is missing or unreadable. Failures fall back to a +/// short notice in the section so the user understands why the full +/// content can't be loaded — better than silent truncation. +fn spillover_pager_section(app: &App, cell_index: usize) -> Option { + use crate::tui::history::{GenericToolCell, HistoryCell, ToolCell}; + + let cell = app.cell_at_virtual_index(cell_index)?; + let HistoryCell::Tool(ToolCell::Generic(GenericToolCell { + spillover_path: Some(path), + .. + })) = cell + else { + return None; + }; + let path_str = path.display().to_string(); + let body = match std::fs::read_to_string(path) { + Ok(text) => text, + Err(err) => format!("(could not read spillover file: {err})"), + }; + Some(format!( + "── Full output (spillover) ──\nFile: {path_str}\n\n{body}" + )) +} + fn open_details_pager_for_cell(app: &mut App, cell_index: usize) -> bool { if let Some(detail) = app.tool_detail_record_for_cell(cell_index) { let input = serde_json::to_string_pretty(&detail.input) @@ -6663,10 +6690,25 @@ fn open_details_pager_for_cell(app: &mut App, cell_index: usize) -> bool { "(not available)".to_string(), std::string::ToString::to_string, ); - let content = format!( - "Tool ID: {}\nTool: {}\n\nInput:\n{}\n\nOutput:\n{}", - detail.tool_id, detail.tool_name, input, output - ); + + // #500: when the tool result was spilled to disk, fold the full + // file content into the pager body so the user can see what was + // elided (the model only ever saw the head). The truncated head + // stays above as `Output:` so the user can compare what the + // model received against the full payload. + let spillover_section = spillover_pager_section(app, cell_index); + + let content = if let Some(section) = spillover_section { + format!( + "Tool ID: {}\nTool: {}\n\nInput:\n{}\n\nOutput:\n{}\n\n{}", + detail.tool_id, detail.tool_name, input, output, section + ) + } else { + format!( + "Tool ID: {}\nTool: {}\n\nInput:\n{}\n\nOutput:\n{}", + detail.tool_id, detail.tool_name, input, output + ) + }; let width = app .viewport diff --git a/crates/tui/src/tui/ui/tests.rs b/crates/tui/src/tui/ui/tests.rs index 704dd869..c240cc63 100644 --- a/crates/tui/src/tui/ui/tests.rs +++ b/crates/tui/src/tui/ui/tests.rs @@ -1664,6 +1664,67 @@ fn open_tool_details_pager_supports_active_virtual_tool_cell() { assert_eq!(app.view_stack.top_kind(), Some(ModalKind::Pager)); } +#[test] +fn spillover_pager_section_returns_none_when_no_spillover() { + let mut app = create_test_app(); + app.history = vec![HistoryCell::Tool(ToolCell::Generic(GenericToolCell { + name: "exec_shell".to_string(), + status: ToolStatus::Success, + input_summary: None, + output: Some("hi".to_string()), + prompts: None, + spillover_path: None, + }))]; + app.resync_history_revisions(); + assert!(spillover_pager_section(&app, 0).is_none()); +} + +#[test] +fn spillover_pager_section_loads_file_when_present() { + use std::io::Write; + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("call-test.txt"); + let mut f = std::fs::File::create(&path).unwrap(); + writeln!(f, "FULL_OUTPUT_BYTES_HERE").unwrap(); + + let mut app = create_test_app(); + app.history = vec![HistoryCell::Tool(ToolCell::Generic(GenericToolCell { + name: "exec_shell".to_string(), + status: ToolStatus::Success, + input_summary: None, + output: Some("(truncated head)".to_string()), + prompts: None, + spillover_path: Some(path.clone()), + }))]; + app.resync_history_revisions(); + + let section = spillover_pager_section(&app, 0).expect("section present"); + assert!(section.contains("Full output (spillover)")); + assert!( + section.contains("FULL_OUTPUT_BYTES_HERE"), + "section missing file body: {section}" + ); + assert!(section.contains(&path.display().to_string())); +} + +#[test] +fn spillover_pager_section_returns_notice_when_file_missing() { + let mut app = create_test_app(); + let bogus = std::path::PathBuf::from("/tmp/this/path/does/not/exist-spill.txt"); + app.history = vec![HistoryCell::Tool(ToolCell::Generic(GenericToolCell { + name: "exec_shell".to_string(), + status: ToolStatus::Success, + input_summary: None, + output: Some("(truncated head)".to_string()), + prompts: None, + spillover_path: Some(bogus), + }))]; + app.resync_history_revisions(); + + let section = spillover_pager_section(&app, 0).expect("still emits a notice section"); + assert!(section.contains("could not read spillover file")); +} + #[test] fn details_shortcut_modifiers_accept_plain_shift_and_alt_only() { assert!(details_shortcut_modifiers(KeyModifiers::NONE));