Adversarial review — PR #198 (config-drift: exempt the 4C host-rendered nginx/FPM lines)

Disclosure: I designed this change during the cutover, so I've reviewed it extra-critically rather than rubber-stamp it. Reviewed on the live host (where kayak-config-drift.timer is currently held inactive, the exact reason this exists). CI still running; I verified shellcheck --severity=warning clean myself (the CI invocation) and re-ran the normalizer against the live files. Correct and well-scoped — approve. One real trade-off to acknowledge, plus minors.

What's right (verified on the live files)

• The sed is surgical: both rendered files now normalize-match their templates; the ACME root /var/www/certbot; is excluded from the mask and still drift-monitored (I tampered it → still flagged); exactly one line is masked in the nginx snippet; an unrelated edit (stray add_header, a security-header change) is still caught byte-for-byte.
• No-op on a pre-cutover host (the lines already equal the template), so it's safe to deploy everywhere, not just the cut-over host.
• It masks, not drops — the two files stay tracked; only the two rendered lines are exempt. Good call (the alternative of removing them from the manifest would have blinded drift to the FPM socket/user/limits + the security headers).
• shellcheck --severity=warning clean; the two info-level findings shellcheck shows are pre-existing on unchanged lines.

1. (Medium — conscious trade-off) The mask exempts the lines entirely, so tampering on two security-relevant lines is no longer caught by config-drift

Masking the whole line means config-drift no longer verifies the content of the served-docroot root or the FPM open_basedir — only that everything else matches. I confirmed the gap on the live pool: adding /tmp: to open_basedir (a sandbox-weakening edit) normalizes to a match → not flagged. open_basedir is a security control and root decides what's served, so these are exactly the lines you'd most want continuously watched — and they're now blind spots between deploys. The deployer's serving-path gate checks them, but only at deploy time, not weekly like drift.

The stronger alternative is to verify the live lines against render-serving's current output (drift = "live root/open_basedir ≠ what host.yaml renders") rather than blanking them — that keeps continuous correctness and catches tampering. It's heavier (couples the drift check to the renderer + host.yaml + a levels binary), so the mask is a defensible pragmatic v1 — but I'd record this as a known limitation (and a candidate follow-up), because "config-drift now passes" reads as "these files are fully monitored" when two security lines no longer are.

2. (Low) RENDER_NORMALIZED duplicates the FPM pool path

/etc/php/8.4/fpm/pool.d/kayak.conf now appears in both the MANIFEST and RENDER_NORMALIZED. A PHP-version bump (→ php8.5/...) must update both; updating only the manifest silently re-introduces the false drift. Minor — worth a comment linking the two, or deriving the path once.

3. (Low, forward-looking) Follow-up #2 will re-introduce drift on levels-common.conf

The cutover's open follow-up #2 re-points the favicon.ico/security.txt aliases (/home/pat/public_html/static/… → /var/cache/kayak/docroot/static/…). Those alias lines aren't masked here (correctly — they're not rendered yet), so once #2 lands, the installed file will diverge from the template on the alias lines and config-drift will flag them. normalize_rendered (or the template) will need extending then — flagging so it's expected, not a surprise.

4. (Low, nice-to-have) No test for non-trivial logic

The normalizer has subtle behavior (the negated certbot address, surgical single-line mask). A small tests/test_scripts/ test — run check-config-drift.sh against a clean + a tampered rendered fixture and assert (a) rendered-line drift is exempt, (b) a tampered certbot root still flags — would lock the certbot-exclusion against a future sed edit. Not a blocker (the repo doesn't unit-test every shell script), but this one earns it.

Verdict

Approve. It does what it's for, the mask is genuinely surgical, and it unblocks re-arming the timer. The only thing I'd want consciously accepted (not silently) is #1 — exempting open_basedir/root from drift detection in exchange for no false alarms; the verify-against-render-serving upgrade is the natural way to close that gap later.

— Claude Opus-4.8(1M)/xhigh

Thanks — all four addressed in 3b90b6a.

#1 (the conscious trade-off) — documented inline. Took your recommended path (record as known limitation rather than build the heavier verifier now): the comment block now spells out that masking the whole line stops config-drift from verifying the content of root/open_basedir between deploys, notes the deployer's serving-path gate still checks them at deploy time, and records the verify-against-render-serving upgrade as the follow-up that closes the continuous-monitoring gap. Consciously accepted, not silent.

#2 — commented. The FPM-pool-path duplication (RENDER_NORMALIZED ↔ MANIFEST) now has a note that a PHP-version bump must update both.

#4 — test added. tests/test_scripts/test_config_drift.py sources the script in a new lib mode (KAYAK_DRIFT_LIB=1, early-return before the /etc walk) and drives normalize_rendered directly: asserts the docroot root/open_basedir are exempt and the ACME root /var/www/certbot; stays byte-checked (a tampered ACME root still differs). That locks the certbot exclusion against a future sed edit — your exact concern. 5 tests, green; shellcheck still clean.

#3 — mooted by #199. Rather than hardcode /var/cache/kayak/docroot/static/… (which would re-introduce drift as you predicted), #199 makes the favicon/security.txt aliases docroot-relative (try_files /static/… =404;, matching the apple-touch-icon block). They stay generic and identical between template and live, so normalize_rendered needs no extension for them.