Fix two low-severity regex edge cases in release-readiness matchers (Copilot follow-ups from #36213) - #36483
Conversation
Follow-ups to Copilot reviewer findings on #36213 that shipped into main. 1. Test-PluginEnabled (Get-PreviewReleaseReadiness.ps1): the enabled-plugin matcher was anchored to the start of a physical line ((?m)^\s*), so a *minified* single-line settings.json reported an enabled plugin as NOT enabled — a false negative that wrongly degrades to AVAILABLE_NOT_ENABLED. Anchor the key to a JSON boundary ({ , or whitespace) via a look-behind instead; comment avoidance is already handled by the string-aware Remove-JsoncComments scrub, so the line anchor was redundant. 2. Test-IsSdkBumpPr (Get-PreviewReadiness.ps1): the trailing \b in 'dotnet/(dotnet|sdk)\b' sits between 't' and '-', so 'Bump dotnet/dotnet-optimization …' was misclassified as an SDK bump. Use the (?![\w-]) boundary its sibling matchers already use (selectPin, Get-ComponentFlowSignal). Adds hermetic regression tests for both (minified/pretty/suffix/absent settings.json; dotnet-optimization collision + real dotnet/sdk-in-trailer). Suite: 853 passed / 0 failed (-SkipE2E). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 36483Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 36483" |
Skill Validation Results
✅ Skill Validation Results —
|
There was a problem hiding this comment.
Pull request overview
This PR makes two small, low-risk follow-up fixes to the release-readiness tooling to address regex edge cases that could cause incorrect classification in specific scenarios (minified Copilot settings.json, and dotnet/dotnet-optimization title collisions). It updates the matchers and adds targeted regression tests to lock the behavior in place.
Changes:
- Fix
Test-PluginEnabledto correctly detect enabled plugins even when Copilotsettings.jsonis minified/single-line (no longer anchored to physical line starts). - Fix
Test-IsSdkBumpPrto avoid misclassifyingdotnet/dotnet-optimizationas an SDK/VMR bump by using a stricter post-repo boundary. - Add hermetic regression tests covering both edge cases.
Show a summary per file
| File | Description |
|---|---|
.github/skills/release-readiness/tests/Test-ReleaseReadiness.ps1 |
Adds regression tests for the minified-settings plugin detection and the dotnet-optimization collision case. |
.github/skills/release-readiness/scripts/Get-PreviewReadiness.ps1 |
Tightens the SDK/VMR bump title matcher to avoid dotnet/dotnet-optimization false positives. |
.github/skills/dependency-flow/scripts/Get-PreviewReleaseReadiness.ps1 |
Adjusts plugin-enabled detection regex to work with minified JSON while still relying on JSONC comment scrubbing for comment avoidance. |
Copilot's findings
- Files reviewed: 3/3 changed files
- Comments generated: 0
kubaflo
left a comment
There was a problem hiding this comment.
🔍 AI-generated review (multi-model: Opus 4.8 · GPT-5.5 · Gemini 3.1 Pro), on behalf of @kubaflo.
✅ LGTM — approving
Two correct, well-tested regex-boundary fixes in the release-readiness matchers (skill/tooling only — no product code, no public API). Independently verified both:
1. Test-PluginEnabled — minified-JSON false negative. (?m)^\s*"key"… → (?<=[{,\s])"key"…. The look-behind requires the key's opening quote to sit on a JSON boundary ({, ,, or whitespace) rather than a physical line start, so a single-line {"enabledPlugins":{"dotnet-release-tracker@x":true}} now matches. Confirmed it still can't false-positive: for plugin dotnet-release-tracker, a suffix key like {"my-dotnet-release-tracker":true} has no boundary-preceded quote immediately before dotnet-, so it stays false (test asserts this). Fails safe either way — never a false enable.
2. Test-IsSdkBumpPr — hyphenated-sibling collision. …dotnet/(dotnet|sdk)\b → …dotnet/(dotnet|sdk)(?![\w-]). The bare \b sat between t and -, so Bump dotnet/dotnet-optimization was misread as an SDK/VMR bump; the (?![\w-]) look-ahead (matching the sibling selectPin/Get-ComponentFlowSignal matchers) excludes the hyphenated repo while a real dotnet/sdk later in the title still matches. Both cases covered by the new asserts.
Both changes only narrow-over-broad / widen-too-narrow in the safe direction, and the added guards in Test-ReleaseReadiness.ps1 are hermetic (throwaway HOME/USERPROFILE, no gh/network) and fail against the old patterns. Skill-eval + hermeticity gates green.
…Copilot follow-ups from dotnet#36213) (dotnet#36483) <!-- Please let the below note in for people that find this PR --> > [!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](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from this PR and let us know in a comment if this change resolves your issue. Thank you! ### What Two small follow-up fixes to the release-readiness reporting skill, closing two **low-severity** edge cases that the GitHub Copilot reviewer flagged on dotnet#36213 and that shipped into `main`. Both are docs/skill-only (PowerShell + tests) — no product code, no public API. **1. `Test-PluginEnabled` — minified `settings.json` false negative** `.github/skills/dependency-flow/scripts/Get-PreviewReleaseReadiness.ps1` The enabled-plugin matcher was anchored to the start of a physical line (`(?m)^\s*`). A **minified / single-line** `settings.json` (e.g. `{"enabledPlugins":{"dotnet-release-tracker@dotnet-release":true}}`) therefore failed to match, so an *enabled* plugin was reported as **not** enabled (a false-negative that wrongly degrades to `AVAILABLE_NOT_ENABLED`). It fails safe — it never produces a false *enabled* — but it's still wrong for anyone whose settings file isn't pretty-printed. Fix: anchor the key to a JSON boundary (`{`, `,`, or whitespace) via a look-behind `(?<=[{,\s])` instead of a line start. Comment-avoidance is already handled by the string-aware `Remove-JsoncComments` scrub applied just below, so the line anchor was redundant. **2. `Test-IsSdkBumpPr` — `dotnet-optimization` collision** `.github/skills/release-readiness/scripts/Get-PreviewReadiness.ps1` `'(?i)\bBump\b.*dotnet/(dotnet|sdk)\b'` — the trailing `\b` sits between `t` and `-`, so `Bump dotnet/dotnet-optimization …` was misclassified as an SDK/VMR bump (which would attach a spurious "verify blessed build locally" emphasis). Fix: use the `(?![\w-])` boundary that its sibling matchers already use (`selectPin`, `Get-ComponentFlowSignal`). Practically dormant on maui today (real dep-flow PRs are titled `[netN.0] Update dependencies from…`), but now correct. ### Tests Added hermetic regression guards in `Test-ReleaseReadiness.ps1`: - `Test-PluginEnabled`: minified, pretty, suffix-only-key (no false positive), and absent-entry cases (writes fixtures into a throwaway `HOME`/`USERPROFILE`, restored in `finally`; no `gh`/network). - `Test-IsSdkBumpPr`: `dotnet/dotnet-optimization` does **not** collide → `false`; a real `dotnet/sdk` later in the same title still → `true`. This mirrors the `Get-ComponentFlowSignal` collision guard that already existed — the sibling matcher just never got the parallel assertion (the exact gap this closes). Suite: **853 passed / 0 failed** (`-SkipE2E`). ### Why low-risk Skill/tooling only. Fix 1 only ever *widens* a previously-too-narrow match and still can't produce a false enable; Fix 2 only *narrows* an over-broad match to exclude a hyphenated sibling. Both are covered by new tests that fail against the old patterns. Co-authored-by: PureWeen <223556219+Copilot@users.noreply.github.com>
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!
What
Two small follow-up fixes to the release-readiness reporting skill, closing two low-severity edge cases that the GitHub Copilot reviewer flagged on #36213 and that shipped into
main. Both are docs/skill-only (PowerShell + tests) — no product code, no public API.1.
Test-PluginEnabled— minifiedsettings.jsonfalse negative.github/skills/dependency-flow/scripts/Get-PreviewReleaseReadiness.ps1The enabled-plugin matcher was anchored to the start of a physical line (
(?m)^\s*). A minified / single-linesettings.json(e.g.{"enabledPlugins":{"dotnet-release-tracker@dotnet-release":true}}) therefore failed to match, so an enabled plugin was reported as not enabled (a false-negative that wrongly degrades toAVAILABLE_NOT_ENABLED). It fails safe — it never produces a false enabled — but it's still wrong for anyone whose settings file isn't pretty-printed.Fix: anchor the key to a JSON boundary (
{,,, or whitespace) via a look-behind(?<=[{,\s])instead of a line start. Comment-avoidance is already handled by the string-awareRemove-JsoncCommentsscrub applied just below, so the line anchor was redundant.2.
Test-IsSdkBumpPr—dotnet-optimizationcollision.github/skills/release-readiness/scripts/Get-PreviewReadiness.ps1'(?i)\bBump\b.*dotnet/(dotnet|sdk)\b'— the trailing\bsits betweentand-, soBump dotnet/dotnet-optimization …was misclassified as an SDK/VMR bump (which would attach a spurious "verify blessed build locally" emphasis). Fix: use the(?![\w-])boundary that its sibling matchers already use (selectPin,Get-ComponentFlowSignal). Practically dormant on maui today (real dep-flow PRs are titled[netN.0] Update dependencies from…), but now correct.Tests
Added hermetic regression guards in
Test-ReleaseReadiness.ps1:Test-PluginEnabled: minified, pretty, suffix-only-key (no false positive), and absent-entry cases (writes fixtures into a throwawayHOME/USERPROFILE, restored infinally; nogh/network).Test-IsSdkBumpPr:dotnet/dotnet-optimizationdoes not collide →false; a realdotnet/sdklater in the same title still →true. This mirrors theGet-ComponentFlowSignalcollision guard that already existed — the sibling matcher just never got the parallel assertion (the exact gap this closes).Suite: 853 passed / 0 failed (
-SkipE2E).Why low-risk
Skill/tooling only. Fix 1 only ever widens a previously-too-narrow match and still can't produce a false enable; Fix 2 only narrows an over-broad match to exclude a hyphenated sibling. Both are covered by new tests that fail against the old patterns.