Preserve all formatting on YAML --preserve set (no more flow-style collapse)

services/preserve_service.go

@@ -68,8 +68,14 @@ func setYAMLPreserving(data []byte, path string, value interface{}) ([]byte, err
 	if out, ok := tryYAMLScalarSplice(data, parts, value, path); ok {
 		return out, nil
 	}
-	// Fallback (new keys, nested creation, complex values, multiline scalars):
-	// node re-encode. This normalizes formatting but always produces valid YAML.
+	// Surgical insert: a new scalar key added to an existing block mapping →
+	// splice one line, leaving every other byte (including multi-line flow style
+	// and blank lines elsewhere) untouched.
+	if out, ok := tryYAMLInsert(data, parts, value, path); ok {
+		return out, nil
+	}
+	// Fallback (nested creation, complex values, multiline scalars): node
+	// re-encode. This normalizes formatting but always produces valid YAML.
 	return setYAMLNodeEncode(data, parts, value)
 }
 
@@ -124,6 +130,167 @@ func tryYAMLScalarSplice(data []byte, parts []string, value interface{}, path st
 	return out, true
 }
 
+// tryYAMLInsert adds a new value by splicing freshly-built lines into the
+// original bytes. It anchors at the deepest existing block-style mapping along
+// the path and appends the missing tail (a single leaf key, or a nested block
+// when intermediate maps are missing too). Unlike the node re-encode fallback it
+// never reformats the rest of the document, so multi-line flow collections,
+// blank lines, comments and quoting elsewhere stay byte-for-byte identical.
+//
+// Returns false (defer to fallback) for anything it cannot do surgically:
+// non-scalar values, multi-line scalars, or no existing block-mapping anchor
+// (e.g. a flow-style or empty parent, or indexing into a scalar).
+func tryYAMLInsert(data []byte, parts []string, value interface{}, path string) ([]byte, bool) {
+	if !isScalarValue(value) {
+		return nil, false
+	}
+	valText, ok := yamlScalarToken(value)
+	if !ok {
+		return nil, false
+	}
+	top, ok := yamlTopNode(data)
+	if !ok {
+		return nil, false
+	}
+
+	// Find the deepest existing block mapping whose next path segment is absent;
+	// the remaining segments (>=1) become the inserted tail. Searching deepest
+	// first means a present leaf-parent yields a one-line insert.
+	anchor, tail, indentCol, ok := yamlInsertAnchor(top, parts)
+	if !ok {
+		return nil, false
+	}
+
+	firstKey := anchor.Content[0]
+	starts := lineStarts(data)
+	if firstKey.Line < 1 || firstKey.Line > len(starts) {
+		return nil, false
+	}
+	// Walk the mapping's lines to find the last one that belongs to it: blank
+	// and deeper-indented lines belong; the first line dedented past the key
+	// column ends the block. We insert right after the last belonging content
+	// line, so trailing blank lines stay below the new entry.
+	lastContent := 0
+	for ln := firstKey.Line; ln <= len(starts); ln++ {
+		s := starts[ln-1]
+		e := len(data)
+		if ln < len(starts) {
+			e = starts[ln]
+		}
+		lineCol, blank := firstNonSpaceCol(data[s:e])
+		if blank {
+			continue
+		}
+		if lineCol < indentCol {
+			break // dedent: belongs to an ancestor mapping
+		}
+		lastContent = ln
+	}
+	if lastContent == 0 {
+		return nil, false
+	}
+
+	insertOff := len(data)
+	if lastContent < len(starts) {
+		insertOff = starts[lastContent]
+	}
+	prefix := ""
+	if insertOff == len(data) && len(data) > 0 && data[len(data)-1] != '\n' {
+		prefix = "\n"
+	}
+	entry := prefix + buildYAMLBlock(tail, indentCol, valText)
+	out := splice(data, insertOff, insertOff, []byte(entry))
+	// Validate: a wrong insertion point would put the value at the wrong path (or
+	// break parsing); confirm the value now resolves at the requested path.
+	if !yamlPathHasValue(out, path, value) {
+		return nil, false
+	}
+	return out, true
+}
+
+// yamlInsertAnchor returns the deepest block-style mapping along parts whose
+// next segment is missing, the remaining (missing) path segments, and the
+// 1-based column at which that mapping's keys sit. ok is false when no such
+// anchor exists (so the surgical insert is impossible).
+func yamlInsertAnchor(top *yaml.Node, parts []string) (anchor *yaml.Node, tail []string, indentCol int, ok bool) {
+	for d := len(parts) - 1; d >= 0; d-- {
+		node, found := findYAMLValueNode(top, parts[:d])
+		if !found {
+			continue
+		}
+		if node.Kind != yaml.MappingNode || node.Style == yaml.FlowStyle || len(node.Content) == 0 {
+			continue
+		}
+		if mappingHasKey(node, parts[d]) {
+			// The next segment already exists; we can't insert it here. A deeper
+			// anchor was already tried, so this path can't be done surgically.
+			return nil, nil, 0, false
+		}
+		return node, parts[d:], node.Content[0].Column, true
+	}
+	return nil, nil, 0, false
+}
+
+func mappingHasKey(node *yaml.Node, key string) bool {
+	for i := 0; i+1 < len(node.Content); i += 2 {
+		if node.Content[i].Value == key {
+			return true
+		}
+	}
+	return false
+}
+
+// buildYAMLBlock renders tail (>=1 map keys) as a block-style YAML fragment
+// indented to baseCol (1-based), with valText as the innermost scalar. Nested
+// levels add 2 spaces each, matching the encoder's default indent.
+func buildYAMLBlock(tail []string, baseCol int, valText string) string {
+	var b strings.Builder
+	for i, seg := range tail {
+		b.WriteString(strings.Repeat(" ", baseCol-1+i*2))
+		b.WriteString(seg)
+		if i == len(tail)-1 {
+			b.WriteString(": ")
+			b.WriteString(valText)
+		} else {
+			b.WriteString(":")
+		}
+		b.WriteString("\n")
+	}
+	return b.String()
+}
+
+// yamlScalarToken renders value as a single-line YAML scalar token (with the
+// minimal quoting yaml.v3 would choose), or false if it cannot be expressed on
+// one line.
+func yamlScalarToken(value interface{}) (string, bool) {
+	b, err := yaml.Marshal(value)
+	if err != nil {
+		return "", false
+	}
+	s := strings.TrimRight(string(b), "\n")
+	if strings.ContainsAny(s, "\n\r") {
+		return "", false // block scalar / multi-line: not a single-line insert
+	}
+	return s, true
+}
+
+// firstNonSpaceCol returns the 1-based column of the first non-space character
+// on a line, and whether the line is blank (only spaces/tabs, ignoring the
+// trailing newline).
+func firstNonSpaceCol(line []byte) (int, bool) {
+	for i := 0; i < len(line); i++ {
+		switch line[i] {
+		case ' ', '\t':
+			continue
+		case '\n', '\r':
+			return 0, true
+		default:
+			return i + 1, false
+		}
+	}
+	return 0, true
+}
+
 func tryYAMLLeafDelete(data []byte, parts []string, path string) ([]byte, bool) {
 	top, ok := yamlTopNode(data)
 	if !ok {

services/preserve_service_test.go

@@ -94,6 +94,182 @@ func TestSetYAMLPreserving_AddsNewKey(t *testing.T) {
 	}
 }
 
+// TestSetYAMLPreserving_InsertKeepsMultilineFlowStyle reproduces the original
+// regression: inserting a new scalar key must NOT collapse multi-line flow-style
+// collections ([ ... ] / { ... } spread across lines) elsewhere in the document.
+// The whole file outside the single inserted line stays byte-for-byte identical.
+func TestSetYAMLPreserving_InsertKeepsMultilineFlowStyle(t *testing.T) {
+	in := []byte("deployment:\n" +
+		"  args: [\n" +
+		"    '--config',\n" +
+		"    '/etc/app.yaml',\n" +
+		"    '--verbose'\n" +
+		"  ]\n" +
+		"  annotations: {\n" +
+		"    team: platform,\n" +
+		"    tier: backend\n" +
+		"  }\n" +
+		"image:\n" +
+		"  repository: myapp\n")
+	want := "deployment:\n" +
+		"  args: [\n" +
+		"    '--config',\n" +
+		"    '/etc/app.yaml',\n" +
+		"    '--verbose'\n" +
+		"  ]\n" +
+		"  annotations: {\n" +
+		"    team: platform,\n" +
+		"    tier: backend\n" +
+		"  }\n" +
+		"image:\n" +
+		"  repository: myapp\n" +
+		"  tag: v1.4.0\n"
+	out, err := setYAMLPreserving(in, ".image.tag", "v1.4.0")
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if string(out) != want {
+		t.Errorf("multi-line flow style not preserved on insert:\ngot:\n%q\nwant:\n%q", out, want)
+	}
+}
+
+// TestSetYAMLPreserving_InsertByteForByte covers blank lines, comment column
+// alignment and quoting around the insertion point.
+func TestSetYAMLPreserving_InsertByteForByte(t *testing.T) {
+	in := []byte("# Chart values\n" +
+		"image:\n" +
+		"  repository: myapp     # registry path\n" +
+		"\n" +
+		"replicas: 3\n")
+	want := "# Chart values\n" +
+		"image:\n" +
+		"  repository: myapp     # registry path\n" +
+		"  tag: v2.0.0\n" +
+		"\n" +
+		"replicas: 3\n"
+	out, err := setYAMLPreserving(in, ".image.tag", "v2.0.0")
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if string(out) != want {
+		t.Errorf("insert not byte-for-byte:\ngot:\n%q\nwant:\n%q", out, want)
+	}
+}
+
+func TestSetYAMLPreserving_InsertTopLevelKey(t *testing.T) {
+	in := []byte("a: 1\nb: 2\n")
+	want := "a: 1\nb: 2\nc: 3\n"
+	out, err := setYAMLPreserving(in, ".c", float64(3))
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if string(out) != want {
+		t.Errorf("got %q want %q", out, want)
+	}
+}
+
+func TestSetYAMLPreserving_InsertNoTrailingNewline(t *testing.T) {
+	in := []byte("image:\n  repository: myapp")
+	want := "image:\n  repository: myapp\n  tag: v1\n"
+	out, err := setYAMLPreserving(in, ".image.tag", "v1")
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if string(out) != want {
+		t.Errorf("got %q want %q", out, want)
+	}
+}
+
+// A string that YAML would otherwise read as a bool/number must be quoted on
+// insert so it round-trips as a string.
+func TestSetYAMLPreserving_InsertQuotesAmbiguousString(t *testing.T) {
+	in := []byte("flags:\n  verbose: true\n")
+	out, err := setYAMLPreserving(in, ".flags.enabled", "yes")
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	got := string(out)
+	if !strings.Contains(got, "enabled: \"yes\"") {
+		t.Errorf("ambiguous string not quoted on insert: %q", got)
+	}
+	if !yamlPathHasValue(out, ".flags.enabled", "yes") {
+		t.Errorf("inserted value does not round-trip as string: %q", got)
+	}
+}
+
+// Inserting into a deeper block must not disturb a multi-line flow sibling that
+// comes after it in the document.
+func TestSetYAMLPreserving_InsertBeforeFlowSibling(t *testing.T) {
+	in := []byte("image:\n" +
+		"  repository: myapp\n" +
+		"ports: [\n" +
+		"  8080,\n" +
+		"  9090\n" +
+		"]\n")
+	want := "image:\n" +
+		"  repository: myapp\n" +
+		"  tag: v1\n" +
+		"ports: [\n" +
+		"  8080,\n" +
+		"  9090\n" +
+		"]\n"
+	out, err := setYAMLPreserving(in, ".image.tag", "v1")
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if string(out) != want {
+		t.Errorf("got:\n%q\nwant:\n%q", out, want)
+	}
+}
+
+// When an intermediate map is missing too, the whole missing tail is created as
+// a nested block — and unrelated multi-line flow style is still left untouched.
+func TestSetYAMLPreserving_InsertCreatesNestedBlock(t *testing.T) {
+	in := []byte("service:\n" +
+		"  ports: [\n" +
+		"    80,\n" +
+		"    443\n" +
+		"  ]\n")
+	want := "service:\n" +
+		"  ports: [\n" +
+		"    80,\n" +
+		"    443\n" +
+		"  ]\n" +
+		"image:\n" +
+		"  tag: v1.4.0\n"
+	out, err := setYAMLPreserving(in, ".image.tag", "v1.4.0")
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if string(out) != want {
+		t.Errorf("nested create not surgical:\ngot:\n%q\nwant:\n%q", out, want)
+	}
+}
+
+func TestSetYAMLPreserving_InsertCreatesDeepNestedBlock(t *testing.T) {
+	in := []byte("name: app\n")
+	want := "name: app\n" +
+		"a:\n" +
+		"  b:\n" +
+		"    c: 5\n"
+	out, err := setYAMLPreserving(in, ".a.b.c", float64(5))
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if string(out) != want {
+		t.Errorf("deep nested create not surgical:\ngot:\n%q\nwant:\n%q", out, want)
+	}
+}
+
+// Indexing through a scalar is impossible; the surgical path must defer to the
+// fallback, which surfaces the error rather than producing wrong output.
+func TestSetYAMLPreserving_InsertUnderScalarErrors(t *testing.T) {
+	in := []byte("image: myapp\n")
+	if _, err := setYAMLPreserving(in, ".image.tag", "v1"); err == nil {
+		t.Fatal("expected error inserting under a scalar value")
+	}
+}
+
 func TestSetYAMLPreserving_RejectsIndexIntoScalar(t *testing.T) {
 	in := []byte("tag: v1.0.0\n")
 	if _, err := setYAMLPreserving(in, ".tag.inner", "x"); err == nil {