Add preview workload installability readiness check - #36569
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3b2e86e0-ce82-4e1b-94ec-d554098fcf02
GitHub Actions
Inactive
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 36569Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 36569" |
|
Azure Pipelines: 1 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
GitHub Actions
Inactive
Skill Validation Results
✅ Skill Validation Results —
|
GitHub Actions
Inactive
There was a problem hiding this comment.
Pull request overview
Adds a deterministic Preview consumer-installability readiness signal to the release-readiness tooling so “green branch/CI” is no longer treated as sufficient evidence that customers can actually install the confirmed Preview workload set (including feed/source isolation, pin coherence, required manifest availability, representative pack probes, prerequisite extraction, and public-safe redaction).
Changes:
- Introduces
PreviewInstallability.ps1to evaluate workload-set + asset installability withREADY/BLOCKED/UNKNOWNsemantics and public-safe output redaction. - Integrates the installability gate into
Get-PreviewReadiness.ps1checks and Markdown report output, with new parameters for confirmed workload-set version and optional authenticated source. - Expands unit test coverage and documentation/methodology to cover the new Preview installability gate and operational workflow.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| .github/skills/release-readiness/tests/Test-ReleaseReadiness.ps1 | Adds offline unit fixtures and assertions for Preview installability resolution, probing, mapping, and redaction. |
| .github/skills/release-readiness/SKILL.md | Documents the new gate, how to run it locally (including PAT guidance), and new CLI parameters. |
| .github/skills/release-readiness/scripts/PreviewInstallability.ps1 | New helper implementing workload-set discovery/validation, source isolation, probing, prerequisites, and sanitization. |
| .github/skills/release-readiness/scripts/Get-PreviewReadiness.ps1 | Wires the installability helper into the Preview readiness checks, report object, and Markdown output. |
| .github/skills/release-readiness/references/methodology.md | Adds methodology section describing the Preview consumer-installability algorithm and trust boundaries. |
GitHub Actions
Inactive
GitHub Actions
Inactive
github-actions
Bot
added
the
area-infrastructure
GitHub Actions
Inactive
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3b2e86e0-ce82-4e1b-94ec-d554098fcf02
Merges main (including PR #36760's release-readiness tracker accuracy and post-ship guidance work) into the preview workload installability readiness check branch. Resolved conflicts in: - .github/skills/release-readiness/SKILL.md - .github/skills/release-readiness/references/methodology.md - .github/skills/release-readiness/scripts/Get-PreviewReadiness.ps1 Preserved the Consumer installability gate (PreviewInstallability.ps1, ConfirmedWorkloadSetVersion/AdditionalPackageSource params) alongside main's PublicReportSanitizer helper, component-pin readiness check, component-policy markdown block, and preview-iteration check. Removed a resulting duplicate $componentPins computation so both the installability gate and the component-build section reuse a single branch-pin lookup. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 68573d91-fd15-4374-adf5-c959169a31b8
There was a problem hiding this comment.
Copilot's findings
Comments suppressed due to low confidence (1)
.github/skills/release-readiness/scripts/PreviewInstallability.ps1:508
Find-PreviewPackageLocationcurrently treats any exception from an additional source as "unknown" (because of-or $resolved.Source.IsAdditional), including HTTP 404. That can incorrectly downgrade a truly missing package (404 from all accessible sources) intounknown, which contradicts the intended "missing vs unknown" semantics (401/403 should be unknown; 404 should count as not-found evidence).
$status = Get-InstallabilityHttpStatus $_.Exception
if ($status -in @(401, 403) -or $resolved.Source.IsAdditional) {
[void]$unknownSources.Add($resolved.Source.Name)
} elseif ($status -ne 404) {
[void]$unknownSources.Add($resolved.Source.Name)
}
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
PureWeen
left a comment
PureWeen
left a comment
There was a problem hiding this comment.
Adversarial code review — 3 independent reviewers + consensus
Methodology: 3 independent reviewers analyzed this PR's 5 changed files in parallel, focusing on regressions, security, logic errors, data loss, and race conditions. Findings were cross-validated for consensus; several borderline/low-confidence items were additionally verified directly against the current source (not just the diff) before inclusion. This review is scoped strictly to the 5 files this PR changed — SKILL.md, references/methodology.md, scripts/Get-PreviewReadiness.ps1, scripts/PreviewInstallability.ps1, tests/Test-ReleaseReadiness.ps1 — the branch was recently merged with main, so the checkout also contains unrelated files that were intentionally excluded from scope.
Merge-correctness note: The recent merge with main combined this PR's installability gate with main's independent release-readiness tracker work (PR #36760). The merge itself was checked for correctness (variable-scoping, ordering between the new "Consumer installability" and "Component pin inventory" sections, and the deduplicated $componentPins computation) — no merge-introduced defects were found; $componentPins is computed exactly once and correctly reused by both sections.
Findings (new, not previously flagged)
All 8 findings below were independently verified against the current source before posting (function bodies read in full, cross-referenced against callers/callees):
- ❌ Data Loss (
PreviewInstallability.ps1:1033) — additional/authenticated source names can leak into the publicUnknownSourcesfield when overall status isinstallable, because the redaction allowlist (RequiredSources) excludes additional sources on the success path. - ❌ Data Loss / Documentation mismatch (
PreviewInstallability.ps1:1091) — the embargoed confirmed CLI/NuGet version and per-component resolved build numbers are rendered into public-safe markdown unconditionally, regardless of$PublicSafe. ⚠️ Security / Regression of convention (PreviewInstallability.ps1:1101) — feed-derived content is rendered into markdown tables without theFormat-MarkdownCellescaping used everywhere else in this skill.⚠️ Logic (PreviewInstallability.ps1:919) — unconfirmed-candidate runs can resolve toBLOCKEDinstead of the documentedUNKNOWNwhen no discovered version has coherent pins.⚠️ Logic (PreviewInstallability.ps1:907) — an'unverified'pin-comparison status (incomplete pin data) is accepted as coherent by the top-level check.- 💡 Security (2/3 reviewer consensus) (
PreviewInstallability.ps1:175) — service-index-derived URLs aren't re-validated against the original source host before attaching credentials. - 💡 Security (
PreviewInstallability.ps1:93) — additional-source URL validation doesn't reject query/fragment components. - 💡 Logic (
PreviewInstallability.ps1:248) — stray/in the discovery search URL construction.
Test coverage observations (non-blocking)
Get-PackageSourceHeaders(credential env-var → Basic-auth-header parsing — the credential choke point this PR's root cause is about) has no direct unit test coverage in the new test block.- The public-redaction tests (
Test-ReleaseReadiness.ps1~1959–1978) use fixtures withResolvedSource = $null, so they wouldn't catch a regression in stripping a populatedResolvedSource.Source.Uri/FlatUrlfrom a found package location.
Prior review status
The existing automated review already flagged 5 issues (tvOS/Emscripten manifest-request gaps at PreviewInstallability.ps1:415/418, the Find-PreviewPackageLocation missing-vs-unknown ambiguity at :518, the ValidAuthenticationTypes=Basic doc/impl mismatch at methodology.md:404, and the unauthenticated-unavailable test gap at Test-ReleaseReadiness.ps1:9121). All were independently re-verified as genuine during this review and are not duplicated above.
Test coverage of changed code
The new installability logic is covered by ~337 new test lines (Test-ReleaseReadiness.ps1 ~8838–9175); the full suite passes (2074/2074 after the merge with main). The gaps noted above are coverage gaps in otherwise well-tested new code, not evidence the new tests are wrong.
Addresses high-confidence findings from a multi-model adversarial code review of PR #36569 (posted as GitHub review #4812833050): - Sensitive-source-name leak: ConvertTo-PublicInstallabilityResult now accepts a -Sources parameter and treats every additional/internal source configured for the run as sensitive, not just the ones that ended up in $Result.RequiredSources. A source that failed for one package but was never "required" (because a later source satisfied it) could otherwise leak its real name through that location's UnknownSources in public-safe output. All 6 call sites of Complete-PreviewInstallabilityResult now pass -Sources through. - Embargoed CLI/NuGet version leak: a confirmed workload-set build's version was still exposed via InstallCommand and each pin comparison's Actual value even though CliVersion/NuGetVersion were withheld. InstallCommand now uses a withheld placeholder and each PinComparisons Actual becomes 'withheld' when VersionConfirmed is true, while WorkloadId/Expected/Status stay intact so the pin- coherence signal is preserved. Unconfirmed (discovered) candidates are left unredacted, matching existing intent. - Unconfirmed-candidate status mapping: an unconfirmed run (no release-owner-supplied CLI version) whose only discoverable candidate failed pin coherence, or had missing/unresolvable assets, was mapped to mismatched/missing -> BLOCKED. Only a genuinely confirmed candidate's asset/pin problems are real BLOCKED signals; an unconfirmed candidate now stays unknown in both cases. - Unverified pin status treated as coherent: a pin comparison whose expected value could not be determined (Compare- PreviewWorkloadSetPins reports unverified) was not excluded from the coherence check, so a workload set could be reported installable even though one of its components was never actually verified against branch pins. Unverified is now treated the same as mismatch/missing for candidate selection. - Markdown injection via feed-derived source names: source names interpolated into the rendered markdown table were not escaped, so a pipe could break table structure and a less-than sign could be read as an HTML tag by downstream renderers. Added a self-contained Format-InstallabilityMarkdownCell helper (mirroring Format- MarkdownCell's intent in Get-PreviewReadiness.ps1, duplicated so this file stays independently dot-sourceable in unit tests) and applied it to all source-name interpolations in Format-PreviewInstallabilityMarkdown. Adds 20 focused regression tests in Test-ReleaseReadiness.ps1 covering each fix directly (redaction with/without -Sources, embargoed-version redaction for CliVersion/NuGetVersion/InstallCommand/PinComparisons, confirmed-vs-unconfirmed status mapping for both mismatch and missing- asset cases, unverified pin handling, and markdown-cell escaping). Test-ReleaseReadiness.ps1 -SkipE2E: 1961/1961 passed (was 1941/1941). Full suite (no -SkipE2E): 2094/2094 passed (was 2074/2074). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 68573d91-fd15-4374-adf5-c959169a31b8
Follow-up: fixes applied for the adversarial review findingsCommit 57c7025 (pushed to this branch) addresses every ❌ and
Added 20 new regression tests in Test results: Remaining 💡-severity items (PAT sent to service-index-derived host without re-validation, additional-source URL validation not rejecting query/fragment components, a stray |
There was a problem hiding this comment.
Copilot's findings
Comments suppressed due to low confidence (1)
.github/skills/release-readiness/references/methodology.md:404
- This section states that
NuGetPackageSourceCredentials_<name>"must contain" non-emptyUsername/PasswordandValidAuthenticationTypes=Basic, but the current implementation (Get-PackageSourceHeadersin PreviewInstallability.ps1) only requires a non-empty Password and does not validateValidAuthenticationTypes. Either tighten the code to enforce these requirements or soften the wording here to match actual behavior.
embedded in a URL is rejected. Credentials come from NuGet's
`NuGetPackageSourceCredentials_<name>` environment variable, where `<name>`
exactly matches the source name. The value must contain non-empty `Username`
and `Password` fields and `ValidAuthenticationTypes=Basic`.
- Files reviewed: 5/5 changed files
- Comments generated: 1
Require conclusive package-source probes, complete manifest evidence, trusted Basic-auth destinations, and public-safe redaction. Add focused regression coverage for the hardened semantics. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 68573d91-fd15-4374-adf5-c959169a31b8
There was a problem hiding this comment.
Copilot's findings
Comments suppressed due to low confidence (2)
.github/skills/release-readiness/scripts/PreviewInstallability.ps1:23
- Format-InstallabilityMarkdownCell escapes
|with a simple-replace '\|', '\|', but it doesn’t implement the existing hardening used by Format-MarkdownCell (Get-PreviewReadiness.ps1) for the\|edge case. If a value already contains a backslash before a pipe (e.g.A \| B), this implementation can produce\\|, which GFM can interpret as a literal\plus an active column delimiter, breaking table structure. Consider porting the same(\\*)\|doubling logic and trimming/collapsing newlines as in Format-MarkdownCell.
if ([string]::IsNullOrEmpty($Value)) { return '' }
$escaped = $Value -replace "`r`n", ' ' -replace "`n", ' ' -replace "`r", ' '
$escaped = $escaped -replace '\|', '\|'
$escaped = $escaped -replace '<', '<' -replace '>', '>'
return $escaped
.github/skills/release-readiness/tests/Test-ReleaseReadiness.ps1:9487
- The new tests for Format-InstallabilityMarkdownCell cover raw
|,<, and newlines, but they don’t cover the already-escaped\|case that Format-MarkdownCell explicitly guards against. Adding a regression assertion forA \| Bwould ensure the installability markdown table can’t be broken by a pre-escaped backslash+pipe sequence.
Assert-Eq -Label "installability markdown: pipe in source name does not break table structure" `
-Expected 'a \| b' -Actual (Format-InstallabilityMarkdownCell 'a | b')
Assert-Eq -Label "installability markdown: angle brackets are escaped" `
-Expected '<script>' -Actual (Format-InstallabilityMarkdownCell '<script>')
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 68573d91-fd15-4374-adf5-c959169a31b8
PureWeen
left a comment
PureWeen
left a comment
There was a problem hiding this comment.
Adversarial review
No unresolved high-confidence correctness findings remain on the current pushed candidate.
This pass found and fixed three consensus-backed issue groups:
- public fallback and authenticated-source-derived versions could escape public-safe redaction, including nested manifest and representative-pack evidence;
- an unresolved SDK pin discarded an already supplied workload-set confirmation and produced incorrect remediation;
- tvOS and Emscripten manifests were required, but their representative pack availability was not part of the READY gate.
The accepted cases now have focused regression coverage, including public-feed candidates remaining visible while confirmed or authenticated-source-derived versions are withheld. All prior review threads are resolved.
Methodology: 3 independent reviewers with adversarial consensus.
kubaflo
left a comment
kubaflo
left a comment
There was a problem hiding this comment.
@rmarinho — adversarial re-review of 0e821e134c33a53991564ded0be686b31fc11679 using independent Claude Opus 5, GPT-5.6 Sol, Gemini 3.1 Pro, MAUI expert verification, cross-pollination, and GPT-5.6 Terra arbitration.
The prior source-redaction, confirmed-version, URL-validation, Basic-auth, manifest-coverage, unconfirmed-candidate, and Markdown-escaping findings are fixed. The new gate still false-blocks a coherent published preview on real workload manifests, and two other evidence-loss paths map uncertainty to BLOCKED; see the three inline comments.
Required CI: license/cla passed; maui-pr is skipping, so required build coverage is undetermined. The title is accurate, but the description's deterministic installability and malformed-metadata claims currently overstate the implementation, and the 2133 passing tests use fixtures that do not model real alias-to pack entries.
Textual verdict: NEEDS_CHANGES. Confidence: low for merge safety because this is shared release infrastructure with skipped required CI; confidence in the reproduced findings is high.
Resolve manifest pack aliases before package probing, distinguish malformed package metadata from confirmed absence, and keep candidates with unavailable pin evidence unknown. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 68573d91-fd15-4374-adf5-c959169a31b8
There was a problem hiding this comment.
Copilot's findings
Suppressed comments (3)
.github/skills/release-readiness/scripts/PreviewInstallability.ps1:118
- Casting with
$uri = [Uri]$uriTextwill throw a generic conversion exception for malformed URIs (before your tailored validation errors run), which can be hard to interpret for users. Prefer usingTryCreate(or wrapping the cast in atry/catch) and throwing a purpose-built message like “Additional package source '' has an invalid URI” so failures are consistently actionable.
$name = $Matches.name
$uriText = $Matches.uri
$uri = [Uri]$uriText
if ($uri.Scheme -ne 'https' -or
$uri.Host -ne 'pkgs.dev.azure.com' -or
-not [string]::IsNullOrEmpty($uri.UserInfo) -or
-not [string]::IsNullOrEmpty($uri.Query) -or
-not [string]::IsNullOrEmpty($uri.Fragment)) {
throw "Additional package source '$name' must be an HTTPS dnceng Azure Artifacts service index without user information, query parameters, or fragments."
.github/skills/release-readiness/scripts/PreviewInstallability.ps1:906
ConvertTo-PublicInstallabilityResultstill emits anInstallCommandin public-safe output. Even though the config path is placeholder text, this can still be interpreted as “safe to copy/paste from a public report,” and it may conflict with the stated intent that installation instructions are local-only when-PublicSafe $false. Consider settingInstallCommandto$nullin public-safe results (and only emitting commands/config in the non-public flow), or updating the docs/tests to explicitly state that a redacted placeholder install command is expected to appear in public JSON.
$copy.NuGetConfig = $null
# A release-owner-confirmed build or a candidate learned from an authenticated/internal
# source is sensitive. Keep public candidates visible, but withhold sensitive top-level
# and nested versions while retaining match/mismatch/missing status as coherence evidence.
if ($withholdVersions) {
$publicSummary = [string]$Result.Summary
foreach ($version in @($Result.CliVersion, $Result.NuGetVersion)) {
if (-not [string]::IsNullOrWhiteSpace([string]$version)) {
$publicSummary = [regex]::Replace(
$publicSummary,
[regex]::Escape([string]$version),
'withheld',
[Text.RegularExpressions.RegexOptions]::IgnoreCase)
}
}
$copy.Summary = $publicSummary
$copy.CliVersion = if ([string]::IsNullOrWhiteSpace([string]$Result.CliVersion)) {
$Result.CliVersion
} else {
'withheld'
}
$copy.NuGetVersion = if ([string]::IsNullOrWhiteSpace([string]$Result.NuGetVersion)) {
$Result.NuGetVersion
} else {
'withheld'
}
$copy.InstallCommand = if ($Result.CliVersion) {
'dotnet workload install maui --version <withheld> --configfile <local-nuget-config>'
} else { $null }
$copy.PinComparisons = @($Result.PinComparisons | ForEach-Object {
[PSCustomObject]@{
WorkloadId = $_.WorkloadId
Expected = $_.Expected
Actual = if ([string]::IsNullOrWhiteSpace([string]$_.Actual)) { $_.Actual } else { 'withheld' }
Status = $_.Status
}
})
} else {
$copy.InstallCommand = if ($Result.CliVersion) {
"dotnet workload install maui --version $($Result.CliVersion) --configfile <local-nuget-config>"
} else { $null }
}
.github/skills/release-readiness/references/methodology.md:402
- Add a space between “NuGet's” and the inline-code span for readability/markdown consistency.
embedded in a URL, query parameters, and fragments are rejected. Credentials come from NuGet's
`NuGetPackageSourceCredentials_<name>` environment variable, where `<name>`
- Files reviewed: 5/5 changed files
- Comments generated: 1
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (1)
.github/skills/release-readiness/scripts/PreviewInstallability.ps1:625
- Get-PreviewRuntimeIdentifier returns RuntimeInformation.RuntimeIdentifier verbatim when available. That value can include OS-version-specific RIDs (e.g. "win10-x64"), which will not match workload-manifest
alias-tokeys like "win-x64" (see unit tests usingalias-towithwin-x64). This can preventResolve-PreviewManifestPackIdfrom resolving the physical pack ID on Windows and incorrectly degrade representative-pack evidence to "not derived"/UNKNOWN.
function Get-PreviewRuntimeIdentifier {
$runtimeIdentifier = [System.Runtime.InteropServices.RuntimeInformation]::RuntimeIdentifier
if (-not [string]::IsNullOrWhiteSpace($runtimeIdentifier)) {
return $runtimeIdentifier
}
$architecture = [System.Runtime.InteropServices.RuntimeInformation]::OSArchitecture.ToString().ToLowerInvariant()
if ([System.Runtime.InteropServices.RuntimeInformation]::IsOSPlatform(
[System.Runtime.InteropServices.OSPlatform]::Windows)) {
return "win-$architecture"
}
if ([System.Runtime.InteropServices.RuntimeInformation]::IsOSPlatform(
[System.Runtime.InteropServices.OSPlatform]::OSX)) {
return "osx-$architecture"
}
return "linux-$architecture"
Withhold nested versions resolved from authenticated sources even when the workload-set package is public, and keep unconfirmed-run remediation focused on supplying the confirmed version. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 68573d91-fd15-4374-adf5-c959169a31b8
There was a problem hiding this comment.
Copilot's findings
Suppressed comments (3)
.github/skills/release-readiness/scripts/PreviewInstallability.ps1:122
- The error message claims the additional source must be an 'Azure Artifacts service index', but the validation currently allows any
https://pkgs.dev.azure.com/dnceng/...path. Since credentials may be attached to requests under this trust boundary, tighten the allowlist to Azure Artifacts NuGet v3 index paths (e.g., require the path to match/_packaging/<feed>/nuget/v3/index.jsonunder/dnceng/). This both aligns behavior with the message and reduces the risk of sending credentials to unexpected endpoints on the same host.
$uri = [Uri]$uriText
if ($uri.Scheme -ne 'https' -or
$uri.Host -ne 'pkgs.dev.azure.com' -or
-not [string]::IsNullOrEmpty($uri.UserInfo) -or
-not [string]::IsNullOrEmpty($uri.Query) -or
-not [string]::IsNullOrEmpty($uri.Fragment)) {
throw "Additional package source '$name' must be an HTTPS dnceng Azure Artifacts service index without user information, query parameters, or fragments."
}
if (-not $uri.AbsolutePath.StartsWith('/dnceng/', [StringComparison]::OrdinalIgnoreCase)) {
throw "Additional Azure Artifacts source '$name' must belong to the dnceng organization."
}
.github/skills/release-readiness/tests/Test-ReleaseReadiness.ps1:8892
- The test overwrites and then clears
NuGetPackageSourceCredentials_<name>but doesn’t restore any prior value that may already exist in the process environment (e.g., when running tests locally with a configured credential). Capture the previous value before setting it, and restore that value infinallyinstead of always setting$null.
[Environment]::SetEnvironmentVariable(
$iiCredentialVariable,
'Username=release-readiness;Password=test-token;ValidAuthenticationTypes=Basic')
$iiBasicHeaders = Get-PackageSourceHeaders -Source $iiCredentialContractSource `
-RequestUrl 'https://pkgs.dev.azure.com/dnceng/internal/_packaging/example/nuget/v3/flat2'
.github/skills/release-readiness/tests/Test-ReleaseReadiness.ps1:8933
- The test overwrites and then clears
NuGetPackageSourceCredentials_<name>but doesn’t restore any prior value that may already exist in the process environment (e.g., when running tests locally with a configured credential). Capture the previous value before setting it, and restore that value infinallyinstead of always setting$null.
} finally {
[Environment]::SetEnvironmentVariable($iiCredentialVariable, $null)
}
- Files reviewed: 5/5 changed files
- Comments generated: 1
Carry authenticated manifest provenance into unresolved representative-pack evidence so public JSON and Markdown cannot disclose embargoed pack versions. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 68573d91-fd15-4374-adf5-c959169a31b8
PureWeen
left a comment
PureWeen
left a comment
There was a problem hiding this comment.
Adversarial review — final clean head
No unresolved high-confidence correctness or security findings remain at 572850a03669a5549384dc39def6f2c323895c09.
The final rounds verified mixed public/authenticated-source version redaction, unresolved-pack manifest provenance, manifest alias-to resolution, malformed package-index UNKNOWN semantics, and confirmed-versus-unconfirmed pin remediation. Test coverage includes focused positive and discriminating negative fixtures across public JSON and Markdown output; the complete release-readiness harness passes 2150/2150 and the deterministic path passes 2017/2017.
Prior review status: all existing threads are resolved, and their findings were independently rechecked against the current head.
Methodology: 3 independent reviewers with adversarial consensus + repo domain specialist. Event: COMMENT.
There was a problem hiding this comment.
Copilot's findings
Suppressed comments (3)
.github/skills/release-readiness/scripts/PreviewInstallability.ps1:22
- Escaping
</>without also escaping&still allows entity-based HTML injection into markdown/HTML renderers (e.g., input<script>will render as<script>). Consider escaping&first (to&) before substituting</>so pre-escaped entities can’t bypass the guard.
$escaped = $escaped -replace '<', '<' -replace '>', '>'
.github/skills/release-readiness/scripts/PreviewInstallability.ps1:160
- If a user has
NuGetPackageSourceCredentials_<name>set for a non-dnceng source (e.g., a commonnuget.org-named credential entry), this throws and can break the entire readiness run even though the script could safely ignore those credentials. A more resilient approach is to return empty headers (optionally logging a warning) when the source/request URL isn’t trusted, rather than throwing after detecting the variable is set.
$variableName = "NuGetPackageSourceCredentials_$($Source.Name)"
$credential = [Environment]::GetEnvironmentVariable($variableName)
if ([string]::IsNullOrWhiteSpace($credential)) { return @{} }
if (-not (Test-IsTrustedDncengPackageUri -Uri ([string]$Source.Uri)) -or
-not (Test-IsTrustedDncengPackageUri -Uri $RequestUrl)) {
throw "Credentials from '$variableName' can only be sent to HTTPS dnceng Azure Artifacts endpoints."
}
.github/skills/release-readiness/scripts/PreviewInstallability.ps1:457
- When a component pin is missing (so
Expectedis empty andStatusbecomesunverified), theExpectedfield is still populated aspreview.<N> build. That fallback only makes sense for the MAUI pattern-based check; for Android/Apple/VMR pins it’s misleading evidence in the rendered report. Consider emitting a pin-specific placeholder (e.g.,unavailable/(pin not found)) when no explicit expected value exists and there is no pattern, and reservingpreview.<N> buildfor the MAUI pattern case.
[void]$comparisons.Add([PSCustomObject]@{
WorkloadId = $expectation.Id
Expected = if ($expectation.Expected) { $expectation.Expected } else { "preview.$Preview build" }
Actual = $actual
Status = $status
})
- Files reviewed: 5/5 changed files
- Comments generated: 1
Note
Are you waiting for the changes in this PR to be merged?
It would be very helpful if you could test the resulting artifacts from this PR and let us know in a comment if this change resolves your issue. Thank you!
Description of Change
READY,BLOCKED, andUNKNOWNsemantics.<clear />NuGet configuration for local validation while recursively removing authenticated source details and sensitive confirmed/authenticated-source-derived version values from public tracker output.ValidAuthenticationTypes=Basic.Root Cause
Preview branch and CI health do not prove that customers can install the release. During Preview 6 validation, the SDK and workload-set pins were coherent, but installation still depended on package-source configuration and assets unavailable from the initially configured feeds. Authentication, network, malformed metadata, and service-index failures must remain
UNKNOWNrather than being misreported as missing packages;MISSINGrequires a conclusive package-index probe.Security Boundary
NuGetPackageSourceCredentials_<name>, require non-emptyUsername/Passwordplus exactlyValidAuthenticationTypes=Basic, and are attached only to HTTPSpkgs.dev.azure.com/dnceng/...requests after revalidating service-index-derived URLs.Validation
pwsh -NoProfile -File .github/skills/release-readiness/tests/Test-ReleaseReadiness.ps1— 2150 passed, 0 failed.pwsh -NoProfile -File .github/skills/release-readiness/tests/Test-ReleaseReadiness.ps1 -SkipE2E— 2017 passed, 0 failed.UNKNOWN) from valid empty or package-index 404 probes (MISSING), while a service-index 404 remainsUNKNOWN.alias-toresolution for host-specific Android/Emscripten physical packages andany-RID Mono runtime packages; confirmed candidates with unavailable expected-pin evidence remainUNKNOWNwith targeted remediation.READY, fallback and nested sensitive-version redaction, SDK-pin-unresolved confirmation preservation, dnceng-only credential forwarding, explicit Basic-auth enforcement, query/fragment rejection, search URL construction, and Markdown escaping.Issues Fixed
N/A — release-readiness infrastructure improvement based on Preview installation validation.