diff --git a/docs/formats.md b/docs/formats.md index dd6746a..6fb1ba3 100644 --- a/docs/formats.md +++ b/docs/formats.md @@ -9,9 +9,10 @@ explains how to select these sources and invoke the engine. The owns the resulting outbound wire behavior. Prompt and profile sources recursively discover files ending in `.yaml` or -`.yml`. YAML decoding is strict: unknown fields are errors for the selected -definition. Definitions are selected by their YAML `id`, not their file name -or directory. +`.yml`. Each prompt-definition file contains exactly one YAML document; +comments and trailing whitespace are allowed. YAML decoding is strict: unknown +fields are errors for the selected definition. Definitions are selected by +their YAML `id`, not their file name or directory. ## Prompt Definitions diff --git a/docs/internal/sources.md b/docs/internal/sources.md index ff2da90..9fb52b1 100644 --- a/docs/internal/sources.md +++ b/docs/internal/sources.md @@ -12,9 +12,12 @@ validation modes, built-in catalog, and source precedence. ## Prompt Definitions -`internal/promptdef` discovers YAML deterministically, decodes and validates -definitions, selects an ID and optional version, and resolves file-backed -message content through an explicit source-root abstraction. Operating-system +`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 diff --git a/internal/promptdef/filesystem_repository.go b/internal/promptdef/filesystem_repository.go index e0f3a50..567f01c 100644 --- a/internal/promptdef/filesystem_repository.go +++ b/internal/promptdef/filesystem_repository.go @@ -5,10 +5,9 @@ import ( "context" "errors" "fmt" + "io" "io/fs" "os" - "path" - "path/filepath" "strings" "gitea.maximumdirect.net/eric/promptkit/internal/domain" @@ -116,33 +115,24 @@ func (r *filesystemRepository) GetPromptDefinition(ctx context.Context, id strin } relPath := filecatalog.RelativePath(r.dir, fullPath) - fileMatch := filecatalog.Stem(filepath.Base(fullPath)) == id - - raw, err := loadPromptDefinitionFile(fullPath) + data, err := os.ReadFile(fullPath) if err != nil { - if fileMatch || promptDefinitionFileHasID(fullPath, id) { + continue + } + raw, err := decodePromptDefinition(data) + if err != nil { + if promptDefinitionDataMatches(data, id, version) { return nil, fmt.Errorf("%w: %s: %v", ErrInvalidYAML, relPath, err) } continue } - - def, err := normalizePromptDefinition(raw, r.sourceRoot, fullPath) - if err != nil { - if fileMatch || strings.TrimSpace(raw.ID) == id { - return nil, fmt.Errorf("%w: %s: %v", ErrInvalidPromptDefinition, relPath, err) - } - continue - } - - if def.ID != id { - continue - } - if version != "" && def.Version != version { + if !promptDefinitionMatches(raw, id, version) { continue } matches = append(matches, promptDefinitionMatch{ - def: def, - path: relPath, + raw: raw, + sourcePath: fullPath, + path: relPath, }) } @@ -158,7 +148,12 @@ func (r *filesystemRepository) GetPromptDefinition(ctx context.Context, id strin } if len(matches) == 1 { - return matches[0].def, nil + match := matches[0] + def, err := normalizePromptDefinition(match.raw, r.sourceRoot, match.sourcePath) + if err != nil { + return nil, fmt.Errorf("%w: %s: %v", ErrInvalidPromptDefinition, match.path, err) + } + return def, nil } return nil, ErrPromptDefinitionNotFound @@ -169,37 +164,9 @@ func (r *fsRepository) GetPromptDefinition(ctx context.Context, id string, versi } type promptDefinitionMatch struct { - def *domain.PromptDefinition - path string -} - -func loadPromptDefinitionFile(path string) (*promptDefinitionFile, error) { - data, err := os.ReadFile(path) - if err != nil { - return nil, fmt.Errorf("failed to read prompt definition file: %w", err) - } - - var raw promptDefinitionFile - decoder := yaml.NewDecoder(bytes.NewReader(data)) - decoder.KnownFields(true) - if err := decoder.Decode(&raw); err != nil { - return nil, err - } - return &raw, nil -} - -func promptDefinitionFileHasID(path string, id string) bool { - data, err := os.ReadFile(path) - if err != nil { - return false - } - var raw struct { - ID string `yaml:"id"` - } - if err := yaml.NewDecoder(bytes.NewReader(data)).Decode(&raw); err != nil { - return false - } - return strings.TrimSpace(raw.ID) == id + 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) { @@ -223,40 +190,25 @@ func loadPromptDefinition(ctx context.Context, fsys fs.FS, root string, sourceRo } relPath := filecatalog.DisplayPath(root, fullPath) - fileMatch := filecatalog.Stem(path.Base(fullPath)) == id data, err := fs.ReadFile(fsys, fullPath) if err != nil { - if fileMatch { - return nil, fmt.Errorf("%w: %s: failed to read prompt definition file: %v", ErrInvalidYAML, relPath, err) - } continue } raw, err := decodePromptDefinition(data) if err != nil { - if fileMatch || promptDefinitionDataHasID(data, id) { + if promptDefinitionDataMatches(data, id, version) { return nil, fmt.Errorf("%w: %s: %v", ErrInvalidYAML, relPath, err) } continue } - - def, err := normalizePromptDefinition(raw, sourceRoot, fullPath) - if err != nil { - if fileMatch || strings.TrimSpace(raw.ID) == id { - return nil, fmt.Errorf("%w: %s: %v", ErrInvalidPromptDefinition, relPath, err) - } - continue - } - - if def.ID != id { - continue - } - if version != "" && def.Version != version { + if !promptDefinitionMatches(raw, id, version) { continue } matches = append(matches, promptDefinitionMatch{ - def: def, - path: relPath, + raw: raw, + sourcePath: fullPath, + path: relPath, }) } @@ -272,7 +224,12 @@ func loadPromptDefinition(ctx context.Context, fsys fs.FS, root string, sourceRo } if len(matches) == 1 { - return matches[0].def, nil + 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 @@ -285,17 +242,39 @@ func decodePromptDefinition(data []byte) (*promptDefinitionFile, error) { if err := decoder.Decode(&raw); err != nil { return nil, err } + var additional yaml.Node + if err := decoder.Decode(&additional); err != io.EOF { + if err != nil { + return nil, err + } + return nil, errors.New("prompt definition file must contain exactly one YAML document") + } return &raw, nil } -func promptDefinitionDataHasID(data []byte, id string) bool { +func promptDefinitionDataMatches(data []byte, id string, version string) bool { var raw struct { - ID string `yaml:"id"` + ID string `yaml:"id"` + Version string `yaml:"version"` } if err := yaml.NewDecoder(bytes.NewReader(data)).Decode(&raw); err != nil { return false } - return strings.TrimSpace(raw.ID) == id + return promptSelectorMatches(raw.ID, raw.Version, id, version) +} + +func promptDefinitionMatches(raw *promptDefinitionFile, id string, version string) bool { + if raw == nil { + return false + } + return promptSelectorMatches(raw.ID, raw.Version, id, version) +} + +func promptSelectorMatches(rawID string, rawVersion string, id string, version string) bool { + if strings.TrimSpace(rawID) != id { + return false + } + return version == "" || strings.TrimSpace(rawVersion) == version } func normalizePromptDefinition(raw *promptDefinitionFile, sourceRoot contentSourceRoot, sourcePath string) (*domain.PromptDefinition, error) { diff --git a/internal/promptdef/repository_test.go b/internal/promptdef/repository_test.go index 1ac640f..478d399 100644 --- a/internal/promptdef/repository_test.go +++ b/internal/promptdef/repository_test.go @@ -288,20 +288,20 @@ output: targetErr error errSubstrs []string }{ - {name: "invalid YAML", id: "invalid_yaml", targetErr: ErrInvalidYAML}, - {name: "missing id", id: "missing_id", targetErr: ErrInvalidPromptDefinition, errSubstrs: []string{"id is required"}}, - {name: "no messages", id: "no_messages", targetErr: ErrInvalidPromptDefinition, errSubstrs: []string{"at least one message is required"}}, - {name: "both content and content_file", id: "both_content_and_content_file", targetErr: ErrInvalidPromptDefinition, errSubstrs: []string{"exactly one"}}, - {name: "neither content nor content_file", id: "neither_content_nor_content_file", targetErr: ErrInvalidPromptDefinition, errSubstrs: []string{"exactly one"}}, - {name: "missing content_file", id: "missing_content_file", targetErr: ErrInvalidPromptDefinition, errSubstrs: []string{"failed to read content_file"}}, - {name: "duplicate input names", id: "duplicate_input_names", targetErr: ErrInvalidPromptDefinition, errSubstrs: []string{"duplicate input name"}}, - {name: "invalid validation mode", id: "invalid_validation_mode", targetErr: ErrInvalidPromptDefinition, errSubstrs: []string{"invalid validation mode"}}, - {name: "json_schema without schema_path", id: "json_schema_without_schema_path", targetErr: ErrInvalidPromptDefinition, errSubstrs: []string{"schema_path"}}, - {name: "unknown input field", id: "unknown_input_field", targetErr: ErrInvalidYAML, errSubstrs: []string{"field unknown_input_setting not found"}}, - {name: "empty cache control type", id: "empty_cache_control_type", targetErr: ErrInvalidPromptDefinition, errSubstrs: []string{"cache_control", "type is required"}}, - {name: "unsupported cache control type", id: "unsupported_cache_control_type", targetErr: ErrInvalidPromptDefinition, errSubstrs: []string{"cache_control", "unsupported type"}}, - {name: "unsupported cache control ttl", id: "unsupported_cache_control_ttl", targetErr: ErrInvalidPromptDefinition, errSubstrs: []string{"cache_control", "unsupported ttl"}}, - {name: "unknown cache control field", id: "unknown_cache_control_field", targetErr: ErrInvalidYAML, errSubstrs: []string{"field unexpected not found"}}, + {name: "unidentifiable invalid YAML is unrelated", id: "invalid-yaml", targetErr: ErrPromptDefinitionNotFound}, + {name: "missing id is not selected by filename", id: "missing_id", targetErr: ErrPromptDefinitionNotFound}, + {name: "no messages", id: "no-messages", targetErr: ErrInvalidPromptDefinition, errSubstrs: []string{"at least one message is required"}}, + {name: "both content and content_file", id: "both-content-and-content-file", targetErr: ErrInvalidPromptDefinition, errSubstrs: []string{"exactly one"}}, + {name: "neither content nor content_file", id: "neither-content-nor-content-file", targetErr: ErrInvalidPromptDefinition, errSubstrs: []string{"exactly one"}}, + {name: "missing content_file", id: "missing-content-file", targetErr: ErrInvalidPromptDefinition, errSubstrs: []string{"failed to read content_file"}}, + {name: "duplicate input names", id: "duplicate-input-names", targetErr: ErrInvalidPromptDefinition, errSubstrs: []string{"duplicate input name"}}, + {name: "invalid validation mode", id: "invalid-validation-mode", targetErr: ErrInvalidPromptDefinition, errSubstrs: []string{"invalid validation mode"}}, + {name: "json_schema without schema_path", id: "json-schema-without-schema-path", targetErr: ErrInvalidPromptDefinition, errSubstrs: []string{"schema_path"}}, + {name: "unknown input field", id: "unknown-input-field", targetErr: ErrInvalidYAML, errSubstrs: []string{"field unknown_input_setting not found"}}, + {name: "empty cache control type", id: "empty-cache-control-type", targetErr: ErrInvalidPromptDefinition, errSubstrs: []string{"cache_control", "type is required"}}, + {name: "unsupported cache control type", id: "unsupported-cache-control-type", targetErr: ErrInvalidPromptDefinition, errSubstrs: []string{"cache_control", "unsupported type"}}, + {name: "unsupported cache control ttl", id: "unsupported-cache-control-ttl", targetErr: ErrInvalidPromptDefinition, errSubstrs: []string{"cache_control", "unsupported ttl"}}, + {name: "unknown cache control field", id: "unknown-cache-control-field", targetErr: ErrInvalidYAML, errSubstrs: []string{"field unexpected not found"}}, } for _, tc := range cases { @@ -441,14 +441,19 @@ func (f *recordingFS) Open(name string) (fs.File, error) { } func (f *recordingFS) wasOpened(name string) bool { + return f.openCount(name) > 0 +} + +func (f *recordingFS) openCount(name string) int { f.mu.Lock() defer f.mu.Unlock() + count := 0 for _, opened := range f.opened { if opened == name { - return true + count++ } } - return false + return count } func TestFSRepositoryRejectsDuplicatePromptIDs(t *testing.T) { @@ -508,58 +513,340 @@ output: } } -func TestPromptRepositoriesApplyOutputContractRules(t *testing.T) { +func TestPromptRepositorySelectionUsesYAMLMetadata(t *testing.T) { + const validDefinition = ` +id: selected-prompt +version: "1" +messages: + - role: user + content: selected +output: + format: text + validation_mode: none +` + tests := []struct { + name string + files map[string]string + wantErr error + diagnostics []string + }{ + { + name: "same-stem strict error with different YAML ID is unrelated", + files: map[string]string{ + "selected-prompt.yaml": ` +id: another-prompt +version: "1" +unknown: true +`, + "valid.yaml": validDefinition, + }, + }, + { + name: "unidentifiable same-stem YAML is unrelated", + files: map[string]string{ + "selected-prompt.yaml": "id: [", + "valid.yaml": validDefinition, + }, + }, + { + name: "same ID invalid different version is unrelated", + files: map[string]string{ + "invalid-version.yaml": ` +id: selected-prompt +version: "2" +output: + format: text + validation_mode: none +`, + "valid.yaml": validDefinition, + }, + }, + { + name: "selected strict error is authoritative", + files: map[string]string{ + "selected-strict.yaml": ` +id: selected-prompt +version: "1" +unknown: true +`, + }, + wantErr: ErrInvalidYAML, + diagnostics: []string{"selected-strict.yaml"}, + }, + { + name: "selected semantic error is authoritative", + files: map[string]string{ + "selected-invalid.yaml": ` +id: selected-prompt +version: "1" +output: + format: text + validation_mode: none +`, + }, + wantErr: ErrInvalidPromptDefinition, + diagnostics: []string{"selected-invalid.yaml", "at least one message"}, + }, + } + + for _, source := range promptRepositorySources() { + for _, tc := range tests { + t.Run(source.name+"/"+tc.name, func(t *testing.T) { + repo := source.newRepository(t, tc.files) + got, err := repo.GetPromptDefinition(context.Background(), "selected-prompt", "1") + if tc.wantErr != nil { + if !errors.Is(err, tc.wantErr) { + t.Fatalf("expected %v, got %v", tc.wantErr, err) + } + for _, diagnostic := range tc.diagnostics { + if !strings.Contains(err.Error(), diagnostic) { + t.Fatalf("expected error to contain %q, got %v", diagnostic, err) + } + } + return + } + if err != nil { + t.Fatalf("load selected prompt: %v", err) + } + if got.ID != "selected-prompt" || got.Version != "1" || got.Templates[0].Content != "selected" { + t.Fatalf("unexpected selected definition: %+v", got) + } + }) + } + } +} + +func TestPromptRepositoryRequiresOneYAMLDocument(t *testing.T) { + const definition = ` +id: one-document +version: "1" +messages: + - role: user + content: selected +output: + format: text + validation_mode: none +` + tests := []struct { + name string + suffix string + wantErr bool + }{ + {name: "comments and trailing whitespace", suffix: "\n# trailing comment\n\n"}, + {name: "second populated document", suffix: "\n---\nid: another\n", wantErr: true}, + {name: "second empty document", suffix: "\n---\n", wantErr: true}, + {name: "malformed trailing YAML", suffix: "\n---\n[", wantErr: true}, + } + + for _, source := range promptRepositorySources() { + for _, tc := range tests { + t.Run(source.name+"/"+tc.name, func(t *testing.T) { + repo := source.newRepository(t, map[string]string{"definition.yaml": definition + tc.suffix}) + _, err := repo.GetPromptDefinition(context.Background(), "one-document", "1") + if tc.wantErr { + if !errors.Is(err, ErrInvalidYAML) { + t.Fatalf("expected ErrInvalidYAML, got %v", err) + } + if !strings.Contains(err.Error(), "definition.yaml") { + t.Fatalf("expected source path in error, got %v", err) + } + return + } + if err != nil { + t.Fatalf("load one document: %v", err) + } + }) + } + } +} + +func TestPromptRepositoryReadsOnlySelectedContent(t *testing.T) { + fsys := &recordingFS{FS: 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")}, + "unrelated.yaml": &fstest.MapFile{Data: []byte(` +id: unrelated +version: "1" +messages: + - role: user + content_file: unrelated.tmpl +output: + format: text + validation_mode: none +`)}, + "unrelated.tmpl": &fstest.MapFile{Data: []byte("unrelated")}, + "other-version.yaml": &fstest.MapFile{Data: []byte(` +id: target +version: "2" +messages: + - role: user + content_file: other-version.tmpl +output: + format: text + validation_mode: none +`)}, + "other-version.tmpl": &fstest.MapFile{Data: []byte("other version")}, + }} + repo := NewFSRepository(fsys, ".") + + for lookup := 1; lookup <= 2; lookup++ { + got, err := repo.GetPromptDefinition(context.Background(), "target", "1") + if err != nil { + t.Fatalf("lookup %d: %v", lookup, err) + } + if got.Templates[0].Content != "selected" { + t.Fatalf("lookup %d content = %q", lookup, got.Templates[0].Content) + } + if count := fsys.openCount("target.tmpl"); count != lookup { + t.Fatalf("selected content opens after lookup %d = %d, want %d", lookup, count, lookup) + } + for _, name := range []string{"unrelated.tmpl", "other-version.tmpl"} { + if count := fsys.openCount(name); count != 0 { + t.Fatalf("unselected content %q opened %d times", name, count) + } + } + for _, name := range []string{"target.yaml", "unrelated.yaml", "other-version.yaml"} { + if count := fsys.openCount(name); count != lookup { + t.Fatalf("metadata %q opens after lookup %d = %d, want %d", name, lookup, count, lookup) + } + } + } +} + +func TestPromptDefinitionNormalizationRules(t *testing.T) { tests := []struct { name string - output string - useFilesystem bool + definition string wantErr bool wantDiagnostic string wantSchemaPath string }{ { - name: "operating-system source rejects unsupported format", - output: " format: binary\n validation_mode: none\n", - useFilesystem: true, + name: "missing version", + definition: ` +id: normalization-rule +messages: + - role: user + content: test +output: + format: text + validation_mode: none +`, + wantErr: true, + wantDiagnostic: "version", + }, + { + name: "blank input name", + definition: ` +id: normalization-rule +version: "1" +inputs: + - name: " " +messages: + - role: user + content: test +output: + format: text + validation_mode: none +`, + wantErr: true, + wantDiagnostic: "input 0", + }, + { + name: "blank message role", + definition: ` +id: normalization-rule +version: "1" +messages: + - role: " " + content: test +output: + format: text + validation_mode: none +`, + wantErr: true, + wantDiagnostic: "role", + }, + { + name: "invalid output format", + definition: ` +id: normalization-rule +version: "1" +messages: + - role: user + content: test +output: + format: binary + validation_mode: none +`, wantErr: true, wantDiagnostic: "format", }, { - name: "fs source rejects negative repair attempts", - output: " format: text\n validation_mode: none\n repair_attempts: -1\n", + name: "negative repair attempts", + definition: ` +id: normalization-rule +version: "1" +messages: + - role: user + content: test +output: + format: text + validation_mode: none + repair_attempts: -1 +`, wantErr: true, wantDiagnostic: "repair_attempts", }, { - name: "source normalization trims a valid schema path", - output: " format: json\n validation_mode: json_schema\n schema_path: ' schema.json '\n", - useFilesystem: true, + name: "explicit blank default profile", + definition: ` +id: normalization-rule +version: "1" +default_profile: " " +messages: + - role: user + content: test +output: + format: text + validation_mode: none +`, + wantErr: true, + wantDiagnostic: "default_profile", + }, + { + name: "schema path normalization", + definition: ` +id: normalization-rule +version: "1" +messages: + - role: user + content: test +output: + format: json + validation_mode: json_schema + schema_path: ' schema.json ' +`, wantSchemaPath: "schema.json", }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - data := `id: output-contract -version: "1" -messages: - - role: user - content: test -output: -` + tt.output + repo := NewFSRepository(fstest.MapFS{ + "definition.yaml": &fstest.MapFile{Data: []byte(tt.definition)}, + }, ".") - var repo Repository - if tt.useFilesystem { - dir := t.TempDir() - writePromptTestFile(t, filepath.Join(dir, "output-contract.yaml"), data) - repo = NewFilesystemRepository(dir) - } else { - repo = NewFSRepository(fstest.MapFS{ - "output-contract.yaml": &fstest.MapFile{Data: []byte(data)}, - }, ".") - } - - got, err := repo.GetPromptDefinition(context.Background(), "output-contract", "") + got, err := repo.GetPromptDefinition(context.Background(), "normalization-rule", "") if tt.wantErr { if !errors.Is(err, ErrInvalidPromptDefinition) { t.Fatalf("expected ErrInvalidPromptDefinition, got %v", err) @@ -579,6 +866,42 @@ output: } } +type promptRepositorySource struct { + name string + newRepository func(t *testing.T, files map[string]string) Repository +} + +func promptRepositorySources() []promptRepositorySource { + return []promptRepositorySource{ + { + name: "operating system", + newRepository: func(t *testing.T, files map[string]string) Repository { + t.Helper() + root := t.TempDir() + for name, content := range files { + filePath := filepath.Join(root, filepath.FromSlash(name)) + if err := os.MkdirAll(filepath.Dir(filePath), 0o755); err != nil { + t.Fatalf("create prompt directory: %v", err) + } + writePromptTestFile(t, filePath, content) + } + return NewFilesystemRepository(root) + }, + }, + { + name: "filesystem", + newRepository: func(t *testing.T, files map[string]string) Repository { + t.Helper() + fsys := make(fstest.MapFS, len(files)) + for name, content := range files { + fsys[name] = &fstest.MapFile{Data: []byte(strings.TrimLeft(content, "\n"))} + } + return NewFSRepository(fsys, ".") + }, + }, + } +} + func assertCacheControl(t *testing.T, got *domain.CacheControl, wantType domain.CacheControlType, wantTTL string) { t.Helper() if got == nil {