fix(phase4): scope macOS acceptance to the runner's own user - #74
Merged
Conversation
The macOS acceptance detected 1Helm processes and listening ports machine-wide, which is unsafe on a shared multi-user Mac. If any other account is running 1Helm, `pgrep -x 1Helm` matches that foreign process, so the "app fully quit" gates fail the job even though the runner's own app exited. Worse, the health-probe port discovery used `lsof -c 1Helm` without a user filter, so it could read another account's listening 1Helm port and probe that instance instead of the one under test. Scope both to the runner's own user: pgrep gains `-U "$(id -u)"` and the lsof port probe gains `-u "$(id -un)"`. On a dedicated single-user host this is a no-op; on a shared host it makes the lane correct and prevents it from observing or waiting on another user's 1Helm. No release, tag, version bump, website deploy, or production change. Co-Authored-By: Claude <noreply@anthropic.com>
📝 WalkthroughWalkthroughmacOS acceptance checks now limit process discovery, port lookups, health checks, and shutdown waits to the dedicated account’s user ID. ChangesmacOS acceptance checks
Estimated code review effort: 2 (Simple) | ~10 minutes 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 78-79: Update the process checks in the wait loops and final
checks at the three indicated locations to treat only pgrep exit status 1 as
“1Helm” absent; propagate every other non-zero status, including errors, instead
of converting them to success or failure based solely on command negation.
🪄 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: 7551296f-30d4-43d4-8665-043440cad30f
📒 Files selected for processing (1)
ops/platform-acceptance/macos.sh
Comment on lines
+78
to
+79
| 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 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
file="ops/platform-acceptance/macos.sh"
printf '%s\n' '--- target file ---'
sed -n '1,180p' "$file"
printf '%s\n' '--- pgrep usage ---'
rg -n -C 4 'pgrep|1Helm' "$file"
printf '%s\n' '--- repository references ---'
rg -n -C 3 'shutdown|platform-acceptance|macos\.sh|pgrep -x -U' . --glob '!node_modules' --glob '!dist' --glob '!build' | head -300Repository: gitcommit90/1Helm
Length of output: 40067
🏁 Script executed:
#!/bin/bash
set -eu
file="ops/platform-acceptance/macos.sh"
sed -n '1,180p' "$file"
rg -n -C 4 'pgrep|1Helm' "$file"Repository: gitcommit90/1Helm
Length of output: 16362
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
text = Path("ops/platform-acceptance/macos.sh").read_text()
gates = []
lines = text.splitlines()
for i, line in enumerate(lines, 1):
if 'for _ in {1..30}; do pgrep -x -U' in line:
gates.append((i, i + 1, lines[i-1:i+1]))
print("shutdown_gates=", len(gates))
for start, end, block in gates:
print(f"{start}-{end}:")
print("\n".join(block))
PY
tmp="$(mktemp -d)"
trap 'rm -rf "$tmp"' EXIT
cat >"$tmp/pgrep" <<'SH'
#!/bin/sh
case "${FAKE_PGREP_STATUS:-1}" in
0) exit 0 ;;
1) exit 1 ;;
2) printf '%s\n' 'synthetic pgrep failure' >&2; exit 2 ;;
*) exit "$FAKE_PGREP_STATUS" ;;
esac
SH
chmod +x "$tmp/pgrep"
cat >"$tmp/probe.sh" <<'SH'
#!/usr/bin/env bash
set -euo pipefail
for _ in {1..1}; do pgrep -x -U "$(id -u)" 1Helm >/dev/null || break; sleep 1; done
! pgrep -x -U "$(id -u)" 1Helm >/dev/null
printf '%s\n' "completed"
SH
chmod +x "$tmp/probe.sh"
for status in 0 1 2; do
printf 'existing_status=%s: ' "$status"
if PATH="$tmp:$PATH" FAKE_PGREP_STATUS="$status" "$tmp/probe.sh" >/tmp/probe.out 2>/tmp/probe.err; then
printf 'exit=0 '
else
printf 'exit=%s ' "$?"
fi
printf 'stdout=%s stderr=%s\n' "$(tr '\n' '|' </tmp/probe.out)" "$(tr '\n' '|' </tmp/probe.err)"
done
cat >"$tmp/strict_probe.sh" <<'SH'
#!/usr/bin/env bash
set -euo pipefail
for _ in {1..1}; do
if pgrep -x -U "$(id -u)" 1Helm >/dev/null; then
sleep 0
else
status=$?
case "$status" in
1) break ;;
*) exit "$status" ;;
esac
fi
done
if pgrep -x -U "$(id -u)" 1Helm >/dev/null; then
exit 1
else
status=$?
case "$status" in
1) ;;
*) exit "$status" ;;
esac
fi
printf '%s\n' "completed"
SH
chmod +x "$tmp/strict_probe.sh"
for status in 0 1 2; do
printf 'strict_status=%s: ' "$status"
if PATH="$tmp:$PATH" FAKE_PGREP_STATUS="$status" "$tmp/strict_probe.sh" >/tmp/strict.out 2>/tmp/strict.err; then
printf 'exit=0 '
else
printf 'exit=%s ' "$?"
fi
printf 'stdout=%s stderr=%s\n' "$(tr '\n' '|' </tmp/strict.out)" "$(tr '\n' '|' </tmp/strict.err)"
doneRepository: gitcommit90/1Helm
Length of output: 937
🌐 Web query:
macOS pgrep man page exit status 0 1 2 3
💡 Result:
On macOS, the pgrep utility uses the following exit status codes [1][2]: 0: One or more processes were matched [1][2]. 1: No processes were matched [1][2]. 2: Invalid options were specified on the command line [1][2]. 3: An internal error occurred [1][2]. These codes are consistent with the BSD implementation of pgrep found on macOS [1][3][2]. While Linux implementations of pgrep (part of procps) often use the same exit codes for the same conditions, they may include additional context in their documentation regarding pkill (e.g., specifying that a signal must be successfully sent) [4][5]. On macOS, pgrep's exit status is commonly used in shell scripts to control flow, such as checking if a process is running before executing a command [6].
Citations:
- 1: https://www.unix.com/man_page/osx/1/pgrep/
- 2: https://ss64.com/mac/pkill.html
- 3: https://man.freebsd.org/cgi/man.cgi?query=pgrep&sektion=1
- 4: https://man7.org/linux/man-pages/man1/pgrep.1.html
- 5: https://manpages.debian.org/bullseye/procps/pgrep.1.en.html
- 6: https://unix.stackexchange.com/questions/507290/why-cant-i-get-pgrep-output-right-to-variable-on-bash-script
🏁 Script executed:
#!/bin/bash
set -eu
tmp="$(mktemp -d)"
trap 'rm -rf "$tmp"' EXIT
cat >"$tmp/pgrep" <<'SH'
#!/bin/sh
exit "${FAKE_PGREP_STATUS:-1}"
SH
chmod +x "$tmp/pgrep"
cat >"$tmp/probe.sh" <<'SH'
#!/usr/bin/env bash
set -euo pipefail
for _ in {1..1}; do
pgrep -x -U "$(id -u)" 1Helm >/dev/null || break
sleep 1
done
! pgrep -x -U "$(id -u)" 1Helm >/dev/null
printf '%s\n' completed
SH
chmod +x "$tmp/probe.sh"
for status in 0 1 2 3; do
if PATH="$tmp:$PATH" FAKE_PGREP_STATUS="$status" "$tmp/probe.sh" >/tmp/probe.out 2>/tmp/probe.err; then
result=0
else
result=$?
fi
printf 'status=%s exit=%s output=%s\n' "$status" "$result" "$(tr '\n' '|' </tmp/probe.out)"
doneRepository: gitcommit90/1Helm
Length of output: 291
Fail closed on pgrep errors.
Treat only status 1 as “1Helm is absent”. Propagate all other non-zero statuses from the wait loops and final checks. Apply this to lines 78–79, 132–133, and 147–148.
🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 79-79: 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 78 - 79, Update the process
checks in the wait loops and final checks at the three indicated locations to
treat only pgrep exit status 1 as “1Helm” absent; propagate every other non-zero
status, including errors, instead of converting them to success or failure based
solely on command negation.
Source: Linters/SAST tools
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.
Why
The macOS acceptance script detects 1Helm processes and listening ports machine-wide. That is unsafe on a Mac shared by more than one account (the intended acceptance host has another account running a live 1Helm):
pgrep -x 1Helmmatches a 1Helm process in any account. The "app fully quit" gates (! pgrep -x 1Helm) then fail the job whenever another user is running 1Helm, even though the runner's own app exited correctly.lsof -c 1Helmwith no user filter, so it could pick up another account's listening 1Helm port and run the health probe against that instance instead of the one under test.Neither touches the other account's data (separate home, file permissions), but both make the lane incorrect/flaky on a shared machine and let it observe a foreign instance.
Change
Scope all process/port detection to the runner's own user:
pgrep -x 1Helm→pgrep -x -U "$(id -u)" 1Helm(3 gates, 6 lines)lsof -nP -a -c 1Helm ...→lsof -nP -a -u "$(id -un)" -c 1Helm ...(3 probes)On a dedicated single-user host this is a no-op. On a shared host it makes the lane observe only its own account.
Verification
node --test test/phase4-platform-acceptance.mjs— 8/8 passbash -n ops/platform-acceptance/macos.sh— cleanpgrep/lsofreferencesScope
No release, tag, version bump, website deploy, or production change. Does not enable the Mac lane (
HELM_PHASE4_MACOS_ENABLEDstays unset).🤖 Generated with Claude Code
Summary by CodeRabbit