fix(install): render the systemd units from their templates, not from heredocs (#547)

* fix(install): render the systemd units from their templates, not from heredocs

The installers carried their own inline copies of units that also exist as
templates under systemd/, and the copies drifted.

install_service.sh renders ledmatrix.service from the template correctly, then
wrote ledmatrix-web.service from a heredoc that predated it -- missing
Wants=network-online.target, RestartSec=10, SyslogIdentifier, CacheDirectory,
CacheDirectoryMode and Environment=USE_THREADING=1. install_web_service.sh had
a third copy, and install_wifi_monitor.sh a fourth, that one already differing
from its template (syslog where the template says journal).

startup_validator.py compares the installed unit against the template, so a
rig installed this way warned on every boot -- and the remedy the warning
names, "re-run scripts/install/install_service.sh", reinstalled the same stale
copy. The warning could never clear. Reproduced on a live rig running exactly
that unit.

All three installers now render systemd/*.service through the same placeholder
substitution. The template gains a __USER__ placeholder rather than hardcoding
User=root, because the web interface runs as whoever installed it.

That last point was a second, independent cause of a permanent warning: the
validator substituted a fixed "root", so any non-root install reported drift
forever. It now reads User= from the installed unit -- an install-time
decision, not something the template dictates -- and compares everything else
strictly. first_time_install.sh already reads the installed User= the same way.

Tests cover a non-root web unit not warning, a genuinely changed directive in
that unit still warning, the User= fallback, and a grep-based guard that no
installer under scripts/install/ contains an inline unit body. That guard is
what found the install_wifi_monitor.sh copy.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014RRtqXDCnvnY6EQwhT5CV9

* fix(install): escape sed replacements, use mktemp, and make render failures fatal

Address CodeRabbit findings on install_service.sh, install_web_service.sh and
install_wifi_monitor.sh:

- Values interpolated into each script's sed expression (project root path,
  username) were not escaped, so a value containing &, \ or the | delimiter
  would corrupt the rendered systemd unit. Add a shared
  sed_escape_replacement() helper in the new scripts/install/lib_systemd_render.sh
  (sourced by all three scripts) and apply it to every sed replacement.
- install_service.sh rendered the main and web units to the predictable path
  /tmp/ledmatrix.service.tmp before installing them -- a symlink/TOCTOU race
  (CWE-377). Use mktemp for both, with a trap to clean up on exit.
- install_service.sh treated a missing template as a mere warning and then
  checked only whether a unit already existed at the destination before
  enabling/starting it, so a render failure could silently fall back to
  enabling a stale, previously-installed unit. Both unit blocks now exit
  non-zero on a missing template or a failed render.

Also rename the ambiguous loop variable `l` to `line` in
test/test_systemd_unit_drift.py (Ruff E741); ruff isn't wired into any CI
workflow in this repo today, so this isn't currently CI-blocking, but the
rename is trivial and correct regardless.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01S3bPMESe2TfrGvbs1ef9c5

* test(install): cover sed_escape_replacement against sed-special characters

CodeRabbit asked for regression coverage using a project path containing an
ampersand; the earlier commits on this branch already fixed the escaping,
mktemp usage, and enable/start-on-fatal-render-failure findings, and the
l->line rename was already applied -- this closes the one remaining gap.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

test/test_systemd_unit_drift.py

@@ -17,6 +17,8 @@ editing files under /etc and restarting services is the installer's job, not
 something a display process should do to a machine while it boots.
 """
 import logging
+import shlex
+import subprocess
 from pathlib import Path
 from unittest.mock import MagicMock
 
@@ -150,3 +152,122 @@ def test_a_missing_installed_unit_is_silent(validator, tmp_path):
     validator._validate_systemd_units()
     assert not validator.warnings
     assert not validator.errors
+
+
+# --- the web unit: one template, three copies, and a warning that never cleared ---
+#
+# install_service.sh and install_web_service.sh each carried their own inline
+# heredoc of ledmatrix-web.service. install_service.sh's had drifted -- no
+# Wants=network-online.target, RestartSec, SyslogIdentifier or CacheDirectory --
+# and that is what was installed on real rigs. The validator correctly reported
+# the drift and told the user to re-run install_service.sh, which reinstalled the
+# same stale copy, so the warning could never clear. Separately, the template
+# hardcoded User=root while the installers write whoever ran them, so even the
+# *correct* installer produced a permanent warning on any non-root install.
+
+
+def _render(template_text, project_root, user):
+    """Exactly what the installers' sed does."""
+    return (template_text
+            .replace("__PROJECT_ROOT_DIR__", str(project_root))
+            .replace("__USER__", user))
+
+
+def test_the_web_unit_installed_as_a_non_root_user_is_not_drift(validator, tmp_path):
+    """The web interface runs as whoever installed it, not as root.
+
+    This is the case that warned forever: nothing the user could do would make
+    an installed `User=pi` match a template that said `User=root`.
+    """
+    project_root = Path("src/startup_validator.py").resolve().parent.parent
+    template_rel = "systemd/ledmatrix-web.service"
+    template = project_root / template_rel
+    if not template.is_file():
+        pytest.skip("repo unit template not present")
+
+    installed = tmp_path / "ledmatrix-web.service"
+    installed.write_text(
+        _render(template.read_text(encoding="utf-8"), project_root, "hdpi"),
+        encoding="utf-8")
+
+    validator._UNITS = ((template_rel, str(installed)),)
+    validator._validate_systemd_units()
+    assert not validator.warnings, (
+        f"a correctly installed non-root web unit warned: {validator.warnings}")
+
+
+def test_the_web_unit_still_reports_a_real_changed_directive(validator, tmp_path):
+    """Ignoring User= must not make the check blind to everything else."""
+    project_root = Path("src/startup_validator.py").resolve().parent.parent
+    template_rel = "systemd/ledmatrix-web.service"
+    template = project_root / template_rel
+    if not template.is_file():
+        pytest.skip("repo unit template not present")
+
+    rendered = _render(template.read_text(encoding="utf-8"), project_root, "hdpi")
+    # Drop RestartSec -- one of the directives the stale heredoc was missing.
+    stale = "\n".join(line for line in rendered.splitlines() if not line.startswith("RestartSec="))
+    installed = tmp_path / "ledmatrix-web.service"
+    installed.write_text(stale + "\n", encoding="utf-8")
+
+    validator._UNITS = ((template_rel, str(installed)),)
+    validator._validate_systemd_units()
+    assert validator.warnings, "a web unit missing RestartSec= produced no warning"
+    assert not validator.errors
+
+
+def test_installed_user_falls_back_to_root():
+    """systemd defaults a system unit with no User= to root, so we must too."""
+    assert StartupValidator._installed_user("[Service]\nExecStart=/x\n") == "root"
+    assert StartupValidator._installed_user("[Service]\nUser=pi\n") == "pi"
+    assert StartupValidator._installed_user("[Service]\n  User=hdpi  \n") == "hdpi"
+
+
+def test_sed_escape_replacement_preserves_special_characters():
+    """A project path or username containing sed-special characters must render literally.
+
+    The installers build their sed expression by interpolating a shell
+    variable into the replacement side of `sed s|pattern|replacement|`.
+    Unescaped, sed treats `&` as "insert the whole match" and `\\` as an
+    escape character, so a path like `/opt/led&matrix` would corrupt the
+    rendered unit instead of being substituted as-is. lib_systemd_render.sh's
+    sed_escape_replacement exists to prevent exactly that.
+    """
+    project_root = Path("src/startup_validator.py").resolve().parent.parent
+    helper = project_root / "scripts" / "install" / "lib_systemd_render.sh"
+    if not helper.is_file():
+        pytest.skip("install helper not present")
+
+    value = "/opt/led&matrix\\pi|two"
+    escape_cmd = f'source {shlex.quote(str(helper))}; sed_escape_replacement {shlex.quote(value)}'
+    escaped = subprocess.run(
+        ["bash", "-c", escape_cmd], capture_output=True, text=True, check=True
+    ).stdout
+
+    rendered = subprocess.run(
+        ["sed", f"s|__X__|{escaped}|g"],
+        input="path=__X__\n", capture_output=True, text=True, check=True,
+    ).stdout
+
+    assert rendered == f"path={value}\n", (
+        "a sed-special character in the replacement was not preserved literally")
+
+
+def test_no_installer_carries_its_own_copy_of_a_unit():
+    """The regression guard.
+
+    Both installers used to inline the unit as a heredoc, and the two copies
+    drifted from the template and from each other. A unit body in a shell script
+    is the bug, so assert there isn't one rather than asserting the current
+    contents match -- matching contents is exactly what silently stops being
+    true.
+    """
+    project_root = Path("src/startup_validator.py").resolve().parent.parent
+    offenders = []
+    for script in sorted((project_root / "scripts" / "install").glob("*.sh")):
+        text = script.read_text(encoding="utf-8", errors="replace")
+        if "[Unit]" in text and "Description=" in text:
+            offenders.append(script.name)
+    assert not offenders, (
+        f"{offenders} contain an inline systemd unit; render "
+        f"systemd/*.service instead so there is one source of truth")