From e26ed29385690e902a2de283ebd2c39e1faf5323 Mon Sep 17 00:00:00 2001 From: ChuckBuilds Date: Sun, 2 Aug 2026 20:40:06 -0400 Subject: [PATCH] =?UTF-8?q?fix(version):=20address=20review=20=E2=80=94=20?= =?UTF-8?q?regex=20strictness,=20OSError,=20stale=20doc=20claim?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From CodeRabbit on #428, all three valid: - The module docstring claimed the tag check "runs at release time in .github/workflows/release-version-check.yml". That workflow is held back to a follow-up PR (the pushing token lacks the `workflow` scope), so the claim was false as written. Both files now describe the script as a manual pre-flight and say the CI wiring is still to come. - `\d` also matches non-ASCII decimal digits, which int() happily parses, and `\s` matches newlines -- so "##\n3.2.0" read as a version heading. Patterns now use [0-9] and [ \t], kept in step across the test and the script, with a regression test pinning both behaviours. - A missing or unreadable CHANGELOG.md raised OSError out of read_text() and printed a traceback. In a release gate that reads as "the tooling is broken"; it now reports the path and a recovery action and exits 1. Verified: v3.2.0 passes, a mismatched tag exits 1, and a missing CHANGELOG exits 1 with the new message instead of a traceback. 5 tests pass. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5 --- scripts/check_release_version.py | 35 ++++++++++++++++++++++++++------ test/test_version_consistency.py | 30 +++++++++++++++++++++++---- 2 files changed, 55 insertions(+), 10 deletions(-) diff --git a/scripts/check_release_version.py b/scripts/check_release_version.py index 1c9bec66..45dc0ead 100644 --- a/scripts/check_release_version.py +++ b/scripts/check_release_version.py @@ -5,9 +5,9 @@ Run it *before* creating a tag to check yourself: python scripts/check_release_version.py v3.2.0 -CI runs it on every pushed `v*` tag and published release -(`.github/workflows/release-version-check.yml`), so a mismatch shows up as a -red check on the release rather than as a silent wrong answer on user devices. +Wiring it into CI (on pushed `v*` tags and published releases) is a follow-up +PR, so for now it is a manual pre-flight: run it before creating the tag and a +mismatch shows up here rather than as a silent wrong answer on user devices. Why this exists: `v3.1.0` was tagged 2026-05-31 while `src/__init__.py` still said `"1.0.0"`; the bump to `"3.1.0"` did not land until 2026-07-12. Devices @@ -27,8 +27,13 @@ from pathlib import Path REPO_ROOT = Path(__file__).resolve().parents[1] sys.path.insert(0, str(REPO_ROOT)) -SEMVER = re.compile(r"^\d+\.\d+\.\d+$") -HEADING = re.compile(r"^##\s+(?P\d+\.\d+\.\d+)\s*$", re.MULTILINE) +# [0-9] rather than \d, and [ \t] rather than \s: \d also matches non-ASCII +# decimal digits (which int() parses), and \s matches newlines, so "##\n3.2.0" +# would otherwise read as a version heading. Keep these in step with +# test/test_version_consistency.py. +SEMVER = re.compile(r"^[0-9]+\.[0-9]+\.[0-9]+$") +HEADING = re.compile( + r"^##[ \t]+(?P[0-9]+\.[0-9]+\.[0-9]+)[ \t]*$", re.MULTILINE) def normalize(tag: str) -> str: @@ -37,6 +42,13 @@ def normalize(tag: str) -> str: def newest_changelog_version(changelog: Path) -> str | None: + """Newest version heading, or None when there is none. + + Raises OSError if the file cannot be read; main() turns that into a clear + message rather than a traceback, because this runs as a release gate and a + traceback there reads as "the tooling is broken", not "your CHANGELOG is + missing". + """ headings = HEADING.findall(changelog.read_text(encoding="utf-8")) return headings[0] if headings else None @@ -52,10 +64,21 @@ def main() -> int: from src import __version__ as core_version tag_version = normalize(args.tag) - changelog_version = newest_changelog_version(REPO_ROOT / "CHANGELOG.md") + changelog_path = REPO_ROOT / "CHANGELOG.md" problems: list[str] = [] + try: + changelog_version = newest_changelog_version(changelog_path) + except OSError as e: + print( + f"Release version check FAILED for tag {args.tag}:\n" + f" - could not read {changelog_path}: {e}\n" + f" Restore the file (git checkout -- CHANGELOG.md) and re-run.", + file=sys.stderr, + ) + return 1 + if not SEMVER.match(tag_version): problems.append( f"tag {args.tag!r} is not vX.Y.Z. Older tags (v2.5) predate this " diff --git a/test/test_version_consistency.py b/test/test_version_consistency.py index e0e2d7b6..03e638a3 100644 --- a/test/test_version_consistency.py +++ b/test/test_version_consistency.py @@ -13,8 +13,12 @@ which is below the `(2, 0, 0)` floor in `PluginLoader._warn_if_incompatible` — so those users get no compatibility warning at all. See `docs/SPORTS_UNIFICATION.md` (phase B4). -The matching tag check runs at release time in -`.github/workflows/release-version-check.yml`; a tag is not available here. +A tag is not available here, so the tag half of the check lives in +`scripts/check_release_version.py`. Wiring that script into CI (on pushed `v*` +tags and published releases) is a follow-up PR; until it lands, run it by hand +before tagging: + + python scripts/check_release_version.py v3.2.0 Note: `src.plugin_system.__version__` is deliberately NOT checked. That module versions the *plugin API* (it sits beside `__api_version__` and is documented as @@ -31,10 +35,15 @@ import src REPO_ROOT = Path(__file__).resolve().parents[1] CHANGELOG = REPO_ROOT / "CHANGELOG.md" -SEMVER = re.compile(r"^(\d+)\.(\d+)\.(\d+)$") +# [0-9] rather than \d: \d also matches non-ASCII decimal digits, which int() +# happily parses, so a heading in Arabic-Indic numerals would pass the pattern +# and then mismatch confusingly. [ \t] rather than \s for the same class of +# reason -- \s matches newlines, so "##\n3.2.0" would read as a heading. +SEMVER = re.compile(r"^([0-9]+)\.([0-9]+)\.([0-9]+)$") # Version headings look like "## 3.2.0". A leading "## Unreleased" section is # allowed and skipped -- it is where module additions are staged before a bump. -HEADING = re.compile(r"^##\s+(?P\d+\.\d+\.\d+)\s*$", re.MULTILINE) +HEADING = re.compile( + r"^##[ \t]+(?P[0-9]+\.[0-9]+\.[0-9]+)[ \t]*$", re.MULTILINE) def test_core_version_is_semver(): @@ -89,3 +98,16 @@ def test_web_interface_version_tracks_the_core(): "web_interface.__version__ has drifted from src.__version__; it should " "re-export the canonical value rather than hardcode its own." ) + + +def test_heading_pattern_is_strict_about_digits_and_whitespace(): + """`\\d` also matches non-ASCII decimal digits and `\\s` matches newlines, + either of which would let a malformed heading through and then fail the + comparison with a confusing message. Pin the tightened patterns.""" + assert HEADING.findall("## 3.2.0\n") == ["3.2.0"] + assert HEADING.findall("##\t3.2.0 \n") == ["3.2.0"] + # A bare "##" whose version sits on the next line is not a heading. + assert HEADING.findall("##\n3.2.0\n") == [] + # Arabic-Indic digits parse via int() but are not our version format. + assert HEADING.findall("## ٣.٢.٠\n") == [] + assert SEMVER.match("٣.٢.٠") is None