feat(rules): add SLP219-222 reviewer gap closure v3 - #91
Merged
Conversation
Add 4 high-impact rules addressing top patterns from benchmark analysis: - SLP219: data race on shared state field without lock (5 findings) - SLP220: filepath.Walk without ctx.Err() check (3 findings) - SLP221: exec.Command without stderr captured (3 findings) - SLP222: UTF-16/BOM data parsed as UTF-8 (2 findings) All rules severity=warn, pre-hook passed, tests green (15 new tests). Updates README rule count to 164.
|
Seems you are using me but didn't get OPENAI_API_KEY seted in Variables/Secrets for this repo. you could follow readme for more information |
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
CodeAnt AI is reviewing your PR. |
Reviewer's GuideAdds four new high-impact Go diff rules (SLP219–SLP222) for reviewer gap closure v3, wires them into the rules registry and README, and introduces targeted tests to validate their detection heuristics and registry count. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
codeant-ai
Bot
added
the
size:XL
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- SLP219 only looks for locking/atomic calls within the added hunk, so it will flag a new shared-state access even if the handler already acquires a lock in existing code; consider scanning the full function/hunk (including context lines) or at least non-added lines for lock/atomic usage to reduce false positives.
- In SLP221,
stderrWireRewill also match occurrences ofStderrin comments or unrelated identifiers, which can suppress findings incorrectly; you may want to ignore commented lines or tighten the regex to assignments on the actual command variable. - In SLP222,
utf8OpReincludesbytes\.ToString, which is not a real API and may indicate the pattern isn't aligned with actual usage; consider either removing that alternative or replacing it with more realistic patterns (e.g.,string(out)orbytes.Runes-style operations you want to catch).
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- SLP219 only looks for locking/atomic calls within the added hunk, so it will flag a new shared-state access even if the handler already acquires a lock in existing code; consider scanning the full function/hunk (including context lines) or at least non-added lines for lock/atomic usage to reduce false positives.
- In SLP221, `stderrWireRe` will also match occurrences of `Stderr` in comments or unrelated identifiers, which can suppress findings incorrectly; you may want to ignore commented lines or tighten the regex to assignments on the actual command variable.
- In SLP222, `utf8OpRe` includes `bytes\.ToString`, which is not a real API and may indicate the pattern isn't aligned with actual usage; consider either removing that alternative or replacing it with more realistic patterns (e.g., `string(out)` or `bytes.Runes`-style operations you want to catch).
## Individual Comments
### Comment 1
<location path="pkg/rules/slp221.go" line_range="62" />
<code_context>
+ if execLine == nil || runLine == nil {
+ continue
+ }
+ if stderrWireRe.MatchString(hunkText) {
+ continue
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** Stderr wiring is only detected in newly added lines, so existing wiring can be missed.
Since hunkText only includes added lines, stderr wiring in unchanged context (e.g., an existing `cmd.Stderr = ...`) is ignored, so this rule can flag a false positive when only the `.Run()`/`.Output()` call is new. Consider building hunkText from the full hunk (context + added) or explicitly scanning nearby unchanged lines for stderr wiring before emitting a finding.
</issue_to_address>
### Comment 2
<location path="pkg/rules/slp222.go" line_range="56" />
<code_context>
+ if !hasWmic || len(utf8Lines) == 0 {
+ continue
+ }
+ if decodeRe.MatchString(hunkText.String()) {
+ continue
+ }
</code_context>
<issue_to_address>
**issue:** UTF-16/BOM decoding is only detected in added lines, ignoring existing decode logic.
Because `hunkText` is built only from added lines, this will miss any existing BOM/UTF‑16 handling in unchanged code and still report a violation when the decode already happens. To reduce false positives, consider including surrounding context (non-added lines) when building `hunkText`, or performing a separate scan that accounts for existing decode logic.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| if execLine == nil || runLine == nil { | ||
| continue | ||
| } | ||
| if stderrWireRe.MatchString(hunkText) { |
There was a problem hiding this comment.
issue (bug_risk): Stderr wiring is only detected in newly added lines, so existing wiring can be missed.
Since hunkText only includes added lines, stderr wiring in unchanged context (e.g., an existing cmd.Stderr = ...) is ignored, so this rule can flag a false positive when only the .Run()/.Output() call is new. Consider building hunkText from the full hunk (context + added) or explicitly scanning nearby unchanged lines for stderr wiring before emitting a finding.
| if !hasWmic || len(utf8Lines) == 0 { | ||
| continue | ||
| } | ||
| if decodeRe.MatchString(hunkText.String()) { |
There was a problem hiding this comment.
issue: UTF-16/BOM decoding is only detected in added lines, ignoring existing decode logic.
Because hunkText is built only from added lines, this will miss any existing BOM/UTF‑16 handling in unchanged code and still report a violation when the decode already happens. To reduce false positives, consider including surrounding context (non-added lines) when building hunkText, or performing a separate scan that accounts for existing decode logic.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces four new linter rules (SLP219, SLP220, SLP221, and SLP222) to detect shared-state data races, uncancellable file walks, uncaptured subprocess stderr, and unhandled UTF-16/BOM output. The review feedback highlights several key improvements: expanding SLP219's regex to support multi-letter receiver variables, excluding CombinedOutput from SLP221 since it already captures stderr, ensuring SLP220 ignores deleted lines (where line number is 0) to avoid false negatives, and correcting the BOM check in the SLP222 test to use the proper UTF-16LE bytes.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| } | ||
|
|
||
| // sharedFieldRe matches s.X, r.X, h.X where X looks like shared mutable state. | ||
| var sharedFieldRe = regexp.MustCompile(`\b([a-zA-Z])\.(Manager|Store|State|Config|Registry|Cache|Pool|Snapshot[A-Z][a-zA-Z]*)\b`) |
There was a problem hiding this comment.
The regular expression sharedFieldRe uses ([a-zA-Z]) which only matches single-letter receiver variables (e.g., s.Config). It will fail to match multi-letter receiver variables (e.g., srv.Config, mgr.Store, state.State), which are very common in Go. Updating it to match any valid Go identifier ensures much better coverage.
Suggested change
| var sharedFieldRe = regexp.MustCompile(`\b([a-zA-Z])\.(Manager|Store|State|Config|Registry|Cache|Pool|Snapshot[A-Z][a-zA-Z]*)\b`) | |
| var sharedFieldRe = regexp.MustCompile(`\b([a-zA-Z_][a-zA-Z0-9_]*)\.(Manager|Store|State|Config|Registry|Cache|Pool|Snapshot[A-Z][a-zA-Z]*)\b`) |
| } | ||
|
|
||
| var execCommandContextRe = regexp.MustCompile(`\bexec\.Command(Context)?\s*\(`) | ||
| var execRunRe = regexp.MustCompile(`\.(Output|Run|CombinedOutput)\s*\(\s*\)`) |
There was a problem hiding this comment.
CombinedOutput() runs the command and returns its combined standard output and standard error. It does not require Stderr to be manually wired up, and in fact, setting Stderr before calling CombinedOutput() causes a runtime error in Go's os/exec package. Therefore, CombinedOutput should be excluded from execRunRe to avoid false positives and incorrect guidance.
Suggested change
| var execRunRe = regexp.MustCompile(`\.(Output|Run|CombinedOutput)\s*\(\s*\)`) | |
| var execRunRe = regexp.MustCompile(`\.(Output|Run)\s*\(\s*\)`) |
| hunkHasCtx := false | ||
| for _, h := range f.Hunks { | ||
| for _, hln := range h.Lines { | ||
| if hln.NewLineNo >= ln.NewLineNo-5 && hln.NewLineNo <= ln.NewLineNo+80 && |
There was a problem hiding this comment.
When checking for context cancellation, hln.NewLineNo can be 0 for deleted lines in the diff. If ln.NewLineNo is small (e.g., <= 5), ln.NewLineNo-5 can be <= 0, which makes 0 >= ln.NewLineNo-5 evaluate to true. This can cause deleted lines containing context checks to be incorrectly matched, leading to false negatives. Adding a check for hln.NewLineNo > 0 prevents this.
Suggested change
| if hln.NewLineNo >= ln.NewLineNo-5 && hln.NewLineNo <= ln.NewLineNo+80 && | |
| if hln.NewLineNo > 0 && hln.NewLineNo >= ln.NewLineNo-5 && hln.NewLineNo <= ln.NewLineNo+80 && |
Comment on lines
+33
to
+35
| + if len(out) >= 3 && out[0] == 0xEF && out[1] == 0xBB && out[2] == 0xBF { | ||
| + out = out[3:] | ||
| + } |
There was a problem hiding this comment.
The test comment mentions decoding a UTF-16 BOM, but the code checks for 0xEF, 0xBB, 0xBF, which is the UTF-8 BOM. Since wmic outputs UTF-16LE by default, the BOM is actually 0xFF, 0xFE. The test code should be updated to check for the correct UTF-16LE BOM to avoid misleading implementation patterns.
Suggested change
| + if len(out) >= 3 && out[0] == 0xEF && out[1] == 0xBB && out[2] == 0xBF { | |
| + out = out[3:] | |
| + } | |
| if len(out) >= 2 && out[0] == 0xFF && out[1] == 0xFE { | |
| out = out[2:] | |
| } |
|
Seems you are using me but didn't get OPENAI_API_KEY seted in Variables/Secrets for this repo. you could follow readme for more information |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds four new Go diff-based rules (SLP219–SLP222), updates the default registry and README to reflect 164 rules, and includes minor formatting/EOF fixes across existing rule files and tests. ChangesReviewer gap closure v3: Four new safety rules
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ 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)
Comment |
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
Comment on lines
+28
to
+71
| var execRunRe = regexp.MustCompile(`\.(Output|Run|CombinedOutput)\s*\(\s*\)`) | ||
| var stderrWireRe = regexp.MustCompile(`\b(Stderr\s*=|StderrPipe|cmd\.Stderr|\.Stderr\s*=)`) | ||
|
|
||
| func (r SLP221) Check(d *diff.Diff) []Finding { | ||
| var out []Finding | ||
| for _, f := range d.Files { | ||
| if f.IsDelete || !strings.HasSuffix(strings.ToLower(f.Path), ".go") { | ||
| continue | ||
| } | ||
| if strings.HasSuffix(strings.ToLower(f.Path), "_test.go") { | ||
| continue | ||
| } | ||
|
|
||
| for _, h := range f.Hunks { | ||
| var execLine *diff.Line | ||
| var runLine *diff.Line | ||
| hunkText := "" | ||
| for _, ln := range h.Lines { | ||
| if ln.Kind == diff.LineAdd { | ||
| c := ln.Content | ||
| hunkText += c + "\n" | ||
| if execCommandContextRe.MatchString(c) { | ||
| cp := ln | ||
| execLine = &cp | ||
| } | ||
| if execRunRe.MatchString(c) { | ||
| cp := ln | ||
| runLine = &cp | ||
| } | ||
| } | ||
| } | ||
| if execLine == nil || runLine == nil { | ||
| continue | ||
| } | ||
| if stderrWireRe.MatchString(hunkText) { | ||
| continue | ||
| } | ||
| // Prefer the run-line position: that's the actual lossy site. | ||
| out = append(out, Finding{ | ||
| RuleID: r.ID(), | ||
| Severity: r.DefaultSeverity(), | ||
| File: f.Path, | ||
| Line: runLine.NewLineNo, | ||
| Message: "exec.Command .Output/.Run without Stderr/StderrPipe — subprocess failure stderr is discarded", |
There was a problem hiding this comment.
🔴 Architect Review — CRITICAL
The rule treats exec.Command(...).CombinedOutput() the same as Output/Run and assumes stderr is discarded when Stderr/StderrPipe is not wired, but CombinedOutput already returns stdout and stderr; valid call sites using CombinedOutput are incorrectly flagged as losing stderr.
Suggestion: Exclude CombinedOutput from execRunRe (or otherwise treat it as a safe stderr-capture path) so the rule only warns on Output/Run cases that truly discard subprocess stderr context.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is an **Architect / Logical Review** comment left during a code review. These reviews are first-class, important findings — not optional suggestions. Do NOT dismiss this as a 'big architectural change' just because the title says architect review; most of these can be resolved with a small, localized fix once the intent is understood.
**Path:** pkg/rules/slp221.go
**Line:** 28:71
**Comment:**
*CRITICAL: The rule treats exec.Command(...).CombinedOutput() the same as Output/Run and assumes stderr is discarded when Stderr/StderrPipe is not wired, but CombinedOutput already returns stdout and stderr; valid call sites using CombinedOutput are incorrectly flagged as losing stderr.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
If a suggested approach is provided above, use it as the authoritative instruction. If no explicit code suggestion is given, you MUST still draft and apply your own minimal, localized fix — do not punt back with 'no suggestion provided, review manually'. Keep the change as small as possible: add a guard clause, gate on a loading state, reorder an await, wrap in a conditional, etc. Do not refactor surrounding code or expand scope beyond the finding.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
| } | ||
|
|
||
| var execCommandContextRe = regexp.MustCompile(`\bexec\.Command(Context)?\s*\(`) | ||
| var execRunRe = regexp.MustCompile(`\.(Output|Run|CombinedOutput)\s*\(\s*\)`) |
There was a problem hiding this comment.
Suggestion: Including CombinedOutput() in the lossy-command regex is incorrect because CombinedOutput already captures stderr by design. This will generate false positives for safe code paths; restrict this rule to Run()/Output() (or explicitly exempt CombinedOutput). [logic error]
Severity Level: Critical 🚨
- ❌ False positives for commands using CombinedOutput correctly.
- ⚠️ Developers may distrust SLP221 rule output.Steps of Reproduction ✅
1. A developer adds new production Go code in a non-test file (SLP221 skips *_test.go* at
pkg/rules/slp221.go:37–38) that runs an external command via `cmd := exec.Command("git",
"status")` followed by `out, err := cmd.CombinedOutput()` and opens a PR with this change.
2. The `slopgate` CLI entrypoint in `cmd/slopgate/main.go:38–80` executes on that PR,
calls `readGitDiff` and then parses the unified diff into a `*diff.Diff` using
`diff.Parse` at `cmd/slopgate/main.go:118–17`.
3. `rules.Default()` in `pkg/rules/registry.go:6–197` builds the registry and registers
`SLP221` at line 195, then `rules.Default().Run(parsed, cfg)` at `cmd/slopgate/main.go:70`
invokes `SLP221.Check` with the parsed diff.
4. Inside `SLP221.Check` (`pkg/rules/slp221.go:31–77`), the added `exec.Command` line
matches `execCommandContextRe` (line 27) and the `.CombinedOutput()` call matches
`execRunRe` (line 28); because no explicit stderr wiring appears in the hunk,
`stderrWireRe` (line 29) does not match the accumulated `hunkText`, the guard at lines
59–63 passes, and the rule appends a `Finding` at lines 66–73 claiming "exec.Command
.Output/.Run without Stderr/StderrPipe — subprocess failure stderr is discarded" even
though `.CombinedOutput()` already returns combined stdout+stderr in `out`, making this a
persistent false-positive whenever CombinedOutput is correctly used.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** pkg/rules/slp221.go
**Line:** 28:28
**Comment:**
*Logic Error: Including `CombinedOutput()` in the lossy-command regex is incorrect because `CombinedOutput` already captures stderr by design. This will generate false positives for safe code paths; restrict this rule to `Run()`/`Output()` (or explicitly exempt `CombinedOutput`).
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix|
|
||
| var execCommandContextRe = regexp.MustCompile(`\bexec\.Command(Context)?\s*\(`) | ||
| var execRunRe = regexp.MustCompile(`\.(Output|Run|CombinedOutput)\s*\(\s*\)`) | ||
| var stderrWireRe = regexp.MustCompile(`\b(Stderr\s*=|StderrPipe|cmd\.Stderr|\.Stderr\s*=)`) |
There was a problem hiding this comment.
Suggestion: Treating any cmd.Stderr text as proof of stderr wiring is too broad and suppresses real findings when code only reads/logs the field instead of assigning it. Match only actual assignment or pipe setup patterns to avoid false negatives. [incorrect condition logic]
Severity Level: Major ⚠️
- ❌ Unwired stderr commands misclassified as safely wired.
- ⚠️ Reviewer loses signal about opaque subprocess failures.Steps of Reproduction ✅
1. A developer adds a new helper in a non-test Go file that runs a command without wiring
stderr, for example:
`cmd := exec.Command("make", "build"); err := cmd.Run(); log.Printf("stderr writer:
%v", cmd.Stderr)`, and pushes a PR with this change so all three lines appear as
additions in a single hunk.
2. The `slopgate` CLI (cmd/slopgate/main.go:38–80) analyzes the PR, parses the git diff
into a `*diff.Diff` via `diff.Parse` (main.go:118–17), and runs all default rules with
`rules.Default().Run(parsed, cfg)` at main.go:70, which includes `SLP221` registered in
`pkg/rules/registry.go:195`.
3. In `SLP221.Check` (`pkg/rules/slp221.go:31–77`), the loop over added lines at 45–57
records the `exec.Command` line as `execLine` using `execCommandContextRe` (line 27) and
the `cmd.Run()` line as `runLine` using `execRunRe` (line 28), while the
`log.Printf("stderr writer: %v", cmd.Stderr)` line contributes the substring `cmd.Stderr`
into `hunkText`.
4. After scanning the hunk, the guard `if execLine == nil || runLine == nil { continue }`
at lines 59–60 passes, but `stderrWireRe.MatchString(hunkText)` at line 62 returns true
solely because the string `cmd.Stderr` appears (from the read-only log statement), so the
function hits `continue` at line 63 and never appends a `Finding`, even though stderr was
never actually wired via assignment or `StderrPipe`, turning this lossy subprocess into a
false negative.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** pkg/rules/slp221.go
**Line:** 29:29
**Comment:**
*Incorrect Condition Logic: Treating any `cmd.Stderr` text as proof of stderr wiring is too broad and suppresses real findings when code only reads/logs the field instead of assigning it. Match only actual assignment or pipe setup patterns to avoid false negatives.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+59
to
+60
| if execLine == nil || runLine == nil { | ||
| continue |
There was a problem hiding this comment.
Suggestion: Skipping when either exec.Command(...) or .Run()/.Output() is absent in the same added hunk misses common incremental changes (for example, only adding cmd.Run() on an existing command variable). This creates false negatives; detection should not require both operations to be newly added together. [incomplete implementation]
Severity Level: Major ⚠️
- ❌ Incremental exec.Run additions escape SLP221 detection.
- ⚠️ Subprocess error details remain uncaptured after refactors.Steps of Reproduction ✅
1. A repository already contains a function with `cmd := exec.Command("make", "build")` in
a non-test Go file from an earlier commit; in a new PR the developer only adds `return
cmd.Run()` on a new line in the same function, without touching the original
`exec.Command` line.
2. Running `slopgate` on this PR (cmd/slopgate/main.go:38–80) parses the staged or base
diff with `diff.Parse` into a `*diff.Diff` at main.go:118–17 and invokes
`rules.Default().Run(parsed, cfg)` at main.go:70, which includes `SLP221` from
`pkg/rules/registry.go:195`.
3. Inside `SLP221.Check` (`pkg/rules/slp221.go:31–77`), the diff for that file contains
the new `return cmd.Run()` line as a `diff.LineAdd`, but the existing `cmd :=
exec.Command(...)` line is context rather than an added line, so the inner loop at 45–57
sets `runLine` via `execRunRe` (line 28) but leaves `execLine` nil because
`execCommandContextRe` only runs on `diff.LineAdd`.
4. When the hunk scan finishes, the condition `if execLine == nil || runLine == nil {
continue }` at lines 59–60 evaluates true (due to `execLine == nil`), causing the rule to
skip stderr wiring checks and emit no `Finding`, despite the fact that this new `.Run()`
call executes an existing unwired `exec.Command` and should be flagged for losing stderr
on failure.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** pkg/rules/slp221.go
**Line:** 59:60
**Comment:**
*Incomplete Implementation: Skipping when either `exec.Command(...)` or `.Run()/.Output()` is absent in the same added hunk misses common incremental changes (for example, only adding `cmd.Run()` on an existing command variable). This creates false negatives; detection should not require both operations to be newly added together.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+62
to
+63
| if stderrWireRe.MatchString(hunkText) { | ||
| continue |
There was a problem hiding this comment.
Suggestion: Using a single hunk-wide stderr check means one properly wired command can hide another unwired command in the same hunk. Track stderr wiring per command instance (or per execution line) instead of globally per hunk to avoid false negatives. [logic error]
Severity Level: Major ⚠️
- ❌ Second unwired command hidden by first wired.
- ⚠️ Multi-command hunks under-report stderr loss risk.Steps of Reproduction ✅
1. A developer refactors a non-test Go file and, in one commit, adds two separate command
invocations in the same diff hunk: the first safely wires stderr (`cmd1 :=
exec.Command(...); cmd1.Stderr = &buf; err1 := cmd1.Run()`), while the second does not
(`cmd2 := exec.Command(...); err2 := cmd2.Run()`), then opens a PR.
2. The `slopgate` CLI (cmd/slopgate/main.go:38–80) runs on this PR, parses the unified
diff via `diff.Parse` into a `*diff.Diff` (main.go:118–17), and calls
`rules.Default().Run(parsed, cfg)` at main.go:70, which executes `SLP221.Check` registered
at `pkg/rules/registry.go:195`.
3. In `SLP221.Check` (`pkg/rules/slp221.go:31–77`), the inner loop over added lines at
45–57 sees both `exec.Command` calls and both `.Run()` calls, appends all added content to
`hunkText`, and records at least the last `.Run()` as `runLine`, while the text
`cmd1.Stderr = &buf` ensures `stderrWireRe` (line 29) will match `hunkText`.
4. After processing the hunk, `execLine` and `runLine` are non-nil, but the guard `if
stderrWireRe.MatchString(hunkText) { continue }` at lines 62–63 fires because some stderr
wiring exists in the hunk, so the function skips emitting any `Finding` for the unwired
`cmd2.Run()` call, meaning one properly wired command masks another vulnerable command in
the same hunk.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** pkg/rules/slp221.go
**Line:** 62:63
**Comment:**
*Logic Error: Using a single hunk-wide stderr check means one properly wired command can hide another unwired command in the same hunk. Track stderr wiring per command instance (or per execution line) instead of globally per hunk to avoid false negatives.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
| } | ||
|
|
||
| // sharedFieldRe matches s.X, r.X, h.X where X looks like shared mutable state. | ||
| var sharedFieldRe = regexp.MustCompile(`\b([a-zA-Z])\.(Manager|Store|State|Config|Registry|Cache|Pool|Snapshot[A-Z][a-zA-Z]*)\b`) |
There was a problem hiding this comment.
Suggestion: The shared-state matcher only allows a single-letter receiver (s.X) and therefore misses common handler receiver names like srv.Store or handler.Config. Broaden the receiver side to support normal identifier lengths, otherwise this rule will silently miss real race-prone accesses. [incorrect condition logic]
Severity Level: Major ⚠️
- ⚠️ SLP219 misses shared-state access with multi-letter receivers.
- ⚠️ Handler data race risks can silently bypass static checks.Steps of Reproduction ✅
1. Run the `slopgate` CLI, which calls `run()` in `cmd/slopgate/main.go:42`, over a git
diff that adds an HTTP handler method like `func (srv *Server) Handle(w
http.ResponseWriter, r *http.Request)` in some Go file.
2. In that diff, include an added line within the handler body such as
`srv.Store.Update(id)` or `handler.Config.FeatureFlags = flags`, so the receiver
identifier has multiple characters and the field name matches `Store`/`Config`.
3. The CLI builds the rule registry via `rules.Default()` (`pkg/rules/registry.go:6-196`)
and then invokes `Registry.Run()` (`pkg/rules/rule.go:164-241`), which in turn calls
`SLP219.Check()` (`pkg/rules/slp219.go:36-84`) on the parsed diff from `diff.Parse()`
(`pkg/diff/parser.go:110-193`).
4. Inside `SLP219.Check`, the line `srv.Store.Update(id)` is tested against
`sharedFieldRe` at `pkg/rules/slp219.go:27-28`, which only matches a single-letter
receiver (`[a-zA-Z]` before the dot). Because `srv`/`handler` is longer than one
character, the regex does not match, and no `SLP219` finding is produced even though the
handler accesses a shared `Store`/`Config` field without any `.Lock()`/atomic usage in the
hunk.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** pkg/rules/slp219.go
**Line:** 28:28
**Comment:**
*Incorrect Condition Logic: The shared-state matcher only allows a single-letter receiver (`s.X`) and therefore misses common handler receiver names like `srv.Store` or `handler.Config`. Broaden the receiver side to support normal identifier lengths, otherwise this rule will silently miss real race-prone accesses.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| var handlerContextRe = regexp.MustCompile(`http\.ResponseWriter|\*http\.Request`) | ||
|
|
||
| // lockCallRe matches mutex acquisition or atomic ops. | ||
| var lockCallRe = regexp.MustCompile(`\.(Lock|Unlock|RLock|RUnlock)\(|atomic\.(Load|Store|Add|Swap|CompareAndSwap)`) |
There was a problem hiding this comment.
Suggestion: Treating Unlock/RUnlock as a safety signal is incorrect, because a hunk can contain an unlock call without any lock acquisition and still be flagged as safe. Restrict the safety check to lock acquisition (Lock/RLock or atomic synchronization) so races are not suppressed incorrectly. [incorrect condition logic]
Severity Level: Critical 🚨
- ❌ SLP219 suppresses races when only unlock is added.
- ⚠️ Concurrency regressions in handlers may pass unflagged.Steps of Reproduction ✅
1. Create a git diff where an HTTP handler method is modified to add a shared-state access
like `s.SnapshotManager.DoSomething()` and also adds a line `s.mu.Unlock()` but does not
add any corresponding `s.mu.Lock()` in the added lines; keep any existing lock acquisition
outside the diff hunk.
2. Run the `slopgate` CLI so `run()` in `cmd/slopgate/main.go:42-187` parses the diff via
`diff.Parse()` (`pkg/diff/parser.go:110-193`) and then executes
`rules.Default().Run(parsed, cfg)` (`pkg/rules/rule.go:164-241`), which includes `SLP219`
in the registry (`pkg/rules/registry.go:193`).
3. Inside `SLP219.Check` (`pkg/rules/slp219.go:36-84`), the added handler lines are
concatenated into `hunkStr` (lines 46-61). The shared-state access line matches
`sharedFieldRe` (`pkg/rules/slp219.go:27-28`), so it is a candidate finding.
4. However, because `lockCallRe` at `pkg/rules/slp219.go:33-34` treats both `.Unlock(` and
`.Lock(` as evidence of synchronization, the presence of `s.mu.Unlock()` in the added
lines causes `lockCallRe.MatchString(hunkStr)` to return true at line 66, so the entire
hunk is skipped as "safe" and no finding is reported, even though the diff could be
introducing a race by adding shared-state access without any added lock acquisition.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** pkg/rules/slp219.go
**Line:** 34:34
**Comment:**
*Incorrect Condition Logic: Treating `Unlock`/`RUnlock` as a safety signal is incorrect, because a hunk can contain an unlock call without any lock acquisition and still be flagged as safe. Restrict the safety check to lock acquisition (`Lock`/`RLock` or atomic synchronization) so races are not suppressed incorrectly.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+43
to
+47
| if hln.NewLineNo >= ln.NewLineNo-5 && hln.NewLineNo <= ln.NewLineNo+80 && | ||
| ctxErrCheckRe.MatchString(hln.Content) { | ||
| hunkHasCtx = true | ||
| break | ||
| } |
There was a problem hiding this comment.
Suggestion: The context-check scan does not filter by added lines, so deleted lines can satisfy the ctx.Err check when NewLineNo is 0 and the walk call is near the top of a file. That can suppress findings based on code that was removed; restrict the scan to added/context lines with valid new line numbers. [logic error]
Severity Level: Major ⚠️
- ❌ SLP220 misses regressions removing ctx.Err checks.
- ⚠️ Non-cancellable directory walks may not be reported.Steps of Reproduction ✅
1. Construct a git diff for a Go file that removes an existing `ctx.Err()` guard near the
top of the file and then adds a new `filepath.Walk` call without any `ctx.Err()` check in
its callback, such that the new `filepath.Walk` line has `NewLineNo <= 5` in the parsed
diff (`pkg/diff/parser.go:183-197`).
2. Run `slopgate` so `run()` in `cmd/slopgate/main.go:42-187` parses the diff using
`diff.Parse()` (`pkg/diff/parser.go:110-193`); deleted lines get `NewLineNo == 0` and
added lines (including the new `filepath.Walk` call) get positive `NewLineNo` values
(`pkg/diff/parser.go:64-70`).
3. `rules.Default().Run()` (`pkg/rules/rule.go:164-241`) invokes `SLP220.Check()`
(`pkg/rules/slp220.go:25-67`), which iterates added lines via `f.AddedLines()`
(`pkg/diff/parser.go:84-95`) and identifies the `filepath.Walk` line (lines 35-38 in
`pkg/rules/slp220.go`).
4. Inside the nested loop at `pkg/rules/slp220.go:41-47`, the deleted `ctx.Err()` line,
which has `hln.NewLineNo == 0`, falls within the window `hln.NewLineNo >= ln.NewLineNo-5`
and `<= ln.NewLineNo+80` (line 43) because `ln.NewLineNo-5` is negative for walks near the
top of the file; `ctxErrCheckRe.MatchString(hln.Content)` matches this deleted line,
setting `hunkHasCtx` to true and causing SLP220 to skip reporting the missing cancellation
check even though the new callback no longer checks `ctx.Err()`.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** pkg/rules/slp220.go
**Line:** 43:47
**Comment:**
*Logic Error: The context-check scan does not filter by added lines, so deleted lines can satisfy the `ctx.Err` check when `NewLineNo` is 0 and the walk call is near the top of a file. That can suppress findings based on code that was removed; restrict the scan to added/context lines with valid new line numbers.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+24
to
+57
| var decodeRe = regexp.MustCompile(`(?i)\b(utf16|unicode/bom|BOM|ByteOrderMark|unicode\/utf16|golang\.org\/x\/text\/encoding\/unicode)`) | ||
|
|
||
| func (r SLP222) Check(d *diff.Diff) []Finding { | ||
| var out []Finding | ||
| for _, f := range d.Files { | ||
| if f.IsDelete || !strings.HasSuffix(strings.ToLower(f.Path), ".go") { | ||
| continue | ||
| } | ||
| if strings.HasSuffix(strings.ToLower(f.Path), "_test.go") { | ||
| continue | ||
| } | ||
|
|
||
| for _, h := range f.Hunks { | ||
| var hasWmic bool | ||
| var utf8Lines []diff.Line | ||
| var hunkText strings.Builder | ||
| for _, ln := range h.Lines { | ||
| if ln.Kind == diff.LineAdd { | ||
| c := ln.Content | ||
| hunkText.WriteString(c) | ||
| hunkText.WriteByte('\n') | ||
| if wmicRe.MatchString(c) { | ||
| hasWmic = true | ||
| } | ||
| if utf8OpRe.MatchString(c) { | ||
| utf8Lines = append(utf8Lines, ln) | ||
| } | ||
| } | ||
| } | ||
| if !hasWmic || len(utf8Lines) == 0 { | ||
| continue | ||
| } | ||
| if decodeRe.MatchString(hunkText.String()) { | ||
| continue |
There was a problem hiding this comment.
Suggestion: The decode guard is overbroad because it matches generic tokens like BOM in the entire hunk text, including comments. A comment such as // TODO: handle BOM can suppress a finding without any real UTF-16/BOM decoding; require a concrete decode operation pattern instead. [incorrect condition logic]
Severity Level: Major ⚠️
- ⚠️ SLP222 may skip WMIC findings due to BOM comments.
- ⚠️ Silent UTF-16/UTF-8 corruption risk can remain undetected.Steps of Reproduction ✅
1. Prepare a git diff for a Go file `snapnative.go` that adds a WMIC call
`exec.Command("wmic", "process", "list", "brief").Output()` and then immediately calls
`utf8.Valid(out)` or `string(out)` on the result, without any actual UTF-16 or BOM
decoding logic, mirroring the pattern used in `TestSLP222_FlagsWmicOutputWithoutDecode`
(`pkg/rules/slp222_test.go:7-23`).
2. In the same added hunk, include only a comment like `// TODO: handle BOM correctly`
near the WMIC handling code but no actual decoding (no `utf16`/`unicode/utf16` usage).
3. Run `slopgate` so `run()` in `cmd/slopgate/main.go:42-187` parses the diff with
`diff.Parse()` (`pkg/diff/parser.go:110-193`) and executes `rules.Default().Run(parsed,
cfg)` (`pkg/rules/rule.go:164-241`), which calls `SLP222.Check()`
(`pkg/rules/slp222.go:26-71`).
4. Inside `SLP222.Check`, the WMIC line and the `utf8.Valid(out)` line are captured as
`hasWmic` and `utf8Lines` (lines 36-49), and the entire added hunk text is accumulated
into `hunkText` (lines 39-44); `decodeRe` at `pkg/rules/slp222.go:24` matches the word
`BOM` in the comment alone, so the `if decodeRe.MatchString(hunkText.String()) { continue
}` check at lines 56-57 causes the rule to emit no findings, even though there is still no
real UTF-16/BOM decoding in the code.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** pkg/rules/slp222.go
**Line:** 24:57
**Comment:**
*Incorrect Condition Logic: The decode guard is overbroad because it matches generic tokens like `BOM` in the entire hunk text, including comments. A comment such as `// TODO: handle BOM` can suppress a finding without any real UTF-16/BOM decoding; require a concrete decode operation pattern instead.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+32
to
+35
| + // Decode UTF-16 BOM | ||
| + if len(out) >= 3 && out[0] == 0xEF && out[1] == 0xBB && out[2] == 0xBF { | ||
| + out = out[3:] | ||
| + } |
There was a problem hiding this comment.
Suggestion: This test claims to validate UTF-16 BOM handling but checks for UTF-8 BOM bytes (EF BB BF), which does not verify the real WMIC UTF-16LE case (FF FE/FE FF). Update the fixture to assert actual UTF-16 BOM handling so the rule is tested against the intended encoding bug. [logic error]
Severity Level: Major ⚠️
- ⚠️ Test asserts BOM handling using UTF-8 bytes, not UTF-16.
- ⚠️ WMIC UTF-16 BOM handling behavior remains unverified by tests.Steps of Reproduction ✅
1. Run `go test ./...` from the repository root, which executes
`TestSLP222_NoFlagWithBOMDecode` in `pkg/rules/slp222_test.go:25-45` along with the rest
of the test suite.
2. This test constructs a diff where WMIC output is assigned to `out` and then a comment
`// Decode UTF-16 BOM` precedes a guard `if len(out) >= 3 && out[0] == 0xEF && out[1] ==
0xBB && out[2] == 0xBF { out = out[3:] }` (`pkg/rules/slp222_test.go:30-35`).
3. The test then calls `SLP222.Check()` and asserts that no findings are produced when
this purported BOM handling is present (`pkg/rules/slp222_test.go:41-44`), implicitly
treating the byte sequence as a correct decode of WMIC's BOM.
4. However, 0xEF 0xBB 0xBF is the UTF-8 BOM, not the UTF-16LE/BE BOM that WMIC actually
emits (which is 0xFF 0xFE or 0xFE 0xFF). As a result, the test validates suppression based
on handling the wrong encoding, so it does not exercise or verify the intended UTF-16 BOM
scenario described in `SLP222`'s comment (`pkg/rules/slp222.go:10-13`), leaving the rule's
behavior for true UTF-16 BOM handling untested.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** pkg/rules/slp222_test.go
**Line:** 32:35
**Comment:**
*Logic Error: This test claims to validate UTF-16 BOM handling but checks for UTF-8 BOM bytes (`EF BB BF`), which does not verify the real WMIC UTF-16LE case (`FF FE`/`FE FF`). Update the fixture to assert actual UTF-16 BOM handling so the rule is tested against the intended encoding bug.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
CodeAnt AI finished reviewing your PR. |
…ve on example logger calls
|
Seems you are using me but didn't get OPENAI_API_KEY seted in Variables/Secrets for this repo. you could follow readme for more information |
|
Seems you are using me but didn't get OPENAI_API_KEY seted in Variables/Secrets for this repo. you could follow readme for more information |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/rules/registry_test.go (1)
172-200:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd missing rules to test verification arrays.
The
newRulesarray does not include SLP215-SLP222, which meansTestDefault_NoExtraRuleswill not verify that these eight rules are actually registered. Similarly,TestDefault_RegistersAllV001Rules(lines 5-156) also does not include these rules in itswantmap.🧪 Proposed fix to add missing rule IDs
Add the missing rule IDs to the
newRulesarray:"SLP208", "SLP209", + "SLP215", "SLP216", "SLP217", "SLP218", + "SLP219", "SLP220", "SLP221", "SLP222", }Also update
TestDefault_RegistersAllV001Rulesto include these rules in thewantmap (after line 144):"SLP208": false, "SLP209": false, + "SLP215": false, + "SLP216": false, + "SLP217": false, + "SLP218": false, + "SLP219": false, + "SLP220": false, + "SLP221": false, + "SLP222": false, }🤖 Prompt for 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. In `@pkg/rules/registry_test.go` around lines 172 - 200, The test arrays are missing rule IDs SLP215 through SLP222; update the newRules slice used by TestDefault_NoExtraRules to include "SLP215","SLP216","SLP217","SLP218","SLP219","SLP220","SLP221","SLP222", and also add those same IDs into the want map inside TestDefault_RegistersAllV001Rules so both the newRules variable and the want map verify these eight rules are registered; locate newRules, TestDefault_NoExtraRules, TestDefault_RegistersAllV001Rules and the want map in pkg/rules/registry_test.go and append the SLP215–SLP222 entries consistently.
🧹 Nitpick comments (1)
pkg/rules/slp221_test.go (1)
8-77: ⚡ Quick winAdd regressions for
CombinedOutput()and mixed-command hunks.Current tests don’t cover the two highest-risk paths in this matcher:
CombinedOutput()(should be non-finding) and one-wired/one-unwired command pairs in the same hunk.🤖 Prompt for 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. In `@pkg/rules/slp221_test.go` around lines 8 - 77, Tests for SLP221 are missing regressions for exec.Cmd.CombinedOutput() (which should not trigger a finding) and for mixed hunks containing both “wired” (stderr handled) and “unwired” commands in the same diff; add two new table tests in pkg/rules/slp221_test.go: one that parses a diff with cmd := exec.Command(...); return cmd.CombinedOutput() and asserts zero findings, and another that parses a single hunk containing two added commands where one handles stderr (e.g., uses StderrPipe or assigns cmd.Stderr) and the other does not, then assert the matcher reports exactly one finding for the unwired command (and optionally check the finding message references stderr). Reference the existing test helpers parseDiff and the SLP221{}.Check(d) call to implement these cases.
🤖 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/slp219.go`:
- Line 28: sharedFieldRe only matches single-letter receiver names; update its
pattern to match full Go identifiers so multi-character names like svc.Config or
handler.Store are detected. Replace the group ([a-zA-Z]) with a proper
identifier pattern such as ([a-zA-Z_][a-zA-Z0-9_]*) so sharedFieldRe will be
regexp.MustCompile(`\b([a-zA-Z_][a-zA-Z0-9_]*)\.(Manager|Store|State|Config|Registry|Cache|Pool|Snapshot[A-Z][a-zA-Z]*)\b`),
keeping the rest of the token checks unchanged.
- Around line 47-68: The handler/lock detection currently tests handlerContextRe
and lockCallRe against only added lines (built via addedText/addedLines),
missing context from unchanged lines and causing false negatives/positives;
change the logic in the hunk loop (where h.Lines is iterated) to build the full
hunk string from all h.Lines (include unchanged, removed, and added
line.Content) and use that full hunkStr for handlerContextRe.MatchString and
lockCallRe.MatchString checks instead of the added-only hunkStr, while still
keeping the addedLines filter for the "no additions" continue case.
In `@pkg/rules/slp220.go`:
- Around line 41-52: The current logic that sets hunkHasCtx by scanning all
f.Hunks and including deleted lines is too broad; update the check so it only
scans the same hunk as the matched filepath.Walk* call (i.e., restrict the loop
to the specific hunk containing ln rather than iterating all f.Hunks) and skip
any diff.LineDelete entries when testing ctxErrCheckRe against hln.Content (use
the hln.Type or equivalent to ignore deleted lines). Ensure you still break out
early when hunkHasCtx is found and keep using the existing variables (f.Hunks,
h.Lines, hln.NewLineNo, ln.NewLineNo, ctxErrCheckRe, hunkHasCtx) to locate and
modify the code.
In `@pkg/rules/slp221.go`:
- Around line 41-64: The current logic uses stderrWireRe on the whole hunkText
and suppresses findings for all command instances in a hunk; instead, extract
the specific command variable identifier from the exec line (the match captured
by execCommandContextRe or execRunRe, e.g., the variable name assigned by "cmd
:= exec.Command..." or the receiver used in "cmd.Run/Output"), then only
consider stderr wiring lines that reference that same identifier when deciding
to continue; update the loop that iterates f.Hunks (and the variables
execLine/runLine/hunkText) to parse the command variable name from execLine or
runLine and search h.Lines for diff.LineAdd entries that match stderrWireRe AND
include that identifier, leaving other command instances unaffected.
- Line 28: The regex execRunRe currently matches ".CombinedOutput()" and thus
produces false positives; update its pattern in the execRunRe definition to stop
matching CombinedOutput (e.g., remove "CombinedOutput" from the alternation so
it only matches ".Output" or ".Run"), then run/update any tests that assert on
execRunRe to ensure CombinedOutput is no longer flagged.
In `@pkg/rules/slp222_test.go`:
- Around line 33-35: The test is checking for a UTF-8 BOM (0xEF,0xBB,0xBF) but
WMIC uses UTF-16LE; update the fixture handling in pkg/rules/slp222_test.go so
the test looks for the UTF-16LE BOM (0xFF,0xFE) on the byte slice named out,
strip those two BOM bytes, and then decode the remaining UTF-16LE bytes into
UTF-8 before assertions; replace or remove the current UTF-8 BOM branch and
ensure the decoding step uses the appropriate UTF-16LE -> UTF-8 conversion so
the test actually exercises the WMIC mitigation.
In `@pkg/rules/slp222.go`:
- Around line 24-57: The current decode suppression is too broad: decodeRe
matches generic tokens (e.g., "BOM") and you scan the whole hunkText so a
comment or import can silence a real UTF-16 find; tighten decodeRe to target
concrete decode APIs/paths (e.g., explicit decoder functions or package+function
names used to transcode UTF-16) and change the detection in SLP222.Check to only
consider added, non-comment executable lines (filter each ln.Content in h.Lines
for comment-only lines and skip them) and/or test decodeRe against the utf8Lines
slice (or the concatenated added non-comment lines) instead of the full hunkText
to avoid comment-based bypasses.
---
Outside diff comments:
In `@pkg/rules/registry_test.go`:
- Around line 172-200: The test arrays are missing rule IDs SLP215 through
SLP222; update the newRules slice used by TestDefault_NoExtraRules to include
"SLP215","SLP216","SLP217","SLP218","SLP219","SLP220","SLP221","SLP222", and
also add those same IDs into the want map inside
TestDefault_RegistersAllV001Rules so both the newRules variable and the want map
verify these eight rules are registered; locate newRules,
TestDefault_NoExtraRules, TestDefault_RegistersAllV001Rules and the want map in
pkg/rules/registry_test.go and append the SLP215–SLP222 entries consistently.
---
Nitpick comments:
In `@pkg/rules/slp221_test.go`:
- Around line 8-77: Tests for SLP221 are missing regressions for
exec.Cmd.CombinedOutput() (which should not trigger a finding) and for mixed
hunks containing both “wired” (stderr handled) and “unwired” commands in the
same diff; add two new table tests in pkg/rules/slp221_test.go: one that parses
a diff with cmd := exec.Command(...); return cmd.CombinedOutput() and asserts
zero findings, and another that parses a single hunk containing two added
commands where one handles stderr (e.g., uses StderrPipe or assigns cmd.Stderr)
and the other does not, then assert the matcher reports exactly one finding for
the unwired command (and optionally check the finding message references
stderr). Reference the existing test helpers parseDiff and the SLP221{}.Check(d)
call to implement these cases.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 66870684-0b0a-4498-9cb9-960d44cd5291
📒 Files selected for processing (23)
README.mdpkg/rules/registry.gopkg/rules/registry_test.gopkg/rules/slp160.gopkg/rules/slp160_test.gopkg/rules/slp161.gopkg/rules/slp162.gopkg/rules/slp162_test.gopkg/rules/slp210.gopkg/rules/slp211.gopkg/rules/slp212.gopkg/rules/slp213.gopkg/rules/slp214.gopkg/rules/slp216_test.gopkg/rules/slp217.gopkg/rules/slp219.gopkg/rules/slp219_test.gopkg/rules/slp220.gopkg/rules/slp220_test.gopkg/rules/slp221.gopkg/rules/slp221_test.gopkg/rules/slp222.gopkg/rules/slp222_test.go
| } | ||
|
|
||
| // sharedFieldRe matches s.X, r.X, h.X where X looks like shared mutable state. | ||
| var sharedFieldRe = regexp.MustCompile(`\b([a-zA-Z])\.(Manager|Store|State|Config|Registry|Cache|Pool|Snapshot[A-Z][a-zA-Z]*)\b`) |
There was a problem hiding this comment.
sharedFieldRe currently misses most receiver names.
The receiver part ([a-zA-Z])\. only matches one-letter identifiers. Common forms like svc.Config or handler.Store won’t be detected.
Suggested patch
-var sharedFieldRe = regexp.MustCompile(`\b([a-zA-Z])\.(Manager|Store|State|Config|Registry|Cache|Pool|Snapshot[A-Z][a-zA-Z]*)\b`)
+var sharedFieldRe = regexp.MustCompile(`\b([a-zA-Z_][a-zA-Z0-9_]*)\.(Manager|Store|State|Config|Registry|Cache|Pool|Snapshot[A-Z][a-zA-Z]*)\b`)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| var sharedFieldRe = regexp.MustCompile(`\b([a-zA-Z])\.(Manager|Store|State|Config|Registry|Cache|Pool|Snapshot[A-Z][a-zA-Z]*)\b`) | |
| var sharedFieldRe = regexp.MustCompile(`\b([a-zA-Z_][a-zA-Z0-9_]*)\.(Manager|Store|State|Config|Registry|Cache|Pool|Snapshot[A-Z][a-zA-Z]*)\b`) |
🤖 Prompt for 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.
In `@pkg/rules/slp219.go` at line 28, sharedFieldRe only matches single-letter
receiver names; update its pattern to match full Go identifiers so
multi-character names like svc.Config or handler.Store are detected. Replace the
group ([a-zA-Z]) with a proper identifier pattern such as
([a-zA-Z_][a-zA-Z0-9_]*) so sharedFieldRe will be
regexp.MustCompile(`\b([a-zA-Z_][a-zA-Z0-9_]*)\.(Manager|Store|State|Config|Registry|Cache|Pool|Snapshot[A-Z][a-zA-Z]*)\b`),
keeping the rest of the token checks unchanged.
Comment on lines
+47
to
+68
| var addedText strings.Builder | ||
| var addedLines []diff.Line | ||
| for _, ln := range h.Lines { | ||
| if ln.Kind == diff.LineAdd { | ||
| addedText.WriteString(ln.Content) | ||
| addedText.WriteByte('\n') | ||
| addedLines = append(addedLines, ln) | ||
| } | ||
| } | ||
| if len(addedLines) == 0 { | ||
| continue | ||
| } | ||
|
|
||
| hunkStr := addedText.String() | ||
| // Only care if hunk is in handler context. | ||
| if !handlerContextRe.MatchString(hunkStr) { | ||
| continue | ||
| } | ||
| // If hunk takes a lock, assume safe. | ||
| if lockCallRe.MatchString(hunkStr) { | ||
| continue | ||
| } |
There was a problem hiding this comment.
Use full hunk context for handler/lock evidence.
handlerContextRe and lockCallRe are evaluated on added lines only, so body-only handler edits and existing lock context are missed. That creates avoidable false negatives and false positives.
Suggested patch
- var addedText strings.Builder
+ var hunkText strings.Builder
var addedLines []diff.Line
for _, ln := range h.Lines {
+ if ln.Kind != diff.LineDelete {
+ hunkText.WriteString(ln.Content)
+ hunkText.WriteByte('\n')
+ }
if ln.Kind == diff.LineAdd {
- addedText.WriteString(ln.Content)
- addedText.WriteByte('\n')
addedLines = append(addedLines, ln)
}
}
@@
- hunkStr := addedText.String()
+ hunkStr := hunkText.String()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| var addedText strings.Builder | |
| var addedLines []diff.Line | |
| for _, ln := range h.Lines { | |
| if ln.Kind == diff.LineAdd { | |
| addedText.WriteString(ln.Content) | |
| addedText.WriteByte('\n') | |
| addedLines = append(addedLines, ln) | |
| } | |
| } | |
| if len(addedLines) == 0 { | |
| continue | |
| } | |
| hunkStr := addedText.String() | |
| // Only care if hunk is in handler context. | |
| if !handlerContextRe.MatchString(hunkStr) { | |
| continue | |
| } | |
| // If hunk takes a lock, assume safe. | |
| if lockCallRe.MatchString(hunkStr) { | |
| continue | |
| } | |
| var hunkText strings.Builder | |
| var addedLines []diff.Line | |
| for _, ln := range h.Lines { | |
| if ln.Kind != diff.LineDelete { | |
| hunkText.WriteString(ln.Content) | |
| hunkText.WriteByte('\n') | |
| } | |
| if ln.Kind == diff.LineAdd { | |
| addedLines = append(addedLines, ln) | |
| } | |
| } | |
| if len(addedLines) == 0 { | |
| continue | |
| } | |
| hunkStr := hunkText.String() | |
| // Only care if hunk is in handler context. | |
| if !handlerContextRe.MatchString(hunkStr) { | |
| continue | |
| } | |
| // If hunk takes a lock, assume safe. | |
| if lockCallRe.MatchString(hunkStr) { | |
| continue | |
| } |
🤖 Prompt for 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.
In `@pkg/rules/slp219.go` around lines 47 - 68, The handler/lock detection
currently tests handlerContextRe and lockCallRe against only added lines (built
via addedText/addedLines), missing context from unchanged lines and causing
false negatives/positives; change the logic in the hunk loop (where h.Lines is
iterated) to build the full hunk string from all h.Lines (include unchanged,
removed, and added line.Content) and use that full hunkStr for
handlerContextRe.MatchString and lockCallRe.MatchString checks instead of the
added-only hunkStr, while still keeping the addedLines filter for the "no
additions" continue case.
Comment on lines
+41
to
+52
| for _, h := range f.Hunks { | ||
| for _, hln := range h.Lines { | ||
| if hln.NewLineNo >= ln.NewLineNo-5 && hln.NewLineNo <= ln.NewLineNo+80 && | ||
| ctxErrCheckRe.MatchString(hln.Content) { | ||
| hunkHasCtx = true | ||
| break | ||
| } | ||
| } | ||
| if hunkHasCtx { | ||
| break | ||
| } | ||
| } |
There was a problem hiding this comment.
Cancellation detection is over-broad and can suppress true findings.
The search currently scans all hunks and includes deleted lines; any nearby ctx.Err() text can mark a walk call as safe even when the callback itself is non-cancellable.
Constrain checks to the same hunk/callback region as the matched filepath.Walk* call, and ignore diff.LineDelete content when deriving safety evidence.
🤖 Prompt for 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.
In `@pkg/rules/slp220.go` around lines 41 - 52, The current logic that sets
hunkHasCtx by scanning all f.Hunks and including deleted lines is too broad;
update the check so it only scans the same hunk as the matched filepath.Walk*
call (i.e., restrict the loop to the specific hunk containing ln rather than
iterating all f.Hunks) and skip any diff.LineDelete entries when testing
ctxErrCheckRe against hln.Content (use the hln.Type or equivalent to ignore
deleted lines). Ensure you still break out early when hunkHasCtx is found and
keep using the existing variables (f.Hunks, h.Lines, hln.NewLineNo,
ln.NewLineNo, ctxErrCheckRe, hunkHasCtx) to locate and modify the code.
| } | ||
|
|
||
| var execCommandContextRe = regexp.MustCompile(`\bexec\.Command(Context)?\s*\(`) | ||
| var execRunRe = regexp.MustCompile(`\.(Output|Run|CombinedOutput)\s*\(\s*\)`) |
There was a problem hiding this comment.
Do not flag CombinedOutput() as missing stderr capture.
CombinedOutput() already returns stderr together with stdout, so including it in execRunRe creates false positives.
Suggested patch
-var execRunRe = regexp.MustCompile(`\.(Output|Run|CombinedOutput)\s*\(\s*\)`)
+var execRunRe = regexp.MustCompile(`\.(Output|Run)\s*\(\s*\)`)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| var execRunRe = regexp.MustCompile(`\.(Output|Run|CombinedOutput)\s*\(\s*\)`) | |
| var execRunRe = regexp.MustCompile(`\.(Output|Run)\s*\(\s*\)`) |
🤖 Prompt for 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.
In `@pkg/rules/slp221.go` at line 28, The regex execRunRe currently matches
".CombinedOutput()" and thus produces false positives; update its pattern in the
execRunRe definition to stop matching CombinedOutput (e.g., remove
"CombinedOutput" from the alternation so it only matches ".Output" or ".Run"),
then run/update any tests that assert on execRunRe to ensure CombinedOutput is
no longer flagged.
Comment on lines
+41
to
+64
| for _, h := range f.Hunks { | ||
| var execLine *diff.Line | ||
| var runLine *diff.Line | ||
| hunkText := "" | ||
| for _, ln := range h.Lines { | ||
| if ln.Kind == diff.LineAdd { | ||
| c := ln.Content | ||
| hunkText += c + "\n" | ||
| if execCommandContextRe.MatchString(c) { | ||
| cp := ln | ||
| execLine = &cp | ||
| } | ||
| if execRunRe.MatchString(c) { | ||
| cp := ln | ||
| runLine = &cp | ||
| } | ||
| } | ||
| } | ||
| if execLine == nil || runLine == nil { | ||
| continue | ||
| } | ||
| if stderrWireRe.MatchString(hunkText) { | ||
| continue | ||
| } |
There was a problem hiding this comment.
Track stderr wiring per command instance, not per hunk.
A single StderrPipe/cmd.Stderr = ... anywhere in hunkText suppresses all findings in that hunk, even for other command variables that still drop stderr.
Use command-variable correlation (cmd := exec.Command... → cmd.Run/Output → cmd.Stderr*) instead of hunk-global suppression.
🤖 Prompt for 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.
In `@pkg/rules/slp221.go` around lines 41 - 64, The current logic uses
stderrWireRe on the whole hunkText and suppresses findings for all command
instances in a hunk; instead, extract the specific command variable identifier
from the exec line (the match captured by execCommandContextRe or execRunRe,
e.g., the variable name assigned by "cmd := exec.Command..." or the receiver
used in "cmd.Run/Output"), then only consider stderr wiring lines that reference
that same identifier when deciding to continue; update the loop that iterates
f.Hunks (and the variables execLine/runLine/hunkText) to parse the command
variable name from execLine or runLine and search h.Lines for diff.LineAdd
entries that match stderrWireRe AND include that identifier, leaving other
command instances unaffected.
Comment on lines
+33
to
+35
| + if len(out) >= 3 && out[0] == 0xEF && out[1] == 0xBB && out[2] == 0xBF { | ||
| + out = out[3:] | ||
| + } |
There was a problem hiding this comment.
This fixture uses UTF-8 BOM bytes, not UTF-16LE BOM.
0xEF 0xBB 0xBF is UTF-8 BOM. For WMIC’s UTF-16LE path, this test should exercise UTF-16LE handling (0xFF 0xFE + decode), otherwise it blesses an incorrect mitigation.
🤖 Prompt for 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.
In `@pkg/rules/slp222_test.go` around lines 33 - 35, The test is checking for a
UTF-8 BOM (0xEF,0xBB,0xBF) but WMIC uses UTF-16LE; update the fixture handling
in pkg/rules/slp222_test.go so the test looks for the UTF-16LE BOM (0xFF,0xFE)
on the byte slice named out, strip those two BOM bytes, and then decode the
remaining UTF-16LE bytes into UTF-8 before assertions; replace or remove the
current UTF-8 BOM branch and ensure the decoding step uses the appropriate
UTF-16LE -> UTF-8 conversion so the test actually exercises the WMIC mitigation.
Comment on lines
+24
to
+57
| var decodeRe = regexp.MustCompile(`(?i)\b(utf16|unicode/bom|BOM|ByteOrderMark|unicode\/utf16|golang\.org\/x\/text\/encoding\/unicode)`) | ||
|
|
||
| func (r SLP222) Check(d *diff.Diff) []Finding { | ||
| var out []Finding | ||
| for _, f := range d.Files { | ||
| if f.IsDelete || !strings.HasSuffix(strings.ToLower(f.Path), ".go") { | ||
| continue | ||
| } | ||
| if strings.HasSuffix(strings.ToLower(f.Path), "_test.go") { | ||
| continue | ||
| } | ||
|
|
||
| for _, h := range f.Hunks { | ||
| var hasWmic bool | ||
| var utf8Lines []diff.Line | ||
| var hunkText strings.Builder | ||
| for _, ln := range h.Lines { | ||
| if ln.Kind == diff.LineAdd { | ||
| c := ln.Content | ||
| hunkText.WriteString(c) | ||
| hunkText.WriteByte('\n') | ||
| if wmicRe.MatchString(c) { | ||
| hasWmic = true | ||
| } | ||
| if utf8OpRe.MatchString(c) { | ||
| utf8Lines = append(utf8Lines, ln) | ||
| } | ||
| } | ||
| } | ||
| if !hasWmic || len(utf8Lines) == 0 { | ||
| continue | ||
| } | ||
| if decodeRe.MatchString(hunkText.String()) { | ||
| continue |
There was a problem hiding this comment.
Decode suppression is too broad and can be bypassed by comments.
decodeRe includes generic tokens like BOM, and matching against full hunkText means a comment/import string can silence a valid finding without real UTF-16 decoding.
Tighten decodeRe to concrete decode operations and evaluate decode evidence from executable added lines (exclude comment-only matches).
🤖 Prompt for 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.
In `@pkg/rules/slp222.go` around lines 24 - 57, The current decode suppression is
too broad: decodeRe matches generic tokens (e.g., "BOM") and you scan the whole
hunkText so a comment or import can silence a real UTF-16 find; tighten decodeRe
to target concrete decode APIs/paths (e.g., explicit decoder functions or
package+function names used to transcode UTF-16) and change the detection in
SLP222.Check to only consider added, non-comment executable lines (filter each
ln.Content in h.Lines for comment-only lines and skip them) and/or test decodeRe
against the utf8Lines slice (or the concatenated added non-comment lines)
instead of the full hunkText to avoid comment-based bypasses.
|
Seems you are using me but didn't get OPENAI_API_KEY seted in Variables/Secrets for this repo. you could follow readme for more information |
messagesgoel-blip
added
the
no-roadmap-task
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.
User description
Summary
Test plan
go test ./...passes (15 new tests + full regression)🤖 Generated with OpenClaude
Summary by Sourcery
Add four new reviewer gap closure rules to the rules registry and tests, expanding static analysis coverage for concurrency, filesystem walks, subprocess stderr handling, and UTF-16/BOM output parsing.
New Features:
Documentation:
Tests:
CodeAnt-AI Description
Add four reviewer gap closure rules for shared-state races, cancellable walks, missing subprocess stderr, and UTF-16 output decoding
What Changed
Impact
✅ Earlier warnings for race-prone handler code✅ Fewer long-running directory walks✅ Clearer subprocess failure messages✅ Fewer misread Windows command outputs💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.
Summary by CodeRabbit
New Features
Documentation
Tests
Style