Skip to content

ci: PR behaviour gates -- diff coverage 80%, diff budget, PR template check - #1814

Closed
zoroyihan7 wants to merge 4 commits into
mainfrom
ci/pr-behaviour-gates
Closed

zoroyihan7 wants to merge 4 commits into
mainfrom
ci/pr-behaviour-gates

Conversation

@zoroyihan7

@zoroyihan7 zoroyihan7 commented Oct 10, 2026 •

Copy link
Copy Markdown
Contributor
  • Description: Implements the PR-behaviour part (P1-4) of the code-principles gate proposal: three checks on what a PR changes, plus moving one existing check to a job that can be required.
    1. Diff coverage (tests-coverage.yml, coverage (Python 3.10) job, pull requests only). After the existing combine, coverage xml + diff-cover==9.7.1 --fail-under=80 against HEAD^1 of the PR merge commit (checkout fetch-depth: 2, so no full-history fetch). The markdown report is appended to the job summary. It runs under always() after the total fail_under gate, and is skipped when a shard failed or is missing (the shard-results gate already reds that case). On 17 measurable recent PRs, 1 was below 80% (feat(tools): kbmine, a standalone no-run uplift estimator over the Recipe KB and Pulse #1778, 77%).
    2. Diff budget (new pr-hygiene.yml, job pr-hygiene). git diff -M --numstat -z HEAD^1 HEAD over production Python (src/, scripts/; tests//test/ dirs, test_*.py, *_test.py, conftest.py excluded; renames count only their edits). Warn above 400, fail above 1000. The size-exception label exempts only if the most recent user who added it has write/maintain/admin on the repo, checked with the read-only token via the issue timeline and collaborators/{user}/permission; any API failure means not exempt. Counts match the research data exactly on 11 recent merged PRs (fix(enablement): make enablement_setting.sh reproduce the setup on its own #1789 703, refactor(kernelforge): restructure knowledge into domain packages #1788 763, fix(sweep): stop a cancelled conc_sweep, and let SWEEP wait for the sweep it granted #1767 151, ...).
    3. PR template completeness (same job, advisory). The body is passed as PR_BODY env (never interpolated into shell) and checked for Tests, Size/complexity, Observable effect, Breaking changes and "PR addresses single concern" (must start with yes/no). An untouched template prompt counts as unanswered. Findings are warning annotations + step summary; the job stays green. Making it blocking is one line: PR_TEMPLATE_ENFORCE: "true" in pr-hygiene.yml. Bot-authored PRs are skipped. On the last 50 merged PRs, 20 of 48 non-bot PRs would get a warning.
    4. scripts/check_cli_references.py moved from docs.yml (which has a paths filter, so a required check there would sit pending) into the always-run pr-hygiene job.
      No PR comments are posted (the sticky-comment mechanism is in ci: code-metrics gate on touched files with baseline-free checks; fold in import-linter, PR hygiene and ruff principle rules #1812).
  • Linked issue(s): none
  • Tests: scripts/tests/test_pr_hygiene.py (41 tests) pins each refusal by message: budget exceeded (1001 lines), a 1001-line test file renamed into production (counted in full), at-limit warning (1000 prod lines with 5000 test lines and a rename), label present but unverifiable, labeler with triage only, latest labeler wins across timeline pages, permission API error; template findings per field, yes/no, untouched template, nested/colon-less bullets, workflow-command injection in the body, advisory vs --enforce. Workflow contracts: no paths filter, read-only permissions, body passed via env, the CLI check runs inside ::stop-commands:: (it prints PR-controlled paths), *.py diff is forced via .git/info/attributes before both diff-based checks (a PR .gitattributes with *.py -diff otherwise hides every line), diff-cover pinned and PR-only. diff-cover invocation reverse-proved locally on a synthetic merge commit: an added function with 5 of 8 lines uncovered -> exit 1 "Failure. Coverage is below 80%"; fully covered -> exit 0; no diff -> exit 0.
  • Size/complexity triggers crossed: ~340 production lines in one new script; it is three small checks sharing argument parsing and annotation output.
  • If this simplifies or refactors: n/a
  • Observable effect: PR authors see a pr-hygiene check (diff budget errors, template warnings, CLI reference errors) and a diff-coverage section in the coverage (Python 3.10) summary; a PR whose new lines are under 80% covered now fails that job.
  • Breaking changes: no for merging (nothing is required yet). Admin action needed to make these block: in ruleset "main", add pr-hygiene and coverage (Python 3.10) as required status checks, and create the size-exception label (it does not exist yet). Note that tests-coverage.yml has a paths-ignore for docs-only changes, so a required coverage (Python 3.10) would stay pending on docs-only PRs until that workflow gets an always-run wrapper; pr-hygiene has no paths filter.
  • PR addresses single concern: yes (PR-behaviour gates, proposal item P1-4)
  • Root cause is upstream (Magpie/TraceLens/GEAK/IntelliKit/AgentKernelArena), ticket filed: n/a

Known limits: a PR can edit pr-hygiene.yml or scripts/pr_hygiene.py itself to bypass the gate (true of every pull_request workflow); /.github/workflows/ and * are owned by @AMD-AGI/SaFE, so this is closed once require_code_owner_review is enabled. A writer who adds size-exception before a later large push keeps the exemption. Pure code moves count as new lines for diff-cover.

🤖 Generated with Claude Code

zoroyihan7 and others added 4 commits October 10, 2026 13:33
- tests-coverage.yml: on pull requests, the Python 3.10 coverage job
  writes coverage.xml and runs diff-cover 9.7.1 against the merge
  commit's base parent with --fail-under=80; the markdown report goes to
  the job summary.
- pr-hygiene.yml (new, always runs, read-only token): diff budget over
  production Python lines (src/, scripts/, tests excluded; warn > 400,
  fail > 1000 unless a writer added the size-exception label), the
  agent-doc CLI reference check moved from docs.yml, and an advisory PR
  template completeness check that reads the body only as data.
- scripts/pr_hygiene.py + tests pinning each refusal message.

Co-Authored-By: Claude <noreply@anthropic.com>
A pure rename has zero numstat lines, so moving a 1001-line test file to
a production path passed the diff budget at 0 lines.

Co-Authored-By: Claude <noreply@anthropic.com>
… text diffs

check_cli_references.py prints PR-controlled file paths; a crafted path
could start a workflow command. A PR's .gitattributes marking *.py as
-diff hid its lines from both the budget and diff-cover; .git/info/
attributes takes precedence over it.

Co-Authored-By: Claude <noreply@anthropic.com>
@zoroyihan7

Copy link
Copy Markdown
Contributor Author

Superseded by #1812, which now carries this change.

@zoroyihan7 zoroyihan7 closed this Oct 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type:task Internal task or chore

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant