diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index 60e98ef7f..53fc50f00 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -1,6 +1,12 @@ -# Rule machinery: changes need review from a core developer other than the -# author. Deliberately narrow: pyproject.toml files and workflow files are not -# owned, so dependency bots and everyday CI edits are not blocked. +# Rule machinery: the files that define and check this repository's rules. +# By team convention, a core developer other than the author approves changes +# to them (sim.process.rule-machinery-code-owned). GitHub doesn't enforce +# this: the default branch's ruleset needs one approving review from anyone +# with write access and doesn't require code-owner review. Listing a path here +# only makes GitHub request a review from its owners on pull requests that +# change it and aren't drafts. +# Deliberately narrow: pyproject.toml files and workflow files are not listed, +# so dependency bots and everyday CI edits don't request that review. /rules/ @PolicyEngine/core-developers /scripts/rules/ @PolicyEngine/core-developers /scripts/run_quality_guards.py @PolicyEngine/core-developers diff --git a/AGENTS.md b/AGENTS.md index 12ba40fb5..bd526fa01 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -162,7 +162,7 @@ To add or change a rule, read `docs/engineering/skills/rules-registry.md`. - Checked: `sim.agents.adapters-point-to-agents-md`, `sim.process.pr-tracking-issue`, `sim.process.rules-registry`, `sim.process.stacked-prs-full-ci-at-bottom` - `sim.process.agents-never-bypass`: Agents never bypass branch rules, even with admin credentials: no gh pr merge --admin and no bypass option; a blocked merge is reported to a human. (doc-only, not checked: follow it yourself) -- `sim.process.rule-machinery-code-owned`: Changes to the rule machinery (rules/, scripts/rules/, scripts/run_quality_guards.py and CODEOWNERS) need approval from a core developer other than the author. (doc-only, not checked: follow it yourself) +- `sim.process.rule-machinery-code-owned`: Changes to the rule machinery (rules/, scripts/rules/, scripts/run_quality_guards.py and CODEOWNERS) need approval from a core developer other than the author. This is a team convention that GitHub doesn't enforce: the default branch's ruleset needs one approving review from anyone with write access and doesn't require code-owner review. (doc-only, not checked: follow it yourself) - `sim.process.same-repo-draft-prs`: PRs are opened as drafts from branches in PolicyEngine/policyengine-sim-api, never from forks. (doc-only, not checked: follow it yourself) diff --git a/docs/generated/rules.md b/docs/generated/rules.md index 7f8323b2c..16a12ec59 100644 --- a/docs/generated/rules.md +++ b/docs/generated/rules.md @@ -28,7 +28,7 @@ Every rule that governs `policyengine-sim-api` is listed here with how it is enf - `sim.observability.id-is-diagnostic-only`: observability_id has diagnostic meaning only: never use it for idempotency, authorization, database identity, routing or business logic. - `sim.observability.registered-stages-only`: Span names come from the stage plans in policyengine_simulation_observability.stages; runtime code never adds span-name string literals. - `sim.process.agents-never-bypass`: Agents never bypass branch rules, even with admin credentials: no gh pr merge --admin and no bypass option; a blocked merge is reported to a human. -- `sim.process.rule-machinery-code-owned`: Changes to the rule machinery (rules/, scripts/rules/, scripts/run_quality_guards.py and CODEOWNERS) need approval from a core developer other than the author. +- `sim.process.rule-machinery-code-owned`: Changes to the rule machinery (rules/, scripts/rules/, scripts/run_quality_guards.py and CODEOWNERS) need approval from a core developer other than the author. This is a team convention that GitHub doesn't enforce: the default branch's ruleset needs one approving review from anyone with write access and doesn't require code-owner review. - `sim.process.same-repo-draft-prs`: PRs are opened as drafts from branches in PolicyEngine/policyengine-sim-api, never from forks. - `sim.quality.every-project-tested`: Every project and library with unit tests runs them in the PR test matrix. - `sim.quality.ruff-format-tests`: Test code is formatted with ruff format too; CI currently checks only src directories. @@ -433,7 +433,7 @@ flowchart LR | `sim.agents.adapters-point-to-agents-md` | CLAUDE.md and .github/copilot-instructions.md only point to AGENTS.md; skills and rules are listed in AGENTS.md alone. | mechanical | enforced | yes | quality guard `rules-registry` | `pr.yml:quality-guards` | yes | | `sim.process.agents-never-bypass` | Agents never bypass branch rules, even with admin credentials: no gh pr merge --admin and no bypass option; a blocked merge is reported to a human. | mechanical | doc-only | no | nothing | — | — | | `sim.process.pr-tracking-issue` | The first line of every PR's description is Fixes #ISSUE_NUMBER, naming an open issue, not a pull request, in this repository; automated PRs open or reuse such an issue. | mechanical | enforced | yes | CI job `pr-description` | `pr-description.yml:pr-description` | yes | -| `sim.process.rule-machinery-code-owned` | Changes to the rule machinery (rules/, scripts/rules/, scripts/run_quality_guards.py and CODEOWNERS) need approval from a core developer other than the author. | mechanical | doc-only | no | nothing | — | — | +| `sim.process.rule-machinery-code-owned` | Changes to the rule machinery (rules/, scripts/rules/, scripts/run_quality_guards.py and CODEOWNERS) need approval from a core developer other than the author. This is a team convention that GitHub doesn't enforce: the default branch's ruleset needs one approving review from anyone with write access and doesn't require code-owner review. | mechanical | doc-only | no | nothing | — | — | | `sim.process.rules-registry` | Every rule is registered in rules/registry.toml, is enforced where it claims to be, and its rendered views are current. | mechanical | enforced | yes | quality guard `rules-registry` | `pr.yml:quality-guards` | **unproven** | | `sim.process.same-repo-draft-prs` | PRs are opened as drafts from branches in PolicyEngine/policyengine-sim-api, never from forks. | mechanical | doc-only | no | nothing | — | — | | `sim.process.stacked-prs-full-ci-at-bottom` | In a stack of pull requests, only the lowest unmerged one runs the full PR workflow and the Modal image smoke. Every pr.yml job is needed by the CI complete gate, which fails on the pull requests above it and otherwise passes only when every job passed, so a stack merges bottom-up and nothing merges untested. | mechanical | enforced | yes | pytest `projects/policyengine-simulation-executor/tests/test_ci_complete_gate.py` | `pr.yml:test`, `pr.yml:ci-complete` | yes | diff --git a/rules/registry.toml b/rules/registry.toml index c3fced421..e4dff74ae 100644 --- a/rules/registry.toml +++ b/rules/registry.toml @@ -1460,7 +1460,7 @@ test = "tests/test_ci_complete_gate.py::test_rule_proof_gate_check" [[rule]] id = "sim.process.rule-machinery-code-owned" category = "process" -statement = "Changes to the rule machinery (rules/, scripts/rules/, scripts/run_quality_guards.py and CODEOWNERS) need approval from a core developer other than the author." +statement = "Changes to the rule machinery (rules/, scripts/rules/, scripts/run_quality_guards.py and CODEOWNERS) need approval from a core developer other than the author. This is a team convention that GitHub doesn't enforce: the default branch's ruleset needs one approving review from anyone with write access and doesn't require code-owner review." rationale = ".github/CODEOWNERS" scope = [".github/CODEOWNERS"] tier = "mechanical" diff --git a/scripts/rules/tests/test_code_owner_rule.py b/scripts/rules/tests/test_code_owner_rule.py new file mode 100644 index 000000000..25fdb01b5 --- /dev/null +++ b/scripts/rules/tests/test_code_owner_rule.py @@ -0,0 +1,56 @@ +"""Tests that the code-owner rule says what GitHub enforces when merging. + +The default branch's ruleset needs one approving review from anyone with write +access and doesn't require code-owner review, so a core developer's approval of +rule changes is a team convention, and the rule's statement has to say so. +""" + +from __future__ import annotations + +import unittest +from pathlib import Path + +from scripts.rules.load import load_registry +from scripts.rules.model import Rule, Status + +REPO_ROOT = Path(__file__).resolve().parents[3] +CODE_OWNER_RULE_ID = "sim.process.rule-machinery-code-owned" +REQUIRED_APPROVAL = ( + "Changes to the rule machinery (rules/, scripts/rules/, " + "scripts/run_quality_guards.py and CODEOWNERS) need approval from a core " + "developer other than the author." +) + + +class CodeOwnerRuleTest(unittest.TestCase): + """The rule keeps its requirement and says GitHub doesn't enforce it.""" + + def code_owner_rule(self) -> Rule: + """Return the code-owner rule from the repository's own registry.""" + registry, violations = load_registry(REPO_ROOT) + if registry is None: + self.fail(f"rules/registry.toml doesn't load: {violations}") + rule = registry.rule(CODE_OWNER_RULE_ID) + if rule is None: + self.fail(f"{CODE_OWNER_RULE_ID} isn't in rules/registry.toml") + return rule + + def test_rule_still_asks_for_a_core_developer_approval(self) -> None: + """The statement still starts with the approval the rule asks for.""" + statement = self.code_owner_rule().statement + self.assertTrue(statement.startswith(REQUIRED_APPROVAL), statement) + + def test_rule_stays_doc_only(self) -> None: + """No check enforces the rule, so it stays doc-only.""" + self.assertIs(self.code_owner_rule().status, Status.DOC_ONLY) + + def test_rule_says_github_does_not_require_code_owner_review(self) -> None: + """The statement says what the ruleset needs, and that it's a convention.""" + statement = self.code_owner_rule().statement + self.assertIn("team convention that GitHub doesn't enforce", statement) + self.assertIn("one approving review from anyone with write access", statement) + self.assertIn("doesn't require code-owner review", statement) + + +if __name__ == "__main__": + unittest.main()