Fix milestone tag audit PR parsing - #36703
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: fd8d3cc9-58a5-42b8-9c4e-562104b9a317
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 36703Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 36703" |
|
Azure Pipelines: 1 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
This PR fixes false-positive “merged PR” detections in the milestone drift/tag-audit tooling by tightening how PR numbers are extracted from git log output, preventing linked-issue references in commit subjects from being treated as PRs.
Changes:
- Added a shared
Get-PrNumbersFromGitLoghelper that only accepts a terminal(#N)PR suffix (with optional trailing whitespace) and deduplicates/sorts results. - Updated both tag-walk paths (
Get-PrNumbersBetweenTags,Get-PrNumbersReachableFromTag) to use the shared terminal-suffix parser. - Added unit and git-backed integration tests covering multiple-parenthesis subjects, malformed suffixes, trailing whitespace, deduplication, and the two tag-based call paths.
Show a summary per file
| File | Description |
|---|---|
| .github/scripts/Fix-MilestoneDrift.ps1 | Centralizes PR-number parsing from git log and restricts matches to terminal squash-merge suffixes to avoid false PR lookups. |
| .github/scripts/Fix-MilestoneDrift.Tests.ps1 | Adds regression and unmocked git integration tests ensuring both tag-based extraction paths share the corrected terminal-suffix behavior. |
Copilot's findings
- Files reviewed: 2/2 changed files
- Comments generated: 0
kubaflo
left a comment
There was a problem hiding this comment.
🔍 AI-generated review — automated multi-model consensus (Claude Opus 4.8 · GPT-5.5 · Gemini 3.1 Pro), run on behalf of @kubaflo. Independent reviews → cross-pollinated → synthesized. This is a COMMENT (not an approval).
✅ LGTM (3-model unanimous, high confidence)
A tightly-scoped, correct fix. The root cause is real and the anchored regex is the right remedy. Every claim was independently verified — I ran the Pester suite locally and all three models fetched fresh at head 16c4946:
| Check | Result |
|---|---|
Anchored regex \(#(\d+)\)\s*$ captures only the terminal squash suffix |
✅ Opus scanned ~200 real maui subjects; the regression signature is real — e.g. [Regression] Fix TypedBinding nested property re-subscription (#34428) (#34449): old greedy code queried linked-ref #34428 via the pulls API (→ false 404), new code keeps only #34449 |
Overflow safety ([int]::TryParse vs old throwing [int] cast) |
✅ strict improvement — (#999999999999999999999999999999) is skipped, not thrown; \d++TryParse cannot throw |
CRLF edge — trailing \r after -split "n"` |
✅ non-issue: \r ∈ \s, so \s*$ still anchors; verified directly and with a real repo at core.autocrlf true → returns only the terminal PR |
| Behavioral equivalence of both tag functions | ✅ both delegate to the shared Get-PrNumbersFromGitLog; git exit-code throw, HashSet dedup, Sort-Object all preserved |
| No external callers relying on old greedy behavior | ✅ both functions are internal to Fix-MilestoneDrift.ps1; all call sites use the result only for -in/count membership; the two other file-matches are comment-only |
| Test suite | ✅ 212/212 pass (incl. the 12 new cases); I re-ran the new blocks: 10 unit + 2 unmocked git init integration tests, all green |
The new tests are genuinely good — they cover the exact bug (issue-ref-before-PR), malformed/non-numeric/overflow refs, trailing whitespace, dedup+sort, and prove parity on real git log output via the unmocked integration block.
Non-blocking (low, pre-existing — @PureWeen your call)
- The
.Tests.ps1file isn't wired into CI. NoInvoke-Pesterexists in any of the 37 workflows, andfix-milestone-drift.ymlruns the script but not its tests — so these (valuable) tests give no automated regression protection. This is pre-existing (all sibling*.Tests.ps1are equally un-wired) and not introduced by this PR, so it doesn't block. Optional: add a workflow step runningInvoke-Pesterover.github/scripts/*.Tests.ps1on PRs touching that folder.
ℹ️ Per policy this bot posts a comment, not a formal approval — merge is @kubaflo's call.
Models: claude-opus-4.8 (LGTM) · gpt-5.5 (LGTM) · gemini-3.1-pro-preview (LGTM) · consensus: LGTM · confidence: high · findings: 1 low pre-existing / 0 blocking
kubaflo
left a comment
There was a problem hiding this comment.
LGTM — approving.
Independent 3-model review (Claude Opus 4.8 / GPT-5.5 / Gemini 3.1 Pro) reached unanimous LGTM with no findings. The fix is correct and well-scoped:
- Terminal-anchor is right.
\(#(\d+)\)\s*$targets exactly the GitHub squash-merge terminal PR token, so backport subjects like… (#34898) (#35031)now resolve to only the PR that landed on the branch (35031) instead of also fetching the linked issue via the pulls API — which was the source of the false 404s on preview6. - Real latent-bug fix. Replacing
[int]$m.Groups[1].Valuewith[int]::TryParseprevents anOverflowExceptioncrash of the whole audit on a huge digit run (covered by the(#999999999999999999999999999999)test). - CRLF-safe.
\s*$natively consumes any\rleft by-split "n", so noRegexOptions.Multiline` is needed. - No downstream risk. Dropping non-terminal references is exactly the intended milestone-drift behavior; the two callers delegate to the shared helper with unchanged signatures.
CI-gate: no required checks are red (maui-pr is correctly path-skipped for a .github/scripts change; only the advisory Build Analysis is pending). I also ran the added Pester block locally — 12/12 new tests pass (10 unit + 2 unmocked temp-git-repo integration asserting 34719,35031 between-tags and 100,34719,35031 reachable).
🤖 AI-generated review (GitHub Copilot CLI, on behalf of @kubaflo).
PureWeen
left a comment
There was a problem hiding this comment.
Adversarial review: no issues found
Three independent reviewers examined the two authoritative changed files, full parser call paths, boundary/overflow behavior, and test isolation. No correctness, security, or regression findings survived consensus.
The added coverage exercises the original issue-reference-before-PR defect, malformed and oversized references, trailing whitespace, deduplication/sorting, and both git-log callers. Existing reviews contained no unresolved error-level findings.
Note
Are you waiting for the changes in this PR to be merged?
It would be very helpful if you could test the resulting artifacts from this PR and let us know in a comment if this change resolves your issue. Thank you!
Description of Change
Root cause
Get-PrNumbersBetweenTagsandGet-PrNumbersReachableFromTagtreated every parenthesized(#N)reference in a commit subject as a merged PR. GitHub squash subjects can contain linked issue references before the actual PR suffix, so preview6 attempted to fetch issues #34716 and #34898 through the pulls API and logged two false 404 errors.Fix
Add one shared
Get-PrNumbersFromGitLoghelper used by both tag paths. It accepts only the final numeric(#N)suffix (with optional trailing whitespace), ignores malformed or non-terminal references, and preserves sorted deduplication.Preview6 proof
For
11.0.0-preview.5.26304.4..11.0.0-preview.6.26360.8:.NET 11.0-preview6([Enhancement] 3D graphics bindings #129).-CloseFixedIssuesremained a dry run: two open issues would close and 13 were already closed; no GitHub state was changed.Tests
Fix-MilestoneDrift.Tests.ps1: 212 passed.MilestoneTrigger.Tests.ps1: 116 passed.Non-goals
This fixes tag-audit parsing on
main. It does not address the missed historical preview6 workflow launch: the preview6 tag was created from a net11.0 commit that did not yet contain the already-mergedmaintag-push trigger. Branch-flow/backport policy is intentionally unchanged.Issues Fixed
N/A — this defect was discovered while auditing the preview6 tag.