Optimize PR validation for XML documentation changes - #10763
Conversation
There was a problem hiding this comment.
Pull request overview
Optimizes validation for XML-documentation-only C# changes by classifying diffs and reducing CI execution.
Changes:
- Adds a fail-closed Roslyn-based PowerShell classifier.
- Routes documentation-only changes through Release build lanes while skipping tests and multi-OS jobs.
Show a summary per file
| File | Description |
|---|---|
eng/classify-xml-doc-change.ps1 |
Classifies XML-documentation-only diffs. |
azure-pipelines.yml |
Uses classification to reduce validation jobs. |
Review details
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
Note
🤖 Automated review by GitHub Copilot. Generated by the Expert Code Review workflow. To request a follow-up action, reply by tagging @copilot directly.
| # | Dimension | Verdict |
|---|---|---|
| 13 | Test Completeness & Coverage | 🟢 1 NIT |
| 17 | Documentation Accuracy | 🟢 1 NIT |
| 20 | Build Infrastructure & Dependencies | 🟡 1 MODERATE |
✅ 19/22 dimensions clean. 3 dimensions N/A (Analyzer Quality, IPC Wire Compatibility, MSBuild Authoring — no relevant files changed).
Summary: Well-designed fail-closed classifier with good self-tests and correct pipeline condition logic. The Roslyn assembly loading is implicit (relying on agent environment), but the bash wrapper's if ! guard ensures graceful degradation. The pipeline conditions correctly route xmlDocsOnly PRs through minimal validation while preserving full validation for any ambiguity.
- Build Infrastructure — Roslyn assembly loading is implicit; consider explicit load with clear error
- Test Completeness — self-tests could cover removed-doc and preprocessor-directive cases
- Documentation — Release lane comment could clarify "validation" means compilation only (tests skipped)
There was a problem hiding this comment.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
eng/classify-xml-doc-change.ps1:272
- The self-tests are not executed by any repository pipeline invocation: the only call site invokes this script with
-Base/-Head. As a result, regressions in this safety-critical classifier (including the raw-string and mixed-comment cases) will not fail CI. Add a-SelfTeststep toDetectChangesbefore using the classifier result.
if ($SelfTest) {
Invoke-SelfTest
exit 0
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
bf064e3 to
3cd7a9d
Compare
There was a problem hiding this comment.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
eng/classify-xml-doc-change.ps1:144
- This explicitly rejects every sample C# file, so
xmlDocsOnlyis false for sample documentation-only PRs andWindowsSamplesstill runs both Release and Debug (along with the normal Debug lane). That contradicts the PR description's stated routing of sample XML-documentation-only changes through only the WindowsSamples Release lane. If retaining full validation for samples is intentional, update the PR description to remove that claimed optimization; otherwise sample classification and matrix gating still need to be implemented.
-not $Path.StartsWith("samples/", [System.StringComparison]::Ordinal))
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Use a fail-closed Roslyn classifier to identify documentation-only C# diffs and reduce them to the applicable Release build lane. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7509ec42-006c-4962-8918-73c3fae329d9
Load PowerShell's Roslyn assemblies explicitly, expand classifier self-tests, and clarify that the optimized Release lane skips tests. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7509ec42-006c-4962-8918-73c3fae329d9
Associate significant trivia with its token and side, cover directive relocation, and run classifier self-tests in the detection job. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7509ec42-006c-4962-8918-73c3fae329d9
Exclude sample-tree changes from the shortcut and reject XML include documentation so non-public samples and platform-sensitive include paths retain their existing validation. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7509ec42-006c-4962-8918-73c3fae329d9
Exercise multi-file documentation, mixed code, added file, and deleted file diffs in a temporary Git repository. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7509ec42-006c-4962-8918-73c3fae329d9
1107122 to
96f1d2a
Compare
Extract and self-test the classifier from the PR target branch, and fail closed when that trusted version is unavailable or cannot classify the diff. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7509ec42-006c-4962-8918-73c3fae329d9
PRs that change only C# XML documentation currently run the full multi-OS build and test matrix even though their meaningful compiler input is limited to generated documentation. PR #10740 is a representative example.
Approach
<include>documentation because their build coverage and path semantics can be platform-sensitive.The target branch does not contain the classifier until this PR merges, so this PR intentionally exercises the full validation matrix. Subsequent PRs can use the optimized path from the trusted merged version.
Validation