Resolve node's real winget package id instead of assuming LTS (#693) - #696
Conversation
winget list --id OpenJS.NodeJS.LTS --exact echoes back the queried id for a package that only shares a publisher and name with what's actually installed. On a host running OpenJS.NodeJS (Current channel), that made every report and action believe OpenJS.NodeJS.LTS was installed, and -Upgrade/-Reinstall would have targeted that wrong id. Node ships under many ids beyond Current and LTS (one per pinned major back to 4), any of which is fine as long as its version clears the LTS floor. Add a Family field to the tools registry (empty for every tool but node) and a Resolve-ToolPackage that cross-references the already-correct installed version against a winget search over the family to find which id it actually belongs to, without trusting winget's id-scoped correlation at all. That resolved id now flows through status, notes, report, and every install/upgrade/remove action. Verified on a host with OpenJS.NodeJS (Current, 26.7.0) installed: report now shows SOURCE=OpenJS.NodeJS instead of the misleading OpenJS.NodeJS.LTS, and -Reinstall -DryRun now targets OpenJS.NodeJS instead of silently swapping to LTS. Other tools are unaffected. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes incorrect winget package-id resolution for Node.js on Windows by resolving the actual installed Node.js winget id (Current/LTS/pinned-major) instead of assuming OpenJS.NodeJS.LTS, preventing reports and actions from targeting the wrong package.
Changes:
- Added a
Familyfield to the tool registry (used only fornode) to represent the shared id prefix. - Added
Read-WingetSearchTableandResolve-ToolPackageto map an installed Node.js version back to the correct sibling winget id via a family search. - Plumbed the resolved package id through reporting, notes, and install/upgrade/remove/reinstall actions.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…rsion match Select-Object -First 1 picked arbitrarily between sibling ids that happen to share the same version, which can occur right at a channel handoff. Require an unambiguous match before trusting it; otherwise fall back to the tool's own default package, same as no match at all. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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.
Suppressed comments (1)
host-setup/windows/install-tools.ps1:584
- Actions now target
$state.Package(resolved installed id), butAvailable/floor is still read from$Tool.Package(Node LTS). If the installed Node id resolves to a pinned-major package whose major is below the LTS floor (e.g.,OpenJS.NodeJS.23while LTS is 24.x),-Upgrade/-Reinstallwill keep upgrading/reinstalling that pinned id even though it can never reach the required major, so the tool stays permanentlyoutdated.
Consider separating “installed package id” from “target package id”, or overriding the action target back to $record.Package when the installed major is below the required major (and updating the reinstall prompt accordingly).
$packages = @($state.Package)
if ($script:WITH_OPTIONAL) { $packages += $record.Optional }
foreach ($package in $packages) {
$code = if ($state.Rows.Count -gt 0 -and $script:MODE -eq 'upgrade' -and $package -eq $state.Package) {
Invoke-WingetUpgrade -Id $package
…oomed retry A node install resolved to an id outside the tool's own default (a pinned major, frozen forever below the LTS floor) previously kept being handed to `winget upgrade --id <that id> --exact`, which can never succeed under that id, so -Upgrade would fail on it every run with no path forward. -Reinstall carried the same problem: it removed and reinstalled the identical capped copy. Introduce $target, the id fresh work should land on. It equals the resolved installed id except when that id is outdated and not the tool's own default, where it falls back to the default (the only id capable of actually clearing the floor). -Upgrade now says plainly that the installed id is a fixed release and points at -Reinstall; -Reinstall now installs $target after removing the old copy, so it genuinely fixes a capped install instead of restoring it unchanged. A healthy alternate channel (e.g. Current, already above the floor) is untouched by this, since $target only diverges from the installed id when the tool is outdated under it. Also fixes a real bug caught while testing this: a bare `$var?` inside a double-quoted string is not `$var` followed by literal `?` in PowerShell, `?` is itself a valid interpolated-name character, so it silently referenced a nonexistent `$again?` variable. Parenthesized to `$($again)?`. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Re the suppressed finding on
Confirmed and fixed in fee7c3a. Added Verified with synthetic state (not a real winget mutation, to avoid installing a legacy Node major on a live host): |
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.
Suppressed comments (1)
host-setup/windows/install-tools.ps1:578
- The new pinned-major detection (
$targetdiffers from$state.Package) is handled for-Upgrade(you log that the id is fixed and require-Reinstall), but-Installon the same state still logs that "-Upgrade moves it". For a pinned major this is inaccurate, since-Upgradewill hit the same fixed-release guard and no-op; the guidance should be consistent and point to-Reinstallwhen$target -ne $state.Packagein both modes.
} elseif ($script:MODE -eq 'upgrade' -and $state.Status -eq 'outdated' -and $target -ne $state.Package) {
log "${ToolName}: $($state.Package) at $($state.Installed) is a fixed release and cannot advance under that id, -Reinstall $ToolName replaces it with $target"
return
} elseif ($script:MODE -eq 'install' -and $state.Status -eq 'outdated') {
log "${ToolName}: at $($state.Installed), the source carries $($state.Available), -Upgrade moves it"
return
…d id -Install on a pinned-major node install still said "-Upgrade moves it", but -Upgrade hits the same fixed-release guard and no-ops, so that pointed the operator at a dead end. Both modes now share the one branch and point at -Reinstall. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Re the suppressed finding on
Confirmed and fixed in eae196a — merged the two branches so |
…Python CI Gates (#718) Thirty-one squashes, `56f4d7d..d54862a`. 115 files, +20436/-5298. **Merge with a merge commit, never a squash, and never with `--delete-branch`.** This pull request's head is `develop` itself. ## What lands **Fleet Skills.** The `.agents/skills/` source tree, the generated `.claude-plugin/` distribution, `scripts/build_dist.py` with its `--check` gate, and `scripts/skills_install.py` with its host stamp (#676). Packaged as skills on top of the scaffold: PR review conduct and Copilot instructions upkeep (#677), comment and doc style (#678), resync-a-repo and fleet-conformance-check (#679), the per-language codestyles (#680), git commit conventions and operational vs release workflow (#681), stand up a repo (#683), and repo-worktree (#717). Coverage gaps closed in three passes (#690, #691, #692) plus the P4 sentence-length opt-in (#697). **Host setup.** The Windows host-setup tooling and its PowerShell gate (#674), the Windows bootstrap loader (#682), Docker install and upgrade on Linux and Windows with a version floor (#701, #705), a `uv` floor in `spec/host-tools.json` (#698), self-healing of a shadowing `uv`, `jq`, or `git-restore-mtime` copy (#689), node's real winget package id (#696), and a README for the Linux host-setup nuances (#710). **Python and CI.** Python tooling in CI with the script tests moved to `scripts/tests` (#704), `ruff format` adopted and gated (#709), and the PSScriptAnalyzer claim conditioned on repos that carry `.ps1` files (#686). **Conduct rules.** Triage-order and scope guardrails in pr-review-conduct (#684), `pr_review.py wait` requesting a review rather than only polling for one (#685), a tech-agnostic signed-commit verification (#708), execution rather than analogy to verify platform-specific code (#715), and a unique worktree for every task (#717). **Docs.** The fleet map and gap register with peer messaging declared (#687), mermaid flow diagrams in the kept-authority docs (#702), and the map pointed at the shipped diagrams and current tooling (#703). ## Issues this promotion closes Each landed on `develop` on its own pull request. The keyword fires only on a merge into `main`, so it sits here rather than on the feature pull requests. Closes #700 Closes #707 Closes #711 Closes #712 Closes #714 Closes #688 #699 stays open on purpose: #717 shipped the layout convention and the skill, and the physical migration of existing checkouts is still tracked there. ## Review record Every squash closed its own Copilot loop on its own pull request before merging to `develop`. This promotion carries no new content of its own, so its review is the merged tree as a whole. ## Consequence worth stating The `GOVERNANCE.md` and `AGENTS.md` sections these squashes changed become the canonical the moment this reaches `main`, and every carrying repository reads as drifted from that point until it resyncs. That is the ordinary consequence of a canonical moving rather than a defect. The Skills installer added here is also how a machine picks the new skills up, so a session that keeps restating a rule already packaged as a skill is the signal to run it.
Summary
Fixes #693.
winget list --id OpenJS.NodeJS.LTS --exactechoes back the queried id for a package that only shares a publisher and name with what's actually installed. On a host runningOpenJS.NodeJS(the Current channel), that made every report and action believeOpenJS.NodeJS.LTSwas installed, and-Upgrade/-Reinstallwould have targeted that wrong id.Node also ships under many more ids than Current/LTS — one per pinned major back to 4 (
OpenJS.NodeJS.4….23) — any of which is fine on a host as long as its version clears the LTS floor.Fix
Familyfield to the tools registry ininstall-tools.ps1(empty for every tool butnode, which gets'OpenJS.NodeJS').Resolve-ToolPackage, which cross-references the already-correctly-read installed version against awinget search --query <family>catalog listing. A version that uniquely matches one of the sibling ids identifies which one is truly installed, without ever trusting winget's id-scopedlist --id --exactcorrelation.$state.Package) now flows through status, notes, the report's SOURCE column, and every install/upgrade/remove action instead of the hardcoded default.Family = ''short-circuits before any extra winget call).Verification (done on a live host)
Host had
OpenJS.NodeJS(Current, 26.7.0) installed, notOpenJS.NodeJS.LTS.-Report node→ now showsSOURCE=OpenJS.NodeJS(was misleadinglyOpenJS.NodeJS.LTS), status still correctlycurrent(26.7.0 ≥ LTS floor 24.19.0).-Upgrade node -DryRun→ correctly no-ops (already current).-Reinstall node -DryRun→ now issueswinget uninstall --id OpenJS.NodeJS/winget install --id OpenJS.NodeJS, i.e. it reinstalls the package that's actually there instead of silently swapping to LTS.-Reportacross all 8 tools → other 7 unchanged, no regressions.Write-Hostwarnings.🤖 Generated with Claude Code