feat: add autofixer system with fixes for 32 lint rules#141
Open
TristanSpeakEasy wants to merge 1 commit intomainfrom
Open
feat: add autofixer system with fixes for 32 lint rules#141TristanSpeakEasy wants to merge 1 commit intomainfrom
TristanSpeakEasy wants to merge 1 commit intomainfrom
Conversation
TristanSpeakEasy
added
enhancement
Contributor
📊 Test Coverage ReportCurrent Coverage: Coverage Change: 📉 -.2% (decreased) Coverage by Package
📋 Detailed Coverage by Function (click to expand)
Generated by GitHub Actions |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Comment on lines
+228
to
+235
|
|
||
| // Write modified document back if any fixes were applied (and not dry-run) | ||
| if len(result.Applied) > 0 && !fixOpts.DryRun { | ||
| processor, err := NewOpenAPIProcessor(cleanFile, "", true) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to create processor: %w", err) | ||
| } | ||
| if err := processor.WriteDocument(ctx, doc); err != nil { |
There was a problem hiding this comment.
🔴 WriteDocument prints status message to stdout, corrupting JSON lint output when using --fix
When --fix (or --fix-interactive) is used together with --format json, the WriteDocument call in applyFixes prints "📄 Document written to: ..." to stdout via fmt.Printf (cmd/openapi/commands/openapi/shared.go:99). This output is then followed by the JSON lint output, producing invalid JSON on stdout.
Root Cause
applyFixes at cmd/openapi/commands/openapi/lint.go:228-234 calls processor.WriteDocument(ctx, doc) which internally uses fmt.Printf to print a status message to stdout. Then lintOpenAPI continues to print the JSON lint output to stdout at lines 192-194.
Stdout would contain:
📄 Document written to: file.yaml
{"results": [...], "summary": {...}}
This makes the JSON output unparseable for any downstream tooling consuming --format json.
Fix reporting messages ("Applied N fix(es)") at line 235 correctly go to stderr, but the WriteDocument status message leaks to stdout.
Impact: Any CI/CD pipeline or tooling that uses openapi spec lint --fix --format json and parses stdout as JSON will break.
Prompt for agents
In cmd/openapi/commands/openapi/lint.go, the applyFixes function calls processor.WriteDocument which prints a status message to stdout. This corrupts JSON output when --format json is used with --fix.
Two possible approaches:
1. Create the OpenAPIProcessor differently to suppress stdout messages - e.g. use a custom WriteDocument method that writes to the file without printing status to stdout, or write the file directly without using OpenAPIProcessor.
2. Alternatively, modify the WriteDocument call site: after calling NewOpenAPIProcessor, set a flag or use a variant that suppresses the fmt.Printf status message at shared.go line 99. The simplest fix is to write the file directly using marshaller.Marshal and os.Create, bypassing WriteDocument's status message entirely.
Was this helpful? React with 👍 or 👎 to provide feedback.
Comment on lines
+149
to
+158
| // Check for conflicts at the same location | ||
| key := conflictKey{Line: vErr.GetLineNumber(), Column: vErr.GetColumnNumber()} | ||
| if key.Line >= 0 && modified[key] { | ||
| result.Skipped = append(result.Skipped, SkippedFix{ | ||
| Error: vErr, | ||
| Fix: fix, | ||
| Reason: SkipConflict, | ||
| }) | ||
| continue | ||
| } |
There was a problem hiding this comment.
🟡 Conflict detection based on line/column incorrectly skips independent fixes at the same YAML node
The fix engine's conflict detection uses exact (line, column) matching, which causes independent fixes targeting different properties of the same YAML node to be incorrectly skipped as "conflicts".
Root Cause
At linter/fix/engine.go:150-158, the engine marks a (line, column) as modified after applying a fix, and skips subsequent fixes at the same location:
key := conflictKey{Line: vErr.GetLineNumber(), Column: vErr.GetColumnNumber()}
if key.Line >= 0 && modified[key] {
result.Skipped = append(result.Skipped, SkippedFix{...SkipConflict})
continue
}This is too coarse-grained. Multiple errors can legitimately point to the same YAML node but require independent, non-conflicting fixes. Key affected scenarios:
-
Contact properties (
contact_properties.go:70-99): When a contact section is missingname,url, ANDemail, three errors are created all pointing tocontactRoot. The first fix (e.g., addname) is applied, but the other two are incorrectly skipped as conflicts. EachaddContactPropertyFixmodifies a different key, so they don't actually conflict. -
Error response rules: The 401, 429, 500, and 400 response rules (
owasp_define_error_responses_*.go) can all produce errors at the sameresponses.GetRootNode(). EachaddErrorResponseFixadds a different status code key, so they're independent. But only the first is applied; the rest are skipped.
Impact: When running --fix, only the first of multiple non-conflicting fixes at the same node is applied. The user must re-run the tool multiple times to fully fix all issues. The fixes are reported as "skipped (conflict)" which is misleading.
Prompt for agents
In linter/fix/engine.go lines 149-158, the conflict detection uses (line, column) as the conflict key. This is too coarse-grained and causes independent fixes at the same YAML node to be skipped incorrectly.
Consider one of these approaches:
1. Include the fix type or a fix-specific key in the conflict detection. For example, use (line, column, rule) as the conflict key, or (line, column, fix.Description()) to distinguish fixes that target different properties.
2. Add a method to the Fix interface like ConflictKey() that returns an opaque string identifying what the fix modifies (e.g., the property name being added), so fixes modifying different properties at the same node don't conflict.
3. Remove the conflict detection entirely and rely on fix idempotency checks (many fixes already have idempotency guards like checking if a key exists before adding it).
Was this helpful? React with 👍 or 👎 to provide feedback.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
--fix(auto) and--fix-interactive(prompted) CLI modes, plus--dry-runpreviewlist-rulescommand with[fixable]markers and JSONfixAvailablefieldFixAvailable()method to all 32 fix-capable rule structs, surfaced in doc generator and CLITest plan
mise cipasses (format, lint, tests, build)openapi lint --fix spec.yamlon a spec with trailing slashes, http URLs, duplicate enums — verify auto-fixes applyopenapi lint --fix-interactive spec.yaml— verify prompts appear for description/limit rulesopenapi lint --dry-run --fix spec.yaml— verify no file changes, fix report printedopenapi spec list-rules— verify[fixable]markers on 32 rulesopenapi lint --fix --fix-interactiveerrors with mutual exclusion message