From 406507a6a0e6627cbf7690bd3e1dba5f1a7d6862 Mon Sep 17 00:00:00 2001 From: Eric Griffin Date: Sat, 12 Sep 2026 23:33:15 -0400 Subject: [PATCH 1/2] ci: require every PR description to link its issue 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. --- .github/PULL_REQUEST_TEMPLATE.md | 16 ++ .github/workflows/ci.yaml | 4 +- .github/workflows/pr-issue-link.yml | 63 +++++++ CLAUDE.md | 21 +++ CONTRIBUTING.md | 16 +- docs/contributing/pull-requests.md | 110 ++++++++---- scripts/check_pr_issue_link.py | 250 ++++++++++++++++++++++++++ scripts/check_pr_issue_link_test.py | 266 ++++++++++++++++++++++++++++ 8 files changed, 706 insertions(+), 40 deletions(-) create mode 100644 .github/workflows/pr-issue-link.yml create mode 100644 scripts/check_pr_issue_link.py create mode 100644 scripts/check_pr_issue_link_test.py diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index 8f389379bb..72fcb2ea34 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -1,3 +1,19 @@ +## Related Issue + + + ## Summary diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 98d45f6468..fb456ab811 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -521,7 +521,7 @@ jobs: - name: Run Python guard tests with coverage run: | mkdir -p coverage - guards='scripts/check_proguard_serial_keep.py,scripts/check_apk_signing_cert.py,scripts/check_native_libs_present.py,scripts/check_bundled_native_assets.py,scripts/check_jni_local_refs.py,scripts/check_dc_process_isolation.py,scripts/check_ci_success_gate.py,scripts/fix_macho_symbol_order.py,scripts/release/sanitize_apple_store_notes.py' + guards='scripts/check_proguard_serial_keep.py,scripts/check_apk_signing_cert.py,scripts/check_native_libs_present.py,scripts/check_bundled_native_assets.py,scripts/check_jni_local_refs.py,scripts/check_dc_process_isolation.py,scripts/check_ci_success_gate.py,scripts/check_pr_issue_link.py,scripts/fix_macho_symbol_order.py,scripts/release/sanitize_apple_store_notes.py' python3 -m coverage run --include="$guards" \ scripts/check_proguard_serial_keep_test.py python3 -m coverage run --append --include="$guards" \ @@ -536,6 +536,8 @@ jobs: scripts/check_dc_process_isolation_test.py python3 -m coverage run --append --include="$guards" \ scripts/check_ci_success_gate_test.py + python3 -m coverage run --append --include="$guards" \ + scripts/check_pr_issue_link_test.py python3 -m coverage run --append --include="$guards" \ scripts/fix_macho_symbol_order_test.py python3 -m coverage run --append --include="$guards" \ diff --git a/.github/workflows/pr-issue-link.yml b/.github/workflows/pr-issue-link.yml new file mode 100644 index 0000000000..25213d3c36 --- /dev/null +++ b/.github/workflows/pr-issue-link.yml @@ -0,0 +1,63 @@ +name: PR Issue Link + +# Every pull request must link the issue it relates to (issue #1800): a +# closing keyword (`Closes #N`) when it resolves the issue, or `Refs #N` / +# `Part of #N` / `Related to #N` when it does not. Without the keyword GitHub +# leaves a resolved issue open after the merge. The rule itself lives in +# scripts/check_pr_issue_link.py, which Script Tests in ci.yaml unit-tests. +# +# Kept out of ci.yaml on purpose. The check has to re-run when the description +# is edited, and adding `edited` to ci.yaml's triggers would restart the whole +# pipeline on every description edit. It blocks a merge by being listed as a +# required status check ("PR Issue Link") next to CI Success in main's branch +# protection. No `paths` filter: a required check that never reports leaves +# the PR waiting forever. + +on: + pull_request: + branches: [main] + types: [opened, edited, reopened, synchronize] + +permissions: + contents: read + pull-requests: read + +concurrency: + group: ${{ github.workflow }}-${{ github.event.pull_request.number }} + cancel-in-progress: true + +jobs: + issue-link: + name: PR Issue Link + runs-on: ubuntu-latest + timeout-minutes: 5 + steps: + - uses: actions/checkout@v7 + with: + sparse-checkout: scripts + + - name: Fetch the current PR description + # Read live rather than from the event payload: re-running a failed + # job replays the original payload, which would still hold the + # description from before the author fixed it. + env: + GH_TOKEN: ${{ github.token }} + REPO: ${{ github.repository }} + PR_NUMBER: ${{ github.event.pull_request.number }} + run: | + gh api "repos/${REPO}/pulls/${PR_NUMBER}" --jq '.body // ""' \ + > "${RUNNER_TEMP}/pr_body.md" + + - name: Check the description links an issue + # Untrusted PR fields reach the script only through env vars, never + # through ${{ }} interpolation inside the shell text. + env: + REPO: ${{ github.repository }} + HEAD_REF: ${{ github.event.pull_request.head.ref }} + AUTHOR_TYPE: ${{ github.event.pull_request.user.type }} + run: | + python3 scripts/check_pr_issue_link.py \ + --body-file "${RUNNER_TEMP}/pr_body.md" \ + --branch "$HEAD_REF" \ + --author-type "$AUTHOR_TYPE" \ + --repo "$REPO" diff --git a/CLAUDE.md b/CLAUDE.md index ae835803d9..3cee800e22 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -32,6 +32,27 @@ git at them. **Bypass (if needed):** `git push --no-verify` +## Pull Requests + +Every PR must relate to an issue, and its description must say which one. The +"PR Issue Link" check (`scripts/check_pr_issue_link.py`) blocks the merge until +it does. + +- **Resolves the issue:** `Closes #123` (or `Fixes` / `Resolves`). GitHub + closes the issue on merge only when the keyword sits directly before the + number in the PR description. A number in the title, or a passing mention in + prose, closes nothing, which is how resolved issues were left open (#1800). +- **Relates without resolving it** (one phase of a larger issue, a follow-up): + `Refs #123`, `Part of #123` or `Related to #123`. The issue stays open. +- **Several issues:** one keyword per issue, `Closes #1, closes #2`. + `Closes #1, #2` closes only #1. +- **Branch names an issue** (`github-issue-1800-...`, `feature-request-1803-...`): + the description must link that issue, with `Refs` if the PR does not resolve it. +- **No issue yet:** open one first. Only bot-authored PRs are exempt. + +Links inside HTML comments, code spans or fenced code blocks are ignored, by +GitHub and by the check. Editing the description re-runs the check. + ## Gotchas - The `dives` table uses `diveDateTime` (not `dateTime`) as the column name to diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index fdc7481bd4..511c375b50 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -81,10 +81,20 @@ files you touched) before committing. 1. Push your branch and open a pull request against `main`. 2. Fill out the [pull request template](.github/PULL_REQUEST_TEMPLATE.md), - describing what changed and why, and link any related issues. -3. Keep PRs focused and reasonably small — one logical change per PR is easier + describing what changed and why. +3. **Link the issue the PR relates to.** Every PR must relate to an issue, and + the "PR Issue Link" check blocks the merge until the description links one: + - `Closes #123` (or `Fixes` / `Resolves`) when the PR fully resolves the + issue. GitHub closes the issue on merge only when the keyword sits + directly before the number in the description; a number in the title does + nothing. Give each issue its own keyword: `Closes #1, closes #2`. + - `Refs #123`, `Part of #123` or `Related to #123` when the PR relates to an + issue without resolving it, so the issue stays open. + + If no issue exists yet, open one first. +4. Keep PRs focused and reasonably small: one logical change per PR is easier to review and merge. -4. Ensure CI passes. Maintainers may request changes; discussion is part of the +5. Ensure CI passes. Maintainers may request changes; discussion is part of the process. For detailed conventions, see: diff --git a/docs/contributing/pull-requests.md b/docs/contributing/pull-requests.md index 7f069a4938..19b1fbd650 100644 --- a/docs/contributing/pull-requests.md +++ b/docs/contributing/pull-requests.md @@ -10,6 +10,9 @@ This guide explains how to submit effective pull requests. 2. Check [open PRs](https://github.com/submersion-app/submersion/pulls) 3. Review the [roadmap](contributing/roadmap.md) +Every PR must relate to an issue (see [Linking Issues](#linking-issues)). If +none exists for your change, open one before you open the PR. + ### Discuss Large Changes For significant changes: @@ -32,7 +35,8 @@ git remote add upstream https://github.com/submersion-app/submersion.git # Create branch git checkout -b feature/your-feature -```text +``` + ### 2. Make Changes - Follow [code style](contributing/code-style.md) @@ -48,12 +52,14 @@ git commit -m "feat: add nitrox calculator" git commit -m "fix: correct MOD calculation for trimix" git commit -m "docs: add calculator documentation" git commit -m "test: add unit tests for gas calculations" -```text +``` + ### 4. Push ```bash git push origin feature/your-feature -```text +``` + ### 5. Open PR 1. Go to your fork on GitHub @@ -62,46 +68,64 @@ git push origin feature/your-feature ## PR Template -```markdown -## Description +GitHub pre-fills new PRs from +[`.github/PULL_REQUEST_TEMPLATE.md`](https://github.com/submersion-app/submersion/blob/main/.github/PULL_REQUEST_TEMPLATE.md). +A filled-out description looks like this: -Brief description of what this PR does. +```markdown +## Related Issue -## Type of Change +Closes #123 -- [ ] Bug fix -- [ ] New feature -- [ ] Breaking change -- [ ] Documentation +## Summary -## Changes Made +Fixes the MOD calculation for trimix, which used the O2 fraction of air. -- Added X -- Modified Y -- Removed Z - -## Testing +## Changes -- [ ] Unit tests added/updated -- [ ] Widget tests added/updated -- [ ] Manual testing performed +- Pass the mix's own O2 fraction into `calculateMod` +- Add unit tests for air, EAN32 and 18/45 trimix -## Checklist +## Test Plan -- [ ] Code follows style guidelines -- [ ] Tests pass locally -- [ ] Documentation updated -- [ ] No new warnings from `flutter analyze` +- [x] `flutter test` passes +- [x] `flutter analyze` passes +- [x] Manual testing on: macOS ## Screenshots -(If applicable) +(Delete this section if not applicable.) +``` -## Related Issues +## Linking Issues + +Every PR must relate to an issue. The **PR Issue Link** check blocks the merge +until the description links one, and re-runs whenever the description is +edited. + +| The PR... | Write | On merge | +| --- | --- | --- | +| fully resolves the issue | `Closes #123` (or `Fixes` / `Resolves`) | the issue closes | +| is one step of a larger issue | `Part of #123` | the issue stays open | +| is related work or a follow-up | `Refs #123` or `Related to #123` | the issue stays open | + +Things that trip people up: + +- **Only the description counts.** GitHub closes an issue only when a closing + keyword sits directly before the number in the PR description. A number in + the PR title, or a passing mention like "builds on #123", links nothing. +- **One keyword per issue.** `Closes #1, closes #2` closes both; + `Closes #1, #2` closes only #1. +- **Comments and code do not count.** A link inside an HTML comment + (``) or a code span is ignored, by GitHub and by the check. The + template's examples live in a comment for that reason, so an unedited + template fails. +- **Branch names are checked.** If the branch name contains an issue number + (`issue-123-...`, `github-issue-123-...`, `feature-request-123-...`), the + description must link that issue, with `Refs` if the PR does not resolve it. +- **No issue yet?** Open one first. Only bot-authored PRs (version bumps, + Dependabot) are exempt. -Fixes #123 -Related to #456 -```text ## PR Best Practices ### Keep PRs Small @@ -114,7 +138,7 @@ Related to #456 - Explain what and why - Include context -- Link related issues +- Link the issue with `Closes #N` or `Refs #N` (see [Linking Issues](#linking-issues)) ### Add Screenshots @@ -155,7 +179,8 @@ For UI changes: git add . git commit -m "fix: address review feedback" git push origin feature/your-feature -```text +``` + ## After Merge ### Clean Up @@ -169,7 +194,8 @@ git branch -d feature/your-feature # Update from upstream git pull upstream main -```text +``` + ### Celebrate Your contribution is now part of Submersion! @@ -179,6 +205,9 @@ Your contribution is now part of Submersion! ### Bug Fixes ```markdown +## Related Issue +Closes #123 + ## Description Fixes incorrect depth unit conversion when switching between metric and imperial. @@ -191,10 +220,14 @@ Corrected the conversion factor from 3.28084 to 0.3048 for feet to meters. ## Testing - Added unit tests for both conversion directions - Manually verified in settings page -```text +``` + ### New Features ```markdown +## Related Issue +Closes #456 + ## Description Adds a nitrox calculator to the tools section. @@ -209,10 +242,14 @@ Adds a nitrox calculator to the tools section. ## Documentation - Updated tools section in user guide -```text +``` + ### Refactoring ```markdown +## Related Issue +Refs #789 + ## Description Refactors dive repository to use a base repository class. @@ -226,7 +263,8 @@ Reduces code duplication across repositories. ## Testing All existing tests pass without modification. -```text +``` + ## Common Issues ### Merge Conflicts diff --git a/scripts/check_pr_issue_link.py b/scripts/check_pr_issue_link.py new file mode 100644 index 0000000000..28a4457d67 --- /dev/null +++ b/scripts/check_pr_issue_link.py @@ -0,0 +1,250 @@ +#!/usr/bin/env python3 +"""Require every pull request description to link the issue it relates to. + +GitHub closes an issue on merge only when a closing keyword sits directly in +front of the issue number in the PR description (or in a commit message that +reaches the default branch). A number in the title, or a passing mention in +prose, links nothing. Issue #1800 found issues left open after the PR that +resolved them had merged, because the description never said `Closes #N`. + +This guard makes the link a deliberate, checked part of every PR. It passes +when the description contains at least one of: + + * a closing link: Close[sd] / Fix(e[sd]) / Resolve[sd] #N + * a non-closing link: Refs / Part of / Related to #N + +The non-closing forms exist for phased work and follow-ups that relate to an +issue without resolving it, so the issue is not closed early. When the branch +name encodes an issue number (`github-issue-1800-...`, `feature-request-1803- +...`, `fix/issue-12`), that particular issue must be one of the links: the +branch says which issue the work is for, so a description that forgets it is +almost certainly the #1800 mistake. + +Bot-authored PRs (the version bump promote.yml opens, Dependabot) pass +unconditionally: they have no issue, and blocking them would stall the bump +PR's auto-merge and with it the beta train. + +Text GitHub does not parse for links is ignored: HTML comments (where the PR +template keeps its examples, so an untouched template fails), fenced code +blocks and inline code spans. Indented code blocks are not stripped, because +telling them apart from nested list content needs a full Markdown parser. + +Pure stdlib, so the workflow needs no dependency install. + +Usage: check_pr_issue_link.py --body-file PATH [--branch NAME] + [--author-type User|Bot] [--repo OWNER/NAME] +""" + +import argparse +import re +import sys + +CLOSING_KEYWORD = r"(?:close[sd]?|fix(?:e[sd])?|resolve[sd]?)" +LINKING_KEYWORD = r"(?:refs|part[ \t]+of|related[ \t]+to)" + +# GitHub accepts an optional colon after the keyword, but the keyword and the +# reference must still be separated by it or by whitespace. +_SEPARATOR = r"(?::[ \t]*|[ \t]+)" +_REFERENCE = r"(?:(?P[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+))?#(?P\d+)\b" + +# re.ASCII: without it `\d` also matches digits such as Arabic-Indic ones, +# which GitHub does not treat as an issue number. +_CLOSING = re.compile( + r"\b" + CLOSING_KEYWORD + r"\b" + _SEPARATOR + _REFERENCE, + re.IGNORECASE | re.ASCII, +) +_LINKING = re.compile( + r"\b" + LINKING_KEYWORD + r"\b" + _SEPARATOR + _REFERENCE, + re.IGNORECASE | re.ASCII, +) + +# `issue-N` must start a path segment or follow a hyphen/underscore, so +# `tissue-5` is not an issue branch. +_BRANCH_ISSUE = re.compile( + r"(?:^|[/_-])(?:issue|feature-request)-(\d+)(?=$|[/_-])", + re.IGNORECASE | re.ASCII, +) + +_FENCE_OPEN = re.compile(r"^ {0,3}(`{3,}|~{3,})(.*)$") +_COMMENT_BLOCK_OPEN = re.compile(r"^ {0,3}`. + An unclosed fence or comment block runs to the end of the text. + """ + kept, fence, in_comment = [], None, False + for line in text.splitlines(): + if fence is not None: + if _closes_fence(line, fence): + fence = None + continue + if in_comment: + in_comment = "-->" not in line + continue + opening = _FENCE_OPEN.match(line) + # A backtick fence's info string cannot contain a backtick, so a line + # like ```x``` is an inline code span, not a fence. + if opening and not ( + opening.group(1)[0] == "`" and "`" in opening.group(2) + ): + fence = opening.group(1) + continue + if _COMMENT_BLOCK_OPEN.match(line): + in_comment = "-->" not in line[line.index("", opening.end()) + close_end = end + 3 if end >= 0 else None + else: + # A code span closes on a backtick run of exactly the same length. + closer = re.compile(r"(? PASS") + return 0 + for line in lines: + if line.startswith(" FAIL"): + print("::error::" + line[len(" FAIL "):]) + print(" -> FAIL: link the issue this PR relates to") + return 1 + + +if __name__ == "__main__": # pragma: no cover + sys.exit(main(sys.argv)) diff --git a/scripts/check_pr_issue_link_test.py b/scripts/check_pr_issue_link_test.py new file mode 100644 index 0000000000..ae5a3726e1 --- /dev/null +++ b/scripts/check_pr_issue_link_test.py @@ -0,0 +1,266 @@ +#!/usr/bin/env python3 +"""Unit tests for check_pr_issue_link.py.""" + +import contextlib +import importlib.util +import io +import os +import tempfile +import unittest + +_HERE = os.path.dirname(os.path.abspath(__file__)) +_spec = importlib.util.spec_from_file_location( + "check_pr_issue_link", + os.path.join(_HERE, "check_pr_issue_link.py"), +) +guard = importlib.util.module_from_spec(_spec) +_spec.loader.exec_module(guard) + +REPO = "submersion-app/submersion" +TEMPLATE = os.path.join(_HERE, os.pardir, ".github", "PULL_REQUEST_TEMPLATE.md") + + +def closing(body): + return guard.closing_refs(body, REPO) + + +def linked(body): + return guard.linking_refs(body, REPO) + + +class ClosingRefsTest(unittest.TestCase): + def test_every_github_closing_keyword_is_recognised(self): + for keyword in ( + "close", "closes", "closed", + "fix", "fixes", "fixed", + "resolve", "resolves", "resolved", + ): + with self.subTest(keyword=keyword): + self.assertEqual(closing(f"{keyword} #10"), {"#10"}) + + def test_keywords_are_case_insensitive(self): + self.assertEqual(closing("CLOSES #10"), {"#10"}) + self.assertEqual(closing("Fixes #10"), {"#10"}) + + def test_a_colon_after_the_keyword_is_allowed(self): + self.assertEqual(closing("Closes: #10"), {"#10"}) + self.assertEqual(closing("Closes:#10"), {"#10"}) + + def test_keyword_must_be_separated_from_the_reference(self): + self.assertEqual(closing("Closes#10"), set()) + + def test_each_issue_needs_its_own_keyword(self): + # GitHub closes only #1 here; the check must not promise more. + self.assertEqual(closing("Fixes #1, #2"), {"#1"}) + self.assertEqual(closing("Fixes #1, fixes #2"), {"#1", "#2"}) + + def test_keyword_must_be_a_whole_word(self): + self.assertEqual(closing("prefixes #3"), set()) + self.assertEqual(closing("unresolved #3"), set()) + + def test_reference_must_end_at_a_word_boundary(self): + self.assertEqual(closing("Fixes #12abc"), set()) + self.assertEqual(closing("Fixes #12."), {"#12"}) + + def test_cross_repository_reference_keeps_its_repository(self): + self.assertEqual( + closing("Fixes octo-org/octo-repo#100"), {"octo-org/octo-repo#100"} + ) + + def test_same_repository_long_form_normalises_to_short_form(self): + self.assertEqual(closing("Fixes Submersion-App/Submersion#7"), {"#7"}) + + def test_mentions_without_a_keyword_do_not_close(self): + self.assertEqual(closing("Follows up on #1790 and #1806."), set()) + + def test_keywords_inside_html_comments_are_ignored(self): + self.assertEqual(closing(""), set()) + self.assertEqual(closing("\nFixes #4"), {"#4"}) + + def test_an_unterminated_html_comment_hides_the_rest(self): + self.assertEqual(closing("Fixes #4\n Fixes #4"), {"#4"}) + + def test_a_comment_opener_inside_inline_code_is_literal(self): + # Whichever construct starts first wins: here the code span does, so + # the `"), set()) + + +class BranchIssueTest(unittest.TestCase): + def test_issue_worktree_branch(self): + self.assertEqual( + guard.branch_issue("ericgriffin/github-issue-1800-6a78c2"), "#1800" + ) + + def test_feature_request_worktree_branch(self): + self.assertEqual( + guard.branch_issue("ericgriffin/feature-request-1803-9f1671"), "#1803" + ) + + def test_conventional_issue_branch(self): + self.assertEqual(guard.branch_issue("fix/issue-12"), "#12") + self.assertEqual(guard.branch_issue("issue-12-short-name"), "#12") + + def test_branches_without_an_issue_number(self): + for branch in ("ericgriffin/bulk-use-set-1754", "tissue-5", "main", ""): + with self.subTest(branch=branch): + self.assertIsNone(guard.branch_issue(branch)) + + +class EvaluateTest(unittest.TestCase): + def evaluate(self, body, branch="feature/x", author_type="User"): + return guard.evaluate(body, branch, author_type, REPO) + + def test_bot_authored_prs_pass_without_a_link(self): + ok, lines = self.evaluate("", author_type="Bot") + self.assertTrue(ok) + self.assertIn("bot", " ".join(lines).lower()) + + def test_empty_description_fails(self): + ok, _ = self.evaluate("") + self.assertFalse(ok) + + def test_missing_description_fails(self): + ok, _ = self.evaluate(None) + self.assertFalse(ok) + + def test_prose_mention_alone_fails(self): + ok, lines = self.evaluate("Builds on #1790 to finish the export.") + self.assertFalse(ok) + self.assertTrue(any("Closes #N" in line for line in lines)) + + def test_closing_keyword_passes(self): + ok, lines = self.evaluate("Fixes #1800") + self.assertTrue(ok) + self.assertIn("#1800", " ".join(lines)) + + def test_non_closing_link_passes(self): + ok, _ = self.evaluate("Part of #1487") + self.assertTrue(ok) + + def test_branch_issue_closed_by_the_description_passes(self): + ok, _ = self.evaluate( + "Closes #1800", branch="ericgriffin/github-issue-1800-6a78c2" + ) + self.assertTrue(ok) + + def test_branch_issue_referenced_without_closing_passes(self): + # A follow-up PR on the same branch, like #1742 after #1733. + ok, _ = self.evaluate( + "Refs #1729", branch="ericgriffin/github-issue-1729-55db0b" + ) + self.assertTrue(ok) + + def test_branch_issue_missing_from_the_description_fails(self): + ok, lines = self.evaluate( + "Closes #1801", branch="ericgriffin/github-issue-1800-6a78c2" + ) + self.assertFalse(ok) + self.assertTrue(any("#1800" in line for line in lines)) + + def test_untouched_pull_request_template_fails(self): + # The template's examples live in HTML comments, so a description left + # as the bare template must not satisfy the check by accident. + with open(TEMPLATE, encoding="utf-8") as fh: + ok, _ = self.evaluate(fh.read()) + self.assertFalse(ok) + + +class MainTest(unittest.TestCase): + def run_main(self, body, *extra): + with tempfile.TemporaryDirectory() as tmp: + path = os.path.join(tmp, "body.md") + with open(path, "w", encoding="utf-8") as fh: + fh.write(body) + out = io.StringIO() + with contextlib.redirect_stdout(out): + code = guard.main( + ["check_pr_issue_link.py", "--body-file", path, + "--repo", REPO, *extra] + ) + return code, out.getvalue() + + def test_linked_description_exits_zero(self): + code, out = self.run_main("Closes #10", "--branch", "feature/x") + self.assertEqual(code, 0) + self.assertIn("PASS", out) + + def test_unlinked_description_exits_one_with_an_error_annotation(self): + code, out = self.run_main("No issue here.", "--branch", "feature/x") + self.assertEqual(code, 1) + self.assertIn("::error::", out) + + def test_bot_author_type_exits_zero(self): + code, _ = self.run_main("", "--author-type", "Bot") + self.assertEqual(code, 0) + + def test_unreadable_body_file_exits_one(self): + out = io.StringIO() + with contextlib.redirect_stdout(out): + code = guard.main( + ["check_pr_issue_link.py", "--body-file", "/nonexistent/body.md"] + ) + self.assertEqual(code, 1) + self.assertIn("::error::", out.getvalue()) + + +if __name__ == "__main__": + unittest.main() From a59f5f844fdb76675d37b718d2b9212a4d33c56e Mon Sep 17 00:00:00 2001 From: Eric Griffin Date: Sun, 13 Sep 2026 00:20:26 -0400 Subject: [PATCH 2/2] fix(ci): run PR Issue Link from main and keep text after comments 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 ` Closes #1800` did not count as a link. --- .github/workflows/pr-issue-link.yml | 16 +++++++++++++++- scripts/check_pr_issue_link.py | 19 ++++++++++++++----- scripts/check_pr_issue_link_test.py | 11 +++++++++++ 3 files changed, 40 insertions(+), 6 deletions(-) diff --git a/.github/workflows/pr-issue-link.yml b/.github/workflows/pr-issue-link.yml index 25213d3c36..34b2cf6b1c 100644 --- a/.github/workflows/pr-issue-link.yml +++ b/.github/workflows/pr-issue-link.yml @@ -12,9 +12,20 @@ name: PR Issue Link # required status check ("PR Issue Link") next to CI Success in main's branch # protection. No `paths` filter: a required check that never reports leaves # the PR waiting forever. +# +# pull_request_target, not pull_request: under pull_request the workflow and +# the script come from the PR's merge ref, so a PR could edit either to report +# green and walk through its own gate. pull_request_target runs both as they +# exist on main, and its check run still lands on the PR's head commit, where +# branch protection looks for it. That trigger is safe here only because this +# job never checks out or executes PR code: the checkout below is main, and +# the description is read through the API as data. Keep it that way. Two +# consequences: a PR that changes the rule is judged by main's copy until it +# merges (Script Tests still runs its own unit tests), and the PR that first +# adds this file gets no run. on: - pull_request: + pull_request_target: branches: [main] types: [opened, edited, reopened, synchronize] @@ -32,9 +43,12 @@ jobs: runs-on: ubuntu-latest timeout-minutes: 5 steps: + # No `ref:`: under pull_request_target the default is main. Never point + # this at the PR head; that would run untrusted code with this token. - uses: actions/checkout@v7 with: sparse-checkout: scripts + persist-credentials: false - name: Fetch the current PR description # Read live rather than from the event payload: re-running a failed diff --git a/scripts/check_pr_issue_link.py b/scripts/check_pr_issue_link.py index 28a4457d67..95095f9caa 100644 --- a/scripts/check_pr_issue_link.py +++ b/scripts/check_pr_issue_link.py @@ -95,8 +95,9 @@ def _strip_blocks(text): Block structure is decided before inline markup, so a ``. - An unclosed fence or comment block runs to the end of the text. + line (it may interrupt a paragraph) and ends at the `-->`; any text after + the closer stays. An unclosed fence or comment block runs to the end of + the text. """ kept, fence, in_comment = [], None, False for line in text.splitlines(): @@ -105,7 +106,11 @@ def _strip_blocks(text): fence = None continue if in_comment: - in_comment = "-->" not in line + end = line.find("-->") + if end >= 0: + in_comment = False + # Text after the closer is visible, so it can hold a link. + kept.append(line[end + 3:]) continue opening = _FENCE_OPEN.match(line) # A backtick fence's info string cannot contain a backtick, so a line @@ -115,9 +120,13 @@ def _strip_blocks(text): ): fence = opening.group(1) continue - if _COMMENT_BLOCK_OPEN.match(line): - in_comment = "-->" not in line[line.index("" not in line[line.index(""), set()) self.assertEqual(closing("\nFixes #4"), {"#4"}) + def test_text_after_a_one_line_comment_is_kept(self): + self.assertEqual(closing(" Closes #1800"), {"#1800"}) + self.assertEqual( + closing(" Closes #1 fixes #3"), + {"#1", "#3"}, + ) + + def test_text_after_a_multi_line_comment_closes_is_kept(self): + body = " Closes #1800" + self.assertEqual(closing(body), {"#1800"}) + def test_an_unterminated_html_comment_hides_the_rest(self): self.assertEqual(closing("Fixes #4\n