diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index a047ffc8..3fdbae88 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -64,6 +64,28 @@ jobs: with: fetch-depth: 0 + # The change detector asks uv what the no-dev resolution was at the last + # tag. Without uv on PATH the script says "changed" and is fail-safe but + # useless, so install it before anything reads its answer. + # + # The SAME uv the images build with, read out of the Dockerfile rather + # than restated here: uv guarantees lockfile compatibility within a minor + # release, and a detector on a different one would be judging a + # resolution nobody ships. Warns rather than fails if it cannot read the + # version -- an unpinned uv still compares both refs with one binary, so + # the answer stays sound; tests/test_release_triggers.py is what keeps the + # Dockerfile readable. (CodeRabbit, PR #354.) + - name: Install uv + run: | + UV_VERSION=$(grep -oE 'astral-sh/uv:[0-9]+\.[0-9]+\.[0-9]+' Dockerfile | head -1 | cut -d: -f2) + if [ -z "$UV_VERSION" ]; then + echo "::warning::could not read the uv version from Dockerfile; installing the newest" + pip install uv --quiet + else + echo "Installing uv $UV_VERSION, the version Dockerfile builds with" + pip install "uv==$UV_VERSION" --quiet + fi + - name: Detect what changed id: changes run: | @@ -94,9 +116,33 @@ jobs: # lan_isolation, notify, ...) produced a GREEN release run that # published nothing at all: no tag, no GitHub Release, no images. # Skipped steps do not fail a run, so it looked like a success. - if echo "$CHANGED" | grep -qE '^(Dockerfile|entrypoint\.sh|pyproject\.toml|uv\.lock|app/)'; then + # uv.lock and pyproject.toml hold the DEV group too, and both images + # build with `uv sync --frozen --no-dev`. Matching on the filename + # meant a pytest or ruff bump -- the most frequent dependency PR there + # is -- cut a full release. v1.36.4 was exactly that: 78 entries in + # site-packages, not one of them different from v1.36.3, and three + # containers restarted on the fleet for a version label. + # + # So the manifests do not trigger by name. The script exports the + # no-dev resolution at both refs with the real resolver and compares, + # and every way of not knowing answers "changed". + RUNTIME_CHANGED=false + if echo "$CHANGED" | grep -qE '^(pyproject\.toml|uv\.lock)$'; then + if [ -n "$LAST_TAG" ]; then + RUNTIME_CHANGED=$(python3 scripts/runtime_deps_changed.py "$LAST_TAG" HEAD || echo true) + else + RUNTIME_CHANGED=true + fi + fi + echo "Runtime dependencies changed: $RUNTIME_CHANGED" + + if echo "$CHANGED" | grep -qE '^(Dockerfile|entrypoint\.sh|app/)'; then BUILD_UI=true fi + if [ "$RUNTIME_CHANGED" = true ]; then + BUILD_UI=true + BUILD_WORKER=true + fi if echo "$CHANGED" | grep -qE '^services/'; then BUILD_UI=true BUILD_WORKER=true @@ -111,7 +157,7 @@ jobs: WORKER_MODULES=$(grep -oE '^COPY[^#]*app/([a-z_]+)\.py' Dockerfile.worker \ | grep -oE 'app/[a-z_]+\.py' | sort -u) echo "Worker modules (from Dockerfile.worker):"; echo "$WORKER_MODULES" - if echo "$CHANGED" | grep -qE '^(Dockerfile\.worker|entrypoint\.sh|pyproject\.toml|uv\.lock)'; then + if echo "$CHANGED" | grep -qE '^(Dockerfile\.worker|entrypoint\.sh)'; then BUILD_WORKER=true fi while IFS= read -r mod; do diff --git a/scripts/runtime_deps_changed.py b/scripts/runtime_deps_changed.py new file mode 100644 index 00000000..0bad243b --- /dev/null +++ b/scripts/runtime_deps_changed.py @@ -0,0 +1,150 @@ +#!/usr/bin/env python3 +"""Did the dependencies that SHIP change between two refs? + +``uv.lock`` holds the dev group as well as the runtime one, and release.yml +treated any change to it as a reason to build. So a bump of pytest or ruff -- +the most frequent kind of dependency PR there is -- cut a full release whose +images were identical to the previous one. v1.36.4 was exactly that: 78 entries +in site-packages, zero difference from v1.36.3, three containers restarted on +the fleet for a version label. + +Both images build with ``uv sync --frozen --no-dev``, so the question that +decides a release is not "did uv.lock change" but "did the no-dev resolution +change". This answers that by exporting it at both refs with the real resolver +and comparing. + + python scripts/runtime_deps_changed.py v1.36.3 HEAD # -> false + python scripts/runtime_deps_changed.py v1.36.2 v1.36.3 # -> true + +FAIL-SAFE, and this is the whole design. Every way of not knowing -- uv is +missing, a ref does not exist, an export fails, the lock disagrees with +pyproject -- prints ``true`` and explains itself on stderr. A release that +should not have happened costs a pointless image. A release that silently did +not happen ships nothing while the run reports success, which is the failure +this repo has been bitten by before. +""" + +from __future__ import annotations + +import argparse +import pathlib +import subprocess +import sys +import tempfile + +#: What both Dockerfiles copy before `uv sync`. Nothing else feeds the resolution. +MANIFESTS = ("pyproject.toml", "uv.lock") + + +def _warn(message: str) -> None: + print(f"runtime_deps_changed: {message}", file=sys.stderr) + + +def _run(args: list[str], **kwargs) -> subprocess.CompletedProcess[str]: + """Never raise. A missing binary is an answer, not a crash. + + ``subprocess.run`` raises FileNotFoundError when the executable is absent, + which is precisely the case this script has to survive: no uv on PATH must + mean "assume it changed", not a traceback that fails the release job. + """ + try: + return subprocess.run(args, capture_output=True, text=True, timeout=180, check=False, **kwargs) + except (OSError, subprocess.SubprocessError) as exc: + return subprocess.CompletedProcess(args, returncode=127, stdout="", stderr=str(exc)) + + +def _materialise(repo: pathlib.Path, ref: str, into: pathlib.Path) -> bool: + """Write the manifests as they were at ``ref``. False if any is unreadable.""" + for name in MANIFESTS: + result = _run(["git", "-C", str(repo), "show", f"{ref}:{name}"]) + if result.returncode != 0: + _warn(f"cannot read {name} at {ref}: {result.stderr.strip()}") + return False + (into / name).write_text(result.stdout, encoding="utf-8") + return True + + +def _runtime_requirements(directory: pathlib.Path) -> set[str] | None: + """Everything the no-dev resolution pins, or None when uv cannot say. + + ``--frozen`` so uv reports a lock that disagrees with its pyproject instead + of quietly re-resolving it, which would need the network and would answer a + different question from the one the Dockerfile asks. + + HASHES ARE INCLUDED. Exporting with ``--no-hashes`` and keeping only the + ``name==version`` lines compares less than the build consumes: a lock can + gain or change an artifact for a version that already exists -- a new wheel + for a platform, a re-resolved sdist -- and every pin still reads the same + while ``uv sync --frozen`` installs something different. That returns + "unchanged" for a change that ships, which is the one direction this script + must never get wrong. (CodeRabbit, PR #354.) + + Comment lines go, and only those. uv writes the command it was run with into + the header, and that names the temp directory, so it differs on every call + by construction. The ``# via ...`` provenance notes are dropped with it; + they restate the graph the pins already describe. + """ + result = _run( + [ + "uv", + "export", + "--directory", + str(directory), + "--frozen", + "--no-dev", + "--format", + "requirements-txt", + ] + ) + if result.returncode != 0: + _warn(f"uv export failed in {directory}: {result.stderr.strip()[:400]}") + return None + return {line.strip() for line in result.stdout.splitlines() if line.strip() and not line.strip().startswith("#")} + + +def runtime_deps_changed(repo: pathlib.Path, base: str, head: str) -> bool: + with tempfile.TemporaryDirectory() as tmp: + root = pathlib.Path(tmp) + exported = [] + for ref in (base, head): + into = root / ref.replace("/", "_") + into.mkdir(parents=True, exist_ok=True) + if not _materialise(repo, ref, into): + _warn("assuming the runtime dependencies changed") + return True + requirements = _runtime_requirements(into) + if requirements is None: + _warn("assuming the runtime dependencies changed") + return True + exported.append(requirements) + + before, after = exported + if before == after: + _warn(f"{len(before)} runtime requirements, identical between {base} and {head}") + return False + + for pin in sorted(after - before): + _warn(f" + {pin}") + for pin in sorted(before - after): + _warn(f" - {pin}") + return True + + +def main(argv: list[str] | None = None) -> int: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("base", help="the ref to compare from, usually the last release tag") + parser.add_argument("head", nargs="?", default="HEAD") + parser.add_argument("--repo", default=".", help="repository root (default: cwd)") + args = parser.parse_args(argv) + + if _run(["uv", "--version"]).returncode != 0: + _warn("uv is not on PATH; assuming the runtime dependencies changed") + print("true") + return 0 + + print("true" if runtime_deps_changed(pathlib.Path(args.repo), args.base, args.head) else "false") + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/tests/test_compose_image_pins.py b/tests/test_compose_image_pins.py index c4aed8cb..28022da0 100644 --- a/tests/test_compose_image_pins.py +++ b/tests/test_compose_image_pins.py @@ -147,10 +147,17 @@ def _runs_the_drift_test(command: str) -> bool: requiring tags there would be a rule about a problem that workflow cannot have. The first version of this guard flagged it, which is how the distinction got noticed. + + Comments are stripped first. release.yml's change detector explains that + a pytest bump used to cut a release, and on the raw text that prose read + as a job that runs the suite — the guard matching someone's writing + rather than a command, which is the same way the skip-marker guard in + test_beads_batch_65.py once fooled itself. """ - if "pytest" not in command: + commands = "\n".join(line for line in command.splitlines() if not line.strip().startswith("#")) + if "pytest" not in commands: return False - return " -m " not in command and " -k " not in command + return " -m " not in commands and " -k " not in commands def _workflows_running_pytest(self): import yaml @@ -174,6 +181,11 @@ def test_a_marker_filtered_run_is_not_required_to_fetch_tags(self): assert self._runs_the_drift_test("uv run pytest") assert self._runs_the_drift_test("pytest tests/ -v --tb=short") + def test_prose_about_pytest_is_not_a_pytest_run(self): + """The control for the comment-stripping, which is the whole point.""" + assert not self._runs_the_drift_test("# a pytest bump used to cut a release\npip install uv --quiet") + assert self._runs_the_drift_test("# install first\nuv run pytest tests/") + def test_each_such_job_checks_out_with_tags(self): offenders = [] for name, doc in self._workflows_running_pytest(): diff --git a/tests/test_release_triggers.py b/tests/test_release_triggers.py index 800b640b..5343f6c4 100644 --- a/tests/test_release_triggers.py +++ b/tests/test_release_triggers.py @@ -25,10 +25,14 @@ from __future__ import annotations +import os import re +import subprocess +import sys from pathlib import Path import pytest +import yaml ROOT = Path(__file__).resolve().parents[1] RELEASE = ROOT / ".github" / "workflows" / "release.yml" @@ -188,3 +192,193 @@ def test_at_least_one_doc_states_the_precedence(self): if "only when that file is absent" in (ROOT / rel).read_text(encoding="utf-8") ] assert len(found) >= 3, f"precedence stated in only {found}" + + +class TestOnlyARealDependencyChangeBuilds: + """CashPilot-#354: a dev-tool bump cut a full release. + + ``uv.lock`` carries the dev group, both images build with + ``uv sync --frozen --no-dev``, and the trigger matched on the filename. So + v1.36.4 shipped for a pytest/pytest-asyncio/ruff bump: 78 entries in + site-packages, not one different from v1.36.3, and three containers + restarted on the fleet for a version label. Dependency PRs land weekly, so + this was the most frequent release the project cut. + + The question that decides a release is whether the no-dev resolution moved, + which is what ``scripts/runtime_deps_changed.py`` answers. + """ + + SCRIPT = ROOT / "scripts" / "runtime_deps_changed.py" + + def _detect_step(self) -> str: + doc = yaml.safe_load(RELEASE.read_text(encoding="utf-8")) + for step in doc["jobs"]["release"]["steps"]: + if step.get("name") == "Detect what changed": + return step["run"] + raise AssertionError("release.yml no longer detects what changed") + + def _answer(self, base: str, head: str) -> str: + result = subprocess.run( + [sys.executable, str(self.SCRIPT), base, head, "--repo", str(ROOT)], + capture_output=True, + text=True, + timeout=300, + check=False, + ) + assert result.returncode == 0, result.stderr + return result.stdout.strip() + + @pytest.mark.parametrize("manifest", [r"pyproject\.toml", r"uv\.lock"]) + def test_the_manifests_no_longer_trigger_by_name(self, manifest): + """Naming a manifest may gate the resolution check, never a build. + + Checked as a BLOCK, not a line. The condition that decides whether to + run the check names both manifests too, so a line-level "does this + mention uv.lock" flags the fix itself. + """ + lines = self._detect_step().splitlines() + offenders = [] + for index, line in enumerate(lines): + if "grep -qE" not in line or manifest not in line: + continue + body = [] + for follower in lines[index + 1 :]: + if follower.strip() == "fi": + break + body.append(follower) + if any("BUILD_UI" in b or "BUILD_WORKER" in b for b in body): + offenders.append(line.strip()) + assert not offenders, ( + f"{manifest} still sets a build flag by filename, so the resolution check cannot matter: {offenders}" + ) + + def test_the_resolution_check_decides_instead(self): + step = self._detect_step() + assert "scripts/runtime_deps_changed.py" in step, "nothing asks whether the shipped dependencies changed" + assert "RUNTIME_CHANGED" in step + assert 'if [ "$RUNTIME_CHANGED" = true ]; then' in step, "the answer is computed but never acted on" + + def test_uv_is_installed_before_anything_reads_the_answer(self): + """Without uv the script is fail-safe but useless: everything builds.""" + doc = yaml.safe_load(RELEASE.read_text(encoding="utf-8")) + names = [s.get("name") or s.get("uses") or "" for s in doc["jobs"]["release"]["steps"]] + assert "Install uv" in names, f"the release job never installs uv: {names}" + assert names.index("Install uv") < names.index("Detect what changed"), names + + def _script_module(self): + """The script is not a package; load it from its path.""" + import importlib.util + + spec = importlib.util.spec_from_file_location("runtime_deps_changed", self.SCRIPT) + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + return module + + def test_the_detector_uses_the_uv_the_images_build_with(self): + """A different uv could read the lock differently from the one that + installs it, and then the check judges a resolution nobody ships. + Derived from the Dockerfile, so there is no second copy to drift. + (CodeRabbit, PR #354.) + """ + import yaml + + doc = yaml.safe_load(RELEASE.read_text(encoding="utf-8")) + step = next(s for s in doc["jobs"]["release"]["steps"] if s.get("name") == "Install uv") + assert "astral-sh/uv:" in step["run"], "the release job installs some uv, not the images' uv" + assert 'pip install "uv==$UV_VERSION"' in step["run"], "the derived version is read but not installed" + + def test_the_dockerfile_still_states_a_uv_version_to_derive(self): + """The derivation above degrades to a warning, so this is what notices.""" + found = re.findall(r"astral-sh/uv:(\d+\.\d+\.\d+)", (ROOT / "Dockerfile").read_text(encoding="utf-8")) + assert found, "Dockerfile no longer pins a uv version, so the release job cannot match it" + worker = re.findall(r"astral-sh/uv:(\d+\.\d+\.\d+)", (ROOT / "Dockerfile.worker").read_text(encoding="utf-8")) + assert set(found) == set(worker), f"the two images build with different uv versions: {found} vs {worker}" + + def test_a_hash_only_change_is_a_change(self): + """`uv sync --frozen` consumes the artifacts, not just the versions. + + A lock can gain or change an artifact for a version that already + exists, and every `name==version` pin still reads the same. Exporting + without hashes returned "unchanged" for something the build installs + differently. (CodeRabbit, PR #354.) + """ + import re as _re + import shutil + import tempfile + + module = self._script_module() + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp) + plain, tampered = root / "plain", root / "tampered" + for target in (plain, tampered): + target.mkdir() + for name in module.MANIFESTS: + shutil.copy(ROOT / name, target / name) + + before = module._runtime_requirements(plain) + assert before, "the baseline export produced nothing, so this test proves nothing" + + hashes = _re.findall(r"--hash=sha256:([0-9a-f]{64})", "\n".join(before)) + assert hashes, "the export carries no hashes, so a hash-only change could never be seen" + + lock = (tampered / "uv.lock").read_text(encoding="utf-8") + flipped = ("0" if hashes[0][0] != "0" else "1") + hashes[0][1:] + assert hashes[0] in lock + (tampered / "uv.lock").write_text(lock.replace(hashes[0], flipped), encoding="utf-8") + + after = module._runtime_requirements(tampered) + assert after is not None, "uv refused the tampered lock, so the comparison never happened" + assert before != after, "a changed artifact hash reads as no change, so the release would be skipped" + + def test_no_tag_to_compare_against_still_builds(self): + """The first release, or a checkout without tags, must not skip. + + There is nothing to diff against, so the honest answer is "changed". + Reaching the script with an empty ref would make it compare HEAD with + nothing and say "unchanged", which publishes no image at all. + """ + step = self._detect_step() + block = step[step.index("RUNTIME_CHANGED=false") :] + block = block[: block.index("BUILD_UI")] + assert 'if [ -n "$LAST_TAG" ]; then' in block, ( + "the detector calls the script even with no tag to compare against" + ) + assert "RUNTIME_CHANGED=true" in block, "the no-tag branch does not force a build" + + def test_a_dev_only_bump_does_not_build(self): + """v1.36.4 over v1.36.3: pytest, pytest-asyncio and ruff, nothing shipped.""" + assert self._answer("v1.36.3", "v1.36.4") == "false" + + def test_a_runtime_bump_does_build(self): + """v1.36.3 over v1.36.2: starlette, uvicorn, idna and friends.""" + assert self._answer("v1.36.2", "v1.36.3") == "true" + + def test_a_ref_it_cannot_read_builds(self): + """Not knowing must never read as "nothing to do".""" + assert self._answer("v99.99.99", "HEAD") == "true" + + def test_a_missing_uv_builds(self, tmp_path): + """The one failure that would otherwise silently disable every release.""" + stripped = dict(os.environ, PATH=str(tmp_path)) + result = subprocess.run( + [sys.executable, str(self.SCRIPT), "v1.36.3", "v1.36.4", "--repo", str(ROOT)], + capture_output=True, + text=True, + timeout=120, + check=False, + env=stripped, + ) + assert result.returncode == 0, result.stderr + assert result.stdout.strip() == "true", result.stdout + assert "uv is not on PATH" in result.stderr + + def test_the_tags_it_measures_against_exist(self): + """Guards the two tests above from passing on a repo with no tags.""" + for tag in ("v1.36.2", "v1.36.3", "v1.36.4"): + result = subprocess.run( + ["git", "-C", str(ROOT), "rev-parse", "--verify", f"{tag}^{{commit}}"], + capture_output=True, + text=True, + check=False, + ) + assert result.returncode == 0, f"{tag} is missing, so the behaviour tests measure nothing"