Skip to content

Edit safety: parse-gate file edits before the write lands #6238

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Hmbown merged 4 commits into from
Sep 15, 2026
Merged

Edit safety: parse-gate file edits before the write lands #6238

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
98c123c
feat(tools): reject edits that break Rust syntax, with syn line:column
Sep 15, 2026
9f047aa
feat(tools): parse-gate TOML and JSON edits before the write lands
Sep 15, 2026
d14b275
feat(tools): normalize edited Rust with rustfmt so anchors stay stable
Sep 15, 2026
d184a8b
docs(changelog): receipt the edit-safety gates (#6204, #6206, #6205)
Sep 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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`
Expand Down
2 changes: 2 additions & 0 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 10 additions & 0 deletions crates/tui/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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`
Expand Down
7 changes: 7 additions & 0 deletions crates/tui/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
87 changes: 85 additions & 2 deletions crates/tui/src/tools/apply_patch.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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<PathBuf> = pending.iter().map(|p| p.path.clone()).collect();
Expand Down Expand Up @@ -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<PathBuf> = pending
Expand Down Expand Up @@ -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> {

@codewhale-agent codewhale-agent Bot Sep 15, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[WARNING] apply_patch's transactional claim is asserted but only tested with a single file

The guard loop in apply_pending_writes is deliberately placed ahead of the write loop so that "one unparseable result must leave every file in the patch untouched rather than half-applied", and the CHANGELOG repeats that apply_patch "cannot half-apply". The only test (patch_refuses_a_hunk_that_breaks_rust_syntax) uses a single file, so it cannot distinguish "guard before the first write" from "per-file guard just before each write" — the two behave identically for one file, and only the second violates the stated contract. A two-file patch whose second file breaks Rust syntax, asserting the first file's bytes are unchanged on disk, is what actually pins the placement; it would also fail today if anyone later moves the loop into the write loop.

// 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 {
Expand Down Expand Up @@ -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() {

@codewhale-agent codewhale-agent Bot Sep 15, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[WARNING] The 'patch cannot half-apply' transactional claim is not actually tested

The doc comment on patch_refuses_a_hunk_that_breaks_rust_syntax says a multi-file patch cannot land half-applied, but the test patches a single file. It therefore cannot distinguish a gate that runs once ahead of the first write (the claim) from a per-file gate, from a gate placed after the first write, or from no gate at all on a second entry. Add a two-file patch whose second file's result is unparseable and assert that both files are byte-identical to their pre-patch contents — that is the only shape that exercises the ordering inside apply_pending_writes.

@codewhale-agent codewhale-agent Bot Sep 15, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extend the refusal test to two files in one patch, with only the second file's post-patch content unparseable, and assert both files are unchanged. That is what actually verifies the 'gate ahead of the first write' ordering inside apply_pending_writes.

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();
Expand Down
54 changes: 50 additions & 4 deletions crates/tui/src/tools/file.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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(

@codewhale-agent codewhale-agent Bot Sep 15, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[WARNING] Both write_file sites gained the gate and normalization with no tests; the apply_patch replace path is untested too

WriteFileTool's small-contract path and its ToolSpec twin are the two sites most likely to be hit by an agent that rewrites a whole file, and neither has a test asserting that (a) a write that breaks a previously-parseable .rs/.toml/.json file is refused with the file untouched, or (b) a full-file write into an already-rustfmt-clean file is normalized. Likewise build_pending_writes_from_replace now feeds normalize_pending_rust and the same guard as the patch path, but every new test exercises file_patches only. Making it worse, the new-file fail-open leg (the contract's other half, "creating a new file is ungated") is only covered at the guard_edit unit level, never through a tool — so a change that started passing Some("") instead of None for created files would silently start gating and rustfmt-normalizing every brand-new Rust file with the suite still green.

@codewhale-agent codewhale-agent Bot Sep 15, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[INFO] File action="write" and fim_edit gate/normalize paths have no test coverage

The PR states the check runs at every write site, but only the edit path (file.rs main + contract), one single-file patch, and the guard unit tests are covered. The write tool's two call sites use a different pre-edit source (existed_before.then(...) + preserve_prior_line_endings) and the fim_edit guard is a distinct call with no test at all. Add at least: (a) File action="write" over an existing parseable .rs/.toml that becomes unparseable is refused and leaves the file untouched; (b) the same over a new file is allowed (creation fails open); (c) fim_edit refuses a generated block that breaks Rust syntax. These are the paths most likely to regress silently.

@codewhale-agent codewhale-agent Bot Sep 15, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add coverage for the write write site (refuse a write that breaks an existing parseable file; allow a write that creates a new file) and for fim_edit's guard, since those are the two gate call sites with no test in this PR.

&file_path,
path_str,
existed_before.then(|| prior_contents.as_ref()),
&written,
)?;
if existed_before

@codewhale-agent codewhale-agent Bot Sep 15, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[INFO] write_file normalizes silently, unlike the edit path

EditFileTool appends NORMALIZED_NOTE so the model learns that the bytes on disk are not the bytes it sent — the entire point of #6205. The two WriteFileTool sites apply the same normalization with no note and no equivalent signal. Worth confirming that the write path's returned diff/metadata is built from the normalized written (it looks like it is, since written is reassigned before write_atomic_workspace); if it is, this is only a reporting inconsistency. If it is not, the model's view of the file diverges from disk in exactly the way this PR exists to prevent.

&& 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()))
})?;
Expand Down Expand Up @@ -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))
Expand Down Expand Up @@ -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| {
Expand Down Expand Up @@ -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))
})?;
Expand Down Expand Up @@ -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 {
Expand Down
Loading
Loading