feat(archsetup): back up system files before in-place edits

Add a backup_system_file helper that snapshots a pre-existing file to <path>.archsetup.bak before archsetup edits it in place, so a botched edit to fstab, mkinitcpio.conf, or sudoers is recoverable. It is idempotent: it never overwrites an existing backup, so the pristine original survives repeated edits within a run and across re-runs. It uses cp -p to preserve mode and ownership.

Only the in-place sed and append edits to pre-existing files route through it (locale.gen, makepkg.conf, pacman.conf, sudoers, wireless-regdom, geoclue.conf, pacman-contrib, fstab, mkinitcpio.conf, vconsole.conf). The brand-new drop-in files archsetup fully owns are skipped: there is no prior state to save, and recovery is just deleting them.

Covered by tests/backup-system-file/ (Normal, Boundary, Error cases, including mode preservation and the no-overwrite guarantee).

3 files changed, 203 insertions, 1 deletions

diff --git a/archsetup b/archsetup
index 6b6a273..e677daa 100755
--- a/archsetup
+++ b/archsetup
@@ -381,6 +381,32 @@ safe_rm_rf() {
     rm -rf "$target"
 }
 
+# backup_system_file <path>
+# Snapshot a pre-existing system file to <path>.archsetup.bak before archsetup
+# edits it in place, so a botched in-place edit (fstab, mkinitcpio.conf,
+# sudoers, ...) is recoverable. Idempotent: never overwrites an existing
+# backup, so the pristine original survives repeated edits within a run and
+# across re-runs of the installer. Returns 0 and does nothing when <path>
+# does not exist (nothing to back up) or when a backup is already present.
+# Uses `cp -p` so a restored sudoers/fstab keeps its mode and ownership.
+# Prints its own warning to stderr and returns non-zero only when the copy
+# itself fails. Self-contained — no dependency on error_warn.
+backup_system_file() {
+    local target="$1"
+    local backup="${target}.archsetup.bak"
+
+    if [ -z "$target" ]; then
+        echo "backup_system_file: empty target" >&2
+        return 1
+    fi
+    [ -f "$target" ] || return 0       # nothing to back up
+    [ -e "$backup" ] && return 0       # pristine backup already captured
+    if ! cp -p -- "$target" "$backup"; then
+        echo "backup_system_file: failed to back up '$target'" >&2
+        return 1
+    fi
+}
+
 # Handle --status flag (must be after state_dir is defined)
 if [ "$show_status_only" = "true" ]; then
     show_status
@@ -891,6 +917,7 @@ prerequisites() {
         action="configuring locale ($locale)" && display "task" "$action"
         # Uncomment the selected locale in locale.gen (format: "en_US.UTF-8 UTF-8")
         locale_entry="${locale} ${locale##*.}"  # e.g., "en_US.UTF-8 UTF-8"
+        backup_system_file /etc/locale.gen
         sed -i "s|^#${locale_entry}|${locale_entry}|" /etc/locale.gen
         (locale-gen >> "$logfile" 2>&1) || error_warn "$action" "$?"
         echo "LANG=$locale" > /etc/locale.conf
@@ -911,6 +938,7 @@ prerequisites() {
     systemctl enable chronyd.service >> "$logfile" 2>&1 || error_warn "$action" "$?"
 
     action="configuring compiler to use all processor cores" && display "task" "$action"
+    backup_system_file /etc/makepkg.conf
     sed -i "s/-j2/-j$(nproc)/;s/^#MAKEFLAGS/MAKEFLAGS/" /etc/makepkg.conf >> "$logfile" 2>&1
 
     action="disabling debug packages in makepkg" && display "task" "$action"
@@ -918,6 +946,7 @@ prerequisites() {
 
     # enable pacman concurrent downloads and color
     action="enabling concurrent downloads" && display "task" "$action"
+    backup_system_file /etc/pacman.conf
     sed -i "s/^#ParallelDownloads.*$/ParallelDownloads = 10/;s/^#Color$/Color/" /etc/pacman.conf
 
     # enable multilib repository (required for 32-bit libraries, Steam, etc.)
@@ -982,6 +1011,7 @@ create_user() {
 
     # give $username sudo nopasswd rights (required for aur installs)
     display "task" "granting permissions"
+    backup_system_file /etc/sudoers
     (echo "%$username ALL=(ALL) NOPASSWD: ALL" >> /etc/sudoers) \
         || error_warn "$action" "$?"
 
@@ -1227,6 +1257,7 @@ EOF
     current_lang="${LANG:-en_US.UTF-8}"
     wireless_region="${current_lang:3:2}"  # extract country code (positions 3-4)
     action="configuring wireless regulatory domain ($wireless_region)" && display "task" "$action"
+    backup_system_file /etc/conf.d/wireless-regdom
     sed -i "s|^#WIRELESS_REGDOM=\"${wireless_region}\"|WIRELESS_REGDOM=\"${wireless_region}\"|" /etc/conf.d/wireless-regdom
 
     # Encrypted DNS (DNS over TLS)
@@ -1385,12 +1416,14 @@ EOF
     # Enable BeaconDB as geoclue wifi location provider (default MLS/Ichnaea API is defunct)
     action="configuring geoclue to use BeaconDB location service" && display "task" "$action"
     if grep -q '^#url=https://api.beacondb.net/v1/geolocate' /etc/geoclue/geoclue.conf 2>/dev/null; then
+        backup_system_file /etc/geoclue/geoclue.conf
         sed -i 's|^#url=https://api.beacondb.net/v1/geolocate|url=https://api.beacondb.net/v1/geolocate|' /etc/geoclue/geoclue.conf
     fi
 
     # Whitelist gammastep in geoclue config (geoclue demo agent is started via hyprland.conf exec-once)
     action="whitelisting gammastep in geoclue" && display "task" "$action"
     if ! grep -q "^\[gammastep\]" /etc/geoclue/geoclue.conf 2>/dev/null; then
+        backup_system_file /etc/geoclue/geoclue.conf
         cat >> /etc/geoclue/geoclue.conf << 'EOF'
 
 [gammastep]
@@ -1432,6 +1465,7 @@ EOF
     systemctl enable --now paccache.timer  >> "$logfile" 2>&1 || error_warn "$action" "$?"
 
     action="configuring paccache to keep 3 versions" && display "task" "$action"
+    backup_system_file /etc/conf.d/pacman-contrib
     sed -i 's/^PACCACHE_ARGS=.*/PACCACHE_ARGS=-k3/' /etc/conf.d/pacman-contrib
 
     # Snapshot Service - filesystem-aware
@@ -2445,6 +2479,7 @@ boot_ux() {
     if grep -qE "^[^#].*[[:space:]]/efi[[:space:]]+vfat[[:space:]]" /etc/fstab \
        && ! grep -E "^[^#].*[[:space:]]/efi[[:space:]]+vfat[[:space:]]" /etc/fstab | grep -q "fmask="; then
         action="tightening /efi mount permissions in fstab" && display "task" "$action"
+        backup_system_file /etc/fstab
         sed -i -E '/^[^#].*[[:space:]]\/efi[[:space:]]+vfat[[:space:]]/ s/([[:space:]]+vfat[[:space:]]+)([^[:space:]]+)/\1\2,fmask=0177,dmask=0077/' /etc/fstab \
             || error_warn "$action" "$?"
     fi
@@ -2453,6 +2488,7 @@ boot_ux() {
     # Ensures NVMe devices are available when ZFS/other hooks try to access them
     if has_nvme_drives; then
         action="adding nvme to mkinitcpio MODULES for early loading" && display "task" "$action"
+        backup_system_file /etc/mkinitcpio.conf
         if grep -q "^MODULES=()" /etc/mkinitcpio.conf; then
             sed -i 's/^MODULES=()/MODULES=(nvme)/' /etc/mkinitcpio.conf
         elif grep -q "^MODULES=(" /etc/mkinitcpio.conf && ! grep -q "nvme" /etc/mkinitcpio.conf; then
@@ -2468,6 +2504,7 @@ boot_ux() {
         error_warn "$action" "$?"
 
     action="configuring console font" && display "task" "$action"
+    backup_system_file /etc/vconsole.conf
     if grep -q "^FONT=" /etc/vconsole.conf 2>/dev/null; then
         sed -i 's/^FONT=.*/FONT=ter-132n/' /etc/vconsole.conf
     else
@@ -2478,6 +2515,7 @@ boot_ux() {
     # ZFS initramfs hook is busybox-based and incompatible with systemd hook
     if ! is_zfs_root; then
         action="delegating fsck messages from udev to systemd" && display "task" "$action"
+        backup_system_file /etc/mkinitcpio.conf
         sed -i '/^HOOKS=/ s/\budev\b/systemd/' /etc/mkinitcpio.conf  || error_warn "$action" "$?"
         mkinitcpio -P  >> "$logfile" 2>&1  || error_warn "running mkinitcpio -P to silence fsck messages" "$?"
     fi
diff --git a/tests/backup-system-file/test_backup_system_file.py b/tests/backup-system-file/test_backup_system_file.py
new file mode 100644
index 0000000..5d48d03
--- /dev/null
+++ b/tests/backup-system-file/test_backup_system_file.py
@@ -0,0 +1,161 @@
+"""Tests for the backup_system_file helper in the archsetup installer.
+
+backup_system_file snapshots a pre-existing system file to
+`<path>.archsetup.bak` before archsetup edits it in place, so a botched
+in-place edit (fstab, mkinitcpio.conf, sudoers, ...) is recoverable. It is
+idempotent: it never overwrites an existing backup, so the pristine original
+survives repeated edits within a run and across re-runs of the installer. It
+no-ops (success) when the target does not exist.
+
+These tests exercise the REAL function body, extracted from the `archsetup`
+script at run time (not a copy), so the production code path is what runs.
+Edits run against real temp files the test creates and tears down.
+
+Run from repo root:
+    python3 -m unittest tests.backup-system-file.test_backup_system_file
+"""
+
+import os
+import shutil
+import stat
+import subprocess
+import tempfile
+import unittest
+
+
+REPO_ROOT = os.path.abspath(os.path.join(os.path.dirname(__file__), "..", ".."))
+ARCHSETUP = os.path.join(REPO_ROOT, "archsetup")
+
+
+class BackupHarness(unittest.TestCase):
+    """Source backup_system_file out of the real archsetup script and invoke it."""
+
+    def setUp(self):
+        self.tmp = tempfile.mkdtemp(prefix="backup-system-file-test-")
+        # A bash wrapper that extracts just the backup_system_file function from
+        # the real installer and invokes it with the test's arg. Sourcing the
+        # sed-extracted function means we test the production code path, not a
+        # reimplementation. The helper is self-contained (prints its own
+        # warnings), so no logger stub is needed.
+        self.wrapper = os.path.join(self.tmp, "run.sh")
+        with open(self.wrapper, "w") as f:
+            f.write(
+                "#!/bin/bash\n"
+                'ARCHSETUP="$1"; shift\n'
+                "source <(sed -n '/^backup_system_file() {/,/^}/p' \"$ARCHSETUP\")\n"
+                'backup_system_file "$@"\n'
+            )
+        os.chmod(self.wrapper, 0o755)
+
+    def tearDown(self):
+        # Restore writability in case a test made a dir read-only.
+        for root, dirs, _ in os.walk(self.tmp):
+            for d in dirs:
+                os.chmod(os.path.join(root, d), 0o755)
+        shutil.rmtree(self.tmp, ignore_errors=True)
+
+    def run_backup(self, target):
+        return subprocess.run(
+            ["bash", self.wrapper, ARCHSETUP, target],
+            capture_output=True, text=True, timeout=10,
+        )
+
+    def write(self, name, content, mode=None):
+        path = os.path.join(self.tmp, name)
+        with open(path, "w") as f:
+            f.write(content)
+        if mode is not None:
+            os.chmod(path, mode)
+        return path
+
+
+# -----------------------------------------------------------------------------
+# Normal cases
+# -----------------------------------------------------------------------------
+
+class TestBackupNormal(BackupHarness):
+
+    def test_existing_file_is_backed_up_with_same_content(self):
+        target = self.write("fstab", "UUID=abc / ext4 defaults 0 1\n")
+        result = self.run_backup(target)
+        self.assertEqual(result.returncode, 0, msg=result.stderr)
+        backup = target + ".archsetup.bak"
+        self.assertTrue(os.path.isfile(backup), "backup should be created")
+        with open(backup) as f:
+            self.assertEqual(f.read(), "UUID=abc / ext4 defaults 0 1\n")
+
+    def test_backup_preserves_mode(self):
+        # sudoers ships 0440; a restored backup must keep restrictive perms.
+        target = self.write("sudoers", "root ALL=(ALL) ALL\n", mode=0o440)
+        result = self.run_backup(target)
+        self.assertEqual(result.returncode, 0, msg=result.stderr)
+        backup = target + ".archsetup.bak"
+        self.assertEqual(stat.S_IMODE(os.stat(backup).st_mode), 0o440)
+
+
+# -----------------------------------------------------------------------------
+# Boundary cases
+# -----------------------------------------------------------------------------
+
+class TestBackupBoundary(BackupHarness):
+
+    def test_existing_backup_is_not_overwritten(self):
+        # The pristine original must survive a later edit + second backup call.
+        target = self.write("pacman.conf", "PRISTINE\n")
+        self.assertEqual(self.run_backup(target).returncode, 0)
+        # Simulate archsetup editing the file in place, then backing up again.
+        with open(target, "w") as f:
+            f.write("EDITED\n")
+        result = self.run_backup(target)
+        self.assertEqual(result.returncode, 0, msg=result.stderr)
+        with open(target + ".archsetup.bak") as f:
+            self.assertEqual(f.read(), "PRISTINE\n", "backup must stay pristine")
+
+    def test_missing_target_is_a_quiet_noop(self):
+        target = os.path.join(self.tmp, "never-existed.conf")
+        result = self.run_backup(target)
+        self.assertEqual(result.returncode, 0, msg=result.stderr)
+        self.assertFalse(os.path.exists(target + ".archsetup.bak"))
+
+    def test_second_call_same_run_is_a_noop(self):
+        # A file edited twice in one run (e.g. mkinitcpio MODULES then HOOKS)
+        # gets backed up once; the second call must not error or re-copy.
+        target = self.write("mkinitcpio.conf", "HOOKS=(base udev)\n")
+        self.assertEqual(self.run_backup(target).returncode, 0)
+        backup = target + ".archsetup.bak"
+        first_mtime = os.stat(backup).st_mtime_ns
+        result = self.run_backup(target)
+        self.assertEqual(result.returncode, 0, msg=result.stderr)
+        self.assertEqual(os.stat(backup).st_mtime_ns, first_mtime,
+                         "backup must not be rewritten on the second call")
+
+
+# -----------------------------------------------------------------------------
+# Error cases
+# -----------------------------------------------------------------------------
+
+class TestBackupErrors(BackupHarness):
+
+    def test_empty_target_is_refused(self):
+        result = self.run_backup("")
+        self.assertNotEqual(result.returncode, 0)
+
+    def test_copy_failure_returns_nonzero(self):
+        # Target exists but its directory is read-only, so the .bak can't be
+        # written. The helper must report failure rather than silently skip.
+        subdir = os.path.join(self.tmp, "ro")
+        os.makedirs(subdir)
+        target = os.path.join(subdir, "fstab")
+        with open(target, "w") as f:
+            f.write("data\n")
+        os.chmod(subdir, 0o500)  # r-x: owner cannot create the .bak here
+        try:
+            result = self.run_backup(target)
+        finally:
+            os.chmod(subdir, 0o755)
+        self.assertNotEqual(result.returncode, 0)
+        self.assertFalse(os.path.exists(target + ".archsetup.bak"))
+
+
+if __name__ == "__main__":
+    unittest.main()
diff --git a/todo.org b/todo.org
index 158ed03..eefaf3a 100644
--- a/todo.org
+++ b/todo.org
@@ -521,7 +521,8 @@ Some operations log to ~$logfile~, others don't - standardize logging
 All package installs should log, all system modifications should log, all errors should log with context
 Makes debugging failed installations easier
 
-** TODO [#B] Add backup before system file modifications :solo:
+** DONE [#B] Add backup before system file modifications :solo:
+CLOSED: [2026-06-25 Thu]
 :PROPERTIES:
 :LAST_REVIEWED: 2026-06-24
 :END:
@@ -529,6 +530,8 @@ Safety net for /etc/X11/xorg.conf.d and other system file edits
 Files like ~/etc/sudoers~, ~/etc/pacman.conf~, ~/etc/default/grub~ modified without backup
 If modifications fail or are incorrect, difficult to recover - should backup files to ~.backup~ before modifying
 
+Done 2026-06-25: added a =backup_system_file <path>= helper next to =safe_rm_rf= — it snapshots a pre-existing file to =<path>.archsetup.bak= before an in-place edit, idempotent (never clobbers an existing backup, so the pristine original survives repeated edits and re-runs), =cp -p= to preserve mode/ownership, no-op when the file is absent. Took the narrow scope (Craig's call): route only the in-place =sed -i= / append edits to *pre-existing* files through it — locale.gen, makepkg.conf, pacman.conf, sudoers, conf.d/wireless-regdom, geoclue.conf, conf.d/pacman-contrib, fstab, mkinitcpio.conf, vconsole.conf — and skip the brand-new drop-in files archsetup fully owns (nothing to back up; recovery is just deleting them). Tests: =tests/backup-system-file/= (7 Normal/Boundary/Error, incl. mode-preserved, existing-backup-not-overwritten, missing-target no-op, cp-failure). =make test-unit= green across all 5 suites; =bash -n= clean; only shellcheck note is the known SC2329 false positive (indirect STEPS dispatch). Integration verification is the next VM run.
+
 ** TODO [#B] Implement Testinfra test suite for archsetup
 :PROPERTIES:
 :LAST_REVIEWED: 2026-06-24