Keel-Linux/keel-wordpress

Fork 0

keel-wp and keel-wordpress-update, with the turnkey names kept as symlinks #6

Merged

marcos-mendez merged 7 commits from feat/keel-named-commands into master

2026-09-29 00:43:08 -03:00

marcos-mendez commented

2026-09-28 00:08:04 -03:00

(Migrated from github.com)

Closes #7. Org-wide context, cross-reference only: Keel-Linux/tracker#12. The policy it implements is handbook decision 0015 (Keel-Linux/handbook#3).

Merge order: Keel-Linux/handbook#3 first. This branch cites decision 0015 in changelog, COVERAGE.md, tests/wrappers.bats and both script headers, and 0015 is not on the handbook's main yet.

The two commands an operator types on this appliance are written by this overlay, so the name they carry is this project's choice. The Keel name becomes the real command and the turnkey-* name is kept as a symlink to it. Both keep working: somebody who pasted a command out of TurnKey's documentation, or who wrote a script against turnkey-wp last year, must not silently lose it.

overlay/usr/local/bin/keel-wp, with turnkey-wp a symlink to it
overlay/usr/local/sbin/keel-wordpress-update, with turnkey-wordpress-update a symlink to it

Both links are relative. An absolute link does not resolve inside fab-chroot, so conf.d/main would stop finding the command at build time; making either absolute fails three tests here.

What does not change

keel-wp still runs wp-cli through runuser as www-data, never as root, with an explicit --path, and still owns /var/www/.wp-cli. keel-wordpress-update still refuses a caller who is not root, updates core, verifies WordPress's own per file checksums and puts the ownership boundary back. Its refusal now names whichever of its two names was typed, so an operator who ran turnkey-wordpress-update is not told about a command they did not run.

keel-wp gains no new override: its WP_DIR, WP_USR and WP_CACHE are the three it already shipped with, and it calls /usr/local/bin/wp literally as before.

keel-wordpress-update reads its WordPress root, web user and wp-cli path from KEEL_TEST_WPROOT, KEEL_TEST_WP_USER and KEEL_TEST_WP_CLI when set, with the previously hard-coded values as defaults, so a booted appliance behaves identically. The prefix is deliberate — see the review response below. It also now refuses a target that is not a WordPress, by the same wp-includes/version.php that conf.d/main asserts after unpacking.

Responding to the review

Finding	What changed
MEDIUM 1 — the `USER` safeguard was untestable, since under bats `$USER` equals `$(id -un)` and the suite used that as the expected value	The web user in the tests is now the sentinel `keel-test-web-user`, which no account can be called; `ME` is gone. A test runs the updater with `USER=root` and requires `wp-content` to still go to the sentinel. The reviewer's mutation, `USER="${USER:-www-data}"` throughout, now fails three tests.
MEDIUM 2 — `WP_CLI=/bin/true WPROOT=/ keel-wordpress-update` walks the whole filesystem as root	Both fixes the reviewer offered, since either alone leaves half of it. The three overrides are namespaced to `KEEL_TEST_`, the prefix this suite already uses; and the script refuses a target with no `wp-includes/version.php` before it rewrites anything. Un-namespacing `WPROOT` now fails eight tests*; dropping the guard fails one.
MEDIUM 3 — `tests/v19.sh:65` compared two command substitutions, passing as `test "" = ""`	Compares against the literal the script already asserted at line 22. `$before`, the yardstick for the two `core version` comparisons, is asserted non-empty where it is taken.
LOW 1 — `-d` is not what preserves the symlink, and the test could not tell	Corrected in `tests/wrappers.bats`, `COVERAGE.md` and the `changelog`. `-R` copies a symlink as a symlink unless `-L` is given; `-P` is already the default and `-d` only adds `--preserve=links`, which is hard links. A new test asserts the real property: a plain `cp -TR` still yields a working `turnkey-wp`, and `cp -TLR` turns it into a second regular file.
LOW 2 — stale comment	`.github/workflows/tests.yml:20` says eight shell files.
LOW 4 — dead code in the wp stub	The unread `first=` loop is gone.
LOW 5 — two test names claimed more than the tests did	Renamed to "hands back the exit code runuser gave it"; the updater's link test now copies the overlay twice as well.
Description omitted that `keel-wp:15` also gained `WP_CLI`	It no longer has it — the knob was never needed there, because `runuser` is a stub and the command string is never executed. The test asserts the literal `/usr/local/bin/wp` instead.
LOW 3 — four names for two concepts	The updater's are now `KEEL_TEST_` prefixed and read only by the tests; `keel-wp`'s two remain the shipped operator-facing ones. The distinction is now meaningful rather than accidental.

I did not remove the second cp -TdR, which the reviewer flagged as redundant but worth keeping.

How the symlink is made, and why that way

Committed into the overlay, not created by ln -s in conf.d/main. fab-apply-overlay is cp -TdR OVERLAY DEST (cmd_apply_overlay in fab). Measured here with coreutils 9.7:

cp -TR   →  turnkey-wp -> keel-wp     symlink kept, hard link split (nlink 1)
cp -TdR  →  turnkey-wp -> keel-wp     symlink kept, hard link kept  (nlink 2)
cp -TLR  →  turnkey-wp               a second regular file — this one flattens

So the property is the absence of -L. tests/wrappers.bats asserts all three rows, copies the overlay with the build's own command twice (once for COMMON_OVERLAYS, once for the product-local ROOT_OVERLAY) and then runs the copied command.

Call sites

A caller left on the old name still works through the symlink, which is exactly why one is easy to miss: the build would pass either way. They were found by grep -rn, not by the gate.

File	What changed
`conf.d/main`	`keel-wp core verify-checksums --version=...` during the build, and the comment above it
`README.rst`	both places that name the supervised core update
`tests/v19.sh`	every `turnkey-wp` call, and the updater invocation

overlay/usr/lib/inithooks/firstboot.d/40wordpress is named in #7 as a call site, and it is not one: it calls wp-cli directly with --allow-root, never through either wrapper. Nothing there needed changing.

Tests

The 21 original tests were written before the code and watched go red. The four added in response to the review were written the same way, and each was checked against the mutation it exists to catch.

tests/v19.sh keeps everything it drove before — siteurl, post create/get/delete, core version, check-update, verify-checksums and the non-root updater guard — now under the Keel name, and additionally asks the compatibility names for the same answers on a booted appliance, each against a literal.

221 tests, 0 failures

kcov line coverage (threshold 97 percent):
 100.00  21/21  keel-wordpress-update
 100.00  8/8  keel-wp
 100.00  31/31  zz-project-packages
 100.00  26/26  zzz-keel-archive
  99.00  99/100  wordpress.sh
  97.73  43/44  40wordpress
  98.95  282/285  boot-test-lib.sh
 100.00  54/54  keel-archive-check

99.12 percent (564/569) over the eight measured shell files, up from 99.07 percent (535/540) over six on master. Both wrappers were at no coverage at all and are at 100 percent with every branch driven. Threshold stays 97, still set by 40wordpress.

Test plan

bats tests/ — 221 tests, 0 failures
COVERAGE_THRESHOLD=97 tests/coverage.sh — exit 0, numbers above
shellcheck clean on both scripts and tests/coverage.sh
Mutation-checked: the reviewer's USER mutation, un-namespacing WPROOT, and dropping the WordPress guard each fail tests now
appliance / build-and-boot — see below
tests/v19.sh on a rebuilt appliance, where the compatibility names are proved on a real machine

appliance / build-and-boot failed on the previous push for a reason outside this branch: the job boots the layer the mirror already serves, which does not carry this change, and every appliance assertion passed. It failed on apt-get update, where the runner resolves archive.keellinux.org to its own loopback (127.0.1.1) and is served a self-signed certificate. This branch touches neither tests/boot-test.sh nor tests/lib/boot-test-lib.sh.

Closes #7. Org-wide context, cross-reference only: Keel-Linux/tracker#12. The policy it implements is handbook decision 0015 (Keel-Linux/handbook#3). **Merge order: Keel-Linux/handbook#3 first.** This branch cites decision 0015 in `changelog`, `COVERAGE.md`, `tests/wrappers.bats` and both script headers, and 0015 is not on the handbook's `main` yet. The two commands an operator types on this appliance are written by this overlay, so the name they carry is this project's choice. The Keel name becomes the real command and the `turnkey-*` name is kept as a symlink to it. Both keep working: somebody who pasted a command out of TurnKey's documentation, or who wrote a script against `turnkey-wp` last year, must not silently lose it. - `overlay/usr/local/bin/keel-wp`, with `turnkey-wp` a symlink to it - `overlay/usr/local/sbin/keel-wordpress-update`, with `turnkey-wordpress-update` a symlink to it Both links are **relative**. An absolute link does not resolve inside `fab-chroot`, so `conf.d/main` would stop finding the command at build time; making either absolute fails three tests here. ## What does not change `keel-wp` still runs wp-cli through `runuser` as `www-data`, never as root, with an explicit `--path`, and still owns `/var/www/.wp-cli`. `keel-wordpress-update` still refuses a caller who is not root, updates core, verifies WordPress's own per file checksums and puts the ownership boundary back. Its refusal now names whichever of its two names was typed, so an operator who ran `turnkey-wordpress-update` is not told about a command they did not run. `keel-wp` gains **no** new override: its `WP_DIR`, `WP_USR` and `WP_CACHE` are the three it already shipped with, and it calls `/usr/local/bin/wp` literally as before. `keel-wordpress-update` reads its WordPress root, web user and wp-cli path from `KEEL_TEST_WPROOT`, `KEEL_TEST_WP_USER` and `KEEL_TEST_WP_CLI` when set, with the previously hard-coded values as defaults, so a booted appliance behaves identically. The prefix is deliberate — see the review response below. It also now refuses a target that is not a WordPress, by the same `wp-includes/version.php` that `conf.d/main` asserts after unpacking. ## Responding to the review | Finding | What changed | | --- | --- | | **MEDIUM 1** — the `USER` safeguard was untestable, since under bats `$USER` equals `$(id -un)` and the suite used that as the expected value | The web user in the tests is now the sentinel `keel-test-web-user`, which no account can be called; `ME` is gone. A test runs the updater with `USER=root` and requires `wp-content` to still go to the sentinel. The reviewer's mutation, `USER="${USER:-www-data}"` throughout, **now fails three tests**. | | **MEDIUM 2** — `WP_CLI=/bin/true WPROOT=/ keel-wordpress-update` walks the whole filesystem as root | Both fixes the reviewer offered, since either alone leaves half of it. The three overrides are namespaced to `KEEL_TEST_*`, the prefix this suite already uses; and the script refuses a target with no `wp-includes/version.php` before it rewrites anything. Un-namespacing `WPROOT` **now fails eight tests**; dropping the guard fails one. | | **MEDIUM 3** — `tests/v19.sh:65` compared two command substitutions, passing as `test "" = ""` | Compares against the literal the script already asserted at line 22. `$before`, the yardstick for the two `core version` comparisons, is asserted non-empty where it is taken. | | **LOW 1** — `-d` is not what preserves the symlink, and the test could not tell | Corrected in `tests/wrappers.bats`, `COVERAGE.md` and the `changelog`. `-R` copies a symlink as a symlink unless `-L` is given; `-P` is already the default and `-d` only adds `--preserve=links`, which is hard links. A new test asserts the real property: a plain `cp -TR` still yields a working `turnkey-wp`, and `cp -TLR` turns it into a second regular file. | | **LOW 2** — stale comment | `.github/workflows/tests.yml:20` says eight shell files. | | **LOW 4** — dead code in the wp stub | The unread `first=` loop is gone. | | **LOW 5** — two test names claimed more than the tests did | Renamed to "hands back the exit code **runuser** gave it"; the updater's link test now copies the overlay twice as well. | | Description omitted that `keel-wp:15` also gained `WP_CLI` | It no longer has it — the knob was never needed there, because `runuser` is a stub and the command string is never executed. The test asserts the literal `/usr/local/bin/wp` instead. | | **LOW 3** — four names for two concepts | The updater's are now `KEEL_TEST_` prefixed and read only by the tests; `keel-wp`'s two remain the shipped operator-facing ones. The distinction is now meaningful rather than accidental. | I did **not** remove the second `cp -TdR`, which the reviewer flagged as redundant but worth keeping. ## How the symlink is made, and why that way Committed into the overlay, not created by `ln -s` in `conf.d/main`. `fab-apply-overlay` is `cp -TdR OVERLAY DEST` (`cmd_apply_overlay` in fab). Measured here with coreutils 9.7: ``` cp -TR → turnkey-wp -> keel-wp symlink kept, hard link split (nlink 1) cp -TdR → turnkey-wp -> keel-wp symlink kept, hard link kept (nlink 2) cp -TLR → turnkey-wp a second regular file — this one flattens ``` So the property is the absence of `-L`. `tests/wrappers.bats` asserts all three rows, copies the overlay with the build's own command twice (once for `COMMON_OVERLAYS`, once for the product-local `ROOT_OVERLAY`) and then **runs** the copied command. ## Call sites A caller left on the old name still works through the symlink, which is exactly why one is easy to miss: the build would pass either way. They were found by `grep -rn`, not by the gate. | File | What changed | | --- | --- | | `conf.d/main` | `keel-wp core verify-checksums --version=...` during the build, and the comment above it | | `README.rst` | both places that name the supervised core update | | `tests/v19.sh` | every `turnkey-wp` call, and the updater invocation | `overlay/usr/lib/inithooks/firstboot.d/40wordpress` is named in #7 as a call site, and it is not one: it calls wp-cli directly with `--allow-root`, never through either wrapper. Nothing there needed changing. ## Tests The 21 original tests were written before the code and watched go red. The four added in response to the review were written the same way, and each was checked against the mutation it exists to catch. `tests/v19.sh` keeps everything it drove before — siteurl, post create/get/delete, `core version`, `check-update`, `verify-checksums` and the non-root updater guard — now under the Keel name, and additionally asks the compatibility names for the same answers on a booted appliance, each against a literal. ``` 221 tests, 0 failures kcov line coverage (threshold 97 percent): 100.00 21/21 keel-wordpress-update 100.00 8/8 keel-wp 100.00 31/31 zz-project-packages 100.00 26/26 zzz-keel-archive 99.00 99/100 wordpress.sh 97.73 43/44 40wordpress 98.95 282/285 boot-test-lib.sh 100.00 54/54 keel-archive-check ``` **99.12 percent (564/569)** over the eight measured shell files, up from 99.07 percent (535/540) over six on master. Both wrappers were at no coverage at all and are at 100 percent with every branch driven. Threshold stays 97, still set by `40wordpress`. ## Test plan - [x] `bats tests/` — 221 tests, 0 failures - [x] `COVERAGE_THRESHOLD=97 tests/coverage.sh` — exit 0, numbers above - [x] `shellcheck` clean on both scripts and `tests/coverage.sh` - [x] Mutation-checked: the reviewer's `USER` mutation, un-namespacing `WPROOT`, and dropping the WordPress guard each fail tests now - [ ] `appliance / build-and-boot` — see below - [ ] `tests/v19.sh` on a rebuilt appliance, where the compatibility names are proved on a real machine `appliance / build-and-boot` failed on the previous push for a reason outside this branch: the job boots the layer the mirror already serves, which does not carry this change, and every appliance assertion passed. It failed on `apt-get update`, where the runner resolves `archive.keellinux.org` to its own loopback (`127.0.1.1`) and is served a self-signed certificate. This branch touches neither `tests/boot-test.sh` nor `tests/lib/boot-test-lib.sh`.

marcos-mendez commented

2026-09-28 01:23:01 -03:00

(Migrated from github.com)

Review

I re-ran the suite and the coverage measurement, read fab's own source for the overlay step, mutated the new tests to see which of them are load-bearing, and read the booted wordpress-demo container. Everything the description claims about the mechanism and the numbers holds. Findings below are ordered by severity; none is CRITICAL or HIGH.

Verified

cmd_apply_overlay in fab really is cp -TdR OVERLAY DEST (fab:363-373), and prefix-relative-paths (share/product.mk:71-75) leaves an absolute entry alone, so Makefile:36's COMMON_OVERLAYS += $(CURDIR)/overlay is applied as itself. ROOT_OVERLAY ?= overlay (share/product.mk:86) applies the same directory a second time at share/product.mk:386. The overlay is applied twice, as claimed.
Ordering holds: share/product.mk:386 (root overlay) precedes :391 (run-conf-scripts $(CONF_SCRIPTS), and CONF_SCRIPTS ?= conf.d), so keel-wp exists when conf.d/main:94 calls it.
A second and third cp -TdR over an existing link leaves a link (GNU coreutils 9.7), and a tar round trip carries it. Both checked by hand.
Both negatives are correct. overlay/usr/lib/inithooks/firstboot.d/40wordpress:105-107 defines its own wp() { command wp --allow-root --path="$WORDPRESS_ROOT" "$@"; } and never touches either wrapper. A per-tracked-file grep over all 21 repositories and 6 worktrees finds no reference to either name outside this repository.
No sudoers rule, cron entry, systemd unit or timer, confconsole plugin or webmin entry anywhere in the organization's trees, or on the booted appliance, invokes either command. /etc/cron.d/wordpress-cron curls wp-cron.php and nothing else. Nothing exports WPROOT, WP_USER, WP_CLI, WP_DIR or WP_USR from /etc/environment, /etc/profile*, /etc/default or root's dotfiles.
COVERAGE_THRESHOLD=97 tests/coverage.sh: 217 tests, 0 failures, and the eight-file table reproduces the description line for line. 562/567 is 99.12 percent. Both wrappers at 100 percent, 9/9 and 18/18.
The WP_USER-rather-than-USER reasoning is right, and the old file shows why. The version on the running appliance opens USER=www-data — a plain assignment, which masks the inherited value. Turning that into ${USER:-www-data} would have read root's login environment, chowned all five wp-content runtime directories to root:root and made wp-config.php group root, so uploads and plugin installs would fail on the first supervised update and not before. Good catch, correctly avoided.
The tests are load-bearing. Nine of eleven mutations were caught: replacing the symlink with a regular file (tests 1, 12, 13), making the link absolute instead of relative (1, 12, 13), reverting ${0##*/} to a literal (15), chmod 0644 on wp-config.php (20), dropping its group (20), dropping verify-checksums (17, 19), removing the DEBUG line (11), removing the cache chown (7, 8, 10), and replacing printf '%q ' with $* (5, 6). The two that were not caught are MEDIUM 1 and LOW 1 below.

MEDIUM 1 — the one safeguard flagged for review is the one the suite cannot fail on

tests/wrappers.bats:25 and tests/wrappers.bats:72

setup() takes ME="$(id -un)" and then exports WP_USR="$ME" WP_USER="$ME". Under bats, $USER already is $(id -un), so ${USER:-www-data} and ${WP_USER:-www-data} expand to the same string and no assertion can tell them apart. I rewrote overlay/usr/local/sbin/keel-wordpress-update to use USER="${USER:-www-data}" throughout — the exact bug the description says was deliberately avoided — and all 21 tests stayed green.

The scenario: someone later "tidies" the variable back to USER, or writes the next wrapper with ${USER:-www-data} because the suite blessed it. The build is green, the appliance boots, and the first keel-wordpress-update an operator runs as root leaves wp-content/uploads owned by root. Media uploads and plugin installs fail from then on, with nothing pointing back at the update.

chown, install and runuser are all stubs that only log, so WP_USER does not need to name a real account. Setting it to a sentinel that $USER can never equal closes this in one line:

export WP_USR=keel-test-web-user WP_USER=keel-test-web-user

ME then has no remaining use.

MEDIUM 2 — a destructive root-run script now takes its target path from an unvalidated environment variable

overlay/usr/local/sbin/keel-wordpress-update:12 and :14

This is the half of the refactor that deserves the scrutiny it was flagged for. Lines 25-27 are chown -R root:root "$WPROOT", find "$WPROOT" -type d -exec chmod 0755 {} + and find "$WPROOT" -type f -exec chmod 0644 {} +, all as root. With WPROOT and WP_CLI both overridable and neither validated, WP_CLI=/bin/true WPROOT=/ keel-wordpress-update walks the whole filesystem and strips setuid from sudo, su and passwd on the way. Before this change that was not expressible.

To be clear about what this is not: it is not a privilege escalation. I checked for a constrained root path and there is none — no sudoers rule, no cron entry, no unit, no confconsole plugin calls this, and sudo's env_reset would strip the variables anyway. The only caller is a root shell, where the environment is already the caller's to choose. That is why this is MEDIUM and not HIGH.

It is still a footgun the hard-coded version did not have, and WPROOT is not an obscure name — this repository's own conf.d/main:23 uses WPROOT for the same path. Two cheap fixes, either one sufficient:

Namespace the overrides. The suite has already established KEEL_TEST_ for exactly this purpose (KEEL_TEST_UID, KEEL_TEST_WP_FAIL, KEEL_TEST_WP_CODE, KEEL_TEST_CHOWN_CODE, KEEL_TEST_RUNUSER_CODE). KEEL_TEST_WPROOT and KEEL_TEST_WP_CLI buy identical testability with none of the exposure.
Or assert the target before touching it. conf.d/main:86 already does this on the same path: [ -f "$WPROOT/wp-includes/version.php" ] || fatal ....

Related, minor: overlay/usr/local/bin/keel-wp:15 also gained WP_CLI, which the description does not list among the new overrides (it names only the updater's three). Lower risk there, since it lands inside runuser … -c and so runs as www-data, but the description should say so.

MEDIUM 3 — the compatibility assertion on a real appliance passes if both sides fail

tests/v19.sh:65

test "$(turnkey-wp option get siteurl)" = "$(keel-wp option get siteurl)"

set -euo pipefail is on, but a command substitution in test's arguments does not trip errexit. If wp-cli is broken, or the path is wrong, or Apache is down, both substitutions are empty and this is test "" = "", which passes. This is the traps.md entry "asserting the configuration is not asserting the behaviour", in the one test offered as proof that the old name still answers on a real machine — and it is the unchecked box in the test plan.

The script already knows the literal it wants; it asserted it at line 22. Comparing against it instead costs nothing and cannot pass on a double failure:

test "$(turnkey-wp option get siteurl)" = https://localhost

Line 64 (core version against $before) has the same shape, but $before is never asserted non-empty and that predates this change, so I am not counting it here.

LOW 1 — `-d` is not what keeps the link a link, and the test cannot tell

tests/wrappers.bats:20 and :186-191, COVERAGE.md:53-55, changelog:13-14

The conclusion is right and I verified it; the attributed mechanism is not. In GNU cp, -P/--no-dereference is already the default under -R, and only -L dereferences. Measured here:

cp -TR  →  link -> real     (symlink preserved; hard link split, nlink 1)
cp -TdR →  link -> real     (symlink preserved; hard link kept, nlink 2)
cp -TLR →  link             (regular file — this is the one that flattens)

So what -d actually contributes to the overlay step is --preserve=links, i.e. hard links. Symlink survival hinges on nobody passing -L. Confirming this: I dropped -d from the test's own cp and no test failed, so the test does not verify the property its comment says it verifies.

The scenario is a reader's, not a runtime one: someone adds a copy step, reads this comment, and either believes a copy without -d will flatten links, or believes adding -d makes any copy safe. Same wording appears in the decision note, where it matters more.

LOW 2 — a comment this change made stale

.github/workflows/tests.yml:20 — "tests/coverage.sh measures six shell files (COVERAGE.md)". tests/coverage.sh:28 now names eight and COVERAGE.md:24 says eight. The threshold: 97 below it is still correct.

LOW 3 — four names for two concepts in two files shipped side by side

overlay/usr/local/bin/keel-wp:12-13 uses WP_DIR and WP_USR; overlay/usr/local/sbin/keel-wordpress-update:12-13 uses WPROOT and WP_USER for the same WordPress root and the same web user. The cost shows up immediately at tests/wrappers.bats:71-72, which has to export all four. keel-wp's two are inherited, so this is a note rather than a request — but if MEDIUM 2 is taken and the updater's overrides are renamed anyway, that is the moment to make them agree.

LOW 4 — dead code in the wp-cli stub

tests/wrappers.bats:60-63. The for arg in "$@" … first=$arg; break loop computes first and nothing ever reads it. In a file whose whole argument is precision about what is being asserted, it reads as a leftover.

LOW 5 — two test descriptions claim slightly more than the tests do

tests/wrappers.bats:150, "keel-wp reports the exit code wp-cli gave it", drives KEEL_TEST_RUNUSER_CODE=3. runuser is a stub that never executes the command string, so wp-cli is not reached; what is proved is that runuser's exit code is handed back. True in practice, but the name asserts the longer chain.
tests/wrappers.bats:204 copies the overlay once where :194-195 copies it twice. The double application is therefore proved for turnkey-wp only. One more line would cover the updater's link too.

Not findings, recorded so they are not re-litigated

The second cp -TdR at tests/wrappers.bats:195 is redundant against coreutils 9.7 — removing it fails nothing. It is still correct insurance and would catch a cp that behaved differently on the second pass, so keep it.
Both symlinks are relative (readlink gives keel-wp and keel-wordpress-update), which is the right choice: an absolute link would not resolve inside fab-chroot. Making the link absolute breaks tests 1, 12 and 13, so this is protected.
Modes are right: 100755 on both scripts, 120000 on both links. Neither script holds a credential. chown root:"$WP_USER" plus chmod 0640 on wp-config.php keeps the database password off world-read, and that is asserted at tests/wrappers.bats:257-258.

What I verified versus what I inferred

Verified by execution or by reading the source: the fab copy command and the double application and their ordering, cp's behaviour over an existing link and what -d does and does not do, the tar round trip, all coverage and test-count numbers, every mutation result above, the absence of any unattended or sudo-constrained caller, and the state of the two files on the booted appliance.

Inferred: that MEDIUM 2 stays MEDIUM. It rests on there being no constrained root path to this script today, which I checked across this organization's trees and one booted appliance. A sudoers rule added later, or an operator tool that exports WPROOT, changes that reading.

Approve. No CRITICAL or HIGH issues. MEDIUM 1 is the one I would like to see before merge — it is a one-line change to the test setup, and without it the safeguard that was explicitly put up for review is unprotected.

## Review I re-ran the suite and the coverage measurement, read fab's own source for the overlay step, mutated the new tests to see which of them are load-bearing, and read the booted `wordpress-demo` container. Everything the description claims about the mechanism and the numbers holds. Findings below are ordered by severity; none is CRITICAL or HIGH. ### Verified - `cmd_apply_overlay` in fab really is `cp -TdR OVERLAY DEST` (`fab:363-373`), and `prefix-relative-paths` (`share/product.mk:71-75`) leaves an absolute entry alone, so `Makefile:36`'s `COMMON_OVERLAYS += $(CURDIR)/overlay` is applied as itself. `ROOT_OVERLAY ?= overlay` (`share/product.mk:86`) applies the same directory a second time at `share/product.mk:386`. **The overlay is applied twice, as claimed.** - Ordering holds: `share/product.mk:386` (root overlay) precedes `:391` (`run-conf-scripts $(CONF_SCRIPTS)`, and `CONF_SCRIPTS ?= conf.d`), so `keel-wp` exists when `conf.d/main:94` calls it. - A second and third `cp -TdR` over an existing link leaves a link (GNU coreutils 9.7), and a `tar` round trip carries it. Both checked by hand. - Both negatives are correct. `overlay/usr/lib/inithooks/firstboot.d/40wordpress:105-107` defines its own `wp() { command wp --allow-root --path="$WORDPRESS_ROOT" "$@"; }` and never touches either wrapper. A per-tracked-file grep over all 21 repositories and 6 worktrees finds no reference to either name outside this repository. - No sudoers rule, cron entry, systemd unit or timer, confconsole plugin or webmin entry anywhere in the organization's trees, or on the booted appliance, invokes either command. `/etc/cron.d/wordpress-cron` curls `wp-cron.php` and nothing else. Nothing exports `WPROOT`, `WP_USER`, `WP_CLI`, `WP_DIR` or `WP_USR` from `/etc/environment`, `/etc/profile*`, `/etc/default` or root's dotfiles. - `COVERAGE_THRESHOLD=97 tests/coverage.sh`: 217 tests, 0 failures, and the eight-file table reproduces the description line for line. 562/567 is 99.12 percent. Both wrappers at 100 percent, 9/9 and 18/18. - The `WP_USER`-rather-than-`USER` reasoning is right, and the old file shows why. The version on the running appliance opens `USER=www-data` — a plain assignment, which masks the inherited value. Turning that into `${USER:-www-data}` would have read root's login environment, chowned all five `wp-content` runtime directories to `root:root` and made `wp-config.php` group root, so uploads and plugin installs would fail on the first supervised update and not before. Good catch, correctly avoided. - The tests are load-bearing. Nine of eleven mutations were caught: replacing the symlink with a regular file (tests 1, 12, 13), making the link absolute instead of relative (1, 12, 13), reverting `${0##*/}` to a literal (15), `chmod 0644` on `wp-config.php` (20), dropping its group (20), dropping `verify-checksums` (17, 19), removing the `DEBUG` line (11), removing the cache `chown` (7, 8, 10), and replacing `printf '%q '` with `$*` (5, 6). The two that were not caught are MEDIUM 1 and LOW 1 below. --- ### MEDIUM 1 — the one safeguard flagged for review is the one the suite cannot fail on `tests/wrappers.bats:25` and `tests/wrappers.bats:72` `setup()` takes `ME="$(id -un)"` and then exports `WP_USR="$ME" WP_USER="$ME"`. Under bats, `$USER` already *is* `$(id -un)`, so `${USER:-www-data}` and `${WP_USER:-www-data}` expand to the same string and no assertion can tell them apart. I rewrote `overlay/usr/local/sbin/keel-wordpress-update` to use `USER="${USER:-www-data}"` throughout — the exact bug the description says was deliberately avoided — and **all 21 tests stayed green**. The scenario: someone later "tidies" the variable back to `USER`, or writes the next wrapper with `${USER:-www-data}` because the suite blessed it. The build is green, the appliance boots, and the first `keel-wordpress-update` an operator runs as root leaves `wp-content/uploads` owned by root. Media uploads and plugin installs fail from then on, with nothing pointing back at the update. `chown`, `install` and `runuser` are all stubs that only log, so `WP_USER` does not need to name a real account. Setting it to a sentinel that `$USER` can never equal closes this in one line: ```bash export WP_USR=keel-test-web-user WP_USER=keel-test-web-user ``` `ME` then has no remaining use. ### MEDIUM 2 — a destructive root-run script now takes its target path from an unvalidated environment variable `overlay/usr/local/sbin/keel-wordpress-update:12` and `:14` This is the half of the refactor that deserves the scrutiny it was flagged for. Lines 25-27 are `chown -R root:root "$WPROOT"`, `find "$WPROOT" -type d -exec chmod 0755 {} +` and `find "$WPROOT" -type f -exec chmod 0644 {} +`, all as root. With `WPROOT` and `WP_CLI` both overridable and neither validated, `WP_CLI=/bin/true WPROOT=/ keel-wordpress-update` walks the whole filesystem and strips setuid from `sudo`, `su` and `passwd` on the way. Before this change that was not expressible. To be clear about what this is not: it is **not** a privilege escalation. I checked for a constrained root path and there is none — no sudoers rule, no cron entry, no unit, no confconsole plugin calls this, and `sudo`'s `env_reset` would strip the variables anyway. The only caller is a root shell, where the environment is already the caller's to choose. That is why this is MEDIUM and not HIGH. It is still a footgun the hard-coded version did not have, and `WPROOT` is not an obscure name — this repository's own `conf.d/main:23` uses `WPROOT` for the same path. Two cheap fixes, either one sufficient: - Namespace the overrides. The suite has already established `KEEL_TEST_` for exactly this purpose (`KEEL_TEST_UID`, `KEEL_TEST_WP_FAIL`, `KEEL_TEST_WP_CODE`, `KEEL_TEST_CHOWN_CODE`, `KEEL_TEST_RUNUSER_CODE`). `KEEL_TEST_WPROOT` and `KEEL_TEST_WP_CLI` buy identical testability with none of the exposure. - Or assert the target before touching it. `conf.d/main:86` already does this on the same path: `[ -f "$WPROOT/wp-includes/version.php" ] || fatal ...`. Related, minor: `overlay/usr/local/bin/keel-wp:15` also gained `WP_CLI`, which the description does not list among the new overrides (it names only the updater's three). Lower risk there, since it lands inside `runuser … -c` and so runs as `www-data`, but the description should say so. ### MEDIUM 3 — the compatibility assertion on a real appliance passes if both sides fail `tests/v19.sh:65` ```bash test "$(turnkey-wp option get siteurl)" = "$(keel-wp option get siteurl)" ``` `set -euo pipefail` is on, but a command substitution in `test`'s arguments does not trip errexit. If wp-cli is broken, or the path is wrong, or Apache is down, both substitutions are empty and this is `test "" = ""`, which passes. This is the traps.md entry "asserting the configuration is not asserting the behaviour", in the one test offered as proof that the old name still answers on a real machine — and it is the unchecked box in the test plan. The script already knows the literal it wants; it asserted it at line 22. Comparing against it instead costs nothing and cannot pass on a double failure: ```bash test "$(turnkey-wp option get siteurl)" = https://localhost ``` Line 64 (`core version` against `$before`) has the same shape, but `$before` is never asserted non-empty and that predates this change, so I am not counting it here. --- ### LOW 1 — `-d` is not what keeps the link a link, and the test cannot tell `tests/wrappers.bats:20` and `:186-191`, `COVERAGE.md:53-55`, `changelog:13-14` The conclusion is right and I verified it; the attributed mechanism is not. In GNU cp, `-P/--no-dereference` is already the default under `-R`, and only `-L` dereferences. Measured here: ``` cp -TR → link -> real (symlink preserved; hard link split, nlink 1) cp -TdR → link -> real (symlink preserved; hard link kept, nlink 2) cp -TLR → link (regular file — this is the one that flattens) ``` So what `-d` actually contributes to the overlay step is `--preserve=links`, i.e. hard links. Symlink survival hinges on nobody passing `-L`. Confirming this: I dropped `-d` from the test's own `cp` and **no test failed**, so the test does not verify the property its comment says it verifies. The scenario is a reader's, not a runtime one: someone adds a copy step, reads this comment, and either believes a copy without `-d` will flatten links, or believes adding `-d` makes any copy safe. Same wording appears in the decision note, where it matters more. ### LOW 2 — a comment this change made stale `.github/workflows/tests.yml:20` — "tests/coverage.sh measures six shell files (COVERAGE.md)". `tests/coverage.sh:28` now names eight and `COVERAGE.md:24` says eight. The `threshold: 97` below it is still correct. ### LOW 3 — four names for two concepts in two files shipped side by side `overlay/usr/local/bin/keel-wp:12-13` uses `WP_DIR` and `WP_USR`; `overlay/usr/local/sbin/keel-wordpress-update:12-13` uses `WPROOT` and `WP_USER` for the same WordPress root and the same web user. The cost shows up immediately at `tests/wrappers.bats:71-72`, which has to export all four. `keel-wp`'s two are inherited, so this is a note rather than a request — but if MEDIUM 2 is taken and the updater's overrides are renamed anyway, that is the moment to make them agree. ### LOW 4 — dead code in the wp-cli stub `tests/wrappers.bats:60-63`. The `for arg in "$@" … first=$arg; break` loop computes `first` and nothing ever reads it. In a file whose whole argument is precision about what is being asserted, it reads as a leftover. ### LOW 5 — two test descriptions claim slightly more than the tests do - `tests/wrappers.bats:150`, "keel-wp reports the exit code wp-cli gave it", drives `KEEL_TEST_RUNUSER_CODE=3`. `runuser` is a stub that never executes the command string, so wp-cli is not reached; what is proved is that `runuser`'s exit code is handed back. True in practice, but the name asserts the longer chain. - `tests/wrappers.bats:204` copies the overlay once where `:194-195` copies it twice. The double application is therefore proved for `turnkey-wp` only. One more line would cover the updater's link too. --- ### Not findings, recorded so they are not re-litigated - The second `cp -TdR` at `tests/wrappers.bats:195` is redundant against coreutils 9.7 — removing it fails nothing. It is still correct insurance and would catch a cp that behaved differently on the second pass, so keep it. - Both symlinks are relative (`readlink` gives `keel-wp` and `keel-wordpress-update`), which is the right choice: an absolute link would not resolve inside `fab-chroot`. Making the link absolute breaks tests 1, 12 and 13, so this is protected. - Modes are right: `100755` on both scripts, `120000` on both links. Neither script holds a credential. `chown root:"$WP_USER"` plus `chmod 0640` on `wp-config.php` keeps the database password off world-read, and that is asserted at `tests/wrappers.bats:257-258`. ### What I verified versus what I inferred Verified by execution or by reading the source: the fab copy command and the double application and their ordering, cp's behaviour over an existing link and what `-d` does and does not do, the tar round trip, all coverage and test-count numbers, every mutation result above, the absence of any unattended or sudo-constrained caller, and the state of the two files on the booted appliance. Inferred: that MEDIUM 2 stays MEDIUM. It rests on there being no constrained root path to this script today, which I checked across this organization's trees and one booted appliance. A sudoers rule added later, or an operator tool that exports `WPROOT`, changes that reading. **Approve.** No CRITICAL or HIGH issues. MEDIUM 1 is the one I would like to see before merge — it is a one-line change to the test setup, and without it the safeguard that was explicitly put up for review is unprotected.

marcos-mendez commented

2026-09-28 02:01:28 -03:00

(Migrated from github.com)

Re-review

I checked each fix by running it, not by reading the diff. Every claim in the update holds, with one numeric correction. The suite is 25 tests in tests/wrappers.bats, 221 across the repository, 0 failures, and the coverage table reproduces byte for byte.

The three MEDIUM findings, re-tested

MEDIUM 1, the USER safeguard — fixed and now load-bearing. ME/$(id -un) is gone; WEB_USER=keel-test-web-user is a name no account can hold. I re-ran my original mutation, rewriting the script to USER="${USER:-www-data}" and using $USER throughout, against the new tree:

not ok 22 keel-wordpress-update puts the ownership boundary back
not ok 23 keel-wordpress-update leaves the five runtime directories to the web server
not ok 24 USER in the environment does not decide who owns wp-content

Three tests, as claimed — the numbers are 22, 23 and 24, not the 20, 23 and 24 in the description. Substance unaffected; worth correcting only because the description is where the next person will look.

Test 24 is the right shape: it sets USER=root in the caller's environment and then requires wp-content/uploads to still go to the sentinel, with ! grep -q '^chown -R root:root .*wp-content' as the negative half. That is the actual failure mode, driven rather than described.

MEDIUM 2, the footgun — both fixes taken, both load-bearing, namespacing complete in the script the finding named.

Un-namespacing WPROOT alone, leaving the other two prefixed, fails eight tests exactly as claimed: 14, 19, 20, 21, 22, 23, 24, 25. Dropping the wp-includes/version.php guard fails exactly one, 18. The mechanism is right too — with WPROOT un-prefixed the script falls back to /var/www/wordpress, the new guard refuses it, and everything downstream stops. The guard is doing work rather than sitting in front of tests that would pass anyway.

Namespacing is complete in keel-wordpress-update: all three knobs are KEEL_TEST_* (lines 19-21), and keel-wp gave WP_CLI back entirely and calls /usr/local/bin/wp literally at line 22, which tests/wrappers.bats:121 now pins as a literal string.

One thing I want to name rather than leave implicit, because it is the only un-namespaced override left anywhere in these two files. keel-wp:14 still reads WP_CACHE=${WP_CACHE:-/var/www/.wp-cli}, and keel-wp:19 is chown -R "$WP_USR":"$WP_USR" "$WP_CACHE", run as root. That is the same shape as the finding, one size smaller. It is not a regression — it is byte-identical to what master shipped, this change did not touch it, and the new comment in the test setup makes the carve-out deliberate and argued ("keel-wp's three knobs are the ones it already shipped with and are the appliance's own"). WP_DIR and WP_USR are genuinely harmless, since they land inside runuser and run as www-data. WP_CACHE is the odd one out: it chooses a path that root then walks with chown -R. So the finding is discharged as filed, and this is a separate, pre-existing, LOW item — the obvious next application of the reasoning the updater now carries in its own comment, not a condition on this merge.

MEDIUM 3, the vacuous v19 assertion — fixed, and the line I had excluded was fixed too. tests/v19.sh:65 is now test "$(turnkey-wp option get siteurl)" = https://localhost. For the version comparison, which cannot be a literal because the version is not known in advance, they added test -n "$before" at line 48 instead. That is the better fix for that line: it makes the empty-empty case impossible at the source rather than at each comparison. The comment above the block states the errexit reasoning, so the next person editing it knows why it is not two substitutions.

The `-d` correction — accepted, and now asserted in a way that fails

This is the one I was most interested in, because the old comment could not fail. Test 15 pins the real semantics: cp -TR must still yield a runnable turnkey-wp, and cp -TLR must turn both compatibility names into regular files.

To confirm it fails when the claim is violated, I put a cp shim first in PATH that rewrites -TR and -TdR to -TLR — that is, a cp that dereferences under -R, which is precisely the world in which this policy's mechanism is wrong:

not ok 13 the overlay copy the build makes keeps turnkey-wp a working link
not ok 14 the overlay copy keeps turnkey-wordpress-update a working link
not ok 15 it is -L that would flatten the link, and -d is not what prevents it

It fails. A coreutils change, or a different cp, would be caught. Committing turnkey-wp as a regular file also fails test 15. This is now a live assertion about the toolchain rather than a sentence about a flag.

The changelog and COVERAGE.md wording was corrected to match, and it is correct as written: -P default under -R, -d adds --preserve=links for hard links, the property is the absence of -L.

The five LOW items

All addressed. The dead first loop is gone from the wp stub; test 9 is renamed to "hands back the exit code runuser gave it", which is what it proves; test 14 now copies twice like test 13, so the double application is covered for both names; .github/workflows/tests.yml:20 says eight shell files. On the naming inconsistency: rather than making WP_DIR/WPROOT and WP_USR/WP_USER agree, the two scripts now have visibly different kinds of interface — appliance knobs on one, KEEL_TEST_ knobs on the other. That is a better resolution than the one I suggested, and the comment at lines 12-18 says why.

The numbers

COVERAGE_THRESHOLD=97 tests/coverage.sh, run here: 221 tests, 0 failures, and every per-file figure matches COVERAGE.md — keel-wordpress-update 21/21 (up from 18 with the guard), keel-wp 8/8 (down from 9 with WP_CLI removed), 564/569 total, which is 99.12 percent. Threshold still 97, still set by 40wordpress at 97.73.

The changelog gate, which moved under this branch

Keel-Linux/.github merged fix/changelog-version-in-the-name (45fdf3c) mid-flight, so entry_version now folds a trailing version in the source name into the comparison. I ran the shipped function against both entries:

turnkey-wordpress-19.0 (3)  ->  19.0-3      (master)
turnkey-wordpress-19.0 (4)  ->  19.0-4      (this branch)

dpkg --compare-versions orders 3 lt 4 under the old reading and 19.0-3 lt 19.0-4 under the new one. Passes both, and the live check agrees at 7 seconds.

The appliance job's green, which I am not counting

appliance / build-and-boot passes now. It should not be read as evidence for this change, and the description is right that nothing in the branch caused the flip.

What the log actually says: the layer is assembled from https://mirror.keellinux.org/layers, every WordPress assertion passes, and then

boot-test: SKIPPED: the four archive proofs. archive.keellinux.org resolves to a
boot-test:          loopback address inside this container, so apt would
boot-test:          reach the container itself. That is the runner's
boot-test:          network, not the appliance: tracker#5.
boot-test: wordpress boot test passed

I confirmed the skip guard is inherited: it came to master in 3a5d5c1, it is an ancestor of this branch, and the branch touches neither tests/boot-test.sh nor tests/lib/boot-test-lib.sh nor .keel-ci. So the red on the earlier run and the green on this one are both the runner's resolver, and neither is about the code.

The deeper point is the one already filed as Keel-Linux/.github#11, case 2: the job boots the published layer, which on a pull request that changes the recipe is the code before the change. So this check was incapable of testing this branch in either direction. I am treating it as contributing nothing, not as a pass.

That leaves the evidence for this change where it was: 25 unit tests, including two that run the compatibility name out of a copy made with the build's own command, and tests/v19.sh on a rebuilt appliance — still the unchecked box in the test plan, and still the only thing that will prove the link on a machine built from this branch. I am content with that for merge, because the unit tests now cover the overlay copy end to end and the remaining risk is confined to a rebuild that is going to happen anyway. Two smaller notes on the skip, both for tracker#5 / .github#11 rather than for this pull request: the skip names what it did not prove, why, and where it is tracked, which is the right shape; but it leaves the final line reading wordpress boot test passed and the check name unqualified, so a reader who sees only the green tick sees an unconditional pass. That is the failure mode #11 exists for, in a milder form.

What I verified versus what I inferred

Verified by running: all 25 tests and the 221-test coverage run; the USER mutation and its three failures; the WPROOT un-namespacing and its eight; the guard removal and its one; the dereferencing-cp shim and its three, including test 15; the regular-file mutation; entry_version on both changelog entries and the dpkg ordering; the appliance log; and that the skip guard predates this branch.

Inferred: that WP_CACHE in keel-wp is worth a follow-up rather than a change here. That rests on it being unchanged from master and on there still being no constrained root path to either command — I checked for one last round and nothing in this round adds an overlay, unit, cron entry or sudoers file.

Approve. Every finding from the first pass is discharged, and the two that could previously pass vacuously now fail when they should. Nothing above LOW remains, and the single LOW is pre-existing rather than introduced here.

## Re-review I checked each fix by running it, not by reading the diff. Every claim in the update holds, with one numeric correction. The suite is 25 tests in `tests/wrappers.bats`, 221 across the repository, 0 failures, and the coverage table reproduces byte for byte. ### The three MEDIUM findings, re-tested **MEDIUM 1, the `USER` safeguard — fixed and now load-bearing.** `ME`/`$(id -un)` is gone; `WEB_USER=keel-test-web-user` is a name no account can hold. I re-ran my original mutation, rewriting the script to `USER="${USER:-www-data}"` and using `$USER` throughout, against the new tree: ``` not ok 22 keel-wordpress-update puts the ownership boundary back not ok 23 keel-wordpress-update leaves the five runtime directories to the web server not ok 24 USER in the environment does not decide who owns wp-content ``` Three tests, as claimed — the numbers are 22, 23 and 24, not the 20, 23 and 24 in the description. Substance unaffected; worth correcting only because the description is where the next person will look. Test 24 is the right shape: it sets `USER=root` in the caller's environment and then requires `wp-content/uploads` to still go to the sentinel, with `! grep -q '^chown -R root:root .*wp-content'` as the negative half. That is the actual failure mode, driven rather than described. **MEDIUM 2, the footgun — both fixes taken, both load-bearing, namespacing complete in the script the finding named.** Un-namespacing `WPROOT` alone, leaving the other two prefixed, fails eight tests exactly as claimed: 14, 19, 20, 21, 22, 23, 24, 25. Dropping the `wp-includes/version.php` guard fails exactly one, 18. The mechanism is right too — with `WPROOT` un-prefixed the script falls back to `/var/www/wordpress`, the new guard refuses it, and everything downstream stops. The guard is doing work rather than sitting in front of tests that would pass anyway. Namespacing is complete in `keel-wordpress-update`: all three knobs are `KEEL_TEST_*` (lines 19-21), and `keel-wp` gave `WP_CLI` back entirely and calls `/usr/local/bin/wp` literally at line 22, which `tests/wrappers.bats:121` now pins as a literal string. One thing I want to name rather than leave implicit, because it is the only un-namespaced override left anywhere in these two files. `keel-wp:14` still reads `WP_CACHE=${WP_CACHE:-/var/www/.wp-cli}`, and `keel-wp:19` is `chown -R "$WP_USR":"$WP_USR" "$WP_CACHE"`, run as root. That is the same shape as the finding, one size smaller. **It is not a regression** — it is byte-identical to what master shipped, this change did not touch it, and the new comment in the test setup makes the carve-out deliberate and argued ("keel-wp's three knobs are the ones it already shipped with and are the appliance's own"). `WP_DIR` and `WP_USR` are genuinely harmless, since they land inside `runuser` and run as `www-data`. `WP_CACHE` is the odd one out: it chooses a path that root then walks with `chown -R`. So the finding is discharged as filed, and this is a separate, pre-existing, LOW item — the obvious next application of the reasoning the updater now carries in its own comment, not a condition on this merge. **MEDIUM 3, the vacuous v19 assertion — fixed, and the line I had excluded was fixed too.** `tests/v19.sh:65` is now `test "$(turnkey-wp option get siteurl)" = https://localhost`. For the version comparison, which cannot be a literal because the version is not known in advance, they added `test -n "$before"` at line 48 instead. That is the better fix for that line: it makes the empty-empty case impossible at the source rather than at each comparison. The comment above the block states the errexit reasoning, so the next person editing it knows why it is not two substitutions. ### The `-d` correction — accepted, and now asserted in a way that fails This is the one I was most interested in, because the old comment could not fail. Test 15 pins the real semantics: `cp -TR` must still yield a runnable `turnkey-wp`, and `cp -TLR` must turn both compatibility names into regular files. To confirm it fails when the claim is violated, I put a `cp` shim first in `PATH` that rewrites `-TR` and `-TdR` to `-TLR` — that is, a `cp` that dereferences under `-R`, which is precisely the world in which this policy's mechanism is wrong: ``` not ok 13 the overlay copy the build makes keeps turnkey-wp a working link not ok 14 the overlay copy keeps turnkey-wordpress-update a working link not ok 15 it is -L that would flatten the link, and -d is not what prevents it ``` It fails. A coreutils change, or a different `cp`, would be caught. Committing `turnkey-wp` as a regular file also fails test 15. This is now a live assertion about the toolchain rather than a sentence about a flag. The `changelog` and `COVERAGE.md` wording was corrected to match, and it is correct as written: `-P` default under `-R`, `-d` adds `--preserve=links` for hard links, the property is the absence of `-L`. ### The five LOW items All addressed. The dead `first` loop is gone from the wp stub; test 9 is renamed to "hands back the exit code runuser gave it", which is what it proves; test 14 now copies twice like test 13, so the double application is covered for both names; `.github/workflows/tests.yml:20` says eight shell files. On the naming inconsistency: rather than making `WP_DIR`/`WPROOT` and `WP_USR`/`WP_USER` agree, the two scripts now have visibly different *kinds* of interface — appliance knobs on one, `KEEL_TEST_` knobs on the other. That is a better resolution than the one I suggested, and the comment at lines 12-18 says why. ### The numbers `COVERAGE_THRESHOLD=97 tests/coverage.sh`, run here: 221 tests, 0 failures, and every per-file figure matches `COVERAGE.md` — `keel-wordpress-update` 21/21 (up from 18 with the guard), `keel-wp` 8/8 (down from 9 with `WP_CLI` removed), 564/569 total, which is 99.12 percent. Threshold still 97, still set by `40wordpress` at 97.73. ### The changelog gate, which moved under this branch `Keel-Linux/.github` merged `fix/changelog-version-in-the-name` (45fdf3c) mid-flight, so `entry_version` now folds a trailing version in the source name into the comparison. I ran the shipped function against both entries: ``` turnkey-wordpress-19.0 (3) -> 19.0-3 (master) turnkey-wordpress-19.0 (4) -> 19.0-4 (this branch) ``` `dpkg --compare-versions` orders `3 lt 4` under the old reading and `19.0-3 lt 19.0-4` under the new one. Passes both, and the live check agrees at 7 seconds. ### The appliance job's green, which I am not counting `appliance / build-and-boot` passes now. It should not be read as evidence for this change, and the description is right that nothing in the branch caused the flip. What the log actually says: the layer is assembled from `https://mirror.keellinux.org/layers`, every WordPress assertion passes, and then ``` boot-test: SKIPPED: the four archive proofs. archive.keellinux.org resolves to a boot-test: loopback address inside this container, so apt would boot-test: reach the container itself. That is the runner's boot-test: network, not the appliance: tracker#5. boot-test: wordpress boot test passed ``` I confirmed the skip guard is inherited: it came to master in 3a5d5c1, it is an ancestor of this branch, and the branch touches neither `tests/boot-test.sh` nor `tests/lib/boot-test-lib.sh` nor `.keel-ci`. So the red on the earlier run and the green on this one are both the runner's resolver, and neither is about the code. The deeper point is the one already filed as `Keel-Linux/.github#11`, case 2: the job boots the *published* layer, which on a pull request that changes the recipe is the code before the change. So this check was incapable of testing this branch in either direction. I am treating it as contributing nothing, not as a pass. That leaves the evidence for this change where it was: 25 unit tests, including two that run the compatibility name out of a copy made with the build's own command, and `tests/v19.sh` on a rebuilt appliance — still the unchecked box in the test plan, and still the only thing that will prove the link on a machine built from this branch. I am content with that for merge, because the unit tests now cover the overlay copy end to end and the remaining risk is confined to a rebuild that is going to happen anyway. Two smaller notes on the skip, both for tracker#5 / `.github#11` rather than for this pull request: the skip names what it did not prove, why, and where it is tracked, which is the right shape; but it leaves the final line reading `wordpress boot test passed` and the check name unqualified, so a reader who sees only the green tick sees an unconditional pass. That is the failure mode #11 exists for, in a milder form. ### What I verified versus what I inferred Verified by running: all 25 tests and the 221-test coverage run; the `USER` mutation and its three failures; the `WPROOT` un-namespacing and its eight; the guard removal and its one; the dereferencing-`cp` shim and its three, including test 15; the regular-file mutation; `entry_version` on both changelog entries and the `dpkg` ordering; the appliance log; and that the skip guard predates this branch. Inferred: that `WP_CACHE` in `keel-wp` is worth a follow-up rather than a change here. That rests on it being unchanged from master and on there still being no constrained root path to either command — I checked for one last round and nothing in this round adds an overlay, unit, cron entry or sudoers file. **Approve.** Every finding from the first pass is discharged, and the two that could previously pass vacuously now fail when they should. Nothing above LOW remains, and the single LOW is pre-existing rather than introduced here.