diff --git a/src/agent/builder.rs b/src/agent/builder.rs index cb174f02..d3998be5 100644 --- a/src/agent/builder.rs +++ b/src/agent/builder.rs @@ -140,18 +140,22 @@ pub async fn build_agent_inner( permission.clone(), ask_tx.clone(), cache.clone(), + // Phase 7 will populate this from build_channels. + None, )), Box::new(tools::WriteTool::with_cache( permission.clone(), ask_tx.clone(), plan_file.clone(), cache.clone(), + None, )), Box::new(tools::EditTool::with_cache( permission.clone(), ask_tx.clone(), plan_file.clone(), cache.clone(), + None, )), Box::new(tools::BashTool::with_cache( permission.clone(), diff --git a/src/agent/tools/edit.rs b/src/agent/tools/edit.rs index 7f7074e0..dc8f5129 100644 --- a/src/agent/tools/edit.rs +++ b/src/agent/tools/edit.rs @@ -1,16 +1,22 @@ use std::path::PathBuf; +use std::sync::Arc; use rig::completion::ToolDefinition; use rig::tool::Tool; use crate::agent::tools::cache::ToolCache; use crate::agent::tools::{AskSender, EditArgs, PermCheck, ToolError, check_perm_path}; +use crate::lsp::manager::LspManager; pub struct EditTool { pub permission: Option, pub ask_tx: Option, plan_file: Option, cache: Option, + /// When set, the tool touches the edited file on the LSP server and + /// appends any diagnostic block to its output. `None` reproduces the + /// pre-LSP behaviour. + lsp_manager: Option>, } impl EditTool { @@ -25,6 +31,7 @@ impl EditTool { ask_tx, plan_file, cache: None, + lsp_manager: None, } } @@ -33,12 +40,14 @@ impl EditTool { ask_tx: Option, plan_file: Option, cache: ToolCache, + lsp_manager: Option>, ) -> Self { EditTool { permission, ask_tx, plan_file, cache: Some(cache), + lsp_manager, } } @@ -203,6 +212,7 @@ impl Tool for EditTool { new_content }; + let write_at = std::time::Instant::now(); tokio::fs::write(&args.path, &output).await?; // File mutated → invalidate cached reads/greps/listings for this turn. if let Some(ref cache) = self.cache { @@ -225,6 +235,16 @@ impl Tool for EditTool { &args.new_text, )); } + + let path = std::path::Path::new(&args.path); + result.push_str( + &crate::agent::tools::write::append_lsp_block( + self.lsp_manager.as_ref(), + path, + write_at, + ) + .await, + ); Ok(result) } } diff --git a/src/agent/tools/mod.rs b/src/agent/tools/mod.rs index 70bf65b6..e3fa877b 100644 --- a/src/agent/tools/mod.rs +++ b/src/agent/tools/mod.rs @@ -20,7 +20,7 @@ mod task_status; mod todo; mod webfetch; mod websearch; -mod write; +pub(crate) mod write; pub use apply_patch::ApplyPatchTool; pub use bash::BashTool; diff --git a/src/agent/tools/read.rs b/src/agent/tools/read.rs index 14e4c193..2363e495 100644 --- a/src/agent/tools/read.rs +++ b/src/agent/tools/read.rs @@ -1,13 +1,20 @@ +use std::sync::Arc; + use rig::completion::ToolDefinition; use rig::tool::Tool; use crate::agent::tools::cache::ToolCache; use crate::agent::tools::{AskSender, PermCheck, ReadArgs, ToolError, check_perm_path}; +use crate::lsp::manager::{LspManager, TouchMode}; pub struct ReadTool { pub permission: Option, pub ask_tx: Option, pub cache: Option, + /// When set, the tool fires off a `touch_file` to warm the LSP server + /// so subsequent edits surface diagnostics quickly. Fire-and-forget: + /// the read tool does not wait or surface diagnostics in its output. + pub lsp_manager: Option>, } impl ReadTool { @@ -17,6 +24,7 @@ impl ReadTool { permission, ask_tx, cache: None, + lsp_manager: None, } } @@ -24,11 +32,13 @@ impl ReadTool { permission: Option, ask_tx: Option, cache: ToolCache, + lsp_manager: Option>, ) -> Self { ReadTool { permission, ask_tx, cache: Some(cache), + lsp_manager, } } } @@ -109,6 +119,16 @@ impl Tool for ReadTool { cache.set(&cache_key, info.clone()); } + // Fire-and-forget LSP warmup so the server already has the file + // open by the time the agent edits it (and we can wait_for_push + // quickly). No diagnostic surfacing on read. + if let Some(manager) = self.lsp_manager.clone() { + let path = std::path::PathBuf::from(&args.path); + tokio::spawn(async move { + manager.touch_file(&path, TouchMode::Notify).await; + }); + } + Ok(info) } } diff --git a/src/agent/tools/write.rs b/src/agent/tools/write.rs index cad0ede0..afeec9e9 100644 --- a/src/agent/tools/write.rs +++ b/src/agent/tools/write.rs @@ -1,16 +1,29 @@ use std::path::{Path, PathBuf}; +use std::sync::Arc; +use std::time::{Duration, Instant}; use rig::completion::ToolDefinition; use rig::tool::Tool; use crate::agent::tools::cache::ToolCache; use crate::agent::tools::{AskSender, PermCheck, ToolError, WriteArgs, check_perm_path}; +use crate::lsp::diagnostic; +use crate::lsp::manager::{LspManager, TouchMode}; + +/// How long to wait for the LSP server to publish fresh diagnostics after +/// a write. Matches opencode's `DIAGNOSTICS_FULL_WAIT_TIMEOUT_MS`. Bounded +/// so a stuck server doesn't hold up the agent's turn. +const DIAGNOSTIC_WAIT: Duration = Duration::from_secs(10); pub struct WriteTool { pub permission: Option, pub ask_tx: Option, plan_file: Option, cache: Option, + /// When set, the tool touches the file on the LSP server after writing + /// and appends any resulting diagnostic block to its output. `None` + /// reproduces the pre-LSP behaviour exactly. + lsp_manager: Option>, } impl WriteTool { @@ -25,6 +38,7 @@ impl WriteTool { ask_tx, plan_file, cache: None, + lsp_manager: None, } } @@ -33,12 +47,14 @@ impl WriteTool { ask_tx: Option, plan_file: Option, cache: ToolCache, + lsp_manager: Option>, ) -> Self { WriteTool { permission, ask_tx, plan_file, cache: Some(cache), + lsp_manager, } } } @@ -90,11 +106,114 @@ impl Tool for WriteTool { tokio::fs::create_dir_all(parent).await?; } let bytes = args.content.len(); + let write_at = Instant::now(); tokio::fs::write(path, &args.content).await?; // File mutated → invalidate cached reads/greps/listings for this turn. if let Some(ref cache) = self.cache { cache.clear(); } - Ok(format!("Written {} bytes to {}", bytes, args.path)) + + let mut output = format!("Written {} bytes to {}", bytes, args.path); + output.push_str(&append_lsp_block(self.lsp_manager.as_ref(), path, write_at).await); + Ok(output) + } +} + +/// Run `touch_file` + diagnostic-report assembly. Returns the appendable +/// block (empty string when there's nothing to surface or no manager). +/// Errors during touch/wait are intentionally swallowed — diagnostic +/// surfacing is a side-effect; the write tool's primary contract is +/// "wrote the file". +pub(crate) async fn append_lsp_block( + manager: Option<&Arc>, + path: &Path, + after: Instant, +) -> String { + let Some(manager) = manager else { + return String::new(); + }; + manager + .touch_file( + path, + TouchMode::AwaitPush { + after, + timeout: DIAGNOSTIC_WAIT, + }, + ) + .await; + let diagnostics = manager.all_diagnostics(); + diagnostic::build_report_block(path, &diagnostics) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::agent::tools::cache::ToolCache; + use crate::lsp::manager::LspManager; + use crate::lsp::spawn::{Spawned, Spawner}; + use futures::future::BoxFuture; + + fn tempfile_in(dir: &Path, name: &str) -> PathBuf { + dir.join(name) + } + + /// Synthetic spawner — never actually invoked because the write paths + /// we test don't have an extension the manager would claim. + struct NopSpawner; + impl Spawner for NopSpawner { + fn spawn<'a>( + &'a self, + _server_id: &'a str, + _root: &'a Path, + ) -> BoxFuture<'a, std::io::Result> { + Box::pin(async { Err(std::io::Error::other("not used")) }) + } + } + + // Regression: when no LSP manager is provided, the tool's output must + // be exactly what it was pre-LSP (just "Written N bytes to PATH"). + // The diagnostic-append code path must not perturb the no-manager case. + #[tokio::test] + async fn regression_no_manager_preserves_existing_output() { + let dir = std::env::temp_dir().join(format!("dirge-write-no-mgr-{}", std::process::id())); + let _ = std::fs::create_dir_all(&dir); + let path = tempfile_in(&dir, "no-mgr.txt"); + + let tool = WriteTool::with_cache(None, None, None, ToolCache::new(), None); + let out = tool + .call(WriteArgs { + path: path.to_string_lossy().into_owned(), + content: "hello".into(), + }) + .await + .unwrap(); + assert!(out.starts_with("Written 5 bytes"), "got: {out}"); + // No diagnostic block since manager is None. + assert!(!out.contains("LSP errors")); + std::fs::remove_dir_all(&dir).ok(); + } + + // When a manager IS provided but has no diagnostics (mock spawner that + // never gets called for the extension), the tool's output still starts + // with the write confirmation and contains no diagnostic block. + #[tokio::test] + async fn manager_with_no_diagnostics_appends_nothing() { + let dir = std::env::temp_dir().join(format!("dirge-write-with-mgr-{}", std::process::id())); + let _ = std::fs::create_dir_all(&dir); + let path = tempfile_in(&dir, "with-mgr.unknown_ext"); + + let manager = Arc::new(LspManager::new(Arc::new(NopSpawner), dir.clone())); + let tool = WriteTool::with_cache(None, None, None, ToolCache::new(), Some(manager)); + + let out = tool + .call(WriteArgs { + path: path.to_string_lossy().into_owned(), + content: "hi".into(), + }) + .await + .unwrap(); + assert!(out.starts_with("Written 2 bytes")); + assert!(!out.contains("LSP errors"), "got: {out}"); + std::fs::remove_dir_all(&dir).ok(); } } diff --git a/src/lsp/diagnostic.rs b/src/lsp/diagnostic.rs new file mode 100644 index 00000000..906d7028 --- /dev/null +++ b/src/lsp/diagnostic.rs @@ -0,0 +1,417 @@ +//! Diagnostic pretty-printing + report blocks for tool output. +//! +//! Mirrors opencode's `lsp/diagnostic.ts`: only ERROR severity surfaces in +//! the report, max 20 per file (overflow becomes "... and N more"), wrapped +//! in `` tags that the LLM recognizes as +//! out-of-band tool context. +//! +//! The full agent-facing block (current file + capped other files) is +//! built by [`build_report_block`]. That's what `write` / `edit` tools +//! append to their `Ok(String)` output in Phase 6. + +use std::collections::HashMap; +use std::path::{Path, PathBuf}; + +use lsp_types::{Diagnostic, DiagnosticSeverity}; + +/// Max diagnostics rendered per file before truncating with a "... and N +/// more" footer. Bounds blast radius for a generated file with hundreds of +/// errors. +const MAX_PER_FILE: usize = 20; + +/// Max additional files surfaced in the project-wide section beyond the +/// just-edited file. Stops a single edit from dumping the entire project's +/// diagnostic state into the agent's context on each turn. +const MAX_PROJECT_DIAGNOSTICS_FILES: usize = 5; + +/// One-line human-readable rendering of an LSP diagnostic. +/// Converts LSP's 0-based line/character to the 1-based form editors and +/// agents typically display. +pub fn pretty(d: &Diagnostic) -> String { + let severity = match d.severity { + Some(DiagnosticSeverity::ERROR) => "ERROR", + Some(DiagnosticSeverity::WARNING) => "WARN", + Some(DiagnosticSeverity::INFORMATION) => "INFO", + Some(DiagnosticSeverity::HINT) => "HINT", + _ => "ERROR", + }; + let line = d.range.start.line.saturating_add(1); + let col = d.range.start.character.saturating_add(1); + format!("{severity} [{line}:{col}] {}", d.message) +} + +/// Render a single file's diagnostics as a `` block. Only +/// ERROR severity is included — warnings and hints would be noise. Returns +/// `None` when there are zero errors (so callers can skip emitting a +/// section heading). +pub fn report(file: &str, issues: &[Diagnostic]) -> Option { + let errors: Vec<&Diagnostic> = issues + .iter() + .filter(|d| d.severity == Some(DiagnosticSeverity::ERROR)) + .collect(); + if errors.is_empty() { + return None; + } + let total = errors.len(); + let limited = errors.iter().take(MAX_PER_FILE); + let mut body: Vec = limited.map(|d| pretty(d)).collect(); + if total > MAX_PER_FILE { + body.push(format!("... and {} more", total - MAX_PER_FILE)); + } + Some(format!( + "\n{}\n", + body.join("\n") + )) +} + +/// Build the full diagnostic block appended to a `write` / `edit` tool's +/// output. Two sections (each optional): +/// - **This file**: errors in the just-edited file. Always surfaced if any. +/// - **Other files**: errors in other files (e.g. a downstream caller that +/// now fails type-checking). Capped at `MAX_PROJECT_DIAGNOSTICS_FILES` +/// so a single edit doesn't dump the entire project's state. +/// +/// Returns an empty string when there's nothing worth reporting, so callers +/// can `output.push_str(&block)` unconditionally. +pub fn build_report_block( + current_file: &Path, + all_diagnostics: &HashMap>, +) -> String { + let current_canonical = current_file + .canonicalize() + .unwrap_or_else(|_| current_file.to_path_buf()); + + let mut out = String::new(); + + // Current-file section. Display the caller-supplied path (not the + // canonical form) so the agent sees the same path it just wrote to — + // e.g. `/tmp/foo.rs` rather than `/private/tmp/foo.rs` on macOS. + if let Some(issues) = lookup_diagnostics(¤t_canonical, all_diagnostics) + && let Some(block) = report(¤t_file.display().to_string(), issues) + { + out.push_str("\n\nLSP errors detected in this file, please fix:\n"); + out.push_str(&block); + } + + // Other-files section. We iterate the diagnostic map deterministically + // (sorted by path) so test assertions don't flake on hash order. + let mut other_paths: Vec<&PathBuf> = all_diagnostics + .keys() + .filter(|p| { + p.canonicalize() + .map(|c| c != current_canonical) + .unwrap_or(p.as_path() != current_canonical) + }) + .collect(); + other_paths.sort(); + + let mut other_count = 0; + for path in other_paths { + if other_count >= MAX_PROJECT_DIAGNOSTICS_FILES { + break; + } + let Some(issues) = all_diagnostics.get(path) else { + continue; + }; + let Some(block) = report(&path.display().to_string(), issues) else { + continue; + }; + if other_count == 0 { + out.push_str("\n\nLSP errors detected in other files:\n"); + } else { + out.push('\n'); + } + out.push_str(&block); + other_count += 1; + } + + out +} + +/// Look up diagnostics for `target` in `map`, trying the canonical form +/// first then the literal path. Cheaper than canonicalizing every key. +fn lookup_diagnostics<'a>( + target: &Path, + map: &'a HashMap>, +) -> Option<&'a Vec> { + if let Some(v) = map.get(target) { + return Some(v); + } + for (k, v) in map.iter() { + if k.canonicalize().map(|c| c == target).unwrap_or(k == target) { + return Some(v); + } + } + None +} + +#[cfg(test)] +mod tests { + use super::*; + use lsp_types::{NumberOrString, Position, Range}; + + fn diag(severity: DiagnosticSeverity, line: u32, col: u32, msg: &str) -> Diagnostic { + Diagnostic { + range: Range { + start: Position { + line, + character: col, + }, + end: Position { + line, + character: col, + }, + }, + severity: Some(severity), + code: Some(NumberOrString::String("E0001".to_string())), + code_description: None, + source: Some("rustc".to_string()), + message: msg.to_string(), + related_information: None, + tags: None, + data: None, + } + } + + // ---- pretty ---- + + #[test] + fn pretty_uses_severity_label_and_one_based_coordinates() { + let d = diag(DiagnosticSeverity::ERROR, 4, 2, "type mismatch"); + // LSP line=4 col=2 → 1-based display "[5:3]". + assert_eq!(pretty(&d), "ERROR [5:3] type mismatch"); + } + + #[test] + fn pretty_handles_all_severities() { + for (sev, label) in [ + (DiagnosticSeverity::ERROR, "ERROR"), + (DiagnosticSeverity::WARNING, "WARN"), + (DiagnosticSeverity::INFORMATION, "INFO"), + (DiagnosticSeverity::HINT, "HINT"), + ] { + assert!(pretty(&diag(sev, 0, 0, "x")).starts_with(label)); + } + } + + // Regression: an absent `severity` field used to crash some pretty- + // printers. We default to ERROR — anything else would silently hide a + // diagnostic from the agent's view. + #[test] + fn regression_missing_severity_defaults_to_error() { + let mut d = diag(DiagnosticSeverity::ERROR, 0, 0, "x"); + d.severity = None; + assert!(pretty(&d).starts_with("ERROR")); + } + + // ---- report ---- + + #[test] + fn report_returns_none_for_no_errors() { + assert!(report("x.rs", &[]).is_none()); + // Warnings alone do not produce a report. + let warnings = vec![diag(DiagnosticSeverity::WARNING, 0, 0, "unused")]; + assert!(report("x.rs", &warnings).is_none()); + } + + // Regression: only ERROR severity is surfaced. Warnings/hints would be + // noise — agents only need to fix the things blocking compilation. + #[test] + fn regression_report_filters_to_errors_only() { + let issues = vec![ + diag(DiagnosticSeverity::ERROR, 0, 0, "real error"), + diag(DiagnosticSeverity::WARNING, 1, 0, "unused"), + diag(DiagnosticSeverity::INFORMATION, 2, 0, "fyi"), + diag(DiagnosticSeverity::HINT, 3, 0, "consider"), + ]; + let block = report("x.rs", &issues).unwrap(); + assert!(block.contains("real error")); + assert!(!block.contains("unused")); + assert!(!block.contains("fyi")); + assert!(!block.contains("consider")); + } + + #[test] + fn report_wraps_in_diagnostics_tags() { + let block = report("/tmp/x.rs", &[diag(DiagnosticSeverity::ERROR, 0, 0, "msg")]).unwrap(); + assert!(block.starts_with("\n")); + assert!(block.ends_with("")); + } + + // Regression: capping at MAX_PER_FILE prevents a generated file with + // 500 errors from blowing the agent's context. The footer tells the + // agent there's more, so they know to look further if needed. + #[test] + fn regression_report_caps_at_max_per_file_with_overflow_footer() { + let issues: Vec = (0..MAX_PER_FILE + 7) + .map(|i| diag(DiagnosticSeverity::ERROR, i as u32, 0, &format!("err {i}"))) + .collect(); + let block = report("x.rs", &issues).unwrap(); + let line_count = block.lines().count(); + // 1 header line + MAX_PER_FILE error lines + 1 overflow footer + 1 closing tag. + assert_eq!(line_count, MAX_PER_FILE + 3); + assert!(block.contains("... and 7 more")); + // First 20 are listed; #20 onward are in the overflow. + assert!(block.contains("err 0")); + assert!(block.contains(&format!("err {}", MAX_PER_FILE - 1))); + assert!(!block.contains("err 25")); + } + + #[test] + fn report_below_cap_has_no_overflow_footer() { + let issues = vec![diag(DiagnosticSeverity::ERROR, 0, 0, "one"); 3]; + let block = report("x.rs", &issues).unwrap(); + assert!(!block.contains("and") && !block.contains("more")); + } + + // ---- build_report_block ---- + + #[test] + fn build_report_block_returns_empty_when_no_diagnostics() { + let block = build_report_block(Path::new("/tmp/x.rs"), &HashMap::new()); + assert_eq!(block, ""); + } + + #[test] + fn build_report_block_emits_current_file_section() { + let path = PathBuf::from("/tmp/edited.rs"); + let mut map = HashMap::new(); + map.insert( + path.clone(), + vec![diag(DiagnosticSeverity::ERROR, 0, 0, "bad type")], + ); + let block = build_report_block(&path, &map); + assert!(block.contains("errors detected in this file")); + assert!(block.contains("bad type")); + assert!(!block.contains("errors detected in other files")); + } + + #[test] + fn build_report_block_emits_other_files_section_when_relevant() { + let current = PathBuf::from("/tmp/a.rs"); + let other = PathBuf::from("/tmp/b.rs"); + let mut map = HashMap::new(); + map.insert( + other.clone(), + vec![diag(DiagnosticSeverity::ERROR, 0, 0, "downstream break")], + ); + + let block = build_report_block(¤t, &map); + assert!(!block.contains("errors detected in this file")); + assert!(block.contains("errors detected in other files")); + assert!(block.contains("downstream break")); + } + + // Regression: cap on other-files section keeps tool output bounded. + // Without this, a refactor that broke 100 dependents would dump 100 + // diagnostic blocks at the agent on each subsequent edit. + #[test] + fn regression_build_report_block_caps_other_files() { + let current = PathBuf::from("/tmp/current.rs"); + let mut map = HashMap::new(); + for i in 0..MAX_PROJECT_DIAGNOSTICS_FILES + 5 { + let p = PathBuf::from(format!("/tmp/other{i:02}.rs")); + map.insert( + p, + vec![diag( + DiagnosticSeverity::ERROR, + 0, + 0, + &format!("err in {i}"), + )], + ); + } + let block = build_report_block(¤t, &map); + let other_blocks = block.matches("