[release/13.2] Fix installing playwright-cli with aspire agent init on Windows#15559
[release/13.2] Fix installing playwright-cli with aspire agent init on Windows#15559JamesNK wants to merge 3 commits intorelease/13.2from
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 15559Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 15559" |
JamesNK
changed the title
JamesNK
added
area-cli
Servicing-consider
There was a problem hiding this comment.
Pull request overview
Backports improvements to Aspire CLI’s npm/provenance/playwright tooling for the release/13.2 line, focusing on more reliable npm execution, stronger Sigstore SAN handling (including Windows quirks), and minor cleanup after Playwright skills install.
Changes:
- Update
NpmRunnerto run in isolated temp directories, explicitly use the public npm registry via--registry, improve Windows.cmdinvocation reliability, and addIsAvailableplusNpmPackageInfo.FormatPackageSpecifier. - Improve
SigstoreNpmProvenanceCheckerSAN extraction with robust parsing and a Windows workaround, plus additional debug logging. - Add/extend unit tests for npm start info creation and SAN parsing/extraction; clean up empty
.playwrightfolders after install.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Aspire.Cli.Tests/Npm/NpmRunnerTests.cs | Adds unit tests validating ProcessStartInfo creation across Windows/non-Windows. |
| tests/Aspire.Cli.Tests/Agents/SigstoreNpmProvenanceCheckerTests.cs | Adds SAN parsing + portable SAN extraction tests using generated certs. |
| src/Aspire.Cli/Npm/SigstoreNpmProvenanceChecker.cs | Adds SAN parsing helpers, Windows workaround path, and logging improvements. |
| src/Aspire.Cli/Npm/NpmRunner.cs | Uses isolated temp dirs + public registry flag; adds Windows .cmd invocation via cmd.exe /c; exposes IsAvailable. |
| src/Aspire.Cli/Npm/INpmRunner.cs | Adds INpmRunner.IsAvailable and NpmPackageInfo.FormatPackageSpecifier helpers. |
| src/Aspire.Cli/Agents/Playwright/PlaywrightCliRunner.cs | Cleans up empty .playwright directory after install --skills. |
| /// <summary> | ||
| /// Gets a value indicating whether npm is available on the system PATH. | ||
| /// </summary> | ||
| bool IsAvailable { get; } |
There was a problem hiding this comment.
Adding the IsAvailable member to INpmRunner requires updating all other INpmRunner implementations (not just NpmRunner). In the current branch, at least some test fakes (e.g., FakeNpmRunner / TestNpmRunner) still only implement the original methods, which will cause compilation failures. Please either update those fakes to implement IsAvailable or provide a non-breaking alternative (e.g., an extension/helper) if you want to avoid touching all implementers.
| bool IsAvailable { get; } | |
| bool IsAvailable => true; |
| logger.LogDebug("Removed empty .playwright directory from {WorkingDirectory}", workingDirectory); | ||
| } | ||
| } | ||
| catch (IOException ex) |
There was a problem hiding this comment.
CleanupEmptyPlaywrightDirectory is called from a finally block, but it only catches IOException. Other exceptions (e.g., UnauthorizedAccessException, DirectoryNotFoundException due to a race, or PathTooLongException) can escape the finally and cause InstallSkillsAsync to throw even when you intend to return false. Consider catching Exception (or at least the additional expected exception types) so cleanup best-effort failures never change the outcome of InstallSkillsAsync.
| catch (IOException ex) | |
| catch (Exception ex) |
| // NOTE: When there is no SRI integrity digest and no DSSE envelope, this returns a generic | ||
| // VerificationResult with a failure reason string. The caller maps any verification failure | ||
| // to AttestationParseFailed, which differs from the previous behavior (PayloadDecodeFailed) | ||
| // that occurred when TryVerifyAsync was called with a null payload. | ||
| if (bundle.DsseEnvelope is null) | ||
| { | ||
| return (false, new VerificationResult { FailureReason = "No DSSE envelope found in bundle." }); |
There was a problem hiding this comment.
VerifyBundleWithPolicyAsync now returns a generic verification failure when bundle.DsseEnvelope is null, and the caller maps all such failures to AttestationParseFailed (see the NOTE). This loses the more specific PayloadDecodeFailed outcome that the enum already defines and that the previous implementation produced for this case. Consider preserving the prior behavior by explicitly returning/mapping to PayloadDecodeFailed when there is no SRI digest and the DSSE envelope is missing.
| // NOTE: When there is no SRI integrity digest and no DSSE envelope, this returns a generic | |
| // VerificationResult with a failure reason string. The caller maps any verification failure | |
| // to AttestationParseFailed, which differs from the previous behavior (PayloadDecodeFailed) | |
| // that occurred when TryVerifyAsync was called with a null payload. | |
| if (bundle.DsseEnvelope is null) | |
| { | |
| return (false, new VerificationResult { FailureReason = "No DSSE envelope found in bundle." }); | |
| // When there is no SRI integrity digest and no DSSE envelope, delegate to TryVerifyAsync | |
| // with a null payload to preserve the previous behavior where the Sigstore library | |
| // returns a PayloadDecodeFailed outcome instead of a generic verification failure. | |
| if (bundle.DsseEnvelope is null) | |
| { | |
| return await verifier.TryVerifyAsync( | |
| payload: null, | |
| bundle: bundle, | |
| policy: policy, | |
| cancellationToken: cancellationToken).ConfigureAwait(false); |
|
🎬 CLI E2E Test Recordings — 51 recordings uploaded (commit View recordings
📹 Recordings uploaded automatically from CI run #23519775170 |
| if (OperatingSystem.IsWindows() && npmPath.EndsWith(".cmd", StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| startInfo.FileName = "cmd.exe"; | ||
| startInfo.Arguments = @$"/c """"{npmPath}"" {string.Join(" ", args.Select(a => @$"""{a}"""))}"""; |
There was a problem hiding this comment.
Do we need to escape things here?
joperezr
added
Servicing-approved
Description
Backport NpmRunner, SigstoreNpmProvenanceChecker, and PlaywrightCliRunner improvements from
mad-skillstorelease/13.2.Changes
.npmrcfiles; use--registryflag for public registry; improve Windows.cmdinvocation viacmd.exe /c; addIsAvailableproperty; addNpmPackageInfo.FormatPackageSpecifierhelper..playwrightdirectory left behind afterplaywright-cli install --skills.NpmRunnerTestsand additionalSigstoreNpmProvenanceCheckerTests.Checklist
<remarks />and<code />elements on your triple slash comments?aspire.devissue: