From 760f712aafc93789e7d31ebd0fe147495925d36c Mon Sep 17 00:00:00 2001 From: Cemil ILIK Date: Tue, 20 Oct 2026 14:20:06 +0300 Subject: [PATCH 1/6] chore(ci): enforce issue dependencies with a blocked label and a required PR check A dependency between issues was recorded only in prose, so nothing kept a contributor from claiming or opening a pull request for work whose prerequisite was still open, and nothing said when it opened up. - tools/issue_dependencies.py derives the `blocked` label from GitHub's native "Blocked by" links. A blocker is resolved only when it is closed as completed; one closed as not planned or as a duplicate keeps the dependent blocked and flags it `needs-triage` with one notice. Removing the label posts one comment and re-runs the failed check of the open pull requests that link the issue. - .github/workflows/issue-dependencies.yml runs it when an issue is closed or reopened, hourly (GitHub sends no event for link edits) and on demand. - .github/workflows/pr-dependencies.yml adds the "Issue dependencies" check: it fails while a linked issue (Fixes / Closes / Resolves / Refs) is blocked, unless the pull request closes every open blocker too or a maintainer sets `dependency-override`. It runs the base commit's copy of the script, never the pull request's, with a read-only token. - /assign refuses a blocked issue (the label, or an open blocker GitHub still counts, so the hourly lag leaves no gap). - CONTRIBUTING gains "Dependencies between issues"; CLAUDE.md, AGENTS.md, the PR template and the CHANGELOG describe the rule. Fixes #1034 Co-Authored-By: Claude Opus 5.5 --- .github/pull_request_template.md | 4 +- .github/workflows/issue-dependencies.yml | 53 +++ .github/workflows/pr-dependencies.yml | 41 ++ AGENTS.md | 6 + CHANGELOG.md | 6 + CLAUDE.md | 6 + CONTRIBUTING.md | 19 +- tests/test_assign_on_comment.py | 44 +- tests/test_issue_dependencies.py | 478 +++++++++++++++++++ tools/assign_on_comment.py | 19 +- tools/issue_dependencies.py | 576 +++++++++++++++++++++++ 11 files changed, 1246 insertions(+), 6 deletions(-) create mode 100644 .github/workflows/issue-dependencies.yml create mode 100644 .github/workflows/pr-dependencies.yml create mode 100644 tests/test_issue_dependencies.py create mode 100644 tools/issue_dependencies.py diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index d5788d04..b37d0d56 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -6,7 +6,9 @@ + (CR-…, DOC-…, TECH-…) you address. Write "None" if there is no issue. + An issue labelled `blocked` waits for another one: the "Issue dependencies" check fails until + its blockers are closed as completed, unless this PR closes them too (`Fixes #…` for each). --> Fixes # ## Changes diff --git a/.github/workflows/issue-dependencies.yml b/.github/workflows/issue-dependencies.yml new file mode 100644 index 00000000..1ef5211a --- /dev/null +++ b/.github/workflows/issue-dependencies.yml @@ -0,0 +1,53 @@ +name: Issue dependencies + +# Keeps the `blocked` label in step with each issue's native "Blocked by" links (the rules live in +# tools/issue_dependencies.py and CONTRIBUTING.md, "Dependencies between issues"): +# - an open issue with a blocker that is not closed as completed gets `blocked`; +# - every other issue loses it, with one comment when an open issue is unblocked; +# - a blocker closed as not planned or as a duplicate leaves the issue blocked and asks a maintainer to re-plan it; +# - an unblocked issue's open pull requests get their failed "Issue dependencies" check re-run. +# +# GitHub sends no workflow event when a "Blocked by" link is added or removed, so this runs when an issue is closed +# or reopened (what unblocks or re-blocks its dependents), hourly as the backstop for link edits, and on demand. +# Every run is a full reconciliation, so the concurrency group may drop a pending run: the one that replaces it +# does the same work. tests/test_issue_dependencies.py pins these properties: +# - the token holds `contents: read`, `issues: write`, `pull-requests: read` and `actions: write`, nothing else; +# - no `${{ }}` appears in a `run:` line; the script reads the environment itself; +# - the checkout takes the default branch, one file, no stored credentials, pinned to a full commit SHA. +# +# Maintainers: to stop it, open Actions > "Issue dependencies" > the "..." menu > "Disable workflow". The links +# stay; only the label stops following them. + +on: + issues: + types: [closed, reopened] + schedule: + - cron: "17 * * * *" + workflow_dispatch: + +permissions: {} + +concurrency: + group: issue-dependencies + cancel-in-progress: false + +jobs: + sync: + if: ${{ github.repository == 'HodeTech/ForgeLM' }} + runs-on: ubuntu-latest + timeout-minutes: 10 + permissions: + contents: read # the checkout reads one script from the default branch + issues: write # add or remove `blocked` / `needs-triage`, and post the notices + pull-requests: read # find the open pull requests that link an unblocked issue + actions: write # re-run their failed dependency check + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false + sparse-checkout: tools/issue_dependencies.py + sparse-checkout-cone-mode: false + - name: Bring the blocked label in step with the Blocked-by links + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + run: python3 tools/issue_dependencies.py sync diff --git a/.github/workflows/pr-dependencies.yml b/.github/workflows/pr-dependencies.yml new file mode 100644 index 00000000..0a45ccbc --- /dev/null +++ b/.github/workflows/pr-dependencies.yml @@ -0,0 +1,41 @@ +name: PR dependencies + +# The required "Issue dependencies" check: fails while the pull request's description links (Fixes / Closes / +# Resolves / Refs #N) an issue that is still blocked, unless the same pull request closes every open blocker or a +# maintainer has labelled it `dependency-override`. Rules: tools/issue_dependencies.py and CONTRIBUTING.md, +# "Dependencies between issues". When a blocker is closed, the "Issue dependencies" workflow re-runs this check. +# +# The pull request's code never runs here: the checkout takes the base commit's copy of the script, so a pull +# request cannot change the check that judges it. The token is read-only, which is all a fork's pull request gets +# anyway. tests/test_issue_dependencies.py pins these properties: +# - `pull_request` only (never `pull_request_target`), no `paths:` filter (a required check must always report); +# - the token holds `contents: read`, `issues: read` and `pull-requests: read`, nothing else; +# - the checkout takes the base SHA, one file, no stored credentials, pinned to a full commit SHA; +# - no `${{ }}` appears in a `run:` line; the script reads the event file itself. + +on: + pull_request: + types: [opened, edited, reopened, synchronize, ready_for_review, labeled, unlabeled] + +permissions: {} + +jobs: + issue-dependencies: + name: Issue dependencies + runs-on: ubuntu-latest + timeout-minutes: 5 + permissions: + contents: read # the checkout reads one script from the base commit + issues: read # read the linked issues and their "Blocked by" links + pull-requests: read # read the pull request's current description and labels + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + ref: ${{ github.event.pull_request.base.sha }} + persist-credentials: false + sparse-checkout: tools/issue_dependencies.py + sparse-checkout-cone-mode: false + - name: Fail if a linked issue is still blocked + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + run: python3 tools/issue_dependencies.py check-pr diff --git a/AGENTS.md b/AGENTS.md index 1b8fd74a..c6d00590 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -387,6 +387,12 @@ Work on ForgeLM is planned and tracked on GitHub, not in local notes: security vulnerability — never a public issue; it goes to a private advisory as [SECURITY.md](SECURITY.md) describes. - **Closing is not triage.** Every item is an issue; which issues appear on the front door is decided by labels and views, never by closing them. +- **Dependencies are links, not prose.** When an issue cannot start before + another is done, add a **Blocked by** link in its sidebar. The `blocked` + label, `/assign` and the required **Issue dependencies** pull-request check + all follow those links (`tools/issue_dependencies.py`; CONTRIBUTING.md, + *Dependencies between issues*). Never set or remove `blocked` by hand, and do + not start work on a `blocked` issue. - **Name what you resolved.** For a grouped issue, list in the pull request the finding IDs your change resolves, so each can be ticked. diff --git a/CHANGELOG.md b/CHANGELOG.md index 7680d5c4..3923bd44 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,6 +27,12 @@ All notable changes to ForgeLM are documented here. when nobody holds the issue, refuses an issue that already has an assignee without changing it, allows at most 2 open assignments per contributor (maintainers are exempt) and leaves `security`, `needs-triage`, `deferred`, epic and Phase 16 issues to a maintainer. Reassigning stays a maintainer's decision. +- Dependencies between issues are enforced. A dependency is a native "Blocked by" link; the `Issue dependencies` + workflow (`tools/issue_dependencies.py`) labels every open issue with an unresolved blocker `blocked` and removes + the label, with one comment, once every blocker is closed as completed. A blocker closed as not planned or as a + duplicate keeps the issue blocked and flags it `needs-triage`. `/assign` refuses a blocked issue, and the new + required `Issue dependencies` check fails a pull request that links one (`Fixes` / `Closes` / `Resolves` / `Refs`) + unless it closes the blockers too or a maintainer sets `dependency-override` (#1034). - `GOVERNANCE.md` and `MAINTAINERS.md` at the repository root, recording how decisions are made, how the maintainer roles work, and which areas the current maintainer owns (#964). diff --git a/CLAUDE.md b/CLAUDE.md index b934017e..8c74caa9 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -387,6 +387,12 @@ Work on ForgeLM is planned and tracked on GitHub, not in local notes: security vulnerability — never a public issue; it goes to a private advisory as [SECURITY.md](SECURITY.md) describes. - **Closing is not triage.** Every item is an issue; which issues appear on the front door is decided by labels and views, never by closing them. +- **Dependencies are links, not prose.** When an issue cannot start before + another is done, add a **Blocked by** link in its sidebar. The `blocked` + label, `/assign` and the required **Issue dependencies** pull-request check + all follow those links (`tools/issue_dependencies.py`; CONTRIBUTING.md, + *Dependencies between issues*). Never set or remove `blocked` by hand, and do + not start work on a `blocked` issue. - **Name what you resolved.** For a grouped issue, list in the pull request the finding IDs your change resolves, so each can be ticked. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index ba4fa056..3968019c 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -68,7 +68,7 @@ Thanks for your interest in contributing! ForgeLM is an open-source project and Work is tracked in two places that play different roles: - **Issues are where the work happens.** Each issue is one unit of work: it describes the problem, links the code, and lists acceptance criteria. Discussion, claiming and pull-request links all happen on the issue. -- **The [ForgeLM Project Plan](https://github.com/orgs/HodeTech/projects/5) is where the work is planned.** It shows every issue with its priority, release target and iteration, in several views: *Backlog* (table), *Board* (by status), *Current iteration* and *Roadmap* (timeline), plus filtered views such as *Front door*, *Small tasks*, *Epics*, *Docs & TR mirror* and *Roadmap — Phase 16 & deferred*. Use it to choose what to work on; you never need to edit it — maintainers keep it current, and a card moves automatically when its issue is closed. +- **The [ForgeLM Project Plan](https://github.com/orgs/HodeTech/projects/5) is where the work is planned.** It shows every issue with its priority, release target and iteration, in several views: *Backlog* (table), *Board* (by status), *Current iteration* and *Roadmap* (timeline), plus filtered views such as *Front door*, *Small tasks*, *Epics*, *Docs & TR mirror*, *Roadmap — Phase 16 & deferred* and *Blocked*. Use it to choose what to work on; you never need to edit it — maintainers keep it current, and a card moves automatically when its issue is closed. ### Planned work lives here too @@ -93,6 +93,8 @@ Issues carry labels, and the project adds planning fields on top of them. | `source: roadmap` | Planned work recorded in [`docs/roadmap/`](docs/roadmap/): a roadmap step or a deferred item | | `phase: 16` | Part of [Phase 16](docs/roadmap/phase-16-trust-surface-hardening.md): one of its remaining steps, one of its two prerequisite standards, or the phase epic. Deferred items are not part of the phase and do not carry it | | `deferred` | Waits for a condition recorded in the issue; check that it holds before starting. A deferred item is labelled `source: roadmap` and `deferred` | +| `blocked` | Waits for another issue, listed under *Blocked by* in the sidebar. Set and removed automatically, never by hand; do not start it — see [Dependencies between issues](#dependencies-between-issues) | +| `dependency-override` | On a pull request: a maintainer accepts that it works on a blocked issue | | `epic` | A parent issue that groups one theme or roadmap phase; its sub-issues are the work | In the project, the **Wave**, **Severity**, **Theme**, **Kind**, **Area**, **Size** (effort: XS–XL) and **Iteration** fields let you filter and group the same issues. **Milestones** are the waves: waves 0–2 carry a target date at the end of their last iteration, and waves 3–4 are open-ended backlog. @@ -111,6 +113,7 @@ Useful starting points: - `/assign` and `/unassign` act on the person who writes them, and the command must be the first thing in the comment; they cannot name someone else. If two people claim an issue in the same second, the maintainer's claim wins, and between two contributors one claim is released again and its author is told so. - Anything the automation does not cover stays a maintainer's call: an issue labelled `help wanted` without `good first issue` (grouped sweeps are taken one finding ID at a time, see below), an issue labelled `needs-triage`, `deferred`, `security`, `epic` or `phase: 16`, and a request for more than 2 open issues. Comment on the issue, say what you would take on and mention a maintainer ([MAINTAINERS.md](MAINTAINERS.md)). A plain comment such as "I would like to work on this" is read by a maintainer too, who assigns it by hand. - Skip issues labelled `needs-triage`, and check the recorded condition of an issue labelled `deferred` before you start. + - Skip issues labelled `blocked` until the label is gone: they wait for another issue, and `/assign` refuses them whatever their other labels (see [Dependencies between issues](#dependencies-between-issues)). 2. **Re-check against `development`.** Issues from the review link code at the commit they were found on; the links stay valid, but line numbers may have moved. Confirm the problem still exists on `development` before fixing it. 3. **Follow the acceptance criteria** in the issue. In particular, a code fix carries a regression test that reproduces the described failure, and a documentation fix updates the English page and its Turkish mirror (`-tr.md`, or the `tr/` tree for user manuals) in the same pull request. 4. **Reference the issue in your pull request.** Use `Fixes #123` when the PR resolves the whole issue, and `Refs #123` when it resolves only part of it. @@ -121,6 +124,18 @@ Useful starting points: **Finding IDs.** Issues created from the review name their findings with stable IDs — `CR-…` for code, `DOC-…` for documentation (including the Turkish mirror), `TECH-…` for dependency and ecosystem currency. Quote the ID in commits and pull requests so the finding can be traced. +### Dependencies between issues + +Some issues can only be done after another one: each Phase 16 step waits for the step before it, a deferred item may wait for the step that changes the same file, and a review issue whose every finding is a unit of a Phase 16 step waits for that step, whose pull request fixes it. Such a dependency is recorded as a **Blocked by** link in the issue's sidebar (*Relationships*), never only in prose. + +- **An issue with an open blocker is labelled `blocked`.** Do not start on it and do not open a pull request for it: `/assign` refuses it, and the required **Issue dependencies** check fails a pull request whose description links it with `Fixes`, `Closes`, `Resolves` or `Refs #…`. +- **The label is automatic.** Nobody sets or removes it by hand. The *Issue dependencies* workflow adds it while a blocker is open and removes it once every blocker is closed as completed — as soon as that blocker is closed, or within an hour of a link being added or removed. The issue then says so in a comment, and the failed check of an open pull request that links it is re-run. +- **A blocker closed as not planned or as a duplicate unblocks nothing**, because the work the issue waits for was not done. The issue stays `blocked`, gets `needs-triage`, and a maintainer re-plans it: removes the link, or links the issue that replaces the blocker. +- **A pull request may close the blocker as well.** When it closes every open blocker of the issues it links (`Fixes #…` for each), the check passes. A maintainer can also accept a dependency deliberately with the `dependency-override` label. +- **Grouped issues are not blocked as a whole** when only some of their findings belong to a Phase 16 step. Those findings say so in the checklist (`KNOWN-OPEN … (S8 open)`): leave them to the step and take the others. + +The project's *Blocked* view lists every blocked issue; *Front door* and *Small tasks* leave them out. + ## Reporting Security Vulnerabilities Please **do not open a public issue** for a security vulnerability, and do not post exploit details in comments. Report it privately through GitHub: on the repository's **Security** tab choose **Report a vulnerability**. [`SECURITY.md`](SECURITY.md) explains what to include, what counts as a vulnerability, and how disclosure works. Maintainers track confirmed vulnerabilities in private security advisories and publish them after a fixed release is available, so some planned security work is intentionally not visible in the public issue list. @@ -238,7 +253,7 @@ keep this list and that one in sync if either changes. Push your branch to your fork and open a Pull Request against `development` (the default base). Link the issue it addresses (`Fixes #123`, or `Refs #123` for part of a grouped issue) and quote any finding IDs — see [Working on an issue](#working-on-an-issue). -**Before a pull request can merge**, branch protection on `development` (and on `main`) requires three things: the six CI checks — `lint`, `test (3.10)`, `test (3.11)`, `test (3.12)`, `test (3.13)` and `validate` — passing; the branch up to date with its target (use *Update branch*); and every review conversation resolved, including the comments of the automated reviewers. +**Before a pull request can merge**, branch protection on `development` (and on `main`) requires three things: the six CI checks — `lint`, `test (3.10)`, `test (3.11)`, `test (3.12)`, `test (3.13)` and `validate` — passing, and on `development` also the **Issue dependencies** check (see [Dependencies between issues](#dependencies-between-issues)); the branch up to date with its target (use *Update branch*); and every review conversation resolved, including the comments of the automated reviewers. Maintainers **squash contributor PRs into `development` by default**. Releases into `main` and synchronization PRs from `main` back into `development` use **merge diff --git a/tests/test_assign_on_comment.py b/tests/test_assign_on_comment.py index c6931ab8..1008d182 100644 --- a/tests/test_assign_on_comment.py +++ b/tests/test_assign_on_comment.py @@ -53,14 +53,18 @@ def __init__( self.human_comments: List[str] = [] # comments by people (the bot's own are in `comments`) self.assigned_pages: Optional[List[List[Dict[str, Any]]]] = None # overrides the count answer, page by page self.refuse_delete = False # accept the DELETE but keep the assignee + self.open_blockers: Optional[int] = 0 # GitHub's count of open "Blocked by" issues; None drops the summary self._reads = 0 def _issue(self) -> Dict[str, Any]: - return { + issue: Dict[str, Any] = { "state": self.state, "labels": [{"name": n} for n in self.labels], "assignees": [{"login": a} for a in self.assignees], } + if self.open_blockers is not None: + issue["issue_dependencies_summary"] = {"blocked_by": self.open_blockers, "total_blocked_by": 3} + return issue def __call__(self, method: str, endpoint: str, payload: Any = None) -> Any: self.calls.append((method, endpoint, payload)) @@ -339,6 +343,44 @@ def test_a_closed_issue_cannot_be_claimed() -> None: assert gh.assignees == [] and gh.comments == [tool.MSG_CLOSED] +# --- dependencies --------------------------------------------------------------------------------------------- + + +def test_a_blocked_good_first_issue_cannot_be_claimed_and_says_why_without_pinging() -> None: + gh = FakeGitHub(labels=["good first issue", "blocked"]) + assert run(gh) == 0 + assert gh.assignees == [] and gh.writes() == [] + assert gh.comments == [tool.MSG_BLOCKED.format(front_door=tool.FRONT_DOOR_URL)] + assert tool.MAINTAINER_MENTION not in gh.comments[0] # nothing to decide: it opens by itself + + +def test_an_open_blocker_refuses_the_claim_before_the_label_has_caught_up() -> None: + gh = FakeGitHub(labels=["good first issue"]) + gh.open_blockers = 1 # linked a minute ago; the hourly sync has not labelled it yet + assert run(gh) == 0 + assert gh.assignees == [] and gh.comments[0].startswith("This issue is blocked") + + +def test_blockers_that_are_all_closed_do_not_refuse_the_claim() -> None: + gh = FakeGitHub(labels=["good first issue"]) + gh.open_blockers = 0 # `total_blocked_by` stays 3: closed blockers no longer count + assert run(gh) == 0 + assert gh.assignees == ["newcomer"] + + +def test_without_a_dependency_summary_the_label_alone_decides() -> None: + free, blocked = FakeGitHub(labels=["good first issue"]), FakeGitHub(labels=["good first issue", "blocked"]) + free.open_blockers = blocked.open_blockers = None + assert run(free) == 0 and free.assignees == ["newcomer"] + assert run(blocked) == 0 and blocked.assignees == [] + + +def test_a_blocked_issue_can_still_be_released() -> None: + gh = FakeGitHub(labels=["good first issue", "blocked"], assignees=["first"]) + assert run(gh, "/unassign", login="first") == 0 + assert gh.assignees == [] + + # --- the per-person limit ------------------------------------------------------------------------------------- diff --git a/tests/test_issue_dependencies.py b/tests/test_issue_dependencies.py new file mode 100644 index 00000000..1b16d97a --- /dev/null +++ b/tests/test_issue_dependencies.py @@ -0,0 +1,478 @@ +"""Tests for tools/issue_dependencies.py and its two workflows. + +The tool derives the ``blocked`` label from GitHub's native "Blocked by" links and fails a pull request that works +on a blocked issue (CONTRIBUTING.md, "Dependencies between issues"). These tests pin the rules, the parsing of +pull-request descriptions written by anyone, the write paths of ``sync`` against a fake API, the fixed-endpoint +discipline of the GitHub calls, and the security posture of both workflow files, since no other guard reads them. +""" + +from __future__ import annotations + +import json +import re +import sys +from pathlib import Path +from typing import Any, Dict, List, Optional, Tuple + +import pytest +import yaml + +ROOT = Path(__file__).resolve().parent.parent +sys.path.insert(0, str(ROOT / "tools")) + +import assign_on_comment # noqa: E402 +import issue_dependencies as tool # noqa: E402 + +REPO = "HodeTech/ForgeLM" + + +def blocker(number: int, state: str = "OPEN", reason: Optional[str] = None, repo: str = REPO) -> tool.Blocker: + return tool.Blocker(number, repo, state, reason) + + +def issue(number: int, *blockers: tool.Blocker, labels: Tuple[str, ...] = (), is_open: bool = True) -> tool.IssueState: + return tool.IssueState(number, is_open, frozenset(labels), tuple(blockers)) + + +# --- the rules ------------------------------------------------------------------------------------------------ + + +@pytest.mark.parametrize( + "state, reason, status", + [ + ("OPEN", None, "waiting"), + ("OPEN", "REOPENED", "waiting"), + ("CLOSED", "COMPLETED", "resolved"), + ("CLOSED", None, "resolved"), # closed before GitHub recorded reasons: closing meant completed + ("CLOSED", "NOT_PLANNED", "not-completed"), + ("CLOSED", "DUPLICATE", "not-completed"), + ], +) +def test_a_blocker_is_resolved_only_when_closed_as_completed(state: str, reason: Optional[str], status: str) -> None: + assert blocker(1, state, reason).status == status + + +def test_an_open_blocker_labels_the_issue_without_a_comment() -> None: + plan = tool.plan_issue(issue(5, blocker(1)), REPO) + assert plan == tool.Plan(add_label=True) + + +def test_an_issue_already_labelled_and_still_waiting_changes_nothing() -> None: + plan = tool.plan_issue(issue(5, blocker(1), labels=("blocked",)), REPO) + assert not plan.changes + + +def test_one_open_blocker_among_closed_ones_keeps_the_issue_blocked() -> None: + plan = tool.plan_issue(issue(5, blocker(1, "CLOSED", "COMPLETED"), blocker(2)), REPO) + assert plan.add_label and not plan.remove_label + + +def test_when_every_blocker_is_completed_the_label_goes_and_one_comment_names_them() -> None: + done = (blocker(1, "CLOSED", "COMPLETED"), blocker(9, "CLOSED", "COMPLETED", repo="HodeTech/Other")) + plan = tool.plan_issue(issue(5, *done, labels=("blocked", "wave: 3")), REPO) + assert plan.remove_label and not plan.add_label + assert "(#1, HodeTech/Other#9)" in plan.comment and plan.comment.startswith("This issue is no longer blocked") + + +def test_a_hand_set_label_without_links_is_removed_with_the_explanation() -> None: + plan = tool.plan_issue(issue(5, labels=("blocked",)), REPO) + assert plan.remove_label and "set and removed automatically" in plan.comment + + +def test_an_issue_that_was_never_blocked_is_left_alone() -> None: + assert not tool.plan_issue(issue(5), REPO).changes + assert not tool.plan_issue(issue(5, blocker(1, "CLOSED", "COMPLETED")), REPO).changes + + +def test_a_closed_issue_loses_the_label_silently() -> None: + plan = tool.plan_issue(issue(5, blocker(1), labels=("blocked",), is_open=False), REPO) + assert plan == tool.Plan(remove_label=True) + + +@pytest.mark.parametrize("reason", ["NOT_PLANNED", "DUPLICATE"]) +def test_a_blocker_closed_without_being_completed_keeps_the_issue_blocked_and_asks_for_triage(reason: str) -> None: + gone = blocker(1, "CLOSED", reason) + plan = tool.plan_issue(issue(5, gone, labels=("blocked",)), REPO) + assert not plan.remove_label and plan.not_completed == (gone,) + notice = tool.not_completed_notice(gone) + assert ("a duplicate" if reason == "DUPLICATE" else "not planned") in notice + assert tool.CANCELLED_MARKER.format(blocker=1) in notice and tool.MAINTAINER_MENTION in notice + + +# --- reading pull-request descriptions ------------------------------------------------------------------------- + + +@pytest.mark.parametrize( + "body, closing, every", + [ + ("Fixes #12", {12}, {12}), + ("fixed #12, closes #13 and resolves #14", {12, 13, 14}, {12, 13, 14}), + ("Resolves: #12", {12}, {12}), + ("Fixes #12, #13 and #14", {12, 13, 14}, {12, 13, 14}), + ("Refs #12", set(), {12}), + ("references #12 & #13", set(), {12, 13}), + ("Fixes HodeTech/ForgeLM#12", {12}, {12}), + ("Fixes https://github.com/HodeTech/ForgeLM/issues/12", {12}, {12}), + ("Fixes other/repo#12 and https://github.com/other/repo/issues/13", set(), set()), + ("FIXES #12", {12}, {12}), + ("Fixes#12", {12}, {12}), # GitHub would not close it; a blocking check errs on the safe side + ("See #12 for context; prefixes #13 are not keywords", set(), set()), + ("\n`Refs #13`\n```\nFixes #14\n```\nRefs #15", set(), {15}), + ], +) +def test_linked_issues(body: str, closing: set, every: set) -> None: + assert tool.linked_issues(body, REPO) == (frozenset(closing), frozenset(every)) + + +def test_the_pull_request_template_itself_links_nothing() -> None: + template = (ROOT / ".github/pull_request_template.md").read_text(encoding="utf-8") + assert tool.linked_issues(template, REPO) == (frozenset(), frozenset()) + + +# --- the pull-request rule ------------------------------------------------------------------------------------- + + +def verdict( + body: str, links: Dict[int, Optional[Tuple[tool.Blocker, ...]]], labels: Tuple[str, ...] = () +) -> tool.Verdict: + closing, linked = tool.linked_issues(body, REPO) + return tool.check_pull_request(frozenset(labels), closing, linked, lambda n: links.get(n, ()), REPO) + + +def test_a_pull_request_on_a_blocked_issue_fails_and_names_the_blocker() -> None: + v = verdict("Fixes #5", {5: (blocker(1),)}) + assert not v.ok and v.lines[0] == "#5 is blocked by #1, which is still open." + + +def test_refs_counts_as_working_on_the_issue() -> None: + assert not verdict("Refs #5", {5: (blocker(1),)}).ok + + +def test_closing_the_blocker_in_the_same_pull_request_passes() -> None: + assert verdict("Fixes #1\nFixes #5", {5: (blocker(1),), 1: ()}).ok + + +def test_only_referring_to_the_blocker_does_not_count_as_closing_it() -> None: + assert not verdict("Fixes #5, refs #1", {5: (blocker(1),), 1: ()}).ok + + +def test_a_blocker_closed_without_being_completed_fails_even_if_named() -> None: + v = verdict("Fixes #5\nFixes #1", {5: (blocker(1, "CLOSED", "NOT_PLANNED"),)}) + assert not v.ok and "closed without being completed" in v.lines[0] + + +def test_completed_blockers_pull_requests_and_closed_issues_pass() -> None: + v = verdict("Fixes #5, #6", {5: (blocker(1, "CLOSED", "COMPLETED"),), 6: None}) + assert v.ok and v.lines == ("No linked issue is blocked (1 issue(s) checked).",) + + +def test_the_override_label_passes_and_says_so() -> None: + v = verdict("Fixes #5", {5: (blocker(1),)}, labels=("dependency-override",)) + assert v.ok and "dependency-override" in v.lines[0] + + +# --- a fake GitHub API ----------------------------------------------------------------------------------------- + + +class FakeGitHub: + """Plays the endpoints the tool calls: GraphQL for issues and their links, REST for writes.""" + + def __init__(self, issues: Dict[int, Dict[str, Any]], closed_labelled: Tuple[int, ...] = ()) -> None: + self.issues = issues # number -> {"labels": [...], "blockers": [(n, state, reason), ...], "state": "OPEN"} + self.closed_labelled = list(closed_labelled) + self.calls: List[Tuple[str, str, Any]] = [] + self.comments: Dict[int, List[Dict[str, Any]]] = {} + self.pulls: List[Dict[str, Any]] = [] + self.runs: Dict[str, List[Dict[str, Any]]] = {} + self.fail_prefix: Optional[str] = None + + def _node(self, number: int) -> Dict[str, Any]: + i = self.issues[number] + nodes = [ + {"number": n, "state": s, "stateReason": r, "repository": {"nameWithOwner": REPO}} + for n, s, r in i["blockers"] + ] + return { + "__typename": i.get("type", "Issue"), + "number": number, + "state": i.get("state", "OPEN"), + "labels": {"nodes": [{"name": n} for n in i["labels"]]}, + "blockedBy": {"totalCount": len(nodes), "nodes": nodes}, + } + + def __call__(self, method: str, endpoint: str, payload: Any = None) -> Any: + self.calls.append((method, endpoint, payload)) + if self.fail_prefix and endpoint.startswith(self.fail_prefix): + raise tool.GhError(f"{method} {endpoint} failed") + if endpoint == "graphql": + assert payload["query"] in tool.QUERIES.values() + v = payload["variables"] + if "number" in v: + found = v["number"] in self.issues + return {"data": {"repository": {"issueOrPullRequest": self._node(v["number"]) if found else None}}} + open_nodes = [self._node(n) for n, i in sorted(self.issues.items()) if i.get("state", "OPEN") == "OPEN"] + return {"data": {"repository": {"issues": {"nodes": open_nodes, "pageInfo": {"hasNextPage": False}}}}} + if method == "GET" and endpoint.startswith(f"repos/{REPO}/issues?state=closed&labels=blocked"): + return [{"number": n} for n in self.closed_labelled] + if method == "GET" and re.fullmatch(rf"repos/{REPO}/issues/\d+/comments\?.*", endpoint): + return self.comments.get(int(endpoint.split("/")[4]), []) + if method == "POST" and endpoint.endswith("/labels"): + self.issues.setdefault(int(endpoint.split("/")[4]), {"labels": [], "blockers": []})["labels"] += payload[ + "labels" + ] + return [] + if method == "DELETE" and "/labels/" in endpoint: + n = int(endpoint.split("/")[4]) + if n in self.issues: + self.issues[n]["labels"].remove("blocked") + return None + if method == "POST" and endpoint.endswith("/comments"): + self.comments.setdefault(int(endpoint.split("/")[4]), []).append( + {"user": {"type": "Bot"}, "body": payload["body"]} + ) + return {} + if method == "GET" and endpoint.startswith(f"repos/{REPO}/pulls?state=open"): + return self.pulls + if method == "GET" and re.fullmatch(rf"repos/{REPO}/pulls/\d+", endpoint): + return next(p for p in self.pulls if p["number"] == int(endpoint.rsplit("/", 1)[1])) + if method == "GET" and "/actions/workflows/pr-dependencies.yml/runs?head_sha=" in endpoint: + return {"workflow_runs": self.runs.get(endpoint.split("head_sha=")[1].split("&")[0], [])} + if method == "POST" and endpoint.endswith("/rerun-failed-jobs"): + return None + raise AssertionError(f"unexpected call {method} {endpoint}") + + def writes(self) -> List[Tuple[str, str]]: + return [(m, e) for m, e, _ in self.calls if m in ("POST", "DELETE") and e != "graphql"] + + +SHA = "a" * 40 + + +def test_sync_labels_blocked_issues_and_unblocks_finished_ones() -> None: + gh = FakeGitHub( + { + 5: {"labels": [], "blockers": [(1, "OPEN", None)]}, + 6: {"labels": ["blocked"], "blockers": [(2, "CLOSED", "COMPLETED")]}, + 7: {"labels": ["blocked"], "blockers": [(1, "OPEN", None)]}, + 8: {"labels": [], "blockers": []}, + }, + closed_labelled=(40,), + ) + assert tool.run_sync(REPO, gh) == 0 + assert ( + gh.issues[5]["labels"] == ["blocked"] and gh.issues[6]["labels"] == [] and gh.issues[7]["labels"] == ["blocked"] + ) + assert set(gh.comments) == {6} and gh.comments[6][0]["body"].startswith("This issue is no longer blocked") + assert (("DELETE", f"repos/{REPO}/issues/40/labels/blocked")) in gh.writes() # a closed issue waits for nothing + assert 40 not in gh.comments + + +def test_a_dry_run_reads_but_never_writes(capsys: pytest.CaptureFixture[str]) -> None: + gh = FakeGitHub({5: {"labels": [], "blockers": [(1, "OPEN", None)]}, 6: {"labels": ["blocked"], "blockers": []}}) + assert tool.run_sync(REPO, gh, dry_run=True) == 0 + assert gh.writes() == [] and "would label #5 blocked" in capsys.readouterr().out + + +def test_a_cancelled_blocker_is_flagged_once_and_a_persons_copy_of_the_marker_does_not_silence_it() -> None: + gh = FakeGitHub({5: {"labels": ["blocked"], "blockers": [(1, "CLOSED", "NOT_PLANNED")]}}) + gh.comments[5] = [{"user": {"type": "User"}, "body": tool.CANCELLED_MARKER.format(blocker=1)}] + assert tool.run_sync(REPO, gh) == 0 + assert gh.issues[5]["labels"] == ["blocked", "needs-triage"] and len(gh.comments[5]) == 2 + assert tool.run_sync(REPO, gh) == 0 # the next run finds the bot's notice and stays quiet + assert len(gh.comments[5]) == 2 and gh.issues[5]["labels"].count("needs-triage") == 1 + + +def test_unblocking_reruns_the_failed_check_of_a_pull_request_that_links_the_issue() -> None: + gh = FakeGitHub({6: {"labels": ["blocked"], "blockers": [(2, "CLOSED", "COMPLETED")]}}) + gh.pulls = [ + {"number": 30, "body": "Fixes #6", "head": {"sha": SHA}}, + {"number": 31, "body": "Fixes #9", "head": {"sha": SHA}}, + ] + gh.runs[SHA] = [{"id": 77, "conclusion": "failure"}] + assert tool.run_sync(REPO, gh) == 0 + assert ("POST", f"repos/{REPO}/actions/runs/77/rerun-failed-jobs") in gh.writes() + assert sum(1 for m, e in gh.writes() if e.endswith("/rerun-failed-jobs")) == 1 + + +def test_a_failing_rerun_is_a_warning_not_a_failed_sync(capsys: pytest.CaptureFixture[str]) -> None: + gh = FakeGitHub({6: {"labels": ["blocked"], "blockers": []}}) + gh.fail_prefix = f"repos/{REPO}/pulls" + assert tool.run_sync(REPO, gh) == 0 + assert gh.issues[6]["labels"] == [] and "warning: could not re-run" in capsys.readouterr().err + + +def test_a_failing_read_fails_the_sync() -> None: + gh = FakeGitHub({}) + gh.fail_prefix = "graphql" + with pytest.raises(tool.GhError): + tool.run_sync(REPO, gh) + + +def test_more_links_than_one_page_is_an_error_not_a_silent_truncation() -> None: + node = {"number": 5, "blockedBy": {"totalCount": 51, "nodes": []}} + with pytest.raises(tool.GhError, match="more than 50 blockers"): + tool._blockers(node) + + +def test_check_pr_reads_the_pull_request_again_instead_of_trusting_the_event() -> None: + gh = FakeGitHub({5: {"labels": [], "blockers": [(1, "OPEN", None)]}, 1: {"labels": [], "blockers": []}}) + gh.pulls = [{"number": 30, "body": "Fixes #5", "labels": []}] + stale = {"pull_request": {"number": 30, "body": "Docs only", "labels": [{"name": "dependency-override"}]}} + assert tool.run_check_pr(stale, REPO, gh) == 3 + gh.pulls[0]["labels"] = [{"name": "dependency-override"}] + assert tool.run_check_pr(stale, REPO, gh) == 0 + + +def test_check_pr_skips_linked_pull_requests_and_closed_issues() -> None: + gh = FakeGitHub( + { + 5: {"labels": [], "blockers": [(1, "OPEN", None)], "type": "PullRequest"}, + 6: {"labels": [], "blockers": [(1, "OPEN", None)], "state": "CLOSED"}, + } + ) + gh.pulls = [{"number": 30, "body": "Refs #5, #6 and #404", "labels": []}] + assert tool.run_check_pr({"pull_request": {"number": 30}}, REPO, gh) == 0 + + +def test_check_pr_prints_errors_as_annotations_with_numbers_only(capsys: pytest.CaptureFixture[str]) -> None: + gh = FakeGitHub({5: {"labels": [], "blockers": [(1, "OPEN", None)]}}) + gh.pulls = [{"number": 30, "body": "Fixes #5 ::set-output name=x::y", "labels": []}] + assert tool.run_check_pr({"pull_request": {"number": 30}}, REPO, gh) == 3 + out = capsys.readouterr().out + assert out.startswith("::error::#5 is blocked by #1") and "set-output" not in out + + +# --- entry point and the GitHub call wrapper ------------------------------------------------------------------- + + +def test_main_rejects_a_missing_repository_a_missing_event_and_a_bad_command( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + gh = FakeGitHub({}) + monkeypatch.delenv("GITHUB_REPOSITORY", raising=False) + assert tool.main(["sync"], gh) == 2 + monkeypatch.setenv("GITHUB_REPOSITORY", REPO) + monkeypatch.delenv("GITHUB_EVENT_PATH", raising=False) + assert tool.main(["check-pr"], gh) == 2 + broken = tmp_path / "event.json" + broken.write_text(json.dumps({"pull_request": {"number": "30"}}), encoding="utf-8") + monkeypatch.setenv("GITHUB_EVENT_PATH", str(broken)) + assert tool.main(["check-pr"], gh) == 2 + assert tool.main(["frobnicate"], gh) == 2 + assert gh.calls == [] + + +def test_main_turns_an_api_failure_into_exit_one(monkeypatch: pytest.MonkeyPatch) -> None: + gh = FakeGitHub({}) + gh.fail_prefix = "graphql" + monkeypatch.setenv("GITHUB_REPOSITORY", REPO) + assert tool.main(["sync"], gh) == 1 + + +@pytest.mark.parametrize( + "method, endpoint, payload", + [ + ("GET", "user", None), + ("PATCH", f"repos/{REPO}/issues/1", {}), + ("GET", f"repos/{REPO}/issues/1 --jq .", None), + ("POST", "graphql", {"query": "mutation { deleteIssue }"}), + ("GET", "graphql", {"query": tool.QUERIES["issue"]}), + ], +) +def test_gh_api_refuses_anything_outside_the_fixed_endpoints(method: str, endpoint: str, payload: Any) -> None: + with pytest.raises(tool.GhError, match="refusing"): + tool.gh_api(method, endpoint, payload) + + +def test_gh_api_runs_a_list_command_without_a_shell(monkeypatch: pytest.MonkeyPatch) -> None: + seen: Dict[str, Any] = {} + + class Done: + returncode, stdout, stderr = 0, "[]", "" + + def fake_run(command: List[str], **kwargs: Any) -> Done: + seen.update(command=command, **kwargs) + return Done() + + monkeypatch.setattr(tool.subprocess, "run", fake_run) + assert tool.gh_api("GET", f"repos/{REPO}/issues/1") == [] + assert seen["command"] == ["gh", "api", "--method", "GET", f"repos/{REPO}/issues/1"] and "shell" not in seen + + +def test_gh_api_failure_text_is_flattened_and_defanged(monkeypatch: pytest.MonkeyPatch) -> None: + class Failed: + returncode, stdout, stderr = 1, "", "bad\n::error::injected" + + monkeypatch.setattr(tool.subprocess, "run", lambda *a, **k: Failed()) + with pytest.raises(tool.GhError) as info: + tool.gh_api("GET", f"repos/{REPO}/issues/1") + assert "\n" not in str(info.value) and "::error::" not in str(info.value) + + +def test_a_graphql_error_is_a_gherror() -> None: + with pytest.raises(tool.GhError, match="GraphQL issue failed"): + tool.graphql(lambda *a: {"errors": [{"message": "nope"}]}, "issue", owner="o", name="n", number=1) + + +def test_the_maintainer_list_matches_the_assign_tool() -> None: + assert tool.MAINTAINER_LOGINS == assign_on_comment.MAINTAINER_LOGINS # that one is checked against MAINTAINERS.md + + +def test_the_links_in_the_notices_resolve_to_contributing_headings() -> None: + text = (ROOT / "CONTRIBUTING.md").read_text(encoding="utf-8") + headings = { + re.sub(r"[^\w\- ]", "", h).strip().lower().replace(" ", "-") for h in re.findall(r"^#+ (.+)$", text, re.M) + } + for anchor in ("dependencies-between-issues", "working-on-an-issue"): + assert anchor in headings + + +# --- the workflow files ---------------------------------------------------------------------------------------- + + +def load(name: str) -> Dict[Any, Any]: + return yaml.safe_load((ROOT / ".github/workflows" / name).read_text(encoding="utf-8")) + + +def steps_of(workflow: Dict[Any, Any]) -> List[Dict[str, Any]]: + (job,) = workflow["jobs"].values() + return job["steps"] + + +@pytest.mark.parametrize("name", ["issue-dependencies.yml", "pr-dependencies.yml"]) +def test_no_expression_reaches_a_shell_and_the_checkout_is_pinned_and_minimal(name: str) -> None: + steps = steps_of(load(name)) + assert all("${{" not in str(step.get("run", "")) for step in steps) + checkout = steps[0] + assert re.fullmatch(r"actions/checkout@[0-9a-f]{40}", checkout["uses"]) + assert checkout["with"]["persist-credentials"] is False + assert checkout["with"]["sparse-checkout"] == "tools/issue_dependencies.py" + + +def test_the_sync_workflow_triggers_permissions_and_concurrency() -> None: + wf = load("issue-dependencies.yml") + assert wf[True] == { + "issues": {"types": ["closed", "reopened"]}, + "schedule": [{"cron": "17 * * * *"}], + "workflow_dispatch": None, + } # YAML 1.1 reads the key `on` as a boolean + assert wf["permissions"] == {} + job = wf["jobs"]["sync"] + assert job["permissions"] == {"contents": "read", "issues": "write", "pull-requests": "read", "actions": "write"} + assert wf["concurrency"] == {"group": "issue-dependencies", "cancel-in-progress": False} + assert "ref" not in steps_of(wf)[0]["with"] # the default branch: what `issues` and `schedule` run on + assert "github.repository == 'HodeTech/ForgeLM'" in job["if"] + assert steps_of(wf)[1]["run"] == "python3 tools/issue_dependencies.py sync" + + +def test_the_pull_request_check_never_runs_the_pull_requests_own_code() -> None: + wf = load("pr-dependencies.yml") + assert list(wf[True]) == ["pull_request"] # never pull_request_target + assert set(wf[True]["pull_request"]) == {"types"} # no paths/branches filter: a required check must report + assert wf["permissions"] == {} + job = wf["jobs"]["issue-dependencies"] + assert job["name"] == "Issue dependencies" # the required check's context + assert job["permissions"] == {"contents": "read", "issues": "read", "pull-requests": "read"} + assert steps_of(wf)[0]["with"]["ref"] == "${{ github.event.pull_request.base.sha }}" + assert steps_of(wf)[1]["run"] == "python3 tools/issue_dependencies.py check-pr" + assert {"edited", "labeled", "unlabeled", "synchronize"} <= set(wf[True]["pull_request"]["types"]) diff --git a/tools/assign_on_comment.py b/tools/assign_on_comment.py index 277597c7..563fb795 100644 --- a/tools/assign_on_comment.py +++ b/tools/assign_on_comment.py @@ -9,6 +9,8 @@ - only issues labelled ``good first issue`` are open to ``/assign``, and never one that also carries ``security``, ``needs-triage``, ``deferred``, ``epic`` or ``phase: 16``; +- a blocked issue is refused: one labelled ``blocked``, or one GitHub still lists an open "Blocked by" issue for + (the label follows the links hourly, so the count closes the gap; ``tools/issue_dependencies.py``); - an issue that already has an assignee is refused, whoever the assignee is and however they were assigned: a later ``/assign`` never replaces, adds to or removes an existing assignment; - a contributor holds at most ``MAX_OPEN_ASSIGNMENTS`` open issues at a time (the maintainers in @@ -56,6 +58,7 @@ # lock the rest), or larger tasks that need a maintainer's answer first. ELIGIBLE_LABELS = frozenset({"good first issue"}) EXCLUDED_LABELS = frozenset({"security", "needs-triage", "deferred", "epic", "phase: 16"}) +BLOCKED_LABEL = "blocked" # set and removed by tools/issue_dependencies.py from the issue's "Blocked by" links # The maintainers, as MAINTAINERS.md lists them (tests/test_assign_on_comment.py fails if the two drift apart). # GitHub's own "author association" is not used: it also calls a read-only organisation member a MEMBER. MAINTAINER_LOGINS = frozenset({"cemililik"}) @@ -74,6 +77,11 @@ NOT_OPEN_MARKER = "" MSG_CLOSED = "This issue is closed, so it cannot be assigned." +MSG_BLOCKED = ( + "This issue is blocked: it waits for another issue to be completed (see **Blocked by** in the sidebar), so it " + "cannot be claimed yet. The `blocked` label is removed automatically once every blocker is closed as completed, " + "and the issue says so in a comment. Until then, pick another one from the [Front door view]({front_door})." +) MSG_NOT_OPEN = ( "Only `good first issue` issues can be claimed with `/assign`; this one needs a maintainer's decision. " "{mention}, could you take a look?\n\n" + NOT_OPEN_MARKER @@ -154,6 +162,7 @@ class IssueFacts: state: str labels: frozenset[str] assignees: tuple[str, ...] + open_blockers: int = 0 # GitHub's count of open "Blocked by" issues @dataclass(frozen=True) @@ -222,6 +231,8 @@ def decide(ctx: Context, issue: IssueFacts, count_open_assignments: Callable[[], return Decision("reply", MSG_NOT_YOURS) if issue.state != "open": return Decision("reply", MSG_CLOSED) + if BLOCKED_LABEL in issue.labels or issue.open_blockers > 0: + return Decision("reply", MSG_BLOCKED.format(front_door=FRONT_DOOR_URL)) if issue.labels & EXCLUDED_LABELS or not issue.labels & ELIGIBLE_LABELS: return Decision("reply", MSG_NOT_OPEN.format(mention=MAINTAINER_MENTION), pings=True) if login in issue.assignees: @@ -306,8 +317,12 @@ def fetch_issue(gh: GhApi, repo: str, number: int) -> IssueFacts: raw = gh("GET", f"repos/{repo}/issues/{number}", None) try: labels = frozenset(str(label["name"]) for label in raw["labels"]) - return IssueFacts(str(raw["state"]), labels, _logins(raw["assignees"])) - except (KeyError, TypeError) as exc: + summary = raw.get("issue_dependencies_summary") + # The summary's `blocked_by` counts only blockers that are still open. Without the summary the label alone + # decides, which is what tools/issue_dependencies.py keeps true. + open_blockers = int(summary.get("blocked_by") or 0) if isinstance(summary, dict) else 0 + return IssueFacts(str(raw["state"]), labels, _logins(raw["assignees"]), open_blockers) + except (KeyError, TypeError, ValueError) as exc: raise GhError("the issue answer lacks labels, state or assignees") from exc diff --git a/tools/issue_dependencies.py b/tools/issue_dependencies.py new file mode 100644 index 00000000..4368bd6b --- /dev/null +++ b/tools/issue_dependencies.py @@ -0,0 +1,576 @@ +#!/usr/bin/env python3 +"""Keep the ``blocked`` label in step with GitHub's "Blocked by" links, and stop pull requests on blocked issues. + +Two workflows run this script: + +- ``.github/workflows/issue-dependencies.yml`` runs ``sync`` when an issue is closed or reopened, hourly, and on + demand. It brings the ``blocked`` label of every issue in line with its native "Blocked by" links. +- ``.github/workflows/pr-dependencies.yml`` runs ``check-pr`` on every pull-request change. It fails while the pull + request links an issue that is still blocked. + +Rules (documented in ``CONTRIBUTING.md``, "Dependencies between issues"): + +- A dependency is a native "Blocked by" link, set by a maintainer in the issue's sidebar. Prose such as "after #12" + is not one. +- A blocker is resolved only when it is closed as completed. While it is open the dependent waits. When it is closed + as not planned or as a duplicate, the work the dependent waits for was not done: the dependent stays blocked, gets + ``needs-triage`` and one comment asking a maintainer to re-plan it (remove the link, or link the issue that replaces + the blocker). +- ``blocked`` is derived and never set by hand: ``sync`` adds it to every open issue with an unresolved blocker and + removes it everywhere else, from a closed issue and from one where someone set it by hand. When it removes the + label from an open issue it posts one comment; it does not comment when an issue becomes blocked. +- ``check-pr`` fails when the pull request's description links a blocked issue with ``Fixes``, ``Closes``, + ``Resolves`` or ``Refs`` — unless the same pull request closes every open blocker (``Fixes``, ``Closes`` or + ``Resolves``), or a maintainer has labelled it ``dependency-override``. It reads the links themselves, not the + label, so it never trails behind ``sync``. When ``sync`` unblocks an issue it re-runs a failed check on the open + pull requests that link it. + +Trust boundary. Issue and pull-request text is written by anyone, so the script never executes or prints it: it +parses the description with regular expressions and writes only fixed messages and issue numbers to the log. The +workflows pass nothing through the shell; the script reads the event file itself. Every subprocess call is a list +without a shell, limited to fixed endpoints (``repos/`` paths built from integers, and ``graphql`` with a query +constant from this module). + +Exit codes (per ``tools/`` contract, not the public 0-6 surface of ``forgelm/``): + +- ``0`` — done: labels in step, or the pull request may merge. +- ``1`` — unexpected failure: an API call failed, or GitHub's answer was not what its API documents. +- ``2`` — the event payload, the arguments or the environment are unusable. +- ``3`` — ``check-pr`` only: the pull request links an issue that is still blocked. +""" + +from __future__ import annotations + +import argparse +import json +import os +import re +import subprocess +import sys +from dataclasses import dataclass +from typing import Any, Callable, Iterable, Mapping, Optional, Sequence +from urllib.parse import quote + +BLOCKED_LABEL = "blocked" +OVERRIDE_LABEL = "dependency-override" +TRIAGE_LABEL = "needs-triage" +PR_WORKFLOW_FILE = "pr-dependencies.yml" +# Hidden marker in the "a blocker was not completed" notice, one per blocker. Only a bot's comment counts, so a +# person cannot forge it to silence the notice. +CANCELLED_MARKER = "" + +_GH_TIMEOUT_SECONDS = 60 +_PAGE_SIZE = 100 +_MAX_PAGES = 30 # 3,000 issues: far above the backlog; bounds every paginated read +_MAX_BLOCKERS = 50 # GitHub shows at most this many "Blocked by" links per issue +_ALLOWED_METHODS = frozenset({"GET", "POST", "DELETE"}) + +MSG_UNBLOCKED = ( + "This issue is no longer blocked: every issue it waited for is closed as completed ({blockers}). It can be " + "picked up now; see [Working on an issue]({contributing})." +) +MSG_UNBLOCKED_NO_LINKS = ( + 'This issue is no longer blocked: it has no open "Blocked by" link. The `blocked` label follows those links ' + "and is set and removed automatically; see [Dependencies between issues]({contributing})." +) +MSG_NOT_COMPLETED = ( + "#{blocker}, which this issue is blocked by, was closed as {reason}, not as completed, so the work this issue " + "waits for was not done and the issue stays blocked. {mention}, please re-plan it: remove the link if the " + "dependency no longer holds, or link the issue that replaces #{blocker}.\n\n{marker}" +) + +MAINTAINER_LOGINS = frozenset({"cemililik"}) # MAINTAINERS.md; tests/test_issue_dependencies.py keeps them equal +MAINTAINER_MENTION = " ".join(f"@{login}" for login in sorted(MAINTAINER_LOGINS)) + +# A reference in a pull-request description: `#12`, `owner/repo#12` or an issue URL, after a keyword. GitHub closes +# an issue for close/fix/resolve and their inflections; `Refs` is this project's word for "part of". +_KEYWORD = r"(?Pclose[sd]?|fix(?:e[sd])?|resolve[sd]?|refs?|references)" +_OWNER, _NAME = r"[A-Za-z0-9-]+", r"[A-Za-z0-9._-]+" +_REF_RE = re.compile( + rf"(?:(?P{_OWNER})/(?P{_NAME}))?#(?P\d+)" + rf"|https://github\.com/(?P{_OWNER})/(?P{_NAME})/issues/(?P\d+)" +) +_REF_PLAIN = rf"(?:(?:{_OWNER}/{_NAME})?#\d+|https://github\.com/{_OWNER}/{_NAME}/issues/\d+)" +# Over-matching is the safe direction for a blocking check, so a colon or a missing space after the keyword counts. +_LINK_RE = re.compile( + rf"(?{_REF_PLAIN}(?:\s*(?:,|and|&)\s*{_REF_PLAIN})*)", re.IGNORECASE +) +_CLOSING = frozenset({"close", "closes", "closed", "fix", "fixes", "fixed", "resolve", "resolves", "resolved"}) +_FENCE_RE = re.compile(r"^ {0,3}(```|~~~).*?^ {0,3}\1[^\S\n]*$", re.MULTILINE | re.DOTALL) +_HTML_COMMENT_RE = re.compile(r"", re.DOTALL) +_INLINE_CODE_RE = re.compile(r"`[^`\n]*`") + +# Every GraphQL document this script may send. A query is chosen by name; only integer and cursor variables vary. +QUERIES = { + "open_issues": """ +query($owner: String!, $name: String!, $after: String) { + repository(owner: $owner, name: $name) { + issues(states: OPEN, first: 100, after: $after) { + pageInfo { hasNextPage endCursor } + nodes { + number + labels(first: 50) { nodes { name } } + blockedBy(first: 50) { totalCount nodes { number state stateReason repository { nameWithOwner } } } + } + } + } +}""", + "issue": """ +query($owner: String!, $name: String!, $number: Int!) { + repository(owner: $owner, name: $name) { + issueOrPullRequest(number: $number) { + __typename + ... on Issue { + number + state + labels(first: 50) { nodes { name } } + blockedBy(first: 50) { totalCount nodes { number state stateReason repository { nameWithOwner } } } + } + } + } +}""", +} + + +class EventError(Exception): + """The event payload, the arguments or the environment are not usable.""" + + +class GhError(RuntimeError): + """A ``gh api`` call failed, or GitHub's answer was not what the API documents.""" + + +# (method, endpoint, optional JSON body) -> parsed JSON answer (None for an empty body). +GhApi = Callable[[str, str, Optional[Mapping[str, Any]]], Any] + + +@dataclass(frozen=True) +class Blocker: + """One "Blocked by" link: the issue it points at and how that issue stands.""" + + number: int + repo: str # owner/name of the blocker's repository + state: str # OPEN or CLOSED + reason: Optional[str] # COMPLETED, NOT_PLANNED, DUPLICATE, REOPENED or None + + @property + def status(self) -> str: + """``waiting`` (open), ``resolved`` (closed as completed) or ``not-completed`` (closed any other way). + + GitHub records a reason for every close since 2022; an older issue closed before that has none and counts as + completed, which is what closing meant then. + """ + if self.state != "CLOSED": + return "waiting" + if self.reason in (None, "COMPLETED"): + return "resolved" + return "not-completed" + + def ref(self, home: str) -> str: + return f"#{self.number}" if self.repo.lower() == home.lower() else f"{self.repo}#{self.number}" + + +@dataclass(frozen=True) +class IssueState: + """What ``sync`` needs to know about one issue.""" + + number: int + is_open: bool + labels: frozenset[str] + blockers: tuple[Blocker, ...] + + +@dataclass(frozen=True) +class Plan: + """What ``sync`` does to one issue. ``comment`` is the unblocked notice; ``not_completed`` lists blockers that + were closed without being completed (each gets one notice and ``needs-triage``).""" + + add_label: bool = False + remove_label: bool = False + comment: str = "" + not_completed: tuple[Blocker, ...] = () + + @property + def changes(self) -> bool: + return self.add_label or self.remove_label or bool(self.not_completed) + + +def contributing_url(repo: str, anchor: str = "dependencies-between-issues") -> str: + return f"https://github.com/{repo}/blob/development/CONTRIBUTING.md#{anchor}" + + +def is_blocked(blockers: Iterable[Blocker]) -> bool: + """Blocked while any blocker is not resolved: still open, or closed without being completed.""" + return any(b.status != "resolved" for b in blockers) + + +def plan_issue(issue: IssueState, repo: str) -> Plan: + """The rules for one issue: is it blocked, and what changes.""" + if not issue.is_open: + return Plan(remove_label=BLOCKED_LABEL in issue.labels) + has_label = BLOCKED_LABEL in issue.labels + if is_blocked(issue.blockers): + not_completed = tuple(b for b in issue.blockers if b.status == "not-completed") + return Plan(add_label=not has_label, not_completed=not_completed) + if not has_label: + return Plan() + if issue.blockers: + refs = ", ".join(b.ref(repo) for b in issue.blockers) + message = MSG_UNBLOCKED.format(blockers=refs, contributing=contributing_url(repo, "working-on-an-issue")) + else: + message = MSG_UNBLOCKED_NO_LINKS.format(contributing=contributing_url(repo)) + return Plan(remove_label=True, comment=message) + + +def not_completed_notice(blocker: Blocker) -> str: + reason = "a duplicate" if blocker.reason == "DUPLICATE" else "not planned" + return MSG_NOT_COMPLETED.format( + blocker=blocker.number, + reason=reason, + mention=MAINTAINER_MENTION, + marker=CANCELLED_MARKER.format(blocker=blocker.number), + ) + + +def _strip_non_prose(body: str) -> str: + """Drop fenced code, inline code and HTML comments: the template's own examples live there.""" + for pattern in (_FENCE_RE, _HTML_COMMENT_RE, _INLINE_CODE_RE): + body = pattern.sub(" ", body) + return body + + +def linked_issues(body: str, repo: str) -> tuple[frozenset[int], frozenset[int]]: + """``(closing, all)``: issue numbers of this repository linked from a pull-request description. + + ``closing`` holds the ones linked with a closing keyword; ``all`` adds those linked with ``Refs``. A reference to + another repository is ignored: this repository's links cannot block on it, and its workflow cannot close it. + """ + owner, name = repo.lower().split("/") + closing: set[int] = set() + every: set[int] = set() + for match in _LINK_RE.finditer(_strip_non_prose(body or "")): + keyword = match.group("kw").lower() + for ref in _REF_RE.finditer(match.group("refs")): + ref_owner = (ref.group("owner") or ref.group("uowner") or owner).lower() + ref_repo = (ref.group("repo") or ref.group("urepo") or name).lower() + if (ref_owner, ref_repo) != (owner, name): + continue + number = int(ref.group("num") or ref.group("unum")) + every.add(number) + if keyword in _CLOSING: + closing.add(number) + return frozenset(closing), frozenset(every) + + +@dataclass(frozen=True) +class Verdict: + """``check-pr``'s answer: may the pull request merge, and why not.""" + + ok: bool + lines: tuple[str, ...] + + +def check_pull_request( + labels: frozenset[str], + closing: frozenset[int], + linked: frozenset[int], + blockers_of: Callable[[int], Optional[tuple[Blocker, ...]]], + repo: str, +) -> Verdict: + """The rules for one pull request. ``blockers_of`` returns an open issue's links, or ``None`` for a pull request, + a closed issue or a number that does not exist (none of those can be blocked work).""" + if OVERRIDE_LABEL in labels: + return Verdict(True, (f"`{OVERRIDE_LABEL}` is set: a maintainer accepted the linked dependencies.",)) + problems: list[str] = [] + checked = 0 + for number in sorted(linked): + blockers = blockers_of(number) + if blockers is None: + continue + checked += 1 + for b in blockers: + if b.status == "resolved": + continue + if b.status == "waiting" and b.repo.lower() == repo.lower() and b.number in closing: + continue # this pull request closes the blocker as well + why = "is still open" if b.status == "waiting" else "was closed without being completed" + problems.append(f"#{number} is blocked by {b.ref(repo)}, which {why}.") + if problems: + return Verdict( + False, + tuple(problems) + + ( + "Wait until the blocker is closed as completed (this check re-runs then), close it in this pull request " + f"too, or ask a maintainer. See {contributing_url(repo)}", + ), + ) + return Verdict(True, (f"No linked issue is blocked ({checked} issue(s) checked).",)) + + +# ------------------------------------------------------------------------------------------------ GitHub access +def _clean(text: str, limit: int = 200) -> str: + """One line, bounded, with ``::`` defanged so it can never open a workflow command.""" + return " ".join(text.split())[:limit].replace("::", ": :") + + +def gh_api(method: str, endpoint: str, payload: Optional[Mapping[str, Any]] = None) -> Any: + """Call the GitHub API through the ``gh`` CLI (authenticated by ``GH_TOKEN``); no shell is involved.""" + if method not in _ALLOWED_METHODS or any(c.isspace() for c in endpoint): + raise GhError("refusing an API call outside the fixed endpoints") + if endpoint == "graphql": + if method != "POST" or not isinstance(payload, Mapping) or payload.get("query") not in QUERIES.values(): + raise GhError("refusing a GraphQL document that is not one of this module's queries") + elif not endpoint.startswith("repos/"): + raise GhError("refusing an API call outside the fixed endpoints") + command = ["gh", "api", "--method", method, endpoint] + stdin = None + if payload is not None: + command += ["--input", "-"] + stdin = json.dumps(payload) + label = f"{method} {endpoint.split('?')[0]}" + try: + # A list without a shell; the method and endpoint were validated above, and an endpoint is built only from + # integers, the repository name and fixed path segments. + proc = subprocess.run( # nosemgrep: python.lang.security.audit.dangerous-subprocess-use-audit + command, input=stdin, capture_output=True, text=True, timeout=_GH_TIMEOUT_SECONDS, check=False + ) + except (OSError, subprocess.TimeoutExpired) as exc: + raise GhError(f"{label} could not run: {type(exc).__name__}") from exc + if proc.returncode != 0: + raise GhError(f"{label} failed (exit {proc.returncode}): {_clean(proc.stderr)}") + if not proc.stdout.strip(): + return None + try: + return json.loads(proc.stdout) + except ValueError as exc: + raise GhError(f"{label} returned something that is not JSON") from exc + + +def graphql(gh: GhApi, query: str, **variables: Any) -> Mapping[str, Any]: + answer = gh("POST", "graphql", {"query": QUERIES[query], "variables": variables}) + if not isinstance(answer, Mapping) or answer.get("errors") or not isinstance(answer.get("data"), Mapping): + errors = answer.get("errors") if isinstance(answer, Mapping) else None + first = ( + errors[0].get("message", "") + if isinstance(errors, list) and errors and isinstance(errors[0], Mapping) + else "" + ) + raise GhError(f"GraphQL {query} failed: {_clean(str(first))}") + return answer["data"] + + +def _blockers(node: Mapping[str, Any]) -> tuple[Blocker, ...]: + try: + links = node["blockedBy"] + if links["totalCount"] > len(links["nodes"]): + raise GhError(f"issue #{node['number']} has more than {_MAX_BLOCKERS} blockers") + return tuple( + Blocker(int(b["number"]), str(b["repository"]["nameWithOwner"]), str(b["state"]), b.get("stateReason")) + for b in links["nodes"] + ) + except (KeyError, TypeError, ValueError) as exc: + raise GhError("a blockedBy answer lacks a field") from exc + + +def _labels(node: Mapping[str, Any]) -> frozenset[str]: + try: + return frozenset(str(label["name"]) for label in node["labels"]["nodes"]) + except (KeyError, TypeError) as exc: + raise GhError("a labels answer lacks a field") from exc + + +def fetch_open_issues(gh: GhApi, repo: str) -> list[IssueState]: + owner, name = repo.split("/") + out: list[IssueState] = [] + after: Optional[str] = None + for _ in range(_MAX_PAGES): + data = graphql(gh, "open_issues", owner=owner, name=name, after=after) + try: + page = data["repository"]["issues"] + nodes, info = page["nodes"], page["pageInfo"] + except (KeyError, TypeError) as exc: + raise GhError("the open-issues answer lacks a field") from exc + out += [IssueState(int(n["number"]), True, _labels(n), _blockers(n)) for n in nodes] + if not info.get("hasNextPage"): + return out + after = str(info["endCursor"]) + raise GhError(f"more than {_MAX_PAGES} pages of open issues") + + +def fetch_closed_labelled(gh: GhApi, repo: str) -> list[int]: + """Closed issues that still carry ``blocked`` (a closed issue no longer waits for anything).""" + numbers: list[int] = [] + for page in range(1, _MAX_PAGES + 1): + raw = gh( + "GET", + f"repos/{repo}/issues?state=closed&labels={quote(BLOCKED_LABEL)}&per_page={_PAGE_SIZE}&page={page}", + None, + ) + if not isinstance(raw, list): + raise GhError("the closed-issues answer is not a list") + numbers += [int(i["number"]) for i in raw if isinstance(i, Mapping) and "pull_request" not in i] + if len(raw) < _PAGE_SIZE: + return numbers + raise GhError(f"more than {_MAX_PAGES} pages of closed issues") + + +def fetch_blockers_of(gh: GhApi, repo: str, number: int) -> Optional[tuple[Blocker, ...]]: + """An open issue's links, or ``None`` when ``number`` is a pull request, a closed issue or does not exist.""" + owner, name = repo.split("/") + data = graphql(gh, "issue", owner=owner, name=name, number=number) + node = (data.get("repository") or {}).get("issueOrPullRequest") + if not isinstance(node, Mapping) or node.get("__typename") != "Issue" or node.get("state") != "OPEN": + return None + return _blockers(node) + + +def _already_noticed(gh: GhApi, repo: str, number: int, marker: str) -> bool: + for page in range(1, _MAX_PAGES + 1): + raw = gh("GET", f"repos/{repo}/issues/{number}/comments?per_page={_PAGE_SIZE}&page={page}", None) + if not isinstance(raw, list): + raise GhError("the comments answer is not a list") + for comment in raw: + user = comment.get("user") if isinstance(comment, Mapping) else None + if isinstance(user, Mapping) and user.get("type") == "Bot" and marker in str(comment.get("body")): + return True + if len(raw) < _PAGE_SIZE: + return False + return False + + +def _rerun_pr_checks(gh: GhApi, repo: str, unblocked: set[int]) -> list[int]: + """Re-run a failed dependency check on the open pull requests that link a just-unblocked issue. + + Optional: when it fails, the check re-runs anyway on the pull request's next edit or push, so a failure is + reported as a warning and does not fail the sync. + """ + rerun: list[int] = [] + try: + for page in range(1, 4): + pulls = gh("GET", f"repos/{repo}/pulls?state=open&per_page={_PAGE_SIZE}&page={page}", None) + if not isinstance(pulls, list): + raise GhError("the pull-request list is not a list") + for pull in pulls: + _, linked = linked_issues(str(pull.get("body") or ""), repo) + if not linked & unblocked: + continue + sha = str(pull["head"]["sha"]) + if not re.fullmatch(r"[0-9a-f]{40}", sha): + raise GhError("a pull request's head sha is malformed") + runs = gh( + "GET", f"repos/{repo}/actions/workflows/{PR_WORKFLOW_FILE}/runs?head_sha={sha}&per_page=5", None + ) + latest = (runs or {}).get("workflow_runs") or [] + if latest and latest[0].get("conclusion") == "failure": + gh("POST", f"repos/{repo}/actions/runs/{int(latest[0]['id'])}/rerun-failed-jobs", None) + rerun.append(int(pull["number"])) + if len(pulls) < _PAGE_SIZE: + break + except (GhError, KeyError, TypeError, ValueError) as exc: + print(f"warning: could not re-run pull-request checks: {_clean(str(exc))}", file=sys.stderr) + return rerun + + +# ----------------------------------------------------------------------------------------------------- commands +def run_sync(repo: str, gh: GhApi, dry_run: bool = False) -> int: + issues = fetch_open_issues(gh, repo) + issues += [IssueState(n, False, frozenset({BLOCKED_LABEL}), ()) for n in fetch_closed_labelled(gh, repo)] + plans = [(issue, plan_issue(issue, repo)) for issue in issues] + blocked = sum(1 for i in issues if i.is_open and is_blocked(i.blockers)) + unblocked: set[int] = set() + for issue, plan in plans: + if not plan.changes: + continue + verb = "would" if dry_run else "did" + if plan.add_label: + print(f"{verb} label #{issue.number} {BLOCKED_LABEL}") + if not dry_run: + gh("POST", f"repos/{repo}/issues/{issue.number}/labels", {"labels": [BLOCKED_LABEL]}) + if plan.remove_label: + print(f"{verb} unlabel #{issue.number} {BLOCKED_LABEL}" + (" and comment" if plan.comment else "")) + if not dry_run: + gh("DELETE", f"repos/{repo}/issues/{issue.number}/labels/{quote(BLOCKED_LABEL)}", None) + if plan.comment: + gh("POST", f"repos/{repo}/issues/{issue.number}/comments", {"body": plan.comment}) + if issue.is_open: + unblocked.add(issue.number) + for b in plan.not_completed: + marker = CANCELLED_MARKER.format(blocker=b.number) + if dry_run: + print(f"would check #{issue.number}: blocker #{b.number} was not completed") + continue + if _already_noticed(gh, repo, issue.number, marker): + continue + print(f"did flag #{issue.number}: blocker #{b.number} was not completed") + gh("POST", f"repos/{repo}/issues/{issue.number}/labels", {"labels": [TRIAGE_LABEL]}) + gh("POST", f"repos/{repo}/issues/{issue.number}/comments", {"body": not_completed_notice(b)}) + rerun = [] if dry_run or not unblocked else _rerun_pr_checks(gh, repo, unblocked) + print( + f"open issues: {sum(1 for i in issues if i.is_open)}, blocked: {blocked}, unblocked now: {len(unblocked)}, " + f"pull-request checks re-run: {len(rerun)}" + ) + return 0 + + +def run_check_pr(event: Mapping[str, Any], repo: str, gh: GhApi) -> int: + try: + number = event["pull_request"]["number"] + except (KeyError, TypeError) as exc: + raise EventError("event payload has no pull_request.number") from exc + if isinstance(number, bool) or not isinstance(number, int) or number < 1: + raise EventError("pull request number is not a positive integer") + # Read the pull request again rather than trust the event: a re-run replays the original payload, and the + # description or the labels may have changed since. + pull = gh("GET", f"repos/{repo}/pulls/{number}", None) + try: + body = str(pull.get("body") or "") + labels = frozenset(str(label["name"]) for label in pull["labels"]) + except (AttributeError, KeyError, TypeError) as exc: + raise GhError("the pull-request answer lacks body or labels") from exc + closing, linked = linked_issues(body, repo) + verdict = check_pull_request(labels, closing, linked, lambda n: fetch_blockers_of(gh, repo, n), repo) + for line in verdict.lines: + print(line if verdict.ok else f"::error::{line}") + return 0 if verdict.ok else 3 + + +def _repo_from_env() -> str: + repo = os.environ.get("GITHUB_REPOSITORY", "") + if not re.fullmatch(r"[A-Za-z0-9-]+/[A-Za-z0-9._-]+", repo): + raise EventError("GITHUB_REPOSITORY must be set to owner/name") + return repo + + +def main(argv: Optional[Sequence[str]] = None, gh: GhApi = gh_api) -> int: + parser = argparse.ArgumentParser(description=__doc__.split("\n\n")[0]) + sub = parser.add_subparsers(dest="command", required=True) + sync = sub.add_parser("sync", help="bring the blocked label in step with the Blocked-by links") + sync.add_argument("--dry-run", action="store_true", help="print what would change, change nothing") + sub.add_parser("check-pr", help="fail when the pull request links a blocked issue (reads GITHUB_EVENT_PATH)") + try: + args = parser.parse_args(argv) + except SystemExit as exc: # argparse exits 2 on a usage error and 0 on --help + return int(exc.code or 0) + try: + repo = _repo_from_env() + if args.command == "sync": + return run_sync(repo, gh, dry_run=args.dry_run) + event_path = os.environ.get("GITHUB_EVENT_PATH", "") + if not event_path: + raise EventError("GITHUB_EVENT_PATH must be set") + try: + with open(event_path, encoding="utf-8") as handle: + event = json.load(handle) + except (OSError, ValueError) as exc: + raise EventError(f"cannot read the event payload: {type(exc).__name__}") from exc + return run_check_pr(event, repo, gh) + except EventError as exc: + print(f"error: {exc}", file=sys.stderr) + return 2 + except GhError as exc: + print(f"error: {exc}", file=sys.stderr) + return 1 + + +if __name__ == "__main__": + sys.exit(main()) From 76de1e814d617a8877fd37258143e33ed74ae55b Mon Sep 17 00:00:00 2001 From: Cemil ILIK Date: Tue, 20 Oct 2026 14:53:11 +0300 Subject: [PATCH 2/6] fix(ci): harden the issue-dependency check after review - Run the "Issue dependencies" check on pull_request_target, so the workflow and the script come from the default branch and a pull request cannot rewrite the check that judges it. Nothing from the pull request is checked out or run, and the token stays read-only. - Count a blocker as closed by the pull request only in the form GitHub closes on merge (`Fixes #N`, one keyword per issue); `Fixes#N`, `Fixes: #N`, a URL or the second issue of a list no longer exempt it. - Honour `dependency-override` only when a maintainer added it last, checked against the issue events. - Reconcile pull-request checks by state on every sync: re-run the check of any open pull request whose last result no longer matches its linked issues, in both directions, across every page of pull requests. A check still running or a failed re-run is picked up by the next run. - Re-read an issue right before unblocking it, post the notice before removing the label and make it idempotent with a marker, so a reopened blocker or a half-finished run never yields a wrong or lost notice. - Key not-completed notices by owner/repo#N; check that labels fit in one page; read pull-request descriptions in linear time (line walker for fences, str.find for HTML comments; regex.md rule 6). - /assign also refuses an issue whose blocker was closed without being completed before the label caught up. - CONTRIBUTING says the check is not yet required until branch protection lists it, and no notice is promised that may not come. Refs #1034 Co-Authored-By: Claude Opus 5.5 --- .github/workflows/issue-dependencies.yml | 15 +- .github/workflows/pr-dependencies.yml | 32 +- AGENTS.md | 2 +- CHANGELOG.md | 6 +- CLAUDE.md | 2 +- CONTRIBUTING.md | 8 +- tests/test_assign_on_comment.py | 32 +- tests/test_issue_dependencies.py | 370 ++++++++++++++++------ tools/assign_on_comment.py | 43 ++- tools/issue_dependencies.py | 374 ++++++++++++++++------- 10 files changed, 650 insertions(+), 234 deletions(-) diff --git a/.github/workflows/issue-dependencies.yml b/.github/workflows/issue-dependencies.yml index 1ef5211a..1d63615f 100644 --- a/.github/workflows/issue-dependencies.yml +++ b/.github/workflows/issue-dependencies.yml @@ -5,13 +5,15 @@ name: Issue dependencies # - an open issue with a blocker that is not closed as completed gets `blocked`; # - every other issue loses it, with one comment when an open issue is unblocked; # - a blocker closed as not planned or as a duplicate leaves the issue blocked and asks a maintainer to re-plan it; -# - an unblocked issue's open pull requests get their failed "Issue dependencies" check re-run. +# - every open pull request whose "Issue dependencies" check no longer matches its linked issues (red although +# they are unblocked, or green although one became blocked) gets that check re-run. # # GitHub sends no workflow event when a "Blocked by" link is added or removed, so this runs when an issue is closed # or reopened (what unblocks or re-blocks its dependents), hourly as the backstop for link edits, and on demand. -# Every run is a full reconciliation, so the concurrency group may drop a pending run: the one that replaces it -# does the same work. tests/test_issue_dependencies.py pins these properties: -# - the token holds `contents: read`, `issues: write`, `pull-requests: read` and `actions: write`, nothing else; +# Every run is a full reconciliation of state, not a reaction to a transition, so the concurrency group may drop a +# pending run, and a run that fails half-way is finished by the next one. tests/test_issue_dependencies.py pins: +# - the token holds `contents: read`, `issues: write`, `pull-requests: read`, `checks: read` and `actions: write`, +# nothing else; # - no `${{ }}` appears in a `run:` line; the script reads the environment itself; # - the checkout takes the default branch, one file, no stored credentials, pinned to a full commit SHA. # @@ -39,8 +41,9 @@ jobs: permissions: contents: read # the checkout reads one script from the default branch issues: write # add or remove `blocked` / `needs-triage`, and post the notices - pull-requests: read # find the open pull requests that link an unblocked issue - actions: write # re-run their failed dependency check + pull-requests: read # read the open pull requests and the issues they link + checks: read # read each one's latest "Issue dependencies" result + actions: write # re-run that check when the result no longer matches steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: diff --git a/.github/workflows/pr-dependencies.yml b/.github/workflows/pr-dependencies.yml index 0a45ccbc..fa417a2c 100644 --- a/.github/workflows/pr-dependencies.yml +++ b/.github/workflows/pr-dependencies.yml @@ -1,20 +1,27 @@ name: PR dependencies -# The required "Issue dependencies" check: fails while the pull request's description links (Fixes / Closes / -# Resolves / Refs #N) an issue that is still blocked, unless the same pull request closes every open blocker or a -# maintainer has labelled it `dependency-override`. Rules: tools/issue_dependencies.py and CONTRIBUTING.md, -# "Dependencies between issues". When a blocker is closed, the "Issue dependencies" workflow re-runs this check. +# The "Issue dependencies" check: fails while the pull request's description links (Fixes / Closes / Resolves / +# Refs #N) an issue that is still blocked, unless the same pull request closes every open blocker or a maintainer has +# labelled it `dependency-override`. Rules: tools/issue_dependencies.py and CONTRIBUTING.md, "Dependencies between +# issues". The "Issue dependencies" workflow re-runs this check whenever its result no longer matches the issues. # -# The pull request's code never runs here: the checkout takes the base commit's copy of the script, so a pull -# request cannot change the check that judges it. The token is read-only, which is all a fork's pull request gets -# anyway. tests/test_issue_dependencies.py pins these properties: -# - `pull_request` only (never `pull_request_target`), no `paths:` filter (a required check must always report); +# Why `pull_request_target`. A `pull_request` run takes this file from the pull request itself, so a pull request +# could rewrite the check that judges it. `pull_request_target` takes the workflow and the script from the default +# branch instead. Its known risk is running the pull request's code with a privileged token; this job never does: +# the checkout has no `ref`, so it reads one file from the default branch; nothing from the pull request is checked +# out, built or executed; the token is read-only; no secret but GITHUB_TOKEN is used; and the script treats the +# description as data. tests/test_issue_dependencies.py pins these properties: +# - `pull_request_target` only, no `paths:` or `branches:` filter (a required check must always report); # - the token holds `contents: read`, `issues: read` and `pull-requests: read`, nothing else; -# - the checkout takes the base SHA, one file, no stored credentials, pinned to a full commit SHA; +# - the checkout has no `ref`, takes one file, keeps no credentials and is pinned to a full commit SHA, and no +# expression in this file names the pull request's head; # - no `${{ }}` appears in a `run:` line; the script reads the event file itself. +# What it cannot stop: a pull request may add a different workflow with a job of the same name, and branch +# protection cannot tell two GitHub Actions checks of one name apart (true of every required Actions check), so a +# maintainer reads every change under `.github/` before merging it. on: - pull_request: + pull_request_target: types: [opened, edited, reopened, synchronize, ready_for_review, labeled, unlabeled] permissions: {} @@ -25,13 +32,12 @@ jobs: runs-on: ubuntu-latest timeout-minutes: 5 permissions: - contents: read # the checkout reads one script from the base commit - issues: read # read the linked issues and their "Blocked by" links + contents: read # the checkout reads one script from the default branch + issues: read # read the linked issues, their "Blocked by" links and the label events pull-requests: read # read the pull request's current description and labels steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: - ref: ${{ github.event.pull_request.base.sha }} persist-credentials: false sparse-checkout: tools/issue_dependencies.py sparse-checkout-cone-mode: false diff --git a/AGENTS.md b/AGENTS.md index c6d00590..34a0f3f6 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -389,7 +389,7 @@ Work on ForgeLM is planned and tracked on GitHub, not in local notes: - **Closing is not triage.** Every item is an issue; which issues appear on the front door is decided by labels and views, never by closing them. - **Dependencies are links, not prose.** When an issue cannot start before another is done, add a **Blocked by** link in its sidebar. The `blocked` - label, `/assign` and the required **Issue dependencies** pull-request check + label, `/assign` and the **Issue dependencies** pull-request check all follow those links (`tools/issue_dependencies.py`; CONTRIBUTING.md, *Dependencies between issues*). Never set or remove `blocked` by hand, and do not start work on a `blocked` issue. diff --git a/CHANGELOG.md b/CHANGELOG.md index 3923bd44..b9d12a69 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,8 +31,10 @@ All notable changes to ForgeLM are documented here. workflow (`tools/issue_dependencies.py`) labels every open issue with an unresolved blocker `blocked` and removes the label, with one comment, once every blocker is closed as completed. A blocker closed as not planned or as a duplicate keeps the issue blocked and flags it `needs-triage`. `/assign` refuses a blocked issue, and the new - required `Issue dependencies` check fails a pull request that links one (`Fixes` / `Closes` / `Resolves` / `Refs`) - unless it closes the blockers too or a maintainer sets `dependency-override` (#1034). + `Issue dependencies` pull-request check fails a pull request that links one (`Fixes` / `Closes` / `Resolves` / + `Refs`) unless it closes the blockers too (in the form GitHub closes on merge) or a maintainer sets + `dependency-override`. The check runs from the default branch (`pull_request_target`, no pull-request code is + checked out), and the workflow re-runs it on every open pull request whose result no longer matches (#1034). - `GOVERNANCE.md` and `MAINTAINERS.md` at the repository root, recording how decisions are made, how the maintainer roles work, and which areas the current maintainer owns (#964). diff --git a/CLAUDE.md b/CLAUDE.md index 8c74caa9..8c28d87e 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -389,7 +389,7 @@ Work on ForgeLM is planned and tracked on GitHub, not in local notes: - **Closing is not triage.** Every item is an issue; which issues appear on the front door is decided by labels and views, never by closing them. - **Dependencies are links, not prose.** When an issue cannot start before another is done, add a **Blocked by** link in its sidebar. The `blocked` - label, `/assign` and the required **Issue dependencies** pull-request check + label, `/assign` and the **Issue dependencies** pull-request check all follow those links (`tools/issue_dependencies.py`; CONTRIBUTING.md, *Dependencies between issues*). Never set or remove `blocked` by hand, and do not start work on a `blocked` issue. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 3968019c..67fa4ad4 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -128,10 +128,10 @@ Useful starting points: Some issues can only be done after another one: each Phase 16 step waits for the step before it, a deferred item may wait for the step that changes the same file, and a review issue whose every finding is a unit of a Phase 16 step waits for that step, whose pull request fixes it. Such a dependency is recorded as a **Blocked by** link in the issue's sidebar (*Relationships*), never only in prose. -- **An issue with an open blocker is labelled `blocked`.** Do not start on it and do not open a pull request for it: `/assign` refuses it, and the required **Issue dependencies** check fails a pull request whose description links it with `Fixes`, `Closes`, `Resolves` or `Refs #…`. -- **The label is automatic.** Nobody sets or removes it by hand. The *Issue dependencies* workflow adds it while a blocker is open and removes it once every blocker is closed as completed — as soon as that blocker is closed, or within an hour of a link being added or removed. The issue then says so in a comment, and the failed check of an open pull request that links it is re-run. +- **An issue with an open blocker is labelled `blocked`.** Do not start on it and do not open a pull request for it: `/assign` refuses it, and the **Issue dependencies** pull-request check fails a pull request whose description links it with `Fixes`, `Closes`, `Resolves` or `Refs #…`. +- **The label is automatic.** Nobody sets or removes it by hand. The *Issue dependencies* workflow adds it while a blocker is open and removes it once every blocker is closed as completed — as soon as that blocker is closed, or within an hour of a link being added or removed. When it removes the label it says so in a comment on the issue. Within the same hour it also re-runs the check of every open pull request whose result no longer matches, so a red check turns green once its blockers are completed, and a green one turns red if a linked issue becomes blocked. - **A blocker closed as not planned or as a duplicate unblocks nothing**, because the work the issue waits for was not done. The issue stays `blocked`, gets `needs-triage`, and a maintainer re-plans it: removes the link, or links the issue that replaces the blocker. -- **A pull request may close the blocker as well.** When it closes every open blocker of the issues it links (`Fixes #…` for each), the check passes. A maintainer can also accept a dependency deliberately with the `dependency-override` label. +- **A pull request may close the blocker as well.** When it closes every open blocker of the issues it links, the check passes. It counts a blocker as closed only in the form GitHub closes on merge — the keyword, a space and the issue, once per issue (`Fixes #12, fixes #13`) — so `Fixes#12`, `Fixes: #12` or the second issue of `Fixes #12, #13` leave the blocker open. A maintainer listed in [MAINTAINERS.md](MAINTAINERS.md) can also accept a dependency deliberately with the `dependency-override` label; the label set by anyone else is ignored. - **Grouped issues are not blocked as a whole** when only some of their findings belong to a Phase 16 step. Those findings say so in the checklist (`KNOWN-OPEN … (S8 open)`): leave them to the step and take the others. The project's *Blocked* view lists every blocked issue; *Front door* and *Small tasks* leave them out. @@ -253,7 +253,7 @@ keep this list and that one in sync if either changes. Push your branch to your fork and open a Pull Request against `development` (the default base). Link the issue it addresses (`Fixes #123`, or `Refs #123` for part of a grouped issue) and quote any finding IDs — see [Working on an issue](#working-on-an-issue). -**Before a pull request can merge**, branch protection on `development` (and on `main`) requires three things: the six CI checks — `lint`, `test (3.10)`, `test (3.11)`, `test (3.12)`, `test (3.13)` and `validate` — passing, and on `development` also the **Issue dependencies** check (see [Dependencies between issues](#dependencies-between-issues)); the branch up to date with its target (use *Update branch*); and every review conversation resolved, including the comments of the automated reviewers. +**Before a pull request can merge**, branch protection on `development` (and on `main`) requires three things: the six CI checks — `lint`, `test (3.10)`, `test (3.11)`, `test (3.12)`, `test (3.13)` and `validate` — passing; the branch up to date with its target (use *Update branch*); and every review conversation resolved, including the comments of the automated reviewers. The **Issue dependencies** check ([Dependencies between issues](#dependencies-between-issues)) runs on every pull request too, but it is not yet among the required checks: a maintainer adds it to the branch protection of `development` once it has run on a pull request after this change (tracked in [#1034](https://github.com/HodeTech/ForgeLM/issues/1034)). Until then a red result is a signal, not a merge block. Maintainers **squash contributor PRs into `development` by default**. Releases into `main` and synchronization PRs from `main` back into `development` use **merge diff --git a/tests/test_assign_on_comment.py b/tests/test_assign_on_comment.py index 1008d182..93b1b629 100644 --- a/tests/test_assign_on_comment.py +++ b/tests/test_assign_on_comment.py @@ -54,6 +54,7 @@ def __init__( self.assigned_pages: Optional[List[List[Dict[str, Any]]]] = None # overrides the count answer, page by page self.refuse_delete = False # accept the DELETE but keep the assignee self.open_blockers: Optional[int] = 0 # GitHub's count of open "Blocked by" issues; None drops the summary + self.closed_blockers: List[Optional[str]] = [] # the state_reason of each closed "Blocked by" issue self._reads = 0 def _issue(self) -> Dict[str, Any]: @@ -63,9 +64,15 @@ def _issue(self) -> Dict[str, Any]: "assignees": [{"login": a} for a in self.assignees], } if self.open_blockers is not None: - issue["issue_dependencies_summary"] = {"blocked_by": self.open_blockers, "total_blocked_by": 3} + total = self.open_blockers + len(self.closed_blockers) + issue["issue_dependencies_summary"] = {"blocked_by": self.open_blockers, "total_blocked_by": total} return issue + def _get_blockers(self, endpoint: str, payload: Any) -> List[Dict[str, Any]]: + return [{"state": "open", "state_reason": None}] * (self.open_blockers or 0) + [ + {"state": "closed", "state_reason": reason} for reason in self.closed_blockers + ] + def __call__(self, method: str, endpoint: str, payload: Any = None) -> Any: self.calls.append((method, endpoint, payload)) for (m, prefix), reason in self.fail.items(): @@ -83,6 +90,8 @@ def _route(self, method: str, endpoint: str): return self._get_issue if "?assignee=" in endpoint: return self._get_assigned_issues + if endpoint.startswith(f"repos/{REPO}/issues/7/dependencies/blocked_by?"): + return self._get_blockers if "/comments?" in endpoint: return self._get_comments elif endpoint.endswith("/assignees"): @@ -361,13 +370,30 @@ def test_an_open_blocker_refuses_the_claim_before_the_label_has_caught_up() -> N assert gh.assignees == [] and gh.comments[0].startswith("This issue is blocked") -def test_blockers_that_are_all_closed_do_not_refuse_the_claim() -> None: +def test_blockers_that_are_all_closed_as_completed_do_not_refuse_the_claim() -> None: gh = FakeGitHub(labels=["good first issue"]) - gh.open_blockers = 0 # `total_blocked_by` stays 3: closed blockers no longer count + gh.closed_blockers = ["completed", None] # None: closed before GitHub recorded reasons assert run(gh) == 0 assert gh.assignees == ["newcomer"] +@pytest.mark.parametrize("reason", ["not_planned", "duplicate"]) +def test_a_blocker_closed_without_being_completed_refuses_before_the_label_has_caught_up(reason: str) -> None: + gh = FakeGitHub(labels=["good first issue"]) + gh.closed_blockers = ["completed", reason] # the summary counts neither as open + assert run(gh) == 0 + assert gh.assignees == [] and gh.comments[0].startswith("This issue is blocked") + + +def test_the_blocker_list_is_read_only_when_the_summary_cannot_decide() -> None: + labelled = FakeGitHub(labels=["good first issue", "blocked"]) + labelled.closed_blockers = ["not_planned"] # the label already refuses + no_closed = FakeGitHub(labels=["good first issue"]) # nothing closed to look up + for gh in (labelled, no_closed): + run(gh) + assert not [c for c in gh.calls if "/dependencies/" in c[1]] + + def test_without_a_dependency_summary_the_label_alone_decides() -> None: free, blocked = FakeGitHub(labels=["good first issue"]), FakeGitHub(labels=["good first issue", "blocked"]) free.open_blockers = blocked.open_blockers = None diff --git a/tests/test_issue_dependencies.py b/tests/test_issue_dependencies.py index 1b16d97a..592d29c5 100644 --- a/tests/test_issue_dependencies.py +++ b/tests/test_issue_dependencies.py @@ -2,8 +2,9 @@ The tool derives the ``blocked`` label from GitHub's native "Blocked by" links and fails a pull request that works on a blocked issue (CONTRIBUTING.md, "Dependencies between issues"). These tests pin the rules, the parsing of -pull-request descriptions written by anyone, the write paths of ``sync`` against a fake API, the fixed-endpoint -discipline of the GitHub calls, and the security posture of both workflow files, since no other guard reads them. +pull-request descriptions written by anyone (including its linear running time), the write paths of ``sync`` and the +pull-request check reconciliation against a fake API, the fixed-endpoint discipline of the GitHub calls, and the +security posture of both workflow files, since no other guard reads them. """ from __future__ import annotations @@ -11,6 +12,7 @@ import json import re import sys +import time from pathlib import Path from typing import Any, Dict, List, Optional, Tuple @@ -24,6 +26,7 @@ import issue_dependencies as tool # noqa: E402 REPO = "HodeTech/ForgeLM" +SHA = "a" * 40 def blocker(number: int, state: str = "OPEN", reason: Optional[str] = None, repo: str = REPO) -> tool.Blocker: @@ -53,13 +56,11 @@ def test_a_blocker_is_resolved_only_when_closed_as_completed(state: str, reason: def test_an_open_blocker_labels_the_issue_without_a_comment() -> None: - plan = tool.plan_issue(issue(5, blocker(1)), REPO) - assert plan == tool.Plan(add_label=True) + assert tool.plan_issue(issue(5, blocker(1)), REPO) == tool.Plan(add_label=True) def test_an_issue_already_labelled_and_still_waiting_changes_nothing() -> None: - plan = tool.plan_issue(issue(5, blocker(1), labels=("blocked",)), REPO) - assert not plan.changes + assert not tool.plan_issue(issue(5, blocker(1), labels=("blocked",)), REPO).changes def test_one_open_blocker_among_closed_ones_keeps_the_issue_blocked() -> None: @@ -72,11 +73,13 @@ def test_when_every_blocker_is_completed_the_label_goes_and_one_comment_names_th plan = tool.plan_issue(issue(5, *done, labels=("blocked", "wave: 3")), REPO) assert plan.remove_label and not plan.add_label assert "(#1, HodeTech/Other#9)" in plan.comment and plan.comment.startswith("This issue is no longer blocked") + assert plan.marker == "" + assert plan.comment.endswith(plan.marker) def test_a_hand_set_label_without_links_is_removed_with_the_explanation() -> None: plan = tool.plan_issue(issue(5, labels=("blocked",)), REPO) - assert plan.remove_label and "set and removed automatically" in plan.comment + assert plan.remove_label and "set and removed automatically" in plan.comment and plan.marker.endswith(":none -->") def test_an_issue_that_was_never_blocked_is_left_alone() -> None: @@ -94,9 +97,17 @@ def test_a_blocker_closed_without_being_completed_keeps_the_issue_blocked_and_as gone = blocker(1, "CLOSED", reason) plan = tool.plan_issue(issue(5, gone, labels=("blocked",)), REPO) assert not plan.remove_label and plan.not_completed == (gone,) - notice = tool.not_completed_notice(gone) - assert ("a duplicate" if reason == "DUPLICATE" else "not planned") in notice - assert tool.CANCELLED_MARKER.format(blocker=1) in notice and tool.MAINTAINER_MENTION in notice + notice = tool.not_completed_notice(gone, REPO) + assert ("a duplicate" if reason == "DUPLICATE" else "not planned") in notice and notice.startswith("#1, which") + assert notice.endswith("") + assert tool.MAINTAINER_MENTION in notice + + +def test_blockers_in_two_repositories_with_one_number_never_share_a_notice() -> None: + here, there = blocker(7, "CLOSED", "NOT_PLANNED"), blocker(7, "CLOSED", "NOT_PLANNED", repo="HodeTech/Other") + a, b = tool.not_completed_notice(here, REPO), tool.not_completed_notice(there, REPO) + assert a.startswith("#7, which") and b.startswith("HodeTech/Other#7, which") + assert a.splitlines()[-1] != b.splitlines()[-1] # --- reading pull-request descriptions ------------------------------------------------------------------------- @@ -107,16 +118,19 @@ def test_a_blocker_closed_without_being_completed_keeps_the_issue_blocked_and_as [ ("Fixes #12", {12}, {12}), ("fixed #12, closes #13 and resolves #14", {12, 13, 14}, {12, 13, 14}), - ("Resolves: #12", {12}, {12}), - ("Fixes #12, #13 and #14", {12, 13, 14}, {12, 13, 14}), + ("FIXES #12", {12}, {12}), + ("Fixes HodeTech/ForgeLM#12", {12}, {12}), + # GitHub closes one issue per keyword: in a list only the first is closed, the rest are linked + ("Fixes #12, #13 and #14", {12}, {12, 13, 14}), + # forms GitHub does not close: linked (the check may fail on them) but never an exemption + ("Fixes#12", set(), {12}), + ("Resolves: #12", set(), {12}), + ("Fixes https://github.com/HodeTech/ForgeLM/issues/12", set(), {12}), ("Refs #12", set(), {12}), ("references #12 & #13", set(), {12, 13}), - ("Fixes HodeTech/ForgeLM#12", {12}, {12}), - ("Fixes https://github.com/HodeTech/ForgeLM/issues/12", {12}, {12}), ("Fixes other/repo#12 and https://github.com/other/repo/issues/13", set(), set()), - ("FIXES #12", {12}, {12}), - ("Fixes#12", {12}, {12}), # GitHub would not close it; a blocking check errs on the safe side - ("See #12 for context; prefixes #13 are not keywords", set(), set()), + ("See #12 for context; prefixes #13 are not keywords; refsfoo/bar#14", set(), set()), + ("Fixes\n#12", set(), set()), # a keyword never reaches across a line ("\n`Refs #13`\n```\nFixes #14\n```\nRefs #15", set(), {15}), ], ) @@ -129,14 +143,51 @@ def test_the_pull_request_template_itself_links_nothing() -> None: assert tool.linked_issues(template, REPO) == (frozenset(), frozenset()) +@pytest.mark.parametrize( + "body, every", + [ + ("Refs #1\n```\nFixes #2\nRefs #3", {1}), # an unclosed fence runs to the end + ("~~~~\nFixes #2\n~~~\nstill code\n~~~~\nRefs #3", {3}), # a closing fence is at least as long + ("```\nFixes #2\n~~~\nRefs #3", set()), # and of the same character + ("```py `x`\nRefs #3\n```", {3}), # a backtick fence's info string has no backtick: not a fence + (" ```\nFixes #2\nRefs #3", {2, 3}), # four spaces: not a fence opener + ("Refs #1 Refs #3", {1, 3}), + ("Refs #1 ` took about 2 s here + "```x\n" * 15_000, # openers that never close: the earlier fence regex took about 5 s here + "```\n" * 15_000, + "~~~~\n```\n" * 7_500, + "Fixes " * 10_000 + "#1", + "Fixes" + " " * 60_000 + "x", + "Fixes #1, " * 6_000, + "`" * 60_000, + "```\n" * 5_000, + ], +) +def test_a_hostile_description_is_read_in_linear_time(body: str) -> None: + # docs/standards/regex.md: a generous absolute floor; a quadratic reader blows far past it at this size. + start = time.perf_counter() + tool.linked_issues(body, REPO) + assert time.perf_counter() - start < 1.0 + + # --- the pull-request rule ------------------------------------------------------------------------------------- def verdict( - body: str, links: Dict[int, Optional[Tuple[tool.Blocker, ...]]], labels: Tuple[str, ...] = () + body: str, links: Dict[int, Optional[Tuple[tool.Blocker, ...]]], override: Optional[bool] = None ) -> tool.Verdict: closing, linked = tool.linked_issues(body, REPO) - return tool.check_pull_request(frozenset(labels), closing, linked, lambda n: links.get(n, ()), REPO) + return tool.check_pull_request(override, closing, linked, lambda n: links.get(n, ()), REPO) def test_a_pull_request_on_a_blocked_issue_fails_and_names_the_blocker() -> None: @@ -152,8 +203,9 @@ def test_closing_the_blocker_in_the_same_pull_request_passes() -> None: assert verdict("Fixes #1\nFixes #5", {5: (blocker(1),), 1: ()}).ok -def test_only_referring_to_the_blocker_does_not_count_as_closing_it() -> None: - assert not verdict("Fixes #5, refs #1", {5: (blocker(1),), 1: ()}).ok +@pytest.mark.parametrize("body", ["Fixes #5, refs #1", "Fixes #5\nFixes#1", "Fixes #5\nFixes: #1", "Fixes #5, #1"]) +def test_a_link_github_would_not_close_never_exempts_the_blocker(body: str) -> None: + assert not verdict(body, {5: (blocker(1),), 1: ()}).ok def test_a_blocker_closed_without_being_completed_fails_even_if_named() -> None: @@ -166,86 +218,133 @@ def test_completed_blockers_pull_requests_and_closed_issues_pass() -> None: assert v.ok and v.lines == ("No linked issue is blocked (1 issue(s) checked).",) -def test_the_override_label_passes_and_says_so() -> None: - v = verdict("Fixes #5", {5: (blocker(1),)}, labels=("dependency-override",)) - assert v.ok and "dependency-override" in v.lines[0] +def test_a_maintainers_override_passes_and_says_so() -> None: + v = verdict("Fixes #5", {5: (blocker(1),)}, override=True) + assert v.ok and "set by a maintainer" in v.lines[0] + + +def test_an_override_set_by_anyone_else_is_ignored_and_says_so() -> None: + v = verdict("Fixes #5", {5: (blocker(1),)}, override=False) + assert not v.ok and any("is ignored" in line for line in v.lines) + + +def label_event(kind: str, login: str, label: str = "dependency-override") -> Dict[str, Any]: + return {"event": kind, "actor": {"login": login}, "label": {"name": label}} + + +@pytest.mark.parametrize( + "events, authorized", + [ + ([label_event("labeled", "cemililik")], True), + ([label_event("labeled", "CemilIlik")], True), + ([label_event("labeled", "contributor")], False), + ([label_event("labeled", "cemililik"), label_event("unlabeled", "x"), label_event("labeled", "x")], False), + ([label_event("labeled", "cemililik"), label_event("unlabeled", "cemililik")], False), + ([label_event("labeled", "contributor", label="bug"), label_event("labeled", "cemililik")], True), + ([], False), + ], +) +def test_the_override_counts_only_when_a_maintainer_set_it_last(events: List[Dict[str, Any]], authorized: bool) -> None: + gh = FakeGitHub({}) + gh.events[30] = events + assert tool.override_authorized(gh, REPO, 30) is authorized # --- a fake GitHub API ----------------------------------------------------------------------------------------- class FakeGitHub: - """Plays the endpoints the tool calls: GraphQL for issues and their links, REST for writes.""" + """Plays the endpoints the tool calls: GraphQL for issues and their links, REST for the rest.""" def __init__(self, issues: Dict[int, Dict[str, Any]], closed_labelled: Tuple[int, ...] = ()) -> None: - self.issues = issues # number -> {"labels": [...], "blockers": [(n, state, reason), ...], "state": "OPEN"} + # number -> {"labels": [...], "blockers": [(n, state, reason[, repo]), ...], "state": "OPEN", "type": "Issue"} + self.issues = issues + self.fresh: Dict[int, Dict[str, Any]] = {} # what a re-read of one issue sees, when it differs self.closed_labelled = list(closed_labelled) self.calls: List[Tuple[str, str, Any]] = [] self.comments: Dict[int, List[Dict[str, Any]]] = {} self.pulls: List[Dict[str, Any]] = [] - self.runs: Dict[str, List[Dict[str, Any]]] = {} - self.fail_prefix: Optional[str] = None + self.check_runs: Dict[str, List[Dict[str, Any]]] = {} + self.events: Dict[int, List[Dict[str, Any]]] = {} + self.fail: List[Tuple[str, str]] = [] # (method, endpoint prefix) - def _node(self, number: int) -> Dict[str, Any]: - i = self.issues[number] + @staticmethod + def _node(number: int, data: Dict[str, Any]) -> Dict[str, Any]: nodes = [ - {"number": n, "state": s, "stateReason": r, "repository": {"nameWithOwner": REPO}} - for n, s, r in i["blockers"] + {"number": b[0], "state": b[1], "stateReason": b[2], "repository": {"nameWithOwner": (b[3:] or (REPO,))[0]}} + for b in data["blockers"] ] return { - "__typename": i.get("type", "Issue"), + "__typename": data.get("type", "Issue"), "number": number, - "state": i.get("state", "OPEN"), - "labels": {"nodes": [{"name": n} for n in i["labels"]]}, + "state": data.get("state", "OPEN"), + "labels": {"totalCount": len(data["labels"]), "nodes": [{"name": n} for n in data["labels"]]}, "blockedBy": {"totalCount": len(nodes), "nodes": nodes}, } + def pull(self, number: int, body: str, labels: Tuple[str, ...] = (), sha: str = SHA) -> None: + self.pulls.append( + {"number": number, "body": body, "labels": [{"name": n} for n in labels], "head": {"sha": sha}} + ) + def __call__(self, method: str, endpoint: str, payload: Any = None) -> Any: self.calls.append((method, endpoint, payload)) - if self.fail_prefix and endpoint.startswith(self.fail_prefix): + if any(m == method and endpoint.startswith(prefix) for m, prefix in self.fail): raise tool.GhError(f"{method} {endpoint} failed") if endpoint == "graphql": assert payload["query"] in tool.QUERIES.values() v = payload["variables"] if "number" in v: - found = v["number"] in self.issues - return {"data": {"repository": {"issueOrPullRequest": self._node(v["number"]) if found else None}}} - open_nodes = [self._node(n) for n, i in sorted(self.issues.items()) if i.get("state", "OPEN") == "OPEN"] - return {"data": {"repository": {"issues": {"nodes": open_nodes, "pageInfo": {"hasNextPage": False}}}}} + data = self.fresh.get(v["number"], self.issues.get(v["number"])) + return {"data": {"repository": {"issueOrPullRequest": self._node(v["number"], data) if data else None}}} + nodes = [ + self._node(n, i) + for n, i in sorted(self.issues.items()) + if i.get("state", "OPEN") == "OPEN" and i.get("type", "Issue") == "Issue" + ] + return {"data": {"repository": {"issues": {"nodes": nodes, "pageInfo": {"hasNextPage": False}}}}} + number = int(endpoint.split("/")[4]) if re.match(rf"repos/{REPO}/(issues|pulls)/\d+", endpoint) else None if method == "GET" and endpoint.startswith(f"repos/{REPO}/issues?state=closed&labels=blocked"): return [{"number": n} for n in self.closed_labelled] - if method == "GET" and re.fullmatch(rf"repos/{REPO}/issues/\d+/comments\?.*", endpoint): - return self.comments.get(int(endpoint.split("/")[4]), []) + if method == "GET" and "/comments?" in endpoint: + return self.comments.get(number, []) + if method == "GET" and "/events?" in endpoint: + return self.events.get(number, []) if method == "POST" and endpoint.endswith("/labels"): - self.issues.setdefault(int(endpoint.split("/")[4]), {"labels": [], "blockers": []})["labels"] += payload[ - "labels" - ] + self.issues.setdefault(number, {"labels": [], "blockers": []})["labels"] += payload["labels"] return [] if method == "DELETE" and "/labels/" in endpoint: - n = int(endpoint.split("/")[4]) - if n in self.issues: - self.issues[n]["labels"].remove("blocked") + if number in self.issues: + self.issues[number]["labels"].remove("blocked") return None if method == "POST" and endpoint.endswith("/comments"): - self.comments.setdefault(int(endpoint.split("/")[4]), []).append( - {"user": {"type": "Bot"}, "body": payload["body"]} - ) + self.comments.setdefault(number, []).append({"user": {"type": "Bot"}, "body": payload["body"]}) return {} if method == "GET" and endpoint.startswith(f"repos/{REPO}/pulls?state=open"): - return self.pulls + page = int(endpoint.rsplit("page=", 1)[1]) + return self.pulls[(page - 1) * 100 : page * 100] if method == "GET" and re.fullmatch(rf"repos/{REPO}/pulls/\d+", endpoint): - return next(p for p in self.pulls if p["number"] == int(endpoint.rsplit("/", 1)[1])) - if method == "GET" and "/actions/workflows/pr-dependencies.yml/runs?head_sha=" in endpoint: - return {"workflow_runs": self.runs.get(endpoint.split("head_sha=")[1].split("&")[0], [])} - if method == "POST" and endpoint.endswith("/rerun-failed-jobs"): + return next(p for p in self.pulls if p["number"] == number) + if method == "GET" and "/check-runs?check_name=Issue%20dependencies" in endpoint: + return {"check_runs": self.check_runs.get(endpoint.split("/")[4], [])} + if method == "POST" and re.fullmatch(rf"repos/{REPO}/actions/jobs/\d+/rerun", endpoint): return None raise AssertionError(f"unexpected call {method} {endpoint}") def writes(self) -> List[Tuple[str, str]]: return [(m, e) for m, e, _ in self.calls if m in ("POST", "DELETE") and e != "graphql"] + def reruns(self) -> List[str]: + return [e for _, e in self.writes() if e.endswith("/rerun")] -SHA = "a" * 40 + +def run( + job_id: int, conclusion: Optional[str], status: str = "completed", app: str = "github-actions" +) -> Dict[str, Any]: + return {"id": job_id, "status": status, "conclusion": conclusion, "app": {"slug": app}} + + +# --- sync ------------------------------------------------------------------------------------------------------ def test_sync_labels_blocked_issues_and_unblocks_finished_ones() -> None: @@ -263,66 +362,149 @@ def test_sync_labels_blocked_issues_and_unblocks_finished_ones() -> None: gh.issues[5]["labels"] == ["blocked"] and gh.issues[6]["labels"] == [] and gh.issues[7]["labels"] == ["blocked"] ) assert set(gh.comments) == {6} and gh.comments[6][0]["body"].startswith("This issue is no longer blocked") - assert (("DELETE", f"repos/{REPO}/issues/40/labels/blocked")) in gh.writes() # a closed issue waits for nothing + assert ("DELETE", f"repos/{REPO}/issues/40/labels/blocked") in gh.writes() # a closed issue waits for nothing assert 40 not in gh.comments def test_a_dry_run_reads_but_never_writes(capsys: pytest.CaptureFixture[str]) -> None: gh = FakeGitHub({5: {"labels": [], "blockers": [(1, "OPEN", None)]}, 6: {"labels": ["blocked"], "blockers": []}}) + gh.pull(30, "Fixes #5") + gh.check_runs[SHA] = [run(77, "success")] assert tool.run_sync(REPO, gh, dry_run=True) == 0 - assert gh.writes() == [] and "would label #5 blocked" in capsys.readouterr().out + out = capsys.readouterr().out + assert gh.writes() == [] and "would label #5 blocked" in out and "would re-run" in out + + +def test_a_blocker_reopened_since_the_snapshot_does_not_unblock_the_issue() -> None: + gh = FakeGitHub({6: {"labels": ["blocked"], "blockers": [(2, "CLOSED", "COMPLETED")]}}) + gh.fresh[6] = {"labels": ["blocked"], "blockers": [(2, "OPEN", "REOPENED")]} + assert tool.run_sync(REPO, gh) == 0 + assert gh.writes() == [] and gh.issues[6]["labels"] == ["blocked"] + + +def test_the_notice_goes_first_so_a_failed_label_removal_is_finished_without_a_second_notice() -> None: + gh = FakeGitHub({6: {"labels": ["blocked"], "blockers": [(2, "CLOSED", "COMPLETED")]}}) + gh.fail = [("DELETE", f"repos/{REPO}/issues/6/labels")] + with pytest.raises(tool.GhError): + tool.run_sync(REPO, gh) + assert len(gh.comments[6]) == 1 and gh.issues[6]["labels"] == ["blocked"] + gh.fail = [] + assert tool.run_sync(REPO, gh) == 0 + assert len(gh.comments[6]) == 1 and gh.issues[6]["labels"] == [] def test_a_cancelled_blocker_is_flagged_once_and_a_persons_copy_of_the_marker_does_not_silence_it() -> None: gh = FakeGitHub({5: {"labels": ["blocked"], "blockers": [(1, "CLOSED", "NOT_PLANNED")]}}) - gh.comments[5] = [{"user": {"type": "User"}, "body": tool.CANCELLED_MARKER.format(blocker=1)}] + gh.comments[5] = [ + {"user": {"type": "User"}, "body": tool.NOT_COMPLETED_MARKER.format(blocker="hodetech/forgelm#1")} + ] assert tool.run_sync(REPO, gh) == 0 assert gh.issues[5]["labels"] == ["blocked", "needs-triage"] and len(gh.comments[5]) == 2 assert tool.run_sync(REPO, gh) == 0 # the next run finds the bot's notice and stays quiet assert len(gh.comments[5]) == 2 and gh.issues[5]["labels"].count("needs-triage") == 1 -def test_unblocking_reruns_the_failed_check_of_a_pull_request_that_links_the_issue() -> None: - gh = FakeGitHub({6: {"labels": ["blocked"], "blockers": [(2, "CLOSED", "COMPLETED")]}}) - gh.pulls = [ - {"number": 30, "body": "Fixes #6", "head": {"sha": SHA}}, - {"number": 31, "body": "Fixes #9", "head": {"sha": SHA}}, - ] - gh.runs[SHA] = [{"id": 77, "conclusion": "failure"}] +def test_cancelled_blockers_with_one_number_in_two_repositories_each_get_a_notice() -> None: + gh = FakeGitHub( + { + 5: { + "labels": ["blocked"], + "blockers": [(7, "CLOSED", "NOT_PLANNED"), (7, "CLOSED", "DUPLICATE", "HodeTech/Other")], + } + } + ) assert tool.run_sync(REPO, gh) == 0 - assert ("POST", f"repos/{REPO}/actions/runs/77/rerun-failed-jobs") in gh.writes() - assert sum(1 for m, e in gh.writes() if e.endswith("/rerun-failed-jobs")) == 1 + bodies = [c["body"] for c in gh.comments[5]] + assert len(bodies) == 2 and bodies[0].startswith("#7,") and bodies[1].startswith("HodeTech/Other#7,") + + +# --- re-running the pull-request check when it no longer matches ----------------------------------------------- + + +@pytest.mark.parametrize( + "blockers, last, rerun", + [ + ([(2, "OPEN", None)], run(77, "success"), True), # green, but the linked issue became blocked + ([(2, "CLOSED", "COMPLETED")], run(77, "failure"), True), # red, but its blocker is completed now + ([(2, "OPEN", None)], run(77, "failure"), False), # red and still blocked + ([(2, "CLOSED", "COMPLETED")], run(77, "success"), False), # green and free + ([(2, "CLOSED", "COMPLETED")], run(77, None, status="in_progress"), False), # still running: next sync looks + ], +) +def test_the_sync_reruns_a_check_whose_result_no_longer_matches(blockers: list, last: dict, rerun: bool) -> None: + gh = FakeGitHub({6: {"labels": ["blocked"] if blockers[0][1] == "OPEN" else [], "blockers": blockers}}) + gh.pull(30, "Fixes #6") + gh.check_runs[SHA] = [run(70, "failure"), last] # the latest run is the one with the highest id + assert tool.run_sync(REPO, gh) == 0 + assert gh.reruns() == ([f"repos/{REPO}/actions/jobs/77/rerun"] if rerun else []) + + +def test_checks_from_another_app_and_pull_requests_that_link_nothing_are_left_alone() -> None: + gh = FakeGitHub({6: {"labels": ["blocked"], "blockers": [(2, "OPEN", None)]}}) + gh.pull(30, "Fixes #6") + gh.pull(31, "Docs only", sha="b" * 40) + gh.check_runs[SHA] = [run(77, "success", app="someone-else")] + gh.check_runs["b" * 40] = [run(78, "success")] + assert tool.run_sync(REPO, gh) == 0 + assert gh.reruns() == [] + + +def test_every_page_of_open_pull_requests_is_reconciled() -> None: + gh = FakeGitHub({6: {"labels": ["blocked"], "blockers": [(2, "OPEN", None)]}}) + for n in range(100): + gh.pull(1000 + n, "Docs only", sha=f"{n:040x}") + gh.pull(30, "Fixes #6") + gh.check_runs[SHA] = [run(77, "success")] + assert tool.run_sync(REPO, gh) == 0 + assert gh.reruns() == [f"repos/{REPO}/actions/jobs/77/rerun"] def test_a_failing_rerun_is_a_warning_not_a_failed_sync(capsys: pytest.CaptureFixture[str]) -> None: - gh = FakeGitHub({6: {"labels": ["blocked"], "blockers": []}}) - gh.fail_prefix = f"repos/{REPO}/pulls" + gh = FakeGitHub({6: {"labels": ["blocked"], "blockers": [(2, "CLOSED", "COMPLETED")]}}) + gh.pull(30, "Fixes #6") + gh.check_runs[SHA] = [run(77, "failure")] + gh.fail = [("POST", f"repos/{REPO}/actions/jobs/77/rerun")] # the listing works; only the re-run fails assert tool.run_sync(REPO, gh) == 0 - assert gh.issues[6]["labels"] == [] and "warning: could not re-run" in capsys.readouterr().err + assert gh.issues[6]["labels"] == [] and "warning: could not reconcile" in capsys.readouterr().err + assert ("POST", f"repos/{REPO}/actions/jobs/77/rerun") in [(m, e) for m, e, _ in gh.calls] def test_a_failing_read_fails_the_sync() -> None: gh = FakeGitHub({}) - gh.fail_prefix = "graphql" + gh.fail = [("POST", "graphql")] with pytest.raises(tool.GhError): tool.run_sync(REPO, gh) -def test_more_links_than_one_page_is_an_error_not_a_silent_truncation() -> None: - node = {"number": 5, "blockedBy": {"totalCount": 51, "nodes": []}} - with pytest.raises(tool.GhError, match="more than 50 blockers"): - tool._blockers(node) +@pytest.mark.parametrize("field, limit", [("blockedBy", "more than 50 blockers"), ("labels", "more than 100 labels")]) +def test_more_links_or_labels_than_one_page_is_an_error_not_a_silent_truncation(field: str, limit: str) -> None: + node = FakeGitHub._node(5, {"labels": ["blocked"], "blockers": [(1, "OPEN", None)]}) + node[field]["totalCount"] = 101 + with pytest.raises(tool.GhError, match=limit): + tool._issue_state(node, True) + + +# --- check-pr -------------------------------------------------------------------------------------------------- def test_check_pr_reads_the_pull_request_again_instead_of_trusting_the_event() -> None: gh = FakeGitHub({5: {"labels": [], "blockers": [(1, "OPEN", None)]}, 1: {"labels": [], "blockers": []}}) - gh.pulls = [{"number": 30, "body": "Fixes #5", "labels": []}] + gh.pull(30, "Fixes #5") stale = {"pull_request": {"number": 30, "body": "Docs only", "labels": [{"name": "dependency-override"}]}} assert tool.run_check_pr(stale, REPO, gh) == 3 gh.pulls[0]["labels"] = [{"name": "dependency-override"}] + gh.events[30] = [label_event("labeled", "cemililik")] assert tool.run_check_pr(stale, REPO, gh) == 0 +def test_check_pr_ignores_an_override_label_a_contributor_set(capsys: pytest.CaptureFixture[str]) -> None: + gh = FakeGitHub({5: {"labels": [], "blockers": [(1, "OPEN", None)]}}) + gh.pull(30, "Fixes #5", labels=("dependency-override",)) + gh.events[30] = [label_event("labeled", "contributor")] + assert tool.run_check_pr({"pull_request": {"number": 30}}, REPO, gh) == 3 + assert "is ignored" in capsys.readouterr().out + + def test_check_pr_skips_linked_pull_requests_and_closed_issues() -> None: gh = FakeGitHub( { @@ -330,13 +512,13 @@ def test_check_pr_skips_linked_pull_requests_and_closed_issues() -> None: 6: {"labels": [], "blockers": [(1, "OPEN", None)], "state": "CLOSED"}, } ) - gh.pulls = [{"number": 30, "body": "Refs #5, #6 and #404", "labels": []}] + gh.pull(30, "Refs #5, #6 and #404") assert tool.run_check_pr({"pull_request": {"number": 30}}, REPO, gh) == 0 def test_check_pr_prints_errors_as_annotations_with_numbers_only(capsys: pytest.CaptureFixture[str]) -> None: gh = FakeGitHub({5: {"labels": [], "blockers": [(1, "OPEN", None)]}}) - gh.pulls = [{"number": 30, "body": "Fixes #5 ::set-output name=x::y", "labels": []}] + gh.pull(30, "Fixes #5 ::set-output name=x::y") assert tool.run_check_pr({"pull_request": {"number": 30}}, REPO, gh) == 3 out = capsys.readouterr().out assert out.startswith("::error::#5 is blocked by #1") and "set-output" not in out @@ -364,7 +546,7 @@ def test_main_rejects_a_missing_repository_a_missing_event_and_a_bad_command( def test_main_turns_an_api_failure_into_exit_one(monkeypatch: pytest.MonkeyPatch) -> None: gh = FakeGitHub({}) - gh.fail_prefix = "graphql" + gh.fail = [("POST", "graphql")] monkeypatch.setenv("GITHUB_REPOSITORY", REPO) assert tool.main(["sync"], gh) == 1 @@ -447,6 +629,7 @@ def test_no_expression_reaches_a_shell_and_the_checkout_is_pinned_and_minimal(na assert re.fullmatch(r"actions/checkout@[0-9a-f]{40}", checkout["uses"]) assert checkout["with"]["persist-credentials"] is False assert checkout["with"]["sparse-checkout"] == "tools/issue_dependencies.py" + assert "ref" not in checkout["with"] # the default branch: never a pull request's code def test_the_sync_workflow_triggers_permissions_and_concurrency() -> None: @@ -458,21 +641,30 @@ def test_the_sync_workflow_triggers_permissions_and_concurrency() -> None: } # YAML 1.1 reads the key `on` as a boolean assert wf["permissions"] == {} job = wf["jobs"]["sync"] - assert job["permissions"] == {"contents": "read", "issues": "write", "pull-requests": "read", "actions": "write"} + assert job["permissions"] == { + "contents": "read", + "issues": "write", + "pull-requests": "read", + "checks": "read", + "actions": "write", + } assert wf["concurrency"] == {"group": "issue-dependencies", "cancel-in-progress": False} - assert "ref" not in steps_of(wf)[0]["with"] # the default branch: what `issues` and `schedule` run on assert "github.repository == 'HodeTech/ForgeLM'" in job["if"] assert steps_of(wf)[1]["run"] == "python3 tools/issue_dependencies.py sync" -def test_the_pull_request_check_never_runs_the_pull_requests_own_code() -> None: +def test_the_pull_request_check_takes_its_definition_from_the_default_branch_and_never_runs_pr_code() -> None: wf = load("pr-dependencies.yml") - assert list(wf[True]) == ["pull_request"] # never pull_request_target - assert set(wf[True]["pull_request"]) == {"types"} # no paths/branches filter: a required check must report + text = (ROOT / ".github/workflows/pr-dependencies.yml").read_text(encoding="utf-8") + # `pull_request` would take this file from the pull request, which could then rewrite the check that judges it. + assert list(wf[True]) == ["pull_request_target"] + assert set(wf[True]["pull_request_target"]) == {"types"} # no paths/branches filter: a required check must report + assert {"edited", "labeled", "unlabeled", "synchronize"} <= set(wf[True]["pull_request_target"]["types"]) assert wf["permissions"] == {} job = wf["jobs"]["issue-dependencies"] - assert job["name"] == "Issue dependencies" # the required check's context + assert job["name"] == tool.CHECK_NAME == "Issue dependencies" # the required check's context assert job["permissions"] == {"contents": "read", "issues": "read", "pull-requests": "read"} - assert steps_of(wf)[0]["with"]["ref"] == "${{ github.event.pull_request.base.sha }}" + code = "\n".join(line.split("#", 1)[0] for line in text.splitlines()) # the YAML without its comments + assert "head" not in code and "github.event.pull_request" not in code + assert code.count("secrets.") == 1 # GITHUB_TOKEN only assert steps_of(wf)[1]["run"] == "python3 tools/issue_dependencies.py check-pr" - assert {"edited", "labeled", "unlabeled", "synchronize"} <= set(wf[True]["pull_request"]["types"]) diff --git a/tools/assign_on_comment.py b/tools/assign_on_comment.py index 563fb795..9deff8f6 100644 --- a/tools/assign_on_comment.py +++ b/tools/assign_on_comment.py @@ -9,8 +9,9 @@ - only issues labelled ``good first issue`` are open to ``/assign``, and never one that also carries ``security``, ``needs-triage``, ``deferred``, ``epic`` or ``phase: 16``; -- a blocked issue is refused: one labelled ``blocked``, or one GitHub still lists an open "Blocked by" issue for - (the label follows the links hourly, so the count closes the gap; ``tools/issue_dependencies.py``); +- a blocked issue is refused: one labelled ``blocked``, or one with a "Blocked by" issue that is still open or was + closed without being completed (the label follows the links within the hour, so reading them closes the gap; + ``tools/issue_dependencies.py``); - an issue that already has an assignee is refused, whoever the assignee is and however they were assigned: a later ``/assign`` never replaces, adds to or removes an existing assignment; - a contributor holds at most ``MAX_OPEN_ASSIGNMENTS`` open issues at a time (the maintainers in @@ -79,8 +80,8 @@ MSG_CLOSED = "This issue is closed, so it cannot be assigned." MSG_BLOCKED = ( "This issue is blocked: it waits for another issue to be completed (see **Blocked by** in the sidebar), so it " - "cannot be claimed yet. The `blocked` label is removed automatically once every blocker is closed as completed, " - "and the issue says so in a comment. Until then, pick another one from the [Front door view]({front_door})." + "cannot be claimed yet. The `blocked` label is removed automatically once every blocker is closed as completed; " + "check this issue again then. Until then, pick another one from the [Front door view]({front_door})." ) MSG_NOT_OPEN = ( "Only `good first issue` issues can be claimed with `/assign`; this one needs a maintainer's decision. " @@ -162,7 +163,7 @@ class IssueFacts: state: str labels: frozenset[str] assignees: tuple[str, ...] - open_blockers: int = 0 # GitHub's count of open "Blocked by" issues + unresolved_blockers: int = 0 # "Blocked by" issues still open or closed without being completed @dataclass(frozen=True) @@ -231,7 +232,7 @@ def decide(ctx: Context, issue: IssueFacts, count_open_assignments: Callable[[], return Decision("reply", MSG_NOT_YOURS) if issue.state != "open": return Decision("reply", MSG_CLOSED) - if BLOCKED_LABEL in issue.labels or issue.open_blockers > 0: + if BLOCKED_LABEL in issue.labels or issue.unresolved_blockers > 0: return Decision("reply", MSG_BLOCKED.format(front_door=FRONT_DOOR_URL)) if issue.labels & EXCLUDED_LABELS or not issue.labels & ELIGIBLE_LABELS: return Decision("reply", MSG_NOT_OPEN.format(mention=MAINTAINER_MENTION), pings=True) @@ -318,14 +319,36 @@ def fetch_issue(gh: GhApi, repo: str, number: int) -> IssueFacts: try: labels = frozenset(str(label["name"]) for label in raw["labels"]) summary = raw.get("issue_dependencies_summary") - # The summary's `blocked_by` counts only blockers that are still open. Without the summary the label alone - # decides, which is what tools/issue_dependencies.py keeps true. - open_blockers = int(summary.get("blocked_by") or 0) if isinstance(summary, dict) else 0 - return IssueFacts(str(raw["state"]), labels, _logins(raw["assignees"]), open_blockers) + # The summary's `blocked_by` counts only blockers that are still open. A blocker closed as not planned or as + # a duplicate blocks too, so when closed ones exist and the label has not caught up, read their reasons. + # Without the summary the label alone decides, which is what tools/issue_dependencies.py keeps true. + unresolved = 0 + if isinstance(summary, dict): + unresolved = int(summary.get("blocked_by") or 0) + if int(summary.get("total_blocked_by") or 0) > unresolved and BLOCKED_LABEL not in labels: + unresolved += count_not_completed_blockers(gh, repo, number) + return IssueFacts(str(raw["state"]), labels, _logins(raw["assignees"]), unresolved) except (KeyError, TypeError, ValueError) as exc: raise GhError("the issue answer lacks labels, state or assignees") from exc +def count_not_completed_blockers(gh: GhApi, repo: str, number: int) -> int: + """The issue's "Blocked by" issues that are closed without being completed (not planned, or a duplicate).""" + found = 0 + for page in range(1, _MAX_PAGES + 1): + raw = gh("GET", f"repos/{repo}/issues/{number}/dependencies/blocked_by?per_page={_PAGE_SIZE}&page={page}", None) + if not isinstance(raw, list): + raise GhError("the blocked-by answer is not a list") + found += sum( + 1 + for b in raw + if isinstance(b, dict) and b.get("state") == "closed" and b.get("state_reason") not in (None, "completed") + ) + if len(raw) < _PAGE_SIZE: + break + return found + + def count_open_assignments(gh: GhApi, repo: str, login: str, stop_at: Optional[int] = None) -> int: """Open issues (not pull requests) assigned to ``login`` in this repository, across pages. diff --git a/tools/issue_dependencies.py b/tools/issue_dependencies.py index 4368bd6b..caa3f4ce 100644 --- a/tools/issue_dependencies.py +++ b/tools/issue_dependencies.py @@ -4,7 +4,8 @@ Two workflows run this script: - ``.github/workflows/issue-dependencies.yml`` runs ``sync`` when an issue is closed or reopened, hourly, and on - demand. It brings the ``blocked`` label of every issue in line with its native "Blocked by" links. + demand. It brings the ``blocked`` label of every issue in line with its native "Blocked by" links, and re-runs the + "Issue dependencies" check of every open pull request whose last result no longer matches its linked issues. - ``.github/workflows/pr-dependencies.yml`` runs ``check-pr`` on every pull-request change. It fails while the pull request links an issue that is still blocked. @@ -20,16 +21,23 @@ removes it everywhere else, from a closed issue and from one where someone set it by hand. When it removes the label from an open issue it posts one comment; it does not comment when an issue becomes blocked. - ``check-pr`` fails when the pull request's description links a blocked issue with ``Fixes``, ``Closes``, - ``Resolves`` or ``Refs`` — unless the same pull request closes every open blocker (``Fixes``, ``Closes`` or - ``Resolves``), or a maintainer has labelled it ``dependency-override``. It reads the links themselves, not the - label, so it never trails behind ``sync``. When ``sync`` unblocks an issue it re-runs a failed check on the open - pull requests that link it. + ``Resolves`` or ``Refs`` — unless the same pull request closes every open blocker, or a maintainer listed in + ``MAINTAINER_LOGINS`` has labelled it ``dependency-override`` (the label set by anyone else is ignored). It reads + the links themselves, not the label, so it never trails behind ``sync``. + +Reading a description. A *link* is read leniently (a colon or a missing space after the keyword still counts), +because reading too many links can only make the check stricter. A *closing* link is read exactly as GitHub closes +issues — ``Fixes #12`` or ``Fixes owner/repo#12``, the keyword followed by whitespace, one keyword per issue — because +it exempts a blocker, and an exemption GitHub would not honour lets blocked work through. Fenced code, inline code +and HTML comments are skipped (the template's own examples live there); an unclosed fence or comment runs to the end, +as in CommonMark. The check sees what a description declares: it cannot see work on an issue the description does not +link, so it guards against honest mistakes, and a maintainer's review remains the backstop. Trust boundary. Issue and pull-request text is written by anyone, so the script never executes or prints it: it -parses the description with regular expressions and writes only fixed messages and issue numbers to the log. The -workflows pass nothing through the shell; the script reads the event file itself. Every subprocess call is a list -without a shell, limited to fixed endpoints (``repos/`` paths built from integers, and ``graphql`` with a query -constant from this module). +parses the description in linear time and writes only fixed messages and issue numbers to the log. The workflows pass +nothing through the shell; the script reads the event file itself. Every subprocess call is a list without a shell, +limited to fixed endpoints (``repos/`` paths built from integers, and ``graphql`` with a query constant from this +module). Exit codes (per ``tools/`` contract, not the public 0-6 surface of ``forgelm/``): @@ -54,15 +62,17 @@ BLOCKED_LABEL = "blocked" OVERRIDE_LABEL = "dependency-override" TRIAGE_LABEL = "needs-triage" -PR_WORKFLOW_FILE = "pr-dependencies.yml" -# Hidden marker in the "a blocker was not completed" notice, one per blocker. Only a bot's comment counts, so a -# person cannot forge it to silence the notice. -CANCELLED_MARKER = "" +CHECK_NAME = "Issue dependencies" # the job name in pr-dependencies.yml, and the required check's context +# Hidden markers that make a notice idempotent: only a bot's comment counts, so a person cannot forge one to silence +# a notice. `{blocker}` / `{blockers}` hold `owner/repo#N` references, so blockers in two repositories never collide. +NOT_COMPLETED_MARKER = "" +UNBLOCKED_MARKER = "" _GH_TIMEOUT_SECONDS = 60 _PAGE_SIZE = 100 -_MAX_PAGES = 30 # 3,000 issues: far above the backlog; bounds every paginated read +_MAX_PAGES = 30 # 3,000 items: far above the backlog; bounds every paginated read _MAX_BLOCKERS = 50 # GitHub shows at most this many "Blocked by" links per issue +_MAX_LABELS = 100 # GitHub allows at most this many labels per issue _ALLOWED_METHODS = frozenset({"GET", "POST", "DELETE"}) MSG_UNBLOCKED = ( @@ -74,16 +84,17 @@ "and is set and removed automatically; see [Dependencies between issues]({contributing})." ) MSG_NOT_COMPLETED = ( - "#{blocker}, which this issue is blocked by, was closed as {reason}, not as completed, so the work this issue " + "{blocker}, which this issue is blocked by, was closed as {reason}, not as completed, so the work this issue " "waits for was not done and the issue stays blocked. {mention}, please re-plan it: remove the link if the " - "dependency no longer holds, or link the issue that replaces #{blocker}.\n\n{marker}" + "dependency no longer holds, or link the issue that replaces {blocker}." ) MAINTAINER_LOGINS = frozenset({"cemililik"}) # MAINTAINERS.md; tests/test_issue_dependencies.py keeps them equal MAINTAINER_MENTION = " ".join(f"@{login}" for login in sorted(MAINTAINER_LOGINS)) # A reference in a pull-request description: `#12`, `owner/repo#12` or an issue URL, after a keyword. GitHub closes -# an issue for close/fix/resolve and their inflections; `Refs` is this project's word for "part of". +# an issue for close/fix/resolve and their inflections; `Refs` is this project's word for "part of". Every class is +# explicit ASCII (GitHub's keywords, logins and repository names are ASCII; docs/standards/regex.md rule 1). _KEYWORD = r"(?Pclose[sd]?|fix(?:e[sd])?|resolve[sd]?|refs?|references)" _OWNER, _NAME = r"[A-Za-z0-9-]+", r"[A-Za-z0-9._-]+" _REF_RE = re.compile( @@ -91,16 +102,21 @@ rf"|https://github\.com/(?P{_OWNER})/(?P{_NAME})/issues/(?P\d+)" ) _REF_PLAIN = rf"(?:(?:{_OWNER}/{_NAME})?#\d+|https://github\.com/{_OWNER}/{_NAME}/issues/\d+)" -# Over-matching is the safe direction for a blocking check, so a colon or a missing space after the keyword counts. +# Horizontal whitespace only (rule 5), and the optional colon owns the whitespace after it, so no two quantifiers +# compete for the same characters (rule 4). _LINK_RE = re.compile( - rf"(?{_REF_PLAIN}(?:\s*(?:,|and|&)\s*{_REF_PLAIN})*)", re.IGNORECASE + rf"(?[ \t]*(?::[ \t]*)?)" + rf"(?P{_REF_PLAIN}(?:[ \t]*(?:,|and|&)[ \t]*{_REF_PLAIN})*)", + re.IGNORECASE, ) _CLOSING = frozenset({"close", "closes", "closed", "fix", "fixes", "fixed", "resolve", "resolves", "resolved"}) -_FENCE_RE = re.compile(r"^ {0,3}(```|~~~).*?^ {0,3}\1[^\S\n]*$", re.MULTILINE | re.DOTALL) -_HTML_COMMENT_RE = re.compile(r"", re.DOTALL) -_INLINE_CODE_RE = re.compile(r"`[^`\n]*`") +_FENCE_RE = re.compile(r" {0,3}(`{3,}|~{3,})") # matched at the start of one line by the walker below +_INLINE_CODE_RE = re.compile(r"`[^`\n]*`") # stops at the next backtick or newline: linear # Every GraphQL document this script may send. A query is chosen by name; only integer and cursor variables vary. +_ISSUE_FIELDS = """ + labels(first: 100) { totalCount nodes { name } } + blockedBy(first: 50) { totalCount nodes { number state stateReason repository { nameWithOwner } } }""" QUERIES = { "open_issues": """ query($owner: String!, $name: String!, $after: String) { @@ -108,9 +124,9 @@ issues(states: OPEN, first: 100, after: $after) { pageInfo { hasNextPage endCursor } nodes { - number - labels(first: 50) { nodes { name } } - blockedBy(first: 50) { totalCount nodes { number state stateReason repository { nameWithOwner } } } + number""" + + _ISSUE_FIELDS + + """ } } } @@ -122,9 +138,9 @@ __typename ... on Issue { number - state - labels(first: 50) { nodes { name } } - blockedBy(first: 50) { totalCount nodes { number state stateReason repository { nameWithOwner } } } + state""" + + _ISSUE_FIELDS + + """ } } } @@ -166,6 +182,11 @@ def status(self) -> str: return "resolved" return "not-completed" + @property + def key(self) -> str: + """``owner/repo#N``, lower-cased: identifies the blocker across repositories.""" + return f"{self.repo.lower()}#{self.number}" + def ref(self, home: str) -> str: return f"#{self.number}" if self.repo.lower() == home.lower() else f"{self.repo}#{self.number}" @@ -182,12 +203,14 @@ class IssueState: @dataclass(frozen=True) class Plan: - """What ``sync`` does to one issue. ``comment`` is the unblocked notice; ``not_completed`` lists blockers that - were closed without being completed (each gets one notice and ``needs-triage``).""" + """What ``sync`` does to one issue. ``comment`` is the unblocked notice and ``marker`` its idempotency key; + ``not_completed`` lists blockers that were closed without being completed (each gets one notice and + ``needs-triage``).""" add_label: bool = False remove_label: bool = False comment: str = "" + marker: str = "" not_completed: tuple[Blocker, ...] = () @property @@ -217,47 +240,91 @@ def plan_issue(issue: IssueState, repo: str) -> Plan: if issue.blockers: refs = ", ".join(b.ref(repo) for b in issue.blockers) message = MSG_UNBLOCKED.format(blockers=refs, contributing=contributing_url(repo, "working-on-an-issue")) + marker = UNBLOCKED_MARKER.format(blockers=",".join(sorted(b.key for b in issue.blockers))) else: message = MSG_UNBLOCKED_NO_LINKS.format(contributing=contributing_url(repo)) - return Plan(remove_label=True, comment=message) + marker = UNBLOCKED_MARKER.format(blockers="none") + return Plan(remove_label=True, comment=f"{message}\n\n{marker}", marker=marker) -def not_completed_notice(blocker: Blocker) -> str: +def not_completed_notice(blocker: Blocker, repo: str) -> str: reason = "a duplicate" if blocker.reason == "DUPLICATE" else "not planned" - return MSG_NOT_COMPLETED.format( - blocker=blocker.number, - reason=reason, - mention=MAINTAINER_MENTION, - marker=CANCELLED_MARKER.format(blocker=blocker.number), - ) + text = MSG_NOT_COMPLETED.format(blocker=blocker.ref(repo), reason=reason, mention=MAINTAINER_MENTION) + return f"{text}\n\n{NOT_COMPLETED_MARKER.format(blocker=blocker.key)}" + + +def _strip_fences(body: str) -> str: + """Drop fenced code blocks with a line walker (linear; docs/standards/regex.md rule 6). + + A fence opens with three or more backticks or tildes after at most three spaces (a backtick fence's info string + may not contain a backtick) and closes on a line holding the same character at least as many times and nothing + else. A fence that never closes runs to the end, as in CommonMark. + """ + kept: list[str] = [] + open_fence: Optional[tuple[str, int]] = None + for line in body.splitlines(): + match = _FENCE_RE.match(line) + if open_fence is None: + if match and not (match.group(1)[0] == "`" and "`" in line[match.end() :]): + open_fence = (match.group(1)[0], len(match.group(1))) + continue + kept.append(line) + elif ( + match + and match.group(1)[0] == open_fence[0] + and len(match.group(1)) >= open_fence[1] + and not line[match.end() :].strip(" \t") + ): + open_fence = None + return "\n".join(kept) + + +def _strip_html_comments(text: str) -> str: + """Drop ```` with ``str.find`` (linear). A comment that never closes runs to the end.""" + out: list[str] = [] + start = 0 + while True: + opening = text.find("", opening + 4) + if closing < 0: + return "".join(out) + start = closing + 3 def _strip_non_prose(body: str) -> str: - """Drop fenced code, inline code and HTML comments: the template's own examples live there.""" - for pattern in (_FENCE_RE, _HTML_COMMENT_RE, _INLINE_CODE_RE): - body = pattern.sub(" ", body) - return body + """Drop fenced code, HTML comments and inline code: the template's own examples live there.""" + return _INLINE_CODE_RE.sub(" ", _strip_html_comments(_strip_fences(body))) def linked_issues(body: str, repo: str) -> tuple[frozenset[int], frozenset[int]]: """``(closing, all)``: issue numbers of this repository linked from a pull-request description. - ``closing`` holds the ones linked with a closing keyword; ``all`` adds those linked with ``Refs``. A reference to - another repository is ignored: this repository's links cannot block on it, and its workflow cannot close it. + ``all`` holds every linked issue, read leniently; ``closing`` the ones GitHub will close on merge, read strictly + (see the module docstring). A reference to another repository is ignored: this repository's links cannot block on + it, and its workflow cannot close it. """ owner, name = repo.lower().split("/") closing: set[int] = set() every: set[int] = set() for match in _LINK_RE.finditer(_strip_non_prose(body or "")): - keyword = match.group("kw").lower() - for ref in _REF_RE.finditer(match.group("refs")): + keyword, sep, refs = match.group("kw").lower(), match.group("sep"), match.group("refs") + if not sep and not refs.startswith("#"): + continue # `refsfoo/bar#1` is a word, not a keyword followed by a reference + # GitHub closes only `KEYWORD #N` / `KEYWORD owner/repo#N`: whitespace after the keyword, no colon, one + # keyword per issue — so only the first reference of a list, and never a URL. + strict = keyword in _CLOSING and bool(sep) and not sep.strip(" \t") + for position, ref in enumerate(_REF_RE.finditer(refs)): ref_owner = (ref.group("owner") or ref.group("uowner") or owner).lower() ref_repo = (ref.group("repo") or ref.group("urepo") or name).lower() if (ref_owner, ref_repo) != (owner, name): continue number = int(ref.group("num") or ref.group("unum")) every.add(number) - if keyword in _CLOSING: + if strict and position == 0 and ref.group("num"): closing.add(number) return frozenset(closing), frozenset(every) @@ -271,16 +338,23 @@ class Verdict: def check_pull_request( - labels: frozenset[str], + override: Optional[bool], closing: frozenset[int], linked: frozenset[int], blockers_of: Callable[[int], Optional[tuple[Blocker, ...]]], repo: str, ) -> Verdict: - """The rules for one pull request. ``blockers_of`` returns an open issue's links, or ``None`` for a pull request, - a closed issue or a number that does not exist (none of those can be blocked work).""" - if OVERRIDE_LABEL in labels: - return Verdict(True, (f"`{OVERRIDE_LABEL}` is set: a maintainer accepted the linked dependencies.",)) + """The rules for one pull request. + + ``override`` is ``None`` without the override label, ``True`` when a maintainer set it and ``False`` when someone + else did (then it is ignored). ``blockers_of`` returns an open issue's links, or ``None`` for a pull request, a + closed issue or a number that does not exist (none of those can be blocked work). + """ + if override: + return Verdict(True, (f"`{OVERRIDE_LABEL}` was set by a maintainer, who accepted the linked dependencies.",)) + notes: tuple[str, ...] = () + if override is False: + notes = (f"`{OVERRIDE_LABEL}` is ignored: only a maintainer listed in MAINTAINERS.md can set it.",) problems: list[str] = [] checked = 0 for number in sorted(linked): @@ -299,12 +373,13 @@ def check_pull_request( return Verdict( False, tuple(problems) + + notes + ( - "Wait until the blocker is closed as completed (this check re-runs then), close it in this pull request " - f"too, or ask a maintainer. See {contributing_url(repo)}", + "Wait until the blocker is closed as completed (this check is re-run within the hour), close it in " + f"this pull request too (`Fixes #N` for each), or ask a maintainer. See {contributing_url(repo)}", ), ) - return Verdict(True, (f"No linked issue is blocked ({checked} issue(s) checked).",)) + return Verdict(True, (f"No linked issue is blocked ({checked} issue(s) checked).",) + notes) # ------------------------------------------------------------------------------------------------ GitHub access @@ -330,7 +405,7 @@ def gh_api(method: str, endpoint: str, payload: Optional[Mapping[str, Any]] = No label = f"{method} {endpoint.split('?')[0]}" try: # A list without a shell; the method and endpoint were validated above, and an endpoint is built only from - # integers, the repository name and fixed path segments. + # integers, the repository name, a validated commit SHA and fixed path segments. proc = subprocess.run( # nosemgrep: python.lang.security.audit.dangerous-subprocess-use-audit command, input=stdin, capture_output=True, text=True, timeout=_GH_TIMEOUT_SECONDS, check=False ) @@ -374,11 +449,21 @@ def _blockers(node: Mapping[str, Any]) -> tuple[Blocker, ...]: def _labels(node: Mapping[str, Any]) -> frozenset[str]: try: - return frozenset(str(label["name"]) for label in node["labels"]["nodes"]) + labels = node["labels"] + if labels["totalCount"] > len(labels["nodes"]): + raise GhError(f"issue #{node['number']} has more than {_MAX_LABELS} labels") + return frozenset(str(label["name"]) for label in labels["nodes"]) except (KeyError, TypeError) as exc: raise GhError("a labels answer lacks a field") from exc +def _issue_state(node: Mapping[str, Any], is_open: bool) -> IssueState: + try: + return IssueState(int(node["number"]), is_open, _labels(node), _blockers(node)) + except (KeyError, TypeError, ValueError) as exc: + raise GhError("an issue answer lacks a field") from exc + + def fetch_open_issues(gh: GhApi, repo: str) -> list[IssueState]: owner, name = repo.split("/") out: list[IssueState] = [] @@ -390,7 +475,7 @@ def fetch_open_issues(gh: GhApi, repo: str) -> list[IssueState]: nodes, info = page["nodes"], page["pageInfo"] except (KeyError, TypeError) as exc: raise GhError("the open-issues answer lacks a field") from exc - out += [IssueState(int(n["number"]), True, _labels(n), _blockers(n)) for n in nodes] + out += [_issue_state(n, True) for n in nodes] if not info.get("hasNextPage"): return out after = str(info["endCursor"]) @@ -414,14 +499,47 @@ def fetch_closed_labelled(gh: GhApi, repo: str) -> list[int]: raise GhError(f"more than {_MAX_PAGES} pages of closed issues") -def fetch_blockers_of(gh: GhApi, repo: str, number: int) -> Optional[tuple[Blocker, ...]]: - """An open issue's links, or ``None`` when ``number`` is a pull request, a closed issue or does not exist.""" +def fetch_issue_state(gh: GhApi, repo: str, number: int) -> Optional[IssueState]: + """One issue as it stands now, or ``None`` when ``number`` is a pull request or does not exist.""" owner, name = repo.split("/") data = graphql(gh, "issue", owner=owner, name=name, number=number) node = (data.get("repository") or {}).get("issueOrPullRequest") - if not isinstance(node, Mapping) or node.get("__typename") != "Issue" or node.get("state") != "OPEN": + if not isinstance(node, Mapping) or node.get("__typename") != "Issue": return None - return _blockers(node) + return _issue_state(node, node.get("state") == "OPEN") + + +def fetch_blockers_of(gh: GhApi, repo: str, number: int) -> Optional[tuple[Blocker, ...]]: + """An open issue's links, or ``None`` when ``number`` is a pull request, a closed issue or does not exist.""" + state = fetch_issue_state(gh, repo, number) + return state.blockers if state is not None and state.is_open else None + + +def override_authorized(gh: GhApi, repo: str, number: int) -> bool: + """True when the latest change of the override label on this pull request is a maintainer adding it. + + Who may *add* a label is a repository permission, not a maintainer decision, so the label alone proves nothing. + The issue events (oldest first) record who added or removed it. + """ + last: Optional[Mapping[str, Any]] = None + for page in range(1, _MAX_PAGES + 1): + raw = gh("GET", f"repos/{repo}/issues/{number}/events?per_page={_PAGE_SIZE}&page={page}", None) + if not isinstance(raw, list): + raise GhError("the events answer is not a list") + for event in raw: + if ( + isinstance(event, Mapping) + and event.get("event") in ("labeled", "unlabeled") + and (event.get("label") or {}).get("name") == OVERRIDE_LABEL + ): + last = event + if len(raw) < _PAGE_SIZE: + break + else: + raise GhError(f"more than {_MAX_PAGES} pages of events") + if last is None or last.get("event") != "labeled": + return False + return str((last.get("actor") or {}).get("login") or "").lower() in MAINTAINER_LOGINS def _already_noticed(gh: GhApi, repo: str, number: int, marker: str) -> bool: @@ -438,50 +556,94 @@ def _already_noticed(gh: GhApi, repo: str, number: int, marker: str) -> bool: return False -def _rerun_pr_checks(gh: GhApi, repo: str, unblocked: set[int]) -> list[int]: - """Re-run a failed dependency check on the open pull requests that link a just-unblocked issue. - - Optional: when it fails, the check re-runs anyway on the pull request's next edit or push, so a failure is - reported as a warning and does not fail the sync. +def _reconcile_one( + gh: GhApi, + repo: str, + pull: Mapping[str, Any], + blockers_of: Callable[[int], Optional[tuple[Blocker, ...]]], + dry_run: bool, +) -> bool: + """Re-run one pull request's dependency check if its last result no longer matches; True when it did.""" + number, sha = int(pull["number"]), str(pull["head"]["sha"]) + if not re.fullmatch(r"[0-9a-f]{40}", sha): + raise GhError("a pull request's head sha is malformed") + labels = frozenset(str(label["name"]) for label in pull["labels"]) + closing, linked = linked_issues(str(pull.get("body") or ""), repo) + override = override_authorized(gh, repo, number) if OVERRIDE_LABEL in labels else None + want_ok = check_pull_request(override, closing, linked, blockers_of, repo).ok + runs = gh("GET", f"repos/{repo}/commits/{sha}/check-runs?check_name={quote(CHECK_NAME)}&per_page=100", None) + candidates = [ + r for r in (runs or {}).get("check_runs") or [] if (r.get("app") or {}).get("slug") == "github-actions" + ] + if not candidates: + return False # the check has not run on this commit; the next pull-request event runs it + latest = max(candidates, key=lambda r: int(r["id"])) + if latest.get("status") != "completed": + return False # still running on what it read; the next sync compares again + if (latest.get("conclusion") == "success") == want_ok: + return False + print(f"{'would re-run' if dry_run else 'did re-run'} the dependency check of pull request #{number}") + if not dry_run: + # An Actions check run's id is its job's id. + gh("POST", f"repos/{repo}/actions/jobs/{int(latest['id'])}/rerun", None) + return True + + +def reconcile_pr_checks( + gh: GhApi, repo: str, open_issues: Mapping[int, IssueState], dry_run: bool = False +) -> tuple[int, int]: + """Re-run the dependency check of every open pull request whose last result no longer matches its linked issues: + red although every blocker is now completed, or green although a linked issue became blocked. + + It compares state on every run instead of reacting to a transition, so a failed re-run, a check that was still + running, or a sync that stopped half-way is simply caught by the next run. Optional by design: a failure is a + warning, never a failed sync. Returns ``(pull requests looked at, checks re-run)``. """ - rerun: list[int] = [] + + def blockers_of(number: int) -> Optional[tuple[Blocker, ...]]: + issue = open_issues.get(number) + return issue.blockers if issue is not None else None + + seen = rerun = 0 try: - for page in range(1, 4): + for page in range(1, _MAX_PAGES + 1): pulls = gh("GET", f"repos/{repo}/pulls?state=open&per_page={_PAGE_SIZE}&page={page}", None) if not isinstance(pulls, list): raise GhError("the pull-request list is not a list") for pull in pulls: - _, linked = linked_issues(str(pull.get("body") or ""), repo) - if not linked & unblocked: - continue - sha = str(pull["head"]["sha"]) - if not re.fullmatch(r"[0-9a-f]{40}", sha): - raise GhError("a pull request's head sha is malformed") - runs = gh( - "GET", f"repos/{repo}/actions/workflows/{PR_WORKFLOW_FILE}/runs?head_sha={sha}&per_page=5", None - ) - latest = (runs or {}).get("workflow_runs") or [] - if latest and latest[0].get("conclusion") == "failure": - gh("POST", f"repos/{repo}/actions/runs/{int(latest[0]['id'])}/rerun-failed-jobs", None) - rerun.append(int(pull["number"])) + seen += 1 + try: + rerun += _reconcile_one(gh, repo, pull, blockers_of, dry_run) + except (GhError, AttributeError, KeyError, TypeError, ValueError) as exc: + print(f"warning: could not reconcile a pull request's check: {_clean(str(exc))}", file=sys.stderr) if len(pulls) < _PAGE_SIZE: break - except (GhError, KeyError, TypeError, ValueError) as exc: - print(f"warning: could not re-run pull-request checks: {_clean(str(exc))}", file=sys.stderr) - return rerun + else: + print(f"warning: more than {_MAX_PAGES} pages of open pull requests; the rest wait", file=sys.stderr) + except (GhError, AttributeError, KeyError, TypeError, ValueError) as exc: + print(f"warning: could not list the open pull requests: {_clean(str(exc))}", file=sys.stderr) + return seen, rerun # ----------------------------------------------------------------------------------------------------- commands def run_sync(repo: str, gh: GhApi, dry_run: bool = False) -> int: - issues = fetch_open_issues(gh, repo) - issues += [IssueState(n, False, frozenset({BLOCKED_LABEL}), ()) for n in fetch_closed_labelled(gh, repo)] - plans = [(issue, plan_issue(issue, repo)) for issue in issues] - blocked = sum(1 for i in issues if i.is_open and is_blocked(i.blockers)) - unblocked: set[int] = set() - for issue, plan in plans: + open_issues = fetch_open_issues(gh, repo) + closed = [IssueState(n, False, frozenset({BLOCKED_LABEL}), ()) for n in fetch_closed_labelled(gh, repo)] + blocked = sum(1 for i in open_issues if is_blocked(i.blockers)) + verb = "would" if dry_run else "did" + unblocked = 0 + for issue in open_issues + closed: + plan = plan_issue(issue, repo) + if plan.remove_label and issue.is_open and not dry_run: + # Read the issue again right before acting on it: a blocker reopened since the snapshot must not lead to + # an "unblocked" notice that a later run cannot take back. + fresh = fetch_issue_state(gh, repo, issue.number) + if fresh is None: + print(f"skipped #{issue.number}: no longer an issue") + continue + issue, plan = fresh, plan_issue(fresh, repo) if not plan.changes: continue - verb = "would" if dry_run else "did" if plan.add_label: print(f"{verb} label #{issue.number} {BLOCKED_LABEL}") if not dry_run: @@ -489,25 +651,26 @@ def run_sync(repo: str, gh: GhApi, dry_run: bool = False) -> int: if plan.remove_label: print(f"{verb} unlabel #{issue.number} {BLOCKED_LABEL}" + (" and comment" if plan.comment else "")) if not dry_run: - gh("DELETE", f"repos/{repo}/issues/{issue.number}/labels/{quote(BLOCKED_LABEL)}", None) - if plan.comment: + # The notice goes first and is idempotent through its marker, and the label goes last: if anything + # fails in between, the label is still there and the next run finishes the job without a second + # notice. + if plan.comment and not _already_noticed(gh, repo, issue.number, plan.marker): gh("POST", f"repos/{repo}/issues/{issue.number}/comments", {"body": plan.comment}) - if issue.is_open: - unblocked.add(issue.number) + gh("DELETE", f"repos/{repo}/issues/{issue.number}/labels/{quote(BLOCKED_LABEL)}", None) + unblocked += issue.is_open for b in plan.not_completed: - marker = CANCELLED_MARKER.format(blocker=b.number) if dry_run: - print(f"would check #{issue.number}: blocker #{b.number} was not completed") + print(f"would check #{issue.number}: blocker {b.ref(repo)} was not completed") continue - if _already_noticed(gh, repo, issue.number, marker): + if _already_noticed(gh, repo, issue.number, NOT_COMPLETED_MARKER.format(blocker=b.key)): continue - print(f"did flag #{issue.number}: blocker #{b.number} was not completed") + print(f"did flag #{issue.number}: blocker {b.ref(repo)} was not completed") gh("POST", f"repos/{repo}/issues/{issue.number}/labels", {"labels": [TRIAGE_LABEL]}) - gh("POST", f"repos/{repo}/issues/{issue.number}/comments", {"body": not_completed_notice(b)}) - rerun = [] if dry_run or not unblocked else _rerun_pr_checks(gh, repo, unblocked) + gh("POST", f"repos/{repo}/issues/{issue.number}/comments", {"body": not_completed_notice(b, repo)}) + seen, rerun = reconcile_pr_checks(gh, repo, {i.number: i for i in open_issues}, dry_run) print( - f"open issues: {sum(1 for i in issues if i.is_open)}, blocked: {blocked}, unblocked now: {len(unblocked)}, " - f"pull-request checks re-run: {len(rerun)}" + f"open issues: {len(open_issues)}, blocked: {blocked}, unblocked now: {unblocked}, " + f"open pull requests: {seen}, dependency checks re-run: {rerun}" ) return 0 @@ -528,7 +691,8 @@ def run_check_pr(event: Mapping[str, Any], repo: str, gh: GhApi) -> int: except (AttributeError, KeyError, TypeError) as exc: raise GhError("the pull-request answer lacks body or labels") from exc closing, linked = linked_issues(body, repo) - verdict = check_pull_request(labels, closing, linked, lambda n: fetch_blockers_of(gh, repo, n), repo) + override = override_authorized(gh, repo, number) if OVERRIDE_LABEL in labels else None + verdict = check_pull_request(override, closing, linked, lambda n: fetch_blockers_of(gh, repo, n), repo) for line in verdict.lines: print(line if verdict.ok else f"::error::{line}") return 0 if verdict.ok else 3 From edcaab0cb6f4ec26865dd6c3f8f63269b54ef496 Mon Sep 17 00:00:00 2001 From: Cemil ILIK Date: Tue, 20 Oct 2026 15:06:10 +0300 Subject: [PATCH 3/6] fix(ci): strip multi-backtick code spans before reading closing links A code span delimited by two or more backticks (``Fixes #1``) was read as two empty single-backtick spans, so the reference inside stayed in the prose and counted as closing: a blocker GitHub would not close was exempted. Code spans are now paired as CommonMark pairs them, a run of N backticks up to the next run of exactly N, within one paragraph so that a stray backtick cannot hide the links in the paragraphs after it. Linear, like the rest of the reader; regression and linear-time cases added. Refs #1034 Co-Authored-By: Claude Opus 5.5 --- tests/test_issue_dependencies.py | 21 ++++++++++++-- tools/issue_dependencies.py | 49 ++++++++++++++++++++++++++++++-- 2 files changed, 65 insertions(+), 5 deletions(-) diff --git a/tests/test_issue_dependencies.py b/tests/test_issue_dependencies.py index 592d29c5..a56f4465 100644 --- a/tests/test_issue_dependencies.py +++ b/tests/test_issue_dependencies.py @@ -153,9 +153,14 @@ def test_the_pull_request_template_itself_links_nothing() -> None: (" ```\nFixes #2\nRefs #3", {2, 3}), # four spaces: not a fence opener ("Refs #1 Refs #3", {1, 3}), ("Refs #1 ```\n" * 5_000, ], ) @@ -203,7 +210,17 @@ def test_closing_the_blocker_in_the_same_pull_request_passes() -> None: assert verdict("Fixes #1\nFixes #5", {5: (blocker(1),), 1: ()}).ok -@pytest.mark.parametrize("body", ["Fixes #5, refs #1", "Fixes #5\nFixes#1", "Fixes #5\nFixes: #1", "Fixes #5, #1"]) +@pytest.mark.parametrize( + "body", + [ + "Fixes #5, refs #1", + "Fixes #5\nFixes#1", + "Fixes #5\nFixes: #1", + "Fixes #5, #1", + "Fixes #5\n``Fixes #1``", # inside a double-backtick code span GitHub closes nothing + "Fixes #5\n`` Fixes #1 ` still code ``", + ], +) def test_a_link_github_would_not_close_never_exempts_the_blocker(body: str) -> None: assert not verdict(body, {5: (blocker(1),), 1: ()}).ok diff --git a/tools/issue_dependencies.py b/tools/issue_dependencies.py index caa3f4ce..09d8a682 100644 --- a/tools/issue_dependencies.py +++ b/tools/issue_dependencies.py @@ -111,7 +111,7 @@ ) _CLOSING = frozenset({"close", "closes", "closed", "fix", "fixes", "fixed", "resolve", "resolves", "resolved"}) _FENCE_RE = re.compile(r" {0,3}(`{3,}|~{3,})") # matched at the start of one line by the walker below -_INLINE_CODE_RE = re.compile(r"`[^`\n]*`") # stops at the next backtick or newline: linear +_BACKTICK_RUN_RE = re.compile(r"`+") # one quantifier over one character: linear # Every GraphQL document this script may send. A query is chosen by name; only integer and cursor variables vary. _ISSUE_FIELDS = """ @@ -295,9 +295,52 @@ def _strip_html_comments(text: str) -> str: start = closing + 3 +def _strip_code_spans(paragraph: str) -> str: + """Drop CommonMark code spans from one paragraph: a run of N backticks up to the next run of exactly N. + + Linear: one pass collects the runs, a backward pass links each run to the next run of its length. An opener + without a matching run is literal text, as in CommonMark. + """ + runs = [(m.start(), m.end()) for m in _BACKTICK_RUN_RE.finditer(paragraph)] + next_same: list[Optional[int]] = [None] * len(runs) + last: dict[int, int] = {} + for idx in range(len(runs) - 1, -1, -1): + length = runs[idx][1] - runs[idx][0] + next_same[idx] = last.get(length) + last[length] = idx + out: list[str] = [] + pos = idx = 0 + while idx < len(runs): + close = next_same[idx] + if close is None: + idx += 1 + continue + out.append(paragraph[pos : runs[idx][0]] + " ") + pos, idx = runs[close][1], close + 1 + out.append(paragraph[pos:]) + return "".join(out) + + +def _strip_inline_code(text: str) -> str: + """Drop code spans paragraph by paragraph. A span never crosses a blank line, so a stray backtick cannot hide + the paragraphs after it, or the links in them.""" + kept: list[str] = [] + paragraph: list[str] = [] + for line in text.splitlines() + [""]: + if line.strip(): + paragraph.append(line) + continue + if paragraph: + kept.append(_strip_code_spans("\n".join(paragraph))) + paragraph = [] + kept.append("") + return "\n".join(kept) + + def _strip_non_prose(body: str) -> str: - """Drop fenced code, HTML comments and inline code: the template's own examples live there.""" - return _INLINE_CODE_RE.sub(" ", _strip_html_comments(_strip_fences(body))) + """Drop fenced code, HTML comments and inline code: the template's own examples live there, and GitHub neither + links nor closes an issue from inside them.""" + return _strip_inline_code(_strip_html_comments(_strip_fences(body))) def linked_issues(body: str, repo: str) -> tuple[frozenset[int], frozenset[int]]: From a156682576d6d6a2a9877ae72998e8359d92be71 Mon Sep 17 00:00:00 2001 From: Cemil ILIK Date: Tue, 20 Oct 2026 15:30:36 +0300 Subject: [PATCH 4/6] fix(ci): drop indented code blocks before reading closing links A reference inside an indented code block (`Fixes #5\n\n Fixes #1`) was counted as closing, so a blocker GitHub would not close was exempted. Closing references are now read without indented code blocks: a line indented four spaces or a tab opens one only at the start, after a blank line or after a heading, never as a paragraph continuation, and the block runs through indented and blank lines (linear line walker). Links are still read with those lines kept. Inside a list item, a line indented four spaces after a blank line is prose, so dropping it from the links could hide a linked blocked issue; dropping it from the closing references can only refuse an exemption. Refs #1034 Co-Authored-By: Claude Opus 5.5 --- tests/test_issue_dependencies.py | 18 +++++++++ tools/issue_dependencies.py | 63 ++++++++++++++++++++++++-------- 2 files changed, 65 insertions(+), 16 deletions(-) diff --git a/tests/test_issue_dependencies.py b/tests/test_issue_dependencies.py index a56f4465..4b97cb2e 100644 --- a/tests/test_issue_dependencies.py +++ b/tests/test_issue_dependencies.py @@ -177,6 +177,8 @@ def test_fences_comments_and_code_spans_follow_commonmark(body: str, every: set) "`" * 60_000, "`a" * 30_000, "``a`" * 15_000, + " Fixes #1\n\n" * 5_000, + "## h\n\tFixes #1\n" * 5_000, "```\n" * 5_000, ], ) @@ -219,12 +221,28 @@ def test_closing_the_blocker_in_the_same_pull_request_passes() -> None: "Fixes #5, #1", "Fixes #5\n``Fixes #1``", # inside a double-backtick code span GitHub closes nothing "Fixes #5\n`` Fixes #1 ` still code ``", + "Fixes #5\n\n Fixes #1", # nor inside an indented code block: after a blank line, + " Fixes #1\nFixes #5", # at the very start, + "## Notes\n Fixes #1\n\nFixes #5", # after a heading, + "Fixes #5\n\n\tFixes #1", # or indented with a tab ], ) def test_a_link_github_would_not_close_never_exempts_the_blocker(body: str) -> None: assert not verdict(body, {5: (blocker(1),), 1: ()}).ok +def test_a_four_space_line_right_after_paragraph_text_is_prose_and_closes() -> None: + # An indented code block cannot interrupt a paragraph, so GitHub closes #1 here. + assert tool.linked_issues("Fixes #5\n Fixes #1", REPO) == (frozenset({1, 5}), frozenset({1, 5})) + assert verdict("Fixes #5\n Fixes #1", {5: (blocker(1),), 1: ()}).ok + + +def test_indented_code_is_dropped_from_the_closing_references_only() -> None: + # Inside a list item an indented paragraph looks like code to a line walker: keeping it as a link errs strict. + body = "- item\n\n Refs #2\n\n Fixes #3\nFixes #4" + assert tool.linked_issues(body, REPO) == (frozenset({4}), frozenset({2, 3, 4})) + + def test_a_blocker_closed_without_being_completed_fails_even_if_named() -> None: v = verdict("Fixes #5\nFixes #1", {5: (blocker(1, "CLOSED", "NOT_PLANNED"),)}) assert not v.ok and "closed without being completed" in v.lines[0] diff --git a/tools/issue_dependencies.py b/tools/issue_dependencies.py index 09d8a682..642a8b8f 100644 --- a/tools/issue_dependencies.py +++ b/tools/issue_dependencies.py @@ -56,7 +56,7 @@ import subprocess import sys from dataclasses import dataclass -from typing import Any, Callable, Iterable, Mapping, Optional, Sequence +from typing import Any, Callable, Iterable, Iterator, Mapping, Optional, Sequence from urllib.parse import quote BLOCKED_LABEL = "blocked" @@ -112,6 +112,8 @@ _CLOSING = frozenset({"close", "closes", "closed", "fix", "fixes", "fixed", "resolve", "resolves", "resolved"}) _FENCE_RE = re.compile(r" {0,3}(`{3,}|~{3,})") # matched at the start of one line by the walker below _BACKTICK_RUN_RE = re.compile(r"`+") # one quantifier over one character: linear +_INDENTED_RE = re.compile(r"(?: {4}| {0,3}\t)") # matched at the start of one line: four spaces, or a tab +_ATX_HEADING_RE = re.compile(r" {0,3}#{1,6}(?:[ \t]|$)") # matched at the start of one line # Every GraphQL document this script may send. A query is chosen by name; only integer and cursor variables vary. _ISSUE_FIELDS = """ @@ -343,17 +345,35 @@ def _strip_non_prose(body: str) -> str: return _strip_inline_code(_strip_html_comments(_strip_fences(body))) -def linked_issues(body: str, repo: str) -> tuple[frozenset[int], frozenset[int]]: - """``(closing, all)``: issue numbers of this repository linked from a pull-request description. +def _strip_indented_code(text: str) -> str: + """Drop indented code blocks with a line walker (linear). - ``all`` holds every linked issue, read leniently; ``closing`` the ones GitHub will close on merge, read strictly - (see the module docstring). A reference to another repository is ignored: this repository's links cannot block on - it, and its workflow cannot close it. + A line indented four spaces (or a tab) opens a block only at the start, after a blank line or after a heading: + an indented code block cannot interrupt a paragraph, so a four-space line right after paragraph text is a + continuation and stays. Once open, the block runs through indented and blank lines. """ + kept: list[str] = [] + in_code = False + block_start = True + for line in text.splitlines(): + blank = not line.strip() + indented = bool(_INDENTED_RE.match(line)) + if in_code and (blank or indented): + kept.append("") + elif indented and block_start and not blank: + in_code = True + kept.append("") + else: + in_code = False + kept.append(line) + block_start = blank or bool(_ATX_HEADING_RE.match(line)) + return "\n".join(kept) + + +def _references(text: str, repo: str) -> Iterator[tuple[int, bool]]: + """Each reference to an issue of this repository in ``text``, with whether GitHub closes it on merge.""" owner, name = repo.lower().split("/") - closing: set[int] = set() - every: set[int] = set() - for match in _LINK_RE.finditer(_strip_non_prose(body or "")): + for match in _LINK_RE.finditer(text): keyword, sep, refs = match.group("kw").lower(), match.group("sep"), match.group("refs") if not sep and not refs.startswith("#"): continue # `refsfoo/bar#1` is a word, not a keyword followed by a reference @@ -363,13 +383,24 @@ def linked_issues(body: str, repo: str) -> tuple[frozenset[int], frozenset[int]] for position, ref in enumerate(_REF_RE.finditer(refs)): ref_owner = (ref.group("owner") or ref.group("uowner") or owner).lower() ref_repo = (ref.group("repo") or ref.group("urepo") or name).lower() - if (ref_owner, ref_repo) != (owner, name): - continue - number = int(ref.group("num") or ref.group("unum")) - every.add(number) - if strict and position == 0 and ref.group("num"): - closing.add(number) - return frozenset(closing), frozenset(every) + if (ref_owner, ref_repo) == (owner, name): + yield int(ref.group("num") or ref.group("unum")), strict and position == 0 and bool(ref.group("num")) + + +def linked_issues(body: str, repo: str) -> tuple[frozenset[int], frozenset[int]]: + """``(closing, all)``: issue numbers of this repository linked from a pull-request description. + + ``all`` holds every linked issue, read leniently; ``closing`` the ones GitHub will close on merge, read strictly + (see the module docstring). A reference to another repository is ignored: this repository's links cannot block on + it, and its workflow cannot close it. + """ + prose = _strip_non_prose(body or "") + every = frozenset(number for number, _ in _references(prose, repo)) + # Closing references are read without indented code blocks as well, and only they are: inside a list item, a + # line indented four spaces after a blank line is still prose, so dropping it from the links could hide a linked + # blocked issue, while dropping it from the closing references can only refuse an exemption. + closing = frozenset(number for number, closes in _references(_strip_indented_code(prose), repo) if closes) + return closing, every @dataclass(frozen=True) From 0c943e0da17b0c9aaf0c7d02ffc113d4dc1e27e5 Mon Sep 17 00:00:00 2001 From: Cemil ILIK Date: Tue, 20 Oct 2026 15:45:08 +0300 Subject: [PATCH 5/6] fix(ci): keep block boundaries when reading closing links An indented line right after a closed fence, a setext underline or a thematic break opens an indented code block in CommonMark, but the reader treated it as a paragraph continuation, so a reference in it counted as closing and exempted an open blocker. - Fenced-code lines are kept as blank placeholders instead of being dropped, so the boundary the fence made survives for the later readers. - _ends_block() adds exact setext-underline and thematic-break forms to the ATX heading check; a line such as `-_-` or `**` stays paragraph text. - Regression and linear-time cases added. Refs #1034 Co-Authored-By: Claude Opus 5.5 --- tests/test_issue_dependencies.py | 21 ++++++++++++++++----- tools/issue_dependencies.py | 31 +++++++++++++++++++++++++------ 2 files changed, 41 insertions(+), 11 deletions(-) diff --git a/tests/test_issue_dependencies.py b/tests/test_issue_dependencies.py index 4b97cb2e..ac722b43 100644 --- a/tests/test_issue_dependencies.py +++ b/tests/test_issue_dependencies.py @@ -179,6 +179,8 @@ def test_fences_comments_and_code_spans_follow_commonmark(body: str, every: set) "``a`" * 15_000, " Fixes #1\n\n" * 5_000, "## h\n\tFixes #1\n" * 5_000, + "- " * 30_000 + "x", # an almost-thematic break + "=" * 60_000 + "x", # an almost-setext underline "```\n" * 5_000, ], ) @@ -224,17 +226,26 @@ def test_closing_the_blocker_in_the_same_pull_request_passes() -> None: "Fixes #5\n\n Fixes #1", # nor inside an indented code block: after a blank line, " Fixes #1\nFixes #5", # at the very start, "## Notes\n Fixes #1\n\nFixes #5", # after a heading, - "Fixes #5\n\n\tFixes #1", # or indented with a tab + "Fixes #5\n\n\tFixes #1", # indented with a tab, + "Fixes #5\n```\nx\n```\n Fixes #1", # after a closed fence, + "Fixes #5\n\nTitle\n=====\n Fixes #1", # after a setext underline, + "Fixes #5\n\n***\n Fixes #1", # or after a thematic break + "Fixes #5\n\n* * *\n Fixes #1", + "Fixes #5\n\n___\n Fixes #1", ], ) def test_a_link_github_would_not_close_never_exempts_the_blocker(body: str) -> None: assert not verdict(body, {5: (blocker(1),), 1: ()}).ok -def test_a_four_space_line_right_after_paragraph_text_is_prose_and_closes() -> None: - # An indented code block cannot interrupt a paragraph, so GitHub closes #1 here. - assert tool.linked_issues("Fixes #5\n Fixes #1", REPO) == (frozenset({1, 5}), frozenset({1, 5})) - assert verdict("Fixes #5\n Fixes #1", {5: (blocker(1),), 1: ()}).ok +@pytest.mark.parametrize( + "body", ["Fixes #5\n Fixes #1", "Fixes #5\n-_-\n Fixes #1", "Fixes #5\n**\n Fixes #1"] +) +def test_a_four_space_line_right_after_paragraph_text_is_prose_and_closes(body: str) -> None: + # An indented code block cannot interrupt a paragraph, so GitHub closes #1 here; `-_-` and `**` are paragraph + # text, not a setext underline or a thematic break. + assert tool.linked_issues(body, REPO) == (frozenset({1, 5}), frozenset({1, 5})) + assert verdict(body, {5: (blocker(1),), 1: ()}).ok def test_indented_code_is_dropped_from_the_closing_references_only() -> None: diff --git a/tools/issue_dependencies.py b/tools/issue_dependencies.py index 642a8b8f..d1c120f5 100644 --- a/tools/issue_dependencies.py +++ b/tools/issue_dependencies.py @@ -114,6 +114,10 @@ _BACKTICK_RUN_RE = re.compile(r"`+") # one quantifier over one character: linear _INDENTED_RE = re.compile(r"(?: {4}| {0,3}\t)") # matched at the start of one line: four spaces, or a tab _ATX_HEADING_RE = re.compile(r" {0,3}#{1,6}(?:[ \t]|$)") # matched at the start of one line +# Whole lines that end a block: a setext underline and a thematic break, in their exact CommonMark forms, so a line +# such as `-_-` stays paragraph text. Every repetition starts with its own character, so nothing backtracks. +_SETEXT_UNDERLINE_RE = re.compile(r" {0,3}(?:=+|-+)[ \t]*") +_THEMATIC_BREAK_RE = re.compile(r" {0,3}(?:(?:\*[ \t]*){3,}|(?:-[ \t]*){3,}|(?:_[ \t]*){3,})") # Every GraphQL document this script may send. A query is chosen by name; only integer and cursor variables vary. _ISSUE_FIELDS = """ @@ -260,7 +264,8 @@ def _strip_fences(body: str) -> str: A fence opens with three or more backticks or tildes after at most three spaces (a backtick fence's info string may not contain a backtick) and closes on a line holding the same character at least as many times and nothing - else. A fence that never closes runs to the end, as in CommonMark. + else. A fence that never closes runs to the end, as in CommonMark. Each dropped line is kept as a blank line, so + the block boundary the fence made survives for the readers after this one. """ kept: list[str] = [] open_fence: Optional[tuple[str, int]] = None @@ -269,15 +274,18 @@ def _strip_fences(body: str) -> str: if open_fence is None: if match and not (match.group(1)[0] == "`" and "`" in line[match.end() :]): open_fence = (match.group(1)[0], len(match.group(1))) + kept.append("") continue kept.append(line) - elif ( + continue + if ( match and match.group(1)[0] == open_fence[0] and len(match.group(1)) >= open_fence[1] and not line[match.end() :].strip(" \t") ): open_fence = None + kept.append("") return "\n".join(kept) @@ -345,12 +353,23 @@ def _strip_non_prose(body: str) -> str: return _strip_inline_code(_strip_html_comments(_strip_fences(body))) +def _ends_block(line: str) -> bool: + """A line after which a new block starts: an ATX heading, a setext underline or a thematic break. + + An `===` or `---` line ends a block only under a paragraph line; read without that context it may end one too + early, which can only drop a closing reference, never add one. + """ + return bool( + _ATX_HEADING_RE.match(line) or _SETEXT_UNDERLINE_RE.fullmatch(line) or _THEMATIC_BREAK_RE.fullmatch(line) + ) + + def _strip_indented_code(text: str) -> str: """Drop indented code blocks with a line walker (linear). - A line indented four spaces (or a tab) opens a block only at the start, after a blank line or after a heading: - an indented code block cannot interrupt a paragraph, so a four-space line right after paragraph text is a - continuation and stays. Once open, the block runs through indented and blank lines. + A line indented four spaces (or a tab) opens a block only at the start, after a blank line or after a line that + ends a block (``_ends_block``): an indented code block cannot interrupt a paragraph, so a four-space line right + after paragraph text is a continuation and stays. Once open, the block runs through indented and blank lines. """ kept: list[str] = [] in_code = False @@ -366,7 +385,7 @@ def _strip_indented_code(text: str) -> str: else: in_code = False kept.append(line) - block_start = blank or bool(_ATX_HEADING_RE.match(line)) + block_start = blank or _ends_block(line) return "\n".join(kept) From f63da37727eb52db0a0238200ead24d58d868bd4 Mon Sep 17 00:00:00 2001 From: Cemil ILIK Date: Tue, 20 Oct 2026 17:00:50 +0300 Subject: [PATCH 6/6] fix(ci): let GitHub decide what a pull request closes; isolate sync failures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closing references: - A blocker is exempt only when GitHub's own closingIssuesReferences for the pull request lists it, and only for a pull request into the default branch. The strict closing-syntax reader (indented code, setext and thematic-break boundaries) is gone: an exemption can no longer be broader than what GitHub does, and `Fixes: #N` / `Fixes owner/repo#N` now count. - Links are read leniently from the title, the description and every commit message, plus GitHub's closing list; references are ASCII digits and whole tokens (`#5abc`, `#５` are not references); `http://` issue URLs count. - A number that names no issue (GitHub's NOT_FOUND for repository.issueOrPullRequest) is treated as no issue instead of a failure; any other GraphQL error still fails. - At most 50 linked issues, 100 commits and 50 closing references are read; more fails without a lookup. One check run per pull request at a time. Sync: - One issue that cannot be read or written no longer stops the others or the pull-request reconciliation; failures are reported and exit 1 at the end. An issue whose links could not be read is never treated as free. - `neutral` / `skipped` count as passing; a check older than 30 days is not re-run (a push re-runs it). Override events are sorted by time. An endless comment thread never triggers a repeated notice. - Manual runs only from the default branch. Messages and /assign: - An override lists what it accepts; a not-completed blocker asks for a re-plan, not a wait; an unblocked issue that still carries needs-triage says so; a context-only reference is told to drop the keyword. - /unassign never reads dependencies; a missing summary or a closed blocker makes /assign read the blocker list and count from it alone. Docs: CONTRIBUTING (rules, limits, maintainer notes, the Actions policy prerequisite), code-review.md merge conditions, PR template, CHANGELOG (advisory until required), good-first-issue links exclude `blocked`, test module count 132. Refs #1034 Co-Authored-By: Claude Opus 5.5 --- .github/pull_request_template.md | 7 +- .github/workflows/issue-dependencies.yml | 4 +- .github/workflows/pr-dependencies.yml | 21 +- AGENTS.md | 2 +- CHANGELOG.md | 17 +- CLAUDE.md | 2 +- CONTRIBUTING.md | 13 +- README.md | 4 +- docs/reference/architecture-tr.md | 2 +- docs/reference/architecture.md | 2 +- docs/standards/code-review.md | 3 + site/index.html | 2 +- tests/test_assign_on_comment.py | 29 +- tests/test_issue_dependencies.py | 572 ++++++++++++++-------- tools/assign_on_comment.py | 38 +- tools/issue_dependencies.py | 581 ++++++++++++++--------- 16 files changed, 831 insertions(+), 468 deletions(-) diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index b37d0d56..6717a337 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -7,8 +7,11 @@ + Put one keyword before each issue (`Fixes #12, fixes #13`): GitHub closes only the first of + `Fixes #12, #13`. The "Issue dependencies" check fails while an issue this PR links (title, + description, commits or the Development sidebar) is blocked, unless GitHub lists its blockers + as closed by this PR too, or a maintainer adds `dependency-override`. Mention an issue without + a keyword if this PR does not work on it. --> Fixes # ## Changes diff --git a/.github/workflows/issue-dependencies.yml b/.github/workflows/issue-dependencies.yml index 1d63615f..597b998e 100644 --- a/.github/workflows/issue-dependencies.yml +++ b/.github/workflows/issue-dependencies.yml @@ -35,7 +35,9 @@ concurrency: jobs: sync: - if: ${{ github.repository == 'HodeTech/ForgeLM' }} + # This repository, and only its default branch: a manual run started from another branch would check out that + # branch's copy of the script with this job's write token. + if: ${{ github.repository == 'HodeTech/ForgeLM' && github.ref == 'refs/heads/development' }} runs-on: ubuntu-latest timeout-minutes: 10 permissions: diff --git a/.github/workflows/pr-dependencies.yml b/.github/workflows/pr-dependencies.yml index fa417a2c..2a1ee613 100644 --- a/.github/workflows/pr-dependencies.yml +++ b/.github/workflows/pr-dependencies.yml @@ -1,9 +1,10 @@ name: PR dependencies -# The "Issue dependencies" check: fails while the pull request's description links (Fixes / Closes / Resolves / -# Refs #N) an issue that is still blocked, unless the same pull request closes every open blocker or a maintainer has -# labelled it `dependency-override`. Rules: tools/issue_dependencies.py and CONTRIBUTING.md, "Dependencies between -# issues". The "Issue dependencies" workflow re-runs this check whenever its result no longer matches the issues. +# The "Issue dependencies" check: fails while the pull request links an issue that is still blocked (Fixes / Closes / +# Resolves / Refs #N in its title, description or commits, or a Development-sidebar link), unless GitHub lists every +# open blocker as closed by the same pull request or a maintainer has labelled it `dependency-override`. Rules: +# tools/issue_dependencies.py and CONTRIBUTING.md, "Dependencies between issues". The "Issue dependencies" workflow +# re-runs this check whenever its result no longer matches the issues. # # Why `pull_request_target`. A `pull_request` run takes this file from the pull request itself, so a pull request # could rewrite the check that judges it. `pull_request_target` takes the workflow and the script from the default @@ -18,7 +19,13 @@ name: PR dependencies # - no `${{ }}` appears in a `run:` line; the script reads the event file itself. # What it cannot stop: a pull request may add a different workflow with a job of the same name, and branch # protection cannot tell two GitHub Actions checks of one name apart (true of every required Actions check), so a -# maintainer reads every change under `.github/` before merging it. +# maintainer reads every change under `.github/` and `tools/` before merging it. +# +# Prerequisite: from 2 November 2026 GitHub blocks `pull_request_target` in public repositories unless an Actions +# policy allows it. This repository's policy (Settings > Actions > Policies) allows it for this file's path only; +# renaming or moving this file needs the policy updated in the same change, or the check stops reporting. +# +# One run per pull request at a time: a newer event cancels the run still going, so a burst of edits costs one run. on: pull_request_target: @@ -26,6 +33,10 @@ on: permissions: {} +concurrency: + group: pr-dependencies-${{ github.event.pull_request.number }} + cancel-in-progress: true + jobs: issue-dependencies: name: Issue dependencies diff --git a/AGENTS.md b/AGENTS.md index 34a0f3f6..765791bf 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -85,7 +85,7 @@ ForgeLM/ │ # export, inference, results, utils, __main__, │ # _pypdf_normalise, _script_sanity, │ # _strip_pattern -├── tests/ # 131 test modules; count grows over time (run `ls tests/test_*.py | wc -l` for current) +├── tests/ # 132 test modules; count grows over time (run `ls tests/test_*.py | wc -l` for current) ├── tools/ # CI guards: check_anchor_resolution, │ # check_bilingual_parity, check_cli_help_consistency, │ # check_field_descriptions, check_no_analysis_refs, diff --git a/CHANGELOG.md b/CHANGELOG.md index b9d12a69..ebb450a9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,14 +27,15 @@ All notable changes to ForgeLM are documented here. when nobody holds the issue, refuses an issue that already has an assignee without changing it, allows at most 2 open assignments per contributor (maintainers are exempt) and leaves `security`, `needs-triage`, `deferred`, epic and Phase 16 issues to a maintainer. Reassigning stays a maintainer's decision. -- Dependencies between issues are enforced. A dependency is a native "Blocked by" link; the `Issue dependencies` - workflow (`tools/issue_dependencies.py`) labels every open issue with an unresolved blocker `blocked` and removes - the label, with one comment, once every blocker is closed as completed. A blocker closed as not planned or as a - duplicate keeps the issue blocked and flags it `needs-triage`. `/assign` refuses a blocked issue, and the new - `Issue dependencies` pull-request check fails a pull request that links one (`Fixes` / `Closes` / `Resolves` / - `Refs`) unless it closes the blockers too (in the form GitHub closes on merge) or a maintainer sets - `dependency-override`. The check runs from the default branch (`pull_request_target`, no pull-request code is - checked out), and the workflow re-runs it on every open pull request whose result no longer matches (#1034). +- Dependencies between issues are tracked and gated. A dependency is a native "Blocked by" link; the + `Issue dependencies` workflow (`tools/issue_dependencies.py`) labels every open issue with an unresolved blocker + `blocked` and removes the label, with one comment, once every blocker is closed as completed. A blocker closed as + not planned or as a duplicate keeps the issue blocked and flags it `needs-triage`. `/assign` refuses a blocked + issue, and the new `Issue dependencies` pull-request check fails a pull request that links one (`Fixes` / + `Closes` / `Resolves` / `Refs` in its title, description or commits, or a *Development* link), unless GitHub will + close the blockers with the same pull request or a maintainer sets `dependency-override`. The check runs from the + default branch (`pull_request_target`; no pull-request code is checked out), and the workflow re-runs it on every + open pull request whose result no longer matches. It is advisory until branch protection lists it (#1034). - `GOVERNANCE.md` and `MAINTAINERS.md` at the repository root, recording how decisions are made, how the maintainer roles work, and which areas the current maintainer owns (#964). diff --git a/CLAUDE.md b/CLAUDE.md index 8c28d87e..8e30b884 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -85,7 +85,7 @@ ForgeLM/ │ # export, inference, results, utils, __main__, │ # _pypdf_normalise, _script_sanity, │ # _strip_pattern -├── tests/ # 131 test modules; count grows over time (run `ls tests/test_*.py | wc -l` for current) +├── tests/ # 132 test modules; count grows over time (run `ls tests/test_*.py | wc -l` for current) ├── tools/ # CI guards: check_anchor_resolution, │ # check_bilingual_parity, check_cli_help_consistency, │ # check_field_descriptions, check_no_analysis_refs, diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 67fa4ad4..bff67b65 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -51,7 +51,7 @@ Thanks for your interest in contributing! ForgeLM is an open-source project and 6. A new contributor's first workflow run may wait for a maintainer to approve it; this is normal [GitHub workflow approval](https://docs.github.com/en/actions/how-tos/manage-workflow-runs/approve-runs-from-forks). -[good-first-issue]: https://github.com/HodeTech/ForgeLM/issues?q=is%3Aissue+is%3Aopen+no%3Aassignee+label%3A%22good+first+issue%22 +[good-first-issue]: https://github.com/HodeTech/ForgeLM/issues?q=is%3Aissue+is%3Aopen+no%3Aassignee+label%3A%22good+first+issue%22+-label%3Ablocked ## Ways to Contribute @@ -128,14 +128,17 @@ Useful starting points: Some issues can only be done after another one: each Phase 16 step waits for the step before it, a deferred item may wait for the step that changes the same file, and a review issue whose every finding is a unit of a Phase 16 step waits for that step, whose pull request fixes it. Such a dependency is recorded as a **Blocked by** link in the issue's sidebar (*Relationships*), never only in prose. -- **An issue with an open blocker is labelled `blocked`.** Do not start on it and do not open a pull request for it: `/assign` refuses it, and the **Issue dependencies** pull-request check fails a pull request whose description links it with `Fixes`, `Closes`, `Resolves` or `Refs #…`. +- **An issue with an open blocker is labelled `blocked`.** Do not start on it and do not open a pull request for it: `/assign` refuses it, and the **Issue dependencies** pull-request check fails a pull request that links it — with `Fixes`, `Closes`, `Resolves` or `Refs #…` in the title, the description or any commit message, or through GitHub's *Development* sidebar. If your pull request only mentions such an issue for context, drop the keyword. The check reads at most 50 linked issues, 100 commits and 50 closing references; a pull request over those limits fails until it is split or a maintainer accepts it. - **The label is automatic.** Nobody sets or removes it by hand. The *Issue dependencies* workflow adds it while a blocker is open and removes it once every blocker is closed as completed — as soon as that blocker is closed, or within an hour of a link being added or removed. When it removes the label it says so in a comment on the issue. Within the same hour it also re-runs the check of every open pull request whose result no longer matches, so a red check turns green once its blockers are completed, and a green one turns red if a linked issue becomes blocked. -- **A blocker closed as not planned or as a duplicate unblocks nothing**, because the work the issue waits for was not done. The issue stays `blocked`, gets `needs-triage`, and a maintainer re-plans it: removes the link, or links the issue that replaces the blocker. -- **A pull request may close the blocker as well.** When it closes every open blocker of the issues it links, the check passes. It counts a blocker as closed only in the form GitHub closes on merge — the keyword, a space and the issue, once per issue (`Fixes #12, fixes #13`) — so `Fixes#12`, `Fixes: #12` or the second issue of `Fixes #12, #13` leave the blocker open. A maintainer listed in [MAINTAINERS.md](MAINTAINERS.md) can also accept a dependency deliberately with the `dependency-override` label; the label set by anyone else is ignored. +- **A blocker closed as not planned or as a duplicate unblocks nothing**, because the work the issue waits for was not done. The issue stays `blocked`, gets `needs-triage`, and a maintainer re-plans it: removes the link, or links the issue that replaces the blocker. Once the dependency is re-planned the label goes, but the issue can be picked up only after a maintainer also removes `needs-triage`. +- **A pull request may close the blocker as well.** When it closes every open blocker of the issues it links, the check passes. What counts is GitHub's own list of issues the pull request will close — the *Development* sidebar — and only for a pull request into `development`, where GitHub closes on merge. Use one keyword per issue (`Fixes #12, fixes #13`): GitHub closes only the first issue of `Fixes #12, #13`. +- **A maintainer can accept a dependency deliberately** with the `dependency-override` label on the pull request. The check then passes and lists what it accepted; the label set by anyone not listed in [MAINTAINERS.md](MAINTAINERS.md) is ignored. The override covers the pull request as it stands, so read it again before merging if it changed after the label was set. - **Grouped issues are not blocked as a whole** when only some of their findings belong to a Phase 16 step. Those findings say so in the checklist (`KNOWN-OPEN … (S8 open)`): leave them to the step and take the others. The project's *Blocked* view lists every blocked issue; *Front door* and *Small tasks* leave them out. +**For maintainers.** Record a dependency with a *Blocked by* link and let the label follow; to apply a change at once instead of within the hour, run the *Issue dependencies* workflow by hand (Actions > *Issue dependencies* > *Run workflow*). To stop the automation, disable either workflow in the Actions tab; the links stay. The **Issue dependencies** check runs on `pull_request_target`, which GitHub blocks in public repositories from 2 November 2026 unless an Actions policy allows it: the repository's policy (Settings > Actions > Policies) allows it for `.github/workflows/pr-dependencies.yml` only, so moving or renaming that file needs the policy updated too, or the check stops reporting. + ## Reporting Security Vulnerabilities Please **do not open a public issue** for a security vulnerability, and do not post exploit details in comments. Report it privately through GitHub: on the repository's **Security** tab choose **Report a vulnerability**. [`SECURITY.md`](SECURITY.md) explains what to include, what counts as a vulnerability, and how disclosure works. Maintainers track confirmed vulnerabilities in private security advisories and publish them after a fixed release is available, so some planned security work is intentionally not visible in the public issue list. @@ -269,7 +272,7 @@ ForgeLM is a single-package layout: a mix of single-file modules and five focused sub-packages (`forgelm/cli/` post-Phase-15 split, `forgelm/data_audit/` post-Phase-14 split, `forgelm/wizard/` from Phase 22, `forgelm/safety/` from the post-v0.9.1 split and `forgelm/verify/` from -Phase 16 S1) under `forgelm/`, 131 test files +Phase 16 S1) under `forgelm/`, 132 test files under `tests/` (collected-test count grows over time — run `pytest --collect-only -q` for current), plus `configs/`, `docs/`, `tools/` (CI guards), and `notebooks/`. For the authoritative module-by-module map diff --git a/README.md b/README.md index e19d295a..aa96c28b 100644 --- a/README.md +++ b/README.md @@ -29,7 +29,7 @@ and an audit trail. Start with SFT, use preference or reasoning methods when you them, and opt into safety evaluation, human approval, and EU AI Act documentation. The same configuration works locally and in a CI/CD pipeline. -**Here to contribute?** Pick an [unassigned good first issue](https://github.com/HodeTech/ForgeLM/issues?q=is%3Aissue%20is%3Aopen%20label%3A%22good%20first%20issue%22%20no%3Aassignee) +**Here to contribute?** Pick an [unassigned good first issue](https://github.com/HodeTech/ForgeLM/issues?q=is%3Aissue%20is%3Aopen%20label%3A%22good%20first%20issue%22%20no%3Aassignee%20-label%3Ablocked) and follow [your first pull request](https://github.com/HodeTech/ForgeLM/blob/development/CONTRIBUTING.md#your-first-pull-request). Documentation, Turkish translations, examples, and tests are welcome alongside code. @@ -181,7 +181,7 @@ translate an example, reproduce a bug, add a regression test, or work on a plann feature. Start small; you do not need a GPU to run the unit tests or documentation checks. 1. Start at the [Front door](https://github.com/orgs/HodeTech/projects/5/views/16): the open, unassigned `good first issue`s - with their size and area. You can also [search by label](https://github.com/HodeTech/ForgeLM/issues?q=is%3Aissue%20is%3Aopen%20label%3A%22good%20first%20issue%22%20no%3Aassignee). + with their size and area. You can also [search by label](https://github.com/HodeTech/ForgeLM/issues?q=is%3Aissue%20is%3Aopen%20label%3A%22good%20first%20issue%22%20no%3Aassignee%20-label%3Ablocked). The rest of the planned work is in the [project plan](https://github.com/orgs/HodeTech/projects/5). 2. Follow [your first pull request](https://github.com/HodeTech/ForgeLM/blob/development/CONTRIBUTING.md#your-first-pull-request) to set up the environment and run the relevant checks. diff --git a/docs/reference/architecture-tr.md b/docs/reference/architecture-tr.md index 5da4fea3..344b9b89 100644 --- a/docs/reference/architecture-tr.md +++ b/docs/reference/architecture-tr.md @@ -96,7 +96,7 @@ ForgeLM/ │ ├── deepspeed/ # ZeRO-2, ZeRO-3, ZeRO-3+Offload ön ayarları │ └── safety_prompts/ # Yerleşik adversarial prompt kütüphanesi (140 prompt, 6 kategori) ├── notebooks/ # 10 Colab-uyumlu Jupyter notebook -├── tests/ # 131 test modülü +├── tests/ # 132 test modülü ├── tools/ # CI guard'ları: bilingual_parity, anchor_resolution, │ # cli_help_consistency, yaml_snippets, │ # audit_event_catalog, library_api_doc, diff --git a/docs/reference/architecture.md b/docs/reference/architecture.md index e3872c43..34fa443c 100644 --- a/docs/reference/architecture.md +++ b/docs/reference/architecture.md @@ -96,7 +96,7 @@ ForgeLM/ │ ├── deepspeed/ # ZeRO-2, ZeRO-3, ZeRO-3+Offload presets │ └── safety_prompts/ # Built-in adversarial prompt library (140 prompts, 6 categories) ├── notebooks/ # 10 Colab-ready Jupyter notebooks -├── tests/ # 131 test modules +├── tests/ # 132 test modules ├── tools/ # CI guards: bilingual_parity, anchor_resolution, │ # cli_help_consistency, yaml_snippets, │ # audit_event_catalog, library_api_doc, diff --git a/docs/standards/code-review.md b/docs/standards/code-review.md index e5aef03b..a02f321e 100644 --- a/docs/standards/code-review.md +++ b/docs/standards/code-review.md @@ -13,6 +13,9 @@ A PR merges only after: 3. **Self-checklist complete** — every box in the PR template ticked honestly. 4. **Every review conversation resolved** — branch protection enforces this on `development` and `main`, including the comments of the automated reviewers. +5. **No linked issue is blocked** — the **Issue dependencies** check passes, or a maintainer accepted the dependency + with `dependency-override` ([CONTRIBUTING.md, "Dependencies between issues"](../../CONTRIBUTING.md#dependencies-between-issues)). + Until branch protection lists that check, a red result does not disable the merge button, so read it. No exceptions for "trivial" changes. Even typo fixes go through CI. diff --git a/site/index.html b/site/index.html index af90addc..dce4c389 100644 --- a/site/index.html +++ b/site/index.html @@ -686,7 +686,7 @@

Contribute to ForgeLM

Start with the first pull request guide and an unassigned good first issue. Documentation, translations, tests, and small fixes all help.

Your first pull request - Browse good first issues + Browse good first issues
diff --git a/tests/test_assign_on_comment.py b/tests/test_assign_on_comment.py index 93b1b629..12fc02ea 100644 --- a/tests/test_assign_on_comment.py +++ b/tests/test_assign_on_comment.py @@ -55,6 +55,7 @@ def __init__( self.refuse_delete = False # accept the DELETE but keep the assignee self.open_blockers: Optional[int] = 0 # GitHub's count of open "Blocked by" issues; None drops the summary self.closed_blockers: List[Optional[str]] = [] # the state_reason of each closed "Blocked by" issue + self.listed_open: Optional[int] = None # open blockers in the list, when it disagrees with the summary self._reads = 0 def _issue(self) -> Dict[str, Any]: @@ -69,7 +70,8 @@ def _issue(self) -> Dict[str, Any]: return issue def _get_blockers(self, endpoint: str, payload: Any) -> List[Dict[str, Any]]: - return [{"state": "open", "state_reason": None}] * (self.open_blockers or 0) + [ + listed = self.open_blockers if self.listed_open is None else self.listed_open + return [{"state": "open", "state_reason": None}] * (listed or 0) + [ {"state": "closed", "state_reason": reason} for reason in self.closed_blockers ] @@ -394,11 +396,20 @@ def test_the_blocker_list_is_read_only_when_the_summary_cannot_decide() -> None: assert not [c for c in gh.calls if "/dependencies/" in c[1]] -def test_without_a_dependency_summary_the_label_alone_decides() -> None: - free, blocked = FakeGitHub(labels=["good first issue"]), FakeGitHub(labels=["good first issue", "blocked"]) - free.open_blockers = blocked.open_blockers = None +def test_without_a_dependency_summary_the_blocker_list_decides() -> None: + free, cancelled = FakeGitHub(labels=["good first issue"]), FakeGitHub(labels=["good first issue"]) + free.open_blockers = cancelled.open_blockers = None + cancelled.closed_blockers = ["not_planned"] assert run(free) == 0 and free.assignees == ["newcomer"] - assert run(blocked) == 0 and blocked.assignees == [] + assert run(cancelled) == 0 and cancelled.assignees == [] + assert [c for c in free.calls if "/dependencies/" in c[1]] # read, not assumed free + + +def test_the_list_is_counted_on_its_own_when_it_disagrees_with_the_summary() -> None: + gh = FakeGitHub(labels=["good first issue"]) + gh.closed_blockers = ["completed"] # the summary: no open blocker, one closed + gh.listed_open = 1 # the list, read a moment later: a blocker was reopened + assert run(gh) == 0 and gh.assignees == [] def test_a_blocked_issue_can_still_be_released() -> None: @@ -407,6 +418,14 @@ def test_a_blocked_issue_can_still_be_released() -> None: assert gh.assignees == [] +def test_unassign_never_reads_the_dependencies_so_a_failing_read_cannot_stop_it() -> None: + gh = FakeGitHub(labels=["good first issue"], assignees=["first"]) + gh.closed_blockers = ["not_planned"] + gh.fail[("GET", f"repos/{REPO}/issues/7/dependencies")] = "boom" + assert run(gh, "/unassign", login="first") == 0 + assert gh.assignees == [] and not [c for c in gh.calls if "/dependencies/" in c[1]] + + # --- the per-person limit ------------------------------------------------------------------------------------- diff --git a/tests/test_issue_dependencies.py b/tests/test_issue_dependencies.py index ac722b43..efbad1bb 100644 --- a/tests/test_issue_dependencies.py +++ b/tests/test_issue_dependencies.py @@ -1,20 +1,22 @@ """Tests for tools/issue_dependencies.py and its two workflows. The tool derives the ``blocked`` label from GitHub's native "Blocked by" links and fails a pull request that works -on a blocked issue (CONTRIBUTING.md, "Dependencies between issues"). These tests pin the rules, the parsing of -pull-request descriptions written by anyone (including its linear running time), the write paths of ``sync`` and the -pull-request check reconciliation against a fake API, the fixed-endpoint discipline of the GitHub calls, and the -security posture of both workflow files, since no other guard reads them. +on a blocked issue (CONTRIBUTING.md, "Dependencies between issues"). These tests pin the rules, the reading of +pull-request texts written by anyone (including its linear running time), the use of GitHub's own closing list for +exemptions, the write paths of ``sync`` and the pull-request check reconciliation against a fake API (with GitHub's +real error shapes), the fixed-endpoint discipline of the GitHub calls, and the security posture of both workflow +files, since no other guard reads them. """ from __future__ import annotations +import datetime import json import re import sys import time from pathlib import Path -from typing import Any, Dict, List, Optional, Tuple +from typing import Any, Dict, List, Optional, Sequence, Tuple import pytest import yaml @@ -26,7 +28,13 @@ import issue_dependencies as tool # noqa: E402 REPO = "HodeTech/ForgeLM" +OTHER = "HodeTech/Other" SHA = "a" * 40 +NOT_FOUND = { + "type": "NOT_FOUND", + "path": ["repository", "issueOrPullRequest"], + "message": "Could not resolve to an issue or pull request with the number of 404.", +} def blocker(number: int, state: str = "OPEN", reason: Optional[str] = None, repo: str = REPO) -> tool.Blocker: @@ -37,6 +45,48 @@ def issue(number: int, *blockers: tool.Blocker, labels: Tuple[str, ...] = (), is return tool.IssueState(number, is_open, frozenset(labels), tuple(blockers)) +def pull_node( + number: int = 30, + body: str = "", + title: str = "", + labels: Sequence[str] = (), + sha: str = SHA, + base: str = "development", + closing: Sequence[Any] = (), + commits: Sequence[str] = (), + closing_total: Optional[int] = None, + commit_total: Optional[int] = None, +) -> Dict[str, Any]: + """A pull request as the GraphQL queries return it; a closing entry is `n` or `(n, "owner/repo")`.""" + refs = [c if isinstance(c, tuple) else (c, REPO) for c in closing] + return { + "number": number, + "title": title, + "body": body, + "headRefOid": sha, + "baseRefName": base, + "labels": {"totalCount": len(labels), "nodes": [{"name": n} for n in labels]}, + "closingIssuesReferences": { + "totalCount": len(refs) if closing_total is None else closing_total, + "nodes": [{"number": n, "repository": {"nameWithOwner": r}} for n, r in refs], + }, + "commits": { + "totalCount": len(commits) if commit_total is None else commit_total, + "nodes": [{"commit": {"message": m}} for m in commits], + }, + } + + +def facts(**kwargs: Any) -> tool.PullFacts: + return tool._pull_facts(pull_node(**kwargs), "development") + + +def verdict( + links: Dict[int, Optional[Tuple[tool.Blocker, ...]]], override: Optional[bool] = None, **kwargs: Any +) -> tool.Verdict: + return tool.check_pull_request(override, facts(**kwargs), lambda n: links.get(n, ()), REPO) + + # --- the rules ------------------------------------------------------------------------------------------------ @@ -69,14 +119,21 @@ def test_one_open_blocker_among_closed_ones_keeps_the_issue_blocked() -> None: def test_when_every_blocker_is_completed_the_label_goes_and_one_comment_names_them() -> None: - done = (blocker(1, "CLOSED", "COMPLETED"), blocker(9, "CLOSED", "COMPLETED", repo="HodeTech/Other")) + done = (blocker(1, "CLOSED", "COMPLETED"), blocker(9, "CLOSED", "COMPLETED", repo=OTHER)) plan = tool.plan_issue(issue(5, *done, labels=("blocked", "wave: 3")), REPO) assert plan.remove_label and not plan.add_label assert "(#1, HodeTech/Other#9)" in plan.comment and plan.comment.startswith("This issue is no longer blocked") + assert "can be picked up now" in plan.comment assert plan.marker == "" assert plan.comment.endswith(plan.marker) +def test_an_unblocked_issue_that_still_needs_triage_does_not_say_it_can_be_picked_up() -> None: + plan = tool.plan_issue(issue(5, blocker(1, "CLOSED", "COMPLETED"), labels=("blocked", "needs-triage")), REPO) + assert plan.remove_label and "still carries `needs-triage`" in plan.comment + assert "can be picked up now" not in plan.comment + + def test_a_hand_set_label_without_links_is_removed_with_the_explanation() -> None: plan = tool.plan_issue(issue(5, labels=("blocked",)), REPO) assert plan.remove_label and "set and removed automatically" in plan.comment and plan.marker.endswith(":none -->") @@ -104,47 +161,47 @@ def test_a_blocker_closed_without_being_completed_keeps_the_issue_blocked_and_as def test_blockers_in_two_repositories_with_one_number_never_share_a_notice() -> None: - here, there = blocker(7, "CLOSED", "NOT_PLANNED"), blocker(7, "CLOSED", "NOT_PLANNED", repo="HodeTech/Other") + here, there = blocker(7, "CLOSED", "NOT_PLANNED"), blocker(7, "CLOSED", "NOT_PLANNED", repo=OTHER) a, b = tool.not_completed_notice(here, REPO), tool.not_completed_notice(there, REPO) assert a.startswith("#7, which") and b.startswith("HodeTech/Other#7, which") assert a.splitlines()[-1] != b.splitlines()[-1] -# --- reading pull-request descriptions ------------------------------------------------------------------------- +# --- reading the texts a pull request is made of --------------------------------------------------------------- @pytest.mark.parametrize( - "body, closing, every", + "text, linked", [ - ("Fixes #12", {12}, {12}), - ("fixed #12, closes #13 and resolves #14", {12, 13, 14}, {12, 13, 14}), - ("FIXES #12", {12}, {12}), - ("Fixes HodeTech/ForgeLM#12", {12}, {12}), - # GitHub closes one issue per keyword: in a list only the first is closed, the rest are linked - ("Fixes #12, #13 and #14", {12}, {12, 13, 14}), - # forms GitHub does not close: linked (the check may fail on them) but never an exemption - ("Fixes#12", set(), {12}), - ("Resolves: #12", set(), {12}), - ("Fixes https://github.com/HodeTech/ForgeLM/issues/12", set(), {12}), - ("Refs #12", set(), {12}), - ("references #12 & #13", set(), {12, 13}), - ("Fixes other/repo#12 and https://github.com/other/repo/issues/13", set(), set()), - ("See #12 for context; prefixes #13 are not keywords; refsfoo/bar#14", set(), set()), - ("Fixes\n#12", set(), set()), # a keyword never reaches across a line - ("\n`Refs #13`\n```\nFixes #14\n```\nRefs #15", set(), {15}), + ("Fixes #12", {12}), + ("fixed #12, closes #13 and resolves #14", {12, 13, 14}), + ("FIXES #12", {12}), + ("Fixes HodeTech/ForgeLM#12", {12}), + ("Fixes #12, #13 and #14", {12, 13, 14}), # every issue of a list counts as linked + ("Fixes#12", {12}), # read leniently: links only make the check stricter + ("Resolves: #12", {12}), + ("Fixes https://github.com/HodeTech/ForgeLM/issues/12", {12}), + ("Refs http://github.com/HodeTech/ForgeLM/issues/12", {12}), + ("Refs #12", {12}), + ("references #12 & #13", {12, 13}), + ("Fixes other/repo#12 and https://github.com/other/repo/issues/13", set()), + ("See #12 for context; prefixes #13 are not keywords; refsfoo/bar#14", set()), + ("Fixes\n#12", set()), # a keyword never reaches across a line + ("Fixes #5abc and refs #７", set()), # a reference is ASCII digits and a whole token + ("\n`Refs #13`\n```\nFixes #14\n```\nRefs #15", {15}), ], ) -def test_linked_issues(body: str, closing: set, every: set) -> None: - assert tool.linked_issues(body, REPO) == (frozenset(closing), frozenset(every)) +def test_linked_issues(text: str, linked: set) -> None: + assert tool.linked_issues(text, REPO) == frozenset(linked) def test_the_pull_request_template_itself_links_nothing() -> None: template = (ROOT / ".github/pull_request_template.md").read_text(encoding="utf-8") - assert tool.linked_issues(template, REPO) == (frozenset(), frozenset()) + assert tool.linked_issues(template, REPO) == frozenset() @pytest.mark.parametrize( - "body, every", + "text, linked", [ ("Refs #1\n```\nFixes #2\nRefs #3", {1}), # an unclosed fence runs to the end ("~~~~\nFixes #2\n~~~\nstill code\n~~~~\nRefs #3", {3}), # a closing fence is at least as long @@ -158,124 +215,121 @@ def test_the_pull_request_template_itself_links_nothing() -> None: ("Refs #1 ``Refs #2` Refs #3", {1, 2, 3}), # unmatched runs are literal text ("A stray ` here\n\nRefs #3\n\nand ` there", {3}), # a span never crosses a blank line ("A span `across\nRefs #2` lines\nRefs #3", {3}), # but it does cross a line break in a paragraph + ("A `span\n```\nx\n```\nRefs #3 ` and", {3}), # nor a fence + ("```\n` took about 2 s here - "```x\n" * 15_000, # openers that never close: the earlier fence regex took about 5 s here - "```\n" * 15_000, - "~~~~\n```\n" * 7_500, - "Fixes " * 10_000 + "#1", - "Fixes" + " " * 60_000 + "x", - "Fixes #1, " * 6_000, - "`" * 60_000, - "`a" * 30_000, - "``a`" * 15_000, - " Fixes #1\n\n" * 5_000, - "## h\n\tFixes #1\n" * 5_000, - "- " * 30_000 + "x", # an almost-thematic break - "=" * 60_000 + "x", # an almost-setext underline - "```\n" * 5_000, - ], -) -def test_a_hostile_description_is_read_in_linear_time(body: str) -> None: +def test_fences_comments_and_code_spans_follow_commonmark(text: str, linked: set) -> None: + assert tool.linked_issues(text, REPO) == frozenset(linked) + + +HOSTILE = { + "unclosed comments": "` took about 2 s here + "unclosed fence openers": "```x\n" * 15_000, # the earlier fence regex took about 5 s here + "alternating fences": "```\n" * 15_000, + "mixed fences": "~~~~\n```\n" * 7_500, + "keywords without references": "Fixes " * 10_000 + "#1", + "a keyword and a long space run": "Fixes" + " " * 60_000 + "x", + "a long reference list": "Fixes #1, " * 6_000, + "one backtick run": "`" * 60_000, + "many single backticks": "`a" * 30_000, + "unequal backtick runs": "``a`" * 15_000, + "digit runs that are not tokens": "#" + "1" * 60_000 + "a", + "comments then fences": "```\n" * 5_000, +} + + +@pytest.mark.parametrize("text", HOSTILE.values(), ids=HOSTILE.keys()) +def test_a_hostile_text_is_read_in_linear_time(text: str) -> None: # docs/standards/regex.md: a generous absolute floor; a quadratic reader blows far past it at this size. start = time.perf_counter() - tool.linked_issues(body, REPO) + tool.linked_issues(text, REPO) assert time.perf_counter() - start < 1.0 # --- the pull-request rule ------------------------------------------------------------------------------------- -def verdict( - body: str, links: Dict[int, Optional[Tuple[tool.Blocker, ...]]], override: Optional[bool] = None -) -> tool.Verdict: - closing, linked = tool.linked_issues(body, REPO) - return tool.check_pull_request(override, closing, linked, lambda n: links.get(n, ()), REPO) - - def test_a_pull_request_on_a_blocked_issue_fails_and_names_the_blocker() -> None: - v = verdict("Fixes #5", {5: (blocker(1),)}) + v = verdict({5: (blocker(1),)}, body="Fixes #5", closing=[5]) assert not v.ok and v.lines[0] == "#5 is blocked by #1, which is still open." + assert any(line.startswith("Wait until the blocker is closed as completed") for line in v.lines) -def test_refs_counts_as_working_on_the_issue() -> None: - assert not verdict("Refs #5", {5: (blocker(1),)}).ok - - -def test_closing_the_blocker_in_the_same_pull_request_passes() -> None: - assert verdict("Fixes #1\nFixes #5", {5: (blocker(1),), 1: ()}).ok +def test_refs_counts_as_working_on_the_issue_and_the_message_says_how_to_drop_it() -> None: + v = verdict({5: (blocker(1),)}, body="Refs #5") + assert not v.ok and any("remove the reference" in line for line in v.lines) @pytest.mark.parametrize( - "body", - [ - "Fixes #5, refs #1", - "Fixes #5\nFixes#1", - "Fixes #5\nFixes: #1", - "Fixes #5, #1", - "Fixes #5\n``Fixes #1``", # inside a double-backtick code span GitHub closes nothing - "Fixes #5\n`` Fixes #1 ` still code ``", - "Fixes #5\n\n Fixes #1", # nor inside an indented code block: after a blank line, - " Fixes #1\nFixes #5", # at the very start, - "## Notes\n Fixes #1\n\nFixes #5", # after a heading, - "Fixes #5\n\n\tFixes #1", # indented with a tab, - "Fixes #5\n```\nx\n```\n Fixes #1", # after a closed fence, - "Fixes #5\n\nTitle\n=====\n Fixes #1", # after a setext underline, - "Fixes #5\n\n***\n Fixes #1", # or after a thematic break - "Fixes #5\n\n* * *\n Fixes #1", - "Fixes #5\n\n___\n Fixes #1", - ], + "where", + [{"title": "Refs #5"}, {"commits": ["feat: x\n\nFixes #5"]}, {"closing": [5]}], # a manual Development link + ids=["title", "commit message", "GitHub's closing list"], ) -def test_a_link_github_would_not_close_never_exempts_the_blocker(body: str) -> None: - assert not verdict(body, {5: (blocker(1),), 1: ()}).ok +def test_every_place_a_pull_request_can_link_an_issue_counts(where: Dict[str, Any]) -> None: + assert not verdict({5: (blocker(1),)}, body="Docs only", **where).ok + + +def test_a_blocker_github_will_close_in_the_same_pull_request_is_exempt() -> None: + assert verdict({5: (blocker(1),), 1: ()}, body="Fixes #1\nFixes #5", closing=[1, 5]).ok + + +def test_a_blocker_in_another_repository_is_exempt_when_github_will_close_it() -> None: + there = blocker(7, repo=OTHER) + assert verdict({5: (there,)}, body="Fixes #5", closing=[5, (7, OTHER)]).ok + assert not verdict({5: (there,)}, body="Fixes #5", closing=[5, 7]).ok # #7 here is another issue @pytest.mark.parametrize( - "body", ["Fixes #5\n Fixes #1", "Fixes #5\n-_-\n Fixes #1", "Fixes #5\n**\n Fixes #1"] + "body", ["Fixes #5\nFixes #1", "Fixes #5\nFixes#1", "Fixes #5, #1", "Fixes #5\n> ~~~\n> Fixes #1\n> ~~~"] ) -def test_a_four_space_line_right_after_paragraph_text_is_prose_and_closes(body: str) -> None: - # An indented code block cannot interrupt a paragraph, so GitHub closes #1 here; `-_-` and `**` are paragraph - # text, not a setext underline or a thematic break. - assert tool.linked_issues(body, REPO) == (frozenset({1, 5}), frozenset({1, 5})) - assert verdict(body, {5: (blocker(1),), 1: ()}).ok +def test_only_githubs_closing_list_exempts_a_blocker_never_the_texts(body: str) -> None: + # GitHub lists only #5 as closed by this pull request, whatever the description seems to say about #1. + assert not verdict({5: (blocker(1),), 1: ()}, body=body, closing=[5]).ok -def test_indented_code_is_dropped_from_the_closing_references_only() -> None: - # Inside a list item an indented paragraph looks like code to a line walker: keeping it as a link errs strict. - body = "- item\n\n Refs #2\n\n Fixes #3\nFixes #4" - assert tool.linked_issues(body, REPO) == (frozenset({4}), frozenset({2, 3, 4})) +def test_off_the_default_branch_nothing_is_exempt_because_github_closes_nothing_on_merge() -> None: + assert not verdict({5: (blocker(1),), 1: ()}, body="Fixes #1\nFixes #5", closing=[1, 5], base="main").ok -def test_a_blocker_closed_without_being_completed_fails_even_if_named() -> None: - v = verdict("Fixes #5\nFixes #1", {5: (blocker(1, "CLOSED", "NOT_PLANNED"),)}) - assert not v.ok and "closed without being completed" in v.lines[0] +def test_a_blocker_closed_without_being_completed_fails_and_asks_for_a_re_plan_not_a_wait() -> None: + v = verdict({5: (blocker(1, "CLOSED", "NOT_PLANNED"),)}, body="Fixes #1\nFixes #5", closing=[1, 5]) + assert not v.ok and "re-plan that dependency" in v.lines[0] + assert not any(line.startswith("Wait until") for line in v.lines) -def test_completed_blockers_pull_requests_and_closed_issues_pass() -> None: - v = verdict("Fixes #5, #6", {5: (blocker(1, "CLOSED", "COMPLETED"),), 6: None}) +def test_completed_blockers_pull_requests_closed_and_missing_issues_pass() -> None: + v = verdict({5: (blocker(1, "CLOSED", "COMPLETED"),), 6: None}, body="Fixes #5, #6") assert v.ok and v.lines == ("No linked issue is blocked (1 issue(s) checked).",) -def test_a_maintainers_override_passes_and_says_so() -> None: - v = verdict("Fixes #5", {5: (blocker(1),)}, override=True) - assert v.ok and "set by a maintainer" in v.lines[0] +def test_more_links_than_the_bound_fail_without_a_single_lookup() -> None: + asked: List[int] = [] + body = " ".join(f"Refs #{n}" for n in range(1, tool._MAX_LINKED + 2)) + v = tool.check_pull_request(None, facts(body=body), lambda n: asked.append(n) or (), REPO) + assert not v.ok and "at most 50" in v.lines[0] and asked == [] + accepted = tool.check_pull_request(True, facts(body=body), lambda n: asked.append(n) or (), REPO) + assert accepted.ok and asked == [] + + +@pytest.mark.parametrize("kwargs", [{"commit_total": 101}, {"closing": [5], "closing_total": 51}]) +def test_lists_longer_than_one_page_fail_instead_of_being_read_in_part(kwargs: Dict[str, Any]) -> None: + assert not verdict({}, body="Docs only", **kwargs).ok + assert verdict({}, override=True, body="Docs only", **kwargs).ok + + +def test_a_maintainers_override_passes_and_lists_what_it_accepts() -> None: + v = verdict({5: (blocker(1),)}, override=True, body="Fixes #5") + assert v.ok and "set by a maintainer" in v.lines[0] and v.lines[1] == "#5 is blocked by #1, which is still open." def test_an_override_set_by_anyone_else_is_ignored_and_says_so() -> None: - v = verdict("Fixes #5", {5: (blocker(1),)}, override=False) + v = verdict({5: (blocker(1),)}, override=False, body="Fixes #5") assert not v.ok and any("is ignored" in line for line in v.lines) -def label_event(kind: str, login: str, label: str = "dependency-override") -> Dict[str, Any]: - return {"event": kind, "actor": {"login": login}, "label": {"name": label}} +def label_event(kind: str, login: str, at: str = "2026-10-20T10:00:00Z", label: str = "dependency-override") -> dict: + return {"event": kind, "actor": {"login": login}, "label": {"name": label}, "created_at": at, "id": 1} @pytest.mark.parametrize( @@ -284,9 +338,18 @@ def label_event(kind: str, login: str, label: str = "dependency-override") -> Di ([label_event("labeled", "cemililik")], True), ([label_event("labeled", "CemilIlik")], True), ([label_event("labeled", "contributor")], False), - ([label_event("labeled", "cemililik"), label_event("unlabeled", "x"), label_event("labeled", "x")], False), - ([label_event("labeled", "cemililik"), label_event("unlabeled", "cemililik")], False), + ( + [ + label_event("labeled", "cemililik", "2026-10-20T10:00:00Z"), + label_event("unlabeled", "x", "2026-10-20T11:00:00Z"), + label_event("labeled", "x", "2026-10-20T12:00:00Z"), + ], + False, + ), + ([label_event("labeled", "cemililik"), label_event("unlabeled", "cemililik", "2026-10-20T11:00:00Z")], False), ([label_event("labeled", "contributor", label="bug"), label_event("labeled", "cemililik")], True), + # out of order: the latest by time decides, not the position in the answer + ([label_event("labeled", "contributor", "2026-10-21T09:00:00Z"), label_event("labeled", "cemililik")], False), ([], False), ], ) @@ -300,7 +363,7 @@ def test_the_override_counts_only_when_a_maintainer_set_it_last(events: List[Dic class FakeGitHub: - """Plays the endpoints the tool calls: GraphQL for issues and their links, REST for the rest.""" + """Plays the endpoints the tool calls: GraphQL for issues and pull requests, REST for the rest.""" def __init__(self, issues: Dict[int, Dict[str, Any]], closed_labelled: Tuple[int, ...] = ()) -> None: # number -> {"labels": [...], "blockers": [(n, state, reason[, repo]), ...], "state": "OPEN", "type": "Issue"} @@ -309,10 +372,11 @@ def __init__(self, issues: Dict[int, Dict[str, Any]], closed_labelled: Tuple[int self.closed_labelled = list(closed_labelled) self.calls: List[Tuple[str, str, Any]] = [] self.comments: Dict[int, List[Dict[str, Any]]] = {} - self.pulls: List[Dict[str, Any]] = [] + self.pulls: Dict[int, Dict[str, Any]] = {} self.check_runs: Dict[str, List[Dict[str, Any]]] = {} self.events: Dict[int, List[Dict[str, Any]]] = {} - self.fail: List[Tuple[str, str]] = [] # (method, endpoint prefix) + self.fail: List[Tuple[str, str]] = [] # (method, endpoint prefix); `query:` for a GraphQL query + self.issue_page = 100 @staticmethod def _node(number: int, data: Dict[str, Any]) -> Dict[str, Any]: @@ -324,36 +388,67 @@ def _node(number: int, data: Dict[str, Any]) -> Dict[str, Any]: "__typename": data.get("type", "Issue"), "number": number, "state": data.get("state", "OPEN"), - "labels": {"totalCount": len(data["labels"]), "nodes": [{"name": n} for n in data["labels"]]}, + "labels": { + "totalCount": data.get("label_total", len(data["labels"])), + "nodes": [{"name": n} for n in data["labels"]], + }, "blockedBy": {"totalCount": len(nodes), "nodes": nodes}, } - def pull(self, number: int, body: str, labels: Tuple[str, ...] = (), sha: str = SHA) -> None: - self.pulls.append( - {"number": number, "body": body, "labels": [{"name": n} for n in labels], "head": {"sha": sha}} - ) + def pull(self, number: int, body: str = "", **kwargs: Any) -> None: + self.pulls[number] = pull_node(number, body, **kwargs) + + def _graphql(self, payload: Dict[str, Any]) -> Any: + query = next(name for name, text in tool.QUERIES.items() if text == payload["query"]) + if ("POST", f"query:{query}") in self.fail: + raise tool.GhError(f"GraphQL {query} failed") + v = payload["variables"] + if query == "issue": + data = self.fresh.get(v["number"], self.issues.get(v["number"])) + if data is None: + return {"data": {"repository": {"issueOrPullRequest": None}}, "errors": [NOT_FOUND]} + return {"data": {"repository": {"issueOrPullRequest": self._node(v["number"], data)}}} + if query == "open_issues": + numbers = [ + n + for n, i in sorted(self.issues.items()) + if i.get("state", "OPEN") == "OPEN" and i.get("type", "Issue") == "Issue" + ] + start = int(v["after"] or 0) + page = numbers[start : start + self.issue_page] + more = start + self.issue_page < len(numbers) + info = {"hasNextPage": more, "endCursor": str(start + self.issue_page)} + return { + "data": { + "repository": {"issues": {"nodes": [self._node(n, self.issues[n]) for n in page], "pageInfo": info}} + } + } + branch = {"defaultBranchRef": {"name": "development"}} + if query == "pull": + return {"data": {"repository": {**branch, "pullRequest": self.pulls.get(v["number"])}}} + if query == "open_pulls": + nodes = [self.pulls[n] for n in sorted(self.pulls)] + start = int(v["after"] or 0) + info = {"hasNextPage": start + 25 < len(nodes), "endCursor": str(start + 25)} + return { + "data": { + "repository": {**branch, "pullRequests": {"nodes": nodes[start : start + 25], "pageInfo": info}} + } + } + raise AssertionError(query) def __call__(self, method: str, endpoint: str, payload: Any = None) -> Any: self.calls.append((method, endpoint, payload)) + if endpoint == "graphql": + return self._graphql(payload) if any(m == method and endpoint.startswith(prefix) for m, prefix in self.fail): raise tool.GhError(f"{method} {endpoint} failed") - if endpoint == "graphql": - assert payload["query"] in tool.QUERIES.values() - v = payload["variables"] - if "number" in v: - data = self.fresh.get(v["number"], self.issues.get(v["number"])) - return {"data": {"repository": {"issueOrPullRequest": self._node(v["number"], data) if data else None}}} - nodes = [ - self._node(n, i) - for n, i in sorted(self.issues.items()) - if i.get("state", "OPEN") == "OPEN" and i.get("type", "Issue") == "Issue" - ] - return {"data": {"repository": {"issues": {"nodes": nodes, "pageInfo": {"hasNextPage": False}}}}} - number = int(endpoint.split("/")[4]) if re.match(rf"repos/{REPO}/(issues|pulls)/\d+", endpoint) else None + number = int(endpoint.split("/")[4]) if re.match(rf"repos/{REPO}/issues/\d+", endpoint) else None if method == "GET" and endpoint.startswith(f"repos/{REPO}/issues?state=closed&labels=blocked"): return [{"number": n} for n in self.closed_labelled] if method == "GET" and "/comments?" in endpoint: - return self.comments.get(number, []) + page = int(endpoint.rsplit("page=", 1)[1]) + return self.comments.get(number, [])[(page - 1) * tool._PAGE_SIZE : page * tool._PAGE_SIZE] if method == "GET" and "/events?" in endpoint: return self.events.get(number, []) if method == "POST" and endpoint.endswith("/labels"): @@ -366,11 +461,6 @@ def __call__(self, method: str, endpoint: str, payload: Any = None) -> Any: if method == "POST" and endpoint.endswith("/comments"): self.comments.setdefault(number, []).append({"user": {"type": "Bot"}, "body": payload["body"]}) return {} - if method == "GET" and endpoint.startswith(f"repos/{REPO}/pulls?state=open"): - page = int(endpoint.rsplit("page=", 1)[1]) - return self.pulls[(page - 1) * 100 : page * 100] - if method == "GET" and re.fullmatch(rf"repos/{REPO}/pulls/\d+", endpoint): - return next(p for p in self.pulls if p["number"] == number) if method == "GET" and "/check-runs?check_name=Issue%20dependencies" in endpoint: return {"check_runs": self.check_runs.get(endpoint.split("/")[4], [])} if method == "POST" and re.fullmatch(rf"repos/{REPO}/actions/jobs/\d+/rerun", endpoint): @@ -384,10 +474,22 @@ def reruns(self) -> List[str]: return [e for _, e in self.writes() if e.endswith("/rerun")] +RECENT = "2026-10-20T09:00:00Z" + + def run( - job_id: int, conclusion: Optional[str], status: str = "completed", app: str = "github-actions" -) -> Dict[str, Any]: - return {"id": job_id, "status": status, "conclusion": conclusion, "app": {"slug": app}} + job_id: int, + conclusion: Optional[str], + status: str = "completed", + app: str = "github-actions", + started: str = RECENT, +) -> dict: + return {"id": job_id, "status": status, "conclusion": conclusion, "app": {"slug": app}, "started_at": started} + + +@pytest.fixture(autouse=True) +def _fixed_clock(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setattr(tool, "_now", lambda: datetime.datetime(2026, 10, 20, 12, 0, tzinfo=datetime.timezone.utc)) # --- sync ------------------------------------------------------------------------------------------------------ @@ -412,6 +514,13 @@ def test_sync_labels_blocked_issues_and_unblocks_finished_ones() -> None: assert 40 not in gh.comments +def test_every_page_of_open_issues_is_read() -> None: + gh = FakeGitHub({n: {"labels": [], "blockers": [(1, "OPEN", None)]} for n in range(10, 15)}) + gh.issue_page = 2 + assert tool.run_sync(REPO, gh) == 0 + assert all(gh.issues[n]["labels"] == ["blocked"] for n in range(10, 15)) + + def test_a_dry_run_reads_but_never_writes(capsys: pytest.CaptureFixture[str]) -> None: gh = FakeGitHub({5: {"labels": [], "blockers": [(1, "OPEN", None)]}, 6: {"labels": ["blocked"], "blockers": []}}) gh.pull(30, "Fixes #5") @@ -431,14 +540,43 @@ def test_a_blocker_reopened_since_the_snapshot_does_not_unblock_the_issue() -> N def test_the_notice_goes_first_so_a_failed_label_removal_is_finished_without_a_second_notice() -> None: gh = FakeGitHub({6: {"labels": ["blocked"], "blockers": [(2, "CLOSED", "COMPLETED")]}}) gh.fail = [("DELETE", f"repos/{REPO}/issues/6/labels")] - with pytest.raises(tool.GhError): - tool.run_sync(REPO, gh) + assert tool.run_sync(REPO, gh) == 1 assert len(gh.comments[6]) == 1 and gh.issues[6]["labels"] == ["blocked"] gh.fail = [] assert tool.run_sync(REPO, gh) == 0 assert len(gh.comments[6]) == 1 and gh.issues[6]["labels"] == [] +def test_one_issue_that_cannot_be_written_stops_neither_the_others_nor_the_reconciliation() -> None: + gh = FakeGitHub( + { + 5: {"labels": [], "blockers": [(1, "OPEN", None)]}, # its label write fails + 6: {"labels": [], "blockers": [(1, "OPEN", None)]}, + } + ) + gh.fail = [("POST", f"repos/{REPO}/issues/5/labels")] + gh.pull(30, "Fixes #6") + gh.check_runs[SHA] = [run(77, "success")] + assert tool.run_sync(REPO, gh) == 1 # reported at the end + assert gh.issues[6]["labels"] == ["blocked"] and gh.reruns() == [f"repos/{REPO}/actions/jobs/77/rerun"] + + +def test_an_issue_whose_links_cannot_be_read_is_reported_and_never_treated_as_free( + capsys: pytest.CaptureFixture[str], +) -> None: + gh = FakeGitHub({5: {"labels": ["blocked"], "blockers": [(1, "OPEN", None)], "label_total": 101}}) + gh.pull(30, "Fixes #5") + gh.check_runs[SHA] = [run(77, "failure")] # red: must not be re-run as if #5 were free + assert tool.run_sync(REPO, gh) == 1 + assert gh.reruns() == [] and "more than 100 labels" in capsys.readouterr().err + + +def test_a_failing_list_of_closed_issues_is_reported_and_the_rest_still_runs() -> None: + gh = FakeGitHub({5: {"labels": [], "blockers": [(1, "OPEN", None)]}}) + gh.fail = [("GET", f"repos/{REPO}/issues?state=closed")] + assert tool.run_sync(REPO, gh) == 1 and gh.issues[5]["labels"] == ["blocked"] + + def test_a_cancelled_blocker_is_flagged_once_and_a_persons_copy_of_the_marker_does_not_silence_it() -> None: gh = FakeGitHub({5: {"labels": ["blocked"], "blockers": [(1, "CLOSED", "NOT_PLANNED")]}}) gh.comments[5] = [ @@ -452,18 +590,29 @@ def test_a_cancelled_blocker_is_flagged_once_and_a_persons_copy_of_the_marker_do def test_cancelled_blockers_with_one_number_in_two_repositories_each_get_a_notice() -> None: gh = FakeGitHub( - { - 5: { - "labels": ["blocked"], - "blockers": [(7, "CLOSED", "NOT_PLANNED"), (7, "CLOSED", "DUPLICATE", "HodeTech/Other")], - } - } + {5: {"labels": ["blocked"], "blockers": [(7, "CLOSED", "NOT_PLANNED"), (7, "CLOSED", "DUPLICATE", OTHER)]}} ) assert tool.run_sync(REPO, gh) == 0 bodies = [c["body"] for c in gh.comments[5]] assert len(bodies) == 2 and bodies[0].startswith("#7,") and bodies[1].startswith("HodeTech/Other#7,") +def test_an_endless_comment_thread_is_never_answered_with_a_repeated_notice(capsys: pytest.CaptureFixture[str]) -> None: + gh = FakeGitHub({}) + gh.comments[5] = [{"user": {"type": "User"}, "body": "chat"}] * (tool._PAGE_SIZE * tool._MAX_PAGES) + assert tool._already_noticed(gh, REPO, 5, "") is True + assert "too many comments" in capsys.readouterr().err + + +def test_an_earlier_notice_is_found_on_a_later_page_and_its_absence_is_reported_after_the_last() -> None: + gh = FakeGitHub({}) + chat = [{"user": {"type": "User"}, "body": "chat"}] * (tool._PAGE_SIZE + 50) + gh.comments[5] = chat + [{"user": {"type": "Bot"}, "body": ""}] + gh.comments[6] = chat + assert tool._already_noticed(gh, REPO, 5, "") is True + assert tool._already_noticed(gh, REPO, 6, "") is False + + # --- re-running the pull-request check when it no longer matches ----------------------------------------------- @@ -471,10 +620,13 @@ def test_cancelled_blockers_with_one_number_in_two_repositories_each_get_a_notic "blockers, last, rerun", [ ([(2, "OPEN", None)], run(77, "success"), True), # green, but the linked issue became blocked + ([(2, "OPEN", None)], run(77, "neutral"), True), # branch protection passes `neutral` too ([(2, "CLOSED", "COMPLETED")], run(77, "failure"), True), # red, but its blocker is completed now ([(2, "OPEN", None)], run(77, "failure"), False), # red and still blocked ([(2, "CLOSED", "COMPLETED")], run(77, "success"), False), # green and free + ([(2, "CLOSED", "COMPLETED")], run(77, "skipped"), False), # `skipped` passes as well ([(2, "CLOSED", "COMPLETED")], run(77, None, status="in_progress"), False), # still running: next sync looks + ([(2, "CLOSED", "COMPLETED")], run(77, "failure", started="2026-09-01T00:00:00Z"), False), # too old to re-run ], ) def test_the_sync_reruns_a_check_whose_result_no_longer_matches(blockers: list, last: dict, rerun: bool) -> None: @@ -497,9 +649,9 @@ def test_checks_from_another_app_and_pull_requests_that_link_nothing_are_left_al def test_every_page_of_open_pull_requests_is_reconciled() -> None: gh = FakeGitHub({6: {"labels": ["blocked"], "blockers": [(2, "OPEN", None)]}}) - for n in range(100): - gh.pull(1000 + n, "Docs only", sha=f"{n:040x}") - gh.pull(30, "Fixes #6") + for n in range(40): + gh.pull(100 + n, "Docs only", sha=f"{n:040x}") + gh.pull(999, "Fixes #6") gh.check_runs[SHA] = [run(77, "success")] assert tool.run_sync(REPO, gh) == 0 assert gh.reruns() == [f"repos/{REPO}/actions/jobs/77/rerun"] @@ -515,9 +667,9 @@ def test_a_failing_rerun_is_a_warning_not_a_failed_sync(capsys: pytest.CaptureFi assert ("POST", f"repos/{REPO}/actions/jobs/77/rerun") in [(m, e) for m, e, _ in gh.calls] -def test_a_failing_read_fails_the_sync() -> None: +def test_a_failing_read_of_the_open_issues_fails_the_sync() -> None: gh = FakeGitHub({}) - gh.fail = [("POST", "graphql")] + gh.fail = [("POST", "query:open_issues")] with pytest.raises(tool.GhError): tool.run_sync(REPO, gh) @@ -535,30 +687,30 @@ def test_more_links_or_labels_than_one_page_is_an_error_not_a_silent_truncation( def test_check_pr_reads_the_pull_request_again_instead_of_trusting_the_event() -> None: gh = FakeGitHub({5: {"labels": [], "blockers": [(1, "OPEN", None)]}, 1: {"labels": [], "blockers": []}}) - gh.pull(30, "Fixes #5") + gh.pull(30, "Fixes #5", closing=[5]) stale = {"pull_request": {"number": 30, "body": "Docs only", "labels": [{"name": "dependency-override"}]}} assert tool.run_check_pr(stale, REPO, gh) == 3 - gh.pulls[0]["labels"] = [{"name": "dependency-override"}] + gh.pull(30, "Fixes #5", closing=[5], labels=["dependency-override"]) gh.events[30] = [label_event("labeled", "cemililik")] assert tool.run_check_pr(stale, REPO, gh) == 0 def test_check_pr_ignores_an_override_label_a_contributor_set(capsys: pytest.CaptureFixture[str]) -> None: gh = FakeGitHub({5: {"labels": [], "blockers": [(1, "OPEN", None)]}}) - gh.pull(30, "Fixes #5", labels=("dependency-override",)) + gh.pull(30, "Fixes #5", labels=["dependency-override"]) gh.events[30] = [label_event("labeled", "contributor")] assert tool.run_check_pr({"pull_request": {"number": 30}}, REPO, gh) == 3 assert "is ignored" in capsys.readouterr().out -def test_check_pr_skips_linked_pull_requests_and_closed_issues() -> None: +def test_check_pr_passes_over_pull_requests_closed_issues_and_numbers_that_name_no_issue() -> None: gh = FakeGitHub( { 5: {"labels": [], "blockers": [(1, "OPEN", None)], "type": "PullRequest"}, 6: {"labels": [], "blockers": [(1, "OPEN", None)], "state": "CLOSED"}, } ) - gh.pull(30, "Refs #5, #6 and #404") + gh.pull(30, "Refs #5, #6 and #404") # #404 names no issue: GitHub answers NOT_FOUND, not a failure assert tool.run_check_pr({"pull_request": {"number": 30}}, REPO, gh) == 0 @@ -582,17 +734,20 @@ def test_main_rejects_a_missing_repository_a_missing_event_and_a_bad_command( monkeypatch.setenv("GITHUB_REPOSITORY", REPO) monkeypatch.delenv("GITHUB_EVENT_PATH", raising=False) assert tool.main(["check-pr"], gh) == 2 - broken = tmp_path / "event.json" - broken.write_text(json.dumps({"pull_request": {"number": "30"}}), encoding="utf-8") - monkeypatch.setenv("GITHUB_EVENT_PATH", str(broken)) + event = tmp_path / "event.json" + event.write_text(json.dumps({"pull_request": {"number": "30"}}), encoding="utf-8") + monkeypatch.setenv("GITHUB_EVENT_PATH", str(event)) + assert tool.main(["check-pr"], gh) == 2 + event.write_text("{not json", encoding="utf-8") assert tool.main(["check-pr"], gh) == 2 assert tool.main(["frobnicate"], gh) == 2 + assert tool.main(["--help"], gh) == 0 assert gh.calls == [] def test_main_turns_an_api_failure_into_exit_one(monkeypatch: pytest.MonkeyPatch) -> None: gh = FakeGitHub({}) - gh.fail = [("POST", "graphql")] + gh.fail = [("POST", "query:open_issues")] monkeypatch.setenv("GITHUB_REPOSITORY", REPO) assert tool.main(["sync"], gh) == 1 @@ -612,15 +767,17 @@ def test_gh_api_refuses_anything_outside_the_fixed_endpoints(method: str, endpoi tool.gh_api(method, endpoint, payload) +class Proc: + def __init__(self, returncode: int, stdout: str = "", stderr: str = "") -> None: + self.returncode, self.stdout, self.stderr = returncode, stdout, stderr + + def test_gh_api_runs_a_list_command_without_a_shell(monkeypatch: pytest.MonkeyPatch) -> None: seen: Dict[str, Any] = {} - class Done: - returncode, stdout, stderr = 0, "[]", "" - - def fake_run(command: List[str], **kwargs: Any) -> Done: + def fake_run(command: List[str], **kwargs: Any) -> Proc: seen.update(command=command, **kwargs) - return Done() + return Proc(0, "[]") monkeypatch.setattr(tool.subprocess, "run", fake_run) assert tool.gh_api("GET", f"repos/{REPO}/issues/1") == [] @@ -628,18 +785,41 @@ def fake_run(command: List[str], **kwargs: Any) -> Done: def test_gh_api_failure_text_is_flattened_and_defanged(monkeypatch: pytest.MonkeyPatch) -> None: - class Failed: - returncode, stdout, stderr = 1, "", "bad\n::error::injected" - - monkeypatch.setattr(tool.subprocess, "run", lambda *a, **k: Failed()) + monkeypatch.setattr(tool.subprocess, "run", lambda *a, **k: Proc(1, "", "bad\n::error::injected")) with pytest.raises(tool.GhError) as info: tool.gh_api("GET", f"repos/{REPO}/issues/1") assert "\n" not in str(info.value) and "::error::" not in str(info.value) -def test_a_graphql_error_is_a_gherror() -> None: - with pytest.raises(tool.GhError, match="GraphQL issue failed"): - tool.graphql(lambda *a: {"errors": [{"message": "nope"}]}, "issue", owner="o", name="n", number=1) +def test_gh_api_hands_a_graphql_answer_with_errors_to_the_caller_but_not_a_rest_one( + monkeypatch: pytest.MonkeyPatch, +) -> None: + # `gh` exits 1 for a GraphQL answer with errors and prints the answer itself; graphql() decides what it accepts. + answer = {"data": {"repository": {"issueOrPullRequest": None}}, "errors": [NOT_FOUND]} + monkeypatch.setattr(tool.subprocess, "run", lambda *a, **k: Proc(1, json.dumps(answer), "gh: Could not resolve")) + assert tool.gh_api("POST", "graphql", {"query": tool.QUERIES["issue"], "variables": {}}) == answer + with pytest.raises(tool.GhError): + tool.gh_api("GET", f"repos/{REPO}/issues/1") + + +@pytest.mark.parametrize( + "errors, accepted", + [ + ([NOT_FOUND], True), + ([NOT_FOUND, NOT_FOUND], False), + ([{**NOT_FOUND, "path": ["repository"]}], False), # the repository itself is missing + ([{**NOT_FOUND, "type": "FORBIDDEN"}], False), + ], +) +def test_graphql_accepts_only_the_one_error_a_missing_issue_number_produces(errors: list, accepted: bool) -> None: + answer = {"data": {"repository": {"issueOrPullRequest": None}}, "errors": errors} + if accepted: + assert tool.graphql(lambda *a: answer, "issue", allow_missing=True, owner="o", name="n", number=1) + else: + with pytest.raises(tool.GhError): + tool.graphql(lambda *a: answer, "issue", allow_missing=True, owner="o", name="n", number=1) + with pytest.raises(tool.GhError, match="GraphQL issue failed"): # never without allow_missing + tool.graphql(lambda *a: answer, "issue", owner="o", name="n", number=1) def test_the_maintainer_list_matches_the_assign_tool() -> None: @@ -668,14 +848,18 @@ def steps_of(workflow: Dict[Any, Any]) -> List[Dict[str, Any]]: @pytest.mark.parametrize("name", ["issue-dependencies.yml", "pr-dependencies.yml"]) -def test_no_expression_reaches_a_shell_and_the_checkout_is_pinned_and_minimal(name: str) -> None: +def test_two_steps_a_pinned_minimal_checkout_and_no_expression_in_a_shell(name: str) -> None: steps = steps_of(load(name)) - assert all("${{" not in str(step.get("run", "")) for step in steps) - checkout = steps[0] + assert len(steps) == 2 + checkout, script = steps assert re.fullmatch(r"actions/checkout@[0-9a-f]{40}", checkout["uses"]) - assert checkout["with"]["persist-credentials"] is False - assert checkout["with"]["sparse-checkout"] == "tools/issue_dependencies.py" - assert "ref" not in checkout["with"] # the default branch: never a pull request's code + assert checkout["with"] == { + "persist-credentials": False, + "sparse-checkout": "tools/issue_dependencies.py", + "sparse-checkout-cone-mode": False, + } # no `ref`: the default branch, never a pull request's code + assert "uses" not in script and "${{" not in script["run"] + assert script["env"] == {"GH_TOKEN": "${{ secrets.GITHUB_TOKEN }}"} def test_the_sync_workflow_triggers_permissions_and_concurrency() -> None: @@ -695,7 +879,7 @@ def test_the_sync_workflow_triggers_permissions_and_concurrency() -> None: "actions": "write", } assert wf["concurrency"] == {"group": "issue-dependencies", "cancel-in-progress": False} - assert "github.repository == 'HodeTech/ForgeLM'" in job["if"] + assert job["if"] == "${{ github.repository == 'HodeTech/ForgeLM' && github.ref == 'refs/heads/development' }}" assert steps_of(wf)[1]["run"] == "python3 tools/issue_dependencies.py sync" @@ -704,13 +888,29 @@ def test_the_pull_request_check_takes_its_definition_from_the_default_branch_and text = (ROOT / ".github/workflows/pr-dependencies.yml").read_text(encoding="utf-8") # `pull_request` would take this file from the pull request, which could then rewrite the check that judges it. assert list(wf[True]) == ["pull_request_target"] - assert set(wf[True]["pull_request_target"]) == {"types"} # no paths/branches filter: a required check must report - assert {"edited", "labeled", "unlabeled", "synchronize"} <= set(wf[True]["pull_request_target"]["types"]) + assert wf[True]["pull_request_target"] == { + "types": ["opened", "edited", "reopened", "synchronize", "ready_for_review", "labeled", "unlabeled"] + } # no paths/branches filter: a required check must always report assert wf["permissions"] == {} + assert wf["concurrency"] == { + "group": "pr-dependencies-${{ github.event.pull_request.number }}", + "cancel-in-progress": True, + } job = wf["jobs"]["issue-dependencies"] assert job["name"] == tool.CHECK_NAME == "Issue dependencies" # the required check's context assert job["permissions"] == {"contents": "read", "issues": "read", "pull-requests": "read"} code = "\n".join(line.split("#", 1)[0] for line in text.splitlines()) # the YAML without its comments - assert "head" not in code and "github.event.pull_request" not in code + assert "head" not in code and code.count("github.event") == 1 # only the pull request's number, for concurrency assert code.count("secrets.") == 1 # GITHUB_TOKEN only assert steps_of(wf)[1]["run"] == "python3 tools/issue_dependencies.py check-pr" + + +def test_no_other_workflow_has_a_job_named_like_the_check() -> None: + # Branch protection cannot tell two Actions checks of one name apart; keep the name unique in this repository. + owners = [ + path.name + for path in sorted((ROOT / ".github/workflows").glob("*.yml")) + for job in (yaml.safe_load(path.read_text(encoding="utf-8")).get("jobs") or {}).values() + if isinstance(job, dict) and job.get("name") == tool.CHECK_NAME + ] + assert owners == ["pr-dependencies.yml"] diff --git a/tools/assign_on_comment.py b/tools/assign_on_comment.py index 9deff8f6..112e69ff 100644 --- a/tools/assign_on_comment.py +++ b/tools/assign_on_comment.py @@ -79,9 +79,10 @@ MSG_CLOSED = "This issue is closed, so it cannot be assigned." MSG_BLOCKED = ( - "This issue is blocked: it waits for another issue to be completed (see **Blocked by** in the sidebar), so it " - "cannot be claimed yet. The `blocked` label is removed automatically once every blocker is closed as completed; " - "check this issue again then. Until then, pick another one from the [Front door view]({front_door})." + "This issue is blocked: it waits for another issue (see **Blocked by** in the sidebar) to be completed, or for a " + "maintainer to re-plan a dependency whose blocker was closed without being completed, so it cannot be claimed " + "yet. The `blocked` label is removed automatically once that is done; check this issue again then. Until then, " + "pick another one from the [Front door view]({front_door})." ) MSG_NOT_OPEN = ( "Only `good first issue` issues can be claimed with `/assign`; this one needs a maintainer's decision. " @@ -314,26 +315,31 @@ def _logins(users: Any) -> tuple[str, ...]: raise GhError("an assignee has no login") from exc -def fetch_issue(gh: GhApi, repo: str, number: int) -> IssueFacts: +def fetch_issue(gh: GhApi, repo: str, number: int, read_blockers: bool = True) -> IssueFacts: + """The issue's facts. ``read_blockers`` is False for ``/unassign``, which never depends on them, so a failing + dependency read can never stop a contributor from giving an issue back.""" raw = gh("GET", f"repos/{repo}/issues/{number}", None) try: labels = frozenset(str(label["name"]) for label in raw["labels"]) - summary = raw.get("issue_dependencies_summary") - # The summary's `blocked_by` counts only blockers that are still open. A blocker closed as not planned or as - # a duplicate blocks too, so when closed ones exist and the label has not caught up, read their reasons. - # Without the summary the label alone decides, which is what tools/issue_dependencies.py keeps true. unresolved = 0 - if isinstance(summary, dict): - unresolved = int(summary.get("blocked_by") or 0) - if int(summary.get("total_blocked_by") or 0) > unresolved and BLOCKED_LABEL not in labels: - unresolved += count_not_completed_blockers(gh, repo, number) + if read_blockers and BLOCKED_LABEL not in labels: # the label already refuses; no need to read further + summary = raw.get("issue_dependencies_summary") + # The summary's `blocked_by` counts only blockers that are still open. A blocker closed as not planned or + # as a duplicate blocks too, so when closed ones exist, or the summary is missing, read the list itself. + if not isinstance(summary, dict): + unresolved = count_unresolved_blockers(gh, repo, number) + else: + unresolved = int(summary.get("blocked_by") or 0) + if int(summary.get("total_blocked_by") or 0) > unresolved: + unresolved = count_unresolved_blockers(gh, repo, number) return IssueFacts(str(raw["state"]), labels, _logins(raw["assignees"]), unresolved) except (KeyError, TypeError, ValueError) as exc: raise GhError("the issue answer lacks labels, state or assignees") from exc -def count_not_completed_blockers(gh: GhApi, repo: str, number: int) -> int: - """The issue's "Blocked by" issues that are closed without being completed (not planned, or a duplicate).""" +def count_unresolved_blockers(gh: GhApi, repo: str, number: int) -> int: + """The issue's "Blocked by" issues that are still open or closed without being completed (not planned, or a + duplicate), counted from the list itself rather than mixed with the summary.""" found = 0 for page in range(1, _MAX_PAGES + 1): raw = gh("GET", f"repos/{repo}/issues/{number}/dependencies/blocked_by?per_page={_PAGE_SIZE}&page={page}", None) @@ -342,7 +348,7 @@ def count_not_completed_blockers(gh: GhApi, repo: str, number: int) -> int: found += sum( 1 for b in raw - if isinstance(b, dict) and b.get("state") == "closed" and b.get("state_reason") not in (None, "completed") + if isinstance(b, dict) and (b.get("state") != "closed" or b.get("state_reason") not in (None, "completed")) ) if len(raw) < _PAGE_SIZE: break @@ -426,7 +432,7 @@ def _settle_after_write(ctx: Context, gh: GhApi, assignees: tuple[str, ...], mes def _perform(ctx: Context, gh: GhApi) -> str: """Read, decide, change the assignment. Returns the reply text ('' for none).""" - issue = fetch_issue(gh, ctx.repo, ctx.number) + issue = fetch_issue(gh, ctx.repo, ctx.number, read_blockers=ctx.command == "/assign") counted: list[int] = [] def held() -> int: # asked for at most once per run, and only if the answer depends on it diff --git a/tools/issue_dependencies.py b/tools/issue_dependencies.py index d1c120f5..d4f64df9 100644 --- a/tools/issue_dependencies.py +++ b/tools/issue_dependencies.py @@ -20,21 +20,27 @@ - ``blocked`` is derived and never set by hand: ``sync`` adds it to every open issue with an unresolved blocker and removes it everywhere else, from a closed issue and from one where someone set it by hand. When it removes the label from an open issue it posts one comment; it does not comment when an issue becomes blocked. -- ``check-pr`` fails when the pull request's description links a blocked issue with ``Fixes``, ``Closes``, - ``Resolves`` or ``Refs`` — unless the same pull request closes every open blocker, or a maintainer listed in - ``MAINTAINER_LOGINS`` has labelled it ``dependency-override`` (the label set by anyone else is ignored). It reads - the links themselves, not the label, so it never trails behind ``sync``. - -Reading a description. A *link* is read leniently (a colon or a missing space after the keyword still counts), -because reading too many links can only make the check stricter. A *closing* link is read exactly as GitHub closes -issues — ``Fixes #12`` or ``Fixes owner/repo#12``, the keyword followed by whitespace, one keyword per issue — because -it exempts a blocker, and an exemption GitHub would not honour lets blocked work through. Fenced code, inline code -and HTML comments are skipped (the template's own examples live there); an unclosed fence or comment runs to the end, -as in CommonMark. The check sees what a description declares: it cannot see work on an issue the description does not -link, so it guards against honest mistakes, and a maintainer's review remains the backstop. +- ``check-pr`` fails while the pull request links a blocked issue — unless the same pull request closes every open + blocker, or a maintainer listed in ``MAINTAINER_LOGINS`` has labelled it ``dependency-override`` (the label set by + anyone else is ignored). It reads the links themselves, not the label, so it never trails behind ``sync``. + +Which issues a pull request links. Two sources, merged: + +- *What GitHub will close*: the pull request's ``closingIssuesReferences`` — GitHub's own list (the "Development" + sidebar), built from its closing keywords and manual links. Only this list can exempt a blocker as "closed by the + same pull request", and only when the pull request targets the default branch, where GitHub closes on merge. The + check never guesses closing syntax, so an exemption is never broader than what GitHub will actually do. +- *Everything else it mentions*: ``Fixes`` / ``Closes`` / ``Resolves`` / ``Refs`` references read leniently from the + title, the description and every commit message (a commit message can close an issue too). Reading too many links + can only make the check stricter. Fenced code, inline code and HTML comments are skipped (the template's own + examples live there). + +The check sees what a pull request declares: it cannot see work on an issue nothing links, so it guards against +honest mistakes, and a maintainer's review remains the backstop. At most ``_MAX_LINKED`` linked issues are read; more +fails the check without reading any, so a description cannot spend the repository's API budget. Trust boundary. Issue and pull-request text is written by anyone, so the script never executes or prints it: it -parses the description in linear time and writes only fixed messages and issue numbers to the log. The workflows pass +parses the text in linear time and writes only fixed messages and issue numbers to the log. The workflows pass nothing through the shell; the script reads the event file itself. Every subprocess call is a list without a shell, limited to fixed endpoints (``repos/`` paths built from integers, and ``graphql`` with a query constant from this module). @@ -42,7 +48,8 @@ Exit codes (per ``tools/`` contract, not the public 0-6 surface of ``forgelm/``): - ``0`` — done: labels in step, or the pull request may merge. -- ``1`` — unexpected failure: an API call failed, or GitHub's answer was not what its API documents. +- ``1`` — unexpected failure: an API call failed, or GitHub's answer was not what its API documents. ``sync`` keeps + going after one issue fails, reconciles the pull-request checks anyway, and exits 1 at the end. - ``2`` — the event payload, the arguments or the environment are unusable. - ``3`` — ``check-pr`` only: the pull request links an issue that is still blocked. """ @@ -50,13 +57,14 @@ from __future__ import annotations import argparse +import datetime import json import os import re import subprocess import sys from dataclasses import dataclass -from typing import Any, Callable, Iterable, Iterator, Mapping, Optional, Sequence +from typing import Any, Callable, Iterable, Mapping, Optional, Sequence from urllib.parse import quote BLOCKED_LABEL = "blocked" @@ -67,18 +75,28 @@ # a notice. `{blocker}` / `{blockers}` hold `owner/repo#N` references, so blockers in two repositories never collide. NOT_COMPLETED_MARKER = "" UNBLOCKED_MARKER = "" +# Conclusions branch protection accepts as a passing check. +PASSING_CONCLUSIONS = frozenset({"success", "neutral", "skipped"}) +_EXIT_BLOCKED = 3 _GH_TIMEOUT_SECONDS = 60 _PAGE_SIZE = 100 _MAX_PAGES = 30 # 3,000 items: far above the backlog; bounds every paginated read _MAX_BLOCKERS = 50 # GitHub shows at most this many "Blocked by" links per issue _MAX_LABELS = 100 # GitHub allows at most this many labels per issue +_MAX_LINKED = 50 # issues one pull request may link; each costs one API call +_MAX_COMMITS = 100 # commits one pull request may have and still be read in full +_RERUN_DAYS = 30 # GitHub re-runs a workflow run only within this many days _ALLOWED_METHODS = frozenset({"GET", "POST", "DELETE"}) MSG_UNBLOCKED = ( "This issue is no longer blocked: every issue it waited for is closed as completed ({blockers}). It can be " "picked up now; see [Working on an issue]({contributing})." ) +MSG_UNBLOCKED_TRIAGE = ( + "This issue is no longer blocked by another issue ({blockers}). It still carries `needs-triage`, so a maintainer " + "has to finish triaging it before it can be picked up." +) MSG_UNBLOCKED_NO_LINKS = ( 'This issue is no longer blocked: it has no open "Blocked by" link. The `blocked` label follows those links ' "and is set and removed automatically; see [Dependencies between issues]({contributing})." @@ -92,16 +110,16 @@ MAINTAINER_LOGINS = frozenset({"cemililik"}) # MAINTAINERS.md; tests/test_issue_dependencies.py keeps them equal MAINTAINER_MENTION = " ".join(f"@{login}" for login in sorted(MAINTAINER_LOGINS)) -# A reference in a pull-request description: `#12`, `owner/repo#12` or an issue URL, after a keyword. GitHub closes -# an issue for close/fix/resolve and their inflections; `Refs` is this project's word for "part of". Every class is -# explicit ASCII (GitHub's keywords, logins and repository names are ASCII; docs/standards/regex.md rule 1). +# A reference after a keyword: `#12`, `owner/repo#12` or an issue URL. Every class is explicit ASCII (keywords, +# logins and repository names are ASCII; docs/standards/regex.md rule 1), and a number is a whole token, so `#5abc` +# and `#５` are not references. _KEYWORD = r"(?Pclose[sd]?|fix(?:e[sd])?|resolve[sd]?|refs?|references)" -_OWNER, _NAME = r"[A-Za-z0-9-]+", r"[A-Za-z0-9._-]+" +_OWNER, _NAME, _NUM = r"[A-Za-z0-9-]+", r"[A-Za-z0-9._-]+", r"[0-9]+(?![A-Za-z0-9_])" _REF_RE = re.compile( - rf"(?:(?P{_OWNER})/(?P{_NAME}))?#(?P\d+)" - rf"|https://github\.com/(?P{_OWNER})/(?P{_NAME})/issues/(?P\d+)" + rf"(?:(?P{_OWNER})/(?P{_NAME}))?#(?P{_NUM})" + rf"|https?://github\.com/(?P{_OWNER})/(?P{_NAME})/issues/(?P{_NUM})" ) -_REF_PLAIN = rf"(?:(?:{_OWNER}/{_NAME})?#\d+|https://github\.com/{_OWNER}/{_NAME}/issues/\d+)" +_REF_PLAIN = rf"(?:(?:{_OWNER}/{_NAME})?#{_NUM}|https?://github\.com/{_OWNER}/{_NAME}/issues/{_NUM})" # Horizontal whitespace only (rule 5), and the optional colon owns the whitespace after it, so no two quantifiers # compete for the same characters (rule 4). _LINK_RE = re.compile( @@ -109,20 +127,18 @@ rf"(?P{_REF_PLAIN}(?:[ \t]*(?:,|and|&)[ \t]*{_REF_PLAIN})*)", re.IGNORECASE, ) -_CLOSING = frozenset({"close", "closes", "closed", "fix", "fixes", "fixed", "resolve", "resolves", "resolved"}) _FENCE_RE = re.compile(r" {0,3}(`{3,}|~{3,})") # matched at the start of one line by the walker below _BACKTICK_RUN_RE = re.compile(r"`+") # one quantifier over one character: linear -_INDENTED_RE = re.compile(r"(?: {4}| {0,3}\t)") # matched at the start of one line: four spaces, or a tab -_ATX_HEADING_RE = re.compile(r" {0,3}#{1,6}(?:[ \t]|$)") # matched at the start of one line -# Whole lines that end a block: a setext underline and a thematic break, in their exact CommonMark forms, so a line -# such as `-_-` stays paragraph text. Every repetition starts with its own character, so nothing backtracks. -_SETEXT_UNDERLINE_RE = re.compile(r" {0,3}(?:=+|-+)[ \t]*") -_THEMATIC_BREAK_RE = re.compile(r" {0,3}(?:(?:\*[ \t]*){3,}|(?:-[ \t]*){3,}|(?:_[ \t]*){3,})") # Every GraphQL document this script may send. A query is chosen by name; only integer and cursor variables vary. _ISSUE_FIELDS = """ labels(first: 100) { totalCount nodes { name } } blockedBy(first: 50) { totalCount nodes { number state stateReason repository { nameWithOwner } } }""" +_PULL_FIELDS = """ + number title body headRefOid baseRefName + labels(first: 100) { totalCount nodes { name } } + closingIssuesReferences(first: 50) { totalCount nodes { number repository { nameWithOwner } } } + commits(first: 100) { totalCount nodes { commit { message } } }""" QUERIES = { "open_issues": """ query($owner: String!, $name: String!, $after: String) { @@ -150,6 +166,29 @@ } } } +}""", + "pull": """ +query($owner: String!, $name: String!, $number: Int!) { + repository(owner: $owner, name: $name) { + defaultBranchRef { name } + pullRequest(number: $number) {""" + + _PULL_FIELDS + + """ + } + } +}""", + "open_pulls": """ +query($owner: String!, $name: String!, $after: String) { + repository(owner: $owner, name: $name) { + defaultBranchRef { name } + pullRequests(states: OPEN, first: 25, after: $after) { + pageInfo { hasNextPage endCursor } + nodes {""" + + _PULL_FIELDS + + """ + } + } + } }""", } @@ -207,6 +246,19 @@ class IssueState: blockers: tuple[Blocker, ...] +@dataclass(frozen=True) +class PullFacts: + """What the check needs to know about one pull request.""" + + number: int + head_sha: str + labels: frozenset[str] + texts: tuple[str, ...] # the title, the description and every commit message + closing: frozenset[str] # `owner/repo#N` keys GitHub will close on merge (empty off the default branch) + listed: frozenset[str] # every key in GitHub's closing list, whatever the base branch + complete: bool # False when GitHub's lists were longer than one page, so not everything could be read + + @dataclass(frozen=True) class Plan: """What ``sync`` does to one issue. ``comment`` is the unblocked notice and ``marker`` its idempotency key; @@ -245,7 +297,8 @@ def plan_issue(issue: IssueState, repo: str) -> Plan: return Plan() if issue.blockers: refs = ", ".join(b.ref(repo) for b in issue.blockers) - message = MSG_UNBLOCKED.format(blockers=refs, contributing=contributing_url(repo, "working-on-an-issue")) + template = MSG_UNBLOCKED_TRIAGE if TRIAGE_LABEL in issue.labels else MSG_UNBLOCKED + message = template.format(blockers=refs, contributing=contributing_url(repo, "working-on-an-issue")) marker = UNBLOCKED_MARKER.format(blockers=",".join(sorted(b.key for b in issue.blockers))) else: message = MSG_UNBLOCKED_NO_LINKS.format(contributing=contributing_url(repo)) @@ -264,8 +317,8 @@ def _strip_fences(body: str) -> str: A fence opens with three or more backticks or tildes after at most three spaces (a backtick fence's info string may not contain a backtick) and closes on a line holding the same character at least as many times and nothing - else. A fence that never closes runs to the end, as in CommonMark. Each dropped line is kept as a blank line, so - the block boundary the fence made survives for the readers after this one. + else. A fence that never closes runs to the end, as in CommonMark. Dropped lines are kept as blank lines, so a + code span is never paired across a fence. """ kept: list[str] = [] open_fence: Optional[tuple[str, int]] = None @@ -348,78 +401,38 @@ def _strip_inline_code(text: str) -> str: def _strip_non_prose(body: str) -> str: - """Drop fenced code, HTML comments and inline code: the template's own examples live there, and GitHub neither - links nor closes an issue from inside them.""" + """Drop fenced code, HTML comments and inline code: the template's own examples live there.""" return _strip_inline_code(_strip_html_comments(_strip_fences(body))) -def _ends_block(line: str) -> bool: - """A line after which a new block starts: an ATX heading, a setext underline or a thematic break. - - An `===` or `---` line ends a block only under a paragraph line; read without that context it may end one too - early, which can only drop a closing reference, never add one. - """ - return bool( - _ATX_HEADING_RE.match(line) or _SETEXT_UNDERLINE_RE.fullmatch(line) or _THEMATIC_BREAK_RE.fullmatch(line) - ) - - -def _strip_indented_code(text: str) -> str: - """Drop indented code blocks with a line walker (linear). +def linked_issues(text: str, repo: str) -> frozenset[int]: + """Issue numbers of this repository that ``text`` references after a keyword, read leniently. - A line indented four spaces (or a tab) opens a block only at the start, after a blank line or after a line that - ends a block (``_ends_block``): an indented code block cannot interrupt a paragraph, so a four-space line right - after paragraph text is a continuation and stays. Once open, the block runs through indented and blank lines. + A colon or a missing space after the keyword still counts, and so does every issue of a list (`Fixes #1, #2`): + these links can only make the check stricter. Whether GitHub *closes* an issue is never decided here — see + ``PullFacts.closing``. A reference to another repository is ignored: this repository's links cannot block on it. """ - kept: list[str] = [] - in_code = False - block_start = True - for line in text.splitlines(): - blank = not line.strip() - indented = bool(_INDENTED_RE.match(line)) - if in_code and (blank or indented): - kept.append("") - elif indented and block_start and not blank: - in_code = True - kept.append("") - else: - in_code = False - kept.append(line) - block_start = blank or _ends_block(line) - return "\n".join(kept) - - -def _references(text: str, repo: str) -> Iterator[tuple[int, bool]]: - """Each reference to an issue of this repository in ``text``, with whether GitHub closes it on merge.""" owner, name = repo.lower().split("/") - for match in _LINK_RE.finditer(text): - keyword, sep, refs = match.group("kw").lower(), match.group("sep"), match.group("refs") - if not sep and not refs.startswith("#"): + found: set[int] = set() + for match in _LINK_RE.finditer(_strip_non_prose(text or "")): + if not match.group("sep") and not match.group("refs").startswith("#"): continue # `refsfoo/bar#1` is a word, not a keyword followed by a reference - # GitHub closes only `KEYWORD #N` / `KEYWORD owner/repo#N`: whitespace after the keyword, no colon, one - # keyword per issue — so only the first reference of a list, and never a URL. - strict = keyword in _CLOSING and bool(sep) and not sep.strip(" \t") - for position, ref in enumerate(_REF_RE.finditer(refs)): + for ref in _REF_RE.finditer(match.group("refs")): ref_owner = (ref.group("owner") or ref.group("uowner") or owner).lower() ref_repo = (ref.group("repo") or ref.group("urepo") or name).lower() if (ref_owner, ref_repo) == (owner, name): - yield int(ref.group("num") or ref.group("unum")), strict and position == 0 and bool(ref.group("num")) + found.add(int(ref.group("num") or ref.group("unum"))) + return frozenset(found) -def linked_issues(body: str, repo: str) -> tuple[frozenset[int], frozenset[int]]: - """``(closing, all)``: issue numbers of this repository linked from a pull-request description. - - ``all`` holds every linked issue, read leniently; ``closing`` the ones GitHub will close on merge, read strictly - (see the module docstring). A reference to another repository is ignored: this repository's links cannot block on - it, and its workflow cannot close it. - """ - prose = _strip_non_prose(body or "") - every = frozenset(number for number, _ in _references(prose, repo)) - # Closing references are read without indented code blocks as well, and only they are: inside a list item, a - # line indented four spaces after a blank line is still prose, so dropping it from the links could hide a linked - # blocked issue, while dropping it from the closing references can only refuse an exemption. - closing = frozenset(number for number, closes in _references(_strip_indented_code(prose), repo) if closes) - return closing, every +def pull_links(pull: PullFacts, repo: str) -> frozenset[int]: + """Every issue of this repository the pull request links: what its texts mention and what GitHub lists.""" + home = repo.lower() + linked: set[int] = set() + for text in pull.texts: + linked |= linked_issues(text, repo) + linked |= {int(key.rsplit("#", 1)[1]) for key in pull.listed if key.rsplit("#", 1)[0] == home} + return frozenset(linked) @dataclass(frozen=True) @@ -432,8 +445,7 @@ class Verdict: def check_pull_request( override: Optional[bool], - closing: frozenset[int], - linked: frozenset[int], + pull: PullFacts, blockers_of: Callable[[int], Optional[tuple[Blocker, ...]]], repo: str, ) -> Verdict: @@ -441,38 +453,57 @@ def check_pull_request( ``override`` is ``None`` without the override label, ``True`` when a maintainer set it and ``False`` when someone else did (then it is ignored). ``blockers_of`` returns an open issue's links, or ``None`` for a pull request, a - closed issue or a number that does not exist (none of those can be blocked work). + closed issue or a number that does not exist (none of those can be blocked work); it may raise ``GhError``. """ - if override: - return Verdict(True, (f"`{OVERRIDE_LABEL}` was set by a maintainer, who accepted the linked dependencies.",)) + linked = pull_links(pull, repo) + if len(linked) > _MAX_LINKED or not pull.complete: + why = ( + f"links {len(linked)} issues; the check reads at most {_MAX_LINKED}" + if len(linked) > _MAX_LINKED + else f"has more than {_MAX_COMMITS} commits or 50 closing references, so not all of its links can be read" + ) + if override: + return Verdict(True, (f"This pull request {why}; `{OVERRIDE_LABEL}` was set by a maintainer.",)) + return Verdict(False, (f"This pull request {why}. Split it, or ask a maintainer.",)) notes: tuple[str, ...] = () if override is False: notes = (f"`{OVERRIDE_LABEL}` is ignored: only a maintainer listed in MAINTAINERS.md can set it.",) problems: list[str] = [] - checked = 0 + waiting = checked = 0 for number in sorted(linked): blockers = blockers_of(number) if blockers is None: continue checked += 1 for b in blockers: - if b.status == "resolved": - continue - if b.status == "waiting" and b.repo.lower() == repo.lower() and b.number in closing: - continue # this pull request closes the blocker as well - why = "is still open" if b.status == "waiting" else "was closed without being completed" - problems.append(f"#{number} is blocked by {b.ref(repo)}, which {why}.") - if problems: + if b.status == "resolved" or (b.status == "waiting" and b.key in pull.closing): + continue # completed, or closed by this pull request as well + if b.status == "waiting": + waiting += 1 + problems.append(f"#{number} is blocked by {b.ref(repo)}, which is still open.") + else: + problems.append( + f"#{number} is blocked by {b.ref(repo)}, which was closed without being completed: a maintainer " + "has to re-plan that dependency first (the issue is flagged `needs-triage`)." + ) + if override: return Verdict( - False, - tuple(problems) - + notes - + ( - "Wait until the blocker is closed as completed (this check is re-run within the hour), close it in " - f"this pull request too (`Fixes #N` for each), or ask a maintainer. See {contributing_url(repo)}", - ), + True, + (f"`{OVERRIDE_LABEL}` was set by a maintainer, who accepts:",) + tuple(problems or ["nothing is blocked"]), ) - return Verdict(True, (f"No linked issue is blocked ({checked} issue(s) checked).",) + notes) + if not problems: + return Verdict(True, (f"No linked issue is blocked ({checked} issue(s) checked).",) + notes) + hints = ( + "If this pull request does not work on that issue, remove the reference (title, description and commit " + "messages count).", + ) + if waiting: + hints = ( + "Wait until the blocker is closed as completed (this check is re-run within the hour), close it in this " + "pull request too (GitHub must list it under Development as closed by this pull request), or ask a " + "maintainer.", + ) + hints + return Verdict(False, tuple(problems) + notes + hints + (f"See {contributing_url(repo)}",)) # ------------------------------------------------------------------------------------------------ GitHub access @@ -482,7 +513,11 @@ def _clean(text: str, limit: int = 200) -> str: def gh_api(method: str, endpoint: str, payload: Optional[Mapping[str, Any]] = None) -> Any: - """Call the GitHub API through the ``gh`` CLI (authenticated by ``GH_TOKEN``); no shell is involved.""" + """Call the GitHub API through the ``gh`` CLI (authenticated by ``GH_TOKEN``); no shell is involved. + + ``gh`` exits non-zero when a GraphQL answer carries errors, even partial ones; that answer is returned so that + ``graphql`` can decide which errors it accepts. + """ if method not in _ALLOWED_METHODS or any(c.isspace() for c in endpoint): raise GhError("refusing an API call outside the fixed endpoints") if endpoint == "graphql": @@ -504,20 +539,39 @@ def gh_api(method: str, endpoint: str, payload: Optional[Mapping[str, Any]] = No ) except (OSError, subprocess.TimeoutExpired) as exc: raise GhError(f"{label} could not run: {type(exc).__name__}") from exc + answer: Any = None + if proc.stdout.strip(): + try: + answer = json.loads(proc.stdout) + except ValueError as exc: + if proc.returncode == 0: + raise GhError(f"{label} returned something that is not JSON") from exc if proc.returncode != 0: + if endpoint == "graphql" and isinstance(answer, Mapping) and "errors" in answer: + return answer raise GhError(f"{label} failed (exit {proc.returncode}): {_clean(proc.stderr)}") - if not proc.stdout.strip(): - return None - try: - return json.loads(proc.stdout) - except ValueError as exc: - raise GhError(f"{label} returned something that is not JSON") from exc + return answer -def graphql(gh: GhApi, query: str, **variables: Any) -> Mapping[str, Any]: +def _only_missing_issue(errors: Any) -> bool: + """Exactly one NOT_FOUND error, for ``repository.issueOrPullRequest``: the number names no issue.""" + return ( + isinstance(errors, list) + and len(errors) == 1 + and isinstance(errors[0], Mapping) + and errors[0].get("type") == "NOT_FOUND" + and errors[0].get("path") == ["repository", "issueOrPullRequest"] + ) + + +def graphql(gh: GhApi, query: str, *, allow_missing: bool = False, **variables: Any) -> Mapping[str, Any]: + """Run one of ``QUERIES``. ``allow_missing`` accepts the one error a missing issue number produces; any other + error, including one for the repository itself, raises.""" answer = gh("POST", "graphql", {"query": QUERIES[query], "variables": variables}) - if not isinstance(answer, Mapping) or answer.get("errors") or not isinstance(answer.get("data"), Mapping): - errors = answer.get("errors") if isinstance(answer, Mapping) else None + errors = answer.get("errors") if isinstance(answer, Mapping) else None + if errors and allow_missing and _only_missing_issue(errors): + errors = None + if not isinstance(answer, Mapping) or errors or not isinstance(answer.get("data"), Mapping): first = ( errors[0].get("message", "") if isinstance(errors, list) and errors and isinstance(errors[0], Mapping) @@ -527,39 +581,30 @@ def graphql(gh: GhApi, query: str, **variables: Any) -> Mapping[str, Any]: return answer["data"] -def _blockers(node: Mapping[str, Any]) -> tuple[Blocker, ...]: - try: - links = node["blockedBy"] - if links["totalCount"] > len(links["nodes"]): - raise GhError(f"issue #{node['number']} has more than {_MAX_BLOCKERS} blockers") - return tuple( - Blocker(int(b["number"]), str(b["repository"]["nameWithOwner"]), str(b["state"]), b.get("stateReason")) - for b in links["nodes"] - ) - except (KeyError, TypeError, ValueError) as exc: - raise GhError("a blockedBy answer lacks a field") from exc - - -def _labels(node: Mapping[str, Any]) -> frozenset[str]: - try: - labels = node["labels"] - if labels["totalCount"] > len(labels["nodes"]): - raise GhError(f"issue #{node['number']} has more than {_MAX_LABELS} labels") - return frozenset(str(label["name"]) for label in labels["nodes"]) - except (KeyError, TypeError) as exc: - raise GhError("a labels answer lacks a field") from exc +def _page(connection: Mapping[str, Any], limit: int, what: str, number: Any) -> list: + if connection["totalCount"] > len(connection["nodes"]): + raise GhError(f"#{number} has more than {limit} {what}") + return list(connection["nodes"]) def _issue_state(node: Mapping[str, Any], is_open: bool) -> IssueState: try: - return IssueState(int(node["number"]), is_open, _labels(node), _blockers(node)) + number = int(node["number"]) + labels = frozenset(str(label["name"]) for label in _page(node["labels"], _MAX_LABELS, "labels", number)) + blockers = tuple( + Blocker(int(b["number"]), str(b["repository"]["nameWithOwner"]), str(b["state"]), b.get("stateReason")) + for b in _page(node["blockedBy"], _MAX_BLOCKERS, "blockers", number) + ) + return IssueState(number, is_open, labels, blockers) except (KeyError, TypeError, ValueError) as exc: raise GhError("an issue answer lacks a field") from exc -def fetch_open_issues(gh: GhApi, repo: str) -> list[IssueState]: +def fetch_open_issues(gh: GhApi, repo: str) -> tuple[list[IssueState], set[int]]: + """Every open issue, and the numbers of those whose links could not be read (each reported to stderr).""" owner, name = repo.split("/") out: list[IssueState] = [] + broken: set[int] = set() after: Optional[str] = None for _ in range(_MAX_PAGES): data = graphql(gh, "open_issues", owner=owner, name=name, after=after) @@ -568,9 +613,14 @@ def fetch_open_issues(gh: GhApi, repo: str) -> list[IssueState]: nodes, info = page["nodes"], page["pageInfo"] except (KeyError, TypeError) as exc: raise GhError("the open-issues answer lacks a field") from exc - out += [_issue_state(n, True) for n in nodes] + for node in nodes: + try: + out.append(_issue_state(node, True)) + except GhError as exc: + broken.add(int(node.get("number") or 0)) + print(f"error: {_clean(str(exc))}", file=sys.stderr) if not info.get("hasNextPage"): - return out + return out, broken after = str(info["endCursor"]) raise GhError(f"more than {_MAX_PAGES} pages of open issues") @@ -593,9 +643,9 @@ def fetch_closed_labelled(gh: GhApi, repo: str) -> list[int]: def fetch_issue_state(gh: GhApi, repo: str, number: int) -> Optional[IssueState]: - """One issue as it stands now, or ``None`` when ``number`` is a pull request or does not exist.""" + """One issue as it stands now, or ``None`` when ``number`` is a pull request or names no issue.""" owner, name = repo.split("/") - data = graphql(gh, "issue", owner=owner, name=name, number=number) + data = graphql(gh, "issue", allow_missing=True, owner=owner, name=name, number=number) node = (data.get("repository") or {}).get("issueOrPullRequest") if not isinstance(node, Mapping) or node.get("__typename") != "Issue": return None @@ -603,39 +653,80 @@ def fetch_issue_state(gh: GhApi, repo: str, number: int) -> Optional[IssueState] def fetch_blockers_of(gh: GhApi, repo: str, number: int) -> Optional[tuple[Blocker, ...]]: - """An open issue's links, or ``None`` when ``number`` is a pull request, a closed issue or does not exist.""" + """An open issue's links, or ``None`` when ``number`` is a pull request, a closed issue or names no issue.""" state = fetch_issue_state(gh, repo, number) return state.blockers if state is not None and state.is_open else None +def _pull_facts(node: Mapping[str, Any], default_branch: str) -> PullFacts: + try: + number, sha = int(node["number"]), str(node["headRefOid"]) + if not re.fullmatch(r"[0-9a-f]{40}", sha): + raise GhError(f"pull request #{number} has a malformed head sha") + labels = frozenset(str(label["name"]) for label in _page(node["labels"], _MAX_LABELS, "labels", number)) + refs, commits = node["closingIssuesReferences"], node["commits"] + complete = refs["totalCount"] <= len(refs["nodes"]) and commits["totalCount"] <= len(commits["nodes"]) + listed = frozenset(f"{str(r['repository']['nameWithOwner']).lower()}#{int(r['number'])}" for r in refs["nodes"]) + texts = (str(node.get("title") or ""), str(node.get("body") or "")) + tuple( + str((c.get("commit") or {}).get("message") or "") for c in commits["nodes"] + ) + # GitHub closes on merge only into the default branch; elsewhere nothing is exempted. + closing = listed if node["baseRefName"] == default_branch else frozenset() + return PullFacts(number, sha, labels, texts, closing, listed, complete) + except (AttributeError, KeyError, TypeError, ValueError) as exc: + raise GhError("a pull-request answer lacks a field") from exc + + +def _default_branch(data: Mapping[str, Any]) -> str: + try: + return str(data["repository"]["defaultBranchRef"]["name"]) + except (KeyError, TypeError) as exc: + raise GhError("the answer lacks the default branch") from exc + + +def fetch_pull(gh: GhApi, repo: str, number: int) -> PullFacts: + owner, name = repo.split("/") + data = graphql(gh, "pull", owner=owner, name=name, number=number) + node = (data.get("repository") or {}).get("pullRequest") + if not isinstance(node, Mapping): + raise GhError(f"pull request #{number} was not found") + return _pull_facts(node, _default_branch(data)) + + def override_authorized(gh: GhApi, repo: str, number: int) -> bool: """True when the latest change of the override label on this pull request is a maintainer adding it. Who may *add* a label is a repository permission, not a maintainer decision, so the label alone proves nothing. - The issue events (oldest first) record who added or removed it. + The issue events record who added or removed it; they are sorted by time rather than trusted to arrive in order. """ - last: Optional[Mapping[str, Any]] = None + changes: list[Mapping[str, Any]] = [] for page in range(1, _MAX_PAGES + 1): raw = gh("GET", f"repos/{repo}/issues/{number}/events?per_page={_PAGE_SIZE}&page={page}", None) if not isinstance(raw, list): raise GhError("the events answer is not a list") - for event in raw: - if ( - isinstance(event, Mapping) - and event.get("event") in ("labeled", "unlabeled") - and (event.get("label") or {}).get("name") == OVERRIDE_LABEL - ): - last = event + changes += [ + e + for e in raw + if isinstance(e, Mapping) + and e.get("event") in ("labeled", "unlabeled") + and (e.get("label") or {}).get("name") == OVERRIDE_LABEL + ] if len(raw) < _PAGE_SIZE: break else: raise GhError(f"more than {_MAX_PAGES} pages of events") - if last is None or last.get("event") != "labeled": + if not changes: return False - return str((last.get("actor") or {}).get("login") or "").lower() in MAINTAINER_LOGINS + last = max(changes, key=lambda e: (str(e.get("created_at") or ""), int(e.get("id") or 0))) + return ( + last.get("event") == "labeled" + and str((last.get("actor") or {}).get("login") or "").lower() in MAINTAINER_LOGINS + ) def _already_noticed(gh: GhApi, repo: str, number: int, marker: str) -> bool: + """True when the bot has posted ``marker`` on the issue. Past the page bound it answers True (and says so), so a + very long thread cannot make the bot repeat a notice every hour.""" for page in range(1, _MAX_PAGES + 1): raw = gh("GET", f"repos/{repo}/issues/{number}/comments?per_page={_PAGE_SIZE}&page={page}", None) if not isinstance(raw, list): @@ -646,25 +737,27 @@ def _already_noticed(gh: GhApi, repo: str, number: int, marker: str) -> bool: return True if len(raw) < _PAGE_SIZE: return False - return False + print(f"warning: issue #{number}: too many comments to look for an earlier notice; not posting", file=sys.stderr) + return True + + +def _now() -> datetime.datetime: + return datetime.datetime.now(datetime.timezone.utc) def _reconcile_one( gh: GhApi, repo: str, - pull: Mapping[str, Any], + pull: PullFacts, blockers_of: Callable[[int], Optional[tuple[Blocker, ...]]], dry_run: bool, ) -> bool: """Re-run one pull request's dependency check if its last result no longer matches; True when it did.""" - number, sha = int(pull["number"]), str(pull["head"]["sha"]) - if not re.fullmatch(r"[0-9a-f]{40}", sha): - raise GhError("a pull request's head sha is malformed") - labels = frozenset(str(label["name"]) for label in pull["labels"]) - closing, linked = linked_issues(str(pull.get("body") or ""), repo) - override = override_authorized(gh, repo, number) if OVERRIDE_LABEL in labels else None - want_ok = check_pull_request(override, closing, linked, blockers_of, repo).ok - runs = gh("GET", f"repos/{repo}/commits/{sha}/check-runs?check_name={quote(CHECK_NAME)}&per_page=100", None) + override = override_authorized(gh, repo, pull.number) if OVERRIDE_LABEL in pull.labels else None + want_ok = check_pull_request(override, pull, blockers_of, repo).ok + runs = gh( + "GET", f"repos/{repo}/commits/{pull.head_sha}/check-runs?check_name={quote(CHECK_NAME)}&per_page=100", None + ) candidates = [ r for r in (runs or {}).get("check_runs") or [] if (r.get("app") or {}).get("slug") == "github-actions" ] @@ -673,17 +766,25 @@ def _reconcile_one( latest = max(candidates, key=lambda r: int(r["id"])) if latest.get("status") != "completed": return False # still running on what it read; the next sync compares again - if (latest.get("conclusion") == "success") == want_ok: + if (latest.get("conclusion") in PASSING_CONCLUSIONS) == want_ok: + return False + started = datetime.datetime.fromisoformat(str(latest.get("started_at") or "").replace("Z", "+00:00")) + if _now() - started > datetime.timedelta(days=_RERUN_DAYS): + print( + f"warning: pull request #{pull.number}: its dependency check is too old to re-run; a push or an edit of the " + "pull request runs it again", + file=sys.stderr, + ) return False - print(f"{'would re-run' if dry_run else 'did re-run'} the dependency check of pull request #{number}") if not dry_run: # An Actions check run's id is its job's id. gh("POST", f"repos/{repo}/actions/jobs/{int(latest['id'])}/rerun", None) + print(f"{'would re-run' if dry_run else 'did re-run'} the dependency check of pull request #{pull.number}") return True def reconcile_pr_checks( - gh: GhApi, repo: str, open_issues: Mapping[int, IssueState], dry_run: bool = False + gh: GhApi, repo: str, open_issues: Mapping[int, IssueState], broken: Iterable[int] = (), dry_run: bool = False ) -> tuple[int, int]: """Re-run the dependency check of every open pull request whose last result no longer matches its linked issues: red although every blocker is now completed, or green although a linked issue became blocked. @@ -692,25 +793,31 @@ def reconcile_pr_checks( running, or a sync that stopped half-way is simply caught by the next run. Optional by design: a failure is a warning, never a failed sync. Returns ``(pull requests looked at, checks re-run)``. """ + unreadable = frozenset(broken) def blockers_of(number: int) -> Optional[tuple[Blocker, ...]]: + if number in unreadable: + raise GhError(f"the Blocked-by links of #{number} could not be read") issue = open_issues.get(number) return issue.blockers if issue is not None else None + owner, name = repo.split("/") seen = rerun = 0 try: - for page in range(1, _MAX_PAGES + 1): - pulls = gh("GET", f"repos/{repo}/pulls?state=open&per_page={_PAGE_SIZE}&page={page}", None) - if not isinstance(pulls, list): - raise GhError("the pull-request list is not a list") - for pull in pulls: + after: Optional[str] = None + for _ in range(_MAX_PAGES): + data = graphql(gh, "open_pulls", owner=owner, name=name, after=after) + default_branch = _default_branch(data) + page = data["repository"]["pullRequests"] + for node in page["nodes"]: seen += 1 try: - rerun += _reconcile_one(gh, repo, pull, blockers_of, dry_run) + rerun += _reconcile_one(gh, repo, _pull_facts(node, default_branch), blockers_of, dry_run) except (GhError, AttributeError, KeyError, TypeError, ValueError) as exc: print(f"warning: could not reconcile a pull request's check: {_clean(str(exc))}", file=sys.stderr) - if len(pulls) < _PAGE_SIZE: + if not page["pageInfo"].get("hasNextPage"): break + after = str(page["pageInfo"]["endCursor"]) else: print(f"warning: more than {_MAX_PAGES} pages of open pull requests; the rest wait", file=sys.stderr) except (GhError, AttributeError, KeyError, TypeError, ValueError) as exc: @@ -719,53 +826,67 @@ def blockers_of(number: int) -> Optional[tuple[Blocker, ...]]: # ----------------------------------------------------------------------------------------------------- commands +def _sync_issue(gh: GhApi, repo: str, issue: IssueState, dry_run: bool) -> bool: + """Apply the rules to one issue; True when an open issue was unblocked.""" + plan = plan_issue(issue, repo) + if plan.remove_label and issue.is_open and not dry_run: + # Read the issue again right before acting on it: a blocker reopened since the snapshot must not lead to an + # "unblocked" notice that a later run cannot take back. + fresh = fetch_issue_state(gh, repo, issue.number) + if fresh is None: + print(f"skipped #{issue.number}: no longer an issue") + return False + issue, plan = fresh, plan_issue(fresh, repo) + verb = "would" if dry_run else "did" + if plan.add_label: + if not dry_run: + gh("POST", f"repos/{repo}/issues/{issue.number}/labels", {"labels": [BLOCKED_LABEL]}) + print(f"{verb} label #{issue.number} {BLOCKED_LABEL}") + if plan.remove_label: + if not dry_run: + # The notice goes first and is idempotent through its marker, and the label goes last: if anything fails in + # between, the label is still there and the next run finishes the job without a second notice. + if plan.comment and not _already_noticed(gh, repo, issue.number, plan.marker): + gh("POST", f"repos/{repo}/issues/{issue.number}/comments", {"body": plan.comment}) + gh("DELETE", f"repos/{repo}/issues/{issue.number}/labels/{quote(BLOCKED_LABEL)}", None) + print(f"{verb} unlabel #{issue.number} {BLOCKED_LABEL}" + (" and comment" if plan.comment else "")) + for b in plan.not_completed: + if dry_run: + print(f"would check #{issue.number}: blocker {b.ref(repo)} was not completed") + continue + if _already_noticed(gh, repo, issue.number, NOT_COMPLETED_MARKER.format(blocker=b.key)): + continue + gh("POST", f"repos/{repo}/issues/{issue.number}/labels", {"labels": [TRIAGE_LABEL]}) + gh("POST", f"repos/{repo}/issues/{issue.number}/comments", {"body": not_completed_notice(b, repo)}) + print(f"did flag #{issue.number}: blocker {b.ref(repo)} was not completed") + return plan.remove_label and issue.is_open + + def run_sync(repo: str, gh: GhApi, dry_run: bool = False) -> int: - open_issues = fetch_open_issues(gh, repo) - closed = [IssueState(n, False, frozenset({BLOCKED_LABEL}), ()) for n in fetch_closed_labelled(gh, repo)] + open_issues, broken = fetch_open_issues(gh, repo) + failures = len(broken) + try: + closed = [IssueState(n, False, frozenset({BLOCKED_LABEL}), ()) for n in fetch_closed_labelled(gh, repo)] + except GhError as exc: + failures += 1 + closed = [] + print(f"error: could not list closed issues that keep `{BLOCKED_LABEL}`: {_clean(str(exc))}", file=sys.stderr) blocked = sum(1 for i in open_issues if is_blocked(i.blockers)) - verb = "would" if dry_run else "did" unblocked = 0 for issue in open_issues + closed: - plan = plan_issue(issue, repo) - if plan.remove_label and issue.is_open and not dry_run: - # Read the issue again right before acting on it: a blocker reopened since the snapshot must not lead to - # an "unblocked" notice that a later run cannot take back. - fresh = fetch_issue_state(gh, repo, issue.number) - if fresh is None: - print(f"skipped #{issue.number}: no longer an issue") - continue - issue, plan = fresh, plan_issue(fresh, repo) - if not plan.changes: - continue - if plan.add_label: - print(f"{verb} label #{issue.number} {BLOCKED_LABEL}") - if not dry_run: - gh("POST", f"repos/{repo}/issues/{issue.number}/labels", {"labels": [BLOCKED_LABEL]}) - if plan.remove_label: - print(f"{verb} unlabel #{issue.number} {BLOCKED_LABEL}" + (" and comment" if plan.comment else "")) - if not dry_run: - # The notice goes first and is idempotent through its marker, and the label goes last: if anything - # fails in between, the label is still there and the next run finishes the job without a second - # notice. - if plan.comment and not _already_noticed(gh, repo, issue.number, plan.marker): - gh("POST", f"repos/{repo}/issues/{issue.number}/comments", {"body": plan.comment}) - gh("DELETE", f"repos/{repo}/issues/{issue.number}/labels/{quote(BLOCKED_LABEL)}", None) - unblocked += issue.is_open - for b in plan.not_completed: - if dry_run: - print(f"would check #{issue.number}: blocker {b.ref(repo)} was not completed") - continue - if _already_noticed(gh, repo, issue.number, NOT_COMPLETED_MARKER.format(blocker=b.key)): - continue - print(f"did flag #{issue.number}: blocker {b.ref(repo)} was not completed") - gh("POST", f"repos/{repo}/issues/{issue.number}/labels", {"labels": [TRIAGE_LABEL]}) - gh("POST", f"repos/{repo}/issues/{issue.number}/comments", {"body": not_completed_notice(b, repo)}) - seen, rerun = reconcile_pr_checks(gh, repo, {i.number: i for i in open_issues}, dry_run) + # One issue that cannot be written (locked, at its label limit, ...) must not stop the others, nor the + # pull-request reconciliation below. + try: + unblocked += _sync_issue(gh, repo, issue, dry_run) + except GhError as exc: + failures += 1 + print(f"error: issue #{issue.number}: {_clean(str(exc))}", file=sys.stderr) + seen, rerun = reconcile_pr_checks(gh, repo, {i.number: i for i in open_issues}, broken, dry_run) print( f"open issues: {len(open_issues)}, blocked: {blocked}, unblocked now: {unblocked}, " - f"open pull requests: {seen}, dependency checks re-run: {rerun}" + f"open pull requests: {seen}, dependency checks re-run: {rerun}, failures: {failures}" ) - return 0 + return 1 if failures else 0 def run_check_pr(event: Mapping[str, Any], repo: str, gh: GhApi) -> int: @@ -776,19 +897,13 @@ def run_check_pr(event: Mapping[str, Any], repo: str, gh: GhApi) -> int: if isinstance(number, bool) or not isinstance(number, int) or number < 1: raise EventError("pull request number is not a positive integer") # Read the pull request again rather than trust the event: a re-run replays the original payload, and the - # description or the labels may have changed since. - pull = gh("GET", f"repos/{repo}/pulls/{number}", None) - try: - body = str(pull.get("body") or "") - labels = frozenset(str(label["name"]) for label in pull["labels"]) - except (AttributeError, KeyError, TypeError) as exc: - raise GhError("the pull-request answer lacks body or labels") from exc - closing, linked = linked_issues(body, repo) - override = override_authorized(gh, repo, number) if OVERRIDE_LABEL in labels else None - verdict = check_pull_request(override, closing, linked, lambda n: fetch_blockers_of(gh, repo, n), repo) + # description, the labels or the commits may have changed since. + pull = fetch_pull(gh, repo, number) + override = override_authorized(gh, repo, number) if OVERRIDE_LABEL in pull.labels else None + verdict = check_pull_request(override, pull, lambda n: fetch_blockers_of(gh, repo, n), repo) for line in verdict.lines: print(line if verdict.ok else f"::error::{line}") - return 0 if verdict.ok else 3 + return 0 if verdict.ok else _EXIT_BLOCKED def _repo_from_env() -> str: