diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md index dacc1ad..e44c43c 100644 --- a/docs/roadmap/remediation.md +++ b/docs/roadmap/remediation.md @@ -6,6 +6,28 @@ contracts in the [roadmap](sources-and-layering.md) and [implementation plan](implementation.md). No implementation fixes are included in this review. +## Resolution status + +All three findings were addressed on 2026-09-29: + +- Composition now fills sequence-descendant origins separately from atomic + conflict claims, so initial underlay attribution reaches every surviving node. +- History reconciliation checks contributing origins and recognizes ancestor + replacement events without duplicating their descendant contributions. +- Native contexts use `fs.ValidPath`; host contexts use `filepath.Localize` + before the existing canonicalization and containment checks. + +`facade_underlay_origins_test.go` covers imported sequence descendants in first +and later tiers, resolver/result/explanation agreement, and later mapping +contributors. `TestResourceContextsPreserveLiteralBackslashes` covers native +and host resource reads and directory listing. The new tests reproduced all +three failures before the fixes. The findings below retain the original review +evidence and acceptance requirements. + +Post-fix validation passed: `./scripts/check.sh` and +`GOPROXY=off go test -race ./...`. Existing sequence replacement, provenance, +history, and resource-containment tests remain passing. + ## 1. Preserve underlay attribution for sequence descendants (P2) **Owners:** `internal/engine/load_request.go:174`, diff --git a/facade_underlay_origins_test.go b/facade_underlay_origins_test.go new file mode 100644 index 0000000..a311fee --- /dev/null +++ b/facade_underlay_origins_test.go @@ -0,0 +1,107 @@ +package configkit_test + +import ( + "reflect" + "testing" + "testing/fstest" + + configkit "gitea.maximumdirect.net/eric/ConfigKit" +) + +func TestUnderlaySequenceDescendantOrigins(t *testing.T) { + for _, later := range []bool{false, true} { + name := "first tier" + if later { + name = "later tier" + } + t.Run(name, func(t *testing.T) { + bundle := configkit.NewFilesystem("bundle", fstest.MapFS{ + "main.yml": &fstest.MapFile{Data: []byte("configkit: {imports: [{path: items.yml}]}\n")}, + "items.yml": &fstest.MapFile{Data: []byte("items: [{name: first, nested: [false]}]\n")}, + }) + tiers := []configkit.Underlay{} + if later { + tiers = append(tiers, configkit.Underlay{Inputs: []configkit.Input{{Source: configkit.Mapping("lower", map[string]any{"other": true})}}}) + } + tiers = append(tiers, configkit.Underlay{Inputs: []configkit.Input{{Source: bundle.File("main.yml"), Imports: true}}}) + paths := []string{"/items", "/items/0", "/items/0/name", "/items/0/nested", "/items/0/nested/0"} + entry := map[string]configkit.Origin{} + result, err := configkit.LoadLayered[map[string]any](configkit.Input{Source: configkit.Mapping("main", map[string]any{"main": true})}, configkit.LayeredOptions[map[string]any]{ + Underlays: tiers, + Definition: configkit.Definition[map[string]any]{Resolve: func(_ *map[string]any, resolution *configkit.Resolution) error { + for _, pointer := range paths { + path, _ := configkit.ParsePath(pointer) + info, ok := resolution.InputInfo(path) + if !ok { + t.Errorf("missing resolver input %s", pointer) + } + entry[pointer] = info.Origin + } + return nil + }}, + }) + if err != nil { + t.Fatal(err) + } + var sourceID string + for _, source := range result.Sources() { + if source.Path == "items.yml" { + sourceID = source.ID + } + } + if sourceID == "" { + t.Fatal("missing imported source") + } + for _, pointer := range paths { + path, _ := configkit.ParsePath(pointer) + info, ok := result.Metadata().Lookup(path) + if !ok || info.Origin.Layer != configkit.LayerUnderlay || len(info.Origin.Locations) != 1 || info.Origin.Locations[0].SourceID != sourceID { + t.Errorf("%s origin = %+v", pointer, info.Origin) + } + if !reflect.DeepEqual(entry[pointer], info.Origin) { + t.Errorf("%s resolver origin differs", pointer) + } + explanation, ok := result.Explain(path) + if !ok || !reflect.DeepEqual(explanation.Origin, info.Origin) { + t.Errorf("%s explanation origin differs", pointer) + } + history := result.Metadata().History(path) + if len(history) == 0 || !reflect.DeepEqual(history[len(history)-1].Origin, info.Origin) { + t.Errorf("%s history = %+v", pointer, history) + } + } + }) + } +} + +func TestUnderlayHistoryRetainsLaterMappingContributors(t *testing.T) { + result, err := configkit.LoadLayered[map[string]any](configkit.Input{Source: configkit.NoSource()}, configkit.LayeredOptions[map[string]any]{Underlays: []configkit.Underlay{ + {Inputs: []configkit.Input{{Source: configkit.Mapping("one", map[string]any{"x": 1})}}}, + {Inputs: []configkit.Input{{Source: configkit.Mapping("two", map[string]any{"x": map[string]any{}})}}}, + {Inputs: []configkit.Input{{Source: configkit.Mapping("three", map[string]any{"x": map[string]any{"y": 2}})}}}, + }}) + if err != nil { + t.Fatal(err) + } + path := configkit.Keys("x") + info, _ := result.Metadata().Lookup(path) + wantOrigin := configkit.Origin{Layer: configkit.LayerUnderlay, Locations: []configkit.Location{{SourceID: "source-2"}, {SourceID: "source-3"}}} + if !reflect.DeepEqual(info.Origin, wantOrigin) { + t.Fatalf("effective origin = %+v, want %+v", info.Origin, wantOrigin) + } + history := result.Metadata().History(path) + if len(history) != 3 || !reflect.DeepEqual(history[len(history)-1].Origin, info.Origin) { + t.Fatalf("history = %+v, effective origin = %+v", history, info.Origin) + } + explanation, ok := result.Explain(path) + if !ok || !reflect.DeepEqual(explanation.Origin, info.Origin) { + t.Fatal("explanation and metadata disagree") + } + if len(explanation.History) != 3 { + t.Fatalf("explanation history = %+v", explanation.History) + } + last := explanation.History[len(explanation.History)-1].Origin + if last.Layer != configkit.LayerUnderlay || len(last.Locations) != 1 || last.Locations[0].SourceID != "source-3" { + t.Fatalf("last explanation origin = %+v", last) + } +} diff --git a/internal/engine/compose.go b/internal/engine/compose.go index 5ef008c..be534b4 100644 --- a/internal/engine/compose.go +++ b/internal/engine/compose.go @@ -92,14 +92,35 @@ func composeDocuments(inputs ...*document.Node) (*composedDocument, []Diagnostic for _, input := range inputs { applications = append(applications, historyApplication{layer: LayerBase, value: input}) } + origins := compositionOrigins(collected.ordered) + completeCompositionOrigins(root, Path{}, origins) return &composedDocument{ root: root, claims: cloneCompositionClaims(collected.ordered), - origins: compositionOrigins(collected.ordered), + origins: origins, applications: applications, }, nil } +// Conflict claims treat sequences atomically, but inspection needs origins for +// every descendant. Preserve composite mapping claims while filling those gaps. +func completeCompositionOrigins(node *document.Node, path Path, origins map[string]Origin) { + pointer := path.String() + if _, present := origins[pointer]; !present { + origins[pointer] = Origin{Layer: LayerBase, Locations: []Location{publicLocation(node.Location)}} + } + switch node.Kind { + case document.Mapping: + for _, entry := range node.Mapping { + completeCompositionOrigins(entry.Value, path.Key(entry.Key), origins) + } + case document.Sequence: + for index, child := range node.Sequence { + completeCompositionOrigins(child, path.Index(index), origins) + } + } +} + func (c *compositionClaims) append(node *document.Node, path Path) { claim := compositionClaim{ path: path.clone(), diff --git a/internal/engine/load_metadata.go b/internal/engine/load_metadata.go index 0e05f79..8219430 100644 --- a/internal/engine/load_metadata.go +++ b/internal/engine/load_metadata.go @@ -125,9 +125,6 @@ func applyOverlayMetadata(metadata *Metadata, records []overlayRecord) { if err != nil { continue } - if history := events[pointer]; len(history) != 0 && history[len(history)-1].Origin.Layer == info.Origin.Layer && history[len(history)-1].Origin.Operation == info.Origin.Operation { - continue - } appendInputOriginEvent(events, baseline, pointer, path, info.Origin) } metadata.history = make(map[string][]Change, len(baseline)+len(events)) @@ -144,12 +141,37 @@ func appendInputOriginEvent(events map[string][]Change, baseline map[string][]Ch return } history := events[pointer] - if len(history) != 0 && originContainedBy(history[len(history)-1].Origin, origin) { + if len(history) != 0 && originRepresentedByChange(path, origin, history[len(history)-1]) { return } appendInputChangeEvent(events, baseline, pointer, Change{Path: path.clone(), Origin: cloneOrigin(origin)}) } +func originRepresentedByChange(path Path, origin Origin, change Change) bool { + if change.Path.String() == path.String() { + return originContainedBy(origin, change.Origin) + } + if !pathContains(change.Path, path) || origin.Layer != change.Origin.Layer || origin.Operation != change.Origin.Operation { + return false + } + // A subtree replacement already records its descendants' contributions. + // Their coordinates differ from the container's, but their sources must be + // covered by the operation; matching only the precedence role is insufficient. + for _, location := range origin.Locations { + found := false + for _, recorded := range change.Origin.Locations { + if location.SourceID == recorded.SourceID { + found = true + break + } + } + if !found { + return false + } + } + return true +} + func appendInputChangeEvent(events map[string][]Change, baseline map[string][]Change, pointer string, value Change) { change := cloneChange(value) history := events[pointer] diff --git a/internal/engine/resource_context.go b/internal/engine/resource_context.go index 39a9a74..22479a3 100644 --- a/internal/engine/resource_context.go +++ b/internal/engine/resource_context.go @@ -36,7 +36,7 @@ type rootedNativeFS struct { } func (r rootedNativeFS) name(name string) (string, error) { - if !fs.ValidPath(name) || strings.Contains(name, "\\") { + if !fs.ValidPath(name) { return "", &fs.PathError{Op: "open", Path: name, Err: fs.ErrInvalid} } joined := path.Join(r.root, name) @@ -75,7 +75,8 @@ type rootedHostFS struct { } func (r rootedHostFS) Open(name string) (fs.File, error) { - if !fs.ValidPath(name) || strings.Contains(name, "\\") { + localName, err := filepath.Localize(name) + if err != nil { return nil, &fs.PathError{Op: "open", Path: name, Err: fs.ErrInvalid} } opener := r.openRoot @@ -86,7 +87,7 @@ func (r rootedHostFS) Open(name string) (fs.File, error) { if err != nil { return nil, &fs.PathError{Op: "open", Path: name, Err: err} } - canonical, err := canonicalExistingPath(filepath.Join(r.root, filepath.FromSlash(name))) + canonical, err := canonicalExistingPath(filepath.Join(r.root, localName)) if err != nil { _ = root.Close() return nil, &fs.PathError{Op: "open", Path: name, Err: err} diff --git a/internal/engine/resource_context_test.go b/internal/engine/resource_context_test.go index b2950fb..9d9a874 100644 --- a/internal/engine/resource_context_test.go +++ b/internal/engine/resource_context_test.go @@ -5,10 +5,46 @@ import ( "io/fs" "os" "path/filepath" + "runtime" "testing" "testing/fstest" ) +func TestResourceContextsPreserveLiteralBackslashes(t *testing.T) { + const directory = `assets\literal` + const name = directory + `/text\file` + check := func(t *testing.T, filesystem fs.FS) { + t.Helper() + data, err := fs.ReadFile(filesystem, name) + if err != nil || string(data) != "content" { + t.Fatalf("read = %q, %v", data, err) + } + entries, err := fs.ReadDir(filesystem, directory) + if err != nil || len(entries) != 1 || entries[0].Name() != `text\file` { + t.Fatalf("entries = %v, %v", entries, err) + } + if _, err := filesystem.Open("../outside"); !errors.Is(err, fs.ErrInvalid) { + t.Fatalf("escape = %v", err) + } + } + t.Run("native", func(t *testing.T) { + check(t, rootedNativeFS{filesystem: fstest.MapFS{"bundle/" + name: &fstest.MapFile{Data: []byte("content")}}, root: "bundle"}) + }) + t.Run("host", func(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("backslash is a host separator on Windows; native case remains covered") + } + root := t.TempDir() + if err := os.Mkdir(filepath.Join(root, directory), 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(root, filepath.FromSlash(name)), []byte("content"), 0o600); err != nil { + t.Fatal(err) + } + check(t, rootedHostFS{root: root}) + }) +} + type trackedHostRoot struct { root *os.Root closed *int