upgrade: decide package-manager ownership before showing the plan

CHANGELOG.md

@@ -22,6 +22,12 @@ useful than saying where the real one is.
   moved by the next `apply` — the diff shows the move before anything is written. `skrog
   wsl-config` and `skrog top` no longer report the ignored line as in effect.
   `wsl-config --json` pending changes gain `section`, and `moved_from` for a move.
+- **`skrog upgrade --apply` under winget or scoop does what its plan says** ([#545](https://github.com/wslkit/skrog/issues/545)).
+  It listed the app step, upgraded the engine and CLI, and only then refused the app
+  because a package manager owns it, exiting 1 after doing everything else. Ownership is
+  now decided before the plan is shown: the app step appears under "will not run" with the
+  manager's command, the rest is applied, and the exit is 0. With only the app behind it
+  prints the command without asking. `--force` still replaces the binary.
 
 ### Added
 

cmd/skrog/upgrade.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"flag"
 	"fmt"
+	"io"
 	"os"
 	"os/exec"
 	"path/filepath"
@@ -128,7 +129,7 @@ flags:
 	if reportOnly {
 		if *dryRun && len(plan) > 0 {
 			fmt.Println("\nwould run:")
-			printUpgradePlan(plan)
+			printUpgradePlan(os.Stdout, plan)
 			fmt.Println("\nnothing was changed (--dry-run)")
 		}
 		// Exit 3 for "there is something to install", matching
@@ -139,50 +140,126 @@ flags:
 		return exitOK
 	}
 
+	return upgradeSteps{
+		apply:     *apply,
+		yes:       *yes,
+		force:     *force,
+		out:       os.Stdout,
+		ownerOf:   runningBinaryOwner,
+		confirm:   confirm,
+		applyPlan: func(p []upgrade.Action) int { return applyUpgradePlan(p, *stateDir) },
+		applyApp:  func(v string) int { return applyApp(v, opts.StateDir) },
+	}.run(rep)
+}
+
+// upgradeSteps is what `skrog upgrade` does once the report is printed. The
+// parts that touch the machine are fields, so a test can drive the real
+// order of decisions without upgrading anything.
+type upgradeSteps struct {
+	apply, yes, force bool
+	out               io.Writer
+
+	ownerOf   func() (upgrade.Owner, bool)
+	confirm   func(prompt string) bool
+	applyPlan func([]upgrade.Action) int
+	applyApp  func(version string) int
+}
+
+func (u upgradeSteps) run(rep upgrade.Report) int {
+	plan := rep.Plan()
 	appBehind := rep.AppStream().Status == upgrade.StatusAvailable
+	latest := rep.AppStream().Latest
+	wantApp := appBehind && u.apply
+
+	// Who owns the install is decided BEFORE the plan is shown (#545). Asked
+	// only after the engine and CLI had been upgraded, the refusal (#370)
+	// turned a run that did everything it could into a non-zero exit, after
+	// the user had confirmed an app step that was never going to happen.
+	var manager *upgrade.Owner
+	if wantApp && !u.force {
+		if owner, owned := u.ownerOf(); owned {
+			manager = &owner
+			wantApp = false
+		}
+	}
+	managedLine := func() {
+		fmt.Fprintf(u.out, "  %-22s %s -> %s  managed by %s — run: %s\n",
+			"app", buildVersion, latest, manager.Name, manager.Command)
+	}
 
-	if len(plan) == 0 && !(appBehind && *apply) {
+	if len(plan) == 0 && !wantApp {
+		if manager != nil {
+			fmt.Fprintf(u.out, "\nthe app is the only thing behind, and %s manages it:\n", manager.Name)
+			managedLine()
+			return exitOK
+		}
 		// The app being behind is not something this command can act on
 		// without --apply, and saying "nothing to do" without that
 		// qualification would read as "you are current".
 		if rep.Available() {
-			fmt.Println("\nnothing here is Skrog's to apply — see above")
+			fmt.Fprintln(u.out, "\nnothing here is Skrog's to apply — see above")
 			if appBehind {
-				fmt.Println("`skrog upgrade --apply` will replace skrog.exe itself")
+				fmt.Fprintln(u.out, "`skrog upgrade --apply` will replace skrog.exe itself")
 			}
 		}
 		return exitOK
 	}
 
-	if !*yes {
-		fmt.Println("\nwill run:")
-		printUpgradePlan(plan)
-		if appBehind && *apply {
-			fmt.Printf("  %-16s %s -> %s  (replaces skrog.exe; the supervisor restarts)\n",
-				"app", buildVersion, rep.AppStream().Latest)
+	if !u.yes {
+		fmt.Fprintln(u.out, "\nwill run:")
+		printUpgradePlan(u.out, plan)
+		if wantApp {
+			fmt.Fprintf(u.out, "  %-22s %s -> %s  (replaces skrog.exe; the supervisor restarts)\n",
+				"app", buildVersion, latest)
+		}
+		if manager != nil {
+			fmt.Fprintln(u.out, "\nwill not run:")
+			managedLine()
 		}
-		if !confirm("\nproceed?") {
-			fmt.Println("nothing was changed")
+		if !u.confirm("\nproceed?") {
+			fmt.Fprintln(u.out, "nothing was changed")
 			return exitOK
 		}
 	}
-	if code := applyUpgradePlan(plan, *stateDir); code != exitOK {
+	if code := u.applyPlan(plan); code != exitOK {
 		return code
 	}
 	// The app last: it is the only step that restarts the supervisor, so a
 	// failure in the engine or CLI half leaves the running binary the one that
 	// produced the log the user is about to read.
-	if appBehind && *apply {
-		return applyApp(rep.AppStream().Latest, opts.StateDir, *force)
+	if wantApp {
+		return u.applyApp(latest)
+	}
+	// Exit 0: everything this command said it would do is done. The app step
+	// was shown as the manager's to run, not as something that failed.
+	if manager != nil {
+		fmt.Fprintf(u.out, "\nthe app is managed by %s; upgrade it with:  %s\n"+
+			"(`skrog upgrade --apply --force` replaces the binary anyway)\n",
+			manager.Name, manager.Command)
 	}
 	return exitOK
 }
 
+// runningBinaryOwner reports the package manager that owns the running
+// skrog.exe, if any (#370).
+//
+// The check needs the RESOLVED path, because a winget install is reached
+// through an alias symlink and only the target is inside the package
+// directory (#360). applyApp's swap deliberately uses the UNRESOLVED one:
+// they are different questions, and conflating them is what #360 was.
+func runningBinaryOwner() (upgrade.Owner, bool) {
+	resolved, err := selfexe.Path()
+	if err != nil {
+		return upgrade.Owner{}, false
+	}
+	return upgrade.OwnedBy(resolved)
+}
+
 // printUpgradePlan shows the commands, not a summary of them: the reader can
 // then run any of them by hand, and can see that nothing else is happening.
-func printUpgradePlan(plan []upgrade.Action) {
+func printUpgradePlan(w io.Writer, plan []upgrade.Action) {
 	for _, a := range plan {
-		fmt.Printf("  skrog %-16s %s -> %s\n", joinArgs(a.Args), orUnset(a.From), a.To)
+		fmt.Fprintf(w, "  skrog %-16s %s -> %s\n", joinArgs(a.Args), orUnset(a.From), a.To)
 	}
 }
 
@@ -359,29 +436,17 @@ func installedCLIVersion(opts provision.Options) string {
 // touch the install directory. Nothing here can leave a half-upgraded install
 // -- SwapBinaries rolls back if any file cannot be replaced, and a download
 // that fails verification never reaches the swap at all.
-func applyApp(version, stateDir string, force bool) int {
+//
+// Whether a package manager owns the install is decided before the plan is
+// shown (upgradeSteps.run), so by the time this runs the answer is "nobody"
+// or --force.
+func applyApp(version, stateDir string) int {
 	exe, err := os.Executable()
 	if err != nil {
 		fmt.Fprintf(os.Stderr, "skrog: locating the running binary: %v\n", err)
 		return exitError
 	}
 
-	// Refuse to fight a package manager (#370).
-	//
-	// The check needs the RESOLVED path, because a winget install is reached
-	// through an alias symlink and only the target is inside the package
-	// directory (#360). The swap below deliberately uses the UNRESOLVED one:
-	// they are different questions, and conflating them is what #360 was.
-	if resolved, rerr := selfexe.Path(); rerr == nil {
-		if owner, owned := upgrade.OwnedBy(resolved); owned && !force {
-			fmt.Fprintf(os.Stderr, "skrog: this install is managed by %s, so `skrog upgrade --apply` "+
-				"would overwrite files it owns and leave it reporting a version you no longer have.\n\n"+
-				"  upgrade with:  %s\n\n"+
-				"Pass --force to replace the binary anyway.\n", owner.Name, owner.Command)
-			return exitError
-		}
-	}
-
 	// Deliberately NOT resolved through symlinks (#360). Everywhere else that
 	// derives a sibling uses internal/selfexe, but replacing a binary is the
 	// one case where following a link would be wrong.

cmd/skrog/upgrade_test.go

@@ -0,0 +1,154 @@
+package main
+
+import (
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+
+	"github.com/wslkit/skrog/internal/upgrade"
+)
+
+// upgradeRun is one drive of upgradeSteps.run with the machine faked out: what
+// was printed, what was asked, and what was applied, in order.
+type upgradeRun struct {
+	code   int
+	out    string
+	events []string
+}
+
+func driveUpgrade(t *testing.T, rep upgrade.Report, exe string, apply, yes, force bool) upgradeRun {
+	t.Helper()
+	var out strings.Builder
+	var r upgradeRun
+	r.code = upgradeSteps{
+		apply: apply, yes: yes, force: force, out: &out,
+		// The real ownership check, against a path the test controls.
+		ownerOf: func() (upgrade.Owner, bool) { return upgrade.OwnedBy(exe) },
+		confirm: func(string) bool {
+			r.events = append(r.events, "confirm")
+			return true
+		},
+		applyPlan: func(p []upgrade.Action) int {
+			for _, a := range p {
+				r.events = append(r.events, "apply "+a.Stream)
+			}
+			return exitOK
+		},
+		applyApp: func(v string) int {
+			r.events = append(r.events, "apply app "+v)
+			return exitOK
+		},
+	}.run(rep)
+	r.out = out.String()
+	return r
+}
+
+// wingetExe is a skrog.exe inside a winget portable package directory, with
+// LOCALAPPDATA pointed at a temp dir so OwnedBy finds that root.
+func wingetExe(t *testing.T) string {
+	t.Helper()
+	local := t.TempDir()
+	t.Setenv("LOCALAPPDATA", local)
+	dir := filepath.Join(local, "Microsoft", "WinGet", "Packages", "wslkit.skrog_Microsoft.Winget.Source_8wekyb3d8bbwe")
+	if err := os.MkdirAll(dir, 0o755); err != nil {
+		t.Fatal(err)
+	}
+	return filepath.Join(dir, "skrog.exe")
+}
+
+// plainExe is a zip unpacked by hand: owned by nobody.
+func plainExe(t *testing.T) string {
+	t.Helper()
+	t.Setenv("LOCALAPPDATA", t.TempDir())
+	return filepath.Join(t.TempDir(), "Programs", "skrog", "skrog.exe")
+}
+
+func behindReport(engineBehind bool) upgrade.Report {
+	engine := upgrade.Stream{Name: "engine", Current: "29.9.1", Latest: "29.9.1", Status: upgrade.StatusCurrent}
+	if engineBehind {
+		engine = upgrade.Stream{Name: "engine", Current: "29.8.0", Latest: "29.9.1", Status: upgrade.StatusAvailable}
+	}
+	return upgrade.Report{Streams: []upgrade.Stream{
+		{Name: "app", Current: "0.8.0", Latest: "0.10.0", Status: upgrade.StatusAvailable},
+		engine,
+		{Name: "cli", Current: "29.9.1", Latest: "29.9.1", Status: upgrade.StatusCurrent},
+	}}
+}
+
+// #545: under winget, --apply used to list the app step, upgrade the engine,
+// and only then refuse the app and exit non-zero. The plan the user confirms
+// must leave the app out, name the manager's command, and the run must exit 0
+// having done exactly what it showed.
+func TestUpgradeApplyUnderWingetDecidesOwnershipBeforeThePlan(t *testing.T) {
+	r := driveUpgrade(t, behindReport(true), wingetExe(t), true, false, false)
+
+	if r.code != exitOK {
+		t.Errorf("exit %d, want %d\n%s", r.code, exitOK, r.out)
+	}
+	if got, want := strings.Join(r.events, ", "), "confirm, apply engine"; got != want {
+		t.Errorf("events = %q, want %q (the app step must never run)", got, want)
+	}
+	shown, _, _ := strings.Cut(r.out, "will not run:")
+	if strings.Contains(shown, "app") {
+		t.Errorf("the plan to confirm lists the app step:\n%s", shown)
+	}
+	if !strings.Contains(r.out, "managed by winget — run: winget upgrade wslkit.skrog") {
+		t.Errorf("the plan does not name winget's command:\n%s", r.out)
+	}
+}
+
+// Nothing to apply except the app, and the app is the manager's: say so and
+// exit 0 without asking a question whose only answer changes nothing.
+func TestUpgradeApplyUnderWingetWithOnlyTheAppBehind(t *testing.T) {
+	r := driveUpgrade(t, behindReport(false), wingetExe(t), true, false, false)
+
+	if r.code != exitOK {
+		t.Errorf("exit %d, want %d\n%s", r.code, exitOK, r.out)
+	}
+	if len(r.events) != 0 {
+		t.Errorf("events = %v, want none: no prompt, nothing applied", r.events)
+	}
+	if !strings.Contains(r.out, "winget upgrade wslkit.skrog") {
+		t.Errorf("output does not name winget's command:\n%s", r.out)
+	}
+}
+
+// --yes shows no plan, so the manager's command is printed after the run.
+func TestUpgradeApplyYesUnderWingetSaysWhatToRunAfter(t *testing.T) {
+	r := driveUpgrade(t, behindReport(true), wingetExe(t), true, true, false)
+
+	if r.code != exitOK {
+		t.Errorf("exit %d, want %d\n%s", r.code, exitOK, r.out)
+	}
+	if got, want := strings.Join(r.events, ", "), "apply engine"; got != want {
+		t.Errorf("events = %q, want %q", got, want)
+	}
+	if !strings.Contains(r.out, "upgrade it with:  winget upgrade wslkit.skrog") {
+		t.Errorf("output does not name winget's command:\n%s", r.out)
+	}
+}
+
+// --force keeps the old behaviour: the app is replaced anyway.
+func TestUpgradeApplyForceUnderWingetReplacesTheApp(t *testing.T) {
+	r := driveUpgrade(t, behindReport(true), wingetExe(t), true, false, true)
+
+	if got, want := strings.Join(r.events, ", "), "confirm, apply engine, apply app 0.10.0"; got != want {
+		t.Errorf("events = %q, want %q", got, want)
+	}
+	if !strings.Contains(r.out, "replaces skrog.exe") {
+		t.Errorf("the plan does not show the app step:\n%s", r.out)
+	}
+}
+
+// An unmanaged install is unchanged by #545: app last, after the plan.
+func TestUpgradeApplyUnmanagedReplacesTheAppLast(t *testing.T) {
+	r := driveUpgrade(t, behindReport(true), plainExe(t), true, false, false)
+
+	if got, want := strings.Join(r.events, ", "), "confirm, apply engine, apply app 0.10.0"; got != want {
+		t.Errorf("events = %q, want %q", got, want)
+	}
+	if strings.Contains(r.out, "managed by") {
+		t.Errorf("an unmanaged install is reported as managed:\n%s", r.out)
+	}
+}

docs/install.md

@@ -109,8 +109,8 @@ scoop install skrog
 Then `scoop update skrog` keeps it current.
 
 Skrog notices when winget or scoop manages it — `skrog upgrade --apply`
-refuses inside a package-manager directory and names the manager's own upgrade
-command, rather than fighting it for the same files. Neither package is
+still upgrades the engine and CLI, but leaves the app to the manager and names
+its upgrade command, rather than fighting it for the same files. Neither package is
 blocked on code signing, which this project believed for longer than it should
 have; only the MSI is.