From 49c0c6d908c0746a0ae49a332691c383ac3ff941 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Wed, 14 Oct 2026 17:21:07 +0000 Subject: [PATCH] Pass effective destinations through import traversal --- docs/roadmap/implementation.md | 11 +- internal/engine/destination.go | 13 +-- internal/engine/destination_import_test.go | 118 +++++++++++++++++++++ internal/engine/destination_test.go | 28 +++++ internal/engine/directives.go | 9 +- internal/engine/imports.go | 39 ++++--- internal/engine/load_input.go | 6 +- 7 files changed, 189 insertions(+), 35 deletions(-) diff --git a/docs/roadmap/implementation.md b/docs/roadmap/implementation.md index cc1bf06..a8e8142 100644 --- a/docs/roadmap/implementation.md +++ b/docs/roadmap/implementation.md @@ -169,7 +169,7 @@ Done: baseline and decision are recorded; implemented behavior is unchanged. ## Stage 2 — Refactor traversal to explicit per-document placement -Status: planned. Scope: private context plumbing; old syntax remains unchanged. +Status: complete. Scope: private context plumbing; old syntax remains unchanged. 1. Add the relative-destination slot to `importDeclaration`, initially zero for all parsed declarations. Add the destination-append helper and bounded @@ -189,10 +189,17 @@ Status: planned. Scope: private context plumbing; old syntax remains unchanged. Public syntax and complete acceptance follow Stage 3. Validate: `go test ./internal/engine -run 'Import|Glob|Destination|Grouped'`, -`go test . -run 'Destination|FacadeLayered'`, and `./scripts/check.sh`. +`go test . -run 'Destination|FacadeLayered|Mapping'`, and `./scripts/check.sh`. Done: baseline loads remain equivalent; all placement consumers use explicit document context and no uniform setter can erase per-import placement. +Evidence (2026-10-14): based on `a8beb3e62014cb960f3c922c79cd98bd04a657e7`, +using `go1.26.8 linux/amd64`. The focused engine command, the root destination, +layered, and Mapping regression tests, and `./scripts/check.sh` all passed. The +private traversal fixture verifies nested accumulation, sibling isolation, and +one shared destination across glob matches; failure fixtures verify entry +placement survives import-context and imported-parse errors. No limitations. + ## Stage 3 — Accept and document the selected YAML syntax Status: planned. Scope: directive parsing and first complete public behavior. diff --git a/internal/engine/destination.go b/internal/engine/destination.go index 8bf791b..1b1451f 100644 --- a/internal/engine/destination.go +++ b/internal/engine/destination.go @@ -29,6 +29,13 @@ func normalizeDestination(path Path) (Path, error) { return Path{segments: segments}, nil } +// appendDestination combines validated mapping-key paths with owned segments. +func appendDestination(parent, relative Path) Path { + segments := clonePathSegments(parent.segments, len(relative.segments)) + segments = append(segments, relative.segments...) + return Path{segments: segments} +} + func invalidDestinationDiagnostic(destination Path) Diagnostic { return Diagnostic{ Code: CodeOptionsInvalid, Severity: SeverityError, @@ -37,12 +44,6 @@ func invalidDestinationDiagnostic(destination Path) Diagnostic { } } -func setSourceDestinations(sources []SourceInfo, destination Path) { - for index := range sources { - sources[index].Destination = destination.clone() - } -} - // placeDocument checks all per-document bounds before allocating destination // wrappers. Aggregate accounting remains the caller's responsibility. func placeDocument(body *document.Node, destination Path, sourceRoot document.Location, maxDepth, maxNodes int) (Path, *document.Node, error) { diff --git a/internal/engine/destination_import_test.go b/internal/engine/destination_import_test.go index ab8ab99..1a49b30 100644 --- a/internal/engine/destination_import_test.go +++ b/internal/engine/destination_import_test.go @@ -71,6 +71,124 @@ first: one } } +func TestImportTraversalUsesPerDeclarationDestinations(t *testing.T) { + t.Parallel() + + directory := t.TempDir() + mainPath := writeImportFile(t, directory, "main.yml", `configkit: + imports: + - path: parent.yml + - path: sibling.yml + - glob: fragments/*.yml +`) + parentPath := writeImportFile(t, directory, "parent.yml", `configkit: + imports: + - path: nested/child.yml +parent: true +`) + childPath := writeImportFile(t, directory, "nested/child.yml", "child: true\n") + siblingPath := writeImportFile(t, directory, "sibling.yml", "sibling: true\n") + alphaPath := writeImportFile(t, directory, "fragments/a.yml", "alpha: true\n") + betaPath := writeImportFile(t, directory, "fragments/b.yml", "beta: true\n") + + main := parseImportMain(t, mainPath) + _, declarations, diagnostics := splitImportDirectives(main.document) + if len(diagnostics) != 0 { + t.Fatalf("main directive diagnostics = %#v", diagnostics) + } + declarations[0].destination = Keys("parent") + declarations[1].destination = Keys("sibling") + declarations[2].destination = Keys("collection") + + access, err := openHostAccess(directory) + if err != nil { + t.Fatal(err) + } + defer func() { _ = access.close() }() + state := importTraversal{ + access: access, accounting: &importAccounting{}, limits: defaultImportLimits(), + active: make(map[resourceIdentity]int), visited: make(map[resourceIdentity]importRoute), + } + if diagnostics := state.walkDeclarations(directory, declarations, 0, Keys("entry")); len(diagnostics) != 0 { + t.Fatalf("walk diagnostics = %#v", diagnostics) + } + + wantSources := []struct { + path string + destination string + }{ + {path: parentPath, destination: "/entry/parent"}, + {path: childPath, destination: "/entry/parent"}, + {path: siblingPath, destination: "/entry/sibling"}, + {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)) + } + for index, want := range wantSources { + got := state.sources[index] + 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) + } + } + + composed, diagnostics := composeDocuments(state.documents...) + if len(diagnostics) != 0 { + t.Fatalf("composition diagnostics = %#v", diagnostics) + } + for _, path := range []Path{ + Keys("entry", "parent", "parent"), Keys("entry", "parent", "child"), + Keys("entry", "sibling", "sibling"), Keys("entry", "collection", "alpha"), + Keys("entry", "collection", "beta"), + } { + if _, present := documentAtPath(composed.root, path); !present { + t.Errorf("no composed value at %q", path.String()) + } + } +} + +func TestEntryDestinationSurvivesImportSetupAndParseFailures(t *testing.T) { + t.Parallel() + + t.Run("import context failure", func(t *testing.T) { + directory := t.TempDir() + mainPath := writeImportFile(t, directory, "main.yml", "configkit: {imports: [{path: child.yml}]}\n") + input, diagnostics := assembleBaseInputTracked( + File(mainPath), + Options[map[string]any]{Imports: true, Filesystem: &FilesystemContext{Root: t.TempDir()}}, + Keys("mounted"), &importAccounting{}, nil, nil, + ) + if len(diagnostics) != 1 || diagnostics[0].Code != CodeImportOutsideRoot { + t.Fatalf("context diagnostics = %#v", diagnostics) + } + if len(input.sources) != 1 || input.sources[0].Destination.String() != "/mounted" { + t.Fatalf("entry source fallback = %#v", input.sources) + } + }) + + t.Run("imported parse failure", func(t *testing.T) { + directory := t.TempDir() + mainPath := writeImportFile(t, directory, "main.yml", "configkit: {imports: [{path: broken.yml}]}\n") + writeImportFile(t, directory, "broken.yml", "broken: [\n") + input, diagnostics := assembleBaseInputTracked( + File(mainPath), Options[map[string]any]{Imports: true}, + Keys("mounted"), &importAccounting{}, nil, nil, + ) + if len(diagnostics) != 1 || diagnostics[0].Code != CodeYAMLInvalid { + t.Fatalf("parse diagnostics = %#v", diagnostics) + } + if len(input.sources) != 2 { + t.Fatalf("parse-failure sources = %#v", input.sources) + } + for index, source := range input.sources { + if source.Destination.String() != "/mounted" { + t.Errorf("source %d destination = %q, want /mounted", index, source.Destination.String()) + } + } + }) +} + func TestLayeredNativeFilesystemImportsInheritInputDestination(t *testing.T) { t.Parallel() diff --git a/internal/engine/destination_test.go b/internal/engine/destination_test.go index c67e056..1065421 100644 --- a/internal/engine/destination_test.go +++ b/internal/engine/destination_test.go @@ -39,6 +39,34 @@ func TestNormalizeDestinationUsesLiteralMappingKeys(t *testing.T) { } } +func TestAppendDestinationCopiesLiteralSegments(t *testing.T) { + t.Parallel() + + parent := Keys("collection", "entry") + relative := Keys("a/b", "0", "~") + combined := appendDestination(parent, relative) + if got, want := combined.String(), "/collection/entry/a~1b/0/~0"; got != want { + t.Fatalf("combined destination = %q, want %q", got, want) + } + if len(combined.segments) != 5 { + t.Fatalf("combined segments = %#v", combined.segments) + } + for index, segment := range combined.segments { + if segment.kind != pathSegmentKey { + t.Errorf("segment %d kind = %v, want mapping key", index, segment.kind) + } + } + + combined.segments[0].key = "changed parent" + combined.segments[2].key = "changed relative" + if got := parent.String(); got != "/collection/entry" { + t.Fatalf("combined path shares parent segment storage: %q", got) + } + if got := relative.String(); got != "/a~1b/0/~0" { + t.Fatalf("combined path shares relative segment storage: %q", got) + } +} + func TestNormalizeDestinationRejectsIndicesAndReservedKeys(t *testing.T) { t.Parallel() diff --git a/internal/engine/directives.go b/internal/engine/directives.go index 4a9399e..86e8f0e 100644 --- a/internal/engine/directives.go +++ b/internal/engine/directives.go @@ -10,10 +10,11 @@ import ( const directiveKey = "configkit" type importDeclaration struct { - path string - glob string - optional bool - location Location + path string + glob string + destination Path + optional bool + location Location } func splitImportDirectives(root *document.Node) (*document.Node, []importDeclaration, []Diagnostic) { diff --git a/internal/engine/imports.go b/internal/engine/imports.go index e56e309..9833e8c 100644 --- a/internal/engine/imports.go +++ b/internal/engine/imports.go @@ -45,11 +45,10 @@ type importAccounting struct { } type importTraversal struct { - access *filesystemAccess - accounting *importAccounting - limits importLimits - rule *VersionRule - destination Path + access *filesystemAccess + accounting *importAccounting + limits importLimits + rule *VersionRule documents []*document.Node sources []SourceInfo @@ -120,10 +119,9 @@ func loadImportedWithAccessTracked(access *filesystemAccess, basePath, mainPath accounting.nodes += countInputNodes(main.document) state := importTraversal{ access: access, accounting: accounting, limits: limits, rule: rule, - destination: destination.clone(), - documents: make([]*document.Node, 0, 1), - sources: []SourceInfo{mainInfo}, - active: make(map[resourceIdentity]int), visited: visited, + documents: make([]*document.Node, 0, 1), + sources: []SourceInfo{mainInfo}, + active: make(map[resourceIdentity]int), visited: visited, } if state.visited == nil { state.visited = make(map[resourceIdentity]importRoute) @@ -143,8 +141,8 @@ func loadImportedWithAccessTracked(access *filesystemAccess, basePath, mainPath state.stack = append(state.stack, mainRoute) body, declarations, diagnostics := splitImportDirectives(main.document) if len(diagnostics) == 0 { - _, placed, err := placeDocument(body, state.destination, body.Location, maxDocumentDepth, maxDocumentNodes) - placementNodes := destinationWrapperNodeCount(state.destination) + _, placed, err := placeDocument(body, destination, body.Location, maxDocumentDepth, maxDocumentNodes) + placementNodes := destinationWrapperNodeCount(destination) if err != nil { diagnostics = []Diagnostic{sourceDiagnostic(CodeSourceLimitExceeded, main.source.ID, "configuration exceeds the source depth or node limit after destination placement")} } else if accounting.nodes+placementNodes > limits.nodes { @@ -152,7 +150,7 @@ func loadImportedWithAccessTracked(access *filesystemAccess, basePath, mainPath } else { accounting.nodes += placementNodes state.documents = append(state.documents, placed) - diagnostics = state.walkDeclarations(basePath, declarations, 0) + diagnostics = state.walkDeclarations(basePath, declarations, 0, destination) } } @@ -187,8 +185,9 @@ func (s *importTraversal) checkInitialLimits() *Diagnostic { return nil } -func (s *importTraversal) walkDeclarations(basePath string, declarations []importDeclaration, depth int) []Diagnostic { +func (s *importTraversal) walkDeclarations(basePath string, declarations []importDeclaration, depth int, parentDestination Path) []Diagnostic { for _, declaration := range declarations { + childDestination := appendDestination(parentDestination, declaration.destination) if declaration.glob != "" { matches, diagnostics := s.expandImportGlob(basePath, declaration) if len(diagnostics) != 0 { @@ -205,20 +204,20 @@ func (s *importTraversal) walkDeclarations(basePath string, declarations []impor matched := declaration matched.path = match matched.glob = "" - if diagnostics := s.visitPath(basePath, matched, depth+1); len(diagnostics) != 0 { + if diagnostics := s.visitPath(basePath, matched, childDestination, depth+1); len(diagnostics) != 0 { return diagnostics } } continue } - if diagnostics := s.visitPath(basePath, declaration, depth+1); len(diagnostics) != 0 { + if diagnostics := s.visitPath(basePath, declaration, childDestination, depth+1); len(diagnostics) != 0 { return diagnostics } } return nil } -func (s *importTraversal) visitPath(basePath string, declaration importDeclaration, depth int) []Diagnostic { +func (s *importTraversal) visitPath(basePath string, declaration importDeclaration, destination Path, depth int) []Diagnostic { location := declaration.location if depth > s.limits.depth { return []Diagnostic{importDiagnostic(CodeImportLimitExceeded, "configuration exceeds the active import-depth limit", &location)} @@ -272,7 +271,7 @@ func (s *importTraversal) visitPath(basePath string, declaration importDeclarati kind = SourceKindFilesystem label = s.access.label + "/" + canonical } - info := SourceInfo{ID: sourceID, Label: label, Path: canonical, Destination: s.destination.clone(), Layer: LayerBase, Kind: kind, ImportedFrom: &location} + info := SourceInfo{ID: sourceID, Label: label, Path: canonical, Destination: destination.clone(), Layer: LayerBase, Kind: kind, ImportedFrom: &location} if kind == SourceKindFilesystem { info.namespace = s.access.token } @@ -303,11 +302,11 @@ func (s *importTraversal) visitPath(basePath string, declaration importDeclarati if len(diagnostics) != 0 { return diagnostics } - _, placed, err := placeDocument(body, s.destination, body.Location, maxDocumentDepth, maxDocumentNodes) + _, placed, err := placeDocument(body, destination, body.Location, maxDocumentDepth, maxDocumentNodes) if err != nil { return []Diagnostic{importDiagnostic(CodeImportLimitExceeded, "configuration exceeds the source depth or node limit after destination placement", &location)} } - placementNodes := destinationWrapperNodeCount(s.destination) + placementNodes := destinationWrapperNodeCount(destination) if s.accounting.nodes+placementNodes > s.limits.nodes { return []Diagnostic{importDiagnostic(CodeImportLimitExceeded, "configuration exceeds the aggregate node limit", &location)} } @@ -325,7 +324,7 @@ func (s *importTraversal) visitPath(basePath string, declaration importDeclarati s.active[identity] = len(s.stack) s.visited[identity] = route s.stack = append(s.stack, route) - diagnostics = s.walkDeclarations(s.access.dir(canonical), declarations, depth) + diagnostics = s.walkDeclarations(s.access.dir(canonical), declarations, depth, destination) delete(s.active, identity) s.stack = s.stack[:len(s.stack)-1] return diagnostics diff --git a/internal/engine/load_input.go b/internal/engine/load_input.go index f98ddde..3967ffc 100644 --- a/internal/engine/load_input.go +++ b/internal/engine/load_input.go @@ -25,10 +25,12 @@ func assembleBaseInputTracked[T any](source Source, options Options[T], destinat parseLimits := defaultParseLimits() parseLimits.source.remaining = limits.bytes - accounting.bytes parsed, diagnostics := parseSource(source, "source-"+strconv.Itoa(accounting.sources+1), parseLimits) + if parsed.source != nil { + parsed.source.Destination = destination.clone() + } input := assembledInput{parsed: parsed, document: parsed.document} if len(diagnostics) != 0 { input.sources = sourceInfos(parsed.source) - setSourceDestinations(input.sources, destination) // Mapping value issues already identify application content. Other early // failures describe source syntax or access, not a placed value. if source.kind == sourceMapping && source.mapIssue != nil && len(destination.segments) != 0 { @@ -42,14 +44,12 @@ func assembleBaseInputTracked[T any](source Source, options Options[T], destinat } if options.Imports { input.composition, input.sources, diagnostics = loadImportedDocumentsTracked(source, options.Filesystem, parsed, destination, options.Definition.Version, limits, accounting, visited, hostToken) - setSourceDestinations(input.sources, destination) if len(diagnostics) != 0 { return input, diagnostics } input.document = input.composition.root } else { input.sources = sourceInfos(parsed.source) - setSourceDestinations(input.sources, destination) if options.Filesystem != nil && !activeOverlay(options.Overlay) { if diagnostic := validateFilesystemContext(source, options.Filesystem, parsed); diagnostic != nil { return input, []Diagnostic{*diagnostic}