Skip to content

ci: require every PR description to link its issue - #1835

Merged
ericgriffin merged 2 commits into
mainfrom
ericgriffin/github-issue-1800-6a78c2
Sep 13, 2026
Merged

ericgriffin merged 2 commits into
mainfrom
ericgriffin/github-issue-1800-6a78c2

Conversation

@ericgriffin

@ericgriffin ericgriffin commented Sep 13, 2026 •

Copy link
Copy Markdown
Member

Related Issue

Closes #1800

Summary

GitHub closes an issue on merge only when a closing keyword sits directly before the issue number in the PR description. A number in the title, or a passing mention in prose, links nothing, so a resolved issue can stay open after its PR merges. This PR makes the link a required, checked part of every PR: its description must say which issue it relates to.

Changes

  • New PR Issue Link workflow (.github/workflows/pr-issue-link.yml) runs scripts/check_pr_issue_link.py on pull_request_target (opened, edited, reopened, synchronize). It fails unless the description contains:
    • a closing link, Closes / Fixes / Resolves #N (all nine GitHub keywords, case-insensitive, optional colon), or
    • a non-closing link, Refs / Part of / Related to #N, for phased work and follow-ups that must not close the issue early.
  • Branch names are checked. When the branch encodes an issue (github-issue-N, feature-request-N, issue-N), that issue must be among the links.
  • Bot-authored PRs are exempt, so the version-bump PR from promote.yml and Dependabot PRs still auto-merge.
  • Text GitHub does not parse for links is ignored: HTML comments, code spans and fenced code, resolved in document order. The PR template keeps its examples in a comment, so a description left as the bare template fails.
  • Why a separate workflow: the check has to re-run on edited. In ci.yaml that trigger would restart the whole pipeline on every description edit.
  • Why pull_request_target: under pull_request the workflow and script come from the PR's merge ref, so a PR could edit either to report green and bypass its own gate. pull_request_target runs both as they exist on main. Its check run still lands on the PR head commit (verified on this repo: a pull_request_target run's head_sha is the PR head), which is where branch protection looks. The trigger is safe here because the job never checks out or runs PR code: it checks out main with credentials not persisted, reads the description through the API, and passes untrusted fields to the script only through env vars.
  • The body is fetched live with gh api, so re-running a failed job sees the corrected description.
  • Tests: unit tests run in ci.yaml's Script Tests coverage step. They cover 100% of the script.
  • Docs:
    • CLAUDE.md, CONTRIBUTING.md and .github/PULL_REQUEST_TEMPLATE.md (new "Related Issue" section) document the rule.
    • docs/contributing/pull-requests.md gains a "Linking Issues" section.
    • Its broken closing fences are repaired: every block closed with ```text, which is not a valid closing fence, so most of the page rendered as one code block.
    • Its stale template sample is replaced.

The examples in #1800

None of the three examples is actually a missed auto-close, so this PR leaves them as they are:

After merge

The check blocks merges only once it is a required status check. This adds it without touching CI Success or any other protection field:

gh api -X POST repos/submersion-app/submersion/branches/main/protection/required_status_checks/contexts -f 'contexts[]=PR Issue Link'

Once it is required, PRs that are already open show "Expected" until someone pushes to them or edits their description. Over the last 40 merged PRs, 26 linked no issue and would have been blocked, so expect most PRs to need an issue opened or referenced first.

Test Plan

  • python3 scripts/check_pr_issue_link_test.py: 48 tests pass on Python 3.14 and 3.9, 100% line coverage of the script
  • Parity check against the last 40 merged PRs: the set of issues the script says will close matches GitHub's closingIssuesReferences for every PR (0 mismatches); both bot bump PRs pass
  • Dry run on the bare PR template fails, and flags the branch's own issue
  • Mutation check: breaking the closing-fence rule turns its test red
  • python3 scripts/check_ci_success_gate.py passes; actionlint reports nothing for the new workflow
  • Pre-push hook (format, analyze) passes
  • The first commit (then on pull_request) ran the check on this PR and passed: ok closes #1800. Since the switch to pull_request_target, the workflow runs as it exists on main, so this PR gets no further run; the first run on another PR happens after merge.

Closes #1800

A merged PR closes an issue only when a closing keyword sits directly
before the issue number in its description. Without one, a resolved
issue stays open after the merge.

Add a "PR Issue Link" workflow that fails unless the description links
an issue: a closing link (Closes/Fixes/Resolves #N) when the PR resolves
it, or a non-closing link (Refs/Part of/Related to #N) when it does not.
When the branch name encodes an issue (github-issue-N,
feature-request-N, issue-N), that issue must be among the links.
Bot-authored PRs are exempt. HTML comments, code spans and fenced code
are ignored, as GitHub ignores them, so the template's examples cannot
satisfy the check.

The rule lives in scripts/check_pr_issue_link.py, unit-tested under
Script Tests. The workflow is separate from ci.yaml because it must
re-run on the `edited` event, which in ci.yaml would restart the whole
pipeline on every description edit.

Document the convention in CLAUDE.md, CONTRIBUTING.md, the PR template
and docs/contributing/pull-requests.md. That guide also gets its broken
closing fences repaired and its stale template sample replaced.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Unresolved workflow security and HTML-comment parsing findings must be addressed.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds automated enforcement that every PR description links an issue, with validation, workflow integration, tests, and documentation updates.

Changes:

  • Adds issue-link parsing, branch validation, and bot exemptions.
  • Adds the PR Issue Link workflow and CI coverage.
  • Updates PR templates and contributor documentation.
File summaries
File Final review notes
scripts/check_pr_issue_link.py Moderate findings (1 and 2 votes): HTML comments can discard valid text following their closing markers.
scripts/check_pr_issue_link_test.py No final review comments.
docs/contributing/pull-requests.md No final review comments.
CONTRIBUTING.md No final review comments.
CLAUDE.md No final review comments.
.github/workflows/pr-issue-link.yml Critical finding (3 votes): the PR-controlled workflow and script can bypass the check; use trusted base-branch execution.
.github/workflows/ci.yaml No final review comments.
.github/PULL_REQUEST_TEMPLATE.md No final review comments.
Review details

Suppressed comments (2)

scripts/check_pr_issue_link.py:109

  • When a multiline comment closes on this line, the continue discards everything after --> as well. A description such as <!--\nexample\n--> Closes #1800 therefore fails even though Closes #1800 is outside the comment; preserve and reprocess the suffix after the closing marker.
        if in_comment:
            in_comment = "-->" not in line
            continue

scripts/check_pr_issue_link.py:159

  • Splitting into paragraphs before stripping inline markup lets an HTML comment span a blank line and then exposes its contents to the matcher. For example, intro <!--\n\nCloses #1800\n--> leaves Closes #1800 in the second paragraph even though GitHub treats it as comment content; strip inline constructs over the full block or carry comment state across paragraph boundaries.
    paragraphs = _PARAGRAPH_BREAK.split(_strip_blocks(body or ""))
    return "\n\n".join(_strip_inline(p) for p in paragraphs)
  • Files reviewed: 8/8 changed files
  • Comments generated: 2
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread .github/workflows/pr-issue-link.yml Outdated
Comment thread scripts/check_pr_issue_link.py Outdated
@codecov

codecov Bot commented Sep 13, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@github-actions

github-actions Bot commented Sep 13, 2026 •

Copy link
Copy Markdown
Contributor

📦 Build artifacts for this PR · commit a59f5f8

Platform Download
Android (APK) android-apk
macOS macos-build
Windows windows-build
Linux linux-build

Artifacts expire in 7 days. Downloading requires being signed in to GitHub. macOS needs two extractions: unzip the downloaded artifact, then unzip the submersion-macos.zip inside it to get a runnable submersion.app. The build is ad-hoc signed — right-click → Open on first launch.

Updated automatically on each push.

Refs #1800

Run the workflow on pull_request_target. Under pull_request the workflow
and script come from the PR's merge ref, so a PR could edit either to
report green and bypass its own gate. pull_request_target runs both as
they exist on main, while its check run still lands on the PR head
commit that branch protection reads. The job never checks out or runs
PR code (main checkout, credentials not persisted, description read
through the API), which is what makes that trigger safe here.

Keep visible text after an HTML comment closes. A comment opening and
closing on one line was dropped with the rest of that line, and the
closing line of a multi-line comment lost its suffix, so
`<!-- note --> Closes #1800` did not count as a link.
@ericgriffin
ericgriffin merged commit 6619eea into main Sep 13, 2026
35 checks passed
@github-project-automation github-project-automation Bot moved this from In review to Done in Submersion Release Tracker Sep 13, 2026
@ericgriffin
ericgriffin deleted the ericgriffin/github-issue-1800-6a78c2 branch September 13, 2026 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Merged PRs sometimes don't auto-close the issue they resolve

2 participants