From 84517cabdeff3b154ff4628c0f4f34a10ac5c2ff Mon Sep 17 00:00:00 2001
From: Zach Vander Velden <zvv@google.com>
Date: Fri, 23 Aug 2024 15:18:56 +0000
Subject: [PATCH 1/6] Add "changelog" xtask script to automate updating
 CHANGELOG.md files.

---
 crates/xtask/Cargo.lock       |   1 +
 crates/xtask/Cargo.toml       |   1 +
 crates/xtask/src/changelog.rs | 332 ++++++++++++++++++++++++++++++++++
 crates/xtask/src/main.rs      |  16 ++
 4 files changed, 350 insertions(+)
 create mode 100644 crates/xtask/src/changelog.rs

diff --git a/crates/xtask/Cargo.lock b/crates/xtask/Cargo.lock
index b6a31fe4..1a971028 100644
--- a/crates/xtask/Cargo.lock
+++ b/crates/xtask/Cargo.lock
@@ -1959,6 +1959,7 @@ dependencies = [
  "log",
  "probe-rs",
  "rustc-demangle",
+ "semver",
  "serde",
  "stack-sizes",
  "wasefire-cli-tools",
diff --git a/crates/xtask/Cargo.toml b/crates/xtask/Cargo.toml
index ed3cf93f..dd51528b 100644
--- a/crates/xtask/Cargo.toml
+++ b/crates/xtask/Cargo.toml
@@ -14,6 +14,7 @@ lazy_static = "1.4.0"
 log = "0.4.21"
 probe-rs = "0.24.0"
 rustc-demangle = "0.1.24"
+semver = "1.0.23"
 serde = { version = "1.0.202", features = ["derive"] }
 stack-sizes = "0.5.0"
 wasefire-cli-tools = { path = "../cli-tools" }
diff --git a/crates/xtask/src/changelog.rs b/crates/xtask/src/changelog.rs
new file mode 100644
index 00000000..7d551390
--- /dev/null
+++ b/crates/xtask/src/changelog.rs
@@ -0,0 +1,332 @@
+// Copyright 2024 Google LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+use anyhow::{ensure, Result};
+use clap::ValueEnum;
+use semver::Version;
+
+#[derive(Debug, Clone, clap::ValueEnum)]
+pub enum ReleaseType {
+    Major,
+    Minor,
+    Patch,
+}
+
+#[derive(Debug, Default, PartialEq, Eq)]
+struct Changelog {
+    releases: Vec<Release>,
+
+    /// Incremented to acknowledge the changelog is already up-to-date.
+    ///
+    /// There is a CI test to make sure the changelog is updated when the crate is modified. Some
+    /// modifications are already documented in the changelog, so the test will have a false
+    /// positive. This counter is used to artificially modify the changelog and explicitly
+    /// acknowledge that it's already up-to-date.
+    skip_counter: Option<u32>,
+}
+
+impl Changelog {
+    fn read_file(crate_path: &str) -> Result<Changelog> {
+        let changelog_path = format!("{crate_path}/CHANGELOG.md");
+
+        let changelog_contents = wasefire_cli_tools::fs::read(&changelog_path)?;
+
+        Self::parse(String::from_utf8(changelog_contents)?)
+    }
+
+    /// Converts raw file contents into a Changelog data structure.
+    fn parse(contents: String) -> Result<Changelog> {
+        let mut skip_counter = None;
+        let mut releases = Vec::new();
+
+        // Trim off the trailing comment: <!-- Increment to skip ... -->
+        let contents: Vec<&str> =
+            contents.split("\n\n<!-- Increment to skip CHANGELOG.md test:").collect();
+
+        // Populate skip_counter if we have one.
+        if contents.len() > 1 {
+            let ending = contents[1]
+                .trim()
+                .strip_suffix("-->")
+                .expect("Malformed 'Increment to skip CHANGELOG.md test'")
+                .trim();
+
+            skip_counter = ending.parse::<u32>().ok();
+        }
+
+        let contents = contents[0];
+
+        // Split by release.
+        // Skip(1) to skip the prefix.
+        for each_release in contents.split("\n## ").skip(1) {
+            let mut version_iter = each_release.split("\n### ").into_iter();
+            let release_string = version_iter.next().expect("No release version detected.").trim();
+            let mut release = Release::from(release_string);
+
+            // Each 'Major', 'Minor', 'Patch' section within this release.
+            for each_version in version_iter {
+                let version_parts: Vec<_> = each_version.split_terminator("\n\n").collect();
+
+                ensure!(version_parts.len() == 2, "Failed to parse release: {}", each_version);
+
+                let version_type = ReleaseType::from_str(version_parts[0], true).expect(
+                    format!(
+                        "Failed to parse version_type: {}. Must be 'Major', 'Minor', or 'Patch'.",
+                        version_parts[0]
+                    )
+                    .as_str(),
+                );
+                let version_contents = version_parts[1]
+                    .split("- ")
+                    .map(|str| str.trim().to_string())
+                    .filter(|str| !str.is_empty())
+                    .collect();
+
+                match version_type {
+                    ReleaseType::Major => release.major = version_contents,
+                    ReleaseType::Minor => release.minor = version_contents,
+                    ReleaseType::Patch => release.patch = version_contents,
+                }
+            }
+
+            releases.push(release);
+        }
+
+        Ok(Changelog { releases, skip_counter })
+    }
+}
+
+#[derive(Debug, PartialEq, Eq)]
+struct Release {
+    version: Version,
+
+    major: Vec<String>,
+    minor: Vec<String>,
+    patch: Vec<String>,
+}
+
+impl Release {
+    fn from(version: &str) -> Self {
+        let semver = Version::parse(version).expect("Invalid string version format.");
+        Release { version: semver, major: Vec::new(), minor: Vec::new(), patch: Vec::new() }
+    }
+}
+
+pub fn execute(crate_path: &str, _version: &ReleaseType, _message: &str) -> Result<()> {
+    ensure!(wasefire_cli_tools::fs::exists(crate_path), "Crate does not exist: {}", crate_path);
+
+    let _changelog = Changelog::read_file(crate_path)?;
+    // TODO: act upon changelog...
+
+    Ok(())
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn parse_changelog_success() {
+        let changelog = r"# Changelog
+
+## 0.2.0
+
+### Major
+
+- major update 1
+- major update 2
+
+### Minor
+
+- minor update 1
+- minor update 2
+
+### Patch
+
+- patch update 1
+- patch update 2
+
+## 0.1.0
+
+### Major
+
+- major update 1
+- major update 2
+
+### Minor
+
+- minor update 1
+- minor update 2
+
+### Patch
+
+- patch update 1
+- patch update 2
+";
+
+        assert_eq!(
+            Changelog::parse(changelog.to_string()).expect("Failed to parse changelog."),
+            Changelog {
+                releases: vec![
+                    Release {
+                        version: Version::parse("0.2.0").unwrap(),
+                        major: vec!["major update 1".to_string(), "major update 2".to_string()],
+                        minor: vec!["minor update 1".to_string(), "minor update 2".to_string()],
+                        patch: vec!["patch update 1".to_string(), "patch update 2".to_string()],
+                    },
+                    Release {
+                        version: Version::parse("0.1.0").unwrap(),
+                        major: vec!["major update 1".to_string(), "major update 2".to_string()],
+                        minor: vec!["minor update 1".to_string(), "minor update 2".to_string()],
+                        patch: vec!["patch update 1".to_string(), "patch update 2".to_string()],
+                    }
+                ],
+                skip_counter: None,
+            }
+        );
+    }
+
+    #[test]
+    fn parse_changelog_with_missing_release_type_success() {
+        let changelog = r"# Changelog
+
+## 0.2.0
+
+### Major
+
+- major update 1
+- major update 2
+
+## 0.1.0
+
+### Minor
+
+- minor update 1
+- minor update 2
+
+## 0.0.1
+
+### Patch
+
+- patch update 1
+- patch update 2
+";
+
+        assert_eq!(
+            Changelog::parse(changelog.to_string()).expect("Failed to parse changelog."),
+            Changelog {
+                releases: vec![
+                    Release {
+                        version: Version::parse("0.2.0").unwrap(),
+                        major: vec!["major update 1".to_string(), "major update 2".to_string()],
+                        minor: Vec::new(),
+                        patch: Vec::new(),
+                    },
+                    Release {
+                        version: Version::parse("0.1.0").unwrap(),
+                        major: Vec::new(),
+                        minor: vec!["minor update 1".to_string(), "minor update 2".to_string()],
+                        patch: Vec::new(),
+                    },
+                    Release {
+                        version: Version::parse("0.0.1").unwrap(),
+                        major: Vec::new(),
+                        minor: Vec::new(),
+                        patch: vec!["patch update 1".to_string(), "patch update 2".to_string()],
+                    }
+                ],
+                skip_counter: None,
+            }
+        );
+    }
+
+    #[test]
+    fn parse_changelog_handles_multi_line_description() {
+        let changelog = r"# Changelog
+
+## 0.1.0
+
+### Major
+
+- short 1
+- my long description
+  that spans many lines.
+- short 2
+
+";
+
+        assert_eq!(
+            Changelog::parse(changelog.to_string()).expect("Failed to parse changelog."),
+            Changelog {
+                releases: vec![Release {
+                    version: Version::parse("0.1.0").unwrap(),
+                    major: vec![
+                        "short 1".to_string(),
+                        "my long description\n  that spans many lines.".to_string(),
+                        "short 2".to_string()
+                    ],
+                    minor: Vec::new(),
+                    patch: Vec::new(),
+                }],
+                skip_counter: None,
+            }
+        );
+    }
+
+    #[test]
+    fn parse_changelog_removes_prefix() {
+        let changelog = r"# Changelog
+
+## 0.1.0
+";
+
+        assert_eq!(
+            Changelog::parse(changelog.to_string()).expect("Failed to parse changelog."),
+            Changelog {
+                releases: vec![Release {
+                    version: Version::parse("0.1.0").unwrap(),
+                    major: Vec::new(),
+                    minor: Vec::new(),
+                    patch: Vec::new(),
+                }],
+                skip_counter: None,
+            }
+        );
+    }
+
+    #[test]
+    fn parse_changelog_handles_increment_comment_at_end() {
+        let changelog = r"# Changelog
+## 0.1.0
+
+### Major
+
+- A change
+
+<!-- Increment to skip CHANGELOG.md test: 5 -->";
+
+        assert_eq!(
+            Changelog::parse(changelog.to_string()).expect("Failed to parse changelog."),
+            Changelog {
+                releases: vec![Release {
+                    version: Version::parse("0.1.0").unwrap(),
+                    major: vec!["A change".to_string()],
+                    minor: Vec::new(),
+                    patch: Vec::new(),
+                }],
+                skip_counter: Some(5),
+            }
+        );
+    }
+}
diff --git a/crates/xtask/src/main.rs b/crates/xtask/src/main.rs
index d18f3af8..209a4afc 100644
--- a/crates/xtask/src/main.rs
+++ b/crates/xtask/src/main.rs
@@ -26,6 +26,7 @@ use probe_rs::{flashing, Permissions, Session};
 use rustc_demangle::demangle;
 use wasefire_cli_tools::{action, cmd, fs};
 
+mod changelog;
 mod footprint;
 mod lazy;
 mod textreview;
@@ -94,6 +95,18 @@ enum MainCommand {
 
     /// Ensures review can be done in printed form.
     Textreview,
+
+    /// Updates change log of a crate and all dependent crates.
+    Changelog {
+        /// Crate to update suchas "crate/board".
+        crate_path: String,
+
+        // Either "major", "minor", or "patch".
+        version: changelog::ReleaseType,
+
+        /// Message added to the CHANGELOG.md.
+        message: String,
+    },
 }
 
 #[derive(clap::Args)]
@@ -222,6 +235,9 @@ impl Flags {
             MainCommand::Runner(runner) => runner.execute(&self.options),
             MainCommand::Footprint { output } => footprint::compare(&output),
             MainCommand::Textreview => textreview::execute(),
+            MainCommand::Changelog { crate_path, version, message } => {
+                changelog::execute(&crate_path, &version, &message)
+            }
         }
     }
 }

From e584ce0c50f149e05f57d82c55cbfb72de60622a Mon Sep 17 00:00:00 2001
From: Zach Vander Velden <zvv@google.com>
Date: Fri, 23 Aug 2024 16:54:52 +0000
Subject: [PATCH 2/6] Split changelog xtask into 2 sub commands: 'ci' and
 'change'.

Next up is implement all the validation for 'ci'.
---
 crates/api-macro/CHANGELOG.md |  2 +-
 crates/cli-tools/src/fs.rs    |  8 +++++-
 crates/xtask/src/changelog.rs | 50 ++++++++++++++++++++++++++---------
 crates/xtask/src/main.rs      | 46 ++++++++++++++++++++++----------
 4 files changed, 78 insertions(+), 28 deletions(-)

diff --git a/crates/api-macro/CHANGELOG.md b/crates/api-macro/CHANGELOG.md
index d38064a6..344d584f 100644
--- a/crates/api-macro/CHANGELOG.md
+++ b/crates/api-macro/CHANGELOG.md
@@ -2,7 +2,7 @@
 
 ## 0.6.2-git
 
-## Patch
+### Patch
 
 - Update dependencies
 
diff --git a/crates/cli-tools/src/fs.rs b/crates/cli-tools/src/fs.rs
index b04d6072..3f72e84a 100644
--- a/crates/cli-tools/src/fs.rs
+++ b/crates/cli-tools/src/fs.rs
@@ -14,7 +14,7 @@
 
 //! Wrappers around `std::fs` with descriptive errors.
 
-use std::fs::Metadata;
+use std::fs::{Metadata, ReadDir};
 use std::io::{ErrorKind, Read, Write};
 use std::path::{Component, Path, PathBuf};
 
@@ -115,6 +115,12 @@ pub fn read(path: impl AsRef<Path>) -> Result<Vec<u8>> {
     std::fs::read(path.as_ref()).with_context(|| format!("reading {name}"))
 }
 
+pub fn read_dir(path: impl AsRef<Path>) -> Result<ReadDir> {
+    let dir = path.as_ref().display();
+    debug!("read_dir < {dir:?}");
+    std::fs::read_dir(path.as_ref()).with_context(|| format!("reading directory {dir}"))
+}
+
 pub fn read_toml<T: DeserializeOwned>(path: impl AsRef<Path>) -> Result<T> {
     let name = path.as_ref().display();
     let contents = read(path.as_ref())?;
diff --git a/crates/xtask/src/changelog.rs b/crates/xtask/src/changelog.rs
index 7d551390..2d98e141 100644
--- a/crates/xtask/src/changelog.rs
+++ b/crates/xtask/src/changelog.rs
@@ -12,7 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-use anyhow::{ensure, Result};
+use anyhow::{ensure, Context, Result};
 use clap::ValueEnum;
 use semver::Version;
 
@@ -37,9 +37,7 @@ struct Changelog {
 }
 
 impl Changelog {
-    fn read_file(crate_path: &str) -> Result<Changelog> {
-        let changelog_path = format!("{crate_path}/CHANGELOG.md");
-
+    fn read_file(changelog_path: &str) -> Result<Changelog> {
         let changelog_contents = wasefire_cli_tools::fs::read(&changelog_path)?;
 
         Self::parse(String::from_utf8(changelog_contents)?)
@@ -72,7 +70,7 @@ impl Changelog {
         for each_release in contents.split("\n## ").skip(1) {
             let mut version_iter = each_release.split("\n### ").into_iter();
             let release_string = version_iter.next().expect("No release version detected.").trim();
-            let mut release = Release::from(release_string);
+            let mut release = Release::from(release_string)?;
 
             // Each 'Major', 'Minor', 'Patch' section within this release.
             for each_version in version_iter {
@@ -117,21 +115,49 @@ struct Release {
 }
 
 impl Release {
-    fn from(version: &str) -> Self {
-        let semver = Version::parse(version).expect("Invalid string version format.");
-        Release { version: semver, major: Vec::new(), minor: Vec::new(), patch: Vec::new() }
+    fn from(version: &str) -> Result<Self> {
+        let semver = Version::parse(version)?;
+
+        Ok(Release { version: semver, major: Vec::new(), minor: Vec::new(), patch: Vec::new() })
     }
 }
 
-pub fn execute(crate_path: &str, _version: &ReleaseType, _message: &str) -> Result<()> {
-    ensure!(wasefire_cli_tools::fs::exists(crate_path), "Crate does not exist: {}", crate_path);
+/// Validates CHANGELOG.md files for all Wasefire crates.
+pub fn execute_ci() -> Result<()> {
+    let dir_entries = wasefire_cli_tools::fs::read_dir("crates")?;
+
+    let existing_changelog_paths = dir_entries.filter_map(|entry| {
+        if let Ok(crate_path) = entry {
+            if crate_path.metadata().is_ok_and(|metadata| metadata.is_dir()) {
+                let changelog_file_path = format!("{}/CHANGELOG.md", crate_path.path().display());
+
+                if wasefire_cli_tools::fs::exists(&changelog_file_path) {
+                    return Some(changelog_file_path);
+                }
+            }
+        }
 
-    let _changelog = Changelog::read_file(crate_path)?;
-    // TODO: act upon changelog...
+        None
+    });
+
+    for changelog_path in existing_changelog_paths {
+        // Validation done during parsing.
+        Changelog::read_file(&changelog_path)
+            .with_context(|| format!("validating changelog file: {changelog_path}"))?;
+    }
 
     Ok(())
 }
 
+/// Updates a CHANGELOG.md file and CHANGELOG.md files of dependencies.
+pub fn execute_change(crate_path: &str, _version: &ReleaseType, _message: &str) -> Result<()> {
+    ensure!(wasefire_cli_tools::fs::exists(crate_path), "Crate does not exist: {}", crate_path);
+
+    let _changelog = Changelog::read_file(format!("{crate_path}/CHANGELOG.md").as_str())?;
+
+    todo!("Implement changelog updates");
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
diff --git a/crates/xtask/src/main.rs b/crates/xtask/src/main.rs
index 209a4afc..a0c01661 100644
--- a/crates/xtask/src/main.rs
+++ b/crates/xtask/src/main.rs
@@ -96,17 +96,8 @@ enum MainCommand {
     /// Ensures review can be done in printed form.
     Textreview,
 
-    /// Updates change log of a crate and all dependent crates.
-    Changelog {
-        /// Crate to update suchas "crate/board".
-        crate_path: String,
-
-        // Either "major", "minor", or "patch".
-        version: changelog::ReleaseType,
-
-        /// Message added to the CHANGELOG.md.
-        message: String,
-    },
+    /// Performs a changelog operation.
+    Changelog(Changelog),
 }
 
 #[derive(clap::Args)]
@@ -228,6 +219,30 @@ struct RunnerOptions {
     memory_page_count: Option<usize>,
 }
 
+#[derive(clap::Args)]
+struct Changelog {
+    #[clap(subcommand)]
+    command: ChangelogCommand,
+}
+
+#[derive(clap::Subcommand)]
+enum ChangelogCommand {
+    /// Validates all CHANGELOG.md files.
+    Ci,
+
+    /// Updates change log of a crate and all dependent crates.
+    Change {
+        /// Crate to update suchas "crate/board".
+        crate_path: String,
+
+        // Either "major", "minor", or "patch".
+        version: changelog::ReleaseType,
+
+        /// Message added to the CHANGELOG.md.
+        message: String,
+    },
+}
+
 impl Flags {
     fn execute(self) -> Result<()> {
         match self.command {
@@ -235,9 +250,12 @@ impl Flags {
             MainCommand::Runner(runner) => runner.execute(&self.options),
             MainCommand::Footprint { output } => footprint::compare(&output),
             MainCommand::Textreview => textreview::execute(),
-            MainCommand::Changelog { crate_path, version, message } => {
-                changelog::execute(&crate_path, &version, &message)
-            }
+            MainCommand::Changelog(subcommand) => match subcommand.command {
+                ChangelogCommand::Ci => changelog::execute_ci(),
+                ChangelogCommand::Change { crate_path, version, message } => {
+                    changelog::execute_change(&crate_path, &version, &message)
+                }
+            },
         }
     }
 }

From 6922aa69a9ebfbdf819079b91f33bb5e3c6fe595 Mon Sep 17 00:00:00 2001
From: Zach Vander Velden <zvv@google.com>
Date: Fri, 23 Aug 2024 18:50:04 +0000
Subject: [PATCH 3/6] add more changelog validation.

---
 crates/xtask/src/changelog.rs | 272 ++++++++++++++++++++++++++++------
 1 file changed, 223 insertions(+), 49 deletions(-)

diff --git a/crates/xtask/src/changelog.rs b/crates/xtask/src/changelog.rs
index 2d98e141..5a067409 100644
--- a/crates/xtask/src/changelog.rs
+++ b/crates/xtask/src/changelog.rs
@@ -12,7 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-use anyhow::{ensure, Context, Result};
+use anyhow::{anyhow, ensure, Context, Result};
 use clap::ValueEnum;
 use semver::Version;
 
@@ -33,7 +33,7 @@ struct Changelog {
     /// modifications are already documented in the changelog, so the test will have a false
     /// positive. This counter is used to artificially modify the changelog and explicitly
     /// acknowledge that it's already up-to-date.
-    skip_counter: Option<u32>,
+    skip_counter: u32,
 }
 
 impl Changelog {
@@ -44,25 +44,31 @@ impl Changelog {
     }
 
     /// Converts raw file contents into a Changelog data structure.
+    ///
+    /// Validates requirements in docs/contributing/changelog.md
     fn parse(contents: String) -> Result<Changelog> {
-        let mut skip_counter = None;
-        let mut releases = Vec::new();
+        let mut releases: Vec<Release> = Vec::new();
+
+        if !contents.starts_with("# Changelog") {
+            return Err(anyhow!("H1 with 'Changelog' is required"));
+        }
 
         // Trim off the trailing comment: <!-- Increment to skip ... -->
         let contents: Vec<&str> =
             contents.split("\n\n<!-- Increment to skip CHANGELOG.md test:").collect();
 
-        // Populate skip_counter if we have one.
-        if contents.len() > 1 {
-            let ending = contents[1]
-                .trim()
-                .strip_suffix("-->")
-                .expect("Malformed 'Increment to skip CHANGELOG.md test'")
-                .trim();
-
-            skip_counter = ending.parse::<u32>().ok();
+        if contents.len() <= 1 {
+            return Err(anyhow!("skip_counter is required"));
         }
 
+        // Populate skip_counter if we have one.
+        let skip_counter = contents[1]
+            .trim()
+            .strip_suffix("-->")
+            .expect("Malformed 'Increment to skip CHANGELOG.md test'")
+            .trim()
+            .parse::<u32>()?;
+
         let contents = contents[0];
 
         // Split by release.
@@ -98,9 +104,34 @@ impl Changelog {
                 }
             }
 
+            if let Some(previous_version) = releases.last() {
+                if release.version > previous_version.version {
+                    return Err(anyhow!("Versions out of order"));
+                }
+            }
+
+            if !releases.is_empty() && !release.version.pre.is_empty() {
+                return Err(anyhow!("Only the first version can be pre-release"));
+            }
+
             releases.push(release);
         }
 
+        if let Some(last_release) = releases.last() {
+            if last_release.version.to_string() != "0.1.0" {
+                return Err(anyhow!("The last release must be version 0.1.0"));
+            }
+
+            if last_release.major.len() > 0
+                || last_release.minor.len() > 0
+                || last_release.patch.len() > 0
+            {
+                return Err(anyhow!("The last release must contain no changes"));
+            }
+        } else {
+            return Err(anyhow!("At least 1 release is required"));
+        }
+
         Ok(Changelog { releases, skip_counter })
     }
 }
@@ -166,7 +197,7 @@ mod tests {
     fn parse_changelog_success() {
         let changelog = r"# Changelog
 
-## 0.2.0
+## 0.3.0
 
 ### Major
 
@@ -183,7 +214,7 @@ mod tests {
 - patch update 1
 - patch update 2
 
-## 0.1.0
+## 0.2.0
 
 ### Major
 
@@ -199,26 +230,35 @@ mod tests {
 
 - patch update 1
 - patch update 2
-";
+
+## 0.1.0
+
+<!-- Increment to skip CHANGELOG.md test: 0 -->";
 
         assert_eq!(
             Changelog::parse(changelog.to_string()).expect("Failed to parse changelog."),
             Changelog {
                 releases: vec![
                     Release {
-                        version: Version::parse("0.2.0").unwrap(),
+                        version: Version::parse("0.3.0").unwrap(),
                         major: vec!["major update 1".to_string(), "major update 2".to_string()],
                         minor: vec!["minor update 1".to_string(), "minor update 2".to_string()],
                         patch: vec!["patch update 1".to_string(), "patch update 2".to_string()],
                     },
                     Release {
-                        version: Version::parse("0.1.0").unwrap(),
+                        version: Version::parse("0.2.0").unwrap(),
                         major: vec!["major update 1".to_string(), "major update 2".to_string()],
                         minor: vec!["minor update 1".to_string(), "minor update 2".to_string()],
                         patch: vec!["patch update 1".to_string(), "patch update 2".to_string()],
+                    },
+                    Release {
+                        version: Version::parse("0.1.0").unwrap(),
+                        major: Vec::new(),
+                        minor: Vec::new(),
+                        patch: Vec::new(),
                     }
                 ],
-                skip_counter: None,
+                skip_counter: 0,
             }
         );
     }
@@ -227,52 +267,61 @@ mod tests {
     fn parse_changelog_with_missing_release_type_success() {
         let changelog = r"# Changelog
 
-## 0.2.0
+## 0.4.0
 
 ### Major
 
 - major update 1
 - major update 2
 
-## 0.1.0
+## 0.3.0
 
 ### Minor
 
 - minor update 1
 - minor update 2
 
-## 0.0.1
+## 0.2.0
 
 ### Patch
 
 - patch update 1
 - patch update 2
-";
+
+## 0.1.0
+
+<!-- Increment to skip CHANGELOG.md test: 0 -->";
 
         assert_eq!(
             Changelog::parse(changelog.to_string()).expect("Failed to parse changelog."),
             Changelog {
                 releases: vec![
                     Release {
-                        version: Version::parse("0.2.0").unwrap(),
+                        version: Version::parse("0.4.0").unwrap(),
                         major: vec!["major update 1".to_string(), "major update 2".to_string()],
                         minor: Vec::new(),
                         patch: Vec::new(),
                     },
                     Release {
-                        version: Version::parse("0.1.0").unwrap(),
+                        version: Version::parse("0.3.0").unwrap(),
                         major: Vec::new(),
                         minor: vec!["minor update 1".to_string(), "minor update 2".to_string()],
                         patch: Vec::new(),
                     },
                     Release {
-                        version: Version::parse("0.0.1").unwrap(),
+                        version: Version::parse("0.2.0").unwrap(),
                         major: Vec::new(),
                         minor: Vec::new(),
                         patch: vec!["patch update 1".to_string(), "patch update 2".to_string()],
+                    },
+                    Release {
+                        version: Version::parse("0.1.0").unwrap(),
+                        major: Vec::new(),
+                        minor: Vec::new(),
+                        patch: Vec::new(),
                     }
                 ],
-                skip_counter: None,
+                skip_counter: 0,
             }
         );
     }
@@ -281,7 +330,7 @@ mod tests {
     fn parse_changelog_handles_multi_line_description() {
         let changelog = r"# Changelog
 
-## 0.1.0
+## 0.2.0
 
 ### Major
 
@@ -290,22 +339,32 @@ mod tests {
   that spans many lines.
 - short 2
 
-";
+## 0.1.0
+
+<!-- Increment to skip CHANGELOG.md test: 0 -->";
 
         assert_eq!(
             Changelog::parse(changelog.to_string()).expect("Failed to parse changelog."),
             Changelog {
-                releases: vec![Release {
-                    version: Version::parse("0.1.0").unwrap(),
-                    major: vec![
-                        "short 1".to_string(),
-                        "my long description\n  that spans many lines.".to_string(),
-                        "short 2".to_string()
-                    ],
-                    minor: Vec::new(),
-                    patch: Vec::new(),
-                }],
-                skip_counter: None,
+                releases: vec![
+                    Release {
+                        version: Version::parse("0.2.0").unwrap(),
+                        major: vec![
+                            "short 1".to_string(),
+                            "my long description\n  that spans many lines.".to_string(),
+                            "short 2".to_string()
+                        ],
+                        minor: Vec::new(),
+                        patch: Vec::new(),
+                    },
+                    Release {
+                        version: Version::parse("0.1.0").unwrap(),
+                        major: Vec::new(),
+                        minor: Vec::new(),
+                        patch: Vec::new(),
+                    }
+                ],
+                skip_counter: 0,
             }
         );
     }
@@ -315,7 +374,8 @@ mod tests {
         let changelog = r"# Changelog
 
 ## 0.1.0
-";
+
+<!-- Increment to skip CHANGELOG.md test: 0 -->";
 
         assert_eq!(
             Changelog::parse(changelog.to_string()).expect("Failed to parse changelog."),
@@ -326,20 +386,16 @@ mod tests {
                     minor: Vec::new(),
                     patch: Vec::new(),
                 }],
-                skip_counter: None,
+                skip_counter: 0,
             }
         );
     }
 
     #[test]
-    fn parse_changelog_handles_increment_comment_at_end() {
+    fn parse_changelog_handles_skip_counter_at_end() {
         let changelog = r"# Changelog
 ## 0.1.0
 
-### Major
-
-- A change
-
 <!-- Increment to skip CHANGELOG.md test: 5 -->";
 
         assert_eq!(
@@ -347,12 +403,130 @@ mod tests {
             Changelog {
                 releases: vec![Release {
                     version: Version::parse("0.1.0").unwrap(),
-                    major: vec!["A change".to_string()],
+                    major: Vec::new(),
                     minor: Vec::new(),
                     patch: Vec::new(),
                 }],
-                skip_counter: Some(5),
+                skip_counter: 5,
             }
         );
     }
+
+    #[test]
+    fn parse_changelog_requires_skip_counter_at_end() {
+        let changelog = r"# Changelog
+## 0.1.0";
+
+        assert!(Changelog::parse(changelog.to_string())
+            .expect_err("Parse changelog was successful.")
+            .to_string()
+            .contains("skip_counter is required"))
+    }
+
+    #[test]
+    fn parse_changelog_must_start_with_h1_changelog() {
+        let changelog = r"
+## 0.1.0";
+
+        assert!(Changelog::parse(changelog.to_string())
+            .expect_err("Parse changelog was successful.")
+            .to_string()
+            .contains("H1 with 'Changelog' is required"))
+    }
+
+    #[test]
+    fn parse_changelog_versions_must_be_in_order() {
+        let changelog = r"# Changelog
+
+## 0.1.0
+
+### Major
+
+- A change
+
+## 0.2.0
+
+### Major
+
+- A change
+
+<!-- Increment to skip CHANGELOG.md test: 0 -->";
+
+        assert!(Changelog::parse(changelog.to_string())
+            .expect_err("Parse changelog was successful.")
+            .to_string()
+            .contains("Versions out of order"))
+    }
+
+    #[test]
+    fn parse_changelog_versions_requires_at_least_one_release() {
+        let changelog = r"# Changelog
+
+<!-- Increment to skip CHANGELOG.md test: 0 -->";
+
+        assert!(Changelog::parse(changelog.to_string())
+            .expect_err("Parse changelog was successful.")
+            .to_string()
+            .contains("At least 1 release is required"))
+    }
+
+    #[test]
+    fn parse_changelog_versions_last_version_must_be_0_1_0() {
+        let changelog = r"# Changelog
+
+## 0.2.0
+
+### Major
+
+- A change
+
+<!-- Increment to skip CHANGELOG.md test: 0 -->";
+
+        assert!(Changelog::parse(changelog.to_string())
+            .expect_err("Parse changelog was successful.")
+            .to_string()
+            .contains("The last release must be version 0.1.0"))
+    }
+
+    #[test]
+    fn parse_changelog_versions_last_version_must_be_empty() {
+        let changelog = r"# Changelog
+
+## 0.1.0
+
+### Major
+
+- A change
+
+<!-- Increment to skip CHANGELOG.md test: 0 -->";
+
+        assert!(Changelog::parse(changelog.to_string())
+            .expect_err("Parse changelog was successful.")
+            .to_string()
+            .contains("The last release must contain no changes"))
+    }
+
+    #[test]
+    fn parse_changelog_versions_only_first_can_be_prerelease() {
+        let changelog = r"# Changelog
+
+## 0.6.0-git
+
+### Major
+
+- A change
+
+## 0.5.0-git
+
+### Major
+
+- A change
+
+<!-- Increment to skip CHANGELOG.md test: 0 -->";
+
+        assert!(Changelog::parse(changelog.to_string())
+            .expect_err("Parse changelog was successful.")
+            .to_string()
+            .contains("Only the first version can be pre-release"))
+    }
 }

From a360f1effe9736eda4444e1ada8049fbeba29c53 Mon Sep 17 00:00:00 2001
From: Zach Vander Velden <zvv@google.com>
Date: Mon, 26 Aug 2024 16:59:59 +0000
Subject: [PATCH 4/6] improve changelog parsing logic - line by line, while
 peeking at next.

---
 crates/xtask/src/changelog.rs | 129 +++++++++++++++++++---------------
 1 file changed, 73 insertions(+), 56 deletions(-)

diff --git a/crates/xtask/src/changelog.rs b/crates/xtask/src/changelog.rs
index 5a067409..b5366016 100644
--- a/crates/xtask/src/changelog.rs
+++ b/crates/xtask/src/changelog.rs
@@ -47,74 +47,91 @@ impl Changelog {
     ///
     /// Validates requirements in docs/contributing/changelog.md
     fn parse(contents: String) -> Result<Changelog> {
+        let mut skip_counter = 0;
         let mut releases: Vec<Release> = Vec::new();
 
-        if !contents.starts_with("# Changelog") {
+        let mut lines = contents.lines().filter(|line| !line.is_empty()).peekable();
+
+        // First line is H1.
+        if lines.next() != Some("# Changelog") {
             return Err(anyhow!("H1 with 'Changelog' is required"));
         }
 
-        // Trim off the trailing comment: <!-- Increment to skip ... -->
-        let contents: Vec<&str> =
-            contents.split("\n\n<!-- Increment to skip CHANGELOG.md test:").collect();
+        while let Some(mut current_line) = lines.next() {
+            let mut next_line = lines.peek();
+
+            // Parse a whole release
+            if let Some(version_string) = current_line.strip_prefix("## ") {
+                let mut release = Release::from(version_string)?;
+
+                // Parse each release type (major, minor, patch)
+                while next_line.is_some_and(|line| line.starts_with("### ")) {
+                    // Advance
+                    current_line = lines.next().unwrap();
+                    next_line = lines.peek();
+
+                    let release_type_string = current_line.trim_start_matches("### ");
+                    let release_type = ReleaseType::from_str(release_type_string, true)
+                        .map_err(|err| anyhow!("Failed to parse version_type: {err}"))?;
+
+                    let mut release_descriptions = Vec::new();
+
+                    // Parse the release descriptions
+                    // Some lines may be a new description. Some lines may be continuation of
+                    // previous descriptions.
+                    while next_line
+                        .is_some_and(|line| line.starts_with("- ") || line.starts_with("  "))
+                    {
+                        // Advance
+                        current_line = lines.next().unwrap();
+                        next_line = lines.peek();
+
+                        // New description
+                        if current_line.starts_with("- ") {
+                            release_descriptions
+                                .push(current_line.trim_start_matches("- ").to_string());
+                        } else if current_line.starts_with("  ") {
+                            // Append to previous description
+                            let mut last = release_descriptions
+                                .pop()
+                                .expect("No last description to append to.");
+                            last.push_str(format!("\n{current_line}").as_str());
+                            release_descriptions.push(last);
+                        }
+                    }
 
-        if contents.len() <= 1 {
-            return Err(anyhow!("skip_counter is required"));
-        }
+                    match release_type {
+                        ReleaseType::Major => release.major = release_descriptions,
+                        ReleaseType::Minor => release.minor = release_descriptions,
+                        ReleaseType::Patch => release.patch = release_descriptions,
+                    }
+                }
 
-        // Populate skip_counter if we have one.
-        let skip_counter = contents[1]
-            .trim()
-            .strip_suffix("-->")
-            .expect("Malformed 'Increment to skip CHANGELOG.md test'")
-            .trim()
-            .parse::<u32>()?;
-
-        let contents = contents[0];
-
-        // Split by release.
-        // Skip(1) to skip the prefix.
-        for each_release in contents.split("\n## ").skip(1) {
-            let mut version_iter = each_release.split("\n### ").into_iter();
-            let release_string = version_iter.next().expect("No release version detected.").trim();
-            let mut release = Release::from(release_string)?;
-
-            // Each 'Major', 'Minor', 'Patch' section within this release.
-            for each_version in version_iter {
-                let version_parts: Vec<_> = each_version.split_terminator("\n\n").collect();
-
-                ensure!(version_parts.len() == 2, "Failed to parse release: {}", each_version);
-
-                let version_type = ReleaseType::from_str(version_parts[0], true).expect(
-                    format!(
-                        "Failed to parse version_type: {}. Must be 'Major', 'Minor', or 'Patch'.",
-                        version_parts[0]
-                    )
-                    .as_str(),
-                );
-                let version_contents = version_parts[1]
-                    .split("- ")
-                    .map(|str| str.trim().to_string())
-                    .filter(|str| !str.is_empty())
-                    .collect();
-
-                match version_type {
-                    ReleaseType::Major => release.major = version_contents,
-                    ReleaseType::Minor => release.minor = version_contents,
-                    ReleaseType::Patch => release.patch = version_contents,
+                // Validate descending versions.
+                if let Some(previous_version) = releases.last() {
+                    if release.version > previous_version.version {
+                        return Err(anyhow!("Versions out of order"));
+                    }
                 }
-            }
 
-            if let Some(previous_version) = releases.last() {
-                if release.version > previous_version.version {
-                    return Err(anyhow!("Versions out of order"));
+                // Validate pre-release.
+                if !releases.is_empty() && !release.version.pre.is_empty() {
+                    return Err(anyhow!("Only the first version can be pre-release"));
                 }
-            }
 
-            if !releases.is_empty() && !release.version.pre.is_empty() {
-                return Err(anyhow!("Only the first version can be pre-release"));
+                releases.push(release);
             }
 
-            releases.push(release);
+            // Hit last line. Done parsing releases. Look for skip counter.
+            if next_line.is_none() {
+                skip_counter = current_line
+                    .trim_start_matches("<!-- Increment to skip CHANGELOG.md test: ")
+                    .trim_end_matches(" -->")
+                    .parse::<u32>()
+                    .map_err(|err| {
+                        anyhow!("Invalid skip_counter at the end of the changelog ({err})")
+                    })?;
+            }
         }
 
         if let Some(last_release) = releases.last() {
@@ -420,7 +437,7 @@ mod tests {
         assert!(Changelog::parse(changelog.to_string())
             .expect_err("Parse changelog was successful.")
             .to_string()
-            .contains("skip_counter is required"))
+            .contains("Invalid skip_counter at the end of the changelog"))
     }
 
     #[test]

From 6c717be45131bc237c47b20fb632b9fed93fb7e6 Mon Sep 17 00:00:00 2001
From: Zach Vander Velden <zvv@google.com>
Date: Thu, 29 Aug 2024 21:22:27 +0000
Subject: [PATCH 5/6] lots of updates based on Julien's feedback.

---
 crates/cli-tools/src/fs.rs    |   4 +-
 crates/xtask/src/changelog.rs | 359 ++++++++++++++++++----------------
 crates/xtask/src/main.rs      |  18 +-
 3 files changed, 204 insertions(+), 177 deletions(-)

diff --git a/crates/cli-tools/src/fs.rs b/crates/cli-tools/src/fs.rs
index 3f72e84a..30e11baf 100644
--- a/crates/cli-tools/src/fs.rs
+++ b/crates/cli-tools/src/fs.rs
@@ -117,8 +117,8 @@ pub fn read(path: impl AsRef<Path>) -> Result<Vec<u8>> {
 
 pub fn read_dir(path: impl AsRef<Path>) -> Result<ReadDir> {
     let dir = path.as_ref().display();
-    debug!("read_dir < {dir:?}");
-    std::fs::read_dir(path.as_ref()).with_context(|| format!("reading directory {dir}"))
+    debug!("walk < {dir:?}");
+    std::fs::read_dir(path.as_ref()).with_context(|| format!("walking {dir}"))
 }
 
 pub fn read_toml<T: DeserializeOwned>(path: impl AsRef<Path>) -> Result<T> {
diff --git a/crates/xtask/src/changelog.rs b/crates/xtask/src/changelog.rs
index b5366016..dc9e6988 100644
--- a/crates/xtask/src/changelog.rs
+++ b/crates/xtask/src/changelog.rs
@@ -12,11 +12,17 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-use anyhow::{anyhow, ensure, Context, Result};
-use clap::ValueEnum;
+use core::str;
+use std::collections::BTreeMap;
+use std::fmt::Write;
+use std::io::BufRead;
+use std::process::Command;
+
+use anyhow::{anyhow, bail, ensure, Context, Result};
 use semver::Version;
+use wasefire_cli_tools::{cmd, fs};
 
-#[derive(Debug, Clone, clap::ValueEnum)]
+#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, clap::ValueEnum)]
 pub enum ReleaseType {
     Major,
     Minor,
@@ -37,118 +43,117 @@ struct Changelog {
 }
 
 impl Changelog {
-    fn read_file(changelog_path: &str) -> Result<Changelog> {
-        let changelog_contents = wasefire_cli_tools::fs::read(&changelog_path)?;
-
-        Self::parse(String::from_utf8(changelog_contents)?)
+    fn read_file(path: &str) -> Result<Changelog> {
+        Self::parse(String::from_utf8(fs::read(&path)?)?.as_str())
     }
 
     /// Converts raw file contents into a Changelog data structure.
     ///
-    /// Validates requirements in docs/contributing/changelog.md
-    fn parse(contents: String) -> Result<Changelog> {
-        let mut skip_counter = 0;
+    /// Validates requirements in docs/contributing/changelog.md.
+    fn parse(contents: &str) -> Result<Changelog> {
+        let mut skip_counter: u32 = 0;
         let mut releases: Vec<Release> = Vec::new();
 
         let mut lines = contents.lines().filter(|line| !line.is_empty()).peekable();
 
         // First line is H1.
-        if lines.next() != Some("# Changelog") {
-            return Err(anyhow!("H1 with 'Changelog' is required"));
-        }
+        ensure!(lines.next() == Some("# Changelog"), "H1 with 'Changelog' is required");
 
         while let Some(mut current_line) = lines.next() {
             let mut next_line = lines.peek();
 
-            // Parse a whole release
-            if let Some(version_string) = current_line.strip_prefix("## ") {
-                let mut release = Release::from(version_string)?;
+            // Hit last line. Done parsing releases. Look for skip counter.
+            if next_line.is_none() {
+                skip_counter = current_line
+                    .strip_prefix("<!-- Increment to skip CHANGELOG.md test: ")
+                    .ok_or_else(|| anyhow!("Malformed or missing skip counter: {current_line}"))?
+                    .strip_suffix(" -->")
+                    .ok_or_else(|| anyhow!("Malformed or missing skip counter: {current_line}"))?
+                    .parse::<u32>()?;
+                break;
+            }
 
-                // Parse each release type (major, minor, patch)
-                while next_line.is_some_and(|line| line.starts_with("### ")) {
+            let mut release = match current_line.strip_prefix("## ") {
+                Some(version_string) => Release::new(version_string)?,
+                None => bail!("Failed to find release starting with '## ': {current_line}"),
+            };
+
+            // Parse each release type (major, minor, patch)
+            while next_line.is_some_and(|line| line.starts_with("### ")) {
+                // Advance
+                current_line = lines.next().unwrap();
+                next_line = lines.peek();
+
+                let release_type_string = match current_line.strip_prefix("### ") {
+                    Some(l) => l,
+                    _ => bail!("Failed to parse version header {current_line}"),
+                };
+                let release_type = match release_type_string {
+                    "Major" => ReleaseType::Major,
+                    "Minor" => ReleaseType::Minor,
+                    "Patch" => ReleaseType::Patch,
+                    _ => bail!("Failed to parse version header {current_line}"),
+                };
+
+                let mut release_descriptions = Vec::new();
+
+                // Parse the release descriptions
+                // Some lines may be a new description. Some lines may be continuation of
+                // previous descriptions.
+                while next_line.is_some_and(|line| line.starts_with("- ") || line.starts_with("  "))
+                {
                     // Advance
                     current_line = lines.next().unwrap();
                     next_line = lines.peek();
 
-                    let release_type_string = current_line.trim_start_matches("### ");
-                    let release_type = ReleaseType::from_str(release_type_string, true)
-                        .map_err(|err| anyhow!("Failed to parse version_type: {err}"))?;
-
-                    let mut release_descriptions = Vec::new();
-
-                    // Parse the release descriptions
-                    // Some lines may be a new description. Some lines may be continuation of
-                    // previous descriptions.
-                    while next_line
-                        .is_some_and(|line| line.starts_with("- ") || line.starts_with("  "))
-                    {
-                        // Advance
-                        current_line = lines.next().unwrap();
-                        next_line = lines.peek();
-
-                        // New description
-                        if current_line.starts_with("- ") {
-                            release_descriptions
-                                .push(current_line.trim_start_matches("- ").to_string());
-                        } else if current_line.starts_with("  ") {
-                            // Append to previous description
-                            let mut last = release_descriptions
-                                .pop()
-                                .expect("No last description to append to.");
-                            last.push_str(format!("\n{current_line}").as_str());
-                            release_descriptions.push(last);
-                        }
-                    }
-
-                    match release_type {
-                        ReleaseType::Major => release.major = release_descriptions,
-                        ReleaseType::Minor => release.minor = release_descriptions,
-                        ReleaseType::Patch => release.patch = release_descriptions,
-                    }
-                }
-
-                // Validate descending versions.
-                if let Some(previous_version) = releases.last() {
-                    if release.version > previous_version.version {
-                        return Err(anyhow!("Versions out of order"));
+                    ensure!(
+                        !current_line.ends_with("."),
+                        "Each description must not end in a '.' ({current_line})"
+                    );
+
+                    // New description
+                    if let Some(l) = current_line.strip_prefix("- ") {
+                        release_descriptions.push(l.to_string());
+                    } else if current_line.starts_with("  ") {
+                        // Append to previous description
+                        write!(
+                            release_descriptions.last_mut().context("Invalid continuation")?,
+                            "\n{current_line}"
+                        )?;
                     }
                 }
 
-                // Validate pre-release.
-                if !releases.is_empty() && !release.version.pre.is_empty() {
-                    return Err(anyhow!("Only the first version can be pre-release"));
-                }
-
-                releases.push(release);
+                ensure!(
+                    release.contents.insert(release_type, release_descriptions).is_none(),
+                    "Duplicate release type detected for {release_type:?}"
+                );
             }
 
-            // Hit last line. Done parsing releases. Look for skip counter.
-            if next_line.is_none() {
-                skip_counter = current_line
-                    .trim_start_matches("<!-- Increment to skip CHANGELOG.md test: ")
-                    .trim_end_matches(" -->")
-                    .parse::<u32>()
-                    .map_err(|err| {
-                        anyhow!("Invalid skip_counter at the end of the changelog ({err})")
-                    })?;
+            // Validate descending versions.
+            if let Some(previous_version) = releases.last() {
+                ensure!(
+                    release.version < previous_version.version,
+                    "Versions should be order by decreasing precedence"
+                );
             }
-        }
 
-        if let Some(last_release) = releases.last() {
-            if last_release.version.to_string() != "0.1.0" {
-                return Err(anyhow!("The last release must be version 0.1.0"));
-            }
+            // Validate pre-release.
+            ensure!(
+                releases.is_empty() || release.version.pre.is_empty(),
+                "Only the first version can be pre-release"
+            );
 
-            if last_release.major.len() > 0
-                || last_release.minor.len() > 0
-                || last_release.patch.len() > 0
-            {
-                return Err(anyhow!("The last release must contain no changes"));
-            }
-        } else {
-            return Err(anyhow!("At least 1 release is required"));
+            releases.push(release);
         }
 
+        let last_release = releases.last().context("At least 1 release is required")?;
+
+        ensure!(
+            last_release.version.to_string() == "0.1.0",
+            "The first release must be version 0.1.0"
+        );
+        ensure!(last_release.contents.is_empty(), "The last release must contain no changes");
+
         Ok(Changelog { releases, skip_counter })
     }
 }
@@ -157,49 +162,35 @@ impl Changelog {
 struct Release {
     version: Version,
 
-    major: Vec<String>,
-    minor: Vec<String>,
-    patch: Vec<String>,
+    contents: BTreeMap<ReleaseType, Vec<String>>,
 }
 
 impl Release {
-    fn from(version: &str) -> Result<Self> {
-        let semver = Version::parse(version)?;
-
-        Ok(Release { version: semver, major: Vec::new(), minor: Vec::new(), patch: Vec::new() })
+    fn new(version: &str) -> Result<Self> {
+        Ok(Release { version: Version::parse(version)?, contents: BTreeMap::new() })
     }
 }
 
 /// Validates CHANGELOG.md files for all Wasefire crates.
 pub fn execute_ci() -> Result<()> {
-    let dir_entries = wasefire_cli_tools::fs::read_dir("crates")?;
-
-    let existing_changelog_paths = dir_entries.filter_map(|entry| {
-        if let Ok(crate_path) = entry {
-            if crate_path.metadata().is_ok_and(|metadata| metadata.is_dir()) {
-                let changelog_file_path = format!("{}/CHANGELOG.md", crate_path.path().display());
-
-                if wasefire_cli_tools::fs::exists(&changelog_file_path) {
-                    return Some(changelog_file_path);
-                }
-            }
-        }
+    let paths = cmd::output(Command::new("git").args(["ls-files", "*/CHANGELOG.md"]))?;
 
-        None
-    });
+    for path in paths.stdout.lines() {
+        let path = path?;
 
-    for changelog_path in existing_changelog_paths {
         // Validation done during parsing.
-        Changelog::read_file(&changelog_path)
-            .with_context(|| format!("validating changelog file: {changelog_path}"))?;
+        Changelog::read_file(&path)
+            .with_context(|| format!("validating changelog file: {path}"))?;
     }
 
     Ok(())
 }
 
 /// Updates a CHANGELOG.md file and CHANGELOG.md files of dependencies.
-pub fn execute_change(crate_path: &str, _version: &ReleaseType, _message: &str) -> Result<()> {
-    ensure!(wasefire_cli_tools::fs::exists(crate_path), "Crate does not exist: {}", crate_path);
+pub fn execute_change(
+    crate_path: &str, _release_type: &ReleaseType, _description: &str,
+) -> Result<()> {
+    ensure!(fs::exists(crate_path), "Crate does not exist: {}", crate_path);
 
     let _changelog = Changelog::read_file(format!("{crate_path}/CHANGELOG.md").as_str())?;
 
@@ -253,26 +244,46 @@ mod tests {
 <!-- Increment to skip CHANGELOG.md test: 0 -->";
 
         assert_eq!(
-            Changelog::parse(changelog.to_string()).expect("Failed to parse changelog."),
+            Changelog::parse(changelog).expect("Failed to parse changelog."),
             Changelog {
                 releases: vec![
                     Release {
                         version: Version::parse("0.3.0").unwrap(),
-                        major: vec!["major update 1".to_string(), "major update 2".to_string()],
-                        minor: vec!["minor update 1".to_string(), "minor update 2".to_string()],
-                        patch: vec!["patch update 1".to_string(), "patch update 2".to_string()],
+                        contents: BTreeMap::from([
+                            (
+                                ReleaseType::Major,
+                                vec!["major update 1".to_string(), "major update 2".to_string()]
+                            ),
+                            (
+                                ReleaseType::Minor,
+                                vec!["minor update 1".to_string(), "minor update 2".to_string()]
+                            ),
+                            (
+                                ReleaseType::Patch,
+                                vec!["patch update 1".to_string(), "patch update 2".to_string()]
+                            )
+                        ]),
                     },
                     Release {
                         version: Version::parse("0.2.0").unwrap(),
-                        major: vec!["major update 1".to_string(), "major update 2".to_string()],
-                        minor: vec!["minor update 1".to_string(), "minor update 2".to_string()],
-                        patch: vec!["patch update 1".to_string(), "patch update 2".to_string()],
+                        contents: BTreeMap::from([
+                            (
+                                ReleaseType::Major,
+                                vec!["major update 1".to_string(), "major update 2".to_string()]
+                            ),
+                            (
+                                ReleaseType::Minor,
+                                vec!["minor update 1".to_string(), "minor update 2".to_string()]
+                            ),
+                            (
+                                ReleaseType::Patch,
+                                vec!["patch update 1".to_string(), "patch update 2".to_string()]
+                            )
+                        ]),
                     },
                     Release {
                         version: Version::parse("0.1.0").unwrap(),
-                        major: Vec::new(),
-                        minor: Vec::new(),
-                        patch: Vec::new(),
+                        contents: BTreeMap::new(),
                     }
                 ],
                 skip_counter: 0,
@@ -310,32 +321,33 @@ mod tests {
 <!-- Increment to skip CHANGELOG.md test: 0 -->";
 
         assert_eq!(
-            Changelog::parse(changelog.to_string()).expect("Failed to parse changelog."),
+            Changelog::parse(changelog).expect("Failed to parse changelog."),
             Changelog {
                 releases: vec![
                     Release {
                         version: Version::parse("0.4.0").unwrap(),
-                        major: vec!["major update 1".to_string(), "major update 2".to_string()],
-                        minor: Vec::new(),
-                        patch: Vec::new(),
+                        contents: BTreeMap::from([(
+                            ReleaseType::Major,
+                            vec!["major update 1".to_string(), "major update 2".to_string()]
+                        )]),
                     },
                     Release {
                         version: Version::parse("0.3.0").unwrap(),
-                        major: Vec::new(),
-                        minor: vec!["minor update 1".to_string(), "minor update 2".to_string()],
-                        patch: Vec::new(),
+                        contents: BTreeMap::from([(
+                            ReleaseType::Minor,
+                            vec!["minor update 1".to_string(), "minor update 2".to_string()]
+                        )]),
                     },
                     Release {
                         version: Version::parse("0.2.0").unwrap(),
-                        major: Vec::new(),
-                        minor: Vec::new(),
-                        patch: vec!["patch update 1".to_string(), "patch update 2".to_string()],
+                        contents: BTreeMap::from([(
+                            ReleaseType::Patch,
+                            vec!["patch update 1".to_string(), "patch update 2".to_string()]
+                        )]),
                     },
                     Release {
                         version: Version::parse("0.1.0").unwrap(),
-                        major: Vec::new(),
-                        minor: Vec::new(),
-                        patch: Vec::new(),
+                        contents: BTreeMap::new(),
                     }
                 ],
                 skip_counter: 0,
@@ -353,7 +365,7 @@ mod tests {
 
 - short 1
 - my long description
-  that spans many lines.
+  that spans many lines
 - short 2
 
 ## 0.1.0
@@ -361,24 +373,23 @@ mod tests {
 <!-- Increment to skip CHANGELOG.md test: 0 -->";
 
         assert_eq!(
-            Changelog::parse(changelog.to_string()).expect("Failed to parse changelog."),
+            Changelog::parse(changelog).expect("Failed to parse changelog."),
             Changelog {
                 releases: vec![
                     Release {
                         version: Version::parse("0.2.0").unwrap(),
-                        major: vec![
-                            "short 1".to_string(),
-                            "my long description\n  that spans many lines.".to_string(),
-                            "short 2".to_string()
-                        ],
-                        minor: Vec::new(),
-                        patch: Vec::new(),
+                        contents: BTreeMap::from([(
+                            ReleaseType::Major,
+                            vec![
+                                "short 1".to_string(),
+                                "my long description\n  that spans many lines".to_string(),
+                                "short 2".to_string()
+                            ]
+                        )]),
                     },
                     Release {
                         version: Version::parse("0.1.0").unwrap(),
-                        major: Vec::new(),
-                        minor: Vec::new(),
-                        patch: Vec::new(),
+                        contents: BTreeMap::new(),
                     }
                 ],
                 skip_counter: 0,
@@ -386,6 +397,26 @@ mod tests {
         );
     }
 
+    #[test]
+    fn parse_changelog_handles_description_must_not_end_with_period() {
+        let changelog = r"# Changelog
+
+## 0.2.0
+
+### Major
+
+- short 1.
+
+## 0.1.0
+
+<!-- Increment to skip CHANGELOG.md test: 0 -->";
+
+        assert!(Changelog::parse(changelog)
+            .expect_err("Parse changelog was successful.")
+            .to_string()
+            .contains("Each description must not end in a '.' (- short 1.)"))
+    }
+
     #[test]
     fn parse_changelog_removes_prefix() {
         let changelog = r"# Changelog
@@ -395,13 +426,11 @@ mod tests {
 <!-- Increment to skip CHANGELOG.md test: 0 -->";
 
         assert_eq!(
-            Changelog::parse(changelog.to_string()).expect("Failed to parse changelog."),
+            Changelog::parse(changelog).expect("Failed to parse changelog."),
             Changelog {
                 releases: vec![Release {
                     version: Version::parse("0.1.0").unwrap(),
-                    major: Vec::new(),
-                    minor: Vec::new(),
-                    patch: Vec::new(),
+                    contents: BTreeMap::new(),
                 }],
                 skip_counter: 0,
             }
@@ -416,13 +445,11 @@ mod tests {
 <!-- Increment to skip CHANGELOG.md test: 5 -->";
 
         assert_eq!(
-            Changelog::parse(changelog.to_string()).expect("Failed to parse changelog."),
+            Changelog::parse(changelog).expect("Failed to parse changelog."),
             Changelog {
                 releases: vec![Release {
                     version: Version::parse("0.1.0").unwrap(),
-                    major: Vec::new(),
-                    minor: Vec::new(),
-                    patch: Vec::new(),
+                    contents: BTreeMap::new(),
                 }],
                 skip_counter: 5,
             }
@@ -434,10 +461,10 @@ mod tests {
         let changelog = r"# Changelog
 ## 0.1.0";
 
-        assert!(Changelog::parse(changelog.to_string())
+        assert!(Changelog::parse(changelog)
             .expect_err("Parse changelog was successful.")
             .to_string()
-            .contains("Invalid skip_counter at the end of the changelog"))
+            .contains("Malformed or missing skip counter"))
     }
 
     #[test]
@@ -445,7 +472,7 @@ mod tests {
         let changelog = r"
 ## 0.1.0";
 
-        assert!(Changelog::parse(changelog.to_string())
+        assert!(Changelog::parse(changelog)
             .expect_err("Parse changelog was successful.")
             .to_string()
             .contains("H1 with 'Changelog' is required"))
@@ -469,10 +496,10 @@ mod tests {
 
 <!-- Increment to skip CHANGELOG.md test: 0 -->";
 
-        assert!(Changelog::parse(changelog.to_string())
+        assert!(Changelog::parse(changelog)
             .expect_err("Parse changelog was successful.")
             .to_string()
-            .contains("Versions out of order"))
+            .contains("Versions should be order by decreasing precedence"))
     }
 
     #[test]
@@ -481,7 +508,7 @@ mod tests {
 
 <!-- Increment to skip CHANGELOG.md test: 0 -->";
 
-        assert!(Changelog::parse(changelog.to_string())
+        assert!(Changelog::parse(changelog)
             .expect_err("Parse changelog was successful.")
             .to_string()
             .contains("At least 1 release is required"))
@@ -499,10 +526,10 @@ mod tests {
 
 <!-- Increment to skip CHANGELOG.md test: 0 -->";
 
-        assert!(Changelog::parse(changelog.to_string())
+        assert!(Changelog::parse(changelog)
             .expect_err("Parse changelog was successful.")
             .to_string()
-            .contains("The last release must be version 0.1.0"))
+            .contains("The first release must be version 0.1.0"))
     }
 
     #[test]
@@ -517,7 +544,7 @@ mod tests {
 
 <!-- Increment to skip CHANGELOG.md test: 0 -->";
 
-        assert!(Changelog::parse(changelog.to_string())
+        assert!(Changelog::parse(changelog)
             .expect_err("Parse changelog was successful.")
             .to_string()
             .contains("The last release must contain no changes"))
@@ -541,7 +568,7 @@ mod tests {
 
 <!-- Increment to skip CHANGELOG.md test: 0 -->";
 
-        assert!(Changelog::parse(changelog.to_string())
+        assert!(Changelog::parse(changelog)
             .expect_err("Parse changelog was successful.")
             .to_string()
             .contains("Only the first version can be pre-release"))
diff --git a/crates/xtask/src/main.rs b/crates/xtask/src/main.rs
index a0c01661..de60026b 100644
--- a/crates/xtask/src/main.rs
+++ b/crates/xtask/src/main.rs
@@ -230,16 +230,16 @@ enum ChangelogCommand {
     /// Validates all CHANGELOG.md files.
     Ci,
 
-    /// Updates change log of a crate and all dependent crates.
+    /// Records a change to a crate.
     Change {
-        /// Crate to update suchas "crate/board".
-        crate_path: String,
+        /// Path to the crate that changed (e.g. `crates/board`).
+        path: String,
 
-        // Either "major", "minor", or "patch".
-        version: changelog::ReleaseType,
+        // Type of change
+        release_type: changelog::ReleaseType,
 
-        /// Message added to the CHANGELOG.md.
-        message: String,
+        /// One-line description of the change.
+        description: String,
     },
 }
 
@@ -252,8 +252,8 @@ impl Flags {
             MainCommand::Textreview => textreview::execute(),
             MainCommand::Changelog(subcommand) => match subcommand.command {
                 ChangelogCommand::Ci => changelog::execute_ci(),
-                ChangelogCommand::Change { crate_path, version, message } => {
-                    changelog::execute_change(&crate_path, &version, &message)
+                ChangelogCommand::Change { path, release_type, description } => {
+                    changelog::execute_change(&path, &release_type, &description)
                 }
             },
         }

From 10d2c628859ce9c9e06cc3551b54b0af9ed7bff8 Mon Sep 17 00:00:00 2001
From: Zach Vander Velden <zvv@google.com>
Date: Fri, 30 Aug 2024 20:39:32 +0000
Subject: [PATCH 6/6] Implement Display for Changelog. Hooked up to parsing
 validation to make sure input string == output string.

---
 crates/xtask/Cargo.lock       |  12 ++-
 crates/xtask/Cargo.toml       |   1 +
 crates/xtask/src/changelog.rs | 134 ++++++++++++++++++++++++++++++----
 3 files changed, 133 insertions(+), 14 deletions(-)

diff --git a/crates/xtask/Cargo.lock b/crates/xtask/Cargo.lock
index 1a971028..12999c97 100644
--- a/crates/xtask/Cargo.lock
+++ b/crates/xtask/Cargo.lock
@@ -795,6 +795,15 @@ dependencies = [
  "either",
 ]
 
+[[package]]
+name = "itertools"
+version = "0.13.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "413ee7dfc52ee1a4949ceeb7dbc8a33f2d6c088194d9f922fb8318faf1f01186"
+dependencies = [
+ "either",
+]
+
 [[package]]
 name = "itoa"
 version = "1.0.11"
@@ -1103,7 +1112,7 @@ dependencies = [
  "gimli",
  "hidapi",
  "ihex",
- "itertools",
+ "itertools 0.12.1",
  "jep106",
  "miniz_oxide",
  "nusb",
@@ -1955,6 +1964,7 @@ dependencies = [
  "anyhow",
  "clap",
  "env_logger",
+ "itertools 0.13.0",
  "lazy_static",
  "log",
  "probe-rs",
diff --git a/crates/xtask/Cargo.toml b/crates/xtask/Cargo.toml
index dd51528b..e87f3ec6 100644
--- a/crates/xtask/Cargo.toml
+++ b/crates/xtask/Cargo.toml
@@ -10,6 +10,7 @@ edition = "2021"
 anyhow = "1.0.86"
 clap = { version = "4.5.4", features = ["derive"] }
 env_logger = "0.11.3"
+itertools = "0.13.0"
 lazy_static = "1.4.0"
 log = "0.4.21"
 probe-rs = "0.24.0"
diff --git a/crates/xtask/src/changelog.rs b/crates/xtask/src/changelog.rs
index dc9e6988..08e40d27 100644
--- a/crates/xtask/src/changelog.rs
+++ b/crates/xtask/src/changelog.rs
@@ -14,11 +14,12 @@
 
 use core::str;
 use std::collections::BTreeMap;
-use std::fmt::Write;
+use std::fmt::{Display, Write};
 use std::io::BufRead;
 use std::process::Command;
 
 use anyhow::{anyhow, bail, ensure, Context, Result};
+use itertools::Itertools;
 use semver::Version;
 use wasefire_cli_tools::{cmd, fs};
 
@@ -29,6 +30,16 @@ pub enum ReleaseType {
     Patch,
 }
 
+impl Display for ReleaseType {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        match self {
+            Self::Major => write!(f, "Major"),
+            Self::Minor => write!(f, "Minor"),
+            Self::Patch => write!(f, "Patch"),
+        }
+    }
+}
+
 #[derive(Debug, Default, PartialEq, Eq)]
 struct Changelog {
     releases: Vec<Release>,
@@ -154,7 +165,28 @@ impl Changelog {
         );
         ensure!(last_release.contents.is_empty(), "The last release must contain no changes");
 
-        Ok(Changelog { releases, skip_counter })
+        let changelog = Changelog { releases, skip_counter };
+        let output_string = format!("{changelog}");
+
+        ensure!(
+            output_string == contents,
+            "Input string does not equal rendered changelog.\n\nIn: {contents}\n\nOut: {contents}"
+        );
+
+        Ok(changelog)
+    }
+}
+
+impl Display for Changelog {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        let releases_string = self.releases.iter().map(|release| format!("{release}")).join("\n\n");
+
+        write!(
+            f,
+            "# Changelog\n\n{releases_string}\n\n<!-- Increment to skip CHANGELOG.md test: {} \
+             -->\n",
+            self.skip_counter
+        )
     }
 }
 
@@ -171,6 +203,21 @@ impl Release {
     }
 }
 
+impl Display for Release {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        write!(f, "## {}", self.version)?;
+
+        for (release_type, descriptions) in &self.contents {
+            let description_string =
+                descriptions.iter().map(|description| format!("- {description}")).join("\n");
+
+            write!(f, "\n\n### {release_type}\n\n{description_string}")?;
+        }
+
+        Ok(())
+    }
+}
+
 /// Validates CHANGELOG.md files for all Wasefire crates.
 pub fn execute_ci() -> Result<()> {
     let paths = cmd::output(Command::new("git").args(["ls-files", "*/CHANGELOG.md"]))?;
@@ -241,7 +288,8 @@ mod tests {
 
 ## 0.1.0
 
-<!-- Increment to skip CHANGELOG.md test: 0 -->";
+<!-- Increment to skip CHANGELOG.md test: 0 -->
+";
 
         assert_eq!(
             Changelog::parse(changelog).expect("Failed to parse changelog."),
@@ -291,6 +339,55 @@ mod tests {
         );
     }
 
+    #[test]
+    fn write_changelog_success() {
+        let changelog = r"# Changelog
+
+## 0.3.0
+
+### Major
+
+- major update 1
+- major update 2
+
+### Minor
+
+- minor update 1
+- minor update 2
+
+### Patch
+
+- patch update 1
+- patch update 2
+
+## 0.2.0
+
+### Major
+
+- major update 1
+- major update 2
+
+### Minor
+
+- minor update 1
+- minor update 2
+
+### Patch
+
+- patch update 1
+- patch update 2
+
+## 0.1.0
+
+<!-- Increment to skip CHANGELOG.md test: 0 -->
+";
+
+        assert_eq!(
+            format!("{}", Changelog::parse(changelog).expect("Failed to parse changelog.")),
+            changelog
+        );
+    }
+
     #[test]
     fn parse_changelog_with_missing_release_type_success() {
         let changelog = r"# Changelog
@@ -318,7 +415,8 @@ mod tests {
 
 ## 0.1.0
 
-<!-- Increment to skip CHANGELOG.md test: 0 -->";
+<!-- Increment to skip CHANGELOG.md test: 0 -->
+";
 
         assert_eq!(
             Changelog::parse(changelog).expect("Failed to parse changelog."),
@@ -370,7 +468,8 @@ mod tests {
 
 ## 0.1.0
 
-<!-- Increment to skip CHANGELOG.md test: 0 -->";
+<!-- Increment to skip CHANGELOG.md test: 0 -->
+";
 
         assert_eq!(
             Changelog::parse(changelog).expect("Failed to parse changelog."),
@@ -409,7 +508,8 @@ mod tests {
 
 ## 0.1.0
 
-<!-- Increment to skip CHANGELOG.md test: 0 -->";
+<!-- Increment to skip CHANGELOG.md test: 0 -->
+";
 
         assert!(Changelog::parse(changelog)
             .expect_err("Parse changelog was successful.")
@@ -423,7 +523,8 @@ mod tests {
 
 ## 0.1.0
 
-<!-- Increment to skip CHANGELOG.md test: 0 -->";
+<!-- Increment to skip CHANGELOG.md test: 0 -->
+";
 
         assert_eq!(
             Changelog::parse(changelog).expect("Failed to parse changelog."),
@@ -440,9 +541,11 @@ mod tests {
     #[test]
     fn parse_changelog_handles_skip_counter_at_end() {
         let changelog = r"# Changelog
+
 ## 0.1.0
 
-<!-- Increment to skip CHANGELOG.md test: 5 -->";
+<!-- Increment to skip CHANGELOG.md test: 5 -->
+";
 
         assert_eq!(
             Changelog::parse(changelog).expect("Failed to parse changelog."),
@@ -494,7 +597,8 @@ mod tests {
 
 - A change
 
-<!-- Increment to skip CHANGELOG.md test: 0 -->";
+<!-- Increment to skip CHANGELOG.md test: 0 -->
+";
 
         assert!(Changelog::parse(changelog)
             .expect_err("Parse changelog was successful.")
@@ -506,7 +610,8 @@ mod tests {
     fn parse_changelog_versions_requires_at_least_one_release() {
         let changelog = r"# Changelog
 
-<!-- Increment to skip CHANGELOG.md test: 0 -->";
+<!-- Increment to skip CHANGELOG.md test: 0 -->
+";
 
         assert!(Changelog::parse(changelog)
             .expect_err("Parse changelog was successful.")
@@ -524,7 +629,8 @@ mod tests {
 
 - A change
 
-<!-- Increment to skip CHANGELOG.md test: 0 -->";
+<!-- Increment to skip CHANGELOG.md test: 0 -->
+";
 
         assert!(Changelog::parse(changelog)
             .expect_err("Parse changelog was successful.")
@@ -542,7 +648,8 @@ mod tests {
 
 - A change
 
-<!-- Increment to skip CHANGELOG.md test: 0 -->";
+<!-- Increment to skip CHANGELOG.md test: 0 -->
+";
 
         assert!(Changelog::parse(changelog)
             .expect_err("Parse changelog was successful.")
@@ -566,7 +673,8 @@ mod tests {
 
 - A change
 
-<!-- Increment to skip CHANGELOG.md test: 0 -->";
+<!-- Increment to skip CHANGELOG.md test: 0 -->
+";
 
         assert!(Changelog::parse(changelog)
             .expect_err("Parse changelog was successful.")