From fc3b3f45f847a7776bf2c1d617733e74924df1b7 Mon Sep 17 00:00:00 2001 From: ancplua Date: Sun, 9 Aug 2026 08:56:59 +0200 Subject: [PATCH] Harden upstream pin freshness reporting --- .github/workflows/ci.yml | 3 + .github/workflows/pin-freshness.yml | 80 ++++---- .../scripts/check_pin_freshness.py | 158 ++++++++++------ tests/scripts/test_check_pin_freshness.py | 178 ++++++++++++++++++ 4 files changed, 325 insertions(+), 94 deletions(-) create mode 100644 tests/scripts/test_check_pin_freshness.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 6f0037a..4e1861f 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -12,6 +12,9 @@ jobs: steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7 + - name: Test pin freshness checker + run: python3 -m unittest discover --start-directory tests/scripts --pattern 'test_*.py' + - name: Setup .NET uses: actions/setup-dotnet@a98b56852c35b8e3190ac28c8c2271da59106c68 # v6 with: diff --git a/.github/workflows/pin-freshness.yml b/.github/workflows/pin-freshness.yml index 6d8d3d8..3ddf460 100644 --- a/.github/workflows/pin-freshness.yml +++ b/.github/workflows/pin-freshness.yml @@ -2,11 +2,9 @@ name: pin-freshness # Reports when an upstream ref pinned in Version.props has fallen behind upstream. # -# Deliberately not a step in ci.yml. A pin falling behind is news about upstream, not -# a defect in this commit, so gating pull requests on it would redden unrelated work -# the moment upstream commits and train everyone to ignore the signal. It runs weekly -# and keeps one tracking issue in step with reality: opened when a pin is behind, -# edited on later runs rather than duplicated, and closed once the pins match again. +# Deliberately not a step in ci.yml: upstream movement is news, not a defect in the +# current commit. It runs weekly and keeps one issue in step with reality: opened +# when a pin differs, updated while it differs, and closed once the pins match again. # # The job itself only fails when freshness could not be determined — a lookup that # cannot complete must not read as "current". @@ -44,8 +42,6 @@ jobs: # default `set -e` would abandon the report before it is published. status=0 python3 "${script}" >"${RUNNER_TEMP}/report.md" 2>"${RUNNER_TEMP}/error.txt" || status=$? - echo "status=${status}" >>"${GITHUB_OUTPUT}" - cat "${RUNNER_TEMP}/report.md" >>"${GITHUB_STEP_SUMMARY}" if [[ -s "${RUNNER_TEMP}/error.txt" ]]; then { @@ -57,38 +53,50 @@ jobs: fi cat "${RUNNER_TEMP}/report.md" - - name: Open or update the tracking issue - if: steps.check.outputs.status == '1' - env: - GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} - run: | - set -euo pipefail - number="$(gh issue list --state open --search "${ISSUE_TITLE} in:title" \ - --json number,title \ - --jq "[.[] | select(.title == \"${ISSUE_TITLE}\") | .number] | first // empty")" - if [[ -n "${number}" ]]; then - gh issue edit "${number}" --body-file "${RUNNER_TEMP}/report.md" - echo "updated issue #${number}" - else - gh issue create --title "${ISSUE_TITLE}" --body-file "${RUNNER_TEMP}/report.md" - fi + case "${status}" in + 0|2|10) ;; + *) + echo "::error::pin freshness checker exited unexpectedly with status ${status}" + exit 1 + ;; + esac + echo "status=${status}" >>"${GITHUB_OUTPUT}" - - name: Close the tracking issue once every pin is current - if: steps.check.outputs.status == '0' + - name: Reconcile the tracking issue env: + CHECK_STATUS: ${{ steps.check.outputs.status }} GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} run: | set -euo pipefail - number="$(gh issue list --state open --search "${ISSUE_TITLE} in:title" \ - --json number,title \ - --jq "[.[] | select(.title == \"${ISSUE_TITLE}\") | .number] | first // empty")" - if [[ -n "${number}" ]]; then - gh issue close "${number}" --comment "Every pinned upstream ref matches upstream again." - fi + find_open_issue() { + gh issue list --state open --search "${ISSUE_TITLE} in:title" --limit 100 \ + --json number,title \ + --jq "[.[] | select(.title == \"${ISSUE_TITLE}\") | .number] | first // empty" + } - - name: Fail when freshness could not be determined - if: steps.check.outputs.status == '2' - run: | - echo "::error::upstream pin freshness could not be determined; the pins are unverified" - cat "${RUNNER_TEMP}/error.txt" >&2 - exit 1 + case "${CHECK_STATUS}" in + 10) + number="$(find_open_issue)" + if [[ -n "${number}" ]]; then + gh issue edit "${number}" --body-file "${RUNNER_TEMP}/report.md" + echo "updated issue #${number}" + else + gh issue create --title "${ISSUE_TITLE}" --body-file "${RUNNER_TEMP}/report.md" + fi + ;; + 0) + number="$(find_open_issue)" + if [[ -n "${number}" ]]; then + gh issue close "${number}" --comment "Every pinned upstream ref matches upstream again." + fi + ;; + 2) + echo "::error::upstream pin freshness could not be determined; the pins are unverified" + cat "${RUNNER_TEMP}/error.txt" >&2 + exit 1 + ;; + *) + echo "::error::unexpected checker status ${CHECK_STATUS}" + exit 1 + ;; + esac diff --git a/src/Qyl.Telemetry.SemanticConventions.SourceGeneration/scripts/check_pin_freshness.py b/src/Qyl.Telemetry.SemanticConventions.SourceGeneration/scripts/check_pin_freshness.py index bec900d..0032bbe 100755 --- a/src/Qyl.Telemetry.SemanticConventions.SourceGeneration/scripts/check_pin_freshness.py +++ b/src/Qyl.Telemetry.SemanticConventions.SourceGeneration/scripts/check_pin_freshness.py @@ -1,17 +1,9 @@ #!/usr/bin/env python3 -"""Report when an upstream ref pinned in Version.props has fallen behind upstream. +"""Report when an upstream ref pinned in Version.props differs from upstream. -The registry pins are exact by design — a moving registry would change generated -constants without a commit here. The cost is that nothing about a stale pin is -self-announcing, and one kind is entirely silent: SemConvGenAiRef is a bare commit -SHA on a branch-tracked upstream, so there is no "a newer version exists" signal to -notice. That is how the pin sat 9 commits behind and reached -gen_ai.request.previous_response.id (upstream #372) late. - -This does not gate anything. A pin falling behind is news about upstream, not a -defect in this repository, so wiring it into ci.yml would redden unrelated pull -requests the moment upstream commits and teach everyone to ignore it. It runs on a -schedule and reports. +The registry pins are exact by design: moving inputs must not change generated +constants without a commit here. This scheduled check reports upstream movement; +it does not decide whether or when to regenerate. Three pins, two shapes: @@ -19,12 +11,10 @@ WeaverVersion release tag v{version} vs the latest release SemConvGenAiRef branch SHA commit distance from the tracked branch head -A lookup that cannot complete exits 2 rather than reporting "current". A check -unable to distinguish a fresh pin from an unreachable upstream is worse than no -check, because it reports green while blind. +A lookup that cannot prove freshness exits 2 rather than reporting "current". -CLI: check_pin_freshness.py (exit 0 = every pin current; exit 1 = a pin is behind, - reported on stdout; exit 2 = a lookup failed) +CLI: check_pin_freshness.py (exit 0 = every pin current; exit 10 = a pin differs, + reported on stdout; exit 2 = freshness could not be determined) """ from __future__ import annotations @@ -46,10 +36,13 @@ WEAVER_REPO = os.environ.get("SEMCONV_WEAVER_UPSTREAM", "open-telemetry/weaver") COMPARE_COMMIT_LIMIT = 10 +EXIT_CURRENT = 0 +EXIT_UNKNOWN = 2 +EXIT_STALE = 10 -class LookupFailed(Exception): - """An upstream lookup could not be completed, so freshness is unknown.""" +class FreshnessUnknown(Exception): + """The checker could not prove whether every pin matches upstream.""" def read_version_property(name: str) -> str: @@ -62,9 +55,12 @@ def read_version_property(name: str) -> str: if override and os.environ.get(override): return os.environ[override].strip() - value = ET.parse(VERSION_PROPS).getroot().findtext(f".//{name}") + try: + value = ET.parse(VERSION_PROPS).getroot().findtext(f".//{name}") + except (OSError, ET.ParseError) as error: + raise FreshnessUnknown(f"could not read {VERSION_PROPS}: {error}") from error if value is None or not value.strip(): - raise SystemExit(f"error: Version.props does not define {name}") + raise FreshnessUnknown(f"{VERSION_PROPS} does not define {name}") return value.strip() @@ -78,34 +74,49 @@ def github_json(path: str) -> dict: try: with urllib.request.urlopen(request, timeout=30) as response: - return json.load(response) + payload = json.load(response) except urllib.error.HTTPError as error: detail = f"HTTP {error.code}" if error.code in (403, 429): detail += " (rate limited; set GITHUB_TOKEN)" elif error.code == 404: detail += " (renamed, deleted, or unknown ref)" - raise LookupFailed(f"{path}: {detail}") from error + raise FreshnessUnknown(f"{path}: {detail}") from error except (urllib.error.URLError, TimeoutError) as error: - raise LookupFailed(f"{path}: {error}") from error + raise FreshnessUnknown(f"{path}: {error}") from error except json.JSONDecodeError as error: - raise LookupFailed(f"{path}: upstream returned malformed JSON: {error}") from error + raise FreshnessUnknown(f"{path}: upstream returned malformed JSON: {error}") from error + + if not isinstance(payload, dict): + raise FreshnessUnknown(f"{path}: upstream response was not a JSON object") + return payload + + +def required_nonnegative_int(payload: dict, key: str, context: str) -> int: + """Read a required GitHub count without turning a malformed response into zero.""" + value = payload.get(key) + if isinstance(value, bool) or not isinstance(value, int) or value < 0: + raise FreshnessUnknown(f"{context}: response carried no valid {key}") + return value def check_release_pin(label: str, repo: str, pinned_version: str) -> tuple[bool, list[str]]: """Compare a pinned release version against the repository's latest release.""" release = github_json(f"repos/{repo}/releases/latest") latest_tag = release.get("tag_name") - if not latest_tag: - raise LookupFailed(f"repos/{repo}/releases/latest: response carried no tag_name") + if not isinstance(latest_tag, str) or not latest_tag: + raise FreshnessUnknown(f"repos/{repo}/releases/latest: response carried no tag_name") pinned_tag = f"v{pinned_version}" if latest_tag == pinned_tag: return True, [f"- **{label}** current at `{pinned_tag}` ({repo})"] + release_url = release.get("html_url") + if not isinstance(release_url, str) or not release_url: + release_url = f"https://github.com/{repo}/releases" return False, [ f"- **{label}** pinned at `{pinned_tag}`, latest release is `{latest_tag}`", - f" - {release.get('html_url', f'https://github.com/{repo}/releases')}", + f" - {release_url}", ] @@ -113,60 +124,91 @@ def check_branch_pin(label: str, repo: str, pinned_sha: str, branch: str) -> tup """Measure how far a pinned commit sits behind the head of a tracked branch.""" comparison = github_json(f"repos/{repo}/compare/{pinned_sha}...{branch}") status = comparison.get("status") - if status is None: - raise LookupFailed(f"repos/{repo}/compare: response carried no status") - - # Compare is expressed from the base's perspective: base...head reports how far - # head runs ahead of the pin, which is how far the pin trails the branch. - behind_by = comparison.get("ahead_by", 0) - if status == "identical" or behind_by == 0: + if status not in {"identical", "ahead", "behind", "diverged"}: + raise FreshnessUnknown(f"repos/{repo}/compare: response carried unknown status {status!r}") + + context = f"repos/{repo}/compare" + branch_ahead_by = required_nonnegative_int(comparison, "ahead_by", context) + branch_behind_by = required_nonnegative_int(comparison, "behind_by", context) + compare_url = comparison.get("html_url") + if not isinstance(compare_url, str) or not compare_url: + compare_url = f"https://github.com/{repo}/compare/{pinned_sha}...{branch}" + + if status == "identical": + if branch_ahead_by != 0 or branch_behind_by != 0: + raise FreshnessUnknown(f"{context}: identical comparison carried non-zero distances") return True, [f"- **{label}** current at `{pinned_sha[:7]}`, the head of `{branch}` ({repo})"] if status == "diverged": + if branch_ahead_by == 0 or branch_behind_by == 0: + raise FreshnessUnknown(f"{context}: diverged comparison carried a zero distance") return False, [ f"- **{label}** pinned at `{pinned_sha[:7]}`, which has **diverged** from `{branch}` " - f"({behind_by} ahead on the branch, {comparison.get('behind_by', 0)} only on the pin)", - f" - {comparison.get('html_url', '')}", + f"({branch_ahead_by} ahead on the branch, {branch_behind_by} only on the pin)", + f" - {compare_url}", ] - total = comparison.get("total_commits", behind_by) + if status == "behind": + if branch_ahead_by != 0 or branch_behind_by == 0: + raise FreshnessUnknown(f"{context}: behind comparison carried inconsistent distances") + return False, [ + f"- **{label}** pinned at `{pinned_sha[:7]}`, but `{branch}` is " + f"**{branch_behind_by} commit(s) behind the pin** ({repo})", + f" - {compare_url}", + " - The pin is not the tracked branch head; check for a force-push or an incorrect pin.", + ] + + if branch_ahead_by == 0 or branch_behind_by != 0: + raise FreshnessUnknown(f"{context}: ahead comparison carried inconsistent distances") + lines = [ - f"- **{label}** pinned at `{pinned_sha[:7]}`, **{behind_by} commit(s) behind** `{branch}` ({repo})", - f" - {comparison.get('html_url', '')}", + f"- **{label}** pinned at `{pinned_sha[:7]}`, **{branch_ahead_by} commit(s) behind** `{branch}` ({repo})", + f" - {compare_url}", ] commits = comparison.get("commits", []) + if not isinstance(commits, list): + raise FreshnessUnknown(f"{context}: response carried no valid commits list") for commit in commits[-COMPARE_COMMIT_LIMIT:]: - subject = (commit.get("commit", {}).get("message") or "").splitlines()[0] - lines.append(f" - `{commit.get('sha', '')[:7]}` {subject}") - if total > len(commits): - lines.append(f" - …{total - len(commits)} further commit(s) not listed by the compare API") + if not isinstance(commit, dict): + raise FreshnessUnknown(f"{context}: response carried a malformed commit") + sha = commit.get("sha") + metadata = commit.get("commit") + if not isinstance(sha, str) or not isinstance(metadata, dict): + raise FreshnessUnknown(f"{context}: response carried a malformed commit") + message = metadata.get("message") + if not isinstance(message, str): + raise FreshnessUnknown(f"{context}: response carried a commit without a message") + lines.append(f" - `{sha[:7]}` {message.splitlines()[0]}") + if branch_ahead_by > len(commits): + lines.append(f" - …{branch_ahead_by - len(commits)} further commit(s) not listed by the compare API") return False, lines def main() -> int: - pins = { - "SemConvSchemaVersion": read_version_property("SemConvSchemaVersion"), - "SemConvGenAiRef": read_version_property("SemConvGenAiRef"), - "WeaverVersion": read_version_property("WeaverVersion"), - } - report: list[str] = ["## Upstream pin freshness", ""] stale = False try: - for current, lines in ( - check_release_pin("SemConvSchemaVersion", CORE_REPO, pins["SemConvSchemaVersion"]), - check_branch_pin("SemConvGenAiRef", GENAI_REPO, pins["SemConvGenAiRef"], GENAI_BRANCH), - check_release_pin("WeaverVersion", WEAVER_REPO, pins["WeaverVersion"]), - ): + pins = { + "SemConvSchemaVersion": read_version_property("SemConvSchemaVersion"), + "SemConvGenAiRef": read_version_property("SemConvGenAiRef"), + "WeaverVersion": read_version_property("WeaverVersion"), + } + checks = ( + (check_release_pin, ("SemConvSchemaVersion", CORE_REPO, pins["SemConvSchemaVersion"])), + (check_branch_pin, ("SemConvGenAiRef", GENAI_REPO, pins["SemConvGenAiRef"], GENAI_BRANCH)), + (check_release_pin, ("WeaverVersion", WEAVER_REPO, pins["WeaverVersion"])), + ) + for check, arguments in checks: + current, lines = check(*arguments) stale = stale or not current report.extend(lines) - except LookupFailed as error: + except FreshnessUnknown as error: print("\n".join(report), flush=True) print(f"\nfreshness unknown: {error}", file=sys.stderr) - return 2 + return EXIT_UNKNOWN report.append("") report.append( @@ -177,7 +219,7 @@ def main() -> int: else "Every pin matches upstream." ) print("\n".join(report)) - return 1 if stale else 0 + return EXIT_STALE if stale else EXIT_CURRENT if __name__ == "__main__": diff --git a/tests/scripts/test_check_pin_freshness.py b/tests/scripts/test_check_pin_freshness.py new file mode 100644 index 0000000..d0e49ea --- /dev/null +++ b/tests/scripts/test_check_pin_freshness.py @@ -0,0 +1,178 @@ +from __future__ import annotations + +import contextlib +import importlib.util +import io +import sys +import unittest +from pathlib import Path +from unittest import mock + + +REPO_ROOT = Path(__file__).resolve().parents[2] +SCRIPT = ( + REPO_ROOT + / "src" + / "Qyl.Telemetry.SemanticConventions.SourceGeneration" + / "scripts" + / "check_pin_freshness.py" +) +SPEC = importlib.util.spec_from_file_location("check_pin_freshness", SCRIPT) +if SPEC is None or SPEC.loader is None: + raise RuntimeError(f"could not load {SCRIPT}") +CHECKER = importlib.util.module_from_spec(SPEC) +sys.modules[SPEC.name] = CHECKER +SPEC.loader.exec_module(CHECKER) + + +class ReleasePinTests(unittest.TestCase): + def test_equal_release_is_current(self) -> None: + with mock.patch.object( + CHECKER, + "github_json", + return_value={"tag_name": "v1.43.0", "html_url": "https://example.test/release"}, + ): + current, lines = CHECKER.check_release_pin("core", "owner/repo", "1.43.0") + + self.assertTrue(current) + self.assertIn("current", lines[0]) + + def test_different_release_is_stale(self) -> None: + with mock.patch.object( + CHECKER, + "github_json", + return_value={"tag_name": "v1.44.0", "html_url": "https://example.test/release"}, + ): + current, lines = CHECKER.check_release_pin("core", "owner/repo", "1.43.0") + + self.assertFalse(current) + self.assertIn("latest release is `v1.44.0`", lines[0]) + + def test_missing_release_tag_is_unknown(self) -> None: + with mock.patch.object(CHECKER, "github_json", return_value={}): + with self.assertRaises(CHECKER.FreshnessUnknown): + CHECKER.check_release_pin("core", "owner/repo", "1.43.0") + + +class BranchPinTests(unittest.TestCase): + def check(self, response: dict) -> tuple[bool, list[str]]: + with mock.patch.object(CHECKER, "github_json", return_value=response): + return CHECKER.check_branch_pin("genai", "owner/repo", "abcdef123456", "main") + + def test_identical_commit_is_current(self) -> None: + current, lines = self.check( + {"status": "identical", "ahead_by": 0, "behind_by": 0, "html_url": "https://example.test"} + ) + + self.assertTrue(current) + self.assertIn("the head of `main`", lines[0]) + + def test_branch_ahead_marks_pin_stale_and_lists_commits(self) -> None: + current, lines = self.check( + { + "status": "ahead", + "ahead_by": 1, + "behind_by": 0, + "html_url": "https://example.test", + "commits": [{"sha": "123456789", "commit": {"message": "Registry change\n\nDetails"}}], + } + ) + + self.assertFalse(current) + self.assertIn("1 commit(s) behind", lines[0]) + self.assertIn("`1234567` Registry change", lines[2]) + + def test_branch_behind_pin_is_not_reported_current(self) -> None: + current, lines = self.check( + {"status": "behind", "ahead_by": 0, "behind_by": 2, "html_url": "https://example.test"} + ) + + self.assertFalse(current) + self.assertIn("behind the pin", lines[0]) + self.assertIn("force-push", lines[2]) + + def test_diverged_pin_is_stale(self) -> None: + current, lines = self.check( + {"status": "diverged", "ahead_by": 3, "behind_by": 2, "html_url": "https://example.test"} + ) + + self.assertFalse(current) + self.assertIn("diverged", lines[0]) + + def test_missing_distance_is_unknown_instead_of_current(self) -> None: + with self.assertRaises(CHECKER.FreshnessUnknown): + self.check({"status": "ahead", "behind_by": 0, "html_url": "https://example.test"}) + + def test_unknown_status_is_unknown(self) -> None: + with self.assertRaises(CHECKER.FreshnessUnknown): + self.check({"status": "surprising", "ahead_by": 0, "behind_by": 0}) + + def test_inconsistent_status_distances_are_unknown(self) -> None: + with self.assertRaises(CHECKER.FreshnessUnknown): + self.check({"status": "identical", "ahead_by": 1, "behind_by": 0}) + + def test_malformed_commit_is_unknown(self) -> None: + with self.assertRaises(CHECKER.FreshnessUnknown): + self.check( + { + "status": "ahead", + "ahead_by": 1, + "behind_by": 0, + "commits": [{"sha": "123456789", "commit": None}], + } + ) + + +class MainTests(unittest.TestCase): + @staticmethod + def version(_name: str) -> str: + return "pin" + + def run_main(self, branch_result: tuple[bool, list[str]]) -> tuple[int, str, str]: + current_release = (True, ["- release current"]) + stdout = io.StringIO() + stderr = io.StringIO() + with ( + mock.patch.object(CHECKER, "read_version_property", side_effect=self.version), + mock.patch.object(CHECKER, "check_release_pin", return_value=current_release), + mock.patch.object(CHECKER, "check_branch_pin", return_value=branch_result), + contextlib.redirect_stdout(stdout), + contextlib.redirect_stderr(stderr), + ): + status = CHECKER.main() + return status, stdout.getvalue(), stderr.getvalue() + + def test_current_exit_status(self) -> None: + status, output, error = self.run_main((True, ["- branch current"])) + + self.assertEqual(CHECKER.EXIT_CURRENT, status) + self.assertIn("Every pin matches upstream", output) + self.assertEqual("", error) + + def test_stale_has_dedicated_exit_status(self) -> None: + status, output, error = self.run_main((False, ["- branch stale"])) + + self.assertEqual(CHECKER.EXIT_STALE, status) + self.assertNotEqual(1, status) + self.assertIn("One or more pins", output) + self.assertEqual("", error) + + def test_unknown_exit_status(self) -> None: + stdout = io.StringIO() + stderr = io.StringIO() + with ( + mock.patch.object( + CHECKER, "read_version_property", side_effect=CHECKER.FreshnessUnknown("broken input") + ), + contextlib.redirect_stdout(stdout), + contextlib.redirect_stderr(stderr), + ): + status = CHECKER.main() + + self.assertEqual(CHECKER.EXIT_UNKNOWN, status) + self.assertIn("Upstream pin freshness", stdout.getvalue()) + self.assertIn("freshness unknown: broken input", stderr.getvalue()) + + +if __name__ == "__main__": + unittest.main()