refactor(sanitize): address PR feedback and improve code quality

- Simplify nested conditions in removeExtensions by consolidating checks
- Add SanitizeResult type to collect and return warnings for invalid glob patterns
- Pre-allocate unknownProperties slice capacity for better performance
- Use map[string]struct{} instead of map[string]bool for memory efficiency
- Add PrintWarning method to OpenAPIProcessor for warning display
- Update all tests and CLI to handle new return signature

Addresses Copilot review feedback in PR #59

Author: TristanSpeakEasy

marshaller/unmarshaller.go

@@ -349,7 +349,7 @@ func unmarshalModel(ctx context.Context, parentName string, node *yaml.Node, str
 
 	// Track unknown properties (non-extension, non-field, non-embedded map properties)
 	var unknownPropertiesMutex sync.Mutex
-	var unknownProperties []string
+	unknownProperties := make([]string, 0, numJobs)
 
 	// Mutex to protect concurrent access to extensionsField
 	var extensionsMutex sync.Mutex

openapi/cmd/sanitize.go

@@ -143,10 +143,16 @@ func sanitizeOpenAPI(ctx context.Context, processor *OpenAPIProcessor) error {
 	}
 
 	// Perform the sanitization
-	if err := openapi.Sanitize(ctx, doc, opts); err != nil {
+	result, err := openapi.Sanitize(ctx, doc, opts)
+	if err != nil {
 		return fmt.Errorf("failed to sanitize OpenAPI document: %w", err)
 	}
 
+	// Report any warnings
+	for _, warning := range result.Warnings {
+		processor.PrintWarning(warning)
+	}
+
 	// Report success
 	reportSanitizationResults(processor, opts)
 

openapi/cmd/shared.go

@@ -114,3 +114,10 @@ func (p *OpenAPIProcessor) PrintInfo(message string) {
 		fmt.Printf("📋 %s\n", message)
 	}
 }
+
+// PrintWarning prints a warning message if not writing to stdout
+func (p *OpenAPIProcessor) PrintWarning(message string) {
+	if !p.WriteToStdout {
+		fmt.Printf("⚠️  Warning: %s\n", message)
+	}
+}

openapi/sanitize.go

@@ -35,6 +35,13 @@ type SanitizeOptions struct {
 	KeepUnknownProperties bool `yaml:"keepUnknownProperties,omitempty"`
 }
 
+// SanitizeResult contains the results of a sanitization operation.
+type SanitizeResult struct {
+	// Warnings contains non-fatal issues encountered during sanitization.
+	// These typically include invalid glob patterns that were skipped.
+	Warnings []string
+}
+
 // Sanitize cleans an OpenAPI document by removing unwanted elements.
 // By default (nil options or zero values), it provides aggressive cleanup:
 //   - Removes all extensions (x-*)
@@ -64,18 +71,21 @@ type SanitizeOptions struct {
 // Example usage:
 //
 //	// Default sanitization: remove all extensions, unused components, and unknown properties
-//	err := Sanitize(ctx, doc, nil)
+//	result, err := Sanitize(ctx, doc, nil)
 //	if err != nil {
 //		return fmt.Errorf("failed to sanitize document: %w", err)
 //	}
+//	for _, warning := range result.Warnings {
+//		fmt.Fprintf(os.Stderr, "Warning: %s\n", warning)
+//	}
 //
 //	// Remove only x-go-* extensions, keep everything else
 //	opts := &SanitizeOptions{
 //		ExtensionPatterns: []string{"x-go-*"},
 //		KeepUnusedComponents: true,
 //		KeepUnknownProperties: true,
 //	}
-//	err := Sanitize(ctx, doc, opts)
+//	result, err := Sanitize(ctx, doc, opts)
 //	if err != nil {
 //		return fmt.Errorf("failed to sanitize document: %w", err)
 //	}
@@ -84,18 +94,21 @@ type SanitizeOptions struct {
 //	opts := &SanitizeOptions{
 //		KeepUnusedComponents: true,
 //	}
-//	err := Sanitize(ctx, doc, opts)
+//	result, err := Sanitize(ctx, doc, opts)
 //
 // Parameters:
 //   - ctx: Context for the operation
 //   - doc: The OpenAPI document to sanitize (modified in place)
 //   - opts: Sanitization options (nil uses defaults: aggressive cleanup)
 //
 // Returns:
+//   - *SanitizeResult: Result containing any warnings from the operation
 //   - error: Any error that occurred during sanitization
-func Sanitize(ctx context.Context, doc *OpenAPI, opts *SanitizeOptions) error {
+func Sanitize(ctx context.Context, doc *OpenAPI, opts *SanitizeOptions) (*SanitizeResult, error) {
+	result := &SanitizeResult{}
+
 	if doc == nil {
-		return nil
+		return result, nil
 	}
 
 	// Use default options if nil
@@ -104,25 +117,27 @@ func Sanitize(ctx context.Context, doc *OpenAPI, opts *SanitizeOptions) error {
 	}
 
 	// Remove extensions based on configuration
-	if err := removeExtensions(ctx, doc, opts); err != nil {
-		return fmt.Errorf("failed to remove extensions: %w", err)
+	warnings, err := removeExtensions(ctx, doc, opts)
+	if err != nil {
+		return result, fmt.Errorf("failed to remove extensions: %w", err)
 	}
+	result.Warnings = append(result.Warnings, warnings...)
 
 	// Remove unknown properties if not keeping them
 	if !opts.KeepUnknownProperties {
 		if err := removeUnknownProperties(ctx, doc); err != nil {
-			return fmt.Errorf("failed to remove unknown properties: %w", err)
+			return result, fmt.Errorf("failed to remove unknown properties: %w", err)
 		}
 	}
 
 	// Clean unused components if not keeping them
 	if !opts.KeepUnusedComponents {
 		if err := Clean(ctx, doc); err != nil {
-			return fmt.Errorf("failed to clean unused components: %w", err)
+			return result, fmt.Errorf("failed to clean unused components: %w", err)
 		}
 	}
 
-	return nil
+	return result, nil
 }
 
 // LoadSanitizeConfig loads sanitize configuration from a YAML reader.
@@ -152,21 +167,23 @@ func LoadSanitizeConfigFromFile(path string) (*SanitizeOptions, error) {
 }
 
 // removeExtensions walks through the document and removes extensions based on options.
-func removeExtensions(ctx context.Context, doc *OpenAPI, opts *SanitizeOptions) error {
+// Returns a slice of warnings for any invalid glob patterns encountered.
+func removeExtensions(ctx context.Context, doc *OpenAPI, opts *SanitizeOptions) ([]string, error) {
 	// Determine removal strategy:
 	// - nil ExtensionPatterns: remove ALL extensions (default)
 	// - empty array []: keep ALL extensions (explicit no-op)
 	// - non-empty array: remove only matching patterns
 
-	if opts != nil && opts.ExtensionPatterns != nil && len(opts.ExtensionPatterns) == 0 {
-		// Empty array explicitly set = keep all extensions
-		return nil
-	}
-
 	var patterns []string
 	removeAll := true
+	var invalidPatterns []string
 
-	if opts != nil && opts.ExtensionPatterns != nil && len(opts.ExtensionPatterns) > 0 {
+	// Handle extension patterns if explicitly set
+	if opts != nil && opts.ExtensionPatterns != nil {
+		if len(opts.ExtensionPatterns) == 0 {
+			// Empty array explicitly set = keep all extensions
+			return nil, nil
+		}
 		// Use patterns for selective removal
 		patterns = opts.ExtensionPatterns
 		removeAll = false
@@ -189,7 +206,14 @@ func removeExtensions(ctx context.Context, doc *OpenAPI, opts *SanitizeOptions)
 						shouldRemove = true
 					} else {
 						// Check if extension matches any pattern
-						shouldRemove = matchesAnyPattern(key, patterns)
+						matched, invalid := matchesAnyPattern(key, patterns)
+						shouldRemove = matched
+						// Collect invalid patterns (only once per pattern)
+						for _, pattern := range invalid {
+							if !contains(invalidPatterns, pattern) {
+								invalidPatterns = append(invalidPatterns, pattern)
+							}
+						}
 					}
 
 					if shouldRemove {
@@ -206,11 +230,17 @@ func removeExtensions(ctx context.Context, doc *OpenAPI, opts *SanitizeOptions)
 			},
 		})
 		if err != nil {
-			return fmt.Errorf("failed to process extensions: %w", err)
+			return nil, fmt.Errorf("failed to process extensions: %w", err)
 		}
 	}
 
-	return nil
+	// Convert invalid patterns to warnings
+	var warnings []string
+	for _, pattern := range invalidPatterns {
+		warnings = append(warnings, fmt.Sprintf("invalid glob pattern '%s' was skipped", pattern))
+	}
+
+	return warnings, nil
 }
 
 // removeUnknownProperties removes properties that are not defined in the OpenAPI specification.
@@ -370,9 +400,9 @@ func removePropertiesFromNode(node *yaml.Node, keysToRemove []string) {
 	}
 
 	// Build a set of keys to remove for efficient lookup
-	removeSet := make(map[string]bool, len(keysToRemove))
+	removeSet := make(map[string]struct{}, len(keysToRemove))
 	for _, key := range keysToRemove {
-		removeSet[key] = true
+		removeSet[key] = struct{}{}
 	}
 
 	// Filter content to exclude keys in the remove set
@@ -385,9 +415,11 @@ func removePropertiesFromNode(node *yaml.Node, keysToRemove []string) {
 		keyNode := node.Content[i]
 		valueNode := node.Content[i+1]
 
-		if keyNode.Kind == yaml.ScalarNode && removeSet[keyNode.Value] {
-			// Skip this key-value pair (it's unknown)
-			continue
+		if keyNode.Kind == yaml.ScalarNode {
+			if _, shouldRemove := removeSet[keyNode.Value]; shouldRemove {
+				// Skip this key-value pair (it's unknown)
+				continue
+			}
 		}
 
 		// Keep this key-value pair
@@ -399,14 +431,28 @@ func removePropertiesFromNode(node *yaml.Node, keysToRemove []string) {
 }
 
 // matchesAnyPattern checks if a string matches any of the provided glob patterns.
-func matchesAnyPattern(str string, patterns []string) bool {
+// Returns (matched bool, invalidPatterns []string) where invalidPatterns contains
+// any patterns that failed to compile.
+func matchesAnyPattern(str string, patterns []string) (bool, []string) {
+	var invalidPatterns []string
 	for _, pattern := range patterns {
 		matched, err := filepath.Match(pattern, str)
 		if err != nil {
-			// Invalid pattern, skip it
+			// Collect invalid pattern
+			invalidPatterns = append(invalidPatterns, pattern)
 			continue
 		}
 		if matched {
+			return true, invalidPatterns
+		}
+	}
+	return false, invalidPatterns
+}
+
+// contains checks if a string slice contains a specific string.
+func contains(slice []string, str string) bool {
+	for _, s := range slice {
+		if s == str {
 			return true
 		}
 	}

openapi/sanitize_test.go

@@ -26,8 +26,9 @@ func TestSanitize_RemoveAllExtensions_Success(t *testing.T) {
 	require.Empty(t, validationErrs, "Input document should be valid")
 
 	// Sanitize with default options (remove all extensions and clean components)
-	err = openapi.Sanitize(ctx, inputDoc, nil)
+	result, err := openapi.Sanitize(ctx, inputDoc, nil)
 	require.NoError(t, err)
+	assert.Empty(t, result.Warnings, "Should not have warnings")
 
 	// Marshal the sanitized document to YAML
 	var buf bytes.Buffer
@@ -63,8 +64,9 @@ func TestSanitize_PatternBased_Success(t *testing.T) {
 		KeepUnusedComponents:  true,
 		KeepUnknownProperties: true,
 	}
-	err = openapi.Sanitize(ctx, inputDoc, opts)
+	result, err := openapi.Sanitize(ctx, inputDoc, opts)
 	require.NoError(t, err)
+	assert.Empty(t, result.Warnings, "Should not have warnings")
 
 	// Marshal the sanitized document to YAML
 	var buf bytes.Buffer
@@ -99,8 +101,9 @@ func TestSanitize_MultiplePatterns_Success(t *testing.T) {
 		ExtensionPatterns:    []string{"x-go-*", "x-internal-*"},
 		KeepUnusedComponents: true,
 	}
-	err = openapi.Sanitize(ctx, inputDoc, opts)
+	result, err := openapi.Sanitize(ctx, inputDoc, opts)
 	require.NoError(t, err)
+	assert.Empty(t, result.Warnings, "Should not have warnings")
 
 	// Marshal the sanitized document to YAML
 	var buf bytes.Buffer
@@ -134,8 +137,9 @@ func TestSanitize_KeepComponents_Success(t *testing.T) {
 	opts := &openapi.SanitizeOptions{
 		KeepUnusedComponents: true,
 	}
-	err = openapi.Sanitize(ctx, inputDoc, opts)
+	result, err := openapi.Sanitize(ctx, inputDoc, opts)
 	require.NoError(t, err)
+	assert.Empty(t, result.Warnings, "Should not have warnings")
 
 	// Marshal the sanitized document to YAML
 	var buf bytes.Buffer
@@ -157,8 +161,9 @@ func TestSanitize_EmptyDocument_Success(t *testing.T) {
 	ctx := t.Context()
 
 	// Test with nil document
-	err := openapi.Sanitize(ctx, nil, nil)
+	result, err := openapi.Sanitize(ctx, nil, nil)
 	require.NoError(t, err)
+	assert.Empty(t, result.Warnings, "Should not have warnings")
 
 	// Test with minimal document (no components, no extensions)
 	doc := &openapi.OpenAPI{
@@ -169,8 +174,9 @@ func TestSanitize_EmptyDocument_Success(t *testing.T) {
 		},
 	}
 
-	err = openapi.Sanitize(ctx, doc, nil)
+	result, err = openapi.Sanitize(ctx, doc, nil)
 	require.NoError(t, err)
+	assert.Empty(t, result.Warnings, "Should not have warnings")
 }
 
 func TestSanitize_NoExtensions_Success(t *testing.T) {
@@ -188,8 +194,9 @@ func TestSanitize_NoExtensions_Success(t *testing.T) {
 	require.Empty(t, validationErrs, "Input document should be valid")
 
 	// Sanitize (should be a no-op for extensions)
-	err = openapi.Sanitize(ctx, inputDoc, nil)
+	result, err := openapi.Sanitize(ctx, inputDoc, nil)
 	require.NoError(t, err)
+	assert.Empty(t, result.Warnings, "Should not have warnings")
 
 	// Document should still be valid
 	assert.NotNil(t, inputDoc)
@@ -265,8 +272,9 @@ keepUnknownProperties: true
 	require.NoError(t, err)
 
 	// Sanitize using config
-	err = openapi.Sanitize(ctx, inputDoc, opts)
+	result, err := openapi.Sanitize(ctx, inputDoc, opts)
 	require.NoError(t, err)
+	assert.Empty(t, result.Warnings, "Should not have warnings")
 
 	// Marshal the sanitized document to YAML
 	var buf bytes.Buffer
@@ -304,8 +312,9 @@ func TestSanitize_KeepExtensionsRemoveUnknownProperties_Success(t *testing.T) {
 		KeepUnusedComponents:  true,
 	}
 
-	err = openapi.Sanitize(ctx, inputDoc, opts)
+	result, err := openapi.Sanitize(ctx, inputDoc, opts)
 	require.NoError(t, err)
+	assert.Empty(t, result.Warnings, "Should not have warnings")
 
 	// Marshal the sanitized document
 	var buf bytes.Buffer