fix(phase4): finish Mac and Windows acceptance - #80
Conversation
The Mac app exposes multiple loopback listeners. Acceptance exited on the first lsof miss under set -e and, once a listener appeared, probed only the first port even when it was not the setup-health API. Poll every candidate listener safely until one returns setup health, and retain the exact failing line in the job log. The Windows script used shell-style escaping inside PowerShell strings, which executed systemctl on Windows after a successful WSL install. Replace all four copies with one literal, semver-bounded WSL assertion helper. Add focused contract assertions for both regressions. The full real Mac flow was also exercised on the dedicated helm-ci account: clean install, prior Stable, candidate updater, loopback health, and state retention all passed. Co-Authored-By: Claude <noreply@anthropic.com>
…otstrap The Windows acceptance host bootstrap existed only as a pre-built ISO on the hypervisor, so it could not be reviewed, reproduced, or corrected. Track it and fix two defects found while provisioning a fresh host. setup.ps1 installed the VirtIO network driver but never the VirtIO serial driver. The QEMU guest agent then starts and reports Running while the host side (qm agent <vmid> ping) stays dead, leaving no guest-exec channel to provision through. setup.ps1 also skipped firewall setup whenever an OpenSSH-Server-In-TCP rule already existed. Add-WindowsCapability creates that rule scoped to the Private profile only, and a freshly bridged VM is categorized Public, so port 22 stayed unreachable even though sshd reported Running. The operator public key is no longer embedded. This repository is public, so publishing which key is authorized as Administrator on the acceptance host is needless disclosure; it is supplied at build time and staged onto the media. Also records two host constraints that are easy to get wrong: Proxmox always creates the TPM state volume as raw, and a raw volume on directory storage blocks snapshots for the whole VM, which the acceptance lane depends on for restoring its accepted clean baseline; and Windows 11 25H2 no longer auto-applies an answer file from secondary media. No release, tag, version bump, website deploy, or production change. Co-Authored-By: Claude <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe PR adds retained Windows 11 acceptance-host provisioning, including unattended installation, bootstrap ISO creation, SSH setup, readiness signaling, and deployment documentation. It also improves macOS health polling and Windows WSL version validation, with corresponding acceptance-test assertions. ChangesPlatform acceptance
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant BootstrapISO
participant WindowsHost
participant AcceptanceChecks
Operator->>BootstrapISO: Build ONEHELM bootstrap media
BootstrapISO->>WindowsHost: Install Windows and run setup.ps1
WindowsHost-->>Operator: Write SSH and readiness configuration
AcceptanceChecks->>WindowsHost: Launch and validate platform services
WindowsHost-->>AcceptanceChecks: Return health and version status
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
test/phase4-platform-acceptance.mjs (1)
213-215: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a regression check for service-test failure propagation.
These assertions prove four call sites and reject one old invocation pattern. They do not prove that
Assert-DistroVersionfails whensystemctl is-activefails. Add a check forset -eor&&, or execute a focused fixture where the service test fails and the version test succeeds.🤖 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/phase4-platform-acceptance.mjs` around lines 213 - 215, Extend the acceptance assertions around Assert-DistroVersion to verify service-test failure propagation: require the generated script to use set -e or && when invoking systemctl, or add a focused fixture where the service test fails while the version test succeeds and assert the overall failure. Keep the existing call-site count and obsolete-pattern checks unchanged.
🤖 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 `@ops/platform-acceptance/macos.sh`:
- Around line 141-149: Update the 1Helm process-wait logic in the macOS
acceptance flow to use an explicit conditional after the polling loop: if pgrep
still finds the process, emit an error and exit nonzero before removing or
replacing the installed app. Preserve the existing wait loop and continue only
when 1Helm has stopped.
- Around line 35-50: Update wait_for_setup_health to apply finite curl
connection and total-request timeouts for each port probe, and enforce an
absolute deadline across the polling loop so an unresponsive listener cannot
block beyond the function’s overall timeout. Preserve the existing
successful-response handling and temporary-file cleanup.
In `@ops/platform-acceptance/windows.ps1`:
- Around line 35-47: Update the bash command constructed in Assert-DistroVersion
so a failure in either the service-status or package-version assertion causes
Invoke-Distro to fail. Add fail-fast behavior or explicitly chain the two test
commands, while preserving execution of both assertions inside WSL.
In `@test/phase4-platform-acceptance.mjs`:
- Around line 125-128: Update the macOS assertions in the phase4 acceptance test
to scope the pipeline check to the relevant process-substitution block rather
than matching any later `|| true`. Within that anchored block, assert the
expected outputs clean-health.json, prior-health.json, and update-health.json,
while preserving the existing wait_for_setup_health and awk-related checks.
---
Nitpick comments:
In `@test/phase4-platform-acceptance.mjs`:
- Around line 213-215: Extend the acceptance assertions around
Assert-DistroVersion to verify service-test failure propagation: require the
generated script to use set -e or && when invoking systemctl, or add a focused
fixture where the service test fails while the version test succeeds and assert
the overall failure. Keep the existing call-site count and obsolete-pattern
checks unchanged.
🪄 Autofix
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: 5c5900a2-3ff3-4ae4-bd40-78e9b8af5e9b
📒 Files selected for processing (9)
docs/phase4-platform-acceptance.mdops/platform-acceptance/macos.shops/platform-acceptance/windows-host/.gitignoreops/platform-acceptance/windows-host/Autounattend.xmlops/platform-acceptance/windows-host/README.mdops/platform-acceptance/windows-host/build-unattend-iso.shops/platform-acceptance/windows-host/setup.ps1ops/platform-acceptance/windows.ps1test/phase4-platform-acceptance.mjs
| wait_for_setup_health() { | ||
| local output="$1" port | ||
| for _ in {1..180}; do | ||
| while IFS= read -r port; do | ||
| [[ "$port" =~ ^[0-9]+$ ]] || continue | ||
| if curl -fsS "http://127.0.0.1:$port/api/setup/status" >"$output.tmp"; then | ||
| mv "$output.tmp" "$output" | ||
| return 0 | ||
| fi | ||
| rm -f -- "$output.tmp" | ||
| done < <(lsof -nP -a -u "$(id -un)" -c 1Helm -iTCP -sTCP:LISTEN 2>/dev/null \ | ||
| | awk '/127\.0\.0\.1:/ {split($9,a,":"); print a[length(a)]}' || true) | ||
| sleep 1 | ||
| done | ||
| return 1 | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- target file ---'
cat -n ops/platform-acceptance/macos.sh | sed -n '1,90p'
printf '%s\n' '--- function usages ---'
rg -n -C 3 'wait_for_setup_health|setup/status|curl ' ops/platform-acceptance --glob '*.sh' || true
printf '%s\n' '--- shell metadata ---'
head -n 5 ops/platform-acceptance/macos.shRepository: gitcommit90/1Helm
Length of output: 12552
🏁 Script executed:
#!/bin/bash
set -eu
tmp="$(mktemp -d)"
trap 'rm -rf "$tmp"' EXIT
cat >"$tmp/lsof" <<'EOF'
#!/bin/sh
printf '1Helm 123 user 10u IPv4 0x0 0t0 TCP 127.0.0.1:45678 (LISTEN)\n'
EOF
cat >"$tmp/curl" <<'EOF'
#!/bin/sh
sleep 10
EOF
chmod +x "$tmp/lsof" "$tmp/curl"
cat >"$tmp/probe.sh" <<'EOF'
#!/usr/bin/env bash
set -euo pipefail
wait_for_setup_health() {
local output="$1" port
for _ in {1..180}; do
while IFS= read -r port; do
[[ "$port" =~ ^[0-9]+$ ]] || continue
if curl -fsS "http://127.0.0.1:$port/api/setup/status" >"$output.tmp"; then
mv "$output.tmp" "$output"
return 0
fi
rm -f -- "$output.tmp"
done < <(lsof -nP -a -u "$(id -un)" -c 1Helm -iTCP -sTCP:LISTEN 2>/dev/null \
| awk '/127\.0\.0\.1:/ {split($9,a,":"); print a[length(a)]}' || true)
sleep 1
done
return 1
}
wait_for_setup_health "$1"
EOF
chmod +x "$tmp/probe.sh"
start="$(date +%s)"
set +e
PATH="$tmp:$PATH" timeout 3 "$tmp/probe.sh" "$tmp/output"
status=$?
elapsed=$(( $(date +%s) - start ))
set -e
printf 'status=%s elapsed_seconds=%s\n' "$status" "$elapsed"
test "$status" -eq 124
test "$elapsed" -lt 5
printf '%s\n' 'A stalled curl prevents the function from reaching its polling loop timeout.'Repository: gitcommit90/1Helm
Length of output: 226
🏁 Script executed:
#!/bin/bash
set -eu
tmp="$(mktemp -d)"
trap 'rm -rf "$tmp"' EXIT
cat >"$tmp/lsof" <<'EOF'
#!/bin/sh
printf '1Helm 123 user 10u IPv4 0x0 0t0 TCP 127.0.0.1:45678 (LISTEN)\n'
EOF
cat >"$tmp/curl" <<'EOF'
#!/bin/sh
sleep 10
EOF
chmod +x "$tmp/lsof" "$tmp/curl"
cat >"$tmp/probe.sh" <<'EOF'
#!/usr/bin/env bash
set -euo pipefail
wait_for_setup_health() {
local output="$1" port
for _ in {1..180}; do
while IFS= read -r port; do
[[ "$port" =~ ^[0-9]+$ ]] || continue
if curl -fsS "http://127.0.0.1:$port/api/setup/status" >"$output.tmp"; then
mv "$output.tmp" "$output"
return 0
fi
rm -f -- "$output.tmp"
done < <(lsof -nP -a -u "$(id -un)" -c 1Helm -iTCP -sTCP:LISTEN 2>/dev/null \
| awk '/127\.0\.0\.1:/ {split($9,a,":"); print a[length(a)]}' || true)
sleep 1
done
return 1
}
wait_for_setup_health "$1"
EOF
chmod +x "$tmp/probe.sh"
set +e
PATH="$tmp:$PATH" "$tmp/probe.sh" "$tmp/output" &
pid=$!
sleep 1
if kill -0 "$pid" 2>/dev/null; then
kill "$pid" 2>/dev/null || true
wait "$pid" 2>/dev/null || true
set -e
printf '%s\n' 'The function remains blocked in curl after one second.'
exit 0
fi
wait "$pid"
status=$?
set -e
printf 'Unexpected completion status: %s\n' "$status"
exit 1Repository: gitcommit90/1Helm
Length of output: 263
Bound each health probe and enforce the polling deadline.
Line 40 calls curl without --connect-timeout or --max-time. A listening but unresponsive process can block indefinitely and prevent the function from reaching its timeout. Use a finite per-port timeout and an absolute deadline.
🤖 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 `@ops/platform-acceptance/macos.sh` around lines 35 - 50, Update
wait_for_setup_health to apply finite curl connection and total-request timeouts
for each port probe, and enforce an absolute deadline across the polling loop so
an unresponsive listener cannot block beyond the function’s overall timeout.
Preserve the existing successful-response handling and temporary-file cleanup.
| wait_for_setup_health "$work/prior-health.json" | ||
| osascript -e 'tell application id "com.gitcommit90.1helm" to quit' || true | ||
| for _ in {1..30}; do pgrep -x -U "$(id -u)" 1Helm >/dev/null || break; sleep 1; done | ||
| ! pgrep -x -U "$(id -u)" 1Helm >/dev/null | ||
| rm -rf -- "$installed" | ||
| ditto "$work/update/1Helm.app" "$installed" | ||
| [[ "$(defaults read "$installed/Contents/Info" CFBundleShortVersionString)" == "$VERSION" ]] | ||
| open -n "$installed" --args --1helm-background | ||
| for _ in {1..180}; do | ||
| PORT="$(lsof -nP -a -u "$(id -un)" -c 1Helm -iTCP -sTCP:LISTEN 2>/dev/null | awk '/127\.0\.0\.1:/ {split($9,a,":"); print a[length(a)]; exit}')" | ||
| [[ "$PORT" =~ ^[0-9]+$ ]] && curl -fsS "http://127.0.0.1:$PORT/api/setup/status" >"$work/update-health.json" && break | ||
| sleep 1 | ||
| done | ||
| [[ -s "$work/update-health.json" ]] | ||
| wait_for_setup_health "$work/update-health.json" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- script header and relevant section ---'
sed -n '1,35p;125,160p' ops/platform-acceptance/macos.sh
printf '%s\n' '--- shell options and related process checks ---'
rg -n 'set -|errexit|pgrep|wait_for_setup_health|1Helm' ops/platform-acceptance/macos.sh
printf '%s\n' '--- shellcheck availability and targeted diagnostic ---'
if command -v shellcheck >/dev/null 2>&1; then
shellcheck --version
shellcheck --shell=bash --enable=all ops/platform-acceptance/macos.sh 2>&1 | sed -n '1,120p'
else
echo 'shellcheck: unavailable'
fiRepository: gitcommit90/1Helm
Length of output: 13139
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- wait_for_setup_health implementation ---'
sed -n '35,60p' ops/platform-acceptance/macos.sh
printf '%s\n' '--- standalone errexit probe ---'
bash -c '
set -euo pipefail
trap '\''status=$?; printf "ERR trap: status=%s command=%s\n" "$status" "$BASH_COMMAND" >&2'\'' ERR
pgrep() { return 0; }
printf "before\n"
! pgrep -x -U "$(id -u)" 1Helm >/dev/null
printf "after negated check: status=%s\n" "$?"
printf "replacement would run\n"
'Repository: gitcommit90/1Helm
Length of output: 1277
Fail when the 1Helm process is still running.
When pgrep finds 1Helm, Bash does not apply errexit to the negated command. The script then replaces the app while the old process is still running. Use an explicit conditional that exits with an error.
🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 144-144: This ! is not on a condition and skips errexit. Use && exit 1 instead, or make sure $? is checked.
(SC2251)
🤖 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 `@ops/platform-acceptance/macos.sh` around lines 141 - 149, Update the 1Helm
process-wait logic in the macOS acceptance flow to use an explicit conditional
after the polling loop: if pgrep still finds the process, emit an error and exit
nonzero before removing or replacing the installed app. Preserve the existing
wait loop and continue only when 1Helm has stopped.
Source: Linters/SAST tools
| function Assert-DistroVersion([string] $ExpectedVersion) { | ||
| if ($ExpectedVersion -notmatch '^\d+\.\d+\.\d+$') { Refuse 'expected distribution version is invalid' } | ||
| # A single-quoted here-string is literal PowerShell text. The previous calls | ||
| # used backslash-escaped double quotes (shell syntax, not PowerShell syntax), | ||
| # which ended the PowerShell string and executed `systemctl` on Windows. | ||
| # Substitute only the already-validated semver after constructing the exact | ||
| # bash command, so every service/version assertion really runs inside WSL. | ||
| $command = @' | ||
| test "$(systemctl is-active 1helm.service)" = active | ||
| test "$(node -p 'require("/opt/1helm/current/package.json").version')" = '__EXPECTED__' | ||
| '@ | ||
| Invoke-Distro ($command.Replace('__EXPECTED__', $ExpectedVersion)) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '1,90p' ops/platform-acceptance/windows.ps1
printf '\n--- Invoke-Distro references ---\n'
rg -n -C 5 'function Invoke-Distro|Invoke-Distro|Assert-DistroVersion' ops/platform-acceptance/windows.ps1 opsRepository: gitcommit90/1Helm
Length of output: 22250
🏁 Script executed:
python3 - <<'PY'
import subprocess
script = r'''set -e
test "$(systemctl is-active 1helm.service)" = active
test "$(node -p 'require("/opt/1helm/current/package.json").version')" = '__EXPECTED__'
'''
for service_output, version_output in [('inactive', '__EXPECTED__'), ('active', 'wrong')]:
cmd = script.replace('systemctl is-active 1helm.service', f"printf '%s\\n' {service_output}")
cmd = cmd.replace("node -p 'require(\"/opt/1helm/current/package.json\").version'", f"printf '%s\\n' {version_output}")
result = subprocess.run(['/bin/bash', '-lc', cmd], capture_output=True, text=True)
print(f"service={service_output!r}, version={version_output!r}, exit={result.returncode}")
PYRepository: gitcommit90/1Helm
Length of output: 248
🏁 Script executed:
python3 - <<'PY'
import subprocess
base = r'''
systemctl() { printf '%s\n' "$SYSTEMCTL_RESULT"; }
node() { printf '%s\n' "$NODE_RESULT"; }
test "$(systemctl is-active 1helm.service)" = active
test "$(node -p 'require("/opt/1helm/current/package.json").version')" = "$EXPECTED_VERSION"
'''
for label, errexit, service, version in [
("without set -e", "", "inactive", "1.2.3"),
("with set -e", "set -e\n", "inactive", "1.2.3"),
("without set -e, both pass", "", "active", "1.2.3"),
]:
script = errexit + base
result = subprocess.run(
["/bin/bash", "-lc", script],
env={"SYSTEMCTL_RESULT": service, "NODE_RESULT": version,
"EXPECTED_VERSION": "1.2.3"},
capture_output=True, text=True,
)
print(f"{label}: exit={result.returncode}")
PYRepository: gitcommit90/1Helm
Length of output: 232
Propagate failures from both WSL assertions.
If the service test fails but the version test passes, the final test returns zero and Invoke-Distro accepts the command. Add set -e or chain the tests with &&.
🤖 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 `@ops/platform-acceptance/windows.ps1` around lines 35 - 47, Update the bash
command constructed in Assert-DistroVersion so a failure in either the
service-status or package-version assertion causes Invoke-Distro to fail. Add
fail-fast behavior or explicitly chain the two test commands, while preserving
execution of both assertions inside WSL.
| const macAccept = read("ops/platform-acceptance/macos.sh"); | ||
| assert.match(macAccept, /wait_for_setup_health/); | ||
| assert.match(macAccept, /lsof[\s\S]*awk[\s\S]*\|\| true/); | ||
| assert.doesNotMatch(macAccept, /print a\[length\(a\)\]; exit/); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- acceptance test context ---'
sed -n '95,145p' test/phase4-platform-acceptance.mjs
printf '%s\n' '--- macOS acceptance script ---'
sed -n '1,240p' ops/platform-acceptance/macos.sh
printf '%s\n' '--- related assertions and call sites ---'
rg -n -C 3 'wait_for_setup_health|health|lsof|process substitution|macAccept' \
test/phase4-platform-acceptance.mjs ops/platform-acceptance/macos.shRepository: gitcommit90/1Helm
Length of output: 16882
🏁 Script executed:
#!/bin/bash
set -eu
node - <<'JS'
const fs = require("fs");
const source = fs.readFileSync("ops/platform-acceptance/macos.sh", "utf8");
const helperPattern = /wait_for_setup_health/;
const pipelinePattern = /lsof[\s\S]*awk[\s\S]*\|\| true/;
const callPattern = /wait_for_setup_health\s+"([^"]+)"/g;
const calls = [...source.matchAll(callPattern)].map((m) => m[1]);
console.log("actual helper assertion:", helperPattern.test(source));
console.log("actual pipeline assertion:", pipelinePattern.test(source));
console.log("actual health outputs:", JSON.stringify(calls));
const withoutPipelineGuard = source.replace(
/(\| awk[\s\S]*?print a\[length\(a\)\]\}) \|\| true/,
"$1"
).replace(
/(! pgrep[\s\S]*?\n)/,
"$1true || true\n"
);
console.log("mutant without pipeline guard plus later || true:", pipelinePattern.test(withoutPipelineGuard));
const helperOnly = source
.replace(/\nwait_for_setup_health "\$work\/[^"]+"\n/g, "\n")
.replace(/wait_for_setup_health\(\) \{/, "wait_for_setup_health() {");
console.log("helper-only mutant passes helper assertion:", helperPattern.test(helperOnly));
console.log("helper-only mutant health outputs:", JSON.stringify([...helperOnly.matchAll(callPattern)].map((m) => m[1])));
JSRepository: gitcommit90/1Helm
Length of output: 463
Make the macOS regression checks path-specific.
Anchor the pipeline assertion to the same process-substitution block so it cannot match an unrelated later || true. Assert the three expected outputs: clean-health.json, prior-health.json, and update-health.json.
🤖 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/phase4-platform-acceptance.mjs` around lines 125 - 128, Update the macOS
assertions in the phase4 acceptance test to scope the pipeline check to the
relevant process-substitution block rather than matching any later `|| true`.
Within that anchored block, assert the expected outputs clean-health.json,
prior-health.json, and update-health.json, while preserving the existing
wait_for_setup_health and awk-related checks.
What changed
systemctlin PowerShell;lsofmiss does not abort underset -e, and every 1Helm listener is probed until the setup-health endpoint responds;Verification
npm run ci: 177 tests, 175 passed, 2 skipped, 0 failed;helm-ciaccount passed all nine checks: signature, notarization, staple, Gatekeeper, clean install, prior-version update, retained state, loopback health, and version;helm-ciapp/data cleanup verified; Lillian's live 1Helm account remained running and untouched.No version bump, tag, release, Stable promotion, website deploy, or production change.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests