From 64be8b4df57b9cb3a0372896299d03e5401f06cf Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Sun, 25 Oct 2026 14:36:39 +0000 Subject: [PATCH] Keep import selection evidence with its declaring source --- docs/roadmap/remediation.md | 11 +++++- internal/engine/destination_import_test.go | 19 +++++----- internal/engine/import_acquisition.go | 32 ++++++++-------- internal/engine/import_evidence_test.go | 43 ++++++++++++++++++++++ internal/engine/import_scope.go | 2 +- 5 files changed, 79 insertions(+), 28 deletions(-) diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md index 864c999..3d773a9 100644 --- a/docs/roadmap/remediation.md +++ b/docs/roadmap/remediation.md @@ -111,7 +111,7 @@ input restrictions, performance thresholds, or release gates. ## 2. Avoid Searching All Sources For Each Empty Import Selection -Status: open. Priority: low. Category: algorithmic efficiency. +Status: complete (2026-10-25). Priority: low. Category: algorithmic efficiency. **Evidence.** `walkDeclarations` calls `recordEmptyImportSelection` for each optional declaration that admits no source. In @@ -141,6 +141,15 @@ A broad-fragment behavioral case is useful if existing coverage does not protect the chosen index handoff. Verify the removed scan by source review rather than adding timing thresholds or assertions about private lookup counts. +**Completion.** Declaration traversal now receives the declaring source's stable +integer position, captured at admission and carried through mapping and item +imports. Empty selections append evidence directly to that record without +searching the source slice or retaining pointers into it. The broad-fragment +test covers source-slice growth, missing-source rollback, and parent evidence +after child traversal; existing detached evidence, nested item, underlay, and +partial-failure tests remain effective. Focused tests, the development check, +and race-enabled import/destination tests passed with Go 1.26.10 on Linux. + ## 3. Restrict Import-Scope Edge Preparation To Relevant Nodes Status: open. Priority: medium. Category: algorithmic efficiency and allocation diff --git a/internal/engine/destination_import_test.go b/internal/engine/destination_import_test.go index 64654af..f5d3371 100644 --- a/internal/engine/destination_import_test.go +++ b/internal/engine/destination_import_test.go @@ -108,13 +108,14 @@ parent: true } defer func() { _ = access.close() }() state := importTraversal{ - access: access, - policy: &acquisitionPolicy{importLimits: defaultImportLimits()}, - state: &acquisitionState{accounting: &importAccounting{}}, - tier: &tierAcquisitionState{visited: make(map[resourceIdentity]importRoute)}, - active: make(map[resourceIdentity]int), + access: access, + policy: &acquisitionPolicy{importLimits: defaultImportLimits()}, + state: &acquisitionState{accounting: &importAccounting{sources: 1}}, + tier: &tierAcquisitionState{visited: make(map[resourceIdentity]importRoute)}, + sources: sourceInfos(main.source), + active: make(map[resourceIdentity]int), } - if diagnostics := state.walkDeclarations(directory, declarations, 0, Keys("entry")); len(diagnostics) != 0 { + if diagnostics := state.walkDeclarations(directory, declarations, 0, Keys("entry"), 0); len(diagnostics) != 0 { t.Fatalf("walk diagnostics = %#v", diagnostics) } @@ -128,11 +129,11 @@ parent: true {path: alphaPath, destination: "/entry/collection"}, {path: betaPath, destination: "/entry/collection"}, } - if len(state.sources) != len(wantSources) { - t.Fatalf("import sources = %#v, want %d", state.sources, len(wantSources)) + if len(state.sources) != len(wantSources)+1 { + t.Fatalf("sources = %#v, want %d including main", state.sources, len(wantSources)+1) } for index, want := range wantSources { - got := state.sources[index] + got := state.sources[index+1] if got.Path != canonicalTestPath(t, want.path) || got.Destination.String() != want.destination { t.Errorf("source %d = (%q, %q), want (%q, %q)", index, got.Path, got.Destination.String(), canonicalTestPath(t, want.path), want.destination) } diff --git a/internal/engine/import_acquisition.go b/internal/engine/import_acquisition.go index 7b09fef..c6a6965 100644 --- a/internal/engine/import_acquisition.go +++ b/internal/engine/import_acquisition.go @@ -22,6 +22,7 @@ type admittedImport struct { identity resourceIdentity nodeKey importNodeKey source SourceInfo + sourceIndex int payload *document.Node declarations []importDeclarationSpec sequenceDeclarations []sequenceDeclarationSpec @@ -156,7 +157,7 @@ func acquireImportedWithAccess(request importAcquisitionRequest, policy *acquisi scopeOwner: mainNodeKey, payload: body, document: body, processing: processing, sequenceDeclarations: cloneSequenceDeclarations(sequenceDeclarations), }) - diagnostics = state.walkDeclarations(basePath, declarations, 0, Path{}) + diagnostics = state.walkDeclarations(basePath, declarations, 0, Path{}, 0) } else { _, placed, err := placeDocument(body, destination, body.Location, maxDocumentDepth, maxDocumentNodes) placementNodes := destinationWrapperNodeCount(destination) @@ -172,7 +173,7 @@ func acquireImportedWithAccess(request importAcquisitionRequest, policy *acquisi destination: destination.clone(), processing: processing, sequenceDeclarations: cloneSequenceDeclarations(sequenceDeclarations), }) - diagnostics = state.walkDeclarations(basePath, declarations, 0, destination) + diagnostics = state.walkDeclarations(basePath, declarations, 0, destination, 0) } } } @@ -239,7 +240,7 @@ func (s *importTraversal) checkInitialLimits() *Diagnostic { return nil } -func (s *importTraversal) walkDeclarations(basePath string, declarations []importDeclarationSpec, depth int, parentDestination Path) []Diagnostic { +func (s *importTraversal) walkDeclarations(basePath string, declarations []importDeclarationSpec, depth int, parentDestination Path, sourceIndex int) []Diagnostic { policies := make([]resolvedImportPolicy, len(declarations)) for index, spec := range declarations { policy, diagnostics := resolveImportPolicy(spec, s.policy.importDefaults) @@ -280,23 +281,19 @@ func (s *importTraversal) walkDeclarations(basePath string, declarations []impor } } if !policy.required && len(s.sources) == sourcesBefore { - s.recordEmptyImportSelection(spec) + s.recordEmptyImportSelection(sourceIndex, spec) } } return nil } -func (s *importTraversal) recordEmptyImportSelection(spec importDeclarationSpec) { - for index := range s.sources { - if s.sources[index].ID != spec.location.SourceID { - continue - } - s.sources[index].importSelections = append(s.sources[index].importSelections, importSelectionEvidence{ - path: spec.path.clone(), location: spec.location, - selector: spec.selector, outcome: importSelectionNone, - }) - return - } +func (s *importTraversal) recordEmptyImportSelection(sourceIndex int, spec importDeclarationSpec) { + // The declaring source's position survives descendant appends and optional + // missing-source rollback, unlike a pointer into the source slice. + s.sources[sourceIndex].importSelections = append(s.sources[sourceIndex].importSelections, importSelectionEvidence{ + path: spec.path.clone(), location: spec.location, + selector: spec.selector, outcome: importSelectionNone, + }) } func (s *importTraversal) admitImportSource(canonical string, identity resourceIdentity, nodeKey importNodeKey, candidate selectedImportCandidate, policy resolvedImportPolicy, destination Path, location Location) (importAdmissionResult, []Diagnostic) { @@ -321,6 +318,7 @@ func (s *importTraversal) admitImportSource(canonical string, identity resourceI if kind == SourceKindFilesystem { info.namespace = s.access.token } + sourceIndex := len(s.sources) s.sources = append(s.sources, info) data, skipped, diagnostic := s.readImport(canonical, sourceID, policy.required) if diagnostic != nil { @@ -369,7 +367,7 @@ func (s *importTraversal) admitImportSource(canonical string, identity resourceI } return importAdmissionResult{source: admittedImport{ canonical: canonical, identity: identity, nodeKey: nodeKey, - source: cloneSourceInfo(info), payload: processing.payload, + source: cloneSourceInfo(info), sourceIndex: sourceIndex, payload: processing.payload, declarations: processing.imports, sequenceDeclarations: cloneSequenceDeclarations(processing.sequences), processing: &processing, @@ -477,7 +475,7 @@ func (s *importTraversal) visitPath(candidate selectedImportCandidate, spec impo s.active[admitted.identity] = len(s.stack) s.tier.visited[admitted.identity] = route s.stack = append(s.stack, route) - diagnostics = s.walkDeclarations(s.access.dir(admitted.canonical), admitted.declarations, depth, destination) + diagnostics = s.walkDeclarations(s.access.dir(admitted.canonical), admitted.declarations, depth, destination, admitted.sourceIndex) delete(s.active, admitted.identity) s.stack = s.stack[:len(s.stack)-1] return diagnostics diff --git a/internal/engine/import_evidence_test.go b/internal/engine/import_evidence_test.go index fe002af..fed2f2a 100644 --- a/internal/engine/import_evidence_test.go +++ b/internal/engine/import_evidence_test.go @@ -2,11 +2,54 @@ package engine import ( "errors" + "fmt" "path/filepath" "reflect" "testing" ) +func TestBroadEmptyImportSelectionsKeepDeclaringSourceOwnership(t *testing.T) { + t.Parallel() + + directory := t.TempDir() + mainPath := writeImportFile(t, directory, "main.yml", `configkit: + imports: + - glob: fragments/*.yml + - path: [] +`) + const fragments = 32 + for index := 0; index < fragments; index++ { + writeImportFile(t, directory, fmt.Sprintf("fragments/fragment-%02d.yml", index), `configkit: + imports: + - path: missing.yml + - path: [] +`) + } + loaded, err := testLoad[map[string]any](File(mainPath), Options[map[string]any]{Imports: true}) + if err != nil { + t.Fatal(err) + } + sources := loaded.Sources() + if len(sources) != fragments+1 { + t.Fatalf("source count = %d, want %d", len(sources), fragments+1) + } + for index, source := range sources { + wantPaths := []string{"/configkit/imports/0", "/configkit/imports/1"} + if index == 0 { + wantPaths = wantPaths[1:] + } + if len(source.importSelections) != len(wantPaths) { + t.Fatalf("source %s selections = %#v", source.ID, source.importSelections) + } + for selectionIndex, selection := range source.importSelections { + if selection.path.String() != wantPaths[selectionIndex] || selection.location.SourceID != source.ID || + selection.selector != importSelectorPath || selection.outcome != importSelectionNone { + t.Errorf("source %s selection %d = %#v", source.ID, selectionIndex, selection) + } + } + } +} + func TestOptionalEmptyImportSelectionsRemainSourceLocalAndDetached(t *testing.T) { t.Parallel() diff --git a/internal/engine/import_scope.go b/internal/engine/import_scope.go index db27a5b..009e843 100644 --- a/internal/engine/import_scope.go +++ b/internal/engine/import_scope.go @@ -184,7 +184,7 @@ func (s *importTraversal) visitItemSource(admitted admittedImport, destination, identity: admitted.nodeKey, role: importRoleItem, scopeOwner: admitted.nodeKey, processing: admitted.processing, sequenceDeclarations: cloneSequenceDeclarations(admitted.sequenceDeclarations), }) - diagnostics := s.walkDeclarations(s.access.dir(admitted.canonical), admitted.declarations, depth, Path{}) + diagnostics := s.walkDeclarations(s.access.dir(admitted.canonical), admitted.declarations, depth, Path{}, admitted.sourceIndex) itemScope := s.scope s.scope, s.itemScope, s.itemPrefix, s.itemFallback = parentScope, parentItemScope, parentPrefix, parentFallback if len(diagnostics) != 0 {