Preserve comments when auto-correcting unused dependencies

[corsonknowles]

check-unused-dependencies --auto-correct silently deletes every comment in a package.yml it touches.

Reproduction

# Header comment.
enforce_dependencies: true
enforce_privacy: true
dependencies:
# Comment inside the dependencies block.
- packs/bar
- packs/baz          # unused
ignored_dependencies:
# Deliberately ignored: bop would cycle.
- packs/bop

Run pks check-unused-dependencies --auto-correct. On main, - packs/baz is removed, and all three comments go with it. That includes the one explaining why an entry is in ignored_dependencies, which is the note a future reader most needs. The loss is easy to miss in review unless you read the whole file.

Cause

remove_reference_to_dependency rebuilds the Pack struct and calls write_pack_to_disk, which re-serializes the whole file through serde. serde has no notion of comments, so every round trip drops them. The same round trip also reorders the top-level keys into struct field order.

Fix

Remove the unused entries as text, then check the result:

1. Edit the text. Drop the matching items from the top-level dependencies: block and leave everything else alone, including comments, key order, line endings and other blocks such as ignored_dependencies:. The matcher handles unindented and indented lists, quoted items, a trailing # comment on an item or on the dependencies: key, and blank lines inside the list. A comment covers the items below it, up to the next blank line or comment, and is removed only when all of those items are. A commented-out entry such as # - packs/a is always kept, along with any comment directly above it. Removing items one at a time gives the same file as removing them together. If no items remain, the whole dependencies: block is removed, including its comments and blank lines, as serializing did.
2. Check the edit. Parse the edited text back into a Pack and require it to equal the original minus exactly the removed dependencies.
3. Fall back. If the check fails, for example on a flow-style dependencies: [a, b], rewrite the file through write_pack_to_disk as before and print a warning on stderr naming the file (relative to the project root) and the removed dependencies, and saying that comments were not preserved and keys may be reordered. Every layout auto-correct handled before is still corrected.

The editing lives in a new module, src/packs/checker/dependency_removal.rs.

Tests

• Unit tests in dependency_removal.rs cover:
  • first, middle and last items, and removing every item
  • dependencies: as the last key, with and without a final newline
  • each layout listed above, a # that is part of a value, CRLF line endings, and ---/... document markers
  • a nested metadata: dependencies: key and ignored_dependencies: left untouched
  • the comment rule: a note kept while any item it covers remains, commented-out entries and the note above them kept, a trailing commented-out entry removed with an emptied block, a footer comment kept
  • blank lines: no doubled or leftover blank line at either end of the list
  • order independence: every subset of a commented six-item list, removed one at a time in both orders, matches removing it at once
  • layouts the text edit declines to handle
• Integration tests:
  • The fixture has comments above the first key, inside the list, and between the list and a following key.
  • The reproduction above, in indented, quoted form with blank lines, is asserted byte for byte with empty stderr.
  • The flow-style fallback asserts the whole rewritten file and the exact warning.
  • Every auto-correct test runs check-unused-dependencies again afterwards and asserts it passes.
• Vetting on real files. A throwaway harness, not committed, ran the text edit over 775 real package.yml files from a large monorepo, plus indented, commented, quoted, blank-line and CRLF variants of each: every single dependency, all of them, and every other one, 61,273 removals in all. None fell back, and every output was a pure line deletion of its input. Two order-dependence bugs it found are fixed and covered above.
• cargo test, cargo clippy --all-targets --all-features -- -D warnings and cargo fmt --all -- --check all pass.

CHANGELOG

The entry is under Unreleased → Fixes, now that main is merged in.

Scope

This fixes only the auto-correct path. Other commands still rewrite a whole pack through write_pack_to_disk and lose its comments: create, rm, add-dependency, update-dependencies-for-constant, and lint-package-yml-files, which rewrites every package.yml. A general fix needs a format-preserving YAML representation, such as yamlpatch or yaml-edit. That is a much larger change, so it isn't in this PR.

🤖 Generated with Claude Code

[thelowlypeon]

fwiw, in the past i've handled this by moving comments into a meta.comments section as text

[dduugg]

Thanks for picking this up. Losing the note above an ignored_dependencies entry is a real problem and worth fixing. As written, though, this swaps a cosmetic bug for a correctness one, so I'm requesting changes.

Auto-correct now silently skips dependencies it can't match as text

remove_dependency_lines compares the parsed names from pack.dependencies against raw lines, and only matches an unquoted - name at column 0 with nothing after it (checker.rs:508-509). Anything else is left in place, and the command still exits 0 with no output. main removes the dependency in every one of these cases:

• a trailing comment on the item: - packs/baz # unused
• an indented list: - packs/baz (the style packwerk's USAGE.md uses)
• quoted items: - "packs/baz", - 'packs/baz'
• flow style: dependencies: [packs/bar, packs/baz]
• a comment on the key line: dependencies: # keep sorted
• a blank line inside the list, which ends the block at line 517

The first case is the reproduction from the PR description. Run verbatim against this branch, auto-correct exits 0, the file is byte-identical to the input, and a second check-unused-dependencies still reports packs/foo depends on packs/baz but does not use it. On main the dependency is removed (and the comments are lost, as you describe).

This hurts most where auto-correct runs unattended. A common setup is a pre-commit hook that runs check-unused-dependencies --auto-correct and stages the result, with CI running the check without --auto-correct. After this change the hook passes, then CI fails and tells the user to run --auto-correct, which they just did. Indented lists aren't rare: 17 of the 33 fixture package.yml files in this repo that have a dependencies list use them, and so do about 5% of the ones in a large monorepo I checked. Files written by pks or by Ruby's YAML.dump are unindented, so the gap is hand-edited files.

Suggested direction: keep the text edit, but check its result. Parse the edited text back into a Pack and confirm it equals the original minus exactly the removed dependencies. If it doesn't, because the file uses a layout the matcher doesn't handle, fall back to write_pack_to_disk with a warning. That loses comments but is correct, so nothing that works today regresses. It's also worth widening the matcher to accept leading indentation, quotes, a trailing # comment, and blank lines, so the fallback is rare. A format-preserving YAML crate (yamlpatch, yaml-edit) could replace all of this later, but that's a bigger change and doesn't need to block this one.

Tests

remove_dependency_lines is a pure function with no unit tests, and the integration fixture only covers the one layout it handles. Please add unit tests for the cases above, plus first/middle/last item, removing every dependency, and dependencies: as the last key. The old fixture had dependencies: last, and the reordered one no longer covers that. It would also help for the integration test to run check-unused-dependencies again after auto-correct and assert it's clean, rather than only comparing strings.

Smaller things

• A comment directly above a removed item stays behind and ends up describing the next item. The comment at line 515 says a comment "belongs to whatever follows it", but the code keeps it even when what follows is deleted.
• Removing every dependency leaves a bare dependencies: (null). pks and packwerk both load that, but pks lint-package-yml-files then deletes the key, so the output is no longer lint-clean the way it was on main. Dropping the key when the list ends up empty would match the old behavior.
• CRLF files come back as LF throughout (lines() plus join("\n")), even when nothing is removed, because the write at line 488 is unconditional. main did the same, so it isn't a regression, but it contradicts "byte for byte identical". split_inclusive('\n') keeps line endings as they are, and skipping the write when nothing changed would help too.
• In the Scope section, the command is update-dependencies-for-constant, not add-constant-dependencies. lint-package-yml-files also rewrites every package.yml through write_pack_to_disk (packs.rs:244), so it strips comments as well.
• This is a user-visible behavior change, so it probably wants an entry under Unreleased in CHANGELOG.md, as #45 and #63 have.
• Nit: the new comments at lines 477-479, 496 and 515-516 run past the 80-column width the rest of the file keeps to. rustfmt doesn't wrap comments on stable, so fmt --check won't flag them. The comment at 477-480 also reads more like commit-message history than a description of the code.

[dduugg]

Thanks for the quick turnaround. This covers everything from my first review: the matcher now handles indented, quoted, inline-comment, key-comment and blank-line layouts; the re-parse check and fallback mean nothing that worked before regresses; and the unit tests cover the cases I asked about. The reproduction from the description now changes only the - packs/baz line. I also ran the new code over about 850 real package.yml files from two large monorepos, removing each dependency in turn (about 10,700 cases). Every case took the text path and produced exactly the expected result.

A few things before this merges:

Comments above a removed item are deleted, whatever they say

pending_comments.clear() (dependency_removal.rs:44-45) drops every comment directly above a removed item. That fixes the orphaned per-item note from my first review, but it also takes out comments that aren't about that item:

dependencies:
# Keep this list sorted.
- packs/baz   # unused: the header is deleted with it, though packs/bar stays
- packs/bar

dependencies:
- packs/bar
# - packs/bop  # re-add after the migration
- packs/baz   # unused: the commented-out entry is deleted too

Neither prints anything. It isn't a regression, since main loses every comment, but the point of this PR is that comments are worth keeping, and the description says everything else is left alone. One option is to treat a comment as covering the run of items below it, up to the next blank line or comment, and drop it only when every item in that run is removed. The simpler one is to never delete comments and accept the occasional stale note. Either way, a commented-out # - entry should probably always be kept, and the suggested CHANGELOG text should describe whichever rule you pick.

Removing every dependency leaves the block's other lines behind

When no items remain, only the dependencies: line is removed (dependency_removal.rs:74-76). Comments and blank lines from inside the list stay, still indented, under the previous key:

enforce_dependencies: true
  # Keep sorted.

enforce_privacy: true

A comment on the key line (dependencies: # keep sorted) is lost along with the key. And if a block scalar comes right before dependencies:, a leftover indented comment becomes part of the scalar. The parse check catches that and falls back, so the whole file loses its comments. Removing the whole block when it ends up empty would fix all three.

CHANGELOG

The conflict only exists because the branch is 10 commits behind main. After merging main in, the entry is a single insert under the existing Unreleased → Fixes section, next to the ones #65 and #66 added. We squash-merge, so leaving it to the merge means someone else writes it. Merging main also gets CI running on a current base: the green run on this head tested a merge with #64, before #59, #65, #66 and #67 landed. (The suite does pass locally on a merge with current main.)

Smaller things

• The fallback test (tests/check_unused_dependencies.rs:189-194) only checks contains. The fallback output is deterministic, so an assert_eq on the whole file would also cover the enforce_* keys and the dropped header.
• The warning (checker.rs:491-496) prints the absolute path. pack.relative_yml() in backticks would match the warnings #65 added. It could also say which dependency was removed, and that the rewrite reorders keys.
• Some(updated) if updated == contents => {} at checker.rs:485 can't be reached. The names always come from the parsed file, so an edit that removes nothing fails is_exact_removal and falls back.
• Nit: removing an item between blank lines leaves two blank lines in a row, and handles_blank_lines_inside_the_list asserts that.