Code review fixes: encryption/merge correctness, install-stage safety, runner cleanup - #10
Merged
Merged
Conversation
…uilders plainBuilder.build and btrfsBuilder.build were ~90% duplicated: probe disk1 geometry, build the ESP partition, optionally append a linux-swap partition, compute the remaining root bytes, and append the root partition. Only the root partition's fs_type, mount options and btrfs subvolume list differed. Extract singleDiskRoot(esp, swap, espBytes, geom, rootSpec) which builds the ESP + optional swap prefix and the root partition, taking the three varying fields via a small rootSpec struct. The newObjID() call order (ESP, swap, root) is preserved, so golden renders stay byte-identical and the archinstall golden tests pass without -update. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…RootShell/TryRoot Issue #8: delete Capture (zero callers), the Out field + outWriter helper (fed only the reverted TUI viewport), wiring Cmd/Shell straight to os.Stdout/os.Stderr. Drop now-unused bytes/io imports. Add run_test.go covering the dry-run recording seam for Cmd/Shell/Root(Sudo on+off)/Try. Issue #6: add Runner.RootShell (run a shell pipeline as root: sudo bash -c in Phase B, bash -c in Phase A) and Runner.TryRoot (best-effort Root). Convert the snapper, grub-theme, and kernel-cmdline shell snippets that hardcoded inner sudo to RootShell with the inner sudo removed, and snapper set-config to TryRoot. The vinceliuice install.sh keeps inner sudo (cloneBuild runs as the user). Equivalent root behaviour in Phase B; updated the affected plan assertions. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Issue #1: luks_on_lvm was silently rendered as lvm_on_luks (encrypting the PVs, the wrong topology) because buildEncryption collapsed both into one branch. The luks-on-lvm topology is unimplemented, so remove it from the encryption oneof set and its renderer branch — Validate() now surfaces a clear "must be one of" error instead of doing the wrong thing silently. Issue #4: configsrc merge-by-name downgrades to wholesale-replace when any slice element lacks a name, so a single nameless hook in an overriding layer silently dropped the base layer's hooks. Make Hook.Name required so a nameless hook fails validation, and document the sharp edge in merge.go. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… dry-run geometry warning Phase-A postInstall fixes: - Issue #2: encrypted layouts no longer remount the still-encrypted plaintext device (rootDevice returns the pre-LUKS path). Until the LUKS remount is implemented + VM-validated, encrypted installs skip the remount, all chroot post-install steps, and Phase B staging, emitting loud warnings (incl. that staging must be done manually). Unencrypted post-install remounts are now CHECKED (Root, aborts on failure) instead of best-effort Try; umounts stay best-effort. - Issue #3: installKernels now runs when kernel.default is set even with no extra packages, so a default pointing at a base kernel is actually pinned in the bootloader. installKernels skips the empty pacman -S. - Issue #11: probeGeometry warns when it falls back to the 512 GiB placeholder for an absent device under dry-run, instead of silently masking a typo'd device path. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The identical partDev was defined in both internal/archinstall and internal/stages. Export it once from archinstall (stages already imports that package) and drop the stages copy. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses eight findings from a full-codebase review (correctness, maintainability, code smells). Built across four parallel worktree agents, then integrated centrally (zero merge conflicts).
gofmtclean;go build/vet/test ./...all pass; archinstall golden renders are byte-identical (no-update).Fixes
luks_on_lvmwas silently rendered aslvm_on_luks(opposite topology, untested). Removed from the validator enum + bothbuildEncryption/encryptionErrorsswitches → now rejected with a clearmust be one of: luks lvm_on_luks.rootDevicereturned the pre-LUKS plaintext device, sopostInstallmounted the still-encrypted partition behind silentTrymounts. Encrypted installs now short-circuit with explicit warnings (LUKS remount is unimplemented / VM-validation-pending); the unencrypted path uses checked mounts so a failure aborts instead of chrooting into nothing.kernel.defaultpointing at abasekernel (with emptypackages) was silently ignored. Gate widened tolen(Packages) > 0 || Default != "", with an empty-pacmanguard.name.Hook.Nameis nowrequired; the sharp edge is documented inmerge.go.sudoin shell strings vs theRunner.Root/Sudoabstraction). AddedRunner.RootShell/TryRoot; converted snapper/grubtheme/helpers; updated affected plan-assertion tests.Capture,Out,outWriter). Addedinternal/run/run_test.go(package previously had no tests).plainBuilder/btrfsBuilder(newsingleDiskRoot+rootSpec) and the duplicatedpartDev(now exported once asarchinstall.PartDev).probeGeometrysilently faked geometry for absent devices under dry-run; now warns, naming the device.Notes
cryptsetup openremount is a possible follow-up.New tests:
encryption_luksonlvm_test.go,hook_name_required_test.go,postinstall_gating_test.go,run_test.go.🤖 Generated with Claude Code