diff --git a/docs/internal/sources.md b/docs/internal/sources.md index cac79ee..cdc4a69 100644 --- a/docs/internal/sources.md +++ b/docs/internal/sources.md @@ -57,6 +57,14 @@ and permits inherited target fields only when a base is named. File-backed bounded JSON-value owner before a profile is published. OpenAI-compatible reserved-field policy remains with the model-client and backend-registry owners. +`LoadFSRepository` is the eager immutable loading boundary for internal +catalog consumers. It discovers and strictly validates every raw profile once, +preserves safe source metadata including explicitly present YAML fields, and +publishes independently copied values from memory. It does not resolve profile +inheritance. Configured consumer sources continue to use the lazy point lookup +repositories described above; engine assembly still uses the embedded built-in +catalog at this point. + The overlay repository consults the next repository only when the higher-precedence repository reports that a profile is absent. A reliably selected malformed profile stops fallback, while an unrelated malformed file diff --git a/docs/roadmap/implementation.md b/docs/roadmap/implementation.md index fd50071..1c616fb 100644 --- a/docs/roadmap/implementation.md +++ b/docs/roadmap/implementation.md @@ -305,7 +305,7 @@ Inspect the copied profile against Promptkit's Stage 1 fixture before release. ## Stage 4: Add Eager Immutable Profile Loading To Promptkit -**Status:** Pending +**Status:** Complete ### Repository diff --git a/internal/profile/eager_repository.go b/internal/profile/eager_repository.go new file mode 100644 index 0000000..76d7f1f --- /dev/null +++ b/internal/profile/eager_repository.go @@ -0,0 +1,95 @@ +package profile + +import ( + "context" + "fmt" + "io/fs" + "sort" + "strings" + + "gitea.maximumdirect.net/eric/promptkit/internal/domain" + "gitea.maximumdirect.net/eric/promptkit/internal/filecatalog" + "gitea.maximumdirect.net/eric/promptkit/internal/jsonvalue" +) + +// LoadedProfileMetadata identifies one profile accepted by LoadFSRepository. +type LoadedProfileMetadata struct { + ID string + Path string + ExplicitFields []string +} + +// LoadFSRepository eagerly validates every profile under root and returns an +// immutable raw repository and independently owned source metadata. +func LoadFSRepository(ctx context.Context, fsys fs.FS, root string) (Repository, []LoadedProfileMetadata, error) { + if fsys == nil { + return nil, nil, fmt.Errorf("failed to read profile directory: filesystem is nil") + } + paths, err := filecatalog.FindFSYAMLFiles(ctx, fsys, root) + if err != nil { + return nil, nil, fmt.Errorf("failed to read profile directory: %w", err) + } + + repository := &loadedRepository{profiles: make(map[string]domain.ExecutionProfile, len(paths))} + metadata := make([]LoadedProfileMetadata, 0, len(paths)) + for _, path := range paths { + if err := ctx.Err(); err != nil { + return nil, nil, err + } + data, err := fs.ReadFile(fsys, path) + if err != nil { + return nil, nil, fmt.Errorf("failed to read profile file %s: %w", filecatalog.DisplayPath(root, path), err) + } + fileMetadata, err := readProfileFileMetadata(data) + if err != nil { + return nil, nil, fmt.Errorf("%w: %s: %v", ErrInvalidYAML, filecatalog.DisplayPath(root, path), err) + } + if fileMetadata.hasRawAPIKey { + return nil, nil, fmt.Errorf("%w: %s", ErrRawAPIKeyNotAllowed, filecatalog.DisplayPath(root, path)) + } + definition, err := decodeProfile(data) + if err != nil { + return nil, nil, fmt.Errorf("%w: %s: %v", ErrInvalidYAML, filecatalog.DisplayPath(root, path), err) + } + definition.ExtraParams, err = jsonvalue.CopyMap(definition.ExtraParams) + if err != nil { + return nil, nil, fmt.Errorf("%w: %s: %v", ErrInvalidProfile, filecatalog.DisplayPath(root, path), err) + } + if err := NormalizeAndValidateDefinition(definition); err != nil { + return nil, nil, fmt.Errorf("%w: %s: %v", ErrInvalidProfile, filecatalog.DisplayPath(root, path), err) + } + if _, exists := repository.profiles[definition.ID]; exists { + return nil, nil, fmt.Errorf("%w: duplicate execution profile id %q", ErrInvalidProfile, definition.ID) + } + repository.profiles[definition.ID] = *definition + fields := append([]string(nil), fileMetadata.explicitFields...) + sort.Strings(fields) + metadata = append(metadata, LoadedProfileMetadata{ + ID: definition.ID, + Path: filecatalog.DisplayPath(root, path), + ExplicitFields: fields, + }) + } + sort.Slice(metadata, func(left, right int) bool { return metadata[left].ID < metadata[right].ID }) + return repository, metadata, nil +} + +type loadedRepository struct { + profiles map[string]domain.ExecutionProfile +} + +func (r *loadedRepository) GetProfile(ctx context.Context, id string) (*domain.ExecutionProfile, error) { + if err := ctx.Err(); err != nil { + return nil, err + } + definition, found := r.profiles[strings.TrimSpace(id)] + if !found { + return nil, ErrProfileNotFound + } + extraParams, err := jsonvalue.CopyMap(definition.ExtraParams) + if err != nil { + return nil, fmt.Errorf("copy loaded profile %q: %w", definition.ID, err) + } + definition.ExtraParams = extraParams + return &definition, nil +} diff --git a/internal/profile/eager_repository_test.go b/internal/profile/eager_repository_test.go new file mode 100644 index 0000000..0b5ba60 --- /dev/null +++ b/internal/profile/eager_repository_test.go @@ -0,0 +1,110 @@ +package profile + +import ( + "context" + "errors" + "io/fs" + "reflect" + "testing" + "testing/fstest" +) + +func TestLoadFSRepositoryLoadsSortedIndependentProfiles(t *testing.T) { + fsys := fstest.MapFS{ + "profiles/z.yml": {Data: []byte("id: z\nbackend: local\nmodel: z-model\nextra_params:\n nested:\n value: one\n")}, + "profiles/a.yml": {Data: []byte("id: a\nbackend: local\nmodel: a-model\nendpoint: ''\n")}, + } + + repository, metadata, err := LoadFSRepository(context.Background(), fsys, "profiles") + if err != nil { + t.Fatalf("load repository: %v", err) + } + if len(metadata) != 2 || metadata[0].ID != "a" || metadata[1].ID != "z" || metadata[0].Path != "a.yml" { + t.Fatalf("unexpected metadata: %#v", metadata) + } + if len(metadata[0].ExplicitFields) != 4 || metadata[0].ExplicitFields[0] != "backend" || metadata[0].ExplicitFields[1] != "endpoint" { + t.Fatalf("expected explicitly empty endpoint metadata, got %#v", metadata[0].ExplicitFields) + } + metadata[1].ExplicitFields[0] = "changed" + first, err := repository.GetProfile(context.Background(), "z") + if err != nil { + t.Fatalf("load profile: %v", err) + } + first.ExtraParams["nested"].(map[string]any)["value"] = "changed" + second, err := repository.GetProfile(context.Background(), "z") + if err != nil { + t.Fatalf("reload profile: %v", err) + } + if second.ExtraParams["nested"].(map[string]any)["value"] != "one" { + t.Fatalf("profile value was not defensively copied: %#v", second) + } +} + +func TestLoadFSRepositoryRejectsInvalidProfiles(t *testing.T) { + tests := map[string]fstest.MapFS{ + "duplicate IDs": { + "profiles/one.yml": {Data: []byte("id: duplicate\nbackend: local\nmodel: one\n")}, + "profiles/two.yml": {Data: []byte("id: duplicate\nbackend: local\nmodel: two\n")}, + }, + "raw API key": { + "profiles/one.yml": {Data: []byte("id: one\nbackend: local\nmodel: one\napi_key: forbidden\n")}, + }, + "multiple documents": { + "profiles/one.yml": {Data: []byte("id: one\nbackend: local\nmodel: one\n---\nid: two\n")}, + }, + } + for name, fsys := range tests { + t.Run(name, func(t *testing.T) { + _, _, err := LoadFSRepository(context.Background(), fsys, "profiles") + if err == nil { + t.Fatal("expected load failure") + } + }) + } +} + +func TestLoadedRepositoryHonorsCancellationAndMissingProfiles(t *testing.T) { + repository, _, err := LoadFSRepository(context.Background(), fstest.MapFS{ + "profiles/one.yml": {Data: []byte("id: one\nbackend: local\nmodel: one\n")}, + }, "profiles") + if err != nil { + t.Fatalf("load repository: %v", err) + } + ctx, cancel := context.WithCancel(context.Background()) + cancel() + if _, err := repository.GetProfile(ctx, "one"); !errors.Is(err, context.Canceled) { + t.Fatalf("expected cancellation, got %v", err) + } + if _, err := repository.GetProfile(context.Background(), "missing"); !errors.Is(err, ErrProfileNotFound) { + t.Fatalf("expected missing profile, got %v", err) + } +} + +func TestLoadFSRepositoryMatchesPointLookupForRawProfiles(t *testing.T) { + fsys := fstest.MapFS{ + "profiles/base.yml": {Data: []byte("id: base\nbackend: local\nmodel: base-model\n")}, + "profiles/derived.yml": {Data: []byte("id: derived\nbase_profile: base\nreasoning_effort: high\n")}, + } + eager, _, err := LoadFSRepository(context.Background(), fsys, "profiles") + if err != nil { + t.Fatalf("load eager repository: %v", err) + } + pointLookup := NewFSRepository(fsys, "profiles") + for _, id := range []string{"base", "derived"} { + t.Run(id, func(t *testing.T) { + got, err := eager.GetProfile(context.Background(), id) + if err != nil { + t.Fatalf("load eager profile: %v", err) + } + want, err := pointLookup.GetProfile(context.Background(), id) + if err != nil { + t.Fatalf("load point-in-time profile: %v", err) + } + if !reflect.DeepEqual(got, want) { + t.Fatalf("raw profiles differ: got %#v, want %#v", got, want) + } + }) + } +} + +var _ fs.FS = fstest.MapFS{} diff --git a/internal/profile/filesystem_repository.go b/internal/profile/filesystem_repository.go index 83b1cd3..a93d79b 100644 --- a/internal/profile/filesystem_repository.go +++ b/internal/profile/filesystem_repository.go @@ -164,8 +164,9 @@ type profileMatch struct { } type profileFileMetadata struct { - ids []string - hasRawAPIKey bool + ids []string + hasRawAPIKey bool + explicitFields []string } func readProfileFileMetadata(data []byte) (profileFileMetadata, error) { @@ -206,6 +207,7 @@ func profileMetadataFromNode(node *yaml.Node) profileFileMetadata { for i := 0; i+1 < len(mapping.Content); i += 2 { key := mapping.Content[i] value := mapping.Content[i+1] + metadata.explicitFields = append(metadata.explicitFields, key.Value) switch key.Value { case "id": metadata.ids = append(metadata.ids, strings.TrimSpace(value.Value)) @@ -228,6 +230,7 @@ func (m profileFileMetadata) matchesID(id string) bool { func (m *profileFileMetadata) merge(other profileFileMetadata) { m.ids = append(m.ids, other.ids...) m.hasRawAPIKey = m.hasRawAPIKey || other.hasRawAPIKey + m.explicitFields = append(m.explicitFields, other.explicitFields...) } func decodeProfile(data []byte) (*domain.ExecutionProfile, error) {