Share secret traversal between resolution and validation

docs/roadmap/equivalence.md

@@ -9,6 +9,17 @@ or a requirement to refactor before release.
 
 ## 1. Share typed-secret slot traversal between resolution and mutation auditing
 
+**Status:** Complete. [secret_walk.go](../../internal/engine/secret_walk.go)
+now owns traversal for both operations, with explicit writable and read-only map
+handling. Resolution and mutation auditing retain separate leaf operations and
+lifecycle passes. The [container regression](../../internal/engine/secret_walk_test.go)
+checks resolved state, mutation paths, nil containers, and lookup counts through
+both passes without deriving expectations from the walker.
+
+Completion validation: the new container cases passed before and after the
+refactor. Focused secret/validation tests, `go test -race ./... -count=1`, and
+`./scripts/check.sh` passed.
+
 ### Evidence
 
 The tests in [secret_reference_test.go](../../internal/engine/secret_reference_test.go)

internal/engine/secret_reference.go

@@ -49,81 +49,45 @@ func resolveSecretReferences(root reflect.Value, schema *schemaDescriptor, input
 		lookup = func(string) (string, bool) { return "", false }
 	}
 	locations := &secretReferenceLocationResolver{input: input}
-	resolveSecretValue(root, schema.root, schema.secrets, Path{}, false, locations, lookup, &result)
+	walkSecretSlots(root, schema.root, secretSlotsWritable, func(value reflect.Value, path Path, optional bool) {
+		resolveSecretReference(value, schema.secrets, path, optional, locations, lookup, &result)
+	})
 	result.diagnostics = canonicalDiagnostics(result.diagnostics)
 	return result
 }
 
-func resolveSecretValue(value reflect.Value, descriptor *typeDescriptor, secrets SecretBinding, path Path, optional bool, locations *secretReferenceLocationResolver, lookup secretLookup, result *secretResolutionResult) {
-	if descriptor.kind == descriptorPointer {
-		if value.IsNil() {
-			return
+func resolveSecretReference(value reflect.Value, secrets SecretBinding, path Path, optional bool, locations *secretReferenceLocationResolver, lookup secretLookup, result *secretResolutionResult) {
+	reference := secrets.Read(value)
+	name := reference.Name
+	if name == "" {
+		code := CodeSecretRequired
+		message := "required secret reference is absent"
+		if optional || reference.Specified {
+			code = CodeSecretInvalidName
+			message = "secret environment name is empty"
 		}
-		resolveSecretValue(value.Elem(), descriptor.element, secrets, path, descriptor.element.kind == descriptorSecret, locations, lookup, result)
+		result.diagnostics = append(result.diagnostics, locations.diagnostic(code, path, message))
 		return
 	}
-
-	switch descriptor.kind {
-	case descriptorSecret:
-		reference := secrets.Read(value)
-		name := reference.Name
-		if name == "" {
-			code := CodeSecretRequired
-			message := "required secret reference is absent"
-			if optional || reference.Specified {
-				code = CodeSecretInvalidName
-				message = "secret environment name is empty"
-			}
-			result.diagnostics = append(result.diagnostics, locations.diagnostic(code, path, message))
-			return
-		}
-		if !validEnvironmentName(name) {
-			result.diagnostics = append(result.diagnostics, locations.diagnostic(CodeSecretInvalidName, path, "secret environment name is invalid"))
-			return
-		}
-		found, exists := result.lookups[name]
-		if !exists {
-			found.value, found.present = lookup(name)
-			result.lookups[name] = found
-		}
-		if !found.present {
-			result.diagnostics = append(result.diagnostics, locations.diagnostic(CodeSecretUnset, path, "secret environment variable is not set"))
-			return
-		}
-		if found.value == "" {
-			result.diagnostics = append(result.diagnostics, locations.diagnostic(CodeSecretEmpty, path, "secret environment variable is empty"))
-			return
-		}
-		value.Set(secrets.Make(SecretState{Name: name, Value: found.value, Specified: true, Resolved: true}))
-		result.references[path.String()] = name
-	case descriptorStruct:
-		for _, member := range descriptor.members {
-			memberPath := path
-			if !member.inline {
-				memberPath = path.Key(member.name)
-			}
-			resolveSecretValue(value.Field(member.index), member.descriptor, secrets, memberPath, false, locations, lookup, result)
-		}
-	case descriptorMap:
-		if value.IsNil() {
-			return
-		}
-		keys := value.MapKeys()
-		slices.SortFunc(keys, func(left, right reflect.Value) int { return cmp.Compare(left.String(), right.String()) })
-		for _, key := range keys {
-			entry := reflect.New(value.Type().Elem()).Elem()
-			entry.Set(value.MapIndex(key))
-			resolveSecretValue(entry, descriptor.element, secrets, path.Key(key.String()), false, locations, lookup, result)
-			value.SetMapIndex(key, entry)
-		}
-	case descriptorSlice, descriptorArray:
-		if descriptor.kind == descriptorSlice && value.IsNil() {
-			return
-		}
-		for index := 0; index < value.Len(); index++ {
-			resolveSecretValue(value.Index(index), descriptor.element, secrets, path.Index(index), false, locations, lookup, result)
-		}
+	if !validEnvironmentName(name) {
+		result.diagnostics = append(result.diagnostics, locations.diagnostic(CodeSecretInvalidName, path, "secret environment name is invalid"))
+		return
 	}
+	found, exists := result.lookups[name]
+	if !exists {
+		found.value, found.present = lookup(name)
+		result.lookups[name] = found
+	}
+	if !found.present {
+		result.diagnostics = append(result.diagnostics, locations.diagnostic(CodeSecretUnset, path, "secret environment variable is not set"))
+		return
+	}
+	if found.value == "" {
+		result.diagnostics = append(result.diagnostics, locations.diagnostic(CodeSecretEmpty, path, "secret environment variable is empty"))
+		return
+	}
+	value.Set(secrets.Make(SecretState{Name: name, Value: found.value, Specified: true, Resolved: true}))
+	result.references[path.String()] = name
 }
 
 func changedSecretStates(root reflect.Value, schema *schemaDescriptor, lookups map[string]secretLookupResult) []Path {
@@ -131,50 +95,15 @@ func changedSecretStates(root reflect.Value, schema *schemaDescriptor, lookups m
 	if schema == nil || schema.root == nil {
 		return nil
 	}
-	collectChangedSecretStates(root, schema.root, schema.secrets, Path{}, lookups, &changed)
-	slices.SortFunc(changed, func(left, right Path) int { return cmp.Compare(left.String(), right.String()) })
-	return changed
-}
-
-func collectChangedSecretStates(value reflect.Value, descriptor *typeDescriptor, secrets SecretBinding, path Path, lookups map[string]secretLookupResult, changed *[]Path) {
-	if descriptor.kind == descriptorPointer {
-		if !value.IsNil() {
-			collectChangedSecretStates(value.Elem(), descriptor.element, secrets, path, lookups, changed)
-		}
-		return
-	}
-	switch descriptor.kind {
-	case descriptorSecret:
-		reference := secrets.Read(value)
+	walkSecretSlots(root, schema.root, secretSlotsReadOnly, func(value reflect.Value, path Path, _ bool) {
+		reference := schema.secrets.Read(value)
 		expected, ok := lookups[reference.Name]
 		if ok && (!expected.present || expected.value == "" || !reference.Resolved || reference.Value != expected.value) {
-			*changed = append(*changed, path.clone())
+			changed = append(changed, path.clone())
 		}
-	case descriptorStruct:
-		for _, member := range descriptor.members {
-			memberPath := path
-			if !member.inline {
-				memberPath = path.Key(member.name)
-			}
-			collectChangedSecretStates(value.Field(member.index), member.descriptor, secrets, memberPath, lookups, changed)
-		}
-	case descriptorMap:
-		if value.IsNil() {
-			return
-		}
-		keys := value.MapKeys()
-		slices.SortFunc(keys, func(left, right reflect.Value) int { return cmp.Compare(left.String(), right.String()) })
-		for _, key := range keys {
-			collectChangedSecretStates(value.MapIndex(key), descriptor.element, secrets, path.Key(key.String()), lookups, changed)
-		}
-	case descriptorSlice, descriptorArray:
-		if descriptor.kind == descriptorSlice && value.IsNil() {
-			return
-		}
-		for index := 0; index < value.Len(); index++ {
-			collectChangedSecretStates(value.Index(index), descriptor.element, secrets, path.Index(index), lookups, changed)
-		}
-	}
+	})
+	slices.SortFunc(changed, func(left, right Path) int { return cmp.Compare(left.String(), right.String()) })
+	return changed
 }
 
 func cloneSecretReferences(values map[string]string) map[string]string {

internal/engine/secret_walk.go

@@ -0,0 +1,67 @@
+package engine
+
+import (
+	"cmp"
+	"reflect"
+	"slices"
+)
+
+type secretSlotAccess uint8
+
+const (
+	secretSlotsReadOnly secretSlotAccess = iota
+	secretSlotsWritable
+)
+
+// walkSecretSlots visits schema-bound secret references. Writable traversal
+// copies map entries into addressable values and writes them back; read-only
+// traversal borrows them. The visitor owns the operation performed at each slot.
+func walkSecretSlots(root reflect.Value, descriptor *typeDescriptor, access secretSlotAccess, visit func(reflect.Value, Path, bool)) {
+	var walk func(reflect.Value, *typeDescriptor, Path, bool)
+	walk = func(value reflect.Value, descriptor *typeDescriptor, path Path, optional bool) {
+		if descriptor.kind == descriptorPointer {
+			if !value.IsNil() {
+				walk(value.Elem(), descriptor.element, path, descriptor.element.kind == descriptorSecret)
+			}
+			return
+		}
+		switch descriptor.kind {
+		case descriptorSecret:
+			visit(value, path, optional)
+		case descriptorStruct:
+			for _, member := range descriptor.members {
+				memberPath := path
+				if !member.inline {
+					memberPath = path.Key(member.name)
+				}
+				walk(value.Field(member.index), member.descriptor, memberPath, false)
+			}
+		case descriptorMap:
+			if value.IsNil() {
+				return
+			}
+			keys := value.MapKeys()
+			slices.SortFunc(keys, func(left, right reflect.Value) int { return cmp.Compare(left.String(), right.String()) })
+			for _, key := range keys {
+				entry := value.MapIndex(key)
+				if access == secretSlotsWritable {
+					copy := reflect.New(value.Type().Elem()).Elem()
+					copy.Set(entry)
+					entry = copy
+				}
+				walk(entry, descriptor.element, path.Key(key.String()), false)
+				if access == secretSlotsWritable {
+					value.SetMapIndex(key, entry)
+				}
+			}
+		case descriptorSlice, descriptorArray:
+			if descriptor.kind == descriptorSlice && value.IsNil() {
+				return
+			}
+			for index := 0; index < value.Len(); index++ {
+				walk(value.Index(index), descriptor.element, path.Index(index), false)
+			}
+		}
+	}
+	walk(root, descriptor, Path{}, false)
+}

internal/engine/secret_walk_test.go

@@ -0,0 +1,137 @@
+package engine
+
+import (
+	"fmt"
+	"strings"
+	"testing"
+)
+
+func TestSecretResolutionAndMutationAuditCoverTypedContainers(t *testing.T) {
+	t.Parallel()
+	type record struct {
+		Token Secret `yaml:"token"`
+	}
+	type inline struct {
+		Token Secret `yaml:"inline_token"`
+	}
+	type config struct {
+		Inline         *inline            `yaml:",inline"`
+		Pointer        *Secret            `yaml:"pointer"`
+		Indirect       **Secret           `yaml:"indirect"`
+		Values         map[string]Secret  `yaml:"values"`
+		Pointers       map[string]*Secret `yaml:"pointers"`
+		Records        map[string]record  `yaml:"records"`
+		PointerRecords map[string]*record `yaml:"pointer_records"`
+		Slice          []record           `yaml:"slice"`
+		Array          [1]Secret          `yaml:"array"`
+		Absent         *Secret            `yaml:"absent"`
+		NilMap         map[string]Secret  `yaml:"nil_map"`
+		NilSlice       []Secret           `yaml:"nil_slice"`
+	}
+	const input = `inline_token: WALK_SECRET
+pointer: WALK_SECRET
+indirect: WALK_SECRET
+values: {"a/b~c": WALK_SECRET}
+pointers: {entry: WALK_SECRET, absent: null}
+records: {entry: {token: WALK_SECRET}}
+pointer_records: {entry: {token: WALK_SECRET}, absent: null}
+slice: [{token: WALK_SECRET}]
+array: [WALK_SECRET]
+`
+	assertResolved := func(t *testing.T, value *config) {
+		t.Helper()
+		if value.Inline == nil || value.Pointer == nil || value.Indirect == nil || *value.Indirect == nil ||
+			value.Pointers["entry"] == nil || value.PointerRecords["entry"] == nil || len(value.Slice) != 1 {
+			t.Fatal("typed secret containers were not decoded")
+		}
+		for _, reference := range []Secret{
+			value.Inline.Token, *value.Pointer, **value.Indirect, value.Values["a/b~c"],
+			*value.Pointers["entry"], value.Records["entry"].Token, value.PointerRecords["entry"].Token,
+			value.Slice[0].Token, value.Array[0],
+		} {
+			if !reference.IsResolved() || reference.EnvName() != "WALK_SECRET" || reference.Value() != "WALK-CREDENTIAL-CANARY" {
+				t.Fatal("a typed secret did not retain its resolved state")
+			}
+		}
+		if value.Absent != nil || value.NilMap != nil || value.NilSlice != nil ||
+			value.Pointers["absent"] != nil || value.PointerRecords["absent"] != nil {
+			t.Fatal("nil secret containers changed")
+		}
+	}
+	for _, test := range []struct {
+		name   string
+		path   string
+		mutate func(*config)
+	}{
+		{name: "read-only validation"},
+		{name: "inline pointer", path: "/inline_token", mutate: func(value *config) {
+			value.Inline.Token = SecretEnv(value.Inline.Token.EnvName())
+		}},
+		{name: "pointer", path: "/pointer", mutate: func(value *config) {
+			*value.Pointer = SecretEnv(value.Pointer.EnvName())
+		}},
+		{name: "pointer chain", path: "/indirect", mutate: func(value *config) {
+			**value.Indirect = SecretEnv((**value.Indirect).EnvName())
+		}},
+		{name: "map value", path: "/values/a~1b~0c", mutate: func(value *config) {
+			value.Values["a/b~c"] = SecretEnv(value.Values["a/b~c"].EnvName())
+		}},
+		{name: "map pointer", path: "/pointers/entry", mutate: func(value *config) {
+			*value.Pointers["entry"] = SecretEnv(value.Pointers["entry"].EnvName())
+		}},
+		{name: "map record", path: "/records/entry/token", mutate: func(value *config) {
+			entry := value.Records["entry"]
+			entry.Token = SecretEnv(entry.Token.EnvName())
+			value.Records["entry"] = entry
+		}},
+		{name: "map record pointer", path: "/pointer_records/entry/token", mutate: func(value *config) {
+			entry := value.PointerRecords["entry"]
+			entry.Token = SecretEnv(entry.Token.EnvName())
+		}},
+		{name: "slice record", path: "/slice/0/token", mutate: func(value *config) {
+			value.Slice[0].Token = SecretEnv(value.Slice[0].Token.EnvName())
+		}},
+		{name: "array", path: "/array/0", mutate: func(value *config) {
+			value.Array[0] = SecretEnv(value.Array[0].EnvName())
+		}},
+	} {
+		t.Run(test.name, func(t *testing.T) {
+			lookups := 0
+			validated := false
+			result, err := testLoadWithSecretLookup(Bytes("secret-containers.yml", []byte(input)), func(name string) (string, bool) {
+				lookups++
+				if name != "WALK_SECRET" {
+					t.Fatalf("unexpected secret name %q", name)
+				}
+				return "WALK-CREDENTIAL-CANARY", true
+			}, Options[config]{Definition: Definition[config]{Validate: func(value *config, _ *Validation) {
+				validated = true
+				assertResolved(t, value)
+				if test.mutate != nil {
+					test.mutate(value)
+				}
+			}}})
+			if !validated || lookups != 1 {
+				t.Fatalf("validation ran = %v, lookups = %d", validated, lookups)
+			}
+			if test.mutate == nil {
+				if err != nil {
+					t.Fatal(err)
+				}
+				assertResolved(t, &result.Config)
+				return
+			}
+			if result != nil {
+				t.Fatal("validator mutation published a result")
+			}
+			loadError := requireSecretLoadCode(t, err, CodeValidateMutation)
+			diagnostics := loadError.Diagnostics()
+			if len(diagnostics) != 1 || diagnostics[0].Path.String() != test.path {
+				t.Fatalf("mutation diagnostics = %#v, want one at %s", diagnostics, test.path)
+			}
+			if strings.Contains(fmt.Sprintf("%v %#v", err, err), "WALK-CREDENTIAL-CANARY") {
+				t.Fatal("validator mutation error exposed a credential")
+			}
+		})
+	}
+}