Skip to content

fix(linter): keep node-level lint fixes when the fixed document is written - #281

Open
RaphaelFakhri wants to merge 1 commit into
speakeasy-api:mainfrom
RaphaelFakhri:fix/lint-node-fixes-lost
Open

RaphaelFakhri wants to merge 1 commit into
speakeasy-api:mainfrom
RaphaelFakhri:fix/lint-node-fixes-lost

Conversation

@RaphaelFakhri

Copy link
Copy Markdown

Summary

openapi spec lint --fix loses fixes that implement validation.NodeFix, because marshaller.Marshal syncs the unchanged model back over the edited YAML nodes. For style-path-trailing-slash, the written document contains both the fixed path and the original path.

This change updates fix.Engine.ProcessErrors as follows:

  • Model fixes run first, in the same location order as before.
  • Node fixes are deferred. The engine syncs the model to the node tree with marshaller.Sync, which keeps the model edits, and then applies the node fixes to that tree.
  • The engine rebuilds the document model from the edited nodes and keeps the original document config. The edits then survive Marshal, and the input format (YAML or JSON) and indentation are preserved.
  • A node fix that fails moves from Applied to Failed. Exiting interactive mode still applies the fixes that the user already accepted.

Testing

  • TestEngine_NodeFix_SurvivesMarshal unmarshals a real document, applies one model fix and two node fixes (a value edit and a path key rename), marshals the document, and checks the exact output. It fails on main: the URL edit is reverted and /pets/ is written next to /pets.
  • TestEngine_NodeFix_FailureIsReported covers a failing deferred node fix.
  • go test ./linter/... ./openapi/linter/... ./validation/... and go test ./... in cmd/openapi pass, and golangci-lint run ./linter/... reports 0 issues.
  • Manual check: openapi spec lint --fix on the document from the issue now writes url: https://api.example.com, a single /pets path, and enum: [a, b], for both YAML and JSON input.

Fixes #280

@RaphaelFakhri
RaphaelFakhri requested a review from a team as a code owner October 24, 2026 15:53

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 issues found across 2 files

Confidence score: 3/5

  • In linter/fix/engine.go, a failed deferred node fix can reserve the conflict key and block later same-rule fixes at that location. Reserve the key only after the fix applies successfully.
  • In linter/fix/engine.go, rebuilding the model can drop document-level comments from node-fixed YAML files when they’re written. Preserve and reattach the original document node, or rebuild from a document wrapper.
Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="linter/fix/engine.go">

<violation number="1" location="linter/fix/engine.go:252">
P2: A deferred node fix reserves its conflict key before it runs, so a failed node fix permanently blocks later same-rule fixes at that location. Reserve the key only after successful application, while preserving conflict handling among pending fixes.</violation>

<violation number="2" location="linter/fix/engine.go:328">
P2: Rebuilding the model discards the original `DocumentNode`, so node-fixed YAML files lose document-level comments when written. Preserve and reattach the original document node, or rebuild from a document wrapper instead of passing only the mapping root.</violation>
</file>

Shadow auto-approve: would not auto-approve because issues were found.

View guided diff | Re-trigger cubic

Comment thread linter/fix/engine.go
}

config := doc.GetCore().GetConfig()
*doc = openapi.OpenAPI{}

@cubic-dev-ai cubic-dev-ai Bot Oct 24, 2026 •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: Rebuilding the model discards the original DocumentNode, so node-fixed YAML files lose document-level comments when written. Preserve and reattach the original document node, or rebuild from a document wrapper instead of passing only the mapping root.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At linter/fix/engine.go, line 328:

<comment>Rebuilding the model discards the original `DocumentNode`, so node-fixed YAML files lose document-level comments when written. Preserve and reattach the original document node, or rebuild from a document wrapper instead of passing only the mapping root.</comment>

<file context>
@@ -269,9 +274,67 @@ func (e *Engine) ProcessErrors(ctx context.Context, doc *openapi.OpenAPI, errs [
+	}
+
+	config := doc.GetCore().GetConfig()
+	*doc = openapi.OpenAPI{}
+	doc.InitCache()
+	doc.GetCore().SetConfig(config)
</file context>
Fix with cubic

Comment thread linter/fix/engine.go
// 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

@cubic-dev-ai cubic-dev-ai Bot Oct 24, 2026 •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: A deferred node fix reserves its conflict key before it runs, so a failed node fix permanently blocks later same-rule fixes at that location. Reserve the key only after successful application, while preserving conflict handling among pending fixes.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At linter/fix/engine.go, line 252:

<comment>A deferred node fix reserves its conflict key before it runs, so a failed node fix permanently blocks later same-rule fixes at that location. Reserve the key only after successful application, while preserving conflict handling among pending fixes.</comment>

<file context>
@@ -241,19 +245,20 @@ func (e *Engine) ProcessErrors(ctx context.Context, doc *openapi.OpenAPI, errs [
+		// 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 {
</file context>
Fix with cubic

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

openapi spec lint --fix drops node-level fixes and can duplicate paths

1 participant