Skip to content

Commit 0b7c6f4

Browse files
Yogthosyogthos
authored andcommitted
LSP Phase 6: edit-tool diagnostic integration
src/lsp/diagnostic.rs (new) - pretty(diagnostic): one-line 'SEVERITY [line:col] msg' rendering with LSP 0-based → display 1-based coordinate conversion. - report(file, issues): wraps in <diagnostics file="..."> block. ERROR severity only (warnings/hints are noise). MAX_PER_FILE=20 with '... and N more' overflow footer. - build_report_block(current, all): the full agent-facing block that write/edit tools append. Two sections (each optional): current file errors, then up to MAX_PROJECT_DIAGNOSTICS_FILES=5 other-file errors (e.g. downstream callers that now fail type-checking). - 15 tests covering: severity labels, 1-based coordinate display, missing-severity defaults to ERROR (regression — pretty must never panic or hide a diagnostic), error filtering (regression — warnings must NOT appear), wrap tags, cap at MAX_PER_FILE with overflow (regression — generated files with hundreds of errors must not blow context), other-files cap (regression — single edit shouldn't dump entire project state), warning-only files don't appear, caller path preserved in display (regression — not canonical form). src/agent/tools/write.rs - New optional Option<Arc<LspManager>> field. With manager set, after the write: touch_file(AwaitPush) + build_report_block + append to output. DIAGNOSTIC_WAIT = 10s matches opencode. - Shared append_lsp_block helper used by both write and edit. - 2 tests: no-manager path preserves pre-LSP output exactly (regression); manager-with-no-diagnostics appends nothing. src/agent/tools/edit.rs - Same Option<Arc<LspManager>> field + integration. Diagnostic block appended after the existing diff block. src/agent/tools/read.rs - New optional Option<Arc<LspManager>> field. Read warms the LSP server with a fire-and-forget tokio::spawn(touch_file(Notify)). The read's primary output is unchanged — no diagnostic block (that's the edit tool's job). src/agent/builder.rs - All three tool constructors now pass None for lsp_manager. Phase 7's CLI/config plumbing will populate it. Code review fixes applied before push: - Display caller path (not canonical form) in current-file section so macOS /tmp / /private/tmp asymmetry doesn't confuse the agent. Phase 5: 100, Phase 6: +16 -> 116 LSP tests. Suite: 405 -> 421.
1 parent c31cc58 commit 0b7c6f4

7 files changed

Lines changed: 583 additions & 2 deletions

File tree

src/agent/builder.rs

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -140,18 +140,22 @@ pub async fn build_agent_inner<M: CompletionModel + 'static>(
140140
permission.clone(),
141141
ask_tx.clone(),
142142
cache.clone(),
143+
// Phase 7 will populate this from build_channels.
144+
None,
143145
)),
144146
Box::new(tools::WriteTool::with_cache(
145147
permission.clone(),
146148
ask_tx.clone(),
147149
plan_file.clone(),
148150
cache.clone(),
151+
None,
149152
)),
150153
Box::new(tools::EditTool::with_cache(
151154
permission.clone(),
152155
ask_tx.clone(),
153156
plan_file.clone(),
154157
cache.clone(),
158+
None,
155159
)),
156160
Box::new(tools::BashTool::with_cache(
157161
permission.clone(),

src/agent/tools/edit.rs

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,22 @@
11
use std::path::PathBuf;
2+
use std::sync::Arc;
23

34
use rig::completion::ToolDefinition;
45
use rig::tool::Tool;
56

67
use crate::agent::tools::cache::ToolCache;
78
use crate::agent::tools::{AskSender, EditArgs, PermCheck, ToolError, check_perm_path};
9+
use crate::lsp::manager::LspManager;
810

911
pub struct EditTool {
1012
pub permission: Option<PermCheck>,
1113
pub ask_tx: Option<AskSender>,
1214
plan_file: Option<PathBuf>,
1315
cache: Option<ToolCache>,
16+
/// When set, the tool touches the edited file on the LSP server and
17+
/// appends any diagnostic block to its output. `None` reproduces the
18+
/// pre-LSP behaviour.
19+
lsp_manager: Option<Arc<LspManager>>,
1420
}
1521

1622
impl EditTool {
@@ -25,6 +31,7 @@ impl EditTool {
2531
ask_tx,
2632
plan_file,
2733
cache: None,
34+
lsp_manager: None,
2835
}
2936
}
3037

@@ -33,12 +40,14 @@ impl EditTool {
3340
ask_tx: Option<AskSender>,
3441
plan_file: Option<PathBuf>,
3542
cache: ToolCache,
43+
lsp_manager: Option<Arc<LspManager>>,
3644
) -> Self {
3745
EditTool {
3846
permission,
3947
ask_tx,
4048
plan_file,
4149
cache: Some(cache),
50+
lsp_manager,
4251
}
4352
}
4453

@@ -203,6 +212,7 @@ impl Tool for EditTool {
203212
new_content
204213
};
205214

215+
let write_at = std::time::Instant::now();
206216
tokio::fs::write(&args.path, &output).await?;
207217
// File mutated → invalidate cached reads/greps/listings for this turn.
208218
if let Some(ref cache) = self.cache {
@@ -225,6 +235,16 @@ impl Tool for EditTool {
225235
&args.new_text,
226236
));
227237
}
238+
239+
let path = std::path::Path::new(&args.path);
240+
result.push_str(
241+
&crate::agent::tools::write::append_lsp_block(
242+
self.lsp_manager.as_ref(),
243+
path,
244+
write_at,
245+
)
246+
.await,
247+
);
228248
Ok(result)
229249
}
230250
}

src/agent/tools/mod.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ mod task_status;
2020
mod todo;
2121
mod webfetch;
2222
mod websearch;
23-
mod write;
23+
pub(crate) mod write;
2424

2525
pub use apply_patch::ApplyPatchTool;
2626
pub use bash::BashTool;

src/agent/tools/read.rs

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,20 @@
1+
use std::sync::Arc;
2+
13
use rig::completion::ToolDefinition;
24
use rig::tool::Tool;
35

46
use crate::agent::tools::cache::ToolCache;
57
use crate::agent::tools::{AskSender, PermCheck, ReadArgs, ToolError, check_perm_path};
8+
use crate::lsp::manager::{LspManager, TouchMode};
69

710
pub struct ReadTool {
811
pub permission: Option<PermCheck>,
912
pub ask_tx: Option<AskSender>,
1013
pub cache: Option<ToolCache>,
14+
/// When set, the tool fires off a `touch_file` to warm the LSP server
15+
/// so subsequent edits surface diagnostics quickly. Fire-and-forget:
16+
/// the read tool does not wait or surface diagnostics in its output.
17+
pub lsp_manager: Option<Arc<LspManager>>,
1118
}
1219

1320
impl ReadTool {
@@ -17,18 +24,21 @@ impl ReadTool {
1724
permission,
1825
ask_tx,
1926
cache: None,
27+
lsp_manager: None,
2028
}
2129
}
2230

2331
pub fn with_cache(
2432
permission: Option<PermCheck>,
2533
ask_tx: Option<AskSender>,
2634
cache: ToolCache,
35+
lsp_manager: Option<Arc<LspManager>>,
2736
) -> Self {
2837
ReadTool {
2938
permission,
3039
ask_tx,
3140
cache: Some(cache),
41+
lsp_manager,
3242
}
3343
}
3444
}
@@ -109,6 +119,16 @@ impl Tool for ReadTool {
109119
cache.set(&cache_key, info.clone());
110120
}
111121

122+
// Fire-and-forget LSP warmup so the server already has the file
123+
// open by the time the agent edits it (and we can wait_for_push
124+
// quickly). No diagnostic surfacing on read.
125+
if let Some(manager) = self.lsp_manager.clone() {
126+
let path = std::path::PathBuf::from(&args.path);
127+
tokio::spawn(async move {
128+
manager.touch_file(&path, TouchMode::Notify).await;
129+
});
130+
}
131+
112132
Ok(info)
113133
}
114134
}

src/agent/tools/write.rs

Lines changed: 120 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,29 @@
11
use std::path::{Path, PathBuf};
2+
use std::sync::Arc;
3+
use std::time::{Duration, Instant};
24

35
use rig::completion::ToolDefinition;
46
use rig::tool::Tool;
57

68
use crate::agent::tools::cache::ToolCache;
79
use crate::agent::tools::{AskSender, PermCheck, ToolError, WriteArgs, check_perm_path};
10+
use crate::lsp::diagnostic;
11+
use crate::lsp::manager::{LspManager, TouchMode};
12+
13+
/// How long to wait for the LSP server to publish fresh diagnostics after
14+
/// a write. Matches opencode's `DIAGNOSTICS_FULL_WAIT_TIMEOUT_MS`. Bounded
15+
/// so a stuck server doesn't hold up the agent's turn.
16+
const DIAGNOSTIC_WAIT: Duration = Duration::from_secs(10);
817

918
pub struct WriteTool {
1019
pub permission: Option<PermCheck>,
1120
pub ask_tx: Option<AskSender>,
1221
plan_file: Option<PathBuf>,
1322
cache: Option<ToolCache>,
23+
/// When set, the tool touches the file on the LSP server after writing
24+
/// and appends any resulting diagnostic block to its output. `None`
25+
/// reproduces the pre-LSP behaviour exactly.
26+
lsp_manager: Option<Arc<LspManager>>,
1427
}
1528

1629
impl WriteTool {
@@ -25,6 +38,7 @@ impl WriteTool {
2538
ask_tx,
2639
plan_file,
2740
cache: None,
41+
lsp_manager: None,
2842
}
2943
}
3044

@@ -33,12 +47,14 @@ impl WriteTool {
3347
ask_tx: Option<AskSender>,
3448
plan_file: Option<PathBuf>,
3549
cache: ToolCache,
50+
lsp_manager: Option<Arc<LspManager>>,
3651
) -> Self {
3752
WriteTool {
3853
permission,
3954
ask_tx,
4055
plan_file,
4156
cache: Some(cache),
57+
lsp_manager,
4258
}
4359
}
4460
}
@@ -90,11 +106,114 @@ impl Tool for WriteTool {
90106
tokio::fs::create_dir_all(parent).await?;
91107
}
92108
let bytes = args.content.len();
109+
let write_at = Instant::now();
93110
tokio::fs::write(path, &args.content).await?;
94111
// File mutated → invalidate cached reads/greps/listings for this turn.
95112
if let Some(ref cache) = self.cache {
96113
cache.clear();
97114
}
98-
Ok(format!("Written {} bytes to {}", bytes, args.path))
115+
116+
let mut output = format!("Written {} bytes to {}", bytes, args.path);
117+
output.push_str(&append_lsp_block(self.lsp_manager.as_ref(), path, write_at).await);
118+
Ok(output)
119+
}
120+
}
121+
122+
/// Run `touch_file` + diagnostic-report assembly. Returns the appendable
123+
/// block (empty string when there's nothing to surface or no manager).
124+
/// Errors during touch/wait are intentionally swallowed — diagnostic
125+
/// surfacing is a side-effect; the write tool's primary contract is
126+
/// "wrote the file".
127+
pub(crate) async fn append_lsp_block(
128+
manager: Option<&Arc<LspManager>>,
129+
path: &Path,
130+
after: Instant,
131+
) -> String {
132+
let Some(manager) = manager else {
133+
return String::new();
134+
};
135+
manager
136+
.touch_file(
137+
path,
138+
TouchMode::AwaitPush {
139+
after,
140+
timeout: DIAGNOSTIC_WAIT,
141+
},
142+
)
143+
.await;
144+
let diagnostics = manager.all_diagnostics();
145+
diagnostic::build_report_block(path, &diagnostics)
146+
}
147+
148+
#[cfg(test)]
149+
mod tests {
150+
use super::*;
151+
use crate::agent::tools::cache::ToolCache;
152+
use crate::lsp::manager::LspManager;
153+
use crate::lsp::spawn::{Spawned, Spawner};
154+
use futures::future::BoxFuture;
155+
156+
fn tempfile_in(dir: &Path, name: &str) -> PathBuf {
157+
dir.join(name)
158+
}
159+
160+
/// Synthetic spawner — never actually invoked because the write paths
161+
/// we test don't have an extension the manager would claim.
162+
struct NopSpawner;
163+
impl Spawner for NopSpawner {
164+
fn spawn<'a>(
165+
&'a self,
166+
_server_id: &'a str,
167+
_root: &'a Path,
168+
) -> BoxFuture<'a, std::io::Result<Spawned>> {
169+
Box::pin(async { Err(std::io::Error::other("not used")) })
170+
}
171+
}
172+
173+
// Regression: when no LSP manager is provided, the tool's output must
174+
// be exactly what it was pre-LSP (just "Written N bytes to PATH").
175+
// The diagnostic-append code path must not perturb the no-manager case.
176+
#[tokio::test]
177+
async fn regression_no_manager_preserves_existing_output() {
178+
let dir = std::env::temp_dir().join(format!("dirge-write-no-mgr-{}", std::process::id()));
179+
let _ = std::fs::create_dir_all(&dir);
180+
let path = tempfile_in(&dir, "no-mgr.txt");
181+
182+
let tool = WriteTool::with_cache(None, None, None, ToolCache::new(), None);
183+
let out = tool
184+
.call(WriteArgs {
185+
path: path.to_string_lossy().into_owned(),
186+
content: "hello".into(),
187+
})
188+
.await
189+
.unwrap();
190+
assert!(out.starts_with("Written 5 bytes"), "got: {out}");
191+
// No diagnostic block since manager is None.
192+
assert!(!out.contains("LSP errors"));
193+
std::fs::remove_dir_all(&dir).ok();
194+
}
195+
196+
// When a manager IS provided but has no diagnostics (mock spawner that
197+
// never gets called for the extension), the tool's output still starts
198+
// with the write confirmation and contains no diagnostic block.
199+
#[tokio::test]
200+
async fn manager_with_no_diagnostics_appends_nothing() {
201+
let dir = std::env::temp_dir().join(format!("dirge-write-with-mgr-{}", std::process::id()));
202+
let _ = std::fs::create_dir_all(&dir);
203+
let path = tempfile_in(&dir, "with-mgr.unknown_ext");
204+
205+
let manager = Arc::new(LspManager::new(Arc::new(NopSpawner), dir.clone()));
206+
let tool = WriteTool::with_cache(None, None, None, ToolCache::new(), Some(manager));
207+
208+
let out = tool
209+
.call(WriteArgs {
210+
path: path.to_string_lossy().into_owned(),
211+
content: "hi".into(),
212+
})
213+
.await
214+
.unwrap();
215+
assert!(out.starts_with("Written 2 bytes"));
216+
assert!(!out.contains("LSP errors"), "got: {out}");
217+
std::fs::remove_dir_all(&dir).ok();
99218
}
100219
}

0 commit comments

Comments
 (0)