From b672ac3ddd7e724da9a1e46ed10168b6733ef6d2 Mon Sep 17 00:00:00 2001
From: David Corson-Knowles <david.corsonknowles@gusto.com>
Date: Tue, 3 Nov 2026 15:52:07 -0800
Subject: [PATCH] Preserve comments when auto-correcting unused dependencies

`check-unused-dependencies --auto-correct` rewrote every package.yml it
touched by serializing the pack, which deleted its comments and
reordered its keys. It now removes the unused entries from the
`dependencies:` list as text and leaves the rest of the file as it was,
line endings included.

A comment covers the entries below it, up to the next blank line or
comment, and is removed only when all of those entries are. A
commented-out entry, and any comment directly above one, is always
kept. When no entries remain, the `dependencies:` key stays, with
nothing under it, while a comment is on it, directly above it or still
under it; otherwise the whole block is removed. pks and packwerk both
read a key with no entries as an empty list. Removing entries one at a
time gives the same file as removing them together, which a test checks
for every subset of every list of up to five lines.

The edited text must parse back to the original pack minus exactly the
removed dependencies. When it doesn't, as for a flow-style list, the
file is rewritten as before and a warning names the file and the
dependencies removed, in sorted order.
---
 CHANGELOG.md                                  |  22 +
 src/packs/checker.rs                          |  47 +-
 src/packs/checker/dependency_removal.rs       | 696 ++++++++++++++++++
 tests/check_unused_dependencies.rs            | 154 +++-
 tests/common/mod.rs                           |   5 +-
 .../packs/foo/package.yml                     |   5 +-
 6 files changed, 900 insertions(+), 29 deletions(-)
 create mode 100644 src/packs/checker/dependency_removal.rs

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 389898a7..9afb7fcf 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -142,6 +142,28 @@ So `pks check` may report a violation it didn't, or no longer report one, and a
 `package_todo.yml` entry for one it no longer reports is stale until you run
 `pks update`.
 
+#### `check-unused-dependencies --auto-correct` keeps comments in `package.yml`
+
+Auto-correct used to rewrite every `package.yml` it touched from scratch, which
+deleted all of its comments and reordered its keys. It now removes the unused
+entries from the `dependencies:` list and leaves the rest of the file as it
+was, line endings included. A comment covers the entries below it, up to the
+next blank line or comment, and is removed only when all of those entries are.
+A commented-out entry such as `# - packs/a` is always kept, along with any
+comment directly above it. If no dependencies remain, the `dependencies:` key
+is kept, with no entries under it, while a comment is on it, directly above it
+or still under it; otherwise the whole block is removed. Removing entries one at
+a time gives the same file as removing them together.
+
+If a list is written in a form that cannot be edited this way, such as
+`dependencies: [packs/a, packs/b]`, the file is rewritten as before and a
+warning names the file and the dependencies removed.
+
+Only this command keeps comments. `lint-package-yml-files`, `add-dependency`,
+`create`, `rm` and `update-dependencies-for-constant` still rewrite the
+`package.yml` they change and drop its comments, so running
+`lint-package-yml-files` afterwards removes the comments kept here.
+
 ## 0.7.0
 
 ### Features
diff --git a/src/packs/checker.rs b/src/packs/checker.rs
index 7cf22bdc..624188c0 100644
--- a/src/packs/checker.rs
+++ b/src/packs/checker.rs
@@ -3,6 +3,7 @@ mod dependency;
 pub(crate) mod layer;
 
 mod common_test;
+mod dependency_removal;
 mod folder_privacy;
 pub(crate) mod pack_checker;
 mod privacy;
@@ -606,15 +607,43 @@ fn remove_reference_to_dependency(
     pack: &Pack,
     dependency_names: &[String],
 ) -> anyhow::Result<()> {
-    let without_dependency = pack
-        .dependencies
-        .iter()
-        .filter(|dependency| !dependency_names.contains(dependency));
-    let updated_pack = Pack {
-        dependencies: without_dependency.cloned().collect(),
-        ..pack.clone()
-    };
-    write_pack_to_disk(&updated_pack)?;
+    let contents = std::fs::read_to_string(&pack.yml).map_err(|e| {
+        anyhow::Error::new(e)
+            .context(format!("Failed to read pack {:?}", pack.yml))
+    })?;
+
+    match dependency_removal::remove_dependencies(&contents, dependency_names) {
+        Some(updated) => std::fs::write(&pack.yml, updated).map_err(|e| {
+            anyhow::Error::new(e)
+                .context(format!("Failed to write pack to disk {:?}", pack.yml))
+        })?,
+        None => {
+            let mut sorted_names: Vec<&String> =
+                dependency_names.iter().collect();
+            sorted_names.sort();
+            eprintln!(
+                "Warning: could not edit the dependencies list in `{}` in \
+                 place to remove {}, so the file was rewritten: its comments \
+                 were not preserved and its keys may be reordered.",
+                pack.relative_yml().display(),
+                sorted_names
+                    .iter()
+                    .map(|name| format!("`{name}`"))
+                    .collect::<Vec<_>>()
+                    .join(", "),
+            );
+            let without_dependency = pack
+                .dependencies
+                .iter()
+                .filter(|dependency| !dependency_names.contains(dependency));
+            let updated_pack = Pack {
+                dependencies: without_dependency.cloned().collect(),
+                ..pack.clone()
+            };
+            write_pack_to_disk(&updated_pack)?;
+        }
+    }
+
     Ok(())
 }
 // Note: Display impl was removed from CheckAllResult. Use write_text() directly with Configuration.
diff --git a/src/packs/checker/dependency_removal.rs b/src/packs/checker/dependency_removal.rs
new file mode 100644
index 00000000..cdeecf32
--- /dev/null
+++ b/src/packs/checker/dependency_removal.rs
@@ -0,0 +1,696 @@
+use crate::packs::pack::Pack;
+
+/// Returns `contents` with `names` removed from its top-level `dependencies:`
+/// list, edited as text so comments, key order and line endings survive.
+///
+/// Returns `None` when the edited text does not parse back to the original
+/// pack minus exactly `names`, which happens when the list uses a layout the
+/// line matcher does not recognise (flow style, multi-line scalars, ...).
+pub(super) fn remove_dependencies(
+    contents: &str,
+    names: &[String],
+) -> Option<String> {
+    let edited = remove_dependency_lines(contents, names);
+    is_exact_removal(contents, &edited, names).then_some(edited)
+}
+
+fn is_exact_removal(original: &str, edited: &str, names: &[String]) -> bool {
+    let (Ok(mut expected), Ok(actual)) = (
+        yaml_serde::from_str::<Pack>(original),
+        yaml_serde::from_str::<Pack>(edited),
+    ) else {
+        return false;
+    };
+    expected.dependencies.retain(|name| !names.contains(name));
+    expected == actual
+}
+
+/// Drops each matching item. A comment covers the items below it, up to the
+/// next blank line or comment, and is dropped only when all of them are. A
+/// commented-out entry, and any comment directly above one, is kept. If no
+/// items are left, the key stays while any comment is on it, directly above
+/// it or left under it, so no comment is ever lost with the block; otherwise
+/// the whole block goes, as serializing the pack would do. Everything else is
+/// verbatim.
+fn remove_dependency_lines(contents: &str, names: &[String]) -> String {
+    let lines: Vec<&str> = contents.split_inclusive('\n').collect();
+    let Some(key) = lines.iter().position(|line| is_dependencies_key(line))
+    else {
+        return contents.to_string();
+    };
+    let mut block_end = key + 1;
+    while lines.get(block_end).is_some_and(|line| {
+        let trimmed = line.trim_start();
+        trimmed.is_empty()
+            || trimmed.starts_with('#')
+            || trimmed.starts_with("- ")
+    }) {
+        block_end += 1;
+    }
+
+    let (block, kept_items) = edit_block(&lines[key + 1..block_end], names);
+    let keep_key = kept_items > 0
+        || has_comment(lines[key])
+        || key > 0 && lines[key - 1].trim_start().starts_with('#')
+        || block.iter().any(|line| line.trim_start().starts_with('#'));
+    let mut out: Vec<&str> = lines[..key].to_vec();
+    if keep_key {
+        out.push(lines[key]);
+        out.extend(block);
+    }
+    out.extend(&lines[block_end..]);
+    out.concat()
+}
+
+/// The block's lines with the named items removed, and how many items are
+/// left.
+fn edit_block<'a>(
+    lines: &[&'a str],
+    names: &[String],
+) -> (Vec<&'a str>, usize) {
+    let mut out: Vec<&'a str> = Vec::new();
+    let mut group = Group::default();
+    let mut kept_items = 0;
+
+    for &line in lines {
+        let trimmed = line.trim_start();
+        if trimmed.starts_with("- ") {
+            group.items.push(line);
+            continue;
+        }
+        if trimmed.starts_with('#') && group.items.is_empty() {
+            group.comments.push(line);
+            continue;
+        }
+        let vanished = group.flush(&mut out, names, &mut kept_items);
+        if trimmed.starts_with('#') {
+            group.comments.push(line);
+        } else if !(vanished && out.last().is_none_or(|l| l.trim().is_empty()))
+        {
+            // Skipped when a removed group sat between a blank line and
+            // another blank line or the start of the list.
+            out.push(line);
+        }
+    }
+    let vanished = group.flush(&mut out, names, &mut kept_items);
+    if vanished && out.last().is_some_and(|l| l.trim().is_empty()) {
+        out.pop();
+    }
+    (out, kept_items)
+}
+
+/// A run of comment lines and the items below them.
+#[derive(Default)]
+struct Group<'a> {
+    comments: Vec<&'a str>,
+    items: Vec<&'a str>,
+}
+
+impl<'a> Group<'a> {
+    /// Moves the group's surviving lines to `out`, returning whether it had
+    /// lines and none survived.
+    fn flush(
+        &mut self,
+        out: &mut Vec<&'a str>,
+        names: &[String],
+        kept_items: &mut usize,
+    ) -> bool {
+        let had_lines = !self.comments.is_empty() || !self.items.is_empty();
+        let had_items = !self.items.is_empty();
+        let kept: Vec<&'a str> = self
+            .items
+            .drain(..)
+            .filter(|line| !is_named_item(line, names))
+            .collect();
+        *kept_items += kept.len();
+        if had_items && kept.is_empty() {
+            // Keep commented-out entries and the comments above them.
+            let last_entry = self
+                .comments
+                .iter()
+                .rposition(|line| is_commented_out_item(line));
+            self.comments
+                .truncate(last_entry.map_or(0, |index| index + 1));
+        }
+        let vanished = had_lines && self.comments.is_empty() && kept.is_empty();
+        out.append(&mut self.comments);
+        out.extend(kept);
+        vanished
+    }
+}
+
+fn is_named_item(line: &str, names: &[String]) -> bool {
+    let item = &line.trim_start()["- ".len()..];
+    names.iter().any(|name| name == item_value(item))
+}
+
+fn is_commented_out_item(line: &str) -> bool {
+    line.trim_start()
+        .strip_prefix('#')
+        .is_some_and(|rest| rest.trim_start().starts_with("- "))
+}
+
+/// Matches the top-level key only: a nested `dependencies:` is indented.
+fn is_dependencies_key(line: &str) -> bool {
+    strip_comment(line).trim_end() == "dependencies:"
+}
+
+fn has_comment(line: &str) -> bool {
+    strip_comment(line).len() < line.len()
+}
+
+/// The scalar value of a block list item, without quotes or a trailing
+/// comment. Escapes inside quotes are not decoded; the parse check in
+/// `remove_dependencies` catches any item that this misreads.
+fn item_value(item: &str) -> &str {
+    let item = item.trim();
+    for quote in ['"', '\''] {
+        if let Some(rest) = item.strip_prefix(quote) {
+            return rest.find(quote).map_or(item, |end| &rest[..end]);
+        }
+    }
+    strip_comment(item).trim_end()
+}
+
+/// In YAML, `#` starts a comment only at the start of a line or after
+/// whitespace, so `packs/a#b` is a plain value.
+fn strip_comment(text: &str) -> &str {
+    text.match_indices('#')
+        .find(|(index, _)| *index == 0 || text[..*index].ends_with([' ', '\t']))
+        .map_or(text, |(index, _)| &text[..index])
+}
+
+#[cfg(test)]
+mod tests {
+    use super::remove_dependencies;
+    use pretty_assertions::assert_eq;
+
+    fn remove(contents: &str, names: &[&str]) -> Option<String> {
+        let names: Vec<String> = names.iter().map(|n| n.to_string()).collect();
+        remove_dependencies(contents, &names)
+    }
+
+    fn assert_removes(before: &str, names: &[&str], after: &str) {
+        assert_eq!(remove(before, names).as_deref(), Some(after));
+    }
+
+    #[test]
+    fn removes_first_middle_and_last_items() {
+        let before = "dependencies:\n- packs/a\n- packs/b\n- packs/c\n";
+        assert_removes(
+            before,
+            &["packs/a"],
+            "dependencies:\n- packs/b\n- packs/c\n",
+        );
+        assert_removes(
+            before,
+            &["packs/b"],
+            "dependencies:\n- packs/a\n- packs/c\n",
+        );
+        assert_removes(
+            before,
+            &["packs/c"],
+            "dependencies:\n- packs/a\n- packs/b\n",
+        );
+        assert_removes(
+            before,
+            &["packs/a", "packs/c"],
+            "dependencies:\n- packs/b\n",
+        );
+    }
+
+    #[test]
+    fn removes_the_key_when_no_items_remain() {
+        assert_removes(
+            "enforce_dependencies: true\ndependencies:\n- packs/a\n- packs/b\nlayer: utilities\n",
+            &["packs/a", "packs/b"],
+            "enforce_dependencies: true\nlayer: utilities\n",
+        );
+        assert_removes("dependencies:\n- packs/a\n", &["packs/a"], "");
+        assert_removes(
+            "# Header.\n\ndependencies:\n- packs/a\n",
+            &["packs/a"],
+            "# Header.\n\n",
+        );
+    }
+
+    #[test]
+    fn handles_dependencies_as_the_last_key() {
+        assert_removes(
+            "enforce_privacy: true\ndependencies:\n- packs/a\n- packs/b\n",
+            &["packs/b"],
+            "enforce_privacy: true\ndependencies:\n- packs/a\n",
+        );
+        assert_removes(
+            "enforce_privacy: true\ndependencies:\n- packs/a\n- packs/b",
+            &["packs/b"],
+            "enforce_privacy: true\ndependencies:\n- packs/a\n",
+        );
+    }
+
+    #[test]
+    fn handles_indented_lists() {
+        assert_removes(
+            "dependencies:\n  - packs/a\n  - packs/b\nlayer: utilities\n",
+            &["packs/a"],
+            "dependencies:\n  - packs/b\nlayer: utilities\n",
+        );
+    }
+
+    #[test]
+    fn handles_quoted_items() {
+        assert_removes(
+            "dependencies:\n- \"packs/a\"\n- 'packs/b'\n- packs/c\n",
+            &["packs/a", "packs/b"],
+            "dependencies:\n- packs/c\n",
+        );
+    }
+
+    #[test]
+    fn handles_a_trailing_comment_on_an_item() {
+        assert_removes(
+            "dependencies:\n- packs/a  # used\n- packs/b  # unused\n",
+            &["packs/b"],
+            "dependencies:\n- packs/a  # used\n",
+        );
+    }
+
+    #[test]
+    fn keeps_a_hash_that_is_part_of_the_value() {
+        assert_removes(
+            "dependencies:\n- packs/a#b\n- packs/a\n",
+            &["packs/a"],
+            "dependencies:\n- packs/a#b\n",
+        );
+    }
+
+    #[test]
+    fn handles_a_comment_on_the_key_line() {
+        assert_removes(
+            "dependencies: # keep sorted\n- packs/a\n- packs/b\n",
+            &["packs/b"],
+            "dependencies: # keep sorted\n- packs/a\n",
+        );
+    }
+
+    #[test]
+    fn handles_inline_comments_on_keys_and_values() {
+        let before = "\
+enforce_privacy: true # why
+dependencies: # deps
+  - packs/a  # used by A
+  - packs/b  # unused
+ignored_dependencies: # note
+  - packs/b # would cycle
+layer: utilities # layer note
+";
+        assert_removes(
+            before,
+            &["packs/b"],
+            "\
+enforce_privacy: true # why
+dependencies: # deps
+  - packs/a  # used by A
+ignored_dependencies: # note
+  - packs/b # would cycle
+layer: utilities # layer note
+",
+        );
+        assert_removes(
+            before,
+            &["packs/a", "packs/b"],
+            "\
+enforce_privacy: true # why
+dependencies: # deps
+ignored_dependencies: # note
+  - packs/b # would cycle
+layer: utilities # layer note
+",
+        );
+    }
+
+    #[test]
+    fn ignores_names_and_syntax_inside_inline_comments() {
+        assert_removes(
+            "dependencies: # a: b, [c]\n- packs/a # packs/b\n- packs/b # - packs/c\n- packs/c\n",
+            &["packs/b"],
+            "dependencies: # a: b, [c]\n- packs/a # packs/b\n- packs/c\n",
+        );
+        assert_removes(
+            "dependencies:\n- \"packs/a\" # see \"packs/b\"\n- 'packs/b' # don't\n",
+            &["packs/b"],
+            "dependencies:\n- \"packs/a\" # see \"packs/b\"\n",
+        );
+        assert_removes(
+            "dependencies: #\n- packs/a #\n- packs/b #\n",
+            &["packs/a"],
+            "dependencies: #\n- packs/b #\n",
+        );
+    }
+
+    #[test]
+    fn handles_inline_comments_after_tabs_and_before_crlf() {
+        assert_removes(
+            "dependencies:\t# k\n- packs/a\t# a\n- packs/b\t# b\n",
+            &["packs/b"],
+            "dependencies:\t# k\n- packs/a\t# a\n",
+        );
+        assert_removes(
+            "dependencies: # k\r\n- packs/a # a\r\n- packs/b # b\r\n",
+            &["packs/b"],
+            "dependencies: # k\r\n- packs/a # a\r\n",
+        );
+    }
+
+    #[test]
+    fn handles_blank_lines_inside_the_list() {
+        assert_removes(
+            "dependencies:\n- packs/a\n\n- packs/b\n\nlayer: utilities\n",
+            &["packs/b"],
+            "dependencies:\n- packs/a\n\nlayer: utilities\n",
+        );
+    }
+
+    #[test]
+    fn removes_the_comment_directly_above_a_removed_item() {
+        assert_removes(
+            "dependencies:\n# Needed for A.\n- packs/a\n# Needed for B.\n- packs/b\n",
+            &["packs/a"],
+            "dependencies:\n# Needed for B.\n- packs/b\n",
+        );
+    }
+
+    #[test]
+    fn keeps_a_comment_separated_from_a_removed_item_by_a_blank_line() {
+        assert_removes(
+            "dependencies:\n# Keep sorted.\n\n- packs/a\n- packs/b\n",
+            &["packs/a"],
+            "dependencies:\n# Keep sorted.\n\n- packs/b\n",
+        );
+    }
+
+    #[test]
+    fn keeps_a_comment_between_the_list_and_the_next_key() {
+        assert_removes(
+            "dependencies:\n- packs/a\n- packs/b\n# About the layer.\nlayer: utilities\n",
+            &["packs/b"],
+            "dependencies:\n- packs/a\n# About the layer.\nlayer: utilities\n",
+        );
+    }
+
+    #[test]
+    fn leaves_ignored_dependencies_and_nested_keys_alone() {
+        assert_removes(
+            "\
+metadata:
+  dependencies:
+  - packs/a
+dependencies:
+- packs/a
+- packs/b
+ignored_dependencies:
+# Deliberately ignored: a would cycle.
+- packs/a
+",
+            &["packs/a"],
+            "\
+metadata:
+  dependencies:
+  - packs/a
+dependencies:
+- packs/b
+ignored_dependencies:
+# Deliberately ignored: a would cycle.
+- packs/a
+",
+        );
+    }
+
+    #[test]
+    fn preserves_crlf_line_endings() {
+        assert_removes(
+            "# Header.\r\ndependencies:\r\n- packs/a\r\n- packs/b\r\n",
+            &["packs/b"],
+            "# Header.\r\ndependencies:\r\n- packs/a\r\n",
+        );
+    }
+
+    #[test]
+    fn keeps_a_comment_while_any_item_it_covers_remains() {
+        assert_removes(
+            "dependencies:\n# Keep this list sorted.\n- packs/baz\n- packs/bar\n",
+            &["packs/baz"],
+            "dependencies:\n# Keep this list sorted.\n- packs/bar\n",
+        );
+        assert_removes(
+            "dependencies:\n# Keep this list sorted.\n- packs/baz\n- packs/bar\n\n- packs/bop\n",
+            &["packs/baz", "packs/bar"],
+            "dependencies:\n- packs/bop\n",
+        );
+    }
+
+    #[test]
+    fn keeps_a_commented_out_entry() {
+        assert_removes(
+            "dependencies:\n- packs/bar\n# - packs/bop  # re-add after the migration\n- packs/baz\n",
+            &["packs/baz"],
+            "dependencies:\n- packs/bar\n# - packs/bop  # re-add after the migration\n",
+        );
+    }
+
+    #[test]
+    fn removes_the_whole_block_when_no_comment_remains() {
+        assert_removes(
+            "enforce_dependencies: true\ndependencies:\n  - packs/a\n\nenforce_privacy: true\n",
+            &["packs/a"],
+            "enforce_dependencies: true\nenforce_privacy: true\n",
+        );
+        assert_removes(
+            "metadata:\n  notes: |\n    Some notes.\ndependencies:\n  # Needed for A.\n  - packs/a\n\n  # Needed for B.\n  - packs/b\nlayer: utilities\n",
+            &["packs/a", "packs/b"],
+            "metadata:\n  notes: |\n    Some notes.\nlayer: utilities\n",
+        );
+    }
+
+    #[test]
+    fn keeps_the_key_when_a_comment_is_on_it_or_directly_above_it() {
+        assert_removes(
+            "enforce_dependencies: true\ndependencies: # keep sorted\n  - packs/a\n  # Keep sorted.\n\nenforce_privacy: true\n",
+            &["packs/a"],
+            "enforce_dependencies: true\ndependencies: # keep sorted\n  # Keep sorted.\n\nenforce_privacy: true\n",
+        );
+        assert_removes(
+            "\
+# A list of this package's dependencies
+# Note that packages in this list require their own `package.yml` file
+dependencies:
+  - packs/baz
+
+# Specify which packages are allowed to see this one
+visible_to:
+  - packs/bar
+",
+            &["packs/baz"],
+            "\
+# A list of this package's dependencies
+# Note that packages in this list require their own `package.yml` file
+dependencies:
+# Specify which packages are allowed to see this one
+visible_to:
+  - packs/bar
+",
+        );
+    }
+
+    #[test]
+    fn keeps_the_key_while_a_comment_under_it_remains() {
+        assert_removes(
+            "dependencies:\n# - packs/bop  # re-add after the migration\n- packs/baz\nlayer: utilities\n",
+            &["packs/baz"],
+            "dependencies:\n# - packs/bop  # re-add after the migration\nlayer: utilities\n",
+        );
+        assert_removes(
+            "dependencies:\n- packs/baz\n# Layers, lowest first:\n# - utilities may not depend on services\nlayer: utilities\n",
+            &["packs/baz"],
+            "dependencies:\n# Layers, lowest first:\n# - utilities may not depend on services\nlayer: utilities\n",
+        );
+        assert_removes(
+            "dependencies:\n- packs/a\n# - packs/old\nlayer: utilities\n",
+            &["packs/a"],
+            "dependencies:\n# - packs/old\nlayer: utilities\n",
+        );
+        assert_removes(
+            "dependencies:\n  - packs/a\n  # - packs/old\n",
+            &["packs/a"],
+            "dependencies:\n  # - packs/old\n",
+        );
+        assert_removes(
+            "dependencies:\n- packs/a\n# Footer.\n",
+            &["packs/a"],
+            "dependencies:\n# Footer.\n",
+        );
+    }
+
+    #[test]
+    fn declines_layouts_it_cannot_edit() {
+        assert_eq!(
+            remove("dependencies: [packs/a, packs/b]\n", &["packs/b"]),
+            None
+        );
+        assert_eq!(
+            remove("dependencies:\n- \"packs/\\u0061\"\n", &["packs/a"]),
+            None
+        );
+        assert_eq!(
+            remove(
+                "dependencies: &deps # anchored\n- packs/a\n- packs/b\n",
+                &["packs/b"]
+            ),
+            None
+        );
+        assert_eq!(
+            remove(
+                "dependencies: !!seq # tagged\n- packs/a\n- packs/b\n",
+                &["packs/b"]
+            ),
+            None
+        );
+    }
+
+    #[test]
+    fn keeps_a_comment_directly_above_a_commented_out_entry() {
+        assert_removes(
+            "dependencies:\n- packs/a\n# Re-add after the migration:\n# - packs/old\n- packs/b\n",
+            &["packs/b"],
+            "dependencies:\n- packs/a\n# Re-add after the migration:\n# - packs/old\n",
+        );
+    }
+
+    #[test]
+    fn a_commented_out_entry_does_not_keep_a_comment_below_it() {
+        assert_removes(
+            "dependencies:\n- packs/a\n# - packs/old\n# Needed for B.\n- packs/b\n",
+            &["packs/b"],
+            "dependencies:\n- packs/a\n# - packs/old\n",
+        );
+    }
+
+    #[test]
+    fn removes_a_blank_line_left_at_either_end_of_the_list() {
+        assert_removes(
+            "dependencies:\n- packs/a\n\n- packs/b\n",
+            &["packs/a"],
+            "dependencies:\n- packs/b\n",
+        );
+        assert_removes(
+            "dependencies:\n- packs/a\n\n- packs/b\nlayer: utilities\n",
+            &["packs/b"],
+            "dependencies:\n- packs/a\nlayer: utilities\n",
+        );
+    }
+
+    #[test]
+    fn leaves_document_markers_alone() {
+        assert_removes(
+            "---\ndependencies:\n- packs/a\n- packs/b\n...\n",
+            &["packs/a"],
+            "---\ndependencies:\n- packs/b\n...\n",
+        );
+    }
+
+    #[test]
+    fn removing_one_at_a_time_matches_removing_together() {
+        let before = "\
+enforce_dependencies: true
+dependencies:
+# Keep sorted.
+- packs/a
+- packs/b
+# - packs/old
+# Needed for C.
+- packs/c  # trailing
+
+# Needed for D and E.
+- packs/d
+- packs/e
+# Re-add after the migration:
+# - packs/older
+- packs/f
+# About the layer.
+layer: utilities
+";
+        let all = [
+            "packs/a", "packs/b", "packs/c", "packs/d", "packs/e", "packs/f",
+        ];
+        for mask in 1..1u32 << all.len() {
+            let names: Vec<&str> = (0..all.len())
+                .filter(|bit| mask & (1 << bit) != 0)
+                .map(|bit| all[bit])
+                .collect();
+            let together = remove(before, &names).unwrap();
+            for order in [names.clone(), names.iter().rev().copied().collect()]
+            {
+                let one_at_a_time =
+                    order.iter().fold(before.to_string(), |text, name| {
+                        remove(&text, &[name]).unwrap()
+                    });
+                assert_eq!(one_at_a_time, together, "removing {order:?}");
+            }
+        }
+    }
+
+    /// Every list of up to five lines drawn from an entry, a comment, a
+    /// commented-out entry and a blank line, with and without a comment
+    /// above the key.
+    #[test]
+    fn removal_order_never_matters_for_generated_layouts() {
+        const KINDS: [&str; 4] = ["item", "# note\n", "# - packs/old\n", "\n"];
+        for prefix in ["", "# Header.\n"] {
+            for len in 1..=5u32 {
+                for code in 0..4usize.pow(len) {
+                    let mut block = String::new();
+                    let mut items: Vec<String> = Vec::new();
+                    for position in 0..len {
+                        let kind = KINDS[code / 4usize.pow(position) % 4];
+                        if kind == "item" {
+                            let name = format!("packs/p{position}");
+                            block.push_str(&format!("- {name}\n"));
+                            items.push(name);
+                        } else {
+                            block.push_str(kind);
+                        }
+                    }
+                    let before = format!(
+                        "{prefix}dependencies:\n{block}layer: utilities\n"
+                    );
+                    assert_removal_order_never_matters(&before, &items);
+                }
+            }
+        }
+    }
+
+    fn assert_removal_order_never_matters(before: &str, items: &[String]) {
+        let names = |mask: u32| -> Vec<&str> {
+            (0..items.len())
+                .filter(|bit| mask & (1 << bit) != 0)
+                .map(|bit| items[bit].as_str())
+                .collect()
+        };
+        for mask in 1..1u32 << items.len() {
+            let names = names(mask);
+            let together = remove(before, &names)
+                .unwrap_or_else(|| panic!("declined {before:?}"));
+            for order in [names.clone(), names.iter().rev().copied().collect()]
+            {
+                let one_at_a_time =
+                    order.iter().fold(before.to_string(), |text, name| {
+                        remove(&text, &[name]).unwrap()
+                    });
+                assert_eq!(
+                    one_at_a_time, together,
+                    "removing {order:?} from {before:?}"
+                );
+            }
+        }
+    }
+}
diff --git a/tests/check_unused_dependencies.rs b/tests/check_unused_dependencies.rs
index 77be8bf3..b8fa9f5b 100644
--- a/tests/check_unused_dependencies.rs
+++ b/tests/check_unused_dependencies.rs
@@ -1,6 +1,6 @@
 use assert_cmd::cargo::cargo_bin_cmd;
 use predicates::prelude::*;
-use std::{error::Error, fs};
+use std::{error::Error, fs, path::Path};
 mod common;
 
 fn assert_check_unused_dependencies(cmd: &str) -> Result<(), Box<dyn Error>> {
@@ -39,15 +39,16 @@ fn assert_auto_correct_unused_dependencies(
 ) -> Result<(), Box<dyn Error>> {
     common::set_up_fixtures();
 
-    let expected_before_autocorrect = [
-        "enforce_dependencies: true",
-        "enforce_privacy: true",
-        "layer: technical_services",
-        "dependencies:",
-        "- packs/bar",
-        "- packs/baz\n",
-    ]
-    .join("\n");
+    let expected_before_autocorrect = r#"# Header comment: proves comments above the first key survive.
+enforce_dependencies: true
+enforce_privacy: true
+dependencies:
+# Comment inside the dependencies block.
+- packs/bar
+- packs/baz
+# Trailing comment, after the list and before another key.
+layer: technical_services
+"#;
     let foo_package_yml = fs::read_to_string("tests/fixtures/app_with_unnecessary_dependencies/packs/foo/package.yml").unwrap();
     assert_eq!(foo_package_yml, expected_before_autocorrect);
 
@@ -60,20 +61,54 @@ fn assert_auto_correct_unused_dependencies(
         .assert()
         .success();
 
-    let expected_autocorrect = [
-        "enforce_privacy: true",
-        "layer: technical_services",
-        "enforce_dependencies: true",
-        "dependencies:",
-        "- packs/bar\n",
-    ]
-    .join("\n");
+    // Comments in all three positions survive, and the key order is unchanged:
+    // the correction deletes the one list item and nothing else.
+    let expected_autocorrect = r#"# Header comment: proves comments above the first key survive.
+enforce_dependencies: true
+enforce_privacy: true
+dependencies:
+# Comment inside the dependencies block.
+- packs/bar
+# Trailing comment, after the list and before another key.
+layer: technical_services
+"#;
     let after_autocorrect = fs::read_to_string("tests/fixtures/app_with_unnecessary_dependencies/packs/foo/package.yml").unwrap();
     assert_eq!(after_autocorrect, expected_autocorrect);
 
+    assert_no_unused_dependencies(Path::new(
+        "tests/fixtures/app_with_unnecessary_dependencies",
+    ));
+
     Ok(())
 }
 
+fn assert_no_unused_dependencies(project_root: &Path) {
+    cargo_bin_cmd!("pks")
+        .arg("--project-root")
+        .arg(project_root)
+        .arg("check-unused-dependencies")
+        .assert()
+        .success();
+}
+
+fn auto_correct_foo_package_yml(before: &str) -> (String, String) {
+    let fixture = common::Fixture::new("app_with_unnecessary_dependencies");
+    let package_yml = fixture.path("packs/foo/package.yml");
+    fs::write(&package_yml, before).unwrap();
+
+    let output =
+        common::pks(&fixture, &["check-unused-dependencies", "--auto-correct"])
+            .success()
+            .get_output()
+            .clone();
+
+    assert_no_unused_dependencies(fixture.root());
+    (
+        fs::read_to_string(&package_yml).unwrap(),
+        String::from_utf8(output.stderr).unwrap(),
+    )
+}
+
 #[test]
 fn test_auto_correct_unnecessary_dependencies() -> Result<(), Box<dyn Error>> {
     assert_auto_correct_unused_dependencies(
@@ -103,3 +138,86 @@ fn test_check_unnecessary_dependencies_no_issue() -> Result<(), Box<dyn Error>>
         .success();
     Ok(())
 }
+
+#[test]
+fn test_auto_correct_preserves_comments_in_hand_edited_layouts() {
+    let before = "\
+# Header comment.
+enforce_dependencies: true
+enforce_privacy: true
+dependencies: # keep sorted
+  # Comment inside the dependencies block.
+  - \"packs/bar\"
+
+  - packs/baz  # unused
+ignored_dependencies:
+  # Deliberately ignored: bop would cycle.
+  - packs/bop
+";
+    let expected = "\
+# Header comment.
+enforce_dependencies: true
+enforce_privacy: true
+dependencies: # keep sorted
+  # Comment inside the dependencies block.
+  - \"packs/bar\"
+ignored_dependencies:
+  # Deliberately ignored: bop would cycle.
+  - packs/bop
+";
+
+    let (after, stderr) = auto_correct_foo_package_yml(before);
+    assert_eq!(after, expected);
+    assert_eq!(stderr, "");
+}
+
+#[test]
+fn test_auto_correct_preserves_inline_comments_on_keys_and_values() {
+    let before = "\
+enforce_dependencies: true # enforced since the split
+enforce_privacy: true # see the privacy docs
+dependencies: # keep sorted
+- packs/bar # used by Foo
+- packs/baz # unused: was used by Foo's old job
+ignored_dependencies: # cycles we accept for now
+- packs/bop # would cycle with packs/baz
+";
+    let expected = "\
+enforce_dependencies: true # enforced since the split
+enforce_privacy: true # see the privacy docs
+dependencies: # keep sorted
+- packs/bar # used by Foo
+ignored_dependencies: # cycles we accept for now
+- packs/bop # would cycle with packs/baz
+";
+
+    let (after, stderr) = auto_correct_foo_package_yml(before);
+    assert_eq!(after, expected);
+    assert_eq!(stderr, "");
+}
+
+#[test]
+fn test_auto_correct_falls_back_to_rewriting_layouts_it_cannot_edit() {
+    let before = "\
+# This comment is lost by the rewrite.
+enforce_dependencies: true
+enforce_privacy: true
+dependencies: [packs/bar, packs/baz]
+";
+
+    let (after, stderr) = auto_correct_foo_package_yml(before);
+    let expected = "\
+enforce_privacy: true
+enforce_dependencies: true
+dependencies:
+- packs/bar
+";
+    assert_eq!(after, expected);
+    assert_eq!(
+        stderr,
+        "Warning: could not edit the dependencies list in \
+         `packs/foo/package.yml` in place to remove `packs/baz`, so the file \
+         was rewritten: its comments were not preserved and its keys may be \
+         reordered.\n"
+    );
+}
diff --git a/tests/common/mod.rs b/tests/common/mod.rs
index 8fcf4485..e1090563 100644
--- a/tests/common/mod.rs
+++ b/tests/common/mod.rs
@@ -343,12 +343,15 @@ enforce_dependencies: true
     );
     let pack_yml_contents = String::from(
         "\
+# Header comment: proves comments above the first key survive.
 enforce_dependencies: true
 enforce_privacy: true
-layer: technical_services
 dependencies:
+# Comment inside the dependencies block.
 - packs/bar
 - packs/baz
+# Trailing comment, after the list and before another key.
+layer: technical_services
 ",
     );
 
diff --git a/tests/fixtures/app_with_unnecessary_dependencies/packs/foo/package.yml b/tests/fixtures/app_with_unnecessary_dependencies/packs/foo/package.yml
index bdb0edeb..ad9202cf 100644
--- a/tests/fixtures/app_with_unnecessary_dependencies/packs/foo/package.yml
+++ b/tests/fixtures/app_with_unnecessary_dependencies/packs/foo/package.yml
@@ -1,6 +1,9 @@
+# Header comment: proves comments above the first key survive.
 enforce_dependencies: true
 enforce_privacy: true
-layer: technical_services
 dependencies:
+# Comment inside the dependencies block.
 - packs/bar
 - packs/baz
+# Trailing comment, after the list and before another key.
+layer: technical_services