The restart line names no part of a value; the new rows join existing tests (issue 1966, review round 1)

Claude-Session: https://claude.ai/code/session_01JG73KEq4zoDfM9WtYpcQgZ

internal/apply/process.go

@@ -562,8 +562,7 @@ func DaemonRoot() string { return daemonRoot }
 
 // unitDifference names what differs between what a process's record says was written and what is wanted now, both
 // "sha256:<hex> <unit>": its bundle when the digest differs, and in the unit file each directive and each environment
-// word that differs, with inside a word the keys of a JSON object (and, one level down, of its objects) or the names of
-// the name=value entries of a list that differ. Names only, never a value, so a secret in a word is never said. Empty
+// word that differs, with inside a word the keys of a JSON object (and, one level down, of its objects) that differ. Names only, never a value, so a secret in a word is never said. Empty
 // when nothing differs or there was no record.
 func unitDifference(was, want string) string {
 	if was == "" {
@@ -630,40 +629,11 @@ func valueDifference(was, now string, depth int) string {
 			if string(a[k]) == string(b[k]) {
 				continue
 			}
-			inner := ""
-			var x, y string
-			if json.Unmarshal(a[k], &x) == nil && json.Unmarshal(b[k], &y) == nil {
-				inner = valueDifference(x, y, 0)
-			} else {
-				inner = valueDifference(string(a[k]), string(b[k]), depth-1)
-			}
+			inner := valueDifference(string(a[k]), string(b[k]), depth-1)
 			names = append(names, k+inner)
 		}
 		return " (" + strings.Join(names, ", ") + ")"
 	}
-	entries := func(list string) map[string]string {
-		m := map[string]string{}
-		for _, e := range strings.Split(list, ",") {
-			if name, _, ok := strings.Cut(e, "="); ok {
-				m[e] = name
-			}
-		}
-		return m
-	}
-	before, after := entries(was), entries(now)
-	named := map[string]bool{}
-	for e, name := range before {
-		if _, ok := after[e]; !ok {
-			named[name] = true
-		}
-	}
-	for e, name := range after {
-		if _, ok := before[e]; !ok {
-			named[name] = true
-		}
-	}
-	if len(named) == 0 {
-		return ""
-	}
-	return " (" + strings.Join(slices.Sorted(maps.Keys(named)), ", ") + ")"
+	// Anything else is a value: said as differing, never split, since a name before an "=" in it can be a secret.
+	return ""
 }

internal/apply/process_test.go

@@ -364,30 +364,27 @@ func TestAProcessAppliedAgainIsUnchangedAndNotRestarted(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	if o := outcomeOf(fourth, "node-tools.runtime"); o.Action != "updated" || !strings.Contains(o.Detail, "its unit's Environment MESH_TOOL_MODULES (b)") {
-		t.Errorf("fourth apply: %+v, want updated and the word and entry that changed named", o)
+	if o := outcomeOf(fourth, "node-tools.runtime"); o.Action != "updated" || !strings.Contains(o.Detail, "its unit's Environment MESH_TOOL_MODULES") ||
+		strings.Contains(o.Detail, "/y/") {
+		t.Errorf("fourth apply: %+v, want updated and the word that changed named, no value", o)
 	}
-}
-
-// A restart nobody asked for names its cause (issue 1966): the unit's own words that differ, the key inside a word that
-// is a JSON object and the entry of a list, never a value.
-func TestAProcessRestartedForAnInputSaysWhichInputChanged(t *testing.T) {
 	for _, row := range []struct {
 		name, was, now, says string
 	}{
 		{"a word of a module inside a JSON word", `{"files":{"OWNED":"/a"},"mail":{"X":"1"}}`, `{"files":{"OWNED":"/b"},"mail":{"X":"1"}}`,
 			"Environment TOOL_ENV (files (OWNED))"},
-		{"an entry of a list", "a=/x/1,b=/y/2", "a=/x/1,b=/y/3", "Environment TOOL_MODULES (b)"},
+		{"a secret-shaped value", "c2VjcmV0MQ==", "c2VjcmV0Mg==", "Environment TOOL_VALUE"},
+		{"a URL with a password", "postgres://u:hunter1@h/db?sslmode=x", "postgres://u:hunter2@h/db?sslmode=x", "Environment TOOL_VALUE"},
 		{"nothing changed", "a=/x/1", "a=/x/1", ""},
 	} {
 		t.Run(row.name, func(t *testing.T) {
-			word := "TOOL_MODULES"
+			word := "TOOL_VALUE"
 			if strings.HasPrefix(row.was, "{") {
 				word = "TOOL_ENV"
 			}
+			r := aProcess()
 			mk := func(v string) string {
-				r := aProcess()
-				r.Env = map[string]string{word: v, "SECRET": "never-said-" + v}
+				r.Env = map[string]string{word: v}
 				return "sha256:" + strings.Repeat("a", 64) + " " + unitFor(r)
 			}
 			got := unitDifference(mk(row.was), mk(row.now))
@@ -397,8 +394,13 @@ func TestAProcessRestartedForAnInputSaysWhichInputChanged(t *testing.T) {
 				}
 				return
 			}
-			if !strings.Contains(got, row.says) || strings.Contains(got, "never-said") || strings.Contains(got, "/b") {
-				t.Fatalf("said %q, want it to name %q and no value", got, row.says)
+			for _, leak := range []string{"c2VjcmV0", "hunter", "sslmode", "postgres", "/b"} {
+				if strings.Contains(got, leak) {
+					t.Fatalf("said %q, which holds part of a value (%s)", got, leak)
+				}
+			}
+			if !strings.Contains(got, row.says) {
+				t.Fatalf("said %q, want it to name %q", got, row.says)
 			}
 		})
 	}

internal/compose/resolution/seat_paths_test.go

@@ -28,6 +28,26 @@ func TestAGeneratedResourceOnASeatsPathIsRefused(t *testing.T) {
 			t.Errorf("the refusal does not name %s: %v", want, err)
 		}
 	}
+	// And the paths a module owns do not change with its build (issue 1966): the holder of node-files is told them in a
+	// word of the tool runner's unit, so a place named for each build restarted the tool runner with each build.
+	digest := "sha256:" + strings.Repeat("ab12", 16)
+	for _, row := range []struct {
+		name string
+		res  map[string]any
+		owns string
+	}{
+		{"a build's own place", map[string]any{"id": "next", "type": "archive", "digest": digest, "path": "/usr/lib/x/versions/ab12ab12ab12"}, "/usr/lib/x/versions"},
+		{"another build's place", map[string]any{"id": "next", "type": "archive", "digest": "sha256:" + strings.Repeat("cd34", 16), "path": "/usr/lib/x/versions/cd34cd34cd34"}, "/usr/lib/x/versions"},
+		{"a fixed place", map[string]any{"id": "app", "type": "archive", "digest": digest, "path": "/opt/app"}, "/opt/app"},
+		{"a file", map[string]any{"id": "f", "type": "file", "path": "/etc/f"}, "/etc/f"},
+	} {
+		t.Run(row.name, func(t *testing.T) {
+			got := DeclaredPaths(mcat.Manifest{Module: "m", Resources: []map[string]any{row.res}})
+			if len(got) != 1 || got[0] != row.owns {
+				t.Fatalf("owns %v, want [%s]", got, row.owns)
+			}
+		})
+	}
 }
 
 // The stepping back comes from the seat, and is of a rendered file only: an uplink holder declaring the
@@ -54,27 +74,3 @@ func TestAnUplinkDeclaringTheResolverFileAsAResourceIsRefusedBesideTheResolver(t
 		t.Errorf("the uplink's holder renders %v beside the resolver", got.Facts)
 	}
 }
-
-// The paths a module owns do not change with its build (issue 1966): the holder of node-files is told them in an
-// environment word of the tool runner's unit, so a place named for each build moved with every build of its module and
-// restarted the tool runner each time.
-func TestAPathNamedForABuildIsOwnedAsTheDirectoryHoldingEveryBuild(t *testing.T) {
-	digest := "sha256:" + strings.Repeat("ab12", 16)
-	for _, row := range []struct {
-		name string
-		res  map[string]any
-		owns string
-	}{
-		{"a build's own place", map[string]any{"id": "next", "type": "archive", "digest": digest, "path": "/usr/lib/x/versions/ab12ab12ab12"}, "/usr/lib/x/versions"},
-		{"another build's place", map[string]any{"id": "next", "type": "archive", "digest": "sha256:" + strings.Repeat("cd34", 16), "path": "/usr/lib/x/versions/cd34cd34cd34"}, "/usr/lib/x/versions"},
-		{"a fixed place", map[string]any{"id": "app", "type": "archive", "digest": digest, "path": "/opt/app"}, "/opt/app"},
-		{"a file", map[string]any{"id": "f", "type": "file", "path": "/etc/f"}, "/etc/f"},
-	} {
-		t.Run(row.name, func(t *testing.T) {
-			got := DeclaredPaths(mcat.Manifest{Module: "m", Resources: []map[string]any{row.res}})
-			if len(got) != 1 || got[0] != row.owns {
-				t.Fatalf("owns %v, want [%s]", got, row.owns)
-			}
-		})
-	}
-}