From 98c123ce39a742696ae5524dc59d32f01d1a424c Mon Sep 17 00:00:00 2001 From: CodeWhale Bot Date: Tue, 15 Sep 2026 13:55:04 -0700 Subject: [PATCH 1/4] feat(tools): reject edits that break Rust syntax, with syn line:column MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Agent file edits land as text. When a patch produces Rust that does not parse, the defect surfaces on the next compile — a turn later, as a wall of cascading errors instead of one actionable message. `syn` implements the real Rust grammar and reports a precise `line:column`, so the model can fix it on the same turn. `crates/tui/src/tools/syntax_check.rs` is the single gate seam for the whole edit path. `guard_edit` runs on post-edit content immediately before the bytes reach disk, at every write site: - `file.rs` — `write`, `edit`, and their small-contract twins - `apply_patch.rs` — once over all pending writes ahead of the first one, so a transactional patch cannot land half-applied - `fim.rs` — model-generated infill The gate refuses an edit 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 fails open, as does creating a new file. Because the check precedes the write, a rejection leaves the file byte-for-byte untouched and there is no rollback path to get wrong. `syn` 3.0.3 was already in the lockfile as a proc-macro build dependency; this reuses that exact version rather than vendoring a second parser (#6151). The only lock change is two lines under `codewhale-tui`. `proc-macro2/span-locations` is what turns a parse failure into the `line:column` the model needs. Evidence (all three regression tests were confirmed FAILED with `guard_edit` stubbed to `Ok(())`, then restored): cargo fmt --all -- --check clean cargo clippy -p codewhale-tui --all-targets --all-features --locked -- -D warnings ... clean cargo test -p codewhale-tui --lib --all-features --locked -- tools::syntax_check:: tools::file::tests::edit_file tools::apply_patch::tests:: tools::file::tests::write_ test result: ok. 80 passed; 0 failed; 0 ignored; 12668 filtered out Closes #6204 Co-Authored-By: Claude Opus 5 (1M context) --- Cargo.lock | 2 + crates/tui/Cargo.toml | 7 + crates/tui/src/tools/apply_patch.rs | 36 ++++ crates/tui/src/tools/file.rs | 17 ++ crates/tui/src/tools/file/tests/tools.rs | 63 ++++++ crates/tui/src/tools/fim.rs | 1 + crates/tui/src/tools/mod.rs | 1 + crates/tui/src/tools/syntax_check.rs | 235 +++++++++++++++++++++++ 8 files changed, 362 insertions(+) create mode 100644 crates/tui/src/tools/syntax_check.rs 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/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..da7caade14 100644 --- a/crates/tui/src/tools/apply_patch.rs +++ b/crates/tui/src/tools/apply_patch.rs @@ -20,6 +20,7 @@ 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; @@ -1292,6 +1293,16 @@ fn build_pending_writes_from_patches( } 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 +1653,31 @@ mod tests { assert_eq!(hunks[0].new_count, 3); } + /// #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..aa571b44ba 100644 --- a/crates/tui/src/tools/file.rs +++ b/crates/tui/src/tools/file.rs @@ -11,6 +11,7 @@ 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; @@ -1444,6 +1445,12 @@ impl WriteFileTool { // `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); + guard_edit( + &file_path, + path_str, + existed_before.then(|| prior_contents.as_ref()), + &written, + )?; crate::utils::write_atomic_workspace(&file_path, written.as_bytes()).map_err(|error| { ToolError::execution_failed(format!("Failed to write {}: {error}", file_path.display())) })?; @@ -1563,6 +1570,13 @@ impl ToolSpec for WriteFileTool { // (Windows) file otherwise silently rewrites every line ending to LF. let written = preserve_prior_line_endings(file_content, &prior_contents); + guard_edit( + &file_path, + path_str, + existed_before.then(|| prior_contents.as_ref()), + &written, + )?; + crate::utils::write_atomic_workspace(&file_path, written.as_bytes()).map_err(|e| { ToolError::execution_failed(format!("Failed to write {}: {}", file_path.display(), e)) })?; @@ -2004,6 +2018,7 @@ impl EditFileTool { 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)); + guard_edit(&file_path, path_str, Some(&raw), &final_content)?; crate::utils::write_atomic_workspace(&file_path, final_content.as_bytes()).map_err( |error| { @@ -2244,6 +2259,8 @@ impl ToolSpec for EditFileTool { )); } + guard_edit(&file_path, path_str, Some(&contents), &updated)?; + crate::utils::write_atomic_workspace(&file_path, updated.as_bytes()).map_err(|e| { ToolError::execution_failed(format!("Failed to write {}: {}", file_path.display(), e)) })?; diff --git a/crates/tui/src/tools/file/tests/tools.rs b/crates/tui/src/tools/file/tests/tools.rs index 4b5ea863e0..7204aa30ea 100644 --- a/crates/tui/src/tools/file/tests/tools.rs +++ b/crates/tui/src/tools/file/tests/tools.rs @@ -885,6 +885,69 @@ async fn edit_file_tool_preserves_executable_bits() { ); } +/// #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..840435dd70 100644 --- a/crates/tui/src/tools/fim.rs +++ b/crates/tui/src/tools/fim.rs @@ -166,6 +166,7 @@ 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)?; 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..d17fb4a962 100644 --- a/crates/tui/src/tools/mod.rs +++ b/crates/tui/src/tools/mod.rs @@ -68,6 +68,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/syntax_check.rs b/crates/tui/src/tools/syntax_check.rs new file mode 100644 index 0000000000..a6b9ba8e2e --- /dev/null +++ b/crates/tui/src/tools/syntax_check.rs @@ -0,0 +1,235 @@ +//! Post-edit syntax gate for the file-mutating tools (#6204). +//! +//! 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). +//! +//! # 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. +//! - **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, +} + +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), + _ => None, + } + } + + fn label(self) -> &'static str { + match self { + Self::Rust => "Rust", + } + } +} + +/// 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), + } +} + +/// 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(), + }) +} + +#[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 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"); + } +} From 9f047aa649093c2a19a8631e8ec2c212cd7b818c Mon Sep 17 00:00:00 2001 From: CodeWhale Bot Date: Tue, 15 Sep 2026 14:00:19 -0700 Subject: [PATCH 2/4] feat(tools): parse-gate TOML and JSON edits before the write lands MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A malformed `Cargo.toml` fails the entire workspace build, not one file, and until now the model only found out on the next `cargo` run — several turns and one confusing error message later. Agents edit manifests constantly (version bumps, feature flags, workspace members), so this is the highest-value file class on the edit path. Extends the `syntax_check` seam from #6204 with two parsers this crate already loads its own config with: `toml_edit::DocumentMut` for `.toml` and `serde_json` for `.json`. No new dependency, no lockfile change. Every call site was wired in the previous commit, so the whole change is the extension plus its tests. `.toml` covers `Cargo.toml`, `deny.toml`, `.cargo/config.toml` and every ordinary `*.toml` uniformly — the extension is the whole rule. `.jsonc`, `.json5`, and `.jsonl` are deliberately *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) does not parse before the edit either, so the before/after rule skips it instead of making it uneditable — covered by a test. Rollback question from the issue, answered: there is nothing to roll back. The check runs before the write at every call site, so a refused edit leaves the file byte-for-byte untouched. Evidence (the four new location-reporting tests were confirmed FAILED with the `.toml`/`.json` arms removed, then restored): cargo fmt --all -- --check clean cargo clippy -p codewhale-tui --all-targets --all-features --locked -- -D warnings ... clean cargo test -p codewhale-tui --lib --all-features --locked -- tools::syntax_check:: tools::file::tests::edit_file tools::apply_patch::tests:: test result: ok. 81 passed; 0 failed; 0 ignored; 12675 filtered out Closes #6206 Co-Authored-By: Claude Opus 5 (1M context) --- crates/tui/src/tools/file/tests/tools.rs | 60 ++++++++++ crates/tui/src/tools/syntax_check.rs | 143 ++++++++++++++++++++++- 2 files changed, 200 insertions(+), 3 deletions(-) diff --git a/crates/tui/src/tools/file/tests/tools.rs b/crates/tui/src/tools/file/tests/tools.rs index 7204aa30ea..cb52f270c2 100644 --- a/crates/tui/src/tools/file/tests/tools.rs +++ b/crates/tui/src/tools/file/tests/tools.rs @@ -885,6 +885,66 @@ async fn edit_file_tool_preserves_executable_bits() { ); } +/// #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] diff --git a/crates/tui/src/tools/syntax_check.rs b/crates/tui/src/tools/syntax_check.rs index a6b9ba8e2e..0fb99f66e4 100644 --- a/crates/tui/src/tools/syntax_check.rs +++ b/crates/tui/src/tools/syntax_check.rs @@ -1,4 +1,4 @@ -//! Post-edit syntax gate for the file-mutating tools (#6204). +//! 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 @@ -9,7 +9,11 @@ //! //! 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). +//! 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 //! @@ -30,7 +34,12 @@ //! 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. +//! 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. @@ -51,6 +60,8 @@ const MAX_CHECKED_BYTES: usize = 2 * 1024 * 1024; #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub(super) enum SyntaxLanguage { Rust, + Toml, + Json, } impl SyntaxLanguage { @@ -59,6 +70,10 @@ impl SyntaxLanguage { 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, } } @@ -66,6 +81,8 @@ impl SyntaxLanguage { fn label(self) -> &'static str { match self { Self::Rust => "Rust", + Self::Toml => "TOML", + Self::Json => "JSON", } } } @@ -104,6 +121,8 @@ pub(super) fn syntax_check(path: &Path, source: &str) -> Option { } match SyntaxLanguage::from_path(path)? { SyntaxLanguage::Rust => check_rust(source), + SyntaxLanguage::Toml => check_toml(source), + SyntaxLanguage::Json => check_json(source), } } @@ -152,6 +171,55 @@ fn check_rust(source: &str) -> Option { }) } +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::*; @@ -177,6 +245,75 @@ mod tests { 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()); From d14b275e0a2e58afefb019a80cd34feaa6e795b4 Mon Sep 17 00:00:00 2001 From: CodeWhale Bot Date: Tue, 15 Sep 2026 14:14:43 -0700 Subject: [PATCH 3/4] feat(tools): normalize edited Rust with rustfmt so anchors stay stable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Model-generated edits rarely match rustfmt output. Once an unformatted edit lands, the next turn's `old_string` or patch context was written against text `cargo fmt` is about to move, so the follow-up edit fails to match and costs a re-read. Normalizing at edit time keeps anchors stable for the rest of the session. `crates/tui/src/tools/rust_format.rs` shells to `rustfmt`, deliberately not `prettyplease`: `cargo fmt --check` is this repository's real gate, and a second formatter with its own opinions would produce files that pass the edit path and fail the gate. `prettyplease` also pins syn 2.x, which would duplicate the syn 3.0.3 adopted in #6204 (#6151). Shelling out additionally honors the project's own rustfmt.toml. Policy: normalize the whole edited file, and only when that file was already rustfmt-clean before the edit. A hand-formatted file is never rewritten. Because a clean file is a formatting fixpoint, reformatting after an edit can only touch the edited region — "whole file" and "edited region" coincide without span arithmetic to get wrong. Common case costs one rustfmt run, not two: if the post-edit content is already canonical there is nothing to check. Runs after the #6204 syntax gate and before the write, at the same call sites, so the returned content and diff are the bytes on disk. `fim_edit` takes the gate but not the normalization: its result reports byte offsets into the written file, and reformatting would move them. Every failure path — missing rustfmt, parse failure, timeout, non-zero exit, CRLF file, oversized file — skips normalization and lets the edit land. Nothing here can fail an edit. One bug worth naming, since the tests initially hid it: `--config-path` pointed at a directory with no rustfmt.toml makes rustfmt exit 1, which silently disabled normalization everywhere while a skip-if-unavailable test branch stayed green. The flag is now passed only when a config file is actually found, and the tests assert rustfmt is present instead of skipping. Evidence (the three normalization tests were confirmed FAILED with `normalize_edit` stubbed to `None`, then restored): cargo fmt --all -- --check clean cargo clippy -p codewhale-tui --all-targets --all-features --locked -- -D warnings ... clean cargo test -p codewhale-tui --lib --all-features --locked -- tools::rust_format:: tools::syntax_check:: tools::file::tests::edit_file tools::file::tests::write_ tools::apply_patch::tests:: test result: ok. 97 passed; 0 failed; 0 ignored; 12668 filtered out Closes #6205 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01AJENKJ2smviQW4FVGzUTk9 --- crates/tui/src/tools/apply_patch.rs | 51 ++++- crates/tui/src/tools/file.rs | 37 +++- crates/tui/src/tools/file/tests/tools.rs | 74 +++++++ crates/tui/src/tools/fim.rs | 4 + crates/tui/src/tools/mod.rs | 1 + crates/tui/src/tools/rust_format.rs | 238 +++++++++++++++++++++++ 6 files changed, 399 insertions(+), 6 deletions(-) create mode 100644 crates/tui/src/tools/rust_format.rs diff --git a/crates/tui/src/tools/apply_patch.rs b/crates/tui/src/tools/apply_patch.rs index da7caade14..eee97a448f 100644 --- a/crates/tui/src/tools/apply_patch.rs +++ b/crates/tui/src/tools/apply_patch.rs @@ -16,6 +16,7 @@ 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, @@ -412,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(); @@ -458,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 @@ -1292,6 +1296,21 @@ 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 @@ -1653,6 +1672,34 @@ 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] diff --git a/crates/tui/src/tools/file.rs b/crates/tui/src/tools/file.rs index aa571b44ba..6ff9cd1339 100644 --- a/crates/tui/src/tools/file.rs +++ b/crates/tui/src/tools/file.rs @@ -7,6 +7,7 @@ //! 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, @@ -1444,13 +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())) })?; @@ -1568,7 +1574,7 @@ 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, @@ -1576,6 +1582,11 @@ impl ToolSpec for WriteFileTool { 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)) @@ -2017,8 +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| { @@ -2261,6 +2275,16 @@ 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)) })?; @@ -2296,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 cb52f270c2..4d1ab1d58f 100644 --- a/crates/tui/src/tools/file/tests/tools.rs +++ b/crates/tui/src/tools/file/tests/tools.rs @@ -885,6 +885,80 @@ 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] diff --git a/crates/tui/src/tools/fim.rs b/crates/tui/src/tools/fim.rs index 840435dd70..cdab60f367 100644 --- a/crates/tui/src/tools/fim.rs +++ b/crates/tui/src/tools/fim.rs @@ -167,6 +167,10 @@ impl ToolSpec for FimEditTool { 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 d17fb4a962..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; 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() + ); + } +} From d184a8bb179595f9cbc34454d8db5b4643ab2e72 Mon Sep 17 00:00:00 2001 From: CodeWhale Bot Date: Tue, 15 Sep 2026 14:43:00 -0700 Subject: [PATCH 4/4] docs(changelog): receipt the edit-safety gates (#6204, #6206, #6205) The three feat commits referenced issues without the changelog receipt the working loop requires. Regenerated with scripts/sync-changelog.sh and web/scripts/derive-changelog.mjs. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01AJENKJ2smviQW4FVGzUTk9 --- CHANGELOG.md | 10 ++++++++++ crates/tui/CHANGELOG.md | 10 ++++++++++ web/lib/changelog.generated.ts | 4 +++- 3 files changed, 23 insertions(+), 1 deletion(-) 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/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/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",