perf(fixer): skip defensive deep copy when caller owns document (#297)

* perf(fixer): skip defensive deep copy when caller owns document

The fixer deep-copies parsed documents before mutating them, protecting
callers who reuse the input. Three code paths in the CLI never reuse the
parsed document, so the copy is wasted work:

- Fix() parses internally and owns the result — now sets MutableInput
  with defer-based save/restore for panic safety
- CLI stdin path owns its locally-parsed result
- CLI source-map path owns its locally-parsed result

Also modernizes isFixEnabled to use slices.Contains (gopls hint).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* test(fixer): add coverage for Fix() mutable input save/restore

Exercises the Fix(path) code path to cover the new MutableInput
save/restore logic, and verifies the field is restored after return.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* test(fixer): add subtest for MutableInput=true save/restore

Addresses CodeRabbit review: the false→false case would also pass if
Fix() hardcoded false instead of saving/restoring. The true→true
subtest validates the defer actually restores the original value.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* test(fixer): add parse-error subtest for Fix() MutableInput

Addresses CodeRabbit review: adds error-path subtest. Note this tests
the early-return path (parse failure at line 408, before save/restore),
not the defer — the field is untouched, not restored.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix(docs): correct dangerous deep copy advice in copilot instructions

The previous guideline recommended JSON marshal/unmarshal for deep
copying OAS documents — this silently loses interface{} type
distinctions (string vs []string in OAS 3.1 schema.Type) and drops
json:"-" fields. Updated to recommend the generated DeepCopy() methods
and document the WithMutableInput(true) opt-in for owned documents.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix(docs): replace JSON marshal deep copy advice across all docs

Audit found locations recommending JSON marshal/unmarshal for deep
copying OAS documents. This approach silently loses interface{} type
distinctions (string vs []string in OAS 3.1 schema.Type) and drops
json:"-" fields.

Updated all to recommend generated DeepCopy() methods instead.

Files fixed:
- AGENTS.md
- .claude/docs/oas-concepts.md
- .claude/agents/developer.md
- CONTRIBUTORS.md

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* docs(claude): add deep copy, make check, and docs/ generation guidance

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>

Author: erraggy

.claude/agents/developer.md

@@ -173,9 +173,7 @@ servers := []*parser.Server{
 }
 
 // Deep copy when modifying to avoid mutations
-data, _ := json.Marshal(original)
-var copy MyType
-json.Unmarshal(data, &copy)
+copy := original.DeepCopy()
 ```
 
 ## Testing Requirements

.claude/docs/oas-concepts.md

@@ -66,7 +66,7 @@ if typeStr, ok := schema.Type.(string); ok {
 1. **Assuming schema.Type is always a string** - Use type assertions and handle both string and []string cases
 2. **Creating value slices instead of pointer slices** - Check parser types and use `&Type{...}` syntax
 3. **Forgetting to track conversion issues** - Add issues for every lossy conversion or unsupported feature
-4. **Mutating source documents** - Always deep copy before modification (use JSON marshal/unmarshal)
+4. **Mutating source documents** - Always deep copy before modification using generated `DeepCopy()` methods (e.g., `doc.DeepCopy()`). Never use JSON marshal/unmarshal — it loses `interface{}` type distinctions and drops `json:"-"` fields
 5. **Not handling operation-level consumes/produces** - Check operation-level first, then fall back to document-level
 6. **Ignoring version-specific features during conversion** - Explicitly check and warn about features that don't convert
 7. **Confusing Version (string) with OASVersion (enum)** - `ParseResult` has TWO version fields:

.github/copilot-instructions.md

@@ -89,7 +89,7 @@ if typeStr, ok := schema.Type.(string); ok {
 1. **Assuming schema.Type is always a string** - Use type assertions and handle both string and []string cases
 2. **Creating value slices instead of pointer slices** - Check parser types and use `&Type{...}` syntax
 3. **Forgetting to track conversion issues** - Add issues for every lossy conversion or unsupported feature
-4. **Mutating source documents** - Always deep copy before modification (use JSON marshal/unmarshal)
+4. **Mutating source documents** - Deep copy before modification using generated `DeepCopy()` methods on parser types (e.g., `doc.DeepCopy()`). Never use JSON marshal/unmarshal for deep copying — it silently loses `interface{}` type distinctions (e.g., `string` vs `[]string` in OAS 3.1 `schema.Type`) and drops fields tagged `json:"-"`. When the caller owns the document and won't reuse it, use `WithMutableInput(true)` to skip the copy entirely.
 5. **Not handling operation-level consumes/produces** - Check operation-level first, then fall back to document-level
 6. **Ignoring version-specific features during conversion** - Explicitly check and warn about features that don't convert
 

AGENTS.md

@@ -146,7 +146,7 @@ For documentation-only changes, only items 3, 6, and 7 apply.
 1. **Type assertions:** Don't assume `schema.Type` is always a string - check both string and []string
 2. **Pointer slices:** Use `&Type{...}` for pointer semantics, not value types
 3. **Conversion issues:** Track all lossy conversions or unsupported features
-4. **Deep copy:** Never mutate source documents - use JSON marshal/unmarshal
+4. **Deep copy:** Never mutate source documents - use generated `DeepCopy()` methods (e.g., `doc.DeepCopy()`), never JSON marshal/unmarshal
 5. **Operation-level overrides:** Check operation-level fields before falling back to document-level
 6. **Version features:** Explicitly handle version-specific features during conversion
 

CLAUDE.md

@@ -44,6 +44,9 @@
 - **Use constants**: `httputil.MethodGet`, `severity.SeverityError`
 - **Always run `go_diagnostics`** after edits—hints improve perf 5-15%
 - **Favor fixing immediately** over deferring issues
+- **Deep copy**: Use generated `doc.DeepCopy()` methods, **never** JSON marshal/unmarshal (loses `interface{}` types, drops `json:"-"` fields)
+- **`make check` before pushing** — not just `go test`; catches lint, formatting, and trailing whitespace
+- **`docs/` is generated**: Files are copied by mkdocs build scripts — always edit source files at repo root
 
 ## Orchestrator Mode
 

CONTRIBUTORS.md

@@ -614,9 +614,7 @@ modified := sourceDoc
 modified.Info.Title = "New Title"  // Changes sourceDoc too!
 
 // ✅ Correct - deep copy first
-data, _ := json.Marshal(sourceDoc)
-var modified parser.OAS3Document
-json.Unmarshal(data, &modified)
+modified := sourceDoc.DeepCopy()
 modified.Info.Title = "New Title"  // Safe
 ```
 

cmd/oastools/commands/fix.go

@@ -230,6 +230,7 @@ func HandleFix(args []string) error {
 		f.GenericNamingConfig = genericConfig
 		f.OperationIdNamingConfig = operationIdConfig
 		f.DryRun = flags.DryRun
+		f.MutableInput = true
 		if flags.StubResponseDesc != "" {
 			f.StubConfig.ResponseDescription = flags.StubResponseDesc
 		}
@@ -264,6 +265,7 @@ func HandleFix(args []string) error {
 				fixer.WithGenericNamingConfig(genericConfig),
 				fixer.WithOperationIdNamingConfig(operationIdConfig),
 				fixer.WithDryRun(flags.DryRun),
+				fixer.WithMutableInput(true),
 			}
 			if parseResult.SourceMap != nil {
 				fixOpts = append(fixOpts, fixer.WithSourceMap(parseResult.SourceMap))

fixer/fixer.go

@@ -2,6 +2,7 @@ package fixer
 
 import (
 	"fmt"
+	"slices"
 
 	"github.com/erraggy/oastools/parser"
 )
@@ -407,6 +408,10 @@ func (f *Fixer) Fix(specPath string) (*FixResult, error) {
 		return nil, fmt.Errorf("fixer: failed to parse specification: %w", err)
 	}
 
+	// Fix() owns the parsed document, so always mutate in place.
+	origMutable := f.MutableInput
+	f.MutableInput = true
+	defer func() { f.MutableInput = origMutable }()
 	return f.FixParsed(*parseResult)
 }
 
@@ -445,12 +450,7 @@ func (f *Fixer) isFixEnabled(fixType FixType) bool {
 	if len(f.EnabledFixes) == 0 {
 		return true // if explicitly set to empty/nil, enable all fixes
 	}
-	for _, ft := range f.EnabledFixes {
-		if ft == fixType {
-			return true
-		}
-	}
-	return false
+	return slices.Contains(f.EnabledFixes, fixType)
 }
 
 // populateFixLocation fills in Line/Column/File from the SourceMap if available.

fixer/fixer_test.go

@@ -601,3 +601,36 @@ paths:
 	assert.Greater(t, len(pathItem.Get.Parameters), originalParamCount,
 		"Original document should be mutated when Fixer.MutableInput is true")
 }
+
+// TestFix_SetsInternalMutableInput verifies that Fix() internally sets
+// MutableInput to skip the defensive copy and restores it afterward.
+func TestFix_SetsInternalMutableInput(t *testing.T) {
+	t.Run("restores false to false", func(t *testing.T) {
+		f := New()
+		assert.False(t, f.MutableInput, "default should be false")
+
+		result, err := f.Fix("../testdata/bench/small-oas3.yaml")
+		require.NoError(t, err)
+		assert.True(t, result.Success)
+		assert.False(t, f.MutableInput, "MutableInput should be restored after Fix()")
+	})
+
+	t.Run("restores true to true", func(t *testing.T) {
+		f := New()
+		f.MutableInput = true
+
+		result, err := f.Fix("../testdata/bench/small-oas3.yaml")
+		require.NoError(t, err)
+		assert.True(t, result.Success)
+		assert.True(t, f.MutableInput, "MutableInput should be restored to original true value")
+	})
+
+	t.Run("untouched on parse error", func(t *testing.T) {
+		f := New()
+		f.MutableInput = true
+
+		_, err := f.Fix("does-not-exist.yaml")
+		require.Error(t, err)
+		assert.True(t, f.MutableInput, "MutableInput should be untouched when Fix() fails before save/restore")
+	})
+}