Edit safety: parse-gate file edits before the write lands - #6238
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Claude finished @Hmbown's task in 2m 40s —— View job Review: Edit safety — parse-gate file edits before the write landsTodo list
SummaryRead the diff against
No correctness bugs or provider/model-routing concerns found (this PR doesn't touch routing at all). Two lower-severity notes below. 1. Test coverage gap: two of the four write sites are ungated by any new test
2. Minor duplication across the four gate call sitesThe Nothing else stood out. Dependency reuse ( |
![codewhale-agent[bot]](https://avatars.githubusercontent.com/u/101357273?s=60&v=4){kind=small}
There was a problem hiding this comment.
Codewhale review
Complete review coverage: 1/1 passes, 12 file patches, sha256:ccc9407614addccb83dcf4b486e0fbe887d2fd41b63abcb79846e5e72ff0a9c2.
Pass 1: The PR adds a post-edit parse gate (syn/toml_edit/serde_json) plus a rustfmt-based normalization step, wired into four sites in file.rs, one shared site in apply_patch.rs, and fim.rs, with two new modules and regression tests. The design is sound and the gate placement (ahead of the first write, before any rollback would be needed) is correct; the before/after fail-open rule is implemented faithfully and the parser choices are defensible. The gaps are mainly evidential: the transactional claim is only tested with one file, the write_file sites and the apply_patch replace path have no test coverage at all, and the --config-path branch the author describes as previously breaking silently is still never exercised. One real contract violation exists: guard_edit cannot distinguish "parsed before" from "declined to check because the file is over MAX_CHECKED_BYTES".
Findings
- [WARNING] apply_patch's transactional claim is asserted but only tested with a single file (
crates/tui/src/tools/apply_patch.rs:1314)
The guard loop inapply_pending_writesis 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 thatapply_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. - [WARNING] Both write_file sites gained the gate and normalization with no tests; the apply_patch replace path is untested too (
crates/tui/src/tools/file.rs:1449)
WriteFileTool's small-contract path and itsToolSpectwin 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/.jsonfile is refused with the file untouched, or (b) a full-file write into an already-rustfmt-clean file is normalized. Likewisebuild_pending_writes_from_replacenow feedsnormalize_pending_rustand the same guard as the patch path, but every new test exercisesfile_patchesonly. Making it worse, the new-file fail-open leg (the contract's other half, "creating a new file is ungated") is only covered at theguard_editunit level, never through a tool — so a change that started passingSome("")instead ofNonefor created files would silently start gating and rustfmt-normalizing every brand-new Rust file with the suite still green. - [WARNING] The rustfmt
--config-pathbranch — the exact failure the PR documents — is still unexercised by any test (crates/tui/src/tools/rust_format.rs:120)
nearest_configis the branch whose previous form (a directory instead of a config file) made rustfmt exit 1 with "unable to find a config file" and silently disabled normalization everywhere while the suite stayed green. Every new test passes a path with norustfmt.toml/.rustfmt.tomlin an ancestor —rust_format::testsuses the relativesrc/lib.rs, and the tool tests use fresh tempdirs — sonearest_configreturnsNonein all of them and--config-pathis never passed. That leaves the known-broken path untested and also means nothing notices arustfmt.tomlwhose contents make rustfmt exit non-zero (e.g. an unstable option under a stable toolchain): normalization would be silently off repo-wide, which is precisely the hazard the tests were supposed to close. A test that writes arustfmt.tomlinto a tempdir and asserts the edit is still normalized (and that a bogus sibling config does not disable it) would cover both. - [WARNING]
guard_edittreats an uncheckedbefore(overMAX_CHECKED_BYTES) as if it had parsed (crates/tui/src/tools/syntax_check.rs:149)
syntax_checkreturnsNonefor two different situations: "this content parses" and "I declined to look (unsupported extension orsource.len() > MAX_CHECKED_BYTES)".guard_editusessyntax_check(path, before).is_some()as the verdict for "the file was broken before", so a pre-edit file larger than 2 MiB is treated as parseable. Concretely, an edit that shrinks a >2 MiB broken.rsfile to under 2 MiB and leaves it broken is refused with "it would leave X unparseable" — the file never parsed before as far as we know, and the documented contract says the gate exists to catch the edit that introduces breakage, never to strand a model repairing one. The size check is the only asymmetric input between the two calls (the extension is identical), so an explicit pre-edit size comparison restores the contract. - [INFO]
write_filenormalizes silently, unlike the edit path (crates/tui/src/tools/file.rs:1455)
EditFileToolappendsNORMALIZED_NOTEso the model learns that the bytes on disk are not the bytes it sent — the entire point of #6205. The twoWriteFileToolsites 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 normalizedwritten(it looks like it is, sincewrittenis reassigned beforewrite_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. - [INFO] Every Rust edit costs one or two rustfmt subprocess spawns, serially per file in a patch (
crates/tui/src/tools/rust_format.rs:76)
normalize_editruns rustfmt onafterfirst and only then onbefore(the ordering is deliberate and correctly documented), so a.rsedit whose result is already canonical still pays one process spawn, and a sloppy edit into a clean file pays two, each with a 5 s budget.normalize_pending_rustiteratespendingsequentially, so a patch touching N Rust files can add N x up to 10 s of blocking latency to the interactive edit path before the first byte is written. Bounding this (skip when the diff is empty, cap the number of formatted files per patch, or run the files concurrently under an overall budget) may be worth considering; the current guards only bound size and time, not count. - [INFO]
line_columncan panic on a byte offset that is not a char boundary (crates/tui/src/tools/syntax_check.rs:212)
&source[..offset]panics ifoffsetsplits a multi-byte character;offset.min(source.len())only guards the upper bound.toml_editspans should always land on token boundaries, so this is likely unreachable today, but a panic inside the edit path is a much worse failure than the refusal this function exists to produce, and the caller already has aNone-means-no-objection convention it could use instead. Cheap defensiveness: fall back to(1, 1)whensource.get(..offset)isNone, or floor the offset to the nearest char boundary before slicing. - [INFO] The generated web changelog truncates the new entry mid-sentence and drops its issue references (
web/lib/changelog.generated.ts:33)
The first new item renders on the public changelog as "...the file untouched and apply_patch…", cutting "cannot half-apply (#6204, #6206)" — so the user-facing page loses both the closing clause and the issue links that the other entries carry. The second new entry is short enough to survive intact, which suggests a generator length cap rather than a bug; shortening the first bullet (or splitting it) would keep the generated page readable.
Suggestions
-
crates/tui/src/tools/syntax_check.rs:149—syntax_checkreports "no objection" both for content that parsed and for content it skipped because it exceedsMAX_CHECKED_BYTES. Using it as thebeforeverdict therefore makes the gate refuse edits to files it never parsed, contradicting the documented fail-open rule (an edit that shrinks a >2 MiB broken file below the cap would be refused). Compare the pre-edit size explicitly so an unchecked file always fails open.if before.len() > MAX_CHECKED_BYTES || syntax_check(path, before).is_some() {
Assessment
Pass 1: The core design is right and well argued: gate immediately before the write, refuse only newly introduced breakage, reuse the parsers already in the tree, and prefer rustfmt over a second formatter so edit-time output matches cargo fmt --check. The implementation matches the prose at every call site I can see, and the guard-before-first-write placement in apply_patch is the correct way to make a patch transactional for this failure mode. What is missing is evidence and one edge case: the multi-file atomicity claim is untested, the write_file sites and the apply_patch replace path have no test at all (nor does new-file fail-open through a tool), the --config-path branch that already burned this PR once is still exercised by nothing, and guard_edit conflates "parsed before" with "not checked because oversized", which refuses a repair the contract promises never to block. Address the size asymmetry and add the multi-file and rustfmt.toml tests and this is a solid, well-scoped gate.
Advisory review by Codewhale (codewhale review --pr 6238 --post, head 674bb880dafcebac192ada0d025b39dfdf63f66d). Line-specific findings are also posted as inline review comments; mechanical fixes arrive as committable suggestions you can apply from the Files tab. CODEOWNERS approval still governs merge.
| } | ||
| } | ||
|
|
||
| fn apply_pending_writes(pending: &[PendingWrite]) -> Result<(), ToolError> { |
There was a problem hiding this comment.
[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.
| // 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( |
There was a problem hiding this comment.
[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.
| // 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 { |
There was a problem hiding this comment.
[WARNING] The rustfmt --config-path branch — the exact failure the PR documents — is still unexercised by any test
nearest_config is the branch whose previous form (a directory instead of a config file) made rustfmt exit 1 with "unable to find a config file" and silently disabled normalization everywhere while the suite stayed green. Every new test passes a path with no rustfmt.toml/.rustfmt.toml in an ancestor — rust_format::tests uses the relative src/lib.rs, and the tool tests use fresh tempdirs — so nearest_config returns None in all of them and --config-path is never passed. That leaves the known-broken path untested and also means nothing notices a rustfmt.toml whose contents make rustfmt exit non-zero (e.g. an unstable option under a stable toolchain): normalization would be silently off repo-wide, which is precisely the hazard the tests were supposed to close. A test that writes a rustfmt.toml into a tempdir and asserts the edit is still normalized (and that a bogus sibling config does not disable it) would cover both.
| let Some(before) = before else { | ||
| return Ok(()); | ||
| }; | ||
| if syntax_check(path, before).is_some() { |
There was a problem hiding this comment.
[WARNING] guard_edit treats an unchecked before (over MAX_CHECKED_BYTES) as if it had parsed
syntax_check returns None for two different situations: "this content parses" and "I declined to look (unsupported extension or source.len() > MAX_CHECKED_BYTES)". guard_edit uses syntax_check(path, before).is_some() as the verdict for "the file was broken before", so a pre-edit file larger than 2 MiB is treated as parseable. Concretely, an edit that shrinks a >2 MiB broken .rs file to under 2 MiB and leaves it broken is refused with "it would leave X unparseable" — the file never parsed before as far as we know, and the documented contract says the gate exists to catch the edit that introduces breakage, never to strand a model repairing one. The size check is the only asymmetric input between the two calls (the extension is identical), so an explicit pre-edit size comparison restores the contract.
| existed_before.then(|| prior_contents.as_ref()), | ||
| &written, | ||
| )?; | ||
| if existed_before |
There was a problem hiding this comment.
[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.
| return None; | ||
| } | ||
|
|
||
| let formatted = rustfmt(path, after).await?; |
There was a problem hiding this comment.
[INFO] Every Rust edit costs one or two rustfmt subprocess spawns, serially per file in a patch
normalize_edit runs rustfmt on after first and only then on before (the ordering is deliberate and correctly documented), so a .rs edit whose result is already canonical still pays one process spawn, and a sloppy edit into a clean file pays two, each with a 5 s budget. normalize_pending_rust iterates pending sequentially, so a patch touching N Rust files can add N x up to 10 s of blocking latency to the interactive edit path before the first byte is written. Bounding this (skip when the diff is empty, cap the number of formatted files per patch, or run the files concurrently under an overall budget) may be worth considering; the current guards only bound size and time, not count.
| /// 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]; |
There was a problem hiding this comment.
[INFO] line_column can panic on a byte offset that is not a char boundary
&source[..offset] panics if offset splits a multi-byte character; offset.min(source.len()) only guards the upper bound. toml_edit spans should always land on token boundaries, so this is likely unreachable today, but a panic inside the edit path is a much worse failure than the refusal this function exists to produce, and the caller already has a None-means-no-objection convention it could use instead. Cheap defensiveness: fall back to (1, 1) when source.get(..offset) is None, or floor the offset to the nearest char boundary before slicing.
| { | ||
| "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…", |
There was a problem hiding this comment.
[INFO] The generated web changelog truncates the new entry mid-sentence and drops its issue references
The first new item renders on the public changelog as "...the file untouched and apply_patch…", cutting "cannot half-apply (#6204, #6206)" — so the user-facing page loses both the closing clause and the issue links that the other entries carry. The second new entry is short enough to survive intact, which suggests a generator length cap rather than a bug; shortening the first bullet (or splitting it) would keep the generated page readable.
| let Some(before) = before else { | ||
| return Ok(()); | ||
| }; | ||
| if syntax_check(path, before).is_some() { |
There was a problem hiding this comment.
syntax_check reports "no objection" both for content that parsed and for content it skipped because it exceeds MAX_CHECKED_BYTES. Using it as the before verdict therefore makes the gate refuse edits to files it never parsed, contradicting the documented fail-open rule (an edit that shrinks a >2 MiB broken file below the cap would be refused). Compare the pre-edit size explicitly so an unchecked file always fails open.
| if syntax_check(path, before).is_some() { | |
| if before.len() > MAX_CHECKED_BYTES || syntax_check(path, before).is_some() { |
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AJENKJ2smviQW4FVGzUTk9
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AJENKJ2smviQW4FVGzUTk9
674bb88 to
d184a8b
Compare
|
Claude finished @Hmbown's task in 1m 54s —— View job Review: Edit safety — parse-gate file edits before the write landsTodo
Re-reviewed the current head ( 1. Correctness bug, still unfixed:
|
There was a problem hiding this comment.
Codewhale review
Complete review coverage: 1/1 passes, 12 file patches, sha256:2f5e8511d519eca32dd452eaa7fcbdf60820ecff0b1062d027c206ed9639bca0.
Pass 1: The change adds a pre-write syntax gate (syn::parse_file / toml_edit / serde_json) plus rustfmt-based normalization to every file-mutating tool. The core contract — reject only when the file parsed before and does not parse after — is implemented correctly in guard_edit, the write sites are ordered gate-before-write (and all guards precede the first write in apply_patch), and the unit tests for the guard are strong. The remaining concerns are a parser-offset slicing hazard, an environment-sensitive test that can pass for the wrong reason, and several untested write paths / claims.
Findings
- [WARNING]
line_columnslices a parser-supplied byte offset without a char-boundary check (crates/tui/src/tools/syntax_check.rs:212)
toml_edit'sTomlError::span()is a raw byte range;line_columnimmediately does&source[..offset]. If that offset is not a UTF-8 char boundary (an error position inside or adjacent to a multi-byte character), the slice panics instead of producing the recoverableToolErrorthe gate is designed to return — a panic on the interactive edit path, in the very path that is supposed to fail soft. Clamp to a boundary first (walk back while!source.is_char_boundary(offset), or usesource.get(..offset).unwrap_or(source)) before slicing. - [WARNING] Normalization test in the tool suite can pass vacuously when
rustfmtis absent (crates/tui/src/tools/file/tests/tools.rs:935)
rust_format::tests::require_rustfmtasserts the formatter is present specifically so a missingrustfmtcannot make the formatting tests green for the wrong reason. The tool-level testedit_file_leaves_a_hand_formatted_file_alonehas no such precondition, and its assertion ("unrelated user formatting must survive the edit") is exactly what a missing/stubbed formatter also produces, so the test proves nothing about the skip-on-dirty-file policy when rustfmt is not on PATH. Add a shared precondition (call arequire_rustfmt-style helper, or assert that an explicitly sloppy edit becomes normalized) so the policy assertion cannot be satisfied by the formatter being gone. - [WARNING] The 'patch cannot half-apply' transactional claim is not actually tested (
crates/tui/src/tools/apply_patch.rs:1706)
The doc comment onpatch_refuses_a_hunk_that_breaks_rust_syntaxsays 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 insideapply_pending_writes. - [INFO]
File action="write"andfim_editgate/normalize paths have no test coverage (crates/tui/src/tools/file.rs:1449)
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. Thewritetool's two call sites use a different pre-edit source (existed_before.then(...)+preserve_prior_line_endings) and thefim_editguard is a distinct call with no test at all. Add at least: (a)File action="write"over an existing parseable.rs/.tomlthat becomes unparseable is refused and leaves the file untouched; (b) the same over a new file is allowed (creation fails open); (c)fim_editrefuses a generated block that breaks Rust syntax. These are the paths most likely to regress silently. - [INFO] Normalization's extension test disagrees with the gate's on case (
crates/tui/src/tools/rust_format.rs:65)
SyntaxLanguage::from_pathlowercases the extension before matching, whilenormalize_editcomparespath.extension()?.to_str()? != "rs"case-sensitively. A file namedLIB.RSis therefore syntax-gated but never normalized, so the two halves of the same feature disagree about what a Rust file is. Useeq_ignore_ascii_case("rs")(or reuseSyntaxLanguage::from_path) so the gate and the formatter agree. - [INFO] JSON gate materializes a whole
Valuetree just to validate (crates/tui/src/tools/syntax_check.rs:188)
serde_json::from_str::<serde_json::Value>(source)allocates the entire document (up toMAX_CHECKED_BYTES) on the interactive edit path, twice per rejected edit, purely to learn whether it parses; the parsed value is discarded. Deserializing intoserde::de::IgnoredAnyvalidates the same grammar without building the tree, and keepserror.line()/column()available. Low priority, but it is pure cost on the hot path this gate sits on.
Suggestions
crates/tui/src/tools/syntax_check.rs:212— Guard the slice against a non-char-boundary offset before taking&source[..offset], e.g. walk the offset back while!source.is_char_boundary(offset)(orsource.get(..offset).unwrap_or(source)), so a span reported inside a multi-byte character degrades to a slightly-off column instead of panicking the edit path. No replacement is offered because the exact anchor line was inferred from the diff.crates/tui/src/tools/file/tests/tools.rs:935— Give the formatting-sensitive tool tests the same precondition the rust_format unit tests use (assertrustfmtis on PATH and produces expected output) soedit_file_leaves_a_hand_formatted_file_alonecannot pass because the formatter is missing rather than because the policy correctly skipped a hand-formatted file.crates/tui/src/tools/apply_patch.rs:1706— 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 insideapply_pending_writes.crates/tui/src/tools/file.rs:1449— Add coverage for thewritewrite site (refuse a write that breaks an existing parseable file; allow a write that creates a new file) and forfim_edit's guard, since those are the two gate call sites with no test in this PR.crates/tui/src/tools/rust_format.rs:65— Match the extension case-insensitively (or delegate toSyntaxLanguage::from_path) so a.RSfile is normalized whenever it is gated.crates/tui/src/tools/syntax_check.rs:188— Validate JSON withserde::de::IgnoredAnyinstead ofserde_json::Valueto avoid allocating the whole document on the edit path while keepingerror.line()/error.column().
Assessment
Pass 1: The gate itself is sound: the before/after asymmetry (fail open on pre-existing breakage and on file creation) is implemented exactly as documented, every write site checks before writing, apply_patch guards all pending writes ahead of the first write, and the guard unit tests plus the edit/patch regression tests are meaningful and were verified to fail without the fix. No correctness defect was found in the gate or the normalization logic — the ordering of normalize_edit (whole-file, only for files that were already rustfmt-clean, CRLF and oversized files skipped, every failure path returning None) is consistent with the stated policy. The remaining work is robustness and evidence: guard the toml_edit byte offset before slicing it, make the formatting-sensitive tool tests assert rustfmt is present so they cannot pass vacuously, test the transactional multi-file claim rather than a single-file patch, and cover the two untested write sites (File action="write", fim_edit). The dependency story is consistent — syn/proc-macro2 reuse the existing lockfile entries (two added lines) and TOML/JSON reuse existing crates.
Advisory review by Codewhale (codewhale review --pr 6238 --post, head d184a8bb179595f9cbc34454d8db5b4643ab2e72). Line-specific findings are also posted as inline review comments; mechanical fixes arrive as committable suggestions you can apply from the Files tab. CODEOWNERS approval still governs merge.
| /// 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]; |
There was a problem hiding this comment.
[WARNING] line_column slices a parser-supplied byte offset without a char-boundary check
toml_edit's TomlError::span() is a raw byte range; line_column immediately does &source[..offset]. If that offset is not a UTF-8 char boundary (an error position inside or adjacent to a multi-byte character), the slice panics instead of producing the recoverable ToolError the gate is designed to return — a panic on the interactive edit path, in the very path that is supposed to fail soft. Clamp to a boundary first (walk back while !source.is_char_boundary(offset), or use source.get(..offset).unwrap_or(source)) before slicing.
|
|
||
| /// A file the author formats by hand is never reformatted wholesale. | ||
| #[tokio::test] | ||
| async fn edit_file_leaves_a_hand_formatted_file_alone() { |
There was a problem hiding this comment.
[WARNING] Normalization test in the tool suite can pass vacuously when rustfmt is absent
rust_format::tests::require_rustfmt asserts the formatter is present specifically so a missing rustfmt cannot make the formatting tests green for the wrong reason. The tool-level test edit_file_leaves_a_hand_formatted_file_alone has no such precondition, and its assertion ("unrelated user formatting must survive the edit") is exactly what a missing/stubbed formatter also produces, so the test proves nothing about the skip-on-dirty-file policy when rustfmt is not on PATH. Add a shared precondition (call a require_rustfmt-style helper, or assert that an explicitly sloppy edit becomes normalized) so the policy assertion cannot be satisfied by the formatter being gone.
| /// #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() { |
There was a problem hiding this comment.
[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.
| // 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( |
There was a problem hiding this comment.
[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.
| /// `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<String> { | ||
| if path.extension()?.to_str()? != "rs" { |
There was a problem hiding this comment.
[INFO] Normalization's extension test disagrees with the gate's on case
SyntaxLanguage::from_path lowercases the extension before matching, while normalize_edit compares path.extension()?.to_str()? != "rs" case-sensitively. A file named LIB.RS is therefore syntax-gated but never normalized, so the two halves of the same feature disagree about what a Rust file is. Use eq_ignore_ascii_case("rs") (or reuse SyntaxLanguage::from_path) so the gate and the formatter agree.
|
|
||
| /// A file the author formats by hand is never reformatted wholesale. | ||
| #[tokio::test] | ||
| async fn edit_file_leaves_a_hand_formatted_file_alone() { |
There was a problem hiding this comment.
Give the formatting-sensitive tool tests the same precondition the rust_format unit tests use (assert rustfmt is on PATH and produces expected output) so edit_file_leaves_a_hand_formatted_file_alone cannot pass because the formatter is missing rather than because the policy correctly skipped a hand-formatted file.
| /// #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() { |
There was a problem hiding this comment.
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.
| // 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( |
There was a problem hiding this comment.
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.
| /// `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<String> { | ||
| if path.extension()?.to_str()? != "rs" { |
There was a problem hiding this comment.
Match the extension case-insensitively (or delegate to SyntaxLanguage::from_path) so a .RS file is normalized whenever it is gated.
| } | ||
|
|
||
| fn check_json(source: &str) -> Option<SyntaxIssue> { | ||
| let error = serde_json::from_str::<serde_json::Value>(source).err()?; |
There was a problem hiding this comment.
Validate JSON with serde::de::IgnoredAny instead of serde_json::Value to avoid allocating the whole document on the edit path while keeping error.line()/error.column().
Three issues, one gate seam in the tools edit path.
Closes
syn::parse_filefor grammar-exactline:columnThe contract, which the issues did not specify
An edit is refused only when the file parsed before and would not parse after. Pre-existing breakage and new-file creation fail open. Repairing a broken file is the commonest reason to edit source at all, and a gate that blocked it would be worse than no gate.
The check runs on post-edit content immediately before the write, at every write site:
file.rs(write,edit, and both small-contract twins),apply_patch.rs(once over all pending writes, ahead of the first, so a transactional patch cannot half-apply), andfim.rs. Because it precedes the write, a rejection leaves the file untouched and there is no rollback path — which also answers #6206's item 3..jsonc/.json5/.jsonlare deliberately not treated as JSON. A.jsonfile that is really JSONC does not parse before the edit either, so the before/after rule skips it rather than making it uneditable.Dependencies: reuse, not addition
syn3.0.3 was already in the lockfile as a proc-macro build dependency, so the same version is reused per #6151 —Cargo.lockgains exactly two lines undercodewhale-tuiand no new package version.proc-macro2gainsspan-locations, which is what makesline:columnavailable at all; the parse feature is inert without it. TOML and JSON go throughtoml_edit::DocumentMutandserde_json, both alreadycodewhale-tuidependencies: no new dependency, no lockfile change.#6205 uses rustfmt, not prettyplease
cargo fmt --checkis this repo's real gate. A second formatter would produce files that pass the edit path and then fail CI.prettypleasealso pinssyn2.x, which would duplicate the 3.0.3 just adopted.Policy: normalize the whole file only when it was already rustfmt-clean before the edit, so hand-formatted files are never rewritten. A clean file is a fixpoint, so "whole file" and "edited region" coincide. Every failure path skips and lets the edit land.
fim_edittakes the gate but not the normalization — its result reports byte offsets that reformatting would invalidate.Evidence
Every regression test was confirmed failing without the fix (gate stubbed to
Ok(()), format arms removed,normalize_editstubbed toNone), then restored.Re-verified independently on this head:
One bug worth recording, because it stayed green while being broken:
--config-pathinitially pointed at a directory, andrustfmtexits 1 with "unable to find a config file" when norustfmt.tomlis there. A skip-if-rustfmt-missing branch swallowed that, silently disabling normalization everywhere while the suite passed. Found by re-running with--nocapture. The config path is now only passed when a file is actually found by walking up, and the formatting tests assertrustfmtis present rather than skipping.Pre-existing, not from this branch
check-blocking-calls-budget.pyfails oncrates/tui/src/runtime_api/workspace.rs(verified byte-identical toorigin/main). That is main's current red; it is fixed in #6229.🤖 Generated with Claude Code