From 2449c25d940ce6459479d57fe897fe1d883e9acc Mon Sep 17 00:00:00 2001 From: Raphael Fakhri <153192858+RaphaelFakhri@users.noreply.github.com> Date: Sat, 24 Oct 2026 15:48:28 +0000 Subject: [PATCH 1/2] fix(linter): keep node-level lint fixes when the fixed document is written --- linter/fix/engine.go | 85 +++++++++++++++++++++++---- linter/fix/engine_test.go | 117 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 191 insertions(+), 11 deletions(-) diff --git a/linter/fix/engine.go b/linter/fix/engine.go index e4610430..a67f9d87 100644 --- a/linter/fix/engine.go +++ b/linter/fix/engine.go @@ -3,8 +3,10 @@ package fix import ( "context" "errors" + "fmt" "sort" + "github.com/speakeasy-api/openapi/marshaller" "github.com/speakeasy-api/openapi/openapi" "github.com/speakeasy-api/openapi/validation" "go.yaml.in/yaml/v3" @@ -158,7 +160,9 @@ func (e *Engine) ProcessErrors(ctx context.Context, doc *openapi.OpenAPI, errs [ result := &Result{} modified := make(map[conflictKey]bool) skippedRules := make(map[string]bool) + var nodeFixes []pendingNodeFix +fixLoop: for _, fe := range fixable { fix := fe.fix vErr := fe.vErr @@ -225,7 +229,7 @@ func (e *Engine) ProcessErrors(ctx context.Context, doc *openapi.OpenAPI, errs [ continue } if errors.Is(err, validation.ErrExitInteractive) { - return result, nil + break fixLoop } result.Failed = append(result.Failed, FailedFix{ Error: vErr, Fix: fix, FixError: err, @@ -241,19 +245,20 @@ func (e *Engine) ProcessErrors(ctx context.Context, doc *openapi.OpenAPI, errs [ } } - // Apply the fix - var applyErr error - if nodeFix, ok := fix.(validation.NodeFix); ok { - rootNode := doc.GetRootNode() - if rootNode != nil { - applyErr = nodeFix.ApplyNode(rootNode) - } else { - applyErr = fix.Apply(doc) + // Node fixes are deferred until all model fixes have been applied, as + // marshalling syncs the model back over the YAML nodes. + if nodeFix, ok := fix.(validation.NodeFix); ok && doc.GetRootNode() != nil { + if key.Line >= 0 { + modified[key] = true } - } else { - applyErr = fix.Apply(doc) + nodeFixes = append(nodeFixes, pendingNodeFix{vErr: vErr, fix: nodeFix, index: len(result.Applied)}) + result.Applied = append(result.Applied, makeAppliedFix(vErr, fix)) + continue } + // Apply the fix + applyErr := fix.Apply(doc) + if applyErr != nil { result.Failed = append(result.Failed, FailedFix{ Error: vErr, Fix: fix, FixError: applyErr, @@ -269,9 +274,67 @@ func (e *Engine) ProcessErrors(ctx context.Context, doc *openapi.OpenAPI, errs [ result.Applied = append(result.Applied, makeAppliedFix(vErr, fix)) } + if err := applyNodeFixes(ctx, doc, result, nodeFixes); err != nil { + return result, err + } + return result, nil } +type pendingNodeFix struct { + vErr *validation.Error + fix validation.NodeFix + index int // position of the fix in Result.Applied +} + +// applyNodeFixes applies fixes that edit the YAML node tree directly. The model +// is synced to the nodes first so earlier model fixes are kept, and the model is +// rebuilt from the edited nodes afterwards so the edits survive marshalling. +func applyNodeFixes(ctx context.Context, doc *openapi.OpenAPI, result *Result, nodeFixes []pendingNodeFix) error { + if len(nodeFixes) == 0 { + return nil + } + + rootNode, err := marshaller.Sync(ctx, doc) + if err != nil { + return fmt.Errorf("failed to sync document before applying node fixes: %w", err) + } + + failed := make(map[int]bool) + for _, nf := range nodeFixes { + if err := nf.fix.ApplyNode(rootNode); err != nil { + failed[nf.index] = true + result.Failed = append(result.Failed, FailedFix{ + Error: nf.vErr, Fix: nf.fix, FixError: err, + }) + } + } + + if len(failed) > 0 { + applied := make([]AppliedFix, 0, len(result.Applied)-len(failed)) + for i, af := range result.Applied { + if !failed[i] { + applied = append(applied, af) + } + } + result.Applied = applied + } + + if len(failed) == len(nodeFixes) { + return nil + } + + config := doc.GetCore().GetConfig() + *doc = openapi.OpenAPI{} + doc.InitCache() + doc.GetCore().SetConfig(config) + if _, err := marshaller.UnmarshalNode(ctx, "", rootNode, doc); err != nil { + return fmt.Errorf("failed to rebuild document after applying node fixes: %w", err) + } + + return nil +} + func makeAppliedFix(vErr *validation.Error, fix validation.Fix) AppliedFix { af := AppliedFix{Error: vErr, Fix: fix} if cd, ok := fix.(validation.ChangeDescriber); ok { diff --git a/linter/fix/engine_test.go b/linter/fix/engine_test.go index 17181a3e..383af6a0 100644 --- a/linter/fix/engine_test.go +++ b/linter/fix/engine_test.go @@ -1,7 +1,9 @@ package fix_test import ( + "bytes" "errors" + "strings" "testing" "github.com/speakeasy-api/openapi/linter/fix" @@ -612,3 +614,118 @@ func TestEngine_SortsByLocation(t *testing.T) { } assert.Equal(t, []string{"fix-line2", "fix-line5", "fix-line10"}, order, "fixes should be applied in location order") } + +// setNodeValueFix is a node fix that replaces the value of a YAML node. +type setNodeValueFix struct { + mockFix + node *yaml.Node + value string +} + +func (f *setNodeValueFix) ApplyNode(_ *yaml.Node) error { + f.node.Value = f.value + return nil +} + +// modelFix is a fix that edits the document model. +type modelFix struct { + mockFix + apply func(doc *openapi.OpenAPI) +} + +func (f *modelFix) Apply(doc any) error { + f.apply(doc.(*openapi.OpenAPI)) + return nil +} + +func TestEngine_NodeFix_SurvivesMarshal(t *testing.T) { + t.Parallel() + ctx := t.Context() + + doc, _, err := openapi.Unmarshal(ctx, strings.NewReader(`openapi: 3.1.0 +info: + title: Test + version: 1.0.0 +servers: + - url: https://api.example.com/ +paths: + /pets/: + get: + responses: + "200": + description: ok +`)) + require.NoError(t, err, "unmarshal should succeed") + + urlNode := doc.Servers[0].GetCore().URL.ValueNode + pathKeyNode := doc.Paths.GetCore().GetMapKeyNodeOrRoot("/pets/", doc.Paths.GetRootNode()) + + engine := fix.NewEngine(fix.Options{Mode: fix.ModeAuto}, nil, nil) + result, err := engine.ProcessErrors(ctx, doc, []error{ + makeError("version-rule", 4, 12, "version", &modelFix{ + mockFix: mockFix{description: "bump version"}, + apply: func(doc *openapi.OpenAPI) { doc.Info.Version = "2.0.0" }, + }), + makeError("url-rule", 6, 10, "url", &setNodeValueFix{ + mockFix: mockFix{description: "trim url"}, + node: urlNode, + value: "https://api.example.com", + }), + makeError("path-rule", 8, 3, "path", &setNodeValueFix{ + mockFix: mockFix{description: "trim path"}, + node: pathKeyNode, + value: "/pets", + }), + }) + require.NoError(t, err, "ProcessErrors should not fail") + assert.Len(t, result.Applied, 3, "should apply all fixes") + + var buf bytes.Buffer + require.NoError(t, openapi.Marshal(ctx, doc, &buf), "marshal should succeed") + assert.Equal(t, `openapi: 3.1.0 +info: + title: Test + version: 2.0.0 +servers: + - url: https://api.example.com +paths: + /pets: + get: + responses: + "200": + description: ok +`, buf.String(), "marshalled document should keep both model and node fixes") +} + +// failingNodeFix is a node fix that always fails. +type failingNodeFix struct { + mockFix +} + +func (f *failingNodeFix) ApplyNode(_ *yaml.Node) error { + return errors.New("node fix failed") +} + +func TestEngine_NodeFix_FailureIsReported(t *testing.T) { + t.Parallel() + ctx := t.Context() + + doc, _, err := openapi.Unmarshal(ctx, strings.NewReader(`openapi: 3.1.0 +info: + title: Test + version: 1.0.0 +paths: {} +`)) + require.NoError(t, err, "unmarshal should succeed") + + engine := fix.NewEngine(fix.Options{Mode: fix.ModeAuto}, nil, nil) + result, err := engine.ProcessErrors(ctx, doc, []error{ + makeError("rule-a", 1, 1, "a", &failingNodeFix{mockFix: mockFix{description: "failing node fix"}}), + makeError("rule-b", 2, 1, "b", &mockFix{description: "model fix"}), + }) + require.NoError(t, err, "ProcessErrors should not fail") + require.Len(t, result.Applied, 1, "should apply only the model fix") + assert.Equal(t, "model fix", result.Applied[0].Fix.Description(), "applied fix should be the model fix") + require.Len(t, result.Failed, 1, "should report the failed node fix") + assert.EqualError(t, result.Failed[0].FixError, "node fix failed", "failure should carry the node fix error") +} From 26442d41c8e74142d7cf7a149f88b1f54004f12f Mon Sep 17 00:00:00 2001 From: Raphael Fakhri <153192858+RaphaelFakhri@users.noreply.github.com> Date: Thu, 29 Oct 2026 12:47:31 +0000 Subject: [PATCH 2/2] fix(linter): keep document comments and retry node fixes after a failure Rebuild the model from the document node after applying node fixes, so document-level comments survive marshalling. Mark a node fix's location as modified only once the fix applies, so a failed node fix no longer blocks a later fix for the same rule and location. --- linter/fix/engine.go | 52 ++++++++++++----- linter/fix/engine_test.go | 115 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 154 insertions(+), 13 deletions(-) diff --git a/linter/fix/engine.go b/linter/fix/engine.go index a67f9d87..d1215fb6 100644 --- a/linter/fix/engine.go +++ b/linter/fix/engine.go @@ -247,11 +247,9 @@ fixLoop: // Node fixes are deferred until all model fixes have been applied, as // marshalling syncs the model back over the YAML nodes. + // Their locations are only marked as modified once they apply. if nodeFix, ok := fix.(validation.NodeFix); ok && doc.GetRootNode() != nil { - if key.Line >= 0 { - modified[key] = true - } - nodeFixes = append(nodeFixes, pendingNodeFix{vErr: vErr, fix: nodeFix, index: len(result.Applied)}) + nodeFixes = append(nodeFixes, pendingNodeFix{vErr: vErr, fix: nodeFix, key: key, index: len(result.Applied)}) result.Applied = append(result.Applied, makeAppliedFix(vErr, fix)) continue } @@ -274,7 +272,7 @@ fixLoop: result.Applied = append(result.Applied, makeAppliedFix(vErr, fix)) } - if err := applyNodeFixes(ctx, doc, result, nodeFixes); err != nil { + if err := applyNodeFixes(ctx, doc, result, nodeFixes, modified); err != nil { return result, err } @@ -284,13 +282,16 @@ fixLoop: type pendingNodeFix struct { vErr *validation.Error fix validation.NodeFix + key conflictKey index int // position of the fix in Result.Applied } // applyNodeFixes applies fixes that edit the YAML node tree directly. The model // is synced to the nodes first so earlier model fixes are kept, and the model is // rebuilt from the edited nodes afterwards so the edits survive marshalling. -func applyNodeFixes(ctx context.Context, doc *openapi.OpenAPI, result *Result, nodeFixes []pendingNodeFix) error { +// A pending fix is skipped as a conflict only if an earlier fix for the same rule +// and location applied, so a failed node fix does not block the ones after it. +func applyNodeFixes(ctx context.Context, doc *openapi.OpenAPI, result *Result, nodeFixes []pendingNodeFix, modified map[conflictKey]bool) error { if len(nodeFixes) == 0 { return nil } @@ -299,36 +300,61 @@ func applyNodeFixes(ctx context.Context, doc *openapi.OpenAPI, result *Result, n if err != nil { return fmt.Errorf("failed to sync document before applying node fixes: %w", err) } + if rootNode == nil { + return errors.New("failed to sync document before applying node fixes: no root node") + } - failed := make(map[int]bool) + notApplied := make(map[int]bool) for _, nf := range nodeFixes { + if nf.key.Line >= 0 && modified[nf.key] { + notApplied[nf.index] = true + result.Skipped = append(result.Skipped, SkippedFix{ + Error: nf.vErr, + Fix: nf.fix, + Reason: SkipConflict, + }) + continue + } + if err := nf.fix.ApplyNode(rootNode); err != nil { - failed[nf.index] = true + notApplied[nf.index] = true result.Failed = append(result.Failed, FailedFix{ Error: nf.vErr, Fix: nf.fix, FixError: err, }) + continue + } + + if nf.key.Line >= 0 { + modified[nf.key] = true } } - if len(failed) > 0 { - applied := make([]AppliedFix, 0, len(result.Applied)-len(failed)) + if len(notApplied) > 0 { + applied := make([]AppliedFix, 0, len(result.Applied)-len(notApplied)) for i, af := range result.Applied { - if !failed[i] { + if !notApplied[i] { applied = append(applied, af) } } result.Applied = applied } - if len(failed) == len(nodeFixes) { + if len(notApplied) == len(nodeFixes) { return nil } + // Rebuild from the document node, if there is one, so its comments are kept. + node := rootNode + if documentNode := doc.GetCore().DocumentNode; documentNode != nil && documentNode.Kind == yaml.DocumentNode { + documentNode.Content = []*yaml.Node{rootNode} + node = documentNode + } + config := doc.GetCore().GetConfig() *doc = openapi.OpenAPI{} doc.InitCache() doc.GetCore().SetConfig(config) - if _, err := marshaller.UnmarshalNode(ctx, "", rootNode, doc); err != nil { + if _, err := marshaller.UnmarshalNode(ctx, "", node, doc); err != nil { return fmt.Errorf("failed to rebuild document after applying node fixes: %w", err) } diff --git a/linter/fix/engine_test.go b/linter/fix/engine_test.go index 383af6a0..9f9dcb4d 100644 --- a/linter/fix/engine_test.go +++ b/linter/fix/engine_test.go @@ -729,3 +729,118 @@ paths: {} require.Len(t, result.Failed, 1, "should report the failed node fix") assert.EqualError(t, result.Failed[0].FixError, "node fix failed", "failure should carry the node fix error") } + +func TestEngine_NodeFix_KeepsDocumentComments(t *testing.T) { + t.Parallel() + ctx := t.Context() + + doc, _, err := openapi.Unmarshal(ctx, strings.NewReader(`# Pets API +# Maintained by the platform team. + +openapi: 3.1.0 +info: + title: Test + version: 1.0.0 +servers: + - url: https://api.example.com/ +paths: {} +`)) + require.NoError(t, err, "unmarshal should succeed") + + urlNode := doc.Servers[0].GetCore().URL.ValueNode + + engine := fix.NewEngine(fix.Options{Mode: fix.ModeAuto}, nil, nil) + result, err := engine.ProcessErrors(ctx, doc, []error{ + makeError("url-rule", 9, 10, "url", &setNodeValueFix{ + mockFix: mockFix{description: "trim url"}, + node: urlNode, + value: "https://api.example.com", + }), + }) + require.NoError(t, err, "ProcessErrors should not fail") + require.Len(t, result.Applied, 1, "should apply the node fix") + + var buf bytes.Buffer + require.NoError(t, openapi.Marshal(ctx, doc, &buf), "marshal should succeed") + assert.Equal(t, `# Pets API +# Maintained by the platform team. + +openapi: 3.1.0 +info: + title: Test + version: 1.0.0 +servers: + - url: https://api.example.com +paths: {} +`, buf.String(), "marshalled document should keep the document comments") +} + +func TestEngine_NodeFix_FailedFixDoesNotBlockSameLocation(t *testing.T) { + t.Parallel() + ctx := t.Context() + + doc, _, err := openapi.Unmarshal(ctx, strings.NewReader(`openapi: 3.1.0 +info: + title: Test + version: 1.0.0 +servers: + - url: https://api.example.com/ +paths: {} +`)) + require.NoError(t, err, "unmarshal should succeed") + + urlNode := doc.Servers[0].GetCore().URL.ValueNode + + engine := fix.NewEngine(fix.Options{Mode: fix.ModeAuto}, nil, nil) + result, err := engine.ProcessErrors(ctx, doc, []error{ + makeError("url-rule", 6, 10, "url", &failingNodeFix{mockFix: mockFix{description: "failing node fix"}}), + makeError("url-rule", 6, 10, "url", &setNodeValueFix{ + mockFix: mockFix{description: "trim url"}, + node: urlNode, + value: "https://api.example.com", + }), + }) + require.NoError(t, err, "ProcessErrors should not fail") + require.Len(t, result.Failed, 1, "should report the failed node fix") + require.Len(t, result.Applied, 1, "should apply the fix after the failed one") + assert.Equal(t, "trim url", result.Applied[0].Fix.Description(), "applied fix should be the second node fix") + assert.Empty(t, result.Skipped, "the failed fix should not cause a conflict") + assert.Equal(t, "https://api.example.com", doc.Servers[0].URL, "document should contain the applied fix") +} + +func TestEngine_NodeFix_SkipsConflictAfterAppliedFix(t *testing.T) { + t.Parallel() + ctx := t.Context() + + doc, _, err := openapi.Unmarshal(ctx, strings.NewReader(`openapi: 3.1.0 +info: + title: Test + version: 1.0.0 +servers: + - url: https://api.example.com/ +paths: {} +`)) + require.NoError(t, err, "unmarshal should succeed") + + urlNode := doc.Servers[0].GetCore().URL.ValueNode + + engine := fix.NewEngine(fix.Options{Mode: fix.ModeAuto}, nil, nil) + result, err := engine.ProcessErrors(ctx, doc, []error{ + makeError("url-rule", 6, 10, "url", &setNodeValueFix{ + mockFix: mockFix{description: "trim url"}, + node: urlNode, + value: "https://api.example.com", + }), + makeError("url-rule", 6, 10, "url", &setNodeValueFix{ + mockFix: mockFix{description: "replace url"}, + node: urlNode, + value: "https://other.example.com", + }), + }) + require.NoError(t, err, "ProcessErrors should not fail") + require.Len(t, result.Applied, 1, "should apply only the first node fix") + assert.Equal(t, "trim url", result.Applied[0].Fix.Description(), "applied fix should be the first node fix") + require.Len(t, result.Skipped, 1, "should skip the second node fix") + assert.Equal(t, fix.SkipConflict, result.Skipped[0].Reason, "second node fix should be skipped as a conflict") + assert.Equal(t, "https://api.example.com", doc.Servers[0].URL, "document should contain only the first fix") +}