From 647bc367634f28ee0daf0f7b7ffaa7ae94737ed0 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Mon, 26 Oct 2026 17:48:50 +0400 Subject: [PATCH 01/12] Require an issue number in every [rule.downgrade] A [rule.downgrade] section now needs `issue`, the number of the issue that tracks restoring the rule, next to a non-empty `reason`. The loader reports an issue that isn't a positive whole number (including TOML's `true`), an empty reason, a section that isn't a table and unknown keys, and reads such a section as absent, so it never allows a weaker rule. The parsed section becomes a Downgrade(reason, issue) in place of Rule.downgrade_reason. The guard's note for a downgrade names the issue, and its fix for a weakened status says that only a person may add the section. Refs #912 Co-Authored-By: Claude Opus 5.5 --- docs/engineering/skills/rules-registry.md | 8 ++- docs/engineering/skills/type-checking.md | 8 ++- scripts/rules/baseline.py | 15 ++-- scripts/rules/load.py | 62 ++++++++++++---- scripts/rules/model.py | 14 +++- scripts/rules/tests/test_guard.py | 40 +++++++---- scripts/rules/tests/test_load.py | 88 +++++++++++++++++++++-- 7 files changed, 193 insertions(+), 42 deletions(-) diff --git a/docs/engineering/skills/rules-registry.md b/docs/engineering/skills/rules-registry.md index d174eb842..03c7d9f69 100644 --- a/docs/engineering/skills/rules-registry.md +++ b/docs/engineering/skills/rules-registry.md @@ -58,7 +58,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 the issue that tracks restoring the rule. Required when a rule moves from enforced/ratcheted to advisory/doc-only, or from enforced to ratcheted. | `blocking` is derived, never declared: a rule blocks merges when its status is `enforced` or `ratcheted` and its tier is not `judgment`. @@ -127,8 +127,10 @@ fail the guard yet. `ratcheted` with a baseline file of tolerated violations. 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`, the number of the issue that tracks restoring the rule. The guard + accepts any non-empty reason and any positive issue number; the reason is + for the reviewer. - **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..227d3a878 100644 --- a/docs/engineering/skills/type-checking.md +++ b/docs/engineering/skills/type-checking.md @@ -114,10 +114,12 @@ After fixing errors: 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`. -It doesn't check: +when 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. It doesn't check: -- what the reason says: any non-empty reason passes; +- what the reason says, or which issue the number names: any non-empty reason + and any positive number 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 diff --git a/scripts/rules/baseline.py b/scripts/rules/baseline.py index b10b64881..09ec11fd3 100644 --- a/scripts/rules/baseline.py +++ b/scripts/rules/baseline.py @@ -13,6 +13,13 @@ 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 = " +) + + class BaseUnavailableError(Exception): """The base ref could not be read.""" @@ -77,9 +84,10 @@ def check_baselines( 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 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,8 +95,7 @@ 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( diff --git a/scripts/rules/load.py b/scripts/rules/load.py index 83a5da5ad..4cb7f8c47 100644 --- a/scripts/rules/load.py +++ b/scripts/rules/load.py @@ -19,6 +19,7 @@ CiRef, CiScripts, Discovery, + Downgrade, Enforcer, Ignored, Kind, @@ -70,6 +71,10 @@ "exception", "downgrade", } +_DOWNGRADE_KEYS = {"reason", "issue"} +DOWNGRADE_ISSUE_FIX = ( + "write issue = " +) _DISCOVERY_KEYS = { "workflows", "guard_test_globs", @@ -348,22 +353,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 +422,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,7 +448,7 @@ def _rule(c: _Collector, raw: object, index: int) -> Rule | None: proof=proof, baseline=baseline, exceptions=exceptions, - downgrade_reason=downgrade_reason, + downgrade=downgrade, ) diff --git a/scripts/rules/model.py b/scripts/rules/model.py index 736e72a39..7865673e4 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: diff --git a/scripts/rules/tests/test_guard.py b/scripts/rules/tests/test_guard.py index 3f28b54d2..ab2cac75f 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.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" @@ -800,7 +803,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 +818,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,14 +852,16 @@ 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], []) diff --git a/scripts/rules/tests/test_load.py b/scripts/rules/tests/test_load.py index 7224fca20..47aaf1d1f 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,81 @@ 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 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() From 70148ca4612ac7663dc49ec1c7a5306443b034da Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Mon, 26 Oct 2026 17:54:54 +0400 Subject: [PATCH 02/12] Fail when a package leaves an enforced Pyright rule without a downgrade The rules guard compared each rule's status with the base branch, but not the packages a Pyright rule lists. Moving a package out of an enforced rule into a new ratcheted or advisory rule passed, because a new rule has no status on the base branch to compare with. check_enforced_packages now holds each package that an enforced Pyright rule lists on the base branch, and whose pyproject.toml still exists, to an enforced Pyright rule on the head. Moving it to another enforced rule, or deleting it, passes. Moving it to a rule that isn't enforced needs [rule.downgrade] on that rule; listing it in no rule fails. A rule that keeps the package under the same id is left to the status comparison, so a ratcheted rule is reported once. Only Pyright enforcers list packages: the schema rejects `projects` on every other kind, and a package that moves between two enforced rules keeps its check only if both run the same checker. The base comparison moves out of run_guard into _compare_with_base, to keep run_guard within ruff's complexity limits. Refs #912 Co-Authored-By: Claude Opus 5.5 --- docs/engineering/skills/rules-registry.md | 7 +- docs/engineering/skills/type-checking.md | 25 +-- scripts/rules/baseline.py | 80 ++++++++- scripts/rules/guard.py | 72 +++++---- scripts/rules/tests/test_guard.py | 187 +++++++++++++++++++++- 5 files changed, 327 insertions(+), 44 deletions(-) diff --git a/docs/engineering/skills/rules-registry.md b/docs/engineering/skills/rules-registry.md index 03c7d9f69..8938f327b 100644 --- a/docs/engineering/skills/rules-registry.md +++ b/docs/engineering/skills/rules-registry.md @@ -58,7 +58,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]` | `reason`, and `issue`: the number of the issue that tracks restoring the rule. Required when a rule moves from enforced/ratcheted to advisory/doc-only, or from enforced to ratcheted. | +| `[rule.downgrade]` | `reason`, and `issue`: the number of the issue that tracks restoring the rule. Required when a rule moves from enforced/ratcheted to advisory/doc-only, or from enforced to ratcheted, and on a rule that lists a package an enforced rule listed 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`. @@ -131,6 +131,11 @@ fail the guard yet. `issue`, the number of the issue that tracks restoring the rule. The guard accepts any non-empty reason and any positive issue number; the reason is for the reviewer. +- A package in an enforced rule's `enforcer.projects` on the base branch (only + `pyright` enforcers list packages) stays in an enforced rule of the same + kind while its `pyproject.toml` exists. If it moves to a rule that isn't + enforced, that rule needs a `[rule.downgrade]` section. Moving it between + enforced rules, or deleting it, passes. - **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 227d3a878..5d245f92a 100644 --- a/docs/engineering/skills/type-checking.md +++ b/docs/engineering/skills/type-checking.md @@ -111,20 +111,25 @@ 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]` section, -which gives a `reason` and an `issue`, the number of the issue that tracks -restoring the rule. It doesn't check: +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 package that an enforced Pyright rule lists on the base branch, and whose + `pyproject.toml` still exists, isn't listed by an enforced Pyright rule, + unless the rule that lists it now has a `[rule.downgrade]` section. Moving a + package to another enforced Pyright rule, or deleting it, passes. + +It doesn't check: - what the reason says, or which issue the number names: any non-empty reason and any positive number 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. + the count and passes. Review is what catches these. `.github/CODEOWNERS` assigns `rules/` to the core developers, but `sim.process.rule-machinery-code-owned` is doc-only: merging diff --git a/scripts/rules/baseline.py b/scripts/rules/baseline.py index 09ec11fd3..e35ca8dc0 100644 --- a/scripts/rules/baseline.py +++ b/scripts/rules/baseline.py @@ -7,7 +7,14 @@ 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 @@ -103,3 +110,74 @@ def check_baselines( for rule_id in sorted(current_ids - {rule.id for rule in base.rules}) ) return violations, notes + + +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_enforced_packages( + current: Registry, base: Registry, root: Path +) -> tuple[list[Violation], list[str]]: + """Check that each package an enforced Pyright rule lists on the base still is. + + Return (violations, notes). A package may move to another enforced Pyright + rule, or leave with its ``pyproject.toml`` deleted. Moving it to a rule + that isn't enforced weakens its check, so that rule needs + ``[rule.downgrade]``. A rule that keeps the package under the same id is + left to ``check_baselines``, which compares its status. + """ + violations: list[Violation] = [] + notes: list[str] = [] + head_listings = _pyright_listings(current) + for package, base_rule in _pyright_listings(base).items(): + if base_rule.status is not Status.ENFORCED: + continue + if not (root / package / "pyproject.toml").is_file(): + continue + head_rule = head_listings.get(package) + if head_rule is None: + violations.append( + Violation( + base_rule.id, + "baseline", + f"package {package} left enforced rule {base_rule.id}, and no " + "Pyright rule lists it now", + f"list {package} in an enforced Pyright rule again, or delete " + "the package", + ) + ) + continue + if head_rule.status is Status.ENFORCED or head_rule.id == base_rule.id: + continue + move = ( + f"package {package} moved from enforced rule {base_rule.id} to " + f"{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"list {package} in an enforced Pyright rule again; " + f"{PERSON_DOWNGRADE}", + ) + ) + return violations, notes diff --git a/scripts/rules/guard.py b/scripts/rules/guard.py index 99ba5e357..307e0d9eb 100644 --- a/scripts/rules/guard.py +++ b/scripts/rules/guard.py @@ -11,6 +11,7 @@ from scripts.rules.baseline import ( BaseUnavailableError, check_baselines, + check_enforced_packages, default_base_ref, load_base_registry, ) @@ -77,10 +78,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() @@ -99,41 +99,51 @@ def run_guard( result.violations += check_adapters(registry.agents, root) result.violations += _stale_outputs(registry, root) - 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 + 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_enforced_packages(registry, base, root), + ): + result.violations += violations + result.notes += notes def check() -> list[str]: diff --git a/scripts/rules/tests/test_guard.py b/scripts/rules/tests/test_guard.py index ab2cac75f..e6def5cfb 100644 --- a/scripts/rules/tests/test_guard.py +++ b/scripts/rules/tests/test_guard.py @@ -7,7 +7,7 @@ from datetime import date from typing import TYPE_CHECKING, ClassVar -from scripts.rules.baseline import check_baselines +from scripts.rules.baseline import check_baselines, check_enforced_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 @@ -578,6 +578,191 @@ 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 EnforcedPackageTest(unittest.TestCase): + """A package an enforced Pyright rule lists stays in an enforced Pyright rule.""" + + 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_enforced_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_packages_that_were_not_enforced_on_the_base_are_not_compared( + 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), ([], [])) + + 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() From 0e14ef95efb87d87bd46d19a8f7a6d81bcc21769 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Mon, 26 Oct 2026 17:58:18 +0400 Subject: [PATCH 03/12] Require a downgrade for a new rule that starts ratcheted A rule the base branch doesn't have produced only the note "rule ... is new", so a new rule could start ratcheted, with a baseline of tolerated violations, and no [rule.downgrade]. Such a rule is weaker than an enforced one from the start. check_baselines now fails a new ratcheted rule whose baseline count is above 0 unless it has [rule.downgrade], and notes the reason and issue when it has one. A new ratcheted rule with a count of 0 passes. Refs #912 Co-Authored-By: Claude Opus 5.5 --- docs/engineering/skills/rules-registry.md | 8 ++-- docs/engineering/skills/type-checking.md | 9 +++-- scripts/rules/baseline.py | 38 ++++++++++++++++-- scripts/rules/tests/test_guard.py | 48 +++++++++++++++++++++++ 4 files changed, 93 insertions(+), 10 deletions(-) diff --git a/docs/engineering/skills/rules-registry.md b/docs/engineering/skills/rules-registry.md index 8938f327b..09f236b3f 100644 --- a/docs/engineering/skills/rules-registry.md +++ b/docs/engineering/skills/rules-registry.md @@ -58,7 +58,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]` | `reason`, and `issue`: the number of the issue that tracks restoring the rule. Required when a rule moves from enforced/ratcheted to advisory/doc-only, or from enforced to ratcheted, and on a rule that lists a package an enforced rule listed on the base branch. | +| `[rule.downgrade]` | `reason`, and `issue`: the number of the issue that tracks restoring the 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 lists a package an enforced rule listed 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,8 +124,10 @@ 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 a `[rule.downgrade]` section with a `reason` and an `issue`, the number of the issue that tracks restoring the rule. The guard diff --git a/docs/engineering/skills/type-checking.md b/docs/engineering/skills/type-checking.md index 5d245f92a..c55bf36d4 100644 --- a/docs/engineering/skills/type-checking.md +++ b/docs/engineering/skills/type-checking.md @@ -119,6 +119,8 @@ with the base branch. It fails when: - 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 Pyright rule lists on the base branch, and whose `pyproject.toml` still exists, isn't listed by an enforced Pyright rule, unless the rule that lists it now has a `[rule.downgrade]` section. Moving a @@ -136,9 +138,10 @@ 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. +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 and make the rule `enforced`. diff --git a/scripts/rules/baseline.py b/scripts/rules/baseline.py index e35ca8dc0..c0412942c 100644 --- a/scripts/rules/baseline.py +++ b/scripts/rules/baseline.py @@ -105,13 +105,43 @@ def check_baselines( 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. diff --git a/scripts/rules/tests/test_guard.py b/scripts/rules/tests/test_guard.py index e6def5cfb..68c8db441 100644 --- a/scripts/rules/tests/test_guard.py +++ b/scripts/rules/tests/test_guard.py @@ -1051,6 +1051,54 @@ def test_ratcheting_an_enforced_rule_needs_reason(self) -> None: ) 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") From 46f8e41f3821b6b07522c06a170a0376268aaa80 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Mon, 26 Oct 2026 18:08:32 +0400 Subject: [PATCH 04/12] Separate the issue lookup from the PR description's fix text check_pr_description.py's lookup raised DescriptionError with messages that end in how to fix the description, so another check couldn't reuse it. require_open_issue now looks an issue up on its own: it raises NotAnOpenIssueError when the number names a missing, moved or closed issue or a pull request (with the issue's state when it is a closed issue here), and IssueLookupError when GitHub can't be asked or its answer isn't an issue. Their messages say only what is wrong. main() adds the same fixes as before, so the PR description check's messages don't change, except that the two lookup failures that said only "Re-run this job." now also say to check the GH_TOKEN the workflow gives the job, as the third one already did. Refs #912 Co-Authored-By: Claude Opus 5.5 --- .github/scripts/check_pr_description.py | 116 +++++++++++++----- .../tests/test_pr_description_check.py | 105 ++++++++++++++++ 2 files changed, 188 insertions(+), 33 deletions(-) diff --git a/.github/scripts/check_pr_description.py b/.github/scripts/check_pr_description.py index 7d16f9682..8733519a4 100644 --- a/.github/scripts/check_pr_description.py +++ b/.github/scripts/check_pr_description.py @@ -23,6 +23,10 @@ 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, for other checks that +need an open issue. Its errors say only what is wrong, and each caller adds its +own fix. """ from __future__ import annotations @@ -41,6 +45,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 +67,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 +136,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 +148,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 +172,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 +242,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/projects/policyengine-simulation-entry/tests/test_pr_description_check.py b/projects/policyengine-simulation-entry/tests/test_pr_description_check.py index 659f356c6..c1e817df4 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,111 @@ def test_the_check_requires_a_pull_request_event(fakes, tmp_path): assert fakes.calls("gh") == [] +# --- The issue lookup, for other checks ----------------------------------- +# +# Other checks can reuse the lookup. 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 ----------------------------------------------------- From f4d37f51e7f846eadeab1e0505f138939e9babcc Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Mon, 26 Oct 2026 18:17:20 +0400 Subject: [PATCH 05/12] Check that each [rule.downgrade] names an open tracking issue The rules guard now looks up the issue of every [rule.downgrade] in the registry on every run (scripts/rules/tracking_issues.py), with the PR description check's require_open_issue, which runs gh api. A number that names a missing, moved or closed issue, or a pull request, fails. gh reads its token from GH_TOKEN or GITHUB_TOKEN. On GitHub Actions, neither being set, a lookup that fails, gh not running, or a lookup script that doesn't load or define the lookup, fails the guard. Outside GitHub Actions, without a token, the guard notes each issue it didn't check instead; with a token, it fails as in CI. scripts/run_quality_guards.py now prints the rules guard's notes, as it already did the ci-scripts guard's, so that note is visible there. The registry names the repository with a new top-level key, issue_repository, so local runs and CI look issues up in the same place. A downgrade in a registry without it, and a downgrade on an enforced rule, are schema violations. The second keeps the issue's lifecycle consistent: the pull request that restores a rule must remove its section, so closing the issue afterwards doesn't fail the guard on every later pull request. Refs #912 Co-Authored-By: Claude Opus 5.5 --- .github/scripts/check_pr_description.py | 7 +- docs/engineering/skills/rules-registry.md | 16 +- docs/engineering/skills/type-checking.md | 25 +- .../tests/test_pr_description_check.py | 6 +- rules/registry.toml | 2 + scripts/rules/guard.py | 17 +- scripts/rules/load.py | 21 + scripts/rules/model.py | 2 + scripts/rules/tests/test_tracking_issues.py | 434 ++++++++++++++++++ scripts/rules/tracking_issues.py | 145 ++++++ scripts/rules/validate.py | 34 ++ 11 files changed, 691 insertions(+), 18 deletions(-) create mode 100644 scripts/rules/tests/test_tracking_issues.py create mode 100644 scripts/rules/tracking_issues.py diff --git a/.github/scripts/check_pr_description.py b/.github/scripts/check_pr_description.py index 8733519a4..0de83d6d6 100644 --- a/.github/scripts/check_pr_description.py +++ b/.github/scripts/check_pr_description.py @@ -24,9 +24,10 @@ - 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, for other checks that -need an open issue. Its errors say only what is wrong, and each caller adds its -own fix. +``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 diff --git a/docs/engineering/skills/rules-registry.md b/docs/engineering/skills/rules-registry.md index 09f236b3f..34ee8e2b0 100644 --- a/docs/engineering/skills/rules-registry.md +++ b/docs/engineering/skills/rules-registry.md @@ -58,7 +58,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]` | `reason`, and `issue`: the number of the issue that tracks restoring the 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 lists a package an enforced rule listed on the base branch. | +| `[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 lists a package an enforced rule listed 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`. @@ -130,9 +130,17 @@ fail the guard yet. against the base branch. - Moving a rule from enforced/ratcheted to advisory/doc-only, or from enforced to ratcheted, needs a `[rule.downgrade]` section with a `reason` and an - `issue`, the number of the issue that tracks restoring the rule. The guard - accepts any non-empty reason and any positive issue number; the reason is - for the reviewer. + `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. Elsewhere, without a token, it + prints a note for each issue it didn't check. Closing the issue while the + rule stays weakened 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 is for the reviewer. - A package in an enforced rule's `enforcer.projects` on the base branch (only `pyright` enforcers list packages) stays in an enforced rule of the same kind while its `pyproject.toml` exists. If it moves to a rule that isn't diff --git a/docs/engineering/skills/type-checking.md b/docs/engineering/skills/type-checking.md index c55bf36d4..d4bad06d0 100644 --- a/docs/engineering/skills/type-checking.md +++ b/docs/engineering/skills/type-checking.md @@ -124,26 +124,39 @@ with the base branch. It fails when: - a package that an enforced Pyright rule lists on the base branch, and whose `pyproject.toml` still exists, isn't listed by an enforced Pyright rule, unless the rule that lists it now has a `[rule.downgrade]` section. Moving a - package to another enforced Pyright rule, or deleting it, passes. + package to another enforced Pyright rule, or deleting it, passes; +- 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. 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, or which issue the number names: any non-empty reason - and any positive number pass; +- 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. + the count and passes; +- 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. 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. +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 and make the rule `enforced`. +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 rule +stays weakened 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 c1e817df4..cab3d3416 100644 --- a/projects/policyengine-simulation-entry/tests/test_pr_description_check.py +++ b/projects/policyengine-simulation-entry/tests/test_pr_description_check.py @@ -431,8 +431,10 @@ def test_the_check_requires_a_pull_request_event(fakes, tmp_path): # --- The issue lookup, for other checks ----------------------------------- # -# Other checks can reuse the lookup. Its errors say only what is wrong; each -# caller adds its own fix, so none of these messages mentions the description. +# 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 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/guard.py b/scripts/rules/guard.py index 307e0d9eb..eec80a0aa 100644 --- a/scripts/rules/guard.py +++ b/scripts/rules/guard.py @@ -31,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] @@ -98,6 +99,9 @@ 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 if compare_base: _compare_with_base(registry, root, base_ref or default_base_ref(), result) @@ -146,6 +150,13 @@ def _compare_with_base( result.notes += notes -def check() -> list[str]: - """Adapter for scripts/run_quality_guards.py.""" - return [str(v) for v in run_guard(REPO_ROOT).violations] +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 4cb7f8c47..6e6bb5716 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 @@ -44,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", @@ -452,6 +456,22 @@ def _rule(c: _Collector, raw: object, index: int) -> Rule | None: ) +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]]: """Turn raw TOML data into a ``Registry``, collecting every schema problem.""" c = _Collector() @@ -485,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 7865673e4..58e08c6b6 100644 --- a/scripts/rules/model.py +++ b/scripts/rules/model.py @@ -281,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_tracking_issues.py b/scripts/rules/tests/test_tracking_issues.py new file mode 100644 index 000000000..787f90f8f --- /dev/null +++ b/scripts/rules/tests/test_tracking_issues.py @@ -0,0 +1,434 @@ +"""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.""" + + 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"): + 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_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_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(unittest.TestCase): + """Where tracking issues live, and which rules may have a downgrade.""" + + 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..83dbce2f6 --- /dev/null +++ b/scripts/rules/tracking_issues.py @@ -0,0 +1,145 @@ +"""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. +""" + +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, rule: Rule, issue: int +) -> tuple[Violation | None, str | None]: + """Look up one downgrade's issue; return a violation or a note.""" + 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}", + "run the guard again; if it fails again, check the token in GH_TOKEN, " + f"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 [], [] + if not any(os.environ.get(name) for name in TOKEN_VARIABLES): + 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, 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 From fa8c97981a343fd3e591e70fe1e95423be3f86f2 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Mon, 26 Oct 2026 18:37:33 +0400 Subject: [PATCH 06/12] Give the quality guards a token that can read issues The rules guard fails on GitHub Actions when it can't look up a downgrade's tracking issue, so the quality-guards job now gives the step that runs scripts/run_quality_guards.py the job's token as GH_TOKEN, which gh reads. The guards run in that process, and gh inherits its environment. The job's permissions are now only contents: read, for the checkout, and issues: read, for the lookup. The rules tool's tests run in an earlier step without the token. test_quality_guards_workflow.py pins both: the job's permissions, and that only the guard step gets the token. Refs #912 Co-Authored-By: Claude Opus 5.5 --- .github/workflows/pr.yml | 9 +++++ .../tests/test_quality_guards_workflow.py | 38 +++++++++++++++++++ 2 files changed, 47 insertions(+) create mode 100644 projects/policyengine-simulation-entry/tests/test_quality_guards_workflow.py 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/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 }}"} From a6991cb975035cbc66e079a1614449df4c8aa971 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Mon, 26 Oct 2026 18:42:20 +0400 Subject: [PATCH 07/12] Hold a ratcheted rule's packages to a rule at least as strong The package comparison skipped every base rule that wasn't enforced. A package could move from a ratcheted Pyright rule into a new advisory rule with no [[rule.ci]], leave the pyright job's matrix and stop being type-checked in CI, with only the note "rule ... is new". The status comparison already treats the same change of a whole rule, ratcheted to advisory, as a downgrade. check_enforced_packages becomes check_pyright_packages. A package from a ratcheted rule must now stay in an enforced or ratcheted Pyright rule, unless the rule that lists it now has [rule.downgrade]. A move between ratcheted rules passes here; check_baselines compares the counts. Both comparisons share _weakens, so they agree on which status changes weaken a rule. Found in the self-review. Refs #912 Co-Authored-By: Claude Opus 5.5 --- docs/engineering/skills/rules-registry.md | 14 ++--- docs/engineering/skills/type-checking.md | 10 ++-- scripts/rules/baseline.py | 65 +++++++++++++--------- scripts/rules/guard.py | 4 +- scripts/rules/tests/test_guard.py | 66 +++++++++++++++++++++-- 5 files changed, 116 insertions(+), 43 deletions(-) diff --git a/docs/engineering/skills/rules-registry.md b/docs/engineering/skills/rules-registry.md index 34ee8e2b0..9c0fada40 100644 --- a/docs/engineering/skills/rules-registry.md +++ b/docs/engineering/skills/rules-registry.md @@ -58,7 +58,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]` | `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 lists a package an enforced rule listed on the base branch. | +| `[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`. @@ -141,11 +141,13 @@ fail the guard yet. 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 is for the reviewer. -- A package in an enforced rule's `enforcer.projects` on the base branch (only - `pyright` enforcers list packages) stays in an enforced rule of the same - kind while its `pyproject.toml` exists. If it moves to a rule that isn't - enforced, that rule needs a `[rule.downgrade]` section. Moving it between - enforced rules, or deleting it, passes. +- 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. Moving it to a rule at least as + strong, or deleting it, passes. - **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 d4bad06d0..2cd9b2947 100644 --- a/docs/engineering/skills/type-checking.md +++ b/docs/engineering/skills/type-checking.md @@ -121,10 +121,12 @@ with the base branch. It fails when: 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 Pyright rule lists on the base branch, and whose - `pyproject.toml` still exists, isn't listed by an enforced Pyright rule, - unless the rule that lists it now has a `[rule.downgrade]` section. Moving a - package to another enforced Pyright rule, or deleting it, passes; +- 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. Moving a package to a rule at least as strong, + or deleting it, passes; - 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 diff --git a/scripts/rules/baseline.py b/scripts/rules/baseline.py index c0412942c..1c869a506 100644 --- a/scripts/rules/baseline.py +++ b/scripts/rules/baseline.py @@ -27,6 +27,22 @@ ) +# 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.""" @@ -85,12 +101,7 @@ 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 _weakens(old.status, new.status): if new.downgrade: notes.append( f"rule {new.id} downgraded to {new.status}: " @@ -157,43 +168,46 @@ def _pyright_listings(registry: Registry) -> dict[str, Rule]: return listings -def check_enforced_packages( +def check_pyright_packages( current: Registry, base: Registry, root: Path ) -> tuple[list[Violation], list[str]]: - """Check that each package an enforced Pyright rule lists on the base still is. - - Return (violations, notes). A package may move to another enforced Pyright - rule, or leave with its ``pyproject.toml`` deleted. Moving it to a rule - that isn't enforced weakens its check, so that rule needs - ``[rule.downgrade]``. A rule that keeps the package under the same id is - left to ``check_baselines``, which compares its status. + """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(): - if base_rule.status is not Status.ENFORCED: - continue - if not (root / package / "pyproject.toml").is_file(): + 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 enforced rule {base_rule.id}, and no " - "Pyright rule lists it now", - f"list {package} in an enforced Pyright rule again, or delete " - "the package", + 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 is Status.ENFORCED or head_rule.id == base_rule.id: + if head_rule.status in allowed or head_rule.id == base_rule.id: continue move = ( - f"package {package} moved from enforced rule {base_rule.id} to " - f"{head_rule.status} rule {head_rule.id}" + 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( @@ -206,8 +220,7 @@ def check_enforced_packages( head_rule.id, "baseline", move, - f"list {package} in an enforced Pyright rule again; " - f"{PERSON_DOWNGRADE}", + f"{relist}; {PERSON_DOWNGRADE}", ) ) return violations, notes diff --git a/scripts/rules/guard.py b/scripts/rules/guard.py index eec80a0aa..ee42a01db 100644 --- a/scripts/rules/guard.py +++ b/scripts/rules/guard.py @@ -11,7 +11,7 @@ from scripts.rules.baseline import ( BaseUnavailableError, check_baselines, - check_enforced_packages, + check_pyright_packages, default_base_ref, load_base_registry, ) @@ -144,7 +144,7 @@ def _compare_with_base( return for violations, notes in ( check_baselines(registry, base), - check_enforced_packages(registry, base, root), + check_pyright_packages(registry, base, root), ): result.violations += violations result.notes += notes diff --git a/scripts/rules/tests/test_guard.py b/scripts/rules/tests/test_guard.py index 68c8db441..44c066b30 100644 --- a/scripts/rules/tests/test_guard.py +++ b/scripts/rules/tests/test_guard.py @@ -7,7 +7,7 @@ from datetime import date from typing import TYPE_CHECKING, ClassVar -from scripts.rules.baseline import check_baselines, check_enforced_packages +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 @@ -604,8 +604,8 @@ def pyright_rule( ) -class EnforcedPackageTest(unittest.TestCase): - """A package an enforced Pyright rule lists stays in an enforced Pyright rule.""" +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"]) @@ -619,7 +619,7 @@ def compare(self, head_rules: str) -> tuple[list[str], list[str]]: 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_enforced_packages(head, base, self.repo.root) + 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: @@ -725,7 +725,7 @@ def test_a_rule_that_keeps_its_packages_is_left_to_the_status_check( self.assertEqual(self.compare(head_rules), ([], [])) - def test_packages_that_were_not_enforced_on_the_base_are_not_compared( + def test_a_package_moved_from_a_ratcheted_to_an_advisory_rule_is_a_downgrade( self, ) -> None: self.BASE_RULES = pyright_rule( @@ -737,8 +737,64 @@ def test_packages_that_were_not_enforced_on_the_base_are_not_compared( "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: + # The new rule's baseline count is compared by check_baselines instead. + 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() From d33eaa87edd5845da19237a6bb90da71d7524a41 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Mon, 26 Oct 2026 18:43:03 +0400 Subject: [PATCH 08/12] Reject true as an exception's issue number [[rule.exception]] issue accepted TOML's `true`, which Python reads as a bool and so as the int 1, although the loader's message says the field must be a positive issue number. It now uses the same _issue_number check as [rule.downgrade] issue, which rejects a bool. Found in the self-review; the behaviour predates this branch. Refs #912 Co-Authored-By: Claude Opus 5.5 --- scripts/rules/load.py | 2 +- scripts/rules/tests/test_load.py | 30 ++++++++++++++++++++++++++++++ 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/scripts/rules/load.py b/scripts/rules/load.py index 6e6bb5716..69406c0b3 100644 --- a/scripts/rules/load.py +++ b/scripts/rules/load.py @@ -336,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", diff --git a/scripts/rules/tests/test_load.py b/scripts/rules/tests/test_load.py index 47aaf1d1f..931ecc2da 100644 --- a/scripts/rules/tests/test_load.py +++ b/scripts/rules/tests/test_load.py @@ -200,6 +200,36 @@ def test_single_tables_stand_for_one_entry_lists(self) -> None: ) +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.""" From 10764cd0d70d80917e1aa3d1be596abf779f76ef Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Mon, 26 Oct 2026 18:43:56 +0400 Subject: [PATCH 09/12] Name the token variable gh used when a lookup fails The fix for a failed tracking-issue lookup always told the reader to check GH_TOKEN, but gh also reads GITHUB_TOKEN, and the guard looks the issue up when only that one is set. The fix now names the variable the token came from, the first of GH_TOKEN and GITHUB_TOKEN that is set, as gh picks it. The rules-registry guide's commands paragraph said only that the commands need nothing beyond the standard library; it now also says that the guard runs gh api for a [rule.downgrade]. Found in the self-review. Refs #912 Co-Authored-By: Claude Opus 5.5 --- docs/engineering/skills/rules-registry.md | 3 ++- scripts/rules/tests/test_tracking_issues.py | 16 ++++++++++++++++ scripts/rules/tracking_issues.py | 19 +++++++++++++------ 3 files changed, 31 insertions(+), 7 deletions(-) diff --git a/docs/engineering/skills/rules-registry.md b/docs/engineering/skills/rules-registry.md index 9c0fada40..a11a9681b 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 diff --git a/scripts/rules/tests/test_tracking_issues.py b/scripts/rules/tests/test_tracking_issues.py index 787f90f8f..a0d588995 100644 --- a/scripts/rules/tests/test_tracking_issues.py +++ b/scripts/rules/tests/test_tracking_issues.py @@ -210,6 +210,22 @@ def test_a_failed_lookup_fails(self) -> None: ], ) + 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() diff --git a/scripts/rules/tracking_issues.py b/scripts/rules/tracking_issues.py index 83dbce2f6..fd41e2899 100644 --- a/scripts/rules/tracking_issues.py +++ b/scripts/rules/tracking_issues.py @@ -57,9 +57,13 @@ def _load_lookup() -> ModuleType: def _check_issue( - lookup: ModuleType, repository: str, rule: Rule, issue: int + 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.""" + """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: @@ -84,8 +88,8 @@ def _check_issue( rule.id, CHECK, f"couldn't check that issue #{issue} is open: {problem}", - "run the guard again; if it fails again, check the token in GH_TOKEN, " - f"which needs read access to the issues of {repository}", + 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 @@ -103,7 +107,10 @@ def check_tracking_issues(registry: Registry) -> tuple[list[Violation], list[str repository = registry.issue_repository if not downgraded or not repository: return [], [] - if not any(os.environ.get(name) for name in TOKEN_VARIABLES): + 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( @@ -137,7 +144,7 @@ def check_tracking_issues(registry: Registry) -> tuple[list[Violation], list[str violations: list[Violation] = [] notes: list[str] = [] for rule, issue in downgraded: - violation, note = _check_issue(lookup, repository, rule, issue) + violation, note = _check_issue(lookup, repository, token_variable, rule, issue) if violation is not None: violations.append(violation) if note is not None: From 4be0b1cacdc93d354f05bc30fdfb8df69a2af528 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Mon, 26 Oct 2026 18:59:50 +0400 Subject: [PATCH 10/12] Keep the tracking-issue schema tests away from the real gh RegistrySchemaTest runs the whole rules guard on registries that have a [rule.downgrade], but it didn't put the fake gh first on PATH or clear the token variables. With GH_TOKEN or GITHUB_TOKEN set, as in a shell where gh is logged in, test_an_enforced_rule_has_no_downgrade ran gh api for issue 912 in example/repo; a gh on PATH that records its calls showed the call. The class now uses TrackingIssueTestCase, like the other lookup tests, and the same run records no call. Found in the self-review. Refs #912 Co-Authored-By: Claude Opus 5.5 --- scripts/rules/tests/test_tracking_issues.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/scripts/rules/tests/test_tracking_issues.py b/scripts/rules/tests/test_tracking_issues.py index a0d588995..6fc6c4f15 100644 --- a/scripts/rules/tests/test_tracking_issues.py +++ b/scripts/rules/tests/test_tracking_issues.py @@ -392,8 +392,12 @@ def test_the_registry_names_this_repository_for_tracking_issues(self) -> None: self.assertEqual(registry.issue_repository, "PolicyEngine/policyengine-sim-api") -class RegistrySchemaTest(unittest.TestCase): - """Where tracking issues live, and which rules may have a downgrade.""" +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"): From 3905fbd271274e5263d98ba97ab6c01c13570a58 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Mon, 26 Oct 2026 19:02:23 +0400 Subject: [PATCH 11/12] Look tracking issues up only in the repository the workflow runs in The guard took the repository to look a downgrade's issue up in from issue_repository in the registry under review, so a pull request could change that one line to another public repository and name an open issue there. On GitHub Actions the runner sets GITHUB_REPOSITORY, which a pull request can't change; when it is set and differs from issue_repository (ignoring case), the guard now fails before it looks anything up. Outside GitHub Actions the registry's value is used. The test case that isolates the lookup also clears GITHUB_REPOSITORY, which the runner sets in the step that runs the rules tests. Found in the self-review. Refs #912 Co-Authored-By: Claude Opus 5.5 --- docs/engineering/skills/rules-registry.md | 7 +++-- docs/engineering/skills/type-checking.md | 7 +++-- scripts/rules/tests/test_tracking_issues.py | 35 +++++++++++++++++++-- scripts/rules/tracking_issues.py | 15 +++++++++ 4 files changed, 57 insertions(+), 7 deletions(-) diff --git a/docs/engineering/skills/rules-registry.md b/docs/engineering/skills/rules-registry.md index a11a9681b..123929ea6 100644 --- a/docs/engineering/skills/rules-registry.md +++ b/docs/engineering/skills/rules-registry.md @@ -136,8 +136,11 @@ fail the guard yet. `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. Elsewhere, without a token, it - prints a note for each issue it didn't check. Closing the issue while the + 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 rule stays weakened 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; diff --git a/docs/engineering/skills/type-checking.md b/docs/engineering/skills/type-checking.md index 2cd9b2947..55d5ee0ed 100644 --- a/docs/engineering/skills/type-checking.md +++ b/docs/engineering/skills/type-checking.md @@ -130,9 +130,10 @@ with the base branch. It fails when: - 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. 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)"`. + 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: diff --git a/scripts/rules/tests/test_tracking_issues.py b/scripts/rules/tests/test_tracking_issues.py index 6fc6c4f15..b60dd8add 100644 --- a/scripts/rules/tests/test_tracking_issues.py +++ b/scripts/rules/tests/test_tracking_issues.py @@ -83,7 +83,11 @@ def issue_answer( class TrackingIssueTestCase(unittest.TestCase): - """Puts the fake gh first on PATH, outside CI and without a token.""" + """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() @@ -107,7 +111,7 @@ def setUp(self) -> None: ) environment.start() self.addCleanup(environment.stop) - for name in ("GH_TOKEN", "GITHUB_TOKEN", "GITHUB_ACTIONS"): + 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: @@ -287,6 +291,33 @@ def test_the_lookup_must_load(self) -> None: ) 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(), []) diff --git a/scripts/rules/tracking_issues.py b/scripts/rules/tracking_issues.py index fd41e2899..6ad05eca4 100644 --- a/scripts/rules/tracking_issues.py +++ b/scripts/rules/tracking_issues.py @@ -8,6 +8,10 @@ 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 @@ -107,6 +111,17 @@ def check_tracking_issues(registry: Registry) -> tuple[list[Violation], list[str 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 ) From 66a940b3268c84ad61da4f982d992003a9936904 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Mon, 26 Oct 2026 19:02:52 +0400 Subject: [PATCH 12/12] Say what the guard still doesn't check about downgrades The self-review found three statements in the type-checking and rules-registry guides that the code doesn't bear out: - Moving a package to a rule at least as strong passes the package comparison, but a new ratcheted rule with a count above 0 still needs [rule.downgrade]. The guides, and a test comment, said the move passes. - Closing the tracking issue fails the guard only while the [rule.downgrade] section is in the registry. Removing the section from a rule that stays ratcheted or advisory passes, so the issue can then be closed without failing anything. The type-checking guide now lists that under "It doesn't check". - The guard compares rules by id, so removing a rule or replacing it with a weaker rule under a new id produces only notes, apart from Pyright packages and new ratcheted rules. The rules-registry guide now says so. Refs #912 Co-Authored-By: Claude Opus 5.5 --- docs/engineering/skills/rules-registry.md | 20 +++++++++++++------- docs/engineering/skills/type-checking.md | 14 +++++++++----- scripts/rules/tests/test_guard.py | 3 ++- 3 files changed, 24 insertions(+), 13 deletions(-) diff --git a/docs/engineering/skills/rules-registry.md b/docs/engineering/skills/rules-registry.md index 123929ea6..520046671 100644 --- a/docs/engineering/skills/rules-registry.md +++ b/docs/engineering/skills/rules-registry.md @@ -140,18 +140,24 @@ fail the guard yet. `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 - rule stays weakened 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 is for the reviewer. + 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. Moving it to a rule at least as - strong, or deleting it, passes. + 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 55d5ee0ed..1a202c579 100644 --- a/docs/engineering/skills/type-checking.md +++ b/docs/engineering/skills/type-checking.md @@ -125,8 +125,9 @@ with the base branch. It fails when: 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. Moving a package to a rule at least as strong, - or deleting it, passes; + `[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 @@ -143,7 +144,10 @@ It doesn't check: the count and passes; - 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. + 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 @@ -158,8 +162,8 @@ 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 rule -stays weakened fails the guard on every pull request. +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/scripts/rules/tests/test_guard.py b/scripts/rules/tests/test_guard.py index 44c066b30..e83883669 100644 --- a/scripts/rules/tests/test_guard.py +++ b/scripts/rules/tests/test_guard.py @@ -752,7 +752,8 @@ def test_a_package_moved_from_a_ratcheted_to_an_advisory_rule_is_a_downgrade( ) def test_a_package_may_move_between_ratcheted_rules(self) -> None: - # The new rule's baseline count is compared by check_baselines instead. + # 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" )