Refactor: enforce canonical messages/output YAML, normalize content_file, and expand fixture-based validation tests

2026-05-05 10:35:39 -05:00
parent 7fffdaede3
commit f6692dd4bb
27 changed files with 416 additions and 249 deletions
--- a/internal/promptdef/filesystem_repository.go
+++ b/internal/promptdef/filesystem_repository.go
@@ -23,6 +23,36 @@ type filesystemRepository struct {
 	dir string
 }

+type promptDefinitionFile struct {
+	ID             string                   `yaml:"id"`
+	Version        string                   `yaml:"version"`
+	DefaultProfile *string                  `yaml:"default_profile"`
+	Description    string                   `yaml:"description"`
+	Inputs         []promptInputFile        `yaml:"inputs"`
+	Messages       []promptMessageFile      `yaml:"messages"`
+	Output         promptOutputContractFile `yaml:"output"`
+}
+
+type promptInputFile struct {
+	Name        string `yaml:"name"`
+	Required    bool   `yaml:"required"`
+	ContentType string `yaml:"content_type"`
+	Description string `yaml:"description"`
+}
+
+type promptMessageFile struct {
+	Role        string `yaml:"role"`
+	Content     string `yaml:"content"`
+	ContentFile string `yaml:"content_file"`
+}
+
+type promptOutputContractFile struct {
+	Format         domain.OutputFormat   `yaml:"format"`
+	ValidationMode domain.ValidationMode `yaml:"validation_mode"`
+	SchemaPath     string                `yaml:"schema_path"`
+	RepairAttempts int                   `yaml:"repair_attempts"`
+}
+
 func NewFilesystemRepository(dir string) Repository {
 	return &filesystemRepository{dir: dir}
 }
@@ -44,22 +74,25 @@ func (r *filesystemRepository) GetPromptDefinition(ctx context.Context, id strin
 		default:
 		}

-		if file.IsDir() || (!strings.HasSuffix(file.Name(), ".yaml") && !strings.HasSuffix(file.Name(), ".yml")) {
+		if file.IsDir() || !isYAMLFile(file.Name()) {
 			continue
 		}

 		fullPath := filepath.Join(r.dir, file.Name())
-		data, err := os.ReadFile(fullPath)
+		fileMatch := promptIDFromFileName(file.Name()) == id
+
+		raw, err := loadPromptDefinitionFile(fullPath)
 		if err != nil {
-			return nil, fmt.Errorf("failed to read prompt definition file %s: %w", file.Name(), err)
+			if fileMatch {
+				return nil, fmt.Errorf("%w: %s: %v", ErrInvalidYAML, file.Name(), err)
+			}
+			continue
 		}

-		var def domain.PromptDefinition
-		decoder := yaml.NewDecoder(bytes.NewReader(data))
-		decoder.KnownFields(true)
-		if err := decoder.Decode(&def); err != nil {
-			if strings.TrimSuffix(strings.TrimSuffix(file.Name(), ".yaml"), ".yml") == id {
-				return nil, fmt.Errorf("%w: %s: %v", ErrInvalidYAML, file.Name(), err)
+		def, err := normalizePromptDefinition(raw, fullPath)
+		if err != nil {
+			if fileMatch || strings.TrimSpace(raw.ID) == id {
+				return nil, fmt.Errorf("%w: %s: %v", ErrInvalidPromptDefinition, file.Name(), err)
 			}
 			continue
 		}
@@ -70,82 +103,166 @@ func (r *filesystemRepository) GetPromptDefinition(ctx context.Context, id strin
 		if version != "" && def.Version != version {
 			continue
 		}
-		if err := validatePromptDefinition(&def); err != nil {
-			return nil, fmt.Errorf("%w: %s: %v", ErrInvalidPromptDefinition, file.Name(), err)
-		}
-		return &def, nil
+		return def, nil
 	}

 	return nil, ErrPromptDefinitionNotFound
 }

-func validatePromptDefinition(d *domain.PromptDefinition) error {
-	if d.ID == "" {
-		return errors.New("prompt id is required")
+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)
 	}
-	if d.Version == "" {
-		return errors.New("prompt version is required")
+
+	var raw promptDefinitionFile
+	decoder := yaml.NewDecoder(bytes.NewReader(data))
+	decoder.KnownFields(true)
+	if err := decoder.Decode(&raw); err != nil {
+		return nil, err
 	}
-	if len(d.Templates) == 0 {
-		return errors.New("at least one prompt template message is required")
+	return &raw, nil
+}
+
+func normalizePromptDefinition(raw *promptDefinitionFile, sourcePath string) (*domain.PromptDefinition, error) {
+	if raw == nil {
+		return nil, errors.New("prompt definition is nil")
 	}
-	if len(d.Inputs) == 0 {
-		return errors.New("at least one prompt input is required")
+
+	id := strings.TrimSpace(raw.ID)
+	if id == "" {
+		return nil, errors.New("id is required")
 	}
-	for i, input := range d.Inputs {
-		if strings.TrimSpace(input.Name) == "" {
-			return fmt.Errorf("input %d has empty name", i)
+
+	version := strings.TrimSpace(raw.Version)
+	if version == "" {
+		return nil, errors.New("version is required")
+	}
+
+	if len(raw.Messages) == 0 {
+		return nil, errors.New("at least one message is required")
+	}
+
+	inputs := make([]domain.PromptInput, 0, len(raw.Inputs))
+	seenInputNames := make(map[string]struct{}, len(raw.Inputs))
+	for i, in := range raw.Inputs {
+		name := strings.TrimSpace(in.Name)
+		if name == "" {
+			return nil, fmt.Errorf("input %d has empty name", i)
+		}
+		if _, exists := seenInputNames[name]; exists {
+			return nil, fmt.Errorf("duplicate input name %q", name)
+		}
+		seenInputNames[name] = struct{}{}
+
+		inputs = append(inputs, domain.PromptInput{
+			Name:        name,
+			Required:    in.Required,
+			ContentType: strings.TrimSpace(in.ContentType),
+			Description: strings.TrimSpace(in.Description),
+		})
+	}
+
+	templates := make([]domain.PromptMessageTemplate, 0, len(raw.Messages))
+	promptDir := filepath.Dir(sourcePath)
+	for i, msg := range raw.Messages {
+		role := strings.TrimSpace(msg.Role)
+		if role == "" {
+			return nil, fmt.Errorf("message %d role is required", i)
+		}
+
+		hasContent := strings.TrimSpace(msg.Content) != ""
+		hasContentFile := strings.TrimSpace(msg.ContentFile) != ""
+		if hasContent == hasContentFile {
+			return nil, fmt.Errorf("message %d (%s) must set exactly one of content or content_file", i, role)
+		}
+
+		templateContent := msg.Content
+		resolvedContentFile := ""
+		if hasContentFile {
+			resolvedPath := strings.TrimSpace(msg.ContentFile)
+			if !filepath.IsAbs(resolvedPath) {
+				resolvedPath = filepath.Join(promptDir, resolvedPath)
+			}
+			resolvedPath = filepath.Clean(resolvedPath)
+
+			body, err := os.ReadFile(resolvedPath)
+			if err != nil {
+				return nil, fmt.Errorf("prompt %q message %d (%s): failed to read content_file %q: %w", id, i, role, msg.ContentFile, err)
+			}
+			templateContent = string(body)
+			resolvedContentFile = resolvedPath
+		}
+
+		templates = append(templates, domain.PromptMessageTemplate{
+			Role:        role,
+			Content:     templateContent,
+			ContentFile: resolvedContentFile,
+		})
+	}
+
+	if !isValidOutputFormat(raw.Output.Format) {
+		return nil, fmt.Errorf("invalid output format: %q", raw.Output.Format)
+	}
+	if !isValidValidationMode(raw.Output.ValidationMode) {
+		return nil, fmt.Errorf("invalid validation mode: %q", raw.Output.ValidationMode)
+	}
+	if raw.Output.ValidationMode == domain.ValidationJSONSchema && strings.TrimSpace(raw.Output.SchemaPath) == "" {
+		return nil, errors.New("output.schema_path is required when output.validation_mode is json_schema")
+	}
+	if raw.Output.RepairAttempts < 0 {
+		return nil, errors.New("output.repair_attempts must be greater than or equal to 0")
+	}
+
+	defaultProfile := ""
+	if raw.DefaultProfile != nil {
+		defaultProfile = strings.TrimSpace(*raw.DefaultProfile)
+		if defaultProfile == "" {
+			return nil, errors.New("default_profile must be a non-empty string when set")
 		}
 	}
-	for i, t := range d.Templates {
-		if !isValidMessageRole(t.Role) {
-			return fmt.Errorf("template message %d has invalid role %q", i, t.Role)
-		}
-		if strings.TrimSpace(t.Content) == "" && strings.TrimSpace(t.ContentFile) == "" {
-			return fmt.Errorf("template message %d must provide content or content_file", i)
-		}
-		if strings.TrimSpace(t.Content) != "" && strings.TrimSpace(t.ContentFile) != "" {
-			return fmt.Errorf("template message %d cannot set both content and content_file", i)
-		}
-	}
-	if !isValidOutputFormat(d.OutputFormat) {
-		return fmt.Errorf("invalid output format: %s", d.OutputFormat)
-	}
-	if !isValidValidationMode(d.Validation.ValidationMode) {
-		return fmt.Errorf("invalid validation mode: %s", d.Validation.ValidationMode)
-	}
-	if d.Validation.RepairAttempts < 0 {
-		return errors.New("validation.repair_attempts must be greater than or equal to 0")
-	}
-	if d.Validation.ValidationMode == domain.ValidationJSONSchema && strings.TrimSpace(d.Validation.SchemaPath) == "" {
-		return errors.New("validation.schema_path is required when validation_mode is json_schema")
-	}
-	if d.Validation.Format != "" && d.Validation.Format != d.OutputFormat {
-		return fmt.Errorf("validation format %q does not match output format %q", d.Validation.Format, d.OutputFormat)
-	}
-	return nil
+
+	return &domain.PromptDefinition{
+		ID:             id,
+		Version:        version,
+		DefaultProfile: defaultProfile,
+		Description:    strings.TrimSpace(raw.Description),
+		Inputs:         inputs,
+		Templates:      templates,
+		OutputFormat:   raw.Output.Format,
+		Validation: domain.OutputContract{
+			Format:         raw.Output.Format,
+			ValidationMode: raw.Output.ValidationMode,
+			SchemaPath:     strings.TrimSpace(raw.Output.SchemaPath),
+			RepairAttempts: raw.Output.RepairAttempts,
+		},
+	}, nil
+}
+
+func isYAMLFile(name string) bool {
+	return strings.HasSuffix(name, ".yaml") || strings.HasSuffix(name, ".yml")
+}
+
+func promptIDFromFileName(name string) string {
+	name = strings.TrimSuffix(name, ".yaml")
+	name = strings.TrimSuffix(name, ".yml")
+	return name
 }

 func isValidOutputFormat(f domain.OutputFormat) bool {
 	switch f {
 	case domain.FormatText, domain.FormatMarkdown, domain.FormatJSON:
 		return true
+	default:
+		return false
 	}
-	return false
 }

 func isValidValidationMode(m domain.ValidationMode) bool {
 	switch m {
 	case domain.ValidationNone, domain.ValidationBasic, domain.ValidationJSON, domain.ValidationJSONSchema:
 		return true
+	default:
+		return false
 	}
-	return false
-}
-
-func isValidMessageRole(role string) bool {
-	switch role {
-	case "system", "user", "assistant", "developer":
-		return true
-	}
-	return false
 }