fix(cargo-anvil): stop writing an unparsable TOML host when a table is hand-written - #162
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Minor but concrete fixups were identified in the updated code/comments that should be addressed before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes a long-standing cargo-anvil failure mode where introducing a managed TOML region (notably deny.toml’s [advisories]) alongside a hand-written copy of the same table could produce duplicate table headers and an unparsable TOML file. It does so by moving table adoption and validation onto toml_edit’s parsed representation, preserving user-only keys as “residue” inside the managed table, and refusing a region introduction when the spliced result would not parse.
Changes:
- Reworked TOML table adoption to be parser-backed, preserving hand-written-only entries as residue and detecting conflicts on differing values.
- Added a “refuse on unparsable result” backstop for TOML region introductions (scoped to the region, not the full run) and masked-region TOML validation.
- Strengthened fixture assertions to require TOML parseability; added a new
deny-conflictend-to-end fixture and related tests/docs updates.
File summaries
| File | Description |
|---|---|
| crates/cargo-anvil/src/region.rs | Parser-backed table discovery/adoption, residue extraction, and masking helpers for managed regions. |
| crates/cargo-anvil/src/emit/managed_region.rs | Adds toml_introduction_refusal, integrates residue insertion after region splice, and validates spliced TOML with other regions masked. |
| crates/cargo-anvil/src/run.rs | Refuses unsafe TOML introductions early and records a scoped refusal + no-op plan item. |
| crates/cargo-anvil/src/emit/mod.rs | Re-exports toml_introduction_refusal. |
| crates/cargo-anvil/tests/fixtures.rs | Adds read_parsing_toml helper and upgrades fixtures to assert parseability; adds deny-conflict test. |
| crates/cargo-anvil/tests/fixtures/deny-conflict/Justfile | New fixture input ensuring non-TOML artifacts remain unaffected when a region is refused. |
| crates/cargo-anvil/tests/fixtures/deny-conflict/deny.toml | New fixture input exercising managed vs hand-written conflict in [advisories]. |
| crates/cargo-anvil/tests/fixtures/deny-conflict/Cargo.toml | New fixture workspace to drive the end-to-end deny-conflict scenario. |
| crates/cargo-anvil/tests/fixtures/deny-conflict/crates/alpha/Cargo.toml | New fixture crate manifest for the deny-conflict workspace. |
| crates/cargo-anvil/tests/fixtures/deny-conflict/crates/alpha/src/lib.rs | New fixture stub source file. |
| crates/cargo-anvil/docs/design/updates.md | Updates design documentation to reflect parser-backed adoption, residue handling, and refusal behavior. |
| crates/cargo-anvil/Cargo.toml | Adds toml_edit as a dev-dependency for tests. |
Review details
- Files reviewed: 12/12 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ Your project status has failed because the head coverage (97.5%) is below the target coverage (100.0%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #162 +/- ##
======================================
Coverage 97.5% 97.5%
======================================
Files 300 300
Lines 68538 69048 +510
======================================
+ Hits 66876 67382 +506
- Misses 1662 1666 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
45f44d3 to
b7dedf2
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The new refusal diagnostic in refuse_region is misleading for cases where other regions may still write to the same host file, and should be corrected to avoid incorrect user guidance.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 12/12 changed files
- Comments generated: 1
- Review effort level: Lite
b7dedf2 to
cfcb764
Compare
There was a problem hiding this comment.
🟡 Changes recommended
There are a couple of correctness/user-guidance issues in the changed code paths (CRLF gap handling in residue insertion, and misleading opt-out wording in the refusal diagnostic) that should be addressed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 12/12 changed files
- Comments generated: 2
- Review effort level: Lite
cfcb764 to
27064d5
Compare
There was a problem hiding this comment.
🟡 Changes recommended
A correctness issue remains in table_entries for dotted-key tables where multiple nested entries share the same computed start offset, producing empty/overlapping spans and risking residue loss/duplication.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 12/12 changed files
- Comments generated: 1
- Review effort level: Lite
27064d5 to
79653a1
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The current residue newline normalization can introduce mixed LF/CRLF line endings, and there’s a redundant toml_edit dev-dependency entry that should be removed for clarity.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 2
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
User-facing diagnostics use indented \ line continuations that introduce unintended whitespace into refusal/error messages.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
crates/cargo-anvil/src/emit/managed_region.rs:227
- Both
app_err!diagnostics here use\line continuations with indentation. In Rust, that indentation becomes literal spaces in the message, so the user-visible error will include large whitespace runs (e.g., between "managed" and "region"), reducing readability and making string matching in tests/logs brittle.
crates/cargo-anvil/src/run.rs:505 - The
format!string uses\line continuations with indentation. In Rust, the indentation spaces on the next line become part of the string, so the refusal diagnostic will contain large runs of spaces before "and other artifacts…" / "one before retrying.", making the message harder to read/copy-paste.
- Files reviewed: 12/12 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
Declining the "Needs a closer look" item in review 5135362024 (the two suppressed comments on A backslash at the end of a line inside a Rust string literal is the line-continuation escape, and it strips the newline and all leading whitespace on the next line. The indentation is not part of the string, so the diagnostics contain no whitespace runs at all — each continued line joins with exactly the single space written before the backslash. Verified rather than argued, with a throwaway test compiled in this crate and then removed: That is the same construct, the same indentation depth, and the same toolchain as the two sites in question, so the reported reading of them does not hold. The string-matching concern also does not arise: No change made. Flagging it here rather than silently ignoring it, since the finding is filed against code that has not changed and would otherwise be re-raised each pass. |
The gate named nine lines in `cargo-anvil`, every one of them a refusal: the paths that decline to act on something anvil cannot read. Each is now pinned by a test that asserts on the diagnostic, so a change that turned one into a silent no-op would fail rather than pass quietly. `toml_introduction_refusal` leaves a malformed marker to the planner instead of blaming TOML for it. `composed_host_state` reports an unreadable region by name rather than treating it as absent. `delta_region_body` says the markers are at fault rather than guessing that the host declares no repository key. `adopt_unmanaged_root_settings` leaves a host it cannot parse alone, as the headed path already does, and `canonical_value` compares datetimes by value so two different timestamps are a conflict rather than a silent deletion. `apply_files` keeps its assertion that only owned files propose, now exercised by a test that fires it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The catalog moved on `main`, so the recorded checksum no longer matched what this branch renders. Regenerated with `cargo anvil`, which reports all 97 artifacts unchanged: only the lock's provenance differs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…heck `retiring_regions` looked the region up in the lock with an exact host, while `live_region_keys`, `push_region_at` and `plan_removals` all use the host's real on-disk name. A lock recording `Deny.toml` for a file now spelled `deny.toml` therefore read as unowned: the old region stayed visible to the parser, and the replacement declaring the same table was refused as a duplicate of something the same plan was about to remove. `Manifest::region_checksum` supplies the region counterpart of `file_checksum`'s existing tolerance, matching the host without case and the id exactly — ids come from the catalog and never travel through the filesystem, so two differing only in case are two different regions. `a_case_only_host_rename_still_retires_a_clean_region` fails against the exact lookup and passes with it. Spellcheck was the next gate to run once cargo-sort stopped failing, and it rejected five words. `closers` and `unmanaged` are this design's own vocabulary and join `.spelling`; the two the new tests introduced are reworded instead of teaching the dictionary a possessive. Also adds `residue_insertion_separates_with_the_hosts_newline_when_there_is_no_gap`, which covers the one shape the CRLF cases missed: a host with no gap at all after the region, where the separator is the only line the insertion cannot copy from the file. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Spellcheck rejected `unowned` in the new test's doc comment. The gate ran clean locally on the previous push only because it was run before that comment was written, not because it tolerates the word — reintroducing it reproduces the CI failure exactly, so the local recipe is trustworthy as long as it runs last. The plain-comment use inside `retiring_regions` is untouched: the spellcheck config sets `dev_comments = false`, so `//` comments are not read. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
One failing whole-host parse has three causes and they do not share a remedy, but every refusal ended with the same sentence: reconcile the hand-written table. For a collision between two of anvil's own regions that sends the reader to reconcile a file they did not write, and for a host that was already unparsable it sends them to the wrong line entirely — every run, for as long as the collision lasts. `toml_introduction_refusal` now returns the parser's reason together with a `TomlRemedy`. The classification is ordered so that only this region's doing is attributed to it: a host that already fails to parse is diagnosed first, because it fails again whatever is spliced into it. Once the base is known good, masking every *other* managed region answers the rest — if the splice parses without them, the collision needs one of them and nothing hand-written is involved. The managed-region remedy also says what a repeat means. A table moving between two live regions is refused once when the region gaining it is planned first, then completes on the next run, because the region giving it up writes in the same pass; `a_table_moving_between_live_regions_settles_on_the_second_run` pins that, including the fourth run being a no-op, so a later change cannot turn one wasted run into a permanent refusal. A catalog that *exchanges* tables between two live regions never settles, and both the refusal and `updates.md` now say so and give the supported route: retire the region giving a table up, then add the one taking it, which composes in a single run because a retiring region is masked for the backstop. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
`anvil-mutants-diff` reported four survivors across `repair_markers`, `remove_region`, `ends_in_toml_table` and `build_plan`. Two of them were equivalent mutants, and no test could have caught either, so the code says what it means instead. `repair_markers` scanned for the closing marker from `start + 1`. Index `start` is an opener by construction and the scan looks for one that closes, so `skip(start)` was already the same walk; the `+ 1` only claimed a precision the code did not rely on. `build_plan` repaired a retiring region's markers early, guarded by the host also carrying a live region. That conjunct never decided anything: `push_region_at` repairs a live region's host when it plans the region, with the region's own comment syntax, and `plan_removals` repairs a retiring one at the point of removal. Dropping it leaves one condition that does decide -- whether the key is retiring -- and the comment now says why a live region is deliberately left to the later path. The remaining two were genuine gaps. `remove_region` eats the blank line below a region and falls back to the one above only at end-of-file; both existing cases had a blank on each side, where the branches produce the same text. `ends_in_toml_table` had no case where the last table was a different one, nor where it was an array-of-tables entry with the same path, so `&&` and `||` agreed on everything it was asked. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
93524fa to
a8e7703
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The edited-retired-region refusal path appends an unrelated “reconcile the hand-written table” remedy, producing a misleading diagnostic that should be corrected before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 38/39 changed files
- Comments generated: 1
- Review effort level: Lite
An edited retired region was refused with `TomlRemedy::HandWrittenTable`, so the diagnostic ended with "Reconcile the hand-written table with the managed one" for a fault where nothing was parsed and no table collided. The advice pointed at a file the reader may not have written, for a reason that is not about TOML at all -- the path is reached for any managed-region host, Dockerfile included. The enum is renamed `RefusalRemedy`, since it no longer classifies only TOML splices, and gains `EditedRetirement`. Its wording says what the reason cannot: why anvil stopped rather than removing the region, and that re-running changes nothing until one of the three listed actions is taken. The refusal also named the host by the spelling the lock recorded, while the marker repair, the read and the region lookup on the same path all use the spelling resolved from disk. A case-only rename of the host makes those differ, so the message named a file that is not there. It now names the resolved host. `Decision::LeaveAlone` is a no-op for both apply and the projected manifest, so this changes the diagnostic and the plan summary label only. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
There are correctness/UX issues in refusal/retirement handling (notably remedy selection for non-table errors and retirement syntax detection) that should be addressed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
crates/cargo-anvil/src/run.rs:986
plan_removalshard-codesCommentSyntax::Hashwhen locating/removing retired regions, but the catalog supports regions withCommentSyntax::SlashSlash(seecatalog/builder.rs). If a downstream catalog ever tracks a// >>> anvil-managed:region, retirement will treat it as absent (AlreadyGone) and drop the lock entry without removing the region text from disk.
- Files reviewed: 38/39 changed files
- Comments generated: 1
- Review effort level: Lite
Three faults, all reported as a hand-written table collision, none of which is one. Unpaired markers. `repair_markers` discarded every marker for an id when they did not form a pair. A region that has lost one sentinel still holds a body anvil generated, so dropping the survivor hands that body to the repository as ordinary text; `find_region` then reports no region and the writer appends the template beside it. Verified: the host ends the run with two copies of the generated body and only the newer one tracked. Unpaired markers are now left exactly as found and the region is refused, which restores the boundary as something only a human can resolve. Redundant markers *around* a complete pair are still cleaned up, because there the boundary is known. Case-only host renames. `plan_managed_region` looked the last render up under the lock's exact host spelling while `push_region_at` resolves the host from disk. After `Deny.toml` to `deny.toml` the lookup missed, `last_rendered` became `None`, and the strict check refused an untouched generated body as an edit -- every run, until someone emptied it by hand. It now uses the case-insensitive `Manifest::region_checksum`, as the retirement path already did. The id stays exact: ids come from the catalog, not the filesystem. Remedy classification. Every `plan_managed_region` failure was refused with `RefusalRemedy::HandWrittenTable`, including the edited-live-region and malformed-marker cases, which have nothing to do with tables. The function now returns a `ManagedRegionRefusal` carrying the remedy, and `splice` classifies its own: adoption conflicts really are hand-written tables and keep that wording, while structural failures do not. `EditedRegion` and `MalformedMarkers` join the enum. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
🤖 Clawpilot here! Posted automatically by Clawpilot (an AI agent), not by a human. Please verify before acting. Answering the suppressed comment in this review — The reading of the code is right, and I am not fixing it here. A retired region is by definition one the catalog no longer declares, so the catalog cannot supply its syntax — and the lock does not record one. That is the actual gap, and closing it means adding a syntax field to the manifest and deciding what to do for every entry written before it existed. A manifest format change plus its migration is outside what this PR is for, which is making TOML region adoption parser-backed and non-corrupting for #148 and #149. The assumption is already stated where it is made, in the comment above the It is also latent rather than live. Every region in the shipped catalog is Worth a tracked issue against the manifest format rather than a patch in this branch, and I would rather leave that call to a maintainer than open one on my own initiative. Two notes on the rest of that review, since the summary attributes both to "refusal/retirement handling":
|
There was a problem hiding this comment.
🟢 Approval recommended
The changes align tests, templates, runtime behavior, and design docs around a clear TOML-adoption/refusal contract, with only a minor internal doc-comment mismatch noted.
Review details
- Files reviewed: 38/39 changed files
- Comments generated: 1
- Review effort level: Lite
The coverage gate found the gap: `src/run.rs:611` and `:1051`, the retiring half of `repair_or_refuse` and the `continue` it drives in `plan_removals`. The live half was covered and the retiring half was only claimed, so the branch that declines to splice out a span it cannot delimit had never actually run. `a_retirement_with_unpaired_markers_is_refused_and_removes_nothing` stages a region the catalog no longer declares, with its closing sentinel gone, and asserts the refusal names the removal it declined, that the surviving sentinel and the body are both still there exactly once, and that the lock entry survives so the retirement can finish once a human restores the boundary. Also corrects two `PlanItem` field docs that no longer described the code: `rendered` is `None` for `repair_region`, which is a `Write`, and `rendered_checksum` is recorded for `Propose` and `InSync` as well as `Write` whenever it is present. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
It makes broad, behavior-defining changes to managed-region planning/splicing and TOML adoption/refusal semantics across multiple hosts, which warrants final human review despite strong test additions.
Review details
- Files reviewed: 38/39 changed files
- Comments generated: 0 new
- Review effort level: Lite
🤖 Clawpilot here! Posted automatically by Clawpilot (an AI agent), not by a human. Please verify before acting.
Closes #148. Closes #149.
Adopt existing TOML without losing user settings
Before, a hand-written table could receive a second managed header, leaving invalid TOML. Anvil now adopts compatible settings and preserves user-only entries outside the managed block.
Before:
After, with other generated settings omitted:
The closing marker is a comment.
ignorestill belongs to[advisories], but Anvil does not own it.Examples and decisions
yanked = "warn"; template"deny"[Hunspell]settingsHunspell.quirks.[[bin]]beside[lints]Effects
vu128andwebpki-root-certs.