diff --git a/.github/scripts/check_pr_description.py b/.github/scripts/check_pr_description.py index 7d16f9682..0de83d6d6 100644 --- a/.github/scripts/check_pr_description.py +++ b/.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__": diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index c68cf38f4..0c27df2f1 100644 --- a/.github/workflows/pr.yml +++ b/.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: diff --git a/docs/engineering/skills/rules-registry.md b/docs/engineering/skills/rules-registry.md index d174eb842..520046671 100644 --- a/docs/engineering/skills/rules-registry.md +++ b/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 diff --git a/docs/engineering/skills/type-checking.md b/docs/engineering/skills/type-checking.md index 7f8b72ad0..1a202c579 100644 --- a/docs/engineering/skills/type-checking.md +++ b/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 diff --git a/projects/policyengine-simulation-entry/tests/test_pr_description_check.py b/projects/policyengine-simulation-entry/tests/test_pr_description_check.py index 659f356c6..cab3d3416 100644 --- a/projects/policyengine-simulation-entry/tests/test_pr_description_check.py +++ b/projects/policyengine-simulation-entry/tests/test_pr_description_check.py @@ -429,6 +429,113 @@ def test_the_check_requires_a_pull_request_event(fakes, tmp_path): assert fakes.calls("gh") == [] +# --- The issue lookup, for other checks ----------------------------------- +# +# The rules guard reuses the lookup to check that each [rule.downgrade] names +# an open issue (scripts/rules/tracking_issues.py). Its errors say only what +# is wrong; each caller adds its own fix, so none of these messages mentions +# the description. + + +@pytest.fixture +def in_process_gh(fakes: FakeCommands, monkeypatch: pytest.MonkeyPatch): + """Put the fake gh first on PATH for lookups made in this process. + + Return a function that sets what the fake gh prints and how it exits. + """ + fake_environment = fakes.env() + monkeypatch.setenv("PATH", fake_environment["PATH"]) + monkeypatch.setenv("FAKE_CALLS_LOG", fake_environment["FAKE_CALLS_LOG"]) + + def answer(**fake_gh_settings: str) -> None: + for name, value in fake_gh_settings.items(): + monkeypatch.setenv(name, value) + + return answer + + +def test_the_lookup_accepts_an_open_issue(check_module, fakes, in_process_gh): + in_process_gh(**_gh_answers(_issue())) + + check_module.require_open_issue(REPOSITORY, 861) + + assert fakes.calls("gh") == [ + ["gh", "api", "repos/PolicyEngine/policyengine-sim-api/issues/861"] + ] + + +@pytest.mark.parametrize( + ("gh_settings", "expected_message", "expected_state"), + [ + (_gh_answers(_issue(state="closed")), "Issue #861 is closed.", "closed"), + ( + _gh_answers( + _issue( + pull_request={ + "url": f"https://api.github.com/repos/{REPOSITORY}/pulls/861" + } + ) + ), + "#861 is a pull request, not an issue.", + None, + ), + ( + _gh_answers( + _issue(115, state="closed", repository="PolicyEngine/policyengine.py") + ), + f"#861 is not an issue in {REPOSITORY}: it moved to " + "PolicyEngine/policyengine.py#115.", + None, + ), + ( + { + "FAKE_GH_STDOUT": NOT_FOUND_STDOUT, + "FAKE_GH_STDERR": NOT_FOUND_STDERR, + "FAKE_GH_EXIT": "1", + }, + f"There is no issue #861 in {REPOSITORY}.", + None, + ), + ], +) +def test_the_lookup_says_what_a_number_names_without_a_fix( + check_module, in_process_gh, gh_settings, expected_message, expected_state +): + in_process_gh(**gh_settings) + + with pytest.raises(check_module.NotAnOpenIssueError) as rejected: + check_module.require_open_issue(REPOSITORY, 861) + + assert str(rejected.value) == expected_message + assert rejected.value.state == expected_state + + +@pytest.mark.parametrize( + ("gh_settings", "expected_message"), + [ + ( + {"FAKE_GH_STDERR": "gh: Bad credentials (HTTP 401)", "FAKE_GH_EXIT": "1"}, + f"Could not look up #861 in {REPOSITORY}: gh: Bad credentials (HTTP 401).", + ), + ({"FAKE_GH_STDOUT": "not JSON"}, "gh api did not print an issue for #861."), + ( + _gh_answers({**_issue(), "repository_url": None}), + "GitHub's answer for #861 names no repository, so it can't be " + f"confirmed as an issue in {REPOSITORY}.", + ), + ], +) +def test_the_lookup_says_why_it_failed_without_a_fix( + check_module, in_process_gh, gh_settings, expected_message +): + in_process_gh(**gh_settings) + + with pytest.raises(check_module.IssueLookupError) as failed: + check_module.require_open_issue(REPOSITORY, 861) + + assert str(failed.value) == expected_message + + # --- The workflow job ----------------------------------------------------- diff --git a/projects/policyengine-simulation-entry/tests/test_quality_guards_workflow.py b/projects/policyengine-simulation-entry/tests/test_quality_guards_workflow.py new file mode 100644 index 000000000..5a0182d85 --- /dev/null +++ b/projects/policyengine-simulation-entry/tests/test_quality_guards_workflow.py @@ -0,0 +1,38 @@ +"""Tests for the token pr.yml's quality-guards job gives the rules guard. + +The rules guard looks up the tracking issue of each [rule.downgrade] with gh, +which reads its token from GH_TOKEN (scripts/rules/tracking_issues.py). On +GitHub Actions the guard fails without a token, so the step that runs it needs +the job's token, and the job needs read access to issues. +""" + +from __future__ import annotations + +import pytest +import yaml +from workflow_script_support import WORKFLOWS_DIR + +PR_WORKFLOW = WORKFLOWS_DIR / "pr.yml" +QUALITY_GUARDS_JOB = "quality-guards" +GUARD_COMMAND = "python scripts/run_quality_guards.py" + + +@pytest.fixture(scope="module") +def quality_guards_job() -> dict: + workflow = yaml.safe_load(PR_WORKFLOW.read_text(encoding="utf-8")) + return workflow["jobs"][QUALITY_GUARDS_JOB] + + +def test_the_job_token_can_only_read_contents_and_issues(quality_guards_job): + assert quality_guards_job["permissions"] == {"contents": "read", "issues": "read"} + + +def test_only_the_step_that_runs_the_guards_gets_the_token(quality_guards_job): + steps_with_a_token = [ + step + for step in quality_guards_job["steps"] + if "GH_TOKEN" in step.get("env", {}) + ] + + assert [step["run"] for step in steps_with_a_token] == [GUARD_COMMAND] + assert steps_with_a_token[0]["env"] == {"GH_TOKEN": "${{ github.token }}"} diff --git a/rules/registry.toml b/rules/registry.toml index be8a3155d..67d7ff178 100644 --- a/rules/registry.toml +++ b/rules/registry.toml @@ -10,6 +10,8 @@ schema_version = 1 repo = "policyengine-sim-api" +# Each [rule.downgrade] names an open issue here that tracks restoring the rule. +issue_repository = "PolicyEngine/policyengine-sim-api" [[category]] id = "boundaries" diff --git a/scripts/rules/baseline.py b/scripts/rules/baseline.py index b10b64881..1c869a506 100644 --- a/scripts/rules/baseline.py +++ b/scripts/rules/baseline.py @@ -7,12 +7,42 @@ from typing import TYPE_CHECKING from scripts.rules.load import REGISTRY_PATH, load_text -from scripts.rules.model import Registry, Status, Violation +from scripts.rules.model import ( + Kind, + Registry, + Rule, + Status, + Violation, + normalized_package, +) if TYPE_CHECKING: from pathlib import Path +# How a person, never an agent, records a weaker rule; each fix ends with it. +PERSON_DOWNGRADE = ( + 'only a person may weaken a rule, by adding [rule.downgrade] with reason = "..." ' + "and issue = " +) + + +# The statuses a package or a rule may keep, or move to, without a downgrade. +_AT_LEAST_AS_STRONG = { + Status.ENFORCED: (Status.ENFORCED,), + Status.RATCHETED: (Status.ENFORCED, Status.RATCHETED), +} + + +def _weakens(old: Status, new: Status) -> bool: + """Whether going from ``old`` to ``new`` weakens a rule that blocked merges. + + A ratcheted rule tolerates its baseline, so ratcheting an enforced rule + weakens it too, with whatever count its new baseline starts at. + """ + return old in _AT_LEAST_AS_STRONG and new not in _AT_LEAST_AS_STRONG[old] + + class BaseUnavailableError(Exception): """The base ref could not be read.""" @@ -71,15 +101,11 @@ def check_baselines( "fix the new violations instead of adding them to the baseline", ) ) - weaker = (Status.ADVISORY, Status.DOC_ONLY) - # A ratcheted rule tolerates its baseline, so ratcheting an enforced - # rule weakens it too, with whatever count its new baseline starts at. - if ( - old.status in (Status.ENFORCED, Status.RATCHETED) and new.status in weaker - ) or (old.status is Status.ENFORCED and new.status is Status.RATCHETED): - if new.downgrade_reason: + if _weakens(old.status, new.status): + if new.downgrade: notes.append( - f"rule {new.id} downgraded to {new.status}: {new.downgrade_reason}" + f"rule {new.id} downgraded to {new.status}: " + f"{new.downgrade.reason} (issue #{new.downgrade.issue})" ) else: violations.append( @@ -87,12 +113,114 @@ def check_baselines( new.id, "baseline", f"status weakened from {old.status} to {new.status}", - 'add [rule.downgrade] reason = "..." so the downgrade is ' - "explicit and reviewable", + f'restore status = "{old.status}"; {PERSON_DOWNGRADE}', ) ) - notes.extend( - f"rule {rule_id} is new" - for rule_id in sorted(current_ids - {rule.id for rule in base.rules}) - ) + for rule_id in sorted(current_ids - {rule.id for rule in base.rules}): + notes.append(f"rule {rule_id} is new") + new_violations, new_notes = _check_new_rule(current.rule(rule_id)) + violations += new_violations + notes += new_notes + return violations, notes + + +def _check_new_rule(rule: Rule | None) -> tuple[list[Violation], list[str]]: + """Check a rule the base branch doesn't have; return (violations, notes). + + A new rule that starts ratcheted with tolerated violations is weaker than + an enforced one from the start, so it needs ``[rule.downgrade]`` too. + """ + if ( + rule is None + or rule.status is not Status.RATCHETED + or rule.baseline is None + or rule.baseline.count <= 0 + ): + return [], [] + starts = f"starts ratcheted with {rule.baseline.count} tolerated violations" + if rule.downgrade: + return [], [ + f"rule {rule.id} {starts}: {rule.downgrade.reason} " + f"(issue #{rule.downgrade.issue})" + ] + return [ + Violation( + rule.id, + "baseline", + f"new rule {starts}", + f"fix them and make the rule enforced; {PERSON_DOWNGRADE}", + ) + ], [] + + +def _pyright_listings(registry: Registry) -> dict[str, Rule]: + """Map each package a Pyright rule lists to that rule. + + A package listed twice maps to the first rule, the one the Pyright check + holds it to. + """ + listings: dict[str, Rule] = {} + for rule in registry.rules: + if rule.enforcer.kind is not Kind.PYRIGHT: + continue + for project in rule.enforcer.get("projects", []): + listings.setdefault(normalized_package(str(project)), rule) + return listings + + +def check_pyright_packages( + current: Registry, base: Registry, root: Path +) -> tuple[list[Violation], list[str]]: + """Check that each package a blocking Pyright rule lists on the base still is. + + Return (violations, notes). A package from an enforced rule stays in an + enforced Pyright rule, and one from a ratcheted rule in an enforced or + ratcheted one, unless its ``pyproject.toml`` was deleted. Moving it to a + weaker rule needs ``[rule.downgrade]`` on that rule. A rule that keeps the + package under the same id is left to ``check_baselines``, which compares + its status, and so is a ratcheted rule's baseline count. + """ + violations: list[Violation] = [] + notes: list[str] = [] + head_listings = _pyright_listings(current) + for package, base_rule in _pyright_listings(base).items(): + allowed = _AT_LEAST_AS_STRONG.get(base_rule.status) + if allowed is None or not (root / package / "pyproject.toml").is_file(): + continue + relist = ( + f"list {package} in an " + f"{' or '.join(str(status) for status in allowed)} Pyright rule again" + ) + head_rule = head_listings.get(package) + if head_rule is None: + violations.append( + Violation( + base_rule.id, + "baseline", + f"package {package} left {base_rule.status} rule {base_rule.id}, " + "and no Pyright rule lists it now", + f"{relist}, or delete the package", + ) + ) + continue + if head_rule.status in allowed or head_rule.id == base_rule.id: + continue + move = ( + f"package {package} moved from {base_rule.status} rule {base_rule.id} " + f"to {head_rule.status} rule {head_rule.id}" + ) + if head_rule.downgrade: + notes.append( + f"{move}: {head_rule.downgrade.reason} " + f"(issue #{head_rule.downgrade.issue})" + ) + else: + violations.append( + Violation( + head_rule.id, + "baseline", + move, + f"{relist}; {PERSON_DOWNGRADE}", + ) + ) return violations, notes diff --git a/scripts/rules/guard.py b/scripts/rules/guard.py index 99ba5e357..ee42a01db 100644 --- a/scripts/rules/guard.py +++ b/scripts/rules/guard.py @@ -11,6 +11,7 @@ from scripts.rules.baseline import ( BaseUnavailableError, check_baselines, + check_pyright_packages, default_base_ref, load_base_registry, ) @@ -30,6 +31,7 @@ resolve_enforcer, resolve_proof_ci, ) +from scripts.rules.tracking_issues import check_tracking_issues from scripts.rules.validate import validate REPO_ROOT = Path(__file__).resolve().parents[2] @@ -77,10 +79,9 @@ def run_guard( today: date | None = None, compare_base: bool = True, ) -> GuardResult: - """Run every registry check, then compare baselines with the base ref. + """Run every registry check, then compare the registry with the base ref. - Stop after the schema errors if the registry does not load. An unreadable - base ref is a violation on GitHub Actions and only a note elsewhere. + Stop after the schema errors if the registry does not load. """ clear_caches() result = GuardResult() @@ -98,44 +99,64 @@ def run_guard( if registry.agents is not None: result.violations += check_adapters(registry.agents, root) result.violations += _stale_outputs(registry, root) + violations, notes = check_tracking_issues(registry) + result.violations += violations + result.notes += notes - ref = base_ref or default_base_ref() - if not compare_base: - return result + if compare_base: + _compare_with_base(registry, root, base_ref or default_base_ref(), result) + return result + + +def _compare_with_base( + registry: Registry, root: Path, ref: str | None, result: GuardResult +) -> None: + """Add what comparing ``registry`` with the one on ``ref`` finds to ``result``. + + An unreadable base ref is a violation on GitHub Actions and only a note + elsewhere. + """ if ref is None: result.notes.append( "no base ref (set --base-ref or GITHUB_BASE_REF); " "baseline comparison skipped" ) - else: - try: - base = load_base_registry(root, ref) - except BaseUnavailableError: - if os.environ.get("GITHUB_ACTIONS"): - result.violations.append( - Violation( - None, - "baseline", - f"base ref {ref} is not available", - "check out with fetch-depth: 0", - ) - ) - else: - result.notes.append( - f"base ref {ref} is not available; baseline comparison skipped" + return + try: + base = load_base_registry(root, ref) + except BaseUnavailableError: + if os.environ.get("GITHUB_ACTIONS"): + result.violations.append( + Violation( + None, + "baseline", + f"base ref {ref} is not available", + "check out with fetch-depth: 0", ) + ) else: - if base is None: - result.notes.append( - f"{ref} has no registry yet; baseline comparison skipped" - ) - else: - violations, notes = check_baselines(registry, base) - result.violations += violations - result.notes += notes - return result - - -def check() -> list[str]: - """Adapter for scripts/run_quality_guards.py.""" - return [str(v) for v in run_guard(REPO_ROOT).violations] + result.notes.append( + f"base ref {ref} is not available; baseline comparison skipped" + ) + return + if base is None: + result.notes.append(f"{ref} has no registry yet; baseline comparison skipped") + return + for violations, notes in ( + check_baselines(registry, base), + check_pyright_packages(registry, base, root), + ): + result.violations += violations + result.notes += notes + + +def check(root: Path = REPO_ROOT) -> list[str]: + """Adapter for scripts/run_quality_guards.py: print the notes, return failures. + + The notes say what the guard didn't check, such as a tracking issue it + had no token to look up. + """ + result = run_guard(root) + for note in result.notes: + print(f"note: {note}") + return [str(violation) for violation in result.violations] diff --git a/scripts/rules/load.py b/scripts/rules/load.py index 83a5da5ad..69406c0b3 100644 --- a/scripts/rules/load.py +++ b/scripts/rules/load.py @@ -6,6 +6,7 @@ from __future__ import annotations +import re import tomllib from datetime import date from enum import Enum @@ -19,6 +20,7 @@ CiRef, CiScripts, Discovery, + Downgrade, Enforcer, Ignored, Kind, @@ -43,7 +45,10 @@ "rule", "agents", "ci_scripts", + "issue_repository", } +# A GitHub repository's owner/name. +_REPOSITORY = re.compile(r"[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+") _AGENTS_KEYS = {"instructions", "adapters", "max_adapter_lines"} _CI_SCRIPTS_KEYS = { "workflows", @@ -70,6 +75,10 @@ "exception", "downgrade", } +_DOWNGRADE_KEYS = {"reason", "issue"} +DOWNGRADE_ISSUE_FIX = ( + "write issue = " +) _DISCOVERY_KEYS = { "workflows", "guard_test_globs", @@ -327,7 +336,7 @@ def _exceptions( else None ) issue = entry.get("issue") - if issue is not None and (not isinstance(issue, int) or issue <= 0): + if issue is not None and _issue_number(issue) is None: c.add( rule_id, f"{label}.exception.issue must be a positive issue number", @@ -348,22 +357,49 @@ def _exceptions( return tuple(exceptions) -def _downgrade_reason( +def _issue_number(value: object) -> int | None: + """Return ``value`` if it is a positive issue number, otherwise None. + + TOML's ``true`` reads as a Python ``bool``, which is an ``int``; it is no + issue number. + """ + if isinstance(value, int) and not isinstance(value, bool) and value > 0: + return value + return None + + +def _downgrade( c: _Collector, rule_id: str | None, raw: dict[str, Any], label: str -) -> str | None: - """Read the reason in ``[rule.downgrade]``, which is optional.""" +) -> Downgrade | None: + """Read ``[rule.downgrade]``, which is optional. + + It needs a reason and the number of the issue that tracks restoring the + rule. A section without both is reported and read as absent, so it never + allows a weaker rule. + """ if "downgrade" not in raw: return None - draw = raw["downgrade"] - if isinstance(draw, dict) and isinstance(draw.get("reason"), str): - reason: str = draw["reason"] - return reason - c.add( - rule_id, - f"{label}.downgrade needs a 'reason'", - 'write [rule.downgrade] reason = "..."', - ) - return None + downgrade_raw = raw["downgrade"] + where = f"{label}.downgrade" + if not isinstance(downgrade_raw, dict): + c.add( + rule_id, + f"{where} must be a table", + 'write [rule.downgrade] with reason = "..." and issue = ', + ) + return None + c.unknown_keys(rule_id, downgrade_raw, _DOWNGRADE_KEYS, where) + reason = c.string(rule_id, downgrade_raw, "reason", where) + issue = _issue_number(downgrade_raw.get("issue")) + if issue is None: + c.add( + rule_id, + f"{where}.issue must be a positive issue number", + DOWNGRADE_ISSUE_FIX, + ) + if not reason or issue is None: + return None + return Downgrade(reason, issue) def _autofix( @@ -390,7 +426,7 @@ def _rule(c: _Collector, raw: object, index: int) -> Rule | None: proof = _proof(c, rule_id, raw, label) baseline = _baseline(c, rule_id, raw, label) exceptions = _exceptions(c, rule_id, raw, label) - downgrade_reason = _downgrade_reason(c, rule_id, raw, label) + downgrade = _downgrade(c, rule_id, raw, label) autofix = _autofix(c, rule_id, raw, label) review_by = None if "review_by" in raw: @@ -416,8 +452,24 @@ def _rule(c: _Collector, raw: object, index: int) -> Rule | None: proof=proof, baseline=baseline, exceptions=exceptions, - downgrade_reason=downgrade_reason, + downgrade=downgrade, + ) + + +def _issue_repository(c: _Collector, raw: dict[str, Any]) -> str: + """Read ``issue_repository``, which is optional but must be owner/name.""" + value = raw.get("issue_repository") + if value is None: + return "" + if isinstance(value, str) and _REPOSITORY.fullmatch(value): + return value + c.add( + None, + "registry.issue_repository must be /, the GitHub repository " + "that tracks issues", + 'set issue_repository = "/"', ) + return "" def parse(raw: dict[str, Any]) -> tuple[Registry | None, list[Violation]]: @@ -453,6 +505,7 @@ def parse(raw: dict[str, Any]) -> tuple[Registry | None, list[Violation]]: rules=tuple(rule for rule in rules if rule is not None), agents=_agents(c, raw.get("agents")), ci_scripts=_ci_scripts(c, raw.get("ci_scripts")), + issue_repository=_issue_repository(c, raw), ) return registry, c.violations diff --git a/scripts/rules/model.py b/scripts/rules/model.py index 736e72a39..58e08c6b6 100644 --- a/scripts/rules/model.py +++ b/scripts/rules/model.py @@ -229,6 +229,18 @@ class RuleException: issue: int | None = None +@dataclass(frozen=True) +class Downgrade: + """A ``[rule.downgrade]``: why a person weakened the rule, and its tracking issue. + + ``issue`` is the number of the open issue, in ``Registry.issue_repository``, + that tracks restoring the rule. + """ + + reason: str + issue: int + + @dataclass(frozen=True) class Rule: """One ``[[rule]]`` registry entry; rules-registry.md explains each field.""" @@ -247,7 +259,7 @@ class Rule: proof: Proof | None = None baseline: Baseline | None = None exceptions: tuple[RuleException, ...] = () - downgrade_reason: str | None = None + downgrade: Downgrade | None = None @property def blocking(self) -> bool: @@ -269,6 +281,8 @@ class Registry: rules: tuple[Rule, ...] agents: Agents | None = None ci_scripts: CiScripts | None = None + # The GitHub repository, as owner/name, whose issues downgrades name. + issue_repository: str = "" def rule(self, rule_id: str) -> Rule | None: """Return the rule with ``rule_id``, or None if the registry has none.""" diff --git a/scripts/rules/tests/test_guard.py b/scripts/rules/tests/test_guard.py index 3f28b54d2..e83883669 100644 --- a/scripts/rules/tests/test_guard.py +++ b/scripts/rules/tests/test_guard.py @@ -5,13 +5,16 @@ import subprocess import unittest from datetime import date -from typing import ClassVar +from typing import TYPE_CHECKING, ClassVar -from scripts.rules.baseline import check_baselines +from scripts.rules.baseline import check_baselines, check_pyright_packages from scripts.rules.load import load_text from scripts.rules.render import render_agents_block, render_rules_md from scripts.rules.tests.support import REGISTRY, Repo +if TYPE_CHECKING: + from scripts.rules.model import Registry + RATCHETED_RULE = """ [[rule]] id = "ex.layers.ratcheted" @@ -575,6 +578,248 @@ def test_package_may_not_have_a_pyrightconfig_json(self) -> None: ) +def pyright_rule( + rule_id: str, packages: list[str], *, status: str = "enforced", tail: str = "" +) -> str: + """Return a Pyright rule over ``packages``; ``tail`` adds tables to its end.""" + listed = ", ".join(f'"{package}"' for package in packages) + baseline = ( + f'[rule.baseline]\nfile = "rules/baselines/{rule_id}.txt"\ncount = 0\n' + if status == "ratcheted" + else "" + ) + return ( + PYRIGHT_RULE.replace('id = "ex.quality.pyright"', f'id = "{rule_id}"') + .replace('status = "enforced"', f'status = "{status}"') + .replace('projects = ["libs/alpha", "projects/beta"]', f"projects = [{listed}]") + + baseline + + tail + ) + + +DOWNGRADE_SECTION = '[rule.downgrade]\nreason = "a Pyright upgrade"\nissue = 912\n' +PERSON_DOWNGRADE_FIX = ( + 'only a person may weaken a rule, by adding [rule.downgrade] with reason = "..." ' + "and issue = ." +) + + +class PyrightPackageTest(unittest.TestCase): + """A package a blocking Pyright rule lists keeps a check at least as strong.""" + + BASE_RULES = pyright_rule("ex.quality.pyright", ["libs/alpha", "projects/beta"]) + + def setUp(self) -> None: + repo = Repo(pyright_files(), render=False) + self.addCleanup(repo.__exit__) + self.repo = repo + + def compare(self, head_rules: str) -> tuple[list[str], list[str]]: + """Compare ``head_rules`` with BASE_RULES; return the violations and notes.""" + base, _ = load_text(REGISTRY + self.BASE_RULES) + head, _ = load_text(REGISTRY + head_rules) + assert base is not None and head is not None + violations, notes = check_pyright_packages(head, base, self.repo.root) + return [str(violation) for violation in violations], notes + + def test_a_package_may_move_to_another_enforced_rule(self) -> None: + head_rules = pyright_rule("ex.quality.pyright", ["libs/alpha"]) + pyright_rule( + "ex.quality.pyright-beta", ["projects/beta"] + ) + + self.assertEqual(self.compare(head_rules), ([], [])) + + def test_a_package_moved_to_a_rule_that_is_not_enforced_is_a_downgrade( + self, + ) -> None: + for status in ("ratcheted", "advisory"): + with self.subTest(status=status): + head_rules = pyright_rule( + "ex.quality.pyright", ["libs/alpha"] + ) + pyright_rule( + "ex.quality.pyright-beta", ["projects/beta"], status=status + ) + + violations, _ = self.compare(head_rules) + + self.assertEqual( + violations, + [ + "[ex.quality.pyright-beta] baseline: package projects/beta " + "moved from enforced rule ex.quality.pyright to " + f"{status} rule ex.quality.pyright-beta. Fix: list " + "projects/beta in an enforced Pyright rule again; " + f"{PERSON_DOWNGRADE_FIX}" + ], + ) + + def test_a_downgrade_on_the_rule_that_lists_the_package_now_allows_it( + self, + ) -> None: + head_rules = pyright_rule("ex.quality.pyright", ["libs/alpha"]) + pyright_rule( + "ex.quality.pyright-beta", + ["projects/beta"], + status="ratcheted", + tail=DOWNGRADE_SECTION, + ) + + self.assertEqual( + self.compare(head_rules), + ( + [], + [ + "package projects/beta moved from enforced rule " + "ex.quality.pyright to ratcheted rule ex.quality.pyright-beta: " + "a Pyright upgrade (issue #912)" + ], + ), + ) + + def test_a_downgrade_on_the_rule_the_package_left_does_not_allow_it( + self, + ) -> None: + head_rules = pyright_rule( + "ex.quality.pyright", ["libs/alpha"], tail=DOWNGRADE_SECTION + ) + pyright_rule( + "ex.quality.pyright-beta", ["projects/beta"], status="advisory" + ) + + violations, _ = self.compare(head_rules) + + self.assertEqual(len(violations), 1) + self.assertIn("package projects/beta moved from enforced rule", violations[0]) + + def test_a_package_that_no_rule_lists_must_be_deleted(self) -> None: + violations, _ = self.compare(pyright_rule("ex.quality.pyright", ["libs/alpha"])) + + self.assertEqual( + violations, + [ + "[ex.quality.pyright] baseline: package projects/beta left enforced " + "rule ex.quality.pyright, and no Pyright rule lists it now. Fix: " + "list projects/beta in an enforced Pyright rule again, or delete " + "the package." + ], + ) + + def test_a_deleted_package_may_leave(self) -> None: + (self.repo.root / "projects/beta/pyproject.toml").unlink() + + self.assertEqual( + self.compare(pyright_rule("ex.quality.pyright", ["libs/alpha"])), ([], []) + ) + + def test_a_package_written_differently_is_still_listed(self) -> None: + head_rules = pyright_rule("ex.quality.pyright", ["./libs/alpha"]) + ( + pyright_rule("ex.quality.pyright-beta", ["projects/beta/"]) + ) + + self.assertEqual(self.compare(head_rules), ([], [])) + + def test_a_rule_that_keeps_its_packages_is_left_to_the_status_check( + self, + ) -> None: + head_rules = pyright_rule( + "ex.quality.pyright", ["libs/alpha", "projects/beta"], status="ratcheted" + ) + + self.assertEqual(self.compare(head_rules), ([], [])) + + def test_a_package_moved_from_a_ratcheted_to_an_advisory_rule_is_a_downgrade( + self, + ) -> None: + self.BASE_RULES = pyright_rule( + "ex.quality.pyright", ["libs/alpha", "projects/beta"], status="ratcheted" + ) + head_rules = pyright_rule( + "ex.quality.pyright", ["libs/alpha"], status="ratcheted" + ) + pyright_rule( + "ex.quality.pyright-beta", ["projects/beta"], status="advisory" + ) + + self.assertEqual( + self.compare(head_rules), + ( + [ + "[ex.quality.pyright-beta] baseline: package projects/beta " + "moved from ratcheted rule ex.quality.pyright to advisory rule " + "ex.quality.pyright-beta. Fix: list projects/beta in an " + "enforced or ratcheted Pyright rule again; " + f"{PERSON_DOWNGRADE_FIX}" + ], + [], + ), + ) + + def test_a_package_may_move_between_ratcheted_rules(self) -> None: + # check_baselines still requires a downgrade for a new ratcheted rule + # with a count above 0; this comparison only checks the move. + self.BASE_RULES = pyright_rule( + "ex.quality.pyright", ["libs/alpha", "projects/beta"], status="ratcheted" + ) + head_rules = pyright_rule( + "ex.quality.pyright", ["libs/alpha"], status="ratcheted" + ) + pyright_rule( + "ex.quality.pyright-beta", ["projects/beta"], status="ratcheted" + ) + + self.assertEqual(self.compare(head_rules), ([], [])) + + def test_a_package_that_left_a_ratcheted_rule_must_be_listed(self) -> None: + self.BASE_RULES = pyright_rule( + "ex.quality.pyright", ["libs/alpha", "projects/beta"], status="ratcheted" + ) + + violations, _ = self.compare( + pyright_rule("ex.quality.pyright", ["libs/alpha"], status="ratcheted") + ) + + self.assertEqual( + violations, + [ + "[ex.quality.pyright] baseline: package projects/beta left ratcheted " + "rule ex.quality.pyright, and no Pyright rule lists it now. Fix: " + "list projects/beta in an enforced or ratcheted Pyright rule again, " + "or delete the package." + ], + ) + + def test_packages_of_advisory_rules_on_the_base_are_not_compared(self) -> None: + self.BASE_RULES = pyright_rule( + "ex.quality.pyright", ["libs/alpha", "projects/beta"], status="advisory" + ) + + self.assertEqual( + self.compare( + pyright_rule("ex.quality.pyright", ["libs/alpha"], status="advisory") + ), + ([], []), + ) + + def test_the_guard_compares_packages_with_the_git_base_ref(self) -> None: + repo = self.repo + repo.render() + git = ["git", "-c", "user.email=t@example.com", "-c", "user.name=t"] + subprocess.run(["git", "init", "-q"], cwd=repo.root, check=True) + subprocess.run([*git, "add", "-A"], cwd=repo.root, check=True) + subprocess.run([*git, "commit", "-qm", "base"], cwd=repo.root, check=True) + repo.write( + "rules/registry.toml", + REGISTRY + + pyright_rule("ex.quality.pyright", ["libs/alpha"]) + + pyright_rule( + "ex.quality.pyright-beta", ["projects/beta"], status="advisory" + ), + ) + repo.render() + + self.assertIn( + "[ex.quality.pyright-beta] baseline: package projects/beta moved from " + "enforced rule ex.quality.pyright to advisory rule ex.quality.pyright-beta", + "\n".join(repo.messages(base_ref="HEAD")), + ) + + class HiddenRuleTest(unittest.TestCase): def assert_violation(self, repo: Repo, fragment: str) -> None: messages = repo.messages() @@ -800,7 +1045,7 @@ def test_baseline_may_not_grow(self) -> None: violations = check_baselines(current, base)[0] self.assertIn("baseline grew from 2 to 3", str(violations[0])) - def test_downgrade_needs_reason(self) -> None: + def test_downgrade_needs_reason_and_issue(self) -> None: base = self.registry("") weakened = REGISTRY.replace( 'status = "enforced"\nautofix = true\n[rule.enforcer]\nkind = "ruff"\n' @@ -815,15 +1060,26 @@ def test_downgrade_needs_reason(self) -> None: violations = check_baselines(current, base)[0] self.assertIn("status weakened from enforced to advisory", str(violations[0])) - with_reason, _ = load_text( - weakened.replace( - 'mode = "format"', - 'mode = "format"\n[rule.downgrade]\nreason = "flaky"', - 1, + def with_downgrade(section: str) -> Registry: + registry, _ = load_text( + weakened.replace('mode = "format"', f'mode = "format"\n{section}', 1) ) + assert registry is not None + return registry + + reason_only = with_downgrade('[rule.downgrade]\nreason = "flaky"') + self.assertIn( + "status weakened from enforced to advisory", + str(check_baselines(reason_only, base)[0][0]), + ) + reason_and_issue = with_downgrade( + '[rule.downgrade]\nreason = "flaky"\nissue = 912' + ) + violations, notes = check_baselines(reason_and_issue, base) + self.assertEqual(violations, []) + self.assertIn( + "rule ex.quality.format downgraded to advisory: flaky (issue #912)", notes ) - assert with_reason is not None - self.assertEqual(check_baselines(with_reason, base)[0], []) def test_ratcheting_an_enforced_rule_needs_reason(self) -> None: ratcheted = RATCHETED_RULE.format(count=50) @@ -838,18 +1094,68 @@ def test_ratcheting_an_enforced_rule_needs_reason(self) -> None: [str(violation) for violation in violations], [ "[ex.layers.ratcheted] baseline: status weakened from enforced to " - 'ratcheted. Fix: add [rule.downgrade] reason = "..." so the ' - "downgrade is explicit and reviewable." + 'ratcheted. Fix: restore status = "enforced"; only a person may ' + 'weaken a rule, by adding [rule.downgrade] with reason = "..." and ' + "issue = ." ], ) with_reason = self.registry( ratcheted.replace( "[rule.baseline]", - '[rule.downgrade]\nreason = "a Pyright upgrade"\n[rule.baseline]', + '[rule.downgrade]\nreason = "a Pyright upgrade"\nissue = 912\n' + "[rule.baseline]", ) ) self.assertEqual(check_baselines(with_reason, base)[0], []) + def test_a_new_rule_starts_ratcheted_only_with_a_downgrade(self) -> None: + base = self.registry("") + current = self.registry(RATCHETED_RULE.format(count=2)) + + violations, notes = check_baselines(current, base) + + self.assertEqual( + [str(violation) for violation in violations], + [ + "[ex.layers.ratcheted] baseline: new rule starts ratcheted with 2 " + "tolerated violations. Fix: fix them and make the rule enforced; " + "only a person may weaken a rule, by adding [rule.downgrade] with " + 'reason = "..." and issue = ." + ], + ) + self.assertEqual(notes, ["rule ex.layers.ratcheted is new"]) + + def test_a_downgrade_lets_a_new_rule_start_ratcheted(self) -> None: + base = self.registry("") + current = self.registry( + RATCHETED_RULE.format(count=2).replace( + "[rule.baseline]", + '[rule.downgrade]\nreason = "a new checker"\nissue = 912\n' + "[rule.baseline]", + ) + ) + + violations, notes = check_baselines(current, base) + + self.assertEqual(violations, []) + self.assertEqual( + notes, + [ + "rule ex.layers.ratcheted is new", + "rule ex.layers.ratcheted starts ratcheted with 2 tolerated " + "violations: a new checker (issue #912)", + ], + ) + + def test_a_new_rule_may_start_ratcheted_with_nothing_tolerated(self) -> None: + base = self.registry("") + current = self.registry(RATCHETED_RULE.format(count=0)) + + self.assertEqual( + check_baselines(current, base), ([], ["rule ex.layers.ratcheted is new"]) + ) + def test_compares_against_git_base_ref(self) -> None: with Repo(with_rule(RATCHETED_RULE.format(count=1)), render=False) as repo: repo.write("rules/baselines/ratcheted.txt", "pkg/routes.py\n") diff --git a/scripts/rules/tests/test_load.py b/scripts/rules/tests/test_load.py index 7224fca20..931ecc2da 100644 --- a/scripts/rules/tests/test_load.py +++ b/scripts/rules/tests/test_load.py @@ -10,6 +10,7 @@ from scripts.rules.model import ( Baseline, CiRef, + Downgrade, Enforcer, Kind, Proof, @@ -31,6 +32,9 @@ "'workflow-job', 'docs-guard', 'llm-review', 'none']" ) MALFORMED = "[ex.layers.malformed] schema: ex.layers.malformed" +DOWNGRADE_ISSUE_FIX = ( + "write issue = " +) MALFORMED_RULE = { "id": "ex.layers.malformed", @@ -87,7 +91,7 @@ "proof": {"test": BOUNDARY_NODE}, "baseline": {"file": "rules/baselines/x.txt", "count": 2, "extra": True}, "exception": {"path": "pkg/*.py", "reason": "legacy", "expires": "2026-12-31"}, - "downgrade": {"reason": "too noisy"}, + "downgrade": {"reason": "too noisy", "issue": 912}, } @@ -127,8 +131,13 @@ def test_every_problem_is_reported_in_order(self) -> None: "'removal'].", f"{MALFORMED}.exception.expires must be a date. " "Fix: write it as YYYY-MM-DD.", - f"{MALFORMED}.downgrade needs a 'reason'. " - 'Fix: write [rule.downgrade] reason = "...".', + "[ex.layers.malformed] schema: unknown key 'why' in " + "ex.layers.malformed.downgrade. Fix: remove it or use one of " + "['issue', 'reason'].", + f"{MALFORMED}.downgrade.reason must be a non-empty string. " + "Fix: set reason.", + f"{MALFORMED}.downgrade.issue must be a positive issue number. " + f"Fix: {DOWNGRADE_ISSUE_FIX}.", f"{MALFORMED}.autofix must be true or false. Fix: set autofix.", f"{MALFORMED}.review_by must be a date. Fix: write it as YYYY-MM-DD.", f"[registry] schema: rule #3.enforcer.kind is 'flake8'. " @@ -186,10 +195,111 @@ def test_single_tables_stand_for_one_entry_lists(self) -> None: proof=Proof({"test": BOUNDARY_NODE}), baseline=Baseline("rules/baselines/x.txt", 2), exceptions=(RuleException("pkg/*.py", "legacy", date(2026, 12, 31)),), - downgrade_reason="too noisy", + downgrade=Downgrade("too noisy", 912), ), ) +class ExceptionIssueTest(unittest.TestCase): + def test_an_exception_issue_must_be_a_positive_whole_number(self) -> None: + for issue in (0, -5, "5", True, 5.0): + with self.subTest(issue=issue): + raw = tomllib.loads(REGISTRY) + raw["rule"] = [ + { + **SINGLE_TABLES_RULE, + "exception": { + "path": "pkg/*.py", + "reason": "legacy", + "issue": issue, + }, + } + ] + + registry, violations = parse(raw) + + self.assertIn( + "[ex.layers.single-tables] schema: ex.layers.single-tables" + ".exception.issue must be a positive issue number. " + "Fix: write issue = .", + [str(violation) for violation in violations], + ) + assert registry is not None + rule = registry.rule("ex.layers.single-tables") + assert rule is not None + self.assertIsNone(rule.exceptions[0].issue) + + +class DowngradeTest(unittest.TestCase): + """A [rule.downgrade] says why the rule is weaker and which issue restores it.""" + + def downgrade_problems(self, downgrade: object) -> tuple[Rule | None, list[str]]: + """Parse SINGLE_TABLES_RULE with ``downgrade``; return it and the problems.""" + raw = tomllib.loads(REGISTRY) + raw["rule"] = [{**SINGLE_TABLES_RULE, "downgrade": downgrade}] + registry, violations = parse(raw) + assert registry is not None + problems = [ + str(violation) + for violation in violations + if "downgrade" in violation.message + ] + return registry.rule("ex.layers.single-tables"), problems + + def test_a_reason_and_an_issue_number_make_a_downgrade(self) -> None: + rule, problems = self.downgrade_problems({"reason": "noisy", "issue": 1}) + + self.assertEqual(problems, []) + assert rule is not None + self.assertEqual(rule.downgrade, Downgrade("noisy", 1)) + + def test_the_issue_must_be_a_positive_whole_number(self) -> None: + for issue in (None, 0, -912, "912", "#912", 912.0, True): + with self.subTest(issue=issue): + downgrade = {"reason": "noisy"} + if issue is not None: + downgrade["issue"] = issue + + rule, problems = self.downgrade_problems(downgrade) + + self.assertEqual( + problems, + [ + "[ex.layers.single-tables] schema: ex.layers.single-tables" + ".downgrade.issue must be a positive issue number. " + f"Fix: {DOWNGRADE_ISSUE_FIX}." + ], + ) + assert rule is not None + self.assertIsNone(rule.downgrade) + + def test_the_reason_must_not_be_empty(self) -> None: + rule, problems = self.downgrade_problems({"reason": " ", "issue": 912}) + + self.assertEqual( + problems, + [ + "[ex.layers.single-tables] schema: ex.layers.single-tables" + ".downgrade.reason must be a non-empty string. Fix: set reason." + ], + ) + assert rule is not None + self.assertIsNone(rule.downgrade) + + def test_a_downgrade_must_be_a_table(self) -> None: + rule, problems = self.downgrade_problems("too noisy") + + self.assertEqual( + problems, + [ + "[ex.layers.single-tables] schema: ex.layers.single-tables" + ".downgrade must be a table. Fix: write [rule.downgrade] with " + 'reason = "..." and issue = .' + ], + ) + assert rule is not None + self.assertIsNone(rule.downgrade) + + if __name__ == "__main__": unittest.main() diff --git a/scripts/rules/tests/test_tracking_issues.py b/scripts/rules/tests/test_tracking_issues.py new file mode 100644 index 000000000..b60dd8add --- /dev/null +++ b/scripts/rules/tests/test_tracking_issues.py @@ -0,0 +1,485 @@ +"""Tests for the check that each [rule.downgrade] names an open tracking issue. + +A fake ``gh`` first on PATH answers the lookup, so no test reaches GitHub. The +fake prints ``.json`` from its answers directory and succeeds, or +prints ``.stderr`` and fails. +""" + +from __future__ import annotations + +import contextlib +import io +import json +import os +import tempfile +import unittest +from pathlib import Path +from unittest import mock + +from scripts.rules import guard, tracking_issues +from scripts.rules.load import load_text +from scripts.rules.tests.support import REGISTRY, Repo + +ISSUE_REPOSITORY = "example/repo" +REGISTRY_WITH_ISSUES = REGISTRY.replace( + 'repo = "example"\n', + f'repo = "example"\nissue_repository = "{ISSUE_REPOSITORY}"\n', +) +FAKE_GH = """#!/bin/sh +printf '%s\\n' "$*" >> "$FAKE_GH_CALLS" +answer="$FAKE_GH_ANSWERS/${2##*/}" +if [ -f "$answer.json" ]; then + cat "$answer.json" + exit 0 +fi +cat "$answer.stderr" >&2 +exit 1 +""" +NOT_FOUND = "gh: Not Found (HTTP 404)" + + +def downgraded_rule(rule_id: str, issue: int) -> str: + """Return a ratcheted rule whose [rule.downgrade] names ``issue``.""" + return f""" +[[rule]] +id = "{rule_id}" +category = "layers" +statement = "Ratcheted rule." +rationale = "docs/guide.md" +scope = ["pkg/*.py"] +tier = "mechanical" +status = "ratcheted" +autofix = false +[rule.enforcer] +kind = "pytest" +nodes = ["tests/unit/test_boundaries.py::test_routes_have_no_sql"] +[[rule.ci]] +workflow = ".github/workflows/pr.yml" +job = "test" +invocation = "make test" +[rule.downgrade] +reason = "a checker upgrade" +issue = {issue} +[rule.baseline] +file = "rules/baselines/{rule_id}.txt" +count = 1 +""" + + +def issue_answer( + number: int, + *, + state: str = "open", + repository: str = ISSUE_REPOSITORY, + **fields: object, +) -> dict[str, object]: + """Return the fields gh api prints for an issue, with ``fields`` added.""" + return { + "number": number, + "state": state, + "repository_url": f"https://api.github.com/repos/{repository}", + **fields, + } + + +class TrackingIssueTestCase(unittest.TestCase): + """Puts the fake gh first on PATH, outside CI and without a token. + + The GitHub Actions runner sets GITHUB_REPOSITORY in every step, including + the one that runs these tests, so it is cleared too. + """ + + def setUp(self) -> None: + fake_directory = tempfile.TemporaryDirectory() + self.addCleanup(fake_directory.cleanup) + self.fake_root = Path(fake_directory.name) + self.bin_directory = self.fake_root / "bin" + self.answers = self.fake_root / "answers" + self.bin_directory.mkdir() + self.answers.mkdir() + fake_gh = self.bin_directory / "gh" + fake_gh.write_text(FAKE_GH) + fake_gh.chmod(0o755) + self.calls_file = self.fake_root / "calls.txt" + environment = mock.patch.dict( + os.environ, + { + "PATH": f"{self.bin_directory}{os.pathsep}{os.environ['PATH']}", + "FAKE_GH_CALLS": str(self.calls_file), + "FAKE_GH_ANSWERS": str(self.answers), + }, + ) + environment.start() + self.addCleanup(environment.stop) + for name in ("GH_TOKEN", "GITHUB_TOKEN", "GITHUB_ACTIONS", "GITHUB_REPOSITORY"): + os.environ.pop(name, None) + + def answer(self, number: int, issue: dict[str, object]) -> None: + """Make the fake gh print ``issue`` for ``number`` and succeed.""" + (self.answers / f"{number}.json").write_text(json.dumps(issue)) + + def fail_lookup(self, number: int, stderr: str) -> None: + """Make the fake gh print ``stderr`` for ``number`` and fail.""" + (self.answers / f"{number}.stderr").write_text(stderr) + + def gh_calls(self) -> list[str]: + """Return the arguments of each gh call, one string per call.""" + if not self.calls_file.exists(): + return [] + return self.calls_file.read_text().splitlines() + + def check(self, rules: str) -> tuple[list[str], list[str]]: + """Check the registry with ``rules`` added; return violations and notes.""" + registry, problems = load_text(REGISTRY_WITH_ISSUES + rules) + assert registry is not None and not problems, problems + violations, notes = tracking_issues.check_tracking_issues(registry) + return [str(violation) for violation in violations], notes + + +class LookupTest(TrackingIssueTestCase): + """With a token, each downgrade's issue is looked up, and must be open.""" + + def setUp(self) -> None: + super().setUp() + os.environ["GH_TOKEN"] = "token-for-the-fake-gh" + + def test_an_open_issue_passes(self) -> None: + self.answer(912, issue_answer(912)) + + self.assertEqual( + self.check(downgraded_rule("ex.layers.ratcheted", 912)), + ([], ["rule ex.layers.ratcheted: tracking issue #912 is open"]), + ) + self.assertEqual(self.gh_calls(), [f"api repos/{ISSUE_REPOSITORY}/issues/912"]) + + def test_a_closed_issue_fails(self) -> None: + self.answer(912, issue_answer(912, state="closed")) + + violations, _ = self.check(downgraded_rule("ex.layers.ratcheted", 912)) + + self.assertEqual( + violations, + [ + "[ex.layers.ratcheted] downgrade: issue = 912 names no open issue: " + "Issue #912 is closed. Fix: reopen issue #912 while the rule stays " + "weakened, or restore the rule and remove [rule.downgrade]." + ], + ) + + def test_a_number_that_names_no_issue_here_fails(self) -> None: + pull_request = issue_answer( + 912, pull_request={"url": "https://api.github.com/pulls/912"} + ) + moved = issue_answer(5, repository="example/elsewhere") + cases = { + "There is no issue #912 in example/repo": None, + "#912 is a pull request, not an issue": pull_request, + "#912 is not an issue in example/repo: it moved to example/elsewhere#5": ( + moved + ), + } + for problem, answer in cases.items(): + with self.subTest(problem=problem): + for previous_answer in self.answers.iterdir(): + previous_answer.unlink() + if answer is None: + self.fail_lookup(912, NOT_FOUND) + else: + self.answer(912, answer) + + violations, _ = self.check(downgraded_rule("ex.layers.ratcheted", 912)) + + self.assertEqual( + violations, + [ + "[ex.layers.ratcheted] downgrade: issue = 912 names no open " + f"issue: {problem}. Fix: set issue to the number of an open " + "issue in example/repo that tracks restoring the rule." + ], + ) + + def test_a_failed_lookup_fails(self) -> None: + self.fail_lookup(912, "gh: Bad credentials (HTTP 401)") + + violations, _ = self.check(downgraded_rule("ex.layers.ratcheted", 912)) + + self.assertEqual( + violations, + [ + "[ex.layers.ratcheted] downgrade: couldn't check that issue #912 is " + "open: Could not look up #912 in example/repo: gh: Bad credentials " + "(HTTP 401). Fix: run the guard again; if it fails again, check " + "the token in GH_TOKEN, which needs read access to the issues of " + "example/repo." + ], + ) + + def test_a_failed_lookup_names_the_variable_gh_read_the_token_from( + self, + ) -> None: + del os.environ["GH_TOKEN"] + os.environ["GITHUB_TOKEN"] = "token-for-the-fake-gh" + self.fail_lookup(912, "gh: Bad credentials (HTTP 401)") + + violations, _ = self.check(downgraded_rule("ex.layers.ratcheted", 912)) + + self.assertEqual(len(violations), 1) + self.assertIn( + "Fix: run the guard again; if it fails again, check the token in " + "GITHUB_TOKEN, which needs read access", + violations[0], + ) + + def test_a_missing_gh_fails(self) -> None: + empty_directory = self.fake_root / "empty" + empty_directory.mkdir() + os.environ["PATH"] = str(empty_directory) + + violations, _ = self.check(downgraded_rule("ex.layers.ratcheted", 912)) + + self.assertEqual(len(violations), 1) + self.assertIn( + "[ex.layers.ratcheted] downgrade: couldn't check that issue #912 is " + "open: gh could not run (", + violations[0], + ) + + def test_github_token_is_a_token_too(self) -> None: + del os.environ["GH_TOKEN"] + os.environ["GITHUB_TOKEN"] = "token-for-the-fake-gh" + self.answer(912, issue_answer(912)) + + self.assertEqual(self.check(downgraded_rule("ex.layers.ratcheted", 912))[0], []) + self.assertEqual(len(self.gh_calls()), 1) + + def test_each_downgrade_is_checked(self) -> None: + self.answer(912, issue_answer(912)) + self.answer(913, issue_answer(913, state="closed")) + + violations, notes = self.check( + downgraded_rule("ex.layers.first", 912) + + downgraded_rule("ex.layers.second", 913) + ) + + self.assertEqual(len(violations), 1) + self.assertIn("[ex.layers.second] downgrade: issue = 913", violations[0]) + self.assertEqual(notes, ["rule ex.layers.first: tracking issue #912 is open"]) + self.assertEqual( + self.gh_calls(), + [ + f"api repos/{ISSUE_REPOSITORY}/issues/912", + f"api repos/{ISSUE_REPOSITORY}/issues/913", + ], + ) + + def test_the_lookup_must_load(self) -> None: + missing_script = self.fake_root / "missing.py" + script_without_the_lookup = self.fake_root / "renamed.py" + script_without_the_lookup.write_text("def look_up_issue():\n pass\n") + for script in (missing_script, script_without_the_lookup): + with self.subTest(script=script.name): + with mock.patch.object(tracking_issues, "LOOKUP_SCRIPT", script): + violations, _ = self.check( + downgraded_rule("ex.layers.ratcheted", 912) + ) + + self.assertEqual(len(violations), 1) + self.assertIn( + f"[registry] downgrade: couldn't load the issue lookup from " + f"{script}: ", + violations[0], + ) + self.assertEqual(self.gh_calls(), []) + + def test_issues_are_looked_up_only_in_the_repository_the_run_is_in( + self, + ) -> None: + # A pull request can change issue_repository, but not the repository + # that runs its workflow. + os.environ["GITHUB_REPOSITORY"] = "example/other" + self.answer(912, issue_answer(912)) + + violations, _ = self.check(downgraded_rule("ex.layers.ratcheted", 912)) + + self.assertEqual( + violations, + [ + "[registry] downgrade: issue_repository is example/repo, but this " + "run is in example/other, whose issues each [rule.downgrade] must " + 'name. Fix: set issue_repository = "example/other".' + ], + ) + self.assertEqual(self.gh_calls(), []) + + def test_the_repository_comparison_ignores_case(self) -> None: + os.environ["GITHUB_REPOSITORY"] = "Example/Repo" + self.answer(912, issue_answer(912)) + + self.assertEqual(self.check(downgraded_rule("ex.layers.ratcheted", 912))[0], []) + self.assertEqual(len(self.gh_calls()), 1) + + def test_a_registry_without_downgrades_looks_nothing_up(self) -> None: + self.assertEqual(self.check(""), ([], [])) + self.assertEqual(self.gh_calls(), []) + + +class WithoutATokenTest(TrackingIssueTestCase): + """Without a token, CI fails; a local run notes what it didn't check.""" + + def test_on_github_actions_a_missing_token_fails(self) -> None: + os.environ["GITHUB_ACTIONS"] = "true" + + violations, notes = self.check(downgraded_rule("ex.layers.ratcheted", 912)) + + self.assertEqual( + violations, + [ + "[ex.layers.ratcheted] downgrade: couldn't check that issue #912 is " + "open: neither GH_TOKEN nor GITHUB_TOKEN is set. Fix: give the step " + "the job's token, with env: GH_TOKEN: ${{ github.token }} and " + "permissions: issues: read." + ], + ) + self.assertEqual(notes, []) + self.assertEqual(self.gh_calls(), []) + + def test_elsewhere_a_missing_token_is_a_note(self) -> None: + violations, notes = self.check(downgraded_rule("ex.layers.ratcheted", 912)) + + self.assertEqual(violations, []) + self.assertEqual( + notes, + [ + "rule ex.layers.ratcheted: tracking issue #912 was not checked, " + "because neither GH_TOKEN nor GITHUB_TOKEN is set. CI checks it; " + 'to check it here, set one, for example GH_TOKEN="$(gh auth token)"' + ], + ) + self.assertEqual(self.gh_calls(), []) + + def test_on_github_actions_no_downgrade_needs_no_token(self) -> None: + os.environ["GITHUB_ACTIONS"] = "true" + + self.assertEqual(self.check(""), ([], [])) + + +class GuardTest(TrackingIssueTestCase): + """The rules guard checks tracking issues, and prints what it didn't check.""" + + def repo(self) -> Repo: + """Return a valid repository whose registry has one downgrade.""" + repo = Repo( + { + "rules/registry.toml": REGISTRY_WITH_ISSUES + + downgraded_rule("ex.layers.ratcheted", 912), + "rules/baselines/ex.layers.ratcheted.txt": "pkg/routes.py\n", + } + ) + self.addCleanup(repo.__exit__) + return repo + + def test_the_guard_fails_on_a_closed_tracking_issue(self) -> None: + repo = self.repo() + os.environ["GH_TOKEN"] = "token-for-the-fake-gh" + self.answer(912, issue_answer(912, state="closed")) + + self.assertEqual( + [message for message in repo.messages() if "downgrade" in message], + [ + "[ex.layers.ratcheted] downgrade: issue = 912 names no open issue: " + "Issue #912 is closed. Fix: reopen issue #912 while the rule stays " + "weakened, or restore the rule and remove [rule.downgrade]." + ], + ) + + def test_the_quality_guard_prints_the_guard_notes(self) -> None: + repo = self.repo() + printed = io.StringIO() + + with contextlib.redirect_stdout(printed): + violations = guard.check(repo.root) + + self.assertEqual(violations, []) + self.assertIn( + "note: rule ex.layers.ratcheted: tracking issue #912 was not checked, ", + printed.getvalue(), + ) + + +class RepositoryLookupTest(unittest.TestCase): + def test_the_pr_description_check_provides_the_lookup(self) -> None: + lookup = tracking_issues._load_lookup() + + for name in tracking_issues.LOOKUP_NAMES: + self.assertTrue(hasattr(lookup, name), name) + + def test_the_registry_names_this_repository_for_tracking_issues(self) -> None: + registry, problems = load_text( + (tracking_issues.REPO_ROOT / "rules/registry.toml").read_text() + ) + + self.assertEqual(problems, []) + assert registry is not None + self.assertEqual(registry.issue_repository, "PolicyEngine/policyengine-sim-api") + + +class RegistrySchemaTest(TrackingIssueTestCase): + """Where tracking issues live, and which rules may have a downgrade. + + The guard runs on registries with a downgrade, so these tests also use the + fake gh, without a token. + """ + + def test_the_issue_repository_is_owner_and_name(self) -> None: + for value in ('"repo"', '"a/b/c"', '"owner/"', "912"): + with self.subTest(value=value): + _, problems = load_text( + REGISTRY.replace( + 'repo = "example"\n', + f'repo = "example"\nissue_repository = {value}\n', + ) + ) + + self.assertEqual( + [str(problem) for problem in problems], + [ + "[registry] schema: registry.issue_repository must be " + "/, the GitHub repository that tracks issues. " + 'Fix: set issue_repository = "/".' + ], + ) + + def test_a_downgrade_needs_the_issue_repository(self) -> None: + with Repo( + { + "rules/registry.toml": REGISTRY + + downgraded_rule("ex.layers.ratcheted", 912), + "rules/baselines/ex.layers.ratcheted.txt": "pkg/routes.py\n", + }, + render=False, + ) as repo: + self.assertIn( + "[registry] schema: a [rule.downgrade] names an issue, but the " + "registry has no issue_repository. Fix: set issue_repository = " + '"/" at the top of rules/registry.toml.', + repo.messages(), + ) + + def test_an_enforced_rule_has_no_downgrade(self) -> None: + enforced = downgraded_rule("ex.layers.restored", 912).replace( + 'status = "ratcheted"', 'status = "enforced"' + ) + enforced = enforced.split("[rule.baseline]")[0] + with Repo( + {"rules/registry.toml": REGISTRY_WITH_ISSUES + enforced}, render=False + ) as repo: + self.assertIn( + "[ex.layers.restored] schema: an enforced rule has no downgrade. " + "Fix: remove [rule.downgrade] once the rule is enforced again; " + "the issue it names can then be closed.", + repo.messages(), + ) + + +if __name__ == "__main__": + unittest.main() diff --git a/scripts/rules/tracking_issues.py b/scripts/rules/tracking_issues.py new file mode 100644 index 000000000..6ad05eca4 --- /dev/null +++ b/scripts/rules/tracking_issues.py @@ -0,0 +1,167 @@ +"""Check that each ``[rule.downgrade]`` names an open issue. + +The issue tracks restoring the rule, and lives in the registry's +``issue_repository``. The lookup is the PR description check's +(``.github/scripts/check_pr_description.py``): it runs ``gh api``, which reads its +token from ``GH_TOKEN`` or ``GITHUB_TOKEN``. + +On GitHub Actions the check never passes without an answer from GitHub: a +missing token, or a lookup that fails, is a violation. Elsewhere, without a +token, it notes each issue it didn't check, and CI checks them. + +A pull request can change ``issue_repository``, but not the repository its +workflow runs in, which the runner sets in ``GITHUB_REPOSITORY``. When that is +set, the two must match, so a pull request can't name an issue elsewhere. +""" + +from __future__ import annotations + +import importlib.util +import os +from pathlib import Path +from typing import TYPE_CHECKING + +from scripts.rules.model import Violation + +if TYPE_CHECKING: + from types import ModuleType + + from scripts.rules.model import Registry, Rule + +REPO_ROOT = Path(__file__).resolve().parents[2] +LOOKUP_SCRIPT = REPO_ROOT / ".github" / "scripts" / "check_pr_description.py" +# gh reads its token from the first of these that is set. +TOKEN_VARIABLES = ("GH_TOKEN", "GITHUB_TOKEN") +CHECK = "downgrade" +NEITHER_VARIABLE_SET = "neither GH_TOKEN nor GITHUB_TOKEN is set" +# What the check uses from the lookup script. +LOOKUP_NAMES = ("require_open_issue", "NotAnOpenIssueError", "IssueLookupError") + + +class LookupUnavailableError(Exception): + """The issue lookup script could not be loaded.""" + + +def _load_lookup() -> ModuleType: + """Load the PR description check, whose ``require_open_issue`` is the lookup.""" + spec = importlib.util.spec_from_file_location("check_pr_description", LOOKUP_SCRIPT) + if spec is None or spec.loader is None: + msg = "it is not a Python module" + raise LookupUnavailableError(msg) + module = importlib.util.module_from_spec(spec) + try: + spec.loader.exec_module(module) + except (ImportError, OSError, SyntaxError) as error: + raise LookupUnavailableError(str(error)) from error + missing = [name for name in LOOKUP_NAMES if not hasattr(module, name)] + if missing: + msg = f"it doesn't define {', '.join(missing)}" + raise LookupUnavailableError(msg) + return module + + +def _check_issue( + lookup: ModuleType, repository: str, token_variable: str, rule: Rule, issue: int +) -> tuple[Violation | None, str | None]: + """Look up one downgrade's issue; return a violation or a note. + + ``token_variable`` is where gh read its token, for the fix of a lookup + that fails. + """ + try: + lookup.require_open_issue(repository, issue) + except lookup.NotAnOpenIssueError as error: + fix = ( + f"set issue to the number of an open issue in {repository} that tracks " + "restoring the rule" + if error.state is None + else f"reopen issue #{issue} while the rule stays weakened, or restore " + "the rule and remove [rule.downgrade]" + ) + problem = str(error).rstrip(".") + return Violation( + rule.id, CHECK, f"issue = {issue} names no open issue: {problem}", fix + ), None + except lookup.IssueLookupError as error: + problem = str(error).rstrip(".") + except OSError as error: + problem = f"gh could not run ({error})" + else: + return None, f"rule {rule.id}: tracking issue #{issue} is open" + return Violation( + rule.id, + CHECK, + f"couldn't check that issue #{issue} is open: {problem}", + f"run the guard again; if it fails again, check the token in " + f"{token_variable}, which needs read access to the issues of {repository}", + ), None + + +def check_tracking_issues(registry: Registry) -> tuple[list[Violation], list[str]]: + """Check that each downgrade's issue is open; return (violations, notes). + + ``validate`` reports a downgrade in a registry without an + ``issue_repository``, so this checks nothing then. + """ + downgraded = [ + (rule, rule.downgrade.issue) + for rule in registry.rules + if rule.downgrade is not None + ] + repository = registry.issue_repository + if not downgraded or not repository: + return [], [] + run_repository = os.environ.get("GITHUB_REPOSITORY") + if run_repository and run_repository.lower() != repository.lower(): + return [ + Violation( + None, + CHECK, + f"issue_repository is {repository}, but this run is in " + f"{run_repository}, whose issues each [rule.downgrade] must name", + f'set issue_repository = "{run_repository}"', + ) + ], [] + token_variable = next( + (name for name in TOKEN_VARIABLES if os.environ.get(name)), None + ) + if token_variable is None: + if os.environ.get("GITHUB_ACTIONS"): + return [ + Violation( + rule.id, + CHECK, + f"couldn't check that issue #{issue} is open: " + f"{NEITHER_VARIABLE_SET}", + "give the step the job's token, with env: GH_TOKEN: " + "${{ github.token }} and permissions: issues: read", + ) + for rule, issue in downgraded + ], [] + return [], [ + f"rule {rule.id}: tracking issue #{issue} was not checked, because " + f"{NEITHER_VARIABLE_SET}. CI checks it; to check it here, set one, " + 'for example GH_TOKEN="$(gh auth token)"' + for rule, issue in downgraded + ] + try: + lookup = _load_lookup() + except LookupUnavailableError as error: + return [ + Violation( + None, + CHECK, + f"couldn't load the issue lookup from {LOOKUP_SCRIPT}: {error}", + "restore .github/scripts/check_pr_description.py, whose " + "require_open_issue looks issues up", + ) + ], [] + violations: list[Violation] = [] + notes: list[str] = [] + for rule, issue in downgraded: + violation, note = _check_issue(lookup, repository, token_variable, rule, issue) + if violation is not None: + violations.append(violation) + if note is not None: + notes.append(note) + return violations, notes diff --git a/scripts/rules/validate.py b/scripts/rules/validate.py index ca3c8854c..ef1199929 100644 --- a/scripts/rules/validate.py +++ b/scripts/rules/validate.py @@ -514,6 +514,39 @@ def _pyright_projects_listed_twice(registry: Registry) -> list[Violation]: return violations +def _validate_downgrades(registry: Registry) -> list[Violation]: + """Check that downgrades can be looked up, and only weakened rules have one. + + An enforced rule is as strong as a rule can be. Its downgrade section would + keep naming a tracking issue that restoring the rule lets someone close, + and the guard would then fail every pull request. + """ + violations = [ + _schema_violation( + rule, + "an enforced rule has no downgrade", + "remove [rule.downgrade] once the rule is enforced again; the issue it " + "names can then be closed", + ) + for rule in registry.rules + if rule.downgrade is not None and rule.status is Status.ENFORCED + ] + if not registry.issue_repository and any( + rule.downgrade is not None for rule in registry.rules + ): + violations.append( + Violation( + None, + "schema", + "a [rule.downgrade] names an issue, but the registry has no " + "issue_repository", + 'set issue_repository = "/" at the top of ' + "rules/registry.toml", + ) + ) + return violations + + def validate(registry: Registry, root: Path, today: date) -> list[Violation]: """Check the registry's fields and file references against the tree at ``root``. @@ -528,4 +561,5 @@ def validate(registry: Registry, root: Path, today: date) -> list[Violation]: for rule in registry.rules: violations.extend(_validate_rule(rule, root, category_ids, today)) violations.extend(_pyright_projects_listed_twice(registry)) + violations.extend(_validate_downgrades(registry)) return violations