Skip to content

Commit c0dc966

Browse files
QuantumMisakaQuantumMisaka
committed
Document agent governance review fixes
1 parent 5e24176 commit c0dc966

1 file changed

Lines changed: 144 additions & 0 deletions

File tree

‎docs/superpowers/specs/2026-06-21-agent-governance-review-fixes-design.md‎

Lines changed: 144 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,144 @@
1+
# Agent Governance Review Fixes Design
2+
3+
## Context
4+
5+
The governance checker added in `tools/03_code_analysis/agent_governance_check.py`
6+
is intended to block deterministic issues only on changed files or diff-added
7+
lines. Three review findings were verified locally:
8+
9+
- CRLF-only normalization diffs are treated as semantic added lines by
10+
`git diff -U0`, so historical default arguments and global dependencies can
11+
be reported as new changes.
12+
- A pull request event with an empty `pull_request.body` is treated the same as
13+
no pull request event, so required PR metadata can be skipped.
14+
- The GitHub Actions workflow passes the PR base tip directly as `--base`.
15+
After the target branch advances, that two-dot comparison no longer matches
16+
the merge-base-to-head PR diff that GitHub presents.
17+
18+
## Goals
19+
20+
- Keep semantic governance rules scoped to meaningful diff-added lines.
21+
- Keep full changed-file LF enforcement intact, including detection of CRLF in
22+
the target file content.
23+
- Treat an empty PR description in a PR event as incomplete metadata.
24+
- Scope CI governance checks to the PR merge base and head commit.
25+
- Add focused regression tests for each verified failure mode.
26+
27+
## Non-Goals
28+
29+
- Do not redesign the checker CLI or change the meaning of `--base` and
30+
`--head`.
31+
- Do not convert warning-only governance rules into blockers.
32+
- Do not change the PR template or governance policy text beyond what is needed
33+
to align implementation behavior with the existing policy.
34+
35+
## Recommended Approach
36+
37+
Use the minimal complete fix:
38+
39+
1. Make `added_lines()` use a CR-at-EOL-insensitive diff:
40+
- For staged mode, run `git diff --cached --ignore-cr-at-eol -U0`.
41+
- For base/head mode, run `git diff --ignore-cr-at-eol -U0 <base> <head>`.
42+
2. Leave `changed_paths()` and `check_line_endings()` unchanged so changed-file
43+
discovery and byte-level LF enforcement still see files whose content changed
44+
only by line endings.
45+
3. Make PR body reading distinguish these cases:
46+
- No `--event-path`: skip PR metadata completeness, because local/staged runs
47+
do not have PR metadata.
48+
- Invalid or non-PR event payload: skip PR metadata completeness.
49+
- PR event with `body: ""` or `body: null`: run metadata completeness and
50+
block as incomplete.
51+
4. In `.github/workflows/agent_governance.yml`, compute
52+
`merge_base=$(git merge-base "$base_sha" "$head_sha")` after checkout and pass
53+
that value as `--base`.
54+
55+
This keeps responsibilities simple: the workflow chooses the correct comparison
56+
range for PR CI, while the checker continues to compare two explicit commits.
57+
58+
## Component Changes
59+
60+
### Semantic Diff Collection
61+
62+
`added_lines()` is the only source of diff-added lines for semantic rules such
63+
as no new globals, no new default parameters, `.hpp` propagation, header include
64+
growth, and INPUT behavior checks. Adding `--ignore-cr-at-eol` there prevents
65+
line-ending normalization from fabricating added semantic lines.
66+
67+
The separate changed-file path list remains based on regular `git diff
68+
--name-status`. This is important because line-ending-only changes still need to
69+
be included in the full-file LF check.
70+
71+
### PR Metadata Reading
72+
73+
Introduce a small internal distinction between "metadata unavailable" and
74+
"metadata available but empty". A simple option is to let `read_pr_body()` return
75+
`Optional[str]`, where `None` means no PR metadata is available and `""` means
76+
the PR body is blank.
77+
78+
`check_pr_metadata()` should skip only when the body is `None`. For an empty
79+
string, it should report all required sections as missing, using the existing
80+
`PR metadata completeness` finding and `allow_exception=False`.
81+
82+
Other PR-body consumers should handle `None` as an empty string for warning
83+
logic, because test evidence and documentation evidence cannot be inferred when
84+
metadata is unavailable.
85+
86+
### Workflow Diff Base
87+
88+
The workflow already uses `actions/checkout@v4` with `fetch-depth: 0`, so the
89+
merge base can be computed locally:
90+
91+
```bash
92+
base_sha="${{ github.event.pull_request.base.sha }}"
93+
head_sha="${{ github.event.pull_request.head.sha }}"
94+
merge_base="$(git merge-base "$base_sha" "$head_sha")"
95+
python3 tools/03_code_analysis/agent_governance_check.py \
96+
--base "$merge_base" \
97+
--head "$head_sha" \
98+
--event-path "$GITHUB_EVENT_PATH" \
99+
--format markdown
100+
```
101+
102+
If `git merge-base` fails, the workflow should naturally fail before publishing
103+
a misleading clean governance result. The existing summary fallback can still
104+
report that the checker failed before producing a summary.
105+
106+
## Error Handling
107+
108+
- Git command failures inside the checker should continue to surface through
109+
`GitError` and exit code 2.
110+
- Invalid JSON or non-PR event payloads should not block local or accidental
111+
non-PR invocations.
112+
- Blank PR bodies in valid PR payloads should block with the existing metadata
113+
completeness rule.
114+
115+
## Tests
116+
117+
Add focused tests to `tools/03_code_analysis/test_agent_governance_check.py`:
118+
119+
- A CRLF-to-LF-only change in a header containing a historical default argument
120+
should not trigger `No new default parameters`.
121+
- A valid PR event with `pull_request.body` set to an empty string should trigger
122+
`PR metadata completeness` and return non-zero.
123+
- A merge-base-scoped comparison in a diverged history should not include
124+
base-branch-only changes, while the existing two-dot base-tip comparison would
125+
demonstrate the risk.
126+
127+
Existing tests for CRLF detection in changed text files must continue to pass.
128+
129+
## Verification
130+
131+
After implementation, run:
132+
133+
```bash
134+
python3 -m unittest tools/03_code_analysis/test_agent_governance_check.py
135+
python3 tools/03_code_analysis/agent_governance_check.py --staged
136+
```
137+
138+
If dependencies are available, also run:
139+
140+
```bash
141+
pre-commit run abacus-agent-governance --all-files
142+
```
143+
144+
Report exact command output or failure details before claiming completion.

0 commit comments

Comments
 (0)