diff --git a/CHANGELOG.md b/CHANGELOG.md index 10f2e5983d..2654212127 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added +- File edits are parse-gated before the write lands: Rust goes through + `syn::parse_file` for a grammar-exact `line:column`, and `.toml` / `.json` + through the parsers already vendored. An edit is refused only when the file + parsed *before* and would not parse *after* — repairing an already-broken + file is the commonest reason to edit source at all, so pre-existing breakage + and new files fail open. The check precedes the write, so a rejection leaves + the file untouched and `apply_patch` cannot half-apply (#6204, #6206). +- Rust files that were already `rustfmt`-clean are re-normalized after an edit, + so the next patch's anchors still match. Hand-formatted files are never + rewritten, and every failure path skips and lets the edit land (#6205). - Native clients can finish provider setup without dropping to the CLI: `DELETE /v1/providers/{id}/key` clears a Codewhale-owned credential through the same shared owner as `codewhale auth clear`, and `GET /v1/providers` diff --git a/Cargo.lock b/Cargo.lock index d4001bcbc0..7ec63c0c56 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1173,6 +1173,7 @@ dependencies = [ "parking_lot", "portable-pty", "pretty_assertions", + "proc-macro2", "qrcode", "ratatui", "regex", @@ -1192,6 +1193,7 @@ dependencies = [ "shellexpand", "shlex 2.0.1", "similar", + "syn 3.0.3", "syntect", "tar", "tempfile", diff --git a/crates/tui/CHANGELOG.md b/crates/tui/CHANGELOG.md index ac999ed5d1..55f75af3fc 100644 --- a/crates/tui/CHANGELOG.md +++ b/crates/tui/CHANGELOG.md @@ -9,6 +9,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added +- File edits are parse-gated before the write lands: Rust goes through + `syn::parse_file` for a grammar-exact `line:column`, and `.toml` / `.json` + through the parsers already vendored. An edit is refused only when the file + parsed *before* and would not parse *after* — repairing an already-broken + file is the commonest reason to edit source at all, so pre-existing breakage + and new files fail open. The check precedes the write, so a rejection leaves + the file untouched and `apply_patch` cannot half-apply (#6204, #6206). +- Rust files that were already `rustfmt`-clean are re-normalized after an edit, + so the next patch's anchors still match. Hand-formatted files are never + rewritten, and every failure path skips and lets the edit land (#6205). - Native clients can finish provider setup without dropping to the CLI: `DELETE /v1/providers/{id}/key` clears a Codewhale-owned credential through the same shared owner as `codewhale auth clear`, and `GET /v1/providers` diff --git a/crates/tui/Cargo.toml b/crates/tui/Cargo.toml index 329d34cf65..22957801f7 100644 --- a/crates/tui/Cargo.toml +++ b/crates/tui/Cargo.toml @@ -84,6 +84,13 @@ serde.workspace = true serde_json = { workspace = true, features = ["preserve_order", "raw_value"] } schemars = { version = "1.2.1", features = ["derive", "preserve_order"] } shellexpand = "3" +# Grammar-exact Rust parsing for the post-edit syntax gate (#6204). `syn` is +# already in the lockfile as a proc-macro build dependency; this reuses that +# same version rather than vendoring a second parser (#6151). `span-locations` +# is what turns a parse failure into the `line:column` the model needs — the +# feature is inert without it. +syn = { version = "3.0.3", default-features = false, features = ["full", "parsing"] } +proc-macro2 = { version = "1.0.107", features = ["span-locations"] } toml.workspace = true toml_edit.workspace = true tokio.workspace = true diff --git a/crates/tui/src/tools/apply_patch.rs b/crates/tui/src/tools/apply_patch.rs index 26125c602f..eee97a448f 100644 --- a/crates/tui/src/tools/apply_patch.rs +++ b/crates/tui/src/tools/apply_patch.rs @@ -16,10 +16,12 @@ use super::diff_format::make_unified_diff; use super::file::{ EXPECTED_HASH_DESCRIPTION, PATCH_PARAMS, PATH_ALIASES, apply_param_aliases, content_hash, }; +use super::rust_format::normalize_edit; use super::spec::{ ApprovalRequirement, ToolCapability, ToolContext, ToolError, ToolResult, ToolSpec, lsp_diagnostics_for_paths, optional_bool, optional_str, optional_u64, }; +use super::syntax_check::guard_edit; /// Maximum lines of context for fuzzy matching (increased for better tolerance) const MAX_FUZZ: usize = 50; @@ -411,8 +413,9 @@ impl ToolSpec for ApplyPatchTool { source_field, } = normalized { - let (pending, stats) = + let (mut pending, stats) = build_pending_writes_from_replace(entries, source_field, context)?; + normalize_pending_rust(&mut pending).await; apply_pending_writes(&pending)?; // Resolve absolute paths for LSP diagnostics query. let abs_paths: Vec = pending.iter().map(|p| p.path.clone()).collect(); @@ -457,8 +460,10 @@ impl ToolSpec for ApplyPatchTool { ApplyPatchPreflightKind::FilePatches(file_patches) => file_patches, }; - let (pending, mut stats) = build_pending_writes_from_patches(file_patches, context, fuzz)?; + let (mut pending, mut stats) = + build_pending_writes_from_patches(file_patches, context, fuzz)?; stats.header_path_mismatch = preflight.summary.header_path_mismatch.clone(); + normalize_pending_rust(&mut pending).await; apply_pending_writes(&pending)?; // Resolve absolute paths for LSP diagnostics query. let abs_paths: Vec = pending @@ -1291,7 +1296,32 @@ fn build_pending_writes_from_patches( Ok((pending, stats)) } +/// Normalize the Rust files a patch rewrites (#6205), before the write and +/// before the result's diff is built, so the rendered diff and the bytes on +/// disk are the same text and the model's next anchor matches reality. +async fn normalize_pending_rust(pending: &mut [PendingWrite]) { + for entry in pending.iter_mut() { + let (Some(content), Some(original)) = (entry.content.as_ref(), entry.original.as_ref()) + else { + continue; + }; + if let Some(normalized) = normalize_edit(&entry.path, original, content).await { + entry.content = Some(normalized); + } + } +} + fn apply_pending_writes(pending: &[PendingWrite]) -> Result<(), ToolError> { + // Syntax gate (#6204) ahead of the first write, not per file: a patch is + // transactional, so one unparseable result must leave every file in the + // patch untouched rather than half-applied and rolled back. + for entry in pending { + if let Some(content) = entry.content.as_ref() { + let display = entry.path.display().to_string(); + guard_edit(&entry.path, &display, entry.original.as_deref(), content)?; + } + } + let mut applied = Vec::new(); for entry in pending { @@ -1642,6 +1672,59 @@ mod tests { assert_eq!(hunks[0].new_count, 3); } + /// #6205 — a patch that lands unformatted Rust in an already-clean file is + /// normalized before the write, and the rendered diff shows the normalized + /// text, so the model's next patch context matches the bytes on disk. + #[tokio::test] + async fn patch_normalizes_rust_in_an_already_clean_file() { + let tmp = tempdir().expect("tempdir"); + let ctx = ToolContext::new(tmp.path().to_path_buf()); + let file = tmp.path().join("clean.rs"); + fs::write(&file, "fn main() {\n let x = 1;\n}\n").expect("write"); + + let patch = "--- a/clean.rs\n+++ b/clean.rs\n@@ -1,3 +1,4 @@\n fn main() {\n let x = 1;\n+ let y=2;\n }\n"; + let result = ApplyPatchTool + .execute(json!({"path": "clean.rs", "patch": patch}), &ctx) + .await + .expect("execute"); + + assert_eq!( + fs::read_to_string(&file).expect("read"), + "fn main() {\n let x = 1;\n let y = 2;\n}\n" + ); + let diff = result.metadata.as_ref().expect("metadata")["mutation"]["diff"] + .as_str() + .expect("diff") + .to_string(); + assert!(diff.contains("+ let y = 2;"), "{diff}"); + assert!(!diff.contains("let y=2;"), "{diff}"); + } + + /// #6204 — a patch whose result does not parse is refused before any file + /// is written, so a multi-file patch cannot land half-applied. + #[tokio::test] + async fn patch_refuses_a_hunk_that_breaks_rust_syntax() { + let tmp = tempdir().expect("tempdir"); + let ctx = ToolContext::new(tmp.path().to_path_buf()); + let file = tmp.path().join("main.rs"); + let original = "fn main() {\n println!(\"hi\");\n}\n"; + fs::write(&file, original).expect("write"); + + let patch = "--- a/main.rs\n+++ b/main.rs\n@@ -1,3 +1,2 @@\n fn main() {\n println!(\"hi\");\n-}\n"; + let error = ApplyPatchTool + .execute(json!({"path": "main.rs", "patch": patch}), &ctx) + .await + .expect_err("a patch that breaks Rust syntax must be refused"); + + let message = error.to_string(); + assert!(message.contains("Rust syntax error at line"), "{message}"); + assert_eq!( + fs::read_to_string(&file).expect("read"), + original, + "a refused patch must leave the file untouched" + ); + } + #[test] fn input_schema_exposes_replace_and_deprecated_changes_alias() { let schema = ApplyPatchTool.input_schema(); diff --git a/crates/tui/src/tools/file.rs b/crates/tui/src/tools/file.rs index c05da74d57..6ff9cd1339 100644 --- a/crates/tui/src/tools/file.rs +++ b/crates/tui/src/tools/file.rs @@ -7,10 +7,12 @@ //! with path validation to prevent escaping the workspace boundary. use super::diff_format::make_unified_diff; +use super::rust_format::{NORMALIZED_NOTE, normalize_edit}; use super::spec::{ ApprovalRequirement, RichToolResult, ToolCapability, ToolContext, ToolError, ToolResult, ToolSpec, lsp_diagnostics_for_paths, optional_str, optional_u64, required_str, }; +use super::syntax_check::guard_edit; use async_trait::async_trait; use serde_json::{Value, json}; use std::borrow::Cow; @@ -1443,7 +1445,18 @@ impl WriteFileTool { // Preserve the existing file's line-ending style on overwrite (see // `preserve_prior_line_endings`); otherwise a CRLF (Windows) file is // silently rewritten with LF line endings. - let written = preserve_prior_line_endings(file_content, &prior_contents); + let mut written = preserve_prior_line_endings(file_content, &prior_contents); + guard_edit( + &file_path, + path_str, + existed_before.then(|| prior_contents.as_ref()), + &written, + )?; + if existed_before + && let Some(normalized) = normalize_edit(&file_path, &prior_contents, &written).await + { + written = normalized; + } crate::utils::write_atomic_workspace(&file_path, written.as_bytes()).map_err(|error| { ToolError::execution_failed(format!("Failed to write {}: {error}", file_path.display())) })?; @@ -1561,7 +1574,19 @@ impl ToolSpec for WriteFileTool { // Preserve the existing file's line-ending style on overwrite (see // `preserve_prior_line_endings`); a full `write_file` over a CRLF // (Windows) file otherwise silently rewrites every line ending to LF. - let written = preserve_prior_line_endings(file_content, &prior_contents); + let mut written = preserve_prior_line_endings(file_content, &prior_contents); + + guard_edit( + &file_path, + path_str, + existed_before.then(|| prior_contents.as_ref()), + &written, + )?; + if existed_before + && let Some(normalized) = normalize_edit(&file_path, &prior_contents, &written).await + { + written = normalized; + } crate::utils::write_atomic_workspace(&file_path, written.as_bytes()).map_err(|e| { ToolError::execution_failed(format!("Failed to write {}: {}", file_path.display(), e)) @@ -2003,7 +2028,11 @@ impl EditFileTool { let normalized = normalize_contract_line_endings(without_bom); let updated = apply_contract_edits(&normalized, &edits, path_str)?; check_file_operation_cancelled(context)?; - let final_content = format!("{bom}{}", restore_contract_line_endings(&updated, ending)); + let mut final_content = format!("{bom}{}", restore_contract_line_endings(&updated, ending)); + guard_edit(&file_path, path_str, Some(&raw), &final_content)?; + if let Some(normalized) = normalize_edit(&file_path, &raw, &final_content).await { + final_content = normalized; + } crate::utils::write_atomic_workspace(&file_path, final_content.as_bytes()).map_err( |error| { @@ -2244,6 +2273,18 @@ impl ToolSpec for EditFileTool { )); } + guard_edit(&file_path, path_str, Some(&contents), &updated)?; + + // #6205 — normalize after the syntax gate so the next turn's anchors + // match the bytes on disk rather than the text the model emitted. + let normalized_formatting = match normalize_edit(&file_path, &contents, &updated).await { + Some(normalized) => { + updated = normalized; + true + } + None => false, + }; + crate::utils::write_atomic_workspace(&file_path, updated.as_bytes()).map_err(|e| { ToolError::execution_failed(format!("Failed to write {}: {}", file_path.display(), e)) })?; @@ -2279,7 +2320,12 @@ impl ToolSpec for EditFileTool { Some(other) => other, None => "", }; - let summary = format!("Replaced 1 occurrence in {display}{fuzz_note}"); + let format_note = if normalized_formatting { + NORMALIZED_NOTE + } else { + "" + }; + let summary = format!("Replaced 1 occurrence in {display}{fuzz_note}{format_note}"); let body = if diff.is_empty() { format!("{summary}\n(no textual changes)") } else { diff --git a/crates/tui/src/tools/file/tests/tools.rs b/crates/tui/src/tools/file/tests/tools.rs index 4b5ea863e0..4d1ab1d58f 100644 --- a/crates/tui/src/tools/file/tests/tools.rs +++ b/crates/tui/src/tools/file/tests/tools.rs @@ -885,6 +885,203 @@ async fn edit_file_tool_preserves_executable_bits() { ); } +/// #6205 — a sloppy edit to a rustfmt-clean file lands normalized, and the +/// tool result's returned diff matches the bytes on disk, so the model's next +/// anchor is the real text. +#[tokio::test] +async fn edit_file_normalizes_a_sloppy_edit_in_a_rustfmt_clean_file() { + let tmp = tempdir().expect("tempdir"); + let ctx = ToolContext::new(tmp.path().to_path_buf()); + let path = tmp.path().join("clean.rs"); + fs::write(&path, "fn main() {\n let x = 1;\n}\n").expect("write"); + read_before_edit(&ctx, "clean.rs").await; + + let result = EditFileTool + .execute( + json!({ + "path": "clean.rs", + "search": " let x = 1;", + "replace": " let x = 1;\n let y=2;", + }), + &ctx, + ) + .await + .expect("execute"); + + // No skip-if-missing branch: rustfmt ships with the pinned toolchain, and a + // test that passes vacuously without it proves nothing. + assert_eq!( + fs::read_to_string(&path).expect("read"), + "fn main() {\n let x = 1;\n let y = 2;\n}\n" + ); + assert!( + result.content.contains("rustfmt-normalized"), + "the result must say the content was normalized: {}", + result.content + ); + let diff = result.metadata.as_ref().expect("metadata")["mutation"]["diff"] + .as_str() + .expect("diff") + .to_string(); + assert!( + diff.contains("+ let y = 2;"), + "the returned diff must show the normalized text, not what was sent: {diff}" + ); + assert!(!diff.contains("let y=2;"), "{diff}"); +} + +/// A file the author formats by hand is never reformatted wholesale. +#[tokio::test] +async fn edit_file_leaves_a_hand_formatted_file_alone() { + let tmp = tempdir().expect("tempdir"); + let ctx = ToolContext::new(tmp.path().to_path_buf()); + let path = tmp.path().join("handmade.rs"); + // Two-space indentation: rustfmt would rewrite every line of this file. + fs::write(&path, "fn main() {\n let x = 1;\n}\n").expect("write"); + read_before_edit(&ctx, "handmade.rs").await; + + EditFileTool + .execute( + json!({ + "path": "handmade.rs", + "search": " let x = 1;", + "replace": " let x = 1;\n let y = 2;", + }), + &ctx, + ) + .await + .expect("execute"); + + assert_eq!( + fs::read_to_string(&path).expect("read"), + "fn main() {\n let x = 1;\n let y = 2;\n}\n", + "unrelated user formatting must survive the edit" + ); +} + +/// #6206 — a dependency bump that leaves `Cargo.toml` unparseable is refused +/// at edit time, not discovered by the next `cargo` invocation. +#[tokio::test] +async fn edit_file_refuses_an_edit_that_breaks_a_cargo_manifest() { + let tmp = tempdir().expect("tempdir"); + let ctx = ToolContext::new(tmp.path().to_path_buf()); + let path = tmp.path().join("Cargo.toml"); + let original = "[dependencies]\nserde = \"1.0\"\n"; + fs::write(&path, original).expect("write"); + read_before_edit(&ctx, "Cargo.toml").await; + + let error = EditFileTool + .execute( + json!({ + "path": "Cargo.toml", + "search": "serde = \"1.0\"", + // Unterminated string: the classic half-finished version bump. + "replace": "serde = \"1.0", + }), + &ctx, + ) + .await + .expect_err("an unparseable manifest must be refused"); + + let message = error.to_string(); + assert!(message.contains("TOML syntax error at line"), "{message}"); + assert_eq!( + fs::read_to_string(&path).expect("read"), + original, + "a refused edit must leave the manifest unchanged" + ); +} + +/// A valid structured-config edit is untouched by the gate. +#[tokio::test] +async fn edit_file_applies_a_valid_json_edit() { + let tmp = tempdir().expect("tempdir"); + let ctx = ToolContext::new(tmp.path().to_path_buf()); + let path = tmp.path().join("data.json"); + fs::write(&path, "{\n \"port\": 8080\n}\n").expect("write"); + read_before_edit(&ctx, "data.json").await; + + EditFileTool + .execute( + json!({ + "path": "data.json", + "search": "8080", + "replace": "9090", + }), + &ctx, + ) + .await + .expect("a valid JSON edit must proceed unchanged"); + + assert_eq!( + fs::read_to_string(&path).expect("read"), + "{\n \"port\": 9090\n}\n" + ); +} + +/// #6204 — an edit that takes a parseable Rust file to an unparseable one is +/// refused before the write, with a `line:column` from `syn`. +#[tokio::test] +async fn edit_file_refuses_an_edit_that_breaks_rust_syntax() { + let tmp = tempdir().expect("tempdir"); + let ctx = ToolContext::new(tmp.path().to_path_buf()); + let path = tmp.path().join("main.rs"); + let original = "fn main() {\n println!(\"hi\");\n}\n"; + fs::write(&path, original).expect("write"); + read_before_edit(&ctx, "main.rs").await; + + let error = EditFileTool + .execute( + json!({ + "path": "main.rs", + // Same brace balance, so the payload-corruption heuristic has + // no objection; the parenthesis is what breaks the grammar. + "search": "fn main() {", + "replace": "fn main( {", + }), + &ctx, + ) + .await + .expect_err("an edit that breaks Rust syntax must be refused"); + + let message = error.to_string(); + assert!(message.contains("Rust syntax error at line"), "{message}"); + assert!(message.contains("Nothing was written"), "{message}"); + assert_eq!( + fs::read_to_string(&path).expect("read"), + original, + "a refused edit must leave the file byte-for-byte unchanged" + ); +} + +/// The gate catches the edit that *introduces* breakage, never the one that +/// repairs it: a file that already fails to parse stays editable. +#[tokio::test] +async fn edit_file_still_repairs_an_already_broken_rust_file() { + let tmp = tempdir().expect("tempdir"); + let ctx = ToolContext::new(tmp.path().to_path_buf()); + let path = tmp.path().join("broken.rs"); + fs::write(&path, "fn main( {\n println!(\"hi\");\n}\n").expect("write"); + read_before_edit(&ctx, "broken.rs").await; + + EditFileTool + .execute( + json!({ + "path": "broken.rs", + "search": "fn main( {", + "replace": "fn main() {", + }), + &ctx, + ) + .await + .expect("repairing a broken file must not be gated"); + + assert_eq!( + fs::read_to_string(&path).expect("read"), + "fn main() {\n println!(\"hi\");\n}\n" + ); +} + #[tokio::test] async fn edit_file_refuses_brace_collapsed_match_arm_payload() { let tmp = tempdir().expect("tempdir"); diff --git a/crates/tui/src/tools/fim.rs b/crates/tui/src/tools/fim.rs index 1c43164553..cdab60f367 100644 --- a/crates/tui/src/tools/fim.rs +++ b/crates/tui/src/tools/fim.rs @@ -166,6 +166,11 @@ impl ToolSpec for FimEditTool { // 7. Build the new content and write it back let generated_len = generated_text.len(); let new_content = format!("{fim_prompt}{generated_text}{fim_suffix}"); + super::syntax_check::guard_edit(&resolved, path, Some(&content), &new_content)?; + // Deliberately not rustfmt-normalized (#6205): this result reports + // `prefix_end`/`suffix_start` as byte offsets into the written file, + // and reformatting would move them. The syntax gate applies; the + // formatting normalization does not. crate::utils::write_atomic_workspace(&resolved, new_content.as_bytes()).map_err(|e| { ToolError::execution_failed(format!("Failed to write {}: {}", resolved.display(), e)) })?; diff --git a/crates/tui/src/tools/mod.rs b/crates/tui/src/tools/mod.rs index 36624c5a08..85a621f4c3 100644 --- a/crates/tui/src/tools/mod.rs +++ b/crates/tui/src/tools/mod.rs @@ -57,6 +57,7 @@ pub(crate) mod review_pr; pub mod rlm; pub mod run_tool; pub mod runtime_mcp; +mod rust_format; pub mod schema_canonicalize; pub mod schema_sanitize; pub mod search; @@ -68,6 +69,7 @@ pub mod skill; pub mod spec; pub mod speech; pub mod subagent; +mod syntax_check; pub mod tasks; #[cfg(not(target_env = "ohos"))] pub mod terminal_session; diff --git a/crates/tui/src/tools/rust_format.rs b/crates/tui/src/tools/rust_format.rs new file mode 100644 index 0000000000..8fa0f4ac8e --- /dev/null +++ b/crates/tui/src/tools/rust_format.rs @@ -0,0 +1,238 @@ +//! Post-edit formatting normalization for Rust files (#6205). +//! +//! Model-generated edits rarely match `rustfmt` output exactly. Once an +//! unformatted edit lands, the *next* turn's `old_string` or patch context was +//! written against text that `cargo fmt` is about to move, so anchors drift and +//! the follow-up edit fails to match. Normalizing at edit time keeps anchors +//! stable for the rest of the session, and the tool result returns the +//! normalized text, so what the model remembers writing is what is on disk. +//! +//! `rustfmt` is the formatter, not `prettyplease`: `cargo fmt --check` is this +//! repository's actual gate, and a second formatter with its own opinions would +//! produce files that pass the edit path and fail the gate. Shelling out also +//! picks up the project's own `rustfmt.toml` — an in-process pretty-printer +//! cannot. +//! +//! # Policy +//! +//! Normalization applies to the whole edited file, and **only when that file +//! was already `rustfmt`-clean before the edit**. A file whose formatting the +//! author has not handed to `rustfmt` is never rewritten. Because a clean file +//! is a formatting fixpoint, reformatting it after an edit can only change the +//! edited region — so "whole file" and "edited region" coincide, without +//! needing span arithmetic to prove it. +//! +//! # Known limitations +//! +//! - **Non-fatal, always.** A missing `rustfmt`, a parse failure, a timeout, or +//! a non-zero exit skips normalization; the edit still lands. Nothing here +//! can fail an edit. +//! - **Silent when skipped.** Only an applied normalization is announced. A +//! "formatting skipped" note on every edit in a project without `rustfmt` +//! would be noise the model cannot act on, and "skipped" is indistinguishable +//! from "would have changed nothing" without running the formatter anyway. +//! - **Edition 2024 is assumed.** A file that `rustfmt` cannot parse under that +//! edition fails the clean-before check and is skipped, so the assumption +//! degrades to "no normalization", never to a mangled file. +//! - **CRLF files are skipped.** `rustfmt` emits LF; rewriting every line +//! ending is exactly the unrelated-churn this policy exists to avoid. + +use std::path::{Path, PathBuf}; +use std::process::Stdio; +use std::time::Duration; + +use tokio::io::AsyncWriteExt; +use tokio::process::Command; + +/// Files larger than this are not normalized. Two formatter runs on a +/// multi-megabyte file cost more interactive latency than stable anchors are +/// worth. +const MAX_FORMATTED_BYTES: usize = 1024 * 1024; + +/// Wall-clock budget for one `rustfmt` run. +const FORMAT_TIMEOUT: Duration = Duration::from_secs(5); + +/// Suffix appended to an edit summary when the content was normalized, so the +/// model knows the returned text is not byte-identical to what it sent. +pub(super) const NORMALIZED_NOTE: &str = " (rustfmt-normalized)"; + +/// Normalize `after` when the edit landed in an already-`rustfmt`-clean file. +/// +/// Returns the formatted text to write instead of `after`, or `None` to leave +/// `after` exactly as the caller produced it. Every failure path returns +/// `None`: normalization is a convenience and may never break an edit. +pub(super) async fn normalize_edit(path: &Path, before: &str, after: &str) -> Option { + if path.extension()?.to_str()? != "rs" { + return None; + } + if after.len() > MAX_FORMATTED_BYTES || before.len() > MAX_FORMATTED_BYTES { + return None; + } + // Preserving a CRLF file's line endings outranks normalizing its layout. + if after.contains('\r') || before.contains('\r') { + return None; + } + + let formatted = rustfmt(path, after).await?; + if formatted == after { + // Already canonical. The common case, and it costs one run, not two. + return None; + } + // Only now is the second run worth paying for: was this file the author's + // to format, or `rustfmt`'s? + if rustfmt(path, before).await? != before { + return None; + } + Some(formatted) +} + +/// Find the `rustfmt.toml` governing `path`, walking up to the filesystem root. +/// +/// `None` means no config file exists, which is the signal to omit +/// `--config-path` entirely and let `rustfmt` use its defaults. +async fn nearest_config(path: &Path) -> Option { + let mut directory = path.parent()?; + loop { + for name in ["rustfmt.toml", ".rustfmt.toml"] { + let candidate = directory.join(name); + if tokio::fs::try_exists(&candidate).await.unwrap_or(false) { + return Some(candidate); + } + } + directory = directory.parent()?; + } +} + +/// Run `rustfmt` over `source`, returning its output, or `None` on any failure. +async fn rustfmt(path: &Path, source: &str) -> Option { + let mut command = Command::new("rustfmt"); + command + .arg("--emit") + .arg("stdout") + .arg("--edition") + .arg("2024") + .arg("--quiet"); + // Pick up the project's own `rustfmt.toml`: with stdin input there is no + // file path for `rustfmt` to search upward from. The flag must name a + // config file that actually exists — pointed at a directory without one, + // `rustfmt` exits 1 with "unable to find a config file", which would + // silently disable normalization everywhere. + if let Some(config) = nearest_config(path).await { + command.arg("--config-path").arg(config); + } + let mut child = command + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .stderr(Stdio::null()) + // A hung formatter must not outlive the edit that spawned it. + .kill_on_drop(true) + .spawn() + .ok()?; + + let mut stdin = child.stdin.take()?; + let payload = source.to_string(); + // Write and wait concurrently: `rustfmt` streams its output, so writing the + // whole input before reading can deadlock on a full pipe buffer. + let writer = tokio::spawn(async move { + let _ = stdin.write_all(payload.as_bytes()).await; + let _ = stdin.shutdown().await; + }); + + let output = match tokio::time::timeout(FORMAT_TIMEOUT, child.wait_with_output()).await { + Ok(Ok(output)) => output, + // A timed-out child is already killed by dropping the future's handle + // on the `wait_with_output` path; either way the edit proceeds. + Ok(Err(_)) | Err(_) => { + writer.abort(); + return None; + } + }; + writer.abort(); + + if !output.status.success() { + return None; + } + String::from_utf8(output.stdout).ok() +} + +#[cfg(test)] +mod tests { + use super::*; + use std::path::PathBuf; + + fn rust_path() -> PathBuf { + PathBuf::from("src/lib.rs") + } + + /// `rustfmt` ships with the toolchain this repository pins (rustup's + /// default profile), and `cargo fmt --check` is a standing gate here, so + /// its absence is a broken environment, not a reason to skip. A test that + /// passes vacuously without the formatter proves nothing — that exact + /// hazard hid a `--config-path` bug which disabled normalization + /// everywhere while the suite stayed green. + async fn require_rustfmt() { + assert_eq!( + rustfmt(&rust_path(), "fn main( ) {}\n").await.as_deref(), + Some("fn main() {}\n"), + "rustfmt must be on PATH for the formatting tests" + ); + } + + #[tokio::test] + async fn misformatted_edit_in_a_clean_file_is_normalized() { + require_rustfmt().await; + let before = "fn main() {\n let x = 1;\n}\n"; + let after = "fn main() {\n let x = 1;\n let y=2;\n}\n"; + let normalized = normalize_edit(&rust_path(), before, after) + .await + .expect("a clean file must be renormalized after a sloppy edit"); + assert_eq!( + normalized, + "fn main() {\n let x = 1;\n let y = 2;\n}\n" + ); + } + + #[tokio::test] + async fn a_file_the_author_formats_by_hand_is_left_alone() { + require_rustfmt().await; + // `rustfmt` would rewrite this file wholesale, so it was never its to + // format: the edit lands verbatim. + let before = "fn main() {\n let x = 1;\n}\n"; + let after = "fn main() {\n let x = 1;\n let y=2;\n}\n"; + assert!(normalize_edit(&rust_path(), before, after).await.is_none()); + } + + #[tokio::test] + async fn already_canonical_content_needs_no_rewrite() { + require_rustfmt().await; + let before = "fn main() {\n let x = 1;\n}\n"; + let after = "fn main() {\n let x = 1;\n let y = 2;\n}\n"; + assert!(normalize_edit(&rust_path(), before, after).await.is_none()); + } + + #[tokio::test] + async fn crlf_files_keep_their_line_endings() { + let before = "fn main() {\r\n let x = 1;\r\n}\r\n"; + let after = "fn main() {\r\n let x = 1;\r\n}\r\n"; + assert!(normalize_edit(&rust_path(), before, after).await.is_none()); + } + + #[tokio::test] + async fn non_rust_files_are_not_formatted() { + assert!( + normalize_edit(Path::new("data.json"), "{}", "{ }") + .await + .is_none() + ); + } + + #[tokio::test] + async fn unparseable_content_degrades_to_no_normalization() { + // `rustfmt` cannot parse this; the edit must still be allowed to land. + assert!( + normalize_edit(&rust_path(), "fn main() {}\n", "fn main( {\n") + .await + .is_none() + ); + } +} diff --git a/crates/tui/src/tools/syntax_check.rs b/crates/tui/src/tools/syntax_check.rs new file mode 100644 index 0000000000..0fb99f66e4 --- /dev/null +++ b/crates/tui/src/tools/syntax_check.rs @@ -0,0 +1,372 @@ +//! Post-edit syntax gate for the file-mutating tools (#6204, #6206). +//! +//! Every tool that rewrites a file — `File write`/`edit`/`patch` in +//! [`super::file`] and [`super::apply_patch`], plus [`super::fim`] — routes its +//! post-edit content through [`guard_edit`] *before* the bytes reach the disk. +//! When the edit would leave a file unparseable, the write never happens and +//! the model gets a `line:column` error it can act on this turn instead of a +//! compiler failure one or more turns later. +//! +//! Rust is parsed by `syn`, which implements the real language grammar and +//! reports grammar-exact errors (as opposed to an error-tolerant CST that +//! builds a tree *around* malformed input). TOML and JSON go through +//! `toml_edit` and `serde_json` — the parsers this crate already loads its own +//! config with. A malformed `Cargo.toml` fails the *entire* workspace build +//! rather than one file, so catching it at edit time is worth more there than +//! anywhere else. +//! +//! # The gate only rejects *newly* introduced breakage +//! +//! [`guard_edit`] fails open unless the file parsed **before** the edit and +//! fails to parse **after** it. That asymmetry is the whole safety argument: +//! repairing a file that is already broken — the single most common reason an +//! agent edits a source file at all — must never be blocked by a gate whose +//! job is to catch the edit that broke it. Creating a new file is likewise +//! ungated, since there is no "before" to have regressed. +//! +//! # Known limitations +//! +//! - **Grammar, not semantics.** A file that parses can still fail to compile; +//! type errors stay the compiler's and the LSP hook's job +//! (`core::engine::lsp_hooks`). +//! - **`syn` tracks the editions it knows.** Source using syntax newer than the +//! pinned `syn` would be reported as a parse error. Because the gate requires +//! the pre-edit file to have parsed, such a file is skipped entirely rather +//! than becoming uneditable. +//! - **Extension-driven.** A Rust file that is not named `*.rs` is not checked; +//! language detection by content is deliberately not attempted. `.jsonc`, +//! `.json5`, and `.jsonl` are *not* treated as JSON: they are different +//! grammars, and a strict parser would reject valid files. +//! - **A `.json` file that is really JSONC** — `tsconfig.json` with comments is +//! the usual one — does not parse before the edit either, so the gate skips +//! it rather than making it uneditable. +//! - **Bounded by [`MAX_CHECKED_BYTES`].** Larger files skip the check rather +//! than spend edit-path latency on a multi-megabyte parse. + +use std::fmt; +use std::path::Path; + +use super::spec::ToolError; + +/// Files larger than this skip the syntax gate. +/// +/// Parsing is linear and fast, but the check sits on the interactive edit path +/// and runs twice on a rejection. 2 MiB covers every hand-written source file +/// in this workspace by a wide margin; past that the file is generated or +/// vendored, where a syntax verdict is worth less than the latency. +const MAX_CHECKED_BYTES: usize = 2 * 1024 * 1024; + +/// A language the edit path can parse. Extensions outside this set fail open. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(super) enum SyntaxLanguage { + Rust, + Toml, + Json, +} + +impl SyntaxLanguage { + /// Pick a parser from the file extension, or `None` to skip the check. + fn from_path(path: &Path) -> Option { + let extension = path.extension()?.to_str()?.to_ascii_lowercase(); + match extension.as_str() { + "rs" => Some(Self::Rust), + // Covers `Cargo.toml`, `deny.toml`, `.cargo/config.toml` and every + // ordinary `*.toml` uniformly — the extension is the whole rule. + "toml" => Some(Self::Toml), + "json" => Some(Self::Json), + _ => None, + } + } + + fn label(self) -> &'static str { + match self { + Self::Rust => "Rust", + Self::Toml => "TOML", + Self::Json => "JSON", + } + } +} + +/// A parse failure, located precisely enough for the model to fix it directly. +#[derive(Debug, Clone)] +pub(super) struct SyntaxIssue { + language: SyntaxLanguage, + /// 1-based line, as every editor and compiler reports it. + line: usize, + /// 1-based column, likewise. + column: usize, + message: String, +} + +impl fmt::Display for SyntaxIssue { + fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + write!( + formatter, + "{} syntax error at line {}, column {}: {}", + self.language.label(), + self.line, + self.column, + self.message + ) + } +} + +/// Parse `source` as the language implied by `path`. +/// +/// `None` means "no objection": the content parses, the extension is not one +/// we parse, or the file is too large to be worth checking. +pub(super) fn syntax_check(path: &Path, source: &str) -> Option { + if source.len() > MAX_CHECKED_BYTES { + return None; + } + match SyntaxLanguage::from_path(path)? { + SyntaxLanguage::Rust => check_rust(source), + SyntaxLanguage::Toml => check_toml(source), + SyntaxLanguage::Json => check_json(source), + } +} + +/// Refuse an edit that would take a parseable file to an unparseable one. +/// +/// `before` is the pre-edit content, or `None` when the edit creates the file. +/// Callers invoke this before writing, so a rejection leaves the file on disk +/// byte-for-byte untouched and there is nothing to roll back. +pub(super) fn guard_edit( + path: &Path, + display_path: &str, + before: Option<&str>, + after: &str, +) -> Result<(), ToolError> { + let Some(issue) = syntax_check(path, after) else { + return Ok(()); + }; + // Fail open on a file that was already broken (or is brand new): the gate + // exists to catch the edit that *introduces* a syntax error, never to + // strand a model that is repairing one. + let Some(before) = before else { + return Ok(()); + }; + if syntax_check(path, before).is_some() { + return Ok(()); + } + Err(ToolError::execution_failed(format!( + "Edit refused: it would leave {display_path} unparseable — {issue}. Nothing was written; \ + the file is unchanged. Recovery: re-read the file with File action=\"read\", check the \ + replacement for unbalanced delimiters or a truncated block, and retry." + ))) +} + +fn check_rust(source: &str) -> Option { + let error = syn::parse_file(source).err()?; + // A `syn::Error` can carry several diagnostics; the first is the earliest + // and the one worth showing. + let first = error.into_iter().next()?; + let start = first.span().start(); + Some(SyntaxIssue { + language: SyntaxLanguage::Rust, + line: start.line, + // `proc-macro2` columns are 0-based; editors and rustc are not. + column: start.column.saturating_add(1), + message: first.to_string(), + }) +} + +fn check_toml(source: &str) -> Option { + let error = source.parse::().err()?; + let (line, column) = error + .span() + .map_or((1, 1), |span| line_column(source, span.start)); + Some(SyntaxIssue { + language: SyntaxLanguage::Toml, + line, + column, + message: error.message().trim().to_string(), + }) +} + +fn check_json(source: &str) -> Option { + let error = serde_json::from_str::(source).err()?; + let rendered = error.to_string(); + // `serde_json` already appends " at line L column C" to its message; the + // location is reported in its own fields, so drop the duplicate tail. + let message = rendered + .split_once(" at line ") + .map_or(rendered.as_str(), |(head, _)| head); + Some(SyntaxIssue { + language: SyntaxLanguage::Json, + // A zero means "position unknown" (e.g. an IO-shaped error); the + // 1-based floor keeps the rendered location honest either way. + line: error.line().max(1), + column: error.column().max(1), + message: message.to_string(), + }) +} + +/// Translate a byte offset into a 1-based line and column. +/// +/// `toml_edit` reports a byte span; every human-facing tool reports line and +/// column. Counting is over `char`s rather than bytes so a column lands where +/// the reader's cursor does in a file with non-ASCII content. +fn line_column(source: &str, offset: usize) -> (usize, usize) { + let offset = offset.min(source.len()); + let head = &source[..offset]; + let line = head.matches('\n').count() + 1; + let column = head + .rfind('\n') + .map_or(head, |index| &head[index + 1..]) + .chars() + .count() + + 1; + (line, column) +} + +#[cfg(test)] +mod tests { + use super::*; + use std::path::PathBuf; + + fn rust_path() -> PathBuf { + PathBuf::from("src/lib.rs") + } + + #[test] + fn valid_rust_passes() { + assert!(syntax_check(&rust_path(), "fn main() {}\n").is_none()); + } + + #[test] + fn missing_brace_reports_line_and_column() { + let issue = syntax_check(&rust_path(), "fn main() {\n let x = 1;\n") + .expect("unbalanced brace must be reported"); + assert_eq!(issue.language, SyntaxLanguage::Rust); + assert!(issue.line >= 1, "{issue}"); + assert!(issue.column >= 1, "{issue}"); + let rendered = issue.to_string(); + assert!(rendered.contains("Rust syntax error at line"), "{rendered}"); + } + + #[test] + fn valid_toml_passes() { + assert!(syntax_check(Path::new("Cargo.toml"), "[package]\nname = \"x\"\n").is_none()); + } + + #[test] + fn broken_toml_reports_line_and_column() { + let issue = syntax_check(Path::new("Cargo.toml"), "[package]\nname = \n") + .expect("a value-less key must be reported"); + assert_eq!(issue.language, SyntaxLanguage::Toml); + assert_eq!(issue.line, 2, "{issue}"); + let rendered = issue.to_string(); + assert!( + rendered.contains("TOML syntax error at line 2"), + "{rendered}" + ); + } + + #[test] + fn valid_json_passes() { + assert!(syntax_check(Path::new("data.json"), "{\"a\": [1, 2]}").is_none()); + } + + #[test] + fn broken_json_reports_line_and_column() { + let issue = syntax_check(Path::new("data.json"), "{\n \"a\": [1, 2,\n}\n") + .expect("a trailing comma must be reported"); + assert_eq!(issue.language, SyntaxLanguage::Json); + assert_eq!(issue.line, 3, "{issue}"); + let rendered = issue.to_string(); + assert!( + rendered.contains("JSON syntax error at line 3"), + "{rendered}" + ); + assert!( + !rendered.contains("at line 3 column"), + "serde_json's duplicate location tail must be stripped: {rendered}" + ); + } + + #[test] + fn jsonc_with_comments_is_not_parsed_as_json_before_the_edit() { + // A `.json` file that is really JSONC does not parse either way, so + // the before/after rule skips it instead of making it uneditable. + let commented = "{\n // note\n \"a\": 1\n}\n"; + assert!(syntax_check(Path::new("tsconfig.json"), commented).is_some()); + guard_edit( + Path::new("tsconfig.json"), + "tsconfig.json", + Some(commented), + "{\n // note\n \"a\": 2\n}\n", + ) + .expect("a JSONC file must stay editable"); + } + + #[test] + fn guard_rejects_an_edit_that_breaks_a_manifest() { + let error = guard_edit( + Path::new("Cargo.toml"), + "Cargo.toml", + Some("[package]\nname = \"x\"\n"), + "[package\nname = \"x\"\n", + ) + .expect_err("an unparseable manifest must be refused at edit time"); + let message = error.to_string(); + assert!(message.contains("TOML syntax error at line"), "{message}"); + assert!(message.contains("Nothing was written"), "{message}"); + } + + #[test] + fn unknown_extension_is_skipped() { + assert!(syntax_check(Path::new("notes.txt"), "fn main() {").is_none()); + } + + #[test] + fn oversized_source_is_skipped() { + let huge = format!("fn main() {{{}", " ".repeat(MAX_CHECKED_BYTES)); + assert!(syntax_check(&rust_path(), &huge).is_none()); + } + + #[test] + fn guard_rejects_newly_broken_rust() { + let error = guard_edit( + &rust_path(), + "src/lib.rs", + Some("fn main() {}\n"), + "fn main() {\n", + ) + .expect_err("an edit that breaks a parseable file must be refused"); + let message = error.to_string(); + assert!(message.contains("src/lib.rs"), "{message}"); + assert!(message.contains("Rust syntax error at line"), "{message}"); + assert!(message.contains("Nothing was written"), "{message}"); + } + + #[test] + fn guard_allows_repairing_an_already_broken_file() { + // Still broken after the edit, but it was broken before: a model + // mid-repair must not be locked out. + guard_edit( + &rust_path(), + "src/lib.rs", + Some("fn main() {\n"), + "fn main() {\n let x = 1;\n", + ) + .expect("pre-existing breakage must fail open"); + } + + #[test] + fn guard_allows_creating_a_new_file() { + guard_edit(&rust_path(), "src/lib.rs", None, "fn main() {\n") + .expect("file creation has no prior state to regress"); + } + + #[test] + fn guard_allows_a_valid_edit() { + guard_edit( + &rust_path(), + "src/lib.rs", + Some("fn main() {}\n"), + "fn main() {\n println!(\"hi\");\n}\n", + ) + .expect("a syntactically valid edit must pass"); + } +} diff --git a/web/lib/changelog.generated.ts b/web/lib/changelog.generated.ts index b9f94bc0a2..f4f6e783e9 100644 --- a/web/lib/changelog.generated.ts +++ b/web/lib/changelog.generated.ts @@ -30,12 +30,14 @@ export const CHANGELOG: ChangelogRelease[] = [ { "heading": "Added", "items": [ + "File edits are parse-gated before the write lands: Rust goes through syn::parse_file for a grammar-exact line:column, and .toml / .json through the parsers already vendored. An edit is refused only when the file parsed *before* and would not parse *after* — repairing an already-broken file is the commonest reason to edit source at all, so pre-existing breakage and new files fail open. The check precedes the write, so a rejection leaves the file untouched and apply_patch…", + "Rust files that were already rustfmt-clean are re-normalized after an edit, so the next patch's anchors still match. Hand-formatted files are never rewritten, and every failure path skips and lets the edit land (#6205).", "Native clients can finish provider setup without dropping to the CLI: DELETE /v1/providers/{id}/key clears a Codewhale-owned credential through the same shared owner as codewhale auth clear, and GET /v1/providers now carries credentialSource / credentialWritable (plus a reason) so a client disables its control with a truthful explanation instead of letting a write fail late. A credential Codewhale does not own — a literal key in a config file, or an active external consent —…", "The interactive approval card can be bounded: [approval] timeout_seconds resolves an unanswered card to deny when the window elapses — the same fail-closed decision the external approval path takes — and the transcript says the bound denied the call, not the operator. Omitted or 0 keeps today's unbounded wait, so nothing changes unless you opt in (#6101).", "Transcript drag selection copies Markdown source by default: every cell the selection touches serializes through the same canonical path Ctrl-Y and /copy use, partial intersections round out to whole cells joined with blank lines, and the toast names the copied cell count. tui.selection_copy_markdown = false keeps the rendered-text payload (#6156).", "The Runtime API serves the workspace files a native client browses and edits: GET /v1/workspace/files lists one directory, GET /v1/workspace/files/read returns a bounded byte window with a whole-file SHA-256 revision, and PUT /v1/workspace/files writes atomically through the confined opener with revision-checked overwrites (409 on drift). .git is never served and symlinks are never followed. A saved session's oversized tool outputs are served as artifacts at GET…" ], - "itemCount": 4 + "itemCount": 6 }, { "heading": "Changed",