fix(34): CR-02 install plugins of an installed module at its version

plugin:install pinned only modules chosen in the same plan, so a second plugin package in a module the application already required was installed at the newest release, moving the plugin already there, and with --source the repository was cloned a second time. PlanInstall now seeds every module of summer.yaml: a plugin in one installs at the installed version with no go get or clone (only its summer.yaml entry), and is refused with a plugin:update hint when that version has no release of it or is not a release. A git URL whose repository is already a submodule, or whose module the application has, is refused.
2026-10-29 22:01:30 +01:00
parent 60551485ed
commit 9b72439533
5 changed files with 264 additions and 3 deletions
--- a/internal/build/install.go
+++ b/internal/build/install.go
@@ -83,6 +83,11 @@ type InstallStep struct {

 	// clone is the checkout a git URL step was planned from.
 	clone string
+	// existing marks a plugin in a module the application already has (a
+	// go.mod require or a submodule checkout): it is installed at that
+	// version and ApplyInstall fetches nothing for it, only adds its
+	// summer.yaml entry (CR-02).
+	existing bool
 }

 // installer carries the state of one PlanInstall run.
@@ -102,6 +107,10 @@ type installer struct {
 	planned       map[string]InstallStep
 	visiting      []string
 	moduleVersion map[string]string
+	// installed holds the modules the application already has, by module
+	// path: a plugin in one of them installs at the version the
+	// application has, never moving it.
+	installed map[string]installedModule
 	// source installs registry plugins as submodules: --source, or the
 	// requirements of a git URL install.
 	source bool
@@ -141,6 +150,9 @@ func PlanInstall(ctx context.Context, opts InstallOptions) (*InstallPlan, error)

 	in.planned = map[string]InstallStep{}
 	in.moduleVersion = map[string]string{}
+	if in.installed, err = installedModules(ctx, appDir, m); err != nil {
+		return nil, err
+	}
 	in.plan.interactive = opts.Interactive
 	in.source = opts.Source
 	ok := false
@@ -184,6 +196,45 @@ func PlanInstall(ctx context.Context, opts InstallOptions) (*InstallPlan, error)
 	return in.plan, nil
 }

+// installedModule is a module the application already has, with the
+// summer.yaml plugin that brought it.
+type installedModule struct {
+	src pluginSource
+	by  string
+}
+
+// release is the version a plugin of the module installs at: the go.mod
+// release of a module install, the base tag of a submodule checkout; ""
+// when there is none to pin (a pseudo-version, an untagged checkout, a
+// local directory).
+func (im installedModule) release() string {
+	switch im.src.Kind {
+	case ModeModule, ModeSubmodule:
+		return releaseOrEmpty(im.src.Version)
+	}
+	return ""
+}
+
+// installedModules maps every module that provides a summer.yaml plugin
+// to how it is installed (see installedSource). Packages of the
+// application module itself are left out.
+func installedModules(ctx context.Context, appDir string, m Manifest) (map[string]installedModule, error) {
+	out := map[string]installedModule{}
+	for _, p := range m.Plugins {
+		src, err := installedSource(ctx, appDir, p.Module)
+		if err != nil {
+			return nil, err
+		}
+		if src.Module == "" {
+			continue
+		}
+		if _, ok := out[src.Module]; !ok {
+			out[src.Module] = installedModule{src: src, by: p.ID}
+		}
+	}
+	return out, nil
+}
+
 // removeTemps deletes the leftover temporary checkouts of the plan.
 func (p *InstallPlan) removeTemps() {
 	for _, dir := range p.temps {
@@ -392,12 +443,23 @@ func (in *installer) choose(id, pin, min string) (InstallStep, kiosk.Requires, e
 		if !kiosk.Satisfies(pin, min) {
 			return InstallStep{}, nil, fmt.Errorf("build: %s %s is older than the required minimum %s", id, pin, min)
 		}
+		if im, ok := in.installed[chosen.Module]; ok && im.release() != pin {
+			return InstallStep{}, nil, in.installedModuleError(id, im)
+		}
 	} else {
 		var eligible []kiosk.RegistryVersion
+		var held *installedModule
 		for _, v := range versions {
 			if !kiosk.ValidVersion(v.Version) || !kiosk.Satisfies(v.Version, min) {
 				continue
 			}
+			// A plugin in a module the application already has installs
+			// at the version it has: installing never moves the plugins
+			// already there.
+			if im, ok := in.installed[v.Module]; ok && v.Version != im.release() {
+				held = &im
+				continue
+			}
 			// A plugin in a module another chosen plugin lives in must
 			// come from the same release of that module.
 			if want, ok := in.moduleVersion[v.Module]; ok && v.Version != want {
@@ -406,6 +468,9 @@ func (in *installer) choose(id, pin, min string) (InstallStep, kiosk.Requires, e
 			eligible = append(eligible, v)
 		}
 		best, ok := kiosk.NewestCompatible(eligible, in.fw)
+		if !ok && held != nil {
+			return InstallStep{}, nil, in.installedModuleError(id, *held)
+		}
 		if !ok {
 			if len(eligible) == 0 {
 				if min != "" {
@@ -424,6 +489,23 @@ func (in *installer) choose(id, pin, min string) (InstallStep, kiosk.Requires, e
 		Package: chosen.Package,
 		Mode:    ModeModule,
 	}
+	if im, ok := in.installed[chosen.Module]; ok {
+		// The module is there already, as a go.mod require or a
+		// checkout: nothing is fetched or cloned, whatever --source says.
+		step.existing, step.Mode = true, im.src.Kind
+		if im.src.Kind == ModeSubmodule {
+			rel := strings.TrimPrefix(strings.TrimPrefix(chosen.Package, chosen.Module), "/")
+			if !dirExists(filepath.Join(im.src.Dir, filepath.FromSlash(rel))) {
+				return InstallStep{}, nil, bonfire.WithHint(fmt.Errorf("build: %s lives in %s, which the application has checked out at %s (for %s), and that checkout has no package %s", id, chosen.Module, displayRef(im.src.Version), im.by, chosen.Package),
+					"plugin:install does not move plugins the application already has. Update the checkout with plugin:update first.",
+					"{bin} plugin:update "+im.by)
+			}
+		}
+		if err := validateStep(step); err != nil {
+			return InstallStep{}, nil, err
+		}
+		return step, chosen.Requires, nil
+	}
 	if in.source {
 		detail, err := in.opts.Registry.Plugin(in.ctx, id)
 		if err != nil {
@@ -443,6 +525,24 @@ func (in *installer) choose(id, pin, min string) (InstallStep, kiosk.Requires, e
 	return step, chosen.Requires, nil
 }

+// installedModuleError is the error for a plugin whose module the
+// application already has, when no release of the plugin matches the
+// version the application has.
+func (in *installer) installedModuleError(id string, im installedModule) error {
+	have := im.release()
+	why := "the registry has no release of " + id + " at " + have
+	if have == "" {
+		why = "that version is not a release plugin:install can pin"
+		have = displayVersion(im.src.Version)
+		if im.src.Kind == sourceLocal {
+			have = "a local directory"
+		}
+	}
+	return bonfire.WithHint(fmt.Errorf("build: %s lives in %s, which the application already has at %s (for %s), and %s", id, im.src.Module, have, im.by, why),
+		"plugin:install does not move plugins the application already has. Move "+im.by+" to a release that ships "+id+" with plugin:update, then install it.",
+		"{bin} plugin:update "+im.by)
+}
+
 // frameworkError names the framework release v needs.
 func (in *installer) frameworkError(id string, v kiosk.RegistryVersion) error {
 	have := in.fw
@@ -490,6 +590,9 @@ func validateStep(s InstallStep) error {
 	switch s.Mode {
 	case ModeModule:
 	case ModeSubmodule:
+		if s.existing {
+			break
+		}
 		if err := validateGitURL(s.Repository); err != nil {
 			return fmt.Errorf("build: %s: %w; refused", s.ID, err)
 		}
@@ -503,7 +606,11 @@ func validateStep(s InstallStep) error {
 func (p *InstallPlan) summary() string {
 	var b strings.Builder
 	for _, s := range p.Steps {
-		fmt.Fprintf(&b, "  %s@%s (%s)\n", s.ID, displayRef(s.Version), s.Mode)
+		mode := s.Mode
+		if s.existing {
+			mode += ", module already in the application"
+		}
+		fmt.Fprintf(&b, "  %s@%s (%s)\n", s.ID, displayRef(s.Version), mode)
 	}
 	return b.String()
 }
@@ -576,6 +683,10 @@ func ApplyInstall(ctx context.Context, plan *InstallPlan, out io.Writer) error {
 		if err := validateStep(s); err != nil {
 			return err
 		}
+		entries = append(entries, Plugin{ID: s.ID, Module: s.Package})
+		if s.existing {
+			continue
+		}
 		switch s.Mode {
 		case ModeModule:
 			if !slices.Contains(getArgs, s.Module+"@"+s.Version) {
@@ -584,7 +695,6 @@ func ApplyInstall(ctx context.Context, plan *InstallPlan, out io.Writer) error {
 		case ModeSubmodule:
 			submodules = true
 		}
-		entries = append(entries, Plugin{ID: s.ID, Module: s.Package})
 	}
 	if submodules {
 		if err := requireGitRepo(ctx, plan.AppDir); err != nil {
@@ -593,7 +703,7 @@ func ApplyInstall(ctx context.Context, plan *InstallPlan, out io.Writer) error {
 		done := map[string]bool{}
 		for i := range plan.Steps {
 			s := &plan.Steps[i]
-			if s.Mode != ModeSubmodule || done[s.Module] {
+			if s.Mode != ModeSubmodule || s.existing || done[s.Module] {
 				continue
 			}
 			if err := applySubmodule(ctx, plan, s, plan.interactive, out); err != nil {
--- a/internal/build/install_smoke_test.go
+++ b/internal/build/install_smoke_test.go
@@ -658,6 +658,49 @@ func TestInstallSmoke(t *testing.T) {
 		return app, blogRepo, reg
 	}

+	t.Run("source_shared_module", func(t *testing.T) {
+		if testing.Short() {
+			t.Skip("clones fixture repositories")
+		}
+		gitTestEnv(t)
+		app, blogRepo, reg := sourceApp(t)
+		blogDir := filepath.Join(app, "plugins", "acme", "blog")
+		reg.set("acme.blogextra", regPlugin("acme.blogextra", blogRepo,
+			regVersion("v0.1.0", "v0.5.0", blogModule, blogModule+"/extra", nil)))
+		// The checkout has no extra package: refused, nothing cloned.
+		_, err := PlanInstall(t.Context(), InstallOptions{StartDir: app, Arg: "acme.blogextra", Source: true, Yes: true, Registry: reg.Client})
+		if err == nil || !strings.Contains(err.Error(), "that checkout has no package "+blogModule+"/extra") {
+			t.Fatalf("err = %v", err)
+		}
+		if err := os.MkdirAll(filepath.Join(blogDir, "extra"), 0o755); err != nil {
+			t.Fatal(err)
+		}
+		var out bytes.Buffer
+		plan, err := PlanInstall(t.Context(), InstallOptions{StartDir: app, Arg: "acme.blogextra", Source: true, Yes: true, Registry: reg.Client, Out: &out})
+		if err != nil {
+			t.Fatalf("plan: %v\n%s", err, out.String())
+		}
+		if got := stepList(plan); got != "acme.blogextra@v0.1.0/submodule" || !plan.Steps[0].existing {
+			t.Fatalf("steps = %s", got)
+		}
+		gitmodules, _ := os.ReadFile(filepath.Join(app, ".gitmodules"))
+		if err := ApplyInstall(t.Context(), plan, &out); err != nil {
+			t.Fatalf("apply: %v\n%s", err, out.String())
+		}
+		if after, _ := os.ReadFile(filepath.Join(app, ".gitmodules")); !bytes.Equal(gitmodules, after) || len(installTemps(t, app)) != 0 {
+			t.Fatalf("a second submodule or checkout was added:\n%s", after)
+		}
+		if strings.Contains(out.String(), "Cloning") {
+			t.Fatalf("cloned again:\n%s", out.String())
+		}
+		// A git URL install of the same repository is refused before cloning.
+		out.Reset()
+		_, err = PlanInstall(t.Context(), InstallOptions{StartDir: app, Arg: blogRepo + ".git", Yes: true, Registry: reg.Client, Out: &out})
+		if err == nil || !strings.Contains(err.Error(), "already checked out at plugins/acme/blog") || strings.Contains(out.String(), "Cloning") {
+			t.Fatalf("git URL of an installed submodule: %v\n%s", err, out.String())
+		}
+	})
+
 	t.Run("update", func(t *testing.T) {
 		if testing.Short() {
 			t.Skip("installs from a file:// module proxy")
--- a/internal/build/install_test.go
+++ b/internal/build/install_test.go
@@ -438,3 +438,74 @@ func TestUpdatePseudoVersion(t *testing.T) {
 		}
 	})
 }
+
+// TestInstallSharedModule pins CR-02: a plugin in a module the
+// application already has installs at the version the application has,
+// fetches nothing, and is refused when that version has no release of it.
+func TestInstallSharedModule(t *testing.T) {
+	reg := &memRegistry{}
+	reg.addIn("acme.pg", "example.test/pg", "example.test/pg", nil, "v0.3.0:v0.6.0", "v0.2.0:v0.6.0")
+	reg.addIn("acme.pgsubs", "example.test/pg", "example.test/pg/subs", nil, "v0.3.0:v0.6.0", "v0.2.0:v0.6.0")
+	reg.addIn("acme.pgnew", "example.test/pg", "example.test/pg/fresh", nil, "v0.3.0:v0.6.0")
+	reg.addIn("acme.shop", "example.test/shop", "example.test/shop", kiosk.Requires{"acme.pgsubs": ""}, "v0.1.0:v0.6.0")
+	plan := func(t *testing.T, app, arg string) (*InstallPlan, string, error) {
+		t.Helper()
+		var out bytes.Buffer
+		p, err := PlanInstall(t.Context(), InstallOptions{StartDir: app, Arg: arg, Yes: true, Registry: reg, Out: &out})
+		return p, out.String(), err
+	}
+
+	t.Run("pinned to the installed module version", func(t *testing.T) {
+		app := installApp(t, "v0.6.0", map[string]string{"acme.pg": "v0.2.0"})
+		p, out, err := plan(t, app, "acme.pgsubs")
+		if err != nil {
+			t.Fatalf("plan: %v\n%s", err, out)
+		}
+		if got := planSteps(p); got != "acme.pgsubs@v0.2.0" || !p.Steps[0].existing {
+			t.Fatalf("steps = %s %+v, want acme.pgsubs at the installed v0.2.0", got, p.Steps)
+		}
+		if !strings.Contains(out, "acme.pgsubs@v0.2.0 (module, module already in the application)") {
+			t.Errorf("plan output:\n%s", out)
+		}
+		gomod, _ := os.ReadFile(filepath.Join(app, "go.mod"))
+		// ApplyInstall runs no go get for it: only summer.yaml changes.
+		if err := ApplyInstall(t.Context(), p, nil); err != nil {
+			t.Fatalf("apply: %v", err)
+		}
+		after, _ := os.ReadFile(filepath.Join(app, "go.mod"))
+		if !bytes.Equal(gomod, after) {
+			t.Errorf("go.mod changed:\n%s", after)
+		}
+		yaml, _ := os.ReadFile(filepath.Join(app, manifestName))
+		if !strings.Contains(string(yaml), "id: acme.pgsubs\n    module: example.test/pg/subs") {
+			t.Errorf("summer.yaml:\n%s", yaml)
+		}
+	})
+
+	t.Run("a requirement in the installed module", func(t *testing.T) {
+		app := installApp(t, "v0.6.0", map[string]string{"acme.pg": "v0.2.0"})
+		p, out, err := plan(t, app, "acme.shop")
+		if err != nil || planSteps(p) != "acme.pgsubs@v0.2.0,acme.shop@v0.1.0" {
+			t.Fatalf("plan: %v %v\n%s", p, err, out)
+		}
+	})
+
+	t.Run("refused when the installed version has no release of it", func(t *testing.T) {
+		app := installApp(t, "v0.6.0", map[string]string{"acme.pg": "v0.2.0"})
+		for _, arg := range []string{"acme.pgnew", "acme.pgsubs@v0.3.0"} {
+			_, out, err := plan(t, app, arg)
+			if err == nil || !strings.Contains(err.Error(), "lives in example.test/pg, which the application already has at v0.2.0 (for acme.pg)") ||
+				!strings.Contains(fullError(err), "plugin:update acme.pg") {
+				t.Errorf("%s: %v\n%s", arg, fullError(err), out)
+			}
+		}
+	})
+
+	t.Run("refused at a pseudo-version", func(t *testing.T) {
+		app := installApp(t, "v0.6.0", map[string]string{"acme.pg": "v0.2.1-0.20261001120000-abcdef123456"})
+		_, out, err := plan(t, app, "acme.pgsubs")
+		if err == nil || !strings.Contains(err.Error(), "not a release plugin:install can pin") {
+			t.Errorf("err = %v\n%s", fullError(err), out)
+		}
+	})
+}
--- a/internal/build/source.go
+++ b/internal/build/source.go
@@ -230,6 +230,11 @@ func (in *installer) planGitURL(rawURL string) error {
 	if err := requireGitRepo(in.ctx, in.appDir); err != nil {
 		return err
 	}
+	if dir := submoduleWithURL(in.ctx, in.appDir, rawURL); dir != "" {
+		return bonfire.WithHint(fmt.Errorf("build: %s is already checked out at %s", rawURL, dir),
+			"plugin:install does not clone a repository the application already has as a submodule. Update the plugins in it with plugin:update.",
+			"{bin} plugin:update")
+	}
 	fmt.Fprintf(in.out, "Cloning %s\n", rawURL)
 	tmp, err := cloneInto(in.ctx, in.appDir, rawURL, in.opts.Interactive)
 	if err != nil {
@@ -306,6 +311,11 @@ func (in *installer) planGitURL(rawURL string) error {
 	if err := validateStep(step); err != nil {
 		return err
 	}
+	if im, ok := in.installed[step.Module]; ok {
+		return bonfire.WithHint(fmt.Errorf("build: %s is module %s, which the application already has (for %s)", rawURL, step.Module, im.by),
+			"plugin:install does not move plugins the application already has, and a second checkout of one module breaks the workspace. Update the module with plugin:update.",
+			"{bin} plugin:update "+im.by)
+	}
 	if _, err := submoduleTarget(in.ctx, in.appDir, tmp, step.ID, rawURL); err != nil {
 		return err
 	}
@@ -320,6 +330,31 @@ func (in *installer) planGitURL(rawURL string) error {
 	return nil
 }

+// submoduleWithURL returns the path of the application's submodule whose
+// .gitmodules URL is rawURL (ignoring a trailing slash or .git), or "".
+func submoduleWithURL(ctx context.Context, appDir, rawURL string) string {
+	out, err := gitOutput(ctx, appDir, "config", "--file", ".gitmodules", "--get-regexp", `^submodule\..*\.url$`)
+	if err != nil {
+		return ""
+	}
+	norm := func(u string) string {
+		return strings.TrimSuffix(strings.TrimRight(u, "/"), ".git")
+	}
+	want := norm(rawURL)
+	for _, line := range strings.Split(strings.TrimSpace(string(out)), "\n") {
+		key, val, ok := strings.Cut(line, " ")
+		if !ok || norm(strings.TrimSpace(val)) != want {
+			continue
+		}
+		name := strings.TrimSuffix(strings.TrimPrefix(key, "submodule."), ".url")
+		if p, err := gitOutput(ctx, appDir, "config", "--file", ".gitmodules", "submodule."+name+".path"); err == nil {
+			return strings.TrimSpace(string(p))
+		}
+		return name
+	}
+	return ""
+}
+
 // readCheckoutManifest reads <dir>/<rel>/plugin.yaml from a work tree; nil
 // when it does not exist.
 func readCheckoutManifest(dir, rel string) (*kiosk.Manifest, error) {