From 8b43a0f273bac4f8a0dd1ad497cdf94e659da814 Mon Sep 17 00:00:00 2001 From: MDietermann Date: Mon, 13 Apr 2026 21:35:14 +0200 Subject: [PATCH] fix: address final-review findings before closing Plan 1 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - privilege: wire sudo -v to real stdin/stdout/stderr so it can prompt - cmd: validate --only/--skip IDs against registry (prevents silent run-all on typo); gate install to Arch-only (spec requirement); mark --only and --skip mutually exclusive - platform: Pacman.Update now runs -Syu --noconfirm, not -Sy (avoids Arch partial-upgrade anti-pattern) - test/integration: trap-clean the named docker volume on any exit - go.mod: tidy — yaml.v3 is a direct dep --- cmd/archduino/main.go | 42 +++++++++++++++++++++++++++++--- go.mod | 6 +++-- go.sum | 1 + internal/platform/pacman.go | 7 ++++-- internal/platform/pacman_test.go | 2 +- internal/privilege/sudo.go | 7 +++++- test/integration/run.sh | 5 ++-- 7 files changed, 59 insertions(+), 11 deletions(-) diff --git a/cmd/archduino/main.go b/cmd/archduino/main.go index 59744b3..bf63438 100644 --- a/cmd/archduino/main.go +++ b/cmd/archduino/main.go @@ -12,6 +12,7 @@ import ( "github.com/MDietermann/ArduinoArchSetup/internal/engine" "github.com/MDietermann/ArduinoArchSetup/internal/module" "github.com/MDietermann/ArduinoArchSetup/internal/modules/hello" + "github.com/MDietermann/ArduinoArchSetup/internal/platform" "github.com/MDietermann/ArduinoArchSetup/internal/privilege" "github.com/MDietermann/ArduinoArchSetup/internal/state" ) @@ -62,12 +63,28 @@ func newInstallCmd() *cobra.Command { ctx = context.Background() } + // Gate: v1 supports Arch only. Non-Arch hosts would silently run + // the hello module today but fail on Plan 3's pacman-dependent + // modules — fail fast with a clear message instead. + if !noSudo { // --no-sudo implies a test/container context; skip the gate + p, derr := platform.Detect() + if derr != nil { + return fmt.Errorf("platform detect: %w", derr) + } + if p.Distro != platform.DistroArch { + return fmt.Errorf("archduino requires Arch Linux (detected distro: %s)", p.Distro) + } + } + cfg, err := config.Load(overlayPath) if err != nil { return err } - selected := resolveSelection(Registry, cfg, only, skip) + selected, err := resolveSelection(Registry, cfg, only, skip) + if err != nil { + return err + } // Acquire sudo unless dry-run or explicitly skipped. if !dryRun && !noSudo { @@ -129,11 +146,30 @@ func newInstallCmd() *cobra.Command { cmd.Flags().StringSliceVar(&only, "only", nil, "comma-separated module IDs to run (exclusive with --skip)") cmd.Flags().StringSliceVar(&skip, "skip", nil, "comma-separated module IDs to skip") cmd.Flags().StringVar(&overlayPath, "config", "", "path to overlay.yaml (default: ~/.config/archduino/overlay.yaml)") + cmd.MarkFlagsMutuallyExclusive("only", "skip") return cmd } -func resolveSelection(reg *module.Registry, cfg config.Config, only, skip []string) []string { +func resolveSelection(reg *module.Registry, cfg config.Config, only, skip []string) ([]string, error) { all := reg.All() + known := map[string]bool{} + for _, m := range all { + known[m.Meta().ID] = true + } + + // Validate every explicit ID the user passed so a typo in --only does not + // silently fall through to "run all modules". + for _, id := range only { + if !known[id] { + return nil, fmt.Errorf("unknown module in --only: %q", id) + } + } + for _, id := range skip { + if !known[id] { + return nil, fmt.Errorf("unknown module in --skip: %q", id) + } + } + disabled := map[string]bool{} for _, d := range cfg.DisabledModules { disabled[d] = true @@ -156,7 +192,7 @@ func resolveSelection(reg *module.Registry, cfg config.Config, only, skip []stri } out = append(out, id) } - return out + return out, nil } func writeState(started, ended time.Time, sum engine.Summary) error { diff --git a/go.mod b/go.mod index c887a0d..bcf06b2 100644 --- a/go.mod +++ b/go.mod @@ -2,10 +2,12 @@ module github.com/MDietermann/ArduinoArchSetup go 1.22 -require github.com/spf13/cobra v1.10.2 +require ( + github.com/spf13/cobra v1.10.2 + gopkg.in/yaml.v3 v3.0.1 +) require ( github.com/inconshreveable/mousetrap v1.1.0 // indirect github.com/spf13/pflag v1.0.9 // indirect - gopkg.in/yaml.v3 v3.0.1 // indirect ) diff --git a/go.sum b/go.sum index ff4d6ec..47edb24 100644 --- a/go.sum +++ b/go.sum @@ -7,6 +7,7 @@ github.com/spf13/cobra v1.10.2/go.mod h1:7C1pvHqHw5A4vrJfjNwvOdzYu0Gml16OCs2GRiT github.com/spf13/pflag v1.0.9 h1:9exaQaMOCwffKiiiYk6/BndUBv+iRViNW+4lEMi0PvY= github.com/spf13/pflag v1.0.9/go.mod h1:McXfInJRrz4CZXVZOBLb0bTZqETkiAhM9Iw0y3An2Bg= go.yaml.in/yaml/v3 v3.0.4/go.mod h1:DhzuOOF2ATzADvBadXxruRBLzYTpT36CKvDb3+aBEFg= +gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= diff --git a/internal/platform/pacman.go b/internal/platform/pacman.go index 2161d1e..027747a 100644 --- a/internal/platform/pacman.go +++ b/internal/platform/pacman.go @@ -68,7 +68,10 @@ func (p Pacman) Install(ctx context.Context, pkgs []string) error { return p.Runner.Run(ctx, "sudo", args...) } -// Update runs `sudo pacman -Sy`. +// Update runs `sudo pacman -Syu --noconfirm` (full sync + system upgrade). +// On Arch, a bare `pacman -Sy` followed by an install is the partial-upgrade +// anti-pattern that can break the system when a dep's ABI changes, so we +// always pair the sync with the upgrade. func (p Pacman) Update(ctx context.Context) error { - return p.Runner.Run(ctx, "sudo", "pacman", "-Sy") + return p.Runner.Run(ctx, "sudo", "pacman", "-Syu", "--noconfirm") } diff --git a/internal/platform/pacman_test.go b/internal/platform/pacman_test.go index a555f51..4d76bc5 100644 --- a/internal/platform/pacman_test.go +++ b/internal/platform/pacman_test.go @@ -80,7 +80,7 @@ func TestPacmanUpdate(t *testing.T) { if err := p.Update(context.Background()); err != nil { t.Fatal(err) } - want := []string{"sudo", "pacman", "-Sy"} + want := []string{"sudo", "pacman", "-Syu", "--noconfirm"} if !reflect.DeepEqual(r.runCalls[0], want) { t.Fatalf("args = %v, want %v", r.runCalls[0], want) } diff --git a/internal/privilege/sudo.go b/internal/privilege/sudo.go index 2d5ec67..cb2fd5c 100644 --- a/internal/privilege/sudo.go +++ b/internal/privilege/sudo.go @@ -5,6 +5,7 @@ package privilege import ( "context" "fmt" + "os" "os/exec" "time" ) @@ -36,7 +37,11 @@ type ExecSudo struct{} func (ExecSudo) Validate(ctx context.Context) error { cmd := exec.CommandContext(ctx, "sudo", "-v") - cmd.Stdin = nil // inherit terminal stdin + // Connect to the real terminal so sudo can prompt for the password. + // exec.Cmd.Stdin defaults to /dev/null, not the parent process's stdin. + cmd.Stdin = os.Stdin + cmd.Stdout = os.Stdout + cmd.Stderr = os.Stderr return cmd.Run() } diff --git a/test/integration/run.sh b/test/integration/run.sh index f6cd17d..17b035a 100755 --- a/test/integration/run.sh +++ b/test/integration/run.sh @@ -14,8 +14,10 @@ echo "--- run 1: fresh install" docker run --rm "$IMAGE" install --no-tui --no-sudo echo "--- run 2: idempotent (marker already present)" -# Persist home across runs via a named volume. +# Persist home across runs via a named volume. Clean up on any exit so a +# failed assertion doesn't leak the volume to the next run. VOL=archduino-integration-home +trap 'docker volume rm "$VOL" >/dev/null 2>&1 || true' EXIT docker volume create "$VOL" >/dev/null docker run --rm -v "$VOL":/home/tester "$IMAGE" install --no-tui --no-sudo docker run --rm -v "$VOL":/home/tester "$IMAGE" install --no-tui --no-sudo @@ -23,5 +25,4 @@ docker run --rm -v "$VOL":/home/tester "$IMAGE" install --no-tui --no-sudo echo "--- assert marker exists in volume" docker run --rm -v "$VOL":/home/tester --entrypoint test "$IMAGE" -f /home/tester/.cache/archduino/hello.ok -docker volume rm "$VOL" >/dev/null echo "--- integration test PASSED"