diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md index 6773ca8..464c515 100644 --- a/docs/roadmap/remediation.md +++ b/docs/roadmap/remediation.md @@ -1,6 +1,6 @@ # Configuration Discovery Review Findings -Status: finding 1 complete; findings 2–5 remain open. +Status: findings 1–2 complete; findings 3–5 remain open. Reviewed commit: `e50fee2`, including the discovery implementation commits after `424c897`. The review compared the implementation with the @@ -53,7 +53,14 @@ or require callers to omit an otherwise valid options value. ## 2. Preserve The Failing Candidate's Identity In Access Diagnostics -Priority: P2. Status: open. +Priority: P2. Status: complete. + +**Completed:** terminal probe failures retain the candidate descriptor and gain +normal file-source evidence at the acquisition boundary. Regression tests compare +ordinary and later-underlay failures with explicit-file errors, including source +IDs, placement, layers, and skipped missing candidates. A controlled permission +probe checks the diagnostic-to-file link. The load regression failed before the +fix; focused discovery/failure tests and `./scripts/check.sh` passed on Linux. **Location:** [discovery.go](../../internal/engine/discovery.go), lines 517–528, and the early resolver-error return in diff --git a/internal/engine/discovery.go b/internal/engine/discovery.go index c4d63b3..b6498e7 100644 --- a/internal/engine/discovery.go +++ b/internal/engine/discovery.go @@ -483,8 +483,23 @@ func defaultDiscoveryHostProbe() discoveryHostProbe { return discoveryHostProbe{evalSymlinks: filepath.EvalSymlinks, stat: os.Stat} } +// resolveSearchInput associates a terminal candidate failure with the same file +// evidence as ordinary acquisition. Exhaustion and base failures have no file. +func resolveSearchInput(source Source, sourceID string, helpers discoveryDirectoryHelpers, probe discoveryHostProbe) (Source, *SourceInfo, []Diagnostic) { + selected, diagnostics := resolveSearchSource(source, source.search, source.searchApp, source.searchRequired, helpers, probe) + if len(diagnostics) == 0 || selected.kind != sourceFile { + return selected, nil, diagnostics + } + info := &SourceInfo{ID: sourceID, Label: selected.label, Path: selected.path, Layer: LayerBase, Kind: SourceKindFile} + for index := range diagnostics { + diagnostics[index].Primary = &Location{SourceID: sourceID} + } + return selected, info, diagnostics +} + // resolveSearchSource selects one ordinary file source. A nil description leaves // an existing source untouched; the selected file is read later by readSource. +// A terminal candidate failure returns its file descriptor for error evidence. func resolveSearchSource(source Source, description *Search, application string, required bool, helpers discoveryDirectoryHelpers, probe discoveryHostProbe) (Source, []Diagnostic) { if description == nil { return source, nil @@ -519,13 +534,15 @@ func resolveSearchSource(source Source, description *Search, application string, if errors.Is(err, fs.ErrNotExist) { continue } - return Source{}, []Diagnostic{classifySourceError(err, "")} + return File(candidate), []Diagnostic{classifySourceError(err, "")} } if _, err := probe.stat(canonical); err != nil { if errors.Is(err, fs.ErrNotExist) { continue } - return Source{}, []Diagnostic{classifySourceError(err, "")} + failed := File(candidate) + failed.path = canonical + return failed, []Diagnostic{classifySourceError(err, "")} } return File(candidate), nil } diff --git a/internal/engine/discovery_load_test.go b/internal/engine/discovery_load_test.go index c9752c7..ff57e66 100644 --- a/internal/engine/discovery_load_test.go +++ b/internal/engine/discovery_load_test.go @@ -157,6 +157,56 @@ func TestSearchSelectedFileFailuresDoNotFallBack(t *testing.T) { }) } +func TestSearchProbeFailureRetainsFileEvidence(t *testing.T) { + t.Parallel() + + root := t.TempDir() + main := writeImportFile(t, root, "main.yml", "value: main\n") + underlay := writeImportFile(t, root, "underlay.yml", "settings: {first: underlay}\n") + bad := filepath.Join(main, "child.yml") + later := writeImportFile(t, root, "later.yml", "value: later\n") + missing := filepath.Join(root, "missing.yml") + for _, entry := range []struct { + name string + id string + load func(Source) (*Loaded[discoveryLoadConfig], error) + }{ + {"ordinary", "source-1", func(source Source) (*Loaded[discoveryLoadConfig], error) { + return Load(source, testSecretBinding(), Options[discoveryLoadConfig]{destination: Keys("settings")}) + }}, + {"underlay after acquired sources", "source-3", func(source Source) (*Loaded[discoveryLoadConfig], error) { + return LoadLayered(Input{Source: File(main)}, testSecretBinding(), LayeredOptions[discoveryLoadConfig]{ + Underlays: []Underlay{{Inputs: []Input{ + {Source: File(underlay)}, {Source: source, Destination: Keys("service", "settings")}, + }}}, + }) + }}, + } { + t.Run(entry.name, func(t *testing.T) { + _, err := entry.load(File(bad)) + var explicit *LoadError + if !errors.As(err, &explicit) || len(explicit.Diagnostics()) != 1 { + t.Fatalf("explicit failed file = %v", err) + } + if explicit.Diagnostics()[0].Code == CodeSourceMissing { + t.Skip("host classifies a non-directory ancestor as absence; controlled probe tests cover access failures") + } + result, err := entry.load(SearchConfig("", Paths(missing, bad, later))) + var searched *LoadError + if result != nil || !errors.As(err, &searched) { + t.Fatalf("failed search = %#v, %v", result, err) + } + if !reflect.DeepEqual(searched.Sources(), explicit.Sources()) || !reflect.DeepEqual(searched.Diagnostics(), explicit.Diagnostics()) { + t.Fatalf("search failure differs from explicit file:\nsources %#v / %#v\ndiagnostics %#v / %#v", searched.Sources(), explicit.Sources(), searched.Diagnostics(), explicit.Diagnostics()) + } + primary := searched.Diagnostics()[0].Primary + if primary == nil || primary.SourceID != entry.id { + t.Fatalf("failed candidate location = %#v, want %s", primary, entry.id) + } + }) + } +} + func TestSearchValidationPrecedesCandidateProbing(t *testing.T) { t.Parallel() diff --git a/internal/engine/discovery_test.go b/internal/engine/discovery_test.go index 67e42dd..65d8988 100644 --- a/internal/engine/discovery_test.go +++ b/internal/engine/discovery_test.go @@ -457,10 +457,16 @@ func TestResolveSearchStopsOnReachedProbeError(t *testing.T) { statCalls++ return nil, fs.ErrPermission }} - _, diagnostics := resolveSearchSource(Source{}, ptrSearch(Paths(first, second)), "", false, discoveryDirectoryHelpers{}, probe) - if len(diagnostics) != 1 || diagnostics[0].Code != CodeSourcePermission || diagnostics[0].Primary != nil || statCalls != 1 { + selected, info, diagnostics := resolveSearchInput(SearchConfig("", Paths(first, second)), "source-3", discoveryDirectoryHelpers{}, probe) + if len(diagnostics) != 1 || diagnostics[0].Code != CodeSourcePermission || statCalls != 1 { t.Fatalf("probe failure = %#v, stat calls = %d", diagnostics, statCalls) } + if selected.kind != sourceFile || info == nil || info.Path != canonicalTestPath(t, first) || info.Label != first || info.Kind != SourceKindFile || info.ID != "source-3" { + t.Fatalf("failed candidate evidence = %#v, %#v", selected, info) + } + if diagnostics[0].Primary == nil || diagnostics[0].Primary.SourceID != info.ID { + t.Fatalf("failed candidate diagnostic does not identify its file: %#v", diagnostics) + } } func TestResolveSearchLeavesLaterDirectoryUnresolvedAfterSelection(t *testing.T) { diff --git a/internal/engine/load_input.go b/internal/engine/load_input.go index a9c3ebc..e8c4004 100644 --- a/internal/engine/load_input.go +++ b/internal/engine/load_input.go @@ -102,11 +102,16 @@ func assembleBaseInputTrackedWithVersion[T any](source Source, options Options[T } func assembleBaseInputTrackedForRole[T any](source Source, options Options[T], destination Path, accounting *importAccounting, visited map[resourceIdentity]importRoute, hostToken *namespaceToken, versionState *sourceVersionState, role sourceVersionRole) (assembledInput, []Diagnostic) { + sourceID := "source-" + strconv.Itoa(accounting.sources+1) selectedSource := source if source.kind == sourceSearch { - resolved, diagnostics := resolveSearchSource(source, source.search, source.searchApp, source.searchRequired, defaultDiscoveryDirectoryHelpers(), defaultDiscoveryHostProbe()) + resolved, failedSource, diagnostics := resolveSearchInput(source, sourceID, defaultDiscoveryDirectoryHelpers(), defaultDiscoveryHostProbe()) if len(diagnostics) != 0 { - return assembledInput{source: source, versionState: versionState, accounting: accounting}, diagnostics + if failedSource != nil { + failedSource.Destination = destination.clone() + failedSource.placementMode = options.placementMode + } + return assembledInput{source: resolved, sources: sourceInfos(failedSource), versionState: versionState, accounting: accounting}, diagnostics } selectedSource = resolved if options.Filesystem != nil { @@ -121,7 +126,7 @@ func assembleBaseInputTrackedForRole[T any](source Source, options Options[T], d limits := defaultImportLimits() parseLimits := defaultParseLimits() parseLimits.source.remaining = limits.bytes - accounting.bytes - parsed, diagnostics := parseSource(selectedSource, "source-"+strconv.Itoa(accounting.sources+1), parseLimits) + parsed, diagnostics := parseSource(selectedSource, sourceID, parseLimits) if parsed.source != nil { parsed.source.Destination = destination.clone() parsed.source.placementMode = options.placementMode