fix: address PR review feedback and add missing test coverage

Author: TristanSpeakEasy

cmd/openapi/commands/cmdutil/cmdutil.go

@@ -26,8 +26,8 @@ func StdinIsPiped() bool {
 	return (fi.Mode() & os.ModeCharDevice) == 0
 }
 
-// InputFileFromArgs returns the input file from args, or "-" if stdin should
-// be used. It extracts the first positional arg or detects piped stdin.
+// InputFileFromArgs returns the first positional arg as the input file path,
+// or the stdin indicator "-" when no args are provided.
 func InputFileFromArgs(args []string) string {
 	if len(args) > 0 {
 		return args[0]
@@ -54,7 +54,7 @@ func StdinOrFileArgs(minArgs, maxArgs int) cobra.PositionalArgs {
 
 // ArgAt returns args[index] if it exists, or defaultVal otherwise.
 func ArgAt(args []string, index int, defaultVal string) string {
-	if index < len(args) {
+	if index >= 0 && index < len(args) {
 		return args[index]
 	}
 	return defaultVal

cmd/openapi/commands/cmdutil/cmdutil_test.go

@@ -68,6 +68,8 @@ func TestArgAt(t *testing.T) {
 		{name: "returns default for empty args", args: []string{}, index: 0, defaultVal: "default", expected: "default"},
 		{name: "returns default for nil args", args: nil, index: 0, defaultVal: "default", expected: "default"},
 		{name: "returns empty default", args: []string{}, index: 0, defaultVal: "", expected: ""},
+		{name: "returns default for negative index", args: []string{"a", "b"}, index: -1, defaultVal: "default", expected: "default"},
+		{name: "returns default for large negative index", args: []string{"a"}, index: -100, defaultVal: "fallback", expected: "fallback"},
 	}
 
 	for _, tt := range tests {
@@ -101,4 +103,27 @@ func TestStdinOrFileArgs(t *testing.T) {
 		assert.Error(t, err)
 		assert.Contains(t, err.Error(), "accepts at most 2 arg(s)")
 	})
+
+	t.Run("unbounded max accepts many args", func(t *testing.T) {
+		t.Parallel()
+		unbounded := StdinOrFileArgs(1, -1)
+		err := unbounded(nil, []string{"a", "b", "c", "d", "e"})
+		assert.NoError(t, err, "negative maxArgs should allow unlimited args")
+	})
+
+	t.Run("zero minArgs accepts any arg count", func(t *testing.T) {
+		t.Parallel()
+		noMin := StdinOrFileArgs(0, 2)
+		err := noMin(nil, []string{})
+		assert.NoError(t, err, "zero minArgs should accept empty args")
+	})
+
+	t.Run("error message includes min arg count", func(t *testing.T) {
+		t.Parallel()
+		min3 := StdinOrFileArgs(3, 5)
+		err := min3(nil, []string{"a", "b"})
+		if err != nil {
+			assert.Contains(t, err.Error(), "requires at least 3 arg(s)")
+		}
+	})
 }

cmd/openapi/commands/openapi/shared_test.go

@@ -1,6 +1,8 @@
 package openapi
 
 import (
+	"bytes"
+	"os"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -140,6 +142,49 @@ func TestOutputFileFromArgs(t *testing.T) {
 	}
 }
 
+func TestOpenAPIProcessor_IOAccessors(t *testing.T) {
+	t.Parallel()
+
+	t.Run("stdin falls back to os.Stdin when nil", func(t *testing.T) {
+		t.Parallel()
+		p := &OpenAPIProcessor{}
+		assert.Equal(t, os.Stdin, p.stdin(), "nil Stdin should fall back to os.Stdin")
+	})
+
+	t.Run("stdout falls back to os.Stdout when nil", func(t *testing.T) {
+		t.Parallel()
+		p := &OpenAPIProcessor{}
+		assert.Equal(t, os.Stdout, p.stdout(), "nil Stdout should fall back to os.Stdout")
+	})
+
+	t.Run("stderr falls back to os.Stderr when nil", func(t *testing.T) {
+		t.Parallel()
+		p := &OpenAPIProcessor{}
+		assert.Equal(t, os.Stderr, p.stderr(), "nil Stderr should fall back to os.Stderr")
+	})
+
+	t.Run("stdin uses override when set", func(t *testing.T) {
+		t.Parallel()
+		custom := &bytes.Buffer{}
+		p := &OpenAPIProcessor{Stdin: custom}
+		assert.Equal(t, custom, p.stdin(), "should use custom Stdin")
+	})
+
+	t.Run("stdout uses override when set", func(t *testing.T) {
+		t.Parallel()
+		custom := &bytes.Buffer{}
+		p := &OpenAPIProcessor{Stdout: custom}
+		assert.Equal(t, custom, p.stdout(), "should use custom Stdout")
+	})
+
+	t.Run("stderr uses override when set", func(t *testing.T) {
+		t.Parallel()
+		custom := &bytes.Buffer{}
+		p := &OpenAPIProcessor{Stderr: custom}
+		assert.Equal(t, custom, p.stderr(), "should use custom Stderr")
+	})
+}
+
 func TestStdinOrFileArgs(t *testing.T) {
 	t.Parallel()
 

internal/utils/string_builder_test.go

@@ -1,11 +1,70 @@
 package utils
 
 import (
+	"fmt"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
 )
 
+func TestAnyToString_Success(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name     string
+		input    any
+		expected string
+	}{
+		{name: "string value", input: "hello", expected: "hello"},
+		{name: "empty string", input: "", expected: ""},
+		{name: "int value", input: 42, expected: "42"},
+		{name: "int zero", input: 0, expected: "0"},
+		{name: "int negative", input: -7, expected: "-7"},
+		{name: "int64 value", input: int64(9999999999), expected: "9999999999"},
+		{name: "int32 value", input: int32(123), expected: "123"},
+		{name: "float64 integer", input: float64(3), expected: "3"},
+		{name: "float64 decimal", input: 3.14, expected: "3.14"},
+		{name: "float64 large", input: 1e18, expected: "1E+18"},
+		{name: "float64 small", input: 0.000123, expected: "0.000123"},
+		{name: "float32 value", input: float32(2.5), expected: "2.5"},
+		{name: "bool true", input: true, expected: "true"},
+		{name: "bool false", input: false, expected: "false"},
+		{name: "uint64 value", input: uint64(100), expected: "100"},
+		{name: "uint value", input: uint(50), expected: "50"},
+		{name: "fallback type", input: []int{1, 2}, expected: fmt.Sprintf("%v", []int{1, 2})},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+			assert.Equal(t, tt.expected, AnyToString(tt.input), "AnyToString should match expected output")
+		})
+	}
+}
+
+// TestAnyToString_MatchesFmtSprintf verifies that AnyToString matches
+// fmt.Sprintf("%v") for common types. Note: float scientific notation uses
+// uppercase 'E' (from 'G' format) vs fmt.Sprintf's lowercase 'e', so floats
+// that produce scientific notation are excluded from this exact-match test.
+func TestAnyToString_MatchesFmtSprintf(t *testing.T) {
+	t.Parallel()
+
+	values := []any{
+		"text", 42, int64(100), int32(10),
+		3.14, float64(0.5),
+		float32(1.5), true, false,
+		uint64(99), uint(7),
+	}
+
+	for _, v := range values {
+		t.Run(fmt.Sprintf("%T(%v)", v, v), func(t *testing.T) {
+			t.Parallel()
+			assert.Equal(t, fmt.Sprintf("%v", v), AnyToString(v),
+				"AnyToString should match fmt.Sprintf for %T", v)
+		})
+	}
+}
+
 func TestBuildAbsoluteReference(t *testing.T) {
 	t.Parallel()
 

openapi/reference.go

@@ -685,14 +685,24 @@ func unmarshaler[T any, V interfaces.Validator[T], C marshaller.CoreModeler](_ *
 	}
 }
 
+// referenceInitGlobalMutex serializes initialization of per-Reference mutexes
+// to avoid data races on r.initMutex when ensureMutex is called concurrently.
+var referenceInitGlobalMutex sync.Mutex
+
 // ensureMutex initializes the mutex if it's nil (lazy initialization).
 // Uses sync.Once (pointer) to guarantee thread-safe single initialization
 // while keeping Reference safe to copy before first use.
 func (r *Reference[T, V, C]) ensureMutex() {
+	// Guard lazy initialization of r.initMutex with a global mutex to avoid
+	// concurrent read/write races on the pointer field.
+	referenceInitGlobalMutex.Lock()
 	if r.initMutex == nil {
 		r.initMutex = &sync.Once{}
 	}
-	r.initMutex.Do(func() {
+	initOnce := r.initMutex
+	referenceInitGlobalMutex.Unlock()
+
+	initOnce.Do(func() {
 		r.cacheMutex = &sync.RWMutex{}
 	})
 }

openapi/reference_mutex_test.go

@@ -0,0 +1,57 @@
+package openapi
+
+import (
+	"sync"
+	"testing"
+
+	"github.com/speakeasy-api/openapi/openapi/core"
+	"github.com/stretchr/testify/assert"
+)
+
+// TestEnsureMutex_ConcurrentAccess verifies that ensureMutex is safe to call
+// concurrently from multiple goroutines on a Reference with nil initMutex.
+func TestEnsureMutex_ConcurrentAccess(t *testing.T) {
+	t.Parallel()
+
+	ref := &Reference[PathItem, *PathItem, *core.PathItem]{}
+
+	const goroutines = 100
+	var wg sync.WaitGroup
+	wg.Add(goroutines)
+
+	for range goroutines {
+		go func() {
+			defer wg.Done()
+			ref.ensureMutex()
+		}()
+	}
+
+	wg.Wait()
+
+	assert.NotNil(t, ref.initMutex, "initMutex should be initialized")
+	assert.NotNil(t, ref.cacheMutex, "cacheMutex should be initialized")
+}
+
+// TestEnsureMutex_CopiedReference verifies that a copied Reference
+// with nil mutexes can safely initialize its own mutexes independently.
+func TestEnsureMutex_CopiedReference(t *testing.T) {
+	t.Parallel()
+
+	original := &Reference[PathItem, *PathItem, *core.PathItem]{}
+	original.ensureMutex()
+
+	// Simulate a copy by creating a new Reference with nil mutexes
+	copied := &Reference[PathItem, *PathItem, *core.PathItem]{}
+
+	assert.Nil(t, copied.initMutex, "copied reference should have nil initMutex")
+	assert.Nil(t, copied.cacheMutex, "copied reference should have nil cacheMutex")
+
+	copied.ensureMutex()
+
+	assert.NotNil(t, copied.initMutex, "copied reference should initialize its own initMutex")
+	assert.NotNil(t, copied.cacheMutex, "copied reference should initialize its own cacheMutex")
+
+	// Original should be unaffected
+	assert.NotNil(t, original.initMutex, "original initMutex should still be set")
+	assert.NotNil(t, original.cacheMutex, "original cacheMutex should still be set")
+}

validation/utils_test.go

@@ -440,6 +440,63 @@ func TestSortValidationErrors_EdgeCases_Success(t *testing.T) {
 		assert.NotErrorAs(t, errors[6], &notValidation, "index 6 should be regular error")
 	})
 
+	t.Run("validation errors with nil underlying errors", func(t *testing.T) {
+		t.Parallel()
+
+		errs := []error{
+			&Error{
+				UnderlyingError: nil,
+				Node:            &yaml.Node{Line: 5, Column: 10},
+			},
+			&Error{
+				UnderlyingError: errors.New("has message"),
+				Node:            &yaml.Node{Line: 5, Column: 10},
+			},
+			&Error{
+				UnderlyingError: nil,
+				Node:            &yaml.Node{Line: 5, Column: 10},
+			},
+		}
+
+		assert.NotPanics(t, func() {
+			SortValidationErrors(errs)
+		}, "sorting with nil UnderlyingError should not panic")
+
+		// Error with message should sort after nil-message errors (empty string < "has message")
+		var err0, err1, err2 *Error
+		require.ErrorAs(t, errs[0], &err0)
+		require.ErrorAs(t, errs[1], &err1)
+		require.ErrorAs(t, errs[2], &err2)
+		assert.NoError(t, err0.UnderlyingError, "nil error should sort first")
+		assert.NoError(t, err1.UnderlyingError, "nil error should sort first")
+		assert.Error(t, err2.UnderlyingError, "non-nil error should sort last")
+	})
+
+	t.Run("one nil and one non-nil underlying error", func(t *testing.T) {
+		t.Parallel()
+
+		errs := []error{
+			&Error{
+				UnderlyingError: errors.New("error B"),
+				Node:            &yaml.Node{Line: 1, Column: 1},
+			},
+			&Error{
+				UnderlyingError: nil,
+				Node:            &yaml.Node{Line: 1, Column: 1},
+			},
+		}
+
+		assert.NotPanics(t, func() {
+			SortValidationErrors(errs)
+		}, "sorting with one nil UnderlyingError should not panic")
+
+		var err0, err1 *Error
+		require.ErrorAs(t, errs[0], &err0)
+		require.ErrorAs(t, errs[1], &err1)
+		require.NoError(t, err0.UnderlyingError, "nil underlying error sorts before non-nil")
+		require.Error(t, err1.UnderlyingError)
+	})
+
 	t.Run("validation errors first then regular errors forces bIsValidationErr", func(t *testing.T) {
 		t.Parallel()