Release readiness skill — SR + Preview (supersedes #35754) - #35807
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 35807Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 35807" |
Skill Validation Results
✅ Skill Validation Results —
|
|
/review -b feature/enhanced-reviewer |
MauiBot
left a comment
There was a problem hiding this comment.
Expert Review — 8 findings
See inline comments for details.
| $evidence += "Backport PR #$($mergedBackport.number) in SR (active)" | ||
| } | ||
| } else { | ||
| $verdict = 'in-sr-active' |
There was a problem hiding this comment.
[major] Logic and Correctness — False in-sr-active when GitHub API reports MERGED but the backport PR is absent from the git-derived sourcePrSet. The git log is the ground truth here; the GitHub state can lag a -NoFetch run or (rarely) reflect a merge to a different branch. Returning in-sr-active with the evidence string "SR contents may need refresh" tells the agent "fix is shipping" when the evidence is actually contradictory.
Suggested fix: downgrade to needs-human-review with confidence: 'low' for this branch:
} else {
$verdict = 'needs-human-review'
$evidence += "Backport PR #$($mergedBackport.number) marked merged by GitHub but NOT found in git-derived sourcePrSet — re-run without -NoFetch to confirm"
}Contrast with line 581 (same block, different branch) which requires sourcePrSet membership before emitting in-sr-active.
| | CI verdict | Meaning | | ||
| |------------|---------| | ||
| | `green` | Latest build on SR HEAD succeeded across all pipelines | | ||
| | `red-known-flakes` | Failures match historical fixture-flake patterns (Appium `OneTimeSetUp`, etc.) | |
There was a problem hiding this comment.
[moderate] Logic and Correctness — CI verdict table documents red-known-flakes and red-new-failures (lines 99–100) but the script (Get-ReleaseReadiness.ps1 lines 350–352) never emits those values — it only ever emits red-needs-review. Agent consumers that key off red-known-flakes in the JSON will never match.
Either:
- Remove
red-known-flakes/red-new-failuresfrom this table and document onlyred-needs-review, OR - Add the split logic to
Get-CIStatusso the script actually emits distinct verdicts.
The script comment on line 352 (# downstream agent classifies known-flakes vs new) suggests option 1 is the intended design, in which case the two rows in this table should be collapsed to red-needs-review.
| | `merged-non-main-only` | Fix merged but only to `inflight/current` (or similar), not `main` | | ||
| | `open-on-main` | Fix PR is OPEN against main, not yet merged | | ||
| | `no-fix-yet` | No fix PR cross-referenced from the regression issue | | ||
| | `out-of-scope` | Issue lacks any of the `-RegressionLabels` (used for sanity checks, not normally surfaced) | |
There was a problem hiding this comment.
[moderate] Logic and Correctness — out-of-scope verdict is documented here and again in the E2E test assertion (Test-ReleaseReadiness.ps1 line 139: #35326 → out-of-scope) but Classify-RegressionCandidate never produces it. Issues that lack the relevant regressed-in-* label are simply never added to allIssues (they are filtered out by gh issue list --label), so the function is never called for them. The E2E test assertion on line 139 saying "#35326 → out-of-scope" will therefore never be validated by the test harness; the issue would just not appear in the results at all.
Options: (a) remove out-of-scope from the verdict table and update the test comment, or (b) add an explicit out-of-scope classification path (e.g. for issues that were pulled in via wildcard and don't match the label filter).
|
|
||
| $url = "https://dev.azure.com/$org/$project/_apis/build/builds?definitions=$defId&branchName=$branchSpec&`$top=5&api-version=7.1" | ||
| try { | ||
| $resp = curl -s -L --max-time 30 "$url" 2>$null |
There was a problem hiding this comment.
[moderate] Build & MSBuild / Cross-Platform — curl is invoked as a bare command on line 305. In PowerShell 7+, the curl → Invoke-WebRequest alias was removed, so this falls through to the system curl binary. On Windows runners (AzDO windows-latest or windows-2022) curl is in System32 since Windows 10 1803, but the AzDO Agent image may not guarantee it. More importantly, the AzDO public pipeline queryable here (dnceng-public) has open endpoints, so Invoke-RestMethod would be idiomatic, pipe-friendly, and not require parsing a JSON string out of raw text:
$obj = Invoke-RestMethod -Uri $url -TimeoutSec 30 -ErrorAction StopThis also lets the catch block work with the native error record instead of checking $resp for null.
| # Is SR HEAD an ancestor of (or equal to) the build's source SHA? | ||
| $check = Invoke-Git "merge-base --is-ancestor $($Ctx.srHeadSha) $sourceSha" | ||
| $isAtOrAhead = ($LASTEXITCODE -eq 0) | ||
| if (-not $isAtOrAhead -and $sourceSha -eq $Ctx.srHeadSha) { $isAtOrAhead = $true } |
There was a problem hiding this comment.
[minor] Logic and Correctness — Redundant equality fallback. git merge-base --is-ancestor X X exits 0 (every commit is an ancestor of itself), so the extra guard if (-not $isAtOrAhead -and $sourceSha -eq $Ctx.srHeadSha) is dead code. It can mislead a reader into thinking the equal case is NOT handled by --is-ancestor. Safe to remove; if kept, add a comment explaining why it is needed.
|
|
||
| @{ | ||
| classification = $best.verdict | ||
| confidence = 'high' |
There was a problem hiding this comment.
[minor] Logic and Correctness — confidence is hardcoded to 'high' for all non-empty verdicts, including needs-human-review. Emitting {classification: 'needs-human-review', confidence: 'high'} is self-contradictory and may mislead the downstream agent into treating the result as strongly evidenced. At minimum, needs-human-review verdicts (and the merged-non-main-only / merged-on-main-no-backport verdicts which depend on the ancestry check but have no backport) should carry confidence: 'medium'.
| - **Wraps**: `maestro-cli` skill (from `dotnet-dnceng@dotnet-arcade-skills` plugin) and maestro MCP tools | ||
| - **Note**: Provides MAUI-specific guardrails on top of core Maestro/darc operations — channel naming, safety deny-list, input validation, and prompt injection defense | ||
|
|
||
| 12. **release-readiness** (`.github/skills/release-readiness/SKILL.md`) |
There was a problem hiding this comment.
[minor] Complexity Reduction — Duplicate list number. This PR inserts 12. **release-readiness** at line 347 but the pre-existing 12. **try-fix** ("Internal Skills") remains at line 356, creating two 12. entries in the Reusable Skills list. The new release-readiness entry should be numbered 13. (or the entire internal-skills sub-list renumbered). This also causes the inline copilot system-prompt rendering (used for routing) to have an ambiguous duplicate key.
| pwsh .github/skills/release-readiness/scripts/Get-ReleaseReadiness.ps1 \ | ||
| -SrBranch <branch> \ | ||
| -RegressionLabels <labels> \ | ||
| -OutputDir /tmp/<branch-slug>-readiness |
There was a problem hiding this comment.
[minor] Build & MSBuild — /tmp/<branch-slug>-readiness is a hard-coded Unix path. On Windows CI agents pwsh maps /tmp to C:\Users\...\AppData\Local\Temp only when using certain builds; other environments will fail. Prefer $env:TEMP or PowerShell's cross-platform [System.IO.Path]::GetTempPath() in documentation examples:
-OutputDir "$(pwsh -c '[System.IO.Path]::GetTempPath()')/<branch-slug>-readiness"(Same concern applies to the SKILL.md Quick Start examples.)
|
/review -b feature/regression-check |
Bundle prerequisites for a daily GitHub Action that maintains release- readiness tracker issues (one per in-flight SR). All work lands in this PR (#35807) per the user's request — the workflow YAML itself is deferred to a follow-up. What's added: 1. Shared versioning module (.github/scripts/shared/MauiReleaseVersioning.psm1): - Extracts 7 helpers from Fix-MilestoneDrift.ps1 (ConvertTo-Milestone, Get-VersionFromGitRef, Get-CurrentMajorVersion, etc.) so detection and milestone-drift scripts share one source of truth. - Net diff: Fix-MilestoneDrift.ps1 shrinks 162 lines; all 91 existing Pester tests still green. 2. Tracker detection script (Find-ReleaseReadinessTrackers.ps1): - Two-lane algorithm: Lane 1 walks real release/N.0.Mxx-srK branches where PatchVersion > HighestShippedPatch; Lane 2 emits a candidate tracker for the next SR cut from main when activity warrants. - Strict branch regex rejects sr-backup, sr-test, sr-next, case mismatches (sr10-test, SR8, etc.); stable-tag regex rejects prerelease tags so 'highest shipped' is correct. - Fail-closed: any git error => non-zero exit, no JSON written. - End-to-end dogfood detects exactly SR7+SR8+SR9 on net10. 3. Get-ReleaseReadiness.ps1 enhancements: - False-positive guard #1 (Test-PrIsToolingOnly): skip candidate fix PRs whose entire change set is .github/, docs/, eng/scripts/, etc. Stops self-referencing agent/skill PRs from being mistaken for real fixes (the #35771 case). - False-positive guard #2 (DUPLICATE detection): issues closed with stateReason=DUPLICATE get classification 'closed-as-duplicate' and skip the expensive PR walk. Bucketed under Tier 3 (informational) in the report (the #35610 case). - Deterministic 🔴/🟡/🟢 verdict (Get-OverallVerdict) with explicit tier table (Get-VerdictTier). Candidate mode downgrades CI noise to advisory; OPEN no-fix-yet blocks but CLOSED does not. - Issue-postable markdown: semantic content (excludes fetchedAt) for idempotent posts a workflow can preserve manual release-captain notes * 60KB body cap with UTF-8-safe truncation - Linkified SHAs and PR/issue numbers via -RepoUrl. - Tier 1/2/3 tables replace the flat tier list in the report body. - Test-mode env var (GET_RELEASE_READINESS_TEST_MODE) lets the test harness dot-source the script without running Invoke-Main. 4. Tests (Test-ReleaseReadiness.ps1): 26 -> 163 assertions - Strict SR branch regex acceptance + 12 rejection cases - Stable-tag regex acceptance + 6 rejection cases (prerelease tags) - Regression-label inference for major=10/11 sr=1..10 - E2E detection against live repo (SR7+SR8+SR9 trackers) - Detection fail-closed on non-git paths - Test-PrIsToolingOnly edge cases (null files, src/.../docs/ vs top-level docs/, eng/cake/ vs eng/scripts/) - Get-VerdictTier table for all 10 classifications + unknown - Get-OverallVerdict: 🟢/🟡/🔴 transitions, candidate-mode CI advisory, OPEN vs CLOSED no-fix-yet blocking - Linkification helpers (full/short SHA, null PR, empty RepoUrl) - Semantic-hash properties: 64-char hex, invariant to fetchedAt, changes on srHeadSha/classification/sourcePrs/verdict, order-independent across sourcePrs reorderings, deterministic - Format-MarkdownReport asserts markers, verdict line, tier headers, linkification, human-notes section, body cap - Idempotency: re-run with same data -> identical hash Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
/review rerun |
|
/review -b feature/enhanced-reviewer -p android |
MauiBot
left a comment
There was a problem hiding this comment.
Expert Review — 7 findings
See inline comments for details.
| $evidence = @() | ||
|
|
||
| # In-SR (with revert check) | ||
| if ($sourcePrSet.ContainsKey($pr.number)) { |
There was a problem hiding this comment.
[major] Release readiness — A reverted backport can still classify the source PR as in-sr-active. Get-CommitsForRevSpec adds both the backport PR and the original source PR to sourcePrs, but revert detection records the reverted backport PR number; this branch checks $sourcePrSet.ContainsKey($pr.number) first and only looks for that same source PR in $revertedPrSet. Scenario: source PR #35001 is backported as #36000 and #36000 is later reverted in the SR; #35001 remains in sourcePrSet and is reported active instead of reverted/needs review.
| if (-not $line) { continue } | ||
| if ($line -match '^[0-9a-f]{40}\s+refs/heads/net(\d+)\.0$') { | ||
| $candidate = [int]$Matches[1] | ||
| if ($candidate -ge $mainMajor) { |
There was a problem hiding this comment.
[major] Release readiness — -AllActiveMajors drops still-active servicing majors after main advances. Once origin/main moves from .NET 10 to .NET 11, the candidate -ge $mainMajor filter excludes net10.0, so daily tracker discovery stops producing .NET 10 SR trackers even though release/10.0.*-srN branches may still be active.
| } | ||
|
|
||
| if ($patch -eq 0) { return ".NET $major.0 GA" } | ||
| if ($patch -lt 10) { return ".NET $major.0 SR1" } |
There was a problem hiding this comment.
[moderate] Build/MSBuild — Early patch tags produce .NET <major>.0 SR1, but Get-MilestoneSortKey only parses .NET <major> SRn. As a result Compare-MauiMilestone (ConvertTo-Milestone 10.0.5) .NET 10 SR2 returns $null, so the earlier-milestone preservation guard in Fix-MilestoneDrift.ps1 can be bypassed for SR1 patch tags. Normalize the producer output or accept the .0 SR form in the sort key parser, and add the regression test from the try-fix.
| param($Ctx) | ||
|
|
||
| # Parse SR version from branch name: release/10.0.1xx-sr7 -> 10.0 | ||
| $branchMatch = [regex]::Match($Ctx.srBranch, '^release/(\d+)\.(\d+)\.\d+xx-sr(\d+)$') |
There was a problem hiding this comment.
[moderate] Release readiness — Candidate-mode label inference always fails here. Resolve-Context sets ctx.srBranch to the main branch in -Candidate mode and stores the real prior SR in ctx.priorSrBranch, but this parser only accepts release/...-srN; the workflow falls back to -InferRegressionLabels when tracker labels are empty, so candidate SR tracker runs skip the regressions phase instead of inferring labels from the prior SR.
| $body = if ($pr.body) { [string]$pr.body } else { '' } | ||
| $title = if ($pr.title) { [string]$pr.title } else { '' } | ||
| $combined = "$title`n$body" | ||
| if ($combined -notmatch "(?i)(?:fix(?:es|ed)?|close[sd]?|resolve[sd]?)\s+(?:[a-z0-9_\-./]+)?#$IssueNumber\b") { |
There was a problem hiding this comment.
[moderate] Build/MSBuild — This validation regex no longer matches the full GitHub issue URL form that Get-LinkedIssues accepts (Fixes https://github.com/dotnet/maui/issues/N). A PR that fixed an issue using the URL form can be found by the initial linked-issue logic but later fail Test-MilestoneValidForIssue, allowing a valid earlier milestone to be overwritten. Reuse the same URL-aware fixing-reference pattern here.
| # and verify the dropdown contains an entry matching this preview. | ||
| try { | ||
| $expectedVersion = "$majorVersion.0.0-preview.$previewNumber" | ||
| $templateBranch = if ($mainBranch) { $mainBranch } else { 'main' } |
There was a problem hiding this comment.
[moderate] Release readiness — The comment says issue templates are global and should be read from main, but this chooses net<major>.0. GitHub issue creation uses the default branch template, so preview readiness can report READY/BLOCKED based on a version dropdown that users never see. Read bug-report.yml from main (or the repo default branch) for this check.
|
/review -b feature/enhanced-reviewer -p android |
The 'Recent CI Failure Scanner signals' table was buried at the bottom
of the report between Known Build Errors and Maintainer next actions.
For branches with active scanner signals (e.g. Preview6 today with
14 net11.0 issues) those failures should be visible immediately after
the blocking + cleanup summaries — not after several PR tables.
Move the section to render right after '🧹 Cleanup follow-ups' so the
'what's currently broken in CI on this branch' signal is one of the
first things a release captain sees.
Also fix a latent strict-mode bug in Get-ReleaseRelevantIssuesByLabel
(Preview): 'return @($null)' is unwrapped to $null on function return,
so '$kbeIssues.Count' threw under Set-StrictMode when the label list
was empty. The pre-existing 'gh issue list --label "Known Build
Error"' returned [] this run (16 KBE issues from earlier are now
closed), which triggered the path. Fix with the leading-comma idiom
(',@($deduped)') to preserve array type even when empty.
All 488 tests pass.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The Candidate PR (e.g. 'June 8th, Candidate') is the mechanism that
promotes a specific main commit as the basis for cutting the next SR.
Without its merge, the SR cycle can't start. Previously it was buried
in an informational '## Candidate PR for next SR cut' section near the
bottom of the report — easy to miss.
Surface it as a real WATCH ship-readiness check so the release captain
sees it in the readiness checks table at the top of the report. Two
states:
- Open candidate PR found → WATCH, with PR link + reminder that this
PR must merge before the SR cut
- No candidate PR on main → WATCH (informational; normal early in
cycle, release captain opens one when ready)
Never BLOCKED: a missing candidate PR is normal early in the cycle.
In-flight mode (SR already cut) skips the check entirely.
The check label includes the next SR number derived from the prior
SR branch — e.g. for release/10.0.1xx-sr8 in candidate mode the
check renders as 'Candidate PR for next SR cut (SR9)' so it's clear
which cut the PR is preparing.
Tests:
- 5 unit tests for nextSr regex (single + two-digit, non-SR branches)
- 493/493 tests pass
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
PureWeen
left a comment
There was a problem hiding this comment.
Adversarial Multi-Model Review (3 reviewers, post-5ab551842a)
Methodology: 3 independent reviewers with adversarial consensus. Findings below were verified against source after initial flagging. Prior MauiBot review threads remain valid; this pass focuses on new findings in commits landed after the most recent MauiBot review (2026-06-13).
Summary of new findings
| # | Severity | Area | Finding |
|---|---|---|---|
| 1 | ❌ | Logic | WATCH ship-checks never escalate the headline verdict, but the wiring comment says they do — and the Preview script does the opposite |
| 2 | Error Handling | gh API failures (rate limit / outage / auth) silently become 🟢 READY for ci-scan + KBE |
|
| 3 | Error Handling | gh pr list failure in Get-CandidatePrChecks is reported as "no Candidate PR found" |
|
| 4 | Security | Any open PR on main with the word "Candidate" in its title spoofs the SR cut PR check |
|
| 5 | Logic | In candidate mode, the cycle-after-next milestone (e.g. SR10 while surveying SR9-candidate) triggers BLOCKED → 🔴 Not Ready |
|
| 6 | Logic | Internal dotnet-maui AzDO pipeline (dnceng/internal) is queried with no auth header → always 401 → silent fallback to unknown |
|
| 7 | Testing | Newest helpers are NOT exercised — tests mirror-implement the regex/logic locally, so a bug in the real code leaves 493 tests green |
Suggestions (lower severity)
- 💡
Get-ReleaseReadiness.ps1:480—Get-MainBumpDateForCycleuses[DateTime]::Parse($parts[0])withoutInvariantCulture. Inconsistent with the file's ownConvertTo-Utchelper (which exists specifically to avoid this). Low risk for ISO-8601, but feeds ship-date math. (Reviewer 1) - 💡
release-readiness.yml:297—gh api ... --jq ".[] | select(.title == \"$MILESTONE_NAME\") | .number"interpolates the milestone name into the jq filter. Today's milestone names (.NET 10 SR8) are quote-free so it's safe in practice, but the pattern is fragile. Consider piping throughjq --arg m "$MILESTONE_NAME" '... select(.title == $m)'. (Reviewer 3) - 💡
Find-ReleaseReadinessTrackers.ps1:446+release-readiness.yml:170,369—New-RegressionLabelListunconditionally returns ≥1 label, so the workflow'selse REG_LABEL_ARG=(-InferRegressionLabels)branch is unreachable dead code. If the team ever adopts non-canonical patch labels (e.g.regressed-in-10.0.74), inference will silently never run. (Reviewer 3) - 💡
Get-ReleaseReadiness.ps1:1651—gh pr view --json filesdoes not paginate; if a PR ever has >100 changed files,Test-PrIsToolingOnlysees only the first 100 and may misclassify a real product fix as tooling-only. Low practical impact (PRs >100 files are rare). Failing safe would be: if returned count == 100, return$false. (Reviewer 3)
User-suggested follow-up (not a defect)
The **Branch**: <name> body-marker parsing in Get-CiScanIssueBranch + Get-CiScanIssuesForSr is functionally correct but architecturally redundant — the label name itself already encodes the branch (ci-scan = main, ci-scan-net11 = net11.0, ci-scan-net12 = net12.0). A simpler design: branch → expected label, query that one label, skip body parsing. Leaving as-is also works.
Where this PR is solid (verified)
- Workflow permission surface — top-level
contents: read, PR validate job is read-only, usespull_request(notpull_request_target) so fork-modified scripts run with read-only token and no secrets. Invoke-Ghcorrectly checks$LASTEXITCODEand returns$nullon failure (the false-green issues above are at the call sites, not inInvoke-Ghitself).- Strict-mode discipline is strong: existence-guarded
PSObject.Properties['x'],,@()unary-comma returns to defeat array unwrap,[int]normalization at extraction. Resolve-Contextcorrectly swapssrBranchandpriorSrBranchsemantics between candidate and in-flight modes — the newGet-CandidatePrChecksand milestone hygiene code use the right field.Test-PrIsToolingOnlyfalse-positive guard against agent/skill PRs that mention regression issue numbers in their body for documentation purposes.
|
|
||
| # CI scanner + KBE issue signals — merged into shipChecks so they appear in the | ||
| # ship-readiness table AND can escalate the verdict (fresh ci-scan → WATCH; never | ||
| # BLOCKED automatically because the scanner can be noisy). |
There was a problem hiding this comment.
❌ Logic — The comment here promises that ci-scan signals "can escalate the verdict (fresh ci-scan → WATCH)", but Get-OverallVerdict at line 2516 only escalates the headline 🔴/🟡/🟢 when Status -eq 'BLOCKED'. WATCH ship-checks never move the verdict. Every signal these recent commits added is WATCH at most: Get-CiSignalChecks ci-scan (2342/2348), KBE (2367), Get-CandidatePrChecks (1148/1160), and the Maestro "no BAR build for HEAD" check.
Cross-script inconsistency: Get-PreviewReadiness.ps1's $StatusRank ranks WATCH = 1 above READY = 0 and Get-OverallStatus is worst-wins — so an identical signal degrades the Preview headline but stays 🟢 Ready on an SR.
Concrete trigger: In candidate mode the ci-scan scanner does match (ctx.srBranch = main / net11.0, which is what ci-status-net11 tags). A ci-scan-net11 issue filed 3 hours before the nightly run emits a WATCH check ("filed in the last 24h… Likely affects this SR"). The author's wiring comment says this escalates to WATCH; Get-OverallVerdict leaves it 🟢. Release captain reading only the headline ships without seeing the fresh CI regression the feature was built to surface.
Fix options: (a) make Get-OverallVerdict treat WATCH ship-checks as Tier-2 (🟡), matching Preview's worst-wins model and the comment; or (b) if WATCH-is-non-escalating is the deliberate design, fix the comment (drop "can escalate the verdict") and reconcile with the Preview script so the same signal yields the same operator-facing verdict in both.
Flagged by: 1/3 reviewers, verified against source.
| #> | ||
| $raw = Invoke-Gh @('label', 'list', '--repo', $script:Repo, | ||
| '--search', 'ci-scan', '--limit', '50', '--json', 'name') | ||
| if (-not $raw) { return @() } |
There was a problem hiding this comment.
Get-CiScanLabels (and the parallel Get-ReleaseRelevantIssuesByLabel at 2198) return @() on gh failure with no out-of-band signal. Then Get-CiSignalChecks at lines 2353-2363 / 2372-2378 reports 🟢 READY: "No open ci-scan issues — scanner has not flagged recurring CI failures." / "No open Known Build Error issues found."
Concrete trigger: GitHub API rate limit / brief outage / token scope issue → gh label list exits non-zero → Invoke-Gh warns to stderr and returns $null → the helper returns @() → the report says 🟢 READY for both ci-scan and KBE. A release captain ships without seeing active blockers because the survey couldn't reach GitHub.
Fix: distinguish API failure from genuinely-empty results. Either return a structured @{Success=$true; Data=@()} envelope (like Invoke-DarcJson already does in this file) or emit an UNKNOWN New-ReadinessCheck with the retry command in NextAction so the captain sees "⚪ Could not query ci-scan labels" instead of false-green.
Flagged by: 1/3 reviewers, verified against source (see Invoke-Gh at line 190 and call sites 2235, 2198, 2353-2378).
| # Scan open PRs targeting main (the Candidate PR is opened on main, not | ||
| # on the SR branch, since the SR branch may not exist yet in candidate | ||
| # mode). Cheap: one gh call returning up to 100 open PRs on main. | ||
| $raw = Invoke-Gh @('pr', 'list', '--repo', $Ctx.repo, '--state', 'open', |
There was a problem hiding this comment.
gh pr list fails (rate limit, outage), $mainPrs stays @() and line 1147-1150 emits a WATCH with text "No open Candidate PR found". A release captain reading this in candidate mode could conclude the team simply hasn't opened one yet, when in reality the survey couldn't reach GitHub.
Fix: distinguish $raw -eq $null (gh failure) from $parsed.Count -eq 0 (genuinely empty). On failure, emit a separate UNKNOWN New-ReadinessCheck with NextAction = 'Retry: gh pr list --repo $($Ctx.repo) --base $($Ctx.mainBranch) --search Candidate' so the captain sees the survey was incomplete rather than a misleading WATCH.
Flagged by: 1/3 reviewers, verified against source.
| } | ||
|
|
||
| # Be conservative — require word boundary so "CandidateView" doesn't match. | ||
| $candidates = @($mainPrs | Where-Object { $_.title -match '(?i)\bcandidate\b' }) |
There was a problem hiding this comment.
main whose title contains the word "Candidate". The match is (?i)\bcandidate\b with no author/branch/label gating. Any contributor — including a fork PR author — can open a PR titled e.g. "Candidate refactor of foo" and have it surfaced as the SR cut PR in the next readiness report.
Real-world impact is bounded (WATCH severity only, doesn't escalate verdict, release captain reviews before cutting), but the report links to the spoofed PR by number from the official tracker issue — which can be embarrassing or confusing.
Fix: require at least one trusted signal — author membership in the dotnet/maui team, a specific label (e.g. release-candidate), or a strict title format the team controls (e.g. ^\[Candidate\] rather than \bcandidate\b). Document the chosen convention in the SKILL.md so the release captain knows how to mark a PR as the canonical Candidate.
Flagged by: 1/3 reviewers, verified against source.
| # === Check 2: Next cycle's milestone exists === | ||
| $nextMs = @($allMs | Where-Object { $expectedTitlesNext -contains $_.title }) | ||
| $nextTitle = $expectedTitlesNext[0] | ||
| if ($nextMs.Count -eq 0) { |
There was a problem hiding this comment.
priorSrBranch = release/10.0.1xx-sr8, cycleNum++ yields current = SR9, next = SR10. Check 2 here BLOCKED-escalates if SR10 doesn't exist; BLOCKED ship-checks flow into Get-OverallVerdict Tier 1 → 🔴 Not Ready (line 2516-2519).
Concrete trigger: Early in the SR8→SR9 candidate window, the team has created SR9 milestone but not yet SR10 (the cycle two ahead — normal this early). The candidate tracker reports 🔴 Not Ready purely because a milestone for the SR-after-the-one-being-cut is absent.
The check's own rationale (line 1039: "Once SR9 ships, open issues will have nowhere to roll forward to") describes the in-flight shipping scenario; in candidate mode SR9 hasn't even been cut yet, so requiring SR10 is premature. Check 1 (current SR9 milestone missing → BLOCKED) is reasonable in both modes; only Check 2 is premature in candidate mode.
Fix: in candidate mode, downgrade the missing next-cycle milestone from BLOCKED to CLEANUP (or WATCH) so it surfaces without flipping the headline to Not Ready before the SR is even cut.
Needs human confirmation on MAUI's milestone pre-creation cadence — if SR10 is genuinely expected to exist before SR9 is cut, this is by design.
Flagged by: 1/3 reviewers, verified against source.
|
|
||
| $url = "https://dev.azure.com/$org/$project/_apis/build/builds?definitions=$defId&branchName=$branchSpec&`$top=5&api-version=7.1" | ||
| try { | ||
| $obj = Invoke-RestMethod -Uri $url -TimeoutSec 30 -ErrorAction Stop |
There was a problem hiding this comment.
Get-PipelineLatestBuilds uses Invoke-RestMethod with no -Headers. This works for the public dnceng-public/public pipelines (anonymous read), but $Script:InternalPipelines (line 173) lists dotnet-maui on dnceng/internal which requires AzDO authentication. The call will always 401 → catch block at 1479-1482 returns $null → Get-CIStatus at 1494-1500 emits verdict: 'unknown' with note "Could not query (auth or outage)".
That tips the CI overall into partial-unknown / unknown, which Get-OverallVerdict (line 2503-2506) escalates to Tier 2 (🟡 Conditionally Ready) every single run. So the SR can never report 🟢 Ready while this pipeline is in the list.
The Preview script acknowledges this scenario explicitly (Get-PreviewReadiness.ps1:1107: "Internal dnceng pipeline details are not queried in public workflow mode.") and gates on -IncludeInternal. The SR script does not.
Fix options: (a) skip the internal pipeline when no AzDO token is available (env-var check); (b) plumb a token via $env:SYSTEM_ACCESSTOKEN and conditionally set Authorization = "Bearer $token"; (c) shell out to az pipelines runs list which handles token lifecycle automatically. (a) is the simplest fix consistent with the Preview script's approach.
Flagged by: 1/3 reviewers, verified against source (pipeline at line 173, query at line 1475, escalation at 2503).
| -Actual (Get-CiScanIssueBranch -Issue $issueEmptyBody) | ||
|
|
||
|
|
||
| # ───── Get-CiScanLabels filters repo labels to ^ci-scan(-|$) ───── |
There was a problem hiding this comment.
Invoke-ShipChecksWithMockedVersions at ~1622 calls the real Get-ReleaseShipChecks; Invoke-MaestroChecksWithMocks at ~2034 overrides global:Invoke-DarcJson to call the real Get-MaestroOperationalChecks). Yet the helpers introduced by the newest commits are re-implemented inline in the test rather than invoked:
- ci-scan label regex: test defines a local
$labelRegex = '^ci-scan(-|$)'here and asserts against the string — it never calls the productionGet-CiScanLabels. - next-SR label: test defines a local
Get-NextSrLabelre-implementing thesr(\d+)$regex (~line 1939) — it never calls the productionGet-CandidatePrChecks. Get-CiScanIssuesForSr,Get-CiSignalChecks,Get-MainBumpDateForCycle(SR) andGet-CiScanIssues(Preview): zero test references.
A regression inside any of them (flipping ^ci-scan(-|$) to ^ci-scan- and losing main, or breaking the +/+++ diff filter in git log -S) leaves all 493 tests green because the tests assert against copies of the patterns, not the functions.
Fix: dot-source and invoke the real function with mocked Invoke-Gh / Invoke-Git (the existing 1622/2034 mock pattern is directly reusable), and delete the local $labelRegex / Get-NextSrLabel copies so the regex lives in exactly one place — the production code.
Flagged by: 2/3 reviewers (Reviewer 1 on mirror-impls, Reviewer 3 on coverage gaps for Get-MainBumpDateForCycle/Get-RegressionLabelsAuto/Test-CiScanIsFresh).
Applies fixes from the adversarial review on PR #35807: - Get-OpenIssuesByLabel (SR script) now returns an envelope { QueryFailed; Issues } so callers can distinguish a gh failure from an empty result. Wired through Get-CiSignalChecks so a failed query produces WATCH (was previously READY = false-green). - Get-CandidatePrChecks adds an authorAssociation gate (OWNER/MEMBER/COLLABORATOR only) so a community PR titled 'Candidate ...' can't spoof the candidate-mode signal, and surfaces WATCH on gh failure instead of silent zero-count READY. - Replace Get-CiScanIssueBranch + Get-CiScanLabels (body-marker filter) with deterministic Get-CiScanLabelForBranch mapping in BOTH SR and Preview scripts. Preview branches map to their parent net<N>.0 scanner label, fixing the in-flight Preview blind spot where '$SurveyRef = release/11.0.1xx-preview6' filtered out 100% of 'ci-scan-net11' signals. - Demote 'Milestone for next cycle missing' from BLOCKED to CLEANUP (universal, both modes): missing roll-forward milestone is housekeeping, not a ship blocker. - Escalate WATCH \u2192 Tier 2 (Conditionally Ready) in Get-OverallVerdict so 'soft yellow' signals don't get rolled into a green verdict. - Add -IncludeInternal switch to gate $Script:InternalPipelines so PR/cron runs without the dnceng PAT don't hit 401 on devdiv pipelines. - Replace [DateTime]::Parse($parts[0]).ToUniversalTime() with ConvertTo-Utc -Value $parts[0] in Get-MainBumpDateForCycle to honor the existing invariant-culture parsing path. - Remove dead -InferRegressionLabels else-branches in release-readiness.yml (both per-tracker-report and validate jobs); New-RegressionLabelList unconditionally returns >=1 label so the fallback was unreachable. Tests: - Updated milestone M3 / M12 to assert CLEANUP (per Finding #5). - Replaced Get-CiScanIssueBranch / Get-CiScanLabels regex tests with Get-CiScanLabelForBranch cases (main, net11.0, future net12.0, preview, SR, empty, garbage). - All 421 tests pass with -SkipE2E. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…on, document when-to-use rule
Multi-model architecture review (Opus-high + GPT-5.5 + Gemini-3.1) flagged
three real gaps in the release-readiness-agent:
1. Agent was SR-only while the skill supports SR + Preview \u2014 a user
asking 'how does net11 preview6 look?' would likely fail to route
here, or route here and try to run Get-ReleaseReadiness.ps1 on a
preview branch (which the script rejects).
2. Agent file duplicated ~70% of SKILL.md content (Outputs table,
Anti-Patterns, -Candidate example, 'What This Agent Does/NOT Do'),
creating a drift trap whenever the script's contract changes.
3. No documented rule for when to invoke /release-readiness (the skill)
vs the release-readiness-agent.
Changes:
- Rewrite .github/agents/release-readiness-agent.agent.md:
- Frontmatter description covers both SR and Preview lanes
- Adds 'Why this is an agent (and not just a skill)' section
explaining the three things the agent uniquely provides
(NL routing, WorkIQ/MCP enrichment, persona contract)
- Step 0 routes by branch shape (sr* \u2192 Get-ReleaseReadiness.ps1,
preview* \u2192 Get-PreviewReadiness.ps1)
- Candidate mode documented for BOTH lanes (SR + Preview)
- WorkIQ enrichment marked SR-only (preview has no rejected-backport
tier); UNKNOWN MCP patching applies to both
- 'See SKILL.md' references replace restated content (Outputs table,
parameter contracts, classification taxonomy)
- Common pitfalls trimmed and made lane-aware
- Persona contract (REPORT ONLY) expanded to include preview refs
and netN.0 inflight refs
- Update .github/copilot-instructions.md:
- Agent entry (line 265) now mentions both SR and Preview, lists
both scripts, and explicitly directs scripted/dashboard consumers
to the skill instead of the agent
- Skill entry (line 347) now lists all three scripts and adds an
explicit 'use this skill directly when ... use the agent when ...'
note
- Delegation examples (line 371) add a Preview trigger and a
raw-JSON example to clarify the rule
Tests:
- No script changes; all 421 Pester unit tests still pass.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…dversarial review Applies the high-confidence findings from a 3-model adversarial review of the release-readiness skill (consensus markers noted per fix): - Revert false-green (highest severity): GitHub's revert subject `Revert "Title (#1234)" (#5678)` carries the reverted PR (1234) inside the quoted title and the revert's OWN PR (5678) as the trailing suffix. The old greedy `Revert.*\(#(\d+)\)` captured 5678, which made $revertsPr truthy and skipped the authoritative reverted-commit SHA lookup — so a reverted regression fix was classified `in-sr-active` (🟢 ready to ship) instead of `in-sr-reverted` (🔴). Extracted a Get-RevertedPrFromSubject helper (explicit "Revert PR #N" → quoted-inner (#N), never the trailing revert PR) and made the reverted-commit SHA subject the authoritative override. Added 5 parser tests including the exact false-green scenario. - Workflow data loss (3/3): the tracker refresh ran `gh issue edit --body-file`, wiping the human-editable "Release Captain Notes" block both engines promise to preserve. The refresh now splices the existing begin/end notes block from the live issue into the new body before editing (works for SR and preview). - Idempotency (2/3): the embedded `release-readiness-hash` marker was never read, so every scheduled run re-edited the issue. The refresh now compares the live vs fresh hash and skips the edit when semantic content is unchanged (preview has no hash → always edits). - PR concurrency (3/3): the concurrency group keyed only on event_name + branch, so all pull_request runs shared one group with cancel-in-progress and cancelled each other. Added github.event.pull_request.number to the group. - Milestone failure masking (verified): Get-AllMilestones returned Success=$true with empty Data when `gh` failed (Invoke-Gh returns $null on non-zero exit), masking an API failure as "zero milestones". Now returns Success=$false so the caller degrades to UNKNOWN (a successful query always returns at least `[]`). - Docs (2/3): SKILL.md listed bug-template / next-cycle-milestone / stale-milestone checks as BLOCKED, but the script emits CLEANUP (post-release housekeeping); added CLEANUP to the status legend and corrected the idempotency description. Discarded single-reviewer findings: Invoke-Gh temp-file handling (verified clean), revert-of-revert topological ordering (rare; false-RED is the safe direction), and UNKNOWN-should-cap-verdict (by-design; the agent patches UNKNOWN rows via MCP). All 495 tests pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…t regex
Second multi-model adversarial review (Reviewer 1/2/3) of the round-1 changes
surfaced one proven critical bug plus several data-loss/robustness gaps, all in
code introduced by round 1. Fixes (consensus + empirical verification noted):
- Get-ReportSemanticHash: use [ordered]@{} instead of [hashtable]@{}. A plain
hashtable enumerates keys via per-process String.GetHashCode(), which .NET Core
randomizes per process — so ConvertTo-Json emitted keys in a different order
every run, producing a DIFFERENT hash for identical content. The workflow's
idempotent no-op compares a hash written by an earlier process against one
computed now, so it NEVER matched and the tracker was edited on every run.
Empirically confirmed: 4 separate processes produced 4 different hashes with
@{} and 4 identical hashes with [ordered]@{}. (Reviewer 3; verified.)
- release-readiness.yml refresh step: a transient `gh issue view` failure left an
empty CUR_BODY_FILE, which skipped the notes splice AND zeroed OLD_HASH, falling
through to `gh issue edit` and overwriting the human-authored Release Captain
Notes with the placeholder. Now the fetch exit status is captured and the whole
refresh is skipped on failure (a missing refresh self-heals; lost notes do not).
(Reviewers 1/2/3 — unanimous.)
- release-readiness.yml notes merge: require a complete, single begin+end marker
pair in the live body before splicing (an unterminated/duplicated block captured
the entire stale report to EOF and re-injected it, growing the body every run),
and match markers as anchored full lines so a note that merely mentions the
marker text can no longer truncate capture. (Reviewers 1/2.)
- Get-RevertedPrFromSubject: harden the quoted-title regex to
(?i)Revert\s+".*\(#(\d+)\)" — case-insensitive (hand-typed lowercase reverts)
and tolerant of internal quotes in the original title (the old [^"]* halted at
the first inner quote and returned null, dropping the reverted PR). (Reviewers
2/3.)
- Docs: scope the idempotent-hash claim in SKILL.md to SR trackers (preview
trackers carry no hash marker and refresh every run); correct methodology.md to
describe the quoted-title revert extraction instead of the removed greedy suffix
heuristic. (Reviewers 1/2/3.)
Tests: +4 (revert internal-quote + lowercase cases; cross-process hash-stability
guard that computes the hash in two fresh processes and asserts equality). Full
suite: 499 pass / 0 fail.
Discarded after dispute: concurrency event_name (Reviewer 1 ruled gh issue edit
atomic + distinct dispatch/schedule groups), hash coarseness (coarseness is by
design), and a narrow unquoted-hand-crafted-revert gap (not a regression vs the
prior regex; covered by the SHA override on the common path).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Round-2 anchored the awk marker patterns to full lines but left the
precondition grep guard using substring matching, desyncing the two and
opening two silent Release Captain Notes data-loss paths flagged by a
3/3 multi-model adversarial review (Opus 4.8, GPT-5.5, Gemini 3.1-pro,
reproduced across BSD awk / gawk / mawk 1.3.4):
* A note that merely MENTIONS the end token made the unanchored
`grep -c` return 2, failing the `-eq 1` guard, skipping the splice,
and letting the next content-changing `gh issue edit` overwrite the
notes.
* Text on a marker line passed the substring guard but the anchored
awk matched nothing, splicing in an EMPTY block and wiping the notes.
Fix:
* Anchor all guard greps to the SAME full-line markers the awk uses
(`grep -cE '^[[:space:]]*<!-- ... -->[[:space:]]*$'`), so guard and
extractor agree on what a valid block is. Resolves the mention case.
* Add a malformed-marker safety net consistent with the existing
fetch-failure guard: when the live body carries notes markers that
don't resolve to a single clean begin+end pair (corrupted/duplicated/
text-on-marker), skip the edit entirely rather than overwrite — a
stale refresh self-heals next run; destroyed captain notes do not.
Verified: 6 vectors (normal, token-mention, text-on-marker, duplicated
begin, no markers, missing-end) x 3 awk implementations + CRLF bodies;
bash -n OK; YAML parses; 499/499 PowerShell tests pass.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Round-4 adversarial review (Opus-4.8, GPT-5.5, Gemini-3.1) surfaced three distinct notes-loss / refresh-correctness paths in the release-readiness tracker machinery. All three are empirically reproduced and fixed: 1. Truncation drops notes markers (SR-only, ❌ data loss). The SR engine's body-size cap did a blind byte-prefix cut, but the human-notes block sits mid-body below potentially unbounded sections. On a busy SR the fresh body could exceed the cap before the notes block, dropping the begin/end markers; the daily refresh would then overwrite a live issue (with real notes) using a markerless body. Fix: Format-MarkdownReport now builds the notes block as a reusable string, strips it before truncating, reserves room for it, and re-appends it — so exactly one clean begin/end pair (and the top hash marker) always survives truncation. Defense-in-depth in the workflow: a new BODY_HAS_CLEAN_NOTES guard skips the edit whenever the live issue has clean notes but the fresh body does not. 2. Locale desync between grep and awk (❌ data loss). GNU grep in the runner's UTF-8 locale matches Unicode spaces (e.g. U+00A0, easily pasted from a web editor) as [[:space:]], but mawk (the runner default) does not. A stray non-breaking space before a marker could PASS the grep guard yet make the awk extract nothing, splicing an empty block over real notes. Fix: LC_ALL=C on every marker grep and the awk so both agree on ASCII-only [[:space:]]; a weird space now fails the guard and freezes the issue (safe) instead. 3. Hash bare-substring → preview false no-op (💡 low). The idempotency no-op extracted the hash with an unanchored substring grep, which also matched a `release-readiness-hash: sha=...` line a human pastes into the notes block. The splice copies that line into the fresh body, so on Preview trackers (which emit no real hash and must refresh every run) OLD_HASH==NEW_HASH and the tracker silently froze. Fix: anchor the grep to the engine's full `<!-- release-readiness-hash: sha=... -->` emission form. Added a regression test asserting the truncated body retains exactly one begin/end pair plus the hash marker. 502/502 PowerShell tests pass; YAML and bash -n clean; a 5-scenario mawk integration sim confirms splice / skip / edit / no-op all behave correctly. Known marginal edge case left as-is: manually swapping the begin/end markers (end before begin) bypasses the count guard and would capture the wrong span. Flagged LOW by one reviewer, requires a bizarre hand edit, and an ordering guard would add set -e fragility — deferred. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Multi-model adversarial review round 5 (3 reviewers + empirical repro): - UTF-8 boundary repair (3/3 consensus): the SR truncation loop trimmed only trailing continuation bytes, so it left an orphan multibyte LEAD byte AND stripped a COMPLETE trailing char to its lead -> GetString() emitted U+FFFD, which re-encodes to 3 bytes and could push the body back over MaxBodyBytes. Replaced with a sequence-length-aware cut that drops only an INCOMPLETE trailing sequence (repro: 6/13 cut positions buggy -> 0 after fix). Added a cap-sweep regression test (multibyte HEAD subject, caps 700-790). - Preview hash full-form freeze (R1): the anchored hash grep still matched the their notes; the splice carried it into the fresh body, freezing Preview trackers (no engine hash) via OLD_HASH==NEW_HASH. Scope extraction to the pre-notes region (sed '/begin/q') so any hash inside the notes is invisible to the compare, regardless of paste form. SR top hash + CRLF still correct. - Preview body-size cap: maestro/target PR tables use Add-PRTable's default 100-row cap, so a busy preview body can exceed GitHub's 65,536-byte limit and fail 'gh issue edit' (tracker stops updating). Ported a notes-safe prefix cap (notes block sits above the unbounded tables; above-notes sections are row-capped) reusing the corrected boundary-repair logic. Verified: SR suite 504/0, YAML parse, bash -n all 6 run-blocks, R1 pipeline under set -euo pipefail, Preview cap 218663->60000 with notes/tracker intact and no U+FFFD, Preview engine e2e smoke 17521B clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…uard (round 6) Adversarial review round 6 (Opus-4.8 + GPT-5.5 + Gemini-3.1-pro) found three issues in the round-5 changes, each independently verified against source with empirical reproduction: - Finding A (2/3 + verified): Preview's body-size cap used a byte-prefix cut, but the '🔴 High-priority items' table above the human-notes block is itemized and UNCAPPED. A busy report could push the notes begin/end markers past the cut, producing a markerless body that freezes the tracker or risks overwriting live Release Captain Notes. Mirror the SR engine: build the notes placeholder as a reusable block, strip it before truncation, then re-append it so exactly one clean begin/end pair always survives regardless of section order. Repro: 400-row table at a 4000-byte cap — old prefix-cut dropped the begin marker (count 0); new strip-and-reappend keeps 1/1, no U+FFFD, within cap. - Finding B (3/3): the workflow awk-splice injects the LIVE notes block into the freshly capped body; the engine caps the fresh body reserving room only for the small placeholder, so a large captain-authored notes block can push the merged body past GitHub's 65,536-byte limit and fail 'gh issue edit' under set -e. Add a post-splice 'wc -c' guard that skips the edit (notes safe, report stays stale, self-heals when report/notes shrink) instead of failing the run. - Finding 2 (3/3): Format-MarkdownCell escaped only '|'. A user-controlled issue/PR title of the form '<!-- release-readiness-hash: sha=... -->' rendered verbatim above the notes block, where the workflow's hash extraction would capture it as the semantic hash and freeze the Preview tracker (which emits no hash of its own). Escape '<'/'>' in cells so titles can't inject HTML comments; also fixes legitimate 'List<T>' titles. Engine markers are emitted via AppendLine (not the cell formatter), so they're unaffected. SR '<>'-escaping parity deliberately deferred: SR is freeze-immune (emits its own hash at the very top via AppendLine; the workflow uses head -n1) so the only benefit would be cosmetic, against real 504-suite breakage risk. Verified: SR suite 504/0; YAML parse OK; all 6 workflow bash run-blocks bash -n clean; Preview script parses; Finding A/B/2 repros pass; Preview e2e smoke 17521 bytes, 1/1 notes markers, no stderr. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
kubaflo
left a comment
There was a problem hiding this comment.
🤖 Multi-model code review — Request changes
Three models reviewed this PR independently (Claude Opus 4.8, GPT-5.5, Gemini 3.1 Pro), then cross-pollinated. (~10k lines, so the reviews prioritized security, workflow safety, and test execution over exhaustive line-by-line.)
Verdict: NEEDS_CHANGES (light) — the security and architecture are excellent; the asks are test-robustness fixes, not safety issues.
This is well-engineered 👍 (all three models agree)
- No injection.
gh/gitare invoked exclusively via argument arrays (& gh @GhArgs,& git -C $Repo @ArgList), so untrusted GitHub/AzDO data (PR titles, issue bodies, branch names) has no command-injection sink; the lone& $varis a local scriptblock formatter. - Safe workflow.
release-readiness.ymlusespull_request(notpull_request_target), keeps top-levelpermissions: contents: read, gates the onlyissues: writejob to non-PR events, and routes untrusted matrix values throughenv:instead of interpolating intorun:. - Scripts launched via
pwsh -NoProfile -File(the repo's recommended pattern), and the test suite is actually wired into CI.
Test-robustness (the asks)
- CI will rot (
release-readiness.yml:466) — the hard gate runs the E2E live known-answer suite (highestShippedTag == '10.0.71', tracker count == 4, SR8/SR9 specifics). These flip to failure as the repo evolves, reddening thevalidatejob on every future PR that touches the skill (plus network flakiness). Make the deterministic offline unit tests the gate; put E2E behind-SkipE2E/ opt-in. returnbypasses the finalexit(Test-ReleaseReadiness.ps1:148) — a thrown E2E invocation records the failure but the top-levelreturnskips the finalexit 1, so CI can go green on that catastrophic path. Fall through to the final exit (orexit 1).- Two low nits:
Invoke-Gitsplits args on spaces (mis-tokenizes paths with spaces; use[string[]]); one--jqfilter interpolates$MILESTONE_NAME(jq-only, fails safe; preferjq --arg).
Independent verdicts: GPT-5.5 — NEEDS_CHANGES (med-high) · Opus 4.8 — NEEDS_DISCUSSION (med) · Gemini 3.1 Pro — LGTM (high). All three found the security/architecture clean; the two non-LGTM verdicts converge on the E2E-in-CI test design.
kubaflo
left a comment
There was a problem hiding this comment.
🤖 Re-review — down to one item
Re-reviewed at the final head (all five hardening rounds in). This is in great shape — the security and architecture were already clean, and rounds 2–5 (stable hash, notes-preservation/awk-anchoring, UTF-8 truncation, Preview hash freeze, concurrency) closed the substantive concerns. I'm withdrawing everything from my earlier review except one item, so this is essentially ready.
The one remaining ask (a real CI false-green)
Test-ReleaseReadiness.ps1:148 — return bypasses the final exit. When the Get-ReleaseReadiness.ps1 invocation raises a PowerShell-level error, the catch increments $script:failed then returns, which skips the terminal exit $(if ($script:failed) {1} else {0}) — so the test runner can exit 0 even though the script under test crashed. It's a narrow trigger (a parent-level throw, not a child non-zero exit), but it defeats the gate in exactly the catastrophic case. One-line fix: exit 1 instead of return (you can't fall through — the lines below assume $outDir exists). See inline.
Downgraded to non-blocking 👍
- Live known-answer E2E as the
validategate (highestShippedTag == '10.0.71', SR7 #35428/#35609, counts): I flagged this as rot-prone, but it's a deliberate, valuable signal and you clearly want live coverage — treating it as a non-blocking suggestion (consider-SkipE2Edeterministic tests as the hard gate with E2E opt-in, so repo evolution doesn't red unrelated PRs). The two earlier low nits (Invoke-Gitspace-splitting; one--jq $MILESTONE_NAMEinterpolation) are fail-safe — not blocking.
@PureWeen — fix that one return and I'm good here. Thanks for the very thorough hardening passes. 🙏
Re-review by the 3-model panel (Opus 4.8 · GPT-5.5 · Gemini 3.1 Pro); the security/architecture remain clean across all three.
A newcomer asking a generic "status on releases and what needs to be done next" had no reliable route: every trigger phrase and "When to Use" example was keyed on a specific named release, the agent's routing was branch-singular (it would ask "which release?"), and the live [Release Readiness] tracker issues were undiscoverable from the human-facing agent path (their existence was documented only in the cron-workflow section). Adds, docs-only (no engine code touched): - Agent: a Portfolio path that enumerates active releases by reading the open tracker issues via the body marker (in:body, not title, which false-positives on the release Epic), reads the generated report and the human Release Captain Notes (which supersede the verdict), guards staleness against updatedAt/hash, and presents a ship-urgency-ordered roll-up. Generic phrases now route here instead of asking which release. - SKILL.md: portfolio When-to-Use entry plus a Reading-trackers-directly fast path so ad-hoc status does not require a 60-120s survey re-run. - copilot-instructions.md: generic portfolio trigger phrases plus a delegation example. Triggers kept release-scoped to avoid colliding with issue-triage / find-reviewable-pr. Validated by two independent model reviews (both confirmed the three gaps and endorsed the fixes with these refinements baked in). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
kubaflo
left a comment
There was a problem hiding this comment.
🤖 Re-review — round 3 (head 6ae8cae)
Reviewed the new commit (portfolio / cross-release discovery).
New feature 👍
The portfolio mode is a clean, doc-only addition — enumerating active releases by reading the [Release Readiness] tracker issues (and correctly keying off the body marker <!-- release-readiness-tracker: rather than the title, which would also match the Epic) is a nice "always-on status board" that avoids a 60–120s survey re-run. The Release-Captain-Notes override path is well-documented. No concerns here.
Still open (carried from round 2)
One real item remains — this commit didn't touch it: Test-ReleaseReadiness.ps1:148 — return bypasses the final exit. The catch does $script:failed++ then return, which skips the terminal exit $(if ($script:failed) {1} else {0}), so a PowerShell-level failure of the Get-ReleaseReadiness.ps1 invocation can still exit 0 (CI green) even though the script under test crashed. One-line fix: exit 1 instead of return.
@PureWeen — just that one line and I'm good. Thanks!
3-model panel basis (Opus 4.8 · GPT-5.5 · Gemini 3.1 Pro); this round was a light re-review of the doc-only feature delta.
A bare `return` in the E2E "script invocation failed" catch block executes
at script scope, which returns from the entire test script and bypasses the
terminal `exit $(if ($script:failed -eq 0) {0} else {1})` at the end. The
catch increments $script:failed but, because the terminal exit never runs,
the process exit code falls back to $LASTEXITCODE (or 0) — so a crash of the
Get-ReleaseReadiness.ps1 script-under-test could still report CI green.
Replace `return` with `exit 1` so an E2E launch failure is an unambiguous
non-zero exit. The change only affects the error path; the passing run is
unaffected (verified: Passed: 504 Failed: 0).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
All review feedback addressed — the E2E catch now exit 1s instead of return (commit bfc12a1), exactly as suggested; the E2E-as-CI-gate concern and the minor nits were previously downgraded to non-blocking. No outstanding asks from this automated multi-model review (LGTM). Dismissing this now-stale changes-request; final approval/merge intentionally deferred to a human maintainer.
…anches from the tracker matrix (#35971) > [!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! Follow-up to #35807. Two independent release-readiness fixes. --- ## Fix 1 — Surface p/0-labelled PRs as Preview release blockers ### Summary The Preview readiness engine (`Get-PreviewReadiness.ps1`) only treated **p/0 issues** as release blockers. **p/0-labelled PRs** targeting a preview/candidate branch were silently bucketed into the generic "Release branch PRs" WATCH count and rendered as "Needs review or triage" rows — never hoisted, never blocking. The root cause is structural: the p/0 blocker path used `gh issue list --label p/0`, which **by design never returns PRs**. So p/0 PRs were invisible to the blocker logic. This was observed live on the **net11.0 preview6** tracker (#35866), where #34758, #35626, and #34600 (all `p/0`, base `net11.0`) appeared only as generic WATCH rows instead of blockers. ### What changed Carves p/0-labelled PRs out of the generic human-PR bucket — the label data is **already fetched** by `Get-OpenPullRequests`, so no extra API call — and: - adds a **BLOCKED `P/0 release-branch PRs`** check (parallel to the p/0-issues check) so the overall verdict turns red when one is open; - itemizes each p/0 PR as a **`🔥 P/0 PR`** row in the hoisted **🔴 High-priority items** section (with base ref + age + per-PR next action); - **excludes** the new check from the **🔴 Blocking** summary (its PRs are already enumerated in the hoist) — exactly matching the p/0-issues treatment; - updates the WATCH note + hoist header/intro text from 3 → 4 high-priority categories. A PR whose base **is** the survey ref is release-relevant by definition, so — unlike issues — no title/milestone relevance filter is applied. ### Testability Adds a small **StrictMode-safe `Test-IsP0Pr`** helper plus a **dot-source guard** on the engine (mirroring `Find-ReleaseReadinessTrackers.ps1`) so the predicate can be unit-tested without invoking the full git/gh-backed report flow. --- ## Fix 2 — Drop stale below-watermark SR branches from the tracker matrix ### Summary The Lane 1 in-flight detector (`Find-ReleaseReadinessTrackers.ps1`) treated **tag-absence** as the sole in-flight signal. Abandoned hotfix leftovers like **SR2** (patch 21) and **SR3** (patch 33) — which never published their stable tags and sit far below the shipped watermark (**SR7** patch 71) — were still emitted as trackers. The workflow then spun up a **no-op matrix job** per branch: the per-job activity gate skipped issue creation, but the job still ran. ### What changed Adds a secondary **`Test-IsStaleSrBranch`** disambiguator applied **only after** `Test-IsBranchInFlight` returns true. A branch is stale when **both**: - its patch is **strictly below** the highest shipped patch, **and** - it has had **no commits** within the activity window (idle). Tag-existence stays the **primary** signal; the idle requirement preserves the out-of-order / security-hotfix case — a real reset branch below the watermark has recent commits and is therefore **not** dropped. Freshly-cut live SRs sit at/above the watermark and are never affected. Dropping these at the detector removes them from the workflow matrix **entirely**. Verified safe: SR2/SR3 have no open tracker issues, so nothing is stranded (only SR8/SR9/preview6 have open trackers). --- ### Tests `Test-ReleaseReadiness.ps1`: - **12** new unit assertions for `Test-IsP0Pr` (predicate: p/0 present/absent, missing/null/empty labels, hashtable-shaped labels, null PR; carve-out semantics: p/0 subset selected, generic bucket excludes them). - **7** new unit assertions for `Test-IsStaleSrBranch` (below-watermark idle → stale; above/equal watermark → not stale; below-watermark but active → not stale; no shipped tags → never fires). - Live-repo E2E expectations updated: net10 now surfaces **2** SR trackers (SR8 + SR9) instead of 4. ``` Passed: 517 Failed: 0 ``` --------- Co-authored-by: Copilot <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!
Summary
Adds a deterministic, evidence-backed release-readiness skill that produces a single "Is
release/X.Y.Zxx-srN(or preview) ready to ship?" report for .NET MAUI release branches — both Servicing Releases (SR) and Previews, in both in-flight and candidate (pre-cut) modes.Supersedes #35754.
What it does
Get-ReleaseReadiness.ps1walks the SR branch, classifies openregressed-in-*issues against branch contents, computes the source-PR list (handling cherry-pick number swaps + non-main forward-flow), and rolls up an "is this ready to ship?" verdict with a Blocking summary hoisted to the top of the report.Posts/refreshes a single
[Release Readiness]GitHub tracking issue per release lane (idempotent via a semantic hash marker — only reposts when something meaningfully changed). See issue #35876 (SR8) for a live example.Ship-readiness checks
A release captain sees these surface as 🟢 READY / 🟡 WATCH / 🔴 BLOCKED / ⚪ UNKNOWN rows. All BLOCKED rows roll up into the Blocking summary at the top.
PreReleaseVersionLabel=servicing+StabilizePackageVersion=truenot applied — branch silently builds prerelease packages.NET <band> SDKin BAR — caught the real SR8 outageEach check that needs external tooling (darc, gh, milestone API) degrades to UNKNOWN with the exact verification command embedded — the report never silently skips.
Expected ship date
Header line surfaces the deadline. Cadence is patch-aware:
PatchVersionends in 0 (80,90,100…) or0(previews) → 2nd Tuesday of the month81,82,91…) → ASAP hotfix, no cadenceCustom agent
.github/agents/release-readiness-agent.agent.mdwraps the skill — handles regression-label confirmation, runs the script, then uses WorkIQ + maestro MCP to:rejected-from-srPRs (chat history, review feedback)Testing
pwsh .github/skills/release-readiness/tests/Test-ReleaseReadiness.ps1 # 447 pass / 0 failDogfooded live against SR7 + SR8 + the .NET 11 preview lane. Caught real-world bugs:
maestro_default_channelsMCP).NET 10 SR6+.NET 10 SR7milestones open with 76 + 63 open issues, past due.github/ISSUE_TEMPLATE/bug-report.ymlmissing10.0.80entryMethodology gotchas (documented in
references/methodology.md)closedByPullRequestsReferencesreturns empty for most MAUI issues; must walkgh api .../issues/N/timelinecross-referenced eventsinflight/currentonly, notmain(real example: PR [iOS / Mac] Fix CollectionView.ScrollTo(index) silently failing whenIsGrouped="True" #35609)Files
.github/skills/release-readiness/SKILL.md— skill entry point + reference docs.github/skills/release-readiness/scripts/Get-ReleaseReadiness.ps1— main orchestrator (deterministic, no MCP).github/skills/release-readiness/tests/Test-ReleaseReadiness.ps1— 447-assertion test suite.github/skills/release-readiness/references/methodology.md— gotchas and patterns.github/agents/release-readiness-agent.agent.md— MCP-enriched agent wrapping the skill