fix: a version collision published nothing and said nothing

.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

.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"

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 <base-cargo.toml> <head-cargo.toml>
+
+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<version>` 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} <base-cargo.toml> <head-cargo.toml>", 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))

test/test_publish_decision.py

@@ -0,0 +1,156 @@
+"""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 os
+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,
+        # 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)},
+    )
+
+
+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