Code review fixes: encryption/merge correctness, install-stage safety, runner cleanup

internal/archinstall/archinstall.go

@@ -141,13 +141,16 @@ type LvmConfiguration struct {
 // DiskEncryption mirrors archinstall's DiskEncryption.json(). encryption_type is
 // "luks" (encrypt the single root partition) or "lvm_on_luks" (encrypt the PV
 // partitions under LVM). partitions holds the encrypted partition obj_ids (the
-// root partition for luks, the PV partitions for lvm_on_luks); lvm_volumes holds
-// LV obj_ids for the luks_on_lvm variant. The password is supplied separately as
-// the top-level encryption_password field.
+// root partition for luks, the PV partitions for lvm_on_luks). The password is
+// supplied separately as the top-level encryption_password field.
 type DiskEncryption struct {
 	EncryptionType string   `json:"encryption_type"` // "luks"/"lvm_on_luks"
 	Partitions     []string `json:"partitions"`
-	LvmVolumes     []string `json:"lvm_volumes"`
+	// LvmVolumes carries LV obj_ids for a per-LV (luks-on-lvm) encryption
+	// topology. That topology is reserved/unused for now — no encryption_type
+	// currently populates it — so it always renders as an empty list. Kept on
+	// the struct so the JSON shape matches archinstall's DiskEncryption.json().
+	LvmVolumes []string `json:"lvm_volumes"`
 }
 
 // DiskConfig mirrors DiskLayoutConfiguration.json(). disk_encryption is nested
@@ -333,14 +336,16 @@ func Build(cfg *config.Config, geom Geometry, password string) (*Config, *Creds,
 // and lvm config. For lvm_on_luks the encrypted partitions are exactly the VG's
 // LvmPvs; for luks it is the single partition mounted at "/". archinstall rejects
 // LVM encryption with more than two PV partitions (device_handler.py) — this is
-// guarded here as well as in config validation.
+// guarded here as well as in config validation. Only "luks" and "lvm_on_luks"
+// are accepted (config validation restricts the set); a per-LV luks-on-lvm
+// topology is not implemented.
 //
-// VM-validation-pending: the encryption_type values, the partitions/lvm_volumes
-// obj_id wiring, and the 2-PV limit are reverse-engineered from archinstall source
-// and must be confirmed against a real archinstall run in a VM.
+// VM-validation-pending: the encryption_type values, the partitions obj_id
+// wiring, and the 2-PV limit are reverse-engineered from archinstall source and
+// must be confirmed against a real archinstall run in a VM.
 func buildEncryption(encType string, devices []Device, lvm *LvmConfiguration) (*DiskEncryption, error) {
 	switch encType {
-	case "lvm_on_luks", "luks_on_lvm":
+	case "lvm_on_luks":
 		if lvm == nil || len(lvm.VolGroups) == 0 {
 			return nil, fmt.Errorf("%s encryption requires an lvm layout", encType)
 		}
@@ -428,7 +433,7 @@ func (b *lvmBuilder) build(geom Geometry) ([]Device, *LvmConfiguration, error) {
 		}
 	}
 	if disk1PV == "" {
-		return nil, nil, fmt.Errorf("no LVM PV found on disk 1 (%s); expected a partition like %s", disk1, partDev(disk1, 2))
+		return nil, nil, fmt.Errorf("no LVM PV found on disk 1 (%s); expected a partition like %s", disk1, PartDev(disk1, 2))
 	}
 
 	// Disk 1: ESP + PV partition (no swap partition). Sizes computed from geometry.
@@ -575,22 +580,51 @@ type plainBuilder struct {
 }
 
 func (b *plainBuilder) build(geom Geometry) ([]Device, *LvmConfiguration, error) {
-	disk1 := b.esp.Device
-	espBytes := b.espBytes
+	rootFs := b.plain.Filesystem
+	devices, err := singleDiskRoot(b.esp, b.swap, b.espBytes, geom, rootSpec{
+		fsType:       rootFs,
+		mountOptions: []string{},
+		btrfs:        []any{},
+	})
+	if err != nil {
+		return nil, nil, err
+	}
+	return devices, nil, nil
+}
+
+// rootSpec describes the per-layout root partition that follows the shared ESP +
+// optional swap prefix on disk 1. Only these three fields differ between the
+// plain and btrfs layouts; everything else (Status/Type/Start/Size/Mountpoint at
+// "/"/Flags) is identical and supplied by singleDiskRoot.
+type rootSpec struct {
+	fsType       string
+	mountOptions []string
+	btrfs        []any
+}
+
+// singleDiskRoot builds the disk-1 layout shared by the plain and btrfs builders:
+// the ESP partition, an optional linux-swap partition (when swap.type is
+// "partition", sized from swap.size), and a root partition consuming the rest of
+// disk 1. The root partition's fs_type, mount options and btrfs subvolume list
+// come from spec; everything else is fixed. The newObjID() call order is ESP,
+// swap (if present), then root — matching both original builders so golden renders
+// stay byte-identical.
+func singleDiskRoot(esp config.ESPConfig, swap config.SwapConfig, espBytes uint64, geom Geometry, spec rootSpec) ([]Device, error) {
+	disk1 := esp.Device
 
 	disk1Total, ok := geom[disk1]
 	if !ok || disk1Total == 0 {
-		return nil, nil, fmt.Errorf("no geometry for disk 1 (%s)", disk1)
+		return nil, fmt.Errorf("no geometry for disk 1 (%s)", disk1)
 	}
 
 	parts := []Partition{espPartition(espBytes)}
 	offset := startOffset + espBytes
 
 	// Optional swap partition (plain/btrfs only). Sized from swap.size.
-	if b.swap.EffectiveType() == "partition" {
-		swapBytes, err := parseSize(b.swap.Size)
+	if swap.EffectiveType() == "partition" {
+		swapBytes, err := parseSize(swap.Size)
 		if err != nil {
-			return nil, nil, fmt.Errorf("swap size: %w", err)
+			return nil, fmt.Errorf("swap size: %w", err)
 		}
 		swapFs := "linux-swap"
 		parts = append(parts, Partition{
@@ -603,20 +637,20 @@ func (b *plainBuilder) build(geom Geometry) ([]Device, *LvmConfiguration, error)
 
 	used := offset + endReserve
 	if disk1Total <= used {
-		return nil, nil, fmt.Errorf("disk 1 (%s, %d bytes) too small for ESP+swap (%d bytes)", disk1, disk1Total, used)
+		return nil, fmt.Errorf("disk 1 (%s, %d bytes) too small for ESP+swap (%d bytes)", disk1, disk1Total, used)
 	}
 	rootBytes := roundDownMiB(disk1Total - used)
 
 	root := "/"
-	rootFs := b.plain.Filesystem
+	rootFs := spec.fsType
 	parts = append(parts, Partition{
 		ObjID: newObjID(), Status: "create", Type: "primary",
 		Start: bytes(offset), Size: bytes(rootBytes),
 		FsType: &rootFs, Mountpoint: &root,
-		MountOptions: []string{}, Flags: []string{}, Btrfs: []any{},
+		MountOptions: spec.mountOptions, Flags: []string{}, Btrfs: spec.btrfs,
 	})
 
-	return []Device{{Device: disk1, Wipe: true, Partitions: parts}}, nil, nil
+	return []Device{{Device: disk1, Wipe: true, Partitions: parts}}, nil
 }
 
 // --- btrfs layout -----------------------------------------------------------
@@ -642,39 +676,6 @@ type btrfsBuilder struct {
 }
 
 func (b *btrfsBuilder) build(geom Geometry) ([]Device, *LvmConfiguration, error) {
-	disk1 := b.esp.Device
-	espBytes := b.espBytes
-
-	disk1Total, ok := geom[disk1]
-	if !ok || disk1Total == 0 {
-		return nil, nil, fmt.Errorf("no geometry for disk 1 (%s)", disk1)
-	}
-
-	parts := []Partition{espPartition(espBytes)}
-	offset := startOffset + espBytes
-
-	if b.swap.EffectiveType() == "partition" {
-		swapBytes, err := parseSize(b.swap.Size)
-		if err != nil {
-			return nil, nil, fmt.Errorf("swap size: %w", err)
-		}
-		swapFs := "linux-swap"
-		parts = append(parts, Partition{
-			ObjID: newObjID(), Status: "create", Type: "primary",
-			Start: bytes(offset), Size: bytes(swapBytes),
-			FsType: &swapFs, MountOptions: []string{}, Flags: []string{"swap"}, Btrfs: []any{},
-		})
-		offset += swapBytes
-	}
-
-	used := offset + endReserve
-	if disk1Total <= used {
-		return nil, nil, fmt.Errorf("disk 1 (%s, %d bytes) too small for ESP+swap (%d bytes)", disk1, disk1Total, used)
-	}
-	rootBytes := roundDownMiB(disk1Total - used)
-
-	root := "/"
-	btrfsFs := "btrfs"
 	mountOpts := []string{}
 	if b.btrfs.Compress != "" {
 		mountOpts = append(mountOpts, "compress="+b.btrfs.Compress)
@@ -687,14 +688,15 @@ func (b *btrfsBuilder) build(geom Geometry) ([]Device, *LvmConfiguration, error)
 		subvols = append(subvols, BtrfsSubvolume{Name: sv.Name, Mountpoint: &mp})
 	}
 
-	parts = append(parts, Partition{
-		ObjID: newObjID(), Status: "create", Type: "primary",
-		Start: bytes(offset), Size: bytes(rootBytes),
-		FsType: &btrfsFs, Mountpoint: &root,
-		MountOptions: mountOpts, Flags: []string{}, Btrfs: subvols,
+	devices, err := singleDiskRoot(b.esp, b.swap, b.espBytes, geom, rootSpec{
+		fsType:       "btrfs",
+		mountOptions: mountOpts,
+		btrfs:        subvols,
 	})
-
-	return []Device{{Device: disk1, Wipe: true, Partitions: parts}}, nil, nil
+	if err != nil {
+		return nil, nil, err
+	}
+	return devices, nil, nil
 }
 
 // --- helpers ----------------------------------------------------------------
@@ -728,8 +730,10 @@ func splitLocale(locale string) (lang, enc string) {
 	return locale, "UTF-8"
 }
 
-// partDev mirrors stages.partDev for the disk-1 PV error hint.
-func partDev(dev string, n int) string {
+// PartDev returns the kernel partition device for a base device and number:
+// /dev/sda -> /dev/sda1, but /dev/nvme0n1 -> /dev/nvme0n1p1. It is the single
+// shared definition; the stages package calls it rather than duplicating it.
+func PartDev(dev string, n int) string {
 	if len(dev) > 0 {
 		if last := dev[len(dev)-1]; last >= '0' && last <= '9' {
 			return fmt.Sprintf("%sp%d", dev, n)

internal/config/config.go

@@ -229,10 +229,16 @@ type Subvol struct {
 	Mountpoint string `yaml:"mountpoint" validate:"required"`
 }
 
-// Encryption enables LUKS. Type: "luks" (encrypt the single root partition,
-// for plain/btrfs) or "lvm_on_luks" (encrypt the PV partitions under LVM).
+// Encryption enables LUKS. Type is one of:
+//   - "luks": encrypt the single root partition (for the plain/btrfs layouts).
+//   - "lvm_on_luks": encrypt the PV partitions under LVM (for the lvm layout).
+//
+// The "luks_on_lvm" topology (encrypt individual LVs on top of an unencrypted
+// VG) is intentionally NOT accepted: it is unimplemented in the archinstall
+// renderer, so allowing it would silently produce the wrong (lvm_on_luks)
+// layout. Rejecting it here surfaces a clear validation error instead.
 type Encryption struct {
-	Type string `yaml:"type" validate:"required,oneof=luks lvm_on_luks luks_on_lvm"`
+	Type string `yaml:"type" validate:"required,oneof=luks lvm_on_luks"`
 }
 
 // SetupConfig drives the Phase B 85-setup stage, which runs after chezmoi has
@@ -269,8 +275,12 @@ type Clone struct {
 // Script and Dir have a leading `~` expanded to the user's home at run time.
 // Script existence is NOT checked at validate time: a hook script may be
 // produced by an earlier hook or stage in the same run.
+//
+// Name is required: it identifies the hook for layered-config merge-by-name (a
+// nameless element would silently flip the whole hooks slice from merge to
+// wholesale replace — see internal/configsrc/merge.go) and for diagnostics.
 type Hook struct {
-	Name   string            `yaml:"name"`
+	Name   string            `yaml:"name"   validate:"required"`
 	At     string            `yaml:"at"     validate:"required,hookpoint"`
 	Run    string            `yaml:"run"    validate:"required_without=Script"`
 	Script string            `yaml:"script" validate:"omitempty"`
@@ -509,7 +519,7 @@ func (c *Config) encryptionErrors() []error {
 	}
 	layout := d.EffectiveLayout()
 	switch d.Encryption.Type {
-	case "lvm_on_luks", "luks_on_lvm":
+	case "lvm_on_luks":
 		if layout != "lvm" {
 			errs = append(errs, fmt.Errorf("disks.encryption.type %s requires the lvm layout", d.Encryption.Type))
 		}

internal/config/encryption_luksonlvm_test.go

@@ -0,0 +1,48 @@
+package config
+
+import (
+	"strings"
+	"testing"
+)
+
+// TestEncryptionLuksOnLvmRejected pins that disks.encryption.type: luks_on_lvm is
+// no longer an accepted value. The archinstall renderer never implemented the
+// per-LV luks-on-lvm topology — it collapsed it into lvm_on_luks, encrypting the
+// PVs (the wrong layout) silently. Removing it from the oneof set makes Validate()
+// surface a clear "must be one of" error instead.
+func TestEncryptionLuksOnLvmRejected(t *testing.T) {
+	const y = `
+system:
+  hostname: arch
+  timezone: Europe/London
+  locale: en_GB.UTF-8
+  keymap: uk
+user:
+  name: adam
+pacstrap: [base-devel, git, zsh, sudo, networkmanager, efibootmgr, intel-ucode]
+kernel:
+  base: [linux]
+disks:
+  esp:
+    device: /dev/nvme0n1
+    size: 1GiB
+  swap:
+    type: swapfile
+    size: 4GiB
+  layout: lvm
+  lvm: {vg: vg0, lv: root, filesystem: ext4, pvs: [/dev/nvme0n1p2]}
+  encryption: {type: luks_on_lvm}
+`
+	err := validateYAML(t, y)
+	if err == nil {
+		t.Fatalf("want error for luks_on_lvm, got nil")
+	}
+	msg := err.Error()
+	if !strings.Contains(msg, "must be one of") {
+		t.Fatalf("error %q does not contain %q", msg, "must be one of")
+	}
+	// The accepted set must be named so the user knows the valid choices.
+	if !strings.Contains(msg, "luks") || !strings.Contains(msg, "lvm_on_luks") {
+		t.Fatalf("error %q should list the accepted types (luks, lvm_on_luks)", msg)
+	}
+}

internal/config/hook_name_required_test.go

@@ -0,0 +1,82 @@
+package config
+
+import (
+	"strings"
+	"testing"
+)
+
+// TestHookNameRequired pins that every hook must carry a name. A nameless hook
+// would silently flip the layered-config merge of the hooks list from
+// merge-by-name to wholesale replace (see internal/configsrc/merge.go), dropping
+// the base layer's hooks. Requiring a name fails validation instead, and the
+// failure is reported against the indexed YAML path hooks[N].name.
+func TestHookNameRequired(t *testing.T) {
+	const y = `
+system:
+  hostname: arch
+  timezone: Europe/London
+  locale: en_GB.UTF-8
+  keymap: uk
+user:
+  name: adam
+pacstrap: [base-devel, git, zsh, sudo, networkmanager, efibootmgr, intel-ucode]
+kernel:
+  base: [linux]
+disks:
+  esp:
+    device: /dev/nvme0n1
+    size: 1GiB
+  swap:
+    type: swapfile
+    size: 4GiB
+  layout: lvm
+  lvm: {vg: vg0, lv: root, filesystem: ext4, pvs: [/dev/nvme0n1p2]}
+hooks:
+  - at: post-install
+    run: echo hi
+`
+	err := validateYAML(t, y)
+	if err == nil {
+		t.Fatalf("want error for nameless hook, got nil")
+	}
+	msg := err.Error()
+	if !strings.Contains(msg, "hooks[0].name") {
+		t.Fatalf("error %q should reference hooks[0].name", msg)
+	}
+	if !strings.Contains(msg, "is required") {
+		t.Fatalf("error %q should say the field is required", msg)
+	}
+}
+
+// TestHookWithNameValid confirms a named hook still validates cleanly, so the new
+// requirement doesn't break correctly-written hooks.
+func TestHookWithNameValid(t *testing.T) {
+	const y = `
+system:
+  hostname: arch
+  timezone: Europe/London
+  locale: en_GB.UTF-8
+  keymap: uk
+user:
+  name: adam
+pacstrap: [base-devel, git, zsh, sudo, networkmanager, efibootmgr, intel-ucode]
+kernel:
+  base: [linux]
+disks:
+  esp:
+    device: /dev/nvme0n1
+    size: 1GiB
+  swap:
+    type: swapfile
+    size: 4GiB
+  layout: lvm
+  lvm: {vg: vg0, lv: root, filesystem: ext4, pvs: [/dev/nvme0n1p2]}
+hooks:
+  - name: greet
+    at: post-install
+    run: echo hi
+`
+	if err := validateYAML(t, y); err != nil {
+		t.Fatalf("want valid, got error: %v", err)
+	}
+}