From 34ea70c239811c745883caf3b49a36bbcf6d98f7 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 7 Oct 2026 14:02:03 +0000 Subject: [PATCH 1/3] A failed tag lookup must refuse, not skip the guard `tag_commit()` returned None both for a confirmed 404 and for any failure, and the caller SKIPS its check when there is no tag -- so a 5xx, a 403 or a timeout read as "no tag" and let a publish through at whatever commit the tag really points at, which is the thing the guard exists to prevent. That is the same shape as the failed-lookup bug already fixed once in this script, in release_exists(), where only a 404 counts as an answer. I reproduced it in the helper written to fix the related bug, one function away from the comment warning about it. Three outcomes now, matching published_latest_version(): a sha, "" for a confirmed 404, None for "cannot tell". The annotated-tag dereference and any ref shape the code does not understand also return None rather than guessing. The caller refuses when it cannot check, naming that an existing tag is never moved by publishing, so there is nothing safe to assume. The sha comparison is exact now (`at != target`) instead of a `startswith` on the first ten characters; both sides are full 40-char shas, one from `git log --format=%H` and one from the API. Tested through main() with the lookup stubbed to each outcome: 5xx refuses, 403 refuses, a 404 correctly skips the guard and proceeds, and a tag genuinely at the wrong commit refuses naming both shas. Plus the regression run -- the newest four prepared tags through main() in each of the three repos, all exiting 0 with the right Latest verdict. Found by Greptile on PolyKybdHost#318 and qmk_firmware#356. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015jui4uFYS3SXuhXSRgew9i --- scripts/publish_release.py | 26 +++++++++++++++++++++----- 1 file changed, 21 insertions(+), 5 deletions(-) diff --git a/scripts/publish_release.py b/scripts/publish_release.py index b660be7c..fbe7bdeb 100644 --- a/scripts/publish_release.py +++ b/scripts/publish_release.py @@ -222,8 +222,15 @@ def _get_json(owner, repo, path, token): def tag_commit(owner, repo, tag, token): - """The commit an EXISTING tag points at; None if there is no such tag or the - lookup failed. + """Where an existing tag points: a sha, "" for a confirmed 404 (no such + tag), or None when it cannot be determined. + + ⚠️ The three are NOT interchangeable, and collapsing them is how this check + goes quiet: the caller SKIPS its guard when there is no tag, so a 5xx or a + timeout reading as "no tag" lets a publish through at whatever commit the + tag really points at. That is the same shape as the failed-lookup bug + already fixed once in release_exists() -- only a 404 is an answer. + (Found by Greptile on PolyKybdHost#318 and qmk_firmware#356.) ⚠️ Needed because `target_commitish` is documented as "Unused if the Git tag already exists" -- creating a release does not move a tag. So when the tag is @@ -235,15 +242,19 @@ def tag_commit(owner, repo, tag, token): dereferenced one step. """ st, ref = _get_json(owner, repo, f"/git/ref/tags/{tag}", token) + if st == 404: + return "" if st != 200 or not ref: return None obj = ref.get("object") or {} if obj.get("type") == "commit": - return obj.get("sha") + return obj.get("sha") or None if obj.get("type") == "tag": st, tg = _get_json(owner, repo, f"/git/tags/{obj.get('sha')}", token) if st == 200 and tg: - return (tg.get("object") or {}).get("sha") + return (tg.get("object") or {}).get("sha") or None + return None + # A ref shape this does not understand is not an answer either. return None @@ -486,7 +497,12 @@ def main(): # guard sees nothing wrong. (Found by Greptile on wincompose#27.) if creating: at = tag_commit(owner, repo, tag, token) - if at and target != default_branch and not at.startswith(target[:10]): + if at is None: + die(f"cannot tell whether the tag {tag} already exists or where it points, so\n" + f" whether the build would come from {target[:10]} is unknown -- and an\n" + f" existing tag is never moved by publishing. Refusing rather than guessing.\n" + f" Retry when the GitHub API is reachable." + unknown_note) + if at and target != default_branch and at != target: die(f"the tag {tag} already exists, at {at[:10]}, and creating a release does\n" f" not move it -- so the build would come from {at[:10]}, not from the\n" f" {target[:10]} this would report. Either dispatch the release workflow on a\n" From e1b086d8a9dcf2d001dbaf70bf6c988d7cae85d4 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 7 Oct 2026 14:37:10 +0000 Subject: [PATCH 2/3] Document the existing-tag rule and the tag-move recovery MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both came out of PK-0.9.20 after #27's docs were written, so the files lagged the code again -- the failure mode this whole change exists to fix. `target_commitish` is documented as "Unused if the Git tag already exists", so publishing never moves a tag: the build comes from wherever the tag points while the script reports the commit it wanted. Reached by an ordinary sequence -- publish early, delete the release, merge the bump, try again -- and PK-0.9.20 sat exactly there, tag on 6cb987cb1a declaring 0.9.19 with the pin resolving 113093cfa2. ⚠️ The note says the check must fire even when the version gate passes, which is the part that is easy to get wrong: there the pin found a correct commit and the mismatch gate sees nothing wrong. RELEASE.md gains its own section for it, because the recovery it already described was the WORSE of the two. A dispatch attaches correct assets to the tag where it is, leaving `git checkout ` on a tree declaring the previous version -- the kind of thing that sends a bisect wrong. Moving the tag and publishing normally fixes the cause, and is what actually resolved 0.9.20; the commands are there, with the caveat to check no release exists on the tag before force-pushing. Also corrects the window in ④ from "the window in between" to ~4 hours, now that it has an end. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015jui4uFYS3SXuhXSRgew9i --- CLAUDE.md | 14 ++++++++++++++ RELEASE.md | 34 +++++++++++++++++++++++++++++++++- 2 files changed, 47 insertions(+), 1 deletion(-) diff --git a/CLAUDE.md b/CLAUDE.md index 072cc142..e3075255 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -352,6 +352,20 @@ before attaching anything, because the script is only one of three ways a release starts (a hand-pushed tag and a `workflow_dispatch` recovery do not go through it). +⚠️ **An EXISTING tag beats the pin, and the pin's own report hides it.** +`target_commitish` is documented as "Unused if the Git tag already exists" — a +release never moves a tag — so the build comes from wherever the tag points while +the script reports the commit it *wanted*. Reached by an ordinary sequence: +publish early, delete the release, merge the bump, try again; the first attempt's +tag is still on the pre-bump commit. **PK-0.9.20 sat exactly there** (tag on +`6cb987cb1a` declaring 0.9.19, pin resolving `113093cfa2`), so the script would +have printed the right commit and published the wrong one. It refuses now, and +⚠️ **that check must fire even when the version gate passes** — there the pin +found a correct commit and the mismatch gate sees nothing wrong. The fix is to +**move the tag** and publish normally, not to dispatch: a dispatch attaches +correct assets while leaving `git checkout ` on a tree that declares the +previous version. `RELEASE.md` → *If the tag is in the wrong place*. + ⚠️ **Do NOT make the gate fire on the tree merely differing from the tag.** The firmware and host tree is normally *ahead* of a prepared tag — every merge auto-bumps — and the pin already makes that safe, so refusing there blocks diff --git a/RELEASE.md b/RELEASE.md index d129a4f8..b2f8f184 100644 --- a/RELEASE.md +++ b/RELEASE.md @@ -95,7 +95,7 @@ exists *before* it bumps, and nothing looks again afterwards. So a release delet after the job has run leaves `Latest` naming a version with no release — the early-bump failure below, arriving by another route. Revert `status.txt` in the same breath as the deletion, or re-create the release (see *If the release build -fails*). 0.9.20 was deleted and re-created on 7 October, and for the window in +fails*). 0.9.20 was deleted and re-created on 7 October, and for the ~4 hours in between every install below 0.9.20 was offered the 0.9.19 download. It was a manual step until 0.9.20, and it went wrong in both directions. Early, @@ -106,6 +106,38 @@ the version they already have and read it as the updater being broken (0.9.18, a happened and nothing about the release itself looks wrong (0.9.19, two weeks, reported from outside as "no notification"). +If the tag is in the wrong place +-------------------------------- + +⚠️ **Publishing never MOVES a tag.** `target_commitish` is documented as "Unused +if the Git tag already exists", so when the tag is already there the build comes +from wherever it points and the commit the script pins is only a claim. +`publish_release.py` refuses to create a release in that case, naming both +commits. + +That is reachable from an ordinary sequence: publish too early, delete the +release, merge the bump, try again. The tag from the first attempt is still on +the pre-bump commit. 0.9.20 did exactly this. + +Two ways out, and the first is better: + +1. **Move the tag to the commit that declares the version**, then publish + normally. The release then builds from a tree that reports the right version, + and `git checkout ` gives you the source the release was made from: + + git fetch origin main + git tag -f PK- + git push --force origin refs/tags/PK- + python scripts/publish_release.py + + `--dry-run` prints the commit to use — it is the `target:` line, and the + script's refusal names it too. Safe as long as no release exists on that tag + yet; check before force-pushing. +2. **Dispatch the workflow** (below) to attach correct assets to the tag where it + is. Quicker, but the tag keeps pointing at a tree declaring the *previous* + version, so a later `git checkout ` misleads — which is the kind of thing + that sends a bisect wrong. + If the release build fails -------------------------- From e17ca1ec633ef0541a0ca794d70e861cb72101b0 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 7 Oct 2026 14:42:10 +0000 Subject: [PATCH 3/3] Point the tag-move recovery at the refusal, not a line that never prints The instruction said `--dry-run` prints the commit to use on its `target:` line. It does not, in the one case the section is about: the tag check dies before the summary is printed, so a dry-run on a misplaced tag shows the refusal and nothing else. Reproduced with the lookups stubbed -- no `target:` line in the output, and the refusal naming both commits -- which is also exactly what the maintainer's own paste of the PK-0.9.20 refusal showed earlier. I wrote the sentence with that evidence already on screen. Describes the ordering rather than citing line numbers, which is the stale-prose hazard this change exists to reduce. Found by Greptile on #28. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015jui4uFYS3SXuhXSRgew9i --- RELEASE.md | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/RELEASE.md b/RELEASE.md index b2f8f184..bfc75ad9 100644 --- a/RELEASE.md +++ b/RELEASE.md @@ -130,9 +130,11 @@ Two ways out, and the first is better: git push --force origin refs/tags/PK- python scripts/publish_release.py - `--dry-run` prints the commit to use — it is the `target:` line, and the - script's refusal names it too. Safe as long as no release exists on that tag - yet; check before force-pushing. + ⚠️ **Take the commit from the refusal, which names both** — where the tag is + and where it should be. That is the only place to read them: the tag check + runs before the script prints its summary, so a `--dry-run` on a misplaced + tag never reaches the `target:` line. Safe as long as no release exists on + that tag yet; check before force-pushing. 2. **Dispatch the workflow** (below) to attach correct assets to the tag where it is. Quicker, but the tag keeps pointing at a tree declaring the *previous* version, so a later `git checkout ` misleads — which is the kind of thing