Skip to content

Commit 81388fd

Browse files
committed
fix: reduce defensive false positives
1 parent 3a237f0 commit 81388fd

4 files changed

Lines changed: 47 additions & 7 deletions

File tree

.claude/knowledge/testing-patterns.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,3 +12,4 @@ Testing strategies, test infrastructure quirks, how to run/debug specific test s
1212
- `gofmt -l .` at repo root is polluted by `.claude/worktrees/` (live agent worktrees) and `.gomodcache/`; scope it to `gofmt -l cmd internal pkg tests changelog.go` or use `make fmt-check`.
1313
- **Bidirectional (server→client) MCP tests** live in `tests/mcp/sampling_test.go`: the test acts as the MCP client, advertises `sampling`/`roots` at `initialize`, and answers the server's server-initiated requests. stdio uses interactive `StdinPipe`/`StdoutPipe` (not the replay harness). HTTP opens the `GET /mcp` SSE stream (waits for the `: ready` comment to avoid the attach race), reads the request off the stream, and POSTs the response with the matching `Mcp-Session-Id`. propose_fix verification is expected to fail on the throwaway diff — assert the round trip fired, not a verified patch. The HTTP helper passes `-config` via `CODEGUARD_TEST_HTTP_CONFIG`.
1414
- **TS tests can be hijacked by the Node semantic engine**: on hosts with a discoverable `typescript.js` (e.g. VS Code installed), TypeScript targets route through the Node runner instead of the per-file Go path. Tests that must exercise the per-file path (tree-sitter differential tests, corpus TS groups) set `CODEGUARD_TYPESCRIPT_LIB_PATH` to an existing-but-invalid lib to force the fallback.
15+
- Defensive precision positive fixtures should avoid UI-ish names such as `render*` unless the test is explicitly covering UI suppression. The defensive boundary/null rules intentionally skip React/UI helper contexts, so a fixture named like a renderer can stop emitting the server-side defensive finding the test expects.

internal/codeguard/checks/quality/quality_defensive.go

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ var (
2828
indexAccessPattern = regexp.MustCompile(`\b([A-Za-z_][\w$]*(?:\.[A-Za-z_][\w$]*)?)\s*\[\s*([^\]\n]+)\s*\]`)
2929
jsonDecodePattern = regexp.MustCompile(`(?i)(json\.Unmarshal|json\.NewDecoder|JSON\.parse|json\.loads|nlohmann::json::parse|decode_json|parseJson)`)
3030
externalCallPattern = regexp.MustCompile(`(?i)(http\.Get|client\.Do|fetch\s*\(|axios\.|requests\.(get|post|put|delete)|curl_easy_perform|httplib::|http_client)`)
31-
resourceReadPattern = regexp.MustCompile(`(?i)(io\.ReadAll|ReadAll|read_to_string|read_to_end|\.read\s*\(|bodyParser|multer|upload|formData\s*\(|request\.body|r\.Body)`)
31+
resourceReadPattern = regexp.MustCompile(`(?i)(io\.ReadAll|ReadAll|read_to_string|read_to_end|\.read\s*\(|\.text\s*\(|arrayBuffer\s*\(|bodyParser|multer|formData\s*\(|request\.body|r\.Body)`)
3232
ormCollectionReadPattern = regexp.MustCompile(`(?i)\bfindMany\s*\(`)
3333
unsafeDefaultPattern = regexp.MustCompile(`(?i)(getenv|process\.env|os\.environ|std::getenv|config).*?(default|fallback|\|\||!=|,\s*['"]).*?(true|false|allow|disable|skip|insecure)`)
3434
switchLikePattern = regexp.MustCompile(`(?i)\b(switch|match)\b[^{:\n]*(status|state|kind|type)`)
@@ -42,6 +42,7 @@ var (
4242
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`)
4343
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`)
4444
prismaTakePattern = regexp.MustCompile(`(?is)\b(?:findMany|findFirst|findUnique|query|search)\s*\([^)]*\btake\s*:`)
45+
sequenceIndexKeyPattern = regexp.MustCompile(`(?i)^(?:i|j|n|idx|index|offset|position|pos|[A-Za-z_$][\w$]*(?:Index|Idx|Offset|Position|Pos))$`)
4546
)
4647

4748
func defensiveBoundaryFindings(env support.Context, file string, fn precisionFunction) []core.Finding {
@@ -52,11 +53,11 @@ func defensiveBoundaryFindings(env support.Context, file string, fn precisionFun
5253
loweredBody := strings.ToLower(body)
5354
findings := make([]core.Finding, 0)
5455

55-
if line, ok := unvalidatedBoundaryInputLine(fn, loweredBody); ok {
56+
if line, ok := unvalidatedBoundaryInputLine(file, fn, loweredBody); ok {
5657
findings = append(findings, precisionWarnFinding(env, defensiveUnvalidatedBoundaryInputRuleID, file, line,
5758
"boundary input is consumed without validation or schema checks", core.ConfidenceMedium))
5859
}
59-
if line, ok := nullAssumptionLine(fn, loweredBody); ok {
60+
if line, ok := nullAssumptionLine(file, fn, loweredBody); ok {
6061
findings = append(findings, precisionWarnFinding(env, defensiveNullAssumptionRuleID, file, line,
6162
"nullable boundary value is dereferenced without a nil/null guard", core.ConfidenceMedium))
6263
}
@@ -137,7 +138,10 @@ func structuralStateContainerLine(line string) bool {
137138
return strings.Contains(lowered, "struct") || strings.Contains(lowered, "interface") || strings.Contains(lowered, "class")
138139
}
139140

140-
func unvalidatedBoundaryInputLine(fn precisionFunction, loweredBody string) (int, bool) {
141+
func unvalidatedBoundaryInputLine(file string, fn precisionFunction, loweredBody string) (int, bool) {
142+
if isUIHelperOrMappingContext(file, fn) || isReactComponentOrHookBoundary(file, fn) {
143+
return 0, false
144+
}
141145
if !boundaryFunctionName(fn.Name) && !hasBoundaryParam(fn.Params) {
142146
return 0, false
143147
}
@@ -198,7 +202,10 @@ func hasBoundaryParam(params []support.ParsedParam) bool {
198202
return false
199203
}
200204

201-
func nullAssumptionLine(fn precisionFunction, loweredBody string) (int, bool) {
205+
func nullAssumptionLine(file string, fn precisionFunction, loweredBody string) (int, bool) {
206+
if isUIHelperOrMappingContext(file, fn) || isReactComponentOrHookBoundary(file, fn) {
207+
return 0, false
208+
}
202209
for _, param := range fn.Params {
203210
name := strings.ToLower(strings.Trim(param.Name, "*& "))
204211
if name == "" || !nullableParam(param) {
@@ -393,7 +400,7 @@ func indexExpressionLooksSequenceAccess(target string, key string, raw string) b
393400
if regexp.MustCompile(`^\d+$`).MatchString(loweredKey) {
394401
return true
395402
}
396-
if containsAny(loweredKey, []string{"index", "idx", "offset", "position", "pos", "i", "j", "n"}) {
403+
if sequenceIndexKeyPattern.MatchString(strings.TrimSpace(strings.Trim(key, `"'`))) {
397404
return true
398405
}
399406
return containsAny(loweredTarget, []string{"array", "list", "slice", "items", "rows", "columns", "chars", "parts", "tokens", "segments", "lines", "values"}) &&

tests/checks/quality_error_defensive_multilang_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -439,7 +439,7 @@ func TestQualityDefensiveBoundariesDetectMultiLanguageSignals(t *testing.T) {
439439
" return payload.user.id;",
440440
"}",
441441
"",
442-
"export function renderUser(user: User | null): string {",
442+
"export function loadUserName(user: User | null): string {",
443443
" return user.name;",
444444
"}",
445445
"",

tests/checks/quality_precision_followup_retune_test.go

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -84,16 +84,29 @@ func TestDefensiveBoundsAssumptionDistinguishesDictionaryFromSequenceAccess(t *t
8484
"export function envValue(name: string) {",
8585
" return process.env[name];",
8686
"}",
87+
"export function modelConfig(config: Record<string, string>, modelKey: string) {",
88+
" return config[modelKey] ?? 'default';",
89+
"}",
90+
"export async function promiseTuple(db: Db, ids: string[]) {",
91+
" const [contracts, risks] = await Promise.all([",
92+
" db.contract.findMany({ where: { id: { in: ids } }, take: 10 }),",
93+
" db.risk.findMany({ where: { id: { in: ids } }, take: 10 }),",
94+
" ]);",
95+
" return { contracts, risks };",
96+
"}",
8797
"export function firstSegment(segments: string[]) {",
8898
" return segments[0];",
8999
"}",
100+
"interface Db { contract: { findMany(input: unknown): Promise<unknown[]> }; risk: { findMany(input: unknown): Promise<unknown[]> } }",
90101
}, "\n"))
91102

92103
report := runQualityPrecisionScan(t, qualityPrecisionConfigForLanguage(dir, "typescript"))
93104

94105
assertFindingRulePresent(t, report, "Code Quality", "defensive.bounds-assumption")
95106
assertCodeQualityRuleAbsentForPath(t, report, "defensive.bounds-assumption", "field-map.ts:2")
96107
assertCodeQualityRuleAbsentForPath(t, report, "defensive.bounds-assumption", "field-map.ts:5")
108+
assertCodeQualityRuleAbsentForPath(t, report, "defensive.bounds-assumption", "field-map.ts:8")
109+
assertCodeQualityRuleAbsentForPath(t, report, "defensive.bounds-assumption", "field-map.ts:11")
97110
}
98111

99112
func TestDefensiveResourceLimitCreditsPrismaTakeAndContentLengthHelpers(t *testing.T) {
@@ -117,12 +130,20 @@ func TestDefensiveResourceLimitCreditsPrismaTakeAndContentLengthHelpers(t *testi
117130
" const form = await request.formData();",
118131
" return form;",
119132
"}",
133+
"export async function readRaw(file: File) {",
134+
" return Buffer.from(await file.arrayBuffer());",
135+
"}",
136+
"export function uploadsRoot() {",
137+
" const fromEnv = process.env.LEGAL_OS_UPLOADS_ROOT?.trim();",
138+
" return fromEnv ?? '/tmp/uploads';",
139+
"}",
120140
}, "\n"))
121141

122142
report := runQualityPrecisionScan(t, qualityPrecisionConfigForLanguage(dir, "typescript"))
123143

124144
assertFindingRulePresent(t, report, "Code Quality", "defensive.missing-resource-limit")
125145
assertCodeQualityRuleAbsentForPath(t, report, "defensive.missing-resource-limit", "search-tools.ts")
146+
assertCodeQualityRuleAbsentForPath(t, report, "defensive.missing-resource-limit", "unbounded-upload.ts:8")
126147
}
127148

128149
func TestDefensiveBroadeningSkipsUIBoundsAndInternalORMReads(t *testing.T) {
@@ -135,6 +156,15 @@ func TestDefensiveBroadeningSkipsUIBoundsAndInternalORMReads(t *testing.T) {
135156
"}",
136157
"interface Props { nodes: Array<{ id: string }>; columns: Array<{ label: string }>; activeIndex: number }",
137158
}, "\n"))
159+
writeFile(t, filepath.Join(dir, "apps/web/components/use-filter-state.tsx"), strings.Join([]string{
160+
"export function useFilterState(input?: { query?: string }) {",
161+
" const query = input?.query ?? '';",
162+
" return { query };",
163+
"}",
164+
"export function SearchToolbar({ input }: { input?: { query?: string } }) {",
165+
" return <button>{input?.query ?? 'Search'}</button>;",
166+
"}",
167+
}, "\n"))
138168
writeFile(t, filepath.Join(dir, "packages/api/src/lib/legal-roster.ts"), strings.Join([]string{
139169
"export async function getLegalRoster(db: Db) {",
140170
" const rows = await db.user.findMany({ where: { active: true } });",
@@ -156,6 +186,8 @@ func TestDefensiveBroadeningSkipsUIBoundsAndInternalORMReads(t *testing.T) {
156186
report := runQualityPrecisionScan(t, qualityPrecisionConfigForLanguage(dir, "typescript"))
157187

158188
assertCodeQualityRuleAbsentForPath(t, report, "defensive.bounds-assumption", "relationship-tree.tsx")
189+
assertCodeQualityRuleAbsentForPath(t, report, "defensive.null-assumption", "use-filter-state.tsx")
190+
assertCodeQualityRuleAbsentForPath(t, report, "defensive.unvalidated-boundary-input", "use-filter-state.tsx")
159191
assertCodeQualityRuleAbsentForPath(t, report, "defensive.missing-resource-limit", "legal-roster.ts")
160192
assertCodeQualityRulePresentForPathWithMessage(t, report, "defensive.missing-resource-limit", "search-tools.ts", "resource limit")
161193
assertCodeQualityRuleAbsentForPath(t, report, "defensive.missing-resource-limit", "search-tools.ts:4")

0 commit comments

Comments
 (0)