Repository navigation
Release guards for publish_release.py, plus the tests they were missing - #318
Merged
Merged
Conversation
`commit_for_version()` pins the tag to a commit whose tree declares the prepared version, which makes a tree that has drifted ahead harmless. It cannot help when NO commit declares that version yet -- the bump has not merged -- and there it warns and falls back to the branch head, whose version is then what labels the assets. wincompose shipped exactly that twice this month. PK-0.9.19 was published 50 minutes before its own bump merged, so its downloads are WinCompose-Setup-0.9.18.exe (real 0.9.19 builds, wrong number), the About tab reads 0.9.18, and the update file could not be bumped at all without offering every install an endless update to itself. PK-0.9.20 repeated it four days later. Nothing in either repo's pipeline refuses this, and the asset-version assertion added to wincompose's release workflow only turns it into a release with no downloads. So: when the pin finds nothing AND the branch head declares a different version AND publishing would CREATE the release, die instead. All three conditions matter. Re-applying notes to an existing release runs no build, so the tree's version is then irrelevant, and `release_exists()` is what tells the two apart -- a tag existing is a different question, since a deleted release, a hand-pushed tag or a build that died before `gh release create` each leave a tag with no release. Only a 404 counts as "no release"; anything else returns None and is treated as creating, the stricter reading, because an auth or network failure reading as success is how the check would go quiet. Deliberately NOT gated on the tree merely differing from the tag: every merge auto-bumps, so the tree is normally ahead of a prepared tag and the pin already makes that safe. Refusing there would block almost every legitimate publish in this repo. A shallow clone reaches the same branch for a different reason -- the search could not see the commit -- so the message names `git fetch origin --unshallow` first. Told to merge a bump that merged months ago, the obvious next move is --allow-version-mismatch, which publishes the release this exists to prevent. Also widens run()'s exec guard from three OSError subclasses to OSError, which subsumes the equivalent try/except wincompose had grown around get_token(); the hoisted token lookup keeps working with no `gh` installed because of it, verified with PATH stripped. The script is now identical in qmk_firmware, PolyKybdHost and wincompose again. It had diverged: this repo gained the pin and the make_latest correctness, wincompose gained the create-time guard, and each copy was the newer one for a different thing. Tested by driving all three outcomes against the real repo with the pin stubbed out (refuse / warn under --allow-version-mismatch / note on an existing release), plus a dry-run in each of the three repos and one with no `gh` on PATH. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015jui4uFYS3SXuhXSRgew9i
![revix-ai-review[bot]](https://avatars.githubusercontent.com/in/3743861?s=60&v=4){kind=small}
There was a problem hiding this comment.
- [FIX] Version mismatch gate: When no commit on the default branch declares the prepared version and the branch head declares a different one, refuses to create a new release unless
--allow-version-mismatchis supplied. The check does not block updates to an existing release, and pinned commits with a newer branch head remain allowed. - [API] Release existence check: Adds a GitHub API lookup that distinguishes an existing release from a missing release; API errors are treated as unknown and therefore as a new release for the gate. Shallow clones include guidance to unshallow before retrying.
- [CHORE] Command error handling: Broadens
run()to catchOSErrorwhen a command cannot be run. - This PR isn't getting a full Revix review right now — reviews are free for open-source projects but limited to a few a day, and this organization has already used today's. Reviews resume on Oct 08 at 00:00 UTC.
Free open-source reviews are in cooldown — Revix reviews open-source pull requests for free every day, and the summary above is still on us. Want every PR reviewed? https://www.revix.ai
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
Contributor
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
urllib's default is the socket default, which is None -- block forever. So a connection GitHub accepts and then stops answering hangs the script rather than failing. That defeats release_exists()'s whole design. It is written to return None on any trouble so the caller takes the stricter path and treats the release as not existing; a hang reaches neither. It also hangs --dry-run, the one command that has to stay quick and safe, since the lookup happens before anything is printed. api() had the same defect, from before this change, and is fixed in the same pass: same one-line remedy, same file, and a partial fix would mean a second three-repo sweep later to keep the copies byte-identical. Its exposure is a real publish rather than a dry-run, where a person is at least watching. One module constant rather than a literal per call site, so the next urlopen cannot quietly omit it. Verified against a socket that accepts and never answers: release_exists() returns None after the configured timeout instead of blocking, and the TimeoutError lands in the existing handler. The same server with no timeout never returns. The three dry-runs still resolve their pinned commits, and the live lookup still answers True for an existing release and False for a tag without one. Found by Greptile on PolyKybdHost#318. Applied to all three copies. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015jui4uFYS3SXuhXSRgew9i
All three found by Greptile, all three verified against the live repo. An EXISTING tag beats the pin, and the pin's own report hid it. `target_commitish` is documented as "Unused if the Git tag already exists" -- creating a release never moves a tag -- so when the tag is already there the build comes from wherever it points, and the commit the script prints is only a claim. PK-0.9.20 is the live case: its tag sits on the pre-bump 6cb987cb1a, its release was deleted, and the pin resolves 113093cfa2, so the script would have printed the right commit and published the wrong one. Creating is now refused when an existing tag points elsewhere.⚠️ This fires even when the version check passes, which is the point: there the pin found the correct commit and the mismatch guard sees nothing wrong. The Latest badge was decided from PREPARED NOTES rather than published releases. Staging notes for a version that has not shipped then withheld the badge from the release being published: `releases/latest`, which the download links resolve, stayed on an older release while CI raised `status.txt` to the new version -- every install offered the new one and handed the old one, which is the 0.9.18 failure again. It now compares against what GitHub calls latest. The original concern is preserved and tested: publishing an older tag with --tag still does not steal the badge. `--allow-version-mismatch` promised one consequence and the repos differ. The text said the workflow refuses the assets, which is true only of wincompose, whose release.yml asserts the built exe's version against the tag. The firmware workflow NAMES its assets from the tag (`VER="${TAG#PolyKybd-fw-v}"`, release.yml:202) and checks nothing, so the override there ships a .bin whose filename and reported FW_VERSION disagree -- the mis-flash class that sent a firmware bisect to a wrong conclusion for several rounds (#258). The flag's --help and the warning now state both outcomes instead of one. Verified: the guard refuses the real PK-0.9.20 publish and stays silent on PK-0.9.19, whose release exists so nothing would be built; the badge decision was exercised both ways against the live `releases/latest`, including across the 0.9.x/0.10.x boundary; the three dry-runs still resolve their pinned commits; and the no-`gh` dry-run still works. The wincompose default dry-run now exits 1, which is the new guard firing on the real state of that tag rather than a regression. Firmware and host release workflows still have no assertion that the built version matches the tag. That is a workflow change, not a script one, so it is not in this scripts-only PR. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015jui4uFYS3SXuhXSRgew9i
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
CodeQL flagged `latest_why` as assigned and never used. It was the visible
symptom of two defects I introduced in the previous commit.
The reason was built and dropped, so the fallback path -- where the
published-releases lookup fails and the decision comes from the prepared
notes instead -- was silent. I had claimed in review that it explains
itself either way; it did not.
Worse, and not what CodeQL named: the line that reports a withheld badge
still referenced `newest_tag`, which now exists only inside that fallback
branch. So on the normal path a not-latest publish died with
UnboundLocalError -- on exactly the path `--tag` exists for:
publish_release.py --dry-run --tag PK-0.9.17
UnboundLocalError: cannot access local variable 'newest_tag'
The badge line now prints in both directions and carries the reason, which
is the thing worth seeing: it says whether the decision came from the
published releases or from the fallback.
⚠️ My test for the previous commit drove the predicate directly and never
ran main(), which is why a crash in the printing right beside it survived
a table of five passing cases. PolyKybdHost's own CLAUDE.md has the rule --
run the real entry point once before believing a mocked suite. Every
prepared tag now goes through main() in all three repos: 5 in wincompose
(4 pass, PK-0.9.20 refused by the tag guard, as it should be) and the
newest 4 in each sibling, all exiting 0 with the badge line reading NO for
older tags and YES for the newest.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015jui4uFYS3SXuhXSRgew9i
`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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015jui4uFYS3SXuhXSRgew9i
Four defects in `publish_release.py` were found in review today and none by a test, because there was no test for it in any of the three repos. Three were the same shape: a FAILED GitHub lookup read as a usable answer, so a guard passed instead of refusing. `release_exists()` folded 401/5xx/DNS into "no release"; `tag_commit()` folded them into "no tag", and its caller SKIPS its check when there is no tag, so a 5xx let a publish through at whatever commit the tag really pointed at. The contract these guards rest on is three-valued and the values are not interchangeable: a sha means "this is so", "" means "confirmed absent", None means "could not tell" and must never be read as either. That is what the tests pin, including the annotated-tag dereference and a ref shape the code does not recognise, plus `main()` refusing rather than publishing when the tag cannot be checked. Also pins the finite urlopen timeout two ways: that each call site passes it, and -- by reading the source -- that no `urlopen(req)` without one has crept back. A new call site is the realistic regression there. Mutation-swept before being trusted, per CLAUDE.md: seven deliberate breaks, each caught, each by its own intended test rather than by collateral damage. Among them the original bug restored verbatim, the 404 branch collapsed into None, the refusal deleted, and the wrong-commit comparison neutered. Green again after restoring. Lives here because PolyKybdHost is the only one of the three repos with a Python test runner; the script is kept byte-identical across all three, so a break in it fails here wherever it was introduced.⚠️ The full suite could not be run in this container (no .venv, so numpy and PyQt5 are absent and a run would silently skip whole modules). `scripts/run_tests.py -s tests/scripts` passes 16 tests, and discovery from `tests/` picks all 16 up. Prompted by Greptile on wincompose#28, which marked it non-blocking. It is not: the collapse it warns about is a mistake already made twice in this file. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015jui4uFYS3SXuhXSRgew9i
`_with_urlopen()` and the timeout test assigned
`urllib.request.urlopen` and never put it back. Loading the script fresh
per test gives a new MODULE object but not a new `urllib.request` -- that
comes from `sys.modules` and is shared with everything else -- and the
supported runner runs the whole suite in one process. So every test that
ran after these inherited a fake that raises OSError("network down").
Reproduced before fixing: after loading this module's 16 tests,
`urllib.request.urlopen is not pristine`, and a call to it raises exactly
that OSError. Both now go through `mock.patch.object` with
`addCleanup(patcher.stop)`, matching the idiom in settings_test.py.
Verified both directions: the probe that caught it reports urlopen
restored, and a real local HTTP server is reachable after the suite runs
(HTTP 204), which is the scenario that would have broken.
Re-ran the mutation sweep after the rewrite, since changing a test file
invalidates the earlier proof: all seven breaks still caught, each by its
own intended test, green again after restoring.
⚠️ The note at the top of the file now says why a fake here must be
patched rather than assigned -- the trap is that a per-test module object
makes it LOOK isolated.
Found by Greptile on #318. Worth recording that this was a bug in the
test written to stop bugs going unnoticed, and that it would have
presented as unrelated network tests failing somewhere else entirely.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015jui4uFYS3SXuhXSRgew9i
`.claude/rules/tests.md` says it outright: `import unittest` beside `from unittest import mock` is one of five CodeQL Python rules new code here keeps tripping, and the fix is `import unittest.mock as mock`. The full list is in docs/testing.md under "No *test* CI", from #301 -- ten alerts in one scan. I had declined CodeQL's alert on this, arguing from the 32 other test files that use the flagged form. Those are drift; the rule is the authority, and it was in the repo the whole time.⚠️ Why the rule never reached me is itself the documented trap: a path-scoped rule fires on the READ tool, not on `cat`/`grep` through Bash, and I inspected the suite's conventions entirely through Bash. The CLAUDE.md note warning about exactly this is the one I walked past. Reading the rule properly also surfaced a guard I did not know existed: `tests/discovery_test.py` checks repo-wide, by AST, that no test class defines a method name twice -- the shadowed-duplicate trap a scripted edit causes, which a green suite cannot see. It passes on this file, as does the file run on its own, which the same rule asks for. Re-ran both proofs after the edit: the urlopen leak probe reports restored with 16 tests green, and the mutation sweep still catches all seven breaks, each by its own intended test. Found by Greptile on #318, citing the rule file -- a claim I verified before acting, and which turned out to be exactly right. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015jui4uFYS3SXuhXSRgew9i
The CLAUDE.md note added with the script changes says "nothing checks that it stays byte-identical" -- but the check-mirrored-artifacts skill is exactly what checks these, and `publish_release.py` was not in its table. So the note was true only because the enforcement was missing. It gets a row, and a⚠️ paragraph on what made this drift unusual: it went BOTH ways at once, so no single-sided comparison would have caught it. qmk + host held `commit_for_version()` and the `make_latest` correctness; wincompose held the create-time version gate. Each copy was therefore the newer one, and the missing half in wincompose is the mechanism behind two mislabelled releases.⚠️ Two things the skill needed spelling out: it is THREE copies, and wincompose is the one a `for` loop over qmk/host silently misses; and the tests live in host `tests/scripts/publish_release_test.py` only, the one repo with a Python runner, so that is where a break in any copy surfaces. Each repo's Releases section gains the same pointer, since a session working in qmk or host had no reason to know the file is shared -- which is how it drifted. The skill is one of the nine mirrored ones: edited in qmk, copied to host, and all nine verified identical with `cmp`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015jui4uFYS3SXuhXSRgew9i
thpoll83
changed the title
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Hardens
scripts/publish_release.pyso a publish cannot ship a build that is not the version on its label, and adds the first test coverage this script has had in any of the three repos.The script is kept byte-identical across
qmk_firmware,PolyKybdHostandwincompose; the same script change is on qmk_firmware#356 and wincompose#28. The tests live here only, since this is the one repo with a Python runner.Why
wincompose shipped two mislabelled releases this month.
PK-0.9.19was published 50 minutes before its version bump merged, so the tag landed on a tree still declaring the previous version: real 0.9.19 builds in files named0.9.18, About tab reading0.9.18, and an update file that could not be bumped at all without offering every install an endless update to itself.PK-0.9.20repeated it four days later. Nothing in any repo's pipeline refused either one.The guards
commit_for_version()pins the tag to a commit whose tree declares it; when nothing does, the bump has not merged and there is nothing to pin to. Only on create, since re-applying notes to an existing release runs no build.target_commitishis "Unused if the Git tag already exists", so publishing never moves a tag: the build comes from where the tag points while the script reports the commit it wanted.releases/latest— which the download links resolve — on an older release while CI raised the update file to the new version.None: a stalled connection hung the script instead of reaching its own error path.release_exists()andtag_commit()both return three values — a result, "confirmed absent", "cannot tell" — because folding failures into "absent" is what let a guard pass instead of refuse. This bit twice in this file.--allow-version-mismatchoverrides the first guard; its help now states both outcomes, since what overriding costs differs per repo (wincompose's workflow refuses mislabelled assets; the firmware's names them from the tag and checks nothing).The tests —
tests/scripts/publish_release_test.py, 16 testsFour defects here were found in review today and none by a test, because none existed. What they pin: the three-valued contract on both lookups (404 vs every other status vs transport failure), the annotated-tag dereference and an unrecognised ref shape,
main()refusing rather than publishing when the tag cannot be checked, and the timeout — asserted at each call site and by reading the source, so a new bareurlopen(req)fails.Mutation-swept before being trusted, per
CLAUDE.md: seven deliberate breaks, each caught, each by its own intended test rather than collateral damage — including the original bug restored verbatim, the 404 branch collapsed, the refusal deleted, and the wrong-commit comparison neutered. Green again after restoring.publish_release.pyis now registered in thecheck-mirrored-artifactsskill, which is what should have caught the drift; the note inCLAUDE.mdpointed at an enforcement that did not exist.Version bump label
None, so the default patch bump. Release tooling and its tests, not a feature.
Testing
scripts/run_tests.py -s tests/scripts; discovery fromtests/picks all 16 up.tests/discovery_test.py(the repo's AST guard against a test class defining a method name twice) passes on the new file.main()in all three repos — herev1.15.0resolvesa5dac6a5af (declares 1.15.0)and notes the drift to 1.15.4, i.e. the legitimate case stays legitimate.Noneafter the timeout instead of hanging.--dry-runwithPATHstripped ofgh, since the token lookup moved earlier inmain()..venvin this container, so numpy and PyQt5 are absent and a run would silently skip whole modules while still printing a pass. Worth a localscripts/run_tests.pybefore merging.🤖 Generated with Claude Code
https://claude.ai/code/session_015jui4uFYS3SXuhXSRgew9i
The PR appears safe to merge; no blocking issue remains from the reviewed findings.
Summary
The PR adds release checks, finite GitHub request timeouts, and tests for failed lookups and release refusal.
Reviews (8) · Last reviewed commit: "Register publish_release.py as a mirrore..." · Reviewed by Greptile