From af8566bf055b71dc84b68637de521c448bf7e298 Mon Sep 17 00:00:00 2001 From: Petr Date: Tue, 25 Aug 2026 23:03:42 +0200 Subject: [PATCH] feat(release): mechanise vNEXT resolution, enforce the heading rule, gate release scope Three release-process gaps, all found by running the v0.91.0 release. Each was a rule that existed only as prose plus a hand-run command, and each failed the same way: a release is exactly when parallel branches converge, so any step of the form 'run this grep when releasing' eventually loses a merge race. 1. make vnext-resolve VERSION=X.Y.Z Checklist step 4 resolved 54 placeholders by hand off the checker's own output. The scanner already separates a live gate from prose with perfect precision, so --resolve reuses it and rewrites only outside inline-code spans -- including on a line carrying both a quoted mention and a live gate, which a line-level sed corrupts. Refuses a VERSION disagreeing with pyproject.toml, because packaging parses 'v0.91' and '0.91' happily. 2. vNEXT in a markdown heading is now fatal on EVERY PR Resolving a placeholder in a heading rewrites its anchor slug and breaks inbound links. The rule was a release-time grep; in 0.91.0 it lost a race (#697 ran two minutes before #694 and #696 landed headings of their own) and three shipped. Checking at authoring time makes the race impossible. Numeric headings are deliberately not flagged -- a resolved tag never changes again, so its slug is stable. 3. make release-scope-check Proves the changelog entry covers every PR the tag will CONTAIN, not the scope collected when the release PR was opened. In 0.91.0, #625 merged nine minutes before the release PR and landed inside the tag with no release note; changelog-check cannot see this, since it proves every released version has an entry, never that an entry covers every commit under the tag. Armed in CI exactly like the vNEXT gate (version-raising PRs only), with the checkout deepened only then and a fail-open path so an ordinary PR can never go red on it. 39 new tests. CONTRIBUTING's release checklist grows to 18 steps. --- .github/workflows/ci.yml | 26 ++++ CONTRIBUTING.md | 50 ++++++-- Makefile | 9 +- scripts/check_release_scope.py | 198 +++++++++++++++++++++++++++++ scripts/check_version_gates.py | 168 +++++++++++++++++++++++++ tests/test_check_release_scope.py | 125 +++++++++++++++++++ tests/test_check_version_gates.py | 199 ++++++++++++++++++++++++++++++ 7 files changed, 767 insertions(+), 8 deletions(-) create mode 100644 scripts/check_release_scope.py create mode 100644 tests/test_check_release_scope.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 76a11687..bf4e8329 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -186,6 +186,32 @@ jobs: fi uv run python scripts/check_version_gates.py --release-if-newer-than "$base" + - name: Release-scope check (release PRs only) + # Proves the new changelog entry covers every PR the tag will CONTAIN, + # not just the scope collected when the release PR was opened. Those + # differ whenever a feature PR merges while the release PR is open -- + # a structural window, since a release PR stays open for as long as its + # CI runs. It bit v0.91.0: #625 merged nine minutes before the release + # PR and landed inside the tag with no release note. `changelog-check` + # cannot see it (it proves every released VERSION has an entry, never + # that an entry covers every COMMIT under the tag). + # + # Armed exactly like the vNEXT gate above -- only for a PR that RAISES + # the version. The check needs tags and real history, which the default + # shallow checkout lacks, so the deepening is done ONLY when the version + # differs; the script fails open (warns, exits 0) if git still cannot + # answer, so an ordinary PR can never go red because of this. + if: github.event_name == 'pull_request' + run: | + base=$(git show "origin/$GITHUB_BASE_REF:pyproject.toml" 2>/dev/null \ + | sed -n 's/^version = "\(.*\)"/\1/p' | head -1) + head=$(sed -n 's/^version = "\(.*\)"/\1/p' pyproject.toml | head -1) + if [ -n "$base" ] && [ "$base" != "$head" ]; then + echo "version changed ($base -> $head); deepening checkout for the scope check" + git fetch --unshallow --tags --quiet 2>/dev/null || git fetch --tags --quiet || true + fi + uv run python scripts/check_release_scope.py --only-if-newer-than "$base" + - name: Error-code enum check # Rejects raw error_code="LITERAL" string literals (must use ErrorCode). run: uv run python scripts/check_error_codes.py diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index bbd712df..a9d16eac 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -625,10 +625,18 @@ silent-drift risks summarized in the entry and the release notes must cover each of them, and nothing else. 2. **Edit `pyproject.toml`** -- bump `version = "X.Y.Z"`. Single source of truth; everything else derives from it. This is the release PR's defining change -- if you are doing this in a feature PR, stop and read the section intro above. 3. **Add a changelog entry** to `src/keboola_agent_cli/changelog.py` -- ONE entry for the new version, covering **every PR merged since the last release** (step 1), no exceptions. CI fails (`make changelog-check`) if this is missing. Author it as the file's docstring describes: **one logical change per bullet** (split the release into several list items rather than one mega-paragraph), each starting with a recognised prefix (`BREAKING:`, `New:`, `Fix:`, `Change:`, `Note:`, `Security:`, ...), carrying its `(#PR)` reference, and leading with a self-contained first sentence. `kbagent changelog` shows only that first sentence per version by default (the rest is revealed by `--full`), so a buried headline or a single wall-of-text bullet reads as an unscannable blob. The first sentence is also **capped at 160 characters**, enforced by `tests/test_changelog_render.py::TestLiveChangelogHeadlines::test_newest_release_notes_are_not_truncated` (so `make check` in step 12 catches it) -- past the cap the default view and the release page show it cut mid-clause. Write a short self-contained first sentence and put the detail in the sentences after it; 2 of 0.90.0's 13 bullets needed exactly this rewrite. -4. **Replace every `vNEXT` placeholder** left behind by the feature PRs with the version being released, then verify none survive: +4. **Replace every `vNEXT` placeholder** left behind by the feature PRs with the version being released. Do it mechanically -- never by hand, and never with a repo-wide `sed`: ```bash + make vnext-resolve VERSION=X.Y.Z make vnext-check ``` + `vnext-resolve` reuses the same scanner `vnext-check` does, so it rewrites + exactly the live gates and leaves every backticked mention of the token + alone -- including a line that carries both at once, which a line-level + `sed` corrupts. It refuses any `VERSION` that disagrees with + `pyproject.toml` (bump that first, in step 2): `packaging` happily parses + `v0.91` and `0.91`, so a typo can look valid and then be stamped into every + gate in the tree at once. A leftover `(since vNEXT)` ships agents a gate no installed version can ever satisfy -- strictly worse than no gate, because they then refuse a command the user has. The release PR is the only place it can be fixed. @@ -651,12 +659,24 @@ silent-drift risks summarized in the > numeric gates -- `docs/sdk.md` writes 14 genuine ones as `` `0.66.0+` ``, > where backticks are ordinary typography rather than quotation. - While resolving, keep version tags **out of markdown headings**: a - `### Foo *(since vNEXT)*` heading changes its generated anchor slug at - every release, breaking each inbound `#foo-...` link (this bit 0.90.0 -- + Version tags must stay **out of markdown headings**: a + `### Foo *(since vNEXT)*` heading changes its generated anchor slug when the + placeholder resolves, breaking each inbound `#foo-...` link (this bit 0.90.0 -- the What's-new section's link broke the moment the placeholder resolved). Put the tag on the section's first body line instead; the gate checks scan whole files, not just headings, so nothing is lost. + + **This is CI-enforced on EVERY PR**, not just at release time -- a `vNEXT` + inside an ATX heading in a `.md` file fails `make version-gate-check` + (already part of `make check`). It is deliberately armed everywhere rather + than only under `--release`, because the rule used to be a hand-run + `grep -rn '^##.*vNEXT' plugins/` at release time and that grep **lost a + merge race in 0.91.0**: PR #697 ran it two minutes before #694 and #696 + landed headings of their own, so all three shipped and had to be cleaned up + after the tag. Any rule of the form "run this grep when releasing" loses + that race eventually, because a release is exactly when parallel branches + converge. Already-numeric headings are *not* flagged -- a resolved tag never + changes again, so its slug is stable. 5. **Run `make version-sync`** -- propagates the new version to `plugins/kbagent/.claude-plugin/plugin.json`. The pre-commit hook does this automatically on `git commit`, but running it explicitly lets you eyeball the diff. 6. **Run `make skill-gen`** -- regenerates the decision table in `SKILL.md`. Idempotent if no commands changed since the previous release. 7. **Add a curated What's-new entry** to `web/frontend/src/whatsnew.ts` when the release ships anything UI-visible -- a `WhatsNewRelease` element keyed by the **exact** new version, newest first. This is the reel the web UI shows once per version; it is deliberately *not* derived from `changelog.py` (see `docs/web-server.md` > "What's-new popup"). Skipping it does not error anywhere: `whatsNewFor` falls back to the previous release's reel, which returning users have already dismissed -- so the release's UI work ships **dark**. A release with no UI-visible changes correctly adds nothing. Only the release PR can write this entry (a feature PR cannot know the version), which is why it lives in this checklist and not the per-command one. @@ -670,17 +690,33 @@ silent-drift risks summarized in the 12. **Run `make check`** -- lint + format + skill freshness + version sync + changelog completeness + error-code enum + full test suite. 13. **Run `make test-e2e`** if any command changed since the last release -- requires `E2E_API_TOKEN` and `E2E_URL`. 14. **Open the release PR** -- link the merged PRs it covers (step 1) and list every plugin file you touched in the description so reviewers can spot what was missed. Plugin files do not auto-show up in CI failures the way Python files do; reviewers are the second line of defence. -15. **Merge via `gh pr merge`, then tag -- the tag push IS the release.** Never push directly to `main` (protected). The only manual action after the merge is: +15. **Re-verify the scope against the commit you are about to tag:** + ```bash + make release-scope-check # in the release PR, before merging + make release-scope-check SCOPE_ARGS="--head origin/main --ignore-pr " + ``` + Step 1 collected the scope when the release PR was *opened*; this proves + the changelog entry covers every PR the **tag will actually contain**. The + two differ whenever a feature PR merges while the release PR is open -- + which is a structural window, not bad luck, since a release PR stays open + for as long as its CI runs. It shipped in v0.91.0: #625 merged nine minutes + before the release PR did, landing inside the tag's tree with no release + note, and was caught only because the tag happened to be deferred. + `make changelog-check` cannot see this: it proves every *released version* + has an entry, never that an entry covers every *commit* under the tag. + Run before merging and nothing needs ignoring -- the release PR's own + number is not in the log until its merge commit exists. +16. **Merge via `gh pr merge`, then tag -- the tag push IS the release.** Never push directly to `main` (protected). The only manual action after the merge is: ```bash git fetch origin && git tag v && git push origin v ``` The tag must point at the release PR's merge commit on `main` -- the pipeline's `gate` job fails the whole release if the tag's `pyproject.toml` disagrees with the tag name. Pushing it triggers `.github/workflows/release-kbagent.yml`, which does **everything else**: re-runs the gates, renders the release notes from `changelog.py` (`scripts/gen_release_notes.py` -- never write them by hand), publishes to PyPI, freezes the native binaries for all platforms, packages deb/rpm, creates the GitHub Release with every asset attached and fills its body, and updates Homebrew/Chocolatey/WinGet. Do **not** pre-create the GitHub Release by hand: the pipeline keeps a hand-written body untouched, which silently discards the changelog-rendered notes. -16. **Verify the publish** -- the pipeline guards against half-releases, but both guards exist because each failure shipped once (v0.66.1 went out with an empty body, v0.64.0 without a wheel), so look anyway: +17. **Verify the publish** -- the pipeline guards against half-releases, but both guards exist because each failure shipped once (v0.66.1 went out with an empty body, v0.64.0 without a wheel), so look anyway: ```bash gh run watch $(gh run list --workflow release-kbagent.yml --limit 1 --json databaseId --jq '.[0].databaseId') ``` then confirm `gh release view v` shows a non-empty body rendered from the changelog and both wheels (`keboola_cli-*` + legacy `keboola_agent_cli-*`) among the assets. A `skipped` winget job is normal; any red job is a real signal. -17. **After the tag: merge the ai-kit publish PR.** The `ai-kit-marketplace` job opens +18. **After the tag: merge the ai-kit publish PR.** The `ai-kit-marketplace` job opens `chore(kbagent): publish vX.Y.Z` against `keboola/ai-kit`, bumping the `kbagent` entry in the `keboola-claude-kit` marketplace to this tag. Until that PR merges, `/plugin install kbagent@keboola-claude-kit` still serves the PREVIOUS version -- diff --git a/Makefile b/Makefile index 53df0c70..6802d616 100644 --- a/Makefile +++ b/Makefile @@ -1,6 +1,6 @@ .DEFAULT_GOAL := help -.PHONY: help install install-server sync test test-unit test-integration test-e2e test-e2e-local test-e2e-invite test-e2e-feature test-e2e-stream test-e2e-auth test-file test-cov lint lint-fix format format-check typecheck typecheck-warn skill-check skill-gen version-sync version-check version-gate-check changelog changelog-check check-error-codes check-sentinel-guards loc-check loc-report loc-baseline command-sync-check gen-command-reference endpoints-gen endpoints-check check clean hooks web-install web-dev-backend web-dev-frontend web-build web-clean +.PHONY: help install install-server sync test test-unit test-integration test-e2e test-e2e-local test-e2e-invite test-e2e-feature test-e2e-stream test-e2e-auth test-file test-cov lint lint-fix format format-check typecheck typecheck-warn skill-check skill-gen version-sync version-check version-gate-check vnext-check vnext-resolve release-scope-check changelog changelog-check check-error-codes check-sentinel-guards loc-check loc-report loc-baseline command-sync-check gen-command-reference endpoints-gen endpoints-check check clean hooks web-install web-dev-backend web-dev-frontend web-build web-clean help: ## Show this help message @grep -E '^[a-zA-Z0-9_-]+:.*?## .*$$' $(MAKEFILE_LIST) | awk 'BEGIN {FS = ":.*?## "}; {printf " \033[36m%-18s\033[0m %s\n", $$1, $$2}' @@ -124,6 +124,13 @@ version-gate-check: ## Reject a (since vX.Y.Z) / X.Y.Z+ marker naming an unrelea vnext-check: ## Reject an unresolved version-gate placeholder -- run in the RELEASE PR uv run python scripts/check_version_gates.py --release +vnext-resolve: ## Rewrite every live vNEXT gate to pyproject's version (RELEASE PR step 4) + @test -n "$(VERSION)" || { echo "usage: make vnext-resolve VERSION=X.Y.Z"; exit 2; } + uv run python scripts/check_version_gates.py --resolve $(VERSION) + +release-scope-check: ## Prove the changelog entry covers every PR the tag will contain + uv run python scripts/check_release_scope.py $(SCOPE_ARGS) + check-sentinel-guards: ## Reject an unguarded kbc-session:// sentinel path (silent-drift gate) uv run python scripts/check_sentinel_guards.py diff --git a/scripts/check_release_scope.py b/scripts/check_release_scope.py new file mode 100644 index 00000000..c0dd9e58 --- /dev/null +++ b/scripts/check_release_scope.py @@ -0,0 +1,198 @@ +#!/usr/bin/env python3 +"""Prove the new changelog entry covers every PR the release tag will contain. + +``make changelog-check`` answers a different question: does every *released +version* have a changelog entry? It never asks whether that entry covers every +*commit* under the tag. The gap is not hypothetical -- it shipped in v0.91.0. + +PR #625 merged to ``main`` at 14:27, after the release PR had branched off and +before the release PR itself merged at 14:36. The changelog entry had been +authored against the earlier scope, so #625 -- a new plugin slash command plus +a rewritten onboarding flow -- sat inside the tag's tree with no release note +of any kind. It was caught only because the tag happened to be deferred. + +The window is structural: a release PR is open for as long as its CI takes, +and that is exactly when parallel feature PRs land. So the scope collected when +the release PR is *opened* is not the scope the tag will *contain*, and the +only trustworthy moment to compare them is immediately before tagging. + +Usage:: + + python scripts/check_release_scope.py # v..HEAD + python scripts/check_release_scope.py --base v0.90.1 + python scripts/check_release_scope.py --head origin/main + python scripts/check_release_scope.py --ignore-pr 699 # repeatable + +Run it in the release PR *before* merging and no PR needs ignoring: the release +PR's own number is not in the log until its merge commit exists. Run it after +merging (against ``origin/main``) and the release PR itself shows up as a miss +-- that is what ``--ignore-pr`` is for. +""" + +from __future__ import annotations + +import re +import subprocess +import sys +from pathlib import Path + +from packaging.version import InvalidVersion, Version + +REPO_ROOT = Path(__file__).resolve().parents[1] + +# GitHub's squash-merge subject ends in ``(#N)``; the "Merge pull request #N" +# subject is the merge-commit form. A ``(#N)`` anywhere else in a subject is a +# cross-reference to another PR ("follow-up to (#686)"), not this commit's own, +# so only the trailing form counts. +_TRAILING_PR_RE = re.compile(r"\(#(\d+)\)\s*$") +_MERGE_COMMIT_RE = re.compile(r"^\S+\s+Merge pull request #(\d+)\b") + +# Any GitHub number cited in a changelog bullet -- a PR decoration like +# ``Fix (#686, #694):`` or an issue named in prose. Both are evidence the +# release notes account for the work. +_ANY_REF_RE = re.compile(r"#(\d+)") + + +def merged_pr_numbers(log_text: str) -> list[str]: + """Return the PR numbers in ``git log --oneline --first-parent`` output. + + Order is preserved and duplicates are dropped, so the report reads in the + same order as the log the caller can eyeball. + """ + found: dict[str, None] = {} + for line in log_text.splitlines(): + match = _TRAILING_PR_RE.search(line) or _MERGE_COMMIT_RE.match(line) + if match: + found.setdefault(match.group(1), None) + return list(found) + + +def referenced_pr_numbers(notes: list[str]) -> set[str]: + """Return every GitHub number cited anywhere in a release's changelog bullets.""" + return {ref for note in notes for ref in _ANY_REF_RE.findall(note)} + + +def missing_references(log_text: str, notes: list[str], ignore: frozenset[str]) -> list[str]: + """Return merged PR numbers that the changelog entry never mentions.""" + referenced = referenced_pr_numbers(notes) + return [pr for pr in merged_pr_numbers(log_text) if pr not in referenced and pr not in ignore] + + +def should_arm(base: str, head: str) -> bool: + """Whether this check applies: only a PR that RAISES the version is a release PR. + + Fails open on anything unreadable or malformed. A shallow CI checkout may + not be able to read the base branch's ``pyproject.toml`` at all, and an + ordinary feature PR must never be blocked by that -- the cost of a missed + arming is one manual ``make release-scope-check``, the cost of a false + arming is every PR in the repo going red. + """ + try: + return Version(head) > Version(base) + except (InvalidVersion, TypeError): + return False + + +class GitUnavailable(RuntimeError): + """Git could not answer -- typically a shallow CI checkout with no tags.""" + + +def _git(*args: str) -> str: + try: + return subprocess.run( + ["git", *args], cwd=REPO_ROOT, capture_output=True, text=True, check=True + ).stdout + except (subprocess.CalledProcessError, OSError) as exc: + raise GitUnavailable(f"git {' '.join(args)} failed") from exc + + +def _last_release_tag() -> str: + """The most recent tag reachable from HEAD, i.e. the release being built on.""" + return _git("describe", "--tags", "--abbrev=0", "--match", "v*").strip() + + +def _arg(name: str, default: str | None = None) -> str | None: + if name not in sys.argv: + return default + index = sys.argv.index(name) + 1 + if index >= len(sys.argv): + raise SystemExit(f"ERROR: {name} needs an argument") + return sys.argv[index] + + +def _ignored() -> frozenset[str]: + ignored: set[str] = set() + for index, token in enumerate(sys.argv): + if token == "--ignore-pr" and index + 1 < len(sys.argv): + ignored.add(sys.argv[index + 1].lstrip("#")) + return frozenset(ignored) + + +def main() -> int: + sys.path.insert(0, str(REPO_ROOT / "src")) + from keboola_agent_cli.changelog import CHANGELOG + + text = (REPO_ROOT / "pyproject.toml").read_text(encoding="utf-8") + match = re.search(r'^version\s*=\s*"([^"]+)"', text, re.MULTILINE) + if not match: + print("ERROR: could not read version from pyproject.toml") + return 1 + version = match.group(1) + + notes = CHANGELOG.get(version) + if notes is None: + print(f"ERROR: pyproject.toml declares {version}, which has no changelog entry.\n") + print("Add the entry before checking its scope (release checklist step 3).") + return 1 + + arm_base = _arg("--only-if-newer-than") + if arm_base is not None and not should_arm(arm_base.strip(), version): + print( + f"Not a release PR (base {arm_base.strip() or ''} -> {version}); " + "release-scope check not armed." + ) + return 0 + + ignore = _ignored() + try: + base = _arg("--base") or _last_release_tag() + head = _arg("--head") or "HEAD" + log_text = _git("log", f"{base}..{head}", "--oneline", "--first-parent") + except GitUnavailable as exc: + # Loud, but never blocking: a shallow checkout without tags cannot + # answer this, and that must not turn into a red build on a PR whose + # content is fine. On a release PR the checklist runs it locally. + print(f"WARNING: release-scope check skipped -- {exc}.") + print(" A shallow checkout has no tags/history. Run 'make release-scope-check' locally.") + return 0 + absent = missing_references(log_text, notes, ignore) + + if absent: + print( + f"ERROR: {len(absent)} PR(s) merged in {base}..{head} are not in the {version} entry.\n" + ) + print( + "The tag will contain this work, but the release notes are rendered\n" + "from the changelog entry -- so it would ship with no note at all.\n" + "Add a bullet for each, or pass --ignore-pr N for the release PR itself.\n" + ) + for pr in absent: + subject = next( + (line for line in log_text.splitlines() if f"#{pr})" in line or f"#{pr} " in line), + "", + ) + print(f" #{pr} {subject.split(' ', 1)[-1] if subject else ''}") + return 1 + + covered = len(merged_pr_numbers(log_text)) + print( + f"Release scope OK: all {covered} PR(s) merged in {base}..{head} " + f"are referenced by the {version} changelog entry." + ) + if ignore: + print(f" (ignored: {', '.join('#' + pr for pr in sorted(ignore))})") + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/scripts/check_version_gates.py b/scripts/check_version_gates.py index 49d5c739..7d281284 100644 --- a/scripts/check_version_gates.py +++ b/scripts/check_version_gates.py @@ -155,6 +155,48 @@ def find_vnext_residue(paths: list[Path]) -> list[VnextResidue]: return residue +# An ATX markdown heading: 1-6 hashes followed by a space (CommonMark requires +# the space, so ``#tag`` is not a heading). Only ``.md`` files are considered -- +# ``src/**/*.py`` is scanned for gates too, but a ``#`` there opens a comment, +# which has no anchor slug to break. +HEADING_RE = re.compile(r"^ {0,3}#{1,6} ") + + +def find_heading_placeholders(paths: list[Path]) -> list[VnextResidue]: + """Return every markdown heading carrying a live ``vNEXT`` placeholder. + + Resolving the placeholder rewrites the heading, which rewrites its + generated anchor slug, which breaks every inbound ``#...`` link. Unlike + :func:`find_vnext_residue`, this is fatal on EVERY PR rather than only in + release mode: the rule used to be a hand-run grep at release time, and in + 0.91.0 that grep lost a merge race (PR #697 ran it two minutes before #694 + and #696 landed their own headings). Checking at authoring time is what + makes the race impossible. + + Numeric versions in headings are deliberately NOT flagged: an already + resolved tag never changes again, so its slug is stable, and flagging the + dozen historical ones would be noise with no inbound link at risk. + """ + flagged: list[VnextResidue] = [] + for path in paths: + if path.suffix != ".md": + continue + try: + rel = path.relative_to(REPO_ROOT).as_posix() + except ValueError: + rel = path.as_posix() + for lineno, line in enumerate( + path.read_text(encoding="utf-8", errors="replace").splitlines(), start=1 + ): + if VNEXT_TOKEN not in line or not HEADING_RE.match(line): + continue + # Same quotation rule as the residue scan: a heading that merely + # names the token in backticks is prose about the placeholder. + if VNEXT_TOKEN in INLINE_CODE_RE.sub("", line): + flagged.append(VnextResidue(path=rel, line=lineno, text=line.strip())) + return flagged + + def collect_gates(paths: list[Path]) -> dict[str, list[tuple[str, int]]]: """Map each gated version to the ``(relative path, line number)`` naming it. @@ -178,6 +220,74 @@ def collect_gates(paths: list[Path]) -> dict[str, list[tuple[str, int]]]: return dict(gates) +# Matches the bare placeholder token. ``vNEXT+`` needs no special case: the +# token is replaced in place, so ``vNEXT+`` becomes ``0.91.0+`` on its own. +VNEXT_SUB_RE = re.compile(re.escape(VNEXT_TOKEN)) + + +def _replace_outside_code(line: str, version: str) -> tuple[str, int]: + """Substitute the placeholder only in the parts of *line* outside code spans. + + Rewriting whole lines is what makes a blanket ``sed`` unsafe: a line may + carry a quoted mention AND a live gate at once (CLAUDE.md's description of + the placeholder is exactly that), and only the live one may change. + """ + pieces: list[str] = [] + replaced = 0 + pos = 0 + for span in INLINE_CODE_RE.finditer(line): + chunk, count = VNEXT_SUB_RE.subn(version, line[pos : span.start()]) + pieces.append(chunk) + replaced += count + pieces.append(span.group(0)) # the code span itself is preserved verbatim + pos = span.end() + chunk, count = VNEXT_SUB_RE.subn(version, line[pos:]) + pieces.append(chunk) + replaced += count + return "".join(pieces), replaced + + +def resolve_vnext(paths: list[Path], version: str) -> list[VnextResidue]: + """Rewrite every live ``vNEXT`` gate in *paths* to *version*, in place. + + Returns one entry per rewritten LINE, carrying the text as it now reads. + Files with nothing to change are not written at all, so a release PR's + diff shows only the files that actually carry a gate. + + This is the mechanical form of release checklist step 4. The scanner + already tells a live gate from prose with perfect precision; having a + human apply that knowledge by hand across ~54 lines only adds error. + """ + try: + Version(version) + except Exception as exc: # packaging raises InvalidVersion + raise ValueError(f"{version!r} is not a valid PEP 440 version") from exc + + changed: list[VnextResidue] = [] + for path in paths: + try: + rel = path.relative_to(REPO_ROOT).as_posix() + except ValueError: + rel = path.as_posix() + original = path.read_text(encoding="utf-8") + if VNEXT_TOKEN not in original: + continue + out_lines: list[str] = [] + file_changed = False + for lineno, line in enumerate(original.splitlines(keepends=True), start=1): + if VNEXT_TOKEN not in line: + out_lines.append(line) + continue + new_line, count = _replace_outside_code(line, version) + out_lines.append(new_line) + if count: + file_changed = True + changed.append(VnextResidue(path=rel, line=lineno, text=new_line.strip())) + if file_changed: + path.write_text("".join(out_lines), encoding="utf-8") + return changed + + def resolve_paths() -> list[Path]: """Expand SCANNED_GLOBS into an ordered, de-duplicated file list.""" seen: dict[Path, None] = {} @@ -233,14 +343,72 @@ def main() -> int: paths = resolve_paths() gates = collect_gates(paths) residue = find_vnext_residue(paths) + heading_residue = find_heading_placeholders(paths) + + if "--resolve" in sys.argv: + index = sys.argv.index("--resolve") + 1 + if index >= len(sys.argv): + print("ERROR: --resolve needs a version argument (e.g. --resolve 0.91.0)") + return 1 + requested = sys.argv[index].strip() + shipped = _pyproject_version() + # Cross-check against pyproject rather than trusting the format alone: + # `packaging` accepts `v0.91` and `0.91`, so a typo can parse cleanly + # and then be stamped into every gate in the tree at once. + if requested.lstrip("v") != shipped: + print( + f"ERROR: --resolve {requested} disagrees with pyproject.toml ({shipped}).\n\n" + "Resolve gates to the version this tree actually ships. Bump\n" + "pyproject.toml first, then re-run.\n" + ) + return 1 + try: + applied = resolve_vnext(paths, shipped) + except ValueError as exc: + print(f"ERROR: {exc}") + return 1 + if not applied: + print(f"No unresolved '{VNEXT_TOKEN}' gates to rewrite.") + return 0 + print(f"Rewrote {len(applied)} '{VNEXT_TOKEN}' gate(s) to {shipped}:\n") + for gate in applied: + print(f" {gate.path}:{gate.line}") + still = find_heading_placeholders(resolve_paths()) + if still: # pragma: no cover - defensive; headings are fatal earlier + print(f"\nWARNING: {len(still)} placeholder(s) remain in headings.") + return 0 if "--list" in sys.argv: for version in sorted(gates, key=lambda v: [int(p) for p in v.split(".")]): mark = " " if version in CHANGELOG else " <-- UNKNOWN" print(f"{version:>10} {len(gates[version]):>3} marker(s){mark}") print(f"{VNEXT_TOKEN:>10} {len(residue):>3} marker(s) <-- unresolved placeholder") + print(f"{'in headings':>10} {len(heading_residue):>3} marker(s) <-- always fatal") return 0 + # Fatal in EVERY mode, unlike the plain residue below: a placeholder in a + # heading is never correct at any point in the release cycle, and deferring + # the complaint to the release PR is exactly how 0.91.0 shipped three of + # them (the hand-run grep in #697 raced #694 and #696). + if heading_residue: + print( + f"ERROR: {len(heading_residue)} '{VNEXT_TOKEN}' placeholder(s) inside a " + "markdown heading.\n" + ) + print( + "Resolving the placeholder rewrites the heading, which rewrites its\n" + "generated anchor slug and breaks every inbound '#...' link to the\n" + "section. Move the tag onto the section's first body line instead:\n" + "\n" + " ## Ignored components\n" + "\n" + f" *(since {VNEXT_TOKEN}, #689)*\n" + ) + for gate in heading_residue: + print(f" {gate.path}:{gate.line}") + print(f" {gate.text[:100]}") + return 1 + unknown = {v: locs for v, locs in gates.items() if v not in CHANGELOG} if unknown: print("ERROR: version gate names a version with no changelog entry.\n") diff --git a/tests/test_check_release_scope.py b/tests/test_check_release_scope.py new file mode 100644 index 00000000..071bd8b0 --- /dev/null +++ b/tests/test_check_release_scope.py @@ -0,0 +1,125 @@ +"""Unit tests for the release-scope audit (``scripts/check_release_scope.py``). + +The check exists because of a real miss in v0.91.0. PR #625 merged to ``main`` +after the release PR had branched but before it merged, so it ended up INSIDE +the tag's tree and OUTSIDE the changelog entry the release notes are rendered +from. Nothing caught it: ``make changelog-check`` proves that every released +version has an entry, never that an entry covers every commit under the tag. +Tagging would have shipped a new plugin command with no release note. +""" + +from __future__ import annotations + +import importlib.util +import sys +from pathlib import Path + +_SCRIPT_PATH = Path(__file__).resolve().parents[1] / "scripts" / "check_release_scope.py" +_spec = importlib.util.spec_from_file_location("check_release_scope", _SCRIPT_PATH) +assert _spec is not None and _spec.loader is not None +check_release_scope = importlib.util.module_from_spec(_spec) +sys.modules["check_release_scope"] = check_release_scope +_spec.loader.exec_module(check_release_scope) + +merged_prs = check_release_scope.merged_pr_numbers +referenced_prs = check_release_scope.referenced_pr_numbers +missing = check_release_scope.missing_references + + +class TestMergedPrNumbers: + """PR numbers come from the squash-merge subject GitHub writes.""" + + def test_extracts_trailing_pr_reference(self) -> None: + log = "b5d4be39 docs(plugin): polish workspace-load guidance (#698)\n" + assert merged_prs(log) == ["698"] + + def test_ignores_a_commit_without_a_pr_reference(self) -> None: + """Local commits on the release branch carry no ``(#N)`` and are not PRs.""" + log = "4523f6d3 chore(release): 0.91.0\n1e584ac0 fix: something (#42)\n" + assert merged_prs(log) == ["42"] + + def test_only_a_trailing_reference_counts(self) -> None: + """A ``(#N)`` mid-subject is a cross-reference, not this commit's PR.""" + log = "abc1234 fix(sync): follow-up to (#686) behaviour change\n" + assert merged_prs(log) == [] + + def test_handles_a_merge_commit_subject(self) -> None: + log = "fc7ae7db Merge pull request #627 from keboola/feat/publish\n" + assert merged_prs(log) == ["627"] + + def test_preserves_order_and_deduplicates(self) -> None: + log = "a1 x (#5)\nb2 y (#7)\nc3 z (#5)\n" + assert merged_prs(log) == ["5", "7"] + + +class TestReferencedPrNumbers: + """Any ``#N`` anywhere in the release's changelog bullets counts as covered.""" + + def test_reads_the_prefix_decoration(self) -> None: + notes = ["New (#692): workspace load now clones."] + assert referenced_prs(notes) == {"692"} + + def test_reads_a_multi_pr_decoration(self) -> None: + notes = ["Fix (#686, #694, #696): push stamps API-derived baselines."] + assert referenced_prs(notes) == {"686", "694", "696"} + + def test_reads_a_reference_from_mid_sentence(self) -> None: + """Issue numbers cited in prose count too -- both are GitHub numbers.""" + notes = ["New: the write path fixes what the #600 audit finds."] + assert referenced_prs(notes) == {"600"} + + +class TestMissingReferences: + """The actual gate: every merged PR must appear in the new entry.""" + + def test_fully_covered_release_reports_nothing(self) -> None: + log = "a1 feat: x (#692)\nb2 fix: y (#694)\n" + notes = ["New (#692): x.", "Fix (#694): y."] + assert missing(log, notes, ignore=frozenset()) == [] + + def test_reports_a_merged_pr_absent_from_the_notes(self) -> None: + """This is the v0.91.0 / #625 miss, reproduced.""" + log = "a1 feat: setup (#625)\nb2 fix: y (#694)\n" + notes = ["Fix (#694): y."] + assert missing(log, notes, ignore=frozenset()) == ["625"] + + def test_ignored_pr_is_skipped(self) -> None: + """The release PR itself is in the log once merged, never in its own notes.""" + log = "a1 chore(release): 0.91.0 (#699)\nb2 fix: y (#694)\n" + notes = ["Fix (#694): y."] + assert missing(log, notes, ignore=frozenset({"699"})) == [] + + def test_reports_every_miss_not_just_the_first(self) -> None: + log = "a1 x (#1)\nb2 y (#2)\nc3 z (#3)\n" + assert missing(log, notes := ["Fix (#2): y."], ignore=frozenset()) == ["1", "3"] + assert notes # guard against the walrus being optimised away by a rewrite + + +class TestArmingAndFailOpen: + """CI must arm this only on a release PR, and never block an ordinary one. + + The check needs tags and real history (``git log v..HEAD``), which a + default shallow CI checkout does not have. Deepening every PR run to buy a + check that only matters on release PRs is the wrong trade, so the script + decides whether it applies and degrades to a warning when git cannot answer. + """ + + def test_not_armed_when_the_version_is_unchanged(self) -> None: + assert check_release_scope.should_arm(base="0.91.0", head="0.91.0") is False + + def test_not_armed_when_the_branch_trails_a_released_main(self) -> None: + """A stale feature branch behind main must not look like a release PR.""" + assert check_release_scope.should_arm(base="0.91.0", head="0.90.1") is False + + def test_armed_when_the_pr_raises_the_version(self) -> None: + assert check_release_scope.should_arm(base="0.90.1", head="0.91.0") is True + + def test_armed_for_a_pre_release_bump(self) -> None: + assert check_release_scope.should_arm(base="0.90.1", head="0.91.0b1") is True + + def test_unreadable_base_does_not_arm(self) -> None: + """Fail open: an unreadable base must never block an ordinary PR.""" + assert check_release_scope.should_arm(base="", head="0.91.0") is False + + def test_malformed_version_does_not_arm(self) -> None: + assert check_release_scope.should_arm(base="not-a-version", head="0.91.0") is False diff --git a/tests/test_check_version_gates.py b/tests/test_check_version_gates.py index 5e7c3c79..c829baf7 100644 --- a/tests/test_check_version_gates.py +++ b/tests/test_check_version_gates.py @@ -22,6 +22,8 @@ collect = check_version_gates.collect_gates residue = check_version_gates.find_vnext_residue +headings = check_version_gates.find_heading_placeholders +resolve_vnext = check_version_gates.resolve_vnext def _write(tmp_path: Path, name: str, body: str) -> Path: @@ -362,3 +364,200 @@ def test_double_backticks_do_not_hide_numeric_gates(self, tmp_path: Path) -> Non """The GATE_RE asymmetry survives: code spans are never stripped for versions.""" f = _write(tmp_path, "mod.py", '"""Device-enrollment primitives (``0.66.0+``)."""\n') assert list(collect([f])) == ["0.66.0"] + + +class TestHeadingPlaceholders: + """A ``vNEXT`` inside a markdown heading is fatal on EVERY PR, not just a release. + + Resolving the placeholder rewrites the heading text, which rewrites the + generated anchor slug, which breaks every inbound ``#...`` link. The rule + predates this check as prose in CONTRIBUTING.md plus a hand-run + ``grep -rn '^##.*vNEXT' plugins/`` at release time -- and that grep lost a + merge race in 0.91.0: PR #697 ran it two minutes before #694 and #696 + landed their own headings, so all three shipped and had to be cleaned up + after the fact. Checking at authoring time is what makes the race + impossible. + """ + + def test_atx_heading_with_placeholder_is_flagged(self, tmp_path: Path) -> None: + f = _write(tmp_path, "g.md", "intro\n\n## Ignored components (since vNEXT, #689)\n") + found = headings([f]) + assert len(found) == 1 + assert found[0].line == 3 + + def test_emphasised_tag_in_heading_is_flagged(self, tmp_path: Path) -> None: + """``### Foo *(since vNEXT)*`` is the exact shape #697 had to clean up.""" + f = _write(tmp_path, "g.md", "### What's-new popup *(since vNEXT)*\n") + assert len(headings([f])) == 1 + + def test_placeholder_on_a_body_line_is_not_a_heading(self, tmp_path: Path) -> None: + """The prescribed fix -- tag on the first body line -- must stay legal.""" + f = _write(tmp_path, "g.md", "## Ignored components\n\n*(since vNEXT, #689)*\n") + assert headings([f]) == [] + + def test_python_comment_is_not_a_heading(self, tmp_path: Path) -> None: + """``src/**/*.py`` is scanned for gates, but ``#`` there is a comment. + + A Python comment has no anchor slug, so flagging it would be a pure + false positive -- and CLAUDE.md's command block is full of them. + """ + f = _write(tmp_path, "mod.py", "# workspace load (since vNEXT): auto-decides\n") + assert headings([f]) == [] + + def test_heading_quoting_the_token_is_prose(self, tmp_path: Path) -> None: + """Same inline-code rule as the residue scan: backticks mean quotation.""" + f = _write(tmp_path, "g.md", "## How the `vNEXT` placeholder works\n") + assert headings([f]) == [] + + def test_hash_without_a_space_is_not_a_heading(self, tmp_path: Path) -> None: + """``#tag`` is not ATX -- CommonMark requires a space after the hashes.""" + f = _write(tmp_path, "g.md", "#vNEXT (since vNEXT)\n") + assert headings([f]) == [] + + def test_live_repository_has_no_placeholder_headings(self) -> None: + """The real tree must stay clean -- this is the check's whole point.""" + assert headings(check_version_gates.resolve_paths()) == [] + + +class TestHeadingCheckIsFatalOutsideRelease: + """The heading rule must fail a FEATURE PR -- that is what closes the race. + + ``find_vnext_residue`` is deliberately advisory outside ``--release``, + because a feature PR is supposed to carry placeholders. A placeholder in a + *heading* is different: it is never correct, at any point in the cycle, so + it has to fail the PR that writes it. + """ + + def _run(self, monkeypatch, tmp_path: Path, body: str, argv: list[str]) -> int: + f = _write(tmp_path, "g.md", body) + monkeypatch.setattr(check_version_gates, "resolve_paths", lambda: [f]) + monkeypatch.setattr(sys, "argv", ["check_version_gates.py", *argv]) + return check_version_gates.main() + + def test_heading_placeholder_fails_a_plain_run(self, monkeypatch, tmp_path: Path) -> None: + rc = self._run(monkeypatch, tmp_path, "## Ignored components (since vNEXT)\n", []) + assert rc == 1 + + def test_body_line_placeholder_still_passes_a_plain_run( + self, monkeypatch, tmp_path: Path + ) -> None: + """The advisory-residue behaviour a feature PR relies on is untouched.""" + rc = self._run(monkeypatch, tmp_path, "## Ignored components\n\n*(since vNEXT)*\n", []) + assert rc == 0 + + +class TestResolveVnext: + """``make vnext-resolve VERSION=X`` rewrites live gates and nothing else. + + Before this existed, the release PR resolved 54 placeholders by hand off + the checker's own output. That is a machine's job: the scanner already + separates a live gate from prose with perfect precision, so a human doing + the edit can only introduce error -- and a blanket ``sed`` provably does, + because the process docs legitimately quote the token. + """ + + def test_live_gate_is_rewritten(self, tmp_path: Path) -> None: + f = _write(tmp_path, "g.md", "- `--flag` (since vNEXT) does a thing\n") + changed = resolve_vnext([f], "0.91.0") + assert len(changed) == 1 + assert f.read_text(encoding="utf-8") == "- `--flag` (since 0.91.0) does a thing\n" + + def test_prose_inside_backticks_is_left_alone(self, tmp_path: Path) -> None: + """CLAUDE.md documents the placeholder; a blanket sed corrupts that.""" + body = "tag it with the literal placeholder **`vNEXT`** -- `(since vNEXT)`.\n" + f = _write(tmp_path, "g.md", body) + assert resolve_vnext([f], "0.91.0") == [] + assert f.read_text(encoding="utf-8") == body + + def test_mixed_line_rewrites_only_the_live_token(self, tmp_path: Path) -> None: + """The case a line-level rewrite gets wrong -- one quoted, one live.""" + f = _write(tmp_path, "g.md", "`vNEXT` is the placeholder; (since vNEXT) is live\n") + changed = resolve_vnext([f], "0.91.0") + assert len(changed) == 1 + expected = "`vNEXT` is the placeholder; (since 0.91.0) is live\n" + assert f.read_text(encoding="utf-8") == expected + + def test_vnext_plus_form_is_rewritten(self, tmp_path: Path) -> None: + """``vNEXT+`` is the other documented placeholder shape.""" + f = _write(tmp_path, "g.md", "- **vNEXT+**: resolves to the first project\n") + resolve_vnext([f], "0.91.0") + assert "0.91.0+" in f.read_text(encoding="utf-8") + + def test_python_docstring_gate_is_rewritten(self, tmp_path: Path) -> None: + """``src/**/*.py`` carries agent-facing gates too, so it must resolve.""" + f = _write(tmp_path, "mod.py", '"""Does a thing (since vNEXT)."""\n') + assert len(resolve_vnext([f], "0.91.0")) == 1 + assert "(since 0.91.0)" in f.read_text(encoding="utf-8") + + def test_is_idempotent(self, tmp_path: Path) -> None: + f = _write(tmp_path, "g.md", "- `--flag` (since vNEXT)\n") + resolve_vnext([f], "0.91.0") + assert resolve_vnext([f], "0.91.0") == [] + + def test_reports_every_rewritten_location(self, tmp_path: Path) -> None: + f = _write(tmp_path, "g.md", "## H\n\n(since vNEXT) one\nplain\n(since vNEXT) two\n") + changed = resolve_vnext([f], "0.91.0") + assert [c.line for c in changed] == [3, 5] + + def test_file_without_a_placeholder_is_not_touched(self, tmp_path: Path) -> None: + """No rewrite means no mtime churn on 900+ scanned files.""" + f = _write(tmp_path, "g.md", "nothing to see\n") + before = f.stat().st_mtime_ns + assert resolve_vnext([f], "0.91.0") == [] + assert f.stat().st_mtime_ns == before + + def test_rejects_a_malformed_version(self, tmp_path: Path) -> None: + """A garbled VERSION= must not be written into every gate in the tree. + + Note ``packaging`` is lenient about shapes that merely LOOK wrong -- + ``v0.91`` and ``0.91`` both parse. Format validation therefore cannot + catch a typo'd-but-parseable version; that is what the pyproject + cross-check in ``main()`` is for (see + :class:`TestResolveModeGuardsTheVersion`). + """ + f = _write(tmp_path, "g.md", "(since vNEXT)\n") + try: + resolve_vnext([f], "0.91.0.banana") + except ValueError: + pass + else: # pragma: no cover - the assert below reports the miss + raise AssertionError("expected ValueError for a malformed version") + assert "vNEXT" in f.read_text(encoding="utf-8") + + +class TestResolveModeGuardsTheVersion: + """``--resolve X`` must refuse any X that is not what pyproject ships. + + Format validation cannot catch this: ``v0.91`` and ``0.91`` are both valid + PEP 440. But resolving gates to a version the release is not actually + shipping recreates the exact bug the gate exists to prevent -- an agent + refusing a command the user has -- across the whole tree at once, and the + ``version-gate-check`` that would notice runs against CHANGELOG keys, not + against pyproject. + """ + + def _run(self, monkeypatch, tmp_path: Path, version: str) -> tuple[int, Path]: + f = _write(tmp_path, "g.md", "- `--flag` (since vNEXT)\n") + monkeypatch.setattr(check_version_gates, "resolve_paths", lambda: [f]) + monkeypatch.setattr(sys, "argv", ["check_version_gates.py", "--resolve", version]) + return check_version_gates.main(), f + + def test_version_matching_pyproject_is_applied(self, monkeypatch, tmp_path: Path) -> None: + shipped = check_version_gates._pyproject_version() + rc, f = self._run(monkeypatch, tmp_path, shipped) + assert rc == 0 + assert f"(since {shipped})" in f.read_text(encoding="utf-8") + + def test_version_disagreeing_with_pyproject_is_refused( + self, monkeypatch, tmp_path: Path + ) -> None: + rc, f = self._run(monkeypatch, tmp_path, "9.9.9") + assert rc == 1 + assert "vNEXT" in f.read_text(encoding="utf-8"), "nothing may be rewritten on refusal" + + def test_malformed_version_is_refused_without_touching_files( + self, monkeypatch, tmp_path: Path + ) -> None: + rc, f = self._run(monkeypatch, tmp_path, "0.91.0.banana") + assert rc == 1 + assert "vNEXT" in f.read_text(encoding="utf-8")