Four non-blocking items from the independent review of #14, filed so they outlive the PR comment thread.
1. Nothing prevents site-key drift (the important one). src/site_key.rs ports ePHPm's normalize_host_key / is_valid_site_key / the #397 stripped-candidate rule. The reviewer verified it matches ePHPm main (f338104) exactly today. But it is a hand-copy in a second repo: if ePHPm changes its derivation, this silently disagrees, and a site-key disagreement is precisely the bug class ephpm#390/#366 fixed (wrong tenant identity → wrong database, wrong KV namespace). Options: (a) depend on a small shared crate exported from ephpm-server, (b) a CI check that diffs the two implementations and fails on divergence, (c) accept it and pin the ePHPm rev the port was verified against in a comment plus a test vector table. (c) is cheapest and still better than nothing.
2. No test asserts deploy_preview actually calls apply_document_root. The override-writing logic is well tested in isolation; the wiring is not. A refactor could stop writing the file and every test would stay green — which is the same partial-revert hole ephpm#451 was about.
3. DocumentRoot::Subdirectory's public fields allow bypassing the charset gate. render_override is safe when the value arrives through declared(), but the variant can be constructed directly with an unvalidated string. Make the field private with a validating constructor, or validate inside render_override as a backstop.
4. Lowercase nit. The default --sites-domain-suffix arm does not lowercase preview_domain while the explicit arm does, so a mixed-case --preview-domain silently switches keys to the full FQDN.
Also carried over from the infra#4 review: file readability depends on ephpm-ctl's umask. systemd's default 0022 gives 0644 and works, but a later UMask=0077 would make every override unreadable and ePHPm's fallback is silent — an explicit UMask=0027 in the unit would pin it.
Four non-blocking items from the independent review of #14, filed so they outlive the PR comment thread.
1. Nothing prevents site-key drift (the important one).
src/site_key.rsports ePHPm'snormalize_host_key/is_valid_site_key/ the #397 stripped-candidate rule. The reviewer verified it matches ePHPmmain(f338104) exactly today. But it is a hand-copy in a second repo: if ePHPm changes its derivation, this silently disagrees, and a site-key disagreement is precisely the bug class ephpm#390/#366 fixed (wrong tenant identity → wrong database, wrong KV namespace). Options: (a) depend on a small shared crate exported from ephpm-server, (b) a CI check that diffs the two implementations and fails on divergence, (c) accept it and pin the ePHPm rev the port was verified against in a comment plus a test vector table. (c) is cheapest and still better than nothing.2. No test asserts
deploy_previewactually callsapply_document_root. The override-writing logic is well tested in isolation; the wiring is not. A refactor could stop writing the file and every test would stay green — which is the same partial-revert hole ephpm#451 was about.3.
DocumentRoot::Subdirectory's public fields allow bypassing the charset gate.render_overrideis safe when the value arrives throughdeclared(), but the variant can be constructed directly with an unvalidated string. Make the field private with a validating constructor, or validate insiderender_overrideas a backstop.4. Lowercase nit. The default
--sites-domain-suffixarm does not lowercasepreview_domainwhile the explicit arm does, so a mixed-case--preview-domainsilently switches keys to the full FQDN.Also carried over from the infra#4 review: file readability depends on
ephpm-ctl's umask. systemd's default 0022 gives 0644 and works, but a laterUMask=0077would make every override unreadable and ePHPm's fallback is silent — an explicitUMask=0027in the unit would pin it.