pr-claim: fail when the body and the commits disagree about closing - #299
Merged
Conversation
…298) `closingIssuesReferences` is GitHub's own resolution of the PR BODY's closing keywords, and this file's header rightly calls that better than a regex: a keyword the check accepts is exactly a keyword that will close the issue. IT CANNOT SEE COMMIT MESSAGES, and a squash merge folds the branch's commits into the merge commit, where GitHub honours closing keywords too. So a PR could pass this check carrying a `Claim-issue:` trailer -- which promises to leave the issue OPEN -- and close that very issue on merge. MEASURED, not hypothetical. #296 carried `Claim-issue: #295` in its body and `Closes #295` in commit 741e8aa. closingIssuesReferences was empty, this check went green, and the merge closed #295 -- which made the follow-up PR that named it unmergeable (.github-private#780). The trap defeats someone doing the visible, reviewable thing correctly: the PR body was right, and the surface nobody re-reads at merge time was not. The predicate is a CONTRADICTION, not a style rule. The PR states its intent twice, in conflict, and only the commit message is honoured. So this fires only when both statements name the SAME issue, in THIS repository -- GitHub does not honour a cross-repo closing keyword on merge, so the comparison is scoped to `$GITHUB_REPOSITORY`. THE SCOPE GUARD MATTERS AS MUCH AS THE CHECK. A plain `Closes` PR with no trailer is the ordinary case and the overwhelming majority of PRs; a check that reddened those would be reverted within a day and would deserve to be. Pinned by a case, as is `Claim-issue: #A` + `Closes #B`, which is legitimate. THE EXTRACTION IS JQ'S, NOT THE SHELL'S, and the first cut got this wrong: it piped commit messages through `read` with a NUL separator. A shell variable cannot hold NUL, and a commit message is multi-line, so line-reading tears it apart -- it silently matched nothing and the guard never fired. `scan` applies the regex inside jq and emits one `oid<TAB>number` row per hit. The tests caught it; nothing else would have, because a guard that finds nothing looks exactly like a guard with nothing to find. The test EXTRACTS THE STEP FROM THE WORKFLOW and runs it against a stubbed `gh`, so editing the check changes what the cases execute rather than leaving a copy to drift (#368). Mutation-checked rather than merely passing: · guard removed entirely -> red · repo scoping dropped -> 2 cases red · keyword list narrowed -> 1 case red · restored -> 6/6 green A PR with more than 100 commits warns that the scan was truncated rather than reporting a clean pass over a set it did not see. 477 tests pass; actionlint clean on this file. Claim-issue: #298 Co-Authored-By: Claude <noreply@anthropic.com>
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
2 participants
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.
closingIssuesReferencesis GitHub's own resolution of the PR body's closing keywords, and this file's header rightly prefers it to a regex: a keyword the check accepts is exactly a keyword that will close the issue.It cannot see commit messages. A squash merge folds the branch's commits into the merge commit, where GitHub honours closing keywords too. So a PR could pass this check carrying a
Claim-issue:trailer — which promises to leave the issue open — and close that very issue on merge.Measured, not hypothetical
#296 carried
Claim-issue: bounded-systems/.github#295in its body andCloses #295in commit741e8aa.closingIssuesReferenceswas empty, this check went green, and the merge closed #295 — which made the follow-up PR that named it unmergeable (.github-private#780).The trap defeats someone doing the visible thing correctly: the PR body was right, and the surface nobody re-reads at merge time was not.
The predicate is a contradiction, not a style rule
The PR states its intent twice, in conflict, and only the commit message is honoured. So this fires only when both statements name the same issue, in this repository — GitHub does not honour a cross-repo closing keyword on merge, so the comparison is scoped to
$GITHUB_REPOSITORY.The scope guard matters as much as the check. A plain
ClosesPR with no trailer is the ordinary case and the overwhelming majority of PRs; a check that reddened those would be reverted within a day and would deserve to be. Pinned by a case, as isClaim-issue: #A+Closes #B, which is legitimate.The extraction is jq's, not the shell's — and the first cut got this wrong
I piped commit messages through
readwith a NUL separator. A shell variable cannot hold NUL, and a commit message is multi-line, so line-reading tears it apart — it silently matched nothing and the guard never fired.The tests caught it. Nothing else would have, because a guard that finds nothing looks exactly like a guard with nothing to find.
scanapplies the regex inside jq and emits oneoid<TAB>numberrow per hit.Verified
The test extracts the step from the workflow and runs it against a stubbed
gh, so editing the check changes what the cases execute rather than leaving a copy to drift (#368).closes477 repo tests pass; actionlint clean on this file.
A PR with more than 100 commits warns that the scan was truncated rather than reporting a clean pass over a set it did not see.
This PR is a positive test of its own guard
Its body and its commit both carry
Claim-issue: bounded-systems/.github#298and no closing keyword — so the new check should pass on the very PR that introduces it.[settings]/[org]None.
Claim-issue: #298
Generated by Claude Code