Skip to content

Release-readiness: cross-major preview leak fix, milestone blocker, E2E refresh, and hoisted candidate-PR section - #36172

Merged
PureWeen merged 5 commits into
mainfrom
pureween/preview7-cross-major-p0-leak
Jul 5, 2026
Merged

Release-readiness: cross-major preview leak fix, milestone blocker, E2E refresh, and hoisted candidate-PR section#36172
PureWeen merged 5 commits into
mainfrom
pureween/preview7-cross-major-p0-leak

Conversation

@PureWeen

@PureWeen PureWeen commented Jun 26, 2026

Copy link
Copy Markdown
Member

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!

Summary

A set of related improvements to the release-readiness tooling (.github/skills/release-readiness/). Four logical changes, all additive, spanning the preview engine, the SR engine, and the shared test suite:

  1. Fix a cross-major previewN scope leak (preview engine)
  2. Flag a missing preview milestone as a ship-readiness blocker (preview engine)
  3. Hoist the SR "Candidate PR" into a prominent readiness section (SR engine)
  4. Refresh drifted E2E snapshot assertions to current release state (tests)

Full suite runs 758/0 locally (offline). Files changed: Get-PreviewReadiness.ps1, Get-ReleaseReadiness.ps1, Test-ReleaseReadiness.ps1.


1. Cross-major preview-number leak

A .NET 10 p/0 regression issue (#31960) was surfacing as a 🔥 P/0 blocker on the .NET 11 preview7 tracker.

Root causeTest-IssueReleaseRelevant (Get-PreviewReadiness.ps1) accepts an issue if its title/milestone/labels contain a bare previewN substring:

if ($haystack -match "(?i)preview\s*$Preview|preview$Preview") { return $true }

That match is major-ambiguous — every major has a previewN. The regression label regressed-in-10-preview7 (a .NET 10 label, milestone .NET 10 SR12) matched when surveying .NET 11 preview7, leaking #31960 onto the wrong tracker.

Fix — add Test-IssueHasForeignMajor: when a previewN match is found, reject it if the issue carries a contradicting foreign-major signal — a regressed-in-<M>-* label, a .NET <M> milestone, or an <M>.0 token whose major differs from the surveyed major. Majors are bounded to 6..99 so build numbers (e.g. …preview.7.26324.11) can't register as a major. The detector is anchored to .NET-shaped tokens only (review round 3dfac6ceca). The wide net is intentionally preserved: genuinely major-less previewN mentions stay relevant, and same-major issues are still caught by the existing major signal first.

2. Missing preview milestone → ship blocker

If a preview tracker is generated but the corresponding GitHub milestone doesn't exist yet, that's a real gap — the milestone must be created right away, not at cut time. The preview engine now emits a P/0 ship-readiness blocker on the tracker when the expected milestone is absent, so it can't be silently forgotten.

3. Hoisted SR "Candidate PR" section

In SR candidate (pre-cut) mode, the single most important PR in the cycle is the open "Candidate" PR that promotes a specific main commit as the SR cut point — the SR branch can't be cut until it merges. It was previously buried as a lone WATCH row in the bottom ship-checks table plus a sparse low section.

It's now surfaced directly under the Blocking summary as a ## 🚩 Candidate PR — SRx cut point (10.0.x0) section with:

  • A live status table: 🟢 Open · ✅ Ready/📝 Draft · mergeable/⚠️ conflicts, PR age ("2026-06-08 (26 days ago)"), last-update, and review decision.
  • A staleness callout (⚠️ Stale (N days old) once ≥14 days) tied to the ship-target date — a long-open cut PR likely points at a now-stale main commit.
  • The SR version base (e.g. 10.0.90) derived from the prior SR branch (…-sr8 → SR9 → targetSr*10).

Design details:

  • New Get-CandidatePrResolution runs the gh query + maintainer spoof-gate once per report; both the WATCH ship-check and the section consume the shared result ($data['candidatePr']) — no duplicate round-trips, no twin-drift.
  • Verdict semantics unchanged: an open/missing candidate PR stays WATCH (normal cycle hygiene), never BLOCKED. The captain decides when to merge.
  • candidatePr is JSON-serialized for downstream automation but excluded from the semantic hash, so the daily-ticking age text doesn't churn the tracker.
  • The noisy open-main-PR dump is now suppressed in candidate mode (kept for live-SR mode); the section links only the maintainer-gated candidate(s) plus a pointer to the full PR list.
  • Maintainer spoof-gate is fail-closed: an unreadable author_association excludes the PR and is counted as unverifiable (distinct from a confirmed non-maintainer spoofer), so a transient blip during a real cut isn't mislabeled.

4. E2E snapshot refresh

Refreshes drifted end-to-end snapshot assertions in the test suite to match current release state, fixing failures caused by real-world release progression rather than code changes.


Tests

  • Cross-major (12 assertions): the exact [.NET10] Horizontal scrollbar appears during pull-to-refresh on C4 #31960 shape is not relevant for Major 11 / preview7 but is for its own Major 10 / preview7; same-major keep-cases; bare-mention wide-net; build-number poisoning guard; the preview6 variant; direct Test-IssueHasForeignMajor coverage.
  • Candidate-PR section: migrated the renderer tests to the hoisted section (the human-notes marker-forgery defense is re-pointed at the candidate table cell so it's still exercised); added 16 Get-CandidatePrResolution unit tests (mode transitions, version-base derivation, spoofer vs unverifiable classification, query-failed/skip short-circuits).
  • Full suite: 758/0 offline.

Scope

Additive across three files in .github/skills/release-readiness/. No behavior change for genuinely same-major issues, and SR verdict semantics are unchanged (candidate PR stays WATCH).

…lter

Test-IssueReleaseRelevant matched a bare "previewN" phrase with no major
anchoring, so a .NET 10 p/0 issue labelled `regressed-in-10-preview7`
(e.g. #31960, milestone `.NET 10 SR12`) leaked onto the .NET 11 preview7
tracker as a 🔥 P/0 blocker. Every major has a previewN, so the substring
alone is major-ambiguous.

Add Test-IssueHasForeignMajor: when a previewN match is found, reject it if
the issue carries a contradicting foreign-major signal (a `regressed-in-<M>-*`
label, a `.NET <M>` milestone, or an `<M>.0` token whose major differs from
the surveyed major). Majors are bounded to 6..99 so build numbers can never
register as a major. The wide net is preserved for genuinely major-less
"previewN" mentions, and genuine same-major issues are still caught by the
existing major signal first.

Adds 12 regression assertions (the #31960 shape, same-major keep cases, the
bare-mention wide-net case, a build-number poisoning guard, and direct
Test-IssueHasForeignMajor coverage). Full preview-engine test region: 54/0.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings June 26, 2026 16:02
@github-actions

Copy link
Copy Markdown
Contributor

🚀 Dogfood this PR with:

⚠️ WARNING: Do not do this without first carefully reviewing the code of this PR to satisfy yourself it is safe.

curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 36172

Or

  • Run remotely in PowerShell:
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 36172"

@github-actions

github-actions Bot commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Skill Validation Results

@PureWeen — new skill validation results are available based on this last commit: 5126c9d.
To request a fresh validation after new comments or commits, comment /evaluate-skills.

Overall Passed Static Passed LLM Skipped Skills 20 Agents 6

Skill Validation Results5126c9d · Fix cross-major preview-number leak in release-readiness relevance filter · 2026-07-05T15:27:29Z

✅ Static Checks Passed

Skills: 20 | Eval specs linted: 7

Full lint output
── .github/skills/agentic-labeler/tests/eval.vally.yaml
npm warn deprecated prebuild-install@7.1.3: No longer maintained. Please contact the author of the relevant native addon; alternatives are available.
✔ .github/skills/agentic-labeler/tests/eval.vally.yaml is valid
── .github/skills/code-review/tests/eval.capability.vally.yaml
✔ .github/skills/code-review/tests/eval.capability.vally.yaml is valid
── .github/skills/code-review/tests/eval.vally.yaml
✔ .github/skills/code-review/tests/eval.vally.yaml is valid
── .github/skills/code-review/tests/hermeticity.vally.yaml
✔ .github/skills/code-review/tests/hermeticity.vally.yaml is valid
── .github/skills/evaluate-pr-tests/tests/eval.vally.yaml
✔ .github/skills/evaluate-pr-tests/tests/eval.vally.yaml is valid
── .github/skills/try-fix/tests/eval.vally.yaml
✔ .github/skills/try-fix/tests/eval.vally.yaml is valid
── .github/skills/verify-tests-fail-without-fix/tests/eval.vally.yaml
✔ .github/skills/verify-tests-fail-without-fix/tests/eval.vally.yaml is valid

⏭️ LLM Evaluation: Skipped

No changed skills with eval specs found.

🔍 Full results and investigation steps

@github-actions github-actions Bot added the area-infrastructure CI, Maestro / Coherency, upstream dependencies/versions label Jun 26, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes an issue in the release-readiness preview tracker engine where a bare previewN substring could incorrectly mark issues from a different .NET major as relevant (e.g., .NET 10 issues leaking into a .NET 11 preview7 tracker). The PR adds a foreign-major detector used to gate previewN-only matches and adds regression tests for the scenarios described.

Changes:

  • Add Test-IssueHasForeignMajor and use it to reject previewN matches when the issue text clearly indicates a different .NET major.
  • Extend Test-ReleaseReadiness.ps1 with regression assertions covering the leak scenario and key keep-cases.
Show a summary per file
File Description
.github/skills/release-readiness/scripts/Get-PreviewReadiness.ps1 Adds foreign-major detection and applies it as a guard to previewN-only relevance matching.
.github/skills/release-readiness/tests/Test-ReleaseReadiness.ps1 Adds unit/regression assertions for cross-major previewN leakage and related edge cases.

Copilot's findings

  • Files reviewed: 2/2 changed files
  • Comments generated: 2

[int]$Major
)

foreach ($m in [regex]::Matches($Haystack, "(?i)(?:net\s*|regressed-in-)(\d+)|(\d+)\.0(?:\.|\b)")) {
-Expected $true -Actual (Test-IssueReleaseRelevant -Issue $bareMention -Major 11 -Preview 7)

# A build-number that contains the preview digits must not register as a major.
$buildNumIssue = New-RelevanceIssue -Title 'fails on 11.0.0-preview.7.26324.11' -Milestone $null -Labels @()

@kubaflo kubaflo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note

🤖 This review was automatically generated by a multi-model AI review system (Claude Opus 4.8, GPT-5.5, Gemini 3.1 Pro). Three models reviewed independently, then cross-pollinated. Findings were empirically verified by running the code.

Multi-Model Review — PR #36172

Verdict: ⚠️ NEEDS_CHANGES
Confidence: High (2 of 3 models flagged the blocker independently; confirmed by running the code)

Summary

The fix correctly closes the reported #31960 cross-major leak — I verified all 12 new asserts pass, and the priority ordering (own-major check first, foreign-major gate only when no own-major signal) is sound. It's a net improvement over the status quo. However, the foreign-major detector's unanchored (\d+)\.0 branch is too broad for MAUI triage data, and introduces a false-negative class that drops exactly the issues this tool exists to surface.

❌ Blocker — Unanchored (\d+)\.0 drops genuine p/0 issues that mention OS/tool versions

Test-IssueHasForeignMajor treats any X.0 token (6 ≤ X ≤ 99) as a foreign .NET major. MAUI issue titles routinely contain OS/tool versions like Android 15.0, iOS 18.0, macOS 14.0, VS 17.0. An untriaged p/0 preview7 regression mentioning one of those — with no .NET 11 milestone yet — is silently dropped.

I confirmed this live (dot-sourced the script, surveyed Major 11 / Preview 7):

Issue (p/0, no milestone) relevant Why
App crashes on Android 15.0 since preview7 False 15.0 → foreign major 15
iOS 18.0 layout broke in preview7 False 18.0 → foreign major 18
macOS 14.0 hang in preview7 False 14.0 → foreign major 14
crash since preview7 (control) True ✅
same, but milestone .NET 11.0 True ✅ own-major check rescues it

This directly contradicts the function's own documented invariant ("false negatives are worse than false positives for release-readiness triage") — and the dropped population is precisely the fresh, untriaged, milestone-less p/0 reports the scan is meant to catch. Both Opus and GPT flagged this independently.

✅ Verified fix (drop the bare .0 branch)

The .0 branch isn't needed for #31960 — that leak is caught entirely by the regressed-in-(\d+) and net\s*(\d+) anchors. Anchoring the detector and dropping the bare branch:

[regex]::Matches($Haystack, "(?i)(?:net\s*|\.net\s*|regressed-in-)(\d+)")

I ran this hardened form against 10 cases — it passes all 5 of your direct foreign-major asserts, removes the Android 15.0/iOS 18.0/macOS 14.0/VS 17.0 false positives, and still flags a genuine .NET 8 mention. It also biases unmatched cases toward the wide net — the explicitly desired direction.

💡 Secondary (non-blocking, worth a conscious decision)

  • Inherent false-negative: Animation broke in preview7 (worked fine in .NET 8) — milestone-less, surveyed M11/P7 — is dropped, because a token-presence detector can't tell "this is a net8 issue" from "this net11 issue regressed vs net8". regressed from <older LTS> is very common phrasing. The anchored fix above doesn't solve this one; recommend a conscious accept + a documenting test.
  • Test gap: none of the 12 asserts cover the dangerous middle — a relevant own-major issue that names a foreign version with no milestone. Adding preview7 on iOS 18.0, no milestone → relevant would lock in the hardened behavior.
  • xcode asymmetry (pre-existing): xcode is in the own-major regex, so Xcode 16.0 issues survive at step 1 while structurally identical macOS 15.0 issues don't. The hardening above makes platform/tool versions uniformly non-fatal and removes the inconsistency.
Model Verdict Confidence
Claude Opus 4.8 NEEDS_DISCUSSION medium
GPT-5.5 NEEDS_CHANGES high
Gemini 3.1 Pro LGTM high

Reviewed at head ffa8dc9a. CI: license/cla ✅ pass; maui-pr skipping (by-design .github/** path-exclusion). The release-readiness Pester suite isn't CI-gated for unit asserts, so the empirical runs above are the verification of record.

Comment thread .github/skills/release-readiness/scripts/Get-PreviewReadiness.ps1 Outdated
PureWeen and others added 3 commits July 4, 2026 08:22
The multi-model review flagged a false-negative blocker in
Test-IssueHasForeignMajor: the unanchored `(\d+)\.0` branch treated any
`X.0` token (6..99) as a foreign .NET major. MAUI issue titles routinely
carry OS/tool versions — `Android 15.0`, `iOS 18.0`, `macOS 14.0`,
`VS 17.0` — so a still-untriaged p/0 previewN regression mentioning one
(no `.NET 11` milestone yet) was silently dropped from the very scan that
exists to surface it, violating the "false negatives are worse" invariant.

Fix (reviewer's verified form):
- Drop the `(\d+)\.0` branch; recognize a foreign major only when the
  digits are anchored to an explicit .NET token: net / .net / regressed-in-.
- Add a null/whitespace haystack guard (returns $false).
- Keep the 6..99 bound as defense-in-depth behind the anchor.
- #31960 is still correctly rejected (title `[.NET10]`, milestone
  `.NET 10 SR12`, and label `regressed-in-10-preview7` each anchor major 10).

Tests:
- Retitle the build-number case so it actually reaches the previewN +
  foreign-major guard path (the old `11.0.0-...` title returned early via
  the own-major regex and never exercised the guard).
- Add must-not-drop relevance asserts for `Android 15.0` / `iOS 18.0`
  preview7 p/0 issues, and direct detector asserts for Android/iOS/VS
  versions (not foreign), a genuine anchored `.NET 8` amid OS noise (foreign),
  an out-of-range `net26324` (not foreign), and whitespace-only (false).

Preview-engine region: +8 assertions, all green; no regressions (the 12
remaining suite failures are pre-existing SR-engine live-data drift,
identical with and without this change).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The E2E detection block in Test-ReleaseReadiness.ps1 pins a hand-maintained
snapshot of the live tracker landscape. Reality advanced one cycle since it
was last refreshed, so 12 assertions failed on main (and turned the PR's
`Validate (PR)` check red) — none of them related to the foreign-major fix:

net10:
- SR8 shipped 2026-07-03 (tag 10.0.80), so its in-flight tracker retired just
  like SR7 before it. The tracker set is now {SR9 candidate} — 1 tracker, not
  {SR8,SR9}. Replaced the SR8 in-flight assertions with an SR8-absent (shipped)
  check and updated the count.
- highestShippedTag drifted 10.0.71 -> 10.0.80. Rather than re-freeze another
  literal that rots on the next SR, derive the expected value from the same
  local tags the detector reads (highest stable N.0.M tag), via the repo path
  the detector reports in its JSON. Applied to both the single-major and
  -AllActiveMajors net10 assertions.

net11:
- preview6 was cut (branch release/11.0.1xx-preview6 exists) so it moved
  candidate -> in-flight (surveyRef/mode/title/branchExists changed), and
  preview7 is now the candidate from net11.0. net11 has 2 preview trackers,
  not 1. Refreshed the preview6 assertions to their in-flight values and added
  a full preview7 candidate block (mirrors preview6's former role), selecting
  each tracker by previewNumber instead of array index so ordering can't
  silently swap them.

Robustness additions so this rots less often:
- Get-ExpectedHighestShippedTag derives highestShippedTag from git ground truth.
- A shipped-exclusion invariant asserts every active SR tracker's expectedTag
  does not yet exist as a git tag (honors the non-uniform patch convention,
  e.g. SR7=10.0.71, SR8=10.0.80).

Full suite now: Passed 730 / Failed 0 (was 705/12).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The preview readiness engine had no milestone-existence check — it only read
each issue's milestone title as a relevance signal. So if a preview tracker
existed but its GitHub milestone (e.g. `.NET 11.0-preview7`) did not, nothing
surfaced it. The SR lane already covers this (Get-MilestoneHygieneChecks), and
in candidate mode it treats the tracker's OWN milestone as current-cycle →
BLOCKED. This ports that coupling to the preview lane.

Policy: a preview release-readiness tracker and its milestone are coupled — if
a tracker exists, the `.NET <major>.0-preview<N>` milestone must exist right
away (not deferred to cut time), otherwise fixed issues have nowhere to land
and the release-notes generator has nothing to query.

- Get-AllMilestones: fetch open+closed milestones via `gh api ... --paginate`,
  returning a Success/Data envelope so a gh outage is distinguishable from
  "zero milestones" (mirrors the SR-lane helper).
- Test-PreviewMilestoneExists: resolves whether the tracker's own preview
  milestone exists. Accepts the modern `.NET <major>.0-preview<N>` and legacy
  `<major>.0-preview<N>` titles (case-insensitive); a gh failure returns
  QueryFailed so the caller emits UNKNOWN, never a false BLOCK.
- New top-level check "Milestone for preview<N> (<title>)": missing → BLOCKED
  (flows into the 🔴 Blocking section and escalates the verdict) with a
  copy-paste `gh api ... milestones` create action; present → READY; gh
  failure → UNKNOWN. Runs independent of branch/survey state (candidate and
  in-flight alike), since milestone existence is repo-global.

Verified against live data: `.NET 11.0-preview6` reads present, the missing
`.NET 11.0-preview7` reads absent (→ BLOCKED). 10 new deterministic unit tests
(Get-AllMilestones stubbed — no network) cover present/missing/legacy/
case-insensitive/query-failure. Full suite: Passed 740 / Failed 0.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings July 4, 2026 14:51

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot's findings

  • Files reviewed: 2/2 changed files
  • Comments generated: 1

Comment on lines +683 to +686
foreach ($m in [regex]::Matches($Haystack, "(?i)(?:net\s*|\.net\s*|regressed-in-)(\d+)")) {
$val = 0
if ([int]::TryParse($m.Groups[1].Value, [ref]$val) -and $val -ge 6 -and $val -le 99 -and $val -ne $Major) {
return $true
kubaflo
kubaflo previously approved these changes Jul 4, 2026

@kubaflo kubaflo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 AI-generated review — multi-model panel (Claude Opus 4.8 · GPT-5.5 · Gemini 3.1 Pro), independent reviews + cross-pollination. Round 2. Findings empirically verified (dot-sourced + ran the script/tests).

✅ LGTM — unanimous (high confidence)

You fixed my R1 blocker exactly, and it's empirically confirmed:

R1 blocker → fixed

Test-IssueHasForeignMajor now anchors to .NET tokens only — (?i)(?:net\s*|\.net\s*|regressed-in-)(\d+) — dropping the unanchored (\d+)\.0 branch. Verified live: untriaged p/0 preview7 issues mentioning OS/tool versions (Android 15.0, iOS 18.0, macOS 14.0, VS 17.0, no milestone) are now kept at M11/P7, while #31960 (.NET 10 SR12 + regressed-in-10-preview7) is still correctly dropped at M11 (and kept at M10). The wide-net invariant is restored.

New milestone-blocker feature — sound + fail-closed 👍

Get-AllMilestones + Test-PreviewMilestoneExists correctly distinguish the three states: a gh/API outage → QueryFailed → UNKNOWN (neither a false BLOCK nor a silent pass — the important one), a genuine zero-milestones → BLOCKED, and an existing current/legacy milestone → READY. Opus verified the exact-match forms against live names (.NET 11.0-preview6, 9.0-preview5) with cross-major correctly rejected. Nice defensive design on the outage path.

Verification

  • 670 / 670 tests pass (under TZ=UTC); the snapshot refresh is legitimate current-state drift (verified: tags 10.0.71/10.0.80 exist → SR8 shipped; preview6 branch exists / preview7 absent) — it drift-proofs the tests rather than masking anything.
  • CI greenValidate (PR) passes; maui-pr skips by-design (.github/skills/**).
Model Verdict Confidence
Claude Opus 4.8 LGTM high
GPT-5.5 LGTM medium
Gemini 3.1 Pro LGTM high

💡 Non-blocking suggestions (Opus)

  • Add a leading word boundary to the net anchor (\bnet\s*(\d+) / \.net). As-is, net matches as a substring, so a title like subnet 24 or telnet 10 (+ a 6–99 number) could register a foreign major and drop a real p/0 — the exact false-negative class this tool calls its worst outcome. Pre-existing (the net\s* anchor predates this PR), low-probability in MAUI titles, but \bnet would harden it.
  • Closed-milestone false-pass: Test-PreviewMilestoneExists queries state=all and matches on title, so a manually-closed .NET X.0-previewN milestone reports Exists=true → READY even though fixed issues can't easily land on it. Very low probability; consider surfacing a closed match as WATCH/CLEANUP instead of READY.
  • Unit-test the verdict wiring — the QueryFailed→UNKNOWN / missing→BLOCKED / exists→READY mapping is only exercised via the network E2E block; a small stubbed unit test would lock it in.
  • Candidate mode blocks on a not-yet-created milestone by design (correct per the tracker⇒milestone coupling) — consider distinct wording for candidate-vs-in-flight to avoid operator alarm.

Reviewed at head 3b1cd89d. CI: Validate (PR) ✅ pass, license/cla ✅; maui-pr skipping (by-design). Thanks for the clean fix + the well-guarded new check.

Comment thread .github/skills/release-readiness/scripts/Get-PreviewReadiness.ps1

@PureWeen PureWeen left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adversarial Review — PR #36172

Methodology: 3 independent reviewers (deep‑reasoning, fast‑pattern, and frontier‑calibration models) reviewed the diff in parallel, followed by an adversarial‑consensus pass and a dispute round on one contested finding. Findings were verified empirically by dot‑sourcing and running the functions under pwsh. Consensus markers below (N/3).

Scope note

The PR description covers only the first commit (the foreign‑major leak fix). Since then the branch has grown to three changes — (1) the foreign‑major guard, (2) the E2E snapshot refresh, and (3) a new preview‑milestone existence check (Get-AllMilestones + Test-PreviewMilestoneExists + a top‑level BLOCKED/READY/UNKNOWN check). The description says "two files, additive, no behavior change", but the milestone check is a new behavior‑affecting feature. Worth updating the description before merge.

Prior blocker — ✅ resolved

The earlier CHANGES_REQUESTED round (multi‑model + Copilot bot, at ffa8dc9a) flagged a ❌ blocker: the unanchored (\d+)\.0 branch dropped genuine p/0 issues that mention OS/tool versions (Android 15.0, iOS 18.0). The anchoring commit (3dfac6ce) resolved it — the detector now only recognizes majors bound to net / .net / regressed-in-, and the Android/iOS/macOS/VS X.0 false‑negatives are gone (verified). That blocker no longer applies to the current head.

Findings

⚠️ Should‑fix (1) — see the inline comment on Get-PreviewReadiness.ps1:683: the bare net\s* alternative still matches net as a word‑internal substring (subnet/telnet/internet/ethernet), which can drop a genuine p/0. Trivial lookbehind fix, 3/3 reviewers. Low real‑world probability, so not a merge blocker — but cheap insurance.

💡 Consider (non‑blocking):

  • Recall limitation — own‑major issue that name‑checks a prior major is dropped (Test-IssueReleaseRelevant / Test-IssueHasForeignMajor). Verified: "Works on net10 but breaks in preview7" (Major 11 / preview7, no .NET 11 milestone) → relevant=False, because the detector vetoes on any anchored foreign major anywhere in the text. This "worked in .NET 10, broke now" phrasing is common in regression reports. 3/3 reviewers agreed it's real, but all three converged on 💡 non‑blocking — it's the same accepted inherent limitation as the prior round's "...worked fine in .NET 8" sibling case. The tempting "precise fix" (bind the foreign major to the same previewN token) was judged unsound — it would re‑open the exact #31960 leak via a .NET 10 SR12 milestone + a preview7 title. Recommendation: keep the conservative veto, add a documenting test + a .NOTES callout, and rely on triage (adding a regressed-in-11-* label / .NET 11 milestone makes the own‑major check rescue the issue first).

  • E2E preview assertions will re‑drift (Test-ReleaseReadiness.ps1, E2E block). The refresh correctly moved the shipped tag to the drift‑proof Get-ExpectedHighestShippedTag helper, but net11's highestShippedPreviewTag (11.0.0-preview.5…), tracker‑count (2), and preview6 = in-flight are still pinned — they'll flip the moment preview6 ships / preview7 is cut, re‑introducing the same class of drift this PR just fixed. 3/3 reviewers. -SkipE2E‑gated and not a correctness bug, but extending the git‑derived approach to the preview tag would prevent the next drift. 2 of 3 rated 💡; 1 rated ⚠️.

  • Duplicated Get-AllMilestones "kept in sync" by comment only (Get-PreviewReadiness.ps1 vs Get-ReleaseReadiness.ps1). The two copies already differ (one takes a -Repo param, this one reads script‑scope $Repository); the doc‑comment asserts sync but nothing enforces it. 2/3 reviewers. Not a bug today ($Repository is a script param with a dotnet/maui default) — just a maintenance drift trap; a shared module or a matching -Repo param would close it.

Verified clean (no action)

Own‑major early‑return runs first (same‑major issues never reach the foreign‑major gate); the 6..99 bound correctly rejects build numbers; Get-AllMilestones puts query params in the URL (avoids the -f → POST/422 trap) and distinguishes outage from empty via its Success/Data envelope; Test-PreviewMilestoneExists normalizes case/.Trim() and surfaces gh failure as UNKNOWN, never a false BLOCKED; the milestone check is fully try/catch‑guarded and variable‑safe at its unconditional top‑level placement; no command injection (all gh calls use argument‑list splatting; the BLOCKED next‑action interpolates only integer‑derived values into fenced documentation, not an executed command); and the script only appends New-Check rows, so it cannot clobber the downstream "Release Captain Notes" human‑edit splice.

Test coverage

The changed relevance/milestone paths are covered by new unit tests (foreign‑major direct cases, boundary majors, empty/null haystack, milestone present/missing/query‑failure). Gap: no test locks in the two ⚠️/💡 cases above (substring net, and own‑major‑references‑prior‑major) — adding those would prevent regressions.

Verdict: COMMENT — no merge blockers; the prior ❌ blocker is resolved. One low‑probability ⚠️ should‑fix (trivial regex boundary) and three 💡 maintenance/robustness notes. Approval remains a human decision.


if ([string]::IsNullOrWhiteSpace($Haystack)) { return $false }

foreach ($m in [regex]::Matches($Haystack, "(?i)(?:net\s*|\.net\s*|regressed-in-)(\d+)")) {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Logic / Regressionnet\s* has no left boundary, so it matches net inside words (subnet, telnet, internet, ethernet, magnet) followed by digits, registering a false foreign major and silently dropping a genuine preview‑N p/0 issue — the tool's own stated worst outcome. Verified live: "Crash on Ethernet 8 adapter since preview7" (Major 11 / preview7, no .NET 11 token) → relevant=False.

Real‑world probability is low, but the consequence (dropping a p/0 from the release tracker) is high and the fix is trivial — anchor the bare net with a lookbehind:

[regex]::Matches($Haystack, "(?i)(?:(?<![a-z])net\s*|\.net\s*|regressed-in-)(\d+)")

Confirmed this still keeps net10, .NET 10, regressed-in-10-preview7, and net 11 correct while rejecting subnet 8 / Ethernet 8. Worth a substring regression test alongside it.

Flagged by: 3/3 reviewers

In candidate (pre-cut) mode the single most important PR in the cycle is the
open "Candidate" PR that promotes a specific main commit as the SR cut point —
the SR branch can't be cut until it merges. It was previously buried as a lone
WATCH row in the bottom ship-checks table plus a sparse low section.

Surface it directly under the Blocking summary as a "🚩 Candidate PR — SRx cut
point (10.0.x0)" section with live status, PR age, mergeability, review state,
and a staleness callout (>=14 days) tied to the ship window and the SR version
base derived from the prior SR branch (e.g. prior sr8 -> SR9 / 10.0.90).

- New Get-CandidatePrResolution runs the gh query + maintainer spoof-gate once
  per report; both the WATCH ship-check and the section consume the shared result
  ($data['candidatePr']), avoiding duplicate network round-trips and twin drift.
- Verdict semantics unchanged: an open/missing candidate PR stays WATCH (normal
  cycle hygiene), never BLOCKED.
- candidatePr is JSON-serialized but excluded from the semantic hash, so the
  daily-ticking age text does not churn the tracker.
- The noisy open-main-PR dump is now suppressed in candidate mode (kept for
  live-SR mode); the section links only the gated candidate(s) plus a pointer to
  the full PR list.
- Migrated renderer tests to the hoisted section (marker-forgery defense
  re-pointed at the candidate table cell) and added Get-CandidatePrResolution
  unit tests (mode transitions, version-base derivation, spoofer vs unverifiable).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@PureWeen PureWeen changed the title Fix cross-major preview-number leak in release-readiness relevance filter Release-readiness: cross-major preview leak fix, milestone blocker, E2E refresh, and hoisted candidate-PR section Jul 5, 2026

@kubaflo kubaflo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note

🔍 AI-generated review — independent 3-model panel (Claude Opus 4.8 · GPT-5.5 · Gemini 3.1 Pro) then cross-pollinated, run on @kubaflo's behalf. A human maintainer makes the final call.

✅ R3 — UNANIMOUS LGTM (high) — safe, additive, no regression

Reviewed the one new commit 5126c9d5 ("Hoist SR candidate-PR into a prominent readiness section") on top of the already-approved R2 base. All three models independently reached LGTM/high; Opus and Gemini both ran the code (StrictMode / Pester) rather than only reading it.

What it does

Extracts Get-CandidatePrResolution so the gh pr list + maintainer author-association spoof-gate runs once, shared by both the existing WATCH ship-check (Get-CandidatePrChecks, now -Resolution-parameterized, recomputing on demand for direct callers) and a new prominent "🚩 Candidate PR" section hoisted under the Blocking summary. The old buried candidate list is now emitted only in live-SR mode (candidate mode targets main, which would dump 100+ PRs).

Cross-pollinated confirmations

  • Spoof-gate preserved byte-identical\bcandidate\b word-boundary match, OWNER/MEMBER/COLLABORATOR gate, and the fail-closed unverifiable ≠ spoofer split (both excluded) all intact. No path admits a non-maintainer Candidate-titled PR into candidates. ✅
  • Shared resolution — proven single gh query (no double-call in Invoke-Main); WATCH ship-check statuses unchanged for no-candidate / spoofer-only / query-failed / resolved. ✅
  • New section — injection-safe (Format-MarkdownTableCell defangs forged <!-- …human-notes --> markers), optional-field guards, deterministic age via ConvertTo-Utc, staleness (≥14d) math correct. ✅
  • versionBaseMajor.Minor.(targetSr*10): SR9→10.0.90, and scales correctly to SR10→10.0.100; graceful fallback for oddly-shaped branches. ✅
  • Tests — new Get-CandidatePrResolution unit tests cover accept / spoofer / unverifiable / query-failed / skip / empty / version-derivation + section render. Validate (PR) Pester gate green (2m10s). ✅

CI

maui-pr skipping (by-design .github/** path-exclusion); Validate (PR) pass. Functional gate is green.

💡 Non-blocking (optional hardening)

Opus noted the # … (all fields optional) comment is only truly honored for isDraft (guarded via PSObject.Properties); the sibling reads ($cp.mergeable, createdAt, updatedAt, reviewDecision) use direct access that would throw under StrictMode if the field were ever absent. It never fires in production because the enriched gh pr list --json projection always supplies those keys — so it's not a regression, just cheap future-proofing if the projection ever changes.

Verdict: LGTM (high, unanimous). ● Approving.

@PureWeen
PureWeen merged commit 7f086db into main Jul 5, 2026
20 of 21 checks passed
@PureWeen
PureWeen deleted the pureween/preview7-cross-major-p0-leak branch July 5, 2026 20:19
@github-actions github-actions Bot added this to the .NET 10 SR9 milestone Jul 5, 2026
PureWeen pushed a commit that referenced this pull request Jul 6, 2026
Resolves the release-readiness E2E conflict in Test-ReleaseReadiness.ps1.
Only one file conflicted; all scripts auto-merged cleanly.

Resolution: took this PR's 2-tracker net10 model (SR8 shipped+refresh-until-closed,
SR9 candidate) — ground-truth-verified by running the merged detection engine, which
retains #36111's shipped-SR-tracker feature. Preserved #36172's drift-proof derived
highestShippedTag assertions (Get-ExpectedHighestShippedTag) and dropped main's
now-incompatible shipped-exclusion invariant (SR8 is a shipped tracker that legitimately
carries tag 10.0.80). Full suite green: 809 passed / 0 failed.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@github-actions github-actions Bot locked and limited conversation to collaborators Aug 5, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-infrastructure CI, Maestro / Coherency, upstream dependencies/versions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants