diff --git a/.coderabbit.yaml b/.coderabbit.yaml index 8f6bb860612..e46e6feb42b 100644 --- a/.coderabbit.yaml +++ b/.coderabbit.yaml @@ -15,6 +15,10 @@ reviews: Focus on newly introduced GlobalV/GlobalC/PARAM dependencies, default parameters in headers, module placement, CMakeLists.txt linkage, C++11 compatibility, and focused tests for behavior changes. + During the legacy global-state migration period, treat a net increase in + GlobalV/GlobalC/PARAM code references as blocking, and treat + migration-neutral added usage as reviewer-visible warnings requiring + reason, scope, risk, and cleanup rationale. - path: "source/source_io/module_parameter/**" instructions: | Treat INPUT parameter metadata, parsing, defaults, descriptions, and diff --git a/.github/instructions/abacus-governance.instructions.md b/.github/instructions/abacus-governance.instructions.md index e1585ec9761..6e0e09df09e 100644 --- a/.github/instructions/abacus-governance.instructions.md +++ b/.github/instructions/abacus-governance.instructions.md @@ -12,7 +12,9 @@ Apply these instructions when reviewing or changing ABACUS code: newly introduced symbols, and changed text files for line-ending checks. - Do not treat untouched historical debt as a default blocker. Mention it only when it affects the changed area, and label it as advisory. -- Flag newly introduced `GlobalV`, `GlobalC`, or `PARAM` cross-layer control. +- Flag PRs that increase `GlobalV`, `GlobalC`, or `PARAM` code references as + blocker-level governance issues. Flag migration-neutral added usage as a + warning that requires reason, scope, risk, and cleanup/follow-up rationale. Prefer explicit dependencies or narrow local interfaces. - Flag new default arguments in existing header interfaces. Prefer explicit call-site updates, overloads, or a clearer configuration object. diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index 4d0cdf52dcf..c10bb1e903a 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -22,7 +22,7 @@ Fix #... - Example: My changes might affect the performance of the application under certain conditions, and I have tested the impact on various scenarios... ### Governance Checklist -- Global dependencies: no new `GlobalV`, `GlobalC`, or `PARAM` cross-layer control, or exception requested below. +- Global dependencies: no net increase in `GlobalV`, `GlobalC`, or `PARAM` code references, or exception requested below with reason, scope, risk, and cleanup plan. - Default parameters: no new default arguments added to existing interfaces, or exception requested below. - Headers: no unnecessary header dependencies or `.hpp` propagation, or rationale provided below. - Line endings: text files use LF; only `.bat` and `.cmd` use CRLF. diff --git a/.github/workflows/agent_governance.yml b/.github/workflows/agent_governance.yml index 2dc36d49f4c..ecc96e17d89 100644 --- a/.github/workflows/agent_governance.yml +++ b/.github/workflows/agent_governance.yml @@ -7,6 +7,7 @@ on: permissions: contents: read pull-requests: read + issues: write jobs: governance: @@ -53,3 +54,40 @@ jobs: if: always() run: | cat agent_governance_summary.md >> "$GITHUB_STEP_SUMMARY" + + - name: Comment governance warnings + if: >- + always() && + github.event.pull_request.head.repo.full_name == github.repository + env: + GH_TOKEN: ${{ github.token }} + PR_NUMBER: ${{ github.event.pull_request.number }} + run: | + if ! grep -qi '^| warning |' agent_governance_summary.md; then + exit 0 + fi + + marker='' + body_file="$(mktemp)" + json_file="$(mktemp)" + { + echo "$marker" + echo + cat agent_governance_summary.md + } > "$body_file" + jq -Rs '{body: .}' < "$body_file" > "$json_file" + + existing_comment_id="$(gh api --paginate \ + "repos/${GITHUB_REPOSITORY}/issues/${PR_NUMBER}/comments" \ + --jq ".[] | select(.body | contains(\"${marker}\")) | .id" \ + | head -n 1)" + + if [ -n "$existing_comment_id" ]; then + gh api --method PATCH \ + "repos/${GITHUB_REPOSITORY}/issues/comments/${existing_comment_id}" \ + --input "$json_file" + else + gh api --method POST \ + "repos/${GITHUB_REPOSITORY}/issues/${PR_NUMBER}/comments" \ + --input "$json_file" + fi diff --git a/AGENTS.md b/AGENTS.md index 07a41911f9a..e95e177691f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -9,8 +9,10 @@ rules. Read the complete governance document before making or reviewing changes: ## Required Baseline - Follow the seven ABACUS coding rules summarized from the project governance: - 1. Do not introduce new cross-layer control through `GlobalV`, `GlobalC`, or - `PARAM`; pass dependencies explicitly. + 1. Do not increase cross-layer control through `GlobalV`, `GlobalC`, or + `PARAM`; pass dependencies explicitly where practical. Migration-neutral + moves must keep the PR-level global dependency budget non-increasing and + explain the remaining global usage. 2. Do not hide workflow switches in mutable member variables that can be changed from multiple places. 3. Keep header dependencies minimal. diff --git a/docs/developers_guide/agent_governance.md b/docs/developers_guide/agent_governance.md index 5ff4f75ca44..874032584d2 100644 --- a/docs/developers_guide/agent_governance.md +++ b/docs/developers_guide/agent_governance.md @@ -69,7 +69,7 @@ decisions. | --- | --- | --- | --- | --- | --- | --- | --- | | Basic text format | LF line endings | phase-one mechanical | hook + CI | medium | block | full changed text file | `.bat` and `.cmd` keep CRLF | | Language baseline | C++11 compatibility | build/toolchain | CI | high | block | build/static tooling | Actual compiler/toolchain result wins | -| New global dependency | Added `GlobalV`/`GlobalC`/`PARAM` as cross-layer control | phase-one mechanical + AI review | CI + AI review | high | block | added code lines | Historical untouched usage and documentation mentions are not blocked | +| 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 | | `.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 | @@ -97,6 +97,13 @@ explicit governance change. For header include warnings, the rationale should state whether the header needs a complete type, for example because it owns a value member rather than a pointer or reference. +For global dependencies, the mechanical checker uses a PR-level budget during +the legacy migration period. A PR blocks only when the number of code references +to `GlobalV`, `GlobalC`, or `PARAM` increases after accounting for deleted +references. If a PR adds global references while deleting at least as many +elsewhere, the checker warns instead of blocking; reviewers should confirm the +change is a migration-neutral move or part of a cleanup path. + ## Automation Responsibilities Local hooks: diff --git a/tools/03_code_analysis/agent_governance_check.py b/tools/03_code_analysis/agent_governance_check.py index 8ef4f1fc5eb..b9f05010a7d 100644 --- a/tools/03_code_analysis/agent_governance_check.py +++ b/tools/03_code_analysis/agent_governance_check.py @@ -120,42 +120,52 @@ def changed_paths(root: Path, args: argparse.Namespace) -> Tuple[Dict[str, str], return parse_name_status(output) -def parse_added_lines(diff_text: str) -> List[DiffLine]: - lines: List[DiffLine] = [] - path = "" +def parse_changed_lines(diff_text: str) -> Tuple[List[DiffLine], List[DiffLine]]: + added: List[DiffLine] = [] + removed: List[DiffLine] = [] + old_path = "" + new_path = "" + old_line: Optional[int] = None new_line: Optional[int] = None - hunk_re = re.compile(r"@@ -\d+(?:,\d+)? \+(\d+)(?:,(\d+))? @@") + hunk_re = re.compile(r"@@ -(\d+)(?:,\d+)? \+(\d+)(?:,\d+)? @@") for raw in diff_text.splitlines(): + if raw.startswith("--- a/"): + old_path = raw[6:] + continue if raw.startswith("+++ b/"): - path = raw[6:] + new_path = raw[6:] continue - if raw.startswith("+++ "): - path = raw[4:] + if raw.startswith("--- ") or raw.startswith("+++ "): continue match = hunk_re.match(raw) if match: - new_line = int(match.group(1)) + old_line = int(match.group(1)) + new_line = int(match.group(2)) + continue + if old_line is None or new_line is None: continue - if new_line is None: + if raw.startswith("\\"): continue if raw.startswith("+") and not raw.startswith("+++"): - lines.append(DiffLine(path, new_line, raw[1:])) + added.append(DiffLine(new_path, new_line, raw[1:])) new_line += 1 elif raw.startswith("-") and not raw.startswith("---"): - continue + removed.append(DiffLine(old_path, old_line, raw[1:])) + old_line += 1 else: + old_line += 1 new_line += 1 - return lines + return added, removed -def added_lines(root: Path, args: argparse.Namespace) -> List[DiffLine]: +def changed_lines(root: Path, args: argparse.Namespace) -> Tuple[List[DiffLine], List[DiffLine]]: if args.staged: output = git(["diff", "--cached", "--ignore-cr-at-eol", "-U0"], root).stdout elif args.base and args.head: output = git(["diff", "--ignore-cr-at-eol", "-U0", args.base, args.head], root).stdout else: output = "" - return parse_added_lines(output) + return parse_changed_lines(output) def read_changed_file_bytes(root: Path, path: str, args: argparse.Namespace) -> bytes: @@ -232,22 +242,57 @@ def check_line_endings( ) -def check_global_dependencies(findings: List[Finding], lines: Iterable[DiffLine]) -> None: - pattern = re.compile(r"\b(GlobalV::|GlobalC::|PARAM(?:\.|->|::|\b))") +GLOBAL_DEPENDENCY_RE = re.compile(r"\b(GlobalV::|GlobalC::|PARAM(?:\.|->|::|\b))") + + +def is_global_dependency_check_path(path: str) -> bool: + if path.startswith("tools/03_code_analysis/"): + return False + return Path(path).suffix.lower() in CODE_EXTENSIONS + + +def global_dependency_hits(lines: Iterable[DiffLine]) -> List[Tuple[DiffLine, int]]: + hits: List[Tuple[DiffLine, int]] = [] for line in lines: - if line.path.startswith("tools/03_code_analysis/"): + if not is_global_dependency_check_path(line.path): continue - if Path(line.path).suffix.lower() not in CODE_EXTENSIONS: - continue - if pattern.search(line.content): - add_finding( + count = len(GLOBAL_DEPENDENCY_RE.findall(line.content)) + if count: + hits.append((line, count)) + return hits + + +def check_global_dependencies( + findings: List[Finding], + added_lines: Iterable[DiffLine], + removed_lines: Iterable[DiffLine], +) -> None: + added_hits = global_dependency_hits(added_lines) + removed_hits = global_dependency_hits(removed_lines) + added_count = sum(count for _, count in added_hits) + removed_count = sum(count for _, count in removed_hits) + delta = added_count - removed_count + if added_count == 0: + return + + severity = BLOCK if delta > 0 else WARN + action = ( + "Reduce or explicitly pass dependencies so this PR does not increase global dependency usage." + if delta > 0 + else "Confirm this is a migration-neutral move or partial cleanup, and explain the remaining global dependency rationale." + ) + for line, count in added_hits: + add_finding( findings, - "No new cross-layer globals", - WARN, + "Global dependency budget", + severity, line.path, line.line, - "Added line introduces GlobalV, GlobalC, or PARAM as a dependency.", - "Prefer explicit parameters or a narrow local interface. Document any required exception in the PR.", + ( + f"Added line introduces {count} GlobalV/GlobalC/PARAM reference(s); " + f"PR total added={added_count}, removed={removed_count}, net_delta={delta}." + ), + action, ) @@ -518,7 +563,7 @@ def check_pr_metadata(findings: List[Finding], body: Optional[str]) -> None: add_finding( findings, "PR metadata completeness", - WARN, + BLOCK, "pull_request.body", None, "; ".join(reason_parts), @@ -621,12 +666,12 @@ def check_documentation_warning(findings: List[Finding], changed: Sequence[str], def collect_findings(root: Path, args: argparse.Namespace) -> List[Finding]: findings: List[Finding] = [] statuses, changed = changed_paths(root, args) - lines = added_lines(root, args) + lines, removed_lines = changed_lines(root, args) body = read_pr_body(args.event_path) body_text = body or "" check_line_endings(findings, root, changed, statuses, args) - check_global_dependencies(findings, lines) + check_global_dependencies(findings, lines, removed_lines) check_default_parameters(findings, lines) check_hpp_warnings(findings, statuses, lines) check_header_include_warnings(findings, lines) diff --git a/tools/03_code_analysis/test_agent_governance_check.py b/tools/03_code_analysis/test_agent_governance_check.py index a62273127d6..b60957e42ea 100644 --- a/tools/03_code_analysis/test_agent_governance_check.py +++ b/tools/03_code_analysis/test_agent_governance_check.py @@ -94,14 +94,43 @@ def test_allows_crlf_in_windows_scripts(self): self.assertEqual(result.returncode, 0, result.stdout + result.stderr) - def test_blocks_new_global_dependencies_on_added_lines(self): + def test_blocks_when_global_dependency_budget_increases(self): self.write("source/source_base/global.cpp", "int n = GlobalV::NPROC + PARAM.inp.nbands;\n") self.write("source/source_base/CMakeLists.txt", "add_library(global global.cpp)\n") head = self.commit_change() result = self.run_checker("--base", self.base, "--head", head) - self.assert_blocked_by(result, "No new cross-layer globals") + self.assert_blocked_by(result, "Global dependency budget") + self.assertIn("net_delta=2", result.stdout) + + def test_warns_when_global_dependency_usage_is_rebalanced(self): + self.write("source/source_base/global.cpp", "int old_n = PARAM.inp.nbands;\n") + self.write("source/source_base/CMakeLists.txt", "add_library(global global.cpp)\n") + self.git("add", ".") + self.git("commit", "-m", "add baseline global usage") + base = self.git("rev-parse", "HEAD").stdout.strip() + self.write("source/source_base/global.cpp", "int moved_n = GlobalV::NPROC;\n") + head = self.commit_change() + + result = self.run_checker("--base", base, "--head", head) + + self.assert_warns_with_success(result, "Global dependency budget") + self.assertIn("net_delta=0", result.stdout) + + def test_allows_global_dependency_budget_reduction(self): + self.write("source/source_base/global.cpp", "int old_n = PARAM.inp.nbands;\n") + self.write("source/source_base/CMakeLists.txt", "add_library(global global.cpp)\n") + self.git("add", ".") + self.git("commit", "-m", "add baseline global usage") + base = self.git("rev-parse", "HEAD").stdout.strip() + self.write("source/source_base/global.cpp", "int old_n = 0;\n") + head = self.commit_change() + + result = self.run_checker("--base", base, "--head", head) + + self.assertEqual(result.returncode, 0, result.stdout + result.stderr) + self.assertNotIn("Global dependency budget", result.stdout) def test_allows_global_names_in_documentation(self): self.write("docs/governance-notes.md", "Mention GlobalV::NPROC and PARAM.inp in documentation.\n") @@ -575,14 +604,14 @@ def test_warns_for_heterogeneous_file_without_test_evidence(self): self.assert_warns_with_success(result, "Heterogeneous test evidence review") - def test_staged_mode_checks_index_content(self): + def test_staged_mode_blocks_global_dependency_budget_increase(self): self.write("source/source_base/staged.cpp", "int n = GlobalC::ucell.nat;\n") self.write("source/source_base/CMakeLists.txt", "add_library(staged staged.cpp)\n") self.git("add", ".") result = self.run_checker("--staged") - self.assert_blocked_by(result, "No new cross-layer globals") + self.assert_blocked_by(result, "Global dependency budget") def test_rejects_staged_with_base_head(self): result = self.run_checker("--staged", "--base", self.base, "--head", self.base)