Fix fresh Linux connector and Windows reboot setup - #50
Merged
Conversation
📝 WalkthroughWalkthroughThis release updates connector discovery, Windows WSL setup recovery, installation metadata, and version references for 0.0.34. Tests cover Linux connector resolution, WSL restart and import handling, and packaged installation paths. ChangesRuntime and release updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Installer as install-wsl-runtime.ps1
participant Windows as Windows features
participant VMCompute as vmcompute
participant WSL as WSL runtime import
Installer->>Windows: enable required features
Windows-->>Installer: feature state and reboot requirement
Installer->>VMCompute: check service availability
VMCompute-->>Installer: available or restart-required error
Installer->>WSL: import runtime into marked directory
WSL-->>Installer: import result
Installer->>Installer: remove marker after successful import
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 |
gitcommit90
force-pushed
the
fix/fresh-platform-runtime-round2
branch
from
August 1, 2026 19:56
4de7695 to
e8bb0bf
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
test/connectors.mjs (1)
23-46: 🎯 Functional Correctness | 🔵 Trivial | 🏗️ Heavy liftMake the assertion prove working-directory resolution.
connectorAvailable()returnstruefor any existing candidate. The test clears environment overrides but leavesPATHand fixed system paths active. A host-installedcloudflaredcan therefore satisfy the assertion without selectingroot/resources/cloudflared-linux-${process.arch}.Assert the selected path, or execute the fixture and verify a marker from that binary while external candidates are isolated.
🤖 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/connectors.mjs` around lines 23 - 46, The test around connectorAvailable() must prove it selected the fixture under root/resources rather than any host-installed cloudflared candidate. Isolate external candidates such as PATH and fixed system locations, then assert the resolved connector path equals binary or execute that fixture and verify its marker; retain the existing environment and temporary-directory cleanup.
🤖 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 `@src/server/connectors.ts`:
- Around line 19-22: Update the appRoots initialization near connectorAvailable
to read process.cwd() inside a try/catch, omitting it when the working directory
is unavailable or throws ENOENT. Preserve HELM_APP_ROOT and the existing
deduplication/filtering behavior so configured and system paths remain usable.
In `@test/connectors.mjs`:
- Around line 23-46: Add an early platform/architecture guard to this test so it
runs only on Linux with process.arch equal to x64 or arm64. Place the guard
before mkdtemp, environment mutations, process.chdir, and other process-wide
setup; leave the existing fixture and assertion unchanged for supported runners.
In `@test/desktop.mjs`:
- Line 199: Update the runtime assertion in the desktop test to validate the
restart-routing call sites directly: add separate assertions for the
default-version failure branch and the import failure branch, ensuring each
routes to Require-WindowsRestart rather than merely matching its later
declaration. Replace the broad combined regular expression while preserving the
existing restart-required behavior being tested.
---
Nitpick comments:
In `@test/connectors.mjs`:
- Around line 23-46: The test around connectorAvailable() must prove it selected
the fixture under root/resources rather than any host-installed cloudflared
candidate. Isolate external candidates such as PATH and fixed system locations,
then assert the resolved connector path equals binary or execute that fixture
and verify its marker; retain the existing environment and temporary-directory
cleanup.
🪄 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: 0ecb5156-a85f-4862-a930-d5d5bdb794ca
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
CHANGELOG.mdREADME.mdpackage.jsonscripts/install-wsl-runtime.ps1site/public/install-linux-units.shsrc/server/channel-computers.tssrc/server/connectors.tssrc/server/db.tstest/channel-computers.mjstest/connectors.mjstest/desktop.mjstest/site.mjs
Comment on lines
+19
to
+22
| // Linux systemd releases run with /opt/1helm/current as their working | ||
| // directory. Keep that installed-root contract usable even if an older unit | ||
| // omitted HELM_APP_ROOT, while preferring the explicit packaged root. | ||
| const appRoots = [...new Set([process.env.HELM_APP_ROOT || "", process.cwd()].filter(Boolean))]; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- src/server/connectors.ts outline ---'
ast-grep outline src/server/connectors.ts --view expanded
printf '%s\n' '--- relevant source ---'
sed -n '1,180p' src/server/connectors.ts
printf '%s\n' '--- connectorBinary references ---'
rg -n -C 3 'connectorBinary|process\.cwd\(|HELM_APP_ROOT' srcRepository: gitcommit90/1Helm
Length of output: 17107
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- process.cwd behavior after directory removal ---'
node - <<'JS'
const fs = require("node:fs");
const os = require("node:os");
const path = require("node:path");
const parent = fs.mkdtempSync(path.join(os.tmpdir(), "cwd-check-"));
const removed = path.join(parent, "removed");
fs.mkdirSync(removed);
process.chdir(removed);
fs.rmSync(removed, { recursive: true });
try {
process.cwd();
console.log("process.cwd: returned");
} catch (error) {
console.log(`process.cwd: threw ${error.code}: ${error.message}`);
}
process.chdir(parent);
fs.rmSync(parent, { recursive: true });
JS
printf '%s\n' '--- connector availability and launch call paths ---'
rg -n -C 5 'connectorAvailable\(|startTunnelConnector\(|startConfiguredConnectors\(' srcRepository: gitcommit90/1Helm
Length of output: 9771
Guard process.cwd() before building candidates.
If the current directory was removed, process.cwd() throws ENOENT. This prevents connectorAvailable() and connector launches from checking configured or system paths. Read it in a try/catch and omit it when unavailable.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn, type ChildProcess } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 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 `@src/server/connectors.ts` around lines 19 - 22, Update the appRoots
initialization near connectorAvailable to read process.cwd() inside a try/catch,
omitting it when the working directory is unavailable or throws ENOENT. Preserve
HELM_APP_ROOT and the existing deduplication/filtering behavior so configured
and system paths remain usable.
Comment on lines
+23
to
+46
| test("Linux service working directory resolves the bundled connector without a legacy app-root environment", async (t) => { | ||
| const root = await mkdtemp(join(tmpdir(), "1helm-linux-connector-root-")); | ||
| const resources = join(root, "resources"); | ||
| const binary = join(resources, `cloudflared-linux-${process.arch}`); | ||
| const originalCwd = process.cwd(); | ||
| const originalAppRoot = process.env.HELM_APP_ROOT; | ||
| const originalResources = process.env.HELM_RESOURCES_PATH; | ||
| const originalBinary = process.env.CLOUDFLARED_BIN; | ||
| await mkdir(resources); | ||
| await writeFile(binary, "#!/bin/sh\nexit 0\n"); | ||
| await chmod(binary, 0o755); | ||
| process.chdir(root); | ||
| delete process.env.HELM_APP_ROOT; | ||
| delete process.env.HELM_RESOURCES_PATH; | ||
| delete process.env.CLOUDFLARED_BIN; | ||
| t.after(async () => { | ||
| process.chdir(originalCwd); | ||
| if (originalAppRoot === undefined) delete process.env.HELM_APP_ROOT; else process.env.HELM_APP_ROOT = originalAppRoot; | ||
| if (originalResources === undefined) delete process.env.HELM_RESOURCES_PATH; else process.env.HELM_RESOURCES_PATH = originalResources; | ||
| if (originalBinary === undefined) delete process.env.CLOUDFLARED_BIN; else process.env.CLOUDFLARED_BIN = originalBinary; | ||
| await rm(root, { recursive: true, force: true }); | ||
| }); | ||
| const connectors = await import(`../src/server/connectors.ts?linux-root-test=${Date.now()}`); | ||
| assert.equal(connectors.connectorAvailable(), true); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate files ---'
git ls-files | rg '(^|/)(connectors\.mjs|connectors\.ts|package\.json|vitest|jest|node:test|test)' | head -200
printf '%s\n' '--- test file outline ---'
ast-grep outline test/connectors.mjs --lang javascript 2>/dev/null || true
printf '%s\n' '--- relevant test section ---'
cat -n test/connectors.mjs | sed -n '1,90p'
printf '%s\n' '--- connector resolver symbols ---'
rg -n -C 8 'connectorBinary|linuxConnector|connectorAvailable|HELM_APP_ROOT|HELM_RESOURCES_PATH|CLOUDFLARED_BIN|process\.cwd|PATH' src test package.jsonRepository: gitcommit90/1Helm
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- connector resolver ---'
cat -n src/server/connectors.ts | sed -n '1,75p'
printf '%s\n' '--- package test commands ---'
cat -n package.json | sed -n '1,140p'
printf '%s\n' '--- connector test references and platform guards ---'
rg -n -C 3 'test\(|process\.platform|t\.skip|PATH|HELM_RESOURCES_PATH|CLOUDFLARED_BIN' test/connectors.mjsRepository: gitcommit90/1Helm
Length of output: 13666
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- test suite runner ---'
cat -n scripts/run-test-suite.mjs | sed -n '1,180p'
printf '%s\n' '--- resolver behavior probe ---'
python3 - <<'PY'
from pathlib import Path
text = Path("src/server/connectors.ts").read_text()
start = text.index("function connectorBinary()")
end = text.index("\n\nconst safeId", start)
print(text[start:end])
PY
node - <<'JS'
const path = require('node:path');
function candidates({platform, arch, env, cwd}) {
const join = path.posix.join;
const resources = env.HELM_RESOURCES_PATH || "";
const appRoots = [...new Set([env.HELM_APP_ROOT || "", cwd].filter(Boolean))];
const pathSep = platform === "win32" ? ";" : ":";
const pathNames = platform === "win32" ? ["cloudflared.exe", "cloudflared"] : ["cloudflared"];
const linuxConnector = platform === "linux" && (arch === "x64" || arch === "arm64")
? `cloudflared-linux-${arch}` : "";
const pathCandidates = String(env.PATH || "").split(pathSep).filter(Boolean)
.flatMap(directory => pathNames.map(name => join(directory, name)));
return [
env.CLOUDFLARED_BIN || "",
resources ? join(resources, "cloudflared.exe") : "",
resources ? join(resources, "cloudflared") : "",
...appRoots.flatMap(appRoot => [
join(appRoot, "cloudflared.exe"),
join(appRoot, "cloudflared"),
linuxConnector ? join(appRoot, "resources", linuxConnector) : "",
]),
"/opt/homebrew/bin/cloudflared", "/usr/local/bin/cloudflared",
"/usr/bin/cloudflared", ...pathCandidates,
].filter(Boolean);
}
for (const [platform, arch] of [["linux","x64"],["linux","arm64"],["linux","ia32"],["darwin","arm64"],["win32","x64"]]) {
const list = candidates({platform, arch, env:{}, cwd:"/tmp/root"});
console.log(platform, arch, list.includes(`/tmp/root/resources/cloudflared-linux-${arch}`), list);
}
JSRepository: gitcommit90/1Helm
Length of output: 6660
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- workflow platforms and connector test invocation ---'
rg -n -C 3 'npm test|test:|runs-on:|node --test|connectors\.mjs' .github site scripts 2>/dev/null | head -240 || true
printf '%s\n' '--- node test skip API availability ---'
node - <<'JS'
import test from "node:test";
test("skip-api-probe", (t) => {
console.log({hasSkip: typeof t.skip === "function"});
t.skip("probe");
});
JSRepository: gitcommit90/1Helm
Length of output: 2766
Skip this Linux-only fixture on unsupported runners.
connectorBinary() adds the Linux fixture only on Linux x64 and arm64. On other runners, the assertion can fail or pass because of an unrelated system binary. Add the guard before mkdtemp and process-wide state changes.
Proposed fix
test("Linux service working directory resolves the bundled connector without a legacy app-root environment", async (t) => {
+ if (process.platform !== "linux" || (process.arch !== "x64" && process.arch !== "arm64")) {
+ t.skip("Linux connector fixture requires a supported Linux architecture");
+ return;
+ }
const root = await mkdtemp(join(tmpdir(), "1helm-linux-connector-root-"));📝 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
| test("Linux service working directory resolves the bundled connector without a legacy app-root environment", async (t) => { | |
| const root = await mkdtemp(join(tmpdir(), "1helm-linux-connector-root-")); | |
| const resources = join(root, "resources"); | |
| const binary = join(resources, `cloudflared-linux-${process.arch}`); | |
| const originalCwd = process.cwd(); | |
| const originalAppRoot = process.env.HELM_APP_ROOT; | |
| const originalResources = process.env.HELM_RESOURCES_PATH; | |
| const originalBinary = process.env.CLOUDFLARED_BIN; | |
| await mkdir(resources); | |
| await writeFile(binary, "#!/bin/sh\nexit 0\n"); | |
| await chmod(binary, 0o755); | |
| process.chdir(root); | |
| delete process.env.HELM_APP_ROOT; | |
| delete process.env.HELM_RESOURCES_PATH; | |
| delete process.env.CLOUDFLARED_BIN; | |
| t.after(async () => { | |
| process.chdir(originalCwd); | |
| if (originalAppRoot === undefined) delete process.env.HELM_APP_ROOT; else process.env.HELM_APP_ROOT = originalAppRoot; | |
| if (originalResources === undefined) delete process.env.HELM_RESOURCES_PATH; else process.env.HELM_RESOURCES_PATH = originalResources; | |
| if (originalBinary === undefined) delete process.env.CLOUDFLARED_BIN; else process.env.CLOUDFLARED_BIN = originalBinary; | |
| await rm(root, { recursive: true, force: true }); | |
| }); | |
| const connectors = await import(`../src/server/connectors.ts?linux-root-test=${Date.now()}`); | |
| assert.equal(connectors.connectorAvailable(), true); | |
| test("Linux service working directory resolves the bundled connector without a legacy app-root environment", async (t) => { | |
| if (process.platform !== "linux" || (process.arch !== "x64" && process.arch !== "arm64")) { | |
| t.skip("Linux connector fixture requires a supported Linux architecture"); | |
| return; | |
| } | |
| const root = await mkdtemp(join(tmpdir(), "1helm-linux-connector-root-")); | |
| const resources = join(root, "resources"); | |
| const binary = join(resources, `cloudflared-linux-${process.arch}`); | |
| const originalCwd = process.cwd(); | |
| const originalAppRoot = process.env.HELM_APP_ROOT; | |
| const originalResources = process.env.HELM_RESOURCES_PATH; | |
| const originalBinary = process.env.CLOUDFLARED_BIN; | |
| await mkdir(resources); | |
| await writeFile(binary, "#!/bin/sh\nexit 0\n"); | |
| await chmod(binary, 0o755); | |
| process.chdir(root); | |
| delete process.env.HELM_APP_ROOT; | |
| delete process.env.HELM_RESOURCES_PATH; | |
| delete process.env.CLOUDFLARED_BIN; | |
| t.after(async () => { | |
| process.chdir(originalCwd); | |
| if (originalAppRoot === undefined) delete process.env.HELM_APP_ROOT; else process.env.HELM_APP_ROOT = originalAppRoot; | |
| if (originalResources === undefined) delete process.env.HELM_RESOURCES_PATH; else process.env.HELM_RESOURCES_PATH = originalResources; | |
| if (originalBinary === undefined) delete process.env.CLOUDFLARED_BIN; else process.env.CLOUDFLARED_BIN = originalBinary; | |
| await rm(root, { recursive: true, force: true }); | |
| }); | |
| const connectors = await import(`../src/server/connectors.ts?linux-root-test=${Date.now()}`); | |
| assert.equal(connectors.connectorAvailable(), true); |
🤖 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/connectors.mjs` around lines 23 - 46, Add an early platform/architecture
guard to this test so it runs only on Linux with process.arch equal to x64 or
arm64. Place the guard before mkdtemp, environment mutations, process.chdir, and
other process-wide setup; leave the existing fixture and assertion unchanged for
supported runners.
| assert.match(windowsRuntime, /1603[\s\S]*Test-PinnedWslRuntime|Test-PinnedWslRuntime[\s\S]*1603/, "MSI 1603 falls back to re-verifying an already-present pinned WSL runtime"); | ||
| assert.match(windowsRuntime, /\$enabledWslFeatureNow[\s\S]*\$enabledVmFeatureNow[\s\S]*\$restartRequired/, "features enabled in the current pass force a reboot before WSL import regardless of DISM enum formatting"); | ||
| assert.match(windowsRuntime, /Get-Service -Name vmcompute[\s\S]*\$restartRequired = \$true/, "an enabled-but-not-registered WSL VM compute service stops setup at the reboot boundary"); | ||
| assert.match(windowsRuntime, /HCS_E_SERVICE_NOT_AVAILABLE[\s\S]*Require-WindowsRestart|Test-WslRestartFailure[\s\S]*Require-WindowsRestart/, "an unavailable VM compute service is reported as restart-required instead of a broken runtime"); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Target the restart-routing call sites.
The expression can match Test-WslRestartFailure and the following Require-WindowsRestart function declaration. It does not verify that either failure branch routes to the restart handler. Assert the default-version and import branches separately.
Proposed test update
- assert.match(windowsRuntime, /HCS_E_SERVICE_NOT_AVAILABLE[\s\S]*Require-WindowsRestart|Test-WslRestartFailure[\s\S]*Require-WindowsRestart/, "an unavailable VM compute service is reported as restart-required instead of a broken runtime");
+ assert.match(windowsRuntime, /if \(Test-WslRestartFailure \$defaultVersion\.Text\) \{ Require-WindowsRestart \}/, "a default-version HCS failure requires a Windows restart");
+ assert.match(windowsRuntime, /if \(Test-WslRestartFailure \$imported\.Text\) \{ Require-WindowsRestart \}/, "an import HCS failure requires a Windows restart");📝 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
| assert.match(windowsRuntime, /HCS_E_SERVICE_NOT_AVAILABLE[\s\S]*Require-WindowsRestart|Test-WslRestartFailure[\s\S]*Require-WindowsRestart/, "an unavailable VM compute service is reported as restart-required instead of a broken runtime"); | |
| assert.match(windowsRuntime, /if \(Test-WslRestartFailure \$defaultVersion\.Text\) \{ Require-WindowsRestart \}/, "a default-version HCS failure requires a Windows restart"); | |
| assert.match(windowsRuntime, /if \(Test-WslRestartFailure \$imported\.Text\) \{ Require-WindowsRestart \}/, "an import HCS failure requires a Windows restart"); |
🤖 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/desktop.mjs` at line 199, Update the runtime assertion in the desktop
test to validate the restart-routing call sites directly: add separate
assertions for the default-version failure branch and the import failure branch,
ensuring each routes to Require-WindowsRestart rather than merely matching its
later declaration. Replace the broad combined regular expression while
preserving the existing restart-required behavior being tested.
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.
Outcome\n\n- exposes the active Linux release root to the systemd service and resolves the bundled architecture-specific Cloudflare connector from the installed working directory\n- stops fresh Windows WSL setup at the required reboot boundary whenever features were just enabled or VM compute has not registered\n- maps HCS service-unavailable import failures to restart-required and safely retries app-owned partial imports\n- bumps the synchronized desktop candidate train to 0.0.34\n\n## Verification\n\n- npm run typecheck\n- npm run build\n- npm run test:onboarding-browser (20 passed)\n- npm test (126 native-world + 115 extended; 2 environment skips, 0 failures)\n- focused connector/site/desktop/channel-computer tests\n- git diff --check
Summary by CodeRabbit
Bug Fixes
Release