release: 0.0.39 — Windows runs the Linux build in WSL, no installer to sign - #59
Merged
Conversation
Windows is moving off Electron/Squirrel to run the ordinary Linux build inside WSL 2, with the browser as the GUI. The one thing the Electron app was doing that WSL cannot do for itself is stay alive: WSL tears down an idle distro roughly 15 seconds after its last session closes, killing the server even though 1helm.service is enabled and would auto-start on boot. This adds a per-user Scheduled Task and a two-layer supervisor. On the Windows side keepalive-run.ps1 holds exactly one wsl.exe anchor session, respawns it if it dies, and periodically confirms both that the distro is running and that the HTTP endpoint answers. Inside the distro keepalive-hold.sh exists to pin the distro up and restarts the unit if it stops. It runs as the signed-in user with an interactive token, never as Local System, because WSL state is user-scoped - the same invariant the rest of the Windows support already honours. Crash recovery comes from a one-minute repeating trigger with IgnoreNew rather than RestartOnFailure, and a per-session mutex enforces a single holder. Two Windows-specific traps are handled explicitly. wsl.exe does not strip quotes from its own option values, so a quoted distro name fails with WSL_E_DISTRO_NOT_FOUND; arguments are now quoted only when whitespace requires it. wsl.exe also needs valid standard handles or it exits instantly with no output, so all three anchor streams are redirected to files - which is both the fix and how the quoting bug was found. Every terminate is targeted at one distro and refuses any name matching -runtime$; `wsl --shutdown` is never invoked, as it would kill every distro for every user on the machine. Verified on a live distro: holds indefinitely with no visible window, recovers from killing the holder with zero HTTP downtime, refuses to stack a second supervisor, removes cleanly, and costs 0.01% CPU. Start-at-logon and survive-reboot are NOT yet proven - the acceptance host's autologon credential is invalid and its Task Scheduler cannot create any process, so both are pending a rebuilt VM. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… starts Validation on a healthy Windows 11 host proved both outstanding requirements - start at logon (5/5, isolated from the crash-recovery trigger) and survive a real reboot (4/4, reachable 89-104s after boot) - and surfaced one dangerous defect plus one timing risk. A non-elevated re-install reported success while doing nothing. When the task folder was created by an elevated process, Register-ScheduledTask fails with Access denied (0x80070005), but because the cmdlet is CIM-backed $ErrorActionPreference = 'Stop' does not make it terminating, so execution continued, printed "Registered scheduled task", inferred success from port 8123 answering - which the stale task satisfied - and exited 0. A failed upgrade was indistinguishable from a clean one. Registration now passes -ErrorAction Stop, is confirmed by reading the task back, and explains the elevation mismatch when it is the cause. The supervisor also restarted 1helm.service while systemd was still starting it. It now checks for the "activating" state and waits instead, and the budget before a distro recycle goes from 180s to 360s: measured cold starts were 33-80s on an 8-core box, which leaves too little margin on modest hardware. Defaults move off the spike names to the shipping distro '1helm' and C:\1helm\keepalive. All three scripts re-parsed clean with the PowerShell parser on Windows. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The Linux host installer built the release on the end user's machine: a full `npm ci` (devDependencies included) plus `npm run build`, which is why the install path needed build-essential and npm registry access, and why node-pty compiled from source on every install. Package that work into the artifact instead. `npm run package:linux` now runs the client/sidecar build on the release builder and injects the gitignored outputs (public/bundle.js, public/bundle.css, public/app.css, the stamped public/index.html, public/excalidraw, and the photon sidecar bundle) into the staging tree, the same way the sealed channel image is already injected. Production dependencies are installed once inside docker.io/library/node:22 with --network=host, so the vendored native addons link against Debian bookworm's glibc 2.36 and stay forward compatible with every supported target. Every shipped `build/Release/*.node` is fingerprinted into resources/linux-native-modules.json with its digest and its maximum GLIBC/GLIBCXX symbol, alongside the builder's architecture and Node ABI, so an installer can refuse a release whose native addons cannot load. The archive digest is written to dist/<archive>.sha256 for pinning. All existing release guards are untouched: packaging still refuses to run outside the exact Git checkout whose HEAD version matches package.json, still requires the sealed channel image, and still verifies the staged package version. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The packaging change in 23e852c made the Linux artifact ship its own production node_modules, prebuilt client assets, and a native-addon manifest, but the tracked user-facing scripts still ran `npm ci` and `npm run build` on the end user's machine, so no user benefited. install.sh, update-host.sh, and apply-linux-release.sh now consume that archive instead of building it: - No compiler is probed or installed. build-essential leaves the apt list and make/c++ leave the dependency probe, because nothing on the user's machine compiles 1Helm any more. python3/python3-venv stay. - Every staged or retained release must prove it is runnable before it is promoted: vendored node_modules, the shipped manifest, built client assets, and a dlopen of every listed addon against the installed Node's exact ABI, including a node-pty that still exposes spawn(). A source-only archive is refused with an actionable message rather than silently falling back to a build the host can no longer do. - The verification always runs outside the armed transaction window, so a release that cannot run leaves the host untouched instead of half-promoted; apply-linux-release.sh reports the refusal as an error status rather than leaving the UI on "installing". - Readiness now requires 1helm.service to be `active`, not merely that something answered on 8123, and prints the unit journal on failure. - Installs refuse to start behind a foreign listener on 8123. WSL 2 puts every distribution in one shared network namespace, so a listener in another distribution or on Windows itself would otherwise answer the readiness probe while 1helm.service crash-looped on EADDRINUSE. The probe binds the port the way the service will instead of parsing `ss`, which is not guaranteed to be installed and would skip in silence. Measured cold on WSL2: 3m39.9s versus 8m49s, with no compiler present. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Windows no longer ships an application. `irm https://1helm.com/install.ps1 | iex` enables WSL 2, imports a digest-pinned Ubuntu root filesystem, runs the ordinary Linux installer inside it, and registers the keepalive. The browser is the interface at http://localhost:8123. Nothing is code-signed because nothing is an executable, so SmartScreen never appears - which matters because 1Helm has no Authenticode identity and was never going to obtain one. Only enabling the Windows optional features and installing Microsoft's WSL package cross the UAC boundary. Importing the distribution, running the Linux installer and registering the keepalive all stay in the signed-in user's session on purpose: WSL state is per-user, so a distribution imported by an elevated session started with over-the-shoulder credentials would belong to that administrator rather than the person at the machine. The restart Windows requires to activate those features is reported as a restart with numbered steps, never as a failure, and every step is idempotent so re-running the same command continues where it stopped. `-Wait` on an elevated ShellExecute is unreliable, so the parent blocks on the real process handle before reading the exit code. Microsoft's WSL package is checked against a pinned digest AND required to carry a valid Microsoft Corporation Authenticode signature; a digest alone only proves we fetched what we expected, not who built it. A port pre-flight refuses to continue when something already holds 8123. Every WSL distribution shares one network namespace with Windows, so a foreign listener would both prevent 1Helm binding and answer its health probe in its place. The keepalive payload moves to site/public/keepalive/ and is fetched from the same origin as the installer, so the two can never be mismatched. Obtaining it is fail-closed: without the keepalive, WSL tears the distribution down when idle and 1Helm silently stops answering, so a missing component aborts the install rather than warning. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The first end-to-end run of install.ps1 on a pristine Windows 11 machine failed at the point where it should have succeeded, and three separate defects were behind it. Invoke-InDistro returned wsl.exe's standard output as well as its exit code. A PowerShell function returns everything it emits, so `$code = Invoke-InDistro ...` collected every line the in-distro command printed and the exit code into one array; `$code -ne 0` is then an array filter that is truthy for any non-empty output. The Linux install completed perfectly and the installer still stopped with the entire apt and install transcript interpolated into "the Linux installer failed (exit ...)". Because the long install step always prints something, this made success unreachable rather than merely unlikely. wsl.exe's output now goes to the host, and only the exit code is returned. install.sh had no local-archive path at all: it always resolved the published release from 1helm.com and downloaded it from GitHub, ignoring the argument install.ps1 was already passing it. -LocalArchive therefore silently installed something other than the archive it was given. It now accepts an archive path, takes the version from that archive's own package.json, and verifies the digest when HELM_RELEASE_SHA256 is pinned (install.ps1 forwards it via the new -LocalArchiveSha256). Resolving the published release stays the default and is unchanged. The Start Menu entry was a .lnk whose TargetPath was set to a URL. WScript.Shell accepts that and saves a shortcut with an empty target, so the entry appeared and did nothing. It is now an Internet Shortcut. Also stop the two readiness loops printing curl's connection errors on every attempt; "connection refused" is the expected state while the unit comes up, and it buried the real outcome under hundreds of lines. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Get-WindowsOptionalFeature -Online requires elevation, and the main pass
runs as the signed-in user on purpose. So the very first thing the
installer did was something it was not permitted to do:
Get-WindowsOptionalFeature : The requested operation requires elevation.
At install.ps1:113
The main pass now uses Test-WslReady, which establishes the same fact from
signals any user can read: Microsoft's pinned WSL responds, the vmcompute
service exists - it does not until VirtualMachinePlatform is active - and
Windows has no servicing restart pending. Feature enumeration is left to
the elevated host-setup pass, where it is both permitted and
authoritative, and Test-RestartPending only attempts it when elevated.
This was invisible from an SSH session: Windows OpenSSH grants an
administrator a full elevated token, so DISM succeeded during automated
testing and failed immediately for a real user in an ordinary PowerShell
window.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Windows publishes no release artifacts now: it installs the Linux archive inside WSL via https://1helm.com/install.ps1. Three places still encoded the old six-file matrix, and each would have failed the day 0.0.39 was published rather than at merge time. latestLinuxRelease() threw unless a Setup executable, full .nupkg and RELEASES were all present with digests. It backs /api/releases/linux/latest, which install.sh resolves to find the archive - so publishing a release without those files would have broken the public Linux AND Windows installers simultaneously, with a "does not contain the complete digest-qualified desktop matrix" error pointing at the wrong thing. It now expects the three artifacts that are actually shipped. RELEASE_FALLBACK advertised the same six files, so an API outage would have handed visitors download links for a Setup executable that does not exist. /download/windows redirected to that executable; it now leads to the install instructions. The Linux installer's port-collision message explained Windows and WSL sharing one network namespace regardless of platform, which reads as nonsense to an operator on a native Debian host. It is now conditional on systemd-detect-virt reporting wsl. test/site.mjs asserted the old contract in two places and is updated to the new one, including explicit assertions that the fallback does NOT advertise a Setup executable, .nupkg or RELEASES. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Windows no longer ships an application: the ordinary Linux build runs inside a WSL 2 distribution named "1helm" and the browser is the interface at http://localhost:8123. Nothing is code-signed because nothing is shipped, so SmartScreen never appears. The documentation still described a Setup executable, a Squirrel feed, an Authenticode disclosure and %APPDATA% data roots, none of which exist. Add site/public/uninstall.ps1, the only supported way off Windows. In order: stop the keepalive (via its own keepalive-remove.ps1 when present, otherwise unregistering \1Helm\ directly), run /opt/1helm/uninstall-host.sh inside the distribution so 1Helm removes its own ownership-checked containers and units while it still can, then terminate and unregister the distribution, then delete C:\1helm and the Start Menu entry. Because `wsl --unregister` deletes the virtual disk holding every channel's files, the database and the provider credentials, it requires typed confirmation stating exactly what is lost; -Force exists for scripted use. It refuses to run as SYSTEM, whose per-user WSL state is not the user's. It never calls `wsl --shutdown`, which would stop every distribution for every user on the machine, and both the targeted terminate and the unregister sit behind the keepalive's protective pattern plus an exact, case-sensitive match against a currently registered distribution. Every step is idempotent and the run ends by reporting what was removed and what was not. Documentation now matches the verified flow at the altitude a first-time user needs: one command, one permission pop-up, a deliberate "Restart required" stop, the same command again, then the browser. It also names the things that look like failures and are not - Microsoft's own "Welcome to WSL" window, the ~40s the channel-computer runtime needs after install, the ExecutionPolicy wall that only appears when the script is downloaded rather than piped - and records that #main's terminal is now bash inside WSL, not cmd.exe. test/site.mjs asserted the retired contract: that getting-started tells Windows users to download a Setup executable and discloses `NotSigned`. Both now assert the opposite, because SmartScreen and Authenticode cannot apply to a product that ships no .exe and repeating them would send people looking for a file that does not exist. New assertions cover /uninstall.ps1 being served, the Windows guide carrying the whole two-phase flow, and the uninstaller's safety guards. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The site has been deployed by hand: git archive a commit into /opt/1helm-site/releases/<sha>, repoint current, restart the unit. That is fragile for something that gates a release - install.ps1 and install.sh are served from here, and install.sh resolves /api/releases/linux/latest to find the Linux archive, so publishing a release without deploying the site leaves Windows with no installer and Linux unable to resolve a version. Refuses any commit that is not an ancestor of origin/main, so the live surface always traces back to reviewed source. Verifies the archive actually contains install.sh, install.ps1 and the keepalive payload before going live, and rolls back to the previous release if the new one fails to answer its health endpoint. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Windows is now the ordinary Linux build running inside a per-user WSL 2 distribution, installed by the site-served install.ps1 and removed by uninstall.ps1. Nothing Electron, Squirrel, or signed ships for Windows. - delete scripts/package-windows.cjs, scripts/install-wsl-runtime.ps1, scripts/windows-removal.cjs and test/windows-wsl-status.ps1 - drop the package:windows and package:windows:release npm scripts and the electron-winstaller/png-to-ico devDependencies they were the only users of - remove Squirrel event handling and the com.squirrel app user model id from desktop/main.cjs; unpackedPath stays (still used for the asset root) - remove the Windows branch of desktop/updater.cjs; the macOS Developer ID feed, states and quiesce-before-install path are untouched - remove the in-app WSL runtime installer that drove install-wsl-runtime.ps1 (server launcher, status plumbing, runtime/install win32 branch, and the onboarding/settings cards that could only ever paint for it) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The governance documents still encoded a six-artifact desktop matrix with a Windows Setup executable, Squirrel package/manifest and Authenticode disclosure. None of that exists: Windows installs the ordinary Linux build into WSL 2 from the site-served install.ps1. - release-checklist, release-lifecycle, GOVERNANCE and release-notes-template now name exactly 1Helm-<version>-arm64.dmg, 1Helm-<version>-mac-arm64.zip and 1Helm-<version>-linux-node.tgz, and state that Windows publishes nothing - the Windows acceptance section is replaced with behavioural requirements: non-elevated one-liner install with a single UAC prompt, mid-install restart and resume, keepalive surviving a reboot, browser reaching localhost:8123, prior-version update with /var/lib/1helm-oci-v1 retained, uninstall.ps1 - a Linux artifact that fails acceptance now explicitly blocks Windows too, and install.ps1/uninstall.ps1/keepalive are documented as site-served - test/release-governance.mjs asserts the new contract: all four documents must name the three artifacts and say Windows publishes nothing, must carry every behavioural requirement, and must not reintroduce Squirrel, .nupkg, RELEASES, Authenticode, a Setup executable, package:windows or a Windows update feed Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Version, changelog and the local/1helm-channel-machine:0.0.39 pins. Points the website's offline release fallback at 0.0.39. Its digests are the digests of this commit's own artifacts, so they cannot exist yet; they are filled in at publish and the site is redeployed. Until then the placeholder is deliberately not 64 hex characters, so latestLinuxRelease() rejects it and /api/releases/linux/latest answers 503 - an installer is told no release is available rather than handed a digest that cannot match what it downloads. The macOS download link is unaffected because it resolves by asset name. test/site.mjs now covers both states and refuses a half-filled fallback, which would serve one real digest and two wrong ones. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughWindows delivery now uses a site-served PowerShell installer for a Linux host inside WSL 2. Native Windows packaging and in-app WSL setup were removed. Linux packaging, update validation, keepalive, uninstall, release metadata, and acceptance tests were updated. ChangesWindows WSL 2 deployment
Native Windows removal
Release policy
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (6)
site/public/install.ps1 (1)
59-59: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
$StatusPathparameter.
$StatusPathis never read. A caller who passes it gets no effect and no error. Either implement it or delete it.♻️ Proposed change
[string] $KeepaliveSource = '', - [switch] $HostSetup, - [string] $StatusPath = '' + [switch] $HostSetup )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@site/public/install.ps1` at line 59, Remove the unused $StatusPath parameter from the install script’s parameter declarations, and update any callers or references to stop passing it. Do not add behavior for the parameter; retain all other installation arguments unchanged.Source: Linters/SAST tools
site/public/keepalive/keepalive-hold.sh (1)
33-39: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd backoff to the restart loop, and validate
INTERVAL.Two robustness gaps in this loop:
- If
$SERVICEcannot start — masked, misconfigured, or crash-looping — this callssystemctl startevery 20 s indefinitely with no backoff. systemd's own start-rate limit then rejects the attempts, and each one writes to the journal. The loop never widens its interval and never reports that it has given up.sleep "$INTERVAL"is the only thing that paces the loop. IfINTERVALis ever non-numeric,sleepfails immediately on every iteration and the loop becomes a busy spin inside the distribution. The current caller always passes an integer, so this is defensive.♻️ Proposed change
+# A non-numeric interval would make `sleep` fail instantly and turn the loop +# below into a busy spin. +case "$INTERVAL" in + ''|*[!0-9]*) INTERVAL=20 ;; +esac + +fails=0 while :; do if ! systemctl is-active --quiet "$SERVICE" 2>/dev/null; then - log "$SERVICE is not active - starting it" - systemctl start "$SERVICE" >/dev/null 2>&1 || log "failed to start $SERVICE" + log "$SERVICE is not active - starting it" + if systemctl start "$SERVICE" >/dev/null 2>&1; then + fails=0 + else + fails=$((fails + 1)) + log "failed to start $SERVICE (attempt $fails)" + fi + else + fails=0 fi - sleep "$INTERVAL" + # Back off up to 8x so a permanently failing unit does not flood the + # journal or trip systemd's start-rate limit every 20 seconds. + delay="$INTERVAL" + i=0 + while [ "$i" -lt "$fails" ] && [ "$delay" -lt $((INTERVAL * 8)) ]; do + delay=$((delay * 2)) + i=$((i + 1)) + done + sleep "$delay" done🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@site/public/keepalive/keepalive-hold.sh` around lines 33 - 39, Update the keepalive restart loop to validate that INTERVAL is a positive numeric value before entering the loop, exiting with a clear log message if invalid. Add restart-failure backoff around the systemctl start path: increase the retry delay after consecutive failures, reset it after a successful service check or start, and log when the maximum backoff is reached instead of retrying at the base interval indefinitely.site/server.mjs (1)
448-448: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueConfine the keepalive route to its own directory.
safeFile(SITE_PUBLIC, path.slice(1))confines the resolved path toSITE_PUBLIC, not toSITE_PUBLIC/keepalive. Any file undersite/publicis therefore reachable through the/keepalive/prefix. Node'sURLnormalizes..in the pathname before line 341, so there is no escape aboveSITE_PUBLICtoday, and the existing/schemas/and/assets/routes use the same pattern. Passing the keepalive directory as the root makes the confinement match the route.🔒️ Proposed hardening
- if (path.startsWith("/keepalive/") && serveFile(req, res, safeFile(SITE_PUBLIC, path.slice(1)), "no-cache")) return; + if (path.startsWith("/keepalive/") && serveFile(req, res, safeFile(join(SITE_PUBLIC, "keepalive"), path.slice("/keepalive/".length)), "no-cache")) return;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@site/server.mjs` at line 448, Update the keepalive route in the request handler to call safeFile with the keepalive directory as its root, while preserving the path-relative argument and existing no-cache behavior. Ensure /keepalive/ requests can resolve only files under SITE_PUBLIC/keepalive, unlike the broader SITE_PUBLIC root used by other routes.scripts/package-linux-host.mjs (2)
65-74: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueAddon discovery only finds
build/Releaselayouts.Line 71 matches
*/build/Release/*.node. Packages that ship prebuilt binaries underprebuilds/,lib/binding/, or a package-localbin/are not fingerprinted, so they are also not covered by the loadability check ininstall.sh,update-host.sh, andapply-linux-release.sh.node-ptyis covered today, so this is not currently broken. Widening the match to any.nodefile undernode_moduleswould keep the guarantee true as dependencies change.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/package-linux-host.mjs` around lines 65 - 74, Update nativeAddons to discover every .node file under node_modules, rather than restricting matches to build/Release paths. Preserve recursive traversal and symbolic-link skipping so all native addons—including those in prebuilds/, lib/binding/, and package-local bin/ directories—are returned for the existing loadability checks.
42-45: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winPin the native builder image and enforce the glibc ceiling.
The default
node:22image uses Debian bookworm with glibc 2.36, but the tag is mutable. Pin it to a verified digest. Compare eachsymbolCeiling(file, "GLIBC")result with the declared maximum, including whenHELM_LINUX_NATIVE_BUILDER_IMAGEoverrides the default, so incompatible addons fail during packaging.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/package-linux-host.mjs` around lines 42 - 45, Update nativeBuilderImage to use a verified immutable digest for the default Node 22 image. In the packaging validation flow, ensure every symbolCeiling(file, "GLIBC") result is compared against the declared maximum regardless of whether the image comes from the default or HELM_LINUX_NATIVE_BUILDER_IMAGE override, and fail packaging when any addon exceeds that ceiling.scripts/deploy-site.sh (1)
33-37: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRefresh
origin/mainbefore the ancestry check.The check compares against whatever
origin/mainthe deploy host last fetched. A stale ref rejects a commit that is already on main, and it also accepts a commit that main has since dropped. Rungit fetch origin mainbefore line 36 so the guarantee is real.♻️ Proposed refactor
SHA="$(git rev-parse --verify "$COMMITISH^{commit}")" # A deployed site must be reproducible from a pushed commit, otherwise the live # surface cannot be traced back to reviewed source. +git fetch --quiet origin main \ + || { echo "Could not fetch origin/main; refusing to deploy against a stale ref." >&2; exit 1; } git merge-base --is-ancestor "$SHA" origin/main 2>/dev/null \ || { echo "Refusing to deploy $SHA: it is not an ancestor of origin/main." >&2; exit 1; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/deploy-site.sh` around lines 33 - 37, In the deployment flow before the git merge-base ancestry check, fetch the latest main reference from origin with git fetch origin main. Keep the existing SHA resolution and rejection behavior unchanged, ensuring the check against origin/main uses the refreshed remote-tracking ref.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Line 1102: Update the [0.0.39] changelog comparison link to use v0.0.38 as the
left-hand predecessor, while keeping v0.0.39 as the right-hand tag.
In `@scripts/deploy-site.sh`:
- Around line 83-85: Extend the post-deploy smoke-test loop in the visible path
list to include /uninstall.ps1, ensuring it is checked alongside the existing
installation and health endpoints.
- Around line 46-58: Move the required release validation from the `else` branch
to after the `if [[ -d "$TARGET" ]]` conditional in `scripts/deploy-site.sh`, so
reused and newly extracted directories both pass the `site/server.mjs` and
required-assets checks before promotion. Preserve the existing cleanup and
failure behavior for incomplete targets.
In `@scripts/package-linux-host.mjs`:
- Around line 46-48: Make the Linux packaging flow consistently x64-only: update
install.sh to reject arm64/aarch64 hosts before downloading or installing, and
revise the corresponding architecture handling in apply-linux-release.sh if
needed so it cannot advertise an unsupported arm64 package. Update the Linux
installation documentation to state x86-64 support only, while preserving the
existing x64 packaging behavior in nativeArchitecture and the builder
validation.
- Around line 172-184: Update the install command in the containerRuntime
spawnSync call to restore ownership of the mounted staging tree to the invoking
host user after npm ci completes, using the container’s available UID/GID
values. Keep the ownership correction inside the container and preserve the
existing install failure check and cleanup behavior.
In `@site/public/install.ps1`:
- Around line 271-277: Wrap the elevated Start-Process call in the
administrator-launch flow with targeted exception handling so a cancelled UAC
prompt does not terminate with a raw .NET error. Ensure the failure leaves the
child process unset and continues to the existing `$null -eq $code` check,
allowing the established `Die` message to report that approval was cancelled or
Windows could not start the step.
- Around line 344-350: Update the systemd configuration block around
$wslConfProbe so enabling systemd preserves existing /etc/wsl.conf contents.
When the file already exists, append or merge the [boot] systemd=true setting
rather than using printf with overwrite redirection; retain the current creation
behavior for a missing file and the subsequent WSL termination flow.
- Around line 243-245: Update the architecture guard in the install script to
derive the effective architecture from PROCESSOR_ARCHITEW6432 when it is set,
otherwise PROCESSOR_ARCHITECTURE, and require that value to be AMD64. Preserve
the existing 64-bit operating-system requirement and call Die before any x64
artifact download, rejecting ARM64 and other architectures.
In `@site/public/keepalive/keepalive-install.ps1`:
- Around line 83-88: Add the existing $Distro command-line filter to both
process queries in site/public/keepalive/keepalive-install.ps1 (lines 83-88), to
the $sup and $anch queries in site/public/keepalive/keepalive-remove.ps1 (lines
68-77), and to the $sup query in site/public/uninstall.ps1 (lines 234-235);
leave the already-scoped uninstall anchor query unchanged.
- Around line 181-188: Use distinct exit statuses for keepalive registration
versus readiness: in site/public/keepalive/keepalive-install.ps1 lines 181-188,
preserve exit 1 for registration failures and return a separate status such as
exit 2 when the task registered but the health probe timed out. In
site/public/install.ps1 lines 413-415, treat only the registration-failure
status as fatal; warn and continue for the readiness-timeout status so the
existing shortcut creation and readiness wait proceed.
In `@site/public/keepalive/keepalive-remove.ps1`:
- Around line 121-130: Update the residual-state reporting block in
keepalive-remove.ps1 to return a non-zero exit code when the scheduled task
remains present or any supervisor or anchor processes remain. Preserve the
existing status output, and exit successfully only when all residual counts
indicate the keepalive teardown is complete so uninstall.ps1 can execute its
fallback path.
In `@site/public/keepalive/keepalive-run.ps1`:
- Around line 230-239: Update the supervision path around the running-service
checks to execute each WslExe/systemctl invocation through a bounded child
process, using dedicated capture files and terminating the child when it exceeds
a timeout so the loop always resumes and escalation remains reachable. Apply the
same timeout protection to the Restart-Distro flow, preserving existing output
parsing, logging, and recovery behavior.
In `@site/public/update-host.sh`:
- Around line 244-245: Introduce a dedicated Node/native-addon architecture
variable in the update flow, mapped explicitly to the manifest values x64 or
arm64, and use it in verify_native_addons instead of CONNECTOR_ARCH. Keep
CONNECTOR_ARCH exclusively for selecting cloudflared assets and preserve the
existing architecture validation behavior.
In `@test/site.mjs`:
- Line 111: Replace the current normalized-path assertion in the test with a
direct safeFile check using ../server.mjs that verifies traversal is rejected,
while retaining a separate HTTP request using an encoded slash to exercise
server handling of encoded paths. Anchor the changes to the existing safeFile
and keepalive route tests, preserving the expected 404 responses.
---
Nitpick comments:
In `@scripts/deploy-site.sh`:
- Around line 33-37: In the deployment flow before the git merge-base ancestry
check, fetch the latest main reference from origin with git fetch origin main.
Keep the existing SHA resolution and rejection behavior unchanged, ensuring the
check against origin/main uses the refreshed remote-tracking ref.
In `@scripts/package-linux-host.mjs`:
- Around line 65-74: Update nativeAddons to discover every .node file under
node_modules, rather than restricting matches to build/Release paths. Preserve
recursive traversal and symbolic-link skipping so all native addons—including
those in prebuilds/, lib/binding/, and package-local bin/ directories—are
returned for the existing loadability checks.
- Around line 42-45: Update nativeBuilderImage to use a verified immutable
digest for the default Node 22 image. In the packaging validation flow, ensure
every symbolCeiling(file, "GLIBC") result is compared against the declared
maximum regardless of whether the image comes from the default or
HELM_LINUX_NATIVE_BUILDER_IMAGE override, and fail packaging when any addon
exceeds that ceiling.
In `@site/public/install.ps1`:
- Line 59: Remove the unused $StatusPath parameter from the install script’s
parameter declarations, and update any callers or references to stop passing it.
Do not add behavior for the parameter; retain all other installation arguments
unchanged.
In `@site/public/keepalive/keepalive-hold.sh`:
- Around line 33-39: Update the keepalive restart loop to validate that INTERVAL
is a positive numeric value before entering the loop, exiting with a clear log
message if invalid. Add restart-failure backoff around the systemctl start path:
increase the retry delay after consecutive failures, reset it after a successful
service check or start, and log when the maximum backoff is reached instead of
retrying at the base interval indefinitely.
In `@site/server.mjs`:
- Line 448: Update the keepalive route in the request handler to call safeFile
with the keepalive directory as its root, while preserving the path-relative
argument and existing no-cache behavior. Ensure /keepalive/ requests can resolve
only files under SITE_PUBLIC/keepalive, unlike the broader SITE_PUBLIC root used
by other routes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2b7d2030-bb1d-42a9-8076-dfca1db29c97
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (41)
CHANGELOG.mdREADME.mddesktop/main.cjsdesktop/updater.cjsdocs/GOVERNANCE.mddocs/USER_GUIDE.mddocs/release-checklist.mddocs/release-lifecycle.mddocs/release-notes-template.mdpackage.jsonpublic/index.htmlscripts/deploy-site.shscripts/install-wsl-runtime.ps1scripts/package-linux-host.mjsscripts/package-windows.cjsscripts/windows-removal.cjssite/content.mjssite/manual.htmlsite/public/apply-linux-release.shsite/public/install.ps1site/public/install.shsite/public/keepalive/keepalive-hold.shsite/public/keepalive/keepalive-install.ps1site/public/keepalive/keepalive-remove.ps1site/public/keepalive/keepalive-run.ps1site/public/uninstall.ps1site/public/update-host.shsite/server.mjssrc/client/api.tssrc/client/onboarding.tssrc/client/settings.tssrc/server/channel-computers.tssrc/server/db.tssrc/server/index.tstest/channel-computers.mjstest/desktop.mjstest/release-governance.mjstest/release-license.mjstest/site.mjstest/update-service.mjstest/windows-wsl-status.ps1
💤 Files with no reviewable changes (5)
- scripts/install-wsl-runtime.ps1
- scripts/windows-removal.cjs
- src/client/api.ts
- test/windows-wsl-status.ps1
- scripts/package-windows.cjs
| Application Support, and isolated Apple container machines. | ||
|
|
||
| [Unreleased]: https://github.com/gitcommit90/1Helm/compare/v0.0.36...HEAD | ||
| [0.0.39]: https://github.com/gitcommit90/1Helm/compare/v0.0.30...v0.0.39 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Link the release to the immediate predecessor.
Line 1102 compares v0.0.30 through v0.0.39. This includes the changes already represented by versions 0.0.31 through 0.0.38. Change the left tag to v0.0.38.
Proposed fix
-[0.0.39]: https://github.com/gitcommit90/1Helm/compare/v0.0.30...v0.0.39
+[0.0.39]: https://github.com/gitcommit90/1Helm/compare/v0.0.38...v0.0.39📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| [0.0.39]: https://github.com/gitcommit90/1Helm/compare/v0.0.30...v0.0.39 | |
| [0.0.39]: https://github.com/gitcommit90/1Helm/compare/v0.0.38...v0.0.39 |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CHANGELOG.md` at line 1102, Update the [0.0.39] changelog comparison link to
use v0.0.38 as the left-hand predecessor, while keeping v0.0.39 as the
right-hand tag.
Comment on lines
+46
to
+58
| if [[ -d "$TARGET" ]]; then | ||
| echo "release directory already exists; reusing it" | ||
| else | ||
| install -d -m 0755 "$TARGET" | ||
| git archive --format=tar "$SHA" | tar -x -C "$TARGET" | ||
| # The site serves the product's own public assets from ../public, so a bare | ||
| # archive of site/ alone would 404 icons and schemas. | ||
| [[ -d "$TARGET/site" && -f "$TARGET/site/server.mjs" ]] \ | ||
| || { echo "archive is missing site/server.mjs" >&2; rm -rf -- "$TARGET"; exit 1; } | ||
| for required in site/public/install.sh site/public/install.ps1 site/public/keepalive/keepalive-install.ps1; do | ||
| [[ -e "$TARGET/$required" ]] || { echo "archive is missing $required" >&2; rm -rf -- "$TARGET"; exit 1; } | ||
| done | ||
| fi |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Validate a reused release directory before promoting it.
The required-asset checks at lines 53-57 only run in the else branch. If an earlier run was interrupted during git archive | tar -x, $TARGET exists but is incomplete. Line 47 then reuses it, and line 61 promotes that partial tree to current. The installers in this same PR guard against exactly this case (install.sh line 306-309, update-host.sh line 282-285).
Move the validation out of the conditional so both the fresh and the reused path are proven.
🐛 Proposed fix: validate on both paths
if [[ -d "$TARGET" ]]; then
echo "release directory already exists; reusing it"
else
install -d -m 0755 "$TARGET"
git archive --format=tar "$SHA" | tar -x -C "$TARGET"
- # The site serves the product's own public assets from ../public, so a bare
- # archive of site/ alone would 404 icons and schemas.
- [[ -d "$TARGET/site" && -f "$TARGET/site/server.mjs" ]] \
- || { echo "archive is missing site/server.mjs" >&2; rm -rf -- "$TARGET"; exit 1; }
- for required in site/public/install.sh site/public/install.ps1 site/public/keepalive/keepalive-install.ps1; do
- [[ -e "$TARGET/$required" ]] || { echo "archive is missing $required" >&2; rm -rf -- "$TARGET"; exit 1; }
- done
fi
+# The site serves the product's own public assets from ../public, so a bare
+# archive of site/ alone would 404 icons and schemas. A directory retained from
+# an interrupted earlier run can also be incomplete, so check both paths.
+[[ -d "$TARGET/site" && -f "$TARGET/site/server.mjs" ]] \
+ || { echo "archive is missing site/server.mjs" >&2; rm -rf -- "$TARGET"; exit 1; }
+for required in site/public/install.sh site/public/install.ps1 site/public/uninstall.ps1 \
+ site/public/keepalive/keepalive-install.ps1; do
+ [[ -e "$TARGET/$required" ]] || { echo "archive is missing $required" >&2; rm -rf -- "$TARGET"; exit 1; }
+done📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if [[ -d "$TARGET" ]]; then | |
| echo "release directory already exists; reusing it" | |
| else | |
| install -d -m 0755 "$TARGET" | |
| git archive --format=tar "$SHA" | tar -x -C "$TARGET" | |
| # The site serves the product's own public assets from ../public, so a bare | |
| # archive of site/ alone would 404 icons and schemas. | |
| [[ -d "$TARGET/site" && -f "$TARGET/site/server.mjs" ]] \ | |
| || { echo "archive is missing site/server.mjs" >&2; rm -rf -- "$TARGET"; exit 1; } | |
| for required in site/public/install.sh site/public/install.ps1 site/public/keepalive/keepalive-install.ps1; do | |
| [[ -e "$TARGET/$required" ]] || { echo "archive is missing $required" >&2; rm -rf -- "$TARGET"; exit 1; } | |
| done | |
| fi | |
| if [[ -d "$TARGET" ]]; then | |
| echo "release directory already exists; reusing it" | |
| else | |
| install -d -m 0755 "$TARGET" | |
| git archive --format=tar "$SHA" | tar -x -C "$TARGET" | |
| fi | |
| # The site serves the product's own public assets from ../public, so a bare | |
| # archive of site/ alone would 404 icons and schemas. A directory retained from | |
| # an interrupted earlier run can also be incomplete, so check both paths. | |
| [[ -d "$TARGET/site" && -f "$TARGET/site/server.mjs" ]] \ | |
| || { echo "archive is missing site/server.mjs" >&2; rm -rf -- "$TARGET"; exit 1; } | |
| for required in site/public/install.sh site/public/install.ps1 site/public/uninstall.ps1 \ | |
| site/public/keepalive/keepalive-install.ps1; do | |
| [[ -e "$TARGET/$required" ]] || { echo "archive is missing $required" >&2; rm -rf -- "$TARGET"; exit 1; } | |
| done |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/deploy-site.sh` around lines 46 - 58, Move the required release
validation from the `else` branch to after the `if [[ -d "$TARGET" ]]`
conditional in `scripts/deploy-site.sh`, so reused and newly extracted
directories both pass the `site/server.mjs` and required-assets checks before
promotion. Preserve the existing cleanup and failure behavior for incomplete
targets.
Comment on lines
+83
to
+85
| for path in /health /install.sh /install.ps1 /keepalive/keepalive-install.ps1 /api/releases/linux/latest; do | ||
| printf ' %-38s %s\n' "$path" "$(curl -s -o /dev/null -w '%{http_code}' "http://127.0.0.1:$PORT$path")" | ||
| done |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Add /uninstall.ps1 to the post-deploy smoke list.
site/server.mjs line 447 serves /uninstall.ps1, and test/site.mjs treats a 404 there as stranding every Windows installation. The smoke list omits it, so a deploy that breaks that route passes silently.
🔎 Proposed fix
-for path in /health /install.sh /install.ps1 /keepalive/keepalive-install.ps1 /api/releases/linux/latest; do
+for path in /health /install.sh /install.ps1 /uninstall.ps1 /keepalive/keepalive-install.ps1 /api/releases/linux/latest; do📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for path in /health /install.sh /install.ps1 /keepalive/keepalive-install.ps1 /api/releases/linux/latest; do | |
| printf ' %-38s %s\n' "$path" "$(curl -s -o /dev/null -w '%{http_code}' "http://127.0.0.1:$PORT$path")" | |
| done | |
| for path in /health /install.sh /install.ps1 /uninstall.ps1 /keepalive/keepalive-install.ps1 /api/releases/linux/latest; do | |
| printf ' %-38s %s\n' "$path" "$(curl -s -o /dev/null -w '%{http_code}' "http://127.0.0.1:$PORT$path")" | |
| done |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/deploy-site.sh` around lines 83 - 85, Extend the post-deploy
smoke-test loop in the visible path list to include /uninstall.ps1, ensuring it
is checked alongside the existing installation and health endpoints.
Comment on lines
+46
to
+48
| const nativeArchitecture = "x64"; | ||
| const nativeManifestPath = "resources/linux-native-modules.json"; | ||
| const requiredNativeModule = "node_modules/node-pty/build/Release/pty.node"; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Packaging is x64-only, but the installers still accept arm64 Linux hosts.
nativeArchitecture is fixed to "x64" and line 93 refuses any non-x64 builder, so only an x64 archive can ever be produced. The consumers still advertise arm64:
site/public/install.shlines 61-64 mapaarch64|arm64toNODE_ARCH="arm64".site/public/apply-linux-release.shlines 16-19 map the same values toNATIVE_ARCH="arm64".docs/USER_GUIDE.mdline 481 states the Linux installer supports "an x86-64 or arm64 CPU".
On an arm64 host the digest check passes, then verify_ready_to_run fails at manifest.arch !== hostArch. The user gets a refusal after the full download and package install, with no published archive that can ever satisfy it.
Either produce an arm64 archive as well, or make the arm64 refusal explicit and early in install.sh and in the documentation.
Also applies to: 93-99
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/package-linux-host.mjs` around lines 46 - 48, Make the Linux
packaging flow consistently x64-only: update install.sh to reject arm64/aarch64
hosts before downloading or installing, and revise the corresponding
architecture handling in apply-linux-release.sh if needed so it cannot advertise
an unsupported arm64 package. Update the Linux installation documentation to
state x86-64 support only, while preserving the existing x64 packaging behavior
in nativeArchitecture and the builder validation.
Comment on lines
+172
to
+184
| const install = spawnSync(containerRuntime, [ | ||
| "run", "--rm", "--network=host", | ||
| "-v", `${join(stage, prefix)}:/workspace`, | ||
| "-w", "/workspace", | ||
| "-e", "PUPPETEER_SKIP_DOWNLOAD=1", | ||
| "-e", "ELECTRON_SKIP_BINARY_DOWNLOAD=1", | ||
| "-e", "npm_config_audit=false", | ||
| "-e", "npm_config_fund=false", | ||
| "-e", "npm_config_update_notifier=false", | ||
| nativeBuilderImage, | ||
| "npm", "ci", "--omit=dev", | ||
| ], { stdio: "inherit" }); | ||
| if (install.status !== 0) throw new Error("Could not install the Linux release production dependencies inside the native builder image"); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
npm ci in a rootful container writes root-owned files into the staging tree.
The stage directory is created by mkdtempSync as the packaging user. Line 172 mounts it into the container and runs npm ci as container root. Under rootful Docker the generated node_modules tree is owned by host root. Two consequences follow:
rmSync(stage, { recursive: true, force: true })in thefinallyblock at line 222 throwsEACCESfor the nested root-owned directories. That error replaces the real packaging error.- The archived files carry root ownership.
Rootless Podman maps container root to the invoking user and is unaffected, so this only reproduces on some builders. Restore ownership inside the container after the install.
🐛 Proposed fix: chown the staged tree back inside the container
nativeBuilderImage,
- "npm", "ci", "--omit=dev",
+ "sh", "-lc", `npm ci --omit=dev && chown -R ${process.getuid()}:${process.getgid()} /workspace`,
], { stdio: "inherit" });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const install = spawnSync(containerRuntime, [ | |
| "run", "--rm", "--network=host", | |
| "-v", `${join(stage, prefix)}:/workspace`, | |
| "-w", "/workspace", | |
| "-e", "PUPPETEER_SKIP_DOWNLOAD=1", | |
| "-e", "ELECTRON_SKIP_BINARY_DOWNLOAD=1", | |
| "-e", "npm_config_audit=false", | |
| "-e", "npm_config_fund=false", | |
| "-e", "npm_config_update_notifier=false", | |
| nativeBuilderImage, | |
| "npm", "ci", "--omit=dev", | |
| ], { stdio: "inherit" }); | |
| if (install.status !== 0) throw new Error("Could not install the Linux release production dependencies inside the native builder image"); | |
| const install = spawnSync(containerRuntime, [ | |
| "run", "--rm", "--network=host", | |
| "-v", `${join(stage, prefix)}:/workspace`, | |
| "-w", "/workspace", | |
| "-e", "PUPPETEER_SKIP_DOWNLOAD=1", | |
| "-e", "ELECTRON_SKIP_BINARY_DOWNLOAD=1", | |
| "-e", "npm_config_audit=false", | |
| "-e", "npm_config_fund=false", | |
| "-e", "npm_config_update_notifier=false", | |
| nativeBuilderImage, | |
| "sh", "-lc", `npm ci --omit=dev && chown -R ${process.getuid()}:${process.getgid()} /workspace`, | |
| ], { stdio: "inherit" }); | |
| if (install.status !== 0) throw new Error("Could not install the Linux release production dependencies inside the native builder image"); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/package-linux-host.mjs` around lines 172 - 184, Update the install
command in the containerRuntime spawnSync call to restore ownership of the
mounted staging tree to the invoking host user after npm ci completes, using the
container’s available UID/GID values. Keep the ownership correction inside the
container and preserve the existing install failure check and cleanup behavior.
Comment on lines
+181
to
+188
| if ($ok) { | ||
| Say "OK: $Distro is up and http://localhost:$HealthPort is answering." 'Green' | ||
| } else { | ||
| Say "WARNING: task registered and started, but :$HealthPort did not answer within ${WaitSeconds}s." 'Red' | ||
| Say " Check $InstallDir\keepalive.log" 'Red' | ||
| } | ||
| Say "Remove with: powershell -NoProfile -ExecutionPolicy Bypass -File $InstallDir\keepalive-remove.ps1" | ||
| if (-not $ok) { exit 1 } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
The keepalive exit code conflates registration failure with a slow first start. keepalive-install.ps1 returns 1 both when registration genuinely fails and when the task registered correctly but http://localhost:8123/ did not answer within $WaitSeconds. install.ps1 treats any non-zero code as a fatal registration failure, so a slow first boot aborts an otherwise successful install with an inaccurate message. keepalive-run.ps1 Lines 225-228 record that a cold distribution boot plus 1Helm start took 33-80 s on an 8-core box and is slower on modest hardware, so the 180 s probe is reachable on target hardware.
site/public/keepalive/keepalive-install.ps1#L181-L188: return a distinct code for "registered but not answering yet" — for exampleexit 2— and keepexit 1for the registration failures at Lines 156 and 163. The task is already proven registered at Line 160, so the probe result must not overwrite that verdict.site/public/install.ps1#L413-L415: treat only the registration-failure code as fatal. On the new "registered, not answering yet" code, print a warning and continue to the Start Menu shortcut and the existing readiness wait at Lines 437-444, which polls for a further 120 s.
📍 Affects 2 files
site/public/keepalive/keepalive-install.ps1#L181-L188(this comment)site/public/install.ps1#L413-L415
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@site/public/keepalive/keepalive-install.ps1` around lines 181 - 188, Use
distinct exit statuses for keepalive registration versus readiness: in
site/public/keepalive/keepalive-install.ps1 lines 181-188, preserve exit 1 for
registration failures and return a separate status such as exit 2 when the task
registered but the health probe timed out. In site/public/install.ps1 lines
413-415, treat only the registration-failure status as fatal; warn and continue
for the readiness-timeout status so the existing shortcut creation and readiness
wait proceed.
Comment on lines
+121
to
+130
| # --- residual state report ------------------------------------------------- | ||
| Say "" | ||
| Say "Residual state:" 'Cyan' | ||
| $t = Get-ScheduledTask -TaskPath $TaskPath -TaskName $TaskName -ErrorAction SilentlyContinue | ||
| Say (" scheduled task : {0}" -f $(if ($t) { 'STILL PRESENT' } else { 'gone' })) | ||
| $s = @(Get-CimInstance Win32_Process -Filter "Name='powershell.exe'" -EA SilentlyContinue | Where-Object { $_.CommandLine -like '*keepalive-run.ps1*' -and $_.ProcessId -ne $PID }) | ||
| Say (" supervisor processes: {0}" -f $s.Count) | ||
| $a = @(Get-CimInstance Win32_Process -Filter "Name='wsl.exe'" -EA SilentlyContinue | Where-Object { $_.CommandLine -like '*keepalive-hold.sh*' }) | ||
| Say (" anchor processes : {0}" -f $a.Count) | ||
| Say "Done." 'Green' |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Exit non-zero when residue remains.
This script computes the residual state at Lines 124-129 and then discards it. It never calls exit, so the child powershell.exe always returns 0.
uninstall.ps1 Line 193 branches on exactly that code. When the task is still present or a supervisor survived, uninstall.ps1 still records keepalive (via its own keepalive-remove.ps1) as removed and skips its own fallback teardown at Lines 201-245 — the fallback that exists to handle this case. The user is told the keepalive is gone while it is still running and still restarting 1helm.service.
Return the residual state to the caller.
🐛 Proposed fix
$a = @(Get-CimInstance Win32_Process -Filter "Name='wsl.exe'" -EA SilentlyContinue | Where-Object { $_.CommandLine -like '*keepalive-hold.sh*' })
Say (" anchor processes : {0}" -f $a.Count)
-Say "Done." 'Green'
+if ($t -or $s.Count -gt 0 -or $a.Count -gt 0) {
+ Say "Done, but residue remains - the caller must fall back to its own teardown." 'Yellow'
+ exit 1
+}
+Say "Done." 'Green'
+exit 0📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # --- residual state report ------------------------------------------------- | |
| Say "" | |
| Say "Residual state:" 'Cyan' | |
| $t = Get-ScheduledTask -TaskPath $TaskPath -TaskName $TaskName -ErrorAction SilentlyContinue | |
| Say (" scheduled task : {0}" -f $(if ($t) { 'STILL PRESENT' } else { 'gone' })) | |
| $s = @(Get-CimInstance Win32_Process -Filter "Name='powershell.exe'" -EA SilentlyContinue | Where-Object { $_.CommandLine -like '*keepalive-run.ps1*' -and $_.ProcessId -ne $PID }) | |
| Say (" supervisor processes: {0}" -f $s.Count) | |
| $a = @(Get-CimInstance Win32_Process -Filter "Name='wsl.exe'" -EA SilentlyContinue | Where-Object { $_.CommandLine -like '*keepalive-hold.sh*' }) | |
| Say (" anchor processes : {0}" -f $a.Count) | |
| Say "Done." 'Green' | |
| # --- residual state report ------------------------------------------------- | |
| Say "" | |
| Say "Residual state:" 'Cyan' | |
| $t = Get-ScheduledTask -TaskPath $TaskPath -TaskName $TaskName -ErrorAction SilentlyContinue | |
| Say (" scheduled task : {0}" -f $(if ($t) { 'STILL PRESENT' } else { 'gone' })) | |
| $s = @(Get-CimInstance Win32_Process -Filter "Name='powershell.exe'" -EA SilentlyContinue | Where-Object { $_.CommandLine -like '*keepalive-run.ps1*' -and $_.ProcessId -ne $PID }) | |
| Say (" supervisor processes: {0}" -f $s.Count) | |
| $a = @(Get-CimInstance Win32_Process -Filter "Name='wsl.exe'" -EA SilentlyContinue | Where-Object { $_.CommandLine -like '*keepalive-hold.sh*' }) | |
| Say (" anchor processes : {0}" -f $a.Count) | |
| if ($t -or $s.Count -gt 0 -or $a.Count -gt 0) { | |
| Say "Done, but residue remains - the caller must fall back to its own teardown." 'Yellow' | |
| exit 1 | |
| } | |
| Say "Done." 'Green' | |
| exit 0 |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@site/public/keepalive/keepalive-remove.ps1` around lines 121 - 130, Update
the residual-state reporting block in keepalive-remove.ps1 to return a non-zero
exit code when the scheduled task remains present or any supervisor or anchor
processes remain. Preserve the existing status output, and exit successfully
only when all residual counts indicate the keepalive teardown is complete so
uninstall.ps1 can execute its fallback path.
Comment on lines
+230
to
+239
| if ($running) { | ||
| $unitState = '' | ||
| try { $unitState = (& $WslExe -d $Distro -u root --exec /usr/bin/systemctl is-active $Service 2>&1 | Out-String).Trim() } catch { } | ||
| if ($unitState -match 'activating') { | ||
| Write-Log "$Service is still activating - waiting rather than restarting it" | ||
| $deepFail-- | ||
| } else { | ||
| try { & $WslExe -d $Distro -u root --exec /usr/bin/systemctl restart $Service 2>&1 | Out-Null } catch { } | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
wsl.exe calls on the supervision path have no timeout.
These & $WslExe ... systemctl calls block until wsl.exe returns. A wedged VM or a distribution that is mid-crash can leave wsl.exe hanging indefinitely. The supervisor then stops inside the try block: the loop never reaches Start-Sleep at Line 252, the tick counter freezes, and the escalation at Lines 240-246 becomes unreachable. The catch at Line 249 cannot recover this, because a hang throws nothing.
The state that triggers this branch is exactly the state in which wsl.exe is most likely to hang, so the watchdog is most likely to stop watching when it is needed most. Restart-Distro at Line 175 has the same exposure.
Bound each call. Run it as a child process and kill it if it exceeds a deadline.
♻️ Proposed approach
+function Invoke-WslBounded {
+ # wsl.exe can hang indefinitely against a wedged VM. Never block the
+ # supervision loop on it.
+ param([string]$ArgLine, [int]$TimeoutSeconds = 30)
+ $p = Start-Process -FilePath $WslExe -ArgumentList $ArgLine -NoNewWindow -PassThru `
+ -RedirectStandardOutput $AnchorOut -RedirectStandardError $AnchorErr
+ if (-not $p.WaitForExit($TimeoutSeconds * 1000)) {
+ Write-Log "wsl.exe timed out after ${TimeoutSeconds}s: $ArgLine" 'ERROR'
+ try { Stop-Process -Id $p.Id -Force -ErrorAction SilentlyContinue } catch { }
+ return $null
+ }
+ return $p.ExitCode
+}Then replace the two inline invocations:
if ($running) {
$unitState = ''
- try { $unitState = (& $WslExe -d $Distro -u root --exec /usr/bin/systemctl is-active $Service 2>&1 | Out-String).Trim() } catch { }
+ $code = Invoke-WslBounded ('-d {0} -u root --exec /usr/bin/systemctl is-active {1}' -f (ConvertTo-WslArg $Distro), $Service)
+ if ($null -ne $code) { $unitState = (Get-AnchorError) }
if ($unitState -match 'activating') {
Write-Log "$Service is still activating - waiting rather than restarting it"
$deepFail--
} else {
- try { & $WslExe -d $Distro -u root --exec /usr/bin/systemctl restart $Service 2>&1 | Out-Null } catch { }
+ $null = Invoke-WslBounded ('-d {0} -u root --exec /usr/bin/systemctl restart {1}' -f (ConvertTo-WslArg $Distro), $Service) 60
}
}Use dedicated capture files rather than the anchor logs if you adopt this shape.
🧰 Tools
🪛 PSScriptAnalyzer (1.25.0)
[warning] 232-232: Empty catch block is used. Please use Write-Error or throw statements in catch blocks.
(PSAvoidUsingEmptyCatchBlock)
[warning] 237-237: Empty catch block is used. Please use Write-Error or throw statements in catch blocks.
(PSAvoidUsingEmptyCatchBlock)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@site/public/keepalive/keepalive-run.ps1` around lines 230 - 239, Update the
supervision path around the running-service checks to execute each
WslExe/systemctl invocation through a bounded child process, using dedicated
capture files and terminating the child when it exceeds a timeout so the loop
always resumes and escalation remains reachable. Apply the same timeout
protection to the Restart-Distro flow, preserving existing output parsing,
logging, and recovery behavior.
Comment on lines
+244
to
+245
| verify_native_addons() { | ||
| "$NODE_LINK/bin/node" - "$1" "$CONNECTOR_ARCH" <<'NODE' |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
verify_native_addons compares the manifest arch against CONNECTOR_ARCH.
CONNECTOR_ARCH names the cloudflared asset at line 229 (resources/cloudflared-linux-$CONNECTOR_ARCH). The manifest arch is produced by scripts/package-linux-host.mjs line 46 as "x64". The two sibling scripts use a dedicated variable for this comparison: install.sh uses NODE_ARCH and apply-linux-release.sh uses NATIVE_ARCH, both mapped explicitly to x64/arm64.
If CONNECTOR_ARCH uses the cloudflared vocabulary (amd64) rather than the Node vocabulary (x64), then line 250 throws on every host and no update can ever be promoted. Introduce a separate arch variable for the manifest comparison even if the values match today.
Run the following script to check how CONNECTOR_ARCH is derived and which cloudflared asset names ship:
#!/bin/bash
set -euo pipefail
rg -n -C4 'CONNECTOR_ARCH' site/public/update-host.sh
fd -t f 'cloudflared-linux' | head -20
rg -n 'cloudflared-linux-' --glob '!node_modules' | head -30🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@site/public/update-host.sh` around lines 244 - 245, Introduce a dedicated
Node/native-addon architecture variable in the update flow, mapped explicitly to
the manifest values x64 or arm64, and use it in verify_native_addons instead of
CONNECTOR_ARCH. Keep CONNECTOR_ARCH exclusively for selecting cloudflared assets
and preserve the existing architecture validation behavior.
| for (const part of ["keepalive-install.ps1", "keepalive-run.ps1", "keepalive-remove.ps1", "keepalive-hold.sh"]) { | ||
| assert.equal((await fetch(`${base}/keepalive/${part}`)).status, 200, `/keepalive/${part} must be served`); | ||
| } | ||
| assert.equal((await fetch(`${base}/keepalive/../server.mjs`)).status, 404, "the keepalive route must not escape its directory"); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm how safeFile normalizes and contains paths in the site server.
set -u
fd -t f 'server.mjs' site || true
rg -n -C 12 'function safeFile|const safeFile' site/ || true
rg -n -C 3 'safeFile\(' site/ || trueRepository: gitcommit90/1Helm
Length of output: 3495
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '--- test/site.mjs ---'
sed -n '80,125p' test/site.mjs
printf '%s\n' '--- site/server.mjs request parsing and routing ---'
sed -n '390,465p' site/server.mjs
printf '%s\n' '--- relevant public files ---'
git ls-files site | rg '(^|/)(server\.mjs|keepalive|schemas|assets|media|icons|brand)/|site/public' | head -80
printf '%s\n' '--- URL behavior probe ---'
node - <<'JS'
for (const value of [
'http://example.test/keepalive/../server.mjs',
'http://example.test/keepalive/%2e%2e/server.mjs',
'http://example.test/keepalive/..%2fserver.mjs',
'http://example.test/keepalive/%2e%2e%2fserver.mjs',
]) {
const url = new URL(value);
console.log(JSON.stringify({input: value, href: url.href, pathname: url.pathname}));
}
JSRepository: gitcommit90/1Helm
Length of output: 9394
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '--- request URL parsing ---'
rg -n -C 10 'new URL|req\.url|pathname|decodeURI|decodeURIComponent' site/server.mjs
printf '%s\n' '--- safeFile imports and path semantics ---'
sed -n '1,45p' site/server.mjs
printf '%s\n' '--- path normalization probe ---'
node - <<'JS'
import { normalize, resolve } from "node:path";
const root = "/repo/site/public";
for (const value of [
"keepalive/..%2fserver.mjs",
"keepalive/%2e%2e%2fserver.mjs",
"keepalive/../server.mjs",
]) {
const relative = normalize(value).replace(/^[/\\]+/, "");
const candidate = resolve(root, relative);
console.log(JSON.stringify({value, relative, candidate, contained: candidate === root || candidate.startsWith(`${root}/`)}));
}
JS
printf '%s\n' '--- files that can provide a served escape target ---'
find site/public -maxdepth 2 -type f -printf '%P\n' | sort | head -100Repository: gitcommit90/1Helm
Length of output: 4980
🏁 Script executed:
#!/bin/bash
set -u
sed -n '45,95p' site/server.mjs
sed -n '250,330p' site/server.mjs
printf '%s\n' '--- exact route and server construction ---'
rg -n -C 8 'createServer|handleRequest|function .*request|new URL' site/server.mjsRepository: gitcommit90/1Helm
Length of output: 7979
Test safeFile directly. fetch and the server’s new URL() both normalize /keepalive/../server.mjs to /server.mjs. Encoding only the slash avoids normalization, but safeFile then treats %2f as a literal character, so the 404 still does not test traversal rejection. Add a direct safeFile test for ../server.mjs, and retain an HTTP test for encoded-path handling.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/site.mjs` at line 111, Replace the current normalized-path assertion in
the test with a direct safeFile check using ../server.mjs that verifies
traversal is rejected, while retaining a separate HTTP request using an encoded
slash to exercise server handling of encoded paths. Anchor the changes to the
existing safeFile and keepalive route tests, preserving the expected 404
responses.
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
2 participants
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.
Windows no longer ships an application. It runs the ordinary Linux build inside a WSL 2 distribution, with the browser as the interface at
http://localhost:8123:No Electron host, no Squirrel, no
.exe— so nothing to code-sign and SmartScreen never appears. That matters because 1Helm has no Authenticode identity and was never going to obtain one.Verified on real hardware
The owner installed this on a Windows 11 VM that was blank an hour earlier, and confirmed it working.
wsl.exeat a flat ~208 ms — measured 281 ms vs 73 ms for identical work. Those crossings no longer exist.spawnSyncon the Electron main thread — the thread Windows needs for its message pump — so a file listing could stall the window past the 5 s "not responding" threshold. There is no window now.enabled=Nand skips it): both the write and the already-compliant branches exercised, grants live in the kernel's effective policy.pty.node.Bugs this found and fixed
Several were only reachable by a real user, because automated testing over SSH gets a pre-elevated Windows token:
Get-WindowsOptionalFeaturerequires elevation and was called from the unelevated pass — the installer failed on its very first action.Invoke-InDistroreturned the command's entire stdout plus its exit code as an array, so$code -ne 0was an array filter. Since the install step always prints, success was unreachable: a completed install reported failure with 40 KB of apt log in the error.install.shhad no local-archive path, so-LocalArchivesilently installed the published release instead of the archive named..lnkTargetPath, which saves an empty target — the icon appeared and did nothing.EADDRINUSE; its probe only required that something answered port 8123, which a foreign listener satisfies.latestLinuxRelease()threw unless a Setup executable,.nupkgandRELEASESexisted. It backs the endpointinstall.shresolves, so the first release without those files would have broken the public Linux and Windows installers simultaneously.Scope
Deleted:
package-windows.cjs,install-wsl-runtime.ps1,windows-removal.cjs, Squirrel handling inmain.cjs, the Windows branch ofupdater.cjs(macOS untouched), and the in-app Windows setup UI. Added:install.ps1,uninstall.ps1, the keepalive payload, and a site deploy script with rollback. Release matrix 6 artifacts → 3; governance docs andAGENTS.mdupdated to match.npm run ci: 122 tests, 120 pass, 0 fail, 2 pre-existing skips.Known, logged, not blocking
Skipper cannot move an attachment between channels — the bytes, the index and a
storage-writeverb all exist, but no tool exposes them. Present on macOS too, unrelated to this change.🤖 Generated with Claude Code
Summary by CodeRabbit