diff --git a/docs/internal/sources.md b/docs/internal/sources.md index ddb0ba0..74afaeb 100644 --- a/docs/internal/sources.md +++ b/docs/internal/sources.md @@ -82,6 +82,9 @@ declaration; absent and null fields remain distinguishable from explicit strings, including empty strings. It closes content-file handles before returning and retains no source capability for public inspection. +Prompt materialization shares a lazy metadata snapshot across root and collection +resources during construction, avoiding a full metadata copy per collection. + Role and cache-control normalization is applied to execution-owned message copies. ConfigKit v0.8.0 retains effective input message metadata and its resource origins; recording a sequence-wide normalization in the retained diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md index a5c53aa..d06a1e8 100644 --- a/docs/roadmap/remediation.md +++ b/docs/roadmap/remediation.md @@ -1,6 +1,6 @@ # Configuration Collections Implementation Review Findings -Status: three findings open. Review completed 2026-10-17. +Status: Finding 1 completed; Findings 2–3 open. Review completed 2026-10-17. Reviewed implementation after `89af2a1` through `0221cd4` against the accepted [feature roadmap](configuration-collections.md), [implementation plan](implementation.md), @@ -35,7 +35,7 @@ addresses a retained public guarantee that no longer describes scoped views. ## Finding 1: Share Origin Metadata Across Collection Materialization -Priority: P2. Status: open. +Priority: P2. Status: completed 2026-10-17. Locations: [load.go](../../internal/configuration/load.go), lines 257–262, and [materializePrompts](../../internal/configuration/prompts.go), lines 84–95. @@ -78,6 +78,15 @@ one file-backed collection, and use a small scaling benchmark to verify the repeated full-snapshot cost is removed. Do not make exact allocations, elapsed times, or private helper call order a new contract. +Completion: prompt materialization now traverses root and collection resources +within one construction-scoped pass sharing the lazy origin metadata snapshot. +Extended the imported/inherited-content test across distinct collection resource +contexts and added a scaling benchmark without allocation or timing assertions. +At 64 collections the same benchmark dropped from 104,574,200 to 21,239,712 +allocated bytes per construction in single-iteration local measurements. +Validation passed: focused origin tests and benchmark, race-enabled configuration +and public API tests, configuration vet, and `git diff --check`. + ## Finding 2: Detach The Optional Label In Internal Prepared Snapshots Priority: P3. Status: open. diff --git a/internal/configuration/benchmark_test.go b/internal/configuration/benchmark_test.go index 48238f6..1810c07 100644 --- a/internal/configuration/benchmark_test.go +++ b/internal/configuration/benchmark_test.go @@ -126,6 +126,40 @@ func layeredDefinitions(count int) configurationShape { } } +func BenchmarkCollectionFileBackedConstruction(b *testing.B) { + for _, count := range []int{4, 16, 64} { + b.Run(fmt.Sprintf("collections_%d", count), func(b *testing.B) { + collections := make(map[string]any, count) + for index := range count { + collections[fmt.Sprintf("collection-%03d", index)] = map[string]any{ + "prompts": map[string]any{ + "example": map[string]any{ + "messages": []any{map[string]any{"role": "user", "content_file": "body.md"}}, + "output": map[string]any{"format": "text", "validation_mode": "none"}, + }, + }, + } + } + namespace := configkit.NewFilesystem("collection benchmark", fstest.MapFS{ + "body.md": {Data: []byte("Hello")}, + }) + main := configkit.Input{ + Source: configkit.Mapping("collections", map[string]any{"collections": collections}), + Filesystem: &configkit.FilesystemContext{FS: &namespace, Root: ".", BaseDir: "."}, + } + b.ReportAllocs() + for b.Loop() { + result, err := load(main, nil) + if err != nil { + b.Fatal(err) + } + constructionResult = result + } + b.ReportMetric(float64(count), "collections/op") + }) + } +} + func BenchmarkConfigurationConstruction(b *testing.B) { for _, scenario := range []struct { name string diff --git a/internal/configuration/load.go b/internal/configuration/load.go index 1ed5c46..97b646f 100644 --- a/internal/configuration/load.go +++ b/internal/configuration/load.go @@ -255,10 +255,8 @@ func load(main configkit.Input, underlays []configkit.Underlay) (*loaded, error) return nil, fmt.Errorf("load configuration: %w", err) } state.result = result - for _, resources := range resourceSets(&result.Config) { - if err := state.materializePrompts(resources.prompts, resources.bases.prompts, resources.namespace); err != nil { - return nil, fmt.Errorf("load configuration: %w", err) - } + if err := state.materializePrompts(); err != nil { + return nil, fmt.Errorf("load configuration: %w", err) } if err := state.buildExecutionProfiles(); err != nil { return nil, fmt.Errorf("load configuration: %w", err) diff --git a/internal/configuration/prompts.go b/internal/configuration/prompts.go index 5162b94..ff70afe 100644 --- a/internal/configuration/prompts.go +++ b/internal/configuration/prompts.go @@ -81,10 +81,8 @@ func resolvePrompts(prompts map[string]promptConfig, basePath configkit.Path, re } } -func (state *loaded) materializePrompts(prompts map[string]promptConfig, basePath configkit.Path, namespace string) error { - if state.prompts == nil { - state.prompts = make(map[string]*promptEntry) - } +func (state *loaded) materializePrompts() error { + state.prompts = make(map[string]*promptEntry) var metadata *configkit.Metadata lookupOrigin := func(path configkit.Path) (configkit.FieldInfo, bool) { if metadata == nil { @@ -93,17 +91,20 @@ func (state *loaded) materializePrompts(prompts map[string]promptConfig, basePat } return metadata.Lookup(path) } - for _, id := range sortedKeys(prompts) { - promptBase := basePath.Key(id) - if strings.TrimSpace(id) == "" { - return fmt.Errorf("%s: prompt ID must not be blank", promptBase) + for _, resources := range resourceSets(&state.result.Config) { + for _, id := range sortedKeys(resources.prompts) { + promptBase := resources.bases.prompts.Key(id) + if strings.TrimSpace(id) == "" { + return fmt.Errorf("%s: prompt ID must not be blank", promptBase) + } + fullID := indexedResourceID(resources.namespace, id) + raw := resources.prompts[id] + entry, err := state.materializePrompt(fullID, raw.Version, raw, promptBase, lookupOrigin) + if err != nil { + return err + } + state.prompts[fullID] = entry } - fullID := indexedResourceID(namespace, id) - entry, err := state.materializePrompt(fullID, prompts[id].Version, prompts[id], promptBase, lookupOrigin) - if err != nil { - return err - } - state.prompts[fullID] = entry } return nil } diff --git a/internal/configuration/prompts_test.go b/internal/configuration/prompts_test.go index 3e14290..e7a73ca 100644 --- a/internal/configuration/prompts_test.go +++ b/internal/configuration/prompts_test.go @@ -16,9 +16,11 @@ import ( func TestLoadCapturesInheritedImportedContent(t *testing.T) { files := fstest.MapFS{ - "app/main.yaml": {Data: []byte("configkit:\n imports:\n - path: ../catalog/base.yaml\ncollections:\n docs:\n prompts:\n child:\n extends: base\n")}, - "catalog/base.yaml": {Data: []byte("collections:\n docs:\n release_version: r1\n prompts:\n base:\n version: \"v1\"\n messages:\n - role: ' USER '\n cache_control:\n type: ' ephemeral '\n ttl: ' 1h '\n content_file: ./body.md\n output:\n format: text\n validation_mode: basic\n")}, - "catalog/body.md": {Data: []byte("Captured {{.name}}")}, + "app/main.yaml": {Data: []byte("configkit:\n imports:\n - path: ../catalog/base.yaml\n - path: ../other/definition.yaml\ncollections:\n docs:\n prompts:\n child:\n extends: base\n")}, + "catalog/base.yaml": {Data: []byte("collections:\n docs:\n release_version: r1\n prompts:\n base:\n version: \"v1\"\n messages:\n - role: ' USER '\n cache_control:\n type: ' ephemeral '\n ttl: ' 1h '\n content_file: ./body.md\n output:\n format: text\n validation_mode: basic\n")}, + "catalog/body.md": {Data: []byte("Captured {{.name}}")}, + "other/definition.yaml": {Data: []byte("collections:\n other:\n prompts:\n child:\n messages:\n - role: user\n content_file: ./body.md\n output:\n format: text\n validation_mode: none\n")}, + "other/body.md": {Data: []byte("Other collection {{.name}}")}, } namespace := configkit.NewFilesystem("embedded", files) state, err := load(configkit.Input{ @@ -56,7 +58,12 @@ func TestLoadCapturesInheritedImportedContent(t *testing.T) { if originPath != "catalog/base.yaml" { t.Fatalf("content origin = %q, want catalog/base.yaml", originPath) } + other, otherInspection, err := state.prompt("other/child") + if err != nil || other.Templates[0].Content != "Other collection {{.name}}" { + t.Fatalf("other collection did not retain its source context: %+v, %v", other, err) + } files["catalog/body.md"].Data = []byte("Changed") + files["other/body.md"].Data = []byte("Changed other collection") again, againInspection, err := state.prompt("docs/child") if err != nil || again.Templates[0].Content != "Captured {{.name}}" || againInspection.PromptHash != inspection.PromptHash { t.Fatalf("captured definition changed after source mutation: %+v, %+v, %v", again, againInspection, err) @@ -66,6 +73,10 @@ func TestLoadCapturesInheritedImportedContent(t *testing.T) { if err != nil || third.Templates[0].Content != "Captured {{.name}}" { t.Fatalf("caller mutation escaped into index: %+v, %v", third, err) } + otherAgain, otherAgainInspection, err := state.prompt("other/child") + if err != nil || otherAgain.Templates[0].Content != "Other collection {{.name}}" || otherAgainInspection.PromptHash != otherInspection.PromptHash { + t.Fatalf("other captured definition changed after source mutation: %+v, %+v, %v", otherAgain, otherAgainInspection, err) + } } func TestLoadUsesExplicitMappingContextAndClosesContent(t *testing.T) {