Repository navigation
Add winapp-pr -Update, -Help, and build freshness in the picker - #702
Conversation
There was a problem hiding this comment.
Pull request overview
Adds -Refresh and avoids reinstalling an already installed build, alongside stacked GitHub CLI setup improvements.
Changes:
- Adds automatic GitHub CLI discovery, installation, and authentication.
- Adds
-Refreshusing recorded repository and branch state. - Skips identical runs unless
-Forceis specified.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
f17169f to
395534f
Compare
395534f to
7061aa6
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (4)
scripts/winapp-pr.ps1:937
Branchis not a unique PR identity because GitHub's.head.ref/head_branchomits the fork owner. If two PRs use the same branch name,Get-BranchRunscan return both and-Updatemay install the newest artifact from the wrong PR/fork. I reproduced an install recorded fromexpected/fork:featureselecting newer run 222 fromother/fork:feature. Persist and resolve the original PR number or head repository plus branch instead of the branch alone.
}
scripts/winapp-pr.ps1:981
- The no-op check ignores the requested architecture even though each run contains both x64 and arm64 packages. On ARM64 I reproduced an installed Arm64 package followed by
-Update -Arch x64exiting 0 with “Already on this build,” so the explicit architecture change is silently ignored. Require an installed package with the requested architecture before returning.
$runs = Get-CandidateRuns -RepoName $repoName -TargetSpec $spec
scripts/winapp-pr.ps1:891
- Authentication is checked before local-only work and validation. With an unauthenticated
gh, I reproduced both-PruneCertsfailing before pruning and-Updatewith no install record asking the user to sign in instead of reporting the missing record. Validate the update state first and deferAssert-GhReadyuntil after the local-PruneCertspath.
if ($state -and $state.Version -eq ($installed | Select-Object -First 1).Version) {
scripts/winapp-pr.ps1:453
- The marker still matches only the branch name, so separate PRs from different forks that both use (for example)
featureare both shown as installed/newer. I reproduced two such PRs both receiving^. Include the PR number or head repository in the stored and menu-item identity before assigning a marker.
This issue also appears on line 937 of the same file.
if ($newest -and $newest.Run.id -ne $state.RunId) { $installedIsCurrent = $false }
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (4)
scripts/winapp-pr.ps1:1013
- The no-op check ignores
-Arch, although each run contains both architecture packages. On ARM64, resolving the currently installed run with-Arch arm64while its x64 package is installed reports “Already on this build” and leaves x64 installed. Require the installed package architecture to match the requested architecture before returning early.
if (-not $Force -and $installState -and $installState.RunId -eq $selected.Run.id -and (Get-InstalledWinapp)) {
scripts/winapp-pr.ps1:923
- This authentication guard also runs for
-PruneCerts, which is entirely local. An expired or missing GitHub login now blocks certificate cleanup and prompts the user to sign in before the pruning branch is reached. Restrict the guard to operations that call GitHub.
Assert-GhReady
scripts/winapp-pr.ps1:452
- Repository scoping does not uniquely identify a PR from a fork:
Repois the base workflow repository, whileBranchis only.head.ref. Two open PRs from different head repositories can therefore receive the same installed marker, and freshness can be calculated from the other fork's run. Include the PR number or.head.repo.full_namein both menu and install-state identity.
if ($state -and $state.Branch -and $state.Repo -eq $RepoName) {
$installedBranch = $state.Branch
$newest = Get-MsixArtifact -RepoName $RepoName -Quiet `
-Runs (Get-BranchRuns -RepoName $RepoName -Branch $installedBranch)
if ($newest -and $newest.Run.id -ne $state.RunId) { $installedIsCurrent = $false }
scripts/winapp-pr.ps1:451
-Quietsuppresses output but does not limit requests:Get-MsixArtifactstill makes one artifact API call for every candidate run until it finds a usable artifact. SinceGet-BranchRunsreturns up to 50 runs, merely opening the picker can add up to 51 serial API calls rather than the documented two, causing substantial latency after several artifact-less builds. Fetch repository artifacts once and match them against the candidate run IDs.
$newest = Get-MsixArtifact -RepoName $RepoName -Quiet `
-Runs (Get-BranchRuns -RepoName $RepoName -Branch $installedBranch)
7061aa6 to
91ee1cf
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
scripts/winapp-pr.ps1:982
-Updatestill trusts a stale record when the package has been uninstalled because this guard only rejects mismatches when$installedPackageis non-null. In that state the command resolves and installs the recorded branch, despite the option promising to update what is currently installed. Reject every non-live state and give the no-package case its own message.
if ($installedPackage -and -not $stateIsLive) {
scripts/winapp-pr.ps1:976
- This liveness result is not applied by the later
-PruneCertspath, which reloads the record and keepsstate.Thumbprintunconditionally. After a manual replacement or uninstall, pruning reports the stale certificate as used and can remove the certificate for the actual package. Gate the retained thumbprint on$stateIsLive.
This issue also appears on line 982 of the same file.
$stateIsLive = Test-StateMatchesInstall -State $installState -Package $installedPackage
scripts/winapp-pr.ps1:182
- Replacing the process PATH with registry values drops session-only entries such as virtual-environment and tool-shim directories. Because the documented web invocation runs in the caller's PowerShell process, those commands remain unavailable after the GitHub CLI installation finishes. Preserve the existing process PATH while adding refreshed registry entries.
$env:Path = $parts -join ';'
91ee1cf to
a4babdd
Compare
a4babdd to
95e82fa
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (4)
scripts/winapp-pr.ps1:1029
-Updatereconstructs the source solely fromhead_branch, losing the PR/fork identity used for the original install. Two fork PRs can both use a branch such asfeature; their pull-request workflow runs live in the same base repo, so this branch query can select and install the other contributor's artifact. Persist and resolve the original PR number or head-repository identity instead of degrading it to a branch name.
if ($Run) {
Write-Step "Resolving build from $repoName"
$runDetail = Invoke-Gh @('api', "repos/$repoName/actions/runs/$Run")
scripts/winapp-pr.ps1:978
- Update mode still defaults the requested package to the host architecture rather than the architecture of the installed package. On an ARM64 machine with an intentionally installed x64 build,
winapp-pr -Updatesilently replaces it with ARM64 instead of updating what is installed. When-Archis omitted, derive the update architecture from the live package (or the recordedArch), while preserving an explicit-Archoverride.
Assert-GhReady
scripts/winapp-pr.ps1:469
- Repository scope does not disambiguate fork PR branches: multiple open PRs in the same base repository can have the same
.head.ref. In that case this code can mark the wrong PR as installed/newer, andGet-BranchRunscan resolve freshness from the wrong fork's run. Include the PR number or head repository in both the stored source identity and marker/freshness matching.
if ($liveState -and $state.Branch -and $state.Repo -eq $RepoName) {
$installedBranch = $state.Branch
$newest = Get-MsixArtifact -RepoName $RepoName -Quiet `
-Runs (Get-BranchRuns -RepoName $RepoName -Branch $installedBranch -HeadRepo $state.HeadRepo)
if ($newest -and $newest.Run.id -ne $state.RunId) { $installedIsCurrent = $false }
scripts/winapp-pr.ps1:990
-Updatetrusts a stale record when no winapp package is installed because this guard runs only when$installedPackageis non-null. A user who uninstalled winapp but still hascurrent.jsonwill therefore reinstall from that stale source, even though the record no longer describes a live package. Reject every-Updatefor which$stateIsLiveis false, with a separate no-package message.
$installState = Get-InstallState
| if (-not $State -or -not $Package) { return $false } | ||
| if ($State.Version -ne $Package.Version) { return $false } | ||
| # Records written before Arch was tracked can't be checked on it; version alone will do. | ||
| if ($State.Arch -and $State.Arch -ne (Get-InstalledArch -Package $Package)) { return $false } |
Picking up newer commits on the build you are running meant remembering which PR or branch it came from and naming it again, and re-resolving the same target silently uninstalled and reinstalled an identical package with no sign you were already current. -Update installs the newest build for whatever is installed. It resolves by the PR the build came from where there was one, because fork PRs run in the base repo and a branch name alone can match another fork's runs; where only a branch is known it pins to that build's head repository. Only an explicit -Repo overrides the recorded source, since WINAPP_PR_REPO is a default for new installs rather than a redirect for an existing one. Any resolution landing on the installed run now reports it and stops; -Force still reinstalls. Updating winapp-pr itself moves to -UpdateTool, since updating the build is the far more common intent. The install record is only trusted while it still describes the package that is actually installed, so a package installed by other means is not mistaken for a known build: -Status says the source is unknown, -Update asks for an explicit target, and the picker stops marking it. The record now tracks architecture too, so asking for a different -Arch is never mistaken for a no-op. The picker distinguishes the installed build being current from having a newer build waiting, and only claims a build is installed when it came from the repo being listed, since branch names collide across forks. Adds -Help. PowerShell's built-in -? covers a script on disk, but throws when the script is run from the web as a scriptblock, which is exactly how the documented installer runs it. Also carries two review fixes for #700, which merged before they were written. The gh readiness check is scoped to the active github.com account, because a bare 'gh auth status' fails when any configured account on any host is stale. And when an invalid GH_TOKEN or GITHUB_TOKEN is the cause, say so rather than offering a sign-in that gh will refuse while an env token is set. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: db5ddd3d-7017-4d4a-9b45-d7f8dc70deaf
95e82fa to
7f0e121
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
scripts/winapp-pr.ps1:928
- The install record is not uniquely tied to the live package. MSIX versions use the commit count since the last
version.jsonchange (scripts/get-build-number.ps1:38-57), so sibling PRs commonly produce the same version and architecture. If another same-version build is installed manually, this check still trusts the stale record;-Updatecan follow the wrong source and the no-op path can skip a needed install. Validate a run-unique package fingerprint (for example, a stamped run ID or the live signer thumbprint) before treating the state as current.
Assert-GhReady
scripts/winapp-pr.ps1:663
- The PR resolution path still falls back to unfiltered branch runs at line 670. When the PR head has no artifact yet, another fork's newer run for the same branch name can be selected and installed—the exact fork collision this change aims to prevent. Capture the PR head repository and apply it to the fallback query.
Where-Object { $_.Name -notlike "*_$Architecture.msix" } |
scripts/winapp-pr.ps1:490
- The marker is matched only by branch name, while the PR query drops each head repository. Two open PRs from different forks with the same
.head.reftherefore both display*or^, incorrectly claiming both are the installed build. Match recorded PR installs bystate.Pr/PR number, and branch installs by both branch and head repository.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
scripts/winapp-pr.ps1:587
- The new fork filter is only applied for branch targets. For a PR, the exact-head runs are safe, but the fallback at line 603 still queries every run with that branch name. If the PR head has no artifact, selecting that PR can therefore install an older artifact from another fork with the same branch name. Include
.head.repo.full_namein the PR metadata and pass it to the fallbackGet-BranchRunscall.
return Get-BranchRuns -RepoName $RepoName -Branch $TargetSpec -HeadRepo $HeadRepo
scripts/winapp-pr.ps1:938
- A stale record is still trusted when the package has been uninstalled because this guard only rejects mismatches when
$installedPackageexists. In that state,winapp-pr -Updateproceeds using the old source even though there is no installed build to update, contrary to the new record-validation contract. Reject a missing package before checking the record match.
if ($installedPackage -and -not $stateIsLive) {
scripts/winapp-pr.ps1:409
- The picker identifies an installed item only by branch name. Two open PRs from different forks can share that name, and a fork PR whose branch is
mainalso collides with the default-branch row, so multiple unrelated entries receive*or^. Carry each PR's number/head repository into the marker comparison, and do not mark the default-branch row for a recorded PR install.
if ($installedBranch -and $installedBranch -eq $Branch) {
return $(if ($installedIsCurrent) { '*' } else { '^' })
}
CI mints a new self-signed certificate per build, so every install left another trusted anchor behind -- 25 had accumulated before -PruneCerts existed, and 4 more within a day of running it. Nobody remembers to run a cleanup command. Installs now retire the certificates this tool previously trusted, in the same elevated call that imports the new one, so cleanup costs no extra prompt and the count stops growing. Nothing is retired when the new certificate is already trusted, since that path does not elevate at all. Cleanup is driven by a record of what we trusted rather than by matching on subject: TrustedPeople is a shared store that also holds unrelated anchors, and CN=runneradmin is not exclusive to this project. -PruneCerts keeps the broader subject-based sweep for certificates trusted before this change. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: db5ddd3d-7017-4d4a-9b45-d7f8dc70deaf
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (5)
scripts/winapp-pr.ps1:597
- For a PR target, exact-head runs are followed by branch runs that are not pinned to the PR's head repository. If the head run has no artifact and another fork has the same branch name,
Get-MsixArtifactcan install that fork's package—the collision this change is intended to prevent. Include the PR head repository in the API projection and use it for the fallback query.
$script:TargetPr = $TargetSpec
scripts/winapp-pr.ps1:914
- Version plus architecture does not uniquely identify a CI build: the MSIX build component is the commit count since
version.json, so sibling PRs/forks commonly share it. If another installer replaces run A with a same-version/same-arch package from run B, this returns true and the new no-op/status logic still claims run A is installed. Use verifiable per-run package identity, or do not authorize the no-op when the record cannot be attested against the installed package.
if ($State.Version -ne $Package.Version) { return $false }
# Records written before Arch was tracked can't be checked on it; version alone will do.
if ($State.Arch -and $State.Arch -ne (Get-InstalledArch -Package $Package)) { return $false }
scripts/winapp-pr.ps1:984
- When the package was manually uninstalled,
$installedPackageis null, so this condition skips the stale-record refusal and-Updateinstalls from a record that no longer describes anything installed. Reject every non-live state; give the no-package case its own message.
if ($installedPackage -and -not $stateIsLive) {
scripts/winapp-pr.ps1:424
- The picker compares only branch names, and the PR projection does not retain
.head.repo.full_name. Two open PRs from different forks with the same head ref therefore both receive*or^, even though freshness was resolved for only the recorded fork. Carry the head repository into each menu item and include it in the marker comparison.
Marker = Get-Marker -Branch $pr.branch
scripts/winapp-pr.ps1:888
- Splitting the here-string and writing each line to the pipeline drops empty-string entries, so all intended blank lines disappear; the actual output joins sections as
options]PICKING A BUILD,runCOMMANDS, and so on. Return the multiline string intact (it remains pipeable) or explicitly emit blank lines.
'@ -split "`r?`n"
Two review findings. Version plus architecture did not prove the installed package was the recorded build. Dev MSIX versions are a commit count, so unrelated branches at the same depth produce identical name/version/architecture and a manually installed package could be mistaken for a known one. Compare the installed package's signing certificate instead, which CI mints per run and is therefore unique. Reading it needs the 4-byte 'PKCX' header stripped before SignedCms will parse AppxSignature.p7x; without that the read failed silently and this identity was never available. Version and architecture remain a fallback where the signature cannot be read. Tracked thumbprints are interpolated into a command run with -Verb RunAs, and their file is user-writable, so a crafted value could append a second elevated statement. Validate them as 40 hex characters wherever they reach that command, and drop anything else on read. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: db5ddd3d-7017-4d4a-9b45-d7f8dc70deaf
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (3)
scripts/winapp-pr.ps1:1069
- Resolving an update by PR is still vulnerable to the fork collision this change is meant to prevent.
Get-CandidateRunsfetches the PR head SHA, but its fallback at line 604 callsGet-BranchRunswithout the PR's head repository. If the head run has no artifact, a newer run from another fork with the same branch name can be selected. A mockedmaincollision selected the unrelated fork's run. Include.head.repo.full_namein the PR metadata and use it to filter the fallback branch runs.
if ($installState.Pr) {
$spec = [string]$installState.Pr
Write-Detail "Updating the installed build: PR #$($installState.Pr)"
scripts/winapp-pr.ps1:409
- The marker is keyed only by branch name, so branch collisions mark unrelated entries as installed/current. For example, with two fork PRs both using
main, this helper marks both PRs and the base repository'smainentry with*. Match PR entries by the recorded PR number (or head repository plus branch), and match the default branch only when the installed head repository is the base repository.
function Get-Marker {
param([string]$Branch)
if ($installedBranch -and $installedBranch -eq $Branch) {
return $(if ($installedIsCurrent) { '*' } else { '^' })
scripts/winapp-pr.ps1:1026
-Updateaccepts a stale install record when no winapp package is installed because this guard runs only when$installedPackageis non-null. It then resolves and installs from that stale source even though the record does not describe a live package. Reject every non-live state, with a distinct message for the no-package case.
if ($installedPackage -and -not $stateIsLive) {
Build Metrics ReportBinary Sizes
Test Results❌ 3716 passed, 3 failed, 4 skipped out of 3723 tests in 565.6s (-26.7s vs. baseline) Test Coverage✅ 94.5% line coverage, 88.6% branch coverage · ✅ no change vs. baseline CLI Startup Time49ms median (x64, Updated 2026-07-30 23:02:05 UTC · commit |
Based directly on
main. Single commit, one file.Picking up newer commits on the build you are already running was more work than it should be. You had to remember which PR or branch it came from —
-Statuswould tell you, but that is an extra step — and name it again. Worse, re-resolving the same target silently uninstalled and reinstalled an identical package with no indication you were already current.Changes
-Updateinstalls the newest build for whatever is installed:It resolves by the PR the build came from where there was one. Fork PRs run in the base repo, so a branch name alone can match another fork's runs —
mainbeing the obvious case — and picking "the newest run on that branch" could hand you an unrelated contributor's build. Where only a branch is known, resolution is pinned to that build's head repository instead.Only an explicit
-Repooverrides the recorded source.WINAPP_PR_REPOis a default for new installs, not a redirect for an existing one; otherwise having it point at the public repo while running a private-fork build would send-Updatelooking for that branch in the wrong place.Updating the tool itself moves from
-Updateto-UpdateTool. Updating the build is by far the more common intent, so it should own the obvious name.Already-installed builds are no longer reinstalled. Any resolution that lands on the run you are already on stops early:
This applies to naming a target explicitly too, so
winapp-pr 681when you are on 681's newest build is a no-op instead of a ~10 second uninstall/reinstall cycle.The install record is only trusted while it still describes the installed package. A package installed by other means — double-clicking an MSIX,
setup-winapprun.ps1— used to leave the record describing a build that was no longer there, which would have let the no-op above skip a genuinely needed install. The record is now validated against the live package first:-Updaterefuses rather than guessing, and points at a concrete alternative:The record now tracks architecture as well, so requesting a different
-Archfor a run you already have is not mistaken for a no-op. Records written before this change lack the newer fields and fall back to the old behaviour, so nobody has to reinstall to get a working record.The picker shows build freshness, distinguishing "you have this" from "there is something newer":
Freshness is resolved the same way an install would resolve it — the newest run that actually has an artifact, pinned to the same head repository — so neither an in-progress build nor another fork's run produces a phantom
^. It costs two extra API calls, only for the one installed branch.-Helplists everything. PowerShell's built-in-?already covers the on-disk case, but it throws when the script is run from the web as a scriptblock:That is exactly how the documented installer runs it, so
-HelpusesGet-Helpwhen there is a file and falls back to the script's own comment-based help block otherwise.Also carries the #700 review fixes
Those review comments were written after #700 had already merged, so they never landed. They are folded in here rather than left on a dead branch:
--active --hostname github.com. A baregh auth statusfails when any configured account on any host is stale, so anyone with a second account would have been pushed into a pointless sign-in.GH_TOKENorGITHUB_TOKENis the cause, say so instead of offering to sign in.gh auth loginrefuses to store credentials while an env token is set, so that prompt could only ever dead-end.Validation
The fork-collision fix was reproduced directly: two
Build and Packageruns on a branch namedfeaturefrom different head repositories, the unrelated one newer. Unfiltered resolution selects the stranger's run; pinned to the head repository it selects the right one.-Updatewas checked withWINAPP_PR_REPOpointing at a different repo, confirming it stays on the recorded source; with a record carrying a PR number, confirming it resolves by PR; and with a legacy record lacking the new fields, confirming the branch path still works. A real install was run end to end to confirmPr,HeadRepo, andArchare persisted.Also exercised against real branch builds:
-Updatewhen current reports it and exits 0;-Update -Forceperforms a full reinstall;-Updatewith no record fails cleanly;-UpdateToolstill targets the tool. Record validation was covered as a table — matching version and architecture, mismatched architecture, legacy record, and a version mismatch standing in for a manual install — and end to end by pointing the record at a version that is not installed. Picker markers were checked in all three states, and-Helpboth from a file and through a scriptblock.