chore(changelog): address review feedback

huitseeker · huitseeker · commit 6bf083fee4b0 · 2026-03-28T05:58:32.000-04:00
diff --git a/scripts/check-changelog.sh b/scripts/check-changelog.sh
@@ -3,68 +3,6 @@ set -euo pipefail
 
 FRAGMENT_GLOB=':(glob).changes/unreleased/*.md'
 
-validate_fragment() {
-    local fragment="$1"
-    local first_delim second_delim kind="" pr_url="" crate="" body=""
-
-    if [[ ! -f "$fragment" ]]; then
-        echo "Fragment file does not exist: $fragment" >&2
-        return 1
-    fi
-
-    first_delim="$(awk 'NR == 1 && $0 == "---" { print NR; exit }' "$fragment")"
-    second_delim="$(awk 'NR > 1 && $0 == "---" { print NR; exit }' "$fragment")"
-
-    if [[ -z "$first_delim" || -z "$second_delim" ]]; then
-        echo "Fragment $fragment must start with YAML front matter delimited by '---'" >&2
-        return 1
-    fi
-
-    while IFS= read -r line; do
-        [[ -z "$line" ]] && continue
-        case "$line" in
-            kind:\ *)
-                kind="${line#kind: }"
-                ;;
-            pr:\ *)
-                pr_url="${line#pr: }"
-                ;;
-            crate:\ *)
-                crate="${line#crate: }"
-                ;;
-            *)
-                echo "Fragment $fragment has an unsupported front matter line: $line" >&2
-                return 1
-                ;;
-        esac
-    done < <(sed -n "2,$((second_delim - 1))p" "$fragment")
-
-    body="$(sed -n "$((second_delim + 1)),\$p" "$fragment" | sed '/^[[:space:]]*$/d')"
-
-    case "$kind" in
-        breaking|change|enhancement|fix) ;;
-        *)
-            echo "Fragment $fragment has invalid kind '$kind'. Expected one of: breaking, change, enhancement, fix" >&2
-            return 1
-            ;;
-    esac
-
-    if [[ ! "$pr_url" =~ ^https://github\.com/[^/]+/[^/]+/pull/[0-9]+$ ]]; then
-        echo "Fragment $fragment must include a GitHub pull request URL in 'pr:'" >&2
-        return 1
-    fi
-
-    if [[ -n "$crate" && ! "$crate" =~ ^[a-z0-9][a-z0-9-]*$ ]]; then
-        echo "Fragment $fragment has invalid crate '$crate'" >&2
-        return 1
-    fi
-
-    if [[ -z "$body" ]]; then
-        echo "Fragment $fragment must contain a non-empty summary body" >&2
-        return 1
-    fi
-}
-
 if [[ "${NO_CHANGELOG_LABEL:-false}" == "true" ]]; then
     echo "\"no changelog\" label has been set"
     exit 0
@@ -89,8 +27,9 @@ EOF
     exit 1
 fi
 
+validate_args=()
 for fragment in "${fragments[@]}"; do
-    validate_fragment "$fragment"
+    validate_args+=(--validate-fragment "$fragment")
 done
 
-echo "Validated ${#fragments[@]} changelog fragment(s)."
+./scripts/prepare-changelog-release.py "${validate_args[@]}"
diff --git a/scripts/prepare-changelog-release.py b/scripts/prepare-changelog-release.py
@@ -14,6 +14,7 @@
 UNRELEASED_DIR = ROOT / ".changes" / "unreleased"
 ARCHIVE_DIR = ROOT / ".changes" / "archive"
 SECTION_ORDER = ["enhancement", "change", "fix"]
+ALLOWED_KINDS = {"breaking", "change", "enhancement", "fix"}
 SECTION_HEADERS = {
     "enhancement": "Enhancements",
     "change": "Changes",
@@ -29,19 +30,37 @@ def parse_args() -> argparse.Namespace:
     parser = argparse.ArgumentParser(
         description="Batch changelog fragments into CHANGELOG.md and archive them."
     )
-    parser.add_argument("--version", required=True, help="Release version, e.g. 0.23.0")
+    parser.add_argument("--version", help="Release version, e.g. 0.23.0")
     parser.add_argument(
         "--date",
         default=dt.date.today().isoformat(),
         help="Release date in YYYY-MM-DD format",
     )
-    return parser.parse_args()
+    parser.add_argument(
+        "--validate-fragment",
+        action="append",
+        default=[],
+        metavar="PATH",
+        help="Validate one or more fragment files and exit without updating the changelog.",
+    )
+    args = parser.parse_args()
+    if args.validate_fragment and args.version:
+        parser.error("--validate-fragment cannot be used together with --version")
+    if not args.validate_fragment and not args.version:
+        parser.error("either --version or --validate-fragment is required")
+    return args
 
 
-def parse_fragment(path: pathlib.Path) -> dict[str, str]:
-    text = path.read_text(encoding="utf-8")
-    lines = text.splitlines()
+def normalize_front_matter_value(raw: str) -> str:
+    value = raw.strip()
+    if len(value) >= 2 and value[0] == value[-1] and value[0] in {"'", '"'}:
+        return value[1:-1]
+    if value in {"null", "~"}:
+        return ""
+    return value
+
 
+def parse_front_matter(path: pathlib.Path, lines: list[str]) -> tuple[dict[str, str], int]:
     if len(lines) < 4 or lines[0] != "---":
         raise FragmentError(f"{path} must start with YAML front matter delimited by '---'")
 
@@ -52,15 +71,29 @@ def parse_fragment(path: pathlib.Path) -> dict[str, str]:
 
     front_matter: dict[str, str] = {}
     for line in lines[1:closing]:
-        if not line.strip():
+        stripped = line.strip()
+        if not stripped or stripped.startswith("#"):
             continue
-        if ":" not in line:
+        if ":" not in stripped:
             raise FragmentError(f"{path} has invalid front matter line: {line}")
-        key, value = line.split(":", 1)
-        front_matter[key.strip()] = value.strip()
+        key, value = stripped.split(":", 1)
+        key = key.strip()
+        if not key:
+            raise FragmentError(f"{path} has invalid front matter line: {line}")
+        if key in front_matter:
+            raise FragmentError(f"{path} defines front matter key '{key}' more than once")
+        front_matter[key] = normalize_front_matter_value(value)
+
+    return front_matter, closing
+
+
+def parse_fragment(path: pathlib.Path) -> dict[str, str]:
+    text = path.read_text(encoding="utf-8")
+    lines = text.splitlines()
+    front_matter, closing = parse_front_matter(path, lines)
 
     kind = front_matter.get("kind", "")
-    if kind not in {"breaking", "change", "enhancement", "fix"}:
+    if kind not in ALLOWED_KINDS:
         raise FragmentError(f"{path} has invalid kind '{kind}'")
 
     pr_url = front_matter.get("pr", "")
@@ -105,15 +138,26 @@ def make_bullet(fragment: dict[str, str]) -> tuple[str, str]:
     return section, f"- {summary} ([#{pr_number}]({pr_url}))."
 
 
+def fragment_sort_key(fragment: dict[str, str]) -> tuple[int, str]:
+    # Keep batched changelog entries deterministic across repeated release-prep runs.
+    return fragment["pr_number"], fragment["filename"]
+
+
 def collect_fragments() -> list[dict[str, str]]:
     paths = sorted(UNRELEASED_DIR.glob("*.md"))
     if not paths:
         raise FragmentError(f"No fragments found in {UNRELEASED_DIR}")
     fragments = [parse_fragment(path) for path in paths]
-    fragments.sort(key=lambda item: (item["pr_number"], item["filename"]))
+    fragments.sort(key=fragment_sort_key)
     return fragments
 
 
+def validate_fragments(paths: list[pathlib.Path]) -> list[dict[str, str]]:
+    if not paths:
+        raise FragmentError("No fragments were provided for validation")
+    return [parse_fragment(path) for path in paths]
+
+
 def build_generated_sections(fragments: list[dict[str, str]]) -> dict[str, list[str]]:
     sections: dict[str, list[str]] = {key: [] for key in SECTION_ORDER}
     for fragment in fragments:
@@ -210,7 +254,23 @@ def update_changelog(version: str, date: str, generated: dict[str, list[str]]) -
     CHANGELOG_PATH.write_text(new_changelog, encoding="utf-8")
 
 
+def flush_archive() -> None:
+    ARCHIVE_DIR.mkdir(parents=True, exist_ok=True)
+
+    for path in ARCHIVE_DIR.iterdir():
+        if path.name.startswith("."):
+            continue
+        if path.is_symlink() or path.is_file():
+            path.unlink()
+        elif path.is_dir():
+            shutil.rmtree(path)
+        else:
+            raise FragmentError(f"Unsupported archive entry: {path}")
+
+
 def archive_fragments(version: str, fragments: list[dict[str, str]]) -> None:
+    # Keep only the current release batch under .changes/archive/.
+    flush_archive()
     destination = ARCHIVE_DIR / version
     destination.mkdir(parents=True, exist_ok=True)
 
@@ -225,6 +285,11 @@ def archive_fragments(version: str, fragments: list[dict[str, str]]) -> None:
 def main() -> int:
     args = parse_args()
     try:
+        if args.validate_fragment:
+            fragments = validate_fragments([pathlib.Path(path) for path in args.validate_fragment])
+            print(f"Validated {len(fragments)} changelog fragment(s).")
+            return 0
+
         fragments = collect_fragments()
         generated = build_generated_sections(fragments)
         update_changelog(args.version, args.date, generated)
diff --git a/scripts/tests/test_prepare_changelog_release.py b/scripts/tests/test_prepare_changelog_release.py
@@ -0,0 +1,108 @@
+from __future__ import annotations
+
+import importlib.util
+import pathlib
+import tempfile
+import unittest
+
+
+SCRIPT_PATH = pathlib.Path(__file__).resolve().parents[1] / "prepare-changelog-release.py"
+SPEC = importlib.util.spec_from_file_location("prepare_changelog_release", SCRIPT_PATH)
+assert SPEC is not None and SPEC.loader is not None
+MODULE = importlib.util.module_from_spec(SPEC)
+SPEC.loader.exec_module(MODULE)
+
+
+def write_fragment(path: pathlib.Path, front_matter: str, body: str) -> None:
+    path.write_text(f"---\n{front_matter}\n---\n\n{body}\n", encoding="utf-8")
+
+
+class PrepareChangelogReleaseTests(unittest.TestCase):
+    def test_parse_fragment_accepts_quotes_comments_and_extra_keys(self) -> None:
+        with tempfile.TemporaryDirectory() as tmp_dir:
+            fragment = pathlib.Path(tmp_dir) / "123.fix.md"
+            write_fragment(
+                fragment,
+                "\n".join(
+                    [
+                        "# generated by a helper",
+                        'kind: "fix"',
+                        "note: internal metadata",
+                        "pr: 'https://github.com/0xMiden/miden-vm/pull/123'",
+                        "crate: miden-processor",
+                    ]
+                ),
+                "Fix a user-facing behavior in one concise sentence.",
+            )
+
+            parsed = MODULE.parse_fragment(fragment)
+
+        self.assertEqual(parsed["kind"], "fix")
+        self.assertEqual(parsed["crate"], "miden-processor")
+        self.assertEqual(parsed["pr_number"], 123)
+        self.assertEqual(
+            parsed["body"], "Fix a user-facing behavior in one concise sentence."
+        )
+
+    def test_collect_fragments_sorts_by_pr_number_then_filename(self) -> None:
+        with tempfile.TemporaryDirectory() as tmp_dir:
+            unreleased = pathlib.Path(tmp_dir)
+            write_fragment(
+                unreleased / "10.fix.md",
+                "kind: fix\npr: https://github.com/0xMiden/miden-vm/pull/10",
+                "Third in sort order.",
+            )
+            write_fragment(
+                unreleased / "2.change.md",
+                "kind: change\npr: https://github.com/0xMiden/miden-vm/pull/2",
+                "First in sort order.",
+            )
+            write_fragment(
+                unreleased / "2.fix.md",
+                "kind: fix\npr: https://github.com/0xMiden/miden-vm/pull/2",
+                "Second in sort order.",
+            )
+
+            original_unreleased_dir = MODULE.UNRELEASED_DIR
+            MODULE.UNRELEASED_DIR = unreleased
+            try:
+                fragments = MODULE.collect_fragments()
+            finally:
+                MODULE.UNRELEASED_DIR = original_unreleased_dir
+
+        self.assertEqual(
+            [fragment["filename"] for fragment in fragments],
+            ["2.change.md", "2.fix.md", "10.fix.md"],
+        )
+
+    def test_archive_fragments_flushes_old_batches_and_keeps_gitkeep(self) -> None:
+        with tempfile.TemporaryDirectory() as tmp_dir:
+            root = pathlib.Path(tmp_dir)
+            archive_root = root / "archive"
+            archive_root.mkdir()
+            (archive_root / ".gitkeep").write_text("", encoding="utf-8")
+            (archive_root / "0.22.0").mkdir()
+            (archive_root / "0.22.0" / "stale.fix.md").write_text("stale", encoding="utf-8")
+            source = root / "12.fix.md"
+            write_fragment(
+                source,
+                "kind: fix\npr: https://github.com/0xMiden/miden-vm/pull/12",
+                "Archived after release prep.",
+            )
+            fragments = [MODULE.parse_fragment(source)]
+
+            original_archive_dir = MODULE.ARCHIVE_DIR
+            MODULE.ARCHIVE_DIR = archive_root
+            try:
+                MODULE.archive_fragments("0.23.0", fragments)
+            finally:
+                MODULE.ARCHIVE_DIR = original_archive_dir
+
+            self.assertFalse(source.exists())
+            self.assertTrue((archive_root / ".gitkeep").exists())
+            self.assertFalse((archive_root / "0.22.0").exists())
+            self.assertTrue((archive_root / "0.23.0" / "12.fix.md").exists())
+
+
+if __name__ == "__main__":
+    unittest.main()
