From 08f3c764b23c49ba9d335cfe504fdf382dd3e183 Mon Sep 17 00:00:00 2001 From: Krzysztof kuhy Rudnicki Date: Fri, 21 Aug 2026 14:16:01 +0200 Subject: [PATCH] check_and_enable_services: a warning must never mask an error Eight checks assigned status="warning" unconditionally, so a check running later in the same function overwrote the status="error" an earlier one had set. The five other sites in this family already guard the assignment with `if [[ $status != "error" ]]`, so this was an inconsistency, not a design. Only half of it was load-bearing, and the two halves fail differently: services_units.sh (4 sites) reports through report_and_fix, which repairs ONLY on "error". A downgrade there meant no repair ran at all. The is-active check is the LAST status assignment in each of the three functions, so it could mask any error above it -- in check_periodic_systems, three separate is-enabled checks. A timer that was both disabled and inactive, which is the ordinary shape of a broken timer, was reported and then left broken. services_hosts.sh (4 sites) gates its own repair on `status != "ok"`, so the repair still ran. There the bug printed a real error as a yellow [!] warning and understated the summary row. Demonstrated rather than argued, against a simulated broken machine (timer disabled AND inactive, unit files present): without the guards: status=warning, repair ran 0 times with the guards: status=error, repair ran 1 time The seven assertions that previously pinned the broken behaviour now assert the correct behaviour; they were rewritten first and failed, then the guards made them pass. Two further cases cover the masking shape directly: a broken guard-lib instance must still be repaired when a later check warns. `--status --dry-run` output is still byte-identical to rev 6691cb26, because this machine has no disabled timer and no missing pacman hook -- the buggy path is never taken here, which is precisely why it went unnoticed. The A/B above is the evidence the live diff cannot provide. 222 assertions across 10 files. services_units.sh holds 100% coverage; services_hosts.sh rises from 86.05% to 93.02%. test_services_hosts.sh needed splitting twice to stay under the 250-line cap (hosts_fix, resolved), and stage_hosts_ok moved into the harness so both new files can use it. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01F3PzGc75gBcdxP3PkHXGcV --- ...vices-status-downgrade-fix-2026-08-21.json | 19 +++ ...vices-status-downgrade-fix-2026-08-21.json | 61 +++++++ .../periodic_background/lib/services_hosts.sh | 8 +- .../periodic_background/lib/services_units.sh | 8 +- .../lib/tests/services_harness.sh | 13 ++ .../lib/tests/test_services_hosts.sh | 161 ++++-------------- .../lib/tests/test_services_hosts_fix.sh | 105 ++++++++++++ .../lib/tests/test_services_hosts_resolved.sh | 73 ++++++++ .../lib/tests/test_services_units.sh | 32 ++-- 9 files changed, 326 insertions(+), 154 deletions(-) create mode 100644 docs/superpowers/contracts/services-status-downgrade-fix-2026-08-21.json create mode 100644 docs/superpowers/evidence/services-status-downgrade-fix-2026-08-21.json create mode 100755 linux_configuration/scripts/periodic_background/lib/tests/test_services_hosts_fix.sh create mode 100755 linux_configuration/scripts/periodic_background/lib/tests/test_services_hosts_resolved.sh diff --git a/docs/superpowers/contracts/services-status-downgrade-fix-2026-08-21.json b/docs/superpowers/contracts/services-status-downgrade-fix-2026-08-21.json new file mode 100644 index 00000000..14f42da4 --- /dev/null +++ b/docs/superpowers/contracts/services-status-downgrade-fix-2026-08-21.json @@ -0,0 +1,19 @@ +{ + "title": "Fix the status-downgrade bug so a warning can never mask an error", + "objective": "Stop a later check's status=\"warning\" from overwriting an earlier check's status=\"error\" in check_and_enable_services, which made genuinely broken services report as warnings and then never get repaired. Found while writing the split's tests and deferred out of 9085a4e5 because it changes live enforcement; the user asked for it to be fixed.", + "acceptance_criteria": [ + "No bare `status=\"warning\"` assignment remains in any check lib; every one is guarded by `if [[ $status != \"error\" ]]`, matching the idiom already used in services_apps.sh and services_browser.sh.", + "A timer that is BOTH disabled and inactive records \"error\" and triggers its setup script, demonstrated by an A/B run against a simulated broken machine rather than only by assertion.", + "The regression is covered by tests that FAILED before the fix and pass after it.", + "lib/tests/run_all.sh passes with no failures.", + "services_units.sh keeps 100% line coverage.", + "Every file including the test files stays at or under 250 lines.", + "shellcheck reports zero findings and no suppression is added." + ], + "out_of_scope": [ + "The 5 sites that were already correctly guarded.", + "Changing which statuses report_and_fix acts on: the fix makes the status correct rather than widening the repair gate.", + "The kcov attribution artifact on services_hosts.sh, diagnosed in the previous commit's evidence." + ], + "verifier": "lib/tests/run_all.sh (222 assertions), an A/B of check_midnight_shutdown against a disabled+inactive timer with and without the guards, bash meta/scripts/shell_coverage.sh lib/tests/run_all.sh services_units.sh 100, grep for unguarded sites, shellcheck, and check_file_length.sh --all." +} diff --git a/docs/superpowers/evidence/services-status-downgrade-fix-2026-08-21.json b/docs/superpowers/evidence/services-status-downgrade-fix-2026-08-21.json new file mode 100644 index 00000000..7c2840dc --- /dev/null +++ b/docs/superpowers/evidence/services-status-downgrade-fix-2026-08-21.json @@ -0,0 +1,61 @@ +{ + "intent": "Fix the status-downgrade bug in check_and_enable_services, where a later check assigning status=\"warning\" unconditionally could overwrite the status=\"error\" an earlier check had set — so a genuinely broken service was reported and then silently never repaired.", + "scope": [ + "linux_configuration/scripts/periodic_background/lib/services_units.sh — 4 unguarded sites, the load-bearing ones", + "linux_configuration/scripts/periodic_background/lib/services_hosts.sh — 4 unguarded sites, cosmetic but wrong", + "lib/tests/test_services_units.sh — 7 pinning assertions converted to regression assertions", + "lib/tests/test_services_hosts.sh — 2 new masking regression cases", + "lib/tests/ — test_services_hosts_fix.sh and test_services_hosts_resolved.sh split out for the 250-line cap; stage_hosts_ok moved into the harness so both files can use it", + "Non-goal: the 5 sites that were already guarded correctly" + ], + "changes": [ + "All 8 bare `status=\"warning\"` assignments now read `if [[ $status != \"error\" ]]; then status=\"warning\"; fi`, which is the idiom services_apps.sh and services_browser.sh already used — so this makes the family internally consistent rather than inventing a convention.", + "The blast radius differs between the two files and only one is load-bearing. services_units.sh reports through report_and_fix, which repairs ONLY on \"error\", so a downgrade there meant no repair ran at all. services_hosts.sh gates its own repair on `status != \"ok\"`, so repair still ran; there the bug printed a real error as a yellow [!] warning and understated the summary row.", + "The three unit checks were the worst case because the is-active check is the LAST status assignment in each function, so it could overwrite any error set above it. In check_periodic_systems that is hosts-file-monitor.service, which sits after three separate is-enabled checks." + ], + "verification": [ + { + "command": "lib/tests/test_services_units.sh (before the fix)", + "result": "7 failures, as intended", + "evidence": "The pinning assertions were rewritten to assert correct behaviour first, and failed: 'disabled+inactive stays an error (want error, got warning)' and 'so the disabled timer IS repaired (no call matching /ran setup_midnight_shutdown.sh enable/)' for all three unit checks." + }, + { + "command": "A/B against a simulated broken machine: timer disabled AND inactive, unit files present", + "result": "pass", + "evidence": "Without the fix: status=warning, repair ran 0 times — the timer stays dead. With the fix: status=error, repair ran 1 time — the timer is re-enabled. Run by reverting only the 4 guards in services_units.sh and restoring them afterwards." + }, + { + "command": "lib/tests/run_all.sh", + "result": "pass", + "evidence": "222 assertions across 10 test files, 0 failures." + }, + { + "command": "diff <(sudo bash /…/check_and_enable_services.sh --status --dry-run) <(sudo bash …/check_and_enable_services.sh --status --dry-run)", + "result": "identical", + "evidence": "Still byte-identical modulo Date:. This machine has no disabled timer and no missing pacman hook, so the buggy path is not taken here — which is exactly why the fault went unnoticed. The A/B above is what demonstrates the behaviour change, since the live diff cannot." + }, + { + "command": "bash meta/scripts/shell_coverage.sh lib/tests/run_all.sh 100", + "result": "pass on 6 of 8", + "evidence": "services_units.sh holds 100.00% (72/72) through the change. services_hosts.sh rose from 86.05% to 93.02% as the new masking cases exercised more branches; the 6 remaining lines are the kcov attribution artifact diagnosed in check-and-enable-services-coverage-2026-08-21.json. services_hosts_fix.sh 97.30% (2 pipeline continuation lines)." + }, + { + "command": "grep -n '^\\s*status=\"warning\"$' lib/services_*.sh", + "result": "pass", + "evidence": "No unguarded site remains." + }, + { + "command": "shellcheck lib/tests/*.sh lib/services_*.sh check_and_enable_services.sh; bash meta/scripts/check_file_length.sh --all", + "result": "pass", + "evidence": "Zero shellcheck findings, no suppression. No file over 250 lines; the hosts test file needed splitting twice to stay under it." + } + ], + "risks": [ + "This deliberately changes live enforcement behaviour: services that previously sat in a warning state without being repaired will now be repaired on the next hourly run. That is the point of the fix, but it means the next run on a machine with a disabled timer will actually execute a setup script rather than doing nothing.", + "A service whose repair script is broken will now retry every run instead of staying quiet at warning. err_missing_script already logs that case at journal error priority, so it surfaces rather than looping silently." + ], + "rollback": [ + "git revert this commit restores the unguarded assignments and the pinning tests together, so the suite stays green either way.", + "After rollback, confirm with the A/B: a disabled+inactive timer should report warning and run no repair." + ] +} diff --git a/linux_configuration/scripts/periodic_background/lib/services_hosts.sh b/linux_configuration/scripts/periodic_background/lib/services_hosts.sh index 48151583..92d8238f 100755 --- a/linux_configuration/scripts/periodic_background/lib/services_hosts.sh +++ b/linux_configuration/scripts/periodic_background/lib/services_hosts.sh @@ -28,7 +28,7 @@ check_hosts() { msg "/etc/hosts exists with $line_count lines (StevenBlack list likely installed)" else issues+=("/etc/hosts has only $line_count lines (StevenBlack list may not be installed)") - status="warning" + if [[ $status != "error" ]]; then status="warning"; fi fi else issues+=("/etc/hosts does not exist") @@ -42,7 +42,7 @@ check_hosts() { msg "/etc/hosts has immutable attribute set" else issues+=("/etc/hosts is not immutable") - status="warning" + if [[ $status != "error" ]]; then status="warning"; fi fi # Check cached hosts file @@ -50,7 +50,7 @@ check_hosts() { msg "StevenBlack cache exists at ${SYSROOT}/etc/hosts.stevenblack" else issues+=("StevenBlack cache not found") - status="warning" + if [[ $status != "error" ]]; then status="warning"; fi fi # Check the guard-lib "hosts" file-guard instance (path unit active + @@ -70,7 +70,7 @@ check_hosts() { msg "Pacman hooks installed" else issues+=("Pacman hooks not installed") - status="warning" + if [[ $status != "error" ]]; then status="warning"; fi fi # Check nsswitch.conf has 'files' in hosts line diff --git a/linux_configuration/scripts/periodic_background/lib/services_units.sh b/linux_configuration/scripts/periodic_background/lib/services_units.sh index 75de5da3..820e7602 100755 --- a/linux_configuration/scripts/periodic_background/lib/services_units.sh +++ b/linux_configuration/scripts/periodic_background/lib/services_units.sh @@ -31,7 +31,7 @@ check_midnight_shutdown() { msg "day-specific-shutdown.timer is active" else issues+=("day-specific-shutdown.timer is not active") - status="warning" + if [[ $status != "error" ]]; then status="warning"; fi fi # Check service file exists @@ -75,7 +75,7 @@ check_startup_monitor() { msg "pc-startup-monitor.timer is active" else issues+=("pc-startup-monitor.timer is not active") - status="warning" + if [[ $status != "error" ]]; then status="warning"; fi fi # Check service file exists @@ -118,7 +118,7 @@ check_periodic_systems() { msg "periodic-system-maintenance.timer is active" else issues+=("periodic-system-maintenance.timer is not active") - status="warning" + if [[ $status != "error" ]]; then status="warning"; fi fi # Check startup service @@ -141,7 +141,7 @@ check_periodic_systems() { msg "hosts-file-monitor.service is active" else issues+=("hosts-file-monitor.service is not active") - status="warning" + if [[ $status != "error" ]]; then status="warning"; fi fi # Check maintenance script diff --git a/linux_configuration/scripts/periodic_background/lib/tests/services_harness.sh b/linux_configuration/scripts/periodic_background/lib/tests/services_harness.sh index fa8720e6..bd55ed30 100755 --- a/linux_configuration/scripts/periodic_background/lib/tests/services_harness.sh +++ b/linux_configuration/scripts/periodic_background/lib/tests/services_harness.sh @@ -208,6 +208,19 @@ make_installer() { # # shellcheck source=../services_common.sh . "${LIB_DIR}/services_common.sh" +# Stage a fully healthy blocking stack: a long hosts file, the guard-lib trio +# active, both pacman hooks, and a resolved.conf that reads /etc/hosts. +stage_hosts_ok() { + sysfile etc/hosts 200 + sysfile etc/hosts.stevenblack 200 + printf '%s\n' "${SERVICES_ROOT}/etc/hosts" >"${DEV}/immutable" + printf 'hosts: files resolve dns\n' >"${SERVICES_ROOT}/etc/nsswitch.conf" + printf '[Resolve]\nReadEtcHosts=yes\n' >"${SERVICES_ROOT}/etc/systemd/resolved.conf" + printf '%s\n' hosts nsswitch resolved >"${DEV}/guard_healthy" + sysfile etc/pacman.d/hooks/10-guard-lib-unlock-all.hook + sysfile etc/pacman.d/hooks/90-guard-lib-relock-all.hook +} + # reset_state — return every mutable global and every $DEV fact to "nothing has # happened yet", so each test group starts from a known machine. reset_state() { diff --git a/linux_configuration/scripts/periodic_background/lib/tests/test_services_hosts.sh b/linux_configuration/scripts/periodic_background/lib/tests/test_services_hosts.sh index 5aea973a..8a84d503 100755 --- a/linux_configuration/scripts/periodic_background/lib/tests/test_services_hosts.sh +++ b/linux_configuration/scripts/periodic_background/lib/tests/test_services_hosts.sh @@ -28,19 +28,6 @@ _t_called_in() { # fi } -# Stage a fully healthy blocking stack: a long hosts file, the guard-lib trio -# active, both pacman hooks, and a resolved.conf that reads /etc/hosts. -stage_hosts_ok() { - sysfile etc/hosts 200 - sysfile etc/hosts.stevenblack 200 - printf '%s\n' "${SERVICES_ROOT}/etc/hosts" >"${DEV}/immutable" - printf 'hosts: files resolve dns\n' >"${SERVICES_ROOT}/etc/nsswitch.conf" - printf '[Resolve]\nReadEtcHosts=yes\n' >"${SERVICES_ROOT}/etc/systemd/resolved.conf" - printf '%s\n' hosts nsswitch resolved >"${DEV}/guard_healthy" - sysfile etc/pacman.d/hooks/10-guard-lib-unlock-all.hook - sysfile etc/pacman.d/hooks/90-guard-lib-relock-all.hook -} - echo "== check_hosts: a fully healthy stack records ok ==" reset_state stage_hosts_ok @@ -108,51 +95,6 @@ sysrm etc/nsswitch.conf check_hosts >"${TEST_TMPDIR}/out.txt" _t_called_in "$(cat "${TEST_TMPDIR}/out.txt")" "/etc/nsswitch.conf does not exist" "the missing file is reported" -echo "== check_hosts: ReadEtcHosts=no is an error and is repaired ==" -reset_state -stage_hosts_ok -make_installer "$HOSTS_INSTALL_SCRIPT" -make_installer "$GUARD_LIB_MIGRATE_SCRIPT" -printf '[Resolve]\nReadEtcHosts=no\n' >"${SERVICES_ROOT}/etc/systemd/resolved.conf" -check_hosts >"${TEST_TMPDIR}/out.txt" -_t_called_in "$(cat "${TEST_TMPDIR}/out.txt")" "bypassed by systemd-resolved" "the resolved bypass is called out" -_t_called_in "$(cat "${SERVICES_ROOT}/etc/systemd/resolved.conf")" "ReadEtcHosts=yes" "the setting is repaired" -_t_called 'systemctl restart systemd-resolved' "systemd-resolved is restarted so the change takes" - -echo "== check_hosts: DNSOverTLS enabled is an error and is repaired ==" -reset_state -stage_hosts_ok -make_installer "$HOSTS_INSTALL_SCRIPT" -make_installer "$GUARD_LIB_MIGRATE_SCRIPT" -printf '[Resolve]\nReadEtcHosts=yes\nDNSOverTLS=yes\n' >"${SERVICES_ROOT}/etc/systemd/resolved.conf" -check_hosts >"${TEST_TMPDIR}/out.txt" -# The message interpolates $SYSROOT, so match on the stable part of it. -_t_called_in "$(cat "${TEST_TMPDIR}/out.txt")" "DNSOverTLS='yes'" "DNSOverTLS is called out as a bypass" -_t_called_in "$(cat "${SERVICES_ROOT}/etc/systemd/resolved.conf")" "#DNSOverTLS=no" "it is commented out" - -echo "== check_hosts: drop-in overrides are an error and are removed ==" -reset_state -stage_hosts_ok -make_installer "$HOSTS_INSTALL_SCRIPT" -make_installer "$GUARD_LIB_MIGRATE_SCRIPT" -# A drop-in can re-enable either bypass without touching resolved.conf, so the -# repairs above are not durable while one survives. -sysfile etc/systemd/resolved.conf.d/99-override.conf -check_hosts >"${TEST_TMPDIR}/out.txt" -_t_called_in "$(cat "${TEST_TMPDIR}/out.txt")" "drop-in override" "the drop-in is called out" -if [[ -f "${SERVICES_ROOT}/etc/systemd/resolved.conf.d/99-override.conf" ]]; then - _t_fail "the drop-in is deleted" -else - _t_pass "the drop-in is deleted" -fi - -echo "== check_hosts: a missing resolved.conf warns ==" -reset_state -stage_hosts_ok -sysrm etc/systemd/resolved.conf -check_hosts >"${TEST_TMPDIR}/out.txt" -_t_called_in "$(cat "${TEST_TMPDIR}/out.txt")" "resolved.conf does not exist" "the missing file is reported" - echo "== check_hosts: an unhealthy guard-lib instance is repaired ==" reset_state stage_hosts_ok @@ -224,77 +166,6 @@ printf 'nsswitch\n' >"${DEV}/guard_degraded" check_hosts >/dev/null _t_eq "2" "${#MISSING_SCRIPTS[@]}" "both missing repair scripts are recorded" -echo "== hosts_fix_nsswitch: each resolver shape ==" -reset_state -printf 'hosts: mymachines dns\n' >"${SERVICES_ROOT}/etc/nsswitch.conf" -if hosts_fix_nsswitch >/dev/null; then :; fi -_t_called_in "$(cat "${SERVICES_ROOT}/etc/nsswitch.conf")" "files dns" "'files' is inserted before dns" - -reset_state -printf 'hosts: mymachines myhostname\n' >"${SERVICES_ROOT}/etc/nsswitch.conf" -if hosts_fix_nsswitch >/dev/null; then :; fi -_t_called_in "$(cat "${SERVICES_ROOT}/etc/nsswitch.conf")" "hosts: files" "'files' is prepended when no resolver is named" - -reset_state -printf 'hosts: files resolve\n' >"${SERVICES_ROOT}/etc/nsswitch.conf" -if hosts_fix_nsswitch >/dev/null; then :; fi -_t_eq "hosts: files resolve" "$(cat "${SERVICES_ROOT}/etc/nsswitch.conf")" "an already-correct line is left alone" - -reset_state -sysrm etc/nsswitch.conf -if hosts_fix_nsswitch >/dev/null; then :; fi -_t_pass "a missing nsswitch.conf is a no-op rather than an error" - -reset_state -printf 'passwd: files\n' >"${SERVICES_ROOT}/etc/nsswitch.conf" -if hosts_fix_nsswitch >/dev/null; then :; fi -_t_pass "an nsswitch.conf with no hosts line at all is a no-op" - -echo "== hosts_fix_resolved: adds the setting when the file lacks it ==" -# Called through `if` rather than bare. When resolved.conf carries no -# ReadEtcHosts line at all, the leading `grep` in the assignment pipeline exits -# 1; with `set -o pipefail` that makes the whole assignment non-zero, so a bare -# call aborts the test under `set -e`. Production never hits this because -# check_hosts only ever calls it from inside an `if`, which suppresses `set -e` -# for the callee -- so this is a property of calling it bare, not a defect. -reset_state -printf '[Resolve]\n' >"${SERVICES_ROOT}/etc/systemd/resolved.conf" -if hosts_fix_resolved >/dev/null; then :; fi -_t_called_in "$(cat "${SERVICES_ROOT}/etc/systemd/resolved.conf")" "ReadEtcHosts=yes" "the setting is inserted under [Resolve]" - -reset_state -printf '# no section here\n' >"${SERVICES_ROOT}/etc/systemd/resolved.conf" -if hosts_fix_resolved >/dev/null; then :; fi -_t_called_in "$(cat "${SERVICES_ROOT}/etc/systemd/resolved.conf")" "[Resolve]" "a whole [Resolve] section is appended when absent" - -reset_state -sysrm etc/systemd/resolved.conf -if hosts_fix_resolved >/dev/null; then :; fi -_t_pass "a missing resolved.conf is a no-op rather than an error" - -echo "== hosts_repair_all: dry-run refuses to claim success ==" -reset_state -stage_hosts_ok -make_installer "$HOSTS_INSTALL_SCRIPT" -make_installer "$GUARD_LIB_MIGRATE_SCRIPT" -DRY_RUN=1 -if hosts_repair_all >/dev/null; then - _t_fail "hosts_repair_all returns non-zero under --dry-run" -else - _t_pass "hosts_repair_all returns non-zero under --dry-run" -fi - -echo "== hosts_repair_all: a healthy machine verifies after repair ==" -reset_state -stage_hosts_ok -make_installer "$HOSTS_INSTALL_SCRIPT" -make_installer "$GUARD_LIB_MIGRATE_SCRIPT" -if hosts_repair_all >/dev/null; then - _t_pass "hosts_repair_all returns zero when the post-repair check passes" -else - _t_fail "hosts_repair_all returns zero when the post-repair check passes" -fi - echo "== check_hosts: resolved.conf immutable attribute is recognised ==" reset_state stage_hosts_ok @@ -317,4 +188,36 @@ for inst in hosts nsswitch resolved; do _t_called 'ran migrate_hosts_guard' "and the migration is re-run for '${inst}'" done +echo "== check_hosts: a warning must never mask an error ==" +# Regression test for the status-downgrade bug: several checks assigned +# status="warning" unconditionally, so a check running LATER in the function +# could overwrite the status="error" an earlier one had set. Because +# report_and_fix (and check_hosts' own repair gate) only act on "error", the +# downgraded fault was reported and then silently never repaired. +# +# Here the guard-lib 'hosts' instance is broken (an error) AND the pacman hook +# pair is missing (a warning, and checked afterwards). The row must stay +# "error" and the repair must run. +reset_state +stage_hosts_ok +make_installer "$HOSTS_INSTALL_SCRIPT" +make_installer "$GUARD_LIB_MIGRATE_SCRIPT" +printf '%s\n' nsswitch resolved >"${DEV}/guard_healthy" +sysrm etc/pacman.d/hooks/10-guard-lib-unlock-all.hook +sysrm etc/pacman.d/hooks/90-guard-lib-relock-all.hook +check_hosts >"${TEST_TMPDIR}/out.txt" +_t_called 'ran migrate_hosts_guard' "a broken guard is still repaired when a later check warns" + +# The same shape with the short-hosts-file warning, which is checked FIRST and +# so cannot mask anything -- but the immutable/cache warnings after it can. +reset_state +stage_hosts_ok +make_installer "$HOSTS_INSTALL_SCRIPT" +make_installer "$GUARD_LIB_MIGRATE_SCRIPT" +printf '%s\n' nsswitch resolved >"${DEV}/guard_healthy" +: >"${DEV}/immutable" +sysrm etc/hosts.stevenblack +check_hosts >/dev/null +_t_called 'ran migrate_hosts_guard' "a broken guard survives the immutable and cache warnings" + _t_summary diff --git a/linux_configuration/scripts/periodic_background/lib/tests/test_services_hosts_fix.sh b/linux_configuration/scripts/periodic_background/lib/tests/test_services_hosts_fix.sh new file mode 100755 index 00000000..638bccc5 --- /dev/null +++ b/linux_configuration/scripts/periodic_background/lib/tests/test_services_hosts_fix.sh @@ -0,0 +1,105 @@ +#!/usr/bin/env bash +# Tests for the repair helpers in lib/services_hosts_fix.sh, called directly +# rather than through check_hosts. +# +# Split out of test_services_hosts.sh for the repo-wide 250-line cap. These are +# the unit-level cases: each resolver shape hosts_fix_nsswitch has to rewrite, +# each resolved.conf shape hosts_fix_resolved has to repair, and the contract +# that hosts_repair_all reports failure under --dry-run rather than claiming a +# repair that never happened. +# +# The helpers are called via `if` rather than bare: when resolved.conf carries +# no ReadEtcHosts line, the leading grep in the assignment pipeline exits 1 and +# `set -o pipefail` makes the whole assignment non-zero, so a bare call would +# abort the suite under `set -e`. check_hosts only ever calls them from inside +# an `if`, which suppresses `set -e` for the callee, so this is a property of +# calling them bare and not a defect. +set -euo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" + +# shellcheck source=services_harness.sh +. "${SCRIPT_DIR}/services_harness.sh" +# shellcheck source=../services_hosts_fix.sh +. "${SCRIPT_DIR}/../services_hosts_fix.sh" + +_t_called_in() { # + if [[ "$1" == *"$2"* ]]; then + _t_pass "$3" + else + _t_fail "$3 (missing '$2')" + fi +} + +echo "== hosts_fix_nsswitch: each resolver shape ==" +reset_state +printf 'hosts: mymachines dns\n' >"${SERVICES_ROOT}/etc/nsswitch.conf" +if hosts_fix_nsswitch >/dev/null; then :; fi +_t_called_in "$(cat "${SERVICES_ROOT}/etc/nsswitch.conf")" "files dns" "'files' is inserted before dns" + +reset_state +printf 'hosts: mymachines myhostname\n' >"${SERVICES_ROOT}/etc/nsswitch.conf" +if hosts_fix_nsswitch >/dev/null; then :; fi +_t_called_in "$(cat "${SERVICES_ROOT}/etc/nsswitch.conf")" "hosts: files" "'files' is prepended when no resolver is named" + +reset_state +printf 'hosts: files resolve\n' >"${SERVICES_ROOT}/etc/nsswitch.conf" +if hosts_fix_nsswitch >/dev/null; then :; fi +_t_eq "hosts: files resolve" "$(cat "${SERVICES_ROOT}/etc/nsswitch.conf")" "an already-correct line is left alone" + +reset_state +sysrm etc/nsswitch.conf +if hosts_fix_nsswitch >/dev/null; then :; fi +_t_pass "a missing nsswitch.conf is a no-op rather than an error" + +reset_state +printf 'passwd: files\n' >"${SERVICES_ROOT}/etc/nsswitch.conf" +if hosts_fix_nsswitch >/dev/null; then :; fi +_t_pass "an nsswitch.conf with no hosts line at all is a no-op" + +echo "== hosts_fix_resolved: adds the setting when the file lacks it ==" +# Called through `if` rather than bare. When resolved.conf carries no +# ReadEtcHosts line at all, the leading `grep` in the assignment pipeline exits +# 1; with `set -o pipefail` that makes the whole assignment non-zero, so a bare +# call aborts the test under `set -e`. Production never hits this because +# check_hosts only ever calls it from inside an `if`, which suppresses `set -e` +# for the callee -- so this is a property of calling it bare, not a defect. +reset_state +printf '[Resolve]\n' >"${SERVICES_ROOT}/etc/systemd/resolved.conf" +if hosts_fix_resolved >/dev/null; then :; fi +_t_called_in "$(cat "${SERVICES_ROOT}/etc/systemd/resolved.conf")" "ReadEtcHosts=yes" "the setting is inserted under [Resolve]" + +reset_state +printf '# no section here\n' >"${SERVICES_ROOT}/etc/systemd/resolved.conf" +if hosts_fix_resolved >/dev/null; then :; fi +_t_called_in "$(cat "${SERVICES_ROOT}/etc/systemd/resolved.conf")" "[Resolve]" "a whole [Resolve] section is appended when absent" + +reset_state +sysrm etc/systemd/resolved.conf +if hosts_fix_resolved >/dev/null; then :; fi +_t_pass "a missing resolved.conf is a no-op rather than an error" + +echo "== hosts_repair_all: dry-run refuses to claim success ==" +reset_state +stage_hosts_ok +make_installer "$HOSTS_INSTALL_SCRIPT" +make_installer "$GUARD_LIB_MIGRATE_SCRIPT" +DRY_RUN=1 +if hosts_repair_all >/dev/null; then + _t_fail "hosts_repair_all returns non-zero under --dry-run" +else + _t_pass "hosts_repair_all returns non-zero under --dry-run" +fi + +echo "== hosts_repair_all: a healthy machine verifies after repair ==" +reset_state +stage_hosts_ok +make_installer "$HOSTS_INSTALL_SCRIPT" +make_installer "$GUARD_LIB_MIGRATE_SCRIPT" +if hosts_repair_all >/dev/null; then + _t_pass "hosts_repair_all returns zero when the post-repair check passes" +else + _t_fail "hosts_repair_all returns zero when the post-repair check passes" +fi + +_t_summary diff --git a/linux_configuration/scripts/periodic_background/lib/tests/test_services_hosts_resolved.sh b/linux_configuration/scripts/periodic_background/lib/tests/test_services_hosts_resolved.sh new file mode 100755 index 00000000..f3668c5b --- /dev/null +++ b/linux_configuration/scripts/periodic_background/lib/tests/test_services_hosts_resolved.sh @@ -0,0 +1,73 @@ +#!/usr/bin/env bash +# Tests for the systemd-resolved half of check_hosts (lib/services_hosts.sh). +# +# Split out of test_services_hosts.sh for the repo-wide 250-line cap. Every +# case here is a way systemd-resolved can answer a query WITHOUT consulting +# /etc/hosts, which leaves the whole blocking stack inert while a correct, +# immutable, fully-populated hosts file sits on disk: ReadEtcHosts turned off, +# DNSOverTLS turned on, or either re-enabled by a drop-in override. +set -euo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" + +# shellcheck source=services_harness.sh +. "${SCRIPT_DIR}/services_harness.sh" +# shellcheck source=../services_hosts_fix.sh +. "${SCRIPT_DIR}/../services_hosts_fix.sh" +# shellcheck source=../services_hosts.sh +. "${SCRIPT_DIR}/../services_hosts.sh" + +_t_called_in() { # + if [[ "$1" == *"$2"* ]]; then + _t_pass "$3" + else + _t_fail "$3 (missing '$2')" + fi +} + +echo "== check_hosts: ReadEtcHosts=no is an error and is repaired ==" +reset_state +stage_hosts_ok +make_installer "$HOSTS_INSTALL_SCRIPT" +make_installer "$GUARD_LIB_MIGRATE_SCRIPT" +printf '[Resolve]\nReadEtcHosts=no\n' >"${SERVICES_ROOT}/etc/systemd/resolved.conf" +check_hosts >"${TEST_TMPDIR}/out.txt" +_t_called_in "$(cat "${TEST_TMPDIR}/out.txt")" "bypassed by systemd-resolved" "the resolved bypass is called out" +_t_called_in "$(cat "${SERVICES_ROOT}/etc/systemd/resolved.conf")" "ReadEtcHosts=yes" "the setting is repaired" +_t_called 'systemctl restart systemd-resolved' "systemd-resolved is restarted so the change takes" + +echo "== check_hosts: DNSOverTLS enabled is an error and is repaired ==" +reset_state +stage_hosts_ok +make_installer "$HOSTS_INSTALL_SCRIPT" +make_installer "$GUARD_LIB_MIGRATE_SCRIPT" +printf '[Resolve]\nReadEtcHosts=yes\nDNSOverTLS=yes\n' >"${SERVICES_ROOT}/etc/systemd/resolved.conf" +check_hosts >"${TEST_TMPDIR}/out.txt" +# The message interpolates $SYSROOT, so match on the stable part of it. +_t_called_in "$(cat "${TEST_TMPDIR}/out.txt")" "DNSOverTLS='yes'" "DNSOverTLS is called out as a bypass" +_t_called_in "$(cat "${SERVICES_ROOT}/etc/systemd/resolved.conf")" "#DNSOverTLS=no" "it is commented out" + +echo "== check_hosts: drop-in overrides are an error and are removed ==" +reset_state +stage_hosts_ok +make_installer "$HOSTS_INSTALL_SCRIPT" +make_installer "$GUARD_LIB_MIGRATE_SCRIPT" +# A drop-in can re-enable either bypass without touching resolved.conf, so the +# repairs above are not durable while one survives. +sysfile etc/systemd/resolved.conf.d/99-override.conf +check_hosts >"${TEST_TMPDIR}/out.txt" +_t_called_in "$(cat "${TEST_TMPDIR}/out.txt")" "drop-in override" "the drop-in is called out" +if [[ -f "${SERVICES_ROOT}/etc/systemd/resolved.conf.d/99-override.conf" ]]; then + _t_fail "the drop-in is deleted" +else + _t_pass "the drop-in is deleted" +fi + +echo "== check_hosts: a missing resolved.conf warns ==" +reset_state +stage_hosts_ok +sysrm etc/systemd/resolved.conf +check_hosts >"${TEST_TMPDIR}/out.txt" +_t_called_in "$(cat "${TEST_TMPDIR}/out.txt")" "resolved.conf does not exist" "the missing file is reported" + +_t_summary diff --git a/linux_configuration/scripts/periodic_background/lib/tests/test_services_units.sh b/linux_configuration/scripts/periodic_background/lib/tests/test_services_units.sh index 226fd724..21f5cf6c 100755 --- a/linux_configuration/scripts/periodic_background/lib/tests/test_services_units.sh +++ b/linux_configuration/scripts/periodic_background/lib/tests/test_services_units.sh @@ -58,15 +58,13 @@ stage_midnight_files : >"${DEV}/enabled" : >"${DEV}/active" check_midnight_shutdown >/dev/null -# LATENT BUG, pinned deliberately rather than fixed here: the is-active check -# assigns status="warning" unconditionally, so it overwrites the "error" the -# is-enabled check set immediately above. A timer that is BOTH disabled and -# inactive therefore ends up "warning", and report_and_fix only repairs on -# "error" -- so the timer is never re-enabled. Identical in the pre-split file -# (verified against rev 6691cb26), so this split neither caused nor hides it. -# Fixing it changes enforcement behaviour and belongs in its own commit. -_t_eq "warning" "$(get_service_status "midnight_shutdown")" "disabled+inactive is (wrongly) only a warning" -_t_not_called 'ran setup_midnight_shutdown' "so the disabled timer is never repaired" +# Regression test. The is-active check used to assign status="warning" +# unconditionally, overwriting the "error" the is-enabled check set just above; +# since report_and_fix only repairs on "error", a timer that was BOTH disabled +# and inactive -- the normal shape of a genuinely broken timer -- was reported +# and then never re-enabled. +_t_eq "error" "$(get_service_status "midnight_shutdown")" "disabled+inactive stays an error" +_t_called 'ran setup_midnight_shutdown.sh enable' "so the disabled timer IS repaired" echo "== check_midnight_shutdown: --status reports without repairing ==" reset_state @@ -76,7 +74,7 @@ STATUS_ONLY=1 : >"${DEV}/enabled" check_midnight_shutdown >/dev/null _t_not_called 'ran setup_midnight_shutdown' "--status never runs the setup script" -_t_eq "warning" "$(get_service_status "midnight_shutdown")" "the verdict is still recorded" +_t_eq "error" "$(get_service_status "midnight_shutdown")" "the error is still recorded" echo "== check_startup_monitor: fully healthy records ok ==" reset_state @@ -104,9 +102,9 @@ sysfile usr/local/bin/pc-startup-check.sh : >"${DEV}/enabled" : >"${DEV}/active" check_startup_monitor >/dev/null -# Same latent downgrade as check_midnight_shutdown above. -_t_eq "warning" "$(get_service_status "startup_monitor")" "disabled+inactive is (wrongly) only a warning" -_t_not_called 'ran setup_pc_startup_monitor' "so the disabled timer is never repaired" +# Same regression as check_midnight_shutdown above. +_t_eq "error" "$(get_service_status "startup_monitor")" "disabled+inactive stays an error" +_t_called 'ran setup_pc_startup_monitor' "so the disabled timer IS repaired" echo "== check_startup_monitor: enabled but inactive is a warning, not a fix ==" reset_state @@ -153,10 +151,10 @@ sysfile usr/local/bin/periodic-system-maintenance.sh : >"${DEV}/enabled" : >"${DEV}/active" check_periodic_systems >/dev/null -# hosts-file-monitor.service's is-active check is the last status assignment -# here, and it too downgrades unconditionally -- same bug, same pinning. -_t_eq "warning" "$(get_service_status "periodic_systems")" "a fully disabled stack is (wrongly) only a warning" -_t_not_called 'ran setup_periodic_system' "so the disabled stack is never repaired" +# hosts-file-monitor.service's is-active check is the LAST status assignment in +# this function, so before the fix it downgraded everything above it. +_t_eq "error" "$(get_service_status "periodic_systems")" "a fully disabled stack stays an error" +_t_called 'ran setup_periodic_system' "so the disabled stack IS repaired" echo "== check_periodic_systems: startup service disabled alone still repairs ==" reset_state