feat(acp-setup): install missing Node and uv toolchains from the wizard - #150
Merged
Conversation
The step-2 warning InfoBar stacked its three recovery actions inside InfoBar.Content, which the InfoBar template renders in a second row under the icon row with zero bottom padding. That produced the issue's symptoms: title and message crammed onto one line, a dead gap between text and buttons, and buttons flush against the bar's bottom border. InfoBar.ActionButton only accepts a single ButtonBase, so it cannot host the three actions either. Keep the InfoBar as a pure warning banner and render the three actions (Install adapter, Detect again, Set up toolchain) as a horizontal row below it, sharing the same IsAdapterMissing visibility so they appear and disappear together. Fixes #137 Co-Authored-By: Claude <[email protected]>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
The wizard could install components but not the toolchain that carries them, so a machine without Node was told to leave the app and come back. Describing an install as data is what lets one downloader serve vendors that agree on nothing. Verified against the live releases, no two archives share a shape: Node's POSIX tarball nests executables under bin/ while its Windows zip puts them at its root, uv's POSIX tarball puts them at its single root, and uv's Windows zip has no root directory at all. Node tags releases v24.20.0 and publishes one SHASUMS256.txt for every artifact; uv tags them 0.12.8 — the v prefix 404s — and publishes a .sha256 beside each asset. Each of those is declared rather than inferred, because guessing yields a directory with no executables in it instead of an error. Platform matrices are explicit lists, not cross products of independent mappings. Composing segments freely invents plausible names for builds that do not exist, and each would send a user to a 404 rather than to the honest answer that this platform has no automatic install. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Re-detection could not see a Node installed by its own vendor installer. The scanned layouts covered version managers and per-user directories but omitted where the official installers land: Node's Windows MSI installs into program files and a Linux distribution package installs into /usr/bin, and neither appeared anywhere in the list. On Windows this was the whole answer rather than a gap, because there is no profile-built PATH to capture there and the disk scan is the only widening available. A user who installed Node exactly as documented and pressed "detect again" was told it was still missing, with nothing they could do about it. Program files is a named root rather than a spelled path: the directory follows the system drive, is localized in some installations, and has a separate 32-bit sibling, so a hard-coded C:\Program Files would miss the machines this exists for. Toolchains this app installs are scanned through the same mechanism as any other, instead of a parallel lookup that would drift from it. Without those entries an install the wizard had just performed would be invisible to the detection that runs immediately afterwards — the same defect, reintroduced by the fix for it. Ordering is preserved: the vendor locations come last, because a version manager's directory is what the user's own shell resolves first and a system-wide install is typically the older one. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Downloads the official archive, verifies its published SHA-256, unpacks it into per-user application data, proves the result runs, and puts it on the user's PATH. Verification is fail-closed and not optional: the payload is executable code fetched over the network, so a digest that cannot be located is treated exactly like one that does not match — nothing is installed. Existence on disk is not evidence either, since an archive for the wrong architecture unpacks perfectly and fails at exec, so the install runs the binary once before claiming success. The archive is untrusted input even over HTTPS with a matching digest, because the digest attests to the bytes rather than to their safety. Every entry's destination is checked before anything is written, and extraction stages into a temporary directory that is moved into place, so an interrupted unpack cannot leave a half-populated version directory for the scan to offer as a candidate. Two archive hazards are handled that would otherwise pass review: links are recreated rather than extracted, because TarEntry.ExtractToFile refuses a link entry outright and Node's npm and npx are relative symlinks into lib/node_modules — without this the whole install fails on the two launchers the wizard exists to provide. Zip carries no Unix mode, so the execute bit is restored explicitly. PATH is per-user on every platform and never elevated. On Windows the existing value is read back from the user target rather than from this process, whose PATH is the user and machine values already merged — appending to that would copy the entire system PATH into the user's own, permanently and invisibly. Registration is idempotent because unbounded PATH growth eventually breaks process creation for the whole session, which is worse than the install failing. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
The shell capture is the only route to a version manager implemented as a shell function. nvm ships no executable at all — command -v nvm finds nothing, only nvm.sh exists on disk — so no amount of directory scanning reveals the node it activated. That capture needs an executable implementing the environment-printing mode, and only the CLI did. The CLI ships as its own package installed independently of the desktop app, so a user who installed only the app lost the capture entirely and every Node component reported as missing on a machine that had one. The running process answers the same protocol, so it is now the final fallback and the chain can no longer come up empty. An installed CLI still wins, being a small single-file executable that the capture starts inside an interactive login shell the user waits on. The protocol moves beside the capture that reads it, rather than being copied into a second writer. Its option name previously existed twice, held together by a test comparing the two constants — a check that they had not drifted yet rather than a structure in which they cannot. The desktop head answers this before initializing anything, including native prerequisites: those dlopen a rendering library, and an environment probe has no business needing graphics. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
…d it missing Turns the dead end into an action. The surface that said a toolchain was required now carries a button that installs it, with the vendor's documentation kept beside it for the platforms and toolchains that have no automatic path. The offer needs three separate facts to hold, and each rules out a different wrong button: the toolchain must actually be missing, this app must publish a source for that particular toolchain, and this platform must be able to run an installer. A toolchain with no source keeps documentation rather than a button that could only fail. Re-detection is invalidated between the install and the re-probe. A first toolchain install creates an entire version directory that did not exist when the detector cached its search paths, so asking again without invalidating reports "missing" moments after the installer succeeded. Late results are matched against the current selection before being applied, since the adapter picker can change while a download runs. PATH registration failing is reported without turning a working install into an error state: whether the toolchain is installed is the re-probe's answer, and a new terminal needing manual configuration is a smaller problem than a successful setup presented as a failure. The agent row raises an event rather than binding a command, matching the existing install action: the button lives in an ItemTemplate and acts on its own item. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
YoungSx
force-pushed
the
feat/acp-wizard-toolchain-install
branch
from
September 1, 2026 05:13
da0426a to
23f43bf
Compare
Build selection asked System.Runtime.InteropServices for the platform it was choosing for. That type answers about the host a process runs on, which is a different question: the resolvers select an archive for a target they are told about, and describing that target with a host-inspection type made the domain depend on the family it must not touch. Two closed enumerations replace it, covering only what the vendors publish and this app supports. The host is inspected in the desktop layer and mapped onto them, so an architecture no toolchain targets resolves to no download rather than to a nearest guess — a build for the wrong architecture unpacks cleanly and then fails to exec, which would surface as a corrupt download instead of as an unsupported platform. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Checking each entry's destination is not enough to keep an extraction inside its directory. An archive can declare a symbolic link and then a regular file at the same name: both names resolve inside the destination, and the file's write follows the link to wherever it points. A symlinked parent directory does the same with an entry name that looks innocent on its own. Hard links are resolved rather than written, so a link source outside the destination imports outside content into the install tree. Every link this extraction creates is now recorded, and no entry of any kind is written until each path component below the destination is known not to be one of them — a directory creation is redirected by a symlinked parent just as a file write is. Recording only this extraction's own links is sound because the destination is a fresh staging directory, so every link in it came from an earlier entry of the same archive. Hard-link sources are refused when they fall outside rather than resolved. Vendor archives are unaffected: Node's npm and npx are leaves that no later entry names as a path component, and their targets are still stored verbatim so the launchers keep the module resolution a copy would break. The guard is proved by three archives no filesystem could hold, hand-assembled through TarWriter because creating them on disk would already perform the escape. Removing the check turns the two symlink tests red, and each asserts the victim outside the destination was left untouched rather than only that something was thrown. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #143.
The wizard could install ACP components but not the toolchain that carries them. A machine
without Node was shown a documentation link and told to leave the app — and the step most
likely to be got wrong, installing somewhere the app cannot see, was entirely the user's to
get right. Separately, a Node installed by its own vendor installer stayed invisible to
re-detection, so a user who did exactly what the wizard asked was told it was still missing.
Both are fixed here, for Node and uv.
What changed
its published SHA-256, unpack into per-user application data, run the binary to prove it
works, then add it to the user's PATH. Unprivileged on every platform.
package, a version manager, or the wizard itself.
route to nvm: it ships no executable, so nothing on disk reveals the node it activated.
destination is not enough: an archive can declare a symlink and then a regular file at the
same name, or a symlinked parent directory and an innocent-looking entry beneath it, and the
write follows the link outside the destination. Every link this extraction creates is now
recorded and no entry of any kind is written through one. Hard-link sources outside the
destination are refused rather than resolved.
for a target they are told about, so the target is named by two closed domain enumerations
instead of
System.Runtime.InteropServices; the host is inspected in the desktop layer andmapped onto them, and an architecture no toolchain targets yields no download rather than a
nearest guess.
Verified
Facts below were measured, not assumed. No two archives share a shape, so each is declared as
data: Node's POSIX tarball nests executables under
bin/while its Windows zip puts them atits root; uv's POSIX tarball puts them at its single root and its Windows zip has no root
directory at all. Node tags releases
v24.20.0with one sharedSHASUMS256.txt; uv tags them0.12.8— thevprefix 404s — with a.sha256beside each asset.Real end-to-end installs on Linux arm64, into an isolated
HOMEand app-data root:All 32 generated download URLs resolve (Node 6 + uv 13 archives, plus uv's 13 digests);
unsupported platform/architecture pairs return null rather than a 404 link. Installer refusal
paths were driven against a local server: checksum mismatch, absent digest, missing launcher,
non-runnable binary, and archive path traversal are each refused, with nothing written.
Reverse-verified — each of these was made to fail on purpose: removing the invalidation,
tampering with the expected digest, dropping
/usr/binfrom the scanned layouts, dropping theuv layout entry, reintroducing uv's
vtag prefix, and removing the link-traversal guard(which turns both symlink-escape tests red).
The link guard is proved by three archives no filesystem could hold, hand-assembled through
TarWriterbecause creating them on disk would already perform the escape: a symlink plus afile at the same name, a symlinked directory plus a file beneath it, and a hard link whose
source is outside the destination. Each asserts the victim outside the destination was left
untouched, not merely that something was thrown.
Tests after the review fixes: Domain 125, Infrastructure 732 (6 platform-gated skips),
Presentation.Core 3274 — all green, 0 failures. The desktop head builds with 0 errors and the
touched projects build with 0 warnings.
Notes for review
Targets
main, so the series carries one commit that was already ondevelop.fix(acp-setup-wizard): move adapter recovery actions out of the InfoBaris the single commitmainwas behind by, and it belongs to the same wizard surface.mainhas nothingdeveloplacks, so the branch fast-forwards and no merge commit is introduced.
uv toolchain only. The catalog has no
Uvxcomponent yet, and the uv-distributed ACPpackages on PyPI are community third-party rather than vendor-official, unlike everything the
catalog currently lists. Adding a catalog entry later needs no change to this install path.
SalmonEgg.slnfull build fails locally on thewasm-toolsworkload being absent. Thatis environmental and unrelated to this change; the desktop, CLI and all library targets build.
GUI smoke was not completed. The app launches and reaches ACP/telemetry init, but the
sandbox Xvfb has no window manager, so no visible window could be captured. The install
action is covered by XAML compliance tests plus the runtime checks above rather than by a
screenshot.
🤖 Generated with Claude Code