diff --git a/.github/scripts/check_extension_version_bump.py b/.github/scripts/check_extension_version_bump.py new file mode 100644 index 0000000000..3c7915b181 --- /dev/null +++ b/.github/scripts/check_extension_version_bump.py @@ -0,0 +1,179 @@ +#!/usr/bin/env python3 +"""Fail a PR that changes bundled extension content without a version bump. + +Update offers from `specify extension update` are version-driven: an +extension is offered (and installed) only when the semver in +`extensions/catalog.json` exceeds the installed copy's registered +version. A content change shipped without a version bump is therefore +never delivered automatically (#4345) — a bump is what makes a change +actually reach existing installs, and this guard is what makes the bump +non-optional. + +This check enforces two invariants on the extensions listed in +`extensions/catalog.json`: + +1. Any change to a file under `extensions//` must increase the + `version:` in that extension's `extension.yml` (PEP 440 comparison, + the same semantics `extension update` uses). +2. The `version` in `extensions/catalog.json` must equal the manifest's + `extension.version` (the catalog is what update checks compare + against, and the update preflight rejects a manifest whose version + differs from the catalog's). + +Usage: + check_extension_version_bump.py BASE_REF [HEAD_REF] + +BASE_REF is a git ref/SHA for the PR base (must be fetchable with +`git show`). HEAD_REF defaults to the working tree's HEAD. Exits 0 when +all invariants hold, 1 otherwise, printing one line per violation. + +Extensions under `extensions/` that are not in the catalog (the +`selftest` fixture and the `template` scaffold) are exempt: no update +flow is driven by their versions. +""" + +from __future__ import annotations + +import json +import subprocess +import sys +from pathlib import PurePosixPath + +import yaml +from packaging.version import InvalidVersion, Version + +EXTENSIONS_ROOT = "extensions" +CATALOG_PATH = f"{EXTENSIONS_ROOT}/catalog.json" + + +def _changed_paths(base_ref: str, head_ref: str) -> list[str]: + """Paths under extensions/ that differ between *base_ref* and *head_ref*. + + Uses NUL-delimited output (``-z``): without it git C-quotes any path + containing non-ASCII or control characters (``"extensions/x/caf\\303\\251"``, + quotes included), so the leading component would no longer equal + ``extensions`` and that change would silently escape the guard. Paths + are decoded with surrogateescape so an undecodable byte can never crash + the check; only the ASCII ``extensions//`` prefix is interpreted. + """ + raw = subprocess.run( + [ + "git", "diff", "--name-only", "-z", "--no-renames", + base_ref, head_ref, "--", EXTENSIONS_ROOT, + ], + check=True, + capture_output=True, + ).stdout + return [ + chunk.decode("utf-8", errors="surrogateescape") + for chunk in raw.split(b"\0") + if chunk + ] + + +def _show(ref: str, path: str) -> str | None: + """Return the file's content at *ref*, or None when absent there.""" + result = subprocess.run( + ["git", "show", f"{ref}:{path}"], capture_output=True, text=True + ) + return result.stdout if result.returncode == 0 else None + + +def _manifest_version(manifest_text: str, origin: str) -> str: + data = yaml.safe_load(manifest_text) + if not isinstance(data, dict) or not isinstance(data.get("extension"), dict): + raise ValueError(f"{origin}: manifest is not a mapping with an 'extension' block") + version = data["extension"].get("version") + if not isinstance(version, str) or not version.strip(): + raise ValueError(f"{origin}: extension.version is missing or not a string") + return version.strip() + + +def main(argv: list[str]) -> int: + if len(argv) < 2 or len(argv) > 3: + print(__doc__, file=sys.stderr) + return 2 + base_ref = argv[1] + head_ref = argv[2] if len(argv) == 3 else "HEAD" + + catalog_text = _show(head_ref, CATALOG_PATH) + if catalog_text is None: + print(f"::error::{CATALOG_PATH} is missing at {head_ref}") + return 1 + catalog = json.loads(catalog_text) + catalog_entries = catalog.get("extensions", {}) + + errors: list[str] = [] + + # -- Invariant 1: content change requires a version bump --------------- + changed_ids = { + parts[1] + for path in _changed_paths(base_ref, head_ref) + if len(parts := PurePosixPath(path).parts) >= 3 and parts[0] == EXTENSIONS_ROOT + } + + for ext_id in sorted(changed_ids): + if ext_id not in catalog_entries: + continue # not driven by `extension update` (selftest, template) + manifest_path = f"{EXTENSIONS_ROOT}/{ext_id}/extension.yml" + head_manifest = _show(head_ref, manifest_path) + if head_manifest is None: + continue # extension removed in this PR + base_manifest = _show(base_ref, manifest_path) + if base_manifest is None: + continue # new extension; any initial version is fine + try: + base_version = _manifest_version(base_manifest, f"{base_ref}:{manifest_path}") + head_version = _manifest_version(head_manifest, f"{head_ref}:{manifest_path}") + except ValueError as exc: + errors.append(str(exc)) + continue + + # Compare with the same PEP 440 semantics the extension update and + # install code use (packaging.version), so prereleases and other + # accepted forms cannot bypass the guard (e.g. 2.0.0 -> 1.0.0rc1 is + # a downgrade). Unparseable versions fail closed. + try: + base_parsed = Version(base_version) + head_parsed = Version(head_version) + except InvalidVersion as exc: + errors.append( + f"{manifest_path}: could not compare versions " + f"{base_version!r} -> {head_version!r}: {exc}" + ) + continue + if head_parsed <= base_parsed: + errors.append( + f"{manifest_path}: files under {EXTENSIONS_ROOT}/{ext_id}/ changed but " + f"extension.version did not increase ({base_version} -> {head_version}). " + f"Installed copies only receive changes when the version is bumped." + ) + + # -- Invariant 2: catalog.json version matches the manifest ------------ + for ext_id, entry in sorted(catalog_entries.items()): + manifest_path = f"{EXTENSIONS_ROOT}/{ext_id}/extension.yml" + head_manifest = _show(head_ref, manifest_path) + if head_manifest is None: + continue # catalog-only entry (e.g. hosted elsewhere) + try: + manifest_version = _manifest_version(head_manifest, f"{head_ref}:{manifest_path}") + except ValueError as exc: + errors.append(str(exc)) + continue + catalog_version = entry.get("version") + if catalog_version != manifest_version: + errors.append( + f"{CATALOG_PATH}: entry '{ext_id}' has version {catalog_version!r} but " + f"{manifest_path} declares {manifest_version!r}. `extension update` " + f"compares against the catalog, so the two must move together." + ) + + for error in errors: + print(f"::error::{error}") + if not errors: + print("Extension version guard: all invariants hold.") + return 1 if errors else 0 + + +if __name__ == "__main__": + sys.exit(main(sys.argv)) diff --git a/.github/workflows/extension-version-guard.yml b/.github/workflows/extension-version-guard.yml new file mode 100644 index 0000000000..26f9f98096 --- /dev/null +++ b/.github/workflows/extension-version-guard.yml @@ -0,0 +1,46 @@ +name: Extension Version Guard + +permissions: + contents: read + +# Bundled extensions only reach existing installs through a version bump: +# `specify extension update` compares the semver in extensions/catalog.json +# against the installed copy and reports "Up to date" whenever they match. +# Content changes shipped without a bump go silently stale on every +# project that already installed the extension (#4345). This guard turns +# "please remember to bump" into a merge requirement. +# +# Deliberately no `paths:` filter: a required status check that is skipped +# by path filtering stays in "Expected" state and blocks every PR that does +# not touch extensions/**. The check runs on every pull request instead and +# the script reports success when nothing under extensions/ changed. +on: + pull_request: + +jobs: + version-bump: + runs-on: ubuntu-latest + steps: + - name: Checkout + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + fetch-depth: 1 + + - name: Set up Python + uses: actions/setup-python@5fda3b95a4ea91299a34e894583c3862153e4b97 # v7.0.0 + with: + python-version: "3.14" + + - name: Install check dependencies + run: python -m pip install --quiet pyyaml packaging + + # For pull_request events the checkout is the merge of the PR head + # into the base tip, so diffing base.sha against HEAD yields exactly + # the PR's changes (same fetch pattern as lint.yml). + - name: Check bundled extension version bumps + env: + PR_BASE_SHA: ${{ github.event.pull_request.base.sha }} + run: | + set -euo pipefail + git fetch --no-tags --depth=1 origin "+${PR_BASE_SHA}:refs/checks/pr-base" + python .github/scripts/check_extension_version_bump.py refs/checks/pr-base diff --git a/extensions/EXTENSION-DEVELOPMENT-GUIDE.md b/extensions/EXTENSION-DEVELOPMENT-GUIDE.md index ac78029f2a..41ed9f4873 100644 --- a/extensions/EXTENSION-DEVELOPMENT-GUIDE.md +++ b/extensions/EXTENSION-DEVELOPMENT-GUIDE.md @@ -620,6 +620,13 @@ See the [Extension Publishing Guide](EXTENSION-PUBLISHING-GUIDE.md) for detailed - **MAJOR**: Breaking changes - **MINOR**: New features - **PATCH**: Bug fixes +- **Bump on every content change**: update offers from `specify extension + update` are version-driven, so a content change shipped without a + version bump is never delivered automatically to already-installed + copies. For the bundled extensions in this repository the bump is + enforced by CI (`extension-version-guard.yml`): a PR that changes + files under `extensions//` must also bump that extension's + `extension.yml` version and keep `extensions/catalog.json` in sync. ### Security diff --git a/tests/contract/test_bundled_extension_versions.py b/tests/contract/test_bundled_extension_versions.py new file mode 100644 index 0000000000..f34a31fcfa --- /dev/null +++ b/tests/contract/test_bundled_extension_versions.py @@ -0,0 +1,64 @@ +"""Contract tests: bundled extension versions must stay in sync with the catalog. + +``specify extension update`` decides whether an installed extension needs +updating by comparing the semver in ``extensions/catalog.json`` against the +installed copy's registered version, and its preflight rejects a manifest +whose version differs from the catalog's. A catalog entry that drifts from +its ``extension.yml`` therefore either hides updates from every installed +copy or makes every offered update fail validation (#4345). + +The companion "content change requires a version bump" rule needs the git +diff of a PR and lives in CI +(``.github/scripts/check_extension_version_bump.py`` via the +``extension-version-guard.yml`` workflow); this test enforces the half that +is checkable from a plain working tree. +""" + +from __future__ import annotations + +import json +from pathlib import Path + +import pytest +import yaml + +REPO_ROOT = Path(__file__).parents[2] +EXTENSIONS_ROOT = REPO_ROOT / "extensions" + + +def _catalog_entries() -> dict[str, dict]: + catalog = json.loads((EXTENSIONS_ROOT / "catalog.json").read_text(encoding="utf-8")) + return catalog["extensions"] + + +def _manifest_version(ext_id: str) -> str: + manifest_path = EXTENSIONS_ROOT / ext_id / "extension.yml" + data = yaml.safe_load(manifest_path.read_text(encoding="utf-8")) + return data["extension"]["version"] + + +def test_catalog_lists_extensions(): + assert _catalog_entries(), "expected at least one extension in extensions/catalog.json" + + +@pytest.mark.parametrize("ext_id", sorted(_catalog_entries())) +def test_catalog_version_matches_manifest(ext_id: str): + entry = _catalog_entries()[ext_id] + manifest_path = EXTENSIONS_ROOT / ext_id / "extension.yml" + if not manifest_path.is_file(): + pytest.skip(f"'{ext_id}' has no in-repo extension directory") + assert entry.get("version") == _manifest_version(ext_id), ( + f"extensions/catalog.json entry '{ext_id}' and {manifest_path.relative_to(REPO_ROOT)} " + f"declare different versions - `specify extension update` compares against the " + f"catalog, so the two must move together" + ) + + +@pytest.mark.parametrize("ext_id", sorted(_catalog_entries())) +def test_bundled_entries_ship_an_extension_directory(ext_id: str): + entry = _catalog_entries()[ext_id] + if not entry.get("bundled"): + pytest.skip(f"'{ext_id}' is not marked bundled") + assert (EXTENSIONS_ROOT / ext_id / "extension.yml").is_file(), ( + f"catalog marks '{ext_id}' as bundled but extensions/{ext_id}/extension.yml is missing" + ) diff --git a/tests/contract/test_extension_version_guard_script.py b/tests/contract/test_extension_version_guard_script.py new file mode 100644 index 0000000000..c90d8297bc --- /dev/null +++ b/tests/contract/test_extension_version_guard_script.py @@ -0,0 +1,212 @@ +"""Tests for the extension version-bump CI guard script (#4345). + +The guard (`.github/scripts/check_extension_version_bump.py`) is the +primary regression prevention for bundled-extension version staleness, so +its failure behavior must be pinned by tests: each scenario builds a real +throwaway git repository and invokes the script against base/head SHAs, +exactly as the `extension-version-guard.yml` workflow does. Without this, +a change to the script's diff or parsing logic could silently disable the +guard while CI stays green. +""" + +from __future__ import annotations + +import json +import subprocess +import sys +from pathlib import Path + +import pytest + +REPO_ROOT = Path(__file__).parents[2] +SCRIPT = REPO_ROOT / ".github" / "scripts" / "check_extension_version_bump.py" + + +def _git(repo: Path, *args: str) -> str: + result = subprocess.run( + ["git", "-C", str(repo), *args], + check=True, + capture_output=True, + text=True, + ) + return result.stdout.strip() + + +def _write_extension(repo: Path, ext_id: str, version: str, script_line: str) -> None: + ext_dir = repo / "extensions" / ext_id + ext_dir.mkdir(parents=True, exist_ok=True) + (ext_dir / "extension.yml").write_text( + 'schema_version: "1.0"\n' + "\n" + "extension:\n" + f" id: {ext_id}\n" + f' version: "{version}"\n', + encoding="utf-8", + ) + (ext_dir / "script.sh").write_text(f"{script_line}\n", encoding="utf-8") + + +def _write_catalog(repo: Path, versions: dict[str, str]) -> None: + (repo / "extensions").mkdir(exist_ok=True) + payload = { + "schema_version": "1.0", + "extensions": { + ext_id: {"id": ext_id, "version": version, "bundled": True} + for ext_id, version in versions.items() + }, + } + (repo / "extensions" / "catalog.json").write_text( + json.dumps(payload, indent=2), encoding="utf-8" + ) + + +def _commit_all(repo: Path, message: str) -> str: + _git(repo, "add", "-A") + _git(repo, "commit", "-q", "-m", message) + return _git(repo, "rev-parse", "HEAD") + + +@pytest.fixture +def guard_repo(tmp_path: Path) -> tuple[Path, str]: + """A git repo with one cataloged and one uncataloged extension at base.""" + repo = tmp_path / "repo" + repo.mkdir() + _git(repo, "init", "-q") + _git(repo, "config", "user.email", "guard-tests@example.com") + _git(repo, "config", "user.name", "Guard Tests") + _git(repo, "config", "commit.gpgsign", "false") + # git's default; pinned so the non-ASCII regression below exercises the + # C-quoting code path even on machines whose global config disables it. + _git(repo, "config", "core.quotePath", "true") + + _write_extension(repo, "demo", "1.0.0", "echo base") + _write_extension(repo, "scratch", "1.0.0", "echo base") # not in catalog + _write_catalog(repo, {"demo": "1.0.0"}) + base_sha = _commit_all(repo, "base") + return repo, base_sha + + +def _run_guard(repo: Path, base: str, head: str = "HEAD") -> subprocess.CompletedProcess: + return subprocess.run( + [sys.executable, str(SCRIPT), base, head], + cwd=repo, + capture_output=True, + text=True, + ) + + +def test_valid_bump_passes(guard_repo): + repo, base = guard_repo + _write_extension(repo, "demo", "1.1.0", "echo changed") + _write_catalog(repo, {"demo": "1.1.0"}) + _commit_all(repo, "content change with bump") + + result = _run_guard(repo, base) + assert result.returncode == 0, result.stdout + result.stderr + assert "all invariants hold" in result.stdout + + +def test_unbumped_content_change_fails(guard_repo): + repo, base = guard_repo + _write_extension(repo, "demo", "1.0.0", "echo changed") + _commit_all(repo, "content change without bump") + + result = _run_guard(repo, base) + assert result.returncode == 1, result.stdout + result.stderr + assert "did not increase" in result.stdout + assert "extensions/demo/extension.yml" in result.stdout + + +def test_unbumped_non_ascii_filename_fails(guard_repo): + """With core.quotePath (git's default) `git diff --name-only` C-quotes a + path like extensions/demo/café.txt, quotes included, so a line-based + parser no longer sees `extensions` as the first component and the + change escapes the guard. The NUL-delimited diff must still catch it.""" + repo, base = guard_repo + (repo / "extensions" / "demo" / "café.txt").write_text("new\n", encoding="utf-8") + _commit_all(repo, "add non-ascii file without bump") + + result = _run_guard(repo, base) + assert result.returncode == 1, result.stdout + result.stderr + assert "did not increase" in result.stdout + assert "extensions/demo/extension.yml" in result.stdout + + +def test_no_extension_changes_passes(guard_repo): + """The workflow runs on every pull request (a path-filtered required check + would block PRs that skip it), so a PR touching nothing under extensions/ + must pass rather than be reported as a violation.""" + repo, base = guard_repo + (repo / "README.md").write_text("docs only\n", encoding="utf-8") + _commit_all(repo, "unrelated change") + + result = _run_guard(repo, base) + assert result.returncode == 0, result.stdout + result.stderr + assert "all invariants hold" in result.stdout + + +def test_downgrade_fails(guard_repo): + repo, base = guard_repo + _write_extension(repo, "demo", "0.9.0", "echo changed") + _write_catalog(repo, {"demo": "0.9.0"}) + _commit_all(repo, "downgrade") + + result = _run_guard(repo, base) + assert result.returncode == 1, result.stdout + result.stderr + assert "did not increase" in result.stdout + + +def test_prerelease_downgrade_fails(guard_repo): + """PEP 440 semantics: 1.0.0rc1 is lower than 1.0.0, and it must not slip + through as a plain string inequality.""" + repo, base = guard_repo + _write_extension(repo, "demo", "1.0.0rc1", "echo changed") + _write_catalog(repo, {"demo": "1.0.0rc1"}) + _commit_all(repo, "prerelease downgrade") + + result = _run_guard(repo, base) + assert result.returncode == 1, result.stdout + result.stderr + assert "did not increase" in result.stdout + + +def test_manifest_bump_without_catalog_sync_fails(guard_repo): + repo, base = guard_repo + _write_extension(repo, "demo", "1.1.0", "echo changed") + _commit_all(repo, "bump without catalog sync") + + result = _run_guard(repo, base) + assert result.returncode == 1, result.stdout + result.stderr + assert "must move together" in result.stdout + assert "catalog.json" in result.stdout + + +def test_uncataloged_extension_change_is_exempt(guard_repo): + repo, base = guard_repo + _write_extension(repo, "scratch", "1.0.0", "echo changed") + _commit_all(repo, "uncataloged change without bump") + + result = _run_guard(repo, base) + assert result.returncode == 0, result.stdout + result.stderr + assert "all invariants hold" in result.stdout + + +def test_new_cataloged_extension_passes_without_base_version(guard_repo): + repo, base = guard_repo + _write_extension(repo, "fresh", "0.1.0", "echo new") + _write_catalog(repo, {"demo": "1.0.0", "fresh": "0.1.0"}) + _commit_all(repo, "add new extension") + + result = _run_guard(repo, base) + assert result.returncode == 0, result.stdout + result.stderr + assert "all invariants hold" in result.stdout + + +def test_unparseable_version_fails_closed(guard_repo): + repo, base = guard_repo + _write_extension(repo, "demo", "not-a-version", "echo changed") + _write_catalog(repo, {"demo": "not-a-version"}) + _commit_all(repo, "unparseable version") + + result = _run_guard(repo, base) + assert result.returncode == 1, result.stdout + result.stderr + assert "could not compare versions" in result.stdout