Repository navigation
A failed tag lookup must refuse, not skip the guard - #28
Merged
Merged
Conversation
`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
![revixappdev[bot]](https://avatars.githubusercontent.com/in/3743861?s=60&v=4){kind=small}
There was a problem hiding this comment.
- [FIX] Tag lookup guard:
tag_commit()now distinguishes a confirmed 404 ("") from an indeterminate lookup (None); release publishing refuses when it cannot determine whether a tag exists, and compares full commit SHAs when checking an existing tag. - 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
|
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}
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 6cb987c declaring 0.9.19 with the pin resolving 113093c.⚠️ 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 <tag>` 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015jui4uFYS3SXuhXSRgew9i
This was referenced Oct 7, 2026
Merged
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015jui4uFYS3SXuhXSRgew9i
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.
Follow-up to #27, which merged. New PR because that branch is done; restarted from
mainatc3a001e.The script
tag_commit()— added in #27 to stop an existing tag defeating the commit pin — returnedNoneboth for a confirmed 404 and for any failure. 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. That is the exact case #27 existed to prevent, and the same shape as a bug already fixed once in this file inrelease_exists(), where only a 404 counts as an answer.Three outcomes now: a sha,
""for a confirmed 404,Nonefor "cannot tell". The annotated-tag dereference and any unrecognised ref shape also returnNonerather than guessing, and the caller refuses when it cannot check. The sha comparison is exact (at != target) rather thanstartswithon ten characters — both sides are full 40-char shas.Byte-identical to qmk_firmware#356 and PolyKybdHost#318, where 16 mutation-swept tests now cover this file.
The docs
Both of these landed after #27's documentation was written, so the files lagged the code again — the failure mode this whole change exists to fix.
CLAUDE.mdgains the existing-tag rule:target_commitishis "Unused if the Git tag already exists", so publishing never moves a tag and the pin's report becomes a claim. PK-0.9.20 sat exactly there — tag on6cb987cb1adeclaring 0.9.19, pin resolving113093cfa2— so the script would have printed the right commit and published the wrong one.RELEASE.mdgains 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, leavinggit checkout <tag>on a tree declaring the previous version. Moving the tag and publishing normally fixes the cause, and is what actually resolved 0.9.20 — commands included, with the caveat to check no release exists on the tag before force-pushing.PK-0.9.20
Released at 14:00 UTC with the tag moved to
113093cfa2.Assert the built version matches the tag: success— its first pass on a real publish — assets named0.9.20,status.txtagreeing,releases/latestcorrect. The guard added in #27 is what caught the mislabelled tag target before the publish, so it paid for itself the same day.Review
Per
CLAUDE.md, a green board here means it compiles, and there is nothing to compile in this diff. CodeRabbit can be asked with@coderabbitai review.🤖 Generated with Claude Code
https://claude.ai/code/session_015jui4uFYS3SXuhXSRgew9i
Summary by Sourcery
Refuse releases when an existing tag cannot be verified or points to a different commit, while documenting safe recovery procedures.
Bug Fixes:
Enhancements:
Documentation:
The PR appears safe to merge; the previous documentation finding is fixed.
Summary
The PR makes
tag_commit()distinguish a missing tag from a failed lookup. Publishing refuses when the tag cannot be checked and compares full commit SHAs.target:line.Reviews (3) · Last reviewed commit: "Point the tag-move recovery at the refus..." · Reviewed by Greptile