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 5 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
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>
Match testfx's existing PowerShell tooling convention (eng/common/, eng/scripts/, .github/scripts/scan-duplicates.ps1) instead of bringing in a Python runtime dependency for a single reminder script. - Adds .github/scripts/check-cli-help-drift.ps1 with the same logic: read a name-only or unified diff, classify each path as either a CLI-options provider or one of the four documented help-expectation files, emit a soft `::notice` reminder when providers changed without expectations. - Removes .github/scripts/check_cli_help_drift.py. - Drops actions/setup-python from the workflow and switches the run step to `shell: pwsh` (cross-platform on ubuntu-latest). - Updates `paths:` to track the new .ps1 filename. - Preserves the BASE_REF / BASE_SHA env-var fallback so existing workflow invocations keep working. Smoke-tested locally with synthetic name-only diff inputs: (a) no providers → exit 0 clean, (b) provider only → reminder + notice, (c) provider + expectation → satisfied message, (d) Tests.cs ignored. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The previous workflow set `BASE_REF: origin/${{ github.event.pull_request.base.ref }}`,
but actions/checkout on `pull_request` checks out a detached merge commit
and doesn't guarantee the `origin/<base-branch>` remote-tracking ref
exists locally. When it's missing, `git diff origin/<base>...HEAD` fails
and this "non-blocking reminder" turns into a hard failure.
`github.event.pull_request.base.sha` is always present in the PR event
payload and is reachable in the checkout (the merge commit has the base
SHA as one of its parents), so the diff resolves reliably and the job
stays advisory.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| - '**/*CommandLineOptionsProvider.cs' | ||
| - '**/PlatformCommandLineProvider.cs' | ||
| - '**/MSTestExtension.cs' |
| $name = [System.IO.Path]::GetFileName($norm) | ||
| foreach ($pattern in $ProviderPatterns) { | ||
| if ($pattern.IsMatch($name)) { | ||
| $providers.Add($norm) | ||
| break | ||
| } | ||
| } | ||
| } |
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.