Add PR-time reminder: CLI options provider changes may need help-text expectation updates#8901
Add PR-time reminder: CLI options provider changes may need help-text expectation updates#8901Evangelink wants to merge 3 commits into
Conversation
When a PR touches a `*CommandLineOptionsProvider*.cs` (or `PlatformCommandLineProvider*.cs`, or `MSTestExtension.cs`) but does not also touch any of the four `--help` / `--info` acceptance-test expectation files documented in copilot-instructions.md, this workflow posts a soft reminder via: - a markdown block written to `\`, AND - a `::notice` workflow annotation that surfaces in the PR Checks tab. The reminder is intentionally NON-BLOCKING (script always exits 0). Pure refactors of provider files commonly do not change the rendered help output, and the acceptance tests at CI time are the authoritative gate. This guard just makes the policy visible at PR creation/synchronize so the author can update expectations in the same push. Files: - .github/scripts/check_cli_help_drift.py - works with both `git diff --name-only` output and full unified diffs; supports `--diff-file` for offline testing. - .github/workflows/cli-help-drift-reminder.yml - `pull_request` workflow, paths-scoped, contents:read only. Tested locally with three synthetic diffs (provider only / provider + expectation / neither): produces the correct reminder, satisfied message, and skip message respectively, exiting 0 in all three cases. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR adds a non-blocking GitHub Actions check to surface an early reminder when a pull request modifies CLI options provider implementations without also updating the corresponding --help / --info acceptance-test expectation files, per the repo’s documented policy.
Changes:
- Add a
pull_requestworkflow that runs only when relevant provider/expectation paths change and emits a soft reminder. - Add a Python script that diffs the PR, classifies changed files (providers vs expectation files), and prints a notice + step summary when expectations weren’t touched.
Show a summary per file
| File | Description |
|---|---|
.github/workflows/cli-help-drift-reminder.yml |
New PR-scoped workflow to run the drift reminder check. |
.github/scripts/check_cli_help_drift.py |
New script to classify changed files and emit the reminder + annotation. |
Copilot's findings
- Files reviewed: 2/2 changed files
- Comments generated: 4
| - '**/*CommandLineOptionsProvider*.cs' | ||
| - '**/PlatformCommandLineProvider*.cs' | ||
| - '**/MSTestExtension.cs' |
There was a problem hiding this comment.
Good catch — fixed in 9953b0d. Workflow paths: now uses **/*CommandLineOptionsProvider.cs and **/PlatformCommandLineProvider.cs (no trailing *), so test files like *ProviderTests.cs no longer kick off the workflow.
| # File-name patterns that indicate a CLI-options provider has been touched. | ||
| # Names rather than paths so the check stays robust against folder moves. | ||
| PROVIDER_PATTERNS: Tuple[re.Pattern[str], ...] = ( | ||
| re.compile(r"CommandLineOptionsProvider.*\.cs$", re.IGNORECASE), | ||
| re.compile(r"^PlatformCommandLineProvider.*\.cs$", re.IGNORECASE), | ||
| re.compile(r"^MSTestExtension\.cs$"), | ||
| ) |
There was a problem hiding this comment.
Fixed in 9953b0d. PROVIDER_PATTERNS now requires the filename to end exactly at Provider.cs: CommandLineOptionsProvider\.cs$ and ^PlatformCommandLineProvider\.cs$. Smoke-tested with both MyCommandLineOptionsProviderTests.cs and PlatformCommandLineProviderTests.cs — both are now classified as non-provider and produce no reminder.
| Exit codes: | ||
| 0 — no provider files changed, OR provider files changed and at least | ||
| one help-expectation file is in the diff (i.e. the contract is | ||
| plausibly satisfied; the acceptance tests in CI are the final word). | ||
| 0 — provider files changed without expectation-file changes. We still | ||
| exit 0 so the workflow stays a *reminder*, not a blocker; refactors | ||
| and visibility tweaks that don't touch any help output are common. | ||
| The reminder is surfaced via stdout + GITHUB_STEP_SUMMARY + | ||
| a workflow notice annotation produced by the calling workflow. | ||
| 2 — usage / IO error. |
There was a problem hiding this comment.
Fixed in 9953b0d. Consolidated to a single 0 — non-blocking by design entry that enumerates the three success conditions, and kept 2 — usage / IO error separate.
| - name: Run reminder | ||
| env: | ||
| BASE_SHA: origin/${{ github.event.pull_request.base.ref }} | ||
| run: python .github/scripts/check_cli_help_drift.py |
There was a problem hiding this comment.
Fixed in 9953b0d. Renamed the workflow env var to BASE_REF (the value is a named ref like origin/main, not a SHA). The script's --base default still falls back to \$BASE_SHA for backward compatibility in case any local invocations relied on the old name.
Evangelink
left a comment
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 |
|---|---|---|
| 1 | Algorithmic Correctness | 🟠 1 MAJOR |
| 20 | Build Infrastructure & Dependencies | 🟡 1 MODERATE |
| 17 | Documentation Accuracy | ⚪ 1 NIT |
✅ 14/21 dimensions clean. 6/21 N/A (Threading, Public API, Performance, Cross-TFM, IPC Wire, Localization, Resource Mgmt, Test Isolation, Assertion Quality, Flakiness, Data-Driven, Analyzer Quality, Test Completeness1).
- Algorithmic Correctness — Six real
ICommandLineOptionsProviderimplementations (AzureDevOpsCommandLineProvider.cs,CrashDumpCommandLineProvider.cs,HangDumpCommandLineProvider.cs,MSBuildCommandLineProvider.cs,HtmlReportGeneratorCommandLine.cs,TrxCommandLine.cs) don't matchPROVIDER_PATTERNSor the workflowpaths:filter because their filenames use*CommandLineProvider.csor*CommandLine.csrather than*CommandLineOptionsProvider.cs. PRs touching only those files will neither trigger the workflow nor produce a reminder. See inline comments. - Build Infrastructure —
actions/checkout@v4andactions/setup-python@v5use mutable version tags; every other workflow in this repo uses full SHA pins. See inline comment.
1 Test Completeness: the parse_changed_paths() function has non-trivial parsing logic (dual unified-diff / name-only mode, deduplication) but no automated test coverage. The --diff-file flag enables offline testing and the PR mentions three synthetic cases were validated manually, which is pragmatic for a CI script in a .NET repo. Consider adding a tests/ neighbour with pytest parametrization if this script evolves further.
Generated by Expert Code Review (on open) for issue #8901 · sonnet46 4.1M
| re.compile(r"CommandLineOptionsProvider.*\.cs$", re.IGNORECASE), | ||
| re.compile(r"^PlatformCommandLineProvider.*\.cs$", re.IGNORECASE), | ||
| re.compile(r"^MSTestExtension\.cs$"), | ||
| ) |
There was a problem hiding this comment.
[MAJOR] Algorithmic Correctness — Provider pattern coverage gap
Six real ICommandLineOptionsProvider / CommandLineOptionsProviderBase implementations in the repo have filenames that do not contain "CommandLineOptionsProvider" and will therefore never be flagged by this script:
| File | Naming pattern |
|---|---|
AzureDevOpsCommandLineProvider.cs |
*CommandLineProvider.cs |
CrashDumpCommandLineProvider.cs |
*CommandLineProvider.cs |
HangDumpCommandLineProvider.cs |
*CommandLineProvider.cs |
MSBuildCommandLineProvider.cs |
*CommandLineProvider.cs |
HtmlReportGeneratorCommandLine.cs |
*CommandLine.cs |
TrxCommandLine.cs |
*CommandLine.cs |
Because the workflow paths: filter uses the same glob (**/*CommandLineOptionsProvider*.cs), those PRs will not even trigger the workflow. The reminder would fire for TerminalTestReporterCommandLineOptionsProvider.cs changes but stay silent for CrashDumpCommandLineProvider.cs changes — an inconsistent signal.
Recommendation: Expand the patterns to cover both naming conventions in the codebase:
PROVIDER_PATTERNS: Tuple[re.Pattern[str], ...] = (
re.compile(r"CommandLineOptionsProvider.*\.cs$", re.IGNORECASE),
re.compile(r"CommandLineProvider.*\.cs$", re.IGNORECASE), # AzureDevOps, CrashDump, HangDump, MSBuild
re.compile(r".*CommandLine\.cs$", re.IGNORECASE), # HtmlReportGeneratorCommandLine, TrxCommandLine
re.compile(r"^PlatformCommandLineProvider.*\.cs$", re.IGNORECASE),
re.compile(r"^MSTestExtension\.cs$"),
)And add matching globs to the workflow paths: filter.
| # to folder moves. | ||
| - '**/*CommandLineOptionsProvider*.cs' | ||
| - '**/PlatformCommandLineProvider*.cs' | ||
| - '**/MSTestExtension.cs' |
There was a problem hiding this comment.
[MAJOR] Algorithmic Correctness — Workflow paths: filter has the same coverage gap
The globs here mirror the PROVIDER_PATTERNS in the script, so they share the same false-negative set: PRs that only change AzureDevOpsCommandLineProvider.cs, CrashDumpCommandLineProvider.cs, HangDumpCommandLineProvider.cs, MSBuildCommandLineProvider.cs, HtmlReportGeneratorCommandLine.cs, or TrxCommandLine.cs will not trigger this workflow at all.
Recommendation: Add the missing globs alongside the script fixes:
paths:
- '**/*CommandLineOptionsProvider*.cs'
- '**/*CommandLineProvider*.cs' # AzureDevOps, CrashDump, HangDump, MSBuild
- '**/*CommandLine.cs' # HtmlReportGeneratorCommandLine, TrxCommandLine
- '**/PlatformCommandLineProvider*.cs'
- '**/MSTestExtension.cs'| runs-on: ubuntu-latest | ||
| timeout-minutes: 5 | ||
| steps: | ||
| - uses: actions/checkout@v4 |
There was a problem hiding this comment.
[MODERATE] Build Infrastructure — Actions not SHA-pinned
Every other workflow in this repo pins third-party actions to a specific commit SHA (e.g. actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2). Using a mutable version tag (@v4, @v5) means a tag update — even an unintentional one — changes the workflow without a diff. The repo-wide convention is SHA pins.
Recommendation: Pin both actions:
- uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
with:
fetch-depth: 0
- uses: actions/setup-python@42375524bcc0f35d6ed4e1d8a3c3a1eb3b82b49e # v5.4.0
with:
python-version: '3.12'(Verify the latest SHA via gh release view --repo actions/checkout before merging.)
| exit 0 so the workflow stays a *reminder*, not a blocker; refactors | ||
| and visibility tweaks that don't touch any help output are common. | ||
| The reminder is surfaced via stdout + GITHUB_STEP_SUMMARY + | ||
| a workflow notice annotation produced by the calling workflow. |
There was a problem hiding this comment.
[NIT] Documentation Accuracy — Docstring is misleading
Line 26 says:
a workflow notice annotation produced by the calling workflow.
But the ::notice annotation is actually printed by this script (print("::notice ...") in main()), not by the calling workflow. The workflow only runs the script — it does not produce annotations itself.
Suggestion: Change to a workflow notice annotation produced by this script.
* Tighten PROVIDER_PATTERNS so test files do not trigger false positives. The previous regexes were `CommandLineOptionsProvider.*\.cs$` and `^PlatformCommandLineProvider.*\.cs$`, which matched `MyCommandLineOptionsProviderTests.cs` and `PlatformCommandLineProviderTests.cs`. Patterns now require the filename to end exactly at `Provider.cs`. * Tighten the workflow `paths:` filter the same way. Removed the trailing `*` from `*CommandLineOptionsProvider*.cs` and `PlatformCommandLineProvider*.cs` so unit-test edits don't kick off this workflow. * Consolidate the docstring's two `0` exit-code entries into one. The script is intentionally non-blocking, so all success paths return 0; spelling them out twice as separate exit codes was confusing. * Rename the env var from `BASE_SHA` to `BASE_REF`. The value is a named ref (`origin/main`), not a SHA, so the old name misled readers. The script's `--base` default still reads `$BASE_SHA` as a fallback for backward compatibility in case any local invocations relied on the old name. * 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. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The previous fix added an explanatory comment ABOVE the try/except, but github-code-quality's empty-except rule looks for a comment INSIDE the except body. Move/extend the comment so it lives next to the `pass`. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| # Named ref (not a SHA) — the script's --base default also reads | ||
| # $BASE_SHA for backward compatibility. | ||
| BASE_REF: origin/${{ github.event.pull_request.base.ref }} |
Summary
Adds a fast, non-blocking PR-time reminder: when a PR touches an
ICommandLineOptionsProviderimplementation but does not also touch any of the four--help/--infoacceptance-test expectation files documented in.github/copilot-instructions.md, the workflow surfaces a reminder via\and a::noticeannotation that shows up in the PR Checks tab.Why
copilot-instructions.md(CLI options guidelines) says:…and enumerates four expectation files:
test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/HelpInfoTests.cstest/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/HelpInfoAllExtensionsTests.cstest/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/MSBuild.KnownExtensionRegistration.cstest/IntegrationTests/MSTest.Acceptance.IntegrationTests/HelpInfoTests.csToday the policy is enforced only by the acceptance tests themselves, which run late in CI. By that point, fixing the expectations costs a follow-up push (and sometimes a CI re-run). This guard fires at PR creation/synchronize so the author can fold the fix into the same push.
Why non-blocking
The script always exits 0. Reasons:
privatemethod on a provider class).The reminder surfaces as a yellow ⚠ in the Checks tab, which is hard to miss without being a blocker.
What changed
.github/scripts/check_cli_help_drift.pygit diff --name-onlyoutput (or full unified diff), classifies paths into "provider" / "expectation file", and emits the reminder when providers are touched without expectations.--diff-fileflag for offline testing..github/workflows/cli-help-drift-reminder.ymlpull_requestworkflow,paths-scoped to providers + expectations + the script/workflow themselves.contents: readonly. Concurrency-cancelling per-PR.Validation
Locally tested with three synthetic diffs:
::noticeannotation, exit 0.The matching is name-based (
CommandLineOptionsProvider*.cs,PlatformCommandLineProvider*.cs,MSTestExtension.cs) rather than path-based, so it stays robust to folder moves.Risk
contents: readonly — workflow cannot mutate anything.🤖 Authored with GitHub Copilot CLI based on a one-month review-comment audit of microsoft/testfx.