Repository navigation
ci(labels): parse issue references from PR body in sync-issue-state-labels - #553
Merged
claymore666 merged 3 commits intoAug 16, 2026
Conversation
claymore666
reviewed
Aug 16, 2026
Owner
There was a problem hiding this comment.
Thanks — this is careful work, and stripping HTML comments before parsing is the detail I would have got wrong (our own PR template line 13 is <!-- e.g. Closes #123 -->).
I have rebased this onto dev for you rather than making you do it — the branch was based on a merge commit on main, and contributions here branch from dev. Your commit is unchanged (identical diff, same authorship); only the base moved. If you have local work on this branch, fetch and reset to the new head before continuing.
Three things left.
1. Fixes #548 → Refs #548. PRs into dev use Refs; the release PR into main carries the batch Closes list.
2. .github/workflows/issue-state-labels.yml line 23 needs updating here. It says "the only PR-derived input is the title". This makes the body an input too, on a pull_request_target trigger. The security property still holds — the body is reduced to digits and intersected with open issue numbers — but the comment is the rationale for that trigger and is now false as written.
3. This fixes has_pr only, so #548 stays open. The script has two consumers of PR text and this changes one:
has_pr, line 228 — yourpr_refsfixes it, correctly.in_dev, line 225 underTHE ONE HOP— stillin_dev |= set(refs(title)) & open_numbers, title-only.
The hop is where the bug actually bit: PR #532's subject carries only (#532) while its body says Closes #530, so #530 read as untouched for days after its work had merged.
Concretely, carrying it would be two edits:
- the hop currently asks the API for
.titlealone; it needsbodyas well, written next topr_titles.jsonin the same{number: value}shape (saypr_bodies.json) - line 225 becomes the union:
in_dev |= (set(refs(title)) | set(body_refs(body))) & open_numbers
and --plan should treat an absent pr_bodies.json as "no bodies", so the existing offline fixtures stay valid.
Happy either way — tell me if you would rather land this half and let us do the hop, and I will take it from here.
claymore666
force-pushed
the
fix/sync-issue-state-labels-closes-parsing
branch
from
August 16, 2026 09:14
243f39f to
1d4e2d6
Compare
Contributor
Author
|
Thanks for the detailed review and for rebasing the branch. Please go ahead and carry the complete one-hop body path from here; I don't want to duplicate the work already on your maintainer branch. I'll leave this PR as-is. I appreciate the concrete notes. |
claymore666
force-pushed
the
fix/sync-issue-state-labels-closes-parsing
branch
from
August 16, 2026 15:11
3be5e30 to
0be5280
Compare
This was referenced Aug 16, 2026
The note said "the only PR-derived input is the title". This PR makes that false: the script now reads the body as well, and a comment that describes a narrower attack surface than the code has is worse than no comment — it is the thing a reviewer checks instead of the code. The distinction that matters is unchanged. A body is arbitrary multi-line text where a title is a line, so it is bigger, but it is handled identically: never executed, never checked out, never written anywhere, matched for closing keywords and issue numbers and discarded. `pull_request_target` still runs without a PR checkout. Also records why the body travels as JSON rather than in a delimited form: hand-rolled framing of arbitrary multi-line input is a parser bug waiting to happen. Refs #548
Builds on the body parser this branch already adds, and closes the half of #548 that parser does not reach. There are two consumers of PR text in this script. `has_pr`, over the open-PR listing, now uses `pr_refs` and reads bodies. `in_dev`, via THE ONE HOP, still asked only for `.title` and ran only `refs()` over it — so the case the issue was filed for was still broken. That case is the ordinary one here, not an edge. A squash subject carries the PR's own number and nothing else: ci: raise the go.mod directive ... (#525) (#529) so an issue named only in the body is reachable through the body or not at all. PR 532's subject is "(#532)" while its body says "Closes #530", and #530 read as untouched for days after its work had merged. The hop now fetches title and body in a single request — the same lookup that was already being made — and writes them as JSON rather than delimited text, because a body is multi-line arbitrary input and any hand-rolled framing would be a parser bug waiting to happen. `pr_bodies.json` is a separate optional file, so every fixture written before this stays valid and keeps working as a negative control. `body_refs` is used rather than `refs`, deliberately: only GitHub's closing keywords count. This repo uses "Refs #N" for "related to", and PR #550 refs #549 while fixing one of its two findings. Counting that would make the label lie in the other direction, which is worse than the under-labelling being fixed. The trust model is unchanged. A body is attacker-controlled on a public repo exactly as a title is; it is never executed, never checked out, and is reduced to digits that must already appear in the repo's own open-issue list before anything is labelled. Tests, all of which fail without this commit and pass with it: - a merged PR naming its issue only in the body earns in-dev - an issue merely mentioned in the body is not dragged in - a body that only says Refs #N earns nothing - a hostile body is inert - an absent pr_bodies.json is a no-op, never an error - an unreadable pr_bodies.json fails loudly, for the same reason its title counterpart does: a hop that silently finds nothing strips labels off issues that had earned them Refs #548
claymore666
force-pushed
the
fix/sync-issue-state-labels-closes-parsing
branch
from
August 16, 2026 15:32
0be5280 to
abf84ef
Compare
Merged
claymore666
added a commit
that referenced
this pull request
Aug 16, 2026
* docs: describe the reclaim and the parent gate, and record how the review found them
v1.6.0's headline behaviour is that leases stop leaking, and internals.md
-- the "How it works" page -- had no section describing the reclaim at
all. It also had none for the per-parent gate, the release's other new
mechanism. Both are added; the reclaim's scoping carries its reasoning,
because releasing on the wrong path is the change that would cause an
outage rather than a leak.
Nothing was WRONG, which is the point. Every counter table was green,
`check-docs-drift`, `check-option-docs`, `check-version-pins` and
`check-cosign-docs` all passed throughout, and no grep finds a section
that does not exist.
So the runbook's documentation review gains the three checks that would
have caught this pass's findings without a person happening to look:
- mechanisms the release added that no page describes -- absence, not
wrongness, and invisible to every gate we have
- standing preconditions that read like old-version prose: the
STATE_DIR block names v1.5.0 but is a precondition for EVERY
install, and a pass tidying stale versions deletes it in good faith
(gate tracked in #579)
- counted claims -- "four flip healthy to false" stays grammatical
when a fifth is added
Step 4 also gains: find outside contributions by query rather than
memory, and confirm the merged commit still carries their authorship,
since rebasing or squashing a fork branch is where that silently becomes
the maintainer's.
Refs #486, #549, #567, #579
* docs: add the v1.6.0 release notes
Leads with what changed for an operator: an address already in use is
now reported, leases are handed back instead of leaking, and ipvlan
networks work where they previously refused.
Restates the v1.5.0 STATE_DIR precondition at the top. It has not
changed, but it still gates every install, and someone reading the notes
for the version they are installing should not have to scroll into an
older section to find that out.
States plainly that images remain amd64-only and why (#507), rather than
leaving a reader to infer it from the absence of a claim.
Credits both outside contributions: @snowyukitty for #553, and @sdjnmxd
for the report behind #507. Both handles verified against the artifacts.
Refs #524, #549, #566, #567, #572, #486, #507, #553
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.
Problem
scripts/sync-issue-state-labels.shresolved issue references from PR titles only. There are two consumers of PR text in that script, and both were affected:has_pr, over the open-PR listing, matchedrefs(pr["title"]). A PR following the repository template (## Related issue/Closes #123) earned nohas-prlabel unless the number was also in the title.in_dev, via the one hop over merged PRs, asked only for.titleand ran onlyrefs()over it — so the case Issue state labels are resolved from PR titles, so a PR that names its issue only in the body is invisible #548 was filed for stayed broken even with the first half fixed.The second is the ordinary case here, not an edge. A squash subject carries the PR's own number and nothing else:
so an issue named only in the body is reachable through the body or not at all.
This is not hypothetical. PR #532's subject is
(#532)while its body saysCloses #530. #530 read as untouched for days after its work had merged — and when the label was applied by hand, the reconciler stripped it again within four minutes. #522 the same. The release PR'sCloses #...list is built from this state, so left unfixed it would omit issues that are genuinely done.Solution
body_refs(body)andpr_refs(pr)parse GitHub's closing keywords (close/closes/closed,fix/fixes/fixed,resolve/resolves/resolved) from PR prose, case-insensitively. HTML comments are stripped first, so a template example like<!-- e.g. Closes #123 -->cannot create a false reference.--json number,title,body, sohas_prsees bodies.pr_bodies.jsonis a separate optional file, so every fixture written before this stays valid and keeps working as a negative control.Refs #Nis deliberately excludedThis repository uses
Refs #Nfor "related to". A PR that merely references an issue has not delivered it — #550 refs #549 while fixing one of its two findings. Counting it would make the label lie in the other direction, which is worse than the under-labelling being fixed.Security
pull_request_targetstill runs without a PR checkout. The note inissue-state-labels.ymlclaimed the title was the only PR-derived input; this change makes that false, so the note is corrected in its own commit. A comment describing a narrower attack surface than the code has is the thing a reviewer checks instead of the code.The body is more attacker-controlled than the title only in length, not in kind: never executed, never checked out, never written anywhere, reduced to digits that must already match an open issue number.
Testing
Closes #N,Fixes #N, #M(both numbers),Resolves: #N, lowercase, uppercase.in-dev; a mere mention does not; a body that only saysRefs #Nearns nothing; a hostile body is inert; an absentpr_bodies.jsonis a no-op; an unreadable one fails loudly, because a hop that silently finds nothing strips labels off issues that had earned them.run-gate-selftests.sh: 31 passed, 1 delegated.Note on the trailer
Refs #548, notFixes. This project batch-closes issues from the release PR when it reachesmain; a closing keyword here would close #548 on thedevmerge and drop it from the release notes.Refs #548