Skip to content

fix(cargo-anvil): stop writing an unparsable TOML host when a table is hand-written - #162

Draft
Evgenii (Vaiz) wants to merge 3 commits into
mainfrom
u/vaiz/2026/09/04/anvil-toml-region-adoption
Draft

fix(cargo-anvil): stop writing an unparsable TOML host when a table is hand-written#162
Evgenii (Vaiz) wants to merge 3 commits into
mainfrom
u/vaiz/2026/09/04/anvil-toml-region-adoption

Conversation

@Vaiz

@Vaiz Evgenii (Vaiz) commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Closes #148. Closes #149.

The defect (#148)

Introducing a managed region that declares a whole table beside a hand-written copy of that table produced two identical headers, which TOML rejects outright. The generator had already rewritten the file and recorded the region by the time anything noticed, so any repository onboarding with a customised deny.toml got one cargo deny cannot read.

The repository's own migration fixture produced exactly that file and passed, because it only asserted that the output contained expected text.

Option 2 from the issue does not work

The issue proposed emitting the deny.toml regions as dotted keys without a table header. Checked directly against the TOML parser:

Shape Result
advisories.yanked = "deny" before a hand-written [advisories] parse error, duplicate key
advisories.yanked = "deny" after a hand-written [advisories] parses, but becomes advisories.advisories.yanked — silently ignored by cargo-deny
header-less body nested inside the hand-written [advisories] valid

So dotted keys either crash or, worse, silently misconfigure. Only nesting works.

What this does instead

Rather than reshape the catalog so regions nest into the user's table — which would make a region body depend on its host, churn every recorded checksum, and need a migration for repositories whose regions already carry the header — the nesting is inverted: the region's own header takes over the hand-written entries. Same end state, no per-host body, no migration.

Adoption now classifies each hand-written entry against the region body:

  • declared by both, same value — dropped; the region re-emits it.
  • declared only by hand — kept as residue and re-emitted after the region's closing sentinel, where it continues the table the region opens. Moved as its original source slice, so comments and spacing survive byte-for-byte.
  • declared by both, different values — a conflict. No output keeps both (TOML forbids the repeated key) and there is no basis for choosing, so the region is refused.

Safety net

Whatever adoption concludes, the spliced result is parsed before it is planned. If it would not parse, the region is refused: the host is left alone, a diagnostic names it, and every other artifact is still planned. The refusal is scoped to the region, not the run, so onboarding continues.

Other managed regions are masked out of that check. Two regions may legitimately declare the same key while a migration is in flight — the old combined region is removed in the same pass that writes the sections replacing it — and judging the intermediate text as a whole would refuse a migration that is about to become valid.

Finishing the move off the line scanner (#149)

Table location moves onto the parsed document, which is what makes the above possible:

  • The multi-line-string guard is gone. A bracketed line inside a """ value is a value to the parser, so it can no longer be mistaken for a header. Adoption previously declined for the whole host whenever """ or ''' appeared anywhere in it. New test: a host with a multi-line string and an adoptable table now succeeds.
  • The in_managed / dropping flag pair is gone. The rewrite is a copy of the gaps between non-overlapping deletion ranges, built up front. Those two flags were the source of both defects found in review of fix(cargo-anvil): adopt an unmanaged TOML table instead of duplicating it #140.

The host is parsed with existing managed regions blanked to spaces. Blanking preserves length, so every span still indexes the original text — and a host that already carries both a region copy and a hand-written copy (the duplicate-header file this repairs) still parses in that view, which a plain parse would not.

Spans required toml_edit::Document, not DocumentMut: the mutable document discards them.

Verification

The new fixture assertion was run against unmodified main and fails with the exact error from the issue:

deny.toml is not valid TOML: TOML parse error at line 8, column 2
  |
8 | [advisories]
  |  ^^^^^^^^^^
duplicate key

Fixtures writing TOML hosts now assert the output parses, and that kept configuration is still an entry of the table it was written under — a relocated key under the wrong header is a different setting that cargo-deny ignores. A new deny-conflict fixture covers the refusal end to end.

anvil-clippy, anvil-spellcheck, anvil-doc-build, anvil-cargo-sort, format, and the full cargo-anvil suite pass. Three cargo-gamma-lib cfg::build::tests failures on this machine were verified pre-existing on main with these changes stashed, and are unrelated.

Notes for review

  • templates/regions/deny-advisories.toml already no longer declares ignore, so the migration fixture's hand-written ignore list is residue rather than a conflict. The repository's own deny.toml still carries a stale ignore = [] inside the region from an earlier template; a future anvil run will drop it. Not touched here.
  • Two existing tests changed premise rather than behaviour, and are re-documented in place: [bin] beside [[bin]], and a host carrying both a region copy and a hand-written copy of one table, are both files TOML does not accept. Adoption declines them, which still guarantees what those tests existed to guarantee — nothing is deleted.

🤖 Posted automatically by Clawpilot (an AI agent), not by a human. Please verify before acting.

Review follow-ups (2026-09-04)

mask_regions no longer converts fallibly. It previously built a Vec<u8>, mutated it, and converted back with String::from_utf8(...).unwrap_or_else(|_| text.to_owned()). That error arm is unreachable — the input is already valid UTF-8 and masking only ever writes single ASCII spaces — but if it were ever reached it would return the text unmasked, handing the adoption parser the managed regions' own tables as though a human had written them. The masked copy is now assembled directly as a String, so it is valid UTF-8 by construction and there is no error arm left. \n and \r are still preserved and every masked byte is still one space, so the copy has the same length as the original and every byte offset and span is unchanged.

table_entries no longer skips an entry it cannot look up. Iterating a Table yields the key as a &str, so the Key carrying the decor and span has to be looked back up with get_key_value. That lookup is infallible — the key came from that very table — and the let ... else { continue } guarding it would have silently dropped one of the user's entries, which is precisely the failure mode this change exists to prevent. It is now an expect that states the invariant.

Coverage. cargo-anvil's 100.0% line-coverage gate was failing at 99.4%. The uncovered lines are now covered by tests for the residue and masking paths: residue insertion when the region is missing, the newlines supplied when the host or the residue lacks one, the blank line kept between relocated residue and what followed the region, masking of an unterminated region, and the stripping of leading blank lines from a relocated entry. just anvil-llvm-cov reports cargo-anvil 100.0% / 100.0% OK locally, with 532/532 tests passing.

Stale ignore = [] in the repository's own root deny.toml is pre-existing and deliberately untouched here: it was already removed from templates/regions/deny-advisories.toml upstream, and the next cargo anvil run drops it from the host.

Mutation gate (2026-09-04)

The mutation gate reported five surviving mutants in region.rs on the previous head. Each was a real hole in the tests, not a tool artefact, and all five are now killed by four new tests — no production code changed.

  • collect_values and table_entries descend into a child table only when it is dotted. Flipping that guard either way went unnoticed, so nothing pinned the distinction between a.b = 1 — configuration belonging to the table being read — and [a.b], which is a table in its own right. Getting it wrong deletes a hand-written key or relocates a nested table out from under its own header.
  • mask_other_managed_regions could be replaced by an empty string, and the newline test inside mask_regions could be inverted so the line breaks were blanked too, with no test noticing. Both destroy the one property the masking exists for: a copy of the host whose every byte offset still lands where it did in the original.

just anvil-mutants-diff now reports 73 mutants, 61 caught, 12 unviable, 0 missed. Coverage stays at cargo-anvil 100.0% / 100.0% OK.

Copilot AI lite review requested due to automatic review settings September 4, 2026 15:07

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 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-conflict end-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.

Comment thread crates/cargo-anvil/src/region.rs Outdated
}
// Every replaced byte became an ASCII space and every retained byte is
// unchanged, so the result is still valid UTF-8.
String::from_utf8(masked).unwrap_or_else(|_| text.to_owned())

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🤖 Clawpilot here! Posted automatically by Clawpilot (an AI agent), not by a human. Please verify before acting.

Agreed, and fixed — but by removing the fallible conversion rather than by changing what its error arm does. mask_regions now assembles the masked copy as a String instead of mutating a byte vector, so it is valid UTF-8 by construction and there is no error arm left to get wrong. Every masked byte is still a single ASCII space and \n/\r are still preserved, so the copy has the same length as the original and every byte offset and span still lands where it did.

That is stronger than swapping in an expect: the invariant is now enforced by the type rather than asserted at runtime, so it cannot silently disable masking or panic in production. Fixed in 45f44d3.

Comment thread crates/cargo-anvil/src/emit/managed_region.rs Outdated
@Vaiz
Evgenii (Vaiz) marked this pull request as draft September 4, 2026 15:18
@codecov-commenter

Codecov Comments Bot (codecov-commenter) commented Sep 4, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.0%. Comparing base (245b029) to head (b7dedf2).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##            main     #162      +/-   ##
=========================================
+ Coverage   97.5%   100.0%    +2.4%     
=========================================
  Files        299       24     -275     
  Lines      67766     2228   -65538     
=========================================
- Hits       66126     2228   -63898     
+ Misses      1640        0    -1640     
Flag Coverage Δ
linux 100.0% <100.0%> (?)
linux-arm 100.0% <100.0%> (?)
windows 100.0% <100.0%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Vaiz
Evgenii (Vaiz) force-pushed the u/vaiz/2026/09/04/anvil-toml-region-adoption branch from e874073 to 45f44d3 Compare September 4, 2026 16:16
Evgenii (Vaiz) and others added 3 commits September 4, 2026 18:25
…s hand-written

Introducing a managed region that declares a whole table beside a
hand-written copy of that table produced two identical headers, which
TOML rejects outright. The generator had already rewritten the file and
recorded the region by the time anything noticed, so a repository
onboarding with a customised `deny.toml` got one that `cargo deny`
cannot read.

Adoption now classifies each hand-written entry against the region body
instead of accepting or declining the table as a whole. An entry the
body also declares with the same value is dropped, because the region
re-emits it. An entry the body does not declare is kept as residue and
re-emitted after the region's closing sentinel, where it continues the
table the region opens -- carried across as its original source slice,
so the comments written around it survive. An entry both declare with
different values has no safe output at all, and is refused.

Whatever adoption concludes, the spliced result is parsed before it is
planned. If it would not parse, the region is refused: the host is left
alone, a diagnostic names it, and every other artifact is still planned.
Other managed regions are masked out of that check, because two regions
may legitimately declare the same key while a migration is in flight.

Table location moves off the line-oriented scanner and onto the parsed
document, which is what makes the above possible and removes two
long-standing limitations. A bracketed line inside a `"""` value is a
value to the parser, so the guard that declined adoption for any host
merely containing a multi-line string is gone. The rewrite is now a
copy of the gaps between non-overlapping deletion ranges, so the
`in_managed`/`dropping` streaming flag pair -- the source of two defects
found in review of #140 -- is gone with it.

The fixtures that write TOML hosts now assert the output parses rather
than that it contains an expected fragment, which is what let this
survive: the `migration` fixture produced the duplicate header and
passed.

Closes #148
Closes #149

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…esidue paths

Masking a managed region built a byte vector and converted it back with a fallible `String::from_utf8`, whose error arm silently returned the text unmasked. That arm can only be reached if the invariant it guards has already broken, and returning unmasked text there would hand the adoption parser the region's own tables as though a human had written them. The copy is now assembled as a `String`, so it is valid UTF-8 by construction and there is no error arm to get wrong; every masked byte is still a single ASCII space and the line breaks are still kept, so every byte offset and span is unchanged.

Recovering a `Key` from a key that table iteration just yielded is likewise infallible, and the `continue` that guarded it would have silently dropped one of the user's entries — the exact failure this module exists to prevent. It now says so with an `expect`.

Adds tests for the residue and masking paths that had no coverage: a residue insertion whose region is missing, the newlines supplied when the host or the residue lacks one, the blank line kept between relocated residue and what followed the region, masking of an unterminated region, and the stripping of leading blank lines from a relocated entry.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The mutation gate reported five surviving mutants in `region.rs`, each of
them a real hole in the tests rather than an artefact of the tool:

* `collect_values` and `table_entries` descend into a child table only
  when it is dotted. Flipping that guard either way went unnoticed, so
  nothing pinned the distinction between `a.b = 1` — configuration
  belonging to the table being read — and `[a.b]`, which is a table of
  its own. Getting it wrong deletes a hand-written key or relocates a
  nested table out from under its own header.
* `mask_other_managed_regions` could return an empty string, and the
  newline test inside `mask_regions` could be inverted to blank the line
  breaks as well, with no test noticing. Both destroy the property the
  masking exists for: a copy of the host whose every byte offset still
  lands where it did in the original.

Four tests close those holes. No production code changes.
@Vaiz
Evgenii (Vaiz) force-pushed the u/vaiz/2026/09/04/anvil-toml-region-adoption branch from 45f44d3 to b7dedf2 Compare September 4, 2026 18:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

3 participants