docs(fpm): setup guide + packages reference, gated by e2e suite#26
Open
Fahl-Design wants to merge 4 commits intomainfrom
Open
docs(fpm): setup guide + packages reference, gated by e2e suite#26Fahl-Design wants to merge 4 commits intomainfrom
Fahl-Design wants to merge 4 commits intomainfrom
Conversation
Add Packages table covering cli/fpm/micro with links to upstream static-php.dev SAPI Reference, and a Running PHP-FPM section with a minimal config, foreground run, systemd user unit, launchd plist, and nginx FastCGI block. Closes #25.
📝 WalkthroughWalkthroughAdds an end-to-end PHP‑FPM test suite (helpers, runner, 15 cases), a CI e2e-fpm job that runs the suite and gates release, a Docker sandbox and e2e README, plus README documentation describing packaged PHP binaries and PHP‑FPM setup. ChangesE2E FPM + CI + README
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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 `@README.md`:
- Around line 99-101: Add a fenced code block language tag to the snippet
containing "$PVM_DIR/versions/<full-semver>/bin/php-fpm" so it no longer
triggers markdownlint MD040; change the fence from ``` to ```text (or another
appropriate language label) around that path-only block in README.md.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Adds tests/e2e/fpm-readme.sh and a new e2e-fpm job in release.yml chained between tests and release, so the README docs cannot drift from the static-php-cli FPM tarball pvm distributes without breaking CI. The script downloads the latest static fpm tarball directly (bypassing pvm's interactive MultiSelect), drops it into the pvm versions layout, and verifies: - pvm ls discovers the version - the documented php-fpm.conf and pool.d/www.conf pass php-fpm -t - php-fpm -F listens on 127.0.0.1:9000 with the documented worker count - php-fpm -c custom.ini -y conf -t works - php-fpm -v / -m emit the expected output The release job now needs [tests, e2e-fpm], so a README change that breaks the documented flow blocks the release. Verified locally in a fresh ubuntu:24.04 container against both the install.sh path and a from-source build of pvm.
Refactor the monolithic fpm-readme.sh into: tests/e2e/run.sh driver tests/e2e/_lib.sh shared helpers (run_under_expect, fcgi_call) tests/e2e/cases/NN_*.sh one file per case The driver runs each case in a fresh bash subprocess, so state from one case can no longer hide bugs in the next (the previous monolith silently passed because pvm's 24h .update_check_guard suppressed the patch-update prompt for any case after the first one to use it). Coverage now includes (Linux, ubuntu-latest): 01 real `pvm install` flow with interactive MultiSelect (expect) 02 pvm ls discovers [cli, fpm] 03 pvm use through shell wrapper switches PATH; which php-fpm 04 pvm current after pvm use 05 .php-version cd-hook auto-switch 06 pvm use <missing-version> → install prompt + decline (#24) 07 patch-update detection on pvm use <minor> 08 README php-fpm.conf + pool.d/{www,sock}.conf passes -t 09 fpm -F listens on TCP :9000 + unix /tmp/php-fpm-www.sock 10 FastCGI roundtrip over TCP returns FCGI_OK 11 FastCGI roundtrip over unix socket returns FCGI_OK 12 -c php.ini effective inside worker (memory_limit, expose_php) 13 pid file + error log written 14 SIGQUIT clean shutdown 15 pvm uninstall removes the version directory CI job in release.yml renamed to "e2e tests - linux", chained between the existing `tests` and `release` jobs. apt-get installs expect and libfcgi-bin (for cgi-fcgi) on top of netcat-openbsd. The suite targets PHP 8.5 by default (PVM_VERSION_MAJOR_MINOR=8.5), overridable per env.
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (4)
tests/e2e/_lib.sh (1)
19-19: 💤 Low valueUnquoted
$scriptin tclspawn— breaks on paths with spaces.
$scriptis expanded by bash into the unquoted heredoc and lands directly in the tclspawncommand. Tcl splits that line on whitespace, so any path containing a space would be tokenised into multiple arguments.♻️ Proposed fix — brace-quote the path for tcl
-spawn bash --norc --noprofile $script +spawn bash --norc --noprofile {$script}Using tcl braces prevents word-splitting on the expanded path while still preserving the bash-substituted value.
🤖 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 `@tests/e2e/_lib.sh` at line 19, The tcl spawn invocation uses an unquoted shell-expanded variable ($script) which allows tcl to split paths with spaces; update the heredoc containing the line with the spawn command so the expanded path is wrapped in tcl braces to prevent word-splitting (i.e., change the spawn invocation that currently uses $script to use a braced form around the expanded variable), ensuring you target the spawn command and the $script variable in the heredoc.tests/e2e/cases/06_use_missing.sh (1)
11-11: 💤 Low valueSame unquoted
spawnfragility as_lib.sh:19—$PVM_BINwith spaces breaks tcl tokenisation.-spawn $PVM_BIN use $MISSING_VER +spawn {$PVM_BIN} use $MISSING_VER🤖 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 `@tests/e2e/cases/06_use_missing.sh` at line 11, The spawn invocation "spawn $PVM_BIN use $MISSING_VER" is fragile when $PVM_BIN contains spaces; update the test to quote the variables so tcl tokenisation treats each as a single argument, e.g. change the line to use quoted variables (spawn "$PVM_BIN" use "$MISSING_VER") so $PVM_BIN and $MISSING_VER cannot be split; apply the same quoting pattern used for other spawn calls referenced in the repo.tests/e2e/cases/14_fpm_shutdown.sh (1)
18-18: 💤 Low valueOptional: compute the elapsed time rather than printing a literal arithmetic expression.
${i}*100msrenders as e.g."after 7*100ms"rather than"after 700ms".- ok "fpm master $FPM_PID exited cleanly (after ${i}*100ms)" + ok "fpm master $FPM_PID exited cleanly (after $(( i * 100 ))ms)"🤖 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 `@tests/e2e/cases/14_fpm_shutdown.sh` at line 18, The test prints a literal arithmetic expression "after ${i}*100ms" instead of the computed elapsed time; update the assertion that currently reads ok "fpm master $FPM_PID exited cleanly (after ${i}*100ms)" to compute the elapsed milliseconds (e.g. using shell arithmetic $((i*100)) into a variable like elapsed or inline $((i*100))) and then format the message as "after ${elapsed}ms" (or "after $((i*100))ms") so the output shows the actual number of milliseconds.tests/e2e/run.sh (1)
52-56: ⚡ Quick winUse
jqinstead ofgrep/sedfor JSON parsing.The multi-step
grep -oE | sedpipeline is sensitive to JSON formatting, key order, and CDN-level minification; any change silently produces an empty result and trips thefailon line 59.jqis idiomatic, already available on GitHub-hosted runners, and should also be added to the prerequisite check on line 17.♻️ Proposed refactor
Add
jqto the tool check:-for tool in curl tar nc expect cgi-fcgi pgrep; do +for tool in curl tar nc expect cgi-fcgi pgrep jq; doReplace the grep/sed pipeline:
-ALL_VERS=$(echo "$INDEX_JSON" \ - | grep -oE "\"php-${ESCAPED_VFILTER}\\.[0-9]+-cli-${TGT}\\.tar\\.gz\"" \ - | sed -E "s|\"php-(${ESCAPED_VFILTER}\\.[0-9]+)-cli-${TGT}\\.tar\\.gz\"|\\1|" \ - | sort -V -u) +ALL_VERS=$(echo "$INDEX_JSON" \ + | jq -r 'keys[]' \ + | grep -E "^php-${ESCAPED_VFILTER}\.[0-9]+-cli-${TGT}\.tar\.gz$" \ + | sed -E "s|^php-(${ESCAPED_VFILTER}\.[0-9]+)-cli-${TGT}\.tar\.gz$|\1|" \ + | sort -V -u)🤖 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 `@tests/e2e/run.sh` around lines 52 - 56, Replace the fragile grep/sed JSON parsing pipeline by using jq to extract matching filenames from the INDEX_JSON variable and populate ALL_VERS; specifically parse INDEX_JSON with jq to select keys/values that match the pattern using the ESCAPED_VFILTER and TGT variables and output the unique sorted versions into ALL_VERS, and also add jq to the prerequisite/tool check near the script header where other tools are validated so the runner fails fast if jq is missing.
🤖 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 `@tests/e2e/_lib.sh`:
- Around line 35-43: fcgi_call currently creates a temporary directory in
script_dir and runs cgi-fcgi but if cgi-fcgi exits non-zero under callers' set
-e the final rm -rf "$script_dir" is never executed; add an unconditional
cleanup trap (e.g., trap 'rm -rf "$script_dir"' EXIT) immediately after creating
script_dir and before invoking cgi-fcgi so the temp dir is removed on any exit,
and then clear the trap (or rely on scope) after cleanup if needed; update the
code around the fcgi_call function and the script_dir creation/cleanup logic to
use this trap.
In `@tests/e2e/cases/02_ls.sh`:
- Around line 9-11: The test uses the A && ok || warn pattern which misfires if
ok returns non-zero; change the control flow to an explicit if statement: run
"$PVM_BIN" ls piped to grep -E "$PREVIOUS.*cli.*fpm" and, if that command
succeeds call ok "pvm ls shows [cli, fpm] for $PREVIOUS", otherwise call warn
"pvm ls did not show both [cli, fpm] tags"; replace the chained &&/|| expression
with that if/then/else structure so warn only runs when the grep actually fails.
In `@tests/e2e/cases/07_patch_update.sh`:
- Around line 11-13: Replace the hardcoded removal of
"$HOME/.local/share/pvm/.update_check_guard" with a PVM_DIR-aware path so tests
use the configured state directory; update the rm -f invocation that touches
".update_check_guard" to use parameter expansion like
"${PVM_DIR:-$HOME/.local/share/pvm}/.update_check_guard" (referencing the
existing rm -f command and the .update_check_guard filename) so it respects
PVM_DIR when set and falls back to the original HOME path otherwise.
In `@tests/e2e/cases/08_fpm_config.sh`:
- Around line 20-25: Replace the hardcoded FastCGI listen address in the
www.conf writer with a parameter so tests can pick an available port: change the
line that writes "listen = 127.0.0.1:9000" to use an environment/config variable
(e.g., FPM_LISTEN or FPM_PORT) and default fallback (e.g.,
"${FPM_LISTEN:-127.0.0.1:9000}"); ensure the test setup code (the script that
exports USER_NAME and creates "$HOME/.config/php-fpm/pool.d/www.conf") exports
that variable or computes a free port before writing so the same address is used
by any subsequent FastCGI client steps.
In `@tests/e2e/cases/09_fpm_run.sh`:
- Around line 36-37: Replace the conditional using "&& ... || ..." with an
explicit if/else to avoid the OK-command's exit status triggering the else
branch; check the WORKER_COUNT comparison (the "[[ \"$WORKER_COUNT\" -ge 2 ]]"
test) and call the ok function when true and warn when false, ensuring the
script will correctly propagate failures from ok or warn under set -euo pipefail
instead of masking them.
In `@tests/e2e/cases/15_uninstall.sh`:
- Around line 11-13: Replace the unsafe `[[ ... ]] && ok ... || fail ...`
pattern with an explicit if/then/else so that `fail` only runs when the test
itself fails and not when `ok` returns non-zero; locate the `[[ ! -d
"$HOME/.local/share/pvm/versions/$PREVIOUS" ]]` check in
tests/e2e/cases/15_uninstall.sh and change the control flow to `if [[ ... ]];
then ok "versions/$PREVIOUS directory removed"; else fail "uninstall left
versions/$PREVIOUS in place"; fi` so the test result is decided solely by the
directory check and not by `ok`'s exit status.
In `@tests/e2e/run.sh`:
- Line 33: The two curl invocations that download install.sh lack timeout flags
and can hang CI; update the curl commands (the lines calling "curl -fsSL
https://raw.githubusercontent.com/WebProject-xyz/php-version-manager/main/install.sh
| bash" and the other curl at line 52) to include sensible timeout options, e.g.
add --connect-timeout 10 and --max-time 60 (or project-preferred values) so the
commands fail fast on network issues and prevent indefinite hanging.
- Around line 33-35: Replace the unpinned "curl | bash" install step by
downloading a specific commit tarball/installer using a full commit SHA (not the
floating main ref), saving it to a temporary file, verifying its integrity
(compare SHA256 checksum or verify a GPG signature) before execution, and only
then running the installer; update the PVM_BIN assignment (symbol PVM_BIN) and
the success check (ok "installed via install.sh") to run after verification and
install succeed, and ensure the script exits non-zero if checksum/signature
validation fails.
---
Nitpick comments:
In `@tests/e2e/_lib.sh`:
- Line 19: The tcl spawn invocation uses an unquoted shell-expanded variable
($script) which allows tcl to split paths with spaces; update the heredoc
containing the line with the spawn command so the expanded path is wrapped in
tcl braces to prevent word-splitting (i.e., change the spawn invocation that
currently uses $script to use a braced form around the expanded variable),
ensuring you target the spawn command and the $script variable in the heredoc.
In `@tests/e2e/cases/06_use_missing.sh`:
- Line 11: The spawn invocation "spawn $PVM_BIN use $MISSING_VER" is fragile
when $PVM_BIN contains spaces; update the test to quote the variables so tcl
tokenisation treats each as a single argument, e.g. change the line to use
quoted variables (spawn "$PVM_BIN" use "$MISSING_VER") so $PVM_BIN and
$MISSING_VER cannot be split; apply the same quoting pattern used for other
spawn calls referenced in the repo.
In `@tests/e2e/cases/14_fpm_shutdown.sh`:
- Line 18: The test prints a literal arithmetic expression "after ${i}*100ms"
instead of the computed elapsed time; update the assertion that currently reads
ok "fpm master $FPM_PID exited cleanly (after ${i}*100ms)" to compute the
elapsed milliseconds (e.g. using shell arithmetic $((i*100)) into a variable
like elapsed or inline $((i*100))) and then format the message as "after
${elapsed}ms" (or "after $((i*100))ms") so the output shows the actual number of
milliseconds.
In `@tests/e2e/run.sh`:
- Around line 52-56: Replace the fragile grep/sed JSON parsing pipeline by using
jq to extract matching filenames from the INDEX_JSON variable and populate
ALL_VERS; specifically parse INDEX_JSON with jq to select keys/values that match
the pattern using the ESCAPED_VFILTER and TGT variables and output the unique
sorted versions into ALL_VERS, and also add jq to the prerequisite/tool check
near the script header where other tools are validated so the runner fails fast
if jq is missing.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 04d070db-db83-46c6-ba25-1c4ea9405ee3
📒 Files selected for processing (18)
.github/workflows/release.ymltests/e2e/_lib.shtests/e2e/cases/01_install.shtests/e2e/cases/02_ls.shtests/e2e/cases/03_use_wrapper.shtests/e2e/cases/04_current.shtests/e2e/cases/05_php_version_hook.shtests/e2e/cases/06_use_missing.shtests/e2e/cases/07_patch_update.shtests/e2e/cases/08_fpm_config.shtests/e2e/cases/09_fpm_run.shtests/e2e/cases/10_fcgi_tcp.shtests/e2e/cases/11_fcgi_sock.shtests/e2e/cases/12_php_ini_effective.shtests/e2e/cases/13_pid_log.shtests/e2e/cases/14_fpm_shutdown.shtests/e2e/cases/15_uninstall.shtests/e2e/run.sh
Comment on lines
+33
to
+35
| curl -fsSL https://raw.githubusercontent.com/WebProject-xyz/php-version-manager/main/install.sh | bash | ||
| PVM_BIN="$HOME/.local/share/pvm/bin/pvm" | ||
| ok "installed via install.sh" |
There was a problem hiding this comment.
Unpinned curl | bash is a supply-chain risk.
Downloading and executing from a floating main ref means any commit pushed to that branch (or a transient CDN compromise) runs arbitrary code in CI. At minimum, pin to a full commit SHA and verify a checksum or GPG signature before execution.
🔒 Proposed fix (pin to a commit SHA + checksum)
- curl -fsSL https://raw.githubusercontent.com/WebProject-xyz/php-version-manager/main/install.sh | bash
+ INSTALL_SHA="<full-40-char-commit-sha>" # pin to a reviewed release commit
+ curl -fsSL "https://raw.githubusercontent.com/WebProject-xyz/php-version-manager/${INSTALL_SHA}/install.sh" \
+ -o /tmp/pvm_install.sh
+ echo "<known-sha256> /tmp/pvm_install.sh" | sha256sum -c -
+ bash /tmp/pvm_install.sh🤖 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 `@tests/e2e/run.sh` around lines 33 - 35, Replace the unpinned "curl | bash"
install step by downloading a specific commit tarball/installer using a full
commit SHA (not the floating main ref), saving it to a temporary file, verifying
its integrity (compare SHA256 checksum or verify a GPG signature) before
execution, and only then running the installer; update the PVM_BIN assignment
(symbol PVM_BIN) and the success check (ok "installed via install.sh") to run
after verification and install succeed, and ensure the script exits non-zero if
checksum/signature validation fails.
Fahl-Design
changed the title
Address CodeRabbit review feedback on PR #26 and harden the suite for local use. Sandboxing: * Add e2e_require_sandbox helper in _lib.sh that refuses to run unless /.dockerenv exists, GITHUB_ACTIONS=true, /proc/1/cgroup looks like a container, or PVM_E2E_FORCE=1 is set. Driver calls it first, so a curious developer cannot accidentally clobber their real $HOME/.local/share/pvm by running tests/e2e/run.sh on the host. * Ship tests/e2e/Dockerfile (ubuntu:24.04 + curl + expect + libfcgi-bin + netcat-openbsd) so contributors can `docker build -t pvm-e2e tests/e2e` and reproduce the CI sandbox locally. * Add tests/e2e/README.md walking through layout, the safety check, the local docker workflow, and the env-var overrides. Review fixups (PR #26): * README.md: add `text` language tag to the path-only fenced block (markdownlint MD040). * _lib.sh fcgi_call: trap RETURN to clean the temp dir even when cgi-fcgi exits non-zero under the caller's set -e. * run.sh: add --connect-timeout/--max-time on both curl invocations so a slow upstream cannot hang CI indefinitely. * run.sh + cases 09/10/12: parameterize the FPM TCP address via FPM_TCP_ADDR (default 127.0.0.1:9000) so a port collision is fixable by env, not a script edit. * cases 02/09/15: replace the SC2015 `A && ok || warn|fail` pattern with explicit if/else so the secondary branch only fires when the condition is false, not when ok itself happens to exit non-zero. * cases 07 + 15: read the pvm state dir from ${PVM_DIR:-…} instead of hardcoding $HOME/.local/share/pvm, so a user-customised PVM_DIR is honoured.
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
tests/e2e/run.sh (1)
37-40:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPin and verify the installer before execution.
Line 38 still executes remote code from a floating
mainref. That allows unreviewed upstream changes to run in CI.🔒 Proposed hardening
- curl -fsSL --connect-timeout 10 --max-time 60 \ - https://raw.githubusercontent.com/WebProject-xyz/php-version-manager/main/install.sh | bash + INSTALL_SHA="<full-40-char-commit-sha>" + INSTALL_URL="https://raw.githubusercontent.com/WebProject-xyz/php-version-manager/${INSTALL_SHA}/install.sh" + INSTALL_PATH="/tmp/pvm_install.sh" + curl -fsSL --connect-timeout 10 --max-time 60 "$INSTALL_URL" -o "$INSTALL_PATH" + echo "<sha256> $INSTALL_PATH" | sha256sum -c - + bash "$INSTALL_PATH"#!/bin/bash # Verify whether a floating-branch installer is still used. rg -n -C2 'raw\.githubusercontent\.com/.*/php-version-manager/main/install\.sh|\|\s*bash' tests/e2e/run.sh🤖 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 `@tests/e2e/run.sh` around lines 37 - 40, Replace the current curl | bash call that fetches the floating main installer with a pinned-and-verified fetch: download the install.sh from a specific commit SHA or tag instead of "main", verify its integrity (e.g., compare a shipped checksum or verify a GPG signature), only then execute it, and keep the existing PVM_BIN assignment (PVM_BIN="$HOME/.local/share/pvm/bin/pvm") and ok "installed via install.sh" behavior; in short, change the URL used in the curl invocation to a fixed commit/tag and add a verification step that aborts if the signature/checksum does not match before piping to bash.
🤖 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 `@tests/e2e/cases/08_fpm_config.sh`:
- Around line 59-61: The test uses the unsafe "A && ok || fail" pattern so fail
may run if ok returns non-zero; change the command check to an explicit
conditional: run the php-fpm syntax test ("$VDIR/php-fpm" -y
"$HOME/.config/php-fpm/php-fpm.conf" -t) and use an if/then/else to call ok
"php-fpm -t OK" on success and fail "php-fpm -t failed" on failure (or
alternately use command || fail directly), replacing the current A && ok || fail
pattern referencing the php-fpm invocation and the ok and fail helpers.
In `@tests/e2e/cases/09_fpm_run.sh`:
- Around line 38-39: The WORKER_COUNT assignment uses the pipeline "pgrep -P
\"$FPM_PID\" 2>/dev/null | wc -l || echo 0" which, under set -euo pipefail, can
append a second "0" when pgrep fails; change the fallback to a no-output true so
the numeric result from wc -l is preserved (i.e., ensure the pipeline uses "||
true" instead of "|| echo 0") so WORKER_COUNT contains only the single-line
count for the subsequent numeric comparison.
---
Duplicate comments:
In `@tests/e2e/run.sh`:
- Around line 37-40: Replace the current curl | bash call that fetches the
floating main installer with a pinned-and-verified fetch: download the
install.sh from a specific commit SHA or tag instead of "main", verify its
integrity (e.g., compare a shipped checksum or verify a GPG signature), only
then execute it, and keep the existing PVM_BIN assignment
(PVM_BIN="$HOME/.local/share/pvm/bin/pvm") and ok "installed via install.sh"
behavior; in short, change the URL used in the curl invocation to a fixed
commit/tag and add a verification step that aborts if the signature/checksum
does not match before piping to bash.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 54deee6b-09cf-4137-9d4c-b995d387e84b
📒 Files selected for processing (12)
README.mdtests/e2e/Dockerfiletests/e2e/README.mdtests/e2e/_lib.shtests/e2e/cases/02_ls.shtests/e2e/cases/07_patch_update.shtests/e2e/cases/08_fpm_config.shtests/e2e/cases/09_fpm_run.shtests/e2e/cases/10_fcgi_tcp.shtests/e2e/cases/12_php_ini_effective.shtests/e2e/cases/15_uninstall.shtests/e2e/run.sh
✅ Files skipped from review due to trivial changes (2)
- tests/e2e/Dockerfile
- README.md
Comment on lines
+59
to
+61
| "$VDIR/php-fpm" -y "$HOME/.config/php-fpm/php-fpm.conf" -t \ | ||
| && ok "php-fpm -t OK" \ | ||
| || fail "php-fpm -t failed" |
There was a problem hiding this comment.
SC2015: remaining A && B || C instance — fail can fire when ok exits non-zero.
This is the same pattern that was fixed in 02_ls.sh and 15_uninstall.sh in the previous round, but this instance was left unchanged. While ok (echo -e) almost never exits non-zero, the semantics are wrong and ShellCheck flags it (SC2015 line 60).
🛡️ Proposed fix
-"$VDIR/php-fpm" -y "$HOME/.config/php-fpm/php-fpm.conf" -t \
- && ok "php-fpm -t OK" \
- || fail "php-fpm -t failed"
+if "$VDIR/php-fpm" -y "$HOME/.config/php-fpm/php-fpm.conf" -t; then
+ ok "php-fpm -t OK"
+else
+ fail "php-fpm -t failed"
+fi📝 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
| "$VDIR/php-fpm" -y "$HOME/.config/php-fpm/php-fpm.conf" -t \ | |
| && ok "php-fpm -t OK" \ | |
| || fail "php-fpm -t failed" | |
| if "$VDIR/php-fpm" -y "$HOME/.config/php-fpm/php-fpm.conf" -t; then | |
| ok "php-fpm -t OK" | |
| else | |
| fail "php-fpm -t failed" | |
| fi |
🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 60-60: Note that A && B || C is not if-then-else. C may run when A is true.
(SC2015)
🤖 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 `@tests/e2e/cases/08_fpm_config.sh` around lines 59 - 61, The test uses the
unsafe "A && ok || fail" pattern so fail may run if ok returns non-zero; change
the command check to an explicit conditional: run the php-fpm syntax test
("$VDIR/php-fpm" -y "$HOME/.config/php-fpm/php-fpm.conf" -t) and use an
if/then/else to call ok "php-fpm -t OK" on success and fail "php-fpm -t failed"
on failure (or alternately use command || fail directly), replacing the current
A && ok || fail pattern referencing the php-fpm invocation and the ok and fail
helpers.
Comment on lines
+38
to
+39
| WORKER_COUNT=$(pgrep -P "$FPM_PID" 2>/dev/null | wc -l || echo 0) | ||
| if [[ "$WORKER_COUNT" -ge 2 ]]; then |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the fragile fallback pattern is present.
rg -n -C2 'WORKER_COUNT=\$\(pgrep -P "\$FPM_PID" 2>/dev/null \| wc -l \|\| echo 0\)' tests/e2e/cases/09_fpm_run.shRepository: WebProject-xyz/php-version-manager
Length of output: 301
🏁 Script executed:
head -20 tests/e2e/cases/09_fpm_run.shRepository: WebProject-xyz/php-version-manager
Length of output: 637
🏁 Script executed:
cat -n tests/e2e/cases/09_fpm_run.sh | sed -n '36,42p'Repository: WebProject-xyz/php-version-manager
Length of output: 387
🏁 Script executed:
python3 << 'EOF'
import subprocess
# Test the behavior of the problematic pipeline with pipefail
test_script = """
#!/bin/bash
set -euo pipefail
# Simulate pgrep finding no processes by using a PID that won't have children
WORKER_COUNT=$(pgrep -P 99999 2>/dev/null | wc -l || echo 0)
echo "RESULT: [$WORKER_COUNT]"
echo "BYTE_COUNT: $(echo -n "$WORKER_COUNT" | wc -c)"
"""
result = subprocess.run(['bash', '-c', test_script], capture_output=True, text=True)
print("=== Test: pipefail with fallback ===")
print("stdout:", repr(result.stdout))
print("stderr:", repr(result.stderr))
print("exit code:", result.returncode)
# Test with proposed fix: || true
test_script2 = """
#!/bin/bash
set -euo pipefail
WORKER_COUNT=$(pgrep -P 99999 2>/dev/null | wc -l || true)
echo "RESULT: [$WORKER_COUNT]"
echo "BYTE_COUNT: $(echo -n "$WORKER_COUNT" | wc -c)"
"""
result2 = subprocess.run(['bash', '-c', test_script2], capture_output=True, text=True)
print("\n=== Test: pipefail with || true ===")
print("stdout:", repr(result2.stdout))
print("stderr:", repr(result2.stderr))
print("exit code:", result2.returncode)
EOFRepository: WebProject-xyz/php-version-manager
Length of output: 281
Fix pipeline operator under pipefail to avoid multi-line fallback values.
Line 38 produces a multi-line value (0\n0) when pgrep finds no child processes, which makes the numeric comparison on line 39 fragile. With set -euo pipefail enabled, the pipeline pgrep -P "$FPM_PID" 2>/dev/null | wc -l fails due to pgrep's non-zero exit, causing || echo 0 to append a second zero to wc's output.
🛠️ Proposed fix
-WORKER_COUNT=$(pgrep -P "$FPM_PID" 2>/dev/null | wc -l || echo 0)
+WORKER_COUNT=$(pgrep -P "$FPM_PID" 2>/dev/null | wc -l || true)The fix uses || true (which produces no output) instead of || echo 0, ensuring WORKER_COUNT contains only the line count from wc -l.
🤖 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 `@tests/e2e/cases/09_fpm_run.sh` around lines 38 - 39, The WORKER_COUNT
assignment uses the pipeline "pgrep -P \"$FPM_PID\" 2>/dev/null | wc -l || echo
0" which, under set -euo pipefail, can append a second "0" when pgrep fails;
change the fallback to a no-output true so the numeric result from wc -l is
preserved (i.e., ensure the pipeline uses "|| true" instead of "|| echo 0") so
WORKER_COUNT contains only the single-line count for the subsequent numeric
comparison.
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.
Summary
Docs
cli/fpm/microwith links to the upstream static-php.dev SAPI Reference, so users know what each tickbox inpvm installactually delivers.php-fpm.conf+wwwpool, foreground run, systemd user unit, launchd plist, nginx FastCGI block.php.inipath; pin full semver in service units; no fpm multiplexing).E2E test suite (new)
Linux end-to-end tests under
tests/e2e/that lock the README + pvm flows in lockstep with the upstream static-php-cli FPM tarball. Drivertests/e2e/run.shruns eachcases/NN_*.shas a fresh bash subprocess so state from one case cannot hide bugs in the next.Cases (15 total):
01_installrealpvm installwith interactive MultiSelect (driven by expect)02_lspvm lsdiscovers the version with[cli, fpm]tags03_use_wrapperpvm use <minor>through the shell wrapper switchesPATH;which php-fpmresolves under pvm04_currentpvm currentafterpvm use05_php_version_hook.php-versioncd-hook auto-switch06_use_missingpvm use <missing>install prompt + decline (covers fix(use): prompt to install when target version missing #24)07_patch_updatepatch-update detection onpvm use <minor>08_fpm_configREADMEphp-fpm.conf+pool.d/{www,sock}.confpassphp-fpm -t09_fpm_runphp-fpm -Flistens on TCP:9000+ unix/tmp/php-fpm-www.sock10_fcgi_tcpFastCGI roundtrip over TCP returnsFCGI_OK11_fcgi_sockFastCGI roundtrip over unix socket returnsFCGI_OK12_php_ini_effective-c php.iniis effective inside the worker (memory_limit=256M,expose_php=Offviaini_get)13_pid_logpid file + error log written14_fpm_shutdownSIGQUITclean shutdown15_uninstallpvm uninstallremoves the version directoryCI
e2e tests - linuxin.github/workflows/release.yml, chained between the existingtestsjob andreleaseso the README + pvm flows must work for a release to cut.expect+libfcgi-bin(forcgi-fcgi) on top ofnetcat-openbsdonubuntu-latest.8.5by default viaPVM_VERSION_MAJOR_MINOR; overridable per env.Closes #25.
Test plan
tests/e2e/run.shend-to-end on Linux (15/15 cases pass against from-source pvm in ubuntu:24.04)e2e tests - linuxjob green on this PRsystemctl --user enable --now php-fpm) — out of CI scopelaunchctl load ...) — out of CI scopeSummary by CodeRabbit
Documentation
Tests
CI