Add package_todo.yml format validation to validate command

src/packs/checker.rs

@@ -1,6 +1,7 @@
 // Module declarations
 mod dependency;
 pub(crate) mod layer;
+mod todo_format;
 
 mod common_test;
 mod folder_privacy;
@@ -282,6 +283,7 @@ fn validate(configuration: &Configuration) -> Vec<String> {
                 [&CheckerType::Layer]
                 .clone(),
         }),
+        Box::new(todo_format::Checker),
     ];
 
     let mut validation_errors: Vec<String> = validators

src/packs/checker/todo_format.rs

@@ -0,0 +1,130 @@
+//! Package todo format validation checker.
+//!
+//! This module provides validation to ensure that package_todo.yml files maintain
+//! the correct serialization format. This prevents issues where manual edits
+//! (such as mass search-replace operations when renaming packs) result in
+//! incorrectly formatted files that create noise when running `pks update`.
+
+use std::fs;
+use std::path::Path;
+
+use crate::packs::bin_locater;
+use crate::packs::{Configuration, PackageTodo};
+use anyhow::{anyhow, Result};
+use rayon::prelude::*;
+
+use super::ValidatorInterface;
+
+/// Validator that checks package_todo.yml files for correct serialization format.
+///
+/// This validator ensures that existing package_todo.yml files match their expected
+/// serialized format by:
+/// 1. Reading the current file content
+/// 2. Deserializing it to a PackageTodo struct
+/// 3. Re-serializing it using the standard serialization logic
+/// 4. Comparing the result with the original file content
+///
+/// If differences are found, it reports validation errors with context-aware
+/// suggestions for the correct command to run based on packs_first_mode.
+pub struct Checker;
+
+impl ValidatorInterface for Checker {
+    /// Validates that all existing package_todo.yml files are correctly formatted.
+    ///
+    /// Iterates through all packs and checks their package_todo.yml files (if they exist)
+    /// to ensure they match the expected serialization format.
+    ///
+    /// # Returns
+    /// - `None` if all files are correctly formatted
+    /// - `Some(Vec<String>)` containing error messages for incorrectly formatted files
+    fn validate(&self, configuration: &Configuration) -> Option<Vec<String>> {
+        let validation_errors: Vec<String> = configuration
+            .pack_set
+            .packs
+            .par_iter()
+            .filter_map(|pack| {
+                let package_todo_path = pack
+                    .yml
+                    .parent()
+                    .expect("Pack yml file should have a parent directory")
+                    .join("package_todo.yml");
+
+                // Skip packs that don't have package_todo.yml files
+                if !package_todo_path.exists() {
+                    return None;
+                }
+
+                validate_package_todo_format(
+                    &package_todo_path,
+                    &pack.name,
+                    configuration.packs_first_mode,
+                )
+                .err()
+                .map(|e| e.to_string())
+            })
+            .collect();
+
+        if validation_errors.is_empty() {
+            None
+        } else {
+            Some(validation_errors)
+        }
+    }
+}
+
+/// Validates the format of a single package_todo.yml file.
+///
+/// This function implements the core validation logic:
+/// 1. Reads the current file content
+/// 2. Deserializes it to ensure it's valid YAML and matches PackageTodo structure
+/// 3. Re-serializes it using the standard serialization logic
+/// 4. Compares the result with the original content
+///
+/// # Arguments
+/// * `package_todo_path` - Path to the package_todo.yml file to validate
+/// * `pack_name` - Name of the pack (used for generating the correct header)
+/// * `packs_first_mode` - Whether the project uses packs.yml (affects command suggestions)
+///
+/// # Returns
+/// * `Ok(())` if the file is correctly formatted
+/// * `Err(String)` with a descriptive error message if validation fails
+///
+/// # Common causes of validation failures
+/// - Missing `---` separator after header comments
+/// - Incorrect ordering of violations or files (should be alphabetically sorted)
+/// - Manual edits that break the standard serialization format
+/// - Wrong header comment (should match packs_first_mode setting)
+fn validate_package_todo_format(
+    package_todo_path: &Path,
+    pack_name: &str,
+    packs_first_mode: bool,
+) -> Result<()> {
+    // Read the current file content
+    let current_content = fs::read_to_string(package_todo_path)?;
+
+    // Deserialize to ensure the file is valid and can be parsed
+    let package_todo: PackageTodo = serde_yaml::from_str(&current_content)?;
+
+    // Re-serialize using the standard serialization logic to get the expected format
+    let expected_content = crate::packs::package_todo::serialize_package_todo(
+        pack_name,
+        &package_todo,
+        packs_first_mode,
+    );
+
+    // Compare the current content with the expected serialized format
+    if current_content != expected_content {
+        let command = if packs_first_mode {
+            format!("{} update", bin_locater::packs_bin_name())
+        } else {
+            "bin/packwerk update-todo".to_string()
+        };
+        return Err(anyhow!(
+            "Package todo file {} is not in the expected format. Please run `{}` to fix it.",
+            package_todo_path.display(),
+            command
+        ));
+    }
+
+    Ok(())
+}

src/packs/package_todo.rs

@@ -171,8 +171,35 @@ pub fn write_violations_to_disk(
     debug!("Finished writing violations to disk");
 }
 
-fn serialize_package_todo(
-    responsible_pack_name: &String,
+/// Serializes a PackageTodo struct into the standard package_todo.yml format.
+///
+/// This function generates the canonical serialized representation of a package_todo.yml file,
+/// including the standard header comments and properly formatted YAML content. The output
+/// format is deterministic and consistent, which is critical for format validation.
+///
+/// # Arguments
+/// * `responsible_pack_name` - The name of the pack that owns this package_todo.yml file
+/// * `package_todo` - The PackageTodo struct containing violation data to serialize
+/// * `packs_first_mode` - Whether the project uses packs.yml (affects header command)
+///
+/// # Returns
+/// A String containing the complete package_todo.yml file content, including:
+/// - Standard header comments explaining the file's purpose
+/// - Appropriate regeneration command based on packs_first_mode
+/// - YAML separator (`---`)
+/// - Sorted violation data in the standard format
+///
+/// # Format Details
+/// The serialized format ensures:
+/// - Violations are sorted alphabetically by defining pack, then constant name
+/// - Files within each violation are sorted alphabetically
+/// - Constant names are properly quoted (using a hack to work around serde_yaml limitations)
+/// - The header matches the project's configuration (packs_first_mode)
+///
+/// This function is used both for writing new package_todo.yml files and for
+/// format validation to ensure existing files match the expected format.
+pub(crate) fn serialize_package_todo(
+    responsible_pack_name: &str,
     package_todo: &PackageTodo,
     packs_first_mode: bool,
 ) -> String {
@@ -223,7 +250,7 @@ fn delete_package_todo_from_disk(responsible_pack: &Pack) {
     }
 }
 
-fn header(responsible_pack_name: &String, packs_first_mode: bool) -> String {
+fn header(responsible_pack_name: &str, packs_first_mode: bool) -> String {
     let command = if packs_first_mode {
         "pks update"
     } else {

tests/fixtures/contains_package_todo/packs/foo/package_todo.yml

@@ -5,6 +5,7 @@
 # You can regenerate this file using the following command:
 #
 # bin/packwerk update-todo
+---
 packs/bar:
   "::Bar":
     violations:

tests/fixtures/incorrectly_formatted_package_todo/package.yml

@@ -0,0 +1,2 @@
+enforce_privacy: false
+enforce_dependencies: false

tests/fixtures/incorrectly_formatted_package_todo/packs/foo/package.yml

@@ -0,0 +1,2 @@
+enforce_privacy: true
+enforce_dependencies: true

tests/fixtures/incorrectly_formatted_package_todo/packs/foo/package_todo.yml

@@ -0,0 +1,14 @@
+# This file is incorrectly formatted (not ordered properly)
+---
+packs/bar:
+  "::Baz":
+    violations:
+    - privacy
+    - dependency
+    files:
+    - packs/foo/app/services/foo.rb
+  "::Bar":
+    violations:
+    - dependency
+    files:
+    - packs/foo/app/services/foo.rb

tests/fixtures/incorrectly_formatted_package_todo/packwerk.yml

@@ -0,0 +1,6 @@
+include:
+  - "**/*.rb"
+  - "**/*.rake"
+exclude:
+  - "{bin,node_modules,script,tmp,vendor}/**/*"
+packs_first_mode: false

tests/todo_format_validation_test.rs

@@ -0,0 +1,95 @@
+//! Tests for package_todo.yml format validation.
+//!
+//! These tests verify that the `pks validate` command correctly identifies
+//! when package_todo.yml files are not in the expected serialization format
+//! and provides appropriate error messages and suggestions.
+
+use assert_cmd::prelude::*;
+use predicates::prelude::*;
+use std::{error::Error, process::Command};
+
+mod common;
+
+/// Tests that validation fails for incorrectly formatted package_todo.yml files.
+///
+/// This test uses a fixture with a package_todo.yml file that has violations in
+/// the wrong order (::Baz should come after ::Bar when sorted alphabetically).
+/// The validation should fail and suggest running the appropriate update command.
+#[test]
+fn test_validate_incorrectly_formatted_package_todo(
+) -> Result<(), Box<dyn Error>> {
+    Command::cargo_bin("pks")
+        .unwrap()
+        .arg("--project-root")
+        .arg("tests/fixtures/incorrectly_formatted_package_todo")
+        .arg("validate")
+        .assert()
+        .failure()
+        .stdout(predicate::str::contains("1 validation error(s) detected:"))
+        .stdout(predicate::str::contains("is not in the expected format"))
+        .stdout(predicate::str::contains("bin/packwerk update-todo"));
+
+    common::teardown();
+    Ok(())
+}
+
+/// Tests that validation passes for correctly formatted package_todo.yml files.
+///
+/// This test uses an existing fixture that has a properly formatted package_todo.yml
+/// file (with correct ordering, headers, and format). The validation should succeed.
+#[test]
+fn test_validate_correctly_formatted_package_todo() -> Result<(), Box<dyn Error>>
+{
+    Command::cargo_bin("pks")
+        .unwrap()
+        .arg("--project-root")
+        .arg("tests/fixtures/contains_package_todo")
+        .arg("validate")
+        .assert()
+        .success()
+        .stdout(predicate::str::contains("Packwerk validate succeeded!"));
+
+    common::teardown();
+    Ok(())
+}
+
+/// Tests that validation passes when violations exist but no package_todo.yml files.
+///
+/// This test uses a fixture with actual privacy violations but no package_todo.yml files
+/// and verifies that the todo format validator still succeeds (it only validates existing
+/// todo files, not whether violations should have todo files).
+#[test]
+fn test_validate_passes_when_violations_exist_but_no_todo_file(
+) -> Result<(), Box<dyn Error>> {
+    Command::cargo_bin("pks")
+        .unwrap()
+        .arg("--project-root")
+        .arg("tests/fixtures/privacy_violation_overrides")
+        .arg("validate")
+        .assert()
+        .success()
+        .stdout(predicate::str::contains("Packwerk validate succeeded!"));
+
+    common::teardown();
+    Ok(())
+}
+
+/// Tests that validation succeeds when there are no violations and no package_todo.yml files.
+///
+/// This test uses a clean fixture with no violations and no package_todo.yml files
+/// to verify the validator succeeds in the simplest case.
+#[test]
+fn test_validate_succeeds_when_no_violations_and_no_todo_files(
+) -> Result<(), Box<dyn Error>> {
+    Command::cargo_bin("pks")
+        .unwrap()
+        .arg("--project-root")
+        .arg("tests/fixtures/simple_app")
+        .arg("validate")
+        .assert()
+        .success()
+        .stdout(predicate::str::contains("Packwerk validate succeeded!"));
+
+    common::teardown();
+    Ok(())
+}