fix(34): WR-11 accept an inline plugins list in plugin:add again

The comment-keeping summer.yaml edit refused a flow-style plugins list
(plugins: [{id: a, module: b}]) that plugin:add used to handle with a
full rewrite. The edit now replaces an inline list, over one or several
lines, with a block list of its items plus the new ones and keeps every
other line. A list it cannot edit in place (a tagged one, say) falls
back to the full SaveManifest rewrite with a note that the comments were
not kept. plugin:install gets the same behaviour.

internal/build/install.go

@@ -733,9 +733,13 @@ func ApplyInstall(ctx context.Context, plan *InstallPlan, out io.Writer) error {
 			return fail(err)
 		}
 	}
-	if err := appendManifestPlugins(filepath.Join(plan.AppDir, manifestName), entries); err != nil {
+	rewrote, err := appendManifestPlugins(filepath.Join(plan.AppDir, manifestName), entries)
+	if err != nil {
 		return fail(err)
 	}
+	if rewrote {
+		fmt.Fprintf(out, "note: %s had an inline plugins list summer could not edit in place; it was written again and its comments were not kept\n", manifestName)
+	}
 	ids := make([]string, len(plan.Steps))
 	for i, s := range plan.Steps {
 		ids[i] = s.ID + "@" + displayRef(s.Version)

internal/build/install_smoke_test.go

@@ -207,7 +207,7 @@ func TestInstallSmoke(t *testing.T) {
 
 	t.Run("installed_old", func(t *testing.T) {
 		app := helloCopy(t)
-		if err := appendManifestPlugins(filepath.Join(app, manifestName), []Plugin{{ID: "acme.translate", Module: translateModule}}); err != nil {
+		if _, err := appendManifestPlugins(filepath.Join(app, manifestName), []Plugin{{ID: "acme.translate", Module: translateModule}}); err != nil {
 			t.Fatal(err)
 		}
 		if err := runGo(t.Context(), app, "mod", "edit", "-require="+translateModule+"@v0.0.9"); err != nil {
@@ -521,7 +521,7 @@ func TestInstallSmoke(t *testing.T) {
 		if left := installTemps(t, app); len(left) != 0 {
 			t.Fatalf("failed plan left checkouts: %v", left)
 		}
-		if err := appendManifestPlugins(filepath.Join(app, manifestName), []Plugin{{ID: "acme.translate", Module: translateModule}}); err != nil {
+		if _, err := appendManifestPlugins(filepath.Join(app, manifestName), []Plugin{{ID: "acme.translate", Module: translateModule}}); err != nil {
 			t.Fatal(err)
 		}
 		if err := runGo(t.Context(), app, "mod", "edit", "-require="+translateModule+"@v0.1.0"); err != nil {
@@ -807,7 +807,7 @@ func TestInstallSmoke(t *testing.T) {
 			t.Fatal(err)
 		}
 		// acme.ghost: a module install the registry does not list.
-		if err := appendManifestPlugins(filepath.Join(app, manifestName), []Plugin{{ID: "acme.ghost", Module: "example.com/acme/ghost"}}); err != nil {
+		if _, err := appendManifestPlugins(filepath.Join(app, manifestName), []Plugin{{ID: "acme.ghost", Module: "example.com/acme/ghost"}}); err != nil {
 			t.Fatal(err)
 		}
 		if err := runGo(t.Context(), app, "mod", "edit", "-require=example.com/acme/ghost@v0.1.0"); err != nil {
@@ -920,7 +920,7 @@ func TestInstallSmoke(t *testing.T) {
 		if err := runGo(t.Context(), app, "mod", "edit", "-require=example.com/acme/gone@v0.1.0"); err != nil {
 			t.Fatal(err)
 		}
-		if err := appendManifestPlugins(filepath.Join(app, manifestName), []Plugin{{ID: "acme.gone", Module: "example.com/acme/gone"}}); err != nil {
+		if _, err := appendManifestPlugins(filepath.Join(app, manifestName), []Plugin{{ID: "acme.gone", Module: "example.com/acme/gone"}}); err != nil {
 			t.Fatal(err)
 		}
 		gomod, _ := os.ReadFile(filepath.Join(app, "go.mod"))
@@ -997,7 +997,7 @@ func TestInstallSmoke(t *testing.T) {
 		}
 		for _, c := range cases {
 			write(t, path, c.in)
-			if err := appendManifestPlugins(path, []Plugin{{ID: "acme.blog", Module: blogModule}}); err != nil {
+			if _, err := appendManifestPlugins(path, []Plugin{{ID: "acme.blog", Module: blogModule}}); err != nil {
 				t.Fatalf("%s: %v", c.name, err)
 			}
 			got, _ := os.ReadFile(path)
@@ -1005,17 +1005,19 @@ func TestInstallSmoke(t *testing.T) {
 				t.Fatalf("%s:\n got %q\nwant %q", c.name, got, c.want)
 			}
 		}
-		inline := "module: example.com/app\nbinary: app\nplugins: [{id: a.one, module: example.com/one}]\n"
+		// An inline list becomes a block list with the new item (WR-11).
+		inline := "# App.\nmodule: example.com/app\nbinary: app\nplugins: [{id: a.one, module: example.com/one}]\n"
 		write(t, path, inline)
-		if err := appendManifestPlugins(path, []Plugin{{ID: "acme.blog", Module: blogModule}}); err == nil {
-			t.Fatal("inline plugins list: want an error")
+		if rewrote, err := appendManifestPlugins(path, []Plugin{{ID: "acme.blog", Module: blogModule}}); err != nil || rewrote {
+			t.Fatalf("inline plugins list: %v, rewrote %v", err, rewrote)
 		}
-		if got, _ := os.ReadFile(path); string(got) != inline {
-			t.Fatalf("inline list was changed: %q", got)
+		want := "# App.\nmodule: example.com/app\nbinary: app\nplugins:\n  - id: a.one\n    module: example.com/one\n  - id: acme.blog\n    module: " + blogModule + "\n"
+		if got, _ := os.ReadFile(path); string(got) != want {
+			t.Fatalf("inline list:\n got %q\nwant %q", got, want)
 		}
 		dup := "module: example.com/app\nbinary: app\nplugins:\n  - id: acme.blog\n    module: example.com/other\n"
 		write(t, path, dup)
-		if err := appendManifestPlugins(path, []Plugin{{ID: "acme.blog", Module: blogModule}}); err == nil {
+		if _, err := appendManifestPlugins(path, []Plugin{{ID: "acme.blog", Module: blogModule}}); err == nil {
 			t.Fatal("duplicate ID: want an error")
 		}
 		if got, _ := os.ReadFile(path); string(got) != dup {

internal/build/manifest.go

@@ -1,6 +1,7 @@
 package build
 
 import (
+	"errors"
 	"fmt"
 	"os"
 	"path/filepath"
@@ -184,34 +185,52 @@ func SaveManifest(path string, m Manifest) error {
 // summer.yaml at path by inserting text, so comments, blank lines and the
 // order of the other keys stay as they are. The entries take the
 // indentation of the existing items; a file without a plugins key gets a
-// new block at its end. The result is parsed again with LoadManifest, and
-// any error restores the original bytes.
-func appendManifestPlugins(path string, plugins []Plugin) error {
+// new block at its end, and an inline (flow) plugins list is rewritten as
+// a block list with the new entries. The result is parsed again with
+// LoadManifest, and any error restores the original bytes. When an inline
+// list cannot be edited in place, the whole file is written again with
+// SaveManifest, as plugin:add did before (WR-11): rewrote is then true and
+// the file's comments are gone.
+func appendManifestPlugins(path string, plugins []Plugin) (rewrote bool, err error) {
 	if len(plugins) == 0 {
-		return nil
+		return false, nil
 	}
 	for i, p := range plugins {
 		if err := ValidatePluginID(p.ID); err != nil {
-			return fmt.Errorf("build: plugins[%d]: %w", i, err)
+			return false, fmt.Errorf("build: plugins[%d]: %w", i, err)
 		}
 		if err := validateModulePath(p.Module); err != nil {
-			return fmt.Errorf("build: plugins[%d] module: %w", i, err)
+			return false, fmt.Errorf("build: plugins[%d] module: %w", i, err)
 		}
 	}
 	orig, err := os.ReadFile(path)
 	if err != nil {
-		return fmt.Errorf("build: read %s: %w", path, err)
+		return false, fmt.Errorf("build: read %s: %w", path, err)
 	}
 	before, err := LoadManifest(path)
 	if err != nil {
-		return err
+		return false, err
+	}
+	want := append(append([]Plugin(nil), before.Plugins...), plugins...)
+	// rewrite is the fallback for an inline list the text edit cannot
+	// handle: the whole manifest again, as SaveManifest writes it.
+	rewrite := func() (bool, error) {
+		m := before
+		m.Plugins = want
+		if err := SaveManifest(path, m); err != nil {
+			return false, err
+		}
+		return true, nil
+	}
+	updated, err := insertManifestPlugins(string(orig), before.Plugins, plugins)
+	if errors.Is(err, errInlinePlugins) {
+		return rewrite()
 	}
-	updated, err := insertManifestPlugins(string(orig), plugins)
 	if err != nil {
-		return fmt.Errorf("build: edit %s: %w", path, err)
+		return false, fmt.Errorf("build: edit %s: %w", path, err)
 	}
 	if err := os.WriteFile(path, []byte(updated), 0o644); err != nil {
-		return fmt.Errorf("build: write %s: %w", path, err)
+		return false, fmt.Errorf("build: write %s: %w", path, err)
 	}
 	restore := func(cause error) error {
 		if werr := os.WriteFile(path, orig, 0o644); werr != nil {
@@ -221,23 +240,28 @@ func appendManifestPlugins(path string, plugins []Plugin) error {
 	}
 	after, err := LoadManifest(path)
 	if err != nil {
-		return restore(fmt.Errorf("build: summer.yaml edit did not parse, left unchanged: %w", err))
+		return false, restore(fmt.Errorf("build: summer.yaml edit did not parse, left unchanged: %w", err))
 	}
-	want := append(append([]Plugin(nil), before.Plugins...), plugins...)
 	if len(after.Plugins) != len(want) {
-		return restore(fmt.Errorf("build: summer.yaml edit gave %d plugins instead of %d, left unchanged", len(after.Plugins), len(want)))
+		return false, restore(fmt.Errorf("build: summer.yaml edit gave %d plugins instead of %d, left unchanged", len(after.Plugins), len(want)))
 	}
 	for i := range want {
 		if after.Plugins[i] != want[i] {
-			return restore(fmt.Errorf("build: summer.yaml edit changed plugins[%d], left unchanged", i))
+			return false, restore(fmt.Errorf("build: summer.yaml edit changed plugins[%d], left unchanged", i))
 		}
 	}
-	return nil
+	return false, nil
 }
 
+// errInlinePlugins reports an inline plugins list the text edit cannot
+// rewrite in place.
+var errInlinePlugins = errors.New("the plugins list is written inline in a form summer cannot edit in place")
+
 // insertManifestPlugins returns src with plugins appended to its
-// top-level plugins block.
-func insertManifestPlugins(src string, plugins []Plugin) (string, error) {
+// top-level plugins block. An inline (flow) list holding existing is
+// replaced by a block list of existing and plugins; one whose end it
+// cannot find gives errInlinePlugins.
+func insertManifestPlugins(src string, existing, plugins []Plugin) (string, error) {
 	lines := strings.SplitAfter(src, "\n")
 	if n := len(lines); n > 0 && lines[n-1] == "" {
 		lines = lines[:n-1]
@@ -268,7 +292,21 @@ func insertManifestPlugins(src string, plugins []Plugin) (string, error) {
 	switch rest {
 	case "", "[]", "~", "null":
 	default:
-		return "", fmt.Errorf("the plugins list is written inline; write it as a block list (one \"- id:\" item per plugin) and run the command again")
+		// An inline list such as plugins: [{id: a, module: b}]: replace
+		// its lines with a block list of the same items and the new ones.
+		end := flowEnd(lines, key, len("plugins:"))
+		if end < 0 {
+			return "", errInlinePlugins
+		}
+		all := append(append([]Plugin(nil), existing...), plugins...)
+		var b strings.Builder
+		b.WriteString(strings.Join(lines[:key], ""))
+		b.WriteString("plugins:\n")
+		for _, p := range all {
+			fmt.Fprintf(&b, "  - id: %s\n    module: %s\n", p.ID, p.Module)
+		}
+		b.WriteString(strings.Join(lines[end+1:], ""))
+		return b.String(), nil
 	}
 	// The block runs until the next line that starts a top-level key.
 	end := key + 1
@@ -310,6 +348,54 @@ func insertManifestPlugins(src string, plugins []Plugin) (string, error) {
 	return out + render(dash, gap, keyIndent) + strings.Join(tail, ""), nil
 }
 
+// flowEnd returns the index of the line where the flow collection that
+// starts at lines[key][col:] closes, or -1 when it does not close, does
+// not start with "[", or has more than a comment after it. Quoted strings
+// are skipped; a "#" after a blank outside quotes starts a comment.
+func flowEnd(lines []string, key, col int) int {
+	depth, started := 0, false
+	var quote byte
+	for i := key; i < len(lines); i++ {
+		line := strings.TrimRight(lines[i], "\r\n")
+		j := 0
+		if i == key {
+			j = col
+		}
+		for ; j < len(line); j++ {
+			c := line[j]
+			switch {
+			case quote != 0:
+				if c == quote {
+					quote = 0
+				}
+			case c == '"' || c == '\'':
+				quote = c
+			case c == '#' && (j == 0 || line[j-1] == ' ' || line[j-1] == '\t'):
+				j = len(line)
+			case c == ' ' || c == '\t':
+			case !started && c != '[':
+				return -1
+			case c == '[' || c == '{':
+				depth++
+				started = true
+			case c == ']' || c == '}':
+				depth--
+				if depth == 0 {
+					after := strings.TrimSpace(line[j+1:])
+					if after != "" && !strings.HasPrefix(after, "#") {
+						return -1
+					}
+					return i
+				}
+			}
+		}
+		if quote != 0 {
+			return -1
+		}
+	}
+	return -1
+}
+
 // stripYAMLComment drops a trailing " # comment" from a YAML value.
 func stripYAMLComment(s string) string {
 	if i := strings.Index(s, " #"); i >= 0 {

internal/build/manifest_append_test.go

@@ -51,6 +51,22 @@ func TestAppendManifestPlugins(t *testing.T) {
 			want: "# App.\nmodule: example.test/app\nbinary: app\nplugins:\n" +
 				"  - id: acme.shop\n    module: example.test/sm-shop-plugin\n  - id: acme.pgsubs\n    module: example.test/pg/subs\n",
 		},
+		{
+			name: "inline list becomes a block with the new items",
+			src:  "# App.\nmodule: example.test/app\nbinary: app\nplugins: [{id: acme.user, module: example.test/u}, {id: acme.blog, module: \"example.test/b\"}] # core\n# Tags.\ntags: []\n",
+			want: "# App.\nmodule: example.test/app\nbinary: app\nplugins:\n" +
+				"  - id: acme.user\n    module: example.test/u\n  - id: acme.blog\n    module: example.test/b\n" +
+				"  - id: acme.shop\n    module: example.test/sm-shop-plugin\n  - id: acme.pgsubs\n    module: example.test/pg/subs\n" +
+				"# Tags.\ntags: []\n",
+		},
+		{
+			name: "inline list over several lines",
+			src:  "module: example.test/app\nbinary: app\nplugins: [\n  {id: acme.user, module: example.test/u}, # first\n  {id: acme.blog,\n   module: example.test/b},\n]\ntags: []\n",
+			want: "module: example.test/app\nbinary: app\nplugins:\n" +
+				"  - id: acme.user\n    module: example.test/u\n  - id: acme.blog\n    module: example.test/b\n" +
+				"  - id: acme.shop\n    module: example.test/sm-shop-plugin\n  - id: acme.pgsubs\n    module: example.test/pg/subs\n" +
+				"tags: []\n",
+		},
 		{
 			name: "empty inline list becomes a block",
 			src:  "module: example.test/app\nbinary: app\nplugins: [] # none yet\ntags: []\n",
@@ -63,7 +79,7 @@ func TestAppendManifestPlugins(t *testing.T) {
 		t.Run(c.name, func(t *testing.T) {
 			path := filepath.Join(t.TempDir(), manifestName)
 			write(t, path, c.src)
-			if err := appendManifestPlugins(path, add); err != nil {
+			if _, err := appendManifestPlugins(path, add); err != nil {
 				t.Fatalf("appendManifestPlugins: %v", err)
 			}
 			got, _ := os.ReadFile(path)
@@ -87,7 +103,6 @@ func TestAppendManifestPlugins(t *testing.T) {
 			plugins   []Plugin
 			want      string
 		}{
-			{"inline list", "module: example.test/app\nbinary: app\nplugins: [{id: acme.user, module: example.test/u}]\n", add, "inline"},
 			{"bad id", "module: example.test/app\nbinary: app\n", []Plugin{{ID: "Acme", Module: "example.test/x"}}, "plugins[0]"},
 			{"bad module", "module: example.test/app\nbinary: app\n", []Plugin{{ID: "acme.x", Module: "-x"}}, "module"},
 			{"broken file", "module: [\n", add, ""},
@@ -99,7 +114,7 @@ func TestAppendManifestPlugins(t *testing.T) {
 		for _, r := range refusals {
 			path := filepath.Join(t.TempDir(), manifestName)
 			write(t, path, r.src)
-			err := appendManifestPlugins(path, r.plugins)
+			_, err := appendManifestPlugins(path, r.plugins)
 			if err == nil || !strings.Contains(err.Error(), r.want) {
 				t.Errorf("%s: err = %v, want %q", r.name, err, r.want)
 			}
@@ -109,11 +124,29 @@ func TestAppendManifestPlugins(t *testing.T) {
 		}
 	})
 
+	t.Run("an inline list it cannot edit is written again", func(t *testing.T) {
+		// A tagged flow list: the text edit gives up and the manifest is
+		// written again, as plugin:add did before it kept comments.
+		path := filepath.Join(t.TempDir(), manifestName)
+		write(t, path, "# gone\nmodule: example.test/app\nbinary: app\nplugins: !!seq [{id: acme.user, module: example.test/u}]\n")
+		if _, err := LoadManifest(path); err != nil {
+			t.Fatalf("fixture does not parse: %v", err)
+		}
+		rewrote, err := appendManifestPlugins(path, add)
+		if err != nil || !rewrote {
+			t.Fatalf("rewrote %v, err %v", rewrote, err)
+		}
+		m, err := LoadManifest(path)
+		if err != nil || len(m.Plugins) != 3 || m.Plugins[0].ID != "acme.user" || m.Plugins[2] != add[1] {
+			t.Errorf("plugins = %+v, %v", m.Plugins, err)
+		}
+	})
+
 	t.Run("nothing to add", func(t *testing.T) {
 		path := filepath.Join(t.TempDir(), manifestName)
 		src := "module: example.test/app\nbinary: app\n"
 		write(t, path, src)
-		if err := appendManifestPlugins(path, nil); err != nil {
+		if _, err := appendManifestPlugins(path, nil); err != nil {
 			t.Fatal(err)
 		}
 		if got, _ := os.ReadFile(path); string(got) != src {
@@ -122,7 +155,7 @@ func TestAppendManifestPlugins(t *testing.T) {
 	})
 
 	t.Run("missing file", func(t *testing.T) {
-		if err := appendManifestPlugins(filepath.Join(t.TempDir(), manifestName), add); err == nil {
+		if _, err := appendManifestPlugins(filepath.Join(t.TempDir(), manifestName), add); err == nil {
 			t.Error("appendManifestPlugins on a missing file succeeded")
 		}
 	})

internal/build/scaffold.go

@@ -204,7 +204,11 @@ func AddPlugin(ctx context.Context, startDir, pluginDir string) error {
 	if already {
 		return nil
 	}
-	return appendManifestPlugins(manifestPath, []Plugin{{ID: id, Module: modPath}})
+	rewrote, err := appendManifestPlugins(manifestPath, []Plugin{{ID: id, Module: modPath}})
+	if rewrote {
+		fmt.Fprintf(os.Stderr, "note: %s had an inline plugins list summer could not edit in place; it was written again and its comments were not kept\n", manifestName)
+	}
+	return err
 }
 
 func pluginName(id string) string {