diff --git a/.github/workflows/agent_governance.yml b/.github/workflows/agent_governance.yml index 1422a0792aa..23de46485e2 100644 --- a/.github/workflows/agent_governance.yml +++ b/.github/workflows/agent_governance.yml @@ -2,7 +2,7 @@ name: Agent Governance on: pull_request: - types: [opened, synchronize, reopened, edited, ready_for_review] + types: [opened, synchronize, reopened, edited, ready_for_review, labeled, unlabeled] permissions: contents: read diff --git a/AGENTS.md b/AGENTS.md index 09be5bcf8fb..56606c2ff40 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -158,7 +158,9 @@ When splitting a function into a new `.cpp`, if its callers span LCAO and PW, pu - Semantic questions such as module ownership, member-variable workflow state, test sufficiency, and exception approval require human review. - Exceptions must be recorded in the PR with reason, scope, risk, and a follow-up - cleanup plan. + cleanup plan, using the `### Governance Exception` template. CI downgrades a + covered blocker to a warning only after a maintainer applies the + `governance-exception-approved` label. - After a refactor, propose brief lessons worth recording in this file, then ask the developer whether to write them in; be cautious and skip unclear or unverified lessons. diff --git a/docs/developers_guide/agent_governance.md b/docs/developers_guide/agent_governance.md index 9f33db289ad..53e911eafe0 100644 --- a/docs/developers_guide/agent_governance.md +++ b/docs/developers_guide/agent_governance.md @@ -86,7 +86,7 @@ decisions. | Header include guard | New `#pragma once` in a header | phase-one mechanical | hook + CI | high | block | added header lines (`.h`/`.hh`/`.hpp`/`.hxx`/`.cuh`) | Use `#ifndef`/`#define`/`#endif` guards; no exception allowed | | Language baseline | C++11 compatibility | build/toolchain | CI | high | block | build/static tooling | Actual compiler/toolchain result wins | | Global dependency budget | Net increase of `GlobalV`/`GlobalC`/`PARAM` references in code diff | phase-one mechanical + AI review | CI + AI review | high | block on net increase, warn on non-increasing added usage | added and removed code lines | Historical untouched usage and documentation mentions are not blocked; migration-neutral moves require reviewer rationale | -| New default parameter | Header declaration adds a default argument | phase-one mechanical + AI review | CI + AI review | high | block | header diff | High misuse risk | +| New default parameter | Header declaration adds a default argument | phase-one mechanical warning + AI review | CI + AI review | high | warn | header diff | High misuse risk; the single-line heuristic is not reliable enough to block, so AI and human review confirm | | `.hpp` propagation | New `.hpp` or header includes `.hpp` | phase-one mechanical warning | CI + AI review | medium | warn | new files and added includes | Exception can be recorded in PR | | Header dependency growth | Header diff adds includes | phase-one mechanical warning + AI review | CI + AI review | medium | warn | added header includes | Necessity is semantic and not mechanically decided | | Member variable workflow switch | Key flow state hidden as mutable member state | AI review + human confirmation | AI + human review | high | human confirmation | semantic review | Static matching is unreliable | @@ -101,7 +101,7 @@ decisions. | Documentation sync | Behavior/interface docs updated | phase-one mechanical warning + AI review | CI + AI review | medium | warn | changed paths and PR body | Major behavior changes escalate to reviewers | | PR metadata completeness | Issue, tests, behavior, INPUT, core impact, exceptions | phase-one mechanical | CI or GitHub bot | medium | warn | PR template fields | Not run by local hook | | AI workflow | Interface lookup, uncertainty, verification report | AI review | AI review | high | warn | review transcript/output | Applies to AI agents | -| Exceptions | Reason, scope, risk, follow-up plan | human confirmation | human review + CI | high | human confirmation | PR exception section | CI checks presence, not approval | +| Exceptions | Reason, scope, risk, follow-up plan | human confirmation | human review + CI | high | human confirmation | PR exception section + approval label | CI downgrades a covered blocker to a warning only when the section is complete and a maintainer applied the `governance-exception-approved` label | Implementations must use this matrix as the default baseline. Upgrading warnings to blockers or converting human-confirmation rules into mechanical blockers @@ -269,6 +269,22 @@ Use this template in the PR when a rule must be bypassed temporarily: - Requested approver: ``` +`Rule` names the checker rule(s) being excepted, using the exact rule names +from the governance summary (for example `Global dependency budget` or +`CMake linkage for new sources`). The CI checker handles the section as follows: + +- An incomplete section (any field empty) produces a warning and changes + nothing else. +- A complete section without approval produces a warning that the exception is + awaiting approval; blocking findings still fail the check. +- A complete section plus the `governance-exception-approved` label, applied by + a maintainer other than the PR author, downgrades the blocking findings whose + rule appears in `Rule` to warnings. Findings marked `Exception allowed: no` + (LF line endings, `#pragma once`) are never downgraded. + +Applying or removing the label re-runs the check. The local `--staged` hook has +no PR body or labels and therefore never applies exceptions. + ## AI Review Finding Format AI and bot review findings should use this shape: diff --git a/tools/03_code_analysis/agent_governance_check.py b/tools/03_code_analysis/agent_governance_check.py index f84103a7137..fc56608b4bb 100644 --- a/tools/03_code_analysis/agent_governance_check.py +++ b/tools/03_code_analysis/agent_governance_check.py @@ -47,6 +47,18 @@ TEST_NAME_PATTERNS = ("test", "tests", "unittest", "pytest", "ctest", "case") HETEROGENEOUS_MARKERS = ("/cuda/", "/rocm/", "/kernels/", "cuda/", "rocm/", "kernels/") PR_SECTION_RE = re.compile(r"^###\s+(.+?)\s*$", re.MULTILINE) +EXCEPTION_SECTION = "Governance Exception" +EXCEPTION_APPROVED_LABEL = "governance-exception-approved" +EXCEPTION_FIELDS = ( + "Rule", + "Reason", + "Scope", + "User or maintenance risk", + "Why the normal rule cannot be followed now", + "Follow-up cleanup plan", + "Requested approver", +) +EXCEPTION_FIELD_RE = re.compile(r"^[-*]\s*([^:]+):\s*(.*)$") @dataclass @@ -578,7 +590,7 @@ def check_cmake_linkage(findings: List[Finding], statuses: Dict[str, str], chang -def read_pr_body(event_path: Optional[str]) -> Optional[str]: +def read_pull_request(event_path: Optional[str]) -> Optional[dict]: if not event_path: return None try: @@ -588,12 +600,31 @@ def read_pr_body(event_path: Optional[str]) -> Optional[str]: return None if "pull_request" not in payload or not isinstance(payload["pull_request"], dict): return None - body = payload["pull_request"].get("body") + return payload["pull_request"] + + +def read_pr_body(event_path: Optional[str]) -> Optional[str]: + pull_request = read_pull_request(event_path) + if pull_request is None: + return None + body = pull_request.get("body") if body is None: return "" return str(body) +def read_pr_labels(event_path: Optional[str]) -> List[str]: + pull_request = read_pull_request(event_path) + if pull_request is None: + return [] + labels = pull_request.get("labels") or [] + return [ + str(label.get("name", "")) + for label in labels + if isinstance(label, dict) + ] + + def pr_sections(body: str) -> Dict[str, str]: matches = list(PR_SECTION_RE.finditer(body)) sections: Dict[str, str] = {} @@ -751,11 +782,84 @@ def check_documentation_warning(findings: List[Finding], changed: Sequence[str], ) +def parse_governance_exception(body: str) -> Optional[Dict[str, str]]: + sections = pr_sections(body) + content = sections.get(EXCEPTION_SECTION) + if content is None: + return None + fields: Dict[str, str] = {} + for raw in content.splitlines(): + match = EXCEPTION_FIELD_RE.match(raw.strip()) + if match: + fields[match.group(1).strip().lower()] = match.group(2).strip() + return fields + + +def apply_governance_exception( + findings: List[Finding], + body: Optional[str], + labels: Sequence[str], +) -> None: + """Downgrade blocking findings covered by an approved Governance Exception. + + The PR body records the exception; the maintainer-applied label is the + approval. Findings with allow_exception=False are never downgraded. + """ + if body is None: + return + approved = EXCEPTION_APPROVED_LABEL in labels + fields = parse_governance_exception(body) + if fields is None or not any(fields.values()): + if approved: + add_finding( + findings, + "Governance exception record", + WARN, + "pull_request.body", + None, + f"PR carries the `{EXCEPTION_APPROVED_LABEL}` label but has no filled `### {EXCEPTION_SECTION}` section.", + "Record the exception with the template in docs/developers_guide/agent_governance.md, or remove the label.", + ) + return + missing = [field for field in EXCEPTION_FIELDS if not fields.get(field.lower())] + if missing: + add_finding( + findings, + "Governance exception record", + WARN, + "pull_request.body", + None, + f"`### {EXCEPTION_SECTION}` section is incomplete; missing fields: " + ", ".join(missing) + ".", + "Fill every field of the exception template; incomplete exceptions do not downgrade blocking findings.", + ) + return + if not approved: + add_finding( + findings, + "Governance exception record", + WARN, + "pull_request.body", + None, + f"Governance exception requested for: {fields['rule']}; awaiting maintainer approval.", + f"A maintainer approves the exception by applying the `{EXCEPTION_APPROVED_LABEL}` label.", + ) + return + excepted_rules = fields["rule"].lower() + for finding in findings: + if finding.severity != BLOCK or not finding.allow_exception: + continue + if finding.rule.lower() not in excepted_rules: + continue + finding.severity = WARN + finding.reason += " Downgraded from error by the approved governance exception." + + def collect_findings(root: Path, args: argparse.Namespace) -> List[Finding]: findings: List[Finding] = [] statuses, changed = changed_paths(root, args) lines, removed_lines = changed_lines(root, args) body = read_pr_body(args.event_path) + labels = read_pr_labels(args.event_path) body_text = body or "" check_line_endings(findings, root, changed, statuses, args) @@ -770,6 +874,7 @@ def collect_findings(root: Path, args: argparse.Namespace) -> List[Finding]: check_test_evidence_warning(findings, changed, body_text) check_heterogeneous_test_warning(findings, changed, body_text) check_documentation_warning(findings, changed, body_text) + apply_governance_exception(findings, body, labels) return findings diff --git a/tools/03_code_analysis/test_agent_governance_check.py b/tools/03_code_analysis/test_agent_governance_check.py index cf7304a44d6..ef68e530f65 100644 --- a/tools/03_code_analysis/test_agent_governance_check.py +++ b/tools/03_code_analysis/test_agent_governance_check.py @@ -623,6 +623,100 @@ def test_allows_existing_pragma_once_on_unchanged_lines(self): self.assertNotIn("No #pragma once", result.stdout) + def write_exception_event(self, rule, labels, approver="@maintainer"): + event = self.repo / "event.json" + body = "\n".join( + [ + "### Governance Exception", + f"- Rule: {rule}", + "- Reason: file is consumed by a generated build list.", + "- Scope: source/source_base/generated.cpp only.", + "- User or maintenance risk: low.", + "- Why the normal rule cannot be followed now: generator lands in a follow-up PR.", + "- Follow-up cleanup plan: wire the file through CMakeLists.txt in the follow-up.", + f"- Requested approver: {approver}", + ] + ) + event.write_text( + json.dumps( + {"pull_request": {"body": body, "labels": [{"name": name} for name in labels]}} + ) + ) + return event + + def test_approved_exception_downgrades_covered_blocker(self): + self.write("source/source_base/generated.cpp", "int generated();\n") + head = self.commit_change() + event = self.write_exception_event( + "CMake linkage for new sources", ["governance-exception-approved"] + ) + + result = self.run_checker("--base", self.base, "--head", head, "--event-path", str(event)) + + self.assert_warns_with_success(result, "CMake linkage for new sources") + self.assertIn("Downgraded from error by the approved governance exception", result.stdout) + + def test_unapproved_exception_keeps_blocker(self): + self.write("source/source_base/generated.cpp", "int generated();\n") + head = self.commit_change() + event = self.write_exception_event("CMake linkage for new sources", []) + + result = self.run_checker("--base", self.base, "--head", head, "--event-path", str(event)) + + self.assert_blocked_by(result, "CMake linkage for new sources") + self.assertIn("awaiting maintainer approval", result.stdout) + + def test_incomplete_exception_keeps_blocker_even_with_label(self): + self.write("source/source_base/generated.cpp", "int generated();\n") + head = self.commit_change() + event = self.write_exception_event( + "CMake linkage for new sources", ["governance-exception-approved"], approver="" + ) + + result = self.run_checker("--base", self.base, "--head", head, "--event-path", str(event)) + + self.assert_blocked_by(result, "CMake linkage for new sources") + self.assertIn("missing fields: Requested approver", result.stdout) + + def test_approved_exception_does_not_downgrade_other_rules(self): + self.write("source/source_base/generated.cpp", "int generated();\n") + head = self.commit_change() + event = self.write_exception_event( + "Global dependency budget", ["governance-exception-approved"] + ) + + result = self.run_checker("--base", self.base, "--head", head, "--event-path", str(event)) + + self.assert_blocked_by(result, "CMake linkage for new sources") + self.assertNotIn("Downgraded from error", result.stdout) + + def test_approved_exception_never_downgrades_non_exceptable_rule(self): + self.write("source/source_base/new_header.h", "#pragma once\n\nint foo();\n") + head = self.commit_change() + event = self.write_exception_event("No #pragma once", ["governance-exception-approved"]) + + result = self.run_checker("--base", self.base, "--head", head, "--event-path", str(event)) + + self.assert_blocked_by(result, "No #pragma once") + self.assertNotIn("Downgraded from error", result.stdout) + + def test_warns_when_approval_label_has_no_exception_section(self): + event = self.repo / "event.json" + event.write_text( + json.dumps( + { + "pull_request": { + "body": "### What's changed?\n- Docs tweak.\n", + "labels": [{"name": "governance-exception-approved"}], + } + } + ) + ) + + result = self.run_checker("--event-path", str(event)) + + self.assert_warns_with_success(result, "Governance exception record") + if __name__ == "__main__": unittest.main()