Skip to content

Commit a63ff23

Browse files
authored
fix: retune precision false positives (#81)
## Summary This PR retunes CodeGuard's local quality precision checks to reduce high-volume false positives from common TypeScript/JavaScript, React, Next.js, and API-boundary patterns while preserving the intended production-readiness signals. ### What changed - Hardened `function.hidden-mutation` so local scratch mutation in pure builders/parsers is not treated as hidden mutation. - Allows local `Set.add`, `Map.set`, `array.push`, `.sort`, `.split().pop()`, object accumulation, and Cheerio cleanup when the function returns a derived value. - Keeps the rule focused on mutation of arguments, module/global state, collaborators, React state, DOM, DB, filesystem, or other external resources. - Retuned `function.command-query-mix` for query-safe builder/parser patterns. - Pure builders/parsers that mutate local scratch state no longer look like command/query violations. - Command functions that intentionally write and return useful results are treated as normal API/application behavior. - Improved boundary/resource validation recognition. - Credits `z.safeParse`, helper-returned validated values, NextResponse early-return guards, `new URL(...)` plus protocol allowlists, shared upload validators, and imported byte-limit constants. - Credits `Content-Length` preflight, explicit `take`/`limit`, `slice`, count/size/byte guards, and bounded response byte-length checks for `defensive.missing-resource-limit`. - Split sequence-allocation risk from arithmetic overflow. - Added `defensive.sequence-collision-risk` for count-derived external ID allocation without guarded unique-collision retry. - Keeps guarded Prisma/P2002 retry loops out of `defensive.integer-overflow`. - Suppresses metric/stat arithmetic from overflow findings. - Reduced naming and smell noise. - Allows common UI boolean/domain names such as `show*`, `matches*`, `visible`, `active`, `open`, `selected`, `enabled`. - Allows collection/domain abbreviations such as `krs`, `docs`, and `ids`. - Tolerates centralized enum/domain dispatch maps for `smell.switch-on-type`. - Added rule metadata and documentation. - Registered `defensive.sequence-collision-risk` in the rule catalog and fix-template catalog. - Updated `docs/checks.md` so users can discover the new defensive rule. ### Tests - Added regression coverage for local builder/parser mutation patterns. - Added regression coverage for boundary validation and resource-limit proof patterns. - Added regression coverage for sequence-collision vs integer-overflow behavior. - Updated existing UI false-positive hardening tests to match the narrower analyzer behavior. ## Validation - `go test ./tests/checks -run 'TestDefensive(Integer|Resource)|TestRulesCatalog' -count=1` - `go test ./tests/checks -count=1` - `go test ./...` - `golangci-lint run` - `make codeguard-ci`
2 parents e810575 + 1fd6f2b commit a63ff23

12 files changed

Lines changed: 622 additions & 199 deletions

docs/checks.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1186,6 +1186,7 @@ These rules live outside the repository-wide `Change Safety` section in report o
11861186
| Defensive programming | `defensive.invalid-state-representable` | warn | Booleans or raw status strings can represent impossible state combinations. |
11871187
| Defensive programming | `defensive.null-assumption` | warn | Nullable boundary values are dereferenced without a nil/null guard. |
11881188
| Defensive programming | `defensive.integer-overflow` | warn | Arithmetic on count, size, or length input lacks an overflow bound check. |
1189+
| Defensive programming | `defensive.sequence-collision-risk` | warn | External ID allocation derives the next value from current count without guarded unique-collision retry. |
11891190
| Defensive programming | `defensive.bounds-assumption` | warn | Indexed access assumes collection bounds without a nearby length check. |
11901191
| Defensive programming | `defensive.unsafe-default` | warn | A config/env fallback can fail open or disable a safety control. |
11911192
| Defensive programming | `defensive.non-exhaustive-branch` | warn | Enum-like state/kind/type branching lacks default or exhaustive handling. |

internal/codeguard/checks/quality/quality_defensive.go

Lines changed: 97 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ const (
1313
defensiveInvalidStateRepresentableRuleID = "defensive.invalid-state-representable"
1414
defensiveNullAssumptionRuleID = "defensive.null-assumption"
1515
defensiveIntegerOverflowRuleID = "defensive.integer-overflow"
16+
defensiveSequenceCollisionRiskRuleID = "defensive.sequence-collision-risk"
1617
defensiveBoundsAssumptionRuleID = "defensive.bounds-assumption"
1718
defensiveUnsafeDefaultRuleID = "defensive.unsafe-default"
1819
defensiveNonExhaustiveBranchRuleID = "defensive.non-exhaustive-branch"
@@ -35,6 +36,10 @@ var (
3536
structStartPattern = regexp.MustCompile(`(?i)\b(type\s+\w+\s+struct|interface\s+\w+|class\s+\w+|struct\s+\w+)`)
3637
boolFieldPattern = regexp.MustCompile(`(?i)\b(bool|boolean)\b`)
3738
stringStateFieldPattern = regexp.MustCompile(`(?i)\b(status|state|kind)\b.*\b(string|str|std::string|String)\b|\b(string|str|std::string|String)\b.*\b(status|state|kind)\b`)
39+
resourceCountGuard = regexp.MustCompile(`(?i)\b(?:count|size|length|len|bytes)\s*(?:<=|<|>|>=)\s*(?:max|limit|quota|cap|[0-9])`)
40+
resourceNamedCountLimit = regexp.MustCompile(`(?i)\b(?:max|limit|quota|cap)[A-Za-z0-9_]*(?:count|size|length|len|bytes)\b`)
41+
sequenceAllocationLine = regexp.MustCompile(`(?i)\b(?:external[_]?id|next[_]?id|sequence|slug|number)\b.*(?:count|max)\s*\+\s*1|(?:count|max)\s*\+\s*1.*\b(?:external[_]?id|next[_]?id|sequence|slug|number)\b`)
42+
jsonReaderSchemaCall = regexp.MustCompile(`(?i)\b(?:read|parse|decode)Json[A-Za-z0-9_]*\s*\([^)\n,]+,\s*[A-Za-z_$][\w$]*(?:Schema|Validator|Codec|Parser)\b`)
3843
)
3944

4045
func defensiveBoundaryFindings(env support.Context, file string, fn precisionFunction) []core.Finding {
@@ -53,6 +58,10 @@ func defensiveBoundaryFindings(env support.Context, file string, fn precisionFun
5358
findings = append(findings, precisionWarnFinding(env, defensiveNullAssumptionRuleID, file, line,
5459
"nullable boundary value is dereferenced without a nil/null guard", core.ConfidenceMedium))
5560
}
61+
if line, ok := sequenceCollisionRiskLine(fn, loweredBody); ok {
62+
findings = append(findings, precisionWarnFinding(env, defensiveSequenceCollisionRiskRuleID, file, line,
63+
"external ID allocation derives the next value from current count without guarded unique-collision retry", core.ConfidenceMedium))
64+
}
5665
if line, ok := integerOverflowLine(file, fn, loweredBody); ok {
5766
findings = append(findings, precisionWarnFinding(env, defensiveIntegerOverflowRuleID, file, line,
5867
"arithmetic on count, size, or length input lacks an overflow bound check", core.ConfidenceMedium))
@@ -130,6 +139,9 @@ func unvalidatedBoundaryInputLine(fn precisionFunction, loweredBody string) (int
130139
if !boundaryFunctionName(fn.Name) && !hasBoundaryParam(fn.Params) {
131140
return 0, false
132141
}
142+
if isValidationOrExtractionHelperName(fn.Name) {
143+
return 0, false
144+
}
133145
if validatedBoundaryInputPattern(fn, loweredBody) {
134146
return 0, false
135147
}
@@ -149,7 +161,13 @@ func formDataHasContentLengthPreflight(loweredBody string) bool {
149161
}
150162

151163
func validatedBoundaryInputPattern(fn precisionFunction, loweredBody string) bool {
152-
if containsAny(loweredBody, []string{"validate", "schema", "sanitize", "bind", "decodevalid", "safeparse", "zod.", "yup.", "pydantic", "jsonschema"}) {
164+
if containsAny(loweredBody, []string{"validate", "schema", "sanitize", "bind", "decodevalid", "safeparse", "z.safeparse", "zod.", "yup.", "pydantic", "jsonschema"}) {
165+
return true
166+
}
167+
if jsonReaderSchemaCall.MatchString(functionRawBody(fn)) {
168+
return true
169+
}
170+
if strings.Contains(loweredBody, "nextresponse.") && containsAny(loweredBody, []string{"return nextresponse", ".json(", "redirect("}) && containsAny(loweredBody, []string{"if (!", "if (!", "if(", "if "}) {
153171
return true
154172
}
155173
if regexp.MustCompile(`(?i)\b(parse|assert|guard|ensure|decode)[A-Z_][A-Za-z0-9_]*(?:Input|Payload|Body|Params|Query|Record|Request|Event|Config)?\s*\(`).MatchString(functionRawBody(fn)) {
@@ -158,6 +176,16 @@ func validatedBoundaryInputPattern(fn precisionFunction, loweredBody string) boo
158176
return false
159177
}
160178

179+
func isValidationOrExtractionHelperName(name string) bool {
180+
lowered := strings.ToLower(strings.Trim(name, "_$"))
181+
if strings.HasPrefix(lowered, "parse") || strings.HasPrefix(lowered, "assert") ||
182+
strings.HasPrefix(lowered, "guard") || strings.HasPrefix(lowered, "ensure") ||
183+
strings.HasPrefix(lowered, "decode") {
184+
return true
185+
}
186+
return containsAny(lowered, []string{"bearertokenfrom", "tokenfrom", "headerfrom", "requestbodyfrom"})
187+
}
188+
161189
func hasBoundaryParam(params []support.ParsedParam) bool {
162190
for _, param := range params {
163191
name := strings.ToLower(param.Name)
@@ -205,7 +233,7 @@ func integerOverflowLine(file string, fn precisionFunction, loweredBody string)
205233
if isUIRenderArithmeticContext(file, fn, loweredBody) {
206234
return 0, false
207235
}
208-
if guardedSequenceCollisionRetry(loweredBody) {
236+
if sequenceAllocationArithmetic(loweredBody) || metricStatArithmeticContext(fn, loweredBody) || dateCountFormattingContext(fn, loweredBody) {
209237
return 0, false
210238
}
211239
if containsAny(loweredBody, []string{"maxint", "math.max", "checked", "saturating", "overflow", "limits<", "safeint"}) {
@@ -220,7 +248,35 @@ func integerOverflowLine(file string, fn precisionFunction, loweredBody string)
220248
return 0, false
221249
}
222250

251+
func sequenceCollisionRiskLine(fn precisionFunction, loweredBody string) (int, bool) {
252+
if !sequenceAllocationArithmetic(loweredBody) || guardedSequenceCollisionRetry(loweredBody) {
253+
return 0, false
254+
}
255+
return firstSequenceAllocationLine(fn), true
256+
}
257+
258+
func sequenceAllocationArithmetic(loweredBody string) bool {
259+
if !containsAny(loweredBody, []string{"count + 1", "count+1", "max + 1", "max+1"}) {
260+
return false
261+
}
262+
return containsAny(loweredBody, []string{"externalid", "external_id", "nextid", "next_id", "sequence", "slug", "number"})
263+
}
264+
265+
func firstSequenceAllocationLine(fn precisionFunction) int {
266+
for _, statement := range fn.Statements {
267+
raw := firstNonEmptyString(statement.Raw, statement.Text)
268+
if sequenceAllocationLine.MatchString(raw) {
269+
return statement.Line
270+
}
271+
}
272+
return fn.StartLine
273+
}
274+
223275
func guardedSequenceCollisionRetry(loweredBody string) bool {
276+
if containsAny(loweredBody, []string{"withexternalidretry", "with_external_id_retry"}) &&
277+
containsAny(loweredBody, []string{"p2002", "unique", "collision", "externalid", "external_id"}) {
278+
return true
279+
}
224280
if !containsAny(loweredBody, []string{"p2002", "unique", "collision", "prisma"}) {
225281
return false
226282
}
@@ -230,6 +286,25 @@ func guardedSequenceCollisionRetry(loweredBody string) bool {
230286
return containsAny(loweredBody, []string{"count + 1", "count+1", "externalid", "external_id", "nextid", "next_id"})
231287
}
232288

289+
func metricStatArithmeticContext(fn precisionFunction, loweredBody string) bool {
290+
loweredName := strings.ToLower(fn.Name)
291+
if containsAny(loweredName, []string{"metric", "metrics", "stat", "stats", "counter", "histogram", "telemetry"}) {
292+
return true
293+
}
294+
return containsAny(loweredBody, []string{"metric.", "metrics.", "counter.", "histogram", "stat.", "stats.", "telemetry", "prometheus", "datadog"})
295+
}
296+
297+
func dateCountFormattingContext(fn precisionFunction, loweredBody string) bool {
298+
loweredName := strings.ToLower(fn.Name)
299+
if !containsAny(loweredName, []string{"format", "display", "label", "render", "summary", "calendar", "date", "time"}) {
300+
return false
301+
}
302+
return containsAny(loweredBody, []string{
303+
"date", "time", "calendar", "duration", "intl.", "datetimeformat", "formatdistance",
304+
"formatrelative", "plural", "label", "title", "subtitle", "`${", " + \"", " + '",
305+
})
306+
}
307+
233308
func isUIRenderArithmeticContext(file string, fn precisionFunction, loweredBody string) bool {
234309
if isUIHelperOrMappingContext(file, fn) {
235310
return true
@@ -333,7 +408,7 @@ func missingSchemaValidationLine(fn precisionFunction, loweredBody string) (int,
333408
if !jsonDecodePattern.MatchString(functionRawBody(fn)) {
334409
return 0, false
335410
}
336-
if validatedBoundaryInputPattern(fn, loweredBody) || containsAny(loweredBody, []string{"jsonschema", "isvalid", "required"}) {
411+
if validatedBoundaryInputPattern(fn, loweredBody) || jsonReaderSchemaCall.MatchString(functionRawBody(fn)) || containsAny(loweredBody, []string{"jsonschema", "isvalid", "required"}) {
337412
return 0, false
338413
}
339414
return firstPatternLine(fn, jsonDecodePattern), true
@@ -343,7 +418,10 @@ func missingResourceLimitLine(fn precisionFunction, loweredBody string) (int, bo
343418
if !resourceReadPattern.MatchString(functionRawBody(fn)) {
344419
return 0, false
345420
}
346-
if containsAny(loweredBody, []string{"limitreader", "maxbytes", "max_bytes", "content-length", "contentlength", "limit(", "take(", "buffer_size", "quota"}) {
421+
if uploadValidationHelperPattern(loweredBody) {
422+
return 0, false
423+
}
424+
if resourceLimitProofPattern(loweredBody) {
347425
return 0, false
348426
}
349427
if boundedReadByteLengthCheck(loweredBody) {
@@ -352,6 +430,21 @@ func missingResourceLimitLine(fn precisionFunction, loweredBody string) (int, bo
352430
return firstPatternLine(fn, resourceReadPattern), true
353431
}
354432

433+
func resourceLimitProofPattern(loweredBody string) bool {
434+
if containsAny(loweredBody, []string{
435+
"limitreader", "maxbytes", "max_bytes", "content-length", "contentlength",
436+
"limit(", "take(", "slice(", ".slice(", "buffer_size", "quota",
437+
}) {
438+
return true
439+
}
440+
return resourceCountGuard.MatchString(loweredBody) || resourceNamedCountLimit.MatchString(loweredBody)
441+
}
442+
443+
func uploadValidationHelperPattern(loweredBody string) bool {
444+
return containsAny(loweredBody, []string{"validateinternaluploadfile", "validateuploadfile", "validatefileupload", "validateupload"}) ||
445+
containsAny(loweredBody, []string{"internal_upload_max_bytes", "upload_max_bytes", "max_upload_bytes", "max_file_bytes"})
446+
}
447+
355448
func boundedReadByteLengthCheck(loweredBody string) bool {
356449
if !containsAny(loweredBody, []string{"arraybuffer", ".text", "readall", ".read"}) {
357450
return false

internal/codeguard/checks/quality/quality_precision_mutation_targets.go

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ import (
1010

1111
var conventionalMutationBoundaryPattern = regexp.MustCompile(`^(accept|apply|approve|archive|clear|close|commit|deliver|download|drop|ensure|exists|fetch|import|list|notify|open|process|read|reconcile|record|run|seed|submit|sync|toggle|upload)`)
1212

13-
var localAccumulatorExprPattern = regexp.MustCompile(`(?i)^(?:new\s+)?(?:array|formdata|map|object|set|urlsearchparams|weakmap|weakset)\b|^\[|^\{|^make\s*\(|^array\.from\b|\.map\s*\(|\.filter\s*\(|\.reduce\s*\(|^(?:bytes|strings)\.buffer\b|^strings\.builder\b`)
13+
var localAccumulatorExprPattern = regexp.MustCompile(`(?i)^(?:new\s+)?(?:array|formdata|map|object|set|urlsearchparams|weakmap|weakset)\b|^\[|^\{|^make\s*\(|^array\.from\b|\.map\s*\(|\.filter\s*\(|\.reduce\s*\(|\.split\s*\(|cheerio\.load\s*\(|^(?:bytes|strings)\.buffer\b|^strings\.builder\b`)
1414

1515
func localMutationTargets(fn precisionFunction) map[string]struct{} {
1616
params := paramNames(fn)
@@ -133,7 +133,7 @@ func isAccumulatorLikeLocalName(name string) bool {
133133
"bucket", "buckets", "buffer", "builder", "calendar", "cells", "copy", "doc",
134134
"document", "filter", "filters", "form", "items", "lines", "params", "parts",
135135
"payload", "primarycells", "query", "result", "rows", "scopes", "sections",
136-
"serializer", "text", "urlparams", "values", "csv", "export", "map",
136+
"serializer", "text", "urlparams", "values", "csv", "export", "map", "$",
137137
} {
138138
if strings.Contains(lowered, token) {
139139
return true
@@ -146,7 +146,7 @@ func isAccumulatorBuilderFunctionName(name string) bool {
146146
lowered := strings.ToLower(strings.Trim(name, "_$"))
147147
for _, token := range []string{
148148
"bucket", "build", "collect", "derive", "format", "group", "map", "parse",
149-
"primary", "render", "serialize", "transform",
149+
"primary", "render", "serialize", "transform", "clean", "filter",
150150
} {
151151
if strings.Contains(lowered, token) {
152152
return true
@@ -178,6 +178,9 @@ func paramNames(fn precisionFunction) map[string]struct{} {
178178
}
179179

180180
func isLocalMutationCall(callee string, localTargets map[string]struct{}) bool {
181+
if isDerivedCollectionMutationCall(callee) {
182+
return true
183+
}
181184
if isBareLocalMutationCall(callee) {
182185
return true
183186
}
@@ -188,6 +191,11 @@ func isLocalMutationCall(callee string, localTargets map[string]struct{}) bool {
188191
return isLocalMutationTarget(target, localTargets)
189192
}
190193

194+
func isDerivedCollectionMutationCall(callee string) bool {
195+
lowered := strings.ToLower(strings.ReplaceAll(strings.TrimSpace(callee), " ", ""))
196+
return strings.Contains(lowered, ".split.") && (strings.HasSuffix(lowered, ".pop") || strings.HasSuffix(lowered, ".sort") || strings.HasSuffix(lowered, ".reverse"))
197+
}
198+
191199
func isBareLocalMutationCall(callee string) bool {
192200
switch strings.TrimSpace(callee) {
193201
case "append", "Set", "Array", "Object", "Map", "WeakMap", "WeakSet", "push_back":
@@ -202,6 +210,9 @@ func mutationCallTarget(callee string) string {
202210
if callee == "" {
203211
return ""
204212
}
213+
if strings.HasPrefix(callee, "$.") {
214+
return "$"
215+
}
205216
for _, sep := range []string{".", "->", "::"} {
206217
if idx := strings.Index(callee, sep); idx > 0 {
207218
return strings.TrimSpace(callee[:idx])
Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
package quality
2+
3+
import "strings"
4+
5+
func isDomainSideEffectBoundaryName(name string) bool {
6+
lowered := strings.ToLower(strings.TrimSpace(name))
7+
if lowered == "" {
8+
return false
9+
}
10+
if strings.HasPrefix(lowered, "maybe") && containsAny(lowered, []string{"alert", "notify", "record", "track", "emit"}) {
11+
return true
12+
}
13+
if strings.HasPrefix(lowered, "evaluate") && containsAny(lowered, []string{"abuse", "policy", "rule", "risk", "fraud", "quota", "limit"}) {
14+
return true
15+
}
16+
if strings.HasPrefix(lowered, "load") && containsAny(lowered, []string{"config", "defaults", "settings", "policy"}) {
17+
return true
18+
}
19+
return false
20+
}
21+
22+
func isAdapterOrOrchestrationFunction(file string, fn precisionFunction) bool {
23+
loweredName := strings.ToLower(strings.Trim(fn.Name, "_$"))
24+
if containsAny(loweredName, []string{"adapter", "bugreport", "bug_report", "slack", "webhook", "sync", "abuseconfig", "abuse_config"}) {
25+
return true
26+
}
27+
if strings.HasPrefix(loweredName, "save") || strings.HasPrefix(loweredName, "insert") || strings.HasPrefix(loweredName, "post") ||
28+
strings.HasPrefix(loweredName, "send") || strings.HasPrefix(loweredName, "publish") || strings.HasPrefix(loweredName, "record") {
29+
if containsAny(loweredName, []string{"config", "report", "slack", "webhook", "audit", "event", "job"}) {
30+
return true
31+
}
32+
}
33+
normalized := strings.ToLower(strings.ReplaceAll(file, "\\", "/"))
34+
return containsAny(normalized, []string{"/adapters/", "/adapter/", "/connectors/", "/connector/", "/integrations/", "/webhooks/", "/slack/", "/jobs/"})
35+
}
36+
37+
func isAdapterOrchestrationName(name string) bool {
38+
loweredName := strings.ToLower(strings.Trim(name, "_$"))
39+
return containsAny(loweredName, []string{"abuseconfig", "abuse_config", "bugreport", "bug_report", "slack", "webhook", "adapter"})
40+
}
41+
42+
func configuredPluralDomainAbbreviation(name string) bool {
43+
switch name {
44+
case "docs", "krs":
45+
return true
46+
default:
47+
return false
48+
}
49+
}

internal/codeguard/checks/quality/quality_precision_ui_conventions.go

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -161,7 +161,13 @@ func isAllowedBooleanUIName(file string, fn precisionFunction, name string) bool
161161
if !isReactComponentOrHookBoundary(file, fn) {
162162
return false
163163
}
164-
switch strings.ToLower(strings.Trim(name, "_$")) {
164+
normalized := strings.ToLower(strings.Trim(name, "_$"))
165+
for _, suffix := range []string{"active", "visible", "enabled", "disabled", "open", "closed", "expanded", "collapsed", "selected", "checked", "pending", "loading"} {
166+
if strings.HasSuffix(normalized, suffix) {
167+
return true
168+
}
169+
}
170+
switch normalized {
165171
case "open", "loading", "active", "pending", "checked", "selected", "expanded", "collapsed":
166172
return true
167173
default:
@@ -191,7 +197,7 @@ func isResourceIdentifierName(name string) bool {
191197
func conventionalCardinalityName(name string) bool {
192198
base := strings.ToLower(strings.Trim(name, "_$"))
193199
switch base {
194-
case "answers", "args", "claims", "columns", "contracts", "entries", "files", "ids", "items", "k", "keys", "matters", "messages", "next", "out", "params", "props", "records", "risks", "rows", "searchparams", "sections", "source", "status", "thresholds", "users", "versions", "v", "i", "j", "x", "y":
200+
case "all", "answers", "args", "claims", "columns", "contracts", "docs", "entries", "files", "filtered", "ids", "items", "k", "keys", "krs", "matters", "messages", "next", "out", "params", "props", "records", "risks", "rows", "searchparams", "sections", "source", "status", "thresholds", "users", "versions", "v", "i", "j", "x", "y":
195201
return true
196202
default:
197203
return len(name) <= 2 ||

0 commit comments

Comments
 (0)