From b03cfbae131544e0ef15ccccaeabf29ea9b37a47 Mon Sep 17 00:00:00 2001 From: James Mackay Date: Sun, 11 Oct 2026 06:48:44 +0100 Subject: [PATCH] fix(release): notes must not open with the bot commit; guard matches the subject (t44 3b/3c) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit t45's review of the release versioning found three lows; two touch behaviour here (the third, README wording, is the next commit). R1 — with a write-back the tag points at the bot's own commit, so the notes range `PREV..TAG` published "- chore(release): vX [skip ci]" as the first line of every release. The range now ends at `${TAG}^` when BUMP=yes (the last commit a developer wrote) and at `${TAG}` when it is not (there the tag IS the pushed commit, and it belongs in the notes). `git describe` for the previous tag is unchanged: it already started from `${TAG}^`, so it keeps naming the previous release. R3 — the job guard matched the `[skip ci]` marker ANYWHERE in the commit message, so a developer commit whose body merely mentioned it silently skipped its own release. The guard is now anchored to the release commit's subject prefix (`!startsWith(message, 'chore(release):')`), which is a structural property, not a substring a body can trip. The marker stays in the commit message: it is the convention Gitea itself may honour to skip the run outright, and the README now says so, together with the fact that the job guard is the layer that holds when it does not. 3c — the resolver's tests used tag fixtures, so `refs/tags/X^{commit}` was never exercised, and that dereference is the entire reason an annotated tag is recognised (for one, `git rev-parse refs/tags/X` is the TAG OBJECT, not the commit, so a comparison that forgot to dereference would miss a released commit). The resolver now also runs inside real temporary repositories built with real annotated and lightweight tags: released-on-HEAD both ways, a tag on an older commit falling back to the date, no tags at all, the same-day scan walking real tags, and gap reuse. The workflow's own run: scripts are executed too — extracted from the YAML by hand rather than with a parser the fast suite does not have — covering the bump/verify/tag path (commit subject, tag == HEAD, the tag's manifest, the pushed tag), the inconsistent-manifest refusal, the rejected-push failure, and the notes range: with BUMP=yes the fixed range yields "- feat: the real change" while the range it replaced demonstrably contains "- chore(release):". The job-level `if:` itself cannot be executed here (no Gitea runner) and stays a contract pin; the script-side layer that makes a bypassed guard harmless is executed instead. Verified: 414 passed / 2 skipped, ruff clean, release.yml parses. --- .gitea/workflows/release.yml | 37 ++-- tests/test_release_versioning.py | 297 +++++++++++++++++++++++++++++++ 2 files changed, 322 insertions(+), 12 deletions(-) diff --git a/.gitea/workflows/release.yml b/.gitea/workflows/release.yml index e1e4e58..4db267f 100644 --- a/.gitea/workflows/release.yml +++ b/.gitea/workflows/release.yml @@ -47,15 +47,18 @@ env: jobs: release: runs-on: ubuntu-latest - # t42: this job pushes the write-back commit itself, and a run starting from - # that commit would release the same version twice. Two layers guard it: the - # commit message carries the conventional `[skip ci]` marker, and this - # condition re-checks the pushed message, so the guard still holds if a Gitea - # version stops honouring the marker. workflow_dispatch has no head_commit, - # hence the first clause. + # t42/t44 R3: this job pushes the write-back commit itself, and a run starting + # from that commit would release the same version twice. The guard is anchored to + # the release commit's SUBJECT (`chore(release): …`), not to the `[skip ci]` + # marker anywhere in the message: a developer commit whose body happens to + # mention [skip ci] must still release — with contains() it silently did not. + # The marker stays in the commit message because it is the convention Gitea + # itself may honour to skip the run outright (README documents that platform + # layer, and that the job guard is a no-op there rather than the trigger). + # workflow_dispatch has no head_commit, hence the first clause. if: >- github.event_name == 'workflow_dispatch' || - !contains(github.event.head_commit.message || '', '[skip ci]') + !startsWith(github.event.head_commit.message || '', 'chore(release):') steps: - uses: actions/checkout@v4 with: @@ -151,18 +154,28 @@ jobs: env: VERSION: ${{ steps.version.outputs.VERSION }} TAG: ${{ steps.version.outputs.TAG }} + BUMP: ${{ steps.version.outputs.BUMP }} SERVER: ${{ github.server_url }} REPO: ${{ github.repository }} TOKEN: ${{ secrets.GITEA_TOKEN }} run: | set -euo pipefail - # Notes: subject lines of the commits in the released range. - PREV="$(git describe --tags --abbrev=0 "${TAG}^" 2>/dev/null || true)" - if [ -n "$PREV" ]; then - RANGE="${PREV}..${TAG}" + # Notes: subject lines of the commits in the released range — the + # commits a DEVELOPER wrote. With a write-back (BUMP=yes) the tag points + # at the bot's own commit, so the range has to stop at its parent or every + # release would open with "chore(release): vX [skip ci]". Without one the + # tag is the pushed commit itself and belongs in the notes (t44 R1). + if [ "$BUMP" = "yes" ]; then + NOTES_END="${TAG}^" else - RANGE="${TAG}" + NOTES_END="${TAG}" + fi + PREV="$(git describe --tags --abbrev=0 "${NOTES_END}^" 2>/dev/null || true)" + if [ -n "$PREV" ]; then + RANGE="${PREV}..${NOTES_END}" + else + RANGE="${NOTES_END}" fi { echo "Release ${TAG}" diff --git a/tests/test_release_versioning.py b/tests/test_release_versioning.py index 9b172a1..80e7305 100644 --- a/tests/test_release_versioning.py +++ b/tests/test_release_versioning.py @@ -333,6 +333,26 @@ def test_the_bot_commit_cannot_trigger_another_release(workflow_text): assert "workflow_dispatch" in workflow_text +def test_the_skip_guard_matches_the_subject_not_the_whole_message(workflow_text): + """t44 R3: `contains(message, '[skip ci]')` skipped a release silently. + + A developer commit whose BODY mentions the marker is not a release commit, so + the guard is anchored to the release bot's subject prefix instead. + """ + assert "!startsWith(github.event.head_commit.message || '', 'chore(release):')" in ( + workflow_text + ) + assert "!contains(github.event.head_commit.message" not in workflow_text + + +def test_the_release_notes_exclude_the_write_back_commit(workflow_text): + """t44 R1: with a write-back the tag IS the bot commit, so notes must skip it.""" + assert 'if [ "$BUMP" = "yes" ]; then' in workflow_text + assert 'NOTES_END="${TAG}^"' in workflow_text + assert 'RANGE="${PREV}..${NOTES_END}"' in workflow_text + assert 'BUMP: ${{ steps.version.outputs.BUMP }}' in workflow_text + + def test_the_preserved_release_behaviour_is_still_there(workflow_text): # one release at a time, never cancelled mid-flight assert "group: release-master" in workflow_text @@ -355,3 +375,280 @@ def test_the_script_reads_the_manifest_the_workflow_bumps(): manifest_version = json.loads(MANIFEST.read_text(encoding="utf-8"))["version"] # the script rejects anything else, so a typo here would fail every release assert re.fullmatch(r"[0-9]{4}\.[0-9]{2}\.[0-9]{2}(\.[0-9]+)?", manifest_version) + + +# ── t44 3c: real repositories, and the workflow's own steps by execution ───── + +MANIFEST_REL = "custom_components/nvidia_shield/manifest.json" + + +def _git(repo, *args) -> str: + return subprocess.run( + ["git", *args], cwd=repo, capture_output=True, text=True, check=True + ).stdout + + +def _repo(tmp_path, *, version="2026.10.09", commits=("a feature commit",)): + """A real repository: the resolver's tag lookups really are git commands.""" + repo = tmp_path / "repo" + repo.mkdir(parents=True) + _git(repo, "init", "-q", "-b", "master") + _git(repo, "config", "user.email", "dev@example.com") + _git(repo, "config", "user.name", "dev") + manifest = repo / MANIFEST_REL + manifest.parent.mkdir(parents=True, exist_ok=True) + for index, subject in enumerate(commits): + manifest.write_text( + json.dumps( + {"domain": "nvidia_shield", "version": version, "note": index} + ) + + "\n" + ) + _git(repo, "add", "-A") + _git(repo, "commit", "-qm", subject) + return repo + + +def _resolve_in(repo, *, today=TODAY): + """Run the resolver inside a real repo — no tag fixtures, the real git paths.""" + result = subprocess.run( + ["bash", str(SCRIPT), "--manifest", MANIFEST_REL, "--today", today], + cwd=repo, capture_output=True, text=True, check=False, + ) + values = dict( + line.split("=", 1) for line in result.stdout.splitlines() if "=" in line + ) + return result, values + + +class TestRealRepositories: + """The tagged-version branches against real git (t44 3c). + + The fixture-based tests above never exercise `refs/tags/X^{commit}`, and that + dereference is the whole reason an annotated tag is recognised: for one, + `git rev-parse refs/tags/X` is the TAG OBJECT, not the commit, so a comparison + that forgot to dereference would miss a released commit entirely. + """ + + def test_an_annotated_tag_on_head_is_already_released(self, tmp_path): + repo = _repo(tmp_path) + _git(repo, "tag", "-a", "v2026.10.09", "-m", "Release v2026.10.09") + + result, values = _resolve_in(repo) + + assert result.returncode == 0, result.stderr + assert values["SOURCE"] == "released" + assert values["RELEASE"] == "no" + assert "nothing to release" in result.stderr + # premise: the tag object really is not the commit + assert _git(repo, "rev-parse", "v2026.10.09").strip() != _git( + repo, "rev-parse", "HEAD" + ).strip() + + def test_a_lightweight_tag_on_head_is_also_already_released(self, tmp_path): + repo = _repo(tmp_path) + _git(repo, "tag", "v2026.10.09") + + _result, values = _resolve_in(repo) + + assert values["SOURCE"] == "released" + assert values["RELEASE"] == "no" + + def test_a_tag_on_an_older_commit_falls_back_to_the_date(self, tmp_path): + repo = _repo(tmp_path, commits=("the released work", "later work")) + _git(repo, "tag", "-a", "v2026.10.09", "-m", "Release v2026.10.09", "HEAD~1") + + _result, values = _resolve_in(repo) + + assert values["VERSION"] == TODAY + assert values["SOURCE"] == "date" + assert values["BUMP"] == "yes" + assert values["RELEASE"] == "yes" + + def test_no_tags_at_all_means_the_manifest_wins(self, tmp_path): + repo = _repo(tmp_path, version="2026.10.10") + + _result, values = _resolve_in(repo) + + assert values["VERSION"] == "2026.10.10" + assert values["SOURCE"] == "manifest" + assert values["BUMP"] == "no" + + def test_the_same_day_scan_walks_real_tags(self, tmp_path): + repo = _repo(tmp_path, commits=("released", "more")) + for tag in ("v2026.10.09", f"v{TODAY}", f"v{TODAY}.2"): + _git(repo, "tag", "-a", tag, "-m", f"Release {tag}", "HEAD~1") + + _result, values = _resolve_in(repo) + + assert values["VERSION"] == f"{TODAY}.3" + assert values["SOURCE"] == "same-day" + + def test_a_gap_in_the_suffixes_is_reused(self, tmp_path): + repo = _repo(tmp_path, commits=("released", "more")) + for tag in ("v2026.10.09", f"v{TODAY}", f"v{TODAY}.3"): + _git(repo, "tag", "-a", tag, "-m", f"Release {tag}", "HEAD~1") + + _result, values = _resolve_in(repo) + + assert values["VERSION"] == f"{TODAY}.2" + + def test_the_guard_has_a_script_side_layer(self, tmp_path): + """Even with the job guard bypassed, a released commit releases nothing. + + The job-level `if:` cannot be executed here (no Gitea runner), but the + decision it protects can: this is the layer that makes a bypass harmless. + """ + repo = _repo(tmp_path) + _git(repo, "tag", "-a", "v2026.10.09", "-m", "Release v2026.10.09") + # a re-run of the same push, guard or no guard + for _attempt in range(2): + _result, values = _resolve_in(repo) + assert values["RELEASE"] == "no" + assert values["BUMP"] == "no" + + +def _step_script(name: str) -> str: + """The `run:` body of one workflow step, read by hand. + + The fast suite has no YAML parser (the verify command parses the file with + pyyaml instead), and executing these steps is worth more than matching their + text, so the block is taken verbatim and dedented. + """ + lines = WORKFLOW.read_text(encoding="utf-8").splitlines() + start = next(i for i, line in enumerate(lines) if line.strip() == f"- name: {name}") + run_at = next( + i for i in range(start, len(lines)) if lines[i].strip() in {"run: |", "run: >-"} + ) + indent = len(lines[run_at + 1]) - len(lines[run_at + 1].lstrip()) + body = [] + for line in lines[run_at + 1 :]: + if line.strip() and not line.startswith(" " * indent): + break + body.append(line[indent:] if len(line) >= indent else "") + return "\n".join(body) + + +def _run_step(name: str, cwd, env): + return subprocess.run( + ["bash", "-euo", "pipefail", "-c", _step_script(name)], + cwd=cwd, capture_output=True, text=True, env={**os.environ, **env}, + ) + + +class TestWorkflowStepsByExecution: + """Run the workflow's own steps against a bare origin (t44 3c). + + Textual pins cannot tell whether the bump lands on the commit the tag names; + these do, and they are what the dry run in the task output was. + """ + + VERSION = "2026.10.11" + TAG = "v2026.10.11" + + def _clone(self, tmp_path, *, protect=False): + origin = tmp_path / "origin.git" + subprocess.run( + ["git", "init", "--bare", "-q", "-b", "master", str(origin)], check=True + ) + work = tmp_path / "work" + subprocess.run(["git", "clone", "-q", str(origin), str(work)], check=True) + _git(work, "config", "user.email", "dev@example.com") + _git(work, "config", "user.name", "dev") + manifest = work / MANIFEST_REL + manifest.parent.mkdir(parents=True, exist_ok=True) + manifest.write_text( + json.dumps({"domain": "nvidia_shield", "version": "2026.10.08"}) + "\n" + ) + _git(work, "add", "-A") + _git(work, "commit", "-qm", "a feature commit") + _git(work, "push", "-q", "origin", "master") + if protect: + hook = origin / "hooks" / "pre-receive" + hook.write_text("#!/bin/sh\necho 'protected branch' >&2\nexit 1\n") + hook.chmod(0o755) + return work + + def _env(self): + return { + "MANIFEST": MANIFEST_REL, + "VERSION": self.VERSION, + "TAG": self.TAG, + } + + def test_the_bump_verify_and_tag_steps_agree_on_one_commit(self, tmp_path): + work = self._clone(tmp_path) + + bump = _run_step("Write the version back into the manifest", work, self._env()) + assert bump.returncode == 0, bump.stderr + assert _git(work, "log", "-1", "--pretty=%s").strip() == ( + f"chore(release): {self.TAG} [skip ci]" + ) + + verify = _run_step("Verify the released manifest on master", work, self._env()) + assert verify.returncode == 0, verify.stderr + assert f"matching {self.TAG}" in verify.stdout + + tag = _run_step("Create tag", work, self._env()) + assert tag.returncode == 0, tag.stderr + + head = _git(work, "rev-parse", "HEAD").strip() + assert _git(work, "rev-parse", f"{self.TAG}^{{commit}}").strip() == head + tagged_manifest = json.loads(_git(work, "show", f"{self.TAG}:{MANIFEST_REL}")) + assert tagged_manifest["version"] == self.VERSION + assert _git(work, "ls-remote", "--tags", "origin", self.TAG).split()[1] == ( + f"refs/tags/{self.TAG}" + ) + + def test_the_verify_step_refuses_an_inconsistent_manifest(self, tmp_path): + work = self._clone(tmp_path) + _run_step("Write the version back into the manifest", work, self._env()) + # someone else's release lands on master before this one is verified + (work / MANIFEST_REL).write_text( + json.dumps({"domain": "nvidia_shield", "version": "2026.10.99"}) + "\n" + ) + _git(work, "add", "-A") + _git(work, "commit", "-qm", "a competing release") + _git(work, "push", "-q", "origin", "master") + + verify = _run_step("Verify the released manifest on master", work, self._env()) + + assert verify.returncode == 1 + assert "inconsistent" in verify.stderr + assert "2026.10.99" in verify.stderr + + def test_the_notes_range_describes_the_developers_commits(self, tmp_path): + """t44 R1, executed: the range the notes use must not open with the bot commit. + + With a write-back the tag IS the bot's commit, so `PREV..TAG` (the range + before this fix) published "- chore(release): vX [skip ci]" as the first + line of every release. The range is built from ``BUMP``, and this runs both + halves through real git. + """ + work = self._clone(tmp_path) + _git(work, "tag", "-a", "v2026.10.08", "-m", "previous release") + _git(work, "commit", "--allow-empty", "-qm", "feat: the real change") + _run_step("Write the version back into the manifest", work, self._env()) + _run_step("Create tag", work, self._env()) + + def subjects(rev_range): + return _git(work, "log", "--pretty=format:- %s", rev_range) + + notes_end = f"{self.TAG}^" # BUMP=yes: the write-back commit is skipped + prev = _git(work, "describe", "--tags", "--abbrev=0", f"{notes_end}^").strip() + + assert prev == "v2026.10.08" + assert subjects(f"{prev}..{notes_end}") == "- feat: the real change" + # ...and the range this replaced really did include the bot's own subject + assert "- chore(release):" in subjects(f"{prev}..{self.TAG}") + + def test_a_rejected_bump_push_fails_and_tags_nothing(self, tmp_path): + work = self._clone(tmp_path, protect=True) + + bump = _run_step("Write the version back into the manifest", work, self._env()) + + assert bump.returncode == 1 + assert "could not push" in bump.stderr + assert "manually" in bump.stderr + assert MANIFEST_REL in bump.stderr + assert _git(work, "tag", "-l").strip() == ""