refactor(install): address audit findings (stdlib reuse, dead-code, comment hygiene)

src/cli/install/mappings.zig

@@ -218,15 +218,6 @@ pub fn writeBinding(
         }
     }
 
-    if (existing != null and (conflict_mode == .force or conflict_mode == .interactive)) {
-        backupFile(allocator, config_path) catch |err| {
-            var errbuf: [256]u8 = undefined;
-            const msg = std.fmt.bufPrint(&errbuf, "error: cannot create backup of {s}: {}, aborting overwrite\n", .{ config_path, err }) catch "error: backup failed, aborting\n";
-            _ = std.posix.write(std.posix.STDERR_FILENO, msg) catch {};
-            return err;
-        };
-    }
-
     var has_target = false;
     if (devices) |devs| {
         for (devs) |d| {
@@ -236,6 +227,17 @@ pub fn writeBinding(
             }
         }
     }
+
+    // Only back up when an existing entry is actually being overwritten; a
+    // pure add (no matching entry) changes nothing that needs a backup.
+    if (has_target and (conflict_mode == .force or conflict_mode == .interactive)) {
+        backupFile(allocator, config_path) catch |err| {
+            var errbuf: [256]u8 = undefined;
+            const msg = std.fmt.bufPrint(&errbuf, "error: cannot create backup of {s}: {}, aborting overwrite\n", .{ config_path, err }) catch "error: backup failed, aborting\n";
+            _ = std.posix.write(std.posix.STDERR_FILENO, msg) catch {};
+            return err;
+        };
+    }
     const old_count = if (devices) |d| d.len else 0;
     const new_count = if (has_target) old_count else old_count + 1;
     var new_devices = try allocator.alloc(user_config_mod.DeviceEntry, new_count);

src/cli/install/phase.zig

@@ -14,7 +14,7 @@ pub var test_probe_alive_override: ?*const fn (path: []const u8) bool = null;
 
 /// Test hook: when non-null, uninstall prefixes runtime paths
 /// (/run/padctl/padctl.sock, .pid) with this root instead of "" so the
-/// PR-2 daemon-stop probe path can be exercised against a tmpdir without
+/// daemon-stop probe path can be exercised against a tmpdir without
 /// flipping the lifecycle scope to .package via opts.destdir.
 pub var test_runtime_root_override: ?[]const u8 = null;
 

src/cli/install/plan.zig

@@ -79,31 +79,8 @@ pub fn writeAll(fd: std.posix.fd_t, s: []const u8) void {
     _ = std.posix.write(fd, s) catch {};
 }
 
-pub fn ensureDir(path: []const u8) !void {
-    std.fs.makeDirAbsolute(path) catch |err| switch (err) {
-        error.PathAlreadyExists => {},
-        else => return err,
-    };
-}
-
-pub fn ensureDirAll(allocator: std.mem.Allocator, path: []const u8) !void {
-    var components = std.ArrayList([]const u8){};
-    defer components.deinit(allocator);
-
-    var remaining = path;
-    while (remaining.len > 1) {
-        try components.append(allocator, remaining);
-        remaining = std.fs.path.dirname(remaining) orelse break;
-    }
-
-    var i: usize = components.items.len;
-    while (i > 0) {
-        i -= 1;
-        ensureDir(components.items[i]) catch |err| switch (err) {
-            error.PathAlreadyExists => {},
-            else => return err,
-        };
-    }
+pub fn ensureDirAll(_: std.mem.Allocator, path: []const u8) !void {
+    try std.fs.cwd().makePath(path);
 }
 
 pub fn dirExistsAbsolute(path: []const u8) bool {
@@ -122,19 +99,9 @@ pub fn dirIsNonEmpty(path: []const u8) bool {
     return false;
 }
 
+// Atomic copy (temp + rename) preserving source mode. No partial dst on failure.
 pub fn copyFile(src: []const u8, dst: []const u8) !void {
-    var src_file = try std.fs.openFileAbsolute(src, .{});
-    defer src_file.close();
-    const stat = try src_file.stat();
-    var dst_file = try std.fs.createFileAbsolute(dst, .{ .truncate = true });
-    defer dst_file.close();
-    try dst_file.chmod(stat.mode & 0o777);
-    var buf: [65536]u8 = undefined;
-    while (true) {
-        const n = try src_file.read(&buf);
-        if (n == 0) break;
-        try dst_file.writeAll(buf[0..n]);
-    }
+    try std.fs.copyFileAbsolute(src, dst, .{});
 }
 
 // Write to {dst}.new then rename(2) over dst — avoids ETXTBSY when dst is currently executing.
@@ -251,7 +218,8 @@ pub fn hostHasInputGroup() bool {
 pub fn userInGroup(name: []const u8) bool {
     const target_gid = groupGid(name) orelse return false;
     if (std.os.linux.getegid() == target_gid) return true;
-    var gids: [64]std.os.linux.gid_t = undefined;
+    // 1024 covers heavy LDAP/AD memberships; getgroups fails (-1) if exceeded.
+    var gids: [1024]std.os.linux.gid_t = undefined;
     const ret = std.os.linux.getgroups(gids.len, &gids[0]);
     if (ret > gids.len) return false;
     for (gids[0..ret]) |g| {
@@ -334,9 +302,8 @@ pub const EnvSnapshot = struct {
 
 /// Single source of truth for every decision `install.run()` makes before it
 /// starts touching the filesystem. All derived axes are computed exactly once
-/// in `compute()` so later phases read a plain struct instead of re-deriving.
-/// All derived axes are computed exactly once so later phases read a plain
-/// struct instead of re-deriving, preventing decision drift between call sites.
+/// in `compute()` so later phases read a plain struct instead of re-deriving,
+/// preventing decision drift between call sites.
 pub const InstallPlan = struct {
     // --- inputs (captured, not re-derived) ---
     opts: InstallOptions,
@@ -348,7 +315,7 @@ pub const InstallPlan = struct {
     // --- single source of truth ---
     scope: LifecycleScope,
 
-    // --- derived axes (computed-from-scope shims; PR-4/PR-5 will remove) ---
+    // --- derived axes ---
     staging_mode: bool,
     effective_user_service: bool,
     immutable_kind: ImmutableKind,
@@ -387,8 +354,7 @@ pub const InstallPlan = struct {
 
         const staging_mode = scope == .package;
 
-        // user-service routing follows scope. Explicit --user-service still wins
-        // for transitional callers; PR-5 removes the orthogonal axis.
+        // user-service routing follows scope. Explicit --user-service still wins.
         const effective_user_service = opts.user_service orelse (scope == .user);
 
         const immutable_kind = detectImmutableOs(allocator, if (staging_mode) destdir else "");

src/cli/install/services.zig

@@ -255,7 +255,10 @@ pub fn buildSystemctlUserArgv(
     if (plan.mode == .skip) return null;
 
     var argv = std.ArrayList([]const u8){};
-    errdefer argv.deinit(allocator);
+    errdefer {
+        for (argv.items) |s| allocator.free(s);
+        argv.deinit(allocator);
+    }
 
     if (plan.mode == .sudo_hop) {
         try argv.append(allocator, try allocator.dupe(u8, "sudo"));
@@ -386,11 +389,16 @@ pub fn stopDaemonScope(allocator: std.mem.Allocator, scope: SystemctlScope) !voi
 
 fn buildSystemctlSystemArgv(allocator: std.mem.Allocator, verbs: []const []const u8) ![][]const u8 {
     var argv = try allocator.alloc([]const u8, 1 + verbs.len);
-    errdefer allocator.free(argv);
+    var filled: usize = 0;
+    errdefer {
+        for (argv[0..filled]) |s| allocator.free(s);
+        allocator.free(argv);
+    }
     argv[0] = try allocator.dupe(u8, "systemctl");
-    errdefer allocator.free(argv[0]);
+    filled = 1;
     for (verbs, 0..) |v, i| {
         argv[1 + i] = try allocator.dupe(u8, v);
+        filled = 2 + i;
     }
     return argv;
 }

src/cli/install/tests.zig

@@ -978,7 +978,7 @@ test "install: uninstall removes runtime state paths under system scope" {
         defer f.close();
     }
 
-    // PR-3: runtime paths are touched only in non-package scopes. Force
+    // runtime paths are touched only in non-package scopes. Force
     // scope=.system and redirect the path root to the staging tmpdir.
     phase_mod.test_runtime_root_override = staging;
     defer phase_mod.test_runtime_root_override = null;
@@ -2505,6 +2505,45 @@ test "install: writeBinding conflict with force - backup + overwrite" {
     try std.testing.expect(found_bak);
 }
 
+test "install: writeBinding force pure-add does not create backup" {
+    const testing_alloc = std.testing.allocator;
+
+    var tmp = std.testing.tmpDir(.{});
+    defer tmp.cleanup();
+    const destdir = try tmp.dir.realpathAlloc(testing_alloc, ".");
+    defer testing_alloc.free(destdir);
+
+    // Pre-existing config with an UNRELATED device entry.
+    try writeBinding(testing_alloc, destdir, "Vader", "vader_map", .skip, mockPromptKeep);
+
+    // Force-bind a NEW device: pure add, no entry overwritten.
+    try writeBinding(testing_alloc, destdir, "DualSense", "ds_map", .force, mockPromptKeep);
+
+    const config_path = try std.fmt.allocPrint(testing_alloc, "{s}/etc/padctl/config.toml", .{destdir});
+    defer testing_alloc.free(config_path);
+    const content = try std.fs.cwd().readFileAlloc(testing_alloc, config_path, 64 * 1024);
+    defer testing_alloc.free(content);
+
+    // New entry added, old entry preserved.
+    try std.testing.expect(std.mem.indexOf(u8, content, "default_mapping = \"ds_map\"") != null);
+    try std.testing.expect(std.mem.indexOf(u8, content, "default_mapping = \"vader_map\"") != null);
+
+    // No backup file: nothing was overwritten.
+    const etc_dir = try std.fmt.allocPrint(testing_alloc, "{s}/etc/padctl", .{destdir});
+    defer testing_alloc.free(etc_dir);
+    var dir = try std.fs.openDirAbsolute(etc_dir, .{ .iterate = true });
+    defer dir.close();
+    var found_bak = false;
+    var it = dir.iterate();
+    while (try it.next()) |entry| {
+        if (std.mem.startsWith(u8, entry.name, "config.toml.bak.")) {
+            found_bak = true;
+            break;
+        }
+    }
+    try std.testing.expect(!found_bak);
+}
+
 test "install: installMapping errors on missing source" {
     const testing = std.testing;
     const allocator = testing.allocator;
@@ -3951,7 +3990,7 @@ test "uninstall: removes 61-padctl-driver-block.rules" {
 }
 
 // ---------------------------------------------------------------------------
-// PR-3: LifecycleScope integration tests
+// LifecycleScope integration tests
 // ---------------------------------------------------------------------------
 
 const scope_mod = @import("scope.zig");
@@ -4122,7 +4161,7 @@ test "uninstall: user scope routes to user systemctl only" {
         try uninstall(allocator, opts);
     }
 
-    // scope=.user must NOT trigger a system-scope stop. The PR-2 probe-and-stop
+    // scope=.user must NOT trigger a system-scope stop. The probe-and-stop
     // path only fires when probeSocketAlive returns true, and there's no socket
     // here, so stopDaemonScope is never called — calls list stays empty.
     try testing.expectEqual(@as(usize, 0), ProbeRig.calls.items.len);

src/cli/install/udev.zig

@@ -86,7 +86,7 @@ pub fn writeServiceSentinel(allocator: std.mem.Allocator, plan: *const InstallPl
     const ds = es.getDaySeconds();
     const iso = std.fmt.bufPrint(&buf, "{d:0>4}-{d:0>2}-{d:0>2}T{d:0>2}:{d:0>2}:{d:0>2}Z", .{
         yd.year,
-        @as(u32, @intFromEnum(md.month)) + 1,
+        @as(u32, @intFromEnum(md.month)),
         @as(u32, md.day_index) + 1,
         ds.getHoursIntoDay(),
         ds.getMinutesIntoHour(),