From d45c474c1e718b9bffd624644a6316bfd3d42f40 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Tue, 11 Aug 2026 22:18:50 +0000 Subject: [PATCH] Unify prompt repository source handling --- docs/internal/sources.md | 27 ++- internal/promptdef/filesystem_repository.go | 128 +++-------- internal/promptdef/repository_test.go | 241 +++++++++++++++----- internal/promptdef/source.go | 63 +++++ 4 files changed, 286 insertions(+), 173 deletions(-) create mode 100644 internal/promptdef/source.go diff --git a/docs/internal/sources.md b/docs/internal/sources.md index 9fb52b1..a10ec60 100644 --- a/docs/internal/sources.md +++ b/docs/internal/sources.md @@ -12,18 +12,21 @@ validation modes, built-in catalog, and source precedence. ## Prompt Definitions -`internal/promptdef` discovers YAML deterministically, requires one strictly -decoded document per file, and scans normalized ID and version metadata before -validating the selected definition. It retains the point-in-time metadata scan -needed for duplicate detection while opening file-backed message content only -for one exact selected candidate. Content resolution uses an explicit -source-root abstraction. Operating-system -sources enforce containment against canonical roots and targets so symlinks -cannot escape. Injected `fs.FS` sources enforce containment in their clean -relative path namespace. A single-file source uses the selected prompt file's -containing directory as its root. Every content path must be relative and is -opened from its exact parsed text after a separate blank check; contained -parent components and whitespace-bearing names remain valid. +`internal/promptdef` uses one source-neutral flow for prompt selection and +normalization. That flow scans normalized YAML ID and version metadata, +requires one strictly decoded document per file, classifies errors for the +selected definition, detects duplicates, and normalizes the exact match. +Small operating-system and `fs.FS` adapters own discovery, byte reads, display +paths, content opening, and root containment. Each lookup remains a +point-in-time scan: definitions and catalogs are not cached, and file-backed +message content is opened only for the exact selected candidate. + +Operating-system sources enforce containment against canonical roots and +targets so symlinks cannot escape. Injected `fs.FS` sources enforce containment +in their clean relative path namespace. A single-file source uses the selected +prompt file's containing directory as its root. Every content path must be +relative and is opened from its exact parsed text after a separate blank check; +contained parent components and whitespace-bearing names remain valid. Exact prompt inspection performs one point-in-time lookup through that same repository and validates referenced message content before returning declared diff --git a/internal/promptdef/filesystem_repository.go b/internal/promptdef/filesystem_repository.go index 567f01c..6e3c80e 100644 --- a/internal/promptdef/filesystem_repository.go +++ b/internal/promptdef/filesystem_repository.go @@ -7,11 +7,9 @@ import ( "fmt" "io" "io/fs" - "os" "strings" "gitea.maximumdirect.net/eric/promptkit/internal/domain" - "gitea.maximumdirect.net/eric/promptkit/internal/filecatalog" "gopkg.in/yaml.v3" ) @@ -21,15 +19,8 @@ var ( ErrInvalidPromptDefinition = errors.New("invalid prompt definition configuration") ) -type filesystemRepository struct { - dir string - sourceRoot contentSourceRoot -} - -type fsRepository struct { - fsys fs.FS - root string - sourceRoot contentSourceRoot +type sourceRepository struct { + source promptDefinitionSource } type promptDefinitionFile struct { @@ -70,38 +61,47 @@ type promptOutputContractFile struct { } func NewFilesystemRepository(dir string) Repository { - return &filesystemRepository{ - dir: dir, - sourceRoot: osContentSourceRoot{root: dir}, + return &sourceRepository{ + source: osPromptSource{ + root: dir, + contentRoot: osContentSourceRoot{root: dir}, + }, } } func NewFSRepository(fsys fs.FS, root string) Repository { - return &fsRepository{ - fsys: fsys, - root: root, - sourceRoot: fsContentSourceRoot{fsys: fsys, root: root}, + return &sourceRepository{ + source: fsPromptSource{ + fsys: fsys, + root: root, + contentRoot: fsContentSourceRoot{fsys: fsys, root: root}, + }, } } // NewFileRepository constructs a repository for one operating-system prompt file. func NewFileRepository(fsys fs.FS, file string, sourceDir string) Repository { - return &fsRepository{ - fsys: fsys, - root: file, - sourceRoot: osContentSourceRoot{ - root: sourceDir, - sourcePathsRelative: true, + return &sourceRepository{ + source: fsPromptSource{ + fsys: fsys, + root: file, + contentRoot: osContentSourceRoot{ + root: sourceDir, + sourcePathsRelative: true, + }, }, } } -func (r *filesystemRepository) GetPromptDefinition(ctx context.Context, id string, version string) (*domain.PromptDefinition, error) { +func (r *sourceRepository) GetPromptDefinition(ctx context.Context, id string, version string) (*domain.PromptDefinition, error) { if strings.TrimSpace(id) == "" { return nil, fmt.Errorf("%w: prompt id is required", ErrInvalidPromptDefinition) } + if r == nil || r.source == nil { + return nil, errors.New("failed to read prompt definition directory: source is nil") + } - files, err := filecatalog.FindYAMLFiles(ctx, r.dir) + files, err := r.source.findYAMLFiles(ctx) if err != nil { return nil, fmt.Errorf("failed to read prompt definition directory: %w", err) } @@ -114,8 +114,8 @@ func (r *filesystemRepository) GetPromptDefinition(ctx context.Context, id strin default: } - relPath := filecatalog.RelativePath(r.dir, fullPath) - data, err := os.ReadFile(fullPath) + relPath := r.source.displayPath(fullPath) + data, err := r.source.readDefinition(fullPath) if err != nil { continue } @@ -149,7 +149,7 @@ func (r *filesystemRepository) GetPromptDefinition(ctx context.Context, id strin if len(matches) == 1 { match := matches[0] - def, err := normalizePromptDefinition(match.raw, r.sourceRoot, match.sourcePath) + def, err := normalizePromptDefinition(match.raw, r.source, match.sourcePath) if err != nil { return nil, fmt.Errorf("%w: %s: %v", ErrInvalidPromptDefinition, match.path, err) } @@ -159,82 +159,12 @@ func (r *filesystemRepository) GetPromptDefinition(ctx context.Context, id strin return nil, ErrPromptDefinitionNotFound } -func (r *fsRepository) GetPromptDefinition(ctx context.Context, id string, version string) (*domain.PromptDefinition, error) { - return loadPromptDefinition(ctx, r.fsys, r.root, r.sourceRoot, id, version) -} - type promptDefinitionMatch struct { raw *promptDefinitionFile sourcePath string path string } -func loadPromptDefinition(ctx context.Context, fsys fs.FS, root string, sourceRoot contentSourceRoot, id string, version string) (*domain.PromptDefinition, error) { - if strings.TrimSpace(id) == "" { - return nil, fmt.Errorf("%w: prompt id is required", ErrInvalidPromptDefinition) - } - if fsys == nil { - return nil, fmt.Errorf("failed to read prompt definition directory: filesystem is nil") - } - - files, err := filecatalog.FindFSYAMLFiles(ctx, fsys, root) - if err != nil { - return nil, fmt.Errorf("failed to read prompt definition directory: %w", err) - } - var matches []promptDefinitionMatch - for _, fullPath := range files { - select { - case <-ctx.Done(): - return nil, ctx.Err() - default: - } - - relPath := filecatalog.DisplayPath(root, fullPath) - data, err := fs.ReadFile(fsys, fullPath) - if err != nil { - continue - } - - raw, err := decodePromptDefinition(data) - if err != nil { - if promptDefinitionDataMatches(data, id, version) { - return nil, fmt.Errorf("%w: %s: %v", ErrInvalidYAML, relPath, err) - } - continue - } - if !promptDefinitionMatches(raw, id, version) { - continue - } - matches = append(matches, promptDefinitionMatch{ - raw: raw, - sourcePath: fullPath, - path: relPath, - }) - } - - if len(matches) > 1 { - paths := make([]string, 0, len(matches)) - for _, match := range matches { - paths = append(paths, match.path) - } - if version != "" { - return nil, fmt.Errorf("%w: duplicate prompt definition id %q version %q found in: %s", ErrInvalidPromptDefinition, id, version, strings.Join(paths, ", ")) - } - return nil, fmt.Errorf("%w: duplicate prompt definition id %q found in: %s", ErrInvalidPromptDefinition, id, strings.Join(paths, ", ")) - } - - if len(matches) == 1 { - match := matches[0] - def, err := normalizePromptDefinition(match.raw, sourceRoot, match.sourcePath) - if err != nil { - return nil, fmt.Errorf("%w: %s: %v", ErrInvalidPromptDefinition, match.path, err) - } - return def, nil - } - - return nil, ErrPromptDefinitionNotFound -} - func decodePromptDefinition(data []byte) (*promptDefinitionFile, error) { var raw promptDefinitionFile decoder := yaml.NewDecoder(bytes.NewReader(data)) diff --git a/internal/promptdef/repository_test.go b/internal/promptdef/repository_test.go index 478d399..f308f47 100644 --- a/internal/promptdef/repository_test.go +++ b/internal/promptdef/repository_test.go @@ -3,6 +3,7 @@ package promptdef import ( "context" "errors" + "fmt" "io/fs" "os" "path/filepath" @@ -14,13 +15,39 @@ import ( "gitea.maximumdirect.net/eric/promptkit/internal/domain" ) -func TestFilesystemRepository_GetPromptDefinition(t *testing.T) { +func TestPromptRepositoryDefinitionFixtures(t *testing.T) { + sources := []struct { + name string + newRepository func(string) Repository + contentPathsAreFull bool + }{ + { + name: "operating system", + newRepository: NewFilesystemRepository, + contentPathsAreFull: true, + }, + { + name: "filesystem", + newRepository: func(root string) Repository { + return NewFSRepository(os.DirFS(root), ".") + }, + }, + } + + for _, source := range sources { + t.Run(source.name, func(t *testing.T) { + testPromptRepositoryDefinitionFixtures(t, source.newRepository, source.contentPathsAreFull) + }) + } +} + +func testPromptRepositoryDefinitionFixtures(t *testing.T, newRepository func(string) Repository, contentPathsAreFull bool) { tmpDir := t.TempDir() if err := copyTree("testdata", tmpDir); err != nil { t.Fatalf("failed to copy testdata: %v", err) } - repo := NewFilesystemRepository(tmpDir) + repo := newRepository(tmpDir) ctx := context.Background() t.Run("valid inline prompt", func(t *testing.T) { @@ -65,8 +92,8 @@ func TestFilesystemRepository_GetPromptDefinition(t *testing.T) { if p.Templates[1].ContentFile == "" { t.Fatal("expected ContentFile source metadata to be preserved") } - if !filepath.IsAbs(p.Templates[1].ContentFile) { - t.Fatalf("expected resolved content_file path to be absolute, got %q", p.Templates[1].ContentFile) + if filepath.IsAbs(p.Templates[1].ContentFile) != contentPathsAreFull { + t.Fatalf("unexpected content_file path representation: %q", p.Templates[1].ContentFile) } }) @@ -456,63 +483,6 @@ func (f *recordingFS) openCount(name string) int { return count } -func TestFSRepositoryRejectsDuplicatePromptIDs(t *testing.T) { - repo := NewFSRepository(fstest.MapFS{ - "one.yaml": &fstest.MapFile{Data: []byte(` -id: duplicate-fs-prompt -version: "1.0.0" -messages: - - role: user - content: First. -output: - format: text - validation_mode: none - repair_attempts: 0 -`)}, - "nested/two.yaml": &fstest.MapFile{Data: []byte(` -id: duplicate-fs-prompt -version: "1.0.0" -messages: - - role: user - content: Second. -output: - format: text - validation_mode: none - repair_attempts: 0 -`)}, - }, ".") - - _, err := repo.GetPromptDefinition(context.Background(), "duplicate-fs-prompt", "") - if !errors.Is(err, ErrInvalidPromptDefinition) { - t.Fatalf("expected ErrInvalidPromptDefinition, got %v", err) - } - if !strings.Contains(err.Error(), "one.yaml") || !strings.Contains(err.Error(), "nested/two.yaml") { - t.Fatalf("expected duplicate paths in error, got %v", err) - } -} - -func TestFSRepositoryRejectsUnknownYAMLFields(t *testing.T) { - repo := NewFSRepository(fstest.MapFS{ - "not_named_like_id.yaml": &fstest.MapFile{Data: []byte(` -id: strict-fs-prompt -version: "1.0.0" -unknown: true -messages: - - role: user - content: Invalid. -output: - format: text - validation_mode: none - repair_attempts: 0 -`)}, - }, ".") - - _, err := repo.GetPromptDefinition(context.Background(), "strict-fs-prompt", "") - if !errors.Is(err, ErrInvalidYAML) { - t.Fatalf("expected ErrInvalidYAML, got %v", err) - } -} - func TestPromptRepositorySelectionUsesYAMLMetadata(t *testing.T) { const validDefinition = ` id: selected-prompt @@ -529,6 +499,7 @@ output: files map[string]string wantErr error diagnostics []string + wantContent string }{ { name: "same-stem strict error with different YAML ID is unrelated", @@ -561,6 +532,41 @@ output: "valid.yaml": validDefinition, }, }, + { + name: "selected content is resolved relative to its definition", + files: map[string]string{ + "nested/selected.yaml": ` +id: selected-prompt +version: "1" +messages: + - role: user + content_file: ./content/selected.tmpl +output: + format: text + validation_mode: none +`, + "nested/content/selected.tmpl": "selected from file", + }, + wantContent: "selected from file", + }, + { + name: "duplicate selected definitions are ambiguous", + files: map[string]string{ + "selected-a.yaml": validDefinition, + "nested/selected-b.yaml": ` +id: selected-prompt +version: "1" +messages: + - role: user + content: duplicate +output: + format: text + validation_mode: none +`, + }, + wantErr: ErrInvalidPromptDefinition, + diagnostics: []string{"duplicate prompt definition id", "selected-a.yaml", "nested/selected-b.yaml"}, + }, { name: "selected strict error is authoritative", files: map[string]string{ @@ -587,6 +593,23 @@ output: wantErr: ErrInvalidPromptDefinition, diagnostics: []string{"selected-invalid.yaml", "at least one message"}, }, + { + name: "selected content error includes definition context", + files: map[string]string{ + "selected-missing-content.yaml": ` +id: selected-prompt +version: "1" +messages: + - role: user + content_file: missing.tmpl +output: + format: text + validation_mode: none +`, + }, + wantErr: ErrInvalidPromptDefinition, + diagnostics: []string{"selected-missing-content.yaml", "failed to read content_file", "missing.tmpl"}, + }, } for _, source := range promptRepositorySources() { @@ -608,7 +631,11 @@ output: if err != nil { t.Fatalf("load selected prompt: %v", err) } - if got.ID != "selected-prompt" || got.Version != "1" || got.Templates[0].Content != "selected" { + wantContent := tc.wantContent + if wantContent == "" { + wantContent = "selected" + } + if got.ID != "selected-prompt" || got.Version != "1" || got.Templates[0].Content != wantContent { t.Fatalf("unexpected selected definition: %+v", got) } }) @@ -616,6 +643,32 @@ output: } } +func TestPromptRepositoryHonorsCancellation(t *testing.T) { + for _, source := range promptRepositorySources() { + t.Run(source.name, func(t *testing.T) { + repo := source.newRepository(t, map[string]string{ + "definition.yaml": ` +id: cancelled-prompt +version: "1" +messages: + - role: user + content: selected +output: + format: text + validation_mode: none +`, + }) + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + _, err := repo.GetPromptDefinition(ctx, "cancelled-prompt", "1") + if !errors.Is(err, context.Canceled) { + t.Fatalf("expected context cancellation, got %v", err) + } + }) + } +} + func TestPromptRepositoryRequiresOneYAMLDocument(t *testing.T) { const definition = ` id: one-document @@ -902,6 +955,70 @@ func promptRepositorySources() []promptRepositorySource { } } +func BenchmarkPromptRepositoryLookup(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": &fstest.MapFile{Data: []byte(` +id: target +version: "1" +messages: + - role: user + content_file: target.tmpl +output: + format: text + validation_mode: none +`)}, + "target.tmpl": &fstest.MapFile{Data: []byte("selected")}, + } + metadataNames := []string{"target.yaml"} + contentNames := make([]string, 0, size-1) + for i := 1; i < size; i++ { + definitionName := fmt.Sprintf("prompt-%04d.yaml", i) + contentName := fmt.Sprintf("prompt-%04d.tmpl", i) + files[definitionName] = &fstest.MapFile{Data: []byte(fmt.Sprintf(` +id: prompt-%04d +version: "1" +messages: + - role: user + content_file: %s +output: + format: text + validation_mode: none +`, i, contentName))} + files[contentName] = &fstest.MapFile{Data: []byte("unrelated")} + metadataNames = append(metadataNames, definitionName) + contentNames = append(contentNames, contentName) + } + + fsys := &recordingFS{FS: files} + repo := NewFSRepository(fsys, ".") + b.ReportAllocs() + b.ResetTimer() + for i := 0; i < b.N; i++ { + if _, err := repo.GetPromptDefinition(context.Background(), "target", "1"); err != nil { + b.Fatal(err) + } + } + b.StopTimer() + + if got := fsys.openCount("target.tmpl"); got != b.N { + b.Fatalf("selected content opens = %d, want %d", got, b.N) + } + for _, name := range contentNames { + if got := fsys.openCount(name); got != 0 { + b.Fatalf("unrelated content %q opened %d times", name, got) + } + } + for _, name := range metadataNames { + if got := fsys.openCount(name); got != b.N { + b.Fatalf("metadata %q opens = %d, want %d", name, got, b.N) + } + } + }) + } +} + func assertCacheControl(t *testing.T, got *domain.CacheControl, wantType domain.CacheControlType, wantTTL string) { t.Helper() if got == nil { diff --git a/internal/promptdef/source.go b/internal/promptdef/source.go new file mode 100644 index 0000000..d8fcbb6 --- /dev/null +++ b/internal/promptdef/source.go @@ -0,0 +1,63 @@ +package promptdef + +import ( + "context" + "errors" + "io/fs" + "os" + + "gitea.maximumdirect.net/eric/promptkit/internal/filecatalog" +) + +type promptDefinitionSource interface { + contentSourceRoot + findYAMLFiles(context.Context) ([]string, error) + readDefinition(string) ([]byte, error) + displayPath(string) string +} + +type osPromptSource struct { + root string + contentRoot osContentSourceRoot +} + +func (s osPromptSource) findYAMLFiles(ctx context.Context) ([]string, error) { + return filecatalog.FindYAMLFiles(ctx, s.root) +} + +func (s osPromptSource) readDefinition(name string) ([]byte, error) { + return os.ReadFile(name) +} + +func (s osPromptSource) displayPath(name string) string { + return filecatalog.RelativePath(s.root, name) +} + +func (s osPromptSource) readContentFile(sourcePath string, contentFile string) (string, string, error) { + return s.contentRoot.readContentFile(sourcePath, contentFile) +} + +type fsPromptSource struct { + fsys fs.FS + root string + contentRoot contentSourceRoot +} + +func (s fsPromptSource) findYAMLFiles(ctx context.Context) ([]string, error) { + if s.fsys == nil { + return nil, errors.New("filesystem is nil") + } + return filecatalog.FindFSYAMLFiles(ctx, s.fsys, s.root) +} + +func (s fsPromptSource) readDefinition(name string) ([]byte, error) { + return fs.ReadFile(s.fsys, name) +} + +func (s fsPromptSource) displayPath(name string) string { + return filecatalog.DisplayPath(s.root, name) +} + +func (s fsPromptSource) readContentFile(sourcePath string, contentFile string) (string, string, error) { + return s.contentRoot.readContentFile(sourcePath, contentFile) +}