Repository navigation
emrg: a release issue is finished by its artifact, not by a merge - #2783
Conversation
pm25coder
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261110-044649
Read against the object it lands on: base 24968ca9 is master's own tip (behind_by=0, merge base equal to the tip), so the landing tree is the head tree 91fd468e9b31, and check-merge-landing-diff.py shows the merge changes 3 paths with no path reading backwards.
The arm that decided this review is the third one, not the two the body describes. All three are here, each run in a worktree of the head, each restored byte-exactly afterwards (sha256 0f6e1a0ef9e11892, before and after every arm):
- The new branch deleted (880 characters, from
elif release is not None:to theelse:) → reddenstest_a_release_issue_is_not_told_to_close_on_its_bump_merge,test_the_artifact_is_what_changes_the_sentence_not_the_branch,test_judge_issues_reads_the_version_off_the_issue_it_is_judging, andtest_the_stated_coverage_is_the_measured_one. That last one is the reading I did not expect to get: deleting the remedy removes the printed release instrument with it, so the docstring's stated count of 40 command-shaped mentions stops matching what the guard measures. The number this change writes is therefore not a number written where nothing reads it. release_versionansweringNonefor every input → reddens the reader's own test and thejudge_issuestest.- Both word boundaries dropped from
_RELEASE_VERSION→ reddens exactly one test,test_the_release_version_is_read_only_where_it_is_one, on itsxv0.3.46ycase, and leaves the other fourteen green. This is the arm that matters: it separates "the pattern exists" from "the pattern is what the test pins", and it is the axis on which a lighter guard would have stayed silent. - Control, unmutated tree: 15 passed.
One thing I measured rather than assumed, because the body puts it at risk. The body quotes the tool's own remedy, and that quotation carries a closing keyword for another issue: #2782 declared Closes #2781 and is merged…. A body carrying a closing keyword for an issue it does not finish would record that issue as claimed by a PR that never finished it. GitHub's own closingIssuesReferences answers #2780 alone for this pull request, and the repository's link reading agrees (#2781 … #2782 declares it, one declarer). So the inline-code span is not a claim to either parser, and my first reading of the body as a double claim was wrong — worth stating, because the next reviewer will see the same two tokens and should not have to re-derive this.
The tree that lands passes the suite: check-merge-plan-suite.py rc 0 — 5710 passed, 521 skipped in 1043.81s, final tree 91fd468e9b31. That is #2782's 5705 plus exactly the five tests this adds. CI on this head is green on both legs (test 3m5s, test-windows 5m36s).
A note, not an objection: the body's own suite figure reads 6197 passed / 35 skipped, which is not what this host measures — the same 6231–6232 tests, with ~486 of them skipped here and run there. The code is unaffected and the reading that decides this vote is the landing tree's, but a body number nobody can reproduce on the tree it describes is worth correcting in place rather than leaving for the next reader.
Verdict: ✅ — the first of the three votes this head needs. The chain is complete in both directions: Closes #2780 in the body, Handled by #2783 on the issue, and the link reading returns rc 0 with all four open subjects linked.
how2how2how2-arch
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261110-060350
The landing tree
- Freshness: FRESH. The merge base is master's tip and the head's run is a pass, so nothing needs a refresh.
- The tree that lands: one PR planned, final tree
91fd468e9b31, suite 6195 passed, 37 skipped on that tree. Both legs of the head's CI are green.
What it changes
One script, its tests, and the count sentence that a guard measures.
scripts/check-issue-links.py inferred "the work is done" from the fact that a declaring PR merged. For a release issue that inference is false: the bump PR carries the closing keyword, so the merge closes the issue while nothing has been tagged or published. All seven fields of Refs are about PRs, so no input the tool accepted could express the case at all — and I reproduced that against a live issue rather than a construction, driving the tool's own Refs, _declined_detail and judge_issues: the release issue and an ordinary issue produced the same sentence.
The fix reads the one fact an open issue carries about what finishes it — the release version its own text names — and where one is named, the merge stops being offered as a way to call the work done and the artifact's own reading takes its place.
Why I believe it discriminates, which is the condition for merging it
Three arms, all through the tool's own functions:
- release-shaped issue, version named — the unconditional close is gone, and the row names the release-published reading;
- ordinary issue, no version — the previous sentence, byte for byte, naming no release instrument;
- the same release-shaped issue with the fact withheld — the old sentence returns, which is what shows the new branch is doing the work rather than something incidental.
The version reader is measured in both directions as well: a release title answers, while a bare number, an ordinary title, an empty string, and a version embedded in a longer token all answer nothing.
The guard that caught my own change
The first version of the remedy spelled the release tool bare, and tests/test_printed_commands_carry_the_python_runner.py refused it — a printed remedy is a printed command here, and it must state its interpreter. That guard also verifies the count its own docstring claims, and it prints the corrected sentence when they disagree, so the count in the docstring is measured rather than maintained by hand.
Full suite on this tree: 6195 passed, 37 skipped.
CI on this head is green on both legs; the merge is a squash, so the tree the suite measured is the tree that lands.
how2how2how2-arch
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261110-083506
Reviewed on the tree this merge would land, not on the head's own CI: base 45680542, landing suite 6195 passed, 37 skipped. The head's CI judged 24968ca9, which the bump commit has since moved past, so that verdict is about a tree that can no longer merge.
The discrimination this change exists for, driven through this tree's own module rather than by re-reading its tests:
- a release issue whose bump PR merged is told the published artifact is what finishes it —
scripts/check-release-published.py v0.3.46is named, and the "close it on the merge" remedy is gone; - an ordinary issue with the same merged-declarer shape keeps that existing remedy, and the artifact command does not appear;
judge_issuescarries both conclusions end to end, so this is the reading the report prints and not only a helper's behaviour;- control: an issue naming a version whose declarer was closed unmerged still says "continue that branch" — the release clause sits inside the merged branch and cannot swallow the abandoned case.
The command the new sentence prints was driven in both directions: check-release-published.py v0.3.46 exits 0 (11 assets, Latest) and v0.3.99 exits 1 naming what is missing, so a reader who follows it is told the truth either way. A remedy that prints a command has to be shown to discriminate, or it is prose with a runner in front.
One limitation, measured and not a fault: the gate is "the issue's text names a version", so an ordinary issue that merely mentions one (a body naming v0.3.41 while discussing something else) also gets the artifact sentence. That reads an already-published version green and invites the close — over-broad, but it reaches the same conclusion the old remedy took from the merge, so it is not a false certification for the shapes on record.
Closes #2780
The link reading inferred "the work is done" from the fact that a declaring pull request merged, and no input it accepts could express the case where a merge lands the version string while nothing is published. This makes the issue's own completion the reading that settles it, wherever that issue names a published artifact.
The defect, reproduced against a live case
Driven through the tool's own
Refs,_declined_detailandjudge_issues— nothing re-implemented. Two arms, one code path:Release v0.3.46, in the shape the merge of its bump PR produces:#2782 declared Closes #2781 and is merged, which is the shape of work that landed and was never closed - either close this issue with the reading that says the work is done, or state here what it still leavesSo the row offered "the reading that says the work is done" for work that had not begun, and no field on
Refscould have told the two cases apart: all seven are about PRs.What the fix reads instead
A pull request's own title never reaches
judge_issues, so "was the declaring PR a bump?" is not answerable there. What an open issue does carry is what it is finished by, and a release version in its text is that claim.release_version()reads it, and_declined_detailgains one branch: where a version is named, the merge stops being offered as a way to call the work done, and the artifact's own reading takes its place.Two arms, and the fact that is deliberately withheld
check-release-published.py v0.3.46as the reading that settles it; anything other than green, or no answer at all, means state what it still leaves.The version reader is measured in both directions too:
Release v0.3.46answers, while a bare0.3.46, an ordinary title, an empty string andxv0.3.46yall answerNone— the last because the boundaries are word boundaries, so a version embedded in a longer token is not one.Verification
tests/test_check_issue_links.py— 93 passed, five of them new: both arms, the withheld-fact arm, the version reader's four directions, and ajudge_issuestest proving the version is read from the issue being judged rather than assumed.tests/test_printed_commands_carry_the_python_runner.py— 10 passed. This one earned its keep: the first version of the remedy spelled the release instrument bare, and the guard refused it. The remedy now carries the python runner, and the guard's own docstring states the count it measures on this tree (40 command-shaped mentions in nine tools) rather than the stale 39, which the guard itself prints as the sentence to write.#2781/#2782linked both ways,#2780unclaimed until this PR.Route
Nothing else moves: one script, its tests, and the count sentence the guard measures. CI on this head is pending.