Modify AI agent#7586
Conversation
# Conflicts: # source/source_estate/module_pot/pot_xc_fdm.cpp
mohanchen
added
Compile & CICD & Docs & Dependencies
|
I think, when a warning is generated by governance checker, it's better to emit a message automatically if possible. This would prevent the warning from being completely overlooked while avoiding a false-positive-like failure status. This seems to be supported by GH Action engine, and could be made to comment only once thus avoiding duplication. A possible example (generated by AI but not validated): Detailsdiff --git a/.github/workflows/agent_governance.yml b/.github/workflows/agent_governance.yml
index 2dc36d49f..xxxxxxxxx 100644
--- a/.github/workflows/agent_governance.yml
+++ b/.github/workflows/agent_governance.yml
@@ -8,7 +8,7 @@ on:
permissions:
contents: read
- pull-requests: read
+ pull-requests: write
jobs:
governance:
@@ -55,3 +55,27 @@ 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='<!-- agent-governance-warning -->'
+ comments="$(gh api --paginate \
+ "repos/${GITHUB_REPOSITORY}/issues/${PR_NUMBER}/comments" \
+ --jq '.[].body')"
+
+ if grep -Fq "$marker" <<< "$comments"; then
+ exit 0
+ fi
+
+ {
+ echo "$marker"
+ cat agent_governance_summary.md
+ } | jq -Rs '{body: .}' | gh api --method POST \
+ "repos/${GITHUB_REPOSITORY}/issues/${PR_NUMBER}/comments" \
+ --input - |
QuantumMisaka
left a comment
QuantumMisaka
left a comment
There was a problem hiding this comment.
Overall conclusion: this PR is not yet a complete severity-downgrade PR. It only changes two add_finding(..., BLOCK, ...) calls to WARN, but it does not update the regression tests, governance documentation, PR template, CodeRabbit/Copilot instructions, or CI warning visibility. If the intended policy is "new GlobalV / GlobalC / PARAM usage should be reviewer-visible during the legacy migration period, but should not be a mechanical blocker", please make the downgrade consistently across the governance surface.
I verified the current PR branch locally against the merged governance baseline. Exporting upstream/pr/7586 and running the governance checker tests gives 6 failures out of 36 tests. The failures are concentrated around the two semantics changed here: new global dependencies and PR metadata completeness.
python3 -m unittest discover -s /private/tmp/abacus-pr7586.*/tools/03_code_analysis -p 'test_agent_governance_check.py'
# Ran 36 tests
# FAILED (failures=6)Required change 1: do not downgrade PR metadata completeness in this PR
This PR is about relaxing mechanical blocking for legacy global-state usage. It should not also downgrade PR metadata completeness. Making an empty or placeholder PR body only a warning weakens issue linkage, test evidence, INPUT impact reporting, core-module impact reporting, and exception/risk documentation for every PR.
Please restore this finding to BLOCK:
diff --git a/tools/03_code_analysis/agent_governance_check.py b/tools/03_code_analysis/agent_governance_check.py
@@
add_finding(
findings,
"PR metadata completeness",
- WARN,
+ BLOCK,
"pull_request.body",
None,
"; ".join(reason_parts),
"Fill the PR template with issue linkage, test evidence, behavior impact, governance notes, and exception details.",
allow_exception=False,
)If maintainers want to relax PR metadata enforcement too, that should be a separate governance PR with its own rationale, because it is not the same policy decision as downgrading global dependency findings.
Required change 2: keep the global dependency downgrade, but fix indentation
The intended WARN change is understandable for the migration period, but the current indentation in check_global_dependencies() no longer follows the surrounding style. Please keep the original call formatting:
diff --git a/tools/03_code_analysis/agent_governance_check.py b/tools/03_code_analysis/agent_governance_check.py
@@
if pattern.search(line.content):
add_finding(
- findings,
- "No new cross-layer globals",
- WARN,
- 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.",
- )
+ findings,
+ "No new cross-layer globals",
+ WARN,
+ 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.",
+ )Required change 3: update the regression tests to match the new intended semantics
If No new cross-layer globals becomes a warning, its tests must assert warning-with-success behavior. The PR metadata tests should stay as blocker tests if Required change 1 is applied.
Please update the global dependency tests like this:
diff --git a/tools/03_code_analysis/test_agent_governance_check.py b/tools/03_code_analysis/test_agent_governance_check.py
@@
- def test_blocks_new_global_dependencies_on_added_lines(self):
+ def test_warns_for_new_global_dependencies_on_added_lines(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_warns_with_success(result, "No new cross-layer globals")
@@
- def test_staged_mode_checks_index_content(self):
+ def test_staged_mode_warns_for_index_global_dependencies(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_warns_with_success(result, "No new cross-layer globals")Please keep these PR metadata tests as blockers:
self.assert_blocked_by(result, "PR metadata completeness")Those tests cover empty PR body, missing PR body, and placeholder template sections. They should continue to fail the governance check.
Required change 4: update the governance matrix
docs/developers_guide/agent_governance.md still says new global dependencies are high-severity blockers. If the script is downgraded, this matrix must be downgraded too, otherwise CI, agents, CodeRabbit, and human reviewers will be operating from inconsistent rules.
Please update the matrix row:
diff --git a/docs/developers_guide/agent_governance.md b/docs/developers_guide/agent_governance.md
@@
-| 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 |
+| New global dependency | Added `GlobalV`/`GlobalC`/`PARAM` as cross-layer control | phase-one mechanical warning + AI review | CI + AI review | high | warn | added code lines | Migration-period warning: PR must provide reason, scope, risk, and cleanup/follow-up plan; historical untouched usage and documentation mentions are not blocked |Also add a short policy paragraph after the warning/blocker explanation:
@@
Deterministic warnings require a
reviewable rationale or cleanup, but they do not become blockers without an
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.
+
+During the legacy global-state migration period, newly introduced `GlobalV`,
+`GlobalC`, or `PARAM` dependencies are reported as deterministic warnings rather
+than mechanical blockers. Reviewers should still require the PR to explain why
+explicit dependency passing is not practical in the touched scope, what user or
+maintenance risk remains, and what follow-up migration path is expected.Required change 5: update AGENTS.md and the PR template
AGENTS.md still states "Do not introduce new cross-layer control". That conflicts with a warning-only checker. Please change it to an avoid-and-document rule:
diff --git a/AGENTS.md b/AGENTS.md
@@
- 1. Do not introduce new cross-layer control through `GlobalV`, `GlobalC`, or
- `PARAM`; pass dependencies explicitly.
+ 1. Avoid introducing new cross-layer control through `GlobalV`, `GlobalC`, or
+ `PARAM`; pass dependencies explicitly whenever practical, and document any
+ migration-period exception with reason, scope, risk, and cleanup plan.The PR template should also ask for a migration-period rationale, not just a hard exception:
diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md
@@
-- Global dependencies: no new `GlobalV`, `GlobalC`, or `PARAM` cross-layer control, or exception requested below.
+- Global dependencies: no new `GlobalV`, `GlobalC`, or `PARAM` cross-layer control, or migration-period rationale provided below with reason, scope, risk, and cleanup plan.Required change 6: update CodeRabbit and Copilot instructions
Automated review instructions should match the downgraded policy. Otherwise automated reviewers may continue to treat new global usage as a hard failure while CI reports success.
Suggested updates:
diff --git a/.github/instructions/abacus-governance.instructions.md b/.github/instructions/abacus-governance.instructions.md
@@
-- Flag newly introduced `GlobalV`, `GlobalC`, or `PARAM` cross-layer control.
- Prefer explicit dependencies or narrow local interfaces.
+- Flag newly introduced `GlobalV`, `GlobalC`, or `PARAM` cross-layer control as
+ migration-period warnings unless the governance matrix says otherwise. Prefer
+ explicit dependencies or narrow local interfaces, and require reason, scope,
+ risk, and cleanup/follow-up rationale when globals remain.diff --git a/.coderabbit.yaml b/.coderabbit.yaml
@@
- 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.
+ 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 new global dependencies as
+ reviewer-visible warnings that require reason, scope, risk, and cleanup
+ rationale rather than automatic request-changes findings.Required change 7: make warning results visible outside the job summary
Once a finding is downgraded to warning, the Agent Governance check turns green. A green check plus warnings hidden in $GITHUB_STEP_SUMMARY is too easy to miss. This PR itself demonstrates the issue: the workflow succeeds while the summary contains a governance warning.
Please either add GitHub annotations or a single updatable PR comment. For a top-level PR comment, use the issue comments API and request issues: write, not just pull-requests: write:
diff --git a/.github/workflows/agent_governance.yml b/.github/workflows/agent_governance.yml
@@
permissions:
contents: read
pull-requests: read
+ issues: write
@@
- name: Publish governance summary
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='<!-- agent-governance-warning -->'
+ 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"
+ fiIf maintainers do not want to grant issues: write, then at minimum emit GitHub Actions annotations with ::warning file=...,line=...::... so warnings are visible in the check UI.
Required change 8: update this PR's own verification section
The PR body is still mostly template text. After the code and policy updates, please fill in exact verification. At minimum:
### Exact Verification Performed
- `python3 -m unittest tools/03_code_analysis/test_agent_governance_check.py -q` — passed.
- `python3 tools/03_code_analysis/agent_governance_check.py --base upstream/develop --head HEAD --format text` — no errors; expected warnings only.
- `pre-commit run abacus-agent-governance --all-files` — passed, or explain why it was not run.
### Governance Exception / Migration Rationale
- Rule: New global dependency review severity downgraded from mechanical blocker to reviewer-visible warning.
- Reason: Legacy ABACUS code still contains broad `GlobalV`/`GlobalC`/`PARAM` usage and needs staged migration.
- Scope: Governance checker severity and associated review instructions only; other deterministic blockers remain unchanged.
- Risk: New global dependencies may still be introduced unless reviewers enforce rationale.
- Follow-up cleanup plan: Require warning comments and PR rationale during migration, then gradually re-tighten modules after migration.Suggested final scope for this PR
The final PR should express one policy change:
No new cross-layer globalschanges from mechanicalerrorto reviewer-visiblewarning.PR metadata completeness, LF line endings, CMake linkage, INPUT documentation linkage, and default-argument checks remain blockers.- Tests, docs, templates, AI-review instructions, and workflow warning visibility are updated to match the new warning semantics.
With those changes, this becomes a controlled downgrade rather than a partial severity flip that silently weakens unrelated governance checks.
QuantumMisaka
left a comment
QuantumMisaka
left a comment
There was a problem hiding this comment.
Follow-up review proposal: instead of a blanket downgrade from BLOCK to WARN, I suggest using a global dependency budget rule. This fits the current diff-scoped governance model better: legacy global usage can be migrated gradually, but a PR should not increase the total amount of GlobalV / GlobalC / PARAM usage.
Theme 1: Replace line-only blocking with a net global dependency budget
Current changed area: tools/03_code_analysis/agent_governance_check.py, around check_global_dependencies().
Suggested policy:
added_global_occurrences = count of GlobalV / GlobalC / PARAM references in added diff lines
removed_global_occurrences = count of GlobalV / GlobalC / PARAM references in removed diff lines
delta = added_global_occurrences - removed_global_occurrences
if delta > 0:
error, because the PR increases global dependency debt
elif added_global_occurrences > 0:
warning, because the PR still adds or moves global dependency usage, but does not increase total debt
else:
no global-dependency finding
This is stricter than a simple warning downgrade, but more migration-friendly than the current "any added global reference blocks" rule.
Suggested code change
Please replace the current added-line-only parser with added/removed diff parsing, then make check_global_dependencies() decide severity from the net delta.
diff --git a/tools/03_code_analysis/agent_governance_check.py b/tools/03_code_analysis/agent_governance_check.py
@@
-def parse_added_lines(diff_text: str) -> List[DiffLine]:
- lines: List[DiffLine] = []
- path = ""
- new_line: Optional[int] = None
- hunk_re = re.compile(r"@@ -\d+(?:,\d+)? \+(\d+)(?:,(\d+))? @@")
+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+))? @@")
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 new_line is None:
+ if old_line is None or new_line is None:
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)Then update global dependency checking:
diff --git a/tools/03_code_analysis/agent_governance_check.py b/tools/03_code_analysis/agent_governance_check.py
@@
-def check_global_dependencies(findings: List[Finding], lines: Iterable[DiffLine]) -> None:
- pattern = re.compile(r"\b(GlobalV::|GlobalC::|PARAM(?:\.|->|::|\b))")
- for line in lines:
- if line.path.startswith("tools/03_code_analysis/"):
- continue
- if Path(line.path).suffix.lower() not in CODE_EXTENSIONS:
- continue
- if pattern.search(line.content):
- add_finding(
- findings,
- "No new cross-layer globals",
- BLOCK,
- 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.",
- )
+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 not is_global_dependency_check_path(line.path):
+ continue
+ 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,
+ "Global dependency budget",
+ severity,
+ line.path,
+ line.line,
+ (
+ f"Added line introduces {count} GlobalV/GlobalC/PARAM reference(s); "
+ f"PR total added={added_count}, removed={removed_count}, net_delta={delta}."
+ ),
+ action,
+ )Finally wire the removed lines into collect_findings():
diff --git a/tools/03_code_analysis/agent_governance_check.py b/tools/03_code_analysis/agent_governance_check.py
@@
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)Theme 2: Keep PR metadata completeness as a blocker
Current changed area: tools/03_code_analysis/agent_governance_check.py, check_pr_metadata().
This rule is unrelated to global-state migration. Downgrading it would allow empty or placeholder PR bodies to pass. Please restore it:
diff --git a/tools/03_code_analysis/agent_governance_check.py b/tools/03_code_analysis/agent_governance_check.py
@@
add_finding(
findings,
"PR metadata completeness",
- WARN,
+ BLOCK,
"pull_request.body",
None,
"; ".join(reason_parts),
"Fill the PR template with issue linkage, test evidence, behavior impact, governance notes, and exception details.",
allow_exception=False,
)Theme 3: Update tests to cover budget behavior, not blanket downgrade behavior
Please replace the current global-dependency blocker-only tests with explicit budget tests.
Suggested test cases:
diff --git a/tools/03_code_analysis/test_agent_governance_check.py b/tools/03_code_analysis/test_agent_governance_check.py
@@
- 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")
+
+ 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_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")Please keep the PR metadata tests as blocker tests:
self.assert_blocked_by(result, "PR metadata completeness")Theme 4: Update governance documentation to describe the budget rule
The current matrix says every added global reference is a blocker. If the implementation becomes budget-based, the docs should say that clearly.
Suggested matrix update:
diff --git a/docs/developers_guide/agent_governance.md b/docs/developers_guide/agent_governance.md
@@
-| 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 |Suggested policy paragraph:
@@
Deterministic errors require a code or documentation fix before merge unless a
maintainer-approved exception is recorded. Deterministic warnings require a
reviewable rationale or cleanup, but they do not become blockers without an
explicit governance change.
+
+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.Theme 5: Update AGENTS.md and PR template wording
Suggested AGENTS.md wording:
diff --git a/AGENTS.md b/AGENTS.md
@@
- 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.Suggested PR template wording:
diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md
@@
-- 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.Theme 6: Keep warning findings visible when the budget is non-increasing
With the budget rule, some global dependency findings will intentionally be warnings. Those warnings should not be hidden only in $GITHUB_STEP_SUMMARY. Please keep or add a warning visibility mechanism, either as GitHub Actions annotations or an updatable PR comment.
At minimum, the workflow should continue to publish the summary and make warnings visible when the job exits successfully. If using a PR comment, prefer issues: write because top-level PR comments use the issue comments API.
Theme 7: Verification expected after implementing this proposal
Please update the PR body with exact verification after changes:
### Exact Verification Performed
- `python3 -m unittest tools/03_code_analysis/test_agent_governance_check.py -q` -- passed.
- `python3 tools/03_code_analysis/agent_governance_check.py --base upstream/develop --head HEAD --format text` -- no unexpected errors.
- A synthetic net-increase fixture blocks with `Global dependency budget`.
- A synthetic add/remove-neutral fixture exits successfully with a warning.
- A synthetic removal-only fixture exits successfully without a global dependency finding.This approach gives maintainers the flexibility requested for legacy migration without allowing the global dependency debt to grow silently.
QuantumMisaka
left a comment
QuantumMisaka
left a comment
There was a problem hiding this comment.
LGTM, and I'll do some modification later base on comments in this PR
3 participants
Reminder
AGENTS.mdanddocs/developers_guide/agent_governance.md.source/changes.Linked Issue
Fix #...
Unit Tests and/or Case Tests for my changes
Exact Verification Performed
What's changed?
Governance Checklist
GlobalV,GlobalC, orPARAMcross-layer control, or exception requested below..hpppropagation, or rationale provided below..batand.cmduse CRLF.CMakeLists.txt, or rationale provided below.@coderabbitai review.INPUT Parameter Changes
docs/parameters.yamlupdated: yes/no/not applicabledocs/advanced/input_files/input-main.mdupdated: yes/no/not applicableCore Module Impact
Governance Exception