Uh oh!
There was an error while loading. Please reload this page.
Add PR-time guard: forbid new \init\ accessors on public API - #8900
Conversation
Adds a lightweight (Python + workflow, no LLM) PR check that fails the build whenever a PR adds a line matching '.init -> void' to any PublicAPI.Unshipped.txt file. Grandfathered entries already in PublicAPI.Shipped.txt are intentionally ignored. Background: copilot-instructions.md and expert-reviewer.agent dimension #4 both already document the no-init policy, but the rule is enforced only by reviewer attention today. Surfaces of this policy: - .github/copilot-instructions.md (Public API guidelines section) - .github/agents/expert-reviewer.agent.md (Overarching Principle #2) This guard runs in <10s, has no LLM cost, and is paths-scoped to PRs that actually touch a PublicAPI.Unshipped.txt file or the guard itself. Files: - .github/scripts/check_public_api_init.py - script with --diff-file support for offline testing, follows the .github/scripts/check_vendored_files.py style already in the repo. - .github/workflows/public-api-init-guard.yml - pull_request workflow, paths-scoped, concurrency-cancelling, contents:read only. Tested locally with a synthetic diff containing two .init additions in Unshipped.txt and one in Shipped.txt: the script correctly reports the two Unshipped violations and ignores the Shipped addition, exits 1. Clean diff exits 0. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Pull request overview
Adds a lightweight, PR-scoped GitHub Actions guard to enforce the repo policy that new public APIs must not introduce init accessors, by scanning PR diffs for additions of .init -> void in PublicAPI.Unshipped.txt.
Changes:
- Adds a new
pull_requestworkflow that runs only when**/PublicAPI.Unshipped.txt(or the guard itself) changes. - Adds a Python script that parses a unified diff and flags newly-added
.init -> voidlines inPublicAPI.Unshipped.txt. - Emits a human-readable report to both console output and
GITHUB_STEP_SUMMARYwhen violations are found.
Show a summary per file
| File | Description |
|---|---|
.github/workflows/public-api-init-guard.yml | New PR workflow wiring: checkout, base computation, and running the Python guard (paths-scoped and least-privilege). |
.github/scripts/check_public_api_init.py | Diff parser + policy enforcement: identifies new .init -> void additions in PublicAPI.Unshipped.txt and prints a markdown report. |
Copilot's findings
- Files reviewed: 2/2 changed files
- Comments generated: 3
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Amaury Levé (Evangelink)
left a comment
There was a problem hiding this comment.
Note
🤖 Automated review by GitHub Copilot. Posted via a maintainer's GitHub token, so it appears under their account — the account owner did not write or approve this content personally. Generated by the Expert Code Review workflow. To request a follow-up action, reply by tagging @copilot directly.
| # | Dimension | Verdict |
|---|---|---|
| 13 | Test Completeness | 🟡 1 MODERATE |
| 17 | Documentation Accuracy | 🔵 1 NIT |
| 20 | Build Infrastructure | 🔵 1 NIT |
✅ 18/21 dimensions clean (15 N/A for Python/YAML-only PR, 3 confirmed LGTM).
- Test Completeness —
parse_diff()has no automated tests
Dimension notes
Dimensions 2, 4–12, 14–16, 18–19: N/A — No C# source, no test files, no IPC protocol changes, no analyzer code. All skipped per the reviewer spec.
Dimension 1 (Correctness): LGTM — The three-dot git diff semantics are correct for PR merge-base comparison. The INIT_LINE regex (\.init\s*->\s*void\s*$) is anchored correctly and matches the exact format the public-API analyzer emits. The startswith("+") / not startswith("+++") guard correctly excludes file headers. New-file hunks (where the entire file is an addition) are handled correctly. The Shipped.txt vs Unshipped.txt distinction is implemented correctly via basename comparison.
Dimension 3 (Security): LGTM — pull_request (not pull_request_target) is the correct trigger; the workflow cannot be hijacked by PR authors to gain elevated permissions. permissions: contents: read is minimal. The subprocess.run(cmd, ...) call passes base as a list element, not in a shell string, so there is no injection surface. ${{ github.event.pull_request.base.ref }} is written to an env: variable and only echo'd into $GITHUB_OUTPUT wrapped in double-quotes — safe for branch names (which cannot contain newlines in GitHub).
Dimension 13 (Test Completeness): MODERATE — The parse_diff() function contains several non-trivial branches and is the core of the entire guardrail, but has no automated tests. The PR description notes "locally tested with two synthetic diffs", which is not reproducible in CI. Regressions in the parsing logic (e.g., a future edit flipping the startswith('+') guard) would silently pass the self-test since the diff of the script itself contains no PublicAPI.Unshipped.txt changes. Worth adding a small pytest suite under .github/scripts/tests/ covering: (a) a diff that creates a new PublicAPI.Unshipped.txt entirely, (b) a diff containing both Unshipped.txt and Shipped.txt hunks, (c) the sentinel-line skip (#/* prefixes), and (d) a clean diff with no violations. Note: check_vendored_files.py similarly has no unit tests, so this is consistent with current repo practice — but for a new policy guardrail this is worth addressing as a follow-up.
Dimension 17 (Documentation): NIT — Inline comment on line 24 filed above (/tmp/pr.diff reference in docstring).
Dimension 20 (Build Infrastructure): NIT — Inline comment on line 53 filed above (unnecessary setup-python step). Actions are pinned to tag aliases (@v4, @v5) rather than commit SHAs; this is consistent with the repo's existing hand-written workflows (check-vendored-files.yml, markdownlint.yml, dedup-analysis.yml), so not a regression, but SHA pinning would provide stronger supply-chain assurance.
Dimension 21 (Scope & PR Discipline): LGTM — Clean single-concern PR. Follow-up work (checking new public API in .cs that doesn't appear in any PublicAPI.Unshipped.txt) is explicitly called out as out of scope with a rationale.
Generated by Expert Code Review (on open) for issue #8900 · sonnet46 2.5M
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
* Docstring: fix two-dot vs three-dot mismatch. The implementation uses `git diff <base>...HEAD` (three-dot) but the usage text said `..HEAD` (two-dot). Updated the docstring to `...HEAD` and added a sentence explaining the three-dot semantics (changes on HEAD since the merge-base with BASE_SHA). * Docstring: drop the misleading `/tmp/pr.diff` example -- CI runs the script directly without writing a diff file. Replaced with a local `git diff -U0 main...HEAD > pr.diff` example so the `--diff-file` flag still has a concrete use case shown. * Workflow: switch the diff base from the named base ref to the exact base SHA captured in the event payload. The named ref can advance between the PR event firing and the checkout fetch, which would pull in unrelated commits and false-flag `.init -> void` lines the PR never touched. `base.sha` is immutable for the duration of the PR event and is the merge-base candidate, so three-dot diff `<base.sha>...HEAD` reliably yields the PR's net public-API change. This also lets us drop the separate "Compute base ref" step. * Empty except: add explanatory comment + `# noqa: BLE001`. The bare except around `sys.stdout.reconfigure` was intentional (output-encoding setup is best-effort and must not fail the script), but the silent `pass` left readers wondering. The comment now documents the intent. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Move bot-author exemption from Python to the workflow `if:`. Per the reviewer, the Python script should focus on diff analysis and not need to know about workflow context. Removed `KNOWN_BOTS`, `is_bot()`, `--author`, and the `PR_AUTHOR` env var from the script. The workflow now skips the whole job via: if: >- github.event.pull_request.user.type != 'Bot' && github.event.pull_request.user.login != 'dotnet-bot' && github.event.pull_request.user.login != 'dotnet-maestro' `user.type == 'Bot'` covers GitHub App identities; the explicit logins cover the dotnet-bot / dotnet-maestro user accounts. * Pin actions to SHA per the repository's lock-file convention. The workflow now uses: actions/checkout@34e1148 # v4.3.1 actions/setup-python@a26af69 # v5.6.0 These are the current tip-of-v4 / tip-of-v5 commits as of 2026-06-07. * Empty except: narrow `except Exception` to `except (AttributeError, ValueError, OSError)` and add an explanatory comment. The previous bare except swallowed any programmer error from `sys.stdout.reconfigure`; the new tuple matches the documented failure modes only. * Drive-by while in the workflow: also switch the diff base from `origin/<base.ref>` to the immutable `base.sha` from the event payload, mirroring the fix applied to PR #8900. A base-branch advance between the event and the checkout fetch could otherwise pull in unrelated commits. * Rename the env var to `BASE_REF` (with `BASE_SHA` fallback in the script's `--base` default for backward compatibility) for consistency with the rest of this PR series. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
testfx's existing tooling is PowerShell (eng/common/, eng/scripts/, .github/scripts/scan-duplicates.ps1) — only Arcade's vendored install-debs.py is Python. Introducing a new Python script + an actions/setup-python step for a 200-line guard rail added a runtime dependency the repo otherwise doesn't need. This rewrite: - Adds .github/scripts/check-public-api-init.ps1 with the same logic: parse unified diff, filter PublicAPI.Unshipped.txt additions, flag any `.init -> void` line, write a Markdown step summary. - Removes .github/scripts/check_public_api_init.py. - Drops the actions/setup-python step from the workflow and switches the run step to `shell: pwsh` (cross-platform on ubuntu-latest). - Updates `paths:` to track the new .ps1 filename. Smoke-tested locally with synthetic diff inputs covering: (a) added .init line in Unshipped.txt → exit 1 with report, (b) shipped .init line ignored, (c) clean Unshipped.txt → exit 0. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Summary
Adds a fast PR-time guard that fails when a PR adds a new
initaccessor to anyPublicAPI.Unshipped.txtfile. The check is a PowerShell script + plain GitHub Actions workflow — no LLM, no agentic workflow, nogh aw compile— so it runs in under 10s on every applicable PR.Why
The no-
initpolicy is already documented in two places:.github/copilot-instructions.md(Public API guidelines):.github/agents/expert-reviewer.agent.md(Overarching Principle Porting latest changes. #2 and Dimension Bug Fix #258333 : Tests running multiple times in case we have multiple test projects #4).…but enforcement today depends on a reviewer noticing the
.init -> voidline in aPublicAPI.Unshipped.txtdiff.expert-reviewer.agentonly runs on opt-in / on a schedule, so a fast-moving PR can land before it weighs in. This guard runs unconditionally on every PR that touches aPublicAPI.Unshipped.txt.Grandfathered entries already in
PublicAPI.Shipped.txtare intentionally not flagged — there are ~50 of them insrc/Platform/Microsoft.Testing.Platform/PublicAPI/PublicAPI.Shipped.txtand the policy explicitly exempts them.What changed
.github/scripts/check-public-api-init.ps1git diffor-DiffFile), finds additions to anyPublicAPI.Unshipped.txt, and flags lines matching.init -> void. Writes a markdown report to$GITHUB_STEP_SUMMARYon failure..github/workflows/public-api-init-guard.ymlpull_requestworkflow,paths-scoped to**/PublicAPI.Unshipped.txtand the script/workflow themselves.contents: readonly. Concurrency-cancelling per-PR. Runs viashell: pwsh(pre-installed onubuntu-latest), no extra setup step.Validation
Locally tested with two synthetic diffs:
Violation diff (two
.init -> voidadditions to anUnshipped.txt, one.init -> voidaddition to aShipped.txt):Correctly ignored the
Shipped.txtaddition and the regular.set -> voidline.Clean diff (regular getter/setter pair):
Risk
**/PublicAPI.Unshipped.txt). Existing PRs with validinitremovals or unrelated edits won't see it.\.init\s*->\s*void\s*$) is tight enough to not flag explanatory comments or the*REMOVED*/*NULLABILITY*sentinel lines used by the public-API analyzer (the script skips lines starting with#or*).contents: readonly — cannot mutate the PR.msbuild-reviewerRule B-3) provides a related precedent for catching grandfather/new boundary cases.Follow-ups (not in this PR)
publicAPI insrc/**/*.csthat doesn't appear in anyPublicAPI.Unshipped.txtdiff, but that needs Roslyn-quality parsing to be reliable — out of scope here.🤖 Authored with GitHub Copilot CLI based on a one-month review-comment audit of microsoft/testfx.