Merge pull request #913 from PolicyEngine/rule-weakening-guard

Catch packages leaving an enforced rule and new ratcheted rules, and give each downgrade an open tracking issue

Author: anth-volk

.github/scripts/check_pr_description.py

@@ -23,6 +23,11 @@
   the line isn't what the author meant.
 - An empty or missing (``null``) description fails.
 - The number is ASCII digits without a leading zero.
+
+``require_open_issue`` is the issue lookup on its own. The rules guard
+(``scripts/rules/tracking_issues.py``) loads this file to check that each
+``[rule.downgrade]`` names an open issue. Its errors say only what is wrong, and
+each caller adds its own fix.
 """
 
 from __future__ import annotations
@@ -41,6 +46,14 @@
     "description runs this check again. See docs/engineering/skills/github-prs.md "
     f"(rule {RULE_ID})."
 )
+RETRY_LOOKUP = (
+    "Re-run this job; if it fails again, check the GH_TOKEN the workflow gives it."
+)
+REOPEN_ISSUE = (
+    "If this pull request still fixes it, reopen it and re-run this job; "
+    "otherwise open an issue for this work and edit the first line to name it "
+    f"(rule {RULE_ID})."
+)
 FIXES_LINE = re.compile(r"Fixes #([1-9][0-9]*)")
 # Markdown line endings: CRLF, a carriage return alone, or a line feed.
 LINE_ENDING = re.compile(r"\r\n|\r|\n")
@@ -55,7 +68,29 @@
 
 
 class DescriptionError(Exception):
-    """The description, or the issue it names, does not satisfy the rule."""
+    """The description's first line does not name an issue as the rule requires."""
+
+
+class IssueLookupError(Exception):
+    """GitHub couldn't be asked about an issue, or its answer wasn't an issue.
+
+    Running the lookup again may succeed. The message says what went wrong,
+    without a fix.
+    """
+
+
+class NotAnOpenIssueError(Exception):
+    """A number names no open issue in the repository.
+
+    ``state`` is the issue's state, such as ``closed``, when the number names an
+    issue in the repository that isn't open, and None when it names no issue
+    there. The message says what the number names, without a fix.
+    """
+
+    def __init__(self, message: str, *, state: str | None = None) -> None:
+        """Keep ``message`` as the error's text, and the issue's ``state``."""
+        super().__init__(message)
+        self.state = state
 
 
 def _shown(line: str) -> str:
@@ -102,7 +137,9 @@ def fetch_issue(repository: str, number: int) -> dict[str, object]:
 
     The issues API also answers for pull requests, and gh follows the redirect
     GitHub sends for an issue transferred to another repository, so the answer
-    still has to be checked with ``check_issue``.
+    still has to be checked with ``check_issue``. Raise ``NotAnOpenIssueError``
+    when GitHub has no issue ``number``, and ``IssueLookupError`` when the
+    lookup fails.
     """
     completed = subprocess.run(
         ["gh", "api", f"repos/{repository}/issues/{number}"],
@@ -112,21 +149,18 @@ def fetch_issue(repository: str, number: int) -> dict[str, object]:
     )
     if completed.returncode != 0:
         if MISSING_ISSUE.search(completed.stderr):
-            msg = f"There is no issue #{number} in {repository}. {HOW_TO_FIX}"
-            raise DescriptionError(msg)
+            msg = f"There is no issue #{number} in {repository}."
+            raise NotAnOpenIssueError(msg)
         reported = completed.stderr.strip() or f"exit status {completed.returncode}"
-        msg = (
-            f"Could not look up #{number} in {repository}: {reported}. Re-run this "
-            "job; if it fails again, check the GH_TOKEN the workflow gives it."
-        )
-        raise DescriptionError(msg)
+        msg = f"Could not look up #{number} in {repository}: {reported}."
+        raise IssueLookupError(msg)
     try:
         issue = json.loads(completed.stdout)
     except json.JSONDecodeError:
         issue = None
     if not isinstance(issue, dict):
-        msg = f"gh api did not print an issue for #{number}. Re-run this job."
-        raise DescriptionError(msg)
+        msg = f"gh api did not print an issue for #{number}."
+        raise IssueLookupError(msg)
     return issue
 
 
@@ -139,30 +173,41 @@ def _repository_of(repository_url: object) -> str | None:
 
 
 def check_issue(repository: str, number: int, issue: dict[str, object]) -> None:
-    """Raise ``DescriptionError`` unless ``issue`` is open issue ``number`` here."""
+    """Return quietly if ``issue``, GitHub's answer for ``number``, is open here.
+
+    Raise ``NotAnOpenIssueError`` when it is a pull request, an issue moved to
+    another repository or an issue that isn't open, and ``IssueLookupError``
+    when the answer names no repository.
+    """
     issue_repository = _repository_of(issue.get("repository_url"))
     if issue_repository is None:
         msg = (
             f"GitHub's answer for #{number} names no repository, so it can't be "
-            f"confirmed as an issue in {repository}. Re-run this job."
+            f"confirmed as an issue in {repository}."
         )
-        raise DescriptionError(msg)
+        raise IssueLookupError(msg)
     if issue_repository.lower() != repository.lower() or issue.get("number") != number:
         msg = (
             f"#{number} is not an issue in {repository}: it moved to "
-            f"{issue_repository}#{issue.get('number')}. {HOW_TO_FIX}"
+            f"{issue_repository}#{issue.get('number')}."
         )
-        raise DescriptionError(msg)
+        raise NotAnOpenIssueError(msg)
     if issue.get("pull_request") is not None:
-        msg = f"#{number} is a pull request, not an issue. {HOW_TO_FIX}"
-        raise DescriptionError(msg)
-    if issue.get("state") != "open":
-        msg = (
-            f"Issue #{number} is {issue.get('state')}. If this pull request still "
-            "fixes it, reopen it and re-run this job; otherwise open an issue for "
-            f"this work and edit the first line to name it (rule {RULE_ID})."
-        )
-        raise DescriptionError(msg)
+        msg = f"#{number} is a pull request, not an issue."
+        raise NotAnOpenIssueError(msg)
+    state = issue.get("state")
+    if state != "open":
+        msg = f"Issue #{number} is {state}."
+        raise NotAnOpenIssueError(msg, state=str(state))
+
+
+def require_open_issue(repository: str, number: int) -> None:
+    """Return quietly if ``number`` is an open issue, not a pull request, here.
+
+    Raise ``NotAnOpenIssueError`` or ``IssueLookupError`` otherwise; see
+    ``fetch_issue`` and ``check_issue``.
+    """
+    check_issue(repository, number, fetch_issue(repository, number))
 
 
 def _required_environment(name: str) -> str:
@@ -198,15 +243,21 @@ def main() -> int:
     description = _description_from_event(_required_environment("GITHUB_EVENT_PATH"))
     try:
         number = issue_number_from_description(description)
-        check_issue(repository, number, fetch_issue(repository, number))
+        require_open_issue(repository, number)
     except DescriptionError as error:
-        print(f"::error title={ANNOTATION_TITLE}::{_annotation_text(str(error))}")
-        return 1
-    print(
-        f"The first line is Fixes #{number}, and #{number} is an open issue in "
-        f"{repository}."
-    )
-    return 0
+        message = str(error)
+    except IssueLookupError as error:
+        message = f"{error} {RETRY_LOOKUP}"
+    except NotAnOpenIssueError as error:
+        message = f"{error} {HOW_TO_FIX if error.state is None else REOPEN_ISSUE}"
+    else:
+        print(
+            f"The first line is Fixes #{number}, and #{number} is an open issue in "
+            f"{repository}."
+        )
+        return 0
+    print(f"::error title={ANNOTATION_TITLE}::{_annotation_text(message)}")
+    return 1
 
 
 if __name__ == "__main__":

.github/workflows/pr.yml

@@ -107,6 +107,11 @@ jobs:
     # Full CI runs only on the lowest unmerged pull request in a stack.
     if: ${{ !github.event.pull_request.stack || github.event.pull_request.stack.base.ref == github.event.pull_request.base.ref }}
     runs-on: ubuntu-latest
+    # The checkout reads the repository, and the rules guard looks up the
+    # tracking issue of each [rule.downgrade] with the job's token.
+    permissions:
+      contents: read
+      issues: read
 
     steps:
     - uses: actions/checkout@v6
@@ -129,6 +134,10 @@ jobs:
       run: pip install "shellcheck-py==0.11.0.1"
 
     - name: Run quality guards
+      env:
+        # Read by gh, which the rules guard runs to confirm that each
+        # [rule.downgrade] names an open issue; without it the guard fails.
+        GH_TOKEN: ${{ github.token }}
       run: python scripts/run_quality_guards.py
 
   pyright:

docs/engineering/skills/rules-registry.md

@@ -21,7 +21,8 @@ python -m unittest discover -s scripts/rules/tests -t .            # the tool's
 ```
 
 All commands use only the standard library, so they run without installing the
-project.
+project. When a rule has a `[rule.downgrade]` section, `guard` also runs
+`gh api` to look up its tracking issue; see [Baselines](#baselines).
 
 ## Adding or changing a rule
 
@@ -58,7 +59,7 @@ gap is visible, rather than leaving it out.
 | `[rule.proof]` | Optional; see "Proof". |
 | `[rule.baseline]` | Only for `ratcheted` rules: `file` listing tolerated violations, and their `count`. |
 | `[[rule.exception]]` | Temporary carve-out: `path`, `reason`, `expires`. |
-| `[rule.downgrade]` | Required `reason` when a rule moves from enforced/ratcheted to advisory/doc-only, or from enforced to ratcheted. |
+| `[rule.downgrade]` | `reason`, and `issue`: the number of an open issue in the registry's `issue_repository` that tracks restoring the rule. Not allowed on an enforced rule. Required when a rule moves from enforced/ratcheted to advisory/doc-only, or from enforced to ratcheted; on a new rule that starts ratcheted with a baseline count above 0; and on a rule that now lists a package from a stronger rule on the base branch. |
 
 `blocking` is derived, never declared: a rule blocks merges when its status is
 `enforced` or `ratcheted` and its tier is not `judgment`.
@@ -124,11 +125,39 @@ fail the guard yet.
   Outside the files that allow file-level exceptions, a justified exception is
   an inline `# noqa: CODE  # reason`.
 - Other enforcers (mypy, Pyright, Import Linter, custom guards) may start
-  `ratcheted` with a baseline file of tolerated violations. Baselines may only
-  shrink: the guard compares counts against the base branch.
+  `ratcheted` with a baseline file of tolerated violations. A new rule that
+  starts ratcheted with a count above 0 needs a `[rule.downgrade]` section,
+  as a downgrade does. Baselines may only shrink: the guard compares counts
+  against the base branch.
 - Moving a rule from enforced/ratcheted to advisory/doc-only, or from enforced
-  to ratcheted, needs an explicit `[rule.downgrade] reason`. The guard accepts
-  any non-empty reason; the reason is for the reviewer.
+  to ratcheted, needs a `[rule.downgrade]` section with a `reason` and an
+  `issue`.
+- A `[rule.downgrade]` section's `issue` is an open issue in
+  `issue_repository`, set at the top of the registry, that tracks restoring
+  the rule. The guard looks up every section's issue on every run, with
+  `gh api` and the token in `GH_TOKEN` or `GITHUB_TOKEN`. On GitHub Actions, a
+  missing token or a failed lookup fails it, and so does an
+  `issue_repository` other than the repository the workflow runs in
+  (`GITHUB_REPOSITORY`), so a pull request can't name an issue elsewhere.
+  Elsewhere, without a token, it prints a note for each issue it didn't
+  check. Closing the issue while the section is still in the registry fails
+  the guard on every pull request, and an enforced rule can't have the
+  section, so the pull request that restores the rule removes it. The guard
+  accepts any non-empty reason and any open issue; whether the issue is about
+  this rule, and whether a section was removed from a rule that stays
+  weakened, is for the reviewer.
+- A package in an enforced or ratcheted rule's `enforcer.projects` on the base
+  branch (only `pyright` enforcers list packages) stays, while its
+  `pyproject.toml` exists, in a rule of the same kind that is at least as
+  strong: enforced for a package from an enforced rule, enforced or
+  ratcheted for one from a ratcheted rule. If it moves to a weaker rule, that
+  rule needs a `[rule.downgrade]` section. Deleting the package, or moving it
+  to a rule at least as strong, passes this check; a new ratcheted rule with a
+  count above 0 still needs the section.
+- The guard compares rules by id. Removing a rule, or replacing it with a
+  weaker rule under a new id, produces only the notes "rule ... was removed"
+  and "rule ... is new", except that the checks above still cover Pyright
+  packages and new ratcheted rules. Review catches the rest.
 - **Weakening a rule is a decision for a person, not an agent.** Agents don't
   downgrade a rule, add a `[rule.downgrade]` section, create a baseline or add
   entries to one, or move a package or path out of a rule's scope. When a rule

docs/engineering/skills/type-checking.md

@@ -111,29 +111,59 @@ After fixing errors:
 2. set the rule's `[rule.baseline] count` in `rules/registry.toml` to the
    number of entries left.
 
-What stops a baseline growing is the rules guard, which compares each existing
-rule's status and baseline count with the base branch. It fails when the count
-doesn't match the file, when the count is higher than on the base branch, and
-when an enforced rule becomes ratcheted without a `[rule.downgrade] reason`.
+What stops a baseline growing is the rules guard, which compares the registry
+with the base branch. It fails when:
+
+- a rule's baseline count doesn't match its file, or is higher than on the
+  base branch;
+- an enforced rule becomes ratcheted without a `[rule.downgrade]` section,
+  which gives a `reason` and an `issue`, the number of the issue that tracks
+  restoring the rule;
+- a rule that the base branch doesn't have starts ratcheted with a baseline
+  count above 0, without a `[rule.downgrade]` section;
+- a package that an enforced or ratcheted Pyright rule lists on the base
+  branch, and whose `pyproject.toml` still exists, is listed by no Pyright
+  rule, or moves to a weaker one (from enforced to any other status, or from
+  ratcheted to advisory), unless the rule that lists it now has a
+  `[rule.downgrade]` section. Deleting the package, or moving it to a rule at
+  least as strong, passes this check; a new ratcheted rule with a count above
+  0 still needs the section, as above;
+- a `[rule.downgrade]` section's `issue` isn't an open issue in this
+  repository. The guard looks up every section's issue on every run, with
+  `gh api` and the token in `GH_TOKEN` or `GITHUB_TOKEN`. In CI, a missing
+  token or a failed lookup fails it, and so does an `issue_repository` at the
+  top of the registry that isn't the repository the workflow runs in. Locally,
+  without a token, it prints a note for each issue it didn't check; to check
+  them, run it with `GH_TOKEN="$(gh auth token)"`.
+
 It doesn't check:
 
-- what the reason says: any non-empty reason passes;
+- what the reason says, or whether the issue is about restoring this rule: any
+  non-empty reason and any open issue pass;
 - which entries changed: a change that fixes one error and adds another keeps
   the count and passes;
-- which packages a rule lists: moving a package from an enforced rule into a
-  new ratcheted rule passes without a reason, because a new rule has no status
-  on the base branch to compare with.
+- whether a `[rule.downgrade]` section is new: a section already on the base
+  branch also covers a further weakening of its rule, such as another package
+  moving into it;
+- whether a rule that stays weakened keeps its `[rule.downgrade]` section:
+  removing the section from a ratcheted or advisory rule passes, and its
+  tracking issue can then be closed without failing anything.
 
 Review is what catches these. `.github/CODEOWNERS` assigns `rules/` to the core
 developers, but `sim.process.rule-machinery-code-owned` is doc-only: merging
 doesn't require their approval. Whoever reviews a pull request that changes a
-baseline, a rule's status or a rule's `enforcer.projects` checks it.
-
-When a person decides to ratchet a package's rule, `--write-baseline` creates
-its baseline: it refuses an error an existing baseline doesn't list, but
-records every current error when the file doesn't exist yet.
-
-Once a baseline is empty, delete it and make the rule `enforced`.
+baseline, a rule's status, a rule's `enforcer.projects` or a
+`[rule.downgrade]` section checks it.
+
+When a person decides to ratchet a package's rule, they add the
+`[rule.downgrade]` section, and `--write-baseline` creates its baseline: it
+refuses an error an existing baseline doesn't list, but records every current
+error when the file doesn't exist yet.
+
+Once a baseline is empty, delete it, make the rule `enforced` and remove its
+`[rule.downgrade]` section, which an enforced rule can't have. That pull
+request can then close the tracking issue. Closing the issue while the
+section is still in the registry fails the guard on every pull request.
 
 ## Fixing errors