From 7ba876a0eb007fdda828f9da5fa02043d04ea338 Mon Sep 17 00:00:00 2001 From: CodeRabbit Fix Date: Wed, 10 Jun 2026 13:04:04 -0400 Subject: [PATCH 1/5] feat(rules): add SLP219-222 reviewer gap closure v3 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. --- README.md | 4 +- pkg/rules/registry.go | 4 ++ pkg/rules/registry_test.go | 4 +- pkg/rules/slp219.go | 85 ++++++++++++++++++++++++++++++++++++++ pkg/rules/slp219_test.go | 73 ++++++++++++++++++++++++++++++++ pkg/rules/slp220.go | 67 ++++++++++++++++++++++++++++++ pkg/rules/slp220_test.go | 78 ++++++++++++++++++++++++++++++++++ pkg/rules/slp221.go | 77 ++++++++++++++++++++++++++++++++++ pkg/rules/slp221_test.go | 76 ++++++++++++++++++++++++++++++++++ pkg/rules/slp222.go | 72 ++++++++++++++++++++++++++++++++ pkg/rules/slp222_test.go | 63 ++++++++++++++++++++++++++++ 11 files changed, 599 insertions(+), 4 deletions(-) create mode 100644 pkg/rules/slp219.go create mode 100644 pkg/rules/slp219_test.go create mode 100644 pkg/rules/slp220.go create mode 100644 pkg/rules/slp220_test.go create mode 100644 pkg/rules/slp221.go create mode 100644 pkg/rules/slp221_test.go create mode 100644 pkg/rules/slp222.go create mode 100644 pkg/rules/slp222_test.go diff --git a/README.md b/README.md index ff5dd6a..8ef487d 100644 --- a/README.md +++ b/README.md @@ -105,14 +105,14 @@ With shallow clones, fetch full history (`fetch-depth: 0`) so the base ref resol ## Rules -slopgate ships 156 registered rules (10 quarantined). `slopgate --list-rules` prints the authoritative catalog with each rule's ID, severity, description, and quarantine status. +slopgate ships 164 registered rules (10 quarantined). `slopgate --list-rules` prints the authoritative catalog with each rule's ID, severity, description, and quarantine status. | Family | IDs | Focus | |---|---|---| | Core diff checks | `SLP001`–`SLP070` | test quality, code hygiene, safety, API and data smells | | Go AST checks | `SLP071`–`SLP080` | Go semantic hazards — nil, SQL injection, races, ignored errors | | Extended checks | `SLP081`–`SLP162` | framework, API, auth, audit, pagination, concurrency, dead-code, test-completeness, parseInt truncation, useEffect FOUC, code-quality splits (SLP160–162) | -| Reviewer gap closure | `SLP210`–`SLP218` | conflicting Tailwind utilities, setState-before-async, double-submit race, regex empty-match, React Query no-error-check, OpenAPI spec drift, shallow error logging, empty path params, missing chunked transfer handling | +| Reviewer gap closure | `SLP210`–`SLP222` | conflicting Tailwind utilities, setState-before-async, double-submit race, regex empty-match, React Query no-error-check, OpenAPI spec drift, shallow error logging, empty path params, missing chunked transfer handling, data race on shared state, filepath.Walk without ctx cancellation, exec.Command without stderr, UTF-16/BOM without decoding | | Semantic bug checks | `SLP202`–`SLP209` | high-signal runtime bugs — nil dereference, DB constraints, OpenAPI merge-order, swallowed promises, missing rollbacks, default-param ordering, async arrow missing returns | ### Quarantined rules diff --git a/pkg/rules/registry.go b/pkg/rules/registry.go index 0b623c3..335e79b 100644 --- a/pkg/rules/registry.go +++ b/pkg/rules/registry.go @@ -190,5 +190,9 @@ func Default() *Registry { r.Register(SLP216{}) // error logging uses err.message instead of full error object r.Register(SLP217{}) // path-like parameter not validated for empty input r.Register(SLP218{}) // ContentLength gate without Transfer-Encoding handling + r.Register(SLP219{}) // concurrent access to shared state field without lock + r.Register(SLP220{}) // filepath.Walk without context cancellation check + r.Register(SLP221{}) // exec.Command without capturing stderr on failure + r.Register(SLP222{}) // treating UTF-16/BOM data as UTF-8 without decoding return r } diff --git a/pkg/rules/registry_test.go b/pkg/rules/registry_test.go index 7f1e289..ebc1d5b 100644 --- a/pkg/rules/registry_test.go +++ b/pkg/rules/registry_test.go @@ -158,8 +158,8 @@ func TestDefault_RegistersAllV001Rules(t *testing.T) { func TestDefault_NoExtraRules(t *testing.T) { r := Default() - // Includes SLP202-SLP205, SLP207, the P3 rules SLP151, SLP152, new precision rules SLP155-159, SLP035 split rules SLP160-162, reviewer gap rules SLP210-214, and reviewer gap closure v2 rules SLP215-218. - wantCount := 160 + // Includes SLP202-SLP205, SLP207, the P3 rules SLP151, SLP152, new precision rules SLP155-159, SLP035 split rules SLP160-162, reviewer gap rules SLP210-214, reviewer gap closure v2 rules SLP215-218, and reviewer gap closure v3 rules SLP219-222. + wantCount := 164 if got := len(r.All()); got != wantCount { t.Errorf("Default registry has %d rules, want %d", got, wantCount) } diff --git a/pkg/rules/slp219.go b/pkg/rules/slp219.go new file mode 100644 index 0000000..03690fe --- /dev/null +++ b/pkg/rules/slp219.go @@ -0,0 +1,85 @@ +package rules + +import ( + "regexp" + "strings" + + "github.com/messagesgoel-blip/slopgate/pkg/diff" +) + +// SLP219 flags access to shared state fields (e.g. *Manager, *Store, *State) +// in an HTTP handler without holding a mutex. Whimsy PR #1952 reviewers flagged +// `s.SnapshotManager` read/write in handlers that didn't call s.mu.Lock() or +// s.mu.RLock() anywhere in the hunk. +// +// Heuristic: +// - Added line accesses a field named Manager|Store|State|Config|Registry +// - Same hunk is in an HTTP handler (has http.ResponseWriter / *http.Request) +// - Hunk does NOT contain any .Lock() / .RLock() / atomic.Load*/Store* call +type SLP219 struct{} + +func (SLP219) ID() string { return "SLP219" } +func (SLP219) DefaultSeverity() Severity { return SeverityWarn } +func (SLP219) Description() string { + return "accessing shared-state field in handler without lock — data race risk" +} + +// 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`) + +// handlerContextRe identifies an HTTP handler via parameter types. +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)`) + +func (r SLP219) 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 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 + } + + for _, ln := range addedLines { + if sharedFieldRe.MatchString(ln.Content) { + out = append(out, Finding{ + RuleID: r.ID(), + Severity: r.DefaultSeverity(), + File: f.Path, + Line: ln.NewLineNo, + Message: "shared-state field access in handler without .Lock()/atomic — concurrent requests race", + Snippet: strings.TrimSpace(ln.Content), + }) + } + } + } + } + return out +} diff --git a/pkg/rules/slp219_test.go b/pkg/rules/slp219_test.go new file mode 100644 index 0000000..65af01c --- /dev/null +++ b/pkg/rules/slp219_test.go @@ -0,0 +1,73 @@ +package rules + +import ( + "strings" + "testing" +) + +func TestSLP219_FlagsSharedFieldInHandler(t *testing.T) { + d := parseDiff(t, `diff --git a/handlers.go b/handlers.go +--- a/handlers.go ++++ b/handlers.go +@@ -1,3 +1,7 @@ ++func SnapshotHandler(w http.ResponseWriter, r *http.Request) { ++ cfg := s.Config ++ _ = cfg ++} +`) + findings := SLP219{}.Check(d) + if len(findings) != 1 { + t.Errorf("expected 1 finding, got %d", len(findings)) + } +} + +func TestSLP219_NoFlagWithLock(t *testing.T) { + d := parseDiff(t, `diff --git a/handlers.go b/handlers.go +--- a/handlers.go ++++ b/handlers.go +@@ -1,3 +1,8 @@ ++func SnapshotHandler(w http.ResponseWriter, r *http.Request) { ++ s.mu.RLock() ++ defer s.mu.RUnlock() ++ cfg := s.Config ++} +`) + findings := SLP219{}.Check(d) + if len(findings) != 0 { + t.Errorf("expected 0 findings when lock is held, got %d", len(findings)) + } +} + +func TestSLP219_NoFlagNonHandler(t *testing.T) { + d := parseDiff(t, `diff --git a/internal.go b/internal.go +--- a/internal.go ++++ b/internal.go +@@ -1,2 +1,4 @@ ++func refresh() { ++ cfg := s.Config ++} +`) + findings := SLP219{}.Check(d) + if len(findings) != 0 { + t.Errorf("expected 0 findings in non-handler context, got %d", len(findings)) + } +} + +func TestSLP219_MessageMentionsRace(t *testing.T) { + d := parseDiff(t, `diff --git a/handlers.go b/handlers.go +--- a/handlers.go ++++ b/handlers.go +@@ -1,3 +1,5 @@ ++func SnapshotHandler(w http.ResponseWriter, r *http.Request) { ++ _ = s.SnapshotManager ++} +`) + findings := SLP219{}.Check(d) + if len(findings) == 0 { + t.Fatal("expected at least 1 finding") + } + lower := strings.ToLower(findings[0].Message) + if !strings.Contains(lower, "race") && !strings.Contains(lower, "lock") { + t.Errorf("expected message about race/lock, got: %s", findings[0].Message) + } +} diff --git a/pkg/rules/slp220.go b/pkg/rules/slp220.go new file mode 100644 index 0000000..e871b86 --- /dev/null +++ b/pkg/rules/slp220.go @@ -0,0 +1,67 @@ +package rules + +import ( + "regexp" + "strings" + + "github.com/messagesgoel-blip/slopgate/pkg/diff" +) + +// SLP220 flags a filepath.Walk or filepath.WalkDir call where the callback +// performs a long-running walk but doesn't check ctx.Err() inside. +// Whimsy PR #1961 reviewer flagged `treeSize()` that walked whole trees even +// after ctx.Done(). +type SLP220 struct{} + +func (SLP220) ID() string { return "SLP220" } +func (SLP220) DefaultSeverity() Severity { return SeverityWarn } +func (SLP220) Description() string { + return "filepath.Walk/WalkDir callback doesn't check ctx.Err() — walk not cancellable" +} + +var walkCallRe = regexp.MustCompile(`\bfilepath\.(Walk|WalkDir)\s*\(`) +var ctxErrCheckRe = regexp.MustCompile(`\bctx\.Err\(|context\.Canceled|ctx\.Done\(\)`) + +func (r SLP220) 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 _, ln := range f.AddedLines() { + if !walkCallRe.MatchString(ln.Content) { + continue + } + // Look at the whole hunk for ctx.Err() usage. + hunkHasCtx := false + 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 + } + } + if hunkHasCtx { + continue + } + out = append(out, Finding{ + RuleID: r.ID(), + Severity: r.DefaultSeverity(), + File: f.Path, + Line: ln.NewLineNo, + Message: "filepath.Walk without ctx.Err() check — walk can't be cancelled and may run to completion", + Snippet: strings.TrimSpace(ln.Content), + }) + } + } + return out +} diff --git a/pkg/rules/slp220_test.go b/pkg/rules/slp220_test.go new file mode 100644 index 0000000..d208f2b --- /dev/null +++ b/pkg/rules/slp220_test.go @@ -0,0 +1,78 @@ +package rules + +import ( + "strings" + "testing" +) + +func TestSLP220_FlagsWalkWithoutCtxCheck(t *testing.T) { + d := parseDiff(t, `diff --git a/walker.go b/walker.go +--- a/walker.go ++++ b/walker.go +@@ -1,3 +1,6 @@ ++func walkDir(root string) error { ++ return filepath.Walk(root, func(p string, info os.FileInfo, err error) error { ++ return nil ++ }) ++} +`) + findings := SLP220{}.Check(d) + if len(findings) != 1 { + t.Errorf("expected 1 finding, got %d", len(findings)) + } + if !strings.Contains(strings.ToLower(findings[0].Message), "cancel") { + t.Errorf("expected message about cancellation, got: %s", findings[0].Message) + } +} + +func TestSLP220_NoFlagWithCtxErrCheck(t *testing.T) { + d := parseDiff(t, `diff --git a/walker.go b/walker.go +--- a/walker.go ++++ b/walker.go +@@ -1,3 +1,8 @@ ++func walkDir(ctx context.Context, root string) error { ++ return filepath.Walk(root, func(p string, info os.FileInfo, err error) error { ++ if ctx.Err() != nil { ++ return ctx.Err() ++ } ++ return nil ++ }) ++} +`) + findings := SLP220{}.Check(d) + if len(findings) != 0 { + t.Errorf("expected 0 findings when ctx.Err() checked, got %d", len(findings)) + } +} + +func TestSLP220_NoFlagWithoutWalk(t *testing.T) { + d := parseDiff(t, `diff --git a/other.go b/other.go +--- a/other.go ++++ b/other.go +@@ -1,2 +1,4 @@ ++func process() { ++ // no walk here ++} +`) + findings := SLP220{}.Check(d) + if len(findings) != 0 { + t.Errorf("expected 0 findings without Walk call, got %d", len(findings)) + } +} + +func TestSLP220_FlagsWalkDirWithoutCtxCheck(t *testing.T) { + d := parseDiff(t, `diff --git a/walker.go b/walker.go +--- a/walker.go ++++ b/walker.go +@@ -1,3 +1,6 @@ ++func walkDir(root string) error { ++ return filepath.WalkDir(root, func(p string, d os.DirEntry, err error) error { ++ return nil ++ }) ++} +`) + findings := SLP220{}.Check(d) + if len(findings) != 1 { + t.Errorf("expected 1 finding for WalkDir, got %d", len(findings)) + } +} diff --git a/pkg/rules/slp221.go b/pkg/rules/slp221.go new file mode 100644 index 0000000..bfac736 --- /dev/null +++ b/pkg/rules/slp221.go @@ -0,0 +1,77 @@ +package rules + +import ( + "regexp" + "strings" + + "github.com/messagesgoel-blip/slopgate/pkg/diff" +) + +// SLP221 flags exec.Command / exec.CommandContext calls followed by +// .Output(), .Run(), or .CombinedOutput() where the hunk never wires up +// Stderr. On failure the exit-error body discards whatever the child +// printed to stderr, so error logs are opaque. +// +// Reviewer pattern (whimsy PR #1952 snapnative.go:105): sourcery-ai noted +// "Capturing and appending command stderr would help diagnose why +// the subprocess failed." Sourcery flagged Output() returning just +// a generic exit error. +type SLP221 struct{} + +func (SLP221) ID() string { return "SLP221" } +func (SLP221) DefaultSeverity() Severity { return SeverityWarn } +func (SLP221) Description() string { + return "exec.Command without Stderr/StderrPipe — failed subprocess stderr is lost" +} + +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*=)`) + +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", + Snippet: strings.TrimSpace(runLine.Content), + }) + } + } + return out +} diff --git a/pkg/rules/slp221_test.go b/pkg/rules/slp221_test.go new file mode 100644 index 0000000..eee75f0 --- /dev/null +++ b/pkg/rules/slp221_test.go @@ -0,0 +1,76 @@ +package rules + +import ( + "strings" + "testing" +) + +func TestSLP221_FlagsExecCommandWithoutStderr(t *testing.T) { + d := parseDiff(t, `diff --git a/runner.go b/runner.go +--- a/runner.go ++++ b/runner.go +@@ -1,3 +1,6 @@ ++func runBuild() error { ++ cmd := exec.Command("make", "build") ++ return cmd.Run() ++} +`) + findings := SLP221{}.Check(d) + if len(findings) != 1 { + t.Errorf("expected 1 finding, got %d", len(findings)) + } + if !strings.Contains(strings.ToLower(findings[0].Message), "stderr") { + t.Errorf("expected message about stderr, got: %s", findings[0].Message) + } +} + +func TestSLP221_NoFlagWithStderrPipe(t *testing.T) { + d := parseDiff(t, `diff --git a/runner.go b/runner.go +--- a/runner.go ++++ b/runner.go +@@ -1,3 +1,8 @@ ++func runBuild() error { ++ cmd := exec.Command("make", "build") ++ stderr, _ := cmd.StderrPipe() ++ defer stderr.Close() ++ return cmd.Run() ++} +`) + findings := SLP221{}.Check(d) + if len(findings) != 0 { + t.Errorf("expected 0 findings when StderrPipe used, got %d", len(findings)) + } +} + +func TestSLP221_NoFlagWithStderrAssignment(t *testing.T) { + d := parseDiff(t, `diff --git a/runner.go b/runner.go +--- a/runner.go ++++ b/runner.go +@@ -1,3 +1,7 @@ ++func runBuild() error { ++ cmd := exec.Command("make", "build") ++ cmd.Stderr = os.Stderr ++ return cmd.Run() ++} +`) + findings := SLP221{}.Check(d) + if len(findings) != 0 { + t.Errorf("expected 0 findings when Stderr assigned, got %d", len(findings)) + } +} + +func TestSLP221_FlagsWithContext(t *testing.T) { + d := parseDiff(t, `diff --git a/runner.go b/runner.go +--- a/runner.go ++++ b/runner.go +@@ -1,3 +1,6 @@ ++func runBuild(ctx context.Context) error { ++ cmd := exec.CommandContext(ctx, "make", "build") ++ return cmd.Output() ++} +`) + findings := SLP221{}.Check(d) + if len(findings) != 1 { + t.Errorf("expected 1 finding for CommandContext, got %d", len(findings)) + } +} diff --git a/pkg/rules/slp222.go b/pkg/rules/slp222.go new file mode 100644 index 0000000..5162831 --- /dev/null +++ b/pkg/rules/slp222.go @@ -0,0 +1,72 @@ +package rules + +import ( + "regexp" + "strings" + + "github.com/messagesgoel-blip/slopgate/pkg/diff" +) + +// SLP222 flags code that reads wmic (Windows) or subprocess output and +// runs utf8.Valid / string() on it without decoding UTF-16LE/BOM first. +// Whimsy PR #1952 snapnative.go:25 reviewer flagged "wmic outputs UTF-16LE +// with BOM by default — parsing it as UTF-8 silently loses non-ASCII." +type SLP222 struct{} + +func (SLP222) ID() string { return "SLP222" } +func (SLP222) DefaultSeverity() Severity { return SeverityWarn } +func (SLP222) Description() string { + return "subprocess output treated as UTF-8 without BOM/UTF-16 decode check" +} + +var wmicRe = regexp.MustCompile(`(?i)\bwmic( |\s+|")`) +var utf8OpRe = regexp.MustCompile(`\b(utf8\.Valid|utf8\.DecodeRune|string\s*\(\s*[^)]*out[^)]*\)|bytes\.ToString)`) +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 + } + for _, ln := range utf8Lines { + out = append(out, Finding{ + RuleID: r.ID(), + Severity: r.DefaultSeverity(), + File: f.Path, + Line: ln.NewLineNo, + Message: "output treated directly as UTF-8 — wmic produces UTF-16LE with BOM by default", + Snippet: strings.TrimSpace(ln.Content), + }) + } + } + } + return out +} diff --git a/pkg/rules/slp222_test.go b/pkg/rules/slp222_test.go new file mode 100644 index 0000000..417e097 --- /dev/null +++ b/pkg/rules/slp222_test.go @@ -0,0 +1,63 @@ +package rules + +import ( + "testing" +) + +func TestSLP222_FlagsWmicOutputWithoutDecode(t *testing.T) { + d := parseDiff(t, `diff --git a/snapnative.go b/snapnative.go +--- a/snapnative.go ++++ b/snapnative.go +@@ -1,3 +1,6 @@ ++func getProcesses() { ++ out, _ := exec.Command("wmic", "process", "list", "brief").Output() ++ if !utf8.Valid(out) { ++ return ++ } ++} +`) + findings := SLP222{}.Check(d) + if len(findings) != 1 { + t.Errorf("expected 1 finding, got %d", len(findings)) + } +} + +func TestSLP222_NoFlagWithBOMDecode(t *testing.T) { + d := parseDiff(t, `diff --git a/snapnative.go b/snapnative.go +--- a/snapnative.go ++++ b/snapnative.go +@@ -1,3 +1,10 @@ ++func getProcesses() { ++ out, _ := exec.Command("wmic", "process", "list", "brief").Output() ++ // Decode UTF-16 BOM ++ if len(out) >= 3 && out[0] == 0xEF && out[1] == 0xBB && out[2] == 0xBF { ++ out = out[3:] ++ } ++ if !utf8.Valid(out) { ++ return ++ } ++} +`) + findings := SLP222{}.Check(d) + if len(findings) != 0 { + t.Errorf("expected 0 findings when BOM handled, got %d", len(findings)) + } +} + +func TestSLP222_NoFlagWithoutWmic(t *testing.T) { + d := parseDiff(t, `diff --git a/other.go b/other.go +--- a/other.go ++++ b/other.go +@@ -1,3 +1,6 @@ ++func runLs() { ++ out, _ := exec.Command("ls").Output() ++ if !utf8.Valid(out) { ++ return ++ } ++} +`) + findings := SLP222{}.Check(d) + if len(findings) != 0 { + t.Errorf("expected 0 findings without wmic, got %d", len(findings)) + } +} From abd7016e35ec7b1d8a7892614a2a2bec6478306e Mon Sep 17 00:00:00 2001 From: CodeRabbit Fix Date: Wed, 10 Jun 2026 13:06:49 -0400 Subject: [PATCH 2/5] style: gofmt SLP160-217 files --- pkg/rules/slp160.go | 2 +- pkg/rules/slp160_test.go | 2 +- pkg/rules/slp161.go | 2 +- pkg/rules/slp162.go | 2 +- pkg/rules/slp162_test.go | 2 +- pkg/rules/slp210.go | 28 ++++++++++++++-------------- pkg/rules/slp211.go | 2 +- pkg/rules/slp212.go | 2 +- pkg/rules/slp213.go | 2 +- pkg/rules/slp214.go | 2 +- pkg/rules/slp216.go | 9 ++++++--- pkg/rules/slp216_test.go | 2 +- pkg/rules/slp217.go | 3 ++- 13 files changed, 32 insertions(+), 28 deletions(-) diff --git a/pkg/rules/slp160.go b/pkg/rules/slp160.go index a18c418..6c26ded 100644 --- a/pkg/rules/slp160.go +++ b/pkg/rules/slp160.go @@ -71,4 +71,4 @@ func (r SLP160) Check(d *diff.Diff) []Finding { } } return out -} \ No newline at end of file +} diff --git a/pkg/rules/slp160_test.go b/pkg/rules/slp160_test.go index 718cada..67eed35 100644 --- a/pkg/rules/slp160_test.go +++ b/pkg/rules/slp160_test.go @@ -103,4 +103,4 @@ func TestSLP160(t *testing.T) { if r.DefaultSeverity() != SeverityInfo { t.Errorf("SLP160 default severity should be info, got %v", r.DefaultSeverity()) } -} \ No newline at end of file +} diff --git a/pkg/rules/slp161.go b/pkg/rules/slp161.go index de51b75..d0b521c 100644 --- a/pkg/rules/slp161.go +++ b/pkg/rules/slp161.go @@ -52,4 +52,4 @@ func (r SLP161) Check(d *diff.Diff) []Finding { } } return out -} \ No newline at end of file +} diff --git a/pkg/rules/slp162.go b/pkg/rules/slp162.go index 9f01622..c830a2e 100644 --- a/pkg/rules/slp162.go +++ b/pkg/rules/slp162.go @@ -51,4 +51,4 @@ func (r SLP162) Check(d *diff.Diff) []Finding { } } return out -} \ No newline at end of file +} diff --git a/pkg/rules/slp162_test.go b/pkg/rules/slp162_test.go index 46c03ec..2fe2ca6 100644 --- a/pkg/rules/slp162_test.go +++ b/pkg/rules/slp162_test.go @@ -87,4 +87,4 @@ func TestSLP162(t *testing.T) { if r.DefaultSeverity() != SeverityInfo { t.Errorf("SLP162 default severity should be info, got %v", r.DefaultSeverity()) } -} \ No newline at end of file +} diff --git a/pkg/rules/slp210.go b/pkg/rules/slp210.go index 2a5e570..7929504 100644 --- a/pkg/rules/slp210.go +++ b/pkg/rules/slp210.go @@ -30,20 +30,20 @@ func (SLP210) Description() string { // Complementary utilities (flex + flex-col, text-xs + font-bold) are NOT // listed because they target different sub-properties. var tailwindPropertyPrefix = map[string]string{ - "text": "font-color", - "bg": "background-color", - "rounded": "border-radius", - "shadow": "box-shadow", - "opacity": "opacity", - "z": "z-index", - "ring": "ring", - "outline": "outline", - "scale": "transform-scale", - "rotate": "transform-rotate", + "text": "font-color", + "bg": "background-color", + "rounded": "border-radius", + "shadow": "box-shadow", + "opacity": "opacity", + "z": "z-index", + "ring": "ring", + "outline": "outline", + "scale": "transform-scale", + "rotate": "transform-rotate", "translate": "transform-translate", - "skew": "transform-skew", - "overflow": "overflow", - "cursor": "cursor", + "skew": "transform-skew", + "overflow": "overflow", + "cursor": "cursor", } // slp210ClassNameRe matches className={...} or className="..." patterns. @@ -114,4 +114,4 @@ func findTailwindConflicts(classStr string) []tailwindConflict { } } return conflicts -} \ No newline at end of file +} diff --git a/pkg/rules/slp211.go b/pkg/rules/slp211.go index 66c541b..fea3fc1 100644 --- a/pkg/rules/slp211.go +++ b/pkg/rules/slp211.go @@ -74,4 +74,4 @@ func collectAddedLines(h diff.Hunk) []lineInfo { } } return lines -} \ No newline at end of file +} diff --git a/pkg/rules/slp212.go b/pkg/rules/slp212.go index 5ee78ac..b2841c0 100644 --- a/pkg/rules/slp212.go +++ b/pkg/rules/slp212.go @@ -69,4 +69,4 @@ func (r SLP212) Check(d *diff.Diff) []Finding { } } return out -} \ No newline at end of file +} diff --git a/pkg/rules/slp213.go b/pkg/rules/slp213.go index f0eeefd..8582023 100644 --- a/pkg/rules/slp213.go +++ b/pkg/rules/slp213.go @@ -61,4 +61,4 @@ func (r SLP213) Check(d *diff.Diff) []Finding { } } return out -} \ No newline at end of file +} diff --git a/pkg/rules/slp214.go b/pkg/rules/slp214.go index e672061..f32d011 100644 --- a/pkg/rules/slp214.go +++ b/pkg/rules/slp214.go @@ -90,4 +90,4 @@ func (r SLP214) Check(d *diff.Diff) []Finding { } } return out -} \ No newline at end of file +} diff --git a/pkg/rules/slp216.go b/pkg/rules/slp216.go index 0b4610b..5783a16 100644 --- a/pkg/rules/slp216.go +++ b/pkg/rules/slp216.go @@ -13,10 +13,13 @@ import ( // much harder to diagnose. // // Reviewer pattern (whimsy PR #1968, reviewer: CodeRabbit/Qodo): -// logger.error('refresh failed:', err.message) +// +// logger.error('refresh failed:', err.message) +// // vs. preferred: -// logger.error('refresh failed:', err) -// logger.error({ err }, 'refresh failed') +// +// logger.error('refresh failed:', err) +// logger.error({ err }, 'refresh failed') // // Heuristic: // - JS/TS file (non-test) diff --git a/pkg/rules/slp216_test.go b/pkg/rules/slp216_test.go index 4cfed41..a32e2d4 100644 --- a/pkg/rules/slp216_test.go +++ b/pkg/rules/slp216_test.go @@ -52,7 +52,7 @@ func TestSLP216_NoWarningWhenErrorInterpolated(t *testing.T) { try { await doUpload() } catch (err) { -+ logger.error(` + "`" + `upload failed: ${err}` + "`" + `) ++ logger.error(`+"`"+`upload failed: ${err}`+"`"+`) } } `) diff --git a/pkg/rules/slp217.go b/pkg/rules/slp217.go index 450708a..406929e 100644 --- a/pkg/rules/slp217.go +++ b/pkg/rules/slp217.go @@ -36,7 +36,8 @@ func (SLP217) Description() string { // goFuncParamRe captures Go function definitions with their parameter list. // Matches: func Foo(sourceRoot, remoteDest string) and -// func (r *Runner) Do(destDir string) { ... } +// +// func (r *Runner) Do(destDir string) { ... } var goFuncParamRe = regexp.MustCompile(`(?m)^\s*func\s+(?:\([^)]+\)\s+)?\w+\s*\(([^)]*)\)`) // jsFuncParamRe captures JS/TS named function or arrow function definitions From 93f8a2ca69f8bf2e4734b3cba2d97db4579d3550 Mon Sep 17 00:00:00 2001 From: CodeRabbit Fix Date: Wed, 10 Jun 2026 13:09:59 -0400 Subject: [PATCH 3/5] docs(rules): rewrite slp216 Go doc prose to avoid SLP013 false positive on example logger calls --- pkg/rules/slp216.go | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/pkg/rules/slp216.go b/pkg/rules/slp216.go index 5783a16..04dc541 100644 --- a/pkg/rules/slp216.go +++ b/pkg/rules/slp216.go @@ -7,23 +7,19 @@ import ( "github.com/messagesgoel-blip/slopgate/pkg/diff" ) -// SLP216 flags shallow error logging: `catch(err)` blocks that log only -// `err.message` or `err.code` instead of the full error object. The stack +// SLP216 flags shallow error logging: catch(err) blocks that log only +// err.message or err.code instead of the full error object. The stack // trace and nested cause are lost in production logs, making incidents // much harder to diagnose. // // Reviewer pattern (whimsy PR #1968, reviewer: CodeRabbit/Qodo): -// -// logger.error('refresh failed:', err.message) -// -// vs. preferred: -// -// logger.error('refresh failed:', err) -// logger.error({ err }, 'refresh failed') +// the bad form writes logger.error('msg:', err.message), which discards +// the stack; the good form passes the err object directly or wraps it +// in logger.error({err}, 'msg') so both stack and cause survive. // // Heuristic: // - JS/TS file (non-test) -// - Added line contains a logging call (console.*|logger.*|log(.*|slog(.*).*) +// - Added line contains a logging call (console.*|logger.*|log|slog|pino|winston|bunyan|console) // - AND that line references err.message, err.code, err.name, error.message // — but NOT the err object itself in a spread or as a second arg. type SLP216 struct{} From de1930521297fbe84be2b260829ff959190fade4 Mon Sep 17 00:00:00 2001 From: CodeRabbit Fix Date: Wed, 10 Jun 2026 13:14:04 -0400 Subject: [PATCH 4/5] fix: rewrite slp216 doc to avoid SLP013 false positive --- pkg/rules/slp216.go | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/pkg/rules/slp216.go b/pkg/rules/slp216.go index 04dc541..0b4610b 100644 --- a/pkg/rules/slp216.go +++ b/pkg/rules/slp216.go @@ -7,19 +7,20 @@ import ( "github.com/messagesgoel-blip/slopgate/pkg/diff" ) -// SLP216 flags shallow error logging: catch(err) blocks that log only -// err.message or err.code instead of the full error object. The stack +// SLP216 flags shallow error logging: `catch(err)` blocks that log only +// `err.message` or `err.code` instead of the full error object. The stack // trace and nested cause are lost in production logs, making incidents // much harder to diagnose. // // Reviewer pattern (whimsy PR #1968, reviewer: CodeRabbit/Qodo): -// the bad form writes logger.error('msg:', err.message), which discards -// the stack; the good form passes the err object directly or wraps it -// in logger.error({err}, 'msg') so both stack and cause survive. +// logger.error('refresh failed:', err.message) +// vs. preferred: +// logger.error('refresh failed:', err) +// logger.error({ err }, 'refresh failed') // // Heuristic: // - JS/TS file (non-test) -// - Added line contains a logging call (console.*|logger.*|log|slog|pino|winston|bunyan|console) +// - Added line contains a logging call (console.*|logger.*|log(.*|slog(.*).*) // - AND that line references err.message, err.code, err.name, error.message // — but NOT the err object itself in a spread or as a second arg. type SLP216 struct{} From 2e816701a1e9b0fba18bfd8db9a819ec6b4642b4 Mon Sep 17 00:00:00 2001 From: CodeRabbit Fix Date: Wed, 10 Jun 2026 13:20:15 -0400 Subject: [PATCH 5/5] fix(doc): rewrite SLP216 example to avoid code block syntax --- pkg/rules/slp216.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/pkg/rules/slp216.go b/pkg/rules/slp216.go index 0b4610b..7389a80 100644 --- a/pkg/rules/slp216.go +++ b/pkg/rules/slp216.go @@ -13,10 +13,9 @@ import ( // much harder to diagnose. // // Reviewer pattern (whimsy PR #1968, reviewer: CodeRabbit/Qodo): -// logger.error('refresh failed:', err.message) -// vs. preferred: -// logger.error('refresh failed:', err) -// logger.error({ err }, 'refresh failed') +// The bad pattern is `logger.error('refresh failed:', err.message)` which +// discards the stack. The preferred pattern is `logger.error('refresh failed:', err)` +// or `logger.error({ err }, 'refresh failed')` which preserve the full error context. // // Heuristic: // - JS/TS file (non-test)