Repository navigation
SLO-208D — Add OpenAPI merge-order semantic rule - #73
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📜 Recent review details🔇 Additional comments (13)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds SLP205: a semantic linter that finds unsafe OpenAPI ChangesSLP205 OpenAPI Path Merge-Order Rule
Sequence Diagram(s)sequenceDiagram
participant DiffScanner
participant FileFilter
participant BlockCollector
participant MergeAnalyzer
participant Reporter
DiffScanner->>FileFilter: iterate diff files and hunks
FileFilter->>BlockCollector: detect "spec.paths = {" lines
BlockCollector->>MergeAnalyzer: extract brace-balanced object block (skip deleted lines)
MergeAnalyzer->>MergeAnalyzer: strip comments, compute brace depths, find spread events
MergeAnalyzer->>Reporter: report Finding when generated-path spread follows spec spread
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Comment |
coderabbitai
Bot
added
enhancement
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/rules/slp205.go`:
- Around line 169-174: The loops that append slp205Event use indices returned
from slp205SpecPathsSpread.FindAllStringIndex and
slp205GeneratedPathsSpread.FindAllStringIndex without checking the slice length;
add defensive guards inside the loops to verify each idx has at least 2 elements
before accessing idx[0] (and idx[1] if needed), and skip or log malformed
matches rather than indexing into a shorter slice; update the two loops that
construct slp205Event (referencing slp205SpecPathsSpread,
slp205GeneratedPathsSpread, slp205Event, kind, pos, and ln.content) to perform
this length check before appending.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2fe8d94b-02c2-4504-b216-429d5e0944c2
📒 Files selected for processing (5)
README.mdpkg/rules/registry.gopkg/rules/registry_test.gopkg/rules/slp205.gopkg/rules/slp205_test.go
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Seer Code Review
🧰 Additional context used
🪛 GitHub Actions: CI / 0_build-and-test.txt
pkg/rules/slp205.go
[warning] SLP071 type assertion — use v, ok := x.(Type) for safe handling.
[warning] 68-70: SLP043 response struct may have duplicate JSON keys: OpenAPI generated path map is spread after spec.paths, overriding richer JSDoc annotations; spread spec.paths last.
[warning] 81-81: SLP050 function slp205CollectObjectBlock accepts lines without validation — add nil/empty check.
[warning] 118-118: SLP050 function slp205BraceDelta accepts content without validation — add nil/empty check.
[warning] 131-131: SLP050 function slp205HasAddedRelevantLine accepts lines without validation — add nil/empty check.
[warning] 145-145: SLP050 function slp205BadPathMergeOrder accepts lines without validation — add nil/empty check.
[warning] 163-163: SLP050 function slp205MergeEvents accepts lines without validation — add nil/empty check.
[warning] 169-169: SLP065 error return ignored — handle or explicitly suppress with _.
[error] 170-170: SLP118 direct index access without length guard — may panic on empty collection (idx[0]).
[warning] 172-172: SLP065 error return ignored — handle or explicitly suppress with _.
[error] 173-173: SLP118 direct index access without length guard — may panic on empty collection (idx[0]).
🪛 GitHub Actions: CI / build-and-test
pkg/rules/slp205.go
[warning] SLP071: type assertion — use v, ok := x.(Type) for safe handling
[info] 15-15: SLP089: exported function/class missing docstring — add JSDoc comment or description for maintainability
[info] 16-16: SLP089: exported function/class missing docstring — add JSDoc comment or description for maintainability
[info] 17-17: SLP089: exported function/class missing docstring — add JSDoc comment or description for maintainability
[info] 23-23: SLP117: unanchored regex pattern — add ^, $, or \b anchors to prevent unintended substring matches
[info] 41-41: SLP055: function Check has 8 conditionals with no comments — explain the complex logic
[info] 41-41: SLP089: exported function/class missing docstring — add JSDoc comment or description for maintainability
[warning] 68-68: SLP043: response struct may have duplicate JSON keys
[warning] 69-69: SLP043: response struct may have duplicate JSON keys
[warning] 70-70: SLP043: response struct may have duplicate JSON keys ("OpenAPI generated path map is spread after spec.paths, overriding richer JSDoc annotations; spread spec.paths last")
[warning] 81-81: SLP050: function slp205CollectObjectBlock accepts lines without validation — add nil/empty check
[warning] 118-118: SLP050: function slp205BraceDelta accepts content without validation — add nil/empty check
[warning] 131-131: SLP050: function slp205HasAddedRelevantLine accepts lines without validation — add nil/empty check
[warning] 145-145: SLP050: function slp205BadPathMergeOrder accepts lines without validation — add nil/empty check
[warning] 163-163: SLP050: function slp205MergeEvents accepts lines without validation — add nil/empty check
[warning] 169-169: SLP065: error return ignored — handle or explicitly suppress with _
[error] 170-170: SLP118: direct index access without length guard — may panic on empty collection
[warning] 172-172: SLP065: error return ignored — handle or explicitly suppress with _
[error] 173-173: SLP118: direct index access without length guard — may panic on empty collection
🔇 Additional comments (15)
README.md (1)
114-114: LGTM!pkg/rules/slp205.go (8)
1-19: LGTM!
21-26: LGTM!
28-39: LGTM!
41-79: LGTM!
81-116: LGTM!
118-129: LGTM!
131-143: LGTM!
145-161: LGTM!pkg/rules/slp205_test.go (2)
5-132: LGTM!
134-145: LGTM!pkg/rules/registry.go (1)
165-166: LGTM!pkg/rules/registry_test.go (3)
138-138: LGTM!
156-157: LGTM!
187-187: LGTM!
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/rules/slp205.go`:
- Around line 179-203: The code currently collects spread matches anywhere in
each ln.content; restrict detection to only root-level spec.paths entries by
making slp205MergeEvents depth-aware: scan the lines slice to locate the
top-level "spec:" then its "paths:" key and record the base indentation for
spec.paths, then when iterating matches from slp205SpecPathsSpread and
slp205GeneratedPathsSpread only append slp205Event if the match's line
indentation equals the spec.paths entry indentation (use leading-space count
from ln.content) so nested objects (greater indentation) are ignored; use
slp205CommentOnlyLinePrefix to skip comments as before and keep symbols
slp205MergeEvents, slp205Event, slp205SpecPathsSpread,
slp205GeneratedPathsSpread unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 86e29783-6aae-466d-a2be-ddc467583874
📒 Files selected for processing (1)
pkg/rules/slp205.go
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Seer Code Review
🔇 Additional comments (1)
pkg/rules/slp205.go (1)
11-177: LGTM!Also applies to: 205-212
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/rules/slp205_test.go`:
- Around line 147-149: The test only checks the count of findings; update the
assertion to also validate each returned finding when tt.want > 0 by iterating
over the got slice and asserting finding.RuleID == "SLP205" and
finding.Location.Line (or finding.Line) > 0; keep the existing count check
(len(got) != tt.want) and then, inside a conditional if tt.want > 0, loop
through got and fail the test if any entry has a different RuleID or a
zero/negative Line to ensure results point to the correct rule and location.
In `@pkg/rules/slp205.go`:
- Around line 240-245: The check for nil on the slice `match` is redundant and
triggers staticcheck S1009; in the `slp205MatchStart` function remove the `if
match == nil || len(match) == 0 { ... }` branch and rely on the subsequent `if
len(match) < 2 { return 0, false }` check (which safely handles nil slices),
keeping the existing behavior and return values intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ced5ecfc-5bd5-4798-98ec-36ae01ea94e7
📒 Files selected for processing (2)
pkg/rules/slp205.gopkg/rules/slp205_test.go
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Seer Code Review
🧰 Additional context used
🪛 golangci-lint (2.12.2)
pkg/rules/slp205.go
[error] 240-240: S1009: should omit nil check; len() for nil slices is defined as zero
(staticcheck)
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/rules/slp205.go`:
- Around line 190-225: The current top-level filter uses
indentationOf(ln.content) and misclassifies same-line nested spreads; replace
the line-level indent check with a position-aware brace depth computed at each
match offset: implement a helper (e.g., braceDepthAt(content string, offset int)
int) that scans content up to pos counting '{' and '}' (and minimally ignoring
escapes/quotes if needed), call it instead of indentationOf for each match when
creating slp205Event (used where
slp205SpecPathsSpread/slp205GeneratedPathsSpread matches are appended to
candidates), use that depth to compute baseIndent (rename to baseDepth if
desired) and to filter events into events slice, and add a one-line regression
test that demonstrates a same-line nested spread (e.g., "spec.paths = { '/x': {
get: { ...oas7Paths } }, ...(spec.paths || {}) }") to ensure nested spread is
not treated as top-level.
- Around line 63-68: The current loop treats lines like "// spec.paths = {" as
real assignments; update the check before calling slp205CollectObjectBlock to
skip lines that are comment starters by detecting leading comment markers.
Concretely, in the loop that inspects ln := h.Lines[i] and uses
slp205SpecPathsAssign, add a guard (e.g., check strings.TrimSpace(ln.Content)
has "//" prefix or use a regexp) so that commented lines are continued over
(similar to the existing Kind==diff.LineDelete check) and only non-comment,
non-deleted matches are passed to slp205CollectObjectBlock.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2e837499-0b33-414d-b4d0-67a1b4c88a03
📒 Files selected for processing (2)
pkg/rules/slp205.gopkg/rules/slp205_test.go
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Seer Code Review
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/rules/slp205.go`:
- Around line 106-117: The variable lineDelta is preinitialized to 0 but
immediately reassigned before any use in the slp205 parsing loop; remove the
wasted initialization and declare/assign lineDelta only where needed by directly
using the slp205BraceDelta(...) call (e.g., replace the current "lineDelta := 0"
with a declaration/assignment that occurs inside each branch or declare "var
lineDelta int" without an initial value and set it in the started=false and
started=true branches using slp205BraceDelta(content[open:]) and
slp205BraceDelta(content) respectively) so that lineDelta is not redundantly
initialized; update references within the loop accordingly (symbols: lineDelta,
started, slp205BraceDelta).
- Around line 252-257: The loop that checks for i == 0 is redundant; directly
return the first element of the slice with a bounds check instead. Replace the
for-loop over match in the function using the match variable with a simple if
len(match) > 0 { return match[0], true } else { return 0, false } so you keep
the same return types and semantics while removing dead code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: db2e692e-bc34-480a-a90d-f18e62ede888
📒 Files selected for processing (2)
pkg/rules/slp205.gopkg/rules/slp205_test.go
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Seer Code Review
🧰 Additional context used
🪛 golangci-lint (2.12.2)
pkg/rules/slp205.go
[error] 106-106: assigned to lineDelta, but reassigned without using the value
(wastedassign)
🔇 Additional comments (6)
pkg/rules/slp205.go (5)
1-46: LGTM!
48-90: LGTM!
134-165: LGTM!
167-244: LGTM!
260-304: LGTM!pkg/rules/slp205_test.go (1)
1-199: LGTM!
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/rules/slp205.go`:
- Around line 104-117: The code is counting braces and matching spreads on raw
line text so inline comments break logic; create a helper that strips JS
comments (both // and /* */) but preserves quoted strings (i.e., remove comments
only when not inside single/double/backtick strings) and use it before calling
slp205BraceDelta, slp205BraceDepthAt and before running any spread regexes
(replace usages where content or slices like content[open:] are passed to those
functions with the comment-stripped version). Implement the helper (e.g.,
stripJSCommentsOutsideStrings) and call it in slp205CollectObjectBlock and any
places between lines 194-229, then add a regression test covering inline-comment
cases like `...(spec.paths || {}), // ...oas7Paths` and `// }` to ensure depth
and spread detection behave correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6fa312a1-75dc-4aaf-be42-92d809e5d06b
📒 Files selected for processing (2)
pkg/rules/slp205.gopkg/rules/slp205_test.go
📜 Review details
🔇 Additional comments (1)
pkg/rules/slp205_test.go (1)
168-184: No changes needed. The module targetsgo 1.22, which fixed range variable capture semantics. The closure in the table-driven subtest safely capturesttwithout requiring the local rebinding (tt := tt). The code is correct as written.
| if s.escaped { | ||
| s.escaped = false | ||
| return true | ||
| } | ||
| if ch == '\\' { | ||
| s.escaped = true | ||
| return true | ||
| } | ||
| if ch == s.quote { | ||
| s.quote = 0 | ||
| } |
There was a problem hiding this comment.
Bug: The function slp205StripJSCommentsOutsideStrings processes code line-by-line and doesn't track state for multi-line comments or strings, which can lead to incorrect violation detection in those edge cases.
Severity: LOW
Suggested Fix
The line-by-line processing logic should be updated to track the state of multi-line constructs like block comments (/* ... */) and template literals (backticks) across lines. This would involve passing the state from one line's processing to the next, ensuring that content inside these multi-line blocks is correctly ignored. Adding test cases for multi-line comments and strings is also recommended to prevent regressions.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: pkg/rules/slp205.go#L307-L348
Potential issue: The function `slp205StripJSCommentsOutsideStrings` processes each line
of code independently, without maintaining state for multi-line constructs. This means
if a multi-line block comment (`/* ... */`) or a multi-line template string (using
backticks) spans several lines, the function will not know it's inside that construct
when processing subsequent lines. This can lead to incorrect behavior, such as
miscalculating brace nesting depth or incorrectly identifying code patterns within
comments as violations, resulting in false positives.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 participant
Closes #67
Implements SLP205 for the Whimsy-exposed OpenAPI path merge-order hazard.
@coderabbitai review