From ef8ebd0b20320819b15a80d3e0ab504044745e56 Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Wed, 9 Sep 2026 16:03:11 +0000 Subject: [PATCH 1/2] fix: a version collision published nothing and said nothing #591 and #593 both bumped to 0.36.0. Git merged that without a conflict -- picking the same number means both sides wrote the identical version line, and the identical `## [0.36.0]` heading, so there was nothing to resolve -- and the second merge published nothing at all. `publish.yml` found the tag and reported `nothing to publish`, which is also what it says for every ordinary push, so the fix landed on main released nowhere with no check red and no line to act on. Two guards, because one place can see it and the other cannot. `scripts/version_untaken.py` runs on the pull request, and it is the one that would have caught #591: compare the version at `pull_request.base.sha` with the branch's, and if the branch proposes a bump, refuse a version that is already tagged. `base.sha` is the base as of the pull request rather than the moving tip of main -- for #591 it was 908a24e, before the rival release, where the version was still 0.35.0 -- so the comparison sees 0.35.0 -> 0.36.0 and asks the only question that matters about it. `test_the_collision_that_happened_is_refused` replays #591's own two manifests rather than a hand-built imitation. `publish.yml` gets the narrower half, and the commit message should be honest about how narrow. It now separates a re-run over the commit it already published (skip, idempotent) from a push that moves the version *onto* somebody else's tag (error, with the reason). That second arm would **not** have caught #591: by then the merge carried the same version as its first parent, so there was no bump left to see. `test_the_case_that_needs_the_pull_request_guard` pins that fact rather than leaving it as a claim in a comment. The decision step is extracted from the workflow and run as the shell it is, so the four arms are tested against the real text instead of a reimplementation that would pass while the original rotted. Claude-Session: https://claude.ai/code/session_01XvY78XEnrkzNjxZE5EtnyW --- .github/workflows/ci.yml | 19 +++++ .github/workflows/publish.yml | 40 +++++++-- scripts/version_untaken.py | 128 ++++++++++++++++++++++++++++ test/test_publish_decision.py | 152 ++++++++++++++++++++++++++++++++++ test/test_version_untaken.py | 127 ++++++++++++++++++++++++++++ 5 files changed, 460 insertions(+), 6 deletions(-) create mode 100755 scripts/version_untaken.py create mode 100644 test/test_publish_decision.py create mode 100644 test/test_version_untaken.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 9f4a2ab1..cdb00663 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -105,6 +105,25 @@ jobs: git show "$BASE_SHA:CHANGELOG.md" > "$RUNNER_TEMP/base-CHANGELOG.md" python3 scripts/changelog_frozen.py \ "$RUNNER_TEMP/base-CHANGELOG.md" CHANGELOG.md + # blooop/devlaunch#591. Two branches bumped to 0.36.0 independently and git + # merged it silently, because the same number on both sides is the identical + # edit and there is nothing to conflict on. The second merge then published + # nothing. This is the last point where the two versions still differ: by the + # time the merge is on `main` its version equals its first parent's, so + # `publish.yml` has nothing to notice. + # + # Same `BASE_SHA` the step above fetched, which is the base as of the pull + # request rather than the moving tip of `main` -- for #591 that was the commit + # before the rival release landed, where the version was still 0.35.0. + - name: A version that is already published is not one to bump to + if: github.event_name == 'pull_request' + env: + BASE_SHA: ${{ github.event.pull_request.base.sha }} + run: | + set -euo pipefail + git show "$BASE_SHA:rust/Cargo.toml" > "$RUNNER_TEMP/base-Cargo.toml" + python3 scripts/version_untaken.py \ + "$RUNNER_TEMP/base-Cargo.toml" rust/Cargo.toml # Flagged `python` since #294, so it is reported as what it is — the # harness, the doc guards and `scripts/` — rather than as "the project's # coverage". The shipped crates are the `rust` flag, uploaded by the diff --git a/.github/workflows/publish.yml b/.github/workflows/publish.yml index cf0252e9..ed57d465 100644 --- a/.github/workflows/publish.yml +++ b/.github/workflows/publish.yml @@ -49,18 +49,46 @@ jobs: fi echo "version=$version" >> "$GITHUB_OUTPUT" - # Already released: a re-run of this workflow over the same commit, or a - # later push that did not touch the version. Nothing to do, and saying - # so beats a failed upload. + git show HEAD^:rust/Cargo.toml > /tmp/previous-cargo.toml 2>/dev/null || : > /tmp/previous-cargo.toml + previous=$(read_version /tmp/previous-cargo.toml) + + # Already tagged. Existence is asked of the ref alone, never of the commit + # behind it: this is a depth-2 clone, so a tag pointing anywhere but here + # may have no commit object to resolve, and a failed resolve must not read + # as "free to publish". if git rev-parse -q --verify "refs/tags/v$version" >/dev/null; then + tagged_at=$(git rev-parse -q --verify "refs/tags/v$version^{commit}" || true) + if [ -n "$tagged_at" ] && [ "$tagged_at" = "$(git rev-parse HEAD)" ]; then + # The tag is on this very commit: a re-run of the workflow over the + # release it already published. `gh release create --target + # $GITHUB_SHA` is what puts it here, and idempotence is the point. + echo "v$version was published from this commit; nothing to publish" + echo "publish=false" >> "$GITHUB_OUTPUT" + exit 0 + fi + if [ "$version" != "$previous" ]; then + # This push moved the version *onto* a number somebody else already + # released. Two branches cut the same one, which git merges with no + # conflict because the same version line on both sides is one edit + # (blooop/devlaunch#591). Refusing to publish is right either way; the + # difference is that this says why, instead of reporting the same + # "nothing to publish" an ordinary push gets -- which is how #591 sat + # merged and released nowhere with nothing red. + # + # Narrow on purpose. It fires only when the version changed against the + # first parent, and #591's own merge did not: both sides were already + # 0.36.0, so there was no bump left to see by then. That case is caught + # a step earlier, by `scripts/version_untaken.py` on the pull request. + echo "::error::this push moves the version to $version, but v$version is already tagged at ${tagged_at:-another commit}. Two branches cut the same release; bump to an unused version and re-file the CHANGELOG entry under it." + exit 1 + fi + # A later push that did not touch the version. Nothing to do, and saying + # so beats a failed upload. echo "v$version is already tagged; nothing to publish" echo "publish=false" >> "$GITHUB_OUTPUT" exit 0 fi - git show HEAD^:rust/Cargo.toml > /tmp/previous-cargo.toml 2>/dev/null || : > /tmp/previous-cargo.toml - previous=$(read_version /tmp/previous-cargo.toml) - if [ "$version" = "$previous" ]; then echo "version is still ${version:-unset}; nothing to publish" echo "publish=false" >> "$GITHUB_OUTPUT" diff --git a/scripts/version_untaken.py b/scripts/version_untaken.py new file mode 100755 index 00000000..4b83f413 --- /dev/null +++ b/scripts/version_untaken.py @@ -0,0 +1,128 @@ +#!/usr/bin/env python3 +"""A branch may not propose a version that has already been published. + +blooop/devlaunch#591. Two branches bumped to 0.36.0 independently: #593 cut it and +published, and #591 -- open across that release -- carried a `release: 0.36.0` +commit of its own. Git merged the collision *silently*, because picking the same +number means both sides made the identical edit: one `version = "0.36.0"` line in +`rust/Cargo.toml` with nothing to resolve, and one byte-identical +`## [0.36.0]` heading in CHANGELOG.md with the differing bodies merged in +underneath as separate additions. Choosing 0.37.0 instead would have conflicted +loudly in `Cargo.toml` and been caught at the merge. + +The cost is that the second merge publishes **nothing**. `publish.yml` sees the +version already tagged and reports `nothing to publish`, which is also what it +says for every ordinary push, so the fix sat merged and unreleased with no check +red and no line in any log to act on. + +**Why this runs at pull-request time and not at publish time.** By the time the +merge reaches `main` the collision is no longer visible: the merge commit's +version equals its first parent's -- both are 0.36.0 -- so "did this push change +the version" is false and there is nothing for `publish.yml` to notice. The +branch is the last place the two numbers still differ. + +**Why comparing against the base commit is enough.** `pull_request.base.sha` is +the base as of the pull request, not the moving tip of `main`: for #591 it was +`908a24e`, the commit before #593 landed, where the version was still 0.35.0. So +the comparison sees 0.35.0 -> 0.36.0, a proposed bump, and asks the one question +worth asking about it -- is `v0.36.0` already out there? + +Usage: + version_untaken.py + +Exits 0 when the branch proposes no new version, or proposes one that has never +been tagged. Exits 1 when it proposes a version that is already published, and 1 +(never 0) when either side cannot be read -- a guard that cannot read its input +has checked nothing. +""" + +import re +import subprocess +import sys +from pathlib import Path + +# `[workspace.package] version` is the first `version = "..."` line in the file; +# the crates inherit it with `version.workspace = true`. `publish.yml` and +# `conda-publish.yml` both read it exactly this way -- keep the three in step. +VERSION = re.compile(r'^version = "(.*)"', re.MULTILINE) + + +class Unreadable(Exception): + """The manifest is not one this guard can take a version out of.""" + + +def version_in(path: Path, where: str) -> str: + try: + text = path.read_text(encoding="utf-8") + except OSError as problem: + raise Unreadable(f"{where}: cannot be read ({problem})") from problem + found = VERSION.search(text) + if not found: + raise Unreadable(f"{where}: no 'version = \"...\"' line") + return found.group(1) + + +def is_tagged(version: str, run=subprocess.run) -> bool | None: + """Whether `v` already names a commit. + + `None` is "could not tell" -- no git, or a git that failed for a reason of its + own -- and the caller refuses on it. A missing tag is a confident `False` and + the ordinary answer for a release being cut. + """ + try: + done = run( + ["git", "rev-parse", "-q", "--verify", f"refs/tags/v{version}^{{commit}}"], + capture_output=True, + text=True, + check=False, + ) + except OSError: + return None + if done.returncode == 0: + return True + # `rev-parse -q --verify` exits 1 for "no such ref" and reserves other codes + # for being unable to answer, which is not the same thing and must not read as + # "free to publish". + return False if done.returncode == 1 else None + + +def main(argv: list[str]) -> int: + if len(argv) != 3: + print(f"usage: {Path(argv[0]).name} ", file=sys.stderr) + return 2 + try: + base = version_in(Path(argv[1]), "base rust/Cargo.toml") + head = version_in(Path(argv[2]), "head rust/Cargo.toml") + except Unreadable as problem: + print(f"the version could not be checked, so it is not passing: {problem}", file=sys.stderr) + return 1 + + if base == head: + print(f"this branch proposes no new version (still {head})") + return 0 + + taken = is_tagged(head) + if taken is None: + print( + f"could not ask git whether v{head} is already tagged, so this is not passing", + file=sys.stderr, + ) + return 1 + if taken: + print( + f"this branch bumps {base} -> {head}, and v{head} is already tagged.\n\n" + f"Someone else cut {head} while this branch was open, so merging it would\n" + f"publish nothing: `publish.yml` finds the tag, reports 'nothing to publish',\n" + f"and the work lands on main released nowhere. Git will not catch it for you --\n" + f"both sides wrote the same version line, so there is nothing to conflict on.\n\n" + f"Bump to the next unused version instead, and check that the CHANGELOG entry\n" + f"is under that heading rather than under {head}'s.\n", + file=sys.stderr, + ) + return 1 + print(f"this branch bumps {base} -> {head}, which has never been tagged") + return 0 + + +if __name__ == "__main__": + sys.exit(main(sys.argv)) diff --git a/test/test_publish_decision.py b/test/test_publish_decision.py new file mode 100644 index 00000000..c2443860 --- /dev/null +++ b/test/test_publish_decision.py @@ -0,0 +1,152 @@ +"""The four answers `publish.yml` can give, exercised as the shell it actually is. + +The step decides whether a push to `main` is a release, and it now has four arms +rather than two. Three of them decline to publish and one of those is an *error*, +which is the whole point of the change: blooop/devlaunch#591 merged a version bump +onto a number another branch had already released, and the workflow reported +`nothing to publish` -- the same sentence an ordinary push gets -- so the work sat +on main released nowhere with no check red. + +The script is extracted from the workflow and run, rather than reimplemented here. +A rewritten copy would be a second hand-maintained version of the logic and would +pass while the real one rotted; running the real text means the test fails if the +step is edited into disagreeing with it. + +What this does **not** claim: that the error arm would have caught #591. It would +not have, and `test_the_case_that_needs_the_pull_request_guard` pins that instead. +By the time #591 reached main its merge carried the same version as its first +parent, so there was no bump left to see. `scripts/version_untaken.py` is the guard +for that, and it runs on the pull request. +""" + +import subprocess +from pathlib import Path + +import pytest + +ROOT = Path(__file__).parent.parent +PUBLISH = ROOT / ".github" / "workflows" / "publish.yml" +STEP = " - name: Decide whether there is a release to publish\n" + + +def decide_script() -> str: + """The `run:` body of the decision step, dedented out of the workflow.""" + text = PUBLISH.read_text(encoding="utf-8") + assert STEP in text, "the decision step is not named what this test looks for" + body = text.split(STEP, 1)[1].split("run: |\n", 1)[1] + lines = [] + for line in body.splitlines(): + if line.strip() and not line.startswith(" " * 10): + break + lines.append(line[10:]) + return "\n".join(lines) + + +def git(*args: str, cwd: Path) -> None: + done = subprocess.run(("git", *args), cwd=cwd, capture_output=True, text=True, check=False) + assert done.returncode == 0, f"git {' '.join(args)} failed: {done.stderr}" + + +@pytest.fixture(name="repo") +def a_repository_to_decide_about(tmp_path: Path) -> Path: + work = tmp_path / "repo" + (work / "rust").mkdir(parents=True) + git("init", "-q", "-b", "main", ".", cwd=work) + git("config", "user.email", "publish@example.invalid", cwd=work) + git("config", "user.name", "Publish Fixture", cwd=work) + return work + + +def commit(repo: Path, version: str) -> None: + (repo / "rust" / "Cargo.toml").write_text( + f'[workspace.package]\nversion = "{version}"\n', encoding="utf-8" + ) + git("add", "-A", cwd=repo) + git("commit", "-qm", f"v{version}", cwd=repo) + + +def decide(repo: Path, tmp_path: Path) -> subprocess.CompletedProcess: + output = tmp_path / "github-output" + output.write_text("", encoding="utf-8") + script = tmp_path / "decide.sh" + script.write_text(decide_script(), encoding="utf-8") + return subprocess.run( + ("bash", str(script)), + cwd=repo, + capture_output=True, + text=True, + check=False, + env={"PATH": "/usr/bin:/bin:/usr/local/bin", "GITHUB_OUTPUT": str(output)}, + ) + + +def test_a_bump_to_a_version_nobody_has_taken_publishes(repo, tmp_path): + commit(repo, "0.36.0") + commit(repo, "0.37.0") + + done = decide(repo, tmp_path) + + assert done.returncode == 0, done.stderr + assert "publishing" in done.stdout + + +def test_a_re_run_over_the_commit_that_published_declines_quietly(repo, tmp_path): + """Idempotence, and the ordinary reason the tag is found at all. + + `gh release create --target $GITHUB_SHA` puts the tag on the commit that + published, so a re-run sees it on HEAD and must do nothing rather than fail. + """ + commit(repo, "0.36.0") + commit(repo, "0.37.0") + git("tag", "v0.37.0", cwd=repo) + + done = decide(repo, tmp_path) + + assert done.returncode == 0, done.stderr + assert "published from this commit" in done.stdout + + +def test_a_later_push_that_did_not_touch_the_version_declines_quietly(repo, tmp_path): + commit(repo, "0.36.0") + commit(repo, "0.37.0") + git("tag", "v0.37.0", cwd=repo) + (repo / "unrelated.txt").write_text("a change that is not a release\n", encoding="utf-8") + git("add", "-A", cwd=repo) + git("commit", "-qm", "later", cwd=repo) + + done = decide(repo, tmp_path) + + assert done.returncode == 0, done.stderr + assert "already tagged; nothing to publish" in done.stdout + + +def test_a_bump_onto_a_version_already_tagged_is_an_error_not_a_shrug(repo, tmp_path): + """The arm that is new: declining, but loudly enough to act on.""" + commit(repo, "0.36.0") + git("tag", "v0.37.0", cwd=repo) + commit(repo, "0.37.0") + + done = decide(repo, tmp_path) + + assert done.returncode == 1 + assert "::error::" in done.stdout + assert "already tagged" in done.stdout + + +def test_the_case_that_needs_the_pull_request_guard(repo, tmp_path): + """#591's shape: the merge carries the version its first parent already had. + + Nothing here can see a bump, so this step correctly says nothing to publish and + correctly does not error. That silence is exactly why the collision has to be + refused on the branch instead, which is `scripts/version_untaken.py`. + """ + commit(repo, "0.36.0") + git("tag", "v0.36.0", cwd=repo) + (repo / "unrelated.txt").write_text("the merge's other side\n", encoding="utf-8") + git("add", "-A", cwd=repo) + git("commit", "-qm", "a merge carrying the same version", cwd=repo) + + done = decide(repo, tmp_path) + + assert done.returncode == 0, done.stderr + assert "::error::" not in done.stdout diff --git a/test/test_version_untaken.py b/test/test_version_untaken.py new file mode 100644 index 00000000..fdcd710a --- /dev/null +++ b/test/test_version_untaken.py @@ -0,0 +1,127 @@ +"""The guard that refuses a bump onto a version somebody already published. + +blooop/devlaunch#591. Two branches bumped to 0.36.0 independently: #593 cut it and +published, and #591 -- open across that release -- carried its own +`release: 0.36.0`. Git merged the collision without a conflict, because picking +the same number means both sides made the identical edit, and the second merge +published nothing at all: `publish.yml` found the tag, said `nothing to publish` +(what it says for every ordinary push too), and the fix landed on main released +nowhere with no check red. + +The tests below are written against that history rather than around it. +`the_collision_that_happened_is_refused` replays #591's own two manifests, so what +is asserted is the real event and not a hand-built imitation of it. + +**Why this is a pull-request guard.** By the time the merge reaches `main` the +collision is invisible: the merge commit's version equals its first parent's -- +both 0.36.0 -- so nothing downstream can see a bump to notice. The branch is the +last place the two numbers still differ, which is why +`a_merge_commit_cannot_see_the_collision_its_branch_could` pins that fact directly +against the history rather than leaving it as a claim in a comment. +""" + +import subprocess +import sys +from pathlib import Path + +ROOT = Path(__file__).parent.parent +GUARD = (ROOT / "scripts" / "version_untaken.py").resolve() +CI = ROOT / ".github" / "workflows" / "ci.yml" + +# The two commits of #591, and the release that beat it to the number. +BASE_OF_591 = "908a24ee21a14207e150071d3af65c5366da13c7" +HEAD_OF_591 = "d318cad8bb6d9c5e7f7870c6bcbfbe88a3fb002b" +MERGE_OF_591 = "84cfaf13792f17c251e183306bd65903919c8bc6" + + +def run(*args: str, cwd: Path | None = None) -> subprocess.CompletedProcess: + return subprocess.run(args, cwd=cwd, capture_output=True, text=True, check=False) + + +def manifest(version: str) -> str: + return f'[workspace.package]\nversion = "{version}"\nedition = "2024"\n' + + +def check(base: str, head: str, tmp_path: Path) -> subprocess.CompletedProcess: + (tmp_path / "base.toml").write_text(base, encoding="utf-8") + (tmp_path / "head.toml").write_text(head, encoding="utf-8") + return run( + sys.executable, + str(GUARD), + str(tmp_path / "base.toml"), + str(tmp_path / "head.toml"), + cwd=ROOT, + ) + + +def at(commit: str, path: str) -> str: + done = run("git", "show", f"{commit}:{path}", cwd=ROOT) + assert done.returncode == 0, f"git show {commit}:{path} failed: {done.stderr}" + return done.stdout + + +def test_the_collision_that_happened_is_refused(tmp_path): + """#591's own manifests: 0.35.0 at its base, 0.36.0 at its head, v0.36.0 taken.""" + done = check(at(BASE_OF_591, "rust/Cargo.toml"), at(HEAD_OF_591, "rust/Cargo.toml"), tmp_path) + + assert done.returncode == 1 + assert "v0.36.0 is already tagged" in done.stderr + assert "publish nothing" in done.stderr + + +def test_a_merge_commit_cannot_see_the_collision_its_branch_could(tmp_path): + """Why the guard runs on the branch and not after the merge. + + The merge's version and its first parent's are the same string, so every + "did this push bump the version" test downstream answers no. There is nothing + left to catch by then, which is the whole argument for checking earlier. + """ + merged = at(MERGE_OF_591, "rust/Cargo.toml") + first_parent = at(f"{MERGE_OF_591}^", "rust/Cargo.toml") + + assert 'version = "0.36.0"' in merged + assert 'version = "0.36.0"' in first_parent + + done = check(first_parent, merged, tmp_path) + assert done.returncode == 0 + assert "proposes no new version" in done.stdout + + +def test_an_ordinary_branch_that_never_touches_the_version_passes(tmp_path): + done = check(manifest("0.37.0"), manifest("0.37.0"), tmp_path) + + assert done.returncode == 0, done.stderr + assert "proposes no new version" in done.stdout + + +def test_a_release_cut_to_a_version_nobody_has_taken_passes(tmp_path): + """The ritual this guard must not break, for a version that cannot be tagged.""" + done = check(manifest("0.37.0"), manifest("99999.0.0"), tmp_path) + + assert done.returncode == 0, done.stderr + assert "never been tagged" in done.stdout + + +def test_a_manifest_with_no_version_fails_rather_than_passes(tmp_path): + """A guard that cannot read its input has checked nothing.""" + done = check("[workspace.package]\n", manifest("99999.0.0"), tmp_path) + + assert done.returncode == 1 + assert "not passing" in done.stderr + + +def test_a_manifest_that_is_not_there_fails_rather_than_passes(tmp_path): + done = run( + sys.executable, str(GUARD), str(tmp_path / "absent.toml"), str(tmp_path / "also.toml") + ) + + assert done.returncode == 1 + assert "not passing" in done.stderr + + +def test_ci_runs_the_guard_on_pull_requests(): + """It is worth a step only if a step actually runs it.""" + ci = CI.read_text(encoding="utf-8") + + assert "scripts/version_untaken.py" in ci + assert "A version that is already published is not one to bump to" in ci From fbe0e61464dcf3ce7ecd46ae4737cb039d0d3568 Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Wed, 9 Sep 2026 16:18:06 +0000 Subject: [PATCH 2/2] fix: two guard tests read history a shallow CI checkout does not have `test_the_collision_that_happened_is_refused` and its neighbour replayed #591's real manifests by SHA, which fails everywhere but a full clone: `actions/checkout` fetches depth 1, so `git show 908a24e:rust/Cargo.toml` is `fatal: path exists on disk, but not in ...`. Reading the real commits was wrong for a second reason the failure did not show. The guard's oracle is `git rev-parse refs/tags/v` in whatever directory it runs in, and those tests ran it in the checkout -- which has not fetched tags by the time pytest runs, since `pixi run ci` is the step before the one that fetches them. The collision test would have gone green by the guard finding no tag and permitting the bump, which is the opposite of what it asserts. Each test builds its own repository and tags it now. The numbers and the shape are #591's still; only the bytes are the test's own. The module docstring said the replay was the point, so it says the opposite now and why. Also stops `test_publish_decision.py` hardcoding `PATH` for the shell it runs: the script needs `git` and `sed`, and naming three directories is a test that passes here and fails on a runner that puts them elsewhere. Claude-Session: https://claude.ai/code/session_01XvY78XEnrkzNjxZE5EtnyW --- test/test_publish_decision.py | 6 +- test/test_version_untaken.py | 109 ++++++++++++++++++++-------------- 2 files changed, 68 insertions(+), 47 deletions(-) diff --git a/test/test_publish_decision.py b/test/test_publish_decision.py index c2443860..4678b097 100644 --- a/test/test_publish_decision.py +++ b/test/test_publish_decision.py @@ -19,6 +19,7 @@ for that, and it runs on the pull request. """ +import os import subprocess from pathlib import Path @@ -76,7 +77,10 @@ def decide(repo: Path, tmp_path: Path) -> subprocess.CompletedProcess: capture_output=True, text=True, check=False, - env={"PATH": "/usr/bin:/bin:/usr/local/bin", "GITHUB_OUTPUT": str(output)}, + # The runner's own PATH, not a guessed one: the script needs `git` and + # `sed`, and hardcoding where they live is a test that passes here and + # fails on a machine that puts them somewhere else. + env={"PATH": os.environ["PATH"], "GITHUB_OUTPUT": str(output)}, ) diff --git a/test/test_version_untaken.py b/test/test_version_untaken.py index fdcd710a..aa8a034b 100644 --- a/test/test_version_untaken.py +++ b/test/test_version_untaken.py @@ -8,41 +8,58 @@ (what it says for every ordinary push too), and the fix landed on main released nowhere with no check red. -The tests below are written against that history rather than around it. -`the_collision_that_happened_is_refused` replays #591's own two manifests, so what -is asserted is the real event and not a hand-built imitation of it. - -**Why this is a pull-request guard.** By the time the merge reaches `main` the -collision is invisible: the merge commit's version equals its first parent's -- -both 0.36.0 -- so nothing downstream can see a bump to notice. The branch is the -last place the two numbers still differ, which is why -`a_merge_commit_cannot_see_the_collision_its_branch_could` pins that fact directly -against the history rather than leaving it as a claim in a comment. +Each test builds its own repository and its own tag rather than reading the real +history back. The first draft did read it -- #591's actual two manifests by SHA -- +and it was wrong twice over: CI checks out shallow, so those commits are not +there, and the guard's oracle is `git rev-parse refs/tags/v` in whatever +directory it runs in, so it would have been asking the checkout about tags the +checkout may not have fetched either. What is worth holding is the shape and the +numbers, and a repository built here has both under the test's own control. """ import subprocess import sys from pathlib import Path +import pytest + ROOT = Path(__file__).parent.parent GUARD = (ROOT / "scripts" / "version_untaken.py").resolve() CI = ROOT / ".github" / "workflows" / "ci.yml" -# The two commits of #591, and the release that beat it to the number. -BASE_OF_591 = "908a24ee21a14207e150071d3af65c5366da13c7" -HEAD_OF_591 = "d318cad8bb6d9c5e7f7870c6bcbfbe88a3fb002b" -MERGE_OF_591 = "84cfaf13792f17c251e183306bd65903919c8bc6" - def run(*args: str, cwd: Path | None = None) -> subprocess.CompletedProcess: return subprocess.run(args, cwd=cwd, capture_output=True, text=True, check=False) +def git(*args: str, cwd: Path) -> None: + done = run("git", *args, cwd=cwd) + assert done.returncode == 0, f"git {' '.join(args)} failed: {done.stderr}" + + def manifest(version: str) -> str: return f'[workspace.package]\nversion = "{version}"\nedition = "2024"\n' -def check(base: str, head: str, tmp_path: Path) -> subprocess.CompletedProcess: +# Named through the decorator so the fixture function and the parameter that +# receives it are not one name in one module, which pylint reads as shadowing. +@pytest.fixture(name="released") +def a_repository_with_a_release_already_out(tmp_path: Path) -> Path: + """A repository where v0.36.0 has been cut and tagged, as #593 left main.""" + work = tmp_path / "repo" + work.mkdir() + git("init", "-q", "-b", "main", ".", cwd=work) + git("config", "user.email", "version@example.invalid", cwd=work) + git("config", "user.name", "Version Fixture", cwd=work) + (work / "seed.txt").write_text("the release\n", encoding="utf-8") + git("add", "-A", cwd=work) + git("commit", "-qm", "cut 0.36.0", cwd=work) + git("tag", "v0.36.0", cwd=work) + return work + + +def check(base: str, head: str, tmp_path: Path, cwd: Path) -> subprocess.CompletedProcess: + """Run the guard in `cwd`, which is the repository whose tags it will consult.""" (tmp_path / "base.toml").write_text(base, encoding="utf-8") (tmp_path / "head.toml").write_text(head, encoding="utf-8") return run( @@ -50,69 +67,69 @@ def check(base: str, head: str, tmp_path: Path) -> subprocess.CompletedProcess: str(GUARD), str(tmp_path / "base.toml"), str(tmp_path / "head.toml"), - cwd=ROOT, + cwd=cwd, ) -def at(commit: str, path: str) -> str: - done = run("git", "show", f"{commit}:{path}", cwd=ROOT) - assert done.returncode == 0, f"git show {commit}:{path} failed: {done.stderr}" - return done.stdout - +def test_the_collision_that_happened_is_refused(released, tmp_path): + """#591's numbers: 0.35.0 at its base, 0.36.0 at its head, v0.36.0 already out. -def test_the_collision_that_happened_is_refused(tmp_path): - """#591's own manifests: 0.35.0 at its base, 0.36.0 at its head, v0.36.0 taken.""" - done = check(at(BASE_OF_591, "rust/Cargo.toml"), at(HEAD_OF_591, "rust/Cargo.toml"), tmp_path) + `pull_request.base.sha` is the base as of the pull request rather than the + moving tip of main -- for #591 it was 908a24e, before the rival release, where + the version was still 0.35.0. That is why the comparison sees a bump at all. + """ + done = check(manifest("0.35.0"), manifest("0.36.0"), tmp_path, released) assert done.returncode == 1 assert "v0.36.0 is already tagged" in done.stderr assert "publish nothing" in done.stderr -def test_a_merge_commit_cannot_see_the_collision_its_branch_could(tmp_path): +def test_a_merge_commit_cannot_see_the_collision_its_branch_could(released, tmp_path): """Why the guard runs on the branch and not after the merge. - The merge's version and its first parent's are the same string, so every - "did this push bump the version" test downstream answers no. There is nothing - left to catch by then, which is the whole argument for checking earlier. + Once #591 merged, the merge commit's version and its first parent's were the + same string -- both 0.36.0 -- so every "did this push bump the version" test + downstream answers no. There is nothing left to catch by then, which is the + whole argument for checking earlier, and the reason `publish.yml`'s own error + arm is not a second answer to this case. """ - merged = at(MERGE_OF_591, "rust/Cargo.toml") - first_parent = at(f"{MERGE_OF_591}^", "rust/Cargo.toml") + done = check(manifest("0.36.0"), manifest("0.36.0"), tmp_path, released) - assert 'version = "0.36.0"' in merged - assert 'version = "0.36.0"' in first_parent - - done = check(first_parent, merged, tmp_path) - assert done.returncode == 0 + assert done.returncode == 0, done.stderr assert "proposes no new version" in done.stdout -def test_an_ordinary_branch_that_never_touches_the_version_passes(tmp_path): - done = check(manifest("0.37.0"), manifest("0.37.0"), tmp_path) +def test_an_ordinary_branch_that_never_touches_the_version_passes(released, tmp_path): + """The common case, and the one a false positive here would block outright.""" + done = check(manifest("0.36.0"), manifest("0.36.0"), tmp_path, released) assert done.returncode == 0, done.stderr - assert "proposes no new version" in done.stdout -def test_a_release_cut_to_a_version_nobody_has_taken_passes(tmp_path): - """The ritual this guard must not break, for a version that cannot be tagged.""" - done = check(manifest("0.37.0"), manifest("99999.0.0"), tmp_path) +def test_a_release_cut_to_a_version_nobody_has_taken_passes(released, tmp_path): + """The ritual this guard must not break.""" + done = check(manifest("0.36.0"), manifest("0.37.0"), tmp_path, released) assert done.returncode == 0, done.stderr assert "never been tagged" in done.stdout -def test_a_manifest_with_no_version_fails_rather_than_passes(tmp_path): +def test_a_manifest_with_no_version_fails_rather_than_passes(released, tmp_path): """A guard that cannot read its input has checked nothing.""" - done = check("[workspace.package]\n", manifest("99999.0.0"), tmp_path) + done = check("[workspace.package]\n", manifest("0.37.0"), tmp_path, released) assert done.returncode == 1 assert "not passing" in done.stderr -def test_a_manifest_that_is_not_there_fails_rather_than_passes(tmp_path): +def test_a_manifest_that_is_not_there_fails_rather_than_passes(released, tmp_path): done = run( - sys.executable, str(GUARD), str(tmp_path / "absent.toml"), str(tmp_path / "also.toml") + sys.executable, + str(GUARD), + str(tmp_path / "absent.toml"), + str(tmp_path / "also.toml"), + cwd=released, ) assert done.returncode == 1