From ea090c7f72b825c7c57f26d5cefd795424d38da0 Mon Sep 17 00:00:00 2001 From: sudolulo Date: Mon, 13 Jul 2026 17:58:15 +0000 Subject: [PATCH] Audit fixes: a compat verdict must never be able to brick a working box The audit found the new machinery could do more harm than the bugs it prevents. - apply.sh reused the 'nothing left to do' exit -- which touches the PERMANENT kill switch, cleared only by install.sh, never by update.sh -- for the incompatible case. On TrueNAS 26 (providers ok, nested opt-out) both modules go quiet, so the switch would fire and the release that fixed 26 could never re-enable itself. Retirement and incompatibility now take different exits. - A network blip, a re-export, or a conditional def all read as BROKEN. Each is now 'unknown', which changes nothing, rather than evidence strong enough to disable a module. - 'native' outranked BROKEN everywhere but apply.sh, so a TrueNAS that reworded the guard AND reshaped the functions rendered as good news. - compat.py --tree read B2_BLOCK's own 'restic = True' as native support, so the documented way to check a live box lied on every patched machine. - The signature check was a name-subset test. It passed reorders, kw-only conversions, and added required params -- and it had already passed a real bug: restic_backup takes 4 args on 24.10/25.04, and the wrapper forwarded 5. Nested backups have been raising TypeError on those releases the whole time. The wrapper now forwards *args/**kwargs. - release.sh --promote was unreachable: it died if the tag existed, the gate died if it did not. The tests hid it by always tagging first. --- .github/workflows/compat.yml | 7 +- .github/workflows/release.yml | 8 +- CHANGELOG.md | 79 ++++++--- patch/apply.sh | 93 +++++++++-- release.sh | 48 +++++- tests/test_apply_blocks.py | 30 ++++ tests/test_compat.py | 233 ++++++++++++++++++++++++++ tests/test_release_gate.py | 39 ++++- tools/compat.py | 304 ++++++++++++++++++++++++++++------ tools/release_gate.py | 32 +++- 10 files changed, 773 insertions(+), 100 deletions(-) create mode 100644 tests/test_compat.py diff --git a/.github/workflows/compat.yml b/.github/workflows/compat.yml index 8fcbda6..b32cafa 100644 --- a/.github/workflows/compat.yml +++ b/.github/workflows/compat.yml @@ -84,12 +84,13 @@ jobs: broken = [ r for r in rows - if any(not m["ok"] and not m["native"] for m in r["modules"].values()) + if any(compat.is_broken(m) for m in r["modules"].values()) ] native = [ (r["ref"], mod) for r in rows - for mod, m in sorted(r["modules"].items()) if m["native"] + for mod, m in sorted(r["modules"].items()) + if m["native"] and not compat.is_broken(m) ] lines = [ @@ -103,7 +104,7 @@ jobs: lines.append(f"### {r['ref']}") lines.append("") for mod, m in sorted(r["modules"].items()): - if m["ok"] or m["native"]: + if not compat.is_broken(m): continue lines.append(f"**{mod}** — the patch will not apply:") lines.append("") diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index b529844..612389b 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -135,8 +135,12 @@ jobs: GH_TOKEN: ${{ github.token }} TAG: ${{ steps.tag.outputs.tag }} run: | + # Lowercased: release_gate/release_notes match the suffix case-INsensitively + # (`is_prerelease` uses re.I), so a `v0.6.0-RC1` skipped the barrier as a + # candidate and then landed here as a case-sensitive MISS -- published as the + # forge's "Latest release" on a commit that was never a candidate. prerelease="" - case "$TAG" in + case "$(printf '%s' "$TAG" | tr '[:upper:]' '[:lower:]')" in *-rc*|*-beta*|*-alpha*) prerelease="--prerelease" ;; esac @@ -156,7 +160,7 @@ jobs: API: ${{ github.server_url }}/api/v1/repos/${{ github.repository }} run: | prerelease=false - case "$TAG" in + case "$(printf '%s' "$TAG" | tr '[:upper:]' '[:lower:]')" in *-rc*|*-beta*|*-alpha*) prerelease=true ;; esac diff --git a/CHANGELOG.md b/CHANGELOG.md index 53ba319..9763bca 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,23 +26,12 @@ worse than no alert, because one day it carries a security fix. then I pushed one more little fix" is refused by name — that is precisely how v0.5.1 happened. -### Changed - -- **A stable release may not leave work stranded under `## Unreleased`.** Either it - is finished and belongs in the release, or the release is premature. Candidates - are exempt: an rc may legitimately have work queued behind it. - -- **`release.sh` refuses to run on an installed box.** The whole repo is cloned onto - every box, so this file is there too; `update.sh` pins the checkout to a tag in - detached HEAD, and `release.sh` now recognises that and says so, rather than - emitting a confusing branch error. - - **TrueNAS compatibility is now checked, not hoped for.** [`tools/compat.py`](tools/compat.py) is a written-down record of everything each module assumes about middlewared, checked in two places: - **CI, daily** — against iXsystems' source at every release line *including - `master` and the current BETA/RC_. When an unreleased TrueNAS breaks the patch + `master` and the current BETA/RC*. When an unreleased TrueNAS breaks the patch it files a bug report automatically, so there is time to fix it before that version reaches anyone. It also refreshes the README's support matrix, so the table cannot quietly become a false promise. @@ -50,11 +39,61 @@ worse than no alert, because one day it carries a security fix. box. **A module whose assumptions no longer hold is not applied.** Stock TrueNAS without a feature beats TrueNAS with a broken backup. - This immediately found a real one: **TrueNAS 26 rewrites the entire `cloud_backup` - path from async to synchronous.** Every block the nested module injects is an - `async def` wrapping an `await`ed original, so on 26 it hands `sync.py` a coroutine - where it unpacks a tuple. Nobody would have found out until a restore failed. On - TrueNAS 26 the nested module now simply stays off. + It immediately found two real breaks: TrueNAS 26 (below), and a nested-snapshot bug + that had been shipping for two releases (below). + +### Fixed + +- **Nested snapshots were broken on TrueNAS 24.10 and 25.04, and had been all + along.** `SYNC_BLOCK`'s wrapper spelled out the stock signature and forwarded five + arguments — but those releases declare `restic_backup(middleware, job, + cloud_backup, dry_run)`; `rate_limit` only arrived in 25.10. Every nested backup on + 24.10/25.04 raised `TypeError: restic_backup() takes 4 positional arguments but 5 + were given`. The wrapper now takes `*args, **kwargs` and forwards whatever it is + handed, so a trailing parameter appearing or disappearing is a non-event. + + Found by the new compatibility check, not by a user — which is the whole argument + for having it. The check it replaced only asked whether the parameter *names* still + appeared somewhere in the signature, so it happily passed a call that could never + work. + +- **TrueNAS 26 rewrites the entire `cloud_backup` path from async to synchronous.** + Every block the nested module injects is an `async def` wrapping an `await`ed + original, so on 26 it would hand `sync.py` a coroutine where it unpacks a tuple — + a broken backup, discovered at restore time. On TrueNAS 26 the nested module now + stays off rather than applying and breaking. + +- **An incompatible TrueNAS no longer sets the permanent kill switch.** `apply.sh` + reused a "nothing left to do" exit that touches `disabled`, which suppresses + patching on every future boot and is cleared only by `install.sh` — never by + `update.sh`. On TrueNAS 26 (providers-compatible, nested opt-out by default) that + branch would have fired, and the very release that fixed 26 could not have + re-enabled itself: the user would run `bash update.sh`, exactly as the update alert + tells them to, and the patch would stay dead with their B2 backups off. + Incompatibility now means "apply nothing this boot, try again next boot". + Retirement and incompatibility are opposite situations and no longer share an exit. + +- **The compatibility check itself could be fooled**, in ways that each had teeth: a + reordered, keyword-only, or newly-required parameter now reads as broken (the patch + calls these positionally); a **re-exported or conditionally-defined** symbol reads + as *unknown* rather than broken, so an innocent upstream refactor cannot make a + working module decline to apply; an **unreadable** source (rate limit, DNS, timeout) + is *unknown* rather than "iXsystems deleted this file", so a network blip cannot + file a bug report, fail CI, and repaint the published support matrix; and `native` + no longer masks `BROKEN`, which used to render a TrueNAS that both reworded the + nesting guard *and* reshaped the functions as good news. + +- **`compat.py --tree` no longer reads the patch's own code as native support.** + `B2_BLOCK` writes `B2RcloneRemote.restic = True` into `b2.py` — exactly the string + the providers native-probe looks for — so the one command the docs recommend for + checking a live box said "retire the providers module" on every *patched* machine. + It now reads only the part of the file iXsystems wrote. + +- **`release.sh --promote` could never succeed.** It refused to run if the stable tag + existed, and the gate refused if it did not — mutually exclusive, so the only way to + cut a stable release was to hand-tag and bypass every gate this work exists to + enforce. The gate now resolves the tag's commit if it exists and `HEAD` otherwise. + The tests hid it by always tagging first. ### Changed @@ -71,9 +110,9 @@ worse than no alert, because one day it carries a security fix. GitHub as a mirror. Both forges run the same workflows and publish the same releases. The update alert now **derives the changelog URL from the `origin` remote** instead of hard-coding GitHub — which matters more than it sounds: when - the changelog cannot be read, the alert deliberately fires *anyway* rather than - risk hiding a security fix, so a stale URL would not have disabled the alert, it - would have made it nag on every release including documentation-only ones. + the changelog cannot be read, the alert deliberately fires *anyway* rather than risk + hiding a security fix, so a stale URL would not have disabled the alert, it would + have made it nag on every release, including documentation-only ones. ### Security diff --git a/patch/apply.sh b/patch/apply.sh index ddfae69..2f8445c 100755 --- a/patch/apply.sh +++ b/patch/apply.sh @@ -227,6 +227,17 @@ fi _TC_COMPAT_JSON="$PATCH_DIR/incompatible.json" rm -f "$_TC_COMPAT_JSON" +_tc_incompatible=0 +_tc_compat=unknown + +# No middlewared directory means the checker has nothing to read -- every module +# would look "broken" because every file is missing, which is the strongest possible +# evidence derived from the weakest possible input. Skip the preflight entirely and +# let the existing "Cannot determine middlewared directory" path handle it. +if [ -z "$_MW_DIR" ]; then + echo "NOTICE: middlewared directory unknown; skipping the compatibility preflight." + _tc_compat=$(printf 'unknown\nunknown\n') +else _tc_compat=$("$PYTHON" - "$PATCH_DIR" "$_MW_DIR" "$_TC_COMPAT_JSON" 2>/dev/null <<'PYEOF' || printf 'unknown\nunknown\n' import json, os, sys @@ -244,7 +255,19 @@ except Exception: raise SystemExit(0) def verdict(r): - return 'broken' if (not r['ok'] and not r['native']) else 'ok' + # Deliberately NOT exempting 'native' here, unlike the CI matrix. + # + # 'native' answers "do we still NEED this module?"; 'ok' answers "is it still + # SAFE to inject?". They are different questions, and letting native mask a + # broken assumption conflates them: a future TrueNAS that both reworded the + # nesting guard (-> native) AND changed the signatures (-> broken) would read + # as safe, and we would patch it anyway. + # + # Refusing to apply is the correct action for BOTH answers -- a native module + # is unnecessary and a broken one is dangerous -- so the apply path only has to + # ask whether the assumptions hold. Whether the feature went native is decided + # separately, by the probes above, and only affects the wording of the notice. + return 'ok' if r['ok'] else 'broken' broken = {m: r for m, r in result.items() if verdict(r) == 'broken'} if broken: @@ -260,6 +283,7 @@ print(verdict(result['providers'])) print(verdict(result['nested'])) PYEOF ) +fi _tc_compat_providers=$(printf '%s' "$_tc_compat" | sed -n '1p') _tc_compat_nested=$(printf '%s' "$_tc_compat" | sed -n '2p') @@ -273,20 +297,63 @@ if [ "$_NESTED_ENABLED" = "1" ] && [ "$_tc_native_nested" != "yes" ]; then _nested_needed=1 fi -if [ "$_tc_compat_providers" = "broken" ]; then +# The native checks above have already zeroed _*_needed for anything TrueNAS now +# does itself, and printed the (good) news. Only complain about a module that is +# still NEEDED and no longer fits -- otherwise a version that took a feature native +# AND reshaped the module would be announced as "NOT COMPATIBLE", which is alarming +# and false. +if [ "$_tc_compat_providers" = "broken" ] && [ "$_providers_needed" = "1" ]; then echo "WARNING: truecloud-patch is NOT COMPATIBLE with this TrueNAS version." echo "WARNING: The B2/S3 providers module will NOT be applied. TrueCloud is" echo "WARNING: left stock, so B2/S3 tasks will not run until this is fixed." echo "WARNING: Details: $_TC_COMPAT_JSON" _providers_needed=0 + _tc_incompatible=1 fi -if [ "$_tc_compat_nested" = "broken" ] && [ "$_NESTED_ENABLED" = "1" ]; then +if [ "$_tc_compat_nested" = "broken" ] && [ "$_nested_needed" = "1" ]; then echo "WARNING: truecloud-patch's nested-snapshot module is NOT COMPATIBLE with" echo "WARNING: this TrueNAS version and will NOT be applied. Backups still" echo "WARNING: run; datasets nested under the target are not included." echo "WARNING: Details: $_TC_COMPAT_JSON" _nested_needed=0 + _tc_incompatible=1 +fi + +_tc_unmount_overlays() { + for _tag in mw ui; do + if mount | grep -qF "truecloud-${_tag} on "; then + _mnt=$(mount | grep "truecloud-${_tag} on " | awk '{print $3}' | head -1) + if umount "$_mnt" 2>/dev/null; then + echo "NOTICE: Unmounted overlay on $_mnt" + fi + fi + done +} + +# INCOMPATIBLE is not the same as RETIRED, and must never take the same exit. +# +# The kill switch below is permanent -- apply.sh checks for it and returns early on +# every future boot -- and only install.sh removes it, NOT update.sh. That is right +# for retirement ("TrueNAS does this natively now; stop forever"), and catastrophic +# for incompatibility: on TrueNAS 26 the providers module fails its assumptions and +# nested is opt-out by default, so BOTH would be zero, the kill switch would fire, +# and the very release that fixes 26 could never re-enable itself. The user would +# run `bash update.sh` -- exactly what the update alert tells them to do -- and the +# patch would stay dead, silently, with their B2 backups off. +# +# So: incompatible means "apply nothing THIS boot, and try again next boot". The +# fix ships, update.sh checks it out, the next boot re-runs the preflight, the +# assumptions hold, and the patch comes back by itself. +if [ "$_tc_incompatible" = "1" ] && [ "$_providers_needed" = "0" ] && [ "$_nested_needed" = "0" ]; then + echo "NOTICE: Nothing can be applied on this TrueNAS version — see the WARNINGs above." + echo "NOTICE: The kill switch is deliberately NOT set: this is an incompatibility," + echo "NOTICE: not a retirement. Install a release that supports this TrueNAS" + echo "NOTICE: bash $PATCH_DIR/update.sh" + echo "NOTICE: and the patch will re-apply itself on the next boot." + _tc_unmount_overlays + echo "=== done ===" + exit 0 fi if [ "$_providers_needed" = "0" ] && [ "$_nested_needed" = "0" ]; then @@ -301,12 +368,7 @@ if [ "$_providers_needed" = "0" ] && [ "$_nested_needed" = "0" ]; then echo "NOTICE: Run the following to fully remove the patch:" echo "NOTICE: bash $PATCH_DIR/uninstall.sh" touch "$PATCH_DIR/disabled" - for _tag in mw ui; do - if mount | grep -qF "truecloud-${_tag} on "; then - _mnt=$(mount | grep "truecloud-${_tag} on " | awk '{print $3}' | head -1) - umount "$_mnt" 2>/dev/null && echo "NOTICE: Unmounted overlay on $_mnt" || true - fi - done + _tc_unmount_overlays echo "=== done ===" exit 0 fi @@ -524,12 +586,21 @@ except ImportError: if _tc_nested is not None: _tc_orig_restic_backup = restic_backup - async def restic_backup(middleware, job, cloud_backup, dry_run=False, rate_limit=None): + async def restic_backup(middleware, job, cloud_backup, *args, **kwargs): + # *args/**kwargs, not the stock signature spelled out. + # + # 24.10 and 25.04 have `restic_backup(middleware, job, cloud_backup, dry_run)`; + # 25.10 added `rate_limit`. Naming them here and forwarding all five raised + # `TypeError: takes 4 positional arguments but 5 were given` on every nested + # backup on the two older releases. Forwarding whatever we were handed makes + # this wrapper indifferent to iX adding or dropping a trailing parameter -- + # which they have now done twice. + # # Our bind mounts pin the ZFS snapshot, so stock's `finally` cannot # destroy it (EBUSY) and logs one benign warning. We unmount here and # then delete the snapshot for real. try: - return await _tc_orig_restic_backup(middleware, job, cloud_backup, dry_run, rate_limit) + return await _tc_orig_restic_backup(middleware, job, cloud_backup, *args, **kwargs) finally: try: await _tc_nested.cleanup_task( diff --git a/release.sh b/release.sh index 3c4aeb8..ed40169 100644 --- a/release.sh +++ b/release.sh @@ -77,9 +77,16 @@ printf '%s' "$target" | grep -Eq '^[0-9]+\.[0-9]+\.[0-9]+$' \ [ -d .git ] || die "not a git checkout" -if ! git diff --quiet || ! git diff --cached --quiet; then - die "working tree is dirty. Commit or stash first -- a release must be - reproducible from a commit, not from whatever happened to be on disk." +# UNTRACKED files count as dirty, because --rc runs `git add -A`: an untracked file +# lying around would be swept into the release commit and pushed. This checkout is +# routinely shared between sessions, so stray files are the normal state here, not +# an exotic one. (Anything genuinely ignorable belongs in .gitignore.) +if [ -n "$(git status --porcelain)" ]; then + echo " Working tree is not clean:" >&2 + git status --short >&2 + die "commit, stash, or ignore the above first -- a release must be reproducible + from a commit, not from whatever happened to be on disk. --rc runs + 'git add -A', so an untracked file here ships inside the release." fi branch="$(git rev-parse --abbrev-ref HEAD)" @@ -103,6 +110,17 @@ if [ -n "$(git log --oneline "origin/$branch..$branch" 2>/dev/null)" ]; then at a commit the world can actually fetch." fi +# BEHIND is just as bad as ahead, and less obvious. --rc commits the version stamp, +# creates the tag, and only THEN pushes -- so on a stale main the push is rejected +# (non-fast-forward) *after* the tag exists and `## Unreleased` has already been +# consumed. Re-running then sees rc1, cuts rc2, and silently skips the stamping +# step; the rc1 tag dangles locally forever. +if [ -n "$(git log --oneline "$branch..origin/$branch" 2>/dev/null)" ]; then + die "local main is BEHIND origin. Pull first: git pull --ff-only + Releasing from a stale main half-completes: the tag is cut locally, the push + is rejected, and '## Unreleased' has already been consumed." +fi + # ── the gates: identical to the ones CI will run ───────────────────────────── run_gates() { @@ -167,11 +185,25 @@ fi if [ "$mode" = "rc" ]; then tag="$(python3 tools/release_gate.py "$target" --next-rc -C .)" + # Tests BEFORE the stamping commit, deliberately. + # + # Stamping consumes `## Unreleased` and makes a "release vX.Y.Z" commit. If the + # suite then failed, that commit was already on main and a re-run died inside + # promote() with "no `## Unreleased` content" -- the release was wedged, and the + # only way out was to hand-unpick a commit. Failing first leaves the tree + # untouched. + run_tests + # The first candidate promotes `## Unreleased` and stamps the version into every # script. Later candidates (rc2+) are re-cuts of an already-stamped version, so # they only tag -- the CHANGELOG section for this version already exists, and # fixes found during rc go into it. - if git rev-parse -q --verify "refs/tags/v$target-rc1" >/dev/null; then + # + # "Already stamped" is decided by the TREE, not by the rc1 tag: if a previous run + # stamped and committed but died before tagging (or before pushing), the tag is + # absent while the stamp is present, and re-stamping would try to promote an + # `## Unreleased` section that is no longer there. + if python3 tools/release_notes.py check "v$target-rc0" >/dev/null 2>&1; then note "v$target is already stamped; cutting a follow-up candidate" else note "promoting '## Unreleased' -> v$target and stamping the scripts" @@ -215,8 +247,8 @@ PY # Gated as the rc tag it is: content is checked against the base version, and the # provenance gate is a no-op for candidates -- being one is the whole point. + # (run_tests already ran, above, before anything was committed.) run_gates "$tag" - run_tests echo note "about to cut $tag" @@ -227,8 +259,12 @@ PY echo " run: bash release.sh $target --promote" confirm "cut $tag?" - git tag -a "$tag" -m "$tag" + # Push the branch FIRST. If it is rejected, no tag has been created yet -- a tag + # pointing at a commit nobody else has is worse than no tag, because the next run + # sees it, counts it as a candidate, and cuts rc2 against a commit that was never + # published. git push --quiet origin main + git tag -a "$tag" -m "$tag" git push --quiet origin "$tag" ok "pushed $tag" echo diff --git a/tests/test_apply_blocks.py b/tests/test_apply_blocks.py index 0d89139..38d417b 100644 --- a/tests/test_apply_blocks.py +++ b/tests/test_apply_blocks.py @@ -307,3 +307,33 @@ def test_guard_is_relaxed_only_after_traversal_is_installed(): src.index("patch_file(crud_py, CRUD_BLOCK)"), ] assert order == sorted(order), "crud.py must be patched last" + + +class TestWrappersDoNotHardcodeStockArity: + """iX changes the tail of these signatures between releases. + + SYNC_BLOCK used to spell out `(middleware, job, cloud_backup, dry_run, rate_limit)` + and forward all five. But 24.10 and 25.04 declare only four -- `rate_limit` arrived + in 25.10 -- so every nested backup on those two releases raised + `TypeError: restic_backup() takes 4 positional arguments but 5 were given`. + It shipped broken and nothing noticed, because the compat check at the time only + asked whether the parameter NAMES still appeared somewhere in the signature. + + Forwarding *args/**kwargs makes the wrapper indifferent to a trailing parameter + being added or dropped, which is the only part iX actually churns. + """ + + def test_restic_backup_forwards_rather_than_naming_stock_params(self): + block = extract_blocks()["SYNC_BLOCK"] + assert "async def restic_backup(middleware, job, cloud_backup, *args, **kwargs)" in block + assert "_tc_orig_restic_backup(middleware, job, cloud_backup, *args, **kwargs)" in block + + # Comments stripped: the block's own commentary explains the rate_limit + # history, and that must not be mistaken for the code re-declaring it. + code = "\n".join( + line for line in block.splitlines() + if not line.lstrip().startswith("#") + ) + assert "rate_limit" not in code, ( + "naming a trailing stock parameter re-introduces the arity bug" + ) diff --git a/tests/test_compat.py b/tests/test_compat.py new file mode 100644 index 0000000..5ca6d9c --- /dev/null +++ b/tests/test_compat.py @@ -0,0 +1,233 @@ +"""Tests for the middlewared compatibility manifest. + +Two failure directions, and they are NOT symmetric: + + * a false **BROKEN** makes a module decline to apply on a box where it works. + Worse, if both modules go quiet, apply.sh used to set a PERMANENT kill switch + that only install.sh clears -- so a network blip or an innocent refactor could + take a working box's B2 backups down until someone noticed by hand. + + * a false **OK** lets the patch inject into middleware it does not fit, which is a + broken backup discovered at restore time. + +Both are tested. The `native` verdict gets its own scrutiny because it is the most +dangerous thing this file can say -- it means "TrueNAS does this now, retire the +module" -- and it rests on nothing more than a substring match. +""" + +import os +import sys + +import pytest + +sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..", "tools")) + +import compat # noqa: E402 +from compat import ( # noqa: E402 + NESTED, + PROVIDERS, + Unreadable, + check, + is_broken, +) + +# A middlewared that the patch fits: TrueNAS 25.10 in miniature. +GOOD = { + "rclone/remote/b2.py": "class B2RcloneRemote(BaseRcloneRemote):\n pass\n", + "plugins/cloud_backup/restic.py": ( + "class ResticConfig:\n cmd: list\n\n" + "def get_restic_config(cloud_backup):\n return ResticConfig([], {})\n" + ), + "plugins/cloud/snapshot.py": ( + 'async def create_snapshot(middleware, path, name="x"):\n return "s", "p"\n' + ), + "plugins/cloud/crud.py": ( + "class CloudTaskServiceMixin:\n" + " async def _validate(self, app, verrors, name, data):\n" + " verrors.add('x', 'datasets that have no further '\n" + " 'nesting')\n" + ), + "plugins/cloud_backup/sync.py": ( + "async def restic_backup(middleware, job, cloud_backup, dry_run=False, " + "rate_limit=None):\n pass\n" + ), +} + + +def loader(files): + def load(path): + if path not in files: + return None + v = files[path] + if isinstance(v, Exception): + raise v + return v + return load + + +def check_files(files, modules=None): + return check(loader(files), modules) + + +def with_(**overrides): + files = dict(GOOD) + files.update(overrides) + return files + + +class TestTheBaseline: + def test_a_good_tree_is_ok_and_not_native(self): + r = check_files(GOOD) + for mod in (PROVIDERS, NESTED): + assert r[mod]["ok"], r[mod]["problems"] + assert not r[mod]["native"] + assert not r[mod]["unknown"] + + +class TestFalseOkWouldBreakBackups: + """The patch calls the originals POSITIONALLY. A name-subset check passed all of + these, and each is a TypeError or -- worse -- silently swapped arguments.""" + + def test_reordered_parameters_are_broken(self): + r = check_files(with_(**{ + "plugins/cloud/snapshot.py": + 'async def create_snapshot(name, path, middleware):\n return 1, 2\n', + })) + assert is_broken(r[NESTED]) + + def test_a_keyword_only_conversion_is_broken(self): + r = check_files(with_(**{ + "plugins/cloud/snapshot.py": + 'async def create_snapshot(middleware, *, path, name="x"):\n return 1, 2\n', + })) + assert is_broken(r[NESTED]) + + def test_a_new_required_parameter_is_broken(self): + r = check_files(with_(**{ + "plugins/cloud/snapshot.py": + 'async def create_snapshot(middleware, path, name, dataset):\n return 1, 2\n', + })) + assert is_broken(r[NESTED]) + + def test_a_new_optional_parameter_is_fine(self): + # The patch simply will not pass it. Refusing here would be false BROKEN. + r = check_files(with_(**{ + "plugins/cloud/snapshot.py": + 'async def create_snapshot(middleware, path, name="x", quiet=False):\n' + " return 1, 2\n", + })) + assert r[NESTED]["ok"], r[NESTED]["problems"] + + def test_the_master_signature_change_is_caught(self): + # iX really did rename this on master: get_restic_config(entry, credentials). + # RESTIC_BLOCK rebinds the module-level name to a 1-arg wrapper, so getting + # this wrong kills EVERY TrueCloud task -- Storj included. + r = check_files(with_(**{ + "plugins/cloud_backup/restic.py": + "class ResticConfig:\n cmd: list\n\n" + "def get_restic_config(entry, credentials):\n pass\n", + })) + assert is_broken(r[PROVIDERS]) + + def test_async_to_sync_is_caught(self): + # THE TrueNAS 26 change. + r = check_files(with_(**{ + "plugins/cloud/snapshot.py": + 'def create_snapshot(middleware, path, name="x"):\n return 1, 2\n', + })) + assert is_broken(r[NESTED]) + assert "async def" in r[NESTED]["problems"][0]["detail"] + + +class TestFalseBrokenWouldDisableWorkingBoxes: + def test_a_conditionally_defined_symbol_is_not_broken(self): + r = check_files(with_(**{ + "plugins/cloud/snapshot.py": + "try:\n" + " from .fast import create_snapshot\n" + "except ImportError:\n" + ' async def create_snapshot(middleware, path, name="x"):\n' + " return 1, 2\n", + })) + assert not is_broken(r[NESTED]), r[NESTED]["problems"] + + def test_a_re_exported_symbol_is_unknown_not_broken(self): + r = check_files(with_(**{ + "plugins/cloud_backup/restic.py": + "from ._impl import ResticConfig, get_restic_config\n", + })) + assert not is_broken(r[PROVIDERS]) + assert r[PROVIDERS]["unknown"] + + def test_an_unreadable_source_is_unknown_not_broken(self): + # A rate limit (the matrix makes ~30 unauthenticated requests) must not be + # able to say "iX deleted six files, both modules are broken". + r = check_files(with_(**{ + "plugins/cloud/snapshot.py": Unreadable("HTTP 429"), + })) + assert not is_broken(r[NESTED]) + assert r[NESTED]["unknown"] + + def test_a_definite_break_still_wins_over_an_unknown(self): + r = check_files(with_(**{ + "plugins/cloud/snapshot.py": Unreadable("HTTP 429"), + "plugins/cloud_backup/sync.py": + "def restic_backup(middleware, job, cloud_backup, dry_run=False, " + "rate_limit=None):\n pass\n", + })) + assert is_broken(r[NESTED]), "unknown must not launder away a proven break" + + +class TestTheNativeVerdict: + """"native" means "retire the module". It is the most destructive thing this file + can say, and it is only a substring match — so it must never outrank BROKEN.""" + + def test_broken_outranks_native(self): + # Guard reworded (reads as native) AND the signatures changed (really broken). + # This used to render as good news: green CI, no bug report, and a README row + # telling users the feature went native while it was in fact broken. + r = check_files(with_(**{ + "plugins/cloud/crud.py": + "class CloudTaskServiceMixin:\n" + " def _validate(self, app, verrors, name, data):\n" + " verrors.add('x', 'no children allowed')\n", + })) + assert r[NESTED]["native"] + assert is_broken(r[NESTED]) + assert compat._verdict(r[NESTED]) == "BROKEN" + + def test_an_already_patched_tree_does_not_read_as_native(self): + # B2_BLOCK writes `B2RcloneRemote.restic = True` into b2.py. Scanning the whole + # file finds OUR OWN line and concludes TrueNAS went native — so the command + # compat.py's docstring recommends for a live box (`--tree /usr/lib/...`) + # reported providers as native on every patched machine. + r = check_files(with_(**{ + "rclone/remote/b2.py": + "class B2RcloneRemote(BaseRcloneRemote):\n pass\n" + "\n# TRUECLOUD_PATCH — added by truenas-truecloud-patch/patch/apply.sh\n" + "B2RcloneRemote.restic = True\n", + })) + assert not r[PROVIDERS]["native"], "read its own patch as native support" + assert r[PROVIDERS]["ok"] + + def test_a_genuinely_native_b2_is_native(self): + r = check_files(with_(**{ + "rclone/remote/b2.py": + "class B2RcloneRemote(BaseRcloneRemote):\n restic = True\n", + })) + assert r[PROVIDERS]["native"] + + +class TestUpdateReadmeCannotPublishAGuess: + def test_it_refuses_when_anything_is_unknown(self, tmp_path): + readme = tmp_path / "README.md" + readme.write_text(f"x\n{compat.BEGIN}\nold\n{compat.END}\ny\n") + rows = [{ + "ref": "TS-25.10.4", "unreleased": False, + "modules": check_files(with_(**{ + "plugins/cloud/snapshot.py": Unreadable("HTTP 429"), + })), + }] + with pytest.raises(Unreadable): + compat.update_readme(rows, path=str(readme)) + assert "old" in readme.read_text(), "a blip must not repaint the matrix" diff --git a/tests/test_release_gate.py b/tests/test_release_gate.py index 9a8a626..210d796 100644 --- a/tests/test_release_gate.py +++ b/tests/test_release_gate.py @@ -65,6 +65,33 @@ def repo(tmp_path): return r +class TestTheBarrierBeforeTheTagExists: + """release.sh calls the gate BEFORE creating the stable tag. + + Every other test here tags first, and that is what let a fatal bug ship green: + `check_promotable` began with `commit_for(vX.Y.Z)` and returned "does not exist", + while release.sh's own guard refuses to run at all IF the tag exists. The two + conditions were mutually exclusive, so `--promote` could never succeed -- the + only way to cut a stable release was to hand-tag, bypassing every gate. + + A gate that can only be satisfied after the thing it gates is not a gate. + """ + + def test_promote_is_allowed_when_head_was_a_candidate_and_the_tag_is_absent(self, repo): + repo.git("tag", "v1.0.0-rc1") # rc on HEAD, no stable tag yet + assert check_promotable("v1.0.0", cwd=str(repo)) == [] + + def test_promote_is_refused_when_head_was_never_a_candidate(self, repo): + assert check_promotable("v1.0.0", cwd=str(repo)) + + def test_promote_is_refused_when_head_moved_past_the_candidate(self, repo): + repo.git("tag", "v1.0.0-rc1") + repo.commit("one more little fix") # HEAD is no longer the candidate + problems = check_promotable("v1.0.0", cwd=str(repo)) + assert problems + assert "no release candidate does" in problems[0] + + class TestTheBarrier: def test_a_tag_with_no_candidate_is_refused(self, repo): repo.git("tag", "v1.0.0") @@ -109,9 +136,15 @@ class TestTheBarrier: repo.git("tag", "v1.0.0") assert check_promotable("v1.0.0", cwd=str(repo)) == [] - def test_missing_tag_is_reported_not_crashed(self, repo): - problems = check_promotable("v9.9.9", cwd=str(repo)) - assert problems and "does not exist" in problems[0] + def test_a_tag_the_numbering_does_not_understand_is_not_a_candidate(self, repo): + # `v1.0.0-rc*` also globs `v1.0.0-rc1-hotfix`, which _rc_number reads as 0. + # The barrier must be satisfied only by something that really was a candidate. + repo.git("tag", "v1.0.0-rc1-hotfix") + repo.git("tag", "v1.0.0") + problems = check_promotable("v1.0.0", cwd=str(repo)) + assert problems + assert "never a release candidate" in problems[0] + assert rc_tags("1.0.0", cwd=str(repo)) == [] class TestRcNumbering: diff --git a/tools/compat.py b/tools/compat.py index 1f23955..6822df2 100644 --- a/tools/compat.py +++ b/tools/compat.py @@ -65,14 +65,19 @@ class Assumption: """ def __init__(self, ident, module, path, symbol, *, kind="function", - is_async=None, params=None, why=""): + is_async=None, params=None, forwards=False, why=""): self.id = ident self.module = module self.path = path self.symbol = symbol self.kind = kind self.is_async = is_async + #: The positional parameters the patch passes, in order. self.params = params or [] + #: True if the wrapper takes *args/**kwargs and forwards the rest. Then a + #: trailing parameter that iX adds or removes is harmless, and only the + #: leading `params` must still match. + self.forwards = forwards self.why = why @@ -115,8 +120,13 @@ ASSUMPTIONS = [ "drop the no-further-nesting error", ), Assumption( + # SYNC_BLOCK's wrapper is (middleware, job, cloud_backup, *args, **kwargs) and + # forwards the rest, precisely because iX keeps changing the tail: 24.10 and + # 25.04 have `(…, dry_run)`, 25.10 added `rate_limit`. Only the leading three + # are named by the patch, so only they have to hold. "restic-backup", NESTED, "plugins/cloud_backup/sync.py", "restic_backup", - is_async=True, params=["middleware", "job", "cloud_backup"], + is_async=True, forwards=True, + params=["middleware", "job", "cloud_backup"], why="SYNC_BLOCK replaces it with `async def` that AWAITS the original, to " "tear down bind mounts in a finally", ), @@ -162,96 +172,239 @@ def _squash(text: str) -> str: # ── AST lookups ────────────────────────────────────────────────────────────── +_DEFS = (ast.ClassDef, ast.FunctionDef, ast.AsyncFunctionDef) + + +def _defs_in(body): + """Definitions in `body`, descending into if/try/else/with. + + A module-level `def` is not always at module level: + + try: + from .fast import create_snapshot + except ImportError: + async def create_snapshot(...): ... + + Scanning only `tree.body` would say "no longer defines create_snapshot" -- a + false BROKEN. And a false BROKEN is not a harmless over-caution here: it makes a + module decline to apply on a box where it works perfectly. + """ + for node in body: + if isinstance(node, _DEFS): + yield node + elif isinstance(node, ast.If | ast.Try | ast.With | ast.AsyncWith): + yield from _defs_in(node.body) + yield from _defs_in(getattr(node, "orelse", [])) + yield from _defs_in(getattr(node, "finalbody", [])) + for h in getattr(node, "handlers", []): + yield from _defs_in(h.body) + + +def _imports(tree, name): + """True if `name` is bound by an import -- i.e. re-exported from elsewhere.""" + for node in ast.walk(tree): + if isinstance(node, ast.Import | ast.ImportFrom): + for alias in node.names: + if (alias.asname or alias.name.split(".")[0]) == name: + return True + return False + + def _find(tree, symbol): - """The node for `name` or `Class.method`, or None.""" + """The def node for `name` or `Class.method`, or None.""" if "." in symbol: cls_name, meth = symbol.split(".", 1) - for node in tree.body: + for node in _defs_in(tree.body): if isinstance(node, ast.ClassDef) and node.name == cls_name: - for sub in node.body: + for sub in _defs_in(node.body): if isinstance(sub, ast.FunctionDef | ast.AsyncFunctionDef) \ and sub.name == meth: return sub return None - for node in tree.body: - if isinstance(node, ast.ClassDef | ast.FunctionDef | ast.AsyncFunctionDef) \ - and node.name == symbol: + for node in _defs_in(tree.body): + if node.name == symbol: return node return None -def _params(node): +def _positional(node): a = node.args - return [p.arg for p in (*a.posonlyargs, *a.args, *a.kwonlyargs)] + return [p.arg for p in (*a.posonlyargs, *a.args)] -def check_source(a: Assumption, src: str | None) -> str | None: - """The reason assumption `a` no longer holds, or None if it does.""" +def _signature_problem(node, symbol, want, forwards=False): + """Why `symbol`'s signature no longer supports how the patch calls it. + + The injected blocks call the original POSITIONALLY and with a fixed arg list: + + await _tc_orig_create_snapshot(middleware, path, name) + _tc_orig_get_restic_config(cloud_backup) + + So a name-subset test ("are these names still in there somewhere?") is not + enough, and that is what this used to be. It passed a reorder, a keyword-only + conversion, and an added required parameter -- each of which is a TypeError or, + worse, silently correct-looking with the arguments swapped. + + The realistic one is not hypothetical: on `master`, iX already renamed + get_restic_config's parameter and added a second. That function is rebound + module-wide by RESTIC_BLOCK, so a wrong wrapper there kills EVERY TrueCloud + task -- Storj included, for users who never wanted this patch's features. + """ + have = _positional(node) + n = len(want) + + if have[:n] != want: + return ( + f"{symbol}{tuple(have)} — positional parameters changed; the patch " + f"calls it as ({', '.join(want)})" + ) + + # Extra parameters are fine only if they are optional -- the patch will not pass + # them -- OR if the wrapper forwards *args/**kwargs, in which case whatever the + # caller supplied is handed straight through. A new REQUIRED one that we neither + # pass nor forward is a TypeError at the first backup. + args = node.args + required = len(have) - len(args.defaults) + if required > n and not forwards: + return ( + f"{symbol} now requires {', '.join(have[n:required])} — the patch does " + f"not pass it" + ) + + req_kwonly = [ + k.arg for k, d in zip(args.kwonlyargs, args.kw_defaults, strict=False) + if d is None + ] + if req_kwonly and not forwards: + return ( + f"{symbol} now requires keyword-only {', '.join(req_kwonly)} — the " + f"patch does not pass it" + ) + + return None + + +def check_source(a: Assumption, src: str | None) -> tuple[str, str | None]: + """("ok"|"broken"|"unknown", detail). + + "unknown" exists so that "I cannot inspect this" is never reported as "this is + broken". Only "broken" makes a module decline to apply, and declining wrongly + breaks a box that was working. + """ if src is None: - return f"{a.path} does not exist" + return "broken", f"{a.path} does not exist" try: tree = ast.parse(src) except SyntaxError as e: - return f"{a.path} does not parse: {e}" + return "unknown", f"{a.path} does not parse: {e}" node = _find(tree, a.symbol) if node is None: - return f"{a.path} no longer defines {a.symbol}" + root = a.symbol.split(".", 1)[0] + if _imports(tree, root): + # Re-exported: `from ._impl import get_restic_config`. The name is still + # there and the patch's rebinding still works; we simply cannot see the + # signature from here. Refusing to apply over a refactor that changed + # nothing would be worse than not checking. + return "unknown", ( + f"{a.path} re-exports {root} from another module; " + f"cannot verify its signature here" + ) + return "broken", f"{a.path} no longer defines {a.symbol}" if a.kind == "class": if not isinstance(node, ast.ClassDef): - return f"{a.symbol} is no longer a class" - return None + return "broken", f"{a.symbol} is no longer a class" + return "ok", None if isinstance(node, ast.ClassDef): - return f"{a.symbol} is a class, expected a function" + return "broken", f"{a.symbol} is a class, expected a function" got_async = isinstance(node, ast.AsyncFunctionDef) if a.is_async is not None and got_async != a.is_async: want = "async def" if a.is_async else "def" got = "async def" if got_async else "def" - return ( - f"{a.symbol} is now `{got}`, the patch requires `{want}` " - f"({a.path})" + return "broken", ( + f"{a.symbol} is now `{got}`, the patch requires `{want}` ({a.path})" ) - have = _params(node) - missing = [p for p in a.params if p not in have] - if missing: - return ( - f"{a.symbol}{tuple(have)} no longer takes {', '.join(missing)}" - ) - - return None + problem = _signature_problem(node, a.symbol, a.params, a.forwards) + return ("broken", problem) if problem else ("ok", None) # ── sources ────────────────────────────────────────────────────────────────── +class Unreadable(Exception): + """The source could not be READ. That is not the same as it not existing. + + Folding these together is how a network blip becomes "iX deleted six files", + which becomes "both modules are broken", which becomes a bug report, a red + support matrix pushed to the README, and -- on a real box -- a module declining + to apply. A transient failure must never be able to say anything about + middleware. + """ + + def _fetch(ref: str, path: str) -> str | None: + """Source at `ref`, None if iX genuinely does not have that file (404). + + Raises Unreadable for anything else: rate limits (the matrix makes ~30 + unauthenticated requests per run and 429 is a real outcome), DNS, timeouts. + """ url = RAW.format(ref=ref, path=path) try: with urllib.request.urlopen(url, timeout=_TIMEOUT) as r: # noqa: S310 - if r.status != 200: + if r.status == 404: return None + if r.status != 200: + raise Unreadable(f"{url} -> HTTP {r.status}") return r.read().decode("utf-8", "replace") - except Exception: - return None + except urllib.error.HTTPError as e: + if e.code == 404: + return None # the file really is gone + raise Unreadable(f"{url} -> HTTP {e.code}") from e + except Unreadable: + raise + except Exception as e: + raise Unreadable(f"{url} -> {e!r}") from e def _read(root: str, path: str) -> str | None: + full = os.path.join(root, *path.split("/")) try: - with open(os.path.join(root, *path.split("/")), encoding="utf-8") as fh: + with open(full, encoding="utf-8") as fh: return fh.read() - except OSError: - return None + except FileNotFoundError: + return None # genuinely absent + except OSError as e: + raise Unreadable(f"{full} -> {e!r}") from e # permissions, I/O, ... + + +def _stock(text: str) -> str: + """Only the part of the file that iX wrote. + + Our own blocks are appended after the MARKER, and they quote the very strings + the probes look for -- B2_BLOCK literally writes `B2RcloneRemote.restic = True` + into b2.py, and CRUD_BLOCK quotes the "no further nesting" message it filters + on. Scanning the whole file on an already-patched box therefore finds OUR text + and concludes TrueNAS went native, i.e. "retire the module". apply.sh has always + cut at the marker for exactly this reason; compat.py did not, so the one command + its own docstring recommends for a live box (`--tree /usr/lib/.../middlewared`) + reported providers as native on every patched machine. + """ + return text.split("\n# TRUECLOUD_PATCH", 1)[0] def check(loader, modules=None) -> dict: - """Check every assumption. `loader(path) -> source|None`. + """Check every assumption. `loader(path) -> source|None`, may raise Unreadable. - Returns {module: {"ok": bool, "native": bool, "problems": [...]}}. + Returns {module: {"ok", "native", "unknown", "problems"}}. + + `unknown` means the sources could not be READ -- a rate limit, a timeout, an + unreadable tree. It is NOT `ok` and it is emphatically NOT `broken`: nothing may + act on a verdict derived from a failed download. """ modules = modules or [PROVIDERS, NESTED] cache = {} @@ -261,27 +414,51 @@ def check(loader, modules=None) -> dict: cache[path] = loader(path) return cache[path] - out = {m: {"ok": True, "native": False, "problems": []} for m in modules} + out = { + m: {"ok": True, "native": False, "unknown": False, "problems": []} + for m in modules + } for a in ASSUMPTIONS: if a.module not in out: continue - problem = check_source(a, src(a.path)) - if problem: + try: + text = src(a.path) + except Unreadable as e: + out[a.module]["unknown"] = True + out[a.module]["problems"].append({ + "id": a.id, "detail": f"could not read {a.path}: {e}", "why": a.why, + }) + continue + + status, detail = check_source(a, text if text is None else _stock(text)) + if status == "broken": out[a.module]["ok"] = False out[a.module]["problems"].append({ - "id": a.id, "detail": problem, "why": a.why, + "id": a.id, "detail": detail, "why": a.why, + }) + elif status == "unknown": + out[a.module]["unknown"] = True + out[a.module]["problems"].append({ + "id": a.id, "detail": detail, "why": a.why, }) for module, (path, phrase, native_when_present) in NATIVE_PROBES.items(): if module not in out: continue - text = src(path) + try: + text = src(path) + except Unreadable: + out[module]["unknown"] = True + continue if text is None: continue - present = _squash(phrase) in _squash(text) + present = _squash(phrase) in _squash(_stock(text)) out[module]["native"] = (present == native_when_present) + # `ok` is cleared ONLY by a definite violation, so "unknown" never needs to + # repair it -- and must not: a module with one unreadable file AND one proven + # broken assumption is broken, not unknown. return out @@ -419,9 +596,26 @@ def matrix(refs=None, remote: str = REPO) -> list[dict]: def _verdict(r: dict) -> str: + """BROKEN outranks native, which outranks unknown. + + "native" used to win outright, which meant a module that was BOTH broken and + apparently-native rendered as good news: green CI, no bug report, and a README + row telling users the feature went native while it was in fact broken. A proven + violation is the strongest signal here and must never be masked by a weaker one + -- and the native probe is only a substring match on iX's source, so it is + exactly the weaker one. + """ + if not r["ok"]: + return "BROKEN" if r["native"]: return "native" - return "ok" if r["ok"] else "BROKEN" + if r["unknown"]: + return "unknown" + return "ok" + + +def is_broken(r: dict) -> bool: + return not r["ok"] #: Versions a human has actually run a backup on, with real data, on real hardware. @@ -456,7 +650,22 @@ END = "" def update_readme(rows: list[dict], path: str = README) -> bool: - """Rewrite the README's matrix block. True if it changed.""" + """Rewrite the README's matrix block. True if it changed. + + Refuses if ANY row could not be fully checked. The published matrix is what a + stranger reads before trusting this with their backups, and CI pushes it + automatically -- so a rate limit or a DNS blip must never be able to repaint it. + A stale-but-true table beats a fresh-but-invented one. + """ + unknown = [ + r["ref"] for r in rows + if any(m["unknown"] for m in r["modules"].values()) + ] + if unknown: + raise Unreadable( + "not rewriting the matrix: could not fully check " + ", ".join(unknown) + ) + with open(path, encoding="utf-8") as fh: text = fh.read() @@ -494,6 +703,7 @@ def render_markdown(rows: list[dict]) -> str: "ok": "ok", "BROKEN": "**BROKEN**", "native": "native", + "unknown": "unknown", }[v]) version = ref.removeprefix("TS-") @@ -579,7 +789,7 @@ def main(argv): shipped_broken = [ r["ref"] for r in rows if not r["unreleased"] - and any(not m["ok"] and not m["native"] for m in r["modules"].values()) + and any(is_broken(m) for m in r["modules"].values()) ] if shipped_broken: print(f"\nBROKEN on shipped releases: {', '.join(shipped_broken)}", @@ -597,7 +807,7 @@ def main(argv): print(render(label, result)) # Exit 1 if any module is broken. "Native" is not broken -- it is good news. - return 1 if any(not r["ok"] and not r["native"] for r in result.values()) else 0 + return 1 if any(is_broken(r) for r in result.values()) else 0 if __name__ == "__main__": diff --git a/tools/release_gate.py b/tools/release_gate.py index 8970544..855ddaf 100644 --- a/tools/release_gate.py +++ b/tools/release_gate.py @@ -50,10 +50,18 @@ def _git(*args: str, cwd: str | None = None) -> str: def rc_tags(version: str, cwd: str | None = None) -> list[str]: - """Every rc tag for this version, oldest first (rc2 sorts after rc1).""" - want = normalise(base_version(version)) - out = _git("tag", "--list", f"v{want}-rc*", cwd=cwd) - tags = [t.strip() for t in out.splitlines() if t.strip()] + """Every rc tag for this version, oldest first (rc2 sorts after rc1). + + The glob is only a prefilter; the anchored regex decides. `v1.0.0-rc*` also + matches `v1.0.0-rc1-hotfix`, which _rc_number reads as 0 -- so a tag the + numbering logic does not understand could satisfy the barrier while never having + been a release candidate. + """ + want = re.escape(normalise(base_version(version))) + exact = re.compile(rf"^v{want}-rc\d+$") + + out = _git("tag", "--list", f"v{normalise(base_version(version))}-rc*", cwd=cwd) + tags = [t.strip() for t in out.splitlines() if exact.match(t.strip())] return sorted(tags, key=_rc_number) @@ -79,9 +87,15 @@ def commit_for(ref: str, cwd: str | None = None) -> str | None: def check_promotable(version: str, cwd: str | None = None) -> list[str]: """Every reason v may not be cut as a stable release. - Empty list means the barrier is satisfied. Does NOT talk to the network: CI - layers the "did that rc's run actually pass" check on top, because only CI can - see workflow results. + Empty list means the barrier is satisfied. + + The commit under test is the tag's if it exists, and HEAD otherwise. Both are + real: CI runs this AFTER the tag is pushed, and release.sh runs it BEFORE + creating the tag -- which is the whole point, since refusing after the tag + exists is too late to be a gate. Requiring the tag unconditionally made + `release.sh --promote` impossible: it dies if the tag already exists, and the + gate died if it did not, so the only way through was to hand-tag and bypass + every check this file exists to enforce. """ if is_prerelease(version): return [] # candidates are what the barrier exists to encourage @@ -91,7 +105,9 @@ def check_promotable(version: str, cwd: str | None = None) -> list[str]: target = commit_for(tag, cwd=cwd) if target is None: - return [f"{tag} does not exist"] + target = commit_for("HEAD", cwd=cwd) + if target is None: + return ["cannot resolve a commit to release (no HEAD?)"] candidates = rc_tags(want, cwd=cwd) if not candidates: