From 731b66cff5ac001d69152fac44144f3665fb2287 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Tue, 11 Aug 2026 22:32:33 +0000 Subject: [PATCH] Avoid decoding unrelated profiles --- docs/internal/sources.md | 14 +-- internal/profile/filesystem_repository.go | 8 +- internal/profile/repository_benchmark_test.go | 58 ++++++++++++ internal/profile/repository_test.go | 91 +++++++++++++++++++ 4 files changed, 161 insertions(+), 10 deletions(-) create mode 100644 internal/profile/repository_benchmark_test.go diff --git a/docs/internal/sources.md b/docs/internal/sources.md index 9660e69..60812f7 100644 --- a/docs/internal/sources.md +++ b/docs/internal/sources.md @@ -42,12 +42,14 @@ duplicate detection, and source containment: `internal/profile` loads and validates execution profiles from an operating-system filesystem or an `fs.FS`. A file contains exactly one YAML document and its trimmed YAML `id` is its only selection identity; filenames do -not confer authority. Strict selected decoding recognizes the optional -`backend` field, trims its value, and requires a model plus at least one -non-blank backend or endpoint. File-backed `extra_params` values are validated -and defensively copied through the shared bounded JSON-value owner before a -profile is published. OpenAI-compatible reserved-field policy remains with the -model-client and backend-registry owners. +not confer authority. Each point lookup reads discovered files once for their +metadata and reuses the selected file's bytes for strict decoding; unrelated +profiles are not fully decoded. Strict selected decoding recognizes the +optional `backend` field, trims its value, and requires a model plus at least +one non-blank backend or endpoint. File-backed `extra_params` values are +validated and defensively copied through the shared bounded JSON-value owner +before a profile is published. OpenAI-compatible reserved-field policy remains +with the model-client and backend-registry owners. The overlay repository consults the next repository only when the higher-precedence repository reports that a profile is absent. A reliably diff --git a/internal/profile/filesystem_repository.go b/internal/profile/filesystem_repository.go index 6187f09..bee533a 100644 --- a/internal/profile/filesystem_repository.go +++ b/internal/profile/filesystem_repository.go @@ -114,13 +114,13 @@ func loadProfile(ctx context.Context, fsys fs.FS, root string, id string) (*doma } continue } + if !idMatch { + continue + } prof, err := decodeProfile(data) if err != nil { - if idMatch { - return nil, fmt.Errorf("%w: %s: %v", ErrInvalidYAML, relPath, err) - } - continue + return nil, fmt.Errorf("%w: %s: %v", ErrInvalidYAML, relPath, err) } prof.ID = strings.TrimSpace(prof.ID) diff --git a/internal/profile/repository_benchmark_test.go b/internal/profile/repository_benchmark_test.go new file mode 100644 index 0000000..41cc6fd --- /dev/null +++ b/internal/profile/repository_benchmark_test.go @@ -0,0 +1,58 @@ +package profile + +import ( + "context" + "fmt" + "testing" + "testing/fstest" +) + +func BenchmarkProfileRepositoryLookup(b *testing.B) { + for _, size := range []int{10, 1000} { + b.Run(fmt.Sprintf("catalog-%d", size), func(b *testing.B) { + files := fstest.MapFS{ + "target.yaml": profileMapFile(` +id: target +endpoint: http://localhost:8000/v1 +model: target-model +extra_params: + selected: true +`), + } + metadataNames := []string{"target.yaml"} + for i := 1; i < size; i++ { + name := fmt.Sprintf("profile-%04d.yaml", i) + files[name] = profileMapFile(fmt.Sprintf(` +id: profile-%04d +endpoint: http://localhost:8000/v1 +model: unrelated-model +temperature: 0.5 +max_tokens: 500 +extra_params: + provider: + order: + - first + - second +`, i)) + metadataNames = append(metadataNames, name) + } + + fsys := &recordingProfileFS{FS: files} + repo := NewFSRepository(fsys, ".") + b.ReportAllocs() + b.ResetTimer() + for i := 0; i < b.N; i++ { + if _, err := repo.GetProfile(context.Background(), "target"); err != nil { + b.Fatal(err) + } + } + b.StopTimer() + + for _, name := range metadataNames { + if got := fsys.openCount(name); got != b.N { + b.Fatalf("metadata %q opens = %d, want %d", name, got, b.N) + } + } + }) + } +} diff --git a/internal/profile/repository_test.go b/internal/profile/repository_test.go index f907761..8bfb9a9 100644 --- a/internal/profile/repository_test.go +++ b/internal/profile/repository_test.go @@ -4,9 +4,11 @@ import ( "context" "errors" "fmt" + "io/fs" "os" "path/filepath" "strings" + "sync" "testing" "testing/fstest" @@ -720,6 +722,70 @@ unknown: true } } +func TestProfileRepositoryReadsSourcesFreshOnEveryLookup(t *testing.T) { + newSource := func() (*recordingProfileFS, Repository) { + fsys := &recordingProfileFS{FS: fstest.MapFS{ + "target.yaml": profileMapFile(` +id: target +endpoint: http://localhost:8000/v1 +model: target-model +`), + "unrelated.yaml": profileMapFile(` +id: unrelated +endpoint: http://localhost:8000/v1 +model: unrelated-model +`), + }} + return fsys, NewFSRepository(fsys, ".") + } + + t.Run("selected source", func(t *testing.T) { + fsys, repo := newSource() + for lookup := 1; lookup <= 2; lookup++ { + got, err := repo.GetProfile(context.Background(), "target") + if err != nil { + t.Fatalf("lookup %d: %v", lookup, err) + } + if got.Model != "target-model" { + t.Fatalf("lookup %d model = %q", lookup, got.Model) + } + for _, name := range []string{"target.yaml", "unrelated.yaml"} { + if count := fsys.openCount(name); count != lookup { + t.Fatalf("%s opens after lookup %d = %d, want %d", name, lookup, count, lookup) + } + } + } + }) + + t.Run("overlay fallthrough", func(t *testing.T) { + primaryFS := &recordingProfileFS{FS: fstest.MapFS{ + "unrelated.yaml": profileMapFile(` +id: unrelated +endpoint: http://localhost:8000/v1 +model: unrelated-model +`), + }} + fallbackFS, fallback := newSource() + repo := NewOverlayRepository(NewFSRepository(primaryFS, "."), fallback) + + for lookup := 1; lookup <= 2; lookup++ { + got, err := repo.GetProfile(context.Background(), "target") + if err != nil { + t.Fatalf("lookup %d: %v", lookup, err) + } + if got.Model != "target-model" { + t.Fatalf("lookup %d model = %q", lookup, got.Model) + } + if count := primaryFS.openCount("unrelated.yaml"); count != lookup { + t.Fatalf("primary opens after lookup %d = %d, want %d", lookup, count, lookup) + } + if count := fallbackFS.openCount("target.yaml"); count != lookup { + t.Fatalf("fallback opens after lookup %d = %d, want %d", lookup, count, lookup) + } + } + }) +} + func TestProfileRepositoriesRejectInvalidExecutionSettings(t *testing.T) { ctx := context.Background() @@ -849,6 +915,31 @@ type profileRepositorySource struct { newRepository func(t *testing.T, files map[string]string) Repository } +type recordingProfileFS struct { + fs.FS + mu sync.Mutex + opened []string +} + +func (f *recordingProfileFS) Open(name string) (fs.File, error) { + f.mu.Lock() + f.opened = append(f.opened, name) + f.mu.Unlock() + return f.FS.Open(name) +} + +func (f *recordingProfileFS) openCount(name string) int { + f.mu.Lock() + defer f.mu.Unlock() + count := 0 + for _, opened := range f.opened { + if opened == name { + count++ + } + } + return count +} + func profileRepositorySources() []profileRepositorySource { return []profileRepositorySource{ {